Skip to content

Add language server, test suite, bug fixes, and TypeScript migration#29

Open
wshlavacek wants to merge 53 commits into
RuleWorld:mainfrom
wshlavacek:main
Open

Add language server, test suite, bug fixes, and TypeScript migration#29
wshlavacek wants to merge 53 commits into
RuleWorld:mainfrom
wshlavacek:main

Conversation

@wshlavacek
Copy link
Copy Markdown

@wshlavacek wshlavacek commented Apr 6, 2026

Summary

Contributes back the improvements from the wshlavacek/BNG_vscode_extension fork, as discussed in #28.

  • Adds a BNGL language server with diagnostics, completions, hover, go-to-definition, and find references.
  • Migrates the extension to TypeScript + esbuild and splits the code into focused modules.
  • Expands tests, documentation, themes, and package/developer tooling.
  • Fixes Python/PyBioNetGen command execution so run and visualize use the selected interpreter reliably.
  • Adds a GitHub Actions workflow for type-checking, build, grammar tests, and VS Code extension tests.

Python/PyBioNetGen execution fixes

  • Preserves the Python extension execCommand as a command plus arguments instead of flattening it into a single string.
  • Invokes PyBioNetGen through from bionetgen.main import main as _bng_main; raise SystemExit(_bng_main()), which works with current PyPI releases that do not support python -m bionetgen.
  • Treats process launch failures and missing exit codes as errors instead of reporting false success.

Testing

  • 96 unit/integration tests via @vscode/test-electron + Mocha.
  • 10 TextMate grammar fixture files via vscode-tmgrammar-test.
  • Manual smoke test in a VS Code Development Host on macOS using a selected Conda interpreter for BNG: Install/Check PyBioNetGen, BNG: Run Simulation, auto-open .gdat and plot viewer, and BNG: Visualize Network.

Test plan

  • npm run check-types
  • npm test
  • npm run test:grammar
  • Manual smoke test in VS Code Development Host on macOS using a selected Conda interpreter

Closes #28
Closes #30

google-labs-jules Bot and others added 20 commits April 2, 2026 00:36
- Handled block synonyms (rules/reaction rules, molecules/molecule types, etc.)
- Ignored trailing comments on 'begin' and 'end' lines
- Fixed runaway fold widgets by clearing mismatched inner blocks from the stack
- Refined '#@' comment folding: sections now fold independently and skip single lines
- Added section descriptions to metadata comment folding logic

Co-authored-by: wshlavacek <19327223+wshlavacek@users.noreply.github.com>
…7292504028945371174

Improve BNGL code folding robustness and correctness
Migrate extension from JavaScript to TypeScript, add robust code folding
for begin/end blocks and #@ comment blocks, modernize build tooling with
esbuild, and improve UI/UX including plotting enhancements.

Squash-merged from Jules' branch (fix/improve-code-folding-bngl).
- Fix invalid JSON in tmLanguage (unquoted keys broke syntax highlighting)
- Fix CSP policy to allow Plotly (unsafe-inline styles, unsafe-eval, blob/data img)
- Fix plot data index alignment between names[] and data[] arrays
- Restore parse_dat .slice(1) to skip # header prefix
- Add sidebar controls for axis scale (linear/log), line style, and export
- Replace multiple toolbar buttons with single BioNetGen icon + QuickPick dropdown
- Consolidate build scripts into top-level package.json, remove duplicate in contributes
- Fix esbuild watch problem matcher in tasks.json
- Add dev/ to .gitignore
Bugs fixed: nested folding splice, command registration order, getPythonPath
null-safety, parse_dat empty file crash, checkGdat race condition, save_image
validation, webview HTML escaping, synchronous file read, process manager
polling and stdout buffering.

Removed duplicate plot_dat command, unused menus setting, .graphml language
hijacking. Added light/hc-dark/hc-light themes, legend toggle, lasso
selection, CI pipeline. Simplified dropdown menu labels, fixed graphml
viewer layout, auto-open visualization results. Updated repo URLs, docs,
changelog, and README with settings/snippet reference.
- Add @vscode/test-electron + mocha test infrastructure (24 tests)
  - parseDat: 9 unit tests (normal data, empty, header-only, edge cases)
  - Folding provider: 7 tests (nested blocks, mismatched, aliases, metadata)
  - Command registration: 8 integration tests (all commands verified)
