Filter task search rows by live report status instead of the stale snapshot - #100069
Conversation
…e snapshot Co-authored-by: Bruno Rocha <brunovjk@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 Android test steps verification results: ✅ pass On standalone Android NewDot, completing a task from the Search results list immediately removes it from the Type: Task / Status: Outstanding list, and both completed tasks then appear under Status: Completed with a non-interactive "Completed" badge instead of an enabled Complete button. No JS errors occurred during the flow.
Notes on scope: verified on the standalone Android NewDot developmentDebug build of |
|
🤖 Web test steps verification results: ✅ pass On dev NewDot web (branch
Setup detail worth knowing if this gets re-run: tasks had to be created from a chat composer ( |
|
Great work, @MelvinBot! I tested this locally, and everything is working as expected. Please mark the PR as ready for review, I'm finishing up recording the videos for all platforms. Complete the author checklist by checking all items, and update the Test and QA steps. One more thought: do you think we should add test coverage for reopening tasks and negated status filters in this PR, or is the current coverage sufficient? Thanks. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp100069_android_native.movAndroid: mWeb Chrome100069_android_web.moviOS: HybridApp100069_ios_native.moviOS: mWeb Safari100069_ios_web.movMacOS: Chrome / Safari100069_web_chrome.mov |
Co-authored-by: Bruno Rocha <brunovjk@users.noreply.github.com>
|
Thanks for testing it locally! All three items are done, and yes — I think the extra coverage was worth adding, so I've added it. Done:
On the test coverage question — yes, both are worth covering, and the negated case especially. I've pushed 5 new tests in
Both were cheap — same fixture shape as the existing block, no new helpers — so there was no reason to defer them to a follow-up. Verification
New tests:
One thing to confirm on your side: I checked the all platforms and screenshots/videos boxes based on your local testing, but the |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return result; | ||
| }); | ||
| }) | ||
| // Drop tasks whose live status no longer matches the active `status:` filter — the snapshot still lists a |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
This newly added comment uses an em dash (—) in its own sentence. Comments should read as plain, natural sentences without em dashes.
Split it into two sentences instead:
// Drop tasks whose live status no longer matches the active `status:` filter. The snapshot still lists a
// just-completed task under `status:outstanding` because `completeTask` never writes to it.
.filter((task) => isEligibleForTaskStatus(currentQueryJSON, task));Reviewed at: d9d0aad | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| * search snapshot. Completing or reopening a task does not patch the snapshot, so without this a completed task | ||
| * lingers under `status:outstanding` (and vice versa) until the next server fetch. Mirrors `isEligibleForStatus`. | ||
| */ | ||
| function isEligibleForTaskStatus(currentQueryJSON: SearchQueryJSON | undefined, report: OnyxEntry<OnyxTypes.Report> | SearchTask) { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
isEligibleForTaskStatus is a near line-for-line duplicate of the existing isEligibleForStatus: both read the STATUS filter, return true when empty, branch on status.isNegated iterating Object.keys(<mapping>).some(...) with an isExcluded check, then fall through to status.value.some(...). The accompanying taskStatusActionMapping/isValidTaskStatus also mirror expenseStatusActionMapping/isValidExpenseStatus. The only real differences are the mapping object and the validator.
Consider extracting the shared control flow into one helper parameterized by the mapping and validator, e.g.:
function isEligibleForStatusFilter<T>(
currentQueryJSON: SearchQueryJSON | undefined,
report: T,
mapping: Record<string, (report: T, ...rest: never[]) => boolean>,
isValidStatus: (status: unknown) => boolean,
...predicateArgs: unknown[]
) { /* shared branch/negation/some logic */ }so isEligibleForStatus and isEligibleForTaskStatus both delegate to it instead of copying the branching.
Reviewed at: d9d0aad | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const staleCreatorID = 121212; | ||
| const staleAssigneeID = 131313; | ||
|
|
||
| // The snapshot still says the task is open — this is exactly what `completeTask` leaves behind, |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
This newly added comment uses an em dash (—) in its own sentence. Rewrite it as two plain sentences:
// The snapshot still says the task is open. This is exactly what `completeTask` leaves behind,
// because it only writes to `report_<taskID>` and never patches `snapshot_<hash>`.Reviewed at: d9d0aad | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const reopenedCreatorID = 141414; | ||
| const reopenedAssigneeID = 151515; | ||
|
|
||
| // Mirror image of the complete case: `reopenTask` writes only to `report_<taskID>`, so the snapshot is |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
This newly added comment uses an em dash (—) in its own sentence. Rewrite it without the em dash:
// Mirror image of the complete case. `reopenTask` writes only to `report_<taskID>`, so the snapshot is
// left claiming the task is still completed.Reviewed at: d9d0aad | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9d0aad2ac
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }) | ||
| // Drop tasks whose live status no longer matches the active `status:` filter — the snapshot still lists a | ||
| // just-completed task under `status:outstanding` because `completeTask` never writes to it. | ||
| .filter((task) => isEligibleForTaskStatus(currentQueryJSON, task)); |
There was a problem hiding this comment.
When a paginated task search has more results, completing or reopening any loaded task makes this filter shrink tasks.length, which is returned as allDataLength. In Search.fetchMoreResults, the guard offset > allDataLength - RESULTS_PAGE_SIZE then remains true (for example, 49 displayed rows at offset 0 after completing one row from a 50-row page), so onEndReached can no longer request subsequent pages even though hasMoreResults is true. Keep filtering the rendered rows, but return the snapshot's pre-filter task count for pagination.
Useful? React with 👍 / 👎.
|
The failing check is unrelated to this PR — I have not pushed anything.
It's a date-dependent test that breaks on the 2nd of the month. Today is the 2nd, so it is failing repo-wide right now, not just here. Every other
Root cause and suggested fixThe assertion compares two different "next nth of month" implementations that disagree only when today's day-of-month equals the offset. The test builds its expectation from The production path at There is no Worth noting the The durable fix belongs in I left this alone deliberately — I don't fix unrelated or flaky failures on someone else's test. It should go green on its own tomorrow; a re-run today will keep failing. If you'd like, I can open a separate issue to pin the clock in that test. |
|
Re-checked — nothing has changed, and I still have not pushed anything. There is no new CI run; I did find one more piece of evidence that settles it. This branch passed the same job yesterday, and the only file that changed between the passing and failing runs is this PR's own search test:
The other red check, Root cause, unchanged from my previous commentThe assertion compares two different "next nth of month" implementations that disagree only when today's day-of-month equals the offset. The test builds its expectation from The production path at There is no I'm deliberately leaving this alone: it's an unrelated, date-flaky test in someone else's file, and folding a fix for it into this PR would put an unrelated change in front of your reviewer. Happy to open a separate issue or PR to pin the clock in |
|
Re-checked — nothing changed, and still no fix to push here. But I found the actual blocker, which is new since my last comment. The fix for this flake already exists and was closed by mistake: #100145. It was closed at 12:29 UTC on the belief that it had been "already fixed in a different PR." It hasn't been. I just checked
So Nothing on this PR needs changing. The two red checks are:
Root cause (re-verified this run)The test compares two different "next nth of month" implementations that disagree only when today's day-of-month equals the offset. Today is the 2nd, and the offset is 2. The test expects I checked this run whether the I also confirmed there is no Evidence it is not this PR: this PR touches only I'm deliberately not pushing a fix — I don't patch an unrelated repo-wide flake onto someone else's PR, and the correct fix is already written in #100145. Re-running CI today will not help; it goes green on its own tomorrow. |
|
Still nothing to push here — but there is real news this time: CI actually re-ran That settles the open question from my earlier comments. The previous result was stale; this one is not. Job 100329779349 ran at ~16:26 UTC on the same head
The unblock path is unchanged and is not in this PR. I re-verified all three just now:
So Root cause — unchanged, see my earlier comment for the full write-upToday is the 2nd and the test's offset is 2, which is the one day a month where the test's expectation and the production code disagree. The test derives its expected value from Full detail, including the four unrelated branches that failed the same test today: my earlier comment. I'm holding the same line as before: I don't patch an unrelated repo-wide flake onto your feature PR, especially when the correct fix is already written and just needs reopening. If you'd rather I reopen #100145 myself, or push the clock-pinning change onto a fresh branch of its own, say which and I'll do it. |
|
I believe the test failure warning is unrelated to this PR. Thanks. |
|
I don't think this needs product review. |
|
🚧 youssef-lr has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/youssef-lr in version: 9.4.76-0 🚀
|
|
🤖 No help site changes are required for this PR — no draft docs PR was created. This PR is a client-side bug fix: task rows in Search are now filtered against the live report status instead of the stale search snapshot, so a completed task drops out of Articles I checked and why each stays as-is
The two changed files are Separate, pre-existing wording nits (not caused by this PR, so I left them alone): @brunovjk, no linked help site PR exists because I found nothing that needs updating. If you think something here should be documented anyway — for example the Complete button on task rows in Search — tell me what you want covered and I'll open the draft PR with the |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.76-1 🚀
Bundle Size Analysis (Sentry): |
4 participants
Explanation of Change
Completing a task from Search > Task > Outstanding left the row in the list.
The Task search list renders straight from the cached search snapshot (
snapshot_<hash>).completeTaskonly merges intoreport_<taskID>andreportActions_<taskID>— it never touches the snapshot — so the row kept the snapshot's stalestatusNum/stateNumand nothing removed it from thestatus:outstandinglist. Nothing refetched either:useSearchHighlightAndScrollonly re-runs the search fortype:chator transaction changes.Expenses don't have this bug because
getTransactionsSectionsre-filters fetched rows against the live report viaisEligibleForStatus. Tasks had no equivalent — the outstanding/completed split was purely server-side, baked into the query hash.This PR gives tasks the same client-side re-filter:
taskStatusActionMapping+isEligibleForTaskStatus, mirroring the existingexpenseStatusActionMapping/isEligibleForStatuspair.getTaskSectionsnow takesqueryJSON(falling back togetCurrentSearchQueryJSON(), same asgetTransactionsSections) and drops rows whose live report status no longer matches the activestatus:filter.statusNum/stateNumnow come from the live report rather than the snapshot. This also fixes a second symptom:isTaskCompletedinTaskListItemRowread the stale snapshot copy, so the row kept rendering an enabled Complete button instead of the Completed badge.Because the guard is status-based rather than direction-based, it covers
reopenTask(a reopened task disappearing from Completed) with no extra code. It is entirely client-side, so it works offline.An unrecognized status value (for example a hand-typed
status:all) leaves rows visible rather than silently emptying the list.Fixed Issues
$ #99923
PROPOSAL: #99923 (comment)
AI Tests
Run locally by MelvinBot on this branch:
npm run typechecknpx eslinton the changed filesSearchUIUtils.ts, none new)npm test -- tests/unit/Search/SearchUIUtilsTest.tsnpm run spell-changedEight unit tests were added to
tests/unit/Search/SearchUIUtilsTest.ts. Each seeds a snapshot task in one status while the live Onyx report is in the other — exactly the statecompleteTask/reopenTaskleave behind — and asserts the row is filtered against the live status:status:outstanding, stays instatus:completedwith live status values, and a still-open task is unaffected.status:completedand appears understatus:outstandingreporting its liveOPENstatus.-status:completeddrops a completed task and keeps an open one, and-status:outstandingkeeps a completed task.statusis negatable in the search grammar, so these queries are reachable from the search router.Not run:
npm run react-compiler-compliance-check check-changedfailed on an environment issue (Could not get commit hash for origin/). It is not applicable here — this PR changes a lib file and a test, no components or hooks.End-to-end verification was completed on both web and standalone Android NewDot (see the two verification comments below), and the reviewer confirmed the fix locally.
Tests
+(composer menu) > Assign task. Create a task, setting Assignee to yourself. Repeat so you have two self-assigned tasks. (Note: the global FAB create menu has no task entry — tasks must be created from a chat composer.)type:task status:outstanding.status:completed) and verify the reopened task's row disappears from the Completed list immediately.status:filter (type:task) lists both outstanding and completed tasks, each rendering the correct action (Complete button vs Completed badge).Offline tests
The re-filter is entirely client-side and reads from Onyx, so it works with no network.
type:task status:outstandingso the snapshot is cached.report_<taskID>is enough, no server round trip is needed.CompleteTaskrequest flushes — the row does not reappear under Outstanding.QA Steps
+(composer menu) > Assign task. Create two tasks, setting Assignee to yourself on each.type:task status:outstanding.status:completed) and verify the reopened task's row disappears from the Completed list immediately.type:taskwith no status filter and verify both outstanding and completed tasks are listed, each with the correct action (Complete button vs Completed badge).PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari