Select partition members by user name - #810
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a cursor-paginated admin endpoint for non-member partition users, a frontend candidate API, and a searchable multi-user picker integrated with batch membership additions and partial-failure feedback. ChangesPartition member candidate selection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant PartitionDetail
participant MemberPicker
participant PartitionAPI
participant PartitionService
participant MembershipRepository
Admin->>PartitionDetail: Open add-users dialog
PartitionDetail->>MemberPicker: Request candidate page
MemberPicker->>PartitionAPI: GET users/candidates with search and cursor
PartitionAPI->>PartitionService: list_member_candidates
PartitionService->>MembershipRepository: Query non-member candidates
MembershipRepository-->>PartitionService: Candidate rows
PartitionService-->>PartitionAPI: Paginated candidate page
PartitionAPI-->>MemberPicker: Candidate identities
Admin->>MemberPicker: Select multiple users
MemberPicker-->>PartitionDetail: Selected candidates
PartitionDetail->>PartitionAPI: Add selected members
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67a43d1016
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/api/routers/admin/test_phase14_partition_routes.py (1)
158-175: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a non-owner authorization regression test.
This test only exercises an allowed owner. Add a case where
require_partition_ownerrejects the request and assert the endpoint returns the expected 403 response. As per coding guidelines, partition operations must enforce the appropriate owner dependency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/api/routers/admin/test_phase14_partition_routes.py` around lines 158 - 175, Add a separate async regression test near test_list_partition_member_candidates_returns_stable_identities that configures require_partition_owner to reject the caller, requests the same candidates endpoint, and asserts the response status is 403. Verify the owner dependency is enforced without changing the existing successful-owner test.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openrag/services/orchestrators/partition_service.py`:
- Around line 680-703: The candidate API must become bounded and paginated
instead of loading every user at once. In
openrag/services/orchestrators/partition_service.py lines 680-703, update
list_member_candidates to accept pagination and search inputs, apply search
while preserving non-member filtering, and return page results with continuation
metadata. In openrag/api/routers/admin/partitions.py lines 408-431, accept and
forward those parameters, return the service’s pagination metadata, and retain
require_partition_owner.
In `@ui/src/pages/admin/partitions/detail.tsx`:
- Around line 489-495: Update the Cancel button’s close action to call
handleDialogOpenChange(false) instead of setDialogOpen(false), ensuring selected
user IDs and role are reset through the shared dialog cleanup handler.
---
Nitpick comments:
In `@tests/unit/api/routers/admin/test_phase14_partition_routes.py`:
- Around line 158-175: Add a separate async regression test near
test_list_partition_member_candidates_returns_stable_identities that configures
require_partition_owner to reject the caller, requests the same candidates
endpoint, and asserts the response status is 403. Verify the owner dependency is
enforced without changing the existing successful-owner test.
🪄 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 Plus
Run ID: bcbc566a-95d7-44f9-9b3a-429dcbd7640e
📒 Files selected for processing (9)
openrag/api/routers/admin/partitions.pyopenrag/services/orchestrators/partition_service.pytests/unit/api/routers/admin/test_phase14_partition_routes.pytests/unit/services/orchestrators/test_partition_service.pyui/src/lib/api/partitions.test.tsui/src/lib/api/partitions.tsui/src/pages/admin/partitions/detail.tsxui/src/pages/admin/partitions/member-picker.test.tsxui/src/pages/admin/partitions/member-picker.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openrag/services/persistence/partition_membership_repo.py (1)
126-165: 🚀 Performance & Scalability | 🔵 TrivialCorrect implementation; one scaling note for later.
The exclusion/search/pagination logic checks out against the integration test. For future scale:
STRPOS(LOWER(...))prevents index usage, andOFFSET-based pagination degrades at very large offsets/user-table sizes. Not an issue at current scale (admin-scoped picker, capped page size), but worth revisiting with a trigram/GIN index or keyset pagination if theuserstable grows large.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/services/persistence/partition_membership_repo.py` around lines 126 - 165, The implementation is correct; no code changes are required. Retain the exclusion, search, ordering, and bounded OFFSET pagination in list_partition_member_candidates, while noting trigram/GIN indexing or keyset pagination as future scaling options only if user-table size warrants them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openrag/services/persistence/partition_membership_repo.py`:
- Around line 126-165: The implementation is correct; no code changes are
required. Retain the exclusion, search, ordering, and bounded OFFSET pagination
in list_partition_member_candidates, while noting trigram/GIN indexing or keyset
pagination as future scaling options only if user-table size warrants them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: da0c569e-6d6c-4206-b874-9c56069bf480
📒 Files selected for processing (12)
openrag/api/routers/admin/partitions.pyopenrag/core/ports/partition_membership_repo.pyopenrag/services/orchestrators/partition_service.pyopenrag/services/persistence/partition_membership_repo.pytests/integration/repos/test_partition_membership_repo.pytests/unit/api/routers/admin/test_phase14_partition_routes.pytests/unit/services/orchestrators/test_partition_service.pyui/src/lib/api/partitions.test.tsui/src/lib/api/partitions.tsui/src/pages/admin/partitions/detail.tsxui/src/pages/admin/partitions/member-picker.test.tsxui/src/pages/admin/partitions/member-picker.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- openrag/api/routers/admin/partitions.py
- ui/src/lib/api/partitions.test.ts
- ui/src/pages/admin/partitions/detail.tsx
…mber-picker # Conflicts: # openrag/services/orchestrators/partition_service.py # tests/unit/services/orchestrators/test_partition_service.py # ui/src/pages/admin/partitions/detail.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/src/pages/admin/partitions/detail.tsx (1)
440-481: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd an
onErrorhandler toaddMutation.Every other mutation in this file (
removeMutation,roleMutation,GeneralTab'smutation) surfaces failures viatoast.error(...).addMutationonly definesmutationFn/onSuccess.addPartitionMembers(member-batch.ts) swallows per-candidate HTTP errors intofailures, so this path should rarely reject — but if it does (e.g. an unexpected exception), the dialog is left silently stuck with no feedback and a re-enabled button, sinceaddFailureswas already cleared beforemutate().🛠️ Proposed fix
addPartitionMembers({ partitionName, candidates: selected, role: selectedRole, }), + onError: (error: Error) => { + toast.error(`Failed to add users: ${error.message}`); + }, onSuccess: ({ addedCandidates, failures }) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/pages/admin/partitions/detail.tsx` around lines 440 - 481, Add an onError handler to addMutation that surfaces unexpected addPartitionMembers rejection errors through toast.error, matching the error handling used by removeMutation, roleMutation, and GeneralTab's mutation. Preserve the existing onSuccess behavior and use the caught error's message when available.
🧹 Nitpick comments (2)
openrag/services/persistence/migrations/alembic/versions/e5f6a7b8c9d0_add_user_display_name_prefix_index.py (1)
23-29: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider
CREATE INDEX CONCURRENTLYto avoid lockingusersduring upgrade.A plain
CREATE INDEXonuserstakes a lock that blocks writes for the duration of the build; on a populated table this can stall auth/user-creation flows during deploy.CONCURRENTLYavoids this but must run outside a transaction block (verify the Alembic env supportsop.get_context().autocommit_block()or non-transactional migrations here before applying).♻️ Proposed fix (requires non-transactional migration context)
def upgrade() -> None: if table_exists("users") and not index_exists("users", _INDEX_NAME): - op.execute( - sa.text( - f"CREATE INDEX {_INDEX_NAME} ON users (LOWER(display_name) text_pattern_ops)", - ), - ) + with op.get_context().autocommit_block(): + op.execute( + sa.text( + f"CREATE INDEX CONCURRENTLY {_INDEX_NAME} ON users (LOWER(display_name) text_pattern_ops)", + ), + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/services/persistence/migrations/alembic/versions/e5f6a7b8c9d0_add_user_display_name_prefix_index.py` around lines 23 - 29, Update upgrade() to create the users display-name index with CREATE INDEX CONCURRENTLY, using the migration context’s supported autocommit mechanism such as op.get_context().autocommit_block(). Preserve the existing table_exists and index_exists guards, and verify the Alembic environment supports this non-transactional execution.ui/src/pages/admin/partitions/member-picker.tsx (1)
29-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnify the candidate fallback-label text between the picker and the failure alert. Both sites format "no display name" candidates independently and now disagree on the fallback text (
User #<id>vsUnnamed user), which can show two different labels for the same user in one dialog.
ui/src/pages/admin/partitions/member-picker.tsx#L29-L31: exportcandidateLabelso it can be reused elsewhere instead of being re-implemented.ui/src/pages/admin/partitions/detail.tsx#L664-L680: import and use the exportedcandidateLabelinstead of the inlinedisplay_name?.trim() || "Unnamed user"fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/pages/admin/partitions/member-picker.tsx` around lines 29 - 31, Unify candidate fallback labels by exporting candidateLabel from ui/src/pages/admin/partitions/member-picker.tsx (lines 29-31). In ui/src/pages/admin/partitions/detail.tsx (lines 664-680), import and use candidateLabel instead of the inline display_name fallback; no other changes are needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ui/src/pages/admin/partitions/detail.tsx`:
- Around line 440-481: Add an onError handler to addMutation that surfaces
unexpected addPartitionMembers rejection errors through toast.error, matching
the error handling used by removeMutation, roleMutation, and GeneralTab's
mutation. Preserve the existing onSuccess behavior and use the caught error's
message when available.
---
Nitpick comments:
In
`@openrag/services/persistence/migrations/alembic/versions/e5f6a7b8c9d0_add_user_display_name_prefix_index.py`:
- Around line 23-29: Update upgrade() to create the users display-name index
with CREATE INDEX CONCURRENTLY, using the migration context’s supported
autocommit mechanism such as op.get_context().autocommit_block(). Preserve the
existing table_exists and index_exists guards, and verify the Alembic
environment supports this non-transactional execution.
In `@ui/src/pages/admin/partitions/member-picker.tsx`:
- Around line 29-31: Unify candidate fallback labels by exporting candidateLabel
from ui/src/pages/admin/partitions/member-picker.tsx (lines 29-31). In
ui/src/pages/admin/partitions/detail.tsx (lines 664-680), import and use
candidateLabel instead of the inline display_name fallback; no other changes are
needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f9460fdf-6c3e-41d4-8083-ff9c01b15386
📒 Files selected for processing (18)
openrag/api/routers/admin/partitions.pyopenrag/core/ports/partition_membership_repo.pyopenrag/services/orchestrators/partition_service.pyopenrag/services/persistence/migrations/alembic/versions/e5f6a7b8c9d0_add_user_display_name_prefix_index.pyopenrag/services/persistence/partition_membership_repo.pyopenrag/services/persistence/schema.pytests/integration/repos/test_partition_membership_repo.pytests/unit/api/routers/admin/test_phase14_partition_routes.pytests/unit/services/orchestrators/test_partition_service.pytests/unit/services/persistence/test_add_partition_member.pytests/unit/services/persistence/test_partition_member_candidates.pyui/src/lib/api/partitions.test.tsui/src/lib/api/partitions.tsui/src/pages/admin/partitions/detail.tsxui/src/pages/admin/partitions/member-batch.test.tsui/src/pages/admin/partitions/member-batch.tsui/src/pages/admin/partitions/member-picker.test.tsxui/src/pages/admin/partitions/member-picker.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/api/routers/admin/test_phase14_partition_routes.py
Review — verified against
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/services/persistence/test_display_name_index_migration.py (1)
1-83: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the Ruff formatting failure.
ruff format --checkcurrently fails for this file, so the lint pipeline cannot pass. Runruff format tests/unit/services/persistence/test_display_name_index_migration.pyand commit the result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/services/persistence/test_display_name_index_migration.py` around lines 1 - 83, Format the test file with Ruff so it passes `ruff format --check`; run Ruff formatting on the file containing the `migration` fixture and upgrade tests, then commit the resulting formatting-only changes.Source: Pipeline failures
🧹 Nitpick comments (1)
tests/unit/services/persistence/test_display_name_index_migration.py (1)
48-58: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the concurrent-index autocommit boundary.
The fake context records
"autocommit", but neither CREATE INDEX test verifies it. A regression that movesCREATE INDEX CONCURRENTLYoutsideop.get_context().autocommit_block()would still pass these tests despite violating the migration contract.Suggested assertions
assert "CREATE INDEX CONCURRENTLY ix_users_lower_display_name_pattern" in statements + assert "autocommit" in fake_op.callsAdd the equivalent assertion to the missing-index test.
Also applies to: 72-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/services/persistence/test_display_name_index_migration.py` around lines 48 - 58, Add an assertion in the missing-index test, alongside the existing CREATE INDEX CONCURRENTLY statement check, verifying that the fake operation recorded the "autocommit" boundary. Ensure the invalid-index rebuild test also confirms CREATE INDEX CONCURRENTLY executes within op.get_context().autocommit_block(), preserving the migration contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/unit/services/persistence/test_display_name_index_migration.py`:
- Around line 1-83: Format the test file with Ruff so it passes `ruff format
--check`; run Ruff formatting on the file containing the `migration` fixture and
upgrade tests, then commit the resulting formatting-only changes.
---
Nitpick comments:
In `@tests/unit/services/persistence/test_display_name_index_migration.py`:
- Around line 48-58: Add an assertion in the missing-index test, alongside the
existing CREATE INDEX CONCURRENTLY statement check, verifying that the fake
operation recorded the "autocommit" boundary. Ensure the invalid-index rebuild
test also confirms CREATE INDEX CONCURRENTLY executes within
op.get_context().autocommit_block(), preserving the migration contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c3b74b42-8c03-49dd-8833-e50bdeedc6be
📒 Files selected for processing (18)
docs/content/docs/documentation/API.mdxopenrag/api/routers/admin/partitions.pyopenrag/services/orchestrators/partition_service.pyopenrag/services/persistence/migrations/alembic/versions/e5f6a7b8c9d0_add_user_display_name_prefix_index.pyopenrag/services/persistence/partition_membership_repo.pytests/integration/repos/test_partition_membership_repo.pytests/unit/api/routers/admin/test_phase14_partition_routes.pytests/unit/services/orchestrators/test_partition_service.pytests/unit/services/persistence/test_add_partition_member.pytests/unit/services/persistence/test_display_name_index_migration.pyui/src/lib/api/partitions.test.tsui/src/lib/api/partitions.tsui/src/pages/admin/partitions/detail.tsxui/src/pages/admin/partitions/member-batch.test.tsui/src/pages/admin/partitions/member-batch.tsui/src/pages/admin/partitions/member-candidate.tsui/src/pages/admin/partitions/member-picker.test.tsxui/src/pages/admin/partitions/member-picker.tsx
🚧 Files skipped from review as they are similar to previous changes (15)
- ui/src/pages/admin/partitions/member-candidate.ts
- ui/src/pages/admin/partitions/member-batch.test.ts
- ui/src/pages/admin/partitions/member-batch.ts
- openrag/api/routers/admin/partitions.py
- tests/unit/api/routers/admin/test_phase14_partition_routes.py
- tests/integration/repos/test_partition_membership_repo.py
- ui/src/pages/admin/partitions/member-picker.tsx
- tests/unit/services/persistence/test_add_partition_member.py
- ui/src/pages/admin/partitions/detail.tsx
- openrag/services/persistence/partition_membership_repo.py
- openrag/services/persistence/migrations/alembic/versions/e5f6a7b8c9d0_add_user_display_name_prefix_index.py
- tests/unit/services/orchestrators/test_partition_service.py
- openrag/services/orchestrators/partition_service.py
- ui/src/lib/api/partitions.test.ts
- ui/src/pages/admin/partitions/member-picker.test.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7df8923dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
andyne13
left a comment
andyne13
left a comment
There was a problem hiding this comment.
Approving — every review finding verified as addressed on 9774c083
- Breaking change —
API.mdxnow documents the409and the "usePATCHto change a role" note; the error message names the endpoint. - ID vs prefix mismatch —
isascii() and isdecimal(). I re-ran the full matrix: UI and backend now agree on every shape, including١٢٣(prefix search, no longer an exact lookup of user 123). Numeric display names became searchable as a bonus. - Directory exposure — resolved as an explicit decision rather than code: the new "Trust model" section states the gating is a load/usability control, not a privacy boundary. That's the honest framing.
- Candidate identity —
emailis selected and rendered throughcandidateSecondaryLabel()with an ID fallback, so duplicate names are distinguishable without requiring email to exist. - Invalid index —
_index_validity()drops and rebuilds. Verified against a real PostgreSQL 16, not just the mock test: fresh → valid; re-run → no rebuild; forcedindisvalid=false→ dropped, rebuilt, valid; and the rebuilt index is used by the candidate query at 200k rows. - Status-string parsing — replaced with
RETURNING 1/created is not None. attempted— removed.
Gates run locally: 170 backend unit tests, 179 UI tests, ruff check, ruff format --check, and the layer-import guard — all green.
Two things left that I'm happy to see ship as-is: the batch add is still a sequential per-user loop, and 2_147_483_647 remains a literal in the route alongside _MAX_POSTGRES_INTEGER.
LGTM.
3 participants
Why
Partition access is granted to people, but partition owners usually know a name rather than an internal numeric ID. Requiring raw IDs makes the workflow difficult to verify and increases the risk of granting access to the wrong account.
What changed
The add-member dialog now supports targeted search by display-name prefix and matching ASCII user ID. Results include email when available so duplicate names can be distinguished, selected identities remain visible, and one role can be assigned to multiple people.
Candidate search is paginated and remains stable while memberships change. Concurrent additions cannot overwrite an existing role, and refresh, paging, and per-user failures remain visible and retryable.
Compatibility
Adding a member is now insert-only. Posting an existing member returns
409and preserves their current role; callers must use the existing role-update endpoint to change it.Trust model
Search gating and pagination prevent the UI from loading the complete user directory at once. They are load and usability controls, not a privacy boundary. Partition owners are trusted to discover the identities needed to manage access.
Validation
Closes #777
Related to #757 and #809.