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.
This commit is contained in:
Maurice
2026-09-02 19:00:49 +02:00
committed by Maurice
parent 9cc427d156
commit c907e0d5e5
6 changed files with 98 additions and 7 deletions
@@ -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<string, unknown>) => void>()
@@ -33,10 +34,12 @@ async function flushCoalesce(): Promise<void> {
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()
})
})
@@ -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>): void {
@@ -51,8 +57,7 @@ export function useBagTotalsPing(enabled: boolean, reload: () => Promise<void>):
})
}
const handler = (event: Record<string, unknown>) => {
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<void>):
}, COALESCE_MS)
}
const handler = (event: Record<string, unknown>) => {
// '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])
+2 -2
View File
@@ -109,7 +109,7 @@ export interface PluginContext {
/** A trip's packing items (hydrated bags/assignees). Needs `db:read:packing`. */
list(tripId: number): Promise<PackingItem[]>;
/** 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<PackingItem>;
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<PackingItem>;
/** Update a packing item. Needs `db:write:packing` + packing_edit. */
update(tripId: number, itemId: number, input: Record<string, unknown>): Promise<PackingItem>;
/** 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<unknown[]>;
createBag(tripId: number, input: { name: string; color?: string }): Promise<unknown>;
createBag(tripId: number, input: { name: string; color?: string; weight_limit_grams?: number }): Promise<unknown>;
updateBag(tripId: number, bagId: number, input: Record<string, unknown>): Promise<unknown>;
deleteBag(tripId: number, bagId: number): Promise<{ deleted: boolean }>;
setBagMembers(tripId: number, bagId: number, userIds: number[]): Promise<unknown>;
+3
View File
@@ -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 });
}
@@ -91,7 +91,7 @@ export interface PluginContext {
/** A trip's packing items (hydrated bags/assignees). Needs 'db:read:packing'. */
list(tripId: number): Promise<unknown[]>;
/** 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<unknown>;
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<unknown>;
/** Update a packing item. Needs 'db:write:packing' + 'packing_edit'. */
update(tripId: number, itemId: number, input: Record<string, unknown>): Promise<unknown>;
/** 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<unknown[]>;
createBag(tripId: number, input: { name: string; color?: string }): Promise<unknown>;
createBag(tripId: number, input: { name: string; color?: string; weight_limit_grams?: number }): Promise<unknown>;
updateBag(tripId: number, bagId: number, input: Record<string, unknown>): Promise<unknown>;
deleteBag(tripId: number, bagId: number): Promise<{ deleted: boolean }>;
setBagMembers(tripId: number, bagId: number, userIds: number[]): Promise<unknown>;
@@ -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);