feat: push OAuth2 sessions filtering and pagination to SQL with total count - #4775
Conversation
|
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 4 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds server-side search, mode filtering, and offset-based pagination to the OAuth2 sessions listing. The ChangesOAuth2 Sessions Filter & Pagination
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (7): Last reviewed commit: "feat: adds pagination to mcp oauth grant..." | Re-trigger Greptile |
af483cc to
eae8b64
Compare
871780b to
4a49cd9
Compare
![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 (2)
ui/lib/store/apis/oauth2SessionsApi.ts (1)
34-48: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCanonicalize
bf_modebefore it becomes the RTK Query cache key.Sorting in
buildOAuth2GrantsListParamsonly affects the outgoing request params; RTK Query’s cache entry is derived from the original query arg, so["user", "vk"]and["vk", "user"]can still fragment the cache. Sort the array before callinguseGetOAuth2GrantsQuery, or add an endpoint-levelserializeQueryArgs.🤖 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/lib/store/apis/oauth2SessionsApi.ts` around lines 34 - 48, Canonicalize bf_mode in the query arg used by oauth2SessionsApi so RTK Query sees a stable cache key; sorting only inside buildOAuth2GrantsListParams does not affect the key derived from getOAuth2Grants. Update the caller path before useGetOAuth2GrantsQuery or add serializeQueryArgs on the getOAuth2Grants endpoint so arrays like ["user","vk"] and ["vk","user"] map to the same cache entry.ui/app/workspace/oauth-grants/page.tsx (1)
45-50: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDepend on a loaded boolean instead of the response object.
datais only a readiness guard here; depending on the full response object can rerun this effect on each refetch/poll. Use a derived boolean such asconst hasLoadedPage = data !== undefinedand depend on that withtotalCountandurlState.offset.Based on learnings: “In paginated views, omit the API response object ... if it is only used as a null-guard and meaningful changes surface through derived primitives.”
🤖 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/app/workspace/oauth-grants/page.tsx` around lines 45 - 50, The effect in oauth-grants/page.tsx is using the full response object as a readiness guard, which can cause reruns on every refetch even when the meaningful state has not changed. Replace the direct dependency on data in the useEffect that updates setUrlState with a derived boolean like hasLoadedPage = data !== undefined, and use that boolean alongside totalCount and urlState.offset in the dependency list. Keep the existing pagination adjustment logic intact, but ensure the effect only reacts to loaded-state transitions and the primitive values it actually uses.Source: Learnings
🤖 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 `@framework/configstore/rdb.go`:
- Around line 7104-7111: The pagination query in the grant list is only ordered
by rt.created_at DESC, which can shuffle rows with identical timestamps between
pages. Update the query built in this block to add a unique secondary sort key
after created_at, using the same query chain that applies Limit and Offset so
the ordering stays stable. Also add or adjust a test around the pagination path
to verify rows with the same timestamp stay in a deterministic order across
pages.
In `@transports/bifrost-http/handlers/mcpoauth2sessions.go`:
- Around line 66-67: The mcpoauth2sessions handler currently accepts any bf_mode
value via parseCommaSeparated, which lets unsupported modes slip through
silently. In the request handling logic around q.Modes, validate the bf_mode
query values against the documented whitelist of user, vk, and session before
calling the store, and return a 400 for any invalid value instead of passing
arbitrary strings onward.
---
Nitpick comments:
In `@ui/app/workspace/oauth-grants/page.tsx`:
- Around line 45-50: The effect in oauth-grants/page.tsx is using the full
response object as a readiness guard, which can cause reruns on every refetch
even when the meaningful state has not changed. Replace the direct dependency on
data in the useEffect that updates setUrlState with a derived boolean like
hasLoadedPage = data !== undefined, and use that boolean alongside totalCount
and urlState.offset in the dependency list. Keep the existing pagination
adjustment logic intact, but ensure the effect only reacts to loaded-state
transitions and the primitive values it actually uses.
In `@ui/lib/store/apis/oauth2SessionsApi.ts`:
- Around line 34-48: Canonicalize bf_mode in the query arg used by
oauth2SessionsApi so RTK Query sees a stable cache key; sorting only inside
buildOAuth2GrantsListParams does not affect the key derived from
getOAuth2Grants. Update the caller path before useGetOAuth2GrantsQuery or add
serializeQueryArgs on the getOAuth2Grants endpoint so arrays like ["user","vk"]
and ["vk","user"] map to the same cache entry.
🪄 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: a91484d5-eddf-4187-ae24-30acb6529e44
📒 Files selected for processing (9)
framework/configstore/rdb.goframework/configstore/rdb_oauth2_test.goframework/configstore/store.gotransports/bifrost-http/handlers/mcpoauth2jwt_test.gotransports/bifrost-http/handlers/mcpoauth2sessions.gotransports/bifrost-http/lib/config_test.goui/app/workspace/oauth-grants/page.tsxui/app/workspace/oauth-grants/views/grantsTable.tsxui/lib/store/apis/oauth2SessionsApi.ts
4a49cd9 to
df6e27c
Compare
3d10adc to
e3dc25b
Compare
df6e27c to
6ea5663
Compare
6ea5663 to
afb130d
Compare
2de7853 to
b197e08
Compare
afb130d to
375e4e3
Compare
Merge activity
|
The base branch was changed.
375e4e3 to
0441eb4
Compare
… count (#4775) ## Summary The OAuth Grants (Connected Clients) UI previously loaded all active sessions in a single query and performed filtering and pagination entirely in the browser. This PR moves filtering (case-insensitive search across client name, client ID, bound identity, and virtual key display name; `bf_mode` filter) and pagination (limit/offset) into SQL, and adds a total-count return value so the UI can render accurate page indicators without holding the full dataset in memory. ## Changes - `ListOAuth2Sessions` now accepts an `OAuth2SessionsQueryParams` struct (search, modes, limit, offset) and returns a second `int64` total-count value alongside the page slice. Filtering and pagination are applied in SQL using a shared base query; a `gorm.Session` fork ensures the count and the page query don't pollute each other's statement. - `GET /api/oauth2/sessions` parses `q`, `bf_mode`, `limit`, and `offset` query parameters, validates them, and forwards them to the store. The response envelope now includes `count`, `total_count`, `limit`, and `offset` fields, matching the shape used by the MCP auth-sessions endpoint. - The UI switches from local `useState` for search/mode/offset to `nuqs` URL query state, so filter and page selections survive navigation and can be bookmarked. Search is debounced (300 ms) before triggering a fetch. RTK Query receives the filter+page params directly, giving each combination its own cache entry. - The grants table gains a sticky header and a scrollable body so the page layout no longer grows unboundedly. The pagination bar is always visible when there are results (not only when results exceed one page), and shows an entry range and page-of-total indicator. - A new `TestListOAuth2Sessions_FilterAndPaginate` test pins ordering (created_at DESC), limit/offset paging, total-count independence from the page slice, `bf_mode` filtering, and case-insensitive search against the joined client name, joined VK display name, and bound identity. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go test ./framework/configstore/... -run TestListOAuth2Sessions go test ./transports/bifrost-http/... # UI cd ui pnpm i pnpm build ``` Navigate to the OAuth Grants page, enter a search term, toggle a mode filter, and verify the URL updates and results narrow correctly. Advance to a second page and confirm the total count and page indicator remain accurate. Revoke the last grant on a non-first page and confirm the offset snaps back rather than leaving a blank page. ## Screenshots/Recordings _Add before/after screenshots of the grants table and pagination bar._ ## Breaking changes - [x] Yes - [ ] No `ListOAuth2Sessions` signature changed: callers must pass an `OAuth2SessionsQueryParams` argument and accept a second `int64` return value. Any mock or alternative implementation of `ConfigStore` must be updated accordingly (the in-tree mocks in `mcpoauth2jwt_test.go` and `lib/config_test.go` have been updated). ## Related issues ## Security considerations Search terms are passed to SQL as `LIKE` parameters via GORM's parameterised query API; no raw interpolation is performed. The `ScopedDB` wrapper is preserved, so row-visibility predicates injected via context continue to apply to both the count and the page query. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
2 participants
Summary
The OAuth Grants (Connected Clients) UI previously loaded all active sessions in a single query and performed filtering and pagination entirely in the browser. This PR moves filtering (case-insensitive search across client name, client ID, bound identity, and virtual key display name;
bf_modefilter) and pagination (limit/offset) into SQL, and adds a total-count return value so the UI can render accurate page indicators without holding the full dataset in memory.Changes
ListOAuth2Sessionsnow accepts anOAuth2SessionsQueryParamsstruct (search, modes, limit, offset) and returns a secondint64total-count value alongside the page slice. Filtering and pagination are applied in SQL using a shared base query; agorm.Sessionfork ensures the count and the page query don't pollute each other's statement.GET /api/oauth2/sessionsparsesq,bf_mode,limit, andoffsetquery parameters, validates them, and forwards them to the store. The response envelope now includescount,total_count,limit, andoffsetfields, matching the shape used by the MCP auth-sessions endpoint.useStatefor search/mode/offset tonuqsURL query state, so filter and page selections survive navigation and can be bookmarked. Search is debounced (300 ms) before triggering a fetch. RTK Query receives the filter+page params directly, giving each combination its own cache entry.TestListOAuth2Sessions_FilterAndPaginatetest pins ordering (created_at DESC), limit/offset paging, total-count independence from the page slice,bf_modefiltering, and case-insensitive search against the joined client name, joined VK display name, and bound identity.Type of change
Affected areas
How to test
Navigate to the OAuth Grants page, enter a search term, toggle a mode filter, and verify the URL updates and results narrow correctly. Advance to a second page and confirm the total count and page indicator remain accurate. Revoke the last grant on a non-first page and confirm the offset snaps back rather than leaving a blank page.
Screenshots/Recordings
Add before/after screenshots of the grants table and pagination bar.
Breaking changes
ListOAuth2Sessionssignature changed: callers must pass anOAuth2SessionsQueryParamsargument and accept a secondint64return value. Any mock or alternative implementation ofConfigStoremust be updated accordingly (the in-tree mocks inmcpoauth2jwt_test.goandlib/config_test.gohave been updated).Related issues
Security considerations
Search terms are passed to SQL as
LIKEparameters via GORM's parameterised query API; no raw interpolation is performed. TheScopedDBwrapper is preserved, so row-visibility predicates injected via context continue to apply to both the count and the page query.Checklist
docs/contributing/README.mdand followed the guidelines