Repository navigation
feat: add Atompack VS Code viewer with native mmap reader - #50
Conversation
Read-only access through a small ReadAt trait so the reader can run where there is no file handle or mmap (WASM, remote bytes). Header, index and extension decoding are shared with AtomDatabase.
Thin wasm32-unknown-unknown build of AtomReader with a raw ABI (no wasm-bindgen): the host supplies read_at, and each export leaves a JSON reply for overview, records, molecule, groups and group columns.
Custom read-only editor for .atp files. The extension host reads the file in ranges through the WASM reader and serves a Svelte + MatterViz webview: overview, paged records with 3D viewer and details, grouping browser with property histograms/filters and role-labelled members, and a synced-camera compare grid.
Histogram and binned scatter of per-record numeric columns (properties, n_atoms, energy, fmax). The file is scanned in chunks on up to 4 worker threads (at most half the cores), sent to the webview as Float32 arrays, with several requests in flight to hide remote round trips. Files past 5M records are sampled in whole chunks spread over the file, and column memory in the webview is capped.
- Plots default to energy per atom, fit axes to the 0.1-99.9th percentiles with an outliers toggle, and list the records in view with an Open in Records action. - Records can be filtered and sorted from the scan, and go-to accepts a formula. - Step records with arrow and vim keys; shrink force arrows and allow hiding them.
Records (when filtered or sorted) and Compare get a copy # button that puts the shown records' numbers, in order, on the clipboard.
Ramlaoui
left a comment
Ramlaoui
left a comment
There was a problem hiding this comment.
Reviewed commit bcc83bc66c757fdd17c9c8ee41807dd62df7154c against main at 6a3e5478cc92da49b678f398cca7a58e4ace7879. I recommend addressing the four correctness findings below before merging. Inline comments include triggers, evidence, and suggested fixes.
Why this changes the core Rust library
The WASM host supplies byte ranges rather than an mmap/native file handle, so the new ReadAt/AtomReader API has a clear purpose. Header selection, schema/extension loading, and index-entry decoding are shared with AtomDatabase to avoid implementing the file format twice. The diff does not introduce a new on-disk format. However, it does change the existing database-opening path and expose a new native reader, so these changes need core-library scrutiny as well as viewer tests. The shared-cursor race below is in that new native reader; it is not just an extension UI issue.
Standards
- Undocumented public Rust API — explicit standard violation. The new
ReadAttrait methods and mostAtomReaderpublic methods lack rustdoc;atompack-wasm::overviewis also undocumented. Contributing guidelines require public Rust APIs to have///documentation. Document the source contract, range/error behavior, and snapshot/read semantics. - Missing extension installation/use docs — documented expectation, judgment call. There is no extension README or user documentation for building, installing, opening files, or understanding sampled filtering/plots. The contributing guide says user-facing changes should include documentation. Include the WASM toolchain prerequisite and the sampling limits.
Spec
No linked issue or independent PRD was available; this axis uses the PR description and feature promises in its commits.
- [P1] Core positioned reads race on a shared file cursor.
Arc<AtomReader<File>>is shareable and its reads accept&self, but its file implementation performs separateseekandread_exactoperations. Eight concurrent readers returned 11,507 wrong counts out of 16,000 calls on a valid 32-record database. Use actual positioned I/O and add a concurrency regression test. - [P2] “Open in Records” changes the plotted population. Automatic ranges lose finite/log-positive constraints, and explicit bounds are rounded. Reproduced scatter selection
[0,2]becoming[0,1,2], log selection[2]becoming[0,1,2], and a visible boundary record disappearing. - [P2] Comparing repeated records across group roles crashes the keyed list. A valid group with record 0 in both reactant and product roles becomes
[0,0], loses one role label, and violates the Svelte keyed-list uniqueness requirement. - [P2] Synchronized cameras omit orthographic zoom. The installed MatterViz default is orthographic and emits
camera_zoom; the handler forwards only position and target. Wheel zoom therefore remains local despite “sync cameras”.
The existing checks pass, but the targeted native-reader and extracted-webview-function reproductions expose the issues above. The duplicate-member fixture was also read through the actual WASM bridge; the zoom finding follows the installed MatterViz camera implementation. Interactive VS Code behavior was not manually exercised in this session.
Totals: Standards 2; Spec/correctness 4. Highest priority: the core reader can silently return the wrong data during concurrent reads.
| export function set_compare(entries: { record: number; label?: string }[]) { | ||
| compare.records = entries.slice(0, MAX_COMPARE).map((e) => e.record) | ||
| compare.labels = Object.fromEntries(entries.filter((e) => e.label).map((e) => [e.record, e.label!])) |
There was a problem hiding this comment.
[P2] Preserve membership identity when a group repeats a record
The storage format permits one record to occupy multiple roles in a group. A valid group with reactant: 0 and product: 0 produces compare.records = [0,0] here and overwrites the reactant label. Compare.svelte keys its viewer loop by record number, so opening this group in Compare triggers Svelte's duplicate-key exception (the installed runtime checks this in production too). I verified the duplicate-role fixture through the actual WASM reader and reproduced this state with set_compare. Please either represent comparison entries with separate membership identities, or deduplicate explicitly with a deliberate policy for retaining role labels.
There was a problem hiding this comment.
Explicitly deferred at the user’s request to #52, with a README limitation and workaround. This thread remains open because repeated group membership is not fixed in this PR.
| on_camera_move={camera | ||
| ? (e) => { | ||
| camera.position = e.camera_position | ||
| camera.target = e.camera_target |
There was a problem hiding this comment.
[P2] Include orthographic zoom in the shared camera state
The locked MatterViz version defaults to orthographic projection. For that projection, wheel zoom changes camera.zoom rather than camera position or target, and MatterViz emits that value as camera_zoom. This handler discards it, and scene_props() only reapplies position/target, so zooming one comparison or group pane leaves the other panes at their previous scale even when sync cameras is checked. Please carry and apply the emitted zoom value along with position and target, or explicitly use a projection whose zoom is fully represented by the shared pose.
There was a problem hiding this comment.
Explicitly deferred at the user’s request to #53, with a README limitation. This thread remains open because orthographic zoom synchronization is not fixed in this PR.
Ramlaoui
changed the title
Ramlaoui
left a comment
Ramlaoui
left a comment
There was a problem hiding this comment.
Refreshed review of commit 3642c793c9d9384909f7e5a4342472f828e9d700 against main at 6a3e5478cc92da49b678f398cca7a58e4ace7879. The previous review covered bcc83bc. The core-library concerns are resolved, but moving to a native binding introduces two new ways to crash the VS Code extension host (inline). The three earlier UI findings are still open.
What changed since the last review
The WASM bridge and the ReadAt/AtomReader range reader are gone. The extension now loads a Node-API binding (atompack-node) around the existing AtomDatabase::open_mmap, and plot scans run in worker_threads, each with its own reader. atompack/ and atompack-py/ are byte-identical to main, so this PR no longer changes the core library. All checks pass, including the five VSIX platform jobs.
Status of previous findings
| Finding | At 3642c79 |
|
|---|---|---|
| Spec P1 | Positioned reads race on a shared file cursor | ✅ Resolved. The range reader was removed. |
| Standards 1 | Undocumented public Rust API | ✅ Resolved for core (no core changes). Minor leftover: NativeReader, its methods and view::overview have no ///. The crate is publish = false, so this is low priority. |
| Standards 2 | No extension install/usage docs | ✅ Resolved. atompack-vscode/README.md covers install, supported platforms, remote hosts and building. The Plots tab already labels partial scans as samples. |
| Spec P2 | "Open in Records" changes the plotted population | ❌ Open. Plots.svelte open_records is unchanged. |
| Spec P2 | Repeated record across group roles crashes Compare | ❌ Open. set_compare is unchanged. |
| Spec P2 | Synced cameras omit orthographic zoom | ❌ Open. Viewer.svelte still forwards only position and target. |
New findings (introduced by the native migration)
- [P2] Rust panics abort the extension host. napi-rs catches panics only when a method has
#[napi(catch_unwind)]. A corrupt index offset makes core slice the mmap out of range, and the process aborts (exit 134). Addingcatch_unwindturns this into a normal JSError; I verified that locally. - [P2] Overwriting an open database crashes the extension host with SIGBUS.
Database(path, overwrite=True)truncates the same inode that the viewer still has mapped, so the next read getsBus error(exit 138).
Both matter more here than in the Python package. The binding runs inside the shared extension host (main thread and scan workers), so one bad or rewritten file takes down every extension in the window, not just this viewer. The earlier WASM design surfaced these faults as catchable JS errors.
Nits
webview/lib/rpc.ts:1still says the host "answers from the WASM reader"..github/workflows/vscode.ymlruns five platform builds on every PR, including Python- or docs-only ones. Apaths:filter (atompack/**,atompack-node/**,atompack-vscode/**,Cargo.*, the workflow file) would save runner time.
Verification: I built atompack-node locally and loaded it directly from Node. I reproduced both crashes against a corrupted and a truncated copy of test/fixtures/groups.atp, and confirmed the catch_unwind fix with a temporary local patch. I did not exercise interactive VS Code behavior.
Totals: 2 new P2, 3 open P2 carried over, 2 nits. Before merging, I recommend adding catch_unwind (a one-line change per method) and choosing a mitigation for the mmap/overwrite crash. The three UI findings also still need fixes or explicit deferral.
| impl NativeReader { | ||
| #[napi(constructor)] | ||
| pub fn new(path: String) -> napi::Result<Self> { | ||
| let database = AtomDatabase::open_mmap(path) |
There was a problem hiding this comment.
[P2] Overwriting an open database crashes the extension host (SIGBUS)
open_mmap maps the whole file. The mapping lives as long as the viewer tab, which is kept alive while hidden by retainContextWhenHidden, plus up to 10 s in idle scan workers. atompack.Database(path, overwrite=True) recreates the file with File::create (atompack/src/storage/mod.rs:216), which truncates the same inode in place. The next read through the stale mapping touches pages past EOF and gets SIGBUS, which JS can't catch. Regenerating a dataset while its tab is open, even in the background, is an ordinary workflow, and the next click or scan chunk would take down every extension in the window.
Repro: open a NativeReader on a copy of the fixture, run fs.truncateSync(path, 0), then call reader.molecule(5) → Bus error: 10 (exit 138).
The README's "treat open databases as immutable" documents the constraint, but the failure is much worse than a stale view. Possible mitigations:
- Use
AtomDatabase::open(non-mmap) for the viewer. It keeps the index in memory and reads each record withFile::open+read_exact, so a truncated file gives an I/O error instead of a signal. The costs are index memory (about 20 B per record) and no zero-copy reads. - Or run the native reader in a separate process (
child_process.fork/ a utility process) instead of the extension host, so any native fault, including the panic above, only kills the viewer and can be reported or restarted.
There was a problem hiding this comment.
Explicitly deferred at the user’s request to #51. The README now states the extension-host crash risk and the workaround of closing all viewer tabs before rewriting the database. This requires a follow-up design change (non-mmap reads or process isolation); catch_unwind does not address SIGBUS.
|
The two required fixes are in Manual testing is now complete using the CI-built
The PR description now reflects the fixes, deferred issues, completed manual check, and demo assets. Core Rust/Python libraries remain unchanged. All checks are now green on |
|
Release CI uses Microsoft Entra workload identity federation. All five native build/test/package jobs and the release-version check passed for |
A record that fills several roles in the selected groups appeared once per role in Compare, which duplicated Svelte's keyed pane and broke the view. Entries now merge per record, joining their role labels, and the cap counts distinct records. Closes #52
Synchronized panes now render in perspective, where zoom moves the camera and so is shared. The shared pose is relative to each structure's fitted frame, so a slab and a gas molecule stay framed alike, and Reset view re-frames every pane. Closes #53
A memory-mapped file truncated while open raises SIGBUS, which no JavaScript handler can catch, so it took down the whole extension host. Each document now reads through its own forked reader process. If that process dies, pending requests fail with a hint to reopen the file, and the next request starts a fresh reader. The reader itself only opens paths; the client writes a private temp copy for remote bytes. Closes #51
Windows refuses to truncate a memory-mapped file, so the SIGBUS case cannot occur there.
MatterViz's dropped-file parser (a 5 MB worker and 5 MB of h5wasm) and three's Draco and KTX2 glTF decoders were bundled although the viewers never load them. Stubbing them shrinks the packaged extension from 4.5 MB to 1.7 MB (17 MB to 5 MB installed), which speeds up installs on remote hosts.
A busy scan worker could post its result after dispose had terminated it, so a chunk requested before closing sometimes resolved. Dispose now rejects in-flight chunks at once and ignores late results.
The Marketplace listing and VS Code's extension page showed only the Compare screenshot. Add the Records, Groups, and Plots screenshots with a short guide to each tab, and note the payloads the webview build leaves out.
The four retina PNG captures totalled 4.1 MB. At 1600 px wide as WebP they total 405 KB with no visible loss, so the README, Marketplace listing, and walkthrough load faster.
1 participant
Opening an
.atpfile in VS Code launches a read-only Atompack viewer with paginated records, interactive 3D structures, group browsing, side-by-side comparisons, and linked plots. The approved Atompack icon appears in the extension listing and editor tabs.The extension uses a small Node-API 8 binding around the existing
AtomDatabase::open_mmapreader. The coreatompack/Rust library andatompack-py/are unchanged relative to main. Local files use mmap, column scans run in Node worker threads, and virtual filesystem resources use a private temporary copy removed on close or failed open.Distribution
Review fixes
#[napi(catch_unwind)]. A corrupt-offset regression test verifies JavaScript errors and continued valid reads in the same process.Deferred limitations are documented and tracked: backing-file overwrite/truncation can crash the mmap reader (#51); repeated record roles can crash Compare (#52); orthographic zoom is not synchronized (#53).
Demo and documentation
Adds an ASE-generated, 845-record catalysis demo with adsorption, site comparison, metal series, and ordered relaxation groups. Energies and forces are explicitly synthetic. Four real VS Code screenshots illustrate filtering, role-based browsing, six-metal comparison, and linked scatter plots. The dataset, generator, capture provenance, README hero, and Sphinx walkthrough are included.
Validation
Workflow lint and release-gating checks passed. Marketplace publication has not been exercised.
All five native build/test/package jobs and the release-version check passed for commit
d2eac77in CI run 36708218712, alongside Rust, Python, and Read the Docs checks. The viewer walkthrough preview is live.npm run check: zero errors or warnings. All 15 reader/worker/query tests passed with the optional 1,000-record smoke database.Rust formatting passed. Local Clippy passed with
-D warnings -A unused-variablesfor main's existing macOSpopulatewarning; Linux CI retains strict Clippy.Manual VS Code smoke check completed with the CI-built macOS ARM64 VSIX: opened the six-record fixture and exercised Overview, Records, Groups, Compare, and Plots. Logarithmic
fmaxproduced the same five records in Plots and Records. The larger demo's zoomed scatter produced the same 584 records after Open in Records, with unrounded bounds and matching sort order.Validated all 845 demo structures, group memberships, capture selections, and four rendered figures. Sphinx HTML builds successfully; strict warning mode exposes only five pre-existing missing benchmark SVGs in
performance.rst, with no warnings from the new viewer page.Follow-up fixes (0.1.1)
Closes #51, closes #52, closes #53.
src/client.ts,src/reader-process.ts). A file truncated while open now fails the pending requests with a "reopen" hint instead of crashing the extension host with SIGBUS. The next request starts a fresh reader. Verified that the Electron plugin-helper child loads the native addon on macOS.Ramlaoui.atompack-vscode). The extension is bumped to 0.1.1, so taggingatompack-vscode-v0.1.1after merge publishes these fixes.npm run checkclean;npm test16 passed / 2 skipped, including a truncation regression test. Zoom/rotation sync and Reset view were checked in a headless browser on the grouped demo.