refactor(server): effectify config json route handlers#1026
Conversation
Astro-Han
added
P2
|
Warning Review limit reached
More reviews will be available in 7 minutes and 17 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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 refactors the configuration routes in packages/opencode/src/server/instance/config.ts to use the Effect library and AppRuntime for managing service dependencies and execution. It also introduces a new test suite in packages/opencode/test/server/config-routes.test.ts to verify the GET routes for configuration and providers. The reviewer suggests removing a redundant mapValues call in the provider listing route to simplify the code, and recommends adding a test case for the PATCH /config route to ensure full coverage of the refactored handlers.
| const providers = await AppRuntime.runPromise( | ||
| Effect.gen(function* () { | ||
| const service = yield* Provider.Service | ||
| const items = yield* service.list() | ||
| return mapValues(items, (item) => item) | ||
| }), | ||
| ) |
There was a problem hiding this comment.
The mapValues(items, (item) => item) call is redundant because it maps each key-value pair to itself, creating an identical record. Since service.list() already returns a Record<ProviderID, Info>, you can return items directly.
const providers = await AppRuntime.runPromise(
Effect.gen(function* () {
const service = yield* Provider.Service
return yield* service.list()
}),
)| }) | ||
| }) |
There was a problem hiding this comment.
The test suite covers reading the configuration and listing providers, but it does not verify the PATCH /config route. Adding a test for the update behavior ensures that the refactored update handler works correctly and returns the expected JSON shape, as requested in the review focus.
})
test("updates config through the route runtime", async () => {
await using tmp = await tmpdir({ git: true })
await Instance.provide({
directory: tmp.path,
fn: async () => {
const response = await app().request("/config", {
method: "PATCH",
headers: {
"Content-Type": "application/json",
},
body: JSON.stringify({
model: "openai/gpt-5",
}),
})
const body = await response.json()
expect(response.status).toBe(200)
expect(body.model).toBe("openai/gpt-5")
},
})
})
})1 participant
Summary
Migrate the config JSON route handlers to the existing
AppRuntime.runPromise(Effect.gen(...))route runtime pattern.Why
This continues the ordinary JSON route migration tracked in #936 without changing public route contracts or touching streaming, auth, workspace sync, v2, or SDK/OpenAPI source generation.
Related Issue
Refs #936
Human Review Status
Pending
Review Focus
Please check that
/config,/config/providers, and config update behavior still return the same JSON shapes while using the Effect services.Risk Notes
Skipped conditional checklist items:
How To Verify
Screenshots or Recordings
Not applicable; no visible UI changes.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.