Opt-in container startup orchestration system#71
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a persisted, dependency-aware startup sequence for containers, with readiness polling, ownership-based shutdown, launch integration, SwiftUI configuration screens, and runtime tests. ChangesStartup sequence orchestration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OrchardApp
participant ContentView
participant StartupSequenceService
participant StartupSequenceRuntimeAdapter
participant ContainerBackend
OrchardApp->>StartupSequenceService: create and inject service
ContentView->>StartupSequenceService: prepareForAutomaticRun()
ContentView->>StartupSequenceService: runIfEnabled(availableContainerIDs)
StartupSequenceService->>StartupSequenceRuntimeAdapter: execute dependency-aware sequence
StartupSequenceRuntimeAdapter->>ContainerBackend: start containers and query status
ContainerBackend-->>StartupSequenceRuntimeAdapter: readiness status
StartupSequenceRuntimeAdapter-->>StartupSequenceService: completed or failed run state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
Orchard/Services/AppServices.swift (1)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent indentation. These lines use tabs for indentation, while the surrounding lines use spaces. Please standardise the indentation to match the rest of the file.
Orchard/Services/AppServices.swift#L22-L24: replace tabs with 4 spaces to align with the surrounding property declarations.Orchard/Services/AppServices.swift#L67-L74: replace tabs with 8 spaces to align with the surrounding method body.🤖 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 `@Orchard/Services/AppServices.swift` around lines 22 - 24, Standardize indentation in Orchard/Services/AppServices.swift: replace tabs with 4 spaces for the property declarations at lines 22-24 and with 8 spaces for the method body at lines 67-74, matching surrounding code.OrchardTests/StartupSequenceServiceTests.swift (2)
65-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMisleading test name.
The test name implies that out-of-order dependencies are rejected, but the assertion explicitly checks that
outOfOrder.validationError() == nil(i.e. they are allowed). Please consider renaming the test to accurately reflect its assertions.♻️ Proposed fix
- `@Test` func rejectsDuplicateAndOutOfOrderDependencies() { + `@Test` func rejectsDuplicatesButAllowsOutOfOrderDependencies() {🤖 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 `@OrchardTests/StartupSequenceServiceTests.swift` around lines 65 - 77, Rename rejectsDuplicateAndOutOfOrderDependencies to accurately describe that duplicate dependencies are rejected while out-of-order dependencies are allowed, preserving both assertions and test behavior.
176-180: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePropagate cancellation in the wait loop.
Swallowing the error from
Task.sleepcan cause a tight loop if the test task is cancelled (for example, by the testing framework's timeout), becauseTask.sleepwill return immediately whileisRunningmay remain true. Explicitly checking for cancellation prevents unnecessary CPU spinning.♻️ Proposed fix
private func waitForRunToFinish(_ service: StartupSequenceService) async { while service.isRunning { try? await Task.sleep(for: .milliseconds(10)) + if Task.isCancelled { break } } }🤖 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 `@OrchardTests/StartupSequenceServiceTests.swift` around lines 176 - 180, Update waitForRunToFinish to check for task cancellation while polling and exit or propagate cancellation instead of swallowing Task.sleep errors. Preserve the existing 10-millisecond polling behavior when the task remains active and uncancelled.Orchard/OrchardAppDelegate.swift (1)
15-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOptional: bound the cleanup task.
stopSequenceOwnedContainers()awaits per-containerruntime.stopStartupContainer(...)with no timeout; if one call hangs,.terminateLaterkeeps the app from quitting until macOS' own termination watchdog fires. Consider a timeout that still callssender.reply(toApplicationShouldTerminate: true)so quit is never stranded.🤖 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 `@Orchard/OrchardAppDelegate.swift` around lines 15 - 20, Bound the cleanup flow in terminationCleanupTask so a hung stopSequenceOwnedContainers call cannot strand application termination. Add a timeout around startupSequenceService.stopSequenceOwnedContainers(), and ensure sender.reply(toApplicationShouldTerminate: true) executes whether cleanup completes or times out, while preserving terminationCleanupTask cleanup.Orchard/Views/Features/Settings/StartupSettingsView.swift (1)
53-63: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrefer identifying groups by their stable
id, not array index.ForEach(...groups.indices, id: \.self)ties view identity to position; combined withmoveGroup(swap) anddeleteGroup(remove), this causes state/animation glitches and risks referencing a stale index during mutation. SinceStartupGrouphas a stableUUID, iterate over the groups (orid: \.id) and resolve the index inside the callbacks.🤖 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 `@Orchard/Views/Features/Settings/StartupSettingsView.swift` around lines 53 - 63, Update the ForEach in StartupSettingsView to identify StartupGroup entries by their stable id instead of groups.indices, and resolve the current array index from that id inside onChange, onMoveUp, onMoveDown, and onDelete before invoking the existing mutation methods. Preserve the current group editor behavior while avoiding stale positional identity during moves and deletions.
🤖 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 `@Orchard/OrchardApp.swift`:
- Around line 10-14: Update OrchardApp.init to assign startupSequenceService
through the existing appDelegate instance directly, rather than retrieving and
casting NSApplication.shared.delegate. Preserve the current
services.startupSequenceService value so applicationShouldTerminate can execute
the shutdown path.
In `@Orchard/Services/StartupSequenceService.swift`:
- Around line 465-483: Update execute(_:) to record container.containerID as
sequence-owned immediately after startStartupContainer succeeds, before
waitUntilRunning. Ensure the parent result handling does not append the same ID
again, while preserving readiness waiting and existing ownership semantics.
- Around line 92-104: Extend dependency validation in the StartupSequenceService
validation flow around containerGroups and the existing waitForContainerIDs
checks to reject cross-group dependency cycles that cannot unblock, including
cases where a container in one group waits on a container whose group is gated
on the first group. Traverse group prerequisites and dependency targets, detect
any cycle involving container dependencies, and return a validation error before
startup; preserve the existing self-reference, duplicate, and missing-container
checks.
In `@Orchard/Views/Features/Settings/StartupSettingsView.swift`:
- Around line 320-328: Update the “Available containers” flow in
StartupSettingsView so it only offers IDs accepted by
validationError(availableContainerIDs:), excluding ungrouped containers from
ungroupedIDs or otherwise aligning the selection with containerGroups. Preserve
the existing exclusion for empty IDs and the current container.containerID.
In `@OrchardTests/StartupSequenceServiceTests.swift`:
- Line 83: In OrchardTests/StartupSequenceServiceTests.swift, update the test
cases at lines 83, 124, 135, and 159-162 to store each generated suite name in a
constant, initialize UserDefaults with that name, and register defer to remove
the corresponding persistent domain after the test completes.
---
Nitpick comments:
In `@Orchard/OrchardAppDelegate.swift`:
- Around line 15-20: Bound the cleanup flow in terminationCleanupTask so a hung
stopSequenceOwnedContainers call cannot strand application termination. Add a
timeout around startupSequenceService.stopSequenceOwnedContainers(), and ensure
sender.reply(toApplicationShouldTerminate: true) executes whether cleanup
completes or times out, while preserving terminationCleanupTask cleanup.
In `@Orchard/Services/AppServices.swift`:
- Around line 22-24: Standardize indentation in
Orchard/Services/AppServices.swift: replace tabs with 4 spaces for the property
declarations at lines 22-24 and with 8 spaces for the method body at lines
67-74, matching surrounding code.
In `@Orchard/Views/Features/Settings/StartupSettingsView.swift`:
- Around line 53-63: Update the ForEach in StartupSettingsView to identify
StartupGroup entries by their stable id instead of groups.indices, and resolve
the current array index from that id inside onChange, onMoveUp, onMoveDown, and
onDelete before invoking the existing mutation methods. Preserve the current
group editor behavior while avoiding stale positional identity during moves and
deletions.
In `@OrchardTests/StartupSequenceServiceTests.swift`:
- Around line 65-77: Rename rejectsDuplicateAndOutOfOrderDependencies to
accurately describe that duplicate dependencies are rejected while out-of-order
dependencies are allowed, preserving both assertions and test behavior.
- Around line 176-180: Update waitForRunToFinish to check for task cancellation
while polling and exit or propagate cancellation instead of swallowing
Task.sleep errors. Preserve the existing 10-millisecond polling behavior when
the task remains active and uncancelled.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: acf1d5cf-086b-435a-aad7-8bfc3046dc2b
📒 Files selected for processing (9)
Orchard/OrchardApp.swiftOrchard/OrchardAppDelegate.swiftOrchard/Services/AppServices.swiftOrchard/Services/StartupSequenceService.swiftOrchard/Views/Features/MenuBar/MenuBarDashboard.swiftOrchard/Views/Features/Settings/SettingsView.swiftOrchard/Views/Features/Settings/StartupSettingsView.swiftOrchard/Views/Layout/Content.swiftOrchardTests/StartupSequenceServiceTests.swift
| for container in group.containers { | ||
| var dependencies = Set<String>() | ||
| for dependencyID in container.waitForContainerIDs { | ||
| guard dependencyID != container.containerID else { | ||
| return "Container \"\(container.containerID)\" cannot wait for itself." | ||
| } | ||
| guard dependencies.insert(dependencyID).inserted else { | ||
| return "Dependency \"\(dependencyID)\" is listed more than once for \"\(container.containerID)\"." | ||
| } | ||
| guard containerGroups[dependencyID] != nil else { | ||
| return "Container \"\(container.containerID)\" waits for a missing container \"\(dependencyID)\"." | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for test coverage of a container waiting on a container in a later/gated group.
rg -nP -C4 'waitForGroupIDs|waitForContainerIDs' OrchardTests/StartupSequenceServiceTests.swiftRepository: andrew-waters/orchard
Length of output: 1284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- StartupSequenceService.swift outline ---'
ast-grep outline Orchard/Services/StartupSequenceService.swift --view expanded || true
echo
echo '--- Relevant test file excerpt ---'
sed -n '80,170p' OrchardTests/StartupSequenceServiceTests.swift
echo
echo '--- Search for readinessTimeout and dependency handling ---'
rg -n 'readinessTimeout|waitForContainerIDs|waitForGroupIDs|validationError|eligible|start' Orchard/Services/StartupSequenceService.swift OrchardTests/StartupSequenceServiceTests.swiftRepository: andrew-waters/orchard
Length of output: 14907
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- validationError() and dependency cycle helpers ---'
sed -n '46,180p' Orchard/Services/StartupSequenceService.swift
echo
echo '--- runtime execution and wait logic ---'
sed -n '404,510p' Orchard/Services/StartupSequenceService.swift
echo
echo '--- test cases around runtime ordering and timeout ---'
sed -n '80,190p' OrchardTests/StartupSequenceServiceTests.swiftRepository: andrew-waters/orchard
Length of output: 12118
Reject container dependencies that can never unblock
A container can wait on another container in a group that is itself gated on the first group finishing (for example, A contains X waiting on Y, while B contains Y and waits for A). That leaves X stuck until readinessTimeout, so this needs validation rather than a runtime failure.
🤖 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 `@Orchard/Services/StartupSequenceService.swift` around lines 92 - 104, Extend
dependency validation in the StartupSequenceService validation flow around
containerGroups and the existing waitForContainerIDs checks to reject
cross-group dependency cycles that cannot unblock, including cases where a
container in one group waits on a container whose group is gated on the first
group. Traverse group prerequisites and dependency targets, detect any cycle
involving container dependencies, and return a validation error before startup;
preserve the existing self-reference, duplicate, and missing-container checks.
| let groupedIDs = Set(allGroups.flatMap { $0.containers.map(\.containerID) }) | ||
| let ungroupedIDs = containerIDs.filter { !$0.isEmpty && !groupedIDs.contains($0) && $0 != container.containerID } | ||
| if !ungroupedIDs.isEmpty { | ||
| Section("Available containers") { | ||
| ForEach(ungroupedIDs, id: \.self) { id in | ||
| prerequisiteToggle(for: id) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and nearby symbols.
ast-grep outline Orchard/Views/Features/Settings/StartupSettingsView.swift --view expanded
# Inspect the relevant range around the snippet and the validator logic.
sed -n '280,360p' Orchard/Views/Features/Settings/StartupSettingsView.swift
# Find the validation code and any messages about missing containers.
rg -n "waits for a missing container|containerGroups\\[dependencyID\\]|validationError|Available containers|prerequisiteToggle|groups\\.indices|id: \\.self" Orchard/Views/Features/Settings/StartupSettingsView.swiftRepository: andrew-waters/orchard
Length of output: 3897
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the validation logic and the group editor list identity.
sed -n '1,120p' Orchard/Views/Features/Settings/StartupSettingsView.swift
sed -n '120,220p' Orchard/Views/Features/Settings/StartupSettingsView.swift
# Show the container dependency check and any surrounding model definitions.
rg -n "wouldCreateContainerDependency|validationError|struct StartupGroup|struct StartupSequence|ForEach\\(startupSequenceService\\.sequence\\.groups\\.indices|id: \\.self" Orchard/Views/Features/Settings/StartupSettingsView.swiftRepository: andrew-waters/orchard
Length of output: 8131
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the sequence validation and dependency model.
rg -n "func validationError|wouldCreateContainerDependency|waitForContainerIDs|waitForGroupIDs|missing container|missing group|availableContainerIDs" Orchard
# Read the sequence model/validation implementation if present.
fd -a "StartupSequence" Orchard
fd -a "StartupGroup" OrchardRepository: andrew-waters/orchard
Length of output: 8504
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,130p' Orchard/Services/StartupSequenceService.swift
sed -n '130,190p' Orchard/Services/StartupSequenceService.swiftRepository: andrew-waters/orchard
Length of output: 6848
"Available containers" offers prerequisites the validator rejects. At Orchard/Views/Features/Settings/StartupSettingsView.swift:320-328, ungroupedIDs includes container IDs that are not in any group, but validationError(availableContainerIDs:) rejects every waitForContainerIDs entry unless it exists in containerGroups. That lets the menu add a dependency that later shows “waits for a missing container” and disables Run Sequence. Filter this section to grouped containers, or loosen the validator if ungrouped dependencies are intended.
🤖 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 `@Orchard/Views/Features/Settings/StartupSettingsView.swift` around lines 320 -
328, Update the “Available containers” flow in StartupSettingsView so it only
offers IDs accepted by validationError(availableContainerIDs:), excluding
ungrouped containers from ungroupedIDs or otherwise aligning the selection with
containerGroups. Preserve the existing exclusion for empty IDs and the current
container.containerID.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Orchard/Views/Features/Settings/StartupSettingsView.swift (1)
179-187: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmpty-state "Add Container" button isn't disabled when no container is available to add.
The non-empty-group "Add Container" button (Lines 203-208) is disabled via
availableContainerIDs.allSatisfy {...}when there's nothing left to add, but this empty-state variant has no such guard. Clicking it with zero eligible containers silently no-ops insideaddContainer()'s guard, which is a confusing (if harmless) UX gap.🩹 Proposed fix
ContentUnavailableView { Label("No containers", systemImage: "shippingbox") } description: { Text("Add a container to this startup group.") } actions: { - Button("Add Container", systemImage: "plus", action: addContainer) + Button("Add Container", systemImage: "plus", action: addContainer) + .disabled(availableContainerIDs.allSatisfy { containersInOtherGroups.contains($0) }) }🤖 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 `@Orchard/Views/Features/Settings/StartupSettingsView.swift` around lines 179 - 187, Update the empty-state “Add Container” button in StartupSettingsView so it uses the same availability guard as the non-empty-group button, disabling it when no eligible container IDs remain. Reuse the existing availableContainerIDs-based condition and leave addContainer() unchanged.
🤖 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 `@Orchard/OrchardAppDelegate.swift`:
- Around line 20-46: Update StartupSequenceService.stopSequenceOwnedContainers
so its reversed container-stop loop checks for cancellation between iterations
and exits promptly when cancelled, ensuring the shutdown budget can interrupt
remaining stop requests. In OrchardAppDelegate’s terminationCleanupTask, await
cleanupTask.value instead of using the completionStream/continuation plumbing,
while preserving the five-second timeout and termination reply behavior.
---
Outside diff comments:
In `@Orchard/Views/Features/Settings/StartupSettingsView.swift`:
- Around line 179-187: Update the empty-state “Add Container” button in
StartupSettingsView so it uses the same availability guard as the
non-empty-group button, disabling it when no eligible container IDs remain.
Reuse the existing availableContainerIDs-based condition and leave
addContainer() unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 17a72475-4eb4-4af2-baa2-a10e683c89dc
📒 Files selected for processing (6)
Orchard/OrchardApp.swiftOrchard/OrchardAppDelegate.swiftOrchard/Services/AppServices.swiftOrchard/Services/StartupSequenceService.swiftOrchard/Views/Features/Settings/StartupSettingsView.swiftOrchardTests/StartupSequenceServiceTests.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- Orchard/Services/AppServices.swift
- Orchard/OrchardApp.swift
- OrchardTests/StartupSequenceServiceTests.swift
- Orchard/Services/StartupSequenceService.swift
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@Orchard/Services/SystemService.swift`:
- Around line 102-106: Update the result.failed branch in the system-start flow
to reset isSystemLoading before returning, ensuring the Start control is no
longer stuck after a failed command. Keep the existing error alert and
checkSystemStatus() behavior unchanged.
- Around line 127-137: Update the readiness polling loop in startSystem around
Task.sleep so task cancellation is preserved instead of converted to false:
detect CancellationError and rethrow or otherwise propagate cancellation, while
retaining false for non-cancellation sleep failures. Ensure the caller does not
show the readiness-failure alert when cancellation propagates.
In `@Orchard/Views/Layout/Content.swift`:
- Line 289: Update StartupSequenceService.prepareForAutomaticRun() to coalesce
concurrent launch requests by memoising the first preparation task or guarding
the start-in-progress state, so later callers await the same operation and only
one startContainerSystem()/startSystem() sequence runs. This applies to the call
sites in Orchard/Views/Layout/Content.swift:289 and
Orchard/Views/Features/MenuBar/MenuBarDashboard.swift:189; no direct call-site
changes are required unless needed to use the shared preparation operation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ad7c429a-c27f-41d3-9728-60826a50cbf7
📒 Files selected for processing (6)
Orchard/Services/StartupSequenceService.swiftOrchard/Services/SystemService.swiftOrchard/Views/Features/MenuBar/MenuBarDashboard.swiftOrchard/Views/Features/Settings/StartupSettingsView.swiftOrchard/Views/Layout/Content.swiftOrchardTests/StartupSequenceServiceTests.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Orchard/Views/Features/Settings/StartupSettingsView.swift
- OrchardTests/StartupSequenceServiceTests.swift
- Orchard/Services/StartupSequenceService.swift
What does this PR do?
Adds an opt-in container startup orchestration system that runs whenever Orchard launches.
The feature allows users to define startup groups containing existing containers, with support for:
status == "running".The sequence is disabled by default for existing users. No macOS login-item registration is included; the sequence runs when Orchard itself opens.
This keeps startup orchestration explicit and safe: Orchard never stops containers that were already running before the sequence began, and downstream containers are not started when their prerequisites fail or time out.
Related issues
No related issue currently linked.
Screenshots / recording
The screenshot shows the new Startup settings interface, including startup groups, group prerequisites, container prerequisites, and sequence controls.
Testing
Added coordinator tests covering:
runningreadiness.The complete test suite passed: