chore: audit component IVersionedStatefulComponent/IInspectableComponent applicability#162
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR clarifies that IVersionedStatefulComponent and IInspectableComponent apply to emulated-hardware core/IO classes (not React presentational components), expands architecture docs with an audit and usage rules, makes small docs formatting tweaks, and bumps APP_VERSION to '4.44.3'. ChangesComponent Contract Scope Clarification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
stid
force-pushed
the
chore/component-contracts-audit
branch
from
cb52185 to
f0bd914
Compare
…lity Resolves #159. Audits which components should implement IVersionedStatefulComponent / IInspectableComponent so CodeRabbit's coding-guideline isn't applied blanket. Finding: both contracts model the emulated-hardware tree and are implemented only by core/IO classes (Apple1, Bus, Clock, CPU6502, PIA6820, RAM, ROM, CRTVideo, Keyboard). No React component in src/components/** implements them, and none should — they render/consume the inspectable tree and hold only ephemeral view state via hooks. Documents the applicability rule + per-component classification table in docs/active/architecture.md and tightens the CLAUDE.md "Key Patterns" wording from "components implement" to the scoped core/IO layer. No contracts added or removed: the audit found everything already correct. Bumps version 4.43.0 -> 4.43.3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
stid
force-pushed
the
chore/component-contracts-audit
branch
from
f0bd914 to
5cfca8f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/active/architecture.md`:
- Around line 259-261: Update the contradictory statement about
IVersionedStatefulComponent so it matches the audit: replace the blanket claim
"All components implement IVersionedStatefulComponent" with a precise sentence
stating that only core/IO/hardware classes (or components that manage versioned
state) implement the contract and that React UI components do not implement it;
ensure the wording references IVersionedStatefulComponent and the audit
conclusion to make the exception explicit.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f76d1f61-ee09-455c-91c0-5561cc1c2e4c
📒 Files selected for processing (3)
CLAUDE.mddocs/active/architecture.mdsrc/version.ts
✅ Files skipped from review due to trivial changes (2)
- src/version.ts
- CLAUDE.md
Comment on lines
+259
to
+261
| **Audit result:** every core/IO class that needs the contracts already implements | ||
| them, and no React component does (correctly). No contract was added or removed by | ||
| this audit. |
There was a problem hiding this comment.
Resolve contradiction with the “All components implement IVersionedStatefulComponent” claim.
This new audit section says only core/IO hardware classes should implement these contracts, but later in this same doc (Line 414) it still says “All components implement IVersionedStatefulComponent.” Please align that statement to avoid conflicting guidance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/active/architecture.md` around lines 259 - 261, Update the contradictory
statement about IVersionedStatefulComponent so it matches the audit: replace the
blanket claim "All components implement IVersionedStatefulComponent" with a
precise sentence stating that only core/IO/hardware classes (or components that
manage versioned state) implement the contract and that React UI components do
not implement it; ensure the wording references IVersionedStatefulComponent and
the audit conclusion to make the exception explicit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #159
What & why
CodeRabbit's coding-guideline flags components that don't implement
IVersionedStatefulComponent/IInspectableComponent. But those contractsmodel the emulated-hardware tree, not the React presentation layer — applying
them blanket to UI components is a false positive. This PR audits the codebase,
documents the applicability rule, and records the per-component classification.
Finding
Both contracts are implemented only by core/IO classes — and that is correct.
No React component in
src/components/**implements (or should implement) them:they render and consume the inspectable tree (typically via an
apple1Instance: IInspectableComponentprop) and hold only ephemeral view statethrough hooks/contexts.
Implementers (the only classes that should)
IInspectableComponentIVersionedStatefulComponentApple1(src/apple1/index.ts)Bus(src/core/Bus.ts)Clock(src/core/Clock.ts)CPU6502(src/core/cpu6502/core.ts)PIA6820(src/core/PIA6820.ts)RAM(src/core/RAM.ts)ROM(src/core/ROM.ts)CRTVideo(src/apple1/WebCRTVideo.ts)Keyboard(src/apple1/WebKeyboard.ts)Component audit —
src/components/**Every component is React presentation / a UI helper. None owns versioned
hardware state or produces an inspectable node, so the contracts are N/A for
all of them (including
ActionsandRegisterRow, which are stateless;AppContentonly delegates save/load to the worker/core;InspectorView/DebuggerLayoutconsume the tree). Full per-component table is indocs/active/architecture.md.The documented rule
persisted/versioned state (→
IVersionedStatefulComponent) and/or exposedebug-inspectable state (→
IInspectableComponent).helpers.
Documented in
docs/active/architecture.md(new "Where these contracts apply"section + implementer table + full component classification table) and the
CLAUDE.md"Key Patterns to Follow" wording was tightened from the blanket"components implement …" to the scoped core/IO layer.
What was implemented
Nothing in code — the audit found every contract correctly placed already. Per the
project's anti-overengineering rule, no contracts were added or removed. Deliverable
is the documented rule + classification. Version bumped 4.43.0 → 4.43.3.
Checks
yarn lint✅yarn type-check✅yarn test:ci✅ (692 passed, 18 skipped — WASM parity/benchmark, gitignored artifacts)🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Documentation