Repository navigation
Honour a saved pinned-thread order in the Threads panel model - #1560
FahadBinHussain wants to merge 1 commit into
Conversation
arrangeThreadPanel now takes the viewer's pinnedOrder alongside the group order, so a pinned thread stays where it was put instead of re-sorting on every message, and movePinnedThread gives the panel the same "move one row" helper moveThreadGroup already gives it for groups. Pinned threads the saved order does not name keep their newest-first place behind the named ones, and without pinnedOrder the behaviour is exactly what it was. The panel wiring that persists the order and offers the drag handle lives in the private tree; this is the shared model half of CodebuffAI#1559.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Nice, focused change.
The doc comments on both functions are unusually good at explaining why the partial-order behavior was chosen, which will make porting and future maintenance easier. Minor things worth double-checking before port: Overall: in scope, correct, tested, and easy to review. Good first contribution. |
|
Thanks for the careful, detailed review — both of the follow-ups are fair, and here's where each lands: 1. Stable-sort assumption — agreed, adding the comment. You're right that spec-guaranteed stability since ES2019 makes this safe as written, and that it's load-bearing rather than incidental: the "never-named threads keep newest-first order" guarantee depends entirely on it. I'll add a one-line comment above the sort in 2. #1559 call-site semantics — flagging for confirmation before port. Good catch that the two PRs are being reviewed independently. Everything else — the Will follow up once the comment is in and the #1559 call site is verified. |
What
arrangeThreadPanelincommon/src/desktop-cloud/threads.tsgains an optionalpinnedOrder: the viewer's own ordering of the pinned block, honoured ahead of activity within each group. Pinned threads the saved order does not name — never arranged, or left over from a thread that was deleted or unpinned — sort in behind the named ones and keep their newest-first place there (sort is stable). WithpinnedOrderabsent the behaviour is byte-for-byte what it was, so no caller changes.movePinnedThread(pinnedOrder, thread, target)is added alongside the existingmoveThreadGroup, giving the panel one helper for "put this row above that one": athreadthe order never named is simply inserted, an unknowntargetsends it to the end of the saved order, and names that no longer match a thread are kept rather than silently trimmed — a saved order outlives threads, and dropping entries is the viewer's call.Both are covered in
threads.test.ts: the saved order winning over a newer pinned thread, the no-order fallback being unchanged, stale names being preserved, and the move helper's insert/unknown-target/never-named cases.Why
Pinned threads are only ever hoisted above the unpinned ones — inside the pinned block they are sorted by
updatedAt, so every message from any pinned thread reshuffles the block. Pinning is meant to say "keep this where I can see it"; right now it only says "keep this above the rest". The group order is already the viewer's to set and persist, and the order inside the pinned block is the same kind of preference.Filed as #1559. The model half is what this mirror can accept; the panel wiring that persists the order and offers the drag handle lives in the private tree and would ride along with the port.
Verification
bun test common/src/desktop-cloud— 65 pass, 0 fail (14 files), including the two new tests.bun run --cwd common typecheck— no errors in the changed files; the 4 errors it reports are the pre-existingSharedV4ProviderOptions/ProviderMetadatamismatches insrc/util/messages.tsandsrc/types/__tests__/dynamic-agent-template.test.ts.bun run build:sdkfails with those same pre-existing declaration-bundling errors on a cleanmain(checked with this change stashed), so it is not from this PR.common/src/desktop-cloud/, inside the accepted paths.