Repository navigation
Store project summaries for the picker (#434) - #472
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
cbb80f6 to
979f23c
Compare
jasonleenaylor
left a comment
There was a problem hiding this comment.
Reviewed with reproductions. Findings 1 to 5 were each confirmed by a throwaway jest test against this branch (finding 2 also against main, for comparison); the three nits are from reading. Sorted by severity below and inline.
- Medium: a Save after a half-failed delete reports success, then the save is lost at the next startup sweep.
- Medium: a listed summary whose record is missing is shown and trusted; each Paratext 9 sync then creates another import (regression from main).
- Medium: a failed summary refresh after the record landed reports the Save, metadata edit or sync as failed.
- Low: the sweep's comment states an invariant
deleteProjectnow breaks. - Low: every Save summarizes the whole analysis while holding the global index lock.
Nits: a missing @throws on createEditableCopy, a test helper that writes a summary shape this build never writes, and a silently skipped perf measurement.
The overall design (one record, Node-safe class, summaries written on every project write) looks right to me; these are all about the places where the listing and the records can disagree.
jasonleenaylor
left a comment
There was a problem hiding this comment.
Verified c21fd43: reran the reproductions from my review and all five findings now behave correctly (Save to an unlisted project returns gone and the sweep cleans up; a ghost import is unlisted and two syncs leave one import; a failed summary refresh logs and the save resolves; the sweep comment and warning read right; the summary is built outside the lock). Also checked a create whose summaries write lands but reports failure: it rejects and leaves no listed-but-missing project. Full suite at 100% coverage and lint pass. The two narrower choices (write then check the listing; staleKept can still return a ghost id until the next converting sync) look fine to me.
Heads-up: this and #475 conflict in three small hunks (two import lines and the updateAnalysis doc comment). I'll take the rebase if #475 lands second.
jasonleenaylor
left a comment
There was a problem hiding this comment.
@jasonleenaylor reviewed 14 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
Part of #424; closes #434.
Summary
Saved projects are listed from one stored record of summaries, which every project write keeps current. The picker and the Paratext 9 lookups read only that record, never a project record.
projectSummariesholds{ modelVersion, summaries }in creation order and replacesprojectIds. Each summary carriesbooksandtokenAnalysisCount, computed at write time. It also carriespt9Importwith its file hashes, which the WebView compares when it opens an import.ProjectSummariesimports nothing from PAPI and works overKeyValueStorage, likeDraftRepository. The perf seeder lists its saved projects through it.getPt9ImportForSourcereturns a summary, since its callers use only the id.listProjectsis removed.wireBytes.projectsForSourceis now the measured size of thegetProjectsForSourceresponse. It used to be estimated as draft size × saved projects.updateAnalysis:summaryis a new host step timing the summarizing alone, done before the summaries lock is taken.Evidence
projectSummarieskey.perf:app: packaged app, one run, on this branch, against the Phase 0 baseline (10 runs at10d81320):projects-fetchwent from 422 to 139 ms on large-loadable, but from 60 to 160 ms on small; a single run on this machine is too noisy to read those.Deviations from the issue and its comment
projectStorage. The comment planned a core plus a thin PAPI wrapper, with the seeder calling the core. Only the summaries moved into a Node-safe class. The seeder still writes the project record itself and lists it through that class. Nothing else needs project records in Node yet.fetchSummarystill pulls the analysis. After an import, a sync or an editable copy, the WebView builds the summary fromgetProject's full record. Phase 1's surface adds no WebView calls, so this waits until a later phase trimsgetProject.Decisions with a strong alternative
Reviewer push-back is welcome.
projectIdsMerge Danger
Door: one-way for saved projects
Projects saved before the merge stop listing, and a revert would stop listing those saved after it. Their records stay on disk.
Blast Radius: saved projects
This touches listing, create, save, metadata edits, delete, and Paratext 9 import and sync.
This change is