Repository navigation
Check analyses against the whole source text (#143) - #399
alex-rawlings-yyc wants to merge 6 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
📝 WalkthroughWalkthroughThe concordance index now supports whole-project text reads and reports reading status and text forms. The loader uses completed reads to re-anchor linked analyses. The catalog adds filters for stale analyses and forms absent from source text. Translations associated with removed text receive fallback positions when the book contains text. ChangesProject text coverage and analysis catalog
Stale translation placement
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant AnalysisCatalogPanel
participant useConcordanceIndex
participant InterlinearizerLoader
participant Draft re-anchoring
AnalysisCatalogPanel->>useConcordanceIndex: request text reading
useConcordanceIndex->>InterlinearizerLoader: report each read book
InterlinearizerLoader->>Draft re-anchoring: re-anchor linked book
useConcordanceIndex->>InterlinearizerLoader: report completed book IDs
InterlinearizerLoader->>Draft re-anchoring: mark linked absent books stale
Merge Risk: 🔵 Low · up to A rapid draft replacement may receive stale re-anchoring results, and a failed book read may appear complete in the concordance. These bounded risks warrant fixes or explicit acceptance before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Out of Scope Changes checkExplanation The changes to ✨ 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 |
75797dc to
db967e9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1d11b5a to
4db4b5d
Compare
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
Review drafted with Claude Opus 5.5; inline comments are moderately reviewed by me.
| <span className="tw:sr-only">{` ${staleCountLabel}`}</span> | ||
| </span> | ||
| )} | ||
| {isNotInText && ( |
There was a problem hiding this comment.
Conflicts with #400: its subgrid has 7 fixed columns, so this badge becomes an unplanned 8th. Coordinate on whichever merges second.
There was a problem hiding this comment.
Whichever of the two PRs lands second will add an 8th auto column to CatalogList's template when it rebases, so the badge gets its own aligned column.
| ## Algorithm | ||
|
|
||
| When it runs: on every book load, in `useReanchorToBook` (`src/components/AnalysisStore.tsx`), for an editable project only. An imported, read-only project is a record of what was imported and is never healed. A pass that moves nothing leaves the analysis identical, so opening a book neither dirties the draft nor writes storage. | ||
| When it runs: whenever the loaded book's text, its boundaries, or the draft changes, and for every other book the draft has records in each time the whole source text is read (for the concordance, or a catalog filter that needs the text), for an editable project only. A book the project no longer has is treated as holding no text, so every approval in it goes stale. An imported, read-only project is a record of what was imported and is never healed. A pass that moves nothing leaves the analysis identical, so opening a book neither dirties the draft nor writes storage. |
There was a problem hiding this comment.
⛏️ One ~60-word sentence. Make it a short list of triggers.
| ); | ||
|
|
||
| /** | ||
| * Re-anchors the draft to the absence of every book it has anything in that the text lacks, |
There was a problem hiding this comment.
⛏️ Hard to parse. Suggest: "Marks links into books the project no longer has as stale."
There was a problem hiding this comment.
Reworded, though not as suggested: only approvals go stale, not every link, and this handler also records the draft as fully re-anchored. It now documents onTextRead in useWholeTextReanchor.
| const [textReanchoredFor, setTextReanchoredFor] = useState<number>(); | ||
| const staleCoversDraft = readingTarget !== undefined && textReanchoredFor === readingTarget; | ||
|
|
||
| /** Re-anchors the draft to a book the whole-text read hands on, where it has anything there. */ |
There was a problem hiding this comment.
⛏️ Suggest: "Re-anchors the draft to each book the whole-text read delivers, if the draft links into it. The loaded book is skipped; it re-anchors to its live text."
There was a problem hiding this comment.
Done, in one sentence; it now documents onBookRead in useWholeTextReanchor.
| loadedBookCodeRef.current = verseBook?.bookRef; | ||
|
|
||
| /** | ||
| * The version of the draft a reading of the whole text re-anchors, `undefined` while it is |
There was a problem hiding this comment.
⛏️ Suggest: "Draft version the whole-text read re-anchors; undefined while loading or showing an import."
There was a problem hiding this comment.
Done, keeping the clause that each change reads the text again, since this value is the reader's readKey. It now documents WholeTextReanchor.readKey.
| placesByVerse: ReadonlyMap<string, Place[]>, | ||
| ): Place | undefined { | ||
| const [chapter, verseNumber] = versePosition(verse); | ||
| const inOrder = [...placesByVerse] |
There was a problem hiding this comment.
⛏️ This re-sorts every place for each vanished translation. Sort once per book.
There was a problem hiding this comment.
Done: the document-ordered list is built once per call, and only when a translation's verse has vanished.
|
|
||
| /** Localized string keys {@link TextReadingStatus} renders. */ | ||
| export const TEXT_READING_STRING_KEYS = [ | ||
| '%interlinearizer_concordance_loading%', |
There was a problem hiding this comment.
⛏️ These keys are shared beyond the concordance now. Rename them to %interlinearizer_textReading_*%?
There was a problem hiding this comment.
Done; _partial% stays a concordance key, since only the concordance shows it.
| @@ -88,16 +109,23 @@ export default function useConcordanceIndex({ | |||
| liveBook, | |||
| enabled, | |||
| shown, | |||
| onBookRead, | |||
There was a problem hiding this comment.
⛏️ This now serves the catalog and re-anchoring too. Split out a useSourceTextReader, or rename the hook?
There was a problem hiding this comment.
Split: useSourceTextReader reads the text, useConcordanceEntries builds the entries from it, and SourceTextContext provides both, replacing ConcordanceIndexContext.
| * The version of the draft a reading of the whole text re-anchors, `undefined` while it is | ||
| * loading or an import is shown in its place; each change reads the text again. | ||
| */ | ||
| const readingTarget = isImportView || isDraftLoading ? undefined : draftVersion; |
There was a problem hiding this comment.
⛏️ These ~45 lines (2 refs + textReanchoredFor) could be a useWholeTextReanchor hook returning readKey / onBookRead / onTextRead / staleCoversDraft, testable with renderHook.
There was a problem hiding this comment.
Done, with its own renderHook suite; the loader keeps a few wiring tests.
| </ul> | ||
| )} | ||
|
|
||
| {!isAwaitingText && |
There was a problem hiding this comment.
⛏️ Pulling out a CatalogRowList would avoid this re-indent and most of the conflict with #400.
There was a problem hiding this comment.
Leaving this: #400 already extracts the <ul> and its sentinel as CatalogList, so a second extraction here would compete with it. Whichever PR lands second wraps the rows in CatalogList when it rebases.
| const rows = useMemo(() => applyCatalogQuery(catalogRows, query), [catalogRows, query]); | ||
|
|
||
| /** Whether the listing waits on the text, a filter needing it before it is read. */ | ||
| const isAwaitingText = filters.notInText && !textForms; |
There was a problem hiding this comment.
⛏️ filterTo (:550) sets filters without requestText(). It's safe today, since the stale notice only sets stale for the loaded book. But any filterTo({ notInText: true }), or filters restored later, would hang here on "Reading books… 0 of 0". Key the request off the wait condition instead:
| const isAwaitingText = filters.notInText && !textForms; | |
| const isAwaitingText = filters.notInText && !textForms; | |
| // Covers every way filters get set, not just the controls. | |
| useEffect(() => { | |
| if (filters.notInText) requestText(); | |
| }, [filters.notInText, requestText]); |
Then the handler only needs if (next.stale) requestText();.
There was a problem hiding this comment.
Done as suggested.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc made 21 comments.
Reviewable status: 0 of 37 files reviewed, 21 unresolved discussions (waiting on alex-rawlings-yyc and imnasnainaec).
| ## Algorithm | ||
|
|
||
| When it runs: on every book load, in `useReanchorToBook` (`src/components/AnalysisStore.tsx`), for an editable project only. An imported, read-only project is a record of what was imported and is never healed. A pass that moves nothing leaves the analysis identical, so opening a book neither dirties the draft nor writes storage. | ||
| When it runs: whenever the loaded book's text, its boundaries, or the draft changes, and for every other book the draft has records in each time the whole source text is read (for the concordance, or a catalog filter that needs the text), for an editable project only. A book the project no longer has is treated as holding no text, so every approval in it goes stale. An imported, read-only project is a record of what was imported and is never healed. A pass that moves nothing leaves the analysis identical, so opening a book neither dirties the draft nor writes storage. |
|
|
||
| const rows = useMemo(() => applyCatalogQuery(catalogRows, query), [catalogRows, query]); | ||
|
|
||
| /** Whether the listing waits on the text, a filter needing it before it is read. */ |
There was a problem hiding this comment.
Done, worded to cover a failed read as well, which also leaves textForms undefined.
| const rows = useMemo(() => applyCatalogQuery(catalogRows, query), [catalogRows, query]); | ||
|
|
||
| /** Whether the listing waits on the text, a filter needing it before it is read. */ | ||
| const isAwaitingText = filters.notInText && !textForms; |
There was a problem hiding this comment.
Done as suggested.
| </ul> | ||
| )} | ||
|
|
||
| {!isAwaitingText && |
There was a problem hiding this comment.
Leaving this: #400 already extracts the <ul> and its sentinel as CatalogList, so a second extraction here would compete with it. Whichever PR lands second wraps the rows in CatalogList when it rebases.
| filters.morphemes, | ||
| filters.zeroUsages, | ||
| filters.stale, | ||
| filters.notInText, |
There was a problem hiding this comment.
Done as TOGGLE_FILTER_KEYS in analysis-query.ts, used by the popover's count and by reconcileFilters. It's named for toggles rather than booleans, since morphemes is 'has' | 'lacks'.
| shown: boolean; | ||
| /** | ||
| * Reads every book again, as a refresh does, whenever this changes; a reading begun under an | ||
| * earlier value hands nothing on. |
There was a problem hiding this comment.
Done; the hook is now useSourceTextReader.
| @@ -88,16 +109,23 @@ export default function useConcordanceIndex({ | |||
| liveBook, | |||
| enabled, | |||
| shown, | |||
| onBookRead, | |||
There was a problem hiding this comment.
Split: useSourceTextReader reads the text, useConcordanceEntries builds the entries from it, and SourceTextContext provides both, replacing ConcordanceIndexContext.
| writingSystemRef.current = writingSystem; | ||
|
|
||
| // Read through refs so a read in flight calls whichever callbacks are current when it lands. | ||
| const onBookReadRef = useRef(onBookRead); |
There was a problem hiding this comment.
Done, along with the three older hand-mirrored refs in the same file, which is now useSourceTextReader.
| if (!wanted) return undefined; | ||
| let isAbandoned = false; | ||
| const key = readKeyRef.current; | ||
| // The key moves on as soon as it renders, ahead of the cleanup that abandons this read. |
| @@ -201,8 +253,19 @@ export default function useConcordanceIndex({ | |||
| return buildConcordanceEntries(books.values(), collator); | |||
| }, [status, readings, mergedLiveVersions, collator]); | |||
|
|
|||
| const textForms = useMemo(() => { | |||
There was a problem hiding this comment.
No comment added: with the reader split out, the held-back copy lives only in useConcordanceEntries, so textForms in useSourceTextReader has nothing to differ from.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc made 21 comments.
Reviewable status: 0 of 37 files reviewed, 21 unresolved discussions (waiting on alex-rawlings-yyc and imnasnainaec).
2 participants
Closes #143.
The Analysis Catalog has a new "Word not in the text" filter. Rows whose form appears nowhere in the source text get a badge. The check covers the whole text: the loaded book as it is now, and other books as they were last read for the concordance. A word gone from the text is safe to discard, while one still in it can be re-applied.
The text is read the first time the new filter or Stale is turned on. A concordance already built in the tab also counts. Just opening the catalog doesn't read it. While the text is being read, the listing shows how many books are done, using the status the concordance already had.
If any book fails to read, no word is marked as not in the text. The catalog says the text could not be read instead. The concordance still lists the books that did read, with a notice above the list that some could not.
Each read also re-anchors every book the draft has analyses in, as that book arrives. This is the whole-draft re-anchoring #349 left here. Once every book is in, the Stale filter drops its "(found when a book is opened)" qualifier. Replacing the draft (New, Open, Wipe) reads the text again.
Links into a book the project no longer has go stale.
A free translation of a verse the book no longer has, which #349 left unreachable, now appears in the segment ending the nearest earlier verse. If no earlier verse remains, it appears in the book's first segment.
Checked in the running app.
This change is
Summary by CodeRabbit