Skip to content

feat: add mock dataset API layer#22

Merged
GunelShukurova merged 6 commits into
mainfrom
feat/mock-api-dataset
Jun 18, 2026
Merged

feat: add mock dataset API layer#22
GunelShukurova merged 6 commits into
mainfrom
feat/mock-api-dataset

Conversation

@GunelShukurova

@GunelShukurova GunelShukurova commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

What does this change?

Introduces a mock API layer for dataset to support frontend development without a backend service.

Adds a Vite development middleware that serves /api/dataset during local development.

Implements an API abstraction (fetchDataset) that supports two modes:

  • Default mode: uses in-memory deterministic dataset (getDataset)
  • HTTP mode (opt-in via VITE_USE_MOCK_API=true): fetches from /api/dataset

Ensures dataset shape remains fully compatible with @cleat/contracts.

Keeps existing dataset generation logic intact and reusable for both local and mock server modes.

Related issue

Closes #15

Area

  • Frontend (apps/web)
  • Backend
  • Docs
  • Other

Screenshots

Not applicable (no UI changes)

Checklist

  • I ran bun run typecheck && bun run build in apps/web (for frontend changes)
  • I bumped the version if this is a meaningful change
  • Commits are small and focused, with no AI attribution lines
  • I updated docs or AGENTS.md if behavior or structure changed

@GunelShukurova GunelShukurova marked this pull request as draft June 11, 2026 09:41
@GunelShukurova GunelShukurova added the area: frontend Frontend web app label Jun 11, 2026
@GunelShukurova GunelShukurova self-assigned this Jun 11, 2026
@GunelShukurova GunelShukurova marked this pull request as ready for review June 11, 2026 15:55

@martian56 martian56 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I pulled this branch and ran it. With a connected account in the store, every /app page renders a permanently blank screen. Not a spinner, not an error message, an empty body, and the console is silent. It behaves identically with VITE_USE_MOCK_API=true and with it unset, I tested both. Typecheck and build pass, which is exactly why the verification bar for frontend changes here is opening the browser, not the compiler.

What is happening, because the mechanism is worth understanding:

  1. Throwing a promise from render is the Suspense protocol: React catches it, abandons the render, and retries when that promise resolves. Yours is new Promise(() => {}), which by construction never resolves.
  2. A component that suspends never commits, and effects only run after commit. So the useEffect above never runs, fetchDataset is never called, setData never fires, and there is no second render. The hang is total and mode-independent.
  3. Sidebar also calls useDataset, and it sits outside the page-level Suspense boundary in App.tsx. When it suspends there is no boundary to catch it, so React renders nothing at all. That is why there is not even a loading spinner.

On scope: #15 asks for a mock API server, something actually serving the dataset over the agreed routes, with the frontend able to target it via an env flag. This PR has the flag but no server. The non-mock path fetches /api/dataset, which nothing anywhere serves. So against the acceptance list: routes not served, flag exists but undocumented, shapes fine.

How I would shape round two:

  1. The default path keeps the app working with zero config: useDataset stays synchronous and in-process, exactly as on main. The env flag opts in to the HTTP path, not out of it.
  2. The server piece is the heart of the issue: a small Vite dev middleware (configureServer in vite.config.ts) or a standalone Bun script that serves GET /api/dataset from getDataset. Add a .env.example documenting the flag.
  3. For the HTTP path, no fake Suspense. Fetch at the shell level with a visible loading state, or cache the in-flight promise per account and throw that, which is how Suspense data fetching actually works. This part is worth a design chat before you write it, come grab me.

One process note: the checklist says docs were updated and the version bumped, but neither is in the diff. The checklist is how a reviewer calibrates trust. An unchecked box is fine, a wrongly checked one is not.

The instinct to put an API seam between components and the data layer is right, and lib/api is the right home for it. The execution needs another pass.

Comment thread apps/web/src/hooks/useDataset.ts Outdated
}, [accountId]);

if (!data) {
throw new Promise(() => {});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This promise never resolves, and the component suspends on first render, so the effect above never gets a chance to run. Effects run after commit; a suspended render never commits. Nothing can ever un-suspend this. Every consumer of the hook hangs forever, and since Sidebar consumes it outside any Suspense boundary, the entire app shell renders blank. Verified in the browser on this branch: empty body after 10 seconds, with the mock flag on and off.

Comment thread apps/web/src/hooks/useDataset.ts Outdated
useEffect(() => {
if (!accountId) return;

fetchDataset(accountId).then(setData);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two more things to handle in the rework: there is no error path (a rejected fetch leaves the page blank forever with an unhandled rejection), and no stale-response guard (switch accounts quickly and the slow response for the old account can land after, and overwrite, the fast one for the new). The usual shape is an effect-scoped cancelled flag or an AbortController.

Comment thread apps/web/src/lib/api/dataset.ts Outdated
*/
export async function fetchDataset(accountId: string): Promise<Dataset> {
// MOCK MODE
if (import.meta.env.VITE_USE_MOCK_API === "true") {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Flag direction: unset should mean the app works, which today means the in-process dataset. Make the HTTP path the opt-in, not the default. And document the flag in a .env.example, otherwise nobody can discover it.

Comment thread apps/web/src/lib/api/dataset.ts Outdated
}

// REAL BACKEND
const res = await fetch(`/api/dataset?accountId=${accountId}`);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nothing serves this route, in dev or anywhere else, so this fetch can only fail today. The server side of this endpoint is the actual deliverable of #15: a Vite configureServer middleware or a small Bun script can serve it from getDataset in a few dozen lines. Also encodeURIComponent(accountId), and include res.status in the error message so failures are diagnosable.

@GunelShukurova GunelShukurova marked this pull request as draft June 13, 2026 15:01
@GunelShukurova GunelShukurova marked this pull request as ready for review June 13, 2026 15:42

@martian56 martian56 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Big step. I pulled the branch, ran typecheck and build (both clean), and drove it in a real browser. The blank screen is gone: /app/overview renders fully, with the sidebar, the real counts, and the account header, no stuck spinner and no console errors. I also hit the endpoint directly: GET /api/dataset?accountId=acct_personal returns 200 application/json, ~82KB, with every expected key. So the two blockers from last round, the never-resolving Suspense hang and the missing server, are both genuinely fixed and verified.

What you did well, beyond the bug fix:

  • useDataset is now an honest useState plus useEffect with a cancelled guard, so the stale-response race I flagged is handled.
  • The Vite mockApiPlugin serving /api/dataset from getDataset is exactly the mock-server piece #15 was asking for.
  • .env.example documents the flag, the fetch path uses encodeURIComponent, and the error includes status and statusText.
  • Pulling the shared types into a @cleat/contracts workspace package is a genuinely good move. It is more than the issue asked for, but it is the right direction: when the backend lands, the frontend and api can share one source of truth for the shapes.

This needs one more focused pass before it merges. None of these are as deep as last round.

  1. No error path in useDataset, inline. A rejected fetch leaves a permanent spinner and an unhandled promise rejection. This matters more than it looks because of point 3.
  2. Over-fetching, inline. One overview load fired 11 requests to /api/dataset. React StrictMode doubles effects in dev so part of that is expected, but the root cause is that every useDataset consumer fetches independently with no sharing. On main this was a memoized synchronous call, free. Against a real backend this is many round-trips per navigation.
  3. The flag direction is inverted from what we discussed, and it interacts badly with point 1. Details inline.
  4. apps/web is still 1.7.0. AGENTS.md asks for a version bump as its own commit on every meaningful change, and this is a meaningful change. Same note as last round about the checklist.

nit: react-loader-spinner is a whole dependency for one spinner, and that package predates React 19. It works here, but we already have a design system and spinner styling, so an in-house spinner avoids the dep. Your call.

The hard part is behind you. Grab me if you want to talk through the caching shape for point 2.

Comment thread apps/web/src/hooks/useDataset.ts Outdated

setData(null);

fetchDataset(accountId).then((d) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No .catch here. If fetchDataset rejects (a 5xx, a network blip, or the dev-only middleware simply not being there outside the dev server), data stays null forever, the page shows the spinner indefinitely, and you get an unhandled rejection in the console. Add a rejection branch that sets an error state the page can render, something like .then(setData-if-not-cancelled).catch(err-if-not-cancelled). Pair it with a visible error message so a failure is diagnosable instead of an eternal spinner.

Comment thread apps/web/src/lib/api/dataset.ts Outdated
return getDataset(accountId);
}

const res = await fetch(`/api/dataset?accountId=${encodeURIComponent(accountId)}`);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Flag direction: with the var unset, the app takes this HTTP path. That only works because mockApiPlugin is registered with apply: 'serve', so the endpoint exists in the dev server and nowhere else. Run bun run build && bun run preview with the var unset and /api/dataset 404s, which (with no error path in useDataset) is a silent permanent spinner. Two clean options: make the in-process branch the default so unset always works, or keep this but document loudly that the HTTP path is dev-only and ensure the error path covers the 404. The in-process default is simpler.

@GunelShukurova GunelShukurova requested a review from martian56 June 18, 2026 11:06

@martian56 martian56 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Round three, and most of it is genuinely fixed. I reinstalled, ran typecheck and build (both clean), and drove it in a real browser again. What I confirmed:

  • Default mode works end to end. With no env set, the app uses the in-process mock, /app/overview renders the full shell and real data, no console errors. This is the path that actually ships today, and it is solid.
  • The over-fetch is gone. That same overview load now fires 1 request instead of the 11 from last round. The requestCache per accountId does its job, and you kept the cleanup so a failed request does not poison the cache.
  • Flag direction is fixed. Unset now means mock, so the app works everywhere by default and the HTTP path is the opt-in. That was the right call.
  • Version bumped to 1.7.1, and react-loader-spinner is gone, replaced with an in-house spinner. Both nits from last round, both handled.
  • All 13 consumers were migrated cleanly to the new { data, error, loading } shape, which is why typecheck stays green.

So the two original blockers are dead and the cleanups are done. Thank you, this was a real round of work.

One thing still does not do what it claims, and it is the error path, which is the headline of your commit. It does not surface, it blanks the whole app. Here is the exact repro:

  1. Run in HTTP mode: VITE_USE_MOCK_API=false bun run dev.
  2. Force the dataset request to fail (I used Playwright to fulfill /api/dataset with a 500; a real backend 500 or a connection refused hits the identical code path).
  3. Result: #root is completely empty. Not the 'Failed to load overview data.' state you added, not the sidebar, not a spinner. A blank white page. And there is no console error and no unhandled rejection, which makes it the worst kind to debug later.

So the catch runs, but the error UI never appears, because whatever renders above the page is what goes blank, and the page-level error branch never gets to mount. The net effect is the same bad outcome as the original hang: a user staring at nothing. Details and where I would start are inline.

Why this still matters even though default mode is fine: the moment the real backend lands and returns its first 5xx, this is what everyone sees, and the silent blank with no console error is exactly the kind of thing that eats an afternoon. It is much cheaper to fix now, while you are in here, than to rediscover it in three weeks. This is the last thing.

setData(d);
}
})
.catch((err) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This catch is correct in isolation: it sets error and clears data, no unhandled rejection. The problem is downstream. When the fetch fails, ds stays null and error is set, and the whole app renders blank rather than the page error state. I could not pin the exact culprit from the outside, but #root goes fully empty (no AppShell, no sidebar), which means something above the page renders nothing when the dataset never loads, so the page's own if (error) branch never mounts. Start by reproducing with the steps in the summary, then walk down from AppShell: render a hardcoded

error
at the top of OverviewPage's error branch and see if it shows; if it does not, the blank is happening above the Outlet, not in the page.

);
}

