Enhance site content management and support for Supabase client#70
Merged
Conversation
- add SiteAction and SitePageContent - Support dynamic callofCode content
Contributor
|
Warning Review limit reached
More reviews will be available in 20 minutes and 9 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds two new Prisma models ( ChangesDynamic Site Content Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant SiteContentRouter
participant SiteContentController
participant SiteContentService
participant Supabase
participant Prisma
rect rgba(100, 149, 237, 0.5)
Note over Client,Prisma: Gallery Photo Add Flow
Client->>SiteContentRouter: POST /site-content/gallery (multipart)
SiteContentRouter->>SiteContentRouter: parsePhotoData middleware
SiteContentRouter->>SiteContentController: addGalleryPhoto(req, res)
SiteContentController->>Supabase: uploadImage(file)
Supabase-->>SiteContentController: imageUrl
SiteContentController->>SiteContentService: addGalleryPhoto(adminId, {imageUrl, caption, ...})
SiteContentService->>SiteContentService: assertManager(adminId)
SiteContentService->>Prisma: findFirst SitePageContent
Prisma-->>SiteContentService: current galleryPhotos JSON
SiteContentService->>SiteContentService: generate UUID, compute sortOrder
SiteContentService->>Prisma: update SitePageContent galleryPhotos
SiteContentService-->>SiteContentController: SiteContentResponse
SiteContentController-->>Client: 201 { success, data }
end
rect rgba(144, 238, 144, 0.5)
Note over Client,Prisma: Gallery Photo Delete Flow
Client->>SiteContentRouter: DELETE /site-content/gallery/:photoId
SiteContentRouter->>SiteContentController: deleteGalleryPhoto(req, res)
SiteContentController->>SiteContentService: deleteGalleryPhoto(adminId, photoId)
SiteContentService->>Prisma: findFirst and update SitePageContent
SiteContentService-->>SiteContentController: deleted imageUrl
SiteContentController->>Supabase: deleteImage(imageUrl)
SiteContentController-->>Client: 200 { success, message }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
tests/SiteContent.test.ts (1)
190-258: ⚡ Quick winAdd coverage for
updateGalleryPhotoimage replacement flow.This suite currently exercises add/delete paths but does not assert the update path behavior (new upload + old image cleanup + service payload). Adding this test would guard the most failure-prone media mutation path.
🤖 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 `@tests/SiteContent.test.ts` around lines 190 - 258, Add a test case for the updateGalleryPhoto function to complete the gallery photo mutation test coverage. Create a new describe block following the deleteGalleryPhoto tests that exercises the image replacement flow: set up request/response mocks similar to the existing tests, mock mockedUploadImage to resolve with a new image URL and siteContentService.updateGalleryPhoto to resolve with updated content, call the updateGalleryPhoto handler, then assert that mockedDeleteImage is called with the old image URL, mockedUploadImage is called with the new file buffer, siteContentService.updateGalleryPhoto is called with the adminId and updated photo data (including the new imageUrl), and res.status is called with 200.
🤖 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 `@prisma/migrations/20260614203131_add_dynamic_site_feature/migration.sql`:
- Around line 15-28: The SiteAction table migration creates an empty table, but
the updateSiteAction service expects rows to exist and uses findUnique with a
key field that will return 404 if missing. To fix this, add INSERT statements at
the end of the migration to seed all built-in SiteAction keys (such as
'recruitment') with their respective labels and default values, ensuring the
update-only API can function on a fresh database without requiring out-of-band
seeding.
In `@src/app.ts`:
- Line 10: Remove the unused import statement for `supabase` from
"./utils/supabaseClient" at the top of the file. This import is not referenced
anywhere in the code and is triggering the `@typescript-eslint/no-unused-vars`
lint error. Simply delete the entire import line to resolve the lint violation.
In `@src/controllers/site-content.controller.ts`:
- Around line 163-170: The previous image is being deleted twice during gallery
photo replacement: once within the uploadImage function call when passing
existing.imageUrl, and again with a separate deleteImage call. Remove the
redundant deleteImage(previousImageUrl) call that runs after uploadImage in both
locations (around line 163-170 and around line 194-196) since uploadImage
already handles deletion of the old image internally. This prevents a failure
from occurring after the database has been successfully updated.
- Around line 60-64: The controller methods lack authentication protection and
incorrectly accept adminId directly from client-supplied request data instead of
binding it to the authenticated user's identity. Add authentication middleware
to all mutating routes and modify the updateSiteContent, updateSiteAction,
addGalleryPhoto, updateGalleryPhoto, and deleteGalleryPhoto methods to extract
adminId from the authenticated user's identity (e.g., req.user.id from JWT or
session) rather than from the request body or parameters. Verify that the
authenticated user is authorized to act as that adminId before proceeding with
validation and service calls. Apply the same authentication and binding pattern
to similar methods in the topic, question, and member controllers.
In `@src/services/site-content.service.ts`:
- Around line 149-170: The gallery photo mutations in the service have a
read-modify-write race condition where concurrent requests can lose writes
because they all read the same snapshot and overwrite the full galleryPhotos
field. Wrap the read-modify-write operations in a Prisma transaction using
prisma.$transaction to ensure atomicity. This should be applied consistently
across all gallery mutation methods that modify the galleryPhotos field (the
addPhoto method shown, plus the update and delete operations mentioned in the
comment). The transaction should encompass both the getPageContent call and the
subsequent prisma.sitePageContent.update call to prevent concurrent requests
from clobbering each other's changes.
In `@tests/SiteContent.test.ts`:
- Around line 81-82: The test file tests/SiteContent.test.ts violates the
no-explicit-any lint rule by using `any` type annotations for request and
response mock objects at multiple locations. Replace each `any` type annotation
with proper TypeScript types: use `Partial<Request>` for request mock objects
and `Pick<Response, "status" | "json">` (or similar pick syntax listing only the
required properties) for response mock objects, or create dedicated mock
interfaces that define only the necessary properties for each test case. Apply
this fix consistently across all occurrences at lines 81-82 (anchor), 103, 112,
135, 142, 155, 166, 192, 205, 232, and 236 in tests/SiteContent.test.ts to
restore lint compliance and maintain type safety.
---
Nitpick comments:
In `@tests/SiteContent.test.ts`:
- Around line 190-258: Add a test case for the updateGalleryPhoto function to
complete the gallery photo mutation test coverage. Create a new describe block
following the deleteGalleryPhoto tests that exercises the image replacement
flow: set up request/response mocks similar to the existing tests, mock
mockedUploadImage to resolve with a new image URL and
siteContentService.updateGalleryPhoto to resolve with updated content, call the
updateGalleryPhoto handler, then assert that mockedDeleteImage is called with
the old image URL, mockedUploadImage is called with the new file buffer,
siteContentService.updateGalleryPhoto is called with the adminId and updated
photo data (including the new imageUrl), and res.status is called with 200.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 522f2783-b50c-489f-93cb-0ab6728b823a
📒 Files selected for processing (17)
prisma/migrations/20260614203131_add_dynamic_site_feature/migration.sqlprisma/schema.prismasrc/app.tssrc/controllers/achievement.controller.tssrc/controllers/project.controller.tssrc/controllers/site-content.controller.tssrc/routes/achievements.tssrc/routes/index.tssrc/routes/members.tssrc/routes/projects.tssrc/routes/site-content.tssrc/services/site-content.service.tssrc/types/site-content.d.tssrc/utils/supabaseClient.tstests/Achievement.test.tstests/Project.test.tstests/SiteContent.test.ts
Comment on lines
+15
to
+28
| CREATE TABLE "SiteAction" ( | ||
| "id" SERIAL NOT NULL, | ||
| "key" TEXT NOT NULL, | ||
| "label" TEXT, | ||
| "url" TEXT, | ||
| "isVisible" BOOLEAN NOT NULL DEFAULT false, | ||
| "updatedById" TEXT, | ||
| "updatedAt" TIMESTAMP(3) NOT NULL, | ||
|
|
||
| CONSTRAINT "SiteAction_pkey" PRIMARY KEY ("id") | ||
| ); | ||
|
|
||
| -- CreateIndex | ||
| CREATE UNIQUE INDEX "SiteAction_key_key" ON "SiteAction"("key"); |
Contributor
There was a problem hiding this comment.
Seed the built-in SiteAction keys before shipping the update-only API.
The migration creates an empty SiteAction table, but the supplied service path first does findUnique({ where: { key } }) and throws 404 when the row is missing; the stack lists updateSiteAction but no create action route/service. On a fresh database, keys such as recruitment cannot be managed until seeded out-of-band. Seed every supported key here, or change the service to upsert by key.
Example migration direction
CREATE UNIQUE INDEX "SiteAction_key_key" ON "SiteAction"("key");
+
+-- Seed every fixed action key supported by the site-content API.
+-- Keep this list in sync with the API/types.
+INSERT INTO "SiteAction" ("key", "label", "url", "isVisible", "updatedAt")
+VALUES
+ ('recruitment', NULL, NULL, false, CURRENT_TIMESTAMP)
+ON CONFLICT ("key") DO NOTHING;🤖 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 `@prisma/migrations/20260614203131_add_dynamic_site_feature/migration.sql`
around lines 15 - 28, The SiteAction table migration creates an empty table, but
the updateSiteAction service expects rows to exist and uses findUnique with a
key field that will return 404 if missing. To fix this, add INSERT statements at
the end of the migration to seed all built-in SiteAction keys (such as
'recruitment') with their respective labels and default values, ensuring the
update-only API can function on a fresh database without requiring out-of-band
seeding.
callofcode07
approved these changes
Jun 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit