Conversation
|
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 (1)
📝 WalkthroughWalkthroughAdds a pending-PR UX and server workflow: lists branches with user commits, lets users create PRs (including draft + issue linking), and adds API endpoints to create PRs and mark drafts ready; integrates draft state into merge status composable and work-item header with localization. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client as Web Client
participant Server as Flumen Server
participant GitHub as GitHub API
User->>Client: Open repo page
Client->>Server: GET /api/repository/{owner}/{repo}/pending-branches
Server->>GitHub: List branches, default branch, open PRs
Server->>GitHub: Compare branch -> default (up to 5)
Server-->>Client: PendingBranch[] (branch, aheadBy, defaultBranch)
User->>Client: Click "Create PR"
Client->>Client: Derive title, detect linked issue, edit form
Client->>Server: POST /api/pull-requests/create {repo, head, base, title, body, draft}
Server->>GitHub: Create pull request (with draft flag)
GitHub-->>Server: PR details (number, url, node_id)
Server-->>Client: Created PR info
Client->>Client: Show toast, navigate to PR
sequenceDiagram
participant User
participant Client as Web Client
participant Server as Flumen Server
participant GitHub as GitHub API
User->>Client: View work item with draft PR
Client->>Client: Show draft hint + "Mark Ready" button
User->>Client: Click "Mark Ready"
Client->>Server: POST /api/repository/{owner}/{repo}/pulls/{number}/ready
Server->>GitHub: Query PR node ID
GitHub-->>Server: Return PR node ID
Server->>GitHub: markPullRequestReadyForReview mutation
GitHub-->>Server: Return updated PR (isDraft)
Server-->>Client: Return isDraft status
Client->>Client: Update UI, show toast, emit ciStatusChanged
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
app/components/repo/PendingPrBanner.vue (1)
108-110: Consider persisting dismissed branches across page reloads.The dismissed set is stored in component state, so dismissed branches will reappear after a page refresh. If this is intentional (temporary dismissal), this is fine. If users expect dismissed branches to stay hidden, consider using
localStorageor a similar persistence mechanism.Optional: Persist dismissed branches to localStorage
+const DISMISSED_KEY = `flumen:dismissed-branches:${props.owner}/${props.repo}` + +const dismissed = ref<Set<string>>(new Set( + JSON.parse(localStorage.getItem(DISMISSED_KEY) || '[]') +)) + function dismiss(branch: string) { dismissed.value = new Set([...dismissed.value, branch]) + localStorage.setItem(DISMISSED_KEY, JSON.stringify([...dismissed.value])) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/repo/PendingPrBanner.vue` around lines 108 - 110, The dismiss logic in PendingPrBanner.vue currently only updates the in-memory dismissed Set (dismissed.value), so dismissed branches reappear after reload; modify the component to persist dismissed branches to localStorage (e.g., store an array or comma-separated string) whenever dismiss(branch: string) updates dismissed.value, and restore that persisted set on component mount/created initialization so the dismissed state survives page reloads; reference the dismissed reactive variable and the dismiss(branch: string) function to add persistence and a restore step (and handle parsing/empty-state safely).
🤖 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/repo/PendingPrBanner.vue`:
- Around line 218-225: Replace the hardcoded "Closes #" in PendingPrBanner.vue
(the span that renders UIcon and Closes #{{ selectedIssue.value }}) with an i18n
lookup (e.g., use the component's i18n function or $t call like
t('pendingPr.closesIssue', { number: selectedIssue.value }) or equivalent
interpolation) so the text is translated; then add the new key
pendingPr.closesIssue to all locale files and update the i18n schema/typing to
include it.
In `@server/api/issues/branch-status.get.ts`:
- Around line 37-45: The GraphQL query BranchStatus currently filters PRs only
by headRefName and then treats totalCount > 0 as hasOpenPr, causing false
positives from forks; update the BranchStatus query to fetch pullRequests {
nodes { headRepository { name owner { login } } headRefName } } (keep first: 1
or more) and then change the hasOpenPr check to verify the returned node's
headRepository.owner.login === $owner and headRepository.name === $repo (or
equivalent repo identifier) before setting hasOpenPr; apply the same change to
the other occurrence flagged (lines 135-136) so we only count PRs that originate
from this repository, not forks.
In `@server/api/pull-requests/create.post.ts`:
- Line 19: The code discards the contributor login from getSessionToken (const {
token, login: _login }) but later builds the PR head using owner instead of the
contributor, breaking forked PRs; update the destructuring to keep the
contributor login (e.g., const { token, login }) and when constructing the head
parameter (the variable built around head/owner/branch) prefix the branch with
the contributor login for forks (use login:branch) rather than owner:branch,
falling back to owner only when appropriate; locate this in the create.post
handler where getSessionToken, token, login/_login, owner, and head are
referenced and replace the usage of owner with the contributor login when
qualifying fork heads.
In `@server/api/repository/`[owner]/[repo]/pending-branches.get.ts:
- Around line 40-54: The current implementation limits candidate branches with
withoutPr.slice(0, 5) before checking ahead_by and author, which can miss
qualifying branches; change the logic to evaluate all candidates (use withoutPr
without the initial slice or iterate fully) and only apply the cap after
filtering (e.g., collect matches via the githubFetchWithToken checks in the
Promise.allSettled or a loop, pushing into results when data.ahead_by > 0 and
userCommitted is true) and then return or use results.slice(0, 5) (or stop early
once results.length reaches 5) so you still limit API returns but only after
filtering by aheadBy and login checks. Ensure you keep references to
githubFetchWithToken, token, owner, repo, defaultBranch, login and results.push
when implementing the change.
- Around line 44-46: The compare API call interpolates raw branch names into the
path in the githubFetchWithToken call inside pending-branches.get.ts (the line
with `/repos/${owner}/${repo}/compare/${defaultBranch}...${branch}`), which
breaks for refs containing `/`; percent-encode the refs before constructing the
path (e.g., apply encodeURIComponent or an equivalent encoder to defaultBranch
and branch) so the compare URL is valid and branches with slashes won't cause
the request to fail.
---
Nitpick comments:
In `@app/components/repo/PendingPrBanner.vue`:
- Around line 108-110: The dismiss logic in PendingPrBanner.vue currently only
updates the in-memory dismissed Set (dismissed.value), so dismissed branches
reappear after reload; modify the component to persist dismissed branches to
localStorage (e.g., store an array or comma-separated string) whenever
dismiss(branch: string) updates dismissed.value, and restore that persisted set
on component mount/created initialization so the dismissed state survives page
reloads; reference the dismissed reactive variable and the dismiss(branch:
string) function to add persistence and a restore step (and handle
parsing/empty-state safely).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbdfe84f-86df-4d8a-908b-b9c54f8e0ff7
📒 Files selected for processing (11)
app/components/repo/PendingPrBanner.vueapp/components/work-item/WorkItemHeader.vueapp/composables/useMergeStatus.tsapp/pages/repos/[owner]/[repo]/index.vuei18n/locales/de.jsoni18n/locales/en.jsoni18n/schema.jsonserver/api/issues/branch-status.get.tsserver/api/pull-requests/create.post.tsserver/api/repository/[owner]/[repo]/pending-branches.get.tsserver/api/repository/[owner]/[repo]/pulls/[number]/ready.post.ts
| query BranchStatus($owner: String!, $repo: String!, $branchRef: String!, $branchName: String!) { | ||
| repository(owner: $owner, name: $repo) { | ||
| viewerPermission | ||
| defaultBranchRef { name } | ||
| url | ||
| ref(qualifiedName: $branchRef) { name } | ||
| pullRequests(headRefName: $branchName, states: OPEN, first: 1) { | ||
| totalCount | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub GraphQL repository.pullRequests headRefNamefilter: when multiple forks have open PRs with the same branch name, does filtering only byheadRefName distinguish the head repository owner, or do you need to inspect the PR's head repository separately?
💡 Result:
Filtering repository.pullRequests(headRefName: "...") matches only the branch name, and does not distinguish which fork/owner the PR’s head branch comes from. (2fd.github.io)
If multiple forks have open PRs whose head branch has the same name, they can all match that filter—so to distinguish them you must inspect each PR’s headRepository / headRepositoryOwner (or headRepository { nameWithOwner }) in the returned nodes. (2fd.github.io)
Citations:
- 1: https://2fd.github.io/graphdoc/github/repository.doc.html
- 2: https://2fd.github.io/graphdoc/github/pullrequest.doc.html
Filter by both branch name and head repository to avoid false positives across forks.
The query filters pull requests only by headRefName. When multiple forks have open PRs with the same branch name (e.g., issue-123), all will match this filter regardless of fork owner. Since the code only checks totalCount > 0, a PR from another fork will incorrectly set hasOpenPr true for a contributor whose branch has no PR yet. Inspect the PR's headRepository or headRepositoryOwner to ensure the PR belongs to the current repository context.
Also applies to: 135-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/api/issues/branch-status.get.ts` around lines 37 - 45, The GraphQL
query BranchStatus currently filters PRs only by headRefName and then treats
totalCount > 0 as hasOpenPr, causing false positives from forks; update the
BranchStatus query to fetch pullRequests { nodes { headRepository { name owner {
login } } headRefName } } (keep first: 1 or more) and then change the hasOpenPr
check to verify the returned node's headRepository.owner.login === $owner and
headRepository.name === $repo (or equivalent repo identifier) before setting
hasOpenPr; apply the same change to the other occurrence flagged (lines 135-136)
so we only count PRs that originate from this repository, not forks.
| } | ||
|
|
||
| export default defineEventHandler(async (event) => { | ||
| const { token, login: _login } = await getSessionToken(event) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub REST create pull request headparameter: for a PR opened from a fork, shouldheaduse the contributor login (for exampleuser:branch) rather than the base repository owner?
💡 Result:
Yes. When creating a PR into the base repo, the REST API’s head is the source branch; for a PR from a fork you should namespace it with the fork owner, e.g. head: "contributorLogin:branchName" (or org:branch). If the branch is in the same repository, head can just be "branchName". For some cross-repo cases where both repos are owned by the same organization, you may also need to set head_repo. [1]
Sources: GitHub REST “Create a pull request” docs [1].
🏁 Script executed:
cd /repo && head -50 server/api/pull-requests/create.post.ts | cat -nRepository: flumen-dev/flumen.dev
Length of output: 120
🏁 Script executed:
pwd && find . -name "create.post.ts" -type f 2>/dev/null | head -5Repository: flumen-dev/flumen.dev
Length of output: 160
🏁 Script executed:
head -50 ./server/api/pull-requests/create.post.ts | cat -nRepository: flumen-dev/flumen.dev
Length of output: 1733
Use the contributor login when qualifying fork heads.
Line 19 discards the contributor's login, but line 32 qualifies the head parameter with owner (the base repository owner, not the fork owner). For PRs opened from forks, the head parameter should use the contributor's login (e.g., "contributorLogin:branchName"). The current logic breaks for contributors without write access to the base repository.
The inline comment on line 31 states the intent ("prefix with user login") but the code does not match it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/api/pull-requests/create.post.ts` at line 19, The code discards the
contributor login from getSessionToken (const { token, login: _login }) but
later builds the PR head using owner instead of the contributor, breaking forked
PRs; update the destructuring to keep the contributor login (e.g., const {
token, login }) and when constructing the head parameter (the variable built
around head/owner/branch) prefix the branch with the contributor login for forks
(use login:branch) rather than owner:branch, falling back to owner only when
appropriate; locate this in the create.post handler where getSessionToken,
token, login/_login, owner, and head are referenced and replace the usage of
owner with the contributor login when qualifying fork heads.
| // 3. Check ahead count — only show branches with commits (limit to 5 to save API calls) | ||
| const results: PendingBranch[] = [] | ||
|
|
||
| await Promise.allSettled(withoutPr.slice(0, 5).map(async (branch) => { | ||
| const { data } = await githubFetchWithToken<{ ahead_by: number, commits: { author: { login: string } | null }[] }>( | ||
| token, `/repos/${owner}/${repo}/compare/${defaultBranch}...${branch}`, | ||
| ) | ||
| if (data.ahead_by === 0) return | ||
|
|
||
| // Only show if the current user authored at least one commit | ||
| const userCommitted = data.commits.some(c => c.author?.login === login) | ||
| if (!userCommitted) return | ||
|
|
||
| results.push({ branch, aheadBy: data.ahead_by, defaultBranch }) | ||
| })) |
There was a problem hiding this comment.
Limit after filtering, not before.
Line 43 caps the search to the first five candidate branches before you know whether any of them are ahead or authored by the current user. On a busy repo, the user's pending branch can sit later in the list, so this endpoint returns [] even though a qualifying branch exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/api/repository/`[owner]/[repo]/pending-branches.get.ts around lines 40
- 54, The current implementation limits candidate branches with
withoutPr.slice(0, 5) before checking ahead_by and author, which can miss
qualifying branches; change the logic to evaluate all candidates (use withoutPr
without the initial slice or iterate fully) and only apply the cap after
filtering (e.g., collect matches via the githubFetchWithToken checks in the
Promise.allSettled or a loop, pushing into results when data.ahead_by > 0 and
userCommitted is true) and then return or use results.slice(0, 5) (or stop early
once results.length reaches 5) so you still limit API returns but only after
filtering by aheadBy and login checks. Ensure you keep references to
githubFetchWithToken, token, owner, repo, defaultBranch, login and results.push
when implementing the change.
1 participant
Closes #234
Now we can create pull requests if we push a new branch (fork)
Summary by CodeRabbit
New Features
Localization