charles/agent-hooks

Fork 0

fix: encapsulate render-queue state in a singleton class (#1140) #1141

Merged

charles merged 1 commit from code-lead/1140 into main

2026-05-13 22:01:49 +00:00

code-lead commented

2026-05-13 21:44:04 +00:00

Collaborator

Closes #1140.

Summary

Folds the module-level requested Set and the draining / drainScheduled flags in apps/server/src/infrastructure/agent-env-sync/render-queue.ts into a single RenderQueue class held in a module-level singleton.
All mutations now go through methods; the file has zero bare module-level let declarations. The drain-loop invariants (one in-flight microtask, one drain cycle at a time, burst-dedup via the pending Set) are documented in one place on the class.
Test seams (setRenderFn, resetRenderFn, setListAgentsFn, resetListAgentsFn) and the public API (enqueueRender, scheduleRenderOnConfigChange, flushRenderQueue) are unchanged — they are thin delegations to the singleton, so callers and existing tests need no updates.

Out of scope

Public API shape — unchanged.
Parallelising renders — single-queue behaviour preserved.

Test plan

just qa — 3359 / 3359 passing, all lints / schema checks green.
render-for-instance.test.ts integration tests for scheduleRenderOnConfigChange + burst dedup pass via the new singleton.

Closes #1140. ## Summary - Folds the module-level `requested` Set and the `draining` / `drainScheduled` flags in `apps/server/src/infrastructure/agent-env-sync/render-queue.ts` into a single `RenderQueue` class held in a module-level singleton. - All mutations now go through methods; the file has zero bare module-level `let` declarations. The drain-loop invariants (one in-flight microtask, one drain cycle at a time, burst-dedup via the pending Set) are documented in one place on the class. - Test seams (`setRenderFn`, `resetRenderFn`, `setListAgentsFn`, `resetListAgentsFn`) and the public API (`enqueueRender`, `scheduleRenderOnConfigChange`, `flushRenderQueue`) are unchanged — they are thin delegations to the singleton, so callers and existing tests need no updates. ## Out of scope - Public API shape — unchanged. - Parallelising renders — single-queue behaviour preserved. ## Test plan - [x] `just qa` — 3359 / 3359 passing, all lints / schema checks green. - [x] `render-for-instance.test.ts` integration tests for `scheduleRenderOnConfigChange` + burst dedup pass via the new singleton.

code-lead added 1 commit

2026-05-13 21:44:04 +00:00

fix: encapsulate render-queue state in a singleton class (#1140 )

qa / sql-layer-check (pull_request) Successful in 7s

Details

qa / dockerfile (pull_request) Successful in 12s

Details

qa / i18n-string-check (pull_request) Successful in 11s

Details

qa / db-schema (pull_request) Successful in 14s

Details

qa / qa-1 (pull_request) Successful in 2m16s

Details

qa / qa (pull_request) Successful in 0s

Details

691f83a8e1

Folds the module-level `requested` Set and the `draining`/`drainScheduled`
flags into a single `RenderQueue` class held in a module-level singleton.
All mutations go through methods, so the file no longer has any bare
`let` declarations and the queue's invariants live in one place.

Public exports (`enqueueRender`, `scheduleRenderOnConfigChange`,
`flushRenderQueue`, `set*Fn` / `reset*Fn`) are unchanged — they delegate
to the singleton — so callers and tests are unaffected.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

charles requested review from reviewer

2026-05-13 21:47:04 +00:00

reviewer approved these changes

2026-05-13 21:49:45 +00:00

reviewer left a comment

Code Review — `fix: encapsulate render-queue state in a singleton class`

CI: ✅ green. Single file changed (render-queue.ts, +152 / −91). Pure refactor — public API surface unchanged.

Overview

All mutable module-level state (let draining, let drainScheduled, let _renderFn, let _listAgentsFn, const requested) is folded into a RenderQueue class behind a single const queue singleton. The exported functions become one-line delegations. The drain logic, invariants, and test-seam contract are all preserved exactly.

What's good

private readonly requested — correct use of readonly on the Set field; the reference can't be reassigned but .add() / .clear() still work, exactly mirroring the old const requested.
Invariant documentation — the JSDoc block on the class is thorough and accurate. The three invariants (single concurrent drain, drainScheduled gate, outer-while picks up late arrivals) map 1-to-1 to the implementation.
enqueueMany — a clean addition that lets scheduleRenderOnConfigChange accumulate a batch of names and call scheduleDrain() exactly once rather than once per agent, which was the implicit intent of the old code.
flush() encapsulation — the polling loop (while draining || drainScheduled || size > 0) is now behind a named method instead of inlined in flushRenderQueue, which is cleaner.
Defensive early-return in drain() — if (this.draining) return is technically unreachable (because scheduleDrain() guards on draining before queuing a microtask), but keeping it is correct and safe.

Minor notes (non-blocking)

Commit prefix — fix: is slightly inaccurate for a pure refactor; refactor: would better match the conventional-commits convention in this repo. Low stakes since there's no semantic-release here.
enqueueMany([]) still calls scheduleDrain() — if the agent-type filter matches zero agents, enqueueMany is called with an empty array, which schedules a no-op microtask drain. The old code had identical behaviour (scheduleDrain() after the loop regardless of whether anything was added), so this is a pre-existing non-issue carried forward cleanly.
No test file changes — acceptable given the public API is byte-for-byte identical and the refactor is mechanical. A unit test for RenderQueue in isolation would be a good follow-up (especially for enqueueMany and the flush() idle-immediately case).

Verdict

LGTM. The encapsulation is correct, the invariants are preserved, and the class doc makes the concurrency model explicit. The notes above are cosmetic or pre-existing; nothing warrants a change request.

## Code Review — `fix: encapsulate render-queue state in a singleton class` CI: ✅ green. Single file changed (`render-queue.ts`, +152 / −91). Pure refactor — public API surface unchanged. --- ### Overview All mutable module-level state (`let draining`, `let drainScheduled`, `let _renderFn`, `let _listAgentsFn`, `const requested`) is folded into a `RenderQueue` class behind a single `const queue` singleton. The exported functions become one-line delegations. The drain logic, invariants, and test-seam contract are all preserved exactly. --- ### What's good - **`private readonly requested`** — correct use of `readonly` on the `Set` field; the reference can't be reassigned but `.add()` / `.clear()` still work, exactly mirroring the old `const requested`. - **Invariant documentation** — the JSDoc block on the class is thorough and accurate. The three invariants (single concurrent drain, `drainScheduled` gate, outer-while picks up late arrivals) map 1-to-1 to the implementation. - **`enqueueMany`** — a clean addition that lets `scheduleRenderOnConfigChange` accumulate a batch of names and call `scheduleDrain()` exactly once rather than once per agent, which was the implicit intent of the old code. - **`flush()` encapsulation** — the polling loop (`while draining || drainScheduled || size > 0`) is now behind a named method instead of inlined in `flushRenderQueue`, which is cleaner. - **Defensive early-return in `drain()`** — `if (this.draining) return` is technically unreachable (because `scheduleDrain()` guards on `draining` before queuing a microtask), but keeping it is correct and safe. --- ### Minor notes (non-blocking) 1. **Commit prefix** — `fix:` is slightly inaccurate for a pure refactor; `refactor:` would better match the conventional-commits convention in this repo. Low stakes since there's no semantic-release here. 2. **`enqueueMany([])` still calls `scheduleDrain()`** — if the agent-type filter matches zero agents, `enqueueMany` is called with an empty array, which schedules a no-op microtask drain. The old code had identical behaviour (`scheduleDrain()` after the loop regardless of whether anything was added), so this is a pre-existing non-issue carried forward cleanly. 3. **No test file changes** — acceptable given the public API is byte-for-byte identical and the refactor is mechanical. A unit test for `RenderQueue` in isolation would be a good follow-up (especially for `enqueueMany` and the `flush()` idle-immediately case). --- ### Verdict LGTM. The encapsulation is correct, the invariants are preserved, and the class doc makes the concurrency model explicit. The notes above are cosmetic or pre-existing; nothing warrants a change request.

charles merged commit 2b3cb65c68 into main

2026-05-13 22:01:49 +00:00

charles deleted branch code-lead/1140

2026-05-13 22:01:49 +00:00

charles referenced this pull request from a commit

2026-05-13 22:01:49 +00:00

fix: encapsulate render-queue state in a singleton class (#1140) (#1141)