chore: cleanup image actions #1262
Conversation
WalkthroughCentralizes image upload/remove into backend RPCs. Removes local server actions for user and organization images, adds UploadImage/RemoveImage RPCs and helpers, updates domain schemas and UI to call RPCs, and adjusts related UI and utility exports/imports. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC as RPC Runtime
participant Handler as UsersRpcs.Handler
participant S3
participant DB as Database
rect rgb(220,240,220)
Note over Client,DB: UploadImage (new centralized flow)
Client->>RPC: UploadImage(data, contentType, fileName, type, entityId, oldImageKey)
RPC->>Handler: invoke UploadImage
Handler->>S3: (opt) delete old key
S3-->>Handler: deletion result
Handler->>S3: upload new object -> returns key
S3-->>Handler: key
Handler->>DB: update user/org record with key
DB-->>Handler: updated
Handler-->>RPC: { key }
RPC-->>Client: success
end
rect rgb(240,220,220)
Note over Client,DB: RemoveImage (new centralized flow)
Client->>RPC: RemoveImage(imageKey, type, entityId)
RPC->>Handler: invoke RemoveImage
Handler->>S3: delete object
S3-->>Handler: deleted
Handler->>DB: clear image field
DB-->>Handler: updated
Handler-->>RPC: { success: true }
RPC-->>Client: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/web/actions/account/remove-profile-image.ts(0 hunks)apps/web/actions/account/upload-profile-image.ts(0 hunks)apps/web/actions/images/remove-image.ts(1 hunks)apps/web/actions/images/upload-image.ts(1 hunks)apps/web/actions/organization/remove-icon.ts(0 hunks)apps/web/actions/organization/upload-organization-icon.ts(0 hunks)apps/web/app/(org)/dashboard/settings/account/Settings.tsx(3 hunks)apps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsx(3 hunks)packages/web-domain/src/index.ts(1 hunks)
💤 Files with no reviewable changes (4)
- apps/web/actions/account/upload-profile-image.ts
- apps/web/actions/organization/upload-organization-icon.ts
- apps/web/actions/organization/remove-icon.ts
- apps/web/actions/account/remove-profile-image.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/web/actions/images/remove-image.tspackages/web-domain/src/index.tsapps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsxapps/web/actions/images/upload-image.tsapps/web/app/(org)/dashboard/settings/account/Settings.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/actions/images/remove-image.tspackages/web-domain/src/index.tsapps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsxapps/web/actions/images/upload-image.tsapps/web/app/(org)/dashboard/settings/account/Settings.tsx
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/actions/images/remove-image.tsapps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsxapps/web/actions/images/upload-image.tsapps/web/app/(org)/dashboard/settings/account/Settings.tsx
apps/web/actions/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All Groq/OpenAI calls must execute in Next.js Server Actions under apps/web/actions; do not invoke AI providers elsewhere
Files:
apps/web/actions/images/remove-image.tsapps/web/actions/images/upload-image.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/actions/images/remove-image.tsapps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsxapps/web/actions/images/upload-image.tsapps/web/app/(org)/dashboard/settings/account/Settings.tsx
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsxapps/web/app/(org)/dashboard/settings/account/Settings.tsx
🧬 Code graph analysis (4)
apps/web/actions/images/remove-image.ts (3)
apps/web/lib/server.ts (1)
runPromise(123-135)packages/database/index.ts (1)
db(18-25)packages/database/schema.ts (2)
users(58-117)organizations(170-202)
apps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsx (2)
apps/web/actions/images/upload-image.ts (1)
uploadImage(12-83)apps/web/actions/images/remove-image.ts (1)
removeImage(13-70)
apps/web/actions/images/upload-image.ts (3)
apps/web/lib/server.ts (1)
runPromise(123-135)packages/database/index.ts (1)
db(18-25)packages/database/schema.ts (2)
users(58-117)organizations(170-202)
apps/web/app/(org)/dashboard/settings/account/Settings.tsx (2)
apps/web/actions/images/upload-image.ts (1)
uploadImage(12-83)apps/web/actions/images/remove-image.ts (1)
removeImage(13-70)
🪛 GitHub Check: CodeQL
apps/web/actions/images/remove-image.ts
[failure] 67-67: Use of externally-controlled format string
Format string depends on a user-provided value.
apps/web/actions/images/upload-image.ts
[failure] 41-41: Use of externally-controlled format string
Format string depends on a user-provided value.
[failure] 80-80: Use of externally-controlled format string
Format string depends on a user-provided value.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (9)
apps/web/actions/images/remove-image.ts (4)
22-38: Excellent path traversal protection.The URL parsing logic properly decodes, normalizes, and validates the S3 key to prevent path traversal attacks.
40-44: Good prefix validation for defense-in-depth.Validating the S3 key prefix before deletion ensures only the correct entity type's images are removed.
67-67: CodeQL format string warning is a false positive.The static analysis tool flags
typeas externally controlled, but TypeScript constrains it to the literal union"user" | "organization", making this safe.
48-58: Database updates are correct and type-safe.Properly handles both entity types with appropriate field updates and branded type constructors.
packages/web-domain/src/index.ts (1)
17-17: LGTM on the UserId export.Cleanly extends the public API surface to support the centralized image actions.
apps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsx (1)
7-8: Clean integration with centralized image actions.The component correctly migrates to
uploadImageandremoveImage, passing all required parameters including entity type, ID, and existing URL for replacement scenarios.Also applies to: 28-33, 52-56
apps/web/app/(org)/dashboard/settings/account/Settings.tsx (1)
95-98: Good validation of user ID before upload.Properly validates that
user?.idexists before callinguploadImage, preventing invalid mutations.apps/web/actions/images/upload-image.ts (2)
41-41: CodeQL format string warnings are false positives.Both instances where
typeis used in format strings are safe because TypeScript constrainstypeto the literal union"user" | "organization".Also applies to: 80-80
60-71: Database updates are correct and type-safe.Properly handles both entity types with appropriate field updates and branded type constructors.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/web/actions/images/remove-image.ts (1)
67-68: Fix the format string in error logging.JavaScript's
console.errordoesn't support printf-style%sformatting. The type variable won't be interpolated as intended.Apply this diff:
- console.error(`Error removing %s image:`, type, error); + console.error(`Error removing ${type} image:`, error);apps/web/actions/images/upload-image.ts (2)
50-50: Fix the format string in error logging.JavaScript's
console.errordoesn't support printf-style%sformatting.Apply this diff:
- console.error(`Error deleting old %s image from S3:`, type, error); + console.error(`Error deleting old ${type} image from S3:`, error);
89-90: Fix the format string in error logging.JavaScript's
console.errordoesn't support printf-style%sformatting.Apply this diff:
- console.error(`Error uploading %s image:`, type, error); + console.error(`Error uploading ${type} image:`, error);
🧹 Nitpick comments (1)
apps/web/actions/images/remove-image.ts (1)
33-36: Consider refining the path traversal check.The current check
normalized.includes("..")may reject valid filenames containing two consecutive dots (e.g., "my..photo.jpg"). A more precise check would verify the normalized path doesn't escape the intended directory structure.However, the existing
expectedPrefixvalidation at line 42 provides strong protection regardless, making this a defense-in-depth measure.Consider this refinement:
const normalized = path.posix.normalize(decoded); - if (normalized.includes("..")) { + if (normalized.startsWith("../") || normalized.includes("/../")) { throw new Error("Invalid S3 key path"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/actions/images/remove-image.ts(1 hunks)apps/web/actions/images/upload-image.ts(1 hunks)apps/web/app/(org)/dashboard/_components/Navbar/SpaceDialog.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/web/actions/images/upload-image.tsapps/web/actions/images/remove-image.tsapps/web/app/(org)/dashboard/_components/Navbar/SpaceDialog.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/actions/images/upload-image.tsapps/web/actions/images/remove-image.tsapps/web/app/(org)/dashboard/_components/Navbar/SpaceDialog.tsx
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/actions/images/upload-image.tsapps/web/actions/images/remove-image.tsapps/web/app/(org)/dashboard/_components/Navbar/SpaceDialog.tsx
apps/web/actions/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All Groq/OpenAI calls must execute in Next.js Server Actions under apps/web/actions; do not invoke AI providers elsewhere
Files:
apps/web/actions/images/upload-image.tsapps/web/actions/images/remove-image.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/actions/images/upload-image.tsapps/web/actions/images/remove-image.tsapps/web/app/(org)/dashboard/_components/Navbar/SpaceDialog.tsx
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/(org)/dashboard/_components/Navbar/SpaceDialog.tsx
🧬 Code graph analysis (2)
apps/web/actions/images/upload-image.ts (3)
apps/web/lib/server.ts (1)
runPromise(123-135)packages/database/index.ts (1)
db(18-25)packages/database/schema.ts (2)
users(58-117)organizations(170-202)
apps/web/actions/images/remove-image.ts (4)
apps/web/lib/server.ts (1)
runPromise(123-135)packages/database/index.ts (1)
db(18-25)packages/database/schema.ts (2)
users(58-117)organizations(170-202)packages/web-domain/src/index.ts (1)
UserId(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Typecheck
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
apps/web/app/(org)/dashboard/_components/Navbar/SpaceDialog.tsx (1)
21-21: LGTM! Clean removal of unused imports.The removal of
useIdfrom the React import is correct—it's not used anywhere in this component.apps/web/actions/images/remove-image.ts (1)
48-58: Database update logic looks correct.The conditional updates to
users.imageandorganizations.iconUrlare properly structured. TheUserId.make()andOrganisationId.make()calls will throw ifentityIdis malformed, which will be caught by the outer error handler.apps/web/actions/images/upload-image.ts (3)
33-41: Path traversal protection correctly implemented.The URL parsing now includes proper decode, normalize, and path traversal checks, addressing the previous review feedback. The implementation matches the protection in
remove-image.ts.
54-57: S3 key generation is consistent with removal logic.The key format
${type}s/${entityId}/${timestamp}.${fileExtension}properly matches the prefix validation inremove-image.ts, ensuring consistency across upload and removal operations.
70-80: Database update logic is correct.The conditional updates properly set the image fields based on entity type, with appropriate type coercion via
UserId.make()andOrganisationId.make().
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/app/(org)/dashboard/settings/account/Settings.tsx (1)
119-124: Partial fix applied for past review comment.The
user.idvalidation has been correctly added (nice work!), addressing part of the previous review concern. Theuser.image || ""fallback is still present but functions safely—removeImagewon't attempt S3 deletion if the string doesn't match the expected prefix, and the database update is idempotent.For slightly better clarity, you could also validate
user.imageexists before callingremoveImage, or explicitly passnullinstead of an empty string. However, this is a minor point since the current implementation works correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/(org)/dashboard/settings/account/Settings.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/web/app/(org)/dashboard/settings/account/Settings.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/app/(org)/dashboard/settings/account/Settings.tsx
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/app/(org)/dashboard/settings/account/Settings.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/app/(org)/dashboard/settings/account/Settings.tsx
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/(org)/dashboard/settings/account/Settings.tsx
🧬 Code graph analysis (1)
apps/web/app/(org)/dashboard/settings/account/Settings.tsx (2)
apps/web/actions/images/upload-image.ts (1)
uploadImage(13-92)apps/web/actions/images/remove-image.ts (1)
removeImage(13-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
apps/web/app/(org)/dashboard/settings/account/Settings.tsx (2)
17-18: LGTM! Consolidated imports align with centralized image actions.The switch to the new centralized
removeImageanduploadImageactions is consistent with the PR objectives.
95-98: Good validation added for upload mutation.The user ID validation prevents calling
uploadImagewith invalid data, and the parameters correctly match the new action signature.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/web/app/(org)/dashboard/settings/account/Settings.tsx (1)
137-171: Validate user.image before calling RemoveImage RPC.Line 145 still uses the fallback
user.image || "", which passes an empty string ifuser.imageis null/undefined. While the backend RPC will skip deletion if the key doesn't match the expected prefix, it's better to validate upfront to avoid unnecessary RPC calls and improve error clarity.Apply this diff to add validation:
const removeProfileImageMutation = useEffectMutation({ mutationFn: () => { if (!user?.id) { return Effect.fail(new Error("User ID is required")); } + if (!user?.image) { + return Effect.fail(new Error("No profile image to remove")); + } return withRpc((rpc) => rpc.RemoveImage({ - imageKey: user.image || "", + imageKey: user.image, type: "user" as const, entityId: user.id, }),
🧹 Nitpick comments (1)
packages/web-backend/src/Users/UsersRpcs.ts (1)
103-112: Improve file extension extraction robustness.Line 105 extracts the file extension with
split(".").pop()which could return the entire filename if no extension exists, or an empty string if the filename ends with a dot. Consider validating the extension or using a more robust extraction method.Apply this diff:
// Generate new S3 key const timestamp = Date.now(); - const fileExtension = payload.fileName.split(".").pop() || "jpg"; + const parts = payload.fileName.split("."); + const fileExtension = parts.length > 1 && parts[parts.length - 1].length > 0 + ? parts[parts.length - 1] + : "jpg"; const s3Key = `${payload.type}s/${payload.entityId}/${timestamp}.${fileExtension}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/app/(org)/dashboard/settings/account/Settings.tsx(2 hunks)apps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsx(3 hunks)packages/web-backend/src/Users/UsersRpcs.ts(2 hunks)packages/web-domain/src/User.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/web/app/(org)/dashboard/settings/account/Settings.tsxpackages/web-domain/src/User.tspackages/web-backend/src/Users/UsersRpcs.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/app/(org)/dashboard/settings/account/Settings.tsxpackages/web-domain/src/User.tspackages/web-backend/src/Users/UsersRpcs.ts
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/app/(org)/dashboard/settings/account/Settings.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/app/(org)/dashboard/settings/account/Settings.tsx
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/(org)/dashboard/settings/account/Settings.tsx
🧬 Code graph analysis (3)
apps/web/app/(org)/dashboard/settings/account/Settings.tsx (2)
apps/web/lib/EffectRuntime.ts (1)
useEffectMutation(23-23)apps/web/lib/Rpcs.ts (1)
withRpc(16-17)
packages/web-domain/src/User.ts (3)
apps/web/lib/Rpcs.ts (1)
Rpc(11-14)packages/web-domain/src/Errors.ts (1)
InternalError(3-6)packages/web-domain/src/Authentication.ts (1)
RpcAuthMiddleware(34-40)
packages/web-backend/src/Users/UsersRpcs.ts (3)
packages/web-backend/src/Users/UsersOnboarding.ts (1)
UsersOnboarding(10-318)packages/web-domain/src/Errors.ts (1)
InternalError(3-6)packages/web-domain/src/Organisation.ts (1)
Organisation(8-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (8)
packages/web-backend/src/Users/UsersRpcs.ts (4)
1-14: LGTM: Imports and service bindings are correct.The new imports support the RPC implementations, and the Database service binding on line 14 follows Effect patterns correctly.
114-144: LGTM: Database update and error handling are correct.The conditional database updates correctly handle both user and organization image types, and the error handling pipeline properly maps S3Error, DatabaseError, and unknown errors to InternalError with appropriate type tags.
174-204: LGTM: Database nullification and error handling are correct.The database updates correctly set
imageoriconUrlto null based on the entity type, and the error handling pipeline is consistent with the UploadImage RPC.
207-207: LGTM: Layer provision correctly includes Database.Adding
Database.Defaultto the layer dependencies is correct since the new RPCs now bind and use the Database service directly.apps/web/app/(org)/dashboard/settings/account/Settings.tsx (2)
14-14: LGTM: Imports align with coding guidelines.The imports of
Effect,useEffectMutation, andwithRpccorrectly follow the coding guideline to use helpers from@/lib/EffectRuntimeand avoid callingManagedRuntimedirectly in components.As per coding guidelines
Also applies to: 19-20
94-135: LGTM: Upload mutation correctly implements Effect-based flow.The mutation properly validates
user?.idbefore proceeding, constructs the Effect pipeline with file conversion and RPC invocation, and handles success/error cases within the Effect context usingEffect.tapandEffect.catchAll.packages/web-domain/src/User.ts (2)
80-101: LGTM: Schema definitions are well-typed and complete.The payload schemas correctly use
Schema.Uint8Arrayfor binary data,Schema.Literalfor discriminated union types, andSchema.optional(Schema.NullOr(...))for nullable optional fields. The result schemas appropriately return the necessary data (keyfor upload,success: truefor removal).
114-123: LGTM: RPC declarations follow established patterns.Both
UploadImageandRemoveImageRPCs are properly declared with the correct payload and result schemas, useInternalErrorfor error handling, and are protected byRpcAuthMiddlewareto ensure authenticated access.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/components/FileInput.tsx (2)
63-63: Remove unused hook call.The
signedUrlvariable is never used. TheSignedImageUrlcomponent at lines 252-258 handles signed URL fetching internally, making this hook call redundant.Apply this diff to remove the dead code:
- // Get signed URL for S3 keys - const { data: signedUrl } = useSignedImageUrl(previewUrl, type); -
293-308: Remove unreachable loading state code.The loading check and spinner at lines 293-298 will never execute because the top-level
isLoadingcheck at line 232 handles this case first. This branch is only reached whenisLoadingis false.Apply this diff to remove the dead code:
<div onClick={() => !disabled && fileInputRef.current?.click()} onDragEnter={handleDragEnter} onDragOver={handleDragOver} onDragLeave={handleDragLeave} onDrop={handleDrop} className={clsx( "flex gap-3 justify-center items-center px-4 w-full h-full rounded-xl border border-dashed transition-all duration-300 cursor-pointer", isDragging ? "border-blue-500 bg-gray-5" : `border-gray-5 hover:bg-gray-2 ${notDraggingClassName}`, isLoading || disabled ? "pointer-events-none opacity-50" : "", )} > - {isLoading ? ( - <FontAwesomeIcon - className="animate-spin size-4 text-gray-10" - icon={faSpinner} - /> - ) : ( - <FontAwesomeIcon - className="size-4 text-gray-10" - icon={faCloudUpload} - /> - )} + <FontAwesomeIcon + className="size-4 text-gray-10" + icon={faCloudUpload} + /> <p className="truncate text-[13px] text-gray-11"> - {isLoading - ? "Uploading..." - : "Choose a file or drag & drop it here"} + Choose a file or drag & drop it here </p> </div>
🧹 Nitpick comments (2)
packages/web-backend/src/Users/helpers.ts (1)
16-27: Wrap URL parsing in Effect.try for explicit error handling.Lines 17 and 21 can throw synchronous exceptions (
new URL()with invalid URLs,decodeURIComponent()with invalid encoding). While these will be caught by the Effect runtime as defects and handled bycatchAllin the RPCs, it's better to wrap them inEffect.tryfor explicit typed error handling.Apply this diff to improve error handling:
let s3Key = imageKey; if (imageKey.startsWith("http://") || imageKey.startsWith("https://")) { - const url = new URL(imageKey); - const raw = url.pathname.startsWith("/") - ? url.pathname.slice(1) - : url.pathname; - const decoded = decodeURIComponent(raw); - const normalized = path.posix.normalize(decoded); - if (normalized.includes("..")) { - return yield* Effect.fail(new InternalError({ type: "unknown" })); - } - s3Key = normalized; + const urlParseResult = yield* Effect.try({ + try: () => { + const url = new URL(imageKey); + const raw = url.pathname.startsWith("/") + ? url.pathname.slice(1) + : url.pathname; + const decoded = decodeURIComponent(raw); + return path.posix.normalize(decoded); + }, + catch: () => new InternalError({ type: "unknown" }), + }); + if (urlParseResult.includes("..")) { + return yield* Effect.fail(new InternalError({ type: "unknown" })); + } + s3Key = urlParseResult; }packages/web-backend/src/Users/UsersRpcs.ts (1)
76-85: Consider validating file extension and content type.Line 78 extracts the file extension using
split(".").pop()without validating against allowed types (e.g., png, jpg, webp, svg). Additionally, there's no check thatpayload.contentTypematches the extracted extension.Consider adding validation similar to
UsersOnboarding.organizationSetup:// Before line 76, add: const allowedExtensions = new Map([ ["image/png", "png"], ["image/jpeg", "jpg"], ["image/webp", "webp"], ["image/svg+xml", "svg"], ]); const fileExtension = allowedExtensions.get(payload.contentType); if (!fileExtension) { return yield* Effect.fail(new InternalError({ type: "unknown" })); } const s3Key = `${payload.type}s/${payload.entityId}/${Date.now()}.${fileExtension}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/components/FileInput.tsx(4 hunks)packages/ui/src/components/LoadingSpinner.tsx(1 hunks)packages/web-backend/src/Users/UsersRpcs.ts(2 hunks)packages/web-backend/src/Users/helpers.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
packages/ui/src/components/LoadingSpinner.tsxpackages/web-backend/src/Users/UsersRpcs.tspackages/web-backend/src/Users/helpers.tsapps/web/components/FileInput.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
packages/ui/src/components/LoadingSpinner.tsxpackages/web-backend/src/Users/UsersRpcs.tspackages/web-backend/src/Users/helpers.tsapps/web/components/FileInput.tsx
packages/ui/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Component files in
packages/uishould use PascalCase naming if they define React/Solid components.
Files:
packages/ui/src/components/LoadingSpinner.tsx
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/components/FileInput.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/components/FileInput.tsx
🧬 Code graph analysis (3)
packages/web-backend/src/Users/UsersRpcs.ts (4)
packages/web-backend/src/Users/UsersOnboarding.ts (1)
UsersOnboarding(10-318)packages/web-backend/src/Users/helpers.ts (1)
parseImageKey(5-36)packages/web-domain/src/Organisation.ts (1)
Organisation(8-11)packages/web-domain/src/Errors.ts (1)
InternalError(3-6)
packages/web-backend/src/Users/helpers.ts (1)
packages/web-domain/src/Errors.ts (1)
InternalError(3-6)
apps/web/components/FileInput.tsx (2)
packages/ui/src/components/LoadingSpinner.tsx (1)
LoadingSpinner(1-42)apps/web/components/SignedImageUrl.tsx (1)
SignedImageUrl(20-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (10)
packages/ui/src/components/LoadingSpinner.tsx (1)
5-5: LGTM! Clean theme support addition.The
themeColorsprop elegantly allows the spinner to use CSS variable-based theming while preserving backward compatibility. The implementation correctly derives colors conditionally and maintains the original behavior as the default.Also applies to: 13-13, 18-19, 25-26
apps/web/components/FileInput.tsx (3)
3-3: LGTM! Necessary imports for the refactored preview logic.The addition of
LoadingSpinnerandSignedImageUrlsupports the centralized image handling approach.Also applies to: 15-15
232-236: LGTM! Clear upload feedback.The top-level loading state provides excellent user feedback during file uploads, using the themed spinner for consistency.
252-258: LGTM! Centralized preview rendering.Using
SignedImageUrlcomponent cleanly delegates the S3 key detection and signed URL fetching logic, improving maintainability.packages/web-backend/src/Users/helpers.ts (1)
5-8: LGTM! Well-designed return type.The return type
Effect.Effect<Option.Option<string>, InternalError>elegantly handles three states: None (no image), Some(key) (valid image), and InternalError (validation failure). This is a good pattern for optional data with validation.packages/web-backend/src/Users/UsersRpcs.ts (5)
1-14: LGTM! Proper dependencies wired in.The new imports (Db schema, Database service, parseImageKey helper) and the database binding on line 14 correctly support the new UploadImage and RemoveImage RPCs.
65-74: Verify deletion failure behavior aligns with intent.If the old image deletion fails (line 73), the entire upload operation will fail due to the error handling pipeline on lines 110-117. This differs from the previous review feedback, which suggested wrapping deletion in
catchAllto log errors and continue with the upload.Is this behavior intentional? If old image cleanup should not block new uploads, consider wrapping the deletion:
- const oldS3KeyOption = yield* parseImageKey( - payload.oldImageKey, - payload.type, - ); - const [bucket] = yield* s3Buckets.getBucketAccess(Option.none()); - - // Delete old image if it exists and is valid - if (Option.isSome(oldS3KeyOption)) { - yield* bucket.deleteObject(oldS3KeyOption.value); - } + const [bucket] = yield* s3Buckets.getBucketAccess(Option.none()); + + // Delete old image if it exists and is valid (don't fail upload on deletion error) + yield* Effect.gen(function* () { + const oldS3KeyOption = yield* parseImageKey( + payload.oldImageKey, + payload.type, + ); + if (Option.isSome(oldS3KeyOption)) { + yield* bucket.deleteObject(oldS3KeyOption.value); + } + }).pipe( + Effect.catchAll((error) => + Effect.logError(`Failed to delete old ${payload.type} image`, error) + ) + );
127-129: Verify deletion failure behavior for RemoveImage.Similar to UploadImage, if S3 deletion fails on line 128, the entire RemoveImage operation fails. This means the database won't be updated to clear the image reference, potentially leaving a dangling reference to a non-existent S3 object.
Consider whether database cleanup should proceed even if S3 deletion fails:
- // Only delete if we have a valid S3 key - if (Option.isSome(s3KeyOption)) { - yield* bucket.deleteObject(s3KeyOption.value); - } + // Only delete if we have a valid S3 key (continue on failure to update DB) + yield* Effect.gen(function* () { + if (Option.isSome(s3KeyOption)) { + yield* bucket.deleteObject(s3KeyOption.value); + } + }).pipe( + Effect.catchAll((error) => + Effect.logError(`Failed to delete ${payload.type} image from S3`, error) + ) + );
132-151: LGTM! Database cleanup logic is correct.The database update correctly sets
image(for users) oriconUrl(for organizations) to null, using appropriate ID types. The logic properly handles both entity types and will execute even if the S3 key is invalid (None), which is the desired behavior for cleanup.
164-164: LGTM! Database layer correctly provided.Adding
Database.Defaultto the layer provision is necessary for the newdbbinding on line 14 and the database operations in UploadImage and RemoveImage.
1 participant
Summary by CodeRabbit