fix(tables): refuse scale suffixes and resolve the lone-separator ambiguity - #6111
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview
Lone comma/dot ambiguity is resolved using the column’s Type conversion checks compatibility via Column update APIs map Reviewed by Cursor Bugbot for commit 66d5479. Configure here. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis follow-up hardens currency conversion and parsing while improving related API and table UI behavior.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains in the revised currency parsing, metadata propagation, or previously reported separator-handling paths.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/table/currency.ts | Restricts textual currency markers and resolves lone separators using marker presence and the currency’s fraction digits. |
| apps/sim/lib/table/column-types/currency.ts | Forwards the complete column currency metadata into value coercion. |
| apps/sim/lib/table/import.ts | Threads target-column currency codes through import coercion for three-decimal currencies. |
| apps/sim/lib/table/columns/service.ts | Uses the complete converted column definition for compatibility checks and removes obsolete transaction commentary. |
| apps/sim/app/api/table/[tableId]/columns/route.ts | Classifies unchanged-type conflicts as client errors rather than internal failures. |
| apps/sim/app/api/v1/tables/[tableId]/columns/route.ts | Aligns the public API’s unchanged-type conflict response with the non-versioned route. |
Reviews (6): Last reviewed commit: "fix(tables): group a lone dot only when ..." | Re-trigger Greptile
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@cursor review |
|
You were right and my defense was wrong — changed in Your alternative ("change the accepted input contract explicitly") sent me looking for a precedent, and it settled it against me: a spreadsheet with a USD-formatted cell stores a typed The rule now keys on the separator, tempered by what the currency can express:
The observation that makes this safe: a currency with one or two decimal places always formats with both separators, so no formatter output is decided by this branch at all. It also let me delete the
|
|
@cursor review |
70581ac to
0879b42
Compare
|
@cursor review |
…iguity A verification pass over the previous commit found that my own fix opened a new hole of the class it closed. Widening the currency marker to 1-3 letters made scale suffixes parse: `1.2 M` read as 1.2, so a column of `1.2 M` / `3.4 M` — an ordinary spreadsheet paste — converted cleanly and rewrote every cell a millionfold too small. Before the widening those were rejected and the data was safe. Now refused explicitly, while `kr` and `zł` still parse despite starting with the same letters. Stripping the marker also newly routed formatted zero-decimal amounts into the lone-separator branch, where a single dot was always decimal: `1.235 ¥` read as 1.235 rather than 1235. That is a wrong number where there used to be a refusal, which is the worse failure. A lone separator followed by three digits is now resolved by two signals — a marker means a formatter produced it, and formatters group; a currency carrying three decimals (KWD, TND) reads them as decimals. `coerce` passes the column's code, so the parser can ask. Bare typed input keeps the decimal reading. Also: the unchanged-type throw fell through to a 500 for every type except currency, whose message happened to contain the word; it now maps to 400. And the last three comments describing the removed two-transaction architecture are gone — the previous commit claimed they were and two survived. Documented, and failing closed rather than guessing: markers written flush against the digits (`Rp12,00`) stay rejected, because a letter touching a digit is the only thing distinguishing a currency marker from a part number, and reading `SKU400` as 400 invents a value where refusing merely inconveniences.
Adding currency made the list nine items (295px with its separator and padding), past the shared dropdown's 240px cap — so Select and Workflow sat below a scrollbar. Sized on this dropdown rather than on the shared `DropdownMenuContent`, which every other menu in the app relies on. Also gave the sidebar's type combobox headroom, since it gained the same entry and was within 4px of its own cap.
Review of the previous commit found the marker signal alone was too coarse. The evidence: a currency with decimal places ALWAYS formats with both separators — `1.234,56 €`, `$1,234.56`, `1.234,000 KWD`. Only a zero-decimal currency emits a lone one (`1.235 ¥`). So a marker does not imply grouping; the currency's own decimal places decide, and a three-decimal currency's trailing three digits are decimals however the value arrived. That matters most for CSV import, which passed no currency at all: a KWD column importing `0,500` read as five hundred rather than a half. The column's code is now forwarded, since nothing in a CSV carries a marker to fall back on. A bare typed `1.234` still reads as decimals.
…e marker Greptile blocked on `$1.234` reading as 1234, and offered the alternative of stating the input contract explicitly. Looking for a precedent settled it against me: a spreadsheet with a USD-formatted cell stores a typed `1.234` as 1.234 and displays $1.23. Reinterpreting it as one thousand two hundred thirty-four is a thousandfold surprise, and my defense of it was wrong. The rule now keys on the separator, tempered by what the currency can express: - A dot is the decimal point in the notation most people type, so it stays a decimal — except for a currency with no decimal places, where `1.235 ¥` cannot be a fraction of a yen and is the one lone-separator form a formatter actually emits. - A comma is grouping by convention (`1,500` is fifteen hundred), except for a three-decimal currency, where `0,500` is a half. A currency with one or two decimal places always formats with both separators, so no formatter output is decided here at all. This also drops the `hadMarker` signal entirely — once the separator and the currency answer the question, where the string came from stops mattering.
…lt stub Bugbot flagged that the conversion gate calls `isValueCompatibleWithType`, which hand-builds a column from loose arguments and never sets `currencyCode` — while the coercion a few lines later reads the real one. The reported impact does not occur. Whether an amount parses does not depend on the currency, only which number it yields, so the gate and the coercion always agree on accept/reject; and the value written back is already coerced against the real column, so no cell is stored wrong. I checked this rather than argued it: across nine currency codes and every separator shape in the parser's repertoire, acceptance never diverged. The stub is still worth removing. It is correct only because of an invariant nobody wrote down, and `convertedColumn` — sitting right there, whose own comment already says it exists so the scan reads the same option set and currency — is what the gate should have used all along. So the gate now takes the target column itself. The loose-argument form moves into the test file as local marshalling, leaving production with only the whole-column shape, which cannot silently drop a key a future type adds. No behavior change, so no test changes: a currency-divergence regression test could not fail today, and pinning an invariant this removes the need for would only block a legitimate future change.
…denylist Bugbot found that `SCALE_SUFFIX` misses `mio`, `mrd`, `bio`, `tsd`, `mln` and `md`. Each is one to three letters, so each matched the currency-marker pattern and was stripped: `1,2 mio` read as 1.2, a millionfold too small, which is the same failure `1.2 M` had. Adding the six words would leave the next six. The denylist is the defect — it has to enumerate every magnitude abbreviation in every language, and each one it misses silently rescales a value rather than refusing it. So the check is inverted. A bare letter token is stripped only when it names a currency: an ISO 4217 code the runtime can enumerate, or one of the common non-ISO markers people type (`kr`, `zł`, `Kč`). Anything else is left in place, fails the amount-shape check, and is refused. `12 units` and `12 pcs` now fall out of the same rule the magnitude words do, without naming either. A marker carrying a currency symbol skips the check — no magnitude abbreviation contains one — so `R$`, `CHF`, `1 234,56 kr` are untouched. This rests on ISO codes and magnitude words being disjoint, which is true for all 162 codes the runtime knows, and is now pinned by a test so a future collision fails loudly instead of silently. `SCALE_SUFFIX` survives only for a runtime without `Intl.supportedValuesOf`, which cannot enumerate codes and so keeps the old permissive behaviour; rejecting every letter marker there would break `USD 12.50`. It no longer has to be complete, since every modern runtime rejects by non-recognition. Verified the new tests fail when the vetting is reverted.
0879b42 to
d4148d1
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d4148d1. Configure here.
…rmatted Greptile found that a bare `1.235` typed into a JPY column stored 1235. The zero-decimal branch read the dot as grouping without asking where the value came from, so typing, pasting, a tool write, or an import all inflated a thousandfold in silence. The rule was inverted relative to its own evidence. Grouping was justified as "the one lone-separator form a formatter emits" — and a formatter always emits its marker too. The marker is exactly what separates formatter output from someone typing, and I had removed it as redundant two commits ago. So a lone dot groups only for a zero-decimal currency AND only when a marker came with it. `1.235 ¥` and `JPY 1.235` still read as 1235; a bare `1.235` now stores 1.235 and displays `¥1` — wrong in a way the writer can see and correct, rather than a silent thousandfold error. This does not walk back the earlier fix. For a currency with one or two decimal places a formatter always emits BOTH separators, so a lone dot there never came from one and stays a decimal whether or not a marker is present — `$1.234` is still 1.234. The marker only carries information in the zero-decimal case, which is the only case that now consults it. The two overlapping lone-separator tests are replaced by three that each state one rule. Verified the new one fails when the marker requirement is reverted.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 66d5479. Configure here.
1 participant
Follow-up to #6106, which merged before this landed. The first item below is live on staging now and destroys data.
Summary
1.2 Mparses as 1.2. Astringcolumn holding1.2 M/3.4 M— an ordinary spreadsheet paste — converts tocurrencywith zero reported incompatibilities and rewrites every cell a millionfold too small. Same for5 K,3.4 bn,10 B. Now refused.1.235 ¥(de-DE JPY) parses as 1.235 instead of 1235. A lone separator followed by three digits is now resolved by two signals: a marker means a formatter produced it and formatters group, while a currency carrying three decimals (KWD, TND) reads them as decimals.coercepasses the column's code so the parser can ask. Bare typed input keeps the decimal reading.currency, whose message happened to contain the word. Now 400.Why it wasn't in #6106
Widening the currency marker to 1–3 letters (to support
kr,zł,R$,CHF) is what made one- and two-letter scale suffixes parse. The previous[A-Za-z]{3}$required exactly three letters, soMandKwere rejected and the data was safe. My fix opened a hole of exactly the class it closed — found by a verification pass that ran after the merge.krstill parses despite starting with the same letter asK; the suffix rule requires the whole trailing token to match.Deliberate limits, documented in the code
Both fail closed —
null, never a wrong number:Rp12,00) stay rejected. A letter touching a digit is the only thing distinguishing a currency marker from a part number, and readingSKU400as 400 invents a value where refusing merely inconveniences.١٢٣). Supporting them is wider than this type — it would also touchnumber, display, and sorting.Type of Change
Testing
kr-vs-Kdistinction.Intllocale sweep from feat(tables): add currency column type on a new column-type registry #6106 still passes: 24 locale/currency pairs × 6 amounts round-trip.check:utils,check:api-validation:strict,check:client-boundary,check:react-querypass.Checklist