Repository navigation
refactor(backend): inline single-use util layers (#1257) - #1343
Conversation
- Inline isDeveloperUser from developerAccess.ts into requireAdmin middleware - Delete unused developerAccess.ts and routeUtils.ts files - Migrate artists routes from wrapHandler to asyncHandler pattern - Migrate auth routes from wrapHandler to asyncHandler pattern - Update middleware integration tests for new error handling pattern - Remove 19 middleware-behavior unit tests from artists.test.ts (now in integration tests) - Update developerAccess unit tests to test requireAdmin directly Changes reduce utility-layer complexity and consolidate error handling in asyncHandler + errorHandler middleware chain.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis PR consolidates single-use backend utility layers by moving ChangesInline single-use backend utilities
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
|
@cubic-dev-ai review |
@LucasSantana-Dev I have started the AI code review. It will take a few minutes to complete. |
|
Failed to generate code suggestions for PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/backend/src/routes/artists.ts`:
- Around line 28-32: The route handler related currently defaults a missing
artistId to '' and calls svc.handleGetRelatedArtists(''), which can cause
unexpected behavior; update the route to validate artistId explicitly by either
adding validateParams middleware with a schema like
artistsSchemas.relatedArtistsParams (z.string().min(1)) at route registration or
by checking inside related (throw new AppError('Missing artistId', 400) if
r.params.artistId is falsy/not a non-empty string) before calling
svc.handleGetRelatedArtists; reference the related handler, AuthenticatedRequest
params, svc.handleGetRelatedArtists, validateParams,
artistsSchemas.relatedArtistsParams, and AppError when making the change.
🪄 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
Run ID: 14a8af5d-dc37-4b23-ba97-bd7733b82fc2
📒 Files selected for processing (10)
packages/backend/src/middleware/asyncHandler.tspackages/backend/src/middleware/requireAdmin.tspackages/backend/src/routes/artists.tspackages/backend/src/routes/auth.tspackages/backend/src/utils/developerAccess.tspackages/backend/src/utils/routeUtils.tspackages/backend/tests/integration/routes/admin.test.tspackages/backend/tests/integration/routes/toggles.test.tspackages/backend/tests/unit/routes/artists.test.tspackages/backend/tests/unit/utils/developerAccess.test.ts
💤 Files with no reviewable changes (2)
- packages/backend/src/utils/developerAccess.ts
- packages/backend/src/utils/routeUtils.ts
📜 Review details
⏰ 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). (6)
- GitHub Check: Checks
- GitHub Check: cubic · AI code reviewer
- GitHub Check: quality / SAST (CodeQL) (javascript-typescript)
- GitHub Check: Build — backend
- GitHub Check: Build — frontend
- GitHub Check: Build — bot
🔇 Additional comments (17)
packages/backend/src/routes/artists.ts (8)
1-10: LGTM!
20-22: LGTM!
24-26: LGTM!
34-41: LGTM!
43-47: LGTM!
49-53: LGTM!
55-62: LGTM!
64-103: LGTM!packages/backend/tests/unit/routes/artists.test.ts (3)
20-20: LGTM!
107-111: LGTM!
147-148: LGTM!Also applies to: 178-179, 250-251, 285-286, 313-314, 343-344, 375-376, 414-415, 461-462, 490-491, 520-521
packages/backend/tests/integration/routes/toggles.test.ts (1)
19-22: Same ineffective mock pattern aspackages/backend/tests/integration/routes/admin.test.tsLines 26-29.packages/backend/src/middleware/asyncHandler.ts (1)
14-14: LGTM!packages/backend/src/middleware/requireAdmin.ts (1)
4-20: LGTM!packages/backend/src/routes/auth.ts (1)
12-12: LGTM!packages/backend/tests/unit/utils/developerAccess.test.ts (1)
2-27: LGTM!packages/backend/tests/integration/routes/admin.test.ts (1)
26-29:isDeveloperUsermock likely doesn’t affect the realrequireAdminpathIn
packages/backend/tests/integration/routes/admin.test.ts(lines 26–29), the mock overrides the exportedisDeveloperUser, but setsrequireAdmintojest.requireActual(...).requireAdmin, which runs the realrequireAdminimplementation frompackages/backend/src/middleware/requireAdmin.ts.If
requireAdminuses a module-localisDeveloperUserbinding (i.e., closes over it), the export override won’t be consulted, so the test may depend on the real predicate (potentiallyprocess.env.DEVELOPER_USER_IDS) instead of the stubbed userId.Adjust the mock to override
requireAdminitself (or refactorrequireAdminto call the exportedisDeveloperUser) so the test actually exercises the intended authorization logic.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 10 files
Auto-approved: Refactors single-use utilities into inline code or middleware. No API behavior changes, all tests pass, and logic is purely structural.
Re-trigger cubic
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Re cubic's artists.ts note: the |
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Removes 19 artists unit tests and restructures error handling across routes. Even though the refactor appears logically safe, deleting tests and changing middleware patterns in a 656-line change carries moderate risk that warrants human review.
Re-trigger cubic
|
This branch was successfully deployed
1 participant
Closes #1257.
artists.tsfromwrapHandlertoasyncHandler+ global error middleware; deleterouteUtils.wrapHandlerisDeveloperUserintorequireAdmin; deleteutils/developerAccess.tsstripUnknownFieldshelper across the three validators inmiddleware/validate.tsasyncHandlernow returns the promise it chains (return result.catch(next)) — no runtime behavior change (express ignores handler return values), makes the wrapper awaitable in testsVerification: backend type:check pass; backend suite 67 suites / 992 tests pass.
Summary by cubic
Refactors backend routing to use asyncHandler with global error handling and inlines developer access logic into requireAdmin. Removes two single-use utils and keeps API behavior unchanged while making handlers easier to test.
Refactors
Tests
Written for commit e67ab1a. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Refactor