Folder/ZIP bundle grouping, multi-model preview, and Send to Slicer - #65
Folder/ZIP bundle grouping, multi-model preview, and Send to Slicer#65thrtnastrx wants to merge 1 commit into
Conversation
…ration. Group related STLs by folder or zip archive, preview all bundle parts in one 3D layout, send models to configured slicers from preview (new instance on macOS), and document behavior in CHANGELOG, GUIDE, and README. Co-authored-by: Cursor <cursoragent@cursor.com>
Conflict-resolved integration of folder/ZIP bundle grouping from #65 into main 2.1.7.
|
Thanks for this contribution! #65 could not be merged as-is ( Conflict-resolved merge landed via #69 (
Closing this PR as completed via #69. |
|
I'll take a look. This PR (#65) is closed and was superseded by #69 ( Let me start by examining the repository to understand what actually merged. |
|
The checkout is the PR #65 head ( |
|
Now let me actually run this logic against a range of real-world inputs to find bugs. There's a subtle one I want to verify: |
|
Confirmed a real bug (and note the extra escaping is just my shell — the actual input is a normal Windows path
Let me verify this is really how it behaves and check the zip-label edge case too. |
|
The shipped tests pass but they only test a POSIX folder path — they never cover a Windows folder path, which is exactly the broken case. Let me check how |
|
Confirmed. The scan builds The real, unambiguous bug is different: the zip |
|
There's the real, concrete bug, now clearly demonstrated. Even under the Windows Let me write a small reproduction test that pins the bug down deterministically regardless of host platform, then post the review. |
|
Important correction to my own analysis: on Windows (where Now — does that POSIX-vs-Windows-path scenario actually occur in this app? Yes, in one specific place: |
|
So it's only used in |
|
Confirmed the shape of it. The migration/backfill runs in That's a legitimate, defensible finding. The clean fix is to I have enough to post a well-grounded review. Let me write it up. |
There was a problem hiding this comment.
This PR is closed (superseded by #69), so this is a review of the code as it stands on feature/bundle-grouping — bundle-keys.js is the shared heuristic that scanning, backfill, preview, and Send-to-Slicer all depend on, so I focused there and ran the actual deriveBundleFromFilePath logic against a spread of real inputs.
What I checked, by running it (node bundle-keys.test.js + ad-hoc cases):
- ✅ POSIX folder grouping, zip-entry grouping, and
url::exclusion all behave as the shipped tests claim. Those three pass. - ✅ On the Windows runtime (
path.win32), folder grouping and labels are correct — so this is not a bug on a normal Windows install. ⚠️ One real cross-platform bug (inline): the folder branch callspath.dirname()on the raw, un-normalized path, while the zip branch normalizes separators first. When this runs on POSIX (path.posix) against Windows-style backslash paths — which happens in the Linux/Docker/server-bridgeruntime this repo ships, backfilling a DB whosefilePaths use\—path.dirname()returns.and the folder bundle comes back empty, so multi-part folders silently don't group. The shipped tests miss it because they only cover a forward-slash folder path.
Verdict: the feature logic is sound on the primary Windows target; the folder branch just needs to normalize separators before deriving the directory (mirroring what the zip branch already does) to be platform-independent. Not a merge blocker for Windows users, worth fixing for the Linux/server path. Everything else in the diff I could reason about (migration DDL, scan insert/update binding the three new columns, getAllModels/filtered SELECT column list) looks internally consistent.
Note: this is a desktop Electron app with no drivable URL, so I verified the pure bundle-keys module directly rather than driving the GUI.
| }; | ||
| } | ||
|
|
||
| const dir = path.dirname(filePath); |
There was a problem hiding this comment.
🎥 Observed in a test run
Folder grouping breaks on POSIX for Windows-style paths — separators aren't normalized before path.dirname.
The zip branch above normalizes first (normalizePath(zipPath) on line 22, then derives), but the folder branch calls path.dirname(filePath) on the raw path and only normalizes the result. When path resolves to path.posix (the Linux/Docker/server-bridge runtime this repo ships), path.dirname doesn't treat \ as a separator.
I ran the shipped function under path.posix with a Windows-style path:
derive('C:\\Models\\project\\part1.stl')
// => { bundleKey: '', bundleLabel: '', bundleKind: '' }
derive('C:\\Models\\project\\part2.stl')
// => { bundleKey: '', bundleLabel: '', bundleKind: '' }
path.dirname returns ".", so the === '.' guard on line 34 returns empty and the two siblings never group. This bites migrateBundleColumns() backfill (main.js:1811) and scan insert/update (main.js:2940) whenever the process runs on Linux against a DB populated with Windows paths — exactly the cross-platform / Docker scenario this repo supports. On a native Windows install (path.win32) it works, which is why the bug is easy to miss.
The shipped tests pass but only cover a forward-slash folder path, so they never exercise this. Fix: normalize first, then derive, e.g.
const normalized = normalizePath(filePath);
const normalizedDir = path.posix.dirname(normalized);so folder behavior is separator-independent, matching the zip branch.
|
Review posted. Since this is an Electron desktop app with no drivable GUI, I reviewed by running the actual SummaryI reviewed PR #65 (closed, superseded by #69). What passed (verified by running):
One real bug (observed, posted inline):
The rest of the diff I could reason about (migration DDL, the scan insert/update binding the three new columns, the SELECT column lists) is internally consistent. I labeled the finding |
Summary
This PR improves how multi-part projects (folders and ZIP archives) appear in the library and how they flow into preview and slicing.
open -n; Prusa-family binaries get--single-instance=0so sends work when a slicer is already open.bundleKey,bundleLabel,bundleKindonmodelswith startup migration/backfill.npm run test:bundleforbundle-keys.js.Documentation
guide.js) — bundle and slicer bulletsTest plan
npm run test:bundlepassesNotes for maintainer
zipPath::entryformat; slicer send extracts via existingextractModelFromZip.Made with Cursor