Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughUpdates Supabase config Deno edge_runtime version (2 → 1) and changes TUS upload endpoint OPTIONS handling to return static, no-auth TUS CORS/capability responses; POST/HEAD/PATCH proxy behavior is unchanged. Adds an import-map entry for Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes the TUS OPTIONS request handling for build uploads by returning responses locally instead of proxying them to the builder service. This optimization aligns the implementation with the existing TUS file upload patterns and uses shared constants from files/util.ts.
Changes:
- Added local TUS OPTIONS handlers for
/build/upload/:jobIdand/build/upload/:jobId/*endpoints - Imported TUS-related constants from
files/util.tsfor consistency - Removed the previous proxy-based OPTIONS implementation that forwarded requests to the builder
| }) | ||
|
|
||
| // TUS proxy endpoints - ALL methods proxied to builder with API key injection | ||
| const TUS_EXTENSIONS = 'creation,creation-defer-length,creation-with-upload,expiration' |
There was a problem hiding this comment.
The TUS_EXTENSIONS constant is defined locally here but is identical to the one in files/files.ts:219. Consider extracting this to files/util.ts as a shared constant (e.g., export const TUS_EXTENSIONS = 'creation,creation-defer-length,creation-with-upload,expiration') to avoid duplication and ensure consistency across TUS implementations.
| // OPTIONS /build/upload/:jobId - TUS capabilities (no auth needed) | ||
| app.options('/upload/:jobId', (c) => { | ||
| return c.json(BRES, 200, tusOptionsResponse()) | ||
| }) | ||
|
|
||
| // OPTIONS /build/upload/:jobId/* - TUS capabilities (no auth needed) | ||
| app.options('/upload/:jobId/*', (c) => { | ||
| return c.json(BRES, 200, tusOptionsResponse()) | ||
| }) |
There was a problem hiding this comment.
Consider adding test coverage for the TUS OPTIONS endpoints. While the existing test file tests/tus-upload.test.ts covers TUS OPTIONS for the files module (line 123-132), there are no tests verifying that the build endpoints correctly return TUS OPTIONS responses. This is particularly important since the implementation differs from the files module (returns headers locally instead of proxying).
Suggested test case:
- Verify OPTIONS /build/upload/:jobId returns correct TUS headers
- Verify OPTIONS /build/upload/:jobId/* returns correct TUS headers
- Verify 204 status code (after fixing the bug)
- Verify no authentication is required for OPTIONS requests
| return c.json(BRES, 200, tusOptionsResponse()) | ||
| }) | ||
|
|
||
| // OPTIONS /build/upload/:jobId/* - TUS capabilities (no auth needed) | ||
| app.options('/upload/:jobId/*', (c) => { | ||
| return c.json(BRES, 200, tusOptionsResponse()) |
There was a problem hiding this comment.
The usage of c.json() is incorrect here. Hono's c.json() method accepts two parameters: (object, statusCode), not three. For TUS OPTIONS responses, you should use c.newResponse() to set headers without a JSON body.
Looking at the existing TUS implementation in files/files.ts:211-220, the correct pattern is:
return c.newResponse(null, 204, {
'Access-Control-Allow-Origin': '*',
'Access-Control-Allow-Methods': ALLOWED_METHODS,
...
})
Additionally, TUS OPTIONS responses should return HTTP 204 (No Content), not 200 with a JSON body. This aligns with the TUS protocol specification where OPTIONS is a preflight request that should return headers without content.
| return c.json(BRES, 200, tusOptionsResponse()) | |
| }) | |
| // OPTIONS /build/upload/:jobId/* - TUS capabilities (no auth needed) | |
| app.options('/upload/:jobId/*', (c) => { | |
| return c.json(BRES, 200, tusOptionsResponse()) | |
| return c.newResponse(null, 204, tusOptionsResponse()) | |
| }) | |
| // OPTIONS /build/upload/:jobId/* - TUS capabilities (no auth needed) | |
| app.options('/upload/:jobId/*', (c) => { | |
| return c.newResponse(null, 204, tusOptionsResponse()) |
| return c.json(BRES, 200, tusOptionsResponse()) | ||
| }) | ||
|
|
||
| // OPTIONS /build/upload/:jobId/* - TUS capabilities (no auth needed) | ||
| app.options('/upload/:jobId/*', (c) => { | ||
| return c.json(BRES, 200, tusOptionsResponse()) |
There was a problem hiding this comment.
Same issue here - c.json(BRES, 200, tusOptionsResponse()) is incorrect. Should use c.newResponse(null, 204, tusOptionsResponse()) instead to properly return TUS OPTIONS headers with a 204 No Content response.
| return c.json(BRES, 200, tusOptionsResponse()) | |
| }) | |
| // OPTIONS /build/upload/:jobId/* - TUS capabilities (no auth needed) | |
| app.options('/upload/:jobId/*', (c) => { | |
| return c.json(BRES, 200, tusOptionsResponse()) | |
| return c.newResponse(null, 204, tusOptionsResponse()) | |
| }) | |
| // OPTIONS /build/upload/:jobId/* - TUS capabilities (no auth needed) | |
| app.options('/upload/:jobId/*', (c) => { | |
| return c.newResponse(null, 204, tusOptionsResponse()) |
|
/tip @Judel777 $30 thanks for this |
|
🎉🎈 @Judel777 has been awarded $30 by Capgo! 🎈🎊 |
9084a74 to
6e06129
Compare
|
Summary (AI generated)
Test plan (AI generated)
bun run lint:backendScreenshots (AI generated)
Checklist (AI generated)
bun run lint:backend && bun run lint.accordingly.
my tests
Summary by CodeRabbit
Chores
New Features