Repository navigation
perf(mobile): memoize PR review merges and share comment moderation mutations - #6587
Conversation
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Executive SummaryNo new defects found in this 17-file perf refactor (memoized inbox/discussion merges, shared comment-moderation provider, cached parsed diff patches, identity-stable per-line tap callbacks); the previously raised per-line map-retention WARNING is fixed by the new Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (17 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit b6863d0)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit b6863d0)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (17 files)
Verified: the decorated merges are order-equivalent to the originals (tie/missing-timestamp cases delegate to the shared comparator), the Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0 Review guidance: REVIEW.md from base branch |
4d13641 to
e38bdc2
Compare
|
bot: rejected: a suggestion, not a defect. A typed review read this finding as needing no code change before merge; the code stays as is. Say so here if one of them does change behaviour and it will be repaired.
|
|
This description names a scenario the proof did not capture:
A repeated proof run rebuilds the same evidence, so no proof run is dispatched for a named gap. Merging with this gap open is your decision. |
Changelog for users
Changelog for maintainers
src/lib/pr-review/use-provider-inbox.ts:303— accepted: the merge ran in the hook body and parsed both operands per comparison; it now runs in auseMemoover the query/source values and parses each row'supdatedAtMsonce before sorting.src/components/pr-review/pr-review-discussion-tab.tsx:401— accepted: the render-body merge is replaced by auseMemoovermergeDiscussionListItemsBySortKey, with a shared empty-conversation reference so the memo key stays stable when a PR has no conversation comments.src/lib/pr-review/diff/pr-diff-list-builder.ts:118— accepted:parsePatchnow runs through aWeakMapkeyed on the file object, so aviewed/expandedchange reuses the parsed patch and itsParsedFileidentity.src/components/pr-review/diff/pr-diff-file-list-render.tsx:130— accepted:useDiffRenderItembinds one identity-stableonTapper line key through refs, soDiffLine's reference comparison can hit; the callback reads the current item andonLineTapthrough refs.src/components/pr-review/discussion/comment-row.tsx:141— accepted: the four per-row moderation mutations moved into one provider; each row calls a singlereport({ action, ... })dispatcher, so N rows hold one set of four mutation listeners instead of 4N.CommentModerationProvideris required;useCommentModerationActionsthrows without it, so everyCommentRowmust render under the provider.moderationFailuremoved fromcomment-row.tsxto the newcomment-moderation.tsx;mergeDiscussionListItemsBySortKeydelegates exact ties to the shared comparator to preserve the existing order.E2E proof
[e7] ux-check: Diff tab: with several files expanded, expand one more — only that file's rows appear; previously expanded files and the scroll position are preserved. — android emulator-5554, real PR Kilo-Org/kilocode#14434 (8 files): with SessionUi.kt and .changeset/… expanded, expanding EditorContextGatherer.kt adds only its rows (e7-expanded3.txt: 'Added line 41: … The selection marker …' [0,1650][1080,1805] under 'Collapse file' [37,1059][942,1193]) while SessionUi.kt's rows stay put ('Context line 963 …' [0,452][1080,520]); scroll is preserved — the same anchor row is at [0,452][1080,520] in both e7-before.log and e7-scene.log while the row below only flips 'Expand file' → 'Collapse file' at [37,1059][942,1193]; captures…
[e4] ux-check: Diff tab: with several files expanded, mark one file viewed — android emulator-5554, real PR #6543 (4 files): e4-mark1.txt shows the marked file collapsed+viewed ('Unmark apps/web/src/lib/ai-gateway/providers/openrouter/inference-provider-id.ts as viewed' [961,505][1043,588]) while mapModelIdToVercel.test.ts stays expanded ('Collapse file' [37,672][942,806]) with rows painted ('Hunk header @@ -174,8 +174,8 @@' [0,910][1080,970], 'Context line 174' [0,970][1080,1125], 'Context line 181' [0,1691][1080,1750]) and the count moves to 'Files · 1 of 4 viewed' [105,394][352,431]; the offset does not jump (two mark toggles give identical bounds in…
[e9] ux-check: Discussion tab: react to / resolve a comment — the change applies without reordering the whole list or moving the scroll position. — android emulator-5606, PR Kilo-Org/kilocode#14219 Discussion tab (7 items) scrolled to the iscekic thread: adding a reaction and resolving the thread changed only the chip and the status label (diff before->after-react '10a11,12 > reaction/1'; after-react->after-resolve '15a16 > RESOLVED'/'17d17 < FILE'), with every other visible item and the diff context lines in identical order, so no reordering and no scroll move; 'SCENE e9 OK'. Zero UX-DEFECTs on the Discussion tab. Stills captured for the visual reviewer: e9-before.png, e9-after-react.png, e9-after-resolve.png, e9.png.
[e9] ux-check: Discussion tab: react to / resolve a comment — the change applies without reordering the whole list or moving the scroll position.
[e1] Discussion tab of the PR — comment moderation sheet — android emulator-5554; start state pr-review (STATE HIT pr-review). The parked screen anchor '/Files, tab, N of M' is an iOS-format digest this build never renders (android digest shows the tab as 'Discussion, 3'), so I edited the e1 entry in e2e-scripts.json to '/Submit review|Discussion, [0-9]+/' and re-ran: e1-run.log line 1 'SCENE e1 OK'. Loaded thread list renders (e1-run.log: 'android.view.ViewGroup Discussion thread apps/mobile/app.config.ts L68 (RIGHT) tappable [37,1606][1043,2152]'). Comment actions opened the sheet with all four options (e1-sheet.log: 'android.widget.TextView Report…
[e1] Discussion tab of the PR — comment moderation sheet
E2E proof — log excerpts
/home/igor_kilocode_ai/.local/share/kwf/sections/mobile-perf-pr-review-cost-9b8f/e2e-mobile-app/e1-run.log/home/igor_kilocode_ai/.local/share/kwf/sections/mobile-perf-pr-review-cost-9b8f/e2e-mobile-app/e1-sheet.log/home/igor_kilocode_ai/.local/share/kwf/sections/mobile-perf-pr-review-cost-9b8f/e2e-mobile-app/e1-moderation-db.log/home/igor_kilocode_ai/.local/share/kwf/sections/mobile-perf-pr-review-cost-9b8f/e2e-mobile-app/e1-nextjs-after-cancel.log/home/igor_kilocode_ai/.local/share/kwf/sections/mobile-perf-pr-review-cost-9b8f/e2e-mobile-app/e2-script-run3.log/home/igor_kilocode_ai/.local/share/kwf/sections/mobile-perf-pr-review-cost-9b8f/e2e-mobile-app/e2-interaction-delta.log/home/igor_kilocode_ai/.local/share/kwf/sections/mobile-perf-pr-review-cost-9b8f/e2e-mobile-app/e2-parse.log/home/igor_kilocode_ai/.local/share/kwf/sections/mobile-perf-pr-review-cost-9b8f/e2e-mobile-app/e3-behavior.log/home/igor_kilocode_ai/.local/share/kwf/sections/mobile-perf-pr-review-cost-9b8f/e2e-mobile-app/e3sel-scene.log/home/igor_kilocode_ai/.local/share/kwf/sections/mobile-perf-pr-review-cost-9b8f/e2e-mobile-app/e3-scene.logOwner request
[e8] ux-check: Discussion tab, PR with many threads: expand then collapse a thread — it toggles in place, list order is unchanged, scroll position is retained, and no row blanks or flickers. — Android emulator-5604, Kilo-Org/kilocode#14219 (7 threads): the thread card toggles with its top edge fixed at y=1705 (Button [37,1705][1043,1940] collapsed vs ViewGroup [37,1705][1043,2152] + Collapse thread [72,1769] + Quoted diff snippet [72,1933] expanded), rows above keep identical bounds in all three digests, and the e8-expanded assert text lay below the captured viewport while its digest shows the re-expanded state; no UX-DEFECT seen in the digest (screenshots captured for the visual reviewer).
[e11] ux-check: Comment overflow: tap the ⋯ control — the native action sheet lists Report content, Report user, Mute, Block and Cancel; Report content shows the success toast with a receipt id; choosing… — android emulator-5604, #5895 Discussion tab; one scripted run asserted the sheet's Report content / Report user / Mute / Block / Cancel, then the toast assert "Report submitted. Receipt" (e11-run.log: "SCENE e11 OK"), backed by the server row 408f2a17-33f4-4544-8014-6e649cef8bfd|pr_discussion_content|comment|5549379252|other|{"platform": "mobile"} (e11-moderation-report-row.log). Block hid the author: final digest has 0 hits for kilo-code-bot and the summaries read "1 comment · started 16 days ago" where they read 2 before (e11-run.log), with the block row…
[e10] Discussion tab on mount (android emulator-5554) — Scene e10 ran OK: Home -> PR Review -> Kilo-Org/kilocode#14219 -> Discussion mounted the 7-item list with a comment row (vercel, 'Fix with Kilo', 'Comment actions'), proving the shared CommentModerationProvider is present (a row without it throws), with no red screen or error text (e10-discussion.log); no UX-DEFECT in the digest, capture e10.png is for the visual reviewer.
[e11] ux-check: Comment overflow: tap the ⋯ control — the native action sheet lists Report content, Report user, Mute, Block and Cancel; Report content shows the success toast with a receipt id; choosing…
[e8] ux-check: Discussion tab, PR with many threads: expand then collapse a thread — it toggles in place, list order is unchanged, scroll position is retained, and no row blanks or flickers.
[e5] ux-check: Diff tab, large PR: tap one diff line — only that line (and the selection range) shows the left selection ring, the action bar reflects the tapped line, and the rest of the mounted lines do… — android emulator-5604, #5895 (195 files), Files tab; one line tapped -> only it is selected (content-desc "Selected Added line 12: {, tap to change selection") and the action bar names the tapped line (text "apps/mobile/drizzle/meta/_journal.json RIGHT L12"); neighbours remain mounted (text "+ 13"); an earlier second tap widened the bar to RIGHT L1–L2. No UX-DEFECT observed. Captures: e5-line-selected.png, e5-diff-line-tapped.png. Replay e5.replay.json validated (SCENE e5 OK).
[e7] ux-check: Diff tab: with several files expanded, expand one more — only that file's rows appear; previously expanded files and the scroll position are preserved. — android emulator-5554, real PR Kilo-Org/kilocode#14434 (8 files): with SessionUi.kt and .changeset/… expanded, expanding EditorContextGatherer.kt adds only its rows (e7-expanded3.txt: 'Added line 41: … The selection marker …' [0,1650][1080,1805] under 'Collapse file' [37,1059][942,1193]) while SessionUi.kt's rows stay put ('Context line 963 …' [0,452][1080,520]); scroll is preserved — the same anchor row is at [0,452][1080,520] in both e7-before.log and e7-scene.log while the row below only flips 'Expand file' → 'Collapse file' at [37,1059][942,1193]; captures…
[e2] PR-review entry/inbox (android emulator-5554) — Scene e2 (amended step list, anchored /^PR Review$/) ran OK: from signed-in Home the PR Review row opened the PR-review screen whose INBOX rendered three review-request rows (Kilo-Org/kilocode#14219, Kilo-Org/kilocode#14198, #6401) and whose RECENT section showed 'No recent reviews' (e2-inbox.log); the one-parse-per-row and stable-array behaviour is asserted by use-provider-inbox.test.ts and use-provider-inbox.mounted.test.tsx, run by the driver; no UX-DEFECT in the settled digest, capture e2.png is for the visual reviewer.
Follow-ups (not changed here)