Implement universal docs routing feature#533
Conversation
- Introduced a new routing system that resolves `/_/<path>` shorthand URLs to tenant-specific canonical paths, enhancing documentation navigation. - Added a new `UniversalRoutePicker` component to handle user context selection for projects and documents. - Updated the routing configuration in `next.config.ts` to support universal routing. - Created tests for the routing functionality to ensure unique matches and consistent path normalization. - Documented the universal routing model and its resolution algorithm in `universal-docs-routing.md` for clarity and future reference.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
WalkthroughAdds universal docs routing: spec docs, a route registry and URL utilities, a Next.js rewrite, a server-side UniversalRoutePicker, unit tests, UI item components, doc updates, and a few import path tweaks. No behavioral changes outside routing additions. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UniversalRoutePicker as "UniversalRoutePicker\n(editor app)"
participant RouteRegistry as "Route Registry\n(editor/host/url)"
participant Auth as "Auth (Supabase)"
participant Database as "Database (Supabase)"
participant UI as "Browser UI"
User->>UniversalRoutePicker: GET /_/<path>
UniversalRoutePicker->>RouteRegistry: normalizeUniversalPath & matchUniversalRoute(path)
RouteRegistry-->>UniversalRoutePicker: matched route or no match
UniversalRoutePicker->>Auth: getUser()
alt Authenticated
Auth-->>UniversalRoutePicker: user session
alt route.scope == "project"
UniversalRoutePicker->>Database: query org memberships & projects
Database-->>UniversalRoutePicker: accessible projects
UniversalRoutePicker->>UI: render project selector (links)
else route.scope == "document"
UniversalRoutePicker->>Database: query documents (filter by doctype if any)
Database-->>UniversalRoutePicker: documents list
UniversalRoutePicker->>UI: render document list (Open links)
end
else Unauthenticated
UniversalRoutePicker-->>User: redirect to sign-in (with next)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/forms/respondent-email-notifications.md`:
- Around line 28-29: The docs currently use universal routing links (/ _/connect
and / _/connect/channels) which will redirect unauthenticated readers to sign-in
via UniversalRoutePicker; change these to public-friendly navigation
instructions by replacing the universal-prefixed URLs with either the previous
plain-text steps (e.g., "In the left sidebar, click Connect → Channels") or with
non-universal absolute paths that don’t use the /_/ prefix, and optionally
include both a universal link (for logged-in users) plus a short parenthetical
note telling unauthenticated readers that the link requires sign-in and to
instead follow the plain navigation steps.
In `@editor/app/`(workspace)/universal/[[...path]]/page.tsx:
- Around line 49-59: The Supabase queries currently only destructure { data:
memberships } (and the other query around line 142) which drops the error object
and hides failures; update both queries to destructure { data, error } (e.g.,
for the client.from("organization_member") call that assigns memberships) then
check for error—log it (console.error or your app logger) and surface a
user-facing error (throw/return an error response or set an error state) instead
of silently showing "No projects/documents available". Ensure you reference and
handle the error for the specific queries that assign memberships and the
document/projects query so failures are visible.
- Around line 122-142: The document query built in docQuery
(client.from("document").select(...).order("updated_at", ...)) has no LIMIT and
likewise the earlier membership query is unbounded; add a reasonable .limit(50)
(or .limit(100)) to both queries and wire a pagination/"show more" mechanism
(e.g., offset/limit or cursor-based paging) so subsequent requests fetch the
next page; ensure the ordering (order("updated_at", ...)) is preserved when
paginating and update the frontend to request additional pages rather than
loading all rows at once.
In `@editor/host/url.ts`:
- Around line 279-293: buildUniversalDestination can insert literal "null" or
produce double slashes when context.docId is null or empty; update the docId
handling so you only include the docId path segment when it is a non-empty
string: in buildUniversalDestination (use getUniversalRouteDefinition,
normalizeUniversalPath, route.scope) replace the current docId assignment with a
guard like: set docId to undefined unless ("docId" in context) && typeof
context.docId === "string" && context.docId.trim() !== "" and then build the
return strings to omit the /{docId} segment when docId is undefined so you never
append "null" or an empty segment before the suffix.
🧹 Nitpick comments (4)
editor/host/url.test.ts (1)
1-5: Missing explicit vitest imports.The sibling test file
editor/host/tenant-url.test.tsexplicitly importsdescribe,expect,testfrom"vitest"(line 1), but this file usesdescribe,it,expectas implicit globals. For consistency (and to avoid breakage if vitest globals config changes), consider adding the import.Suggested fix
+import { describe, it, expect } from "vitest"; import { matchUniversalRoute, normalizeUniversalPath, universalRoutes, } from "@/host/url";docs/wg/platform/universal-docs-routing.md (1)
14-16: Add language identifiers to fenced code blocks to satisfy markdownlint (MD040).The URL-pattern blocks can use
```textand the uniqueness test sketch can use```ts.Proposed fix
Line 14:
-``` +```textLine 20:
-``` +```textLine 57:
-``` +```textLine 109:
-``` +```tsAlso applies to: 20-22, 57-59, 109-115
editor/app/(workspace)/universal/[[...path]]/page.tsx (2)
138-140: The.in()return value is discarded — works due to Supabase's mutable builder, but the pattern is fragile.Supabase's
PostgrestFilterBuildermutatesthisand returnsthis, so discarding the return value ofdocQuery.in(...)is technically safe. However, chaining it or reassigning would be more idiomatic and resilient to future library changes.Suggested change
if (route.requiredDoctypes?.length) { - docQuery.in("doctype", route.requiredDoctypes); + docQuery.in("doctype", route.requiredDoctypes as string[]); }Or better, chain the filter into the query construction:
const docQuery = client .from("document") .select(/* ... */) .order("updated_at", { ascending: false }); // Apply optional filter and always use the returned builder const filteredQuery = route.requiredDoctypes?.length ? docQuery.in("doctype", route.requiredDoctypes as string[]) : docQuery; const { data: documents } = await filteredQuery;
76-119: The two rendering branches (project picker / document picker) are nearly identical in structure.Both branches render the same layout pattern (header → empty state → list of links). Consider extracting a shared
PickerLayoutcomponent to reduce duplication and make future UI changes easier. This is non-blocking for now.Also applies to: 159-206
| 2. In the left sidebar, click [**Connect**](https://grida.co/_/connect). | ||
| 3. Click [**Channels**](https://grida.co/_/connect/channels). |
There was a problem hiding this comment.
Universal routing links in docs — verify they work for readers without auth context.
These links use the new /_/ universal routing prefix (https://grida.co/_/connect and https://grida.co/_/connect/channels). Per the PR's routing spec, universal routes require context resolution (org/project), and the UniversalRoutePicker page redirects unauthenticated users to sign-in.
For documentation readers who aren't logged in or land on this page externally, these links will redirect to sign-in rather than showing the referenced UI. Consider whether plain text navigation steps (the previous approach) or a mix of both would be more user-friendly for docs that are publicly accessible.
🤖 Prompt for AI Agents
In `@docs/forms/respondent-email-notifications.md` around lines 28 - 29, The docs
currently use universal routing links (/ _/connect and / _/connect/channels)
which will redirect unauthenticated readers to sign-in via UniversalRoutePicker;
change these to public-friendly navigation instructions by replacing the
universal-prefixed URLs with either the previous plain-text steps (e.g., "In the
left sidebar, click Connect → Channels") or with non-universal absolute paths
that don’t use the /_/ prefix, and optionally include both a universal link (for
logged-in users) plus a short parenthetical note telling unauthenticated readers
that the link requires sign-in and to instead follow the plain navigation steps.
| const { data: memberships } = await client | ||
| .from("organization_member") | ||
| .select( | ||
| ` | ||
| organization:organization( | ||
| name, | ||
| projects:project(name) | ||
| ) | ||
| ` | ||
| ) | ||
| .eq("user_id", auth.user.id); |
There was a problem hiding this comment.
Supabase query errors are silently swallowed.
Both queries destructure only { data }, discarding the error field. If a query fails (network issue, RLS misconfiguration, etc.), the user sees "No projects/documents available" with no indication of the real problem.
At minimum, log the error or surface a user-facing error state.
Example for the document query
- const { data: documents } = await docQuery;
+ const { data: documents, error } = await docQuery;
+ if (error) {
+ console.error("Failed to fetch documents", error);
+ // Optionally: throw or render an error state
+ }Also applies to: 142-142
🤖 Prompt for AI Agents
In `@editor/app/`(workspace)/universal/[[...path]]/page.tsx around lines 49 - 59,
The Supabase queries currently only destructure { data: memberships } (and the
other query around line 142) which drops the error object and hides failures;
update both queries to destructure { data, error } (e.g., for the
client.from("organization_member") call that assigns memberships) then check for
error—log it (console.error or your app logger) and surface a user-facing error
(throw/return an error response or set an error state) instead of silently
showing "No projects/documents available". Ensure you reference and handle the
error for the specific queries that assign memberships and the document/projects
query so failures are visible.
| const docQuery = client | ||
| .from("document") | ||
| .select( | ||
| ` | ||
| id, | ||
| title, | ||
| doctype, | ||
| updated_at, | ||
| project:project( | ||
| name, | ||
| organization:organization(name) | ||
| ) | ||
| ` | ||
| ) | ||
| .order("updated_at", { ascending: false }); | ||
|
|
||
| if (route.requiredDoctypes?.length) { | ||
| docQuery.in("doctype", route.requiredDoctypes); | ||
| } | ||
|
|
||
| const { data: documents } = await docQuery; |
There was a problem hiding this comment.
Document query fetches all matching rows with no pagination or limit.
Both the document query here and the membership query (Line 49) lack any LIMIT. For users with many organizations/projects or large document sets, this will fetch unbounded result sets, causing latency and memory pressure on the server component.
Consider adding a reasonable .limit() (e.g., 50–100) and a "show more" or pagination mechanism.
Minimal fix — add a limit
)
- .order("updated_at", { ascending: false });
+ .order("updated_at", { ascending: false })
+ .limit(100);🤖 Prompt for AI Agents
In `@editor/app/`(workspace)/universal/[[...path]]/page.tsx around lines 122 -
142, The document query built in docQuery
(client.from("document").select(...).order("updated_at", ...)) has no LIMIT and
likewise the earlier membership query is unbounded; add a reasonable .limit(50)
(or .limit(100)) to both queries and wire a pagination/"show more" mechanism
(e.g., offset/limit or cursor-based paging) so subsequent requests fetch the
next page; ensure the ordering (order("updated_at", ...)) is preserved when
paginating and update the frontend to request additional pages rather than
loading all rows at once.
| export function buildUniversalDestination( | ||
| page: UniversalRouteType, | ||
| context: UniversalRouteContext | ||
| ) { | ||
| const route = getUniversalRouteDefinition(page); | ||
| const base = `/${context.org}/${context.proj}`; | ||
| const suffix = normalizeUniversalPath(route.path); | ||
|
|
||
| if (route.scope === "project") { | ||
| return suffix ? `${base}/${suffix}` : base; | ||
| } | ||
|
|
||
| const docId = "docId" in context ? context.docId : ""; | ||
| return suffix ? `${base}/${docId}/${suffix}` : `${base}/${docId}`; | ||
| } |
There was a problem hiding this comment.
buildUniversalDestination can produce malformed URLs when docId is null or empty.
The loose overload accepts docId?: string | null. Two edge cases:
docIdisnull→"docId" in contextistrue, socontext.docIdevaluates tonull, producing/<org>/<proj>/null/<suffix>.docIdis""→ produces/<org>/<proj>//<suffix>(double slash).
Both are reachable at runtime when the function is called via the loose overload with a dynamic UniversalRouteType.
Proposed fix
- const docId = "docId" in context ? context.docId : "";
- return suffix ? `${base}/${docId}/${suffix}` : `${base}/${docId}`;
+ const docId = "docId" in context && context.docId ? context.docId : null;
+ if (!docId) {
+ throw new Error(
+ `buildUniversalDestination: document-scoped route "${page}" requires a docId`
+ );
+ }
+ return suffix ? `${base}/${docId}/${suffix}` : `${base}/${docId}`;🤖 Prompt for AI Agents
In `@editor/host/url.ts` around lines 279 - 293, buildUniversalDestination can
insert literal "null" or produce double slashes when context.docId is null or
empty; update the docId handling so you only include the docId path segment when
it is a non-empty string: in buildUniversalDestination (use
getUniversalRouteDefinition, normalizeUniversalPath, route.scope) replace the
current docId assignment with a guard like: set docId to undefined unless
("docId" in context) && typeof context.docId === "string" &&
context.docId.trim() !== "" and then build the return strings to omit the
/{docId} segment when docId is undefined so you never append "null" or an empty
segment before the suffix.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@editor/components/ui/item.tsx`:
- Around line 8-16: ItemGroup sets role="list" but its child component Item does
not provide the complementary role, breaking accessibility semantics; update the
Item component (the component rendered as children of ItemGroup) to include
role="listitem" on the top-level element it returns (or accept/pass a prop to
set role) so direct children of ItemGroup have role="listitem" and preserve any
existing className/props handling in Item.
- Line 34: The CSS string in the Item component uses the wrong Tailwind
arbitrary selector `[a]:hover:bg-accent/50` and `[a]:transition-colors`, which
target an attribute named "a" instead of descendant anchor elements; update
those selectors to use the descendant selector syntax
(`[&_a]:hover:bg-accent/50` and `[&_a]:transition-colors`) wherever the class
string is defined (the long class string in the Item component at the top of
editor/components/ui/item.tsx) so descendant <a> elements receive the hover and
transition styles.
🧹 Nitpick comments (1)
editor/components/ui/item.tsx (1)
43-43: Trailing whitespace in size variant string.
"gap-4 p-4 "has a trailing space — harmless but inconsistent with other variant strings.Fix
- default: "gap-4 p-4 ", + default: "gap-4 p-4",
- Introduced new routes for project and document scopes, including "ciam/portal", "new/event", "new/referral", and various form-related routes. - Enhanced the connect section with additional routes for integrations and store management, improving overall routing capabilities.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@editor/host/url.ts`:
- Around line 311-320: The function resolve_next currently uses a falsy check
(if (!uri)) which causes infinite recursion when uri is null and fallback is the
empty string; change the guard to check only for null/undefined (e.g., if (uri
== null) or if (uri === undefined || uri === null)) so resolve_next(origin,
fallback) is only invoked when uri is missing, allowing an empty-string fallback
to be handled normally; update the check in resolve_next and keep the rest
(isAbsolute regex and new URL(uri, origin)) unchanged.
🧹 Nitpick comments (2)
editor/host/url.ts (2)
256-266:matchUniversalRoutere-normalizes every route path on each call.
universalRoutesis pre-built at module load, butmatchUniversalRoutecallsnormalizeUniversalPath(route.path)for every route on every invocation. Since the paths are static, you could store the normalized form inUniversalRouteDefinitiononce and compare directly.♻️ Pre-normalize in the definition
export function getUniversalRouteDefinition( id: UniversalRouteType ): UniversalRouteDefinition { const config: UniversalRouteConfig = UNIVERSAL_ROUTE_CONFIGS[id]; - const path = config.path ?? id; + const path = normalizeUniversalPath(config.path ?? id); return { id, path, scope: config.scope, requiredDoctypes: config.requiredDoctypes, samplePath: path, }; } export function matchUniversalRoute(path: string) { const normalized = normalizeUniversalPath(path); - return universalRoutes.filter( - (route) => normalizeUniversalPath(route.path) === normalized - ); + return universalRoutes.filter((route) => route.path === normalized); }
398-406:as anyescape hatch forCHALLENGE_EMAIL_NOT_VERIFIED.Line 405 uses
as anybecause thebadrequestparams type inFormLinkURLParamsdoesn't include anerrorfield. This silently bypasses type safety. Consider extendingFormLinkURLParams["badrequest"]to accommodate theerrorparameter properly, which would also benefit the otherbadrequestcases (lines 369-375).♻️ Type the badrequest params explicitly
- badrequest?: {}; + badrequest: { error?: string };Then the
as anycast on line 405 can be removed:- return formlink(host, form_id, "badrequest", { - error: "CHALLENGE_EMAIL_NOT_VERIFIED", - } as any); + return formlink(host, form_id, "badrequest", { + error: "CHALLENGE_EMAIL_NOT_VERIFIED", + });
| export function resolve_next( | ||
| origin: string, | ||
| uri?: string | null, | ||
| fallback = "/" | ||
| ) { | ||
| if (!uri) return resolve_next(origin, fallback); | ||
| // Check if the URL is absolute | ||
| const isAbsolute = /^https?:\/\//i.test(uri); | ||
|
|
||
| // If the URL is absolute, return it as is | ||
| if (isAbsolute) { | ||
| return uri; | ||
| } | ||
|
|
||
| // If the URL is relative, combine it with the origin | ||
| const combinedUri = new URL(uri, origin).toString(); | ||
|
|
||
| return combinedUri; | ||
| if (isAbsolute) return uri; | ||
| return new URL(uri, origin).toString(); | ||
| } |
There was a problem hiding this comment.
resolve_next could infinitely recurse if called with uri = null and fallback = "".
Line 316 recurses when !uri is true. If a caller passes an explicit empty-string fallback (resolve_next(origin, null, "")), the recursion never terminates because "" is also falsy. The default "/" is safe, but the function signature allows the unsafe call.
🛡️ Guard against infinite recursion
export function resolve_next(
origin: string,
uri?: string | null,
fallback = "/"
) {
- if (!uri) return resolve_next(origin, fallback);
+ if (!uri) return resolve_next(origin, fallback || "/");
const isAbsolute = /^https?:\/\//i.test(uri);📝 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.
| export function resolve_next( | |
| origin: string, | |
| uri?: string | null, | |
| fallback = "/" | |
| ) { | |
| if (!uri) return resolve_next(origin, fallback); | |
| // Check if the URL is absolute | |
| const isAbsolute = /^https?:\/\//i.test(uri); | |
| // If the URL is absolute, return it as is | |
| if (isAbsolute) { | |
| return uri; | |
| } | |
| // If the URL is relative, combine it with the origin | |
| const combinedUri = new URL(uri, origin).toString(); | |
| return combinedUri; | |
| if (isAbsolute) return uri; | |
| return new URL(uri, origin).toString(); | |
| } | |
| export function resolve_next( | |
| origin: string, | |
| uri?: string | null, | |
| fallback = "/" | |
| ) { | |
| if (!uri) return resolve_next(origin, fallback || "/"); | |
| const isAbsolute = /^https?:\/\//i.test(uri); | |
| if (isAbsolute) return uri; | |
| return new URL(uri, origin).toString(); | |
| } |
🤖 Prompt for AI Agents
In `@editor/host/url.ts` around lines 311 - 320, The function resolve_next
currently uses a falsy check (if (!uri)) which causes infinite recursion when
uri is null and fallback is the empty string; change the guard to check only for
null/undefined (e.g., if (uri == null) or if (uri === undefined || uri ===
null)) so resolve_next(origin, fallback) is only invoked when uri is missing,
allowing an empty-string fallback to be handled normally; update the check in
resolve_next and keep the rest (isAbsolute regex and new URL(uri, origin))
unchanged.
1 participant
/_/<path>shorthand URLs to tenant-specific canonical paths, enhancing documentation navigation.New Features
Documentation
Tests