Wire up template-based project creation via JSON snapshot import#2281
Open
myieye wants to merge 6 commits into
Open
Wire up template-based project creation via JSON snapshot import#2281myieye wants to merge 6 commits into
myieye wants to merge 6 commits into
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements end-to-end template-based project creation, allowing users to create blank projects with custom codes and vernacular writing systems. It adds a SQL template resource with GUID hydration and hash-chain rebuild logic, backend APIs for template-based creation, frontend UI flows, comprehensive test infrastructure, and localization updates. ChangesTemplate-based project creation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
Bot
added
the
💻 FW Lite
Contributor
Contributor
C# Unit Tests165 tests 165 ✅ 19s ⏱️ Results for commit 7a38c81. ♻️ This comment has been updated with latest results. |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
myieye
force-pushed
the
claude/investigate-issue-1920-f6yHG
branch
from
5d1291f to
5d41a94
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs (1)
211-225: ⚡ Quick winReturn the CRDT tasks directly instead of wrapping them in
Task.Run.These methods already delegate to async APIs. The extra
Task.Runjust adds a thread-pool hop and makes cancellation/context flow harder to reason about.Suggested fix
[JSInvokable] - public Task CreateProject(string name, string code, WritingSystemId vernacularWs) - { - return Task.Run(async () => - { - await crdtProjectsService.CreateProjectFromTemplate(new(name, code, Role: UserProjectRole.Manager, VernacularWs: vernacularWs)); - }); - } + public Task CreateProject(string name, string code, WritingSystemId vernacularWs) => + crdtProjectsService.CreateProjectFromTemplate( + new(name, code, Role: UserProjectRole.Manager, VernacularWs: vernacularWs)); [JSInvokable] - public Task CreateDemoProject(string name) - { - return Task.Run(async () => - { - await crdtProjectsService.CreateExampleProject(name); - }); - } + public Task CreateDemoProject(string name) => + crdtProjectsService.CreateExampleProject(name);🤖 Prompt for 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. In `@backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs` around lines 211 - 225, CreateProject and CreateDemoProject are wrapping already-async crdtProjectsService calls in Task.Run, causing unnecessary thread-pool hops and lost context/cancellation; remove the Task.Run wrappers and return the underlying tasks directly (either by making the methods async and awaiting crdtProjectsService.CreateProjectFromTemplate(...) and crdtProjectsService.CreateExampleProject(...), or by returning those Task-returning calls directly) while keeping the [JSInvokable] attribute on CreateDemoProject and preserving the parameter types (name, code, vernacularWs) and role construction used with CreateProjectFromTemplate.
🤖 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 `@backend/FwLite/FwLiteWeb/Routes/ProjectRoutes.cs`:
- Around line 84-89: The ValidateProjectCode method performs ProjectExists(code)
before enforcing the project code regex, which can allow malformed/path-like
input to reach storage; reorder the checks in ValidateProjectCode so the format
validation using CrdtProjectsService.ProjectCode().IsMatch(code) runs
immediately after the null/whitespace check and before calling
projectService.ProjectExists(code), returning the existing "Only letters,
numbers, '-' and '_' are allowed" BadRequest when the regex fails; keep the
other checks and return values unchanged.
In `@backend/FwLite/LcmCrdt/CrdtProjectsService.cs`:
- Around line 197-212: The current OverwriteProjectDataRow performs an
unfiltered ExecuteUpdateAsync on dbContext.ProjectData which can overwrite every
row; add a fail-fast guard that restricts the update to the intended row and
verifies existence before mutating: first query (e.g., using Any/Count or
FirstOrDefault) to ensure a single ProjectData with the given data.Id exists
(throw if not exactly one), then call ExecuteUpdateAsync on
dbContext.ProjectData with a Where filter for p => p.Id == data.Id so only that
row is updated; reference the OverwriteProjectDataRow method, the ProjectData
DbSet, and the ExecuteUpdateAsync call when making these changes.
- Around line 158-183: The template creation flow calls
CurrentProjectService.SetupProjectContextForNewDb(crdtProject) but then awaits
RefreshProjectData() without assigning its return value, leaving
crdtProject.Data null before invoking request.AfterCreate; change the code to
assign the result of currentProjectService.RefreshProjectData() to
crdtProject.Data (i.e., crdtProject.Data = await
currentProjectService.RefreshProjectData()) so downstream callers like
request.AfterCreate receive the initialized ProjectData.
In `@backend/FwLite/LcmCrdt/CurrentProjectService.cs`:
- Around line 116-118: The current logic in CurrentProjectService that checks
projectData (variable projectData) and then skips morph-type seeding if
projectData is null causes a permanent skip while the one-time migration cache
is set; change the flow so that a null ProjectData does not mark the
migration/seed as completed and does not block future attempts: ensure you only
compute PreDefinedData.MorphTypesSeedCommitId(projectData.Id) and run the Commit
existence check when projectData != null, but do not set the migration cache or
mark seeding as done when projectData is null—instead leave the cache unset (or
schedule a retry) so morph-type seeding (the Commit/seed path) will be retried
once ProjectData becomes available; update the code around
dbContext.ProjectData, PreDefinedData.MorphTypesSeedCommitId, Commit and the
migration cache logic to implement this behavior.
In `@backend/FwLite/LcmCrdt/Project/ProjectTemplate.cs`:
- Around line 48-60: The template replacement injects request-derived strings
(vernacularWs.Code and the AbbreviationFor(...) fallback) directly into
resolvedSql via Replace, which can break or inject SQL; before calling
HydrateGuids(...).Replace(...) validate or strictly whitelist the writing-system
values used for VernacularWsPlaceholder and VernacularNamePlaceholder and the
AbbreviationFor(...) result (e.g., allow only a tight regex like
alphanumeric/underscore/hyphen) and reject the request if they don't match, or
alternatively escape them safely for SQL string literals; ensure this validation
is applied to vernacularWs.Code and the value returned by
AbbreviationFor(vernacularWs) and only then build cmd.CommandText (resolvedSql +
"\nPRAGMA writable_schema=RESET;") so no unvalidated user data can break
templateSql or targetSqlitePath usage.
In `@frontend/viewer/src/home/HomeView.svelte`:
- Around line 79-81: Trim the user-provided inputs (customNewProjectName and
customNewProjectCode) before deciding whether to use them or fall back to
defaults: compute trimmedName = customNewProjectName?.trim() and trimmedCode =
customNewProjectCode?.trim(), then use trimmedName ||
`blank-dev-${dateTimeProjectSuffix()}` for projectName and trimmedCode ||
projectName for projectCode, and pass those values into
projectsService.createProject to prevent whitespace-only values from being
treated as valid.
In `@frontend/viewer/src/lib/services/projects-service.ts`:
- Around line 30-40: The current input validation in createProject and
createDemoProject (methods in projects-service.ts) uses falsy checks (if
(!name), if (!code), if (!vernacularWs)) which permit whitespace-only strings;
update both createProject and createDemoProject to trim inputs first and reject
when trimmed values are empty (e.g., const trimmedName = name.trim(); if
(!trimmedName) throw ...), and use the trimmed values when building the request
URL so whitespace-only values are rejected client-side before sending to the
backend.
In `@frontend/viewer/src/locales/ko.po`:
- Around line 435-438: Add Korean translations for the newly introduced
blank-project strings by filling the empty msgstr entries in
frontend/viewer/src/locales/ko.po: provide a localized Korean string for the
msgid "Create Blank Project" and do the same for the two other new msgid entries
added in the same blank-project flow (the entries noted at the ranges around
lines 1466-1469 and 2025-2028), ensuring each msgstr contains the correct Korean
translation and preserves any punctuation/formatting from the msgid.
In `@frontend/viewer/src/locales/ms.po`:
- Around line 435-438: Fill in the missing Malay translations for the new
blank-project message keys in frontend/viewer/src/locales/ms.po: set msgstr for
the shown msgid "Create Blank Project" and the two other blank msgid/msgstr
pairs referenced at ranges 1466-1469 and 2025-2028; update each msgstr with the
correct Malay translation (ensure UTF-8 encoding and preserve the original msgid
lines and PO file formatting).
In `@frontend/viewer/src/locales/sw.po`:
- Around line 435-438: The PO entries for the new project-creation strings
(e.g., msgid "Create Blank Project" in viewer/src/locales/sw.po and the other
untranslated entries at the ranges you noted) are missing Swahili translations;
open viewer/src/locales/sw.po, locate each untranslated msgid (like "Create
Blank Project") and fill in the corresponding msgstr with an accurate Swahili
translation, making sure none remain empty and preserving plural/context markers
and file references; after updating, run your i18n validation/compile step to
ensure the PO file is syntactically valid.
In `@frontend/viewer/src/locales/vi.po`:
- Around line 435-438: Several new Vietnamese translations are missing: locate
the msgid entries such as "Create Blank Project" (and the other empty msgstr
blocks noted around the other ranges) in frontend/viewer/src/locales/vi.po and
provide appropriate Vietnamese translations by filling each msgstr with the
correct Vietnamese text (e.g., a natural translation for "Create Blank Project")
for all empty entries referenced (including the blocks at the other mentioned
ranges) so the new blank-project UI displays localized text.
---
Nitpick comments:
In `@backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs`:
- Around line 211-225: CreateProject and CreateDemoProject are wrapping
already-async crdtProjectsService calls in Task.Run, causing unnecessary
thread-pool hops and lost context/cancellation; remove the Task.Run wrappers and
return the underlying tasks directly (either by making the methods async and
awaiting crdtProjectsService.CreateProjectFromTemplate(...) and
crdtProjectsService.CreateExampleProject(...), or by returning those
Task-returning calls directly) while keeping the [JSInvokable] attribute on
CreateDemoProject and preserving the parameter types (name, code, vernacularWs)
and role construction used with CreateProjectFromTemplate.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 53d155a9-ef29-4a5b-b169-3e9703e04d6a
📒 Files selected for processing (25)
backend/FwLite/FwLiteProjectSync.Tests/ProjectTemplateTests.ApplyTemplate.verified.txtbackend/FwLite/FwLiteProjectSync.Tests/ProjectTemplateTests.csbackend/FwLite/FwLiteProjectSync.Tests/SqliteDump.csbackend/FwLite/FwLiteProjectSync.Tests/TemplateGuidScrubbing.csbackend/FwLite/FwLiteShared/Projects/CombinedProjectsService.csbackend/FwLite/FwLiteWeb/Routes/ProjectRoutes.csbackend/FwLite/LcmCrdt.Tests/OpenProjectTests.csbackend/FwLite/LcmCrdt/CrdtProjectsService.csbackend/FwLite/LcmCrdt/CurrentProjectService.csbackend/FwLite/LcmCrdt/LcmCrdt.csprojbackend/FwLite/LcmCrdt/Objects/PreDefinedData.csbackend/FwLite/LcmCrdt/Project/ProjectTemplate.csbackend/FwLite/LcmCrdt/Templates/template.sqlfrontend/viewer/src/home/HomeView.sveltefrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/ICombinedProjectsService.tsfrontend/viewer/src/lib/services/projects-service.tsfrontend/viewer/src/locales/en.pofrontend/viewer/src/locales/es.pofrontend/viewer/src/locales/fr.pofrontend/viewer/src/locales/id.pofrontend/viewer/src/locales/ko.pofrontend/viewer/src/locales/ms.pofrontend/viewer/src/locales/sw.pofrontend/viewer/src/locales/vi.pofrontend/viewer/src/project/demo/in-memory-demo-api.ts
💤 Files with no reviewable changes (1)
- backend/FwLite/LcmCrdt/Objects/PreDefinedData.cs
github-actions
Bot
added
the
📦 Lexbox
myieye
force-pushed
the
claude/investigate-issue-1920-f6yHG
branch
from
636dbe5 to
00c93ac
Compare
myieye
force-pushed
the
claude/investigate-issue-1920-f6yHG
branch
3 times, most recently
from
c28ad1f to
a8efac4
Compare
myieye
added a commit
that referenced
this pull request
Jun 2, 2026
Comment-only pass on PR #2281: - Fix a stale claim in CurrentProjectService: after the ReseedProject switch, templated projects no longer resolve their morph-types commit to MorphTypesSeedCommitId, so the re-seed adds one redundant (harmless) commit on first open rather than deduping. - Drop duplicated rationale (SeedMorphTypes stated 3x → param doc only; local-only stated 2x → SyncService.UploadProject only). - Trim over-long test comments; drop a reference to a renamed test. Bumps the harmony submodule to b043d87 (CommitBase.cs LF normalization). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
hahn-kev
reviewed
Jun 10, 2026
myieye
changed the title
myieye
added a commit
that referenced
this pull request
Jun 17, 2026
Comment-only pass on PR #2281: - Fix a stale claim in CurrentProjectService: after the ReseedProject switch, templated projects no longer resolve their morph-types commit to MorphTypesSeedCommitId, so the re-seed adds one redundant (harmless) commit on first open rather than deduping. - Drop duplicated rationale (SeedMorphTypes stated 3x → param doc only; local-only stated 2x → SyncService.UploadProject only). - Trim over-long test comments; drop a reference to a renamed test. Bumps the harmony submodule to b043d87 (CommitBase.cs LF normalization). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
myieye
force-pushed
the
claude/investigate-issue-1920-f6yHG
branch
2 times, most recently
from
4cafdb7 to
bb3fe8e
Compare
Squash of the template-creation feature, rebased onto develop's harmony-NuGet migration. Imports a persisted ProjectSnapshot (template.json) via the MiniLcm import path; no SQL template, no ReseedProject. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Revert the server-side CrdtCommits composite (ProjectId, Id) primary-key change out of this PR — it's unrelated to template-based project creation. The change is preserved as a git patch (plus a unique-index follow-up) in #2365. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Naming: rename the ApplyTemplate test to VerifyCreateFromTemplate, the embedded template to blank-project-template.json, and the create endpoints to /api/project/create and /api/project/create-demo. Regenerate the verified snapshot. Drop now-redundant seeding: with every create path going through the template, the SeedMorphTypes flag and the test-only SeedNewProjectData / SeedSystemData path are dead, along with the predefined parts-of-speech / complex-form-type / semantic-domain / custom-view seeders and their hard-coded GUIDs. ExampleProjectData resolves Noun and Compound by name instead. PreDefinedData keeps only the morph-type seeder still used by the open-time backfill. Revert unrelated drift back to develop (CurrentProjectService morph-type backfill, SyncService.UploadProject), drop the in-memory template cache in favour of reading the embedded snapshot per use, remove two redundant templated-project tests, and fix stale/backwards comments (create is dev-only and demo is user-facing; the snapshot serialization keeps a complete record, it isn't required by the sync diff). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Make CreateProjectFromTemplate take vernacular (required) + analysis (optional) writing systems as explicit params instead of optional CreateProjectRequest properties, so the required one is enforced at compile time. - Create the requested writing systems by adding them to the loaded template snapshot before import (everything created together, in dependency order) rather than a post-import CreateWritingSystem call. - Let callers specify an analysis WS end-to-end (endpoint, JSInvokable, dialog); English always ships in the template, the requested one is added if different. - Restore NewProject_HasAllCanonicalMorphTypes (the open-time backfill keeps it green) instead of deleting it; SanitizeProjectCode now throws rather than inventing a fallback code. - New Project button: show "(Local only)" and drop the "Add your own entries" line. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
I shouldn't have dropped the second line — only the first-line "(Local only)" caveat was requested. Bring back the subtitle as "Create a new FieldWorks Lite project". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Neutralizes a CWE-117 log-injection finding on the EnsureDeleteProject log lines. The value is already effectively guarded (project code is regex- validated, the only path-supplying caller composes a config root + GUID), so this is defense-in-depth rather than a live vuln. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
myieye
force-pushed
the
claude/investigate-issue-1920-f6yHG
branch
from
9d52617 to
7a38c81
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This feature is still dev-only, because it's a dead-end and we don't want to sell it as being more than that. The "Create example project" is what we still expose to our users, because it's so explicitly a demo feature. But it now also uses the template, so it gets the canonical morph-types, sem-doms, PoS etc.