build: generate icons from a Vite plugin and run the server as TypeScript

[arpitdalal]

Closes #8. One PR for both tasks, as agreed — they touch the same three files and splitting them would have left build:icons and esbuild half-wired.

Net: -1082 / +284 lines, and four scripts, one CI step × 3 jobs, and a build-output directory gone.

Task 1 — the icon sprite is a Vite plugin now

other/build-icons.ts (153 lines) shelled out to prettier after every write and had to be run by hand before anything else could work. vite-plugin-icons-spritesheet does the same job inside the Vite pipeline, which also means the sprite regenerates when a file in other/svg-icons is added, changed or deleted — the old script did not.

Gone: other/build-icons.ts, the build:icons and predev scripts, the sly.json postinstall hook, and the 🖼 Build icons step in all three CI jobs.

The react-router typegen step that typecheck runs does instantiate the Vite config, so the generated files exist before tsc does — verified from a clean rm -rf app/components/ui/icons, which is what let the CI steps go.

The sprite stays a separate request, with the preload. The issue's premise here was out of date: assetsInlineLimit returning false means don't inline, so the sprite was already one immutable-cached ~5 KB request that app/root.tsx preloads. That is the better of the two options the issue weighs (inlining would put ~7 KB of base64 in every document and make the preload dead weight), so nothing about the delivery changed. The only edit is dropping the favicon.svg branch, which has been dead since the favicon became a .png.

Task 2 — the server runs as TypeScript

