felladrin / felladrin/LinkedTimer

Stopping the timer broadcasts a periodic sync that still says isRunning: true

Open Beginner friendly
#1,202 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
6
Forks
2
Avg merge
3h 51m
Merged PRs (30d)
33

Description

Stopping the timer emits one periodic sync that still reports `isRunning: true`.

In `configureTimerEventHandlers` (`src/webview/scripts/constants/timer.ts`), the handler that updates the counters is registered before the one that updates `isTimerRunning`, and both listen to `stopped`:

```ts
(["started", "stopped", "secondsUpdated"] satisfies TimerEventType[]).forEach(...) // calls setTotalTimerSeconds
(["started", "stopped"] satisfies TimerEventType[]).forEach(...) // calls setTimerRunning
```

easytimer dispatches listeners in registration order, and `create-pubsub` publishes on every set with no equality check, so stopping the timer runs this chain:

1. `timer.stop()` resets the counters and dispatches `stopped`.
2. The first handler calls `setTotalTimerSeconds(0)`.
3. `onTotalTimerSecondsUpdated.ts` emits `PeriodicSync` with `isRunning: isTimerRunning()`, which is still `true`.
4. Only then does the second handler set it to `false`.

So every stop broadcasts one frame of `{ isRunning: true, totalSeconds: 0, timeValues: 00:00:00 }`.

What it costs, checked against `easytimer.js` rather than assumed:

- A peer that is **stopped** calls `startTimerWithValues({ hours: 0, minutes: 0, seconds: 0 })` if its own total differs by more than a second. That does not start anything: a countdown timer started at zero has already reached its target, so `isRunning` stays `false` and the UI stays on the editor. Verified with `new Timer({ countdown: true }).start({ startValues: { hours: 0, minutes: 0, seconds: 0 } })`, which leaves `isRunning()` at `false`.
- A peer that is **running** gets stopped by it, because `startTimerWithValues` stops the timer before the start that then no-ops.

The second case needs a room that is already desynced, which is why this is not urgent. #1198 makes it rarer rather than worse: it removes the state where a running joiner sits in a stopped room.

The fix is to register the `setTimerRunning` handler first. That also changes the Stop-button path, where the drift tolerance in `handlePeriodicSyncEvent` currently absorbs the stale frame, so it wants its own testing. See the comment above the condition in `handleInitialSyncEvent` (`src/webview/scripts/subscriptions/onRoomUpdated.ts`) for why that tolerance is there.

Found while reviewing #1198.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in src/webview/scripts/constants/timer.ts at configureTimerEventHandlers and inspect the ordering of the stopped handlers. Review src/webview/scripts/subscriptions/onRoomUpdated.ts, especially the tolerance comment and the periodic-sync handling, then test the Stop-button path and confirm stopping no longer broadcasts a frame with isRunning: true and totalSeconds: 0.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.