fix(sidepanel): fix TODAY rollup lag — daily summary now updates on every turn#50
Conversation
Turn writes land under conv: keys and did not trigger loadToday(). TODAY lagged the active conversation by up to the background alarm interval (~30 min). When hasConvChange fires, now also call computeDailySummary so the daily: key is fresh. The existing hasDailyChange arm picks it up and calls loadToday() immediately.
Three tests encode the product-level invariants GET-18 depends on: daily summary totalTurns >= any single conv contribution, fresh computeDailySummary after a new turn reflects it immediately, and no double-counting across two conversations in the same day.
Hook comment now names the exact trade-offs (dual writers, full-scan cost). Test file rewritten: WHY comments per test, hook integration gap documented, null-cost test added for the unknown-model case.
|
@DevanshuNEU is attempting to deploy a commit to the Dev's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 25 minutes and 19 seconds. ⌛ 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request enhances the dashboard hook to automatically recompute daily summaries when conversations change, and introduces a comprehensive test suite validating daily rollup aggregation accuracy, cost handling, and freshness guarantees. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
entrypoints/sidepanel/hooks/useDashboardData.ts (1)
314-328:⚠️ Potential issue | 🟠 MajorBatch the daily recompute before writing storage.
This can start multiple overlapping full-org scans and
daily:writes for one burst of turn/index changes. If an earlier scan finishes after a later one, it can overwrite the fresher summary; the empty catch also hides the failure path that would keep TODAY stale. Debounce the recompute for 200ms and log failures.Proposed fix
// Ref mirror of isClaudeTab so event listeners (closures) can read the current // value without capturing a stale boolean from the render cycle. const isClaudeTabRef = useRef(false); + const dailySummaryRecomputeTimerRef = useRef<ReturnType<typeof window.setTimeout> | null>(null); @@ // Trade-off: reads all org conv records on each turn. Cost grows with // history depth but is fire-and-forget and fine at 90-day scale. [GET-18] const orgId = orgIdRef.current; if (orgId) { - void computeDailySummary(orgId, todayDateString()).catch(() => {}); + if (dailySummaryRecomputeTimerRef.current !== null) { + window.clearTimeout(dailySummaryRecomputeTimerRef.current); + } + dailySummaryRecomputeTimerRef.current = window.setTimeout(() => { + dailySummaryRecomputeTimerRef.current = null; + if (orgIdRef.current !== orgId) return; + void computeDailySummary(orgId, todayDateString()).catch((err) => { + console.error('[Saar] Failed to recompute daily summary:', err); + }); + }, 200); } } @@ return () => { + if (dailySummaryRecomputeTimerRef.current !== null) { + window.clearTimeout(dailySummaryRecomputeTimerRef.current); + } chrome.storage.onChanged.removeListener(onStorageChanged); chrome.tabs.onActivated.removeListener(onTabActivated); chrome.tabs.onUpdated.removeListener(onTabUpdated);As per coding guidelines,
entrypoints/**/*.ts: Implement 200ms batching on postMessage and storage writes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@entrypoints/sidepanel/hooks/useDashboardData.ts` around lines 314 - 328, The code triggers computeDailySummary immediately on every conversation change, which can start overlapping org scans and storage writes; debounce these recomputes for 200ms so only the last call in a burst runs: add a persistent timer/ref (e.g., recomputeTimerRef) near orgIdRef/tabIdRef, clear any existing timer before scheduling a new setTimeout that calls computeDailySummary(orgId, todayDateString()), and on the promise catch log the error via processLogger/console (do not swallow it with an empty catch); ensure the timer is cleared when appropriate (cleanup) to prevent leaks and keep the existing loadConversations/loadActiveConversation behavior 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 `@tests/unit/today-rollup.test.ts`:
- Around line 56-71: The test uses Date.now() as completedAt for recordTurn but
calls computeDailySummary(todayDateString()) separately, which can flake across
midnight; capture the date string from the same now (e.g. compute a summaryDate
derived from now) and pass that into computeDailySummary so the summary date
matches the turns' completedAt timestamps; update all similar blocks that call
recordTurn(..., completedAt: now) then computeDailySummary(todayDateString()) to
instead derive and use the date from now.
---
Outside diff comments:
In `@entrypoints/sidepanel/hooks/useDashboardData.ts`:
- Around line 314-328: The code triggers computeDailySummary immediately on
every conversation change, which can start overlapping org scans and storage
writes; debounce these recomputes for 200ms so only the last call in a burst
runs: add a persistent timer/ref (e.g., recomputeTimerRef) near
orgIdRef/tabIdRef, clear any existing timer before scheduling a new setTimeout
that calls computeDailySummary(orgId, todayDateString()), and on the promise
catch log the error via processLogger/console (do not swallow it with an empty
catch); ensure the timer is cleared when appropriate (cleanup) to prevent leaks
and keep the existing loadConversations/loadActiveConversation behavior
unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13d29690-dd91-4b80-9048-85c721ccdd32
📒 Files selected for processing (2)
entrypoints/sidepanel/hooks/useDashboardData.tstests/unit/today-rollup.test.ts
…[GET-18] Silent catch made TODAY stay stale with zero indication when the recompute failed. Consistent with every other error path in this hook.
1 participant
Summary
The TODAY section in the side panel was lagging the active conversation by up to 30 minutes. Turn writes land under
conv:keys; the storage listener only calledloadToday()ondaily:key changes, which only happened when the background alarm fired. This PR fixes that by recomputing the daily summary immediately on each turn, so TODAY and ACTIVE CONVERSATION always agree.Type of Change
feat— New featurefix— Bug fixrefactor— Code restructure, no behavior changetest— Tests onlychore— Build, CI, tooling, dependenciesdocs— Documentation onlyWhat Was Changed
entrypoints/sidepanel/hooks/useDashboardData.ts— InonStorageChanged, whenhasConvChangefires, also callcomputeDailySummary(orgId, todayDateString())fire-and-forget. This writesdaily:, which the existinghasDailyChangearm catches and feeds toloadToday(). The chain completes in one storage round-trip instead of waiting for the alarm.tests/unit/today-rollup.test.ts(new) — Four tests encoding the GET-18 product invariants: multi-conv day sums correctly (notMath.max), a new turn is reflected immediately, no double-counting, and null cost propagates as null rather than 0.How to Test
.output/chrome-mv3/in ChromeChecklist
bun run test)bun run compile)bun run build)feat:,fix:,refactor:,test:,chore:)Related Issues
Closes GET-18
Notes for Reviewer
Path A (
Math.max(today.totalTurns, activeConv.turnCount)) was considered and rejected: it breaks for multi-conversation days where the active conv has fewer turns than the daily total. Path B (recompute on turn) is correct in all cases becausecomputeDailySummaryalways scans all conversations.Known trade-off:
computeDailySummaryreads all org conv records on each turn, not just today's (the conv index stores IDs only, no date metadata). At 90-day retention scale this is a batch read of ~100-300 records, fire-and-forget, and imperceptible in practice. If power users hit this limit, the fix is a date-indexed secondary index inconversation-store.ts.Hook integration test gap: the storage event wiring in
useDashboardData.tscannot be unit-tested withoutrenderHook+ Chrome API mocks (not in this repo's test setup). The four tests intoday-rollup.test.tsprove the data contract at the storage layer. Manual verification covers the hook wiring.Summary by CodeRabbit
Bug Fixes
Tests