From 30fe88a33469f48f29a48d7a8487e2ba1d9e4816 Mon Sep 17 00:00:00 2001 From: maziggy Date: Sat, 9 May 2026 08:43:51 +0200 Subject: [PATCH] fix(inventory): show all per-printer/per-nozzle variants in spool form's Slicer Preset dropdown (issue #1248) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in buildFilamentOptions, surfaced together: 1. The function was precedence-based — cloud presets short-circuited the local-presets branch, silently hiding any imported Local Profile while the user was logged into Bambu Cloud. The wiki documents the dropdown as "merged and deduplicated" across cloud + local + built-in. 2. Cloud default presets and local presets were being collapsed by base name (everything after "@" stripped), so all P1S/X1C/A1 variants of "Bambu PLA Basic" rendered as a single row. The spool form is printer-agnostic by design, so the right semantic is to show every variant individually — the union across all printers — not collapse them. AMS Slot is per-printer (it filters), the spool form is union-of-all (it doesn't). Rewrote the merge to push each cloud setting_id and each LocalPreset row as its own FilamentOption with the full @printer suffix preserved in displayName. Built-in dedup against cloud setting_id is kept (mirrors ConfigureAmsSlotModal.tsx). Wired api.getBuiltinFilaments() into both callers. slicer_filament persistence is unchanged so existing spools keep slicing correctly. --- CHANGELOG.md | 1 + .../components/SpoolFormBulk.test.tsx | 1 + .../components/SpoolFormModal.test.tsx | 1 + .../spool-form/buildFilamentOptions.test.ts | 162 ++++++++++++++++ frontend/src/components/SpoolFormModal.tsx | 10 +- .../components/spool-form/FilamentSection.tsx | 2 +- frontend/src/components/spool-form/utils.ts | 162 +++++++++------- .../spoolbuddy/SpoolBuddyWriteTagPage.tsx | 7 +- .../{index-W4CVK7gF.js => index-CdUD30tt.js} | 182 +++++++++--------- static/index.html | 2 +- 10 files changed, 364 insertions(+), 166 deletions(-) create mode 100644 frontend/src/__tests__/components/spool-form/buildFilamentOptions.test.ts rename static/assets/{index-W4CVK7gF.js => index-CdUD30tt.js} (86%) diff --git a/CHANGELOG.md b/CHANGELOG.md index afb55e31d..3d024c11a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ All notable changes to Bambuddy will be documented in this file. ## [0.2.4b4] - Unreleased ### Fixed +- **Spool form's "Slicer Preset" dropdown silently dropped Local Profiles when Bambu Cloud was connected, and collapsed per-printer/per-nozzle variants of cloud and local presets into a single entry** ([#1248](https://github.com/maziggy/bambuddy/issues/1248), reported by @andretietz) — Two distinct defects in the same code path. **Defect 1 (the reported bug):** `buildFilamentOptions` in `frontend/src/components/spool-form/utils.ts` was precedence-based — `if (cloudPresets.length > 0)` returned the cloud list and never reached the local-presets branch, so any Local Profile imported via Profiles → Local Profiles was silently invisible whenever the user was logged into Bambu Cloud (the same profile rendered fine with a green `Local` badge in the AMS Slot configuration modal). The wiki documents the dropdown as "merged and deduplicated" across cloud + local + built-in. **Defect 2 (surfaced during fix verification):** the spool form was collapsing all `@Bambu Lab P1S 0.4 nozzle` / `@Bambu Lab X1C 0.4 nozzle` / `@Bambu Lab A1 0.4 nozzle` variants of "Bambu PLA Basic" into a single dropdown entry by stripping the `@printer` suffix and dedup'ing by base name (one Map.set per family for cloud defaults, one per family for local presets). The AMS Slot modal lists each variant individually and filters by the active printer model, so the user observed strictly more entries in the AMS Slot than in the Add Spool modal even after the merge fix. The right semantic for the spool form — printer-agnostic by design, since a spool isn't bound to a printer — is to show every variant as its own row, exactly as if you'd summed the AMS Slot's per-printer-filtered output across all printers. **Fix:** rewrote `buildFilamentOptions` to (a) actually merge all three sources, dropping the precedence early-return, and (b) push each cloud `setting_id` and each `LocalPreset` row as its own `FilamentOption` instead of collapsing by `name.replace(/@.*$/, '')`. `displayName` now keeps the full `@printer 0.4 nozzle` suffix so users can pick the right variant. Built-in dedup against cloud setting_id is preserved (mirrors `ConfigureAmsSlotModal.tsx:498` exactly). Wired `api.getBuiltinFilaments()` into both callers — `SpoolFormModal` and `SpoolBuddyWriteTagPage`. **Persistence safety:** the saved `slicer_filament` shape is unchanged — cloud picks still persist their `setting_id`, local picks still persist `preset.filament_type || String(preset.id)` (consumed by `backend/app/utils/filament_ids.py::normalize_slicer_filament` which expects `GFL05`/`GFSL05` shapes; persisting the bare LocalPreset row id would break slicing). Local-preset `allCodes` now carries both the `filament_type` form and the `String(preset.id)` form so `findPresetOption` resolves both old (pre-fix) and new picks. **React-key collision:** with collapse removed, two LocalPreset rows can share the same `code` if they share `filament_type`; the dropdown key in `FilamentSection.tsx` is now composed `${option.code}::${option.name}` to stay unique. **Tests:** new `frontend/src/__tests__/components/spool-form/buildFilamentOptions.test.ts` with 9 cases — the #1248 regression case, "one entry per cloud setting_id, no @printer collapse", "list each local preset individually", "@printer suffix preserved in displayName", local `allCodes` carrying both shapes, the `GFA00`↔`GFSA00` built-in dedup, the all-empty fallback, and the alphabetical sort. The two existing `vi.mock('../../api/client')` blocks in `SpoolFormModal.test.tsx` and `SpoolFormBulk.test.tsx` were updated with the new `getBuiltinFilaments` stub. - **SpoolBuddy install.sh re-run failed with `Permission denied` on root-owned files in update mode** — `download_spoolbuddy()` ran `git fetch + git checkout + git reset --hard` *before* the post-install chown at the end of the function. If a previous install left stray root-owned files in the tree (e.g. `static/assets/*` written by an earlier `sudo` run, or a frontend build that wrote as root), the `git reset --hard` step aborted with EACCES on the unlink/replace step before reaching the chown. The script then exited and the kiosk's underlying ownership problem persisted, so the next attempt would fail the same way. **Fix:** pre-emptively `chown -R spoolbuddy:spoolbuddy "$INSTALL_PATH"` in the update branch *before* any git operation runs. The script already runs as root (enforced by `check_root`), so the chown is always safe. The existing post-install chown at the end stays — it now mostly catches new files created during this run that need their ownership normalised. Same root cause showed up on the kiosk's *runtime* SSH update path (Bambuddy → kiosk: `git checkout dev && git reset --hard origin/dev` running as the `spoolbuddy` user) but that path can't `chown` without sudoers expansion — the install.sh fix is the immediate recovery, and re-running the install script restores a clean ownership baseline that the runtime updater can keep healthy thereafter. - **SpoolBuddy SSH update aborted with `TypeError: startswith first arg must be bytes or a tuple of bytes, not str` after the host-key store succeeded** — `perform_ssh_update` calls `asyncssh.import_known_hosts(...)` to materialise an `SSHKnownHosts` object for `_run_ssh_command`'s `known_hosts=` keyword arg. Both call sites (the stored-key path at line 221 and the just-stored TOFU re-parse at line 272) passed `f"{ip} {key}\n".encode()` — i.e. `bytes`. asyncssh's parser does line-based string operations (`line.startswith('#')` with a `str` literal), so any `bytes` input crashes inside its loader with `TypeError`. The two `try`/`except` clauses caught only `(ValueError, asyncssh.Error)`, missing `TypeError`, so the crash bubbled up and aborted the whole update right after the schema fix successfully persisted the host key. **Fix:** drop the `.encode()` at both call sites — pass the str directly. Widened both except clauses to `(ValueError, TypeError, asyncssh.Error)` so any future asyncssh API surprise degrades to the existing fallback (TOFU mode without host-key verification, with a logger.warning) instead of crashing the update. Existing SSH tests all mocked `asyncssh.import_known_hosts` itself so they never reached the parser — added `test_perform_ssh_update_passes_str_not_bytes_to_import_known_hosts` to capture both call sites' arguments and assert `isinstance(arg, str)` so re-introducing `.encode()` fails CI immediately. - **SpoolBuddy SSH update crashed on Postgres with `value too long for type character varying(500)` when storing the device's RSA host key** — `spoolbuddy_devices.ssh_host_key` was declared as `String(500)`, which is fine for SQLite (ignores VARCHAR length) and for ed25519 host keys (~120 chars), but RSA host keys in OpenSSH format are typically 370 chars (2048-bit) → 544 chars (3072-bit) → ~720 chars (4096-bit). Postgres enforces the limit strictly, so any kiosk reporting an RSA-3072 or larger host key on the first SSH update aborted at the `UPDATE spoolbuddy_devices SET ssh_host_key=...` flush — the `git fetch + pip install + systemctl restart` may have run successfully but the persistence of the TOFU host key failed and the device's update_status was never written. **Fix:** widened `ssh_host_key` from `String(500)` → `Text` on the model, plus an idempotent `ALTER TABLE spoolbuddy_devices ALTER COLUMN ssh_host_key TYPE TEXT` migration gated on `not is_sqlite()` (Postgres-only; SQLite is a no-op since it doesn't enforce VARCHAR length). Existing rows are preserved — `TYPE TEXT` is a metadata-only change on Postgres for `VARCHAR(N)` → `TEXT` so it's a fast migration even on populated tables. Originally introduced in the H1 SSH-host-key TOFU security fix; the 500-char floor was a guess based on ed25519 sizes that the RSA case immediately blew past. diff --git a/frontend/src/__tests__/components/SpoolFormBulk.test.tsx b/frontend/src/__tests__/components/SpoolFormBulk.test.tsx index 8ebf6b69a..024daf8e9 100644 --- a/frontend/src/__tests__/components/SpoolFormBulk.test.tsx +++ b/frontend/src/__tests__/components/SpoolFormBulk.test.tsx @@ -31,6 +31,7 @@ vi.mock('../../api/client', () => ({ getSpoolCatalog: vi.fn().mockResolvedValue([]), getColorCatalog: vi.fn().mockResolvedValue([]), getLocalPresets: vi.fn().mockResolvedValue({ filament: [] }), + getBuiltinFilaments: vi.fn().mockResolvedValue([]), getPrinters: vi.fn().mockResolvedValue([]), getSpoolUsageHistory: vi.fn().mockResolvedValue([]), createSpool: vi.fn().mockResolvedValue({ id: 99 }), diff --git a/frontend/src/__tests__/components/SpoolFormModal.test.tsx b/frontend/src/__tests__/components/SpoolFormModal.test.tsx index 68bf77e3b..d227d6f72 100644 --- a/frontend/src/__tests__/components/SpoolFormModal.test.tsx +++ b/frontend/src/__tests__/components/SpoolFormModal.test.tsx @@ -23,6 +23,7 @@ vi.mock('../../api/client', () => ({ getSpoolCatalog: vi.fn().mockResolvedValue([]), getColorCatalog: vi.fn().mockResolvedValue([]), getLocalPresets: vi.fn().mockResolvedValue({ filament: [] }), + getBuiltinFilaments: vi.fn().mockResolvedValue([]), getPrinters: vi.fn().mockResolvedValue([]), getSpoolUsageHistory: vi.fn().mockResolvedValue([]), createSpool: vi.fn().mockResolvedValue({ id: 99 }), diff --git a/frontend/src/__tests__/components/spool-form/buildFilamentOptions.test.ts b/frontend/src/__tests__/components/spool-form/buildFilamentOptions.test.ts new file mode 100644 index 000000000..c8787f45d --- /dev/null +++ b/frontend/src/__tests__/components/spool-form/buildFilamentOptions.test.ts @@ -0,0 +1,162 @@ +/** + * Regression tests for buildFilamentOptions (#1248). + * + * The original bug was precedence-based merging: cloud presets, when present, + * fully replaced the local-presets branch and silently hid Local Profiles. + * + * The follow-up clarification: the spool form is printer-agnostic, so it must + * show every per-printer / per-nozzle variant of a preset as its own entry — + * unlike the AMS Slot modal which is per-printer and filters down to the + * active printer model. Both surfaces should render the same set of presets + * if you summed the AMS Slot's per-printer-filtered output across all + * printers; the spool form just shows the union directly. + */ + +import { describe, it, expect } from 'vitest'; +import { buildFilamentOptions } from '../../../components/spool-form/utils'; +import type { SlicerSetting, LocalPreset, BuiltinFilament } from '../../../api/client'; + +const cloudPreset = (overrides: Partial = {}): SlicerSetting => ({ + setting_id: 'GFSL00@P1S', + name: 'Bambu PLA Basic @Bambu Lab P1S 0.4 nozzle', + type: 'filament', + version: null, + user_id: null, + updated_time: null, + is_custom: false, + ...overrides, +}); + +const localPreset = (overrides: Partial = {}): LocalPreset => ({ + id: 1, + name: 'My Custom PETG @Bambu Lab P2S 0.4 nozzle', + preset_type: 'filament', + source: 'local', + filament_type: 'GFG00', + filament_vendor: 'Acme', + nozzle_temp_min: 230, + nozzle_temp_max: 260, + pressure_advance: null, + default_filament_colour: null, + filament_cost: null, + filament_density: null, + compatible_printers: null, + inherits: null, + version: null, + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-01T00:00:00Z', + ...overrides, +}); + +const builtin = (overrides: Partial = {}): BuiltinFilament => ({ + filament_id: 'GFA00', + name: 'Bambu ASA Basic', + ...overrides, +}); + +describe('buildFilamentOptions', () => { + it('returns one entry per cloud setting_id (no @printer collapse)', () => { + const options = buildFilamentOptions( + [ + cloudPreset({ setting_id: 'GFSL00@P1S', name: 'Bambu PLA Basic @Bambu Lab P1S 0.4 nozzle' }), + cloudPreset({ setting_id: 'GFSL00@X1C', name: 'Bambu PLA Basic @Bambu Lab X1C 0.4 nozzle' }), + cloudPreset({ setting_id: 'GFSL00@A1', name: 'Bambu PLA Basic @Bambu Lab A1 0.4 nozzle' }), + ], + new Set(), + ); + expect(options).toHaveLength(3); + expect(options.map(o => o.code)).toEqual([ + 'GFSL00@A1', + 'GFSL00@P1S', + 'GFSL00@X1C', + ]); + }); + + it('keeps the @printer suffix in displayName so users can tell variants apart', () => { + const options = buildFilamentOptions( + [cloudPreset({ name: 'Bambu PLA Basic @Bambu Lab P1S 0.4 nozzle' })], + new Set(), + ); + expect(options[0].displayName).toBe('Bambu PLA Basic @Bambu Lab P1S 0.4 nozzle'); + }); + + it('merges local profiles even when cloud has presets (#1248 regression)', () => { + const options = buildFilamentOptions( + [cloudPreset()], + new Set(), + [localPreset()], + ); + const names = options.map(o => o.name); + expect(names).toContain('Bambu PLA Basic @Bambu Lab P1S 0.4 nozzle'); + expect(names).toContain('My Custom PETG @Bambu Lab P2S 0.4 nozzle'); + }); + + it('merges built-in filaments alongside cloud and local sources', () => { + const options = buildFilamentOptions( + [cloudPreset()], + new Set(), + [localPreset()], + [builtin()], + ); + const names = options.map(o => o.name); + expect(names).toContain('Bambu PLA Basic @Bambu Lab P1S 0.4 nozzle'); + expect(names).toContain('My Custom PETG @Bambu Lab P2S 0.4 nozzle'); + expect(names).toContain('Bambu ASA Basic'); + }); + + it('lists each local preset individually (no @printer collapse)', () => { + const options = buildFilamentOptions( + [], + new Set(), + [ + localPreset({ id: 1, name: 'My PETG @Bambu Lab P2S 0.4 nozzle' }), + localPreset({ id: 2, name: 'My PETG @Bambu Lab X1C 0.4 nozzle' }), + localPreset({ id: 3, name: 'My PETG @Bambu Lab P2S 0.6 nozzle' }), + ], + ); + expect(options).toHaveLength(3); + expect(options.map(o => o.name).sort()).toEqual([ + 'My PETG @Bambu Lab P2S 0.4 nozzle', + 'My PETG @Bambu Lab P2S 0.6 nozzle', + 'My PETG @Bambu Lab X1C 0.4 nozzle', + ]); + }); + + it('local-preset allCodes carries both filament_type and the row id for findPresetOption', () => { + const options = buildFilamentOptions( + [], + new Set(), + [localPreset({ id: 42, filament_type: 'GFL00' })], + ); + expect(options[0].allCodes).toEqual(expect.arrayContaining(['GFL00', '42'])); + }); + + it('falls back to hardcoded list only when every source is empty', () => { + const options = buildFilamentOptions([], new Set(), [], []); + expect(options.length).toBeGreaterThan(0); + expect(options.map(o => o.name)).toContain('Generic PLA'); + }); + + it('skips a built-in whose setting_id is already covered by cloud', () => { + const options = buildFilamentOptions( + [cloudPreset({ setting_id: 'GFSA00', name: 'Bambu ASA Basic' })], + new Set(), + undefined, + [builtin()], + ); + const asaCount = options.map(o => o.name).filter(n => n === 'Bambu ASA Basic').length; + expect(asaCount).toBe(1); + }); + + it('result is sorted alphabetically by displayName', () => { + const options = buildFilamentOptions( + [], + new Set(), + [ + localPreset({ id: 1, name: 'Zebra PLA' }), + localPreset({ id: 2, name: 'Alpha PLA' }), + ], + ); + expect(options.map(o => o.name)).toEqual(['Alpha PLA', 'Zebra PLA']); + }); +}); diff --git a/frontend/src/components/SpoolFormModal.tsx b/frontend/src/components/SpoolFormModal.tsx index 7e3c33b23..6730497ab 100644 --- a/frontend/src/components/SpoolFormModal.tsx +++ b/frontend/src/components/SpoolFormModal.tsx @@ -3,7 +3,7 @@ import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; import { useTranslation } from 'react-i18next'; import { X, Loader2, Save, Beaker, Palette, Zap, Tag, Unlink } from 'lucide-react'; import { api, ApiError } from '../api/client'; -import type { InventorySpool, SlicerSetting, SpoolCatalogEntry, LocalPreset, SpoolmanBulkCreateResult, SpoolKProfileInput, SpoolmanFilamentEntry } from '../api/client'; +import type { InventorySpool, SlicerSetting, SpoolCatalogEntry, LocalPreset, BuiltinFilament, SpoolmanBulkCreateResult, SpoolKProfileInput, SpoolmanFilamentEntry } from '../api/client'; import { Button } from './Button'; import { useToast } from '../contexts/ToastContext'; import type { SpoolFormData, PrinterWithCalibrations, ColorPreset } from './spool-form/types'; @@ -71,6 +71,9 @@ export function SpoolFormModal({ // Local presets (OrcaSlicer imports) const [localPresets, setLocalPresets] = useState([]); + // Built-in filaments (static fallback) + const [builtinFilaments, setBuiltinFilaments] = useState([]); + // Color catalog const [colorCatalog, setColorCatalog] = useState<{ manufacturer: string; color_name: string; hex_color: string; material: string | null }[]>([]); @@ -130,6 +133,7 @@ export function SpoolFormModal({ } api.getColorCatalog().then(setColorCatalog).catch(console.error); api.getLocalPresets().then(r => setLocalPresets(r.filament)).catch(console.error); + api.getBuiltinFilaments().then(setBuiltinFilaments).catch(console.error); // Fetch printer calibrations if not provided via props if (printersWithCalibrations.length === 0) { @@ -182,8 +186,8 @@ export function SpoolFormModal({ // Build filament options: cloud → local → fallback const filamentOptions = useMemo( - () => buildFilamentOptions(cloudPresets, new Set(), localPresets), - [cloudPresets, localPresets], + () => buildFilamentOptions(cloudPresets, new Set(), localPresets, builtinFilaments), + [cloudPresets, localPresets, builtinFilaments], ); // Extract brands from presets diff --git a/frontend/src/components/spool-form/FilamentSection.tsx b/frontend/src/components/spool-form/FilamentSection.tsx index 1ee7fb966..15c0ac227 100644 --- a/frontend/src/components/spool-form/FilamentSection.tsx +++ b/frontend/src/components/spool-form/FilamentSection.tsx @@ -164,7 +164,7 @@ export function FilamentSection({ ) : ( filteredPresets.map(option => (