refactor(server): effectify project json route handlers#1029
Conversation
Astro-Han
added
P2
|
Warning Review limit reached
More reviews will be available in 3 minutes and 46 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 project routes in packages/opencode/src/server/instance/project.ts to use Effect and AppRuntime.runPromise for handling asynchronous operations, and adds corresponding integration tests. The review feedback suggests two improvements: using Effect.tryPromise instead of Effect.promise when calling Instance.reload to ensure potential failures are properly captured in the Effect failure channel, and removing the unnecessary AppRuntime.runPromise wrapper around the synchronous Instance.project getter to avoid overhead.
| const next = await AppRuntime.runPromise( | ||
| Effect.gen(function* () { | ||
| const service = yield* Project.Service | ||
| const project = yield* service.initGit({ | ||
| directory: dir, | ||
| project: prev, | ||
| }) | ||
| if (project.id === prev.id && project.vcs === prev.vcs && project.worktree === prev.worktree) return project | ||
| yield* Effect.promise(() => | ||
| Instance.reload({ | ||
| directory: dir, | ||
| worktree: dir, | ||
| project, | ||
| }), | ||
| ) | ||
| return project | ||
| }), | ||
| ) | ||
| return c.json(next) |
There was a problem hiding this comment.
When wrapping operations that can fail (such as database queries or filesystem operations like Instance.reload) in Effect, use Effect.tryPromise with a try/catch structure instead of Effect.promise so that failures are properly captured in the Effect failure channel rather than causing unrecoverable crashes. Ensure regression tests are added to verify that failures stay in the failure channel.
yield* Effect.tryPromise({
try: () => Instance.reload({
directory: dir,
worktree: dir,
project: next,
}),
catch: (error) => new Error(String(error))
})References
- When wrapping operations that can fail (such as database queries or filesystem operations) in Effect, use
Effect.tryPromisewith atry/catchstructure instead ofEffect.promiseso that failures are properly captured in the Effect failure channel rather than causing unrecoverable crashes. Ensure regression tests are added to verify that failures stay in the failure channel.
| const project = await AppRuntime.runPromise( | ||
| Effect.gen(function* () { | ||
| return Instance.project | ||
| }), | ||
| ) | ||
| return c.json(project) |
There was a problem hiding this comment.
Wrapping Instance.project in AppRuntime.runPromise is unnecessary because it is a synchronous getter that does not use any Effect services. We can directly return c.json(Instance.project) to avoid the overhead of creating an Effect fiber.
return c.json(Instance.project)1 participant
Summary
Migrate the project JSON route handlers to the existing
AppRuntime.runPromise(Effect.gen(...))route runtime pattern.Why
This continues the #936 ordinary JSON route migration for the project route group without changing public route contracts or touching forbidden streaming, auth, workspace sync, v2, or SDK/OpenAPI source areas.
Related Issue
Refs #936
Human Review Status
Pending
Review Focus
Please check that project list/current/init/update still delegate to the same project/instance behavior while route service access moves through the Effect runtime.
Risk Notes
PATCH /project/:projectIDis migrated through the sameProject.Service.updatepath; the focused local route coverage exercises list/current/init because the simple fixture does not create an updateable project row without broadening into product setup.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.