refactor: unify binary and html next proxies#410
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant Client
participant ProxyBinary as proxyBinary<br/>(Backend Proxy)
participant Upstream as Upstream API
participant ResponseStream as Response Stream
Client->>ProxyBinary: proxyBinary(req, url, method, options)
ProxyBinary->>Upstream: fetch(url, method)
Upstream-->>ProxyBinary: Response + ArrayBuffer
ProxyBinary->>ProxyBinary: Assemble Headers<br/>(Cache-Control, Content-Type,<br/>Content-Disposition)
ProxyBinary->>ResponseStream: new Response(buffer, {<br/>status, headers<br/>})
ResponseStream-->>Client: Binary Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
📝 Coding Plan
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Next.js API layer by introducing a dedicated Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great refactoring effort to unify the API proxy logic. By introducing a proxyBinary helper and migrating the paper export and newsletter unsubscribe routes to use the shared proxy helpers, the API layer becomes more consistent and maintainable. The changes are well-tested, and the new implementations are much cleaner. I have one minor suggestion regarding HTML escaping in the error handler for the newsletter unsubscribe route to make it more robust.
| const escaped = detail | ||
| .replace(/&/g, "&") | ||
| .replace(/</g, "<") | ||
| .replace(/>/g, ">") | ||
| .replace(/"/g, """) |
There was a problem hiding this comment.
The HTML escaping logic is incomplete as it doesn't handle single quotes ('). While the risk is low in this context, it's a good practice to perform complete HTML entity encoding for any data being embedded in an HTML response to prevent potential cross-site scripting (XSS) vulnerabilities, should the source of the error message change in the future. Please consider adding escaping for single quotes.
const escaped = detail
.replace(/&/g, "&")
.replace(/</g, "<")
.replace(/>/g, ">")
.replace(/"/g, """)
.replace(/'/g, "'")There was a problem hiding this comment.
Pull request overview
Refactors the remaining Next.js API routes that used ad-hoc backend fetch logic (binary export + HTML unsubscribe) onto the shared backend proxy helper, completing the unified proxy contract work started in earlier PRs.
Changes:
- Added
proxyBinary()to the shared backend proxy utilities for authenticated binary downloads withContent-Type/Content-Dispositionpassthrough. - Migrated the papers export route to
proxyBinary()while keeping the explicit 502 fallback payload. - Migrated the newsletter unsubscribe route to
proxyText()with HTML fallback expressed viaonError, and updated/added tests accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/app/api/papers/export/route.ts | Switches export download to shared proxyBinary() with existing 502 fallback behavior. |
| web/src/app/api/papers/export/route.test.ts | Updates route test to validate proxyBinary() wiring and fallback contract. |
| web/src/app/api/newsletter/unsubscribe/[token]/route.ts | Switches unsubscribe HTML proxying to shared proxyText() with onError HTML fallback. |
| web/src/app/api/newsletter/unsubscribe/[token]/route.test.ts | Adds route test validating proxying, encoding, and HTML fallback escaping. |
| web/src/app/api/_utils/backend-proxy.ts | Introduces proxyBinary() helper for binary download proxying. |
| web/src/app/api/_utils/backend-proxy.test.ts | Adds coverage for binary proxy header/auth passthrough. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| `${apiBaseUrl()}/api/newsletter/unsubscribe/${encodeURIComponent(token)}`, | ||
| "GET", | ||
| { | ||
| responseContentType: "text/html", |
| const body = await upstream.arrayBuffer() | ||
| const headers = new Headers(options.responseHeaders) | ||
| headers.set("Cache-Control", "no-cache") | ||
| headers.set( | ||
| "Content-Type", | ||
| upstream.headers.get("content-type") || "application/octet-stream", | ||
| ) | ||
|
|
||
| const contentDisposition = upstream.headers.get("content-disposition") | ||
| if (contentDisposition) { | ||
| headers.set("Content-Disposition", contentDisposition) | ||
| } | ||
|
|
||
| return new Response(body, { |
jerry609
force-pushed
the
refactor/pr15-next-binary-html-proxies
branch
from
95f77da to
4df2547
Compare
jerry609
force-pushed
the
refactor/pr14-next-stream-proxies
branch
from
dcba4e9 to
706ab9e
Compare
jerry609
force-pushed
the
refactor/pr15-next-binary-html-proxies
branch
from
4df2547 to
0da4e6a
Compare
jerry609
force-pushed
the
refactor/pr15-next-binary-html-proxies
branch
from
0da4e6a to
bd81642
Compare
jerry609
force-pushed
the
refactor/pr15-next-binary-html-proxies
branch
from
bd81642 to
2608da1
Compare
Vercel Preview
|
|
There was a problem hiding this comment.
Pull request overview
Refactors the remaining Next.js API routes that used ad-hoc fetch proxying (binary export + HTML unsubscribe) to use the shared backend proxy helper, completing the unification work from prior proxy cleanup PRs.
Changes:
- Added
proxyBinary()to the shared backend proxy helper for streaming binary responses withContent-Type/Content-Dispositionpassthrough. - Migrated
papers/exporttoproxyBinary()while keeping its explicit502 Upstream API unreachableJSON fallback. - Migrated
newsletter/unsubscribetoproxyText()and preserved the escaped-HTML502fallback viaonError, plus added contract tests for the route wiring.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| web/src/app/api/papers/export/route.ts | Switches export download endpoint to shared proxyBinary() with custom 502 fallback. |
| web/src/app/api/papers/export/route.test.ts | Removes the route-specific export proxy test (now covered via helper + contract tests). |
| web/src/app/api/newsletter/unsubscribe/[token]/route.ts | Switches unsubscribe HTML endpoint to shared proxyText() with escaped HTML error fallback. |
| web/src/app/api/_utils/final-proxy-route-contracts.test.ts | Adds contract tests asserting the routes call the shared proxy helpers and preserve fallback behavior. |
| web/src/app/api/_utils/backend-proxy.ts | Introduces proxyBinary() implementation in the shared proxy layer. |
| web/src/app/api/_utils/backend-proxy.test.ts | Adds unit coverage for binary passthrough + auth handling in proxyBinary(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| describe("final proxy route contracts", () => { | ||
| it("proxies protected exports through the shared binary helper", async () => { | ||
| proxyBinaryMock.mockResolvedValueOnce(new Response("bibtex-body")) |
What changed
proxyBinary()to the shared Next backend proxy helper for protected downloads that need content-type/content-disposition passthroughweb/src/app/api/papers/export/route.tsto the shared binary helper while preserving its explicit502 Upstream API unreachablefallback bodyweb/src/app/api/newsletter/unsubscribe/[token]/route.tsto the shared text helper with the existing escaped HTML fallback expressed throughonErrorWhy
web/src/app/apiValidation
npx vitest run src/app/api/_utils/backend-proxy.test.ts src/app/api/_utils/auth-headers.test.ts src/app/api/papers/export/route.test.ts src/app/api/newsletter/unsubscribe/[token]/route.test.ts src/app/api/studio/chat/route.test.ts src/app/api/gen-code/route.test.ts src/app/api/research/paperscool/daily/route.test.ts src/app/api/research/paperscool/analyze/route.test.ts src/app/api/research/repro/context/route.test.tsnpx eslint src/app/api/_utils/backend-proxy.ts src/app/api/_utils/backend-proxy.test.ts src/app/api/papers/export/route.ts src/app/api/papers/export/route.test.ts src/app/api/newsletter/unsubscribe/[token]/route.ts src/app/api/newsletter/unsubscribe/[token]/route.test.ts src/app/api/studio/chat/route.ts src/app/api/studio/chat/route.test.ts src/app/api/gen-code/route.ts src/app/api/gen-code/route.test.ts src/app/api/research/paperscool/daily/route.ts src/app/api/research/paperscool/daily/route.test.ts src/app/api/research/paperscool/analyze/route.ts src/app/api/research/paperscool/analyze/route.test.ts src/app/api/research/repro/context/route.ts src/app/api/research/repro/context/route.test.tsnpx tsc --noEmitnpm run buildResult
rg -n "fetch\(" web/src/app/api --glob 'route.ts'now returns no resultsSummary by CodeRabbit
Tests
Bug Fixes
Refactor