refactor: use RECOUP_ORG_ID for snapshot persistence on merge#269
Merged
sweetmantech merged 9 commits intotestfrom Mar 9, 2026
Merged
Conversation
When the 'Merge All PRs' action button is clicked, the handler now calls upsertAccountSnapshot with the latest snapshotId from thread state and the coding-agent account ID, so new sandboxes start from the post-merge state. - Import upsertAccountSnapshot directly (no HTTP self-call) - Use CODING_AGENT_ACCOUNT_ID constant matching tasks codebase - Gracefully log errors without blocking merge result posting - Added tests: snapshot persistence, skip when no snapshotId, error handling
Replace local CODING_AGENT_ACCOUNT_ID constant with the shared RECOUP_ORG_ID from lib/const.ts so snapshots are stored under the correct Recoup org account. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe merge handler now determines if all PRs merged, sets thread state to "merged" only when true (otherwise "pr_created"), and delegates post-merge cleanup/persistence to a new Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.5)
participant Agent as Coding Agent Handler
participant PRState as PR State Store
participant SnapshotSvc as AccountSnapshot Service
participant Logger as Logger
end
Agent->>PRState: fetch individual PR merge results
PRState-->>Agent: results[]
Agent->>Agent: compute allMerged = results.every(...endsWith("merged"))
alt allMerged
Agent->>SnapshotSvc: upsertAccountSnapshot(account_id, snapshotId, expires_at) (if snapshotId)
SnapshotSvc-->>Agent: success / error
Agent->>PRState: set thread state -> "merged"
Agent->>PRState: delete shared PR state for each repo/branch
alt snapshot error
SnapshotSvc-->>Logger: log snapshot persistence error
end
else not allMerged
Agent->>PRState: set thread state -> "pr_created"
end
Agent->>PRState: post final merge results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Skip snapshot upsert when any GitHub merge fails, and set thread state to "merge_failed" instead of "merged". Adds test for the merge failure case. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/coding-agent/handlers/onMergeAction.ts (1)
51-55:⚠️ Potential issue | 🟡 MinorUnguarded
JSON.parsemay throw on non-JSON error responses.If GitHub returns a non-JSON error (e.g., HTML error page during outage), this will throw
SyntaxErrorand abort the loop, leaving remaining PRs unprocessed and potentially corrupting state.🛡️ Proposed fix: Safe parse with fallback
} else { const errorBody = await response.text(); console.error(`[coding-agent] merge failed for ${pr.repo}#${pr.number}: ${response.status} ${errorBody}`); - const error = JSON.parse(errorBody); - results.push(`${pr.repo}#${pr.number} failed: ${error.message}`); + let message = errorBody; + try { + const error = JSON.parse(errorBody); + message = error.message ?? errorBody; + } catch { + // Non-JSON response; use raw body + } + results.push(`${pr.repo}#${pr.number} failed: ${message}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/coding-agent/handlers/onMergeAction.ts` around lines 51 - 55, The unguarded JSON.parse on the response body in onMergeAction.ts can throw on non-JSON error responses and abort processing; wrap the parse in a try/catch (or attempt JSON.parse and fallback) so that if parsing fails you extract a safe message (e.g., use the raw errorBody or a generic message) and still push `${pr.repo}#${pr.number} failed: <message>` into results, ensuring the loop continues; update the block that reads response.text(), JSON.parse(...) and results.push(...) to use this safe-parse pattern referencing response, errorBody, pr.repo, pr.number and results.
🧹 Nitpick comments (3)
lib/coding-agent/handlers/onMergeAction.ts (3)
65-80: Solid error handling pattern for non-blocking persistence.The conditional guard (
allMerged && state.snapshotId) and non-throwing error handling align well with the design goal of not blocking user feedback on persistence failures.One small readability improvement: the expiration calculation uses a magic number. A named constant would clarify intent.
✨ Optional: Extract expiration constant
+const SNAPSHOT_EXPIRY_MS = 365 * 24 * 60 * 60 * 1000; // 1 year + // Inside the handler: - expires_at: new Date(Date.now() + 365 * 24 * 60 * 60 * 1000).toISOString(), + expires_at: new Date(Date.now() + SNAPSHOT_EXPIRY_MS).toISOString(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/coding-agent/handlers/onMergeAction.ts` around lines 65 - 80, Extract the hard-coded expiration duration into a well-named constant (e.g., ONE_YEAR_MS or SNAPSHOT_TTL_MS) and use it in the expires_at calculation inside the upsertAccountSnapshot call; update the block that checks allMerged && state.snapshotId (and the upsertAccountSnapshot invocation) to compute expires_at with new Date(Date.now() + ONE_YEAR_MS).toISOString(), keeping the same non-throwing error handling and references to RECOUP_ORG_ID, state.snapshotId, and snapshotResult.
15-83: Handler exceeds 50-line guideline; consider extracting helpers.The handler callback spans ~67 lines with multiple responsibilities (merge API calls, result tracking, state management, snapshot persistence). While cohesive, extracting focused helpers would improve testability and align with the 50-line guideline.
♻️ Suggested decomposition
Consider extracting:
mergePullRequest(owner, repo, prNumber, token)– handles a single PR merge API callpersistMergeSnapshot(accountId, snapshotId)– encapsulates the upsert logicThis would reduce the handler to orchestration logic, making each piece independently testable.
// Example: lib/coding-agent/handlers/mergePullRequest.ts export async function mergePullRequest( owner: string, repo: string, prNumber: number, token: string, ): Promise<{ success: boolean; message: string }> { const response = await fetch( `https://api.github.com/repos/${owner}/${repo}/pulls/${prNumber}/merge`, { method: "PUT", headers: { Authorization: `Bearer ${token}`, Accept: "application/vnd.github+json", "X-GitHub-Api-Version": "2022-11-28", }, body: JSON.stringify({ merge_method: "squash" }), }, ); if (response.ok) { return { success: true, message: "merged" }; } const errorBody = await response.text(); let message = errorBody; try { const error = JSON.parse(errorBody); message = error.message ?? errorBody; } catch { // Non-JSON response } return { success: false, message }; }As per coding guidelines: "Keep functions under 50 lines" and "Single responsibility per function".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/coding-agent/handlers/onMergeAction.ts` around lines 15 - 83, The onAction handler registerOnMergeAction is doing too much; extract the merge API call and snapshot persistence into helpers to keep the handler under 50 lines and improve testability. Implement mergePullRequest(owner, repo, prNumber, token) that performs the fetch to the GitHub merge endpoint and returns { success: boolean, message: string } (handle non-JSON error bodies safely), and implement persistMergeSnapshot(accountId, snapshotId) that wraps upsertAccountSnapshot and logs/returns errors; then simplify registerOnMergeAction to iterate state.prs, call mergePullRequest for each PR, collect results, call deleteCodingAgentPRState(state.prs[0].repo, state.branch) if needed, and call persistMergeSnapshot(RECOUP_ORG_ID, state.snapshotId) only when all merges succeed.
58-60: String-based success detection is fragile.The
allMergedcheck depends on the exact message format ending with"merged". If the success message in line 49 changes, this logic silently breaks. Consider tracking merge outcomes with a structured type rather than relying on string inspection.♻️ Optional: Track outcomes explicitly
- const results: string[] = []; + const outcomes: { pr: string; success: boolean; message: string }[] = []; for (const pr of state.prs) { // ... merge logic ... if (response.ok) { - results.push(`${pr.repo}#${pr.number} merged`); + outcomes.push({ pr: `${pr.repo}#${pr.number}`, success: true, message: "merged" }); } else { // ... error handling ... - results.push(`${pr.repo}#${pr.number} failed: ${error.message}`); + outcomes.push({ pr: `${pr.repo}#${pr.number}`, success: false, message: error.message }); } } - const allMerged = results.every(r => r.endsWith("merged")); + const allMerged = outcomes.every(o => o.success); + const results = outcomes.map(o => `${o.pr} ${o.success ? "merged" : `failed: ${o.message}`}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/coding-agent/handlers/onMergeAction.ts` around lines 58 - 60, The current check uses a fragile string-inspection (const allMerged = results.every(r => r.endsWith("merged"))); instead, change the code that populates results to store structured outcomes (e.g., boolean success flags or an object like {id, success, message}), then replace the allMerged computation with an explicit check such as results.every(r => r.success === true) and keep the thread.setState call (thread.setState({ status: allMerged ? "merged" : "merge_failed" })) but driven by the new structured field; update any callers that push into results to push the structured outcome rather than plain message strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/coding-agent/handlers/onMergeAction.ts`:
- Around line 51-55: The unguarded JSON.parse on the response body in
onMergeAction.ts can throw on non-JSON error responses and abort processing;
wrap the parse in a try/catch (or attempt JSON.parse and fallback) so that if
parsing fails you extract a safe message (e.g., use the raw errorBody or a
generic message) and still push `${pr.repo}#${pr.number} failed: <message>` into
results, ensuring the loop continues; update the block that reads
response.text(), JSON.parse(...) and results.push(...) to use this safe-parse
pattern referencing response, errorBody, pr.repo, pr.number and results.
---
Nitpick comments:
In `@lib/coding-agent/handlers/onMergeAction.ts`:
- Around line 65-80: Extract the hard-coded expiration duration into a
well-named constant (e.g., ONE_YEAR_MS or SNAPSHOT_TTL_MS) and use it in the
expires_at calculation inside the upsertAccountSnapshot call; update the block
that checks allMerged && state.snapshotId (and the upsertAccountSnapshot
invocation) to compute expires_at with new Date(Date.now() +
ONE_YEAR_MS).toISOString(), keeping the same non-throwing error handling and
references to RECOUP_ORG_ID, state.snapshotId, and snapshotResult.
- Around line 15-83: The onAction handler registerOnMergeAction is doing too
much; extract the merge API call and snapshot persistence into helpers to keep
the handler under 50 lines and improve testability. Implement
mergePullRequest(owner, repo, prNumber, token) that performs the fetch to the
GitHub merge endpoint and returns { success: boolean, message: string } (handle
non-JSON error bodies safely), and implement persistMergeSnapshot(accountId,
snapshotId) that wraps upsertAccountSnapshot and logs/returns errors; then
simplify registerOnMergeAction to iterate state.prs, call mergePullRequest for
each PR, collect results, call deleteCodingAgentPRState(state.prs[0].repo,
state.branch) if needed, and call persistMergeSnapshot(RECOUP_ORG_ID,
state.snapshotId) only when all merges succeed.
- Around line 58-60: The current check uses a fragile string-inspection (const
allMerged = results.every(r => r.endsWith("merged"))); instead, change the code
that populates results to store structured outcomes (e.g., boolean success flags
or an object like {id, success, message}), then replace the allMerged
computation with an explicit check such as results.every(r => r.success ===
true) and keep the thread.setState call (thread.setState({ status: allMerged ?
"merged" : "merge_failed" })) but driven by the new structured field; update any
callers that push into results to push the structured outcome rather than plain
message strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59a755e7-f46b-4b34-a6eb-40ecb9e00392
⛔ Files ignored due to path filters (1)
lib/coding-agent/__tests__/onMergeAction.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (1)
lib/coding-agent/handlers/onMergeAction.ts
…-api-onmergebutton-update-snapshot-for-recoup_org
When a merge fails, keep status as pr_created so handleFeedback continues accepting thread replies. Also skip PR state cleanup on failure since the user may retry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 `@lib/coding-agent/handlers/onMergeAction.ts`:
- Around line 9-11: Update the function doc comment in onMergeAction.ts to
accurately describe the implementation: replace the line that says it "persists
the latest snapshot via PATCH /api/sandboxes" with text stating that the handler
calls upsertAccountSnapshot to persist the latest snapshot directly (include the
exact function name upsertAccountSnapshot and mention the squash-merge behavior
already described), so the comment matches the actual code path and avoids
confusion for future maintainers.
- Around line 68-73: The upsert of an account snapshot uses RECOUP_ORG_ID (an
org UUID) and a hardcoded expiry; confirm and fix the foreign-key mismatch by
verifying the correct account ID is used (either ensure the Recoup organization
UUID exists in the accounts table or replace RECOUP_ORG_ID with the proper
account ID) before calling upsertAccountSnapshot({ account_id, snapshot_id:
state.snapshotId, ... }), and extract the expiration duration into a named
constant (e.g., SNAPSHOT_EXPIRATION_MS) used to compute expires_at so the value
isn’t hardcoded in onMergeAction.ts; also ensure you handle and surface any
upsert errors rather than failing silently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 537e6cb3-bb99-4811-a79e-a97fc4c30ce4
⛔ Files ignored due to path filters (1)
lib/coding-agent/__tests__/onMergeAction.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (1)
lib/coding-agent/handlers/onMergeAction.ts
Comment on lines
+9
to
+11
| * Squash-merges each PR via the GitHub API, then persists the latest | ||
| * snapshot via PATCH /api/sandboxes so the coding-agent account stays | ||
| * up-to-date. |
There was a problem hiding this comment.
Doc comment doesn't match implementation.
The comment states "persists the latest snapshot via PATCH /api/sandboxes" but the code actually calls upsertAccountSnapshot directly. This could confuse future maintainers.
📝 Proposed fix
* Registers the "Merge All PRs" button action handler on the bot.
-* Squash-merges each PR via the GitHub API, then persists the latest
-* snapshot via PATCH /api/sandboxes so the coding-agent account stays
-* up-to-date.
+* Squash-merges each PR via the GitHub API, then persists the latest
+* snapshot via upsertAccountSnapshot so the coding-agent account stays
+* up-to-date.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/coding-agent/handlers/onMergeAction.ts` around lines 9 - 11, Update the
function doc comment in onMergeAction.ts to accurately describe the
implementation: replace the line that says it "persists the latest snapshot via
PATCH /api/sandboxes" with text stating that the handler calls
upsertAccountSnapshot to persist the latest snapshot directly (include the exact
function name upsertAccountSnapshot and mention the squash-merge behavior
already described), so the comment matches the actual code path and avoids
confusion for future maintainers.
Comment on lines
+68
to
+73
| if (allMerged && state.snapshotId) { | ||
| const snapshotResult = await upsertAccountSnapshot({ | ||
| account_id: RECOUP_ORG_ID, | ||
| snapshot_id: state.snapshotId, | ||
| expires_at: new Date(Date.now() + 365 * 24 * 60 * 60 * 1000).toISOString(), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if RECOUP_ORG_ID is used consistently as an account_id elsewhere
# and check if there's any setup/seeding that creates this account
# Search for how RECOUP_ORG_ID is used across the codebase
rg -n -C3 'RECOUP_ORG_ID'
# Check for any migration or seed files that might create this account
fd -e sql -e ts 'seed|migration' --exec grep -l -i 'account' {} \; 2>/dev/null | head -10Repository: recoupable/api
Length of output: 14386
🏁 Script executed:
#!/bin/bash
# Find the upsertAccountSnapshot function definition
fd -type f -name 'upsertAccountSnapshot.ts'
# Check for migrations and seed files
fd -type f \( -name '*migration*' -o -name '*seed*' \) | head -20
# Search for account schema or type definitions
rg -l 'account.*table\|accounts.*schema' --type ts | head -10Repository: recoupable/api
Length of output: 399
🏁 Script executed:
#!/bin/bash
# Find upsertAccountSnapshot function
find . -name 'upsertAccountSnapshot.ts' | head -5
# Search for account migrations or schema
find . -path '*/migrations/*' -o -path '*/seeds/*' | grep -i account | head -10
# Check for account table schema or types
rg 'account.*schema\|CREATE TABLE.*account' --max-count=20Repository: recoupable/api
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Read the upsertAccountSnapshot function
cat ./lib/supabase/account_snapshots/upsertAccountSnapshot.ts
# Also check for any migrations related to account_snapshots
find . -path '*/migrations/*' -type f | xargs grep -l 'account_snapshot' 2>/dev/null | head -5Repository: recoupable/api
Length of output: 956
🏁 Script executed:
#!/bin/bash
# Find the database types file
find . -name 'database.types.ts' -o -name 'database.ts' | head -5
# Also search for any SQL migrations that define account_snapshots table
find . -name '*.sql' | xargs grep -l 'account_snapshots' 2>/dev/null | head -5Repository: recoupable/api
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Read the database types file and search for account_snapshots definition
cat ./types/database.types.ts | grep -A 30 'account_snapshots'Repository: recoupable/api
Length of output: 1546
🏁 Script executed:
#!/bin/bash
# Look for accounts table schema
cat ./types/database.types.ts | grep -A 50 '"accounts":' | head -60
# Search for any initialization or seed code that creates the RECOUP_ORG_ID account
rg -i 'seed\|init\|create.*account' lib/ --type ts | grep -i recoup | head -10
# Also search for any .sql migration files
find . -name '*.sql' | xargs grep -l 'accounts' 2>/dev/null | head -5Repository: recoupable/api
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Get the entire database.types.ts and search for accounts definition
rg -A 50 'accounts.*Row:' ./types/database.types.ts | head -80
# Search for any supabase migrations folder
find . -type d -name 'migrations' -o -type d -name 'supabase'
# Check if there's initialization code for RECOUP_ORG_ID
rg 'RECOUP_ORG_ID.*account|account.*RECOUP_ORG_ID' --type ts -B 3 -A 3 | head -50Repository: recoupable/api
Length of output: 698
🏁 Script executed:
#!/bin/bash
# Search for accounts table definition in database types
rg 'accounts.*\{' ./types/database.types.ts -A 60 | head -100
# Also check how many lines the file has
wc -l ./types/database.types.ts
# Search for any setup/initialization in the codebase
find . -name 'setup*' -o -name 'init*' -o -name 'seed*' | grep -v node_modules | head -10Repository: recoupable/api
Length of output: 3072
🏁 Script executed:
#!/bin/bash
# Check the sandbox setup
cat ./app/api/sandboxes/setup
# Search for where accounts are created/inserted with RECOUP_ORG_ID
rg 'insert.*account|create.*account|upsert.*account' lib/ --type ts -B 2 -A 2 | grep -A 5 -B 5 'RECOUP' | head -30
# Check if there's any initialization of RECOUP_ORG_ID as an account
rg "id.*RECOUP_ORG_ID|RECOUP_ORG_ID.*id" lib/ --type ts | head -10Repository: recoupable/api
Length of output: 1713
🏁 Script executed:
#!/bin/bash
# Verify the full context of onMergeAction.ts
cat ./lib/coding-agent/handlers/onMergeAction.ts | head -85
# Check the coding guidelines: is this file in lib/!(supabase)/**/*.ts?
# If so, it shouldn't import from lib/supabase/serverClient directlyRepository: recoupable/api
Length of output: 3039
Extract expiration constant and verify RECOUP_ORG_ID account exists in the accounts table.
The account_id is semantically misnamed—RECOUP_ORG_ID is the organization UUID, not an account ID. Since upsertAccountSnapshot enforces a foreign key constraint to the accounts table (per account_snapshots_account_id_fkey), the upsert will fail silently if this UUID doesn't exist as an account record. Verify that the Recoup organization is properly seeded as an account or refactor to use the correct account ID.
Additionally, extract the hardcoded expiration duration into a named constant:
Suggested refactor
+const SNAPSHOT_EXPIRY_MS = 365 * 24 * 60 * 60 * 1000; // 1 year
+
// Persist the latest snapshot only when every PR merged successfully
// so new sandboxes start from the post-merge state.
if (allMerged && state.snapshotId) {
const snapshotResult = await upsertAccountSnapshot({
account_id: RECOUP_ORG_ID,
snapshot_id: state.snapshotId,
- expires_at: new Date(Date.now() + 365 * 24 * 60 * 60 * 1000).toISOString(),
+ expires_at: new Date(Date.now() + SNAPSHOT_EXPIRY_MS).toISOString(),
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/coding-agent/handlers/onMergeAction.ts` around lines 68 - 73, The upsert
of an account snapshot uses RECOUP_ORG_ID (an org UUID) and a hardcoded expiry;
confirm and fix the foreign-key mismatch by verifying the correct account ID is
used (either ensure the Recoup organization UUID exists in the accounts table or
replace RECOUP_ORG_ID with the proper account ID) before calling
upsertAccountSnapshot({ account_id, snapshot_id: state.snapshotId, ... }), and
extract the expiration duration into a named constant (e.g.,
SNAPSHOT_EXPIRATION_MS) used to compute expires_at so the value isn’t hardcoded
in onMergeAction.ts; also ensure you handle and surface any upsert errors rather
than failing silently.
Move PR state cleanup and snapshot persistence into a dedicated handleMergeSuccess function with its own tests. Simplify onMergeAction to delegate on success. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/coding-agent/handlers/onMergeAction.ts (1)
7-9:⚠️ Potential issue | 🟡 MinorDoc comment still describes the old persistence path.
The implementation now delegates to
handleMergeSuccess()and persists viaupsertAccountSnapshot, so thePATCH /api/sandboxesreference is stale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/coding-agent/handlers/onMergeAction.ts` around lines 7 - 9, Update the top doc comment in onMergeAction.ts to reflect the current behavior: remove the stale "PATCH /api/sandboxes" reference and state that the function delegates to handleMergeSuccess() which persists the snapshot via upsertAccountSnapshot; mention that it squash-merges PRs via the GitHub API and then calls handleMergeSuccess() to persist the latest account snapshot using upsertAccountSnapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/coding-agent/handleMergeSuccess.ts`:
- Around line 10-27: handleMergeSuccess currently awaits
deleteCodingAgentPRState and upsertAccountSnapshot without try/catch, so if
either throws the post-merge success flow (marked merged by onMergeAction) can
be interrupted; wrap these operations in error-handling so failures are logged
but do not rethrow — e.g., in handleMergeSuccess use individual try/catch blocks
(or Promise.allSettled) around deleteCodingAgentPRState(state.prs[0].repo,
state.branch) and upsertAccountSnapshot({... snapshot_id: state.snapshotId ...})
to log errors and continue, ensuring the success path completes even when
cleanup/persistence fails.
- Around line 11-12: handleMergeSuccess currently only deletes the PR state for
state.prs[0].repo which leaves keys for other merged repos; update the logic in
handleMergeSuccess to iterate over state.prs (ensure state.prs is defined) and
call deleteCodingAgentPRState for each pr's repo (using
deleteCodingAgentPRState(pr.repo, state.branch) or pr.repo and pr.branch if
branches vary) so cleanup runs for every merged repo instead of just the first
entry.
---
Duplicate comments:
In `@lib/coding-agent/handlers/onMergeAction.ts`:
- Around line 7-9: Update the top doc comment in onMergeAction.ts to reflect the
current behavior: remove the stale "PATCH /api/sandboxes" reference and state
that the function delegates to handleMergeSuccess() which persists the snapshot
via upsertAccountSnapshot; mention that it squash-merges PRs via the GitHub API
and then calls handleMergeSuccess() to persist the latest account snapshot using
upsertAccountSnapshot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 067353f5-d7bd-44e5-bdcd-0f352f9f3ef6
⛔ Files ignored due to path filters (2)
lib/coding-agent/__tests__/handleMergeSuccess.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/coding-agent/__tests__/onMergeAction.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (2)
lib/coding-agent/handleMergeSuccess.tslib/coding-agent/handlers/onMergeAction.ts
…-api-onmergebutton-update-snapshot-for-recoup_org
- Wrap post-merge cleanup in try/catch so merge results always post - Delete PR state for all repos, not just the first - Extract SNAPSHOT_EXPIRY_MS constant - Fix doc comment to match implementation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract snapshot expiration duration to lib/const.ts and update both handleMergeSuccess and updateSnapshotPatchHandler to use it. Changed from 1 year to 7 days. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
CODING_AGENT_ACCOUNT_ID = "coding-agent"with the sharedRECOUP_ORG_IDfromlib/const.tsTest plan
pnpm test lib/coding-agent/__tests__/onMergeAction.test.ts— all 5 tests pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes