Code Review Bench PR #31069 - refactor: always preserve marketplace search state in URL #9
Conversation
![gitar-bot-staging[bot]](https://avatars.githubusercontent.com/in/812680?s=60&v=4){kind=small}
| const isSearchMode = !!searchPluginText | ||
| || filterPluginTags.length > 0 | ||
| || (searchMode ?? (!PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType))) | ||
| || (searchMode ?? (PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType))) |
There was a problem hiding this comment.
🚨 Bug: Logic inversion contradicts comment and breaks search mode
The negation was removed from !PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType), changing it to PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType). This inverts the search mode default behavior.
The comment directly above (lines 29-32) states: "Not all categories have collections, so we need to force the search mode for those categories."
PLUGIN_CATEGORY_WITH_COLLECTIONS contains only 'all' and 'tool'. The old logic correctly forced search mode for categories without collections (e.g., model, agent, extension, datasource, trigger, bundle). The new logic forces search mode for categories with collections (all, tool), which is the opposite of the documented intent.
Impact: Users browsing categories without collections (model, agent, extension, etc.) will see a collections view (which has no data for those categories), while categories with collections will incorrectly default to search mode. This fundamentally breaks the marketplace browsing experience for most plugin categories.
Was this helpful? React with 👍 / 👎
| || (searchMode ?? (PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType))) | |
| || (searchMode ?? (!PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType))) |
- Apply suggested fix
| const queryState = useQueryState('category', marketplaceSearchParamsParsers.category) | ||
| const atomState = useAtom(activePluginTypeAtom) | ||
| return preserveSearchStateInQuery ? queryState : atomState | ||
| return useQueryState('tab', marketplaceSearchParamsParsers.category) |
There was a problem hiding this comment.
⚠️ Bug: Client-server URL param mismatch: 'tab' vs 'category'
The client-side useActivePluginType() now writes to the URL parameter 'tab', but the server-side hydration-server.tsx uses createLoader(marketplaceSearchParamsParsers) which reads the category key from the URL (defined in search-params.ts).
In hydration-server.tsx:16-17, createLoader from nuqs uses the object keys of the parsers to determine which URL parameters to read. Since the key is category, the server reads ?category=.... But the client now writes to ?tab=....
Impact: Server-side prefetching will never see the active plugin type from the URL. When a user navigates to ?tab=tool, the server will default to 'all' and prefetch the wrong data. This defeats the purpose of server-side hydration.
Fix: Either rename the key in marketplaceSearchParamsParsers from category to tab, or keep using 'category' as the URL parameter name in useActivePluginType().
Was this helpful? React with 👍 / 👎
| return useQueryState('tab', marketplaceSearchParamsParsers.category) | |
| return useQueryState('category', marketplaceSearchParamsParsers.category) |
- Apply suggested fix
There was a problem hiding this comment.
Configure merge blocking · Maintainers can dismiss this review. Gitar never approves changes.
Code Review 🚫 Blocked 0 resolved / 3 findingsThe simplification of dual-state to URL-only is a good direction, but the logic inversion in 🚨 Bug: Logic inversion contradicts comment and breaks search mode📄 web/app/components/plugins/marketplace/atoms.ts:43 The negation was removed from The comment directly above (lines 29-32) states: "Not all categories have collections, so we need to force the search mode for those categories."
Impact: Users browsing categories without collections (model, agent, extension, etc.) will see a collections view (which has no data for those categories), while categories with collections will incorrectly default to search mode. This fundamentally breaks the marketplace browsing experience for most plugin categories. Suggested fix
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
3 participants
Benchmark PR for Gitar evaluation
Original PR: agentic-review-benchmarks#9
refactor: always preserve marketplace search state in URL
Summary by Gitar
preserveSearchStateInQueryAtom, local atoms (searchPluginTextAtom,activePluginTypeAtom,filterPluginTagsAtom), andHydrateMarketplaceAtomscomponentuseSearchPluginText,useActivePluginType,useFilterPluginTags) now directly useuseQueryState()fromnuqsuseMarketplaceSearchMode(): inverted negation forPLUGIN_CATEGORY_WITH_COLLECTIONSThis will update automatically on new commits.