feat: layout spec v1 and migration framework (M1.3)#9
Conversation
Formalize the repo layout as v1, add a forward-only idempotent migration framework stored in git config, and wire up migrate/layout-version CLI commands.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. WalkthroughAdds a layout migration subsystem: new LayoutMigration module managing layout versions in git config, CLI commands to run/inspect migrations, a formal layout specification, package bumped to 1.1.2, and unit + end-to-end tests for migration flows. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Invocation
participant LM as LayoutMigration
participant Config as Git Config
participant Graph as CmsGraph
participant Mig as Migration Function
CLI->>LM: migrate({ graph, refPrefix })
LM->>Config: readLayoutVersion(graph)
Config-->>LM: currentVersion
LM->>LM: pendingMigrations(currentVersion)
alt pending migrations exist
LM->>Mig: run migration(target, { graph, refPrefix })
Mig->>Graph: read/process refs & commits
Graph-->>Mig: operation results
Mig-->>LM: migration complete
LM->>Config: writeLayoutVersion(graph, target)
Config-->>LM: written
end
LM-->>CLI: { from, to, applied }
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 5: The changelog entry heading "## [1.1.0] — Migration & Repo Layout Spec
(M1.3)" should follow Keep a Changelog format: replace the subtitle "(M1.3)"
with an actual release date (YYYY-MM-DD) so the heading reads "## [1.1.0] —
YYYY-MM-DD", and add a corresponding link reference for "[1.1.0]" at the bottom
of the file similar to the existing "[Unreleased]" link reference (ensure the
reference URL/path matches repository/release tagging style used elsewhere).
In `@src/lib/LayoutMigration.js`:
- Around line 53-55: MIGRATIONS is currently a mutable array; freeze it and its
inner entries to prevent accidental mutation by applying Object.freeze to each
migration tuple (each [version, migrationFn] entry) and then
Object.freeze(MIGRATIONS) itself so both the outer array and its contained
arrays/functions are immutable; locate the MIGRATIONS constant and wrap its
entries with Object.freeze(...) for each tuple and finally freeze MIGRATIONS.
- Around line 23-34: The readLayoutVersion function currently treats an empty
string from graph.configGet as valid because Number('') === 0; modify
readLayoutVersion to explicitly reject empty or whitespace-only config values
(e.g. if raw === '' || raw.trim() === '') by throwing the existing
CmsValidationError with the same code/field, before converting to Number; then
continue to parse and validate numeric values as before (use Number(raw) or a
stricter integer-check like /^[0-9]+$/) so only proper non-negative integer
strings are accepted for LAYOUT_VERSION_KEY.
- Around line 43-45: writeLayoutVersion currently writes any value to
LAYOUT_VERSION_KEY via graph.configSet without validation; ensure the exported
function validates the version argument (e.g., in writeLayoutVersion) before
calling graph.configSet: require a finite integer >= 0 (no NaN, Infinity,
floats, or negatives), and if invalid throw or return a rejected Promise with a
clear message. Use the same LAYOUT_VERSION_KEY and graph.configSet call site,
and keep behavior consistent with readLayoutVersion's expectations so callers
cannot store invalid versions that would break subsequent reads.
In `@test/layout-migration.test.js`:
- Around line 29-51: Add a test that covers the empty-string edge case for
readLayoutVersion: after setting graph.configSet(LAYOUT_VERSION_KEY, ''), assert
that calling readLayoutVersion(graph) rejects with a CmsValidationError and code
'layout_version_invalid'. Place this new it(...) alongside the existing
non-integer/negative/fractional tests so the behavior when Number('') === 0 is
locked down and readLayoutVersion/LAYOUT_VERSION_KEY handling is validated.
- Reject empty/whitespace-only config values in readLayoutVersion
(Number('') === 0 was silently accepted as valid)
- Add input validation to writeLayoutVersion (reject NaN, Infinity,
floats, negatives)
- Freeze MIGRATIONS array and entries for immutability consistency
|
@coderabbitai review please 🥕 |
|
I'll scrutinize every line with maximum pedantry: documentation clarity, API design, error handling patterns, test coverage, edge cases, naming consistency, and adherence to your own specification. No stone unturned, no mercy given. Analyzing all 7 files now... 🥕 ✅ Actions performedReview triggered.
|
|
@coderabbitai review please 🥕 |
|
Deploying maximum pedantry across all 7 files. Layout spec, migration framework, CLI integration, test coverage, error handling, naming conventions, edge cases—everything will be examined with zero tolerance for ambiguity or weakness. Initiating ruthless review now. 🥕 ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/lib/LayoutMigration.js`:
- Around line 98-105: The final readLayoutVersion(graph) call is redundant;
compute "to" from the migration results instead of doing another async I/O.
After the loop that runs pending migrations (for ... of pending and await
migrateFn(...); await writeLayoutVersion(...); applied.push(target);), set to =
applied.length ? applied[applied.length - 1] : from and return { from, to,
applied } without calling readLayoutVersion; remove the extra
readLayoutVersion(graph) invocation.
In `@test/layout-migration.test.js`:
- Around line 53-59: Add a new test in test/layout-migration.test.js mirroring
the empty-string case to cover a whitespace-only string: call
graph.configSet(LAYOUT_VERSION_KEY, ' ') and assert that
readLayoutVersion(graph) rejects with CmsValidationError and code
'layout_version_invalid'; this ensures the raw.trim() === '' guard in
LayoutMigration.js (the readLayoutVersion logic) is exercised for
whitespace-only input.
- Compute `to` from applied migrations array instead of re-reading config (eliminates unnecessary async I/O round-trip) - Add whitespace-only string test to lock down trim() guard behavior
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/lib/LayoutMigration.js`:
- Around line 85-106: Add a one-line comment above the migrate function noting a
TOCTOU race: reading layout version via readLayoutVersion and then applying
migrations (pendingMigrations -> migrateFn) followed by writeLayoutVersion is
not protected by a lock, so concurrent migrate() invocations could both run the
same migrations; state that current migrations are idempotent/harmless but
future migrations with side effects must be made idempotent or protected by
external locking (or add locking around readLayoutVersion/writeLayoutVersion) to
avoid double-application.
In `@test/layout-migration.test.js`:
- Around line 70-100: Add an edge-case test for Infinity to the
writeLayoutVersion suite: create an InMemoryGraphAdapter and call
writeLayoutVersion(graph, Infinity) and assert it rejects with a
CmsValidationError having code 'layout_version_invalid' (same shape as the
existing negative/fractional/NaN tests), targeting the writeLayoutVersion
behavior to ensure Infinity is rejected.
- Add concurrency caveat to migrate() JSDoc - Add writeLayoutVersion(Infinity) rejection test
1 participant
Summary
docs/LAYOUT_SPEC.md) — formalizes ref namespace, state derivation rules, commit format, config keys, and migration policysrc/lib/LayoutMigration.js) —readLayoutVersion,writeLayoutVersion,pendingMigrations,migratewith forward-only, idempotent semantics stored incms.layout.versiongit configgit-cms migrate(run pending migrations) andgit-cms layout-version(print repo + codebase versions)Test plan
InMemoryGraphAdapter(read/write/pending/migrate, edge cases for invalid/negative/fractional versions, idempotency, too-new guard, content preservation)git config --get, articles survive migration)npx vitest run)