Skip to content

refactor: always preserve marketplace search state in URL#9

Open
tomerqodo wants to merge 2 commits intogreptile_full_base_refactor_always_preserve_marketplace_search_state_in_url_pr9from
greptile_full_head_refactor_always_preserve_marketplace_search_state_in_url_pr9
Open

refactor: always preserve marketplace search state in URL#9
tomerqodo wants to merge 2 commits intogreptile_full_base_refactor_always_preserve_marketplace_search_state_in_url_pr9from
greptile_full_head_refactor_always_preserve_marketplace_search_state_in_url_pr9

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#9

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Jan 26, 2026

Greptile Overview

Greptile Summary

Refactored marketplace to always preserve search state in URL by removing conditional logic and the preserveSearchStateInQueryAtom. All search filters (q, tab, tags) now consistently use useQueryState instead of switching between query state and atom state.

Critical issues found:

  • Logic inverted in useMarketplaceSearchMode() - removed negation operator on line 43 reverses which categories trigger search vs collection mode
  • Query parameter renamed from category to tab breaks backward compatibility with existing bookmarked URLs
  • Type safety reduced by changing selector type to any, violating TypeScript rule refactor: always preserve marketplace search state in URL #9

Positive changes:

  • Simplified architecture by removing HydrateMarketplaceAtoms component and conditional state management
  • Cleaner code with consistent URL-based state management

Confidence Score: 1/5

  • This PR contains a critical logic error that will break core functionality
  • The inverted logic in useMarketplaceSearchMode() (atoms.ts:43) fundamentally breaks the marketplace's behavior by reversing which plugin categories use search mode vs collection mode. This will cause incorrect UI rendering and data fetching for all plugin categories
  • Pay close attention to web/app/components/plugins/marketplace/atoms.ts - the logic inversion on line 43 must be fixed before merging

Important Files Changed

Filename Overview
web/app/components/plugins/marketplace/atoms.ts Removed conditional URL persistence logic, but introduced critical logic inversion in search mode detection and URL param naming inconsistency
web/app/components/plugins/plugin-page/context.tsx Changed selector type from generic to any, reducing type safety

Sequence Diagram

sequenceDiagram
    participant User
    participant Browser
    participant Marketplace
    participant useSearchPluginText
    participant useActivePluginType
    participant useFilterPluginTags
    participant URL as URL Query Params
    participant useMarketplaceSearchMode
    participant useMarketplaceData

    User->>Browser: Navigate to marketplace
    Browser->>Marketplace: Load component
    Marketplace->>useSearchPluginText: Call hook
    useSearchPluginText->>URL: Read 'q' param via useQueryState
    URL-->>useSearchPluginText: Return search text
    
    Marketplace->>useActivePluginType: Call hook
    useActivePluginType->>URL: Read 'tab' param via useQueryState
    URL-->>useActivePluginType: Return active type
    
    Marketplace->>useFilterPluginTags: Call hook
    useFilterPluginTags->>URL: Read 'tags' param via useQueryState
    URL-->>useFilterPluginTags: Return tags array
    
    Marketplace->>useMarketplaceSearchMode: Calculate search mode
    useMarketplaceSearchMode->>useMarketplaceSearchMode: Check if activePluginType in PLUGIN_CATEGORY_WITH_COLLECTIONS
    Note over useMarketplaceSearchMode: BUG: Logic inverted<br/>Now returns true for 'all'/'tool'<br/>instead of 'model'/'agent'
    useMarketplaceSearchMode-->>Marketplace: Return isSearchMode
    
    Marketplace->>useMarketplaceData: Fetch data
    alt isSearchMode = true
        useMarketplaceData->>useMarketplaceData: Use search query
    else isSearchMode = false
        useMarketplaceData->>useMarketplaceData: Use collections query
    end
    
    User->>Marketplace: Change filter/search
    Marketplace->>URL: Update query params
    Note over URL: All state now persisted in URL<br/>(removed conditional logic)
Loading

Copy link
Copy Markdown

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

const isSearchMode = !!searchPluginText
|| filterPluginTags.length > 0
|| (searchMode ?? (!PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType)))
|| (searchMode ?? (PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType)))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic inverted - removed negation operator changes when search mode activates

Before: search mode was true for categories NOT in PLUGIN_CATEGORY_WITH_COLLECTIONS (e.g., 'model', 'agent')
After: search mode is true for categories IN the set (e.g., 'all', 'tool')

This completely reverses which plugin categories trigger search mode vs collection mode

Suggested change
|| (searchMode ?? (PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType)))
|| (searchMode ?? (!PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType)))
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/app/components/plugins/marketplace/atoms.ts
Line: 43:43

Comment:
logic inverted - removed negation operator changes when search mode activates

Before: search mode was `true` for categories NOT in `PLUGIN_CATEGORY_WITH_COLLECTIONS` (e.g., 'model', 'agent')
After: search mode is `true` for categories IN the set (e.g., 'all', 'tool')

This completely reverses which plugin categories trigger search mode vs collection mode

```suggestion
    || (searchMode ?? (!PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType)))
```

How can I resolve this? If you propose a fix, please make it concise.

}

export function usePluginPageContext(selector: (value: PluginPageContextValue) => any) {
export function usePluginPageContext(selector: any) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any type bypasses TypeScript type checking, violating project rule #9

The selector should use a generic type parameter like other contexts in the codebase (e.g., app-context.tsx:48, modal-context.tsx:67)

Suggested change
export function usePluginPageContext(selector: any) {
export function usePluginPageContext<T>(selector: (value: PluginPageContextValue) => T): T {

Context Used: Context from dashboard - AGENTS.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI
This is a comment left during a code review.
Path: web/app/components/plugins/plugin-page/context.tsx
Line: 51:51

Comment:
`any` type bypasses TypeScript type checking, violating project rule #9

The selector should use a generic type parameter like other contexts in the codebase (e.g., `app-context.tsx:48`, `modal-context.tsx:67`)

```suggestion
export function usePluginPageContext<T>(selector: (value: PluginPageContextValue) => T): T {
```

**Context Used:** Context from `dashboard` - AGENTS.md ([source](https://app.greptile.com/review/custom-context?memory=16b28b0e-8897-4cdb-9b17-0e9c857390af))

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

const queryState = useQueryState('category', marketplaceSearchParamsParsers.category)
const atomState = useAtom(activePluginTypeAtom)
return preserveSearchStateInQuery ? queryState : atomState
return useQueryState('tab', marketplaceSearchParamsParsers.category)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed query param name from category to tab - this breaks URL compatibility

Users with existing bookmarks or shared links using ?category=tool will lose their filter state. The parser still expects category (marketplaceSearchParamsParsers.category in search-params.ts:6)

Prompt To Fix With AI
This is a comment left during a code review.
Path: web/app/components/plugins/marketplace/atoms.ts
Line: 23:23

Comment:
changed query param name from `category` to `tab` - this breaks URL compatibility

Users with existing bookmarks or shared links using `?category=tool` will lose their filter state. The parser still expects `category` (`marketplaceSearchParamsParsers.category` in `search-params.ts:6`)

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants