Merge PR #65: bundle grouping, multi-preview, Send to Slicer

[TechJeeper]

Summary

Conflict-resolved merge of #65 into current main (2.1.7).

• Bundle grouping for folder + ZIP multi-file projects (supersedes ZIP-only display grouping; parent-model grouping still nests on top)
• Multi-model 3D preview + bundle details panel
• Send to Slicer from preview (multi-slicer, new instance on macOS)
• DB bundleKey / bundleLabel / bundleKind with migration
• Kept existing rating/favorite, New-badge helpers, and 3MF simplified-preview notes from main

Test plan

• npm run test:bundle / node bundle-keys.test.js
• Scan multi-file folders and ZIPs → bundle rows
• Click bundle → combined preview; double-click → details
• Send to Slicer from preview
• Rating/favorite and New badge still work

Closes #65 once merged.

[testdriverai]

I'll review this PR by reading the code carefully. Let me start by examining the key files, especially the parts of the diff that were truncated.

[testdriverai]

Now let me look at the key logic changes. Let me start with main.js (the largest logic change) and the truncated parts.

[testdriverai]

I notice const selectCols at column 0 (no indentation) in a couple of these — that's just whitespace within a function, not a functional issue. Let me check whether that broke indentation is actually a syntax problem — no, const selectCols inside a function body is fine regardless of indentation. Let me look more carefully at the escapeShellArg function, which is a real concern (shell injection / correctness), and the rest of saveModel.

[testdriverai]

Now let me examine the escapeShellArg and buildSlicerLaunchCommand more carefully. On Linux/Windows non-app path, it uses exec with a shell command. Let me check how paths are escaped and whether Prusa --single-instance=0 placement is a problem. Let me look at the preview.js Send to Slicer flow and renderer.js bundle grouping logic, which is where most behavioral bugs live.

[testdriverai]

