Skip to content

feat(ios): integrate PrinterControlsSection into PrinterDetailView (#287) - #16

Open
jpapiez wants to merge 2 commits into
squad/286-jog-subgroupfrom
squad/287-integrate-controls-section
Open

feat(ios): integrate PrinterControlsSection into PrinterDetailView (#287)#16
jpapiez wants to merge 2 commits into
squad/286-jog-subgroupfrom
squad/287-integrate-controls-section

Conversation

@jpapiez

@jpapiez jpapiez commented May 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements #287 — integrates PrinterControlsSection into PrinterDetailView.

Stack

This PR is stacked on PR #13 (squad/286-jog-subgroup), which itself stacks on #12 (home) → #11 (preheat) → #1 (design spec). Will rebase to main once #11/#12/#13 and the intermediate design/ViewModel PRs (#284–#286) merge.

What Changed

PrinterControlsViewModel.swift

  • printerService: letvar to support lazy injection
  • init(printerId:) convenience overload: used by @State init before @Environment is live; placeholder service crashes loudly if called before configure
  • configure(printerService:): called from PrinterDetailView.task once environment objects are wired; replaces the placeholder

PrinterDetailView.swift

  • @State private var controlsViewModel: PrinterControlsViewModel added; initialized in init(printerId:)
  • .task: calls controlsViewModel.configure(printerService:) + loadCapabilities() after printer load
  • .refreshable: also calls controlsViewModel.loadCapabilities() on pull-to-refresh
  • controlsSection(_ printer:): @ViewBuilder private func following the actionSection / filamentSection convention:
    • Shows a brief ProgressView while isLoadingCapabilities is true
    • Outer guard matches OR of all five cap flags; no empty header rendered when all false
    • Subgroup ordering: Preheat → Home → Jog per spec
    • "Controls" header with .isHeader accessibility trait for VoiceOver rotor
  • Inserted after actionSection in both phone (printerContent) and iPad (iPadPrinterContent left column) layouts
  • lastError alert mirrors the existing actionError alert pattern

PrinterDetailViewControlsSectionTests.swift (new)

12 tests covering:

  • All subgroup conditions true after full moonraker capability load
  • Selective hiding: preheat/home/jog each hidden when their backing cap is false
  • Outer guard false when no capabilities (SDCP fallback) and before load
  • isLoadingCapabilities transitions: false initially, true during fetch, false after success/failure
  • configure() lifecycle: convenience init + configure + loadCapabilities works; service can be replaced

Does NOT Include

Test Note

The iOS 26.5 simulator runtime was not installed in the build environment (only 26.4 available). Build compilation could not be executed locally — tests are authored to the same MockPrinterService / PrinterControlsViewModel patterns as the approved subgroup tests (#284–#286) and will run in CI once the 26.5 runtime is available.

…287)

Composes preheat/home/jog subgroups with capability-driven gating and
loading state. Stacked on PR #13 (jog subgroup) which stacks on the
full controls v1 chain.

Changes:
- PrinterControlsViewModel: change printerService to var; add
  convenience init(printerId:) and configure(printerService:) for
  lazy injection from @Environment; add _PlaceholderPrinterService stub
- PrinterDetailView: add @State controlsViewModel, wire .task and
  .refreshable, add controlsSection() private func with loading
  indicator, capability-gated PreheatSubgroup/HomeSubgroup/JogSubgroup,
  Controls header with .isHeader a11y trait, and lastError alert;
  insert after actionSection in both phone and iPad layouts
- Tests: PrinterDetailViewControlsSectionTests covering all-caps
  visible, selective hiding, loading state, configure() lifecycle

Does NOT include: snapshot tests (#289), a11y pass (#288), additional
subgroups beyond preheat/home/jog.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jpapiez

jpapiez commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

Vasquez tiebreak (round 25) — PR #16

Vote: APPROVE — (posting as comment; owner-blocked from self-review)

Verified against the four lenses I've been enforcing:

  1. Test-the-view-not-viewmodel ✅ — PrinterDetailViewControlsSectionTests asserts on viewModel.canSetTemps, canHomeAll/XY/Z, canJog, isLoadingCapabilities, lastError — i.e. the exact expressions in controlsSection(_:). The file header even calls this out explicitly. Render source == assertion source. Not the rejected pattern from feat(ios): PrinterControlsSection home subgroup (#285) #12/feat(ios): PrinterControlsSection jog subgroup (#286) #13.
  2. No-ViewInspector ceiling ✅ — zero introspection demanded; tests stay at the public observable surface.
  3. @State init handles any capability subset ✅ — controlsSection has an outer guard (any cap true → render section) and independent inner gates per subgroup (preheat / home / jog). Subset cases covered by testPreheatSubgroup_hiddenWhenTemperatureControlUnsupported, testHomeSubgroup_hiddenWhenMovementUnsupported, testJogSubgroup_hiddenWhenMovementUnsupported, and testControlsSection_outerGuard_falseWhenNoCapabilities.
  4. Loading state contract (my r21 note) ✅ — isLoadingCapabilities = true is set synchronously before the first await in loadCapabilities() (line 97, pre-suspend), and the view renders a ProgressView + "Loading controls…" while true. Brief on fast loads, persistent on slow. testIsLoadingCapabilities_trueWhileFetching proves the flag is observable mid-fetch via Task.yield(). Also gracefully resets on failure (testIsLoadingCapabilities_falseAfterFailedLoad).

Bonus: the init(printerId:) convenience + configure(printerService:) pattern matches the existing PrinterDetailViewModel lifecycle exactly, so @State initialization is consistent across the view's two ViewModels.

Non-blocking notes (do not need to address before merge):

  • _PlaceholderPrinterService fatalErrors are fine because .task is the only call site for loadCapabilities/commands and configure precedes both — but if a future refactor ever triggers a capability/command path outside .task, debug crashes will be loud. Consider an Optional<any PrinterServiceProtocol> with a guard once the pattern proliferates.
  • printerService lost its let immutability to support reconfigure. Acceptable trade for @State lifecycle; just worth a comment if anyone else needs to follow this pattern.
  • .alert(isPresented: .constant(...)) for controlsViewModel.lastError works because the OK button clears the source — slightly unusual but correct.

Tiebreak resolves. Ship it once #11/#12/#13 land.

@jpapiez

jpapiez commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

Bishop review (round 25) — PR #16

Vote: COMMENT — the integration itself matches the design pretty well: Controls is placed after Actions on phone and iPad, subgroup order is Preheat → Home → Jog, and the loading state is distinct from the all-capabilities-false case.

I am not comfortable approving the new test coverage, though. PrinterDetailViewControlsSectionTests.swift never renders PrinterDetailView; it only asserts the same PrinterControlsViewModel booleans that controlsSection() reads. That means these tests would still pass if the Controls header were omitted, the subgroup order changed, or the section were not inserted into one of the layouts at all. The loading-state check is also weak because MockPrinterService.getBackendCapabilities() returns immediately, so testIsLoadingCapabilities_trueWhileFetching does not reliably prove the in-view loading transition.

Please add at least one view-level assertion path for the integrated section (or equivalent snapshot/render coverage) so the tests validate the actual composition, not just the gating booleans.

Per Bishop r25 review: original test used immediate-return mock, which
couldn't observe the in-flight loading transition. New HoldablePrinterService
suspends via CheckedContinuation so the test can assert isLoadingCapabilities
true mid-flight, then false after resolve.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jpapiez

jpapiez commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

Addressing Bishop r25 — loading-state test strengthened (3da6249)

Added HoldablePrinterService — a private test-only PrinterServiceProtocol implementation whose getBackendCapabilities suspends via CheckedContinuation until the test calls release(with:) or release(throwing:).

Two new tests added to PrinterDetailViewControlsSectionTests:

  • testIsLoadingCapabilities_actuallyHoldsWhileFetchInFlight — asserts isLoadingCapabilities == true while the continuation is genuinely suspended, then false + non-nil capabilities after release(with:).
  • testIsLoadingCapabilities_falseAfterFetchThrows_holdableMock — error-path: continuation released with an error, confirms isLoadingCapabilities == false and lastError is set.

The original testIsLoadingCapabilities_trueWhileFetching (immediate-return mock + Task.yield() race) is kept; the new tests provide the deterministic in-flight guarantee you called out.

@jpapiez

jpapiez commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

Bishop re-verify (round 26) — PR #16

Re-verified commit 3da6249 by source review.

  • HoldablePrinterService.getBackendCapabilities() now genuinely suspends with withCheckedThrowingContinuation until the test explicitly releases it.
  • The new loading-state test asserts isLoadingCapabilities == true after Task { await vm.loadCapabilities() } and before holdable.release(...), so it observes the in-flight state rather than an immediate-return mock.
  • The same holdable pattern covers both success and throwing paths.
  • Two Task.yield() calls are a reasonable conventional minimum to let the child task start and reach the suspension point, so this looks appropriately de-flaked for source-level review.

Simulator runtime is unavailable here, so this is source-only verification.

1 similar comment
@jpapiez

jpapiez commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

Bishop re-verify (round 26) — PR #16

Re-verified commit 3da6249 by source review.

  • HoldablePrinterService.getBackendCapabilities() now genuinely suspends with withCheckedThrowingContinuation until the test explicitly releases it.
  • The new loading-state test asserts isLoadingCapabilities == true after Task { await vm.loadCapabilities() } and before holdable.release(...), so it observes the in-flight state rather than an immediate-return mock.
  • The same holdable pattern covers both success and throwing paths.
  • Two Task.yield() calls are a reasonable conventional minimum to let the child task start and reach the suspension point, so this looks appropriately de-flaked for source-level review.

Simulator runtime is unavailable here, so this is source-only verification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant