Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure composer not full size when onyx state missing #31602

Merged
merged 3 commits into from
Nov 21, 2023
Merged

fix: ensure composer not full size when onyx state missing #31602

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac7a0c5
fix: ensure composer not full size when onyx state missing
erquhart Nov 21, 2023
281a940
drop deeplink handler early return for empty routes
erquhart Nov 21, 2023
ef75931
improve getReportID() solution comment
erquhart Nov 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2002,6 +2002,9 @@ function openReportFromDeepLink(url, isAuthenticated) {
Session.waitForUserSignIn().then(() => {
Navigation.waitForProtectedRoutes().then(() => {
const route = ReportUtils.getRouteFromLink(url);
if (!route) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good additional fix but I am concerned this will cause another unexpected regression we don't know.
If route is falsy, it redirects to ROUTES.HOME as fallback which is fine.
Let's just fix OP here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It redirects after nav state hydration has already occurred here in the default report setter:

App/src/libs/Navigation/AppNavigator/ReportScreenIDSetter.js

Lines 85 to 91 in 04b5fa4

// If there is no reportID in route, try to find last accessed and use it for setParams
const reportID = getLastAccessedReportID(reports, !canUseDefaultRooms, policies, isFirstTimeNewExpensifyUser, lodashGet(route, 'params.openOnAdminRoom', false));
// It's possible that reports aren't fully loaded yet
// in that case the reportID is undefined
if (reportID) {
navigation.setParams({reportID: String(reportID)});

I'm finding that just fixing the onyx key still leaves a flash that's too fast to catch on video, but it's observable if you try to repro with this branch. It isn't the composer size, so it's technically out of scope for this issue - happy to remove if you prefer. But resetting nav state twice, which only happens on wide screens loading the root domain with no path, is causing problems, we really shouldn't be doing it. It's covered in step 4 of the root cause in my proposal.

Copy link
Contributor Author

@erquhart erquhart Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is a lot of UI conditionals hinge on whether there's a report id. When you load the root domain, the default report setter provides a report id, but then the deep link handler redirects, blowing up nav state and taking report id back to an empty string. This leaves a gap where various components are rendering when there's no report ID.

That happens earlier in the root cause before getReportID() fails to return "null", and is technically a more accurate root cause.

getReportID() handling empty report ids is just a backup to provide default values. Both fixes are accurate, and both are related. We're counting on a lot of default value handling across a lot of components to cover this, but there's no need for the double redirect in the first place.

I'm pretty confident that not only will this not cause more bugs, it will likely fix some seemingly unrelated issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will likely fix some seemingly unrelated issues

I'd suggest to raise this in slack with clear repro step and video of the buggy behavior and let team make create new GH

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'll update the PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate you digging into this and trying to fix the problem. I agree that it would be great to handle that in a separately scoped issue and PR. That makes everything easier to review/test/revert.

if (route === ROUTES.CONCIERGE) {
navigateToConciergeChat(true);
return;
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/ReportScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ const defaultProps = {
*/
function getReportID(route) {
// // The reportID is used inside a collection key and should not be empty, as an empty reportID will result in the entire collection being returned.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// // The reportID is used inside a collection key and should not be empty, as an empty reportID will result in the entire collection being returned.
// The reportID is used inside a collection key and should not be empty, as an empty reportID will result in the entire collection being returned.

This has been driving me nuts for weeks 😂 mind fixing it? It's also great that the null default value completely went against the warning in the comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, also... you could maybe update the comment to explain why the 0 is outside of lodashGet() instead of using it as the default value of lodashGet. In fact, maybe I missed that part... why isn't the code like this?

lodashGet(route, 'params.reportID', 0)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'' is not falsy and doesn't fallback.
Similar in TS: '' ?? 0 returns ''

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, that's right. Can you please add that to the comment so that others will understand that too? It's very subtle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

return String(lodashGet(route, 'params.reportID', null));
return String(lodashGet(route, 'params.reportID') || 0);
}

function ReportScreen({
Expand Down
Loading