docs(agents): add KO/ViewComponent guidelines from review

[brianmhunt]

Summary

Ports architectural review guidelines from a downstream MinuteBox CodeRabbit review config (NetPleadings/MinuteBox#9518) back upstream into TKO's public LLM-facing docs. Rules reworded for TKO primitives (LifeCycle, BindingHandler, ComponentABC, ko.subscribable) — no app-specific references.

Skipped MB rules 4 (method/file size), 5 (empty catch), and style nits — not framework-level.

Credit

Guidelines originate from NetPleadings/MinuteBox#9518 by @jameskozlowskimb with input from @ctcarton. Ported upstream and reworded for TKO primitives.

Changes

tko.io/public/llms.txt

3 new Gotchas bullets: flat JSX attrs vs params={{…}} wrapper, no observable/computed/subscribe inside a computed evaluator, || undefined for binary HTML attrs. Caveman-compressed. New /agents/process.md pointer added to agent-docs list.

tko.io/public/agents/contract.md

• DOM Mutation Containment — DOM APIs belong inside BindingHandler subclasses; explicit violation list + safe zones.
• Component Design: Binding Handlers and Component View Models — narrow BH scope (one DOM task), component-as-renderer-only.
• Component Communication via ko.subscribable — pub/sub over observable-of-function for imperative commands between components and binding handlers.

tko.io/public/agents/guide.md

• Observables vs computed dependency management — this.computed / this.subscribe preference in LifeCycle subclasses; never create reactive primitives inside a computed evaluator.
• Reactive styling — two example patterns (computed className, data-attr + CSS selector), not a mandate; codebases should pick one and stay consistent.
• JSX scope rule — JSX inside components / top-level tko.jsx.render only, not orphan utility modules.
• Component params in JSX — each param as its own attribute.
• 3 new Gotchas bullets: null (not false) for empty HTML computeds; binary attr || undefined; prefer named computed variables over inline.

tko.io/public/agents/process.md (new)

• Never ship docs that reference things that don't exist on the target branch — mandatory pre-staging checklist.
• Adversarial review is mandatory — how-to, in-scope/out-of-scope list, audit-trail format.
• Credits section naming the MinuteBox PR.

AGENTS.md

• One-line mandate + link for "Never ship broken references" (full rule in process.md).
• One-paragraph mandate for adversarial review (in-scope trigger, dark-factory motivation, audit-trail pointer); full how-to in process.md.
• Failure-modes list gains a bullet for docs referencing non-existent packages/APIs/spec paths.
• Net line delta vs main: ~+7 (down from ~+55 before the extract).

tko.io/public/agents/verified-behaviors/*.md

Left untouched. These files are regenerated from packages/*/verified-behaviors.json on every build, so hand-edits would have been wiped at CI time.

Verification

• bun run build in tko.io/: 65 pages, clean build.
• All CI checks passing on latest commit.
• New sections confirmed present in dist/ output.

[coderabbitai]

Warning

Rate limit exceeded

@brianmhunt has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 35 minutes and 48 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 35 minutes and 48 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 861fffa3-eaac-48cc-a116-3b8089bf2e05

📥 Commits

Reviewing files that changed from the base of the PR and between 4adc87d and 5f9ad79.

📒 Files selected for processing (4)

• AGENTS.md
• tko.io/public/agents/guide.md
• tko.io/public/agents/process.md
• tko.io/public/llms.txt

📝 Walkthrough

Walkthrough

This PR adds and updates documentation to establish stricter architectural guidance around DOM mutation containment, binding handler responsibilities, component design patterns, JSX usage rules, and lifecycle management in Knockout applications. It also introduces new agent process verification requirements and documentation quality standards.

Changes

Cohort / File(s)	Summary
Binding & Component Architecture Guidance tko.io/public/agents/contract.md, tko.io/public/agents/guide.md	Introduces new sections defining DOM mutation containment rules (mutations only in BindingHandler), binding handler design constraints, component view model patterns, component communication via subscribable, JSX attribute rules, and lifecycle-aware observable/computed/subscription creation guidelines.
Verified Behaviors Documentation tko.io/public/agents/verified-behaviors/computed.md, tko.io/public/agents/verified-behaviors/lifecycle.md, tko.io/public/agents/verified-behaviors/utils-jsx.md	Adds anti-patterns section warning against creating observables/computeds/subscriptions inside computed evaluators, new usage guidance for LifeCycle subclasses, and explicit behavior for binary HTML attributes (omit when null/undefined/false).
Documentation Standards & Index tko.io/public/llms.txt, AGENTS.md	Updates documentation index descriptions and adds new agent process rules: mandatory doc verification (trackedness, package/spec path existence), mandatory adversarial review with failure-mode enumeration, and audit trail requirements in PR descriptions.
Configuration .gitignore	Adds *.original.md to ignored files.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Revert AI governance framework (PR #293) #304: Modifies AGENTS.md governance rules and documentation standards, creating potential intersection with this PR's governance updates.
• Add interactive examples showcase #291: Updates the same documentation surfaces (agents/contract.md, agent guide) and extends binding/component architectural guidance.

Poem

🐰 Hop along the binding path so fine,
Where DOM lives in containment's line,
No leaks in computed, clear lifecycle's call,
Architecture guides us, protecting us all! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The PR title directly describes the main change: adding KO/ViewComponent architectural guidelines ported from a MinuteBox review into TKO's public documentation.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch docs/llm-guidance-mb-imports

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d7d76d4e8d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Correct binary-attribute false/null semantics

This gotcha states that binary attributes are omitted when the value is null or false, but packages/utils.jsx/src/JsxObserver.ts removes attributes only for undefined and otherwise stringifies the value via setAttributeNS, so false/null become present attributes ("false"/"null"). For boolean HTML attrs like disabled, presence keeps the attribute active, so code generated from this guidance can leave controls disabled when the computed returns false; please align this rule (and the copied wording in verified-behaviors/utils-jsx.md) with actual runtime behavior.

Useful? React with 👍 / 👎.

[Copilot]

Pull request overview

Ports Knockout/ViewComponent architectural guidance into TKO’s public LLM-facing docs, adding new “gotchas” and expanding agent documentation and “verified behaviors” references to align with TKO primitives (LifeCycle, BindingHandler, ComponentABC, ko.subscribable).

Changes:

• Add new JSX/computed/HTML attribute gotchas to llms.txt and the main agent guide.
• Add contract guidance for DOM mutation containment, component/BH responsibilities, and component communication via ko.subscribable.
• Extend “verified behaviors” docs (including a new @tko/debug stub) with additional lifecycle/computed/jsx guidance.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tko.io/public/llms.txt	Adds new gotchas for JSX params flattening, computed anti-patterns, and binary HTML attributes.
tko.io/public/agents/verified-behaviors/utils-jsx.md	Documents JSX binary HTML attribute behavior (needs correction to match implementation).
tko.io/public/agents/verified-behaviors/lifecycle.md	Adds lifecycle “usage guidance” section (currently not aligned with “verified behaviors” contract).
tko.io/public/agents/verified-behaviors/debug.md	Adds generated no-tests-found stub for @tko/debug.
tko.io/public/agents/verified-behaviors/computed.md	Adds an “Anti-patterns” section (currently not test-backed per the directory’s stated contract).
tko.io/public/agents/guide.md	Adds extended guidance on lifecycle-owned reactivity, JSX usage, styling via attributes, component params, and new gotchas (includes binary-attr wording that needs correction).
tko.io/public/agents/contract.md	Adds architectural contract rules around DOM mutation containment and component/BH communication patterns.

[Copilot]

This new "Anti-patterns" entry is in the "verified-behaviors" directory, but it doesn't cite a spec that actually verifies the described leak, and it includes a "test sketch" rather than a real test reference. Given the file header says non-test-backed behaviors do not belong here, either add/point to a concrete spec that covers this behavior (and cite it as "Specs:"), or move this guidance to /agents/guide.md (and keep this file strictly test-backed).

Suggested change

	## Anti-patterns
	- Creating observables, computeds, or subscriptions **inside** a computed's evaluator leaks instances. The evaluator re-runs on every dependency change, producing a new un-disposed subscriber each time, so memory and subscriber count grow without bound.
	Test sketch:
	```ts
	const dep = ko.observable(0)
	const instances: KnockoutObservable<number>[] = []
	const leaky = ko.computed(() => {
	const obs = ko.observable(dep() + 1)
	instances.push(obs)
	return obs()
	})
	expect(instances.length).toBe(1)
	dep(1); expect(instances.length).toBe(2)
	dep(2); expect(instances.length).toBe(3)
	```
	Fix: create the observable/subscription once outside the computed, or inside a `LifeCycle` subclass constructor where `this.subscribe` / `this.computed` own disposal.
	Related specs: `packages/lifecycle/spec/LifeCycleBehaviors.ts` (disposal semantics).

[Copilot]

The "Usage guidance" section adds non-test-backed recommendations to a "verified-behaviors" doc whose header states that behaviors not covered by unit tests do not belong here. Please either (a) move this guidance into /agents/guide.md, or (b) explicitly connect each bullet to an existing spec (using the same "Specs:" convention as the rest of the file) so readers can verify the claim from tests.

Suggested change

	## Usage guidance
	- Prefer `this.computed(...)` over standalone `ko.computed(...)` inside a `LifeCycle` subclass — the computed is added to the instance's disposal set automatically.
	- Prefer `this.subscribe(observable, callback)` over `observable.subscribe(callback)` inside a `LifeCycle` subclass for the same reason.
	- Do not create observables, computeds, or subscriptions inside a computed's evaluator — see the anti-pattern in `computed.md`.

[Copilot]

The claim that binary HTML attributes are omitted when the value is null or false doesn't match the current JSX implementation: JsxObserver.setNodeAttribute only removes attributes when the unwrapped value is undefined; null/false are stringified and set (e.g., disabled="false" still leaves the attribute present). Please update this bullet to reflect that omission only happens via undefined, and keep || undefined (or an explicit conditional) as the recommended way to remove the attribute.

Suggested change

	- Binary HTML attributes (`disabled`, `readonly`, `hidden`, `required`, `checked`, `selected`) omit the attribute when the observable/computed value is `null`, `undefined`, or `false`; any other value (including the string `"false"`) sets the attribute. Use `|| undefined` in computeds to express "no attribute" explicitly.
	- Binary HTML attributes (`disabled`, `readonly`, `hidden`, `required`, `checked`, `selected`) omit the attribute only when the observable/computed value is `undefined`; `null` and `false` are stringified and set, so even `disabled="false"` still leaves the attribute present. Use `|| undefined` in computeds, or an explicit conditional, to express "no attribute" explicitly.

[Copilot]

This Gotcha says binary HTML attributes are omitted when the value is null, undefined, or false, but JSX attribute setting currently only removes attributes when the value is undefined (other values, including null/false, are stringified and set). Please revise the wording to avoid implying that null/false automatically omit the attribute, and keep the guidance focused on returning undefined (e.g., via || undefined) to ensure removal.

Suggested change

- **Binary HTML attributes** (`disabled`, `readonly`, `hidden`, `required`, `checked`, `selected`) omit the attribute when the value is `null`, `undefined`, or `false`. Use `|| undefined` in computeds to make "no attribute" explicit: `disabled={this.computed(() => shouldBeDisabled() || undefined)}`. Never return the string `"false"` — it keeps the attribute set.

- **Binary HTML attributes** (`disabled`, `readonly`, `hidden`, `required`, `checked`, `selected`) should return `undefined` when you want JSX to remove the attribute. Use `|| undefined` in computeds to make "no attribute" explicit: `disabled={this.computed(() => shouldBeDisabled() || undefined)}`. Do not rely on `null` or `false` to omit the attribute, and never return the string `"false"` — it keeps the attribute set.

[Copilot]

This section states that creating ko.observable() inside a computed evaluator causes instances to pile up and never dispose. In @tko/computed, dependency tracking disposes subscriptions to prior dependencies on each evaluation, so creating an observable inside an evaluator doesn't inherently leak unless you retain references or create unmanaged subscriptions/computeds. Please narrow this guidance to the cases that actually leak (e.g., creating subscriptions or computeds inside the evaluator) or reword it as a performance/clarity anti-pattern rather than an unbounded leak for plain observables.

Suggested change

- **Never** create `ko.observable()`, `ko.computed()`, or call `.subscribe()` inside a computed's evaluator. The evaluator runs on every dependency change, so new observables/subscriptions pile up and never dispose — memory and CPU grow unbounded. Create them once outside the computed, or in a `LifeCycle`-enabled constructor.

- Avoid creating reactive objects inside a computed's evaluator. Creating `ko.computed()` instances or calling `.subscribe()` there can accumulate unmanaged work unless you dispose them manually, because the evaluator runs on every dependency change. Creating a plain `ko.observable()` there does not inherently leak, but it is usually a performance/clarity anti-pattern unless it is purely temporary and you do not retain references to it. Prefer creating observables, computeds, and subscriptions once outside the evaluator, or in a `LifeCycle`-enabled constructor.

[Copilot]

This Gotcha asserts that creating ko.observable() inside a computed evaluator leaks instances. @tko/computed disposes prior dependency subscriptions on each evaluation, so plain observables created inside the evaluator don't necessarily accumulate unless references are retained; the leak risk is much clearer for creating subscriptions/computeds inside the evaluator. Please adjust the wording to avoid stating a guaranteed leak for ko.observable() and focus on the cases that actually leak or on the performance implications.

Suggested change

	- Never create `ko.observable()`, `ko.computed()`, or `.subscribe()` inside a computed evaluator — re-runs leak instances
	- Avoid creating `ko.observable()`, `ko.computed()`, or `.subscribe()` inside a computed evaluator — `ko.computed()` and `.subscribe()` can accumulate unless disposed, while `ko.observable()` usually just creates unnecessary transient instances unless references are retained

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tko.io/public/agents/contract.md (1)

76-76: Optional: Consider simplifying "because" clauses for conciseness.

Static analysis suggests these phrases could be more concise:

• Line 76: "a handler that is complex because its single DOM task is inherently complex" → "a handler whose single DOM task is inherently complex"
• Line 89: "a component that is large because it composes many small child components" → "a component that composes many small child components"

Also applies to: 89-89

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tko.io/public/agents/contract.md` at line 76, Replace verbose "because"
clauses with shorter relative phrasing in the descriptive lines: change the
phrase "a handler that is complex because its single DOM task is inherently
complex" (in contract.md) to "a handler whose single DOM task is inherently
complex" and similarly change "a component that is large because it composes
many small child components" to "a component that composes many small child
components" to improve conciseness while preserving meaning.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tko.io/public/agents/verified-behaviors/utils-jsx.md`:
- Around line 33-34: The docs say binary attributes (disabled, readonly, hidden,
required, checked, selected) should be omitted for null/undefined/false, but
JsxObserver currently only omits on undefined; update the attribute-update logic
in JsxObserver.ts (the function handling setAttributeNS/attribute writes around
the node.setAttributeNS(ns, name, String(value)) calls) to treat null and false
the same as undefined by normalizing value => undefined for these specific
attribute names before calling setAttributeNS or removeAttribute, add unit tests
in packages/utils.jsx/spec/jsxBehaviors.ts to cover null and false cases, and
regenerate packages/utils.jsx/verified-behaviors.json so the verified behaviors
match the new behavior.

---

Nitpick comments:
In `@tko.io/public/agents/contract.md`:
- Line 76: Replace verbose "because" clauses with shorter relative phrasing in
the descriptive lines: change the phrase "a handler that is complex because its
single DOM task is inherently complex" (in contract.md) to "a handler whose
single DOM task is inherently complex" and similarly change "a component that is
large because it composes many small child components" to "a component that
composes many small child components" to improve conciseness while preserving
meaning.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3e8f188c-074b-44b5-b9ab-ff5eab20946a

📥 Commits

Reviewing files that changed from the base of the PR and between 6629d63 and d7d76d4.

📒 Files selected for processing (7)

• tko.io/public/agents/contract.md
• tko.io/public/agents/guide.md
• tko.io/public/agents/verified-behaviors/computed.md
• tko.io/public/agents/verified-behaviors/debug.md
• tko.io/public/agents/verified-behaviors/lifecycle.md
• tko.io/public/agents/verified-behaviors/utils-jsx.md
• tko.io/public/llms.txt

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify binary HTML attribute rendering behavior with null/false values

echo "=== Searching for binary attribute preprocessing logic ==="
rg -n -C5 'disabled|readonly|hidden|required|checked|selected' packages/utils.jsx/src/ --type ts

echo ""
echo "=== Searching for null/false to undefined conversion ==="
rg -n -C5 'null.*undefined|false.*undefined' packages/utils.jsx/src/JsxObserver.ts

echo ""
echo "=== Looking for attribute value normalization ==="
ast-grep --pattern $'function $FUNC($$$) {
  $$$
  if ($VALUE === null || $VALUE === false) {
    $$$
  }
  $$$
}'

Repository: knockout/tko

Length of output: 536

---

🏁 Script executed:

# Find and examine JsxObserver.ts file
find . -name "JsxObserver.ts" -type f

Repository: knockout/tko

Length of output: 95

---

🏁 Script executed:

# Read the utils.jsx spec file to understand expected behavior
find . -path "*/packages/utils.jsx/spec/jsxBehaviors.ts" -type f

Repository: knockout/tko

Length of output: 97

---

🏁 Script executed:

# Read the documentation file being reviewed
find . -path "*/tko.io/public/agents/verified-behaviors/utils-jsx.md" -type f

Repository: knockout/tko

Length of output: 110

---

🏁 Script executed:

# Check sizes of the files
wc -l ./packages/utils.jsx/src/JsxObserver.ts ./packages/utils.jsx/spec/jsxBehaviors.ts ./tko.io/public/agents/verified-behaviors/utils-jsx.md

Repository: knockout/tko

Length of output: 222

---

🏁 Script executed:

# Read JsxObserver.ts around lines 398-408
sed -n '390,420p' ./packages/utils.jsx/src/JsxObserver.ts

Repository: knockout/tko

Length of output: 1120

---

🏁 Script executed:

# Read the spec file
cat ./packages/utils.jsx/spec/jsxBehaviors.ts

Repository: knockout/tko

Length of output: 37565

---

🏁 Script executed:

# Read the documentation file
cat ./tko.io/public/agents/verified-behaviors/utils-jsx.md

Repository: knockout/tko

Length of output: 2236

---

🏁 Script executed:

# Search for binary attribute tests in the spec file
grep -n "disabled\|readonly\|hidden\|required\|checked\|selected" ./packages/utils.jsx/spec/jsxBehaviors.ts

Repository: knockout/tko

Length of output: 38

---

🏁 Script executed:

# Search for tests that check attribute removal with null/false
grep -n -B3 -A3 "null\|false" ./packages/utils.jsx/spec/jsxBehaviors.ts | grep -A5 -B5 "attribute"

Repository: knockout/tko

Length of output: 1683

---

🏁 Script executed:

# Search for where setNodeAttribute is called
grep -n "setNodeAttribute" ./packages/utils.jsx/src/JsxObserver.ts

Repository: knockout/tko

Length of output: 434

---

🏁 Script executed:

# Look for NativeProvider.addValueToNode to see if it does preprocessing
find . -name "*NativeProvider*" -type f

Repository: knockout/tko

Length of output: 163

---

🏁 Script executed:

# Read NativeProvider.ts to see if addValueToNode handles binary attributes
cat ./packages/provider.native/src/NativeProvider.ts

Repository: knockout/tko

Length of output: 2203

---

🏁 Script executed:

# Get more context around setNodeAttribute calls
sed -n '360,380p' ./packages/utils.jsx/src/JsxObserver.ts

Repository: knockout/tko

Length of output: 663

---

🏁 Script executed:

# Search for "binary" in the codebase to find relevant code
rg -n "binary" --type ts

Repository: knockout/tko

Length of output: 361

---

🏁 Script executed:

# Read the full JsxObserver.ts to understand the attribute handling flow
cat ./packages/utils.jsx/src/JsxObserver.ts

Repository: knockout/tko

Length of output: 13764

---

🏁 Script executed:

# Search spec file for tests on binary attributes like "disabled", "checked", etc.
grep -n "disabled\|checked\|readonly\|hidden\|required\|selected" ./packages/utils.jsx/spec/jsxBehaviors.ts

Repository: knockout/tko

Length of output: 38

---

🏁 Script executed:

# Search for any tests involving null or false on attributes
grep -n -B5 -A10 "attributes.*null\|attributes.*false" ./packages/utils.jsx/spec/jsxBehaviors.ts

Repository: knockout/tko

Length of output: 38

---

🏁 Script executed:

# Look for tests that verify attribute removal
grep -n -B3 -A3 "removeAttribute" ./packages/utils.jsx/spec/jsxBehaviors.ts

Repository: knockout/tko

Length of output: 38

---

🏁 Script executed:

# Let's verify the documented behavior by checking the verified-behaviors.json file
find . -name "verified-behaviors.json" -path "*/utils.jsx/*" -type f

Repository: knockout/tko

Length of output: 100

---

🏁 Script executed:

# Let's also test the actual behavior by creating a small test to see what happens
# Read any existing attribute-related test to understand how they test attribute behavior
grep -n "attributes:" ./packages/utils.jsx/spec/jsxBehaviors.ts | head -20

Repository: knockout/tko

Length of output: 1720

---

🏁 Script executed:

# Look for the test that verifies toggles observable attribute
grep -n -B2 -A8 "toggles an observable attribute" ./packages/utils.jsx/spec/jsxBehaviors.ts

Repository: knockout/tko

Length of output: 479

---

🏁 Script executed:

# Read the verified-behaviors.json file
cat ./packages/utils.jsx/verified-behaviors.json

Repository: knockout/tko

Length of output: 1564

---

🏁 Script executed:

# Check the test that toggles empty observable attribute (just before the one we found)
grep -n -B2 -A8 "toggles an empty observable attribute" ./packages/utils.jsx/spec/jsxBehaviors.ts

Repository: knockout/tko

Length of output: 469

---

Fix documented binary HTML attribute behavior or update documentation to match implementation.

The documentation states that binary HTML attributes (disabled, readonly, hidden, required, checked, selected) omit the attribute when the value is null, undefined, or false. However, the code in JsxObserver.ts:398-410 only removes attributes when value === undefined. For null and false values, the code calls node.setAttributeNS(ns, name, String(value)), which renders "null" and "false" as attribute values rather than omitting them.

Additionally, this behavior is not covered by any tests in packages/utils.jsx/spec/jsxBehaviors.ts and is not included in packages/utils.jsx/verified-behaviors.json (which is generated from unit tests). Either implement the documented behavior with proper preprocessing to convert null and false to undefined for binary attributes, or update the documentation to reflect the actual implementation that only omits attributes for undefined values.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tko.io/public/agents/verified-behaviors/utils-jsx.md` around lines 33 - 34,
The docs say binary attributes (disabled, readonly, hidden, required, checked,
selected) should be omitted for null/undefined/false, but JsxObserver currently
only omits on undefined; update the attribute-update logic in JsxObserver.ts
(the function handling setAttributeNS/attribute writes around the
node.setAttributeNS(ns, name, String(value)) calls) to treat null and false the
same as undefined by normalizing value => undefined for these specific attribute
names before calling setAttributeNS or removeAttribute, add unit tests in
packages/utils.jsx/spec/jsxBehaviors.ts to cover null and false cases, and
regenerate packages/utils.jsx/verified-behaviors.json so the verified behaviors
match the new behavior.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@AGENTS.md`:
- Line 191: The package-existence verification currently only runs "ls
packages/<name>" which misses valid public packages under builds/, so update the
rule to check both locations (packages/<name> and builds/<name>) or perform a
single lookup that searches both directories and verifies a matching
package.json; replace references to the lone check "ls packages/<name>" in the
doc/guideline text with the broadened check logic/description so docs no longer
flag legitimate packages like "@tko/build.knockout" as orphans.

In `@tko.io/public/agents/verified-behaviors/computed.md`:
- Line 4: The directory gate sentence in computed.md is ungrammatical; replace
"If behavior not covered by unit tests, not belong in this directory." with a
clear grammatical rule such as "Behaviors not covered by unit tests do not
belong in this directory." or "If a behavior is not covered by unit tests, it
does not belong in this directory." Update the line in computed.md accordingly
to use one of these corrected phrasings so the normative rule is clear and
grammatical.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a30784ec-5433-4306-aa27-7365a9abdedc

📥 Commits

Reviewing files that changed from the base of the PR and between d7d76d4 and 4adc87d.

📒 Files selected for processing (7)

• .gitignore
• AGENTS.md
• tko.io/public/agents/guide.md
• tko.io/public/agents/verified-behaviors/computed.md
• tko.io/public/agents/verified-behaviors/lifecycle.md
• tko.io/public/agents/verified-behaviors/utils-jsx.md
• tko.io/public/llms.txt

✅ Files skipped from review due to trivial changes (2)

• .gitignore
• tko.io/public/llms.txt

🚧 Files skipped from review as they are similar to previous changes (3)

• tko.io/public/agents/verified-behaviors/utils-jsx.md
• tko.io/public/agents/verified-behaviors/lifecycle.md
• tko.io/public/agents/guide.md

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Broaden package existence check to include builds/* packages

ls packages/<name> is incomplete for valid public packages like @tko/build.knockout, which live under builds/. This verification rule can misclassify legitimate references as invalid.

💡 Suggested doc fix

-2. For each package name in the doc: `ls packages/<name>` and confirm a matching `package.json`. Orphan `@tko/*` references mislead both humans and downstream agents.
+2. For each package name in the doc: confirm a matching `package.json` exists in either `packages/*` or `builds/*` (for example, by searching package names rather than assuming one root). Orphan `@tko/*` references mislead both humans and downstream agents.

As per coding guidelines: "Never ship documentation that references packages, exports, functions, spec paths, or URLs that do not exist on the target branch".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@AGENTS.md` at line 191, The package-existence verification currently only
runs "ls packages/<name>" which misses valid public packages under builds/, so
update the rule to check both locations (packages/<name> and builds/<name>) or
perform a single lookup that searches both directories and verifies a matching
package.json; replace references to the lone check "ls packages/<name>" in the
doc/guideline text with the broadened check logic/description so docs no longer
flag legitimate packages like "@tko/build.knockout" as orphans.