[codex] Fix suite session recovery - #98
Conversation
![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: c5c71d265c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| }; | ||
|
|
||
| const snapshot = readSnapshot(resolveStorage('sessionStorage')) || readSnapshot(resolveStorage('localStorage')); |
There was a problem hiding this comment.
Select between both mirrors using validity and lastUpdate rather than always preferring sessionStorage. If a later sessionStorage.setItem fails (for example because that backend reaches quota) while the corresponding localStorage write succeeds, the old session value remains present and this expression restores it, silently rolling back newer drafts, completed results, and the active passage even though the fallback contains the current snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Still reproducible on 196aa17. Snapshot selection is only as fresh as session.lastUpdate, but the new SIMULATION_DRAFT_SYNC path persists changed drafts without advancing it. If sessionStorage retains the old draft and rejects the next write while localStorage accepts it, both snapshots have the same timestamp; the stable sort keeps sessionStorage first and restores the old answer. Please advance a monotonic revision/lastUpdate on every persisted mutation and cover the equal-timestamp case. Keeping this open.
| status: status === 'initializing' ? 'active' : status, | ||
| startTime: Number(snapshot.startTime) || Date.now(), | ||
| sequence, | ||
| currentIndex: activeIndex >= 0 ? activeIndex : sequenceIndex, |
There was a problem hiding this comment.
Do not always let activeExamId override currentIndex when restoring an auto-advancing suite. After a passage is submitted, handleSuitePracticeComplete increments and mirrors currentIndex before awaiting cleanup, but activeExamId remains the just-completed passage until _advanceSuiteToNext runs; a refresh or crash during that await therefore restores the completed passage rather than the next one and lets the user reopen it. Persist the target atomically or prefer the advanced index when the active entry already has a result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The non-terminal case is fixed, but the terminal state is still reproducible on 196aa17: a snapshot with currentIndex === sequence.length, activeExamId === p3, and a stored P3 result is clamped to length - 1 before the completed-result logic runs. Restore returns index 2 and resumeSuitePractice() opens P3 again. Please preserve a terminal/finalizing state and resume finalization instead of reopening the completed passage. Keeping this open.
| if (typeof this._recordSuiteCloseAttempt === 'function') { | ||
| this._recordSuiteCloseAttempt(suiteSession, 'window_closed'); | ||
| } else if (typeof this._mirrorSessionToStorage === 'function') { | ||
| this._mirrorSessionToStorage(suiteSession); |
There was a problem hiding this comment.
When the active suite uses classic or stationary flow and the user closes the child window before submitting or navigating, this branch only mirrors the draft already held by the parent. Those flows do not emit the simulation-only periodic SIMULATION_DRAFT_SYNC, so recent answers, highlights, and notes remain solely in the child DOM and are destroyed on close; resuming then opens a blank passage despite the message claiming progress was saved. Capture a final draft during the child unload lifecycle or avoid reporting this session as fully recoverable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Still not fixed for the unified-reading production path. reading-practice-unified.html loads reading-page.bundle.js, not practice-page-enhancer.bundle.js; unifiedReadingPage only syncs drafts while state.simulationMode is true and registers no unload/visibility final-sync handler. Closing a classic or stationary unified-reading passage therefore never reaches the new sender. Please add the close-time sync to the unified runtime (or a shared lifecycle) and cover that path. Keeping this open.
k-undurkhaan-2
left a comment
k-undurkhaan-2
left a comment
There was a problem hiding this comment.
Found three additional recovery regressions not covered by the current tests: a stale close watcher can detach a newly resumed window, a permanently invalid snapshot can trap suite mode in an endless retry loop, and the close path reports persistence success even when neither storage backend accepted the snapshot. Details are inline.
|
已经针对审计出的八个comment进行了修复并提交，请PR owner在本地环境执行对抗审计 @dijia702 |
196aa17 to
738de54
Compare
3 participants
Summary
sessionStorageorlocalStorageand reopen the current passage after a main-page refresh.suiteSessionIdacross navigation andfile://fallback paths.Root cause
The parent page only mirrored suite state after later events, while child-window closure called
_abortSuiteSession, which cleared the persisted snapshot. Refreshes andfile://storage differences could therefore lose the active suite.Validation
file://suite E2E flow passes.The repository's existing static CI baseline still reports unrelated failures in CSS convergence, settings-button coverage, on-demand test mocks, service-facade CSS checks, and IndexedDB shadow cleanup.