Add the Paratext 9 import WebView experience with a first-open offer - #274
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 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 |
591ebbf to
7706947
Compare
352ac8a to
96db39b
Compare
7706947 to
5ce31d8
Compare
a7b6817 to
3ca877f
Compare
d5f84f5 to
378ab64
Compare
3ca877f to
d4070a8
Compare
e23f7d9 to
d74a2cf
Compare
|
❓ |
378ab64 to
6b4b8d7
Compare
e5c7fe8 to
3fd34ea
Compare
6b4b8d7 to
2504539
Compare
|
Deliberate. No resolver runs during import yet, so folding the sense counters would show zero resolved and everything unresolved on every run, which reads as a failure for work that is not wired up. The counters are populated and pinned on the converter side so they are ready when a resolver lands. Folding and rendering them belongs with the resolver work tracked in #280, which now lists it as a scope point; ReportTotals carries a TODO pointing there (ccb28ea). --drafted by Claude-- |
3f6df95 to
ef8e70b
Compare
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
I've pushed a review-fixes branch for this PR: review-fixes/pt9-parsed-webview (branched off pt9-parsed-webview at ef8e70b, #295). Five defects, all with regression tests that fail against the current head.
Read-only enforcement had three holes. The PR guards BoundaryControl and PhraseStripParts, but three parallel mutation routes stayed live in an import view:
SegmentListView.tsx:428— the between-rows merge button. Clicking it dispatchessegmentationDispatch.merge, which writes boundaries into the user's editable draft and flips the dirty indicator. Nothing visible happens in the import, becausebookis computed withresegmentBook(verseBook, undefined)there — so it silently corrupts an unrelated draft.SegmentView.tsx:191— Alt+click on a baseline-text split gap. Same silent draft write, in continuous-scroll mode. The Alt-gatedSplitMarkerinPhraseStripPartsis covered; this is its baseline-mode twin.ArcOverlay.tsx:291— the arc split button.analysisMerger.ts:401does emitphraseAnalysisLinks, so imports draw arcs; hovering one revealsLink2Offand a click deletes a 2-token phrase outright. WithPhraseBox's pill andTokenLinkIconboth gated, this was the only surviving route that visibly mutates the import.
For ArcOverlay I used a readOnly prop rather than the hook — it's a presentational leaf whose tests render it bare, so the hook would have forced a provider onto every call site.
The offer mode never reached the modal. InterlinearizerLoader.tsx:1261 collapsed pt9Mode === 'offer' into 'import', which made Pt9ImportModal's entire mode === 'offer' branch dead outside its own unit test. Two consequences, both contradicting the spec added in user-questions.md ("a single Open (dismissing the report also opens…)"): the offer report rendered Close + Open, and dismissing it routed through handlePt9Close → setModal('none'), so a successful conversion was never opened — the user had to go find it in Select. The test "returns to the plain view when an offer-run report is closed" locked that in; I replaced it with two tests asserting the documented behavior.
A failed import-analysis load rendered a blank pane. Once the book has loaded and neither book error applies, all three of loadingOrErrorPanel's conditionals are falsy, so the user got an empty content area plus a toast. Reachable via openImportedProject's :718 fallback. Added an importLoadFailed state, an on-screen message, and a new localized string.
handlePt9Open wrote state after the user backed out. The report stays dismissable during fetchSummary, so Escape → back to Select → resolve closed the Select modal and switched the active project anyway. Added a modalRef in-flight guard.
One reported finding I investigated and rejected, in case it comes up: a claim that a manual sync remounts the store seeded with the pre-sync analysis (initialAnalysis being a non-reactive lazy-ref seed while the key carries updatedAt). I instrumented the actual mount sequence with and without the proposed fix and got identical seeds both ways — React's batching means the remount already happens after the fresh analysis lands. No code change; I kept a test asserting the correct behavior, since nothing covered it before.
Also verified clean, so nobody needs to re-check: savePt9Import's buildNew() spread is field-for-field equivalent to the literal it replaced; the openRequest effect's exhaustive-deps disable has no stale closure; hasDraft/hasNoInterlinearizerState handle ENOENT correctly; and __setMockAnalysisReadOnly's module state is reset in every file that touches it.
Two things left for your judgment. isPt9TooLargeError calls error.message.includes(...) after isPlatformError, whose real guard only checks for platformErrorVersion — a platform error without message would throw out of the catch in runPt9Import. I couldn't verify whether any producer omits it, so I left it as a robustness question rather than guessing. And the on-open auto-sync path had no test at all; I added one covering the sync wording, which is worth a look to confirm that's what you want users to see.
Beyond the fixes: a comment-rules pass (four fixes — two mock docs naming the wrong lifecycle hook, one over-long doc, and one mechanism-explaining inline).
@alex-rawlings-yyc reviewed 48 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on jasonleenaylor).
The UI half of the PT9 import. The select modal gains the import button (shown only when the source serves convertible data, via usePt9ImportAvailability); Pt9ImportModal carries the run, its report, and its failures, including the too-large refusal recognized by the RESOURCE_EXHAUSTED platform error code with the documented message marker as fallback. An import opens read-only: every editing affordance stays away, a banner carries sync and copy-to-editable, and CopyToEditableModal clones an import into an editable project. On the first open of a source with convertible PT9 data and no stored state, Pt9ConvertPromptModal offers the conversion up front: Yes runs the import as the only project created, No (or dismissing) persists the empty draft so the offer never repeats, per the user-questions entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Loader imports isPt9ImportReport from the converter surface that now owns it, the frontend-mock helper rides this tree with its users, and the offer and hasDraft tests use the shared ENOENT fixture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Seal the read-only import and make its sync land Every editing affordance the read-only import view still offered is gone, and a sync now reaches the view it refreshed. - Merge, baseline-split, and arc-split controls read the store's read-only flag; the import view's segmentation dispatch is inert as a backstop, and a phrase mode entered on the draft no longer carries into the import. - The import's analysis is cleared before each fetch, so the store's mount-time seed cannot pin pre-sync content in the view; a fetch that brings back nothing says so in the view area. - The accepted first-open offer gets its intended single-door report. - Opening an import runs through the select modal's submit guard, and the first-open probe gives up rather than hanging the tab. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Bound the manifest read and catch the report's Open Reading a Paratext 9 manifest now goes through one helper that gives up when the provider never answers, so no caller can wait on it forever. - The select modal is held inert for the whole of an import open, so a hung manifest read had left it with no Escape, no outside-click, and a disabled Cancel. A read that never answers is now an ordinary failure: one warning, the stored import opens, the modal comes back. - The first-open probe reads through the same helper and keeps its plain try/catch. - A rejecting fetch behind the report's Open is logged and notified rather than escaping the click handler unhandled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Tag the import analysis with the version it was fetched for The view derives what to show from the tag rather than having an effect clear the previous analysis: a fetched analysis and the version it belongs to now reach the view in the same commit, so the commit that carries a sync's new modification time has no pre-sync analysis to paint - previously it mounted the whole interlinear tree on the old content for a frame before the placeholder replaced it. Also records that the manifest timeout bounds the wait rather than the read, PAPI offering no cancellation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Trim the import-analysis comments to what outlives the code Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Report a failed import load once, in the panel The imported-analysis fetch sent a toast on top of the panel's own failure line, so one failure produced two messages with different advice - and the toast in the catch ran even for a fetch a sync or a switch back to the draft had already superseded. The panel line is now the whole report: it stays on screen next to the empty view instead of disappearing. Also seals two ways a stale mode or hover could outlive the control it came from: the import view pins its phrase mode to view, since the reset effect only covers crossing into the import and a mode set from inside it has no crossing to reset it; and a split hover clears when the analysis turns read-only, since the button that vanishes never fires its own mouse-leave. The read-only mock boilerplate that had been copied into three test files moves to a module of its own, and resets in beforeEach rather than afterEach. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Drop an Open fetch the user has already walked away from Adopted from the review fixes on #295. The report stays dismissable while the Open's summary fetch is in flight, so Escape back to the picker and then a summary landing switched the active project and closed the picker the user had returned to. The handler now checks the report is still the modal on screen before acting, which also keeps its failure notice out of whatever they moved on to. Two smaller things from the same review: the failed-load line no longer prints under "Loading..." or a book error, which would have contradicted them; and `Pt9ImportModal`'s `onOpen` doc said Open renders in `import` mode alone, which stopped being true when the offer report was given its Open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
160aee6 to
750aa55
Compare
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 21 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on jasonleenaylor).
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 7 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on jasonleenaylor).
|
All five landed, but through #292 rather than through me, so I checked them against the current head (160aee6) rather than taking either of your words for it. Read-only holes: the merge control in You spotted the overlap yourself and closed #295 within half an hour of opening it, which saved everyone the reconciliation. Thanks for that. One difference worth naming rather than letting the merge decide it silently. You used a Your two judgment items and the rejected finding I'll answer separately. |
isPlatformError only tests for a platformErrorVersion property, so a value that passes it is not proven to carry a message even though PlatformError declares one as required. isPt9TooLargeError then read .includes off it. Every newPlatformError path does set message, and it is deliberately made enumerable so it survives the iframe boundary, so no platform producer reaches this. It is guarded anyway because the call sits inside a catch: a throw here escapes runPt9Import's handler and turns a failed import into an unhandled rejection rather than the reported failure the user should see. The RESOURCE_EXHAUSTED code check short-circuits first and is unaffected. The plain-Error branch needs nothing, since Error guarantees a string message through its prototype. Found by alex-rawlings-yyc while reviewing #274, who raised it as a robustness question rather than guessing at the producers.
The UI half of the PT9 import, stacked on #273 (converter: #272).
the source serves convertible data (usePt9ImportAvailability probes the
manifest through the projectInterface).
the too-large refusal, recognized by the RESOURCE_EXHAUSTED platform error
code with the documented message marker as fallback (pt9-import-error).
carries manual sync and copy-to-editable, and CopyToEditableModal clones an
import into an editable project through the normal draft-open flow.
stored interlinearizer state asks "This project has Paratext 9 interlinear
data. Would you like to convert it now?" Yes runs the import as the only
project created; No (or dismissing) persists the empty draft so the offer
never repeats. Recorded in user-questions.md for outside review.
This change is