refactor: always preserve marketplace search state in URL #35
Conversation
WalkthroughThese changes consolidate marketplace state management by removing internal Jotai atoms and their hydration wrapper, shifting to direct useQueryState hooks. Additionally, the usePluginPageContext hook's selector parameter type is relaxed to accept any type, reducing type constraints. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/app/components/plugins/marketplace/atoms.ts (1)
29-44: Verify the search mode logic inversion is intentional.The comment on lines 29-32 states that search mode should be forced for categories that do not have collections. However, line 43 now enables search mode when
PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType)is true—i.e., for categories that do have collections ('all'and'tool').The AI summary indicates the negation was removed, which inverts the semantics. Either the comment is stale, or the logic change is unintentional.
🐛 If the original behavior should be preserved, restore the negation
const isSearchMode = !!searchPluginText || filterPluginTags.length > 0 - || (searchMode ?? (PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType))) + || (searchMode ?? (!PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType)))
🤖 Fix all issues with AI agents
In `@web/app/components/plugins/marketplace/atoms.ts`:
- Around line 22-24: The hook useActivePluginType currently reads the wrong
query key ('tab'), causing a mismatch with the parser configuration; change the
query key passed to useQueryState from 'tab' to 'category' so it aligns with
marketplaceSearchParamsParsers.category and the search-params.ts configuration
(update the call in useActivePluginType to use 'category' instead of 'tab').
🧹 Nitpick comments (1)
web/app/components/plugins/plugin-page/context.tsx (1)
51-53: Avoid usinganyfor the selector parameter; use generics to preserve type safety.Changing the selector type to
anyremoves compile-time type checking for both the selector input and return type. This can lead to runtime errors that TypeScript would otherwise catch.♻️ Proposed fix using generics
-export function usePluginPageContext(selector: any) { +export function usePluginPageContext<T>(selector: (state: PluginPageContextValue) => T): T { return useContextSelector(PluginPageContext, selector) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
web/app/components/plugins/marketplace/atoms.tsweb/app/components/plugins/marketplace/hydration-client.tsxweb/app/components/plugins/marketplace/index.tsxweb/app/components/plugins/plugin-page/context.tsx
💤 Files with no reviewable changes (1)
- web/app/components/plugins/marketplace/hydration-client.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
web/app/components/plugins/marketplace/atoms.ts (2)
web/app/components/plugins/marketplace/search-params.ts (1)
marketplaceSearchParamsParsers(5-9)web/app/components/plugins/marketplace/constants.ts (1)
PLUGIN_CATEGORY_WITH_COLLECTIONS(25-30)
🔇 Additional comments (2)
web/app/components/plugins/marketplace/index.tsx (1)
22-34: LGTM!The simplified component structure by removing the
HydrateMarketplaceAtomswrapper aligns well with the PR objective of always preserving search state in the URL. The direct composition underHydrateQueryClientis cleaner.web/app/components/plugins/marketplace/atoms.ts (1)
19-27: Good refactor: DirectuseQueryStatehooks simplify state management.Removing the intermediate atoms and using
useQueryStatedirectly ensures search state is always persisted to the URL, which aligns with the PR objective.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| 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) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine search-params.ts
fd -n "search-params" -t f | head -20Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 313
🏁 Script executed:
# Look at the atoms.ts file around line 23
cat -n web/app/components/plugins/marketplace/atoms.ts | sed -n '15,30p'Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 666
🏁 Script executed:
# Search for marketplaceSearchParamsParsers definition and usage
rg -n "marketplaceSearchParamsParsers" -A 5 -B 2 | head -50Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 4484
🏁 Script executed:
# Search for query parameter key definitions - look for 'tab' and 'category'
rg -n "useQueryState\(" --type=ts --type=tsx -g "*marketplace*" -A 1 | head -40Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 110
🏁 Script executed:
# Search for 'tab' parameter usage in the marketplace
rg -n "tab" web/app/components/plugins/marketplace/ --type ts | grep -i "query\|param\|url\|state"Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 204
🏁 Script executed:
# Look for useActivePluginType usage to see how the hook is called
rg -n "useActivePluginType" --type ts --type jsRepository: qodo-benchmark/dify-combined-coderabbit
Length of output: 839
🏁 Script executed:
# Search for URL parameters or links that might use 'tab'
rg -n "\btab\b" web/app/components/plugins/marketplace/ --type ts | head -30Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 204
🏁 Script executed:
# Check if there are any URL building or navigation that references the params
rg -n "searchParams\|query\|params" web/app/components/plugins/marketplace/atoms.ts -A 2 -B 2Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 65
Query parameter key mismatch: 'tab' should be 'category'.
useActivePluginType uses 'tab' as the query parameter key, but marketplaceSearchParamsParsers.category is defined with key 'category' in search-params.ts. This mismatch means the URL will contain ?tab=... while the parser configuration and server-side code expect ?category=.... The parser options (default value, history behavior) are defined for the 'category' key and won't apply correctly to the 'tab' key.
🐛 Proposed fix
export function useActivePluginType() {
- return useQueryState('tab', marketplaceSearchParamsParsers.category)
+ return useQueryState('category', marketplaceSearchParamsParsers.category)
}🤖 Prompt for AI Agents
In `@web/app/components/plugins/marketplace/atoms.ts` around lines 22 - 24, The
hook useActivePluginType currently reads the wrong query key ('tab'), causing a
mismatch with the parser configuration; change the query key passed to
useQueryState from 'tab' to 'category' so it aligns with
marketplaceSearchParamsParsers.category and the search-params.ts configuration
(update the call in useActivePluginType to use 'category' instead of 'tab').
Benchmark PR from agentic-review-benchmarks#9
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.