Build client-side Ashby Plot Studio with Tailwind, shadcn UI, and Plotly.js#5
Build client-side Ashby Plot Studio with Tailwind, shadcn UI, and Plotly.js#5
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces the starter home page with a fully client-side “Ashby Plot Studio” that lets users edit a JSON config and preview/export Plotly-based Ashby plots directly in the browser, alongside adding lightweight shadcn-style UI primitives and Plotly-related dependencies/types.
Changes:
- Replaced
app/routes/home.tsxwith a JSON-driven plot studio including config editing, validation, and a live Plotly preview + export. - Added minimal UI primitives (
Button,Card) and a Tailwind class merging helper (cn). - Added Plotly + react-plotly + UI utility dependencies and TypeScript declaration shims for dynamic imports.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds Plotly/react-plotly and UI utility dependencies. |
| package-lock.json | Locks the newly added dependency graph (notably large due to Plotly ecosystem deps). |
| app/types.d.ts | Adds module declarations to support dynamic imports for Plotly + react-plotly factory. |
| app/routes/home.tsx | Implements the client-side plot studio with config editor, validation, and Plotly rendering/export. |
| app/lib/utils.ts | Adds cn() helper via clsx + tailwind-merge. |
| app/components/ui/card.tsx | Adds Card UI primitives. |
| app/components/ui/button.tsx | Adds Button primitive with CVA variants and Radix Slot support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "lucide-react": "^1.8.0", | ||
| "plotly.js-dist-min": "^3.5.0", | ||
| "react": "^19.2.4", | ||
| "react-dom": "^19.2.4", | ||
| "react-router": "7.14.0" | ||
| "react-plotly.js": "^2.6.0", | ||
| "react-router": "7.14.0", |
There was a problem hiding this comment.
Adding plotly.js-dist-min alongside react-plotly.js results in both plotly.js (auto-installed as a peer dependency) and plotly.js-dist-min being present in the lockfile, which significantly increases install size/time. Consider satisfying the peer dependency with a single package (e.g., depend on plotly.js and lazy-load it, or use an npm alias so the peer is met without installing both).
| const [{ default: createPlotlyComponent }, plotly] = await Promise.all([ | ||
| import("react-plotly.js/factory"), | ||
| import("plotly.js-dist-min"), | ||
| ]); | ||
| if (alive) setPlot(() => createPlotlyComponent(plotly as any)); |
There was a problem hiding this comment.
The Plotly factory is being initialized with the module namespace object returned by import("plotly.js-dist-min"), not the Plotly instance itself. createPlotlyComponent expects the Plotly object (the module's default export), so this will break at runtime unless you pass plotly.default (or destructure { default: Plotly } from the import).
| const [{ default: createPlotlyComponent }, plotly] = await Promise.all([ | |
| import("react-plotly.js/factory"), | |
| import("plotly.js-dist-min"), | |
| ]); | |
| if (alive) setPlot(() => createPlotlyComponent(plotly as any)); | |
| const [{ default: createPlotlyComponent }, { default: Plotly }] = await Promise.all([ | |
| import("react-plotly.js/factory"), | |
| import("plotly.js-dist-min"), | |
| ]); | |
| if (alive) setPlot(() => createPlotlyComponent(Plotly)); |
| if (parsed.config.version !== 3) { | ||
| errors.push(`Config version must be 3. Got ${parsed.config.version}.`); | ||
| return { data: [], layout: {}, errors }; | ||
| } | ||
|
|
||
| const dataframe = parsed.config.dataframes[selectedDf]; | ||
| if (!dataframe) { |
There was a problem hiding this comment.
parsed.config is cast to AppConfig without runtime validation. If the JSON has version: 3 but dataframes is missing/not an array, parsed.config.dataframes[selectedDf] will throw and crash the route. Consider validating that dataframes/frames/materials are arrays (and that required keys exist) before indexing, and return an error instead of throwing.
| return { data, layout, errors }; | ||
| }, [parsed.config, selectedDf, selectedFrame]); | ||
|
|
||
| const frameOptions = parsed.config?.dataframes[selectedDf]?.frames ?? []; |
There was a problem hiding this comment.
frameOptions uses optional chaining only on parsed.config, but not on dataframes itself. If dataframes is undefined/null (or not an array) this expression will still throw when evaluating dataframes[selectedDf]. Use optional chaining with ?.[index] (e.g., parsed.config?.dataframes?.[selectedDf]?.frames) to avoid runtime crashes while editing JSON.
| const frameOptions = parsed.config?.dataframes[selectedDf]?.frames ?? []; | |
| const frameOptions = parsed.config?.dataframes?.[selectedDf]?.frames ?? []; |
| ], | ||
| }; | ||
|
|
||
| type Mode = "default" | "max" | "min" | "span"; |
There was a problem hiding this comment.
Mode includes "max" and "min", but resolveQuantity currently only treats "span" specially and otherwise falls back to a single-value lookup. Either implement "max"/"min" handling or drop these modes from the type so configs can't specify unsupported behavior.
| type Mode = "default" | "max" | "min" | "span"; | |
| type Mode = "default" | "span"; |
| if (line.type === "slope" && line.x0 != null && line.y0 != null && line.slope != null) { | ||
| const x1 = line.x0 * 5; | ||
| const y1 = line.y0 * Math.pow(x1 / line.x0, line.slope); | ||
| shapes.push({ type: "line", x0: line.x0, y0: line.y0, x1, y1, xref: "x", yref: "y", line: { color: line.color ?? "#64748b", width: line.width ?? 1.5, dash: "dot" } }); |
There was a problem hiding this comment.
The "slope" guideline computes y1 using a power-law formula (y ∝ x^slope), which is correct for log-log plots but incorrect for linear axes. If log_x/log_y can be false, consider computing slope guidelines differently based on axis type (or explicitly restrict slope guidelines to log scales and validate accordingly).
| function Button({ | ||
| className, | ||
| variant, | ||
| size, | ||
| asChild = false, | ||
| ...props |
There was a problem hiding this comment.
Button doesn't set a default type, so when used inside a <form> without an explicit type it will default to submit and can trigger unintended form submissions. Consider defaulting to type="button" when asChild is false and no type is provided.
Motivation
Description
app/routes/home.tsxthat includes a config editor, dataframe/frame selectors, version validation (version === 3), inline errors, and a live Plotly preview.x/ydivided by another field), optional log axes, legend placement, guideline overlays (vertical/horizontal/slope), colored-area overlays, and annotations with arrows.app/components/ui/button.tsx,app/components/ui/card.tsx, andapp/lib/utils.ts(cnutil usingclsx+tailwind-merge).app/types.d.tsto allow dynamic imports ofplotly.js-dist-minandreact-plotly.js/factory, and updatepackage.json/ lockfile to include Plotly, react-plotly, UI utilities, icon package, and related type packages.Testing
npm run typecheckwhich completed successfully after adding the necessary type packages and declaration wrappers.npm run buildwhich completed successfully; the production build emits an expected large-chunk warning due to the Plotly runtime being included, but the build finished without errors.Codex Task