if (error) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This error branch is exactly right and is what I want users to see on failure. In my repro it never renders, the page is blank before it gets here. Once you find why the tree blanks on a failed fetch, verify the fix by confirming this 'Failed to load overview data.' message actually appears on a 500, on at least this page and one more. A quick win regardless: give it a retry affordance (a button that re-triggers the fetch) so a transient backend blip is recoverable without a full reload.

@GunelShukurova GunelShukurova requested a review from martian56 June 18, 2026 13:14

@martian56 martian56 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving this. Let me be straight about what is solid and what is still off, because the headline of your last commit, the error handling, still does not work, and I want that on the record even though I am not blocking on it.

What I verified by running the branch (typecheck and build both clean):

  • Default mode is the path that ships, and it is solid. With no env set, the app uses the in-process mock, /app/overview renders the full shell and real data, no console errors. The over-fetch is fixed for real: that load now fires 1 request, down from 11.
  • The requestCache dedupes per accountId and self-heals on failure. Good.
  • Flag direction is right (unset means mock, HTTP is opt-in), version is bumped to 1.7.1, react-loader-spinner is gone, and all 13 consumers moved cleanly to { data, error, loading, retry }. typecheck stays green, which is the proof the migration was complete.

So the actual deliverable of #15, the mock API seam the frontend can later point at a live backend, is here and works. That is why I am approving.

Now the part that still does not work, and I went deep on it this round so you do not have to rediscover what I did. When the dataset request fails, the whole app goes blank. I confirmed it is a real bug, not a test quirk:

  • Repro: VITE_USE_MOCK_API=false bun run dev, then make /api/dataset fail. I tested both a 500 and a connection abort (the abort is exactly what a real backend being down looks like). Both produce a fully blank page, #root empty from the first frame.
  • It is not the interception: routing an unrelated path, or letting /api/dataset through, both render fine. Only the failure blanks it.
  • The Retry button you added never appears, because the page never mounts. The blank is happening above the page, not inside it.

Then I tried to localize it and hit something genuinely odd, which is the useful part for you: I wrapped in BOTH a top-level error boundary and a Suspense boundary with visible fallbacks. On failure, neither fallback renders, #root stays empty, and there is no console error, no React error, nothing. Even a trace at the top of App() does not fire. So this is not a 'missing error boundary' fix and it is not a thrown error. Something in the render path never commits when the dataset is unavailable, most likely a render that keeps invalidating itself rather than the catch logic. That is where to dig: what re-runs on the failure path that does not on the success path.

Why I am approving despite that: it is purely latent today. The default is mock, which never fails, and there is no live backend yet, so nothing can trigger this in normal use. It is also clearly subtle enough to deserve its own focused investigation rather than a fifth round on this PR. But it absolutely has to be fixed before the real API is wired, or the first 5xx anyone hits is a silent white screen with nothing in the console. Please open a follow-up issue and link it here so it does not evaporate. Keep the page-level Retry, it is harmless and correct once the deeper problem is solved.

One small thing: this commit deleted .env.example. Minor, since mock is the default now, but a one-line note in the README about the VITE_USE_MOCK_API=false opt-out would save the next person a grep.

Good iteration across these rounds. Merge it, file the follow-up.

setData(d);
}
})
.catch((err) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For the follow-up, not this PR: this catch is correct in isolation, it sets error and produces no unhandled rejection. The blank is not here. When the fetch fails the entire app fails to render (#root empty, App() never even runs), which a top-level error boundary and a Suspense fallback both fail to catch in my testing. So the lead is not the error handling, it is something on the failure render path that never commits. Suggested starting point: reproduce per the summary, then bisect by temporarily making fetchDataset reject immediately in mock mode so you can iterate without the HTTP setup, and watch which component stops rendering.

@GunelShukurova GunelShukurova merged commit 1bc61d4 into main Jun 18, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: frontend Frontend web app

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mock API server for the live shapes

2 participants