refactor(server): effectify global config route handlers#1021
Conversation
Astro-Han
added
P2
|
Warning Review limit reached
More reviews will be available in 42 minutes and 16 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 global configuration routes to fetch and update configurations using AppRuntime.runPromise and Config.Service instead of direct static calls, and adds a new test suite to verify these routes. The feedback suggests simplifying the route handlers by using Config.Service.use instead of full generator functions with Effect.gen to reduce boilerplate and align with idiomatic patterns in the codebase.
| const config = await AppRuntime.runPromise( | ||
| Effect.gen(function* () { | ||
| const service = yield* Config.Service | ||
| return yield* service.getGlobal() | ||
| }), | ||
| ) |
There was a problem hiding this comment.
Using Config.Service.use is more concise and idiomatic in this codebase than writing a full generator function with Effect.gen for a single service call.
const config = await AppRuntime.runPromise(
Config.Service.use((svc) => svc.getGlobal()),
)| const next = await AppRuntime.runPromise( | ||
| Effect.gen(function* () { | ||
| const service = yield* Config.Service | ||
| return yield* service.updateGlobal(config) | ||
| }), | ||
| ) |
There was a problem hiding this comment.
Similarly, we can simplify this block by using Config.Service.use instead of Effect.gen to reduce boilerplate.
const next = await AppRuntime.runPromise(
Config.Service.use((svc) => svc.updateGlobal(config)),
)1 participant
Summary
Effectify the global configuration JSON handlers by resolving
Config.ServicethroughAppRuntime.Why
Issue #936 is moving route handlers onto AppRuntime-backed services while preserving the existing HTTP API shape. This keeps
/configGET and PATCH on the Effect service path without touching global SSE, dispose, or upgrade routes.Related Issue
Fixes part of #936.
Human Review Status
Pending
Review Focus
Please focus on whether
/configGET/PATCH preserve their response shape and whether the test isolates global config writes to a temporary PawWork Home.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.
Global config file writes are covered by an isolated temporary
PAWWORK_HOMEroute 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.