From 11fe68578efcb6e65f00e3f2c76ab25670b7209f Mon Sep 17 00:00:00 2001 From: maziggy Date: Sun, 26 Apr 2026 11:45:18 +0200 Subject: [PATCH] fix(#1133): list every spool in the AMS-slot inventory picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The picker that opens from / SpoolBuddy's slot-action sheet had two stacked filters that together blocked a real workflow: 1. AssignSpoolModal only listed spools whose tag_uid AND tray_uuid were both null, hiding any Bambu Lab spool that had been auto-created from RFID or scanned via SpoolBuddy NFC. 2. FilamentHoverCard rendered its inventory section only when the slot's vendor was not 'Bambu Lab', so even with #1 fixed the assign button wasn't visible on a BL slot. Both filters blocked the same use case: a user has a Bambu Lab spool in inventory but doesn't want to scan via SpoolBuddy NFC every time and just wants to pick it from the list. Both gates removed. Modal lists every spool that isn't already taken by another (printer/ams_id/tray_id) tuple. Hover-card inventory section renders for every vendor including Bambu Lab. The AMS-vs-external special-case in the modal collapsed too — external slots used to be the only path that allowed picking a tagged spool, that distinction is gone. Empty slots lost their assign affordance entirely. A physically empty slot has no spool to attach an inventory record to, and offering the action there only led to users assigning the wrong spool to a slot the printer hadn't actually loaded yet. Bambuddy: EmptySlotHoverCard's inventory prop removed; PrintersPage drops the matching inventory props on three sites. SpoolBuddy: slot-action picker gates the assign/unassign block on slotActionPicker.tray !== null. Modal also gets defensive hardening from the rollout investigation: its own dedicated cache key (['inventory-spools', 'assign-modal']) so it can't be poisoned by other components calling getSpools() with different includeArchived args; getSpools(true) + client-side !archived_at filter so the picker sees the full inventory regardless of cache priming order; "Show all spools" toggle now bypasses BOTH filters (was only bypassing material/profile, label was a lie); a small "X fetched · Y archived · Z assigned" counter in the empty state so future "missing spool" reports are debuggable from screenshots. i18n.inventory.noManualSpools renamed to inventory.noAvailableSpools with new copy ("No spools available. Add a spool to your inventory or unassign one from another slot first.") since the empty-state premise changed. Localised across all 8 languages. 15 frontend tests: assign/unassign render for vendor: 'Bambu Lab', non-BL vendors unchanged, EmptySlotHoverCard renders no assign affordance, configure button still works on empty slots, picker lists BL spools alongside manual ones, picker drops spools assigned elsewhere unless toggle is on, picker drops archived spools always, toggle escape-hatch shows everything, empty-state copy update. SpoolBuddy unassign now invalidates both ['spool-assignments'] and ['spool-assignments', printerId] so the modal's cache stays fresh (dual cache-key consolidation deferred to its own PR). --- .../components/AssignSpoolModal.test.tsx | 61 ++++++++-- .../components/FilamentHoverCard.test.tsx | 112 +++++++++++++++++- frontend/src/components/AssignSpoolModal.tsx | 67 +++++++++-- frontend/src/components/FilamentHoverCard.tsx | 34 +++--- frontend/src/i18n/locales/de.ts | 2 +- frontend/src/i18n/locales/en.ts | 2 +- frontend/src/i18n/locales/fr.ts | 2 +- frontend/src/i18n/locales/it.ts | 2 +- frontend/src/i18n/locales/ja.ts | 2 +- frontend/src/i18n/locales/pt-BR.ts | 2 +- frontend/src/i18n/locales/zh-CN.ts | 2 +- frontend/src/i18n/locales/zh-TW.ts | 2 +- frontend/src/pages/PrintersPage.tsx | 48 +------- .../pages/spoolbuddy/SpoolBuddyAmsPage.tsx | 22 +++- 14 files changed, 269 insertions(+), 91 deletions(-) diff --git a/frontend/src/__tests__/components/AssignSpoolModal.test.tsx b/frontend/src/__tests__/components/AssignSpoolModal.test.tsx index 21098cb75..0927b0a67 100644 --- a/frontend/src/__tests__/components/AssignSpoolModal.test.tsx +++ b/frontend/src/__tests__/components/AssignSpoolModal.test.tsx @@ -77,19 +77,23 @@ describe('AssignSpoolModal', () => { expect(screen.queryByText('Assign Spool')).not.toBeInTheDocument(); }); - it('filters out Bambu Lab spools (with tag_uid/tray_uuid)', async () => { + // Inverted from the original "filters out BL spools" expectation in #1133. + // Bambu Lab spools (tag_uid + tray_uuid populated by SpoolBuddy NFC scan or + // auto-creation) used to be hidden from this picker, blocking the workflow + // where a user has a BL spool in inventory but doesn't want to scan it via + // SpoolBuddy each time and just wants to pick it from the list. The picker + // now lists every spool that isn't already assigned to another slot. + it('lists Bambu Lab spools (with tag_uid/tray_uuid) alongside manual ones (#1133)', async () => { render(); await waitFor(() => { expect(screen.getByText(/Polymaker/)).toBeInTheDocument(); }); - // Manual spools should be visible expect(screen.getByText(/Polymaker/)).toBeInTheDocument(); expect(screen.getByText(/Overture/)).toBeInTheDocument(); - - // BL spool should NOT be visible - expect(screen.queryByText(/Jade White/)).not.toBeInTheDocument(); + // The previously-excluded BL spool is now visible. + expect(screen.getByText(/Jade White/)).toBeInTheDocument(); }); it('filters out spools already assigned to other slots', async () => { @@ -125,16 +129,57 @@ describe('AssignSpoolModal', () => { expect(screen.getByText(/Polymaker/)).toBeInTheDocument(); }); - it('shows noManualSpools message when all spools are BL or assigned', async () => { - (api.getSpools as ReturnType).mockResolvedValue([blSpool]); + // Empty-state premise reworked for #1133: BL spools no longer trigger + // the empty state by virtue of being BL, so we exercise the only + // remaining trigger — every spool already taken by another slot. + it('shows noAvailableSpools message when every spool is already assigned elsewhere', async () => { + (api.getSpools as ReturnType).mockResolvedValue([manualSpool]); + (api.getAssignments as ReturnType).mockResolvedValue([ + // manualSpool (id=1) is taken by a different (printer/ams/tray) tuple, + // so it must be filtered out of THIS slot's picker. + { id: 99, spool_id: 1, printer_id: 1, ams_id: 0, tray_id: 1 }, + ]); render(); await waitFor(() => { - expect(screen.getByText(/No manually added spools/i)).toBeInTheDocument(); + expect(screen.getByText(/No spools available/i)).toBeInTheDocument(); }); }); + // The toggle's label says "Show all spools" but originally only bypassed + // material/profile filtering — spools assigned elsewhere stayed hidden + // even with the toggle on. That made it impossible to recover from the + // case where MQTT auto-reassignment beat a manual unassign by a few + // milliseconds, leaving the just-freed spool in another slot's + // assignment row and out of reach of this picker. With the toggle on, + // every spool is now listed regardless of where it's currently assigned. + it('lists spools assigned to other slots when "Show all spools" toggle is enabled', async () => { + (api.getSpools as ReturnType).mockResolvedValue([manualSpool, anotherManualSpool]); + (api.getAssignments as ReturnType).mockResolvedValue([ + // anotherManualSpool (id=3) is taken by a different slot. + { id: 99, spool_id: 3, printer_id: 1, ams_id: 0, tray_id: 1 }, + ]); + + render(); + + // Default state: spool 3 is hidden because it's assigned elsewhere. + await waitFor(() => { + expect(screen.getByText(/Polymaker/)).toBeInTheDocument(); + }); + expect(screen.queryByText(/Overture/)).not.toBeInTheDocument(); + + // Flip the toggle — both spools must now appear, including the one + // currently assigned to the other slot. + const toggle = screen.getByLabelText(/show all spools/i); + toggle.click(); + + await waitFor(() => { + expect(screen.getByText(/Overture/)).toBeInTheDocument(); + }); + expect(screen.getByText(/Polymaker/)).toBeInTheDocument(); + }); + it('lists spool with no slicer profile when material matches the tray (#1047)', async () => { const spoolWithoutSlicerProfile = { id: 10, diff --git a/frontend/src/__tests__/components/FilamentHoverCard.test.tsx b/frontend/src/__tests__/components/FilamentHoverCard.test.tsx index be9f5decb..6a0cff533 100644 --- a/frontend/src/__tests__/components/FilamentHoverCard.test.tsx +++ b/frontend/src/__tests__/components/FilamentHoverCard.test.tsx @@ -5,7 +5,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { render, screen, fireEvent, waitFor } from '../utils'; -import { FilamentHoverCard } from '../../components/FilamentHoverCard'; +import { FilamentHoverCard, EmptySlotHoverCard } from '../../components/FilamentHoverCard'; const baseFilamentData = { vendor: 'Bambu Lab' as const, @@ -165,4 +165,114 @@ describe('FilamentHoverCard', () => { }); }); }); + + // The inventory section was previously hidden for `vendor === 'Bambu Lab'` + // because BL spools were assumed to be managed entirely via RFID. #1133 + // removed that gate so users who don't want to scan via SpoolBuddy NFC + // can still pick a BL spool from inventory the same way they pick a + // third-party one. + describe('inventory section vendor visibility (#1133)', () => { + it('shows the assign-spool button on a Bambu Lab slot when the spool is unassigned', async () => { + const onAssign = vi.fn(); + renderWithHover( + +
trigger
+ + ); + vi.advanceTimersByTime(100); + await waitFor(() => { + expect(screen.getByText(/assign/i)).toBeInTheDocument(); + }); + }); + + it('shows the unassign button on a Bambu Lab slot when an inventory spool is already assigned', async () => { + // Regression guard: the original gate hid BOTH the assign and unassign + // buttons for BL slots. A user who'd already assigned an inventory + // spool to a BL slot couldn't undo it without dropping into the + // inventory page directly. + const onUnassign = vi.fn(); + renderWithHover( + +
trigger
+ + ); + vi.advanceTimersByTime(100); + await waitFor(() => { + expect(screen.getByText(/unassign/i)).toBeInTheDocument(); + }); + }); + + it('still shows the assign-spool button for a non-Bambu vendor (no behaviour change)', async () => { + const onAssign = vi.fn(); + renderWithHover( + +
trigger
+ + ); + vi.advanceTimersByTime(100); + await waitFor(() => { + expect(screen.getByText(/assign/i)).toBeInTheDocument(); + }); + }); + }); +}); + +// EmptySlotHoverCard is the hover wrapper rendered for a physically empty +// AMS slot. #1133 removed its inventory affordance: a slot with nothing +// loaded has no spool to attach an inventory record to, and offering the +// action there only led to users assigning the wrong spool to a slot the +// printer hadn't actually loaded yet. The configure-slot affordance is +// kept, since "preset for the next spool to land here" is still a sensible +// thing to do on an empty slot. +describe('EmptySlotHoverCard (#1133)', () => { + beforeEach(() => { + vi.useFakeTimers({ shouldAdvanceTime: true }); + }); + + it('does not render an assign-spool affordance', async () => { + const result = render( + +
trigger
+ + ); + fireEvent.mouseEnter(result.container.firstElementChild as HTMLElement); + vi.advanceTimersByTime(100); + await waitFor(() => { + // The card itself is showing — guard the negative assertion against + // a card that simply never opened. + expect(screen.getByText(/empty/i)).toBeInTheDocument(); + }); + expect(screen.queryByText(/assign/i)).not.toBeInTheDocument(); + }); + + it('still shows the configure button on an empty slot', async () => { + const onConfigure = vi.fn(); + const result = render( + +
trigger
+ + ); + fireEvent.mouseEnter(result.container.firstElementChild as HTMLElement); + vi.advanceTimersByTime(100); + await waitFor(() => { + expect(screen.getByText(/configure/i)).toBeInTheDocument(); + }); + }); }); diff --git a/frontend/src/components/AssignSpoolModal.tsx b/frontend/src/components/AssignSpoolModal.tsx index 7670f5a24..e196957d6 100644 --- a/frontend/src/components/AssignSpoolModal.tsx +++ b/frontend/src/components/AssignSpoolModal.tsx @@ -49,9 +49,22 @@ export function AssignSpoolModal({ isOpen, onClose, printerId, amsId, trayId, tr } }, [isOpen]); + // Unique cache key — different consumers of `['inventory-spools']` call + // `getSpools()` with different `includeArchived` arguments (InventoryPage: + // true, SpoolBuddyDashboard / SpoolBuddyInventoryPage: false), but they + // all share the same key. React Query treats them as one query and + // serves whichever response landed first, so a SpoolBuddy component + // priming the cache with the archived-excluded payload makes the picker + // miss spools that *are* archived OR (more subtly) miss any spool that + // wasn't yet present when SpoolBuddy ran its initial fetch. The picker + // gets its own key + a fetch-everything call so this consumer is never + // at the mercy of someone else's cache state. Archived spools are then + // explicitly excluded client-side because the backend rejects archived + // assignments with HTTP 400 anyway, so listing them would only let the + // user click a button that fails. const { data: spools, isLoading } = useQuery({ - queryKey: ['inventory-spools'], - queryFn: () => api.getSpools(), + queryKey: ['inventory-spools', 'assign-modal'], + queryFn: () => api.getSpools(true), enabled: isOpen, }); @@ -137,11 +150,27 @@ export function AssignSpoolModal({ isOpen, onClose, printerId, amsId, trayId, tr .filter(a => !(a.printer_id === printerId && a.ams_id === amsId && a.tray_id === trayId)) .map(a => a.spool_id) ); - // External slots (amsId 254 or 255) have no RFID reader, so show all spools. - // AMS slots only show manual spools (no tag_uid or tray_uuid). - const isExternalSlot = amsId === 254 || amsId === 255; - const manualSpools = spools?.filter((spool: InventorySpool) => - !assignedSpoolIds.has(spool.id) && (isExternalSlot || (!spool.tag_uid && !spool.tray_uuid)) + // Show every spool that isn't already taken by another slot — including + // RFID-tagged Bambu Lab spools (#1133). The earlier "manual spools only" + // gate (tag_uid && tray_uuid both null) blocked the workflow where a + // user has a Bambu Lab spool in inventory but doesn't want to scan it + // via SpoolBuddy NFC every time and just wants to pick it from the list. + // External slots (amsId 254/255) have always been allowed to pick from + // any spool because the slot itself has no RFID reader; that + // distinction collapses now that AMS slots also accept any spool. + // + // The "Show all spools" toggle (disableFiltering) bypasses BOTH this + // gate and the material/profile filter below, making it a real escape + // hatch for cases where MQTT has auto-reassigned a spool to another + // slot a fraction of a second after a manual unassign — without this, + // the toggle's label is a lie ("Show all" but actually filters by + // assignment). The backend's assign_spool route is upsert-per- + // (printer, ams, tray), so picking a spool that's currently taken by + // a different slot creates a second assignment row; that's a foot-gun + // for normal flows but exactly the recovery path the toggle is for. + const availableSpools = spools?.filter((spool: InventorySpool) => + !spool.archived_at && + (disableFiltering || !assignedSpoolIds.has(spool.id)) ); // Filtering logic with toggle: search filter always applies, AMS tray profile filter is optional. @@ -149,7 +178,7 @@ export function AssignSpoolModal({ isOpen, onClose, printerId, amsId, trayId, tr // tray's material (partial-match both directions — "PLA" spool accepts a "PLA Basic" slot and // vice versa). Manually-added inventory spools typically have no slicer_filament_name; gating // on strict profile equality alone hid them even when the material matched (#1047). - let filteredSpools = manualSpools; + let filteredSpools = availableSpools; if (!disableFiltering) { const trayProfile = stripProfileQualifier(normalizeValue(trayInfo?.profile)); const trayMaterial = normalizeValue(trayInfo?.material || trayInfo?.type); @@ -326,13 +355,31 @@ export function AssignSpoolModal({ isOpen, onClose, printerId, amsId, trayId, tr ))} - ) : manualSpools && manualSpools.length === 0 ? ( + ) : availableSpools && availableSpools.length === 0 ? (
-

{t('inventory.noManualSpools')}

+

{t('inventory.noAvailableSpools')}

+ {/* Diagnostic counter — when the picker is empty, having + the raw fetch / filter counts visible makes a + "spool I expected to see is missing" report + immediately answerable: if `total fetched` is 0 the + backend / cache returned nothing; if it's > 0 then + the archived / assigned-elsewhere filter ate the + spool and the toggle is the right escape hatch. */} + {spools && ( +

+ {spools.length} fetched · {spools.filter(s => s.archived_at).length} archived ·{' '} + {spools.filter(s => assignedSpoolIds.has(s.id)).length} assigned to other slots +

+ )}
) : (

{t('inventory.noSpoolsMatch')}

+ {availableSpools && ( +

+ {availableSpools.length} unassigned spools — {(availableSpools.length) - (filteredSpools?.length ?? 0)} filtered by tray match. Try "Show all spools". +

+ )}
)} diff --git a/frontend/src/components/FilamentHoverCard.tsx b/frontend/src/components/FilamentHoverCard.tsx index 7473b380c..3cc1a7f0c 100644 --- a/frontend/src/components/FilamentHoverCard.tsx +++ b/frontend/src/components/FilamentHoverCard.tsx @@ -345,8 +345,12 @@ export function FilamentHoverCard({ data, children, disabled, className = '', sp )} - {/* Inventory section - only for non-Bambu spools */} - {inventory && data.vendor !== 'Bambu Lab' && ( + {/* Inventory section — shown for every vendor including + Bambu Lab (#1133). The earlier "non-Bambu only" gate + prevented users from manually assigning a Bambu spool + in inventory to an AMS slot when they didn't want to + re-scan via SpoolBuddy NFC. */} + {inventory && (
{inventory.assignedSpool ? ( <> @@ -468,13 +472,18 @@ interface EmptySlotHoverCardProps { children: ReactNode; className?: string; configureSlot?: ConfigureSlotConfig; - inventory?: InventoryConfig; } /** - * Wrapper for empty slots - shows "Empty" on hover with optional configure button + * Wrapper for empty slots - shows "Empty" on hover with optional configure button. + * + * The "Assign spool" affordance was removed from empty slots in #1133: a + * physically empty slot has no spool to attach to, and offering the + * action there only led to users assigning the wrong spool to a slot + * the printer hadn't actually loaded yet. Assignment now requires a + * loaded slot (which renders FilamentHoverCard, where the button lives). */ -export function EmptySlotHoverCard({ children, className = '', configureSlot, inventory }: EmptySlotHoverCardProps) { +export function EmptySlotHoverCard({ children, className = '', configureSlot }: EmptySlotHoverCardProps) { const { t } = useTranslation(); const [isVisible, setIsVisible] = useState(false); const timeoutRef = useRef | null>(null); @@ -531,21 +540,6 @@ export function EmptySlotHoverCard({ children, className = '', configureSlot, in
)} - {/* Assign spool button - allows assigning inventory spool to empty slot */} - {inventory?.onAssignSpool && ( -
- -
- )}
- {/* Inventory: Assign or Unassign */} - {!spoolmanEnabled && (assignment ? ( + {/* Inventory: Assign or Unassign — only when a spool is + physically loaded in the slot (#1133). An empty slot + has nothing to attach an inventory record to, and + showing the action there only led to users assigning + the wrong spool to a slot the printer hadn't actually + loaded yet. handleAmsSlotClick / handleExtSlotClick + both pass tray=null for empty slots. */} + {!spoolmanEnabled && slotActionPicker?.tray && (assignment ? (