feat(mobile): add edit and delete for own PR review comments - #6694
Conversation
…view-own-comment-crud-f555/s1)
… (kwf app-pr-review-own-comment-crud-f555/s2)
…w-own-comment-crud-f555/s3)
…ete confirm (kwf app-pr-review-own-comment-crud-f555/s4)
… (kwf app-pr-review-own-comment-crud-f555/s5)
…the discussion (retention must not resurrect it) (kwf app-pr-review-own-comment-crud-f555/c1)
…y CTA this surface's other actions use (kwf app-pr-review-own-comment-crud-f555/c2)
…e (kwf app-pr-review-own-comment-crud-f555/ux1)
…em (kwf app-pr-review-own-comment-crud-f555/ux2)
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe ownership fix ( Overview
Issue Details (click to expand)WARNING
Files Reviewed
Fix these issues in Kilo Cloud Previous Review Summaries (4 snapshots, latest commit 18904c2)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 18904c2)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental delta since the prior review is a clean merge of Files Reviewed (3 files, current-HEAD verification)
The only incremental change since Previous review (commit 88174f7)Status: No Issues Found | Recommendation: Merge Files Reviewed (8 files)
The only delta since 347499b is a clean merge of Previous review (commit 347499b)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental fix commit resolves both prior findings — own-comment 404s are now terminal on the edit surface and Files Reviewed (11 files, incremental commit 347499b)
Previous review (commit d2d59dc)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (31 source/test files plus 86 locale catalogs)
Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0 Review guidance: REVIEW.md from base branch |
…-comment-crud-f555 # Conflicts: # apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.mounted.test.tsx # apps/mobile/src/components/pr-review/discussion/pr-review-discussion-list.tsx # apps/mobile/src/i18n/catalog-parity.test.ts
…before an idempotent delete Kilobot review on #6694: - WARNING: classifyPrReviewMutationError falls through to retryable for NOT_FOUND, so editing a comment deleted elsewhere showed the generic retryable copy with Save live and the intended commentEditUnavailable copy stayed unreachable. commentCrudFailure now returns the surface's terminal copy for a 404, and mutationErrorDisplay maps an edit-surface 404 to the new terminal `not-found` kind, which keeps Save down in PrCommentEditSheet. - SUGGESTION: every provider 404 was reported as a successful delete, but classifyGitHubHttpError maps a missing PR / repo / App access to the same NOT_FOUND. deleteComment now confirms the PR is still reachable before reporting the delete as done; otherwise the 404 propagates.
…-comment-crud-f555 One conflict in discussion-thread.tsx: this branch extracts ResolveToggle into discussion-thread-resolve-toggle.tsx, and main renames the tap-target module and the compact 44pt slop constant. Keep the extraction, and let the extracted file import COMPACT_H11_HIT_SLOP_DP from @/lib/a11y/tap-target, which is main's renamed name and keeps the touch-target test green.
…-comment-crud-f555
…-comment-crud-f555 # Conflicts: # apps/mobile/src/app/(app)/pr-review/[owner]/[repo]/[number]/_layout.tsx # apps/mobile/src/i18n/catalog-parity.test.ts
iscekic
added
merge-by-human
…-comment-crud-f555
eshurakov
left a comment
eshurakov
left a comment
There was a problem hiding this comment.
Warning — comment edit/delete have no server-side ownership check.
updateComment / deleteComment (apps/web/src/routers/github-pr-review-router.ts:1894,1934) forward owner / repo / commentId straight to pulls.* / issues.*. The "own comment" rule is enforced only by the mobile UI; the procedures themselves do not compare the target comment's author to ctx. A caller who bypasses the UI (for example a collaborator with push access) can still edit or delete another author's comment through these procedures. GitHub's own token scoping still blocks non-collaborators and there is no cross-repo path, but if "own comment only" is a requirement it should be checked server-side rather than in the UI.
…-comment-crud-f555
The mobile row shows Edit and Delete only on the viewer's own comment. The procedures did not compare the comment author with the caller, so a caller that skipped the app could change another author's comment. `assertViewerOwnsComment` reads the comment author and the caller's own GitHub login, then refuses a mismatch with FORBIDDEN. The viewer login comes from a live read, because a GitHub rename would leave the recorded login stale. A 404 on the comment read is not an ownership verdict: the guard reports the comment as missing, so the idempotent delete path stays unchanged.
|
Fixed in 334cc8a. The remark is valid. The router now calls Two details decide the shape:
Verification: |
2 participants
Changelog for users
Changelog for maintainers
githubPrReview.updateCommentanddeleteComment, dispatching onkindtopulls.*(review) orissues.*(conversation); delete confirms the PR is still reachable before treating a provider 404 as success, so an already-deleted comment stays idempotent while a missing PR / repo / App access surfaces as a real failure.updateCommentruns the UGC terms gate before any GitHub write, and both mutations takenumberso the overview can be invalidated after a write.applyCommentBodyUpdate/applyCommentRemovalreducers with snapshot rollback and settle invalidation; the writes carry no operation-ledger key because both are idempotent.deleted-comment-retentionstore that filters a deleted comment out of refetched pages and discounts the Discussion badge, reconciled once the overview count falls below the delete's baseline.retainConversationAcrossMountsnow takesfirstPageLoaded, so a loaded-but-empty first page stays the source of truth and deleting the last conversation comment empties the discussion.comment-editformSheet route andPrCommentEditSheet;useComposerInlineErrortakes asurfaceselecting the edit-specific bad-request and retryable copy.CommentRowreplaces the disabled self-moderation trio with Edit/Delete only when both callbacks are supplied for the viewer's own comment; read-only provider rows keep the prior menu.updateCommentanddeleteCommentnow check comment ownership on the server (assertViewerOwnsComment), not only in the mobile row. A caller that bypasses the app cannot edit or delete another author's comment. The guard reads the comment author and the caller's own GitHub login, and refuses a mismatch with FORBIDDEN.services/kilo-mcp/catalog.jsonnow listsgithubPrReview.updateCommentanddeleteComment, so this PR must justify the exposure of a comment delete to MCP agents.apps/web/src/scripts/mcp-catalog/catalog.ts), and CI fails on catalog drift, so a hand-written exclusion does not exist. Main already publishes 80 delete/remove/revoke/cancel mutations, includinggithubPrReview.removeReaction.assertViewerOwnsCommentrefuses another author's comment server-side, so an MCP agent can change only a comment that the caller's own GitHub token already lets it change. The use case is cleanup: an agent that posted a review comment on the caller's behalf can correct its text or remove it.E2E proof
Owner request
[e3] delete an own comment after one confirmation (default Android): create a conversation comment, Comment actions > Delete comment, exactly one confirmation dialog ('Delete comment?'), Delete, and the… — android emulator-5554: after Comment actions > Delete comment exactly one confirmation dialog is present (e3-confirm-dialog.txt / jev-drive-1790189111342.log: title 'Delete comment?', message 'This comment will be deleted from the pull request.', buttons Cancel/Delete), Delete removes the comment (e3-final-gone.txt: no 'kwf-del-final' text; e3-after-delete.txt: no 'kwf-delete-me' text) and it stays gone after a fresh reload of the pr-review state (e3-final-stays-gone.txt, e3-stays-gone.txt: zero 'kwf-' comment bodies). The parked scene cannot be used as-is: e3-final-scene.log shows its…
[e1] read then update an own comment (default Android): create a conversation comment, see its full text in the discussion (read), open Comment actions > Edit comment, the sheet shows the full text… — banked from the progress ledger at the turn cap
[e3] ux-check: own-comment overflow lists Edit comment / Delete comment / Report content / Cancel; another author's comment and a deleted-author comment show neither — android emulator-5604, declared state pr-review on real GitHub #6054 Discussion tab. Own arm: e3-discussion.txt shows the rows text="kilo-code-bot" (other author) and text="iscekic" beside text="e9 second comment" (the viewer's own); opening Comment actions on that own row yields a sheet with exactly text="Edit comment", text="Delete comment", text="Report content", text="Cancel" (e3-own-overflow.txt, e3-own-overflow.png). Other-author arm: opening Comment actions on the kilo-code-bot row yields text="Report content", text="Report user", text="Mute", text="Block", text="Cancel"…
[e3] ux-check: own-comment overflow lists Edit comment / Delete comment / Report content / Cancel; another author's comment and a deleted-author comment show neither
[e9] posts two conversation comments and deletes the older one — Scripted scene MISSed because the declared state's real PR was unavailable while github-stub was up; stopped the stub, restored state pr-review and hand-drove on real PR #6054 (posted 'e9 first comment' then 'e9 second comment', deleted the older via Comment actions > Delete comment > 'Delete comment?' > Delete): e9-after-delete.txt contains text="e9 second comment" with 0 occurrences of 'e9 first comment' (present before the delete in e9-two-comments.txt) and e9-real-pr-comments.log lists only '5799399364 | iscekic | e9 second comment'; UX audit: zero defects.
[e1] update failure is visible and the text is kept (default Android) — needs:fault: the update write fails (transport/5xx). — android emulator-5554: with nextjs down (e1-fault.log
fault.sh: nextjs killed (port 6100 refuses), the corrected replay e1.replay.json endsSCENE e1 OKin e1-scene.log (the expectedCouldn't save your commentcopy assert passed) and e1-failure-state.txt shows the sheet still open after the failed write with the typed text kept (class="android.widget.EditText" text="-x") and Save still usable (content-desc="Save" ... clickable="true" enabled="true"); nothing was written and the comment list is unchanged; failure-state still captured as e1-failure-state.png for the visual reviewer.[e17] ux-check: Failed write (server rejection): the edit sheet shows a visible inline error and keeps the typed body and Save available for retry; a failed delete restores the row and shows a retryable… — android emulator-5554, PR #6054; with nextjs down the edit sheet showed the inline error 'fetch failed: java.io.IOException: unexpected end of stream on http://127.0.0.1:6100/...' and the toast 'Couldn't save your comment. Check your connection and try again.' with Button "Save" still enabled and the typed body kept (Cancel opened 'Discard comment?'), and the failed delete restored the 'kwf-read-edit' row then showed the 'Something went wrong'/'Couldn't delete your comment. Check your connection and try again.' alert whose Button "Retry" re-ran the delete instead of a silent…
[e19] ux-check: Offline delete: with the app confirmed offline, confirming Delete shows the failure feedback and leaves the comment row in place rather than optimistically vanishing. — android emulator-5554, PR #6054 Discussion tab: with the app confirmed offline (banner TextView "No internet connection", radio off) the own row "e9 second comment" showed one confirmation dialog (e19-confirm.txt), and the digest right after confirming Delete shows TextView "Couldn't delete your comment. Check your connection and try again." together with TextView "e9 second comment" and android.view.View "Discussion, 3" (e19-delete-failed.log) — failure feedback shown and the row left in place, never optimistically removed; the scripted confirm also ended SCENE e19 OK with both…
[e17] ux-check: Failed write (server rejection): the edit sheet shows a visible inline error and keeps the typed body and Save available for retry; a failed delete restores the row and shows a retryable…
[e18] ux-check: Offline edit: with the app confirmed offline (offline banner shown before opening), tapping Save in the edit sheet shows the retryable inline copy immediately with no indefinite spinner… — android emulator-5554, PR #6054; radio off, the 'No internet connection' banner was on the Discussion before the edit sheet opened, tapping Save with the body edited showed 'Couldn't save your comment. Check your connection and try again.' at once with Button "Save" and Button "Cancel" enabled (no spinner), and Cancel opened the 'Discard comment?' gate (e18-offline-edit.log, e18-offline-banner.png, e18-offline-inline-copy.png); no UX-DEFECT in the digests.
[e7] empty discussion: no comment management anywhere (default Android) — needs:seed: the seeded PR has no review threads and no conversation comments. — android; state.sh pr-review STATE HIT, hermetic stub fixture kilo-stub/discussion-empty#3 opened, harvested replay: 'SCENE e7 OK' with digest showing 'No discussion yet' / 'No review threads or conversation comments on this pull request.' and no 'Comment actions' (still e7.png).
[e15] ux-check: update dismisses the sheet and shows the new text without a full reload (android emulator-5606) — Changed the body to 'kwf-e10-read-comment edited v2' and tapped Save on real GitHub (#6401): e15-update.log line 1 'SCENE e15 OK' and the final digest shows the Discussion row text="kwf-e10-read-comment edited v2" with the 'Edit comment' sheet header absent, i.e. the sheet dismissed and the row updated in place with no full reload and no duplicate loading indicator; no UX defect.
[e14] Read: Edit comment formSheet shows the posted comment text in full — Tapped Comment actions > Edit comment on the viewer's own comment; the formSheet EditText (content-desc="Comment body") carries the full row text ending 'end-marker' identical to the discussion row, and the Save button is content-desc="Save" ... enabled="false" while the body is unchanged (same in e14-edit.txt for the short comment); UX audit: zero defects.
[e10] read + update own comment on the PR review page (android emulator-5606) — Ran on real GitHub (#6401) after github-stub.sh stop + github-user-token.sh seed + github-installation.sh, because the pack's stub serves no comment PATCH (404 'stub: unhandled PATCH'); the edit sheet opened with the full body (e10-real-read-sheet.txt: text="kwf-e10-read-comment" beside text="Edit comment"), appending ' edited' (e10-real-dirty-sheet.txt: text="kwf-e10-read-comment edited") and Save made the sheet dismiss and the Discussion row show text="kwf-e10-read-comment edited" (e10-real-updated.txt), confirmed server-side by e10-github-verify.log ('kwf-e10-read-comment…
[e12] needs:fault:nextjs down, delete own comment then Retry after recovery (default Android) — android; viewer comment kwf-e12-real-del seeded on #6054; with nextjs down (fault.sh) the confirmed Delete showed 'Something went wrong' + 'Couldn't delete your comment. Check your connection and try again.' + Retry (e12-error.txt); Cancel left the row present (e12-rollback.txt); after fault.sh up nextjs, Retry removed it — e12-final.txt shows Discussion count 3 with no kwf-e12-real-del.
[e18] ux-check: Offline edit: with the app confirmed offline (offline banner shown before opening), tapping Save in the edit sheet shows the retryable inline copy immediately with no indefinite spinner…
[e3] ux-check: own-comment overflow lists Edit comment / Delete comment / Report content / Cancel; another author's comment and a deleted-author comment show neither
Follow-ups (not changed here)
Owner manual verification
Owner verification is pending; these checks did not pass automatically.
Open findings (not fixed here)
e2e/github-stub.sh start <email>; harness-only, outside the product diff) - the viewer posts one conversation comment and deletes