Polish image pull flow and image size display#49
Open
aapw01 wants to merge 2 commits into
Open
Conversation
Owner
|
Hi @aapw01 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? |
- Add pull-by-reference input + Docker Hub search pagination (sentinel onAppear with atomic check-and-set guard), dismissable pull progress banner moved to the Images list (no longer trapped inside the search sheet). - Fix image size in the list and detail Overview: previously showed the OCI manifest descriptor bytes (~hundreds of B). Now shows the real layer size from inspectImage(), picking the host-matching variant. Host arch resolved at runtime via sysctl.proc_translated so Rosetta slices on Apple Silicon report arm64 instead of amd64. - Add Run Container "+" button on the Containers tab, reusing RunContainerView in picker mode with a searchable local-image dropdown that also accepts pasted references. - Fix collapsed sidebar layout by binding NavigationSplitViewVisibility on both layout branches and pushing the search header below the traffic lights when the sidebar is collapsed. - Misc correctness from review: retry .failed image inspects on refresh, canonicalize references at pullImage entry so by-reference pull aligns with search-result pull progress keys, single source of truth for the image picker field, isAlreadyPulled handles digest pins, derivedName follows OCI grammar (digest → last path segment → tag), Enter / Search button bypass typing debounce, pull progress rows sorted by image name for stable visual order. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aapw01
force-pushed
the
pull-flow-and-ui-polish
branch
from
July 8, 2026 14:41
57ab4d4 to
e29386b
Compare
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds host-aware image size tracking and canonical Docker Hub reference handling in ChangesImage size, canonicalisation, search, and run-container UX
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ImageService
participant InspectGate
participant Backend
ImageService->>ImageService: resolve host architecture
ImageService->>ImageService: canonicalImageReference(_:)
ImageService->>InspectGate: gate inspection work
ImageService->>Backend: inspect image data
Backend-->>ImageService: image variants
sequenceDiagram
participant User
participant ThreeColumnLayout
participant RunContainerView
participant ImageService
participant ContainerListService
User->>ThreeColumnLayout: tap Run Container
ThreeColumnLayout->>RunContainerView: present sheet
RunContainerView->>ImageService: load local images
User->>RunContainerView: choose image and run
RunContainerView->>ContainerListService: runContainer(config:)
ContainerListService-->>RunContainerView: complete and dismiss
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Orchard/Services/ImageService.swift (1)
195-220: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNew
dismissPullProgresscan race with the existing auto-removal timer, hiding a fresh pull's progress.
pull()'s completion schedulesDispatchQueue.main.asyncAfter(+3s) { self.pullProgress.removeValue(forKey: cleanImageName) }, keyed only bycleanImageName. If a user dismisses a completed row and re-pulls the same image within that 3s window, the stale timer will fire later and remove the entry for the new pull — even if it's still.pullingor freshly.completed— making the progress banner vanish mid-flight until the next status write.Guard the scheduled removal so it only clears an entry that is still in the state it scheduled for, e.g.:
🩹 Proposed fix
Task { await self.load() } - DispatchQueue.main.asyncAfter(deadline: .now() + 3) { - self.pullProgress.removeValue(forKey: cleanImageName) - } + DispatchQueue.main.asyncAfter(deadline: .now() + 3) { + if case .completed = self.pullProgress[cleanImageName]?.status { + self.pullProgress.removeValue(forKey: cleanImageName) + } + }🤖 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/ImageService.swift` around lines 195 - 220, The delayed cleanup in pull(_:), which removes pullProgress[cleanImageName] after completion, can clear a newer pull for the same image if the row was dismissed and re-pulled within the delay. Update the asyncAfter removal logic so it only deletes the entry when it still matches the completion state that scheduled it, rather than blindly removing by cleanImageName. Use the existing pull(_:), pullProgress, and ImagePullProgress status values to verify the current row hasn’t been replaced by a newer .pulling or .completed entry before removing it.
🧹 Nitpick comments (3)
OrchardTests/ImageServiceTests.swift (1)
78-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a
host:port/repo(non-localhost) and digest-reference case.Current cases cover bare/tagged/namespaced Docker Hub names and an explicit-registry name, but not a generic
registry:5000/repo(colon-triggered registry detection) or an@sha256:digest form — both are distinct branches incanonicalImageReference.🤖 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/ImageServiceTests.swift` around lines 78 - 85, The `imageReferenceCanonicalization` test in `ImageServiceTests` should cover the missing branches in `canonicalImageReference`: add an assertion for a non-localhost `host:port/repo` reference so colon-based registry detection is exercised, and add an assertion for an `@sha256:` digest reference to verify digest handling. Keep the existing coverage for Docker Hub defaults and explicit registries, and extend this test function with the new cases so the canonicalization behavior is fully covered.Orchard/Services/ImageService.swift (2)
57-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider cancellation-awareness for
InspectGate.Accounting is correct for the happy path (enter/leave pairs balance via FIFO waiter resumption), but
withCheckedContinuationwon't observe task cancellation — a cancelled caller waiting inwaitersstays queued and is resumed/counted like any other, wasting a concurrency slot on stale work. Not currently exploitable since callers here are never explicitly cancelled, but worth hardening if that 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 `@Orchard/Services/ImageService.swift` around lines 57 - 79, Make InspectGate cancellation-aware by preventing cancelled tasks from remaining queued in waiters. Update the InspectGate.actor enter() / leave() flow so a task waiting in withCheckedContinuation can detect cancellation, remove its continuation if cancelled, and avoid consuming an inflight slot when resumed. Keep the FIFO behavior and adjust the accounting in enter, leave, and the waiters storage to handle cancelled waiters safely.
123-130: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFailed image-size lookups are retried unboundedly, every 5s poll.
.failedentries are evicted here soenrichImageSizesretries them on everyload()call. For a permanently-unresolvable image (e.g. malformed manifest), this hammersbackend.inspectImageindefinitely rather than backing off, contrary to the stated goal of "retry behaviour for transient failures."Consider tracking a retry count/timestamp per reference and applying simple backoff (e.g. stop retrying after N attempts, or only retry every Nth poll) instead of unconditionally clearing
.failedstate each cycle.🤖 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/ImageService.swift` around lines 123 - 130, The retry handling in ImageService’s load/enrichImageSizes flow is evicting .failed entries from imageSizes on every poll, which causes backend.inspectImage to be retried forever for permanently broken images. Update the image-size caching logic around imageSizes.filter and enrichImageSizes(for:) to keep failure state per reference and apply backoff, such as recording a retry count or last-attempt timestamp and only retrying after a delay or after N attempts. Make the decision based on the existing reference keys and the status tracked in ImageService so transient failures can recover without unbounded polling.
🤖 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/ImageService.swift`:
- Around line 222-252: The search pagination flow in ImageService’s
fetchSearchPage can append stale results after a new search starts. Add a fresh
guard in fetchSearchPage right before mutating searchResults, comparing the
captured query against lastSearchQuery and ignoring the response if they differ.
Keep the existing search(_:) and loadMoreSearchResults() behavior, but ensure
only the currently active query can update searchResults, searchResultsPage, and
searchResultsHasMore.
- Around line 160-189: The `enrichImageSizes(for:)` task can block forever while
waiting on `backend.inspectImage(reference:)`, leaving `imageSizes[ref]` stuck
in `.loading` and holding an `inspectGate` slot. Add a timeout/cancellation
guard around the `inspectImage` await inside `ImageService.enrichImageSizes`,
and treat timeout as a failure so the status is updated from `.loading` to
`.failed`. Use the existing `inspectGate`, `backend.inspectImage(reference:)`,
and `imageSizes` flow to keep the retry/state handling consistent.
- Around line 254-299: The search URL construction in fetchSearchPage is
vulnerable because addingPercentEncoding(withAllowedCharacters:) can leave
reserved delimiters like & and = unescaped. Replace the manual URL string
interpolation in fetchSearchPage(query:page:append:) with URLComponents and
URLQueryItem so the query value is encoded safely before creating the URL. Keep
the existing URLSession.shared.data(from:) flow and error handling unchanged.
In `@Orchard/Views/Features/Containers/RunContainer.swift`:
- Around line 151-155: The image change handler in RunContainerView only derives
config.name once when it is empty, so the auto-generated name stops following
subsequent edits. Update the .onChange(of: config.image) logic to track whether
config.name is still the last derived value from
RunContainerView.derivedName(from:) and only overwrite it when it has not been
manually changed, then keep calling validateContainerName() after syncing. Use
the existing config.name, config.image, and RunContainerView.derivedName(from:)
symbols to implement the auto-name tracking cleanly.
In `@Orchard/Views/Features/Images/ImageSearch.swift`:
- Around line 273-275: The isPulling computed property in ImageSearch should
only return true for active in-progress pulls, not for any non-nil pullProgress
entry. Update the logic in ImageSearch.isPulling to check the pull state for
result.name and treat only the active/running case as pulling, so .completed and
.failed no longer keep the spinner visible and retry remains available.
- Around line 410-421: The dismiss button is currently skipped for rows in the
.pulling state because the ProgressView branch is mutually exclusive with the
onDismiss action. Update the conditional logic in the row view in
ImageSearch.swift so that the dismiss control can still render when
progress.status is .pulling and onDismiss is provided, while preserving the
progress indicator. Use the existing progress.status check and onDismiss
callback as the key symbols to adjust the branching.
---
Outside diff comments:
In `@Orchard/Services/ImageService.swift`:
- Around line 195-220: The delayed cleanup in pull(_:), which removes
pullProgress[cleanImageName] after completion, can clear a newer pull for the
same image if the row was dismissed and re-pulled within the delay. Update the
asyncAfter removal logic so it only deletes the entry when it still matches the
completion state that scheduled it, rather than blindly removing by
cleanImageName. Use the existing pull(_:), pullProgress, and ImagePullProgress
status values to verify the current row hasn’t been replaced by a newer .pulling
or .completed entry before removing it.
---
Nitpick comments:
In `@Orchard/Services/ImageService.swift`:
- Around line 57-79: Make InspectGate cancellation-aware by preventing cancelled
tasks from remaining queued in waiters. Update the InspectGate.actor enter() /
leave() flow so a task waiting in withCheckedContinuation can detect
cancellation, remove its continuation if cancelled, and avoid consuming an
inflight slot when resumed. Keep the FIFO behavior and adjust the accounting in
enter, leave, and the waiters storage to handle cancelled waiters safely.
- Around line 123-130: The retry handling in ImageService’s
load/enrichImageSizes flow is evicting .failed entries from imageSizes on every
poll, which causes backend.inspectImage to be retried forever for permanently
broken images. Update the image-size caching logic around imageSizes.filter and
enrichImageSizes(for:) to keep failure state per reference and apply backoff,
such as recording a retry count or last-attempt timestamp and only retrying
after a delay or after N attempts. Make the decision based on the existing
reference keys and the status tracked in ImageService so transient failures can
recover without unbounded polling.
In `@OrchardTests/ImageServiceTests.swift`:
- Around line 78-85: The `imageReferenceCanonicalization` test in
`ImageServiceTests` should cover the missing branches in
`canonicalImageReference`: add an assertion for a non-localhost `host:port/repo`
reference so colon-based registry detection is exercised, and add an assertion
for an `@sha256:` digest reference to verify digest handling. Keep the existing
coverage for Docker Hub defaults and explicit registries, and extend this test
function with the new cases so the canonicalization behavior is fully covered.
🪄 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: 6effb378-c6e2-4c30-b831-fea21360786a
📒 Files selected for processing (8)
Orchard/Models.swiftOrchard/Services/ImageService.swiftOrchard/Views/Features/Containers/ContainerDetail.swiftOrchard/Views/Features/Containers/RunContainer.swiftOrchard/Views/Features/Images/ImageSearch.swiftOrchard/Views/Features/Images/ListImages.swiftOrchard/Views/Layout/ThreeColumnLayout.swiftOrchardTests/ImageServiceTests.swift
Author
done |
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
2 participants
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.
Pull flow:
refs at pullImage entry so by-reference and search-result pulls share
pullProgress keys.
check-and-set guard against duplicate page fetches.
dismissable, honest indeterminate spinner (was a stuck linear bar
bound to a value that never updates), truncated overflow messages,
rows sorted by image name for stable order.
boundary matching (was producing false positives like nginx vs
nginx-unprivileged); matched rows in search show an inert
"Already pulled" instead of a misplaced Run button.
Image size:
size (~hundreds of B). Now shows the real layer size via
inspectImage, picking the host-matching variant. Host arch resolved
at runtime via sysctl.proc_translated so Rosetta slices on Apple
Silicon report arm64 instead of amd64.
populated through a max-4-concurrent InspectGate actor; .failed
entries cleared on next refresh so transient failures recover.
Run Container on Containers tab:
TextField is the single source of truth for the picker field.
segment → strip :tag), producing valid names for ghcr.io and
digest-pinned refs.
Layout:
invisible in light mode — switched to .primary.
sidebar collapse state persists across tab changes.
collapsed.
Misc: