Prevent child thread names bleeding through sticky sidebar rows - #1183
Conversation
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
I am SlopCop. I am reviewing this pull request now.
I started parallel security, code quality, and performance reviews. I will also test the sidebar behavior when the local setup permits it.
| "linear-gradient(var(--state-active), var(--state-active))", | ||
| ); | ||
| expect(rule).toContain( | ||
| "linear-gradient(var(--sidebar), var(--sidebar))", |
There was a problem hiding this comment.
🚨 slopcop/review — These separate checks do not protect the required layer order.
The test still passes if a change swaps the layers. That swap puts the opaque sidebar above the active tint.
Please check the complete ordered background-image value. This check will then protect both the opaque backing and the visible selection.
| other content surfaces. A sticky sidebar row needs an opaque sidebar layer | ||
| beneath that tint, or scrolled descendant text paints through the row. */ | ||
| [data-sidebar-sticky-stack] | ||
| [data-sidebar-sticky-tier].bb-sidebar-selected-row { |
There was a problem hiding this comment.
🚨 slopcop/review — This selector leaves the same bleed path for open-in-split sticky rows.
The other split-pane row uses bg-sidebar-accent/50 in ThreadRow.tsx.
Tailwind Merge removes bg-sidebar when it merges that class into SidebarStickyTier. The row stays translucent, but it does not receive this marker.
A child name can still paint through that sticky row after a scroll. Please give that state an opaque backing too.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5
The sidebar pins a selected parent thread while its child rows move under it. The old tint let child names show through.
This PR adds a solid sidebar color below that tint. The selected sticky row now hides the child names.
Review result
I found one medium functional gap. I also found two low maintenance issues and one optional performance note.
I found no security issue.
Medium: the split-pane state still has the defect
ThreadRow.tsx:601 uses bg-sidebar-accent/50 for a thread in another split pane. Tailwind Merge removes the tier's bg-sidebar class.
The new rule only matches the selected-row marker. Therefore, the split-pane sticky row can still show a child name through its tint.
I left an inline comment on the new selector. I confirmed the class merge, but I did not reproduce this split state in the browser.
Low: the test does not check the layer order
The two toContain checks still pass after a layer swap. Such a swap puts the opaque layer above the selected tint.
I left an inline comment that asks for one ordered value check.
Low: one changed test file fails the format check
Prettier reports theme.test.ts. Prettier also reports sidebarRowClasses.ts, but that file had an earlier format difference on the base branch.
CI does not run this format check, so this issue does not fail the current checks.
Performance
The rule adds two image layers to no more than one selected sticky row. I found no practical render, layout, bundle, or paint problem.
One gradient and one solid color could produce the same result. That form needs the correct CSS layer, so the current form is reasonable.
Architecture and duplicate scan
The app already uses solid surface tokens where translucent surfaces cannot work. No current helper directly fits this selected sidebar state.
The web package copies the sticky-stack CSS block, but it has no sidebar component. This PR does not need to change that unused copy.
Verification
- The security review found no input, URL, overlay, or data leak risk.
- The full app test suite passed: 323 files and 2,430 tests.
- The app type check passed through Turbo.
- GitHub CI passed all active checks.
git diff --checkpassed.- The browser test scrolled 520 pixels with a child row directly under the selected parent.
- The selected row used the active tint above an opaque sidebar image. The screenshot showed no child text through the row.
I posted this review as a comment. I did not approve the pull request or request changes.
2 participants
Summary
Fixes #1161
Test plan
pnpm exec turbo run test --filter=@bb/app -- src/components/sidebar/sidebarRowClasses.test.ts src/components/ui/theme.test.tspnpm exec turbo run typecheck --filter=@bb/apppnpm exec turbo run lint --filter=@bb/app