refactor: always preserve marketplace search state in URL #9
Conversation
📝 WalkthroughWalkthroughThis PR removes atom-based state management for marketplace search preservation, replacing it with direct Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@web/app/components/plugins/marketplace/atoms.ts`:
- Around line 40-44: isSearchMode computes fallback by using (searchMode ??
(PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType))) which is inverted: it
forces search mode for categories with collections; negate the set check so
categories without collections force search mode. Update the fallback expression
in the isSearchMode calculation to use (searchMode ??
!PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType)) (keeping the rest:
searchPluginText and filterPluginTags) so the logic uses searchMode when present
and otherwise enables search for categories not in
PLUGIN_CATEGORY_WITH_COLLECTIONS.
- Around line 19-27: The useActivePluginType hook uses the wrong query key
('tab') causing mismatch with the parser and server loader; update
useActivePluginType to call useQueryState with 'category' (i.e.,
useQueryState('category', marketplaceSearchParamsParsers.category)) so the URL
param name matches the parser and the server-side hydration expecting
params.category.
🧹 Nitpick comments (1)
web/app/components/plugins/plugin-page/context.tsx (1)
51-53: Restore typed selector for context access.
Line 51 now acceptsany, which drops compile-time safety and can hide bad field access in selectors. Prefer a generic signature so selector types stay aligned withPluginPageContextValue.♻️ Suggested change
-export function usePluginPageContext(selector: any) { - return useContextSelector(PluginPageContext, selector) -} +export function usePluginPageContext<T>( + selector: (value: PluginPageContextValue) => T, +) { + return useContextSelector(PluginPageContext, selector) +}
| export function useSearchPluginText() { | ||
| const preserveSearchStateInQuery = useAtomValue(preserveSearchStateInQueryAtom) | ||
| const queryState = useQueryState('q', marketplaceSearchParamsParsers.q) | ||
| const atomState = useAtom(searchPluginTextAtom) | ||
| return preserveSearchStateInQuery ? queryState : atomState | ||
| return useQueryState('q', marketplaceSearchParamsParsers.q) | ||
| } | ||
| export function useActivePluginType() { | ||
| const preserveSearchStateInQuery = useAtomValue(preserveSearchStateInQueryAtom) | ||
| const queryState = useQueryState('category', marketplaceSearchParamsParsers.category) | ||
| const atomState = useAtom(activePluginTypeAtom) | ||
| return preserveSearchStateInQuery ? queryState : atomState | ||
| return useQueryState('tab', marketplaceSearchParamsParsers.category) | ||
| } | ||
| export function useFilterPluginTags() { | ||
| const preserveSearchStateInQuery = useAtomValue(preserveSearchStateInQueryAtom) | ||
| const queryState = useQueryState('tags', marketplaceSearchParamsParsers.tags) | ||
| const atomState = useAtom(filterPluginTagsAtom) | ||
| return preserveSearchStateInQuery ? queryState : atomState | ||
| return useQueryState('tags', marketplaceSearchParamsParsers.tags) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for useQueryState calls with query param keys
echo "=== useQueryState calls with query param keys ==="
rg -nP --type=ts --type=tsx "useQueryState\('(?:tab|category|q|tags)'" -A 1
echo ""
echo "=== searchParams.get calls with query param keys ==="
rg -nP --type=ts --type=tsx "searchParams\.get\('(?:tab|category|q|tags)'" -A 1
echo ""
echo "=== Direct string references to 'tab' or 'category' param keys ==="
rg -nP --type=ts --type=tsx "['\"](tab|category)['\"].*(?:param|query|search)" -A 1
echo ""
echo "=== Look for marketplaceSearchParamsParsers definition ==="
rg -nP --type=ts --type=tsx "marketplaceSearchParamsParsers" -B 2 -A 10Repository: agentic-review-benchmarks/dify-combined-coderabbit
Length of output: 454
🏁 Script executed:
#!/bin/bash
# Find marketplace-related files
echo "=== Marketplace-related TypeScript files ==="
fd -e ts -e tsx "marketplace"
echo ""
echo "=== useQueryState calls ==="
rg "useQueryState\(" -A 1 | grep -E "(tab|category|q|tags)" -B 1 -A 1
echo ""
echo "=== searchParams\.get calls ==="
rg "searchParams\.get\(" | grep -E "(tab|category|q|tags)"
echo ""
echo "=== marketplaceSearchParamsParsers definition ==="
rg "marketplaceSearchParamsParsers" -B 5 -A 15Repository: agentic-review-benchmarks/dify-combined-coderabbit
Length of output: 10986
Fix query parameter key mismatch: use 'category' instead of 'tab' in useActivePluginType().
Line 23 uses useQueryState('tab', marketplaceSearchParamsParsers.category), but the parser key is 'category', not 'tab'. The nuqs library requires the URL parameter name to match the parser key name. The server-side loader in hydration-server.tsx expects params.category, and all other components in the codebase (provider-list, apps/list, plugin-page/context) consistently use 'category' as the query key. Change to: useQueryState('category', marketplaceSearchParamsParsers.category).
🤖 Prompt for AI Agents
In `@web/app/components/plugins/marketplace/atoms.ts` around lines 19 - 27, The
useActivePluginType hook uses the wrong query key ('tab') causing mismatch with
the parser and server loader; update useActivePluginType to call useQueryState
with 'category' (i.e., useQueryState('category',
marketplaceSearchParamsParsers.category)) so the URL param name matches the
parser and the server-side hydration expecting params.category.
| const searchMode = useAtomValue(searchModeAtom) | ||
| const isSearchMode = !!searchPluginText | ||
| || filterPluginTags.length > 0 | ||
| || (searchMode ?? (!PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType))) | ||
| || (searchMode ?? (PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType))) | ||
| return isSearchMode |
There was a problem hiding this comment.
Fallback search-mode check seems inverted.
The comment indicates we should force search mode for categories without collections, and PLUGIN_CATEGORY_WITH_COLLECTIONS implies the opposite set. Line 43 currently returns has(activePluginType) when searchMode is null, which would force search mode for categories with collections. If the constant name reflects its contents, this should be negated.
🐛 Suggested fix
- const isSearchMode = !!searchPluginText
- || filterPluginTags.length > 0
- || (searchMode ?? (PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType)))
+ const isSearchMode = !!searchPluginText
+ || filterPluginTags.length > 0
+ || (searchMode ?? !PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const searchMode = useAtomValue(searchModeAtom) | |
| const isSearchMode = !!searchPluginText | |
| || filterPluginTags.length > 0 | |
| || (searchMode ?? (!PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType))) | |
| || (searchMode ?? (PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType))) | |
| return isSearchMode | |
| const searchMode = useAtomValue(searchModeAtom) | |
| const isSearchMode = !!searchPluginText | |
| || filterPluginTags.length > 0 | |
| || (searchMode ?? !PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType)) | |
| return isSearchMode |
🤖 Prompt for AI Agents
In `@web/app/components/plugins/marketplace/atoms.ts` around lines 40 - 44,
isSearchMode computes fallback by using (searchMode ??
(PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType))) which is inverted: it
forces search mode for categories with collections; negate the set check so
categories without collections force search mode. Update the fallback expression
in the isSearchMode calculation to use (searchMode ??
!PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType)) (keeping the rest:
searchPluginText and filterPluginTags) so the logic uses searchMode when present
and otherwise enables search for categories not in
PLUGIN_CATEGORY_WITH_COLLECTIONS.
Benchmark PR from agentic-review-benchmarks#9
Summary by CodeRabbit