Skip to content

fix(mobile): polish pull request review UX papercuts - #6343

Open
iscekic wants to merge 1 commit into
mainfrom
kwf/owner-pr-review-ux-papercuts-d600
Open

iscekic wants to merge 1 commit into
mainfrom
kwf/owner-pr-review-ux-papercuts-d600

Conversation

@iscekic

@iscekic iscekic commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator

Changelog for users

  • The Files tab summary row now has space above and below, clear of the tab selector and the first file path.
  • The "Finish review" card now sits just above the device safe area instead of above a wide blank band.
  • The pull request header shows a Merge button while the pull request is mergeable; tapping it opens the merge confirmation sheet.
  • The header's Share, Submit review, and Merge actions are now icon buttons, leaving room for the title.
  • The conversation header no longer shows a Copy link button.
  • The context details sheet now has a Copy link row; tapping it copies the session's resume link and shows the result inline.

Changelog for maintainers

  • The Files tab summary container's vertical padding changed from py-2 to py-4; the gutter, background, and hairline are unchanged.
  • The Finish review footer's bottom padding changed from 24 + insets.bottom to Math.max(insets.bottom, 8); landscape side insets are untouched.
  • Share and Submit review are now Button size="icon" variant="ghost"; Submit review still renders only on the Overview tab.
  • The Merge button appears only for mergeable pull requests and mirrors the Overview merge section's gate, opening the same sheet.
  • The context sheet now requires anchorMessageId; both copy rows share an extracted CopyRow and a useCopyRowFeedback hook guarded by a generation counter.
  • copySessionLink was added to the session row actions, and session-copy-link-action.tsx was deleted, so the loading header no longer reserves that control.
  • No catalog keys were added; the change reuses common.copyLink, prReview.merge.mergeNow, agentChat.chatLink.linkCopied, and agentChat.chatLink.couldNotCopyLink.
  • Review first: header title behavior between loading and loaded states, and merge-gate parity between the header and the Overview merge section.

E2E proof

[e1] Four PR-screen papercuts: Files-tab summary padding, Finish-review footer spacing, header Merge CTA (icon buttons), Copy link moved into the context details sheet — e2e-mobile-app/e1-merge.png

[e1] Four PR-screen papercuts: Files-tab summary padding, Finish-review footer spacing, header Merge CTA (icon buttons), Copy link moved into the context details sheet — e2e-mobile-app/e1-header.png

[e1] Four PR-screen papercuts: Files-tab summary padding, Finish-review footer s -> pass :: android emulator-5554 on the packed tree: header shows Share/Submit review/Merge now icon buttons [679,149][795,265]/[804,149][920,265]/[928,149][1043,265] and tapping Merge lands on 'Merge pull request' (e1-merge OK, text absent from every Overview tree); Files summary 'Files · 0 of 2 viewed' [105,459][352,496] sits between the tab selector (ends y 414) and 'src/alpha.ts' (starts y 545) while 'Finish review' [67,2191][1013,2307] rests 93 px above the 2400 px screen edge; the conversation header has only the context pill (e1-session-probe.xml) and the sheet's COPY LINK row shows LINK COPIED only after the tap (absent→tap→present in e1-context.replay.json). Padding/dead-space appearance and t
/home/igor_kilocode_ai/.local/share/kwf/sections/kwf-fix-proof-c3296af-ac0d/e2e-mobile-app/e1-files-scenes2.log
android.view.View Overview tappable [46,329][369,414]
android.widget.TextView Overview tappable [140,348][276,394]
android.view.View Files tappable [379,329][703,414]
android.widget.TextView Files tappable [506,348][576,394]
android.view.View Discussion, 9 tappable [711,329][1034,414]
android.widget.TextView Discussion tappable [762,348][924,394]
android.widget.TextView 9 tappable [951,351][970,390]
android.widget.Button Expand file tappable [37,545][942,633]
android.widget.TextView src/alpha.ts tappable [175,545][942,591]
android.widget.TextView Modified tappable [175,596][286,633]
android.widget.TextView +6 tappable [304,596][336,633]
android.widget.TextView -1 tappable [354,596][380,633]
android.widget.CheckBox Mark src/alpha.ts as viewed tappable [961,547][1043,630]
android.widget.Button Expand file tappable [37,691][942,779]
android.widget.TextView src/beta.ts tappable [175,691][942,737]
android.widget.TextView Modified tappable [175,742][286,779]
android.widget.TextView +4 tappable [304,742][336,779]
android.widget.TextView -1 tappable [354,742][380,779]
android.widget.CheckBox Mark src/beta.ts as viewed tappable [961,693][1043,776]
android.widget.TextView 2 files loaded of 2 tappable [453,845][676,882]
android.widget.Button Open file navigator tappable [37,437][371,519]
android.widget.TextView Files · 0 of 2 viewed tappable [105,459][352,496]
android.widget.Button Finish review tappable [67,2191][1013,2307]
android.widget.TextView Finish review tappable [443,2225][637,2271]
/home/igor_kilocode_ai/.local/share/kwf/sections/kwf-fix-proof-c3296af-ac0d/e2e-mobile-app/e1-header.log
android.widget.TextView Mixed discussion fixture tappable [37,569][1045,647]
android.widget.TextView alice tappable [110,680][180,726]
android.widget.TextView feature/stub tappable [92,758][344,805]
android.widget.TextView ← tappable [362,758][395,804]
android.widget.TextView main tappable [413,758][497,805]
android.widget.TextView 1 commit tappable [87,833][227,879]
android.widget.TextView 2 files tappable [314,833][404,879]
android.widget.TextView 10 tappable [492,833][530,879]
android.widget.TextView / −2 tappable [544,833][603,879]
android.widget.TextView Opened 6 months ago · Updated 2 hours ago tappable [37,925][1045,971]
android.widget.TextView LABELS tappable [37,1008][138,1045]
android.widget.TextView bug tappable [59,1072][107,1109]
android.widget.TextView documentation tappable [167,1072][357,1109]
android.widget.TextView good first issue tappable [416,1072][610,1109]
android.widget.TextView REVIEWERS tappable [82,1155][234,1192]
android.widget.TextView carol tappable [110,1214][850,1260]
android.widget.TextView Approved tappable [923,1219][1043,1256]
android.widget.TextView dave tappable [110,1288][727,1334]
android.widget.TextView Changes requested tappable [800,1293][1043,1330]
android.widget.TextView erin tappable [110,1361][816,1407]
android.widget.TextView Commented tappable [889,1366][1043,1403]
android.widget.TextView frank tappable [110,1435][772,1481]
android.widget.TextView Awaiting review tappable [845,1440][1043,1477]
android.widget.TextView ASSIGNEES tappable [82,1523][234,1560]
/home/igor_kilocode_ai/.local/share/kwf/sections/kwf-fix-proof-c3296af-ac0d/e2e-mobile-app/e1-merge-scene.log
android.widget.TextView Mixed discussion fixture tappable [37,569][1045,647]
android.widget.TextView alice tappable [110,680][180,726]
android.widget.TextView feature/stub tappable [92,758][344,805]
android.widget.TextView ← tappable [362,758][395,804]
android.widget.TextView main tappable [413,758][497,805]
android.widget.TextView 1 commit tappable [87,833][227,879]
android.widget.TextView 2 files tappable [314,833][404,879]
android.widget.TextView 10 tappable [492,833][530,879]
android.widget.TextView / −2 tappable [544,833][603,879]
android.widget.TextView Opened 6 months ago · Updated 2 hours ago tappable [37,925][1045,971]
android.widget.TextView LABELS tappable [37,1008][138,1045]
android.widget.TextView bug tappable [59,1072][107,1109]
android.widget.TextView documentation tappable [167,1072][357,1109]
android.widget.TextView good first issue tappable [416,1072][610,1109]
android.widget.TextView REVIEWERS tappable [82,1155][234,1192]
android.widget.TextView carol tappable [110,1214][850,1260]
android.widget.TextView Approved tappable [923,1219][1043,1256]
android.widget.TextView dave tappable [110,1288][727,1334]
android.widget.TextView Changes requested tappable [800,1293][1043,1330]
android.widget.TextView erin tappable [110,1361][816,1407]
android.widget.TextView Commented tappable [889,1366][1043,1403]
android.widget.TextView frank tappable [110,1435][772,1481]
android.widget.TextView Awaiting review tappable [845,1440][1043,1477]
android.widget.TextView ASSIGNEES tappable [82,1523][234,1560]
/home/igor_kilocode_ai/.local/share/kwf/sections/kwf-fix-proof-c3296af-ac0d/e2e-mobile-app/e1-context-scene.log
android.widget.TextView Search for copy link and context sheet tappable [128,571][692,617]
android.view.ViewGroup Assistant message tappable [0,647][1080,957]
android.widget.Button Thought tappable [40,659][1042,733]
android.widget.TextView THOUGHT tappable [68,677][206,714]
android.widget.Button session-context-sheet.tsx tool, completed tappable [40,756][1042,839]
android.widget.TextView session-context-sheet.tsx tappable [128,774][511,820]
android.widget.Button Find copy link usages in context sheet tool, completed tappable [40,863][1042,946]
android.widget.TextView Find copy link usages in context sheet tappable [128,881][690,927]
android.view.ViewGroup Assistant message tappable [0,957][1080,1274]
android.widget.Button Thought tappable [40,969][1042,1043]
android.widget.TextView THOUGHT tappable [68,987][206,1024]
android.widget.Button Inspect recent commits and status tool, running tappable [40,1066][1042,1156]
android.widget.TextView Inspect recent commits and status tappable [138,1088][650,1134]
android.widget.Button Find copy link tests in mounted test tool, completed tappable [40,1179][1042,1262]
android.widget.TextView Find copy link tests in mounted test tappable [128,1197][653,1243]
android.widget.TextView Running commands · 18 min, 15 sec tappable [107,2082][642,2128]
android.widget.TextView Permission required tappable [76,1451][1004,1497]
android.widget.TextView The agent is waiting for permission. While this request is open, your message to the agent is paused. tappable [76,1506][1004,1580]
android.widget.TextView Allow Bash? tappable [76,1647][1004,1693]
android.widget.TextView Applies to: tappable [95,1739][986,1776]
android.widget.TextView • git log --oneline -15 tappable [95,1785][986,1822]
android.widget.TextView • echo --- tappable [95,1831][986,1868]
## Follow-ups (not changed here)

Open findings (not fixed here)

  • r the next round (.kwf-keep-device)
    mobile-device: verifier passed
    mobile-device: spot skips 2 still(s) of missed scripted scene(s)
    mobile-device: spot check clean
    mobile-device: signed-in app data restored on emulator-5554
    mobile-device: signed-in app data frozen on emulator-5554 (66547200 bytes)
  • the '## E2E proof' section carries no log excerpt, so nothing shows the change was driven end to end

e1-merge

e1-header

@iscekic
iscekic marked this pull request as draft September 19, 2026 05:35
Comment thread apps/mobile/src/app/(app)/agent-chat/[session-id].tsx Outdated
@kilo-code-bot

kilo-code-bot Bot commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

The rebased mobile UX-papercut change holds together: the loading header comment now matches the loaded right cluster, the new copy-link row and its generation-guarded inline feedback are sound, and the header Merge CTA mirrors the Overview merge gate. The previously raised warning is resolved.

Files Reviewed (15 files)
  • apps/mobile/src/app/(app)/agent-chat/[session-id].mounted.test.tsx
  • apps/mobile/src/app/(app)/agent-chat/[session-id].tsx
  • apps/mobile/src/components/agents/session-context-sheet.mounted.test.tsx
  • apps/mobile/src/components/agents/session-context-sheet.tsx
  • apps/mobile/src/components/agents/session-copy-link-action.tsx (deleted)
  • apps/mobile/src/components/agents/session-detail-content.test.ts
  • apps/mobile/src/components/agents/session-detail-content.tsx
  • apps/mobile/src/components/agents/session-row-actions.test.ts
  • apps/mobile/src/components/agents/session-row-actions.ts
  • apps/mobile/src/components/pr-review/diff/pr-diff-file-list-header.test.tsx
  • apps/mobile/src/components/pr-review/diff/pr-diff-file-list-header.tsx
  • apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.test.tsx
  • apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.tsx
  • apps/mobile/src/components/pr-review/pr-review-screen.test.tsx
  • apps/mobile/src/components/pr-review/pr-review-screen.tsx
Previous Review Summary (commit 6f6b98e)

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

Previous review (commit 6f6b98e)

Status: 1 Issue Found | Recommendation: Address before merge

Executive Summary

The loading state of the session screen loses its only copy-link affordance because the sheet it moved into is not mounted there, and the replacement comment misdescribes the loaded header's right cluster.

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
apps/mobile/src/app/(app)/agent-chat/[session-id].tsx 156 Loading header no longer offers a way to copy the session link; comment claims the pill is the loaded header's only right-cluster control though SessionPrBadge also renders there.
Files Reviewed (15 files)
  • apps/mobile/src/app/(app)/agent-chat/[session-id].mounted.test.tsx
  • apps/mobile/src/app/(app)/agent-chat/[session-id].tsx - 1 issue
  • apps/mobile/src/components/agents/session-context-sheet.mounted.test.tsx
  • apps/mobile/src/components/agents/session-context-sheet.tsx
  • apps/mobile/src/components/agents/session-copy-link-action.tsx (deleted)
  • apps/mobile/src/components/agents/session-detail-content.test.ts
  • apps/mobile/src/components/agents/session-detail-content.tsx
  • apps/mobile/src/components/agents/session-row-actions.test.ts
  • apps/mobile/src/components/agents/session-row-actions.ts
  • apps/mobile/src/components/pr-review/diff/pr-diff-file-list-header.test.tsx
  • apps/mobile/src/components/pr-review/diff/pr-diff-file-list-header.tsx
  • apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.test.tsx
  • apps/mobile/src/components/pr-review/diff/pr-diff-floating-actions.tsx
  • apps/mobile/src/components/pr-review/pr-review-screen.test.tsx
  • apps/mobile/src/components/pr-review/pr-review-screen.tsx

Fix these issues in Kilo Cloud


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

Review guidance: REVIEW.md from base branch main

@iscekic

iscekic commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Rejected, no code change (kwf kwf-fix-platform-7abd).

Why: (already implemented, verified live: no change needed: The requested cross-platform clipboard and success-haptic implementation already exists, and targeted checks found no platform fork. Existing implementation: apps/mobile/src/components/agents/session-row-actions.ts:67 calls Clipboard.setStringAsync and Haptics.notificationAsync wit

@iscekic
iscekic force-pushed the kwf/owner-pr-review-ux-papercuts-d600 branch from d79e5ad to c3296af Compare September 19, 2026 10:17
@iscekic
iscekic marked this pull request as ready for review September 19, 2026 10:26
@iscekic iscekic added the human-ready The PR is ready for human review. label Sep 19, 2026
@iscekic iscekic self-assigned this Sep 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

human-ready The PR is ready for human review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant