fix: sdk service-account view#1574
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds a React Query-backed token cache and hook, updates service-account dialogs and tables with CSS-module styling and fixed column widths, replaces inline truncation with tooltip-based project display, updates several import paths to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/sdk/react/views-new/service-accounts/components/service-account-columns.tsx (1)
77-77:⚠️ Potential issue | 🟡 MinorEmpty column header lacks an accessible name.
header: ''produces a<th>with no accessible label for the actions column. Screen-reader users navigating column headers will hear an empty cell. Consider providing a visually-hidden label (e.g.,header: () => <VisuallyHidden>Actions</VisuallyHidden>) or anaria-labelon the cell, consistent with how the similar actions column is handled inmanage-project-access-dialog.tsx(which usesheader: 'Actions').
🧹 Nitpick comments (4)
web/sdk/react/views-new/service-accounts/components/projects-cell.tsx (1)
44-55: Tooltip fires even when no truncation occurs.The tooltip is rendered unconditionally for any non-empty
projectNames, so for service accounts with one or two short project names (no overflow) hovering the cell still pops a tooltip duplicating the visible text. Consider rendering the tooltip only when the trigger is actually truncated (e.g., comparescrollWidth > clientWidthon the text element, or skip the tooltip when there's only one project and it fits).web/sdk/react/views-new/service-accounts/hooks/useServiceUserTokens.ts (2)
57-69:freshTokensMapis read imperatively and won't trigger re-renders on its own.
queryClient.getQueryData(...)doesn't subscribe the component to that cache key. IfcacheFreshServiceUserToken(orremoveQueriesinclearFreshTokens) is called from a context that doesn't also mutatelistServiceUserTokens, this hook's consumer won't re-render and the mergedtokenswill appear stale.Today this happens to work because every caller in the PR also writes to the
listServiceUserTokenscache (which is subscribed viauseQuery), but it's a fragile coupling. Consider subscribing to the fresh-token cache too — e.g. via a dedicateduseQuerykeyed bygetFreshTokensCacheKey(id), oruseSyncExternalStoreover the QueryCache — so the merge stays correct regardless of how/when the fresh cache mutates.
83-109: Add generic type parameter togetQueryDatacalls for type safety.Lines 85 and 99 are missing generic type parameters. Without them,
currentDatais typed asunknown, makingcurrentData?.tokensunsafe. This is inconsistent with thegetQueryData<FreshTokensMap>(...)pattern already used elsewhere in this file (lines 34, 77, 112).♻️ Proposed change
- const currentData = queryClient.getQueryData(queryKey); + const currentData = + queryClient.getQueryData<MessageInitShape<typeof ListServiceUserTokensResponseSchema>>( + queryKey + );web/sdk/react/views-new/service-accounts/service-accounts-view.tsx (1)
276-276: Prefer a CSS class for the danger color.
style={{ color: 'var(--rs-color-foreground-danger-primary)' }}(also repeated at line 252 inservice-account-details-view.tsx) is fine functionally, but moving it intoservice-accounts-view.module.css(e.g. a.dangerMenuItemclass) would keep theming consistent with the rest of this file and avoid duplicating the literal CSS var.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 463cd448-e890-417e-b359-f50fb9aa1c66
📒 Files selected for processing (15)
web/sdk/react/views-new/service-accounts/components/add-service-account-dialog.tsxweb/sdk/react/views-new/service-accounts/components/delete-service-account-dialog.module.cssweb/sdk/react/views-new/service-accounts/components/delete-service-account-dialog.tsxweb/sdk/react/views-new/service-accounts/components/manage-project-access-dialog.tsxweb/sdk/react/views-new/service-accounts/components/projects-cell.module.cssweb/sdk/react/views-new/service-accounts/components/projects-cell.tsxweb/sdk/react/views-new/service-accounts/components/revoke-token-dialog.module.cssweb/sdk/react/views-new/service-accounts/components/revoke-token-dialog.tsxweb/sdk/react/views-new/service-accounts/components/service-account-columns.module.cssweb/sdk/react/views-new/service-accounts/components/service-account-columns.tsxweb/sdk/react/views-new/service-accounts/hooks/useServiceUserTokens.tsweb/sdk/react/views-new/service-accounts/service-account-details-view.module.cssweb/sdk/react/views-new/service-accounts/service-account-details-view.tsxweb/sdk/react/views-new/service-accounts/service-accounts-view.module.cssweb/sdk/react/views-new/service-accounts/service-accounts-view.tsx
Coverage Report for CI Build 25057152887Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage remained the same at 42.353%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
rohanchkrabrty
changed the title
3 participants
Summary
size={2}and the access dropdown disables while a row is loading.CopyButtonfor the generated key, and reveal it on container hover only.tokenvalue) in a side cache that survives the details-page refetch, merge into the displayed list, and clear on navigate-away or delete.views-new/service-accountsself-contained by copyinguseServiceUserTokensinto the feature folder and routing all its consumers there; legacyviews/api-keyshook left untouched for old views.