Link container tool in error alert#70
Conversation
📝 WalkthroughWalkthrough
ChangesContainer startup alert
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SystemService
participant AlertCenter
participant ContentView
participant openURL
SystemService->>AlertCenter: report missing Container binary with download button
AlertCenter->>ContentView: publish alert with extraButtons
ContentView->>openURL: open supplied download URL
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Orchard/Services/AlertCenter.swift (2)
3-8: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a collision-proof identifier for alert buttons.
Concatenating
textand the URL does not guarantee uniqueness; two identical buttons receive the same ID, which can makeForEachtreat distinct actions as the same view. Use a storedUUIDor another guaranteed-unique key. (developer.apple.com)🤖 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/AlertCenter.swift` around lines 3 - 8, Update AlertButton’s Identifiable implementation to use a stored UUID or another guaranteed-unique key instead of deriving id by concatenating text and url. Preserve the existing text and url properties while ensuring distinct button instances always receive distinct identifiers for ForEach.Source: MCP tools
3-5: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDefine behaviour for URL-less buttons.
urlis optional, butContentViewrenders every button and silently does nothing when it isnil. Make the URL required for this contract or add explicit non-URL action semantics to avoid clickable no-op buttons.🤖 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/AlertCenter.swift` around lines 3 - 5, Update AlertButton and the ContentView button-handling flow to define behavior when url is nil: either make AlertButton.url non-optional and require every button to provide a destination, or add an explicit action representation and handle it in ContentView. Ensure no rendered button remains a clickable no-op.
🤖 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/AlertCenter.swift`:
- Around line 3-8: Update AlertButton’s Identifiable implementation to use a
stored UUID or another guaranteed-unique key instead of deriving id by
concatenating text and url. Preserve the existing text and url properties while
ensuring distinct button instances always receive distinct identifiers for
ForEach.
- Around line 3-5: Update AlertButton and the ContentView button-handling flow
to define behavior when url is nil: either make AlertButton.url non-optional and
require every button to provide a destination, or add an explicit action
representation and handle it in ContentView. Ensure no rendered button remains a
clickable no-op.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 828c2460-a4a3-4893-87b6-ded7136fc4ce
📒 Files selected for processing (4)
CHANGELOG.mdOrchard/Services/AlertCenter.swiftOrchard/Services/SystemService.swiftOrchard/Views/Layout/Content.swift
It's technically correct that the ID isn't collision resistant, but multiple buttons with identical text and link effectively are the same button for SwiftUI identity purposes (it would log warnings, but this would be a programming area so that's probably helpful…) RE buttons without a link: They'll dismiss the alert. A future extension might be to pass in a closure, but that seems out-of-scope for this change. |
|
Thanks for this @dwrss 👍🏼 |
What does this PR do?
If the
containerbinary cannot be found, this adds a "Download Apple Container" button to the error alert. It is clear you need this in the README, but I can't be the only one who thought they had it installed 😅.Related issues
None
Screenshots / recording
Checklist
Orchardscheme)⌘U/xcodebuild test)CHANGELOG.md(Added / Changed / Fixed)