feat(build): implement build synchronization and Docker context management#37
Merged
Conversation
…ement - Added `StartBuildSync` and `StartDockerContextManager` methods to manage build history synchronization and Docker context activation. - Introduced new API endpoints for handling builds, including filtering by tag and retrieving build logs and sources. - Enhanced build artifact handling with new functions for listing and inspecting build history. - Updated configuration to support Docker context management, ensuring seamless integration with Docker CLI. - Added tests for new functionality in build history and Docker context management.
Closed
|
Warning Review limit reached
Next review available in: 7 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 (20)
📝 WalkthroughWalkthroughThis PR adds persisted build history with a background sync from Docker Buildx history, structured build metadata (steps, dependencies, artifacts, tags, timing), an expanded builds API (list/create/detail/source/logs), Docker context (CLI) management, and corresponding Flutter UI: a build detail view, updated builds list, and a settings toggle. ChangesBackend: Build Lifecycle, History Sync, and Docker Context
UI: Build Detail View and Docker Context Settings
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as api.Server
participant Runtime
participant Store as buildstore
Client->>Server: POST /v1/builds
Server->>Server: newBuild(running, platform)
Server-->>Client: 202 Accepted
Server->>Runtime: runBuildJob (goroutine)
Runtime-->>Server: BuildResult
Server->>Store: persistBuildsLocked()
Client->>Server: GET /v1/builds/{id}
Server->>Store: loadBuilds()
Server-->>Client: enriched Build JSON
sequenceDiagram
participant Ticker as StartBuildSync
participant BuildHistory as buildhistory.List
participant Server as api.Server
participant Store as buildstore
Ticker->>BuildHistory: List(ctx, socket)
BuildHistory-->>Ticker: []Row
Ticker->>Server: updateSyncedBuilds / buildFromHistoryRow
Server->>Store: persistBuildsLocked()
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
- Updated the trailing widget condition in the `_SectionHeader` class to use a more concise syntax. - Modified test cases to include a new property `cached` in the `BuildStep` for consistency in build logs handling.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/lib/app_shell.dart (1)
297-326: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
applyConfig()silently resets Docker context management to enabled.This constructs a fresh
Config(...)withoutdockerContextManaged, so it falls back to the constructor's default oftrue(client.dartline 1450) and gets serialized viatoJson()'sdocker_context_managedkey. If a user disables "Use Calf for Docker CLI" and later adjusts CPU/Memory/Swap here, clicking "Apply" will silently re-enable Docker context management, overwriting the user's explicit choice.
setDockerContextManagedalready avoids this by usingcurrent.copyWith(...);applyConfigshould do the same.🐛 Proposed fix
try { final updated = await widget.apiClient.updateConfig( - Config( - pollIntervalMs: current.pollIntervalMs, - cpus: _draftCpus.toInt(), - memoryGB: _draftMemory.toInt(), - memorySwapGB: _draftSwap.toInt(), - hostCPUs: current.hostCPUs, - hostMemoryGB: current.hostMemoryGB, - ), + current.copyWith( + cpus: _draftCpus.toInt(), + memoryGB: _draftMemory.toInt(), + memorySwapGB: _draftSwap.toInt(), + ), );🤖 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 297 - 326, `applyConfig()` is rebuilding a new Config with only CPU, memory, swap, and host values, which drops the existing dockerContextManaged setting and can reset it to the constructor default. Update `applyConfig` in app_shell.dart to preserve the current config state by using `current.copyWith(...)` (as `setDockerContextManaged` already does) or otherwise carrying over `dockerContextManaged` from `current` when calling `widget.apiClient.updateConfig`, so the user’s Docker CLI management preference is not overwritten.
🧹 Nitpick comments (9)
backend/internal/api/build_sync.go (1)
110-126: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueLength-only equality for
Dependencies/Results/Tags.
buildsEqualcompares these slices by length only, whileenrichHistoryBuildre-runsEnrichSyncedBuild/BuildArtifactsunconditionally every cycle (line 164, 166-168). If content changes without a count change, the update is silently dropped (loopcontinues inupdateSyncedBuilds) and never persisted.🤖 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/build_sync.go` around lines 110 - 126, The buildsEqual helper is only comparing Dependencies, Results, and Tags by slice length, so content changes with the same count are missed and updateSyncedBuilds will skip persistence. Update buildsEqual to compare the actual contents of those fields (along with the existing comparisons in buildsEqual) so changes from enrichHistoryBuild/EnrichSyncedBuild and BuildArtifacts are detected even when slice lengths do not change.backend/internal/buildstore/store.go (2)
55-76: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNon-atomic write risks a corrupted
builds.json.
Save()writes directly to the target path. If the process is killed or crashes mid-write (e.g., during shutdown, OOM),builds.jsoncan be left truncated/corrupted, and the nextLoad()will fail, silently discarding all build history (perloadBuilds()'s warn-and-continue behavior inbackend/internal/api/server.go).♻️ Proposed fix: write to a temp file then rename
func Save(builds []runtime.Build, seq int) error { path, err := Path() if err != nil { return err } if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { return err } file := File{ Builds: trimBuilds(builds), Seq: seq, } data, err := json.MarshalIndent(file, "", " ") if err != nil { return err } - return os.WriteFile(path, data, 0o644) + tmp := path + ".tmp" + if err := os.WriteFile(tmp, data, 0o644); err != nil { + return err + } + return os.Rename(tmp, path) }🤖 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/buildstore/store.go` around lines 55 - 76, Save in buildstore/store.go is writing builds.json directly, which can leave the file corrupted if interrupted. Update Save to write the marshaled File data to a temporary file in the same directory, then atomically rename it into place using Path() so the final write is all-or-nothing. Keep the existing error handling around Path, os.MkdirAll, json.MarshalIndent, and os.WriteFile, and make sure the temp-file flow is contained within Save.
78-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUndocumented ordering invariant.
trimBuildskeeps the firstmaxBuildsentries, which is only correct because callers always prepend new builds (newest-first). Worth a short comment noting this invariant so a future refactor toappend-based insertion doesn't silently start dropping the newest builds instead of the oldest.🤖 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/buildstore/store.go` around lines 78 - 84, The trimBuilds helper in store.go relies on an implicit newest-first ordering invariant, since it returns the first maxBuilds entries and only behaves correctly when callers prepend new builds. Add a short comment near trimBuilds explaining that builds must be kept in newest-first order so trimming removes the oldest entries. Reference trimBuilds and any caller that inserts into the builds slice so future changes to append-based insertion do not break this assumption.backend/test/buildstore/buildstore_test.go (1)
12-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the 200-build cap.
The core retention behavior (
trimBuilds) isn't exercised here. Consider adding a case that saves >200 builds and asserts only the first 200 (newest) are retained.🤖 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/test/buildstore/buildstore_test.go` around lines 12 - 49, The current buildstore test only verifies save/load for a single build and does not cover the retention logic in trimBuilds. Extend TestSaveAndLoadBuilds (or add a new test in buildstore_test.go) to save more than 200 runtime.Build entries through buildstore.Save, then load them back with buildstore.Load and assert that only 200 builds remain and that the retained entries are the newest ones in the expected order.backend/internal/buildhistory/history.go (2)
173-208: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
ParseDurationMsdoesn't handle an hour component.Tokens ending in
"h"(e.g.,"1h5m10s") are never matched by either the"m"or"s"suffix checks, so any hour portion is silently dropped/misparsed. Impact is currently mitigated by the timestamp-diff fallback inBuildDurationMs, and available evidence suggests the primaryList()path may not actually populateDurationfrom the JSON format used here, but this remains a latent gap if that assumption changes.🤖 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/buildhistory/history.go` around lines 173 - 208, ParseDurationMs in history.go only handles minutes and seconds, so hour values are skipped when parsing combined durations like 1h5m10s. Update ParseDurationMs to recognize and accumulate an hour component alongside the existing minute/second parsing, and make sure the tokenization logic still works for mixed duration strings used by BuildDurationMs and List().
210-219: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider bounding
runDockerwith its own timeout.This helper backs all buildhistory CLI calls (used by periodic background sync per
build_sync.go). It relies entirely on the caller'sctxfor a deadline; a hang from an unresponsivedockerdaemon would block the calling goroutine indefinitely with no defense at this layer.🛡️ Suggested defensive timeout
func runDocker(ctx context.Context, socket string, args ...string) ([]byte, error) { + ctx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() command := exec.CommandContext(ctx, "docker", args...) command.Env = append(os.Environ(), "DOCKER_HOST=unix://"+socket) output, err := command.CombinedOutput()🤖 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/buildhistory/history.go` around lines 210 - 219, The runDocker helper currently depends only on the caller’s ctx, so a hung docker invocation can block buildhistory work indefinitely. Add a defensive timeout inside runDocker by deriving a child context with context.WithTimeout before calling exec.CommandContext, then cancel it properly after CombinedOutput. Keep the existing error wrapping and apply this change in runDocker so all buildhistory CLI paths inherit the safeguard.backend/internal/runtime/build_parser_test.go (1)
10-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test with a continuation line before
DONE.None of the fixtures place a progress/continuation line (e.g.
#N some progress text) between a step's header and itsDONE, so the header-overwrite issue flagged inbuild_parser.gowouldn't be caught here. Adding a case like aRUNstep with an interim output line beforeDONEwould guard against regressions once that fix lands.🤖 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/runtime/build_parser_test.go` around lines 10 - 61, Add a parser test in TestParseBuildOutputSteps that includes a build step with an intermediate progress/continuation line between the step header and its DONE line, so ParseBuildOutput is exercised against the header-overwrite case. Extend the existing output fixture to include a step like RUN with a non-DONE line before completion, then assert the step is still captured correctly with the right name, caching state, and duration. Use the existing TestParseBuildOutputSteps and ParseBuildOutput symbols to locate the change.ui/test/widget_test.dart (1)
375-400: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew build methods on
_ErrorCalfClientreturn success data instead of simulating a daemon error, unlike its other overrides.Every other method on this class throws
ApiException(..., statusCode: 503)to simulate "daemon unavailable" for error-path tests. The newly addedfetchBuildDetail/fetchBuildSource/fetchBuildLogsinstead return fixed success payloads, so any future widget test using_ErrorCalfClientto exercise the build detail error states (_detailError/_sourceError/_logsError) won't actually see an error.♻️ Proposed fix for consistency
`@override` - Future<BuildDetail> fetchBuildDetail(String id) async => BuildDetail( - id: id, - tag: 'demo', - context: '.', - status: 'success', - createdAt: '2026-01-01T00:00:00Z', - ); - - `@override` - Future<BuildSource> fetchBuildSource(String id) async => const BuildSource( - path: 'Dockerfile', - filename: 'Dockerfile', - content: 'FROM alpine', - platform: 'arm64', - ); - - `@override` - Future<BuildLogs> fetchBuildLogs(String id) async => const BuildLogs( - rawLog: '`#1` DONE 0.1s', - steps: [ - BuildStep(index: 1, total: 1, name: 'load build definition', durationMs: 100), - ], - ); + Future<BuildDetail> fetchBuildDetail(String id) async { + await Future<void>.delayed(Duration.zero); + throw ApiException('daemon unavailable', statusCode: 503); + } + + `@override` + Future<BuildSource> fetchBuildSource(String id) async { + await Future<void>.delayed(Duration.zero); + throw ApiException('daemon unavailable', statusCode: 503); + } + + `@override` + Future<BuildLogs> fetchBuildLogs(String id) async { + await Future<void>.delayed(Duration.zero); + throw ApiException('daemon unavailable', statusCode: 503); + }🤖 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/test/widget_test.dart` around lines 375 - 400, The new build-related overrides on _ErrorCalfClient are returning successful payloads instead of matching the daemon-unavailable behavior used by the rest of the class. Update fetchBuildDetail, fetchBuildSource, and fetchBuildLogs to throw the same ApiException with statusCode 503 that the other _ErrorCalfClient methods use, so widget tests can actually exercise the _detailError, _sourceError, and _logsError paths consistently.ui/lib/screens/build_detail_screen.dart (1)
1296-1335: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate status/duration formatting logic with
resources_screen.dart.
_statusColor/_formatDurationhere are near-identical to_buildStatusColor/_formatBuildDurationadded inui/lib/screens/resources_screen.dart. Consider extracting a sharedbuild_formatting.dart(or similar) helper to avoid divergence.♻️ Proposed shared helper sketch
// ui/lib/utils/build_format.dart Color buildStatusColor(String status, ShadThemeData theme) { ... } String formatBuildDuration(int durationMs, {String zeroLabel = '0.0s'}) { ... }🤖 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/screens/build_detail_screen.dart` around lines 1296 - 1335, The status and duration formatting logic in build_detail_screen is duplicated with the newer helpers in resources_screen, so extract the shared behavior into a common utility and have both screens call it. Move the shared logic from _statusLabel, _statusColor, and _formatDuration alongside the matching _buildStatusColor and _formatBuildDuration usage into a reusable helper such as build_format.dart, then update build_detail_screen to use the shared functions to keep formatting consistent and avoid divergence.
🤖 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/build_sync.go`:
- Around line 61-171: `s.buildsMu` is held while `syncBuildHistory` performs
slow Buildx/CLI-backed enrichment, which can block all build access if a
subprocess hangs. Update `syncBuildHistory` and its helpers
(`updateSyncedBuilds`, `buildFromHistoryRow`, `enrichHistoryBuild`) so external
calls like `buildhistory.Inspect`, `buildhistory.Logs`,
`runtime.EnrichSyncedBuild`, and `buildhistory.BuildArtifacts` run with a
bounded timeout and/or outside the mutex, then re-lock only to merge the updated
`s.builds` results.
- Around line 74-78: The build import path in build_sync.go is reversing the
newest-first order returned by buildhistory.MergeRows by prepending each item
individually inside the imported loop. Update the logic around
buildhistory.MergeRows and s.buildFromHistoryRow so the entire imported batch is
added in one shot, or iterate imported in reverse, and keep trimBuilds working
on the resulting newest-first slice so newer builds are not dropped before older
ones.
In `@backend/internal/api/builds.go`:
- Around line 215-270: The background build path in runBuildJob currently uses
context.Background() when calling s.runtime.RunBuild, so a hung build can run
forever with no cancellation. Update runBuildJob to create a deadline-bound
context for the build execution, pass that context into RunBuild, and ensure the
timeout/cancellation is handled when setting build.Status and build.Error. Use
the existing runBuildJob and s.runtime.RunBuild symbols to locate the change.
- Around line 331-371: enrichHistoryBuildIfNeeded can overwrite a newer snapshot
because it enriches a build read outside buildsMu and then writes the whole
struct back later; adjust the flow so the latest build state is reloaded under
s.buildsMu before persisting, or merge only the missing enrichment fields into
the current slice entry instead of replacing the full runtime.Build. Keep the
fix anchored in enrichHistoryBuildIfNeeded, buildsEqual, and the s.builds update
loop so StartBuildSync updates are preserved.
In `@backend/internal/api/docker_cli.go`:
- Around line 29-37: `Server.cfg` is accessed concurrently without
synchronization, causing a data race between background Docker context checks
and request/migration code. Add a `cfgMu sync.RWMutex` to `Server`, use a read
lock in `ensureDockerContext`, `dockerCLIStatus`, and `configResponse`, and use
a write lock anywhere `s.cfg` is mutated or read during updates in the
`handleConfig` PUT path and the `migrate.go` config save/post-migration flow.
In `@backend/internal/api/handlers.go`:
- Around line 95-110: The PUT handler in handlers.go can hang indefinitely
because activateDockerContext is called synchronously with only r.Context() and
no explicit deadline. Update the request path around activateDockerContext to
use a bounded timeout context (for example via context.WithTimeout) before
invoking the docker CLI work, and make sure the timeout is applied consistently
through config PUT so stalled docker context creation/update/use cannot block
the response forever. Also verify the timeout propagates through
activateDockerContext and any helper it calls, especially contextExists in
context.go.
In `@backend/internal/api/migrate.go`:
- Around line 91-95: The post-migration Docker context activation in
migrateCompleted/activateDockerContext is using context.Background(), which
makes the docker CLI call unbounded and can hang indefinitely. Replace it with
an appropriate cancellable context from the surrounding flow (or derive one with
timeout/deadline consistent with the periodic manager and config PUT handler),
and thread that context through activateDockerContext so the post-migration path
can abort cleanly if docker stalls.
In `@backend/internal/buildhistory/artifacts.go`:
- Around line 171-187: The traceArtifact helper is using buildx history trace,
which returns Jaeger UI/status output instead of actual trace bytes. Update
traceArtifact to either call a real trace export path that produces
OpenTelemetry data before hashing it with digestForBytes, or remove the artifact
entirely if no export is available. Keep the logic in traceArtifact and its
runDocker usage aligned with the real byte-producing source so the BuildArtifact
name, digest, and size reflect exported trace data.
In `@backend/internal/buildhistory/inspect.go`:
- Around line 20-56: The parsing in parseInspectDetail is splitting the buildx
history inspect output on whitespace, which breaks Context values containing
spaces and can bypass the empty check. Update inspect to request structured
output from runDocker for buildx history inspect (for example JSON via the
format flag) and parse that payload directly in parseInspectDetail instead of
using strings.Fields. Add a test case covering a Context path with spaces to
verify the value is preserved end-to-end.
In `@backend/internal/dockercli/context.go`:
- Around line 66-74: The `contextExists` helper currently uses `exec.Command`
without any `context.Context`, so Docker inspection can hang indefinitely and
block callers like `StatusFor` and `EnsureContext`. Update `contextExists` to
accept a context and use a context-aware command execution path, then thread a
bounded timeout context from the `StatusFor` and `EnsureContext` call sites so
Docker CLI checks can be canceled or time-limited. Keep the change localized
around `contextExists`, `StatusFor`, and `EnsureContext` to preserve existing
behavior while preventing unbounded blocking.
In `@backend/internal/runtime/build_parser.go`:
- Around line 10-15: The build parser is treating continuation/progress lines as
new headers, which overwrites the current step name and can misclassify timing
in classifyTiming. Update the parsing logic in build_parser.go around
buildStepHeaderRe and the step-handling branch so that a line only becomes a
header when it is the first match for a step; if a step is already open, route
bracket-less progress/output lines into the log-append path instead of resetting
step.Name. Use buildStepHeaderRe, classifyTiming, and the existing open-step
handling to keep genuine headers like exporting lines working while preserving
the original step display name and timing bucket.
In `@ui/lib/screens/build_detail_screen.dart`:
- Around line 592-665: The timing charts in _TimingCharts are double-counting
values and producing misleading pie percentages. Update the “Accumulated time”
card to use its own aggregated slices instead of reusing realTime, and revise
cacheSlices so the segments represent a single partition of totalSteps rather
than including both total and its parts. Also fix the “Parallel execution”
slices so Active and Idle are mutually exclusive and do not sum to totalMs
twice; use the existing BuildTiming fields in _timingSlices and
_TimingChartCardState to keep tooltip percentages accurate.
- Around line 517-531: The Build results table is passing `item.size` as the
last cell, so `_DataTable`’s copy action uses the wrong value. Update the
`BuildDetailScreen` rows passed into `_DataTable` so the digest is the final
entry, matching the copy behavior used by the other tables. Keep the `columns`
and row order aligned in the table-building logic around `results.map(...)`.
- Around line 545-590: The _PlatformFilter button menu is anchored to a fixed
overlay origin instead of the trigger widget. Update the showMenu call inside
_PlatformFilter.build/onPressed to compute the popup position from the button’s
actual RenderBox/overlay location, so the menu opens aligned under the “Filter
by platform” CalfButton rather than at the screen corner.
---
Outside diff comments:
In `@ui/lib/app_shell.dart`:
- Around line 297-326: `applyConfig()` is rebuilding a new Config with only CPU,
memory, swap, and host values, which drops the existing dockerContextManaged
setting and can reset it to the constructor default. Update `applyConfig` in
app_shell.dart to preserve the current config state by using
`current.copyWith(...)` (as `setDockerContextManaged` already does) or otherwise
carrying over `dockerContextManaged` from `current` when calling
`widget.apiClient.updateConfig`, so the user’s Docker CLI management preference
is not overwritten.
---
Nitpick comments:
In `@backend/internal/api/build_sync.go`:
- Around line 110-126: The buildsEqual helper is only comparing Dependencies,
Results, and Tags by slice length, so content changes with the same count are
missed and updateSyncedBuilds will skip persistence. Update buildsEqual to
compare the actual contents of those fields (along with the existing comparisons
in buildsEqual) so changes from enrichHistoryBuild/EnrichSyncedBuild and
BuildArtifacts are detected even when slice lengths do not change.
In `@backend/internal/buildhistory/history.go`:
- Around line 173-208: ParseDurationMs in history.go only handles minutes and
seconds, so hour values are skipped when parsing combined durations like
1h5m10s. Update ParseDurationMs to recognize and accumulate an hour component
alongside the existing minute/second parsing, and make sure the tokenization
logic still works for mixed duration strings used by BuildDurationMs and List().
- Around line 210-219: The runDocker helper currently depends only on the
caller’s ctx, so a hung docker invocation can block buildhistory work
indefinitely. Add a defensive timeout inside runDocker by deriving a child
context with context.WithTimeout before calling exec.CommandContext, then cancel
it properly after CombinedOutput. Keep the existing error wrapping and apply
this change in runDocker so all buildhistory CLI paths inherit the safeguard.
In `@backend/internal/buildstore/store.go`:
- Around line 55-76: Save in buildstore/store.go is writing builds.json
directly, which can leave the file corrupted if interrupted. Update Save to
write the marshaled File data to a temporary file in the same directory, then
atomically rename it into place using Path() so the final write is
all-or-nothing. Keep the existing error handling around Path, os.MkdirAll,
json.MarshalIndent, and os.WriteFile, and make sure the temp-file flow is
contained within Save.
- Around line 78-84: The trimBuilds helper in store.go relies on an implicit
newest-first ordering invariant, since it returns the first maxBuilds entries
and only behaves correctly when callers prepend new builds. Add a short comment
near trimBuilds explaining that builds must be kept in newest-first order so
trimming removes the oldest entries. Reference trimBuilds and any caller that
inserts into the builds slice so future changes to append-based insertion do not
break this assumption.
In `@backend/internal/runtime/build_parser_test.go`:
- Around line 10-61: Add a parser test in TestParseBuildOutputSteps that
includes a build step with an intermediate progress/continuation line between
the step header and its DONE line, so ParseBuildOutput is exercised against the
header-overwrite case. Extend the existing output fixture to include a step like
RUN with a non-DONE line before completion, then assert the step is still
captured correctly with the right name, caching state, and duration. Use the
existing TestParseBuildOutputSteps and ParseBuildOutput symbols to locate the
change.
In `@backend/test/buildstore/buildstore_test.go`:
- Around line 12-49: The current buildstore test only verifies save/load for a
single build and does not cover the retention logic in trimBuilds. Extend
TestSaveAndLoadBuilds (or add a new test in buildstore_test.go) to save more
than 200 runtime.Build entries through buildstore.Save, then load them back with
buildstore.Load and assert that only 200 builds remain and that the retained
entries are the newest ones in the expected order.
In `@ui/lib/screens/build_detail_screen.dart`:
- Around line 1296-1335: The status and duration formatting logic in
build_detail_screen is duplicated with the newer helpers in resources_screen, so
extract the shared behavior into a common utility and have both screens call it.
Move the shared logic from _statusLabel, _statusColor, and _formatDuration
alongside the matching _buildStatusColor and _formatBuildDuration usage into a
reusable helper such as build_format.dart, then update build_detail_screen to
use the shared functions to keep formatting consistent and avoid divergence.
In `@ui/test/widget_test.dart`:
- Around line 375-400: The new build-related overrides on _ErrorCalfClient are
returning successful payloads instead of matching the daemon-unavailable
behavior used by the rest of the class. Update fetchBuildDetail,
fetchBuildSource, and fetchBuildLogs to throw the same ApiException with
statusCode 503 that the other _ErrorCalfClient methods use, so widget tests can
actually exercise the _detailError, _sourceError, and _logsError paths
consistently.
🪄 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: 03d8000b-ee33-4c43-8cab-fae41ac7ca0c
📒 Files selected for processing (41)
ROADMAP.mdbackend/api.testbackend/buildstore.testbackend/cmd/calf/main.gobackend/internal/api/build_sync.gobackend/internal/api/builds.gobackend/internal/api/docker_cli.gobackend/internal/api/handlers.gobackend/internal/api/migrate.gobackend/internal/api/server.gobackend/internal/buildhistory/artifacts.gobackend/internal/buildhistory/artifacts_test.gobackend/internal/buildhistory/history.gobackend/internal/buildhistory/history_test.gobackend/internal/buildhistory/inspect.gobackend/internal/buildhistory/inspect_test.gobackend/internal/buildhistory/logs.gobackend/internal/buildstore/store.gobackend/internal/config/config.gobackend/internal/config/config.yamlbackend/internal/dockercli/context.gobackend/internal/dockercli/context_test.gobackend/internal/runtime/build.gobackend/internal/runtime/build_enrich.gobackend/internal/runtime/build_enrich_test.gobackend/internal/runtime/build_parser.gobackend/internal/runtime/build_parser_test.gobackend/internal/runtime/lima.gobackend/internal/runtime/mock.gobackend/internal/runtime/native.gobackend/internal/runtime/nerdctl.gobackend/internal/runtime/runtime.gobackend/runtime.testbackend/test/api/api_test.gobackend/test/api/builds_test.gobackend/test/buildstore/buildstore_test.goui/lib/api/client.dartui/lib/app_shell.dartui/lib/screens/build_detail_screen.dartui/lib/screens/resources_screen.dartui/test/widget_test.dart
Comment on lines
+29
to
+37
| func (s *Server) ensureDockerContext(ctx context.Context) { | ||
| if !s.cfg.DockerContextManaged { | ||
| return | ||
| } | ||
|
|
||
| socket := s.runtime.DockerSocket() | ||
| if socket == "" { | ||
| return | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Data race on s.cfg across goroutines.
ensureDockerContext reads s.cfg.DockerContextManaged from the periodic background goroutine (StartDockerContextManager ticker), while handlers.go's handleConfig PUT branch and migrate.go's post-migration block mutate/read the same Server.cfg fields from separate per-request/migration goroutines. Server has no mutex protecting cfg (only buildsMu/migrateMu exist for other fields), so this is an unsynchronized concurrent read/write — a data race per Go's memory model.
🔒 Suggested approach
Add a cfgMu sync.RWMutex to Server, take a read lock in ensureDockerContext/dockerCLIStatus/configResponse, and a write lock wherever s.cfg fields are mutated (handlers.go PUT handler, migrate.go's SaveConfig callback and post-migration activation check), mirroring the existing buildsMu/migrateMu pattern.
🤖 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/docker_cli.go` around lines 29 - 37, `Server.cfg` is
accessed concurrently without synchronization, causing a data race between
background Docker context checks and request/migration code. Add a `cfgMu
sync.RWMutex` to `Server`, use a read lock in `ensureDockerContext`,
`dockerCLIStatus`, and `configResponse`, and use a write lock anywhere `s.cfg`
is mutated or read during updates in the `handleConfig` PUT path and the
`migrate.go` config save/post-migration flow.
Comment on lines
+517
to
+531
| _DataTable( | ||
| theme: theme, | ||
| columns: const ['Artifact', 'Platform', 'Digest', 'Size'], | ||
| rows: results | ||
| .map( | ||
| (item) => [ | ||
| item.name, | ||
| item.platform, | ||
| item.digest, | ||
| item.size, | ||
| ], | ||
| ) | ||
| .toList(), | ||
| onCopy: onCopy, | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Copy button on the "Build results" table copies Size, not Digest, unlike the other tables.
_DataTable assumes row.last is the copyable value (works for Dependencies/Tags where the last column is Digest), but here the last column is item.size, so tapping copy on a result row copies the size string instead of the digest.
🐛 Proposed fix — reorder columns so digest is last
_DataTable(
theme: theme,
- columns: const ['Artifact', 'Platform', 'Digest', 'Size'],
+ columns: const ['Artifact', 'Platform', 'Size', 'Digest'],
rows: results
.map(
(item) => [
item.name,
item.platform,
- item.digest,
item.size,
+ item.digest,
],
)
.toList(),
onCopy: onCopy,
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _DataTable( | |
| theme: theme, | |
| columns: const ['Artifact', 'Platform', 'Digest', 'Size'], | |
| rows: results | |
| .map( | |
| (item) => [ | |
| item.name, | |
| item.platform, | |
| item.digest, | |
| item.size, | |
| ], | |
| ) | |
| .toList(), | |
| onCopy: onCopy, | |
| ), | |
| _DataTable( | |
| theme: theme, | |
| columns: const ['Artifact', 'Platform', 'Size', 'Digest'], | |
| rows: results | |
| .map( | |
| (item) => [ | |
| item.name, | |
| item.platform, | |
| item.size, | |
| item.digest, | |
| ], | |
| ) | |
| .toList(), | |
| onCopy: onCopy, | |
| ), |
🤖 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/screens/build_detail_screen.dart` around lines 517 - 531, The Build
results table is passing `item.size` as the last cell, so `_DataTable`’s copy
action uses the wrong value. Update the `BuildDetailScreen` rows passed into
`_DataTable` so the digest is the final entry, matching the copy behavior used
by the other tables. Keep the `columns` and row order aligned in the
table-building logic around `results.map(...)`.
Comment on lines
+592
to
+665
| class _TimingCharts extends StatelessWidget { | ||
| const _TimingCharts({ | ||
| required this.theme, | ||
| required this.timing, | ||
| required this.totalMs, | ||
| required this.cachedSteps, | ||
| required this.totalSteps, | ||
| }); | ||
|
|
||
| final ShadThemeData theme; | ||
| final BuildTiming timing; | ||
| final int totalMs; | ||
| final int cachedSteps; | ||
| final int totalSteps; | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| final realTime = _timingSlices(timing); | ||
| final cacheSlices = [ | ||
| _TimingSlice('Executions', totalSteps.toDouble(), const Color(0xFF3B82F6)), | ||
| _TimingSlice('Cached steps', cachedSteps.toDouble(), const Color(0xFF22C55E)), | ||
| _TimingSlice('Other steps', (totalSteps - cachedSteps).toDouble().clamp(0, double.infinity), theme.colorScheme.mutedForeground), | ||
| ]; | ||
|
|
||
| return GridView.count( | ||
| crossAxisCount: 2, | ||
| shrinkWrap: true, | ||
| physics: const NeverScrollableScrollPhysics(), | ||
| crossAxisSpacing: 12, | ||
| mainAxisSpacing: 12, | ||
| childAspectRatio: 1.4, | ||
| children: [ | ||
| _TimingChartCard( | ||
| theme: theme, | ||
| title: 'Real time (${_formatDuration(totalMs)})', | ||
| slices: realTime, | ||
| formatValue: (value) => _formatDuration(value.toInt()), | ||
| ), | ||
| _TimingChartCard( | ||
| theme: theme, | ||
| title: 'Accumulated time (${_formatDuration(totalMs)})', | ||
| slices: realTime, | ||
| formatValue: (value) => _formatDuration(value.toInt()), | ||
| ), | ||
| _TimingChartCard( | ||
| theme: theme, | ||
| title: 'Cache usage ($cachedSteps/$totalSteps)', | ||
| slices: cacheSlices, | ||
| formatValue: (value) => '${value.toInt()}', | ||
| ), | ||
| _TimingChartCard( | ||
| theme: theme, | ||
| title: 'Parallel execution', | ||
| slices: [ | ||
| _TimingSlice('Active', totalMs > 0 ? totalMs.toDouble() : 1, const Color(0xFF3B82F6)), | ||
| _TimingSlice('Idle', timing.idleMs.toDouble(), theme.colorScheme.mutedForeground), | ||
| ], | ||
| formatValue: (value) => _formatDuration(value.toInt()), | ||
| ), | ||
| ], | ||
| ); | ||
| } | ||
|
|
||
| List<_TimingSlice> _timingSlices(BuildTiming timing) { | ||
| return [ | ||
| _TimingSlice('Local file transfers', timing.localTransfersMs.toDouble(), const Color(0xFF166534)), | ||
| _TimingSlice('Image pulls', timing.imagePullsMs.toDouble(), const Color(0xFF4ADE80)), | ||
| _TimingSlice('Executions', timing.executionsMs.toDouble(), const Color(0xFF3B82F6)), | ||
| _TimingSlice('File operations', timing.fileOperationsMs.toDouble(), const Color(0xFFEF4444)), | ||
| _TimingSlice('Result exports', timing.resultExportsMs.toDouble(), const Color(0xFFA855F7)), | ||
| _TimingSlice('Idle', timing.idleMs.toDouble(), theme.colorScheme.mutedForeground), | ||
| ].where((slice) => slice.value > 0).toList(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Timing pie charts double-count totals, producing misleading percentages.
Two issues in _TimingCharts:
- "Real time" and "Accumulated time" cards (Lines 624-635) both render
slices: realTime— they show identical data, so "Accumulated time" adds no information. cacheSlices(Lines 610-614) includes'Executions'(=totalSteps) alongside'Cached steps'(=cachedSteps) and'Other steps'(=totalSteps - cachedSteps). SinceCached + Other == Executions == totalSteps, the slices sum to2 × totalSteps, so the pie (and the touch tooltip percentage in_TimingChartCardState) will show roughly half the correct cache ratio.- Similarly, the "Parallel execution" chart (Lines 642-650) sets
'Active' = totalMsand'Idle' = timing.idleMs, but idle time is presumably already part oftotalMs, so this again double-counts and skews the percentages.
🐛 Proposed fix for the cache-usage double counting
- final cacheSlices = [
- _TimingSlice('Executions', totalSteps.toDouble(), const Color(0xFF3B82F6)),
- _TimingSlice('Cached steps', cachedSteps.toDouble(), const Color(0xFF22C55E)),
- _TimingSlice('Other steps', (totalSteps - cachedSteps).toDouble().clamp(0, double.infinity), theme.colorScheme.mutedForeground),
- ];
+ final cacheSlices = [
+ _TimingSlice('Cached steps', cachedSteps.toDouble(), const Color(0xFF22C55E)),
+ _TimingSlice('Other steps', (totalSteps - cachedSteps).toDouble().clamp(0, double.infinity), theme.colorScheme.mutedForeground),
+ ];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| class _TimingCharts extends StatelessWidget { | |
| const _TimingCharts({ | |
| required this.theme, | |
| required this.timing, | |
| required this.totalMs, | |
| required this.cachedSteps, | |
| required this.totalSteps, | |
| }); | |
| final ShadThemeData theme; | |
| final BuildTiming timing; | |
| final int totalMs; | |
| final int cachedSteps; | |
| final int totalSteps; | |
| @override | |
| Widget build(BuildContext context) { | |
| final realTime = _timingSlices(timing); | |
| final cacheSlices = [ | |
| _TimingSlice('Executions', totalSteps.toDouble(), const Color(0xFF3B82F6)), | |
| _TimingSlice('Cached steps', cachedSteps.toDouble(), const Color(0xFF22C55E)), | |
| _TimingSlice('Other steps', (totalSteps - cachedSteps).toDouble().clamp(0, double.infinity), theme.colorScheme.mutedForeground), | |
| ]; | |
| return GridView.count( | |
| crossAxisCount: 2, | |
| shrinkWrap: true, | |
| physics: const NeverScrollableScrollPhysics(), | |
| crossAxisSpacing: 12, | |
| mainAxisSpacing: 12, | |
| childAspectRatio: 1.4, | |
| children: [ | |
| _TimingChartCard( | |
| theme: theme, | |
| title: 'Real time (${_formatDuration(totalMs)})', | |
| slices: realTime, | |
| formatValue: (value) => _formatDuration(value.toInt()), | |
| ), | |
| _TimingChartCard( | |
| theme: theme, | |
| title: 'Accumulated time (${_formatDuration(totalMs)})', | |
| slices: realTime, | |
| formatValue: (value) => _formatDuration(value.toInt()), | |
| ), | |
| _TimingChartCard( | |
| theme: theme, | |
| title: 'Cache usage ($cachedSteps/$totalSteps)', | |
| slices: cacheSlices, | |
| formatValue: (value) => '${value.toInt()}', | |
| ), | |
| _TimingChartCard( | |
| theme: theme, | |
| title: 'Parallel execution', | |
| slices: [ | |
| _TimingSlice('Active', totalMs > 0 ? totalMs.toDouble() : 1, const Color(0xFF3B82F6)), | |
| _TimingSlice('Idle', timing.idleMs.toDouble(), theme.colorScheme.mutedForeground), | |
| ], | |
| formatValue: (value) => _formatDuration(value.toInt()), | |
| ), | |
| ], | |
| ); | |
| } | |
| List<_TimingSlice> _timingSlices(BuildTiming timing) { | |
| return [ | |
| _TimingSlice('Local file transfers', timing.localTransfersMs.toDouble(), const Color(0xFF166534)), | |
| _TimingSlice('Image pulls', timing.imagePullsMs.toDouble(), const Color(0xFF4ADE80)), | |
| _TimingSlice('Executions', timing.executionsMs.toDouble(), const Color(0xFF3B82F6)), | |
| _TimingSlice('File operations', timing.fileOperationsMs.toDouble(), const Color(0xFFEF4444)), | |
| _TimingSlice('Result exports', timing.resultExportsMs.toDouble(), const Color(0xFFA855F7)), | |
| _TimingSlice('Idle', timing.idleMs.toDouble(), theme.colorScheme.mutedForeground), | |
| ].where((slice) => slice.value > 0).toList(); | |
| } | |
| } | |
| class _TimingCharts extends StatelessWidget { | |
| const _TimingCharts({ | |
| required this.theme, | |
| required this.timing, | |
| required this.totalMs, | |
| required this.cachedSteps, | |
| required this.totalSteps, | |
| }); | |
| final ShadThemeData theme; | |
| final BuildTiming timing; | |
| final int totalMs; | |
| final int cachedSteps; | |
| final int totalSteps; | |
| `@override` | |
| Widget build(BuildContext context) { | |
| final realTime = _timingSlices(timing); | |
| final cacheSlices = [ | |
| _TimingSlice('Cached steps', cachedSteps.toDouble(), const Color(0xFF22C55E)), | |
| _TimingSlice('Other steps', (totalSteps - cachedSteps).toDouble().clamp(0, double.infinity), theme.colorScheme.mutedForeground), | |
| ]; | |
| return GridView.count( | |
| crossAxisCount: 2, | |
| shrinkWrap: true, | |
| physics: const NeverScrollableScrollPhysics(), | |
| crossAxisSpacing: 12, | |
| mainAxisSpacing: 12, | |
| childAspectRatio: 1.4, | |
| children: [ | |
| _TimingChartCard( | |
| theme: theme, | |
| title: 'Real time (${_formatDuration(totalMs)})', | |
| slices: realTime, | |
| formatValue: (value) => _formatDuration(value.toInt()), | |
| ), | |
| _TimingChartCard( | |
| theme: theme, | |
| title: 'Accumulated time (${_formatDuration(totalMs)})', | |
| slices: realTime, | |
| formatValue: (value) => _formatDuration(value.toInt()), | |
| ), | |
| _TimingChartCard( | |
| theme: theme, | |
| title: 'Cache usage ($cachedSteps/$totalSteps)', | |
| slices: cacheSlices, | |
| formatValue: (value) => '${value.toInt()}', | |
| ), | |
| _TimingChartCard( | |
| theme: theme, | |
| title: 'Parallel execution', | |
| slices: [ | |
| _TimingSlice('Active', totalMs > 0 ? totalMs.toDouble() : 1, const Color(0xFF3B82F6)), | |
| _TimingSlice('Idle', timing.idleMs.toDouble(), theme.colorScheme.mutedForeground), | |
| ], | |
| formatValue: (value) => _formatDuration(value.toInt()), | |
| ), | |
| ], | |
| ); | |
| } | |
| List<_TimingSlice> _timingSlices(BuildTiming timing) { | |
| return [ | |
| _TimingSlice('Local file transfers', timing.localTransfersMs.toDouble(), const Color(0xFF166534)), | |
| _TimingSlice('Image pulls', timing.imagePullsMs.toDouble(), const Color(0xFF4ADE80)), | |
| _TimingSlice('Executions', timing.executionsMs.toDouble(), const Color(0xFF3B82F6)), | |
| _TimingSlice('File operations', timing.fileOperationsMs.toDouble(), const Color(0xFFEF4444)), | |
| _TimingSlice('Result exports', timing.resultExportsMs.toDouble(), const Color(0xFFA855F7)), | |
| _TimingSlice('Idle', timing.idleMs.toDouble(), theme.colorScheme.mutedForeground), | |
| ].where((slice) => slice.value > 0).toList(); | |
| } | |
| } |
🤖 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/screens/build_detail_screen.dart` around lines 592 - 665, The timing
charts in _TimingCharts are double-counting values and producing misleading pie
percentages. Update the “Accumulated time” card to use its own aggregated slices
instead of reusing realTime, and revise cacheSlices so the segments represent a
single partition of totalSteps rather than including both total and its parts.
Also fix the “Parallel execution” slices so Active and Idle are mutually
exclusive and do not sum to totalMs twice; use the existing BuildTiming fields
in _timingSlices and _TimingChartCardState to keep tooltip percentages accurate.
- Introduced timeouts for build synchronization and Docker context operations to improve reliability. - Refactored build history synchronization logic to utilize enriched build data and ensure accurate updates. - Updated context management to handle Docker context activation with timeouts, enhancing responsiveness. - Improved error handling in build job execution to provide clearer feedback on timeouts and cancellations. - Added tests for new timeout behaviors and build synchronization enhancements.
This was referenced Jul 8, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
StartBuildSyncandStartDockerContextManagermethods to manage build history synchronization and Docker context activation.Summary by CodeRabbit
New Features
Bug Fixes