Repository navigation
revert(web): remove the Spaces sidebar grouping and thread tabs - #248
Conversation
NoahHendrickson
left a comment
There was a problem hiding this comment.
Code quality review of f9d4c398277c2363037c2ea6327a7eb4675af9dd: no major structural issues found.
I checked that this is a faithful revert, not just a large deletion:
- Leaving out index lines and hunk offsets,
git diff 3c8d7cec5e f9d4c39827is exactly the inverse of the #242 merge (06fe8eb4ef→ its first parent). git diff 06fe8eb4ef^1 f9d4c39827matchesgit diff 06fe8eb4ef 3c8d7cec5eline for line. So the tree is the pre-#242 state plus #243–#247, with no #242 changes left over and nothing from #243–#247 removed.- No references remain to
custom/spaces,fork-spaces,/changelog,ProviderMarks, or the?space=filter inapps/web/src,.fork, ordocs. useSidebarV2Groupinggoes back to the two-boolean model. A device that had Spaces selected had both of those booleans cleared, so it now reads as"none", which is the fallback the description gives. The leftoversidebar-v2-group-by-space:v1key is never read again, and that's acceptable.
On structure: the PR only deletes code. Sidebar.tsx drops from 5734 to 5465 lines and ChatView.tsx loses the tab-row wiring. The grouping helper goes back to three modes, so the "spaces" branches that #242 spread through the grouping, collapse and header code are gone. Both components are still well past 1k lines, but that was true before this PR and isn't something it adds.
I didn't rerun check:changed in this worktree because dependencies aren't installed. Since the tree is byte-for-byte a state whose parts each passed CI, the checks reported in the PR description are credible.
Reviewed by Claude Opus 5.5 (Claude Code). Comment only, not an approval.
NoahHendrickson
left a comment
There was a problem hiding this comment.
Code review — f9d4c398277c2363037c2ea6327a7eb4675af9dd
No issues found.
What I checked:
- Exact revert.
git diff 3c8d7cec5e f9d4c39827matches the inverse of the #242 merge (git diff 06fe8eb4ef 06fe8eb4ef^1) line for line. The only differences are blob indexes and hunk offsets, which moved because #243–#247 edited the same files. - Overlap with #243–#247. Four files were touched both by #242 and by later PRs:
.fork/customizations.yaml,sidebarV2CardRows.test.ts,ChatView.tsxandtheme.custom.css. None of the later edits touch the Spaces hunks, so the revert keeps all of their changes, including the #246 approval wiring in ChatView and the #247 favicon manifest text. - No dangling references. Nothing in
apps/web/srcor.fork/still mentionscustom/spaces,ProviderMarks,SpaceThreadTabs,fork-spaces,keepEmptyGroups, the"spaces"grouping value orchangelog, apart from the regeneratedrouteTree.gen.ts, which no longer has the route. - Stored preference fallback.
useSidebarV2Groupingnow reads only the project and status booleans. A device that hadgroup-by-space:v1 = trueresolves to"none", as the description says. The orphaned key is harmless because nothing reads it.
CI was still pending when I reviewed.
🤖 Generated with Claude Code
NoahHendrickson
left a comment
There was a problem hiding this comment.
Reviewed commit f9d4c398277c2363037c2ea6327a7eb4675af9dd against base 3c8d7cec5e7269e45fba10b4e45ac8672008669f. One regression found in the retained project grouping, detailed inline.
Validation: 91 test files / 679 tests passed (fork guards, project grouping, sidebar animation, Automations search logic, and split/float state). A focused headless Chromium reproduction using the base/head SidebarSplitPairRow components and the existing project-group CSS confirmed the hover and drop-indicator regression. No files changed.
| data-fork-row-shape={props.shape} | ||
| className={cn("grid grid-cols-2", SIDEBAR_V2_CARD_ALIGNMENT.listGap)} | ||
| > | ||
| <li className="list-none" data-fork-split-pair=""> |
There was a problem hiding this comment.
[P2] Preserve the project-section marker on split-pair rows
data-fork-project-section is also needed by the retained Group by project mode: the hover-reveal and drop-edge selectors in theme.custom.css walk sibling section items. Removing it from this outer <li> breaks that walk, because the marked thread cards are nested inside its <ul>. With a project whose visible cards form a split pair, hovering either card no longer reveals the header actions; during manual project reorder, the “below” indicator appears under the header rather than below the pair (or under an earlier card when one precedes the pair). This reproduces at the reviewed head and works at the supplied base. Keep the projectSection prop/call-site value and outer marker while removing the Spaces-specific shape/tray styling.
There was a problem hiding this comment.
Fixed in 57a9192: SidebarSplitPairRow takes the projectSection prop again and the call site passes options.underProjectHeader === true, so the outer <li> gets data-fork-project-section back. The Spaces-only shape prop and the data-fork-row-shape tray hook stay removed.
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
…oject The revert dropped the project-section marker #242 put on split-pair rows, which the retained project grouping's hover reveal and drop edge also rely on. Restore just the marker. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Spaces UX from #242 isn't the direction we want, so this takes it off
custom. The full feature is parked on thespaces-uxbranch (custom tip3c8d7cec5e) in case we revisit it.What changed
A clean
git revert -m 1of the #242 merge (06fe8eb4ef). That removes:apps/web/src/custom/spaces/, the/changelogroute, the provider marks, and thefork-spacesguard, manifest entry and CSS.?space=filter on Automations.One piece of #242 is kept: split-pair rows still carry
data-fork-project-section, which Group by project needs for its hover reveal and drop edge.The changes from #243–#247 are untouched. Anyone who had Spaces selected falls back to "No grouping". Spaces' old localStorage keys are left in place and nothing reads them.
Checks
vp run check:changed: format, lint, knip and typecheck pass. The tests pass except the Windows cross-arch case inscripts/build-desktop-artifact.test.ts, which also fails on pristine upstream on Apple Silicon..fork/lint-owned.mjsreports no blocking warnings.Reviewed with sol-loop: 2 rounds with gpt-6.1-sol on high. Sol verdict: approved
Built with Claude Opus 5.5 in Claude Code (running in T3 Code).
🤖 Generated with Claude Code