From 56c632d2060bf6eb85117c95c1c093652e628963 Mon Sep 17 00:00:00 2001 From: Demian Date: Thu, 2 Apr 2026 23:43:37 -0400 Subject: [PATCH 1/3] fix: apply post-merge review follow-up - enforce non-negative org inventory quantity filters in controller parsing - add regression coverage for negative minQuantity and maxQuantity inputs - stabilize fallback inline drafts so memoized inventory rows do not rerender unnecessarily --- .../org-inventory.controller.spec.ts | 24 ++++++++ .../org-inventory/org-inventory.controller.ts | 6 ++ frontend/src/pages/Inventory.tsx | 58 +++++++++++++++---- 3 files changed, 76 insertions(+), 12 deletions(-) diff --git a/backend/src/modules/org-inventory/org-inventory.controller.spec.ts b/backend/src/modules/org-inventory/org-inventory.controller.spec.ts index fbaaf8f..b85d579 100644 --- a/backend/src/modules/org-inventory/org-inventory.controller.spec.ts +++ b/backend/src/modules/org-inventory/org-inventory.controller.spec.ts @@ -93,4 +93,28 @@ describe('OrgInventoryController', () => { new BadRequestException('location_id must be an integer'), ); }); + + it('throws a bad request for negative quantity filters', async () => { + await expect( + controller.list({ user: { userId: 7 } }, 42, { + gameId: '1', + minQuantity: '-0.25', + }), + ).rejects.toThrow( + new BadRequestException( + 'min_quantity must be greater than or equal to 0', + ), + ); + + await expect( + controller.list({ user: { userId: 7 } }, 42, { + gameId: '1', + maxQuantity: '-1', + }), + ).rejects.toThrow( + new BadRequestException( + 'max_quantity must be greater than or equal to 0', + ), + ); + }); }); diff --git a/backend/src/modules/org-inventory/org-inventory.controller.ts b/backend/src/modules/org-inventory/org-inventory.controller.ts index 6ac23e2..0f2eb68 100644 --- a/backend/src/modules/org-inventory/org-inventory.controller.ts +++ b/backend/src/modules/org-inventory/org-inventory.controller.ts @@ -158,11 +158,17 @@ export class OrgInventoryController { query, ['min_quantity', 'minQuantity'], 'min_quantity', + { + min: 0, + }, ), maxQuantity: this.readOptionalNumber( query, ['max_quantity', 'maxQuantity'], 'max_quantity', + { + min: 0, + }, ), }; diff --git a/frontend/src/pages/Inventory.tsx b/frontend/src/pages/Inventory.tsx index bbcd1ed..46ccc99 100644 --- a/frontend/src/pages/Inventory.tsx +++ b/frontend/src/pages/Inventory.tsx @@ -59,6 +59,7 @@ import { OrgPermission, permissionsService } from '../services/permissions.servi type InventoryRecord = InventoryItem | OrgInventoryItem; type ActionMode = 'edit' | 'split' | 'share' | 'delete' | null; +type InlineDraft = { locationId: number | ''; quantity: number | '' }; const GAME_ID = 1; const EDITOR_MODE_QUANTITY_MAX = 100000; @@ -155,9 +156,9 @@ const InventoryPage = () => { const [page, setPage] = useState(0); const [rowsPerPage, setRowsPerPage] = useState(25); const [density, setDensity] = useState<'standard' | 'compact'>(() => readStoredDensity()); - const [inlineDrafts, setInlineDrafts] = useState< - Record - >({}); + const [inlineDrafts, setInlineDrafts] = useState>( + {}, + ); const [inlineSaving, setInlineSaving] = useState>(new Set()); const [inlineSaved, setInlineSaved] = useState>(new Set()); const [inlineError, setInlineError] = useState>({}); @@ -226,6 +227,16 @@ const InventoryPage = () => { } } }, [orgOptions, selectedOrgId, viewMode]); + + useEffect(() => { + const itemIds = new Set(items.map((item) => item.id.toString())); + inlineDraftFallbacks.current.forEach((_, key) => { + if (!itemIds.has(key)) { + inlineDraftFallbacks.current.delete(key); + } + }); + }, [items]); + const debouncedNewItemSearch = useDebounce(newRowItemInput, 300); const debouncedNewLocationSearch = useDebounce(newRowLocationInput, 200); const getRowOrder = useCallback( @@ -253,6 +264,7 @@ const InventoryPage = () => { const locationRefs = useRef>({}); const quantityRefs = useRef>({}); const saveRefs = useRef>({}); + const inlineDraftFallbacks = useRef>(new Map()); const newRowItemRef = useRef(null); const newRowLocationRef = useRef(null); const newRowQuantityRef = useRef(null); @@ -1236,11 +1248,36 @@ const InventoryPage = () => { }; }, [debouncedNewItemSearch, isEditorMode]); + const getInlineDraft = useCallback( + (item: InventoryRecord): InlineDraft => { + const existingDraft = inlineDrafts[item.id]; + if (existingDraft) { + return existingDraft; + } + + const rowKey = item.id.toString(); + const nextFallback: InlineDraft = { + locationId: Number(item.locationId) || '', + quantity: Number(item.quantity) || 0, + }; + const cachedFallback = inlineDraftFallbacks.current.get(rowKey); + + if ( + cachedFallback && + cachedFallback.locationId === nextFallback.locationId && + cachedFallback.quantity === nextFallback.quantity + ) { + return cachedFallback; + } + + inlineDraftFallbacks.current.set(rowKey, nextFallback); + return nextFallback; + }, + [inlineDrafts], + ); + const handleInlineSave = useCallback(async (item: InventoryRecord) => { - const draft = inlineDrafts[item.id] ?? { - locationId: item.locationId ?? '', - quantity: Number(item.quantity) || 0, - }; + const draft = getInlineDraft(item); const parsedLocationId = draft.locationId === '' ? NaN : Number(draft.locationId); const parsedQuantity = Number(draft.quantity); @@ -1327,7 +1364,7 @@ const InventoryPage = () => { } }, [ focusController, - inlineDrafts, + getInlineDraft, inlineSaving, items, locationNameById, @@ -1785,10 +1822,7 @@ const InventoryPage = () => { const showEmptyState = filteredItems.length === 0 && !refreshing; const renderInlineRow = (item: InventoryRecord) => { const rowKey = item.id.toString(); - const draft = inlineDrafts[item.id] ?? { - locationId: Number(item.locationId) || '', - quantity: Number(item.quantity) || 0, - }; + const draft = getInlineDraft(item); const originalLocationId = Number(item.locationId) || ''; const draftLocationId = normalizeDraftLocationId(draft.locationId); const originalQuantity = Number(item.quantity) || 0; From 4f9d50c1b16fab00ef1d3bd819337f706ebd2c85 Mon Sep 17 00:00:00 2001 From: Demian Date: Tue, 7 Apr 2026 20:00:18 -0400 Subject: [PATCH 2/3] docs: document intentional ref mutation in getInlineDraft Adds inline comment explaining why writing to inlineDraftFallbacks.current during render is safe: the value is derived deterministically from item data so concurrent/aborted renders are idempotent, and stale entries are pruned by the existing [items] effect. --- frontend/src/pages/Inventory.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/frontend/src/pages/Inventory.tsx b/frontend/src/pages/Inventory.tsx index 46ccc99..d038835 100644 --- a/frontend/src/pages/Inventory.tsx +++ b/frontend/src/pages/Inventory.tsx @@ -1270,6 +1270,10 @@ const InventoryPage = () => { return cachedFallback; } + // Writing to a ref during render is intentional here. nextFallback is + // derived deterministically from item data, so concurrent/aborted renders + // always write the same value for a given key — no stale state is possible. + // Stale entries for removed items are pruned by the [items] effect above. inlineDraftFallbacks.current.set(rowKey, nextFallback); return nextFallback; }, From 97f7b7fc338ae7c115f7106d2ea1e8f1d29b36c1 Mon Sep 17 00:00:00 2001 From: Demian Date: Tue, 7 Apr 2026 20:23:59 -0400 Subject: [PATCH 3/3] fix: move inlineDraftFallbacks declaration above items effect Resolves use-before-define ordering: useRef was declared after the useEffect that references it. --- frontend/src/pages/Inventory.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frontend/src/pages/Inventory.tsx b/frontend/src/pages/Inventory.tsx index d038835..26cb996 100644 --- a/frontend/src/pages/Inventory.tsx +++ b/frontend/src/pages/Inventory.tsx @@ -228,6 +228,8 @@ const InventoryPage = () => { } }, [orgOptions, selectedOrgId, viewMode]); + const inlineDraftFallbacks = useRef>(new Map()); + useEffect(() => { const itemIds = new Set(items.map((item) => item.id.toString())); inlineDraftFallbacks.current.forEach((_, key) => { @@ -264,7 +266,6 @@ const InventoryPage = () => { const locationRefs = useRef>({}); const quantityRefs = useRef>({}); const saveRefs = useRef>({}); - const inlineDraftFallbacks = useRef>(new Map()); const newRowItemRef = useRef(null); const newRowLocationRef = useRef(null); const newRowQuantityRef = useRef(null);