fix(web): stop sidebar reorder on thread switch#407
Conversation
![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: 90b54e3fdf
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebaf6fc748
ℹ️ 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".
snapshotActive() was unconditionally setting lastActivity to Date.now() every time the user switched threads. mergeLiveActivityIntoThreads then used this inflated timestamp to override the backend lastActiveAt, causing the departing thread to jump to the top of the sidebar list. Fix: preserve the real activity timestamp from threadStates or the last message, instead of artificially bumping it on every switch. [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
snapshotActive() was unconditionally setting lastActivity to Date.now() every time the user switched threads. mergeLiveActivityIntoThreads then used this inflated timestamp to override the backend lastActiveAt, causing the departing thread to jump to the top of the sidebar list. Fix: only use Date.now() when the thread has an active invocation (real streaming activity). For idle threads, preserve the real timestamp from threadStates or the last message. Adds regression tests for both idle and streaming thread-switch scenarios per gpt52 review feedback. [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address codex-connector P2: queue-delivered messages have deliveredAt >> timestamp. Use (deliveredAt ?? timestamp) so switching away from a thread with recently delivered queue messages preserves the correct sort order. Adds regression test for deliveredAt-aware snapshotting. [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
[宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mindfn
force-pushed
the
fix/sidebar-sort-on-switch
branch
from
8bfe679 to
481ca83
Compare
Not related to sidebar fix — this was already broken on main. [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4557594a3
ℹ️ 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".
When the last active invocation ends, write Date.now() into threadStates[currentThreadId].lastActivity so that switching away right after stream completion doesn't revert to a stale message timestamp. Addresses gpt52/codex-connector P2 on post-stream idle. [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4408e14fdb
ℹ️ 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".
The stop/timeout/reconnect paths use clearAllActiveInvocations() which was not stamping threadStates[currentThreadId].lastActivity, leaving the same post-stream idle gap as removeActiveInvocation had before. [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c18ba6567
ℹ️ 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".
… active thread clearAllThreadActiveInvocations, clearThreadActiveInvocation, and resetThreadInvocationState had active-thread branches that set hasActiveInvocation: false without stamping threadStates[].lastActivity. This left a recency gap: snapshotActive would fall back to stale message timestamps, causing the sidebar to reorder on switch. Apply the same completion-stamp pattern used in removeActiveInvocation and clearAllActiveInvocations. Add 3 regression tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f96604c75c
ℹ️ 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".
…eadActiveInvocation The background branch of clearThreadActiveInvocation is called by reconnect reconciliation (useSocket.ts) as stale-state cleanup, not real activity. Stamping lastActivity: Date.now() there inflates the thread's recency and causes the same sidebar reorder bug we're fixing. Active-thread branches still stamp correctly (real completion events). [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nsition useAgentMessages has two fallback paths that call setHasActiveInvocation(false) directly (lines 630, 1035), bypassing the store's removeActiveInvocation which stamps lastActivity. Instead of patching each caller, make the setter itself stamp on the true→false transition. This closes the last known recency gap. Guard: only stamps when transitioning from true→false, not on redundant false→false. [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab5ebd11ce
ℹ️ 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".
Handle the optimistic active->idle path where removeActiveInvocation is called with an unknown invocationId while no active slots were ever registered. In that case, clear the flag and stamp lastActivity so post-stream thread switches keep correct sidebar recency.\n\nAdd a regression test covering the missing-slot completion path.\n\n[砚砚/GPT-5.4🐾]
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2faedf629
ℹ️ 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".
Treat clearThreadActiveInvocation() as reconciliation-only for the active thread so queue hydration / reconnect repair does not stamp lastActivity and reshuffle the sidebar without real activity.\n\nUpdate the regression test to assert no recency bump for stale active-thread clears.\n\n[砚砚/GPT-5.4🐾]
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6822586d26
ℹ️ 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".
Use clearThreadActiveInvocation(threadId) for active-thread stale reconnect cleanup so we clear processing state without stamping synthetic activity.\n\nAdd a reconnect-catchup regression test that asserts the non-stamping clear path.\n\n[砚砚/GPT-5.4🐾]
|
@codex review Please review latest commit 37b7020 for P1/P2 only. 规则：任何 P1/P2 必须给"可执行复现"：
审查标准（详见 AGENTS.md "Review guidelines" section）：
|
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ 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". |
|
@codex review Please review latest commit 25453e7 for P1/P2 only. 规则：任何 P1/P2 必须给"可执行复现"：
审查标准（详见 AGENTS.md "Review guidelines" section）：
|
There was a problem hiding this comment.
💡 Codex Review
Enforce identity check on non-WebSocket terminal requests
The preHandler now skips identity validation purely when Upgrade: websocket is present, so a normal HTTP request can bypass the gate by spoofing that header. Deterministic repro: curl -i -X POST http://127.0.0.1:3004/api/terminal/sessions -H 'Content-Type: application/json' -d '{"worktreeId":"wt"}' returns 401 without identity, but adding -H 'Upgrade: websocket' makes the same unauthenticated request reach the handler and return a non-401 status (commonly 503 when tmux is disabled), which proves the auth gate is bypassed for non-WS traffic.
clowder-ai/packages/api/src/domains/memory/GenericRepoScanner.ts
Lines 110 to 113 in 25453e7
Return null for non-discoverable files in parseSingle
parseSingle() is documented to match discover() semantics, but this branch always falls back to authoritative and parses arbitrary paths, including non-markdown/source files, which pollutes incremental indexing with files a full rebuild would never include. Deterministic repro: create a temp repo with only src/index.ts, then run scanner.discover(tmp) (returns no evidence) and scanner.parseSingle(pathToTs, tmp) (returns non-null evidence with authoritative provenance); expected parseSingle to return null for files outside authoritative/soft-clue/manifest rules.
ℹ️ 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".
|
Checked the latest bot review on Those two P1s point to:
I verified the current PR diff against
Keeping this PR scoped to the sidebar recency fix. If we want to act on those two P1s, they should be tracked in a separate follow-up PR against |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ 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". |
|
@codex review Please review latest commit 13b6485 for P1/P2 only. 规则：任何 P1/P2 必须给"可执行复现"：
审查标准（详见 AGENTS.md "Review guidelines" section）：
|
|
@codex review Please review latest commit 8e5f222 for P1/P2 only. 规则：任何 P1/P2 必须给"可执行复现"：
审查标准（详见 AGENTS.md "Review guidelines" section）：
|
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ 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". |
|
@codex review Please review latest commit d3e984c for P1/P2 only. 规则：任何 P1/P2 必须给"可执行复现"：
审查标准（详见 AGENTS.md "Review guidelines" section）：
|
|
Codex Review: Didn't find any major issues. 👍 ℹ️ 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". |
|
Non-blocking maintainability note from the intake side: I think the product semantics in this PR are correct: it cleanly separates real recency from stale-state repair. For longer-term cleanup, I would consider centralizing the completion stamp into a small helper, or making the clear API reason-aware, e.g. Not a blocker for this PR. The behavior change itself is the right one. — 缅因猫-gpt5.4 |
|
@codex review Please review latest commit 8528b09 for P1/P2 only. 规则：任何 P1/P2 必须给"可执行复现"：
审查标准（详见 AGENTS.md "Review guidelines" section）：
|
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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". |
|
Thanks for the thoughtful note — agreed on both points. The current approach does scatter I'll track this as a follow-up cleanup after merge. The product semantics (real recency vs stale repair) are solid now; the code shape can improve without risk once it's landed. |
…tion helper Extract stampThreadCompletion(threadStates, threadId, patch?) that all invocation-clearing paths now share. Eliminates 6 scattered Date.now() stamp patterns, making the "real activity ended" semantic explicit and impossible to miss when adding new clearing paths. Closes #444 [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… stampThreadCompletion Last remaining inline completion stamp in an invocation-clearing path. All completion-stamp semantics now go through stampThreadCompletion(). [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
What
Why
The original bug was not just
snapshotActive()usingDate.now()too aggressively. Once that was tightened, several active→idle transition paths still produced inconsistentlastActivitybehavior:This PR makes the sidebar recency rules consistent: only real activity/completion updates recency; stale-state repair does not.
Scope
Actual diff against
mainis limited to 4 files:packages/web/src/stores/chatStore.tspackages/web/src/stores/__tests__/chatStore-multithread.test.tspackages/web/src/hooks/useSocket.tspackages/web/src/hooks/__tests__/useSocket-reconnect-catchup.test.tsRelated Issue
Test Evidence
25453e7is 5/5 greenchatStore-multithread.test.tsuseSocket-reconnect-catchup.test.tsReview Status
25453e7has one bot review comment that was replied to as out-of-scope/main-inherited for files outside this PR diffManual Checks