feat(progress-monitor): replace setInterval with progressive setTimeout schedule#486
Conversation
|
🔍 Reviewing PR... |
nhopeatall
left a comment
There was a problem hiding this comment.
LGTM — Clean, well-tested replacement of setInterval with a progressive setTimeout chain.
The key design decisions are sound:
-
setTimeout chain eliminates tick overlap by construction — the next tick isn't scheduled until the current one completes, making the
isGeneratingguard purely defense-in-depth. -
startedflag is the right idempotency check — the oldif (this.timer)check would have been unreliable with setTimeout chains sincethis.timeris transientlynullbetween tick completion and next schedule. -
stoppedflag handles the in-flight race —clearTimeoutalone wouldn't prevent a tick that's already executing from scheduling the next one. The flag covers both paths. -
Default schedule applied internally — callers don't need changes, and
scheduleMinutesis exposed on the config interface for testability and future customization. -
Test coverage is thorough — 4 new tests verify the progressive schedule, fallback to
intervalMinutes, default schedule behavior, and stop() preventing further ticks. Existing tests correctly updated for the new 1-minute first tick.
One minor observation (not blocking): ProgressMonitorOptions in progress.ts doesn't expose scheduleMinutes, so createProgressMonitor() always uses the default. This is fine for now since the PR's goal is changing the internal schedule, but worth noting if callers ever need to customize it.
Summary
Replaces the fixed-interval
setIntervalinProgressMonitorwith asetTimeout-based progressive schedule. The first progress update now fires at 1 minute, the second at 3 minutes, the third at 5 minutes, then every 5 minutes (steadyintervalMinutes) thereafter.scheduleMinutes?: number[]optional field toProgressMonitorConfiginterfaceDEFAULT_SCHEDULE_MINUTES = [1, 3, 5]module-level constant as the default progressive schedulesetIntervalwith asetTimeoutchain using new private methods:scheduleNextTick()andtickAndScheduleNext()tickIndex,stopped, andstartedprivate fields for correct progressive scheduling and idempotencystop()to useclearTimeoutand set astoppedflag so in-flight ticks don't schedule further ticksintervalMinutes, default schedule behavior, and stop() preventing further ticksCard: https://trello.com/c/699b3be2b5e20d99a047c533
Test plan
biome check— no issues)tsc --noEmit— no errors)