Review follow-ups for [16]: persist the regulation stack, withdraw the unsourced 2005 load editions, distribute wind by height - #97
Open
diegokingston wants to merge 3 commits into
Conversation
…apshot/restore `regulations` and `revisions` were declared on `StructureModel` and on `ModelSnapshot`, but `snapshot()` built an explicit literal that omitted both and `restore()` never read them back. Every path that round-trips the model goes through that pair — .ded save/open, autosave, URL share, tab capture and undo/redo — so the whole role stack and the staleness stamps were dropped on each one, silently resetting the project to `defaultRegulations()`. `clear()` did not reset them either, so a new project inherited the previous one's stack. The restore path goes through `migrateRegulations` rather than assigning raw, which is the caller that function was written for and never got: a stored project naming an edition since withdrawn now comes back UNSET instead of bound to rules the app cannot apply, and a project saved before the stack existed migrates from the v1 `codeSettings` shape. Also imports `StoredRegulations`/`RevisionVector`, which the interface referenced without importing.
… supplied either CIRSOC 201-2005 is correctly reserved: `availability: 'UNAVAILABLE_SOURCE'`, because the official text is not supplied and its rules therefore cannot be written. The 101-2005 and 102-2005 options carried the same `textAvailable: false` in `REGULATIONS` but no `availability` field, so `availabilityOf` defaulted them to AVAILABLE, `optionsForRole` offered them and `roleUsable` accepted them. Nothing implements those editions. `cirsoc102/wind.ts` is a rebuild against the 2025 text with no edition parameter; `cirsoc101/combinations.ts` and `live-loads.ts` likewise hardcode 2025 in every clause ref. Selecting "CIRSOC 102 (2005)" therefore produced a project stamped 2005 whose wind loads were computed by, and cited, the 2025 rules — the exact mislabelling `optionsForRole`'s own docstring says the model exists to prevent, and the note shown was only "Superseded edition", which declares no substitution. The v1 migration bound those adapter ids directly, which would now throw in `bindRole`; it binds the edition in force and emits `editionWithdrawn` per role instead, as it already did for concrete. The guard test asserted `expect(o.role === 'concrete').toBe(false)`, which for a basis/loads/wind option reduces to `expect(false).toBe(false)` — it passed vacuously for precisely the roles it was not covering. It now collects every offending option and asserts the list is empty, and names them on failure.
… base value `wind.ts` evaluates the windward wall at two heights — `[min(5, h), h]` — with a comment saying it does so "so the caller can distribute". The caller took `.find()`, which returns the FIRST entry, i.e. the z = 5 m sample at the weakest point of the profile, computed the net pressure once outside the per-level loop and applied it unchanged to every storey. The mean-roof-height value it had just computed was never read. K_z = 2.41·(max(z,5)/zg)^(2/α) rises monotonically with height, so this under-predicted wind on everything above 5 m — roughly 20% at z = 15 m and 30% at z = 30 m for 45 m/s in exposure C — and the §1.10 minimum does not mask it for ordinary multi-storey buildings. The error is unconservative and flows straight into member sizing. `netAlongWindPressureAt` lives in `wind.ts` beside the pressure rules rather than in the load generator, so the height rule keeps one implementation. Only the windward term varies: leeward is q_h over the full height per §2.4.1, and GC_pi cancels between the two faces.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the code review on #79. Three fixes, each with the regression test that failed first. Based on
pr/16-cirsoc-2025-load-codesat44d7c9a5, and targeted at that branch so it lands inside #79 rather than racing it.Nothing here disputes the design of #79 — all three are cases where the code does not yet do what the branch's own comments and tests say it does.
1. The regulation stack never survived a save (
5393dba1)regulationsandrevisionsare declared onStructureModeland onModelSnapshot, butsnapshot()built an explicit object literal that omitted both, andrestore()never read them back. Every persistence path goes through that pair —.dedsave/open, autosave, URL share, tab capture, and undo/redo — so the whole role stack and the staleness stamps were dropped on each one and the project silently fell back todefaultRegulations().clear()did not reset them either, so a new project inherited the previous one's bindings.The restore path now goes through
migrateRegulationsinstead of assigning raw. That function was written for exactly this caller and never had one: a stored project naming a withdrawn edition comes back unset rather than bound to rules the app cannot apply, and a project saved before the stack existed migrates from the v1codeSettingsshape.Worth knowing while reviewing the tests: the first draft of them passed against the unfixed code, because
clear()leaves these fields in place and the stale value satisfied the assertion. They only turn red once the live fields are blanked between snapshot and restore, which is whatblankLiveStack()is for.This commit also adds imports for
StoredRegulations/RevisionVector, whichmodel.svelte.tsreferenced without importing.2. CIRSOC 101-2005 and 102-2005 were selectable but ran the 2025 rules (
be34d9fd)201-2005 is correctly reserved as
UNAVAILABLE_SOURCE. 101-2005 and 102-2005 carry the sametextAvailable: falseinREGULATIONS, but had noavailabilityfield, soavailabilityOfdefaulted them toAVAILABLE,optionsForRoleoffered them, androleUsableaccepted them.Nothing implements those editions.
cirsoc102/wind.tsis a rebuild against the 2025 text with no edition parameter, andcirsoc101/combinations.tsandlive-loads.tshardcode 2025 in every clause ref. So picking "CIRSOC 102 (2005)" gave a project stamped 2005 whose wind loads were computed by, and cited, the 2025 rules — whatoptionsForRole's own docstring says the availability model exists to prevent. The note shown was only "Superseded edition, kept for legacy projects", which declares no substitution.Two consequences handled here:
bindRole; it binds the edition in force and emitseditionWithdrawnper role, as it already did for concrete;roles-revisions.test.tsasserted the old contract in two places (one comment read "101-2005 and 102-2005 remain available"), so both are updated.The guard test in
edition-availability.test.tscould not have caught this: it assertedexpect(o.role === 'concrete').toBe(false), which for a basis/loads/wind option reduces toexpect(false).toBe(false)and passed vacuously for precisely the roles it was not covering. It now collects every offending option, asserts the list is empty, and names them on failure.No UI change, same as when 201-2005 was withdrawn — the panel already lists non-available editions read-only with their reason.
3. Wind was not distributed by height (
302d2e0c)wind.tsevaluates the windward wall at[min(5, h), h]specifically "so the caller can distribute". The caller used.find(), which returns the first entry — the z = 5 m sample, the weakest point of the profile — computed the net pressure once outside the per-level loop, and applied it unchanged to every storey. The mean-roof-height value it had just computed was never read.K_z = 2.41·(max(z,5)/z_g)^(2/α)rises monotonically with height, so this under-predicted wind on everything above 5 m: about 20% at z = 15 m and 30% at z = 30 m for 45 m/s in exposure C. The §1.10 minimum does not mask it for ordinary multi-storey buildings. It is unconservative and it reaches member sizing.netAlongWindPressureAtlives inwind.tsbeside the pressure rules rather than in the load generator, so the height rule keeps one implementation. Only the windward term varies with height — leeward isq_hover the full height per §2.4.1, andGC_picancels between the two faces.Tests
3428 passed / 0 failedon a quiet machine, against3435on the unmodified branch plus the 6 new tests here.One caveat on how that was measured: this needs a locally built WASM engine. Without
npm run wasmthe suite fails ~1100 tests on the branch as-is, which is an environment artifact and not a signal. Under concurrent load I also saw a handful ofTest timed outfailures appear and disappear on both the modified and unmodified branch — flaky, load-sensitive, and unrelated to these changes.Not addressed
migrateRegulationsreturnsnotices, includingeditionWithdrawn, and nothing surfaces them. After this PR the bindings are right, but a user opening a project that named a withdrawn edition is still not told. Wiring that to a UI notification is a design decision that belongs to you rather than to a review follow-up, so I left it.Separately, and not a code change: the ~230 new
codes.*/regulations.*/loadPlan.*/maturity.*keys land inen/esonly, so the other 12 locales silently render English via thedicts.enfallback. I understand that's the declared EN/ES scope. Flagging it becauselocale-parity.test.tsexists for this exact pattern and is scoped todesign.*, so it does not cover the new namespaces — widening it would be cheap.🤖 Generated with Claude Code