feat(kits): batch B-arch — Simple/Advanced color mode contract (no code)#24
Conversation
Architecture-only spec for the deferred Batch B (Simple/Advanced color mode), per AGENT_HANDOFF.md Batch A "next recommended batch" section. Locks the contract before any implementation code lands. Editor R11 (new): Simple shows 2 controls (Primary, Neutral) + derives the other 7 deterministically; Advanced shows all 9 with no derivation. Mode switches are undoable. R1 annotated as mode-aware. Persistence R8 (new): optional `colorMode` and `colorOverrides` fields on the PersistedRecord wrapper; both omittable; legacy records load as Simple with empty overrides; schema version stays at 1. R6 clarified that wrapper-field additions don't bump version. Product Boundary: changelog-only confirmation that Simple/Advanced falls under R1 capability "Theme design via interactive controls" — no R revision required. Overview: domain index counts updated (Editor R1–R11, Persistence R1–R8, total reqs 53 → 55), Editor→Persistence cross-ref added. Zero src/, tests/, or schema changes. Boundary R2 unviolated. Catalog unchanged at 11 widget IDs. Gates: lint 0, typecheck pass, test 267/267, build pass.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75fc7c5f11
ℹ️ 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".
| **Description:** The editor must expose one editable control per color slot defined by the schema. Each control must allow the user to set a valid hex color and reflect the current value. Control rendering is mode-aware per R11 (Simple shows 2 controls; Advanced shows all 9). | ||
| **Acceptance Criteria:** | ||
| - [ ] One control is rendered for each color slot in the active theme |
There was a problem hiding this comment.
R11 introduces a valid Simple mode where only two color controls are shown, but R1 still requires rendering one control for each color slot; these requirements cannot both be true in Simple mode. This creates an internally inconsistent spec that will cause implementers/tests to fail one requirement while satisfying the other, so R1’s acceptance criteria should be made explicitly mode-conditional (or scoped to Advanced mode only).
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Simple/Advanced' color mode for the theme editor, allowing users to either derive secondary colors deterministically or edit all nine slots independently. The changes span the editor requirements, persistence schema, and the project overview. Feedback focuses on logical inconsistencies between the editor and persistence requirements regarding how overrides are handled and discarded, as well as potential data redundancy in the persistence layer. A suggestion was also provided to clarify the architectural relationship between the Editor and Persistence domains in the overview documentation.
| **Dependencies:** none | ||
|
|
||
| ### R11: Color Mode (Simple / Advanced) | ||
| **Description:** The editor must expose a Simple ↔ Advanced color-mode toggle. Simple mode renders exactly two color inputs (Primary, Neutral) and derives the remaining seven color slots deterministically from those two inputs (per the Batch A handoff rule: secondary = primary hue-shifted +40°; background, text, muted, hairline, inkSoft, surfaceInvert, onInvert derived from neutral via fixed L values; exact derivation values are intentionally left to the implementation cavekit-revision or to the implementation batch). Advanced mode renders all nine color controls with no derivation, each independently editable. Switching Advanced→Simple discards manual overrides for the seven derived slots and resumes derivation. Switching Simple→Advanced seeds the nine controls with the currently displayed (derived) values, treated as overrides going forward. The mode is persisted across sessions via cavekit-persistence.md R8. |
There was a problem hiding this comment.
There is a logical inconsistency between the definition of Simple mode here and the persistence requirements in cavekit-persistence.md R8. This requirement states that switching from Advanced to Simple discards manual overrides. However, R8 describes colorOverrides as values applied "on top of" the derived base and requires persisting them even in Simple mode. If Simple mode is strictly deterministic and discards overrides upon entry, the colorOverrides field appears redundant or incorrectly defined.
| **Dependencies:** R1, R2 | ||
|
|
||
| ### R8: Optional Color-Mode Fields on Persisted Record | ||
| **Description:** The PersistedRecord may carry two optional fields — `colorMode` (a string equal to `'simple'` or `'advanced'`) and `colorOverrides` (a partial map of `ColorTokens` representing user-edited slot values applied on top of the Simple-mode-derived base). Both fields are omittable. The persistence schema generation (version) stays at 1; legacy records lacking these fields load as Simple mode with an empty override map. `ThemeConfigSchema` is NOT extended — these fields live on the PersistedRecord wrapper around the theme. |
There was a problem hiding this comment.
The colorOverrides field seems redundant given the existing architecture. Since ThemeConfig (the theme field in the PersistedRecord) is the single source of truth for all domains and already contains all 9 color slots, any manual edits made in Advanced mode are already captured within that object. Storing them separately in colorOverrides introduces data duplication and potential synchronization risks between the theme and the overrides map.
| **Description:** The PersistedRecord may carry two optional fields — `colorMode` (a string equal to `'simple'` or `'advanced'`) and `colorOverrides` (a partial map of `ColorTokens` representing user-edited slot values applied on top of the Simple-mode-derived base). Both fields are omittable. The persistence schema generation (version) stays at 1; legacy records lacking these fields load as Simple mode with an empty override map. `ThemeConfigSchema` is NOT extended — these fields live on the PersistedRecord wrapper around the theme. | ||
| **Acceptance Criteria:** | ||
| - [ ] PersistedRecord shape accepts records with or without the `colorMode` and `colorOverrides` fields without error | ||
| - [ ] At save time, when the editor is in Simple mode the record persists `colorMode='simple'` plus any active overrides; when in Advanced mode the record persists `colorMode='advanced'` plus the full override set |
There was a problem hiding this comment.
This acceptance criterion specifies that in Simple mode, the record persists "any active overrides". This contradicts cavekit-editor.md R11, which states that switching to Simple mode discards all overrides. If the overrides are discarded by the editor, they cannot be persisted as "active" while in Simple mode.
| | Export | Editor | Reads active theme on demand (read-only, no mutation) | | ||
| | Persistence | Schema | Validates records on save, restore, and import | | ||
| | Persistence | Editor | Saves active-theme changes; supplies restored or imported themes via the editor's external-adoption interface | | ||
| | Editor | Persistence | Persists `colorMode` + `colorOverrides` per cavekit-editor.md R11 / cavekit-persistence.md R8 | |
There was a problem hiding this comment.
The interaction description "Persists colorMode + colorOverrides" suggests that the Editor domain is the active agent performing the persistence. However, the existing architecture (line 31) defines the Persistence domain as the actor that saves changes by observing the Editor. To maintain consistency, this should be phrased to show that Persistence reads these fields from the Editor.
| | Editor | Persistence | Persists `colorMode` + `colorOverrides` per cavekit-editor.md R11 / cavekit-persistence.md R8 | | |
| | Editor | Persistence | Supplies `colorMode` and `colorOverrides` for storage per cavekit-editor.md R11 / cavekit-persistence.md R8 | |
1 participant
Summary
Architecture-only spec for the deferred Batch B (Simple/Advanced color mode), per
AGENT_HANDOFF.mdBatch A "Next recommended batch" section. Locks the contract before any implementation code lands. Pure cavekit edits — zero source, schema, or test changes.Changes (4 files, all under
context/kits/)cavekit-editor.mdcavekit-persistence.mdcolorMode,colorOverrides— both omittable, no version bump, legacy records load as Simple with empty overrides). R6 clarified that wrapper-field additions don't bump version.cavekit-product-boundary.mdcavekit-overview.mdConstraints honored
git diff --statshows exactly 4 files, all undercontext/kits/Preflight
gh pr list --author app/dependabot --state open— emptymainbefore branchingAGENT_HANDOFF.mdBatch A read for derivation ruleVerification
git diff --statcontext/kits/grep -c '^### R' cavekit-editor.mdgrep -c '^### R' cavekit-persistence.mdgrep -c '^### R' cavekit-product-boundary.mdnpm run lintnpm run typechecknpm run testnpm run buildTest plan
AGENTS.md: review PR comments + formal reviews + checks before mergeNext batch
Batch B-impl — wire R11 + R8 into editor/store/persistence. Add tests for derivation determinism and round-trip preservation. Stays within R1 capabilities; no new boundary revisions required.