feat: add reusable search component with category filter and sorting#4199
Draft
HarshitVerma109 wants to merge 6 commits intoOWASP:mainfrom
Draft
feat: add reusable search component with category filter and sorting#4199HarshitVerma109 wants to merge 6 commits intoOWASP:mainfrom
HarshitVerma109 wants to merge 6 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:
WalkthroughAdds a reusable SearchWithFilters UI and wiring across Projects and Project Health dashboards; implements GraphQL project filtering, ordering, and pagination; extends REST ordering semantics and Algolia facets; adds tests, test utilities, and frontend backend-selection/configuration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 |
![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 32 files
Confidence score: 3/5
- There is some real regression risk:
frontend/src/hooks/useSearchPage.tsandfrontend/src/app/projects/dashboard/metrics/page.tsxhave URL/state sync gaps (category key mismatch, ignoredlevel, and sort UI drift), which can make active filters appear incorrect after navigation. backend/apps/owasp/api/internal/queries/project_health_metrics.pylikely applies filters twice inside a@strawberry_django.field(filters=...)resolver, which can change query behavior unexpectedly and is user-impacting at the data layer.- Several test issues reduce safety nets rather than directly breaking runtime behavior (e.g., shallow assertions and mock reuse/leakage in
frontend/__tests__/unit/components/SearchWithFilters.test.tsx,frontend/__tests__/unit/pages/Contribute.test.tsx, and backend query tests), so regressions may slip through. - Pay close attention to
frontend/src/hooks/useSearchPage.ts,frontend/src/app/projects/dashboard/metrics/page.tsx, andbackend/apps/owasp/api/internal/queries/project_health_metrics.py- filter synchronization and duplicate queryset filtering are the main merge risks.
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/hooks/useSearchProjectsGraphQL.ts">
<violation number="1" location="frontend/src/hooks/useSearchProjectsGraphQL.ts:10">
P2: `options.pageSize` is declared in the hook options type but never used; pagination always uses the positional `pageSize` argument, creating a misleading and inconsistent API contract.</violation>
</file>
<file name="frontend/__tests__/unit/components/SearchWithFilters.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/SearchWithFilters.test.tsx:63">
P2: Behavioral tests are shallow: fallback/selected-value tests only assert element presence, so key filter/sort regressions can pass unnoticed.</violation>
</file>
<file name="backend/apps/owasp/api/internal/queries/project_health_metrics.py">
<violation number="1" location="backend/apps/owasp/api/internal/queries/project_health_metrics.py:65">
P2: Filters are manually applied inside a `@strawberry_django.field(filters=...)` resolver, likely causing duplicate filter application on the same queryset.</violation>
</file>
<file name="frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx:42">
P2: Test mock rewrites `Needs Attention` to `Need Attention`, desynchronizing mocked behavior from real component labels.</violation>
<violation number="2" location="frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx:813">
P2: `useSearchParams` mock implementations can leak across tests because teardown uses `clearAllMocks` instead of resetting mock implementations.</violation>
</file>
<file name="frontend/src/hooks/useSearchPage.ts">
<violation number="1" location="frontend/src/hooks/useSearchPage.ts:89">
P2: Category is pushed to the URL but never synchronized back from `searchParams`, causing URL/state divergence and stale filtering after navigation.</violation>
</file>
<file name="backend/tests/apps/owasp/api/internal/queries/project_health_metrics_test.py">
<violation number="1" location="backend/tests/apps/owasp/api/internal/queries/project_health_metrics_test.py:213">
P2: Combined query+filters tests reuse the same mock queryset for all stages, masking chaining/hand-off regressions.</violation>
</file>
<file name="frontend/__tests__/unit/pages/Contribute.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/Contribute.test.tsx:150">
P2: Search test assertion is satisfied by initial mount fetch, so it can pass without validating search/clear interaction behavior.</violation>
</file>
<file name="frontend/src/app/projects/dashboard/metrics/page.tsx">
<violation number="1" location="frontend/src/app/projects/dashboard/metrics/page.tsx:136">
P2: Sort controls can become out of sync with actual data ordering because URL updates only resync `ordering`, not `sortBy`/`order` UI state.</violation>
<violation number="2" location="frontend/src/app/projects/dashboard/metrics/page.tsx:138">
P2: Category state is hydrated from URL with raw values (`healthy`) instead of namespaced option keys (`health:healthy`), and ignores `level` URL filters, causing filter UI to not reflect active URL-applied filters.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
backend/apps/owasp/api/internal/queries/project_health_metrics.py
Outdated
Show resolved
Hide resolved
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx
Outdated
Show resolved
Hide resolved
backend/tests/apps/owasp/api/internal/queries/project_health_metrics_test.py
Outdated
Show resolved
Hide resolved
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/hooks/useSearchPage.ts (1)
75-93:⚠️ Potential issue | 🟠 MajorSwitch this URL sync to
replace, or make the URL the source of truth.
router.pushadds a history entry for every search/filter/page change, but this hook still never restoressearchQuery,sortBy,order, orcategoryfromsearchParams. After a few changes, Back/Forward can leave the URL on one filter state and the rendered results on another.Minimal safe fix if you do not want filter history
- router.push(`?${params.toString()}`) + router.replace(`?${params.toString()}`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useSearchPage.ts` around lines 75 - 93, The current useEffect in useSearchPage uses router.push which creates history entries but the hook never restores state from the URL; either switch to router.replace to avoid polluting history or make the URL the source of truth by reading search params on mount and syncing them into searchQuery, sortBy, order, category and currentPage. Concretely: in the effect that builds params (inside useSearchPage) replace router.push(`?${params.toString()}`) with router.replace(...) to avoid back/forward mismatches, and add an initialization step (on mount) that parses URLSearchParams/router.query and sets the hook state (searchQuery, sortBy, order, category, currentPage) so the UI always reflects the URL if you prefer URL-driven state.
🧹 Nitpick comments (7)
frontend/src/server/fetchAlgoliaData.ts (1)
14-18: Consider handling conflictingidx_is_activevalues.The check
!facetFilters.includes('idx_is_active:true')prevents duplication, but if a caller passesidx_is_active:false, the code would addidx_is_active:truealongside it, resulting in conflicting filters. Depending on Algolia's behavior, this could return empty results or behave unexpectedly.If inactive items should never be returned for these indices, consider checking for any existing
idx_is_activefilter:♻️ Proposed fix to handle conflicting values
const shouldForceActiveFilter = indexName.startsWith('projects') || indexName === 'chapters' +const hasActiveFilter = facetFilters.some((f) => f.startsWith('idx_is_active:')) const computedFacetFilters = - shouldForceActiveFilter && !facetFilters.includes('idx_is_active:true') + shouldForceActiveFilter && !hasActiveFilter ? [...facetFilters, 'idx_is_active:true'] - : [...facetFilters] + : facetFiltersThis also avoids creating an unnecessary shallow copy when no modification is needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/server/fetchAlgoliaData.ts` around lines 14 - 18, The computedFacetFilters logic may add a conflicting idx_is_active:true alongside idx_is_active:false; update the code that builds computedFacetFilters (and uses shouldForceActiveFilter and facetFilters) to detect any existing filter that starts with "idx_is_active:" — if one exists and is "idx_is_active:true" leave facetFilters unchanged, if it exists as "idx_is_active:false" replace that entry with "idx_is_active:true", and if none exists append "idx_is_active:true"; also avoid creating a shallow copy when no modification is needed (return facetFilters directly in that case).frontend/src/components/SearchPageLayout.tsx (1)
20-20: Make the replacement contract explicit.
searchBarChildrenalso suppressessortChildren, so callers are replacing the whole controls row, not just the search bar. Renaming the prop or making sort suppression explicit would make this layout API harder to misuse.♻️ Possible tidy-up
- searchBarChildren?: React.ReactNode + searchControlsChildren?: React.ReactNode ... - searchBarChildren, + searchControlsChildren, ... - {searchBarChildren ? ( - <div className="w-full max-w-4xl">{searchBarChildren}</div> + {searchControlsChildren ? ( + <div className="w-full max-w-4xl">{searchControlsChildren}</div> ) : ( ... - {!searchBarChildren && totalPages !== 0 && ( + {!searchControlsChildren && totalPages !== 0 && (Also applies to: 35-35, 47-49, 61-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/SearchPageLayout.tsx` at line 20, The prop searchBarChildren currently replaces the entire controls row and implicitly suppresses sortChildren, which is confusing; update the API on SearchPageLayout to make this explicit by either renaming searchBarChildren to something like controlsRowChildren or adding an explicit boolean (e.g., replaceControlsRow or suppressSort) and adjust usages to pass the new prop; update the prop type/definition and all places that pass searchBarChildren (and where sortChildren is relied on) so callers intentionally opt into replacing the whole controls row rather than accidentally hiding sorting controls.frontend/src/wrappers/testUtil.tsx (1)
9-11: Type assertion may cause runtime issues.The type
React.ComponentType<Record<string, unknown>> | nullsuggests ApolloProvider could be null, but the require should always succeed if@apollo/clientis installed. The current casting chain is fragile - ifApolloProvideris not exported as expected, this will silently result inundefinedrather thannull.Consider adding a runtime check or simplifying:
♻️ Suggested improvement
-// eslint-disable-next-line `@typescript-eslint/no-require-imports` -const ApolloProvider = (require('@apollo/client/react') as Record<string, unknown>) - .ApolloProvider as React.ComponentType<Record<string, unknown>> | null +// eslint-disable-next-line `@typescript-eslint/no-require-imports` +const ApolloProviderModule = require('@apollo/client/react') as Record<string, unknown> +const ApolloProvider = ApolloProviderModule?.ApolloProvider as + | React.ComponentType<{ client: unknown; children: React.ReactNode }> + | undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wrappers/testUtil.tsx` around lines 9 - 11, The current require-and-cast for ApolloProvider is fragile and can produce undefined at runtime; change the code that loads ApolloProvider (the require('@apollo/client/react') call and the ApolloProvider variable) to perform a runtime check after require: retrieve the module, verify that module.ApolloProvider is a function/component, and if not throw a clear error or provide a safe fallback; remove the union type that allows null and instead narrow the type after the runtime assertion so callers can assume ApolloProvider is a valid React.ComponentType (reference the ApolloProvider symbol and the require call to locate the code).backend/apps/owasp/api/internal/queries/project_health_metrics.py (1)
99-108: Consider extracting duplicated filtering logic.The query filtering logic (lines 99-106) duplicates lines 58-65. While acceptable for two occurrences, extracting to a helper method would improve maintainability if more query methods are added.
♻️ Suggested helper extraction
def _apply_query_filter(queryset, query: str): """Apply query string filter to queryset.""" cleaned_query = query.strip() if query else "" if cleaned_query: queryset = queryset.filter(project__name__icontains=cleaned_query) return queryset🤖 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 99 - 108, The filtering logic for query strings is duplicated between the count path and the main query path; extract it into a small helper (e.g., _apply_query_filter) and call it from both places so ProjectHealthMetrics.get_latest_health_metrics() result is passed through the same routine; implement _apply_query_filter(queryset, query) to trim the query, return the original queryset if empty, and otherwise apply .filter(project__name__icontains=cleaned_query), then replace the duplicated blocks in the methods that currently perform that inline filtering with calls to _apply_query_filter.frontend/src/hooks/useSearchProjectsGraphQL.ts (2)
66-70: Minor: Redundant condition check.The check
!category || category === ''can be simplified to just!categorysince an empty string is falsy in JavaScript.♻️ Simplify condition
const filters = useMemo(() => { - if (!category || category === '') return undefined + if (!category) return undefined const projectType = mapCategoryToProjectType(category) return projectType ? { type: projectType } : undefined }, [category])🤖 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 66 - 70, The condition inside the useMemo for computing filters is redundant: simplify the guard from `if (!category || category === '')` to just `if (!category)` in the filters computation so the memo block (useMemo(() => { ... }, [category])) returns undefined for falsy categories; update the code around the filters constant and keep the call to mapCategoryToProjectType and the return shape unchanged.
88-92: Theas Project[]cast is unnecessary and redundant. Apollo Client already types thedataobject based onGetProjectsListDocument, sodata?.searchProjectsis already properly typed. Relying on Apollo's generated types instead of using the assertion maintains type safety and clarity. Remove the cast and let Apollo's type system handle it.🤖 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 88 - 92, Remove the unnecessary type assertion on projects in useSearchProjectsGraphQL: eliminate the "as Project[]" cast on the returned items and return projects directly (items: projects) so Apollo Client's generated types from GetProjectsListDocument are relied upon; update the return in the function that defines projects and items (the projects variable and the return object in useSearchProjectsGraphQL) accordingly.frontend/src/hooks/useSearchPage.ts (1)
95-105: Don't exposegraphqlas a generic backend option yet.
useSearchPage<T>still looks generic, but the new GraphQL branch always callsuseSearchProjectsGraphQLand ignoresindexName. Any non-project caller that opts intouseBackend="graphql"will silently fetch project data into the wrongT. Either narrow this option to project pages or guardindexName === 'projects'here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useSearchPage.ts` around lines 95 - 105, The GraphQL branch in useSearchPage<T> should not run for non-project indexes; update the condition so useSearchProjectsGraphQL is only invoked when useBackend === 'graphql' AND indexName === 'projects' (or otherwise restrict useBackend='graphql' to project-only callers), so that useSearchPage<T> does not silently fetch project data into the wrong T; locate the GraphQL call site (useSearchProjectsGraphQL) and the surrounding isGraphQLBackend check and add the indexName guard or narrow the API to project pages.
🤖 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 117-118: Remove the string values "level" and "-level" from the
ordering options so clients cannot sort alphabetically; update the ordering
configuration used in ProjectViewSet (or wherever ordering_fields/ordering is
defined) to only expose "level_raw" and "-level_raw" (or ensure the existing
"level_raw" entries remain) for project level ordering. Locate the ordering list
that currently includes "level" and "-level" and delete those two entries,
leaving numeric-backed fields (level_raw/-level_raw) as the sole level-ordering
options.
In `@backend/apps/owasp/api/internal/filters/project.py`:
- Around line 14-17: The custom filter method type(self, value: ProjectType,
prefix: str) currently hardcodes Q(type=...) and ignores the prefix; change it
to build the lookup key with the prefix (e.g. lookup_key = f"{prefix}type" or
f"{prefix}type" when prefix may be empty) and return Q(**{lookup_key:
ProjectType(value)}) if value else Q(); update the method that constructs the Q
so Strawberry-composed nested filters target the correct related field instead
of always the top-level "type".
In `@frontend/src/app/projects/dashboard/metrics/page.tsx`:
- Around line 116-120: currentFilters' unified "category" seed is only reading
healthFilter so URLs like ?level=lab don't prefill the shared dropdown; update
the code that builds currentFilters (the logic that sets category from
healthFilter) to also consider levelFilter (and fallback to healthFilter) —
i.e., when computing category use levelFilter ?? healthFilter ?? '' (or
equivalent) so the dropdown reflects either ?level=... or ?health=... on initial
load; apply the same change where category is seeded elsewhere (the other
instance referenced around the block using healthFilter/levelFilter).
---
Outside diff comments:
In `@frontend/src/hooks/useSearchPage.ts`:
- Around line 75-93: The current useEffect in useSearchPage uses router.push
which creates history entries but the hook never restores state from the URL;
either switch to router.replace to avoid polluting history or make the URL the
source of truth by reading search params on mount and syncing them into
searchQuery, sortBy, order, category and currentPage. Concretely: in the effect
that builds params (inside useSearchPage) replace
router.push(`?${params.toString()}`) with router.replace(...) to avoid
back/forward mismatches, and add an initialization step (on mount) that parses
URLSearchParams/router.query and sets the hook state (searchQuery, sortBy,
order, category, currentPage) so the UI always reflects the URL if you prefer
URL-driven state.
---
Nitpick comments:
In `@backend/apps/owasp/api/internal/queries/project_health_metrics.py`:
- Around line 99-108: The filtering logic for query strings is duplicated
between the count path and the main query path; extract it into a small helper
(e.g., _apply_query_filter) and call it from both places so
ProjectHealthMetrics.get_latest_health_metrics() result is passed through the
same routine; implement _apply_query_filter(queryset, query) to trim the query,
return the original queryset if empty, and otherwise apply
.filter(project__name__icontains=cleaned_query), then replace the duplicated
blocks in the methods that currently perform that inline filtering with calls to
_apply_query_filter.
In `@frontend/src/components/SearchPageLayout.tsx`:
- Line 20: The prop searchBarChildren currently replaces the entire controls row
and implicitly suppresses sortChildren, which is confusing; update the API on
SearchPageLayout to make this explicit by either renaming searchBarChildren to
something like controlsRowChildren or adding an explicit boolean (e.g.,
replaceControlsRow or suppressSort) and adjust usages to pass the new prop;
update the prop type/definition and all places that pass searchBarChildren (and
where sortChildren is relied on) so callers intentionally opt into replacing the
whole controls row rather than accidentally hiding sorting controls.
In `@frontend/src/hooks/useSearchPage.ts`:
- Around line 95-105: The GraphQL branch in useSearchPage<T> should not run for
non-project indexes; update the condition so useSearchProjectsGraphQL is only
invoked when useBackend === 'graphql' AND indexName === 'projects' (or otherwise
restrict useBackend='graphql' to project-only callers), so that useSearchPage<T>
does not silently fetch project data into the wrong T; locate the GraphQL call
site (useSearchProjectsGraphQL) and the surrounding isGraphQLBackend check and
add the indexName guard or narrow the API to project pages.
In `@frontend/src/hooks/useSearchProjectsGraphQL.ts`:
- Around line 66-70: The condition inside the useMemo for computing filters is
redundant: simplify the guard from `if (!category || category === '')` to just
`if (!category)` in the filters computation so the memo block (useMemo(() => {
... }, [category])) returns undefined for falsy categories; update the code
around the filters constant and keep the call to mapCategoryToProjectType and
the return shape unchanged.
- Around line 88-92: Remove the unnecessary type assertion on projects in
useSearchProjectsGraphQL: eliminate the "as Project[]" cast on the returned
items and return projects directly (items: projects) so Apollo Client's
generated types from GetProjectsListDocument are relied upon; update the return
in the function that defines projects and items (the projects variable and the
return object in useSearchProjectsGraphQL) accordingly.
In `@frontend/src/server/fetchAlgoliaData.ts`:
- Around line 14-18: The computedFacetFilters logic may add a conflicting
idx_is_active:true alongside idx_is_active:false; update the code that builds
computedFacetFilters (and uses shouldForceActiveFilter and facetFilters) to
detect any existing filter that starts with "idx_is_active:" — if one exists and
is "idx_is_active:true" leave facetFilters unchanged, if it exists as
"idx_is_active:false" replace that entry with "idx_is_active:true", and if none
exists append "idx_is_active:true"; also avoid creating a shallow copy when no
modification is needed (return facetFilters directly in that case).
In `@frontend/src/wrappers/testUtil.tsx`:
- Around line 9-11: The current require-and-cast for ApolloProvider is fragile
and can produce undefined at runtime; change the code that loads ApolloProvider
(the require('@apollo/client/react') call and the ApolloProvider variable) to
perform a runtime check after require: retrieve the module, verify that
module.ApolloProvider is a function/component, and if not throw a clear error or
provide a safe fallback; remove the union type that allows null and instead
narrow the type after the runtime assertion so callers can assume ApolloProvider
is a valid React.ComponentType (reference the ApolloProvider symbol and the
require call to locate the code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4ce2a264-72e9-431d-abfd-5a76bcc0f587
⛔ 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 (29)
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/tests/apps/owasp/api/internal/filters/project_test.pybackend/tests/apps/owasp/api/internal/queries/project_health_metrics_test.pybackend/tests/apps/owasp/api/internal/queries/project_test.pyfrontend/.env.e2e.examplefrontend/.env.examplefrontend/__tests__/unit/components/SearchWithFilters.test.tsxfrontend/__tests__/unit/pages/Contribute.test.tsxfrontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsxfrontend/src/app/projects/dashboard/metrics/page.tsxfrontend/src/app/projects/page.tsxfrontend/src/components/SearchPageLayout.tsxfrontend/src/components/SearchWithFilters.tsxfrontend/src/components/SortBy.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/searchWithFilters.tsfrontend/src/utils/backendConfig.tsfrontend/src/utils/helpers/mockApolloClient.tsfrontend/src/utils/sortingOptions.tsfrontend/src/wrappers/testUtil.tsx
Contributor
There was a problem hiding this comment.
5 issues found across 11 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:141">
P2: `jest.restoreAllMocks()` does not clear call history for `jest.fn()` mocks, so `mockReplace`/`useQuery` calls can leak between tests and make assertions order-dependent. Use `jest.clearAllMocks()` (or `resetAllMocks`) to clear call history between tests.</violation>
</file>
<file name="backend/apps/owasp/api/internal/queries/project_health_metrics.py">
<violation number="1" location="backend/apps/owasp/api/internal/queries/project_health_metrics.py:65">
P1: `project_health_metrics` no longer applies the `filters` argument, so filtered queries return unfiltered lists and can disagree with `project_health_metrics_distinct_length`.</violation>
</file>
<file name="frontend/src/hooks/useSearchPage.ts">
<violation number="1" location="frontend/src/hooks/useSearchPage.ts:68">
P2: URL-driven category updates do not reset `currentPage`, which can leave stale/out-of-range pagination after category changes via back/forward or manual URL edits.</violation>
</file>
<file name="frontend/src/app/projects/dashboard/metrics/page.tsx">
<violation number="1" location="frontend/src/app/projects/dashboard/metrics/page.tsx:138">
P2: Single-value category state can hide active double-filtering when both `health` and `level` URL params are present.</violation>
</file>
<file name="frontend/__tests__/unit/pages/Contribute.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/Contribute.test.tsx:148">
P2: Test can be flaky/false-positive because `mockClear` runs before mount-triggered fetch is guaranteed to execute, allowing initial empty-query call to satisfy later assertion.</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: 7
🧹 Nitpick comments (1)
backend/tests/apps/owasp/api/internal/queries/project_health_metrics_test.py (1)
222-234: Consider adding a test for whitespace-only query.The production code uses
query.strip(), so a whitespace-only query like" "should behave the same as an empty string. Adding a test for this edge case would ensure thestrip()logic is covered.🧪 Optional test for whitespace-only query
`@patch`( "apps.owasp.models.project_health_metrics.ProjectHealthMetrics.get_latest_health_metrics" ) def test_project_health_metrics_with_whitespace_only_query(self, mock_get_latest_metrics): """Test project_health_metrics treats whitespace-only query as empty.""" base_qs = MagicMock() mock_get_latest_metrics.return_value = base_qs query = ProjectHealthMetricsQuery() result = query.project_health_metrics(query=" ", pagination=None, ordering=None) base_qs.filter.assert_not_called() assert result == base_qs🤖 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_health_metrics_test.py` around lines 222 - 234, Add a new unit test that mirrors test_project_health_metrics_without_query but passes a whitespace-only string to ProjectHealthMetricsQuery.project_health_metrics to ensure strip() behavior is covered: patch apps.owasp.models.project_health_metrics.ProjectHealthMetrics.get_latest_health_metrics, return a MagicMock base_qs, call ProjectHealthMetricsQuery().project_health_metrics(query=" ", pagination=None, ordering=None), assert base_qs.filter.assert_not_called() and that the result equals base_qs so whitespace-only queries are treated like empty ones.
🤖 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 102-118: The public ordering literal currently exposes
"level_raw"; change the ordering Literal to use "level" and "-level" instead of
"level_raw"/"-level_raw" and then map that public key to the internal column
before applying ordering: when you parse the ordering value (the ordering
variable used in project listing), translate "level" -> "level_raw" and "-level"
-> "-level_raw" prior to passing the result into order_by() so callers see the
backend-agnostic "level" option but the query still uses the internal column.
- Line 136: The query ordering needs a stable tiebreaker to avoid page
shuffling—update the queryset.order_by call (the one that currently uses
ordering or "-level_raw", "-stars_count", "-forks_count") to append a unique key
(use "pk" as the final sort field) so ties are deterministic; implement this by
ensuring the final order args include "pk" (or skip adding if the supplied
ordering string already contains "pk") when building the arguments passed to
queryset.order_by.
In `@frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx`:
- Around line 140-142: Tests leak router/search-param mock state because
module-level mocks (e.g., mockReplace and useSearchParams) keep call history; to
fix, add a beforeEach block that explicitly clears/reset these mocks (call
mockReplace.mockClear() and reset the mocked useSearchParams return or its mock
implementation) before each test, and keep the existing afterEach
jest.restoreAllMocks(); update references in the test file where mockReplace and
useSearchParams are defined so the new beforeEach targets those exact mock
variables.
In `@frontend/src/app/projects/dashboard/metrics/page.tsx`:
- Around line 122-128: Normalize the active category before composing filters:
determine a single category (e.g., prefer health if healthFilter is present,
otherwise use level) and use that chosen category value to both set the
dropdown's category state and to build the query filters, rather than merging
both healthFiltersMapping[healthFilter] and levelFiltersMapping[levelFilter]
into currentFilters; update the logic around currentFilters, healthFilter,
levelFilter, healthFiltersMapping, levelFiltersMapping and the code that
produces filters (also update the similar block at the later occurrence) so the
visible dropdown category and the generated filters are derived from the same
normalized category decision.
- Around line 178-188: The handlers handleSortChange and handleOrderChange
currently each build URLSearchParams from the same stale searchParams and call
router.replace separately, which can cause one update to overwrite the other;
fix this by consolidating updates so both sortBy and order are applied in a
single router.replace call—either create a new helper like
updateSortAndOrder(sortBy: string, order: string) or change updateUrlParams to
accept both keys and merge them against the current URLSearchParams before
calling router.replace; update handleSortChange and handleOrderChange to call
that single merge/update function (referencing handleSortChange,
handleOrderChange, updateUrlParams and router.replace) so both parameters are
set together as in the ProjectsHealthDashboardMetrics.test.tsx flow.
In `@frontend/src/hooks/useSearchPage.ts`:
- Around line 63-75: The hook's URL-sync logic updates category via
categoryParam -> setCategory but doesn't treat a category change as a reason to
reset pagination; update the reset condition(s) inside useSearchPage to also
consider category changes by computing a boolean like categoryChanged
(categoryParam !== category) and include it in the if that calls
setCurrentPage(1) (both the block using searchQueryChanged/sortOrOrderChanged
and the other sync block later that mirrors this logic) so navigating to a
different ?category resets currentPage to 1.
In `@frontend/src/hooks/useSearchProjectsGraphQL.ts`:
- Around line 33-53: buildGraphQLOrdering currently returns only the primary
sort, which can produce nondeterministic paging; modify buildGraphQLOrdering to
always append a deterministic secondary tie-breaker ordering by the project name
(e.g., { name: 'ASC' }) when the primary sort field is not the name, so the
returned array contains the primary ordering followed by the alphabetical
tie-breaker; also apply the same change to the other identical
ordering-construction block referenced in the diff (the second occurrence of
this logic).
---
Nitpick comments:
In
`@backend/tests/apps/owasp/api/internal/queries/project_health_metrics_test.py`:
- Around line 222-234: Add a new unit test that mirrors
test_project_health_metrics_without_query but passes a whitespace-only string to
ProjectHealthMetricsQuery.project_health_metrics to ensure strip() behavior is
covered: patch
apps.owasp.models.project_health_metrics.ProjectHealthMetrics.get_latest_health_metrics,
return a MagicMock base_qs, call
ProjectHealthMetricsQuery().project_health_metrics(query=" ", pagination=None,
ordering=None), assert base_qs.filter.assert_not_called() and that the result
equals base_qs so whitespace-only queries are treated like empty ones.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9efc3e21-a2a8-4da7-a887-4cf3dc3f394b
📒 Files selected for processing (11)
backend/apps/api/rest/v0/project.pybackend/apps/owasp/api/internal/filters/project.pybackend/apps/owasp/api/internal/queries/project_health_metrics.pybackend/tests/apps/owasp/api/internal/filters/project_test.pybackend/tests/apps/owasp/api/internal/queries/project_health_metrics_test.pyfrontend/__tests__/unit/components/SearchWithFilters.test.tsxfrontend/__tests__/unit/pages/Contribute.test.tsxfrontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsxfrontend/src/app/projects/dashboard/metrics/page.tsxfrontend/src/hooks/useSearchPage.tsfrontend/src/hooks/useSearchProjectsGraphQL.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/tests/unit/components/SearchWithFilters.test.tsx
- backend/apps/owasp/api/internal/filters/project.py
- backend/tests/apps/owasp/api/internal/filters/project_test.py
Contributor
There was a problem hiding this comment.
2 issues found across 6 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/hooks/useSearchPage.ts">
<violation number="1" location="frontend/src/hooks/useSearchPage.ts:97">
P2: Page reset can be skipped because reset detection relies on side effects inside queued state updaters, then is read immediately.</violation>
</file>
<file name="backend/apps/api/rest/v0/project.py">
<violation number="1" location="backend/apps/api/rest/v0/project.py:115">
P2: Backward-incompatible API change: `ordering=level_raw` is no longer accepted by the Literal-validated query parameter, so existing clients using the previous values will now fail validation before the mapping logic runs.</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.
♻️ Duplicate comments (1)
frontend/src/app/projects/dashboard/metrics/page.tsx (1)
122-128:⚠️ Potential issue | 🟡 MinorFilter/category state mismatch when URL contains both health and level params.
When the URL contains both
?health=healthy&level=lab,currentFiltersapplies both constraints (lines 122-128), but the dropdown only displays one category viacomputeInitialCategory(health takes precedence). This creates an inconsistency where the visible state doesn't match the actual filter behavior.Consider normalizing to a single active filter to match the dropdown's single-selection model:
🛠️ Suggested fix to normalize filters
let currentFilters: Record<string, unknown> = {} - if (healthFilter && healthFiltersMapping[healthFilter]) { - currentFilters = { ...currentFilters, ...healthFiltersMapping[healthFilter] } - } - if (levelFilter && levelFiltersMapping[levelFilter]) { - currentFilters = { ...currentFilters, ...levelFiltersMapping[levelFilter] } + // Apply only one filter, matching the dropdown's single-selection model + if (healthFilter && healthFiltersMapping[healthFilter]) { + currentFilters = { ...healthFiltersMapping[healthFilter] } + } else if (levelFilter && levelFiltersMapping[levelFilter]) { + currentFilters = { ...levelFiltersMapping[levelFilter] } }🤖 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 122 - 128, The code builds currentFilters from both healthFilter and levelFilter, but the UI dropdown is single-selection via computeInitialCategory; to fix, normalize to a single active filter before constructing currentFilters: call computeInitialCategory(healthFilter, levelFilter) (or reuse its logic) to determine which filter wins, then only merge the corresponding mapping (healthFiltersMapping or levelFiltersMapping) into currentFilters and ignore the other; update places referencing healthFilter/levelFilter/state to reflect this single-source selection so UI and filter behavior remain consistent.
🧹 Nitpick comments (7)
backend/tests/apps/api/rest/v0/project_test.py (1)
282-291: Tests correctly updated forpktiebreaker; consider expanding coverage.The parametrized tests accurately verify the
level→level_rawmapping andpksuffix. For completeness, consider extending the test cases to include the newly added ordering fields (contributors_count,forks_count,stars_count,name) to ensure all ordering paths are validated.♻️ Example extension for parametrized test cases
`@pytest.mark.parametrize`( ("ordering", "expected_order"), [ ("created_at", ("created_at", "-stars_count", "-forks_count", "pk")), ("-created_at", ("-created_at", "-stars_count", "-forks_count", "pk")), ("updated_at", ("updated_at", "-stars_count", "-forks_count", "pk")), ("-updated_at", ("-updated_at", "-stars_count", "-forks_count", "pk")), ("level", ("level_raw", "-stars_count", "-forks_count", "pk")), ("-level", ("-level_raw", "-stars_count", "-forks_count", "pk")), + ("stars_count", ("stars_count", "-stars_count", "-forks_count", "pk")), + ("-stars_count", ("-stars_count", "-stars_count", "-forks_count", "pk")), + ("forks_count", ("forks_count", "-stars_count", "-forks_count", "pk")), + ("contributors_count", ("contributors_count", "-stars_count", "-forks_count", "pk")), + ("name", ("name", "-stars_count", "-forks_count", "pk")), + ("-name", ("-name", "-stars_count", "-forks_count", "pk")), ], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/api/rest/v0/project_test.py` around lines 282 - 291, Add parametrized cases to the existing pytest.mark.parametrize block (the parameters named "ordering" and "expected_order") to cover the new ordering fields contributors_count, forks_count, stars_count and name; for each new ordering string (e.g., "contributors_count", "-contributors_count", "forks_count", "-forks_count", "stars_count", "-stars_count", "name", "-name") supply the corresponding expected_order tuple that maps to the actual DB order keys used by the view (preserving the existing tiebreaker sequence like "-stars_count", "-forks_count", "pk" or the equivalent for name) so the test function that consumes ordering/expected_order validates these new ordering paths as well.backend/apps/api/rest/v0/project.py (1)
102-118: Ordering options properly expanded with backend-agnosticlevelkey.The expanded ordering options align well with the GraphQL ordering API by exposing
levelas the public key while mapping it internally tolevel_raw(lines 136-138). This addresses the previous review feedback about backend consistency.However, the parametrized tests (lines 282-291 in test file) only cover
created_at,updated_at, andlevelvariants. Consider adding test cases for the new fields (contributors_count,forks_count,stars_count,name) to ensure complete coverage.🤖 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 102 - 118, The ordering enum in project.py was expanded to include contributors_count, forks_count, stars_count, and name (alongside level which maps to level_raw), but the parametrized tests only cover created_at, updated_at, and level; update the test suite's parametrized test (the test that iterates ordering values) to include both ascending and descending variants for contributors_count, forks_count, stars_count, and name so each new ordering option is exercised; ensure the tests assert the same expectations used for created_at/updated_at (sorting direction and field mapping), and keep the existing level test that verifies mapping to level_raw.frontend/src/hooks/useSearchProjectsGraphQL.ts (2)
82-91: Simplify redundant nullish coalescing.The
filters ?? undefinedandordering ?? undefinedexpressions are unnecessary sinceundefined ?? undefinedequalsundefined.♻️ Suggested simplification
const { data, loading, error } = useQuery(GetProjectsListDocument, { variables: { query: searchParam, - filters: filters ?? undefined, - ordering: ordering ?? undefined, + filters, + ordering, pagination: { offset, limit: pageSize }, }, errorPolicy: 'all', skip: options?.enabled === false, })🤖 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 - 91, The variables object passed to useQuery includes redundant nullish coalescing (filters ?? undefined and ordering ?? undefined); update the variables for GetProjectsListDocument to pass filters and ordering directly (i.e., use filters and ordering as-is) and keep pagination as { offset, limit: pageSize }, leaving skip and errorPolicy unchanged so behavior remains identical.
71-75: Simplify redundant condition.The condition
!category || category === ''is redundant since an empty string is falsy in JavaScript.♻️ Suggested simplification
const filters = useMemo(() => { - if (!category || category === '') return undefined + if (!category) return undefined const projectType = mapCategoryToProjectType(category) return projectType ? { type: projectType } : undefined }, [category])🤖 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 71 - 75, The useMemo block creating filters contains a redundant check (!category || category === ''); simplify it by checking only falsiness (e.g., if (!category) return undefined) inside the useMemo for filters. Update the useMemo that references category, mapCategoryToProjectType, and returns { type: projectType } to remove the redundant empty-string comparison and keep the same behavior.frontend/src/hooks/useSearchPage.ts (2)
190-193: JSON.stringify comparison may be expensive for large result sets.Using
JSON.stringifyto compare arrays on every render can be slow with large datasets. Consider using a reference comparison or a shallow equality check.♻️ Suggested improvement using reference check
setItems((prev) => { const newItems = graphqlItems as T[] - if (JSON.stringify(prev) === JSON.stringify(newItems)) return prev + // Reference equality is sufficient since graphqlItems is a new array on data change + if (prev === newItems || (prev.length === 0 && newItems.length === 0)) return prev return newItems })Alternatively, since Apollo returns a new array reference on each query result, checking reference equality or using a stable selector pattern would be more efficient.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useSearchPage.ts` around lines 190 - 193, The JSON.stringify comparison in the setItems updater inside useSearchPage is expensive for large result sets; replace it with a faster equality check — either simple reference equality (compare prev === graphqlItems) since Apollo normally returns new array references only when data changes, or implement a shallow equality (compare lengths and each item by ===) between prev and graphqlItems before returning prev; update the setItems updater (the block that defines newItems = graphqlItems as T[] and compares prev vs newItems) to use one of these faster checks.
83-95: Consider simplifying negated conditions (SonarCloud).The ternary expressions use negated comparisons which can be harder to read. Per SonarCloud hints, consider inverting the logic.
♻️ Suggested simplification
setSortBy((prev) => { - if (prev !== sortByParam && indexName === 'projects') { + if (prev === sortByParam) return prev + if (indexName === 'projects') { shouldResetPage = true } - return prev !== sortByParam ? sortByParam : prev + return sortByParam }) setOrder((prev) => { - if (prev !== orderParam && indexName === 'projects') { + if (prev === orderParam) return prev + if (indexName === 'projects') { shouldResetPage = true } - return prev !== orderParam ? orderParam : prev + return orderParam })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useSearchPage.ts` around lines 83 - 95, The current setSortBy and setOrder updater functions use negated comparisons which reduces readability; invert the logic so you compare equality rather than inequality: in setSortBy's updater check if prev === sortByParam (and similarly in setOrder check prev === orderParam) to skip changing state and avoid the ternary using negation, and update shouldResetPage when the equality check fails only for indexName === 'projects'; reference the setSortBy and setOrder updater callbacks, sortByParam, orderParam, indexName, and shouldResetPage when making this change.frontend/src/app/projects/dashboard/metrics/page.tsx (1)
200-218: Consider consolidating sort and order URL updates.
handleSortChangeupdates bothsortByandorderparams, andhandleOrderChangeupdates bothorderDirandorderparams. While this works, the naming is slightly inconsistent (sortByvsorderDirvsorder). More importantly, both handlers redundantly compute and set theorderparam viaorderingKey.♻️ Consider clearer parameter naming
The URL uses three related params:
sortBy,orderDir, andorder. Consider whetherorder(which contains values likescoreDesc) is necessary given thatsortByandorderDiralready encode the same information separately. This could simplify the URL structure.🤖 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 200 - 218, Both handlers redundantly compute and set the combined "order" URL param; pick a single canonical URL shape (prefer keeping sortBy and orderDir) and remove the extra "order" param updates so the URL is consistent and non-redundant. In handleSortChange and handleOrderChange use sortFieldToOrderingKey/parseOrderParam/buildGraphQLOrdering only to derive the GraphQL ordering (setOrdering) and pagination reset, then call updateUrlParams with the canonical keys (e.g., updateUrlParams({ sortBy: newSort }) in handleSortChange and updateUrlParams({ orderDir: newOrder }) in handleOrderChange), remove or stop updating the composite "order" param, and keep setSortBy/setOrder/setPagination as before; update any consumers that relied on the "order" param to reconstruct it from sortBy+orderDir if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/app/projects/dashboard/metrics/page.tsx`:
- Around line 122-128: The code builds currentFilters from both healthFilter and
levelFilter, but the UI dropdown is single-selection via computeInitialCategory;
to fix, normalize to a single active filter before constructing currentFilters:
call computeInitialCategory(healthFilter, levelFilter) (or reuse its logic) to
determine which filter wins, then only merge the corresponding mapping
(healthFiltersMapping or levelFiltersMapping) into currentFilters and ignore the
other; update places referencing healthFilter/levelFilter/state to reflect this
single-source selection so UI and filter behavior remain consistent.
---
Nitpick comments:
In `@backend/apps/api/rest/v0/project.py`:
- Around line 102-118: The ordering enum in project.py was expanded to include
contributors_count, forks_count, stars_count, and name (alongside level which
maps to level_raw), but the parametrized tests only cover created_at,
updated_at, and level; update the test suite's parametrized test (the test that
iterates ordering values) to include both ascending and descending variants for
contributors_count, forks_count, stars_count, and name so each new ordering
option is exercised; ensure the tests assert the same expectations used for
created_at/updated_at (sorting direction and field mapping), and keep the
existing level test that verifies mapping to level_raw.
In `@backend/tests/apps/api/rest/v0/project_test.py`:
- Around line 282-291: Add parametrized cases to the existing
pytest.mark.parametrize block (the parameters named "ordering" and
"expected_order") to cover the new ordering fields contributors_count,
forks_count, stars_count and name; for each new ordering string (e.g.,
"contributors_count", "-contributors_count", "forks_count", "-forks_count",
"stars_count", "-stars_count", "name", "-name") supply the corresponding
expected_order tuple that maps to the actual DB order keys used by the view
(preserving the existing tiebreaker sequence like "-stars_count",
"-forks_count", "pk" or the equivalent for name) so the test function that
consumes ordering/expected_order validates these new ordering paths as well.
In `@frontend/src/app/projects/dashboard/metrics/page.tsx`:
- Around line 200-218: Both handlers redundantly compute and set the combined
"order" URL param; pick a single canonical URL shape (prefer keeping sortBy and
orderDir) and remove the extra "order" param updates so the URL is consistent
and non-redundant. In handleSortChange and handleOrderChange use
sortFieldToOrderingKey/parseOrderParam/buildGraphQLOrdering only to derive the
GraphQL ordering (setOrdering) and pagination reset, then call updateUrlParams
with the canonical keys (e.g., updateUrlParams({ sortBy: newSort }) in
handleSortChange and updateUrlParams({ orderDir: newOrder }) in
handleOrderChange), remove or stop updating the composite "order" param, and
keep setSortBy/setOrder/setPagination as before; update any consumers that
relied on the "order" param to reconstruct it from sortBy+orderDir if needed.
In `@frontend/src/hooks/useSearchPage.ts`:
- Around line 190-193: The JSON.stringify comparison in the setItems updater
inside useSearchPage is expensive for large result sets; replace it with a
faster equality check — either simple reference equality (compare prev ===
graphqlItems) since Apollo normally returns new array references only when data
changes, or implement a shallow equality (compare lengths and each item by ===)
between prev and graphqlItems before returning prev; update the setItems updater
(the block that defines newItems = graphqlItems as T[] and compares prev vs
newItems) to use one of these faster checks.
- Around line 83-95: The current setSortBy and setOrder updater functions use
negated comparisons which reduces readability; invert the logic so you compare
equality rather than inequality: in setSortBy's updater check if prev ===
sortByParam (and similarly in setOrder check prev === orderParam) to skip
changing state and avoid the ternary using negation, and update shouldResetPage
when the equality check fails only for indexName === 'projects'; reference the
setSortBy and setOrder updater callbacks, sortByParam, orderParam, indexName,
and shouldResetPage when making this change.
In `@frontend/src/hooks/useSearchProjectsGraphQL.ts`:
- Around line 82-91: The variables object passed to useQuery includes redundant
nullish coalescing (filters ?? undefined and ordering ?? undefined); update the
variables for GetProjectsListDocument to pass filters and ordering directly
(i.e., use filters and ordering as-is) and keep pagination as { offset, limit:
pageSize }, leaving skip and errorPolicy unchanged so behavior remains
identical.
- Around line 71-75: The useMemo block creating filters contains a redundant
check (!category || category === ''); simplify it by checking only falsiness
(e.g., if (!category) return undefined) inside the useMemo for filters. Update
the useMemo that references category, mapCategoryToProjectType, and returns {
type: projectType } to remove the redundant empty-string comparison and keep the
same behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 776b29b8-71ea-49d0-8fe4-71386a6067f4
📒 Files selected for processing (6)
backend/apps/api/rest/v0/project.pybackend/tests/apps/api/rest/v0/project_test.pyfrontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsxfrontend/src/app/projects/dashboard/metrics/page.tsxfrontend/src/hooks/useSearchPage.tsfrontend/src/hooks/useSearchProjectsGraphQL.ts
HarshitVerma109
force-pushed
the
feat/reusable-search-component
branch
from
a981be6 to
f4ec2aa
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/hooks/useSearchPage.ts (1)
48-50:⚠️ Potential issue | 🟠 MajorSync
currentPagefromsearchParamstoo.
currentPageis only seeded from the URL during the first render. After that, a URL change that only updates?page=leaves the hook on the old page while the address bar shows the new one.Suggested fix
useEffect(() => { if (!searchParams) return + const pageParam = Math.max(1, Number.parseInt(searchParams.get('page') || '1', 10)) const searchQueryParam = searchParams.get('q') || '' const sortByParam = searchParams.get('sortBy') || defaultSortBy const orderParam = searchParams.get('order') || defaultOrder const categoryParam = searchParams.get('category') || defaultCategory let shouldResetPage = false + + setCurrentPage((prev) => (prev === pageParam ? prev : pageParam)) setSearchQuery((prev) => { if (prev !== searchQueryParam) { shouldResetPage = true return searchQueryParamAlso applies to: 58-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useSearchPage.ts` around lines 48 - 50, currentPage is only initialized from searchParams once; add a sync so the hook updates when the URL changes by watching searchParams and updating currentPage accordingly. Inside useSearchPage, add a useEffect that reads Number.parseInt(searchParams.get('page') || '1'), validates/clamps it (avoid NaN), and calls setCurrentPage with the parsed value; also ensure the same pattern is applied where currentPage is used/derived (the logic around setCurrentPage and any derived page state in this hook) so the hook stays in sync with searchParams updates.
♻️ Duplicate comments (3)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
133-143:⚠️ Potential issue | 🟠 MajorReset
useSearchParamsto the empty default before each test.
clearAllMocks()clears call history, but it does not undomockReturnValue(...)from the URL-param tests later in this file. That leaves later cases dependent on whichever params a previous test installed.Suggested test setup hardening
beforeEach(() => { + mockReplace.mockClear() + const { useSearchParams } = jest.requireMock('next/navigation') + ;(useSearchParams as jest.Mock).mockReturnValue(new URLSearchParams()) + ;(useQuery as unknown as jest.Mock).mockReturnValue({ data: mockHealthMetricsData, loading: false, error: null, }) }) afterEach(() => { jest.clearAllMocks() - jest.restoreAllMocks() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx` around lines 133 - 143, The tests leave a mocked useSearchParams state between tests because jest.clearAllMocks() doesn't reset mockReturnValue on useSearchParams; in beforeEach add an explicit reset of the useSearchParams mock to return the empty/default params (e.g., call (useSearchParams as jest.Mock).mockReturnValue([new URLSearchParams(), jest.fn()]) or equivalent) so each test starts with no URL params, and keep the existing afterEach jest.clearAllMocks()/jest.restoreAllMocks() cleanup; target the useSearchParams mock where it's currently manipulated in beforeEach/afterEach.frontend/src/app/projects/dashboard/metrics/page.tsx (2)
116-143:⚠️ Potential issue | 🟠 MajorDerive the initial
filtersfrom the same normalized category you render.
computeInitialCategory()chooses a single visible category, butcurrentFiltersstill merges bothhealthandlevel. A URL like?health=healthy&level=labwill therefore show one selection while querying the intersection of both filters.🤖 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 116 - 143, computeInitialCategory() picks a single visible category (health or level) but currentFilters merges both, causing the UI category and query to disagree; update the initial filters to follow the same normalization logic as computeInitialCategory (e.g., create a computeInitialFilters() that returns only healthFiltersMapping[healthFilter] if computeInitialCategory() chose `health:...`, or only levelFiltersMapping[levelFilter] if it chose `level:...`, otherwise {}), then pass that result into useState for filters (replace useState(currentFilters) with useState(computeInitialFilters())). Ensure references to healthFilter, levelFilter, healthFiltersMapping, levelFiltersMapping, computeInitialCategory, and setFilters are consistent.
200-217:⚠️ Potential issue | 🟠 MajorSort and order updates can still overwrite each other.
These handlers still run as two independent URL writes. When the shared component triggers
onSortChange()andonOrderChange()in the same interaction,handleOrderChange()reads the oldsortBy, and bothrouter.replace()calls are built from the samesearchParamssnapshot. The last write can revert the first one.Suggested consolidation
+ const updateSorting = (nextSortBy: string, nextOrder: string) => { + const normalizedSortBy = nextSortBy === 'default' ? 'score' : nextSortBy + const orderingKey = sortFieldToOrderingKey(normalizedSortBy, nextOrder) + const parsed = parseOrderParam(orderingKey) + const nextOrdering = buildGraphQLOrdering(parsed.field, parsed.direction) + + setSortBy(nextSortBy) + setOrder(nextOrder) + setOrdering(nextOrdering) + setPagination({ offset: 0, limit: PAGINATION_LIMIT }) + updateUrlParams({ + sortBy: nextSortBy, + orderDir: nextOrder, + order: orderingKey, + }) + } + const handleSortChange = (newSort: string) => { - setSortBy(newSort) - const orderingKey = sortFieldToOrderingKey(newSort === 'default' ? 'score' : newSort, order) - const parsed = parseOrderParam(orderingKey) - const newOrdering = buildGraphQLOrdering(parsed.field, parsed.direction) - setOrdering(newOrdering) - setPagination({ offset: 0, limit: PAGINATION_LIMIT }) - updateUrlParams({ sortBy: newSort, order: orderingKey }) + updateSorting(newSort, order) } const handleOrderChange = (newOrder: string) => { - setOrder(newOrder) - const orderingKey = sortFieldToOrderingKey(sortBy === 'default' ? 'score' : sortBy, newOrder) - const parsed = parseOrderParam(orderingKey) - const newOrdering = buildGraphQLOrdering(parsed.field, parsed.direction) - setOrdering(newOrdering) - setPagination({ offset: 0, limit: PAGINATION_LIMIT }) - updateUrlParams({ orderDir: newOrder, order: orderingKey }) + updateSorting(sortBy, newOrder) }🤖 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 200 - 217, Both handlers race when called together because each reads stale state and calls updateUrlParams separately; update handleSortChange and handleOrderChange to compute the new sort and order together (use the incoming newSort/newOrder values rather than reading sortBy/order state), build a single orderingKey via sortFieldToOrderingKey(...), parseOrderParam and buildGraphQLOrdering, setOrdering and setPagination, then call updateUrlParams once with both parameters (e.g., { sortBy: ..., orderDir: ..., order: orderingKey }) so a single router.replace/update applies both changes atomically; alternatively extract the shared logic into a helper that accepts (sort, order) and use it from both handlers.
🧹 Nitpick comments (3)
backend/tests/apps/owasp/api/internal/queries/project_health_metrics_test.py (1)
203-234: Consider adding a test for whitespace-only query strings.The implementation trims query strings via
query.strip(), meaning a query like" "should be treated as empty. Adding a test for this edge case would improve coverage:🧪 Optional test for whitespace-only query
`@patch`( "apps.owasp.models.project_health_metrics.ProjectHealthMetrics.get_latest_health_metrics" ) def test_project_health_metrics_whitespace_query_treated_as_empty(self, mock_get_latest_metrics): """Test project_health_metrics treats whitespace-only query as empty.""" base_qs = MagicMock() mock_get_latest_metrics.return_value = base_qs query = ProjectHealthMetricsQuery() result = query.project_health_metrics(query=" ", pagination=None, ordering=None) base_qs.filter.assert_not_called() assert result == base_qs🤖 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_health_metrics_test.py` around lines 203 - 234, Add a unit test that verifies ProjectHealthMetricsQuery.project_health_metrics treats whitespace-only query strings as empty: mock ProjectHealthMetrics.get_latest_health_metrics to return a MagicMock queryset, call project_health_metrics(query=" ", pagination=None, ordering=None), assert base_qs.filter.assert_not_called() and that the result equals the unfiltered base_qs; name the test e.g. test_project_health_metrics_whitespace_query_treated_as_empty to mirror existing tests.backend/apps/owasp/api/internal/queries/project.py (1)
75-84: Consider avoiding mutation of the pagination input.The code directly mutates
pagination.offsetandpagination.limit. While this works for the current use case, mutating input arguments can lead to unexpected side effects if the caller inspects the pagination object afterward.♻️ Alternative: use local variables instead of mutation
if pagination: if pagination.offset < 0: return [] - pagination.offset = min(pagination.offset, MAX_OFFSET) + effective_offset = min(pagination.offset, MAX_OFFSET) + pagination.offset = effective_offset if pagination.limit is not None and pagination.limit is not strawberry.UNSET: if pagination.limit <= 0: return [] - pagination.limit = min(pagination.limit, MAX_PROJECTS_LIMIT) + effective_limit = min(pagination.limit, MAX_PROJECTS_LIMIT) + pagination.limit = effective_limitNote: If mutation is intentional for strawberry_django to pick up the clamped values, document this behavior with a brief comment.
🤖 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 75 - 84, The pagination handling currently mutates pagination.offset and pagination.limit; instead compute local variables (e.g., local_offset and local_limit) from pagination.offset and pagination.limit, clamp them with MAX_OFFSET and MAX_PROJECTS_LIMIT (respect strawberry.UNSET for limit), and use those locals for validation and subsequent queries/returns rather than assigning back to pagination.offset/limit; if mutation was intentional for strawberry_django behavior, add a short comment above this block documenting that side effect.frontend/src/utils/helpers/mockApolloClient.ts (1)
3-8: Consider the implications of returning empty data for all queries.The mock link always returns
{ data: {} }, which will cause any component using Apollo queries to receive empty/undefined data. Per the relevant snippets:
testUtil.tsxwraps ALL test components with this mock client unconditionally- Components like
ProgramFormthat callclient.query()expectingdata.myPrograms.programswill getundefinedWhile tests like
ProgramForm.test.tsxexplicitly mockuseApolloClientto override this, any test that doesn't explicitly mock Apollo will silently receive empty data. This could lead to:
- Tests passing when they shouldn't (validation logic that falls through on undefined)
- Difficulty debugging why query data is empty
Consider either:
- Documenting that tests requiring Apollo data must explicitly mock
useApolloClient- Making the mock link configurable to return specific data per query
💡 Optional: Make the mock configurable
-export const createMockApolloClient = () => { - const mockLink = new ApolloLink(() => { - return new Observable((observer) => { - observer.next({ data: {} }) - observer.complete() - }) - }) +export const createMockApolloClient = (mockData: Record<string, unknown> = {}) => { + const mockLink = new ApolloLink(() => { + return new Observable((observer) => { + observer.next({ data: mockData }) + observer.complete() + }) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/utils/helpers/mockApolloClient.ts` around lines 3 - 8, The mock Apollo link currently always returns { data: {} } which causes components (e.g., ProgramForm) that call client.query() or rely on data.myPrograms.programs to receive undefined; update mockApolloClient so mockLink (the ApolloLink/Observable implementation) accepts a configurable response resolver (e.g., a map keyed by operation.operationName or a function taking the operation) and returns the matching mocked payload or a clear default (such as an error or an explicit null-with-message) instead of empty data; reference mockLink, ApolloLink, Observable and ensure testUtil.tsx can pass the resolver or per-test overrides, or alternatively document in testUtil that tests must mock useApolloClient when they need query data.
🤖 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 102-103: The condition checking cleaned_query and
len(cleaned_query) < MIN_SEARCH_QUERY_LENGTH is unreachable when
MIN_SEARCH_QUERY_LENGTH == 1; remove the dead branch or update the constant if a
larger minimum was intended—specifically, delete the if-block that returns []
(the check using cleaned_query and MIN_SEARCH_QUERY_LENGTH) or change
MIN_SEARCH_QUERY_LENGTH to the intended value (>1) and ensure any callers/tests
reflect that change; locate the check around the cleaned_query variable in the
project query code and apply the chosen fix.
---
Outside diff comments:
In `@frontend/src/hooks/useSearchPage.ts`:
- Around line 48-50: currentPage is only initialized from searchParams once; add
a sync so the hook updates when the URL changes by watching searchParams and
updating currentPage accordingly. Inside useSearchPage, add a useEffect that
reads Number.parseInt(searchParams.get('page') || '1'), validates/clamps it
(avoid NaN), and calls setCurrentPage with the parsed value; also ensure the
same pattern is applied where currentPage is used/derived (the logic around
setCurrentPage and any derived page state in this hook) so the hook stays in
sync with searchParams updates.
---
Duplicate comments:
In `@frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx`:
- Around line 133-143: The tests leave a mocked useSearchParams state between
tests because jest.clearAllMocks() doesn't reset mockReturnValue on
useSearchParams; in beforeEach add an explicit reset of the useSearchParams mock
to return the empty/default params (e.g., call (useSearchParams as
jest.Mock).mockReturnValue([new URLSearchParams(), jest.fn()]) or equivalent) so
each test starts with no URL params, and keep the existing afterEach
jest.clearAllMocks()/jest.restoreAllMocks() cleanup; target the useSearchParams
mock where it's currently manipulated in beforeEach/afterEach.
In `@frontend/src/app/projects/dashboard/metrics/page.tsx`:
- Around line 116-143: computeInitialCategory() picks a single visible category
(health or level) but currentFilters merges both, causing the UI category and
query to disagree; update the initial filters to follow the same normalization
logic as computeInitialCategory (e.g., create a computeInitialFilters() that
returns only healthFiltersMapping[healthFilter] if computeInitialCategory()
chose `health:...`, or only levelFiltersMapping[levelFilter] if it chose
`level:...`, otherwise {}), then pass that result into useState for filters
(replace useState(currentFilters) with useState(computeInitialFilters())).
Ensure references to healthFilter, levelFilter, healthFiltersMapping,
levelFiltersMapping, computeInitialCategory, and setFilters are consistent.
- Around line 200-217: Both handlers race when called together because each
reads stale state and calls updateUrlParams separately; update handleSortChange
and handleOrderChange to compute the new sort and order together (use the
incoming newSort/newOrder values rather than reading sortBy/order state), build
a single orderingKey via sortFieldToOrderingKey(...), parseOrderParam and
buildGraphQLOrdering, setOrdering and setPagination, then call updateUrlParams
once with both parameters (e.g., { sortBy: ..., orderDir: ..., order:
orderingKey }) so a single router.replace/update applies both changes
atomically; alternatively extract the shared logic into a helper that accepts
(sort, order) and use it from both handlers.
---
Nitpick comments:
In `@backend/apps/owasp/api/internal/queries/project.py`:
- Around line 75-84: The pagination handling currently mutates pagination.offset
and pagination.limit; instead compute local variables (e.g., local_offset and
local_limit) from pagination.offset and pagination.limit, clamp them with
MAX_OFFSET and MAX_PROJECTS_LIMIT (respect strawberry.UNSET for limit), and use
those locals for validation and subsequent queries/returns rather than assigning
back to pagination.offset/limit; if mutation was intentional for
strawberry_django behavior, add a short comment above this block documenting
that side effect.
In
`@backend/tests/apps/owasp/api/internal/queries/project_health_metrics_test.py`:
- Around line 203-234: Add a unit test that verifies
ProjectHealthMetricsQuery.project_health_metrics treats whitespace-only query
strings as empty: mock ProjectHealthMetrics.get_latest_health_metrics to return
a MagicMock queryset, call project_health_metrics(query=" ", pagination=None,
ordering=None), assert base_qs.filter.assert_not_called() and that the result
equals the unfiltered base_qs; name the test e.g.
test_project_health_metrics_whitespace_query_treated_as_empty to mirror existing
tests.
In `@frontend/src/utils/helpers/mockApolloClient.ts`:
- Around line 3-8: The mock Apollo link currently always returns { data: {} }
which causes components (e.g., ProgramForm) that call client.query() or rely on
data.myPrograms.programs to receive undefined; update mockApolloClient so
mockLink (the ApolloLink/Observable implementation) accepts a configurable
response resolver (e.g., a map keyed by operation.operationName or a function
taking the operation) and returns the matching mocked payload or a clear default
(such as an error or an explicit null-with-message) instead of empty data;
reference mockLink, ApolloLink, Observable and ensure testUtil.tsx can pass the
resolver or per-test overrides, or alternatively document in testUtil that tests
must mock useApolloClient when they need query data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bbcf299f-8a28-406f-8e29-eef7300cf751
⛔ 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 (30)
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/tests/apps/api/rest/v0/project_test.pybackend/tests/apps/owasp/api/internal/filters/project_test.pybackend/tests/apps/owasp/api/internal/queries/project_health_metrics_test.pybackend/tests/apps/owasp/api/internal/queries/project_test.pyfrontend/.env.e2e.examplefrontend/.env.examplefrontend/__tests__/unit/components/SearchWithFilters.test.tsxfrontend/__tests__/unit/pages/Contribute.test.tsxfrontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsxfrontend/src/app/projects/dashboard/metrics/page.tsxfrontend/src/app/projects/page.tsxfrontend/src/components/SearchPageLayout.tsxfrontend/src/components/SearchWithFilters.tsxfrontend/src/components/SortBy.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/searchWithFilters.tsfrontend/src/utils/backendConfig.tsfrontend/src/utils/helpers/mockApolloClient.tsfrontend/src/utils/sortingOptions.tsfrontend/src/wrappers/testUtil.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- frontend/tests/unit/components/SearchWithFilters.test.tsx
- frontend/src/app/projects/page.tsx
- frontend/src/server/queries/projectsHealthDashboardQueries.ts
- frontend/src/wrappers/testUtil.tsx
- backend/apps/owasp/api/internal/ordering/project.py
- frontend/.env.e2e.example
- frontend/src/utils/backendConfig.ts
- frontend/src/server/queries/projectQueries.ts
- backend/apps/owasp/api/internal/queries/project_health_metrics.py
- backend/tests/apps/api/rest/v0/project_test.py
- backend/tests/apps/owasp/api/internal/filters/project_test.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/apps/owasp/api/internal/queries/project.py (1)
69-145: Extract the shared queryset builder for list/count parity.
projects,search_projects, andsearch_projects_countnow each own parts of the sameis_active/query/filter setup. Pulling that into a private helper would make it much harder for the list and count endpoints to drift the next time search semantics change.🤖 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 69 - 145, Extract the common "active projects + optional name filter + applied filters" logic into a private helper function (e.g. _build_project_base_queryset(query: str = "", filters: ProjectFilter | None = None) -> QuerySet) and use it from projects, search_projects, and search_projects_count so list and count share the same base query; leave ordering and pagination handling in the calling functions (projects and search_projects) but replace their Project.objects.filter(is_active=True) and duplicate name/filter application with a call to _build_project_base_queryset(cleaned_query, filters), and ensure search_projects_count uses the same helper before calling .count().
🤖 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 74-82: The current pagination check only enforces
MAX_PROJECTS_LIMIT when the caller supplies pagination.limit; update the logic
in the projects (and searchProjects) query handlers to ensure an absent or
strawberry.UNSET limit is defaulted to MAX_PROJECTS_LIMIT and then clamped (also
ensure offset is clamped by MAX_OFFSET); specifically, when pagination is
provided but pagination.limit is None or strawberry.UNSET, set pagination.limit
= MAX_PROJECTS_LIMIT, and if pagination is missing entirely, ensure the code
supplies a pagination object or local limit variable initialized to
MAX_PROJECTS_LIMIT before building the queryset; apply the same change to the
analogous block referenced around lines 113-121.
---
Nitpick comments:
In `@backend/apps/owasp/api/internal/queries/project.py`:
- Around line 69-145: Extract the common "active projects + optional name filter
+ applied filters" logic into a private helper function (e.g.
_build_project_base_queryset(query: str = "", filters: ProjectFilter | None =
None) -> QuerySet) and use it from projects, search_projects, and
search_projects_count so list and count share the same base query; leave
ordering and pagination handling in the calling functions (projects and
search_projects) but replace their Project.objects.filter(is_active=True) and
duplicate name/filter application with a call to
_build_project_base_queryset(cleaned_query, filters), and ensure
search_projects_count uses the same helper before calling .count().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a020327e-7e8e-4c2c-ad51-4a1a354592f1
📒 Files selected for processing (2)
.pre-commit-config.yamlbackend/apps/owasp/api/internal/queries/project.py
|
Contributor
There was a problem hiding this comment.
2 issues 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/apps/owasp/api/internal/queries/project.py">
<violation number="1" location="backend/apps/owasp/api/internal/queries/project.py:84">
P2: Forcing `pagination.limit` to 1000 when omitted overrides Strawberry-Django default pagination behavior and can cause unexpected over-fetching.</violation>
</file>
<file name=".pre-commit-config.yaml">
<violation number="1">
P2: Removing `language_version` from the node-based markdownlint hook makes its runtime implicit and environment-dependent, risking CI/local lint drift.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
@arkid15r , please review it and let me know if any changes are required. |
Contributor
Author
|
Hi @kasya, I’ve implemented the new hierarchical category structure with up to three levels of nesting as requested. I’ve recorded a quick demo of it in action. Could you take a look? If everything looks good, I’ll go ahead and push the code. Thanks! Screen.Recording.2026-03-09.000246.mp4 |
Contributor
Author
|
Hi @kasya, any feedback on the fix above? |
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
Resolves #4086
Added a reusable
SearchWithFilterscomponent with category filter and sorting dropdowns, shared across the Projects page and Project Health Dashboard.Checklist
make check-testlocally: all warnings addressed, tests passed