Add created/modified timestamps to analyses - #209
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAnalysis records and links now include ChangesAnalysis timestamp support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant analysisSlice
participant projectStorage
participant AnalysisData
Editor->>analysisSlice: dispatch prepared analysis action
analysisSlice->>AnalysisData: create or update record and link timestamps
projectStorage->>AnalysisData: read persisted project or draft
projectStorage->>AnalysisData: backfill missing timestamps
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Analysis payloads record when the content entered the project; links record when each token, segment, or phrase took it on — the two diverge whenever a payload is shared. Legacy records are backfilled on load.
8bbc39e to
dbfc9ae
Compare
Restore real timers from afterEach so a failing assertion can no longer freeze the clock for later tests, and pin the two invariants left unasserted: link updatedAt survives backfill, and differing timestamps still dedupe.
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc resolved 1 discussion.
Reviewable status: 0 of 20 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
A project stored before it carried `updatedAt` passed `undefined` as the backfill fallback, so the oldest analysis records stayed unstamped and the picker showed an invalid Modified date; it falls back to `createdAt` instead. Also retunes the analysis timestamp docs to the record-lifecycle semantics every write path already implements.
imnasnainaec
left a comment
There was a problem hiding this comment.
Review pass over the analysis-timestamp work, against 2407b86. Nothing blocking: one behavior regression in getProject, one stability improvement in getDraft, and three documentation notes. Verified locally — 1663 tests pass at 100% coverage.
— Claude Opus 5 (inspired by Devin)
A stored project whose analysis is missing no longer faults the read, and a draft that needed stamping is written back so the stand-in stops drifting between reads. Also corrects the timestamp docs on shared payloads and on what the analysis-record guard accepts.
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc made 5 comments.
Reviewable status: 0 of 21 files reviewed, 5 unresolved discussions (waiting on alex-rawlings-yyc and imnasnainaec).
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 `@src/services/projectStorage.ts`:
- Around line 633-637: Update the draft backfill flow around getDraft and
backfillAnalysisTimestamps so the read, timestamp mutation, and conditional
direct storage write execute within a single enqueueSerialized(draftQueues,
sourceProjectId, ...) operation. Replace the nested saveDraft call with the
appropriate direct persistence step inside that serialized callback, preserving
the existing error logging and avoiding a second queue submission.
🪄 Autofix
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 Plus
Run ID: 4aa45d78-ac4a-4f97-b33e-d1a47bd39a75
📒 Files selected for processing (9)
src/__tests__/services/projectStorage.test.tssrc/__tests__/utils/analysis-identity.test.tssrc/__tests__/utils/analysis-timestamps.test.tssrc/__tests__/utils/suggestion-engine.test.tssrc/services/projectStorage.tssrc/store/analysisSlice.tssrc/types/interlinearizer.d.tssrc/types/type-guards.tssrc/utils/analysis-timestamps.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/tests/utils/analysis-identity.test.ts
- src/types/interlinearizer.d.ts
- src/tests/utils/analysis-timestamps.test.ts
- src/tests/utils/suggestion-engine.test.ts
- src/store/analysisSlice.ts
The write-back was a second queue submission, so an auto-save landing between the two was clobbered by the stale copy the backfill stamped.
A record damaged outside the extension can carry neither time, which left the analysis backfill stamping undefined onto every record.
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc made 1 comment.
Reviewable status: 0 of 21 files reviewed, 6 unresolved discussions (waiting on alex-rawlings-yyc and imnasnainaec).
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec partially reviewed 15 files, made 1 comment, and resolved 5 discussions.
Reviewable status: 14 of 21 files reviewed, 2 unresolved discussions (waiting on alex-rawlings-yyc).
src/utils/analysis-timestamps.ts at r5 (raw file):
❓ I'm pondering if we even need to handle legacy analyses. We haven't even done a fully public release. It'd be simpler if we can just enforce fully stamped now.
|
Previously, imnasnainaec (D. Ror.) wrote…
That's what my initial thought was, but I figured it would be better to be safer to handle them by default and strip away legacy handling later if we decided it wasn't necessary. Since it exists already I'm fine with it, but I think I would really prefer if we didn't have code that handled legacy analyses |
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 7 files and all commit messages, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
This change is
Summary by CodeRabbit
New Features
Bug Fixes
Tests