From 98b97edb00e764a43d30d7b925f886d8834a66a3 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 12:40:50 +0000 Subject: [PATCH] fix(packing,journey,server): address review findings on the three fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adversarial review of the diff found sixteen defects that survived verification. The substantive ones: - The new `packing:bag-totals` event was never classified in wsEventPolicy, so the registry-parity ratchet was red. It belongs in HANDLED_OUTSIDE_TRIP_STORE: it carries no state to apply, which is the point of it. - The Journey Settings tracks switch called onSaved(), which closes the dialog. That destroyed the switch the moment it was flipped AND skipped the unsaved-changes guard, so a title typed but not saved was silently lost. It refreshes instead. - Deleting a bag sends no totals ping, yet packing_items.bag_id is ON DELETE SET NULL: everything in the bag lands in the unassigned pile and both figures move. The comment claiming otherwise was wrong. - Only the REST handlers pinged. The MCP tools and the plugin RPC are two more live write paths, and since the displayed number now comes from the server rather than from the item events, weights simply stopped updating for anything written through them. - Every item write pinged, including checking an item off — the most frequent packing write there is — and clearing twelve checked items sent twelve. The ping now fires only when a weight-relevant field changed, and the clients coalesce bursts and never overlap two refetches. - Bags have no repo and no Dexie table, so offline the server totals froze at the last online read and went blind to the mutation queue. A stale absolute number measured against an airline limit is worse than an honest partial one, so offline the surfaces sum what they can see. - SIGTERM arriving mid-bootstrap called server.close() on undefined, and the fire-and-forget promise turned that into an unhandled rejection: on Node 22 a crash, which is the opposite of the fix. - Promise.allSettled swallowed a rejected nestApp.close(), reporting a clean exit 0 while the crons and the plugin children were never torn down. This bug class is diagnosed from container logs. Also: the track fetch gate read a stale `current` during journey-to-journey navigation and fetched the previous journey's tracks; listBags ran the weight aggregate twice per request, on a route the ping now fires constantly; the grand total counted unassigned weight whose row was not rendered; the client declared show_trip_tracks boolean while the wire carries SQLite 0|1; and the six broadcast calls had no test, so removing all of them left the suite green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01S5VwmiBQ9sPCJwfmpv64r3 --- client/src/api/wsEventPolicy.ts | 5 ++ .../JourneyDetailPageSettingsDialog.tsx | 5 +- client/src/components/Journey/JourneyMap.tsx | 9 ++- .../Packing/PackingListPanelBagModal.tsx | 13 ++-- .../Packing/PackingListPanelBagSidebar.tsx | 12 ++-- .../Packing/packingListPanel.helpers.ts | 23 ++++-- .../components/Packing/useBagTotalsPing.ts | 70 +++++++++++++++++++ .../components/Packing/usePackingListPanel.ts | 40 ++++++----- .../mobile/screens/trip/tabs/MBagsSheet.tsx | 11 +-- .../screens/trip/tabs/MPackingListTab.tsx | 34 ++++----- .../pages/journeyDetail/useJourneyDetail.ts | 7 +- client/src/store/journeyStore.ts | 6 +- server/src/nest/packing/packing.controller.ts | 15 ++-- server/src/nest/packing/packing.mcp.ts | 8 +++ server/src/nest/packing/packing.rpc.ts | 7 ++ server/src/nest/packing/packing.service.ts | 26 +++++-- server/src/shutdown.ts | 10 ++- .../unit/nest/packing.controller.test.ts | 50 ++++++++++++- .../tests/unit/nest/packing.service.test.ts | 19 +++++ server/tests/unit/shutdown.test.ts | 14 ++++ 20 files changed, 311 insertions(+), 73 deletions(-) create mode 100644 client/src/components/Packing/useBagTotalsPing.ts diff --git a/client/src/api/wsEventPolicy.ts b/client/src/api/wsEventPolicy.ts index f89b6dec6..5b974a0a0 100644 --- a/client/src/api/wsEventPolicy.ts +++ b/client/src/api/wsEventPolicy.ts @@ -30,6 +30,11 @@ export const HANDLED_OUTSIDE_TRIP_STORE = [ // In-app notifications — hooks/useInAppNotificationListener 'notification:new', 'notification:updated', + // Bag weights (#2191) — content-free ping; components/Packing/usePackingListPanel + // and mobile/screens/trip/tabs/MPackingListTab re-read the bags on it. It carries + // no state to apply, which is the point: the totals are summed server-side across + // members whose items this client may not see. + 'packing:bag-totals', // Collections — pages/collections/useCollections ('collections:' prefix listener) 'collections:updated', 'collections:accepted', diff --git a/client/src/components/Journey/JourneyDetailPageSettingsDialog.tsx b/client/src/components/Journey/JourneyDetailPageSettingsDialog.tsx index 990e8e924..b0617445a 100644 --- a/client/src/components/Journey/JourneyDetailPageSettingsDialog.tsx +++ b/client/src/components/Journey/JourneyDetailPageSettingsDialog.tsx @@ -84,7 +84,10 @@ export function JourneySettingsDialog({ journey, onClose, onSaved, onOpenInvite, setSavingTracks(true) try { await updateJourney(journey.id, { show_trip_tracks: !journey.show_trip_tracks }) - onSaved() + // onRefresh, not onSaved: onSaved closes the dialog, which would destroy the + // switch the moment it is flipped AND skip handleClose's unsaved-changes + // guard, silently dropping a title the owner had typed but not saved yet. + onRefresh() } catch { toast.error(t('journey.settings.saveFailed')) } finally { diff --git a/client/src/components/Journey/JourneyMap.tsx b/client/src/components/Journey/JourneyMap.tsx index 98f2d32bb..a11828db5 100644 --- a/client/src/components/Journey/JourneyMap.tsx +++ b/client/src/components/Journey/JourneyMap.tsx @@ -276,8 +276,13 @@ function JourneyMap( * trail between them. Letting a recorded GPX into the bounds meant one drive * across a country zoomed the map out until the entries were specks, which * is what the reporter's screenshot shows. JourneyMapGL already fits on - * entries + trail alone (JourneyMapGL.tsx), so this also ends an asymmetry - * where the same journey framed differently per renderer. + * entries + trail alone, so for every journey that has entries the two + * renderers now agree where they used to differ. + * + * They still differ for a journey with tracks and nothing else: this one + * frames the tracks, JourneyMapGL falls back to the world view. Framing the + * only thing on the map is the better of the two, and converging the GL side + * is a change to a renderer this issue is not about. */ const trackCoords: L.LatLngTuple[] = [] diff --git a/client/src/components/Packing/PackingListPanelBagModal.tsx b/client/src/components/Packing/PackingListPanelBagModal.tsx index 67411dbfb..cf95195ab 100644 --- a/client/src/components/Packing/PackingListPanelBagModal.tsx +++ b/client/src/components/Packing/PackingListPanelBagModal.tsx @@ -6,7 +6,7 @@ import { BagCard } from './PackingListPanelBagCard' export function BagModal(S: PackingState) { const { setShowBagModal, t, bags, items, tripId, tripMembers, canEdit, currentUserId, handleDeleteBag, handleUpdateBag, handleSetBagMembers, - showAddBag, setShowAddBag, newBagName, setNewBagName, handleCreateBag, unassignedWeightGrams, + showAddBag, setShowAddBag, newBagName, setNewBagName, handleCreateBag, unassignedWeightGrams, serverWeightsFresh, } = S // The ITEM LISTS still describe what you are carrying — an item someone shared // with you stays in your list, but they are the one bringing it (#1767). @@ -14,7 +14,7 @@ export function BagModal(S: PackingState) { // The WEIGHTS no longer do. A bag's load is the bag's, whoever packed it and // whether or not you may see the items, so it comes from the server (#2191). const bagWeightOf = (bag: typeof bags[number]) => - bagTotalWeight(bag, myItems.filter(i => i.bag_id === bag.id)) + bagTotalWeight(bag, myItems.filter(i => i.bag_id === bag.id), serverWeightsFresh) // Reference for bags without a limit of their own — computed once instead of per bag. const heaviestBagWeight = Math.max(...bags.map(bagWeightOf), 1) return ( @@ -44,8 +44,11 @@ export function BagModal(S: PackingState) { {/* Unassigned */} {(() => { const unassigned = myItems.filter(i => !i.bag_id) - const unassignedWeight = unassignedTotalWeight(unassignedWeightGrams, unassigned) - if (unassigned.length === 0) return null + const unassignedWeight = unassignedTotalWeight(unassignedWeightGrams, unassigned, serverWeightsFresh) + // Shown whenever there is weight to account for, even with no visible + // items: the grand total counts it, and a total nothing adds up to is + // the confusion this issue was about (#2191). + if (unassigned.length === 0 && unassignedWeight === 0) return null return (
@@ -68,7 +71,7 @@ export function BagModal(S: PackingState) { // Same rule as the rows above it: a grand total mixing true bag // weights with a per-viewer remainder would be worse than either. const w = bags.reduce((s, b) => s + bagWeightOf(b), 0) - + unassignedTotalWeight(unassignedWeightGrams, myItems.filter(i => !i.bag_id)) + + unassignedTotalWeight(unassignedWeightGrams, myItems.filter(i => !i.bag_id), serverWeightsFresh) return w >= 1000 ? `${(w / 1000).toFixed(1)} kg` : `${w} g` })()}
diff --git a/client/src/components/Packing/PackingListPanelBagSidebar.tsx b/client/src/components/Packing/PackingListPanelBagSidebar.tsx index affe5ce26..a9018efcb 100644 --- a/client/src/components/Packing/PackingListPanelBagSidebar.tsx +++ b/client/src/components/Packing/PackingListPanelBagSidebar.tsx @@ -6,14 +6,14 @@ import { BagCard } from './PackingListPanelBagCard' export function BagSidebar(S: PackingState) { const { t, bags, items, tripId, tripMembers, canEdit, currentUserId, handleDeleteBag, handleUpdateBag, handleSetBagMembers, - showAddBag, setShowAddBag, newBagName, setNewBagName, handleCreateBag, unassignedWeightGrams, + showAddBag, setShowAddBag, newBagName, setNewBagName, handleCreateBag, unassignedWeightGrams, serverWeightsFresh, } = S // The ITEM LISTS still describe what you are carrying — an item someone shared // with you stays in your list, but they are the one bringing it (#1767). const myItems = items.filter(i => countsTowardsMyLoad(i, currentUserId)) // The WEIGHTS no longer do: a bag's load is the bag's, whoever packed it (#2191). const bagWeightOf = (bag: typeof bags[number]) => - bagTotalWeight(bag, myItems.filter(i => i.bag_id === bag.id)) + bagTotalWeight(bag, myItems.filter(i => i.bag_id === bag.id), serverWeightsFresh) // Reference for bags without a limit of their own — computed once instead of per bag. const heaviestBagWeight = Math.max(...bags.map(bagWeightOf), 1) return ( @@ -34,8 +34,10 @@ export function BagSidebar(S: PackingState) { {/* Unassigned */} {(() => { const unassigned = myItems.filter(i => !i.bag_id) - const unassignedWeight = unassignedTotalWeight(unassignedWeightGrams, unassigned) - if (unassigned.length === 0) return null + const unassignedWeight = unassignedTotalWeight(unassignedWeightGrams, unassigned, serverWeightsFresh) + // Shown whenever there is weight to account for, even with no visible + // items — the grand total counts it either way (#2191). + if (unassigned.length === 0 && unassignedWeight === 0) return null return (
@@ -57,7 +59,7 @@ export function BagSidebar(S: PackingState) { {(() => { // Same rule as the rows above it (#2191). const w = bags.reduce((s, b) => s + bagWeightOf(b), 0) - + unassignedTotalWeight(unassignedWeightGrams, myItems.filter(i => !i.bag_id)) + + unassignedTotalWeight(unassignedWeightGrams, myItems.filter(i => !i.bag_id), serverWeightsFresh) return w >= 1000 ? `${(w / 1000).toFixed(1)} kg` : `${w} g` })()}
diff --git a/client/src/components/Packing/packingListPanel.helpers.ts b/client/src/components/Packing/packingListPanel.helpers.ts index c4e3e6341..3d25ff5fc 100644 --- a/client/src/components/Packing/packingListPanel.helpers.ts +++ b/client/src/components/Packing/packingListPanel.helpers.ts @@ -38,23 +38,34 @@ export const countsTowardsMyLoad = ( * could only ever produce the part of the bag the viewer is allowed to look at, * which is the wrong number to measure against an airline's weight limit. * - * The local sum stays as the fallback for exactly one case: a bag that reached - * the client without the field, i.e. an offline Dexie row cached before #2191. - * Note `?? 0` semantics — an empty bag legitimately weighs 0 and must not fall - * through to the local sum. + * The local sum is the fallback in two cases: a bag that reached the client + * without the field (a row cached before #2191), and `serverFresh === false`, + * which the surfaces pass while offline — bags have no repo and no Dexie table, + * so their totals are frozen at the last online read and blind to whatever the + * mutation queue is holding. A stale absolute number measured against an + * airline limit is worse than an honest partial one. + * + * Note the explicit null check rather than `??` on a falsy value: an empty bag + * legitimately weighs 0 and must not fall through to the local sum. */ export const bagTotalWeight = ( bag: { total_weight_grams?: number | null }, visibleItems: { weight_grams?: number | null; quantity?: number | null }[], + serverFresh = true, ): number => - bag.total_weight_grams ?? visibleItems.reduce((sum, i) => sum + itemWeight(i), 0) + serverFresh && bag.total_weight_grams != null + ? bag.total_weight_grams + : visibleItems.reduce((sum, i) => sum + itemWeight(i), 0) /** The same rule for the pile that is in no bag (#2191). */ export const unassignedTotalWeight = ( serverTotal: number | null | undefined, visibleItems: { weight_grams?: number | null; quantity?: number | null }[], + serverFresh = true, ): number => - serverTotal ?? visibleItems.reduce((sum, i) => sum + itemWeight(i), 0) + serverFresh && serverTotal != null + ? serverTotal + : visibleItems.reduce((sum, i) => sum + itemWeight(i), 0) /** * How full a bag's bar reads. A bag with a weight limit is measured against that limit — diff --git a/client/src/components/Packing/useBagTotalsPing.ts b/client/src/components/Packing/useBagTotalsPing.ts new file mode 100644 index 000000000..f454f9b4f --- /dev/null +++ b/client/src/components/Packing/useBagTotalsPing.ts @@ -0,0 +1,70 @@ +import { useEffect, useRef } from 'react' +import { addListener, removeListener } from '../../api/websocket' + +/** + * How long pings are collected before one refetch goes out. + * + * Long enough to fold a burst into a single request — clearing twelve checked + * items issues twelve deletes, and an import writes a whole list — short enough + * that a single edit still reads as instant. + */ +const COALESCE_MS = 250 + +/** + * Re-read the bag weights when the server says they moved (#2191). + * + * The ping is content-free by design: totals are summed across every member, + * including people whose items this client may not see, so the event cannot + * carry the new numbers without leaking what produced them. That makes every + * ping a refetch, which is why this coalesces bursts and never lets two + * requests overlap — the server pings the whole room, the originating socket + * included, so a busy trip would otherwise have every client refetching once + * per written item. + * + * `reload` may change identity every render; the latest one is always used. + */ +export function useBagTotalsPing(enabled: boolean, reload: () => Promise): void { + const reloadRef = useRef(reload) + reloadRef.current = reload + + useEffect(() => { + if (!enabled) return + let timer: ReturnType | null = null + let inFlight = false + /** A ping that arrived while a refetch was running: serve it once that lands. */ + let pendingAfterFlight = false + let cancelled = false + + const run = (): void => { + if (cancelled) return + if (inFlight) { + pendingAfterFlight = true + return + } + inFlight = true + void reloadRef.current().finally(() => { + inFlight = false + if (pendingAfterFlight && !cancelled) { + pendingAfterFlight = false + run() + } + }) + } + + const handler = (event: Record) => { + if (event.type !== 'packing:bag-totals') return + if (timer) return // already collecting this burst + timer = setTimeout(() => { + timer = null + run() + }, COALESCE_MS) + } + + addListener(handler) + return () => { + cancelled = true + removeListener(handler) + if (timer) clearTimeout(timer) + } + }, [enabled]) +} diff --git a/client/src/components/Packing/usePackingListPanel.ts b/client/src/components/Packing/usePackingListPanel.ts index 12cfe8430..95b774b1c 100644 --- a/client/src/components/Packing/usePackingListPanel.ts +++ b/client/src/components/Packing/usePackingListPanel.ts @@ -7,7 +7,8 @@ import { useToast } from '../shared/Toast' import { useTranslation } from '../../i18n' import { packingApi, tripsApi } from '../../api/client' import { useAddonStore } from '../../store/addonStore' -import { addListener, removeListener } from '../../api/websocket' +import { useNetworkMode } from '../../hooks/useNetworkMode' +import { useBagTotalsPing } from './useBagTotalsPing' import type { PackingItem, PackingBag } from '../../types' import { BAG_COLORS, PACKING_PLACEHOLDER_NAME } from './packingListPanel.constants' import { parseImportLines } from './packingListPanel.helpers' @@ -221,31 +222,34 @@ export function usePackingList({ tripId, items, openImportSignal = 0, clearCheck if (!addonsLoaded) loadAddons() }, [addonsLoaded, loadAddons]) - const reloadBags = useCallback(() => { + const reloadBags = useCallback(async () => { if (!bagTrackingEnabled) return - packingApi.listBags(tripId) - .then(r => { - setBags(r.bags || []) - setUnassignedWeightGrams(r.unassigned_weight_grams ?? null) - }) - .catch(() => {}) + try { + const r = await packingApi.listBags(tripId) + setBags(r.bags || []) + setUnassignedWeightGrams(r.unassigned_weight_grams ?? null) + } catch { + // Offline or a failed read: the surfaces fall back to the local sum + // (see `serverWeightsFresh` below), so there is nothing to roll back. + } }, [tripId, bagTrackingEnabled]) - useEffect(() => { reloadBags() }, [reloadBags]) + useEffect(() => { void reloadBags() }, [reloadBags]) // Bag weights are summed server-side across every member (#2191), so an item // this viewer may not even see still moves them. The item events cannot carry // that — a private item is delivered only to its owner, which is the very rule // that made the totals wrong — so the server pings the room content-free and // we re-read the numbers. - useEffect(() => { - if (!bagTrackingEnabled) return - const handler = (event: Record) => { - if (event.type === 'packing:bag-totals') reloadBags() - } - addListener(handler) - return () => removeListener(handler) - }, [bagTrackingEnabled, reloadBags]) + useBagTotalsPing(bagTrackingEnabled, reloadBags) + + // Bags are not part of the offline cache (no repo, no Dexie table), so while + // offline the server totals are frozen at the last online read and cannot see + // the optimistic item writes the mutation queue is holding. A stale absolute + // number measured against an airline limit is worse than an honest partial + // one, so offline the surfaces sum what they can see instead (#2191). + const { offline } = useNetworkMode() + const serverWeightsFresh = !offline const handleCreateBag = async () => { if (!newBagName.trim()) return @@ -398,7 +402,7 @@ export function usePackingList({ tripId, items, openImportSignal = 0, clearCheck filter, setFilter, addingCategory, setAddingCategory, newCatName, setNewCatName, tripMembers, categoryAssignees, handleSetAssignees, allCategories, gruppiert, abgehakt, fortschritt, handleAddItemToCategory, handleAddNewCategory, handleRenameCategory, handleDeleteCategory, handleDeleteItem, handleClearChecked, - bagTrackingEnabled, bags, unassignedWeightGrams, newBagName, setNewBagName, showAddBag, setShowAddBag, showBagModal, setShowBagModal, + bagTrackingEnabled, bags, unassignedWeightGrams, serverWeightsFresh, newBagName, setNewBagName, showAddBag, setShowAddBag, showBagModal, setShowBagModal, handleCreateBag, handleCreateBagByName, handleDeleteBag, handleUpdateBag, handleSetBagMembers, availableTemplates, showTemplateDropdown, setShowTemplateDropdown, applyingTemplate, showSaveTemplate, setShowSaveTemplate, saveTemplateName, setSaveTemplateName, diff --git a/client/src/mobile/screens/trip/tabs/MBagsSheet.tsx b/client/src/mobile/screens/trip/tabs/MBagsSheet.tsx index 233489d55..186934d0b 100644 --- a/client/src/mobile/screens/trip/tabs/MBagsSheet.tsx +++ b/client/src/mobile/screens/trip/tabs/MBagsSheet.tsx @@ -16,6 +16,8 @@ export interface MBagsSheetProps { bags: PackingBag[] /** Server-summed weight of everything in no bag (#2191); null when unknown. */ unassignedWeightGrams?: number | null + /** False while offline, when the server totals are frozen and blind to queued writes. */ + serverWeightsFresh?: boolean items: PackingItem[] tripMembers: TripMember[] canEdit: boolean @@ -33,7 +35,7 @@ export interface MBagsSheetProps { * is the caller's business — this sheet just renders whatever bags it's given. */ export default function MBagsSheet({ - planner, open, onClose, bags, items, unassignedWeightGrams, tripMembers, canEdit, currentUserId, onCreateBag, onUpdateBag, onDeleteBag, onSetBagMembers, + planner, open, onClose, bags, items, unassignedWeightGrams, serverWeightsFresh = true, tripMembers, canEdit, currentUserId, onCreateBag, onUpdateBag, onDeleteBag, onSetBagMembers, }: MBagsSheetProps) { const { t } = planner const [addingBag, setAddingBag] = useState(false) @@ -44,9 +46,9 @@ export default function MBagsSheet({ const myItems = items.filter(i => countsTowardsMyLoad(i, currentUserId)) // The WEIGHTS no longer do: a bag's load is the bag's, whoever packed it (#2191). const bagWeightOf = (bag: PackingBag) => - bagTotalWeight(bag, myItems.filter(i => i.bag_id === bag.id)) + bagTotalWeight(bag, myItems.filter(i => i.bag_id === bag.id), serverWeightsFresh) const unassigned = myItems.filter(i => !i.bag_id) - const unassignedWeight = unassignedTotalWeight(unassignedWeightGrams, unassigned) + const unassignedWeight = unassignedTotalWeight(unassignedWeightGrams, unassigned, serverWeightsFresh) const totalWeight = bags.reduce((s, b) => s + bagWeightOf(b), 0) + unassignedWeight // Reference for bags without a limit of their own — computed once instead of per bag. const heaviestBagWeight = Math.max(...bags.map(bagWeightOf), 1) @@ -84,7 +86,8 @@ export default function MBagsSheet({ ) })} - {unassigned.length > 0 && ( + {/* Weight with no visible items still gets a row: the total counts it (#2191). */} + {(unassigned.length > 0 || unassignedWeight > 0) && (
diff --git a/client/src/mobile/screens/trip/tabs/MPackingListTab.tsx b/client/src/mobile/screens/trip/tabs/MPackingListTab.tsx index 3e633a2fd..a0ccea717 100644 --- a/client/src/mobile/screens/trip/tabs/MPackingListTab.tsx +++ b/client/src/mobile/screens/trip/tabs/MPackingListTab.tsx @@ -9,7 +9,8 @@ import { useAuthStore } from '../../../../store/authStore' import { useAddonStore } from '../../../../store/addonStore' import { useTripStore } from '../../../../store/tripStore' import { packingApi } from '../../../../api/client' -import { addListener, removeListener } from '../../../../api/websocket' +import { useNetworkMode } from '../../../../hooks/useNetworkMode' +import { useBagTotalsPing } from '../../../../components/Packing/useBagTotalsPing' import type { PackingUpdateBagRequest } from '@trek/shared' import type { PackingBag, PackingItem, TripMember } from '../../../../types' import type { TripPlanner } from '../MTripShell' @@ -68,28 +69,26 @@ export default function MPackingListTab({ planner }: { planner: TripPlanner }) { const [templates, setTemplates] = useState([]) const [categoryAssignees, setCategoryAssignees] = useState>({}) - const reloadBags = useCallback(() => { + const reloadBags = useCallback(async () => { if (!bagTrackingEnabled) return - packingApi.listBags(tripId) - .then(r => { - setBags(r.bags || []) - setUnassignedWeightGrams(r.unassigned_weight_grams ?? null) - }) - .catch(() => {}) + try { + const r = await packingApi.listBags(tripId) + setBags(r.bags || []) + setUnassignedWeightGrams(r.unassigned_weight_grams ?? null) + } catch { + // Offline or a failed read: the sheet falls back to the local sum. + } }, [tripId, bagTrackingEnabled]) - useEffect(() => { reloadBags() }, [reloadBags]) + useEffect(() => { void reloadBags() }, [reloadBags]) // Bag weights are server-summed across every member (#2191), so an item this // viewer cannot see still moves them; the server pings the room content-free. - useEffect(() => { - if (!bagTrackingEnabled) return - const handler = (event: Record) => { - if (event.type === 'packing:bag-totals') reloadBags() - } - addListener(handler) - return () => removeListener(handler) - }, [bagTrackingEnabled, reloadBags]) + useBagTotalsPing(bagTrackingEnabled, reloadBags) + + // Bags have no offline cache, so their totals go stale the moment the device + // does; offline the sheet sums what it can see instead (#2191). + const { offline } = useNetworkMode() useEffect(() => { packingApi.listTemplates(tripId).then(r => setTemplates(r.templates || [])).catch(() => {}) @@ -492,6 +491,7 @@ export default function MPackingListTab({ planner }: { planner: TripPlanner }) { bags={bags} items={items} unassignedWeightGrams={unassignedWeightGrams} + serverWeightsFresh={!offline} tripMembers={tripMembers} canEdit={canEdit} currentUserId={currentUserId} diff --git a/client/src/pages/journeyDetail/useJourneyDetail.ts b/client/src/pages/journeyDetail/useJourneyDetail.ts index 27174b1d1..d2f655522 100644 --- a/client/src/pages/journeyDetail/useJourneyDetail.ts +++ b/client/src/pages/journeyDetail/useJourneyDetail.ts @@ -85,7 +85,12 @@ export function useJourneyDetail() { // for. It stays untouched on the server so TREK Studio, which freezes the // same endpoint's output into a saved book, keeps the set it laid out with. const [tracks, setTracks] = useState(NO_TRACKS) - const showTripTracks = !!current?.show_trip_tracks + // Gate on the journey the route actually names: loadJourney leaves the previous + // `current` in place while the next one loads, so reading the flag unguarded + // would fetch the tracks of journey A under the id of journey B — the exact + // megabyte-sized request the switch exists to avoid. + const journeyLoaded = current?.id === Number(id) + const showTripTracks = journeyLoaded && !!current?.show_trip_tracks useEffect(() => { if (!id || !showTripTracks) { // A stable empty array: a fresh literal here would be a new reference on diff --git a/client/src/store/journeyStore.ts b/client/src/store/journeyStore.ts index 5a81b3023..bde04a5ed 100644 --- a/client/src/store/journeyStore.ts +++ b/client/src/store/journeyStore.ts @@ -15,8 +15,12 @@ export interface Journey { * Draw the GPX tracks of the journey's linked trips on its map (#2194). * Off unless the owner asks for it — see the migration for why the default * flipped. Optional so a cached pre-#2194 journey still parses. + * + * Typed as the wire shape, not as the concept: the column is INTEGER and the + * journey rows are read with SELECT *, so what arrives is 0 or 1. Declaring it + * boolean would make `=== true` compile and never hold. */ - show_trip_tracks?: boolean + show_trip_tracks?: number created_at: number updated_at: number } diff --git a/server/src/nest/packing/packing.controller.ts b/server/src/nest/packing/packing.controller.ts index 573adb6a2..fd9485c5c 100644 --- a/server/src/nest/packing/packing.controller.ts +++ b/server/src/nest/packing/packing.controller.ts @@ -151,7 +151,12 @@ export class PackingController { throw new HttpException({ error: 'Bag not found' }, 400); } this.packing.broadcastUpdate(tripId, id, updated as PackingItemRow, !!before?.is_private, socketId); - this.packing.broadcastBagTotals(tripId); + // Only when the write could actually move a weight. Checking an item off is + // the most frequent packing write there is, and every ping costs every + // connected client a listBags round trip. + if (['weight_grams', 'quantity', 'bag_id'].some(k => Object.keys(body).includes(k))) { + this.packing.broadcastBagTotals(tripId); + } return { item: updated }; } @@ -256,10 +261,7 @@ export class PackingController { // unassigned_weight_grams rides along so the "no bag" pile and the grand // total follow the same rule as the bags themselves (#2191) — a screen // mixing true totals with per-viewer ones would be worse than either. - return { - bags: this.packing.listBags(tripId), - unassigned_weight_grams: this.packing.unassignedWeightGrams(tripId), - }; + return this.packing.listBagsWithWeights(tripId); } @RequirePermission('packing_edit') @@ -311,6 +313,9 @@ export class PackingController { throw new HttpException({ error: 'Bag not found' }, 404); } this.packing.broadcast(tripId, 'packing:bag-deleted', { bagId: Number(bagId) }, socketId); + // bag_id is ON DELETE SET NULL, so everything that was in it just landed in + // the unassigned pile — both figures moved. + this.packing.broadcastBagTotals(tripId); return { success: true }; } diff --git a/server/src/nest/packing/packing.mcp.ts b/server/src/nest/packing/packing.mcp.ts index 742f61207..cefa8082b 100644 --- a/server/src/nest/packing/packing.mcp.ts +++ b/server/src/nest/packing/packing.mcp.ts @@ -99,6 +99,7 @@ export class PackingMcp { // A restricted item (#858) reaches its owner and recipients only; a Common // one answers null here and goes to the whole room. this.guards.safeBroadcast(tripId, 'packing:created', { item }, this.packing.viewersOf(item)); + this.packing.broadcastBagTotals(String(tripId)); return ok({ item }); } @@ -146,6 +147,7 @@ export class PackingMcp { // deleteItem hands back the row it removed, so the delete can be scoped to // the same people the item was ever visible to (#1976). this.guards.safeBroadcast(tripId, 'packing:deleted', { itemId }, this.packing.viewersOf(deleted)); + this.packing.broadcastBagTotals(String(tripId)); return ok({ success: true }); } @@ -207,6 +209,7 @@ export class PackingMcp { } // Newly common: the members who never had the row need it created, not updated. if (wasPrivate) this.guards.safeBroadcast(tripId, 'packing:created', { item }); + this.packing.broadcastBagTotals(String(tripId)); this.guards.safeBroadcast(tripId, 'packing:updated', { item }); } @@ -237,6 +240,7 @@ export class PackingMcp { // it back to whoever may see it now, as the REST route does. this.guards.safeBroadcast(tripId, 'packing:deleted', { itemId }); this.guards.safeBroadcast(tripId, 'packing:created', { item }, this.packing.viewersOf(item)); + this.packing.broadcastBagTotals(String(tripId)); return ok({ item }); } @@ -439,6 +443,7 @@ export class PackingMcp { const items = this.packing.applyTemplate(tripId, templateId); if (items === null) return errorResult('Template not found.'); this.guards.safeBroadcast(tripId, 'packing:template-applied', { items }); + this.packing.broadcastBagTotals(String(tripId)); return ok({ items, count: items.length }); } @@ -527,6 +532,9 @@ export class PackingMcp { for (const item of created) { this.guards.safeBroadcast(tripId, 'packing:created', { item }, this.packing.viewersOf(item)); } + // Once for the whole import, not once per item: the ping is content-free and + // every one of them costs each connected client a listBags round trip. + this.packing.broadcastBagTotals(String(tripId)); return ok({ items: created, count: created.length }); } diff --git a/server/src/nest/packing/packing.rpc.ts b/server/src/nest/packing/packing.rpc.ts index c59a08068..a09067e46 100644 --- a/server/src/nest/packing/packing.rpc.ts +++ b/server/src/nest/packing/packing.rpc.ts @@ -51,6 +51,7 @@ export class PackingRpc { // A referenced bag must exist on this trip (#2154), as on the REST route. if (isInvalidBagRef(item)) throw new BadParams(`no packing bag ${parsed.data.bag_id} on trip ${tripId}`); this.packing.emitToViewers(String(tripId), 'packing:created', { item }, item, undefined); + this.packing.broadcastBagTotals(String(tripId)); return item; } @@ -71,6 +72,9 @@ export class PackingRpc { // A referenced bag must exist on this trip (#2154), as on the REST route. if (isInvalidBagRef(updated)) throw new BadParams(`no packing bag ${parsed.data.bag_id} on trip ${tripId}`); this.packing.broadcastUpdate(String(tripId), itemId, updated as PrivacyItem, !!before?.is_private, undefined); + if (['weight_grams', 'quantity', 'bag_id'].some(k => Object.keys(input).includes(k))) { + this.packing.broadcastBagTotals(String(tripId)); + } return updated; } @@ -83,6 +87,7 @@ export class PackingRpc { const deleted = this.packing.deleteItem(String(tripId), String(itemId), actor) as PrivacyItem | null; if (!deleted) throw new ForbiddenResource(`no packing item ${itemId} on trip ${tripId}`); this.packing.emitToViewers(String(tripId), 'packing:deleted', { itemId }, deleted, undefined); + this.packing.broadcastBagTotals(String(tripId)); return { deleted: true }; } @@ -132,6 +137,8 @@ export class PackingRpc { throw new ForbiddenResource(`no packing bag ${bagId} on trip ${tripId}`); } this.realtime.broadcast(tripId, 'packing:bag-deleted', { bagId }, undefined); + // bag_id is ON DELETE SET NULL — its items just moved to the unassigned pile. + this.packing.broadcastBagTotals(String(tripId)); return { deleted: true }; } diff --git a/server/src/nest/packing/packing.service.ts b/server/src/nest/packing/packing.service.ts index 29720312f..0d2616b33 100644 --- a/server/src/nest/packing/packing.service.ts +++ b/server/src/nest/packing/packing.service.ts @@ -120,8 +120,10 @@ export class PackingService { * own client cannot recompute a server-side total from the item it just * wrote either. Everyone refetches, everyone gets numbers. * - * Called for item writes, not bag writes — a bag's own create/update already - * broadcasts the row, and neither changes what is inside it. + * Called for item writes, and for a bag DELETE: packing_items.bag_id is + * ON DELETE SET NULL, so deleting a bag moves everything in it to the + * unassigned pile and moves both figures. A bag create or rename does not, + * and already broadcasts its own row. */ broadcastBagTotals(tripId: string): void { this.broadcast(tripId, 'packing:bag-totals', {}, undefined); @@ -537,9 +539,25 @@ export class PackingService { return this.bagWeightTotals(tripId).get(null) ?? 0; } - listBags(tripId: string | number) { - const bags = this.db.all('SELECT * FROM packing_bags WHERE trip_id = ? ORDER BY sort_order, id', tripId); + /** + * The bags plus the unassigned pile, from ONE pass over the aggregate. + * + * The REST list route wants both, and the WS ping (#2191) makes that route + * fire on every item write for every connected client — running the same + * SUM…GROUP BY twice per request is not a cost worth paying for a nicer + * method list. + */ + listBagsWithWeights(tripId: string | number): { bags: unknown[]; unassigned_weight_grams: number } { const totals = this.bagWeightTotals(tripId); + return { bags: this.decorateBags(tripId, totals), unassigned_weight_grams: totals.get(null) ?? 0 }; + } + + listBags(tripId: string | number) { + return this.decorateBags(tripId, this.bagWeightTotals(tripId)); + } + + private decorateBags(tripId: string | number, totals: Map) { + const bags = this.db.all('SELECT * FROM packing_bags WHERE trip_id = ? ORDER BY sort_order, id', tripId); const members = this.db.all<{ bag_id: number; user_id: number; username: string; avatar: string | null }>(` SELECT bm.bag_id, bm.user_id, COALESCE(u.display_name, u.username) AS username, u.avatar FROM packing_bag_members bm diff --git a/server/src/shutdown.ts b/server/src/shutdown.ts index b93f731d8..55bc1bcaa 100644 --- a/server/src/shutdown.ts +++ b/server/src/shutdown.ts @@ -158,7 +158,15 @@ export async function runShutdown(signal: string, deps: ShutdownDeps): Promise { + // The totals are summed server-side, so nothing else tells the other + // members' screens to re-read them. + const createItem = vi.fn().mockReturnValue({ id: 9, name: 'Socks', is_private: 0 }); + const broadcastBagTotals = vi.fn(); + const svc = makeService({ createItem, broadcastBagTotals } as Partial); + new PackingController(svc).create(user, '5', { name: 'Socks', weight_grams: 250 }, 'sock'); + expect(broadcastBagTotals).toHaveBeenCalledWith('5'); + }); + + it('pings after a delete, and after a bag delete that orphans its items (#2191)', () => { + const deleteItem = vi.fn().mockReturnValue({ id: 9, is_private: 0 }); + const broadcastBagTotals = vi.fn(); + const svc = makeService({ deleteItem, broadcastBagTotals } as Partial); + new PackingController(svc).remove(user, '5', '9', 'sock'); + expect(broadcastBagTotals).toHaveBeenCalledWith('5'); + + // bag_id is ON DELETE SET NULL: the bag's items land in the unassigned pile. + const deleteBag = vi.fn().mockReturnValue(true); + const bagPing = vi.fn(); + const bagSvc = makeService({ deleteBag, broadcastBagTotals: bagPing } as Partial); + new PackingController(bagSvc).deleteBag(user, '5', '7', 'sock'); + expect(bagPing).toHaveBeenCalledWith('5'); + }); + + it('does NOT ping when the update cannot move a weight (#2191)', () => { + // Checking an item off is the most frequent packing write there is, and + // every ping costs each connected client a listBags round trip. + const updateItem = vi.fn().mockReturnValue({ id: 9, is_private: 0 }); + const broadcastBagTotals = vi.fn(); + const svc = makeService({ updateItem, broadcastBagTotals } as Partial); + + new PackingController(svc).update(user, '5', '9', { checked: true }, 'sock'); + expect(broadcastBagTotals).not.toHaveBeenCalled(); + + new PackingController(svc).update(user, '5', '9', { quantity: 3 }, 'sock'); + expect(broadcastBagTotals).toHaveBeenCalledWith('5'); + }); + it('routes a Shared item create only to the owner + recipients (#858)', () => { const item = { id: 9, name: 'Power bank', is_private: 1, owner_id: 1, recipients: [{ user_id: 2 }] }; const createItem = vi.fn().mockReturnValue(item); @@ -319,13 +358,18 @@ describe('PackingController (parity with the legacy /api/trips/:tripId/packing r describe('bags', () => { it('GET /bags lists bags for the trip, with the unassigned weight alongside (#2191)', () => { - const listBags = vi.fn().mockReturnValue([{ id: 3, name: 'Carry-on', total_weight_grams: 800 }]); - const unassignedWeightGrams = vi.fn().mockReturnValue(150); - const svc = makeService({ listBags, unassignedWeightGrams } as Partial); + // One service call, so the SUM…GROUP BY runs once per request — this route + // now fires on every item write for every connected client. + const listBagsWithWeights = vi.fn().mockReturnValue({ + bags: [{ id: 3, name: 'Carry-on', total_weight_grams: 800 }], + unassigned_weight_grams: 150, + }); + const svc = makeService({ listBagsWithWeights } as Partial); expect(new PackingController(svc).listBags(user, '5')).toEqual({ bags: [{ id: 3, name: 'Carry-on', total_weight_grams: 800 }], unassigned_weight_grams: 150, }); + expect(listBagsWithWeights).toHaveBeenCalledTimes(1); }); it('400 on bag create with blank name (bespoke check — the schema cannot see whitespace)', () => { diff --git a/server/tests/unit/nest/packing.service.test.ts b/server/tests/unit/nest/packing.service.test.ts index f8bf4becd..22ae0c0c7 100644 --- a/server/tests/unit/nest/packing.service.test.ts +++ b/server/tests/unit/nest/packing.service.test.ts @@ -746,6 +746,18 @@ describe('create-path fields + bag trip scope (#2154)', () => { expect((svc.listBags(trip.id) as any[]).find(b => b.id === bag.id).total_weight_grams).toBe(0); }); + it('PACK-SVC-080: listBagsWithWeights returns bags and the unassigned pile from one pass (#2191)', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const bag = svc.createBag(trip.id, { name: 'Duffel' }) as any; + svc.createItem(trip.id, { name: 'Tent', weight_grams: 900, bag_id: bag.id }, user.id); + svc.createItem(trip.id, { name: 'Loose sandwich', weight_grams: 150 }, user.id); + + const { bags, unassigned_weight_grams } = svc.listBagsWithWeights(trip.id) as any; + expect(bags[0].total_weight_grams).toBe(900); + expect(unassigned_weight_grams).toBe(150); + }); + it('PACK-SVC-055: createItem refuses a bag off the trip with the invalidBag sentinel, inserting nothing', () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); @@ -803,6 +815,13 @@ describe('broadcast helpers (#858 scoping)', () => { expect(broadcastMock).toHaveBeenCalledWith('5', 'packing:created', { item: 1 }, 'sock'); }); + it('broadcastBagTotals pings the whole room, content-free and without excluding the sender (#2191)', () => { + // No socket exclusion on purpose: the payload carries nothing to echo, and + // the writer cannot recompute a server-side total from its own write either. + svc.broadcastBagTotals('5'); + expect(broadcastMock).toHaveBeenCalledWith('5', 'packing:bag-totals', {}, undefined); + }); + it('broadcastItem broadcasts a shared item to the whole room (no onlyUserId)', () => { svc.broadcastItem('5', 'packing:created', { item: 1 }, { is_private: 0, owner_id: 7 }, 'sock'); expect(broadcastMock).toHaveBeenCalledWith('5', 'packing:created', { item: 1 }, 'sock', undefined); diff --git a/server/tests/unit/shutdown.test.ts b/server/tests/unit/shutdown.test.ts index 135c0377c..ab608db20 100644 --- a/server/tests/unit/shutdown.test.ts +++ b/server/tests/unit/shutdown.test.ts @@ -142,6 +142,20 @@ describe('runShutdown', () => { expect(deps.closeDb).toHaveBeenCalledTimes(1); }); + it('SHUTDOWN-010 says so when Nest teardown fails, and still exits cleanly', async () => { + // allSettled absorbs the rejection; without an explicit report the operator + // would read "Shutdown complete" and exit 0 while the crons and the plugin + // children were never torn down. + const deps = makeDeps({ closeNestApp: vi.fn(async () => { throw new Error('hook boom'); }) }); + + const run = runShutdown('SIGTERM', deps); + (deps.server as unknown as ReturnType).finishClose(); + await run; + + expect(deps.logError).toHaveBeenCalledWith(expect.stringContaining('hook boom')); + expect(deps.exit).toHaveBeenCalledWith(0); + }); + it('SHUTDOWN-009 still exits when the signal beats the bootstrap', async () => { // An orchestrator stopping a container mid-boot: there is no http server yet. // The old path called server.close() on undefined, and the fire-and-forget