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