other/build-server.ts and server/dev-server.js existed to hand Node a compiled copy of server/*.ts and to choose between it and the source. Node strips the types itself, so both are gone, along with server-build/, index.js (now index.ts), and build:server.

• index.ts unconditionally imports ./server/index.ts.
• The React Router request handler moves to server/app.ts, which is now the entry of the SSR build. vite build bundles it into build/server/index.js, and server/index.ts mounts it from there. One handler definition, two loading strategies: through Vite in development (which is what lets virtual:react-router/server-build resolve), from the build in production. That also deleted the hand-rolled getBuild() and the build: closure that existed only to swallow import errors.
• Relative imports in the server graph now carry their real extension, because Node does not remap .js to .ts.

⚖️ The decision you will want to weigh: dev still auto-restarts

server/dev-server.js wrapped the process in tsx watch, so editing server/ restarted it. Upstream dropped that and node index.ts leaves you to Ctrl-C. This PR keeps the watcher, per the issue's own recommendation:

cross-env NODE_ENV=development FORCE_COLOR=true tsx watch --clear-screen=false --inspect \
  --ignore ".cache/**" --ignore "app/**" --ignore "vite.config.ts.timestamp-*" \
  --ignore "build/**" --ignore "node_modules/**" ./index.ts

app/ stays on Vite's HMR, server/ restarts, tsx is a devDependency so it costs nothing in the shipped image, and the --inspect flag and FORCE_COLOR that the deleted file passed to its child are both preserved.

Two things in the issue that do not work as written

Both were flagged as risks in the issue, and both are documented in the code:

1. build.rollupOptions.input at the top level does nothing. @react-router/dev replaces it with its virtual server build in its own config hook — the SSR build came out as the plain ServerBuild with no app export. The override has to go in environments.ssr.build.rollupOptions.input, which is the key the plugin actually reads.
2. config.isSsrBuild is not set for the SSR environment build. With the Environment API the user config is resolved once for the whole build, so upstream's config.isSsrBuild ? './server/app.ts' : undefined evaluates to undefined and silently does nothing. The input is therefore unconditional.

One non-obvious consequence: noExternal: ['@react-router/express']

In development, server/app.ts is loaded by Vite, and React Router's development export condition gives Vite and Node two different builds of the package. @react-router/express is external by default, so it got Node's copy while the RouterContextProvider came from Vite's — and the handler's instanceof check failed on every request:

Invalid `context` value provided to `handleRequest`. You must return an instance
of `RouterContextProvider` from your `getLoadContext` function.

@react-router/express is therefore bundled into the SSR graph instead of externalized, so the handler and the provider share one module instance. This is specific to React Router 8, which requires getLoadContext to return a RouterContextProvider; upstream is on 7.16, has no such split, and carries no noExternal.

Smaller things

• tsconfig sets erasableSyntaxOnly: Node now runs the server graph directly, and tsx compiles an enum happily in development, so nothing would have caught one landing in server/ and taking production down at boot. verbatimModuleSyntax, the other setting Node's docs suggest, is deliberately not set — it would stop the type-only @/icon-name import from being elided and Vite cannot resolve that tsconfig paths alias. Both are commented.
• The generated icon types are name.ts, not name.d.ts. The plugin writes export const iconNames = [...] as const, and a const initializer in an ambient context is an error (TS1254) that only skipLibCheck was hiding.
• eslint ignores app/components/ui/icons/. It is gitignored, so CI never sees it — and a leftover name.d.ts from the old script made npm run lint fail on every existing checkout with "not found by the project service", because tsc drops a .d.ts that has a .ts beside it.
• The plugin's formatter: 'prettier' option is not set, against the issue's explicit instruction. Prettier 3 ignores .gitignored paths by default and that directory is gitignored, so format:check never reads those files — the option was dead config that cost two subprocess spawns per build. Proven: a deliberately misformatted file in other/ is reported, the same file in the icon directory is not.
• @sentry/node is declared in dependencies. It is imported directly by two server files and only resolved because @sentry/profiling-node hoists it.
• build collapses to react-router build now that it has one step, and the stale /server-build entries are out of .gitignore, .prettierignore, eslintIgnore, the Dockerfile and Sentry's filesToDeleteAfterUpload.
• Pruned with their last consumer gone: esbuild, execa (a runtime dep that only the dev wrapper used), fs-extra, glob, node-html-parser, @types/fs-extra.

How this was verified

npm run validate	green, including from a clean rm -rf build .react-router app/components/ui/icons
npm run build	green; build/server/index.js exports app, route chunks intact in build/client/assets
npm run test:run	136/136
npm start	/, /talks, /uses, /contact, /resources/healthcheck all 200; fly-region/fly-app, the per-request CSP nonce and Server-Timing all still present
npm run dev	all of the above plus /robots.txt and /sitemap.xml; an app/ edit is served without a restart, a server/ edit restarts the process
Icons	all 14 ids in the sprite; every <use href> on /, /talks, /uses, /contact resolves; sprite served 200 image/svg+xml with immutable
Docker	not built here — the build stage uses RUN --mount=type=secret, so it needs BuildKit, and this machine has no docker buildx. Left to the deploy job, which builds the image on Fly. As a substitute I reproduced the final image layout locally: the .dockerignore-respecting context, npm install --omit=dev, build/ copied in, no dev deps and no server-build — npm start served 200s on /, /talks, /uses, /contact and the healthcheck

Known trade-offs, deliberately

• npm start on an unbuilt tree now fails at boot instead of logging an error per request. getBuild()'s try/catch is gone because the issue called for it, and fail-fast is the better behaviour — but it presents on Fly as a crashloop with Cannot find module '../build/server/index.js' rather than a 5xx. Both server tests keep an explicit existsSync guard with a message naming npm run build.
• The build output is two files, not one. build/server/index.js is a 3.4 kB entry that lazily imports the React Router build from a hashed sibling chunk in build/server/assets/. Anything that copies, warms or uploads the server build has to take the whole build/server tree. That is called out in server/index.ts; the issue expected one inlined file.
• The test suite still needs npm run build first — tests/talk-card.test.tsx imports a component that imports the sprite, and vitest never runs the plugin's buildStart. Unchanged from main (the CI step that used to generate it is now npm run build), but worth stating.

Summary by CodeRabbit

• Build & Development
  • Updated application build and server startup workflows for development and production.
  • Icon sprites and icon-name types now regenerate during Vite builds, development sessions, and typechecks; no separate generation command is needed.
• Documentation
  • Updated guidance for configuring split icon sprites.
• Asset Handling
  • Favicon SVGs can now be inlined in generated assets.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 346f1a37-4b29-40c9-912a-e16c6f59e004

📥 Commits

Reviewing files that changed from the base of the PR and between 4b7771c and 874100e.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (20)

• .github/workflows/deploy.yml
• .gitignore
• .prettierignore
• app/routes/_seo+/sitemap[.]xml.ts
• eslint.config.js
• index.ts
• other/Dockerfile
• other/build-icons.ts
• other/build-server.ts
• other/sly/sly.json
• other/svg-icons/README.md
• package.json
• server/app.ts
• server/dev-server.js
• server/index.ts
• tests/allow-indexing.test.ts
• tests/server.smoke.test.ts
• tsconfig.json
• types/icon-name.d.ts
• vite.config.ts

💤 Files with no reviewable changes (8)

• .prettierignore
• other/sly/sly.json
• .gitignore
• .github/workflows/deploy.yml
• other/build-icons.ts
• server/dev-server.js
• other/Dockerfile
• other/build-server.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The change replaces the custom icon and server build scripts. Vite generates icon assets and builds the server app. Node runs the TypeScript server entry point, with separate development and production app-loading paths.

Changes

Build and server migration

Layer / File(s)	Summary
Vite icon generation other/build-icons.ts, other/sly/sly.json, vite.config.ts, other/svg-icons/README.md, types/icon-name.d.ts, tsconfig.json, eslint.config.js, .github/workflows/deploy.yml, package.json	The custom icon script and its postinstall and CI calls are removed. Vite generates the sprite and TypeScript icon names. The README, type references, lint ignores, and plugin dependency are updated.
Server request handling server/app.ts, server/index.ts, index.ts, server/dev-server.js, app/routes/_seo+/sitemap[.]xml.ts, tsconfig.json	The Node entry point now imports server/index.ts in all environments. Development requests load the app through Vite; production imports the built app. Type-stripping constraints are enabled in TypeScript configuration.
Build and runtime wiring vite.config.ts, other/build-server.ts, package.json, other/Dockerfile, .gitignore, .prettierignore, tests/*	The build uses React Router and Vite instead of the removed esbuild server build. Scripts, Docker contents, test entry points, ignore rules, and build dependencies are updated. Vite’s server build targets server/app.ts.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Refactor

Sequence Diagram(s)

sequenceDiagram
  participant Node as index.ts
  participant Server as server/index.ts
  participant Vite
  participant App as server/app.ts
  participant Router as React Router request handler
  participant BuiltApp as build/server/index.js
  Node->>Server: start server
  Server->>Vite: development request loads server/app.ts
  Vite-->>Server: return app
  Server->>App: forward request
  App->>Router: handle request
  Server->>BuiltApp: production imports built app

Loading

Merge Risk: ⚪ Minimal · up to 87410

No actionable merge-blocking issue was established; the change is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 87410

The server migration warrants design review, but the inspected request path keeps its existing security controls ahead of the application handler. No new security bypass was established; production-build behavior remains to be verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The handler-loading change can affect requests reaching the public React Router application, but the inspected mounts remain behind the outer server middleware rather than creating a separate public listener.

Trust Boundaries and Controls

• observed — Attacker-supplied request headers do not choose the rendering nonce: server middleware replaces the nonce header, Helmet uses it for CSP, and the rendering entrypoint reads it after that handoff.

Resilience and Maintainability Implications

• inferred — The production handler import occurs before the server begins accepting requests, so an unavailable bundle is a startup failure rather than a route that runs without the mounted handler. This was established from source, not a production-image run.

Hardening Proposals

• proposed — Verify the built production image starts with the generated handler and serves a representative route with the expected CSP nonce before rollout.

🚥 Pre-merge checks | ✅ 3 | ❌ 1 | ❓ 1

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 9 files. (3 skipped: 3…	Write docstrings for the functions missing them to satisfy the coverage threshold.
Linked Issues check	❓ Inconclusive	The visible changes address the coding requirements in [#8]: the Vite icon plugin replaces the custom script; generated icon names use kebab-case; obsolete scripts, CI steps, server wrapper, and `serv…	Provide reviewable evidence for the excluded package-lock.json state and for a clean install that regenerates the icon types and sprite, then runs the required build and tests.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes both primary changes: generating icons through a Vite plugin and running the server as TypeScript.
Out of Scope Changes check	✅ Passed	The reviewed changes stay within [#8]. Workflow, ignore-file, ESLint, TypeScript, README, server, Docker, package, test, and Vite changes directly support the icon pipeline replacement or the native N…

Full details: Linked Issues check

Explanation

The visible changes address the coding requirements in [#8]: the Vite icon plugin replaces the custom script; generated icon names use kebab-case; obsolete scripts, CI steps, server wrapper, and server-build references are removed; Node executes index.ts; server/app.ts supplies the React Router handler; explicit .ts imports and the Sentry sourcemap cleanup remain; SSR output and smoke tests use build/server/index.js; and tsx watch preserves development restarts. The sprite remains a separate asset, and the favicon exclusion is removed. However, package-lock.json is excluded from review, so dependency-lock consistency and the clean CI-style install and generation result cannot be established.

Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 9 files. (3 skipped: 3 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}