🔔 feat: Unseen Reply Indicators, Reply Notifications, and Mark as Unread - #15025
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an unread-reply/read-state system for conversations using two server-managed timestamps (lastResponseAt, lastSeenAt) and wires it through backend persistence paths and the client UI to support unseen indicators, per-device reply notifications, tab/title/favicon badges, and “mark as unread”.
Changes:
- Extend conversation schema/types and list payloads to include
lastResponseAt/lastSeenAt, plus DB methods + API routes to mark seen/unread and stamp assistant replies. - Implement client-side unseen derivation + caching helpers, plus UI affordances (sidebar dot + a11y label, tab/favicon badge, notifications/sound, and “mark as unread” action).
- Add targeted Jest coverage across data-schemas, data-provider, API routes, and client hooks/mutations.
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/data-schemas/src/types/convo.ts | Adds lastResponseAt/lastSeenAt to conversation type. |
| packages/data-schemas/src/schema/convo.ts | Adds lastResponseAt/lastSeenAt fields to Mongoose schema. |
| packages/data-schemas/src/methods/conversation.ts | Adds seen/unread/stamp DB methods and includes fields in list selection. |
| packages/data-schemas/src/methods/conversation.spec.ts | Tests for seen/unread/stamp behavior + list field inclusion. |
| packages/data-provider/src/types/queries.ts | Expands MinimalConversation to include unseen timestamps. |
| packages/data-provider/src/types.ts | Adds request/response wire types for seen/unread endpoints. |
| packages/data-provider/src/schemas.ts | Adds zod schema fields; strips timestamps from presets. |
| packages/data-provider/src/schemas.spec.ts | Tests that presets strip unseen timestamps (and other runtime fields). |
| packages/data-provider/src/keys.ts | Adds mutation keys for convo seen/unread. |
| packages/data-provider/src/data-service.ts | Adds data-service methods for seen/unread endpoints. |
| packages/data-provider/src/config.ts | Adds unseen timestamps to excludedKeys for safe partial updates. |
| packages/data-provider/src/api-endpoints.ts | Adds /convos/seen and /convos/unread endpoints. |
| client/src/utils/convos.ts | Adds unseen derivation helper and cached-list lookup helper; preserves unseen fields on updates. |
| client/src/utils/convos.spec.ts | Tests unseen derivation + cache update/merge behavior. |
| client/src/store/settings.ts | Adds per-device settings atoms for badge/notifications/sound. |
| client/src/routes/Root.tsx | Mounts reply-notification subsystem in an isolated subtree. |
| client/src/locales/en/translation.json | Adds localization keys for notifications + mark-unread + settings section. |
| client/src/hooks/SSE/useEventHandlers.ts | Stamps list cache with lastResponseAt on run completion. |
| client/src/hooks/Conversations/useUnseenConversations.ts | Derives unseen conversation set from cached list queries. |
| client/src/hooks/Conversations/useUnseenBadge.ts | Implements title + favicon badging based on unseen count. |
| client/src/hooks/Conversations/useReplyWatcher.ts | Adds background watcher to merge timestamps from job completion + away polling. |
| client/src/hooks/Conversations/useReplyAlerts.ts | Adds desktop notifications + synthesized chime + permission request helper. |
| client/src/hooks/Conversations/useConversationSeen.ts | Marks conversations seen based on focus + scroll position + cache events. |
| client/src/hooks/Conversations/index.ts | Exports new conversation-related hooks/utilities. |
| client/src/hooks/Conversations/tests/useUnseenConversations.spec.tsx | Tests unseen derivation from query cache + dedupe + identity stability. |
| client/src/hooks/Conversations/tests/useUnseenBadge.spec.tsx | Tests title prefixing and icon href recording behavior. |
| client/src/hooks/Conversations/tests/useReplyWatcher.spec.tsx | Tests job-completion merge + away polling + invalidation behavior. |
| client/src/hooks/Conversations/tests/useReplyAlerts.spec.tsx | Tests notification/sound behavior + permission request + click navigation. |
| client/src/hooks/Conversations/tests/useConversationSeen.spec.tsx | Tests “seen” triggers and cost-guard behavior. |
| client/src/data-provider/mutations.ts | Adds optimistic seen/unread mutations updating cached list rows. |
| client/src/data-provider/tests/useMarkConversationUnreadMutation.test.tsx | Tests optimistic unread + rollback behavior. |
| client/src/data-provider/tests/useMarkConversationSeenMutation.test.tsx | Tests optimistic seen + rollback behavior. |
| client/src/components/Nav/Settings/types.ts | Adds Notifications section to Settings tab metadata. |
| client/src/components/Nav/Settings/registry.tsx | Adds toggles for unseen badge / desktop notifications / chime sound. |
| client/src/components/Nav/Settings/controls.tsx | Extends toggle control to support onCheckedChange callback. |
| client/src/components/Conversations/utils.ts | Ensures conversation row memoization compares unseen timestamps. |
| client/src/components/Conversations/ConvoOptions/ConvoOptions.tsx | Adds “Mark as unread” option, gated by active/unseen state. |
| client/src/components/Conversations/Convo.tsx | Adds unseen dot + aria-label augmentation for screen readers. |
| client/src/components/Chat/Messages/MessagesView.tsx | Hooks “seen” reporting into existing near-bottom observer path. |
| api/server/utils/import/importBatchBuilder.js | Strips unseen timestamps when duplicating/forking conversations. |
| api/server/services/Threads/manage.js | Stamps lastResponseAt when assistant message is persisted (threads path). |
| api/server/routes/convos.js | Adds /api/convos/seen and /api/convos/unread routes. |
| api/server/routes/tests/convos.spec.js | Adds route coverage for seen/unread endpoints. |
| api/server/routes/test-utils/convos-route-mocks.js | Adds mocks for markConvoSeen/markConvoUnread. |
| api/server/controllers/agents/resume.js | Best-effort stamping for resumed turns that bypass BaseClient save payload. |
| api/server/controllers/agents/responses.js | Stamps lastResponseAt for Responses API conversation saves. |
| api/server/controllers/agents/request.js | Best-effort stamping for terminal abort re-save path. |
| api/app/clients/specs/BaseClient.test.js | Ensures BaseClient stamps only assistant replies; excludedKeys prevents wipe. |
| api/app/clients/BaseClient.js | Stamps lastResponseAt for persisted assistant replies (non-temporary). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const identityOf = (unseen: UnseenConversation[]): string => | ||
| unseen | ||
| .map((c) => c.conversationId) | ||
| .sort() | ||
| .join(','); |
There was a problem hiding this comment.
Fixed: the identity now includes the title, so an auto-titled conversation refreshes the array instead of keeping the stale entry. useReplyAlerts keys arrivals off conversationId, so the extra re-run cannot re-notify.
| const FAVICON_SELECTOR = 'link[rel="icon"]'; | ||
| const FALLBACK_BADGE_RGB = '59 130 246'; | ||
|
|
||
| /** | ||
| * Reads the badge colour from the live theme so the favicon dot tracks the same semantic role | ||
| * as the sidebar indicator instead of hard-coding a palette value. | ||
| */ | ||
| const badgeColor = (): string => { | ||
| const value = getComputedStyle(document.documentElement).getPropertyValue('--status-info').trim(); | ||
| return `rgb(${value || FALLBACK_BADGE_RGB})`; | ||
| }; |
There was a problem hiding this comment.
Fixed: the hard-coded fallback is gone. badgeColor() now returns null when --status-info cannot be read, and the effect leaves the plain favicon in place rather than painting a colour the theme never chose.
| const image = new Image(); | ||
| let cancelled = false; | ||
|
|
||
| image.onload = () => { | ||
| if (cancelled) { | ||
| return; | ||
| } | ||
| const size = 32; | ||
| const canvas = document.createElement('canvas'); | ||
| canvas.width = size; | ||
| canvas.height = size; | ||
| const context = canvas.getContext('2d'); | ||
| if (!context) { | ||
| return; | ||
| } | ||
| context.drawImage(image, 0, 0, size, size); | ||
| const radius = size * 0.28; | ||
| const center = size - radius - 1; | ||
| context.beginPath(); | ||
| context.arc(center, center, radius, 0, Math.PI * 2); | ||
| context.fillStyle = badgeColor(); | ||
| context.fill(); | ||
| onReady(canvas.toDataURL('image/png')); | ||
| }; |
There was a problem hiding this comment.
Fixed both: the canvas now uses the image natural size (falling back to 32 for a sizeless SVG icon), and toDataURL() is guarded so a tainted canvas leaves the unbadged favicon instead of throwing.
5d32ef0 to
33c35ad
Compare
|
@codex review |
33c35ad to
391f1ad
Compare
![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: 33c35ade27
ℹ️ 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 result = await Conversation.updateOne( | ||
| { conversationId, user }, | ||
| { $set: { lastSeenAt: new Date() } }, | ||
| { timestamps: false }, |
There was a problem hiding this comment.
When another device persists a reply after this client decides it is caught up but before this update executes, stamping lastSeenAt with the server's current time makes it newer than that unobserved reply and silently clears its indicator and alerts. Send the observed lastResponseAt with the mutation and conditionally acknowledge only that response so a concurrent newer reply remains unseen.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the client sends the lastResponseAt it had on screen and markConvoSeen filters on lastResponseAt: { $lte: observed }, so a reply persisted from another device mid-flight is not acknowledged with it. The stamp itself stays server-side.
| const arrivals = unseen.filter( | ||
| (conversation) => !known.has(conversation.conversationId) && conversation.conversationId, | ||
| ); |
There was a problem hiding this comment.
When a conversation is already unseen and receives another reply while the user remains away, its ID is already in known, so this filter produces no arrival and neither the desktop notification nor chime fires. Track the latest lastResponseAt per conversation rather than only IDs so subsequent replies to an unread conversation still trigger the configured alerts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: UnseenConversation now carries lastResponseAt, and arrivals are keyed on that stamp changing rather than on set membership, so a follow-up reply to an already-unseen chat alerts. It is in the identity too, or the array would not even change.
| sharedContext = sharedContext ?? new AudioContext(); | ||
| if (sharedContext.state === 'suspended') { | ||
| void sharedContext.resume(); | ||
| } |
There was a problem hiding this comment.
On browsers that suspend Web Audio until an AudioContext is created or resumed during a user activation, the first context is created only when an alert arrives while the document is unfocused; resume() then lacks a user gesture and the sound setting remains silent. Initialize or resume the shared context from the sound toggle's change handler, then reuse that unlocked context here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: added unlockReplyNotificationSound(), called from the sound toggle change handler the same way the notification permission request is. The chime reuses that context.
| const queries = queryClient | ||
| .getQueryCache() | ||
| .findAll([QueryKeys.allConversations], { exact: false }); |
There was a problem hiding this comment.
A pinned conversation older than the loaded Chats pages can exist only in the dedicated pinnedConversations cache, but this helper searches only allConversations. Opening such an unseen pin therefore makes useConversationSeen treat it as missing/seen and never send the /seen mutation, leaving its unread dot stuck until an unrelated list fetch happens to load it; resolve conversations from the pinned cache as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: findConvoInAllQueries now falls back to the pinned cache, so a pin older than the loaded chat pages resolves and its dot clears instead of sticking.
| const { data: activeJobsData } = useActiveJobs(); | ||
| const activeJobIds = activeJobsData?.activeJobIds; |
There was a problem hiding this comment.
When the cached active-job set is empty, useActiveJobs disables its polling interval, so this subscription cannot observe a job starting on another device while the tab remains focused. The fallback list poll also explicitly skips focused documents, allowing the remote run to start and finish without refreshing the sidebar timestamp until some unrelated refetch; retain a low-rate foreground poll or add another completion signal for this case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one. The mechanics are right (useActiveJobs returns false from refetchInterval once the set is empty, and the away poll skips a focused document), but the conclusion is a deliberate trade rather than a defect: the feature is about replies that land while you are away, and a foreground poll would add a recurring request for every user to shave latency off a cross-device case where the reply is already visible in the open tab. The list still refetches on window focus, so the indicator appears as soon as the user returns. Adding an always-on foreground poll is a cost I would rather not put on everyone.
| const cachedStamp = findConvoInAllQueries(queryClient, conversationId)?.lastResponseAt; | ||
| const cachedAt = cachedStamp ? new Date(cachedStamp).getTime() : 0; | ||
| if (cachedAt > lastObservedAt) { | ||
| continue; |
There was a problem hiding this comment.
If the server clock is ahead of the browser, a previous reply's server-stamped lastResponseAt can be greater than this client-side lastObservedAt, causing completion of the current job to skip the fetch even though the cache was never updated for that job. Record the cached stamp when the job is first observed and test whether it changed instead of ordering timestamps from different clocks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the watcher now records the cached stamp when the job is first seen running and checks whether it changed at completion, so no server stamp is ordered against the browser clock.
| /** Only reached once a response has been generated; drives the unseen-reply indicator. */ | ||
| ...(req?.body?.isTemporary !== true && { lastResponseAt: new Date() }), |
There was a problem hiding this comment.
For stored Responses API calls, saveConversation runs before saveInputMessages and saveResponseOutput; if either later write fails, this successful conversation write leaves lastResponseAt set even though no assistant reply was persisted. That produces a phantom unseen indicator/notification whose conversation has no corresponding message, so move the stamp after a successful response save or perform it as part of the durable response path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the stamp moved out of saveConversation into stampResponseReply, called after saveResponseOutput succeeds at both store sites, so a failed message write can no longer leave a dot with no reply behind it.
| const known = knownRef.current; | ||
| knownRef.current = new Set(unseen.map((conversation) => conversation.conversationId)); | ||
|
|
||
| if (known === null) { | ||
| return; |
There was a problem hiding this comment.
On a background reload, this effect commonly runs first with unseen empty because the conversation query is still loading, so it changes knownRef from null to an empty set. When the existing unseen backlog arrives asynchronously, every conversation is then classified as a new arrival and can emit a burst of notifications or chimes; keep the initialization state until the initial conversation query has settled, or distinguish replies by a session-start timestamp.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: useUnseenConversations returns null until a list has actually resolved, and the alerts hold their uninitialized state through it, so a backlog arriving after a background reload is recorded rather than announced.
| const notification = new Notification(localize('com_ui_reply_ready'), { | ||
| body: conversation.title || localize('com_ui_untitled'), | ||
| tag: conversation.conversationId, | ||
| }); |
There was a problem hiding this comment.
On mobile browsers that expose the Notification API and permission state but do not support new Notification() outside a service worker, this constructor throws a TypeError; other platform-level notification failures can throw here as well. Because the effect does not catch it, a reply can raise an uncaught React effect error instead of degrading silently, so use the service-worker notification path where available or catch constructor failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the constructor and its onclick wiring are wrapped, so an unsupported platform degrades quietly instead of throwing out of the effect.
| const color = badgeColor(); | ||
| if (activeCount === 0 || color == null) { |
There was a problem hiding this comment.
With a nonzero unseen count, switching between light, dark, or a custom theme changes --status-info but leaves activeCount unchanged, so this effect never reruns and the favicon retains the previous theme's badge color. Subscribe to the active theme or observe the root theme attribute so this semantic appearance follows the live theme.
AGENTS.md reference: AGENTS.md:L5-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the favicon effect repaints under a MutationObserver on the root class and data-theme attributes, so the badge follows the live theme, custom ones included.
391f1ad to
e24cbc6
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e24cbc6c71
ℹ️ 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 previous = findConvoInAllQueries(queryClient, vars.conversationId)?.lastSeenAt; | ||
| updateConvoInAllQueries(queryClient, vars.conversationId, (convo) => ({ | ||
| ...convo, | ||
| lastSeenAt: new Date().toISOString(), |
There was a problem hiding this comment.
When the browser clock is behind the server, this optimistic lastSeenAt remains earlier than the server-stamped lastResponseAt, so the conversation still appears unseen. The conversation-query cache update then triggers useConversationSeen's subscription again, repeatedly starting the same mutation and cache update, potentially causing a request loop or stack overflow. Set the optimistic value from the observed lastResponseAt (which equality treats as seen), or return and cache the server's stamp.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the optimistic value is now the observed lastResponseAt rather than the browser clock, and equality reads as caught up, so a clock behind the server can no longer leave the row unseen and restart the mutation from its own cache write. Covered by a test asserting isConversationUnseen is false after the optimistic pass.
|
|
||
| const seenIds = new Set<string>(); | ||
| const unseen: UnseenConversation[] = []; |
There was a problem hiding this comment.
For a pinned conversation older than the loaded Chats pages, marking it unread updates only the dedicated pinned cache, but this aggregation reads and subscribes exclusively to allConversations. Its row gains the unread dot while the tab count and reply-alert state never include it. Fresh evidence beyond the earlier point-lookup fix is that findConvoInAllQueries now falls back to pinned data, whereas this separate aggregate still neither scans nor subscribes to pinnedConversations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the aggregation scans pinnedConversations as well and subscribes to that key, deduping against the chats pages. Readiness still keys on the chats list, which is where the backlog arrives.
| /** Only reached once the assistant message above has been persisted; drives the | ||
| * unseen-reply indicator the same way BaseClient's reply path does. */ | ||
| ...(req?.body?.isTemporary !== true && { lastResponseAt: new Date() }), |
There was a problem hiding this comment.
For both Assistants chat controllers, the final event and res.end() occur before saveAssistantMessage invokes this stamp. If a focused client at the bottom handles that final event and posts /seen before these database writes finish, the first reply does not match the observed-response filter at all, while an existing conversation can be marked seen only to receive this newer response stamp afterward. In either case a subsequent refetch resurrects the unread dot for a reply the user already viewed; persist the assistant message/stamp before publishing final or reconcile seen state after persistence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed at the client, not by reordering the stream. The root cause was the client inventing a stamp: useEventHandlers wrote serverConversation.lastResponseAt ?? new Date().toISOString(), so it could offer the server a browser-clock value the observed-response filter can never match. It now writes only a server-originated stamp and leaves the cache alone otherwise, with useReplyWatcher fetching the real one.
That makes the sequence converge rather than resurrect: the acknowledgement carries a real server stamp, and if a newer one lands the next refetch brings it, the seen trigger fires again with that newer value and succeeds. Each pass strictly advances lastResponseAt, so it terminates. I would rather not move the Assistants persistence ahead of final for this; that reorders the streaming lifecycle for a transient the client already settles.
| /* The default page is the newest conversations, which is where an unseen reply always | ||
| lands: the list sorts by `updatedAt` descending. */ | ||
| const { conversations } = await dataService.listConversations(); |
There was a problem hiding this comment.
When the document is unfocused and a resumed or terminal-abort path completes for a conversation that has fallen beyond the first page, this poll cannot discover the reply: those paths call stampConvoLastResponse, which explicitly uses timestamps: false, so the reply does not move the conversation into the updatedAt-sorted page fetched here. Unlike the previously discussed focused-tab polling tradeoff, this defeats the dedicated away poll even though notifications or the tab badge are enabled; poll by lastResponseAt, update updatedAt on these durable reply paths, or fetch enough data to cover their stamps.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at the source: stampConvoLastResponse no longer passes timestamps: false, so a persisted reply lifts the conversation exactly as BaseClient already does for the normal path. The away poll then finds it on the first page. markConvoSeen keeps timestamps: false, since reading must not reorder.
| return () => { | ||
| themeObserver.disconnect(); | ||
| for (const cancel of cancellations) { | ||
| cancel(); | ||
| } |
There was a problem hiding this comment.
When an authenticated user logs out while the unseen count is nonzero, ReplyNotifications unmounts, but this cleanup only cancels pending image work and leaves every favicon link pointing at the already-rendered badged data URL. The login page therefore continues showing the previous account's unread indicator until another authenticated mount repaints it; call restore() during cleanup as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the cleanup calls restore(), so signing out leaves the plain favicon rather than the previous account: unread count.
| if (reqCtx.isTemporary !== true) { | ||
| try { | ||
| await stampConvoLastResponse(reqCtx.userId, response.conversationId); |
There was a problem hiding this comment.
When the normal Stop endpoint wins the terminal claim, GenerationJobManager.abortJob persists the unfinished assistant row in the beforePublish callback in api/server/routes/agents/index.js, and this request path returns at the earlier !terminalClaim check. It therefore never reaches this stamp, so the common stopped-turn owner path saves a reply without changing lastResponseAt, despite the new code explicitly treating stopped replies as unseen-worthy. Stamp the conversation in the abort callback after its response write succeeds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the stamp now happens in the abort beforePublish callback, right after the response write succeeds and gated on the non-temporary case, so the common stopped-turn owner path reaches it. Kept best-effort so it cannot suppress the normal FINAL.
e24cbc6 to
01c8eaa
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01c8eaab55
ℹ️ 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".
| [ | ||
| { | ||
| $set: { | ||
| lastSeenAt: '$$REMOVE', | ||
| lastResponseAt: { $ifNull: ['$lastResponseAt', new Date()] }, |
There was a problem hiding this comment.
On Amazon DocumentDB, every POST /api/convos/unread request fails because this array argument is an aggregation-pipeline update and also uses $$REMOVE; the repository's packages/data-schemas/misc/documentdb/documentdb-compat.md explicitly records that pipeline-form updates are unsupported across the supported DocumentDB engines. Implement the atomic transition with compatible classic operators/CAS writes instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, that was a real regression against a documented constraint. Rewritten with classic operators only: a filtered findOneAndUpdate that matches solely a never-replied conversation does the conditional stamp, and anything already carrying a reply falls through to a plain $unset of the catch-up. No pipeline form and no $$REMOVE anywhere in the method now.
| }; | ||
| updateConvoInAllQueries(queryClient, vars.conversationId, (convo) => ({ | ||
| ...convo, | ||
| lastResponseAt: convo.lastResponseAt ?? new Date().toISOString(), |
There was a problem hiding this comment.
When a conversation has no prior reply, this optimistic value is created before the request, while markConvoUnread later stores a different server timestamp. Opening the chat then sends the earlier cached value to /seen, whose $lte filter does not match the newer server stamp under normal same-clock request latency, so the optimistic dot clears but returns on the next refetch. Return and cache the server's actual lastResponseAt, or avoid independently inventing this marker on the client.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed alongside the rewrite: markConvoUnread returns the stamp it settled on and the mutation caches it in onSuccess, so the optimistic guess never survives to be sent to /seen.
| /** Only reached once the assistant message above has been persisted; drives the | ||
| * unseen-reply indicator the same way BaseClient's reply path does. */ | ||
| ...(req?.body?.isTemporary !== true && { lastResponseAt: new Date() }), |
There was a problem hiding this comment.
When an OpenAI Assistants run is stopped, abortRun bypasses saveAssistantMessage and calls checkMessageGaps, whose syncMessages path persists the partial assistant message with recordMessage but saves the conversation without lastResponseAt. Consequently a stopped reply—especially one cancelled from another device or while its end is outside the viewport—never becomes unseen; apply the stamp after that synchronization succeeds as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: abortRun stamps after checkMessageGaps resolves, gated on the non-temporary case and best-effort so it cannot fail the abort response. That covers the cancelled-run partial that syncMessages persists without one.
| const { conversationId, lastResponseAt, lastSeenAt } = convo; | ||
| if (!conversationId || !lastResponseAt) { |
There was a problem hiding this comment.
When either watcher fetches a conversation completed on another device, this merge copies only the read-state stamps and leaves the cached updatedAt and row position unchanged, even though every reply stamp now advances updatedAt on the server. The changed setQueryData also refreshes the list query's dataUpdatedAt, so returning to the tab within its stale window may not refetch and the newly active conversation remains at its old date and position. Unlike the previously discussed focused-tab polling tradeoff, this occurs after the watcher successfully fetched the completed conversation; merge the server updatedAt and move the row to the top as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed, and it was a consequence of the updatedAt change you prompted last round. mergeTimestamps now carries the server updatedAt and repositions the row, but only when the server actually reports it moved, so a bare read-state merge still leaves the order alone. updateConvoInAllQueries and the pinned updater now keep an updatedAt a caller deliberately changed instead of overwriting it with the browser clock, matching what the sibling helpers in that file already did.
| updateConvoInAllQueries(queryClient, vars.conversationId, (convo) => ({ | ||
| ...convo, | ||
| lastSeenAt: context?.previous, | ||
| })); |
There was a problem hiding this comment.
When /seen rejects while the tab is focused and the user is near the bottom—for example while offline or during a server outage—this rollback makes the conversation unseen again, which emits a query-cache update that useConversationSeen immediately handles by starting the same mutation again. Each failure therefore schedules another request without delay or a retry limit. Fresh evidence beyond the earlier clock-skew fix is that the onError path itself restores the exact state that re-arms the subscription; suppress that trigger or use a bounded retry/backoff strategy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: useConversationSeen records the reply each conversation was acknowledged for and will not re-send the same pair, so the rollback can no longer re-arm itself. A genuinely newer reply releases the guard, as does refocusing the tab, which keeps a write that failed offline retryable at human pace. Three tests cover the storm, the newer-reply release, and the focus retry.
01c8eaa to
02fda22
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02fda229a5
ℹ️ 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".
| updateConvoInAllQueries(queryClient, vars.conversationId, (convo) => ({ | ||
| ...convo, | ||
| lastSeenAt: acknowledged, | ||
| })); |
There was a problem hiding this comment.
When a stale conversation-list refetch is already in flight—most notably when a user focuses the tab after the five-minute stale window—it can read the old lastSeenAt before /seen commits, then replace this optimistic cache update after the mutation succeeds. Because the mutation has no success reconciliation or invalidation, and useConversationSeen records the response stamp in attemptedRef, the cache remains unseen and the same reply is not acknowledged again until another focus or refetch. Cancel matching list queries before the optimistic write and reconcile or invalidate them on success. CLAUDE.mdL152-L156
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the mutation now settles against the server answer in onSuccess, re-applying the acknowledgement when it was accepted and restoring the real catch-up when the observed reply was no longer newest. It writes only when the cache actually drifted, so a normal pass costs nothing. I went with reconciliation rather than cancel plus invalidate: invalidating the list on every seen write would refetch it each time a user opens an unread chat, and the attempt guard from the last round keeps either outcome from re-arming the request.
| for (const query of queries) { | ||
| const data = queryClient.getQueryData<InfiniteData<ConversationCursorData>>(query.queryKey); | ||
| const found = findConversationInInfinite(data, conversationId); | ||
| if (found) { | ||
| return found; |
There was a problem hiding this comment.
When the same conversation is cached in multiple allConversations variants, such as the unfiltered list plus a project or tag filter, only the mounted variant may refetch and receive a newer lastResponseAt. This helper returns the first matching cache entry without comparing stamps, so if an older query was created first, useConversationSeen reads the stale copy as already seen and never posts /seen even while the active filtered row shows an unseen dot; readUnseen has the same first-copy behavior and can omit the reply from alerts and the badge. Reconcile duplicate entries or select the one with the newest response stamp.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: added freshestConvo, and both findConvoInAllQueries and readUnseen now pick the copy whose read state actually moved rather than the first one found. Reply stamp decides, catch-up breaks the tie, which works because both only move forward. readUnseen keys by conversation id instead of first-wins dedupe.
| const arrivals = unseen.filter( | ||
| (conversation) => | ||
| conversation.conversationId && | ||
| known.get(conversation.conversationId) !== conversation.lastResponseAt, |
There was a problem hiding this comment.
When another device marks a conversation unread while this tab is unfocused, the conversation was absent from known while seen and now enters unseen, so this comparison classifies it as a newly arrived reply and emits the configured chime or desktop notification. This also affects never-replied chats because /unread creates an artificial lastResponseAt, producing a misleading “Reply ready” notification despite no assistant response. Track actual reply activity separately—such as by retaining stamps for all cached conversations or using a dedicated reply marker—rather than treating every seen-to-unseen transition as an arrival.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed for the case that is fixable, and the residual is worth stating plainly.
knownRef now retains reply stamps instead of replacing the map each pass, so a conversation that was read and is later marked unread from another device comes back carrying the stamp it always had and is not treated as an arrival. A stamp that actually moved still alerts. Two tests cover both directions.
What that does not fix is your second point: for a conversation with no reply yet, /unread writes an artificial lastResponseAt, and nothing downstream can tell it apart from a first real reply. Retaining stamps does not help there, because the row was genuinely absent from the unseen set with no stamp before. That matters more than it sounds, since every conversation predating this feature has no lastResponseAt.
Removing that lie needs a dedicated marker on the schema, with isConversationUnseen reading it alongside the reply stamp and the seen path clearing it. That is a real design change touching the schema, both mutations, the observed-reply filter and their tests, and I would rather it get its own review than be appended to this one late in a review loop. Flagging it for Danny as the follow-up.
| const badge = activeCount > 0 ? `(${activeCount}) ` : ''; | ||
| const base = | ||
| writtenBadge !== '' && document.title.startsWith(writtenBadge) | ||
| ? document.title.slice(writtenBadge.length) | ||
| : document.title; |
There was a problem hiding this comment.
When the unseen count is 3 and the active conversation is renamed to a legitimate title such as (3) Notes, the title observer sees the new title starting with writtenBadge and assumes that prefix belongs to this hook. When the count changes or the hook unmounts, cleanup strips the prefix and leaves Notes, corrupting the browser title set by the rename handler. Distinguish mutations written by this hook from external title updates instead of identifying ownership solely by matching the prefix.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: ownership is now the exact title this hook last wrote rather than a prefix match, so a rename to "(3) Notes" while three replies are unread is left intact and cleanup no longer strips the user text. The badge composes onto it instead, which is honest about both. Test added for the case.
02fda22 to
bbd7706
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbd7706242
ℹ️ 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 data = queryClient.getQueryData<InfiniteData<ConversationCursorData>>(query.queryKey); | ||
| freshest = freshestConvo(freshest, findConversationInInfinite(data, conversationId)); | ||
| } | ||
| return freshestConvo(freshest, findPinnedConversation(queryClient, conversationId)); |
There was a problem hiding this comment.
When an old conversation exists only in the pinned cache and is cached under multiple tag variants, an unmounted variant can retain an older read state while the mounted variant receives a newer lastResponseAt. Fresh evidence beyond the earlier fix is that this line still receives only the first match returned by findPinnedConversation, whereas the allConversations loop and readUnseen compare every variant; consequently useConversationSeen can read the stale pin as already seen and never acknowledge the reply whose dot is visible. Reduce all matching pinned variants through freshestConvo as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: findPinnedConversation reduces its variants through freshestConvo too, so the pinned side matches the chats side instead of returning the first match. Test covers a pin cached under two bookmark filters where only one carries the newer reply.
| /** Only reached once the assistant message above has been persisted; drives the | ||
| * unseen-reply indicator the same way BaseClient's reply path does. */ | ||
| ...(req?.body?.isTemporary !== true && { lastResponseAt: new Date() }), |
There was a problem hiding this comment.
When an Assistants run errors after OpenAI produced output that is missing locally, both chatV1's error handler and controllers/assistants/errors.js call checkMessageGaps; its syncMessages path persists the missing assistant row through recordMessage, but saves the conversation without lastResponseAt and then returns without reaching this success-path stamp. If the user left while the run was active, that durable partial/error response therefore produces no unseen indicator or alert; stamp the conversation when error recovery actually synchronizes assistant output, as the abort path now does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed, and consolidated rather than patched per call site. The stamp now lives in syncMessages itself, gated on that synchronization actually having recorded an assistant message, so it covers the abort path and both error handlers at once. checkMessageGaps always reaches syncMessages, so the abort-specific stamp I added last round was redundant and is gone.
| const unsubscribe = queryClient.getQueryCache().subscribe((event) => { | ||
| if (event?.query?.queryKey?.[0] !== QueryKeys.allConversations) { | ||
| return; | ||
| } | ||
| markSeenIfCaughtUp(); |
There was a problem hiding this comment.
On a direct reload of an old pinned conversation that is absent from the loaded Chats pages, the messages-end observer can report before either list resolves. If allConversations then resolves before the pinned query, this subscription performs one still-missing lookup, but the later pinnedConversations cache event is ignored, so no remaining trigger posts /seen and the open conversation's dot stays stuck. Fresh evidence beyond the earlier pinned lookup fix is that the helper now searches pinned data, while this subscriber still listens only to the Chats key; accept both query roots.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the subscription accepts both query roots now, matching the lookup it feeds. That was a gap left by the pinned lookup fix, same as the one you caught in the aggregate hook earlier.
| notification.onclick = () => { | ||
| window.focus(); | ||
| navigate(`/c/${conversation.conversationId}`); |
There was a problem hiding this comment.
When replies arrive to both the currently open conversation and another chat while the window is unfocused, clicking the other chat's desktop notification calls window.focus() before navigating. The focus listener in useConversationSeen consequently acknowledges the currently open conversation if it was near the bottom, even though navigation happens in the same click and the user never sees that reply; its unseen indicator is silently cleared. Suppress that focus acknowledgement during notification navigation or focus only after the destination conversation is active.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: added a small notificationNavigation module next to the two hooks. The notification click arms a one-shot suppression before focusing, and the seen trigger consumes it instead of acknowledging whatever was still open behind the notification. Navigation now runs before window.focus() as well, so the conversation id has already changed by the time focus lands.
The flag is only armed while the window is unfocused, which is the only state alerts fire in, so a click that raises no focus event cannot leave it set for a later genuine one. Two tests: the suppressed focus, and the next real focus still acknowledging.
| (current) => ({ | ||
| ...current, | ||
| lastResponseAt, | ||
| lastSeenAt, | ||
| updatedAt: updatedAt ?? current.updatedAt, |
There was a problem hiding this comment.
When a remote reply lands in the conversation currently open at the bottom, this watcher merges only the conversation timestamps and never refreshes that conversation's messages. The resulting list-cache event—or the next focus event after an away poll—makes useConversationSeen reuse the still-true near-bottom state from the old message tree and post /seen, so the server acknowledges a reply that was never rendered in this tab. Refresh the message cache before exposing the new stamp to the seen trigger, or bind acknowledgement to a response that is actually present in the rendered message list.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the watcher marks that conversation's messages stale before exposing the new stamp, so the open conversation refetches and renders the reply it is about to be credited with. Anywhere else the messages query is unmounted and it costs nothing. Ordering matters here, which is why the invalidation runs ahead of the merge rather than after it.
bbd7706 to
a7b6ee8
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7b6ee89c7
ℹ️ 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".
| belongs to the previous message. Marking the messages stale first means the conversation | ||
| the user is looking at refetches and shows the reply it is about to be credited with; | ||
| anywhere else the query is unmounted, so this costs nothing. */ | ||
| queryClient.invalidateQueries([QueryKeys.messages, conversationId]); |
There was a problem hiding this comment.
When a remote reply arrives for the currently open conversation while it remains near the bottom, invalidateQueries only marks the existing message data stale and starts an asynchronous refetch; it does not render the new reply before the synchronous list update below notifies useConversationSeen. The hook can therefore post /seen using the old message tree and silently clear a reply the user has not seen. Fresh evidence beyond the earlier comment is that this invalidation is neither awaited nor tied to successful message data arrival; await the refetch before merging the stamp or gate acknowledgement on the stamped response being present in the rendered messages. CLAUDE.mdL152-L156
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair, the invalidation being unawaited left the ordering unfixed. mergeTimestamps is async now and awaits the invalidation, which in React Query 4 resolves once the active refetch has landed, so the open conversation is rendering the reply before the stamp reaches the seen trigger. Where the messages query is unmounted nothing is fetched and it resolves immediately, so the away poll does not pay for it.
| if (reqCtx.isTemporary !== true) { | ||
| try { | ||
| await stampConvoLastResponse(reqCtx.userId, response.conversationId); |
There was a problem hiding this comment.
For an ordinary non-temporary resumable agent completion, response.databasePromise has already run BaseClient.saveMessageToDatabase, which stamps lastResponseAt and returns the conversation snapshot used in the final event. This unconditional second write creates a newer server stamp after that snapshot: the client acknowledges the older final-event value, /seen rejects it against the newer value, and useReplyWatcher skips fetching the correction because the cached stamp already changed since the job started. Consequently a reply visibly read at the bottom can retain its unread dot until an unrelated refetch; gate this stamp to terminal paths that actually bypassed BaseClient persistence or refresh the published conversation after stamping.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and this was mine from an earlier round: the guard said "stopped turns" but the condition was only the temporary check, so ordinary completions got a second, later stamp than the one already in the final-event snapshot. Now gated on responseIsUnfinished, which is exactly the set of turns BaseClient did not write a completed response for.
| await Conversation.updateOne( | ||
| { conversationId, user }, | ||
| { $set: { lastResponseAt: new Date() } }, | ||
| ); |
There was a problem hiding this comment.
When a direct-save path such as finalizeResumedTurn completes a reply in a project conversation, this write advances the conversation's updatedAt but bypasses the project-stat maintenance performed by saveConvo. The projects UI sorts by ChatProject.lastConversationAt, so a reply finishing after a long approval pause can lift the conversation while leaving its project at the old activity time and position even after the client invalidates the projects query. Return the updated conversation and reconcile its project activity, or route this write through the existing project-stat-aware path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the stamp reads the conversation back and routes its project through updateChatProjectLastConversationForUser, the same maintenance saveConvo does, so a reply landing on a project chat lifts the project with it. This one followed from making the stamp move updatedAt at all, which was the earlier change.
| updateConvoInAllQueries(queryClient, vars.conversationId, (convo) => ({ | ||
| ...convo, | ||
| lastResponseAt, | ||
| })); |
There was a problem hiding this comment.
When a conversation-list or pinned-list refetch is already in flight, it can read the old lastSeenAt before /unread commits and replace the optimistic cache state after this mutation starts. This success reconciliation writes only lastResponseAt, so it preserves the stale refetch's lastSeenAt; the server has successfully marked the conversation unread, but the row loses its dot until another refetch. Cancel matching queries before the optimistic update and explicitly settle lastSeenAt to undefined on success, or invalidate the affected queries after the mutation. CLAUDE.mdL152-L156
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: onSuccess now settles lastSeenAt to undefined alongside the stamp rather than writing the stamp alone, so a refetch that committed the old catch-up cannot quietly take the dot back off. It still writes only when the cache actually drifted. Two tests: the refetch race, and the server marker being cached for a conversation that had no reply.
a7b6ee8 to
04f003b
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04f003bcd6
ℹ️ 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".
| if (!isNearBottomRef.current || !document.hasFocus()) { | ||
| return; | ||
| } | ||
| const cached = findConvoInAllQueries(queryClient, conversationId); |
There was a problem hiding this comment.
When an old unseen conversation is opened directly and is absent from the loaded conversation-list pages, ChatRoute still loads it into [QueryKeys.conversation, conversationId], but this lookup searches only list and pinned caches. The viewport can therefore be focused at the bottom while every trigger treats the conversation as missing and never posts /seen, leaving the reply unread on the server and on other devices. Include the active point-query copy when resolving the conversation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the lookup now also considers the [QueryKeys.conversation, id] point query, reduced through the same freshness comparison so a list copy carrying a newer reply still wins. getConvo returns the whole document, so that payload does carry both stamps. Two tests: the point query as the only source, and the list copy winning when it is newer.
| if (hasNewlyUnknownConversation) { | ||
| queryClient.invalidateQueries([QueryKeys.allConversations]); |
There was a problem hiding this comment.
When a tag or search filter is active and another device creates or replies to a conversation absent from prior caches, this invalidation refetches only the mounted filtered list, which may still exclude that conversation; the polled conversation is discarded, and unknownIdsRef then suppresses subsequent invalidations for the same ID. As a result, the global badge, chime, and desktop notification never receive that reply until the filter or cache state changes. Preserve or fetch unknown rows independently of the active sidebar filter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one, and the code already documents the limitation it describes.
The mechanism is right: with only a filtered list cached, invalidating allConversations refetches a list that can still exclude the conversation, and unknownIdsRef then stops re-invalidating for it. That guard exists deliberately, or the poll would refetch the filtered list every tick for a conversation that can never enter it.
The fix you suggest means keeping unseen state for conversations that are in no cached list, which is a second source of truth alongside the cache-derived aggregate, with no way to tell when the user has read those entries since they are in no cache to observe. That is a meaningful amount of new state for a case that needs the user to be away, with a tag or search filter active, and a reply arriving on a conversation the filter excludes.
The robust answer is the same server-side unseen source that would fix the manual-unread count I flagged earlier. I would rather Danny take that as one deliberate follow-up than have me approximate it here.
| } | ||
| const result = await Conversation.updateOne( | ||
| filter, | ||
| { $set: { lastSeenAt: new Date() } }, |
There was a problem hiding this comment.
In a multi-replica deployment where the node that persisted the reply is ahead of the node handling /seen, the observed-response filter succeeds but this node writes an earlier lastSeenAt. A later refetch therefore still satisfies lastSeenAt < lastResponseAt even though the route returned modified: true, resurrecting the unread indicator. Set the catch-up stamp to at least observedResponseAt (or atomically copy the matched response stamp) rather than generating another independently clocked timestamp.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good one, and cheap to make airtight: the write clamps the catch-up so it is never earlier than the reply being acknowledged. The filter already guarantees the stored response is no newer than what the client observed, so max(now, observed) leaves the normal single-node case writing a real "now" while a replica running behind can no longer produce a catch-up that reads as unseen on the next refetch. Test added with an observed stamp deliberately ahead of the writing node.
| const { data: activeJobsData } = useActiveJobs(); | ||
| const activeJobIds = activeJobsData?.activeJobIds; |
There was a problem hiding this comment.
When this query has most recently returned an empty job set, useActiveJobs disables its interval, so a job started on another device while this window remains focused is never observed; the away-list poll also returns immediately because the document has focus. The completion transition described by this hook therefore never occurs, and even a cached conversation receives no new dot until an unrelated focus or list refetch. Keep a discovery poll running while focused or use another signal that can announce remotely started jobs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is the same finding as the foreground-poll one from the first review round, and my answer is unchanged, so I will state it once more and leave it open rather than keep going around.
The mechanics are accurate: useActiveJobs returns false from refetchInterval once the set is empty, and the away poll returns early while the document has focus. What I disagree with is the remedy. This feature is about replies that land while you are away; a discovery poll that runs while focused adds a recurring request for every user, forever, to shave latency off a case where the user is already looking at the app and the sidebar refetches on the next focus or list refresh anyway. That is the one cost this design was explicitly built to avoid.
Flagging it as an open disagreement for Danny rather than resolving it either way. If he wants foreground discovery, the honest version is a server-side unseen source rather than a poll, which is the same thing I suggested for the unseen-count point earlier.
04f003b to
bac102d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bac102d856
ℹ️ 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".
| updateConvoInAllQueries( | ||
| queryClient, | ||
| conversationId, |
There was a problem hiding this comment.
When an old conversation opened by direct URL exists only in [QueryKeys.conversation, id], findConvoInAllQueries now finds it, so both watcher paths classify it as known and call this merge. However, updateConvoInAllQueries updates only list and pinned queries, meaning the fetched reply stamp is discarded when neither contains the row; the newly rendered remote reply therefore cannot trigger /seen and remains unread until an unrelated list refetch. Update the point-query copy as part of this merge. This is fresh evidence beyond the earlier point-lookup fix: reads include the point cache, but this write still excludes it. CLAUDE.mdL152-L156
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed, and you are right that the asymmetry was the bug: I added the point query to reads last round and left writes behind. updateConvoInAllQueries now applies the updater to the point copy as well. It applies the updater to that copy rather than writing a list row over it, since the point cache carries fields the list rows do not, messages among them. Test asserts both the stamp landing and messages surviving.
| /** Drives the unseen-reply indicator; only assistant replies count, never the user's own turn. */ | ||
| if (message.isCreatedByUser === false && reqCtx.isTemporary !== true) { | ||
| fieldsToKeep.lastResponseAt = new Date(); |
There was a problem hiding this comment.
In a multi-replica deployment with clock skew, a reply persisted later on a slower node can overwrite lastResponseAt with a value earlier than the previous reply or lastSeenAt. Since unseen state is determined solely by timestamp ordering, that genuinely new reply is then treated as already seen and produces no dot or alert. Persist a per-conversation monotonic marker rather than replacing it directly with each node's wall-clock time; the direct stamping paths need the same protection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, but I am not taking it in this PR, and the reasoning is about cost rather than doubt.
I accepted the replica-skew argument last round for markConvoSeen, where the fix was a one-line clamp on a write this feature owns end to end. This one is not that shape. The common write is BaseClient putting lastResponseAt into its saveConvo payload, so making the marker monotonic there means changing saveConvo itself, which every conversation write in the app goes through. And as you note, the direct stamping paths need the same treatment.
$max alone would not actually buy the behaviour either: it stops the marker regressing below a previous reply, but a reply stamped below the existing lastSeenAt still reads as already seen, which is the case that matters. The invariant worth enforcing is that persisting a reply leaves lastResponseAt above lastSeenAt, and expressing that without pipeline updates means a read-then-write on every reply save.
Also worth being honest about the exposure: it needs skew large enough to cover the gap between one reply and the next, or between a reply and the user catching up. On NTP-synced hosts that is not reachable by human conversation timing; on hosts without it, it is.
Filing it with the other design item rather than patching the rarer paths and leaving the common one exposed. If Danny wants it, the coherent version is a per-conversation monotonic sequence rather than wall-clock timestamps.
| const settled = data.modified ? context?.acknowledged : context?.previous; | ||
| const cached = findConvoInAllQueries(queryClient, vars.conversationId); | ||
| if (!cached || cached.lastSeenAt === settled) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
When a second reply arrives while the /seen request for the first reply is still pending, the hook can start another mutation and optimistically acknowledge the newer stamp. If the older request then returns modified: false because the server already holds the newer reply, this reconciliation restores the older mutation's previous value and overwrites the newer acknowledgement; attemptedRef already records the newer stamp, so the conversation stays falsely unseen until another focus or refetch. Settle only while the cache still contains this mutation's own optimistic acknowledgement. CLAUDE.mdL152-L156
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: settling now requires either that the cache still holds this mutation own acknowledgement, or that the settle moves the catch-up forward. That keeps the refetch-clobber repair working while a late-returning request for an older reply can no longer take back a newer acknowledgement. Test drives both requests with the newer one landing first.
| if ( | ||
| cached && | ||
| cached.lastResponseAt === lastResponseAt && | ||
| cached.lastSeenAt === (lastSeenAt ?? undefined) && | ||
| (updatedAt === undefined || cached.updatedAt === updatedAt) |
There was a problem hiding this comment.
When two watcher requests for the same conversation resolve out of order—such as overlapping 30-second away polls or completion fetches for concurrent jobs—the older payload still passes this inequality check and is merged after the newer one. That regresses lastResponseAt and lastSeenAt, potentially removing or resurrecting an unread state until another poll or refetch repairs it. Compare the incoming reply stamp with the freshest cached stamp and ignore snapshots that move read state backward. CLAUDE.mdL152-L156
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: a snapshot whose reply stamp is older than the cached one is rejected outright. Whole snapshot rather than field-wise max, because the two stamps come from one server read and lastSeenAt legitimately clears when another device marks the conversation unread, so max-ing that field would hide the dot it is meant to raise.
bac102d to
97ad165
Compare
40e406e to
b617967
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b61796706a
ℹ️ 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 { message: savedMessage, conversation: persistedConversation } = | ||
| await saveAssistantMessage(req, { ...responseMessage, model }); | ||
| if (!savedMessage) { | ||
| throw new Error('Assistant response could not be persisted before final publication'); | ||
| } |
There was a problem hiding this comment.
Move this persistence barrier into a typed packages/api helper and leave both assistant controllers as call-through wiring. This new block makes a CJS controller own response/user-write sequencing, persistence-result validation, and final-payload shaping, and the same policy is duplicated in chatV2.js; the repository explicitly requires branches, validation, and service behavior under /api to live in packages/api.
AGENTS.md reference: AGENTS.md:L53-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3b9febccfc. The barrier (write before FINAL, check the row and the conversation both persisted, unwrap the snapshot) is settleAssistantFinal in packages/api/src/conversations/announce.ts; chatV1.js and chatV2.js now only call it. Existing assistant controller specs pass unchanged.
| return ( | ||
| hasPersistableAbortContent(reply.content) || | ||
| (typeof reply.text === 'string' && reply.text.trim().length > 0) |
There was a problem hiding this comment.
When a persisted agent reply consists only of attachments—for example, a resumed workspace/file artifact with empty content and text—the row is still renderable: resume.js persists accumulated attachments, and ContentParts explicitly renders attachment-only workspace changes. This predicate nevertheless returns false because PersistedReply carries no attachments, so no lastResponseAt is written and an away user receives no unseen dot, badge, chime, or desktop notification for that completed output; include renderable attachments in this centralized predicate and pass them through each announcement path.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3b9febccfc. PersistedReply carries attachments and isAnnounceableReply accepts a non-empty list; BaseClient, request.js, resume.js and the abort route pass them through. announce.spec.ts covers the attachment-only reply.
| /** The rows this turn already wrote, so the stamp write does not reload the whole history. */ | ||
| appendMessageIds?: SavedMessageId[]; |
There was a problem hiding this comment.
SavedMessageId is derived from ConversationMethods['saveConvo'], so this exported packages/api interface exposes Types.ObjectId[] and requires callers to pass storage-document _id values; TurnConversationWrite.savedMessageId in save.ts widens the same dependency. Define these public inputs as plain IDs and put any casting or append operation behind a narrow data-schemas method, rather than making consumers of packages/api depend on Mongoose's identifier type.
AGENTS.md reference: AGENTS.md:L59-L64
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3b9febccfc and 1d91b19b4e. StoppedReplyAnnouncement takes the ids as the caller holds them and normalizes to plain strings inside packages/api; saveConvo accepts string ids and casts them, so Types.ObjectId no longer appears in the packages/api contract. packages/api tsc clean, announce.spec.ts 26/26.
b617967 to
1d91b19
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d91b19b4e
ℹ️ 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".
1d91b19 to
b94d2c0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b94d2c0767
ℹ️ 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".
| if (!awaitingPermission) { | ||
| for (const [conversationId, lastResponseAt] of stamps) { | ||
| next.set(conversationId, lastResponseAt); | ||
| } |
There was a problem hiding this comment.
When the tab that owns the focus lease is killed or crashes without firing blur/pagehide, the lease remains valid for up to 60 seconds. If this background tab receives a reply during that window, this block records the stamp in knownRef before the focus guard suppresses the alert; lease expiry triggers no effect rerun, and later polls now treat the reply as known, so neither its notification nor its chime is ever delivered. Defer baselining arrivals suppressed only by another tab's lease, or schedule a reevaluation when that lease expires.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 471426e and 9213694. Arrivals held back only by another tab's focus lease are kept and rechecked when that lease lapses (a killed tab) or is released (a normal blur), and dropped once read or once this tab takes focus. useReplyAlerts.spec covers the lapse, the release, and a lease the focused tab keeps alive; the lapse and release cases fail without the fix.
| const timer = window.setInterval(() => { | ||
| if (document.hasFocus()) { | ||
| void refreshConversationLists(queryClient, [], pollLimit).catch(() => {}); | ||
| } |
There was a problem hiding this comment.
When an operator lowers interface.replyNotifications.pollLimit and the sidebar is showing a filtered list, this also truncates the canonical unfiltered refresh used while the tab is focused. If more than that many off-filter conversations receive replies between refreshes, the same newest rows occupy every discovery page, so older replied rows never enter an aggregate cache and remain absent from the global unseen count until the user removes the filter. Use the full server page bound for focused discovery or expose a separate focused-discovery limit instead of reusing the documented background-poll limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 471426e. pollLimit now bounds only the away poll; the focused refresh reads a full server page. useReplyWatcher.spec covers an operator limit of 10 with a focused tab and fails without the fix.
…nread A reply that lands while the user is elsewhere now leaves a trace. The conversation carries a durable reply stamp written with the assistant message that backs it, the sidebar row shows an unread dot until that message is actually rendered, and an optional tab badge, desktop notification and chime announce arrivals in a tab that is away. Rows can also be marked unread by hand from the row menu, in the archived view as well as the main list. The stamp is written by a compare-and-swap so concurrent replies order on the database rather than on the application hosts' clocks, and only once output that a reader can actually see is persisted: a stopped turn with nothing to read, an empty synchronized row, or a failed message write never lights a dot that cannot be cleared. Read intent survives archives, metadata edits and preset applies, and a later read always outranks a delayed unread.
Three review rounds kept finding the same two shapes, so both are answered once rather than patched again. The alert capabilities are now operator-configurable through `interface.replyNotifications` in `librechat.yaml`: the tab badge, desktop notifications and the chime each have a gate, and the away poll's page size is a field rather than a constant, with defaults that reproduce the shipped behavior. A capability the operator turns off reads as off whatever the device stored, and its settings toggle is not offered. Deciding whether a persisted turn may raise an indicator now lives in one place in `packages/api`, with the Responses API controller, the abort route, the Assistants thread sync and the client's override path all calling it instead of carrying their own copy. That closes the last case where a reply nobody can read lit a dot nothing could clear: a `/v1/responses` completion whose output held only reasoning or tool calls persists an empty row, and stamping it left an unread conversation that opening could not settle. Also: the retention backfill no longer advances `updatedAt` past a reply stamp, which an away tab reads as a metadata-only promotion and withholds the alert for; arrival evidence is swept of conversations no cached list still holds, so a long-lived tab stops growing with its lifetime number of replies; and generated Lighthouse profile directories are ignored where the runner leaves them.
The drawer became a mounted, sliding panel on canary, so the controls this helper read no longer say what it assumed. `close-sidebar-button` answers from the off-canvas copy before the expanded state commits, which returned the helper with the conversation list still hidden behind the pane, and the panel's own `new-chat-button` has a rail copy that a plain locator reaches first. Both readings passed on the previous drawer, which unmounted when closed. Settling on the close control's accessible name, and on the first candidate that is actually painted, is what a reader perceives and survives either drawer. One click is one toggle and the opener sits in the pane the drawer marks inert for the whole of its travel, so each attempt is given that travel to itself rather than reissued into it. Also serves `interface.replyNotifications` in the operator-gate scenario with a fresh body instead of the upstream response, since reusing its encoding headers for a re-serialized payload leaves the client unable to read the config, which looks exactly like an operator who configured nothing.
…g the poll limit Two gaps this round's own work left behind. `saveAssistantMessage` still decided to stamp from the message id alone, so an Assistants run that persisted an empty row raised a dot that opening the conversation could never clear. It now asks `isAnnounceableReply`, the same predicate every other persistence path asks, which is the point of having moved that decision into one place. `pollLimit` advertised a range up to 1000 while `getConvosByCursor` clamps every conversation read to a hundred-row page, so an operator who set 500 would have been told it applied and silently served a hundred. The schema now stops at the page the server will actually return; covering a deployment whose replies outpace one page needs the server-side unseen query, not a wider page.
…rting on Three scenarios opened the drawer on a page they only ever send a reply from. On the mobile project that drawer sits over the composer and marks the pane inert, so there was nothing left to type in and the run waited out its whole budget. They ask for the composer now, which is what they were always about. The two that read a row after leaving the list open it again first. A reply sent from another tab closes this one's drawer, because `sidebarExpanded` is stored per browser rather than per tab, and opening a chat closes it the way it does for a reader. Both are what someone would do to look at the row, and neither changes what is being asserted. Mark as unread is dispatched rather than clicked, like the row controls around it: the menu is portaled, and behind the drawer's scrim it inherits `pointer-events: none` and goes click-dead.
The portaled menu sits under the mobile drawer's scrim, which intercepts the pointer, so a real click retried until the budget ran out. The sibling scenarios and the row's own controls already dispatch; these two were the last real clicks.
…olved temporary state The capability gate treated a startup config that had not loaded yet the same as one that set nothing, so a device that stored "on" could badge, poll or notify in the window before an operator's `false` arrived. Nothing is permitted now until the config is in hand; only a loaded config that lacks the field reads as the shipped default, which is the case a backend predating the setting produces. The Responses API announcement read the temporary flag from the request body alone, while the conversation save beside it resolves it from the stored conversation first. A temporary chat restored or resolved on the server without the flag in the body would have been announced through the unread indicators; both completion branches now resolve it the same way the save does.
…ck failed claims Moving the announce decision into packages/api left three callers still deciding for themselves. The agents controller re-stamped an unfinished turn whose terminal persistence was skipped, and the resumed legacy turn stamped its directly saved row, both from the message id alone, so a preempted or stopped turn that persisted nothing readable lit a dot that could never be acknowledged. Both ask isAnnounceableReply now. The non-agents abort path carried an inline copy of the same predicate; it calls announceReply instead. Error turns keep their direct stamp: the error card is what renders them. A notification claim was written to shared storage before the notification was constructed, so a constructor that throws left the reply claimed in every tab with nothing ever shown. The claim is handed back when construction fails, removed only while it still names that exact stamp.
…k failed chimes The regular BaseClient save still stamped any assistant reply that persisted, readable or not, because the conversation write took a bare reply id. It takes the persisted reply now and asks isAnnounceableReply before it stamps, which leaves the error turns as the only direct stamps, and those are rendered by the error card. saveTurnConversation is exercised against a real store in both directions. announce.ts derives its write-context type locally so the two modules do not import each other. The chime claimed its replies before building the tones, so an output that failed while they were being scheduled left them claimed in every tab with nothing played. Those claims are handed back on failure, the same way a notification that could not be constructed now releases its own.
The away poll and the focused refresh were fixed at 30 seconds and five minutes, which are request-rate levers on the conversation list and nothing an operator could tune. Both are fields on `interface.replyNotifications` now, bounded and defaulting to the values they had, and the watcher reads them through the same capability hook as the rest of the block.
… no longer has The list merge carried three of the four read-state fields by hand and left out `lastResponseMessageId`, so an SSE update on an unseen conversation erased which branch the reply landed on, and a missing identity reads as visible. It uses the same `preserveReadState` helper as the other two merge paths now, and the test that covered the other three fields covers this one too. A local cache merge was only inspected on a list's first page for new arrivals. A title or oldest-first sort leaves a replied row where it is, so its first reply on a later page went into the alerts baseline without an arrival and its chime or notification was lost. Local merges scan every loaded page; server responses keep the first-page rule, and only rows already known are considered. An unread request that matched nothing means the conversation is gone: the server's owner-scoped update returns the row even for a no-op. The mutation rolled back the read fields and kept the row; it removes it from the caches now, as a delete does.
The fetch that settles a finished job had no teardown guard, unlike the away poll. Signing out or switching accounts unmounts the watcher, and a response still in flight for the previous session would then merge into the caches the next one reads. The result is dropped once the watcher has unmounted, tracked on unmount rather than in the jobs effect's cleanup, which re-runs on every change to the running set while its earlier completions are still wanted.
…unread no-matches Error turns were stamped in CJS: the error middleware checked eligibility, called the stamp and shaped the event's conversation snapshot itself, and the agents controller stamped its own. Both call announceErrorTurn now, and the skipped-persistence restamp goes through announceReply, so no stamp is issued directly from /api any more. The existing sendError tests pass against the real helper unchanged. An unread call that no longer owned the row dropped a server no-match with the rest of its stale answers, so a deleted conversation could be restored by a newer call's rollback. The no-match now evicts before ownership is consulted. It stays after the superseded check: a superseded call never reached the server, and its synthetic `modified: false` is not a no-match. Also: Settings offers the reply toggles only once the startup config has loaded, matching the capability hook; a cancelled reply-discovery snapshot is restarted even though nothing observes it; and the focused refresh is capped at the five minutes the discovery snapshot stays authoritative, since that refresh is what renews it.
…only replies Both assistant controllers carried the same persistence barrier: write the response, check the row and the conversation both persisted, and unwrap the settled conversation for the final event. That is behaviour, and it was duplicated in CJS; settleAssistantFinal owns it now and the controllers call it. Their existing tests pass unchanged. The announce predicate ignored attachments, so a reply made only of files, such as a resumed workspace artifact with empty content and text, raised no dot, badge or alert. The message body renders attachments and acknowledgement checks the body, so such a reply can be cleared as well as announced; every announce path now passes them through. The stopped-reply announcement took the storage engine's own id type in its exported signature. It takes plain string ids now, and saveConvo accepts them alongside its own, since the update casts them against the schema; the id type stays inside data-schemas.
The abort route filtered and stringified the ids of the rows it had written before handing them to announceStoppedReply. That is data shaping, and it sat in CJS; the announcement takes the ids as the caller holds them and drops the unwritten ones itself.
The v4 upgrade on canary changed the class order Prettier enforces; these three rows carried the v3 order.
…sed refresh at a full page A tab killed without firing blur or pagehide leaves its focus lease for up to a minute. A reply arriving under it was baselined and then suppressed, and nothing reran when the lease lapsed, so its chime and notification were lost. Arrivals held back only by another tab's lease are kept and rechecked when that lease expires; a lease the focused tab keeps refreshing still holds them. The focused refresh reused the away-poll limit, so an operator who lowered it also shrank the discovery page that keeps the unseen count complete behind a filtered list. The limit now bounds only the away poll.
A tab that blurs normally clears its lease long before the lapse, and a reply held back for it is due as soon as nobody is looking. Lease changes from other tabs now trigger the recheck, not only the expiry timer.
b94d2c0 to
9213694
Compare
…ead (#15025) * feat: add unseen reply indicators, reply notifications, and mark as unread A reply that lands while the user is elsewhere now leaves a trace. The conversation carries a durable reply stamp written with the assistant message that backs it, the sidebar row shows an unread dot until that message is actually rendered, and an optional tab badge, desktop notification and chime announce arrivals in a tab that is away. Rows can also be marked unread by hand from the row menu, in the archived view as well as the main list. The stamp is written by a compare-and-swap so concurrent replies order on the database rather than on the application hosts' clocks, and only once output that a reader can actually see is persisted: a stopped turn with nothing to read, an empty synchronized row, or a failed message write never lights a dot that cannot be cleared. Read intent survives archives, metadata edits and preset applies, and a later read always outranks a delayed unread. * fix: gate reply alerts on config and announce only readable replies Three review rounds kept finding the same two shapes, so both are answered once rather than patched again. The alert capabilities are now operator-configurable through `interface.replyNotifications` in `librechat.yaml`: the tab badge, desktop notifications and the chime each have a gate, and the away poll's page size is a field rather than a constant, with defaults that reproduce the shipped behavior. A capability the operator turns off reads as off whatever the device stored, and its settings toggle is not offered. Deciding whether a persisted turn may raise an indicator now lives in one place in `packages/api`, with the Responses API controller, the abort route, the Assistants thread sync and the client's override path all calling it instead of carrying their own copy. That closes the last case where a reply nobody can read lit a dot nothing could clear: a `/v1/responses` completion whose output held only reasoning or tool calls persists an empty row, and stamping it left an unread conversation that opening could not settle. Also: the retention backfill no longer advances `updatedAt` past a reply stamp, which an away tab reads as a metadata-only promotion and withholds the alert for; arrival evidence is swept of conversations no cached list still holds, so a long-lived tab stops growing with its lifetime number of replies; and generated Lighthouse profile directories are ignored where the runner leaves them. * test: open the sidebar by what the drawer paints, not by its test ids The drawer became a mounted, sliding panel on canary, so the controls this helper read no longer say what it assumed. `close-sidebar-button` answers from the off-canvas copy before the expanded state commits, which returned the helper with the conversation list still hidden behind the pane, and the panel's own `new-chat-button` has a rail copy that a plain locator reaches first. Both readings passed on the previous drawer, which unmounted when closed. Settling on the close control's accessible name, and on the first candidate that is actually painted, is what a reader perceives and survives either drawer. One click is one toggle and the opener sits in the pane the drawer marks inert for the whole of its travel, so each attempt is given that travel to itself rather than reissued into it. Also serves `interface.replyNotifications` in the operator-gate scenario with a fresh body instead of the upstream response, since reusing its encoding headers for a re-serialized payload leaves the client unable to read the config, which looks exactly like an operator who configured nothing. * fix: gate the assistant save on renderable output and stop overselling the poll limit Two gaps this round's own work left behind. `saveAssistantMessage` still decided to stamp from the message id alone, so an Assistants run that persisted an empty row raised a dot that opening the conversation could never clear. It now asks `isAnnounceableReply`, the same predicate every other persistence path asks, which is the point of having moved that decision into one place. `pollLimit` advertised a range up to 1000 while `getConvosByCursor` clamps every conversation read to a hundred-row page, so an operator who set 500 would have been told it applied and silently served a hundred. The schema now stops at the page the server will actually return; covering a deployment whose replies outpace one page needs the server-side unseen query, not a wider page. * test: let the mobile scenarios use the surface they are actually asserting on Three scenarios opened the drawer on a page they only ever send a reply from. On the mobile project that drawer sits over the composer and marks the pane inert, so there was nothing left to type in and the run waited out its whole budget. They ask for the composer now, which is what they were always about. The two that read a row after leaving the list open it again first. A reply sent from another tab closes this one's drawer, because `sidebarExpanded` is stored per browser rather than per tab, and opening a chat closes it the way it does for a reader. Both are what someone would do to look at the row, and neither changes what is being asserted. Mark as unread is dispatched rather than clicked, like the row controls around it: the menu is portaled, and behind the drawer's scrim it inherits `pointer-events: none` and goes click-dead. * test: dispatch the row menu's unread item like the controls around it The portaled menu sits under the mobile drawer's scrim, which intercepts the pointer, so a real click retried until the budget ran out. The sibling scenarios and the row's own controls already dispatch; these two were the last real clicks. * fix: hold reply alerts until the deployment answers, and read the resolved temporary state The capability gate treated a startup config that had not loaded yet the same as one that set nothing, so a device that stored "on" could badge, poll or notify in the window before an operator's `false` arrived. Nothing is permitted now until the config is in hand; only a loaded config that lacks the field reads as the shipped default, which is the case a backend predating the setting produces. The Responses API announcement read the temporary flag from the request body alone, while the conversation save beside it resolves it from the stored conversation first. A temporary chat restored or resolved on the server without the flag in the body would have been announced through the unread indicators; both completion branches now resolve it the same way the save does. * fix: route the last reply stamps through the shared owner and hand back failed claims Moving the announce decision into packages/api left three callers still deciding for themselves. The agents controller re-stamped an unfinished turn whose terminal persistence was skipped, and the resumed legacy turn stamped its directly saved row, both from the message id alone, so a preempted or stopped turn that persisted nothing readable lit a dot that could never be acknowledged. Both ask isAnnounceableReply now. The non-agents abort path carried an inline copy of the same predicate; it calls announceReply instead. Error turns keep their direct stamp: the error card is what renders them. A notification claim was written to shared storage before the notification was constructed, so a constructor that throws left the reply claimed in every tab with nothing ever shown. The claim is handed back when construction fails, removed only while it still names that exact stamp. * fix: judge the normal reply save by the shared predicate and hand back failed chimes The regular BaseClient save still stamped any assistant reply that persisted, readable or not, because the conversation write took a bare reply id. It takes the persisted reply now and asks isAnnounceableReply before it stamps, which leaves the error turns as the only direct stamps, and those are rendered by the error card. saveTurnConversation is exercised against a real store in both directions. announce.ts derives its write-context type locally so the two modules do not import each other. The chime claimed its replies before building the tones, so an output that failed while they were being scheduled left them claimed in every tab with nothing played. Those claims are handed back on failure, the same way a notification that could not be constructed now releases its own. * feat: let operators set the reply polling cadence The away poll and the focused refresh were fixed at 30 seconds and five minutes, which are request-rate levers on the conversation list and nothing an operator could tune. Both are fields on `interface.replyNotifications` now, bounded and defaulting to the values they had, and the watcher reads them through the same capability hook as the rest of the block. * fix: keep reply identity through list merges and drop rows the server no longer has The list merge carried three of the four read-state fields by hand and left out `lastResponseMessageId`, so an SSE update on an unseen conversation erased which branch the reply landed on, and a missing identity reads as visible. It uses the same `preserveReadState` helper as the other two merge paths now, and the test that covered the other three fields covers this one too. A local cache merge was only inspected on a list's first page for new arrivals. A title or oldest-first sort leaves a replied row where it is, so its first reply on a later page went into the alerts baseline without an arrival and its chime or notification was lost. Local merges scan every loaded page; server responses keep the first-page rule, and only rows already known are considered. An unread request that matched nothing means the conversation is gone: the server's owner-scoped update returns the row even for a no-op. The mutation rolled back the read fields and kept the row; it removes it from the caches now, as a delete does. * fix: discard a job completion that resolves after the watcher has gone The fetch that settles a finished job had no teardown guard, unlike the away poll. Signing out or switching accounts unmounts the watcher, and a response still in flight for the previous session would then merge into the caches the next one reads. The result is dropped once the watcher has unmounted, tracked on unmount rather than in the jobs effect's cleanup, which re-runs on every change to the running set while its earlier completions are still wanted. * fix: move error-turn stamps into packages/api and settle overlapping unread no-matches Error turns were stamped in CJS: the error middleware checked eligibility, called the stamp and shaped the event's conversation snapshot itself, and the agents controller stamped its own. Both call announceErrorTurn now, and the skipped-persistence restamp goes through announceReply, so no stamp is issued directly from /api any more. The existing sendError tests pass against the real helper unchanged. An unread call that no longer owned the row dropped a server no-match with the rest of its stale answers, so a deleted conversation could be restored by a newer call's rollback. The no-match now evicts before ownership is consulted. It stays after the superseded check: a superseded call never reached the server, and its synthetic `modified: false` is not a no-match. Also: Settings offers the reply toggles only once the startup config has loaded, matching the capability hook; a cancelled reply-discovery snapshot is restarted even though nothing observes it; and the focused refresh is capped at the five minutes the discovery snapshot stays authoritative, since that refresh is what renews it. * fix: settle the assistant final in packages/api, announce attachment-only replies Both assistant controllers carried the same persistence barrier: write the response, check the row and the conversation both persisted, and unwrap the settled conversation for the final event. That is behaviour, and it was duplicated in CJS; settleAssistantFinal owns it now and the controllers call it. Their existing tests pass unchanged. The announce predicate ignored attachments, so a reply made only of files, such as a resumed workspace artifact with empty content and text, raised no dot, badge or alert. The message body renders attachments and acknowledgement checks the body, so such a reply can be cleared as well as announced; every announce path now passes them through. The stopped-reply announcement took the storage engine's own id type in its exported signature. It takes plain string ids now, and saveConvo accepts them alongside its own, since the update casts them against the schema; the id type stays inside data-schemas. * fix: normalize the stopped reply's written ids in packages/api The abort route filtered and stringified the ids of the rows it had written before handing them to announceStoppedReply. That is data shaping, and it sat in CJS; the announcement takes the ids as the caller holds them and drops the unwritten ones itself. * style: order the conversation row classes for Tailwind v4 The v4 upgrade on canary changed the class order Prettier enforces; these three rows carried the v3 order. * fix: replay replies a dead tab's focus lease held back, keep the focused refresh at a full page A tab killed without firing blur or pagehide leaves its focus lease for up to a minute. A reply arriving under it was baselined and then suppressed, and nothing reran when the lease lapsed, so its chime and notification were lost. Arrivals held back only by another tab's lease are kept and rechecked when that lease expires; a lease the focused tab keeps refreshing still holds them. The focused refresh reused the away-poll limit, so an operator who lowered it also shrank the discovery page that keeps the unseen count complete behind a filtered list. The limit now bounds only the away poll. * fix: announce a held reply as soon as the focused tab releases its lease A tab that blurs normally clears its lease long before the lapse, and a reply held back for it is due as soon as nobody is looking. Lease changes from other tabs now trigger the recheck, not only the expiry timer.
…ead (#15025) * feat: add unseen reply indicators, reply notifications, and mark as unread A reply that lands while the user is elsewhere now leaves a trace. The conversation carries a durable reply stamp written with the assistant message that backs it, the sidebar row shows an unread dot until that message is actually rendered, and an optional tab badge, desktop notification and chime announce arrivals in a tab that is away. Rows can also be marked unread by hand from the row menu, in the archived view as well as the main list. The stamp is written by a compare-and-swap so concurrent replies order on the database rather than on the application hosts' clocks, and only once output that a reader can actually see is persisted: a stopped turn with nothing to read, an empty synchronized row, or a failed message write never lights a dot that cannot be cleared. Read intent survives archives, metadata edits and preset applies, and a later read always outranks a delayed unread. * fix: gate reply alerts on config and announce only readable replies Three review rounds kept finding the same two shapes, so both are answered once rather than patched again. The alert capabilities are now operator-configurable through `interface.replyNotifications` in `librechat.yaml`: the tab badge, desktop notifications and the chime each have a gate, and the away poll's page size is a field rather than a constant, with defaults that reproduce the shipped behavior. A capability the operator turns off reads as off whatever the device stored, and its settings toggle is not offered. Deciding whether a persisted turn may raise an indicator now lives in one place in `packages/api`, with the Responses API controller, the abort route, the Assistants thread sync and the client's override path all calling it instead of carrying their own copy. That closes the last case where a reply nobody can read lit a dot nothing could clear: a `/v1/responses` completion whose output held only reasoning or tool calls persists an empty row, and stamping it left an unread conversation that opening could not settle. Also: the retention backfill no longer advances `updatedAt` past a reply stamp, which an away tab reads as a metadata-only promotion and withholds the alert for; arrival evidence is swept of conversations no cached list still holds, so a long-lived tab stops growing with its lifetime number of replies; and generated Lighthouse profile directories are ignored where the runner leaves them. * test: open the sidebar by what the drawer paints, not by its test ids The drawer became a mounted, sliding panel on canary, so the controls this helper read no longer say what it assumed. `close-sidebar-button` answers from the off-canvas copy before the expanded state commits, which returned the helper with the conversation list still hidden behind the pane, and the panel's own `new-chat-button` has a rail copy that a plain locator reaches first. Both readings passed on the previous drawer, which unmounted when closed. Settling on the close control's accessible name, and on the first candidate that is actually painted, is what a reader perceives and survives either drawer. One click is one toggle and the opener sits in the pane the drawer marks inert for the whole of its travel, so each attempt is given that travel to itself rather than reissued into it. Also serves `interface.replyNotifications` in the operator-gate scenario with a fresh body instead of the upstream response, since reusing its encoding headers for a re-serialized payload leaves the client unable to read the config, which looks exactly like an operator who configured nothing. * fix: gate the assistant save on renderable output and stop overselling the poll limit Two gaps this round's own work left behind. `saveAssistantMessage` still decided to stamp from the message id alone, so an Assistants run that persisted an empty row raised a dot that opening the conversation could never clear. It now asks `isAnnounceableReply`, the same predicate every other persistence path asks, which is the point of having moved that decision into one place. `pollLimit` advertised a range up to 1000 while `getConvosByCursor` clamps every conversation read to a hundred-row page, so an operator who set 500 would have been told it applied and silently served a hundred. The schema now stops at the page the server will actually return; covering a deployment whose replies outpace one page needs the server-side unseen query, not a wider page. * test: let the mobile scenarios use the surface they are actually asserting on Three scenarios opened the drawer on a page they only ever send a reply from. On the mobile project that drawer sits over the composer and marks the pane inert, so there was nothing left to type in and the run waited out its whole budget. They ask for the composer now, which is what they were always about. The two that read a row after leaving the list open it again first. A reply sent from another tab closes this one's drawer, because `sidebarExpanded` is stored per browser rather than per tab, and opening a chat closes it the way it does for a reader. Both are what someone would do to look at the row, and neither changes what is being asserted. Mark as unread is dispatched rather than clicked, like the row controls around it: the menu is portaled, and behind the drawer's scrim it inherits `pointer-events: none` and goes click-dead. * test: dispatch the row menu's unread item like the controls around it The portaled menu sits under the mobile drawer's scrim, which intercepts the pointer, so a real click retried until the budget ran out. The sibling scenarios and the row's own controls already dispatch; these two were the last real clicks. * fix: hold reply alerts until the deployment answers, and read the resolved temporary state The capability gate treated a startup config that had not loaded yet the same as one that set nothing, so a device that stored "on" could badge, poll or notify in the window before an operator's `false` arrived. Nothing is permitted now until the config is in hand; only a loaded config that lacks the field reads as the shipped default, which is the case a backend predating the setting produces. The Responses API announcement read the temporary flag from the request body alone, while the conversation save beside it resolves it from the stored conversation first. A temporary chat restored or resolved on the server without the flag in the body would have been announced through the unread indicators; both completion branches now resolve it the same way the save does. * fix: route the last reply stamps through the shared owner and hand back failed claims Moving the announce decision into packages/api left three callers still deciding for themselves. The agents controller re-stamped an unfinished turn whose terminal persistence was skipped, and the resumed legacy turn stamped its directly saved row, both from the message id alone, so a preempted or stopped turn that persisted nothing readable lit a dot that could never be acknowledged. Both ask isAnnounceableReply now. The non-agents abort path carried an inline copy of the same predicate; it calls announceReply instead. Error turns keep their direct stamp: the error card is what renders them. A notification claim was written to shared storage before the notification was constructed, so a constructor that throws left the reply claimed in every tab with nothing ever shown. The claim is handed back when construction fails, removed only while it still names that exact stamp. * fix: judge the normal reply save by the shared predicate and hand back failed chimes The regular BaseClient save still stamped any assistant reply that persisted, readable or not, because the conversation write took a bare reply id. It takes the persisted reply now and asks isAnnounceableReply before it stamps, which leaves the error turns as the only direct stamps, and those are rendered by the error card. saveTurnConversation is exercised against a real store in both directions. announce.ts derives its write-context type locally so the two modules do not import each other. The chime claimed its replies before building the tones, so an output that failed while they were being scheduled left them claimed in every tab with nothing played. Those claims are handed back on failure, the same way a notification that could not be constructed now releases its own. * feat: let operators set the reply polling cadence The away poll and the focused refresh were fixed at 30 seconds and five minutes, which are request-rate levers on the conversation list and nothing an operator could tune. Both are fields on `interface.replyNotifications` now, bounded and defaulting to the values they had, and the watcher reads them through the same capability hook as the rest of the block. * fix: keep reply identity through list merges and drop rows the server no longer has The list merge carried three of the four read-state fields by hand and left out `lastResponseMessageId`, so an SSE update on an unseen conversation erased which branch the reply landed on, and a missing identity reads as visible. It uses the same `preserveReadState` helper as the other two merge paths now, and the test that covered the other three fields covers this one too. A local cache merge was only inspected on a list's first page for new arrivals. A title or oldest-first sort leaves a replied row where it is, so its first reply on a later page went into the alerts baseline without an arrival and its chime or notification was lost. Local merges scan every loaded page; server responses keep the first-page rule, and only rows already known are considered. An unread request that matched nothing means the conversation is gone: the server's owner-scoped update returns the row even for a no-op. The mutation rolled back the read fields and kept the row; it removes it from the caches now, as a delete does. * fix: discard a job completion that resolves after the watcher has gone The fetch that settles a finished job had no teardown guard, unlike the away poll. Signing out or switching accounts unmounts the watcher, and a response still in flight for the previous session would then merge into the caches the next one reads. The result is dropped once the watcher has unmounted, tracked on unmount rather than in the jobs effect's cleanup, which re-runs on every change to the running set while its earlier completions are still wanted. * fix: move error-turn stamps into packages/api and settle overlapping unread no-matches Error turns were stamped in CJS: the error middleware checked eligibility, called the stamp and shaped the event's conversation snapshot itself, and the agents controller stamped its own. Both call announceErrorTurn now, and the skipped-persistence restamp goes through announceReply, so no stamp is issued directly from /api any more. The existing sendError tests pass against the real helper unchanged. An unread call that no longer owned the row dropped a server no-match with the rest of its stale answers, so a deleted conversation could be restored by a newer call's rollback. The no-match now evicts before ownership is consulted. It stays after the superseded check: a superseded call never reached the server, and its synthetic `modified: false` is not a no-match. Also: Settings offers the reply toggles only once the startup config has loaded, matching the capability hook; a cancelled reply-discovery snapshot is restarted even though nothing observes it; and the focused refresh is capped at the five minutes the discovery snapshot stays authoritative, since that refresh is what renews it. * fix: settle the assistant final in packages/api, announce attachment-only replies Both assistant controllers carried the same persistence barrier: write the response, check the row and the conversation both persisted, and unwrap the settled conversation for the final event. That is behaviour, and it was duplicated in CJS; settleAssistantFinal owns it now and the controllers call it. Their existing tests pass unchanged. The announce predicate ignored attachments, so a reply made only of files, such as a resumed workspace artifact with empty content and text, raised no dot, badge or alert. The message body renders attachments and acknowledgement checks the body, so such a reply can be cleared as well as announced; every announce path now passes them through. The stopped-reply announcement took the storage engine's own id type in its exported signature. It takes plain string ids now, and saveConvo accepts them alongside its own, since the update casts them against the schema; the id type stays inside data-schemas. * fix: normalize the stopped reply's written ids in packages/api The abort route filtered and stringified the ids of the rows it had written before handing them to announceStoppedReply. That is data shaping, and it sat in CJS; the announcement takes the ids as the caller holds them and drops the unwritten ones itself. * style: order the conversation row classes for Tailwind v4 The v4 upgrade on canary changed the class order Prettier enforces; these three rows carried the v3 order. * fix: replay replies a dead tab's focus lease held back, keep the focused refresh at a full page A tab killed without firing blur or pagehide leaves its focus lease for up to a minute. A reply arriving under it was baselined and then suppressed, and nothing reran when the lease lapsed, so its chime and notification were lost. Arrivals held back only by another tab's lease are kept and rechecked when that lease expires; a lease the focused tab keeps refreshing still holds them. The focused refresh reused the away-poll limit, so an operator who lowered it also shrank the discovery page that keeps the unseen count complete behind a filtered list. The limit now bounds only the away poll. * fix: announce a held reply as soon as the focused tab releases its lease A tab that blurs normally clears its lease long before the lapse, and a reply held back for it is due as soon as nobody is looking. Lease changes from other tabs now trigger the recheck, not only the expiry timer.
Summary
Conversations now track read state with two server-stamped timestamps:
lastResponseAt, advanced monotonically with a database compare-and-set whenever an assistant reply is persisted, andlastSeenAt, set when the user catches up with the newest message (POST /api/convos/seen, idempotent, ownership-checked, andtimestamps: falseso reading never reorders the sidebar). The client derives the unseen state from those timestamps with no extra request.That state drives a dot on the conversation row (folded into the row's accessible name), a count badge in the tab title plus a dot on every declared favicon, optional desktop notifications with a chime (three per-device toggles under General > Notifications, permission requested from the toggle's own click), a background watcher for replies that finish outside the tab, and Mark as unread in the row options menu (
POST /api/convos/unread).Reply stamping covers every persistence path through one owner:
announceReplyandannounceStoppedReplyinpackages/api/src/conversations/announce.tsdecide whether a persisted turn may raise an indicator, and the agents abort route, the Responses API controller, the Assistants thread sync andBaseClientcall it rather than each carrying the rule. A turn that persisted nothing a reader can open never lights a dot, which covers a stopped turn interrupted before its first token, a cancelled Assistants run, and a/v1/responsescompletion whose output held only reasoning or tool calls. Forks and duplicates start fresh, presets and imports strip the read-state fields, and older conversations are treated as read.What the alerts may do is an operator's call.
interface.replyNotificationsinlibrechat.yamlgates the tab badge, desktop notifications and the chime, and sets the away poll's page size and both polling cadences (pollLimit,pollIntervalMs,focusedRefreshMs, defaulting to 100 rows, 30 seconds and five minutes); the preferences themselves stay per device, because notification permission and audio output belong to the machine the reader is sitting at. A capability the operator turns off reads as off whatever that device stored, and its settings toggle is not offered, and nothing is offered until the startup config has loaded, so a stored "on" cannot badge or notify before an operator'sfalsearrives. The default chats list gains a{ user, updatedAt, _id }index for the away poll, which neither existingupdatedAtindex can serve.Change Type
Testing
reviewctl precheckagainstorigin/canary(the pull request's CI lanes run locally on the diff): static checks,api(5,572 tests),packages/api,client,data-provideranddata-schemasall pass. The full localapirun has order-dependent failures in three specs this pull request does not touch (AuthService,optionalShareFileAuth,loadAsyncEndpoints); each passes alone and in a rerun.reviewctl verify --all: fifteen acceptance scenarios, each run on desktop light, desktop dark and mobile, against a client bundle rebuilt from the head. They cover opening a chat clearing its dot and the title count, Mark as unread, the accessible name, a reply finishing in another tab, notification permission and withheld replies, clock-skewed stamps, a stale seen acknowledgement, metadata and preset edits, a stopped turn with nothing to read, read ordering, a reply on a hidden sibling branch, and an operator who disables the tab badge.announce.spec.ts),saveTurnConversationagainst a real in-memory Mongo in both directions, the Assistants and resumed-turn stamps, the Responses API, failed notification and chime claims being handed back, config readiness, the configured poll cadence, reply identity through list merges, later-page arrivals, and an unread request for a deleted conversation. Each was checked to fail without its fix.Test Configuration:
Node 24, MongoDB via mongodb-memory-server for the data-schemas and
packages/apispecs, jsdom for the client specs. The acceptance scenarios run in the Playwright mock harness with a per-run database and Redis prefix.Checklist