refactor(cache): remove timestamp fields from lock#32
Conversation
commit: |
There was a problem hiding this comment.
Pull request overview
This PR refactors the docs cache lockfile schema to remove timestamp fields (generatedAt on the lock, updatedAt per source) so lockfiles are stable and don’t change due to time-based noise.
Changes:
- Remove
generatedAt/updatedAtfrom lockfile TypeScript interfaces and runtime validation. - Stop generating timestamps in the sync command when building the lock.
- Update test fixtures and several sync-related tests to match the new schema.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/sync-offline-fail.test.js | Removes timestamp fields from lock JSON written in offline sync tests. |
| tests/sync-materialize.test.js | Removes timestamp fields from lock JSON written in materialization tests. |
| tests/lock.test.js | Updates lock read/write test to no longer depend on generatedAt. |
| tests/fixtures/empty.docs-lock.json | Removes generatedAt from fixture. |
| tests/fixtures/docs-lock.json | Removes generatedAt and per-source updatedAt from fixture. |
| src/commands/sync.ts | Stops generating and writing timestamps when building lock entries. |
| src/cache/lock.ts | Updates lock/source interfaces and validateLock to drop timestamp requirements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughWalkthroughRemoved per-lock and per-source timestamp fields: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
tests/lock.test.js (1)
49-50: Consider asserting full lock round-trip, not a single field.Line 50 checks
toolVersion, butdeepEqual(parsed, lock)would better protect against accidental omissions in serialized lock fields.As per coding guidelines, “Add or update tests for behavior changes and bug fixes; prefer extending existing test coverage in `tests/` before adding new files”.♻️ Suggested test tightening
const parsed = await module.readLock(tmpPath); -assert.equal(parsed.toolVersion, lock.toolVersion); +assert.deepEqual(parsed, lock);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lock.test.js` around lines 49 - 50, Replace the single-field assertion with a full round-trip equality check: after calling module.readLock(tmpPath) (the parsed variable) assert that parsed deeply equals the original lock object (lock) instead of only checking parsed.toolVersion; update the test assertion to use a deep equality matcher (e.g., deepEqual/strictDeepEqual depending on test framework) so the entire serialized/deserialized lock is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/lock.test.js`:
- Around line 49-50: Replace the single-field assertion with a full round-trip
equality check: after calling module.readLock(tmpPath) (the parsed variable)
assert that parsed deeply equals the original lock object (lock) instead of only
checking parsed.toolVersion; update the test assertion to use a deep equality
matcher (e.g., deepEqual/strictDeepEqual depending on test framework) so the
entire serialized/deserialized lock is validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c7907ed-710c-4969-b336-03b4bb922c05
📒 Files selected for processing (7)
src/cache/lock.tssrc/commands/sync.tstests/fixtures/docs-lock.jsontests/fixtures/empty.docs-lock.jsontests/lock.test.jstests/sync-materialize.test.jstests/sync-offline-fail.test.js
💤 Files with no reviewable changes (4)
- tests/fixtures/empty.docs-lock.json
- tests/sync-materialize.test.js
- tests/sync-offline-fail.test.js
- src/cache/lock.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/edge-cases.test.js (1)
241-262:⚠️ Potential issue | 🟠 MajorThis test asserts raw JSON shape, not lock-schema validation behavior.
assert.equal(parsed.toolVersion, undefined)only proves the file was written without that key. It does not verify that the lock reader/validator rejects missing required fields (toolVersion,sources). This can pass even if validation regresses.Please assert the validation path throws (e.g., via
validateLock/lock read API) for this payload instead of assertingundefinedon rawJSON.parse.As per coding guidelines: “
tests/**/*.{js,ts,mjs,cjs}: Add or update tests for behavior changes and bug fixes; prefer extending existing test coverage intests/before adding new files”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/edge-cases.test.js` around lines 241 - 262, The test currently only asserts the raw JSON shape (parsed.toolVersion === undefined) instead of verifying the lock reader/validator rejects missing required fields; update the test to call the lock validation/read API (e.g., validateLock(invalidLock) or await readLock(lockPath) / readLockFile(lockPath)) and assert that it throws an error for this invalid payload; keep the existing invalidLock, lockPath and DEFAULT_LOCK_FILENAME variables but replace the JSON.parse assertion with a thrown-validation assertion (using your test framework's reject/throws helper) to ensure missing toolVersion and sources are rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/edge-cases.test.js`:
- Around line 241-262: The test currently only asserts the raw JSON shape
(parsed.toolVersion === undefined) instead of verifying the lock
reader/validator rejects missing required fields; update the test to call the
lock validation/read API (e.g., validateLock(invalidLock) or await
readLock(lockPath) / readLockFile(lockPath)) and assert that it throws an error
for this invalid payload; keep the existing invalidLock, lockPath and
DEFAULT_LOCK_FILENAME variables but replace the JSON.parse assertion with a
thrown-validation assertion (using your test framework's reject/throws helper)
to ensure missing toolVersion and sources are rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28d79157-1d96-43d2-b543-e5a679897a1e
📒 Files selected for processing (2)
tests/edge-cases.test.jstests/lock.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/lock.test.js
Summary
Eliminate timestamp tracking from lock file schema.
Motivation
It was introducing changes and possibly merge conflicts while nothing in the referenced sources was actually changed and provided no benefits.
Changes
generatedAtfrom lock interface and validationupdatedAtfrom source interface and validationSummary by CodeRabbit
Refactor
Tests