feat: legacy permalink hydration — /:tool/:id fetches S3 read-only - #161
Conversation
Route /:tool/:id in the SPA: legacy /<tool>/<32-hex-id> URLs passed through by the PHP front controller now hydrate in the client. A read-only fetchLegacyPermalink(id) GETs data/<id> straight from the production S3 bucket (build-time VITE_S3_DATA_URL, cors mode) and a single element-id → converter-state mapping table populates the converter in the matching direction. The URL is never rewritten and nothing is ever written back; CI gains a zero-tolerance no-write-API check (spec: Old share links keep resolving — read-only).
Component tests for hydrate/not-found/failure-with-retry states and the read-only contract (URL never rewritten, plain GET with cors mode, no calls without a permalink id), unit tests for path parsing, shape matching, and fetch failure classes, plus a manual E2E script that hydrates a real 2019 bucket object through the unmocked code paths.
![obvious-autobuild-staging[bot]](https://avatars.githubusercontent.com/u/31830338?s=60&v=4){kind=small}
There was a problem hiding this comment.
Obvious Code Review
Verdict: COMMENT (1 Blocker, 1 High, 1 Medium, 1 Suggestion)
Summary
- Blocker: 1 —
app/src/lib/permalink.ts:158— Hydration routes on payload shape alone (the URL's tool segment is discarded), so non-converter legacy permalinks misroute. Worst case, verified against the legacy views on master: an sql2json save containsjson: true(a format radio, boolean perAPP.save), so the link hydrates the json2csv converter with an empty input and no notice — the user's SQL silently vanishes. json_beautifier links hydrate as the wrong tool; json_validator links get a "This data doesn't exist (or was deleted)" notice for data that does exist. All three contradict the PR's stated contract: "Only csv2json/json2csv hydrate; other tools fall through to the converter normally." - High: 1 —
app/scripts/e2e-legacy-permalink.ts:9— The committed E2E script crashes on run (import.meta.envis undefined under tsx; theimport_meta_envglobal is inert). The PR's headline "Real-object E2E … E2E OK" evidence is not reproducible as committed. - Medium: 1 —
app/src/hooks/useLegacyPermalink.ts:49— Objects that exist but don't map are reported as "This data doesn't exist (or was deleted)" — factually wrong for live json_validator/Data Janitor objects.
Suggestions
app/src/lib/permalink.ts:34—VITE_S3_DATA_URLwithout a trailing slash silently concatenates to a bad key; normalize or validate at startup.
Verified while reviewing: 96/96 vitest and tsc -b green on head; the CI write-verb grep empirically catches method: "POST", method: 'PUT', and putObject; PERMALINK_TOOLS matches the PHP shim exactly; the csv2json/json2csv element-id table is accurate against origin/master.
| ): HydratedConverterState | null { | ||
| if (!isLegacyShape(data)) return null; | ||
| const hasCsvInput = "csv" in data; | ||
| const hasJsonInput = "json" in data; |
There was a problem hiding this comment.
Blocker · reliability — Hydration routes on payload shape ("json" in data), but the URL's tool segment was discarded in parsePermalinkPath (line 44), and the legacy json element id is overloaded across tools with different meanings. Verified against origin/master:application/views/*_view.php + APP.save in js/src/main.js:
- sql2json saves
{sql: "...", json: true, javascript: false, minify: false}—json/javascriptare format radios, stored as booleans.hasJsonInputis true, sohydrateConverterreturns directionjson2csvwithinput = toText(true) = ""(line 170): the permalink silently hydrates an empty converter with no notice — the user's SQL is dropped from view. - json_beautifier saves its input under
json(textarea) → hydrates as the json2csv converter: data intact, wrong tool. - json_validator saves
{result: ...}only →null→ the "This data doesn't exist (or was deleted)" notice, for data that exists.
This contradicts the PR's stated contract ("Only csv2json/json2csv hydrate; other tools fall through to the converter normally") and is the premortem outcome for this feature: a 2013 sql2json link resolves to a blank converter with no explanation.
Fix: return the tool from parsePermalinkPath (e.g. {tool, id}) and gate hydration to csv2json/json2csv (plus csvjson2json, whose {csv, minify} shape maps correctly) — every other tool falls through to the plain converter with no fetch and no notice. Independently, only treat csv/json as an input key when its value is a non-empty string, so a boolean radio can never masquerade as input text. Add unit fixtures built from the real legacy view ids (sql2json, json_beautifier, json_validator) to pin the routing.
There was a problem hiding this comment.
Fixed in 3254f23. parsePermalinkPath now returns {tool, id} and hydration is gated to converter-shaped tools (csv2json, json2csv, csvjson2json) — sql2json, json_beautifier, json_validator, and datajanitor links fall through to the plain converter with no fetch and no notice. Independently, only a non-empty string now counts as a saved input, so sql2jsons json: true format radio can never masquerade as input text (it returns null → unsupported notice, never an empty hydrate). Unit fixtures built from the real legacy view shapes (sql2json {sql, json: true, javascript: false, minify: false}, json_beautifier {json: ...}, json_validator {result}) pin the routing; 105/105 tests green.
| const REAL_ID = "000c44f43e2f62cc15c48d9d7c5a4582"; | ||
|
|
||
| // Minimal import.meta.env shim — matches .env / code default. | ||
| (globalThis as Record<string, unknown>).import_meta_env = {}; |
There was a problem hiding this comment.
High · reliability — This shim is inert: globalThis.import_meta_env (underscored) cannot influence the import.meta.env expression in permalink.ts:35 — import.meta is syntax, not a global lookup. Run as committed, the script crashes at import:
$ npx tsx scripts/e2e-legacy-permalink.ts
TypeError: Cannot read properties of undefined (reading 'VITE_S3_DATA_URL')
at app/src/lib/permalink.ts:35:19
e2e exit: 1
So the PR body's headline test evidence ("Real-object E2E … E2E OK: real legacy object hydrated read-only") was not produced by the committed script — the criterion-6 evidence is not reproducible, and the claimed live-CORS verification is unverifiable. The same module-scope import.meta.env.VITE_S3_DATA_URL read also means permalink.ts throws at load in any non-Vite runtime.
Fix: guard the env read in permalink.ts:34–36 (e.g. (import.meta as { env?: Record<string, string | undefined> }).env?.VITE_S3_DATA_URL ?? DEFAULT), delete this shim line, re-run the E2E, and paste the actual output into the PR body.
There was a problem hiding this comment.
Fixed in 3254f23. The env read in permalink.ts now goes through an optional chain ((import.meta as { env?: ... }).env?.VITE_S3_DATA_URL ?? DEFAULT) with trailing-slash normalization, the inert globalThis.import_meta_env shim is deleted, and the script runs as committed — verified: npx tsx scripts/e2e-legacy-permalink.ts → E2E OK: real legacy object hydrated read-only against data/000c44f43e2f62cc15c48d9d7c5a4582 (fetched keys [csv, parseNumbers, parseJSON, transpose, output-array, output-hash, minify, result]; direction csv2json; options {"parseNumbers":true,"parseJSON":true,"transpose":false,"hash":false}; input head sonicMatchId;roundNumber;date;…). PR body updated with the actual output and live CORS verification (Access-Control-Allow-Origin: *, methods HEAD/GET).
| } else { | ||
| // Object exists but no longer maps onto the converter (e.g. a | ||
| // Data Janitor session) — same fallback as a missing object. | ||
| setPhase("not-found"); |
There was a problem hiding this comment.
Medium · reliability — When the object fetches fine but hydrateConverter returns null, this branch sets the same not-found phase used for real 403/404s, so the notice tells the user "This data doesn't exist (or was deleted)" for objects that are live in the bucket (json_validator saves, Data Janitor sessions, any future unmappable payload). On links that may be a decade old, that message is factually wrong and unnecessarily alarming.
Fix: distinguish the cases — e.g. an unsupported phase with copy like "This link's saved data can't be shown in the new converter" plus the link home — and reserve "doesn't exist (or was deleted)" for genuine 403/404.
There was a problem hiding this comment.
Fixed in 3254f23. The hook now has a distinct unsupported phase with its own notice: "This link\u2019s saved data can\u2019t be shown in the new converter" + link home. "This data doesn\u2019t exist (or was deleted)" is reserved for genuine 403/404. Component test added: a live Data-Janitor-style payload on a csv2json URL renders the unsupported notice and never the deleted copy.
Address the automated review (1 blocker, 1 high, 1 medium):
- parsePermalinkPath now returns {tool, id} and hydration is gated to
converter-shaped tools (csv2json, json2csv, csvjson2json). A sql2json
save's `json: true` format radio can no longer masquerade as input:
only a non-empty string counts as saved input, so a boolean radio
can never hydrate an empty json2csv converter. Fixtures built from
the real legacy view ids (sql2json, json_beautifier, json_validator)
pin the routing.
- Objects that exist but no longer map (Data Janitor sessions, any
unmappable shape) get a distinct "can't be shown in the new
converter" notice — "doesn't exist (or was deleted)" is reserved
for genuine 403/404.
- The env read goes through an optional chain with trailing-slash
normalization, so permalink.ts loads in non-Vite runtimes and the
committed E2E script runs as-is under tsx (inert globalThis shim
deleted). Unit fixtures pin the sql2json/json_beautifier/
json_validator view shapes.
There was a problem hiding this comment.
Obvious Code Review — Pass 2 (head 3254f23)
Verdict: COMMENT — No findings — clean pass.
All Pass 1 findings are resolved and verified on head:
- Blocker (permalink misroute) — Resolved. Hydration is now gated by the URL's tool segment (
parsePermalinkPathreturns{tool, id},isHydratableToolallows only csv2json/json2csv/csvjson2json); sql2json/json_validator/json_beautifier/datajanitor links fall through with no fetch and no notice, matching the stated contract. The second-layerisInputTextguard ensures a boolean format radio can never masquerade as input text. Unit fixtures built from the real legacy view ids pin all three Pass 1 failure cases. - High (E2E not reproducible) — Resolved. The inert shim is gone and the env read is optionally chained; re-ran the committed script — it fetches the real S3 object, hydrates csv2json, prints
E2E OK(exit 0). - Medium (false "deleted" copy) — Resolved. Distinct
unsupportedphase with honest copy; tests assert the not-found notice stays absent for live-but-unmappable objects. - Suggestion (trailing slash) — Resolved.
S3_DATA_URLis normalized, with a unit test.
Verified on head: 105/105 vitest, tsc -b clean, eslint clean, real-network E2E green.
|
All three findings fixed and pushed in 3254f23 — replies are in each findings thread. Verified while fixing: 105/105 vitest, |
|
Thanks — clean pass acknowledged. All four items verified fixed on head 3254f23 with 105/105 tests green; keeping the PR as the record of the tool-gate + unsupported-notice + runnable-E2E fixes. |
eb990f2
into
release/rebuild-csvjson-spa
…ration
parsePermalinkPath(window.location.pathname) ran during render and blew
up the build-time prerender ("window is not defined") after the rebase
onto #161. Guard with the codebase's standard typeof-window check —
the shell renders the default converter state server-side; the hook
already accepts null and only fetches in a client effect.
…red data (#162) * feat: prerender single route at build The SPA route is rendered to static HTML at build time (scripts/ prerender.mjs boots Vite in middleware mode, renderToStrings the App, and writes it into dist/index.html) — a build step, not SSR. The deployed index.html now carries the title, an sr-only H1 naming both directions, meta description, OG/Twitter tags, and canonical to /, so crawlers index the page without executing JavaScript. CI runs npm run build (which ends with the prerender) and then curls the served build, grepping for each on-page target; any miss fails the run (spec criterion 10). * feat: faq accordion and option hints Adds the collapsed-by-default "How it works & FAQ" accordion below the converter (how it works, what the options do, TSV vs CSV, and privacy — conversion never leaves the browser), plus info-icon hints on the non-obvious options (parse numbers, parse JSON, transpose, hash output, minify, flatten). Every answer and hint is rendered into the DOM at load; clicks only toggle visibility via the hidden attribute, so the prerendered shell carries the full copy for crawlers and nothing is mounted on click (spec: SEO disclosure pattern). FAQ copy lives in one module the accordion renders from. Tests pin the initial-DOM rule for both the FAQ and the option hints. * chore: structured data SoftwareApplication JSON-LD in the document head names the app and both conversion directions; the FAQ block carries FAQPage markup derived from the same items the accordion renders, so the machine-readable questions can never drift from the visible copy. Both are rendered unconditionally, so the prerendered HTML ships them regardless of UI state. CI greps for both blocks in the built index.html. * fix: neutralize the pre-mount input-loss window; FAQ aria linkage The prerendered shell is real markup a user can see before the bundle loads, and React's fresh mount discards #root's contents — anything typed during that window was silently wiped. An inline script now sets inert on #root before first paint and main.tsx lifts it right after createRoot(...).render(...), so the shell is never interactive while its state is disposable (review: Medium reliability). Also links the FAQ toggle to its region (useId + aria-controls), matching the OptionHint convention (review suggestion). * fix: guard window read so the prerender survives #161's permalink hydration parsePermalinkPath(window.location.pathname) ran during render and blew up the build-time prerender ("window is not defined") after the rebase onto #161. Guard with the codebase's standard typeof-window check — the shell renders the default converter state server-side; the hook already accepts null and only fetches in a client effect. --------- Co-authored-by: obvious-autobuild-staging[bot] <262703071+obvious-autobuild-staging[bot]@users.noreply.github.com>
1 participant
Why
Every legacy share link on csvjson.com (
/<tool>/<32-hex-id>, millions of form states saved since 2013) resolves today through the PHP front controller into a full-page render plus a client$.getJSONof the object. The rebuild's SPA must keep those URLs resolving or every saved permalink on the web goes dead. The approved spec ("Old share links keep resolving — read-only") is the contract: hydrate read-only, never rewrite the URL, never write back.What
/:tool/:idparsed in the SPA with the tool segment preserved ({tool, id}); hydration is gated to converter-shaped tools (csv2json,json2csv,csvjson2json). Legacy tools with no converter equivalent (sql2json, json_validator, json_beautifier, datajanitor) fall through to the plain converter with no fetch and no notice. The PHP front controller already passes<tool>/<id>through.app/src/lib/permalink.ts):fetchLegacyPermalink(id)GETsdata/<id>straight from S3 via the build-time constantVITE_S3_DATA_URL(defaulthttps://csvjson.s3.us-east-2.amazonaws.com/data/, matching the buckets today's CSP whitelists; trailing-slash normalized), read through an optional chain so the module loads in non-Vite runtimes too. 403/404 →PermalinkNotFoundError; other non-ok responses, network failures, and malformed bodies →PermalinkFetchError. Bad ids (not 32-hex) are rejected before any request.hydrateConverter(state)): the legacy{elementId: value}shape (e.g.{csv2json_csv, csv2json_json, parseNumbers, ...}) maps through ONE element-id → converter-state table keyed by tool prefix — no per-tool special-casing. Only a non-empty string counts as saved input, so a sql2jsonjson: trueformat radio can never masquerade as input text. Unmappable objects (e.g. Data Janitor sessions) surface a distinct unsupported notice, never the deleted copy. The URL is never rewritten; edits behave normally afterward.permalink-loading; missing/deleted id → inline "This data doesn't exist (or was deleted)" notice with a link to/; live object that no longer maps → distinct "This link's saved data can't be shown in the new converter" notice; network error → retry affordance that re-runs the same read-only GET. The converter stays mounted underneath in every case — never a blank page.app/srcforputObject|deleteObject|copyObjectand explicit write-method fetches, mirroring the existing zero-tolerance checks. The bucket's own CORS policy (Access-Control-Allow-Methods: HEAD, GET) enforces the same at the source.How to Review
app/src/lib/permalink.ts— path parsing (tool-aware), fetch classes, the mapping table (single source of truth for element ids; derived from the legacy modules per the spec's restore contract), and the HYDRATABLE_TOOLS gate.app/src/hooks/useLegacyPermalink.ts— fetch lifecycle: idle/loading/hydrated/not-found/unsupported/error + retry.app/src/App.tsx— wiring: hook + one-shot hydration effect; input/options/direction only.Test Evidence
Component tests (hydrated/not-found/retry/fallthrough/unsupported) + unit tests incl. legacy-tool fixtures: 105/105 passed on head 3254f23; lint/typecheck/build green; CI (ci + shim) green.
Real-object E2E as committed (
app/scripts/e2e-legacy-permalink.ts, no mocks) — actual output ofnpx tsx scripts/e2e-legacy-permalink.ts:Object
data/000c44f43e2f62cc15c48d9d7c5a4582(a real 2019 save, discovered via the bucket's public listing), fetched through the unmockedfetchLegacyPermalink+hydrateConverter. CORS verified live:Access-Control-Allow-Origin: *, methodsHEAD, GETonly — the bucket itself rejects writes.Trade-offs: hydration applies option values over current defaults like a paste (once); the converter stays mounted under the notice so failures are never a blank page; non-converter tools fall through with no notice per the spec contract.
🔗 Obvious Project · 🧵 Obvious Thread