Skip to content

refactor(mobile): centralize safe-area spacing in useScreenInsets#2314

Merged
Gilbert09 merged 2 commits into
mainfrom
mobile-screen-insets-hook
May 28, 2026
Merged

refactor(mobile): centralize safe-area spacing in useScreenInsets#2314
Gilbert09 merged 2 commits into
mainfrom
mobile-screen-insets-hook

Conversation

@Gilbert09
Copy link
Copy Markdown
Member

Why

On mobile, screens each called useSafeAreaInsets() and then hand-added their own magic offset for top/bottom spacing (12 / 16 / 20 / 24 / 32 / 40 / 50). The device insets are the correct cross-platform/cross-device primitive — and they're used — but the policy layered on top was duplicated and inconsistent per screen. (Originated from a question about the gap under the task-screen composer: Math.max(insets.bottom, 50), correct for that screen but not shared anywhere.)

What

  • useScreenInsets hook — single source of truth for spacing policy. Wraps useSafeAreaInsets() and exposes a rationalized 12/24/40 scale via bottom("compact" | "default" | "roomy"), plus composerBottom(), fabBottom(), contentTop(), and raw insets.
  • SheetContainer component — extracts the duplicated bottom-sheet shell (backdrop, pinned panel, rounded top, border, shadow, drag handle, inset-aware padding). SelectSheet and AttachmentSheet now render through it.
  • Migrated 17 call sites — FABs, composers, page sheets, and the mcp/settings/pr-diff/automation-create screens.

Rationalization (intentional)

Two values snap onto the scale; everything else maps exactly:

  • settings bottom: 32 → 24
  • pr-diff bottom: 16 → 24

Left bespoke on purpose

Functional clearances that aren't safe-area policy: DismissReportSheet's +120 (clears its sticky footer), automation/create's computed floating-button clearance, inbox/[id]'s +100/+16, and measured floating-header heights (insets.top + 60/64).

Testing

  • biome check: clean
  • tsc --noEmit: no new errors (pre-existing test-file errors unchanged vs. main)
  • ⚠️ Not visually verified on a simulator — the two 8px shifts (settings, pr-diff) are low-risk but worth an eyeball in light/dark.

🤖 Generated with Claude Code

Screens previously hand-rolled their bottom/top safe-area spacing, each
calling useSafeAreaInsets() and adding its own magic offset
(12/16/20/24/32/40/50). The device insets are the right cross-platform
primitive, but the policy on top of them was duplicated and inconsistent.

Add a useScreenInsets hook that owns that policy with a rationalized
12/24/40 scale (compact/default/roomy) plus composerBottom, fabBottom,
and contentTop helpers. Extract the duplicated bottom-sheet shell into a
SheetContainer component (backdrop, panel, shadow, drag handle, inset)
and move SelectSheet/AttachmentSheet onto it.

Migrate FABs, composers, page sheets, and the mcp/settings/pr-diff/
automation-create screens onto the hook. Two values snap to the scale
(settings 32->24, pr-diff 16->24); everything else maps exactly.
Functional clearances (clearing a sticky footer or floating button,
measured header heights) are left bespoke since they aren't safe-area
policy.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Gilbert09 Gilbert09 requested review from dmarticus and oliverb123 May 22, 2026 21:07
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 22, 2026

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
apps/mobile/src/app/task/[id].tsx:62-63
`composerBottom` is a plain JS function from `useMemo`, not a Reanimated worklet. `useAnimatedStyle` callbacks that read a `SharedValue` (`height.value`) are serialized and executed on the UI thread, where calling non-worklet JS functions throws a runtime error. The sibling file `task/index.tsx` avoids this correctly by pre-computing `restingBottom = bottom("compact")` outside the animated style and capturing the primitive number in the worklet closure. The same pattern should be applied here.

```suggestion
  const { insets, composerBottom } = useScreenInsets();
  const composerBottomValue = composerBottom();
  const themeColors = useThemeColors();
```

### Issue 2 of 2
apps/mobile/src/app/task/[id].tsx:125-128
Once the value is pre-computed outside the worklet, use the scalar directly and update the dependency array accordingly.

```suggestion
    return {
      marginBottom: height.value < 0 ? 0 : composerBottomValue,
    };
  }, [composerBottomValue]);
```

Reviews (1): Last reviewed commit: "refactor(mobile): centralize safe-area s..." | Re-trigger Greptile

Comment thread apps/mobile/src/app/task/[id].tsx
Comment thread apps/mobile/src/app/task/[id].tsx Outdated
Copy link
Copy Markdown
Contributor

@dmarticus dmarticus left a comment

Choose a reason for hiding this comment

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

Nice cleanup. The hook and SheetContainer both pull their weight — 12/24/40 is the right level of abstraction for the scatter it's replacing, and the sheet shell is genuinely shared rather than over-fit. Approving; comments below are non-blocking nits to look at before/after merge.

One thing worth eyeballing on a simulator given you flagged it: the two declared 8px shifts (settings 32→24, pr-diff 16→24) and the roomy variant collapsing the previous +40 sites — 60 seconds on iPhone SE + Pro Max in both modes should be enough.

Comment thread apps/mobile/src/app/task/[id].tsx Outdated
Comment thread apps/mobile/src/hooks/useScreenInsets.ts Outdated
Comment thread apps/mobile/src/hooks/useScreenInsets.ts
Comment thread apps/mobile/src/components/SheetContainer.tsx
Comment thread apps/mobile/src/components/SheetContainer.tsx
Comment thread apps/mobile/src/hooks/useScreenInsets.ts
- Pre-compute composerBottom() outside the useAnimatedStyle worklet in
  task/[id].tsx so the UI thread never calls the non-worklet getter
- Rename contentTop() -> sheetContentTop() to match its sheet-only scope
- Document fabBottom()/composerBottom() as frozen, non-variant choices
- Note compact is the inherited sheet default on SheetContainer
- Accept a style prop on SheetContainer for measured overrides
- Add a unit test pinning the spacing scale
@Gilbert09 Gilbert09 enabled auto-merge (squash) May 28, 2026 10:31
@Gilbert09 Gilbert09 merged commit 9457c1e into main May 28, 2026
17 checks passed
@Gilbert09 Gilbert09 deleted the mobile-screen-insets-hook branch May 28, 2026 10:43
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.

2 participants