Skip to content

refactor(mobile): consolidate tap-target geometry into one module - #6707

Merged
iscekic merged 2 commits into
mainfrom
kwf/janitor-2026-09-24-maintainability-92be
Sep 25, 2026
Merged

iscekic merged 2 commits into
mainfrom
kwf/janitor-2026-09-24-maintainability-92be

Conversation

@iscekic

@iscekic iscekic commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

Changelog for users

  • The session filter sheet shows the PLATFORM section only when a platform option exists, so no empty PLATFORM heading sits above PROJECT.
  • The 32px header controls and the h-11 PR-review controls keep the same size and tap reach.

Changelog for maintainers

  • apps/mobile/src/lib/a11y/tap-target.ts now owns all mobile tap-target geometry; touch-target.ts and its test are deleted.
  • No import of @/lib/a11y/touch-target remains, and rg -n 'export const COMPACT_CONTROL_HIT_SLOP_DP' apps/mobile/src returns exactly one hit, the 8dp value in tap-target.ts.
  • The h-11 geometry now uses distinct names, COMPACT_H11_FRAME_DP = 38.5 and COMPACT_H11_HIT_SLOP_DP = 3.
  • compactControlTargetDp() delegates to tapTargetReachDp, and MIN_TAP_TARGET_DP = 28 is the single 28dp floor.
  • The PR resolve control and the comment-row overflow import the renamed constants; their classes, hit slop, and numbers are unchanged.
  • The session filter sheet hides an empty PLATFORM section, matching the existing PROJECT guard, and a mounted test covers the empty case.
  • The four geometry suites pass (28 tests across 6 files); typecheck, lint, class-check, and format pass for the mobile app.
  • Review tap-target.ts first; the rest is rename and repoint, and a stale import fails typecheck instead of drifting silently.

E2E proof

[e2] ux-check: Agents header search opens and filter opens its sheet, 32px box — Aandroid emulator-5554 from signed-in-home (STATE HIT, e2-state7.log); filter sheet opens — e2-scene5.log 'SCENE e2 OK' with 'android.widget.TextView PROJECT tappable [101,1102][979,1139]' and 'android.widget.Button Apply tappable [821,1277][979,1393]' and no orphaned PLATFORM label (the changed guard); search opens — e2-search.log 'SCENE e2search OK' with 'android.widget.Button Clear search tappable [902,280][1001,380]' and 'android.widget.TextView No sessions match tappable [360,1084][720,1149]'; header tree e2-header.txt shows 'content-desc="Filter sessions"…

[e4] ux-check: header search opens search and filter opens its sheet — android emulator-5554; e4-scene.log 'SCENE e4 OK' with digest 'android.view.View Filter sessions tappable [100,971][978,1027]' plus PLATFORM/PROJECT/Apply proves the filter sheet; e4-header.txt shows the header controls (content-desc="Filter sessions" resource-id="agents-open-filters", content-desc="Search sessions") and e4-search.txt shows content-desc="Clear search" with 'No sessions match' after typing, so search opens. Header capture e4-header.png (header, search, filter stills) left for the visual reviewer; 32px visual box is their judgement. No UX-DEFECT observed.

