Make Browse the default Extensions tab - #1115
Conversation
brsbl
changed the title
|
🚨 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 one pull request: #1115, “Make Browse the default Extensions tab.” I found no later stack pull requests after the required second search. I confirmed that @brsbl has write access. I will post phase results as each review finishes. |
|
🚨 SLOP COP 🚨 · Security phase complete: no security findings. I checked the new route memory, query parsing, same-origin navigation, injection risks, authorization boundaries, and registry requests. The changed code keeps navigation under The security agent also ran 60 focused tests and the |
|
🚨 SLOP COP 🚨 · End-to-end phase complete: passed on the top and only stack PR. I started the development app at head SHA
Page titles, selected tabs, and URLs matched each expected state. |
| location.pathname === getRegistrySkillsRoutePath() || | ||
| new URLSearchParams(location.search).get("view") === "browse"; | ||
| (location.pathname === getSkillsRoutePath() && | ||
| new URLSearchParams(location.search).get("view") !== "library"); |
There was a problem hiding this comment.
🚨 slopcop/review — The default Skills route starts a large registry request set.
This condition enables the list request and both enrichment request sets on /tools/skills. Browser QA recorded 20 registry requests on one visit.
Please reduce focus refetches and cache the list response before Browse becomes the default.
There was a problem hiding this comment.
Fixed in dc5b4c6. The Skills registry list now has a 30-minute stale window and disables focus refetches. Regression coverage verifies refocus and Library → Browse reuse the cached list without another request; the existing enrichment queries retain their longer caches and disabled focus refetches.
| label={item.label} | ||
| to={ | ||
| item.id === activeSection | ||
| ? currentRoutePath |
There was a problem hiding this comment.
🚨 slopcop/review — The active section row no longer returns to its list.
This target equals the current URL. A click on the active row now has no useful effect from a detail route.
Please use the section root for the active row. Keep the saved route for the other section.
There was a problem hiding this comment.
Fixed in dc5b4c6. The active section row now targets its section root, while the inactive section continues to use its saved route. The detail-route regression now verifies Plugins returns to .
|
🚨 SLOP COP 🚨 · Performance phase complete: one medium finding and two low findings.
I posted an inline comment for the request fan-out. The focused tests and the app typecheck still pass. |
| function modeFromSearchParams(value: string | null): PluginsCollectionMode { | ||
| if (value === "browse") return value; | ||
| return "installed"; | ||
| if (value === "installed") return value; |
There was a problem hiding this comment.
🚨 slopcop/review — The new default changes an old management redirect.
/settings/plugins still redirects to the bare Plugins path. It now opens Browse instead of Installed.
Please update that compatibility redirect to ?view=installed. Add a test for its search string.
There was a problem hiding this comment.
Fixed in dc5b4c6. The exact compatibility route now redirects to , with an assertion covering both pathname and search string.
|
🚨 SLOP COP 🚨 · Code quality and architecture phase complete: two user-visible defects remain.
I confirmed both defects in browser QA and posted inline comments. The owned-view mapping also appears in several files. A shared route helper can remove the repeated The broad review passed 531 tests, the app typecheck, and lint with no new warnings. |
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Review complete for the one-PR stack: #1115.
Security review found no issues.
The review found three main issues:
- The default Skills Browse route made 20 registry requests during browser QA.
/settings/pluginsnow opens Browse instead of Installed.- The active Extensions row no longer returns from a detail page to its section list.
Two low performance issues remain. The static plugin catalog can refetch on focus, and the old Browse redirect emits a noncanonical query.
The owned-view mapping appears in several files. A shared route helper can reduce this duplication.
Verification passed 531 tests, the app typecheck, and lint with no new warnings. End-to-end checks passed for defaults, tab memory, and Extensions re-entry.
I posted three inline comments with fixes. This review uses the comment-only state.
2 participants
Summary
Testing
pnpm exec vitest run --config vitest.config.ts src/hooks/useAppSettingsRouteMemory.test.tsx src/components/tools/ToolsSidebar.test.tsx src/components/plugin/PluginsOverview.test.tsx src/views/SkillsView.test.tsx src/components/layout/AppLayout.tools-breadcrumbs.test.ts src/App.legacy-skill-route.test.tsxpnpm exec turbo run typecheck --filter=@bb/app