Skip to content

fix: bell mode regression, plus adversarial review and dark-mode follow-ups - #138

Merged
mmazzarolo merged 4 commits into
masterfrom
claude/overnight-improvements
Aug 3, 2026
Merged

fix: bell mode regression, plus adversarial review and dark-mode follow-ups#138
mmazzarolo merged 4 commits into
masterfrom
claude/overnight-improvements

Conversation

@mmazzarolo

Copy link
Copy Markdown
Owner

Follow-up to #137, which merged while three adversarial reviewers were still running. They found two problems in that merge, so master currently carries a user-facing regression. This branch fixes it.

bun run validate:static passes (81 tests). The full Maestro suite passes on both platforms against Release builds — Android with R8, iOS re-run after the startup change. The iOS dark-mode fix was verified on device by reproducing the exact failing configuration.

The regression in master

In origin/master today:

bell: { breatheIn: cueBell1, breatheOut: cueBell2, hold: cueBell1 }

buildStepsMetadata gives the audio id hold to both the step after the inhale and the step after the exhale, so any bell assigned to it sounds twice per cycle:

Pattern Before #137 On master now
Square (the default) bell1 bell2 bell1 bell2 bell1 bell1 bell2 bell1
Pranayama, custom default alternating three identical bells across the loop
Awake, Ujjayi four identical fixed

#137 fixed the patterns with no holds and broke the ones with holds — including the default — and added a test asserting the broken mapping.

Bell mode now cues only the two direction changes and leaves the holds silent, so every pattern alternates. Cue players are built independently rather than all-or-nothing, which is what lets a mode cue some steps and not others. The test walks a full Square cycle instead of checking the asset map.

Native views ignored the chosen theme

Reported from a screenshot: with "Use system theme" off and Dark chosen on a light phone, the iOS large title rendered near-black on the app's dark background, and the picker wheel was drawn in light-mode chrome.

Both are UIKit, and UIKit follows the system appearance. Rather than colour each native view by hand, the app now overrides its own appearance with Appearance.setColorScheme, so every native view follows the chosen theme — including any added later. "unspecified" hands control back when the user is following the system.

Worth flagging for reviewers: colors.test.ts asserts the contrast of colours React Native draws and is structurally blind to anything UIKit paints. Three reviewers and I all missed this; it took looking at the screen.

{ stopTogether: false } was doing no work, and cost the loop its failure signal

The reviewer rebuilt the composition without the flag and ran every reachable step length — all reach their target and report finished. The 0.35 ratio alone fixes the short-step bug.

What the flag added was a silent failure: an interrupted circle reported finished: true, so loopAnimations would step on forever with a frozen circle instead of stopping. Removed. The 25 exercise tests still pass, which is the proof it was redundant.

iOS audio interruptions

The answer to "does pausing only on background mean the app talks over a phone call?" is no — expo-audio deactivates the session. But two real consequences fell out:

  • during an interruption the exercise, visuals and haptics keep running with no audio at all
  • afterwards expo-audio replays the cue it interrupted, so a fragment of "Breathe in" can land mid-exhale — and the same applies on backgrounding, because the JS AppState event arrives after expo-audio's native observers, making the existing stopExerciseAudio() too late

Audio now also stops on the return to active, which covers both. Still uncovered: an interruption that begins and ends without an AppState transition. expo-audio exposes no JS event for it, so that is a known gap rather than an oversight.

Smaller review findings

  • customPatternDurationLimits is a four-tuple. normalizePersistedSettingsState maps over it, so its length is what makes the as CustomPatternSteps cast true — as a plain array it was simply untrue, and a fifth entry would have produced a five-element result still typed as four.
  • The settings read retried in the same tick, so it could only survive a bridge hiccup, not the locked database its own comment claimed. It pauses first now.
  • animate() coalesces its easing, so an explicit undefined no longer falls through to React Native's default curve.
  • The accessibility hooks ask at module load rather than first mount, and a change event landing mid-query is no longer overwritten by the stale answer — which, cached at module scope, used to poison every later mount in the process.
  • The paused screen announces itself and carries a header role. It was silent to screen readers while the completion screen already announced.
  • useKeepAwake no longer covers the completion screen, which never dismisses itself and held the display on until the user came back.
  • The iOS settings secondary text used raw slate-500 — 3.07:1 on the dark card, the longest body text in the app.
  • hideBottomBorderhideBottomBorderWeb: only web honours it, and the platform-neutral name promised parity a TypeScript contract cannot enforce.

