Delete Image ownership check - #1450
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughImage uploads now include owner metadata. The deletion route validates source folders and ownership for profile, sponsor, grant, and configured image sources. Cloudinary checks both image and raw resources during ownership lookup and deletion. ChangesImage ownership authorization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SignRoute
participant generateSignedUploadParams
participant UploadHook
participant TiptapUpload
participant Cloudinary
SignRoute->>generateSignedUploadParams: Pass source and authenticated userId
generateSignedUploadParams-->>SignRoute: Return signed parameters with owner_id context
SignRoute-->>UploadHook: Return upload parameters
UploadHook->>Cloudinary: Upload with context
TiptapUpload->>Cloudinary: Upload with context
sequenceDiagram
participant Client
participant DeleteRoute
participant Cloudinary
participant GrantTranches
Client->>DeleteRoute: Request image deletion
DeleteRoute->>DeleteRoute: Validate source folder and authorization
DeleteRoute->>Cloudinary: Read owner metadata
Cloudinary-->>DeleteRoute: Return owner_id or not found
DeleteRoute->>GrantTranches: Check legacy grant ownership when needed
GrantTranches-->>DeleteRoute: Return ownership result
DeleteRoute->>Cloudinary: Delete image and raw resources when authorized
Cloudinary-->>DeleteRoute: Return deletion result
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 2
🧹 Nitpick comments (3)
src/lib/image-upload/types.ts (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse explicit
| undefinedandreadonlyfor the new field.The new
context?: string;field uses the optional-property shorthand. Usecontext: string | undefined;instead, so object literals must explicitly assign the field. Also mark itreadonly, sinceSignedUploadParamsvalues are not mutated after construction.As per coding guidelines, "Use
property: Type | undefinedinstead ofproperty?: Typefor TypeScript type definitions to force explicit property passing and prevent bugs from accidentally omitting required properties" and "Usereadonlyproperties for object types by default in TypeScript to prevent accidental mutation at runtime."♻️ Proposed fix
export interface SignedUploadParams { - signature: string; - timestamp: number; - cloudName: string; - apiKey: string; - folder: string; - publicId?: string; - context?: string; - eager?: string; + readonly signature: string; + readonly timestamp: number; + readonly cloudName: string; + readonly apiKey: string; + readonly folder: string; + readonly publicId: string | undefined; + readonly context: string | undefined; + readonly eager: string | undefined; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/image-upload/types.ts` at line 26, Update the new context field in SignedUploadParams to be readonly and explicitly typed as string | undefined instead of using optional-property shorthand, ensuring object literals must provide it while preserving its string-or-undefined value.Source: Coding guidelines
src/lib/image-upload/cloudinary-client.ts (1)
57-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the caught error as
unknown, notany.
catch (error: any)disables type checking forerror. The rest of the codebase (for examplesign/route.tsanddelete/route.ts) types caught errors asunknown. Useunknownhere too, and narrow before readinghttp_code.As per coding guidelines, "Outside of generic functions, use
anytype extremely sparingly."♻️ Proposed fix
- } catch (error: any) { - if (error?.http_code !== 404 && error?.error?.http_code !== 404) { - throw error; - } - } + } catch (error: unknown) { + const httpCode = + (error as { http_code?: number })?.http_code ?? + (error as { error?: { http_code?: number } })?.error?.http_code; + if (httpCode !== 404) { + throw error; + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/image-upload/cloudinary-client.ts` around lines 57 - 75, Update the catch clause in getImageOwnerId to type error as unknown, then narrow it before accessing http_code or error.http_code while preserving the existing 404 handling and rethrow behavior for other errors.Source: Coding guidelines
src/app/api/image/delete/route.ts (1)
47-75: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider filtering grant tranches at the database level.
isLegacyGrantImageOwnedByUserloads everygrantTrancherow for the user, then scans each JSON array in application code to find the matching public ID. For users with many tranches or largeeventPictures/eventReceipts/aiReceiptsarrays, this pulls more data into memory than necessary. A database-level JSON containment filter would scale better, though this is not urgent given the current expected data volumes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/image/delete/route.ts` around lines 47 - 75, Update isLegacyGrantImageOwnedByUser to filter grantTranche records in the Prisma query using the selected source field and publicId JSON containment, returning whether any matching row exists via the database instead of loading all arrays and scanning them with jsonContainsPublicId.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/api/image/delete/route.ts`:
- Around line 33-36: Extract the grant image source list into a shared exported
as-const array and derive GrantImageSource from it. In
src/app/api/image/delete/route.ts lines 33-36, 262-266, and 65-69, import and
use the shared symbols for type narrowing, source checks, and fieldBySource
keys; in src/app/api/image/sign/route.ts lines 103-112, replace the local
grantImageSources Set with the shared list. Ensure all four sites use one source
of truth.
In `@src/lib/image-upload/cloudinary-client.ts`:
- Around line 77-87: Update deleteImage so its catch block does not silently
discard failures from cloudinary.uploader.destroy: distinguish expected
not-found responses from other errors using the existing getImageOwnerId
pattern, or at minimum log caught errors with relevant context before
continuing. Preserve the retry across both resource types and the existing
boolean result behavior.
---
Nitpick comments:
In `@src/app/api/image/delete/route.ts`:
- Around line 47-75: Update isLegacyGrantImageOwnedByUser to filter grantTranche
records in the Prisma query using the selected source field and publicId JSON
containment, returning whether any matching row exists via the database instead
of loading all arrays and scanning them with jsonContainsPublicId.
In `@src/lib/image-upload/cloudinary-client.ts`:
- Around line 57-75: Update the catch clause in getImageOwnerId to type error as
unknown, then narrow it before accessing http_code or error.http_code while
preserving the existing 404 handling and rethrow behavior for other errors.
In `@src/lib/image-upload/types.ts`:
- Line 26: Update the new context field in SignedUploadParams to be readonly and
explicitly typed as string | undefined instead of using optional-property
shorthand, ensuring object literals must provide it while preserving its
string-or-undefined value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ab15bbd5-1f22-4e33-b120-84e104441d95
📒 Files selected for processing (6)
src/app/api/image/delete/route.tssrc/app/api/image/sign/route.tssrc/hooks/use-image-upload.tssrc/lib/image-upload/cloudinary-client.tssrc/lib/image-upload/index.tssrc/lib/image-upload/types.ts
| type GrantImageSource = | ||
| | 'grant-event-pictures' | ||
| | 'grant-event-receipts' | ||
| | 'grant-agentic-receipts'; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Extract a single shared list of grant image sources.
The set of "grant image" sources (grant-event-pictures, grant-event-receipts, grant-agentic-receipts) is defined independently in four places across two files. This is one root cause: no shared source of truth. A future addition or rename of a grant source requires updating all four sites; missing one silently breaks either context attachment on upload or ownership verification on delete.
src/app/api/image/delete/route.ts#L33-L36: deriveGrantImageSourcefrom a single exportedas constarray (e.g. inconfig.ts), instead of an independent literal union.src/app/api/image/delete/route.ts#L262-L266: replace theelse ifliteral comparisons with a check against the shared array/set (e.g.GRANT_IMAGE_SOURCES.includes(source)), narrowingsourcetoGrantImageSource.src/app/api/image/delete/route.ts#L65-L69: keepfieldBySourcekeyed by the sharedGrantImageSourcetype so TypeScript flags a missing entry when a new grant source is added.src/app/api/image/sign/route.ts#L103-L112: replace the inlinegrantImageSourcesSet with the same shared array/set, instead of a locally re-declared list.
♻️ Proposed shared constant
// src/lib/image-upload/config.ts
export const GRANT_IMAGE_SOURCES = [
'grant-event-pictures',
'grant-event-receipts',
'grant-agentic-receipts',
] as const;
export type GrantImageSource = (typeof GRANT_IMAGE_SOURCES)[number];// src/app/api/image/sign/route.ts
- const grantImageSources = new Set([
- 'grant-event-pictures',
- 'grant-event-receipts',
- 'grant-agentic-receipts',
- ]);
+ const grantImageSources = new Set(GRANT_IMAGE_SOURCES);// src/app/api/image/delete/route.ts
-type GrantImageSource =
- | 'grant-event-pictures'
- | 'grant-event-receipts'
- | 'grant-agentic-receipts';
+import type { GrantImageSource } from '`@/lib/image-upload/config`';
+import { GRANT_IMAGE_SOURCES } from '`@/lib/image-upload/config`';
...
- } else if (
- source === 'grant-event-pictures' ||
- source === 'grant-event-receipts' ||
- source === 'grant-agentic-receipts'
- ) {
+ } else if (
+ (GRANT_IMAGE_SOURCES as readonly string[]).includes(source)
+ ) {📍 Affects 2 files
src/app/api/image/delete/route.ts#L33-L36(this comment)src/app/api/image/delete/route.ts#L262-L266src/app/api/image/delete/route.ts#L65-L69src/app/api/image/sign/route.ts#L103-L112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/image/delete/route.ts` around lines 33 - 36, Extract the grant
image source list into a shared exported as-const array and derive
GrantImageSource from it. In src/app/api/image/delete/route.ts lines 33-36,
262-266, and 65-69, import and use the shared symbols for type narrowing, source
checks, and fieldBySource keys; in src/app/api/image/sign/route.ts lines
103-112, replace the local grantImageSources Set with the shared list. Ensure
all four sites use one source of truth.
1 participant
What does this PR do?
Where should the reviewer start?
How should this be manually tested?
Any background context you want to provide?
What are the relevant issues?
Screenshots (if appropriate)
Summary by CodeRabbit
Bug Fixes
Enhancements