Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. WalkthroughCentralizes Zod schemas into a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Component (Form)
participant Schema as Schema Module
participant ORPC as ORPC Router
participant DB as Database / Service
UI->>Schema: import <inputSchema>, zodResolver
UI->>Schema: validate form (zodResolver)
UI->>ORPC: call procedure with validated data
ORPC->>Schema: import <input/output schemas> (validate/shape)
ORPC->>DB: perform operation (use typed input)
DB-->>ORPC: result
ORPC->>Schema: format result with output schema
ORPC-->>UI: response (typed via schema)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
orpc/routers/credential.ts (3)
523-566: Critical: missing server-side plan check on container change enables plan bypass.When updating a credential, restricted users (canOnlyAccessDefaultContainers) can connect to a non-default container. Mirror the create flow’s check to enforce plan constraints.
.handler(async ({ input, context }): Promise<CredentialSimpleOutput> => { const { id, ...updateData } = input @@ if (!existingCredential) { throw new ORPCError("NOT_FOUND") } + // Enforce plan: restricted users may only use default containers + if ( + context.permissions?.canOnlyAccessDefaultContainers && + updateData.containerId + ) { + const allowedContainer = await database.container.findFirst({ + where: { + id: updateData.containerId, + userId: context.user.id, + isDefault: true, + }, + }) + if (!allowedContainer) { + throw new ORPCError("FORBIDDEN", { + message: "Your plan only allows using default containers.", + }) + } + }As per orpc/routers guidelines on auth/permissions.
687-777: Apply the same plan enforcement in updateCredentialWithSecuritySettings.Replicate the default-container restriction before updates.
.handler(async ({ input, context }): Promise<CredentialSimpleOutput> => { const { id, passwordProtection, twoFactorAuth, accessLogging, ...updateData } = input @@ if (!existingCredential) { throw new ORPCError("NOT_FOUND") } + // Enforce plan: restricted users may only use default containers + if ( + context.permissions?.canOnlyAccessDefaultContainers && + updateData.containerId + ) { + const allowedContainer = await database.container.findFirst({ + where: { + id: updateData.containerId, + userId: context.user.id, + isDefault: true, + }, + }) + if (!allowedContainer) { + throw new ORPCError("FORBIDDEN", { + message: "Your plan only allows using default containers.", + }) + } + }
1-46: Remove unused imports to comply with coding guidelines.Verification confirms these three imports are unused in
orpc/routers/credential.ts:
credentialFormInputSchema(line 14) — defined in schemas but not referenced in this routercredentialKeyValuePairOutputSchema(line 16) — never used in this filekeyValueWithValueOutputSchema(line 47) — only appears in the import statementRemove them as suggested to satisfy the "Disallow unused imports" guideline.
-import { - createCredentialInputSchema, - credentialFormInputSchema, +import { + createCredentialInputSchema, credentialFormWithIdInputSchema, - credentialKeyValuePairOutputSchema, credentialKeyValuePairsArrayOutputSchema, credentialKeyValuePairsWithValueArrayOutputSchema, credentialKeyValuePairValueOutputSchema, - credentialKeyValuePairWithValueOutputSchema, credentialPasswordOutputSchema, credentialSecuritySettingsOutputSchema, @@ -} from "@/schemas/credential" -import { keyValueWithValueOutputSchema } from "@/schemas/credential/key-value" +} from "@/schemas/credential"
🧹 Nitpick comments (17)
schemas/user/user/input.ts (2)
48-51: Complete list input: add search/filters and integer constraints.Lists should include pagination, search, and filtering with bounds; also mark numbers as integers.
-export const listUsersInputSchema = z.object({ - page: z.number().min(1).default(1), - limit: z.number().min(1).max(100).default(10), -}) +export const listUsersInputSchema = z.object({ + page: z.number().int().min(1).default(1), + limit: z.number().int().min(1).max(100).default(10), + search: z.string().trim().max(100).default(""), + filters: z.record(z.string(), z.any()).default({}).optional(), +})Based on learnings.
9-10: Tighten string bounds and normalize emails.Add sensible max lengths and normalize emails; cap password length to avoid DoS/BCrypt cost spikes.
- name: z.string().min(2, "Name must be at least 2 characters"), - email: z.string().email("Please enter a valid email address"), + name: z.string().min(2, "Name must be at least 2 characters").max(100, "Name must be at most 100 characters"), + email: z.string().email("Please enter a valid email address").max(320).transform(v => v.toLowerCase()),export const loginInputSchema = z.object({ - email: z.string().email("Please enter a valid email address"), - password: z.string().min(8, "Password must be at least 8 characters"), + email: z.string().email("Please enter a valid email address").max(320).transform(v => v.toLowerCase()), + password: z.string().min(8, "Password must be at least 8 characters").max(128, "Password must be at most 128 characters"), rememberMe: z.boolean(), })export const signUpInputSchema = z.object({ - name: z.string().min(2, "Name must be at least 2 characters"), - email: z.string().email("Please enter a valid email address"), - password: z.string().min(8, "Password must be at least 8 characters"), + name: z.string().min(2, "Name must be at least 2 characters").max(100, "Name must be at most 100 characters"), + email: z.string().email("Please enter a valid email address").max(320).transform(v => v.toLowerCase()), + password: z.string().min(8, "Password must be at least 8 characters").max(128, "Password must be at most 128 characters"), image: z.string().url("Please enter a valid image URL").optional(), })Based on learnings.
Also applies to: 59-63, 67-73
components/app/auth-register-form.tsx (2)
36-41: Align schema/UI: image default is unused at submit.You set a default image but override with a computed URL. Either remove image from defaultValues or add a field if you want user-provided images.
- defaultValues: { - email: "", - name: "", - password: "", - image: "https://avatar.vercel.sh/default", - }, + defaultValues: { + email: "", + name: "", + password: "", + },Also applies to: 52-52
145-150: Adopt disabled opacity pattern on buttons.Match button loading/disabled UX convention.
-<Button type="submit" className="w-full" disabled={isLoading}> +<Button type="submit" className="w-full disabled:opacity-50" disabled={isLoading}>Based on learnings.
components/app/marketing-roadmap-subscription.tsx (2)
40-52: Clean up nested timeout to avoid state updates after unmount.The inner setTimeout isn’t cleared; track and clear both timers.
useEffect(() => { if (showSuccess) { - const timer = setTimeout(() => { + const timer = setTimeout(() => { setIsTransitioning(true) - setTimeout(() => { + const fadeTimer = setTimeout(() => { setShowSuccess(false) setIsTransitioning(false) - }, 300) // Wait for fade out animation + }, 300) // Wait for fade out animation }, 3000) // Show success for 3 seconds - return () => clearTimeout(timer) + return () => { + clearTimeout(timer) + // Clear any in-flight fade timer + try { clearTimeout((fadeTimer as unknown as number)) } catch {} + } } }, [showSuccess])
115-124: Adopt disabled opacity pattern on buttons.Enhance disabled-state affordance.
-<Button - type="submit" - className="w-full sm:w-auto" - disabled={subscribeToRoadmapMutation.isPending} -> +<Button + type="submit" + className="w-full sm:w-auto disabled:opacity-50" + disabled={subscribeToRoadmapMutation.isPending} +>Based on learnings.
components/app/auth-login-form.tsx (1)
138-143: Adopt disabled opacity pattern on buttons.Match loading button UX across the app.
-<Button type="submit" className="w-full" disabled={isLoading}> +<Button type="submit" className="w-full disabled:opacity-50" disabled={isLoading}>Based on learnings.
components/app/dashboard-qr-code-dialog.tsx (1)
47-58: Address TODO comments for QR code functionality.There are three TODO items for QR code download, copy, and generation. These represent incomplete core functionality for this dialog.
Would you like me to generate implementations for:
- QR code generation using a library like
qrcodeorqr-code-styling- Download functionality to save QR code as PNG/SVG
- Copy to clipboard functionality using the Clipboard API
Or should I open an issue to track these tasks?
schemas/utils/container/input.ts (1)
5-5: Use specific file import to avoid potential circular dependencies.The import from
"../utils"should reference the specific file to prevent barrel export cycles.Based on coding guidelines for schemas, apply this diff:
-import { EntityTypeSchema } from "../utils" +import { EntityTypeSchema } from "@/schemas/utils/utils"This follows the guideline: "Avoid circular dependencies by importing from specific schema files, not barrel exports."
components/app/dashboard-credential-move-dialog.tsx (2)
193-201: Add explicit type to buttons for accessibility/spec compliance.Set explicit type to avoid default behavior ambiguity and to satisfy the “Always include a type attribute for button elements” rule.
- <Button + <Button + type="button" variant="outline" onClick={() => handleOpenChange(false)} disabled={isMoving} className="order-2 sm:order-1" > Cancel </Button> ... - <Button + <Button + type="button" onClick={form.handleSubmit(handleMove)} disabled={ isMoving || selectedContainerId === credential.containerId || !canMoveCredential } className="order-1 disabled:opacity-50 sm:order-2" >As per coding guidelines.
Also applies to: 201-213
105-108: Avoid console in production paths; use a structured logger.Replace console.error with your logging utility (e.g., server logger) to comply with “Don't use console”.
- console.error("Failed to move credential:", error) + // TODO: route through structured logger + // logger.error({ err: error, credentialId: credential.id }, "Failed to move credential")schemas/credential/input.ts (1)
117-123: Harden validation: trim and reject empty strings.containerId and confirmation text should reject whitespace-only strings.
export const moveCredentialInputSchema = z.object({ - containerId: z.string().optional(), + containerId: z.string().trim().min(1, "Container ID cannot be empty").optional(), }) export type MoveCredentialInput = z.infer<typeof moveCredentialInputSchema> // Delete credential confirmation export const deleteCredentialConfirmationInputSchema = z.object({ - confirmationText: z.string().min(1, "Confirmation text is required"), + confirmationText: z.string().trim().min(1, "Confirmation text is required"), })Based on learnings.
components/app/dashboard-credential-delete-dialog.tsx (1)
194-203: Add explicit type to the destructive button.Set type to comply with button-type rule and avoid accidental submits if DOM structure changes.
- <Button + <Button + type="button" onClick={form.handleSubmit(handleDelete)} disabled={!isConfirmationValid || isDeleting} variant="destructive" className="order-1 disabled:opacity-50 sm:order-2" >schemas/credential/key-value/input.ts (1)
55-60: Tighten key and ID validators (trim/min).Prevent empty/whitespace keys and IDs; maintain optional value for update semantics.
export const credentialKeyValuePairInputSchema = z.object({ id: z.string().optional(), - key: z.string(), + key: z.string().trim().min(1, "Key is required"), value: z.string().optional(), }) ... export const updateCredentialKeyValuePairsInputSchema = z.object({ - credentialId: z.string(), + credentialId: z.string().trim().min(1, "Credential ID is required"), keyValuePairs: z.array(credentialKeyValuePairInputSchema), }) ... export const getCredentialKeyValuePairValueInputSchema = z.object({ - credentialId: z.string(), - keyValuePairId: z.string(), + credentialId: z.string().trim().min(1, "Credential ID is required"), + keyValuePairId: z.string().trim().min(1, "Key-value pair ID is required"), })Based on learnings.
Also applies to: 65-68, 74-77
schemas/credential/key-value/output.ts (1)
109-123: Deduplicate repeated “success: boolean” outputs.You define both updateCredentialPasswordOutputSchema and updateCredentialKeyValuePairsOutputSchema with identical shapes. Consider a shared successOnlyOutputSchema to DRY.
+export const successOnlyOutputSchema = z.object({ success: z.boolean() }) ... -export const updateCredentialPasswordOutputSchema = z.object({ - success: z.boolean(), -}) +export const updateCredentialPasswordOutputSchema = successOnlyOutputSchema ... -export const updateCredentialKeyValuePairsOutputSchema = z.object({ - success: z.boolean(), -}) +export const updateCredentialKeyValuePairsOutputSchema = successOnlyOutputSchemaorpc/routers/credential.ts (2)
125-133: Performance: avoid fetching heavy includes when returning key-only data.Use a narrower select for metadata.keyValuePairs (id,key,createdAt,updatedAt) to reduce payload/CPU.
- const credential = await database.credential.findFirst({ - where: { id: input.id, userId: context.user.id }, - include: CredentialQuery.getInclude(), // full include - }) + const credential = await database.credential.findFirst({ + where: { id: input.id, userId: context.user.id }, + include: { + metadata: { + include: { + keyValuePairs: true, // or select only necessary fields + }, + }, + }, + })Also applies to: 160-170
214-222: Replace console.error with structured logging.Follow the “Don't use console” rule; use your logger and include contextual fields (credential/kvPair ids).
- console.error(`Failed to decrypt key-value pair ${kvPair.id}:`, error) + // logger.error({ err: error, kvPairId: kvPair.id }, "Failed to decrypt key-value pair") ... - console.error("Failed to decrypt password:", error) + // logger.error({ err: error, credentialId: input.id }, "Failed to decrypt password") ... - console.error("Error creating credential with metadata:", error) + // logger.error({ err: error }, "Error creating credential with metadata")Also applies to: 332-337, 1015-1016
| export const userInputSchema = z.object({ | ||
| name: z.string().min(2, "Name must be at least 2 characters"), | ||
| email: z.string().email("Please enter a valid email address"), | ||
| emailVerified: z.boolean().default(false), | ||
| image: z.string().url().optional(), | ||
| }) |
There was a problem hiding this comment.
Block external control of emailVerified in create/update.
Allowing clients to set emailVerified enables bypassing verification. Omit it from public create/update inputs.
Apply this minimal, safe diff:
export const userInputSchema = z.object({
name: z.string().min(2, "Name must be at least 2 characters"),
email: z.string().email("Please enter a valid email address"),
- emailVerified: z.boolean().default(false),
+ emailVerified: z.boolean().default(false), // server-managed only; excluded below
image: z.string().url().optional(),
})
// Create
-export const createUserInputSchema = userInputSchema
+export const createUserInputSchema = userInputSchema.omit({ emailVerified: true })
// Update
-export const updateUserInputSchema = userInputSchema.partial().extend({
- id: z.string().min(1, "User ID is required"),
-})
+export const updateUserInputSchema = userInputSchema
+ .omit({ emailVerified: true })
+ .partial()
+ .extend({
+ id: z.string().min(1, "User ID is required"),
+ })Based on learnings.
Also applies to: 22-25, 34-36
🤖 Prompt for AI Agents
In schemas/user/user/input.ts around lines 8-13 (and similarly at 22-25 and
34-36), the public input schema includes emailVerified which lets clients set
verification status; remove emailVerified from the public create/update schemas
so it cannot be supplied by clients, and ensure server-side code sets or updates
emailVerified internally (e.g., default false or set true only after
verification) rather than accepting it from the request payload.
1 participant
Summary by CodeRabbit
Refactor
New Features
Documentation