Skip to content

Fix the new message button doesn't work on some comment linkings #46627

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

Merged
francoisl merged 17 commits into from
Aug 12, 2024
Merged

Fix the new message button doesn't work on some comment linkings #46627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
00a7bed
Fix the new message button doesn't work for some comment linkings
tsa321 Aug 1, 2024
69ce490
Merge branch 'main' into fixNwMessageBtnCmntLnkng
tsa321 Aug 1, 2024
4987917
Fix pagination util prev page
tsa321 Aug 1, 2024
76f9489
Using one source of route for report screen, view and list
tsa321 Aug 2, 2024
7036096
using one source of route in report screen, view and list and fixees …
tsa321 Aug 2, 2024
4b4b369
Merge branch 'main' into fixNwMessageBtnCmntLnkng
tsa321 Aug 2, 2024
3235453
revert index of linked action search
tsa321 Aug 2, 2024
ceae85f
Fix typechek error
tsa321 Aug 2, 2024
a9d1374
Fix perf-test error
tsa321 Aug 2, 2024
02fca25
Change method to get last item of result
tsa321 Aug 5, 2024
da00fa5
Fix lint
tsa321 Aug 5, 2024
19844ab
Merge branch 'main' into fixNwMessageBtnCmntLnkng
tsa321 Aug 5, 2024
7553ed6
Revert route scheme
tsa321 Aug 6, 2024
abae889
Revert perf-test
tsa321 Aug 6, 2024
c498c92
Fix commment linking jumping around and new message button in android
tsa321 Aug 8, 2024
0993bd4
Revert changes
tsa321 Aug 10, 2024
c3bc9d1
Merge main
tsa321 Aug 10, 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
2 changes: 1 addition & 1 deletion src/libs/PaginationUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ function mergeAndSortContinuousPages<TResource>(sortedItems: TResource[], pages:
const result = [sortedPages[0]];
for (let i = 1; i < sortedPages.length; i++) {
const page = sortedPages[i];
const prevPage = sortedPages[i - 1];
const prevPage = result[result.length - 1];

@s77rt s77rt Aug 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

prepage (result[result.length - 1]) could be same as page if the result contains only one page

@tsa321 tsa321 Aug 5, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@s77rt What? I don't understand. Could you elaborate please?
Do you mean when the sortedPages.length is 1. If it is 1 it won't even enter the for loop.

@s77rt s77rt Aug 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If result length is 1, then:
page = sortedPages[0]
and
prevPage = sortedPages[0]

page is same as prevPage which is misleading and could cause bugs

@tsa321 tsa321 Aug 5, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@s77rt Maybe I still don't understand.
The i in the for loop starts at 1, not 0, so page = sortedPages[0] is not possible. Consequently, page and prevPage won't be the same.

Am I missing something?

const prevPage = result[result.length - 1]; Here I am using the last updated page of result and not the sortedPages.
The end of result could be only 1 continuous page for for example 10 pages in sortedPages. Because the function merge the pages into one continuous page.

@s77rt s77rt Aug 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah right! my bad


// Current page is inside the previous page, skip
if (page.lastIndex <= prevPage.lastIndex && page.lastID !== CONST.PAGINATION_END_ID) {
Expand Down
3 changes: 2 additions & 1 deletion src/pages/home/report/ReportActionsView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ function ReportActionsView({
const reactionListRef = useContext(ReactionListContext);
const route = useRoute<RouteProp<AuthScreensParamList, typeof SCREENS.REPORT>>();
const reportActionID = route?.params?.reportActionID;
const prevReportActionID = usePrevious(reportActionID);
const didLayout = useRef(false);
const didLoadOlderChats = useRef(false);
const didLoadNewerChats = useRef(false);
Expand Down Expand Up @@ -141,7 +142,7 @@ function ReportActionsView({

// Change the list ID only for comment linking to get the positioning right
const listID = useMemo(() => {
if (!reportActionID) {
if (!reportActionID && !prevReportActionID) {
// Keep the old list ID since we're not in the Comment Linking flow
return listOldID;
}
Expand Down