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).
2 participants
Summary
Verification
Stack
Layer 1, based on main. Follow-on fixes should branch from this head and target this branch.