PhpInterpreterService: capabilities.canRun not reactive to setWorkspace (non-signal hasBlocks) #813

New Issue

2026-07-23T11:54:26Z

spikerj commented

2026-07-23 11:54:26 +00:00

Summary

PhpInterpreterService (dev-tools-blockly/.../interpreters/php-interpreter.service.ts) computes capabilities (which gates the playground's Run button) from hasBlocks(), but hasBlocks reads a plain, non-signal field:

private workspace: any = null;
private readonly hasBlocks = () => this.workspace?.getAllBlocks(false).length > 0;

readonly capabilities = computed<InterpreterCapabilities>(() => ({
  canRun: this.hasBlocks() && !this._isRunning() && !this._isPhpWasmLoading(),
  ...
}));

setWorkspace(workspace: any): void { this.workspace = workspace; }  // no signal touched

Because computed() only tracks signal reads, this.workspace is not a dependency. Once capabilities() has been evaluated (e.g. during the toolbar's first render, while workspace is still null → canRun: false), calling setWorkspace(ws) does not invalidate the computed. capabilities().canRun stays false until some other dependency (_isRunning / _isPhpWasmLoading) happens to change — so the Run button can remain disabled even though blocks are present.

Contrast with the working sibling

JavascriptInterpreterService in the same folder handles this correctly: it keeps a _blockCount signal and an updateBlockCount() method, and hasBlocks = computed(() => this._blockCount() > 0). So its capabilities reacts to workspace changes. The PHP service appears to have missed that pattern.

Impact

In the PHP Blockly playground, if capabilities() is read before setWorkspace() (typical for a template binding evaluated on first change detection), the Run button stays disabled after the workspace loads until an unrelated state signal toggles.

Fix direction

Mirror the JS interpreter: back hasBlocks with a signal set from setWorkspace() (e.g. a _blockCount/_hasBlocks signal updated in setWorkspace), so capabilities recomputes when the workspace changes.

Notes

Found via unit tests in the frontend coverage sweep (PR #551). Current behavior pinned by a characterization test in php-interpreter.service.spec.ts referencing this issue.

### Summary `PhpInterpreterService` (`dev-tools-blockly/.../interpreters/php-interpreter.service.ts`) computes `capabilities` (which gates the playground's Run button) from `hasBlocks()`, but `hasBlocks` reads a **plain, non-signal field**: ```ts private workspace: any = null; private readonly hasBlocks = () => this.workspace?.getAllBlocks(false).length > 0; readonly capabilities = computed<InterpreterCapabilities>(() => ({ canRun: this.hasBlocks() && !this._isRunning() && !this._isPhpWasmLoading(), ... })); setWorkspace(workspace: any): void { this.workspace = workspace; } // no signal touched ``` Because `computed()` only tracks signal reads, `this.workspace` is not a dependency. Once `capabilities()` has been evaluated (e.g. during the toolbar's first render, while `workspace` is still `null` → `canRun: false`), calling `setWorkspace(ws)` does **not** invalidate the computed. `capabilities().canRun` stays `false` until some *other* dependency (`_isRunning` / `_isPhpWasmLoading`) happens to change — so the Run button can remain disabled even though blocks are present. ### Contrast with the working sibling `JavascriptInterpreterService` in the same folder handles this correctly: it keeps a `_blockCount` **signal** and an `updateBlockCount()` method, and `hasBlocks = computed(() => this._blockCount() > 0)`. So its `capabilities` reacts to workspace changes. The PHP service appears to have missed that pattern. ### Impact In the PHP Blockly playground, if `capabilities()` is read before `setWorkspace()` (typical for a template binding evaluated on first change detection), the Run button stays disabled after the workspace loads until an unrelated state signal toggles. ### Fix direction Mirror the JS interpreter: back `hasBlocks` with a signal set from `setWorkspace()` (e.g. a `_blockCount`/`_hasBlocks` signal updated in `setWorkspace`), so `capabilities` recomputes when the workspace changes. ### Notes Found via unit tests in the frontend coverage sweep (PR #551). Current behavior pinned by a characterization test in `php-interpreter.service.spec.ts` referencing this issue.

spikerj commented

2026-07-24 20:02:41 +00:00

Confirmed still present on master, fixed in spikersoft-angular PR #571 (fix/813-814-interpreter-caps-qr-escape) — awaiting CI/review.

Block count is now a signal (_blockCount + hasBlocks = computed(...)), mirroring the JavaScript interpreter that the ticket pointed at. I also wired blockly.component's workspace change listener to refresh all four interpreters, not just the JS one — otherwise canRun would react to the initial setWorkspace() but still go stale when the user adds or deletes blocks afterwards, which is the more common path.

Scope correction worth noting: this bug was not PHP-only. python-interpreter.service.ts and lua-interpreter.service.ts had the byte-identical hasBlocks = () => this.workspace?.getAllBlocks(false).length > 0 defect. Both are fixed in the same PR.

More interesting: their specs already knew. The Python spec carried this comment —

NOTE: capabilities is a computed() but hasBlocks() reads the non-signal workspace field, so canRun only re-evaluates when a tracked signal changes — it does NOT react to setWorkspace on its own (self-heals on the next run-state change). Documented, not filed: low impact (workspace is set once; any run toggles recompute).

and Lua's referred back to it ("memoizes on the non-signal workspace field (see #python note)"). So the same defect was found three times, filed once, and the two silent instances were assessed as low impact on the assumption that the workspace is set once — which the block-edit path contradicts. Both comments are now replaced with reactivity assertions.

Verified: feature-dev-tools-blockly 74 tests pass.

— Opus 5 Agent

Confirmed still present on master, fixed in spikersoft-angular **PR #571** (`fix/813-814-interpreter-caps-qr-escape`) — awaiting CI/review. Block count is now a signal (`_blockCount` + `hasBlocks = computed(...)`), mirroring the JavaScript interpreter that the ticket pointed at. I also wired `blockly.component`'s workspace change listener to refresh all four interpreters, not just the JS one — otherwise `canRun` would react to the initial `setWorkspace()` but still go stale when the user adds or deletes blocks afterwards, which is the more common path. **Scope correction worth noting: this bug was not PHP-only.** `python-interpreter.service.ts` and `lua-interpreter.service.ts` had the byte-identical `hasBlocks = () => this.workspace?.getAllBlocks(false).length > 0` defect. Both are fixed in the same PR. More interesting: their specs already knew. The Python spec carried this comment — > NOTE: `capabilities` is a computed() but hasBlocks() reads the non-signal `workspace` field, so canRun only re-evaluates when a tracked signal changes — it does NOT react to setWorkspace on its own (self-heals on the next run-state change). **Documented, not filed: low impact** (workspace is set once; any run toggles recompute). and Lua's referred back to it ("memoizes on the non-signal workspace field (see #python note)"). So the same defect was found three times, filed once, and the two silent instances were assessed as low impact on the assumption that the workspace is set once — which the block-edit path contradicts. Both comments are now replaced with reactivity assertions. Verified: `feature-dev-tools-blockly` 74 tests pass. — Opus 5 Agent

spikerj commented

2026-07-24 20:34:37 +00:00

Fixed and verified. spikersoft-angular PR #571 merged to master (721d171b).

Verified on master: all three interpreters (php, python, lua) now carry _blockCount + hasBlocks = computed(...), and blockly.component's workspace change listener refreshes all four so canRun tracks block edits, not just the initial setWorkspace(). The characterization test is flipped to assert the reactive behaviour, and Python/Lua gained equivalent guards where they previously had only explanatory comments.

PR CI was green end to end (test-and-lint, build, e2e-anonymous, e2e-smoke, SonarQube Scan). Closing.

— Opus 5 Agent

Fixed and verified. spikersoft-angular PR #571 merged to master (`721d171b`). Verified on master: all three interpreters (`php`, `python`, `lua`) now carry `_blockCount` + `hasBlocks = computed(...)`, and `blockly.component`'s workspace change listener refreshes all four so `canRun` tracks block edits, not just the initial `setWorkspace()`. The characterization test is flipped to assert the reactive behaviour, and Python/Lua gained equivalent guards where they previously had only explanatory comments. PR CI was green end to end (`test-and-lint`, `build`, `e2e-anonymous`, `e2e-smoke`, `SonarQube Scan`). Closing. — Opus 5 Agent

spikerj closed this issue

2026-07-24 20:34:37 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: spikerj/spikersoft-issues#813