[P2] Replay always charges CCR oxygen to the departure oxygen cylinder: a mid-dive oxygen-supply change gives false reserve/empty results, and an OC-to-CCR switch mid-dive makes replay throw #108

New issue

Open

opened 2026-10-31 19:47:40 +00:00 by pedro · 0 comments

pedro commented

2026-10-31 19:47:40 +00:00

Owner

Priority: P2
Type: bug
Source: full codebase review requested 2026-10-31 (independently reproduced against 2cac7cf)

Location: src/replay/diveReplay.ts:236-238, src/replay/diveReplay.ts:253, src/App.tsx:806, src/App.tsx:886-889, src/App.tsx:897-908, src/App.tsx:1850-1868, src/App.tsx:2482-2486, src/replay/DiveSummary.tsx:20-23

Problem and evidence

The CCR oxygen supply cylinder is captured once, in the replay seed (oxygenCylinderId: oxygenCylinder, App.tsx:806). That seed is latched as replayStart.current = previousReplay.current when the dive leaves the surface (App.tsx:888). DiveSample records breathingGas, rmv and activeCylinderId, but not the oxygen supply cylinder. The sample-dedup condition (App.tsx:897-908) also ignores supply changes. As a result, appendLeg always runs charge(seed.oxygenCylinderId, gas.ccr.oxygenRate * dt) (diveReplay.ts:253). The live dive instead charges whichever oxygen cylinder is currently selected (chargeCylinders, App.tsx:208-209). Two consequences follow:
(a) Neither the 'CCR oxygen supply' select (App.tsx:1850-1868) nor the Rebreather checkbox (App.tsx:2482-2486) is disabled during a dive. If the diver changes the oxygen supply mid-dive, 'Original reconstruction' charges all oxygen to the departure cylinder and nothing to the new one. That cylinder can drain to 0 bar, which produces a false 'First cylinder reserve reached' and 'First cylinder empty' and the shortfall alert. The tank overlays then contradict the recorded dive.
(b) If no Oxygen cylinder existed at departure, seed.oxygenCylinderId is undefined, for example on an OC dive switched to CCR mid-dive. Every CCR leg then throws 'The recorded active cylinder is unavailable.' (diveReplay.ts:237-238), so the unmodified replay cannot be computed at all. This also happens when an existing departure cylinder is edited to Oxygen mid-dive. The cause is the undefined seed oxygen id, not only the missing auto-added 'ccr-oxygen' cylinder. This contradicts USER-GUIDE.md:168, which says replay retains 'recorded gas/cylinder changes'.

Scratch harness /tmp/divesim-verify-78Frtg/t.ts calls the real replayDive with originalReplay.
(a) Setup: seed oxygenCylinderId '2', cylinders 1 = 12 L, 2 = 1 L, 3 = 12 L, all at 200 bar with 50 bar reserve. CCR diluent with oxygenRate 5 L/min at 20 m for 46 min, then surface at 48 min. The oxygen supply is intended to switch to '3' at 21 min.
Replay final pressures: {1: 198.7, 2: 0, 3: 200}, reserveTime 29.6 min, emptyTime 39.5 min. The live dive would have charged about 105 L to cylinder 2 (about 95 bar left) and the rest to cylinder 3, with no empty cylinder.
(b) Setup: seed oxygenCylinderId undefined, samples OC air, then CCR diluent from minute 7. replayDive throws 'The recorded active cylinder is unavailable.'
Code trace: there is no disabled attribute on the oxygen-supply select or the Rebreather checkbox. The checkbox only calls setRunning(false), which pauses the dive and does not reset it. The earlier reviewer's Playwright run against the production build reproduced both paths through the real UI: (a) wrong Original pressures and false empty/reserve rows; (b) error only, no table.

Suggested correction

Record the oxygen supply on each sample in App.tsx, e.g. oxygenCylinderId: circuit === "closed" ? oxygenCylinder : undefined. Add current.oxygenCylinderId !== last.oxygenCylinderId to the push-dedup condition so a supply change creates a sample. In appendLeg, pass the leg's start sample and charge a.oxygenCylinderId ?? seed.oxygenCylinderId. For older samples without the field, fall back to the Oxygen cylinder marked active in a.cylinders. Only throw when no supply can be resolved. The seed's cylinders map must also contain any cylinder that is referenced. This overlaps with the separately reported cylinder-added-mid-dive issue: either snapshot all cylinders referenced by samples, or add cylinders to the replay context with their pressure at first use.

Acceptance criteria

Regression test for the oxygen-supply switch: a CCR dive whose oxygen supply changes from cylinder A to cylinder B mid-dive. The Original replay must charge oxygen to A before the change and to B after it, and its final pressures must match the recorded final sample pressures within tolerance. No false reserve or empty time may be reported.
Regression test for an OC dive switched to CCR mid-dive where no Oxygen cylinder existed at departure (including the variant where an existing departure cylinder is edited to Oxygen). replayDive must not throw, and it must charge oxygen to the recorded supply.
Existing replay tests still pass. pnpm verify passes.

Scope: maintenance of the educational simulator. Preserve historical R1 acceptance and operational-validation limits.

Priority: P2 Type: bug Source: full codebase review requested 2026-10-31 (independently reproduced against `2cac7cf`) Location: `src/replay/diveReplay.ts:236-238`, `src/replay/diveReplay.ts:253`, `src/App.tsx:806`, `src/App.tsx:886-889`, `src/App.tsx:897-908`, `src/App.tsx:1850-1868`, `src/App.tsx:2482-2486`, `src/replay/DiveSummary.tsx:20-23` Problem and evidence The CCR oxygen supply cylinder is captured once, in the replay seed (`oxygenCylinderId: oxygenCylinder`, App.tsx:806). That seed is latched as `replayStart.current = previousReplay.current` when the dive leaves the surface (App.tsx:888). DiveSample records breathingGas, rmv and activeCylinderId, but not the oxygen supply cylinder. The sample-dedup condition (App.tsx:897-908) also ignores supply changes. As a result, appendLeg always runs `charge(seed.oxygenCylinderId, gas.ccr.oxygenRate * dt)` (diveReplay.ts:253). The live dive instead charges whichever oxygen cylinder is currently selected (chargeCylinders, App.tsx:208-209). Two consequences follow: (a) Neither the 'CCR oxygen supply' select (App.tsx:1850-1868) nor the Rebreather checkbox (App.tsx:2482-2486) is disabled during a dive. If the diver changes the oxygen supply mid-dive, 'Original reconstruction' charges all oxygen to the departure cylinder and nothing to the new one. That cylinder can drain to 0 bar, which produces a false 'First cylinder reserve reached' and 'First cylinder empty' and the shortfall alert. The tank overlays then contradict the recorded dive. (b) If no Oxygen cylinder existed at departure, seed.oxygenCylinderId is undefined, for example on an OC dive switched to CCR mid-dive. Every CCR leg then throws 'The recorded active cylinder is unavailable.' (diveReplay.ts:237-238), so the unmodified replay cannot be computed at all. This also happens when an existing departure cylinder is edited to Oxygen mid-dive. The cause is the undefined seed oxygen id, not only the missing auto-added 'ccr-oxygen' cylinder. This contradicts USER-GUIDE.md:168, which says replay retains 'recorded gas/cylinder changes'. Scratch harness /tmp/divesim-verify-78Frtg/t.ts calls the real replayDive with originalReplay. (a) Setup: seed oxygenCylinderId '2', cylinders 1 = 12 L, 2 = 1 L, 3 = 12 L, all at 200 bar with 50 bar reserve. CCR diluent with oxygenRate 5 L/min at 20 m for 46 min, then surface at 48 min. The oxygen supply is intended to switch to '3' at 21 min. Replay final pressures: {1: 198.7, 2: 0, 3: 200}, reserveTime 29.6 min, emptyTime 39.5 min. The live dive would have charged about 105 L to cylinder 2 (about 95 bar left) and the rest to cylinder 3, with no empty cylinder. (b) Setup: seed oxygenCylinderId undefined, samples OC air, then CCR diluent from minute 7. replayDive throws 'The recorded active cylinder is unavailable.' Code trace: there is no `disabled` attribute on the oxygen-supply select or the Rebreather checkbox. The checkbox only calls setRunning(false), which pauses the dive and does not reset it. The earlier reviewer's Playwright run against the production build reproduced both paths through the real UI: (a) wrong Original pressures and false empty/reserve rows; (b) error only, no table. Suggested correction Record the oxygen supply on each sample in App.tsx, e.g. `oxygenCylinderId: circuit === "closed" ? oxygenCylinder : undefined`. Add `current.oxygenCylinderId !== last.oxygenCylinderId` to the push-dedup condition so a supply change creates a sample. In appendLeg, pass the leg's start sample and charge `a.oxygenCylinderId ?? seed.oxygenCylinderId`. For older samples without the field, fall back to the Oxygen cylinder marked `active` in `a.cylinders`. Only throw when no supply can be resolved. The seed's `cylinders` map must also contain any cylinder that is referenced. This overlaps with the separately reported cylinder-added-mid-dive issue: either snapshot all cylinders referenced by samples, or add cylinders to the replay context with their pressure at first use. Acceptance criteria 1. Regression test for the oxygen-supply switch: a CCR dive whose oxygen supply changes from cylinder A to cylinder B mid-dive. The Original replay must charge oxygen to A before the change and to B after it, and its final pressures must match the recorded final sample pressures within tolerance. No false reserve or empty time may be reported. 2. Regression test for an OC dive switched to CCR mid-dive where no Oxygen cylinder existed at departure (including the variant where an existing departure cylinder is edited to Oxygen). replayDive must not throw, and it must charge oxygen to the recorded supply. 3. Existing replay tests still pass. pnpm verify passes. Scope: maintenance of the educational simulator. Preserve historical R1 acceptance and operational-validation limits.

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: pedro/DiveSim#108

No description provided.

Rows
Columns