Skip to content

fix(sessions): open test pane when enabling debug so the debug UI is visible#9998

Open
AlexRV12 wants to merge 1 commit into
mainfrom
ah/debug-pane-opens-in-sessions
Open

fix(sessions): open test pane when enabling debug so the debug UI is visible#9998
AlexRV12 wants to merge 1 commit into
mainfrom
ah/debug-pane-opens-in-sessions

Conversation

@AlexRV12

@AlexRV12 AlexRV12 commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

In an AI session (frontend/src/lib/components/sessions) with a script open in the right-hand preview panel, clicking Toggle Debug Mode did nothing visible — the debug pane never appeared. Clicking Test afterwards revealed it.

The debug UI (DebugToolbar + DebugPanel) is rendered inside the script editor's right-hand test/preview pane. In an AI session that pane is force-collapsed to width 0 (initialTestPanelCollapsed), and enabling debug flipped debugMode on and connected the DAP client but never widened the pane — so the debug UI mounted inside a zero-width pane and stayed invisible. The debug pane and the collapsed test panel are the same layout slot.

Changes

  • toggleDebugMode(): on the enable path, expand the test pane when it's collapsed (testPanelSize === 0) so the debug UI is immediately visible without a follow-up Test click.
  • Extract expandTestPanel() / collapseTestPanel() out of toggleTestPanel() so the debug enable path can express "expand" directly instead of relying on the toggle's branch semantics. Behavior-preserving: toggleTestPanel() keeps its testPanelSize > 0 ? collapse : expand dispatch and all four existing callers are unchanged.

Design decisions (deliberate)

  • One-shot imperative expand, not a reactive $effect. A standing "debugMode && collapsed → open" effect would fight the user — they could never collapse the pane while debugging. The invariant belongs at the transition (toggle time), not as a continuous rule.
  • Asymmetric: leave the pane open on disable. Auto-collapsing on disable would need "debug owns this pane" ownership state invalidated across every width-change path (drag handle, HideButton, Cmd+U, runTest resizes) — miss one and we collapse a pane the user deliberately kept open. It's also worse UX: the user is looking at the pane they just opened.
  • === 0 guard avoids re-toggling or snapping a pane the user already has open at a custom width.

Known limitation (future work)

The debug UI and the test pane still share one layout slot, so debugMode can be true while the UI is invisible on other width-change paths. This PR patches the one transition that matters today. If this slot keeps accreting modes (test / debug / preview / args), the real fix is to give debug its own slot or add an explicit "what is this pane showing" discriminant — out of scope here.

Test plan

  • npm run check — clean for ScriptEditor.svelte (pre-existing unrelated errors elsewhere untouched)
  • AI session, script open, pane collapsed → Toggle Debug Mode opens the pane immediately and shows the DebugPanel once the DAP client connects
  • Toggle Debug Mode off → debug turns off, pane stays open (deliberate)
  • Standalone /scripts/edit unchanged (pane already open; the === 0 branch never fires)
  • Glance at the pipeline asset-graph details pane (previewLayout="bottom") for the documented cosmetic re-open-on-enable case (min-clamped, non-breaking)

Screenshots

Before — AI session with a script open in the right panel; the test/preview pane is collapsed to width 0. Clicking Toggle Debug Mode used to do nothing visible.

page-2026-07-08T06-52-45-932Z

After enabling debug — one click on Toggle Debug Mode now expands the pane and, once the DAP client connects, shows the DebugPanel (step controls, breakpoints hint, args + logs).

page-2026-07-08T06-53-19-159Z

After disabling debug — debug turns off and the toolbar reverts to the normal Test button, but the pane is deliberately left open (see design decisions).

page-2026-07-08T06-54-04-862Z

Standalone /scripts/edit (regression check) — unchanged: the pane is open by default, so the new === 0 branch never fires.

page-2026-07-08T06-54-39-397Z

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 41c98fb
Status: ✅  Deploy successful!
Preview URL: https://6443725a.windmill.pages.dev
Branch Preview URL: https://ah-debug-pane-opens-in-sessi.windmill.pages.dev

View logs

@AlexRV12 AlexRV12 marked this pull request as ready for review July 8, 2026 07:20
@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @AlexRV12's task in 58s —— View job


Code Review

Good to merge.

I reviewed the single changed file (frontend/src/lib/components/ScriptEditor.svelte, +20/-5) against origin/main. The fix is correct, minimal, and the PR description already documents the design trade-offs well.

