Persist thread read state across refreshes, ports, and browsers#17
Open
SHAREN wants to merge 1 commit intofriuns2:mainfrom
Open
Persist thread read state across refreshes, ports, and browsers#17SHAREN wants to merge 1 commit intofriuns2:mainfrom
SHAREN wants to merge 1 commit intofriuns2:mainfrom
Conversation
Review Summary by QodoPersist thread read state across refreshes, ports, and browsers
WalkthroughsDescription• Persist thread read state to shared machine-local store • Merge shared state with browser-local state on load • Sync read timestamps across refreshes, ports, browsers • Compare timestamps by ordering to prevent regressions Diagramflowchart LR
A["Browser localStorage"] -->|load| B["Merge Read State"]
C["Shared Global Store"] -->|load| B
B -->|newer timestamp| D["readStateByThreadId"]
D -->|mark as read| E["commitReadState"]
E -->|save| A
E -->|persist| C
File Changes1. src/api/codexGateway.ts
|
Code Review by Qodo
1. Shared read-state gets pruned
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
1128
to
1132
| const activeThreadIds = new Set(flatThreads.map((thread) => thread.id)) | ||
| const nextReadState = pruneThreadStateMap(readStateByThreadId.value, activeThreadIds) | ||
| if (nextReadState !== readStateByThreadId.value) { | ||
| readStateByThreadId.value = nextReadState | ||
| saveReadStateMap(nextReadState) | ||
| commitReadState(nextReadState) | ||
| } |
There was a problem hiding this comment.
1. Shared read-state gets pruned 🐞 Bug ✓ Correctness
pruneThreadScopedState prunes readStateByThreadId to only the currently loaded threads and now persists that pruned map to the shared store via commitReadState(). Since thread/list is limited to 100 threads, this deletes read timestamps for older threads from the shared store and can resurrect unread dots when those threads reappear in the top 100.
Agent Prompt
## Issue description
`pruneThreadScopedState()` prunes `readStateByThreadId` to only the currently loaded thread IDs and then calls `commitReadState()`, which persists that pruned map to the shared store. Because `/thread/list` is capped at 100 threads, this deletes read timestamps for threads outside the current page and defeats the goal of stable unread indicators.
## Issue Context
The UI only fetches 100 threads (`thread/list` limit), but the shared store should retain read timestamps beyond the current in-memory thread list so threads that re-enter the top 100 don't look unread again.
## Fix Focus Areas
- src/composables/useDesktopState.ts[1127-1132]
- src/composables/useDesktopState.ts[1081-1085]
- src/api/codexGateway.ts[93-99]
## Suggested fix
- Split "update local read-state" from "persist to shared store":
- Keep pruning for `localStorage`/memory if desired.
- Do **not** call `persistThreadReadState()` when pruning; only persist on positive events (e.g., `markThreadAsRead`, or after merging shared state).
- Alternatively, if you must persist after pruning, first merge with the existing shared store (server-side merge is preferable; see separate finding) so missing keys are not deleted.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+2219
to
+2228
| if (req.method === 'PUT' && url.pathname === '/codex-api/thread-read-state') { | ||
| const payload = await readJsonBody(req) | ||
| const record = asRecord(payload) | ||
| if (!record) { | ||
| setJson(res, 400, { error: 'Invalid body: expected object' }) | ||
| return | ||
| } | ||
| await writeThreadReadStateMap(normalizeThreadReadStateMap(record.state ?? record)) | ||
| setJson(res, 200, { ok: true }) | ||
| return |
There was a problem hiding this comment.
2. Read-state put overwrites map 🐞 Bug ⛯ Reliability
PUT /codex-api/thread-read-state replaces the entire stored map with the request body instead of merging with existing state. If multiple browsers/windows are open, a stale client can overwrite and delete other threads’ read timestamps (lost updates).
Agent Prompt
## Issue description
The shared read-state store is updated with last-write-wins semantics: the server overwrites `thread-read-state` with whatever map the client sends. With two UIs open, a client that hasn't synced recently can erase keys written by the other client.
## Issue Context
Read-state is inherently multi-writer (refreshes, ports, browsers). The server must be robust to out-of-order/stale updates.
## Fix Focus Areas
- src/server/codexAppServerBridge.ts[2219-2228]
- src/server/codexAppServerBridge.ts[537-560]
## Suggested fix
- On PUT, implement a merge with the existing persisted map instead of replacement:
- `const existing = await readThreadReadStateMap()`
- `const incoming = normalizeThreadReadStateMap(record.state ?? record)`
- For each `threadId`, keep the max(readAtIso) by timestamp ordering (ISO lexical compare is OK for `toISOString()`-style strings; otherwise parse and compare).
- Write the merged result.
- Avoid deleting keys that are absent from the incoming payload (treat PUT as upsert/merge, or change to PATCH semantics).
- Optionally return the merged map (or a version/etag) so clients can converge quickly.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+548
to
+560
| async function writeThreadReadStateMap(state: Record<string, string>): Promise<void> { | ||
| const statePath = getCodexGlobalStatePath() | ||
| let payload: Record<string, unknown> = {} | ||
| try { | ||
| const raw = await readFile(statePath, 'utf8') | ||
| payload = asRecord(JSON.parse(raw)) ?? {} | ||
| } catch { | ||
| payload = {} | ||
| } | ||
|
|
||
| payload['thread-read-state'] = normalizeThreadReadStateMap(state) | ||
| await writeFile(statePath, JSON.stringify(payload), 'utf8') | ||
| } |
There was a problem hiding this comment.
3. Global state write races 🐞 Bug ⛯ Reliability
Multiple endpoints update the same ~/.codex/.codex-global-state.json using independent read-modify-write cycles without serialization, so concurrent requests can clobber unrelated keys. Adding frequent thread-read-state writes increases the likelihood of losing workspace roots or thread title updates.
Agent Prompt
## Issue description
`~/.codex/.codex-global-state.json` is mutated by multiple request handlers (thread titles, workspace roots, and now thread read-state). Each handler performs an uncoordinated read-modify-write of the entire JSON file, allowing concurrent requests to overwrite each other and lose unrelated keys.
## Issue Context
This PR increases write frequency (mark-as-read + pruning persistence), making collisions more likely.
## Fix Focus Areas
- src/server/codexAppServerBridge.ts[401-402]
- src/server/codexAppServerBridge.ts[524-535]
- src/server/codexAppServerBridge.ts[548-560]
- src/server/codexAppServerBridge.ts[656-671]
## Suggested fix
- Introduce a single global-state write path guarded by a per-file async mutex/queue so writes are serialized.
- Perform atomic writes (write to a temp file then `rename`) to reduce partial-write corruption risk.
- Prefer an API like `updateGlobalState((draft) => { draft[key]=... })` that ensures all endpoint updates compose safely.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
Если вкратце, то это чтобы отметка о прочитанном сохранялась |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Persist per-thread read state outside browser-local storage so unread dots survive page refreshes, different local ports, and different browsers on the same machine.
Problem
Right now read/unread state is effectively browser-local because it lives in
localStorage.That creates a few user-visible problems:
In practice this means users see old unread indicators again and have to remember manually which threads they already checked.
What This PR Changes
GET /codex-api/thread-read-statePUT /codex-api/thread-read-state~/.codex/.codex-global-state.json) underthread-read-stateWhy This Approach
This keeps the current UX exactly the same while moving the source of truth for read state to a machine-local shared store that already exists for other Codex desktop/web persistence.
That makes unread markers stable across:
Scope
This is intentionally machine-local persistence only. It does not try to sync read state across different machines or accounts.
Testing
npm run buildGET /codex-api/thread-read-statereturns200 OK