Skip to content

Migrate frontend from Vue to React + TypeScript + react-query - #179

Merged
skearnes merged 48 commits into
mainfrom
react
May 23, 2026
Merged

Migrate frontend from Vue to React + TypeScript + react-query#179
skearnes merged 48 commits into
mainfrom
react

Conversation

@bdeadman

@bdeadman bdeadman commented May 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

Port the Open Reaction Database web frontend from Vue 3 + Vuex + Pug + Sass + Vue CLI to React 19 + TypeScript + Vite + react-router 7 + TanStack Query 5 + SCSS. The Flask + FastAPI backend (ord_interface/) is unchanged; only the SPA under app/ was rewritten.

Why

The Vue app was on Vue CLI tooling and used a hand-rolled polling protocol against /api/submit_query + /api/fetch_query_result that had been the source of multiple bug fixes (#124, #129, #137, #143, #151, #152). React + TanStack Query lets us:

  • Type the protobuf models we render directly from ord-schema's *.AsObject types instead of any.
  • Use one shared useSearchTask hook that submits the task once per query string and lets react-query's refetchInterval drive polling, eliminating the per-view ref/interval/timeout bookkeeping the Vue code had duplicated across MainSearch and MainDatasetView.
  • Stay on the same Vite tooling the Vue app was on after Migrate SPA to Vite; adopt Prettier #176; the prettier config and dev-server proxy are carried over.

What changed under app/

  • React 19 + TypeScript everywhere. All routed views (Home, About, MainBrowse, MainSelectedSet, MainSearch, MainDatasetView, MainReactionView) and the reusable components (HeaderNav, MainFooter, EntityTable, ReactionCard, CompoundView, FloatingModal, ModalKetcher, DownloadResults, CopyButton, LoadingSpinner, ChartView, SearchOptions, SearchResults) are .tsx. The reaction-view sub-pages (SetupView, ConditionsView, NotesView, ObservationsView, WorkupsView, OutcomesView, ProvenanceView, EventsView) are ported with the same per-section detail rendering as the Vue originals. vue-app/ is removed.
  • Polling extracted. app/src/hooks/useSearchTask.ts runs the submit-query / poll-fetch_query_result protocol through react-query. Two-minute timeout preserved. Used by both MainSearch and MainDatasetView.
  • Types from ord-schema. app/src/types/search.ts re-exports Reaction.AsObject, ReactionConditions.AsObject, etc. as shared types so result rows and section views are no longer typed as any. app/src/utils/enum.ts adds enumName(map, value) to replace ~20 inline Object.keys(map).find(key => (map as any)[key] === value) patterns. Result: 80 → 0 @typescript-eslint/no-explicit-any errors.
  • Shared rendering helpers ported from Vue. app/src/utils/amount.ts (amountObj / amountStr for mass / volume / moles / unmeasured), app/src/utils/outcomes.ts (formattedTime, formatPercentage), and app/src/utils/conditions.ts (tempType, tempSetPoint, pressureType, pressureSetPoint, pressureAtmo, stirType, stirRate, illumType, lengthStr, electrochemType, flowType) replace inline stubs in CompoundView/MainReactionView and back the new section views.
  • Generic EntityTable<T>. The table render-prop callback now sees the real element type instead of any[].
  • Vite-only. app/vite.config.ts proxies /api to http://127.0.0.1:5000 (matches uvicorn's default bind and the README); the Vue-era vite.config.js is gone.

Bugs caught and fixed during the migration

Bugs the React port introduced (now fixed):

  • useSearchTask falsely fired the 120s timeout on first load under <StrictMode> dev. Two separate refs (taskIdRef, startTimeRef) plus a useEffect(…, [queryString]) reset created a race: React's strict-mode double-invocation reset startTimeRef to 0 between queryFn setting it and the next poll's timeout check, so Date.now() - 0 > 120_000 immediately. Collapsed all polling state onto a single ref keyed by the queryString that owns it; reset detection moved inside queryFn. (c751477)
  • MainSelectedSet only fetched one selected reaction. Used searchParams.get('reaction_id') (single value); switched to getAll('reaction_id') so multi-select downloads work — matches Vue post-Convert reactionIds to a list from a string as expected by API #148 behavior.
  • MainSelectedSet never deserialized protobuf. It was stubbing data: { identifiersList: [{ value: 'Reaction <id>' }] } so ReactionCard rendered placeholder text. Now does the same base64 → Reaction.deserializeBinary().toObject() flow the other views use.
  • /selected-set route was unreachable. SearchResults.tsx navigates to /selected-set?reaction_id=… but App.tsx never wired the route. Added.
  • dataset-view/SearchResults.tsx goToViewSelected built a reaction_ids=a,b,c URL while MainSelectedSet reads getAll('reaction_id') — the destination always saw an empty array. The wiring was also unreachable from the UI (isSelectable={false} hardcoded). Removed the dead state and handler. (ac96a5a)
  • MainDatasetView.isOverflow was hardcoded false, so any dataset with more than 100 reactions rendered the title as "Reactions in this Dataset (100 Reactions)" instead of "100 Reactions From This Dataset (Sample)". Now computed from datasetData.num_reactions > searchResults.length. (a76f4ea)
  • useSearchTask didn't gate /api/submit_query on response.ok — a 4xx/5xx body was JSON-parsed and assigned to taskIdRef as if it were a task ID. Added the same if (!submitRes.ok) throw guard the poll branch had.
  • ModalKetcher leaked its poll loop on unmount, and re-fired the loop after Ketcher loaded. getKetcher was a useCallback with [contWin, loading, drawSmiles] deps called from a useEffect; finding the iframe set contWin, which flipped the callback identity, which re-ran the effect with a fresh 30 s idle interval until the new setTimeout cleaned it up. Replaced with a single mount-time effect ([] deps) whose return clears both the interval and timeout. (6e61655)
  • search/SearchResults flashed empty for one frame on mount. formattedResults was a state mirror of the searchResults prop populated by a useEffect. Render directly from the prop; selection restore from localStorage stays in its own effect keyed only on location.search. (6e61655)
  • Ketcher 404'd in every production / Docker build. The vendored bundle was being copied into app/src/ketcher/ and fetched at runtime via /src/ketcher/asset-manifest.json. That works in npm run dev (the dev server serves all src/ files), but Vite only copies public/ verbatim into dist/, so production nginx saw nothing under /src/. Moved the bundle to app/public/ketcher/, KETCHER_BASE = '/ketcher', updated the Dockerfile cp target and the README, and dropped the matching hash-pinned @import '../../ketcher/static/css/main.<hash>.css' from MainKetcher.scss (Ketcher loads its own CSS inside the iframe, and the pinned hash was a separate version-rot trap). (464340d)
  • Ketcher rendered visibly broken in the iframe. ModalKetcher's iframe was pointing at /ketcher — our React Router route, which mounted a custom MainKetcher shell that fetched the asset-manifest and injected <script> / <link> tags. That booted Ketcher but resolved its own internal asset paths against the wrong base (the SPA's document, not the Ketcher bundle directory). The Vue app had the same architecture and the same symptom. Point the iframe at /ketcher/index.html — Ketcher's own self-contained shell with relative asset paths — and delete the MainKetcher view + route entirely. (fccb8c7)
  • SetupView always rendered the automation platform as "UNSPECIFIED" because it read setup.automation_platform (snake_case) instead of automationPlatform (the schema's AsObject convention).
  • ProvenanceView.experimentStart was always invalid Date. experimentStart is a DateTime.AsObject with a .value string field; the code passed the whole object to new Date(). Now reads .value.
  • ConditionsView Electrochemistry / Flow type rows rendered raw enum numbers ("3" instead of "GALVANOSTAT"). Added electrochemType / flowType enum helpers and wired them in.
  • viewKetcher/MainKetcher.tsx was a dead duplicate of the routed ketcher-view/MainKetcher.tsx (which is now itself removed; the iframe points directly at Ketcher's index.html). Deleted.
  • contribute/MainContribute.tsx was a stub that wasn't routed (the header links externally). Deleted.
  • utils/{amount,conditions,outcomes}.js were Vue-era helpers that the React app never imported. Deleted (the new TS versions replace them where the section views need them).
  • ord_interface/api/dump.rdb was a committed Redis dump file; deleted and added **/dump.rdb to .gitignore so it can't sneak back in from a subdirectory.
  • SearchOptions opened the Reaction Options accordion on every search page load that carried any URL params. Number(q.max_yield) !== 100 evaluated to NaN !== 100 === true whenever max_yield wasn't in the URL. Each bound is now gated on its URL param being present before being compared, with matching min_conversion / max_conversion checks. (d568ef3)
  • CompoundView and ChartView didn't gate /api/compound_svg on response.ok — a 4xx/5xx HTML error page would be fed to dangerouslySetInnerHTML in the compound SVG slot or chart tooltip. Same pattern that was fixed for reaction_summary in earlier rounds. CompoundView logs and bails; ChartView.getMolHtml throws so the call site's existing .catch sets molHtml to null. (d568ef3)
  • ChartView re-fetched /api/${apiCall} every time the chart panel was collapsed or expanded because isCollapsed / createChart were in the data-fetch effect's deps. Removed them — the separate resize effect already redraws on isCollapsed change. (d568ef3)

Bugs that pre-existed in the Vue app and were fixed in the port:

  • utils/conditions.stirRate passed the whole StirringRate object to a function that compared it to numeric enum values, so the Conditions → Stirring "Rate" row always rendered as undefined. The TS port reads rate.type explicitly.
  • ConditionsView peak wavelength read peakWaveLength but the schema's AsObject is peakWavelength, so the Illumination tab always showed "None". The TS port reads the right field and formats with lengthStr().
  • ConditionsView distance to vessel rendered the Length object directly (showed [object Object]). The TS port formats it via lengthStr().
  • OutcomesView CUSTOM measurements compared the type-name string to an undefined identifier CUSTOM, so the CUSTOM-measurement details modal was unreachable. The TS port compares against the literal string.
  • Ketcher rendered visibly broken in the iframe — the Vue app pointed the iframe at the SPA's own /ketcher route, same root cause as the React regression above. Fixed in this PR for both architectures.

Recent Vue bug fixes verified preserved in the port

Vue fix Where it lives in React
#124 ModalKetcher modal not appearing (display: block) app/src/components/ModalKetcher.scss
#124 SearchOptions null-safe ?.length checks app/src/views/search/SearchOptions.tsx
#129 Long-running query submit/poll protocol app/src/hooks/useSearchTask.ts
#132 Reaction viewer uses reaction_ids (plural) MainReactionView.tsx getReactionData
#137 Stale poll callback dedup Handled by react-query's queryKey lifecycle
#143 Treat all >= 500 as terminal useSearchTask.ts throws on non-200/202
#145 DownloadResults uses reaction_ids + "Download Results" title DownloadResults.tsx
#147 selected-set uses reaction_ids MainSelectedSet.tsx
#148 reactionIds normalized to a list MainSelectedSet.tsx uses .getAll()
#151 / #152 Search/dataset results not cleared on timeout react-query keeps last successful data across refetches
#159 "Mined" badge on ReactionCard ReactionCard.tsx provenance?.isMined

Infrastructure

  • Dockerfile fix for CI (9553df2): vite build was dying in the test_app job with Cannot find module @rollup/rollup-linux-x64-gnu (npm cli#4828 — package-lock.json captures platform-specific optional binaries by host OS, and ours was regenerated on macOS). The Dockerfile now rm -f package-lock.json before npm install so the Linux binary is resolved at build time.
  • Ketcher copy target moved to app/public/ketcher/ (464340d) so Vite copies it into dist/; the Dockerfile cp -r .../standalone/ ... target and the README extraction path are updated accordingly.
  • Served root renamed /app/ord-interface/vue/app/ord-interface/spa and nginx.conf updated to match.
  • .dockerignore added so a local macOS node_modules/ can't sneak into the build context.
  • End-to-end Dockerfile build verified locally — ran the full docker build -f ord_interface/Dockerfile -t openreactiondatabase/ord-interface:react-test .. --build-arg=ARCH=aarch_64; image lands dist/ketcher/asset-manifest.json, dist/ketcher/index.html, and dist/ketcher/static/js/main.<hash>.js under the served root.

Known gaps

  • The four dangerouslySetInnerHTML call sites (ReactionCard reactionTable, MainReactionView reactionSummary, CompoundView compoundSVG, ChartView molHtml) trust the FastAPI render endpoints. Same trust assumption as the Vue app.
  • ConditionsView Electrochemistry and Flow tabs render type + details + headline fields; the schema's deeper fields (current / voltage / electrode separation / tubing dimensions) are still placeholders behind a TODO comment. The Vue versions had the same TODO markers.
  • Temperature / pressure measurement lists render a count rather than per-measurement detail (matches the Vue TODO placeholder).
  • The proto3 zero-default ambiguity for fields like targetPh / conditions.ph (0 means "unset" or "actually strongly acidic"): the React port falsy-gates on 0 to match Vue's v-if='ph' behavior. Documented inline; switching to a has* accessor on the underlying message is a follow-up if/when real records ever set pH 0 explicitly.
  • No tests were added for the React migration. (The Vue app didn't have any either.)

Quality gates

tsc -b               # 0 errors
eslint .             # 0 errors, 0 warnings
prettier --check .   # all matched files use Prettier code style
addlicense -check    # all tracked files carry the project license header
vite build           # builds dist/ (~920 kB JS, ~25 kB gzip CSS), with dist/ketcher/ present
docker build         # full Dockerfile completes locally (linux/arm64, ARCH=aarch_64)
pre-commit run       # addlicense / ruff / ty / prettier all pass

Engine note: Vite 7 prefers Node 20.19+ / 22.12+; the build succeeds on 22.5.1 but emits an engine warning.

Test plan

  • cd app && npm install && npm run dev and load /, /about, /browse, /search, /dataset/<id>, /id/<reaction_id>, /selected-set against the bundled test database (ORD_INTERFACE_TESTING=TRUE uv run uvicorn ord_interface.api.main:app --port 5000 --reload).
  • Search by component SMILES and verify polling resolves and renders cards with yield/conversion/conditions populated from the schema.
  • Select 2+ reactions on the search results page, click "View N selected reactions", and verify all of them load on /selected-set and decode to real SMILES (not "Reaction " placeholders).
  • Open /dataset/<id> and verify the two frequency charts render, the reaction list resolves within the 120s polling window, and a dataset with more than 100 rows shows the "Sample" title variant.
  • On a reaction view, walk every section tab (Identifiers, Inputs, Setup vessel/environment/automation, Conditions temperature/pressure/stirring/illumination/electrochemistry/flow/other, Notes, Observations, Workups, Outcomes, Provenance, Record Events, Full Record) and confirm fields render with real values rather than placeholders.
  • On an outcome with a CUSTOM measurement, click CUSTOM and verify the custom-details modal opens.
  • Open the Ketcher modal from SearchOptions and verify the iframe loads Ketcher's own index.html (toolbar + canvas render correctly), accepts a structure, and returns SMILES on Save.
  • Click "Download Search Results" on a non-empty search and verify the resulting .pb.gz opens.

🤖 Generated with Claude Code

Greptile Summary

This PR replaces the Vue 3 + Vuex + Vue CLI SPA with React 19 + TypeScript + Vite + react-router 7 + TanStack Query, leaving the Flask/FastAPI backend untouched. The migration also fixes a substantial backlog of pre-existing bugs (wrong protobuf field names, stale enum formatting, unreachable routes, incorrect URL parameter shapes, and Ketcher iframe asset resolution).

  • useSearchTask consolidates all polling state onto a single per-queryString ref, eliminating the StrictMode double-invocation timeout false-positive; every fetch now checks response.ok before consuming the body.
  • Infrastructure: Ketcher's vendored bundle moves to app/public/ketcher/ so Vite includes it in dist/; package-lock.json is regenerated inside the Docker image to resolve Linux-specific @rollup/rollup-linux-x64-gnu; nginx root updated to /app/ord-interface/spa.
  • Remaining note: ChartView reads datasetId directly from window.location.pathname instead of receiving it as a prop (see inline comments).

Confidence Score: 5/5

This is a large but carefully executed frontend rewrite; all previously flagged defects have been corrected and no new blocking issues were found.

Every prior round's findings are demonstrably fixed in the current HEAD. The only remaining notes are in ChartView, where datasetId is read from window.location.pathname instead of props and the stats-endpoint fetch lacks a response.ok guard; both are non-blocking style issues that don't affect correctness under the current route structure.

app/src/views/dataset-view/ChartView.tsx — two minor robustness issues; all other changed files reviewed without concerns.

Important Files Changed

Filename Overview
app/src/hooks/useSearchTask.ts Core polling hook — single-ref design correctly prevents StrictMode timeout false-positive; submit_query ok guard and deduplication are solid.
app/src/components/ModalKetcher.tsx Mount-scoped polling effect correctly clears both interval and timeout on unmount; iframe now points at /ketcher/index.html.
app/src/views/search/SearchOptions.tsx NaN-on-undefined auto-open bug fixed by gating each bound on the URL param being present; default value handling is correct.
app/src/views/dataset-view/ChartView.tsx Reads datasetId from window.location.pathname instead of props, bypassing React Router; also missing response.ok guard on the stats fetch.
app/src/views/dataset-view/MainDatasetView.tsx isOverflow now correctly computed as (datasetData?.num_reactions ?? 0) > searchResults.length.
app/src/components/ReactionCard.tsx response.ok guard added before calling response.text(), preventing HTML error body injection via dangerouslySetInnerHTML.
app/src/views/reaction-view/MainReactionView.tsx response.ok guard in getReactionSummary returns empty string on error instead of injecting raw error HTML.
app/src/views/browse/selected-set/MainSelectedSet.tsx Correctly uses searchParams.getAll('reaction_id') and performs full protobuf deserialization; /api/reactions ok guard in place.
app/src/App.tsx All routes wired including the previously missing /selected-set; QueryClientProvider wraps the router correctly.
ord_interface/Dockerfile Ketcher bundle now copied to app/public/ketcher/ so Vite includes it in dist/; package-lock.json removed before npm install to resolve Linux rollup binaries.
ord_interface/nginx.conf Serves SPA from /app/ord-interface/spa with try_files fallback to index.html for React Router; Ketcher static files served correctly.

Reviews (11): Last reviewed commit: "Guard compound_svg fetches and fix React..." | Re-trigger Greptile

Michael Kennel and others added 30 commits September 23, 2025 15:34
- Drop dump.rdb (removed in main).
- Keep vue-app/ scaffolding (.eslintignore, .eslintrc.js, babel.config.js,
  public/index.html, vue.config.js) preserved as Vue-CLI reference; main's
  vite migration only updated the new app/ tree.
- app/package.json: keep React + Vite + TypeScript stack; adopt main's
  prettier setup (prettier devDep, format / format:check scripts).
- app/eslint.config.js: keep React + tseslint flat config; adopt main's
  license header, additional ignores (node_modules/, src/ketcher/), and
  CI-aware no-console / no-debugger rules.
- app/index.html: keep React entry point (#root, /src/main.tsx, favicon);
  apply main's prettier formatting (singleAttributePerLine).
- app/README.md: keep the React+Vite template notes.
- README.md: take main's restructured copy; relabel Vue references as
  React and note vue-app/ as transitional reference.
- app/package-lock.json: regenerate against the merged dependency set.
The Vue scaffolding under vue-app/ was kept during the React migration as a
reference; with the React app in place there's nothing left consulting it.
Drop the directory and the corresponding README bullet.
The two preceding commits (98d81f2, 0fc63ff) were authored with Claude's
assistance but landed without the standard trailer. Recording the
attribution here since rewriting those commits would require force-push.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI fix:
- Dockerfile / nginx.conf: the test_app CI job was failing in vite
  build with "Cannot find module @rollup/rollup-linux-x64-gnu". The
  package-lock.json captures rollup's platform-specific optional
  binaries by host OS, and the lockfile was regenerated on macOS during
  the merge-conflict pass, so the Linux binary never landed in it.
  npm cli#4828 covers the bug. Work around it by removing
  package-lock.json before `npm install` inside the image, so npm
  resolves the Linux-native rollup binary at build time.
- Also rename the served SPA root from /app/ord-interface/vue to
  /app/ord-interface/spa (it's React now, not Vue) and update
  nginx.conf to match.
- Add .dockerignore so local `node_modules/` / `dist/` / `.venv/` /
  test caches stay out of the build context (a macOS-built
  node_modules in there would still pull the wrong rollup binary even
  with the lockfile rebuilt).

Review fixes:
- ConditionsView Electrochemistry / Flow type rows rendered the raw
  numeric enum value ("3") instead of the name. Add electrochemType
  and flowType helpers in utils/conditions and use them.
- OutcomesView preserved productsIdx / analysesIdx / modal state when
  the user switched the outer Outcome tab in MainReactionView. Pass
  key={outcomesTab} so each tab gets a fresh OutcomesView and stale
  indices can't reference a now-undefined product.
- OutcomesView measurements grid rendered the Type / Value / Analysis
  / Raw header row even when the outcome had no products. Wrap the
  whole measurements block plus the per-product CompoundView in a
  `{currentProduct && (…)}` guard.
- Add utils/outcomes.formatPercentage and use it from
  ReactionCard.getYield, ReactionCard.getConversion, and
  OutcomesView's Conversion row + percentage measurements, so all
  four sites format the percentage identically (X% or X% ± Y, rounded
  to one decimal).
- WorkupsView.getNameIdentifier: replace the magic number 6 with
  reaction_pb.CompoundIdentifier.CompoundIdentifierType.NAME.
- WorkupsView.targetPh and ConditionsView "other" pH: add comments
  explaining the proto3 zero-default tradeoff (0 is ambiguous between
  "unset" and "actually strongly acidic"). Keep the Vue-equivalent
  `!== 0` falsy gate for now; switching to a `has*` accessor on the
  underlying message is a follow-up if/when records ever set pH 0
  explicitly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@skearnes

Copy link
Copy Markdown
Member

@greptileai review

Comment thread app/src/views/dataset-view/SearchResults.tsx Outdated
Comment thread app/src/hooks/useSearchTask.ts Outdated
Comment thread app/src/components/ModalKetcher.tsx
Comment thread app/src/views/dataset-view/SearchResults.tsx Outdated
P1 - dataset-view/SearchResults selected-set URL mismatch
  goToViewSelected built `/selected-set?reaction_ids=a,b,c` (single
  comma-joined param), but MainSelectedSet reads
  `searchParams.getAll('reaction_id')` — so the destination always saw
  an empty array. The selection wiring in this view was also
  unreachable from the UI (`isSelectable={false}` is hardcoded on every
  ReactionCard, and the persisted `sessionStorage` query string can't
  match a `/dataset/<id>` URL). Drop the dead `selectedReactions` /
  `goToViewSelected` / `useEffect`-from-sessionStorage block entirely;
  the view now just renders the dataset's reactions and a download
  button. (Vue had the same dead state — same conclusion.)

P2 - useSearchTask doesn't gate submit_query on response.ok
  A 4xx/5xx from /api/submit_query was being JSON-parsed and assigned
  to taskIdRef as if it were a task ID, so the next poll request
  attached the error body as `task_id` and the user saw a confusing
  "Search task <html> failed (HTTP …)" message. Add the same
  `if (!submitRes.ok) throw` guard the poll branch already has.

P2 - ModalKetcher polling not cleaned up on unmount
  getKetcher created a 1s setInterval and a 30s setTimeout but never
  returned them for cleanup. If the user closed the modal before the
  iframe's `window.ketcher` showed up, the interval kept firing and
  calling setLoading/setContWin on an unmounted component. Return a
  cancel function from getKetcher and call it from the effect's
  cleanup.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@skearnes

Copy link
Copy Markdown
Member

@greptileai review

The hook kept `taskIdRef` / `startTimeRef` in two separate refs and
reset them from a `useEffect(…, [queryString])`. In dev under
<StrictMode> React deliberately invokes effects twice (mount → cleanup
→ mount again). When that second invocation landed between queryFn
setting `startTimeRef = Date.now()` and the first poll's timeout
check, `startTimeRef` got reset to 0 — so
`Date.now() - 0 > POLL_TIMEOUT_MS` fired immediately and the user saw
"Failed to load reactions: Search task <id> timed out after 120s" on
the first attempt, regression vs the Vue version.

Collapse all polling state onto a single ref that carries the
queryString that owns it, and detect resets inside queryFn itself by
comparing the stored queryString. No separate useEffect → no
race-vs-StrictMode.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread app/src/views/dataset-view/MainDatasetView.tsx
isOverflow was hardcoded false, so a dataset with more than 100
reactions rendered the title as "Reactions in this Dataset (100
Reactions)" instead of "100 Reactions From This Dataset (Sample)".
Compare datasetData.num_reactions against the fetched search results
length, matching the Vue template's behavior.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@skearnes
skearnes marked this pull request as ready for review May 22, 2026 03:19
@skearnes
skearnes requested a review from mikennel May 22, 2026 03:19
Two leftover findings from the Greptile review summary:

search/SearchResults.tsx
  formattedResults was a state mirror of the searchResults prop populated
  by a useEffect. On the first commit, formattedResults was still []
  while searchResults already had rows, so the EntityTable + selection
  block flickered empty for a frame before re-rendering. Drop the
  state entirely and render directly from the prop; selection
  restoration from localStorage stays in its own effect keyed only on
  location.search.

ModalKetcher.tsx
  getKetcher was a useCallback with [contWin, loading, drawSmiles] in
  its deps, called from a useEffect that re-fired whenever those
  changed. When the interval succeeded it set contWin + loading, which
  flipped the callback identity and re-ran the effect — starting a
  fresh interval that ran idle (the body short-circuits on `contWin`
  being set) for 30 seconds until the new setTimeout cleaned it up.
  Move the poll into a single mount-time effect with [] deps so it
  fires once, captures the iframe handle once, and cleans up on
  unmount — no idle restart.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread app/src/views/ketcher-view/MainKetcher.tsx Outdated
skearnes and others added 2 commits May 21, 2026 23:46
Greptile flagged this: MainKetcher.tsx was fetching
`/src/ketcher/asset-manifest.json` and injecting a `<script>` whose
src lived under `/src/ketcher/...`. That worked in `npm run dev`
because the Vite dev server serves all `src/` files, but in
production / Docker only `public/` is copied verbatim into `dist/`.
Nothing in TypeScript imports `src/ketcher/`, so Vite never bundles
it, nginx 404s the manifest, and the iframe stays blank.

Move the vendored bundle target to `app/public/ketcher/` so Vite
copies it into `dist/ketcher/`, and update everything that pointed at
the old path:

- MainKetcher.tsx: `KETCHER_BASE = '/ketcher'` and a comment
  explaining the public/ choice.
- MainKetcher.scss: drop the hardcoded `@import
  '../../ketcher/static/css/main.8e693d51.css'`. Ketcher's bundle
  loads its own CSS when its main.<hash>.js boots inside the iframe,
  so the host stylesheet shouldn't be pulling it in — and the
  hash-pinned path was a separate version-rot trap on top of the
  build-time miss.
- Dockerfile: `cp -r .../standalone/ app/public/ketcher` instead of
  `app/src/ketcher`.
- README: point users at `./app/public/ketcher/` for local extraction.
- eslint.config.js + .prettierignore: switch ignore patterns
  accordingly (the latter already covers `public/`, drop the
  redundant `src/ketcher/`).

`vite build` now emits `dist/ketcher/asset-manifest.json` and the
`static/js/main.<hash>.js` it points at, so the runtime fetch +
script-injection path resolves end-to-end in production.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ModalKetcher iframe was pointing at `/ketcher` — our React Router
route, which rendered a custom MainKetcher shell that fetched
asset-manifest.json and injected `<script>` / `<link>` tags. That made
Ketcher boot, but its bundle then resolved its own assets (fonts,
sprites, dynamic chunks) relative to the surrounding React document,
which is at `/ketcher` without a trailing slash and lives inside our
SPA's index.html — wrong base, so Ketcher rendered visibly broken. The
Vue app had the same architecture and the same symptom.

Ketcher's standalone zip ships its own index.html with relative paths
(`./static/js/main.<hash>.js`, `./static/css/main.<hash>.css`,
`./favicon.ico`, …). Point the iframe at `/ketcher/index.html`
instead, and every relative path resolves correctly against
`/ketcher/static/…`. No runtime manifest fetch, no script injection,
no shell.

- Update ModalKetcher's iframe src to `/ketcher/index.html`.
- Remove the `/ketcher` React Router route and the
  `noHeaderFooter` branch in App.tsx (only the now-deleted route
  consumed it).
- Delete `app/src/views/ketcher-view/MainKetcher.tsx` +
  `MainKetcher.scss` — Ketcher hosts itself now.

Verified locally: `curl /ketcher/index.html` serves Ketcher's HTML
through Vite dev, `/ketcher/static/js/main.<hash>.js` and
`main.<hash>.css` both return 200, and the modal renders Ketcher's
toolbar / canvas properly (no more misrendered editor).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread app/src/components/ReactionCard.tsx Outdated
Comment thread app/src/views/reaction-view/MainReactionView.tsx
skearnes and others added 4 commits May 22, 2026 00:05
Greptile P1 (×2): both ReactionCard and MainReactionView fetched
`/api/reaction_summary` and piped the response text straight into
`dangerouslySetInnerHTML` without checking response.ok. A non-2xx
return — backend error, invalid reaction ID, etc. — yields an HTML
error page that would then render verbatim inside the card / the
reaction summary panel.

Add the same `if (!response.ok)` guard at both call sites: log the
status and bail (ReactionCard leaves its existing LoadingSpinner up,
MainReactionView returns an empty string so the summary section
stays blank).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces a client-side filter over /api/datasets with a direct
/api/dataset?dataset_id=... lookup, and renders the error message when
the metadata request fails instead of silently showing fallback values.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the hardcoded 3 in ReactionCard with
reaction_pb.ProductMeasurement.ProductMeasurementType.YIELD so a schema
renumber would fail at compile time. Skips the "± 0" suffix in
tempSetPoint and pressureSetPoint when precision is the proto3
zero-default.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile flagged two remaining gaps:

types/search.ts: the backend tags each search-result row with a
parent dataset_id and ReactionCard renders a dataset link from it,
but the SearchResult interface didn't have the field. ReactionCard
papered over it with `reaction: SearchResult & { dataset_id?: string }`.
Move the optional field onto the interface and drop the intersection
workaround in ReactionCard.

hooks/useSearchTask.ts: two concurrent queryFn invocations (StrictMode
dev double-invoke, a manual refetch racing a polling refetch, etc.)
could both see taskRef.current.taskId === null and each fire its own
`/api/submit_query`, leaving the loser's task orphaned on the
backend. Carry the in-flight submit promise on the ref so the
second caller awaits the first one's result instead of submitting a
duplicate. Worst outcome reduces from "duplicate request" to "no
duplicate".

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread app/src/views/search/SearchOptions.tsx Outdated
Comment thread app/src/views/reaction-view/CompoundView.tsx
- CompoundView, ChartView: skip response.ok=false bodies before passing
  them to dangerouslySetInnerHTML; matches the reaction_summary fix.
- ChartView: drop isCollapsed/createChart from the data-fetch effect's
  deps so toggling collapse doesn't refetch.
- SearchOptions: Number(q.max_yield) !== 100 evaluated to true when
  max_yield was absent (NaN !== 100), so the Reaction Options panel
  expanded on every search page load. Gate each bound on its URL param
  being present before comparing, and check conversion bounds too.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@mikennel mikennel left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple non-blocking things to fix in the future (maybe make some gh issues for these):

  • ChartView reads window.location.pathname directly, bypassing React Router. It should instead receive params from parent component
  • ChartView should have a response.ok guard. On a 4/5xx error, response.json() will try to parse the error html, providing nonsense to the downstream functions/chart.

@skearnes
skearnes merged commit 1bf7727 into main May 23, 2026
16 checks passed
@skearnes
skearnes deleted the react branch May 23, 2026 22:55
@skearnes

skearnes commented May 23, 2026

Copy link
Copy Markdown
Member

Filed for follow-up:

🤖 Generated with Claude Code

skearnes added a commit that referenced this pull request Jul 10, 2026
The React about page listed "Steven Kearnes (Relay)". #162 corrected this
to Genesis in ord_interface/about.html, but the Vue-to-React migration
(#179) rebuilt the page from an older source and reintroduced the stale
affiliation.

ord_interface/about.html is unreachable: nginx serves / from
/app/ord-interface/spa, the Dockerfile copied the file to
/app/ord-interface/ord_interface/, and the Flask app exposes only
/ketcher. Nothing links to it. Drop the file and its COPY directive.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants