[P3] Pre-dive main-page gas select offers to replace all cylinders even when a cylinder already holds that gas #124

New issue

Open

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

pedro commented

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

Owner

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

Location: src/App.tsx:233-266, src/App.tsx:1361-1368, src/App.tsx:3046-3074, docs/USER-GUIDE.md:74

Problem and evidence

Before the dive starts (depth 0 and no cylinder usage), selectMainGas never checks whether a cylinder already holds the chosen gas. If there is more than one non-oxygen cylinder, it always sets pendingGas and opens the "Replace cylinder setup?" dialog. Accepting rewrites the active cylinder to the new gas ([id]: { ...cylinder, gas: gas.name }) and removes every other cylinder (except the CCR oxygen cylinder). That includes a cylinder that already held the chosen gas, along with its label, volume, pressures and reserve. It also removes ascent switches for other gases. Rejecting keeps the old gas. So pre-dive, the main-page select cannot simply switch to a gas you have already set up in another cylinder. USER-GUIDE.md:74 says gas changes use the selected cylinder if its mix matches, otherwise the first matching cylinder. The guide does not mention the replace-setup prompt at all. During the dive the same select goes straight to switchGas and follows the documented rule.

Line-by-line trace of selectMainGas at HEAD 2cac7cf:

beforeDive is true when dive.depth === 0 and all cylinderUsage values are 0.
extraCylinders holds every cylinder except the CCR oxygen cylinder. With two OC cylinders (Cylinder 1 Air, Cylinder 2 EAN32) its length is 2 and accepted is false, so setPendingGas(EAN32) runs and the function returns. Nothing ever looks at cylinders[*].gas.
On "Accept replacement" (line 3065), selectMainGas(pendingGas, true) runs. id = activeCylinder ("1"). setCylinders gets only { "1": {...cyl1, gas: "EAN32"} } (plus the oxygen cylinder on CCR), so Cylinder 2 is deleted. setPlannedSwitches keeps only EAN32 switches (and Oxygen on CCR).
The dialog text (lines 3054-3058) confirms the replacement is intentional in general: "Replace your configured cylinders with one X cylinder? Additional cylinders and their settings will be removed."
The earlier reviewer's Playwright run against the production build matches this trace: after accepting, the only Active cylinder option is "Cylinder 1 · EAN32".
grep found no mention of this replacement in docs/, and no test covers this dialog.

Suggested correction

At the start of the beforeDive branch in selectMainGas, look for a matching cylinder: prefer selectedCylinder if its gas matches, otherwise the first Object.keys(cylinders).find(id => cylinders[id].gas === gas.name). If one exists, call setSelectedCylinder(id), clear automatic deco and switchGas without prompting or changing the cylinder layout. Keep the replace-setup prompt only when no cylinder holds the chosen gas, and describe that pre-dive behaviour in USER-GUIDE.md near line 74.

Acceptance criteria

Regression test (App-level, Testing Library or Playwright):

Pre-dive, add Cylinder 2 as EAN32 labelled "Stage EAN32", then choose EAN32 in the "Main breathing gas" select. No "Replace cylinder setup?" dialog appears. The Active cylinder becomes Cylinder 2, both cylinders and their labels/settings remain, and ascent switches are unchanged.
Pre-dive with two cylinders, choosing a gas that no cylinder holds still shows the replace dialog, and Accept/Reject behave as today.
During the dive, behaviour is unchanged.
USER-GUIDE.md describes the pre-dive replace prompt. pnpm verify passes.

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

Priority: P3 Type: bug Source: full codebase review requested 2026-10-31 (independently reproduced against `2cac7cf`) Location: `src/App.tsx:233-266`, `src/App.tsx:1361-1368`, `src/App.tsx:3046-3074`, `docs/USER-GUIDE.md:74` Problem and evidence Before the dive starts (depth 0 and no cylinder usage), `selectMainGas` never checks whether a cylinder already holds the chosen gas. If there is more than one non-oxygen cylinder, it always sets `pendingGas` and opens the "Replace cylinder setup?" dialog. Accepting rewrites the active cylinder to the new gas (`[id]: { ...cylinder, gas: gas.name }`) and removes every other cylinder (except the CCR oxygen cylinder). That includes a cylinder that already held the chosen gas, along with its label, volume, pressures and reserve. It also removes ascent switches for other gases. Rejecting keeps the old gas. So pre-dive, the main-page select cannot simply switch to a gas you have already set up in another cylinder. USER-GUIDE.md:74 says gas changes use the selected cylinder if its mix matches, otherwise the first matching cylinder. The guide does not mention the replace-setup prompt at all. During the dive the same select goes straight to `switchGas` and follows the documented rule. Line-by-line trace of selectMainGas at HEAD 2cac7cf: - `beforeDive` is true when `dive.depth === 0` and all `cylinderUsage` values are 0. - `extraCylinders` holds every cylinder except the CCR oxygen cylinder. With two OC cylinders (Cylinder 1 Air, Cylinder 2 EAN32) its length is 2 and `accepted` is false, so `setPendingGas(EAN32)` runs and the function returns. Nothing ever looks at `cylinders[*].gas`. - On "Accept replacement" (line 3065), `selectMainGas(pendingGas, true)` runs. `id` = activeCylinder ("1"). `setCylinders` gets only `{ "1": {...cyl1, gas: "EAN32"} }` (plus the oxygen cylinder on CCR), so Cylinder 2 is deleted. `setPlannedSwitches` keeps only EAN32 switches (and Oxygen on CCR). - The dialog text (lines 3054-3058) confirms the replacement is intentional in general: "Replace your configured cylinders with one X cylinder? Additional cylinders and their settings will be removed." - The earlier reviewer's Playwright run against the production build matches this trace: after accepting, the only Active cylinder option is "Cylinder 1 · EAN32". - grep found no mention of this replacement in docs/, and no test covers this dialog. Suggested correction At the start of the beforeDive branch in selectMainGas, look for a matching cylinder: prefer `selectedCylinder` if its gas matches, otherwise the first `Object.keys(cylinders).find(id => cylinders[id].gas === gas.name)`. If one exists, call `setSelectedCylinder(id)`, clear automatic deco and `switchGas` without prompting or changing the cylinder layout. Keep the replace-setup prompt only when no cylinder holds the chosen gas, and describe that pre-dive behaviour in USER-GUIDE.md near line 74. Acceptance criteria Regression test (App-level, Testing Library or Playwright): 1. Pre-dive, add Cylinder 2 as EAN32 labelled "Stage EAN32", then choose EAN32 in the "Main breathing gas" select. No "Replace cylinder setup?" dialog appears. The Active cylinder becomes Cylinder 2, both cylinders and their labels/settings remain, and ascent switches are unchanged. 2. Pre-dive with two cylinders, choosing a gas that no cylinder holds still shows the replace dialog, and Accept/Reject behave as today. 3. During the dive, behaviour is unchanged. USER-GUIDE.md describes the pre-dive replace prompt. 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#124

No description provided.

Rows
Columns