refactor(links): drizzle-zod link schemas, tightened routers, UI streamlining + cleanup#545
Conversation
… router access Move link and link-folder zod schemas into links.schemas.ts generated from the Drizzle tables via drizzle-orm/zod, and centralize access checks behind requireOrganizationId / requireLinkAccess / getLinkOrThrow helpers. Simplifies the routers and collapses the duplicated test setup.
Simplify the link sheet, folder sheet, QR code, and expiration picker components and the use-links hook against the tightened rpc link schemas.
A rejected readOgFonts() promise was cached permanently, so one transient font load failure broke OG image generation until restart. Clear the cache on error so the next request retries.
…eturns empty Ignore trailing empty path segments when deciding whether to mask, and fall back to '/' so a fully-masked path never serializes to an empty string.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
The latest updates on your projects. Learn more about Unkey Deploy
|
Greptile SummaryThis PR centralises link and link-folder Zod schemas into a new
Confidence Score: 3/5The refactor is largely clean, but getLinkOrThrow allows soft-deleted links to be read, updated, or re-deleted — a meaningful gap relative to the folder equivalent that guards against this. All the dashboard and tracker changes look safe. The main concern is in links.ts: the new getLinkOrThrow helper omits the isNull(links.deletedAt) guard that getFolderOrThrow already carries, so soft-deleted links remain reachable through get, update, and delete. The access-control helpers are also copy-pasted verbatim into both routers rather than shared. packages/rpc/src/routers/links.ts — getLinkOrThrow missing soft-delete filter; packages/rpc/src/routers/link-folders.ts — duplicate requireOrganizationId / requireLinkAccess definitions. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Client Request] --> B{Procedure Type}
B -->|list| C[requireOrganizationId]
B -->|get / update / delete| D[getLinkOrThrow]
C --> E[requireLinkAccess]
D --> F{link found?}
F -->|no| G[rpcError.notFound]
F -->|yes - deletedAt unchecked| E
E --> H[withWorkspace permission check]
H -->|denied| I[rpcError.forbidden]
H -->|allowed| J[DB operation]
J --> K[Return linkOutputSchema]
subgraph schemas["links.schemas.ts (new)"]
S1[createLinkSchema]
S2[updateLinkSchema]
S3[linkOutputSchema]
S4[linkFolderOutputSchema]
end
schemas -.->|imported by| D
schemas -.->|imported by| J
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Client Request] --> B{Procedure Type}
B -->|list| C[requireOrganizationId]
B -->|get / update / delete| D[getLinkOrThrow]
C --> E[requireLinkAccess]
D --> F{link found?}
F -->|no| G[rpcError.notFound]
F -->|yes - deletedAt unchecked| E
E --> H[withWorkspace permission check]
H -->|denied| I[rpcError.forbidden]
H -->|allowed| J[DB operation]
J --> K[Return linkOutputSchema]
subgraph schemas["links.schemas.ts (new)"]
S1[createLinkSchema]
S2[updateLinkSchema]
S3[linkOutputSchema]
S4[linkFolderOutputSchema]
end
schemas -.->|imported by| D
schemas -.->|imported by| J
Reviews (1): Last reviewed commit: "chore(repo): stop tracking dump.rdb (Red..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async function getLinkOrThrow(context: Context, id: string): Promise<LinkRow> { | ||
| const [link] = await context.db | ||
| .select() | ||
| .from(links) | ||
| .where(eq(links.id, id)) | ||
| .limit(1); | ||
|
|
||
| if (!link) { | ||
| throw rpcError.notFound("link", id); | ||
| } | ||
|
|
||
| return link; |
There was a problem hiding this comment.
getLinkOrThrow
getLinkOrThrow queries without isNull(links.deletedAt), so it returns soft-deleted records. The parallel getFolderOrThrow in link-folders.ts explicitly guards with and(eq(linkFolders.id, id), isNull(linkFolders.deletedAt)). Because the new helper is now used for get, update, and delete, a caller who knows a deleted link's ID can read its data back or trigger another update/soft-delete on an already-deleted row — producing misleading 200s instead of the expected 404.
| async function getLinkOrThrow(context: Context, id: string): Promise<LinkRow> { | |
| const [link] = await context.db | |
| .select() | |
| .from(links) | |
| .where(eq(links.id, id)) | |
| .limit(1); | |
| if (!link) { | |
| throw rpcError.notFound("link", id); | |
| } | |
| return link; | |
| async function getLinkOrThrow(context: Context, id: string): Promise<LinkRow> { | |
| const [link] = await context.db | |
| .select() | |
| .from(links) | |
| .where(and(eq(links.id, id), isNull(links.deletedAt))) | |
| .limit(1); |
| function requireOrganizationId( | ||
| organizationId: string | null | undefined | ||
| ): string { | ||
| if (!organizationId) { | ||
| throw rpcError.badRequest("Organization ID is required"); | ||
| } | ||
| return organizationId; | ||
| } | ||
|
|
||
| function requireLinkAccess( | ||
| context: Context, | ||
| organizationId: string, | ||
| permission: LinkPermission | ||
| ) { | ||
| const permissions: [LinkPermission] = [permission]; | ||
| return withWorkspace(context, { | ||
| organizationId, | ||
| resource: "link", | ||
| permissions, | ||
| }); | ||
| } |
There was a problem hiding this comment.
requireOrganizationId, requireLinkAccess, and the LinkPermission type are copy-pasted verbatim into both links.ts and link-folders.ts. The PR already centralised schemas into links.schemas.ts, so these shared access-control helpers are a natural fit for the same file (or a thin links.utils.ts). Keeping them in two places means a future permission-model change must be applied twice.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
The shared get/update/delete helper queried by id only, so a soft-deleted link could be read or re-mutated (200 instead of 404). Guard with isNull(links.deletedAt), matching getFolderOrThrow.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found and verified against the latest diff
Confidence score: 2/5
- In
apps/dashboard/app/(main)/links/_components/expiration-picker.tsx,withTimecan receive an emptytype="time"value and produce invalid Day.js state, which can crash the links UI at runtime when users clear the time input — guard empty/invalid time strings before splitting and keep the previous/default time when blank. - In
packages/rpc/src/routers/links.schemas.ts,createvalidatesexpiresAtasz.date()whileupdateexpects a date string format, so clients can pass one shape successfully in one route and fail in the other, causing avoidable API errors/regressions — align both schemas to one sharedexpiresAtcontract before merging. - In
apps/dashboard/app/(main)/links/_components/link-folder-sheet.tsx, whitespace-only folder names now submit as a silent no-op with no feedback, which looks like a broken form flow to end users — trim input and surface a validation message (or disable submit) when the trimmed name is empty. - There is noticeable duplication in
packages/rpc/src/routers/link-folders.tsvspackages/rpc/src/routers/links.ts(access helpers) and inapps/dashboard/app/(dby)/dby/og/brand.tsxvsapps/docs/lib/og.tsx(font-cache logic), plusapps/docs/lib/og.tsxretries font reads on every failure; merging as-is increases drift and repeated outages/perf churn if one copy diverges — centralize shared helpers and limit font-promise resets to transient errors.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/rpc/src/routers/link-folders.ts">
<violation number="1" location="packages/rpc/src/routers/link-folders.ts:25">
P2: The `requireOrganizationId` and `requireLinkAccess` helpers (along with the `LinkPermission` type) are duplicated identically in both `link-folders.ts` and `links.ts`. Since this PR's stated goal is to centralize access helpers, these should live in a shared module so that authorization changes only need to be made in one place and stay consistent across both routers.</violation>
</file>
<file name="apps/dashboard/app/(dby)/dby/og/brand.tsx">
<violation number="1" location="apps/dashboard/app/(dby)/dby/og/brand.tsx:24">
P2: The OG font-loading cache and its new rejection-reset behavior are maintained in two exact copies: this dashboard file and `apps/docs/lib/og.tsx` (the files are byte-for-byte identical). Because the fix had to be applied in both places, future changes to error handling, font paths, or returned font metadata can easily drift if one copy is updated without the other. Consider either extracting this into a shared package utility or adding cross-reference comments at the top of each file so future edits stay aligned.</violation>
</file>
<file name="apps/dashboard/app/(main)/links/_components/expiration-picker.tsx">
<violation number="1" location="apps/dashboard/app/(main)/links/_components/expiration-picker.tsx:95">
P1: The `withTime` helper crashes at runtime when the `type="time"` input value is an empty string. `time.split(":").map(Number)` on `""` yields `[NaN]`, so `minutes` is `undefined`; Day.js then interprets `.minute(undefined)` as a getter and returns a number, making the next `.second(0)` call throw `TypeError`. Because `withTime` is now called during render for the preview text (whenever `picker.customDate` is set), simply clearing the time input after choosing a custom date will crash the component. Add a guard for empty or malformed time strings before calling `withTime`, or make the helper return `null` / throw a controlled error so the UI can degrade gracefully.</violation>
</file>
<file name="apps/dashboard/app/(main)/links/_components/link-folder-sheet.tsx">
<violation number="1" location="apps/dashboard/app/(main)/links/_components/link-folder-sheet.tsx:25">
P2: Submitting a whitespace-only folder name now silently no-ops without any user feedback. The native `required` attribute treats whitespace as a valid non-empty value, and the button is no longer disabled for empty/whitespace-only input, so users can click "Create Folder" and watch nothing happen with no explanation. Previously `disabled={!trimmedName}` proactively prevented this. Consider keeping the button disabled when the trimmed value is empty, or add a `pattern` attribute (e.g., `pattern=".*\\S.*"`) to the input so the browser rejects whitespace-only values and shows a tooltip.</violation>
</file>
<file name="apps/docs/lib/og.tsx">
<violation number="1" location="apps/docs/lib/og.tsx:24">
P2: The catch handler clears the module-level font promise on **every** error, not just transient ones. If fonts are permanently missing or unreadable, every OG-image request will retry `readOgFonts()` (three `readFile` calls each), amplifying filesystem I/O and log noise. Consider limiting retries or distinguishing permanent errors so the cache stays rejected after a bounded number of attempts.</violation>
</file>
<file name="packages/rpc/src/routers/links.schemas.ts">
<violation number="1" location="packages/rpc/src/routers/links.schemas.ts:94">
P1: The `create` schema inherits `expiresAt` from the generated Drizzle insert schema, which produces a `z.date()` validator by default. The `update` schema, however, explicitly overrides `expiresAt` with `z.string().datetime().nullable().optional()`. This creates an asymmetric API contract: a JSON client can send an ISO datetime string to update a link’s expiration, but the same payload will fail schema validation on create.
Since the create router already runs `new Date(input.expiresAt)` before inserting, the intent is clearly to accept serialized dates. To keep the two routes aligned, override `expiresAt` in `createLinkSchema` the same way it is overridden in `updateLinkSchema`.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| return `Expires ${target.format("MMM D, YYYY")}`; | ||
| } | ||
|
|
||
| function withTime(date: Date, time: string) { |
There was a problem hiding this comment.
P1: The withTime helper crashes at runtime when the type="time" input value is an empty string. time.split(":").map(Number) on "" yields [NaN], so minutes is undefined; Day.js then interprets .minute(undefined) as a getter and returns a number, making the next .second(0) call throw TypeError. Because withTime is now called during render for the preview text (whenever picker.customDate is set), simply clearing the time input after choosing a custom date will crash the component. Add a guard for empty or malformed time strings before calling withTime, or make the helper return null / throw a controlled error so the UI can degrade gracefully.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/dashboard/app/(main)/links/_components/expiration-picker.tsx, line 95:
<comment>The `withTime` helper crashes at runtime when the `type="time"` input value is an empty string. `time.split(":").map(Number)` on `""` yields `[NaN]`, so `minutes` is `undefined`; Day.js then interprets `.minute(undefined)` as a getter and returns a number, making the next `.second(0)` call throw `TypeError`. Because `withTime` is now called during render for the preview text (whenever `picker.customDate` is set), simply clearing the time input after choosing a custom date will crash the component. Add a guard for empty or malformed time strings before calling `withTime`, or make the helper return `null` / throw a controlled error so the UI can degrade gracefully.</comment>
<file context>
@@ -71,6 +67,36 @@ function formatPresetPreview(preset: ExpirationPreset): string {
+ return `Expires ${target.format("MMM D, YYYY")}`;
+}
+
+function withTime(date: Date, time: string) {
+ const [hours, minutes] = time.split(":").map(Number);
+ return dayjs(date).hour(hours).minute(minutes).second(0);
</file context>
| organizationId: z.string().optional(), | ||
| name: z.string().min(1).max(255), | ||
| targetUrl: z.url(), | ||
| slug: slugSchema.optional(), |
There was a problem hiding this comment.
P1: The create schema inherits expiresAt from the generated Drizzle insert schema, which produces a z.date() validator by default. The update schema, however, explicitly overrides expiresAt with z.string().datetime().nullable().optional(). This creates an asymmetric API contract: a JSON client can send an ISO datetime string to update a link’s expiration, but the same payload will fail schema validation on create.
Since the create router already runs new Date(input.expiresAt) before inserting, the intent is clearly to accept serialized dates. To keep the two routes aligned, override expiresAt in createLinkSchema the same way it is overridden in updateLinkSchema.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/rpc/src/routers/links.schemas.ts, line 94:
<comment>The `create` schema inherits `expiresAt` from the generated Drizzle insert schema, which produces a `z.date()` validator by default. The `update` schema, however, explicitly overrides `expiresAt` with `z.string().datetime().nullable().optional()`. This creates an asymmetric API contract: a JSON client can send an ISO datetime string to update a link’s expiration, but the same payload will fail schema validation on create.
Since the create router already runs `new Date(input.expiresAt)` before inserting, the intent is clearly to accept serialized dates. To keep the two routes aligned, override `expiresAt` in `createLinkSchema` the same way it is overridden in `updateLinkSchema`.</comment>
<file context>
@@ -0,0 +1,229 @@
+ organizationId: z.string().optional(),
+ name: z.string().min(1).max(255),
+ targetUrl: z.url(),
+ slug: slugSchema.optional(),
+ expiredRedirectUrl: z.url().nullable().optional(),
+ ogTitle: z.string().max(200).nullable().optional(),
</file context>
| updateLinkFolderSchema, | ||
| } from "./links.schemas"; | ||
|
|
||
| type LinkPermission = "read" | "create" | "update" | "delete"; |
There was a problem hiding this comment.
P2: The requireOrganizationId and requireLinkAccess helpers (along with the LinkPermission type) are duplicated identically in both link-folders.ts and links.ts. Since this PR's stated goal is to centralize access helpers, these should live in a shared module so that authorization changes only need to be made in one place and stay consistent across both routers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/rpc/src/routers/link-folders.ts, line 25:
<comment>The `requireOrganizationId` and `requireLinkAccess` helpers (along with the `LinkPermission` type) are duplicated identically in both `link-folders.ts` and `links.ts`. Since this PR's stated goal is to centralize access helpers, these should live in a shared module so that authorization changes only need to be made in one place and stay consistent across both routers.</comment>
<file context>
@@ -13,78 +13,60 @@ import { z } from "zod";
+ updateLinkFolderSchema,
+} from "./links.schemas";
+
+type LinkPermission = "read" | "create" | "update" | "delete";
+type LinkFolderRow = typeof linkFolders.$inferSelect;
</file context>
|
|
||
| export function loadOgFonts() { | ||
| ogFontsPromise ??= readOgFonts(); | ||
| ogFontsPromise ??= readOgFonts().catch((error) => { |
There was a problem hiding this comment.
P2: The OG font-loading cache and its new rejection-reset behavior are maintained in two exact copies: this dashboard file and apps/docs/lib/og.tsx (the files are byte-for-byte identical). Because the fix had to be applied in both places, future changes to error handling, font paths, or returned font metadata can easily drift if one copy is updated without the other. Consider either extracting this into a shared package utility or adding cross-reference comments at the top of each file so future edits stay aligned.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/dashboard/app/(dby)/dby/og/brand.tsx, line 24:
<comment>The OG font-loading cache and its new rejection-reset behavior are maintained in two exact copies: this dashboard file and `apps/docs/lib/og.tsx` (the files are byte-for-byte identical). Because the fix had to be applied in both places, future changes to error handling, font paths, or returned font metadata can easily drift if one copy is updated without the other. Consider either extracting this into a shared package utility or adding cross-reference comments at the top of each file so future edits stay aligned.</comment>
<file context>
@@ -21,7 +21,10 @@ const FONT_DIR = path.join(process.cwd(), "fonts", "lt-superior");
export function loadOgFonts() {
- ogFontsPromise ??= readOgFonts();
+ ogFontsPromise ??= readOgFonts().catch((error) => {
+ ogFontsPromise = undefined;
+ throw error;
</file context>
| const form = event.currentTarget; | ||
| const name = new FormData(form).get("name")?.toString().trim() ?? ""; | ||
|
|
||
| if (!name) { |
There was a problem hiding this comment.
P2: Submitting a whitespace-only folder name now silently no-ops without any user feedback. The native required attribute treats whitespace as a valid non-empty value, and the button is no longer disabled for empty/whitespace-only input, so users can click "Create Folder" and watch nothing happen with no explanation. Previously disabled={!trimmedName} proactively prevented this. Consider keeping the button disabled when the trimmed value is empty, or add a pattern attribute (e.g., pattern=".*\\S.*") to the input so the browser rejects whitespace-only values and shows a tooltip.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/dashboard/app/(main)/links/_components/link-folder-sheet.tsx, line 25:
<comment>Submitting a whitespace-only folder name now silently no-ops without any user feedback. The native `required` attribute treats whitespace as a valid non-empty value, and the button is no longer disabled for empty/whitespace-only input, so users can click "Create Folder" and watch nothing happen with no explanation. Previously `disabled={!trimmedName}` proactively prevented this. Consider keeping the button disabled when the trimmed value is empty, or add a `pattern` attribute (e.g., `pattern=".*\\S.*"`) to the input so the browser rejects whitespace-only values and shows a tooltip.</comment>
<file context>
@@ -17,21 +17,16 @@ export function LinkFolderSheet({
+ const form = event.currentTarget;
+ const name = new FormData(form).get("name")?.toString().trim() ?? "";
+
+ if (!name) {
return;
}
</file context>
|
|
||
| export function loadOgFonts() { | ||
| ogFontsPromise ??= readOgFonts(); | ||
| ogFontsPromise ??= readOgFonts().catch((error) => { |
There was a problem hiding this comment.
P2: The catch handler clears the module-level font promise on every error, not just transient ones. If fonts are permanently missing or unreadable, every OG-image request will retry readOgFonts() (three readFile calls each), amplifying filesystem I/O and log noise. Consider limiting retries or distinguishing permanent errors so the cache stays rejected after a bounded number of attempts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/docs/lib/og.tsx, line 24:
<comment>The catch handler clears the module-level font promise on **every** error, not just transient ones. If fonts are permanently missing or unreadable, every OG-image request will retry `readOgFonts()` (three `readFile` calls each), amplifying filesystem I/O and log noise. Consider limiting retries or distinguishing permanent errors so the cache stays rejected after a bounded number of attempts.</comment>
<file context>
@@ -21,7 +21,10 @@ const FONT_DIR = path.join(process.cwd(), "fonts", "lt-superior");
export function loadOgFonts() {
- ogFontsPromise ??= readOgFonts();
+ ogFontsPromise ??= readOgFonts().catch((error) => {
+ ogFontsPromise = undefined;
+ throw error;
</file context>
1 participant
Commits the outstanding working-tree WIP in coherent slices, rebased onto the current staging (which now carries the drizzle-orm dep the schema extraction needs).
Slices
links.schemas.tsgenerated from the Drizzle tables viadrizzle-orm/zod; centralize access behindrequireOrganizationId/requireLinkAccess/getLinkOrThrow; collapse duplicated router test setup (links.test.ts drops ~800 lines).use-linkshook against the tightened schemas./so a fully-masked path never serializes to empty.dump.rdb(a committed Redis snapshot; already covered by the*.rdbgitignore rule).Verification
turbo check-typespasses for rpc + dashboard with the WIP; pre-commit hooks (format + types) green on every slice. Not exhaustively run beyond type-check since this is in-progress work being committed for review.Summary by cubic
Refactored Links RPC to use
drizzle-orm/zod-generated schemas with stricter access checks, and streamlined the Links dashboard UI and hooks for simpler state and fewer edge cases. Also fixed OG font-loading retries, made pathname masking safer, and ensured soft-deleted links can’t be read or mutated.Refactors
links.schemas.ts; addedrequireOrganizationId/requireLinkAccess/getLinkOrThrow; simplified routers and tests.use-linkscache updates.Bug Fixes
getLinkOrThrownow ignores soft-deleted links (returns 404), preventing reads/updates on deleted records.mask-pathnameignores trailing empty segments and falls back to/when fully masked.Written for commit 5a76ec8. Summary will update on new commits.