Skip to content

fix(gps): warn in dev when polling refcount goes out of bounds#128

Merged
jasoneplumb merged 2 commits intomainlinefrom
fix/gps-polling-refcount-leaks
Apr 19, 2026
Merged

fix(gps): warn in dev when polling refcount goes out of bounds#128
jasoneplumb merged 2 commits intomainlinefrom
fix/gps-polling-refcount-leaks

Conversation

@jasoneplumb
Copy link
Copy Markdown
Owner

@jasoneplumb jasoneplumb commented Apr 19, 2026

Summary

  • Adds dev-only console.warn guards in both activatePolling() and deactivatePolling() using import.meta.env.DEV (stripped by Vite in production builds)
  • activatePolling(): warns when updateCallback > 2 (above expected max of 2)
  • deactivatePolling(): warns when updateCallback < 0 (underflow from unmatched deactivate)
  • Both warnings include the current value for fast diagnosis

Changes

  • src/main.ts: Added 3-line warn block in activatePolling() (overflow guard) and 3-line warn block in deactivatePolling() (underflow guard)

Test plan

  • npm run type-check — passes
  • npm run lint — passes
  • In dev build: trigger leak scenario to verify warn appears in console
  • In production build: verify warns are absent (Vite strips import.meta.env.DEV blocks)

Assumptions

  • import.meta.env.DEV is the correct Vite idiom for dev-only guards; no polyfill or type augmentation needed
  • No clamping: invalid values are left in place intentionally so callers can observe the leaked state

Closes #125

@jasoneplumb jasoneplumb added bug Something isn't working review-requested Triggers Claude code review labels Apr 19, 2026
Adds a dev-only console.warn guarded by import.meta.env.DEV (stripped by
Vite in production builds) that fires when updateCallback climbs above the
expected maximum of 2, surfacing unmatched activatePolling() calls early.

Closes #125
@jasoneplumb jasoneplumb force-pushed the fix/gps-polling-refcount-leaks branch from 6339090 to 4e204ab Compare April 19, 2026 02:47
@claude
Copy link
Copy Markdown

claude Bot commented Apr 19, 2026

Code Review

Overview

This PR adds a 3-line dev-only guard that warns when state.updateCallback exceeds 2, indicating a leaked/unmatched activatePolling() call. Clean, focused change that closes #125.


Code Quality & Style ✅

  • Correct Vite idiom (import.meta.env.DEV) — will be dead-code-eliminated in production builds
  • Warning message is descriptive and includes the current value, which is immediately actionable
  • Placement is correct: fires after the increment, before the startWatching side-effect
  • No unnecessary comments added (consistent with project style)

Suggestions

Symmetric guard in deactivatePolling() (recommended)

The refcount can leak in two directions — too high (covered here) and too low (underflow). An unmatched deactivatePolling() call would decrement below 0, potentially stopping the watcher prematurely while a consumer still thinks it's active. A mirror check would complete the picture:

function deactivatePolling(): void {
  state.prior = 1000;
  state.updateCallback -= 1;
  if (import.meta.env.DEV && state.updateCallback < 0) {
    console.warn('[webmap] updateCallback < 0 — possible deactivatePolling() leak', state.updateCallback);
  }
  if (state.updateCallback === 0) stopWatching(map);
}

Optional: console.trace for call-site context

Since these are developer-facing warnings, swapping console.warn for console.trace (or appending a console.trace() call) would print the call stack inline — making it much faster to pinpoint which caller is over-activating. Low noise since it only fires on a bug path.


Correctness & Risk ✅

  • No production behaviour change — the import.meta.env.DEV block is fully stripped by Vite
  • No type errors expected; state.updateCallback is a number throughout
  • The > 2 threshold is correct given the documented 0–2 range

Test Coverage

The test plan in the PR description covers the key cases (type-check, lint, manual dev/prod verification). Given the project has no automated test framework, this is consistent with existing practice.


Overall: Approve as-is; the underflow guard is a nice-to-have improvement but not blocking.

Mirrors the activatePolling() overflow guard: warns in dev when
updateCallback drops below 0, catching unmatched deactivatePolling()
calls that would prematurely stop GPS watching.

Addresses review feedback on #128.
@jasoneplumb jasoneplumb added review-requested Triggers Claude code review and removed review-requested Triggers Claude code review labels Apr 19, 2026
@claude
Copy link
Copy Markdown

claude Bot commented Apr 19, 2026

Code Review

Overview

This PR adds dev-only refcount boundary guards to both activatePolling() and deactivatePolling() using import.meta.env.DEV, which Vite strips from production builds. It's a small, focused diagnostic aid that closes #125.


What's Good

  • Correct Vite idiom: import.meta.env.DEV is the right guard — no polyfill needed, and Vite's dead-code elimination will remove these blocks entirely from production builds.
  • Placement is correct: In activatePolling() the warn fires before the === 1 check (which won't trigger at > 2 anyway), and in deactivatePolling() before the === 0 check (which won't trigger at < 0). No accidental double-starts or double-stops.
  • Value included in the message: Logging state.updateCallback alongside the warning makes diagnosing the offending call much faster.
  • Consistent message format: Both messages follow the same [webmap] ... — possible ... leak pattern.

Issues / Suggestions

1. PR description doesn't match the diff (minor)

The summary says:

Added 3-line warn block after the state.updateCallback += 1 increment in activatePolling()

But the diff actually adds 6 lines across two functions — activatePolling() and deactivatePolling(). The title also only names activatePolling(). Worth updating both title and summary to reflect the complete change (e.g. "warn in dev when refcount goes out of bounds in activate/deactivatePolling").

2. State is left in an invalid value after the warning (worth a comment)

After the guard fires, the refcount is already at the invalid value (> 2 or < 0) and stays there. For example, if updateCallback hits -1, stopWatching() never fires (it checks === 0), so the polling loop keeps running. That's probably the right tradeoff for a diagnostic — a clamp would hide the bug — but a short inline comment would make the intent explicit:

// No clamp: leave the invalid value in place so callers can observe it.
console.warn('[webmap] updateCallback < 0 — possible deactivatePolling() leak', state.updateCallback);

3. Could log a stack trace for faster call-site identification (optional)

console.warn without a thrown error won't include a stack trace in all browsers. If you want to make it trivial to find the offending caller in DevTools, consider:

console.warn('[webmap] updateCallback > 2 — possible activatePolling() leak', state.updateCallback, new Error().stack);

This is a quality-of-life suggestion, not a blocker.


Summary

Correctness ✅ No bugs introduced
Conventions ✅ Matches project style
Production safety ✅ Stripped by Vite
Description accuracy ⚠️ Summary/title only mention activatePolling()
Test plan ✅ Appropriate given no test framework

The logic is sound and the implementation is clean. The description mismatch is the only thing worth fixing before merge. The other two points are optional improvements.


Reviewed by Claude Sonnet 4.6

@jasoneplumb jasoneplumb changed the title fix(gps): warn in dev when activatePolling() refcount exceeds 2 fix(gps): warn in dev when polling refcount goes out of bounds Apr 19, 2026
@jasoneplumb jasoneplumb merged commit ba2e565 into mainline Apr 19, 2026
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working review-requested Triggers Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: GPS polling refcount leak guard — warn when updateCallback > 2

1 participant