Repository navigation
Pin conversations and group them into folders - #1007
dcartertwo wants to merge 3 commits into
Conversation
Preview:
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| const move = (change: () => void) => { | ||
| change(); | ||
| requestAnimationFrame(() => | ||
| document.querySelector<HTMLElement>(`[data-chat-actions="${chatId}"]`)?.focus()); |
There was a problem hiding this comment.
[P2] Preserve focus when moving a conversation into hidden Other
With a folder present, pin a conversation, hide Other, then choose Unpin. The row is removed and Other stays collapsed, so this selector finds no replacement trigger. I reproduced focus falling to document.body in Chromium, with the next Tab restarting at the first control. Remove from folder has the same path. Please focus the Other disclosure when the destination row is hidden, or expand Other before restoring row focus. The frontend guide (packages/workshop-frontend/AGENTS.md) requires: "Preserve keyboard behavior, focus management, and accessible names when extracting interactive UI."
There was a problem hiding this comment.
Fixed in b5e599d: when the moved conversation lands in a hidden Other, focus now goes to Other's toggle. Checked in Chromium with the keyboard for Unpin and Remove from folder; the next Tab continues to the composer.
|
Review: 1 findings. Posted one actionable inline comment, verified in Chromium. Reviewed all 10 changed files against the frontend guidance. |
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
b5e599d9.
This review, of 00c7212b
💬 Approved with comments — No blocking or warning-level issues. There are two optional suggestions: validate each stored folder entry, and handle localStorage write failures.
Findings: 2 suggestions (2 are inline comments on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: 💬 2 suggestions
The feature is well structured. Two optional hardening suggestions for the localStorage store.
Recommendation: 💬 Optional Nits
- [SUGGESTION] Stored folders aren't validated per entry, so a null entry would make the chat list throw on every render (
packages/workshop-frontend/src/features/chat-organizer/chatOrganizerStore.ts:21) - [SUGGESTION] A localStorage write failure throws uncaught from the menu handlers and the user sees nothing (
packages/workshop-frontend/src/features/chat-organizer/chatOrganizerStore.ts:41)
View full review
- Per-entry validation of stored data (
chatOrganizerStore.ts:21):parsechecks the overall shape but not each entry infolders. Anullentry, for example from a hand-edited value, would makeorganizeChatsthrow onfolder.idwhile the list renders, every time it renders, until the key is cleared. - Write failures (
chatOrganizerStore.ts:41): asetItemerror (full quota, or storage blocked) propagates out of the menu click handlers uncaught. The user gets no feedback, andcreateFolderskips entering rename mode.
✅ All other checks passed
🔒 Security: ✅ LGTM
localStorage holds only chat ids, folder ids (from crypto.randomUUID) and folder names, under a key scoped to the user and workspace. React escapes folder names when it renders them, and the parsed data is used only locally.
📖 AGENTS.md: ✅ LGTM
No repository guideline issues.
⚡ Performance: ✅ LGTM
Organizing and bucketing are single linear passes. Store snapshots are cached until the stored value changes, and listeners are cleaned up correctly.
📝 Documentation: ✅ LGTM
No documentation issues.
Reviewed commit: 00c7212b
| try { | ||
| const value = JSON.parse(raw) as Partial<ChatOrganization> | null; | ||
| return { | ||
| folders: Array.isArray(value?.folders) ? value.folders : [], |
There was a problem hiding this comment.
💬 SUGGESTION · Code Quality
Stored folders aren't validated per entry, so a null entry would make the chat list throw on every render
parse checks that folders is an array but not what it contains. If a stored entry is null (a hand-edited value, or some future shape), organizeChats throws on folder.id during render, and the chat list stays broken until the key is cleared. You could keep only the entries that are objects with string id and name, and reject a places that is an array. A test next to the existing invalid-JSON case would cover it.
There was a problem hiding this comment.
Fixed in b5e599d: each saved folder must be an object with a string id and name, and an array places is ignored. Added a test beside the invalid-JSON case.
|
|
||
| /** Saves an arrangement. A storage error (a full quota) propagates: the change didn't happen. */ | ||
| export const writeChatOrganization = (key: string, organization: ChatOrganization) => { | ||
| localStorage.setItem(key, JSON.stringify(organization)); |
There was a problem hiding this comment.
💬 SUGGESTION · Code Quality
A localStorage write failure throws uncaught from the menu handlers and the user sees nothing
Every caller of writeChatOrganization is a menu or button click handler. If setItem throws (full quota, or storage blocked), the error goes uncaught and the click silently does nothing. In createFolder, the folder also never enters rename mode. You could catch the error in useChatOrganizer's update and show it to the user, for example with a toast.
There was a problem hiding this comment.
Fixed in b5e599d: a change that can't be saved now shows an error toast ("Couldn't save that change"), and a new folder only enters rename mode once it's saved. Added a test.
There was a problem hiding this comment.
Superseded by the review of
e2d597f2.
This review, of b5e599d9
✅ Approved — No findings. The two earlier suggestions (per-entry validation of stored folders, handling localStorage write failures) are fixed and have tests.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
No security issues. localStorage holds only chat ids, folder ids and folder names, under a key scoped to the user and workspace. React escapes folder names when it renders them.
📖 AGENTS.md: ✅ LGTM
No repository guideline issues.
📋 Code Quality: ✅ LGTM
- Per-entry folder validation (
chatOrganizerStore.ts): fixed. Each stored entry is checked byisFolder, anduseChatOrganizer.test.tsxcovers null and malformed entries. - Storage write failures (
useChatOrganizer.ts): fixed.updatecatches the error, shows an error toast and returns false, andcreateFolderonly enters rename mode when the save succeeded. organizeChatstreats an unknown or non-string place value as Other, so loosely validatedplacesentries cannot throw.
⚡ Performance: ✅ LGTM
Organizing and counting statuses are single linear passes. The parsed snapshot is cached until the stored value changes, and subscriptions are cleaned up correctly.
📝 Documentation: ✅ LGTM
No documentation issues.
Reviewed commit: b5e599d9
| <DropdownMenu.Item | ||
| icon={<Trash className="size-3" />} | ||
| variant="danger" | ||
| onClick={() => organizer.deleteFolder(folder.id)} |
There was a problem hiding this comment.
[P2] Restore keyboard focus after deleting a folder
Choosing Delete folder removes the focused menu and its trigger, but this path never transfers focus to a surviving control. I reproduced this in Chromium using the keyboard: after deletion, document.activeElement is document.body, and the next Tab restarts at the first page control. Please focus a nearby surviving control (for example, the next folder, Other, or New folder) after the deletion, including when the last folder is removed. packages/workshop-frontend/AGENTS.md requires: "Preserve keyboard behavior, focus management, and accessible names when extracting interactive UI."
|
Since last review: 1 resolved, 0 still open, 1 new. Posted one actionable inline comment. All 14 organizer tests pass, and Chromium confirms the earlier focus fix. |
A chat's metadata can now say whether it's pinned and which folder it's in, so everyone in the workspace sees the same arrangement on every device. setChatPlacement() changes either or both in one write. A field it isn't given stays as it is, so changing one can't undo someone else's change to the other. A folder is just the name its chats share, so renameChatFolder() refiles every chat in it, and renaming to null deletes the folder. Neither changes when the chat was last active, so arranging the list doesn't reorder it. "use" share-link sessions can't call either. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The chat list shows the workspace's pins and folders, and changes to them appear for collaborators as they happen. - A conversation's menu gets Pin to top and Move to folder, which lists the folders and has New folder… to name a new one. - Pinned conversations show only under Pinned, keeping their folder for when they're unpinned. Filing a pinned conversation unpins it. - Everything not pinned or filed goes under Other, which can be hidden. - A collapsed folder or a hidden Other says how many of its conversations are working or have pending changes, and how many unread messages they have. - Deleting a folder asks first, since it changes everyone's list. - Which folders are collapsed, and whether Other is hidden, stay per person in localStorage. - Focus waits until the list shows a change before following the moved conversation, because its new metadata can arrive just before or just after the change resolves. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b5e599d to
e2d597f
Compare
github-actions
Bot
added
kernel
There was a problem hiding this comment.
Superseded by the review of
5005edcf.
This review, of e2d597f2
💬 Approved with comments — Kernel and API changes are correct, access is denied to share-link sessions as intended, and tests cover them. One INFO: renaming a collapsed folder updates the saved collapsed state before the rename succeeds.
Findings: 1 info (1 is an inline comment on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: 💬 1 info
One minor note: renaming a collapsed folder updates the saved collapsed state before the server rename succeeds.
Recommendation: 💬 Optional Nits
- [INFO] Collapsed-state rename is applied before
renameChatFoldersucceeds, so a failed rename expands the folder and leaves a stale entry (packages/workshop-frontend/src/features/chat-organizer/useChatOrganizer.ts:142)
View full review
- Collapsed state is updated before the rename succeeds (
useChatOrganizer.ts:142-151). When the rename RPC fails, the folder keeps its old name but is no longer incollapsedFolders, so it opens unexpectedly. The new name stays in the list as a stale entry. This only affects one person's view preference.
The new Overseer methods are atomic and leave lastActive alone. The other metadata writers keep the new fields, and both the backend and the frontend hook have tests.
✅ All other checks passed
🔒 Security: ✅ LGTM
UseOverseerInterface denies setChatPlacement and renameChatFolder, the same way it denies setChatTitle, and the DENIED_OVERSEER integration table covers both. Arguments are validated by @validateRpc(), and assertFolderName rejects blank names. React escapes folder names when it renders them.
📖 AGENTS.md: ✅ LGTM
No repository guideline issues. The kernel and API change is in its own commit, apart from the UI.
⚡ Performance: ✅ LGTM
No performance issues.
📝 Documentation: ✅ LGTM
The new Overseer methods and the ChatPlacement type have doc comments, as the kernel rules require.
Reviewed commit: e2d597f2
| setFocusRequest({ targets: [() => findFolderToggle(folder)], shown: showsNow }); | ||
| return; | ||
| } | ||
| if (view.collapsedFolders.includes(folder)) { |
There was a problem hiding this comment.
ℹ️ INFO · Code Quality
Collapsed-state rename is applied before renameChatFolder succeeds, so a failed rename expands the folder and leaves a stale entry
updateView swaps the old folder name for the new one in collapsedFolders before renameChatFolder runs. If the RPC fails, the folder keeps its old name but is no longer marked collapsed, so it opens unexpectedly. The new name also stays saved, so a folder later created with that name starts out collapsed. To fix this, apply the collapsedFolders update only after await overseer.renameChatFolder(...) resolves inside the save callback.
There was a problem hiding this comment.
Fixed in 5005edc. The collapsed state still moves to the new name right away, so the folder doesn't flash open while the rename saves, but a failed rename now moves it back to the old name.
|
LGTM! |
Renaming a collapsed folder moves its collapsed state to the new name right away, so it doesn't flash open while the rename is saved. If the rename fails, that move is now undone, so the folder stays collapsed under its old name and the new name isn't left marked collapsed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ Approved — The earlier INFO about a failed rename losing the collapsed state is fixed and tested. No new issues were found in the changed code.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
UseOverseerInterface denies setChatPlacement and renameChatFolder, just as it denies setChatTitle. assertFolderName rejects blank names. The rollback in the latest commit only changes this user's own view state in localStorage.
📖 AGENTS.md: ✅ LGTM
The change follows the repository guidelines, and the kernel and API change is in its own commit, apart from the UI.
📋 Code Quality: ✅ LGTM
The earlier note is fixed. In useChatOrganizer.ts, finishNaming moves a folder's collapsed state to its new name as the rename starts. If renameChatFolder fails, it moves the state back to the old name and rethrows, and save shows the error toast. The new test covers this: after a rejected rename, the folder is still collapsed under its old name.
⚡ Performance: ✅ LGTM
No performance issues.
📝 Documentation: ✅ LGTM
The new Overseer methods and the ChatPlacement type have doc comments.
Reviewed commit: 5005edcf
|
LGTM! |
Eval resultsVerdict: ⚪ Unchanged. No task moved beyond what these runs can tell apart from noise.
Failed checks
|
🔬 Eval runs reviewPerformanceThe comparison is unchanged: chess improved from 5/10 to 7/10 passes, incident-desk fell from 9/10 to 8/10, worker-logs improved from 9/10 to 10/10, and the remaining task/model rows passed every run; none moved beyond noise. Neither prompt-cache hit nor break rates changed significantly, and costs were mixed: mean chess cost rose $0.0178 → $0.0223 while incident-desk fell $0.0170 → $0.0135; the single Sonnet compaction run remained the largest bill at $1.4572 → $1.5129. Chess lost the most candidate runs (3/10) and increased from 27.2 to 35.1 steps and 3.6 to 3.9 minutes, partly because nine rather than five runs reached PGN, with repeated failed edits adding avoidable work. ⚪ VERDICT: NO REGRESSION FROM THIS PRThe diff adds conversation organization and placement RPCs, leaving agent prompts, tools and eval checks unchanged, and the trajectories show generated-code mistakes rather than failures attributable to those additions. TriageFailure modes
Tool errors
What to do
|
Workspaces with a lot of conversations make it hard to keep the few you're working on in reach. This lets a workspace arrange its conversation list:
Until someone pins a conversation or files one in a folder, the list looks exactly as it does today.
Pins and folders belong to the workspace, like conversation titles: collaborators see the same arrangement on every device, and see changes as they happen. Pins are shared along with folders, unlike pinned workspaces, which are each user's own. Each conversation's metadata gets an optional
pinnedflag andfoldername, set by two newOverseermethods:setChatPlacement(chatId, {pinned?, folder?})changes either or both in one write. A field it isn't given stays as it is, so pinning a conversation can't undo someone else's move of it into a folder.renameChatFolder(folder, newName)refiles every conversation in a folder; a nullnewNamedeletes the folder. A folder is only the name its conversations share, so there's no folder record to keep in sync and no empty folders.Neither method changes when a conversation was last active, so arranging the list doesn't reorder it. A conversation pinned while an agent is replying stays pinned, because metadata updates read and write the record without waiting in between. "use" share-link sessions can't call either method, just as they can't rename conversations.
Which folders are collapsed, and whether Other is hidden, is each person's own view, so that stays in localStorage per user and workspace.
The kernel and API change is the first commit, apart from the UI.
Collapsing a folder or hiding Other would also hide the Working, Pending changes and unread labels on the conversations inside, so a collapsed header shows how many are working or have pending changes ("1 working", "2 pending") and how many unread messages they have.