Skip to content

fix(mobile): recurring 3-dots menu no-op — structural fix + regression guards (#12) - #324

Merged
thomasluizon merged 2 commits into
mainfrom
fix/mobile-menu-anchored
Jun 27, 2026
Merged

fix(mobile): recurring 3-dots menu no-op — structural fix + regression guards (#12)#324
thomasluizon merged 2 commits into
mainfrom
fix/mobile-menu-anchored

Conversation

@thomasluizon

Copy link
Copy Markdown
Owner

Recurring mobile 3-dots menu no-op — fixed at the root + guarded

Fixes the habit 3-dots menu that shows its press animation but doesn't open — recurring across ~9 past fixes.

Root cause

setMenuVisible(true) ran only inside an async measureInWindow callback. On Android Fabric/Hermes release builds (or a flattened ref view) that native measure silently no-ops → callback never fires → menu never opens, while the Pressable still animates. It throws nothing (invisible to Sentry) and the test mock hard-coded measureInWindow to always succeed (invisible to CI). Structural — it returns through a new door after each trigger-only fix.

Fix

  • Open synchronously: useAnchoredMenu().open() sets visibility immediately; measureInWindow only refines the anchor.
  • Fallback anchor: AnchoredMenu positions from anchorRect ?? fallback so an "open" menu always paints.
  • One seam: centralized useAnchoredMenu + MenuAnchorHost (collapsable={false}) replace all 4 ad-hoc sites (habit-row, today ×2, goals) — the invariant can't be dropped per-site again. Web unchanged (DOM Popover, no measure no-op).

Regression guards (why this stops recurring)

  • Behavioral test: presses the 3-dots with a mock where measureInWindow never fires (+ a null-ref variant) and asserts the menu opens — fails on the old code, passes on the fix.
  • Lint rule (no-gorhom-sheet): bans @gorhom/bottom-sheet and imperative .present()/.dismiss() outside the sheet wrapper — catches the prior reintroduction vector at CI time.

Validation

mobile type-check + lint (0 errors) + 639 tests; web + shared green.

🤖 Generated with Claude Code

… guards (#12)

Root cause: setMenuVisible ran only inside an async measureInWindow callback that silently no-ops on Android Fabric release builds. Now opens synchronously via a centralized useAnchoredMenu hook + MenuAnchorHost (all 4 sites) with a fallback anchor. Guards: behavioral test (menu opens when measureInWindow never fires / ref null) + no-gorhom-sheet lint rule (bans gorhom + imperative sheet present/dismiss outside the wrapper). Web unaffected (DOM Popover).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented Jun 27, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
orbit-ui-mobile-web Ignored Ignored Jun 27, 2026 5:22pm

Request Review

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE — PR #324 structural fix review: no Critical/High findings. Root cause fix (synchronous visibility + fallback anchor + collapsable=false), one-seam consolidation across all 4 sites, behavioral regression tests that fail on old code, no-gorhom-sheet lint guard. Comment policy, type safety, parity all clean. See full review report in PR comments.

# Conflicts:
#	apps/mobile/components/today/today-habits-header.tsx
#	apps/mobile/test-mocks/lucide-react-native.ts
@thomasluizon
thomasluizon merged commit 8af4bab into main Jun 27, 2026
8 checks passed
@thomasluizon
thomasluizon deleted the fix/mobile-menu-anchored branch June 27, 2026 17:22
@sonarqubecloud

Copy link
Copy Markdown

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE — No Critical or High findings. See review summary below.

Reviewed with Claude Code /pr-review. Full findings follow.

SUMMARY: Structural fix for recurring Android Fabric/Hermes menu no-op. setMenuVisible(true) was gated inside async measureInWindow callback that silently no-ops on flattened native views. Fix is correct: visibility set synchronously, measureInWindow only refines anchor position, getFallbackAnchorRect guards the always-paints invariant, MenuAnchorHost (collapsable=false) centralises the invariant so it cannot be dropped per-site.

FINDINGS:
Critical: None
High: None
Medium: None
Low/Info: AnchoredMenu function is 136 lines (apps/mobile/components/ui/anchored-menu.tsx:111), above the 100-line soft guideline. Adversarial pass found pieces are structurally coupled — React 19 derived-state pattern must stay in render scope, both animation useEffects share the same Animated.Value that feeds JSX interpolations, menuHeight updated by onLayout is consumed by position useMemo. No clean extraction exists without a 4-5 return-value hook with worse encapsulation. ~25 lines are irreducible Modal JSX boilerplate. No action required on merge.

SUBAGENTS:
parity-checker: N/A — mobile-only change; web uses DOM Popover with getBoundingClientRect, correctly unchanged
i18n-syncer: IN SYNC — no new user-facing strings or i18n keys
contract-aligner: N/A — no packages/shared/types or orbit-api changes
security-reviewer: N/A — no orbit-api changes

VALIDATION: N/A (CI) — author reports 0 lint/type errors, 639 tests passing

DEFERRED:

  • Contract drift + backward-compat: N/A — no Zod/DTO changes, old-client break guard has nothing to check
  • Security: N/A — no trust boundaries, auth flows, or API endpoints changed
  • Backend hard rules: N/A — no orbit-api changes
  • DESIGN.md: Scanned — rgba(0,0,0,0.16) backdrop scrim is documented in DESIGN.md shadow specs, not a token violation
  • All 15 changed files verdicted

WHAT IS GOOD:

  • Root-cause inversion: open() sets visible synchronously, measureInWindow only refines — correct fix
  • getFallbackAnchorRect: menu always paints near top-right when measurement fails; unit tests for this exact path
  • Single seam: MenuAnchorHost with collapsable=false in one place prevents per-site invariant drops
  • Regression tests that fail on old code: __setMeasureInWindowImpl(() => {}) and __setHostRefsNull(true) model the production failure mode precisely
  • no-gorhom-sheet ESLint rule: blocks static imports, dynamic import(), require(), and .present()/.dismiss() outside the wrapper — closes the reintroduction vector
  • Type safety: no any in production code; AnchoredMenuController well-typed throughout

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