refactor(server): effectify experimental json route handlers#1020
refactor(server): effectify experimental json route handlers#1020Astro-Han wants to merge 1 commit into
Conversation
Astro-Han
added
P2
|
Warning Review limit reached
More reviews will be available in 45 minutes and 34 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 several experimental routes in packages/opencode/src/server/instance/experimental.ts to run within the Effect runtime using AppRuntime.runPromise and Effect.gen instead of calling static methods directly. It also introduces a new test suite in packages/opencode/test/server/experimental-routes.test.ts to verify these routes. The feedback suggests combining two separate AppRuntime.runPromise calls in the worktree removal route into a single Effect generator to improve efficiency and handle errors more idiomatically.
| const session = await AppRuntime.runPromise( | ||
| Effect.gen(function* () { | ||
| const sessions = yield* Session.Service | ||
| return yield* sessions.findActiveWorktreeBinding(body.directory) | ||
| }), | ||
| ) | ||
| if (session) { | ||
| throw new Error(`Worktree is in use by session "${session.title}". Call ExitWorktree from that session first.`) | ||
| } | ||
| await Worktree.remove(body) | ||
| await AppRuntime.runPromise( | ||
| Effect.gen(function* () { | ||
| const worktrees = yield* Worktree.Service | ||
| yield* worktrees.remove(body) | ||
| }), | ||
| ) |
There was a problem hiding this comment.
These two separate AppRuntime.runPromise calls can be combined into a single Effect generator. Combining them is more efficient as it avoids the overhead of entering and exiting the Effect runtime twice, and it allows you to handle the error flow idiomatically using Effect.fail.
await AppRuntime.runPromise(
Effect.gen(function* () {
const sessions = yield* Session.Service
const session = yield* sessions.findActiveWorktreeBinding(body.directory)
if (session) {
return yield* Effect.fail(
new Error("Worktree is in use by session \"" + session.title + "\". Call ExitWorktree from that session first.")
)
}
const worktrees = yield* Worktree.Service
yield* worktrees.remove(body)
}),
)1 participant
Summary
Effectify the ordinary JSON handlers in the experimental route for tool, worktree, and MCP resource endpoints.
Why
Issue #936 is moving route handlers onto AppRuntime-backed services while preserving the existing HTTP API shape. This keeps the experimental JSON route handlers on the Effect service path without touching the workspace sub-router or session pagination generator.
Related Issue
Fixes part of #936.
Human Review Status
Pending
Review Focus
Please focus on whether the migrated handlers preserve the same response shape and avoid crossing into
/experimental/workspaceor/experimental/sessionbehavior.Risk Notes
No visible UI or copy changed, so the UI checklist item is not applicable.
No platform, packaging, updater, signing, path, shell, or permissions surface was touched, so the platform checklist item is not applicable.
No docs, release notes, dependencies, permissions, credentials, deletion behavior, generated content, or local file surface changed beyond the focused route test.
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.