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

Blinking on chat switching #39040

Merged
merged 3 commits into from
Mar 28, 2024
Merged

Blinking on chat switching #39040

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9a2561c
fix ID
perunt Mar 27, 2024
5724807
move the comment to the top so it's more noticeable
perunt Mar 28, 2024
8ec3050
update comment
perunt Mar 28, 2024
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
5 changes: 5 additions & 0 deletions src/pages/home/report/ReportActionsView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,12 @@ function ReportActionsView({
setCurrentReportActionID('');
}, [route]);

// Change the list ID only for comment linking to get the positioning right
const listID = useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@perunt Would you kindly add a comment above the listID declaration describing its role and why it is passed down to the FlatList component as (React) key instead of passing it to the existing extraData array here ?

I also asked in #38838 (comment) as part of #38838 (issue).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey! Definitely, I'll put my comment up top so it's easier to spot. Thanks!

So, here's the deal: I need to make the whole list refresh itself from scratch. It's all about the virtualization thing the list does. Basically, this refresh trick is mostly needed when you're linking comments, especially when jumping from looking at a bunch of messages to just a few. The virtualization in FlatList gets a bit wonky when the amount of stuff shown shrinks as you move to a specific message. So, to keep everything looking right, we gotta refresh the whole list, not just poke the FlatList to update what it's already showing. So if we put it to extraData, the comment linking would lead to the wrong place in some specific cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me check your links, will also leave this info there

if (!reportActionID) {
// Keep the old list ID since we're not in the Comment Linking flow
return listOldID;
}
Comment on lines +137 to +140
Copy link
Contributor

Choose a reason for hiding this comment

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

We missed the case where we switch from a report with comment linking to a report without comment linking, in this case we shouldn't use the same list id

  • reportActionID: 123 -> reportActionID: 555 => Use different id
  • reportActionID: 123 -> reportActionID: null => Use different id
  • reportActionID: null -> reportActionID: 123 => Use different id
  • reportActionID: null -> reportActionID: null => Use same id

(Coming from #45004)

isFirstLinkedActionRender.current = true;
const newID = generateNewRandomInt(listOldID, 1, Number.MAX_SAFE_INTEGER);
listOldID = newID;
Expand Down
Loading