[e2] PR review resolve control still toggles (android, platform:both scope) — Android: restored pr-review, then opened a real PR with a review thread (#6694) because that state opens #6054 which has no review thread, and ran the replay (e2nav.log: 'SCENE e2nav OK'); the tap flipped the thread to resolved (e2-thread-resolved.txt: content-desc="Unresolve thread", text="RESOLVED") and back to unresolved (e2-thread-unresolved.txt: content-desc="Resolve thread"), the control's frame bounds "[908,508][1009,609]" identical in both states; harness obstacle filed as obstacle:state-pr-review-empty-discussion.

[e5] ux-check: Resolve control toggles to Unresolve and back and keeps its h-11 frame (android, platform:both scope) — Android: tapped Resolve then Unresolve on the thread header control (e2nav.log: 'SCENE e2nav OK'; e2-thread-resolved.txt: content-desc="Unresolve thread" with text="RESOLVED"; e2-thread-unresolved.txt: content-desc="Resolve thread"), the 101px h-11 frame bounds "[908,508][1009,609]" unchanged on tap; UX audit of the visited thread-header, PR Overview and PR Review entry screens found no UX-DEFECT (screenshots e2-resolve-control-resolved.png, e2-resolve-control-unresolved.png, e2-pr-overview.png, e2-pr-review-entry.png).

[e5] ux-check: Resolve control toggles to Unresolve and back and keeps its h-11 frame (android, platform:both scope)

[p4] ux-check: On the Agents screen, tapping the compact header search control opens search and tapping the filter control opens its sheet; both still occupy their 32px visual box (screenshot the header). — Android (emulator-5604), pr-review start state restored (STATE HIT). One scene call: PR-detail -> Go back -> Agents tab -> tap 'Search sessions'+type -> assert 'Clear search' passed -> Clear -> tap 'Filter sessions' -> assert sheet passed; p4-scene.log line 108 'SCENE p4 OK' with the sheet digest 'android.widget.TextView PROJECT' plus Cancel/Apply and no PLATFORM label — the empty-platform guard omits the orphaned section (e4-filter repaired). Search-open recorded in p4search-scene.log:34 'SCENE p4search OK' showing 'android.widget.Button Clear search tappable [902,345][1001,445]' beside 'No…

Owner request

Surface: the mobile app (apps/mobile).

Problem

Mobile tap-target geometry is claimed to have one owner, but two modules export the same constant name with different values. Importing COMPACT_CONTROL_HIT_SLOP_DP from the wrong file silently applies 3dp instead of 8dp (or the reverse), so a compact icon button and an h-11 PR-review control can drift without a type error.

Evidence:

  • apps/mobile/src/lib/a11y/tap-target.ts:1-3 — comment: the one place mobile tap-target geometry lives.
  • apps/mobile/src/lib/a11y/tap-target.ts:10 — MIN_TAP_TARGET_DP = 28.
  • apps/mobile/src/lib/a11y/tap-target.ts:26 — 32px compact box class.
  • apps/mobile/src/lib/a11y/tap-target.ts:35 — COMPACT_CONTROL_HIT_SLOP_DP = 8 (32 + 16 = 48pt reach).
  • apps/mobile/src/lib/a11y/tap-target.ts:120-121 — tapTargetReachDp(boxDp, slopDp) is box + 2 * slop.
  • apps/mobile/src/components/ui/icon-button.tsx:3 and :10-14 — IconButton imports that 8dp slop from tap-target.
  • apps/mobile/src/lib/a11y/touch-target.ts:17 — MIN_AUDITED_CONTROL_FRAME_DP = 28 (same floor, second name).
  • apps/mobile/src/lib/a11y/touch-target.ts:20 — COMPACT_CONTROL_FRAME_DP = 38.5 (h-11 at a 14pt rem).
  • apps/mobile/src/lib/a11y/touch-target.ts:23 — COMPACT_CONTROL_HIT_SLOP_DP = 3 (same export name, different value).
  • apps/mobile/src/lib/a11y/touch-target.ts:26-27 — compactControlTargetDp() repeats tapTargetReachDp arithmetic.
  • apps/mobile/src/components/pr-review/discussion/discussion-thread.tsx:42 and :362-363 — resolve control imports the 3dp slop from touch-target and uses className="h-11 w-11 ...".
  • apps/mobile/src/lib/pr-review/comment-trailing-controls.ts:15 and :42-46 — overflow hit slop is the 3dp value from touch-target.

Call sites today pick the correct module by accident. Nothing in the type system stops a future import of COMPACT_CONTROL_HIT_SLOP_DP from the other file.

Requested behavior

Make apps/mobile/src/lib/a11y/tap-target.ts the only geometry module.

  1. Move the h-11 numbers into tap-target.ts under distinct names. Keep COMPACT_CONTROL_HIT_SLOP_DP = 8 as the 32px icon-button slop. Add something like COMPACT_H11_FRAME_DP = 38.5 and COMPACT_H11_HIT_SLOP_DP = 3 (names may differ; they must not reuse COMPACT_CONTROL_HIT_SLOP_DP). Reuse MIN_TAP_TARGET_DP for the 28dp floor; do not keep a second 28 constant.
  2. Implement compactControlTargetDp() as tapTargetReachDp(COMPACT_H11_FRAME_DP, COMPACT_H11_HIT_SLOP_DP) (or equivalent) in tap-target.ts.
  3. Point comment-trailing-controls.ts and discussion-thread.tsx at the new names from tap-target.
  4. Delete apps/mobile/src/lib/a11y/touch-target.ts. Fold or rewrite touch-target.test.ts into tap-target.test.ts so both geometries stay covered: 32+8 reach stays >= 44, and 38.5+3 reach stays >= 44.
  5. Update the three PR-review tests that import from touch-target so they import from tap-target.

Do not change any numeric value, className, or on-screen layout. The 32px IconButton path and the h-11 PR-review path must keep the same hit slop they have today.

Exclusions

  • Do not restyle IconButton, session-list header/search/filter controls, or the PR resolve/overflow controls.
  • Do not change FIX_WITH_KILO_HIT_SLOP or other comment-trailing arithmetic except the import source / renamed frame+slop constants.
  • Do not add screens, routes, schema, or packages.
  • Do not edit en.json or other catalogs.

Acceptance

  • rg -n "export const COMPACT_CONTROL_HIT_SLOP_DP" apps/mobile/src has exactly one hit, in tap-target.ts, value 8.
  • apps/mobile/src/lib/a11y/touch-target.ts does not exist. No remaining import of @/lib/a11y/touch-target.
  • discussion-thread.tsx resolve Pressable still uses h-11 w-11 and a 3dp per-side slop under the new name.
  • comment-trailing-controls.ts overflow slop is still 3 per side on a 38.5dp frame.
  • From apps/mobile/: pnpm test covering tap-target, comment-trailing-controls, discussion-thread.touch-target, and comment-row.touch-target; pnpm typecheck; pnpm lint.

E2E (both platforms)

Behavior is unchanged, so proof is regression on the two geometries.

  1. Sign in. Open Agents. Tap the compact header search and filter controls (32px + 8dp slop). They still open. Screenshot the header. Log: digest showing those controls tappable.
  2. Open a PR with a review discussion thread. Tap Resolve (or Unresolve) on a thread (h-11 + 3dp slop). It still toggles. Screenshot the thread header control. Log: the resolve control tappable and the status change.
  3. On a comment row with trailing overflow, the overflow still opens and does not steal the Fix-with-Kilo pill tap. Screenshot. Log: overflow vs pill hit targets.

Attach those screenshots and sanitized log excerpts. Quote the rg one-definition line and the unit-test pass lines in the PR.

[e6] ux-check: overflowing comment row - overflow opens moderation sheet, pill opens session (android) — ux-check pass on android: from the pr-review state, tapping the h-11 overflow rendered the sheet options 'Report content'/'Report user'/'Mute'/'Block' plus 'Cancel' (e6-overflow.txt, still e6-overflow.png), and after Cancel the pill's own tap rendered 'New session' with the prefilled PR-comment link and none of the sheet options (e6-pill.txt, still e6-pill.png); the same both-branch result is in e3-run3.log.

[e6] ux-check: overflowing comment row - overflow opens moderation sheet, pill opens session (android) — e6-overflow.png

[e6] ux-check: overflowing comment row - overflow opens moderation sheet, pill opens session (android)

[e6] ux-check: overflowing comment row - overflow opens moderation sheet, pill opens session (android) — e6-pill.png

[p4] ux-check: On the Agents screen, tapping the compact header search control opens search and tapping the filter control opens its sheet; both still occupy their 32px visual box (screenshot the header). — Android (emulator-5604), pr-review start state restored (STATE HIT). One scene call: PR-detail -> Go back -> Agents tab -> tap 'Search sessions'+type -> assert 'Clear search' passed -> Clear -> tap 'Filter sessions' -> assert sheet passed; p4-scene.log line 108 'SCENE p4 OK' with the sheet digest 'android.widget.TextView PROJECT' plus Cancel/Apply and no PLATFORM label — the empty-platform guard omits the orphaned section (e4-filter repaired). Search-open recorded in p4search-scene.log:34 'SCENE p4search OK' showing 'android.widget.Button Clear search tappable [902,345][1001,445]' beside 'No…

[p4] ux-check: On the Agents screen, tapping the compact header search control opens search and tapping the filter control opens its sheet; both still occupy their 32px visual box (screenshot the header). — p4search.png

[p4] ux-check: On the Agents screen, tapping the compact header search control opens search and tapping the filter control opens its sheet; both still occupy their 32px visual box (screenshot the header).

[p4] ux-check: On the Agents screen, tapping the compact header search control opens search and tapping the filter control opens its sheet; both still occupy their 32px visual box (screenshot the header). — p4head.png

Follow-ups (not changed here)

  • not proved live: Agents header compact controls still open: the filter control (36px box + 8dp slop) opens the filter sheet and the search field's clear control (38px box + 8dp slop) clears the query, and New session still opens the picker — platform:both(the owner request asks for both platforms; the shared tap-target geometry and its 8dp slop render on both, and the 44pt reach behind it was measured on Android physical pixels, PR 6378) (no capture cited it)
  • not proved live: PR review resolve control still toggles: the h-11 resolve Pressable with its 3dp per-side slop flips the thread status and back — platform:both(the owner request asks for both platforms; the h-11 frame measures 38.5pt at NativeWind's shared 14pt rem on both, and the resolve Pressable's hitSlop is the shared constant) (no capture cited it)
  • not proved live: ux-check: On a PR with a review discussion thread, tapping the Resolve control toggles it to Unresolve and back; the control keeps its 44px h-11 frame and responds on tap (screenshot the thread header control). Pass/fail. (no capture cited it)
  • not proved live: ux-check: On a review-comment row with trailing overflow, tapping the overflow opens the moderation sheet while tapping the Fix-with-Kilo pill opens the session (the pill tap is not stolen by the overflow). Pass/fail. (no capture cited it)

Open findings (not fixed here)

  • e4: ux-check: On the Agents screen, tapping the compact header search control opens search and tapping the filter control opens its sheet; both still occupy their 32px visual box (screenshot the header). Pass/fail.
  • the '## E2E proof' section carries no log excerpt, so nothing shows the change was driven end to end

e2-pr-review-entry

e2-pr-overview

Surface: the mobile app (apps/mobile).

## Problem

Mobile tap-target geometry is claimed to have one owner, but two modules export the same constant name with different values. Importing `COMPACT_CONTROL_HIT_SLOP_DP` from the wrong file silently applies 3dp instead of 8dp (or the reverse), so a compact icon button and an `h-11` PR-review control can drift without a type error.

Evidence:

- `apps/mobile/src/lib/a11y/tap-target.ts:1-3` — comment: the one place mobile tap-target geometry lives.
- `apps/mobile/src/lib/a11y/tap-target.ts:10` — `MIN_TAP_TARGET_DP = 28`.
- `apps/mobile/src/lib/a11y/tap-target.ts:26` — 32px compact box class.
- `apps/mobile/src/lib/a11y/tap-target.ts:35` — `COMPACT_CONTROL_HIT_SLOP_DP = 8` (32 + 16 = 48pt reach).
- `apps/mobile/src/lib/a11y/tap-target.ts:120-121` — `tapTargetReachDp(boxDp, slopDp)` is box + 2 * slop.
- `apps/mobile/src/components/ui/icon-button.tsx:3` and `:10-14` — IconButton imports that 8dp slop from `tap-target`.
- `apps/mobile/src/lib/a11y/touch-target.ts:17` — `MIN_AUDITED_CONTROL_FRAME_DP = 28` (same floor, second name).
- `apps/mobile/src/lib/a11y/touch-target.ts:20` — `COMPACT_CONTROL_FRAME_DP = 38.5` (`h-11` at a 14pt rem).
- `apps/mobile/src/lib/a11y/touch-target.ts:23` — `COMPACT_CONTROL_HIT_SLOP_DP = 3` (same export name, different value).
- `apps/mobile/src/lib/a11y/touch-target.ts:26-27` — `compactControlTargetDp()` repeats `tapTargetReachDp` arithmetic.
- `apps/mobile/src/components/pr-review/discussion/discussion-t
@iscekic
iscekic marked this pull request as draft September 24, 2026 18:04
@kilo-code-bot

kilo-code-bot Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental commit d3b9e0d adds a minimal react-native mock (I18nManager.isRTL = false) to comment-trailing-controls.test.ts; the module under test reaches react-native only transitively via tap-target.ts, which imports I18nManager alone, so the mock is sufficient and introduces no runtime or test-quality defect.

Files Reviewed (1 file)
  • apps/mobile/src/lib/pr-review/comment-trailing-controls.test.ts

Incremental review of d3b9e0d6b8c47d87c0001bb733a3fb963320b4cf (previous 71f75e73c). No Code Review Findings remain open.

Previous Review Summary (commit 71f75e7)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 71f75e7)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The tap-target consolidation is a value-preserving rename (38.5/3 and 28/8 mappings verified identical) and the new empty-PLATFORM guard matches the existing PROJECT guard; no dangling references to the deleted touch-target module remain.

Files Reviewed (11 files)
  • apps/mobile/src/components/agents/platform-filter-modal.mounted.test.tsx
  • apps/mobile/src/components/agents/platform-filter-modal.tsx
  • apps/mobile/src/components/pr-review/discussion/comment-row.touch-target.test.tsx
  • apps/mobile/src/components/pr-review/discussion/discussion-thread.touch-target.test.tsx
  • apps/mobile/src/components/pr-review/discussion/discussion-thread.tsx
  • apps/mobile/src/lib/a11y/tap-target.test.ts
  • apps/mobile/src/lib/a11y/tap-target.ts
  • apps/mobile/src/lib/a11y/touch-target.test.ts (deleted)
  • apps/mobile/src/lib/a11y/touch-target.ts (deleted)
  • apps/mobile/src/lib/pr-review/comment-trailing-controls.test.ts
  • apps/mobile/src/lib/pr-review/comment-trailing-controls.ts

Notes

  • COMPACT_H11_FRAME_DP = 38.5 / COMPACT_H11_HIT_SLOP_DP = 3 preserve the former touch-target values, and compactControlTargetDp() still yields 44.5 via tapTargetReachDp.
  • discussion-thread.tsx and comment-trailing-controls.ts now import the H11 names (not the 8dp COMPACT_CONTROL_HIT_SLOP_DP), so the 32px and h-11 geometries cannot be cross-wired.
  • rg confirms no remaining @/lib/a11y/touch-target import and a single COMPACT_CONTROL_HIT_SLOP_DP definition (8) in tap-target.ts.
  • The platforms.length > 0 guard hides only the label when no platform rows exist; saved-but-unavailable selections are still merged in by mergePlatformOptions, so filter removal is unaffected.
  • PR is still a draft; no memory-leak or resource-lifetime concerns in the changed code.

Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0

Review guidance: REVIEW.md from base branch main

@iscekic iscekic added the human-ready The PR is ready for human review. label Sep 25, 2026
@iscekic

iscekic commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed d3b9e0d to fix the two failing jobs (test, test (kilo-app)).

Cause. The rename moved comment-trailing-controls.test.ts from @/lib/a11y/touch-target to @/lib/a11y/tap-target. The deleted touch-target.ts imported nothing from React Native; tap-target.ts imports I18nManager. Vitest then loaded react-native/index.js (Flow syntax) and failed: RolldownError: Parse failure: Flow is not supported.

Fix. The test mocks react-native with I18nManager: { isRTL: false }, the pattern already used by src/lib/rtl-text.test.ts. No product code changed.

Evidence. All runs for d3b9e0d pass: kilo-app CI (test, typecheck, lint, format-check, check-unused, i18n-leftover incl. check:classes), CI (typecheck, lint, format-check, drizzle-check, test (kilo-app)), secret scanning, catalog, mobile gate. PR is MERGEABLE against main.

@iscekic
iscekic marked this pull request as ready for review September 25, 2026 13:49
@iscekic
iscekic merged commit d6fe80f into main Sep 25, 2026
29 checks passed
@iscekic
iscekic deleted the kwf/janitor-2026-09-24-maintainability-92be branch September 25, 2026 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

human-ready The PR is ready for human review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants