Handle remote thread archive notifications#187
Open
tdtdtq-ux wants to merge 2 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a context-compaction UI and end-to-end flow (UI, API, normalization, runtime handling, rendering) plus realtime handling for ChangesContext compaction feature
Thread archive notification handling
Sequence Diagram(s)sequenceDiagram
participant ThreadComposer as ThreadComposer (UI)
participant App as App.vue
participant DesktopState as useDesktopState
participant CodexGateway as codexGateway.compactThread
participant Codex as Codex notifications
participant ThreadConversation as ThreadConversation (renderer)
ThreadComposer->>App: emit compact-context
App->>DesktopState: compactSelectedThread()
DesktopState->>DesktopState: insert pending contextCompaction.live message
DesktopState->>CodexGateway: callRpc thread/compact/start
CodexGateway->>Codex: RPC request -> compaction job started
Codex-->>DesktopState: notification item/started (contextcompaction)
DesktopState->>DesktopState: upsert pending live compaction message
Codex-->>DesktopState: notification item/completed (contextcompaction)
DesktopState->>DesktopState: replace pending with completed system message
DesktopState->>ThreadConversation: new message list includes contextCompaction system/live
ThreadConversation->>ThreadConversation: render context compaction separator
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Review Summary by QodoHandle remote thread archive notifications in realtime
WalkthroughsDescription• Handle thread/archived realtime notifications from app-server • Remove remotely archived threads from UI immediately • Move selection to adjacent thread when selected thread archived • Add regression tests for remote archive synchronization Diagramflowchart LR
A["thread/archived notification"] -->|"extractThreadId"| B["applyArchivedThreadNotification"]
B -->|"wasSelectedThread"| C["findAdjacentThreadId"]
B -->|"removeThread"| D["removeArchivedThreadFromLoadedLists"]
C -->|"setSelectedThreadId"| E["loadMessages"]
D -->|"updateUI"| F["projectGroups updated"]
File Changes1. src/composables/useDesktopState.ts
|
Code Review by Qodo
1. No perf audit for thread/archived
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+3755
to
+3761
| if (notification.method === 'thread/archived') { | ||
| const threadId = extractThreadIdFromNotification(notification) | ||
| if (threadId) { | ||
| applyArchivedThreadNotification(threadId) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
1. No perf audit for thread/archived 📘 Rule violation ➹ Performance
This PR adds realtime handling for thread/archived notifications but does not include any performance audit evidence or explicit code-path analysis for the new realtime path. This risks regressions (extra work per notification and/or extra message loads) without the required documented evaluation.
Agent Prompt
## Issue description
A feature/behavior change was introduced in a high-risk area (realtime notification handling / thread list updates) without the required performance audit evidence or explicit analysis.
## Issue Context
The new `thread/archived` handler can introduce per-notification work (e.g., repeated `flattenThreads(...)`) and can trigger an additional `loadMessages(...)` call when the archived thread was selected. Compliance requires documenting concrete evidence (measurements/traces/request counts) or explicitly stating what could not be measured.
## Fix Focus Areas
- src/composables/useDesktopState.ts[3755-3761]
- src/composables/useDesktopState.ts[4231-4249]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+4231
to
+4249
| function applyArchivedThreadNotification(threadId: string): void { | ||
| const normalizedThreadId = threadId.trim() | ||
| if (!normalizedThreadId) return | ||
|
|
||
| const wasSelectedThread = selectedThreadId.value === normalizedThreadId | ||
| const nextSelectedThreadId = wasSelectedThread | ||
| ? findAdjacentThreadId(flattenThreads(projectGroups.value), normalizedThreadId) | ||
| : selectedThreadId.value | ||
|
|
||
| if (wasSelectedThread) { | ||
| setSelectedThreadId(nextSelectedThreadId) | ||
| } | ||
|
|
||
| removeArchivedThreadFromLoadedLists(normalizedThreadId) | ||
| pruneThreadScopedState(flattenThreads(projectGroups.value)) | ||
|
|
||
| if (wasSelectedThread && nextSelectedThreadId) { | ||
| void loadMessages(nextSelectedThreadId, { silent: true }) | ||
| } |
There was a problem hiding this comment.
2. Manual test docs not updated 📘 Rule violation ⚙ Maintainability
The PR changes thread-list/selection behavior when a thread is archived remotely but does not update the relevant manual test documentation under tests/<domain>/. This leaves manual verification instructions stale for the new behavior.
Agent Prompt
## Issue description
Feature work was completed without updating the relevant manual test documentation under `tests/<domain>/`.
## Issue Context
This PR adds behavior for remotely-archived threads (immediate removal from loaded list; selection moves to adjacent thread). The existing thread-archive manual test document focuses on local archive/delete behavior and does not cover remote archive notifications.
## Fix Focus Areas
- tests/thread-loading-state/thread-archive-recovery-and-sidebar-pruning.md[1-30]
- src/composables/useDesktopState.ts[3755-3761]
- src/composables/useDesktopState.ts[4231-4249]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Surface thread token usage directly in the composer as a circular context indicator with a hover/focus details popover. The popover includes a compact context action that calls thread/compact/start for the active thread while keeping the new-thread composer disabled for compaction. Render v2 contextCompaction items in the conversation as Codex-style divider rows, and show live compaction progress from item/started without duplicating the activity overlay. Historical thread/read payloads now normalize contextCompaction items so completed compactions remain visible after reload. Tests: - pnpm test:unit -- src/api/normalizers/v2.test.ts src/composables/useDesktopState.test.ts - pnpm run build:frontend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/composables/useDesktopState.test.ts (1)
680-758: ⚡ Quick winAdd a rejection-path test for manual compaction.
Current coverage validates success and notification flow, but not the
compactThreadfailure path (e.g., pending live marker cleanup and surfaced error state). A focused failure test here would harden this feature.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/composables/useDesktopState.test.ts` around lines 680 - 758, Add a test that mocks gatewayMocks.compactThread to reject (e.g., mockRejectedValue(new Error('fail'))), calls state.primeSelectedThread('thread-compact') and then awaits state.compactSelectedThread() (handling the thrown rejection with try/catch or expect(...).rejects), and assert that gatewayMocks.compactThread was called with 'thread-compact', that the pending "Compacting context…" live marker is removed from state.messages (no lingering pending id like the manual pending marker), that state.selectedLiveOverlay.value is null, and that a system error message reflecting the compaction failure (e.g., a message with role:'system' and a messageType indicating compaction error) is appended to state.messages so the failure is surfaced. Ensure you reference compactSelectedThread, gatewayMocks.compactThread, state.messages, and state.selectedLiveOverlay in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/content/ThreadComposer.vue`:
- Around line 796-802: The new user-facing strings in the computed
contextCompactButtonText (and the similar strings added around the second
occurrence) are hardcoded English; replace each literal ('Open thread to
compact', 'Compact after current turn', 'Compact context', and the other added
literals at the second location) with calls to the i18n translator (e.g.
t('thread.openToCompact'), t('thread.compactAfterTurn'),
t('thread.compactContext') or equivalent keys) so the component uses t(...) like
the rest of the file; update or add the corresponding translation keys in your
locale files to provide the English text and translations.
In `@src/components/content/ThreadConversation.vue`:
- Around line 4621-4630: Remove the dark-mode override from the component
stylesheet in ThreadConversation.vue and instead add the dark-theme rule for the
shared selector ".context-compaction-separator" into src/style.css;
specifically, delete the :global(.dark) .context-compaction-separator block from
ThreadConversation.vue and create a top-level rule in src/style.css using the
same selector ".dark .context-compaction-separator" with the identical
linear-gradient background so the decisive dark override lives in the shared
stylesheet rather than the component.
In `@src/composables/useDesktopState.ts`:
- Around line 3688-3693: The code currently extracts only camelCase ids via
readString(params?.threadId)/readString(params?.turnId), causing snake_case or
nested turn payloads to be missed; replace this manual extraction with the
shared notification-ID extraction helper used elsewhere (call it where you
create params) to populate threadId and turnId (so it handles thread_id/turn_id
and nested payload shapes), then compute turnIndex via
turnIndexByTurnIdByThreadId.value[threadId]?.[turnId] as before; update the
variables params, threadId, turnId, and turnIndex to use that shared helper
instead of direct readString calls.
---
Nitpick comments:
In `@src/composables/useDesktopState.test.ts`:
- Around line 680-758: Add a test that mocks gatewayMocks.compactThread to
reject (e.g., mockRejectedValue(new Error('fail'))), calls
state.primeSelectedThread('thread-compact') and then awaits
state.compactSelectedThread() (handling the thrown rejection with try/catch or
expect(...).rejects), and assert that gatewayMocks.compactThread was called with
'thread-compact', that the pending "Compacting context…" live marker is removed
from state.messages (no lingering pending id like the manual pending marker),
that state.selectedLiveOverlay.value is null, and that a system error message
reflecting the compaction failure (e.g., a message with role:'system' and a
messageType indicating compaction error) is appended to state.messages so the
failure is surfaced. Ensure you reference compactSelectedThread,
gatewayMocks.compactThread, state.messages, and state.selectedLiveOverlay in the
test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a72fa05c-9808-4479-a137-6bfeac53a12c
📒 Files selected for processing (9)
src/App.vuesrc/api/codexGateway.tssrc/api/normalizers/v2.test.tssrc/api/normalizers/v2.tssrc/components/content/ThreadComposer.vuesrc/components/content/ThreadConversation.vuesrc/composables/useDesktopState.test.tssrc/composables/useDesktopState.tssrc/style.css
Comment on lines
+796
to
+802
| const contextCompactButtonText = computed(() => | ||
| props.contextCompactionAvailable === false | ||
| ? 'Open thread to compact' | ||
| : props.isTurnInProgress | ||
| ? 'Compact after current turn' | ||
| : 'Compact context', | ||
| ) |
Contributor
There was a problem hiding this comment.
Localize newly added context-compaction copy.
Line 796 and Line 1007 introduce new user-facing text as hardcoded English, while this component otherwise uses t(...). This will bypass localization.
💡 Suggested fix
const contextCompactButtonText = computed(() =>
props.contextCompactionAvailable === false
- ? 'Open thread to compact'
+ ? t('Open thread to compact')
: props.isTurnInProgress
- ? 'Compact after current turn'
- : 'Compact context',
+ ? t('Compact after current turn')
+ : t('Compact context'),
)Also applies to: 1007-1013
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/content/ThreadComposer.vue` around lines 796 - 802, The new
user-facing strings in the computed contextCompactButtonText (and the similar
strings added around the second occurrence) are hardcoded English; replace each
literal ('Open thread to compact', 'Compact after current turn', 'Compact
context', and the other added literals at the second location) with calls to the
i18n translator (e.g. t('thread.openToCompact'), t('thread.compactAfterTurn'),
t('thread.compactContext') or equivalent keys) so the component uses t(...) like
the rest of the file; update or add the corresponding translation keys in your
locale files to provide the English text and translations.
Comment on lines
+4621
to
+4630
| :global(.dark) .context-compaction-separator { | ||
| background: | ||
| linear-gradient( | ||
| to bottom, | ||
| transparent calc(50% - 0.5px), | ||
| rgb(63 63 70) calc(50% - 0.5px), | ||
| rgb(63 63 70) calc(50% + 0.5px), | ||
| transparent calc(50% + 0.5px) | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move dark-theme override for context compaction separator to src/style.css.
Line 4621 adds a decisive dark-mode surface rule in a component-scoped stylesheet. For this shared conversation surface, keep dark-theme override wiring in src/style.css and leave the component with base styles only.
Proposed change
-:global(.dark) .context-compaction-separator {
- background:
- linear-gradient(
- to bottom,
- transparent calc(50% - 0.5px),
- rgb(63 63 70) calc(50% - 0.5px),
- rgb(63 63 70) calc(50% + 0.5px),
- transparent calc(50% + 0.5px)
- );
-}/* add in src/style.css */
.dark .context-compaction-separator {
background:
linear-gradient(
to bottom,
transparent calc(50% - 0.5px),
rgb(63 63 70) calc(50% - 0.5px),
rgb(63 63 70) calc(50% + 0.5px),
transparent calc(50% + 0.5px)
);
}As per coding guidelines: “src/style.css: For shared route surfaces and large feature UIs, put decisive dark-theme overrides in src/style.css instead of relying only on component-scoped :global(:root.dark) blocks”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/content/ThreadConversation.vue` around lines 4621 - 4630,
Remove the dark-mode override from the component stylesheet in
ThreadConversation.vue and instead add the dark-theme rule for the shared
selector ".context-compaction-separator" into src/style.css; specifically,
delete the :global(.dark) .context-compaction-separator block from
ThreadConversation.vue and create a top-level rule in src/style.css using the
same selector ".dark .context-compaction-separator" with the identical
linear-gradient background so the decisive dark override lives in the shared
stylesheet rather than the component.
Comment on lines
+3688
to
+3693
| const params = asRecord(notification.params) | ||
| const threadId = readString(params?.threadId) | ||
| const turnId = readString(params?.turnId) | ||
| const turnIndex = threadId && turnId | ||
| ? turnIndexByTurnIdByThreadId.value[threadId]?.[turnId] | ||
| : undefined |
Contributor
There was a problem hiding this comment.
Use the shared notification ID extraction here.
This helper only reads camelCase threadId/turnId. The rest of the file accepts snake_case and nested turn payloads, so compaction messages can lose their turnIndex binding and render out of order for those payload shapes.
Suggested fix
const params = asRecord(notification.params)
- const threadId = readString(params?.threadId)
- const turnId = readString(params?.turnId)
+ const threadId = extractThreadIdFromNotification(notification)
+ const turn = asRecord(params?.turn)
+ const turnId =
+ readString(params?.turnId) ||
+ readString(params?.turn_id) ||
+ readString(turn?.id)
const turnIndex = threadId && turnId
? turnIndexByTurnIdByThreadId.value[threadId]?.[turnId]
: undefined🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/composables/useDesktopState.ts` around lines 3688 - 3693, The code
currently extracts only camelCase ids via
readString(params?.threadId)/readString(params?.turnId), causing snake_case or
nested turn payloads to be missed; replace this manual extraction with the
shared notification-ID extraction helper used elsewhere (call it where you
create params) to populate threadId and turnId (so it handles thread_id/turn_id
and nested payload shapes), then compute turnIndex via
turnIndexByTurnIdByThreadId.value[threadId]?.[turnId] as before; update the
variables params, threadId, turnId, and turnIndex to use that shared helper
instead of direct readString calls.
Comment on lines
+4017
to
+4026
| const contextCompactionMessage = readContextCompactionNotification(notification) | ||
| if (contextCompactionMessage && notificationThreadId) { | ||
| const pendingId = `context-compaction:pending:${notificationThreadId}` | ||
| const previousLiveAgent = liveAgentMessagesByThreadId.value[notificationThreadId] ?? [] | ||
| const withoutPending = previousLiveAgent.filter((message) => message.id !== pendingId) | ||
| setLiveAgentMessagesForThread( | ||
| notificationThreadId, | ||
| upsertMessage(withoutPending, contextCompactionMessage), | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Successful compactions can leave the thread stuck busy.
compactSelectedThread() marks the thread inProgress and inserts a pending compaction message, but only the failure path clears that state. This completion block merely swaps the live message for the currently selected thread, so navigating away before item/completed lands can leave the original thread permanently busy until a later reload.
Also applies to: 4922-4946
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes realtime synchronization when a thread is archived from another client.
The app-server broadcasts
thread/archived, but the web client only removed archived threads after a local archive action or a full thread list refresh. This caused threads archived from Codex Desktop to remain visible in the web UI until refresh.Changes
thread/archivedin the realtime notification path.Tests
pnpm test:unit -- src/composables/useDesktopState.test.tspnpm run build:frontendSummary by CodeRabbit
New Features
UI
Tests