-
Notifications
You must be signed in to change notification settings - Fork 121
Enterprise 260207-001 #530
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Replace hardcoded base URL construction with a new `buildTenantSiteBaseUrl` function to dynamically generate tenant site URLs based on the campaign's `www_name` and `www_route_path`. This change ensures the use of canonical domains when available and improves URL handling in both the invite and refresh routes. Additionally, add unit tests for the new URL building logic to ensure correctness across various scenarios.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughRefactors tenant base-URL construction with canonical-domain resolution, replaces manual URL assembly in invite/refresh routes, adds buildTenantSiteBaseUrl and its tests, introduces a FormView translation context with lang propagation, tightens i18n Translation shape to require previously-optional keys, and threads locale through the invitation signup flow. Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Server
participant Builder as buildTenantSiteBaseUrl()
participant RPC as Service-Role RPC
participant DB as Database
Client->>Builder: buildTenantSiteBaseUrl({www_name, www_route_path, hosted, prefer_canonical})
alt hosted = false
Builder->>Client: http://{www_name}.localhost:3000{route}
else hosted = true
Builder->>Builder: platformHost = platformSiteHostnameForTenant(www_name)
alt prefer_canonical = true
Builder->>RPC: www_get_canonical_hostname(www_name)
RPC->>DB: query canonical hostname
DB-->>RPC: canonical hostname or null
RPC-->>Builder: canonical hostname or null
alt canonical available
Builder->>Client: https://{canonicalHost}{route}
else fallback
Builder->>Client: https://{platformHost}{route}
end
else prefer_canonical = false
Builder->>Client: https://{platformHost}{route}
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Refactor the form view to utilize shared i18n resources for dynamic translation based on the selected language. Introduce a context provider for translations, allowing components to access localized strings. Update the form components to use the new translation context, ensuring that the UI is properly localized. Additionally, modify the invitation page template to pass the locale to the form view provider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@editor/lib/tenant-url.ts`:
- Around line 43-48: normalizeRoutePath currently preserves trailing slashes
(e.g., "/west/") which violates the "no trailing /" invariant; update the
function (normalizeRoutePath) to trim whitespace, then remove any trailing
slashes before further processing, and ensure the returned path has at most one
leading slash and no trailing slash (treat "/" or empty input as returning "").
Implement this by rtrim-ing trailing "/" characters from the intermediate raw
value (or using a regex) and then returning raw.startsWith("/") ? raw :
`/${raw}` after the trailing-slash removal.
- Around line 50-56: The JSDoc block above the tenant URL builder contains raw
escape artifacts like "\n+" that should be removed; edit the doc comment in
editor/lib/tenant-url.ts (the comment describing "Builds the **tenant site base
URL**...") to eliminate the "\n+" sequences and reformat the example list into
plain JSDoc lines (e.g., each example as " - `https://acme.example.com/west`")
while keeping the intended content and note about no trailing slash and usage;
ensure the comment is valid JSDoc without raw escape characters so hover
tooltips and generated docs render cleanly.
🧹 Nitpick comments (4)
editor/app/(api)/(public)/v1/west/t/invite/route.ts (1)
8-8:IS_HOSTEDis duplicated across route files; consider importing fromeditor/env.ts.Both
invite/route.tsandrefresh/route.tsdefineconst IS_HOSTED = process.env.VERCEL === "1"locally, whileeditor/env.tsalready exports the same constant. Using the shared export avoids drift if the check ever changes.editor/lib/tenant-url.test.ts (2)
34-45: Duplicate test — identical to the test on lines 20–32 minus the RPC assertion.This test uses the exact same inputs and expected output as the one above. Consider removing it or differentiating the scenario (e.g., test with
prefer_canonical: falsein local dev).
113-144: Consider adding a test case for a trailing-slash route path (e.g."/west/").This would document the expected behavior and guard the "no trailing slash" invariant. Currently
normalizeRoutePathdoesn't strip trailing slashes, so adding this test would either confirm the gap or serve as a regression guard once fixed.editor/theme/templates/enterprise/west-referral/invitation/page.tsx (1)
410-414: Missing dependency inuseEffectcleanup.
clearSessionStorageis referenced inside the effect but omitted from the dependency array. With React 19's strict mode this effect fires twice in dev, which is fine for cleanup, but the stale closure is still a concern ifclearSessionStorageidentity ever changes. This appears to be pre-existing, but worth noting since you're touching adjacent code.useEffect(() => { return () => { clearSessionStorage(); }; - }, []); + }, [clearSessionStorage]);
Summary by CodeRabbit
New Features
Refactor
Tests