feat: add explicit terminal state to search snapshot#96043
feat: add explicit terminal state to search snapshot#96043BartekObudzinski wants to merge 9 commits into
Conversation
| * Optional because snapshots persisted before this field existed (and snapshots written by | ||
| * non-search actions) may not carry it. | ||
| * | ||
| * TODO: nothing reads this field yet. The existing isLoading/type/status-based loading and error gates |
There was a problem hiding this comment.
this should already link to an issue, there's nothing that tracks it once we merge a TODO and follow-up PR to code.
There was a problem hiding this comment.
Agreed, a TODO without a link dies on merge. The tracking issue for the read side is being created, I will put the link in this TODO before the PR leaves draft.
There was a problem hiding this comment.
Went with removing the TODO marker for now since the consistency rule flags it. The follow-up is tracked on our side and the GitHub issue will be created together with the read side PR, I will link it here once it exists.
There was a problem hiding this comment.
Dropped the TODO since issues here get created on the fly and there is nothing to link yet. The follow-up is tracked on my side and lands in the next PR together with its issue
|
@codex review |
|
@parasharrajat Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * Optional because snapshots persisted before this field existed (and snapshots written by | ||
| * non-search actions) may not carry it. | ||
| * | ||
| * TODO: nothing reads this field yet. The existing isLoading/type/status-based loading and error gates |
There was a problem hiding this comment.
❌ CONSISTENCY-11 (docs)
This new TODO: comment describes deferred/future work ("nothing reads this field yet... migrate to read it in a follow-up PR"). Per contributingGuides/philosophies/OVERENGINEERING.md, future work should be tracked in a GitHub issue rather than an in-code TODO, which is invisible to planning and tends to rot silently in the codebase.
Remove the TODO marker and capture the follow-up (wiring the read side to consume state) as a GitHub issue. You can keep the explanatory prose without the TODO token:
/** Explicit terminal lifecycle state of the most recent search request for this snapshot.
* Optional because snapshots persisted before this field existed (and snapshots written by
* non-search actions) may not carry it.
*
* Nothing reads this field yet; the existing isLoading/type/status-based loading and error gates
* are migrated to read it separately (tracked in a GitHub issue).
*
* Residual limitation: if the app is killed or reloaded mid-request, no cleanup runs, so `loading` can
* still be stranded on disk. The future read side must treat a `loading` state with no in-flight request
* as stale. */
state?: ValueOf<typeof CONST.SEARCH.SNAPSHOT_STATE>;Reviewed at: d8febfa | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Removed the TODO marker, the prose stays as a plain description. The read side migration is tracked on our side and will get its own GitHub issue when that PR starts.
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
Explanation of Change
Today, whether a search snapshot has finished loading is inferred from a mix of fields: whether
dataisundefined, theisLoadingflag, and whethertype/statusmatch the current query. This breaks when a request resolves with a 200 response but writes nodataat all: nothing marks the snapshot as done, so the page is stuck showing a loading state with nothing left to re-drive it. This is a live production bug.Search requests are read commands, so they never enter the offline request queue. That means the snapshot itself has to own its own lifecycle instead of relying on the queue to retry or resolve it.
This PR adds an explicit
statefield (loading/loaded/error) to the search snapshot.getOnyxLoadingDatanow writesloadingoptimistically when a request starts, and a newsuccessDatabranch writesloadedon any successful (jsonCode 200) resolve, even one with no snapshot data.failureDatawriteserror, andfinallyDataintentionally leavesstatealone since it runs after success/failure and would otherwise overwrite theerrorresult. The field is optional so snapshots persisted before this change stay valid, and the writes only apply to real search requests (not the other two callers of this helper) so they can't get stuck inloadingwith no terminal write to clear it.This PR is write side only. It does not change any UI or the existing loading/error gates. Those will move over to read the new field in a follow up PR.
Fixed Issues
$ #96061
PROPOSAL:
Tests
Offline tests
N/A — the search request never fires while offline (
useSearchPageSetupreturns early onisOffline), so there is no offline behavior to exercise.QA Steps
Same as tests
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