Skip to content

feat: add waitTimeout option to executeTerminalCommand - #405

Open
88871 wants to merge 2 commits into
estruyf:mainfrom
88871:feat/terminal-command-wait-timeout
Open

feat: add waitTimeout option to executeTerminalCommand#405
88871 wants to merge 2 commits into
estruyf:mainfrom
88871:feat/terminal-command-wait-timeout

Conversation

@88871

@88871 88871 commented May 20, 2026

Copy link
Copy Markdown

Closes #400

Adds an optional waitTimeout parameter to executeTerminalCommand steps that lets users override the default 5-second shell-integration wait cap.

Usage:

{
  "command": "cargo build",
  "waitTimeout": 30000
}

Defaults to 5000ms so all existing demos are unaffected.

Changes:

  • packages/common/src/models/Demos.ts — added waitTimeout?: number to ITerminal interface (which Step extends)
  • apps/vscode-extension/src/services/TerminalService.ts — destructure waitTimeout from the step and forward it to waitForTerminalExecuted; waitForTerminalExecuted now accepts an optional maxWaitTimeMs parameter (defaults to 5000)
  • docs/public/demo-time.schema.json — added waitTimeout property to the executeTerminalCommand action block and a corresponding definition entry

Summary by CodeRabbit

  • New Features
    • Terminal command execution now supports a per-step timeout setting so users can control how long the app waits for shell integration to confirm command completion (default 5000 ms).
  • Documentation
    • Demo schema and configuration docs updated to expose the new timeout option and its validation (minimum 1 ms).

Review Change Stack

Note

Add waitTimeout option to TerminalService.executeCommand for shell integration

  • Adds an optional waitTimeout field to the ITerminal interface and the terminal step JSON schema, allowing per-step override of the shell integration confirmation timeout.
  • TerminalService.waitForTerminalExecuted now accepts an optional maxWaitTimeMs parameter, defaulting to the previous hardcoded 5000ms when omitted.
  • Behavioral Change: commands using shell integration will now wait up to waitTimeout ms (instead of always 5000ms) before proceeding; commands without a waitTimeout set are unaffected.

Macroscope summarized bd022d6.

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

Adds an optional per-step waitTimeout for executeTerminalCommand, surfaces it in the shared model and JSON schema, and forwards it through TerminalService.executeCommand into waitForTerminalExecuted (default 5000ms) for both auto-execute paths.

Changes

Terminal Wait Timeout Feature

Layer / File(s) Summary
Data contract and schema definition
packages/common/src/models/Demos.ts, docs/public/demo-time.schema.json
ITerminal adds optional waitTimeout?: number. executeTerminalCommand step schema gains waitTimeout referencing #/definitions/waitTimeout; definitions.waitTimeout added (number, default 5000, minimum 1).
Service implementation and timeout wiring
apps/vscode-extension/src/services/TerminalService.ts
executeCommand destructures waitTimeout from the step and passes it to waitForTerminalExecuted in both character-by-character and instant auto-execute flows. waitForTerminalExecuted accepts optional maxWaitTimeMs and defaults to 5000ms when omitted.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I nibble at timeouts, small and neat,
Grant demos more patience when builds run long.
Five seconds no longer the only beat,
Each step may hum its own extended song.
Hop on—commands finish steady and strong.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: adding a waitTimeout option to executeTerminalCommand.
Linked Issues check ✅ Passed The PR fully implements all coding requirements from issue #400: optional waitTimeout field on executeTerminalCommand steps, default 5000ms behavior preserved, forward to waitForTerminalExecuted, and schema validation added.
Out of Scope Changes check ✅ Passed All changes are directly scoped to implementing the waitTimeout feature across the three files: models, service implementation, and schema definition.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
docs/public/demo-time.schema.json (1)

1300-1304: ⚡ Quick win

Consider adding a minimum constraint to prevent confusing values.

The waitTimeout definition lacks a minimum constraint. Other numeric fields in this schema (like highlightBlur at lines 1343-1348 and highlightOpacity at lines 1350-1357) include minimum and maximum validation. Without a minimum, users could provide negative or zero values, which would cause immediate timeout and confusing behavior. A minimum of 0 or 1 would prevent this edge case.

📋 Suggested schema constraint
 "waitTimeout": {
   "type": "number",
   "default": 5000,
+  "minimum": 0,
   "title": "Maximum time in milliseconds to wait for shell integration to confirm command completion. Overrides the default 5000ms cap. Useful for long-running commands such as builds or test suites."
 },
🤖 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 `@docs/public/demo-time.schema.json` around lines 1300 - 1304, The waitTimeout
numeric schema (symbol: waitTimeout) lacks a minimum constraint, allowing
zero/negative values that cause immediate timeout; update the
demo-time.schema.json definition for waitTimeout to include a minimum (e.g.,
"minimum": 1) to prevent zero/negative inputs and keep it consistent with other
numeric fields that have minimum/maximum validation.
🤖 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.

Nitpick comments:
In `@docs/public/demo-time.schema.json`:
- Around line 1300-1304: The waitTimeout numeric schema (symbol: waitTimeout)
lacks a minimum constraint, allowing zero/negative values that cause immediate
timeout; update the demo-time.schema.json definition for waitTimeout to include
a minimum (e.g., "minimum": 1) to prevent zero/negative inputs and keep it
consistent with other numeric fields that have minimum/maximum validation.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 167bd0d3-e716-4802-aa40-1d3e3c97162b

📥 Commits

Reviewing files that changed from the base of the PR and between 4fded69 and 340f9d4.

📒 Files selected for processing (3)
  • apps/vscode-extension/src/services/TerminalService.ts
  • docs/public/demo-time.schema.json
  • packages/common/src/models/Demos.ts

88871 added 2 commits May 20, 2026 04:48
Adds an optional `waitTimeout` parameter to `executeTerminalCommand` steps
that lets users override the default 5-second shell-integration wait cap.
Existing steps without `waitTimeout` continue to use the 5000ms default,
making the change fully backward-compatible.
@88871
88871 force-pushed the feat/terminal-command-wait-timeout branch from 964d868 to bd022d6 Compare May 20, 2026 02:48
@sonarqubecloud

Copy link
Copy Markdown

@estruyf

estruyf commented May 20, 2026

Copy link
Copy Markdown
Owner

@88871 can you change it to the dev branch instead of main.

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.

executeTerminalCommand: allow overriding the 5s shell-integration wait cap

2 participants