- Extract parseDat to src/parseDat.ts with empty-data guard (fixes RuleWorld#4)
- Split extension.ts (615→56 lines) into focused modules:
  - src/commands/handlers.ts — run, viz, setup, upgrade handlers
  - src/commands/menu.ts — QuickPick menu
  - src/plotting/PlotPanel.ts — webview panel class
  - src/folding/foldingProvider.ts — begin/end + #@ folding
- Add .vscode-test/ and out/ to .gitignore
Previously the menu silently returned when clicked from a webview panel.
Now it shows Install/Upgrade options regardless of editor context.
…nd hover

Implements LSP phases 23a-23h from the development plan:
- Language server (src/server/server.ts) with IPC transport
- Line-by-line state machine parser (src/server/parser.ts) producing structured AST
- Diagnostics: mismatched begin/end blocks, duplicate definitions, unused parameters, empty blocks
- Autocomplete: block types, 28 actions with snippets, parameters, functions, observables,
  molecule types, 28 built-in math functions, rate law types (Sat/MM/Hill/Arrhenius/FunctionProduct),
  math constants (_pi, _e)
- Go-to-definition and Find All References for all symbol types
- Hover: parameter values, molecule signatures, function bodies, action docs, built-in function docs
- Parser handles: line continuation (\), top-level actions, compartments, energy patterns,
  rule labels, DeleteMolecules/MoveConnected/TotalRate/priority keywords, $ species modifier,
  Counter observables, protocol blocks, string-aware comment stripping, numeric line labels
- 68 new parser unit tests including integration tests against real BNGL model files
…erence

- Delete 6 outdated GIF files (featured, plotting, runner, snippets, visualize)
- Delete stale bngl-grammar-ebnf.pdf (from 2018, missing tfun and other features)
- Add docs/bngl-grammar.md: comprehensive grammar reference derived from BioNetGen
  source code, covering all block types, pattern syntax, rate law types, built-in
  functions, actions, and math constants
- Update README.md, docs/guide.md, docs/source/usage.rst, docs/source/index.rst
  to remove broken GIF references and describe current UI
10 test files covering scope assertions for all major BNGL constructs:
parameters, molecule types, reaction rules, observables, functions,
actions, species/compartments, expressions/built-ins, comments, and
a full model integration test. Run via: npm run test:grammar
- install.rst: simplified, mentions PyBioNetGen auto-install, VSIX install option
- developers.rst: rewritten with architecture overview, npm scripts table,
  testing instructions, and VSIX packaging
- README: rename to "BioNetGen (BNG) VS Code Extension", rewrite intro
  to describe the full platform (editing, simulation, visualization,
  plotting), add Usage section with workflow walkthrough, replace
  "Professional UI" with specific feature descriptions, remove "alpha
  stage" release note
- docs/install.md: complete rewrite with three installation methods
  (marketplace, VSIX for evaluators, source for developers) including
  full build-from-source instructions
- .vscodeignore: update to exclude src/, docs/, dev/, .github/, and
  TypeScript source files from the packaged extension
- jsconfig.json: scope to media/ only (TypeScript files use tsconfig.json)
- Move example.bngl and nfkb.bngl from repo root to examples/
- Add copyright line for William S. Hlavacek (2026) to LICENSE
- Add Resources section to README linking all docs: starter guide,
  install guide, BNGL grammar reference, example models, developer
  guide, and ReadTheDocs
- Link built-in functions table from grammar reference in Features
Copy Gardner2000.bngl and Kholodenko2000.bngl into examples/ so
integration tests don't depend on a sibling repo path. Update test
paths to reference examples/ relative to the repo root.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolves RuleWorld/PyBioNetGen#62 — the extension now runs
`python -m bionetgen` using the resolved workspace interpreter rather
than spawning bare `bionetgen` via PATH lookup. This fixes ENOENT errors
and wrong-binary issues on macOS when the venv is not on the extension
host PATH.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@wshlavacek
Copy link
Copy Markdown
Author

wshlavacek commented May 12, 2026

I took a closer look at this PR and I do not think the "4 of 5 tasks" indicator is the real issue. That appears to be just the markdown checklist in the PR body; the remaining unchecked item is the note about additional real-world testing. GitHub itself was showing the PR as mergeable when I checked.

The bigger concern is merge readiness versus maintainer time to validate a large rewrite.

More detailed notes from local review:

  1. Python command launching may be incorrect on some setups.
  • In src/utils/getPythonPath.ts, the code reads the Python extension's getExecutionDetails(...).execCommand value, which is an array.
  • It then does execCommand.join(" ") and returns a single string.
  • Later, src/utils/spawnAsync.ts passes that string into child_process.spawn(command, args) as the executable name.
  • That only works reliably if the Python extension returns a bare interpreter path.
  • If it returns an executable plus extra arguments or some wrapper form, the extension may try to spawn a non-existent executable like "/path/to/python -X utf8", which can break run, visualize, setup, and upgrade.
  1. The validation/CI story is weaker than the PR description suggests.
  • package.json defines npm run lint as eslint ..
  • But eslint is not present in devDependencies, so npm run lint fails on a clean checkout.
  • The GitHub Actions workflow currently runs npm ci, npm run check-types, and npm run build, but not lint, not npm test, and not npm run test:grammar.
  • So even if CI is green, that currently only demonstrates install + typecheck + build, not the full validation described here.
  1. My local spot-check was mixed.
  • npm run check-types passed.
  • npm run test:grammar passed.
  • npm test got through downloading VS Code and installing the Python extension, but I could not independently confirm the integration suite from my environment because the Electron test host exited with bad option launch errors.

Given the size of this PR and the limited time available to validate it carefully, I think the safe interpretation is: this looks promising, but it does not feel ready to merge without at least fixing the Python-launching issue and tightening or clarifying the validation story.

@wshlavacek wshlavacek marked this pull request as draft May 12, 2026 21:17
@wshlavacek
Copy link
Copy Markdown
Author

Suggested plan to get this draft PR back to mergeable shape later, with the smallest useful path first:

  1. Fix Python command launching first.
  • Refactor the Python-resolution path so we preserve the executable and its arguments separately instead of flattening execCommand into one string.
  • Then run a quick manual smoke test of setup, upgrade, run, and visualize using a Python environment selected by the VS Code Python extension.
  • This is the highest-priority item because it can affect the extension's core functionality for end users.
  1. Make the validation story honest and repeatable.
  • Decide whether we really want lint and the two test suites to be required for this project.
  • If yes: add the missing lint dependency/config and wire CI to run lint, test:grammar, and whatever form of npm test is stable in CI.
  • If not: remove or soften the claims in the PR body so the stated validation matches what we actually run.
  1. Investigate the VS Code integration test harness separately.
  • npm test did not fully validate from my environment because the Electron test host exited with bad option launch errors.
  • Treat that as a separate task from the core feature review: confirm whether the issue is macOS-specific, @vscode/test-electron usage drift, launch-arg drift, or a local environment quirk.
  • If needed, land grammar/unit-level validation first and follow up on full integration-test hardening in a smaller PR.
  1. Reduce review risk before merge.
  • Because this PR is very large, consider either:
    • splitting out obvious low-risk pieces (docs/theme/assets/test-infra), or
    • doing one focused maintainer review pass on only the user-critical behavior: Python execution, plotting, visualization, and language-server basics.
  • Even if we keep this as one PR, having an explicit review scope will make it much easier to finish.
  1. Exit criteria for taking this out of draft.
  • Python launch path is fixed and manually smoke-tested.
  • Validation expectations are aligned with CI and/or the PR description.
  • At least one maintainer has done a targeted review of the user-facing workflows.
  • Then update the PR description with the final verified test story and mark ready for review again.

My recommendation for future us: do not start by rereading the whole PR. Start with the Python-launch fix and the validation/CI alignment; those two items will tell us quickly whether the rest of the rewrite is close enough to finish or better handled in smaller pieces.

@wshlavacek wshlavacek marked this pull request as ready for review May 15, 2026 19:07
@wshlavacek
Copy link
Copy Markdown
Author

Follow-up on reviewer feedback: @akutuva21's standalone Contact Map concerns were addressed with higher-contrast day/night palettes and label contrast fixes, centered regulatory labels, improved non-overlapping layout behavior, persisted/lockable layout selection, and component/internal-state visibility toggles (with molecule-level promoted edges when components are hidden). @efm46's setup issue was addressed by updating bng.setup / bng.upgrade to pin setuptools<82 and repair environments where pkg_resources is missing after newer setuptools installs.

wshlavacek and others added 25 commits May 28, 2026 17:02
ProcessManagerProvider.refresh() armed a self-rescheduling setTimeout
that re-armed unconditionally, with no stored handle and no way to
cancel it, so the 500ms poll kept firing for the life of the extension
host -- including after deactivate(). The provider was also created
inline and never registered for disposal (disposing the wrapping
TreeView does not dispose its data provider).

Make ProcessManagerProvider implement vscode.Disposable: store the
timer handle, add a _disposed guard so an in-flight async callback
cannot re-arm after teardown, and clear the timer and tree-data emitter
in dispose(). Register the provider in context.subscriptions.

Addresses PR RuleWorld#29 review finding #1.
deactivate() fired bng.process_cleanup but dropped the returned
thenable, so the extension host could tear down before
killAllProcesses() finished, orphaning perl/NFsim/run_network
children. Return the thenable so the host waits for cleanup to finish.

Addresses PR RuleWorld#29 review finding #2.
_save_image and _save_graphml_export built the destination path by
joining a webview-controlled title into the results folder; because
Uri.joinPath normalizes "..", a crafted title could escape the folder
and write anywhere. Add resolveSafeOutputUri(), which rejects filenames
that are not a single path segment (separators, ".", "..", absolute
paths) and confirms the resolved path stays under the target folder,
then route all three save paths through it. Also guard the SVG
decodeURIComponent against malformed input.

Addresses PR RuleWorld#29 review finding RuleWorld#3.
The plot sidebar var-item was assembled with innerHTML interpolating
observable names taken from the data-file header (untrusted) -- the only
such untrusted-data innerHTML in the webview. Rebuild the row with
createElement and set the name via textContent, matching the data-table
code path. CSP already blocked inline script execution, so this is
defense-in-depth.

Addresses PR RuleWorld#29 review finding RuleWorld#4.
The standalone operation-only RuleViz path was superseded a day later by
the unified RuleViz browser (pattern/operation toggle), leaving
bng.run_ruleviz_operation orphaned: the command was never declared in
package.json, createRulevizOperationHandler delegated to the full
RuleViz handler, and the 'ruleviz_operation' VisualizationType
discriminator was unreachable. Drop the command, the alias, the
discriminator and its dead branches, and the test-only input builder.
The live 'ruleviz_operation' usages (BNGL visualize action strings and
GraphML file classification) are unchanged.

Addresses PR RuleWorld#29 review finding RuleWorld#5.
checkUnusedParameters built its reference corpus from rule, function,
seed-species, action, and compartment text plus the energy half of
energy patterns, but omitted observable patterns and the pattern half of
energy patterns. Because parseEnergyPatternLine splits the energy off at
the last whitespace, a multi-token energy expression (e.g. "Gbond * 0.5")
leaves the parameter in the pattern half, so a genuinely used parameter
was falsely flagged "never referenced". Add both pattern sources to the
corpus.

Addresses PR RuleWorld#29 review finding RuleWorld#6.
The transpose took its column count from the first data row, so ragged
rows produced undefined cells (violating the string[][] return type) or
silently dropped columns, and a header/data width mismatch misaligned
series names against series data. Drive the column count from the header
names instead: short rows get '' for missing cells and extra unnamed
columns are dropped, keeping names and data aligned.

Addresses PR RuleWorld#29 review finding RuleWorld#8.
The Windows Get-WmiObject path passes '|' and a comma-joined property
list as discrete argv under {shell} and parses Select-Object's
width-formatted table output by whitespace split -- both brittle and
unverified on Windows. Add a NOTE pointing to the upstream tracking
issue instead of rewriting it blind.

Refs RuleWorld#31
onReferences stripped comments with indexOf('#'), which misfires on a '#'
inside a quoted string and could hide a real reference. Reuse the parser's
string-aware stripComment (now exported) to compute the comment boundary.
Also document two existing heuristics: the last-token rate split in
parseRuleLine, and getEnclosingBlockType's reliance on doc.blocks being
ordered by close time so the innermost nested block is matched first.

Addresses PR RuleWorld#29 review finding RuleWorld#9.
Fix the double-indented registerCommand lines in activate(), and remove
the dead cancellation pre-check in onSpawn -- onSpawn runs synchronously
during cp.spawn, before the withProgress cancellation token exists, so
that branch is unreachable; cancellation is handled by the token handler.
Also document why the run-folder timestamp stays second-resolution (the
retention matcher in resultsFolders.ts is anchored to that exact format).

Addresses PR RuleWorld#29 review finding RuleWorld#10.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant