Paginate skill detail content - #991
Conversation
|
🚨 SLOP COP 🚨 · I am the SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior. The stack contains only #991, “Paginate skill detail content.” I found no draft or unauthorized pull request to skip. |
|
🚨 SLOP COP 🚨 · Security phase complete. I found no security issue. This change adds no trust boundary, injection sink, network call, or data exposure. React receives only numeric values in the new transform style. |
|
🚨 SLOP COP 🚨 · Code quality phase complete. I found an actionable focus and scroll-state risk. Content outside the visible page stays active. Focus or find-in-page can move the hidden scroll container without changing the pagination state. The code also repeats viewport measurement logic from the shared pagination module. The browser phase will verify the user-visible behavior. |
|
🚨 SLOP COP 🚨 · Performance phase complete. I found no severe performance regression. The main concern is the React key at The pagination clips one complete document. It does not reduce parsing or DOM memory. The resize path also performs redundant measurements. |
|
🚨 SLOP COP 🚨 · End-to-end phase complete. All 41 focused tests passed. The Next control and viewport reflow worked. The browser test confirmed the hidden-scroll defect. Focusing an off-page link changed |
| <div | ||
| ref={setViewport} | ||
| data-skill-content-viewport | ||
| className="max-h-[60dvh] overflow-hidden" |
There was a problem hiding this comment.
🚨 slopcop/review — Hidden content can scroll without pagination state.
overflow-hidden remains a scroll container. In the browser test, an off-page link received focus. The viewport scrollTop changed from 0 to 2073.
The navigation still showed “Page 1 of 26.” This leaves the visual position and page state inconsistent.
Use overflow-clip, or prevent focus and scroll outside the visible page. Add a browser test for off-page focus.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Review complete for the one-pull-request stack: #991.
I found no security issue. The change adds no trust boundary, injection sink, dependency, or protocol change.
I found one defect that needs a fix before merge. The new overflow-hidden viewport remains programmatically scrollable. Off-page links also remain focusable.
The browser test focused one hidden link. The viewport scrollTop changed from 0 to 2073. The navigation still showed page 1 of 26.
Use overflow-clip, or prevent focus and scroll outside the visible page. Add a browser test for this case.
The performance review found no severe regression. The full-content React key creates avoidable work and remounts the preview after content changes.
The code repeats viewport measurement logic from the shared pagination module. A shared measurement hook could remove this duplicate code.
Verification completed:
- 41 focused tests passed.
- Normal Next navigation worked.
- Window reflow changed 26 pages to 34 pages and kept page 2 selected.
- The browser test reproduced the focus defect.
I used a comment review. I did not approve or request changes.
|
🚨 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. |
|
🚨 SLOP COP 🚨 · Security phase complete. I found no security issues in the nine pull requests. |
|
🚨 SLOP COP 🚨 · Performance phase complete. The phase raised one candidate concern in #991. The final review gate will check whether the pull request caused a regression. |
| <div | ||
| ref={setViewport} | ||
| data-skill-content-viewport | ||
| className="max-h-[60dvh] overflow-hidden" |
There was a problem hiding this comment.
🚨 slopcop/review — Hidden pages remain in the focus order and the accessibility tree.
overflow-hidden and the pixel transform only move one full FilePreview. Off-page links remain focusable, and page boundaries can split content.
Use normal scrolling or render only the active semantic page. Add a browser test with links on later pages.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Review complete.
I found one medium accessibility issue. Hidden pages remain available to keyboard users and screen readers.
The performance candidate is not a regression. The old code already parsed and rendered the full file.
The security phase found no issues. The final GPT-5.6 gate confirmed this finding.
The app typecheck passed. GitHub checks passed. Host load blocked the focused local test and browser route.
I used a comment-only review. I did not approve or request changes.
## Summary - reduce capability names using the established text-xs typography token - keep scheduled jobs and other shared detail-row consumers unchanged - add focused regression coverage for capability name styling ## Verification - pnpm exec turbo run test --filter=@bb/app -- --run src/components/tools/detail-page-recipes.test.tsx (38 passed) - pnpm exec turbo run typecheck --filter=@bb/app - git diff --check - visually verified Automations and Workflows plugin details in the branch dev app, including proportional, monospace, and long-description capability rows ## Stack Layer 2. Base: bb/skill-detail-pagination-thr_cxz5rwzns3 (PR #991).
## Summary Four regressions left plugin panes with broken chrome. Three came from #991; one predates it. Each was reproduced in the dev app before the fix. **Docs sidebar looked like the host's shared sidebar.** #991 restyled it to `bg-sidebar` and painted a fake background up into the app header. #1019 then gave every header its seam, which that background cut short. The sidebar sits below the header again, with the seam edge to edge and the plugin's own `bg-muted/20` tint. **The Docs header toggle did nothing.** #991 deleted the shared sidebar store once its mounts hit zero. The header and the sidebar live in separate React subtrees and find each other only through that module-level map, so a deleted entry left each subtree on a different store object: two toggles rendered, and the header one was dead (measured 288px to 288px on click). Entries now live for the session. A reopened pane still starts clean, because the view resets the state on its first mount. **Plugin panes lost full screen, move-to-side, and their corner reserve.** #991 excluded `plugin-panel` content from all three. Without the reserve, the host's pinned panel toggle landed on Close pane whenever a plugin pane held the workspace corner — measured overlap of 22 of 28px, with the toggle winning the click. Position alone decides the reserve now, and plugin panes carry the same pane controls as every other pane. **A pane with no right panel lost its toggle and placeholder.** #736 shipped both. #962 disabled the button and deleted the placeholder; #991 removed the control outright. Both are restored: the toggle always works (including `Ctrl+J`), and a pane that publishes no panel shows `This pane has no right panel.` in a resizable panel whose width persists. Since that empty state has no chrome of its own, the corner button stays visible while it is open — otherwise no button could close it. The host exposes this as `pinsCornerToggle`, and pane headers reserve the corner from the same flag. Two smaller fixes in the same area: - The reserve no longer leaves an empty 28px slot when the host hides its toggle. This showed as dead space beside Exit Full Screen and predates #991 (it came in with #781). - The pinned toggle moved from `right-2.5` to `right-4`, so it shares the pane header's `px-4` action axis. Measured after: Full Screen 1652–1680, Close pane 1684–1712, toggle 1716–1744 — an even 4px rhythm, with the toggle landing exactly in the reserved slot. ## Tests - `turbo run test --filter=@bb/app`: 2364 pass (315 files). - `turbo run test --filter=bb-plugin-simple-notes`: 59 pass. - `turbo run typecheck` passes for `@bb/app` and `bb-plugin-simple-notes`. Lint reports 0 errors. - New regression tests: the corner reserve for a plugin pane at the top right (verified failing against the old condition), the reserve dropping while a full-screen pane hides the toggle, and the empty panel state with a live toggle. - Updated the tests from #962 and #991 that locked in the removed controls. ## QA Checked in the branch dev app with the Docs, Tasks, and GitHub plugins installed: the header seam, a single working Docs toggle, plugin full screen and restore, no button collision, the empty-state panel opening and collapsing to 0px, and even header spacing. ## Risks Restoring move-to-side for plugin panes was not requested directly. It came out in the same #991 block as full screen, so I put it back with the rest. Say the word if it should stay off. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
Verification
Stack
Layer 1, based on main. Follow-on fixes should branch from this head and target this branch.