My Work: batch-capped resync + progress modal, plus identity_tasks schema - #14
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ress UI Two runs against production both showed the same signature: many real AI successes over several minutes, then every subsequent summarizer call failing instantly for the rest of the PR list — a rate limit or per-request ceiling, not a code defect (confirmed working correctly otherwise). Caps summarization at 20 PRs per runBackfill invocation and paces calls 500ms apart, both overridable via opts for tests. A capped run reports summaryBudgetExhausted so callers know more remain. The frontend now drives this transparently: clicking "Sync GitHub" chains batched requests automatically (capped at 10 batches client-side against a never-converges edge case) while showing a disabled, live "Syncing… N summarized" button state, replacing the previous one-shot fire-and-forget click with no progress signal.
…nc GitHub Replaces the button-only "Syncing... N summarized" text with a proper centered modal (dimmed backdrop, spinner, progress bar) showing structured PR-summary count against the total PR count. runBackfill now returns structuredCount — how many of `prs` currently have a structured summary (already-had-one + just-got-one this call), correctly excluding any PR that fell back to excerpt because its AI call failed, so the bar never claims a PR is "done" when it isn't. The button itself stays disabled with a spinner during a sync; the modal carries the live count.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
canopy | 601728a | Commit Preview URL Branch Preview URL |
Jul 04 2026, 07:23 AM |
📝 WalkthroughWalkthroughAdds a new ChangesIdentity tasks and admin backfill batching/progress
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as web/src/main.ts
participant API as /admin/backfill
participant Backfill as runBackfill
participant Summarizer
User->>Frontend: Click "Sync GitHub"
Frontend->>Frontend: init state.backfillSync, rerender
loop until summaryBudgetExhausted=false or max batches
Frontend->>API: POST /admin/backfill
API->>Backfill: runBackfill(opts)
loop per PR (up to summaryBatchLimit)
Backfill->>Summarizer: summarize PR
Summarizer-->>Backfill: structured summary
Backfill->>Backfill: increment structuredCount, delay
end
Backfill-->>API: {structuredCount, summaryBudgetExhausted, ...}
API-->>Frontend: response
Frontend->>Frontend: update state.backfillSync, rerender modal
end
Frontend->>Frontend: clear backfillSync, flash completion message
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
migrations/0016_identity_tasks.sql (1)
7-13: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider a CHECK constraint on
status.The column comment documents
statusas'pending' | 'resolved', but nothing in the schema enforces it. ACHECKconstraint would guard against invalid values from any future raw-SQL write path bypassing the app layer.♻️ Optional constraint addition
- status TEXT NOT NULL DEFAULT 'pending', -- 'pending' | 'resolved' + status TEXT NOT NULL DEFAULT 'pending' CHECK (status IN ('pending', 'resolved')),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@migrations/0016_identity_tasks.sql` around lines 7 - 13, Add a CHECK constraint to the identity_tasks schema so the status column is restricted to the documented values only. Update the CREATE TABLE definition in the migration for identity_tasks to enforce that status can only be 'pending' or 'resolved', keeping the constraint alongside the existing status default and related columns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tools/backfill.ts`:
- Around line 232-258: The delay logic in backfill.ts still runs after the last
successful summarizer call when the PR list ends before summaryBatchLimit, so
update the pacing around the summarize loop in the backfill workflow to avoid
any trailing wait. Adjust the timing near storePrSummary and summarized so the
delay is applied before each call instead of after it, skipping only the first
iteration, and keep the existing summaryBudgetExhausted behavior intact. Make
sure the change matches the intent in the surrounding comment that no delay
should run when nothing follows.
In `@web/src/main.ts`:
- Around line 300-323: The admin backfill flow in runAdminBackfillLoop is
missing a way to escape a hanging batch because postJson/adminBackfill has no
timeout and the backfill overlay cannot be dismissed. Add a cancel or timeout
path so the user can stop the sync while it is running, and make
runAdminBackfillLoop handle that case by clearing state.backfillSync,
rerendering, and exiting cleanly. Wire the UI in the backfill modal to an
explicit cancel/dismiss action and ensure the adminBackfill/postJson path
respects abort or timeout signals.
---
Nitpick comments:
In `@migrations/0016_identity_tasks.sql`:
- Around line 7-13: Add a CHECK constraint to the identity_tasks schema so the
status column is restricted to the documented values only. Update the CREATE
TABLE definition in the migration for identity_tasks to enforce that status can
only be 'pending' or 'resolved', keeping the constraint alongside the existing
status default and related columns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c29e5a38-8ecd-42db-a9c7-82631a6c75a3
📒 Files selected for processing (10)
migrations/0016_identity_tasks.sqlshared/rows.tssrc/tools/backfill.tstest/apply-migrations.tstest/backfill.test.tstest/identity-schema.test.tstest/render.mywork.test.tsweb/src/api.tsweb/src/main.tsweb/src/render.ts
Comment on lines
+232
to
+258
| if (alreadyStructured) { | ||
| structuredCount++; | ||
| continue; | ||
| } | ||
|
|
||
| if (summarized >= summaryBatchLimit) { | ||
| summaryBudgetExhausted = true; | ||
| continue; | ||
| } | ||
|
|
||
| const parsed = JSON.parse(ev.raw) as { pr: { number: number; title: string; body: string | null } }; | ||
| const stored = await storePrSummary(env.DB, summarizer, { | ||
| semantic_key: ev.semantic_key, | ||
| pr_number: parsed.pr.number, | ||
| title: parsed.pr.title, | ||
| body: parsed.pr.body ?? "", | ||
| }); | ||
| summarized++; | ||
| // storePrSummary can still fall back to excerpt if the AI call failed — | ||
| // only count it toward "done" if it actually landed in structured form. | ||
| if (parseStructuredSummary(stored.summary) !== null) structuredCount++; | ||
|
|
||
| // Pace summarizer calls so one invocation doesn't burst past whatever | ||
| // limit caused the wall above — skip the trailing delay once the batch | ||
| // is done, nothing follows it. | ||
| if (summarized < summaryBatchLimit && summaryCallDelayMs > 0) { | ||
| await new Promise((resolve) => setTimeout(resolve, summaryCallDelayMs)); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Trailing delay fires even when nothing follows it.
The skip-condition summarized < summaryBatchLimit only prevents the delay when the batch cap is hit — it doesn't account for the prList simply running out before the cap (the common case, since the default cap is 20). In that scenario, the delay still fires after the very last summarizer call in the whole invocation, contradicting the adjacent comment ("skip the trailing delay once the batch is done, nothing follows it") and adding an avoidable up-to-500ms tail latency to every ordinary Sync. The test at test/backfill.test.ts:230-241 only asserts elapsed >= 40, so it wouldn't catch two delays firing instead of one.
Moving the delay to run before each call (skipped on the first) removes the trailing-delay case entirely, since it's never added after the actual last call.
⏱️ Proposed fix: delay before call, not after
const parsed = JSON.parse(ev.raw) as { pr: { number: number; title: string; body: string | null } };
+ // Pace summarizer calls so one invocation doesn't burst past whatever
+ // limit caused the wall above. Delaying *before* (skipping on the first
+ // call) means there's never a trailing delay after the actual last call.
+ if (summarized > 0 && summaryCallDelayMs > 0) {
+ await new Promise((resolve) => setTimeout(resolve, summaryCallDelayMs));
+ }
const stored = await storePrSummary(env.DB, summarizer, {
semantic_key: ev.semantic_key,
pr_number: parsed.pr.number,
title: parsed.pr.title,
body: parsed.pr.body ?? "",
});
summarized++;
// storePrSummary can still fall back to excerpt if the AI call failed —
// only count it toward "done" if it actually landed in structured form.
if (parseStructuredSummary(stored.summary) !== null) structuredCount++;
-
- // Pace summarizer calls so one invocation doesn't burst past whatever
- // limit caused the wall above — skip the trailing delay once the batch
- // is done, nothing follows it.
- if (summarized < summaryBatchLimit && summaryCallDelayMs > 0) {
- await new Promise((resolve) => setTimeout(resolve, summaryCallDelayMs));
- }
}📝 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
| if (alreadyStructured) { | |
| structuredCount++; | |
| continue; | |
| } | |
| if (summarized >= summaryBatchLimit) { | |
| summaryBudgetExhausted = true; | |
| continue; | |
| } | |
| const parsed = JSON.parse(ev.raw) as { pr: { number: number; title: string; body: string | null } }; | |
| const stored = await storePrSummary(env.DB, summarizer, { | |
| semantic_key: ev.semantic_key, | |
| pr_number: parsed.pr.number, | |
| title: parsed.pr.title, | |
| body: parsed.pr.body ?? "", | |
| }); | |
| summarized++; | |
| // storePrSummary can still fall back to excerpt if the AI call failed — | |
| // only count it toward "done" if it actually landed in structured form. | |
| if (parseStructuredSummary(stored.summary) !== null) structuredCount++; | |
| // Pace summarizer calls so one invocation doesn't burst past whatever | |
| // limit caused the wall above — skip the trailing delay once the batch | |
| // is done, nothing follows it. | |
| if (summarized < summaryBatchLimit && summaryCallDelayMs > 0) { | |
| await new Promise((resolve) => setTimeout(resolve, summaryCallDelayMs)); | |
| if (alreadyStructured) { | |
| structuredCount++; | |
| continue; | |
| } | |
| if (summarized >= summaryBatchLimit) { | |
| summaryBudgetExhausted = true; | |
| continue; | |
| } | |
| const parsed = JSON.parse(ev.raw) as { pr: { number: number; title: string; body: string | null } }; | |
| // Pace summarizer calls so one invocation doesn't burst past whatever | |
| // limit caused the wall above. Delaying *before* (skipping on the first | |
| // call) means there's never a trailing delay after the actual last call. | |
| if (summarized > 0 && summaryCallDelayMs > 0) { | |
| await new Promise((resolve) => setTimeout(resolve, summaryCallDelayMs)); | |
| } | |
| const stored = await storePrSummary(env.DB, summarizer, { | |
| semantic_key: ev.semantic_key, | |
| pr_number: parsed.pr.number, | |
| title: parsed.pr.title, | |
| body: parsed.pr.body ?? "", | |
| }); | |
| summarized++; | |
| // storePrSummary can still fall back to excerpt if the AI call failed — | |
| // only count it toward "done" if it actually landed in structured form. | |
| if (parseStructuredSummary(stored.summary) !== null) structuredCount++; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tools/backfill.ts` around lines 232 - 258, The delay logic in backfill.ts
still runs after the last successful summarizer call when the PR list ends
before summaryBatchLimit, so update the pacing around the summarize loop in the
backfill workflow to avoid any trailing wait. Adjust the timing near
storePrSummary and summarized so the delay is applied before each call instead
of after it, skipping only the first iteration, and keep the existing
summaryBudgetExhausted behavior intact. Make sure the change matches the intent
in the surrounding comment that no delay should run when nothing follows.
Comment on lines
+300
to
+323
| async function runAdminBackfillLoop(): Promise<void> { | ||
| let summarizedSoFar = 0; | ||
| let batchesSoFar = 0; | ||
| let last: Awaited<ReturnType<typeof adminBackfill>> | null = null; | ||
| try { | ||
| do { | ||
| last = await adminBackfill(); | ||
| batchesSoFar++; | ||
| summarizedSoFar += last.summarized; | ||
| state.backfillSync = { structuredCount: last.structuredCount, prsTotal: last.prs }; | ||
| rerender(); | ||
| } while (last.summaryBudgetExhausted && batchesSoFar < MAX_BACKFILL_BATCHES); | ||
|
|
||
| state.backfillSync = null; | ||
| const more = last.summaryBudgetExhausted ? " — more remain, click Sync again" : ""; | ||
| flash(`Synced: ${last.captured} captured, ${last.unchanged} unchanged, ${summarizedSoFar} summaries updated${more}`); | ||
| loadMyWork(); | ||
| } catch (e) { | ||
| state.backfillSync = null; | ||
| if (e instanceof Unauthorized) { state.view = "auth"; state.authStep = "login"; rerender(); return; } | ||
| flash(e instanceof ApiError ? e.message : "Sync failed"); | ||
| rerender(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -B2 -A15 'function postJson' web/src/api.ts
rg -n 'AbortController|AbortSignal|timeout' web/src/api.tsRepository: SaplingLearn/canopy
Length of output: 797
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first
git ls-files web/src | sed -n '1,200p'
echo
echo '--- main.ts outline ---'
ast-grep outline web/src/main.ts --view expanded || true
echo
echo '--- relevant lines around runAdminBackfillLoop ---'
sed -n '260,360p' web/src/main.ts
echo
echo '--- search for backfillSync and cancel/escape affordances ---'
rg -n 'backfillSync|adminBackfill|cancel|escape|AbortController|AbortSignal|timeout' web/srcRepository: SaplingLearn/canopy
Length of output: 8027
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1480,1525p' web/src/render.tsRepository: SaplingLearn/canopy
Length of output: 2878
Add a timeout or cancel path for admin backfill.
postJson() uses fetch() with no timeout, and the full-screen backfill modal has no dismiss/cancel action. If one batch hangs, the sync flow stays blocked behind the overlay with no user escape.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/main.ts` around lines 300 - 323, The admin backfill flow in
runAdminBackfillLoop is missing a way to escape a hanging batch because
postJson/adminBackfill has no timeout and the backfill overlay cannot be
dismissed. Add a cancel or timeout path so the user can stop the sync while it
is running, and make runAdminBackfillLoop handle that case by clearing
state.backfillSync, rerendering, and exiting cleanly. Wire the UI in the
backfill modal to an explicit cancel/dismiss action and ensure the
adminBackfill/postJson path respects abort or timeout signals.
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
runBackfillnow caps AI summarization at 20 PRs per invocation and paces calls 500ms apart, after two live runs both showed the same "many successes then instant mass-failure" signature (a rate/duration limit, not a code defect). ReportssummaryBudgetExhaustedso callers know more remain.structuredCountfield on the backfill result (correctly excluding any PR that fell back to excerpt because its AI call failed).feat(triage): identity_tasks store for unknown-login triage (0016): a new migration + schema for identity task tracking, committed to this branch by a separate session.Test plan
npm test— 319/319 passing (My Work resync/progress work); full suite green including the identity_tasks migration testsnpm run typecheck— cleannpm run build:web— clean/admin/backfillresponse sequence (screenshotted climbing from 0 to 146 of 146)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes