docs(web): add frontend architecture conventions (CONVENTIONS.md)#30683
Conversation
Covers domain-based code organization, Zustand state management, route-driven component boundaries, component patterns (hooks vs components, thin orchestrators, pure handlers, ref-stabilized callbacks), framework-agnostic domain logic, React Query data fetching, testing conventions, and dead code cleanup rules. Each section links to authoritative references (React docs, Zustand docs, React Router docs, Vite docs) so guidance stays verifiable. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de975651bf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Barrel files: never use (was: use for multi-file modules) - Remove barrel index.ts from streaming example - Soften top-level prohibition: nearest common ancestor can be top-level - Dead code: unrelated dead code gets its own separate PR, never just an issue Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
…vel shared dirs, Zustand store conventions, lib/ rationale, design system Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
There was a problem hiding this comment.
✦ REQUEST_CHANGES
Value: Establishes the architectural ground rules for the web move — domain-driven structure, Zustand, co-located tests, no barrel files. Without this, the move to apps/web/ has no shared conventions and agents will drift. This is a prerequisite for the migration to be maintainable.
What this does: Adds CONVENTIONS.md to apps/web/ documenting architecture decisions from today's FE meeting — code organization, state management (Zustand), component patterns, framework strategy, testing, and dead code rules.
⚠️ Blocking: Design Library path is wrong
The doc says:
packages/design-library/outsideapps/web/
But the decision post-meeting is that the design library stays within apps/ for now and moves to top-level packages/ later. The path should be apps/design-library/ (or similar), with a note that it's destined for top-level packages once things stabilize.
As written, someone following this doc would create packages/design-library/ today, which is premature — the monorepo config for a true top-level package isn't set up yet.
🟡 Non-blocking: React Query wasn't discussed in today's meeting
The data fetching section introduces TanStack React Query as the convention for server state. This wasn't on the meeting agenda and the team didn't align on it. Before it lands in the conventions doc, a quick async thumbs up from Jason, Tirman, and Maddie would be good — they're the ones being asked to follow it.
✅ Bots' STYLE_GUIDE.md finding — correctly self-resolved
Both Codex (P2) and Devin flagged the ./STYLE_GUIDE.md link on line 6. The link is a forward reference to companion PR #30684. Since both PRs are open and will merge together, the broken-link window is zero. No action needed here.
✅ Rest of the doc looks solid
domains/naming, nearest-common-ancestor rule for shared code, top-levelhooks//utils//types//lib/— all correct and matches meeting decisions- Barrel files: NEVER ✅
- Zustand with selector rationale (critical for streaming at ~50ms intervals) ✅
- Co-located tests ✅
- Dead code cleanup — same-PR rule + immediate separate PR if unrelated ✅
callbackRefpattern for stabilizing external callbacks — good addition, addresses a real pain point in the current codebase- Framework-agnostic domain logic (no RR7 imports in reducers/handlers) ✅
Vellum Constitution — Inviting: clear conventions with explicit rationale make the open-source codebase approachable for contributors who weren't in the room.
Reviewed at b78dc98a
|
Thanks for the thorough review. Two responses: Design library path: Fair point. The doc currently says React Query: Correct — this wasn't a finalized meeting decision. The doc presents it as a convention but the PR description already marks it as "(not discussed in meeting — open for future decision)". I'll flag to Ashlee whether to keep it as a stated convention or soften it to a recommendation pending team alignment. |
|
Re: design library path — Ashlee confirmed the team's decision is Re: React Query — Good catch that it wasn't discussed in the meeting. Flagged to Ashlee for decision on whether to keep it as a stated convention or soften to a recommendation. Will update based on her response. |
There was a problem hiding this comment.
✦ APPROVE (superseding REQUEST_CHANGES — prior concern withdrawn)
Correction: I flagged packages/design-library/ as premature. Boss confirmed this is intentional — starting in packages/ from day one gives a clean path to @vellum/design-library npm publishing and avoids a future move. The PR had it right. Apologies for the noise.
Structure looks solid. The full directory tree confirms the decisions from today's meeting:
domains/with 20+ well-scoped bounded contexts, each owning its own store/types/components ✅packages/design-library/at monorepo root with@vellum/design-libraryalias ✅- Top-level
hooks/,utils/,types/,lib/,runtime/for cross-domain shared code ✅ - No barrel files anywhere... except one:
Minor: streaming/handlers/index.ts is explicitly labeled "barrel for handler module." The team just decided "never use barrel files" — if this is intentional for the handlers module's internal API surface, it warrants a brief note in the doc explaining the exception (or a quick async check with Jason/Tirman/Maddie).
Non-blocking — everything else is clean. React Query note from my earlier review still stands as a soft team-alignment item before the conventions doc merges.
Vellum Constitution — Inviting: explicit domain structure and a top-level design library lower the bar for external contributors to understand and extend the codebase.
Reviewed at b78dc98a
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
c6e17e0
There was a problem hiding this comment.
✦ APPROVE
Value: Codifies the architectural decisions from today's FE meeting into a single reference doc — domain structure, Zustand, no barrel files, co-located tests, dead code rules. Agents and contributors now have explicit, reasoned conventions to work from.
Re-review at c6e17e0a (new commit: React Query + HeyAPI data fetching section)
The React Query addition is now well-justified — the key differentiator is the HeyAPI TanStack Query plugin that auto-generates type-safe useQuery/useMutation hooks directly from the OpenAPI spec. No equivalent plugin exists for SWR. That resolves my earlier "wasn't aligned in the meeting" concern.
The Zustand (client state) vs React Query (server state) split is clearly delineated in the doc — no ambiguity about which to reach for when.
Everything else from my prior review holds. Structure is clean, rationale is sound throughout.
Vellum Constitution — Inviting: explicit conventions with reasoning lower the bar for contributors who weren't in the room to write code that fits.
Reviewed at c6e17e0a
Frontend Conventions — Meeting Outline
Companion PR: #30684 — STYLE_GUIDE.md
This document (
CONVENTIONS.md) covers architecture decisions and patterns for the newapps/webVite + React Router v7 SPA. It consolidates decisions made across 30+ PRs in thevellum-assistant-platformrefactoring series into a single reference doc so the team can align on conventions before feature work begins.Each section includes the decision, the reasoning, and references. Sections with checkboxes had multiple viable options — checked items reflect what the team decided in the May 14 meeting.
Updates since last revision
Data fetching section finalized with React Query + HeyAPI codegen decisions:
@tanstack/react-queryplugin that auto-generates type-safe hooks from the OpenAPI spec. No equivalent plugin exists for SWR (still in proposal stage) or other libraries. Added explicit responsibility split: React Query = server state, Zustand = client state.@tanstack/react-queryfor hooks,@hey-api/client-fetchfor HTTP client,@hey-api/typescriptfor type generation. Generated output atsrc/generated/heyapi/.fetch, with escape hatch for SSE/streaming endpoints.Human review checklist
src/generated/heyapi/location: Confirm this is the right output directory for generated code.bun run openapi-tscommand: Referenced in the doc but doesn't exist yet — this is aspirational. Should we note it differently, or is it fine as a target convention?lib/reinstatement: Earlier meeting leaned "nolib/", but platform analysis found configured third-party wrappers (API client, CSRF, Sentry) that don't fitutils/orruntime/. Does the team agreelib/should stay for this specific category?domains/naming: Confirm the rationale matches the meeting consensus.use-{domain}-store.tsconvention. Not yet implemented — confirm the team wants to commit to this naming now.packages/design-library/: Confirm monorepo location and@vellum/design-libraryalias.Prompt / plan
Update
CONVENTIONS.mdbased on decisions from the May 14 FE team meeting (Ashlee, Tirman, Jason, Maddie) plus platform repo analysis and post-meeting research on data fetching strategy. Extract finalized decisions from the meeting transcript, analyze the platform repo's current code to validate the proposed structure, update the document to reflect those decisions, and flag deferred/undiscussed items.Test plan
Doc-only change — no code affected. CI passes (markdown lint only).
Meeting outline (preserved for team context)
1. Architecture: Vite + React Router v7 SPA
Decision: The web app uses Vite + React Router v7 in library / data-router mode (
createBrowserRouter+<RouterProvider>).Why: We're migrating off Next.js. Vite gives us sub-second HMR (vs. Next.js multi-second cold starts on large pages) and a simpler build pipeline — no server runtime, no edge functions, no middleware chain. React Router v7's data-router mode (loaders, actions, nested routes) gives us URL-driven routing without custom navigation state or URL-to-state sync effects. The platform's current Next.js setup uses almost none of the SSR/RSC features that justify Next.js complexity — we're paying the framework tax without using the framework features.
References:
2. Route-driven component boundaries
Decision: Each route only mounts the hooks and state it actually needs. No "god components" that render on every route.
Why:
AssistantPageClient.tsxin the platform repo was a 2,900+ line god component that mounted everything on every route — including routes like the identity route where most hooks were unnecessary. This caused needless re-renders, made the code impossible to reason about, and created a massive prop-drilling chain. Route-level splitting ensures each route only pays for what it uses — and makes lazy loading trivial (each route = oneReact.lazy()boundary).References:
3. Code organization strategy
Decision: Group code by what it does (messages, conversations, streaming, interactions), not by what it is (hooks, utils, components). Top-level folder is
domains/.Why: This is the "folders by feature" vs. "folders by type" debate — one of the oldest in React. Technical-layer folders (
hooks/,utils/,components/) work fine for small projects (~20 files) but become dumping grounds at scale — unrelated code sits side by side, and figuring out "what depends on what" requires jumping between 5 folders. Domain folders make dependencies explicit and map cleanly to lazy-loaded route chunks. Cross-domain shared code lives at the nearest common ancestor, not nested inside one consumer's folder.Team decision — organization approach:
features/messages/,features/streaming/,features/conversations/. Each folder owns its hooks, components, utils, types, and tests. Scales well; used by React Router docs, Bulletproof React, and most 2025-2026 architecture guidesdomains/— domain folders for business logic, plus top-levelhooks/,utils/,types/,lib/,runtime/,components/for cross-domain shared primitives. Design system library lives outsideapps/web/atpackages/design-library/with@vellum/design-libraryalias.domains/chosen overfeatures/because features implies product-level grouping while domains signals bounded contexts with distinct lifecycles.hooks/,components/,utils/,types/. Familiar to the team, but degrades as the app grows beyond ~50 files per folderReferences:
4. Barrel files (
index.ts)Decision: Never use barrel files. Import from the source file directly.
Team decision — barrel file policy:
stream-handlers/index.tsre-exporting 8 handlers = good.components/Button/index.tsre-exporting one file = skip itindex.ts. Clean imports (@/domains/messages) but lots ofindex.tstabs in the editor@/domains/messages/use-message-store.ts). No indirection. Revisit if a genuine need arises.References:
5. Zustand for shared mutable state
Decision: Use Zustand for state shared across multiple components — messages, turn state, interactions, conversation list, viewer state.
Why Zustand over Context + useReducer:
useStore(selector)lets each component subscribe to only the slice it needs. Context has no selector support — every consumer re-renders on any change, which is unacceptable during streaming (messages update every ~50ms).turnReducer,interactionReducer,conversationListReducerall work as Zustand actions with no modification.Team decision — shared state library:
References:
6. Zustand store organization
Decision: How should we organize Zustand stores? One giant store, or many small ones?
Why this matters: The platform's current approach collapses all chat state into a single React context. This was a major source of re-render problems — updating the message list re-rendered the conversation list sidebar. Store boundaries determine re-render boundaries.
Team decision — store granularity: DEFERRED. The team agreed to decide when actually implementing Zustand. Ashlee leans toward one store per domain. When the first store PR is opened, the whole team will review to align on the pattern.
useChatStore,useConversationListStore,useStreamStore, etc. Each store is independently subscribable. Cross-store reads usegetState()in actions. This is what the Zustand maintainers recommend for medium-to-large appsuseAppStorewith slices:createMessageSlice,createConversationSlice, etc. Simpler mental model (one store), middleware applies globally (devtools, persist). But all selectors must be careful to avoid over-subscribing. Zustand docs on slicesReferences:
7. useReducer for related state within a component
Decision: When 2+ pieces of state change together within a single component/hook, use
useReducerwith typed action events. ReserveuseStatefor independent, single-value state.Why: Multiple
setStatecalls that must stay in sync are a bug factory — we found several instances in the platform wheresetIsLoading(false)andsetError(err)could get out of sync during race conditions.useReducermakes transitions atomic and self-documenting (dispatch({ type: "FETCH_ERROR", error })is clearer than twosetStatecalls). Extracting the reducer to its own file makes it testable as a pure function with no React rendering needed.References:
8. Components render UI; hooks perform side effects
Decision: If something renders
nulland only performs side effects, it should be a custom hook, not a component.Why: We found multiple "components" in the platform codebase that rendered
nulland only useduseEffect(e.g.,ActiveAppPinSync). These are hooks masquerading as components — they add nodes to the React tree, appear in DevTools, and participate in reconciliation, all for zero visual output. Hooks are React's mechanism for reusable side-effect logic; components are for rendering UI.References:
9. Thin orchestrator hooks + pure handler functions
Decision: Top-level hooks that wire multiple domains together should be thin orchestrators (compose domain hooks, build context, delegate). Event-handling logic should be extracted into pure functions that take a context object and return results.
Why: The platform's
handleStreamEventwas a singleuseCallbackwith a 32-case switch statement and 15+ closure dependencies. Each case captured stale closures differently, making bugs nearly impossible to reproduce. Extracting each case into a pure handler function ((context, event) => result) makes them individually unit-testable, eliminates closure-over-state bugs, and turns the orchestrator hook into a thin dispatcher.Signs a hook needs decomposition:
useCallbackwith a switch/if-else over many casesuseEffectblocksReferences:
10. Stabilize external callbacks with refs
Decision: When a hook receives callbacks that may not be memoized upstream, store them in refs to keep the consuming
useCallbackidentity stable.Why: Without this, every parent re-render that passes a new function reference causes the child's
useCallbackto get a new identity, defeating memoization and causing cascading re-renders. This is the standard workaround untiluseEffectEventships as stable. The React team acknowledged this pain point anduseEffectEventis the official long-term solution.References:
11. Keep domain logic framework-agnostic
Decision: Reducers, pure handler functions, state machines, and domain types must not import from any framework-specific module (
next/navigation,react-router, etc.). Framework-specific routing calls belong in thin adapter layers.Why: We're migrating from Next.js to React Router v7. Every
next/navigationimport in a domain module is a migration tax. In the platform refactoring, we removednext/navigationfromuseConversationLoader— it haduseRoutercalls mixed into data-fetching logic, making it impossible to reuse outside Next.js. Pure TypeScript domain logic transfers cleanly between frameworks and is easier to test (no mocking router context).References:
12. URL-driven routing as target architecture
Decision: The target architecture uses URL routing directly via React Router v7 nested routes. Each view state maps to a route; the URL is the source of truth. No custom navigation state or URL-to-state sync effects.
Why: The current platform architecture uses in-memory state (
mainView, customnavigationReducer) synced to URLs via effects. This is fragile — state and URL can drift, causing ghost states where the URL says one thing but the UI shows another. We observed this in the platform when navigating between conversations during streaming. URL-driven routing eliminates this entire class of bugs — the URL is the state, not a reflection of it.References:
13. SSR/build-safe rendering
Decision: Route and layout components must not access
window/localStorage/documentduring synchronous render. Client-only reads belong inuseEffector runtime adapters.Why: Keeps the door open for future static prerendering or hybrid runtimes. Even as an SPA, build-time rendering (Vite's SSG mode) and test environments (jsdom) break on unconditional
windowaccess. This is a low-cost constraint that prevents a class of bugs from ever appearing.References:
14. Data fetching: React Query + HeyAPI codegen
Decision: Use TanStack React Query for server-derived data. Generate API clients from the platform's OpenAPI spec using HeyAPI (
@hey-api/openapi-ts) with the@tanstack/react-queryplugin.Why React Query: HeyAPI has a production-ready
@tanstack/react-queryplugin that auto-generates type-safe query/mutation hooks from the OpenAPI spec. No equivalent plugin exists for SWR (still in proposal stage, #1479) or RTK Query. This makes React Query the only viable choice if we want generated hooks — and switching to SWR would mean writing manual wrapper hooks for every endpoint, negating the benefit of codegen. Beyond the plugin, React Query has first-class mutation support, optimistic updates, DevTools, and 12M+ weekly downloads (2026).Why codegen in assistant repo: Per discussion with the HeyAPI maintainer, the platform publishes the OpenAPI spec and the assistant repo runs HeyAPI codegen locally. This gives us full control over the HeyAPI version, React Query version, and regeneration timing — no peer dependency coordination needed.
Team decision — data-fetching library:
useState+useEffect(current platform approach) — no dependency, but requires manually implementing caching, deduplication, retries, and race condition protection in every componentReferences:
15. Testing conventions
Testing strategy has multiple independent decisions. Let's walk through each.
15a. Test runner
The platform currently uses
bun:test. The new Vite app gives us an opportunity to re-evaluate.Team decision — test runner: (not discussed in meeting — open for future decision)
--watchUI, no official React Testing Library adapter15b. Test file location
Where do test files live relative to source files? This is one of the most debated patterns in the React ecosystem.
Team decision — test file organization:
use-send-message.ts+use-send-message.test.tsin the same folder. Most discoverable; when you move a source file, the test moves with it. This is the pattern Vitest and most 2025-2026 guides recommend__tests__/subdirectory —hooks/use-send-message.ts+hooks/__tests__/use-send-message.test.ts. Keeps the source folder less cluttered; still colocated. Jest docs default to thisuse-send-message/index.ts+use-send-message/use-send-message.test.ts. The hook and its test live in a self-contained folder. Cleanimport from "./use-send-message"paths, and you can add fixtures, mocks, and stories alongside. But adds a folder for every hook/component, even simple onestests/mirror —src/domains/messages/...mirrored intests/domains/messages/.... Full separation of test and source code. Harder to keep in sync; tests can drift from source structure. Mostly used in older projects15c. Mock strategy
Decision: Mock at the API client level (
client.get,client.post), notglobalThis.fetch.Why: Mocking
globalThis.fetchis too low-level — it doesn't catch request-building bugs (wrong URL, missing headers, wrong body shape). Mockingclient.get(...)catches those errors while still isolating from the network. We discovered this during the refactoring when afetchmock passed but the actual API client was building the wrong URL.References:
16. Dead code cleanup
Decision: Delete dead code immediately in the same PR. No commented-out code. Audit proactively when fixing convention violations.
Why: The platform codebase accumulated significant dead code — 120+ line functions that were never called, hooks with boolean flags that were declared but never set to
true, entire components imported but rendered behind permanently-false conditions. This happened because extraction PRs didn't clean up the originals. Dead code creates false positives in code search ("I found where this is used... oh wait, that's dead code"), makes the codebase look bigger than it is, and confuses both humans and AI coding agents. The cost of deletion is near-zero with version control — if you need it back, it's in git history.Link to Devin session: https://app.devin.ai/sessions/57905118879948a69946c94c6cd332d7
Requested by: @ashleeradka