feat(app): cell execution state indicators + flush partial stdout#103
feat(app): cell execution state indicators + flush partial stdout#103
Conversation
Add visual feedback during cell execution (issue #38) and fix prompts without trailing newlines being buffered indefinitely (issue #99). Execution state machine: idle → pending → running → success/error with colored left-border indicators and bracket notation in the gutter. Timeout-based flush (150ms) for partial stdout lines in both the React component and data layer, with IOPub classification deferral preserved. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[codex-cuj-presubmit][sha:da90538c443d3f40873b53af191700c2c9683e24] Required steps:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da90538c44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| setPending(true); | ||
| setPid(null); | ||
| setExitCode(null); | ||
| cellData.run(); |
There was a problem hiding this comment.
runCode always sets pending before invoking cellData.run(), but there is no failure path that clears it when execution never reaches PID/exit callbacks. This happens when NotebookData.runCodeCell cannot start (e.g. no runner endpoint) or when the stream errors before emitting exitCode, so the cell stays stuck in pending with a spinner and [*] indefinitely; add a way to clear pending on startup failure/error.
Useful? React with 👍 / 👎.
| if (stdoutBufferRef.current) { | ||
| flushTimerRef.current = setTimeout(() => { | ||
| flushTimerRef.current = null; | ||
| flushStdoutBuffer(); | ||
| }, 150); |
There was a problem hiding this comment.
The new 150ms timer flushes any partial stdout buffer via flushStdoutBuffer(), which falls back to plain-text output when parsing fails; if an IPykernel JSON line is split across frames and the next fragment arrives after 150ms, the first fragment is flushed prematurely and the full message can no longer be reconstructed, producing raw JSON/noisy output instead of parsed notebook output. Apply the same deferral guard used in bindStreamsToCell before scheduling this timeout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR introduces a visual execution state machine for notebook cells and implements timeout-based flushing of partial stdout to improve user experience when cells wait for input. The changes address issue #38 (unclear cell execution status) and issue #99 (prompts not visible when waiting for input).
Changes:
- Adds a 5-state execution lifecycle (idle → pending → running → success/error) with colored left-border indicators and bracket notation in the cell toolbar
- Implements 150ms timeout-based flush for partial stdout lines to display prompts like "Password:" or "[y/n]" that don't end with newlines
- Adds comprehensive test coverage with 13 unit tests and browser integration tests
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/components/Actions/Actions.tsx | Introduces CellExecState type, refactors RunActionButton to show execution states, adds state transition logic with pending/running/success/error indicators, updates bracket label to show state symbols |
| app/src/lib/notebookData.ts | Implements flush timer in bindStreamsToCell with 150ms timeout, adds deferral logic to preserve IOPub classification, clears timer in exitCode/errors/dispose paths |
| app/src/components/Actions/CellConsole.tsx | Adds flushTimerRef for 150ms timeout-based partial line flushing, includes cleanup in useEffect return |
| app/src/index.css | Adds colored left-border indicators for execution states (amber for pending/running, green for success, red for error) with pulse animation |
| app/src/components/Actions/Actions.test.tsx | Adds 6 tests covering state transitions from idle through pending/running to success/error, including re-run behavior |
| app/src/components/Actions/CellConsole.test.tsx | Adds 3 tests for flush behavior including partial line timeout, double-write prevention, and immediate complete line handling |
| app/test/browser/test-notebook-ui.sh | Adds Phase 8 to verify data-exec-state attribute and cell-bracket testid in rendered page |
| app/test/fixtures/notebooks/stdin-prompt-test.runme.md | New test fixture with three cells demonstrating stdin prompts: echo+read, password prompt, and y/n confirmation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (code !== null) { | ||
| setPid(null); | ||
| } |
There was a problem hiding this comment.
The state transition logic has a potential edge case: if exitCode arrives as null (line 349-350), it will be stored but won't clear the pending state unless code is non-null at line 352-354. This means if exitCode is explicitly set to null, the cell could remain in a non-idle state. Consider whether setExitCode(null) should also clear pending state, or if null exitCode should be ignored entirely.
| if (code !== null) { | |
| setPid(null); | |
| } | |
| setPid(null); |
| const sequenceLabel = useMemo(() => { | ||
| if (!cell) { | ||
| return " "; | ||
| } | ||
| if (execState === "error") return "!"; | ||
| if (execState === "pending" || execState === "running") return "*"; | ||
| if (!cell) return " "; | ||
| const seq = Number(cell.metadata[RunmeMetadataKey.Sequence]); | ||
| if (!seq) { | ||
| return " "; | ||
| } | ||
| return seq.toString(); | ||
| }, [cell, pid, exitCode]); | ||
| return seq ? seq.toString() : " "; | ||
| }, [cell, execState]); |
There was a problem hiding this comment.
The bracket label will always display "[ ]", "[]", or "[!]" even when there's a sequence number in error/pending/running states. Once a cell has been successfully executed and has a sequence number, if it's re-run and enters pending/running state, users lose visibility of which execution number it was. Consider displaying the sequence number alongside the state indicator (e.g., "[1]" for cell 1 running) to maintain execution history context.
| expect(text.match(/Password:/g)?.length).toBe(1); | ||
| document.body.removeChild(div); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The test case "does not double-write when newline arrives after flush" validates that the buffer is properly cleared after a flush. However, it doesn't verify what happens if more data arrives while the timer is running but before it fires. Consider adding a test case where: 1) partial data arrives, 2) timer is scheduled, 3) more data arrives completing the line before timer fires, 4) verify the line appears once and timer is cleared.
| it("does not double-write when line completes before flush timer fires", async () => { | |
| vi.useFakeTimers(); | |
| const { streams, div } = await renderWithStreams(); | |
| // Emit partial line (no newline) to schedule the flush timer | |
| await act(async () => { | |
| streams.emitStdout(new TextEncoder().encode("Password:")); | |
| }); | |
| // Advance time, but not enough to trigger the flush | |
| await act(async () => { | |
| vi.advanceTimersByTime(100); | |
| }); | |
| // Partial line should still be buffered | |
| expect(getTerminalText(div)).not.toContain("Password:"); | |
| // Now complete the line before the timer fires | |
| await act(async () => { | |
| streams.emitStdout(new TextEncoder().encode("\n")); | |
| }); | |
| // "Password:" should appear exactly once after completing the line | |
| let text = getTerminalText(div); | |
| expect(text.match(/Password:/g)?.length).toBe(1); | |
| // Advance timers past the flush threshold; line should not be written again | |
| await act(async () => { | |
| vi.advanceTimersByTime(200); | |
| }); | |
| text = getTerminalText(div); | |
| expect(text.match(/Password:/g)?.length).toBe(1); | |
| document.body.removeChild(div); | |
| }); |
| // Fake streams for testing PID/exitCode callbacks through CellConsole subscriptions. | ||
| class FakeStreams { | ||
| private pidCbs = new Set<(pid: number) => void>(); | ||
| private exitCbs = new Set<(code: number) => void>(); | ||
| private stdoutCbs = new Set<(data: Uint8Array) => void>(); | ||
| private stderrCbs = new Set<(data: Uint8Array) => void>(); | ||
|
|
||
| stdout = { | ||
| subscribe: (cb: (data: Uint8Array) => void) => { | ||
| this.stdoutCbs.add(cb); | ||
| return { unsubscribe: () => this.stdoutCbs.delete(cb) }; | ||
| }, | ||
| }; | ||
| stderr = { | ||
| subscribe: (cb: (data: Uint8Array) => void) => { | ||
| this.stderrCbs.add(cb); | ||
| return { unsubscribe: () => this.stderrCbs.delete(cb) }; | ||
| }, | ||
| }; | ||
| pid = { | ||
| subscribe: (cb: (pid: number) => void) => { | ||
| this.pidCbs.add(cb); | ||
| return { unsubscribe: () => this.pidCbs.delete(cb) }; | ||
| }, | ||
| }; | ||
| exitCode = { | ||
| subscribe: (cb: (code: number) => void) => { | ||
| this.exitCbs.add(cb); | ||
| return { unsubscribe: () => this.exitCbs.delete(cb) }; | ||
| }, | ||
| }; | ||
|
|
||
| emitPid(pid: number) { this.pidCbs.forEach((cb) => cb(pid)); } | ||
| emitExitCode(code: number) { this.exitCbs.forEach((cb) => cb(code)); } | ||
| emitStdout(data: Uint8Array) { this.stdoutCbs.forEach((cb) => cb(data)); } | ||
|
|
||
| setCallback() {} | ||
| sendExecuteRequest() {} | ||
| close() {} | ||
| } |
There was a problem hiding this comment.
The FakeStreams class is duplicated between CellConsole.test.tsx and Actions.test.tsx with identical implementation. Consider extracting this to a shared test utility file to reduce code duplication and ensure consistency across test files. This would also make it easier to maintain and extend the FakeStreams API in the future.
| @keyframes pulse-border { | ||
| 0%, 100% { border-left-color: #fbbf24; } |
There was a problem hiding this comment.
The hardcoded color value #fbbf24 in the keyframe animation should use the Tailwind amber-400 color for consistency. The @apply directive at line 156 uses border-l-amber-400, but the animation hardcodes a hex value. If Tailwind's amber-400 color is ever changed in the theme, these values will diverge. Consider using CSS custom properties or ensuring the hex value matches Tailwind's amber-400 exactly.
| @keyframes pulse-border { | |
| 0%, 100% { border-left-color: #fbbf24; } | |
| :root { | |
| /* Tailwind amber-400 */ | |
| --color-amber-400: #fbbf24; | |
| } | |
| @keyframes pulse-border { | |
| 0%, 100% { border-left-color: var(--color-amber-400); } |
|
[codex-cuj-presubmit][sha:e0a0ea09615255def5b067513e014f0b83155fe2] Required steps:
|
|
[codex-cuj-presubmit][sha:4e2daeeed3589362666d7ffc17d25d0df4d97046] Required steps:
|
hamelsmu
force-pushed
the
feat/cell-exec-state-and-stdout-flush
branch
from
4e2daee to
da90538
Compare
Summary
idle → pending → running → success → error) with colored left-border indicators on cell cards and bracket notation ([ ],[*],[1],[!]) in the gutter — addresses Cell Execution Status isn't clear #38Password:,[y/n]) in both the React CellConsole component and thebindStreamsToCelldata layer, with IOPub classification deferral preserved — addresses Not clear when bash cell output waiting for input #99stdin-prompt-test.runme.md) and Phase 8 intest-notebook-ui.shChanges
Actions.tsxCellExecStatetype, rewroteRunActionButton, addedpendingstate +handlePid+execStatederivation,data-exec-stateattribute, colored bracketCellConsole.tsxflushTimerRefwith 150ms timeout for partial lines, cleanup on unmountnotebookData.tsflushTimerinbindStreamsToCell,appendStdoutChunkreturns boolean to preserve IOPub deferral, timer cleanup in exitCode/errors/disposeindex.csspulse-borderkeyframe animation on border-left-colorActions.test.tsxCellConsole.test.tsxtest-notebook-ui.shdata-exec-stateandcell-bracketin pagestdin-prompt-test.runme.mdTest plan
pnpm run buildpassesvitest run src/components/Actions/)test-smoke.sh) with frontend runningread -p "prompt: ", confirm prompt appears within ~150ms🤖 Generated with Claude Code