fix(platform): show user emails in usage table and add skeleton loading to governance tabs#1553
Merged
Merged
Conversation
…ng to all governance tabs Usage table now resolves user IDs to display names (email/name) via getUserNamesBatch. All governance editor components show skeleton placeholders while loading instead of rendering nothing.
Contributor
📝 WalkthroughWalkthroughThis PR introduces skeleton loading UI patterns across multiple governance editor components by replacing empty loading states with visual placeholders marked with Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/app/routes/dashboard/`$id/settings/governance.tsx:
- Around line 191-204: The route-level loading wrapper (the top-level <div
className="flex gap-6"> returned in the loading branch) should include the
accessibility attribute aria-busy="true"; update the JSX returned in the loading
state to add aria-busy="true" on that container (the div with className "flex
gap-6") so assistive technologies are informed that the region is loading.
In `@services/platform/convex/governance/queries.ts`:
- Around line 138-143: The user-name batch lookup can fan out too many internal
queries when rawEntries has many unique userIds; in getUsageSummary (or the code
that builds userIds/rawEntries) add a guard: compute unique userIds from
rawEntries, and if the count exceeds a safe threshold constant (e.g.
MAX_USER_NAME_LOOKUPS), skip calling getUserNamesBatch and set displayName =
userId for those entries, otherwise call getUserNamesBatch; alternatively
implement chunked lookups with bounded concurrency (e.g. Promise.all on chunks)
and fall back to userId on errors or when threshold exceeded; update the code
around rawEntries.map, userIds, getUserNamesBatch and entries to use this
behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 506e1d9f-caa1-4a27-8ad6-750771957be4
📒 Files selected for processing (12)
services/platform/app/features/settings/governance/components/budget-editor.tsxservices/platform/app/features/settings/governance/components/default-model-editor.tsxservices/platform/app/features/settings/governance/components/login-policy-editor.tsxservices/platform/app/features/settings/governance/components/model-access-editor.tsxservices/platform/app/features/settings/governance/components/password-policy-editor.tsxservices/platform/app/features/settings/governance/components/pii-config.tsxservices/platform/app/features/settings/governance/components/retention-editor.tsxservices/platform/app/features/settings/governance/components/system-prompt-editor.tsxservices/platform/app/features/settings/governance/components/upload-policy-editor.tsxservices/platform/app/features/settings/governance/components/usage-dashboard.tsxservices/platform/app/routes/dashboard/$id/settings/governance.tsxservices/platform/convex/governance/queries.ts
Comment on lines
+191
to
+204
| return ( | ||
| <div className="flex gap-6"> | ||
| <div className="w-[16rem] shrink-0 space-y-2"> | ||
| <Skeleton className="h-9 w-full rounded-md" /> | ||
| <Skeleton className="h-9 w-full rounded-md" /> | ||
| <Skeleton className="h-9 w-full rounded-md" /> | ||
| </div> | ||
| <div className="flex-1 space-y-3"> | ||
| <Skeleton className="h-6 w-48" /> | ||
| <Skeleton className="h-4 w-72" /> | ||
| <Skeleton className="h-10 w-full" /> | ||
| </div> | ||
| </div> | ||
| ); |
Contributor
There was a problem hiding this comment.
Add aria-busy to the route-level loading container.
Line 192 returns the loading skeleton wrapper without aria-busy="true", so assistive tech won’t be informed that this region is in a loading state.
Suggested fix
if (abilityLoading) {
return (
- <div className="flex gap-6">
+ <div aria-busy="true" className="flex gap-6">
<div className="w-[16rem] shrink-0 space-y-2">
<Skeleton className="h-9 w-full rounded-md" />
<Skeleton className="h-9 w-full rounded-md" />
<Skeleton className="h-9 w-full rounded-md" />
</div>As per coding guidelines: Use aria-busy="true" on containers whose content is loading for accessibility.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return ( | |
| <div className="flex gap-6"> | |
| <div className="w-[16rem] shrink-0 space-y-2"> | |
| <Skeleton className="h-9 w-full rounded-md" /> | |
| <Skeleton className="h-9 w-full rounded-md" /> | |
| <Skeleton className="h-9 w-full rounded-md" /> | |
| </div> | |
| <div className="flex-1 space-y-3"> | |
| <Skeleton className="h-6 w-48" /> | |
| <Skeleton className="h-4 w-72" /> | |
| <Skeleton className="h-10 w-full" /> | |
| </div> | |
| </div> | |
| ); | |
| return ( | |
| <div aria-busy="true" className="flex gap-6"> | |
| <div className="w-[16rem] shrink-0 space-y-2"> | |
| <Skeleton className="h-9 w-full rounded-md" /> | |
| <Skeleton className="h-9 w-full rounded-md" /> | |
| <Skeleton className="h-9 w-full rounded-md" /> | |
| </div> | |
| <div className="flex-1 space-y-3"> | |
| <Skeleton className="h-6 w-48" /> | |
| <Skeleton className="h-4 w-72" /> | |
| <Skeleton className="h-10 w-full" /> | |
| </div> | |
| </div> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/routes/dashboard/`$id/settings/governance.tsx around
lines 191 - 204, The route-level loading wrapper (the top-level <div
className="flex gap-6"> returned in the loading branch) should include the
accessibility attribute aria-busy="true"; update the JSX returned in the loading
state to add aria-busy="true" on that container (the div with className "flex
gap-6") so assistive technologies are informed that the region is loading.
Comment on lines
+138
to
+143
| const userIds = rawEntries.map((e) => e.userId); | ||
| const userNameMap = await getUserNamesBatch(ctx, userIds); | ||
|
|
||
| const entries = rawEntries.map((e) => | ||
| Object.assign(e, { displayName: userNameMap.get(e.userId) ?? e.userId }), | ||
| ); |
Contributor
There was a problem hiding this comment.
Add a guardrail for user-name lookup fan-out.
Line 139 can trigger one internal user lookup per unique userId; on large monthly ledgers this can create high parallel query pressure and degrade getUsageSummary reliability/latency. Add a threshold/concurrency guard and fall back to userId when exceeded.
🔧 Proposed defensive change
- const userIds = rawEntries.map((e) => e.userId);
- const userNameMap = await getUserNamesBatch(ctx, userIds);
+ const uniqueUserIds = [...new Set(rawEntries.map((e) => e.userId).filter(Boolean))];
+ const MAX_USER_NAME_LOOKUPS = 500;
+ const userNameMap =
+ uniqueUserIds.length <= MAX_USER_NAME_LOOKUPS
+ ? await getUserNamesBatch(ctx, uniqueUserIds)
+ : new Map<string, string>();As per coding guidelines, **/convex/**/*.{ts,tsx}: Consider to use rate limiting and action caching in Convex.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const userIds = rawEntries.map((e) => e.userId); | |
| const userNameMap = await getUserNamesBatch(ctx, userIds); | |
| const entries = rawEntries.map((e) => | |
| Object.assign(e, { displayName: userNameMap.get(e.userId) ?? e.userId }), | |
| ); | |
| const uniqueUserIds = [...new Set(rawEntries.map((e) => e.userId).filter(Boolean))]; | |
| const MAX_USER_NAME_LOOKUPS = 500; | |
| const userNameMap = | |
| uniqueUserIds.length <= MAX_USER_NAME_LOOKUPS | |
| ? await getUserNamesBatch(ctx, uniqueUserIds) | |
| : new Map<string, string>(); | |
| const entries = rawEntries.map((e) => | |
| Object.assign(e, { displayName: userNameMap.get(e.userId) ?? e.userId }), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/governance/queries.ts` around lines 138 - 143, The
user-name batch lookup can fan out too many internal queries when rawEntries has
many unique userIds; in getUsageSummary (or the code that builds
userIds/rawEntries) add a guard: compute unique userIds from rawEntries, and if
the count exceeds a safe threshold constant (e.g. MAX_USER_NAME_LOOKUPS), skip
calling getUserNamesBatch and set displayName = userId for those entries,
otherwise call getUserNamesBatch; alternatively implement chunked lookups with
bounded concurrency (e.g. Promise.all on chunks) and fall back to userId on
errors or when threshold exceeded; update the code around rawEntries.map,
userIds, getUserNamesBatch and entries to use this behavior.
Avoid mutating rawEntries in getUsageSummary by using Object.assign with an empty target. Fix empty-string name fallback in getUserNamesBatch so emails are shown when name is blank. Add aria-busy to the governance page skeleton for screen reader consistency. Truncate long display names in the usage table User column to prevent layout overflow.
Keep showing skeleton until server data has been applied to local state, avoiding the visual flash where controls briefly show default values before syncing to the actual saved configuration.
…init Replace useEffect-based state sync with render-time initialization using useRef. React discards the render and retries with new state before paint, so the form is never shown with stale default values. This prevents the brief checkbox/switch/input flash when switching between governance tabs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
getUserNamesBatchhelper in thegetUsageSummaryquery, so the User column shows readable emails instead of raw Convex IDsreturn nullloading guards withSkeletonplaceholders across all 9 governance editor components and the route-level ability check, matching the existing pattern infeature-flags-editor.tsxTest plan
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements