Stop one mouse button from driving both navigation directions

[anibalribeiro]

The bug

One mouse button could end up driving both navigation directions, and which one fired was decided by the process's hash seed — so the same mapping navigated back in one launch and forward in the next.

AppSettings resolved each direction's stored button independently. Neither call could see what the other direction already occupied, so when a stored value this build reserves fell back to its own direction's default, it could land on the button the other direction was already using:

// before
var backButton: Int    { button(Key.backButton,    swappedTo: NavigationDirection.forward.defaultButton) }
var forwardButton: Int { button(Key.forwardButton, swappedTo: NavigationDirection.back.defaultButton) }

NavigationController then picked between the two with buttons.first { $0.value == buttonNumber }?.key. Dictionary iteration order depends on Swift's per-process hash seed, so the winner changed from launch to launch. I confirmed that empirically rather than assuming it: a binary that resolves a doubly-mapped button, run 12 times, answered back 5 times and forward 7 times.

Reachable states

I set out to fix the case where an older build stored the middle button for one direction while the other was mapped by hand. Writing an exhaustive test showed the problem is wider than that — these all collided:

• Legacy swapSideButtons install plus any unusable stored value. The flag makes an unmapped direction prefer the opposite default, which is exactly what the other direction's fallback lands on.
• Forward mapped to button 3 while back was never mapped. Back's default is 3.
• Both directions storing the same button. Not writable through the UI, since assign trades places, but a hand-edited defaults domain can produce it.

The fix

A new SideButtons library target resolves both directions in a single pass, following the SwipeGesturePoster and ScrollEngine precedent of keeping pure logic in a testable target. A direction the user explicitly mapped is always honored; a direction that needs a fallback takes its historical default, or the other default when that's taken, with the assignable range as a backstop that cannot fail to find a free button. Two stored values that match resolve in favor of back, so the outcome no longer depends on read order.

AppSettings now reads through it, and direction(for:) checks the two directions in a fixed order. That second change is defense in depth: resolve rules out the collision, so the order only matters if that guarantee is ever broken.

Behavior for anyone whose mapping was already valid is unchanged, including the legacy swapSideButtons path.

Tests

SideButtonsTests covers the defaults, the legacy swap, honoring stored values, the reserved-button fallback, both collision directions, and two matching stored values. The last test is the one that matters most: it asserts the invariant across every combination of 11 stored values per direction against both legacySwap settings — 242 cases — checking that the result is always two distinct buttons that are both assignable. That's what turned up the wider reachable states above.

The regression tests were written first and watched failing against the previous per-direction logic, which is preserved as the first version of the new type so the red run reproduces the real bug rather than a hypothetical one.

Verification

swift test — 24 tests, 0 failures. swift build and ./scripts/build-app.sh — clean under -warnings-as-errors. Both commits build and pass tests independently, so the range bisects.

Found while investigating an unrelated report, and unrelated to the reverse-scroll feature in #2.

Made with Cursor

Review

Reviewed by two independent reviewers. Bugbot found no bugs. A senior-reviewer pass swept resolve over 2,888 stored-value combinations without finding a collision or an out-of-range result, mutation-tested it with 10 injected bugs, and confirmed the assign round trip still works now that the getters return a resolved rather than a raw stored value.

It also caught a real regression this PR had introduced. The first version collapsed "never mapped" and "mapped to a button this build reserves" into the same case, which made the superseded swapSideButtons flag apply to both — where previously it applied only to a direction with no stored value at all. A legacy-swap install with an out-of-range stored button would have had its other direction silently move. Fixed by threading that distinction through, with a regression test pinning it.

To verify the fix rather than assume it, I reimplemented the old per-direction resolution and compared it against the new one across 2,888 combinations: 2,778 identical, 110 changed, and all 110 are configurations the old code resolved to a single button driving both directions. No mapping that previously worked changes.

Two further review findings are addressed: the back-wins tie-break now has an assertion (a mutation flipping it previously passed the whole suite), and the unvalidated button setters are private so assign is the only way to write a mapping.

Not taken: normalizing the resolved pair back into UserDefaults on write, which would also stop a shadowed duplicate reappearing when the user edits the other direction. That state is only reachable by hand-editing the defaults domain, since assign cannot write a duplicate pair, and a migration is a larger change than this bug warrants. Worth doing as follow-up if the legacy flag is ever retired.