feat: implement batch deletion and shift range selection#53
Conversation
2e8c8d9 to
d002e86
Compare
|
Hi @lynicis thanks for the contribution. I've spent quite a bit of time refactoring some of the internals to get some proper test coverage and split out the responsibilities . Unfortunately, that has left this PR with quite a bit of drift. Would you like to rebase your branch and apply the changes again? |
…rs, images, mounts, dns, and networks
d002e86 to
4ada151
Compare
|
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:
📝 WalkthroughWalkthroughOrchard adds shared multi-selection handling, batch deletion APIs, multi-resource list interactions, multi-item detail cards, and selection-state propagation across the layout for images, mounts, DNS domains, and networks. ChangesOrchard multi-selection and batch actions
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ListView
participant SelectionHandler
participant ContentView
participant DetailContentView
participant MultiCardsView
ListView->>SelectionHandler: handleSelection(clickedId, modifiers)
SelectionHandler-->>ListView: updated selection set
ListView->>ContentView: update selected bindings
ContentView->>DetailContentView: propagate selection state
DetailContentView->>MultiCardsView: render when selection count exceeds one
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Orchard/Views/Features/DNS/ListDNS.swift (1)
91-97: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win"Make Default" restores the wrong (dead) selection binding.
This preserves
selectedDNSDomain, but the List's actual selection state isselectedDNSDomains. SincednsService.setDefault()reloads the list, the liveselectedDNSDomainsset is never restored, so the row's visual selection can be lost after making a domain default.🐛 Proposed fix
Button("Make Default") { - let currentSelection = selectedDNSDomain + let currentSelection = selectedDNSDomains Task { await dnsService.setDefault(domain.domain) - selectedDNSDomain = currentSelection + selectedDNSDomains = currentSelection } }🤖 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/DNS/ListDNS.swift` around lines 91 - 97, The "Make Default" action in ListDNS.swift restores the wrong selection state because it writes back to selectedDNSDomain instead of the List’s live selectedDNSDomains set. Update the Button action in the Make Default handler to preserve and restore the actual selection binding used by the list, so after dnsService.setDefault(domain.domain) reloads the data the row remains visually selected. Use the existing List selection state and the Make Default Button closure to locate the fix.Orchard/Views/Layout/DetailContent.swift (1)
26-57: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDNS and Network detail views are missing multi-card wiring;
selectedDNSDomains/selectedNetworksare unused.
imageDetailViewandmountDetailViewwere correctly updated to branch toMultiImageCardsView/MultiMountCardsViewwhen more than one item is selected, but the.dnsand.networkscases inbody(lines 34-57) were not updated the same way.selectedDNSDomainsandselectedNetworks(declared at lines 14 and 16) are threaded all the way down fromContentViewthroughThreeColumnLayoutbut are never referenced in this file's body — they're effectively dead parameters for these two tabs.Given the PR explicitly claims "multi-selection detail summary views for containers, images, mounts, DNS domains, and networks", and this layer's own scope description states it "conditionally renders multi-card views when multiple items are selected" for DNS domains and networks too, this is a functional gap: shift-clicking/Cmd+A-selecting multiple DNS domains or networks will update the underlying
Set(viaDNSListView/NetworksListView), but the detail pane will still fall back to single-selection behaviour (or "Select a DNS domain"/"Select a network").🐛 Suggested fix (mirror the images/mounts pattern)
case .dns: - if let selectedDNSDomain = selectedDNSDomain { + if selectedDNSDomains.count > 1 { + MultiDNSCardsView( + domainIds: selectedDNSDomains, + selectedDNSDomainsBinding: $selectedDNSDomainsBinding + ) + } else if let selectedDNSDomain = selectedDNSDomain { DNSDetailView( domain: selectedDNSDomain, selectedTab: $selectedTabBinding, selectedContainer: $selectedContainerBinding ) } else { Text("Select a DNS domain") .foregroundColor(.secondary) .frame(maxWidth: .infinity, maxHeight: .infinity) } case .networks: - if let selectedNetwork = selectedNetwork { + if selectedNetworks.count > 1 { + MultiNetworkCardsView( + networkIds: selectedNetworks, + selectedNetworksBinding: $selectedNetworksBinding + ) + } else if let selectedNetwork = selectedNetwork { NetworkDetailView( networkId: selectedNetwork, selectedTab: $selectedTabBinding, selectedContainer: $selectedContainerBinding ) } else { Text("Select a network") .foregroundColor(.secondary) .frame(maxWidth: .infinity, maxHeight: .infinity) }Adjust
domainIds/networkIdsparameter names to whateverMultiDNSCardsView/MultiNetworkCardsViewactually declare.🤖 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/Layout/DetailContent.swift` around lines 26 - 57, The DNS and network detail branches in DetailContent.body still only handle single-item selection, so the multi-selection sets selectedDNSDomains and selectedNetworks are unused. Update the .dns and .networks cases to mirror the existing imageDetailView and mountDetailView pattern by switching to the appropriate multi-card views when multiple items are selected, and pass through the matching collection parameters expected by MultiDNSCardsView and MultiNetworkCardsView; keep the existing single-selection fallback for the one-item case.
🧹 Nitpick comments (5)
.github/workflows/release.yml (1)
358-368: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFail loudly when the gh-pages SHA can't be resolved.
git ls-remoteexits0with empty output when the ref isn't found (only a hard transport error is caught by the defaultpipefailshell). An emptyshaoutput then propagates as an emptybuild_version, andpages.ymlsilently falls back togithub.sha— the exact SHAci.ymlalready deployed. That re-introduces the no-op/stale-appcast failure this PR is designed to prevent, with no signal in the logs. A guard surfaces the problem instead of shipping a stale appcast.♻️ Proposed guard
- name: Capture gh-pages commit for the Pages deploy id: ghpages_sha if: steps.appcast.outputs.generated == 'true' run: | SHA=$(git ls-remote "${{ github.server_url }}/${{ github.repository }}" refs/heads/gh-pages | cut -f1) + if [[ -z "$SHA" ]]; then + echo "::error::Could not resolve gh-pages HEAD; the Pages deploy would fall back to github.sha and risk a stale no-op." + exit 1 + fi echo "sha=$SHA" >> "$GITHUB_OUTPUT" echo "gh-pages HEAD: $SHA"🤖 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/release.yml around lines 358 - 368, The gh-pages SHA capture step in release workflow can silently produce an empty sha when git ls-remote returns no ref, causing pages deploy to fall back to github.sha. Update the Capture gh-pages commit for the Pages deploy step to explicitly validate the SHA before writing GITHUB_OUTPUT, and fail the job with a clear error if it is missing. Use the existing ghpages_sha step and the SHA lookup command as the place to add the guard so the release stops instead of deploying a stale appcast.Orchard/Services/ContainerListService.swift (1)
455-494: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate reconstruction logic vs
recoverContainer, with a subtle divergence.The env/port/volume-mapping rebuild here (lines 455-480) duplicates
recoverContainer(lines 345-369) almost verbatim, and the two have already drifted: this method explicitly setsremoveAfterStop: falseandcommandOverride(lines 486, 491), whilerecoverContainer's equivalentrunConfig(lines 371-381) omits both. Extracting a shared private helper (e.g.func runConfig(from container: Container, excludingMounts: [ContainerMount] = [])) would prevent this kind of divergence going forward.🤖 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/ContainerListService.swift` around lines 455 - 494, The run-config reconstruction logic here duplicates the same env/port/volume mapping setup used in recoverContainer and has already diverged on fields like removeAfterStop and commandOverride. Extract the shared rebuild into a private helper, referenced from both ContainerListService methods, so the ContainerRunConfig creation stays consistent and future changes only need to be made in one place.Orchard/Services/DNSService.swift (1)
129-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAggregated error omits underlying failure detail, unlike the sibling services.
ImageService.deleteImagesandNetworkService.deleteNetworksboth tracklastErrorand surfacelastError?.localizedDescriptionin the aggregated alert.deleteDNSDomainsdrops the underlying error entirely ("Failed to delete \(failedCount) DNS domain(s)"), losing diagnostic detail the other two batch methods provide.🤖 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/DNSService.swift` around lines 129 - 150, The aggregated DNS delete alert in deleteDNSDomains currently drops the underlying failure detail. Update DNSService.deleteDNSDomains to track the last encountered error like ImageService.deleteImages and NetworkService.deleteNetworks, and include lastError?.localizedDescription in the alertCenter.error message when failures occur. Keep the existing per-domain loop and counters, but preserve and surface the most recent error so the batch alert matches the sibling service behavior.Orchard/Views/Components/SelectionHandler.swift (1)
24-33: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNon-deterministic new anchor after removing the current anchor via Cmd-click.
When the removed item equals
lastSelectedId, the fallbackselectedSet.first(line 28) picks an arbitrary element sinceSethas no defined order. This makes the next Shift-click's computed range non-deterministic/inconsistent for the user. Prefer a deterministic pick, e.g. the first remaining item in visual order:- if lastSelectedId == clickedId { - lastSelectedId = selectedSet.first - } + if lastSelectedId == clickedId { + lastSelectedId = orderedIds.first { selectedSet.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/Components/SelectionHandler.swift` around lines 24 - 33, The Cmd-click removal path in SelectionHandler should not assign lastSelectedId from selectedSet.first because Set ordering is arbitrary and makes the next range selection unpredictable. Update the selectedSet/remove logic in the command-click branch so that when clickedId matches lastSelectedId, the new anchor is chosen deterministically from the remaining items in visual order using the relevant selection ordering logic in SelectionHandler, rather than relying on Set iteration order.Orchard/Views/Features/DNS/MultiDNSCards.swift (1)
118-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelete confirmation logic duplicated from
ListDNS.swift.This inline
NSAlertconstruction repeats the same pattern asconfirmDNSDomainsDeletioninListDNS.swift(and again inMultiMountCards.swift,MultiNetworkCards.swift). Consider extracting a shared confirmation helper (e.g. onSelectionHandleror a small dedicated utility) to avoid drift between the near-identical implementations.🤖 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/DNS/MultiDNSCards.swift` around lines 118 - 131, The delete confirmation flow in this Button action duplicates the same inline NSAlert logic used elsewhere, so extract it into the shared confirmation helper already represented by confirmDNSDomainsDeletion in ListDNS.swift or a common utility/SelectionHandler method, then update MultiDNSCards to call that helper instead of building the alert locally. Keep the behavior and button titles identical, and reuse the same shared entry point in the other duplicate sites like MultiMountCards and MultiNetworkCards to prevent the implementations from drifting.
🤖 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/ContainerListService.swift`:
- Line 448: The container lookup in ContainerListService’s mount-removal loop is
silently skipping missing containers instead of treating them as failures.
Update the branch around containers.first(where:) so that when no matching
container is found, it increments failedCount and records the failure just like
the other error paths before continuing. Keep the fix localized to the
containerId lookup logic in the mount removal flow.
- Around line 446-509: The batch delete/recreate flow in
ContainerListService.deleteMounts is missing the same per-container
serialization used by startContainer and stopContainer, so concurrent operations
can race on the same container ID. Add a containerOperationLocks-based lock
around the backend.deleteContainer and runContainer sequence for each
containerId, using the same lock key pattern as the existing start/stop paths.
Make sure the lock is acquired before deleting, held through the restart
attempt, and released in a defer so failures still unblock later operations.
- Around line 443-513: deleteMounts(_:) is rebuilding the same container once
per mount using a stale oldConfig snapshot from containers, which can
reintroduce mounts when multiple mounts from one container are deleted together.
Update ContainerListService.deleteMounts(_:) to group the incoming
ContainerMounts by containerId, compute a single filtered mounts list per
container, and recreate each affected container once with all selected mounts
removed before calling backend.deleteContainer and runContainer.
In `@Orchard/Services/DNSService.swift`:
- Around line 129-150: Add a defensive guard in DNSService.delete(_:) so the
current default domain cannot be deleted even if called from DNSDetailHeader.
Mirror the existing default-domain filtering used by ListDNS and MultiDNSCards
before delegating to deleteDNSDomains(_:) or running the delete flow, and exit
early with an appropriate alert or no-op when the target matches the default
domain.
- Around line 129-150: The bulk delete flow in DNSService.deleteDNSDomains
currently invokes runWithSudo once per domain, which can trigger repeated admin
prompts. Update deleteDNSDomains to handle the entire domains array in a single
privileged execution by batching the dns delete operations into one command
using runWithSudo and safeContainerBinaryPath, while preserving
deletedCount/failedCount tracking and the existing load() and alertCenter.error
behavior.
In `@Orchard/Views/Features/DNS/ListDNS.swift`:
- Around line 64-69: The delete/copy confirmation text can show domains in a
non-deterministic order because `targetDomains` in `ListDNS` is built from
`Array(selectedDNSDomains)`, and `selectedDNSDomains` is a `Set`. Update the
`targetDomains` construction to produce a stable order by sorting the set before
use, and make sure the `domainsList` shown in the alert uses that ordered array
consistently. Use the `ListDNS` selection/confirmation flow and the
`domainsList` generation near the confirmation alert as the places to fix,
matching the sorted behavior already used in
`MultiDNSCardsView.selectedDNSList`.
In `@Orchard/Views/Features/Images/ListImages.swift`:
- Around line 80-84: The destructive image removal action in ListImages.swift
currently calls imageService.deleteImages(targetIds) immediately from the Button
handler with no confirmation. Add a confirmation flow consistent with
confirmDNSDomainsDeletion, confirmMountsDeletion, and confirmNetworksDeletion by
introducing a confirmImagesDeletion(references:) helper and invoking it before
deletion, so both single-image and bulk context-menu removals require user
approval.
In `@Orchard/Views/Features/Images/MultiImageCards.swift`:
- Around line 128-131: The ImageSummaryCard “Remove” action deletes immediately,
unlike DNSSummaryCard, MountSummaryCard, and NetworkSummaryCard which confirm
first; update the Button("Remove") handler in ImageSummaryCard to present the
same NSAlert confirmation flow before calling imageService.deleteImages. Use the
existing per-card confirmation pattern from the other summary card actions and
only proceed with Task { await imageService.deleteImages([ref]) } if the user
confirms.
In `@Orchard/Views/Layout/Content.swift`:
- Line 48: The isInIntentionalConfigurationMode state in Content is never set,
so the selection-reset guards in Content’s tab and service update flow are dead
code. Update the no-second-column path around TabColumnView.selectTab and the
related selection handling in Content to set this flag while clearing selections
for .registries, .systemLogs, and .dashboard, then clear it after the
intentional reset so subsequent service updates do not repopulate
selectedContainer, selectedImage, or other cleared state; if that flow is not
needed, remove isInIntentionalConfigurationMode and the guarded branches
entirely.
In `@Orchard/Views/Layout/MainInterface.swift`:
- Around line 37-43: The early isConfigurationMode return in MainInterface
should not override tabs that intentionally have no resource title. Update the
title logic in the relevant switch so that "Configuration" is only used as a
fallback for selection-based tabs, and leave the Dashboard, Registries, and
System Logs cases returning their existing blank title. Use the existing symbols
MainInterface and isConfigurationMode, and keep the per-tab cases for
.dashboard, .registries, and .systemLogs reachable before any generic
nil-selection fallback.
---
Outside diff comments:
In `@Orchard/Views/Features/DNS/ListDNS.swift`:
- Around line 91-97: The "Make Default" action in ListDNS.swift restores the
wrong selection state because it writes back to selectedDNSDomain instead of the
List’s live selectedDNSDomains set. Update the Button action in the Make Default
handler to preserve and restore the actual selection binding used by the list,
so after dnsService.setDefault(domain.domain) reloads the data the row remains
visually selected. Use the existing List selection state and the Make Default
Button closure to locate the fix.
In `@Orchard/Views/Layout/DetailContent.swift`:
- Around line 26-57: The DNS and network detail branches in DetailContent.body
still only handle single-item selection, so the multi-selection sets
selectedDNSDomains and selectedNetworks are unused. Update the .dns and
.networks cases to mirror the existing imageDetailView and mountDetailView
pattern by switching to the appropriate multi-card views when multiple items are
selected, and pass through the matching collection parameters expected by
MultiDNSCardsView and MultiNetworkCardsView; keep the existing single-selection
fallback for the one-item case.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 358-368: The gh-pages SHA capture step in release workflow can
silently produce an empty sha when git ls-remote returns no ref, causing pages
deploy to fall back to github.sha. Update the Capture gh-pages commit for the
Pages deploy step to explicitly validate the SHA before writing GITHUB_OUTPUT,
and fail the job with a clear error if it is missing. Use the existing
ghpages_sha step and the SHA lookup command as the place to add the guard so the
release stops instead of deploying a stale appcast.
In `@Orchard/Services/ContainerListService.swift`:
- Around line 455-494: The run-config reconstruction logic here duplicates the
same env/port/volume mapping setup used in recoverContainer and has already
diverged on fields like removeAfterStop and commandOverride. Extract the shared
rebuild into a private helper, referenced from both ContainerListService
methods, so the ContainerRunConfig creation stays consistent and future changes
only need to be made in one place.
In `@Orchard/Services/DNSService.swift`:
- Around line 129-150: The aggregated DNS delete alert in deleteDNSDomains
currently drops the underlying failure detail. Update
DNSService.deleteDNSDomains to track the last encountered error like
ImageService.deleteImages and NetworkService.deleteNetworks, and include
lastError?.localizedDescription in the alertCenter.error message when failures
occur. Keep the existing per-domain loop and counters, but preserve and surface
the most recent error so the batch alert matches the sibling service behavior.
In `@Orchard/Views/Components/SelectionHandler.swift`:
- Around line 24-33: The Cmd-click removal path in SelectionHandler should not
assign lastSelectedId from selectedSet.first because Set ordering is arbitrary
and makes the next range selection unpredictable. Update the selectedSet/remove
logic in the command-click branch so that when clickedId matches lastSelectedId,
the new anchor is chosen deterministically from the remaining items in visual
order using the relevant selection ordering logic in SelectionHandler, rather
than relying on Set iteration order.
In `@Orchard/Views/Features/DNS/MultiDNSCards.swift`:
- Around line 118-131: The delete confirmation flow in this Button action
duplicates the same inline NSAlert logic used elsewhere, so extract it into the
shared confirmation helper already represented by confirmDNSDomainsDeletion in
ListDNS.swift or a common utility/SelectionHandler method, then update
MultiDNSCards to call that helper instead of building the alert locally. Keep
the behavior and button titles identical, and reuse the same shared entry point
in the other duplicate sites like MultiMountCards and MultiNetworkCards to
prevent the implementations from drifting.
🪄 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: 9713e28e-16f3-4bb3-8fa0-831ce0717944
📒 Files selected for processing (19)
.github/workflows/pages.yml.github/workflows/release.ymlOrchard/Services/ContainerListService.swiftOrchard/Services/DNSService.swiftOrchard/Services/ImageService.swiftOrchard/Services/NetworkService.swiftOrchard/Views/Components/SelectionHandler.swiftOrchard/Views/Features/DNS/ListDNS.swiftOrchard/Views/Features/DNS/MultiDNSCards.swiftOrchard/Views/Features/Images/ListImages.swiftOrchard/Views/Features/Images/MultiImageCards.swiftOrchard/Views/Features/Mounts/ListMounts.swiftOrchard/Views/Features/Mounts/MultiMountCards.swiftOrchard/Views/Features/Networks/ListNetworks.swiftOrchard/Views/Features/Networks/MultiNetworkCards.swiftOrchard/Views/Layout/Content.swiftOrchard/Views/Layout/DetailContent.swiftOrchard/Views/Layout/MainInterface.swiftOrchard/Views/Layout/ThreeColumnLayout.swift
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Orchard/Views/Layout/Content.swift (2)
357-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead
if falseplaceholders. This block (and the matching one at Lines 373-375 instartRefreshTimer) is unreachable dead code standing in for the "check for updates" feature. Either wire up the update check or drop these stubs to avoid confusion.Want me to open an issue to track the startup update-check?
🤖 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/Layout/Content.swift` around lines 357 - 360, The startup update-check stub is dead code: the empty if false block in Content.swift and the matching placeholder in startRefreshTimer are unreachable and should be removed or replaced with the real update-check logic. Update the relevant logic in Content to either call the actual update-check flow from the startup path and timer path, or delete both placeholder branches entirely so there are no fake "check for updates" stubs left behind.
292-322: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFragile 0.2s selection delay. The DNS and Network navigation paths schedule selection via
DispatchQueue.main.asyncAfter(deadline: .now() + 0.2), which is a timing guess: on slower renders the selection/focus may not stick, and rapid repeated notifications stack independent delayed writes. Prefer driving selection off an observable state change (e.g. once the domain/network appears in the list) rather than a fixed delay.🤖 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/Layout/Content.swift` around lines 292 - 322, The DNS and Network navigation handlers in Content.swift rely on a fixed 0.2s asyncAfter delay before setting selectedDNSDomain, selectedNetwork, and listFocusedTab, which makes the update timing fragile. Replace the delay-based selection in the NavigateToDNS and NavigateToNetwork Task blocks with state-driven logic that reacts when dnsService.dnsDomains or networkService.networks actually contains the target item, and apply the selection/focus immediately from that observable update path instead of scheduling independent delayed writes.Orchard/Services/DNSService.swift (1)
150-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBulk-delete failures are opaque.
deletedCountis computed but never read, and because the per-domain delete output is discarded (>/dev/null 2>&1) the aggregatedalertCenter.error("Failed to delete \(failedCount) DNS domain(s)")cannot tell the user which domains failed or why. Consider echoing the failing domain (and capturing its stderr) so the message is actionable, and drop the unuseddeletedCount.The batching itself correctly resolves the earlier per-item admin-prompt concern, and passing domains as positional
$@args avoids shell injection.🤖 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/DNSService.swift` around lines 150 - 171, The bulk-delete flow in DNSService should make failures actionable instead of only reporting a count. In the delete path that parses `result.stdout` and calls `alertCenter.error`, stop discarding per-domain output, capture stderr from the underlying delete command, and include the specific failed domain(s) plus the error details in the failure alert. Also remove the unused `deletedCount` variable from this bulk-delete handling block.
🤖 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.
Nitpick comments:
In `@Orchard/Services/DNSService.swift`:
- Around line 150-171: The bulk-delete flow in DNSService should make failures
actionable instead of only reporting a count. In the delete path that parses
`result.stdout` and calls `alertCenter.error`, stop discarding per-domain
output, capture stderr from the underlying delete command, and include the
specific failed domain(s) plus the error details in the failure alert. Also
remove the unused `deletedCount` variable from this bulk-delete handling block.
In `@Orchard/Views/Layout/Content.swift`:
- Around line 357-360: The startup update-check stub is dead code: the empty if
false block in Content.swift and the matching placeholder in startRefreshTimer
are unreachable and should be removed or replaced with the real update-check
logic. Update the relevant logic in Content to either call the actual
update-check flow from the startup path and timer path, or delete both
placeholder branches entirely so there are no fake "check for updates" stubs
left behind.
- Around line 292-322: The DNS and Network navigation handlers in Content.swift
rely on a fixed 0.2s asyncAfter delay before setting selectedDNSDomain,
selectedNetwork, and listFocusedTab, which makes the update timing fragile.
Replace the delay-based selection in the NavigateToDNS and NavigateToNetwork
Task blocks with state-driven logic that reacts when dnsService.dnsDomains or
networkService.networks actually contains the target item, and apply the
selection/focus immediately from that observable update path instead of
scheduling independent delayed writes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 45983ce8-2873-43b6-b2e0-1e382aef56df
📒 Files selected for processing (7)
Orchard/Services/ContainerListService.swiftOrchard/Services/DNSService.swiftOrchard/Views/Features/DNS/ListDNS.swiftOrchard/Views/Features/Images/ListImages.swiftOrchard/Views/Features/Images/MultiImageCards.swiftOrchard/Views/Layout/Content.swiftOrchard/Views/Layout/MainInterface.swift
🚧 Files skipped from review as they are similar to previous changes (5)
- Orchard/Views/Features/Images/ListImages.swift
- Orchard/Views/Layout/MainInterface.swift
- Orchard/Services/ContainerListService.swift
- Orchard/Views/Features/Images/MultiImageCards.swift
- Orchard/Views/Features/DNS/ListDNS.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Views/Layout/Content.swift`:
- Around line 139-149: The selection fallback in the Content.swift onChange
handlers can run before the deferred target is recorded, causing the first
DNS/network item to be selected instead of the intended destination. Update the
load(showLoading:) flow and the related onChange logic around
pendingDNSSelection and pendingNetworkSelection so the pending selection is set
before triggering any load that can fire these observers. Make the same
adjustment in the DNS, network, and any similar selection handlers to ensure the
deferred target is always available before the fallback branch runs.
🪄 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: 3e97f5ea-0d89-46e1-a6d2-294e87781189
📒 Files selected for processing (2)
Orchard/Services/DNSService.swiftOrchard/Views/Layout/Content.swift
|
@andrew-waters, before you merge my PR, could you create a Slack or Discord channel for better communication between the maintainers? |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Orchard/Views/Layout/Content.swift (2)
129-131: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRedundant, divergent mount auto-selection inside the containers handler.
This block runs on
containerListService.containerschanges but has noselectedTab == .mountsguard and never updates the pluralselectedMounts, unlike the dedicatedallMountshandler at Lines 144-154 which does both. SinceallMountsis derived from containers, its ownonChangealready fires and handles this correctly, so this block just eagerly setsselectedMountwhile the user is on another tab (e.g..containersat launch). Recommend removing it and relying on the dedicated handler.♻️ Proposed removal
let existingIds = Set(newContainers.map { $0.configuration.id }) let pruned = selectedContainers.intersection(existingIds) if pruned != selectedContainers { selectedContainers = pruned } - if selectedMount == nil && !containerListService.allMounts.isEmpty { - selectedMount = containerListService.allMounts[0].id - } }🤖 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/Layout/Content.swift` around lines 129 - 131, Remove the redundant selectedMount auto-selection block from the containers-change handler in ContentView. Rely on the dedicated allMounts change handler to perform mount selection, including its selectedTab == .mounts guard and selectedMounts updates.
324-342: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueFragile fixed delay for machine selection/focus.
Setting
selectedMachine/listFocusedTabinsideDispatchQueue.main.asyncAfter(deadline: .now() + 0.2)relies on an arbitrary timing assumption about when the list has rendered. If rendering is slower, focus can silently fail; if faster, the delay is pointless latency. Consider driving focus off the actual list-population state (e.g. via the existingapplyServiceSyncHandlersmachineonChange) rather than a magic delay.🤖 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/Layout/Content.swift` around lines 324 - 342, The NavigateToMachine handler should not use the fixed 0.2-second DispatchQueue delay to set selectedMachine and listFocusedTab. Coordinate these updates with the actual machine-list population state, reusing the existing applyServiceSyncHandlers machine onChange flow so selection and focus occur after the requested machine is available, while preserving the current navigation and validation behavior.
🤖 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.
Nitpick comments:
In `@Orchard/Views/Layout/Content.swift`:
- Around line 129-131: Remove the redundant selectedMount auto-selection block
from the containers-change handler in ContentView. Rely on the dedicated
allMounts change handler to perform mount selection, including its selectedTab
== .mounts guard and selectedMounts updates.
- Around line 324-342: The NavigateToMachine handler should not use the fixed
0.2-second DispatchQueue delay to set selectedMachine and listFocusedTab.
Coordinate these updates with the actual machine-list population state, reusing
the existing applyServiceSyncHandlers machine onChange flow so selection and
focus occur after the requested machine is available, while preserving the
current navigation and validation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 17598822-25f7-48b4-9c5d-13c530e64bae
📒 Files selected for processing (6)
Orchard/Services/ContainerListService.swiftOrchard/Services/NetworkService.swiftOrchard/Views/Layout/Content.swiftOrchard/Views/Layout/DetailContent.swiftOrchard/Views/Layout/MainInterface.swiftOrchard/Views/Layout/ThreeColumnLayout.swift
🚧 Files skipped from review as they are similar to previous changes (5)
- Orchard/Services/NetworkService.swift
- Orchard/Views/Layout/MainInterface.swift
- Orchard/Views/Layout/DetailContent.swift
- Orchard/Services/ContainerListService.swift
- Orchard/Views/Layout/ThreeColumnLayout.swift
- Remove redundant selectedMount auto-selection block from the containers-change handler - Coordinate NavigateToMachine updates with actual machine-list population state using the existing applyServiceSyncHandlers machine onChange flow - Fix missing properties in DetailColumnView
2 participants
This PR implements batch deletion, shift-click range selection, Command+A select-all, and multi-selection detail summary views for containers, images, mounts, DNS domains, and networks. Additionally, it resolves a compiler bottleneck in Content.swift and a Cocoa events warning.