refactor: unify debugger run-state + execution controls into one always-visible bar#168
Merged
Conversation
… 4.48.0
Phase 1-4 of Spec-Driven Development for unifying the debugger's run-state
indicator and execution controls. Adds docs/specs/debugger-exec-bar/{spec,plan,
tasks}.md and one failing test per acceptance criterion (AC-1..AC-11) ahead of
implementation. Version bumped 4.47.0 -> 4.48.0 (first commit of branch).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Single glanceable run-state indicator: word + non-colour glyph + colour, paused uses the info token (distinct from warning/error), role=status aria-live, halt address when paused. Export ExecutionState for the prop type (plan amendment). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Single execution-control surface: run-state badge + Step/Run-Pause/Reset + engine
switch, one global F5/F10 listener. Reset = keyDown('Tab'). Engine labelled
'Engine:' (distinct from run-state).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- AC-9: Actions becomes settings-only (run/pause/reset/engine removed). - AC-7: engine panel relabelled (Active:, not Status:); CPU flags relabelled Flags. - Mount the always-visible ExecutionControlsCluster in the right-panel tab row; it owns run/pause/step/reset + engine switch on every tab. - Strip the duplicate controls + run-state badge from the Disassembly toolbar (keeps →PC/F8/F9 navigation only) and delete the dead CompactExecutionControls. - Delete ExecutionControls (superseded by the bar); remove its Overview card. - Update existing Actions/AppContent/DebuggerLayout/Disassembler tests for the relocated controls. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The tab row + run-state badge + Step/Run-Pause/Reset + engine switch overflowed the right edge at common widths (Reset clipped, engine switch off-screen). Make the tab row and the cluster flex-wrap so the bar drops to its own line when space is tight (still inline at wide widths). Verified in-browser: running (green ▶) and paused (blue ⏸ $addr) states both fully visible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughExecution and engine controls were moved into ExecutionControlsCluster and RunStateBadge; Actions became a left-side settings panel (backspace, cycle timing, save/load). Disassembler and DebuggerLayout removed inline execution UI; EmulationContext now exports ExecutionState and AppContent integrates the new cluster and navigation state. ChangesExecution Controls Refactoring
Sequence Diagram(s)Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/components/__tests__/Actions-debugger-exec-bar.vitest.test.tsx (1)
12-29: ⚡ Quick winRemove the
unknowndouble-cast so this test enforces the real prop contract.Line 29 bypasses TypeScript checks and lets removed props linger, so this test won’t catch
ActionsPropsdrift.Proposed cleanup
-const props = { - onReset: vi.fn(), - onBS: vi.fn(), - supportBS: true, - onSaveState: vi.fn(), - onLoadState: vi.fn(), - onPauseResume: vi.fn(), - isPaused: false, - onRefocus: vi.fn(), - onCycleAccurateTiming: vi.fn(), - cycleAccurateTiming: true, - currentEngine: 'WASM', - wasmAvailable: true, - isSwitchingEngine: false, - onEngineSwitch: vi.fn(), -} as unknown as React.ComponentProps<typeof Actions>; +const props: React.ComponentProps<typeof Actions> = { + onBS: vi.fn(), + supportBS: true, + onSaveState: vi.fn(), + onLoadState: vi.fn(), + onRefocus: vi.fn(), + onCycleAccurateTiming: vi.fn(), + cycleAccurateTiming: true, +};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/__tests__/Actions-debugger-exec-bar.vitest.test.tsx` around lines 12 - 29, The test currently double-casts the props object with "as unknown as React.ComponentProps<typeof Actions>", which bypasses TypeScript checks and allows stale/removed props to remain; remove the "as unknown as ..." double-cast so the local props variable is typed directly as React.ComponentProps<typeof Actions> (or replace the literal with a factory/helper that returns a correctly-typed ActionsProps), update the props object to satisfy the real Actions prop type (remove any props no longer in ActionsProps and add required ones), and ensure the test compiles against the actual Actions prop contract instead of circumventing it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/__tests__/DebuggerLayout-debugger-exec-bar.vitest.test.tsx`:
- Around line 50-52: Add an explicit negative assertion to the AC-7 test that
checks the flag register: after the existing
expect(screen.getByText(/Flags/i)).toBeInTheDocument(); add an assertion using
screen.queryByText(/Status/i) and expect(...).not.toBeInTheDocument() so the
test also fails if the old "Status" label reappears; update the test block (the
test containing the Flags assertion) to include this negative check.
In `@src/components/ExecutionControlsCluster.tsx`:
- Around line 42-54: The global keydown handler in ExecutionControlsCluster.tsx
(inside the useEffect's onKeyDown) prevents default but doesn't stop
propagation, so handled shortcuts ('F10', ' ' (Space) when isPaused, and 'F5')
still reach AppContent's hidden-input and get forwarded to
workerManager.keyDown; update onKeyDown to call e.stopPropagation() (or
e.stopImmediatePropagation()) after e.preventDefault() for those handled
branches (the ones that call step() or handleRunPause()) so the key events do
not propagate to AppContent/workerManager.
In `@src/version.ts`:
- Line 1: The APP_VERSION constant currently set to '4.48.0' should be bumped
only at the patch level for this refactor PR—update the APP_VERSION export
(symbol: APP_VERSION) to the next patch (e.g., '4.48.1') so the version reflects
a patch increment rather than a minor/major change.
---
Nitpick comments:
In `@src/components/__tests__/Actions-debugger-exec-bar.vitest.test.tsx`:
- Around line 12-29: The test currently double-casts the props object with "as
unknown as React.ComponentProps<typeof Actions>", which bypasses TypeScript
checks and allows stale/removed props to remain; remove the "as unknown as ..."
double-cast so the local props variable is typed directly as
React.ComponentProps<typeof Actions> (or replace the literal with a
factory/helper that returns a correctly-typed ActionsProps), update the props
object to satisfy the real Actions prop type (remove any props no longer in
ActionsProps and add required ones), and ensure the test compiles against the
actual Actions prop contract instead of circumventing it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 94e0c9bf-738a-474a-aeab-f324b8b34a2a
📒 Files selected for processing (21)
src/components/Actions.tsxsrc/components/AppContent.tsxsrc/components/CompactExecutionControls.tsxsrc/components/DebuggerLayout.tsxsrc/components/DisassemblerPaginated.tsxsrc/components/EngineSwitcher.tsxsrc/components/ExecutionControls.tsxsrc/components/ExecutionControlsCluster.tsxsrc/components/RunStateBadge.tsxsrc/components/__tests__/Actions-debugger-exec-bar.vitest.test.tsxsrc/components/__tests__/Actions.vitest.test.tsxsrc/components/__tests__/AppContent.vitest.test.tsxsrc/components/__tests__/CompactExecutionControls.vitest.test.tsxsrc/components/__tests__/DebuggerLayout-debugger-exec-bar.vitest.test.tsxsrc/components/__tests__/DebuggerLayout.vitest.test.tsxsrc/components/__tests__/DisassemblerPaginated.vitest.test.tsxsrc/components/__tests__/EngineSwitcher-debugger-exec-bar.vitest.test.tsxsrc/components/__tests__/ExecutionControlsCluster-debugger-exec-bar.vitest.test.tsxsrc/components/__tests__/RunStateBadge-debugger-exec-bar.vitest.test.tsxsrc/contexts/EmulationContext.tsxsrc/version.ts
💤 Files with no reviewable changes (3)
- src/components/tests/CompactExecutionControls.vitest.test.tsx
- src/components/CompactExecutionControls.tsx
- src/components/ExecutionControls.tsx
Comment on lines
+42
to
+54
| useEffect(() => { | ||
| const onKeyDown = (e: KeyboardEvent) => { | ||
| if ((e.key === 'F10' || e.key === ' ') && isPaused) { | ||
| e.preventDefault(); | ||
| step(); | ||
| } else if (e.key === 'F5') { | ||
| e.preventDefault(); | ||
| handleRunPause(); | ||
| } | ||
| }; | ||
| window.addEventListener('keydown', onKeyDown); | ||
| return () => window.removeEventListener('keydown', onKeyDown); | ||
| }, [isPaused, step, handleRunPause]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and print the relevant region with line numbers
ls -la src/components/ExecutionControlsCluster.tsx
sed -n '1,140p' src/components/ExecutionControlsCluster.tsx | cat -n
# Search for the hidden input / AppContent forwarding path for keydown -> workerManager.keyDown
rg -n "keyDown|keydown" src/components src | head -n 80
rg -n "workerManager\.keyDown|keyDown\(" src | head -n 80
rg -n "ExecutionControlsCluster" -S src | head -n 40
# Also locate AppContent and any key forwarding listeners/handlers
rg -n "AppContent" src | head -n 80
rg -n "hidden.*input|input.*hidden" src | head -n 80Repository: stid/Apple1JS
Length of output: 25875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant portion of the component (with line numbers)
echo "=== src/components/ExecutionControlsCluster.tsx (first 200 lines) ==="
sed -n '1,200p' src/components/ExecutionControlsCluster.tsx | cat -n
# Trace keydown -> workerManager.keyDown and any hidden-input key forwarding
echo "=== rg keydown/keyDown in src ==="
rg -n "keydown|keyDown" src | head -n 200
echo "=== rg workerManager.keyDown ==="
rg -n "workerManager\.keyDown" src || true
echo "=== Locate AppContent ==="
rg -n "AppContent" src | head -n 100
echo "=== Locate hidden input ==="
rg -n "hidden.*input|input.*hidden" src | head -n 100Repository: stid/Apple1JS
Length of output: 20684
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the keydown/hidden input handling in AppContent
echo "=== src/components/AppContent.tsx (lines ~80-200) ==="
sed -n '80,230p' src/components/AppContent.tsx | cat -n
echo "=== src/components/AppContent.tsx (lines ~230-330) ==="
sed -n '230,330p' src/components/AppContent.tsx | cat -n
echo "=== Search AppContent tests for F5/F10/Space ==="
rg -n "'F5'|\"F5\"|'F10'|\"F10\"|Space| ' ' |\" \"|\\bkey:\\s*'F5'|\\bkey:\\s*'F10'" src/components/__tests__/AppContent.vitest.test.tsx src/components/__tests__ -S || trueRepository: stid/Apple1JS
Length of output: 13237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect WorkerManager.keyDown passthrough
echo "=== src/services/WorkerManager.ts around keyDown ==="
sed -n '160,240p' src/services/WorkerManager.ts | cat -n
# Inspect apple1 WorkerAPI keyDown mapping
echo "=== src/apple1/WorkerAPI.ts around keyDown ==="
rg -n "keyDown\\(key: string\\)" -n src/apple1/WorkerAPI.ts
sed -n '450,560p' src/apple1/WorkerAPI.ts | cat -n
# Search for handling of F5/F10/space in WorkerAPI / other key mapping
echo "=== search for F5/F10/Space in apple1 ==="
rg -n "'F5'|\"F5\"|'F10'|\"F10\"|\\bSpace\\b|key === ' '" src/apple1 | head -n 200Repository: stid/Apple1JS
Length of output: 7619
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== rg for F5/F10/Space in AppContent tests ==="
rg -n "F5|F10|Space|' '" src/components/__tests__/AppContent.vitest.test.tsx || true
echo "=== rg for F5/F10/Space in ExecutionControlsCluster tests ==="
rg -n "F5|F10|Space|' '" src/components/__tests__/ExecutionControlsCluster-debugger-exec-bar.vitest.test.tsx || true
echo "=== Show ExecutionControlsCluster-debugger-exec-bar test file ==="
sed -n '1,220p' src/components/__tests__/ExecutionControlsCluster-debugger-exec-bar.vitest.test.tsx | cat -nRepository: stid/Apple1JS
Length of output: 5764
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the keyboard.write implementation usage
rg -n "keyboard\\.write\\(" src/apple1 src | head -n 50
# Locate keyboard class / write method
rg -n "class .*Keyboard|function .*Keyboard|keyboard.*write|write\\(key" src/apple1 | head -n 80
# Search for explicit handling of F5/F10/space keys in apple1
rg -n "'F5'|\"F5\"|'F10'|\"F10\"|key === 'F5'|key === 'F10'|key === ' '|key === \" \"|Space" src/apple1 | head -n 200
# Also search in apple1 for 'function key' or similar
rg -n "F5|F10|function key|space" src/apple1 | head -n 200Repository: stid/Apple1JS
Length of output: 1892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/apple1/WebKeyboard.ts ==="
sed -n '1,220p' src/apple1/WebKeyboard.ts | cat -n
echo "=== src/apple1/KeyboardLogic.ts ==="
sed -n '1,220p' src/apple1/KeyboardLogic.ts | cat -nRepository: stid/Apple1JS
Length of output: 4978
Stop Space (and handled shortcuts) from reaching the emulator keyboard input
In ExecutionControlsCluster.tsx, the global window keydown handler prevents default but doesn’t stop propagation, so AppContent’s hidden-input keydown listener still forwards the same key to workerManager.keyDown(e.key). For Space (' '), WebKeyboard.write treats single-character keys as ASCII (so it injects a space strobe into the Apple 1) while the shortcut simultaneously triggers step() when paused.
Suggested fix (capture + stopPropagation for handled shortcuts)
useEffect(() => {
const onKeyDown = (e: KeyboardEvent) => {
+ if (e.metaKey || e.ctrlKey || e.altKey) return;
+
if ((e.key === 'F10' || e.key === ' ') && isPaused) {
e.preventDefault();
+ e.stopPropagation();
step();
} else if (e.key === 'F5') {
e.preventDefault();
+ e.stopPropagation();
handleRunPause();
}
};
- window.addEventListener('keydown', onKeyDown);
- return () => window.removeEventListener('keydown', onKeyDown);
+ window.addEventListener('keydown', onKeyDown, true);
+ return () => window.removeEventListener('keydown', onKeyDown, true);
}, [isPaused, step, handleRunPause]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ExecutionControlsCluster.tsx` around lines 42 - 54, The global
keydown handler in ExecutionControlsCluster.tsx (inside the useEffect's
onKeyDown) prevents default but doesn't stop propagation, so handled shortcuts
('F10', ' ' (Space) when isPaused, and 'F5') still reach AppContent's
hidden-input and get forwarded to workerManager.keyDown; update onKeyDown to
call e.stopPropagation() (or e.stopImmediatePropagation()) after
e.preventDefault() for those handled branches (the ones that call step() or
handleRunPause()) so the key events do not propagate to
AppContent/workerManager.
| @@ -1 +1 @@ | |||
| export const APP_VERSION = '4.47.0'; | |||
| export const APP_VERSION = '4.48.0'; | |||
There was a problem hiding this comment.
Use a patch bump for this refactor PR.
For a refactor/ PR, this should be a patch increment, not minor. Update APP_VERSION from 4.47.0 to 4.47.1 (or next patch if already advanced).
As per coding guidelines: "src/version.ts: Bump src/version.ts before every PR — use patch/minor/major per branch prefix (refactor/ = patch)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/version.ts` at line 1, The APP_VERSION constant currently set to '4.48.0'
should be bumped only at the patch level for this refactor PR—update the
APP_VERSION export (symbol: APP_VERSION) to the next patch (e.g., '4.48.1') so
the version reflects a patch increment rather than a minor/major change.
…ke conflict The always-visible bar's global keydown bound Space (when paused) to Step; Space also propagated to the hidden input and was injected into the Apple 1. Bind only the non-printable function keys (F10 step, F5 run/pause) and ignore modifier combos. Add a negative 'Status' assertion to the AC-7 flags test. Addresses CodeRabbit review on #168. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ExecutionControlsCluster.tsx (1)
48-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIgnore Shift-modified shortcuts in the global key handler.
Line 48 filters
meta/ctrl/altbut notshift, soShift+F5/Shift+F10still trigger emulator controls. That conflicts with the stated “ignore modifier combos” behavior and can steal browser shortcuts.Suggested patch
- if (e.metaKey || e.ctrlKey || e.altKey) return; + if (e.metaKey || e.ctrlKey || e.altKey || e.shiftKey) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ExecutionControlsCluster.tsx` around lines 48 - 54, The global keydown handler currently ignores meta/ctrl/alt but not Shift, so add shift to the early-modifier check: treat e.shiftKey the same as e.metaKey/e.ctrlKey/e.altKey and return early if any modifier is present before handling F10/F5; update the handler around the checks that call step(), handleRunPause(), and reference isPaused so Shift+F5/Shift+F10 no longer trigger emulator controls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/ExecutionControlsCluster.tsx`:
- Around line 48-54: The global keydown handler currently ignores meta/ctrl/alt
but not Shift, so add shift to the early-modifier check: treat e.shiftKey the
same as e.metaKey/e.ctrlKey/e.altKey and return early if any modifier is present
before handling F10/F5; update the handler around the checks that call step(),
handleRunPause(), and reference isPaused so Shift+F5/Shift+F10 no longer trigger
emulator controls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 879d9ce2-f2df-4536-a448-f2efae96f731
📒 Files selected for processing (2)
src/components/ExecutionControlsCluster.tsxsrc/components/__tests__/DebuggerLayout-debugger-exec-bar.vitest.test.tsx
…xec-bar # Conflicts: # src/version.ts
Address CodeRabbit findings on PR #168: - ExecutionControlsCluster: bail on Shift modifier too, so Shift+F5/F10 no longer hijack run/pause/step (matches the "modifier combos left to browser/OS" intent). - Actions-debugger-exec-bar test: type props directly as ActionsProps instead of an `as unknown as` double-cast, so stale execution/engine props can't silently linger. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The debugger was impossible to read at a glance: the word "Status" labelled three unrelated things — run-state (RUNNING/PAUSED), the active CPU engine (WASM/JS), and the CPU flag register — all as similar coloured badges. And run/pause/step/reset controls were duplicated across four places (Overview card, Disassembly toolbar, the dead
CompactExecutionControls, and the left CRT panel) with inconsistent labels/styling. Paused was styled like a warning (amber), reading as an error.Approach (Spec-Driven Development)
Built via SDD (spec → plan → tasks → failing tests → red-green → audit; artifacts local in
docs/specs/debugger-exec-bar/).ExecutionControlsCluster) in the right-panel tab row — visible on every tab (Info/Inspector/Debugger). It is the single home for run/pause/step/reset + the engine switch, and owns the one global F5/F10 listener.RunStateBadge— run-state by three redundant channels (word + glyph + colour), never colour-only;role="status" aria-live. Paused isinfoblue, notwarningamber — a breakpoint pause is intentional, not a fault. Shows the halt address when paused.ExecutionControlsand the deadCompactExecutionControls.Tests
One failing test per acceptance criterion (AC-1..AC-11) written first, then driven to green. Existing Actions/AppContent/DebuggerLayout/Disassembler tests updated for the relocated controls.
yarn test:ci(eslint + tsc + vitest): 689 passed / 19 skipped. Cross-path audit: PASS (every AC bound to its handler file + anAC-N (RENDER)test).Visual verification (browser)
▶ RUNNING, Step disabled, Pause shown.⏸ PAUSED $0000, Step enabled, Run shown.JS/WASMswitch present, WASM highlighted.Version bumped 4.47.0 → 4.48.0.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UI Improvements
Tests
Version