feat: Phase 0 — Foundations#9
Conversation
Move entry point to cmd/calf and extract api/config packages. Closes #2. Co-authored-by: Cursor <cursoragent@cursor.com>
Add GET /v1/health and GET /v1/status with JSON responses. Remove the /hello placeholder endpoint. Closes #3. Co-authored-by: Cursor <cursoragent@cursor.com>
Load settings from ~/.config/calf/config.yaml with defaults on first run. Expose log_level in /v1/status. Closes #4. Co-authored-by: Cursor <cursoragent@cursor.com>
Use slog with configurable log level, request logging, panic recovery, and consistent JSON error responses. Closes #5. Co-authored-by: Cursor <cursoragent@cursor.com>
Add sidebar navigation, daemon status view, and read-only settings screen backed by /v1/status. Closes #6. Co-authored-by: Cursor <cursoragent@cursor.com>
Add Makefile, bin/ gitignore, and build docs in DEVELOPMENT.md. Closes #7. Co-authored-by: Cursor <cursoragent@cursor.com>
Run go vet/test/build and flutter analyze/test/build on push and PR. Closes #8. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR restructures the Go backend into cmd/internal/api and internal/config packages, adds a versioned REST API (/v1/health, /v1/status) with middleware and persistent YAML configuration, removes the old placeholder server, adds a Flutter navigation shell consuming the new API, and adds CI/build tooling with updated docs. ChangesBackend daemon restructure and API
Estimated code review effort: 3 (Moderate) | ~30 minutes Flutter UI navigation shell and API client
Estimated code review effort: 2 (Simple) | ~15 minutes CI workflow, build tooling, and documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)sequenceDiagram
participant UI as Flutter UI
participant ApiClient
participant Server as api.Server
participant Config as config.Load
UI->>ApiClient: fetchStatus()
ApiClient->>Server: GET /v1/status
Server->>Config: read cfg fields
Server-->>ApiClient: JSON status (version, uptime, listen_addr, log_level)
ApiClient-->>UI: DaemonStatus
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
ui/lib/api/client.dart (2)
47-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider carrying the raw status code.
Storing
statusCodeas anintfield (in addition to the message) would let callers branch on error type without parsing strings later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/api/client.dart` around lines 47 - 54, ApiException only stores a message right now, so callers can’t inspect the HTTP status without parsing text. Update ApiException to carry the raw status code as an int field alongside message, and make sure any construction sites in the API client pass the status through so consumers of ApiException can branch on it directly.
20-27: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueFragile JSON casts produce unclear crashes on schema drift.
Hard
ascasts throw generictype 'Null' is not a subtype of type 'String'errors instead of actionable messages if a field is missing or malformed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/api/client.dart` around lines 20 - 27, The DaemonStatus.fromJson factory uses fragile hard casts for version, uptimeSeconds, listenAddr, and logLevel, which leads to generic crashes when the JSON schema changes. Update DaemonStatus.fromJson in client.dart to validate each expected field with clearer handling for missing or malformed values, and surface an actionable parse error instead of relying on direct as casts.ui/lib/app_shell.dart (2)
100-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate fetch/loading/error state logic between StatusScreen and SettingsScreen.
Both
_StatusScreenStateand_SettingsScreenStateduplicate identical_status/_error/_loadingfields and_loadStatusimplementation. Extract into a shared mixin or a smallStatusFetchercontroller to avoid divergence.Also applies to: 204-239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/app_shell.dart` around lines 100 - 135, The status loading/error state handling is duplicated between _StatusScreenState and _SettingsScreenState, including the _status, _error, _loading fields and the _loadStatus flow. Extract this shared behavior into a reusable mixin or a small StatusFetcher controller, then have both StatusScreen and SettingsScreen delegate to it so the async fetch, mounted checks, and state updates live in one place and cannot diverge.
13-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAppShell offers no seam to inject a test/fake ApiClient.
_apiClientis hardcoded toconst ApiClient()with the defaultlocalhost:8080base URL. Sincemain.dartrendersAppShell()directly, any widget test that pumpsMainApp/AppShellwill trigger a real network call inStatusScreen/SettingsScreen'sinitState. Consider accepting an optionalApiClientinAppShell's constructor (defaulting toconst ApiClient()) so tests can supply a fake. This affectsui/test/widget_test.dart, which currently pumps the real shell with no mocking.🧪 Proposed seam
class AppShell extends StatefulWidget { - const AppShell({super.key}); + const AppShell({super.key, ApiClient? apiClient}) + : _apiClient = apiClient ?? const ApiClient(); + + final ApiClient _apiClient; `@override` State<AppShell> createState() => _AppShellState(); } class _AppShellState extends State<AppShell> { int _selectedIndex = 0; - final _apiClient = const ApiClient(); + ApiClient get _apiClient => widget._apiClient;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/lib/app_shell.dart` around lines 13 - 16, AppShell currently hardcodes its ApiClient, which prevents tests from injecting a fake and causes real network calls when StatusScreen or SettingsScreen initialize. Update AppShell to accept an optional ApiClient through its constructor, defaulting to const ApiClient(), and store/use that injected client in _AppShellState instead of creating it inline. Keep the existing AppShell, _AppShellState, and ApiClient symbols intact so widget tests like widget_test.dart can pass a fake client when pumping MainApp/AppShell..github/workflows/ci.yml (2)
18-18: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon checkout steps.Neither job needs to push, so persisting the token in local git config after checkout unnecessarily widens the attack surface for any subsequent step/dependency.
🔒 Proposed fix
- uses: actions/checkout@v4 + with: + persist-credentials: false - uses: actions/setup-go@v5(apply similarly to the
uijob's checkout step)Also applies to: 33-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 18, The checkout steps in the CI workflow currently use actions/checkout without disabling credential persistence, which leaves the Git token in local config unnecessarily. Update the checkout configuration in both jobs (including the ui job’s checkout step) to set persist-credentials to false, and keep the existing actions/checkout@v4 references so the change is applied consistently.Source: Linters/SAST tools
1-42: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd explicit
permissions:block.No top-level or job-level
permissions:is set, so the workflow runs with the default (often broad)GITHUB_TOKENpermissions. Since these jobs only build/test/lint, scope the token to read-only.🔒 Proposed fix
name: CI +permissions: + contents: read + on: push: branches: - main🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 1 - 42, The CI workflow currently relies on default GITHUB_TOKEN access, so add an explicit permissions block to scope it to read-only for build/test jobs. Update the workflow in ci.yml by adding a top-level permissions setting (or matching job-level permissions for backend and ui) that grants only the minimal read access needed by actions/checkout and the tool setup steps. Keep the existing backend and ui job definitions unchanged aside from the new permissions configuration.Source: Linters/SAST tools
backend/internal/api/middleware.go (1)
9-19: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRequest log omits response status code.
loggingMiddlewarelogs method/path/duration but not the resulting status code, making it hard to distinguish successful vs. failed requests from logs alone.♻️ Capture status via a wrapping ResponseWriter
+type statusRecorder struct { + http.ResponseWriter + status int +} + +func (r *statusRecorder) WriteHeader(status int) { + r.status = status + r.ResponseWriter.WriteHeader(status) +} + func loggingMiddleware(logger *slog.Logger, next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { start := time.Now() - next.ServeHTTP(w, r) + rec := &statusRecorder{ResponseWriter: w, status: http.StatusOK} + next.ServeHTTP(rec, r) logger.Info("request", "method", r.Method, "path", r.URL.Path, + "status", rec.status, "duration_ms", time.Since(start).Milliseconds(), ) }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/api/middleware.go` around lines 9 - 19, The loggingMiddleware request log is missing the response status code, so update it to capture status by wrapping the ResponseWriter before calling next.ServeHTTP. Use a custom ResponseWriter wrapper in loggingMiddleware to record the final status written by the handler, then include that status value in the logger.Info call alongside method, path, and duration_ms so the request log can distinguish success and failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/api/server.go`:
- Around line 25-34: The Server.Run method is using http.ListenAndServe with no
server-level timeouts, which leaves the HTTP daemon vulnerable to slow clients.
Replace the direct ListenAndServe call with an explicit http.Server in Run, keep
the existing mux and withMiddleware handler, and configure ReadTimeout,
WriteTimeout, IdleTimeout, and ReadHeaderTimeout on that server before starting
it with ListenAndServe.
In `@ui/lib/api/client.dart`:
- Around line 35-44: The fetchStatus method in client.dart performs an unbounded
http.get call, which can leave the UI waiting forever if the daemon is
unresponsive. Update fetchStatus to apply a timeout to the HTTP request and
handle the timeout by surfacing an ApiException or equivalent error path,
keeping the behavior consistent with DaemonStatus loading and error handling.
In `@ui/test/widget_test.dart`:
- Around line 2-11: The widget test is relying on MainApp/AppShell’s default
ApiClient, which triggers a real localhost network request and can leave an
unfinished async operation. Update the test to inject a fake ApiClient into
AppShell (or the relevant MainApp wiring) instead of using the hardcoded client,
then use pumpAndSettle to drive the async state changes deterministically and
assert the loading plus final loaded/error UI states.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 18: The checkout steps in the CI workflow currently use actions/checkout
without disabling credential persistence, which leaves the Git token in local
config unnecessarily. Update the checkout configuration in both jobs (including
the ui job’s checkout step) to set persist-credentials to false, and keep the
existing actions/checkout@v4 references so the change is applied consistently.
- Around line 1-42: The CI workflow currently relies on default GITHUB_TOKEN
access, so add an explicit permissions block to scope it to read-only for
build/test jobs. Update the workflow in ci.yml by adding a top-level permissions
setting (or matching job-level permissions for backend and ui) that grants only
the minimal read access needed by actions/checkout and the tool setup steps.
Keep the existing backend and ui job definitions unchanged aside from the new
permissions configuration.
In `@backend/internal/api/middleware.go`:
- Around line 9-19: The loggingMiddleware request log is missing the response
status code, so update it to capture status by wrapping the ResponseWriter
before calling next.ServeHTTP. Use a custom ResponseWriter wrapper in
loggingMiddleware to record the final status written by the handler, then
include that status value in the logger.Info call alongside method, path, and
duration_ms so the request log can distinguish success and failure.
In `@ui/lib/api/client.dart`:
- Around line 47-54: ApiException only stores a message right now, so callers
can’t inspect the HTTP status without parsing text. Update ApiException to carry
the raw status code as an int field alongside message, and make sure any
construction sites in the API client pass the status through so consumers of
ApiException can branch on it directly.
- Around line 20-27: The DaemonStatus.fromJson factory uses fragile hard casts
for version, uptimeSeconds, listenAddr, and logLevel, which leads to generic
crashes when the JSON schema changes. Update DaemonStatus.fromJson in
client.dart to validate each expected field with clearer handling for missing or
malformed values, and surface an actionable parse error instead of relying on
direct as casts.
In `@ui/lib/app_shell.dart`:
- Around line 100-135: The status loading/error state handling is duplicated
between _StatusScreenState and _SettingsScreenState, including the _status,
_error, _loading fields and the _loadStatus flow. Extract this shared behavior
into a reusable mixin or a small StatusFetcher controller, then have both
StatusScreen and SettingsScreen delegate to it so the async fetch, mounted
checks, and state updates live in one place and cannot diverge.
- Around line 13-16: AppShell currently hardcodes its ApiClient, which prevents
tests from injecting a fake and causes real network calls when StatusScreen or
SettingsScreen initialize. Update AppShell to accept an optional ApiClient
through its constructor, defaulting to const ApiClient(), and store/use that
injected client in _AppShellState instead of creating it inline. Keep the
existing AppShell, _AppShellState, and ApiClient symbols intact so widget tests
like widget_test.dart can pass a fake client when pumping MainApp/AppShell.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 17bc29ec-ca4c-4596-b088-0e8fdf1ac8e1
⛔ Files ignored due to path filters (1)
backend/go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
.github/workflows/ci.yml.gitignoreCHANGELOG.mdDEVELOPMENT.mdMakefilebackend/cmd/calf/main.gobackend/go.modbackend/internal/api/handlers.gobackend/internal/api/handlers_test.gobackend/internal/api/middleware.gobackend/internal/api/middleware_test.gobackend/internal/api/server.gobackend/internal/api/version.gobackend/internal/config/config.gobackend/internal/config/config_test.gobackend/internal/config/logger.gobackend/main.goui/lib/api/client.dartui/lib/app_shell.dartui/lib/main.dartui/test/widget_test.dart
💤 Files with no reviewable changes (1)
- backend/main.go
- Introduced a status recorder to capture HTTP response status in logging middleware. - Updated server configuration to include timeouts for improved request handling. - Refactored API client to implement a timeout for status requests and added error handling for response validation. This improves observability and reliability of the backend services.
…cture - Deleted outdated test files for API handlers and middleware. - Introduced new comprehensive tests for health and status endpoints in a dedicated test file. - Updated server handler method to improve testability and maintainability. This refactor enhances the testing framework and ensures better coverage for the API endpoints.
1 participant
Summary
cmd/calfwithinternal/apiandinternal/configGET /v1/health,GET /v1/status)~/.config/calf/config.yaml/v1/statusMakefilefor local builds and macOS CI workflowCloses #1
Closes #2
Closes #3
Closes #4
Closes #5
Closes #6
Closes #7
Closes #8
Test plan
cd backend && CGO_ENABLED=0 go test ./...cd ui && flutter analyze && flutter testmake backend && make uiMade with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores