From c907e0d5e58219402082dbb183b649cd53e8c137 Mon Sep 17 00:00:00 2001 From: Maurice Date: Wed, 2 Sep 2026 19:00:49 +0200 Subject: [PATCH] fix(packing): bag totals survive reconnects and MCP bag deletes (#2191) Deleting a bag over MCP moves every figure, the items fall into the unassigned pile, yet it sent no totals ping; it pings the room now like REST and the plugin RPC do. The client refetches totals after a room re-join and when coming back online, through the same coalescing window. The plugin SDK types learn the create fields the routes accept since #2154, on both hand-kept declarations. --- .../Packing/useBagTotalsPing.test.tsx | 45 ++++++++++++++++++- .../components/Packing/useBagTotalsPing.ts | 28 +++++++++++- plugin-sdk/src/index.ts | 4 +- server/src/nest/packing/packing.mcp.ts | 3 ++ server/src/nest/plugins/runtime/plugin-sdk.ts | 4 +- .../unit/mcp/tools-packing-advanced.test.ts | 21 +++++++++ 6 files changed, 98 insertions(+), 7 deletions(-) diff --git a/client/src/components/Packing/useBagTotalsPing.test.tsx b/client/src/components/Packing/useBagTotalsPing.test.tsx index 6ca171e96..189ad0924 100644 --- a/client/src/components/Packing/useBagTotalsPing.test.tsx +++ b/client/src/components/Packing/useBagTotalsPing.test.tsx @@ -1,4 +1,4 @@ -// FE-COMP-BAGPING-001 to FE-COMP-BAGPING-006 +// FE-COMP-BAGPING-001 to FE-COMP-BAGPING-009 /** * The bag-totals ping listener (#2191). @@ -13,6 +13,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' import { renderHook, act } from '@testing-library/react' import { useBagTotalsPing } from './useBagTotalsPing' +import { setForcedOffline, _resetNetworkMode } from '../../sync/networkMode' const listeners = new Set<(e: Record) => void>() @@ -33,10 +34,12 @@ async function flushCoalesce(): Promise { beforeEach(() => { listeners.clear() + _resetNetworkMode() vi.useFakeTimers() }) afterEach(() => { + setForcedOffline(false) vi.useRealTimers() }) @@ -118,4 +121,44 @@ describe('useBagTotalsPing', () => { expect(listeners.size).toBe(0) expect(reload).not.toHaveBeenCalled() }) + + it('FE-COMP-BAGPING-007: reloads when the socket re-joins the room', async () => { + // A ping sent while the socket was down is gone for good (a container + // restart, a tunnel that dropped), and bags have no offline cache to catch + // up from. The re-join is the only notice this client gets. + const reload = vi.fn(async () => {}) + renderHook(() => useBagTotalsPing(true, reload)) + + act(() => emit('joined')) + await flushCoalesce() + + expect(reload).toHaveBeenCalledTimes(1) + }) + + it('FE-COMP-BAGPING-008: reloads on the way back online, not on the way out', async () => { + const reload = vi.fn(async () => {}) + renderHook(() => useBagTotalsPing(true, reload)) + + act(() => setForcedOffline(true)) + await flushCoalesce() + expect(reload).not.toHaveBeenCalled() + + // Going online is where the surfaces stop summing locally and show the + // server totals again, so those totals had better not be the pre-outage ones. + act(() => setForcedOffline(false)) + await flushCoalesce() + expect(reload).toHaveBeenCalledTimes(1) + }) + + it('FE-COMP-BAGPING-009: stops watching the network mode after unmount', async () => { + const reload = vi.fn(async () => {}) + const { unmount } = renderHook(() => useBagTotalsPing(true, reload)) + + act(() => setForcedOffline(true)) + unmount() + act(() => setForcedOffline(false)) + await flushCoalesce() + + expect(reload).not.toHaveBeenCalled() + }) }) diff --git a/client/src/components/Packing/useBagTotalsPing.ts b/client/src/components/Packing/useBagTotalsPing.ts index f454f9b4f..e63687749 100644 --- a/client/src/components/Packing/useBagTotalsPing.ts +++ b/client/src/components/Packing/useBagTotalsPing.ts @@ -1,5 +1,6 @@ import { useEffect, useRef } from 'react' import { addListener, removeListener } from '../../api/websocket' +import { isEffectivelyOnline, onNetworkModeChange } from '../../sync/networkMode' /** * How long pings are collected before one refetch goes out. @@ -21,6 +22,11 @@ const COALESCE_MS = 250 * included, so a busy trip would otherwise have every client refetching once * per written item. * + * A ping sent while this client is disconnected is gone for good, and bags have + * no offline cache to sync back, so the two moments where that can have happened + * count as pings of their own: the room re-join after a reconnect, and the + * return to online. + * * `reload` may change identity every render; the latest one is always used. */ export function useBagTotalsPing(enabled: boolean, reload: () => Promise): void { @@ -51,8 +57,7 @@ export function useBagTotalsPing(enabled: boolean, reload: () => Promise): }) } - const handler = (event: Record) => { - if (event.type !== 'packing:bag-totals') return + const schedule = (): void => { if (timer) return // already collecting this burst timer = setTimeout(() => { timer = null @@ -60,10 +65,29 @@ export function useBagTotalsPing(enabled: boolean, reload: () => Promise): }, COALESCE_MS) } + const handler = (event: Record) => { + // 'joined' is the room re-join the socket sends on every reconnect, so it + // is also the moment the totals on screen are the ones from before the + // drop: whatever moved them in the meantime pinged into a closed socket. + if (event.type !== 'packing:bag-totals' && event.type !== 'joined') return + schedule() + } + + let wasOnline = isEffectivelyOnline() + const onMode = (): void => { + const online = isEffectivelyOnline() + // Only the way back matters: that is when the surfaces stop summing what + // this client can see and go back to trusting the server numbers. + if (online && !wasOnline) schedule() + wasOnline = online + } + addListener(handler) + const unsubscribeMode = onNetworkModeChange(onMode) return () => { cancelled = true removeListener(handler) + unsubscribeMode() if (timer) clearTimeout(timer) } }, [enabled]) diff --git a/plugin-sdk/src/index.ts b/plugin-sdk/src/index.ts index bb7df184b..1ade37925 100644 --- a/plugin-sdk/src/index.ts +++ b/plugin-sdk/src/index.ts @@ -109,7 +109,7 @@ export interface PluginContext { /** A trip's packing items (hydrated bags/assignees). Needs `db:read:packing`. */ list(tripId: number): Promise; /** Add a packing item (owner = acting user). Needs `db:write:packing` + packing_edit. */ - create(tripId: number, input: { name: string; category?: string; checked?: boolean; is_private?: boolean; visibility?: 'common' | 'personal' | 'shared'; recipient_ids?: number[] }): Promise; + create(tripId: number, input: { name: string; category?: string; checked?: boolean; weight_grams?: number | null; bag_id?: number | null; quantity?: number; is_private?: boolean; visibility?: 'common' | 'personal' | 'shared'; recipient_ids?: number[] }): Promise; /** Update a packing item. Needs `db:write:packing` + packing_edit. */ update(tripId: number, itemId: number, input: Record): Promise; /** Delete a packing item. Needs `db:write:packing` + packing_edit. */ @@ -118,7 +118,7 @@ export interface PluginContext { // `db:write:packing` + packing_edit. /** Needs 'db:write:packing' (intentional — bags are the write-side structure; packing.list is the read surface). */ listBags(tripId: number): Promise; - createBag(tripId: number, input: { name: string; color?: string }): Promise; + createBag(tripId: number, input: { name: string; color?: string; weight_limit_grams?: number }): Promise; updateBag(tripId: number, bagId: number, input: Record): Promise; deleteBag(tripId: number, bagId: number): Promise<{ deleted: boolean }>; setBagMembers(tripId: number, bagId: number, userIds: number[]): Promise; diff --git a/server/src/nest/packing/packing.mcp.ts b/server/src/nest/packing/packing.mcp.ts index cefa8082b..047bc2cf5 100644 --- a/server/src/nest/packing/packing.mcp.ts +++ b/server/src/nest/packing/packing.mcp.ts @@ -363,6 +363,9 @@ export class PackingMcp { // { bagId } matches the REST route and the plugin host (the legacy // registrar's { id } was the odd one out). this.guards.safeBroadcast(tripId, 'packing:bag-deleted', { bagId }); + // packing_items.bag_id is ON DELETE SET NULL, so the contents land in the + // unassigned pile and both numbers move (#2191), as on REST and plugin RPC. + this.packing.broadcastBagTotals(String(tripId)); return ok({ success: true }); } diff --git a/server/src/nest/plugins/runtime/plugin-sdk.ts b/server/src/nest/plugins/runtime/plugin-sdk.ts index 7939dd589..3fbd0fa50 100644 --- a/server/src/nest/plugins/runtime/plugin-sdk.ts +++ b/server/src/nest/plugins/runtime/plugin-sdk.ts @@ -91,7 +91,7 @@ export interface PluginContext { /** A trip's packing items (hydrated bags/assignees). Needs 'db:read:packing'. */ list(tripId: number): Promise; /** Add a packing item (owner = acting user). Needs 'db:write:packing' + 'packing_edit'. */ - create(tripId: number, input: { name: string; category?: string; checked?: boolean; is_private?: boolean; visibility?: 'common' | 'personal' | 'shared'; recipient_ids?: number[] }): Promise; + create(tripId: number, input: { name: string; category?: string; checked?: boolean; weight_grams?: number | null; bag_id?: number | null; quantity?: number; is_private?: boolean; visibility?: 'common' | 'personal' | 'shared'; recipient_ids?: number[] }): Promise; /** Update a packing item. Needs 'db:write:packing' + 'packing_edit'. */ update(tripId: number, itemId: number, input: Record): Promise; /** Delete a packing item. Needs 'db:write:packing' + 'packing_edit'. */ @@ -103,7 +103,7 @@ export interface PluginContext { * (intentional — bags are the write-side structure; packing.list is the read surface). */ listBags(tripId: number): Promise; - createBag(tripId: number, input: { name: string; color?: string }): Promise; + createBag(tripId: number, input: { name: string; color?: string; weight_limit_grams?: number }): Promise; updateBag(tripId: number, bagId: number, input: Record): Promise; deleteBag(tripId: number, bagId: number): Promise<{ deleted: boolean }>; setBagMembers(tripId: number, bagId: number, userIds: number[]): Promise; diff --git a/server/tests/unit/mcp/tools-packing-advanced.test.ts b/server/tests/unit/mcp/tools-packing-advanced.test.ts index a107e3b49..7e477e383 100644 --- a/server/tests/unit/mcp/tools-packing-advanced.test.ts +++ b/server/tests/unit/mcp/tools-packing-advanced.test.ts @@ -256,6 +256,27 @@ describe('Tool: delete_packing_bag', () => { }); }); + it('pings the room to re-read the bag weights (#2191)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const r = testDb.prepare('INSERT INTO packing_bags (trip_id, name, color) VALUES (?, ?, ?)').run(trip.id, 'Carry-on', '#000000'); + const bagId = r.lastInsertRowid as number; + testDb.prepare('INSERT INTO packing_items (trip_id, name, category, checked, bag_id, weight_grams) VALUES (?, ?, ?, 0, ?, ?)') + .run(trip.id, 'Tent', 'Camping', bagId, 3000); + await withHarness(user.id, async (h) => { + await h.client.callTool({ + name: 'delete_packing_bag', + arguments: { tripId: trip.id, bagId }, + }); + // bag_id is ON DELETE SET NULL, so the 3 kg move into the unassigned pile: + // both numbers change and every client has to re-read them, exactly as on + // the REST route and the plugin RPC. + expect(broadcastMock).toHaveBeenCalledWith(String(trip.id), 'packing:bag-totals', {}, undefined); + expect(broadcastMock).toHaveBeenCalledTimes(2); + expect(testDb.prepare('SELECT bag_id FROM packing_items WHERE trip_id = ?').get(trip.id)).toEqual({ bag_id: null }); + }); + }); + it('returns access denied for non-member', async () => { const { user } = createUser(testDb); const { user: other } = createUser(testDb);