[No QA] Add SEQUENTIAL_QUEUE.md#93279
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@arosiclair 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] |
|
@codex review |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
There was a problem hiding this comment.
Pull request overview
Adds an internal reference document describing the SequentialQueue subsystem (offline-first serialized WRITE queue), intended as a sibling to existing architecture docs like NETWORK_STATE_DETECTION.md.
Changes:
- Added
contributingGuides/SEQUENTIAL_QUEUE.mddocumenting SequentialQueue architecture, lifecycle, collaborators, edge cases, and test “contract” references. - Updated
cspell.jsonto whitelist new technical terms used by the documentation (beforeunload,reauthenticating).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
cspell.json |
Adds new allowed terms needed for the new guide’s vocabulary. |
contributingGuides/SEQUENTIAL_QUEUE.md |
New in-depth subsystem documentation for SequentialQueue behavior, boundaries, and known sharp edges/open questions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. 🎉 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". |
|
@MelvinBot review this PR for being true to reality. Can you confirm all the claims from the new document? |
|
Let me also update the PR since #91734 got merged over the weekend. |
The QueuedOnyxUpdates not-signed-in filter preserves 15 keys, not 14. Also name the focus-mode NVP and the three RAM-only flags explicitly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR Expensify#91734 made push() async and await the Onyx disk commit before flushing, closing the enqueue crash window the doc centred on. Rewrite the affected sections to current reality: - push() awaits persistencePromise before flush(false); enqueue write is now gated ahead of the network (was: fired off in-memory state) - redraw the disk/network ordering diagram (commit precedes network) - isReadyPromise mechanism: starts resolved, armed via setIsReadyPromisePending(); add isReadyPromisePending (7 state vars) - four resolveIsReadyPromise sites (finally + all-empty/not-leader guards + offline-during-persist in push), not just the finally - persist .catch handlers now Log.alert (storage emergency) - note new SequentialQueueTest coverage; clarify persist-before-fire vs persist-before-optimistic (the latter still open) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@mountiny as you can read in the PR explanation there are some open questions for now that I couldn't address yet/guess:
|
|
@MelvinBot Can you do a thorough review of this PR? |
|
@codex review |
Review —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99ac5490e6
ℹ️ 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".
- getLength() does not gate read-blocking: drop the claim; waitForWrites
always awaits waitForSequentialQueueIdle() (isReadyPromise) regardless
of count (MelvinBot)
- Reauthentication throws new Error('Failed to reauthenticate') (MelvinBot)
- Qualify persist-before-fire to post-init only; pre-init save() returns
Promise.resolve() and the enqueue write is deferred + un-awaited (Codex P2)
- isQueuePaused is a data-gap/deferred-update pause only, not offline;
offline is a separate isOfflineNetwork() check. Add Open Question #6 on
the misleading source comment (Codex P3)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@MelvinBot thanks — both confirmed and fixed in
|
|
I think there are now more and more open questions in the doc that need to be addressed before merging. |
Fold the persistWhenOngoing, dead !onyxUpdates guard term, stale "must be last" comment, and inaccurate isQueuePaused offline comment into their relevant blocks as present-tense facts. Open Questions now holds only the two items that genuinely need a maintainer to ratify: silent give-up data loss, and knownOngoingRequestIDs sufficiency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both remaining items were "the code clearly shows what happens; only whether it's intended was unclear." Under document-AS-IS, that intent speculation comes out: the behaviors now stand as plain observations in the Error Handling and PersistedRequests sharp edges. Drop the section and its inbound references (intro, Contents, two inline links). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove the "Relationship to Other Docs" section (the Overview already links the two sibling docs), slim "Key Modules Reference" to a terse code-location index with section links, and simplify the architecture diagram to a high-level map (drop middleware names, guard-ordering lists, and the disk-write legend that the disk section already covers). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Resolved all threads I've been interested in, the doc reads well for me. |
|
LGTM, just a few more Codex findings: [P2] SEQUENTIAL_QUEUE.md#L5 overstates the delivery guarantee. The overview says every change is delivered “once” / exactly once, but the doc itself later describes duplicate-send windows during mid-flush leadership changes and crash/retry edges. The code also relies on retry/re-drive behavior, not a client-side exactly-once lock. I’d rephrase this as “sent serially and deduplicated/reconciled where needed” rather than “delivered once.” [P2] SEQUENTIAL_QUEUE.md#L92, #L132, and #L329 describe push() arming isReadyPromise before all offline early-outs, but the code does not. In push(), if the app is already offline, it awaits persistence and returns before setIsReadyPromisePending() is called. The doc should distinguish “offline at entry” from “goes offline during the awaited persist.” Otherwise readers may think offline writes arm and release the READ gate, when they actually leave it untouched. [P3] SEQUENTIAL_QUEUE.md#L105 and #L147 conflict on whether a crash can leave the same request both queued and ongoing. The restart section says that state can happen and explains head dedupe; the PersistedRequests section says the atomic multiSet prevents it. If dedupe is only defensive/legacy protection, the doc should say that. If the duplicate state is still possible, line 147 should be softened. |
@mountiny
Explanation of Change
Adds
contributingGuides/SEQUENTIAL_QUEUE.md, an observational reference document for the SequentialQueue subsystem — the offline-first WRITE queue that serialises every mutating API call.Documentation-only: the new doc plus one
cspell.jsonword entry. No runtime code changes.The doc mirrors the style and scope of the sibling
NETWORK_STATE_DETECTION.md(problem → how it works today → sharp edges per block) and refers to code by module and function names, not line numbers, so it survives refactors. It covers:SequentialQueue) and its 7-variable state machinePersistedRequests— the durable store and its in-memory-authoritative inversion patternpush()awaits the Onyx disk commit before flushing, the remaining fire-and-forget persist windows, and thenull/MemoryOnlyProvider/retry caveats on the commit handleRequestThrottle— jittered exponential back-off and the give-up signalQueuedOnyxUpdates/queueFlushedData— the two anti-flicker buffersAPI.write/API.read/makeRequestWithSideEffectspublic contract and the inbound consumers that drive the queueFixed Issues
$ #93422
PROPOSAL:
Tests
contributingGuides/SEQUENTIAL_QUEUE.mdin a Markdown viewer (GitHub preview, VS Code, etc.) and verify headings, code blocks, and internal anchor links render correctly.contributingGuides/that already exist).Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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