# PR #5490 — Review Comments Tracker

PR: [feat(Import): detect new field type from column data #CAIRO-4131](https://github.com/wix-private/cairo/pull/5490)
Reviewers: **liorgu-wix** (human) + **Codex** (`github-actions[bot]`). CI/bundle/status bots excluded.

> **liorgu-wix summary (no blocking bugs):** "The DI seam (core owns the `NewFieldType`
> vocabulary + sampling/vote, fields injects media-awareness) is the right shape and
> respects `utils → core → cairo/fields`. Wiring is correct and happy-path tests pass."
> Recurring theme: code/comments claim fidelity to cm-import the implementation doesn't
> actually have (sample size, URL validation, ordering, audio/vector) — either match
> cm-import or own the deliberate divergences.
> [review](https://github.com/wix-private/cairo/pull/5490#pullrequestreview-4539585402)

Status: `open` / `done` / `dismiss` · Severity: Major / Test gap / Minor / Nit / Info

The **vs cm-import today** column records how our behavior compares to cm-import
(`wix-data-client/cm-import`) — the source of liorgu's recurring "claims fidelity it
doesn't have" theme.

| # | Comment link | Description | vs cm-import today | Severity | Status | Resolution recommendation |
|---|---|---|---|---|---|---|
| 1 | [codex](https://github.com/wix-private/cairo/pull/5490#discussion_r3436393090) — `ImportState.ts` (date) | Date detection uses browser-dependent `new Date(value)` after a regex check; same CSV can infer `date` in one browser, `text` in another → inconsistent `updateSchema` payloads. | **Same** — cm-import's `detectFieldConfig.js` uses the identical `new Date(value).getTime()` heuristic. The reviewer wants us to *improve on* cm-import, not match it. | Major | open | Strict-parse the supported date formats (validate y/m/d components) instead of `new Date`/`Date.parse` heuristics. |
| 2 | [liorgu §1](https://github.com/wix-private/cairo/pull/5490#pullrequestreview-4539585402) — cross-cutting | Confirm **no current `updateSchema` consumer** (esp. the CMS source adapter) switches on `fieldType` and would now receive `number`/`date`/`image`/… and mishandle/throw. If CMS is already live, that's a regression vector, not a follow-up. | **Diverges** — cm-import's `createFieldDefinition` maps each detected `fieldType` and creates it. Ours: the only consumer (auto-patterns `useImport`) hardcoded `SHORT_TEXT`; the CMS `addField`/`addFields` reverse map threw "not supported" on anything but text/number/boolean/date. | Major (verify) | **done** | **Audit: no live regression** — the only consumer discarded the detected type (always `SHORT_TEXT`). **Fixed forward** in auto-cairo: the 4-type cap was a **UI limitation, not an API one** — `@wix/data` creates all field types — so widened `PatternsFieldTypeToCmsFieldType` to add URL/IMAGE/VIDEO/DOCUMENT/AUDIO, and `useImport` now maps each detected `NewFieldType` → its field type (SHORT_TEXT fallback). `addFields` creates the real type instead of throwing. Tests updated (media now creates; throw-path uses REFERENCE) — 29 green. Forward-compatible once #5490 publishes. (auto-cairo repo.) |
| 3 | [liorgu](https://github.com/wix-private/cairo/pull/5490#discussion_r3448316713) — `Table.importButton.spec.tsx:743` | Test gaps: mixed column → `text` (the ⅔ vote itself), empty-heavy column, `audio`/`video`, **core default (no injection)** proving the DI seam gates media detection, and direct unit specs for the pure functions. | N/A (our test coverage) — cm-import exercises the majority/fallback in its own `guessFieldConfigFromSamples` specs; we don't yet. | Test gap | **done** | Added direct unit specs: `core/__tests__/state/detectFieldType.spec.ts` (14 — number/date/boolean/url branch edges, majority, **fallback**, **empty-heavy**, sample truncation, DI hook, and **core-default media→text/url** proving the seam) and `fields/src/__tests__/utils/detect-field-type.spec.ts` (6 — image/video/document/**audio**/vector, external-image-URL→image, primitives, mixed→text). |
| 4 | [liorgu](https://github.com/wix-private/cairo/pull/5490#discussion_r3448316710) — `detect-field-type.ts:23` | Detects **more** than cm-import while comments claim a match: `audio` and `vector→image`. | **Diverges (superset)** — cm-import's `validationOrder` has **no audio** entry (never detects audio); its *detection* path (`detectionSchema` = `wix-image-with-extension`) does **not** check `isWixVectorUri`. We detect both. | Minor + Test gap | **done** | Reworded the `detectMediaType` comment to document audio + vector→image as **intentional supersets** of cm-import's detection path (dropped the "matches cm-import" framing); added the audio test (fields spec). |
| 5 | [liorgu](https://github.com/wix-private/cairo/pull/5490#discussion_r3448316711) + [codex](https://github.com/wix-private/cairo/pull/5490#discussion_r3436393101) — `ImportState.ts:456` | `_columnValues()` materializes the whole column although only the first 50 are used → O(rows×cols) allocations at parse time. | **Diverges (less efficient)** — cm-import's `_getSamples` does `importData.slice(0, sampleSize).map(...)` — slices *before* mapping, so it never allocates the full column. We map the full column then slice. | Minor (perf) | **done** | Exported `TYPE_SAMPLE_SIZE` from `detectFieldType`; `_columnValues` now `slice(0, TYPE_SAMPLE_SIZE)` **before** mapping, so only the head is copied — matching cm-import's `_getSamples`. |
| 6 | [liorgu](https://github.com/wix-private/cairo/pull/5490#discussion_r3448316706) — `detectFieldType.ts:38` | `SAMPLE_SIZE = 50` is attributed to cm-import, but the comment is wrong. | **Diverges** — cm-import's `constants.sampleSize` is **6**; we use **50**. | Nit (doc) | **done** | Kept 50; rewrote the `TYPE_SAMPLE_SIZE` comment + `detectColumnType` JSDoc to own it as a deliberate larger sample than cm-import's 6 (same 2/3 vote). |
| 7 | [liorgu](https://github.com/wix-private/cairo/pull/5490#discussion_r3448316707) — `detectFieldType.ts:59` | URL/email/phone helpers are re-implementations, not a port; `isValidWebsiteUrl` is laxer than cm-import. | **Diverges (looser)** — cm-import's `validators/url.js` delegates to `@wix/santa-core-utils` `isValidUrl`/`isValidEmail` for structural validation **and** requires a protocol; ours only checks protocol-prefix + no-whitespace, so e.g. `https://{garbage}` → we say `url`, cm-import says no. | Nit (doc) / Minor | **done** | `isValidWebsiteUrl` now **delegates to the platform `URL` parser** (rejects empty-host / whitespace / malformed authorities) instead of a prefix check; `@wix/santa-core-utils` isn't available here, so the comment owns the residual gap (`URL` accepts some odd hosts santa rejects). Added invalid-URL→text tests. |
| 8 | [liorgu](https://github.com/wix-private/cairo/pull/5490#discussion_r3448316709) — `detect-field-type.ts:48` | "media-first" comment claims cm-import precedence it doesn't have. | **Diverges (order; equivalent result)** — cm-import runs `detectPrimitiveType` (number → boolean → date) **first**, media only on fall-through; we check media first. Same result in practice (media URIs don't match the number/bool/date regexes). | Nit (doc) | **done** | Reworded the `detectValueTypeWithMedia` comment to state our order differs from cm-import's and why the result still matches (fixed alongside #4 in the same block). |
| 9 | [liorgu](https://github.com/wix-private/cairo/pull/5490#discussion_r3448316712) — `ImportState.ts:577` | `index >= 0 ? … : 'text'` — `header` always comes from `csvHeaders`, so the `'text'` branch is dead. | N/A — our code structure (cm-import keys samples off the selected column, no such guard). | Nit | open | Drop the guard, or add a one-line "defensive" comment. |
| 10 | [liorgu](https://github.com/wix-private/cairo/pull/5490#discussion_r3448316708) — `detectFieldType.ts:102` | Empty cells count as `text` votes; an optional column >⅓ blank falls back to `text` even if every non-blank value is a number. | **Same** — cm-import's `detectFieldConfig('') → text` and `_getSamples` doesn't filter blanks. Faithful, but a sharp edge worth flagging. | Info | open | Flag to product; optionally ignore blanks in the vote (would depart from cm-import — decide consciously). |
| 11 | [liorgu §2](https://github.com/wix-private/cairo/pull/5490#pullrequestreview-4539585402) — cross-cutting | Leading-zero data loss later: zip/phone/IDs (`01234`) detect as `number` once the consumer honors it. | **Same** — inherited directly from cm-import's number detection. Out of scope here. | Info | open | Note on the consumer follow-up ticket so it's a conscious choice. |
| 12 | [liorgu](https://github.com/wix-private/cairo/pull/5490#discussion_r3448316714) — `import-create-new-field-issues.md:10` | Non-blocking / pre-existing: git-tracked issue-tracker markdown in the component source tree goes stale and reads as noise in a feature diff. | N/A — repo hygiene, unrelated to cm-import. | Info (non-blocking) | open | Team decision: keep these `.md` files in-repo vs. move to Jira/docs. Not introduced by this PR. |

## Notes

- **Build-failure bot (resolved):** `devex-ai-build-agent` flagged a `TranslationsKeys`
  mismatch at `ImportStepConfiguration.tsx:78` on an earlier commit (before the
  patterns-fields title keys were added/rebuilt). Latest Falcon CI is **PASSED** on
  `c80c2f9` — already fixed, no action.
- **Theme across 4, 6, 7, 8:** all the same root note from liorgu — the "ported from
  cm-import" framing overclaims fidelity. Cheapest resolution is a doc pass that owns
  the deliberate divergences (sample size, URL approximation, ordering, audio/vector
  superset), unless we choose to match cm-import exactly.