Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -162,11 +162,10 @@ export function CommentRow({
{relative}
</Text>
{/* `gap-3` (10.5pt at NativeWind's 14pt rem) exceeds the pill's 2pt
right hitSlop plus the overflow's 3pt left slop, leaving
commentTrailingControlsClearanceDp() dp between the two tap areas,
so a tap anywhere on the pill — including its right edge — opens the
session and never the moderation sheet (vr1). See
comment-trailing-controls.ts. */}
right hitSlop plus the overflow's 3pt left slop, leaving 5.5pt
between the two tap areas, so a tap anywhere on the pill — including
its right edge — opens the session and never the moderation sheet
(vr1). See comment-trailing-controls.ts and its test. */}
<View className="ml-auto flex-row items-center gap-3">
<PrCommentFixWithKilo
owner={owner}
Expand Down
10 changes: 10 additions & 0 deletions apps/mobile/src/lib/pr-review/comment-composer-params.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,16 @@ describe('parseComposerParams', () => {
expect(parseComposerParams(valid({ line: 'abc' }))).toBeNull();
});

it('rejects a partially numeric line or startLine instead of truncating it', () => {
// `Number.parseInt` stops at the first non-digit, so these used to resolve
// to lines 1, 12 and 9 and post an inline comment to a line the deep link
// never named.
expect(parseComposerParams(valid({ line: '1.5' }))).toBeNull();
expect(parseComposerParams(valid({ line: '12abc' }))).toBeNull();
expect(parseComposerParams(valid({ line: '9x', startLine: '2' }))).toBeNull();
expect(parseComposerParams(valid({ line: '9', startLine: '9x' }))).toBeNull();
});

it('rejects startLine greater than line', () => {
expect(parseComposerParams(valid({ line: '5', startLine: '6' }))).toBeNull();
});
Expand Down
25 changes: 8 additions & 17 deletions apps/mobile/src/lib/pr-review/comment-composer-params.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { parseParam } from '@/lib/route-params';
import { parseParam, parsePositiveIntParam } from '@/lib/route-params';

type RawComposerParams = {
owner?: string | string[] | undefined;
Expand All @@ -24,32 +24,23 @@ type ParsedComposerParams = {
pendingId?: string;
};

function parsePositiveInt(value: string | string[] | undefined): number | null {
const text = parseParam(value);
if (!text) {
return null;
}
const parsed = Number.parseInt(text, 10);
if (!Number.isInteger(parsed) || parsed <= 0) {
return null;
}
return parsed;
}

/**
* Runtime-validates the comment-composer route params before the screen
* queries or renders the composer. Returns `null` for any invalid
* combination (missing owner/repo/number, empty path, invalid side,
* non-positive line, or startLine greater than line).
* non-positive or partially numeric line or startLine, or startLine greater
* than line). The numeric params go through `parsePositiveIntParam` so a
* segment like `1.5` or `12abc` is rejected rather than silently truncated to
* a line the user never selected.
*/
export function parseComposerParams(raw: RawComposerParams): ParsedComposerParams | null {
const owner = parseParam(raw.owner);
const repo = parseParam(raw.repo);
const number = parsePositiveInt(raw.number);
const number = parsePositiveIntParam(raw.number);
const path = parseParam(raw.path);
const side = parseParam(raw.side, ['LEFT', 'RIGHT'] as const);
const line = parsePositiveInt(raw.line);
const startLine = parsePositiveInt(raw.startLine);
const line = parsePositiveIntParam(raw.line);
const startLine = parsePositiveIntParam(raw.startLine);
const hasStartLine = parseParam(raw.startLine) !== null;
// Optional edit-mode id: absent → undefined; present non-empty string
// passes through. Empty/array values are treated as absent (not a hard
Expand Down
87 changes: 66 additions & 21 deletions apps/mobile/src/lib/pr-review/comment-trailing-controls.test.ts
Original file line number Diff line number Diff line change
@@ -1,24 +1,82 @@
// The clearance arithmetic only keeps the pill's and the overflow's tap areas
// apart if the row lays them out the way this reads it. `comment-row.tsx`
// spells the trailing group's gap as a literal `gap-3` class (NativeWind reads
// the class at build time, so the component cannot import a number) and hands
// each control its shared hit-slop object. This guards those actual values by
// reading the row and the pill source and pairing them with the two slop
// objects, instead of re-deriving a helper the row never calls.

// eslint-disable-next-line import/no-nodejs-modules -- vitest-only guard, runs in node, never bundled into the app
import { readFileSync } from 'node:fs';
// eslint-disable-next-line import/no-nodejs-modules -- vitest-only guard, runs in node, never bundled into the app
import { fileURLToPath } from 'node:url';

import { describe, expect, it, vi } from 'vitest';

import { MIN_TAP_TARGET_DP } from '@/lib/a11y/tap-target';
import { MIN_TAP_TARGET_DP, TOUCH_TARGET_DP } from '@/lib/a11y/tap-target';
import {
COMMENT_ACTIONS_FRAME_DP,
COMMENT_ACTIONS_HIT_SLOP,
COMMENT_ACTIONS_VISUAL_DP,
COMMENT_TRAILING_CONTROLS_GAP_DP,
commentTrailingControlsClearanceDp,
FIX_WITH_KILO_HIT_SLOP,
FIX_WITH_KILO_VISUAL_DP,
} from '@/lib/pr-review/comment-trailing-controls';

vi.mock('react-native', () => ({ I18nManager: { isRTL: false } }));

// Removes `//` line comments and `/* */` block comments, preserving line
// breaks, so a comment that merely names a class or a hit-slop object cannot
// satisfy the guards below.
function stripComments(source: string): string {
return source
.replaceAll(/\/\*[\s\S]*?\*\//g, match => match.replaceAll(/[^\n]/g, ''))
.replaceAll(/\/\/[^\n]*/g, '');
}

const discussionDir = fileURLToPath(
new URL('../../components/pr-review/discussion', import.meta.url)
);
const rowSource = stripComments(readFileSync(`${discussionDir}/comment-row.tsx`, 'utf8'));
const pillSource = stripComments(
readFileSync(`${discussionDir}/pr-comment-fix-with-kilo.tsx`, 'utf8')
);

describe('comment trailing controls tap areas', () => {
it('finds the row and the pill it guards', () => {
// A renamed control or a moved file would make the assertions below pass
// vacuously.
expect(rowSource).toContain('PrCommentFixWithKilo');
expect(pillSource).toContain('FIX_WITH_KILO_HIT_SLOP');
});

it('lays the two controls out with the gap the clearance assumes', () => {
// The regression this guards: a change to the row's actual gap class would
// leave COMMENT_TRAILING_CONTROLS_GAP_DP stating a spacing the row no
// longer renders.
const groupClassName = /className="([^"]*ml-auto[^"]*)"/.exec(rowSource)?.[1] ?? '';
const classes = groupClassName.split(/\s+/);
expect(classes).toContain('flex-row');
expect(classes).toContain('items-center');
expect(classes).toContain('gap-3');
});

it('wires the shared hit slop objects into the row and the pill', () => {
expect(rowSource).toContain('hitSlop={COMMENT_ACTIONS_HIT_SLOP}');
expect(pillSource).toContain('hitSlop={FIX_WITH_KILO_HIT_SLOP}');
});

it('keeps the pill and the overflow hit areas apart', () => {
// The regression this guards: the overflow's 8pt left hitleed used to
// reach into the pill's right edge, so a tap on the pill opened the
// moderation sheet instead of the session.
expect(commentTrailingControlsClearanceDp()).toBeGreaterThan(0);
// The overflow's 3pt left slop used to reach into the pill's right edge, so
// a tap on the pill opened the moderation sheet instead of the session. The
// gap and both slops are measured from the frames, so the overflow frame's
// inset around its visible circle does not enter here.
const clearance =
COMMENT_TRAILING_CONTROLS_GAP_DP -
FIX_WITH_KILO_HIT_SLOP.right -
COMMENT_ACTIONS_HIT_SLOP.left;
expect(clearance).toBeGreaterThan(0);
expect(clearance).toBe(5.5);
});

it('keeps the overflow frame at or above the 28dp the size audit measures', () => {
Expand All @@ -35,22 +93,9 @@ describe('comment trailing controls tap areas', () => {
// the pill needs its own vertical slop to get there.
expect(
COMMENT_ACTIONS_FRAME_DP + COMMENT_ACTIONS_HIT_SLOP.top + COMMENT_ACTIONS_HIT_SLOP.bottom
).toBeGreaterThanOrEqual(44);
).toBeGreaterThanOrEqual(TOUCH_TARGET_DP);
expect(
FIX_WITH_KILO_HIT_SLOP.top + FIX_WITH_KILO_HIT_SLOP.bottom + FIX_WITH_KILO_VISUAL_DP
).toBeGreaterThanOrEqual(44);
});

it('derives the clearance from the gap and the two facing slops', () => {
// The gap and both tap areas are measured from the frames, so the overflow
// frame's inset around its visible circle does not enter here. `gap-3` is
// 0.75rem, which is 10.5dp at NativeWind's 14pt rem, so the pill's 2pt
// right slop and the overflow's 3pt left slop leave 5.5dp.
expect(commentTrailingControlsClearanceDp()).toBe(
COMMENT_TRAILING_CONTROLS_GAP_DP -
FIX_WITH_KILO_HIT_SLOP.right -
COMMENT_ACTIONS_HIT_SLOP.left
);
expect(commentTrailingControlsClearanceDp()).toBe(5.5);
).toBeGreaterThanOrEqual(TOUCH_TARGET_DP);
});
});
20 changes: 4 additions & 16 deletions apps/mobile/src/lib/pr-review/comment-trailing-controls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@
// that same rem, so its own vertical slop, not its frame, is what reaches the
// minimum. The pill's expanded right edge and the overflow's expanded left edge
// must never meet: a tap on the pill — including its right edge — must open the
// session, never the moderation sheet. Keeping the arithmetic in one place lets
// the row's gap and the two tap areas be checked together (vr1, 2026-09-16).
// session, never the moderation sheet. The row keeps the literal classes and
// the slop objects below; `comment-trailing-controls.test.ts` reads the row's
// source and these two slop objects together so the gap and the two tap areas
// are checked against what the row actually renders (vr1, 2026-09-16).

import { COMPACT_H11_FRAME_DP, COMPACT_H11_HIT_SLOP_DP } from '@/lib/a11y/tap-target';

Expand Down Expand Up @@ -48,17 +50,3 @@ export const COMMENT_ACTIONS_HIT_SLOP = {

/** The trailing group's `gap-3` class, in dp: 0.75rem at NativeWind's 14pt rem. */
export const COMMENT_TRAILING_CONTROLS_GAP_DP = 10.5;

/**
* Horizontal dp between the pill's expanded right edge and the overflow's
* expanded left edge: the gap between the two frames less each control's slop
* on the facing side. The overflow's frame is wider than its visible circle,
* but the gap and both slops are measured from the frame, so the frame's inset
* does not enter here. A positive value means the two tap areas never overlap,
* so each control keeps every tap that lands on it.
*/
export function commentTrailingControlsClearanceDp(): number {
return (
COMMENT_TRAILING_CONTROLS_GAP_DP - FIX_WITH_KILO_HIT_SLOP.right - COMMENT_ACTIONS_HIT_SLOP.left
);
}
Loading