Skip to content

refactor: always preserve marketplace search state in URL#9

Open
tomerqodo wants to merge 2 commits intosentry_full_base_refactor_always_preserve_marketplace_search_state_in_url_pr9from
sentry_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 intosentry_full_base_refactor_always_preserve_marketplace_search_state_in_url_pr9from
sentry_full_head_refactor_always_preserve_marketplace_search_state_in_url_pr9

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#9

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.

Bug: The logic for isSearchMode is inverted, causing categories with collections to show the search view and categories without collections to incorrectly attempt to show the collections view.
Severity: CRITICAL

Suggested Fix

Restore the correct logic by re-adding the negation operator ! to the expression. The line should be changed back to || (searchMode ?? !PLUGIN_CATEGORY_WITH_COLLECTIONS.has(activePluginType)), which correctly enables search mode only for categories that do not have collections.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: web/app/components/plugins/marketplace/atoms.ts#L43

Potential issue: The logic determining whether to show the plugin collections view or
the search/list view has been inverted due to the removal of a `!` operator. In the new
code, when a user navigates to a plugin category that has collections (e.g., 'all' or
'tool'), the `isSearchMode` flag is incorrectly set to `true`, forcing the search view
and hiding the collections. Conversely, for categories without collections,
`isSearchMode` becomes `false`, causing the UI to incorrectly attempt to render a
non-existent collections view. This breaks the core navigation and display functionality
of the plugin marketplace.

Did we get this right? 👍 / 👎 to inform future reviews.

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.

Bug: The client-side hook uses the tab query parameter, but the server-side hydration logic still expects category. This mismatch breaks server-side rendering state preservation for the marketplace.
Severity: HIGH

Suggested Fix

To resolve the mismatch, update the server-side logic to expect the tab query parameter. In web/app/components/plugins/marketplace/search-params.ts, rename the category key in the marketplaceSearchParamsParsers object to tab to align with the client-side useQueryState('tab', ...) hook.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: web/app/components/plugins/marketplace/atoms.ts#L23

Potential issue: A mismatch exists between the client-side and server-side query
parameter names for the active plugin category. The client-side `useActivePluginType`
hook was updated to use the URL parameter `?tab=`, but the server-side rendering logic
in `hydration-server.tsx` still expects the parameter to be `?category=`. This
discrepancy breaks server-side rendering (SSR) hydration for the marketplace page. When
a user navigates to a URL with `?tab=...`, the server will fail to read the active tab
state, leading to an incorrect initial page render and loss of state on page reloads.

Did we get this right? 👍 / 👎 to inform future reviews.

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