From 126d673a9874b4d57b6678e80c979f8bda0deca1 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Mon, 14 Sep 2026 01:17:14 +0200 Subject: [PATCH 1/3] chore: seed the lode and enable the pre-PR gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every fact about this gem currently lives in a comment, an issue number, or a reviewer's memory. The lode is where it stops being tacit: twenty-two files describing the system as it is, each claim read off the code and cited to the file and method that makes it true. What it holds. Nine subsystem summaries — core and config, component, endpoint, streaming, async and defer, client runtime, tooling, testing and CI, the docs site — plus the root summary's three invariants (signed identity never state, default-deny with the signature not being authorization, pgbus optional and runtime-probed), the terminology this repo actually speaks, and the practices the .claude/rules do not state: capability probes over constant checks, failing at declaration time rather than click time, enumerating once, what may and may not be memoized on the render path. Review rules. Seven files, thirty-seven rules, from two sources. All eight cubic learnings were kept and verified against the current code; ten rules cite one. The merged PR threads on #253, #250, #229, #227, #223, #217 and #210 supplied the rest, including four rejections the author argued down and the reviewer withdrew, recorded as "Not a bug" so the next reviewer does not re-open them. Cubic learnings and their PR-thread twins were merged rather than written twice where they are the same finding seen from both ends. Nothing was dropped: every rule's subject still exists and the code still agrees with it. Twenty-three rules name the spec that proves them; ten say plainly that no automated test covers them — the docs-prose rules, the release-task rules (that task has no test; it is hand-verified against copies of the real lockfiles) and the test-hygiene rules, which are properties of assertions rather than behaviour. The four "Not a bug" entries carry the reasoning instead of a spec. Corrections. Seven claims in the drafted summaries disagreed with the code and were rewritten rather than kept: the loader's exclusions are six ignores plus two do_not_eager_loads, not five; only six settings guard their default with defined? (the rest are ||=, which is safe only because none of those defaults is false); the renderer default is conditional on ActionController::Base being defined; the vendored client is five named files, not every .js in that directory; the ShowConditions parity fixture is not under fixtures/files; and the defer endpoint's rescue chain also lacks the AuthorizationNotVerified clause. Nine doc-versus-code disagreements found along the way are reported separately, not recorded in the lode — a document states only what is true of the tree it ships in. Workflow. lode/workflow.md fills all ten headings from the code, CLAUDE.md, .claude/rules and the local commands this commit retires, so the shared /lode:* skills behave as though written for this repo: the generated client artifacts that must never be hand-merged, the shapes every change is checked against (both servers, both transports, pgbus absent/old/current, a dom-id String as well as a record), and the thirteen reviewer suggestions that are wrong here. Retired: lfg, tdd, plan, github-review-pr, github-review-failures and github-review-comments, superseded by the plugin's. finish-prs and debug-flaky were not present. architect, perf, security and review-pr have no plugin equivalent and stay. CLAUDE.md's command table and model-tier note were updated so neither names a command that no longer exists. Claude-Session: https://claude.ai/code/session_01Hga3idYBp7fWdj7GmmfofF --- .claude/commands/github-review-comments.md | 237 ------------------ .claude/commands/github-review-failures.md | 177 -------------- .claude/commands/github-review-pr.md | 164 ------------- .claude/commands/lfg.md | 267 --------------------- .claude/commands/plan.md | 101 -------- .claude/commands/tdd.md | 118 --------- .claude/settings.json | 13 + .gitignore | 3 + CLAUDE.md | 51 ++-- lode/async-and-defer/summary.md | 64 +++++ lode/client-runtime/summary.md | 54 +++++ lode/component/summary.md | 81 +++++++ lode/core-and-config/summary.md | 91 +++++++ lode/docs-site/summary.md | 61 +++++ lode/endpoint/summary.md | 94 ++++++++ lode/lode-map.md | 37 +++ lode/plans/README.md | 10 + lode/practices.md | 47 ++++ lode/review/async-actions.md | 61 +++++ lode/review/client-runtime.md | 39 +++ lode/review/collections.md | 36 +++ lode/review/docs-and-changelog.md | 29 +++ lode/review/observability.md | 28 +++ lode/review/release-and-changelog.md | 30 +++ lode/review/testing.md | 33 +++ lode/streaming/summary.md | 72 ++++++ lode/summary.md | 11 + lode/terminology.md | 39 +++ lode/testing-and-ci/summary.md | 61 +++++ lode/tooling/summary.md | 66 +++++ lode/workflow.md | 136 +++++++++++ 31 files changed, 1229 insertions(+), 1082 deletions(-) delete mode 100644 .claude/commands/github-review-comments.md delete mode 100644 .claude/commands/github-review-failures.md delete mode 100644 .claude/commands/github-review-pr.md delete mode 100644 .claude/commands/lfg.md delete mode 100644 .claude/commands/plan.md delete mode 100644 .claude/commands/tdd.md create mode 100644 .claude/settings.json create mode 100644 lode/async-and-defer/summary.md create mode 100644 lode/client-runtime/summary.md create mode 100644 lode/component/summary.md create mode 100644 lode/core-and-config/summary.md create mode 100644 lode/docs-site/summary.md create mode 100644 lode/endpoint/summary.md create mode 100644 lode/lode-map.md create mode 100644 lode/plans/README.md create mode 100644 lode/practices.md create mode 100644 lode/review/async-actions.md create mode 100644 lode/review/client-runtime.md create mode 100644 lode/review/collections.md create mode 100644 lode/review/docs-and-changelog.md create mode 100644 lode/review/observability.md create mode 100644 lode/review/release-and-changelog.md create mode 100644 lode/review/testing.md create mode 100644 lode/streaming/summary.md create mode 100644 lode/summary.md create mode 100644 lode/terminology.md create mode 100644 lode/testing-and-ci/summary.md create mode 100644 lode/tooling/summary.md create mode 100644 lode/workflow.md diff --git a/.claude/commands/github-review-comments.md b/.claude/commands/github-review-comments.md deleted file mode 100644 index 5a5f0486..00000000 --- a/.claude/commands/github-review-comments.md +++ /dev/null @@ -1,237 +0,0 @@ ---- -model: sonnet -description: "Use when a PR has unresolved review comments that need responses -- evaluates each comment, implements valid fixes, pushes back on incorrect suggestions, and resolves all threads." -argument-hint: "PR number (e.g., 123 or #123)" -allowed-tools: Bash(gh pr view:*), Bash(gh pr diff:*), Bash(gh pr comment:*), Bash(gh api:*), Bash(git log:*), Bash(git blame:*), Bash(git push:*), Bash(git commit:*), Bash(git add:*), Bash(bundle exec:*), Read, Write, Edit, Glob, Grep, Agent ---- - -# Review GitHub PR Comments: $ARGUMENTS - -You are reviewing and responding to all unresolved review comments on a GitHub pull request. Apply technical rigour -- evaluate each comment against the actual codebase before accepting or rejecting it. - -## Phase 0: Determine the PR Number - -The user may provide a PR number as `$ARGUMENTS`. Parse it flexibly: - -- `PR123`, `PR 123`, `pr123` -> PR 123 -- `123` -> PR 123 -- `#123` -> PR 123 -- Empty/blank -> auto-detect from current branch - -**If no PR number is provided**, detect it automatically: - -```bash -gh pr list --author=@me --head="$(git branch --show-current)" --state=open --json number,title -``` - -If exactly one open PR exists for the current branch, use it. If none or multiple, ask the user. - -Once you have the PR number, confirm it: - -```bash -gh pr view --json title,state,url -``` - ---- - -## Phase 1: Fetch All Unresolved Review Comments - -Retrieve all review comments and identify unresolved ones: - -```bash -# Get all review comments (not resolved) -gh api "repos/mhenrixon/phlex-reactive/pulls//comments" --paginate - -# Get all review threads to check resolution status -gh api graphql -f query=' - query($owner: String!, $repo: String!, $pr: Int!) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $pr) { - reviewThreads(first: 100) { - nodes { - id - isResolved - path - line - comments(first: 20) { - nodes { - id - databaseId - body - author { login } - createdAt - } - } - } - } - } - } - } -' -f owner=mhenrixon -f repo=pgbus -F pr= -``` - -For each unresolved thread, extract: -- Thread ID (for resolving) -- Comment body (the review feedback) -- File path and line number (if inline) -- Author (to understand context) - -Filter to only **unresolved** threads. Skip bot comments (CodeRabbit, dependabot), resolved threads, and PR description comments. - -If there are no unresolved review comments, report that and stop. - ---- - -## Phase 2: Read and Categorise Each Comment - -For each unresolved comment, read the full body and categorise it: - -| Category | Action | -|----------|--------| -| Valid fix needed | Implement the fix | -| Valid test gap | Add the missing test | -| Valid style/consistency issue | Fix it | -| Incorrect suggestion | Push back with technical reasoning | -| Suggestion conflicts with architecture | Push back, reference existing patterns | -| Over-engineering / YAGNI | Push back, explain why it's unnecessary | -| Unclear | Ask for clarification (do NOT implement) | - -**Before categorising**, always: -1. Read the actual file and line being commented on -2. Check if the suggestion is technically correct for THIS codebase -3. Check if it would break existing functionality -4. Check if existing patterns/conventions contradict the suggestion -5. Check CLAUDE.md rules -- project conventions override reviewer preferences - ---- - -## Phase 3: Implement Accepted Fixes - -For all comments you've decided to accept: - -1. **Make the code changes** -- edit the relevant files -2. **Run affected tests** to verify nothing breaks: - ```bash - bundle exec rspec - ``` -3. **Run validators**: - ```bash - bundle exec rubocop - ``` -4. **Commit** all fixes together with a clear message: - ```bash - git commit -m "$(cat <<'EOF' - fix: address PR review feedback - - - Description of fix 1 - - Description of fix 2 - EOF - )" - ``` -5. **Push** to the remote branch: - ```bash - git push - ``` - ---- - -## Phase 4: Reply to Every Comment - -For **each** unresolved thread, reply: - -### For accepted fixes: - -Reply with what was fixed and the commit SHA: - -```bash -gh api "repos/mhenrixon/phlex-reactive/pulls//comments//replies" \ - --method POST \ - -f 'body=Fixed in . .' -``` - -### For rejected suggestions: - -Reply with technical reasoning: - -```bash -gh api "repos/mhenrixon/phlex-reactive/pulls//comments//replies" \ - --method POST \ - -f 'body=' -``` - -### Resolving threads (via GraphQL): - -After replying, resolve the thread: - -```bash -gh api graphql -f query=' - mutation($threadId: ID!) { - resolveReviewThread(input: {threadId: $threadId}) { - thread { isResolved } - } - } -' -f threadId= -``` - -### For general PR comments (not inline review threads): - -Reply directly: - -```bash -gh pr comment --body "" -``` - ---- - -## Phase 5: Verify Completion - -After processing all comments, verify no unresolved threads remain: - -```bash -gh api graphql -f query=' - query($owner: String!, $repo: String!, $pr: Int!) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $pr) { - reviewThreads(first: 100) { - totalCount - nodes { isResolved } - } - } - } - } -' -f owner=mhenrixon -f repo=pgbus -F pr= -``` - -Report the final tally: how many comments were accepted/fixed, how many were pushed back on, and confirm all threads are resolved. - ---- - -## Response Style - -When replying to comments: - -- **No performative agreement** -- never say "Great point!" or "You're absolutely right!" -- **No gratitude** -- never say "Thanks for catching that" -- **Be direct** -- state the fix or the reasoning, nothing more -- **Reference commits** -- always include the short SHA when a fix was made -- **Be specific** -- when pushing back, reference actual code, not abstract principles - -When pushing back: - -- Use technical reasoning grounded in the actual codebase -- Reference existing patterns if the suggestion contradicts them -- Reference CLAUDE.md rules when applicable -- Explain what would break or what edge case the reviewer missed -- If the suggestion is valid in principle but wrong for this context, say so - ---- - -## Important Notes - -- Always read the actual code before evaluating a comment -- reviewers sometimes misread diffs -- If a comment reveals a genuine bug you missed, fix it without defensiveness -- If multiple comments suggest the same change, implement it once and reference the fix in all replies -- Bot reviewers (CodeRabbit, etc.) sometimes suggest changes that conflict with project conventions -- verify against CLAUDE.md -- If a new round of review comments appears after your push (from re-review), report that to the user rather than entering an infinite loop - -Now begin by determining the PR number from `$ARGUMENTS` or the current branch. diff --git a/.claude/commands/github-review-failures.md b/.claude/commands/github-review-failures.md deleted file mode 100644 index 83ba40a2..00000000 --- a/.claude/commands/github-review-failures.md +++ /dev/null @@ -1,177 +0,0 @@ ---- -model: sonnet -description: "Use when CI checks are failing on a PR — fetches failure logs, diagnoses root causes, implements fixes, and pushes until CI is green." -argument-hint: "PR number (e.g., 41 or #41)" -allowed-tools: Bash(gh pr view:*), Bash(gh pr checks:*), Bash(gh pr diff:*), Bash(gh api:*), Bash(gh run view:*), Bash(git log:*), Bash(git diff:*), Bash(git push:*), Bash(git commit:*), Bash(git add:*), Bash(bundle exec:*), Read, Write, Edit, Glob, Grep, Agent ---- - -# Fix GitHub CI Failures: $ARGUMENTS - -You are diagnosing and fixing CI failures on a GitHub pull request. Work systematically: identify failures, read logs, diagnose root causes, fix locally, verify, push. - -## Phase 0: Determine the PR Number - -The user may provide a PR number as `$ARGUMENTS`. Parse it flexibly: - -- `PR41`, `PR 41`, `pr41` -> PR 41 -- `41` -> PR 41 -- `#41` -> PR 41 -- Empty/blank -> auto-detect from current branch - -**If no PR number is provided**, detect it automatically: - -```bash -gh pr list --author=@me --head="$(git branch --show-current)" --state=open --json number,title -``` - -If exactly one open PR exists for the current branch, use it. If none or multiple, ask the user. - -Once you have the PR number, confirm it: - -```bash -gh pr view --json title,state,url,mergeable -``` - -**Pre-flight: merge conflicts (detection only).** If `mergeable` is `CONFLICTING`, STOP — do not diagnose CI on a conflicted branch (the merge itself may fix or cause the failures). Report the conflict and hand off to `/github-review-pr`, whose Phase A0 owns the resolution runbook — this command's toolset deliberately does not include the merge machinery. If `mergeable` is `UNKNOWN`, note it and proceed: the orchestrator resolves the ambiguity; a standalone run shouldn't block on GitHub's recompute. - ---- - -## Phase 1: Identify Failing Checks - -```bash -gh pr checks -``` - -Categorise each failing check: - -| Check Type | Examples | How to Get Logs | -|------------|----------|----------------| -| Lint (rubocop) + gem build | `Lint` | `gh run view --job= --log-failed` | -| Unit + request specs | `Ruby 3.2` / `3.3` / `3.4` | `gh run view --job= --log-failed` | -| Browser system specs (Playwright) | `System (browser)` | `gh run view --job= --log-failed` | - -Extract the run ID and job IDs from the check URLs. The URL format is: -`https://github.com/mhenrixon/phlex-reactive/actions/runs//job/` - -If all checks pass or are pending, report that and stop. - ---- - -## Phase 2: Fetch Failure Logs - -For each failing check, get the logs: - -```bash -# Get the failed job logs (condensed output) -gh run view --job= --log-failed -``` - -If `--log-failed` output is too large or unclear, try: - -```bash -# Full log for a specific job -gh run view --job= --log 2>&1 | tail -100 -``` - ---- - -## Phase 3: Diagnose Each Failure - -For each failure, determine the root cause: - -### Lint Failures - -Look for: -- RuboCop offenses: file path, line number, cop name, message - -**Key**: RuboCop failures can often be auto-fixed with `bundle exec rubocop -A `. - -### Spec Failures - -Look for: -- Test name and file path -- Error class and message -- Relevant backtrace lines (ignore framework noise) -- Whether it's a test environment issue vs actual code bug - -**Key patterns**: -- `NameError: uninitialized constant` -> missing require or renamed class -- `NoMethodError: undefined method` -> API change, missing method -- `ActiveRecord::StatementInvalid` -> migration issue, missing column -- `expected: X, got: Y` -> logic bug or test needs updating - -### Build Failures - -Look for: -- Gem build errors: missing files in gemspec, syntax errors -- Bundle install failures: dependency conflicts - ---- - -## Phase 4: Fix Locally - -For each diagnosed failure: - -1. **Read the relevant file** to understand context before fixing -2. **Make the fix** -- edit the file -3. **Verify locally** before committing: - -```bash -# For rubocop failures -bundle exec rubocop - -# For spec failures -bundle exec rspec - -# For full validation -bundle exec rake -``` - -### Fix Priority Order - -1. **Lint/style fixes** first (fast, deterministic) -2. **Spec failures** second (may require understanding the code change) -3. **Build issues** third (usually gemspec or dependency) - ---- - -## Phase 5: Commit and Push - -```bash -git add -git commit -m "$(cat <<'EOF' -fix(ci): - -- Fix 1 description -- Fix 2 description -EOF -)" -git push -``` - ---- - -## Phase 6: Verify - -After pushing, check if CI has been re-triggered: - -```bash -gh pr checks -``` - -If there are still pending checks, report which checks are running and what was fixed. Do NOT poll in a loop -- report the status and let the user know. - -If you can identify that certain failures will persist for environmental reasons (e.g., a pgbus-dependent spec that needs PostgreSQL, or a system spec where Playwright browsers failed to install), flag that explicitly. - ---- - -## Important Notes - -- **Read before fixing** -- always read the actual failing code before attempting a fix -- **Fix the root cause** -- don't add `# rubocop:disable` to bypass lint; fix the actual issue (a targeted `# rubocop:disable` is acceptable only when RuboCop is demonstrably wrong, e.g. `save_screenshot` flagged as a debugger) -- **Don't fix unrelated failures** -- if a spec was already failing on main, note it but don't fix it in this PR -- **CI environment differences** -- system specs run Playwright (cached browsers) under a server matrix: Puma (default) AND Falcon (`CAPYBARA_SERVER=falcon`). A failure on only one server is a transport-specific bug, not flakiness. pgbus-dependent specs run only on Ruby >= 3.3 (pgbus's floor) and need PostgreSQL; on 3.2 they are skipped by design. -- **Flaky tests** -- if a test passes locally but fails in CI, note it as potentially flaky rather than adding workarounds. Browser specs that assert a value right after a click (racing the morph) are a common false-flake — fix them to use a waiting matcher. -- **Don't retry CI blindly** -- diagnose first, fix, then push. Each push triggers a full CI run. - -Now begin by determining the PR number and fetching the failing checks. diff --git a/.claude/commands/github-review-pr.md b/.claude/commands/github-review-pr.md deleted file mode 100644 index 769f82f4..00000000 --- a/.claude/commands/github-review-pr.md +++ /dev/null @@ -1,164 +0,0 @@ ---- -model: opus -description: "Use when a PR needs full review — resolves merge conflicts with the base first, then fixes CI failures, then addresses unresolved review comments. Conflicts first so CI diagnoses the post-merge reality; failures before comments because comment fixes trigger new CI runs that obscure the original failures." -argument-hint: "PR number (e.g., 156 or #156)" -allowed-tools: Bash(gh pr list:*), Bash(gh pr view:*), Bash(gh pr checks:*), Bash(gh pr checkout:*), Bash(gh pr diff:*), Bash(gh pr comment:*), Bash(gh api:*), Bash(gh run view:*), Bash(git log:*), Bash(git blame:*), Bash(git diff:*), Bash(git status:*), Bash(git switch:*), Bash(git fetch:*), Bash(git merge:*), Bash(git merge-tree:*), Bash(git rev-parse:*), Bash(git push:*), Bash(git commit:*), Bash(git add:*), Bash(bundle exec:*), Bash(bundle install:*), Bash(rake:*), Bash(bun:*), Bash(cp:*), Bash(cd:*), Read, Write, Edit, Glob, Grep, Agent ---- - -# Review GitHub PR (full pass): $ARGUMENTS - -You are running a full review pass on a pull request. The pass has three phases that MUST run in this order: - -1. **Phase A0: merge conflicts** — bring the branch up to date with its base and resolve any conflicts before anything else. -2. **Phase A: CI failures** — fix anything red before touching review comments. -3. **Phase B: review comments** — only after Phase A leaves CI green (or pending green after a push). - -## Why this order matters - -**Conflicts before failures**: CI results only matter for the code that will actually merge. On a conflicted (or stale) branch you'd diagnose failures against a base that no longer exists — and the conflict resolution itself changes code, invalidating the run you just fixed. Resolving conflicts first means Phase A reads CI for the post-merge reality, and you spend exactly one extra CI cycle instead of two. - -**Failures before comments**: if you fix review comments first, every commit pushes a new CI run. By the time the review-comment fixes finish, the original failure logs are buried under new pipeline runs. Symptoms: - -- The "specs (8, 4) failed" log you needed to read is now from a stale run; the latest run is still in progress on top of your unrelated comment fixes. -- A review-comment fix accidentally repairs the CI failure as a side effect, and you lose the chance to verify the failure was real. -- A review-comment fix accidentally INTRODUCES a CI failure, and you can't tell whether the new failure was pre-existing or your fault. - -Conflicts-first, then failures-first eliminates this confusion. CI is either green or red on a known commit against the current base; the review-comment fixes layer cleanly on top. - -## Phase 0: Determine the PR Number - -The user may provide a PR number as `$ARGUMENTS`. Parse it flexibly: - -- `PR156`, `PR 156`, `pr156` → PR 156 -- `156` → PR 156 -- `#156` → PR 156 -- Empty/blank → auto-detect from current branch - -**If no PR number is provided**, detect it automatically: - -```bash -gh pr list --author=@me --head="$(git branch --show-current)" --state=open --json number,title -``` - -If exactly one open PR exists for the current branch, use it. If none or multiple, ask the user. - -Once you have the PR number, confirm it: - -```bash -gh pr view --json title,state,url -``` - ---- - -## Phase A0: Merge conflicts - -Check whether the branch merges cleanly into its base: - -```bash -gh pr view --json mergeable,mergeStateStatus,baseRefName -``` - -| `mergeable` | Action | -|-------------|--------| -| `MERGEABLE` | Skip to Phase A. | -| `UNKNOWN` | GitHub is recomputing (common right after pushes, and it can stay UNKNOWN for minutes). Don't poll it — verify **locally**, against the PR's actual head (NOT `HEAD`, which may be some other checked-out branch): `git fetch origin ` and `git fetch origin pull//head`, verify both refs resolve (`git rev-parse --verify origin/^{commit}` and `git rev-parse --verify FETCH_HEAD^{commit}` — a bad ref also exits 1 from merge-tree, so exit code alone can't be trusted), then `git merge-tree --write-tree --name-only origin/ FETCH_HEAD`. Clean exit → no conflicts, skip to Phase A. Exit 1 **with conflict output** → resolve below (the `--name-only` file list is your work list). | -| `CONFLICTING` | Resolve, below. | - -### Resolution procedure - -1. Check out the PR's branch (`gh pr checkout `) with a clean tree (`git status`). Stash nothing — if the tree is dirty, stop and ask the user. -2. `git fetch origin ` then **`git merge origin/`** — MERGE, never rebase. The branch is shared (it has a PR); a rebase would require a force-push, which `.claude/rules/git-workflow.md` forbids on shared branches. -3. Resolve every conflicted file **semantically** — read both sides and produce the version that preserves BOTH changes' intent. Never blanket `--ours`/`--theirs` a source file. Repo-specific rules: - - **The gem's generated client artifacts** — `app/javascript/phlex/reactive/*.min.js` + `*.min.js.map`, and the FIVE vendored twins named in `spec/phlex/vendored_controller_sync_spec.rb`'s map (`spec/dummy/public/vendor/{reactive_controller,confirm,confirm_predicate,compute,inspect}.js`): NEVER hand-merge the artifact. Resolve the SOURCE file (`reactive_controller.js` etc.) semantically, then regenerate: `rake build:js` and `cp` each rebuilt `.min.js` over its vendored twin. `rake build:js_check` and the sync spec will catch a hand-merged artifact — trust them. Everything ELSE under `spec/dummy/public/vendor/` (stimulus.js, turbo.js, the turbo-rails/pgbus shims, the dummy's hand-written reducers) is NOT generated — merge those semantically like any source, or re-vendor from upstream. - - **`CHANGELOG.md` (Unreleased)**: union — keep BOTH sides' entries (main's landed bullets and this branch's), most recent first, without duplicating the `### Added`/`### Fixed` subheads. Losing either side is a real regression reviewers rarely catch. - - **`lib/phlex/reactive/version.rb`**: releases land DIRECTLY on `main` via `rake release` (no PR), so an ordinary feature branch never edits this file — a conflict here means the BRANCH bumped it on purpose (a release-prep PR). Keep the branch's bump in that case; if the intent isn't obvious from the branch's own commits, stop and ask. Only take the base's version when the branch's edit was clearly accidental. - - **`Gemfile.lock` and `docs/Gemfile.lock`** (both tracked since #246; both pin the gem by local path, so `rake release` re-locks and commits both): take the base's file, then run `bundle install` (in the gem root, or in `docs/`) so the branch's own dependency changes, if any, re-resolve on top. Never hand-edit a lockfile. - - **Append-only registries** (`docs/app/models/doc.rb`, route files, `spec/dummy/config/routes.rb`): both sides usually appended — keep both lines, in base order first. -4. Run the verification gates BEFORE pushing the merge — scoped to what the conflict touched, at minimum: - ```bash - bundle exec rubocop - bundle exec rspec spec/phlex spec/requests - # client artifacts involved: - bun test spec/javascript && rake build:js_check - # docs/ files involved (docs' CI lint gate is its own rake lint task — - # a bare rubocop there inspects 0 files): - cd docs && bundle exec rake lint && bundle exec rspec - ``` -5. Commit the merge (keep git's standard merge-commit message; add a body line naming any non-obvious resolution choice) and `git push` — a merge commit never needs force. - -### Phase A0 exit criteria - -- The PR reports `MERGEABLE` (or the local `git merge-tree` check is clean), AND the merge commit (if one was needed) is pushed. -- If the merge produced changes, CI is now re-running — that's expected; Phase A reads the fresh run. -- If a conflict cannot be resolved with confidence (both sides rewrote the same logic and the correct combination isn't decidable from the code), **stop and ask the user** — a guessed resolution that compiles is worse than a question. - ---- - -## Phase A: Run `/github-review-failures` - -Invoke the existing `/github-review-failures` slash command with the same `$ARGUMENTS` value. Its purpose: fix every failing CI check, push, leave the branch in a state where CI is either green or running-pending-toward-green. - -Follow that command's full process — phases 1–6 of the failures runbook. The slash command is at `.claude/commands/github-review-failures.md`. Its workflow: - -1. Identify failing checks via `gh pr checks `. -2. Fetch failure logs. -3. Diagnose root cause for each. -4. Fix locally — lint first (fast, deterministic), then specs, then build issues. -5. Verify locally before commit (`bundle exec rspec `, `bundle exec rubocop`). -6. Commit + push + report which checks are now running. - -### Phase A exit criteria - -Before moving to Phase B, one of these must be true: - -- All CI checks are green on the latest pushed commit. OR -- All CI checks are pending (running) on the latest pushed commit, AND no checks failed in the most recent completed run on this commit. OR -- A persistent CI failure exists that is **not caused by changes on this branch** (e.g., a flaky test on `main`, a secret-scanning job that fails for environmental reasons). Report this explicitly and proceed to Phase B with the caveat noted. - -If failures persist on this branch's changes, **do NOT proceed to Phase B**. Report what's still failing, what's been tried, and ask the user how to proceed. - ---- - -## Phase B: Run `/github-review-comments` - -Once Phase A's exit criteria are met, invoke `/github-review-comments` with the same `$ARGUMENTS`. Its purpose: address every unresolved review thread on the PR, push fixes, reply with commit SHAs, and resolve the threads. - -The slash command is at `.claude/commands/github-review-comments.md`. Its workflow: - -1. Fetch all unresolved review threads via the GitHub GraphQL API. -2. Read and categorise each comment (valid fix / invalid suggestion / unclear). -3. Implement accepted fixes; verify locally (specs, validators, rubocop). -4. Commit all fixes together with a clear message; push. -5. Reply to every thread with the commit SHA (for accepted fixes) or technical reasoning (for rejections). -6. Resolve each thread via the GraphQL `resolveReviewThread` mutation. -7. Verify no unresolved threads remain. - -### Phase B exit criteria - -- All unresolved review threads have been replied to and resolved (or the user has explicitly approved leaving a specific thread open). -- The branch has been pushed with all accepted fixes. - ---- - -## Phase C: Final report - -Before reporting, re-check mergeability once more (`gh pr view --json mergeable`, or the local `git merge-tree` check if UNKNOWN) — the base can move underneath a long pass. If a NEW conflict appeared, loop back to Phase A0. - -After all phases complete, report: - -1. **Phase A0 summary**: whether the branch was conflicted, which files conflicted, how each was resolved (and the merge commit SHA) — or "clean merge, no action". -2. **Phase A summary**: which CI failures were diagnosed and fixed. Note the commit SHAs for the fixes. -3. **Phase B summary**: which review comments were accepted (with commit SHAs), which were pushed back on (with reasoning), and the final unresolved-thread count (should be 0). -4. **End state**: final mergeability + CI status on the latest commit. -5. **Outstanding work**: anything that still needs attention — e.g., CI was pending at the end of Phase B and the user should verify the latest run after the comment fixes. - ---- - -## Important Notes - -- **Do not interleave the phases.** Don't fix a CI failure, then a review comment, then another CI failure. The whole point of this command is the strict ordering. -- **A new CI failure emerging during Phase B** (e.g., a comment fix breaks a spec) means looping back to Phase A — fix the new failure before continuing comment work. Likewise, **a new conflict appearing mid-pass** (the base moved) means looping back to Phase A0. These loop-backs are the only allowed reverse directions. -- **If the PR is already merged**, there is nothing to review — report that and stop. (A stale `$ARGUMENTS` or a just-merged PR shows up as `state: MERGED` in Phase 0's confirm step.) -- **If the PR merges cleanly, has no failures AND no unresolved comments**, report "PR is clean" and stop. -- **If `$ARGUMENTS` is the same as the current open PR**, the two child slash commands will see the same PR. They share state through the git branch and the GitHub API, not through any in-process variable. -- **Don't re-implement the child slash commands' logic**. Invoke them and let them do their work. This command is the orchestrator. diff --git a/.claude/commands/lfg.md b/.claude/commands/lfg.md deleted file mode 100644 index f2dd5382..00000000 --- a/.claude/commands/lfg.md +++ /dev/null @@ -1,267 +0,0 @@ ---- -model: opus -description: "Executes full autonomous engineering workflow with verification. Use when implementing complete features, tackling GitHub issues, or running end-to-end development cycles." -argument-hint: "GitHub issue number/URL or feature description" -allowed-tools: Bash(gh issue view:*), Bash(gh search:*), Bash(gh issue list:*), Bash(gh pr create:*), Bash(gh pr view:*), Bash(bundle exec:*), Bash(git:*), Read, Write, Edit, Glob, Grep, Agent ---- - -# LFG - Full Autonomous Workflow - -Execute a complete engineering workflow with verification at each phase. - -## Phase 0: Branch Setup - -**BEFORE any other work, prepare the git branch:** - -1. Check the current branch: `git branch --show-current` -2. If NOT on `main`, switch: `git checkout main` -3. Pull latest: `git pull origin main` -4. Create feature branch: `git checkout -b issue-{number}-{brief-description}` (or `feature/{description}` if no issue number) - ---- - -## Phase 1: Understand - -### Step 1: Gather Requirements - -If `$ARGUMENTS` is a GitHub issue number or URL: - -```bash -gh issue view --json title,body,labels,assignees,comments -``` - -If `$ARGUMENTS` is a description, use it directly. - -### Step 2: Define Acceptance Criteria - -**MANDATORY:** Write explicit acceptance criteria: - -- **GIVEN** [context/setup] -- **WHEN** [action taken] -- **THEN** [expected outcome] - -You MUST NOT proceed until you can articulate these clearly. - -### Step 3: Comprehension Gate - -Before proceeding, you must: - -1. State the problem/feature in one sentence -2. Explain WHY this is needed (the user-facing payoff — "a pleasure to work with") -3. List what changes from the developer's perspective (the API delta) -4. Identify edge cases not explicitly mentioned -5. Explain the data flow: client event → endpoint → action → re-render → DOM, and/or the broadcast path - -If you cannot complete ALL five items, investigate further. - -### Step 4: Create Task List - -Create a TaskCreate todo list with specific implementation steps. - ---- - -## Phase 2: Explore - -1. Find related files (Glob/Grep or Explore agent) -2. Read existing patterns in similar features -3. Understand integration points across the layers -4. Check existing test coverage in `spec/` -5. Review the Streamable mixin in `lib/phlex/reactive/streamable.rb` -6. Review the Component mixin in `lib/phlex/reactive/component.rb` -7. Review the action endpoint in `app/controllers/phlex/reactive/actions_controller.rb` -8. Review the client runtime in `app/javascript/phlex/reactive/reactive_controller.js` -9. If a pgbus primitive is involved, **verify its real signature** in `~/Code/mhenrixon/pgbus` — do not assume the wire format - ---- - -## Phase 3: Plan - -1. List files to modify with specific changes -2. List new files to create with purpose -3. Identify the pgbus-present vs pgbus-absent behavior (the optionality invariant) -4. Plan test coverage across layers (TDD: tests FIRST) — unit, request, broadcast, system -5. Update the task list -6. Consider backwards compatibility (existing components must keep working verbatim) - ---- - -## Phase 4: Implement (TDD) - -### The deviation log (keep it from the first edit) - -The plan is the map; the codebase is the territory. The moment reality forces a choice the plan or issue didn't settle, log it in `implementation-notes.md` at the repo root — one line, at the moment it happens, not reconstructed later: - -- **Deviations** — the plan said X, you did Y, because Z -- **Discoveries** — facts about the codebase the plan didn't know -- **Judgment calls** — choices the user might have made differently (defaults, naming, scope cuts) - -Pick the conservative option and keep going. The log is how the user audits your judgment afterwards. Never commit the file: its contents move into the PR body (Phase 7), then the file is deleted. - -For each logical unit: - -### 4.1: Write Failing Test First - -```bash -bundle exec rspec -``` - -### 4.2: Implement Minimum Code - -Write the MINIMUM code to make the test pass. Follow project patterns: - -| Never Do | Always Do | -|----------|-----------| -| Hand-pick a Turbo target | Component self-targets via `#id` | -| Ship state to the client | Sign identity (`{c, gid}`); re-find server-side | -| Trust client input for authz | `authorize!` inside the action | -| Undeclared / raw-param actions | `action :name, params: {...}` (default-deny) | -| Fabricate a view context | Render through `Phlex::Reactive.renderer` | -| Assume pgbus / call a pgbus keyword blind | Capability-gate (`pgbus_streams?`) + fallback | -| `dom_id` (Phlex helper) in `#id` | `Streamable#dom_id` (render-context-free) | - -### 4.3: Refactor - -Once green, refactor while keeping tests passing. - -### 4.4: Validate - -```bash -bundle exec rubocop -``` - -### 4.5: Repeat - -Move to the next unit. Mark task items complete. - ---- - -## Phase 5: Deep Root Cause Analysis (Bug Fixes Only) - -**If this is a bug fix, investigate before implementing.** - -### Trace the lifecycle - -For the failing interaction: -- Where did the client event originate? What token did it carry? -- Did the action re-render, or did a broadcast deliver it? Both? -- What ASSUMPTIONS does the code make at the failure point? Which was violated, and WHY? - -### Use git history - -```bash -git log --oneline -20 -git blame -``` - -### Map all callers - -Use Grep to find every call site. Does the bug happen only on the pgbus path? Only on Action Cable? Only for the actor (echo)? Only on the first action (connection-id race)? - -### Five Whys - -Keep asking WHY until you reach the real fix point. - -### Fix-location principle - -The best fix is usually NOT where the error surfaced: -- Double-applied broadcast → suppress the actor echo / dedup by id, not a `rescue` -- `ArgumentError: unknown keyword :exclude` → the capability gate, not a `begin/rescue` -- Stale token under rapid clicks → the request queue + token threading, not a debounce -- `HelpersCalledBeforeRenderError` in `#id` → use `Streamable#dom_id`, not a `rescue` - -### Unacceptable superficial fixes — DO NOT DO THESE - -- `rescue nil` / bare `rescue` to silence an error you don't understand -- `&.` to paper over a nil without finding why it's nil -- `return if x.nil?` to silently skip -- swallowing errors instead of logging + fixing the cause - -**These HIDE bugs. Find the EARLIEST point you could prevent the error and fix there.** - ---- - -## Phase 6: Verify - -**ALL of these must pass before committing:** - -```bash -bundle exec rubocop -bundle exec rspec spec/phlex spec/requests -# client-touching changes: also run the browser suite -bundle exec rspec spec/system # Puma (default); CAPYBARA_SERVER=falcon for the async server -bundle exec rake spec:system_servers # client-touching changes: run BOTH real servers (puma + falcon) -``` - -### Solution verification - -- "If I were the requester, is this fully resolved?" -- "Did I fix the ROOT CAUSE, not the symptom?" -- "Do the tests prove it, including the pgbus-absent fallback?" -- "Does every existing component still work verbatim (backwards compatible)?" - ---- - -## Phase 7: Commit & PR - -### Commit - -```bash -git add -git commit -m "$(cat <<'EOF' -feat(scope): brief description - -## Summary -[What changed and why] - -## Test Coverage -- spec 1: validates X -- spec 2: validates the pgbus-absent fallback - -## Verification -- [x] bundle exec rubocop passes -- [x] bundle exec rspec passes -EOF -)" -``` - -### Push & PR - -```bash -git push -u origin $(git branch --show-current) - -gh pr create --title "feat(scope): brief description" --body-file /tmp/pr-body.md -``` - -Write the PR body to a temp file (`--body-file`) to avoid shell-interpolation of -backticks/tables. The body is copied verbatim — if you would not type a -backslash in a GitHub comment, do not type one in the heredoc. - -The PR body MUST end with a `## Deviations & judgment calls` section copied from -`implementation-notes.md` (then delete the file). If the plan held completely, -write "None — the plan held." This section is read FIRST in review — it is the -audit trail for every decision the plan didn't make. - ---- - -## Phase 8: Comprehension Close-Out - -The tests prove the CODE is right; this phase keeps the USER's mental model right. After the PR is up, end your final message with: - -1. **The decisions, not the diff** — the 3–5 non-obvious choices in this change someone must understand to maintain it. Lead with anything from the deviation log; the user has never seen those. -2. **Three merge-gate questions** the user should be able to answer before merging (e.g. "why does the verify run inside the transaction?"). If any answer isn't obvious to them, offer a walkthrough — an unanswerable question is comprehension debt, and merging anyway is how it compounds. - ---- - -## Verification Checklist - -- [ ] All acceptance criteria met -- [ ] Tests written BEFORE implementation -- [ ] `bundle exec rubocop` passes -- [ ] `bundle exec rspec` passes (browser suite too, if the client changed) -- [ ] Backwards compatible — existing components unchanged -- [ ] pgbus optionality preserved (works with pgbus AND on Action Cable) -- [ ] PR created with summary + test plan -- [ ] PR body ends with `## Deviations & judgment calls` (from implementation-notes.md, since deleted) -- [ ] Comprehension close-out delivered (decisions + three merge-gate questions) - -Now, execute this workflow for the provided issue or feature. diff --git a/.claude/commands/plan.md b/.claude/commands/plan.md deleted file mode 100644 index c7069d55..00000000 --- a/.claude/commands/plan.md +++ /dev/null @@ -1,101 +0,0 @@ ---- -model: fable -description: "Investigates the codebase, designs a solution, and produces a durable plan artifact — a GitHub issue or a plan markdown under docs/plans/. Read-only: never edits library or app code. Use before /lfg for anything non-trivial." -argument-hint: "issue | md | " -allowed-tools: Bash(gh issue create:*), Bash(gh issue list:*), Bash(gh issue view:*), Bash(gh search:*), Bash(gh label list:*), Bash(git log:*), Bash(git diff:*), Bash(git branch:*), Bash(date:*), Read, Grep, Glob, Write, Agent, AskUserQuestion ---- - -# Plan — design expensive, execute cheap - -You are the planning specialist. This command runs on the most capable model deliberately: the thinking happens here, the execution happens later on cheaper models (`/lfg` on Opus, `/tdd` and the review specialists on Sonnet). That split only works if the plan is **self-contained** — an executor with none of this session's context must be able to implement it without guessing. - -## Output mode from $ARGUMENTS - -| $ARGUMENTS starts with | Artifact | -|------------------------|----------| -| `issue` | GitHub issue (default — feeds directly into `/lfg `) | -| `md` or `file` | Markdown file at `docs/plans/YYYY-MM-DD-.md` (date from `date +%F`) | -| anything else | GitHub issue | - -## Hard constraints - -- **Read-only for source code.** Never edit library (`lib/`, `app/`) or spec code, never commit, never create branches. The only file you may Write is a new plan markdown under `docs/plans/`. -- **Never reproduce secrets** (keys, tokens, credentials, the `MessageVerifier` secret) in the plan, even redacted ones you encounter while reading config. -- **Dedupe before creating an issue**: `gh issue list --search ""` — if an existing issue covers this, extend it in your summary instead of duplicating. - -## Phase 1 — Investigate - -Protect this session's context: delegate mechanical exploration to cheaper subagents and keep Fable for judgment. - -1. Fan out Explore agents (`subagent_type: Explore`) for file discovery and naming-convention sweeps across the gem (`lib/`, `app/`) and the dummy app (`spec/dummy/`). Launch independent explorations in parallel. If a pgbus primitive is involved, point one at `~/Code/mhenrixon/pgbus` to verify its real signature — don't assume the wire format. -2. Read the load-bearing files yourself — the ones the design decision actually hinges on. Don't design from subagent summaries alone. The layers: `lib/phlex/reactive/streamable.rb`, `lib/phlex/reactive/component.rb`, `lib/phlex/reactive/response.rb`, `app/controllers/phlex/reactive/actions_controller.rb`, `app/javascript/phlex/reactive/reactive_controller.js`, `lib/phlex/reactive.rb`. -3. Read `CLAUDE.md` and the matching `.claude/rules/*.md` (coding-style, testing, performance, git-workflow, agents) — the invariants and gotchas live there. The published `docs/` site pages (architecture, security, broadcasting, transport-pgbus, testing, performance) are the deeper reference. -4. Check `git log` for recent related work; the design should extend it, not fight it. - -## Phase 2 — Surface the unknowns (blindspot pass + interview) - -Investigation tells you what the codebase says; this phase finds what the REQUEST doesn't say. Run it BEFORE designing — a wrong assumption caught here costs one question; caught in review it costs a rewrite. - -1. **Blindspot pass.** Write down the unknowns you are carrying into the design: - - decisions the request leaves open (defaults, naming, public API/config surface, rollout & upgrade story) - - edge cases the codebase makes possible that the request never mentions - - anything with no precedent in this repo — flag it explicitly as unknown-unknown territory -2. **Interview the user** with AskUserQuestion, one question at a time, prioritized by blast radius: architecture-changing answers first, then public API / config surface, then UX. Rules: - - Skip anything the codebase, CLAUDE.md, or an existing issue already answers. - - 2–5 questions is the sweet spot; zero is fine when the request is genuinely unambiguous — say so rather than inventing questions. - - Every question offers concrete options with a recommended default, never an open-ended essay prompt. -3. **Record the answers** in the plan's Decision section as `Settled in interview:` bullets — constraints the executor must not re-litigate. - -## Phase 3 — Design - -- Develop 2–3 candidate approaches with real tradeoffs. Pick one and say why; record why the others lost. -- The chosen design must respect the project invariants (see CLAUDE.md "Critical Rules"): - - **Signed identity, never state** — the DOM carries `{c, gid}` or `{c, state}`, never raw state; re-find the record server-side. - - **Default-deny actions** — only methods declared `action :name` run; mutating actions `authorize!` inside the action. - - **Declared, coerced params** — any action taking input declares a `params:` schema; no raw mass assignment. - - **The component self-targets via `#id`** — no hand-picked Turbo Stream targets; `#id` is render-context-free (`Streamable#dom_id`, never the Phlex render-time helper). - - **pgbus is optional** — capability-gate every pgbus-only feature (`Phlex::Reactive.pgbus_streams?`) and fall back to `Turbo::StreamsChannel`. Must work on Action Cable AND pgbus. - - **Re-render through a real view context** — `Phlex::Reactive.renderer` / the controller, never a fabricated context. -- Decide the test strategy per `.claude/rules/testing.md`: unit (`spec/phlex`), request (`spec/requests`), broadcast (`spec/requests/*_broadcast_spec.rb`), system (`spec/system`, under Puma AND Falcon). Specs are named before the implementation steps they cover (TDD). -- If the change touches a hot path (render, token signing, param coercion, broadcast, client dispatch), the plan must include a `rake bench` baseline-and-after step per `.claude/rules/performance.md`. - -## Phase 4 — Emit the plan artifact - -Use this structure for the issue body or markdown file. Every section is load-bearing — an executor uses Context to avoid re-discovery, Steps to act, Gates to verify, Boundaries to stop. - -```markdown -# - -## Problem / Goal -<What's wrong or missing, who it affects, what done looks like.> - -## Context (read these first) -<Bullet list: `path/to/file.rb` — why it matters to this change. Include the mixin, the endpoint, the client controller, the relevant specs and dummy components. Self-contained: no references to "as discussed" or this session.> - -## Decision -<Chosen approach and rationale. Then: alternatives considered and why each was rejected. Call out the pgbus-present vs pgbus-absent behavior explicitly. End with `Settled in interview:` bullets for every constraint the user confirmed in Phase 2 — the executor must not re-litigate these.> - -## Implementation steps -<Ordered, small, each mapped to a specialist where useful (/tdd, /architect, /perf). Specs come before the code they cover. Name exact files to create or change.> - -## Verification gates -<Exact commands + expected outcome:> -- `bundle exec rspec spec/phlex spec/requests` — all green -- `bundle exec rspec spec/system` — green (client-touching changes; also `CAPYBARA_SERVER=falcon` / `rake spec:system_servers` for both real servers) -- `bundle exec rubocop` — no offenses -- `bundle exec rake bench` — before/after captured (only if a hot path was touched) - -## Out of scope -<Explicit boundaries — the adjacent things an eager executor must NOT do. E.g. "do not add a hard pgbus dependency", "do not break backwards compatibility for existing components".> - -## Execution -Execute with `/lfg <issue-number>` (or `/lfg docs/plans/<file>.md`). -``` - -For GitHub issues: create with `gh issue create --title "..." --body "$(cat <<'EOF' ... EOF)"` — single-quoted heredoc delimiter, backticks unescaped (see `.claude/rules/git-workflow.md`). Apply the `plan` label if it exists (`gh label list`); don't create labels. - -For markdown files: Write to `docs/plans/YYYY-MM-DD-<slug>.md`. Leave it uncommitted — committing is the user's call. - -## Phase 5 — Handoff - -Report back: link to the issue (or file path), the chosen approach in 2–3 sentences, and the exact execute command. Stop there — do not start implementing. diff --git a/.claude/commands/tdd.md b/.claude/commands/tdd.md deleted file mode 100644 index a955021f..00000000 --- a/.claude/commands/tdd.md +++ /dev/null @@ -1,118 +0,0 @@ ---- -model: opus -description: "Use when implementing any feature or fixing any bug — enforces RED-GREEN-REFACTOR: write failing test first, implement minimum code to pass, then refactor." ---- - -# TDD Command - -Enforce test-driven development with RED → GREEN → REFACTOR. - -## The TDD Cycle - -```text -RED: Write a failing test (it MUST fail first) -GREEN: Write MINIMAL code to pass (nothing more) -REFACTOR: Improve code while keeping tests green -REPEAT: Next scenario -``` - -## When to Use - -- Implementing a new reactive feature (a new `action`, a Streamable method) -- Adopting a pgbus primitive (exclude:, typed events, coalescing, presence) -- Fixing a bug (write the reproducing test FIRST) -- Changing the client runtime, the endpoint, or the mixins - -## Workflow - -### Step 1: Write Failing Tests (RED) - -Pick the cheapest layer that proves the behavior: - -```ruby -# Unit (no Rails): the DSL / identity / capability gate -RSpec.describe Phlex::Reactive::Component do - it "registers a declared action" do - expect(klass.reactive_action?(:increment)).to be(true) - end -end - -# Request: the endpoint -RSpec.describe "Reactive actions", type: :request do - it "forbids an undeclared action" do - post_action(CounterComponent, payload: {...}, act: "drop_table") - expect(response).to have_http_status(:forbidden) - end -end - -# Broadcast: the server→client half -it "broadcasts an append to the room stream" do - expect { send_message(...) }.to broadcast_an_append_to(stream) -end - -# System: the real browser loop -it "increments without a full page reload" do - visit "/counter"; find("[data-testid=inc]").click - expect(page).to have_css("[data-testid=count]", text: "1") -end -``` - -### Step 2: Run — Verify FAIL - -```bash -bundle exec rspec <spec_file> -# FAIL — confirms the test runs, tests the right thing, and the code doesn't already exist -``` - -### Step 3: Implement Minimal Code (GREEN) - -### Step 4: Run — Verify PASS - -```bash -bundle exec rspec <spec_file> -# N examples, 0 failures -``` - -### Step 5: Refactor - -Improve while staying green: extract methods, improve names, reduce duplication. - -### Step 6: Run Full Suite + Lint - -```bash -bundle exec rspec spec/phlex spec/requests -bundle exec rubocop -``` - -## Coverage Expectations - -| Code | Minimum | -|------|---------| -| All code | 80% | -| identity sign/verify (tamper, wrong key, wrong purpose) | 100% | -| the action endpoint (default-deny, schema coercion, 403/404/400) | 100% | -| the capability gate (pgbus absent / too old / present) | 100% | - -## pgbus features: test BOTH paths - -Any feature using a pgbus primitive MUST have specs for: -- **pgbus present** — the primitive is called with the right args -- **pgbus absent / too old** — the fallback path runs, no `ArgumentError` leaks - (use a "0.9.1-shaped" `broadcast` double without `:exclude` as the gate's regression guard) - -## Best Practices - -**DO:** test FIRST; verify RED; minimal GREEN; refactor green; drive browser specs -by `data-testid` with waiting matchers; mock the verifier in unit specs. - -**DON'T:** implement before testing; assert a snapshot value right after a click -(race the morph); test implementation details; skip the pgbus-absent path. - -## Checklist - -- [ ] Tests written BEFORE implementation; RED verified -- [ ] Minimal GREEN; refactored green -- [ ] Coverage meets the bar (100% on security + the gate) -- [ ] Edge + error paths covered -- [ ] pgbus-present AND pgbus-absent both tested -- [ ] `bundle exec rubocop` passes diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 00000000..90b8653d --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,13 @@ +{ + "extraKnownMarketplaces": { + "zoolutions": { + "source": { + "source": "github", + "repo": "zoolutions/claude-plugins" + } + } + }, + "enabledPlugins": { + "lode@zoolutions": true + } +} diff --git a/.gitignore b/.gitignore index 94249fa7..dc772e77 100644 --- a/.gitignore +++ b/.gitignore @@ -17,3 +17,6 @@ spec/dummy/db/*.sqlite3 .rspec_status docs/plans/2026-07-06-api-audit-internal-evidence.md + +# lode scratch (gate reports, handovers) — never memory +/lode/tmp/ diff --git a/CLAUDE.md b/CLAUDE.md index 89420908..46db16d7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -4,6 +4,13 @@ Reactive [Phlex](https://www.phlex.fun) components for Rails — Livewire-style actions and live cross-tab updates, without writing Stimulus controllers or hand-picking Turbo Stream targets. +## Memory + +Durable project memory lives in `lode/` (index: `lode/lode-map.md`). Read it before +exploring the code. `lode/review/` holds accepted review findings as rules about the +system; `/lode:gate` enforces them before any push, and `/lode:learn` adds to them. +`lode/workflow.md` is the profile the shared `/lode:*` workflow skills read. + ## Tech Stack - **Ruby**: >= 3.4 | **Rails**: >= 7.1 @@ -73,27 +80,34 @@ re-sync command. | Command | Purpose | |---------|---------| -| `/plan` | Fable-powered planning → GitHub issue or `docs/plans/` markdown (read-only; execute with `/lfg`) | -| `/lfg` | Full autonomous workflow: branch → understand → explore → plan → TDD → verify → PR | -| `/tdd` | Enforce RED → GREEN → REFACTOR | +| `/lode:plan` | Read-only planning → GitHub issue or `docs/plans/` markdown (execute with `/lode:lfg`) | +| `/lode:lfg` | Full autonomous workflow: branch → understand → explore → plan → TDD → verify → PR | +| `/lode:tdd` | Enforce RED → GREEN → REFACTOR | +| `/lode:review-pr` | Full PR pass: merge conflicts, then CI failures, then review comments (in that order) | +| `/lode:finish-prs` | Drive a stack of open PRs to merge-ready, one at a time | +| `/lode:debug-flaky` | Root-cause an intermittent test — evidence → repro → stress-proofed fix | +| `/lode:gate` | Pre-PR gate: fresh-context review against the rules and `lode/review/`, loops until clean | +| `/lode:learn` | Write accepted review findings into `lode/review/` | +| `/lode:sync` | Keep `lode/` true to the code after a change; `audit`, `handover` | | `/perf` | Benchmark the branch vs main (same-machine before/after) and keep perf docs in sync | | `/architect` | Coordinate a change across the component → endpoint → client layers | | `/security` | Security audit (signed identity, default-deny, params, CSRF, connection-id) | -| `/review-pr` | Review a PR for pattern compliance | -| `/github-review-pr` | Full PR pass: fix CI failures, then resolve review comments (in that order) | -| `/github-review-failures` | Fix failing CI checks until green | -| `/github-review-comments` | Process unresolved PR review comments | - -Commands pin a model tier via frontmatter aliases: `haiku` for mechanical/config -work, `sonnet` for the prescriptive pattern-following passes (`/github-review-comments`, -`/github-review-failures`), `opus` for orchestration, security, review synthesis, -and the reasoning-heavy specialists (`/lfg`, `/architect`, `/security`, `/review-pr`, -`/github-review-pr`, `/tdd`, `/perf`). Fable is pinned only on `/plan` — read-only -planning that hands execution to cheaper models; otherwise choose it per-session -with `/model` for architecture and the hardest debugging. Use the tier alias, -never a full model ID, so commands track the latest model in each tier. When -spawning subagents for mechanical work (file finding, pattern scans), pass a -cheaper model explicitly rather than letting them inherit the session model. +| `/review-pr` | Review a PR for pattern compliance (this repo's own reviewer; `/lode:review-pr` is the full pass) | + +The `/lode:` commands come from the `lode@zoolutions` plugin (enabled in +`.claude/settings.json`); they read `lode/workflow.md` for everything specific to +this repository, which is why the local copies of `/lfg`, `/tdd`, `/plan` and the +three `/github-review-*` commands were retired. The four commands that remain local +— `/architect`, `/perf`, `/security`, `/review-pr` — have no plugin equivalent. + +Local commands pin a model tier via frontmatter aliases: `haiku` for +mechanical/config work, `sonnet` for prescriptive pattern-following passes, `opus` +for orchestration, security, review synthesis and the reasoning-heavy specialists +(`/architect`, `/security`, `/review-pr`, `/perf`). Choose Fable per-session with +`/model` for architecture and the hardest debugging. Use the tier alias, never a +full model ID, so commands track the latest model in each tier. When spawning +subagents for mechanical work (file finding, pattern scans), pass a cheaper model +explicitly rather than letting them inherit the session model. ## Architecture @@ -188,4 +202,5 @@ token signing (`reactive_token`), and param coercion. Key facts: See `.claude/` and `docs/`: - `.claude/commands/` — slash command definitions - `.claude/rules/` — coding style, git workflow, testing, performance, agents +- `lode/` — durable project memory (start at `lode/lode-map.md`) - `docs/` — published site (architecture, security, broadcasting, transport-pgbus, testing, performance, examples) diff --git a/lode/async-and-defer/summary.md b/lode/async-and-defer/summary.md new file mode 100644 index 00000000..c132b0d1 --- /dev/null +++ b/lode/async-and-defer/summary.md @@ -0,0 +1,64 @@ +# Async: deferred renders and the settle lifecycle + +`lib/phlex/reactive/defer.rb`, `deferred_render_job.rb`, `pending.rb`, `settle.rb`, `settles.rb`, plus `component/lazy.rb` and the endpoint's `#deferred`. + +Two features, one transport story. **Defer** takes a RENDER off the actor's critical path. **Settle** takes the OUTCOME of work the action merely enqueued and delivers it when the job knows it. + +## Why a settle exists + +The endpoint runs an action inside a transaction and renders the reply THERE, while a queue adapter publishes on COMMIT. So any `reply.morph` after an enqueue renders from a database the job has not touched yet, and is guaranteed to draw the pre-job world — rows still present, buttons still live — next to the "Queued 177 transfers" flash the same reply emitted. `reply.pending` replies truthfully instead. + +## Delivery lanes + +| Lane | How | Availability | +|---|---|---| +| **pull** (`:fetch`) | the directive carries a purpose-scoped, short-TTL defer token; the client POSTs it to `/reactive/defer` off the action queue | always — it is just HTTP | +| **push** (`:stream`) | a pgbus durable one-shot stream plus `DeferredRenderJob`; the reply carries a signed SSE src and `since-id="0"` | `defer_push_capable?` | + +`Defer.resolve_via` is evaluated per reply: `:fetch` forces pull; `:stream` requests push but degrades to pull with a once-per-process warning when the capability is absent; `:auto` picks push iff capable. `since-id="0"` on a FRESH key is what closes the broadcast-before-subscribe race — pgbus's connect-time read_after replays the job's durable broadcast even when the job beat the subscription, which is also why `durable: true` is load-bearing and why the one-shot queue is left to pgbus's orphan sweep rather than dropped eagerly. + +**A settle has no pull lane at all.** The client cannot poll "is the job done yet", and redeeming such a token at the defer endpoint would render the PRE-JOB component — the exact bug `reply.pending` exists to fix. So `settle_capable?` requires the push lane, and the pending directive deliberately carries NO `data-reactive-defer-token` (that attribute is the client's degrade-to-fetch path). + +## `reply.defer` + +`Response#defer` records a `Defer::Segment(component, placeholder, morph)`; the ENDPOINT turns it into wire streams after the transaction committed. `validate_segment!` fails loudly at the call site for a non-reactive component (its identity could never be rebuilt) or a bogus placeholder type, and `Response#defer` refuses a redirect reply outright (the client is navigating away). + +`streams_for` emits the optional placeholder shell FIRST — so the pending state paints before delivery starts — then the directive. The shell is a `<div>` that OWNS the component's id (it is the stream target) carrying `data-reactive-defer-pending`, `aria-busy` and `.reactive-defer-placeholder`, and deliberately NO defer token: the directive owns delivery, and a token on the shell would double-fetch through the lazy-mount connect probe. `placeholder:` is nil (keep current content), `true` (the component's `deferred_placeholder`, else an empty shell), a String, or a Phlex component — resolved through the same escape contract as flash and `also`. + +Bare `reply.defer(component)` builds on `Response.build_streams(@component)`, i.e. a token-ONLY refresh rather than a full self-replace: a full replace would render the acting component synchronously, and `reply.defer(self)` would then be silently defeated by a double render. + +`one_shot_stream_key` sizes its random hex suffix to pgbus's LIVE queue-name budget (preferring pgbus's own `Key.queue_name_budget`, then `47 − prefix − 1`, then the documented default), because a fixed-width key would raise `StreamNameTooLong` in the JOB — after the directive already shipped, leaving the shimmer hanging forever. The suffix is hex and never contains hyphens: pgbus's sanitizer strips them, which would collide two keys differing only by a hyphen. When the budget cannot fit `DEFER_KEY_MARKER` plus the 16-char collision-safe minimum it RAISES, and `push_directive_attrs`' rescue degrades the segment to `:fetch`. + +`push_directive_attrs` runs AFTER the action committed, so a signing or enqueue failure must not 500 a reply whose mutation already persisted — it warns and falls back to the fetch directive. It also ships a fallback defer token ALONGSIDE the stream src: the server picks push on SERVER-side capability alone, but the browser may not have the pgbus client loaded. + +## `DeferredRenderJob` + +Rebuilds the component from its identity payload off the request thread, renders, and broadcasts durably to the one-shot key. Every payload ends with a remove of the client's `<pgbus-stream-source>` by its deterministic id (`reactive-defer-src-<target>`), whose `disconnectedCallback` closes the SSE — so the subscription tears itself down with the content it delivered, and there is no client-side arrival bookkeeping. + +A non-reactive class raises OUTSIDE the render rescue (a bad enqueue is a programming error and must broadcast nothing). Everything else — a record deleted while queued, `render? == false`, a render that raises — broadcasts the CLEANUP instead (pending-clear ops + teardown): the stream lane has no client-side timeout, so a silent job death would hang the shimmer forever. If the cleanup broadcast itself succeeds the original error is swallowed (a retry would raise identically) and logged unless it was a `RecordNotFound`; if the cleanup broadcast fails, that propagates so the retry policy gets a chance. + +## `reply.pending` → `Pending` + +`build_segment` does three things in order: materialize `records` ONCE, mint one handle, run the caller's enqueue with the handle installed in a thread-local, and record a `Segment`. + +**One key per call is a hard design rule.** A durable pgbus broadcast to a never-seen key creates a real PGMQ queue reclaimed only by an hourly orphan sweep at a 24h threshold — a key per record would leave 177 tables for a day and open 177 SSE connections. So all N settles of one call share ONE key and ONE subscription, anchored on the container's id. Because the key is shared, teardown must be explicit (`reactive_settle(finish:)`), not per-arrival. + +`Handle` carries `stream_key`, `container_class`, `container_payload`, `anchor`, `collection`, `target_ids`, `peers`, `connection_id`, and round-trips through plain JSON (`to_h_wire` / `from_wire`) because it rides ActiveJob metadata. `resolve_targets` maps records through the collection declaration when `in:` is given; without it every entry must already be a Streamable component. `resolve_peers` turns `peers: true` into the container's own record (GlobalID-serialized so it survives the trip into the job) and refuses a state-backed container, which has no record stream. + +`run_enqueue`: an explicit block wins, else the `job:`/`args:` sugar. The sugar enqueues ONE job PER RECORD with the handle NARROWED to that record's target id — which is what makes a failure attributable. The block form cannot be narrowed (the gem cannot map an arbitrary `perform_later` back to a record), so it carries the whole target list. An Array `args:` for more than one record raises as ambiguous. + +`streams_for` emits, in apply order: one `reactive:js` pending marker per target (setting `data-reactive-pending` and `aria-busy` through the existing op lane, so the client needed no change), the same marker on the anchor, then the single subscription directive. + +Without the push lane `build_segment` warns once, still runs the enqueue, and returns nil — no handle (so `reactive_settle` no-ops exactly like a sweep-enqueued job) and no pending markers. Today's behaviour, never a shimmer that could never resolve. + +## `Settles` and `Settle` — the job side + +`include Phlex::Reactive::Settles` in the job. `#serialize` copies the ambient handle into the job's metadata and `#deserialize` restores it, so **`perform`'s arity is untouched** and every OTHER caller of the same job — a nightly sweep, a webhook — enqueues it with no handle, in which case `reactive_settle` is a no-op returning nil. (`perform_now` skips serialize/deserialize and so carries no handle either, which is the correct reading: a synchronous call has no pending UI waiting on it.) + +`Settle` is a MUTABLE accumulator (a settle block is imperative), not a value object; every verb returns self. `replace`, `remove`, `append`, `prepend`, `move` (ordered remove-then-append so a row is never momentarily in both containers), `count`, `flash`, `js`, `streams!`. `from:`/`to:` default to the collection `reply.pending` named; a settle with neither raises. Every collection verb routes through the SAME `Collections` decisions the reply path uses — that shared bookkeeping is the whole point. + +`deliver_settle` joins every stream into ONE durable message (the row, the count and the empty-state belong to the same instant, which is also why the ACTOR path needs no coalescing), appends the finish streams when `finish_settle?`, and then delivers peers. `finish: :auto` finishes only when the call marked exactly one target; a fan-out passes `finish: true` from whatever knows it is last. + +If the block raises, the target's pending markers are still cleared (the shimmer must not lie) and the error is RE-RAISED so the retry policy sees it; the subscription is deliberately NOT torn down, because a retry must still reach the actor. + +Related: `../streaming/summary.md`, `../review/async-actions.md` (the attribution, peer and key rules, each with its proving spec). diff --git a/lode/client-runtime/summary.md b/lode/client-runtime/summary.md new file mode 100644 index 00000000..9d2b3cb7 --- /dev/null +++ b/lode/client-runtime/summary.md @@ -0,0 +1,54 @@ +# The client runtime + +`app/javascript/phlex/reactive/*.js` — five authored modules, 5787 lines of comment-dense source, minified into the twins the gem actually ships. + +| Module | Role | Pinned | +|---|---|---| +| `reactive_controller.js` | the ONE generic Stimulus controller plus the custom turbo-stream actions | `preload: true` | +| `confirm.js` | the overridable confirm resolver (`setConfirmResolver`) | `preload: true` | +| `confirm_predicate.js` | the conditional-confirm predicate registry (`setConfirmPredicate`) | `preload: true` | +| `compute.js` | the client-side reducer registry (`setComputeReducer`) | `preload: true` | +| `inspect.js` | the on-demand console inspector | `preload: false` — a debugging tool nobody should pay for | + +## The build + +`rake build:js` runs `scripts/build_client.js`, which minifies each entry ON ITS OWN with `sourcemap: "linked"` and the four cross-module specifiers kept EXTERNAL. Per-file, never a bundle: the modules are pinned separately so an app can override the `confirm`/`compute`/`confirm_predicate` seams, and bundling would inline them and break every override. The output is deterministic (bun derives the sourcemap debugId from content), which is what lets the `.min.js`/`.map` be checked in and `rake build:js_check` gate CI on `git diff --exit-code` — with the pathspec QUOTED so git expands it against the index, or deleting a module would slip past the guard. + +A source edit is therefore a THREE-file change: the source, the rebuilt `.min.js` + `.map`, and the re-synced `spec/dummy/public/vendor/<name>.js`. Two guards enforce it: `rake build:js_check` and `spec/phlex/vendored_controller_sync_spec.rb`, whose failure message prints the exact re-sync command. The vendored copies are byte-identical to the MINIFIED builds on purpose — production ships minified, so the browser suite must exercise minified code or a minifier-induced bug (a mangled name breaking a lifecycle hook, a dropped export) ships untested. + +## Imports are bare specifiers, never relative + +`reactive_controller.js` imports `"phlex/reactive/confirm"`, not `"./confirm.js"`. Under importmap-rails + Propshaft the controller is served at its DIGESTED url; Propshaft rewrites only `RAILS_ASSET_URL(...)` and the import map resolves only BARE specifiers, so a relative sibling import would resolve to an undigested `/assets/…/confirm.js` that 404s — and a throwing import takes down every Stimulus controller on the page. The engine's `importmap.pin` calls are what make the bare specifier resolve; `tsconfig.json` `paths` make bun resolve it the same way for the tests. + +## The wire + +`POST <action path>` with a turbo-stream `Accept`, body `{ token, act, params }` as JSON — or the SAME payload as multipart `FormData` (token/act flat, params bracketed, files appended) when the root holds a chosen `<input type="file">`. Only the encoding differs. `act`, not `action`: `action` is a reserved Rails routing param. The response is a `<turbo-stream>` Turbo applies by the component's id. + +## Custom turbo-stream actions + +Registered once on `window.Turbo.StreamActions` (never an `@hotwired/turbo` named import — unreliable under importmap/esbuild), each registration idempotent: + +| Action | What it does | +|---|---| +| `reactive:visit` | `Turbo.visit(url)` — a 200-carried redirect, so `response.redirected` still catches real auth/CSRF redirects | +| `reactive:token` | writes the fresh token onto the root as a pure attribute set — a focused input and its caret survive | +| `reactive:js` | runs the server's op chain through the same allowlist interpreter `on_client` uses (unknown op → warn and skip) | +| `reactive:defer` | the delivery directive: `via="fetch"` POSTs the token, `via="stream"` mounts a `<pgbus-stream-source>`, with supersession per target id | + +Plus the document-level wrappers `registerReactiveDismiss` (any `[data-reactive-dismiss-after]` self-removes), `registerReactiveEffects` (wraps element stream renders in the enter/exit/update choreography), `registerReactiveOffline` and `registerReactiveActions`. + +## Controller invariants + +- **The root's `id` must equal `component.id`.** The client self-matches its next token by `this.element.id`; an empty id makes `#extractToken` fall back to the FIRST token in the response — a child's — and the next action POSTs a foreign token to a silent 403. `connect()` warns on an empty id so the failure surfaces on page load, not on click 2. +- **The trigger is `event.currentTarget`, not `event.target`.** A `<button><span>Save</span></button>` click has `target === span`, which carries no params. +- **`preventDefault()` runs synchronously in the event handler**, before any async confirm resolver — once awaited it is too late and a `submit` trigger natively POSTs and navigates. Window-bound triggers are exempt (they hear every matching event on the page), and so is a `checked: :keep` optimistic hint, which exists precisely to let the native flip happen. +- **Behaviour is decided from `event.params`**, never by sniffing the Stimulus descriptor. +- **Feature gates are decided once at `connect()`**, and each gate wires its `turbo:morph-element` listener only when the component actually uses the feature — a component that never uses `reactive_lazy`, dirty tracking, show bindings, filters, tags, nested JSON rows, compute seeding or the clipboard gate pays for no listener at all. Every bound handler is held on a private field so `disconnect()` removes exactly it. +- **State that must survive re-renders belongs in a signed action.** Client ops are ephemeral UI: any server re-render rebuilds from server state and resets what they toggled. +- **A gate that HIDES a dead control must consider the root itself**, not only descendants — a button-only component mixes `on_client` onto `reactive_root`, so the root can be the trigger (see `../review/client-runtime.md`). + +## The overridable seams + +`setConfirmResolver` (async confirm UI), `setConfirmPredicate` (multi-field conditional confirm), `setComputeReducer` (named client-side reducers behind `reactive_compute`). Each lives in its own pinned module so an app's `import { setX } from "phlex/reactive/…"` and the controller's own import resolve to the same instance. + +Related: `../component/summary.md` (the Ruby half of every binding), `../testing-and-ci/summary.md` (the bun suite and the browser matrix). diff --git a/lode/component/summary.md b/lode/component/summary.md new file mode 100644 index 00000000..e8b8774c --- /dev/null +++ b/lode/component/summary.md @@ -0,0 +1,81 @@ +# Component: the declaration DSL and the view helpers + +`lib/phlex/reactive/component.rb` and `component/{dsl,helpers,identity,lazy,registry}.rb`, plus `client_bindings.rb`, `param_schema.rb`, `show_conditions.rb`, `js.rb` and `effects.rb`. + +## The include stack + +`include Phlex::Reactive::Component` pulls in, in order: `Streamable` (render/broadcast/`#id`, mixed first so its methods sit below the component's own), `ClientBindings` (= `Component::DSL` + `Component::Helpers`), `Identity`, `Lazy`. `ClientBindings` is the ONE implementation of the client-only surface and is includable on its own — a token-bearing component is a SUPERSET of a client-only one, not a fork. The three server-action macros (`action`, `reactive_record`, `reactive_state`) call `require_server_actions!`, which keys on `Identity` being in the ancestry, so they raise at class-definition time on a `ClientBindings`-only class rather than silently signing nothing. + +Four `Data.define` declaration shapes live on `Component` itself: `ActionDefinition`, `ComputeDefinition`, `OnCompleteDefinition`, `CollectionDefinition` (with `#size_for`, which `instance_exec`s the declared `size:` proc against the bound container). `ActionDefinition` is deliberately not named `Action`: this module sits in every reactive component's ancestry, so a bare `Action` constant shadows a host app's Phlex kit component of the same name under lazy autoloading. **Keep every constant here suffixed or implausible as a kit component name.** + +## Registry: one inheritance semantic + +`Component::Registry` gives all twelve class-level registries (`actions`, `state_keys`, `collections`, `computes`, `on_completes`, `record_key`, `scope`, `dirty`, `lazy`, `effects`, `skip_all`, `skip_actions`) the same semantic: **resolve through the superclass at read time**, memoized per class against a process-wide generation counter bumped on any write. Hash-shaped registries merge ancestors-first with the nearest declaration winning, and the resolved hash is FROZEN (it is the default-deny dispatch table); list-shaped ones concatenate ancestors-then-own; scalar-shaped ones take the class's own declaration if present, else the nearest ancestor's. + +The hot-path contract is the part to preserve: the generation check gates registry RESOLUTION only. `Identity`'s `@reactive_record_ivar` and `@reactive_state_ivars`, read on every `reactive_token`, stay bare `defined?`/`||=` with no per-read compare, and are invalidated at WRITE time — `bump!` removes them from the writing class and recurses through `klass.subclasses`. Storage is all on the component class itself, so a Zeitwerk reload's fresh class object starts clean and nothing global retains a reference to app classes. `WRITE_MUTEX` serializes the generation increment; `resolution_cache` writes the new (empty) cache BEFORE the new generation so a concurrent reader never finds the stale one. + +## Identity + +`reactive_identity_payload` builds `{"c" => class name}` and adds: + +- `"gid"` — only for a present, PERSISTED record. `signable_gid?` treats anything not responding to `persisted?` as signable; an unsaved AR draft has no id and `to_gid` would raise `MissingModelIdError`, so the gid is omitted and the declared `reactive_state` is the draft seed. +- `"s"` — `{ key => ivar.as_json }` for every `reactive_state` key, walked through the precomputed `[string_key, ivar_symbol]` pairs so no String/Symbol is allocated per render. + +`from_identity` (on `DSL`) rebuilds from a verified payload: locate the record by GlobalID (raising `ActiveRecord::RecordNotFound` when it is gone), or, for a draft token with no gid, omit the kwarg entirely — with `ensure_draft_default!` raising a guided `Phlex::Reactive::Error` when `initialize` requires the record keyword, instead of a bare missing-keyword `ArgumentError` deep inside `new(**kwargs)`. State keys are restored by KEY PRESENCE, not truthiness, so a signed `nil` or `false` round-trips distinctly from an absent key. + +## Lazy initial mount + +`Lazy` overrides Phlex 2's `around_template` (not yield-then-decorate — the shell REPLACES the template). A `reactive_lazy` component's page-embedded first render emits a shell owning the component's `id` with `data-reactive-defer-token` on the root; the client's `connect()` probes it and enters the same fetch path a `reply.defer` directive uses. **Lazy applies only to the initial mount:** every render that goes through the reactive machinery runs inside `Defer.with_real_render` (`Streamable.render_component` and `Phlex::Reactive.render` both set it), so an action reply, a broadcast, the defer endpoint and the class stream builders all render the REAL template. The shell's token is minted `unbound: true` — the page render happens before a session exists. + +## The declaration macros (`Component::DSL`) + +`reactive_record`, `reactive_state`, `reactive_scope`, `reactive_dirty`, `reactive_effects`, `reactive_lazy`, `action`, `skip_verify_authorized`, `reactive_collection`, `reactive_compute`, `reactive_on_complete`. + +- `action(name, params: {})` compiles the schema ONCE at declaration, so a typo'd type symbol raises `UnknownParamType` at class load. `params:` also accepts a Symbol naming a registered `Phlex::Reactive.param_schema`. +- `reactive_scope` and `action` cross-check each other (`assert_no_scope_double_nesting!`) in both declaration orders: a schema already nested under the scope key would be double-peeled at the endpoint. +- `reactive_compute`'s `inputs:` takes three shapes, all degenerate cases of the permit form: a pure Hash (typed), an Array with a trailing type Hash (bare symbols default to `:number`), or a bare Array (untyped — nil types, so the wire stays byte-identical and the client keeps numeric coercion). `mirror:` targets are validated at declare time against `MIRROR_ID_SELECTOR` — id selectors only, never arbitrary selectors. +- `reactive_on_complete` compiles its conditions through `ShowConditions.normalize` and its `run:` chain through the same allowlist a raw ops list gets. `reactive_on_complete_attr` memoizes the JSON wire per class against `Registry.generation`, because `reactive_attrs` is the token-signing hot path — same trick as `reactive_effect_attrs`, which memoizes against both `Registry.generation` and `Phlex::Reactive.effects_generation`. + +## The view helpers (`Component::Helpers`) + +`reactive_attrs` builds the root's `data:` and is where every opt-in surfaces: `controller: "reactive"` always; `reactive_token_value` only when `respond_to?(:reactive_token, true)` (the include-private check — `reactive_token` is private); then `reactive_debug`, `reactive_verbose`, `reactive_scope`, the resolved effect attrs and the on-complete JSON, each omitted entirely when off so the wire stays byte-stable. **Boolean-true attributes are written as the STRING `"true"`**: Phlex renders a `true` attribute valueless, which `getAttribute` reads as `""` — falsy in JS — so the client guard would never fire. + +`reactive_root(**overrides)` is the whole root in one spread: it binds `id:` to the SAME element as `reactive_attrs`, because `id:` on a child leaves the controller root's `id` empty and the client self-matches its next token by `this.element.id`. Overrides go through Phlex's `mix` (deep merge) so a caller's `class:`/`data:` never clobbers the controller/token data; `id` is resolved separately as a clean replace, since `mix` would string-concat two ids. + +`on(action_name, …)` emits the dispatch descriptor: `event[@window]->reactive#dispatch[:once]`, the action name, the params JSON (`"{}"` when empty), and optional `debounce`/`throttle` (mutually exclusive — declaring both raises), `confirm`, `optimistic`, `busy`, `outside`, `window`. It forces `type="button"` for a click trigger, EXCEPT when `optimistic` declares `checked: :keep` — that hint exists to let a click-bound checkbox flip natively. `on_client(event, ops)` is the zero-round-trip sibling: a non-empty `JS` chain only, no token, no POST. + +The rest of the helper surface is field/binding compilation: `reactive_field`/`reactive_input`/`reactive_select`/`reactive_text`, `reactive_show`/`reactive_show_targets`/`reactive_filter`, `reactive_listnav`, `reactive_tags*`, the nested-attributes family (`reactive_nested_list`/`_template`/`_row`/`_add`/`_remove`, `nested_field_name`, `nested_attributes`, `nested_update!`), `reactive_persist`, `busy_on`, `reactive_compute_attrs`. Each validating helper raises at render time on a bad selector, identifier or scope rather than emitting a binding that silently matches nothing in the browser. + +## ParamSchema + +Compiled once per action. A type is a scalar Symbol, a Hash schema (nested object), or a one-element Array (array of that). Eight built-in types ship: `string`, `integer`, `float`, `boolean`, `file`, `date`, `datetime`, `decimal` — `file` and the composites are handled structurally in `#coerce`, so their registry entry exists only for compile-time validation. + +The coercion contract is **drop, don't fabricate**. `DROP` is a public sentinel a custom `param_type` returns to reject a value; a dropped key is simply not assigned, so the method's keyword default applies exactly as if the client had omitted it. + +| Input | Result | +|---|---| +| an undeclared key | dropped (no mass assignment) | +| `"abc"` for `:integer` | `0` — `to_i`'s own semantics, kept verbatim from the pre-extraction controller | +| an unparseable `:date`/`:datetime`/`:decimal` | `DROP` (the parse is rescued to it) | +| a non-uploaded value for `:file` | `DROP` — a file is duck-typed on `original_filename` + `read`, never a class name | +| a scalar where a Hash schema is declared | `DROP`, never a fabricated `{}` | +| a scalar where an Array is declared | `DROP`, never a fabricated `[]` | +| a Rails index hash `{"0" => …}` for an Array | coerced in index order | +| an array whose every element drops | `DROP`; a genuinely empty input array stays `[]` | +| a malformed TOP-LEVEL container | normalized to `{}` — the top level holds the kwargs, so a bad container means "no params" | + +`to_param_hash` also expands bracket notation (`invoice[date]` → `{"invoice" => {"date" => …}}`, `items[0][qty]` → the index form), deep-merging so a bracket key and a pre-nested object for the same key coalesce whichever arrives first. Under `verbose_errors` a collector accumulates `[bracketed_path, :undeclared|:uncoercible]`; with the flag off the collector is nil and every diagnostic branch early-returns. + +## ShowConditions + +The ONE conditions language for `reactive_show`, `reactive_show_targets`, `reactive_filter` and `reactive_on_complete`. Ruby values compile to a DNF wire shape — an array of groups, terms AND within a group, groups OR — and `ShowConditions.match?` evaluates that same shape in Ruby so the server's first-paint `hidden:` and the client's live toggling cannot drift. `spec/fixtures/show_predicate_vectors.json` is the shared parity fixture both sides run. + +Value language: scalar → equals (stringified), `true`/`false` → `"true"`/`"false"`, `nil` → `""`, Array → membership, a Range → `gte`/`lte`/`lt` terms, `{ length: … }` → the `len_*` family counted in CODEPOINTS (so Ruby's `String#length` and JS's `[...value].length` agree on multibyte). `unless:` negates by De Morgan and a bounded range's complement splits the group. There is no expression surface: every term is a declared literal predicate. A referenced field absent from the values map reads as `""` (fail-closed). + +## JS ops and Effects + +`Phlex::Reactive::JS` is an immutable chain of 17 verbs (`show`/`hide`/`toggle`, the three class ops, the three attr ops, `focus`/`focus_first`, `submit`, `paste_into`, `text`, `dispatch`, `persist_state`/`persist_clear`). Targets resolve WITHIN the component's root by default (nested reactive roots excluded); `:root` is the root itself; `global: true` opts one op out. The attribute allowlist refuses `on*` (XSS), the six URL-bearing attributes and `style`, enforced at build time by the builder AND by `JS.assert_ops_allowed!` on every raw `[[op, args], …]` escape hatch — and again by the client interpreter. + +`Effects` owns the enter/exit/update vocabulary: five built-ins (`fade`, `slide`, `scale`, `highlight`, `shake`), `:random`, `false` to disable a hook, or custom `{ during:, from:, to: }` legs compiled to the `[during, from, to]` wire array. Resolution is global ⊕ component ⊕ per-call, most specific wins; `false` survives normalization so a component-level `update: false` can cancel a global hook. Validation happens at WRITE time. See `../review/client-runtime.md` for the blank-leg rule. + +Related: `../endpoint/summary.md` (what the endpoint does with these declarations), `../client-runtime/summary.md` (the other half of every binding). diff --git a/lode/core-and-config/summary.md b/lode/core-and-config/summary.md new file mode 100644 index 00000000..d3df2ba3 --- /dev/null +++ b/lode/core-and-config/summary.md @@ -0,0 +1,91 @@ +# Core and configuration + +`lib/phlex/reactive.rb` (the `Phlex::Reactive` singleton) plus `lib/phlex/reactive/engine.rb`. Everything the rest of the gem reads for configuration, identity signing, capability detection and per-request state lives here. + +## Boot + +`lib/phlex-reactive.rb` is the gem-name shim; `lib/phlex/reactive.rb` is the real entry. It `require_relative`s `reactive/version` up front (the file defines `VERSION`, a constant, not a `Version` class) and then sets up a Zeitwerk loader rooted at `lib/`. The loader keeps eight paths out of its normal handling — six `loader.ignore` calls and two `loader.do_not_eager_load`s — each commented in place: + +| Path | Why it is ignored / not eager-loaded | +|---|---| +| `lib/phlex-reactive.rb` | the plain-require gem-name shim | +| `lib/phlex/reactive/version.rb` | defines `VERSION`, not `Version` — `eager_load_all` would raise `Zeitwerk::NameError` | +| `lib/generators` | Rails' generator system owns discovery; the path/constant scheme is deliberately non-Zeitwerk | +| `test_helpers/matchers.rb`, `test_helpers/system.rb` | define `RSpec::Matchers` / need Capybara; `test_helpers.rb` requires each only when the dependency is present | +| `mcp/` | the subtree subclasses the OPTIONAL `mcp` gem's constants at class-definition time; `MCP.load!` requires it in dependency order | +| `engine.rb`, `deferred_render_job.rb` | `do_not_eager_load` — the engine is required only when `Rails::Engine` is defined; the job subclasses `ActiveJob::Base`, which is not a dependency | + +Inflections: `js` → `JS`, `dsl` → `DSL`, `mcp` → `MCP`, `apm` → `APM`. + +`Engine` (`isolate_namespace Phlex::Reactive`) then does five things: appends `POST action_path` → `actions#create` and `POST defer_path` → `actions#deferred`; adds `app/javascript` and `app/assets/stylesheets` to the asset paths and precompiles the five `*.min.js` + maps + `effects.css`; pins those five modules into an importmap app (only `inspect` with `preload: false` — it is a console-loaded debugging tool); resets the memoized view contexts on every `to_prepare`; and, in `after_initialize`, warns about a shadowed route, attaches the `LogSubscriber` when `log_events`, attaches the APM adapter when `apm` is set, and freezes both the param-type and named-schema registries. + +## Identity tokens + +`sign`/`verify` are the single choke points. `sign` merges `"v" => TOKEN_VERSION` (currently 1) and generates under `IDENTITY_PURPOSE`; `verify` verifies and then runs `upgrade_token` so an older payload is migrated before `from_identity` sees it. `upgrade_token`'s contract is the interesting part: + +- no `"v"` → version 0, the pre-versioning shape. With no upgrader registered this is a pure passthrough, so introducing versioning invalidated nothing in flight. +- `v == TOKEN_VERSION` → returned as-is (the hot path: one integer compare). +- `v > TOKEN_VERSION` → **nil**, so a rolled-back deploy fails closed through the endpoint's `|| raise(InvalidToken)` → 400 rather than guessing a newer shape. +- a non-Integer or negative `"v"` → nil for the same reason. + +`register_token_upgrader(from_version)` fills a sparse `from_version => callable` map; `upgrade_from` walks it and only re-stamps `"v"` when an upgrader actually reshaped the payload. + +Defer tokens are a second family: `sign_defer` / `verify_defer` under `DEFER_PURPOSE` with `defer_token_ttl` (120s). The purposes are disjoint BY SIGNATURE, so an action token posted to the defer endpoint fails and a defer token can never invoke an action. `defer_purpose` additionally folds the current actor's binding into the purpose string, so a leaked `reply.defer` token cannot be exchanged by another actor for a fresh, non-expiring action token. `defer_binding_for(request)` returns the id of an ALREADY-PERSISTED session (`session.respond_to?(:exists?) && session.exists?` — a bare `session.id` lazily generates an id that is never persisted, so two requests for the same read-only page would disagree) and degrades to nil on any store error. `sign_defer(unbound: true)` is the `reactive_lazy` case: a lazy shell renders during the page render, before a session exists. + +## Configuration surface + +Writers with lazy defaults. Six readers guard with `defined?(@x)` rather than `||=` — `verbose_errors`, `log_events`, `debug`, `verify_authorized`, `authorization_methods` and `effects` — because their defaults are truthy or their false value is meaningful, so an explicit `= false` (or a narrower list) has to stick. Every other reader is a plain `@x ||= default`, which is safe only because none of those defaults is `false` or `nil`: + +| Setting | Default | Notes | +|---|---|---| +| `verifier` | `Rails.application.message_verifier(IDENTITY_PURPOSE)` | raises a guided error outside Rails | +| `renderer` | `ActionController::Base` when it is defined, else nil | the controller a view context is built from | +| `base_controller_name` | `"ActionController::Base"` | String, resolved lazily by `base_controller` | +| `action_path` / `defer_path` | `/reactive/actions` / `/reactive/defer` | read before boot by the engine's route append | +| `authorization_errors` | `[]` | rendered as 403 by both endpoints | +| `verbose_errors` | `Rails.env.local?` | diagnostic bodies + dropped-param logging + render-time `on(:typo)` + the client's zero-target op warning | +| `verify_authorized` | `true` | default-ON; see `endpoint/summary.md` | +| `authorization_methods` | `%i[authorize! authorize allowed_to?]` | what the interceptor wraps | +| `debug` | `false` | stamps `data-reactive-debug` so the client console-groups each dispatch | +| `log_events` | `false` | the gem's own log lines; the events fire for APMs regardless | +| `apm` | `nil` | Symbol, custom adapter object, or nil | +| `effects` | `nil` (off) | normalized and validated at WRITE time; bumps `effects_generation` | +| `error_flash` | `nil` | `->(kind) { message }`; renders a flash on every endpoint rescue path | +| `flash_component` | `nil` | a CALLABLE `(level, content)`; a bare Class raises with the lambda rewrite | +| `flash_target` | `"flash"` | the container `Response#flash` appends into | +| `defer_transport` | `:auto` | validated at assignment against `DEFER_TRANSPORTS` | +| `defer_token_ttl` / `defer_job_queue` | `120` / `"default"` | | +| `settle_coalesce_window_ms` | `50` | applies to a settle's AGGREGATE peer streams only | + +There is deliberately **no `settle_token_ttl`** — see `../review/async-actions.md`. + +Registries frozen by the engine's `after_initialize`, so registration is initializer-only: `param_type(name) { }` (custom coercions, returning `ParamSchema::DROP` to reject) and `param_schema(name, hash)` (reusable named schemas, deep-frozen so a nested schema cannot be mutated through the memoized reader). `reset_param_types!` / `reset_param_schemas!` exist for tests. + +Hooks, each with a `reset_*!` for test isolation: `around_action` (folded by the endpoint so the LAST registered runs outermost) and `on_action_error`. + +## Capability gates + +``` +pgbus? defined?(::Pgbus) && ::Pgbus.respond_to?(:stream) # necessary, NOT sufficient +pgbus_streams? + Pgbus::Streams::Stream#broadcast takes :exclude # the >= 0.9.2 probe +defer_push_capable? + Pgbus::Streams::SignedName.respond_to?(:sign) + ActiveJob::Base +settle_capable? defer_push_capable? && defer_transport != :fetch +``` + +`pgbus_streams?` is the gate that prevents `ArgumentError: unknown keyword :exclude` on an old pgbus, and it probes the actual keyword because pgbus < 0.9.2 also defines `::Pgbus`. + +## Rendering off-request + +`request_bound_view_context(controller_class)` replicates what `ActionController::Renderer#render` does to build its mock request — an `ActionDispatch::Request` from the renderer's env, routes bound, `set_request!` + `set_response!` — then returns the controller's `view_context` instead of rendering a template. That is what makes `form_authenticity_token`, `protect_against_forgery?` and host-aware URL helpers work off-request. The instance's singleton `url_options` merges `Phlex::Reactive.current_url_options` over its memo, so a reply renders absolute URLs for the REQUESTING host while an off-request caller (job, console, broadcast) gets the frozen memo untouched. + +`off_request_view_context` and `stream_builder` come from one per-thread cache keyed on `renderer.equal?` plus `off_request_view_context_generation`; `reset_stream_builder!` bumps the generation for all threads. `flash_builder` / `reset_flash_builder!` are gone and raise a `NoMethodError` naming the replacement. + +`Phlex::Reactive.broadcast_to` is the module-level twin of the class-level form for a BUILT, possibly non-Streamable payload; both share `Streamable.broadcast_component`. + +## Route guard + +`action_route_ok?` force-loads the route set (it may run before the host's routes are drawn) and asks `recognize_path(path, method: :post)` whether it reaches `"phlex/reactive/actions"`. A host catch-all appended above the engine's route shadows it and every reactive POST 404s with nothing to see; `warn_unless_action_route_mounted!` turns that into one boot-time log line naming the catch-all. + +## Instrumentation + +`instrument(event, payload, &)` wraps `ActiveSupport::Notifications.instrument("#{event}.phlex_reactive", …)`, yielding the mutable payload so a rescue can finalize `:outcome`. Four events: `action`, `defer`, `render`, `broadcast`. **Payloads carry names, outcome and sizes only — never the token, params or state.** `report_error(error, context)` forwards a fresh `context.slice(*ERROR_CONTEXT_KEYS)` (`:component, :action, :outcome`) to the resolved APM adapter and every `on_action_error` hook, each wrapped in `safely_report` so a broken reporter can never replace the original 500. diff --git a/lode/docs-site/summary.md b/lode/docs-site/summary.md new file mode 100644 index 00000000..653497fc --- /dev/null +++ b/lode/docs-site/summary.md @@ -0,0 +1,61 @@ +# The docs site (`docs/`) + +A self-contained Rails app under `docs/` with its own bundle, RuboCop, RSpec and bun lockfile, deployed to https://phlex-reactive.zoolutions.llc. It is both the published documentation and the **demo app** — it depends on the gem by `path: ".."`, so every example page on the site is a live reactive component the site's own system suite drives in a browser. A showcase that cannot rot silently is the point. + +## Shape + +It is a [docs-kit](https://github.com/zoolutions/docs-kit) site: a Phlex/daisyUI chrome where every page is a `DocsUI::Page` subclass and the sidebar, table of contents, search and Markdown twin come free. The authoring contract is `docs/AGENTS.md`; the short version is that you never hand-write HTML or daisyUI markup, you compose `DocsUI::` helpers. + +Pages live in `docs/app/views/docs/pages/` (29 files) and are routed only through `Doc::REGISTRY` in `docs/app/models/doc.rb` (29 entries, one per page class). **A page with no registry line is neither routed nor in the nav**, so scaffold with the generator rather than adding a file by hand: + +```bash +cd docs && bin/rails g docs_kit:page "Title" --group=Guide +``` + +which writes the page class and injects its registry line in one idempotent step. + +## Which page documents what + +| Behaviour | Page | +|---|---| +| `action`, `on`, the dispatch descriptors, params | `actions_events.rb` | +| the mental model, the two mixins, the re-render unit | `architecture.rb` | +| signed identity, default-deny, the threat model | `security.rb` | +| `broadcast_to`, `each:`, `exclude:`, collections | `broadcasting.rb` | +| the pgbus transport and its capability gates | `transport_pgbus.rb` | +| `reply.defer`, `reactive_lazy`, the two lanes | `deferred_rendering.rb` | +| `reply.pending`, `reactive_settle`, the handle | `async_actions.rb` | +| the enter/exit/update vocabulary | `effects.rb` | +| the suites, the helpers, the matchers | `testing.rb` | +| the hot paths and the benchmark contract | `performance.rb` | +| instrumentation events and the APM adapters | `observability.rb` | +| doctor, inspector, the MCP server, the generators | `tooling.rb` | +| everything else | the `example_*.rb` pages, one live demo each | + +A behaviour change updates its page in the same PR. A setting that is *removed* is removed from the config, the README, the page and the CHANGELOG together, each carrying a short note on why it is absent — `settle_token_ttl` is the worked example. + +## Its own gates + +```bash +cd docs && bundle exec rake lint # RuboCop with an explicit file list +cd docs && bundle exec rspec # views, requests and system specs (40 files) +cd docs && bin/dev # run it locally +``` + +`rake lint` rather than bare `rubocop` is not a preference: the gem's ancestor `.rubocop.yml` excludes `docs/**/*`, so a bare run in that directory inspects zero files and passes vacuously. The rake task passes `app/**/*.rb spec/**/*.rb Rakefile config.ru` explicitly to defeat the inherited exclude. + +CI runs the site under both real servers in the `site` job of `main.yml`, with `rake lint` on the puma cell only. + +## The `path: ".."` pin + +`docs/Gemfile.lock` pins `phlex-reactive (X.Y.Z)` in two places — the PATH source spec and the CHECKSUMS block — and so does the root `Gemfile.lock` (tracked since #246, because the root `Gemfile` says `gemspec`). Both carry the version string, so a version bump that misses them leaves a committed lockfile stale and the Release workflow's frozen install refuses it. `rake release` therefore rewrites the pin in both with a text edit; see [`../review/release-and-changelog.md`](../review/release-and-changelog.md) for why it is an edit and not a re-resolve. + +The `site` CI job sets `BUNDLE_FROZEN: "false"` for a different reason: the gemspec lists files with `git ls-files`, so every commit changes the file list and the path gem's digest never matches a previously-committed lock. + +## Deploy + +`deploy-docs.yml` fires on `release: published` (and `workflow_dispatch`), delegating to the shared `zoolutions/docs-kit/.github/workflows/deploy.yml`. So the docs go live with the gem. `image`/`service` are the REPO name (`phlex-reactive`, not `-docs`) so the pushed ghcr package auto-links to this repo and `GITHUB_TOKEN` can push and pull it without a PAT; they must match `service:`/`image:` in `docs/config/deploy.yml` and the Dockerfile `LABEL`. The caller must grant `packages: write` — a reusable workflow can narrow the permissions it is given but never escalate them, and the repo default is read-only. + +Plans live in `docs/plans/`; see [`../plans/README.md`](../plans/README.md). + +Related: [`../testing-and-ci/summary.md`](../testing-and-ci/summary.md), [`../review/docs-and-changelog.md`](../review/docs-and-changelog.md). diff --git a/lode/endpoint/summary.md b/lode/endpoint/summary.md new file mode 100644 index 00000000..9ebf9d75 --- /dev/null +++ b/lode/endpoint/summary.md @@ -0,0 +1,94 @@ +# The endpoint: ActionsController, Response, Stream, Authorization + +`app/controllers/phlex/reactive/actions_controller.rb` (the only controller in the gem), `response.rb`, `reply.rb`, `stream.rb`, `authorization.rb`. + +It inherits from `Phlex::Reactive.base_controller` — `ActionController::Base` by default, an app's `ApplicationController` when the app wants its auth/CSRF/Current. `wrap_parameters false`: the JSON body's keys collide with Rails' reserved routing params, which is also why the action travels as `act`. + +## `#create` — the action round trip + +``` +with_url_options(url_options_for(request)) # the ACTOR's protocol/host/port + with_defer_binding(defer_binding_for(request)) # so reply.defer tokens are actor-bound + instrument("action", event) # ONE event, outcome set on every exit path + create_action(event) +``` + +`create_action` in order: `verified_payload` (`verify` or raise `InvalidToken`) → `resolve_component` (must `safe_constantize` AND include `Phlex::Reactive::Component`; the two failures carry distinct diagnostics) → look the action up in the FROZEN `reactive_actions` hash, and 403 when absent (default-deny) → `from_identity` → `coerce_params` → instrument the class for `verify_authorized` → `run_action` → `render turbo_stream: response_streams(result, component)`. + +`run_action` nests deliberately: + +``` +with_connection_id(request.headers["X-Pgbus-Connection"]) # so a broadcast in the action can exclude the actor's echo + with_around_actions(...) # OUTSIDE the transaction: a rejection never opens one + transaction_wrapper # ActiveRecord::Base.transaction when AR is defined + Authorization.with_tracking # open the window + component.public_send(action, **coerced) + Authorization.verify! # INSIDE the transaction: unverified rolls back +``` + +`with_around_actions` short-circuits with a bare `yield` on an empty stack (one `Array#empty?` check on the default request). Otherwise it builds a frozen `ActionContext(component, action_name, params, request)` — with a `coerced.dup.freeze`, because the same hash is splatted into the action and a wrapper mutating it would defeat the schema-coercion guarantee — and folds the stack so the FIRST-registered wraps the action and the LAST-registered runs outermost. **Every wrapper must return `action.call`'s value**; one that returns its logger's result instead silently downgrades the reply to the implicit self-replace. + +### Error paths + +Rescue order is load-bearing; each sets `event[:outcome]` first. + +| Raised | Outcome | HTTP | +|---|---|---| +| `AuthorizationNotVerified` | `:unverified` | tagged, then **re-raised** — a developer error, so a 500 an error tracker sees | +| `InvalidToken` | `:invalid_token` | 400; `event[:component]` stays nil (the name came from an unverified token) | +| `ActiveRecord::RecordNotFound` | `:not_found` | 404 | +| a registered `authorization_errors` class | `:unauthorized` | 403 | +| anything else | `:error` | observed by `report_action_error`, then **re-raised unchanged** | + +`report_action_error` tags the outcome, calls `Phlex::Reactive.report_error`, and renders the `error_flash` at `:internal_server_error` so the actor sees a flash for a crash — every step guarded so the observation path can never replace the error the caller is about to re-raise. + +`reactive_error(status, message, kind:)` never changes the STATUS with any flag, only the body: an `error_flash` turbo-stream wins, else the `verbose_errors` plain-text diagnostic, else a bare `head`. The warn log fires in every environment first, so a misbehaving client is debuggable from the server log alone. `error_flash_stream` degrades to nil when the configured lambda raises, so one failure never becomes a 500. + +### Params + +`coerce_params` builds the collector only under `verbose_errors`, peels one `reactive_scope` level via `unwrap_scope` (only when the component declares a scope AND the raw params carry that single key mapping to a nested params/hash), and coerces through the action's compiled schema. `log_dropped_params` emits ONE warn line naming every dropped key with a reason, and `shape_hint` adds the `#16`/`#21` hint when a dropped segment matches a declared key at a different nesting level — searching exactly one level, hash or array-of-hash. + +## `#deferred` — the pull lane's render leg + +Same `with_url_options` / `with_defer_binding` wrapper, instrumenting `defer`. It verifies the purpose-scoped, short-TTL defer token (an action token is rejected BY SIGNATURE), rebuilds the component, and returns `to_stream_replace(morph: payload["m"] == "morph")` — the morph mode rides inside the SIGNED payload so the client cannot flip it. **No action runs and no transaction opens: this is a read.** A component that answers `render? == false` gets a 204 (keep content, clear pending). The rescue chain mirrors `create_action`'s minus the `AuthorizationNotVerified` clause (no action runs, so nothing can be unverified) and minus the action name in its messages. + +## Token-refresh guards + +The client reads its next signed token out of the response body, so the endpoint's real invariant is "a fresh `data-reactive-token-value` is present", not "some stream targets self". `response_streams` enforces it in two guards: + +- **Guard 1 (target-scoped)** — `carries_token_for?`: does one of the streams already refresh THIS component's token by re-rendering its own root? A `Stream` answers structurally via `rx_refreshes_token_for?` (carries a token AND renders the root AND same target AND a `SELF_RENDER_ACTIONS` action); a raw string falls back to the legacy opening-tag regex. A sibling's replace targets a different id and does not count; an appended child row carries its OWN token but does not render the container's root, so the container still refreshes (without this the list was add-once-only). +- **Guard 2 (global)** — for a `render_self?` reply: does ANY stream carry a token? Deliberately un-scoped, because scoping it would regress update/morph of self on an aliased id. When none does, a reply with a `subject_component` gets a full self-replace prepended, and a companion-only `reply.with` gets a token-ONLY refresh appended instead — so a live input is never clobbered by a forced replace. + +Deferred and pending segments are appended LAST, after every render and op stream, because Turbo applies in document order and because this runs after `run_action` returned — i.e. after the transaction COMMITTED, so a rolled-back action can never leak a directive or a pending marker. + +A redirect is `<turbo-stream action="reactive:visit" data-url="…">` at 200, NOT an HTTP 3xx — the client hard-bails on `response.redirected`, which still correctly catches real auth/CSRF redirects. + +## `Stream` + +An `ActiveSupport::SafeBuffer` subclass carrying `rx_action`, `rx_target`, `rx_renders_root?` and `rx_carries_token?`. The subclassing is verified against actionpack/turbo-rails behaviour: `render turbo_stream: [s1, s2]` only ever does `plain_string << s`, which needs `#to_str`, so the bytes on the wire are byte-identical to raw TagBuilder output. + +`rx_carries_token?` is ONE `include?(TOKEN_ATTR)` scan at BUILD time — **ground truth from the bytes, never inferred from the action**. `renders_root` is set structurally by the builder that knows its own semantics; `append`/`prepend` set it false because they insert children. + +Metadata loss is a feature: `dup`/`+` keep the class and ivars; `gsub`, interpolation and `*` return a plain String, which is exactly when the object is no longer a structurally-known stream — and the endpoint's `is_a?(Stream) && rx_action` guard routes every such loss to the safe legacy regex path. **Never `+`/`gsub`/`<<` a built `Stream` and keep using it as one; re-`wrap` the result.** + +## `Response` and `Reply` + +`Response` is an immutable, frozen value object; every chainable verb returns a NEW one. `Reply` (from `Component::Helpers#reply`) is NOT a Response and does not subclass one — each verb calls a `Response.build_*` class method with the bound component as the subject and returns the real Response. The ten former public `Response.<verb>` class methods raise a guided rewrite naming `reply.<verb>`. + +Three component slots explain the endpoint's behaviour: + +- `subject_component` — set by `replace`/`morph`/`update`; the component a self-targeting builder re-rendered. It does NOT trip `refresh_token?`; it exists so `#js`'s target defaults to the bound root. +- `token_component` — set by `streams`, the collection verbs and `pending`; a reply that does NOT re-render self but still needs its token rolled forward. Without it a collection is add-once-only. +- `render_self?` — false for `remove`, `redirect`, the collection verbs, `streams` and `pending`. + +Verbs: `replace`/`morph`/`update` (self), `remove` (bare = self; with `from:` = a collection row), `append`/`prepend` (`to:` required), `redirect(url)`, `with(*strings)`, `streams(*strings)`, `defer`, `pending`. Chainables: `.stream`, `.flash`, `.also`, `.js`, `.defer`, `.pending`. `reply.also` dispatches on ARGUMENT TYPE — a Streamable component is a replace at its own `#id`, `target => content` pairs are inner-HTML updates — and refuses both forms in one call or neither. + +Content resolution is one contract everywhere (`render_html`): a Phlex component renders through the configured renderer (auto-escaped); anything else is `to_s`'d and handed to Turbo's TagBuilder, which escapes a plain String and passes an `html_safe` one verbatim. `js_ops_json` rejects an empty chain (a dead `reactive:js` stream) and re-applies the attribute allowlist to a raw list. + +## `Authorization` + +`verify_authorized` is default-ON. `instrument!(component_class)` prepends a module wrapping every configured `authorization_methods` name the class defines (public or private, own or inherited) with `super` then `mark!` — so a DENIAL, which raises, never marks and still propagates to the 403 path. It is idempotent per class OBJECT via an ivar, so a Zeitwerk reload re-instruments naturally. `Module#prepend` is bound explicitly through `MODULE_PREPEND` because `Streamable` defines a class method `prepend(target:, model:)` that shadows it. + +`with_tracking` opens a fresh window that starts UNMARKED regardless of an outer mark. `verify!` is a no-op when the feature is off, when the action or whole component declares `skip_verify_authorized`, or when anything marked; otherwise it raises `AuthorizationNotVerified` naming the component#action and all three remedies. `mark_authorized!` (a component helper) always counts. + +Related: `../component/summary.md`, `../streaming/summary.md`, `../async-and-defer/summary.md`. diff --git a/lode/lode-map.md b/lode/lode-map.md new file mode 100644 index 00000000..539c4760 --- /dev/null +++ b/lode/lode-map.md @@ -0,0 +1,37 @@ +# Lode map + +The index of this repository's durable memory. Read this first; it beats a directory listing. Every file describes the system as it is now, with the rationale; `../CHANGELOG.md` records what changed. + +- `summary.md` — what phlex-reactive is and the three invariants every change is measured against +- `terminology.md` — the words this repo uses (identity token, draft token, defer vs settle, handle, anchor, ops chain, companion, actor vs peer…) +- `practices.md` — practices learned from the code and from review that `../.claude/rules/` does not state: capability probes, failing at declaration time, enumerate-once, caching on the render path, threading request state, escaping +- `workflow.md` — the profile the shared `/lode:*` workflow skills read: commands, layers, shapes, constraints, CI, flake sources, conflict rules +- `plans/README.md` — where plans live + +## Subsystems + +- `core-and-config/summary.md` — `Phlex::Reactive` and the engine: the Zeitwerk loader's exclusions, identity and defer token signing with the version upgrade path, the whole settings surface, the four capability gates, off-request rendering, the route guard, instrumentation +- `component/summary.md` — the include stack, the twelve-registry inheritance semantic, `Identity`, lazy mounts, every declaration macro, the view helpers, `ParamSchema`'s drop-don't-fabricate table, `ShowConditions`, `JS` and `Effects` +- `endpoint/summary.md` — `ActionsController#create` and `#deferred` step by step, the around-action fold, the rescue-order table, the two token-refresh guards, `Stream`'s metadata, `Response` vs `Reply`, `Authorization` +- `streaming/summary.md` — `Streamable`: the `#id` contract, `render_in` and the per-thread view context, every stream builder, `broadcast_to`'s one-verb rule and its transport options, and `Collections`' shared decisions +- `async-and-defer/summary.md` — why a settle exists, the pull and push lanes, `reply.defer`, the one-shot stream key, `DeferredRenderJob`, `reply.pending` → `Pending` → `Settles`/`Settle` +- `client-runtime/summary.md` — the five authored modules, the per-file build and its two drift guards, the bare-specifier rule, the wire, the custom turbo-stream actions, the controller's invariants, the overridable seams +- `tooling/summary.md` — `Doctor`, `Inspector`, the MCP diagnostic server, the APM adapters, the three generators, the test helpers and matchers +- `testing-and-ci/summary.md` — the five suites, the server x transport matrix, pgbus in tests, the five CI jobs and their quirks, release and deploy +- `docs-site/summary.md` — the docs-kit app under `docs/`: page-to-behaviour map, its own gates, the `path: ".."` pin, the deploy + +## Review rules (`review/`) + +Accepted review findings rewritten as rules about the system, each verified against the current code and carrying the spec that proves it. `/lode:gate` reads every file here before reviewing a diff; `/lode:learn` adds to them. + +- `review/async-actions.md` — no `settle_token_ttl`, materialize once, reject a misspelled keyword, one subscription per anchor, attributable pending cleanup, best-effort peers, the peer replace path, row kwargs to peers, dom-id Strings +- `review/collections.md` — resolve the size once, edge-trigger the empty state, a replace moves no boundary, bind the container as `token_component`, accept a dom-id String everywhere +- `review/client-runtime.md` — gate the root too, the effect reentrancy token, all-blank vs single-blank legs, deep-freeze op payloads, and two *Not a bug* entries on the connect-time gate posture and the legacy show arm +- `review/observability.md` — snapshot the reporter context, stable built-in adapter instances, probe the SDK's arity, never change what propagates +- `review/release-and-changelog.md` — validate before anything destructive, count the lockfile pins, text-edit not re-resolve, one block per changelog section +- `review/docs-and-changelog.md` — name the exceptions to a "nothing leaves the browser" claim, document real ordering, remove a setting everywhere at once, keep the demo's controls working +- `review/testing.md` — restore every global, capture-then-restore a shared adapter, guard ordering indexes, and two *Not a bug* entries on the confirm teardown and lexical constant lookup + +## Not memory + +- `tmp/` — git-ignored: gate diffs and reports, handovers, scratch diff --git a/lode/plans/README.md b/lode/plans/README.md new file mode 100644 index 00000000..973adebb --- /dev/null +++ b/lode/plans/README.md @@ -0,0 +1,10 @@ +# Plans + +Plans for this repository live in two places, and which one depends on how the work will be picked up: + +- **GitHub issues** — the default. `zoolutions/phlex-reactive` issues are the execution unit the implementation workflow takes by number, and the codebase cross-references them heavily (nearly every non-obvious comment in `lib/` names the issue that produced it). Dedupe with `gh issue list --search "<keywords>"` before opening one, and apply the `plan` label if it already exists rather than creating labels. +- **`../../docs/plans/`** — markdown, named `YYYY-MM-DD-<slug>.md`, for a plan too long or too exploratory for an issue body. `docs/plans/165-deferred-reply-segments.md` is the existing example; note it is named for its issue number rather than by date, so match whichever convention the neighbouring file uses when you add one. + +A plan file is left uncommitted unless the author decides otherwise — committing it is a choice, not a step. + +Working notes, gate reports and handovers are NOT plans: they go in `../tmp/`, which is git-ignored. diff --git a/lode/practices.md b/lode/practices.md new file mode 100644 index 00000000..453ecc16 --- /dev/null +++ b/lode/practices.md @@ -0,0 +1,47 @@ +# Practices + +The binding rules live in `../CLAUDE.md` and `../.claude/rules/` (coding-style, testing, performance, git-workflow, agents). This file adds the practices those do not state, read off the code and off accepted review findings. The specific rules with their proofs are in `review/`. + +## Optionality + +- A feature that needs another gem is gated on a **capability probe of the thing you are about to call**, not on the constant existing. `pgbus_streams?` asks `Stream.instance_method(:broadcast).parameters` for `:exclude`; `defer_push_capable?` additionally checks `SignedName.respond_to?(:sign)` and `ActiveJob::Base`; `APM::Appsignal.available?` checks the SDK constant. A version string is never the gate. +- An optional gem's constants must not be referenced at class-definition or eager-load time. Three paths are held back for exactly this — `mcp/` and `test_helpers/system.rb` are `loader.ignore`d (their classes subclass the `mcp` gem / need Capybara at definition), and `deferred_render_job.rb` is `do_not_eager_load`ed (it subclasses `ActiveJob::Base`, so it must autoload on first reference, behind `defer_push_capable?`, rather than at boot). The loader config at the foot of `lib/phlex/reactive.rb` says why for each. +- Missing capability **degrades and says so once per process**, never per call: `Defer.warn_stream_degraded`, `Pending.warn_no_lane`, `APM.warn_and_nil`. A per-reply warning would bury the signal. + +## Failing loudly, at the right time + +- Declaration-time over request-time. An unknown param type raises `UnknownParamType` when `action` runs (`ParamSchema.compile`); an unknown effect name raises at `reactive_effects` / `Phlex::Reactive.effects=`; a bad `reactive_on_complete` chain raises at class load; a mirror target that is not an id selector raises at declare time. +- Render-time over click-time, but only in dev/test. `on(:typo)` raises at render **only** under `verbose_errors` and **only** when the component declares at least one action of its own — a cross-component dispatch helper with an empty registry is a supported pattern, and production must not 500 on a stale page after a deploy. +- A removed API gets a stub that raises with the exact rewrite, not a silent absence: `Streamable::REMOVED_BROADCASTS` (11 methods), `Response::REMOVED_CLASS_VERBS` (10), `Helpers::REMOVED_ON_KWARGS`, `reactive_compute_def`, `flash_builder`. Clean break plus a guided error; never an alias kept "for compatibility". +- A dead construct is a failure, not a no-op: an empty ops chain, an empty `reply.also`, `reply.defer` on a redirect, a `reactive_on_complete` with no ops. + +## Enumerate once + +- A `records` argument may be a lazy Enumerator or an ActiveRecord Relation. Walk it ONCE into an Array before two consumers read it (`Pending.materialize`), or the marked targets and the enqueued jobs can disagree. +- A size resolver is usually a DB count. Resolve it once per delta and pass the same value to every decision that reads it (`Collections.size_of` → `count_refresh` + `empty_toggle`), or a concurrent write between the two reads ships a count that disagrees with the empty-state toggle beside it. + +## Caching on the render path + +- The identity memos read on every render (`@reactive_record_ivar`, `@reactive_state_ivars`) are bare `defined?`/`||=` with NO generation compare. Coherence comes from the WRITE side: `Component::Registry.bump!` sweeps them off the declaring class and every descendant. Declarations are class-load-shaped and rare; renders are not. +- Anything holding a view context is cached PER THREAD, keyed on the renderer object's identity AND a generation integer, and reset from the engine's `to_prepare`. An ActionView context carries a mutable output buffer, so one shared instance can interleave content across threads. + +## Threading request state + +- Per-request values reach off-request code through a `with_*` / `current_*` pair on `Phlex::Reactive` that saves and restores in `ensure` (`with_connection_id`, `with_url_options`, `with_defer_binding`, `Pending.with_handle`, `Defer.with_real_render`, `Authorization.with_tracking`). `Thread.current` is fiber-local in Ruby, so Falcon's fiber-per-request model is safe. +- A broadcast render deliberately CLEARS the actor's url_options (`with_url_options(nil)`): subscribers can be on other hosts, so "URLs in broadcast-rendered components are host-relative" is the broadcast contract. + +## Strings that reach the browser + +- Every interpolation into a hand-built `<turbo-stream>` goes through `ERB::Util.html_escape` (or `CGI.escapeHTML`) before `.html_safe`, and the comment says the buffer is safe by construction. Concatenate html_safe pieces; `safe + plain` escapes the whole tag. +- Caller-supplied content has one contract everywhere (flash, `also`, defer placeholder): a Phlex component renders through the configured renderer, an `html_safe` String passes verbatim, any other value is escaped data. +- The attribute allowlist is enforced on BOTH sides and in BOTH doors — the `JS` builder at build time, `JS.assert_ops_allowed!` on any raw `[[op, args], …]` escape hatch, and the client interpreter again. + +## Broadcasts are not replies + +- An op that acts on the actor's own focus, form or clipboard is refused on a broadcast (`Streamable::BROADCAST_REFUSED_OPS`): broadcasting it would steal focus, force-submit or read the clipboard in every subscriber's tab. +- Peer delivery is best effort and never fails the job (`Settles#deliver_peers`); the actor's message is already on the wire and a retry would re-send it. + +## Docs and changelog + +- The CHANGELOG declares Keep a Changelog, so `### Added` precedes `### Fixed` and each `## [Unreleased]` carries at most one block per section — merge, never append a second. +- A behaviour change updates the matching page under `docs/app/views/docs/pages/` in the same PR, and a removed setting is removed from the config, the README, the docs page and the CHANGELOG together, with a short note on why it is absent. diff --git a/lode/review/async-actions.md b/lode/review/async-actions.md new file mode 100644 index 00000000..55cf3b2c --- /dev/null +++ b/lode/review/async-actions.md @@ -0,0 +1,61 @@ +# Review rules: the settle lifecycle (`reply.pending` → `reactive_settle`) + +How a pending marker, a handle and a peer broadcast are allowed to behave. Nearly all of these came out of one PR's review; they are the rules that keep a shimmer from lying. + +### A settle has no pull lane, so there is no `settle_token_ttl` to govern one +- **Holds because:** the client cannot poll "is the job done yet", and a pull token redeemed at `/reactive/defer` would render the PRE-JOB component — the exact bug `reply.pending` exists to fix. A TTL setting with nothing to govern tells an app it can extend a wait window it cannot extend. The absence is recorded in place: `lib/phlex/reactive.rb:717` carries a `NOTE: there is deliberately NO settle_token_ttl`, and the README, the docs page and the CHANGELOG each say why it is absent rather than staying silent. It may only come back alongside a real pull-token lane it governs. +- **Where:** `lib/phlex/reactive.rb` (the NOTE above `settle_coalesce_window_ms`); `Phlex::Reactive.settle_capable?` +- **Proven by:** `spec/phlex/reactive/settle_config_spec.rb:"exposes no settle_token_ttl — a settle has no pull lane for a token to govern"` +- **Origin:** cubic learning 6bc297de; PR #250 + +### `records` is walked into an Array exactly once before anything reads it +- **Holds because:** `reply.pending` resolves target ids and then runs the caller's enqueue from the same argument. A one-shot `Enumerator` is exhausted by the first read, so the jobs were silently never enqueued; a re-enumerable `ActiveRecord::Relation` was queried twice, so a concurrent write between the walks could mark one set of rows pending and enqueue jobs for a different set. `Pending.materialize` does the walk (`records.is_a?(Enumerable) && !records.is_a?(String) ? records.to_a : [records]`) and raises on an empty list; `resolve_targets` and `run_enqueue` both take the already-walked Array. +- **Where:** `lib/phlex/reactive/pending.rb#materialize`, called from `#build_segment` +- **Safe direction:** materializing a Relation that was already an Array costs one allocation; not materializing costs a silently unenqueued job. +- **Proven by:** `spec/phlex/reactive/pending_spec.rb:"walks a one-shot Enumerable ONCE — the targets and the enqueue share it"`, `:"resolves a Relation once, so the marked rows and the enqueued jobs cannot disagree"` +- **Origin:** PR #250 + +### A misspelled `reply.pending` keyword raises; it is never swallowed into `**opts` +- **Holds because:** `in` is a Ruby keyword and cannot be a named parameter, which forces every other keyword into `**opts` where a typo like `jbo:` would vanish. The result is rows marked pending with nothing enqueued to settle them — a permanently shimmering row, which is the failure this whole feature exists to prevent. `Response.pending_collection!` deletes `:in` and raises naming the leftover keys and the four legal ones; both `Reply#pending` and the chained `Response#pending` go through it. +- **Where:** `lib/phlex/reactive/response.rb#pending_collection!`, called from `Response#pending` and `Reply#pending` +- **Proven by:** `spec/phlex/reactive/pending_spec.rb:"refuses an unknown keyword rather than silently dropping it"` +- **Origin:** PR #250 + +### A second `reply.pending` for the same container anchor is rejected, not merged +- **Holds because:** every pending segment on one container emits a directive targeting that container's id, and the client keys its in-flight subscriptions BY TARGET — the second directive supersedes the first and silently orphans the jobs the first call enqueued. Merging looked friendlier but does not survive contact: two calls can name different collections and different `peers:`, and `Settle` resolves its default collection from `handle.collection`, so a merge would silently drop one call's default. `Response#pending` raises when `@pending_segments.any? { it.handle.anchor == subject.id }`, naming the rewrite. A **different** anchor is fine — one subscription per anchor, not one per reply. +- **Where:** `lib/phlex/reactive/response.rb#pending` (the anchor guard) +- **Proven by:** `spec/phlex/reactive/pending_spec.rb:"refuses a SECOND pending on the same container — the client would supersede the first"` +- **Origin:** cubic learning d4e51dca; PR #250 + +### Pending markers are cleared on both the success and the failure path, and the failure path clears only what it can attribute +- **Holds because:** the shimmer must resolve even when the work blew up, but clearing every target on one job's failure would un-dim rows that are still legitimately working. `Handle` carries `target_ids`; `finish_streams` clears `handle.target_ids + [handle.anchor]` and tears the source down. On failure, `reactive_settle` rescues, broadcasts `broadcast_settle_cleanup` and **re-raises** so ActiveJob's retry policy still sees the error — and deliberately does NOT tear the subscription down, because a retry still has to reach the actor. Attribution comes from `run_enqueue`: the `job:`/`args:` sugar enqueues one job per record through `each_with_narrowed_handle`, narrowing that job's handle to that record's target id. The **block form cannot be narrowed** — the gem cannot map an arbitrary `perform_later` back to a record — so it carries the whole target list; `finish: true` sweeps those up. +- **Where:** `lib/phlex/reactive/settles.rb#reactive_settle` (the rescue), `#finish_streams`; `lib/phlex/reactive/pending.rb#run_enqueue`, `#each_with_narrowed_handle` +- **Proven by:** `spec/phlex/reactive/settles_spec.rb:"clears the TARGET's pending markers (not just the container's) and re-raises"`, `:"clears nothing for an UNATTRIBUTABLE fan-out failure, rather than un-dimming 176 live rows"`, `:"does NOT tear the subscription down on failure — a retry must still reach the actor"`, `:"clears the pending markers off every TARGET, not just the container"`; `spec/phlex/reactive/pending_spec.rb:"narrows the handle to ONE target per job, so a failure can be attributed"`, `:"gives the BLOCK form the whole target list — an arbitrary enqueue cannot be mapped back"` +- **Origin:** cubic learning 1bd58b61; PR #250 + +### Peer delivery is best effort: it rescues, logs and returns — it never fails the job +- **Holds because:** by the time `deliver_peers` runs the actor's durable message is already on the wire. Re-raising hands the job to the retry policy, which re-runs `perform` and sends the actor's settle a SECOND time — duplicating exactly the pieces that are not idempotent (the flash, the empty-state append). A peer missing a cross-tab courtesy is much smaller than an actor seeing the flash twice, and every other broadcast in the gem is already best effort. `deliver_peers` ends in `rescue ::StandardError => e; log_peer_failure(e)`, which logs why the job was NOT failed. +- **Where:** `lib/phlex/reactive/settles.rb#deliver_peers` +- **Safe direction:** losing a peer's courtesy update, never re-sending the actor's settle. +- **Proven by:** `spec/phlex/reactive/settles_spec.rb:"never fails the job when a peer broadcast blows up — the actor settle already landed"` +- **Origin:** cubic learning 7e297305; PR #250 + +### `Settle#replace` records a peer replace and rides the ordinary row broadcast — no count, no empty-state +- **Holds because:** without a peer op a failed re-execution goes back to actionable for the actor while every other operator keeps the stale row. But a replace moves no collection boundary, so it must NOT go through `broadcast_collection_to`, which would emit a count companion and an empty-state toggle for a size that did not change. `deliver_peer_op` branches on `action == :replace` and calls `definition.item.broadcast_to(*keys, replace: definition.item.send(:build, model, row_kwargs || {}), exclude: handle.connection_id)`. A built component records a `nil` collection name and is broadcast as itself, since it self-targets. +- **Where:** `lib/phlex/reactive/settle.rb#replace`; `lib/phlex/reactive/settles.rb#deliver_peer_op` +- **Proven by:** `spec/phlex/reactive/settles_spec.rb:"reaches peers on a REPLACE too — otherwise other operators keep the stale row"`, `:"re-renders the row in place with no count churn (a replace moves no boundary)"` +- **Origin:** cubic learning 94f127d5; PR #250 + +### The row kwargs a settle was given travel to the peer row build +- **Holds because:** a row component with a required keyword beyond the model renders fine for the actor and **raises** on the peer path — an asymmetry that is nasty to debug, and at best peers render different markup. `Settle`'s `peer_ops` entries carry the row kwargs as their fourth element, `broadcast_collection_to` takes a `row:` keyword, and it threads into `definition.item.build(model, row)`. +- **Where:** `lib/phlex/reactive/settle.rb` (`peer_ops` entries), `lib/phlex/reactive/streamable.rb#broadcast_collection_to`, `#broadcast_collection_row` +- **Proven by:** `spec/phlex/reactive/settles_spec.rb:"carries the row kwargs to peers — a required kwarg would otherwise raise there"` +- **Origin:** cubic learning 63a6014d; PR #250 + +### A DOM-id String is a remove target everywhere a record is, including the peer path +- **Holds because:** `reply.remove(id, from:)` and `Settle#remove` both accept an already-built dom id, matching `Collections.row_remove_stream` — and with `peers: true` that String went straight into `definition.item.build`, which fails before the aggregate streams are emitted. `broadcast_collection_row` short-circuits a String model to `Streamable.broadcast_raw(definition.item, :remove, model, nil, keys, exclude:, visible_to:)` so it never reaches `build`. The bug is reachable from two directions — the direct `broadcast_collection_to` call and the settle — which is why the regression spec drives it from the settle side. +- **Where:** `lib/phlex/reactive/streamable.rb#broadcast_collection_row` (the `action == :remove` branch) +- **Proven by:** `spec/phlex/reactive/settles_spec.rb:"handles a dom-id STRING remove on the peer path without building a row from it"` +- **Origin:** PR #250 + +Related: [`../async-and-defer/summary.md`](../async-and-defer/summary.md), [`collections.md`](collections.md). diff --git a/lode/review/client-runtime.md b/lode/review/client-runtime.md new file mode 100644 index 00000000..c804576b --- /dev/null +++ b/lode/review/client-runtime.md @@ -0,0 +1,39 @@ +# Review rules: the client runtime and the effect wire + +Rules about `app/javascript/phlex/reactive/*.js` and the Ruby that compiles its payloads. Three of the five entries are *Not a bug* — the client's connect-time gate posture and its legacy arms draw a lot of reviewer fire, and the reasons they stay are worth keeping. + +### A gate that hides a dead control must consider the component's own root, not only its descendants +- **Holds because:** a button-only component mixes `on_client(...)` onto `reactive_root`, so the ROOT can be the trigger. A gate that only walks `querySelectorAll` leaves a dead paste button visible in a browser with no Clipboard API — the one thing the gate exists to prevent. `#clipboardGateEnabled` checks `this.element.getAttribute("data-reactive-clipboard")` FIRST and falls through to the descendant scan, and `#syncClipboardTriggers` sets `this.element.hidden` on a marked root before iterating descendants. `#dirtyTrackingEnabled` is the precedent and reads the same way — root attribute first, then `querySelectorAll`. +- **Where:** `app/javascript/phlex/reactive/reactive_controller.js` — `#clipboardGateEnabled`, `#syncClipboardTriggers`, `#dirtyTrackingEnabled` +- **Proven by:** two bun regression tests for the root case (reveal and hide) in `spec/javascript/reactive_paste_op.test.js` +- **Origin:** PR #229 + +### A fire-and-forget effect run carries a per-element token it re-checks at every await point +- **Holds because:** the custom `legs` branch is invoked fire-and-forget for enter and update, and its final `classList.remove(...during, ...to)` always fires after its own settle. Two rapid updates on one element let the first run's cleanup strip classes the second just added, cutting the second animation short. `runLegsEffect` takes the per-element `__reactiveFxToken`, restart-clears an earlier run's leg classes up front, and checks the token at BOTH awaits — post-frame and post-settle — so the mid-flight from→to swap is guarded as well as the cleanup. A superseded exit run resolves early, which only lets Turbo's removal proceed sooner. The built-in className branch already had the token; the rule is that any new async choreography gets it too. +- **Where:** `app/javascript/phlex/reactive/reactive_controller.js#runLegsEffect`, `#runEnterOrUpdateEffect` +- **Proven by:** a bun test that freezes run 2 mid-choreography with manual frame control and proves the stale settle is a no-op +- **Origin:** PR #217 + +### All-blank effect legs raise; a single blank leg is legal +- **Holds because:** `LEG_KEYS.all? { legs.key?(it) }` checks key PRESENCE, so `{ during: nil, from: "opacity-0", to: "opacity-100" }` passed and compiled to an empty-string leg. All-blank is a dead effect with no classes to animate, and now raises — the same loud-failure contract `js_ops_json` applies to an empty ops chain. A SINGLE blank leg stays legal on purpose: an element whose own CSS carries the `transition` property needs no `during:` utilities, and `js.rb`'s `normalize_transition` already tolerates blank legs, so rejecting them here would fork one legs vocabulary into two. Client-side a blank leg splits to zero classes, a `classList` no-op, so neither choice crashes. +- **Where:** `lib/phlex/reactive/effects.rb#legs_wire` +- **Proven by:** `spec/phlex/reactive/effects_spec.rb:"rejects ALL-blank legs — a dead effect with no classes to animate"`, `:"tolerates a single blank leg (element-owned transitions need no during: utilities — the #186 contract)"` +- **Origin:** PR #217 + +### Nested arrays inside an op payload are frozen, not just the containers +- **Holds because:** the JS `OpsChain` mirrors the Ruby builder's immutability, and freezing only the outer containers left `classes` and the transition tuple mutable through `ops.ops` / `toJSON()` — so an already-constructed reducer chain could still be changed. `normalizeTransition` returns `Object.freeze([...])` and `classArgs` returns `classes: Object.freeze(list)`; every `add` returns `new OpsChain(Object.freeze([...this.ops, Object.freeze([name, Object.freeze(args)])]))`. +- **Where:** `app/javascript/phlex/reactive/compute.js` — `normalizeTransition`, `classArgs`, `OpsChain#add` +- **Proven by:** a deep-immutability bun unit test (`Object.isFrozen` down to `classes`/`transition`, plus a throwing `push`) +- **Origin:** PR #227 + +### Not a bug: feature gates are decided once at `connect()` and are not re-armed by a later morph +- **Holds because:** every sibling gate — dirty tracking, show bindings, filters, tags — decides once at connect. `turbo:morph-element` fires per morphed element and bubbles, so wiring the listener unconditionally would cost EVERY component a handler plus a scoped query per morphed element, to serve a trigger that is both conditionally rendered AND first introduced by an in-place morph. The documented contract is "render the paste trigger unconditionally"; a full replace re-connects and re-evaluates the gate, which covers the conditional case. The reviewer withdrew the finding on that reasoning. +- **Where:** `app/javascript/phlex/reactive/reactive_controller.js#connect` and the `#*Enabled` gates +- **Origin:** PR #229 (CodeRabbit, withdrawn) + +### Not a bug: the legacy flat show-attribute arm is not extended with new predicates +- **Holds because:** `showBindingMatches` is the pre-0.10 flat-attribute arm (`data-reactive-show-equals` / `-not` / `-in` / numeric), kept only for deploy overlap and marked for deletion in its own comments. The 0.10 wire always emits the DNF payload, which routes through `showPredicateMatches` where the `len_*` predicates ARE evaluated — and the shared parity fixture proves both sides. No Ruby version ever emitted flat `len_*` attributes, so no real page can reach the legacy arm with a length predicate; a hand-built flat attribute warn-skips under the default-deny posture. Adding predicates to a scheduled-for-deletion arm that never carried them is dead code. +- **Where:** `app/javascript/phlex/reactive/reactive_controller.js` — `showBindingMatches` (legacy) vs `showPredicateMatches` (current); `spec/fixtures/show_predicate_vectors.json` +- **Origin:** PR #227 (CodeRabbit, withdrawn) + +Related: [`../client-runtime/summary.md`](../client-runtime/summary.md), [`../component/summary.md`](../component/summary.md), [`testing.md`](testing.md). diff --git a/lode/review/collections.md b/lode/review/collections.md new file mode 100644 index 00000000..51a94148 --- /dev/null +++ b/lode/review/collections.md @@ -0,0 +1,36 @@ +# Review rules: collection bookkeeping + +`Collections` exists because three callers — the actor's reply, the job-side settle and the peers' broadcast — must make the SAME two decisions about a row delta. These are the rules that keep those decisions from drifting or from disagreeing with each other inside one delta. + +### The size resolver runs ONCE per delta and the same value reaches every decision that reads it +- **Holds because:** the `size:` resolver is usually a DB count. Evaluating it separately for the count companion and for the empty-state boundary costs an extra query on every add and remove — and, worse, a concurrent write landing between the two reads ships a count companion that contradicts the empty-state toggle beside it in the same payload. `Collections.size_of(definition, container)` is the single resolver call; `count_refresh` and `empty_toggle` both take `size = :__unresolved` and resolve only when the caller did not, so every real caller resolves first and passes the value into both. `:__unresolved` is a sentinel distinct from a legitimately nil size, which means "no count stream", not "not computed yet". +- **Where:** `lib/phlex/reactive/collections.rb#size_of`, `#count_refresh`, `#empty_toggle`, `#add_streams`, `#remove_streams`; `lib/phlex/reactive/streamable.rb#broadcast_collection_aggregates` +- **Trap in the same shape:** destructuring `target, size = count_refresh(...)` rebinds `size` to the count's **String** form; the broadcast path had exactly this and handed a String to `empty_toggle`. Name the destructured locals apart from the resolved size. +- **Proven by:** `spec/phlex/reactive/collections_module_spec.rb:"evaluates the size resolver exactly once for an add"`, `:"evaluates the size resolver exactly once for a remove"`, `:"passes the SAME size to the count companion and the empty-state boundary"` +- **Origin:** cubic learning from PR #250 (pre-existing in `collection_add_streams`, fixed in the PR that refactored it) + +### The empty-state toggle is edge-triggered off the live size, never off a client-side increment +- **Holds because:** `empty_toggle` returns `:clear` only when an ADD brought the size to exactly 1 and `:restore` only when a REMOVE brought it to 0. Any other delta leaves the empty state alone, so a list that was already populated does not churn and a list that still has rows does not flash its empty state. Deriving the boundary from a counter the client keeps would drift the moment a broadcast, a settle and a reply all touched the same list. +- **Where:** `lib/phlex/reactive/collections.rb#empty_toggle` +- **Proven by:** `spec/phlex/reactive/collections_module_spec.rb:"leaves the empty-state alone when the list was already populated"`, `spec/phlex/reactive/collection_streams_spec.rb:"does NOT touch the empty-state when the list was already non-empty (size > 1)"`, `:"does NOT restore the empty-state while rows remain (size > 0)"` +- **Origin:** read off `Collections` while verifying the size-resolver rule above (PR #250's subject) + +### A replace emits only the row — no count, no empty state +- **Holds because:** a replace moves no boundary. `replace_streams` emits the row stream alone, and the same reasoning routes `Settle#replace`'s peer op through the ordinary row broadcast rather than `broadcast_collection_to`. +- **Where:** `lib/phlex/reactive/collections.rb#replace_streams` +- **Proven by:** `spec/phlex/reactive/settles_spec.rb:"re-renders the row in place with no count churn (a replace moves no boundary)"` +- **Origin:** cubic learning 94f127d5, generalised from the settle path to every caller + +### A collection reply binds the container as `token_component`, or the list is act-once-only +- **Holds because:** an appended child row carries its OWN token, not the container's, and does not re-render the container's root — so the endpoint's target-scoped guard correctly decides the container still needs a refresh. If the collection verb did not set `token_component`, the reply would carry no fresh container token and the second add would POST a stale one. +- **Where:** `lib/phlex/reactive/response.rb` (the `build_collection_*` verbs); `app/controllers/phlex/reactive/actions_controller.rb#response_streams` (guard 1) +- **Proven by:** `spec/phlex/reactive/collection_streams_spec.rb:"binds the container as token_component so its token rolls forward"`, `:"binds the container as token_component so repeated removes work (cosmos#1939)"` +- **Origin:** the specs' own cosmos#1939 reference; recorded here because the endpoint guard and the reply verb have to agree + +### A row identifier may always be an already-built dom-id String, on every path +- **Holds because:** `reply.remove(id, from:)`, `Settle#remove` and `Collections.row_remove_stream` all accept one, so any new consumer that assumes a record will raise for a caller doing something the rest of the API allows. The peer broadcast path is where this was actually missed; see [`async-actions.md`](async-actions.md). +- **Where:** `lib/phlex/reactive/collections.rb#row_remove_stream`; `lib/phlex/reactive/pending.rb#row_dom_id`; `lib/phlex/reactive/streamable.rb#broadcast_collection_row` +- **Proven by:** `spec/phlex/reactive/collections_module_spec.rb:"accepts an already-built dom-id string"`, `spec/phlex/reactive/collection_streams_spec.rb:"accepts a dom-id string as well as a model"` +- **Origin:** PR #250 (the peer path was the one caller that had missed it) + +Related: [`../streaming/summary.md`](../streaming/summary.md), [`async-actions.md`](async-actions.md). diff --git a/lode/review/docs-and-changelog.md b/lode/review/docs-and-changelog.md new file mode 100644 index 00000000..87889b4a --- /dev/null +++ b/lode/review/docs-and-changelog.md @@ -0,0 +1,29 @@ +# Review rules: documentation prose + +Prose drifts from code faster than tests do, and reviewers read the prose. These are the wordings this repository has already had to correct. + +### A "nothing leaves the browser" claim has to name its exceptions +- **Holds because:** the client-ops page said a `JS` chain sends nothing anywhere, while two of the seventeen verbs do reach outside the page: `submit` hands the form to its own native or intercepted submit path, from which the form may POST or navigate, and `paste_into` reads the system clipboard. The page now names both as the deliberate exceptions and scopes the local-only claim to everything else. Any new op that touches the network, the clipboard or navigation joins that enumeration in the same PR. +- **Where:** `docs/app/views/docs/pages/example_client_ops.rb`; the op list in `lib/phlex/reactive/js.rb` +- **Proven by:** no automated test — docs prose has none. `grep` the README and `CHANGELOG.md` for the same claim before shipping a wording change. +- **Origin:** PR #229 + +### Describe the ordering the runtime actually produces, not the one the API reads like +- **Holds because:** `paste_into` starts `readText().then(...)` and returns immediately, so chained siblings apply WITHOUT waiting; the value write, the bubbling `input` event and the focus all happen when the read resolves. The docs said "on click it awaits", which documents an ordering the code does not have — and the same sentence had been copied into the CHANGELOG. When a fire-and-forget call is documented, say fire-and-forget, and grep the changelog for the same sentence. +- **Proven by:** no automated test — the docs request specs render every page but assert nothing about wording. +- **Where:** `docs/app/views/docs/pages/example_client_ops.rb`; `CHANGELOG.md` +- **Origin:** PR #229 + +### A removed setting is removed from every place that mentions it, each carrying why it is absent +- **Holds because:** an app that reads the config, the README or the docs page in isolation must not come away believing it can tune something that does not exist. When `settle_token_ttl` was dropped it went from the config, the README, the docs page and the CHANGELOG together, each replaced with a short note that a settle's wait is bounded by the job, not a TTL — and a spec asserts `Phlex::Reactive` does not respond to it, so it cannot come back by accident. +- **Where:** `lib/phlex/reactive.rb` (the NOTE), `README.md`, `docs/app/views/docs/pages/async_actions.rb`, `CHANGELOG.md` +- **Proven by:** `spec/phlex/reactive/settle_config_spec.rb:"exposes no settle_token_ttl — a settle has no pull lane for a token to govern"` +- **Origin:** cubic learning 6bc297de; PR #250 + +### A demo page's visible controls all work +- **Holds because:** the docs site is the demo app, so a broken control on a page is a broken claim about the gem. A collection row rendered a dismiss button that dispatched an action the container never declared, so clicking it errored instead of acting. The fix was a row variant whose control dispatches an action the container DOES declare — not declaring the extra action, because a synchronous dismiss would have muddied what an async-archiving demo teaches. A demo change carries a comment saying why it did not reuse the obvious existing component. +- **Proven by:** no test asserts a demo's controls are wired to declared actions; the docs system suite drives some pages but not this one. +- **Where:** `spec/dummy/app/components/archive_queue_component.rb`, `archive_row_component.rb` +- **Origin:** PR #250 + +Related: [`../docs-site/summary.md`](../docs-site/summary.md), [`release-and-changelog.md`](release-and-changelog.md). diff --git a/lode/review/observability.md b/lode/review/observability.md new file mode 100644 index 00000000..3bb549c9 --- /dev/null +++ b/lode/review/observability.md @@ -0,0 +1,28 @@ +# Review rules: instrumentation, error reporting and the APM adapters + +### An error reporter is handed a fresh SNAPSHOT of the context, never the live event hash +- **Holds because:** the same mutable hash is the `ActiveSupport::Notifications` payload, and the notifier adds `:exception` / `:exception_object` to it during unwinding — after `report_error` has already dispatched. A reporter that retains the hash would later read keys the gem never promised it, silently widening the name-only contract. `report_error` forwards `context.slice(*ERROR_CONTEXT_KEYS)` — `:component`, `:action`, `:outcome` — to the resolved APM adapter and to every `on_action_error` hook, each call wrapped in `safely_report` so a broken reporter can never replace the original 500. +- **Where:** `lib/phlex/reactive.rb#report_error`, `ERROR_CONTEXT_KEYS`, `#safely_report` +- **Safe direction:** a reporter seeing too little is a missing tag; a reporter seeing too much is a token or a param in an APM. +- **Proven by:** `spec/requests/apm_error_spec.rb:"forwards a name-only SNAPSHOT — mutating the live event hash after does not leak in"`, `:"does NOT carry token/params/state to the reporter (name-only context)"` +- **Origin:** PR #210 + +### A built-in APM adapter resolves to a STABLE instance per symbol +- **Holds because:** `Subscriber.install` keys its idempotency on adapter identity (`@adapter.equal?(adapter)`), and two separately-`new`'d instances of one class are never `equal?`. `APM.detect` calling `klass.new` per invocation therefore made `attach!` uninstall-and-resubscribe every time — contradicting its own documented "a second call with the SAME adapter is a no-op" — across console reloads, re-run engine initialization and any future hot-reload path. `detect` now memoizes `built_in_instances[apm] ||= klass.new`. The gap had been untested because the idempotency spec only exercised a stored custom-adapter *instance*, which satisfies `equal?` naturally. +- **Where:** `lib/phlex/reactive/apm.rb#detect`, `built_in_instances`; `lib/phlex/reactive/apm/subscriber.rb#install` +- **Proven by:** `spec/phlex/reactive/apm/attach_spec.rb:"resolves a built-in Symbol to a STABLE instance so attach! stays idempotent"`, `:"is idempotent for the same adapter (no double subscription)"` +- **Origin:** PR #210 + +### An adapter probes the vendor SDK's method shape, never its version +- **Holds because:** AppSignal 4.x removed the positional tags argument from `set_error` and requires the block form; 3.x accepts the hash. Pinning a major, or branching on a version string, would make the adapter wrong on one of them and would break the moment either shipped a change. `record_error` branches on `::Appsignal.method(:set_error).arity != 1` — the pgbus capability-gate posture applied inside a gem: ask the thing you are about to call. +- **Where:** `lib/phlex/reactive/apm/appsignal.rb#record_error`, `#set_error_takes_tags?` +- **Proven by:** `spec/phlex/reactive/apm/appsignal_spec.rb:"on AppSignal 3.x passes the tags positionally to set_error"`, `:"on AppSignal 4.x uses the block form (set_error(error) { add_tags(...) })"` +- **Origin:** PR #210 + +### The observation path never changes what propagates +- **Holds because:** `report_action_error` runs on the endpoint's catch-all rescue and is followed immediately by a bare `raise` of the ORIGINAL error, so Rails' own error reporting and the app's middleware fire exactly as they would without it. Every step inside is best-effort: the reporters are isolated in `report_error`, and the `error_flash` render degrades to nil when the configured lambda raises. The status never changes — this catch adds no new 4xx. +- **Where:** `app/controllers/phlex/reactive/actions_controller.rb#report_action_error` and the final `rescue => e` in `#create_action` and `#deferred_action` +- **Proven by:** `spec/requests/apm_error_spec.rb:"still re-raises the ORIGINAL error when an on_action_error hook itself raises"`, `:"still re-raises the ORIGINAL error even when the error_flash lambda itself raises"`, `:"renders the error_flash for the crash yet still re-raises (flash does not swallow)"`, `:"reports to the hook BEFORE the flash (report → flash → re-raise ordering)"` +- **Origin:** PR #210, the contract the other three rules on this page were reviewed against + +Related: [`../tooling/summary.md`](../tooling/summary.md), [`../core-and-config/summary.md`](../core-and-config/summary.md), [`../endpoint/summary.md`](../endpoint/summary.md). diff --git a/lode/review/release-and-changelog.md b/lode/review/release-and-changelog.md new file mode 100644 index 00000000..4f19ff97 --- /dev/null +++ b/lode/review/release-and-changelog.md @@ -0,0 +1,30 @@ +# Review rules: the release task and the changelog + +A release is cheap to re-run and expensive to get wrong, which is the reasoning behind every rule here. + +### Every read and every validation happens BEFORE the first destructive or irreversible step +- **Holds because:** `rake release` does two irreversible things — `gh release delete --cleanup-tag`, which takes the GitHub release, its generated notes and its assets with it, and the writes to `version.rb` and the lockfiles. A validation that aborts after either one leaves the release destroyed or the tree dirty, and the task's own clean-tree guard then blocks the retry. That is exactly the state a failed v0.13.1 attempt left behind. The lockfile preflight only READS files, so there was never a reason for it to run late; it now sits immediately after the header, ordered `header → 0a preflight → 0b force cleanup → 1 version → 1b lockfiles`, and Step 1b writes from the already-read contents. +- **Where:** `Rakefile`, the `:release` task — Step 0a (preflight), Step 0b (force cleanup), Step 1, Step 1b +- **Safe direction:** aborting while failing is still free. An abort must be able to say "Nothing has been modified" truthfully. +- **Proven by:** no automated test — the task is verified by hand against copies of both real lockfiles (the PR records the four branches: clean bump, re-run, no pin, partial). Any change here needs the same manual matrix. +- **Origin:** PR #253 + +### The lockfile pin is COUNTED first: zero aborts, all-current skips, anything stale is rewritten +- **Holds because:** comparing `bumped == content` cannot distinguish "no pin at all" from "pins present and already at the new version". The second is a legitimate state — a re-run after a partial failure — so it cannot simply abort; the first means the file does not pin the gem the way the task thinks, and proceeding would ship `version.rb` bumped against a lockfile still naming the old version. `pin_pattern` is `/^(\s+phlex-reactive) \(([^)]*)\)$/`, which matches the PATH-source spec and the CHECKSUMS pin and deliberately does not match the version-less `phlex-reactive!` in DEPENDENCIES. **Do not assert exactly two matches:** a lockfile written by an older bundler has no CHECKSUMS section and legitimately carries one pin, and a half-applied edit (one stale, one current) passes a `== 2` check while still needing a write. +- **Proven by:** no automated test — verified by hand against copies of both real lockfiles across four branches (clean bump, re-run, no pin, partial). +- **Where:** `Rakefile`, the `:release` task — `pin_pattern`, the Step 0a scan, the Step 1b rewrite +- **Origin:** cubic learning fd99aadf; PR #253 + +### The pin is bumped with a text edit, not a re-resolve +- **Holds because:** `bundle lock --local` is a full re-resolve, and a re-resolve trips over constraints unrelated to this gem — `docs/Gemfile.lock` declares platform-specific entries a laptop cannot satisfy. The only line a version bump needs to change is the path-gem pin, so the task rewrites exactly that line in both tracked lockfiles and leaves everything else byte-identical. +- **Proven by:** no automated test; the Release workflow's frozen `bundle install` is the downstream gate that catches a stale pin. +- **Where:** `Rakefile`, `:release` Step 1b; the two tracked lockfiles are `Gemfile.lock` (the root `Gemfile` says `gemspec`) and `docs/Gemfile.lock` (`path: ".."`) +- **Origin:** PR #253 + +### `## [Unreleased]` carries ONE block per section, `### Added` before `### Fixed` +- **Holds because:** the file declares the Keep a Changelog format, which orders Added first. Two blocks of the same section under one heading is how a merge or a hand-edit hides an entry — and two bullets describing the same mechanism is how the changelog ends up contradicting itself (one bullet claiming the release task "re-locks every tracked lockfile" while the one above it says the pin is bumped with a text edit). Both bullets sat under the same unshipped heading, so there was no reason to keep two descriptions of one mechanism: merge, never append a second. +- **Proven by:** no automated test — this is a review-time check, and it is in `../workflow.md`'s conflict table because a merge is where the duplicate block appears. +- **Where:** `CHANGELOG.md`, the `## [Unreleased]` heading +- **Origin:** cubic learnings 0fa5ffc3; PR #253 + +Related: [`../testing-and-ci/summary.md`](../testing-and-ci/summary.md), [`docs-and-changelog.md`](docs-and-changelog.md). diff --git a/lode/review/testing.md b/lode/review/testing.md new file mode 100644 index 00000000..719cf9b6 --- /dev/null +++ b/lode/review/testing.md @@ -0,0 +1,33 @@ +# Review rules: test hygiene + +Two suites here share a process — bun runs every `spec/javascript` file in one, and RSpec shares a global ActiveJob adapter — so leaked state fails a LATER file and the failure names the wrong test. + +### A bun test file restores every global it installed, not just the interesting one +- **Holds because:** bun runs `spec/javascript` in ONE process, so a fake `fetch`, `document` or `window` left installed is inherited by whichever file runs next, and the resulting failure depends on execution order. The paste-op suite restored `navigator` and left the other three; all four are now snapshotted at module load and restored in `afterEach`. +- **Proven by:** no automated test can prove the absence of a leak; the guard is that `REAL_*` constants are captured at module load and restored in `afterEach`, which is greppable. +- **Where:** `spec/javascript/reactive_paste_op.test.js` and the sibling files' `afterEach` +- **Origin:** PR #229 + +### A spec that changes a global adapter captures the previous value and restores THAT +- **Holds because:** hard-coding the restore (`ActiveJob::Base.queue_adapter = :test` in `after`) restores nothing when `before` already set it to `:test`, and leaks `:test` into every following example if the surrounding suite expected something else. An `around` hook captures and restores, and has the side benefit of dropping the instance variable `RSpec/InstanceVariable` would flag in a `before`/`after` pair. +- **Proven by:** no automated test; `RSpec/InstanceVariable` flags the `before`/`after` form, which is the nearest mechanical signal. +- **Where:** `spec/requests/pending_settle_spec.rb` and any spec touching `ActiveJob::Base.queue_adapter` +- **Origin:** PR #250 + +### An ordering assertion guards its indexes before comparing them +- **Holds because:** `payload.index(...)` returns nil when the stream is missing, and `expect(nil).to be < 3` raises `NoMethodError: undefined method '<' for nil` — a generic crash instead of "the row was never removed". Assert both indexes are non-nil first, so the failure names the real problem. +- **Proven by:** no automated test — it is a property of the assertion itself. +- **Where:** `spec/phlex/reactive/settles_spec.rb` (the `s.move` ordering assertion) +- **Origin:** PR #250 + +### Not a bug: the confirm suites' `afterAll` resolver reads `window.confirm` lazily and must keep doing so +- **Holds because:** the teardown resolver is a closure — `(message) => Promise.resolve(globalThis.window.confirm(message))` — that resolves `globalThis.window.confirm` at CALL time, not at teardown time, so it delegates to whatever the NEXT file installs rather than wrapping this file's last stub. Nothing can leak either way: every confirm test file reassigns `globalThis.window` to a fresh object in its own `buildController` before stubbing, so a stale `confirm` is discarded before it could be read. Capturing and restoring an "original" would be actively wrong — at module-load time bun provides no `window` at all, so there is no pristine value to capture, and freezing the resolver to this file's last stub would break the lazy delegation the next file relies on. The reviewer verified both cross-file orderings and withdrew. +- **Where:** `spec/javascript/reactive_nested.test.js`, `reactive_confirm.test.js`, `reactive_confirm_conditional.test.js`, `reactive_confirm_resolver.test.js`, `reactive_run_ops_confirm.test.js` — each file's `buildController` and `afterAll` +- **Origin:** PR #223 (CodeRabbit, withdrawn) + +### Not a bug: a constant referenced unqualified from a nested module is not a `NameError` +- **Holds because:** `Component::DSL` is lexically nested inside `Phlex::Reactive::Component`, so an unqualified `OnCompleteDefinition` resolves to the `Data.define` beside `ComputeDefinition` and `CollectionDefinition` in `component.rb` — the same path those two already take. A reviewer reading `dsl.rb` alone will report this shape as an undefined constant; the answer is the lexical scope, not a qualification. The reviewer verified and withdrew. +- **Where:** `lib/phlex/reactive/component.rb` (the four `Data.define`s), referenced from `lib/phlex/reactive/component/dsl.rb` +- **Origin:** PR #227 (CodeRabbit, withdrawn) + +Related: [`../testing-and-ci/summary.md`](../testing-and-ci/summary.md), [`client-runtime.md`](client-runtime.md). diff --git a/lode/streaming/summary.md b/lode/streaming/summary.md new file mode 100644 index 00000000..91a1733d --- /dev/null +++ b/lode/streaming/summary.md @@ -0,0 +1,72 @@ +# Streaming: Streamable and Collections + +`lib/phlex/reactive/streamable.rb` (859 lines) and `lib/phlex/reactive/collections.rb`. + +## `#id` — the one contract + +Every streamable component must return a stable DOM id from `#id`, and the root element in `view_template` must carry it: that id IS the Turbo Stream target, which is why targets are never hand-picked. A record-backed component (`reactive_record :x`) gets `dom_id(record)` for free; everything else raises `NotImplementedError` with the fix in the message, because a class-name default would collide the moment two instances render on one page. Two DIFFERENT component classes rendering the SAME record on one page also collide on the default — one of them needs a prefixed id. + +`Streamable#dom_id` delegates to `ActionView::RecordIdentifier`, NOT Phlex's render-time helper: the machinery calls `#id` BEFORE rendering, so Phlex's `dom_id` would raise `HelpersCalledBeforeRenderError`. + +## Rendering + +`render_component(component)` uses phlex-rails' `#render_in` against a memoized off-request view context — a direct `component.call` that skips `ActionController::Renderer`'s `TemplateRenderer`/`LookupContext`/log-subscriber machinery: roughly 2× faster with about half the allocations, byte-identical HTML, full helper access. It wraps the render in the `render.phlex_reactive` event (component name + html bytesize, filled after rendering) and inside `Defer.with_real_render` so a `reactive_lazy` component emits its real template. + +The view context and its `Turbo::Streams::TagBuilder` are cached PER THREAD (`ThreadViewContext`), keyed on the renderer object's identity and a per-class generation. Per-thread and not per-process because an ActionView context carries mutable `output_buffer`/`view_flow` state that `render_in`'s capture swaps — one shared instance can interleave content on Puma or in concurrent jobs. `reset_turbo_view_context!` bumps the generation; `Streamable.reset_all_view_contexts!` walks a `WeakMap`-backed registry of every includer and is called from the engine's `to_prepare`. The registry is a `WeakMap` used as a set so a Zeitwerk-reloaded class is GC'd instead of pinned. + +**The gem deliberately does NOT include `Turbo::Streams::ActionHelper`** — it pulls in `ActionView::Helpers::TagHelper`, which overrides Phlex's internal `tag` method and breaks rendering. `Turbo::Streams::TagBuilder` is used directly. + +## Stream builders + +Class-level, each returning a wrapped `Stream`: + +| Builder | Target | `renders_root` | +|---|---|---| +| `replace(model, morph:, effect:)` | the built component's `#id` | true | +| `update(model, morph:, effect:)` | the built component's `#id` | true | +| `append(target:, model:, effect:)` | the caller's container id | **false** — it inserts a CHILD, and a reactive child's own token is not the container's | +| `prepend(target:, model:, effect:)` | the caller's container id | false | +| `remove(model, effect:)` | the built component's `#id` | false | + +Instance-level: `to_stream_replace(morph:, effect:)`, `to_stream_update(morph:, effect:)`, `to_stream_remove(effect:)`, and `to_stream_token` — a body-less `<turbo-stream action="reactive:token">` carrying just the fresh signed token, so a partial reply rolls the identity forward without tearing down a live input. `to_stream_token` reads the token through `respond_to?(:reactive_token, true)` **with the include-private flag**: `reactive_token` is private, so a plain `respond_to?` is false for every Component and the stream would silently carry an EMPTY token — which makes any non-self-rendering reply act-once-only. + +`morph: true` emits `method="morph"` so Turbo 8's bundled Idiomorph morphs in place and a focused input keeps its caret; the default is the plain outerHTML swap and is byte-identical to the pre-morph wire. `to_stream_morph` is removed and raises with the `to_stream_replace(morph: true)` rewrite. + +## `broadcast_to` — one method, one verb kwarg + +```ruby +Item.broadcast_to(@list, :todos, replace: @todo, morph: true) +Item.broadcast_to(@list, :todos, append: todo, target: dom_id(@list, :todos), exclude: reactive_connection_id) +Counter.broadcast_to(each: accounts.map { [it, :counters] }, replace: counter) +Badge.broadcast_to(user, :alerts, js: js.add_class("#bell", "has-unread")) +Phlex::Reactive.broadcast_to(@list, :todos, update: TodoCount.new(list: @list), target: "todos-count") +``` + +Exactly ONE of the six verbs (`replace`, `update`, `append`, `prepend`, `remove`, `js`); zero or two raise. `replace`/`remove` are SELF-TARGETING — they derive the target from the payload's `#id` and require a Streamable payload, with a guided error steering a plain component to `update:`. `update` self-targets too when no `target:` is given; `append`/`prepend` require an explicit `target:`. The payload is a record (built via `model_param_name`), an init-kwargs Hash (used verbatim — no `**options` collision), or an already-built Phlex component. `each:` fans ONE build + ONE render + ONE signing out to K keys with K cheap channel calls. + +Both the class-level form and the module-level `Phlex::Reactive.broadcast_to` funnel through `Streamable.broadcast_component`, so neither can be silently un-instrumented and the actor-only op gate is reachable from both doors. `broadcast_raw` is the same path for already-rendered HTML (the count companion is a plain number with no render leg). + +`exclude:`, `visible_to:` and `coalesce:` are TRANSPORT options, not init args, and they do **not** travel as kwargs to `Turbo::StreamsChannel` — turbo-rails swallows unknown kwargs into its render locals, so passing `exclude:` that way silently dropped the actor-echo suppression. They ride thread-locals (`Thread.current[:pgbus_broadcast_exclude]` and siblings) that pgbus's `broadcast_stream_to` patch reads, set by `with_pgbus_broadcast_opts` under the `pgbus_streams?` gate and cleared in `ensure`. On Action Cable nothing reads them, so the whole thing is a no-op. `coalesce:` needs a newer pgbus still; an older one simply ignores the thread-local (more messages, same correctness), which is why it has no gate of its own. + +`morph:` and `effect:` reach the broadcast wire through `attributes:` (the broadcast path has no `method:` kwarg), compiled by `broadcast_wire` — `{}` when neither, so a plain call is byte-identical. + +A broadcast render runs inside `with_url_options(nil)`: subscribers can be on different hosts, so absolute URLs in broadcast-rendered components keep the process defaults. + +`js:` broadcasts go through `broadcast_js_ops_json`, which refuses the six `BROADCAST_REFUSED_OPS` (`focus`, `focus_first`, `submit`, `paste_into`, `persist_state`, `persist_clear`) — broadcasting focus steals it in every tab, `submit` force-submits every subscriber's form, `paste_into` reads every subscriber's clipboard. That gate lives on the module singleton so BOTH doors reach it. + +The eleven `broadcast_*_to` / `_to_each` methods are removed; each is defined as a stub raising with the exact `broadcast_to` rewrite for that verb. + +## `Collections` — the shared bookkeeping + +A collection row is never just a row: adding one must also refresh the `count:` companion and clear the `empty:` state at the 0→1 boundary; removing one must refresh the count and restore the empty state at 1→0. `Collections` holds the two DECISIONS three callers share — `Response.build_collection_*` (the actor's reply), `Settle` (the job-side settle) and `Streamable.broadcast_collection_to` (the peers' broadcast): + +- `count_refresh(definition, container, size)` → `[count_target, size.to_s]` or nil (nil when no `count:` is declared or the resolver returned nil — the count stream is simply omitted, so a list with only rows still works). +- `empty_toggle(definition, container, delta, size)` → `:clear` (add, size just became 1), `:restore` (remove, size is now 0) or nil. Edge-triggered off the LIVE size, never a client-side increment. + +The reply/settle paths build `<turbo-stream>` STRINGS and the broadcast path hands pieces to `Turbo::StreamsChannel`, so they cannot share the rendering — the decisions are what they share, because the decisions are what drifts. `size_of` resolves the size ONCE per delta and every renderer passes the same value down; `:__unresolved` is the not-computed-yet sentinel, distinct from a legitimately nil size. + +`row_remove_stream` and `Pending.row_dom_id` both accept an already-built dom-id STRING as well as a record. `replace_streams` emits only the row — a replace moves no boundary, so no count and no empty-state stream. + +`broadcast_collection_to(*keys, container:, in:, append:|prepend:|remove:, row:, coalesce:, exclude:)` is the peers' counterpart of `reply.append`/`reply.remove`: the plain `broadcast_to(append:)` emits the BARE row because it has no container instance to resolve the declaration or run the size resolver. `in:` cannot be a named parameter (it is a Ruby keyword), so it is pulled out of `**opts`. The ROW stream is never coalesced (an append is not idempotent and each row is a distinct target); the aggregate streams are. + +Related: `../async-and-defer/summary.md` (the settle that reuses these decisions), `../review/async-actions.md`. diff --git a/lode/summary.md b/lode/summary.md new file mode 100644 index 00000000..5882d456 --- /dev/null +++ b/lode/summary.md @@ -0,0 +1,11 @@ +# phlex-reactive + +A Rails engine that makes a [Phlex](https://www.phlex.fun) component reactive: declare `action :name` in Ruby and one generic Stimulus controller turns a click or a form input into an HTTP round trip that re-renders that component and applies it back into the DOM, while a background `broadcast_to` pushes the same re-render to every other tab over Turbo Streams. Client interactivity and server-pushed live updates converge on ONE unit — the component, targeted by the stable DOM `id` it owns (`Streamable#id`, defaulting to `dom_id(record)` for a record-backed component). No per-feature Stimulus controllers, no hand-picked Turbo Stream targets. + +Three invariants govern every change: + +- **Signed identity, never state.** The DOM root carries a `MessageVerifier`-signed `{"c" => class, "gid" => …}` and/or `{"s" => declared state}` (`Component::Identity#reactive_identity_payload`), purpose-scoped to `IDENTITY_PURPOSE` and version-stamped. The endpoint re-finds the record through GlobalID; no client-supplied snapshot is ever trusted back. +- **Default-deny, and the signature is not authorization.** Only a method declared with `action` is invokable (`ActionsController#create_action`), only declared params reach it (`ParamSchema#coerce` drops everything else), and the default-ON `verify_authorized` guard raises inside the action's transaction when nothing authorized (`Authorization.verify!`) — so a forgotten `authorize!` rolls back and surfaces as a 500, not a silent hole. +- **pgbus is optional and runtime-detected.** Broadcasts route through `Turbo::StreamsChannel`, which [pgbus](https://github.com/zoolutions/pgbus) patches to run over Postgres SSE. Every pgbus-only feature is gated on a capability PROBE (`Phlex::Reactive.pgbus_streams?` asks whether `Stream#broadcast` accepts `:exclude`), never `defined?(::Pgbus)` alone or a version string, and degrades to the Action Cable path when absent. The same optionality rule governs the APM adapters and the `mcp` gem. + +The gem's runtime dependencies are railties, phlex-rails, turbo-rails, globalid and zeitwerk (`phlex-reactive.gemspec`); pgbus, the vendor APM SDKs, the `mcp` gem, Capybara and RSpec are all dev/test or absent-tolerant. Ruby >= 3.4, Rails >= 7.1 (`railties >= 7.1, < 9.0`). diff --git a/lode/terminology.md b/lode/terminology.md new file mode 100644 index 00000000..a539094c --- /dev/null +++ b/lode/terminology.md @@ -0,0 +1,39 @@ +# Terminology + +The words this repository uses. Where a word names a constant or method, the definition is the code. + +## The round trip + +- **reactive component** — a Phlex component that `include Phlex::Reactive::Component`. Gets `Streamable` (id + streams + broadcasts), `ClientBindings` (the declaration DSL + view helpers), `Identity` (the token) and `Lazy` in that order (`component.rb`). +- **client-only component** — `include Phlex::Reactive::ClientBindings` instead: the same DSL and helpers with NO `Streamable` and NO `Identity`, so no `#id` is required and the root is tokenless. The server-action macros (`action`, `reactive_record`, `reactive_state`) raise at class-definition time on one (`Component::DSL#require_server_actions!`). +- **identity token / signed identity** — the `MessageVerifier` payload on the root's `data-reactive-token-value`: `{"c" => class name}` plus `"gid"` (a persisted record's GlobalID) and/or `"s"` (the declared `reactive_state` ivars), stamped `"v" => TOKEN_VERSION`. Built by `Component::Identity#reactive_identity_payload`, signed by `Phlex::Reactive.sign`. +- **draft token** — the same payload with NO `gid`, minted for an unsaved (`persisted? == false`) or nil record. `from_identity` omits the record kwarg so the component's `initialize` default seeds a fresh draft. +- **defer token** — the same payload signed under `DEFER_PURPOSE` with `defer_token_ttl` (120s default). Disjoint from an action token by purpose, so neither can be redeemed at the other's endpoint. +- **defer binding** — the persisted session id folded into a defer token's PURPOSE string (`Phlex::Reactive.defer_purpose`), so a token minted under one actor fails verification under another. Unbound (no persisted session) is a documented, supported state. +- **`act`** — the wire name of the action in the POST body. Not `action`: that is a reserved Rails routing param. +- **default-deny** — only a declared `action` is invokable (server), only an allowlisted op name/attribute runs (client). Both sides enforce independently. + +## Replies and streams + +- **reply** — `Component::Helpers#reply` returns a `Phlex::Reactive::Reply` bound to the component; each verb builds and returns the frozen `Phlex::Reactive::Response` the endpoint reads. `Reply` is not a `Response` and does not subclass one. +- **`Stream`** — an `ActiveSupport::SafeBuffer` subclass that IS the `<turbo-stream>` bytes but also carries `rx_action`, `rx_target`, `rx_renders_root?` and a `rx_carries_token?` flag computed once from the bytes. The endpoint reads fields instead of regexing markup. +- **token refresh** — the endpoint guarantees the reply carries a fresh token: either a stream that re-renders the root (`Stream::SELF_RENDER_ACTIONS` = replace / update / `reactive:token`) or the tiny inert `to_stream_token` stream. +- **companion** — an element re-rendered alongside the subject: `reply.also(component)` (a replace at its own `#id`) or `reply.also(target => content)` (an inner-HTML update of that id). +- **count companion / empty-state** — the `count:` and `empty:` members of a `reactive_collection`; every row add/remove also refreshes the count and toggles the empty state at the 0↔1 boundary (`Collections`). +- **actor / peer** — the actor is the client that made the request and gets the HTTP reply; peers are every other subscriber of the stream. `exclude: reactive_connection_id` suppresses the actor's own broadcast echo. + +## Async + +- **defer (`reply.defer`, `reactive_lazy`)** — take a render off the actor's critical path. Two lanes: **pull** (`:fetch` — the client POSTs the defer token to `/reactive/defer`) and **push** (`:stream` — a pgbus durable one-shot stream plus `DeferredRenderJob`). `:auto` picks push iff `defer_push_capable?`. +- **settle (`reply.pending` → `reactive_settle`)** — mark targets pending, let the app's own job report the outcome. Push lane only: a settle has no pull fallback, because the client cannot poll "is the job done yet". +- **handle** — `Pending::Handle`, the JSON-round-trippable record of a settle (shared stream key, container class + identity payload, anchor id, collection, target ids, peers, connection id). It rides ActiveJob metadata via `Settles#serialize`. +- **anchor** — the container component's DOM id: the settle's subscription target and teardown target. +- **one-shot stream key** — a `prdefer_`-prefixed random key sized to pgbus's live queue-name budget; one PGMQ queue per key, reclaimed by pgbus's orphan sweep. + +## Client vocabulary + +- **op / ops chain** — `Phlex::Reactive::JS`, an immutable builder of declarative DOM commands (17 chainable verbs) serialized to `[[name, args], …]`. Ephemeral UI: any server re-render resets what they toggled. +- **`@root`** — `JS::ROOT_SENTINEL`, "the component's own root element" in an op target. +- **conditions / DNF groups** — the one `if:` / `if_any:` / `unless:` language `ShowConditions` compiles to an array of groups (terms AND within a group, groups OR) and evaluates identically in Ruby and in JS. +- **effect** — an enter/exit/update animation name (`fade`, `slide`, `scale`, `highlight`, `shake`), `:random`, `false`, or custom `{ during:, from:, to: }` legs; opt-in globally, per component and per call. +- **vendored client** — the five files under `spec/dummy/public/vendor/` named in `spec/phlex/vendored_controller_sync_spec.rb`'s map (`reactive_controller.js`, `confirm.js`, `confirm_predicate.js`, `compute.js`, `inspect.js`), each byte-identical to the built `*.min.js` so the browser suite exercises the minified code production ships. The rest of that directory (stimulus, turbo, trix, lexxy, the dummy's own reducers) is vendored third-party or hand-written and no guard covers it. diff --git a/lode/testing-and-ci/summary.md b/lode/testing-and-ci/summary.md new file mode 100644 index 00000000..7d6849c6 --- /dev/null +++ b/lode/testing-and-ci/summary.md @@ -0,0 +1,61 @@ +# Testing and CI + +Five suites, two of them browser-driven, one of them JavaScript. All the Ruby ones drive `spec/dummy`, a minimal Rails app with nine models and 71 example components. + +## The suites + +| Suite | Path | Files | Boots | Proves | +|---|---|---|---|---| +| Unit | `spec/phlex/**` | 58 | nothing, or a stubbed verifier | identity signing, the DSL, the registries, the capability gates, `ParamSchema`, `JS`, `Effects`, `Collections` | +| Request | `spec/requests/**` | 43 | the dummy app | the endpoint: token verify, default-deny, coercion, 400/403/404, `reply.*`, broadcast payloads | +| System | `spec/system/**` | 55 | dummy + Capybara/Playwright | the real browser loop — click to morph, no reload, rapid-click races, live SSE delivery | +| Generators | `spec/generators/**` | 3 | Rails' generator harness | the three generators' output | +| JavaScript | `spec/javascript/**` | 52 | bun, no browser | the client runtime's units, run with `bun test spec/javascript` | + +`bundle exec rake` is `spec` + `rubocop`, where `spec` is the pattern `spec/{phlex,requests}/**/*_spec.rb` — the fast suite. The system suite is deliberately NOT in the default task; invoke it with `rake spec:system`. + +## What the browser suite runs + +It serves the **vendored minified** client from `spec/dummy/public/vendor/`, not the authored source. Production ships minified, so a minifier-induced bug — a mangled name breaking a Stimulus lifecycle hook, a dropped export — would otherwise ship untested. `spec/phlex/vendored_controller_sync_spec.rb` names the five files that must stay byte-identical to their `*.min.js` twins and prints the exact `rake build:js && cp …` re-sync command in its failure message; `rake build:js_check` is the same guard from the other side, comparing a fresh build against the index. + +Two real servers, two transports, run as a 2x2: + +- **server**: `puma` (sync, thread pool) or `falcon` (async, fiber-per-request), via `CAPYBARA_SERVER`. No webrick — it is not a real server. `Thread.current` is fiber-local in Ruby, which is what makes the `with_*` request-state pairs safe under Falcon; the Falcon cells are the proof. +- **transport**: `cable` (Action Cable over SQLite) or `pgbus` (Postgres SSE), via `TRANSPORT`. The pgbus cells additionally prove real cross-tab delivery and actor-echo exclusion over live SSE. + +`rake spec:system_servers` runs both servers locally; `rake spec:system_matrix` runs the full 2x2 and **skips the pgbus cells with a clear note when `pg_isready` fails**, so the cable cells still prove the round trip on a machine with no Postgres. `rake pgbus:prepare_test_db` (via `spec/support/prepare_pgbus_db.rb`) boots the dummy under `TRANSPORT=pgbus`, loads the schema and installs pgbus's vendored PGMQ — no `CREATE EXTENSION`, so a plain `postgres:18` image works. + +## pgbus in tests + +pgbus is a dev/test dependency with `require: false` and a Ruby >= 3.3 floor (the gem's own runtime floor is 3.4). The default suite needs no Postgres. A pgbus-specific spec guards with `defined?(Pgbus)` or a tag and asserts BOTH paths — pgbus present and the capability-gate fallback. The regression guard for the `ArgumentError: unknown keyword :exclude` class of bug is a double shaped like old pgbus: a `broadcast` whose signature has no `:exclude`. + +## Fixtures + +`spec/fixtures/show_predicate_vectors.json` is the shared parity fixture: the same predicate vectors run through Ruby's `ShowConditions.match?` and through the client's evaluator, so the two cannot drift. `spec/fixtures/files/` holds the upload fixtures (`receipt.txt`, `page1.txt`, `page2.txt`). + +## CI (`.github/workflows/main.yml`) + +Triggers on `push` to `main` and on every `pull_request`. Five jobs: + +| Job | Cells | Runs | +|---|---|---| +| `lint` | 1 (Ruby 4.0) | `bundle exec rubocop`, then `gem build phlex-reactive.gemspec --strict` | +| `test` | Ruby 3.4, 4.0 | `bundle exec rspec spec/phlex spec/requests spec/generators` | +| `bench` | 1 (Ruby 3.4) | `rake bench:micro` and `rake bench:request`, uploaded as an artifact — **report-only, never a hard fail** | +| `site` | puma, falcon | the `docs/` app's own request and system suites; `rake lint` on the puma cell only | +| `system` | server x transport = 4 | `bun test spec/javascript` (cable cells), `rake build:js_check` (puma+cable only), the pgbus DB prep on pgbus cells, then `rspec spec/system`; failure screenshots uploaded | + +All three matrix jobs (`test`, `site`, `system`) set `fail-fast: false`, so one red cell does not hide the others; `lint` and `bench` are single cells with no matrix. + +Two CI quirks worth knowing before diagnosing a failure: + +- The `site` job sets `BUNDLE_FROZEN: "false"`. The docs app depends on the gem by `path: ".."` and the gemspec lists files with `git ls-files`, so every commit changes the file list and the path gem's gemspec digest never matches a previously-committed lock. Frozen mode would fail with "the gemspecs for path gems changed" on every PR. +- The docs app's lint gate is `bundle exec rake lint`, not bare `rubocop`. The gem's ancestor `.rubocop.yml` excludes `docs/**/*`, so a bare run there inspects zero files and passes vacuously; the rake task passes the file list explicitly. + +## Release and deploy + +`bin/release` is the front door (`list`, `--dry-run`, `patch|minor|major|X.Y.Z`, `--force`); it refuses anything but a clean, up-to-date `main` and hands off to `rake release[X.Y.Z]`. That task bumps `version.rb`, rewrites the `phlex-reactive (X.Y.Z)` pin in both tracked lockfiles with a text edit, commits, pushes `main` and publishes the GitHub Release. `release.yml` then publishes to RubyGems by trusted publishing (OIDC + Sigstore) — never `gem push` by hand — and `deploy-docs.yml` ships the docs site on the same `release: published` event, through the shared `zoolutions/docs-kit` deploy workflow. + +The release task's ordering is itself a rule; see [`../review/release-and-changelog.md`](../review/release-and-changelog.md). + +Related: [`../client-runtime/summary.md`](../client-runtime/summary.md), [`../docs-site/summary.md`](../docs-site/summary.md), [`../workflow.md`](../workflow.md). diff --git a/lode/tooling/summary.md b/lode/tooling/summary.md new file mode 100644 index 00000000..c08fc0ba --- /dev/null +++ b/lode/tooling/summary.md @@ -0,0 +1,66 @@ +# Tooling: doctor, inspector, MCP, APM, generators, test helpers + +The surfaces an adopter and an agent touch that are not the round trip itself. Everything here is **read-only or install-time**, and each is optional-dependency-tolerant in the same way the pgbus path is. + +## `Doctor` — "nothing happens, why?" + +`lib/phlex/reactive/doctor.rb`, driven by `bin/rails phlex_reactive:doctor` (`lib/tasks/phlex_reactive.rake`). Five closed integration papercuts (boot/eager-load, route shadowing, a lost request, an unregistered Stimulus controller, an importmap 404) only ever surfaced after something already broke; the doctor turns them into a checklist you run before and after setup. + +`build_checks` returns nine checks in this order: `route_check`, `defer_route_check`, `stimulus_check`, `csrf_check`, `verifier_check`, `base_controller_check`, then the three registry-reading ones `action_check`, `id_check`, `authorization_check`. Each answers a `Check` value object — a status (`:ok` / `:fail` / `:unknown`), a human message, and on anything but `:ok` a `fix:` line. + +Two contracts hold it in place: + +- **It is read-only.** It never mounts a component, mutates state or crosses the default-deny boundary; the worst it does is a throwaway sign then verify round trip and one pass over the loaded `Streamable` registry. +- **`Doctor.run` returns true when nothing FAILED** — an advisory `?` does not count — so the rake task can `abort unless` it and a setup script or CI can gate on the exit code. `GLYPHS` are plain Unicode with no ANSI, so log capture reads cleanly. + +`run` calls `Rails.application.eager_load!` itself, because the three component checks read the `Streamable` registry and it is empty otherwise. + +## `Inspector` and `Inspector::Report` + +`inspector.rb` walks the same registry into two `Data` shapes — `ActionInfo(name, params, source_location, definition)` and `ComponentInfo(klass, name, path, record_key, state_keys, actions)` — and `inspector/report.rb` renders them. Three rake tasks consume the pair: + +| Task | Output | +|---|---| +| `phlex_reactive:actions` | every declared action as `component \| action \| params \| file:line \| auth`; `FORMAT=json` for tooling | +| `phlex_reactive:find[query]` | a fuzzy component match plus its actions with the method's source | +| `phlex_reactive:doctor` | the checklist above | + +Names, paths and schemas only — no token, no state, no param values. The same discipline the instrumentation payloads keep. + +## MCP: the diagnostic server + +`mcp.rb` plus `mcp/{base_tool,runner,server}.rb` and five tools — `phlex_reactive_components`, `phlex_reactive_actions`, `phlex_reactive_find`, `phlex_reactive_doctor`, `phlex_reactive_config`. Run over stdio with `bin/rails phlex_reactive:mcp`. + +The `mcp` gem is **optional and absent from the gemspec**. The whole `mcp/` subtree is `loader.ignore`d because its tools subclass `MCP::Tool` at class-definition time; `MCP.load!` requires the gem — raising a `Phlex::Reactive::Error` naming the Gemfile line when it is absent — and then requires the tree in dependency order. `mcp.rb` itself references no gem constant at load time, so Zeitwerk autoloads it normally. + +The runner's one hard rule: **nothing but JSON-RPC frames may reach stdout**, so a host app's chatty initializer breaks the transport. + +## APM adapters + +`apm.rb` plus `apm/{adapter,appsignal,datadog,sentry,subscriber}.rb`. `Phlex::Reactive.apm` takes a Symbol, a custom adapter object (responding to `record_action` / `record_error`), or nil. `BUILT_INS` maps three symbols — `:appsignal`, `:sentry`, `:datadog` — to adapter class names. + +Resolution is deferred to the engine's `after_initialize`, so a vendor SDK loaded by an app initializer is already there. Two rules are load-bearing and both came out of review: + +- **`detect` memoizes the instance per symbol** (`built_in_instances[apm] ||= klass.new`). `Subscriber.install` keys idempotency on `@adapter.equal?(adapter)`, so a fresh `klass.new` per call would uninstall and re-subscribe on every `attach!` instead of no-op'ing. +- **The adapter probes the SDK's shape, not its version.** `Appsignal#record_error` branches on `::Appsignal.method(:set_error).arity` — 3.x takes the tags positionally, 4.x needs the block form — so one adapter spans both majors without pinning either. The pgbus capability-gate posture applied inside a gem. + +A set-but-undetectable SDK logs ONE warning at boot through `warn_and_nil` and no-ops. No vendor SDK is ever a hard dependency. + +## Generators + +Three, all under `lib/generators/phlex/reactive/` and all `loader.ignore`d — Rails' generator system owns their discovery and their path/constant scheme is deliberately non-Zeitwerk: + +- `phlex:reactive:install` — writes the initializer from `templates/phlex_reactive.rb.erb`, registers the Stimulus controller, prints the next steps. +- `phlex:reactive:component NAME` — the component plus its spec, with `--record` and `--state` class options; the spec is skipped when RSpec is absent. +- `phlex:reactive:claude` — installs the packaged debugging skill from `lib/phlex/reactive/claude/skills/` into the host app and configures the MCP entry. Its `source_root` points into the gem's own `claude/skills`, which the gemspec therefore has to package. + +## Test helpers + +`test_helpers.rb` is the request-spec surface: `reactive_token_for`, `post_reactive_action`, `post_reactive_multipart` and `run_reactive` — which runs a declared action through the same coercion and transaction wrapper the endpoint uses and hands back a `Result` answering `replace?` / `remove?` / `redirect?` / `streams`. + +Its two companions are required conditionally at the foot of the file, which is exactly why both are `loader.ignore`d: + +- `test_helpers/matchers.rb` — `have_reactive_replace`, `have_reactive_remove`, `have_reactive_token_for`, each accepting a component instance or a bare DOM id. Required only `if defined?(RSpec::Matchers)`; it defines constants under `RSpec::Matchers`, not under `Phlex::Reactive`. In an `RSpec::Matchers.define` block the `do |arg|` parameter is the EXPECTED value and `match do |actual|` the actual — a RuboCop autocorrect that conflates the two silently guts the matcher, which is why the file carries the cop disable. +- `test_helpers/system.rb` — `wait_for_reactive`, `have_reactive_value`, `have_reactive_text`: polling matchers for the browser suite's async morph, clocked on `Process::CLOCK_MONOTONIC`. Required only `if defined?(Capybara)`, so an eager load in production never defines browser helpers with no Capybara. + +Related: [`../core-and-config/summary.md`](../core-and-config/summary.md) (the settings these read), [`../testing-and-ci/summary.md`](../testing-and-ci/summary.md), [`../review/observability.md`](../review/observability.md). diff --git a/lode/workflow.md b/lode/workflow.md new file mode 100644 index 00000000..72f8f439 --- /dev/null +++ b/lode/workflow.md @@ -0,0 +1,136 @@ +# Workflow profile + +Everything the shared workflow skills (`/lode:lfg`, `/lode:review-pr`, `/lode:finish-prs`, `/lode:debug-flaky`, `/lode:tdd`, `/lode:plan`) need to know about this repository that is not already in `../CLAUDE.md`, `../.claude/rules/` or the rest of `lode/`. + +## Commands + +| Purpose | Command | Notes | +|---|---|---| +| fast loop (one file) | `bundle exec rspec <file>` (e.g. `bundle exec rspec spec/phlex/reactive/pending_spec.rb`) | no services | +| full suite | `bundle exec rake` (= `rspec spec/{phlex,requests}` + `rubocop`) | no network, no services; **safe in two worktrees at once** — the dummy's test database is SQLite `:memory:`, so nothing is shared between processes | +| browser suite | `bundle exec rspec spec/system` (`CAPYBARA_SERVER=puma\|falcon`) | needs Playwright and a free port; **NOT safe in parallel** — it binds a server port, and `TRANSPORT=pgbus` additionally uses one named local database (`phlex_reactive_pgbus_test`) after `rake pgbus:prepare_test_db` | +| every server x transport | `bundle exec rake spec:system_matrix` | skips the pgbus cells with a note when `pg_isready` fails | +| JS suite | `bun test spec/javascript` | one bun process for all 52 files | +| lint | `bundle exec rubocop` (`-A` to autocorrect) | all new cops on | +| one CI cell locally | `CAPYBARA_SERVER=falcon TRANSPORT=cable bundle exec rspec spec/system` | mirrors one `system` matrix cell | +| docs build / check | `cd docs && bundle exec rake lint && bundle exec rspec` | **`rake lint`, never bare `rubocop`** — the gem's ancestor `.rubocop.yml` excludes `docs/**/*`, so a bare run inspects zero files | +| client rebuild | `rake build:js`, then `rake build:js_check` | needs bun | +| run the app | `cd docs && bin/dev` (the demo site) or `rake dummy:server` (the dummy, PORT=3010) | | +| benchmarks | `rake bench` (micro), `rake bench:request` (derailed), `rake bench:client` (bun) | | +| release | `bin/release [patch\|minor\|major\|X.Y.Z] [-n]` | `list` and `--dry-run` are read-only | + +## Branches and PRs + +- Default branch: `main` +- Work branches: `issue-<n>-<slug>` when the work has an issue (the dominant pattern in merged history), otherwise `feature/*`, `fix/*`, `refactor/*`, `ci/*`, `chore/*`. All rooted off fresh `origin/main`. `.claude/rules/git-workflow.md` lists only the prefixed forms +- Commits: conventional (`feat:`, `fix:`, `refactor:`, `perf:`, `docs:`, `test:`, `chore:`, `ci:`); the body says WHY. Scopes map to the architecture: `streamable`, `component`, `controller`, `client`, `engine`, `docs`, `ci` +- PR body sections, in order: Summary, Test plan / Test coverage, **Deviations & judgment calls**, Gate. The deviations section is mandatory — it is where the judgment calls made mid-implementation are audited, and "the plan held completely" is a valid body for it +- Write the PR body to a file and pass `--body-file`; a single-quoted heredoc passes backticks through verbatim, so never escape them +- Merge policy: squash on `main` after green and approval. Never force-push a published branch — merge `main` forward into it +- Attribution: **no** `Co-Authored-By: Claude`, no "Generated with" line. Add only the `Claude-Session:` trailer the session's own instructions specify +- Releases land DIRECTLY on `main` via `bin/release`, not through a PR + +## Layers + +| Layer | Files | Edit rule | +|---|---|---| +| Client runtime | `app/javascript/phlex/reactive/{reactive_controller,confirm,confirm_predicate,compute,inspect}.js` | owned here — but a source edit is a THREE-file change (source, rebuilt `.min.js` + `.map`, re-synced vendored twin) | +| Generated client | the same directory's `*.min.js` and `*.min.js.map` | **generated — never hand-edit or hand-merge.** Fix the source and `rake build:js` | +| Vendored twins | the five files in `spec/phlex/vendored_controller_sync_spec.rb`'s map under `spec/dummy/public/vendor/` | generated — `cp` from the rebuilt `.min.js`. Everything ELSE in that directory (stimulus, turbo, trix, lexxy, the dummy's reducers) is hand-written or third-party: merge it like source | +| Endpoint | `app/controllers/phlex/reactive/actions_controller.rb` | owned here; the only controller in the gem | +| Component mixin | `lib/phlex/reactive/component.rb` + `component/{dsl,helpers,identity,lazy,registry}.rb`, `client_bindings.rb` | owned here | +| Streaming | `lib/phlex/reactive/{streamable,collections}.rb` | owned here | +| Reply surface | `lib/phlex/reactive/{response,reply,stream}.rb` | owned here | +| Async | `lib/phlex/reactive/{defer,deferred_render_job,pending,settle,settles}.rb` | owned here | +| Core + engine | `lib/phlex/reactive.rb`, `engine.rb` | owned here | +| Tooling | `doctor.rb`, `inspector*`, `mcp/`, `apm/`, `lib/generators/`, `lib/tasks/` | owned here | +| Docs site | `docs/` | owned here, but a **separate app** — run its commands from inside `docs/`, with its own bundle | +| Lockfiles | `Gemfile.lock`, `docs/Gemfile.lock`, `bun.lock`, `docs/bun.lock` | generated — never hand-merge; take the base's and re-run the installer | + +## Shapes + +Check a change against these before calling it done; a reviewer will name the one you forgot. + +- **pgbus absent, pgbus present but < 0.9.2, pgbus current.** Every pgbus feature needs the capability-probe gate AND its fallback asserted. +- **Both real servers.** Puma (threads) and Falcon (fibers) — `Thread.current` is fiber-local, so anything using the `with_*` request-state pairs must be proven under both. +- **Both transports.** Action Cable and Postgres SSE. +- **A record-backed component AND a state-backed one**, plus the draft case: an unsaved or nil record mints a token with no `gid`. +- **A collection delta at its boundaries**: 0→1 and 1→0 for the empty state, plus a size resolver that is nil. +- **A row identified by a record AND by a bare dom-id String** — every removal path accepts both. +- **An action with no params, with declared params, and with an undeclared/misspelled key** (which must be dropped, never fabricated). +- **`verbose_errors` on and off** — the diagnostic branches early-return when it is off. +- **A client-only component** (`include ClientBindings` alone, no `#id`, no token) as well as a full reactive one. +- **An op mixed onto the reactive ROOT**, not only onto a descendant. +- **A reply that does not re-render self** — `streams`, `with`, the collection verbs, `pending` — which still has to roll the token forward. +- **Ruby 3.4 and 4.0**, the two CI cells. + +## Constraints + +Suggestions that are wrong in this repository. Push back on sight. + +| Suggestion | Why it is wrong here | +|---|---| +| "Hand-edit the `.min.js` / the vendored copy to match" | They are build outputs. Fix the source, `rake build:js`, re-sync. `build:js_check` and the sync spec exist to catch the hand edit | +| "Add pgbus (or an APM SDK, or `mcp`) to the gemspec" | Optionality is a core invariant. Every one of these is runtime-probed and degrades | +| "Gate on `defined?(::Pgbus)` / a version string" | pgbus < 0.9.2 also defines `::Pgbus`. Probe the keyword you are about to pass | +| "Wire the morph listener unconditionally so a later-introduced trigger is gated" | Every sibling gate decides once at `connect()`; see `review/client-runtime.md` | +| "Extend the legacy flat show-attribute arm with the new predicates" | It is scheduled for deletion and no Ruby version ever emitted those attributes; see `review/client-runtime.md` | +| "Restore the original `window.confirm` in the bun teardown" | There is no pristine value to capture and it would break the lazy delegation the next file needs; see `review/testing.md` | +| "Assert exactly two lockfile pins in the release task" | A lockfile with no CHECKSUMS section legitimately has one; see `review/release-and-changelog.md` | +| "Use `bundle lock --local` to bump the release pin" | A full re-resolve trips over constraints unrelated to this gem | +| "Run bare `rubocop` in `docs/`" | The ancestor config excludes `docs/**/*`; it inspects zero files and passes vacuously | +| "Use Phlex's `dom_id` helper in `#id`" | `#id` runs BEFORE render; use `Streamable#dom_id`, which delegates to `ActionView::RecordIdentifier` | +| "Include `Turbo::Streams::ActionHelper`" | It pulls in `ActionView::Helpers::TagHelper`, which overrides Phlex's internal `tag` and breaks rendering | +| "Pass `exclude:` straight to `Turbo::StreamsChannel`" | turbo-rails swallows unknown kwargs into its render locals, silently dropping the actor-echo suppression | +| "Claim a speedup without a measured baseline" | Any hot-path change ships with a same-machine before/after from `rake bench` | + +## Docs + +- User-facing docs live in `docs/app/views/docs/pages/`; a page is routed and in the nav ONLY through its entry in `docs/app/models/doc.rb`. The behaviour-to-page map is in [`docs-site/summary.md`](docs-site/summary.md) +- The README is the long-form twin and repeats most of that content; grep it for any sentence you change on a page +- Changelog: `CHANGELOG.md`, under `## [Unreleased]`, `### Added` before `### Fixed`, one block per section — merge into the existing block, never append a second +- A change to a setting, a verb, a wire attribute or a client op always updates its docs page AND the README AND the CHANGELOG in the same PR +- Files that pin a version and drift after a release: `Gemfile.lock` and `docs/Gemfile.lock` (both pin `phlex-reactive (X.Y.Z)` in two places). `rake release` rewrites both; after any manual `VERSION` move, re-run it or edit the pins the same way + +## CI + +- Workflows: `main.yml` (push to `main` + every PR — jobs `lint`, `test`, `bench`, `site`, `system`), `release.yml` (`release: published` → RubyGems trusted publishing), `deploy-docs.yml` (`release: published` → the docs site via `zoolutions/docs-kit`'s reusable deploy) +- Matrix: `test` on Ruby 3.4 and 4.0; `site` on puma and falcon; `system` on server (puma/falcon) x transport (cable/pgbus) = 4 cells. `fail-fast: false` on all three matrix jobs +- Cells that differ from local: the pgbus cells get a `postgres:18` service and run `rake pgbus:prepare_test_db` first; `bun test spec/javascript` runs only on the cable cells; `rake build:js_check` runs only on puma+cable; the `site` job runs with `BUNDLE_FROZEN: "false"` +- Fetch a failure: `gh run view --job <id> --log-failed`; the `system` job also uploads `capybara-screenshots-<server>-<transport>` on failure +- "Green" means every cell of all five jobs **except `bench`**, which is run-and-report and uploads an artifact — it is never a hard fail +- Shared or rate-limited services: none in CI — each pgbus cell gets its own `postgres:18` service container, so PRs need not run one at a time. **Locally** the pgbus cells share one named database, so do not run two of them at once + +## Flake sources + +- **The browser suite's async morph.** A snapshot assertion taken immediately after a click races the round trip. Use the waiting matchers (`have_css(..., text:)`, `have_field(with:)`, `wait_for_reactive`) as the barrier — never `sleep`. +- **Falcon's fiber-per-request model** against anything caching a view context or holding request state. The per-thread view-context cache and the `with_*` save/restore pairs are the answer; a new cache that is per-process rather than per-thread shows up here first. +- **pgbus SSE timing** — the broadcast-before-subscribe race is closed by `since-id="0"` on a fresh one-shot key plus `durable: true`. A "the shimmer never resolved" flake is usually a key or durability regression, not the network. +- **Shared process state in the bun suite** — a global left installed by an earlier file (see `review/testing.md`). +- **A shared ActiveJob adapter** left switched by an earlier example. +- **Playwright browser install / port binding** in CI, which is environmental, not a code flake. + +## Conflicts + +| File | Rule | +|---|---| +| `app/javascript/phlex/reactive/*.min.js`, `*.min.js.map` | never hand-merge. Resolve the SOURCE `.js` semantically, then `rake build:js` | +| the five vendored twins under `spec/dummy/public/vendor/` | never hand-merge. `cp` each rebuilt `.min.js` over its twin. The rest of that directory is ordinary source | +| `CHANGELOG.md` | union under `## [Unreleased]` — keep BOTH sides' bullets, most recent first, WITHOUT duplicating the `### Added` / `### Fixed` subheads | +| `lib/phlex/reactive/version.rb` | releases land directly on `main`, so an ordinary feature branch never edits this. A conflict means the branch bumped it deliberately (a release-prep PR) — keep the branch's bump. If the intent is not obvious from the branch's own commits, stop and ask | +| `Gemfile.lock`, `docs/Gemfile.lock` | take the base's file, then `bundle install` in the gem root or in `docs/`. Never hand-edit a lockfile | +| `bun.lock`, `docs/bun.lock` | take the base's, then `bun install` in that directory | +| `docs/app/models/doc.rb` | append-only, base order first — each side's page entries must all survive | +| `spec/fixtures/show_predicate_vectors.json` | union the vectors; both sides' cases are parity proofs | +| fixtures / dummy components | add a second component rather than merging two shapes into one | + +After resolving, run the gates scoped to what the conflict touched, BEFORE pushing the merge: `bundle exec rubocop`, `bundle exec rspec spec/phlex spec/requests`, plus `bun test spec/javascript && rake build:js_check` if client artifacts were involved and `cd docs && bundle exec rake lint && bundle exec rspec` if `docs/` was. + +## Verification + +- The manual check a user of this change would do: `cd docs && bin/dev`, open the page that demonstrates the behaviour, and drive it in a browser with a second tab open to see the broadcast half. For a CLI-shaped change, `bin/rails phlex_reactive:doctor` in the dummy or the docs app +- Stress iterations for a flake proof: **50** runs of the single example under the cell that showed it (both server values when the suspicion is concurrency) +- A hot-path change ships a same-machine before/after from `rake bench` — throughput AND allocations, and say whether the win is method-level or request-level +- Where evidence goes: `lode/tmp/` (git-ignored) unless the PR needs an auditable trail, in which case it goes in the PR body + +Related: [`lode-map.md`](lode-map.md), [`practices.md`](practices.md), [`../.claude/rules/`](../.claude/rules/). From 0de2edbcc88b273f3dbfcc27668e9bf64270cede Mon Sep 17 00:00:00 2001 From: mhenrixon <mikael@mhenrixon.com> Date: Mon, 14 Sep 2026 01:25:24 +0200 Subject: [PATCH 2/3] =?UTF-8?q?docs(lode):=20gate=20findings=20=E2=80=94?= =?UTF-8?q?=20the=20authoring=20guide=20and=20the=20route-file=20merge=20r?= =?UTF-8?q?ule?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit .claude/README.md and SKILL_TEMPLATE.md still taught the retired commands and pointed at a deleted tdd.md; they now name the four local commands and the plugin's entry points. The retired github-review-pr's append-only rule for route files (spec/dummy/config/routes.rb) is restored beside doc.rb in the Conflicts table. Claude-Session: https://claude.ai/code/session_01Hga3idYBp7fWdj7GmmfofF --- .claude/README.md | 10 +++++----- .claude/SKILL_TEMPLATE.md | 2 +- lode/workflow.md | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.claude/README.md b/.claude/README.md index 588d7c76..6bcf3779 100644 --- a/.claude/README.md +++ b/.claude/README.md @@ -14,7 +14,7 @@ the whole team (and every autonomous session) shares the same conventions. ## Anatomy of a command A command is a markdown file under `commands/` with a YAML frontmatter block -followed by the prompt body. `.claude/commands/tdd.md` is a good reference. +followed by the prompt body. `.claude/commands/review-pr.md` is a good reference. ```markdown --- @@ -47,15 +47,15 @@ latest model in that tier, so a command never goes stale on an outdated pin. | Tier | Use for | Commands here | |------|---------|---------------| | `haiku` | Mechanical / config work, diff pattern-scanning | *(none yet)* | -| `sonnet` | Prescriptive, pattern-following passes with a tight prompt | `/github-review-comments`, `/github-review-failures` | -| `opus` | Orchestration, security, review synthesis, and reasoning-heavy specialists | `/lfg`, `/architect`, `/security`, `/review-pr`, `/github-review-pr`, `/tdd`, `/perf` | -| `fable` | Read-only planning that hands execution to cheaper models | `/plan` | +| `sonnet` | Prescriptive, pattern-following passes with a tight prompt | *(none local — `/lode:review-pr` covers this)* | +| `opus` | Orchestration, security, review synthesis, and reasoning-heavy specialists | `/architect`, `/security`, `/review-pr`, `/perf` | +| `fable` | Read-only planning that hands execution to cheaper models | *(none local — `/lode:plan` is the planning entry point; pick `fable` per session with `/model`)* | Rules of thumb: - **Always use the alias**, never `claude-opus-4-8` or another full model ID — aliases track the latest model per tier and never rot. -- **`fable` is pinned only on `/plan`.** For a plain interactive session, pick it +- **No local command pins `fable`.** The workflow commands (`/lode:lfg`, `/lode:plan`, `/lode:tdd`, `/lode:review-pr`, `/lode:finish-prs`, `/lode:debug-flaky`) come from the `lode@zoolutions` plugin and read `lode/workflow.md`; only `/architect`, `/security`, `/review-pr` and `/perf` live here. For a plain interactive session, pick `fable` per-session with `/model` when you want the most capable model for architecture or the hardest debugging. - **Subagents don't inherit the tier for free.** When a command (or you) spawns a diff --git a/.claude/SKILL_TEMPLATE.md b/.claude/SKILL_TEMPLATE.md index 67b11b38..7ece601d 100644 --- a/.claude/SKILL_TEMPLATE.md +++ b/.claude/SKILL_TEMPLATE.md @@ -10,7 +10,7 @@ work, `sonnet` for prescriptive pattern-following passes, `opus` for orchestration, security, review synthesis, and reasoning-heavy specialists. Always use the tier alias, never a full model ID — aliases track the latest model in the tier. Pin `fable` only on read-only planning commands that hand execution -to cheaper models (see `/plan`); otherwise choose it per-session with `/model`. +to cheaper models (the plugin's `/lode:plan` is the example); otherwise choose it per-session with `/model`. ```markdown --- diff --git a/lode/workflow.md b/lode/workflow.md index 72f8f439..79f0ce99 100644 --- a/lode/workflow.md +++ b/lode/workflow.md @@ -120,7 +120,7 @@ Suggestions that are wrong in this repository. Push back on sight. | `lib/phlex/reactive/version.rb` | releases land directly on `main`, so an ordinary feature branch never edits this. A conflict means the branch bumped it deliberately (a release-prep PR) — keep the branch's bump. If the intent is not obvious from the branch's own commits, stop and ask | | `Gemfile.lock`, `docs/Gemfile.lock` | take the base's file, then `bundle install` in the gem root or in `docs/`. Never hand-edit a lockfile | | `bun.lock`, `docs/bun.lock` | take the base's, then `bun install` in that directory | -| `docs/app/models/doc.rb` | append-only, base order first — each side's page entries must all survive | +| `docs/app/models/doc.rb`, `spec/dummy/config/routes.rb` and any other route file | append-only registries, base order first — each side's entries must all survive | | `spec/fixtures/show_predicate_vectors.json` | union the vectors; both sides' cases are parity proofs | | fixtures / dummy components | add a second component rather than merging two shapes into one | From 99f8487403f655a1f54b6a693cea7034b4864a49 Mon Sep 17 00:00:00 2001 From: mhenrixon <mikael@mhenrixon.com> Date: Mon, 14 Sep 2026 01:25:36 +0200 Subject: [PATCH 3/3] docs(lode): the README tree line named the retired commands too Claude-Session: https://claude.ai/code/session_01Hga3idYBp7fWdj7GmmfofF --- .claude/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/README.md b/.claude/README.md index 6bcf3779..693575d9 100644 --- a/.claude/README.md +++ b/.claude/README.md @@ -5,7 +5,7 @@ the whole team (and every autonomous session) shares the same conventions. ``` .claude/ -├── commands/ Slash commands (/lfg, /tdd, /plan, /security, …) — one markdown file each +├── commands/ Local slash commands (/architect, /security, /review-pr, /perf) — one markdown file each; the workflow commands are /lode:* from the plugin ├── rules/ Standing rules auto-loaded into context (coding-style, testing, performance, git-workflow, agents) ├── README.md This file — how to author a command └── SKILL_TEMPLATE.md Copy-paste starting point for a new command