fix(site/src): treat chat deleted watch events as archive instead of eviction - #27921
fix(site/src): treat chat deleted watch events as archive instead of eviction#27921DanielleMaywood wants to merge 5 commits into
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 769b54971a
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9268285cd
ℹ️ 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".
| chat: TypesGen.Chat, | ||
| ) => { | ||
| void cancelChatListRefetches(queryClient); | ||
| void cancelLoadedChatEntityRefetch(queryClient, chat.id); |
There was a problem hiding this comment.
Reconcile lifecycle events during the initial detail fetch
When AgentChatPage has mounted but its first chat(agentId) request has not populated the cache, cancelLoadedChatEntityRefetch explicitly returns without canceling, and the following archive patch is a no-op on undefined. A GET whose database snapshot predates the archive commit can therefore resolve after this watch event with archived: false; because this path never invalidates the detail query, the page remains writable until an unrelated refetch. The created unarchive helper has the symmetric race because it only patches an already-cached archived entity. Seed the detail from the full watch payload or arrange a post-fetch invalidation so the initial loading state converges.
AGENTS.md reference: site/AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 241da75, with a correction to the suggested mechanism.
The race is real and this PR introduced it: pre-PR removeChatEntity destroyed the query and silently discarded the in-flight fetch, so the stale payload never landed; the patch-in-place path left the fetch untouched. A new semantic operation resetUnloadedChatEntity now covers both watch helpers: when the entity is absent, it resets the exact entity query; the cached path keeps the existing cancel-and-patch behavior unchanged.
On the proposed fix: invalidateChatEntity does not close this race. Verified empirically against the installed TanStack v5.82.0 build: invalidateQueries only cancel-and-restarts when state.data !== undefined (query.fetch, the cancel branch); an in-flight initial fetch has data === undefined, so invalidation returns the existing stale promise and the archived:false payload still wins (fetchCount stays 1). resetQueries is the mechanism that works: it silently cancels the stale fetch and refetches active queries, converging to the post-commit server state (fetchCount 2, final archived:true). It is a no-op when nothing is mounted, so there is no 404 hazard or fetch created for an unopened chat. Seeding setQueryData from the payload is also insufficient on its own because the in-flight GET overwrites the seed.
The symmetric unarchive case is fixed the same way, and the child created branch now recovers a mounted initial child fetch while unmounted sub-agent spawns stay no-ops. Tests pin the race end-to-end with a mounted QueryObserver and deferred first fetch for both directions, plus no-op guards.
Reply generated by Coder Agents.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 241da758ae
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…eviction The deleted watch event always means archive on the server (one event per family member), but the frontend evicted the entity and filtered list rows, causing a loading flash or zombie render on the open route and never recovering open archived tabs on unarchive. - deleted now patches archive state in place via applyWatchedChatArchived (cancel guards, entity/list/search/child patch, by-workspace removal, family invalidations) and keeps the route mounted - created detects unarchive via the cached entity and repairs caches; prepend skips lists whose archived filter conflicts with the chat - applyChatArchiveStateToCaches now also patches loaded search rows - removeHardDeletedChatFromCaches added as a distinct, unwired effect for a future hard-delete wire event (no event maps to it today)
…mily unarchive A created event for a child chat previously only added it to its parent children array, so a retained archived child entity stayed archived until an unrelated refetch. Apply the unarchive recovery when the cached child entity is archived; new sub-agent spawns skip it.
…fect No watch event or retention purge can invoke this helper today, and the archive fix does not use it. Remove the exported effect and its tests to keep the public cache API surface minimal; reintroduce it alongside a real hard-delete wire event. chatEntitiesFamilyKey stays because chatEntityKey builds on it.
04df0b6 to
d10251f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04df0b605f
ℹ️ 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 (row.id !== chatId) { | ||
| return row; | ||
| } | ||
| const patched = patchChatArchiveState(row, archived); |
There was a problem hiding this comment.
Remove chats from search caches with mismatched filters
When a chat changes archive state while search results are cached, this patches the row in every search query without checking that query's archived filter. Ordinary searches default to archived:false, while the dialog also supports archived:true, so archiving leaves an archived chat in active-only results and unarchiving leaves an active chat in archived-only results. The subsequent invalidation usually hides this after a successful refetch, but the incorrect row remains visible during the refetch and indefinitely if it fails; filter or remove rows whose query no longer accepts the new state.
AGENTS.md reference: site/AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d49af07, via removal rather than filter-aware patching.
The insight: a cached search row matched its query archived filter before the change, so after an archive-state change it belongs to a different result set entirely. In every non-degenerate filter the row no longer belongs in any query where it is cached. So the correct uniform behavior is to remove the row from every cached search, and let the search invalidations this PR already issues (invalidateChatSearches from the mutation and watch paths) repopulate any result set that still matches.
This is always correct, avoids parsing the q string client-side (which would duplicate the backend quote/colon tokenizer and its default-false semantics), and eliminates the stale-row window the comment described. The only behavioral cost is a brief row-disappearance flicker for the degenerate case where a query has no resolvable archived filter, which the refetch repairs.
Tests updated to the removal semantics: active-only searches lose the row on archive, archived-only searches lose it on unarchive, unrelated rows survive, and array references are preserved when the row was never cached.
Reply generated by Coder Agents.
An archive-state change flips whether a chat belongs to a search result's archived filter, so patching the row in place leaves it visible in results that no longer accept it. Remove the row from every cached search instead; the search invalidations issued by callers repopulate any result set that still matches.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Problem
The frontend treated the
deletedchat watch event as a hard delete: it evicted the open chat's entity (causing a loading flash or zombie render on the mounted route), filtered the chat out of every list cache includingarchived:truelists where it should be patched, skipped the cancellation guards, and missed the list-family invalidation. Unarchive never recovered an open archived tab because thecreatedevent only prepended list rows and never touched the cached entity.On the server,
deletedalways means archive:ArchiveChatand auto-archive publish onedeletedevent per family member with the full post-transition chat (archived: true), andUnarchiveChatpublishescreatedper member (chatd.go archive/unarchive publish paths, auto_archive.go). There is no chat DELETE API route; the onlyDELETE FROM chatsis the retention purge, which publishes no watch event. Archived chats remain readable server-side.Fix
deletednow runsapplyWatchedChatArchived: cancellation guards, in-place archive patch of entity/list/child/search caches (entity is never removed, so the open route stays mounted and flips to the read-only banner with no flash), by-workspace removal, and list/by-workspace/search invalidations. The per-member events replace the oldroot_chat_idbulk cascade filter.created(roots) runsapplyWatchedChatCreatedOrUnarchived: a cached entity witharchived: trueidentifies an unarchive and flips it back with list-row repair; a truly new chat gets only the family invalidations, no speculative entity entry.prependToInfiniteChatsCachenow skips lists whose archived filter conflicts with the chat.applyChatArchiveStateToCaches(also used by the archive/unarchive mutations) additionally patches loaded search rows.removeHardDeletedChatFromCachesis added as a distinct, deliberately unwired effect (prefix entity-family removal, cascade handling, cost-tree removal/invalidation) for a future hard-delete wire event; no current event maps to it, andarchiveAndDeleteMutationkeeps using the archive path.Covered by Vitest suites for the new helpers and search-row patching, plus two Storybook stories that drive the real chat-watch socket through the layout with the actual
AgentChatPagemounted, asserting the read-only banner appears (archive) and clears (unarchive) without unmounting.Deferred: reconnect entity-detail convergence for events missed while offline (Phase 3 subscription manager, noted in
onOpen); watch-effect module extraction and coalescing (later Phase 2).PR generated by Coder Agents.