What the change does (verified)

  • toggleDebugMode() enable path now calls expandTestPanel() when testPanelSize === 0 (line 1296). This is the actual bug fix — the debug UI ({#if debugMode && isDebuggableScript} at line 1899) mounts inside the test pane, which initialTestPanelCollapsed seeds to rawTestPanelSize = 0 in AI sessions (line 1624). Before this PR, enabling debug mounted the UI into a zero-width slot.
  • expandTestPanel() / collapseTestPanel() are extracted verbatim from the two branches of toggleTestPanel() (lines 1631–1639). The refactor is behavior-preserving: toggleTestPanel() keeps its testPanelSize > 0 ? collapse : expand dispatch, and the drag/HideButton/Cmd+U callers are untouched.

Correctness checks

  • HoistingtoggleDebugMode() (line 1283) calls expandTestPanel() which is declared later at line 1631. Both are plain function declarations at instance scope, so hoisting applies and the forward reference resolves fine.
  • === 0 guard — matches the sentinel semantics of testPanelSize (rawTestPanelSize === 0 ? 0 : Math.max(...) at line 1626), so it correctly detects only the fully-collapsed state and won't re-snap a pane the user opened at a custom width.
  • Non-debuggable scripts — the toggle button only renders under {#if isDebuggableScript ...} (line 2565), so the "expand pane but debug UI never mounts" edge case can't be triggered from this control. Not a concern.
  • Asymmetric disable (leaving the pane open) — reasonable given there's no "debug owns this pane" ownership state to invalidate across the other width-change paths. Agree with keeping it a one-shot imperative expand rather than a reactive $effect, which would fight the user.

AGENTS.md compliance

  • Comments record the non-obvious constraint (why one-shot, not an effect) rather than narrating the code — consistent with the repo's comment guidance.
  • No $bindable(default_value) on optional props, no crypto.randomUUID(). Clean.

Test coverage

Frontend Svelte change with no new pure-logic utility (no sibling *.test.ts surface), so per repo policy no automated tests are expected here. The extracted expandTestPanel/collapseTestPanel are trivial reassignments and don't warrant unit tests.

Manual verification still worth doing before merge (matches the PR's own test plan): in an AI session with a script open and the test pane collapsed, click Toggle Debug Mode once and confirm the pane expands and the DebugPanel appears once the DAP client connects; toggle off and confirm the pane deliberately stays open; and re-check standalone /scripts/edit where the pane is open by default so the new === 0 branch never fires. The PR includes before/after screenshots covering these.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Pi Review

Good to merge.

No issues found. The change is tightly scoped — a single 1-file diff in ScriptEditor.svelte — and well-reasoned.

What it does

  • toggleDebugMode() now calls expandTestPanel() when the test pane is collapsed (testPanelSize === 0), so enabling debug in an AI session immediately reveals the debug UI instead of requiring a follow-up Test click.
  • expandTestPanel() and collapseTestPanel() are extracted from toggleTestPanel() as standalone helpers. toggleTestPanel() dispatches through them identically to before — all four callers (Cmd+U, HideButton ×2, on:toggleTestPanel) are unchanged.

Design checks

  • One-shot, not reactive — the comment at the guard site states the invariant clearly: a reactive $effect would fight the user by re-opening the pane whenever they collapsed it mid-debug. This follows the AGENTS.md principle: "State each invariant once, at the place where someone would break it."
  • Asymmetric (leave open on disable) — documented in the PR body with a thorough rationale covering the ownership-state tracking edge cases. Reasonable trade-off.
  • === 0 guard — avoids snapping a pane the user already has at a custom width.
  • No $bindable(default_value) on optional props introduced.
  • No raw HTML or custom CSS — no svelte-frontend skill violations.

Test coverage

Frontend-only Svelte component change. The codebase does not test Svelte components, and the extracted helpers (expandTestPanel, collapseTestPanel) are tightly coupled to component state — no sibling *.test.ts pattern applies here. No automated tests expected.

Manual verification: the PR body includes a thorough test plan with screenshots covering all four scenarios (AI session enable, AI session disable, standalone /scripts/edit regression, pipeline preview layout). No additional manual verification needed before merge.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex Review

Good to merge

No issues found. I checked the Svelte diff for bugs, security/performance regressions, AGENTS.md compliance, and new public surfaces.

Test coverage

Frontend-only UI behavior change in frontend/src/lib/components/ScriptEditor.svelte. No automated component test is expected under the repo policy, and no new pure-logic utility was added. I did not run Playwright as part of this read-only review; before merge, manually verify that enabling debug mode from a collapsed test pane opens the pane and shows the debug UI, and that collapsing it while debug mode remains enabled does not immediately reopen it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant