Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR introduces personal access token (PAT) management functionality to the Frontier client-demo application and React SDK by adding new routes, page components, navigation items, and multiple PAT-related view and dialog components for creating, updating, revoking, and displaying PATs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
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 |
Coverage Report for CI Build 24316973212Coverage remained the same at 41.606%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/apps/client-demo/src/pages/Settings.tsx (1)
53-56:⚠️ Potential issue | 🟡 MinorKeep “Personal Access Tokens” nav active on details pages.
With exact-path matching,
/settings/pats/:patIdwon’t mark the PAT nav item active.Proposed fix
const fullPath = `/${orgId}/settings/${item.path}`; - const isActive = location.pathname === fullPath; + const isActive = + location.pathname === fullPath || + location.pathname.startsWith(`${fullPath}/`);
🧹 Nitpick comments (1)
web/apps/client-demo/src/pages/settings/PatDetails.tsx (1)
13-14: De-duplicate identical navigation callbacks.Both props pass the same function; extracting one callback keeps this component cleaner.
Proposed refactor
export default function PatDetails() { const { orgId, patId } = useParams<{ orgId: string; patId: string }>(); const navigate = useNavigate(); + const navigateToPats = () => navigate(`/${orgId}/settings/pats`); if (!patId) return null; return ( <PATDetailsView patId={patId} - onNavigateToPats={() => navigate(`/${orgId}/settings/pats`)} - onDeleteSuccess={() => navigate(`/${orgId}/settings/pats`)} + onNavigateToPats={navigateToPats} + onDeleteSuccess={navigateToPats} /> ); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11dfd2ef-2ed3-4148-96eb-43c634d2c1de
📒 Files selected for processing (15)
web/apps/client-demo/src/Router.tsxweb/apps/client-demo/src/pages/Settings.tsxweb/apps/client-demo/src/pages/settings/PatDetails.tsxweb/apps/client-demo/src/pages/settings/Pats.tsxweb/sdk/react/index.tsweb/sdk/react/views-new/pat/components/pat-created-dialog.tsxweb/sdk/react/views-new/pat/components/pat-form-dialog.tsxweb/sdk/react/views-new/pat/components/regenerate-pat-dialog.tsxweb/sdk/react/views-new/pat/components/revoke-pat-dialog.tsxweb/sdk/react/views-new/pat/components/token-cell.tsxweb/sdk/react/views-new/pat/index.tsweb/sdk/react/views-new/pat/pat-details-view.module.cssweb/sdk/react/views-new/pat/pat-details-view.tsxweb/sdk/react/views-new/pat/pat-view.module.cssweb/sdk/react/views-new/pat/pat-view.tsx
| <Dialog.Header> | ||
| <Dialog.Title>Success</Dialog.Title> | ||
| </Dialog.Header> | ||
| <Dialog.Body> | ||
| <Flex direction="column" gap={7}> | ||
| <Text size="small"> | ||
| You've successfully added a new personal access token. Copy | ||
| the token now | ||
| </Text> |
There was a problem hiding this comment.
Use neutral copy in the shared success dialog.
This component is also used after regenerate from web/sdk/react/views-new/pat/pat-details-view.tsx, so “successfully added a new personal access token” is inaccurate there. Neutral wording avoids misleading users about whether the token was created or rotated.
There was a problem hiding this comment.
Agree. We need better copy.
There was a problem hiding this comment.
@paanSinghCoder, glad we're aligned! Here are two options depending on how much flexibility you want:
Option A — Neutral copy (simplest fix)
Change just the body text so it reads correctly in both the create and regenerate flows:
<Text size="small">
Your personal access token is ready. Make sure to copy it now — it won't be shown again.
</Text>Option B — Caller-supplied description prop (more flexible)
Add an optional description prop so each call-site can pass context-specific text, while falling back to neutral copy:
export interface PATCreatedDialogProps {
handle: ReturnType<typeof Dialog.createHandle<string>>;
onClose?: () => void;
description?: string;
}
const DEFAULT_DESCRIPTION =
"Your personal access token is ready. Make sure to copy it now — it won't be shown again.";
export function PATCreatedDialog({ handle, onClose, description }: PATCreatedDialogProps) {
// ...
<Text size="small">{description ?? DEFAULT_DESCRIPTION}</Text>
}Then callers can be specific:
- Create flow (
pat-view.tsx):description="Your new personal access token has been created. Copy it now — it won't be shown again." - Regenerate flow (
pat-details-view.tsx):description="Your personal access token has been regenerated. Copy it now — it won't be shown again."
Option B keeps the component reusable while still allowing precise messaging per context. Option A is the quickest path if a single neutral phrase is sufficient.
🧠 Learnings used
Learnt from: AmanGIT07
Repo: raystack/frontier PR: 1447
File: core/organization/service.go:326-328
Timestamp: 2026-03-12T12:10:03.670Z
Learning: In `raystack/frontier`, a PAT (Personal Access Token) belongs to exactly one org and `PAT.OrgID` is immutable after creation. For `ListByUser` in `core/organization/service.go`, using `PAT.OrgID` directly to intersect org results is intentional and safe; no SpiceDB `LookupResources` call for the PAT scope is needed here.
Learnt from: AmanGIT07
Repo: raystack/frontier PR: 1475
File: core/userpat/service.go:221-222
Timestamp: 2026-03-25T04:18:43.439Z
Learning: In `raystack/frontier` (`core/userpat/service.go`), `validateScopes` and `createProjectScopedPolicies` do not currently verify that caller-supplied project resource IDs belong to the PAT's org (`PAT.OrgID`). This is a known gap: a PAT for org A could be granted project policies on org B's projects. The fix (deferred to a separate PR) is to add a `ProjectService` interface dependency to the PAT service and validate that every project resource ID in a scope belongs to the PAT's org before any policy write.
| const handleTitleBlur = useCallback(async () => { | ||
| const title = getValues('title'); | ||
| if (!title || !orgId) return; | ||
|
|
||
| // In update mode, skip check if title is unchanged | ||
| if (isUpdateMode && title === initialData?.title) { | ||
| setTitleAvailable(true); | ||
| return; | ||
| } | ||
|
|
||
| setTitleChecking(true); | ||
| try { | ||
| const result = await checkTitle( | ||
| create(CheckCurrentUserPATTitleRequestSchema, { orgId, title }) | ||
| ); | ||
| setTitleAvailable(result?.available); | ||
| } catch { | ||
| // Ignore check failure — don't block the user | ||
| } finally { | ||
| setTitleChecking(false); | ||
| } | ||
| }, [getValues, orgId, checkTitle, isUpdateMode, initialData]); |
There was a problem hiding this comment.
Harden the async title-check state before gating submit on it.
A transient failure in checkCurrentUserPATTitle leaves titleAvailable non-true, and the submit button stays disabled even though the comment says “don't block the user”. This same flow can also apply a stale availability result from an older blur to the current title.
💡 Suggested direction
const handleTitleBlur = useCallback(async () => {
const title = getValues('title');
if (!title || !orgId) return;
// In update mode, skip check if title is unchanged
if (isUpdateMode && title === initialData?.title) {
setTitleAvailable(true);
return;
}
+ const checkedTitle = title;
setTitleChecking(true);
try {
const result = await checkTitle(
create(CheckCurrentUserPATTitleRequestSchema, { orgId, title })
);
- setTitleAvailable(result?.available);
+ if (getValues('title') === checkedTitle) {
+ setTitleAvailable(result?.available ?? true);
+ }
} catch {
- // Ignore check failure — don't block the user
+ if (getValues('title') === checkedTitle) {
+ setTitleAvailable(true);
+ }
} finally {
setTitleChecking(false);
}
}, [getValues, orgId, checkTitle, isUpdateMode, initialData]);Also applies to: 500-505
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
paanSinghCoder
left a comment
paanSinghCoder
left a comment
There was a problem hiding this comment.
Please update the UI with new design changes in Figma.
| @@ -0,0 +1,35 @@ | |||
| .section { | |||
| border: 1px solid var(--rs-color-border-base-primary); | |||
| border-radius: 4px; | |||
There was a problem hiding this comment.
Replace 4px with var(--rs-radius-2).
|
|
||
| .detailRow { | ||
| display: flex; | ||
| gap: 8px; |
There was a problem hiding this comment.
Replace 8px with --rs-space-3
|
|
||
| .detailLabel { | ||
| flex-shrink: 0; | ||
| min-width: 120px; |
There was a problem hiding this comment.
Replace 120px with --rs-space-17
| .chipGroup { | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
| gap: 4px; |
There was a problem hiding this comment.
Replace 4px with var(--rs-radius-2).
|
|
||
| .tokenIcon { | ||
| width: 48px; | ||
| height: 48px; |
There was a problem hiding this comment.
Replace height and width with --rs-space-11
| </> | ||
| )} | ||
|
|
||
| <PATFormDialog |
There was a problem hiding this comment.
Fix in Apsara: If the menu item text is too long, the UI is breaking. We will have to add an ellipsis and a fixed max-width for the popup.
| onRevoked={() => refetch()} | ||
| /> | ||
| </ViewContainer> | ||
| ); |
There was a problem hiding this comment.
There are a few design updates in Figma (Projects in Create PAT dialog). Please update.
| } from '@raystack/apsara-v1'; | ||
| import { useFrontier } from '../../../contexts/FrontierContext'; | ||
| import { DEFAULT_DATE_FORMAT } from '../../../utils/constants'; | ||
| import { handleConnectError } from '~/utils/error'; |
There was a problem hiding this comment.
Only handleConnectError imported with ~/utils/error and everything else in the same file uses relative paths. We can make this consistent,
3 participants
Summary