Repository navigation
Conversation
mrivas00
added
priority: medium
mrivas00
left a comment
mrivas00
left a comment
There was a problem hiding this comment.
Automated review of this PR's diff: 14 findings, 11 inline above and 3 grouped here because their file or line is not part of this diff.
Headline: the PR's own fix does not hold in the common case, and UNCHANGED opens three paths to silently wrong stored data. Detail on the two most important ones is below and inline.
apps/web/src/screens/CarbonInventory/components/EmissionEditor/hooks/useEmissionEditorForm.ts:353 — not in this diff
[alta] The auto-load recommendation overwrites a legacy line's factor before isFactorUnchanged ever runs, so the headline fix of this PR — a line saved before the snapshot carried a catalog id keeps its factor — does not hold in the common case.
A legacy line comes back with factorSource: "IPCC", factorValue: 520, emissionFactorId: null, so useEmissionCaptureData seeds baseFactorId: null. The user edits only the quantity, handleCellChange ends in tryToLoadDetermineFactorPlatform, whose guard is hasCatalogFactorSelected = formLine.baseFactorId !== null (false) and isOwnFactorSelected (false, "IPCC" is not in CUSTOM_FACTOR_SOURCES), so handleFactorSelectionChange rewrites factorSource, appliedFactorYear, baseFactorId, emissionFactorId, factorValue and factorRateMeasurementUnitId to the recommended vintage. isFactorUnchanged then returns false, the save sends CATALOG(recommended), and the historical snapshot is replaced by the current vintage's year and value — silently, from a quantity edit. Same for any snapshot with no catalog id whose source is not one of the custom ones. The new web test exercises mapLinesToSyncRequest in isolation, so it passes regardless.
Fix: treat a line with a loaded snapshot as already having a factor — extend the guard with line.loadedFactor?.factorValue != null, which is exactly the field this PR adds.
apps/api/src/features/carbonInventories/syncCarbonInventoryLines/helper.ts:448 — line not in this diff (directTotalEmissionsInKg / createLineResult)
[alta] inputType: "DIRECT" combined with factorSelection: { type: "UNCHANGED" } is accepted and writes a preserved factor snapshot with no result row, so the line's emissions silently vanish while it still displays a factor.
directTotalEmissionsInKg returns null for anything that is not a DIRECT selection, and createLineResult's first branch requires inputType === DIRECT && directTotal !== null while the second requires SIMPLIFIED || EXPERT. An UNCHANGED update sent with inputType: "DIRECT" falls through both: createLineFactor still copies value, unit, source, year and emissionFactorId, directTotalEmissions is written as null, and no carbonInventoryLineResult row is created. The new schema variant is the only thing that made this combination expressible, and nothing in UpdateFactorSelectionSchema or the service ties inputType to the selection variant — today only the web-side !line.isManualTotalEmissions guard keeps it out of the wire.
Fix: reject the combination in SyncCarbonInventoryLinesRequestSchema alongside its existing refines, so UNCHANGED is expressible only for SIMPLIFIED / EXPERT.
openspec/changes/add-emission-factor-year/specs/emission-factor-year-selection/spec.md:91 — not in this diff
[baja] The governing openspec delta still declares the line-sync contract as exactly three mutually exclusive variants (CATALOG, CUSTOM, DIRECT) and says the API "SHALL load the selected factor and derive source, year, canonical value and converted applied value" — which UNCHANGED deliberately does not do. No scenario pins the two behaviours this PR calls out as newly guaranteed (a catalog edit after capture is not adopted; a retired factor no longer fails the save), and tasks.md gains no task, so the archived change will not record the variant.
Fix: add the variant to the contract block plus those two scenarios, and the matching task in tasks.md.
| } | ||
| } | ||
|
|
||
| const [dimensions, factors, rateUnits] = await Promise.all([ |
There was a problem hiding this comment.
[baja] emissionFactorDimension.findMany runs unguarded, unlike the factor and rate-unit queries next to it (factorIds.size > 0 / rateUnitIds.size > 0), and requiredPositions is consumed only by resolveCatalogFactor. So an all-UNCHANGED sync — the exact case this PR creates — and a delete-only sync both pay for a query whose result is never read, the latter with subcategoryId: { in: [] }. subcategoryIds is also collected from every request row rather than only CATALOG rows, widening the IN list for nothing.
Fix: guard the query on factorIds.size > 0 and collect subcategory ids only inside the FactorSelectionType.CATALOG branch.
There was a problem hiding this comment.
Left open on purpose — your call. Not touched: the round of local fixes covered the correctness and efficiency findings, and this one still stands as written, the query unguarded and subcategoryIds collected from every request row.
There was a problem hiding this comment.
Heads-up: this thread went outdated with the push — the line it was anchored to changed when the stored-snapshot query joined the Promise.all. The finding still stands as written; the query is now the emissionFactorDimension call inside the Promise.all at helper.ts line 212, still unguarded, with subcategoryIds still collected from every request row rather than only the CATALOG ones.
| function mapUpdateFactorSelection( | ||
| line: EmissionCaptureFormLine | ||
| ): UpdateFactorSelection | null { | ||
| if (!line.isManualTotalEmissions && isFactorUnchanged(line)) { |
There was a problem hiding this comment.
[baja] The !line.isManualTotalEmissions guard cannot usefully be false and duplicates a check isFactorUnchanged already makes: a manual-total line is DIRECT server-side, so it has no carbonInventoryLineFactor row and no manualFactor, and mapLineToResponse returns factorValue: null / factorRateMeasurementUnitId: null, which isFactorUnchanged's own "nothing was stored" guard already rejects. It reads as a second independent rule the reader has to reconcile with the first.
Fix: drop the condition and let isFactorUnchanged be the single rule — but only together with the server-side rejection of DIRECT + UNCHANGED noted in the review body, which is what the guard accidentally prevents today.
There was a problem hiding this comment.
Left open on purpose — your call, and it now comes with a caveat. Since DIRECT + UNCHANGED is rejected in the request schema (local commit f21dbd1), this guard is the only thing that keeps the web from sending a request the server now answers with a 400. Dropping it is still right, but it means letting isFactorUnchanged cover the manual-total case, not just deleting the condition.
| const factorIds = new Map<string, bigint>(); | ||
| const rateUnitIds = new Map<string, bigint>(); | ||
|
|
||
| const remember = (into: Map<string, bigint>, id: string) => { |
There was a problem hiding this comment.
[baja] remember and the three Map<string, bigint> collectors exist only to dedup bigints, which a Set<bigint> already does (new Set([BigInt(1), BigInt(1)]).size === 1 — bigint primitives dedup under SameValueZero). The if (!into.has(id)) guard is dead weight even for a Map, since set is idempotent, and subcategoryIds bypasses remember and calls .set directly — so the file carries two idioms for the same job three lines apart.
Fix: use Set<bigint> for all three collectors (factorIds.add(BigInt(id)), [...factorIds]) and delete remember.
There was a problem hiding this comment.
Left open on purpose — your call. 35c8a93 touched remember only to canonicalize the key it stores under, so the three Map<string, bigint> and the helper are still there where a Set<bigint> would do.
| typeof CatalogFactorSelectionSchema | ||
| >; | ||
|
|
||
| export type UnchangedFactorSelection = z.infer< |
There was a problem hiding this comment.
[baja] UnchangedFactorSelection is exported and has no consumer anywhere in apps/, packages/ or tools/ (only UpdateFactorSelection is used, in the api helper and the web transformer). Relatedly, the context.lineId === undefined arm of the UNCHANGED case in resolveFactorSelection is dead by construction — the create union genuinely excludes the variant — and its only effect is to keep lineId optional in the context type, which hides a missing lineId behind a silent null instead of a type error.
Fix: drop the unused type, and make lineId required on the update path so the ternary can go.
There was a problem hiding this comment.
Left open on purpose — your call. The unused export and the dead lineId === undefined arm are both still there. A second-round comment on line 33 of the api helper reached the same defect from the other side — ItemData is shared by the create and update loops, which is what keeps UNCHANGED representable on a create — and is resolved as a duplicate, so its remedy belongs here: make lineId required, either by splitting the resolver or by making ItemData generic over its selection.
| if ( | ||
| !isSameMagnitudeFamily( | ||
| { | ||
| numeratorMagnitudeId: |
There was a problem hiding this comment.
This rebuilds RateUnitMagnitudeFamily by hand, and resolveRateUnitMagnitudeFamily's doc comment still claims this call site.
The object literal is exactly RateUnitMagnitudeFamily from @/features/measurementUnits/helpers.js, whose comment says it exists precisely so that "every write path has to derive it the same way — factor create/update, the seed, and the line-sync check that a requested applied unit is convertible from the selected catalog factor. Shared here so those callers cannot drift." After this PR line-sync derives the pair inline, so the shared definition is down to two callers and that sentence is stale.
A one-line pure helper next to isSameMagnitudeFamily — taking { numeratorMeasurementUnit: { magnitudeId }, denominatorMeasurementUnit: { magnitudeId } } and returning RateUnitMagnitudeFamily — lets the async DB variant and this batched in-memory variant keep sharing one shape. Either way the comment in measurementUnits/helpers.ts should drop the line-sync mention.
There was a problem hiding this comment.
Confirmed, and not raised in the earlier round. resolveRateUnitMagnitudeFamily is down to two callers (createEmissionFactor and updateEmissionFactor); line-sync derives the pair inline now, so the doc comment's caller list is stale on that clause — and a grep finds no seed caller either, so that half of the sentence is stale independently of this PR. Keeping this open: the shared pure helper next to isSameMagnitudeFamily is the fix, and the comment in measurementUnits/helpers.ts should be corrected with it.
|
Two findings from the first review round were raised in the review body rather than inline, because their file or line falls outside this diff's hunks, so they have no thread to resolve. Both are fixed and pushed:
The remaining open threads are the four low-severity cleanups and the two findings from the second round; each says what it is waiting on. |
… family
A factor used to carry its vintage informally inside `source` ("DEFRA 2025"),
and the index keyed identity on (subcategory, dimensions, source). That made a
second vintage for the same activity impossible, and let two rate units in the
same physical family — kg/kg and kg/ton are both mass/mass — sit in the catalog
as separate rows.
`emission_factor` now stores `year` explicitly, where NULL means the factor is
confirmed transversal and applies to every reporting year; it never means
"unknown". It also denormalizes the numerator/denominator magnitude pair,
because Postgres cannot reach those through `rate_measurement_unit` from a
unique index, and identity has to compare the unit *family* rather than the
exact unit. `carbon_inventory_line_factor` gains `applied_factor_year` so a
saved line stays reproducible; whether it disagrees with the footprint year is
derived at read time, never stored, or it would go stale the moment someone
re-dated the inventory.
The migration refuses to guess. An unclassified source aborts the whole
transaction rather than defaulting to transversal, and two same-family factors
whose values disagree after conversion abort too — that is a methodology
question, not a migration one. Duplicate unit representations are retired by
soft delete after captured lines are repointed at the survivor, since the FK is
ON DELETE SET NULL and would otherwise drop those lines' catalog link. No
applied value, unit, source or result is rewritten.
BREAKING CHANGE: the API requires the new columns, so the migration must run
before the new API starts. See openspec/changes/add-emission-factor-year/rollout.md.
Applies the reviewed classification to both datasets: `source` keeps only the provider name and `year` carries the reporting year, written out explicitly as an integer or as null. Seed `year` is required-but-nullable so omitting it fails validation — null has to be a deliberate statement that the factor is transversal, not the result of leaving a key out. The 16 duplicate unit representations are consolidated, keeping kg/ton. Both rows were the same IPCC factor written twice, and the methodology payload already expands a factor into every unit of its family, so the kg/kg reading is generated by conversion instead of stored. kg/ton keeps the larger numeric value (1460 rather than 1.46), which leaves the most significant digits inside Decimal(28, 10). The magnitude pair is derived from each rate unit here, the same way every production write path derives it, so a freshly seeded catalog and a migrated one end up with identical business keys — verified: the only difference between them is the one factor `base` has and `testing` does not. The row-count check now counts ACTIVE factors only. The migration retires duplicate representations by soft delete, so a migrated database keeps rows this seed data no longer describes and would otherwise fail the check while being perfectly correct.
`checkDuplicateEmissionFactor` now compares the full identity — required dimension values, year, source and the magnitude pair — matching null explicitly, because `year = null` is a transversal factor rather than a wildcard and has to collide with another transversal factor from the same provider. Normalization of non-required dimension slots applies to the comparison only, never to what is stored. A value a maintainer entered in an optional slot is real catalog data; it simply does not earn the factor a separate identity. That leaves the check marginally stricter than the index, which compares raw columns — the same asymmetry the catalog already had, and safe in that direction. `validateSourceConsistency` is removed outright rather than scoped by year. It forced every active factor in a subcategory to share one source, which made the accepted multi-provider case impossible; source is part of a factor's identity now, so DEFRA (2025) and IPCC (2025) are two legitimate alternatives. The magnitude pair is derived from the selected rate unit by a shared helper and re-derived on every update, not only when the rate unit changes — a stale pair would leave the row outside the identity the index is enforcing. Methodology duplication carries year and both magnitude IDs across, or the copy would collapse several vintages into one row.
…olumn Year is a nullable column of its own, where a blank cell is the explicit choice that declares a factor transversal. Persistent helper text asks for the provider name without the year — capture appends it, so "DEFRA 2025" would display as "DEFRA 2025 (2025)" — and a four-digit year in the name raises an advisory icon beside the field. It never blocks saving: a provider whose name really contains a number is legitimate, so this is data quality guidance, not a domain rule. Source locking is removed. It pinned every factor in a subcategory to the first one's source, which is exactly what made a second provider impossible to enter, and its tooltip claimed a subcategory-wide invariant that no longer exists. The duplicate-factor message now explains the real conflict — source, year, required dimensions and unit family, noting that kg/kg and kg/ton are one family — and the source-conflict message is gone with the validation behind it. The spreadsheet gets a separate year column, blank rather than "-" for a transversal factor: an empty year cell is how transversal reads, and it must not look like missing data.
Pinning a non-required slot to null asks for rows whose slot is also empty, which is not the same as ignoring it. An existing factor with a value parked in that slot was invisible to the check, and since the unique index compares the raw columns it did not catch the insert either — leaving two ACTIVE rows sharing one identity, two indistinguishable options in the capture selector, and a tie that stops the recommendation from firing. The slot is now left out of the query entirely, which is what the function's own doc comment already claimed: reject a little more than the constraint would, never less. The existing test covered the direction that happened to work — stored slot empty, candidate populated. Adds the one that did not.
The year was an unbounded integer, so a typo in the maintainer's numeric cell was stored as-is. It then became part of the factor's identity, sorted to the top of the capture selector, won the vintage ranking, and flagged every line using it as mismatched — with no way to notice from the UI. The bounds live in @repo/constants because the plausible range is a per-deployment judgement, and the upper one is fixed rather than derived from today so the same payload does not start being rejected overnight.
The four CATALOG_EMISSION_FACTOR_* codes had no entry, so every one of them reached the user as "Error al guardar la huella" — with nothing to indicate the line at fault or that the fix is re-picking a factor. Each message names what happened and what to do. The wording is deliberately the same shape across the four, because from the organization's side they are one situation: the factor on that line can no longer be applied.
The tagged union makes the pair representable, and nothing checked it: a SIMPLIFIED line carrying a DIRECT total persisted the total with no result row — a number in the editor that counts as zero everywhere — while a DIRECT line carrying a catalog selection wrote a factor snapshot no result reads.
The summary read the source from the live catalog row while reading the year from the line snapshot, so a renamed factor produced a provider/vintage pair that never existed — in the table and in the Excel export alike.
Rows were deduplicated by catalog factor id, but the year and the value are snapshotted per line: once a maintainer re-dates a factor, the lines holding the older vintage collapsed into the newer row and their year warning disappeared from the summary.
Moving the persisted conversion to Decimal dropped the finiteness check on the result, and nothing bounded it against the column it is written to. A non-finite result is now rejected for both callers, and a value too large for applied_factor_value fails as a typed data error instead of a numeric overflow mid-transaction.
Each catalog line ran its own required-dimension query and two rate-unit queries, all of them repeats within a subcategory save: forty lines meant around a hundred and sixty sequential round-trips inside the transaction, against Prisma's five-second budget. The shared part is now read once per request and the rate units are cached as they are seen.
The row id joins the factor, its applied year and its applied value; a transversal factor has no year, and an empty segment made the id read as a typo. It says "transversal" instead.
Two lines holding two vintages of one re-dated factor must stay two rows; the existing assertions on the row id move to the vintage-keyed form.
With one affected line and one vintage the alert read "1 de 1 línea … usan factores de 2020": the verb and the factor phrase stayed plural, and the test asserted the ungrammatical form.
The summary table gained an "Año del factor" column and the workbook did not, so the copy people send to a reviewer was the one without the vintage — the place a stale factor matters most.
The required-dimension query filtered on the literal "ACTIVE" while the query right above it already read its status from the enum.
The docblock still said the string variant goes through a double and can round the value, which its own docblock — and the code, since it delegates here — contradict.
The non-null assertion holds only because the validation loop above throws for any update id missing from the stored lines; a reader had to reconstruct that.
An update can now say that it did not touch the line's factor, instead of restating a selection. Only updates get the variant: a create has no stored snapshot to keep, so UNCHANGED is unrepresentable there rather than silently meaning "no factor". This is what lets the server leave a captured snapshot alone. Restating a catalog selection on every save made the stored value depend on the catalog still saying the same thing — and made a line saved before the snapshot carried a catalog id lose its factor entirely.
An update that does not restate its factor keeps the snapshot the line already has: read from its active input, with nothing read from the catalog. That is the only correct answer for a line saved before the snapshot carried a catalog id, and it stops an edit elsewhere in the inventory from depending on the catalog row still being there. The snapshot lives on the input this loop supersedes, so it is resolved before that input is deactivated. A preserved source can be null, which is why the resolved shape mirrors the column instead of inventing one.
…ng it Each loaded line now keeps a frozen copy of the factor fields the server sent, and a save compares against it. When they still match, the update says UNCHANGED; otherwise it states the selection in full, as before. This is what stops a line saved before the snapshot carried a catalog id from losing its factor. Those lines come back with no catalog id and a catalog source name, which the mapping recognises as neither custom nor catalog, so it sent null — and the server, reading null as "no factor", wrote neither a snapshot nor a result. The manual-total toggle re-sends every line of the subcategory, so one click could zero a whole inventory's factor-based emissions. The comparison is one-sided on purpose: reporting a change that did not happen only costs a redundant catalog lookup, while missing one would keep a snapshot the user had replaced.
Five cases, each pinning one thing the previous behaviour got wrong: a catalog value edited after capture is not adopted; a line whose factor was retired still saves; a custom factor keeps its own line-input columns and not just the snapshot; a line that never had a factor degrades to "still incomplete" rather than erroring; and UNCHANGED on a create is rejected by the schema. The retirement case is the one that used to take the whole batch down with it, including lines that had nothing to do with that factor.
Pins both directions of the comparison: an untouched factor is declared unchanged even when other fields were edited, and any real edit — a different catalog factor, a retyped custom value, a switch to a declared total — still states the selection in full. The legacy case has its own test, since it is the one that used to lose the factor: a line with a catalog source and no catalog id, which the mapping recognised as neither variant and sent as null.
The builder returns a fully-typed line, so the trailing assertion was redundant and tripped no-unnecessary-type-assertion. Belongs with the test it was added in; kept separate rather than rewriting a commit.
Reading the snapshot per line put a query back inside the write transaction on the most common save there is: touching one cell sends every line of the subcategory as an update, and each one declares its factor unchanged. The ids are known before the loop, so they load with the rest of the resolution context in one query, alongside the dimension positions rather than after them. The context now takes what the request needs instead of a bare subcategory list, so the next kind of lookup is a field rather than another argument. With the snapshots read up front, the update loop can supersede the old input first again, as it did before this feature.
The carried-forward custom factor required both `manualFactor` and `manualFactorRateUnitId`, but both columns are nullable: a line saved before the unit was filled in lost its declared factor from the new input on any unchanged save, leaving only the snapshot and dropping the line out of the measurement-unit reference counts. The snapshot holds the same value in the same unit, so it stands in for the missing column.
An untouched line used to restate CATALOG, so every save re-ran the required-dimension check. Declaring the factor unchanged skipped it, and an update that changed a required dimension kept a catalog snapshot describing a different line: its quantity reported against another selection's factor, with the year-mismatch warning and the factor reference counts pointing at the wrong row. The check compares the request against the selections the line's own input recorded, so it holds without reading the catalog.
`isFactorUnchanged` compared `emissionFactorId` while the catalog selection is built from `baseFactorId`, so it validated a field the request never sends. The two only agree by convention — `resetFactorValueFields` clears `baseFactorId` and leaves `emissionFactorId` stale — and this is the direction that must not fail: a false "unchanged" keeps a snapshot the user replaced.
The auto-load guard only recognised a choice recorded in `baseFactorId`, which a line captured before the snapshot carried a catalog id does not have. Editing only the quantity therefore rewrote source, year, value and unit to the current vintage before the save could declare the factor unchanged — the case that selection was added for. A loaded snapshot now counts as a factor already there.
The stored-factor copy only moved when a refetch landed, so between a save and its refetch it described a snapshot the server had already replaced. A factor changed and then changed back in that window matched the copy field for field, so the save declared it unchanged and the server kept the value the user had moved away from — the one direction the comparison must never get wrong. The sync response now refreshes the copy for every line it persisted; a line the request did not touch keeps the one it has, and a line created in the same save keeps none, since its id only arrives with the refetch.
The batched resolution is invisible in a single-line, single-subcategory sync, which is all the file had: a subcategory's required positions leaking into another one would have passed unnoticed. These cover both directions of that, a batch mixing unchanged, restated and replaced factors against a catalog that moved between the saves, and the two requests an unchanged selection must now refuse.
The factor row was the one lookup still running once per line inside the write transaction: a 40-line save with a catalog selection on each paid 40 sequential round-trips for rows the request already named up front. They load with the rest of the resolution context now, in one query filtered to ACTIVE exactly as the per-line read was, so a missing or retired factor is still the same 404. The map is keyed by the parsed id rather than the request text, because `IdSchema` accepts any digit string and "042" has to reach the same entry as "42" — which is what the per-line query got for free by passing the id through `BigInt` into the `where`.
7270a89 to
6e8df0b
Compare
1 participant
What & why
CATALOGselection — so an untouched line went through the whole derive-and-snapshot path on every save.UNCHANGEDvariant so the client can say "leave this line's factor alone", and batches the catalog lookups a sync needs.feat/mati/emission-factor-server-authority.Key changes & decisions
1.
UNCHANGEDas a fourth request variant, not an omitted fieldsyncCarbonInventoryLines/schemas.ts,types.ts· 1fa3b172. The server keeps the stored snapshot verbatim
syncCarbonInventoryLines/helper.ts,service.ts· 23248f2UNCHANGEDthe existing applied value, unit, source and year are preserved and the factor is not re-read.3. Web declares untouched instead of restating
useEmissionCaptureData.ts,emissionCaptureTransformers.ts(+2) · 9bbc59a4. Batched catalog lookups
syncCarbonInventoryLines/helper.ts,service.ts· 4e6c5feTest plan
Automated gates that actually ran on this branch:
pnpm format:check && pnpm lint && pnpm type-check— green.pnpm test:api -- /carbonInventories --coverage=false— 554/554 (adds theUNCHANGEDpreservation cases).pnpm test:web— 878/878. Remaining api domains run in the CI matrix.Manual, by risk — not run in this session:
Chain — merge in order:
UNCHANGEDvariant + batched catalog lookups ← this PROut of scope