fix: improve bulk rules progress tracking#990
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces numeric thread counting with a Set of processed thread IDs, adds a runResult state for run outcomes, changes onRun to report queued thread IDs and completion status/count, updates pagination to call onThreadsQueued and onComplete, and adjusts UI to show progress and a zero-result message. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BulkRunRules
participant Parent
User->>BulkRunRules: Click "Start Run"
BulkRunRules->>BulkRunRules: Clear runResult & processedThreadIds
BulkRunRules->>BulkRunRules: Fetch first page of threads
loop For each page
BulkRunRules->>Parent: onThreadsQueued(threadIds)
Parent->>Parent: Queue threads for processing
BulkRunRules->>BulkRunRules: Add threadIds to processedThreadIds / increment totalProcessed
BulkRunRules->>BulkRunRules: Fetch next page (or stop)
end
alt Completed normally
BulkRunRules->>Parent: onComplete("success", totalProcessed)
else Aborted
BulkRunRules->>Parent: onComplete("cancelled", totalProcessed)
end
Parent->>BulkRunRules: (ack) update UI
BulkRunRules->>User: Show progress or "No unread emails found..." if count === 0
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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: 1
🧹 Nitpick comments (3)
apps/web/app/(app)/[emailAccountId]/assistant/BulkRunRules.tsx (3)
31-33: Set-based tracking is solid; remaining computation could be lighter-weightUsing
Set<string>forprocessedThreadIdsand derivingremainingvia intersection with the AI queue is a good way to avoid double-counting and to gate progress on actual queue state. The only nit is thatremainingallocates a newSetevery render:const remaining = new Set( [...processedThreadIds].filter((id) => queue.has(id)), ).size;For large runs you could instead just count matches in a loop (or via
reduce) and/or wrap the computation in auseMemokeyed onprocessedThreadIdsandqueueto avoid extra allocations. Not urgent, but would slightly simplify and optimize this hot-path calculation.Also applies to: 45-47, 51-55
116-149: Guard againstrunninggetting stuck true ifonRunthrows
setRunning(false)is only invoked in theonCompletecallback:(count) => { setRunning(false); if (count === 0) { setRunResult({ count }); } }and
onComplete(totalProcessed)is called at the end ofrun(). IffetchWithAccount,runAiRules, or JSON parsing ever throw synchronously (rather than returning a non‑OK response),run()will reject andonCompletewill never be called, leavingrunningstucktrueand the cancel button visible.Consider wrapping the body of
run()intry/finallyto guarantee thatonComplete(or at least a “done/error” callback that clearsrunning) is invoked exactly once even on unexpected exceptions:async function run() { try { // existing loop, fetch, queue, sleep, etc. } finally { onComplete(totalProcessed); } }You can extend this later to pass an optional error or status flag if you want the caller to distinguish success vs failure.
Also applies to: 184-201, 262-265
76-82: Progress messaging semantics are good; slight UX edge casesUsing
processedThreadIds.sizeas the total andprocessedThreadIds.size - remainingas “completed” gives a meaningful progress indicator tied to actual queue membership:
- While some of this run’s threads are still in the AI queue, you show
Progress: completed/total emails completed.- Once none of this run’s IDs are in the queue, you flip to
Success: Processed total emails.This is a nice improvement over pure counting. One minor UX quirk to be aware of: if the queue drains between pages and then new pages get queued, the banner may briefly show “Success” before reverting to a “Progress” state for the newly queued IDs. That’s acceptable, but if it ever feels jumpy you could gate the “Success” message on receiving the final
onCompleteas well.Also applies to: 247-249, 259-260
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/(app)/[emailAccountId]/assistant/BulkRunRules.tsx(10 hunks)version.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/app/(app)/[emailAccountId]/assistant/BulkRunRules.tsx (1)
apps/web/components/Typography.tsx (1)
SectionDescription(127-127)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: test
🔇 Additional comments (2)
version.txt (1)
1-1: Version bump matches scope of changesVersion increment to
v2.20.10is appropriate for this behavioral/UI tweak to bulk rules.apps/web/app/(app)/[emailAccountId]/assistant/BulkRunRules.tsx (1)
90-94: Date changes correctly reset run stateClearing
runResultandprocessedThreadIdswhen start/end dates change avoids showing stale progress or “no unread” messages for a different date range. This wiring looks correct and keeps the UI state consistent with the selected filters.Also applies to: 100-104
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="apps/web/app/(app)/[emailAccountId]/assistant/BulkRunRules.tsx">
<violation number="1" location="apps/web/app/(app)/[emailAccountId]/assistant/BulkRunRules.tsx:146">
`runResult` is set whenever the run completes with `count === 0`, but `onRun` calls `onComplete(totalProcessed)` even after errors or user cancellation. This causes the UI to claim “No unread emails found…” when the run was actually aborted or failed, misleading the user.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| }, | ||
| (count) => { | ||
| setRunning(false); | ||
| if (count === 0) { |
There was a problem hiding this comment.
runResult is set whenever the run completes with count === 0, but onRun calls onComplete(totalProcessed) even after errors or user cancellation. This causes the UI to claim “No unread emails found…” when the run was actually aborted or failed, misleading the user.
Prompt for AI agents
Address the following comment on apps/web/app/(app)/[emailAccountId]/assistant/BulkRunRules.tsx at line 146:
<comment>`runResult` is set whenever the run completes with `count === 0`, but `onRun` calls `onComplete(totalProcessed)` even after errors or user cancellation. This causes the UI to claim “No unread emails found…” when the run was actually aborted or failed, misleading the user.</comment>
<file context>
@@ -111,9 +132,21 @@ export function BulkRunRules() {
+ },
+ (count) => {
+ setRunning(false);
+ if (count === 0) {
+ setRunResult({ count });
+ }
</file context>
✅ Addressed in e574ec2
There was a problem hiding this comment.
1 issue found across 1 file (reviewed changes from recent commits).
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="apps/web/app/(app)/[emailAccountId]/assistant/BulkRunRules.tsx">
<violation number="1" location="apps/web/app/(app)/[emailAccountId]/assistant/BulkRunRules.tsx:269">
The `runAiRules` function, which is called by this component, does not handle errors within its queued jobs. This can lead to a permanent inconsistency between the AI processing queue state and the UI, causing progress indicators to freeze on an error, even though this component reports that the queuing was successful.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| } | ||
|
|
||
| onComplete(); | ||
| onComplete("success", totalProcessed); |
There was a problem hiding this comment.
The runAiRules function, which is called by this component, does not handle errors within its queued jobs. This can lead to a permanent inconsistency between the AI processing queue state and the UI, causing progress indicators to freeze on an error, even though this component reports that the queuing was successful.
Prompt for AI agents
Address the following comment on apps/web/app/(app)/[emailAccountId]/assistant/BulkRunRules.tsx at line 269:
<comment>The `runAiRules` function, which is called by this component, does not handle errors within its queued jobs. This can lead to a permanent inconsistency between the AI processing queue state and the UI, causing progress indicators to freeze on an error, even though this component reports that the queuing was successful.</comment>
<file context>
@@ -249,14 +254,19 @@ async function onRun(
}
- onComplete(totalProcessed);
+ onComplete("success", totalProcessed);
}
</file context>
1 participant
Summary by CodeRabbit
Bug Fixes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.