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
11 changes: 10 additions & 1 deletion .claude/CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,15 @@ pnpm logs # View production worker logs
- Each file should handle one coherent responsibility
- Group related components in subdirectories with barrel exports

### Static Assets and Edge Configuration

Two files in `packages/web/public/` are consumed by Cloudflare Workers Static Assets at deploy time. They follow the Cloudflare Pages convention and are easy to miss because they are not TypeScript:

- **`packages/web/public/_headers`** - Cache-Control rules and security headers (CSP, X-Frame-Options, Referrer-Policy, Permissions-Policy) applied to static responses. Update this when adding a new external domain (CSP `connect-src` / `script-src` / `img-src` / `font-src` / `style-src`), introducing a new asset type with custom cache requirements, or relaxing/tightening any security header. Test changes locally with `pnpm --filter web build && pnpm --filter web preview` before deploying.
- **`packages/web/public/_redirects`** - 301 redirects for trailing-slash canonicalization on public routes. **When you add a new public marketing, legal, auth, or resources route, add a `/route/ /route 301` entry here.** Cloudflare Workers Static Assets emits a 307 (temporary) for trailing-slash variants by default, which causes search engines to repeatedly probe the slashed form and report it under "Page with redirect" in Google Search Console. The 301 rules in `_redirects` override that behavior so the slashed variant is permanently canonicalized.

Both files are copied verbatim from `public/` to the build output by Vite. The build log will show `Parsed N valid redirect rules` and `Parsed N valid header rules` to confirm they were picked up.

### Libraries (MUST USE)

- **Zod**: Schema and input validation (backend)
Expand Down Expand Up @@ -116,7 +125,7 @@ retries += 1;

**When Not to Comment:**

- Don't narrate what the code is doing the code already says that
- Don't narrate what the code is doing - the code already says that
- Don't duplicate function or variable names in plain English
- Don't leave stale comments that contradict the code
- Don't reference removed or obsolete code paths (e.g. "No longer uses X format")
Expand Down
19 changes: 3 additions & 16 deletions knip.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,7 @@ const config: KnipConfig = {
],
},
'packages/workers': {
entry: [
'src/index.ts',
'src/db/migrations/**/*.ts',
'vitest.config.ts',
],
entry: ['src/index.ts', 'src/db/migrations/**/*.ts', 'vitest.config.ts'],
project: ['src/**/*.ts'],
ignoreDependencies: ['wrangler', 'cloudflare'],
},
Expand All @@ -53,17 +49,8 @@ const config: KnipConfig = {
ignore: ['**/*'],
},
},
ignore: [
'**/routeTree.gen.ts',
'**/*.config.{ts,js}',
'.claude/**',
],
ignoreDependencies: [
'agent-browser',
'dotenv',
'turbo',
'wrangler',
],
ignore: ['**/routeTree.gen.ts', '**/*.config.{ts,js}', '.claude/**'],
ignoreDependencies: ['agent-browser', 'dotenv', 'turbo', 'wrangler'],
ignoreBinaries: ['hono', 'agent-browser'],
};

Expand Down
96 changes: 51 additions & 45 deletions packages/docs/audits/reconciliation-rerender-audit.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ Every Yjs change to the study -- even an unrelated field -- triggers this entire
### 1. Unconditional `setLocalFinal` on every `finalAnswers` reference change

**Files:**

- `packages/web/src/components/project/reconcile-tab/amstar2-reconcile/ReconciliationQuestionPage.tsx:89-104`
- `packages/web/src/components/project/reconcile-tab/amstar2-reconcile/MultiPartQuestionPage.tsx:80-103`

Expand All @@ -46,10 +47,10 @@ The "Initialize from props" effect runs whenever the `finalAnswers` prop changes
```js
// ReconciliationQuestionPage.tsx:89
useEffect(() => {
if (finalAnswers) {
setLocalFinal(JSON.parse(JSON.stringify(finalAnswers))); // always sets state
// ... selectedSource logic
}
if (finalAnswers) {
setLocalFinal(JSON.parse(JSON.stringify(finalAnswers))); // always sets state
// ... selectedSource logic
}
}, [finalAnswers, reviewer1Answers, reviewer2Answers]);
```

