feat(web): session list status indicators (attention + scheduled)#699
feat(web): session list status indicators (attention + scheduled)#699heavygee wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
Findings
- [Major] Selected sessions can be marked unread after the user has already read them —
markSessionSeenonly runs whenselectedSessionIdchanges, so a selected session's seen watermark is not advanced when SSE/refetch updates itsupdatedAtwhile the chat is open. When the user later navigates away,classifySessionAttentioncompares the newerupdatedAtto the stale open-timelastSeenAtand shows a falseNew activitydot. Evidence:web/src/router.tsx:153andweb/src/lib/sessionAttention.ts:29.
Suggested fix:const selectedSession = useMemo( () => sessions.find(session => session.id === selectedSessionId) ?? null, [sessions, selectedSessionId] ) useEffect(() => { if (selectedSessionId) { markSessionSeen(selectedSessionId, Math.max(Date.now(), selectedSession?.updatedAt ?? 0)) } }, [selectedSessionId, selectedSession?.updatedAt])
Summary
- Review mode: initial
- Found one issue in the unread indicator state flow: the selected session is cleared on open, but not kept cleared as new activity arrives while it remains selected.
Testing
- Not run (automation)
HAPI Bot
Keep the unread watermark in sync with live session activity so navigating away from an open session does not show a false unread dot. Addresses review on tiann#699 Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Findings
-
[Major] Inactive sessions can never show unread activity —
classifySessionAttentionreturns before checkingupdatedAtwheneversummary.activeis false, so a background session that finishes/stops after the user last saw it loses the new-activity dot exactly when the sidebar needs to explain why it stopped. Evidence:web/src/lib/sessionAttention.ts:13.
Suggested fix:if (options.selected || summary.thinking) { return null } if (summary.active && summary.backgroundTaskCount > 0) { return { kind: 'background' } } if (summary.updatedAt > options.lastSeenAt) { return { kind: 'unread' } }
-
[Major] Browser clock skew can hide future unread updates — the seen watermark is stored as
Math.max(Date.now(), selectedSession.updatedAt), but unread compares hub/sessionupdatedAtagainst that local browser timestamp. If a phone/browser clock is ahead of the hub, opening a session records a future watermark and later hub updates with smallerupdatedAtvalues will not showNew activity. Evidence:web/src/router.tsx:161andweb/src/lib/sessionAttention.ts:29.
Suggested fix:useEffect(() => { if (!selectedSessionId || !selectedSession) { return } markSessionSeen(selectedSessionId, selectedSession.updatedAt) }, [selectedSessionId, selectedSession?.updatedAt])
Summary
- Review mode: follow-up after new commits
- The prior bot finding was addressed, but the current attention logic still drops unread indicators for inactive sessions and can suppress future unread indicators when client and hub clocks differ.
Testing
- Not run (automation)
HAPI Bot
|
Follow-up commits address bot review: keep last-seen watermark synced while selected session receives updates, and compare hub updatedAt only (no client clock). Local typecheck + targeted vitest pass. |
|
Addressed remaining review findings in 99e9ad8:
Also added a Display → Session list status setting:
Tests + typecheck green locally. |
|
Temporarily closing to retrigger CI on latest commits. |
There was a problem hiding this comment.
Findings
-
[Major] Detailed mode marks old viewed sessions as unread —
markSessionSeenonly runs whensessionListStatusMode === 'detailed', while unread falls back tolastSeenAt = 0. If a user views sessions in standard mode and later enables detailed status, every unselected session with anyupdatedAtvalue can showNew activityeven though the activity was already seen. Evidence:web/src/router.tsx:160.
Suggested fix:useEffect(() => { if (!selectedSessionId || !selectedSession) { return } markSessionSeen(selectedSessionId, selectedSession.updatedAt) }, [selectedSessionId, selectedSession?.updatedAt])
-
[Minor] localStorage write failures can break the sessions page — the new last-seen writer catches read/parse failures but lets
localStorage.setItemthrow. In browsers where storage is disabled, full, or denied in an embedded context, selecting a session in detailed mode can throw from this effect path instead of degrading to “no unread persistence.” Evidence:web/src/lib/sessionLastSeen.ts:29.
Suggested fix:function writeStore(store: LastSeenStore): void { if (typeof localStorage === 'undefined') { return } try { localStorage.setItem(STORAGE_KEY, JSON.stringify(store)) } catch { // Ignore storage errors } }
Summary
- Review mode: follow-up after new commits
- The previous inactive-session and clock-skew findings were addressed, but the new settings gate can still produce false unread dots, and the new persistence write path needs the same storage hardening used elsewhere.
Testing
- Not run (automation)
- Coverage gap:
shared/src/sessionSummary.test.tsis not included in rootbun run testbecauseshared/package.jsonhas no test script; the PR’s targeted shared command is manual-only.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Minor] Storage-denied browsers can still crash the session list —
typeof localStorageruns before thetry, so an embedded/opaque-origin browser that throws while resolving thelocalStorageproperty can still throw duringgetSessionLastSeenAtrender ormarkSessionSeen, instead of degrading to no unread persistence. Evidence:web/src/lib/sessionLastSeen.ts:6.
Suggested fix:function getLocalStorage(): Storage | null { if (typeof window === 'undefined') { return null } try { return window.localStorage } catch { return null } } function readStore(): LastSeenStore { const storage = getLocalStorage() if (!storage) { return {} } try { const raw = storage.getItem(STORAGE_KEY) // existing parse path... } catch { return {} } } function writeStore(store: LastSeenStore): void { const storage = getLocalStorage() if (!storage) { return } try { storage.setItem(STORAGE_KEY, JSON.stringify(store)) } catch { // Ignore storage errors } }
Questions
- None
Summary
- Review mode: follow-up after new commits
- The previous two findings are addressed. One storage-access edge remains in the new unread tracking helper.
Testing
- Not run (automation)
- Coverage gap:
shared/src/sessionSummary.test.tsis still not included in rootbun run testbecauseshared/package.jsonhas no test script.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Minor] New shared test is not included in root test automation — the added
shared/src/sessionSummary.test.tsexercises the new summary fields, but repo-levelbun run testonly runs cli/hub/web andshared/package.jsonhas no test script, so this coverage is easy to miss in CI or local pre-merge checks. Evidence:shared/src/sessionSummary.test.ts:1,package.json:21,shared/package.json:1.
Suggested fix:// package.json { "scripts": { "test": "bun run test:cli && bun run test:hub && bun run test:web && bun run test:shared", "test:shared": "cd shared && bun run test" } } // shared/package.json { "scripts": { "test": "bun test" } }
Questions
- None
Summary
Review mode: follow-up after new commits
- The prior storage-access finding is addressed.
- One automation coverage gap remains for the new shared summary tests.
Testing
- Attempted
bun run typecheck,cd web && bun test ..., andcd shared && bun test ...; not run becausebunis not installed in this runner shell.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] Shared test runner mismatch breaks root test automation — the follow-up wires
bun run testto executecd shared && bun run test, and that script runsbun test; however the new shared test imports fromvitest. Existing Bun-runner tests in this repo importbun:test, while Vitest tests are executed throughvitest run, so the standard root test path will fail before exercising the new coverage. Evidence:shared/src/sessionSummary.test.ts:1,shared/package.json:22.
Suggested fix:import { describe, expect, it } from 'bun:test'
Questions
- None
Summary
Review mode: follow-up after new commits
- The prior automation gap was addressed, but the new shared package test command now points Bun's test runner at a Vitest-importing test file.
Testing
- Not run (automation);
bunis not installed in this runner shell.
HAPI Bot
There was a problem hiding this comment.
Findings
No issues found.
Summary
Review mode: follow-up after new commits
Reviewed the full latest diff, with prior-review/compare context unavailable from the provided review API responses. Residual risk: manual UI behavior for unread clearing and attention-dot rendering still needs browser validation.
Testing
Not run (automation: bun is not available on this runner PATH).
HAPI Bot
There was a problem hiding this comment.
Findings
No issues found.
Summary
Review mode: follow-up after new commits
Reviewed the full latest diff, using prior bot review and compare-diff context. No blocker/major/minor issues found in the added or modified lines. Residual risk: browser-only UI behavior for the detailed status setting and scheduled clock indicator still needs manual validation.
Testing
Not run (automation: bun is not available on this runner PATH).
HAPI Bot
Keep the unread watermark in sync with live session activity so navigating away from an open session does not show a false unread dot. Addresses review on tiann#699 Co-authored-by: Cursor <cursoragent@cursor.com>
f90dae5 to
2a785eb
Compare
Keep the unread watermark in sync with live session activity so navigating away from an open session does not show a false unread dot. Addresses review on tiann#699 Co-authored-by: Cursor <cursoragent@cursor.com>
2a785eb to
84f4723
Compare
There was a problem hiding this comment.
Findings
- [Major] Keep the global SSE stream while viewing a session —
eventSubscriptionswitches to{ sessionId }whenever a chat is open, soSSEManager.shouldSend()will only deliver that one session's events. Updates from background sessions no longer reach the client, which breaks the new sidebar attention/unread indicators exactly when the user is inside another session. Evidence:web/src/App.tsx:298.
Suggested fix:const globalEventSubscription = useMemo(() => ({ all: true }), []) const sessionEventSubscription = useMemo( () => selectedSessionId ? { sessionId: selectedSessionId } : null, [selectedSessionId] ) const sseEnabled = Boolean(api && token) const { subscriptionId: globalSubscriptionId } = useSSE({ enabled: sseEnabled, token: token ?? '', baseUrl, subscription: globalEventSubscription, onConnect: handleSseConnect, onDisconnect: handleSseDisconnect, onEvent: () => {}, onToast: handleToast }) const { subscriptionId: sessionSubscriptionId } = useSSE({ enabled: sseEnabled && Boolean(sessionEventSubscription), token: token ?? '', baseUrl, subscription: sessionEventSubscription ?? undefined, onEvent: handleSseEvent })
- [Major] Refresh scheduled counts when queued scheduled state changes — the
/sessionsresponse now computesfutureScheduledMessageCount, but the SSE cache path preserves the old value fromexistingand handlesmessage-received/message-cancelled/messages-consumedwithout invalidating the session list. After scheduling or canceling a future message, the sidebar clock can stay missing or stuck until a full reconnect/refetch. Evidence:web/src/hooks/useSSE.ts:292.
Suggested fix:if (event.type === 'message-received') { ingestIncomingMessages(event.sessionId, [event.message]) if (event.message.scheduledAt != null) { queueSessionListInvalidation() } } if (event.type === 'message-cancelled' || event.type === 'messages-consumed') { queueSessionListInvalidation() }
Questions
- None.
Summary
Review mode: follow-up after new commits
The latest diff introduces two regressions in the live session-list status path: background-session events are no longer subscribed while a session is selected, and scheduled-count state is not refreshed after schedule/cancel/consume events.
Testing
Not run (automation: bun is not available on this runner PATH).
HAPI Bot
|
Rebased onto Stack order for daily driver: voice (#692) → session attention (#699). One conflict in Branch force-pushed: Daily driver rebuilt and active on this tip. |
Keep the unread watermark in sync with live session activity so navigating away from an open session does not show a false unread dot. Addresses review on tiann#699 Co-authored-by: Cursor <cursoragent@cursor.com>
84f4723 to
84a40ff
Compare
|
Branch recovered for upstream review The prior rebase onto `feat/pluggable-voice-backend` was a mistake for this PR. The branch is now rebased on `upstream/main` only:
|
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits
No issues found above the reporting threshold in the latest diff. The previous bot findings appear addressed by keeping separate global/session SSE subscriptions and invalidating the session list on scheduled-message, cancel, and consume events. Residual risk: UI behavior was not manually exercised in a browser.
Testing
Not run (automation/security: did not execute PR code).
HAPI Bot
Extend SessionSummary with pending request kinds and background task counts, classify sidebar attention state (permission, input, background, unread), and track last-seen timestamps client-side when opening sessions. Fixes tiann#698 Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the unread watermark in sync with live session activity so navigating away from an open session does not show a false unread dot. Addresses review on tiann#699 Co-authored-by: Cursor <cursoragent@cursor.com>
Compare unread state using hub timestamps only so client clock skew cannot hide or fabricate sidebar activity indicators. Co-authored-by: Cursor <cursoragent@cursor.com>
Allow inactive sessions to show unread activity, require explicit last-seen timestamps, and gate detailed attention dots behind a new Display setting (Standard default, Detailed opt-in). Co-authored-by: Cursor <cursoragent@cursor.com>
Track selected session updatedAt in standard and detailed modes so enabling detailed status does not flood unread dots. Ignore localStorage write failures on the write path. Co-authored-by: Cursor <cursoragent@cursor.com>
Access window.localStorage inside try/catch so embedded browsers that throw on the property getter degrade safely. Co-authored-by: Cursor <cursoragent@cursor.com>
Wire shared tests into bun run test so sessionSummary coverage runs in CI. Co-authored-by: Cursor <cursoragent@cursor.com>
Match the shared package test runner so root bun run test succeeds. Co-authored-by: Cursor <cursoragent@cursor.com>
Add futureScheduledMessageCount to SessionSummary (hub batch query) and render the composer ScheduleIcon in Detailed mode when a session has future scheduled messages pending. Co-authored-by: Cursor <cursoragent@cursor.com>
Invalidate session list when scheduled messages are received, cancelled, or consumed so futureScheduledMessageCount stays accurate in Detailed mode. Co-authored-by: Cursor <cursoragent@cursor.com>
84a40ff to
53e5e0d
Compare
There was a problem hiding this comment.
Findings
- [Minor] Scheduled count can go stale after a schedule matures —
countFutureScheduledBySessionIdsonly counts rows withscheduled_at > now, but the web cache is invalidated only on message receive/cancel/consume events. When a scheduled row crosses its timestamp and the CLI is offline or has not acked yet, no web event refreshes/api/sessions, so detailed mode can show stale clock state or hide a still-pending scheduled row after a later refetch. Evidencehub/src/store/messages.ts:273.
Suggested fix:const rows = db.prepare(` SELECT session_id, COUNT(*) AS count FROM messages WHERE session_id IN (${placeholders}) AND invoked_at IS NULL AND local_id IS NOT NULL AND scheduled_at IS NOT NULL GROUP BY session_id `).all(...sessionIds) as { session_id: string; count: number }[]
Questions
- None.
Summary
Review mode: follow-up after new commits
One user-visible freshness issue found in the scheduled-message indicator. No other issues above the reporting threshold in the reviewed diff.
Testing
Not run (automation/security: did not execute PR code).
HAPI Bot
When the mature-scan releases scheduled messages to the CLI, notify web clients so futureScheduledMessageCount drops without waiting for consume. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
- Review mode: follow-up after new commits
- No issues above the reporting threshold in the latest diff. The prior scheduled-count freshness concern is covered by the new
scheduled-maturedevent and session-list invalidation path. Residual risk: UI behavior for detailed status/unread indicators still depends on manual verification across real multi-session flows.
Testing
- Not run (automation/security: did not execute PR code).
HAPI Bot
Avoid refetching the session list every 5s tick while CLI ack is pending. Aligns SSE with the inactivity scan cadence (scheduledAt > now - 5s). Co-authored-by: Cursor <cursoragent@cursor.com>
|
Follow-up 2274138: cold review caught scheduled-matured firing every 5s tick until CLI ack; now only emits when scheduledAt falls within the current mature-scan window (same cadence as expireInactive). |
There was a problem hiding this comment.
Findings
- [Major] Scheduled indicator can stay stale after delayed mature scans —
scheduled-maturedis now emitted only whenscheduledAt > now - 5_000, but the release tick is a best-effortsetInterval; hub restarts, event-loop stalls, or a suspended process can make the first scan happen more than 5s afterscheduled_at. In that case the CLI still receives the matured message, but the global SSE session-list invalidation never fires, sofutureScheduledMessageCountcan keep showing the clock until some unrelated session-list refresh. Evidence:hub/src/sync/messageService.ts:488.
Suggested fix:for (const msg of mature) { maturedSessionIds.add(msg.sessionId) const update = { // existing update payload } this.io.of('/cli').to(`session:${msg.sessionId}`).emit('update', update) }
Questions
- None.
Summary
Review mode: follow-up after new commits
The latest follow-up change fixes repeated scheduled-matured emissions by using a 5s window, but that makes the UI refresh lossy whenever the mature scan is delayed. Residual risk: detailed sidebar behavior still needs real multi-session/manual verification.
Testing
- Not run (automation/security: did not execute PR code).
HAPI Bot
| const mature = this.store.messages.getMatureScheduledMessages(now) | ||
| const maturedSessionIds = new Set<string>() | ||
| for (const msg of mature) { | ||
| if (isScheduledNewlyMature(msg.scheduledAt, now)) { |
There was a problem hiding this comment.
[MAJOR] This time-window guard can miss the only session-list refresh that clears the scheduled clock. releaseMatureScheduledMessages() is driven by a 5s setInterval, but the first scan after scheduled_at can happen later than 5s after the target time if the hub was restarted, the process was suspended, or the event loop was blocked. The message still gets emitted to the CLI, but scheduled-matured is skipped, so global SSE clients do not invalidate /sessions and futureScheduledMessageCount can remain stale until an unrelated refresh.
Suggested fix:
for (const msg of mature) {
maturedSessionIds.add(msg.sessionId)
const update = {
// existing update payload
}
this.io.of('/cli').to(`session:${msg.sessionId}`).emit('update', update)
}
Summary
Adds session-list status indicators so background sessions show why they stopped or what is pending — not just spinners or generic pending counts. Builds on merged #694 (SSE freshness).
SessionSummarywithpendingRequestKinds,backgroundTaskCount, andfutureScheduledMessageCount(shared/hub)lastSeenAtper session in localStorage; advance watermark when opening/updating selected sessionpending Ntext when a kind-specific indicator is shown (Detailed)Related: #270 (broader unread/session sidebar enhancements)
Test plan
bun run typecheckbun run test(shared + hub + web)Issues
Fixes #698