Unify Docs right-panel navigation - #1000
Conversation
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request under the The stack contains #993, #994, #995, #996, #997, #1000, and #1001. All seven pull requests are ready for review, and @brsbl has write access. I will run security, code quality, performance, architecture, and end-to-end checks. I will post each phase result when it completes. |
|
🚨 SLOP COP 🚨 · I am the SlopCop. I am reviewing this pull request under the The stack contains #994, #995, #996, #997, #1000, and #1001. I will review each change separately. I will judge each change against the final stack state. I found no skipped pull requests. All stack pull requests are ready. The author has write access. I started parallel security, code quality, and performance reviews. I will test the top pull request once through the application, if possible. |
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
I am SlopCop. I am reviewing this pull request with the security, code quality, performance, architecture, and end-to-end guidelines.
The stack contains #995, #996, #997, #1000, and #1001, from bottom to top. I will judge each pull request after the full stack lands. I will run the end-to-end test once on #1001.
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.
The stack contains these pull requests:
I will review security, code quality, performance, architecture, and tests. I will test the top pull request in the browser when possible.
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request under the configured security, quality, performance, architecture, and end-to-end test rules. The stack contains these pull requests, from bottom to top:
I will judge each pull request against the final state after the complete stack lands. I will run the end-to-end test on #1001 only. |
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this stack under the The stack contains these pull requests, from bottom to top:
I will run security, code quality and architecture, performance, and one end-to-end phase on top PR #1001. I skipped no pull requests. |
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Security phase complete for #1000.
The security phase is complete. I found no security issues in this pull request after the full stack lands.
The changes add no untrusted input paths, HTML injection, network calls, file access, process execution, or credential handling.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Performance phase complete for #1000.
The performance phase is complete. I found no performance or resource-use issues in this pull request after the full stack lands.
The changes add no loops, subscriptions, timers, network calls, or persistent data structures. The React and layout work stays small.
|
🚨 SLOP COP 🚨 · Security phase complete. I found no security issue in this PR after I reviewed the complete stack. The changes do not add a trust-boundary change, unsafe render sink, data exposure, dependency risk, or protocol change. |
|
🚨 SLOP COP 🚨 · Security phase complete: no finding. The refactor moves existing client controls and keeps their handlers. Search uses local string matching against an already loaded vault. It adds no route, authorization, data, dependency, or host protocol surface. |
|
🚨 SLOP COP 🚨 · Performance phase complete. I found no performance or resource-use defects in this pull request. I reviewed it against the final seven-pull-request stack. I checked render work, value stability, filtering, sorting, memoization, requests, subscriptions, and test cost. |
|
🚨 SLOP COP 🚨 · Security phase complete. I found no security or privacy defects in this pull request. I reviewed it against the final seven-pull-request stack. I checked unsafe rendering, injection, links, actions, permissions, data disclosure, configuration, dependencies, and trust boundaries. |
|
🚨 SLOP COP 🚨 · Security phase complete. No security or privacy findings. The refactor preserves action gates and keeps the search query out of the shared sidebar store. Validation included the complete stacked diff, trust-boundary tracing, and unsafe-render checks. |
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Performance review completed for PR #1000.
No performance finding exists. The change adds constant render and handler costs. It adds no network work or scale-dependent processing.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Security review completed for PR #1000.
No security finding exists. The change moves existing controls. It does not change authorization, input handling, data access, dependencies, or trust boundaries.
|
|
||
| function NotesSidebarNavigation(props: NotesSidebarNavigationProps) { | ||
| return ( | ||
| <nav |
There was a problem hiding this comment.
🚨 slopcop/review — Low: Use a toolbar for these commands.
This landmark contains Search, New note, and New folder commands. The note destinations remain outside the landmark. This structure gives screen readers an incorrect navigation landmark.
Use a toolbar for these commands. Put the note list in the navigation landmark.
| } | ||
| <div | ||
| data-testid="notes-sidebar-header" | ||
| className="-mr-4 flex h-12 shrink-0 items-center justify-end border-l border-border bg-sidebar pr-4" |
There was a problem hiding this comment.
🚨 slopcop/review — Medium: Keep this background segment clear of the host pane controls.
SplitThreadArea adds the maximize and close controls after this component. The negative right margin overlaps the next control. The segment also stops before the pane edge. This breaks the sidebar alignment in split Docs panes.
Please let the host own the full action area, or include the later controls in the aligned segment. Add a split-pane header test.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Code quality and architecture review completed for PR #1000.
I found one medium split-pane layout issue and one low accessibility issue. I posted both findings on the affected lines.
The change reuses the new sidebar components well. I found no harmful duplicate implementation elsewhere in the codebase.
| setSidebarWidth( | ||
| Math.min(480, Math.max(220, startWidth + startX - moveEvent.clientX)), | ||
| ); | ||
| updateNotesSidebarState(sidebarStore, { |
There was a problem hiding this comment.
🚨 slopcop/review — Coalesce sidebar resize updates once per frame.
This handler now writes every pointer move into the vault-wide external store. Each write notifies the header and all same-vault trees. Each tree rebuilds the full row list. High-rate pointer events can now cause extra full-tree renders during a drag.
Please buffer the latest width and write it once per animation frame. Cancel the pending frame during cleanup.
| } | ||
| <div | ||
| data-testid="notes-sidebar-header" | ||
| className="-mr-4 flex h-12 shrink-0 items-center justify-end border-l border-border bg-sidebar pr-4" |
There was a problem hiding this comment.
🚨 slopcop/review — Do not assume this plugin action is the final header item.
A split pane adds maximize and close controls after this plugin action. The negative margin then pulls those controls into the 288-pixel sidebar segment.
Please let the host own edge padding, or apply the bleed only when this action is last.
|
|
||
| function NotesSidebarNavigation(props: NotesSidebarNavigationProps) { | ||
| return ( | ||
| <nav |
There was a problem hiding this comment.
🚨 slopcop/review — Use toolbar semantics for this action row.
This landmark contains search, create, and close controls. The note tree is outside it. The “Notes sidebar” navigation landmark therefore does not contain the note navigation.
Please use a labelled toolbar or a plain group for these controls.
|
🚨 SLOP COP 🚨 · Performance phase complete. I found one low-risk issue. Drag-resize now writes every pointer event into a vault-wide external store. This can refresh all same-vault trees. The remaining render, search, and layout costs do not regress. |
|
🚨 SLOP COP 🚨 · Code quality and architecture phase complete. I found two issues. The header segment assumes that it is the final split-pane action. The action row also uses an incorrect navigation landmark. The shared toggle removes valid duplication. The resize clamp and state logic remain correct. |
|
🚨 SLOP COP 🚨 · Performance phase complete. No performance findings. The extraction preserves element count, state, handlers, and existing render behavior. Validation covered render cost, repeated work, layout, test runtime, and bundle impact. |
| // the user toggles the sidebar explicitly. | ||
| const sidebarCollapsed = sidebar.userCollapsed ?? sidebar.paneNarrow; | ||
| const [sidebarWidth, setSidebarWidth] = useState(288); | ||
| const sidebarWidth = sidebar.width; |
There was a problem hiding this comment.
🚨 slopcop/review — Sidebar width is now shared by all panes for one vault.
The store key contains only the first path segment. Two panes that show the same vault use one width value. A resize in one pane now resizes the other pane.
Keep the width per pane. Share only the value that connects one pane body to its header.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Result: I found one medium pane-state issue.
The sidebar width moved from local component state into a store keyed by vault. Two panes for one vault now share one width. A resize also causes updates in every matching pane.
The security review found no injection or permission issue. The repeated icon buttons can use a small local helper, but that refactor is not necessary for correctness.
Current CI passed.
Inline finding: #1000 (comment)
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Final review result for PR #1000: two findings.
- Medium: The Docs sidebar header segment conflicts with later split-pane controls. The current test does not use the real host header.
- Low: The action buttons use a navigation landmark, but the note destinations remain outside that landmark.
Security and performance reviews found no issue. The architecture scan found no harmful duplicate implementation. All active GitHub checks passed.
The browser test could not start because a shared Turbo cache lock blocked the repository QA server build.
The final GPT-5.6 gate confirmed both findings. I used a comment review. I did not approve or request changes on GitHub.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Review complete.
I found no issues in this pull request after the complete stack lands.
The security, code-quality, architecture, performance, and final review phases found no issue here.
I used a comment-only review. I did not approve or request changes.
| <span className="min-w-0 flex-1" /> | ||
| </> | ||
| )} | ||
| <div className="relative flex items-center border-b border-border p-2"> |
There was a problem hiding this comment.
🚨 slopcop/review — Restore the wrapper gap, or add spacing before the fallback toggle. When the shared header is absent and search is open, the navigation has no trailing spacer. The sibling collapse toggle then sits directly beside Close search with zero spacing. The old wrapper used gap-1, and the current test does not cover this fallback search state.
|
🚨 SLOP COP 🚨 · Code-quality and architecture phase complete. One visual regression remains in the fallback search layout. I filed an inline comment: #1000 (comment) I checked correctness, tests, duplication, architectural fit, stale names, and later stack fixes. |
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Code quality and architecture phase complete for #1000.
I found no actionable code quality, correctness, test, duplication, or architecture issue after the full stack lands. The extraction is clean and removes three copies of the sidebar toggle. The fallback and header mount paths remain covered.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Final review for #1000: no findings.
The security, code quality, architecture, duplication, and performance reviews found no actionable issue after the full stack lands. The one top-stack browser test passed through the Installed Plugins route and the Automations detail page.
All GitHub CI jobs passed on the top pull request. I did not approve or request changes, and I posted no inline findings.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Final comment-only review for #1000.
No actionable findings.
The security review found no security or privacy defect. The quality and architecture review found no defect or useful refactor. The performance review found no defect.
I ran the end-to-end attempt only on the top pull request. The shared host did not produce a ready development server.
GitHub CI passed on the latest stack head. The GPT-5.6 review gate approved this review result.
|
🚨 SLOP COP 🚨 · Performance phase complete. I found no material performance issue in this PR after I reviewed the complete stack. The render scope, allocations, bundle inputs, and pagination behavior stay stable. |
|
|
||
| function NotesSidebarNavigation(props: NotesSidebarNavigationProps) { | ||
| return ( | ||
| <nav |
There was a problem hiding this comment.
🚨 slopcop/review — This navigation landmark contains actions, not navigation.
The region contains search, create, and folder controls. None of these controls navigate.
Use a labeled toolbar or a plain div. This gives screen-reader users the correct landmark.
|
🚨 SLOP COP 🚨 · Code-quality and architecture phase complete. I left one accessibility finding. The named navigation landmark contains actions, not navigation. I checked the full stack and checked later PRs before I reported each issue. |
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Review complete with three findings.
- High: the sidebar header segment assumes it is the final action. Split-pane maximize and close controls follow it.
- Medium: the “Notes sidebar” navigation landmark contains only action controls, not the note tree.
- Low: drag-resize writes each pointer event into the vault-wide store and can refresh all same-vault trees.
Security checks found no issue. The browser test verified full-page expanded, collapsed, and restored sidebar behavior. All GitHub checks pass.
Please fix the split-pane header layout before merge.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
One nonblocking visual finding remains.
The fallback layout loses the old four-pixel gap when search is open. Close search can sit directly beside the fallback collapse toggle. See #1000 (comment).
Security and performance reviews found no problems. All GitHub checks passed. Browser QA confirmed the shared-header search, collapse, expand, and width handoff.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Review result: one accessibility finding.
- Security review found no issue.
- Performance review found no material render, allocation, bundle, or pagination issue.
- Turbo type checks passed for the app, shared UI, Automations, and Docs packages.
- The affected tests passed after clean reruns. The latest Docs suite passed all 57 tests.
The latest Docs route passed the browser collapse and expand checks. The action group uses an incorrect navigation landmark.
## Summary - restore Plugin as the Skills Type filter label - keep All, Plugin, and bb official semantics intact ## Verification - pnpm exec turbo run test --filter=@bb/app -- --run src/views/SkillsView.test.tsx - pnpm exec turbo run typecheck --filter=@bb/app - branch dev-app visual verification of default and toggled Type states
f16aea4
into
bb/compact-automation-filter-menus-thr_cxz5rwzns3
2 participants
Summary
Verification
pnpm exec turbo run test --filter=bb-plugin-simple-notes -- --run app.test.tsx(22 passed)pnpm exec turbo run typecheck --filter=bb-plugin-simple-notes