Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds edit/delete workflows to work item timelines: UI components (edit actions, delete modal), mutation composable, server endpoints for review comment patch/delete, permission propagation through mappings/types, page wiring, i18n keys, and unit tests — enabling inline comment editing and deletion. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as CommentCard / ReviewThread
participant Composable as useWorkItemMutations
participant Server as API Server
participant GitHub as GitHub API
participant Cache as WorkItem Cache
User->>UI: Click Edit
UI->>Composable: startEdit(entryId)
Composable->>Composable: set editingId
User->>UI: Save edited body
UI->>Composable: saveComment(entryId, body)
Composable->>Server: PATCH /issues/comments or /pull-requests/review-comments (body)
Server->>GitHub: Update comment
GitHub-->>Server: Updated comment
Server->>Cache: invalidateWorkItemDetailCache(workItemId?)
Server-->>Composable: success
Composable->>UI: update timeline (emit/notify)
User->>UI: Click Delete
UI->>UI: Open DeleteModal → Confirm
UI->>Composable: deleteComment(entryId)
Composable->>Server: DELETE /issues/comments or /pull-requests/review-comments
Server->>GitHub: Delete comment
GitHub-->>Server: 204
Server->>Cache: invalidateWorkItemDetailCache(workItemId?)
Server-->>Composable: success
Composable->>UI: remove entry from timeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
server/api/pull-requests/review-comments.delete.ts (1)
19-30: PrefergithubFetchWithTokenfor consistency with sibling endpoints.Line 19 through Line 30 manually re-implements GitHub request/error behavior that is already centralized and used by
server/api/pull-requests/review-comments.patch.ts.Suggested refactor
-import { getSessionToken, invalidateWorkItemDetailCache } from '~~/server/utils/github' +import { getSessionToken, githubFetchWithToken, invalidateWorkItemDetailCache } from '~~/server/utils/github' @@ - const response = await fetch(`https://api.github.com/repos/${owner}/${repo}/pulls/comments/${commentId}`, { - method: 'DELETE', - headers: { - 'Authorization': `token ${token}`, - 'Accept': 'application/vnd.github+json', - 'X-GitHub-Api-Version': '2022-11-28', - }, - }) - - if (!response.ok) { - throw createError({ statusCode: response.status, message: `GitHub API ${response.status}: ${response.statusText}` }) - } + await githubFetchWithToken( + token, + `/repos/${owner}/${repo}/pulls/comments/${commentId}`, + { method: 'DELETE' }, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/pull-requests/review-comments.delete.ts` around lines 19 - 30, Replace the manual fetch/delete call with the shared helper githubFetchWithToken used by sibling endpoints (e.g., review-comments.patch.ts): call githubFetchWithToken with the DELETE method and the path `repos/${owner}/${repo}/pulls/comments/${commentId}`, pass the token, and rely on the helper to throw or return a normalized response so you can remove the manual response.ok check and createError logic; ensure the function still returns/forwards the helper's result or error so calling code behavior remains unchanged.server/api/issues/body.patch.ts (1)
71-73: Guard issue-detail cache invalidation to issue updates only.Line 71 through Line 73 runs even for
type === 'pull'. You can skip this invalidation for pull updates to avoid unnecessary cache operations.Suggested tweak
- if (repo && issueNumber) { + if (type !== 'pull' && repo && issueNumber) { await invalidateIssueDetailCache(login, repo, issueNumber) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/issues/body.patch.ts` around lines 71 - 73, The invalidateIssueDetailCache call is executed for pull updates too; guard it so it only runs for issue updates by checking the incoming event type (e.g., only when type === 'issue' or type !== 'pull') before calling invalidateIssueDetailCache(login, repo, issueNumber). Update the conditional that currently checks repo && issueNumber to also verify the event type (reference variables type, repo, issueNumber and function invalidateIssueDetailCache) so pull events skip this invalidation.server/api/repository/[owner]/[repo]/work-items/[id].get.ts (1)
160-161: Remove or map unusedPullRequestReviewpermission fields.Line 160 and Line 161 fetch
viewerCanUpdate/viewerCanDelete, but Line 485 through Line 494 does not propagate them into the timeline entry. Either map them or drop them from the query to avoid payload/confusion drift.Also applies to: 485-494
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/repository/`[owner]/[repo]/work-items/[id].get.ts around lines 160 - 161, The query currently selects PullRequestReview fields viewerCanUpdate and viewerCanDelete but the timeline mapping (the code that builds timeline entries for PullRequestReview) does not include them; either remove these fields from the GraphQL selection or add them into the timeline entry object so they are propagated. Locate the PullRequestReview selection (viewerCanUpdate, viewerCanDelete) and either drop those two fields, or update the timeline-entry construction code that maps PullRequestReview into the timeline (the mapping that creates the timeline entry object for PullRequestReview) to include viewerCanUpdate and viewerCanDelete properties so the fetched permissions are actually used.app/composables/useWorkItemMutations.ts (1)
87-89: Consider logging errors for debugging.The catch blocks silently discard errors after showing a toast. Logging the error would aid debugging in production.
Example for one catch block
- catch { + catch (err) { + console.error('Failed to save comment:', err) toast.add({ title: t('workItems.timeline.editError'), color: 'error' }) }Also applies to: 111-113, 149-151, 183-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useWorkItemMutations.ts` around lines 87 - 89, The catch blocks currently swallow errors; update each catch (including the one that shows t('workItems.timeline.editError') and the other similar blocks at the other ranges) to accept the thrown error (e) and log it before showing the toast—e.g., call console.error with a brief context string (like "Failed to edit work item timeline") and the error object so stack/details are preserved; do this for all catch blocks in useWorkItemMutations.ts that currently just show a toast so debugging information is available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/timeline/EditActions.vue`:
- Around line 26-34: The icon-only UButton controls (the edit and delete
buttons) lack accessible names; update the UButton instances (the one with
`@click`="emit('edit')" and the delete button at lines 40–47) to include explicit
aria-label attributes (e.g., aria-label="Edit item" and aria-label="Delete item"
or localized equivalents) so screen readers can announce their purpose; ensure
the aria-label text clearly describes the action and remains present even when
:disabled (use plain aria-label or :aria-label binding if you need i18n).
In `@app/components/timeline/ReviewCommentThread.vue`:
- Around line 57-67: saveEdit currently clears editingCommentId.value and
editBody.value immediately after emitting edit events (emit('editComment'...) /
emit('editReply'...)), which loses user input if the parent/API save fails;
instead, stop clearing those local editing states in saveEdit and let the parent
signal success to clear them (either by switching to a parent-controlled
editingId prop or by accepting a success callback/ack event from the parent).
Locate saveEdit, editingCommentId, and editBody in ReviewCommentThread.vue and
remove the immediate clearing there; implement/expect a parent-driven clear
(e.g., parent updates a prop or emits an 'editSaved' event that the component
listens for to set editingCommentId.value = null and editBody.value = '').
In `@app/composables/useWorkItemMutations.ts`:
- Around line 166-179: The deletion flow currently removes the root comment from
the timeline (see updateTimeline mapping that filters reviewComments by
commentId and filters replies) but GitHub only deletes the single comment ID so
replies remain orphaned; add a unit/integration test that deletes a root comment
with replies and asserts that child replies still exist in the backend DB to
reproduce the issue, then fix by either (A) updating the delete handler to
enumerate and delete child replies after the root deletion (call the API to
delete each reply id found under reviewComments.replies for the given
entryId/commentId) or (B) explicitly document/accept orphaned replies; target
code symbols to change: the frontend mutation updateTimeline, the backend delete
handler that calls GitHub, and any test file for work item mutations to include
the new test case that references entryId, commentId, reviewComments, and
replies.
In `@test/unit/workItemMutations.test.ts`:
- Around line 37-77: Tests currently re-implement mutation helpers
(editCommentBody, deleteComment, editReviewComment, deleteReviewComment) instead
of using the production implementations from useWorkItemMutations; remove these
duplicated functions from test/unit/workItemMutations.test.ts and import the
shared mutation helpers used by the composable (or extract those helpers from
useWorkItemMutations into a small exported util module) so the tests exercise
the real mutation logic rather than a test-local copy.
---
Nitpick comments:
In `@app/composables/useWorkItemMutations.ts`:
- Around line 87-89: The catch blocks currently swallow errors; update each
catch (including the one that shows t('workItems.timeline.editError') and the
other similar blocks at the other ranges) to accept the thrown error (e) and log
it before showing the toast—e.g., call console.error with a brief context string
(like "Failed to edit work item timeline") and the error object so stack/details
are preserved; do this for all catch blocks in useWorkItemMutations.ts that
currently just show a toast so debugging information is available.
In `@server/api/issues/body.patch.ts`:
- Around line 71-73: The invalidateIssueDetailCache call is executed for pull
updates too; guard it so it only runs for issue updates by checking the incoming
event type (e.g., only when type === 'issue' or type !== 'pull') before calling
invalidateIssueDetailCache(login, repo, issueNumber). Update the conditional
that currently checks repo && issueNumber to also verify the event type
(reference variables type, repo, issueNumber and function
invalidateIssueDetailCache) so pull events skip this invalidation.
In `@server/api/pull-requests/review-comments.delete.ts`:
- Around line 19-30: Replace the manual fetch/delete call with the shared helper
githubFetchWithToken used by sibling endpoints (e.g., review-comments.patch.ts):
call githubFetchWithToken with the DELETE method and the path
`repos/${owner}/${repo}/pulls/comments/${commentId}`, pass the token, and rely
on the helper to throw or return a normalized response so you can remove the
manual response.ok check and createError logic; ensure the function still
returns/forwards the helper's result or error so calling code behavior remains
unchanged.
In `@server/api/repository/`[owner]/[repo]/work-items/[id].get.ts:
- Around line 160-161: The query currently selects PullRequestReview fields
viewerCanUpdate and viewerCanDelete but the timeline mapping (the code that
builds timeline entries for PullRequestReview) does not include them; either
remove these fields from the GraphQL selection or add them into the timeline
entry object so they are propagated. Locate the PullRequestReview selection
(viewerCanUpdate, viewerCanDelete) and either drop those two fields, or update
the timeline-entry construction code that maps PullRequestReview into the
timeline (the mapping that creates the timeline entry object for
PullRequestReview) to include viewerCanUpdate and viewerCanDelete properties so
the fetched permissions are actually used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 707e4d2e-3183-44d0-89f0-289b80a18df0
📒 Files selected for processing (19)
app/components/timeline/CommentCard.vueapp/components/timeline/DeleteModal.vueapp/components/timeline/EditActions.vueapp/components/timeline/ReviewCommentThread.vueapp/composables/useWorkItemMutations.tsapp/pages/repos/[owner]/[repo]/work-items/[id].vuei18n/locales/de.jsoni18n/locales/en.jsoni18n/schema.jsonserver/api/issues/body.patch.tsserver/api/issues/comments.delete.tsserver/api/issues/comments.patch.tsserver/api/pull-requests/review-comments.delete.tsserver/api/pull-requests/review-comments.patch.tsserver/api/repository/[owner]/[repo]/work-items/[id].get.tsserver/utils/review-replies.tsshared/types/work-item.tstest/unit/reviewReplies.test.tstest/unit/workItemMutations.test.ts
Comment on lines
+57
to
+67
| function saveEdit(comment: ReviewComment, type: 'comment' | 'reply') { | ||
| if (!editBody.value.trim() || !comment.databaseId) return | ||
| if (type === 'comment') { | ||
| emit('editComment', comment.id, comment.databaseId, editBody.value) | ||
| } | ||
| else { | ||
| emit('editReply', comment.id, comment.databaseId, editBody.value) | ||
| } | ||
| editingCommentId.value = null | ||
| editBody.value = '' | ||
| } |
Contributor
There was a problem hiding this comment.
Editing state cleared before confirming save success.
saveEdit clears editingCommentId and editBody immediately after emitting, before the parent confirms the save succeeded. If the API call fails, the user's edits are lost and they'd need to re-enter their changes.
Consider keeping the editing state until the parent signals success (e.g., via a prop change or callback), or accept this as intentional optimistic UX where failures are rare.
Alternative: Let parent control editing state
One approach is to have the parent pass an editingId prop (similar to CommentCard.vue) and control clearing it only after a successful save:
-function saveEdit(comment: ReviewComment, type: 'comment' | 'reply') {
- if (!editBody.value.trim() || !comment.databaseId) return
- if (type === 'comment') {
- emit('editComment', comment.id, comment.databaseId, editBody.value)
- }
- else {
- emit('editReply', comment.id, comment.databaseId, editBody.value)
- }
- editingCommentId.value = null
- editBody.value = ''
-}
+function saveEdit(comment: ReviewComment, type: 'comment' | 'reply') {
+ if (!editBody.value.trim() || !comment.databaseId) return
+ if (type === 'comment') {
+ emit('editComment', comment.id, comment.databaseId, editBody.value)
+ }
+ else {
+ emit('editReply', comment.id, comment.databaseId, editBody.value)
+ }
+ // Let parent clear editing state on success
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/timeline/ReviewCommentThread.vue` around lines 57 - 67,
saveEdit currently clears editingCommentId.value and editBody.value immediately
after emitting edit events (emit('editComment'...) / emit('editReply'...)),
which loses user input if the parent/API save fails; instead, stop clearing
those local editing states in saveEdit and let the parent signal success to
clear them (either by switching to a parent-controlled editingId prop or by
accepting a success callback/ack event from the parent). Locate saveEdit,
editingCommentId, and editBody in ReviewCommentThread.vue and remove the
immediate clearing there; implement/expect a parent-driven clear (e.g., parent
updates a prop or emits an 'editSaved' event that the component listens for to
set editingCommentId.value = null and editBody.value = '').
Comment on lines
+166
to
+179
| updateTimeline(timeline => | ||
| timeline.map((e) => { | ||
| if (e.id !== entryId) return e | ||
| return { | ||
| ...e, | ||
| reviewComments: e.reviewComments | ||
| ?.filter(rc => rc.id !== commentId) | ||
| .map(rc => ({ | ||
| ...rc, | ||
| replies: rc.replies?.filter(r => r.id !== commentId), | ||
| })), | ||
| } | ||
| }), | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -A 10 "deleteReviewComment|deletePullRequestReviewComment" --type ts --glob '!node_modules/**'Repository: flumen-dev/flumen.dev
Length of output: 6203
🏁 Script executed:
# Search for the mutation/API call definition in the composable
rg -n "deleteReviewComment" app/composables/useWorkItemMutations.ts -A 20Repository: flumen-dev/flumen.dev
Length of output: 847
🏁 Script executed:
# Look for related GraphQL mutations or API calls that might handle the deletion
fd -e ts -e graphql . --exec grep -l "deleteReviewComment\|deletePullRequestReviewComment" {} \;Repository: flumen-dev/flumen.dev
Length of output: 144
🏁 Script executed:
# Find the backend API handler for review comments deletion
fd -e ts -e js . --path "*/api/*" --exec grep -l "review-comments" {} \;Repository: flumen-dev/flumen.dev
Length of output: 298
🏁 Script executed:
# Search for the backend handler more broadly
rg -n "review-comments" --type ts --type js --glob '!node_modules/**' | head -20Repository: flumen-dev/flumen.dev
Length of output: 844
🏁 Script executed:
# Look for database models or cascade delete configurations
rg -n "cascade|onDelete|orphan" --type ts --glob '!node_modules/**' -iRepository: flumen-dev/flumen.dev
Length of output: 207
🏁 Script executed:
# Find the server/API implementation directory
find . -type d -name "server" -o -name "api" -o -name "routes" | grep -v node_modules | head -20Repository: flumen-dev/flumen.dev
Length of output: 102
🏁 Script executed:
# Search for the actual API route handler
rg -n "review-comments" --type ts --glob '!node_modules/**' -B 5 -A 15 | grep -A 15 "DELETE\|delete"Repository: flumen-dev/flumen.dev
Length of output: 1408
🏁 Script executed:
# Look in common API directories for route definitions
ls -la server/ 2>/dev/null || ls -la app/ 2>/dev/null || find . -maxdepth 3 -name "*.ts" -path "*/api/*" | head -20Repository: flumen-dev/flumen.dev
Length of output: 399
🏁 Script executed:
# List the server/api directory structure
find server/api -type f -name "*.ts" | head -30Repository: flumen-dev/flumen.dev
Length of output: 1271
🏁 Script executed:
# Search for pull-requests review-comments API handler
rg -n "review-comments\|ReviewComment" server/ --type ts -A 10Repository: flumen-dev/flumen.dev
Length of output: 47
🏁 Script executed:
# Read the delete handler for review comments
cat -n server/api/pull-requests/review-comments.delete.tsRepository: flumen-dev/flumen.dev
Length of output: 1483
Backend leaves orphaned replies when a root comment is deleted.
The backend delegates deletion to GitHub's API (lines 19-26), which only deletes the specified comment ID. When a root comment with replies is deleted, GitHub does not cascade-delete the replies—they remain in the database as orphaned entities. The frontend's updateTimeline removes the parent from the timeline, implicitly hiding the replies from the UI, but the orphaned data persists in the backend.
The existing tests do not cover this scenario. Add a test case that verifies deletion of a root comment with replies and confirm whether the application should:
- Explicitly delete orphaned replies via separate API calls, or
- Accept orphaned replies as acceptable technical debt
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/composables/useWorkItemMutations.ts` around lines 166 - 179, The deletion
flow currently removes the root comment from the timeline (see updateTimeline
mapping that filters reviewComments by commentId and filters replies) but GitHub
only deletes the single comment ID so replies remain orphaned; add a
unit/integration test that deletes a root comment with replies and asserts that
child replies still exist in the backend DB to reproduce the issue, then fix by
either (A) updating the delete handler to enumerate and delete child replies
after the root deletion (call the API to delete each reply id found under
reviewComments.replies for the given entryId/commentId) or (B) explicitly
document/accept orphaned replies; target code symbols to change: the frontend
mutation updateTimeline, the backend delete handler that calls GitHub, and any
test file for work item mutations to include the new test case that references
entryId, commentId, reviewComments, and replies.
This was referenced Mar 6, 2026
Merged
8 tasks
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
1 participant
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
Related issue(s)
Closes #192
Type of change
Checklist
Summary by CodeRabbit
New Features
Localization
Tests