refactor: always preserve marketplace search state in URL#9
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the marketplace to always preserve search state in URL query parameters instead of conditionally using either URL state or local atom state. The changes simplify state management by removing the conditional logic and hydration mechanism.
Changes:
- Removed conditional state management logic that toggled between URL query params and local atoms
- Deleted the
HydrateMarketplaceAtomscomponent and related hydration setup - Simplified hooks to always use
useQueryStatefor marketplace filters
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| web/app/components/plugins/plugin-page/context.tsx | Weakened type signature for usePluginPageContext selector parameter |
| web/app/components/plugins/marketplace/index.tsx | Removed HydrateMarketplaceAtoms wrapper component |
| web/app/components/plugins/marketplace/hydration-client.tsx | Deleted file - no longer needed for conditional state hydration |
| web/app/components/plugins/marketplace/atoms.ts | Simplified state hooks to always use URL query parameters; changed query param name from 'category' to 'tab' |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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.
The negation operator was removed from this condition, which inverts the logic. According to the comment on line 30-31, categories WITHOUT collections should force search mode. However, this code now forces search mode for categories WITH collections (the opposite behavior). This should be !PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType) to match the intended behavior and the usage pattern in plugin-type-switch.tsx line 91-92, where categories with collections set searchMode to null.
| || (searchMode ?? (PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType))) | |
| || (searchMode ?? (!PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType))) |
| } | ||
|
|
||
| export function usePluginPageContext(selector: (value: PluginPageContextValue) => any) { | ||
| export function usePluginPageContext(selector: any) { |
There was a problem hiding this comment.
The type signature was changed from (selector: (value: PluginPageContextValue) => any) to (selector: any), removing type safety. This eliminates the ability to catch errors where the selector function expects the wrong context value type. Consider keeping the typed signature to maintain type safety across the codebase.
| export function usePluginPageContext(selector: any) { | |
| export function usePluginPageContext<T>(selector: (value: PluginPageContextValue) => T) { |
| 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.
The query parameter name was changed from 'category' to 'tab', but the server-side hydration code in hydration-server.tsx still uses marketplaceSearchParamsParsers which expects 'category'. This creates a mismatch: the server will look for '?category=...' in the URL (line 16 of hydration-server.tsx uses createLoader with marketplaceSearchParamsParsers), while the client reads/writes '?tab=...'. This will cause hydration failures and the server won't properly prefetch data based on the URL parameters. Either update the parser key in search-params.ts to 'tab', or keep using 'category' as the parameter name.
| return useQueryState('tab', marketplaceSearchParamsParsers.category) | |
| return useQueryState('category', marketplaceSearchParamsParsers.category) |
Benchmark PR from agentic-review-benchmarks#9