docs: trim AGENTS.md to one rule per feature, whys stay in ADRs#264
Merged
Merged
Conversation
Eight feature sections restated their ADRs at length. Cut the rationale each ADR verifiably owns (audited section-by-section against docs/adr/), keeping every operational rule, file pointer, and e2e spec reference. 143,472 -> 137,296 bytes (~4.3%) loaded into every agent session. Trimmed: mobile actions bar (0030), desktop selection toolbar (0036), spotlight (0033), Cmd+K (0034), rich links (0005/0016), markdown paste (0044/0045), data layer gotchas (0008/0013), daily notes (0041/0038). Skipped on purpose: virtualized rendering, Cloudflare deploy, plugins seam table - rule-dense, near-zero trim ROI. Also adds the missing token-kit.ts/token-splice.ts pointer for plugin authors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR updates ChangesRepository Guidance Refresh
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/purple-clouds-rush.md:
- Around line 1-2: Replace the empty fragment in the changeset with a valid bump
entry describing the documentation-only change, or update the release flow to
permit an all-empty no-op when no package release is required. Ensure the
resulting behavior allows bun run release to complete when this is the only
changeset.
In `@AGENTS.md`:
- Line 278: Update the structural-edits guidance to reference the
implementation’s waitForSeqE symbol instead of waitForSeq, preserving the rest
of the atomicity and call-site instructions unchanged.
- Line 312: Update the mobile actions bar testing note in AGENTS.md to state
that e2e/mobile-actions-bar.spec.ts covers coarse-pointer browser mounting,
focus/blur visibility, and button wiring, while only real-device iPhone keyboard
and viewport behavior requires manual validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a07fd97-f24a-48d0-89ce-cabc5fb92b6b
📒 Files selected for processing (2)
.changeset/purple-clouds-rush.mdAGENTS.md
Comment on lines
+1
to
+2
| --- | ||
| --- |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Prevent the empty changeset from blocking release.
This fragment parses as intentionally empty, but scripts/release.ts exits when all available fragments are empty. Therefore, if this is the only changeset, bun run release cannot complete for this documentation-only PR. Update the release flow to allow an all-empty no-op, or provide a valid bump fragment if this PR must produce a release.
As per coding guidelines, every PR must add a changeset and release validation must use bun run release.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.changeset/purple-clouds-rush.md around lines 1 - 2, Replace the empty
fragment in the changeset with a valid bump entry describing the
documentation-only change, or update the release flow to permit an all-empty
no-op when no package release is required. Ensure the resulting behavior allows
bun run release to complete when this is the only changeset.
Source: Coding guidelines
|
|
||
| - **Nodes live in a per-user Durable Object's SQLite** ([per-user DO sync](./docs/adr/0008-sync-via-a-per-user-durable-object.md)). `nodesCollection` is a TanStack DB _custom sync_ collection over `/api/sync` (`collection.ts` + `realtime.ts`); **field** writes PATCH `/api/nodes` (`api.ts`), **structural** writes go through `runStructural` as one atomic batch POST `{ops}` (see the next bullet). The socket is an **Effect scoped resource** ([ADR 0013](./docs/adr/0013-sync-socket-as-an-effect-resource.md)): `realtime.ts`'s `makeSyncStream` is a pure `Stream<SyncEvent>` producer (`Socket.makeWebSocket` + a backoff loop + reset-after-stable), and `collection.ts` forks **one long-lived fiber** on the shared `appRuntime` (`runtime.ts`) to fold it into the sync primitives. Don't reintroduce a callback socket. The echo waiters are Effect too: `waitForSeqE`/`waitForNodeE` (`collection.ts`) lift the registration with `Effect.callback` (resolve-on-timeout vs fail-on-timeout), and `runStructural` composes the batch send + echo-hold as ONE Effect bridged at its `mutationFn`; only the daily loser-path keeps a thin `waitForNode` Promise wrapper. The cursor (`appliedSeq`) is still a plain module value with callback resolves. **Side-collections (`tag-colors.ts`, `daily-index.ts`, `saved-queries.ts`) ride the same DO** as query collections over `/api/kv?collection=<name>` (`kv-api.ts` + `query-client.ts`); each passes its **concrete** Effect `Schema` inline (wrapped with `Schema.toStandardSchemaV1`, which TanStack DB consumes as a StandardSchema; a generic factory loses schema inference). **A new side-collection must ALSO be added to the Worker's `KV_COLLECTIONS` allowlist in `worker/index.ts`** — the e2e kv mock accepts any collection name, so only a live Worker catches the miss (every write 400s with `unknown kv collection`; this exact bug shipped with `saved-queries`). `saved-queries.ts` (ADR 0048) is the newest, mirroring `tag-colors.ts`'s `subscribeChanges` + `useSyncExternalStore` reactive reads (NOT `useLiveQuery`, prerender-safe). The old `dotflowy-oss:*` localStorage keys are no longer read. | ||
| - **Nodes live in a per-user Durable Object's SQLite** ([per-user DO sync](./docs/adr/0008-sync-via-a-per-user-durable-object.md)). `nodesCollection` is a TanStack DB _custom sync_ collection over `/api/sync` (`collection.ts` + `realtime.ts`); **field** writes PATCH `/api/nodes` (`api.ts`), **structural** writes go through `runStructural` as one atomic batch POST `{ops}` (see the next bullet). The socket is an **Effect scoped resource** ([ADR 0013](./docs/adr/0013-sync-socket-as-an-effect-resource.md)): `realtime.ts`'s `makeSyncStream` is a pure `Stream<SyncEvent>` producer that `collection.ts` folds into the sync primitives on **one long-lived fiber** on the shared `appRuntime` (`runtime.ts`). **Don't reintroduce a callback socket.** Echo waiters are Effect too (`waitForSeqE`/`waitForNodeE`, `collection.ts`); `runStructural` composes the batch send + echo-hold as ONE Effect at its `mutationFn`. **Side-collections (`tag-colors.ts`, `daily-index.ts`, `saved-queries.ts`) ride the same DO** as query collections over `/api/kv?collection=<name>` (`kv-api.ts` + `query-client.ts`); each passes its **concrete** Effect `Schema` inline (wrapped with `Schema.toStandardSchemaV1`, which TanStack DB consumes as a StandardSchema; a generic factory loses schema inference). **A new side-collection must ALSO be added to the Worker's `KV_COLLECTIONS` allowlist in `worker/index.ts`** — the e2e kv mock accepts any collection name, so only a live Worker catches the miss (every write 400s with `unknown kv collection`; this exact bug shipped with `saved-queries`). `saved-queries.ts` (ADR 0048) is the newest, mirroring `tag-colors.ts`'s `subscribeChanges` + `useSyncExternalStore` reactive reads (NOT `useLiveQuery`, prerender-safe). The old `dotflowy-oss:*` localStorage keys are no longer read. | ||
| - **Structural edits are atomic; field edits are direct.** Any tree-shape mutation (insert/indent/outdent/move/reparent/remove, undo/redo restore, daily get-or-create) MUST be wrapped in `runStructural` (`structural.ts`) so all its `nodesCollection` writes land as ONE batch (`POST /api/nodes {ops}` → DO `applyBatch` → one commit; batches over 500 ops echo as multiple consecutive-seq frames — chunked `recordChange`) AND the optimistic overlay is held until that frame's echo (`waitForSeq`) — both are load-bearing; removing either reintroduces the sibling-chain corruption. **Field edits** (`setText`, `toggleCompleted/Collapsed`, `setIsTask`, `toggleBookmark`) stay direct — single-field PATCH, already atomic, and the keystroke path must NOT await an echo. Wrap at the editor `commands`/history/plugin call sites, not inside `mutations.ts` (keeps it pure; `runStructural` self-guards nesting). Why: [Atomic structural writes](./docs/adr/0009-atomic-structural-writes.md). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the implementation’s waitForSeqE name.
src/data/structural.ts uses waitForSeqE, while this bullet says waitForSeq; update it to avoid directing contributors to a nonexistent symbol.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` at line 278, Update the structural-edits guidance to reference the
implementation’s waitForSeqE symbol instead of waitForSeq, preserving the rest
of the atomicity and call-site instructions unchanged.
| - **Dumb chrome over a facade.** `useMobileBarActions` re-exposes `commands`/`undo`/`redo` as zero-arg methods resolving `findFocusedId()` internally, so the bar inherits `runStructural` atomicity + protected-node guards and adds **no new mutation path**. Buttons are always-enabled; invalid actions no-op with feedback at the row. | ||
| - **Every button `preventDefault`s on `onPointerDown`** (keeps the caret/keyboard alive), but the **action fires on `pointerup`, gated by a ~10px movement threshold** so a scroll of the `overflow-x-auto` pill doesn't trigger the button under your finger. | ||
| - **The `/` button inserts a literal `/` via `document.execCommand("insertText", false, "/")`** so the row's own `detectSlash`/`useSlashMenu` opens the palette — don't hand-splice the DOM. | ||
| - **Not e2e-testable → manual iPhone checklist in the PR.** `e2e/mobile-actions-bar.spec.ts` covers coarse-only mount, focus/blur visibility, and each button's wiring. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== AGENTS.md excerpt ==\n'
sed -n '300,320p' AGENTS.md
printf '\n== Search for referenced test ==\n'
git ls-files 'e2e/mobile-actions-bar.spec.ts' 'e2e/*mobile*actions*bar*' 'e2e/**/*mobile*actions*bar*'
printf '\n== If present, show the test file excerpt ==\n'
if git ls-files 'e2e/mobile-actions-bar.spec.ts' >/dev/null 2>&1; then
sed -n '1,240p' e2e/mobile-actions-bar.spec.ts
fiRepository: cameronapak/dotflowy
Length of output: 12059
Clarify the e2e scope. e2e/mobile-actions-bar.spec.ts covers browser coarse-pointer mount gating, focus/blur visibility, and button wiring; only real-device iPhone keyboard/viewport behavior stays manual.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` at line 312, Update the mobile actions bar testing note in
AGENTS.md to state that e2e/mobile-actions-bar.spec.ts covers coarse-pointer
browser mounting, focus/blur visibility, and button wiring, while only
real-device iPhone keyboard and viewport behavior requires manual validation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Eight AGENTS.md feature sections restated their ADRs at length, so every agent session paid ~6 KB of context for rationale
docs/adr/already owns. Each section was audited against its ADRs and cut back to its operational rules — every trap, file pointer, and e2e reference kept.Changes
token-kit.ts/token-splice.ts), previously undocumented in AGENTS.md.Start here: change 1 — scan the daily-notes and data-layer hunks for any rule you rely on that went missing; the whole risk of this PR lives there.
Test plan
docs/adr/*.mdlink in AGENTS.md resolves to an existing file##headings, before and after)package.jsonscripts andsrc/plugins/index.ts(commands and plugin array match the doc)bunx changeset --emptyadded (docs-only, not news)🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores