Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds PR review UI and backend: new ReviewActions component and composable, server endpoints for submitting/dismissing reviews and deleting PR head branches, work-item and waiting-on-me data/type extensions (needs-review), i18n keys, and WorkItemHeader integration emitting a reviewed event. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ReviewActions as ReviewActions
participant useReviewActions as useReviewActions
participant Server as API Server (/reviews.post)
participant GitHub as GitHub API
User->>ReviewActions: choose event & (optional) body, submit
ReviewActions->>useReviewActions: submitReview(event, body, workItemId)
useReviewActions->>Server: POST /pulls/{number}/reviews {event, body, workItemId}
Server->>GitHub: POST /repos/{owner}/{repo}/pulls/{number}/reviews
alt success
GitHub-->>Server: review created
Server->>useReviewActions: mapped ReviewResponse
useReviewActions-->>ReviewActions: success
ReviewActions->>User: show success, emit "reviewed"
else failure
GitHub-->>Server: error
Server-->>useReviewActions: error
useReviewActions-->>ReviewActions: null
ReviewActions->>User: show error
end
sequenceDiagram
actor User
participant WorkItemHeader as WorkItemHeader
participant API as API Server (/delete-branch.post)
participant GitHub as GitHub API
User->>WorkItemHeader: Click "Delete Branch"
WorkItemHeader->>API: POST /pulls/{number}/delete-branch {branch, repo?}
API->>GitHub: DELETE /repos/{owner}/{repo}/git/refs/heads/{branch}
alt deleted (200/204) or already gone (422)
GitHub-->>API: success/not-found
API-->>WorkItemHeader: {deleted:true, branch}
WorkItemHeader->>User: show success, set branchDeleted
else failure
GitHub-->>API: error
API-->>WorkItemHeader: error
WorkItemHeader->>User: show error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 9
🧹 Nitpick comments (2)
app/composables/useReviewActions.ts (2)
1-1: Use ambient shared-type auto-imports in this composable.
Reviewercan be referenced directly here without an explicit type import, which keeps this file aligned with project conventions.Based on learnings: In the flumen.dev Nuxt project, all pure TypeScript types exported from ROOT/shared/types are globally auto-imported across components, composables, and stores.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useReviewActions.ts` at line 1, The file imports the Reviewer type explicitly which duplicates project-wide ambient auto-imports; remove the import line referring to Reviewer in useReviewActions.ts and update any references to continue using Reviewer directly (no import), ensuring the composable relies on the global shared-type auto-imports and aligns with project conventions for types exported from shared/types.
37-40: Extract duplicated fetch-error parsing into a helper.Both catch blocks implement the same parsing logic; centralizing it avoids drift and keeps error behavior consistent.
♻️ Suggested refactor
+ function extractErrorMessage(e: unknown): string { + const fetchErr = e as { data?: { data?: { errorKey?: string } }, message?: string } + return fetchErr.data?.data?.errorKey ?? fetchErr.message ?? 'unknown' + } + async function submitReview(event: ReviewEvent, body?: string, workItemId?: string): Promise<ReviewResponse | null> { @@ - catch (e: unknown) { - const fetchErr = e as { data?: { data?: { errorKey?: string } }, message?: string } - error.value = fetchErr.data?.data?.errorKey ?? fetchErr.message ?? 'unknown' + catch (e: unknown) { + error.value = extractErrorMessage(e) return null } @@ - catch (e: unknown) { - const fetchErr = e as { data?: { data?: { errorKey?: string } }, message?: string } - error.value = fetchErr.data?.data?.errorKey ?? fetchErr.message ?? 'unknown' + catch (e: unknown) { + error.value = extractErrorMessage(e) return null }Also applies to: 57-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useReviewActions.ts` around lines 37 - 40, There are duplicated fetch-error parsing blocks in useReviewActions.ts (the two catch blocks around lines 37–40 and 57–60); create a small helper function (e.g., parseFetchError or extractFetchError) inside the module to accept e: unknown, coerce it to the expected shape and return the resolved string (fetchErr.data?.data?.errorKey ?? fetchErr.message ?? 'unknown'); then replace both catch bodies to call that helper and assign its return to error.value and return null, keeping existing typing and behavior.
🤖 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/work-item/ReviewActions.vue`:
- Around line 98-135: Add an approval-count summary to the reviewer header by
computing the number of approved reviewers and rendering a compact summary like
"2/3 approved" next to the avatars; specifically, add a computed property (e.g.,
approvedCount) that counts reviewers where reviewer.state indicates approval
(use the same state values checked by getReviewerStateLabel/getReviewerIcon),
then in the avatar/header block in ReviewActions.vue (the div containing the
v-for UTooltip loop) insert a small text element that displays
`${approvedCount}/${reviewers.length} approved` (or use your i18n t(...) string)
and style it consistently with existing classes so it appears before or beside
the avatars without breaking the overflow-x-auto layout.
In `@app/components/work-item/WorkItemHeader.vue`:
- Around line 542-554: The WorkItemReviewActions block is currently rendered
above the merge controls; move the entire <WorkItemReviewActions ... />
component (including its v-if="isPRNotClosed" and props reviewers, ownerRef,
repoRef, pr-number, work-item-id and the `@reviewed`="emit('reviewed')" handler)
so it appears below the Merge section (the div marked "<!-- Row 6: Merge (PR
only) -->") in WorkItemHeader.vue so review actions render beneath merge
controls to match the feature contract.
- Around line 124-137: The component-level flags branchDeleted and
deletingBranch can persist when the component is reused for a different
props.workItem; add a watcher on props.workItem (or a stable identifier like
props.workItem.id) to reset branchDeleted.value = false and deletingBranch.value
= false whenever the displayed work item changes; place the watcher near the
existing reactive declarations (alongside branchDeleted, deletingBranch,
headBranch, showDeleteBranch) so the UI/actions are recalculated for the new
work item and do not leak state across navigations.
In `@server/api/repository/`[owner]/[repo]/pulls/[number]/delete-branch.post.ts:
- Around line 14-23: Trim and normalize the branch value once and reuse it
everywhere: create a local const (e.g., normalizedBranch) from
body.branch?.trim(), use that for the emptiness check instead of calling trim
inline, and replace all usages of body.branch in the fetch URL and subsequent
response handling (including the logic around lines 34-35) with normalizedBranch
so whitespace-padded input cannot break the API call; keep targetRepo logic
unchanged.
- Around line 37-40: The code currently treats any HTTP 422 as "already
deleted"; change this to parse and inspect the response body/message when
response.status === 422 and only return { deleted: true, branch: body.branch }
if the message explicitly indicates the ref is missing (e.g., contains
"Reference does not exist", "ref not found", or equivalent GitHub phrase); for
any other 422 validation messages (default branch, protected branch, malformed
ref) throw or return an error so the failure is surfaced. Locate the 422
handling in delete-branch.post.ts (the block checking response.status === 422
and using body.branch) and update it to read and match the response JSON/message
before deciding success versus error.
In `@server/api/repository/`[owner]/[repo]/pulls/[number]/reviews.post.ts:
- Around line 55-60: The endpoint currently forwards an APPROVE review via
githubFetchWithToken without enforcing the "no self-approval" rule; before
calling githubFetchWithToken in the reviews.post handler, fetch the pull request
data (e.g., GET /repos/{owner}/{repo}/pulls/{number}) to obtain the PR author
and resolve the caller identity from the provided token (the authenticated
user), then if reviewBody.event === 'APPROVE' and the caller login/id equals the
PR author login/id, return a 4xx error (forbidden/bad request) instead of
posting the review; update the logic around githubFetchWithToken/
GitHubReviewResponse to perform this check and short-circuit with an appropriate
error response.
- Around line 43-53: The validation currently calls body.body?.trim() which will
throw if body.body is not a string; change both checks to first assert typeof
body.body === 'string' before trimming (e.g. use typeof body.body === 'string'
&& body.body.trim()) so the REQUEST_CHANGES guard and the later
conditional/assignment for reviewBody.body only call .trim() on actual strings;
update the assignment to set reviewBody.body = body.body.trim() after that type
check.
In
`@server/api/repository/`[owner]/[repo]/pulls/[number]/reviews/[reviewId]/dismiss.post.ts:
- Around line 36-45: The check using body?.message?.trim() can throw for
non-string message values; update the guard in the dismiss POST handler to first
verify typeof body?.message === 'string' and that body.message.trim() is
non-empty, and throw createError({ statusCode: 400, message: 'Dismiss message is
required' }) if not. Then pass the trimmed string to
githubFetchWithToken<GitHubDismissResponse>(token,
`/repos/${owner}/${repo}/pulls/${number}/reviews/${reviewId}/dismissals`, {
method: 'PUT', body: { message: trimmedMessage } }). Ensure the variable used in
the fetch is the validated trimmedMessage and not the original unvalidated
body.message.
---
Nitpick comments:
In `@app/composables/useReviewActions.ts`:
- Line 1: The file imports the Reviewer type explicitly which duplicates
project-wide ambient auto-imports; remove the import line referring to Reviewer
in useReviewActions.ts and update any references to continue using Reviewer
directly (no import), ensuring the composable relies on the global shared-type
auto-imports and aligns with project conventions for types exported from
shared/types.
- Around line 37-40: There are duplicated fetch-error parsing blocks in
useReviewActions.ts (the two catch blocks around lines 37–40 and 57–60); create
a small helper function (e.g., parseFetchError or extractFetchError) inside the
module to accept e: unknown, coerce it to the expected shape and return the
resolved string (fetchErr.data?.data?.errorKey ?? fetchErr.message ??
'unknown'); then replace both catch bodies to call that helper and assign its
return to error.value and return null, keeping existing typing and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c0a39782-b0b6-4b94-a7a9-bb56f9a550e3
📒 Files selected for processing (15)
app/components/dashboard/WaitingOnMeCard.vueapp/components/work-item/ReviewActions.vueapp/components/work-item/WorkItemHeader.vueapp/composables/useReviewActions.tsapp/pages/repos/[owner]/[repo]/work-items/[id].vuei18n/locales/de.jsoni18n/locales/en.jsoni18n/schema.jsonserver/api/dashboard/waiting-on-me.get.tsserver/api/repository/[owner]/[repo]/pulls/[number]/delete-branch.post.tsserver/api/repository/[owner]/[repo]/pulls/[number]/reviews.post.tsserver/api/repository/[owner]/[repo]/pulls/[number]/reviews/[reviewId]/dismiss.post.tsserver/api/repository/[owner]/[repo]/work-items/[id].get.tsshared/types/waiting-on-me.tsshared/types/work-item.ts
Comment on lines
+45
to
+49
| const canSubmit = computed(() => { | ||
| if (submitting.value) return false | ||
| if (bodyRequired.value && !reviewBody.value.trim()) return false | ||
| return true | ||
| }) |
Contributor
There was a problem hiding this comment.
Add client-side guards for self-approval and reviewer permissions.
The submit gating currently checks only loading/body rules, so invalid review actions remain clickable and fail only after the API call. The feature objective calls for preventing self-approval and permission-based disabling with a reason in the UI.
Also applies to: 137-157, 197-207
Comment on lines
+542
to
554
| <!-- Row 5: Review Actions (PR only, open) --> | ||
| <WorkItemReviewActions | ||
| v-if="isPRNotClosed" | ||
| :reviewers="workItem.reviewers ?? []" | ||
| :owner="ownerRef" | ||
| :repo="repoRef" | ||
| :pr-number="workItem.number" | ||
| :work-item-id="workItem.id" | ||
| @reviewed="emit('reviewed')" | ||
| /> | ||
|
|
||
| <!-- Row 6: Merge (PR only) --> | ||
| <div |
Contributor
There was a problem hiding this comment.
Place review actions below merge controls to match the feature contract.
This currently renders the review section above merge, but the linked objective specifies review actions should be placed below the merge section in the Action Sidebar.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/work-item/WorkItemHeader.vue` around lines 542 - 554, The
WorkItemReviewActions block is currently rendered above the merge controls; move
the entire <WorkItemReviewActions ... /> component (including its
v-if="isPRNotClosed" and props reviewers, ownerRef, repoRef, pr-number,
work-item-id and the `@reviewed`="emit('reviewed')" handler) so it appears below
the Merge section (the div marked "<!-- Row 6: Merge (PR only) -->") in
WorkItemHeader.vue so review actions render beneath merge controls to match the
feature contract.
server/api/repository/[owner]/[repo]/pulls/[number]/delete-branch.post.ts
Outdated
Show resolved
Hide resolved
server/api/repository/[owner]/[repo]/pulls/[number]/delete-branch.post.ts
Outdated
Show resolved
Hide resolved
server/api/repository/[owner]/[repo]/pulls/[number]/reviews.post.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+55
to
+60
| try { | ||
| const result = await githubFetchWithToken<GitHubReviewResponse>( | ||
| token, | ||
| `/repos/${owner}/${repo}/pulls/${number}/reviews`, | ||
| { method: 'POST', body: reviewBody }, | ||
| ) |
Contributor
There was a problem hiding this comment.
Missing server-side self-approval enforcement.
This endpoint forwards APPROVE directly without checking whether the caller is the PR author, so the “prevent self-approval” rule can be bypassed via direct API calls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/api/repository/`[owner]/[repo]/pulls/[number]/reviews.post.ts around
lines 55 - 60, The endpoint currently forwards an APPROVE review via
githubFetchWithToken without enforcing the "no self-approval" rule; before
calling githubFetchWithToken in the reviews.post handler, fetch the pull request
data (e.g., GET /repos/{owner}/{repo}/pulls/{number}) to obtain the PR author
and resolve the caller identity from the provided token (the authenticated
user), then if reviewBody.event === 'APPROVE' and the caller login/id equals the
PR author login/id, return a 4xx error (forbidden/bad request) instead of
posting the review; update the logic around githubFetchWithToken/
GitHubReviewResponse to perform this check and short-circuit with an appropriate
error response.
server/api/repository/[owner]/[repo]/pulls/[number]/reviews/[reviewId]/dismiss.post.ts
Outdated
Show resolved
Hide resolved
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 #174
Type of change
Checklist
Summary by CodeRabbit
New Features
Localization