Repository navigation
feat(frontend): /settings short link redirects to the scoped settings page - #5141
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
size:XL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughA new ChangesSettings Redirect Implementation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| let last = 0 | ||
| let m: RegExpExecArray | null | ||
| TOKEN_RE.lastIndex = 0 | ||
| while ((m = TOKEN_RE.exec(template)) !== null) { |
| "content-type": "application/json", | ||
| "content-length": Buffer.byteLength(payload), | ||
| }); | ||
| res.end(payload); |
| if (!isPlainObject(cursor[key])) cursor[key] = {}; | ||
| cursor = cursor[key] as Record<string, unknown>; | ||
| } | ||
| cursor[parts[parts.length - 1]] = value; |
dosubot
Bot
added
size:M
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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 (1)
web/ee/src/pages/settings/index.tsx (1)
1-3: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse the explicit OSS package path in
web/ee/src/pages/settings/index.tsx
@/oss/*in EE resolves throughweb/ee/src/*first, so this page should importSettingsRedirectfrom@agenta/oss/src/...to keep the OSS dependency explicit.🔧 Proposed fix
-import SettingsRedirect from "`@/oss/components/pages/SettingsRedirect`" +import SettingsRedirect from "`@agenta/oss/src/components/pages/SettingsRedirect`"Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 34ec8e4e-eae2-4dde-9d18-9042b719e27c
📒 Files selected for processing (3)
web/ee/src/pages/settings/index.tsxweb/oss/src/components/pages/SettingsRedirect/index.tsxweb/oss/src/pages/settings/index.tsx
| useEffect(() => { | ||
| if (!router.isReady) return | ||
| if (pendingRef.current) return | ||
|
|
||
| pendingRef.current = true | ||
|
|
||
| let cancelled = false | ||
| const searchIndex = router.asPath.indexOf("?") | ||
| const search = searchIndex >= 0 ? router.asPath.slice(searchIndex) : "" | ||
|
|
||
| const redirect = async (target: string | null) => { | ||
| if (!target || cancelled) return | ||
| const normalizedTarget = target.split("?")[0] | ||
| const latestPath = router.asPath.split("?")[0] | ||
| if (normalizedTarget === latestPath) return | ||
| await router.replace(target) | ||
| } | ||
|
|
||
| const resolveTarget = async (): Promise<string | null> => { | ||
| if (directPath) return `${directPath}${search}` | ||
|
|
||
| const context = await waitForWorkspaceContext({ | ||
| requireProjectId: true, | ||
| requireOrgData: true, | ||
| }) | ||
| if (cancelled) return null | ||
|
|
||
| const resolvedPath = await buildPostLoginPathResolved(context) | ||
| if (cancelled) return null | ||
|
|
||
| // Fully resolved paths end in /apps; swap the segment for /settings. | ||
| if (resolvedPath.endsWith("/apps")) { | ||
| return `${resolvedPath.slice(0, -"/apps".length)}/settings${search}` | ||
| } | ||
|
|
||
| // Partial context (/w or /w/<ws>) — hand off to the workspace selection flow. | ||
| return resolvedPath | ||
| } | ||
|
|
||
| void resolveTarget() | ||
| .then(redirect) | ||
| .finally(() => { | ||
| pendingRef.current = false | ||
| }) | ||
|
|
||
| return () => { | ||
| cancelled = true | ||
| } | ||
| }, [directPath, router, router.isReady]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
pendingRef isn't reset in cleanup — can permanently block the redirect.
pendingRef.current is only reset inside the async chain's .finally() (Line 64), not in the effect's cleanup (Lines 67-69). If directPath transitions from null to a real value (e.g. workspaceId/projectURL finish loading after mount) while the first invocation is still awaiting waitForWorkspaceContext, React runs cleanup for the old effect (setting cancelled = true) and then immediately invokes the new effect body. The new invocation hits if (pendingRef.current) return (Line 24) and exits early, since pendingRef.current is still true at that point. Meanwhile, the old chain's waitForWorkspaceContext eventually resolves, checks cancelled (now true), and returns null — so no redirect ever fires. No further dependency change occurs to retrigger the effect, leaving the spinner shown indefinitely.
This is a real risk on the very case this PR targets: cold navigation to /settings, where workspaceId/projectURL typically populate asynchronously after router.isReady becomes true.
🔧 Proposed fix: reset the guard synchronously in cleanup
return () => {
cancelled = true
+ pendingRef.current = false
}
}, [directPath, router, router.isReady])📝 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.
| useEffect(() => { | |
| if (!router.isReady) return | |
| if (pendingRef.current) return | |
| pendingRef.current = true | |
| let cancelled = false | |
| const searchIndex = router.asPath.indexOf("?") | |
| const search = searchIndex >= 0 ? router.asPath.slice(searchIndex) : "" | |
| const redirect = async (target: string | null) => { | |
| if (!target || cancelled) return | |
| const normalizedTarget = target.split("?")[0] | |
| const latestPath = router.asPath.split("?")[0] | |
| if (normalizedTarget === latestPath) return | |
| await router.replace(target) | |
| } | |
| const resolveTarget = async (): Promise<string | null> => { | |
| if (directPath) return `${directPath}${search}` | |
| const context = await waitForWorkspaceContext({ | |
| requireProjectId: true, | |
| requireOrgData: true, | |
| }) | |
| if (cancelled) return null | |
| const resolvedPath = await buildPostLoginPathResolved(context) | |
| if (cancelled) return null | |
| // Fully resolved paths end in /apps; swap the segment for /settings. | |
| if (resolvedPath.endsWith("/apps")) { | |
| return `${resolvedPath.slice(0, -"/apps".length)}/settings${search}` | |
| } | |
| // Partial context (/w or /w/<ws>) — hand off to the workspace selection flow. | |
| return resolvedPath | |
| } | |
| void resolveTarget() | |
| .then(redirect) | |
| .finally(() => { | |
| pendingRef.current = false | |
| }) | |
| return () => { | |
| cancelled = true | |
| } | |
| }, [directPath, router, router.isReady]) | |
| useEffect(() => { | |
| if (!router.isReady) return | |
| if (pendingRef.current) return | |
| pendingRef.current = true | |
| let cancelled = false | |
| const searchIndex = router.asPath.indexOf("?") | |
| const search = searchIndex >= 0 ? router.asPath.slice(searchIndex) : "" | |
| const redirect = async (target: string | null) => { | |
| if (!target || cancelled) return | |
| const normalizedTarget = target.split("?")[0] | |
| const latestPath = router.asPath.split("?")[0] | |
| if (normalizedTarget === latestPath) return | |
| await router.replace(target) | |
| } | |
| const resolveTarget = async (): Promise<string | null> => { | |
| if (directPath) return `${directPath}${search}` | |
| const context = await waitForWorkspaceContext({ | |
| requireProjectId: true, | |
| requireOrgData: true, | |
| }) | |
| if (cancelled) return null | |
| const resolvedPath = await buildPostLoginPathResolved(context) | |
| if (cancelled) return null | |
| // Fully resolved paths end in /apps; swap the segment for /settings. | |
| if (resolvedPath.endsWith("/apps")) { | |
| return `${resolvedPath.slice(0, -"/apps".length)}/settings${search}` | |
| } | |
| // Partial context (/w or /w/<ws>) — hand off to the workspace selection flow. | |
| return resolvedPath | |
| } | |
| void resolveTarget() | |
| .then(redirect) | |
| .finally(() => { | |
| pendingRef.current = false | |
| }) | |
| return () => { | |
| cancelled = true | |
| pendingRef.current = false | |
| } | |
| }, [directPath, router, router.isReady]) |
59ad253 to
8b69a97
Compare
|
Pushed a fix after live QA on the dev box: the first version's redirect never fired (bare routes have no workspace/project in |
|
✅ Action performedReview finished.
|
Context
Sharing a link to the settings page required knowing the workspace and project ids:
/w/<ws>/p/<proj>/settings?tab=apiKeys. A barehttps://<host>/settings?tab=apiKeysreturned a 404, so there was no short, memorable URL you could give someone to open their own API keys page.Changes
A bare
/settingsroute now resolves the visitor's workspace and project client-side and forwards to the scoped page, preserving the full query string.Before:
/settings?tab=apiKeysrenders a 404.After:
/settings?tab=apiKeysredirects to/w/<workspace_id>/p/<project_id>/settings?tab=apiKeys.The new
SettingsRedirectcomponent follows the same reactive pattern as the siblingWorkspaceRedirect: derive the target from atoms in a memo, redirect in an effect that re-runs as they populate. The project comes fromprojectAtom(last-used, then default, then first non-demo) and the workspace id from the project's ownworkspace_idfield. That last part matters:urlAtom's workspace and project ids are parsed from the URL, which a bare route does not have, so they stay empty here. Three new files: the component plus thin OSS and EE page entries atpages/settings/index.tsx.Note for future bare-route redirects: a first version used a one-shot async wait (
waitForWorkspaceContext) behind apendingRefguard. Under React StrictMode's double-mount the first run gets cancelled and the second bails on the guard, so the redirect never fires. The reactive pattern avoids that class of bug entirely.Scope / risk
Only
/settingsis covered; other bare paths (/observability,/playground, ...) still 404 on purpose. Playground URLs are app-scoped, so a bare path has no unambiguous target there. No existing route or resolver logic changed; the risk surface is the new page only. Logged-out visitors go through the normal auth flow first and land via the standard post-login redirect, not the deep link (known limitation, small follow-up if we want it).Tests
/settings?tab=apiKeysand bare/settingsboth redirect to the scoped settings page with the API Keys tab rendered; query string preserved./settings?tab=apiKeyslands on/auth?redirectToPath=%2Fsettings%3Ftab%3DapiKeys, and after login the normal post-login flow takes over.pnpm lint-fixpasses for OSS and EE.How to QA
Prerequisites: any running stack (local dev via
run.sh, or the Hetzner dev box), logged in.Steps:
https://<host>/settings?tab=apiKeysdirectly (paste in the address bar).Expected result: you land on
/w/<your-ws>/p/<your-project>/settings?tab=apiKeyswith the API Keys tab active. The project is your last-used one.Automated tests: none added; the component is a thin composition of already-tested selectors.
Edge cases: open the link in a fresh browser profile (logged out). You should get the login page, and after login land in your workspace normally (not on settings; see scope note).
https://claude.ai/code/session_01DjaJLpnLtnoRhgbYe7NTC3