feat: sync thread read state across devices - #6078
Conversation
Opening a finished thread now stamps a server-synced lastVisitedAt via a new thread.visit command, so reading a thread on web clears the unread cue on mobile and vice versa. Local storage stays as the offline and old-server fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:trusted
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| // Version skew: never send thread.visit to a server that predates it. | ||
| useEffect(() => { | ||
| if (selectedThread === null) return; | ||
| const supportsThreadVisits = |
There was a problem hiding this comment.
🟡 Medium threads/ThreadRouteScreen.tsx:228
The thread.visit effect reads environmentServerConfigsAtom imperatively via appAtomRegistry.get(...), so it never re-runs when the server config arrives later. When a cached thread is already present but the connection's server config is still null at mount, the effect returns early at the capability check; once the config loads with threadVisits: true, neither selectedThread nor visitThreadMutation has changed, so the effect does not re-run and the finished thread is never acknowledged as seen. The server config should be read through a reactive hook and the derived supportsThreadVisits flag included in the effect's dependency array.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/threads/ThreadRouteScreen.tsx around line 228:
The `thread.visit` effect reads `environmentServerConfigsAtom` imperatively via `appAtomRegistry.get(...)`, so it never re-runs when the server config arrives later. When a cached thread is already present but the connection's server config is still `null` at mount, the effect returns early at the capability check; once the config loads with `threadVisits: true`, neither `selectedThread` nor `visitThreadMutation` has changed, so the effect does not re-run and the finished thread is never acknowledged as seen. The server config should be read through a reactive hook and the derived `supportsThreadVisits` flag included in the effect's dependency array.
| // Local visit stamps win; the shell's server-synced lastVisitedAt fills the | ||
| // gap so a visit on another device still clears the unread cue here. | ||
| const localLastVisitedAt = useUiStateStore((state) => state.threadLastVisitedAtById[threadKey]); | ||
| const lastVisitedAt = localLastVisitedAt ?? thread.lastVisitedAt ?? undefined; |
There was a problem hiding this comment.
🟡 Medium components/LegacySidebar.tsx:378
localLastVisitedAt ?? thread.lastVisitedAt always prefers the local stamp even when it is older than the newly synced thread.lastVisitedAt. After this device visited an older completion, another device visits a newer completion, and the shell delivers that newer stamp — but this row keeps comparing against the stale local timestamp and never clears the unread cue. Cross-device clearing only works on devices with no local history. Consider taking the later of the two timestamps so a newer server-synced visit clears the unread state on already-used devices.
Also found in 1 other location(s)
apps/web/src/components/Sidebar.tsx:732
localLastVisitedAt ?? thread.lastVisitedAtalways prefers any existing local stamp, even when it is older than the newly synced server stamp. For example, after this device locally visited completion A, another device visits newer completion B; the shell delivers B's newerthread.lastVisitedAt, but this row continues comparing against A and remains unread. Thus cross-device reads only clear the cue on devices with no local history, defeating the sync behavior for previously used devices. Choose the later valid timestamp, while preserving the explicit older local timestamp used by mark-unread (which likely requires separately tracking that override intent).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/LegacySidebar.tsx around line 378:
`localLastVisitedAt ?? thread.lastVisitedAt` always prefers the local stamp even when it is older than the newly synced `thread.lastVisitedAt`. After this device visited an older completion, another device visits a newer completion, and the shell delivers that newer stamp — but this row keeps comparing against the stale local timestamp and never clears the unread cue. Cross-device clearing only works on devices with no local history. Consider taking the later of the two timestamps so a newer server-synced visit clears the unread state on already-used devices.
Also found in 1 other location(s):
- apps/web/src/components/Sidebar.tsx:732 -- `localLastVisitedAt ?? thread.lastVisitedAt` always prefers any existing local stamp, even when it is older than the newly synced server stamp. For example, after this device locally visited completion A, another device visits newer completion B; the shell delivers B's newer `thread.lastVisitedAt`, but this row continues comparing against A and remains unread. Thus cross-device reads only clear the cue on devices with no local history, defeating the sync behavior for previously used devices. Choose the later valid timestamp, while preserving the explicit older local timestamp used by mark-unread (which likely requires separately tracking that override intent).
| // shell's synced value so the echo of our own visit does not re-dispatch; | ||
| // the decider is monotonic, so a raced older visit is a server-side no-op. | ||
| // Version skew: never send thread.visit to a server that predates it. | ||
| useEffect(() => { |
There was a problem hiding this comment.
🟡 Medium threads/ThreadRouteScreen.tsx:226
The useEffect at line 226 stamps a thread visit whenever selectedThread changes, but it does not check navigation focus. React Navigation keeps this screen mounted after it pushes routes like ThreadFile, ThreadTerminal, or GitOverview, so if a turn completes while the user is viewing one of those pushed screens, the still-mounted thread screen receives the updated selectedThread and dispatches visitThreadMutation with that turn's completedAt. This marks the thread as visited across all devices even though the user never saw the completed turn, incorrectly clearing the unread cue. Consider gating the side effect on focus via useIsFocused or useFocusEffect so visits are only stamped when the thread screen is actually visible.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/threads/ThreadRouteScreen.tsx around line 226:
The `useEffect` at line 226 stamps a thread visit whenever `selectedThread` changes, but it does not check navigation focus. React Navigation keeps this screen mounted after it pushes routes like `ThreadFile`, `ThreadTerminal`, or `GitOverview`, so if a turn completes while the user is viewing one of those pushed screens, the still-mounted thread screen receives the updated `selectedThread` and dispatches `visitThreadMutation` with that turn's `completedAt`. This marks the thread as visited across all devices even though the user never saw the completed turn, incorrectly clearing the unread cue. Consider gating the side effect on focus via `useIsFocused` or `useFocusEffect` so visits are only stamped when the thread screen is actually visible.
| if (!completedAt) return; | ||
| const syncedVisitedAt = activeThreadShell.lastVisitedAt; | ||
| if (syncedVisitedAt != null && Date.parse(syncedVisitedAt) >= Date.parse(completedAt)) return; | ||
| void visitThreadMutation({ |
There was a problem hiding this comment.
🟡 Medium components/ChatView.tsx:4094
A failed visitThreadMutation is silently discarded with void, so a transient transport/server failure while opening a finished thread leaves the thread's unread state unsynced on other devices — lastVisitedAt never advances past completedAt. No retry is scheduled, and every effect dependency stays unchanged, so the visit is never re-dispatched until an unrelated shell update or thread re-entry. Consider handling the failure (e.g., re-attempting after backoff or re-triggering the effect) instead of fire-and-forget.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/ChatView.tsx around line 4094:
A failed `visitThreadMutation` is silently discarded with `void`, so a transient transport/server failure while opening a finished thread leaves the thread's unread state unsynced on other devices — `lastVisitedAt` never advances past `completedAt`. No retry is scheduled, and every effect dependency stays unchanged, so the visit is never re-dispatched until an unrelated shell update or thread re-entry. Consider handling the failure (e.g., re-attempting after backoff or re-triggering the effect) instead of fire-and-forget.
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f52f172. Configure here.
| environmentId: selectedThread.environmentId, | ||
| input: { threadId: selectedThread.id, visitedAt: completedAt }, | ||
| }); | ||
| }, [selectedThread, visitThreadMutation]); |
There was a problem hiding this comment.
Mobile visit guard never sees sync
High Severity
The visit effect gates on selectedThread.lastVisitedAt, but when the shell is missing (for example archived threads, which are excluded from the active shell) selectedThread comes from threadDetailToShell, which never copies lastVisitedAt. The behind-check always fails open, so each thread.visited echo rebuilds selectedThread and dispatches another visit.
Reviewed by Cursor Bugbot for commit f52f172. Configure here.
| environmentId: selectedThread.environmentId, | ||
| input: { threadId: selectedThread.id, visitedAt: completedAt }, | ||
| }); | ||
| }, [selectedThread, visitThreadMutation]); |
There was a problem hiding this comment.
Visit skipped until thread changes
Medium Severity
The mobile visit effect reads threadVisits via a one-shot appAtomRegistry.get and only depends on selectedThread and visitThreadMutation. If server config is still unset on first run, the effect returns early and does not re-run when the capability later becomes true. Web includes reactive supportsThreadVisits in its effect deps for this reason.
Reviewed by Cursor Bugbot for commit f52f172. Configure here.
ApprovabilityVerdict: Needs human review 4 blocking correctness issues found. This PR introduces a new cross-device feature for syncing thread read state, spanning mobile, web, server, contracts, and adding a database migration. Multiple unresolved findings identify potential issues including an infinite dispatch loop for archived threads, non-reactive config reads, and timestamp merge logic bugs. You can customize Macroscope's approvability policy. Learn more. |
1 participant
Opening a thread on web did not clear the unread cue anywhere else. The visit stamp lived in web localStorage only, so a fresh mobile install treated every finished thread as needing attention even after I already read them all on desktop.
Now the server owns the acknowledgement. A new
thread.visitcommand stampslastVisitedAton the thread projection, and it flows to every client through the existing shell stream.completedAt(not wall-clock), matching web's existing local semantics: a completion that lands while the thread is open still reads as unseen, and clock skew can't leak in.updatedAt, so reading a thread cannot churn ordering.threadVisitscapability so old servers never see the command. Clients only dispatch when the synced value is behind, so the event log gets at most one visit per completion.Mobile does not render anything from
lastVisitedAtyet; UI changes come separately. This PR only makes the state real and synced.Testing: new decider tests for
thread.visit(first visit, monotonic advance, stale re-emit, archived thread, unknown thread); projection fixtures updated; full typecheck plus contracts / client-runtime / web / mobile / server suites pass (the only server failures are the pre-existing service-launcher version mismatches in this worktree, verified present without this change).Built by Claude Fable 5 running in Claude Code.
🤖 Generated with Claude Code
Note
Medium Risk
Touches orchestration commands, projections, and cross-client unread semantics; behavior is guarded by capability and monotonic visits, but version skew and race handling matter for correctness.
Overview
Adds server-synced thread read acknowledgement so opening a finished thread clears the unread cue on every client, not only in web localStorage.
A new
thread.visitcommand andthread.visitedevent persistlastVisitedAton the thread projection (DB migration, decider, projector, shell queries). Visits use the acknowledged turn’scompletedAt, advance monotonically, work on archived threads, and do not bumpupdatedAt. Servers advertisethreadVisitscapability; clients gate dispatch on capability and only send when syncedlastVisitedAtis behind the latest completion.Web and mobile dispatch visits when a thread is open (mirroring existing local
markThreadVisitedon web). Web sidebar/status indicators prefer local visit stamps but fall back tothread.lastVisitedAtfrom the shell so another device’s read clears the cue here.Reviewed by Cursor Bugbot for commit f52f172. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Sync thread read state across devices via server-persisted
lastVisitedAtthread.visitcommand andthread.visitedevent to the contracts, server decider, and projections so that opening a completed thread stampslastVisitedAton the server.thread.visitwhen a thread is viewed andlastVisitedAtis missing or stale relative tolatestTurn.completedAt.lastVisitedAtfrom the thread shell when no local stamp exists.capabilities.threadVisitsflag on the server descriptor gates the feature so older clients remain unaffected.last_visited_attoprojection_threads.📊 Macroscope summarized f52f172. 21 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.