Expand All @@ -59,12 +60,12 @@ useEffect(() => {

```js
useEffect(() => {
if (finalAnswers) {
if (!answersEqual(localFinal, finalAnswers)) {
setLocalFinal(JSON.parse(JSON.stringify(finalAnswers)));
}
// ... selectedSource logic (also guard with comparison)
if (finalAnswers) {
if (!answersEqual(localFinal, finalAnswers)) {
setLocalFinal(JSON.parse(JSON.stringify(finalAnswers)));
}
// ... selectedSource logic (also guard with comparison)
}
}, [finalAnswers, reviewer1Answers, reviewer2Answers]);
```

Expand All @@ -81,13 +82,13 @@ The `answersEqual` function already exists in the same file. For `MultiPartQuest
The clamp logic is a `useEffect`, which runs after the render commits. When `navItems` shrinks (ROB2 aim change synced from the other user), there is one render frame where `currentPage` exceeds `navItems.length - 1`:

```js
const currentItem = navItems[currentPage] ?? null; // null for one frame
const currentItem = navItems[currentPage] ?? null; // null for one frame

useEffect(() => {
const clamped = Math.max(0, Math.min(currentPage, totalPages - 1));
if (clamped !== currentPage) {
setCurrentPage(clamped); // fires after render
}
const clamped = Math.max(0, Math.min(currentPage, totalPages - 1));
if (clamped !== currentPage) {
setCurrentPage(clamped); // fires after render
}
}, [totalPages, currentPage]);
```

Expand All @@ -98,15 +99,13 @@ This is not infinite (the auto-fill write does not change `navItems` length), bu
**Fix:** Derive the effective page synchronously so `currentItem` is never null due to a stale `currentPage`:

```js
const effectivePage = totalPages > 0
? Math.max(0, Math.min(currentPage, totalPages - 1))
: currentPage;
const effectivePage = totalPages > 0 ? Math.max(0, Math.min(currentPage, totalPages - 1)) : currentPage;

// Persist the clamped value to state (for localStorage, awareness, etc.)
useEffect(() => {
if (totalPages > 0 && effectivePage !== currentPage) {
setCurrentPage(effectivePage);
}
if (totalPages > 0 && effectivePage !== currentPage) {
setCurrentPage(effectivePage);
}
}, [totalPages, effectivePage, currentPage]);

const currentItem = navItems[effectivePage] ?? null;
Expand All @@ -126,40 +125,40 @@ A `setInterval` calls `setRefreshTick(Date.now())` every 1000ms, which forces `u

```js
useEffect(() => {
const intervalId = setInterval(() => {
setRefreshTick(Date.now());
}, 1000);
return () => clearInterval(intervalId);
const intervalId = setInterval(() => {
setRefreshTick(Date.now());
}, 1000);
return () => clearInterval(intervalId);
}, []);

const usersWithCursors = useMemo(() => {
void refreshTick; // force re-eval
return remoteUsers.filter(user => user.cursor != null && user.currentPage === getCurrentPage);
void refreshTick; // force re-eval
return remoteUsers.filter(user => user.cursor != null && user.currentPage === getCurrentPage);
}, [remoteUsers, getCurrentPage, refreshTick]);
```

**Fix:** Move stale cursor detection into `buildRemoteUsers` (which already runs on every awareness change) and remove the timer entirely:

```js
function buildRemoteUsers(aw, currentUserRef, checklistTypeRef) {
const STALE_CURSOR_MS = 5000;
const now = Date.now();
// ... existing logic, then:
const cursor = state.cursor;
const isStaleCursor = cursor && (now - cursor.timestamp) > STALE_CURSOR_MS;

states.push({
// ...
cursor: isStaleCursor ? null : cursor,
});
const STALE_CURSOR_MS = 5000;
const now = Date.now();
// ... existing logic, then:
const cursor = state.cursor;
const isStaleCursor = cursor && now - cursor.timestamp > STALE_CURSOR_MS;

states.push({
// ...
cursor: isStaleCursor ? null : cursor,
});
}
```

Then `usersWithCursors` can drop the `refreshTick` dependency:

```js
const usersWithCursors = useMemo(() => {
return remoteUsers.filter(user => user.cursor != null && user.currentPage === getCurrentPage);
return remoteUsers.filter(user => user.cursor != null && user.currentPage === getCurrentPage);
}, [remoteUsers, getCurrentPage]);
```

Expand All @@ -176,6 +175,7 @@ Remove the `refreshTick` state and the `setInterval` entirely.
The sync manager replaces `project.studies` with a new array on every Yjs change. `selectStudy` does `studies.find(...)` which returns a new object even if the study data is structurally identical. This is the root amplifier: one Yjs change to one field invalidates every memo that depends on `currentStudy` in `ReconciliationWrapper`.

The store already acknowledges this risk at line 169-171:

```js
// Stable fallback constants -- must be module-level so they're referentially equal
// across renders. Without these, selectors return new objects/arrays on every call
Expand All @@ -194,14 +194,20 @@ import { useShallow } from 'zustand/react/shallow';

// Subscribe to just what you need:
const checklist1Meta = useProjectStore(
useShallow(s => {
const study = selectStudy(s, projectId, studyId);
if (!study?.checklists) return null;
const c = study.checklists.find(c => c.id === checklist1Id);
if (!c) return null;
return { id: c.id, type: c.type, status: c.status, assignedTo: c.assignedTo,
outcomeId: c.outcomeId, createdAt: c.createdAt };
})
useShallow(s => {
const study = selectStudy(s, projectId, studyId);
if (!study?.checklists) return null;
const c = study.checklists.find(c => c.id === checklist1Id);
if (!c) return null;
return {
id: c.id,
type: c.type,
status: c.status,
assignedTo: c.assignedTo,
outcomeId: c.outcomeId,
createdAt: c.createdAt,
};
}),
);
```

Expand Down
Loading