Added category filter and unified sorting to Projects search component#4168
Open
anurag2787 wants to merge 16 commits intoOWASP:mainfrom
Open
Added category filter and unified sorting to Projects search component#4168anurag2787 wants to merge 16 commits intoOWASP:mainfrom
anurag2787 wants to merge 16 commits intoOWASP:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR implements a full-stack category filtering and unified search system. Adds a hierarchical ProjectCategory model with REST and GraphQL APIs, refactors frontend search into a reusable UnifiedSearchBar component, and enables filtering/sorting via both Algolia and GraphQL backends. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/app/projects/dashboard/metrics/page.tsx (1)
170-177:⚠️ Potential issue | 🟠 MajorURL-driven state sync is incomplete for search/filter.
Line 170-Line 177 rehydrates only ordering from
searchParams. On back/forward navigation,searchQueryandfilterscan diverge from URL, so fetched results no longer match visible URL state.💡 Proposed fix
useEffect(() => { const { field: f, direction: d } = parseOrderParam(searchParams.get('order')) const nextOrdering = buildGraphQLOrdering(f, d) if (JSON.stringify(nextOrdering) !== JSON.stringify(ordering)) { setOrdering(nextOrdering) } + + const nextSearch = searchParams.get('search') || '' + if (nextSearch !== searchQuery) { + setSearchQuery(nextSearch) + } + + const nextHealth = searchParams.get('health') + const nextLevel = searchParams.get('level') + let nextFilters = {} + if (nextHealth && nextHealth in healthFiltersMapping) { + nextFilters = healthFiltersMapping[nextHealth as keyof typeof healthFiltersMapping] + } else if (nextLevel && nextLevel in levelFiltersMapping) { + nextFilters = levelFiltersMapping[nextLevel as keyof typeof levelFiltersMapping] + } + if (JSON.stringify(nextFilters) !== JSON.stringify(filters)) { + setFilters(nextFilters) + } // eslint-disable-next-line react-hooks/exhaustive-deps }, [searchParams])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/projects/dashboard/metrics/page.tsx` around lines 170 - 177, The effect that currently only rehydrates ordering (inside the useEffect using parseOrderParam/buildGraphQLOrdering and setOrdering) must also parse and apply URL-driven search and filter state: read the relevant query param keys from searchParams (e.g. the search query param like 'q' and the filters param(s)), deserialize them into the same shapes used by the component, compare to current searchQuery and filters (deep-equal or JSON.stringify) and call setSearchQuery and setFilters when they differ; update the useEffect that references parseOrderParam/buildGraphQLOrdering to include this logic so back/forward navigation keeps searchQuery, filters, and ordering in sync with the URL.
🧹 Nitpick comments (4)
frontend/src/components/Search.tsx (1)
16-16:testIdprop is currently dead API surface.
testIdis added to props but never applied to the DOM, so consumers can pass it without effect. Either wire it (for example asdata-testid) or remove it to avoid confusion.Proposed fix
<input ref={inputRef} id={inputId} + data-testid={testId} type="text" value={searchQuery} onChange={handleSearchChange} placeholder={placeholder} className={inputClassName} />Also applies to: 27-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/Search.tsx` at line 16, The testId prop is declared on the Search component but never applied to the DOM; either remove testId from the props interface/type or wire it into the rendered element(s) so it is useful. To fix, update the Search component (the functional component named Search and its props type that includes testId?: string) to add data-testid={testId} on the root DOM element (e.g., the top-level div/Container returned by Search) and guard it so it only renders when testId is provided, or alternatively remove testId from the props type and consumers; make the change consistently where the prop is declared/used in Search.tsx.backend/apps/owasp/api/internal/queries/project_health_metrics.py (1)
61-64: Extract shared query-filtering logic to keep list/count behavior locked together.The same normalization +
icontainsblock is duplicated in both resolvers. A shared helper reduces drift risk.Refactor sketch
class ProjectHealthMetricsQuery: @@ + `@staticmethod` + def _apply_query_filter(queryset, query: str): + cleaned_query = query.strip() if query else "" + if cleaned_query: + return queryset.filter(project__name__icontains=cleaned_query) + return queryset @@ - cleaned_query = query.strip() if query else "" - if cleaned_query: - queryset = queryset.filter(project__name__icontains=cleaned_query) + queryset = self._apply_query_filter(queryset, query) @@ - cleaned_query = query.strip() if query else "" - if cleaned_query: - queryset = queryset.filter(project__name__icontains=cleaned_query) + queryset = self._apply_query_filter(queryset, query)Also applies to: 102-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/owasp/api/internal/queries/project_health_metrics.py` around lines 61 - 64, Extract the duplicated normalization + icontains filtering into a single helper function (e.g., apply_project_name_query_filter(queryset, query)) that performs cleaned_query = query.strip() if query else "" and, if len(cleaned_query) >= MIN_SEARCH_QUERY_LENGTH, returns queryset.filter(project__name__icontains=cleaned_query) else returns the original queryset; replace the duplicated blocks in both resolver functions (the list resolver and the count resolver in project_health_metrics.py) to call this helper so list/count behavior stays identical and DRY.frontend/src/utils/sortingOptions.ts (1)
11-17: TypetypeOptionsProjectwith the shared option type for stronger safety.Using
CategoryOption[]here prevents shape drift across search option sources.Suggested refactor
+import type { CategoryOption } from 'types/category' ... -export const typeOptionsProject = [ +export const typeOptionsProject: CategoryOption[] = [ { label: 'All Types', key: '' }, { label: 'Code', key: 'idx_type:code' }, { label: 'Tool', key: 'idx_type:tool' }, { label: 'Documentation', key: 'idx_type:documentation' }, { label: 'Other', key: 'idx_type:other' }, ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/utils/sortingOptions.ts` around lines 11 - 17, The array typeOptionsProject is untyped and should use the shared CategoryOption type to prevent shape drift; update the declaration of typeOptionsProject to have the type CategoryOption[] and ensure you import or reference the existing CategoryOption type used by other search option sources so each item matches that interface (e.g., keep objects with label and key) and adjust any mismatched properties to conform.frontend/src/hooks/useSearchProjectsGraphQL.ts (1)
99-100: Counting viaprojectsTotal.lengthis expensive at scale.Fetching all matching IDs just to compute total count adds unnecessary payload and memory pressure. Prefer a dedicated count field (e.g.,
searchProjectsCount) from GraphQL and return an integer directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useSearchProjectsGraphQL.ts` around lines 99 - 100, The code currently computes totalProjects by counting data?.projectsTotal?.length which fetches all matching IDs and is costly; update the GraphQL schema/query to expose a dedicated integer count (e.g., searchProjectsCount), update the useSearchProjectsGraphQL query to request that field instead of projectsTotal, and replace the usage of data?.projectsTotal?.length with data?.searchProjectsCount || 0; also update any related TypeScript types/interfaces and server resolver to return the integer count to avoid loading the full ID list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/api/rest/v0/project.py`:
- Around line 109-124: The public REST ordering Literal named ordering is
missing the level_raw option while fallback ordering and existing sort keys
still reference "level_raw" and "-level_raw"; add both "level_raw" and
"-level_raw" to the ordering Literal so the public contract matches the fallback
and existing sort keys (ensure the symbol ordering in project.py includes
"level_raw" and "-level_raw" and that any fallback or sortBy handling code that
mentions "-level_raw" remains consistent).
In `@backend/apps/owasp/api/internal/queries/project_health_metrics.py`:
- Around line 62-64: The current guard (if cleaned_query and len(cleaned_query)
>= MIN_SEARCH_QUERY_LENGTH) skips filtering for non-empty short queries so
results remain unfiltered; change the logic to apply the icontains filter
whenever cleaned_query is non-empty (i.e., if cleaned_query: queryset =
queryset.filter(project__name__icontains=cleaned_query)) and remove the length
check, and make the same change at the second occurrence (the other resolver
block using cleaned_query, MIN_SEARCH_QUERY_LENGTH, and queryset) so 1–2
character inputs produce filtered results immediately.
In `@frontend/src/app/projects/dashboard/metrics/page.tsx`:
- Around line 121-122: The page currently computes currentCategory as
healthFilter || levelFilter which lets an old health filter mask a newly
selected level because the level selection code (select level handler around the
block that retains both filters) doesn't clear the other filter; change the
selection logic so the UI behaves as a single-select: when setting levelFilter
(e.g., in the level selection handler referenced in the 240-247 block)
explicitly clear healthFilter, and when setting healthFilter explicitly clear
levelFilter; also update currentCategory to use levelFilter || healthFilter ||
'' (or otherwise rely on the most recently set filter) so the visible dropdown
reflects the newly selected single category. Ensure you update the functions
that modify filters (the level selection handler and the health selection
handler) rather than only changing the computed value.
In `@frontend/src/hooks/useSearchPage.ts`:
- Around line 159-160: The computed total pages for GraphQL results forces a
minimum of 1 which diverges from Algolia's behavior; change the calculation in
the block using calculatedTotalPages, graphqlTotalCount, hitsPerPage and
setTotalPages so that when graphqlTotalCount is 0 calculatedTotalPages becomes 0
(e.g., remove the "|| 1" behavior) and then update setTotalPages((prev) => (prev
=== calculatedTotalPages ? prev : calculatedTotalPages)) accordingly so
empty-result pagination matches Algolia.
- Around line 60-67: The GraphQL hook useSearchProjectsGraphQL is being invoked
unconditionally in useSearchPage, causing GraphQL requests even when Algolia is
selected; change useSearchPage to only call useSearchProjectsGraphQL when
GraphQL backend is active (e.g., guard the call with a backend check like
backend === 'graphql' or !isAlgoliaBackend) or pass an enabled flag into the
hook call, and then update useSearchProjectsGraphQL.ts so the hook accepts an
options.enabled (or maps to Apollo's skip) and does not execute the query when
enabled is false; reference the hook invocation
useSearchProjectsGraphQL(searchQuery, category, sortBy, order, currentPage,
hitsPerPage, { pageSize: hitsPerPage }) and the hook implementation in
useSearchProjectsGraphQL.ts to add/consume the enabled/skip behavior.
In `@frontend/src/hooks/useSearchProjectsGraphQL.ts`:
- Around line 62-75: The fieldMapping in useSearchProjectsGraphQL.ts is missing
a mapping for the UI's "level_raw" key causing sortBy to send level_raw to
GraphQL; update the fieldMapping object to include ['level_raw', 'level'] (so
that when sortBy === 'level_raw' graphQLField becomes 'level') and keep the
existing fallback behavior using graphQLField/ sortBy and return [{
[graphQLField]: orderDirection }] unchanged; ensure any other UI-specific raw
keys are similarly mapped to their GraphQL ProjectOrder equivalents.
In `@frontend/src/server/fetchAlgoliaData.ts`:
- Around line 14-16: The code in fetchAlgoliaData (around variables facetFilters
and indexName) mutates the incoming facetFilters array with
facetFilters.push('idx_is_active:true'), which can cause duplicate filters if
the same array is reused; instead, do not mutate the caller's array—create a new
array when adding the active filter (e.g., use a spread/concat to produce a
newFacetFilters or check for existing 'idx_is_active:true' before adding) and
then use that new array for the Algolia query so fetchAlgoliaData never mutates
its input.
In `@frontend/src/server/queries/projectQueries.ts`:
- Around line 207-209: The current projectsTotal field calls searchProjects and
returns all matching ids, causing a full search payload; change the query to use
a dedicated count endpoint/field (e.g., searchProjectsCount) instead of
searchProjects to compute totals. Update the client-side query where
projectsTotal is defined to call searchProjectsCount(query: $query, filters:
$filters) and remove the id selection, and ensure any resolver or GraphQL schema
that currently backs projectsTotal maps to or exposes the new
searchProjectsCount field so the server returns only an integer count rather
than full nodes. Also update any usages of projectsTotal in components to expect
a number instead of an array.
In `@frontend/src/utils/backendConfig.ts`:
- Around line 24-25: The check "envBackend in SEARCH_BACKENDS" can match
inherited properties; update the validation to use an own-property check such as
Object.prototype.hasOwnProperty.call(SEARCH_BACKENDS, envBackend) (or use
Object.keys/values and .includes) so only real keys in SEARCH_BACKENDS are
accepted; locate the conditional that references envBackend and SEARCH_BACKENDS
and replace the "in" check with the hasOwnProperty-style check, keeping the
return envBackend as SearchBackend behavior unchanged.
---
Outside diff comments:
In `@frontend/src/app/projects/dashboard/metrics/page.tsx`:
- Around line 170-177: The effect that currently only rehydrates ordering
(inside the useEffect using parseOrderParam/buildGraphQLOrdering and
setOrdering) must also parse and apply URL-driven search and filter state: read
the relevant query param keys from searchParams (e.g. the search query param
like 'q' and the filters param(s)), deserialize them into the same shapes used
by the component, compare to current searchQuery and filters (deep-equal or
JSON.stringify) and call setSearchQuery and setFilters when they differ; update
the useEffect that references parseOrderParam/buildGraphQLOrdering to include
this logic so back/forward navigation keeps searchQuery, filters, and ordering
in sync with the URL.
---
Nitpick comments:
In `@backend/apps/owasp/api/internal/queries/project_health_metrics.py`:
- Around line 61-64: Extract the duplicated normalization + icontains filtering
into a single helper function (e.g., apply_project_name_query_filter(queryset,
query)) that performs cleaned_query = query.strip() if query else "" and, if
len(cleaned_query) >= MIN_SEARCH_QUERY_LENGTH, returns
queryset.filter(project__name__icontains=cleaned_query) else returns the
original queryset; replace the duplicated blocks in both resolver functions (the
list resolver and the count resolver in project_health_metrics.py) to call this
helper so list/count behavior stays identical and DRY.
In `@frontend/src/components/Search.tsx`:
- Line 16: The testId prop is declared on the Search component but never applied
to the DOM; either remove testId from the props interface/type or wire it into
the rendered element(s) so it is useful. To fix, update the Search component
(the functional component named Search and its props type that includes testId?:
string) to add data-testid={testId} on the root DOM element (e.g., the top-level
div/Container returned by Search) and guard it so it only renders when testId is
provided, or alternatively remove testId from the props type and consumers; make
the change consistently where the prop is declared/used in Search.tsx.
In `@frontend/src/hooks/useSearchProjectsGraphQL.ts`:
- Around line 99-100: The code currently computes totalProjects by counting
data?.projectsTotal?.length which fetches all matching IDs and is costly; update
the GraphQL schema/query to expose a dedicated integer count (e.g.,
searchProjectsCount), update the useSearchProjectsGraphQL query to request that
field instead of projectsTotal, and replace the usage of
data?.projectsTotal?.length with data?.searchProjectsCount || 0; also update any
related TypeScript types/interfaces and server resolver to return the integer
count to avoid loading the full ID list.
In `@frontend/src/utils/sortingOptions.ts`:
- Around line 11-17: The array typeOptionsProject is untyped and should use the
shared CategoryOption type to prevent shape drift; update the declaration of
typeOptionsProject to have the type CategoryOption[] and ensure you import or
reference the existing CategoryOption type used by other search option sources
so each item matches that interface (e.g., keep objects with label and key) and
adjust any mismatched properties to conform.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectsHealthDashboardQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (23)
backend/apps/api/rest/v0/project.pybackend/apps/owasp/api/internal/filters/project.pybackend/apps/owasp/api/internal/ordering/project.pybackend/apps/owasp/api/internal/queries/project.pybackend/apps/owasp/api/internal/queries/project_health_metrics.pybackend/apps/owasp/index/registry/project.pybackend/apps/owasp/index/search/project.pyfrontend/.env.e2e.examplefrontend/.env.examplefrontend/src/app/projects/dashboard/metrics/page.tsxfrontend/src/app/projects/page.tsxfrontend/src/components/Search.tsxfrontend/src/components/SearchPageLayout.tsxfrontend/src/components/UnifiedSearchBar.tsxfrontend/src/hooks/useSearchPage.tsfrontend/src/hooks/useSearchProjectsGraphQL.tsfrontend/src/server/fetchAlgoliaData.tsfrontend/src/server/queries/projectQueries.tsfrontend/src/server/queries/projectsHealthDashboardQueries.tsfrontend/src/types/category.tsfrontend/src/types/unifiedSearchBar.tsfrontend/src/utils/backendConfig.tsfrontend/src/utils/sortingOptions.ts
backend/apps/owasp/api/internal/queries/project_health_metrics.py
Outdated
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
10 issues found across 26 files
Confidence score: 3/5
- Several medium-severity, user-facing state sync issues in
frontend/src/app/projects/dashboard/metrics/page.tsxandfrontend/src/hooks/useSearchPage.tscan leave filters/pagination stale after navigation or show inconsistent empty states. - Sorting and ordering concerns in
frontend/src/hooks/useSearchProjectsGraphQL.tsandbackend/apps/api/rest/v0/project.pycould send invalid sort fields or produce incorrect maturity ordering. - Score reflects multiple concrete behavior inconsistencies that could affect search/metrics UX, though no clear hard failures are reported.
- Pay close attention to
frontend/src/app/projects/dashboard/metrics/page.tsx,frontend/src/hooks/useSearchPage.ts,frontend/src/hooks/useSearchProjectsGraphQL.ts,backend/apps/api/rest/v0/project.py- filter/URL sync and sorting correctness.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/app/projects/dashboard/metrics/page.tsx">
<violation number="1" location="frontend/src/app/projects/dashboard/metrics/page.tsx:142">
P2: Search and filter state are not kept in sync with URL param changes, causing stale GraphQL query variables after navigation/back-forward.</violation>
<violation number="2" location="frontend/src/app/projects/dashboard/metrics/page.tsx:240">
P2: Selecting a level preserves existing health filter, causing hidden combined filters while category UI shows only one selected value.</violation>
</file>
<file name="frontend/src/hooks/useSearchPage.ts">
<violation number="1" location="frontend/src/hooks/useSearchPage.ts:55">
P2: Search state is initialized from URL only once and is not re-synced when query params change, so back/forward navigation can leave UI/data stale vs URL.</violation>
<violation number="2" location="frontend/src/hooks/useSearchPage.ts:65">
P2: GraphQL query hook is called unconditionally, causing unnecessary GraphQL requests even when Algolia backend is selected.</violation>
<violation number="3" location="frontend/src/hooks/useSearchPage.ts:159">
P2: GraphQL pagination incorrectly forces at least one page for zero results, causing inconsistent and broken empty-state behavior.</violation>
</file>
<file name="frontend/src/server/queries/projectQueries.ts">
<violation number="1" location="frontend/src/server/queries/projectQueries.ts:207">
P2: `projectsTotal` is computed by a second unpaginated `searchProjects` call, causing avoidable extra query/load overhead and brittle total calculation semantics.</violation>
</file>
<file name="frontend/src/hooks/useSearchProjectsGraphQL.ts">
<violation number="1" location="frontend/src/hooks/useSearchProjectsGraphQL.ts:73">
P2: Sorting whitelist is bypassed by falling back to raw `sortBy`, allowing invalid ordering fields from URL/state to be sent to GraphQL.</violation>
</file>
<file name="frontend/src/components/UnifiedSearchBar.tsx">
<violation number="1" location="frontend/src/components/UnifiedSearchBar.tsx:66">
P2: Controlled Select uses an invalid fallback key (`''`) instead of empty selection when category is unset.</violation>
</file>
<file name="backend/apps/api/rest/v0/project.py">
<violation number="1" location="backend/apps/api/rest/v0/project.py:122">
P2: `ordering=level` sorts by text enum value instead of rank, causing incorrect project maturity ordering compared with the intended `level_raw` semantics.</violation>
</file>
<file name="frontend/src/utils/backendConfig.ts">
<violation number="1" location="frontend/src/utils/backendConfig.ts:24">
P2: `in`-based backend validation accepts prototype-chain keys, allowing invalid env values to be returned as `SearchBackend`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…-filter-implementation
Contributor
There was a problem hiding this comment.
1 issue found across 15 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/app/projects/dashboard/metrics/page.tsx">
<violation number="1" location="frontend/src/app/projects/dashboard/metrics/page.tsx:121">
P2: Initial URL parsing still applies both `health` and `level` filters, causing hidden active constraints while the UI shows only one category in the new single-select flow.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/app/projects/dashboard/metrics/page.tsx (1)
121-135:⚠️ Potential issue | 🟡 MinorURL bootstrap can still create hidden dual-category filtering.
If both
healthandlevelare present in the URL, both filters are merged, while the UI shows one selected category.💡 Suggested fix
- if (healthFilter && healthFilter in healthFiltersMapping) { + if (currentCategory && currentCategory in healthFiltersMapping) { currentFilters = { ...currentFilters, - ...healthFiltersMapping[healthFilter as keyof typeof healthFiltersMapping], + ...healthFiltersMapping[currentCategory as keyof typeof healthFiltersMapping], } - } - if (levelFilter && levelFilter in levelFiltersMapping) { + } else if (currentCategory && currentCategory in levelFiltersMapping) { currentFilters = { ...currentFilters, - ...levelFiltersMapping[levelFilter as keyof typeof levelFiltersMapping], + ...levelFiltersMapping[currentCategory as keyof typeof levelFiltersMapping], } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/projects/dashboard/metrics/page.tsx` around lines 121 - 135, The code merges both health and level mappings into currentFilters when both URL params exist, causing a hidden dual-category filter; change the logic to derive currentCategory (already set via currentCategory = levelFilter || healthFilter || '') and then apply only the mapping that corresponds to currentCategory (use an if/else or switch) instead of independently merging healthFiltersMapping and levelFiltersMapping; update the blocks around currentFilters, healthFilter, levelFilter, healthFiltersMapping and levelFiltersMapping so only the mapping for the chosen currentCategory (and not both) is spread into currentFilters.
🧹 Nitpick comments (1)
backend/apps/owasp/api/internal/queries/project_health_metrics.py (1)
60-62: Consider extracting shared query-filter logic to avoid drift.The same
cleaned_query+icontainsblock appears twice; a tiny helper would reduce maintenance risk.♻️ Suggested refactor
class ProjectHealthMetricsQuery: """Project health metrics queries.""" + + `@staticmethod` + def _apply_query_filter(queryset, query: str): + cleaned_query = query.strip() if query else "" + if cleaned_query: + return queryset.filter(project__name__icontains=cleaned_query) + return queryset @@ - cleaned_query = query.strip() if query else "" - if cleaned_query: - queryset = queryset.filter(project__name__icontains=cleaned_query) + queryset = self._apply_query_filter(queryset, query) @@ - cleaned_query = query.strip() if query else "" - if cleaned_query: - queryset = queryset.filter(project__name__icontains=cleaned_query) + queryset = self._apply_query_filter(queryset, query)Also applies to: 101-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/owasp/api/internal/queries/project_health_metrics.py` around lines 60 - 62, The query cleaning and icontains filter logic is duplicated; extract a small helper (e.g. _apply_project_name_filter(queryset, query) or apply_project_name_filter) that trims the incoming query (handle None -> "") and, if non-empty, applies queryset.filter(project__name__icontains=cleaned_query); replace both occurrences that currently set cleaned_query and call queryset.filter(...) with calls to this helper to centralize behavior and avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/owasp/api/internal/queries/project.py`:
- Around line 126-127: The current condition skips applying name__icontains for
short queries (checked via cleaned_query and MIN_SEARCH_QUERY_LENGTH), causing
1–2 char searches to bypass filtering; change the condition to always apply the
filter when cleaned_query is present (e.g., replace "if cleaned_query and
len(cleaned_query) >= MIN_SEARCH_QUERY_LENGTH:" with "if cleaned_query:") for
both occurrences that use base_queryset.filter(name__icontains=cleaned_query) so
short queries are properly filtered.
In `@frontend/src/hooks/useSearchProjectsGraphQL.ts`:
- Around line 85-90: The code currently rewrites short non-empty queries to ''
causing GraphQL to return broad results; in useSearchProjectsGraphQL
(searchParam, useQuery, GET_PROJECTS_LIST, searchQuery) stop masking valid short
inputs—use the trimmed searchQuery directly (e.g., const searchParam =
searchQuery.trim()) and pass that value (or undefined only when truly empty)
into the variables so the server receives the actual user input instead of an
empty string.
---
Duplicate comments:
In `@frontend/src/app/projects/dashboard/metrics/page.tsx`:
- Around line 121-135: The code merges both health and level mappings into
currentFilters when both URL params exist, causing a hidden dual-category
filter; change the logic to derive currentCategory (already set via
currentCategory = levelFilter || healthFilter || '') and then apply only the
mapping that corresponds to currentCategory (use an if/else or switch) instead
of independently merging healthFiltersMapping and levelFiltersMapping; update
the blocks around currentFilters, healthFilter, levelFilter,
healthFiltersMapping and levelFiltersMapping so only the mapping for the chosen
currentCategory (and not both) is spread into currentFilters.
---
Nitpick comments:
In `@backend/apps/owasp/api/internal/queries/project_health_metrics.py`:
- Around line 60-62: The query cleaning and icontains filter logic is
duplicated; extract a small helper (e.g. _apply_project_name_filter(queryset,
query) or apply_project_name_filter) that trims the incoming query (handle None
-> "") and, if non-empty, applies
queryset.filter(project__name__icontains=cleaned_query); replace both
occurrences that currently set cleaned_query and call queryset.filter(...) with
calls to this helper to centralize behavior and avoid drift.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (13)
.pre-commit-config.yamlbackend/apps/api/rest/v0/project.pybackend/apps/owasp/api/internal/filters/project.pybackend/apps/owasp/api/internal/queries/project.pybackend/apps/owasp/api/internal/queries/project_health_metrics.pyfrontend/src/app/projects/dashboard/metrics/page.tsxfrontend/src/components/Search.tsxfrontend/src/components/UnifiedSearchBar.tsxfrontend/src/hooks/useSearchPage.tsfrontend/src/hooks/useSearchProjectsGraphQL.tsfrontend/src/server/fetchAlgoliaData.tsfrontend/src/server/queries/projectQueries.tsfrontend/src/utils/backendConfig.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/utils/backendConfig.ts
- frontend/src/server/fetchAlgoliaData.ts
- frontend/src/components/Search.tsx
- frontend/src/components/UnifiedSearchBar.tsx
Contributor
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/owasp/api/internal/queries/project.py">
<violation number="1" location="backend/apps/owasp/api/internal/queries/project.py:126">
P2: Minimum query-length protection was removed, causing broad `icontains` filtering for 1–2 character inputs in both search and count paths, which can significantly increase DB load.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/src/hooks/useSearchProjectsGraphQL.ts (1)
9-12:options.pageSizeis declared but never applied.This creates a confusing API surface: callers can pass
options.pageSize, but pagination always uses the positionalpageSizeargument.Proposed fix
export function useSearchProjectsGraphQL( searchQuery = '', category = '', sortBy = '', order = '', currentPage = 1, pageSize = 25, options?: UseSearchProjectsGraphQLOptions ): UseSearchProjectsGraphQLReturn { + const effectivePageSize = options?.pageSize ?? pageSize ... - const offset = (currentPage - 1) * pageSize + const offset = (currentPage - 1) * effectivePageSize ... pagination: { offset, - limit: pageSize, + limit: effectivePageSize, },Also applies to: 40-41, 83-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useSearchProjectsGraphQL.ts` around lines 9 - 12, The options interface UseSearchProjectsGraphQLOptions exposes pageSize but the hook never uses it; update the useSearchProjectsGraphQL implementation to read options.pageSize and apply it to all pagination logic (replace hardcoded/positional pageSize usage with options.pageSize ?? pageSizeParam), including where page queries and cursor/offset calculations occur (searchProjectsQuery handler, pagination helpers, and any loadMore/hasMore logic). Ensure the default behavior remains the same by falling back to the existing pageSize parameter when options.pageSize is undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/owasp/api/internal/queries/project.py`:
- Around line 91-94: The pagination.limit checks currently treat
strawberry.UNSET like a valid value and perform numeric comparisons, causing
TypeError; update the guard around pagination.limit in the functions that use it
(the blocks referencing pagination.limit and MAX_PROJECTS_LIMIT) to ensure the
value is a real number before comparing — e.g., only proceed if pagination.limit
is not None and pagination.limit is not strawberry.UNSET (or alternatively
verify type is int) before doing <= 0 or min(..., MAX_PROJECTS_LIMIT); apply the
same change to both places that currently read pagination.limit so UNSET is
skipped safely.
In `@frontend/src/app/projects/dashboard/metrics/page.tsx`:
- Around line 133-136: The component currently only re-syncs ordering from
searchParams, causing searchQuery and filters to drift; update the effect that
derives ordering (the effect around currentOrdering/currentOrdering
dependencies) to also call setSearchQuery(searchQueryParam) and
setFilters(currentFilters) whenever the relevant searchParams change (or when
currentFilters/currentOrdering/searchQueryParam values update). Ensure you
reference and update the state setters setSearchQuery, setFilters, and
setOrdering so the UI state (filters, ordering, searchQuery) is consistently
derived from the URL parameters rather than drifting.
In `@frontend/src/hooks/useSearchProjectsGraphQL.ts`:
- Line 3: Replace the removed ApolloError type with ErrorLike from Apollo Client
v4: update the import (bring ErrorLike in from '@apollo/client') and change any
type references to ApolloError in this file—especially the return interface that
currently uses ApolloError—so it uses ErrorLike instead; also verify imports for
useQuery are from '@apollo/client' if necessary and update any other occurrences
of ApolloError in functions like useQuery usage.
---
Nitpick comments:
In `@frontend/src/hooks/useSearchProjectsGraphQL.ts`:
- Around line 9-12: The options interface UseSearchProjectsGraphQLOptions
exposes pageSize but the hook never uses it; update the useSearchProjectsGraphQL
implementation to read options.pageSize and apply it to all pagination logic
(replace hardcoded/positional pageSize usage with options.pageSize ??
pageSizeParam), including where page queries and cursor/offset calculations
occur (searchProjectsQuery handler, pagination helpers, and any loadMore/hasMore
logic). Ensure the default behavior remains the same by falling back to the
existing pageSize parameter when options.pageSize is undefined.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/owasp/api/internal/queries/project.pyfrontend/src/app/projects/dashboard/metrics/page.tsxfrontend/src/hooks/useSearchProjectsGraphQL.tsfrontend/src/utils/backendConfig.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/utils/backendConfig.ts
Contributor
Author
|
Hi @arkid15r pr is ready for review let me know if any changes are required or should i start writing test for the changes? |
Contributor
There was a problem hiding this comment.
7 issues found across 13 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx:90">
P2: Loading/error/empty-state tests were weakened to only check the mocked search bar or header, so they no longer validate the actual loading spinner or empty/error UI and can pass even if those states regress.</violation>
<violation number="2" location="frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx:254">
P3: The “no-op” test ends with `expect(true).toBe(true)`, so it never verifies that URL updates were skipped. This leaves the edge case untested and could hide regressions in order handling.</violation>
</file>
<file name="frontend/src/wrappers/testUtil.tsx">
<violation number="1" location="frontend/src/wrappers/testUtil.tsx:11">
P2: ApolloProvider is initialized asynchronously at module load, so render() can run before it is set and return content without Apollo context, leading to flaky tests when Apollo hooks are used.</violation>
</file>
<file name="frontend/__tests__/unit/components/UnifiedSearchBar.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/UnifiedSearchBar.test.tsx:180">
P3: Category-change test does not assert that onCategoryChange is called, so it can pass even if the callback wiring breaks.</violation>
</file>
<file name="frontend/src/components/SearchPageLayout.tsx">
<violation number="1" location="frontend/src/components/SearchPageLayout.tsx:61">
P2: Removing `w-full` from the loaded-content wrapper inside a `flex-col items-center` container allows the wrapper to shrink to content width, which can break right alignment for the sort row and narrow full-width child layouts.</violation>
</file>
<file name="backend/apps/owasp/api/internal/queries/project.py">
<violation number="1" location="backend/apps/owasp/api/internal/queries/project.py:124">
P2: search_projects now rejects short/overlong queries, but search_projects_count still counts them, leading to inconsistent result vs count behavior for the same query.</violation>
<violation number="2" location="backend/apps/owasp/api/internal/queries/project.py:148">
P2: search_projects now materializes and slices the queryset to 3 items before returning, which prevents strawberry-django from applying its automatic filtering/ordering/pagination and hard-caps results to 3 regardless of pagination inputs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/api/rest/v0/project.py (1)
89-100:⚠️ Potential issue | 🟠 Major
ProjectFilter.typeis declared twice.The second
typefield overrides the first one, so the first declaration is dead code and the filter contract is ambiguous. Keep only onetypedeclaration and align it with the intended query semantics.Suggested cleanup
class ProjectFilter(FilterSchema): @@ - type: ProjectType | None = Field( - None, - description="Type (category) of the project", - ) @@ type: list[ProjectType] | None = Field( None, description="Type of the project", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/api/rest/v0/project.py` around lines 89 - 100, ProjectFilter currently declares the attribute "type" twice (once as ProjectType | None and once as list[ProjectType] | None); remove the duplicate and keep the correct shape for the filter (choose either a single ProjectType or a list of ProjectType). Edit the ProjectFilter class to have a single "type" Field, remove the other declaration, and update the Field's type annotation and description to match the intended query semantics (e.g., use list[ProjectType] | None if callers should be able to filter by multiple types, or ProjectType | None for a single value); ensure only the remaining "type" symbol is present and its description is accurate.
🧹 Nitpick comments (1)
backend/tests/apps/owasp/api/internal/filters/project_test.py (1)
45-46: Remove the duplicated assertion in this loop test.
assert result == Q(type=project_type)is repeated twice; the second assertion adds no extra signal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/api/internal/filters/project_test.py` around lines 45 - 46, The test contains a duplicated assertion checking equality between result and Q(type=project_type); remove the redundant second assertion so the loop only asserts once. Locate the duplicate line referencing result and Q(type=project_type) in the test (variables: result, project_type, and Q) and delete the repeated assertion to avoid pointless duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/owasp/api/internal/queries/project.py`:
- Around line 98-103: The resolver search_projects currently materializes and
hard-caps results with list(base_queryset[:SEARCH_PROJECTS_LIMIT]) which
prevents Strawberry-Django from applying filters/ordering/pagination; change
search_projects to return the QuerySet itself (base_queryset or
base_queryset.all()) instead of a list slice so the framework can apply
filters/ordering/pagination, and if you must enforce a maximum cap use a
QuerySet-level limit only after framework pagination (or implement cap via
pagination settings) and avoid using SEARCH_PROJECTS_LIMIT to materialize
results inside search_projects.
In `@frontend/__tests__/unit/components/UnifiedSearchBar.test.tsx`:
- Around line 180-188: The test "calls onCategoryChange when category is
selected" currently only fires a change on the select in UnifiedSearchBar;
update it to assert that the mock callback mockOnCategoryChange was invoked with
the selected value (e.g., 'tech') using the appropriate Jest matcher
(toHaveBeenCalledWith or toHaveBeenCalledTimes + toHaveBeenCalledWith). Apply
the same change to the other similar test (the one around lines 428-435) so both
verify the callback behavior on change.
In `@frontend/src/app/projects/dashboard/metrics/page.tsx`:
- Around line 137-147: handleSearchChange currently treats whitespace-only input
as an active query; trim the incoming query first (e.g., const trimmed =
query.trim()), then use trimmed for setSearchQuery and for deciding
pagination/URL updates so that empty or whitespace-only strings clear the search
state and remove the 'search' param (use newParams.set only when trimmed is
non-empty, otherwise newParams.delete). Apply the same trimming logic to the
other search handler that updates searchParams (the similar handler around line
156) so both UI state and URL are normalized to backend-trimmed behavior.
In `@frontend/src/components/UnifiedSearchBar.tsx`:
- Around line 63-65: The Select is controlled with selectedKeys but uses a DOM
onChange; replace the onChange handler in UnifiedSearchBar with HeroUI's
onSelectionChange to receive a Set<React.Key>, convert that set to the single
key/string you expect, and call onCategoryChange with that value (e.g.,
Array.from(selectedKeys)[0] or selectedKeys.values().next().value). Update the
handler tied to the Select component (where selectedKeys is used) to use
onSelectionChange and ensure the value passed into onCategoryChange matches the
expected string type.
In `@frontend/src/wrappers/testUtil.tsx`:
- Around line 11-20: The silent catch in the async IIFE that imports
'@apollo/client/react' masks real import failures for ApolloProvider; either
remove the try/catch so the dynamic import error propagates, or change the catch
to accept an error (e.g., catch (err)) and explicitly rethrow or log and throw a
new error so test initialization fails loudly; update the IIFE that assigns
ApolloProvider to use the chosen option so import issues for ApolloProvider are
visible during test setup.
---
Outside diff comments:
In `@backend/apps/api/rest/v0/project.py`:
- Around line 89-100: ProjectFilter currently declares the attribute "type"
twice (once as ProjectType | None and once as list[ProjectType] | None); remove
the duplicate and keep the correct shape for the filter (choose either a single
ProjectType or a list of ProjectType). Edit the ProjectFilter class to have a
single "type" Field, remove the other declaration, and update the Field's type
annotation and description to match the intended query semantics (e.g., use
list[ProjectType] | None if callers should be able to filter by multiple types,
or ProjectType | None for a single value); ensure only the remaining "type"
symbol is present and its description is accurate.
---
Nitpick comments:
In `@backend/tests/apps/owasp/api/internal/filters/project_test.py`:
- Around line 45-46: The test contains a duplicated assertion checking equality
between result and Q(type=project_type); remove the redundant second assertion
so the loop only asserts once. Locate the duplicate line referencing result and
Q(type=project_type) in the test (variables: result, project_type, and Q) and
delete the repeated assertion to avoid pointless duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 157df032-0ade-4970-968c-131f3b8b20a9
📒 Files selected for processing (14)
backend/apps/api/rest/v0/project.pybackend/apps/owasp/api/internal/queries/project.pybackend/tests/apps/owasp/api/internal/filters/project_test.pybackend/tests/apps/owasp/api/internal/queries/project_test.pybackend/tests/apps/owasp/index/search/project_test.pyfrontend/__tests__/unit/components/UnifiedSearchBar.test.tsxfrontend/__tests__/unit/pages/Contribute.test.tsxfrontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsxfrontend/src/app/projects/dashboard/metrics/page.tsxfrontend/src/components/SearchPageLayout.tsxfrontend/src/components/UnifiedSearchBar.tsxfrontend/src/hooks/useSearchProjectsGraphQL.tsfrontend/src/utils/helpers/mockApolloClient.tsfrontend/src/wrappers/testUtil.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/SearchPageLayout.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/__tests__/unit/components/UnifiedSearchBar.test.tsx (1)
277-286: Consider verifying selected category value.This test verifies the category select exists but doesn't assert the selected value is
'science'. While the component'sselectedKeysprop is tested implicitly through the mock, explicitly checking the select's value would strengthen the test.Optional enhancement
const categorySelect = screen.getByLabelText('Filter by category') expect(categorySelect).toBeInTheDocument() + expect(categorySelect).toHaveValue('science')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/UnifiedSearchBar.test.tsx` around lines 277 - 286, The test currently only asserts the category select exists but not that it reflects the passed-in category; update the 'renders with correct selected category' test for UnifiedSearchBar to also assert the select's value is 'science' by retrieving the element (categorySelect via screen.getByLabelText('Filter by category')) and adding an expectation like checking categorySelect.value or using expect(categorySelect).toHaveValue('science') so the component's selectedKeys behavior is explicitly verified.frontend/src/app/projects/dashboard/metrics/page.tsx (1)
307-318: Redundant empty state handling.The
emptyprop is passed to UnifiedSearchBar (line 307), but the component also renders its own empty state message (lines 314-318). Based on the SearchPageLayout implementation (context snippet 2),emptyis used by the layout when no children content exists. However, since children always renders either the loading spinner or the grid (with its own empty message), theemptyprop may never be displayed.Consider removing the inline empty state handling and relying on the
emptyprop for consistency, or remove theemptyprop if the inline handling is preferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/projects/dashboard/metrics/page.tsx` around lines 307 - 318, The component is rendering its own empty-message inside the children while also passing an empty prop to UnifiedSearchBar; remove the redundant inline empty state so the layout's empty prop is used consistently. Concretely, in page.tsx inside the UnifiedSearchBar children (the block that conditionally renders LoadingSpinner or the grid), stop rendering the fallback div when metrics.length === 0 — either render nothing (null) or only render the grid when metrics.length > 0 and still render LoadingSpinner when loading is true; keep the empty prop as-is. Update references to the metrics array and searchQuery only for conditional rendering (do not change the empty prop or UnifiedSearchBar invocation), and ensure LoadingSpinner remains displayed while loading.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/apps/owasp/api/internal/queries/project_test.py`:
- Around line 498-513: Update the test to reflect the production behavior where
overly-long queries (>100 chars) return 0 instead of truncating: in
test_search_projects_count_with_long_query_bounded, change the expected result
assertion from 3 to 0 and replace the
mock_queryset.filter.assert_called_with(...) check with an assertion that
Project.objects.filter was not called (e.g., mock_filter.assert_not_called()),
referencing the tested methods search_projects_count and search_projects to
locate the logic to align with.
---
Nitpick comments:
In `@frontend/__tests__/unit/components/UnifiedSearchBar.test.tsx`:
- Around line 277-286: The test currently only asserts the category select
exists but not that it reflects the passed-in category; update the 'renders with
correct selected category' test for UnifiedSearchBar to also assert the select's
value is 'science' by retrieving the element (categorySelect via
screen.getByLabelText('Filter by category')) and adding an expectation like
checking categorySelect.value or using
expect(categorySelect).toHaveValue('science') so the component's selectedKeys
behavior is explicitly verified.
In `@frontend/src/app/projects/dashboard/metrics/page.tsx`:
- Around line 307-318: The component is rendering its own empty-message inside
the children while also passing an empty prop to UnifiedSearchBar; remove the
redundant inline empty state so the layout's empty prop is used consistently.
Concretely, in page.tsx inside the UnifiedSearchBar children (the block that
conditionally renders LoadingSpinner or the grid), stop rendering the fallback
div when metrics.length === 0 — either render nothing (null) or only render the
grid when metrics.length > 0 and still render LoadingSpinner when loading is
true; keep the empty prop as-is. Update references to the metrics array and
searchQuery only for conditional rendering (do not change the empty prop or
UnifiedSearchBar invocation), and ensure LoadingSpinner remains displayed while
loading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff9c02ce-8cf4-4943-992c-2372ebfbd01a
📒 Files selected for processing (9)
backend/apps/api/rest/v0/project.pybackend/apps/owasp/api/internal/queries/project.pybackend/tests/apps/owasp/api/internal/filters/project_test.pybackend/tests/apps/owasp/api/internal/queries/project_test.pyfrontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsxfrontend/__tests__/unit/components/UnifiedSearchBar.test.tsxfrontend/src/app/projects/dashboard/metrics/page.tsxfrontend/src/components/UnifiedSearchBar.tsxfrontend/src/wrappers/testUtil.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/tests/apps/owasp/api/internal/filters/project_test.py
- backend/apps/api/rest/v0/project.py
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/tests/apps/owasp/api/internal/queries/project_test.py">
<violation number="1" location="backend/tests/apps/owasp/api/internal/queries/project_test.py:134">
P2: Short-query tests are too weak: `assert result != []` does not verify that query filtering behavior actually occurs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
Hi @arkid15r the pr is ready for review Thanks! |
…-filter-implementation
Contributor
Author
|
I am working on it I need a little more time I will update it by tomorrow |
anurag2787
force-pushed
the
feature/search-filter-implementation
branch
from
8e1db81 to
b96c1a0
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
3 issues found across 19 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/owasp/api/internal/filters/project.py">
<violation number="1" location="backend/apps/owasp/api/internal/filters/project.py:29">
P2: Filtering by an active category can still include projects from inactive descendant categories because descendants are not filtered by is_active, contradicting the model’s stated filtering behavior.</violation>
</file>
<file name="backend/apps/owasp/migrations/0073_projectcategory_project_categories_and_more.py">
<violation number="1" location="backend/apps/owasp/migrations/0073_projectcategory_project_categories_and_more.py:75">
P2: Redundant index on `name`: field is already indexed via `unique=True`, so this adds unnecessary duplicate index maintenance.</violation>
<violation number="2" location="backend/apps/owasp/migrations/0073_projectcategory_project_categories_and_more.py:79">
P2: Redundant index on `parent`: ForeignKey fields are auto-indexed by Django, so this extra index is duplicate overhead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
backend/apps/owasp/migrations/0073_projectcategory_project_categories_and_more.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/migrations/0073_projectcategory_project_categories_and_more.py
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (11)
frontend/src/hooks/useSearchProjectsGraphQL.ts (1)
82-94: Minor: Simplify redundant filter check.The
filtersvariable fromuseMemo(line 51) already returnsundefinedwhen empty. The additionalObject.keys(filters || {}).length > 0check at line 85 is redundant.♻️ Proposed simplification
const { data, loading, error } = useQuery(GetProjectsListDocument, { variables: { query: searchParam, - filters: Object.keys(filters || {}).length > 0 ? filters : undefined, + filters, ordering: ordering && ordering.length > 0 ? ordering : undefined, pagination: { offset, limit: pageSize, }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useSearchProjectsGraphQL.ts` around lines 82 - 94, The filters existence check in the useQuery call is redundant because the useMemo that computes filters already returns undefined when empty; update the variables passed to useQuery (in useSearchProjectsGraphQL where GetProjectsListDocument is used) to pass filters directly (i.e., use filters instead of Object.keys(filters || {}).length > 0 ? filters : undefined) and remove the unnecessary Object.keys(...) conditional so the code uses the memoized filters value as-is.frontend/src/components/NestedCategorySelect.tsx (1)
23-50: Keyboard navigation within menu items is limited.
CategoryItemhastabIndex={-1}which prevents keyboard users from tabbing between menu items. While clicking and Enter/Space work on focused items, users cannot navigate to items using Tab or arrow keys.Consider implementing arrow key navigation for better accessibility:
♻️ Basic arrow key navigation approach
const CategoryItem: React.FC<CategoryItemProps> = ({ slug, name, isSelected, hasChildren, onSelect, + onKeyNavigation, }) => ( <div role="menuitem" - tabIndex={-1} + tabIndex={0} onClick={() => onSelect(slug)} onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault() onSelect(slug) + } else if (e.key === 'ArrowDown' || e.key === 'ArrowUp') { + e.preventDefault() + onKeyNavigation?.(e.key) } }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/NestedCategorySelect.tsx` around lines 23 - 50, CategoryItem is inaccessible to Tab navigation because it sets tabIndex={-1}; make each CategoryItem focusable and support arrow-key navigation by switching to a roving focus/tabindex pattern: give interactive items tabIndex={0} when they should be focusable (or manage a focusedIndex in the parent component and set tabIndex={ focusedIndex === index ? 0 : -1 }), add arrow key handling in CategoryItem's onKeyDown to call onSelect or a provided onMoveFocus callback for ArrowUp/ArrowDown/ArrowLeft/ArrowRight, and expose/select via Enter/Space as already implemented; also consider adding aria-selected and ensuring the parent manages focus changes (use refs or parent-level focus management) so keyboard users can tab into the menu and then use arrow keys to move between CategoryItem elements.frontend/src/app/projects/page.tsx (1)
73-94: Remove redundant client-side filter.The
.filter((project) => project.isActive)at line 93 is redundant. Both backend paths guarantee only active projects are returned:
- Algolia backend:
fetchAlgoliaDataautomatically addsidx_is_active:truefacet filter for projects- GraphQL backend:
search_projectsresolver filtersis_active=Truebefore returning results♻️ Proposed fix
> - {projects?.filter((project) => project.isActive).map(renderProjectCard)} + {projects?.map(renderProjectCard)} </UnifiedSearchBar>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/projects/page.tsx` around lines 73 - 94, Remove the redundant client-side filter on projects by deleting the .filter((project) => project.isActive) call inside the UnifiedSearchBar children; the backend (fetchAlgoliaData and search_projects resolver) already ensures only active projects are returned, so pass projects?.map(renderProjectCard) (or just projects?.map using renderProjectCard) as the children to UnifiedSearchBar to avoid double-filtering and keep rendering logic in renderProjectCard unchanged.backend/apps/owasp/management/commands/populate_sample_categories.py (1)
117-131: Make reruns converge instead of silently skipping drifted rows.
setup-categoriesis now a repeatable bootstrap path, but this branch only no-ops on an existing slug. If a sample category already has the wrongparent,description, oris_activevalue, rerunning the command keeps the taxonomy stale and downstream assignment works against the wrong tree.update_or_create(..., defaults=...)or an explicit reconcile step would make this deterministic.Possible refactor
- existing = ProjectCategory.objects.filter(slug=slug).first() - if existing: - self.stdout.write( - self.style.WARNING(f" Category '{name}' already exists - skipping") - ) - category = existing - else: - category = ProjectCategory.objects.create( - name=name, - slug=slug, - description=description, - parent=parent_category, - is_active=True, - ) + category, created = ProjectCategory.objects.update_or_create( + slug=slug, + defaults={ + "name": name, + "description": description, + "parent": parent_category, + "is_active": True, + }, + ) + + if not created: + self.stdout.write( + self.style.WARNING( + f" Category '{name}' already exists - synchronized" + ) + ) + else: + depth = " " + if parent_category: + depth = " " if parent_category.parent else " " + self.stdout.write( + self.style.SUCCESS(f"{depth}✓ Created: {category.full_path}") + ) - depth = " " - if parent_category: - depth = " " if parent_category.parent else " " - self.stdout.write( - self.style.SUCCESS(f"{depth}✓ Created: {category.full_path}") - )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/owasp/management/commands/populate_sample_categories.py` around lines 117 - 131, The current logic skips creating a ProjectCategory when a slug exists, which leaves existing rows with drifted fields unchanged; change this to reconcile or use update_or_create so reruns converge: replace the fetch-and-skip branch around ProjectCategory.objects.filter(slug=slug).first() with ProjectCategory.objects.update_or_create(slug=slug, defaults={ 'name': name, 'description': description, 'parent': parent_category, 'is_active': True }) (or perform an explicit update on the returned instance) so name, description, parent and is_active are kept in sync on every run.backend/apps/owasp/management/commands/assign_sample_categories.py (1)
40-45: Consider performance implications oforder_by("?").Using
order_by("?")for random ordering performs a full table scan on PostgreSQL, which can be expensive on large datasets. For a test/development utility with a--limitparameter, this is acceptable, but be aware of this if the projects table grows significantly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/owasp/management/commands/assign_sample_categories.py` around lines 40 - 45, The current use of Project.active_projects.all().order_by("?") (assigned to the projects variable in assign_sample_categories.py) causes a full-table random sort which is slow on large tables; replace it by sampling IDs in Python: fetch ids via Project.active_projects.values_list('id', flat=True'), handle the case where count <= limit, otherwise use random.sample to pick limit ids, then re-query Project.objects.filter(id__in=selected_ids) (and optionally preserve order) so you avoid order_by("?") and full table scans while keeping the rest of the command logic unchanged.backend/apps/owasp/api/internal/filters/project.py (1)
25-32: Consider optimizing database queries in the category filter loop.Each iteration performs at least two database queries: one to fetch the category and one for
get_descendants(). For N category slugs, this results in approximately 2N queries.Consider fetching all matching categories in a single query and then gathering descendants, though the hierarchical nature adds complexity. This is acceptable for small category lists but may become a performance concern with many categories.
♻️ Potential optimization approach
def categories(self, value: list[str], prefix: str): """Filter by project categories.""" if not value: return Q() + # Fetch all matching categories in one query + categories = ProjectCategory.objects.filter(slug__in=value, is_active=True) + category_q = Q() - for category_slug in value: - try: - category = ProjectCategory.objects.get(slug=category_slug, is_active=True) - category_ids = [category.id] - category_ids.extend(category.get_descendants().values_list("id", flat=True)) - category_q |= Q(categories__id__in=category_ids) - except ProjectCategory.DoesNotExist: - continue + for category in categories: + category_ids = [category.id] + category_ids.extend(category.get_descendants().values_list("id", flat=True)) + category_q |= Q(categories__id__in=category_ids) return category_q🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/owasp/api/internal/filters/project.py` around lines 25 - 32, The loop is issuing multiple DB hits per slug; instead, fetch all matching categories in one query via ProjectCategory.objects.filter(slug__in=value, is_active=True) (replace the per-iteration ProjectCategory.objects.get for category_slug), then build the set of category IDs to filter on by aggregating each category's id plus descendants; if your tree library exposes bulk descendant filtering (tree_id/lft/rght or a bulk API) use that to fetch all descendants in one query, otherwise call get_descendants only on the small returned set; finally OR into category_q once using the combined id list (references: ProjectCategory, get_descendants, category_slug, category_q, value).backend/tests/apps/owasp/api/internal/queries/project_test.py (1)
121-146: Weak assertions for single/two character query tests.The assertions
assert result != []on lines 134 and 146 only verify the result is not an empty list, but don't confirm the expected queryset was returned. This could pass with any truthy value.♻️ Suggested improvement
def test_search_projects_single_char_query_returns_results(self): """Test search_projects returns results for 1-character queries. MIN_SEARCH_QUERY_LENGTH is now 1. """ with patch("apps.owasp.models.project.Project.objects.filter") as mock_filter: mock_queryset = MagicMock() mock_queryset.filter.return_value = mock_queryset mock_filter.return_value = mock_queryset query = ProjectQuery() result = query.__class__.__dict__["search_projects"](query, query="a") - assert result != [] + # Verify queryset chaining occurred and result is the ordered queryset + mock_queryset.filter.assert_called_with(name__icontains="a") + assert result == mock_queryset.filter.return_value.order_by.return_value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/api/internal/queries/project_test.py` around lines 121 - 146, The tests test_search_projects_single_char_query_returns_results and test_search_projects_two_char_query_returns_results use weak assertions (assert result != []) which can be satisfied by any truthy value; update them to assert the actual mocked queryset is returned and that the filter was called with expected parameters: call ProjectQuery.search_projects (via ProjectQuery.__class__.__dict__["search_projects"]) as before, then assert result is the MagicMock mock_queryset (e.g., result is mock_queryset) and/or assert mock_filter.assert_called_with(...) was invoked with the expected query pattern/arguments so the test verifies the correct queryset and filter invocation for ProjectQuery.search_projects.backend/apps/owasp/api/internal/queries/project.py (1)
123-126: Dead code: condition can never be true.With
MIN_SEARCH_QUERY_LENGTH = 1, the conditioncleaned_query and len(cleaned_query) < MIN_SEARCH_QUERY_LENGTHchecks if a non-empty string has length < 1, which is impossible. A non-empty string always has length >= 1.♻️ Proposed fix - remove dead code
cleaned_query = query.strip() - if cleaned_query and len(cleaned_query) < MIN_SEARCH_QUERY_LENGTH: - return [] if len(cleaned_query) > MAX_SEARCH_QUERY_LENGTH: return []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/owasp/api/internal/queries/project.py` around lines 123 - 126, The if-check "if cleaned_query and len(cleaned_query) < MIN_SEARCH_QUERY_LENGTH" is dead code given MIN_SEARCH_QUERY_LENGTH = 1 (a non-empty string cannot have length < 1); remove that conditional block and its return so only the upper-bound check using MAX_SEARCH_QUERY_LENGTH remains, or if the intent was to enforce a minimum length >1, update MIN_SEARCH_QUERY_LENGTH accordingly and adjust callers—locate the condition by its symbols cleaned_query and MIN_SEARCH_QUERY_LENGTH in the search/query logic and delete the unreachable branch.backend/apps/owasp/api/internal/nodes/category.py (2)
33-39: Useselect_relatedfor FK field, notprefetch_related.The
parentfield is a ForeignKey (single object), not a reverse relation. For FKs,select_relatedis the appropriate hint to perform a JOIN, whileprefetch_relatedis designed for reverse FKs and M2M relations.♻️ Proposed fix
`@strawberry_django.field`( - prefetch_related=["parent"], + select_related=["parent"], description="Parent category if this is a subcategory", ) def parent(self, root: ProjectCategory) -> "ProjectCategoryNode | None":Based on learnings: "In Strawberry Django usage with DjangoOptimizerExtension, when using a path like author__user_badges__badge in select_related hints on a strawberry_django.field decorator, it may yield better performance..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/owasp/api/internal/nodes/category.py` around lines 33 - 39, The decorator on the parent resolver uses prefetch_related for a ForeignKey; change the strawberry_django.field hint to use select_related instead (replace prefetch_related=["parent"] with select_related=["parent"]) in the parent method of ProjectCategoryNode (the def parent(self, root: ProjectCategory) resolver) so the FK is joined rather than prefetched.
41-47: Consider usingPrefetchobject with queryset filter for children.The current implementation prefetches all children, then filters in Python. Using a
Prefetchobject with a filtered queryset would push theis_active=Truefilter to the database level.♻️ Optional improvement
+from django.db.models import Prefetch + `@strawberry_django.field`( - prefetch_related=["children"], + prefetch_related=[ + Prefetch( + "children", + queryset=ProjectCategory.objects.filter(is_active=True), + ) + ], description="Direct child categories", ) def children(self, root: ProjectCategory) -> list["ProjectCategoryNode"]: """Resolve direct child categories.""" - return list(root.children.filter(is_active=True)) + return list(root.children.all())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/owasp/api/internal/nodes/category.py` around lines 41 - 47, Summary: Prefetching currently pulls all child categories then filters in Python; use a Django Prefetch with a filtered queryset so the is_active=True filter is applied in the DB. Update the strawberry_django.field decorator on the children resolver to use django.db.models.Prefetch for the "children" relation with a queryset that filters is_active=True (e.g., Prefetch("children", queryset=ProjectCategory.objects.filter(is_active=True))), import Prefetch at the top, and then in def children(self, root: ProjectCategory) return the prefetched related set (e.g., list(root.children.all())) so the filter is served from the DB rather than post-filtered in Python.backend/apps/api/rest/v0/project.py (1)
148-161: Category filtering has O(depth × slugs) query complexity.Each category slug triggers a database query for
get()plusget_descendants(), which performs recursive Python traversal with per-level queries. With multiple categories selected, this can result in many database round-trips.This logic also duplicates the filter implementation in
backend/apps/owasp/api/internal/filters/project.py:14-33.♻️ Consider extracting shared category filtering logic
Extract the category filtering logic into a shared utility to avoid duplication and ensure consistent behavior between REST and GraphQL endpoints:
# In a shared utility module def get_category_filter_q(category_slugs: list[str]) -> Q: """Build Q object for category filtering including descendants.""" category_q = Q() for slug in category_slugs: try: category = ProjectCategory.objects.get(slug=slug, is_active=True) category_ids = [category.id] category_ids.extend(category.get_descendants().values_list("id", flat=True)) category_q |= Q(categories__id__in=category_ids) except ProjectCategory.DoesNotExist: continue return category_q🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/api/rest/v0/project.py` around lines 148 - 161, The current per-slug loop (filters.categories) issues a DB get() and descendant traversal per slug causing O(depth×slugs) queries; extract this into a shared utility (e.g., get_category_filter_q(category_slugs: list[str])) that first fetches all ProjectCategory rows for slug__in=category_slugs and is_active=True in one query, collects each category's descendant IDs (using category.get_descendants().values_list("id", flat=True) or the model's bulk descendant query), builds a single combined Q (category_q) of Q(categories__id__in=...) for the aggregated ids, and returns it; then replace the inline loop in project.py (and the duplicate block in backend/apps/owasp/api/internal/filters/project.py) to call get_category_filter_q(filters.categories) and apply queryset = queryset.filter(category_q).distinct().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/api/rest/v0/category.py`:
- Around line 39-43: The list_categories view returns ProjectCategory queryset
without eager-loading parents, causing N+1 queries when serializing the
full_path and level; update the queryset in list_categories to use
select_related("parent", "parent__parent") on ProjectCategory so parent and
grandparent FKs are fetched in the same query and avoid extra queries when
accessing full_path/level during serialization.
In `@backend/apps/owasp/api/internal/ordering/category.py`:
- Around line 13-15: The ordering type declares ordering fields named created_at
and updated_at which don't exist on the ProjectCategory model (it uses
TimestampedModel fields nest_created_at and nest_updated_at), causing runtime
errors; update the ordering type to use nest_created_at and nest_updated_at
(replace the strawberry.auto fields named created_at and updated_at with
nest_created_at and nest_updated_at) so the ordering keys match the
ProjectCategory model and the GraphQL node/admin definitions.
In `@backend/apps/owasp/api/internal/queries/category.py`:
- Around line 32-44: The `@strawberry_django.field` decorator on the query
returning ProjectCategory queryset is missing pagination=True so automatic
pagination isn't applied; update the decorator to include pagination=True
(keeping the field signature intact), and stop mutating the input pagination
object — instead compute safe_offset = max(0, min(pagination.offset,
MAX_OFFSET)) and safe_limit = None or min(pagination.limit,
MAX_CATEGORIES_LIMIT) as appropriate and apply them to the queryset via slicing
(queryset[safe_offset : safe_limit and safe_offset + safe_limit]) before
returning; reference symbols: `@strawberry_django.field`, pagination,
ProjectCategory, MAX_OFFSET, MAX_CATEGORIES_LIMIT.
In `@backend/apps/owasp/management/commands/assign_sample_categories.py`:
- Around line 51-52: The NOSONAR suppression comment before the random.sample
call is using incorrect syntax; update the comment on the random_categories line
(the random.sample usage) to include the specific Sonar rule key reported by
SonarCloud, e.g. change the current "# NOSONAR: Using random for test data
generation, not security-sensitive" to the proper form "# NOSONAR
python:<RULE_KEY> — Using random for test data generation, not
security-sensitive" (replace <RULE_KEY> with the exact rule id Sonar reported),
so the suppression properly targets the rule for the random usage.
In
`@backend/apps/owasp/migrations/0073_projectcategory_project_categories_and_more.py`:
- Around line 56-60: The migration's options specify ordering = ["parent",
"name"] which conflicts with the model's Meta.ordering = ["name"] in the
Category model; update the migration
(0073_projectcategory_project_categories_and_more.py) to use ordering = ["name"]
to match the model's Meta.ordering (or alternatively update the model if you
intended parent to be included) so makemigrations stops detecting a mismatch —
edit the "ordering" entry in the migration's options to match the Category
model's Meta.ordering.
In `@docker-compose/local/compose.yaml`:
- Line 70: Revert the feature-branch volume rename by replacing the volume
reference "db-data-4168" with the standard "db-data" (and remove any other
occurrences of the "-4168" suffix), so the compose volume names remain
consistent with the main branch; locate the entry where "db-data-4168" is used
in the docker-compose service volume list and update it back to "db-data" (if
branch isolation was intended instead, apply the same "-4168" suffix
consistently to all volumes such as "backend-venv", "cache-data", "docs-venv",
"frontend-next", and "frontend-node-modules").
In `@frontend/src/app/projects/dashboard/metrics/page.tsx`:
- Line 131: Remove the unused GraphQL fetch by deleting the call to
useProjectCategories() (remove the const { categories } = useProjectCategories()
line) and stop passing the resulting categories into UnifiedSearchBar so it
cannot override the intended filters; instead ensure UnifiedSearchBar receives
the computed categoryOptions (constructed from healthFiltersMapping and
levelFiltersMapping) as its categoryOptions prop and do not provide a categories
prop so UnifiedSearchBar displays the health/level-derived options.
In `@frontend/src/components/NestedCategorySelect.tsx`:
- Around line 214-228: The dropdown can show duplicate "All" options because
FILTER_OPTIONS contains an entry with key '' and there's also a hardcoded "All
Categories" CategoryItem; to fix, filter out any options with an empty key
before mapping/rendering (e.g., derive a filtered list from filterOptions
excluding option.key === ''), then map over that filtered list when rendering
CategoryItem in NestedCategorySelect.tsx so that the existing hardcoded "All
Categories" (CategoryItem at the top) is the only empty-key entry shown; keep
using the same props (slug/name/isSelected/onSelect) and symbols (filterOptions,
CategoryItem, selected, handleSelect) when replacing the map source.
---
Nitpick comments:
In `@backend/apps/api/rest/v0/project.py`:
- Around line 148-161: The current per-slug loop (filters.categories) issues a
DB get() and descendant traversal per slug causing O(depth×slugs) queries;
extract this into a shared utility (e.g., get_category_filter_q(category_slugs:
list[str])) that first fetches all ProjectCategory rows for
slug__in=category_slugs and is_active=True in one query, collects each
category's descendant IDs (using category.get_descendants().values_list("id",
flat=True) or the model's bulk descendant query), builds a single combined Q
(category_q) of Q(categories__id__in=...) for the aggregated ids, and returns
it; then replace the inline loop in project.py (and the duplicate block in
backend/apps/owasp/api/internal/filters/project.py) to call
get_category_filter_q(filters.categories) and apply queryset =
queryset.filter(category_q).distinct().
In `@backend/apps/owasp/api/internal/filters/project.py`:
- Around line 25-32: The loop is issuing multiple DB hits per slug; instead,
fetch all matching categories in one query via
ProjectCategory.objects.filter(slug__in=value, is_active=True) (replace the
per-iteration ProjectCategory.objects.get for category_slug), then build the set
of category IDs to filter on by aggregating each category's id plus descendants;
if your tree library exposes bulk descendant filtering (tree_id/lft/rght or a
bulk API) use that to fetch all descendants in one query, otherwise call
get_descendants only on the small returned set; finally OR into category_q once
using the combined id list (references: ProjectCategory, get_descendants,
category_slug, category_q, value).
In `@backend/apps/owasp/api/internal/nodes/category.py`:
- Around line 33-39: The decorator on the parent resolver uses prefetch_related
for a ForeignKey; change the strawberry_django.field hint to use select_related
instead (replace prefetch_related=["parent"] with select_related=["parent"]) in
the parent method of ProjectCategoryNode (the def parent(self, root:
ProjectCategory) resolver) so the FK is joined rather than prefetched.
- Around line 41-47: Summary: Prefetching currently pulls all child categories
then filters in Python; use a Django Prefetch with a filtered queryset so the
is_active=True filter is applied in the DB. Update the strawberry_django.field
decorator on the children resolver to use django.db.models.Prefetch for the
"children" relation with a queryset that filters is_active=True (e.g.,
Prefetch("children", queryset=ProjectCategory.objects.filter(is_active=True))),
import Prefetch at the top, and then in def children(self, root:
ProjectCategory) return the prefetched related set (e.g.,
list(root.children.all())) so the filter is served from the DB rather than
post-filtered in Python.
In `@backend/apps/owasp/api/internal/queries/project.py`:
- Around line 123-126: The if-check "if cleaned_query and len(cleaned_query) <
MIN_SEARCH_QUERY_LENGTH" is dead code given MIN_SEARCH_QUERY_LENGTH = 1 (a
non-empty string cannot have length < 1); remove that conditional block and its
return so only the upper-bound check using MAX_SEARCH_QUERY_LENGTH remains, or
if the intent was to enforce a minimum length >1, update MIN_SEARCH_QUERY_LENGTH
accordingly and adjust callers—locate the condition by its symbols cleaned_query
and MIN_SEARCH_QUERY_LENGTH in the search/query logic and delete the unreachable
branch.
In `@backend/apps/owasp/management/commands/assign_sample_categories.py`:
- Around line 40-45: The current use of
Project.active_projects.all().order_by("?") (assigned to the projects variable
in assign_sample_categories.py) causes a full-table random sort which is slow on
large tables; replace it by sampling IDs in Python: fetch ids via
Project.active_projects.values_list('id', flat=True'), handle the case where
count <= limit, otherwise use random.sample to pick limit ids, then re-query
Project.objects.filter(id__in=selected_ids) (and optionally preserve order) so
you avoid order_by("?") and full table scans while keeping the rest of the
command logic unchanged.
In `@backend/apps/owasp/management/commands/populate_sample_categories.py`:
- Around line 117-131: The current logic skips creating a ProjectCategory when a
slug exists, which leaves existing rows with drifted fields unchanged; change
this to reconcile or use update_or_create so reruns converge: replace the
fetch-and-skip branch around ProjectCategory.objects.filter(slug=slug).first()
with ProjectCategory.objects.update_or_create(slug=slug, defaults={ 'name':
name, 'description': description, 'parent': parent_category, 'is_active': True
}) (or perform an explicit update on the returned instance) so name,
description, parent and is_active are kept in sync on every run.
In `@backend/tests/apps/owasp/api/internal/queries/project_test.py`:
- Around line 121-146: The tests
test_search_projects_single_char_query_returns_results and
test_search_projects_two_char_query_returns_results use weak assertions (assert
result != []) which can be satisfied by any truthy value; update them to assert
the actual mocked queryset is returned and that the filter was called with
expected parameters: call ProjectQuery.search_projects (via
ProjectQuery.__class__.__dict__["search_projects"]) as before, then assert
result is the MagicMock mock_queryset (e.g., result is mock_queryset) and/or
assert mock_filter.assert_called_with(...) was invoked with the expected query
pattern/arguments so the test verifies the correct queryset and filter
invocation for ProjectQuery.search_projects.
In `@frontend/src/app/projects/page.tsx`:
- Around line 73-94: Remove the redundant client-side filter on projects by
deleting the .filter((project) => project.isActive) call inside the
UnifiedSearchBar children; the backend (fetchAlgoliaData and search_projects
resolver) already ensures only active projects are returned, so pass
projects?.map(renderProjectCard) (or just projects?.map using renderProjectCard)
as the children to UnifiedSearchBar to avoid double-filtering and keep rendering
logic in renderProjectCard unchanged.
In `@frontend/src/components/NestedCategorySelect.tsx`:
- Around line 23-50: CategoryItem is inaccessible to Tab navigation because it
sets tabIndex={-1}; make each CategoryItem focusable and support arrow-key
navigation by switching to a roving focus/tabindex pattern: give interactive
items tabIndex={0} when they should be focusable (or manage a focusedIndex in
the parent component and set tabIndex={ focusedIndex === index ? 0 : -1 }), add
arrow key handling in CategoryItem's onKeyDown to call onSelect or a provided
onMoveFocus callback for ArrowUp/ArrowDown/ArrowLeft/ArrowRight, and
expose/select via Enter/Space as already implemented; also consider adding
aria-selected and ensuring the parent manages focus changes (use refs or
parent-level focus management) so keyboard users can tab into the menu and then
use arrow keys to move between CategoryItem elements.
In `@frontend/src/hooks/useSearchProjectsGraphQL.ts`:
- Around line 82-94: The filters existence check in the useQuery call is
redundant because the useMemo that computes filters already returns undefined
when empty; update the variables passed to useQuery (in useSearchProjectsGraphQL
where GetProjectsListDocument is used) to pass filters directly (i.e., use
filters instead of Object.keys(filters || {}).length > 0 ? filters : undefined)
and remove the unnecessary Object.keys(...) conditional so the code uses the
memoized filters value as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8051e367-d262-472b-aba9-d10e091d0b46
⛔ Files ignored due to path filters (3)
frontend/src/types/__generated__/categoryQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (34)
backend/Makefilebackend/apps/api/rest/v0/__init__.pybackend/apps/api/rest/v0/category.pybackend/apps/api/rest/v0/project.pybackend/apps/owasp/admin/__init__.pybackend/apps/owasp/admin/category.pybackend/apps/owasp/admin/project.pybackend/apps/owasp/api/internal/filters/category.pybackend/apps/owasp/api/internal/filters/project.pybackend/apps/owasp/api/internal/nodes/category.pybackend/apps/owasp/api/internal/nodes/project.pybackend/apps/owasp/api/internal/ordering/category.pybackend/apps/owasp/api/internal/queries/__init__.pybackend/apps/owasp/api/internal/queries/category.pybackend/apps/owasp/api/internal/queries/project.pybackend/apps/owasp/management/commands/assign_sample_categories.pybackend/apps/owasp/management/commands/populate_sample_categories.pybackend/apps/owasp/migrations/0073_projectcategory_project_categories_and_more.pybackend/apps/owasp/models/__init__.pybackend/apps/owasp/models/category.pybackend/apps/owasp/models/project.pybackend/tests/apps/owasp/api/internal/queries/project_test.pydocker-compose/local/compose.yamlfrontend/src/app/projects/dashboard/metrics/page.tsxfrontend/src/app/projects/page.tsxfrontend/src/components/NestedCategorySelect.tsxfrontend/src/components/SearchPageLayout.tsxfrontend/src/components/UnifiedSearchBar.tsxfrontend/src/hooks/useProjectCategories.tsfrontend/src/hooks/useSearchProjectsGraphQL.tsfrontend/src/server/queries/categoryQueries.tsfrontend/src/server/queries/projectQueries.tsfrontend/src/types/project.tsfrontend/src/types/unifiedSearchBar.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/components/UnifiedSearchBar.tsx
- frontend/src/components/SearchPageLayout.tsx
- frontend/src/types/unifiedSearchBar.ts
backend/apps/owasp/migrations/0073_projectcategory_project_categories_and_more.py
Show resolved
Hide resolved
…-filter-implementation
anurag2787
force-pushed
the
feature/search-filter-implementation
branch
from
3e1eea7 to
ef3d842
Compare
|
Contributor
Author
|
Hi @kasya,
These commands is used locally to generate sample categories and assign them to projects for verifying the filtering functionality.: Screencast.from.2026-03-10.19-32-48.webmAre these changes looks good? should i start writing tests for this functionality? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
This PR extends the Projects search functionality by introducing a category filter and a unified sorting dropdown within a reusable search component.
The search UI has been refactored into a shared component used across both the Projects list page and the Project Health dashboard to ensure consistent behavior and UX.
Resolves #4086
Screencast.from.2026-03-04.06-18-20.webm
Checklist
make check-testlocally: all warnings addressed, tests passed