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 ? (