feat(sync): make /admin/sync answer what the press did, and what the fan-out covers - #118
Merged
Conversation
Groups job types into sweep/on-demand/housekeeping per which page control can reach them. Record<JobType, JobGroup> makes a new JOB_CRON key without an assignment a compile error, matching the JobType exhaustiveness argument.
- Lift the outbox payload -> job types mapping into src/core/dispatch-plan.ts (jobsFor), the single source of truth planDispatch and getSyncStatus both read, so the admin page's queued marker can never drift from what the worker will actually dispatch. planDispatch's behaviour (drop arms, RERUNNABLE gate, singleton keys, data payloads) is unchanged. - getSyncStatus now returns `queued: boolean` per job type, true iff an undispatched outbox row's payload targets that job type (member-triggered account/discord-user rows count the same as admin-triggered ones). Costs one extra unlocked read query (services/outbox.ts: undispatchedPayloads), not one per job type. - Add collapseRuns (src/core/run-summary.ts) to fold consecutive runs sharing an identical outcome (status + counts, both finished) into one group entry carrying the run count and time range, for the runs drawer.
contacts.ts, wanderer.ts, and discord-roles.ts build errorSummary from per-target error lines that counts never reflects, so two partial runs can share status and counts while a different target failed for a different reason each time. Collapsing them hid the second run's diagnostics behind the first's, in exactly the view admins open to read them. errorSummary now joins status and counts in sameOutcome; null and undefined are normalized to the same "nothing to show" fact, matching that the field is optional.
…ypeset The page's one write action left the strip byte-identical: an admin pressed the fan-out, got a notice at the top, and saw no change in the seven rows the press actually affected. Rows now carry a queued marker fed by undispatched outbox rows, beside the health token rather than inside RowHealth — folding it into health would set four rows actionable at once and blow open four drawers on a single press, the failure overdue was excluded from auto-open to prevent. Scope moves out of the button label and into the strip. "Sync membership, contacts, wanderer, discord-roles" spent 48 characters of uppercase mono storing what the strip can show: three named groups (sweep / on-demand / housekeeping) answer which rows a press covers, so the button is just a verb. The groups are three aria-labelledby'd lists, not painted-on labels — the nouns left the button, so they have to be perceivable non-visually or screen reader users end up worse off than before. Drawer rows collapse when consecutive finished runs agree, so a healthy job stops spending five near-identical rows saying nothing changed. Typeset defects the design system already documented: .worker leaves the label register (prose word plus a value, the same test that excluded .push__next), .st declares the 600 its siblings declare, and a RuleHead heads the strip the way every other admin data region is headed.
Three findings from the polish pass, plus four comment corrections.
PlannedJob typed jobType as string. Before the lift into core,
planDispatch used the QUEUES constants, so renaming a queue was a
compile error at every send site; the bare literals that replaced them
were checked against nothing. jobType is JobType again.
PlannedJob was also structural rather than discriminated, and its
{ jobType } arm subsumed the other two: a job carrying both accountId
and discordUserId typechecked, and sendFor would have taken the
narrower branch and dropped the Discord scoping. A fourth arm would
have compiled with sendFor untouched, falling through to a global send
with a ${queue}:all singleton key and no error anywhere. It discriminates
on scope now, and sendFor switches with a never arm. No observable send
changes: queue names, data shapes and both roles: singleton-key
exceptions are identical, and tests/dispatcher.test.ts needed no edits.
OutboxPayload was declared verbatim in both core and services, each with
a comment claiming it could never drift. Core owns it; services
re-exports.
Comments: the removed button label was 50 characters, not 48 (three
sites). The runs table's width rationale had Status backwards - it is
the only column with a width constraint, not the only one without. The
group row's font-style has no cascade-order claim on its position, so
it moves below the media query. .strip__health declares no gap, so the
margin cross-reference pointed at nothing.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 8 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 (17)
Comment |
guarzo
enabled auto-merge (squash)
August 5, 2026 17:23
guarzo
added a commit
that referenced
this pull request
Aug 5, 2026
…120) * refactor(sync): make /admin/sync stop claiming more than it checked 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 * refactor(sync): fold the group's min/max duration into one nullable field 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. * fix(sync): keep the queued marker when its age does not survive the read 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. * fix(sync): reject inverted and non-finite spans from a group's duration 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.
guarzo
added a commit
that referenced
this pull request
Aug 6, 2026
One conflict, in DESIGN.md's label-register paragraph: both sides rewrote the `.st` weight note and told opposite stories about it. Main's #130 is the correct one and is what survives. This branch had "corrected" the paragraph to say `globals.css` has declared 600 on `.st` for as long as the rule existed, so the document had always been wrong. Checking the history rather than the current file: `.st` was added in #11 with no `font-weight` and inherited 400; #118 added the explicit 600 and rewrote `.st--lead`'s comment to say the weight there was "redundant with `.st`'s own 600 NOW". So the original paragraph described a real inconsistency that was real when written and has since been fixed -- which is exactly what main says. The correction was itself the error, made by reading the stylesheet as it stands today and mistaking that for how it has always stood.
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.
From an
/impeccable critiqueof/admin/sync, scoped to P1-P3 plus the.stweight defect.The problem
Pressing Sync now enqueued an outbox row and re-rendered the strip byte-identical. The page's one write action left no trace until a worker run landed seconds later. Separately, the fan-out's scope lived only in the primary button's label - 50 characters of uppercase mono - so the strip never said which of the seven jobs the press covered.
What this does
getSyncStatusreads undispatched outbox rows and returnsqueuedper job type; affected rows get a marker. Deliberately not aRowHealthmember: folding it in would make one press set four rows actionable and auto-open four drawers at once, the exact failure modeoverduewas excluded from auto-open to prevent (view.ts:96-104).<ul role="list" aria-labelledby>- since the four job nouns left the button on the grounds the strip now carries scope, visual-only grouping would have left screen reader users worse off than before..workerout of the label register, aRuleHeadabove the strip,font-weight: 600on.st, and width caps so values stop pinning to the container edge.The interesting part
jobsForin the newsrc/core/dispatch-plan.tsis the single mapping from an outbox payload to the jobs it targets. Both the worker (which sends) andgetSyncStatus(which only asks "is anything queued for this type") read it, so the marker cannot claim a job is queued that the worker would not dispatch.sameOutcomecompareserrorSummary, not just status and counts.contacts.ts:245,wanderer.ts:162-166anddiscord-roles.ts:190build it from per-target error lines thatcountsnever reflects, so twopartialruns can both showfailed: 1while different characters failed for different reasons. Collapsing those would erase it.Self-inflicted regression, caught and fixed
Lifting the payload mapping into
src/core/replaced theQUEUES.*constants with bare literals typedjobType: string. Renaming a queue used to be a compile error at every send site; afterwards{ jobType: "wanderer" }compiled whether or not the queue existed.PlannedJobwas also structural rather than discriminated, and its{ jobType }arm subsumed the other two - so{ jobType, accountId, discordUserId }typechecked andsendForwould have taken the narrower branch, silently dropping the Discord scoping.Both fixed in the final commit. All 1024 tests passed the whole time this was broken; only a type-design review found it. Send behaviour is unchanged - queue names, data shapes and both
roles:singleton-key exceptions are identical, andtests/dispatcher.test.tsneeded no edits.Verification
npx vitest runnpm run test:e2enpm run typechecknpm run lint<img>warningnpm run format:checkNot exercised: real Postgres under load, or a genuinely dead worker. The two deferred items below are reasoned from the code, not observed.
Reviewer focus
sendFor's rewrite (dispatcher.ts:48-79) - the one place a mistake changes what actually gets sent.sameOutcome's merge conditions - anything it ignores becomes invisible in the drawer.queuedshould have stayed out ofRowHealth.Deferred, not fixed here
Eight review findings were left alone. Three worth knowing about:
undispatchedPayloadshas noLIMIT, and undispatched rows are never purged - so the row count grows without bound exactly while the worker is down, which is when an admin is most likely to load this page.sameOutcomenever compares duration, so a 47-minute degraded run hides among four fast ones, andentry.tois not in the accessibility tree at all.