From 639c40f8de64d624cbb0697e7b1accc9be6e9dc3 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 12:01:28 +0000 Subject: [PATCH] fix(packing): a bag weighs what everyone put in it, not just what you can see MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bag's weight was summed in the browser over the item list the viewer is allowed to see. Personal items belonging to other members are filtered out of that list by design (#858), so their weight vanished with them: two people looking at the same shared bag saw two different totals, and neither saw the real one. That number is then measured against `weight_limit_grams`, an absolute airline limit. A per-viewer subtotal there is not merely incomplete but wrong — a shared bag could sit over its limit and warn nobody. The sum now happens in SQL over every row and ships as `total_weight_grams` on the bag, with `unassigned_weight_grams` alongside so the "no bag" pile and the grand total follow the same rule; a screen mixing true totals with per-viewer ones would be worse than either. Only integers cross the wire: a member learns that a bag is heavier than the items they can see, never a name, category, quantity or owner. That disclosure is deliberate and bounded, and it is what the issue asks for. The item lists stay viewer-scoped, so #1767 ("someone shared it with you, but they are the one bringing it") is unchanged — it just no longer decides what a bag weighs. Totals also had to stay live, because the reported repro is two people looking at the same screen. A private item's event reaches only its owner, which is the very rule that made the totals wrong, so item writes now also ping the room with a content-free `packing:bag-totals` and everyone re-reads the numbers. All three surfaces — the bag sidebar, the bag modal and the mobile sheet — read the server figure and fall back to the local sum only for a bag cached offline before this change. Closes #2191 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01S5VwmiBQ9sPCJwfmpv64r3 --- .../Packing/PackingListPanelBagModal.tsx | 26 +++++++--- .../Packing/PackingListPanelBagSidebar.tsx | 24 ++++++--- .../Packing/packingListPanel.helpers.test.ts | 27 +++++++++- .../Packing/packingListPanel.helpers.ts | 26 ++++++++++ .../components/Packing/usePackingListPanel.ts | 33 ++++++++++-- .../mobile/screens/trip/tabs/MBagsSheet.tsx | 23 +++++---- .../screens/trip/tabs/MPackingListTab.tsx | 28 +++++++++-- server/src/nest/packing/packing.controller.ts | 14 +++++- server/src/nest/packing/packing.service.ts | 50 +++++++++++++++++++ .../unit/nest/packing.controller.test.ts | 14 ++++-- .../tests/unit/nest/packing.service.test.ts | 49 ++++++++++++++++++ shared/src/packing/packing.schema.ts | 9 ++++ shared/src/realtime/events.schema.spec.ts | 8 +-- shared/src/realtime/events.schema.ts | 11 ++++ 14 files changed, 301 insertions(+), 41 deletions(-) diff --git a/client/src/components/Packing/PackingListPanelBagModal.tsx b/client/src/components/Packing/PackingListPanelBagModal.tsx index 8f43d2dbc..67411dbfb 100644 --- a/client/src/components/Packing/PackingListPanelBagModal.tsx +++ b/client/src/components/Packing/PackingListPanelBagModal.tsx @@ -1,18 +1,22 @@ import { X, Plus } from 'lucide-react' import type { PackingState } from './usePackingListPanel' -import { bagFillPct, countsTowardsMyLoad, itemWeight } from './packingListPanel.helpers' +import { bagFillPct, bagTotalWeight, countsTowardsMyLoad, unassignedTotalWeight } from './packingListPanel.helpers' 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, + showAddBag, setShowAddBag, newBagName, setNewBagName, handleCreateBag, unassignedWeightGrams, } = S - // These numbers describe what you are carrying. An item someone shared with you stays - // in your list, but they are the one bringing it, so it is not your weight (#1767). + // 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 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)) // Reference for bags without a limit of their own — computed once instead of per bag. - const heaviestBagWeight = Math.max(...bags.map(b => myItems.filter(i => i.bag_id === b.id).reduce((s, i) => s + itemWeight(i), 0)), 1) + const heaviestBagWeight = Math.max(...bags.map(bagWeightOf), 1) return (
{ const bagItems = myItems.filter(i => i.bag_id === bag.id) - const totalWeight = bagItems.reduce((sum, i) => sum + itemWeight(i), 0) + const totalWeight = bagWeightOf(bag) const pct = bagFillPct(totalWeight, bag.weight_limit_grams, heaviestBagWeight) return ( handleDeleteBag(bag.id)} onUpdate={handleUpdateBag} onSetMembers={handleSetBagMembers} t={t} /> @@ -40,7 +44,7 @@ export function BagModal(S: PackingState) { {/* Unassigned */} {(() => { const unassigned = myItems.filter(i => !i.bag_id) - const unassignedWeight = unassigned.reduce((s, i) => s + itemWeight(i), 0) + const unassignedWeight = unassignedTotalWeight(unassignedWeightGrams, unassigned) if (unassigned.length === 0) return null return (
@@ -60,7 +64,13 @@ export function BagModal(S: PackingState) {
{t('packing.totalWeight')} - {(() => { const w = myItems.reduce((s, i) => s + itemWeight(i), 0); return w >= 1000 ? `${(w / 1000).toFixed(1)} kg` : `${w} g` })()} + {(() => { + // 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)) + 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 eac5b044d..affe5ce26 100644 --- a/client/src/components/Packing/PackingListPanelBagSidebar.tsx +++ b/client/src/components/Packing/PackingListPanelBagSidebar.tsx @@ -1,18 +1,21 @@ import { Plus } from 'lucide-react' import type { PackingState } from './usePackingListPanel' -import { bagFillPct, countsTowardsMyLoad, itemWeight } from './packingListPanel.helpers' +import { bagFillPct, bagTotalWeight, countsTowardsMyLoad, unassignedTotalWeight } from './packingListPanel.helpers' 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, + showAddBag, setShowAddBag, newBagName, setNewBagName, handleCreateBag, unassignedWeightGrams, } = S - // These numbers describe what you are carrying. An item someone shared with you stays - // in your list, but they are the one bringing it, so it is not your weight (#1767). + // 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)) // Reference for bags without a limit of their own — computed once instead of per bag. - const heaviestBagWeight = Math.max(...bags.map(b => myItems.filter(i => i.bag_id === b.id).reduce((s, i) => s + itemWeight(i), 0)), 1) + const heaviestBagWeight = Math.max(...bags.map(bagWeightOf), 1) return (
@@ -21,7 +24,7 @@ export function BagSidebar(S: PackingState) { {bags.map(bag => { const bagItems = myItems.filter(i => i.bag_id === bag.id) - const totalWeight = bagItems.reduce((sum, i) => sum + itemWeight(i), 0) + const totalWeight = bagWeightOf(bag) const pct = bagFillPct(totalWeight, bag.weight_limit_grams, heaviestBagWeight) return ( handleDeleteBag(bag.id)} onUpdate={handleUpdateBag} onSetMembers={handleSetBagMembers} t={t} compact /> @@ -31,7 +34,7 @@ export function BagSidebar(S: PackingState) { {/* Unassigned */} {(() => { const unassigned = myItems.filter(i => !i.bag_id) - const unassignedWeight = unassigned.reduce((s, i) => s + itemWeight(i), 0) + const unassignedWeight = unassignedTotalWeight(unassignedWeightGrams, unassigned) if (unassigned.length === 0) return null return (
@@ -51,7 +54,12 @@ export function BagSidebar(S: PackingState) {
{t('packing.totalWeight')} - {(() => { const w = myItems.reduce((s, i) => s + itemWeight(i), 0); return w >= 1000 ? `${(w / 1000).toFixed(1)} kg` : `${w} g` })()} + {(() => { + // 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)) + return w >= 1000 ? `${(w / 1000).toFixed(1)} kg` : `${w} g` + })()}
diff --git a/client/src/components/Packing/packingListPanel.helpers.test.ts b/client/src/components/Packing/packingListPanel.helpers.test.ts index d3d33bc5b..f95f5e3f3 100644 --- a/client/src/components/Packing/packingListPanel.helpers.test.ts +++ b/client/src/components/Packing/packingListPanel.helpers.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest' -import { katColor, itemWeight, bagFillPct, countsTowardsMyLoad, parseCsvLine, parseImportLines } from './packingListPanel.helpers' +import { katColor, itemWeight, bagFillPct, bagTotalWeight, countsTowardsMyLoad, parseCsvLine, parseImportLines, unassignedTotalWeight } from './packingListPanel.helpers' import { KAT_COLORS } from './packingListPanel.constants' describe('packingListPanel.helpers', () => { @@ -128,4 +128,29 @@ describe('packingListPanel.helpers', () => { expect(rows[0].name).toBe('Passport') }) }) + + describe('bagTotalWeight / unassignedTotalWeight (#2191)', () => { + it('prefers the server total over anything summable locally', () => { + // The whole point: the local list is privacy-filtered, so it can only ever + // be the part of the bag this viewer is allowed to see. + expect(bagTotalWeight({ total_weight_grams: 1000 }, [{ weight_grams: 800, quantity: 1 }])).toBe(1000) + }) + + it('keeps a server-reported zero instead of falling back to the local sum', () => { + // An empty bag really weighs 0; only an ABSENT field means "not told". + expect(bagTotalWeight({ total_weight_grams: 0 }, [{ weight_grams: 800, quantity: 1 }])).toBe(0) + }) + + it('falls back to the local sum for a bag cached before the field existed', () => { + expect(bagTotalWeight({}, [{ weight_grams: 250, quantity: 3 }, { weight_grams: 50 }])).toBe(800) + expect(bagTotalWeight({ total_weight_grams: null }, [{ weight_grams: 120 }])).toBe(120) + }) + + it('applies the same rule to the unassigned pile', () => { + expect(unassignedTotalWeight(150, [{ weight_grams: 900 }])).toBe(150) + expect(unassignedTotalWeight(0, [{ weight_grams: 900 }])).toBe(0) + expect(unassignedTotalWeight(null, [{ weight_grams: 900 }])).toBe(900) + expect(unassignedTotalWeight(undefined, [])).toBe(0) + }) + }) }) diff --git a/client/src/components/Packing/packingListPanel.helpers.ts b/client/src/components/Packing/packingListPanel.helpers.ts index 659bf94f1..c4e3e6341 100644 --- a/client/src/components/Packing/packingListPanel.helpers.ts +++ b/client/src/components/Packing/packingListPanel.helpers.ts @@ -30,6 +30,32 @@ export const countsTowardsMyLoad = ( return i.owner_id == null || i.owner_id === currentUserId } +/** + * What a bag weighs, as shown (#2191). + * + * The server sums every member's items — including the private ones this viewer + * may not see — and sends the figure on the bag itself. Adding it up locally + * 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. + */ +export const bagTotalWeight = ( + bag: { total_weight_grams?: number | null }, + visibleItems: { weight_grams?: number | null; quantity?: number | null }[], +): number => + 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 }[], +): number => + 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 — * that is the number an airline cares about. Without one there is nothing absolute to diff --git a/client/src/components/Packing/usePackingListPanel.ts b/client/src/components/Packing/usePackingListPanel.ts index f4f388954..12cfe8430 100644 --- a/client/src/components/Packing/usePackingListPanel.ts +++ b/client/src/components/Packing/usePackingListPanel.ts @@ -1,4 +1,4 @@ -import { useState, useMemo, useRef, useEffect } from 'react' +import { useState, useMemo, useRef, useEffect, useCallback } from 'react' import type { ChangeEvent } from 'react' import { useTripStore } from '../../store/tripStore' import { useCanDo } from '../../store/permissionsStore' @@ -7,6 +7,7 @@ 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 type { PackingItem, PackingBag } from '../../types' import { BAG_COLORS, PACKING_PLACEHOLDER_NAME } from './packingListPanel.constants' import { parseImportLines } from './packingListPanel.helpers' @@ -210,6 +211,8 @@ export function usePackingList({ tripId, items, openImportSignal = 0, clearCheck const addonsLoaded = useAddonStore(s => s.loaded) const loadAddons = useAddonStore(s => s.loadAddons) const [bags, setBags] = useState([]) + /** Server-summed weight of everything in no bag (#2191); null until the first load. */ + const [unassignedWeightGrams, setUnassignedWeightGrams] = useState(null) const [newBagName, setNewBagName] = useState('') const [showAddBag, setShowAddBag] = useState(false) const [showBagModal, setShowBagModal] = useState(false) @@ -218,10 +221,32 @@ export function usePackingList({ tripId, items, openImportSignal = 0, clearCheck if (!addonsLoaded) loadAddons() }, [addonsLoaded, loadAddons]) - useEffect(() => { - if (bagTrackingEnabled) packingApi.listBags(tripId).then(r => setBags(r.bags || [])).catch(() => {}) + const reloadBags = useCallback(() => { + if (!bagTrackingEnabled) return + packingApi.listBags(tripId) + .then(r => { + setBags(r.bags || []) + setUnassignedWeightGrams(r.unassigned_weight_grams ?? null) + }) + .catch(() => {}) }, [tripId, bagTrackingEnabled]) + useEffect(() => { 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]) + const handleCreateBag = async () => { if (!newBagName.trim()) return try { @@ -373,7 +398,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, newBagName, setNewBagName, showAddBag, setShowAddBag, showBagModal, setShowBagModal, + bagTrackingEnabled, bags, unassignedWeightGrams, 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 451ac3eec..233489d55 100644 --- a/client/src/mobile/screens/trip/tabs/MBagsSheet.tsx +++ b/client/src/mobile/screens/trip/tabs/MBagsSheet.tsx @@ -6,14 +6,16 @@ import { FIELD_CLS, FormSheetHeader } from '../sheets/PlSheetChrome' import { avatarSrc } from '../../../../utils/avatarSrc' import type { PackingBag, PackingItem, TripMember } from '../../../../types' import type { TripPlanner } from '../MTripShell' -import { formatWeight, packingItemWeight } from './listsModel' -import { bagFillPct, countsTowardsMyLoad } from '../../../../components/Packing/packingListPanel.helpers' +import { formatWeight } from './listsModel' +import { bagFillPct, bagTotalWeight, countsTowardsMyLoad, unassignedTotalWeight } from '../../../../components/Packing/packingListPanel.helpers' export interface MBagsSheetProps { planner: TripPlanner open: boolean onClose: () => void bags: PackingBag[] + /** Server-summed weight of everything in no bag (#2191); null when unknown. */ + unassignedWeightGrams?: number | null items: PackingItem[] tripMembers: TripMember[] canEdit: boolean @@ -31,20 +33,23 @@ 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, tripMembers, canEdit, currentUserId, onCreateBag, onUpdateBag, onDeleteBag, onSetBagMembers, + planner, open, onClose, bags, items, unassignedWeightGrams, tripMembers, canEdit, currentUserId, onCreateBag, onUpdateBag, onDeleteBag, onSetBagMembers, }: MBagsSheetProps) { const { t } = planner const [addingBag, setAddingBag] = useState(false) const [newBagName, setNewBagName] = useState('') - // These numbers describe what you are carrying. An item someone shared with you stays - // in your list, but they are the one bringing it, so it is not your weight (#1767). + // 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: PackingBag) => + bagTotalWeight(bag, myItems.filter(i => i.bag_id === bag.id)) const unassigned = myItems.filter(i => !i.bag_id) - const unassignedWeight = unassigned.reduce((s, i) => s + packingItemWeight(i), 0) - const totalWeight = myItems.reduce((s, i) => s + packingItemWeight(i), 0) + const unassignedWeight = unassignedTotalWeight(unassignedWeightGrams, unassigned) + 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(b => myItems.filter(i => i.bag_id === b.id).reduce((s, i) => s + packingItemWeight(i), 0)), 1) + const heaviestBagWeight = Math.max(...bags.map(bagWeightOf), 1) const submitNewBag = () => { if (!newBagName.trim()) return @@ -60,7 +65,7 @@ export default function MBagsSheet({
{bags.map(bag => { const bagItems = myItems.filter(i => i.bag_id === bag.id) - const bagWeight = bagItems.reduce((s, i) => s + packingItemWeight(i), 0) + const bagWeight = bagWeightOf(bag) const pct = bagFillPct(bagWeight, bag.weight_limit_grams, heaviestBagWeight) return ( (null) const [bags, setBags] = useState([]) + /** Server-summed weight of everything in no bag (#2191); null until first load. */ + const [unassignedWeightGrams, setUnassignedWeightGrams] = useState(null) const [templates, setTemplates] = useState([]) const [categoryAssignees, setCategoryAssignees] = useState>({}) + const reloadBags = useCallback(() => { + if (!bagTrackingEnabled) return + packingApi.listBags(tripId) + .then(r => { + setBags(r.bags || []) + setUnassignedWeightGrams(r.unassigned_weight_grams ?? null) + }) + .catch(() => {}) + }, [tripId, bagTrackingEnabled]) + + useEffect(() => { 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 - packingApi.listBags(tripId).then(r => setBags(r.bags || [])).catch(() => {}) - }, [tripId, bagTrackingEnabled]) + const handler = (event: Record) => { + if (event.type === 'packing:bag-totals') reloadBags() + } + addListener(handler) + return () => removeListener(handler) + }, [bagTrackingEnabled, reloadBags]) useEffect(() => { packingApi.listTemplates(tripId).then(r => setTemplates(r.templates || [])).catch(() => {}) @@ -470,6 +491,7 @@ export default function MPackingListTab({ planner }: { planner: TripPlanner }) { onClose={() => setShowBagsSheet(false)} bags={bags} items={items} + unassignedWeightGrams={unassignedWeightGrams} tripMembers={tripMembers} canEdit={canEdit} currentUserId={currentUserId} diff --git a/server/src/nest/packing/packing.controller.ts b/server/src/nest/packing/packing.controller.ts index be2dc90df..573adb6a2 100644 --- a/server/src/nest/packing/packing.controller.ts +++ b/server/src/nest/packing/packing.controller.ts @@ -85,6 +85,7 @@ export class PackingController { for (const item of created) { this.packing.broadcastItem(tripId, 'packing:created', { item }, item, socketId); } + this.packing.broadcastBagTotals(tripId); return { items: created, count: created.length }; } @@ -104,6 +105,7 @@ export class PackingController { throw new HttpException({ error: 'Bag not found' }, 400); } this.packing.emitToViewers(tripId, 'packing:created', { item }, item, socketId); + this.packing.broadcastBagTotals(tripId); return { item }; } @@ -149,6 +151,7 @@ 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); return { item: updated }; } @@ -166,6 +169,7 @@ export class PackingController { } // Scope the delete to the people who could see it (owner + recipients, #858). this.packing.emitToViewers(tripId, 'packing:deleted', { itemId: Number(id) }, deleted as PackingItemRow, socketId); + this.packing.broadcastBagTotals(tripId); return { success: true }; } @@ -207,6 +211,7 @@ export class PackingController { } // The clone is personal to the caller — only their sockets need it. this.packing.emitToViewers(tripId, 'packing:created', { item }, item, socketId); + this.packing.broadcastBagTotals(tripId); return { item }; } @@ -248,7 +253,13 @@ export class PackingController { @Get('bags') listBags(@CurrentUser() user: User, @Param('tripId') tripId: string) { - return { bags: this.packing.listBags(tripId) }; + // 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), + }; } @RequirePermission('packing_edit') @@ -324,6 +335,7 @@ export class PackingController { throw new HttpException({ error: 'Template not found or empty' }, 404); } this.packing.broadcastItem(tripId, 'packing:template-applied', { items: added }, added[0], socketId); + this.packing.broadcastBagTotals(tripId); return { items: added, count: added.length }; } diff --git a/server/src/nest/packing/packing.service.ts b/server/src/nest/packing/packing.service.ts index 006c16073..29720312f 100644 --- a/server/src/nest/packing/packing.service.ts +++ b/server/src/nest/packing/packing.service.ts @@ -112,6 +112,21 @@ export class PackingService { } } + /** + * Tell the whole room its bag weights moved (#2191). + * + * Deliberately NOT excluding the originating socket, which every other + * broadcast here does: the payload carries nothing to echo, and the sender's + * 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. + */ + broadcastBagTotals(tripId: string): void { + this.broadcast(tripId, 'packing:bag-totals', {}, undefined); + } + /** * The four public/private transitions after an update (#858). `wasPrivate` must be * read BEFORE the write: getting it wrong leaks a freshly-privatized item to the @@ -489,8 +504,42 @@ export class PackingService { // ── Bags ─────────────────────────────────────────────────────────────────── + /** + * What each bag actually weighs (#2191). + * + * Every weight TREK showed used to be a client-side sum over `listItems`, + * which is privacy-filtered — so a bag's "total" silently omitted the private + * items of every other member, and no one but their owner could ever see the + * real figure. That number is then measured against `weight_limit_grams`, an + * absolute airline limit, which makes a per-viewer subtotal not merely + * incomplete but wrong: a shared bag could sit over its limit and warn nobody. + * + * So the sum is computed here, over EVERY row, and only integers cross the + * wire. A member learns that a bag is heavier than the items they can see — + * never a name, category, quantity or owner. That is a deliberate, bounded + * disclosure and the point of the issue. + * + * Keyed by bag id, with the unassigned pile under `null` — the same shape the + * "no bag" row on every packing surface needs. + */ + private bagWeightTotals(tripId: string | number): Map { + const rows = this.db.all<{ bag_id: number | null; total: number | null }>(` + SELECT bag_id, SUM(COALESCE(weight_grams, 0) * COALESCE(quantity, 1)) AS total + FROM packing_items + WHERE trip_id = ? + GROUP BY bag_id + `, tripId); + return new Map(rows.map(r => [r.bag_id, r.total ?? 0])); + } + + /** The weight of everything in the trip that is in no bag (#2191). */ + unassignedWeightGrams(tripId: string | number): number { + 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); + const totals = this.bagWeightTotals(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 @@ -506,6 +555,7 @@ export class PackingService { return bags.map(b => ({ ...b, members: (membersByBag.get(b.id) || []).map(m => ({ ...m, avatar: avatarUrl(m) })), + total_weight_grams: totals.get(b.id) ?? 0, })); } diff --git a/server/tests/unit/nest/packing.controller.test.ts b/server/tests/unit/nest/packing.controller.test.ts index 9db2edafb..9558cc1e9 100644 --- a/server/tests/unit/nest/packing.controller.test.ts +++ b/server/tests/unit/nest/packing.controller.test.ts @@ -16,6 +16,8 @@ function makeService(overrides: Partial = {}): PackingService { broadcast: vi.fn(), broadcastItem: vi.fn(), broadcastToViewers: vi.fn(), + // Content-free "the bag weights moved" ping fired after every item write (#2191). + broadcastBagTotals: vi.fn(), // Real viewer logic so the emit-to-viewers routing is exercised faithfully. viewersOf: (item: { is_private?: number; owner_id?: number | null; recipients?: { user_id: number }[] } | null | undefined) => !item || !item.is_private ? null : [item.owner_id, ...(item.recipients || []).map(r => r.user_id)].filter((x): x is number => x != null), @@ -316,10 +318,14 @@ describe('PackingController (parity with the legacy /api/trips/:tripId/packing r }); describe('bags', () => { - it('GET /bags lists bags for the trip', () => { - const listBags = vi.fn().mockReturnValue([{ id: 3, name: 'Carry-on' }]); - const svc = makeService({ listBags } as Partial); - expect(new PackingController(svc).listBags(user, '5')).toEqual({ bags: [{ id: 3, name: 'Carry-on' }] }); + 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); + expect(new PackingController(svc).listBags(user, '5')).toEqual({ + bags: [{ id: 3, name: 'Carry-on', total_weight_grams: 800 }], + unassigned_weight_grams: 150, + }); }); 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 87887f199..f8bf4becd 100644 --- a/server/tests/unit/nest/packing.service.test.ts +++ b/server/tests/unit/nest/packing.service.test.ts @@ -697,6 +697,55 @@ describe('create-path fields + bag trip scope (#2154)', () => { expect(item).toMatchObject({ weight_grams: 250, bag_id: bag.id, quantity: 3 }); }); + it('PACK-SVC-077: a bag weighs what EVERY member put in it, private items included (#2191)', () => { + const { user } = createUser(testDb); + const { user: buddy } = createUser(testDb, { username: 'bag-buddy' }); + const trip = createTrip(testDb, user.id); + addTripMember(testDb, trip.id, buddy.id); + const bag = svc.createBag(trip.id, { name: 'Duffel' }) as any; + + // The reporter's repro: 500 g of mine (personal) + 300 g common + 200 g of + // my buddy's personal list. Their item is invisible to me by design (#858) + // and used to be missing from the weight along with it. + svc.createItem(trip.id, { name: 'Boots', weight_grams: 500, bag_id: bag.id, visibility: 'personal' }, user.id); + svc.createItem(trip.id, { name: 'Stove', weight_grams: 300, bag_id: bag.id }, user.id); + svc.createItem(trip.id, { name: 'Book', weight_grams: 200, bag_id: bag.id, visibility: 'personal' }, buddy.id); + + // What I am allowed to SEE is still only my own two items... + const visible = svc.listItems(trip.id, user.id) as any[]; + expect(visible.map(i => i.name).sort()).toEqual(['Boots', 'Stove']); + // ...but the bag weighs all three. + expect((svc.listBags(trip.id) as any[])[0].total_weight_grams).toBe(1000); + }); + + it('PACK-SVC-078: bag weight multiplies by quantity and survives null weights (#2191)', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const bag = svc.createBag(trip.id, { name: 'Carry-On' }) as any; + + svc.createItem(trip.id, { name: 'Socks', weight_grams: 60, bag_id: bag.id, quantity: 3 }, user.id); + // No weight and no quantity: contributes nothing rather than NaN. + svc.createItem(trip.id, { name: 'Passport', bag_id: bag.id }, user.id); + + expect((svc.listBags(trip.id) as any[])[0].total_weight_grams).toBe(180); + }); + + it('PACK-SVC-079: an empty bag reports 0, and the unassigned pile is summed too (#2191)', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const bag = svc.createBag(trip.id, { name: 'Empty' }) as any; + + // 0, never undefined — the client's fallback keys off the field being absent, + // so an empty bag must not read as "the server did not tell me". + expect((svc.listBags(trip.id) as any[])[0].total_weight_grams).toBe(0); + expect(svc.unassignedWeightGrams(trip.id)).toBe(0); + + svc.createItem(trip.id, { name: 'Loose sandwich', weight_grams: 150 }, user.id); + expect(svc.unassignedWeightGrams(trip.id)).toBe(150); + // Still empty: the loose item belongs to no bag. + expect((svc.listBags(trip.id) as any[]).find(b => b.id === bag.id).total_weight_grams).toBe(0); + }); + 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); diff --git a/shared/src/packing/packing.schema.ts b/shared/src/packing/packing.schema.ts index 733a5d623..1a9f68226 100644 --- a/shared/src/packing/packing.schema.ts +++ b/shared/src/packing/packing.schema.ts @@ -73,6 +73,15 @@ export const packingBagSchema = z.object({ assigned_username: z.string().nullable().optional(), created_at: z.string().optional(), members: z.array(packingBagMemberSchema).optional(), + /** + * What the bag really weighs, summed server-side over every member's items + * (#2191). The client used to add this up from the item list it was allowed + * to see, which silently dropped other members' private items and left the + * figure measured against an absolute weight limit wrong for everyone but + * its owner. Optional: an offline-cached bag from before #2191 has no such + * field, and the surfaces fall back to their local sum when it is absent. + */ + total_weight_grams: z.number().optional(), }); export type PackingBag = z.infer; diff --git a/shared/src/realtime/events.schema.spec.ts b/shared/src/realtime/events.schema.spec.ts index 1533d1adf..27884ece2 100644 --- a/shared/src/realtime/events.schema.spec.ts +++ b/shared/src/realtime/events.schema.spec.ts @@ -41,6 +41,7 @@ const FIXTURES: Record> = { 'packing:bag-members-updated': { bagId: 1, members: [{ user_id: 1 }] }, 'packing:assignees': { category: 'clothes', assignees: [1, 2] }, 'packing:template-applied': { items: [{ id: 7 }] }, + 'packing:bag-totals': {}, 'todo:created': { item: { id: 4 } }, 'todo:updated': { item: { id: 4 } }, 'todo:deleted': { itemId: 4 }, @@ -134,10 +135,11 @@ const DRIFT_VARIANTS: Partial[]> }; describe('@trek/shared realtime event registry', () => { - it('WSEVT-REG-001: pins the authoritative inventory counts (65 trip + 32 user = 97)', () => { - expect(TREK_WS_TRIP_EVENT_NAMES).toHaveLength(65); + it('WSEVT-REG-001: pins the authoritative inventory counts (66 trip + 32 user = 98)', () => { + // 66th trip event: packing:bag-totals (#2191). + expect(TREK_WS_TRIP_EVENT_NAMES).toHaveLength(66); expect(TREK_WS_USER_EVENT_NAMES).toHaveLength(32); - expect(TREK_WS_EVENT_NAMES).toHaveLength(97); + expect(TREK_WS_EVENT_NAMES).toHaveLength(98); }); it('WSEVT-REG-002: every name is domain:action shaped and outside the reserved plugin: namespace', () => { diff --git a/shared/src/realtime/events.schema.ts b/shared/src/realtime/events.schema.ts index 84903d12b..4c721e070 100644 --- a/shared/src/realtime/events.schema.ts +++ b/shared/src/realtime/events.schema.ts @@ -114,6 +114,17 @@ export const TREK_WS_EVENTS = { payload: z.object({ category: z.string(), assignees: z.array(z.unknown()) }), }, 'packing:template-applied': { scope: 'trip', payload: z.object({ items: z.array(z.unknown()) }) }, + /** + * "A bag's weight may have changed" — deliberately content-free (#2191). + * + * Bag totals are summed server-side across every member's items, so a private + * item nobody else may see still moves the number everybody else reads. The + * item events themselves cannot carry that: a private one is delivered only + * to its owner, which is exactly the privacy rule that made the totals wrong + * in the first place. This ping goes to the whole room and says nothing at + * all; receivers refetch the bags and get numbers, never contents. + */ + 'packing:bag-totals': { scope: 'trip', payload: z.object({}) }, // ── Todo ───────────────────────────────────────────────────────────────── 'todo:created': { scope: 'trip', payload: z.object({ item: entity }) },