Repository navigation
feat(fork): group sidebar v2 threads by project #24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| // @effect-diagnostics nodeBuiltinImport:off | ||
| /** | ||
| * Fork guard — see `.fork/README.md` §4b and | ||
| * `.fork/customizations.yaml#sidebar-v2-project-grouping`. | ||
| * | ||
| * Grouping re-buckets the active cards, so the one thing an upstream rewrite of | ||
| * SidebarV2 can silently break without breaking the render is the ordered | ||
| * thread list: it backs the jump labels, arrow navigation and range selection, | ||
| * and nothing about a grouped list *looks* wrong when that list still holds the | ||
| * flat order. Most of this file is about that seam. | ||
| */ | ||
|
|
||
| import * as NodeFS from "node:fs"; | ||
| import * as NodeURL from "node:url"; | ||
| import { describe, expect, it } from "vite-plus/test"; | ||
|
|
||
| import { UNGROUPED_PROJECT_KEY, groupThreadsByProject } from "../custom/sidebarV2ProjectGrouping"; | ||
|
|
||
| function readSibling(relativePath: string): string { | ||
| return NodeFS.readFileSync(NodeURL.fileURLToPath(new URL(relativePath, import.meta.url)), "utf8"); | ||
| } | ||
|
|
||
| const sidebar = readSibling("../components/SidebarV2.tsx"); | ||
| const chromeRows = readSibling("../custom/SidebarV2ChromeRows.tsx"); | ||
|
|
||
| describe("fork guard: sidebar-v2-project-grouping", () => { | ||
| it("offers the switch from the project scope menu", () => { | ||
| expect(chromeRows).toContain("Group by project"); | ||
| expect(chromeRows).toContain("onGroupByProjectChange"); | ||
| // Toggling a view preference must not dismiss the menu it lives in. | ||
| expect(chromeRows).toContain("closeOnClick={false}"); | ||
| expect(sidebar).toContain("groupByProject={groupByProject}"); | ||
| }); | ||
|
|
||
| it("drives keyboard order from the grouped order, not the flat one", () => { | ||
| // The whole point of the fenced hunk: orderedThreads is what jump hints, | ||
| // arrow nav and shift-range selection read, so it has to be the order rows | ||
| // are painted in. | ||
| expect(sidebar).toContain("[...orderedActiveThreads, ...visibleSnoozedThreads"); | ||
| const definition = sidebar.indexOf("const orderedActiveThreads"); | ||
| const use = sidebar.indexOf("[...orderedActiveThreads,"); | ||
| expect(definition).toBeGreaterThanOrEqual(0); | ||
| expect(use).toBeGreaterThan(definition); | ||
| }); | ||
|
|
||
| it("groups the active cards only, and not while a project scope is set", () => { | ||
| expect(sidebar).toContain("groupByProject && projectScopeKey === null"); | ||
| // The shelves keep their flat, time-ordered rendering. | ||
| expect(sidebar).toContain("for (const thread of renderedSettledThreads)"); | ||
| expect(sidebar).toContain("for (const thread of visibleSnoozedThreads)"); | ||
| }); | ||
|
|
||
| it("drops the card's project name under a project header", () => { | ||
| // Otherwise every card repeats the header two rows above it, and the branch | ||
| // — the half that actually separates two threads on one project — keeps | ||
| // yielding width to it. | ||
| // Scoped to the grouped section on purpose: the snoozed and settled shelves | ||
| // are not grouped, so their rows have no header to inherit the project | ||
| // from and must keep naming it themselves. | ||
| expect(sidebar).toContain('activeThreadProjectGroups !== null && section === "active"'); | ||
| }); | ||
|
|
||
| it("keeps every thread when a project no longer resolves to a group", () => { | ||
| const groups = groupThreadsByProject( | ||
| [ | ||
| { environmentId: "local", projectId: "p1" }, | ||
| { environmentId: "local", projectId: "deleted" }, | ||
| ], | ||
| [ | ||
| { | ||
| projectKey: "first", | ||
| displayName: "First", | ||
| memberProjectRefs: [{ environmentId: "local", projectId: "p1" }], | ||
| }, | ||
| ], | ||
| ); | ||
|
|
||
| expect(groups.flatMap((group) => group.threads)).toHaveLength(2); | ||
| expect(groups.at(-1)?.projectKey).toBe(UNGROUPED_PROJECT_KEY); | ||
| }); | ||
|
|
||
| it("keeps the caller's project order and each group's thread order", () => { | ||
| // Grouping must not become a second sort: upstream already ordered both | ||
| // the projects and the threads. | ||
| const groups = groupThreadsByProject( | ||
| [ | ||
| { id: "a", environmentId: "local", projectId: "p1" }, | ||
| { id: "b", environmentId: "local", projectId: "p2" }, | ||
| { id: "c", environmentId: "local", projectId: "p1" }, | ||
| ], | ||
| [ | ||
| { | ||
| projectKey: "second", | ||
| displayName: "Second", | ||
| memberProjectRefs: [{ environmentId: "local", projectId: "p2" }], | ||
| }, | ||
| { | ||
| projectKey: "first", | ||
| displayName: "First", | ||
| memberProjectRefs: [{ environmentId: "local", projectId: "p1" }], | ||
| }, | ||
| ], | ||
| ); | ||
|
|
||
| expect(groups.map((group) => group.projectKey)).toEqual(["second", "first"]); | ||
| expect(groups[1]?.threads.map((thread) => thread.id)).toEqual(["a", "c"]); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,13 +129,20 @@ import { | |
| SidebarV2WorkingRain, | ||
| type SidebarV2DotTone, | ||
| } from "~/custom/SidebarV2StatusIndicator"; | ||
| import { SidebarV2ThreadCardMeta } from "~/custom/SidebarV2ThreadCardMeta"; | ||
| import { SidebarV2ThreadCardMeta, threadCardShowsMetaRow } from "~/custom/SidebarV2ThreadCardMeta"; | ||
| import { SidebarV2ProjectScopeRow, SidebarV2SearchRow } from "~/custom/SidebarV2ChromeRows"; | ||
| import { | ||
| threadCardTitleClassName, | ||
| threadCardTitleRecedes, | ||
| threadRowSurfaceClassName, | ||
| } from "~/custom/sidebarV2RowPolicy"; | ||
| /* fork:begin sidebar-v2-project-grouping — see .fork/customizations.yaml#sidebar-v2-project-grouping */ | ||
| import { SidebarV2ProjectGroupHeader } from "~/custom/SidebarV2ProjectGroupHeader"; | ||
| import { | ||
| groupThreadsByProject, | ||
| useSidebarV2GroupByProject, | ||
| } from "~/custom/sidebarV2ProjectGrouping"; | ||
| /* fork:end sidebar-v2-project-grouping */ | ||
| import { getTriggerDisplayModelLabel } from "./chat/providerIconUtils"; | ||
| import { deriveProviderInstanceEntries, type ProviderInstanceEntry } from "../providerInstances"; | ||
| import { primaryServerProvidersAtom } from "../state/server"; | ||
|
|
@@ -822,11 +829,21 @@ const SidebarV2Row = memo(function SidebarV2Row(props: { | |
| } | ||
|
|
||
| const diff = latestTurnDiff(thread); | ||
| // A card is three lines only when it has a PR or a diff to put on the third; | ||
| // the hint has to follow, or the scrollbar lies about every skipped row. | ||
| const showsMetaRow = threadCardShowsMetaRow({ | ||
| hasPr: prBadge !== null, | ||
| insertions: diff?.insertions ?? null, | ||
| deletions: diff?.deletions ?? null, | ||
| }); | ||
|
|
||
| return ( | ||
| <li | ||
| data-thread-item | ||
| className="list-none py-0.5 [content-visibility:auto] [contain-intrinsic-size:auto_86px]" | ||
| className={cn( | ||
| "list-none py-0.5 [content-visibility:auto]", | ||
| showsMetaRow ? "[contain-intrinsic-size:auto_86px]" : "[contain-intrinsic-size:auto_64px]", | ||
| )} | ||
| > | ||
| <Tooltip> | ||
| <TooltipTrigger | ||
|
|
@@ -986,6 +1003,9 @@ export default function SidebarV2() { | |
| const confirmThreadDelete = useClientSettings((s) => s.confirmThreadDelete); | ||
| const sidebarProjectSortOrder = useClientSettings((s) => s.sidebarProjectSortOrder); | ||
| const projectGroupingSettings = useClientSettings(selectProjectGroupingSettings); | ||
| /* fork:begin sidebar-v2-project-grouping — see .fork/customizations.yaml#sidebar-v2-project-grouping */ | ||
| const [groupByProject, setGroupByProject] = useSidebarV2GroupByProject(); | ||
| /* fork:end sidebar-v2-project-grouping */ | ||
| const { settleThread, unsettleThread, snoozeThread, unsnoozeThread, deleteThread } = | ||
| useThreadActions(); | ||
| const updateThreadMetadata = useAtomCommand(threadEnvironment.updateMetadata, { | ||
|
|
@@ -1484,9 +1504,31 @@ export default function SidebarV2() { | |
| return routeThread === undefined ? [] : [routeThread]; | ||
| }, [routeThreadKey, snoozedShelfExpanded, snoozedThreads]); | ||
|
|
||
| /* fork:begin sidebar-v2-project-grouping — see .fork/customizations.yaml#sidebar-v2-project-grouping */ | ||
| // Null means "render flat", which is both the default and what a scoped | ||
| // sidebar gets: scoping already reduces the list to one project, so grouping | ||
| // it would only repeat the scope row's own label as a header. | ||
| const activeThreadProjectGroups = useMemo( | ||
| () => | ||
| groupByProject && projectScopeKey === null | ||
| ? groupThreadsByProject(activeThreads, projectGroups) | ||
| : null, | ||
| [activeThreads, groupByProject, projectGroups, projectScopeKey], | ||
| ); | ||
| // Grouping reorders the cards, and this list is what the ⌘-jump labels, | ||
| // arrow navigation and shift-range selection read. It has to be the order | ||
| // the rows are actually painted in, or a jump hint points at another row. | ||
| const orderedActiveThreads = useMemo( | ||
| () => | ||
| activeThreadProjectGroups === null | ||
| ? activeThreads | ||
| : activeThreadProjectGroups.flatMap((group) => [...group.threads]), | ||
| [activeThreadProjectGroups, activeThreads], | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this null-as-mode dual model is the structural problem. code-judo: extract a single section model in |
||
| ); | ||
| /* fork:end sidebar-v2-project-grouping */ | ||
| const orderedThreads = useMemo( | ||
| () => [...activeThreads, ...visibleSnoozedThreads, ...renderedSettledThreads], | ||
| [activeThreads, visibleSnoozedThreads, renderedSettledThreads], | ||
| () => [...orderedActiveThreads, ...visibleSnoozedThreads, ...renderedSettledThreads], | ||
| [orderedActiveThreads, visibleSnoozedThreads, renderedSettledThreads], | ||
| ); | ||
| const orderedThreadKeys = useMemo( | ||
| () => | ||
|
|
@@ -2215,6 +2257,8 @@ export default function SidebarV2() { | |
| void handleProjectActions(event, project); | ||
| }} | ||
| onAddProject={openAddProjectCommandPalette} | ||
| groupByProject={groupByProject} | ||
| onGroupByProjectChange={setGroupByProject} | ||
| /> | ||
| {/* fork:end fork-sidebar-chrome */} | ||
| <SidebarGroup className="min-h-0 flex-1 overflow-y-auto px-2 py-1 [scrollbar-gutter:stable]"> | ||
|
|
@@ -2285,11 +2329,19 @@ export default function SidebarV2() { | |
| projectCwd={ | ||
| projectCwdByKey.get(`${thread.environmentId}:${thread.projectId}`) ?? null | ||
| } | ||
| /* fork:begin sidebar-v2-project-grouping — see .fork/customizations.yaml#sidebar-v2-project-grouping */ | ||
| // Under a project header the card's repo line would only | ||
| // repeat the header two rows up, once per card. The | ||
| // branch, which is what actually tells two threads on one | ||
| // project apart, takes the whole line instead. | ||
| projectTitle={ | ||
| projectDisplayNameByKey.get( | ||
| `${thread.environmentId}:${thread.projectId}`, | ||
| ) ?? null | ||
| activeThreadProjectGroups !== null && section === "active" | ||
| ? null | ||
| : (projectDisplayNameByKey.get( | ||
| `${thread.environmentId}:${thread.projectId}`, | ||
| ) ?? null) | ||
| } | ||
|
Comment on lines
2380
to
2384
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this also: the fork guard currently pins this exact expression, which will fight the refactor. drop that string assert when you collapse the model. |
||
| /* fork:end sidebar-v2-project-grouping */ | ||
| providerEntryByInstanceId={providerEntryByInstanceId} | ||
| onThreadClick={handleThreadClick} | ||
| onThreadActivate={navigateToThread} | ||
|
|
@@ -2308,9 +2360,21 @@ export default function SidebarV2() { | |
| /> | ||
| ); | ||
| }; | ||
| const items: ReactNode[] = activeThreads.map((thread) => | ||
| renderThreadRow(thread, "active"), | ||
| ); | ||
| /* fork:begin sidebar-v2-project-grouping — see .fork/customizations.yaml#sidebar-v2-project-grouping */ | ||
| // Only the active cards group. The two shelves below are | ||
| // time-ordered tails whose value is that they stay short. | ||
| const items: ReactNode[] = | ||
| activeThreadProjectGroups === null | ||
| ? activeThreads.map((thread) => renderThreadRow(thread, "active")) | ||
| : activeThreadProjectGroups.flatMap((group, groupIndex) => [ | ||
| <SidebarV2ProjectGroupHeader | ||
| key={`project-group-header:${group.projectKey}`} | ||
| label={group.displayName} | ||
| isFirst={groupIndex === 0} | ||
| />, | ||
| ...group.threads.map((thread) => renderThreadRow(thread, "active")), | ||
| ]); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dual render path is the symptom of the null mode above. once sections always exist, this is one minor while you are here: |
||
| /* fork:end sidebar-v2-project-grouping */ | ||
| // Snoozed shelf: between the inbox and Settled — out of the | ||
| // way, never gone. The header always renders while anything | ||
| // is snoozed (the count is the whole footprint when | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these two cases re-test
groupThreadsByProjectorphan retention and project/thread order — already covered (better) incustom/sidebarV2ProjectGrouping.test.ts. the guard's job is the SidebarV2 seam (ordered list wiring, menu toggle, active-only / no-scope gating), not a second copy of the pure function's behavior.delete the duplicated behavior tests from the guard. keeping them here also encourages pinning implementation strings like the null-sentinel
projectTitlecheck, which locks in the dual-mode design we should be deleting.