Skip to content

Honour a saved pinned-thread order in the Threads panel model - #1560

Open
FahadBinHussain wants to merge 1 commit into
CodebuffAI:mainfrom
FahadBinHussain:pinned-thread-order
Open

FahadBinHussain wants to merge 1 commit into
CodebuffAI:mainfrom
FahadBinHussain:pinned-thread-order

Conversation

@FahadBinHussain

Copy link
Copy Markdown

What

arrangeThreadPanel in common/src/desktop-cloud/threads.ts gains an optional pinnedOrder: 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). With pinnedOrder absent the behaviour is byte-for-byte what it was, so no caller changes.

movePinnedThread(pinnedOrder, thread, target) is added alongside the existing moveThreadGroup, giving the panel one helper for "put this row above that one": a thread the order never named is simply inserted, an unknown target sends 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-existing SharedV4ProviderOptions / ProviderMetadata mismatches in src/util/messages.ts and src/types/__tests__/dynamic-agent-template.test.ts.
  • bun run build:sdk fails with those same pre-existing declaration-bundling errors on a clean main (checked with this change stashed), so it is not from this PR.
  • Scoped to common/src/desktop-cloud/, inside the accepted paths.

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.
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-10-27T11:40:42.218733Z
We couldn’t complete this review. Comment @codex review to try again.
af4face PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@codebuff-team

Copy link
Copy Markdown
Contributor

Nice, focused change. arrangeThreadPanel's new pinnedOrder option is backward compatible (default [] yields the old byte-for-byte behavior, confirmed by the 'nothing saved' test case), and the partial-order semantics (unnamed threads fall to the tail, stable sort preserves their newest-first order, stale names are kept rather than silently dropped) are sensible defaults for a persisted viewer preference.

movePinnedThread is a small, well-reasoned helper: filtering the thread out before re-splicing at the target's index correctly produces 'insert directly above target', and the edge cases (thread never in the order, unknown target, empty order) are all covered by the new tests in threads.test.ts.

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: Array.prototype.sort's stability is guaranteed by spec since ES2019, so relying on it here is fine, but it's worth a one-line comment noting that assumption explicitly since it's load-bearing for the 'never-named threads keep newest-first order' guarantee. Also worth confirming on the maintainer side that the companion panel-wiring PR (#1559, in the private tree) actually consumes movePinnedThread with the same insert-above semantics tested here, since the two PRs are being reviewed independently.

Overall: in scope, correct, tested, and easy to review. Good first contribution.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree labels Oct 27, 2026
@FahadBinHussain

Copy link
Copy Markdown
Author

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 arrangeThreadPanel stating that assumption explicitly, so anyone rewriting the sort later sees that swapping in an unstable comparison would silently break that case rather than just reordering cosmetic entries.

2. #1559 call-site semantics — flagging for confirmation before port.

Good catch that the two PRs are being reviewed independently. movePinnedThread's "filter out first, then re-splice at the target's index" behavior is what produces insert-directly-above, and the tests here encode exactly that. I'll confirm the panel-wiring PR consumes it with the same semantics rather than splicing by raw index, since a mismatch there would invert the intended order without failing anything visible in either PR in isolation.

Everything else — the pinnedOrder: [] default preserving byte-for-byte prior behavior, and the partial-order treatment of unnamed and stale-named threads — is intentional, so no changes planned there.

Will follow up once the comment is in and the #1559 call site is verified.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants