feat(mobile): PR review discussion UX + toast entry feedback - #4883
Merged
Conversation
Select diffHunk on each thread's first comment in REVIEW_THREADS_QUERY, carry it through the review-thread DTO (nullable, '' mapped to null), and cover the mapping with mapper and router tests.
Render the thread's diffHunk (from the first comment) above the comments when expanded, parsed via parsePatch and drawn with DiffLine, capped at 30 lines with a trailing count row. File-level and malformed anchors fall back to the label only. Resolve toggle becomes an icon-only button so the Resolved badge is the single resolved indicator.
Build discussion-mixed via buildMixedFixture(idSuffix) registered at pulls 1 (iOS) and 11 (Android) with globally unique ids; add diffHunk strings, reaction-count overrides, outdated/file/resolved threads, and stateful ResolveThread/UnresolveThread/AddReaction/RemoveReaction mutations resolved by scanning fixtures for the unique id.
Render only non-zero reaction buckets as pills and add a single SmilePlus button that opens a bottom-sheet picker with all eight emoji (Portal name pr-review-reactions, backdrop, BackHandler on Android). Split readOnly from disabled so conversation comments show read-only pills with no add icon while pending mutations only lock presses.
Mount one stable Text between the input row and Open button; swap only its copy (message or non-breaking-space placeholder) and color token so helper messages appear and clear without moving surrounding controls.
The review-thread DTO gained a required nullable diffHunk; the shared makeThread helpers now include it so typecheck passes.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of Files Reviewed (1 file)
Previous findings — current state
Notes and assumptions
Previous Review Summaries (3 snapshots, latest commit 0bc3f1b)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 0bc3f1b)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryIncremental review of Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files)
Previous findings — current state
Notes and assumptions
Fix these issues in Kilo Cloud Previous review (commit 47ebec3)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of Files Reviewed (9 files)
Previous findings — current state
Notes and assumptions
Previous review (commit dd659d4)Status: 3 Issues Found | Recommendation: Address before merge Executive SummaryThe quoted-diff snippet keeps the first 30 hunk lines instead of the last, so for long hunks it hides the very line the review comment is anchored to. Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (21 files)
Notes and assumptions
Reviewed by claude-opus-5 · Input: 28 · Output: 5.3K · Cached: 609.6K Review guidance: REVIEW.md from base branch |
Android a11y label nodes are leaves (group card content geometrically); helper selectors are full-string regexes (escape literals, never let an empty findRects pass vacuously); scrollUntilVisible can land yet throw (probe the hierarchy before retrying).
Empty-clipboard paste now surfaces 'Clipboard is empty' via sonner toast and never sets the inline helper, so the entry layout does not engage the message slot for that state at all. The reserved slot stays for the invalid-link state; the helper-slot selector drops the now-unreachable clipboard-empty state (copy constant kept for the toast). The NBSP placeholder line is now hidden from screen readers.
GitHub's diffHunk ends at the anchored line, so the 30-line cap now keeps the hunk's tail with a leading 'more lines above' row — the commented line always renders. Threads no longer parse the hunk while collapsed, and the memoized snippet keeps DiffLine's memo effective.
Android toast a11y-invisibility + deep-link dedupe techniques; iOS XCUI a11y parents are leaves (assert by geometry); PR entry Open button's content-driven clear-X shift is pre-existing and must be isolated from helper-slot measurements.
Empty-clipboard paste, pasted non-URL text, and typed invalid links all surface error toasts now; the inline helper-message UI and its reserved slot are gone from the entry screen, so the layout has no message row at all. The clear-button selector and the toast copy move to pr-link-paste (pr-link-helper-slot is deleted). Open is disabled only when the field is empty.
iscekic
changed the title
sonner toasts never enter the Android a11y tree, so the toast-only entry feedback was silent for screen-reader users. Route the three failure paths through announcingToast.error, which keeps the identical visual toast and adds announceForA11y.
pandemicsyn
approved these changes
Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four PR-review UX improvements on the mobile app, plus the one backend field they
needed.
What: Discussion threads quote the diff lines a comment is anchored to (syntax
highlighting and line numbers, capped at the last 30 lines so the anchored line
always renders); reactions collapse to non-zero pills behind a single add-reaction
icon with a bottom-sheet emoji picker; a resolved thread shows exactly one
"Resolved" indicator (the badge — the toggle is icon-only); and PR-link entry
feedback (empty clipboard, invalid link — typed or pasted) surfaces as toasts.
Why: Reviewing on mobile lacked the context GitHub's own clients show: threads
displayed only a text anchor label, all eight reaction emoji rendered on every
comment (including zero-count ones), "Resolved" rendered twice per resolved thread,
and the entry screen's inline helper messages shifted layout and competed with the
input.
How: The GraphQL review-threads query now selects
diffHunkon each thread'sfirst comment, carried through the DTO as a required-nullable field (
''→ null).The app parses it with the existing
parsePatchand draws it with the Files tab'sDiffLine, only when the thread is expanded (memoized); outdated threads show theoriginal hunk (GitHub semantics), file-level threads degrade to the label only.
Reactions filter to count > 0 via a tested pure selector, with a new
readOnly/disabledsplit so conversation comments stay read-only while pendingmutations merely lock presses. Entry feedback uses the app's existing
toast.errorpattern — an earlier iteration of this PR reserved an inline slot forthe messages; per the requester's direction the inline helper UI was removed
entirely (the helper-slot lib is deleted, surviving pieces moved to
pr-link-paste.ts), so the layout has no message row and cannot shift. The E2EGitHub stub became stateful (resolve/unresolve/reactions) with per-platform
fixtures so iOS and Android verifiers run in parallel without racing.
Verification
kilo-stub/discussion-mixedrefactor(bot): Extract platform-agnostic tool system #1 iOS / Add env lazy loading for dev environment #11 Android): all four acceptance criteriapassed at
47ebec33d— iOS full pass (round 2); Android full pass (round 2) plusa focused round-3 re-check of the invalid-link layout claim with a
constant-field-state protocol (a round-2 FAIL on that point was disproven by the
verifier's own hierarchies: the slot held 46px in every state; the observed
+14px was the pre-existing clear-button row growth from Mobile review surfaces and invoice download #4776, present on main).
Evidence: screenshots below and per-case hierarchies/logs retained in the run
record.
0bc3f1bba, invalid-link → toast + helper-slotremoval): deliberately NOT re-run through E2E per the requester's call — the
toast mechanism and paste state machine are the exact path the E2E rounds
live-verified for clipboard-empty (screenshot below), and this delta is
presentation-only on that mechanism. Covered instead by unit tests plus
typecheck/lint/format (all green).
Automated coverage at HEAD (
0bc3f1bba): backend mapper/router tests for the newfield (139 web tests green, including the GraphQL schema validation of the edited
query); mobile unit tests for the snippet selector, reaction pills, paste/toast
copy, and clear-button selector (full mobile suite, 2470 tests, green); typecheck,
lint, and unused-export checks green on both packages; root format clean.
Visual Changes
After shots are from the E2E verification runs (iOS). The AC4 "before" is the
inline helper message from the first E2E round, before the toast scope change.
Reviewer Notes
github-pr-review-router.ts: onlydiffHunkwas added to the query/type/threadbuilder;
CONVERSATION_COMMENTS_QUERYintentionally unchanged (IssueComment hasno diffHunk).
discussion-thread.tsx: the "Resolved" badge is the sole resolved text; thetoggle kept its accessibility labels. The snippet cap keeps the hunk's TAIL —
GitHub's
diffHunkends at the anchored line (a Kilobot finding, fixed).server.mjsis committed E2E harness code:buildMixedFixture(idSuffix)registers identical fixtures at pulls 1 and 11 with globally unique ids, and the
new mutations mutate per-key fixture objects in place (process lifetime).
pre-existing (Mobile review surfaces and invoice download #4776 on main) and out of scope here; both verifiers isolated it
from the helper-message measurements.
E2E; the E2E skip for that final delta is the requester's documented call.