diff --git a/.kilo_workflow/dispatch-role.sh b/.kilo_workflow/dispatch-role.sh index 1093f46eae..1101c57bc0 100755 --- a/.kilo_workflow/dispatch-role.sh +++ b/.kilo_workflow/dispatch-role.sh @@ -34,7 +34,13 @@ STRIP='$(env | grep -oE "^(KILO|OPENCODE)[A-Za-z0-9_]*" | sed "s/^/-u /" | tr "\ # Redirection below means an attached pane shows nothing at all. Say so in the # pane itself — a blank window reads as a dead agent otherwise. This prints to # the terminal only, never into the log, so the EXITCODE contract is untouched. -CMD="echo $(printf '%q' "$NAME: output goes to $LOG — this pane stays blank by design; watch with: tail -f $LOG") && cd $(printf '%q' "$WT") && env $STRIP kilo run $(printf '%q' "$MSG") --agent $(printf '%q' "$ROLE") --title $(printf '%q' "$NAME")" +# `--auto`: a headless `kilo run` auto-REJECTS every permission ask it cannot +# pre-empt (e.g. protected ~/.config/kilo reads), and the reject kills the +# round silently with EXITCODE=0 and no sentinel (see +# learnings/system/headless-role-dispatch-dies-on-permission-ask.md in the main +# checkout). Role definitions already grant what these agents need; --auto +# stops the CLI answering "no" on the agent's behalf. +CMD="echo $(printf '%q' "$NAME: output goes to $LOG — this pane stays blank by design; watch with: tail -f $LOG") && cd $(printf '%q' "$WT") && env $STRIP kilo run $(printf '%q' "$MSG") --agent $(printf '%q' "$ROLE") --auto --title $(printf '%q' "$NAME")" for arg in "$@"; do CMD+=" $(printf '%q' "$arg")"; done CMD+=" > $(printf '%q' "$LOG") 2>&1; echo EXITCODE=\$? >> $(printf '%q' "$LOG")" diff --git a/.kilo_workflow/learnings/mobile-android-a11y-bounds-clamped-to-viewport.md b/.kilo_workflow/learnings/mobile-android-a11y-bounds-clamped-to-viewport.md new file mode 100644 index 0000000000..62f6edb8fc --- /dev/null +++ b/.kilo_workflow/learnings/mobile-android-a11y-bounds-clamped-to-viewport.md @@ -0,0 +1,19 @@ +# mobile: Android a11y bounds are clamped to the visible viewport — clip probes need pixels or control offsets + +Symptom: on Android, `maestro hierarchy` bounds for a partially off-screen React Native +card report only the VISIBLE band (clamped at the screen's content edge, e.g. y=444 on a +2560px-tall emulator), unlike iOS which reports window/content coordinates past the edge. +Consequences for E2E geometry probes: + +- A card's true top cannot be read once it crosses the viewport edge; repeated dumps show + the card "stuck" at the edge value. Compute the true top from a child control's reported + offset (PR-review thread cards: control row sits ~35px below the card's true top). +- Taps on a control whose band is clamped to a few px (e.g. reported `[72,444][775,453]`) + are unreliable and usually miss — Maestro and ADB/uiautomator taps alike. +- Zero-motion assertions remain valid with clamped bands: if neither the card's true rect + nor the viewport moved, the clamped band is byte-identical; any true motion changes it. + +For header-visibility questions (does a title render behind the app chrome), don't trust +a11y at all: take a screenshot and scan pixels — locate the card background color's first +visible row and count dark text pixels in the header band (PR-review card bg is +(240,238,230), page bg (251,250,245); the chrome is opaque, not blurred). diff --git a/.kilo_workflow/learnings/mobile-e2e-android-a11y-phantom-expanded-band.md b/.kilo_workflow/learnings/mobile-e2e-android-a11y-phantom-expanded-band.md new file mode 100644 index 0000000000..6b0e4f38a4 --- /dev/null +++ b/.kilo_workflow/learnings/mobile-e2e-android-a11y-phantom-expanded-band.md @@ -0,0 +1,22 @@ +# mobile: Android Maestro hierarchy invents phantom-tall bounds for clamped expanded cards — verify with uiautomator text rows + +Symptom: after expanding a PR-review thread whose header flew off-screen, `maestro +hierarchy` reports the card as e.g. `[37,444][1043,1496]` (a 1052px-tall band starting at +the a11y clamp edge), suggesting the card is merely clamped at top. `adb uiautomator dump` +on the same screen shows the truth: the card's tail (last comment + Reply row) ends ~y510 +and the next thread starts at y520 — the reported 1052px band is phantom. + +Consequence: never read expand-state or card geometry from Maestro bounds once the a11y +clamp (y=444 on pixel9 API35) is involved. The reliable probes: + +- `adb shell uiautomator dump /sdcard/window.xml` — text rows keep true y per visible row; + a thread's title ABSENT from the dump means its header is off-screen (the flow-4 failure + signature). Identify which expanded thread a clamped tail belongs to by matching its + comment text against the fixture (`grep body: server.mjs`), not by position. +- The thread's Expand/Collapse control DISAPPEARS from both dumps when its true bounds are + fully above the clamp edge — a missing control is itself evidence the header is off-screen. + +Tap geometry note (Android, same runs): the expand pressable is the title row — its band is +title-text-top +20 to +63 (e.g. title y555 → tappable ~555-598). Taps even ~10px below the +band hit the badge/meta row and silently do nothing; derive the tap point from the CURRENT +uiautomator title row (+35/+50), never from a stale dump or a previous thread's offsets. diff --git a/.kilo_workflow/learnings/mobile-e2e-drag-cancel-race-instrument-timing.md b/.kilo_workflow/learnings/mobile-e2e-drag-cancel-race-instrument-timing.md new file mode 100644 index 0000000000..d043bb1d58 --- /dev/null +++ b/.kilo_workflow/learnings/mobile-e2e-drag-cancel-race-instrument-timing.md @@ -0,0 +1,30 @@ +# mobile: D3 drag-cancel race (7b) — iOS drag NEVER lands mid-settle with Maestro; Android adb combined instrument works + +Symptom: verifying "drag the list mid-settle cancels the deferred expand" (D3 guard). Tap a +top-clipped thread's expander, then swipe. The thread expands anyway. On iOS this is NOT a +product defect and NOT (as an earlier version of this learning claimed) a drag that "lands +in time". + +Cause, measured on-device 2026-07-29 (pr-review-d957 r4, iOS 26.5 sim): the deferred settle +completes in **~316ms** (scrollToIndex animated park + promise resolution). Maestro's +tap→swipe turnaround in ONE flow file is **~700ms+** on iOS. The drag's `onScrollBeginDrag` +arrives ~400ms AFTER the expansion applied; the "failure" is the product correctly +expanding post-settle and the drag then scrolling the expanded list. r2's iOS 7b "pass" was +the same artifact (the cancel verdict was luck, not mechanism). + +Decisive readout (use it again): temporary `console.log` in `invalidateSettle(source)` and +around the scrollToIndex await in `pr-review-discussion-tab.tsx` (byte-restored after) — +Metro captures `[D3R4] begin gen N` / `resolved gen N current M match ` / +`invalidateSettle from drag|retap at `. `match false` = cancel won; +`invalidate` timestamp after `resolved` = instrument too slow. + +Working instruments: +1. **Android only**: one `adb shell "input tap x y && input swipe 540 1400 540 1100 250"` + (~30-50ms gap) on an UNclamped control (top > a11y clamp edge, e.g. y=444 on pixel9 + API35). Confirmed again in r4: drag landed 51ms into the settle, `match false`, thread + stayed collapsed. +2. iOS: NO sanctioned instrument lands mid-settle (simctl has no touch injection; a second + concurrent Maestro process wedges the driver). Report iOS 7b as instrument-blocked with + the trace, and rely on (a) the wiring trace (`invalidateSettle from drag` DOES fire for + real drags) plus (b) Android 7b for the cancel verdict. Do NOT classify an iOS expansion + after a Maestro tap→swipe as a product failure without the trace. diff --git a/.kilo_workflow/learnings/mobile-e2e-ios-mvcp-suppress-toggle-blanks-list.md b/.kilo_workflow/learnings/mobile-e2e-ios-mvcp-suppress-toggle-blanks-list.md new file mode 100644 index 0000000000..f20a057712 --- /dev/null +++ b/.kilo_workflow/learnings/mobile-e2e-ios-mvcp-suppress-toggle-blanks-list.md @@ -0,0 +1,32 @@ +# mobile: iOS FlashList blanks entirely when maintainVisibleContentPosition is toggled off for one commit and back on (Android tolerates it) + +Symptom (pr-review-d957 r5, head df4b6c379): with the deferred-expand path suppressing +`maintainVisibleContentPosition` for exactly the expand commit (`{disabled: true}` for one +auto-batched render, re-enabled on a 150ms timeout), EVERY fired top-clipped deferred +expand on iOS (4/4: gamma, epsilon, zeta geometries, incl. one uninstrumented) leaves the +discussion list COMPLETELY BLANK — no thread rows in the a11y tree, uniform page-colored +viewport, not recoverable by swipes; only a remount (tab switch) brings content back. +`getAbsoluteLastScrollOffset()` at +600ms post-expand reads the deterministic garbage +value `-997949.6666666666` (identical across runs). The settle itself is healthy +(`scrollToIndex resolved … match true` at +330-400ms, offset ~786 — the pre-expand park +lands correctly); the corruption happens at/after the suppressed expand commit or at +re-enable. Android on the same build PASSES all geometries: offset stable post-expand, +header parked at y1300 px on screen (gamma 1248 / epsilon 1314 / zeta 1140 dark px in the +header band, uiautomator title row present). + +Consequence for E2E: an iOS blank list after a deferred expand is reproducible 100% and is +NOT the r1-r4 flight signature (header off-screen by expansion height) — it is a distinct, +more severe failure mode. Diagnose with a temporary console.log of +`getLayout(index).y` / `getFirstItemOffset()` / `getAbsoluteLastScrollOffset()` at tap, +settle-resolved, and +600ms post-expand (the D3R5 trace pattern, 7 anchored insertions in +`pr-review-discussion-tab.tsx`); the -997949 offset is the smoking gun. Verify blank vs +flown with a screenshot pixel scan (uniform (251,250,245) below the chrome = blank) plus +`maestro hierarchy` showing no `Discussion thread` rows. + +iOS tap gotcha recorded on the same runs: coordinate taps at y ≤ ~182pt just under the PR +screen's tab chrome are silently swallowed (Maestro logs COMPLETED, handler never runs); +taps at y ≥ 187pt land. Place clipped-thread expand taps at ≥ y190pt. + +Fix direction for the product (not applied): do not toggle `disabled` around the commit on +iOS — e.g. gate the suppression to Android, or replace it with the recorded fallback +(exact `scrollToOffset` settle, completion-polled) so mVCP is never cycled. diff --git a/.kilo_workflow/learnings/mobile-e2e-maestro-concurrent-commands-driver-timeout.md b/.kilo_workflow/learnings/mobile-e2e-maestro-concurrent-commands-driver-timeout.md new file mode 100644 index 0000000000..3b5a30054d --- /dev/null +++ b/.kilo_workflow/learnings/mobile-e2e-maestro-concurrent-commands-driver-timeout.md @@ -0,0 +1,24 @@ +# mobile: never run two maestro commands concurrently on one UDID; screenshot loops finish before maestro's slow startup + +Symptom: `IOSDriverTimeoutException: iOS driver not ready in time` mid-run on iOS when a +background `maestro test` (tap) overlapped a `maestro hierarchy` started 1.5s later on the +same simulator. Two XCUITest channels on one UDID wedge the driver. + +Recovery: kill only the stale `xcodebuild test-without-building` PID bound to the UDID +(see `mobile-maestro-ios-driver-timeout-kill-stale-only.md`); app state, scroll position, +and login all survive. Retry with `MAESTRO_DRIVER_STARTUP_TIMEOUT=300000`. + +Second, related timing fact (iOS, machine under parallel-workflow load): maestro's JVM + +driver startup takes 15-25+s before the tap lands. A `xcrun simctl io screenshot` loop +(10-30 shots, ~0.5s each) started concurrently with the tap command ALWAYS finishes before +the tap lands — every frame is pre-tap. Timed screenshot/hierarchy capture of sub-second +windows (e.g. an 800ms probe delay) is not achievable this way, and concurrent maestro +for the window is what causes the timeout above. + +What works instead: instrument the app with temporary `console.log` probes (captured from +`pnpm dev:capture mobile`), take hierarchy dumps at leisure before and after the +interaction, and DERIVE mid-window screen geometry from the logged scroll values: +screen_y = (layoutY + firstItemOffset) - absoluteLastScrollOffset + K, where K (the +screen-y of the list viewport's content-top edge) is calibrated from a moment where both a +hierarchy bound and the logged offsets are known. On pr-review-d957 iOS K=367, confirmed +within ±1.3pt at five independent moments across three runs and scroll regions. diff --git a/.kilo_workflow/learnings/mobile-e2e-top-clip-positioning-flashlist-clamp.md b/.kilo_workflow/learnings/mobile-e2e-top-clip-positioning-flashlist-clamp.md new file mode 100644 index 0000000000..d2808fda8a --- /dev/null +++ b/.kilo_workflow/learnings/mobile-e2e-top-clip-positioning-flashlist-clamp.md @@ -0,0 +1,29 @@ +# mobile: positioning a thread at the top clip — FlashList max-offset clamp and sub-threshold swipes + +Symptom: E2E flows that need a collapsed resolved thread clipped ~5-20pt above the visible +content top (PR-review flows 4 / 7a / 7b) cannot nudge the card high enough: swipes stop +moving the list, always at the same card position. + +Cause: two independent constraints. +1. The list is at FlashList's computed max scroll offset. The clamp releases only when the + content BELOW the target card exceeds one viewport height AND has been measured. With + freshly mounted data (collapsed cards + unmeasured conversation comments), a mid-list + card simply cannot reach the clip. In the pr-review-d957 fixture (iOS), gamma clamped at + y228 until page 2 was loaded via "Load more" — r1's own flow-4 setup dump shows page-2 + threads (zeta/eta) already loaded for the same reason. On Android (2424px), zeta could + not be clipped until theta (below it) was expanded, adding ~850px below. +2. Short Maestro swipes (<~10% of screen height, e.g. 1-6%) are silently ignored on both + platforms (velocity/distance below the scroll-recognition threshold); hierarchy bounds + stay byte-identical. Do not retry them in a loop — they never land. + +Working sequence per platform: +- Load page 2 first (`scrollUntilVisible` 'Load more comments' + tap), or expand a thread + below the target to grow content. +- Approach with 30% Maestro swipes, one command at a time with a hierarchy probe between + (back-to-back commands after big swipes get ignored intermittently). +- Final 40-100px positioning: iOS — a 6-8% swipe usually lands once the clamp is released; + Android — `adb shell input swipe x y1 x y2 300` works reliably but ONLY once the offset + can grow (it is also clamped, not broken). +- Android reminder: the a11y clamp edge (y=444 on pixel9 API35, 1080x2424) is NOT the + opaque-chrome bottom (~y536 pixels); verify clip geometry with a screenshot pixel scan, + not bounds alone. diff --git a/.kilo_workflow/learnings/mobile-github-stub-seed-github-user-id-collision.md b/.kilo_workflow/learnings/mobile-github-stub-seed-github-user-id-collision.md new file mode 100644 index 0000000000..968df3b73d --- /dev/null +++ b/.kilo_workflow/learnings/mobile-github-stub-seed-github-user-id-collision.md @@ -0,0 +1,24 @@ +# mobile: devSeedUserGithubToken 500s when a sibling e2e user already owns github_user_id 999001 + +Symptom: `githubApps.devSeedUserGithubToken` with `githubUserId: "999001"` returns a 500 +("Failed query: insert into user_github_app_tokens ... on conflict ..."), even though the +runbook says a sibling seed should surface as a benign `upserted: false`. + +Cause: the local postgres on this machine is SHARED across worktrees (this worktree's stack +used the base `postgres` DB on localhost:5432 — the `postgres-N` DBs belong to other flows; +find yours via `select count(*) from user_auth_provider where provider_account_id=''` +across DBs). A previous section seeded github_user_id 999001 for a DIFFERENT kilo_user_id +(`e2e-mobile-mobile-audit-w1-pr-safety@example.com`). The upsert targets +(kilo_user_id, github_app_type); with no row for YOUR user it attempts a plain insert, which +violates unique index `UQ_user_github_app_tokens_github_user_app` on (github_user_id, app_type). + +Fix: list existing ids first — +`psql -h localhost -p 5432 -U postgres -d postgres -c "select github_user_id, github_login from user_github_app_tokens"` +— and seed with a FREE id (999003 worked). The github_user_id value only feeds the token +envelope AAD, which round-trips through the same row, so any unique value works against the stub. + +Also: the seed mutation needs an authenticated call. The web fake-login cookie path did NOT +satisfy tRPC (`UNAUTHORIZED`). Working path with curl, same as the app's own native login: +`POST /api/auth/native/otp {email}` → read the 6-digit code from the newest `dev/logs/emails/*.html` +→ `POST /api/auth/native/token {provider:'email', email, code}` → `{token}` → call tRPC with +`Authorization: Bearer `. diff --git a/.kilo_workflow/learnings/mobile-maestro-childof-fails-flat-a11y-tree.md b/.kilo_workflow/learnings/mobile-maestro-childof-fails-flat-a11y-tree.md new file mode 100644 index 0000000000..92da06fc5e --- /dev/null +++ b/.kilo_workflow/learnings/mobile-maestro-childof-fails-flat-a11y-tree.md @@ -0,0 +1,25 @@ +# mobile: Maestro childOf never matches RN a11y cards on iOS — tap by bounds-derived point + +Symptom: `tapOn: {text: 'Expand thread', childOf: {text: 'Discussion thread src/gamma.ts L33 (RIGHT)'}}` +fails with "Element not found: Text matching regex: Expand thread", while `maestro hierarchy` +clearly shows both labels on screen. + +Cause: in Maestro's flattened iOS a11y tree, a RN View with `accessibilityLabel` (the thread card) +and the Pressables INSIDE it (`Expand thread`, `Unresolve thread`) are SIBLINGS at the same depth, +not parent/descendant — verified by walking the hierarchy JSON. `childOf` finds zero parents, and +the error misleadingly names the child selector. + +Fix: parse `maestro --device hierarchy` output — every element carries `bounds` as +`[x1,y1][x2,y2]` — and `tapOn: {point: ,}`. Verify positions with a fresh +hierarchy dump immediately before the point tap; positions were stable across repeated dumps. + +Second trap: taps whose y is under the app's fixed header chrome are silently swallowed — Maestro +logs COMPLETED but nothing happens (same signature as the keyboard swallow). On the PR-review +screen the tab selector row ends at y≈166 (iPhone 17 Pro) and the effective dead zone extends to +about y≈178-180; a tap at y=174 was eaten, y=180 worked. When positioning a row for a header tap, +leave its control at y≳185; a collapsed thread card's expand control sits only 13pt below the card +top, so a "partially clipped at the list top" card's control is always inside the dead zone. + +Also: Maestro text matching treats the pattern as regex — literal parens in labels like +`L33 (RIGHT)` still MATCH because the matching is not strict full-string in every code path +(`scrollUntilVisible` found the element with the unescaped pattern). diff --git a/.kilo_workflow/learnings/mobile-maestro-ios-driver-timeout-kill-stale-only.md b/.kilo_workflow/learnings/mobile-maestro-ios-driver-timeout-kill-stale-only.md new file mode 100644 index 0000000000..fa4e091956 --- /dev/null +++ b/.kilo_workflow/learnings/mobile-maestro-ios-driver-timeout-kill-stale-only.md @@ -0,0 +1,14 @@ +# mobile: IOSDriverTimeoutException — killing ONLY the stale xcodebuild sufficed (no sim reboot) + +Update to `mobile-maestro-ios-driver-timeout-stale-xcodebuild.md` (which prescribes +kill xcodebuild + `simctl shutdown && boot`). + +Observed 2026-07-29 (pr-review-d957 repro run, sibling section thrashing Maestro driver restarts +on their own UDID every ~8s): `IOSDriverTimeoutException: iOS driver not ready in time` mid-run. +`ps aux | grep "xcodebuild test-without-building" | grep ` showed exactly one stale driver +bound to my UDID. Killing just that PID (no simulator shutdown, no reboot) and retrying with +`MAESTRO_DRIVER_STARTUP_TIMEOUT=300000` recovered immediately — app state, scroll position, and +login all survived, so the in-flight E2E probe sequence continued without re-navigation. + +Try the kill-only path first; keep the learning's shutdown+boot as the fallback when the driver +still won't come up. diff --git a/.kilo_workflow/learnings/mobile-maestro-swipe-eaten-by-sticky-header-overlay.md b/.kilo_workflow/learnings/mobile-maestro-swipe-eaten-by-sticky-header-overlay.md new file mode 100644 index 0000000000..05011437c4 --- /dev/null +++ b/.kilo_workflow/learnings/mobile-maestro-swipe-eaten-by-sticky-header-overlay.md @@ -0,0 +1,16 @@ +# mobile: Maestro `direction: DOWN` swipe is eaten by a sticky FlashList header overlay + +Symptom: on a list with `stickyHeaderIndices` (PR-review Files tab), swipes that scroll +content DOWN (offset decreases, e.g. `direction: DOWN` or start≈30% end≈70%) silently do +nothing — Maestro logs COMPLETED, hierarchy is byte-identical. Up-swipes work fine. + +Cause: FlashList's StickyHeaders overlay is an absolute-positioned sibling ON TOP of the +list, not part of the scroll content. A gesture starting inside the overlay's frame (on the +PR Files screen it sits at the list top, y≈233-267 ≈ 27-30% of an 874pt screen) does not +scroll the list. Maestro's directional DOWN swipe starts near y≈30%, inside the overlay. + +Fix: use explicit-coordinate swipes whose start is below the overlay +(`start: 50%, 40%` → `end: 50%, 75%`). Verify movement with a before/after hierarchy grep +(bounds of a stable row). Note: real users dragging from the stuck header row hit the same +non-scrollable strip — standard FlashList StickyHeaders behavior, not a defect to report +unless the product wants drag-from-header scrolling. diff --git a/.kilo_workflow/learnings/mobile-tablet-app-startup-broken.md b/.kilo_workflow/learnings/mobile-tablet-app-startup-broken.md new file mode 100644 index 0000000000..86e045d5c7 --- /dev/null +++ b/.kilo_workflow/learnings/mobile-tablet-app-startup-broken.md @@ -0,0 +1,17 @@ +# mobile: app startup/consent unusable on tablet simulators (iPad + tablet AVD) — blocks tablet E2E login + +Symptom (observed 2026-07-29, pr-review-d957, both surfaces on the same day): +- iPad Pro 11-inch (iOS 26.5): app boots to "Welcome to Kilo Code" but the consent body + and `Accept and continue` button never enter the a11y tree (logo + title only, "1 page" + scroll bars). `login.sh` and its cold-relaunch retry both fail — never reaches the email + field. +- kilo_pixel_tablet_api35 (Android): after the dev-client deep link, the resumed + MainActivity exposes NO app content (a11y tree = status bar only; screenshot pixels are + mostly black + page-bg). Metro serves the Android bundle fine. force-stop + deep-link + relaunch (the one supported recovery) does not recover. + +The phone surfaces (iPhone 17 Pro iOS 26.5, kilo_pixel9_api35) work the same day, so this +is tablet-form-factor specific. The welcome/consent screen and startup are outside any +PR-review change area. Impact: tablet side-by-side regression checks (PR-review flow 9) +are environment-limited until app startup on tablets is fixed; do not classify PR-area +failures from tablet evidence until login works there. diff --git a/.kilo_workflow/learnings/pr-review-d957-stub-edit-extraction-from-verifier-logs.md b/.kilo_workflow/learnings/pr-review-d957-stub-edit-extraction-from-verifier-logs.md new file mode 100644 index 0000000000..e550f89190 --- /dev/null +++ b/.kilo_workflow/learnings/pr-review-d957-stub-edit-extraction-from-verifier-logs.md @@ -0,0 +1,25 @@ +# pr-review-d957: re-applying the GitHub-stub temp fixture edits from prior verifier logs + +The sanctioned temp edits to `apps/mobile/e2e/github-api-stub/server.mjs` (9 threads +[a,b,c open + r1-r6 resolved], 8 long-patch files, `/files` route, page cap 5, +per-fixture counts) exist only inside `e2e-verifier-r0.log` / `-r1.log` as Edit-tool +unified diffs — no patch file was ever saved. + +Extraction traps (cost ~30min in r3): +1. Each Edit block appears TWICE consecutively in the log (terminal echo) — dedupe + consecutive identical blocks (12 blocks → 6 unique). +2. The diffs' context lines lost leading whitespace (a log artifact; dedent varies per + hunk: 2-4 spaces) — they do NOT match the pristine file, so `git apply` of the + concatenated hunks fails. +3. The `+` lines and the `@@` line numbers ARE intact; `@@` numbers are in + evolving-file coordinates (each Edit diffed the already-edited file). + +Working recipe: `git apply` hunks 1-2 (their context survived), then for hunks 3-6 +extract only the `+` lines, re-indent to match sibling code, and insert by anchored line +surgery at the real file locations (thread-array close, `conversationComment(2006,...)`, +`changed_files: 2,` triple, before the check-runs route). r1's page-cap edit (not in r0) +goes in the `PrReviewThreads` handler verbatim from the r1 log. Verify with +`node --check` plus curl probes: `/pulls/1/files` → 8 files with 4-13KB patches; +GraphQL `PrReviewThreads` page 1 → 5 threads hasNextPage=true, page 2 → 4 threads +(r4-r6,c). r3's ready-made hunk files and insert content are in +`$SCRATCH/e2e-r3/hunk*.patch` / `hunk*-insert.txt`. diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-file-list-render.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-file-list-render.tsx index 00ee8a5125..57cf8f24c5 100644 --- a/apps/mobile/src/components/pr-review/diff/pr-diff-file-list-render.tsx +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-file-list-render.tsx @@ -18,6 +18,7 @@ import { HunkSideBySideHeader, SideBySideRow, } from '@/components/pr-review/diff/pr-diff-side-by-side-row'; +import { collapseOnMarkViewed } from '@/lib/pr-review/diff/collapse-on-mark-viewed'; import { type ExpandSeparatorItem, type ListItem } from '@/lib/pr-review/diff/pr-diff-list-items'; import { type ParsedHunk } from '@/lib/pr-review/diff/parse-patch'; import { sideForDiffLineType } from '@/lib/pr-review/diff-selection'; @@ -89,6 +90,7 @@ export function useDiffRenderItem({ }} onToggleViewed={() => { void viewed.toggle(item.file.path); + setExpanded(prev => collapseOnMarkViewed(prev, item.file.path, item.viewed)); }} /> ); @@ -101,6 +103,7 @@ export function useDiffRenderItem({ githubUrl={item.githubUrl} onToggleViewed={() => { void viewed.toggle(item.file.path); + setExpanded(prev => collapseOnMarkViewed(prev, item.file.path, item.viewed)); }} /> ); diff --git a/apps/mobile/src/components/pr-review/diff/pr-diff-file-list.tsx b/apps/mobile/src/components/pr-review/diff/pr-diff-file-list.tsx index 465144c84e..d695a7f8ea 100644 --- a/apps/mobile/src/components/pr-review/diff/pr-diff-file-list.tsx +++ b/apps/mobile/src/components/pr-review/diff/pr-diff-file-list.tsx @@ -48,6 +48,7 @@ import { import { dedupeFilesByPath } from '@/lib/pr-review/diff/dedupe-file-pages'; import { buildItems } from '@/lib/pr-review/diff/pr-diff-list-builder'; import { itemTypeFor, type ListItem } from '@/lib/pr-review/diff/pr-diff-list-items'; +import { stickyFileHeaderIndices } from '@/lib/pr-review/diff/sticky-file-headers'; import { usePrDiffContextLoader } from '@/lib/pr-review/diff/use-pr-diff-context-loader'; import { useFetchToCompletion, @@ -179,6 +180,8 @@ export function PrReviewFileList({ ] ); + const stickyHeaderIndices = useMemo(() => stickyFileHeaderIndices(items), [items]); + const { handleContentSizeChange } = usePrDiffListScroll({ owner, repo, @@ -271,6 +274,11 @@ export function PrReviewFileList({ renderItem={renderItem} keyExtractor={item => item.key} getItemType={item => itemTypeFor(item)} + stickyHeaderIndices={stickyHeaderIndices} + // Height changes above the viewport (expand / collapse-on-mark) + // misfire mVCP and jump the list; gap-context insert after + // scroll-away is rare and acceptable without anchor hold. + maintainVisibleContentPosition={{ disabled: true }} // Re-measure rows when the bounded font scale changes. extraData={diffFontMetrics.scale} onContentSizeChange={handleContentSizeChange} 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 95c2c88344..742c492aef 100644 --- a/apps/mobile/src/components/pr-review/discussion/discussion-thread.tsx +++ b/apps/mobile/src/components/pr-review/discussion/discussion-thread.tsx @@ -17,7 +17,6 @@ import * as Haptics from 'expo-haptics'; import { Check, CheckCheck, ChevronDown, ChevronUp } from 'lucide-react-native'; -import { useState } from 'react'; import { Pressable, View } from 'react-native'; import { CommentRow } from '@/components/pr-review/discussion/comment-row'; @@ -45,12 +44,19 @@ type DiscussionThreadProps = { readonly repo: string; readonly number: number; readonly thread: ReviewThread; + /** Controlled by the Discussion tab (keyed by threadId). */ + readonly expanded: boolean; + readonly onToggleExpand: () => void; }; -export function DiscussionThread({ owner, repo, number, thread }: Readonly) { - // Resolved threads start collapsed; active threads start expanded. - const [expanded, setExpanded] = useState(!thread.isResolved); - +export function DiscussionThread({ + owner, + repo, + number, + thread, + expanded, + onToggleExpand, +}: Readonly) { const resolve = useResolveThreadMutation(); const unresolve = useUnresolveThreadMutation(); const addReaction = useAddReactionMutation(thread.threadId); @@ -98,9 +104,7 @@ export function DiscussionThread({ owner, repo, number, thread }: Readonly { - setExpanded(prev => !prev); - }} + onToggleExpand={onToggleExpand} onToggleResolve={onToggleResolve} resolveDisabled={isResolving} /> 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 new file mode 100644 index 0000000000..726364f913 --- /dev/null +++ b/apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.tsx @@ -0,0 +1,168 @@ +// Happy-path FlashList for the Discussion tab (extracted so the tab stays +// under the max-lines cap). Expansion state and settle bookkeeping stay in +// the tab; this file only renders the virtualized list. + +import { FlashList, type FlashListRef } from '@shopify/flash-list'; +import { type RefObject } from 'react'; +import { View } from 'react-native'; + +import { CommentRow } from '@/components/pr-review/discussion/comment-row'; +import { DiscussionThread } from '@/components/pr-review/discussion/discussion-thread'; +import { Button } from '@/components/ui/button'; +import { Text } from '@/components/ui/text'; +import { + type DiscussionListItem, + type ReviewThread, +} from '@/lib/pr-review/discussion/review-discussion-types'; +import { expandedForThread } from '@/lib/pr-review/discussion/thread-expansion'; + +const DISCUSSION_LIST_CONTENT_STYLE = { paddingTop: 12 }; +const noopReactionToggle = () => { + // Conversation comments are read-only (A2.3): no reaction mutations. +}; + +type PrReviewDiscussionListProps = { + readonly owner: string; + readonly repo: string; + readonly number: number; + readonly listItems: readonly DiscussionListItem[]; + readonly listRef: RefObject | null>; + readonly expansion: Record; + readonly suppressContentPosition: boolean; + readonly onToggleExpand: (thread: ReviewThread, index: number) => void; + readonly onScrollBeginDrag: () => void; + readonly hasNextPage: boolean; + readonly isFetchingNextPage: boolean; + readonly laterPageError: boolean; + readonly onLoadMore: () => void; + readonly onRetryLoadMore: () => void; +}; + +export function PrReviewDiscussionList({ + owner, + repo, + number, + listItems, + listRef, + expansion, + suppressContentPosition, + onToggleExpand, + onScrollBeginDrag, + hasNextPage, + isFetchingNextPage, + laterPageError, + onLoadMore, + onRetryLoadMore, +}: Readonly) { + return ( + item.kind} + onScrollBeginDrag={onScrollBeginDrag} + // Stays enabled (load-more inserts rows mid-list); the tab disables it + // only for the exact commit of a deferred expand — see its comment. + maintainVisibleContentPosition={{ disabled: suppressContentPosition }} + renderItem={({ item, index }) => { + if (item.kind === 'comment') { + return ( + + + + + + ); + } + const thread = item.thread; + return ( + + { + onToggleExpand(thread, index); + }} + /> + + ); + }} + contentContainerStyle={DISCUSSION_LIST_CONTENT_STYLE} + keyboardShouldPersistTaps="handled" + automaticallyAdjustKeyboardInsets + ListFooterComponent={ + + } + /> + ); +} + +function keyForItem(item: DiscussionListItem): string { + return item.kind === 'thread' + ? `thread:${item.thread.threadId}` + : `comment:${item.comment.nodeId}`; +} + +type ListFooterProps = { + readonly hasNextPage: boolean; + readonly isFetchingNextPage: boolean; + readonly laterPageError: boolean; + readonly onLoadMore: () => void; + readonly onRetryLoadMore: () => void; +}; + +function ListFooter({ + hasNextPage, + isFetchingNextPage, + laterPageError, + onLoadMore, + onRetryLoadMore, +}: Readonly) { + if (laterPageError) { + return ( + + + Could not load more comments. + + + + ); + } + if (!hasNextPage) { + return ; + } + return ( + + + + ); +} diff --git a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx index 0194f6b6dc..88bc9081b1 100644 --- a/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx @@ -37,13 +37,16 @@ // inside the screen's tab shell and needs a fresh FlatList so the // list can virtualize when a PR has hundreds of threads. (Same // approach as the Files tab.) +// +// Happy-path list lives in discussion/pr-review-discussion-list.tsx +// (max-lines extraction). Expansion settle bookkeeping stays here. -import { FlashList } from '@shopify/flash-list'; +import { type FlashListRef } from '@shopify/flash-list'; import { MessageSquarePlus } from 'lucide-react-native'; -import { View } from 'react-native'; +import { useEffect, useRef, useState } from 'react'; +import { Platform, View } from 'react-native'; -import { CommentRow } from '@/components/pr-review/discussion/comment-row'; -import { DiscussionThread } from '@/components/pr-review/discussion/discussion-thread'; +import { PrReviewDiscussionList } from '@/components/pr-review/discussion/pr-review-discussion-list'; import { PrReviewReconnectNotice } from '@/components/pr-review/pr-review-reconnect-notice'; import { EmptyState } from '@/components/empty-state'; import { QueryError } from '@/components/query-error'; @@ -54,7 +57,15 @@ import { type DiscussionListItem, isDiscussionEmpty, mergeDiscussionListItems, + type ReviewThread, } from '@/lib/pr-review/discussion/review-discussion-types'; +import { + expandedForThread, + expandThread, + seedThreadExpansion, + shouldDeferExpand, + toggleThreadExpanded, +} from '@/lib/pr-review/discussion/thread-expansion'; import { usePrReviewDiscussionThreads } from '@/lib/pr-review/discussion/use-pr-review-discussion-threads'; type PrReviewDiscussionTabProps = { @@ -69,10 +80,6 @@ type PrReviewDiscussionTabProps = { }; const SKELETON_ROW_COUNT = 4; -const DISCUSSION_LIST_CONTENT_STYLE = { paddingTop: 12 }; -const noopReactionToggle = () => { - // Conversation comments are read-only (A2.3): no reaction mutations. -}; export function PrReviewDiscussionTab({ owner, @@ -87,6 +94,109 @@ export function PrReviewDiscussionTab({ number, }); + const [expansion, setExpansion] = useState>({}); + const [suppressContentPosition, setSuppressContentPosition] = useState(false); + const expansionRef = useRef(expansion); + const listRef = useRef>(null); + const settleGenerationRef = useRef(0); + const settleThreadIdRef = useRef(null); + + // Single write path: the ref is the tap-time source of truth (render-closure + // state can lag a queued update on rapid taps). + const applyExpansion = (next: Record) => { + expansionRef.current = next; + setExpansion(next); + }; + + // First-sight seeding: a resolve/unresolve must NOT change expansion (see + // thread-expansion.ts). No-op on the loading branch (threads is []). + useEffect(() => { + const seeded = seedThreadExpansion(expansionRef.current, threads); + if (seeded !== expansionRef.current) { + applyExpansion(seeded); + } + }, [threads]); + + // A data change mid-settle (load-more re-sort, optimistic mutation) cancels the + // settle: the captured scroll index can point at a different row after a re-sort. + // The user taps again; a wrong-row settle is worse. + useEffect(() => { + settleGenerationRef.current += 1; + settleThreadIdRef.current = null; + }, [threads]); + + // Unmount cancels any in-flight settle. + useEffect( + () => () => { + settleGenerationRef.current += 1; + settleThreadIdRef.current = null; + }, + [] + ); + + const invalidateSettle = () => { + settleGenerationRef.current += 1; + settleThreadIdRef.current = null; + }; + + const handleToggleExpand = (thread: ReviewThread, index: number) => { + // Same-thread retap during an in-flight settle: cancel that settle, then run + // THIS tap's path below (cancel + supersede — never two concurrent settles, + // and exactly one expand results, which E2E flow 7a asserts). + if (settleThreadIdRef.current === thread.threadId) { + invalidateSettle(); + } + const expanded = expandedForThread(expansionRef.current, thread.threadId, thread.isResolved); + if (!expanded) { + const layout = listRef.current?.getLayout(index); + const rowTop = layout ? layout.y + (listRef.current?.getFirstItemOffset() ?? 0) : null; + const offset = listRef.current?.getAbsoluteLastScrollOffset() ?? 0; + if (shouldDeferExpand(rowTop, offset)) { + settleGenerationRef.current += 1; + const generation = settleGenerationRef.current; + settleThreadIdRef.current = thread.threadId; + void (async () => { + // Settle target, verified on-device (iOS, calibrated ±1.3pt — E2E r3): + // maintainVisibleContentPosition anchors the first FULLY visible row, and a + // row parked at the visible top edge still loses the anchor to the next row — + // the expansion then scrolls the list by the full expansion height and the + // tapped header flies off screen. viewOffset: +firstItemOffset parks the row + // ~|firstItemOffset| below the viewport top (fully inside the visible region, + // with rows above it visible), so a row above stays the anchor and the + // post-expand adjustment measures ≈ 0; the header never leaves the screen. + const firstItemOffset = listRef.current?.getFirstItemOffset() ?? 0; + await listRef.current?.scrollToIndex({ + index, + viewPosition: 0, + viewOffset: firstItemOffset, + animated: true, + }); + if (settleGenerationRef.current === generation) { + settleThreadIdRef.current = null; + // Android only: suppress maintainVisibleContentPosition for exactly + // the expand commit. Measured across E2E rounds r1-r5: every + // top-clipped flight equals the expansion height — the post-expand + // native mVCP adjustment, not a settle-target error. Android's + // shallower park loses the anchor without this (r4); with it the + // adjustment cannot fire (r5: all Android flows pass). iOS must + // NOT cycle the prop: removing and re-adding native mVCP blanks + // the whole list (r5, deterministic -997949 offset), and iOS's + // deeper park is anchor-safe without suppression (r4: 3/3 pass). + if (Platform.OS === 'android') { + setSuppressContentPosition(true); + } + applyExpansion(expandThread(expansionRef.current, thread.threadId)); + setTimeout(() => { + setSuppressContentPosition(false); + }, 150); + } + })(); + return; + } + } + applyExpansion(toggleThreadExpanded(expansionRef.current, thread.threadId, thread.isResolved)); + }; + // ── First-page error / terminal states ───────────────────────────── if (firstPageErrorState) { if (firstPageErrorState.kind === 'permission') { @@ -171,104 +281,25 @@ export function PrReviewDiscussionTab({ const listItems = mergeDiscussionListItems(threads, conversation); return ( - item.kind} - renderItem={({ item }) => { - if (item.kind === 'comment') { - return ( - - - - - - ); - } - return ( - - - - ); + { + void query.fetchNextPage(); + }} + onRetryLoadMore={() => { + void query.refetch(); }} - contentContainerStyle={DISCUSSION_LIST_CONTENT_STYLE} - keyboardShouldPersistTaps="handled" - automaticallyAdjustKeyboardInsets - ListFooterComponent={ - { - void query.fetchNextPage(); - }} - onRetryLoadMore={() => { - void query.refetch(); - }} - /> - } /> ); } - -function keyForItem(item: DiscussionListItem): string { - return item.kind === 'thread' - ? `thread:${item.thread.threadId}` - : `comment:${item.comment.nodeId}`; -} - -// ── Footer (Load more / error row) ─────────────────────────────────── - -type ListFooterProps = { - readonly hasNextPage: boolean; - readonly isFetchingNextPage: boolean; - readonly laterPageError: boolean; - readonly onLoadMore: () => void; - readonly onRetryLoadMore: () => void; -}; - -function ListFooter({ - hasNextPage, - isFetchingNextPage, - laterPageError, - onLoadMore, - onRetryLoadMore, -}: Readonly) { - if (laterPageError) { - return ( - - - Could not load more comments. - - - - ); - } - if (!hasNextPage) { - return ; - } - return ( - - - - ); -} diff --git a/apps/mobile/src/lib/pr-review/diff/collapse-on-mark-viewed.test.ts b/apps/mobile/src/lib/pr-review/diff/collapse-on-mark-viewed.test.ts new file mode 100644 index 0000000000..1af8e3b62c --- /dev/null +++ b/apps/mobile/src/lib/pr-review/diff/collapse-on-mark-viewed.test.ts @@ -0,0 +1,28 @@ +import { describe, expect, it } from 'vitest'; + +import { collapseOnMarkViewed } from './collapse-on-mark-viewed'; + +describe('collapseOnMarkViewed', () => { + it('collapses an expanded path when marking viewed', () => { + const expanded = { 'a.ts': true, 'b.ts': true }; + expect(collapseOnMarkViewed(expanded, 'a.ts', false)).toEqual({ + 'a.ts': false, + 'b.ts': true, + }); + }); + + it('returns the same reference when un-marking (never re-expands)', () => { + const expanded = { 'a.ts': false, 'b.ts': true }; + expect(collapseOnMarkViewed(expanded, 'a.ts', true)).toBe(expanded); + }); + + it('returns the same reference when path is already collapsed', () => { + const expanded = { 'a.ts': false }; + expect(collapseOnMarkViewed(expanded, 'a.ts', false)).toBe(expanded); + }); + + it('returns the same reference when path is absent (falsy)', () => { + const expanded = { 'b.ts': true }; + expect(collapseOnMarkViewed(expanded, 'a.ts', false)).toBe(expanded); + }); +}); diff --git a/apps/mobile/src/lib/pr-review/diff/collapse-on-mark-viewed.ts b/apps/mobile/src/lib/pr-review/diff/collapse-on-mark-viewed.ts new file mode 100644 index 0000000000..fdf377c7a7 --- /dev/null +++ b/apps/mobile/src/lib/pr-review/diff/collapse-on-mark-viewed.ts @@ -0,0 +1,21 @@ +// Collapse an expanded file row when the user marks it viewed. +// Un-marking never re-expands; no-op branches keep the same reference. + +/** + * Returns updated `expanded` after a mark-viewed toggle. + * `currentlyViewed` is the row's pre-toggle viewed flag — true means this + * tap is an UN-mark and must not re-expand. + */ +export function collapseOnMarkViewed( + expanded: Record, + path: string, + currentlyViewed: boolean +): Record { + if (currentlyViewed) { + return expanded; + } + if (!expanded[path]) { + return expanded; + } + return { ...expanded, [path]: false }; +} diff --git a/apps/mobile/src/lib/pr-review/diff/sticky-file-headers.test.ts b/apps/mobile/src/lib/pr-review/diff/sticky-file-headers.test.ts new file mode 100644 index 0000000000..a0c733c436 --- /dev/null +++ b/apps/mobile/src/lib/pr-review/diff/sticky-file-headers.test.ts @@ -0,0 +1,99 @@ +import { describe, expect, it } from 'vitest'; + +import { buildItems } from '@/lib/pr-review/diff/pr-diff-list-builder'; +import { type BuildItemsArgs } from '@/lib/pr-review/diff/pr-diff-list-items'; +import { type PrReviewFile } from '@/lib/pr-review/diff/pr-review-file-types'; +import { stickyFileHeaderIndices } from '@/lib/pr-review/diff/sticky-file-headers'; + +function makeFile(path: string, patch: string | null): PrReviewFile { + return { + path, + previousPath: null, + status: 'modified', + additions: patch ? 1 : 0, + deletions: patch ? 1 : 0, + patch, + patchMissing: patch === null, + }; +} + +function baseArgs(overrides: Partial = {}): BuildItemsArgs { + return { + files: [], + expanded: {}, + expandedContext: {}, + viewed: () => false, + headSha: 'abc', + owner: 'owner', + repo: 'repo', + number: 1, + changedFiles: 0, + isLoading: false, + isFetchingNextPage: false, + hasNextPage: false, + laterPageError: false, + fetchToCompletionRunning: false, + fetchToCompletionLoaded: 0, + totalFiles: null, + ...overrides, + }; +} + +const smallPatch = [ + 'diff --git a/a.ts b/a.ts', + '@@ -1,2 +1,2 @@', + ' line one', + '-old', + '+new', +].join('\n'); + +describe('stickyFileHeaderIndices', () => { + it('returns empty for empty input', () => { + expect(stickyFileHeaderIndices([])).toEqual([]); + }); + + it('returns indices of expanded and collapsed file headers', () => { + const files = [ + makeFile('expanded.ts', smallPatch), + makeFile('collapsed-a.ts', smallPatch), + makeFile('collapsed-b.ts', null), + ]; + const items = buildItems( + baseArgs({ + files, + expanded: { 'expanded.ts': true }, + changedFiles: files.length, + totalFiles: files.length, + }) + ); + const headerIndices = items + .map((item, index) => (item.kind === 'file-header' ? index : -1)) + .filter(index => index >= 0); + + expect(stickyFileHeaderIndices(items)).toEqual(headerIndices); + expect(headerIndices.length).toBe(3); + // Expanded file contributes more than its header alone. + expect(items.some(item => item.kind === 'diff-line' || item.kind === 'hunk-header')).toBe(true); + }); + + it('shifts indices when a truncation banner leads the list', () => { + // Banner when changedFiles exceeds GitHub's 3000-file list cap. + const files = [makeFile('a.ts', null), makeFile('b.ts', null)]; + const items = buildItems( + baseArgs({ + files, + changedFiles: 3001, + totalFiles: 3001, + }) + ); + + expect(items[0]?.kind).toBe('truncation-banner'); + const indices = stickyFileHeaderIndices(items); + expect(indices[0]).toBe(1); + expect(indices).toEqual( + items + .map((item, index) => (item.kind === 'file-header' ? index : -1)) + .filter(index => index >= 0) + ); + }); +}); diff --git a/apps/mobile/src/lib/pr-review/diff/sticky-file-headers.ts b/apps/mobile/src/lib/pr-review/diff/sticky-file-headers.ts new file mode 100644 index 0000000000..a02c6944b9 --- /dev/null +++ b/apps/mobile/src/lib/pr-review/diff/sticky-file-headers.ts @@ -0,0 +1,12 @@ +import { type ListItem } from '@/lib/pr-review/diff/pr-diff-list-items'; + +/** Layout indices of every `file-header` row for FlashList `stickyHeaderIndices`. */ +export function stickyFileHeaderIndices(items: readonly ListItem[]): number[] { + const indices: number[] = []; + for (let i = 0; i < items.length; i += 1) { + if (items[i]?.kind === 'file-header') { + indices.push(i); + } + } + return indices; +} diff --git a/apps/mobile/src/lib/pr-review/discussion/thread-expansion.test.ts b/apps/mobile/src/lib/pr-review/discussion/thread-expansion.test.ts new file mode 100644 index 0000000000..9b48dd2a90 --- /dev/null +++ b/apps/mobile/src/lib/pr-review/discussion/thread-expansion.test.ts @@ -0,0 +1,103 @@ +import { describe, expect, it } from 'vitest'; + +import { + expandedForThread, + expandThread, + seedThreadExpansion, + shouldDeferExpand, + toggleThreadExpanded, +} from './thread-expansion'; + +describe('thread-expansion', () => { + describe('expandedForThread', () => { + it('defaults unresolved threads to expanded', () => { + expect(expandedForThread({}, 't1', false)).toBe(true); + }); + + it('defaults resolved threads to collapsed', () => { + expect(expandedForThread({}, 't1', true)).toBe(false); + }); + + it('prefers an explicit stored value over the resolution default', () => { + expect(expandedForThread({ t1: true }, 't1', true)).toBe(true); + expect(expandedForThread({ t1: false }, 't1', false)).toBe(false); + }); + }); + + describe('seedThreadExpansion', () => { + it('adds missing threads with first-sight defaults', () => { + const seeded = seedThreadExpansion({}, [ + { threadId: 'a', isResolved: false }, + { threadId: 'b', isResolved: true }, + ]); + expect(seeded).toEqual({ a: true, b: false }); + }); + + it('preserves explicit entries and only fills gaps', () => { + const state = { a: false }; + const seeded = seedThreadExpansion(state, [ + { threadId: 'a', isResolved: false }, + { threadId: 'b', isResolved: true }, + ]); + expect(seeded).toEqual({ a: false, b: false }); + expect(seeded).not.toBe(state); + }); + + it('returns the same reference when nothing is new', () => { + const state = { a: true, b: false }; + const seeded = seedThreadExpansion(state, [ + { threadId: 'a', isResolved: false }, + { threadId: 'b', isResolved: true }, + ]); + expect(seeded).toBe(state); + }); + + it('does not change a seeded entry when resolution flips (first-sight)', () => { + let state = seedThreadExpansion({}, [{ threadId: 'a', isResolved: false }]); + expect(state).toEqual({ a: true }); + // Thread becomes resolved; re-seed must keep the open entry. + state = seedThreadExpansion(state, [{ threadId: 'a', isResolved: true }]); + expect(state).toEqual({ a: true }); + expect(expandedForThread(state, 'a', true)).toBe(true); + }); + }); + + describe('toggleThreadExpanded', () => { + it('flips the effective value and stores it explicitly', () => { + expect(toggleThreadExpanded({}, 't1', true)).toEqual({ t1: true }); + expect(toggleThreadExpanded({}, 't1', false)).toEqual({ t1: false }); + expect(toggleThreadExpanded({ t1: true }, 't1', true)).toEqual({ t1: false }); + }); + }); + + describe('expandThread', () => { + it('sets the thread expanded', () => { + expect(expandThread({}, 't1')).toEqual({ t1: true }); + expect(expandThread({ t1: false }, 't1')).toEqual({ t1: true }); + }); + + it('returns the same reference when already true', () => { + const state = { t1: true }; + expect(expandThread(state, 't1')).toBe(state); + }); + }); + + describe('shouldDeferExpand', () => { + it('defers when layout is unknown (null)', () => { + expect(shouldDeferExpand(null, 0)).toBe(true); + expect(shouldDeferExpand(null, 100)).toBe(true); + }); + + it('defers when the row top is scrolled above the viewport', () => { + expect(shouldDeferExpand(50, 80)).toBe(true); + }); + + it('does not defer when exactly at the top boundary (strict >)', () => { + expect(shouldDeferExpand(100, 100)).toBe(false); + }); + + it('does not defer when fully visible (scroll below row top)', () => { + expect(shouldDeferExpand(100, 40)).toBe(false); + }); + }); +}); diff --git a/apps/mobile/src/lib/pr-review/discussion/thread-expansion.ts b/apps/mobile/src/lib/pr-review/discussion/thread-expansion.ts new file mode 100644 index 0000000000..8492819db0 --- /dev/null +++ b/apps/mobile/src/lib/pr-review/discussion/thread-expansion.ts @@ -0,0 +1,76 @@ +// Pure helpers for discussion-thread expand/collapse state. +// +// Expansion is owned by the Discussion tab (keyed by threadId) so FlashList +// recycling cannot leak open/closed state across rows, and so a single path +// can gate expand behind a short scroll settle when the tapped row is +// top-clipped (maintainVisibleContentPosition anchors on the first fully +// visible row and would otherwise jump the header off-screen). +// +// First-sight seeding preserves mount-time defaults: resolving a thread does +// NOT auto-collapse it (matches today's useState(!isResolved) behavior). + +export function expandedForThread( + state: Record, + threadId: string, + isResolved: boolean +): boolean { + return state[threadId] ?? !isResolved; +} + +/** + * Seed `!isResolved` for previously-unseen threadIds. Preserves every existing + * entry. Returns the same reference when nothing is new. + */ +export function seedThreadExpansion( + state: Record, + threads: readonly { readonly threadId: string; readonly isResolved: boolean }[] +): Record { + let next: Record | null = null; + for (const thread of threads) { + if (!Object.hasOwn(state, thread.threadId)) { + next ??= { ...state }; + next[thread.threadId] = !thread.isResolved; + } + } + return next ?? state; +} + +/** Flip the effective expanded value and store it explicitly. */ +export function toggleThreadExpanded( + state: Record, + threadId: string, + isResolved: boolean +): Record { + const current = expandedForThread(state, threadId, isResolved); + return { ...state, [threadId]: !current }; +} + +/** Force expand (deferred settle path). Same reference if already true. */ +export function expandThread( + state: Record, + threadId: string +): Record { + if (state[threadId] === true) { + return state; + } + return { ...state, [threadId]: true }; +} + +/** + * Whether expand must wait for a scroll settle before the row grows. + * + * | condition | result | + * |---|---| + * | null layout | defer (unknown geometry) | + * | absoluteScrollOffset > rowTopContentOffset | defer (top-clipped) | + * | otherwise | expand directly | + */ +export function shouldDeferExpand( + rowTopContentOffset: number | null, + absoluteScrollOffset: number +): boolean { + if (rowTopContentOffset === null) { + return true; + } + return absoluteScrollOffset > rowTopContentOffset; +} diff --git a/apps/mobile/src/lib/pr-review/discussion/use-pr-review-discussion-threads.ts b/apps/mobile/src/lib/pr-review/discussion/use-pr-review-discussion-threads.ts index 15f635f32d..e7b8a88b0d 100644 --- a/apps/mobile/src/lib/pr-review/discussion/use-pr-review-discussion-threads.ts +++ b/apps/mobile/src/lib/pr-review/discussion/use-pr-review-discussion-threads.ts @@ -18,6 +18,7 @@ // only — simpler than flatten-then-dedupe and matches the guarantee. import { useInfiniteQuery } from '@tanstack/react-query'; +import { useMemo } from 'react'; import { classifyPrReviewQueryState } from '@/lib/pr-review/classify-pr-review-query-state'; import { useTRPC } from '@/lib/trpc'; @@ -50,7 +51,10 @@ export function usePrReviewDiscussionThreads(args: { // Flat list of all threads across all loaded pages, in page order. // Ordering for display is applied by `mergeDiscussionListItems` in // the tab (full re-sort of the entire loaded set). - const threads = (query.data?.pages ?? []).flatMap(page => page.threads); + // Memoized so identity changes only when page data changes (RQ + // structural sharing keeps `pages` stable across unrelated re-renders). + const pages = query.data?.pages; + const threads = useMemo(() => (pages ?? []).flatMap(page => page.threads), [pages]); // First page only — backend guarantees later pages return []. const conversation = query.data?.pages[0]?.conversation ?? [];