Refactor routes and improve error handling; added health check endpoint; and removed comments #39
Conversation
…nt and removed unused parameters
WalkthroughAdds /health and /docs endpoints to the app, refactors interviews router to no longer require injected Multer/Supabase, updates its registration, and tweaks JSDoc for the delete interview endpoint. Also includes whitespace and comment cleanups with no functional changes elsewhere. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExpressApp as Express App
participant InterviewsRouter as Interviews Router
participant Controller as interviewCtrl
participant Supabase as Supabase Client
Client->>ExpressApp: DELETE /api/v1/interviews/:id (memberId in body)
ExpressApp->>InterviewsRouter: Route dispatch
InterviewsRouter->>Controller: deleteInterviewById(req, res)
Controller->>Supabase: Delete interview by id (validate memberId)
Supabase-->>Controller: Result / Error
Controller-->>Client: HTTP response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (5)
src/routes/achievements.ts (2)
2-2: Standardize the “Achievements” naming (fix typos)Multiple identifiers are misspelled as “acheivements”. Consider renaming to “achievements” for consistency and maintainability.
-import * as acheivementsCtrl from '../controllers/achievement.controller'; +import * as achievementsCtrl from '../controllers/achievement.controller'; -export default function acheivementsRouter(upload: Multer, supabase: SupabaseClient) { +export default function achievementsRouter(upload: Multer, supabase: SupabaseClient) { - router.get('/', acheivementsCtrl.getAchievements); + router.get('/', achievementsCtrl.getAchievements); - router.get("/:achievementId", acheivementsCtrl.getAchievementById); + router.get("/:achievementId", achievementsCtrl.getAchievementById); - router.post("/", upload.single('image') , parseCreateAchievementData ,acheivementsCtrl.createAchievement); + router.post("/", upload.single('image') , parseCreateAchievementData ,achievementsCtrl.createAchievement); - router.patch("/:achievementId",upload.single('image') , parseCreateAchievementData , acheivementsCtrl.updateAchievementById); + router.patch("/:achievementId",upload.single('image') , parseCreateAchievementData , achievementsCtrl.updateAchievementById); - router.delete("/:achievementId", acheivementsCtrl.deleteAchievementById); + router.delete("/:achievementId", achievementsCtrl.deleteAchievementById); - router.delete("/:achievementId/members/:memberId", acheivementsCtrl.removeMemberFromAchievement); + router.delete("/:achievementId/members/:memberId", achievementsCtrl.removeMemberFromAchievement);Note: Update the import and usage in src/routes/index.ts accordingly.
Also applies to: 21-21, 33-33, 47-47, 67-67, 90-90, 105-105, 119-119
6-6: Nit: spacing after comma in importInsert a space after the comma for readability.
-import { Request, Response,NextFunction } from 'express'; +import { Request, Response, NextFunction } from 'express';src/utils/apiError.ts (1)
29-50: Optional: add structured logging in the error handlerConsider logging operational and programmer errors (without leaking PII), especially outside tests, to aid debugging and incident response.
Example minimal change:
export function errorHandler( err: unknown, req: Request, res: Response, next: NextFunction, ): void { + // TODO: replace console with your logger (e.g., pino/winston) + if (process.env.NODE_ENV !== "test") { + // Safe log: avoid dumping full req/res; include route and method + // Only attach stack for non-production or explicitly operational errors + const safe = { + path: req.path, + method: req.method, + statusCode: err instanceof ApiError ? err.statusCode : 500, + message: err instanceof Error ? err.message : String(err), + }; + // eslint-disable-next-line no-console + console.error("Unhandled error", safe, process.env.NODE_ENV !== "production" && err); + }src/routes/interviews.ts (1)
96-105: DELETE with request body: confirm client compatibilityThe JSDoc now requires memberId in the DELETE body. While Express supports this, some clients/proxies may strip or ignore DELETE bodies. Consider accepting memberId via a header or query param as a fallback, or document the constraint clearly.
Would you like a patch to accept memberId via header (e.g., X-Member-Id) if body is absent?
src/app.ts (1)
38-41: Nit: use app.get for the health checkHealth is typically a GET/HEAD endpoint. Using app.get is clearer and avoids handling unintended methods.
-app.use("/health",(req,res)=>{ +app.get("/health",(req,res)=>{ res.status(200).json({ message: "OK" }); -}) +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app.ts(1 hunks)src/routes/achievements.ts(1 hunks)src/routes/index.ts(1 hunks)src/routes/interviews.ts(2 hunks)src/utils/apiError.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/routes/achievements.ts (3)
src/routes/progress.ts (1)
progressRouter(7-55)src/routes/projects.ts (1)
projectsRouter(28-196)src/routes/topics.ts (1)
topicRouter(13-151)
src/routes/interviews.ts (2)
src/controllers/interview.controller.ts (2)
req(104-133)req(34-59)tests/Interview.test.ts (2)
it(180-308)req(384-405)
src/app.ts (3)
src/routes/index.ts (1)
routes(12-29)src/utils/apiError.ts (1)
errorHandler(29-50)src/routes/progress.ts (1)
progressRouter(7-55)
src/routes/index.ts (1)
src/routes/interviews.ts (1)
interviewRouter(5-112)
🔇 Additional comments (4)
src/routes/achievements.ts (1)
23-23: Whitespace-only change is fineBlank line after router initialization matches the pattern used in other route modules. No functional impact.
src/utils/apiError.ts (1)
19-24: Comment cleanup is fineRemoving the redundant JSDoc above ErrorResponse doesn’t affect behavior.
src/routes/interviews.ts (1)
5-7: Router factory signature simplification LGTMRemoving DI here is consistent with its usage. The route registrations remain intact.
src/routes/index.ts (1)
20-21: Interview router factory usage verified
All references to interviewRouter() have been updated to the new signature, and no residual dependencies remain.• Call site confirmed in src/routes/index.ts (line 20)
• Router factory defined once in src/routes/interviews.ts (line 5)
• No Multer or SupabaseClient imports found in the interviews router
• Only one import from './interviews' (in src/routes/index.ts)
| app.use( | ||
| cors({ | ||
| origin: config.ALLOWED_ORIGINS || "*", // e.g. 'https://club.example.com' | ||
| origin: config.ALLOWED_ORIGINS || "*", | ||
| methods: ["GET", "POST", "PATCH", "DELETE", "OPTIONS"], | ||
| credentials: true, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
CORS misconfiguration: wildcard origin with credentials
Using origin: "" together with credentials: true will fail CORS in browsers (Access-Control-Allow-Origin cannot be "" when credentials are sent).
Use a dynamic origin or whitelist and align credentials accordingly:
-app.use(
- cors({
- origin: config.ALLOWED_ORIGINS || "*",
- methods: ["GET", "POST", "PATCH", "DELETE", "OPTIONS"],
- credentials: true,
- }),
-);
+const allowedOrigins = (config.ALLOWED_ORIGINS || "")
+ .split(",")
+ .map(o => o.trim())
+ .filter(Boolean);
+
+app.use(cors({
+ // If list provided, reflect only allowed origins; otherwise reflect request origin
+ origin: allowedOrigins.length ? allowedOrigins : true,
+ methods: ["GET", "POST", "PATCH", "DELETE", "OPTIONS"],
+ // Credentials only when not using wildcard
+ credentials: allowedOrigins.length ? true : true, // origin:true reflects request origin, safe for credentials
+}));If you prefer a stricter policy, set credentials: false when no whitelist is configured.
🤖 Prompt for AI Agents
In src/app.ts around lines 20 to 26 you are passing origin: "*" together with
credentials: true which breaks browser CORS (Access-Control-Allow-Origin cannot
be "*" when credentials are allowed); update the cors config to use a dynamic
origin checker or whitelist: parse config.ALLOWED_ORIGINS into an array and
provide origin as a function that validates the request origin and calls
callback(null, true) only for allowed origins (and sets
Access-Control-Allow-Credentials true), otherwise call callback(null, false) (or
reject); alternatively, if you do not have a whitelist, set credentials: false
to avoid the conflict; ensure the chosen approach returns the correct boolean to
the cors middleware so the Access-Control-Allow-Origin header is the specific
origin when credentials are enabled.
| // 404 handler | ||
| app.use((req, res) => { | ||
| res.status(404).json({ message: "Not Found" }); | ||
| }); |
There was a problem hiding this comment.
/docs route is unreachable and error handler ordering is incorrect
The catch-all 404 (Lines 45-47) responds before /docs (Lines 54-55), so /docs will 404. Additionally, the global error handler (Line 51) should be the last middleware; placing routes after it bypasses centralized error handling.
Move /docs above the 404 and keep the error handler last:
app.use("/api/v1", routes(upload, supabase));
+// Serve API documentation
+app.use("/docs", express.static(path.join(__dirname, "..", "docs/apidoc")));
+
-// 404 handler
+// 404 handler
app.use((req, res) => {
res.status(404).json({ message: "Not Found" });
});
-// Global error handler
-app.use(errorHandler);
+// Global error handler (must be last)
+app.use(errorHandler);
-// Serve API documentation
-app.use("/docs", express.static(path.join(__dirname, "..", "docs/apidoc")));Also applies to: 53-55, 50-51
🤖 Prompt for AI Agents
In src/app.ts around lines 44-55, the 404 catch-all middleware is registered
before the /docs route and the global error handler is not the last middleware;
move the /docs route (currently at ~54-55) to be registered before the 404
handler (lines 44-47) so /docs is reachable, and ensure the global
error-handling middleware (around lines 50-51) is placed after the 404 handler
so it is the very last middleware in the stack; adjust order accordingly so: all
routes (including /docs) -> 404 catch-all -> global error handler.
Summary by CodeRabbit
New Features
Documentation
Refactor
Style