[PB Extension] Create FW project - #2394
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a lexicon creation form, localized UI strings, a creation command, MiniLCM project creation integration, project-type URL routing, and selection web-view states for creating and selecting lexicons. ChangesLexicon creation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
imnasnainaec
changed the title
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
Generally looks fine to me. 👍
|
|
||
| function isValidLangTag(tag: string): boolean { | ||
| try { | ||
| return Intl.getCanonicalLocales(tag).length === 1; |
There was a problem hiding this comment.
What's the === 1 about? I'd expect >= 1.
There was a problem hiding this comment.
Intl.getCanonicalLocales called on a string (instead of on an array) returns an array of length at most 1. The length check would need to be changed if we refactored the param from tag: string to tags: string[].
| async getProjects(): Promise<IProjectModel[]> { | ||
| return (await this.fetchPath('localProjects')) as IProjectModel[]; | ||
| const projects = (await this.fetchPath('localProjects')) as IProjectModel[]; | ||
| projects.forEach((p) => FwLiteApi.projectTypeByCode.set(p.code, p.crdt ? 'Harmony' : 'FwData')); |
There was a problem hiding this comment.
If I'm not mistaken, these projects are sometimes flagged as both crdt and fwdata.
There was a problem hiding this comment.
Correct, and if the project has both flags, don't we want to default to Harmony interaction?
There was a problem hiding this comment.
I'm not entirely sure, but I think it makes sense to surface both project types. 😕
Paratext users are probably more on the power-user side of things, which means some would likely benefit from using their fwdata/FLEx project directly.
That probably needs a bit of thought, because it sort of means
Perhaps the project-type needs to be considered part of a lexicon's identifier?
In FWL we display all projects with the harmony and fwdata ones in separate lists (which means some are in both lists). Then when a project is open the URL tells us if we're working on the harmony or fwdata copy.
There was a problem hiding this comment.
Possible follow-up issue: #2394 (comment)
26cae9e to
77b6250
Compare
Address review feedback on #2394: - validateLexiconCode checked `.analysis` truthiness, but writing-system fields are arrays, so an empty array always passed. Require a vernacular writing system instead (analysis is optional; selectLexicon handles its absence). - The create-lexicon form now blocks codes that already exist before the create round-trips, showing a localized message instead of a raw 400. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ee4770c to
07ce2d1
Compare
Address review feedback on #2394: - validateLexiconCode checked `.analysis` truthiness, but writing-system fields are arrays, so an empty array always passed. Require a vernacular writing system instead (analysis is optional; selectLexicon handles its absence). - The create-lexicon form now blocks codes that already exist before the create round-trips, showing a localized message instead of a raw 400. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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.
|
@myieye Thanks for your previous review and approval! I've made enough changes, I think additional review is warranted. |
|
@imnasnainaec here are my notes. I found one major bug, one that results from it, and a couple nits:
|
| async getProjects(): Promise<IProjectModel[]> { | ||
| return (await this.fetchPath('localProjects')) as IProjectModel[]; | ||
| const projects = (await this.fetchPath('localProjects')) as IProjectModel[]; | ||
| projects.forEach((p) => FwLiteApi.projectTypeByCode.set(p.code, p.crdt ? 'Harmony' : 'FwData')); |
There was a problem hiding this comment.
I'm not entirely sure, but I think it makes sense to surface both project types. 😕
Paratext users are probably more on the power-user side of things, which means some would likely benefit from using their fwdata/FLEx project directly.
That probably needs a bit of thought, because it sort of means
Perhaps the project-type needs to be considered part of a lexicon's identifier?
In FWL we display all projects with the harmony and fwdata ones in separate lists (which means some are in both lists). Then when a project is open the URL tells us if we're working on the harmony or fwdata copy.
| 'lexicon.createLexicon', | ||
| async (name: string, code: string, vernacularWs: string, analysisWs?: string) => { | ||
| try { | ||
| await fwLiteApi.createProject(name, code, vernacularWs, analysisWs); |
There was a problem hiding this comment.
At least we know that this creates a harmony project.
Although...it would probably be pretty easy to extend the api for whipping up an fwdata project instead 😆.
Address review feedback on #2394: - validateLexiconCode checked `.analysis` truthiness, but writing-system fields are arrays, so an empty array always passed. Require a vernacular writing system instead (analysis is optional; selectLexicon handles its absence). - The create-lexicon form now blocks codes that already exist before the create round-trips, showing a localized message instead of a raw 400. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
e692e79 to
7b2c404
Compare
|
Note Claude-drafted — potential follow-up issue, not filed. Captures @myieye's review thread on [P.B ext] Distinguish FwData vs Harmony copies of the same lexiconFollow-up to review discussion on this PR (thread by @myieye). ProblemThe extension keys a project's API type by lexicon code alone. In
Why it mattersParatext users skew power-user; some would legitimately want to work against their existing fwdata/FLEx project rather than a CRDT copy. FW Lite itself surfaces Harmony and FwData projects in separate lists (a dual-flagged project appears in both), and disambiguates via the open URL ( Proposed direction
Out of scopeLexicon creation always produces a Harmony project (this PR), so Relationship to #2461#2461 (remote project selection) adds a location axis (local vs. remote/Lexbox); this adds a type axis (FwData vs. Harmony). Both break the same assumption that |
Addresses later review feedback on #2394: - Show a validation message for an invalid vernacular or analysis language tag in the create-lexicon form; previously the Create button was disabled with no explanation (per myieye). - Re-check a project's stored lexicon code before acting on it; if it no longer resolves (e.g. deleted in FW Lite) clear it and reopen the selector instead of routing Browse/Add/Find to a dead endpoint (per alex-rawlings-yyc). - Add a DEV-ONLY lexicon switcher (lexicon.changeLexicon menu command) so the selector is reachable after a lexicon is chosen; marked for removal before release. Canonical switch remains clearing lexicon.lexiconCode in project settings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review feedback on #2394: - validateLexiconCode checked `.analysis` truthiness, but writing-system fields are arrays, so an empty array always passed. Require a vernacular writing system instead (analysis is optional; selectLexicon handles its absence). - The create-lexicon form now blocks codes that already exist before the create round-trips, showing a localized message instead of a raw 400. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses later review feedback on #2394: - Show a validation message for an invalid vernacular or analysis language tag in the create-lexicon form; previously the Create button was disabled with no explanation (per myieye). - Re-check a project's stored lexicon code before acting on it; if it no longer resolves (e.g. deleted in FW Lite) clear it and reopen the selector instead of routing Browse/Add/Find to a dead endpoint (per alex-rawlings-yyc). - Add a DEV-ONLY lexicon switcher (lexicon.changeLexicon menu command) so the selector is reachable after a lexicon is chosen; marked for removal before release. Canonical switch remains clearing lexicon.lexiconCode in project settings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
18b33f9 to
036841a
Compare
`fetchUrl` now reads the response as text before conditionally parsing JSON, so endpoints that return an empty 200 (like DELETE entry and the new project/create route) no longer throw a SyntaxError. `FwLiteApi.createProject` wraps `POST /api/project/create`, which was added in #2281 to resolve #1920. Closes #2061. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds an end-to-end flow for creating a new FW Lite CRDT project from within the SelectLexicon dialog: - New `lexicon.createLexicon` PAPI command wrapping FwLiteApi.createProject - New CreateLexicon form component (name, auto-derived code, vernacular WS pre-filled from the Paratext project language, optional analysis WS) - "Create new lexicon" button in LexiconComboBox opens the form - On successful creation the new lexicon is auto-selected and the dialog shows the standard "Lexicon selection saved" confirmation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…key order - fw-lite-api: add static projectTypeByCode cache (shared across FwLiteApi instances including EntryService) so CRDT projects created via createProject route through mini-lcm/Harmony/... instead of mini-lcm/FwData/... getProjects() populates the cache; createProject() seeds 'Harmony' immediately so the writingSystems call in selectLexicon succeeds without a re-fetch - fw-lite-api: include response body text in error messages so backend validation failures (duplicate code, invalid lang tag, etc.) reach the logger - create-lexicon: validate vernacular and analysis language tags with Intl.getCanonicalLocales() before enabling the submit button - localized-string-keys / localizedStrings.json: restore alphabetical ordering Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- deriveCode now replaces invalid characters with '-' instead of removing them, matching the backend SanitizeProjectCode behavior - WebView onCreated catches selectLexicon failure: logs the error, refreshes the lexicon list, and falls back to the combo box so the user can manually select the project that was created on disk Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The Svelte frontend routes FwData projects via /paratext/fwdata/{code}
and CRDT/Harmony projects via /paratext/project/{code}. The standalone
getBrowseUrl always used 'fwdata', producing a broken URL for any
Harmony lexicon.
Move getBrowseUrl into FwLiteApi as a method so it can consult the
projectTypeByCode cache and emit 'project' vs 'fwdata' accordingly.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Hints the default analysis writing system on the Create new lexicon form, per review feedback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Display the backend error message (e.g. duplicate code, invalid language tag) instead of only logging it, per review feedback. Also drop the now- pointless useCallback around handleSubmit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ping
- createLexicon now returns the backend validation message (e.g. "Project
already exists", "not a valid IETF language tag") instead of a generic
failure, so the create-lexicon form shows the actual reason. Also logs the
real message rather than JSON.stringify(e), which rendered Errors as {}.
- Type the SelectLexicon WebView provider and its caller as
LexiconWebViewOptions so vernacularLanguage is part of the declared contract.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cons The in-memory projectTypeByCode map is empty after an extension restart, so a previously created Harmony/CRDT lexicon was misrouted to the FwData endpoints (browse, search, and entry operations all failed) until the user reopened the lexicon selector. checkLexiconCode and getBrowseUrl now resolve the type via a new resolveProjectType helper, which reloads the project list from the backend on a cache miss before falling back to FwData. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
selectLexicon awaited the command but ignored its result, so onCreated and
LexiconComboBox.saveSetting showed a "saved" confirmation even if the command
returned { success: false }. Throw on a non-success result so the create flow
falls back to the combo box and the combo box surfaces its save error.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review feedback on #2394: - validateLexiconCode checked `.analysis` truthiness, but writing-system fields are arrays, so an empty array always passed. Require a vernacular writing system instead (analysis is optional; selectLexicon handles its absence). - The create-lexicon form now blocks codes that already exist before the create round-trips, showing a localized message instead of a raw 400. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
doesProjectMatchLangTag used Object.keys on the vernacular IWritingSystem[],
which yields index strings ('0', '1', ...) rather than language tags, so
getProjectsMatchingLanguage never matched and always fell back to all
projects. Map over the array and read each writing system's wsId instead.
Also drops a redundant comment on the lexicon-code validator.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Previously an invalid code (uppercase, spaces, symbols) silently disabled Submit with no feedback, unlike the codeExists case which already had a visible error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Auto-lowercase the code as typed (matching Lexbox's create-project zod schema) instead of erroring on uppercase, and add Lexbox's 4-character minimum length with its own message. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses later review feedback on #2394: - Show a validation message for an invalid vernacular or analysis language tag in the create-lexicon form; previously the Create button was disabled with no explanation (per myieye). - Re-check a project's stored lexicon code before acting on it; if it no longer resolves (e.g. deleted in FW Lite) clear it and reopen the selector instead of routing Browse/Add/Find to a dead endpoint (per alex-rawlings-yyc). - Add a DEV-ONLY lexicon switcher (lexicon.changeLexicon menu command) so the selector is reachable after a lexicon is chosen; marked for removal before release. Canonical switch remains clearing lexicon.lexiconCode in project settings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
isLexiconCodeValid and resolveProjectType previously collapsed every error into "the lexicon is gone" / "default to FwData", so FW Lite still starting up (or any other transient failure) could silently clear a project's stored lexicon choice or misroute a Harmony project. Add HttpStatusError to distinguish a real backend response from a request that never reached it at all, and only treat the former as confirmed-invalid. resolveProjectType now propagates a getProjects failure instead of guessing, since that endpoint has no per-code failure mode to misinterpret. Full correctness (narrowing to a 404-specific check) is blocked on #2487, which tracks the backend currently returning 500 for "not found" too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ge filtering Backend error responses are JSON either way (a bare string from Results.BadRequest/NotFound, or a ProblemDetails object from Results.Problem), but fetchUrl surfaced the raw response text verbatim, so create-lexicon validation errors reached the user still wrapped in JSON-string quotes. extractErrorMessage unwraps either shape. getProjectsMatchingLanguage used Promise.all, so one project's failed writingSystems lookup (e.g. a broken/deleted project) rejected the whole batch and silently fell back to the unfiltered project list. Switch to Promise.allSettled so a single failure just drops that project instead of discarding every other project's match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
saveSetting caught a failed selectLexicon call, logged it, and cleared settingSaving without any visible sign of failure — the UI just silently returned to the combo box. Add a saveError state and render it next to the combo box, reusing the existing (previously log-only) %lexicon_selectLexicon_saveError% string's intent. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
080a42f to
91c442b
Compare
3 participants
Lets users create a brand-new FW Lite lexicon directly from the lexicon selector in Platform.Bible instead of only picking an existing one. This wires up the backend create endpoint and the project-type-aware URL routing the flow needs, and fixes a few adjacent bugs (empty-response parsing, language-tag filtering, lexicon-code validation) surfaced along the way.
Summary
FwLiteApi.createProject(name, code, vernacularWs, analysisWs?)to the Platform.Bible extension's HTTP client, wrapping thePOST /api/project/createendpoint added in Wire up template-based project creation via JSON snapshot import #2281.fetchUrl: it previously calledresponse.json()unconditionally, which throws aSyntaxErroron empty-body responses (HTTP 200 with no content). The fix reads the body as text first and only parses JSON when the body is non-empty. On error responses it now throws with the response body, so backend validation messages survive. This also silently affecteddeleteEntry.FwDatavsHarmony/CRDT) so lexicon and browse URLs route to the correct backend endpoints. The lookup is self-healing: because the cache is in-memory only, on a miss (e.g. after an extension restart) it reloads the project list from the backend before falling back toFwData, so a previously created CRDT lexicon stays reachable without the user reopening the selector.lexicon.lexiconCodeproject-settings validator, which accepted any resolvable code because it tested an always-truthy array (.analysis); it now requires at least one vernacular writing system (analysis writing systems remain optional and are handled byselectLexicon).Changes
platform.bible-extension/src/utils/fw-lite-api.tsfetchUrl:results.json()→results.text()+ conditionalJSON.parse; throws the response body on error responsesFwLiteApi.createProject: new public method usingURLSearchParamsfor the create endpointprojectTypeByCodecache +resolveProjectType:checkLexiconCodeandgetBrowseUrl(now async) resolve a project's type, repopulating from the backend on a cache missdoesProjectMatchLangTag: reads each vernacular writing system'swsIdinstead ofObject.keyson the array (which returned index strings), sogetProjectsMatchingLanguageactually filters — a path the select-lexicon flow newly exercises by passingprojectIdtolexicon.lexiconsplatform.bible-extension/src/main.ts— registers thelexicon.createLexiconcommand (returns the backend error message on failure);validateLexiconCodenow checks for a vernacular writing system instead of the always-truthy.analysisarrayplatform.bible-extension/src/types/lexicon.d.ts—'lexicon.createLexicon'command handler type; optionalerroronSuccessHolderplatform.bible-extension/src/types/localized-string-keys.ts+contributions/localizedStrings.json— 9 new%lexicon_createLexicon_*%stringsplatform.bible-extension/src/components/create-lexicon.tsx— new form component; pre-checks the entered code against existing lexicons and surfaces backend error messagesplatform.bible-extension/src/components/lexicon-combo-box.tsx— optionalonCreateNewprop adds the "Create new lexicon" buttonplatform.bible-extension/src/utils/project-manager.ts— passes the project's vernacular language tag when opening the SelectLexicon WebViewplatform.bible-extension/src/web-views/select-lexicon.web-view.tsx— wires up the create flow with auto-selection on success, passing existing codes for the duplicate pre-checkplatform.bible-extension/src/web-views/index.tsx— types the SelectLexicon provider asLexiconWebViewOptionssovernacularLanguageis part of the declared contractImplemented by Claude Sonnet 4.6 & Opus 4.8.
Devin review: https://app.devin.ai/review/sillsdev/languageforge-lexbox/pull/2394
Closes #2061