Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,7 @@ Each decision's full rationale lives in [`docs/decisions.md`](./docs/decisions.m
the headline here is the rule; follow the link for the "why". Short entries carry
their rationale inline. **Do not relitigate these.**
- **Dashboard is a CLIENT of LoreKit's REST API** — [rationale](./docs/decisions.md#dashboard-is-a-client-of-lorekits-rest-api)
- **Client reads are never server actions** (Next runs actions one at a time and they cannot be aborted) — [rationale](./docs/decisions.md#client-reads-are-never-server-actions)
- **MCP server endpoint is a static production URL** — [rationale](./docs/decisions.md#mcp-server-endpoint-is-a-static-production-url)
- **Lore Explorer filters through ONE two-level command menu + pills** — [rationale](./docs/decisions.md#lore-explorer-filters-through-one-two-level-command-menu)
- **The Explorer's Activity panel has a DISPLAY default (24h), separate from the list's (all time)** — [rationale](./docs/decisions.md#the-explorers-activity-panel-has-a-display-default-separate-from-the-lists)
Expand Down
4 changes: 4 additions & 0 deletions docs/decisions.md
Original file line number Diff line number Diff line change
Expand Up @@ -857,3 +857,7 @@ score stays in `lesson-rank.ts` / `lessons-pure.mjs`. The ranking is set-relativ
normalises against the most-recurring candidate) and it has to agree exactly with the CLI hook's
ordering — an agreement `lesson-rank-parity.spec.ts` can hold two TypeScript-family
implementations to and could not hold a plpgsql third copy to at all.

## Client reads are never server actions

**A React Query read in `packages/web` must not call a server action.** Next's app router runs server actions one at a time per client (the router's action queue starts the next one only when the previous resolves), and an action cannot take an `AbortSignal`. A read served by one queues behind every other action in flight, cannot be cancelled when its query is superseded, and keeps `useIsFetching()` (and so the TopBar activity bar) non-zero until the queue drains. It was measured, not theorised: on `/lore` the memory badge (`getPlanUsage`), the pending-invites query (`listPendingInvitesForMe`) and the Explorer list (`listMemories`) were all actions. Every load and every window focus (`FocusRefetcher`) ran them strictly in that order, each `POST /lore` starting the millisecond the previous one ended. The list arrived last, and the bar stayed on for seconds after the parallel REST panels had loaded. In the worst case seen, a 50 s Supabase `auth/v1/user` call inside the list action, plus a focus refetch queued behind it, kept the bar on for about 85 s. The fix is transport only: the list calls `POST /memories/list` from the browser (`fetchMemoryPage`, body from the pure `lib/memory-list.ts`), and the two reads with no REST endpoint yet go through GET route handlers (`/api/plan/usage`, `/api/invites/pending`) read with `getAppRoute(path, signal)`. Moving those two onto proper edge endpoints is still the backlog `packages/web/CLAUDE.md` names; the route handlers are the step that removes the queue without a schema/handler/OpenAPI change. Server actions remain right for mutations and for server components, where calling the function is a plain call. Guarded by `lib/queries/parallel-reads.spec.ts`, which mocks the action modules to throw.
3 changes: 3 additions & 0 deletions docs/key-files.md
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ else. Grouped loosely by subsystem in migration/PR order.
| `packages/mcp-core/src/auth/tenant-scope-usage.spec.ts` | Source-scan drift guard: asserts every edge `tools.ts` read handler (`toolRead`/`toolList`/`toolSearch`/`toolListArchived`) routes through `applyTenantScope`, not an inlined `.eq('user_id', ...)` |
| `packages/web/src/lib/pagination/` | Pure, audit-decoupled keyset-pagination + filter module: `cursor.ts` (opaque base64url cursor codec, fails closed to `null` on any malformed/forged input — safe because callers still apply their own `user_id` scoping), `keyset.ts` (`clampPageSize`, the `(created_at desc, id desc)` `.or()` predicate, fetch+1 → page assembly), `filters.ts` (`normalizeActions` against a caller-supplied allow-set, LIKE/PostgREST-escaping `substringNeedle`, inclusive-`from`/half-open-`to` `dateRangeBounds`), `apply.ts` (thin supabase-js query-builder shell). Each pure file has a co-located `.spec.ts` (mirrors `aggregations.spec.ts`/`repo-format.spec.ts`); composed by `listAuditLog` in `audit-log.ts`, the only current consumer |
| `packages/web/src/lib/queries/audit-log.ts` | `useAuditLog` — `useInfiniteQuery` over the `listAuditLog` server action, keyed by the URL-backed filter state (action set / name / date range); drives the feed's "Load more" |
| `packages/web/src/lib/memory-list.ts` | Pure Lore Explorer list request: `MemoryFilters` → the `POST /memories/list` body (`memoryListBody`: default `created_at` sort, page-size clamp, legacy `tags` folded into a `label` filter, retention thresholds) and the response → `MemoryPage` (`memoryPageFromResponse`). Moved out of the former `listMemories` server action when `useMemories` became a browser request; `memory-list.spec.ts` pins the body |
| `packages/web/src/lib/app-route.ts` | `getAppRoute(path, signal)` — the browser GET for the dashboard's own `app/api/**` route handlers (same-origin cookie auth, `no-store`, throws `AppRouteError` on non-2xx). Used for reads that still need server code but must not be server actions (see `packages/web/CLAUDE.md` → "A client read is never a server action") |
| `packages/web/src/app/api/plan/usage/route.ts`, `packages/web/src/app/api/invites/pending/route.ts` | GET route handlers wrapping `getPlanUsage` / `listPendingInvitesForMe` for `useMemoryTotal` and `usePendingInvitesForMe`. Same contract as the functions (`null` / `[]` when signed out or on failure), `force-dynamic`, `no-store`. `lib/queries/parallel-reads.spec.ts` asserts the hooks read them with React Query's signal and never call the server actions |
| `supabase/migrations/00012_audit_log_search.sql` | Enables `pg_trgm` + a GIN trigram index on `audit_log.target` (index-backed `ilike` name search) and a `(user_id, created_at desc, id)` index covering the keyset seek (00010's `(user_id, created_at desc)` index lacks the `id` tiebreaker) |
| `supabase/migrations/00017_org_roles_and_author.sql` | Adds `viewer` to the `org_members.role` CHECK; adds `memories.created_by`/`updated_by`; creates `lorekit_org_role` + `lorekit_org_can(user, org, capability)` — the single `SECURITY DEFINER` capability source for org writes/deletes (org-sharing Phase 2; see plan `.agent/claude/org-sharing-phase-2-writes/plan.md`) |
| `supabase/migrations/00018_org_limits.sql` | `org_limits` table (mirrors `user_limits`) + `lorekit_get_org_limit`; re-creates `enforce_memory_cap()` with an org branch that runs BEFORE the `user_id IS NULL` service exemption, so an org-owned row (always `user_id NULL`) is never cap-exempt |
Expand Down
9 changes: 5 additions & 4 deletions docs/storybook.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,11 @@ Shipped examples:

Client pages (`'use client'`) render as true full-page stories under MSW. Server-component
pages cannot — story their largest client subtree, don't refactor an RSC page to client just
to story it. One honest edge: the `/lore` lesson list reads the `listMemories` **server
action**, which is gated on `supabase.auth.getUser()`; with no session in the mocked browser
context it returns an empty page, so the results panel shows its empty state while the rest of
the page populates. Every part driven by a *browser* fetch is populated.
to story it. Every part driven by a *browser* fetch is populated, which on `/lore` now includes
the lesson list (`useMemories` reads `POST /memories/list` from the browser). A read that still
goes through a **server action** cannot be served this way: the action runs with no session in
the mocked context and returns its signed-out shape, so its panel shows an empty state. That is
one more reason reads do not belong in server actions (see `packages/web/CLAUDE.md`).

### Determinism

Expand Down
30 changes: 30 additions & 0 deletions packages/web/CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,36 @@ or `.rpc(…)` for anything other than **auth**, stop and add an endpoint instea
are the migration backlog, not a licence to add a new direct `.from(…)`/`.rpc(…)`
read for product data — new surfaces get an endpoint.

## A client read is never a server action (REQUIRED)

React Query `queryFn`s must not call a server action (`'use server'` module).
Next's app router runs server actions **one at a time per client**, and an
action cannot take an `AbortSignal`. Every read served that way queues behind
whatever action is already in flight, and React Query can never cancel it.
`useIsFetching()`, and with it the TopBar `ActivityIndicator`, stays non-zero
until the whole queue drains.

- **This happened.** On `/lore` the memory badge, the pending-invites query and
the Explorer's list were all server actions, fired on every load and every
window focus (`FocusRefetcher`). They ran strictly in sequence, with the list
last, so the activity bar stayed lit for seconds after every visible panel had
loaded. In the worst case seen, a slow Supabase auth call kept it on for about
85 s.
- **What to use instead.** If a REST endpoint exists, call it from the browser
through `src/lib/api/*` with the query's `signal` (`fetchMemoryPage`,
`fetchScopes`). If the read still needs server-side code (a surface on the
migration backlog), wrap it in a GET route handler under `app/api/**` and read
it with `getAppRoute(path, signal)` (`lib/app-route.ts`), as
`/api/plan/usage` and `/api/invites/pending` do.
- **Server actions stay for mutations**, which are one user gesture each, and
for server components, where calling the function is a plain call with no
queue.
- **Known remaining offenders** (not on `/lore`'s refresh path):
Comment thread
dash0-dev[bot] marked this conversation as resolved.
`useAuditLog` → `listAuditLog`; `LessonDetailSheet`'s member lookup →
`listMemberIdentities`; `OrganizationManager`'s org-data effect →
`listScopeBindings`, `listMembers`, `listMemberIdentities`, `listInvites`,
`listAvailableScopes`; and `InviteDetailsDialog` → `getInviteOrgDetails`.

## Conventions specific to this package

- **Theme:** dark-only, driven by CSS custom properties in
Expand Down
23 changes: 9 additions & 14 deletions packages/web/src/app/(dashboard)/lore/LorePage.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,15 @@ import {
* mirroring `/insights`) that a server component cannot render in the browser —
* so this story mounts its extracted `'use client'` body, `LorePageClient`,
* directly, the same convention `/insights` uses for `InsightsPage`.
* `LorePageClient` drives its scope tree and contribution heatmap from
* `useScopeTree` / `useLoreData` — TanStack Query hooks that call the Supabase
* *browser* client (PostgREST over HTTP), which MSW mocks. So the component
* renders end-to-end here, not a hand-assembled subtree.
* `LorePageClient` drives its scope tree, contribution heatmap and lesson list
* from TanStack Query hooks (`useScopeTree`, `useLoreData`, `useMemories`) that
* call the `memories` REST function from the browser, which MSW mocks. So the
* component renders end-to-end here, not a hand-assembled subtree.
*
* The lesson list inside `LoreExplorer` reads the `listMemories` **server
* action**. With `next/headers` auto-mocked by `@storybook/nextjs-vite` it does
* execute in the browser, but it is gated on `supabase.auth.getUser()`, which
* short-circuits to no session in the mocked context and returns an empty page —
* so the results panel renders its "No memories in this scope" empty state while
* the scope tree + heatmap populate from the MSW-mocked PostgREST reads. That is
* the honest edge of the mixed rendering model: the component renders end-to-end,
* and every part that is driven by a *browser* fetch is populated.
* The lesson list used to read the `listMemories` server action, which had no
* session in the mocked context and returned an empty page, so this story
* showed "No memories in this scope". The list is a browser request now
* (`POST /memories/list`), so it populates from the same fixtures as the tree.
*
* Determinism: the clock is frozen to {@link FROZEN_NOW} (heatmap + freshness are
* time-relative) and the query client retries off / never refetches.
Expand Down Expand Up @@ -63,8 +59,7 @@ type Story = StoryObj<typeof LorePageClient>;

/**
* Populated Lore Explorer: the scope tree, contribution heatmap, view-mode tabs,
* and filter bars all resolve from the MSW-mocked dataset. The lesson results
* panel shows its empty state (see the auth-gate note above).
* filter bars and the lesson list all resolve from the MSW-mocked dataset.
*/
export const Default: Story = {};

Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
25 changes: 25 additions & 0 deletions packages/web/src/app/api/invites/pending/route.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { NextResponse } from 'next/server';
import { listPendingInvitesForMe } from '@/lib/org-invites';

/**
* GET /api/invites/pending
*
* Pending org invites addressed to the caller — `listPendingInvitesForMe` over
* HTTP, for the shared `['pending-invites']` query behind the top bar's invites
* badge, the `/lore` banner and the Organization nav badge. Those used to call
* the server action directly. Next's router runs server actions one at a time
* per client, so on `/lore` this read sat in a queue with the plan-usage read
* and the Explorer's list on every load and every window focus. As a route
* handler it is an ordinary request that runs in parallel and the browser can
* abort.
*
* Same contract as the action: `[]` when there is no session or the query
* fails. Never cached: accepting or declining elsewhere must show up on the
* next refetch.
*/
export const dynamic = 'force-dynamic';

export async function GET() {
const invites = await listPendingInvitesForMe();
return NextResponse.json(invites, { headers: { 'Cache-Control': 'no-store' } });
}
23 changes: 23 additions & 0 deletions packages/web/src/app/api/plan/usage/route.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { NextResponse } from 'next/server';
import { getPlanUsage } from '@/lib/plan';

/**
* GET /api/plan/usage
*
* The caller's plan and active memory count — `getPlanUsage` over HTTP, for the
* top bar's memory badge (`useMemoryTotal`). The badge used to call the server
* action directly. Next's router runs server actions one at a time per client,
* so on `/lore` it led a queue that the pending-invites read and the Explorer's
* list waited behind on every load and every window focus. As a route handler
* it is an ordinary request that runs in parallel and the browser can abort.
*
* Same contract as the action: `null` when there is no session or the RPC
* fails, and the badge degrades to 0. Never cached, because the count moves
* whenever an agent writes.
*/
export const dynamic = 'force-dynamic';

export async function GET() {
const usage = await getPlanUsage();
return NextResponse.json(usage, { headers: { 'Cache-Control': 'no-store' } });
}
72 changes: 72 additions & 0 deletions packages/web/src/app/api/read-routes.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/**
* `GET /api/plan/usage` and `GET /api/invites/pending` are transports, not new
* logic: each wraps an existing server function so the browser can read it as
* an ordinary, parallel, abortable request instead of a queued server action.
* Pinned here: they answer exactly what the function returns (including its
* signed-out / failure shape) and are never cached.
*/

import { beforeEach, describe, expect, it, vi } from 'vitest';

const getPlanUsage = vi.fn();
const listPendingInvitesForMe = vi.fn();

vi.mock('@/lib/plan', () => ({ getPlanUsage: () => getPlanUsage() }));
vi.mock('@/lib/org-invites', () => ({
listPendingInvitesForMe: () => listPendingInvitesForMe(),
}));

const planUsageRoute = await import('./plan/usage/route');
const pendingInvitesRoute = await import('./invites/pending/route');

beforeEach(() => {
vi.clearAllMocks();
});

describe('GET /api/plan/usage', () => {
it('answers the caller plan usage as JSON, uncached', async () => {
getPlanUsage.mockResolvedValue({ count: 3318, limit: 5000, plan: 'free' });

const response = await planUsageRoute.GET();

expect(response.status).toBe(200);
expect(response.headers.get('cache-control')).toBe('no-store');
await expect(response.json()).resolves.toEqual({ count: 3318, limit: 5000, plan: 'free' });
});

it('answers null when there is no session or the RPC failed, as the action did', async () => {
getPlanUsage.mockResolvedValue(null);

const response = await planUsageRoute.GET();

expect(response.status).toBe(200);
await expect(response.json()).resolves.toBeNull();
});

it('is rendered per request, never statically', () => {
expect(planUsageRoute.dynamic).toBe('force-dynamic');
});
});

describe('GET /api/invites/pending', () => {
it('answers the invites addressed to the caller as JSON, uncached', async () => {
const invites = [{ id: 'inv-1', org_id: 'org-1', status: 'pending', org: { name: 'Acme', slug: 'acme' } }];
listPendingInvitesForMe.mockResolvedValue(invites);

const response = await pendingInvitesRoute.GET();

expect(response.status).toBe(200);
expect(response.headers.get('cache-control')).toBe('no-store');
await expect(response.json()).resolves.toEqual(invites);
});

it('answers [] when there is no session or the query failed, as the action did', async () => {
listPendingInvitesForMe.mockResolvedValue([]);

await expect((await pendingInvitesRoute.GET()).json()).resolves.toEqual([]);
});

it('is rendered per request, never statically', () => {
expect(pendingInvitesRoute.dynamic).toBe('force-dynamic');
});
});
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Loading