refactor: unify fallback next json proxies#408
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe changes introduce caching support to the backend proxy utility and refactor three API route handlers to use the centralized proxy mechanism instead of direct fetch calls, ensuring consistent request forwarding and error handling across routes. Changes
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 refactors several Next.js API routes to utilize a unified backend proxy helper. The primary goal is to centralize backend fetch logic, enhance maintainability, and ensure consistent handling of caching and error fallbacks across different API endpoints. This continues the effort to streamline backend interactions, reducing the number of custom fetch implementations and improving code consistency. 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
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
|
There was a problem hiding this comment.
Pull request overview
Continues the proxy cleanup from #407 by migrating three more hand-rolled backend fetch handlers (studio/cwd, studio/status, paperscool/sessions/[sessionId]) to the shared proxyJson helper, adding cache support to the proxy layer, and preserving route-specific fallback behavior via onError.
Changes:
- Added
cacheoption toProxyOptionsand forwarded it to the upstreamfetchcall inbackend-proxy.ts - Migrated
studio/cwd,studio/status, andpaperscool/sessions/[sessionId]routes to useproxyJson - Added focused tests for all three migrated routes and for the new cache-forwarding behavior
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
web/src/app/api/_utils/backend-proxy.ts |
Added cache field to ProxyOptions and passed it through to fetch |
web/src/app/api/_utils/backend-proxy.test.ts |
Test verifying cache directive is forwarded to upstream fetch |
web/src/app/api/studio/status/route.ts |
Migrated to proxyJson with onError fallback |
web/src/app/api/studio/status/route.test.ts |
New test for status route proxy + fallback |
web/src/app/api/studio/cwd/route.ts |
Migrated to proxyJson with onError fallback |
web/src/app/api/studio/cwd/route.test.ts |
New test for cwd route proxy + fallback |
web/src/app/api/research/paperscool/sessions/[sessionId]/route.ts |
Migrated to proxyJson with cache: "no-store" |
web/src/app/api/research/paperscool/sessions/[sessionId]/route.test.ts |
New test for paperscool session proxy |
💡 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.
![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 that unifies several backend proxy route handlers into a shared helper function. The introduction of cache support in proxyJson and its use in the paperscool session route is a good addition. Migrating the studio/cwd and studio/status routes to use the onError callback for their fallback logic significantly cleans up the code and makes it more declarative. The new tests are thorough and correctly validate the refactored logic, including the fallback behaviors. I have a couple of suggestions to further improve observability by adding logging in the new onError handlers.
| onError: () => | ||
| Response.json( | ||
| { | ||
| cwd: process.env.HOME || "/tmp", | ||
| source: "fallback", | ||
| error: "Failed to get working directory from backend", | ||
| }, | ||
| { status: 200 }, | ||
| ), |
There was a problem hiding this comment.
For better observability and easier debugging, it would be beneficial to log the error details provided by the onError context. The original implementation didn't log the error, but since you're refactoring to a more robust proxy helper, this is a good opportunity to add logging.
onError: (context) => {
console.error("Studio CWD proxy failed:", context.error)
return Response.json(
{
cwd: process.env.HOME || "/tmp",
source: "fallback",
error: "Failed to get working directory from backend",
},
{ status: 200 },
)
},| onError: () => | ||
| Response.json( | ||
| { | ||
| claude_cli: false, | ||
| error: "Failed to check Claude CLI status", | ||
| fallback: "anthropic_api", | ||
| }, | ||
| { status: 500 }, | ||
| ), |
There was a problem hiding this comment.
Similar to the cwd route, it would be beneficial for observability to log the error details provided by the onError context here. This will help in debugging issues with the upstream service.
onError: (context) => {
console.error("Studio status proxy failed:", context.error)
return Response.json(
{
claude_cli: false,
error: "Failed to check Claude CLI status",
fallback: "anthropic_api",
},
{ status: 500 },
)
},
jerry609
changed the base branch from
refactor/pr12-next-proxy-remaining
to
dev
jerry609
force-pushed
the
refactor/pr13-next-json-fallback-proxies
branch
from
2462ff9 to
2310e36
Compare
|
Vercel Preview
|
What changed
cachesupport to the shared Next backend proxy helper so routes can preservecache: "no-store"web/src/app/api/studio/cwd,web/src/app/api/studio/status, andweb/src/app/api/research/paperscool/sessions/[sessionId]to the shared proxy helperonErrorinstead of inlinefetch/tryblocksWhy
Validation
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/studio/cwd/route.test.ts src/app/api/studio/status/route.test.ts src/app/api/research/paperscool/sessions/[sessionId]/route.test.tsnpx eslint src/app/api/_utils/backend-proxy.ts src/app/api/_utils/backend-proxy.test.ts src/app/api/studio/cwd/route.ts src/app/api/studio/cwd/route.test.ts src/app/api/studio/status/route.ts src/app/api/studio/status/route.test.ts src/app/api/research/paperscool/sessions/[sessionId]/route.ts src/app/api/research/paperscool/sessions/[sessionId]/route.test.tsnpx tsc --noEmitnpm run buildFollow-up
Summary by CodeRabbit
New Features
Tests