refactor(sync): make /admin/sync stop claiming more than it checked - #120
Merged
Conversation
Eight review findings deferred out of #118. The page made three assertions it could not support. The queued marker was a bare boolean, so a row enqueued 2s ago and one wedged 3 days read identically -- reachable, since startDispatcher swallows dispatch failures into console.error and retries forever while the cron scheduler keeps the worker looking fresh. The "picks it up within a few seconds" promise was gated on a 90-minute freshness window. And "no runs recorded" rendered as "the worker is not running", asserting from absence of evidence about a worker that may have booted seconds ago. All three now state the age instead of a verdict. queuedNotice's worker argument is required rather than defaulted, so a forgetful caller is a compile error instead of silently borrowing the reassuring string. Collapsing consecutive same-outcome runs also dropped the entire time axis: five hourly runs, one of which took 47 minutes because ESI was degraded, collapsed to a row that said nothing about the 47 minutes. sameOutcome deliberately still ignores duration -- comparing it would stop anything from ever collapsing -- so the span is carried forward and rendered in Took, which is also where "N runs" had been sitting under a header promising a duration. - outbox: group by payload + min(createdAt); the old unbounded SELECT grew without bound exactly while the worker was down - run-summary: group `to` is Date, not Date | null, as its own comment already claimed; errorSummary required, normalizeErrorSummary removed - page: group status drops an unreachable `?? "running"` that would have claimed a finished group was running - dispatcher: RERUNNABLE's comment now says isJobType is the real gate
…ield Two independent nullables that are always both-null or both-set spell four representable states for the two that exist, and force every consumer to test both to rule out the two that cannot happen -- the same defect class this branch's type-design findings exist to remove. Also drops the non-null assertion in toGroup's duration map by filtering on both ends rather than asserting past finishedAt.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe sync status pipeline now records the oldest queued timestamp per job. The admin sync page uses this data for age-aware queued markers. Grouped runs now expose duration ranges, counts, and precise time endpoints. ChangesSync status and queued age
Grouped run display
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdminSyncPage
participant getSyncStatus
participant undispatchedSummary
participant OutboxDatabase
AdminSyncPage->>getSyncStatus: request sync status
getSyncStatus->>undispatchedSummary: read queued payload summaries
undispatchedSummary->>OutboxDatabase: group undispatched rows by payload
OutboxDatabase-->>undispatchedSummary: oldest queued timestamps
undispatchedSummary-->>getSyncStatus: queued and queuedSince values
getSyncStatus-->>AdminSyncPage: render age-aware queue markers
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/app/admin/sync/page.tsx`:
- Line 122: Update the lede construction around queuedNotice in the admin sync
page to remove claims based on worker.fresh about pickup timing or whether the
worker is running. Replace those branches with neutral queue guidance or the
known workerAge, while preserving the existing queued and notice behavior.
In `@src/core/run-summary.ts`:
- Around line 256-277: Update the duration calculation in the grouped-run
summary around the durations map to exclude intervals unless both dates produce
finite timestamps and finishedAt is not earlier than startedAt, matching the
validation used by formatDuration. Preserve valid non-negative durations and
return null when none remain, then add coverage for a grouped run whose
finishedAt precedes startedAt.
In `@tests/sync-view.test.ts`:
- Around line 302-315: Extend the “stays a bare marker under the notable
threshold” test around queuedMarkerText to assert that at(-2 * MIN) returns the
age-bearing ", queued 2m ago" text. Keep the existing below-threshold assertions
unchanged so the exact QUEUED_AGE_NOTABLE_MS boundary is covered.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 21b5e51e-bb78-4997-b9d1-8a06689b403f
📒 Files selected for processing (12)
e2e/sync.spec.tssrc/app/admin/sync/page.tsxsrc/app/admin/sync/view.tssrc/app/globals.csssrc/core/run-summary.tssrc/services/outbox.tssrc/services/sync-status.tssrc/worker/dispatcher.tstests/outbox.test.tstests/run-summary.test.tstests/sync-status.test.tstests/sync-view.test.ts
undispatchedSummary dropped a row whose min(createdAt) came back null, and getSyncStatus derived `queued` from the presence of that row -- so an impossible null would not merely have cost the "3d ago" suffix, it would have made a job with work waiting in the outbox render as idle. That is the exact under-reporting the queued marker exists to prevent. The row is now carried through with a nullable age, presence and age are tracked separately in the fold, and both marker helpers degrade on their own: no age means a bare ", queued" and never an escalation, since escalation is a claim about elapsed time.
toGroup subtracted the pair unconditionally, unlike formatDuration, which has always guarded Number.isFinite and ms >= 0 on the same computation. A clock adjustment between the two writes inverts the pair, and an Invalid Date subtracts to NaN -- either would let Math.min/max report a span the group cannot have taken, and one bad member poisons a row standing in for several good ones. Also covers the exact QUEUED_AGE_NOTABLE_MS boundary, which sat between the existing under- and over-threshold assertions.
This was referenced Aug 6, 2026
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.
Eight code-review findings deliberately deferred out of #118. Items 1–3 and 8
were mechanical; items 4–7 changed what the page is permitted to claim and were
decided before implementing. All eight re-verified as still reproducing at
d679af9— none were skipped.What the page is now allowed to say
The queued marker carries an age (4).
undispatchedPayloadsbecameundispatchedSummary: grouped by payload withmin(createdAt), so the statusread no longer scales with how long the worker has been down — the one condition
under which an admin loads this page.
getSyncStatusfolds those timestampsthrough
jobsForto a per-job-typequeuedSince, and the dot now reads", queued 3d ago" and turns amber past 15 minutes.
startDispatcherswallowsdispatch failures into
console.errorand retries forever while pg-boss cronkeeps
worker.freshtrue, so a green worker line beside a silent queued dot wasthe exact shape of a stuck outbox. It is no longer silent.
The "few seconds" promise is gone (5, 6). It was gated on a 90-minute
staleness window, which cannot support a claim about seconds. And "no runs
recorded" was being rendered as "the worker is not running" — an assertion from
absence of evidence.
queuedNoticenow states the worker's actual age, or saysplainly that there is nothing to date it by. Its
workerAgeparameter isrequired, so a forgetful caller is a compile error rather than a default to the
reassuring string.
Collapsing keeps the time axis (7). The finding's own first remedy — add
duration to
sameOutcome— would stop anything from ever collapsing, sincedurations are near-never equal. Instead the group carries a
durationMsmin/max span and renders it in the "Took" column, so a 47-minute run during an
ESI slowdown no longer reads identically to a 2-minute one. That also resolves
(2): the run count was squatting in that column under a header promising a
duration, announced to screen readers as "Took: 4 runs". The count moved beside
the time range in cell 1, and both ends of the range now reach the
accessibility tree at every width.
Mechanical
SELECTover undispatched rows → grouped, folded into (4).{entry.status ?? "running"}in the group branch — unreachable; removed.The single-run branch keeps its fallback, where null really is in flight.
CollapsedRun.to: Date | null→Date, matching its own comment.errorSummary?: string | null→ required;normalizeErrorSummarygone.RERUNNABLE's comment, stale sinceisJobTypebecame the gate.SyncStatusGroup.Verification
No schema change, no migration, no change to token handling or the OAuth state
flow. The one new query is read-only and takes no lock, deliberately: a
FOR UPDATEread here would contend with the dispatcher's own claim.Summary by CodeRabbit
New Features
Bug Fixes