diff --git a/.kilo_workflow/learnings/mobile-android-a11y-label-leaves-geometric-grouping.md b/.kilo_workflow/learnings/mobile-android-a11y-label-leaves-geometric-grouping.md new file mode 100644 index 0000000000..1a87f0f893 --- /dev/null +++ b/.kilo_workflow/learnings/mobile-android-a11y-label-leaves-geometric-grouping.md @@ -0,0 +1,19 @@ +# mobile/android: RN a11y-label container nodes are LEAVES — group card content by geometric containment + +Symptom: an Appium page-source parse shows a PR-review thread card's label node +(`Discussion thread ...`) with its full card bounds but ZERO children; the snippet, +comments, and pills look "outside" the card when checked by XML parentage. + +Cause: on Android (uiautomator2), React Native hoists a labeled container's accessible +children to siblings in the flattened a11y tree; the label node keeps the card's rect +but stays a leaf. + +Fix: assign content to cards GEOMETRICALLY — a node belongs to the card whose +label-node rect contains it (`inside(cardRect, nodeRect)` with a 2px tolerance). Works +with the viewport-clamp learning (clamped bands still nest). Two companion traps: +`scrollUntilVisible` stops the instant the label peeks past the clamp edge — the card's +header controls are not necessarily rendered yet, so swipe in a bounded loop until the +card's own control is INSIDE its rect; and after a settle swipe, re-query rects (the +card moved) — a stale rect in a swipe loop reads as missing content. Expansion state is +component state and survives Appium sessions: remount the tab (tap sibling tab, tap +back) at flow start when a flow depends on default expansion. diff --git a/.kilo_workflow/learnings/mobile-android-pr-review-e2e-toast-a11y-deeplink-dedupe.md b/.kilo_workflow/learnings/mobile-android-pr-review-e2e-toast-a11y-deeplink-dedupe.md new file mode 100644 index 0000000000..e44e041490 --- /dev/null +++ b/.kilo_workflow/learnings/mobile-android-pr-review-e2e-toast-a11y-deeplink-dedupe.md @@ -0,0 +1,49 @@ +# mobile/android PR-review E2E: toast a11y-invisibility, deep-link dedupe, disabled-Pressable attribute (pr-review-ux-7f22 r2, 2026-07-30) + +Non-machine-specific techniques confirmed on the pixel9 API35 emulator: + +1. **sonner-native toasts are NOT in the uiautomator tree at all.** A + `toast.error('Clipboard is empty')` rendered visually for ~4-8s while 10 + consecutive UiSelector polls (text + content-desc, full-string) returned + zero matches. Detection must be pixel-based: screenshot ~1.2s after the + triggering tap, crop the top band (~y17-237 on 1080x2424), diff against a + baseline frame; auto-dismiss shows up as the band returning to baseline. + Extends `android-picker-tap-races-and-toast-capture` (which assumed the + toast is catchable by timing; on this build it is never in the tree). + +2. **RN disabled Pressable on Android exports `enabled="false"` but keeps + `clickable="true"`** (handler artifact). Assert `enabled === 'false'`, and + for a read-only control also do a functional no-op probe (elementClick, + then assert state + backend request log unchanged). + +3. **Same-route `mobile: deepLink` dedupes**: the router keeps the mounted + screen, so scroll offset, expansion state, and uncontrolled-field text all + survive across verifier runs. Scroll-to-top loops must not stop at the + first a11y sliver of a card (clamped band) — scroll until the needed child + (e.g. the first hunk gutter line) intersects the card rect. For a truly + fresh mount: `stopApp` + `launchApp`, then wait for a shell element + (`Home, tab, 1 of 4`) BEFORE the deep link — a link fired during boot is + dropped. + +4. **CSS `uppercase` transforms the a11y text**: the Resolved badge matches + `RESOLVED`, not `Resolved`. Check the source for text-transform classes + before pinning selectors. + +5. **DiffLine a11y labels (`Added line 9: ...`) are not exported to + uiautomator**; the visible per-line signal is the gutter glyph text + (`+ 9`, `- 8`, `· 15`) plus the merged code text node. Scope glyph/text + assertions to the card rect (sibling cards render the same strings). + +6. **Android uncontrolled-field cleanup**: the entry screen's clear button + calls `TextInput.clear()`, which on Android unmounts the button (state + says empty) but leaves the native text. Use driver-level + `eraseText()` (elementClear) — a real edit that fires onChangeText and + clears dependent helpers. + +7. **adb wedge recovery variant**: guest wedged under host load (3 slots, + load ~30-60) presented as adbd timeouts -> `adb devices` shows the device + OFFLINE with qemu at 0% CPU. `adb root` (then `unroot`) restarted adbd and + the guest came back; bounded polls of `getprop sys.boot_completed`, + `pidof system_server`, and `pm list packages` confirm health before + rerunning login.sh. No emulator relaunch needed. Same family as + `android-emulator-systemserver-restart-systemui-anr-under-load`. diff --git a/.kilo_workflow/learnings/mobile-e2e-fullstring-regex-selector-escaping.md b/.kilo_workflow/learnings/mobile-e2e-fullstring-regex-selector-escaping.md new file mode 100644 index 0000000000..260b309fd4 --- /dev/null +++ b/.kilo_workflow/learnings/mobile-e2e-fullstring-regex-selector-escaping.md @@ -0,0 +1,22 @@ +# mobile e2e: helper patterns are full-string REGEXES — escape parens/dots/plus in literal labels + +Symptom: a flow's `waitVisible('Discussion thread src/alpha.ts L10 (RIGHT)')` times out +for 20s while the exact card is on screen; `scrollUntilVisible('Conversation comment at T+2')` +overscrolls to the list bottom; an invariant that tolerates empty `findRects` results then +passes VACUOUSLY. + +Cause: `e2e/wdio/helpers.js` `findAll` feeds the pattern to Android UiSelector +`textMatches`/`descriptionMatches` (whole-string Java regex) or the iOS `MATCHES` predicate. +Literal labels containing regex specials silently never match: `(RIGHT)` reads as a group +(missing the literal parens), `.` matches anything, and `T+2` reads as "one or more T then 2". +Worse, `(await findRects(bad))` returns `[]`, so "no X intersects Y" invariants pass with the +target never located. + +Fix: in flow files, wrap every literal label in an escaper and assert presence before geometry: + +```js +const rx = s => new RegExp('^' + s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + '$'); +``` + +A `^...$` anchor is harmless (the driver matches whole strings anyway). When a wait/scroll +times out on something visibly on screen, suspect the pattern before suspecting the app. diff --git a/.kilo_workflow/learnings/mobile-e2e-scrolluntilvisible-lands-but-throws.md b/.kilo_workflow/learnings/mobile-e2e-scrolluntilvisible-lands-but-throws.md new file mode 100644 index 0000000000..d0cb6fd0e9 --- /dev/null +++ b/.kilo_workflow/learnings/mobile-e2e-scrolluntilvisible-lands-but-throws.md @@ -0,0 +1,14 @@ +# mobile: scrollUntilVisible throws "scrolled N times without finding" but lands on the target + +Symptom: `scrollUntilVisible('')` throws after exhausting maxScrolls, yet a +hierarchy dump taken immediately after shows the target fully visible and correctly +positioned (observed twice on iOS PR-review Discussion tab, pr-review-ux-7f22). + +Cause: the helper checks visibility between flicks; the final flick moves the list +past the last check point (momentum + FlashList re-layout), so the loop exhausts while +the end state is fine. It is the same flick-overshoot family as +`mobile-e2e-top-clip-positioning-flashlist-clamp` — not a new defect. + +Fix: after a failed scrollUntilVisible, dump the hierarchy before retrying or +classifying — the target is often already on screen. For flows that must not throw, +wrap in `.catch(() => {})` and follow with an explicit `assertVisible` probe. diff --git a/.kilo_workflow/learnings/mobile-ios-pr-link-open-button-clearx-shift.md b/.kilo_workflow/learnings/mobile-ios-pr-link-open-button-clearx-shift.md new file mode 100644 index 0000000000..ef87e6d62b --- /dev/null +++ b/.kilo_workflow/learnings/mobile-ios-pr-link-open-button-clearx-shift.md @@ -0,0 +1,18 @@ +# mobile iOS: PR-link entry screen — Open button shifts +7pt when the field gains content (clear-X) + +Symptom: verifying "invalid link renders INLINE in the reserved slot, Open button bounds +unchanged" by recording Open bounds on the empty entry screen, pasting `not-a-url`, and +re-measuring FAILS: Open moves y 236→243 and grows h 39→40 even though the helper slot is +documented as layout-stable. + +Cause: the in-field clear-X (`Clear pull request link`, h-13/w-13 ≈ 46pt) appears whenever the +field has content; it is taller than the 39pt input, so the input ROW grows 39→46 and pushes the +helper slot and Open button down ~7pt. This is field-CONTENT state, not helper-message state — +the clear-X predates the pr-review-ux-7f22 changes (present at r1 head `dd659d4a9`). + +Fix (decisive experiment for the slot invariant): hold field state constant across the helper +transition — paste `not-a-url` (helper visible, clear-X present), tap the clear-X: the shipped +clear handler does NOT clear `helperMessage`, so the invalid helper stays mounted with an empty +field, and Open returns EXACTLY to the initial rect ({x:21,y:236,w:360,h:39} on iPhone 17). +That byte-identical comparison is the assertion the AC intends; the +7pt with a filled field is +pre-existing approved behavior, not a regression. diff --git a/.kilo_workflow/learnings/mobile-ios-xcuitest-a11y-tree-geometry-containment.md b/.kilo_workflow/learnings/mobile-ios-xcuitest-a11y-tree-geometry-containment.md new file mode 100644 index 0000000000..9a842b0016 --- /dev/null +++ b/.kilo_workflow/learnings/mobile-ios-xcuitest-a11y-tree-geometry-containment.md @@ -0,0 +1,29 @@ +# mobile iOS: XCUI tree — a11y-labeled parent Views are leaves; assert by geometry, not XML ancestry + +Symptom: a flow parses `driver.getPageSource()` to assert "snippet inside thread card" via XML +parent/child containment and finds the card node has ZERO descendants — while pills, snippets and +comment texts all render on screen. Plain-text labels also appear exactly TWICE in the tree, and +`visible="true"` matches nothing, so visibility filtering empties the probe. + +Cause (iOS 26.5 sim, RN 0.86, PR-review Discussion tab, verified 2026-07-30): +- A RN View with `accessibilityLabel` (thread cards, header Pressables) becomes ONE accessibility + element; its children are NOT XML descendants — they appear as geometric SIBLINGS elsewhere in + the tree. Containment must be rect-in-rect on the page-source coordinates (`x/y/width/height`). +- Plain RN Texts are mirrored across two windows at identical rects — dedupe by + (label,x,y,width,height) before counting, and never assert `count === 1` on a Text label + (transiently or consistently 2). Interactive elements (Pressables with labels) appear once. +- Badge Texts using NativeWind `uppercase` expose the TRANSFORMED string (`RESOLVED`, `OUTDATED`, + `FILE`) — match the uppercase form. +- Off-screen FlashList rows stay mounted in the tree; the `visible` attribute is useless (never + `true`). Before coordinate-tapping a node, require `0 <= y && y+h <= screenHeight`; swipe + (clamped, harmless) until the node is in the viewport. +- Diff-line gutters (line numbers, +/- markers) are `accessibilityElementsHidden`; the code + container's `buildDiffLineAccessibilityLabel` (`Deleted line 8: ...`) is NOT what reaches the + tree — the inner selectable RNText's raw code string is. Assert snippet content via code texts + (`// stub change`, `return 1;`), and rely on screenshots for line-number evidence. +- Emoji labels (`👍 reaction, 2 reactions`) match fine through the helpers' predicate path. + +Fix: parse page source into a rect tree (fast-xml-parser from the repo's `.pnpm` store requires +fine from scratch flows), keep `flat()` (dedupe) + `insideRect()` + `inViewport()` helpers, and +tap by node-rect centre via W3C actions instead of global `tapOn` when the same label exists in +several cards (e.g. `Resolve thread`). diff --git a/apps/mobile/e2e/github-api-stub/server.mjs b/apps/mobile/e2e/github-api-stub/server.mjs index b1e2812856..98bc078f94 100644 --- a/apps/mobile/e2e/github-api-stub/server.mjs +++ b/apps/mobile/e2e/github-api-stub/server.mjs @@ -4,9 +4,23 @@ * Node built-ins only. Logs every request; GraphQL logs operation name + variables. * * Identities: - * kilo-stub/discussion-mixed#1 — interleaved review + conversation fixture - * kilo-stub/discussion-conversation-only#2 — conversation comments only (0 review threads) - * kilo-stub/discussion-empty#3 — empty discussion + * kilo-stub/discussion-mixed/1 — iOS verifier (id suffix p1) + * kilo-stub/discussion-mixed/11 — Android verifier (id suffix p11) + * kilo-stub/discussion-conversation-only#2 — conversation comments only (0 review threads) + * kilo-stub/discussion-empty#3 — empty discussion + * + * discussion-mixed timeline (T+minutes from T0): + * T+1 threadA c1, T+2 conv1 (dave), T+3 conv2, T+4 threadB c1, + * T+5 threadA c2 (reply), T+6 conv3, T+7 outdated, T+8 file-level, T+9 resolved + * + * Id-suffix scheme (D9): every thread/comment/node id ends in `_p{suffix}` so parallel + * platform verifiers never share mutable ids. Numeric databaseIds are offset per key + * (e.g. 1001 → 11001 for /11). Mutations carry only threadId/subjectId — the stub + * resolves targets by scanning ALL fixtures for the unique id. + * + * Mutation support (stateful, process-lifetime in-place on per-key fixture objects): + * ResolveThread / UnresolveThread — { input: { threadId } } + * AddReaction / RemoveReaction — { input: { subjectId, content } } */ import http from 'node:http'; import fs from 'node:fs'; @@ -20,7 +34,8 @@ const LOG_PATH = const T0 = '2026-03-01T12:00:00.000Z'; // Interleaved timeline (T+minutes from T0): -// T+1 threadA c1, T+2 conv1, T+3 conv2, T+4 threadB c1, T+5 threadA c2, T+6 conv3 +// T+1 threadA c1, T+2 conv1, T+3 conv2, T+4 threadB c1, T+5 threadA c2, T+6 conv3, +// T+7 outdated, T+8 file-level, T+9 resolved const ts = minutes => { const d = new Date(T0); d.setUTCMinutes(d.getUTCMinutes() + minutes); @@ -39,23 +54,40 @@ const restUser = login => ({ site_admin: false, }); -const reactionGroups = () => - ['THUMBS_UP', 'THUMBS_DOWN', 'LAUGH', 'HOORAY', 'CONFUSED', 'HEART', 'ROCKET', 'EYES'].map( - content => ({ +const REACTION_CONTENTS = [ + 'THUMBS_UP', + 'THUMBS_DOWN', + 'LAUGH', + 'HOORAY', + 'CONFUSED', + 'HEART', + 'ROCKET', + 'EYES', +]; + +/** + * Full 8-group array. Optional overrides patch named contents only; + * count is written to the nested reactors.totalCount path. + * @param {Record} [overrides] + */ +const reactionGroups = (overrides = {}) => + REACTION_CONTENTS.map(content => { + const o = overrides[content] ?? {}; + return { content, - viewerHasReacted: false, - reactors: { totalCount: 0 }, - }) - ); + viewerHasReacted: o.viewerHasReacted ?? false, + reactors: { totalCount: o.count ?? 0 }, + }; + }); /** GraphQL IssueComment node for pullRequest.comments (PrReviewConversationComments). */ -const conversationComment = (databaseId, login, body, minutes) => ({ - id: `IC_stub_${databaseId}`, +const conversationComment = (databaseId, login, body, minutes, idSuffix, reactionOverrides) => ({ + id: `IC_stub_${databaseId}_p${idSuffix}`, databaseId, author: author(login), body, createdAt: ts(minutes), - reactionGroups: reactionGroups(), + reactionGroups: reactionGroups(reactionOverrides), }); /** REST pull-request file entry (GET /repos/{owner}/{repo}/pulls/{n}/files shape). */ @@ -88,15 +120,32 @@ const stubFiles = () => [ ), ]; -/** @type {Record} */ -const FIXTURES = { - 'kilo-stub/discussion-mixed/1': { +const DIFF_HUNK_ALPHA = + '@@ -8,4 +8,9 @@ export function alpha() {\n- return 1;\n+ // stub change\n+ return 2;\n+}\n+\n+export function alphaExtra() {\n+ return 3;\n }'; +const DIFF_HUNK_BETA = + '@@ -18,3 +18,6 @@ export function beta() {\n- return 1;\n+ return 2;\n+}\n+\n+export function betaExtra() {\n+ return 3;\n }'; +const DIFF_HUNK_ALPHA_OUTDATED = + '@@ -7,5 +7,5 @@ export function alpha() {\n const x = 0;\n- return 1;\n+ return 1;\n // end\n }'; + +/** + * Fresh mixed-discussion fixture per call (D9). idSuffix is the platform key + * fragment (1 = iOS, 11 = Android). databaseIds offset so nothing keys on dupes. + * @param {string|number} idSuffix + */ +function buildMixedFixture(idSuffix) { + const s = String(idSuffix); + const dbOff = (Number(idSuffix) - 1) * 1000; + const db = n => n + dbOff; + const tid = name => `PRRT_${name}_p${s}`; + const cid = name => `PRRC_${name}_p${s}`; + + return { title: 'Mixed discussion fixture', body: 'PR body for mixed fixture.', files: stubFiles(), threads: [ { - id: 'PRRT_thread_a', + id: tid('thread_a'), isResolved: false, isOutdated: false, subjectType: 'LINE', @@ -110,16 +159,20 @@ const FIXTURES = { pageInfo: { hasNextPage: false, endCursor: null }, nodes: [ { - databaseId: 1001, - id: 'PRRC_a1', + databaseId: db(1001), + id: cid('a1'), body: 'Thread A comment at T+1 (inline review)', createdAt: ts(1), author: author('alice'), - reactionGroups: reactionGroups(), + diffHunk: DIFF_HUNK_ALPHA, + reactionGroups: reactionGroups({ + THUMBS_UP: { count: 2, viewerHasReacted: false }, + HEART: { count: 1, viewerHasReacted: true }, + }), }, { - databaseId: 1005, - id: 'PRRC_a2', + databaseId: db(1005), + id: cid('a2'), body: 'Thread A reply at T+5 (inline review)', createdAt: ts(5), author: author('bob'), @@ -129,7 +182,7 @@ const FIXTURES = { }, }, { - id: 'PRRT_thread_b', + id: tid('thread_b'), isResolved: false, isOutdated: false, subjectType: 'LINE', @@ -143,11 +196,89 @@ const FIXTURES = { pageInfo: { hasNextPage: false, endCursor: null }, nodes: [ { - databaseId: 1004, - id: 'PRRC_b1', + databaseId: db(1004), + id: cid('b1'), body: 'Thread B comment at T+4 (inline review)', createdAt: ts(4), author: author('carol'), + diffHunk: DIFF_HUNK_BETA, + reactionGroups: reactionGroups(), + }, + ], + }, + }, + { + id: tid('thread_outdated'), + isResolved: false, + isOutdated: true, + subjectType: 'LINE', + path: 'src/alpha.ts', + line: null, + startLine: null, + originalLine: 9, + originalStartLine: null, + diffSide: 'LEFT', + comments: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [ + { + databaseId: db(1007), + id: cid('outdated1'), + body: 'Outdated thread comment at T+7', + createdAt: ts(7), + author: author('alice'), + diffHunk: DIFF_HUNK_ALPHA_OUTDATED, + reactionGroups: reactionGroups(), + }, + ], + }, + }, + { + id: tid('thread_file'), + isResolved: false, + isOutdated: false, + subjectType: 'FILE', + path: 'src/alpha.ts', + line: null, + startLine: null, + originalLine: null, + originalStartLine: null, + diffSide: null, + comments: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [ + { + databaseId: db(1008), + id: cid('file1'), + body: 'File-level comment at T+8', + createdAt: ts(8), + author: author('bob'), + reactionGroups: reactionGroups(), + }, + ], + }, + }, + { + id: tid('thread_resolved'), + isResolved: true, + isOutdated: false, + subjectType: 'LINE', + path: 'src/beta.ts', + line: 18, + startLine: 18, + originalLine: 18, + originalStartLine: 18, + diffSide: 'RIGHT', + comments: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [ + { + databaseId: db(1009), + id: cid('resolved1'), + body: 'Resolved thread comment at T+9', + createdAt: ts(9), + author: author('carol'), + diffHunk: DIFF_HUNK_BETA, reactionGroups: reactionGroups(), }, ], @@ -157,19 +288,27 @@ const FIXTURES = { // GraphQL pullRequest.comments nodes. REST issues/{n}/comments is intentionally // not served — S2 reads conversation comments over GraphQL only. conversationComments: [ - conversationComment(2002, 'dave', 'Conversation comment at T+2', 2), - conversationComment(2003, 'erin', 'Conversation comment at T+3', 3), - conversationComment(2006, 'frank', 'Conversation comment at T+6', 6), + conversationComment(db(2002), 'dave', 'Conversation comment at T+2', 2, s, { + THUMBS_UP: { count: 1, viewerHasReacted: false }, + }), + conversationComment(db(2003), 'erin', 'Conversation comment at T+3', 3, s), + conversationComment(db(2006), 'frank', 'Conversation comment at T+6', 6, s), ], - }, + }; +} + +/** @type {Record} */ +const FIXTURES = { + 'kilo-stub/discussion-mixed/1': buildMixedFixture(1), + 'kilo-stub/discussion-mixed/11': buildMixedFixture(11), 'kilo-stub/discussion-conversation-only/2': { title: 'Conversation-only fixture', body: 'PR body for conversation-only fixture.', files: stubFiles(), threads: [], conversationComments: [ - conversationComment(3001, 'dave', 'Only conversation comment one', 2), - conversationComment(3002, 'erin', 'Only conversation comment two', 6), + conversationComment(3001, 'dave', 'Only conversation comment one', 2, '2'), + conversationComment(3002, 'erin', 'Only conversation comment two', 6, '2'), ], }, 'kilo-stub/discussion-empty/3': { @@ -201,6 +340,48 @@ function getFixture(owner, repo, number) { return FIXTURES[fixtureKey(owner, repo, number)] ?? null; } +/** Walk all fixtures for a thread id (mutations carry no owner/repo/number). */ +function findThreadById(threadId) { + for (const fx of Object.values(FIXTURES)) { + const threads = fx.threads ?? []; + for (const thread of threads) { + if (thread.id === threadId) return thread; + } + } + return null; +} + +/** + * Walk all fixtures for a comment node id — review-comment nodes and + * conversation-comment nodes. + */ +function findCommentById(subjectId) { + for (const fx of Object.values(FIXTURES)) { + for (const thread of fx.threads ?? []) { + for (const node of thread.comments?.nodes ?? []) { + if (node.id === subjectId) return node; + } + } + for (const node of fx.conversationComments ?? []) { + if (node.id === subjectId) return node; + } + } + return null; +} + +function flipReaction(node, content, add) { + const groups = node.reactionGroups ?? []; + const group = groups.find(g => g.content === content); + if (!group) return; + if (add) { + group.viewerHasReacted = true; + group.reactors.totalCount = (group.reactors?.totalCount ?? 0) + 1; + } else { + group.viewerHasReacted = false; + group.reactors.totalCount = Math.max(0, (group.reactors?.totalCount ?? 0) - 1); + } +} + function restPull(owner, repo, number, fx) { const full = `${owner}/${repo}`; const sha = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; @@ -390,6 +571,74 @@ function handleGraphql(body, res) { }); } + // Stateful mutations — resolve target by scanning all fixtures (ids are unique + // across platform keys). Mutate fixture objects in place for process lifetime. + if (op === 'ResolveThread') { + const threadId = variables.input?.threadId; + const thread = threadId ? findThreadById(threadId) : null; + if (!thread) { + return json(res, 200, { data: { resolveReviewThread: null } }); + } + thread.isResolved = true; + return json(res, 200, { + data: { + resolveReviewThread: { + thread: { id: thread.id, isResolved: true }, + }, + }, + }); + } + + if (op === 'UnresolveThread') { + const threadId = variables.input?.threadId; + const thread = threadId ? findThreadById(threadId) : null; + if (!thread) { + return json(res, 200, { data: { unresolveReviewThread: null } }); + } + thread.isResolved = false; + return json(res, 200, { + data: { + unresolveReviewThread: { + thread: { id: thread.id, isResolved: false }, + }, + }, + }); + } + + if (op === 'AddReaction') { + const subjectId = variables.input?.subjectId; + const content = variables.input?.content; + const node = subjectId ? findCommentById(subjectId) : null; + if (!node || !content) { + return json(res, 200, { data: { addReaction: null } }); + } + flipReaction(node, content, true); + return json(res, 200, { + data: { + addReaction: { + reaction: { content }, + }, + }, + }); + } + + if (op === 'RemoveReaction') { + const subjectId = variables.input?.subjectId; + const content = variables.input?.content; + const node = subjectId ? findCommentById(subjectId) : null; + if (!node || !content) { + return json(res, 200, { data: { removeReaction: null } }); + } + flipReaction(node, content, false); + return json(res, 200, { + data: { + removeReaction: { + reaction: { content }, + }, + }, + }); + } + // Unknown GraphQL — non-401 so retry path does not rotate tokens. return json(res, 200, { data: null, diff --git a/apps/mobile/src/components/pr-review/discussion/comment-row.tsx b/apps/mobile/src/components/pr-review/discussion/comment-row.tsx index 00effc3eba..7282e38855 100644 --- a/apps/mobile/src/components/pr-review/discussion/comment-row.tsx +++ b/apps/mobile/src/components/pr-review/discussion/comment-row.tsx @@ -25,12 +25,14 @@ type CommentRowProps = { readonly comment: ReviewComment; readonly onToggleReaction: (content: ReviewReactionContent) => void; readonly reactionsDisabled?: boolean; + readonly readOnly?: boolean; }; export function CommentRow({ comment, onToggleReaction, reactionsDisabled, + readOnly, }: Readonly) { const authorName = selectCommentAuthorName(comment.author); const timestamp = parseTimestamp(comment.createdAt); @@ -61,6 +63,7 @@ export function CommentRow({ reactions={comment.reactions} onToggle={onToggleReaction} disabled={reactionsDisabled} + readOnly={readOnly} /> ); diff --git a/apps/mobile/src/components/pr-review/discussion/discussion-thread.tsx b/apps/mobile/src/components/pr-review/discussion/discussion-thread.tsx index 742c492aef..17787a82f4 100644 --- a/apps/mobile/src/components/pr-review/discussion/discussion-thread.tsx +++ b/apps/mobile/src/components/pr-review/discussion/discussion-thread.tsx @@ -1,8 +1,15 @@ -// Single review-thread card: anchor header + comments list + reply input. +// Single review-thread card: anchor header + optional quoted diff + +// comments list + reply input. // // - The thread header shows the anchor label ("src/a.ts L10 (RIGHT)" // or "File comment on src/a.ts" or "Outdated on ...") and the -// "Outdated" / "Resolved" badges when applicable. +// "Outdated" / "Resolved" badges when applicable. The "Resolved" +// badge is the sole Resolved text indicator; the resolve control +// is an icon-only circular button (a11y: "Resolve thread" / +// "Unresolve thread"). +// - Expanded LINE-anchored threads render a capped quoted diff +// snippet (from thread.diffHunk) above the comments list. File- +// level / empty / unparseable hunks show no snippet. // - Resolved threads are COLLAPSED by default (tapping the header // expands them). The repo's UI/UX rule for compact product rhythm // is to keep the noise level down on the happy path, so an @@ -17,10 +24,12 @@ import * as Haptics from 'expo-haptics'; import { Check, CheckCheck, ChevronDown, ChevronUp } from 'lucide-react-native'; +import { useMemo } from 'react'; import { Pressable, View } from 'react-native'; import { CommentRow } from '@/components/pr-review/discussion/comment-row'; import { ReplyInput } from '@/components/pr-review/discussion/reply-input'; +import { ThreadDiffSnippet } from '@/components/pr-review/discussion/thread-diff-snippet'; import { Text } from '@/components/ui/text'; import { type ReviewComment, @@ -29,6 +38,7 @@ import { selectThreadAnchorLabel, selectThreadBadges, } from '@/lib/pr-review/discussion/review-discussion-types'; +import { selectThreadDiffSnippet } from '@/lib/pr-review/discussion/thread-diff-snippet'; import { useAddReactionMutation, useRemoveReactionMutation, @@ -65,6 +75,13 @@ export function DiscussionThread({ const anchorLabel = selectThreadAnchorLabel(thread); const badges = selectThreadBadges(thread); + // Parse only when expanded; memoize so DiffLine's memo comparator sees a + // stable `lines` identity across parent re-renders (e.g. reaction toggles). + const { diffHunk, subjectType, path } = thread; + const diffSnippet = useMemo( + () => (expanded ? selectThreadDiffSnippet({ diffHunk, subjectType, path }) : null), + [expanded, diffHunk, subjectType, path] + ); const firstComment = thread.comments[0]; const isResolving = resolve.isPending || unresolve.isPending; const isReacting = addReaction.isPending || removeReaction.isPending; @@ -110,6 +127,7 @@ export function DiscussionThread({ /> {expanded ? ( <> + {diffSnippet ? : null} {thread.comments.map((comment, index) => ( 0 && 'border-t border-border pt-4')}> @@ -247,14 +265,9 @@ function ResolveToggle({ resolved, disabled, onPress }: Readonly - - - {resolved ? 'Resolved' : 'Resolve'} - + ); } diff --git a/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.tsx b/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.tsx index 726364f913..43571a3ca4 100644 --- a/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.tsx +++ b/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.tsx @@ -70,11 +70,7 @@ export function PrReviewDiscussionList({ return ( - + ); diff --git a/apps/mobile/src/components/pr-review/discussion/reaction-picker-sheet.tsx b/apps/mobile/src/components/pr-review/discussion/reaction-picker-sheet.tsx new file mode 100644 index 0000000000..347befaed8 --- /dev/null +++ b/apps/mobile/src/components/pr-review/discussion/reaction-picker-sheet.tsx @@ -0,0 +1,118 @@ +// Bottom-sheet picker for GitHub's 8 review-comment reactions. +// Pattern-copied from kilo-chat's message-reaction-picker-sheet +// (Portal + backdrop fade + slide-up card + Android BackHandler). +// Portal name is unique so chat and PR-review sheets never clobber +// each other when both trees mount. + +import { Portal } from '@rn-primitives/portal'; +import { X } from 'lucide-react-native'; +import { useEffect } from 'react'; +import { BackHandler, Pressable, View } from 'react-native'; +import Animated, { FadeIn, FadeOut, SlideInDown, SlideOutDown } from 'react-native-reanimated'; +import { useSafeAreaInsets } from 'react-native-safe-area-context'; + +import { Text } from '@/components/ui/text'; +import { useThemeColors } from '@/lib/hooks/use-theme-colors'; +import { REACTION_EMOJI, REACTION_LABEL } from '@/lib/pr-review/discussion/reaction-pills'; +import { + REVIEW_REACTION_CONTENTS, + type ReviewReactionContent, +} from '@/lib/pr-review/discussion/review-discussion-types'; +import { cn } from '@/lib/utils'; + +type ReactionPickerSheetProps = { + readonly visible: boolean; + readonly reactions: readonly { + readonly content: string; + readonly count: number; + readonly viewerHasReacted: boolean; + }[]; + readonly onClose: () => void; + readonly onPick: (content: ReviewReactionContent) => void; +}; + +export function ReactionPickerSheet({ + visible, + reactions, + onClose, + onPick, +}: Readonly) { + const colors = useThemeColors(); + const insets = useSafeAreaInsets(); + + useEffect(() => { + if (!visible) { + return undefined; + } + const subscription = BackHandler.addEventListener('hardwareBackPress', () => { + onClose(); + return true; + }); + return () => { + subscription.remove(); + }; + }, [visible, onClose]); + + if (!visible) { + return null; + } + + const reacted = new Set(); + for (const r of reactions) { + if (r.viewerHasReacted) { + reacted.add(r.content); + } + } + + return ( + + + + + + Reactions + + + + + + {REVIEW_REACTION_CONTENTS.map(content => { + const isReacted = reacted.has(content); + return ( + { + onPick(content); + }} + > + {REACTION_EMOJI[content]} + + ); + })} + + + + + ); +} diff --git a/apps/mobile/src/components/pr-review/discussion/reactions-row.tsx b/apps/mobile/src/components/pr-review/discussion/reactions-row.tsx index 43a2f0d2d5..c9ded74f62 100644 --- a/apps/mobile/src/components/pr-review/discussion/reactions-row.tsx +++ b/apps/mobile/src/components/pr-review/discussion/reactions-row.tsx @@ -1,49 +1,32 @@ // Reactions row for a single review comment. // -// GitHub's review-comment reactions are a fixed set of 8 emoji -// (`THUMBS_UP, THUMBS_DOWN, LAUGH, HOORAY, CONFUSED, HEART, -// ROCKET, EYES`). Each one is rendered as a small pill that shows -// the current count when > 0 and a darker fill when the viewer -// has already reacted. +// Renders non-zero known reaction buckets as pills, plus one +// smiley-plus "Add reaction" control that opens a picker sheet of +// all 8 GitHub review reactions. Tapping a pill or picking from the +// sheet toggles via `onToggle`; the optimistic cache reducer flips +// count + fill in the same frame. // -// Tapping a pill toggles: if the viewer has reacted, fire -// `removeReaction`; otherwise `addReaction`. The optimistic cache -// reducer (`applyReactionToggle`) updates the row instantly, so the -// count + fill flip in the same frame as the tap. -// -// Disabled state is exposed for callers that want to lock the row -// during the mutation's pending phase (rare — the optimistic update -// makes the row look responsive; the hook will still rollback on -// error). +// `disabled` locks presses during a pending mutation but keeps the +// add icon mounted (no flicker). `readOnly` (conversation comments) +// makes pills non-pressable and hides the add icon entirely; a +// zero-pill read-only row renders null. import * as Haptics from 'expo-haptics'; +import { SmilePlus } from 'lucide-react-native'; +import { useState } from 'react'; import { Pressable, View } from 'react-native'; +import { ReactionPickerSheet } from '@/components/pr-review/discussion/reaction-picker-sheet'; import { Text } from '@/components/ui/text'; -import { - REVIEW_REACTION_CONTENTS, - type ReviewReactionContent, -} from '@/lib/pr-review/discussion/review-discussion-types'; +import { useThemeColors } from '@/lib/hooks/use-theme-colors'; +import { REACTION_EMOJI, selectReactionPills } from '@/lib/pr-review/discussion/reaction-pills'; +import { type ReviewReactionContent } from '@/lib/pr-review/discussion/review-discussion-types'; import { cn } from '@/lib/utils'; -// Map each GitHub reaction content to the emoji that GitHub itself -// renders in its UI. Kept inline (not in a shared emoji module) so -// the discussion tab stays a self-contained slice. -const REACTION_EMOJI: Record = { - THUMBS_UP: '👍', - THUMBS_DOWN: '👎', - LAUGH: '😄', - HOORAY: '🎉', - CONFUSED: '😕', - HEART: '❤️', - ROCKET: '🚀', - EYES: '👀', -}; - type ReactionsRowProps = { // Raw reactions from the DTO — `content` is a plain string (GitHub can - // return content outside the 8 emoji). We index by string and only render - // + toggle the fixed 8 known reactions. + // return content outside the 8 emoji). We only render known contents + // with count > 0. readonly reactions: readonly { readonly content: string; readonly count: number; @@ -51,43 +34,75 @@ type ReactionsRowProps = { }[]; readonly onToggle: (content: ReviewReactionContent) => void; readonly disabled?: boolean; + readonly readOnly?: boolean; }; -export function ReactionsRow({ reactions, onToggle, disabled }: Readonly) { - // Index existing reactions by content for O(1) lookup. Missing - // reactions render as an empty pill (no count) so the user can - // discover the full set. - const byContent = new Map(); - for (const r of reactions) { - byContent.set(r.content, { count: r.count, viewerHasReacted: r.viewerHasReacted }); +export function ReactionsRow({ + reactions, + onToggle, + disabled, + readOnly, +}: Readonly) { + const colors = useThemeColors(); + const [pickerOpen, setPickerOpen] = useState(false); + const pills = selectReactionPills(reactions); + const isDisabled = Boolean(disabled); + const isReadOnly = Boolean(readOnly); + const pillDisabled = isDisabled || isReadOnly; + + if (pills.length === 0 && isReadOnly) { + return null; } + return ( - - {REVIEW_REACTION_CONTENTS.map(content => { - const existing = byContent.get(content); - const count = existing?.count ?? 0; - const reacted = existing?.viewerHasReacted ?? false; - return ( - { - void Haptics.selectionAsync(); - onToggle(content); - }} - /> - ); - })} + + {pills.map(pill => ( + { + void Haptics.selectionAsync(); + onToggle(pill.content); + }} + /> + ))} + {isReadOnly ? null : ( + { + void Haptics.selectionAsync(); + setPickerOpen(true); + }} + className={cn( + 'rounded-full border border-border bg-card p-1.5', + isDisabled && 'opacity-50' + )} + > + + + )} + { + setPickerOpen(false); + }} + onPick={content => { + void Haptics.selectionAsync(); + onToggle(content); + setPickerOpen(false); + }} + /> ); } type ReactionPillProps = { - readonly content: ReviewReactionContent; readonly emoji: string; readonly count: number; readonly viewerHasReacted: boolean; @@ -118,16 +133,14 @@ function ReactionPill({ )} > {emoji} - {count > 0 ? ( - - {count} - - ) : null} + + {count} + ); } diff --git a/apps/mobile/src/components/pr-review/discussion/thread-diff-snippet.tsx b/apps/mobile/src/components/pr-review/discussion/thread-diff-snippet.tsx new file mode 100644 index 0000000000..7a03b34819 --- /dev/null +++ b/apps/mobile/src/components/pr-review/discussion/thread-diff-snippet.tsx @@ -0,0 +1,37 @@ +// Quoted diff snippet rendered above comments in an expanded LINE-anchored +// discussion thread. Reuses DiffLine (no onTap — static quote only). +// When truncated, hidden lines are at the TOP (cap keeps the tail). + +import { View } from 'react-native'; + +import { DiffLine } from '@/components/pr-review/diff/diff-line'; +import { Text } from '@/components/ui/text'; +import { type ThreadDiffSnippet as ThreadDiffSnippetData } from '@/lib/pr-review/discussion/thread-diff-snippet'; + +type ThreadDiffSnippetProps = { + readonly snippet: ThreadDiffSnippetData; +}; + +export function ThreadDiffSnippet({ snippet }: Readonly) { + const truncatedCount = snippet.totalLineCount - snippet.lines.length; + return ( + + {truncatedCount > 0 ? ( + + … {truncatedCount} more lines above + + ) : null} + {snippet.lines.map((line, index) => ( + + ))} + + ); +} diff --git a/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx b/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx index 01f16cfd3c..06b2097f60 100644 --- a/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx @@ -15,18 +15,17 @@ import { EmptyState } from '@/components/empty-state'; import { ScreenHeader } from '@/components/screen-header'; import { Button } from '@/components/ui/button'; import { Text } from '@/components/ui/text'; +import { announcingToast } from '@/lib/a11y/announcing-toast'; import { parseGitHubPrUrl } from '@/lib/github-pr-url'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { getPrReviewPath } from '@/lib/profile-agent-navigation'; +import { consumePrLinkInputEcho, pushPrLinkInputEcho } from '@/lib/pr-review/pr-link-input-echo'; import { - PR_LINK_HELPER_CLIPBOARD_EMPTY_COPY, - PR_LINK_HELPER_INVALID_COPY, - type PrLinkHelperMessage, + decidePrLinkPaste, + PR_LINK_TOAST_CLIPBOARD_EMPTY_COPY, + PR_LINK_TOAST_INVALID_COPY, selectPrLinkClearButtonVisible, - selectPrLinkHelperSlotState, -} from '@/lib/pr-review/pr-link-helper-slot'; -import { consumePrLinkInputEcho, pushPrLinkInputEcho } from '@/lib/pr-review/pr-link-input-echo'; -import { decidePrLinkPaste } from '@/lib/pr-review/pr-link-paste'; +} from '@/lib/pr-review/pr-link-paste'; import { getRecentPrs, type RecentPr, upsertRecentPr } from '@/lib/pr-review/recent-prs'; const URL_PLACEHOLDER = 'https://github.com/owner/repo/pull/123'; @@ -36,17 +35,16 @@ export function PrReviewEntryScreen() { const colors = useThemeColors(); // Uncontrolled iOS input — keep the raw text in a ref so the submit // handler reads the latest value without re-rendering on every - // keystroke. State is only for derived UI (whether there's any text, - // active helper message). The TextInput component ref is for focus() - // and setNativeProps on programmatic paste. + // keystroke. State is only for derived UI (whether there's any text). + // The TextInput component ref is for focus() and setNativeProps on + // programmatic paste. const inputRef = useRef(null); const inputValueRef = useRef(''); // FIFO of values written via setNativeProps. Matching onChangeText values - // are treated as programmatic echoes (do not clear helpers / clobber ref). + // are treated as programmatic echoes (do not clobber ref). // Order-agnostic membership so double-taps and delayed native echoes work. const pendingProgrammaticTextsRef = useRef([]); const [hasInput, setHasInput] = useState(false); - const [helperMessage, setHelperMessage] = useState(null); const [recent, setRecent] = useState(null); useFocusEffect( @@ -78,10 +76,9 @@ export function PrReviewEntryScreen() { const raw = inputValueRef.current; const parsed = parseGitHubPrUrl(raw.trim()); if (!parsed) { - setHelperMessage('invalid'); + announcingToast.error(PR_LINK_TOAST_INVALID_COPY); return; } - setHelperMessage(null); // Title is backfilled on first successful load (S5). await upsertRecentPr({ owner: parsed.owner, @@ -97,16 +94,15 @@ export function PrReviewEntryScreen() { const clipboard = await Clipboard.getStringAsync(); const decision = decidePrLinkPaste(clipboard); if (decision.kind === 'empty') { - setHelperMessage('clipboard-empty'); + announcingToast.error(PR_LINK_TOAST_CLIPBOARD_EMPTY_COPY); return; } // Replace entire field (never append-at-cursor): native field + ref + hasInput. applyFieldText(decision.text); if (decision.kind === 'non-url-text') { - setHelperMessage('invalid'); + announcingToast.error(PR_LINK_TOAST_INVALID_COPY); return; } - setHelperMessage(null); await handleSubmit(); }; @@ -122,22 +118,7 @@ export function PrReviewEntryScreen() { router.push(getPrReviewPath(entry.owner, entry.repo, entry.number)); }; - const slotState = selectPrLinkHelperSlotState({ - message: helperMessage, - }); const showClearButton = selectPrLinkClearButtonVisible({ hasInput }); - const isInvalid = helperMessage === 'invalid'; - - let helperContent: ReactNode = null; - if (slotState === 'invalid') { - helperContent = {PR_LINK_HELPER_INVALID_COPY}; - } else if (slotState === 'clipboard-empty') { - helperContent = ( - - {PR_LINK_HELPER_CLIPBOARD_EMPTY_COPY} - - ); - } let recentsBody: ReactNode = null; if (recent === null) { @@ -226,22 +207,11 @@ export function PrReviewEntryScreen() { if (decision.kind === 'echo') { // Echo of setNativeProps: inputValueRef already holds the // intentional value from applyFieldText — do not clobber it - // with a delayed/stale echo, and do not clear helpers. + // with a delayed/stale echo. return; } inputValueRef.current = value; setHasInput(value.length > 0); - // Any real edit clears transient helper messages (invalid / - // clipboard-empty). Last-set message is replaced by null. - if (helperMessage !== null) { - setHelperMessage(null); - } - }} - onFocus={() => { - // clipboard-empty clears on input focus; invalid stays until edit. - if (helperMessage === 'clipboard-empty') { - setHelperMessage(null); - } }} // leading-[normal] so no lineHeight reaches the style: an explicit lineHeight // makes iOS draw the placeholder lower than the typed text (see AGENTS.md). @@ -288,9 +258,8 @@ export function PrReviewEntryScreen() { - {helperContent}