Feature: Filter and Sort Support for Members Page#4306
Open
mrkeshav-05 wants to merge 15 commits intoOWASP:mainfrom
Open
Feature: Filter and Sort Support for Members Page#4306mrkeshav-05 wants to merge 15 commits intoOWASP:mainfrom
mrkeshav-05 wants to merge 15 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 user indexing fields and affinity flags, propagates faceting when configuring Algolia replicas for users, exposes new UserIndexMixin properties, integrates MembersFilter and SortBy into the members page with hook/type updates, and expands unit and accessibility tests across frontend and backend. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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.
3 issues found across 18 files
Confidence score: 3/5
- There is moderate regression risk:
backend/apps/github/models/mixins/user.pyadds affinity index properties that do per-user.exists()checks, which can amplify queries during bulk user reindexing and impact performance. - The highest-impact issue is backend-side (severity 6/10, high confidence), while the other findings are test-quality gaps rather than direct runtime breakage, so this looks fixable but not risk-free.
frontend/__tests__/unit/components/MembersFilter.test.tsxandbackend/tests/apps/github/models/mixins/user_test.pyhave assertion gaps (dropdown not actually opened, missing explicit false-state coverage), which can let behavior regressions slip through.- Pay close attention to
backend/apps/github/models/mixins/user.py,frontend/__tests__/unit/components/MembersFilter.test.tsx, andbackend/tests/apps/github/models/mixins/user_test.py- query amplification risk and incomplete tests may hide regressions.
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/github/models/mixins/user.py">
<violation number="1" location="backend/apps/github/models/mixins/user.py:355">
P2: New affinity index properties perform per-user `.exists()` ORM checks, causing query amplification during bulk user reindexing.</violation>
</file>
<file name="backend/tests/apps/github/models/mixins/user_test.py">
<violation number="1" location="backend/tests/apps/github/models/mixins/user_test.py:202">
P2: New boolean index tests miss explicit false-state coverage, allowing incorrect truthy-only implementations to pass.</violation>
</file>
<file name="frontend/__tests__/unit/components/MembersFilter.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/MembersFilter.test.tsx:60">
P2: Test name claims to verify dropdown opened behavior but never opens the dropdown</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: 6
🧹 Nitpick comments (3)
backend/tests/apps/github/index/registry/user_test.py (1)
40-42: Strengthen this test by asserting thebase_settingsargument too.Line 40-42 validates index name + replicas dict, but misses the third positional argument that carries faceting settings.
✅ Suggested assertion upgrade
assert mock_configure_replicas.call_count == 1 assert mock_configure_replicas.call_args[0][0] == "users" assert isinstance(mock_configure_replicas.call_args[0][1], dict) + assert isinstance(mock_configure_replicas.call_args[0][2], dict) + assert "attributesForFaceting" in mock_configure_replicas.call_args[0][2]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/github/index/registry/user_test.py` around lines 40 - 42, The test currently asserts the first two positional args passed to mock_configure_replicas (index name and replicas dict) but omits validating the faceting/settings payload; modify the assertion in the test (around mock_configure_replicas.call_args checks) to also assert mock_configure_replicas.call_args[0][2] (the base_settings argument) matches the expected settings structure (either equality to the expected dict of faceting fields or at least contains required keys/values), ensuring the third positional argument is validated alongside the index name and replicas dict.backend/apps/github/models/mixins/user.py (1)
353-376: Extract shared repository-affinity lookup to avoid duplicated query logic.Line 355-358 and Line 372-375 repeat the same
RepositoryContributor.exists()shape with only the prefix changed; this is easy to centralize and keep consistent.♻️ Suggested refactor
class UserIndexMixin: @@ + def _has_repo_prefix_affinity(self, prefix: str) -> bool: + from apps.github.models.repository_contributor import RepositoryContributor + + return RepositoryContributor.objects.filter( + user=self, + repository__key__startswith=prefix, + ).exists() + @@ def idx_has_chapter_affinity(self) -> bool: @@ - from apps.github.models.repository_contributor import RepositoryContributor - - has_contributions = RepositoryContributor.objects.filter( - user=self, - repository__key__startswith="www-chapter-", - ).exists() + has_contributions = self._has_repo_prefix_affinity("www-chapter-") return has_contributions or self.chapters.exists() @@ def idx_has_committee_affinity(self) -> bool: @@ - from apps.github.models.repository_contributor import RepositoryContributor from apps.owasp.models.committee import Committee - has_contributions = RepositoryContributor.objects.filter( - user=self, - repository__key__startswith="www-committee-", - ).exists() + has_contributions = self._has_repo_prefix_affinity("www-committee-") return has_contributions or self._get_led_entities(Committee).exists()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/github/models/mixins/user.py` around lines 353 - 376, Duplicate RepositoryContributor.exists() queries in the properties above and idx_has_committee_affinity should be refactored into a single helper that accepts the repository key prefix; add a private method on the model (e.g. _has_repository_affinity(self, prefix: str) -> bool) that imports RepositoryContributor, performs the .filter(user=self, repository__key__startswith=prefix).exists() check, and then update the two properties (the chapter-affinity property above and idx_has_committee_affinity) to call this helper and combine its result with the existing checks (self.chapters.exists() or self._get_led_entities(Committee).exists()).backend/apps/github/index/registry/user.py (1)
93-107: Avoid config drift by reusing shared facet constants andindex_name.
attributesForFacetingand"users"are duplicated here and insettings. If one changes later, primary/replica filter behavior can diverge silently. Prefer deriving both from shared class-level values.♻️ Refactor sketch
class UserIndex(IndexBase): """User index.""" index_name = "users" + facet_attributes = [ + "idx_key", + "idx_name", + "idx_title", + "idx_is_owasp_staff", + "idx_owasp_board_member", + "idx_owasp_gsoc_mentor", + "idx_has_project_affinity", + "idx_has_chapter_affinity", + "idx_has_committee_affinity", + ] settings = { - "attributesForFaceting": [ - "idx_key", - "idx_name", - "idx_title", - "idx_is_owasp_staff", - "idx_owasp_board_member", - "idx_owasp_gsoc_mentor", - "idx_has_project_affinity", - "idx_has_chapter_affinity", - "idx_has_committee_affinity", - ], + "attributesForFaceting": facet_attributes, ... } `@staticmethod` def configure_replicas() -> None: # type: ignore[override] ... - base_settings = { - "attributesForFaceting": [ - "idx_key", - "idx_name", - "idx_title", - "idx_is_owasp_staff", - "idx_owasp_board_member", - "idx_owasp_gsoc_mentor", - "idx_has_project_affinity", - "idx_has_chapter_affinity", - "idx_has_committee_affinity", - ] - } + base_settings = {"attributesForFaceting": UserIndex.facet_attributes} - IndexBase.configure_replicas("users", replicas, base_settings) + IndexBase.configure_replicas(UserIndex.index_name, replicas, base_settings)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/github/index/registry/user.py` around lines 93 - 107, The attributesForFaceting list and the literal "users" are duplicated and risk config drift; update this block to reuse the shared class-level constants instead of hardcoding: reference the shared facet constant (e.g., FACET_ATTRIBUTES or whatever shared name is defined in the settings/IndexBase) for base_settings["attributesForFaceting"] and use the shared index name constant (e.g., INDEX_NAME or a class property) when calling IndexBase.configure_replicas("users", ...) so both primary and replica configuration derive from the same constants (ensure you import or access the class-level symbols and remove the duplicated literal list and string).
🤖 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/github/index/registry/user.py`:
- Around line 35-40: The new affinity index entries (idx_has_project_affinity,
idx_has_chapter_affinity, idx_has_committee_affinity) cause per-user relation
lookups inside get_entities (see the boolean property accessors in the User
mixin) which will amplify DB work during reindex; fix by removing these
per-record property accesses from the index path and instead compute affinities
in bulk: alter the registry index setup to stop invoking the per-user properties
during indexing (remove or defer adding idx_has_* here), and implement a batched
affinity calculation using annotate/Exists or a single query that prefetches
related project/chapter/committee memberships and sets boolean flags for all
users before indexing (update the get_entities consumer to accept precomputed
affinity annotations or a bulk helper that computes and attaches
has_project_affinity/has_chapter_affinity/has_committee_affinity to the
queryset).
In `@frontend/__tests__/unit/pages/Users.test.tsx`:
- Around line 98-103: The test for UsersPage should wait for the async UI to
finish by awaiting the presence of the loaded marker 'members-filter' before
asserting the absence of 'members-sidebar'; replace the current assertion inside
waitFor with await screen.findByTestId('members-filter') (or waitFor(() =>
expect(...toBeInTheDocument()))) to ensure loading completed, then call
expect(screen.queryByTestId('members-sidebar')).not.toBeInTheDocument() outside
the waitFor; update the test titled "does not render a standalone sidebar —
filters are inline" accordingly.
In `@frontend/src/app/members/page.tsx`:
- Around line 15-38: The member-type state is currently a single string
(selectedMemberType) which prevents combining toggles; change selectedMemberType
to a string[] (and setSelectedMemberType accordingly) so the UI can add/remove
values instead of replacing them, update the toggle handlers to toggle
membership in that array, and then update the facetFilters useMemo to check
includes (e.g., selectedMemberType.includes('staff')) and push the corresponding
filters (idx_is_owasp_staff:true, idx_owasp_board_member:true,
idx_owasp_gsoc_mentor:true) when present; also treat an empty array or a special
'all' value as no member-type filters so behavior matches the original 'all'
default.
In `@frontend/src/components/MembersFilter.tsx`:
- Around line 122-129: The handler in MembersFilter.tsx currently clears the
opposite filter (calls onMemberTypeChange('all') when option.type === 'affinity'
and vice versa), preventing simultaneous affinity + member-type selections;
modify the logic in the change handler so each branch only updates its own
filter (call onAffinityChange(option.key) when option.type === 'affinity' and
onMemberTypeChange(option.key) when option.type === 'role') and do not force the
other to 'all'—preserve existing handling for option.key === 'all' to allow
explicit clearing but remove the automatic clearing of the other filter so both
dimensions can be active together.
In `@frontend/src/types/user.ts`:
- Around line 23-24: The User type is missing the committee affinity field used
by new member filters; add an optional boolean property named
hasCommitteeAffinity to the User shape (where hasChapterAffinity and
hasProjectAffinity are defined) and also add the same hasCommitteeAffinity?:
boolean to the other User-like type declared later (the one on lines
corresponding to the duplicate block near hasChapterAffinity/hasProjectAffinity)
so both definitions stay in sync with the filtering surface.
In `@frontend/src/utils/sortingOptions.ts`:
- Around line 17-22: The sortOptionsUser array currently only exposes four
options and is missing Date Joined, Last Updated, and Ranking Score; update the
sortOptionsUser export to include additional objects for these three sorts
(e.g., labels "Date Joined", "Last Updated", "Ranking Score" with unique keys
such as "date_joined", "last_updated", "ranking_score") so the Members page can
present and use those sort choices; keep the array order and formatting
consistent with existing entries in sortOptionsUser.
---
Nitpick comments:
In `@backend/apps/github/index/registry/user.py`:
- Around line 93-107: The attributesForFaceting list and the literal "users" are
duplicated and risk config drift; update this block to reuse the shared
class-level constants instead of hardcoding: reference the shared facet constant
(e.g., FACET_ATTRIBUTES or whatever shared name is defined in the
settings/IndexBase) for base_settings["attributesForFaceting"] and use the
shared index name constant (e.g., INDEX_NAME or a class property) when calling
IndexBase.configure_replicas("users", ...) so both primary and replica
configuration derive from the same constants (ensure you import or access the
class-level symbols and remove the duplicated literal list and string).
In `@backend/apps/github/models/mixins/user.py`:
- Around line 353-376: Duplicate RepositoryContributor.exists() queries in the
properties above and idx_has_committee_affinity should be refactored into a
single helper that accepts the repository key prefix; add a private method on
the model (e.g. _has_repository_affinity(self, prefix: str) -> bool) that
imports RepositoryContributor, performs the .filter(user=self,
repository__key__startswith=prefix).exists() check, and then update the two
properties (the chapter-affinity property above and idx_has_committee_affinity)
to call this helper and combine its result with the existing checks
(self.chapters.exists() or self._get_led_entities(Committee).exists()).
In `@backend/tests/apps/github/index/registry/user_test.py`:
- Around line 40-42: The test currently asserts the first two positional args
passed to mock_configure_replicas (index name and replicas dict) but omits
validating the faceting/settings payload; modify the assertion in the test
(around mock_configure_replicas.call_args checks) to also assert
mock_configure_replicas.call_args[0][2] (the base_settings argument) matches the
expected settings structure (either equality to the expected dict of faceting
fields or at least contains required keys/values), ensuring the third positional
argument is validated alongside the index name and replicas dict.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6674fd6c-595b-4ef1-9bc0-7818975a5cb0
📒 Files selected for processing (18)
backend/apps/common/index.pybackend/apps/common/management/commands/algolia_update_replicas.pybackend/apps/core/utils/index.pybackend/apps/github/index/registry/user.pybackend/apps/github/models/mixins/user.pybackend/tests/apps/common/management/commands/algolia_update_replicas_test.pybackend/tests/apps/core/utils/match_test.pybackend/tests/apps/github/index/registry/user_test.pybackend/tests/apps/github/models/mixins/user_test.pyfrontend/__tests__/unit/components/MembersFilter.test.tsxfrontend/__tests__/unit/pages/Users.test.tsxfrontend/src/app/members/page.tsxfrontend/src/components/MembersFilter.tsxfrontend/src/components/SearchPageLayout.tsxfrontend/src/hooks/useSearchPage.tsfrontend/src/server/fetchAlgoliaData.tsfrontend/src/types/user.tsfrontend/src/utils/sortingOptions.ts
💤 Files with no reviewable changes (1)
- frontend/src/components/SearchPageLayout.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/components/MembersFilter.tsx (1)
129-145:⚠️ Potential issue | 🟠 MajorSelecting one filter group still clears the other, so combined filters cannot stay active.
handleChangeresets the opposite state (onMemberTypeChange('all')/onAffinityChange('all')), which blocks affinity + role combinations.💡 Minimal behavior fix
if (option.type === 'affinity') { onAffinityChange(option.key) - if (selectedMemberType !== 'all') onMemberTypeChange('all') + return } if (option.type === 'role') { onMemberTypeChange(option.key) - if (selectedAffinity !== 'all') onAffinityChange('all') + return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/MembersFilter.tsx` around lines 129 - 145, handleChange currently forces the opposite filter to 'all' when you pick an affinity or role, preventing combined affinity+role filtering; update handleChange (the function that finds option via combinedOptions.find and calls onAffinityChange/onMemberTypeChange) so that selecting a specific affinity only calls onAffinityChange(option.key) and selecting a specific role only calls onMemberTypeChange(option.key) — remove the lines that call onMemberTypeChange('all') inside the affinity branch and onAffinityChange('all') inside the role branch; keep the existing 'all' branch behavior that clears both when value === 'all'.
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/MembersFilter.test.tsx (1)
46-58: Callback isolation tests miss the active-opposite-filter case.The current assertions use default props (
'all'), so they don’t catch regressions when the other filter is already active. Add cases like “selectedMemberType='staff' then choose affinity” and “selectedAffinity='projects' then choose role” to verify the other callback is not force-reset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/MembersFilter.test.tsx` around lines 46 - 58, Tests for MembersFilter currently only use default props and miss cases where the opposite filter is already active; add two isolated tests that set selectedMemberType (e.g., 'staff') and then change the affinity select to ensure onMemberTypeChange is not called, and symmetrically set selectedAffinity (e.g., 'projects') and then change the member type select to ensure onAffinityChange is not called. Locate the component in the tests (MembersFilter) and the callbacks (onMemberTypeChange, onAffinityChange) and modify the test setup to pass selectedMemberType='staff' for the affinity-change test and selectedAffinity='projects' for the member-type-change test, then fireEvent.change the appropriate hidden combobox and assert the opposite callback was not called.
🤖 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/github/models/mixins/user.py`:
- Around line 346-406: The three affinity properties (idx_has_chapter_affinity,
idx_has_committee_affinity, idx_has_project_affinity) cause heavy per-user DB
queries during bulk reindexing; change them to first read annotated boolean
fields (e.g., expect annotations like annotated_idx_has_chapter_affinity,
annotated_idx_has_committee_affinity, annotated_idx_has_project_affinity
provided by the user queryset in backend/apps/github/index/registry/user.py) and
return those when present, falling back to the current exists() checks only if
the annotation is missing; for idx_has_committee_affinity avoid calling
_get_led_entities unnecessarily by preferring an annotation for committee
leadership and only invoking _get_led_entities(Committee) as a last-resort
fallback.
---
Duplicate comments:
In `@frontend/src/components/MembersFilter.tsx`:
- Around line 129-145: handleChange currently forces the opposite filter to
'all' when you pick an affinity or role, preventing combined affinity+role
filtering; update handleChange (the function that finds option via
combinedOptions.find and calls onAffinityChange/onMemberTypeChange) so that
selecting a specific affinity only calls onAffinityChange(option.key) and
selecting a specific role only calls onMemberTypeChange(option.key) — remove the
lines that call onMemberTypeChange('all') inside the affinity branch and
onAffinityChange('all') inside the role branch; keep the existing 'all' branch
behavior that clears both when value === 'all'.
---
Nitpick comments:
In `@frontend/__tests__/unit/components/MembersFilter.test.tsx`:
- Around line 46-58: Tests for MembersFilter currently only use default props
and miss cases where the opposite filter is already active; add two isolated
tests that set selectedMemberType (e.g., 'staff') and then change the affinity
select to ensure onMemberTypeChange is not called, and symmetrically set
selectedAffinity (e.g., 'projects') and then change the member type select to
ensure onAffinityChange is not called. Locate the component in the tests
(MembersFilter) and the callbacks (onMemberTypeChange, onAffinityChange) and
modify the test setup to pass selectedMemberType='staff' for the affinity-change
test and selectedAffinity='projects' for the member-type-change test, then
fireEvent.change the appropriate hidden combobox and assert the opposite
callback was not called.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd5e77a8-4110-4447-9bf1-0511aaff4733
📒 Files selected for processing (6)
backend/apps/github/models/mixins/user.pybackend/tests/apps/github/models/mixins/user_test.pyfrontend/__tests__/unit/components/MembersFilter.test.tsxfrontend/__tests__/unit/pages/Users.test.tsxfrontend/src/components/MembersFilter.tsxfrontend/src/types/user.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/tests/unit/pages/Users.test.tsx
- backend/tests/apps/github/models/mixins/user_test.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/components/MembersFilter.tsx (1)
56-68: Consider derivingcombinedOptionsfrom existing constants to reduce duplication.
combinedOptionsduplicates the key/label data fromAFFINITY_OPTIONSandMEMBER_TYPE_OPTIONS. If these constants change,combinedOptionscould fall out of sync.♻️ Suggested refactor to derive combinedOptions
- const combinedOptions = [ - { key: 'all', label: 'All Filters', type: 'all' as const }, - { key: 'projects', label: 'Projects', type: 'affinity' as const }, - { key: 'chapters', label: 'Chapters', type: 'affinity' as const }, - { key: 'committees', label: 'Committees', type: 'affinity' as const }, - { key: 'staff', label: 'OWASP Staff', type: 'role' as const }, - { key: 'board', label: 'Board Member', type: 'role' as const }, - { key: 'gsoc', label: 'GSoC Mentor', type: 'role' as const }, - ] + const combinedOptions = [ + { key: 'all', label: 'All Filters', type: 'all' as const }, + ...AFFINITY_OPTIONS.filter((o) => o.key !== 'all').map((o) => ({ + ...o, + type: 'affinity' as const, + })), + ...MEMBER_TYPE_OPTIONS.filter((o) => o.key !== 'all').map((o) => ({ + ...o, + type: 'role' as const, + })), + ]Also applies to: 96-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/MembersFilter.tsx` around lines 56 - 68, combinedOptions currently duplicates entries from AFFINITY_OPTIONS and MEMBER_TYPE_OPTIONS; change combinedOptions to be derived programmatically by mapping AFFINITY_OPTIONS and MEMBER_TYPE_OPTIONS into the combined shape instead of hardcoding values so updates to those constants stay in sync. Locate the combinedOptions usage in MembersFilter (reference symbol combinedOptions) and replace the literal array with a concatenation like AFFINITY_OPTIONS.map(...) and MEMBER_TYPE_OPTIONS.map(...) (preserving keys and labels) or a small helper that transforms those constants into the combined format, ensuring any existing consumers still receive the same { key, label } objects.backend/apps/github/index/registry/user.py (1)
93-105: DuplicatedattributesForFacetinglist violates DRY.The
attributesForFacetinglist inbase_settingsis identical to the one insettings(lines 44-54). If attributes are added or removed, both locations must be updated, risking inconsistency.Consider extracting the faceting attributes to a class-level constant or referencing
UserIndex.settings["attributesForFaceting"]directly.♻️ Proposed refactor
`@staticmethod` def configure_replicas() -> None: # type: ignore[override] """Configure the settings for user replicas.""" replicas = { "contributions_count_asc": ["asc(idx_contributions_count)"], "contributions_count_desc": ["desc(idx_contributions_count)"], "followers_count_asc": ["asc(idx_followers_count)"], "followers_count_desc": ["desc(idx_followers_count)"], "public_repositories_count_asc": ["asc(idx_public_repositories_count)"], "public_repositories_count_desc": ["desc(idx_public_repositories_count)"], } - base_settings = { - "attributesForFaceting": [ - "idx_key", - "idx_name", - "idx_title", - "idx_is_owasp_staff", - "idx_owasp_board_member", - "idx_owasp_gsoc_mentor", - "idx_has_project_affinity", - "idx_has_chapter_affinity", - "idx_has_committee_affinity", - ] - } + base_settings = { + "attributesForFaceting": UserIndex.settings["attributesForFaceting"] + } IndexBase.configure_replicas("users", replicas, base_settings)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/github/index/registry/user.py` around lines 93 - 105, The duplicated attributesForFaceting list should be centralized: extract the faceting attribute array from the duplicated locations by defining a single constant or class-level attribute (e.g., add FACETING_ATTRIBUTES on UserIndex or a module-level FACETING_ATTRIBUTES) and then use that single symbol in both UserIndex.settings and base_settings instead of repeating the literal list; update references to use FACETING_ATTRIBUTES in the base_settings variable and in the UserIndex.settings definition to remove the duplicate and keep them in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/MembersFilter.tsx`:
- Around line 106-114: getCurrentKey may return undefined when selectedAffinity
=== 'all' and selectedMemberTypes is an empty array; update getCurrentKey to
check for an empty selectedMemberTypes and return a safe fallback (e.g., 'all'
or '') instead of selectedMemberTypes[0]. Locate the getCurrentKey function and
add a guard like if (selectedAffinity === 'all' && selectedMemberTypes.length
=== 0) return 'all' (or another explicit default used by the Select component)
so that selectedKeys never receives undefined.
---
Nitpick comments:
In `@backend/apps/github/index/registry/user.py`:
- Around line 93-105: The duplicated attributesForFaceting list should be
centralized: extract the faceting attribute array from the duplicated locations
by defining a single constant or class-level attribute (e.g., add
FACETING_ATTRIBUTES on UserIndex or a module-level FACETING_ATTRIBUTES) and then
use that single symbol in both UserIndex.settings and base_settings instead of
repeating the literal list; update references to use FACETING_ATTRIBUTES in the
base_settings variable and in the UserIndex.settings definition to remove the
duplicate and keep them in sync.
In `@frontend/src/components/MembersFilter.tsx`:
- Around line 56-68: combinedOptions currently duplicates entries from
AFFINITY_OPTIONS and MEMBER_TYPE_OPTIONS; change combinedOptions to be derived
programmatically by mapping AFFINITY_OPTIONS and MEMBER_TYPE_OPTIONS into the
combined shape instead of hardcoding values so updates to those constants stay
in sync. Locate the combinedOptions usage in MembersFilter (reference symbol
combinedOptions) and replace the literal array with a concatenation like
AFFINITY_OPTIONS.map(...) and MEMBER_TYPE_OPTIONS.map(...) (preserving keys and
labels) or a small helper that transforms those constants into the combined
format, ensuring any existing consumers still receive the same { key, label }
objects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47a330a4-c780-4fb2-a9c8-2c7d6ecb7cb8
📒 Files selected for processing (5)
backend/apps/github/index/registry/user.pybackend/apps/github/models/mixins/user.pycspell/custom-dict.txtfrontend/src/app/members/page.tsxfrontend/src/components/MembersFilter.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/members/page.tsx
Contributor
There was a problem hiding this comment.
2 issues 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="frontend/src/components/MembersFilter.tsx">
<violation number="1" location="frontend/src/components/MembersFilter.tsx:111">
P2: Array-based member type state is treated as a single value, producing `undefined`/incorrect active state for empty arrays and collapsing multi-value state.</violation>
</file>
<file name="frontend/src/app/members/page.tsx">
<violation number="1" location="frontend/src/app/members/page.tsx:31">
P1: The filtering logic uses `else if` chains for multiple member type selections, which causes only the first matching filter to be applied even though the state now supports multiple selected member types (array)</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
mrkeshav-05
force-pushed
the
feat/member-filter-sort
branch
from
8b80bb7 to
a0c5e8b
Compare
mrkeshav-05
force-pushed
the
feat/member-filter-sort
branch
from
a0c5e8b to
4bc5c2a
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/src/components/MembersFilter.tsx (1)
106-156:⚠️ Potential issue | 🟠 MajorThis still blocks combined member filters.
selectedMemberTypesis an array, butgetCurrentKey(),selectedKeys={[currentKey]}, andhandleChange()reduce the control to one active key and reset the opposite group. Users still can't keep combinations likeProjects + OWASP Staffactive together, and they can never hold more than one member-type filter at once, so the page never reaches the combinedfacetFiltersflow it is prepared to send.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/MembersFilter.tsx` around lines 106 - 156, getCurrentKey/currentKey and handleChange collapse multiple filters into a single key and reset the opposite group; instead compute selectedKeys from selectedAffinity and selectedMemberTypes (derive an array containing affinity if not 'all' plus all member-type keys except 'all'), pass that array to the Select selectedKeys prop, and change handleChange to toggle the clicked option in its group rather than replacing the whole group (use combinedOptions to find the option, call onAffinityChange with a new affinity state that allows multiple affinities or 'all' as appropriate, and call onMemberTypesChange with a new array that adds/removes the role key and does not force-reset the affinity). Ensure getTestId remains consistent for tests but remove logic that assumes a single currentKey so the UI and facetFilters flow can represent combined selections.
🧹 Nitpick comments (3)
frontend/__tests__/a11y/pages/Users.a11y.test.tsx (1)
40-43: Arbitrary timeout may cause test flakiness.Using
setTimeout(resolve, 100)to wait for async updates is a test anti-pattern — it can lead to intermittent failures if the component takes longer under CI load, or wastes time if updates complete faster.Consider waiting for a deterministic DOM condition that indicates the Select component has finished rendering, such as:
await waitFor(() => { expect(screen.getByRole('button', { name: /relevancy/i })).toBeInTheDocument() })If HeroUI's Select has internal async behavior that doesn't expose a stable waiting target, this workaround may be acceptable, but document why it's necessary.
♻️ Suggested approach using deterministic waiting
- // Wait for HeroUI Select component to finish async updates - await act(async () => { - await new Promise((resolve) => setTimeout(resolve, 100)) - }) + // Wait for Select/SortBy component to finish rendering + await waitFor(() => { + expect(screen.getByRole('button', { name: /sort/i })).toBeEnabled() + })Adjust the selector to match the actual rendered element from your SortBy or MembersFilter component.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/a11y/pages/Users.a11y.test.tsx` around lines 40 - 43, The test uses an arbitrary 100ms timeout inside act to wait for the HeroUI Select async updates; replace that with a deterministic wait (use waitFor and a stable DOM assertion) instead of setTimeout — e.g. await waitFor(() => expect(screen.getByRole('button', { name: /relevancy/i })).toBeInTheDocument()) — targeting the visible element rendered by your SortBy/MembersFilter Select so the test waits only until the Select has finished rendering; if no stable target exists, keep the minimal timeout but add a comment explaining why and reference the Select component.frontend/__tests__/unit/components/MembersFilter.test.tsx (1)
32-58: Add a regression case with the other filter group already selected.All of these interaction tests start from the cleared
allstate, so they won't catch the current behavior where choosing an affinity clears an existing member-type filter, or vice versa. One case that begins with an active member type or active affinity would lock down the intended combined-filter flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/MembersFilter.test.tsx` around lines 32 - 58, Add regression tests that begin with one filter already active to ensure choosing the other filter doesn't clear it: render MembersFilter with defaultProps, first fireEvent.change on the hidden select to set an initial member type (e.g., value 'staff') and then fireEvent.change again to set an affinity (e.g., value 'projects'); assert onAffinityChange was called with 'projects' and that onMemberTypesChange was not cleared (still called/retained as ['staff'] or not called again depending on expected behavior). Mirror this with the reverse: pre-select an affinity (e.g., 'chapters') then select a member type (e.g., 'board') and assert onMemberTypesChange receives ['board'] and onAffinityChange is not cleared; use the existing hiddenSelect lookup and the defaultProps spies (onAffinityChange, onMemberTypesChange) to locate and verify behavior.backend/tests/apps/github/models/mixins/user_test.py (1)
202-224: Please cover the affinity flags in this suite too.
UserIndexMixinnow also exposesidx_has_chapter_affinity,idx_has_committee_affinity, andidx_has_project_affinity, each with an annotated fast-path and a query fallback. This block only exercises the OWASP-profile booleans, so regressions in the new members-filter flags would slip until reindexing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/github/models/mixins/user_test.py` around lines 202 - 224, Add tests exercising the three new affinity flags on UserIndexMixin: idx_has_chapter_affinity, idx_has_committee_affinity, and idx_has_project_affinity. For each flag add a "with_profile" test that simulates the annotated fast-path (mirror existing pattern like setting user_index_mixin_instance.owasp_profile = MagicMock(...)) so the mixin returns True, and a "without_profile" test that constructs a plain UserIndexMixin() and asserts the flag is False; name them e.g. test_idx_has_chapter_affinity_with_profile / _without_profile, test_idx_has_committee_affinity_with_profile / _without_profile, and test_idx_has_project_affinity_with_profile / _without_profile to match the existing style and ensure both the annotated fast-path and the query-fallback/absence case are covered.
🤖 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/components/MembersFilter.tsx`:
- Around line 106-156: getCurrentKey/currentKey and handleChange collapse
multiple filters into a single key and reset the opposite group; instead compute
selectedKeys from selectedAffinity and selectedMemberTypes (derive an array
containing affinity if not 'all' plus all member-type keys except 'all'), pass
that array to the Select selectedKeys prop, and change handleChange to toggle
the clicked option in its group rather than replacing the whole group (use
combinedOptions to find the option, call onAffinityChange with a new affinity
state that allows multiple affinities or 'all' as appropriate, and call
onMemberTypesChange with a new array that adds/removes the role key and does not
force-reset the affinity). Ensure getTestId remains consistent for tests but
remove logic that assumes a single currentKey so the UI and facetFilters flow
can represent combined selections.
---
Nitpick comments:
In `@backend/tests/apps/github/models/mixins/user_test.py`:
- Around line 202-224: Add tests exercising the three new affinity flags on
UserIndexMixin: idx_has_chapter_affinity, idx_has_committee_affinity, and
idx_has_project_affinity. For each flag add a "with_profile" test that simulates
the annotated fast-path (mirror existing pattern like setting
user_index_mixin_instance.owasp_profile = MagicMock(...)) so the mixin returns
True, and a "without_profile" test that constructs a plain UserIndexMixin() and
asserts the flag is False; name them e.g.
test_idx_has_chapter_affinity_with_profile / _without_profile,
test_idx_has_committee_affinity_with_profile / _without_profile, and
test_idx_has_project_affinity_with_profile / _without_profile to match the
existing style and ensure both the annotated fast-path and the
query-fallback/absence case are covered.
In `@frontend/__tests__/a11y/pages/Users.a11y.test.tsx`:
- Around line 40-43: The test uses an arbitrary 100ms timeout inside act to wait
for the HeroUI Select async updates; replace that with a deterministic wait (use
waitFor and a stable DOM assertion) instead of setTimeout — e.g. await
waitFor(() => expect(screen.getByRole('button', { name: /relevancy/i
})).toBeInTheDocument()) — targeting the visible element rendered by your
SortBy/MembersFilter Select so the test waits only until the Select has finished
rendering; if no stable target exists, keep the minimal timeout but add a
comment explaining why and reference the Select component.
In `@frontend/__tests__/unit/components/MembersFilter.test.tsx`:
- Around line 32-58: Add regression tests that begin with one filter already
active to ensure choosing the other filter doesn't clear it: render
MembersFilter with defaultProps, first fireEvent.change on the hidden select to
set an initial member type (e.g., value 'staff') and then fireEvent.change again
to set an affinity (e.g., value 'projects'); assert onAffinityChange was called
with 'projects' and that onMemberTypesChange was not cleared (still
called/retained as ['staff'] or not called again depending on expected
behavior). Mirror this with the reverse: pre-select an affinity (e.g.,
'chapters') then select a member type (e.g., 'board') and assert
onMemberTypesChange receives ['board'] and onAffinityChange is not cleared; use
the existing hiddenSelect lookup and the defaultProps spies (onAffinityChange,
onMemberTypesChange) to locate and verify behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87e3043b-0bfd-459d-896f-ebf68e9483ee
📒 Files selected for processing (7)
backend/apps/github/models/mixins/user.pybackend/tests/apps/github/index/registry/user_test.pybackend/tests/apps/github/models/mixins/user_test.pyfrontend/__tests__/a11y/components/MembersFilter.a11y.test.tsxfrontend/__tests__/a11y/pages/Users.a11y.test.tsxfrontend/__tests__/unit/components/MembersFilter.test.tsxfrontend/src/components/MembersFilter.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/apps/github/index/registry/user_test.py
There was a problem hiding this comment.
Pull request overview
Adds structured filtering and replica-based sorting to the /members (users) search page, aligning it with the existing Algolia-driven search UX used elsewhere in the app. This requires both frontend UI/state updates and backend Algolia index/replica configuration to expose the new facets and sortable replicas.
Changes:
- Frontend: adds inline filter + sort UI to the Members page and wires facetFilters/sortBy/order into
useSearchPage. - Backend: extends the
usersAlgolia index with new facet attributes and configures sorting replicas for contributions/followers/public repos. - Tests/tooling: adds unit + a11y coverage for the new UI, updates index parameter retrieval, and extends replica update command coverage.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/utils/sortingOptions.ts | Adds user sort option definitions used by the Members page SortBy. |
| frontend/src/types/user.ts | Extends User type with new affinity/role flags exposed from the search index. |
| frontend/src/server/fetchAlgoliaData.ts | Broadens facetFilters typing to support Algolia OR groups. |
| frontend/src/hooks/useSearchPage.ts | Allows page reset on sort/order changes for the users index; supports typed facetFilters. |
| frontend/src/components/SearchPageLayout.tsx | Minor prop interface formatting changes (no functional changes). |
| frontend/src/components/MembersFilter.tsx | New inline combined filter control for affinity/role filtering. |
| frontend/src/app/members/page.tsx | Wires Members filtering/sorting into useSearchPage and renders MembersFilter + SortBy. |
| frontend/tests/unit/pages/Users.test.tsx | Adds unit assertions for inline sort + filter rendering. |
| frontend/tests/unit/components/MembersFilter.test.tsx | Adds unit tests for MembersFilter behavior and option presence. |
| frontend/tests/a11y/pages/Users.a11y.test.tsx | Updates a11y test setup for navigation mocking and new UI. |
| frontend/tests/a11y/components/MembersFilter.a11y.test.tsx | Adds a11y coverage for MembersFilter. |
| cspell/custom-dict.txt | Adds “reindexing” to the spellchecker dictionary. |
| backend/apps/github/models/mixins/user.py | Adds new idx_* properties for staff/board/mentor and affinity booleans. |
| backend/apps/github/index/registry/user.py | Adds new fields/facets to the users index and configures user sorting replicas. |
| backend/apps/core/utils/index.py | Adds new users attributes to retrieve from Algolia proxy. |
| backend/apps/common/index.py | Extends replica configuration to optionally apply shared base settings (faceting) to replicas. |
| backend/apps/common/management/commands/algolia_update_replicas.py | Runs replica configuration for the users index as well. |
| backend/tests/apps/github/models/mixins/user_test.py | Adds unit tests for the new idx_* user mixin properties. |
| backend/tests/apps/github/index/registry/user_test.py | Adds unit test for user index replica configuration + queryset construction. |
| backend/tests/apps/core/utils/match_test.py | Updates expected retrieved attributes for the users index. |
| backend/tests/apps/common/management/commands/algolia_update_replicas_test.py | Extends command tests to assert users replica configuration is invoked. |
Comments suppressed due to low confidence (1)
frontend/src/server/fetchAlgoliaData.ts:16
idx_is_active:trueis only appended whenindexNameis exactlyprojects/chapters. When sorting is enabled,useSearchPagequeries replica indices likeprojects_stars_count_desc, so this check won’t run and inactive projects can leak into sorted results. Consider checking the base index name (e.g.,indexName.split('_', 1)[0]) before applying default facet filters.
facetFilters: (string | string[])[] = []
): Promise<AlgoliaResponse<T>> => {
try {
if (['projects', 'chapters'].includes(indexName)) {
facetFilters.push('idx_is_active:true')
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/src/components/MembersFilter.tsx (1)
132-147:⚠️ Potential issue | 🟠 MajorStop clearing the opposite filter dimension in
handleChange.Line [142] and Line [146] still force-reset the other filter group, which prevents affinity + member-type filters from being active together.
💡 Minimal behavior fix
if (option.type === 'affinity') { onAffinityChange(option.key) - if (!selectedMemberTypes.includes('all')) onMemberTypesChange(['all']) + return } if (option.type === 'role') { onMemberTypesChange([option.key]) - if (selectedAffinity !== 'all') onAffinityChange('all') + return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/MembersFilter.tsx` around lines 132 - 147, handleChange currently resets the opposite filter dimension whenever an affinity or role is selected (see handleChange, option.type === 'affinity' and option.type === 'role'), which prevents combined affinity + member-type filtering; remove the lines that call onMemberTypesChange(['all']) inside the affinity branch and the lines that call onAffinityChange('all') inside the role branch so selection of one option no longer force-resets the other filter, but still keep the existing early-return for value === 'all' that resets both when the user explicitly chooses "all".backend/apps/github/models/mixins/user.py (1)
347-420:⚠️ Potential issue | 🟠 MajorThe annotation fast-path still never runs during bulk reindexing.
backend/apps/github/index/registry/user.py:get_entities()only doesselect_related("owasp_profile")andprefetch_related("chapters", "projects"); it never sets anyannotated_idx_has_*attributes. So reindexing still executes the fallbackRepositoryContributor.exists()queries here for every user, and committee affinity adds another_get_led_entities(...).exists()lookup on top. Please add the three annotations in the indexing queryset (or another precomputed path) so these properties actually avoid the N+1 cost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/github/models/mixins/user.py` around lines 347 - 420, The indexing queryset in get_entities() must add boolean annotations that mirror the idx_has_* properties so the annotated fast-path is used: add annotated_idx_has_chapter_affinity, annotated_idx_has_committee_affinity, and annotated_idx_has_project_affinity to the queryset returned by get_entities() in backend/apps/github/index/registry/user.py; compute each annotation with Exists subqueries against apps.github.models.repository_contributor.RepositoryContributor (filters: repository__key__startswith="www-chapter-" for chapter, "www-committee-" for committee, repository_id__in=Project.objects.values_list("owasp_repository_id", flat=True) for project) and also include existence of related leadership/membership by using Exists subqueries on the led entity tables (e.g., Committee.objects.filter(leaders__pk=OuterRef('pk')).exists() and equivalent for chapters/projects) or by Coalesce(Exists(contrib_subquery), Exists(led_subquery)) so the annotated_idx_has_* booleans match the logic in idx_has_chapter_affinity, idx_has_committee_affinity, and idx_has_project_affinity and avoid the fallback queries during reindexing.
🧹 Nitpick comments (2)
frontend/__tests__/a11y/components/MembersFilter.a11y.test.tsx (1)
21-31: Run an a11y audit with the dropdown opened as well.This test currently validates only the closed trigger state; popover/listbox accessibility is not exercised.
🧪 Example extension
-import { render } from '@testing-library/react' +import { render, screen } from '@testing-library/react' +import userEvent from '@testing-library/user-event' ... it('should not have any accessibility violations', async () => { + const user = userEvent.setup() const { baseElement } = render( <main> <MembersFilter {...defaultProps} /> </main> ) + await user.click(screen.getByRole('button')) const results = await axe(baseElement) expect(results).toHaveNoViolations() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/a11y/components/MembersFilter.a11y.test.tsx` around lines 21 - 31, The accessibility test for MembersFilter currently only audits the closed trigger; extend it to also open the dropdown/popover and run axe against the DOM with the listbox visible. In the test that renders <MembersFilter {...defaultProps} />, simulate user interaction to open the dropdown (e.g., click or keyboard event on the trigger element returned by render), wait for the popover/listbox to appear, then call axe() on the updated container/baseElement and assert expect(results).toHaveNoViolations(); reference the MembersFilter render, defaultProps, and the axe/baseElement variables to locate where to add the interaction and second audit.frontend/src/components/MembersFilter.tsx (1)
56-68: Avoid maintaining the same option keys/labels in multiple constants.
AFFINITY_OPTIONS,MEMBER_TYPE_OPTIONS, andcombinedOptionsrepeat the same data. This is easy to desync during future filter updates.♻️ Suggested consolidation
- const combinedOptions = [ - { key: 'all', label: 'All Filters', type: 'all' as const }, - { key: 'projects', label: 'Projects', type: 'affinity' as const }, - { key: 'chapters', label: 'Chapters', type: 'affinity' as const }, - { key: 'committees', label: 'Committees', type: 'affinity' as const }, - { key: 'staff', label: 'OWASP Staff', type: 'role' as const }, - { key: 'board', label: 'Board Member', type: 'role' as const }, - { key: 'gsoc', label: 'GSoC Mentor', type: 'role' as const }, - ] + const combinedOptions = [ + { key: 'all', label: 'All Filters', type: 'all' as const }, + ...AFFINITY_OPTIONS.filter((o) => o.key !== 'all').map((o) => ({ + ...o, + type: 'affinity' as const, + })), + ...MEMBER_TYPE_OPTIONS.filter((o) => o.key !== 'all').map((o) => ({ + ...o, + type: 'role' as const, + })), + ]Also applies to: 96-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/MembersFilter.tsx` around lines 56 - 68, AFFINITY_OPTIONS and MEMBER_TYPE_OPTIONS are duplicated elsewhere (e.g., combinedOptions) causing drift; consolidate by creating a single source of truth (e.g., a shared AFFINITY_OPTION_DEFS and MEMBER_TYPE_OPTION_DEFS or a factory function) and reuse those definitions when building AFFINITY_OPTIONS, MEMBER_TYPE_OPTIONS and combinedOptions so all consumers reference the same constants (update any code that references combinedOptions, AFFINITY_OPTIONS, or MEMBER_TYPE_OPTIONS to import/use the shared definitions or generator).
🤖 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/github/models/mixins/user.py`:
- Around line 312-321: The idx_is_owasp_staff method is incorrectly reading
is_owasp_staff from self.owasp_profile (which MemberProfile doesn't define)
causing AttributeError and mis-indexing; update idx_is_owasp_staff to read
is_owasp_staff from the User instance itself (use getattr(self,
"is_owasp_staff", False) or check hasattr(self, "is_owasp_staff") and return
that value) so users with no owasp_profile but with is_owasp_staff set are
indexed correctly and no AttributeError is raised.
---
Duplicate comments:
In `@backend/apps/github/models/mixins/user.py`:
- Around line 347-420: The indexing queryset in get_entities() must add boolean
annotations that mirror the idx_has_* properties so the annotated fast-path is
used: add annotated_idx_has_chapter_affinity,
annotated_idx_has_committee_affinity, and annotated_idx_has_project_affinity to
the queryset returned by get_entities() in
backend/apps/github/index/registry/user.py; compute each annotation with Exists
subqueries against
apps.github.models.repository_contributor.RepositoryContributor (filters:
repository__key__startswith="www-chapter-" for chapter, "www-committee-" for
committee, repository_id__in=Project.objects.values_list("owasp_repository_id",
flat=True) for project) and also include existence of related
leadership/membership by using Exists subqueries on the led entity tables (e.g.,
Committee.objects.filter(leaders__pk=OuterRef('pk')).exists() and equivalent for
chapters/projects) or by Coalesce(Exists(contrib_subquery),
Exists(led_subquery)) so the annotated_idx_has_* booleans match the logic in
idx_has_chapter_affinity, idx_has_committee_affinity, and
idx_has_project_affinity and avoid the fallback queries during reindexing.
In `@frontend/src/components/MembersFilter.tsx`:
- Around line 132-147: handleChange currently resets the opposite filter
dimension whenever an affinity or role is selected (see handleChange,
option.type === 'affinity' and option.type === 'role'), which prevents combined
affinity + member-type filtering; remove the lines that call
onMemberTypesChange(['all']) inside the affinity branch and the lines that call
onAffinityChange('all') inside the role branch so selection of one option no
longer force-resets the other filter, but still keep the existing early-return
for value === 'all' that resets both when the user explicitly chooses "all".
---
Nitpick comments:
In `@frontend/__tests__/a11y/components/MembersFilter.a11y.test.tsx`:
- Around line 21-31: The accessibility test for MembersFilter currently only
audits the closed trigger; extend it to also open the dropdown/popover and run
axe against the DOM with the listbox visible. In the test that renders
<MembersFilter {...defaultProps} />, simulate user interaction to open the
dropdown (e.g., click or keyboard event on the trigger element returned by
render), wait for the popover/listbox to appear, then call axe() on the updated
container/baseElement and assert expect(results).toHaveNoViolations(); reference
the MembersFilter render, defaultProps, and the axe/baseElement variables to
locate where to add the interaction and second audit.
In `@frontend/src/components/MembersFilter.tsx`:
- Around line 56-68: AFFINITY_OPTIONS and MEMBER_TYPE_OPTIONS are duplicated
elsewhere (e.g., combinedOptions) causing drift; consolidate by creating a
single source of truth (e.g., a shared AFFINITY_OPTION_DEFS and
MEMBER_TYPE_OPTION_DEFS or a factory function) and reuse those definitions when
building AFFINITY_OPTIONS, MEMBER_TYPE_OPTIONS and combinedOptions so all
consumers reference the same constants (update any code that references
combinedOptions, AFFINITY_OPTIONS, or MEMBER_TYPE_OPTIONS to import/use the
shared definitions or generator).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29da4641-37d2-4f8d-90ab-6bef63ee1e23
📒 Files selected for processing (7)
backend/apps/github/models/mixins/user.pybackend/tests/apps/github/index/registry/user_test.pybackend/tests/apps/github/models/mixins/user_test.pyfrontend/__tests__/a11y/components/MembersFilter.a11y.test.tsxfrontend/__tests__/a11y/pages/Users.a11y.test.tsxfrontend/__tests__/unit/components/MembersFilter.test.tsxfrontend/src/components/MembersFilter.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/tests/a11y/pages/Users.a11y.test.tsx
- frontend/tests/unit/components/MembersFilter.test.tsx
- backend/tests/apps/github/index/registry/user_test.py
- backend/tests/apps/github/models/mixins/user_test.py
Collaborator
|
@mrkeshav-05 hi! Are you still working on this PR? There are a lot of comments that were not resolved.. |
Contributor
Author
|
Hi @kasya, Yes I'm working on this PR and address those comments soon. |
Contributor
There was a problem hiding this comment.
1 issue 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/src/components/MembersFilter.tsx">
<violation number="1" location="frontend/src/components/MembersFilter.tsx:47">
P1: Role filter functionality (OWASP Staff/Board/GSoC) was removed from members filtering without replacement, causing a user-visible filtering regression.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Adds structured filtering and sorting to the /members (users) search experience, aligning it with the existing Algolia-powered search patterns used elsewhere (e.g., projects/chapters) and requiring new indexed fields + replicas on the backend.
Changes:
- Frontend: adds inline sort dropdown and an affinity filter control to the Members page, wiring filters into
useSearchPage/ Algolia facetFilters. - Backend: adds
idx_has_{project,chapter,committee}_affinityfields to the user index and configures Algolia replicas for sortable metrics (contributions/followers/public repos). - Tests/tooling: adds unit + a11y tests for the new filter component and updates replica configuration command/tests.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/utils/sortingOptions.ts | Adds user sorting options for SortBy dropdown. |
| frontend/src/types/user.ts | Extends User type with new fields used by indexing/UI. |
| frontend/src/server/fetchAlgoliaData.ts | Updates facetFilters typing to support Algolia OR-groups. |
| frontend/src/hooks/useSearchPage.ts | Updates facetFilters typing and ensures page reset on users sort changes. |
| frontend/src/components/SearchPageLayout.tsx | Adds data-testid hooks for inline sort rendering. |
| frontend/src/components/MembersFilter.tsx | New Members affinity filter UI component (HeroUI Select). |
| frontend/src/app/members/page.tsx | Wires MembersFilter + SortBy into users search and builds facetFilters. |
| frontend/tests/unit/pages/Users.test.tsx | Adds assertions for inline SortBy and MembersFilter rendering. |
| frontend/tests/unit/components/MembersFilter.test.tsx | New unit tests for MembersFilter behavior and constants. |
| frontend/tests/a11y/pages/Users.a11y.test.tsx | Updates a11y coverage for Users page with new UI elements. |
| frontend/tests/a11y/components/MembersFilter.a11y.test.tsx | New a11y test for MembersFilter component. |
| cspell/custom-dict.txt | Adds “reindexing” to dictionary. |
| backend/tests/unit/apps/github/models/mixins/user_test.py | Updates user mixin fixture to include staff flag. |
| backend/tests/unit/apps/github/index/registry/user_test.py | Adds replica configuration test + updates queryset expectations. |
| backend/tests/unit/apps/core/utils/match_test.py | Ensures users params include idx_contributions_count. |
| backend/tests/unit/apps/common/management/commands/algolia_update_replicas_test.py | Ensures user replicas are configured by management command. |
| backend/apps/github/models/mixins/user.py | Adds affinity indexing properties (idx_has_*_affinity). |
| backend/apps/github/index/registry/user.py | Adds affinity fields to index + defines user replica sorting. |
| backend/apps/core/utils/index.py | Ensures users retrieval params include idx_contributions_count. |
| backend/apps/common/management/commands/algolia_update_replicas.py | Adds UserIndex.configure_replicas() to command. |
| backend/apps/common/index.py | Extends configure_replicas to optionally apply base settings (faceting) to replicas. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
1 issue found across 3 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/github/index/registry/user.py">
<violation number="1" location="backend/apps/github/index/registry/user.py:117">
P2: The new affinity boolean properties (`idx_has_chapter_affinity`, `idx_has_project_affinity`, `idx_has_committee_affinity`) fall back to per-user `RepositoryContributor.exists()` queries when the annotated value is not present on the instance. Since `get_entities()` does not annotate `annotated_idx_has_chapter_affinity`, `annotated_idx_has_project_affinity`, or `annotated_idx_has_committee_affinity`, bulk reindexing will hit the fallback path for every user, resulting in an N+1 query pattern. Consider adding `Exists`/`OuterRef` annotations here so the properties can use precomputed values during indexing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
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
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 introduces filtering and sorting capabilities to the
/memberspage, aligning it with the existing UX patterns used in the Chapters and Projects pages.Previously, the Members page supported only text-based search, which made it difficult to discover relevant contributors among thousands of OWASP members. This update significantly improves discoverability by enabling structured filtering and sorting.
Added the following filters:
Added sorting options:
Before testing this PR, you must reindex the data to populate the new fields:
make index-dataResolves: #4246
Screen.Recording.2026-04-04.at.10.03.33.PM.mov
Checklist
make check-testlocally: all warnings addressed, tests passed