Repository navigation
fix(web): new thread starts in the sidebar's filtered Squadron - #317
tyler-barton-horizon wants to merge 2 commits into
Conversation
When the sidebar was scoped to a Squadron, the New thread button, the chat.new shortcut, and the chat header button still opened the Squadron picker. Only the landing route honored the filter. Every new-thread door now resolves through resolveCurrentThreadNewThreadDestination: the active thread's durable home first, then the sidebar's ambient Squadron filter, then the sole ready Squadron, otherwise the picker. The command palette offers a "New thread in <name>" row for the filtered Squadron under the same rule. The redundant single-Squadron helper is removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: Jacksondr5/j5code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: Comment |
bryantderosier
left a comment
There was a problem hiding this comment.
I reviewed this for correctness, code quality, and security. Nothing is blocking, so I'm approving. Routing every new-thread door through resolveCurrentThreadNewThreadDestination is a real simplification. Removing canCreateThreadWithoutSquadronPicker gets rid of the Sidebar's duplicate exact-one check, and resolveIndexDraftDestination behaves the same as before. The resolver tests cover the filtered, stale, and unavailable cases.
Should-fix (inline below)
- The palette row's ⌘N hint doesn't match what ⌘N does (
CommandPalette.tsx:1701). The comment says the row follows the same rule aschat.new, butchat.newonly reads the Sidebar filter. The palette also skips the draft's chosen Squadron, which the header respects. - The ambient fallback kicks in while a thread's home is still loading (
ChatView.tsx:2377, same atCommandPalette.tsx:1704). The header can say "New thread in Alpha" for a thread homed in Bravo, then flip to Bravo once the home loads. - A filtered Squadron with no local folder makes the Sidebar button do nothing (
Sidebar.tsx:4396). The header and the palette row have the same problem. The resolver only checksavailable, notfolder. - FORK.md cases 16 and 19 still describe the old rule ("exactly one ready Registrar Squadron retains direct creation" / "choose the sole ready Registrar entry … or open the Squadron picker"). All four component and route files here are upstream-owned. Please update those cases to the new order: home → ambient filter → exact-one → picker. Case 19 should also say the palette's "New thread in " row can now render from the filter or from the sole Squadron. Otherwise the next person who rebases on upstream might "restore" the exact-one-only behavior from the inventory.
Nits
_chat.tsx:40:ambientSquadronIdholds aScopedSquadronRef, but ChatView uses that name for the string id and calls the refambientSquadronScope. I'd rename it to match ChatView.Sidebar.tsx:4390: a stale filter now blocks the exact-one shortcut (details inline).Sidebar.tsx:4388:setOpenMobile(false)now runs first, and the zero-Squadron branch calls it again throughopenSquadronCreateFromSidebar(). Harmless, just redundant.SquadronPicker.logic.ts:73: "Every new thread door resolves through here" is true, but each door passes different inputs. Once 1 and 2 are fixed, I'd like one small helper that builds the input (home, else draft, else filter) so the header, palette, andchat.newcan't drift apart again.
Security: nothing to report. The resolver matches on both environmentId and squadronId. Stale or unreachable filters open the picker. Squadron names render as escaped React text, and thread creation still goes through the existing server path.
Surfaces: keybinding, palette, header, sidebar, and landing all go through the resolver now. Desktop inherits web, and mobile has no Squadron filter. The branch/fork door deliberately keeps the source thread's home, which I think is right.
| scopedThreadKey(scopeThreadRef(activeThread.environmentId, activeThread.id)), | ||
| ) | ||
| : undefined; | ||
| // Same rule as the chat.new shortcut: the active thread's durable home, |
There was a problem hiding this comment.
Should-fix: this row's ⌘N hint doesn't match what ⌘N does. chat.new (_chat.tsx:106) only passes the Sidebar filter and never looks at the active thread's home. This row still sets shortcutCommand: "chat.new".
Example: the Sidebar is filtered to Alpha and the open thread is homed in Bravo. The palette shows "New thread in Bravo ⌘N", but pressing ⌘N starts the thread in Alpha. Before this PR the same mismatch fell back to the picker. Now ⌘N quietly goes to a different, specific Squadron.
This input also skips the draft's chosen Squadron, which the header does respect via resolveHeaderSquadronRef(... draftSquadronId). On a draft that picked Charlie with the filter on Bravo, the header says Charlie and this row says Bravo. That's a new mismatch, since the row didn't render for drafts before.
Suggested fix: build this input the same way ChatView does (home, else draft, else filter), ideally with a shared helper in SquadronPicker.logic.ts. Then either have chat.new use the same input, or only show the chat.new hint when this row's destination is what ⌘N would pick. And please fix the comment.
| durableHomeId: durableSquadronHome?.id ?? null, | ||
| draftSquadronId: draftSquadron.squadronId, | ||
| }), | ||
| }) ?? ambientSquadronScope, |
There was a problem hiding this comment.
Should-fix: the filter fallback kicks in while the thread's home is still loading. useThreadHomes leaves a thread out of the map until its home read returns, so activeThreadHome is undefined, durableSquadronHome is null, and this falls through to ambientSquadronScope. A thread that truly has no home comes back as kind: "unknown".
Example: the filter is Alpha and I open a thread homed in Bravo from a cold start, or the home read for that environment fails. The header says "New thread in Alpha", then flips to Bravo. A click before the flip starts the thread in Alpha. Before this PR that window used the exact-one shortcut or the picker, so it never picked a specific wrong home.
Suggested fix: only fall back to the filter when the thread is a draft (serverThread === null) or its home resolved to kind: "unknown". While a server thread's home is undefined, pass null so the shortcut or picker applies. CommandPalette.tsx:1704 needs the same rule for activeHome.
| if (destination.kind === "single-squadron") { | ||
| void startSquadronDraft({ | ||
| entry, | ||
| entry: destination.entry, |
There was a problem hiding this comment.
Should-fix: a filtered Squadron with no local folder makes this button do nothing. The resolver only checks entry.available. buildSquadronPickerEntries sets folder: null when the Squadron's project isn't in projects for that environment, for example before project shells finish loading after a connect, or after the folder was removed. startSquadronDraft then returns null, this void drops it, and on mobile-web the sheet has already closed. Before this PR, a filtered list with more than one Squadron opened the picker, which at least shows that row as disabled.
The ChatView header (handleNewThreadInActiveProject) and the palette row drop the null the same way, and the palette row isn't disabled for folder === null the way buildSquadronPickerRow is. Only chat.new shows the "Squadron folder unavailable" toast.
Suggested fix: have resolveCurrentThreadNewThreadDestination treat entry.folder === null like unavailable and return the picker. That also covers the index route. Please add a folder: null test case.
| if (isMobile) setOpenMobile(false); | ||
| if (isMobile) setOpenMobile(false); | ||
| const destination = resolveCurrentThreadNewThreadDestination( | ||
| squadronScopeId, |
There was a problem hiding this comment.
Nit: a stale filter now blocks the exact-one shortcut. If the filter names a Squadron the directory no longer lists (deleted on another device, so forgetDeletedSquadron never ran here) and exactly one ready Squadron remains, this button, chat.new, and the palette all open the picker. Before this PR, the Sidebar and chat.new would have used the single remaining Squadron. The new test only checks stale-then-picker with two Squadrons. If you think it's worth it, fall through to resolveNewThreadShortcutDestination when the filtered entry is missing, and keep the picker for found-but-unavailable.
| const keybindings = useAtomValue(primaryServerKeybindingsAtom); | ||
| const projects = useProjects(); | ||
| const { status: squadronDirectoryStatus, squadrons } = useSquadronDirectory(); | ||
| const ambientSquadronId = useSquadronAmbientScope(); |
There was a problem hiding this comment.
Nit: this holds a ScopedSquadronRef, not an id. ChatView uses ambientSquadronId for the string id and calls this same value ambientSquadronScope. Renaming it here to ambientSquadronScope keeps the two files consistent.
|
Heads-up: this PR is superseded by work that has now merged. What changed on
Still to come. The server migration re-keys the ledger from Squadrons to projects, removes For this PR. The problem it describes, that New thread opened the Squadron picker even with the sidebar filtered, no longer exists: there is no Squadron picker. The files it changes are either deleted ( Upstream's own behavior is back in its place: with more than one project, the New thread button opens upstream's project picker, and shift-click or the second shortcut creates directly in the current project. If you still want the sidebar's project filter to choose where a new thread goes, that would now be a small change on top of upstream's flow and is worth a fresh PR or issue. This one was right about the friction; it is what started the discussion in #412. Posted by an AI agent on Jackson's behalf. |
Problem
With the sidebar scoped to a Squadron, clicking New thread, pressing the
chat.newshortcut, or using the chat header button still opened the Squadron picker. Only the landing route honored the filter, so users had to re-pick the Squadron they were already looking at.Fix
Every new-thread door now resolves through the same function,
resolveCurrentThreadNewThreadDestination:The picker still appears when the filter points at a Squadron the directory no longer lists or whose environment is unreachable. The command palette gains a "New thread in " row for the filtered Squadron under the same rule. The redundant
canCreateThreadWithoutSquadronPickerhelper is removed.Surfaces
chat.newshortcut, chat header button, command palette, landing route (already correct).Verification
SquadronPicker.logic.test.tsandCommandPalette.logic.test.tspass, with new cases for the filtered, stale, and unavailable Squadron paths.Model: Claude Fable 5.1. Harness: Claude Code in T3 Code.
🤖 Generated with Claude Code