Fix OAuth Apps layout #2967
Fix OAuth Apps layout #2967
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds an OAuth Apps layout and a Create OAuth App client component with a permission check; simplifies several OAuth Apps page client layouts and adjusts minor spacing on the app details page. Changes
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant OAuthAppsLayout
participant CreateOAuthAppButton
participant PermissionCheck
User->>Browser: Navigate to /{slug}/settings/oauth-apps
Browser->>OAuthAppsLayout: Render layout + controls
OAuthAppsLayout->>CreateOAuthAppButton: Mount component
CreateOAuthAppButton->>PermissionCheck: clientAccessCheck("oauth_apps.write", role)
alt permission granted
PermissionCheck-->>CreateOAuthAppButton: no error
CreateOAuthAppButton->>Browser: Render enabled Link/Button -> /{slug}/settings/oauth-apps/new
User->>Browser: Click -> navigate to /new
else permission denied
PermissionCheck-->>CreateOAuthAppButton: permissionsError
CreateOAuthAppButton->>Browser: Render disabled Button with tooltip (permissionsError)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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 (2)
apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/new/page-client.tsx (1)
18-20: Consider a more specific redirect target.The redirect goes to the generic settings page. Consider redirecting to
/${slug}/settings/oauth-appsinstead, so users land on the OAuth Apps listing page where they can see existing apps (if any) even without write permissions.if (permissionsError) { - redirect(`/${slug}/settings`); + redirect(`/${slug}/settings/oauth-apps`); }apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/create-oauth-app-button.tsx (1)
22-30: Consider conditional prop spreading for consistency.The
disabledTooltipprop receivespermissionsErrordirectly, which can befalseor a string. For consistency with the pattern used inAddOAuthAppForm(line 596 fromadd-edit-app-form.tsx), consider using conditional spreading to only pass the prop when there's an actual error.<Link href={`/${slug}/settings/oauth-apps/new`}> <Button className="flex h-10 items-center justify-center whitespace-nowrap rounded-lg border px-4 text-sm" text="Create OAuth App" - disabledTooltip={permissionsError} + {...(permissionsError && { + disabledTooltip: permissionsError, + })} /> </Link>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/create-oauth-app-button.tsx(1 hunks)apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/layout.tsx(1 hunks)apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/new/page-client.tsx(1 hunks)apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/page.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T01:05:43.266Z
Learnt from: steven-tey
PR: dubinc/dub#2958
File: apps/web/app/app.dub.co/(dashboard)/[slug]/settings/members/page-client.tsx:432-457
Timestamp: 2025-10-15T01:05:43.266Z
Learning: In apps/web/app/app.dub.co/(dashboard)/[slug]/settings/members/page-client.tsx, defer refactoring the custom MenuItem component (lines 432-457) to use the shared dub/ui MenuItem component to a future PR, as requested by steven-tey.
Applied to files:
apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/new/page-client.tsx
🧬 Code graph analysis (4)
apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/new/page-client.tsx (1)
apps/web/ui/oauth-apps/add-edit-app-form.tsx (1)
AddOAuthAppForm(43-599)
apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/create-oauth-app-button.tsx (1)
apps/web/lib/api/tokens/permissions.ts (1)
clientAccessCheck(41-65)
apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/page.tsx (1)
apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/page-client.tsx (1)
OAuthAppsPageClient(12-45)
apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/layout.tsx (2)
apps/web/ui/layout/page-content/index.tsx (1)
PageContent(10-39)apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/create-oauth-app-button.tsx (1)
CreateOAuthAppButton(9-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/page.tsx (1)
3-4: LGTM!The simplified page component correctly delegates to the client component. Layout responsibilities are now properly handled by the layout.tsx file, following Next.js App Router conventions.
apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/layout.tsx (1)
6-20: LGTM!The layout component follows Next.js App Router conventions and provides a clean separation of concerns. The PageContent structure with title, documentation link, and controls is well-organized.
apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/create-oauth-app-button.tsx (1)
18-20: LGTM!The pathname check correctly ensures the button only appears on the main OAuth Apps listing page and not on sub-pages like
/newor individual app pages.apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/new/page-client.tsx (1)
23-29: The double wrapping is intentional and serves distinct purposes—no changes needed.PageWidthWrapper constrains to
max-w-screen-xl(1280px) with padding and container query support, while MaxWidthWrapper also defaults tomax-w-screen-xlbut the page-client explicitly narrows it tomax-w-screen-lg(1024px). This creates a two-level layout hierarchy: the outerPageWidthWrapper(from the layout) provides app-wide width constraints, while the innerMaxWidthWrapper(in the page-client) intentionally constrains the form content to a narrower width with additional vertical spacing viaspace-y-6. This pattern is consistent across similar form pages in the codebase and functions as designed.
apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/create-oauth-app-button.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/create-oauth-app-button.tsx (2)
18-20: Consider a more precise pathname check.The
pathname.endsWith()check could theoretically match unintended paths. Given the route structure, a more explicit pattern might be clearer.- if (!pathname.endsWith("/settings/oauth-apps")) { + if (!pathname.match(/\/[^\/]+\/settings\/oauth-apps$/)) { return null; }
15-15: Use ES6 shorthand property syntax.action: "oauth_apps.write", - role: role, + role,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/[appId]/page-client.tsx(1 hunks)apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/create-oauth-app-button.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/[appId]/page-client.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/create-oauth-app-button.tsx (1)
apps/web/lib/api/tokens/permissions.ts (1)
clientAccessCheck(41-65)
🔇 Additional comments (2)
apps/web/app/app.dub.co/(dashboard)/[slug]/settings/oauth-apps/create-oauth-app-button.tsx (2)
13-16: Permission action is now correct.The previously flagged critical issue has been resolved—the permission check now correctly uses
"oauth_apps.write"instead of"webhooks.write", aligning with the other OAuth app components in the codebase.
22-30: Button not actually disabled despite permission check.The
disabledTooltipshows an error message when the user lacks permission, but theButtoncomponent is not disabled and theLinkwrapper remains clickable. Users withoutoauth_apps.writepermission can still navigate to the create page.Consider one of these approaches:
Option 1: Disable the button
<Link href={`/${slug}/settings/oauth-apps/new`}> <Button className="flex h-10 items-center justify-center whitespace-nowrap rounded-lg border px-4 text-sm" text="Create OAuth App" disabledTooltip={permissionsError} + disabled={!!permissionsError} /> </Link>Option 2: Conditionally render the Link
- <Link href={`/${slug}/settings/oauth-apps/new`}> - <Button - className="flex h-10 items-center justify-center whitespace-nowrap rounded-lg border px-4 text-sm" - text="Create OAuth App" - disabledTooltip={permissionsError} - /> - </Link> + {permissionsError ? ( + <Button + className="flex h-10 items-center justify-center whitespace-nowrap rounded-lg border px-4 text-sm" + text="Create OAuth App" + disabledTooltip={permissionsError} + disabled + /> + ) : ( + <Link href={`/${slug}/settings/oauth-apps/new`}> + <Button + className="flex h-10 items-center justify-center whitespace-nowrap rounded-lg border px-4 text-sm" + text="Create OAuth App" + /> + </Link> + )}⛔ Skipped due to learnings
Learnt from: TWilson023 PR: dubinc/dub#2936 File: apps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/settings/analytics/add-hostname-modal.tsx:28-34 Timestamp: 2025-10-08T21:33:23.553Z Learning: In the dub/ui Button component, when the `disabledTooltip` prop is set to a non-undefined value (e.g., a string), the button is automatically disabled. Therefore, it's not necessary to also add the same condition to the `disabled` prop—setting `disabledTooltip={permissionsError || undefined}` is sufficient to disable the button when there's a permissions error.
2 participants
Summary by CodeRabbit