Add search to user management - #809
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:
📝 WalkthroughWalkthroughThe admin users page now supports case-insensitive client-side search, result counts, contextual empty states, and retryable loading errors. Partition members now expose display names and email addresses through bulk backend enrichment and updated UI components. ChangesAdmin user and partition updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant UserListPage
participant DataTable
Admin->>UserListPage: Enter or clear search text
UserListPage->>UserListPage: Normalize and filter users
UserListPage->>DataTable: Pass filteredUsers and emptyMessage
DataTable-->>Admin: Render rows, count, or empty state
sequenceDiagram
participant PartitionService
participant UserRepository
participant PartitionUsersUI
PartitionService->>PartitionService: Fetch partition membership rows
PartitionService->>UserRepository: Bulk-fetch referenced users
UserRepository-->>PartitionService: Return matching identities
PartitionService-->>PartitionUsersUI: Return enriched member records
PartitionUsersUI-->>PartitionUsersUI: Render identity and email fields
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
|
@codex review |
![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: f616af6bea
ℹ️ 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: 1
🧹 Nitpick comments (1)
ui/src/pages/admin/users/list.test.tsx (1)
82-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for searching by displayed user number.
list.tsxmatchesString(user.id)and`User #${user.id}`for search, per the PR objective of filtering by "displayed user number", but no test here exercises that path (only name/email/external-id are covered).✅ Suggested addition
await userEvent.click(screen.getByRole("button", { name: "Clear user search" })); await userEvent.type(search, "zArA operator"); expect(await screen.findByText("Zara Operator")).toBeTruthy(); + + await userEvent.click(screen.getByRole("button", { name: "Clear user search" })); + await userEvent.type(search, "User `#12`"); + + expect(await screen.findByText("Zara Operator")).toBeTruthy(); });🤖 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/users/list.test.tsx` around lines 82 - 121, Extend the “searches visible identifiers across paginated rows” test to search for a displayed user number, such as “User `#12`” or its numeric identifier, and assert that Zara Operator is returned with the expected single-user status. Keep the existing name, email, and external-ID coverage unchanged.
🤖 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 `@ui/src/pages/admin/users/list.tsx`:
- Around line 199-227: Update the user search Input in the users list to
suppress the browser’s native search cancel button while preserving the existing
custom clear Button behavior. Apply the appropriate WebKit search-cancel
pseudo-element styling through the Input’s className or established styling
mechanism.
---
Nitpick comments:
In `@ui/src/pages/admin/users/list.test.tsx`:
- Around line 82-121: Extend the “searches visible identifiers across paginated
rows” test to search for a displayed user number, such as “User `#12`” or its
numeric identifier, and assert that Zara Operator is returned with the expected
single-user status. Keep the existing name, email, and external-ID coverage
unchanged.
🪄 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: 0266618b-be23-4b08-97b9-84be3819449e
📒 Files selected for processing (3)
ui/src/components/shared/data-table.tsxui/src/pages/admin/users/list.test.tsxui/src/pages/admin/users/list.tsx
|
Addressed the remaining non-inline review request in 653809f. The displayed user number was a supported search key but had no regression coverage, so that contract could have been removed accidentally while the name/email/external-ID tests still passed. The existing cross-page search test now searches for Validation after all review fixes: 162 Admin UI tests passed, the production UI build passed, 32 focused backend tests passed, and Python lint passed. UI lint reports only the existing unrelated hook warning in the models page. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 653809f921
ℹ️ 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".
|
Integrated the partition-member identity work from #757 and extended it with email while keeping the stable user ID visible. The original per-member concurrent lookup was replaced with one bulk lookup so large partitions do not put unnecessary pressure on the database connection pool. Missing profile data still falls back safely to the user ID.\n\nValidated with the focused Python tests, the complete UI test suite, the production UI build, Ruff, and ESLint. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 415db6aa2b
ℹ️ 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".
Review pass — 1 fix, 2 remarks, 1 observationReviewed the full diff and ran the suites locally against the branch: 2215 backend unit tests, 168 UI tests, One commit pushed — The two substantive findings are left as remarks in their threads rather than as pushed changes — both are design calls that belong to you:
The rest of the change reads well. The bulk Observation, not a blocker and not in scope here: no workflow runs the |
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/orchestrators/test_partition_service.py (1)
238-247: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the batching test distinguish one batch from per-member lookups.
The test at Line 875 contains only one member, so a per-member lookup still passes. Additionally, Line 239 overwrites the previous request, preventing the fake from proving that exactly one batch was used. Track all lookup batches, use at least two members, and assert one call containing both IDs.
Also applies to: 875-892
🤖 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/orchestrators/test_partition_service.py` around lines 238 - 247, Update the fake service’s get_users_by_ids method to accumulate every requested batch instead of overwriting the prior request, then revise the batching test to use at least two member IDs and assert exactly one lookup batch containing both IDs. Keep the existing returned-user behavior unchanged.
🤖 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/orchestrators/test_partition_service.py`:
- Around line 238-247: Update the fake service’s get_users_by_ids method to
accumulate every requested batch instead of overwriting the prior request, then
revise the batching test to use at least two member IDs and assert exactly one
lookup batch containing both IDs. Keep the existing returned-user behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 484fe1fe-b49c-4b7d-98ea-b8f14ba9d765
📒 Files selected for processing (7)
openrag/api/routers/admin/partitions.pyopenrag/api/routers/admin/users.pyopenrag/services/orchestrators/partition_service.pytests/unit/services/orchestrators/test_partition_service.pyui/src/components/shared/data-table.tsxui/src/pages/admin/users/list.test.tsxui/src/pages/admin/users/list.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/src/pages/admin/users/list.tsx
- ui/src/pages/admin/users/list.test.tsx
2ee700e to
bd9e494
Compare
Re-review of
|
Ahmath-Gadji
left a comment
Ahmath-Gadji
left a comment
There was a problem hiding this comment.
Approving. Every finding I raised is addressed and verified on 251586f5, and all six CI checks are green.
For the record, what I checked rather than assumed: both fix commits were tested for vacuity — collapsing the list_members split, removing pageResetKey, and dropping the partition_memberships guard each turn the corresponding test red, so none of them pass by accident. Locally on this head: 2214 backend unit tests, 168 UI tests, tsc -b and ESLint pass.
One thing to settle before merge: the init commit
3ef20eb3 is titled "init". It is not a scratch commit — it is the original partition-member display-name work, authored by Baptiste (bnoleau@ik.me), and it is the seed of the identity feature this PR builds on. It still exists independently as #757, which is open.
That makes the obvious fix the wrong one. This repo merges rather than squashes, so the message lands in develop verbatim — but squash-merging to hide it would collapse Baptiste's authorship into a single commit attributed to this PR's author. The commit is worth attributing; only its message is the problem.
Suggested instead:
- Reword
3ef20eb3in place (git rebase, keeping theAuthor:line intact) to something like "Show partition member display name in the partitions UI". Authorship is preserved, history stays readable. - Close #757 with a note pointing at this PR, so it does not sit stale after the content merges here.
Neither is blocking from my side — flagging because both are easy to miss and awkward to undo after the merge.
3 participants
Closes #776
Context
Administrators need to find an account quickly as the user directory grows. Scanning pages manually slows routine account management and makes selecting the wrong account more likely.
Change
The user-management view now filters the complete authorized user list by display name, email, external identity, or displayed user number. Search works across the existing pagination, reports the result count, can be cleared in one action, and distinguishes an empty directory from a search with no matches.
The existing admin-only permissions and user API remain unchanged.
Validation
All 161 Admin UI tests pass, including coverage for searches across paginated rows and the empty states. The production UI build succeeds, and linting reports no new warnings or errors.
Summary by CodeRabbit