There's an important detail here. Let me note a discrepancy: the main.js deriveBundleFromFilePath groups folders even when there's only ONE parent level (it just requires dir isn't . or /), whereas the renderer's deriveBundleFieldsForModel requires parts.length >= 2. But more importantly, both produce a bundleKey for every model in a folder — but a bundle should require 2+ files. Let me check how buildGroupedDisplayRecords handles single-item groups (whether it collapses them back to individual rows).

[testdriverai]

Good — single-item groups collapse back to individual models (children.length <= 1), so folder single-file grouping is handled. That matches the docs.

Now I have a key observation. The bundle grouping runs FIRST, then parent-model grouping runs on the result. When bundle grouping produces a group record, the second pass (buildGroupedDisplayRecords for parentModel) checks record.type !== 'model' and pushes group records through unchanged — good.

Now let me focus on the most likely real bug: the escapeShellArg on Windows. buildSlicerLaunchCommand produces a shell string passed to exec. On Windows, exec runs via cmd.exe, where \" is NOT the escape for a double-quote and backslash-escaping doesn't apply. But this is pre-existing behavior pattern (they already used "${slicerPath}"). Let me check the more concrete concern: --single-instance=0 is appended AFTER the slicer path but the model paths come after that — that's fine (flag then files).

Let me look at the preview.js Send to Slicer and openBundlePreview which is brand new and large.

[testdriverai]

Now let me see the rest of handlePreviewSendToSlicer — the slicer picker flow, which is the key logic.

[testdriverai]

I've got a solid understanding. Now let me verify a couple of concrete concerns:

1. getPreviewSlicerFilePaths for a single-model preview of a ZIP entry: it returns [currentFilePath] if currentFilePath doesn't start with url::. For a zip entry like foo.zip::part.stl, isPreviewableModelPath isn't checked here (only for bundles). That's fine — main resolves it.

2. Let me verify the slicer.js window.openSlicerSettings export and preload.js openFileInSlicer exposure actually exist (referenced by preview.js). If missing, Send to Slicer breaks entirely.

[testdriverai]

Those wirings are all present. Now let me verify a real concern: the item.addEventListener('click') on bundle groups opens the preview for BOTH bundle AND zip groupKind — but the zip-only grouping was replaced by bundle grouping (getZipArchiveGroupLabel is no longer used in buildParentModelDisplayRecords). So zip groupKind records may no longer be produced. Let me check whether zip groups can still exist elsewhere. Not a bug though.

Let me now focus on verifying my strongest candidate bugs. Let me check the renderer.js executeClientCommand path — the escapeSlicerShellArg is defined inside DOMContentLoaded but buildSlicerLaunchCommand (the one calling it) is defined at line ~10858 also inside DOMContentLoaded. But the exec call at ~10820 references buildSlicerLaunchCommand — let me check ordering (function hoisting).

[testdriverai]

Function declarations are hoisted within the same function scope, so buildSlicerLaunchCommand is callable at 10829 even though defined at 10878. That's fine.

Now the strongest real concern I've spotted. In the renderer client path (line ~10817), when there are multiple filePaths that include ZIP entries, modelPaths = rawPaths.filter(Boolean) — the zip entries are passed to the slicer raw (foo.zip::part.stl), which the slicer can't open. But only if rawPaths.length === 1 does it show the extraction message. For a multi-path bundle with zip entries, it passes unextracted zip paths. However, this is the web/server-client fallback where hasNodeAccess extraction isn't available anyway — a pre-existing limitation.

Let me look at the strongest, cleanest candidate: getBundleContainerPath / folder derivation mismatch between main and renderer. Main groups a folder even at the root's immediate child (dir isn't .//), but renderer requires parts.length >= 2. Let me check whether this creates inconsistency where DB has a bundleKey but renderer would return empty (or vice versa), causing double-derivation. Actually renderer prefers model.bundleKey from DB if present. So DB drives it. Fine.

Let me now check one concrete production-hurting bug candidate: the folder bundleKey uses lowercased normalized dir in both — consistent.

Let me check the styles.css and index.html for the bundle-details panel — is it inside a container that's already shown, or floating? Let me verify the preview-send-to-slicer button interplay: a single STL preview where currentFilePath is a plain path works. Let me double check one subtle bug: getPreviewSlicerFilePaths when previewing a single ZIP entry model that is NOT previewable extension — not relevant.

Let me verify the biggest candidate carefully: the Prusa --single-instance=0 flag ordering with multiple files and the macOS open -n with multiple --args. Let me check how the slicer receives multiple files. Actually open -n -a App --args file1 file2 passes both as args — fine for slicers that accept multiple.

Let me look at whether centerAndScaleModel/cleanupPreviewScene properly dispose the bundle root group's cloned materials (memory), and check hasColorData. That's minor. Let me instead confirm a regression: was getZipArchiveGroupLabel/Key used anywhere else that now breaks, and is the zip-only display grouping fully removed such that existing zip-expanded state is orphaned? Minor.

Let me verify my top concern about shell injection / spaces in paths on exec is actually a behavior change vs pre-existing. It's pre-existing pattern. Not a regression.

Let me look carefully at one thing that could be a genuine bug — the applyPartTint clones materials but bundle cleanup. Let me check cleanupPreviewScene.

[testdriverai]

disposeObject3D traverses recursively, so the bundle root group's children get disposed. Fine.

Now, let me check the migration's transaction usage — db.transaction(...) wrapping backfill. Let me verify migrateBundleColumns runs at startup during initializeDatabase, and whether the backfill query WHERE bundleKey IS NULL OR bundleKey = '' could be a performance issue on huge DBs but that's not a bug.

Let me examine one genuine correctness concern I want to confirm: In main.js deriveBundleFromFilePath, for a Windows path like C:\Models\part.stl, path.dirname on a Linux/macOS runtime (path.posix behavior depends on platform) — Electron main uses the OS's path. On the actual user's OS the path separators match, so path.dirname works. But the ZIP branch uses path.basename(normalized) where normalized uses forward slashes — path.basename on Windows handles both, on posix only /. Since they normalize to / first, path.basename works cross... actually on posix path.basename('c:/models/flower.zip') → flower.zip. Good. The test confirms Windows zip path works.

Now let me verify the one thing most likely to be an actual bug — whether openBundlePreview is triggered for a single-file bundle. Groups only exist with 2+ children, so fine.

Let me check renderer.js around line 20340 for the dblclick handler on bundles (showBundleDetails) to ensure it doesn't conflict with single-click preview.

[testdriverai]

Now I have a comprehensive picture. Let me verify one final genuine concern I want to nail down: the getPreviewSlicerFilePaths() for a bundle filters children by isPreviewableModelPath (STL/3MF only), which is correct. But for the single-model case it returns [currentFilePath] without checking previewable — that's fine since you can only preview STL/3MF anyway.

Let me do a final check on the one thing that stood out as a likely genuine bug: in openBundlePreview, initPreviewScene() is called, but for a normal single openPreview, initPreviewScene is called inside loadPreviewModel? Let me verify the scene isn't double-added / leftover previewModel from a previous open isn't cleaned. openBundlePreview calls initPreviewScene() then adds root. Does it clean up the previous previewModel? Let me check initPreviewScene.

[testdriverai]

initPreviewScene cleans up prior scene. Good.

I've now thoroughly reviewed the diff and surrounding code. Let me do one last targeted check on a genuine bug candidate: the open -n -a on macOS with --args passing multiple files. Actually the real subtle bug: open -n -a AppBundle --args file1 file2 — when the slicer app is Prusa-family, the macOS branch does NOT add --single-instance=0, but open -n handles new-instance. That's intentional and documented. Fine.

Let me verify the one thing that could actually break in production — slicer.id matching. getSlicerBySelection matches slicer.id === slicerId, and preview passes slicerId: slicer.id. But loadConfiguredSlicers legacy fallback returns { id: null, ... }, and sendPreviewToSlicer passes slicerId: null, slicerName: 'Slicer'. In main, getSlicerBySelection sees slicerId == null → falls to slicerName match → but there's no slicer named 'Slicer' in the DB (legacy path setting, not in slicers table). So it returns... slicers[0] only if slicerName is also falsy. slicerName='Slicer' is truthy → slicers.find(name==='Slicer') → none → returns null → throws "No slicer configured".

This is a real bug: when only a legacy slicerPath setting exists (no entries in the slicers table), the preview "Send to Slicer" will fail. Let me confirm openFileInSlicerHandler reads only from the slicers table and doesn't consult the legacy slicerPath setting.

[testdriverai]

Confirmed. Let me trace the exact scenario. loadConfiguredSlicers in preview.js: if getSlicers() returns entries, uses them. If empty, falls back to legacy slicerPath setting → returns [{ id: null, name: 'Slicer', path: legacyPath }]. Then handlePreviewSendToSlicer with 1 slicer calls sendPreviewToSlicer(slicers[0], filePaths) → openFileInSlicer({ filePaths, slicerId: null, slicerName: 'Slicer' }).

In main openFileInSlicerHandler: reads db.SELECT * FROM slicers → empty array. getSlicerBySelection([], {slicerId:null, slicerName:'Slicer'}) → returns null (empty array short-circuits). Throws "No slicer configured. Add a slicer in Settings."

So a user with only a legacy single slicer path configured (the pre-2.x way) gets "No slicer configured" when using the new preview Send-to-Slicer, even though the renderer thought it had a slicer and showed the button as enabled. This is a genuine regression/edge-case bug. Let me confirm getSlicerBySelection returns null for empty array.

Yes — line: if (!Array.isArray(slicers) || slicers.length === 0) return null;. Confirmed.

Let me verify the legacy slicerPath setting actually exists as a real mechanism (not dead code) — the context menu path.

[testdriverai]

The context menu also only uses the slicers table (not the legacy slicerPath setting), so the legacy fallback in preview.js is inconsistent with the rest of the app. Regardless, the concrete bug stands: when preview.js falls back to the legacy slicerPath, openFileInSlicerHandler can never resolve it, so Send to Slicer fails with a misleading "No slicer configured" error for that user. This is suspected (I reasoned it from the code; didn't run it).

Now let me confirm a second, lower-confidence finding about the client-side (server mode) executeClientCommand renderer path passing raw zip entries. Actually that's a pre-existing limitation and the fallback shows a message for single. I'll leave it — not confident it's a regression.

Let me also double-check one thing: the slicers[0] being an array normalization (slicers.length === 1 && Array.isArray(slicers[0])) in loadConfiguredSlicers. That guards against getSlicers returning [[...]]. Odd but defensive; not a bug.

I have one solid suspected finding. Let me look for any others in the truncated main.js scan portion I haven't seen — the scan handler's index/first part.

[testdriverai]

This is all sound. The migration and schema look correct.

I've completed a thorough review. I found one genuine, production-affecting bug (suspected — reasoned from the code, not run, and this turn I can't run anything). The bulk of the change is well-structured and consistent. Let me post the review, pinning the finding to the relevant diff line in main.js.

The finding is about getSlicerBySelection returning null when the slicers table is empty but a legacy path was passed. The best line to pin is in the openFileInSlicerHandler / getSlicerBySelection. Let me get the exact new-file line number for getSlicerBySelection.

[testdriverai]

Reviewed the bundle-grouping / multi-preview / Send-to-Slicer merge by reading the diff and the surrounding code (main.js, preview.js, renderer.js, preload/slicer/server-bridge wiring). I couldn't run the app this turn, so every finding below is reasoned from the code (suspected), not observed.

Overall this is a solid, internally-consistent change:

• The DB migration (migrateBundleColumns) is guarded (PRAGMA check before ALTER, index create, transactional backfill) and correctly wired into initializeDatabase; all the selectCols/insert/update statements were updated in lockstep with the new columns.
• bundle-keys.js derivation matches the renderer's deriveBundleFieldsForModel, and single-file "bundles" correctly collapse back to individual rows in buildGroupedDisplayRecords (children.length <= 1), matching the documented behavior.
• The slicer-launch refactor (buildSlicerLaunchCommand / runSlicerWithModelPaths) is shared cleanly between the context menu and the new preview flow, and the preview Send to Slicer IPC (open-file-in-slicer), preload exposure, and window.openSlicerSettings export are all present.

I found one real edge-case bug — the legacy single-slicer-path fallback in the preview flow can't be resolved by the main-process handler, so those users get a misleading "No slicer configured" error. Details inline.

[testdriverai]

⚠️ Possible issue — from reading the diff; I didn't reproduce this.

Legacy slicerPath fallback in the preview flow is unreachable here, so Send to Slicer fails with a misleading error for users who only have the old single-slicer path configured.

In preview.js, loadConfiguredSlicers() falls back to the legacy setting when the slicers table is empty:

const legacyPath = await window.electron?.getSetting?.('slicerPath');
if (legacyPath) return [{ id: null, name: 'Slicer', path: legacyPath }];

So the preview button is enabled and handlePreviewSendToSlicer calls openFileInSlicer({ filePaths, slicerId: null, slicerName: 'Slicer' }).

But openFileInSlicerHandler only ever looks at the slicers table:

const slicers = db.prepare('SELECT * FROM slicers').all(); // [] for a legacy-only user
const slicer = getSlicerBySelection(slicers, { slicerId, slicerName });
if (!slicer) throw new Error('No slicer configured. Add a slicer in Settings.');

With an empty table, getSlicerBySelection returns null on its first guard (slicers.length === 0), and slicerPath from the legacy fallback is never consulted. Net effect: a user whose slicer is configured the old way sees an enabled Send to Slicer button in the preview, clicks it, and gets "No slicer configured. Add a slicer in Settings." — which is both wrong and confusing.

Either drop the legacy fallback from loadConfiguredSlicers (to match the context menu, which also ignores the legacy path), or have openFileInSlicerHandler honor a passed slicerPath / read the legacy slicerPath setting when the table is empty.

[testdriverai]

Review posted on PR #69.

I read the full diff plus the surrounding code (main.js, preview.js, renderer.js, and the preload/slicer/server-bridge wiring), since the interesting bugs in a change like this live in the code the patch doesn't show — the IPC handler that receives the new payload, and the grouping/collapse logic.

Verdict: The change is well-built and internally consistent. The DB migration is properly guarded and transactional, every selectCols/insert/update statement was updated in lockstep with the new bundleKey/bundleLabel/bundleKind columns, single-file bundles correctly collapse back to individual rows, the shared slicer-launch helper is wired cleanly into both the context menu and the preview, and all the IPC/preload/global exports the new flow depends on are present.

One real finding (suspected, not run): the preview's loadConfiguredSlicers() falls back to the legacy single slicerPath setting ({ id: null, name: 'Slicer', path: legacyPath }) when the slicers table is empty — but openFileInSlicerHandler only reads the slicers table, so getSlicerBySelection returns null and throws "No slicer configured." A user with only the old-style slicer path gets an enabled Send-to-Slicer button that always fails with a misleading error.

I marked it suspected because I reasoned it from the code — this install isn't signed in to TestDriver, so I couldn't start a session and watch it happen. Nothing else in the diff rose above style/noise, so I posted just the one inline comment.

[thrtnastrx]

Thanks for landing this via #69.

Bug: I agree with the folder-branch separator fix

Feature request – Folder bundling control

Right now, every parent folder is treated as a potential bundle. That causes category folders under STL Home (such as art, kitchen, toys, etc.) to group unrelated models together. ZIP files already behave correctly—the issue only appears with the folder structure after extraction.

My suggestion would be:

• Treat all folders as content folders by default (folders containing two or more models become a bundle)
• Allow users to mark specific folders as directory folders. These would be organizational only and would never become bundles
• Make STL Home a directory folder by default (or always exclude it from bundling)
• Subfolders beneath a directory folder would still default to content folders, so an extracted project like art/LampProject/ would still bundle correctly

[testdriverai]

This is a feature request about how folder bundling should behave — designing and implementing "directory folder" vs "content folder" controls is application/product work, which is outside what I do. I'm the TestDriver test agent: I write, fix, and maintain automated tests (and review PRs by running them), so I can't design or build that feature.

What I can do here, once someone implements it, is cover it with tests. For the behavior you're describing, that would be:

• Unit tests for the bundling rule — extend bundle-keys.test.js (or a new spec) so the derivation logic is pinned down: a folder with 2+ models bundles, a folder marked "directory" never bundles, STL Home is excluded by default, and a subfolder beneath a directory folder (e.g. art/LampProject/) still bundles. That's the cheapest place to lock in the rule and catch regressions.
• An end-to-end TestDriver test — scan a fixture tree with category folders (art, kitchen, toys) under STL Home plus one real multi-part project folder, then assert the category folders show as individual rows while the project folder shows as one bundle row. This is exactly the kind of behavior I can drive in the real app and verify visually.

One thing worth flagging for whoever implements it, since it affects testability: the bundle key is currently derived purely from the file path (deriveBundleFromFilePath), with no notion of a "directory folder" flag. To make the exclusion testable and consistent, that flag will need to live somewhere the derivation can see it (a settings list of directory-folder paths, or a DB column), and both the main-process and renderer derivation paths will need to honor it — otherwise the grid and the persisted bundleKey can disagree.

Want me to draft the unit + end-to-end tests now, against the rules as you've described them (folders bundle at 2+; directory folders and STL Home never bundle; subfolders under a directory folder still bundle)? They'd start red and turn green once the feature lands — a ready-made spec for the change. If so, tell me where your test fixtures/sample STL tree live (or I can create a small fixture folder), and I'll put them together.