Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds admin "merge without waiting" bypass support and ETag-based real-time polling for work items; updates WorkItemHeader PR/merge UI and state handling; exposes new merge metadata from GraphQL; adds a lightweight /check endpoint; fixes i18n; and introduces a broken import in image upload that needs attention. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Admin User
participant UI as WorkItemHeader
participant API as Merge-status API
participant GitHub as GitHub API
User->>UI: Click merge (bypass option shown)
UI->>API: GET merge-status (reads canBypassRules)
API->>GitHub: GraphQL query (includes viewerCanMergeAsAdmin)
GitHub-->>API: returns mergeability + viewerCanMergeAsAdmin
API-->>UI: merge-status with canBypassRules
alt Admin bypass allowed
User->>UI: Confirm merge (bypass)
UI->>API: POST merge (REST)
API->>GitHub: execute merge (may bypass checks)
GitHub-->>API: merge result
API-->>UI: success/failure
end
sequenceDiagram
participant Page as Work Item Page
participant Poll as useWorkItemPolling
participant Check as /check endpoint
participant GitHub as GitHub REST API
participant Cache as Server Cache
Page->>Poll: onMounted → start interval
loop every interval
Poll->>Check: GET /work-items/{id}/check (with ETag)
Check->>GitHub: conditional REST GET (ETag)
alt 304 Not Modified
GitHub-->>Check: 304
Check-->>Poll: {changed:false}
else 200 Modified
GitHub-->>Check: 200 + body
Check->>Cache: invalidate work-item cache
Check-->>Poll: {changed:true}
Poll-->>Page: merge fresh data into workItem
end
end
Page->>Poll: stop on navigation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/composables/useWorkItemPolling.ts (1)
15-35: Guard against overlapping poll ticks.
setIntervalwith an asynctickcan start a new round before the previous one completes, causing duplicate concurrent requests under slow responses.Suggested refactor
+ let inFlight = false + async function tick() { + if (inFlight) return + inFlight = true try { // Lightweight check with ETag — 304 = free, no API cost const { changed } = await requestFetch<{ changed: boolean }>(checkUrl.value) if (!changed) return @@ } catch { // Network error — skip this round } + finally { + inFlight = false + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useWorkItemPolling.ts` around lines 15 - 35, The tick function can overlap when previous async work hasn't finished; add a reentrancy guard (e.g., a boolean like isTickRunning) referenced by tick and start to prevent concurrent runs: at the top of tick return early if isTickRunning is true, set it true before starting async operations and reset it in a finally block so it always clears after success or error; update references to timer/POLL_INTERVAL only if you switch scheduling strategy, but the minimal fix is to add and use isTickRunning inside tick (affecting tick, start, requestFetch and workItem usage remains unchanged).
🤖 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/composables/useWorkItemPolling.ts`:
- Around line 44-49: The empty catch in the onMounted block is causing a lint
failure; replace the empty catch with proper error handling by capturing the
error from requestFetch(checkUrl.value) and logging or handling it (e.g., catch
(err) { console.error('requestFetch failed for', checkUrl.value, err) } or call
an existing error handler), then continue to call start(); update the onMounted
async function around requestFetch/checkUrl and ensure requestFetch, checkUrl,
and start references remain unchanged.
---
Nitpick comments:
In `@app/composables/useWorkItemPolling.ts`:
- Around line 15-35: The tick function can overlap when previous async work
hasn't finished; add a reentrancy guard (e.g., a boolean like isTickRunning)
referenced by tick and start to prevent concurrent runs: at the top of tick
return early if isTickRunning is true, set it true before starting async
operations and reset it in a finally block so it always clears after success or
error; update references to timer/POLL_INTERVAL only if you switch scheduling
strategy, but the minimal fix is to add and use isTickRunning inside tick
(affecting tick, start, requestFetch and workItem usage remains unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 481fa364-b31b-4534-8afc-2d643b2d12af
📒 Files selected for processing (10)
app/components/work-item/WorkItemHeader.vueapp/composables/useWorkItemPolling.tsapp/pages/repos/[owner]/[repo]/work-items/[id].vuei18n/locales/de.jsoni18n/locales/en.jsoni18n/schema.jsonserver/api/repository/[owner]/[repo]/pulls/[number]/merge-status.get.tsserver/api/repository/[owner]/[repo]/work-items/[id]/check.get.tsserver/api/upload/image.post.tsshared/types/merge.ts
💤 Files with no reviewable changes (1)
- server/api/upload/image.post.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/composables/useWorkItemPolling.ts`:
- Around line 23-25: The current guard skips updating workItem.value when it is
initially null/undefined, losing valid refresh payloads; in the
useWorkItemPolling composable change the conditional logic around workItem.value
so that when fresh is present you always assign/merge it into workItem.value
(e.g. merge with an empty object or assign fresh directly) instead of requiring
an existing workItem.value; apply this fix to both occurrences that reference
workItem.value and fresh (the block around the first if (fresh &&
workItem.value) and the similar block at lines 57-59) so fresh updates are not
dropped when workItem.value was previously null.
- Around line 18-19: The polling code in useWorkItemPolling is destructuring
const { changed } = await requestFetch(checkUrl.value) which throws a TypeError
when requestFetch returns undefined for HTTP 304; change the call so you
explicitly inspect the raw response/status before destructuring (e.g., await
requestFetch(...) -> resp; if !resp || resp.status === 304 return early) and
only then access resp.data or destructure changed; update any dependent logic in
useWorkItemPolling and keep checkUrl.value as the request target so we avoid
using exceptions to handle the 304 case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 858323cd-ebed-4f99-9a31-0bdd39fadfdc
📒 Files selected for processing (1)
app/composables/useWorkItemPolling.ts
Comment on lines
+18
to
+19
| const { changed } = await requestFetch<{ changed: boolean }>(checkUrl.value) | ||
| if (!changed) return |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Nuxt 4.3.1 (useRequestFetch/ofetch), what is the behavior of $fetch<{ changed: boolean }>(url) when the server responds with HTTP 304 and an empty body—does it throw, return undefined, or parse to an object?
💡 Result:
In Nuxt 4.3.1, $fetch is Nuxt’s wrapper around ofetch. (nuxt.com)
In ofetch, HTTP 304 is treated as a “null body” response (nullBodyResponses includes 304), so ofetch skips reading/parsing the body entirely. (github.com)
As a result:
$fetch<{ changed: boolean }>(url)does not throw just because the status is 304 (errors are only thrown for status>= 400unless you opt out). (github.com)- It returns
undefined(because_datais never set when there’s no body, and$fetchreturnsr._data). (github.com) - It does not parse to an object like
{ changed: false }automatically. (github.com)
Citations:
- 1: https://nuxt.com/docs/4.x/api/utils?utm_source=openai
- 2: https://github.com/unjs/ofetch/blob/main/src/fetch.ts
- 3: https://github.com/unjs/ofetch/blob/main/src/fetch.ts
- 4: https://github.com/unjs/ofetch/blob/main/src/fetch.ts
- 5: https://github.com/unjs/ofetch/blob/main/src/fetch.ts
🏁 Script executed:
cat -n app/composables/useWorkItemPolling.ts | head -80Repository: flumen-dev/flumen.dev
Length of output: 2421
Handle HTTP 304 explicitly instead of relying on exception handling.
Line 18 destructures from a response that becomes undefined when the server returns a 304 (no body). This throws a TypeError that falls into the catch block—functionally correct for this polling logic, but swallows the expected response together with real network errors and obfuscates intent.
A defensive check avoids unnecessary exception overhead:
Proposed fix
// Lightweight check with ETag — 304 = free, no API cost
- const { changed } = await requestFetch<{ changed: boolean }>(checkUrl.value)
- if (!changed) return
+ const response = await requestFetch<{ changed: boolean }>(checkUrl.value)
+ if (!response?.changed) return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { changed } = await requestFetch<{ changed: boolean }>(checkUrl.value) | |
| if (!changed) return | |
| const response = await requestFetch<{ changed: boolean }>(checkUrl.value) | |
| if (!response?.changed) return |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/composables/useWorkItemPolling.ts` around lines 18 - 19, The polling code
in useWorkItemPolling is destructuring const { changed } = await
requestFetch(checkUrl.value) which throws a TypeError when requestFetch returns
undefined for HTTP 304; change the call so you explicitly inspect the raw
response/status before destructuring (e.g., await requestFetch(...) -> resp; if
!resp || resp.status === 304 return early) and only then access resp.data or
destructure changed; update any dependent logic in useWorkItemPolling and keep
checkUrl.value as the request target so we avoid using exceptions to handle the
304 case.
Comment on lines
+23
to
+25
| if (fresh && workItem.value) { | ||
| workItem.value = { ...workItem.value, ...fresh } | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t drop updates when workItem.value is initially null/undefined.
Line 23 and Line 57 currently skip assignment unless workItem.value already exists. That can lose legitimate refresh results.
Proposed fix
- if (fresh && workItem.value) {
- workItem.value = { ...workItem.value, ...fresh }
- }
+ if (fresh) {
+ workItem.value = workItem.value
+ ? { ...workItem.value, ...fresh }
+ : fresh
+ }
...
- if (fresh && workItem.value) {
- workItem.value = { ...workItem.value, ...fresh }
- }
+ if (fresh) {
+ workItem.value = workItem.value
+ ? { ...workItem.value, ...fresh }
+ : fresh
+ }Also applies to: 57-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/composables/useWorkItemPolling.ts` around lines 23 - 25, The current
guard skips updating workItem.value when it is initially null/undefined, losing
valid refresh payloads; in the useWorkItemPolling composable change the
conditional logic around workItem.value so that when fresh is present you always
assign/merge it into workItem.value (e.g. merge with an empty object or assign
fresh directly) instead of requiring an existing workItem.value; apply this fix
to both occurrences that reference workItem.value and fresh (the block around
the first if (fresh && workItem.value) and the similar block at lines 57-59) so
fresh updates are not dropped when workItem.value was previously null.
This was referenced Mar 15, 2026
Merged
Merged
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 #231
Type of change
Checklist
Summary by CodeRabbit
New Features
Performance
Localization