Bug Bash #1: Fix block execution, ANSI errors, cls scrollback, remove IPC timeout#592
Merged
tnaum-ms merged 9 commits intorel/release_0.8.0from Apr 20, 2026
Merged
Conversation
… statements
When a playground block starts with a bare direct command like 'use mydb'
followed by a query on the next line, only the 'use' is executed and the
query is silently discarded. This happens because the evaluation pipeline
detects 'use' as a special command token by splitting on all whitespace
(including newlines), consuming the entire multi-line input as arguments
to the 'use' handler.
Fix: normalize bare 'use <name>' and 'show <name>' into function-call
form ('use("name")' / 'show("name")') before evaluation when the
input contains multiple lines. The function-call form bypasses the
direct command short-circuit and lets the full block be evaluated
normally. Single-line inputs are left unchanged.
Bug Bash #1 — two additional fixes: 1. Playground error messages contained raw ANSI escape sequences (color codes, bold, reset) that rendered as visible control characters in the read-only output panel. Added stripAnsi() in resultFormatter.ts to remove SGR sequences before display. The interactive shell is unaffected (it renders ANSI codes via the PTY). 2. The cls/clear command in the interactive shell cleared the visible screen but left the scrollback buffer intact — users could scroll up to see previous output. Added the \x1b[3J (Erase Scrollback) escape sequence to match standard terminal behavior.
tnaum-ms
changed the title
use/show in multi-line playground blocks silently drops subsequent statementsuse/show block execution, ANSI codes in errors, cls scrollback
tnaum-ms
changed the title
use/show block execution, ANSI codes in errors, cls scrollback
tnaum-ms
force-pushed
the
db/tnaum/bug-bash-1-fixes
branch
from
1db925f to
a704078
Compare
The documentDB.timeout setting controlled a setTimeout that killed the worker thread after 30s, ignoring user-specified .maxTimeMS() values. Changes: - Removed the documentDB.timeout setting entirely - Removed all IPC-level setTimeout logic for eval messages in WorkerSessionManager — the user cancels via Cancel button or Ctrl+C - Init timeout (documentDB.shell.initTimeout) is unchanged - MaxTimeMSExpired errors from the DocumentDB API are now detected in both playground and shell, with a tip to use .maxTimeMS()
tnaum-ms
force-pushed
the
db/tnaum/bug-bash-1-fixes
branch
from
a704078 to
6acebbb
Compare
tnaum-ms
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes several Query Playground and Interactive Shell “Bug Bash #1” issues by normalizing multi-line use/show handling, improving error output readability, making terminal clear fully clear scrollback, and removing the IPC-level eval timeout in favor of query-level timeouts/cancellation.
Changes:
- Normalize bare
use <db>/show <arg>in multi-line blocks to avoid@mongoshdirect-command short-circuiting. - Strip ANSI SGR sequences from playground error output and surface a
.maxTimeMS()tip for DB timeout errors (code 50) in both shell and playground. - Remove the
documentDB.timeoutsetting and IPC eval timeout plumbing; keep init timeout behavior; clear terminal scrollback oncls/clear.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/extensionVariables.ts | Removes the documentDB.timeout settings key constant. |
| src/documentdb/shell/ShellSessionManager.ts | Drops eval-timeout parameter from evaluate() and worker calls. |
| src/documentdb/shell/DocumentDBShellPty.ts | Removes shell timeout usage, adds code-50 timeout tip, and clears scrollback on clear. |
| src/documentdb/shell/DocumentDBShellPty.test.ts | Updates mocks/assertions to match new evaluate() signature and clear sequence. |
| src/documentdb/playground/workerTypes.ts | Adds optional code to evalError IPC messages. |
| src/documentdb/playground/resultFormatter.ts | Adds ANSI stripping for error output and code-50 timeout tip. |
| src/documentdb/playground/resultFormatter.test.ts | Adds unit tests for ANSI stripping and code-50 hint behavior. |
| src/documentdb/playground/playgroundWorker.ts | Extracts numeric error code and includes it in evalError messages. |
| src/documentdb/playground/WorkerSessionManager.ts | Removes eval IPC timeout; makes timeouts optional for init/shutdown; reconstructs error code. |
| src/documentdb/playground/PlaygroundEvaluator.ts | Stops reading/passing documentDB.timeout; updates VS Code import to type-only. |
| packages/documentdb-shell-runtime/src/index.ts | Re-exports normalizeDirectCommands from the package entrypoint. |
| packages/documentdb-shell-runtime/src/DocumentDBShellRuntime.ts | Adds normalizeDirectCommands() and applies it before evaluation. |
| packages/documentdb-shell-runtime/src/DocumentDBShellRuntime.test.ts | Adds tests for normalization behavior. |
| package.json | Removes the documentDB.timeout contributes.configuration entry. |
| l10n/bundle.l10n.json | Adds localized string for the .maxTimeMS() tip. |
The previous global regex rewrote any matching `use <name>` / `show <name>` line anywhere in the multi-line input, including text inside template literals, multi-line strings, and block comments. That changed program semantics for any block that happened to contain the pattern in a non-code region. Scope the rewrite to the first non-empty statement line only — that is the single observed failure mode (bare `use`/`show` as the first token of a multi-line block short-circuits the evaluator and silently drops the rest of the block). Addresses PR #592 review comment (discussion r3111470977).
`normalizeDirectCommands()` rewrote `use <name>` to `use("<name>")`
without a trailing semicolon. If the next line began with `[`, `(`,
`+`, `-`, or `/`, JavaScript's Automatic Semicolon Insertion would
fail to split the statements and parse them as a single expression
(e.g. `use("mydb")[1,2,3].forEach(...)` becomes member access on
the call result).
Always emit a trailing `;` in the rewrite so the next line is
guaranteed to start a fresh statement. Added a regression test.
Addresses PR #592 review comment (discussion r3111470941).
The `documentDB.timeout` setting was removed in the preceding commit but a few references lingered in JSDoc examples and in the shell terminal link-provider tests. Replace them with `documentDB.shell.initTimeout` (the remaining timeout-related setting) so the code and tests no longer reference a non-existent configuration key. No behavioral change. Addresses PR #592 review comment (discussion r3111470914).
`normalizeDirectCommands` is an internal helper applied automatically by `DocumentDBShellRuntime.evaluate()`. Re-exporting it from `@microsoft/documentdb-vscode-shell-runtime` expanded the public API surface for a test-oriented helper. The unit test already imports it directly from `./DocumentDBShellRuntime`, so the entrypoint export was unused by consumers and is safe to remove. Addresses PR #592 review comment (discussion r3111470833).
`sendRequest()` used a hardcoded `documentDB.shell.initTimeout` setting key in the `SettingsHintError` it raised on timeout. That made sense for the init call, but the shutdown path (5-second timeout, error is swallowed) would have produced a misleading hint had it ever surfaced, since `documentDB.shell.initTimeout` does not control shutdown. Parameterize the setting key. The init caller passes `documentDB.shell.initTimeout`; the shutdown caller omits it, which causes a plain `Error` to be thrown instead of a `SettingsHintError` with the wrong key. No user-facing behavior change today (shutdown swallows the error), but the class invariant is now consistent and robust to future callers.
Collaborator
Author
Additional follow-up fix:
|
The previous first-line-only heuristic was conservative but narrow:
a bare `use`/`show` on any subsequent line was still passed through
unchanged and would short-circuit the evaluator.
Switch to a tiny hand-written lexical scanner that tracks strings,
template literals (including `${...}` nesting), line/block comments,
and regex literals. Line starts that fall outside every one of those
contexts are candidates for rewriting, so a `use`/`show` anywhere
in the input is normalized while text inside quoted/escaped regions is
still left alone.
Zero new dependencies. A full JS parser (e.g. `acorn`) would be
needed to also disambiguate nested-block positions and declared-
identifier shadowing from statement starters, but those cases are not
observed in user input and adding a parser to a package intended to
ship standalone for CLI tooling is disproportionate.
Tests updated to assert the extended coverage (mid-block rewrites,
multiple top-level commands, strings/templates/comments/regex literals
left unchanged).
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
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.
Bug Bash #1 — Fixes
Four issues fixed, one closed as not reproduced.
1.
use/showin multi-line blocks silently drops subsequent statementsProblem: When a playground block starts with
use mydbfollowed by a query, only theuseexecutes. The query is silently discarded because the evaluation pipeline treats the entire multi-line input as arguments to theusehandler.Fix: Normalize bare
use <name>andshow <name>into function-call form in multi-line input before evaluation. Single-line inputs are unchanged.DocumentDBShellRuntime.ts: AddednormalizeDirectCommands()DocumentDBShellRuntime.test.ts: 13 new test casesCredit: Jade Welton
2. ANSI escape codes visible in playground error messages
Problem: Errors contain ANSI color codes that appear as raw escape characters in the playground output panel.
Fix: Added
stripAnsi()inresultFormatter.ts.resultFormatter.ts: AddedstripAnsi(), applied informatError()resultFormatter.test.ts: New test file with test casesCredit: Marcelo Fonseca
3.
cls/cleardoes not clear scrollback bufferProblem:
cls/clearclears the visible screen but leaves scrollback history.Fix: Added
\x1b[3J(Erase Scrollback) to the clear escape sequence.DocumentDBShellPty.ts:\x1b[2J\x1b[H→\x1b[2J\x1b[3J\x1b[HDocumentDBShellPty.test.ts: Updated assertionCredit: Marcelo Fonseca
4. Remove IPC timeout for query execution
Problem: The
documentDB.timeoutsetting controlled an IPC-levelsetTimeoutthat killed the worker thread after 30s — ignoring user-specified.maxTimeMS()values. The timeout was applied at the wrong layer (IPC, not query).Fix: Removed the
documentDB.timeoutsetting and all IPC-level timeout logic for eval messages entirely. Users control query execution time via:.maxTimeMS()in playground/shell codedocumentDB.shell.initTimeout) is unchangedAdditionally:
codefield on theevalErrormessage.maxTimeMS()tip in both playground and shellChanges
package.json: RemoveddocumentDB.timeoutsettingextensionVariables.ts: Removed setting keyworkerTypes.ts: AddedcodetoevalErrormessageplaygroundWorker.ts: Extracts and sends error codeWorkerSessionManager.ts: Timeout optional (init/shutdown only), reconstructs error codePlaygroundEvaluator.ts: No longer reads/passes timeoutShellSessionManager.ts: Removed timeout fromevaluate()signatureDocumentDBShellPty.ts: RemovedgetShellTimeoutMs(), added error code 50 detectionresultFormatter.ts: AddedisMaxTimeMSError()using error code 50Credit: Marcelo Fonseca
Not reproduced