[rvhelden]

The PR for integrating the new nushell debugger protocol into vscode.
With this PR comes all the features of the original repo https://github.com/rvhelden/vscode-nushell-dap

[rvhelden]

@fdncred Here is the PR for the debugger in vscode

[fdncred]

wow, that's a lot of changes. LOL

[fdncred]

Notes from my clanker.

Lint, both typechecks, compile and the 33 unit tests pass on d3b563b. The comments below come from reading through the debugger code. The examples for script-signatures.ts and split-args.ts come from running those helpers under node with the inputs shown.

The resolve.ts change is probably the one to settle first, since it affects language server and terminal users who never open the debugger. Most of the rest is the launch-time signature parsing. With some common signatures it skips the argument prompt when the script needs arguments, and with others it shows the prompt when the script needs none.

[rvhelden]

Also reverted much of the unrelated changes to keep it as small as possible

[fdncred]

let's do one more review to make sure things look in order and look at the ci issue as well.

[fdncred]

Notes from my clanker, second pass at 61f7edb.

Earlier review threads

All nine are addressed. Three of them have a follow-up, posted inline.

• resolve.ts bad setting: the PATH fallback and the warning are back. Follow-up: the warning now fires from every caller, including quietNushell, so one F5 can show it twice.
• script-signatures.ts (three threads: space-separated params, [^\]]* and nested modules, dashed optionals and comments): moving to ast --json fixes all of them, and only top-level defs are read now. Follow-up: a def with an attribute in front (@example, @category, @search-terms) comes back wrapped in AttributeBlock and gets skipped. That brings back the original bug, no prompt and main runs without its argument.
• entry-point.ts relative program: absoluteProgram resolves it. Follow-up: a relative cwd is used as the base for program, but it goes to the adapter unresolved.
• split-args.ts: --name="hello world" x now gives ['--name=hello world', 'x'].
• restart-on-save.ts: only manual saves of the session's program restart now.
• pinned-variable.ts: pinVariable/unpinVariable bump generation. Follow-up (low): the first fetch in visualizeVariable isn't guarded.
• descriptor-factory.ts: throws CancellationError, so only the resolver's message shows.

CI failure

The build failure on 61f7edb has nothing to do with the PR. GitHub builds the Docker image for lannonbr/vsce-action@master before the job's steps run, and its FROM node:20-slim pull got 504 Gateway Timeout from auth.docker.io three times in a row. Checkout, npm install, typecheck:webview, test:unit and vsce package were all skipped, so this head has never had a real CI run. Re-running the job should fix it. Since this comes from a fork, the re-run may need maintainer approval like the earlier action_required run did.

On 61f7edb locally: npm run lint, tsc -p client --noEmit, npm run typecheck:webview, npm run test:unit (35 pass), npm run compile and vsce package all succeed, and the .vsix has out/debug/ir-panel.html, visualize-panel.html and visualize-panel.webview.js.

Optional, and it could be its own PR: the workflow now runs actions/setup-node, so - run: npx @vscode/vsce package could replace lannonbr/vsce-action@master. That drops the Docker Hub pull from every run and stops depending on an unpinned @master.

Minor, no inline comment

• shared/html.ts makeNonce uses Math.random(). crypto.randomBytes(16).toString('base64') is the usual choice for a CSP nonce.
• test:unit passes a glob to node --test, which needs Node 21 or newer. CI pins 22, but CONTRIBUTING doesn't mention it, and on Node 20 the command fails.
• The Show IR button sits in the title bar of every Nushell editor, even with no debug session or with a nu older than 0.116. Adding inDebugMode to its when clause would hide it outside debugging, if that's what you want.
• The comment on LOCALS_REFERENCE in visualize-request.ts points at "the fork's crates/nu-dap/src/state.rs". nu-dap is upstream now, so the comment could point there.
• In restart-on-save.ts, the isNuDocument check is redundant once isSessionProgram matches the path.
• const nushell = async () => ensureNushell(); in extension.ts wraps a sync call in a promise. Meanwhile configuration-provider.ts imports resolveNushell directly instead of using the injected resolver, so the debugger gets nu two different ways.
• On main, the not-found error told the user to run "Nushell: Start Language Server" after installing. The new message dropped that. A setting change restarts the server, but installing nu onto PATH doesn't.

[fdncred]

Please respond to each review comment when you're done so we can know that each issue has been addressed. Thanks for this!

[rvhelden]

Sorry wasn't done yet, was backtracking some of my changes, I will let you know when I'm finished with the changes and comment on each of your findings with how I resolved it.

[rvhelden]

@fdncred Done reviewing comments, and the minor items you told in you review commit

• CI: packages with npx @vscode/vsce package instead of lannonbr/vsce-action.
• makeNonce now uses crypto.randomBytes(16).
• Node 20 LTS is now required and documented; test:unit no longer uses a glob.
• Show IR now only shows during a Nushell debug session.
• LOCALS_REFERENCE now points at upstream Nushell.
• Removed the redundant isNuDocument check.
• Removed the async wrapper; quietNushell keeps resolveNushell on purpose (no warning).
• The not-found message has the "Nushell: Start Language Server" hint again.

[fdncred]

great work!