Known gaps, deliberately not fixed

  • Android's stepper buttons still have no accessible name. The current @expo/ui API cannot express it: semantics takes only contentType, FilledTonalButton exposes no content description, and Icon needs a vector drawable. Documented at the call site.
  • dark.image does nothing on Android — the plugin only writes a dark drawable when drawable.darkIcon is set. No visible fault, because that splash icon is transparent and the dark colour does apply.
  • splash-dark.png ships ~28 KB into the web bundle, where SplashScreenManager returns early and never renders it.

mmazzarolo and others added 4 commits August 3, 2026 12:38
`customPatternDurationLimits` is now a four-tuple. `normalizePersistedSettingsState`
maps over it to build the steps, so its length is what makes the
`as CustomPatternSteps` cast true. As a plain array it did not: adding a fifth
entry produced a five-element result still typed as a four-tuple, and
`buildStepsMetadata` would then read `undefined` for a duration and let
`Animated.timing` fall back to its own default. Type-level only, no runtime change.

The storage read retried in the same tick, so it could only survive a bridge
hiccup — not the locked database its own comment claimed. It now pauses first.

`animate()` coalesces the easing instead of assigning it above the spread. A
caller passing an explicit `undefined` used to fall through to React Native's
`Easing.inOut(Easing.ease)` rather than the app's quad curve. No caller does that
today; this keeps the default true regardless.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ettings text

Two findings from an adversarial review.

The bell remap was a net regression. `buildStepsMetadata` gives the audio id
`hold` to both the step after the inhale and the step after the exhale, so the
bell assigned to it sounded twice per cycle. On Square — the default pattern —
the sequence became bell1, bell1, bell2, bell1, which puts three identical bells
in a row across the loop boundary. It fixed the patterns with no holds and broke
the ones with holds, including the default, and the test asserted the broken
behaviour.

Bell mode now cues only the two direction changes and leaves the holds silent,
so every pattern alternates. Each cue player is built independently instead of
all-or-nothing, which is what lets a mode cue some steps and not others. Tests
cover a full Square cycle rather than only the asset mapping.

The iOS settings screen still read `slate-500` directly, so its secondary text
measured 3.07:1 on the dark card — worse than anything the earlier contrast
commit fixed, and it is the longest body text in the app. It now reads
`useThemeColors()` like everything else.

`hideBottomBorder` becomes `hideBottomBorderWeb`: only the web implementation
honours it, and the old platform-neutral name promised parity that a TypeScript
contract cannot enforce. The parity test comment claimed the contract could
require an accessibility label; it cannot, and now says so.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`{ stopTogether: false }` was doing no work. The 0.35 ratio already keeps both
label fades inside the step, so nothing interrupts the circle — verified across
every reachable step length. What the flag did add was a silent failure: an
interrupted circle reported `finished: true`, so `loopAnimations` would step on
forever with a frozen circle instead of stopping. A loud failure is the right one
in the path that drives the whole exercise.

Audio now also stops when the app returns to `active`. `expo-audio` pauses the
players it interrupts and replays them afterwards, and the JS `AppState` event
arrives after its native observers have run — so a cue caught mid-word finished
in the middle of the wrong step, seconds or minutes later. This is the cost of
pausing only on a real background, and it is now paid deliberately. An iOS
interruption that begins and ends without an `AppState` transition is still
uncovered; expo-audio exposes no JS event for it.

The accessibility hooks ask their question at module load instead of on first
mount, so the answer is in flight before anything renders, and a change event
that lands while the query is outstanding is no longer overwritten by the stale
answer — which, because the value is cached at module scope, used to poison every
later mount in the process.

The paused screen announces itself and carries a header role. The step
announcements just stopped, so a screen-reader user was told nothing, while the
completion screen already announced.

`useKeepAwake` no longer covers the completion screen, which never dismisses
itself and would hold the display on until the user came back to the phone.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ance

The iOS large title and the picker wheel are UIKit, and UIKit takes its colours
from the system appearance. A user who turned "Use system theme" off and chose
Dark on a light phone got a near-black title on the app's dark background, and a
picker drawn in light-mode chrome — both barely legible.

Override the app's own appearance instead of colouring each native view by hand:
every native view then follows the chosen theme, including any added later.
"unspecified" hands control back when the user is following the system, so that
path is unchanged. React Native's own documentation confirms this affects the
app only and never the system UI.

Worth noting for anyone reading `colors.test.ts`: it asserts the contrast of
colours React Native draws, and can say nothing about what UIKit paints. This
class of bug is invisible to it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mmazzarolo
mmazzarolo merged commit 4107588 into master Aug 3, 2026
1 check passed
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