Skip to content

🐛 fix: PasturaBackButton のルーター読み出しを optional 化 (#1683) - #1684

Merged
tyabu12 merged 3 commits into
mainfrom
fix/back-button-optional-router
Sep 6, 2026
Merged

🐛 fix: PasturaBackButton のルーター読み出しを optional 化 (#1683)#1684
tyabu12 merged 3 commits into
mainfrom
fix/back-button-optional-router

Conversation

@tyabu12

@tyabu12 tyabu12 commented Sep 6, 2026

Copy link
Copy Markdown
Owner

Summary

Reads PasturaBackButton's router as an optional @Environment(AppRouter.self) so a missing router degrades instead of terminating the app.

A TestFlight 1.3 (888) crash on iOS 26.6 (iPhone17,5, reported during the ADR-023 S5-4 soak) hit the SwiftUICore EnvironmentValues.subscript assertion — the "No Observable object of type X found" trap — from UIKitBarItemHost.initializeSize() via BarAppearanceBridge.didMoveToWindowNavigationButtonBar.ItemLayout.updateItemView, inside the NavigationStack push of SimulationView. iOS 26 sizes the bar-item host synchronously during the transition, so the item's separate view graph can read the environment before TabNavigationStack's .environment(router) reaches it, and the non-optional read traps rather than yielding nil. The only Pastura frames were main; there were no PasturaSharedEngine frames and the crash preceded .task, so the Kotlin engine behind the S5-4 toggle is not implicated.

PasturaBackButton is the only toolbar item in the app that reads an Observable from the environment (all 42 ToolbarItem sites checked), so this one read covers the crash shape app-wide.

The nil arm logs a fixed string and calls dismiss() rather than returning silently. Five of the six callsites rely on the default pop and .navigationBarBackButtonHidden(true) removes the system button, so a quiet nil would trade a crash for a screen with no visible way out and nothing in the log to say why. The dismiss() recovery is documented as best-effort: in the very failure mode described, an incompletely propagated environment can hand back the default no-op DismissAction, so the dependable gain is the absence of a trap plus an observable line.

Set aside, with the rationale recorded in the commit body: sharedBackgroundVisibility(.hidden) (via hidingPasturaSharedBackground()) sits on all six back-button toolbar items and is adjacent to BarAppearanceBridge in the stack. It is an appearance modifier and does not change environment propagation; even if it is the trigger, the proximate cause is the non-optional read, and tolerating a missing value removes the crash without giving the Liquid Glass capsule back. Reverting that opt-out is not warranted by one non-reproducible report.

Test plan

  • scripts/xcodebuild.sh test — full suite green: 3724 unit tests in 303 suites, plus 10 UI tests.
  • swiftlint lint --quiet --strict — clean.
  • No new unit test. A body-level environment read is not reachable from pure logic and ADR-009 rules out render tests; the nil arm additionally needs a real push transition with a partially-propagated environment, which no UI test can stage deterministically. The reviewer agreed with this call. ScreenshotTourTests already exercises the non-nil router.pop() path, so the regression that is testable (breaking the normal tap) stays guarded, and the error-level log is the substitute instrumentation for the arm that is not.

Review

Reviewer model: Opus. Plan critique: claude-kit:critic (Opus) — no Critical, four Warnings, all folded in before implementation (the logged dismiss() fallback instead of a silent nil arm, the doc-comment and rule updates, and the recorded rationale for setting aside the sharedBackgroundVisibility hypothesis).

Code review verdict: PASS, 0 Critical / 0 Warning / 3 Suggestions. All three applied in 243b01e4:

  1. The doc-comment over-trusted dismiss() as a guaranteed pop — softened to best-effort with the reason.
  2. navigation.md stated the incident, not the rule — generalized to "a router read from inside a ToolbarItem must be optional; body-level reads stay non-optional", so a future router-reading toolbar component trips the rule instead of repeating 🐛 fix: PasturaBackButton の非 optional な環境読み出しが push 遷移中にクラッシュしうる #1683. The six body-level reads keep failing loudly on a genuine wiring mistake.
  3. Added navigation-qa.md scenario 19 as a triage recipe: the Console filter (subsystem:app.pastura.Pastura category:PasturaBackButton) that finds the nil arm on a real device, since neither CI nor the suite can observe it.

Nothing rejected or left unfixed.

Device QA

Required — the crash is a real-device, real-push-transition failure that the simulator did not surface.

  • On each of the six PasturaBackButton screens (ScenarioDetailView, ResultsView, ResultDetailView, GalleryScenarioDetailView, ScenarioEditorView, SimulationView), confirm the back chevron pops exactly one screen, by tap and by edge swipe-back.
  • On SimulationView, confirm the back tap still routes through the confirm-on-leave dialog mid-run (the action: override path, ✨ feat: Home P3 PR2 — confirm-on-leave pause for in-flight runs #673).
  • Confirm the chevron still renders flat on iOS 26 with no Liquid Glass capsule (the hidingPasturaSharedBackground() opt-out is untouched, so this is a no-change check).
  • During the next TestFlight cycle, filter the device log on subsystem:app.pastura.Pastura category:PasturaBackButton. A line reading no AppRouter in environment; dismissing means the nil arm was taken on a real device and belongs on 🐛 fix: PasturaBackButton の非 optional な環境読み出しが push 遷移中にクラッシュしうる #1683.

Closes #1683
Part of #501

🤖 Generated with Claude Code

https://claude.ai/code/session_01RGxx55Kkb7f18Z4FzSwhXV

tyabu12 and others added 2 commits September 6, 2026 21:22
A TestFlight 1.3 (888) crash on iOS 26.6 terminated the app inside the
NavigationStack push of SimulationView: the SwiftUICore
`EnvironmentValues.subscript` assertion, reached from
`UIKitBarItemHost.initializeSize()` via `BarAppearanceBridge.didMoveToWindow`
→ `NavigationButtonBar.ItemLayout.updateItemView`. iOS 26 sizes the bar-item
host synchronously inside the transition, so the item's separate view graph
can read the environment before `TabNavigationStack`'s `.environment(router)`
reaches it, and the non-optional `@Environment(AppRouter.self)` traps rather
than yielding nil. `PasturaBackButton` is the only toolbar item in the app
that reads an `Observable` from the environment (all 42 `ToolbarItem` sites
checked), so this one read covers the crash shape app-wide.

The nil arm logs a fixed string and calls `dismiss()` instead of returning
silently: five of the six callsites rely on the default pop, and
`.navigationBarBackButtonHidden(true)` removes the system button, so a quiet
nil would trade a crash for a screen with no visible way out. The log line is
also the only way to learn in production whether the nil path is ever taken —
the crash never reproduced, ADR-009 rules out render tests, and the failure
needs a real push transition, so neither CI nor the suite can observe it.

Considered and set aside: `sharedBackgroundVisibility(.hidden)` (via
`hidingPasturaSharedBackground()`) sits on all six back-button toolbar items
and is adjacent to `BarAppearanceBridge` in the stack. It is an appearance
modifier and does not change environment propagation; even if it is the
trigger, the proximate cause is the non-optional read, and tolerating a
missing value removes the crash without giving the Liquid Glass capsule back.
Reverting that opt-out is not warranted by one non-reproducible report.

Found during the ADR-023 S5-4 soak cycle.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RGxx55Kkb7f18Z4FzSwhXV
Applies the three review suggestions on the fix commit. Documentation
only — no behaviour change.

The doc-comment claimed `dismiss()` "pops a NavigationStack destination
the same way". In the exact failure mode it describes, the environment is
incompletely propagated, so `dismiss()` can resolve to the default no-op
`DismissAction`. What the nil arm reliably buys is the absence of a trap
plus a log line; the recovery is a bonus when the environment is merely
routerless. Stated that way now, so the next editor does not over-trust it.

`navigation.md` gains the general form of the rule: a router read from
inside a `ToolbarItem` must be optional, a body-level read stays
non-optional. The incident note alone would not stop a future
router-reading toolbar component from repeating #1683, and the six
body-level reads should keep failing loudly on a genuine wiring mistake.

`navigation-qa.md` gains scenario 19 as a triage recipe rather than a
walkthrough — the crash cannot be staged on demand, so what a tester can
act on is the Console filter that finds the nil arm on a real device.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RGxx55Kkb7f18Z4FzSwhXV
@tyabu12 tyabu12 added the bug Something isn't working label Sep 6, 2026
@tyabu12 tyabu12 self-assigned this Sep 6, 2026
)

The crash narrative and the "keep the ? " guard already live in
PasturaBackButton.swift and docs/qa/navigation-qa.md; navigation.md loads on
every Views/App read, so keep only the generalized ToolbarItem rule there.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ByMsx9msPnR9beMTrPcXvH
@tyabu12
tyabu12 enabled auto-merge (squash) September 6, 2026 13:08
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown

CI Report

Lint

SwiftLint: passed

Release Build (ADR-005 §8 guard)

Release-iphoneos symbol guard: passed

Demo Replay Drift Guard

Demo replay drift guard: passed

Test Results

  • Unit: > Could not parse test output.
  • UI: > Could not parse test output.

Coverage

74.66% line coverage


View full CI run

@tyabu12
tyabu12 merged commit 1d41e77 into main Sep 6, 2026
21 checks passed
@tyabu12
tyabu12 deleted the fix/back-button-optional-router branch September 6, 2026 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐛 fix: PasturaBackButton の非 optional な環境読み出しが push 遷移中にクラッシュしうる

1 participant