feat(injectappmodel): inject TranslationTerm MetaModel per app - #252
Merged
Conversation
- Register TranslationTerm Spec first with EnsureServiceEntry and softDelete false. - Materialize a virtual service/index.ts, mutate in-memory entry for sibling Specs, and adopt builder entryPoint via Effects without writing package.json. - Add TranslationTermBaseModel with fields, unique-index ensure, and GetTranslations; widen backend OnResolve for virtual sources. - Revert Ensure'd ServiceEntryPoint after build/Persist so DB stays disk-sourced; include empty-entry hosts in app bundles for TranslationTerm. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change adds ChangesTranslationTerm injection and service entries
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ModuleBuilder
participant InjectAppModels
participant BackendPlugin
participant TranslationTermBaseModel
ModuleBuilder->>InjectAppModels: apply TranslationTerm injection
InjectAppModels->>TranslationTermBaseModel: generate translation-term model source
InjectAppModels-->>ModuleBuilder: return virtual files and ServiceEntryPath
ModuleBuilder->>BackendPlugin: SetEntryPoint(ServiceEntryPath)
BackendPlugin->>BackendPlugin: resolve virtual sources and load imports
ModuleBuilder->>ModuleBuilder: revert temporary service entry before persistence
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
PR Code Suggestions ✨No code suggestions found for the PR. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Drop registerMissingEntryVirtualSource so missing on-disk entries still fail as before (hooks tests). - Gate Ensure on Spec base model availability when ModulesPath exists, and avoid mutating ServiceEntryPoint during BundleInject. - Keep TranslationTermBaseModel side-effect-only (no core/service export) to preserve the export-surface unit test. Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
internal/module/artifact/build/injectappmodel/inject.go (1)
216-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
appendforout.Importsto match the file handling on Line 215.Line 215 appends to
out.Filesto preserve the virtual service entry. Line 216 replacesout.Imports. The two lines are only equivalent because nothing writesout.Importsearlier in this function. If a future ensure step adds an import, this line discards it.♻️ Proposed change
- out.Imports = []string{path} + out.Imports = append(out.Imports, path)🤖 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 `@internal/module/artifact/build/injectappmodel/inject.go` at line 216, Update the import assignment in the relevant injection function to append path to out.Imports rather than replacing the slice, matching the out.Files handling and preserving any imports added earlier.modules/core/service/orm/model/translation_term_base_model.ts (2)
176-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKey the DDL cache by dialect and table, and log the swallowed error.
ensuredUniqueIndexTablesstores only the table name. If two applications in the same process use the same table name against different databases, the second application skips the DDL. The emptycatchalso hides real failures, so a permanently missing unique index is invisible.♻️ Proposed change
- if (!table || ensuredUniqueIndexTables.has(table)) return; - - const dialect = String(($choysum as any)?.db?.dialectName || 'sqlite').toLowerCase(); + if (!table) return; + const dialect = String(($choysum as any)?.db?.dialectName || 'sqlite').toLowerCase(); + const cacheKey = `${dialect}\u0000${table}`; + if (ensuredUniqueIndexTables.has(cacheKey)) return; const indexName = `uq_${table}_key`; @@ - await exec.call(($choysum as any).db, ddl, '[]'); - ensuredUniqueIndexTables.add(table); + await exec.call(($choysum as any).db, ddl, '[]'); + ensuredUniqueIndexTables.add(cacheKey); } - } catch { + } catch (err) { // Best-effort: uniqueness still enforced when writers collide. + console.warn(`ensureTermUniqueIndex failed for ${table}:`, err); }🤖 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 `@modules/core/service/orm/model/translation_term_base_model.ts` around lines 176 - 201, Update ensureTermUniqueIndex to key ensuredUniqueIndexTables by both the normalized dialect and table name, so identical tables on different database dialects each execute their DDL. In the catch block, log the DDL execution error with sufficient context instead of silently swallowing it, while preserving the existing best-effort behavior.
282-285: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftAvoid reading and hashing the full language catalog on every request.
Searchignoreslimit: 0because it applies limits only when the value is truthy, so this call returns all matching rows. Cache the catalog/hash or update it when terms change to avoid repeated language-scoped full reads and in-memory hashing for large catalogs.🤖 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 `@modules/core/service/orm/model/translation_term_base_model.ts` around lines 282 - 285, Update the catalog lookup around TranslationTermBaseModel.Search so requests do not repeatedly read and hash every language-scoped row. Reuse a cached catalog/hash and invalidate or refresh it when translation terms change, or otherwise maintain the hash incrementally through term mutations. Ensure normal request handling avoids full-catalog reads while preserving correct hashes after updates.internal/module/artifact/build/injectappmodel/session.go (1)
169-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStart the exported doc comment with the exported name.
The comment documents
revertEnsuredServiceEntry, but it is attached toRevertEnsuredServiceEntry. Go doc convention and thereviveexported rule require the comment to begin with the exported identifier.♻️ Proposed change
-// revertEnsuredServiceEntry restores Module.ServiceEntryPoint when Ensure mutated -// it for this build. Exported for Persist so DB stays aligned with package.json -// (cold builds always re-Ensure from an empty disk entry). +// RevertEnsuredServiceEntry restores Module.ServiceEntryPoint when Ensure mutated +// it for this build. Persist calls it so the DB stays aligned with package.json +// (cold builds always re-Ensure from an empty disk entry).🤖 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 `@internal/module/artifact/build/injectappmodel/session.go` around lines 169 - 174, Update the doc comment above Session.RevertEnsuredServiceEntry to begin with the exact exported identifier “RevertEnsuredServiceEntry”, while preserving the existing explanation of its behavior.
🤖 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 `@internal/module/artifact/build/injectappmodel/bundle.go`:
- Around line 82-89: Update the EnsureServiceEntry branch around
spec.EnsureServiceEntry to resolve relative ServiceEntryPoint paths against
mod.Path before calling os.Stat, while preserving absolute paths. Create the
VirtualFile only when os.Stat returns os.IsNotExist(err); propagate permission
and all other stat errors instead of treating them as missing files.
In `@internal/module/artifact/build/injectappmodel/helpers.go`:
- Around line 54-55: Update the os.Stat error handling in Ensure to return true
only when the error indicates os.ErrNotExist; return false for permission or
other filesystem errors so inaccessible module roots are rejected.
In `@internal/module/artifact/build/injectappmodel/inject.go`:
- Around line 192-205: Keep the ensured service entry build-local: update
materializeInject and the Session/Effects flow in
internal/module/artifact/build/injectappmodel/inject.go:192-205 and
session.go:155-167 to store the effective entry path in Session/Effects, then
pass that value to the builder and plugins instead of mutating shared
meta.Module.ServiceEntryPoint. Ensure BuildWithoutPersist failure paths,
including updatePrebuildResult, build, and validate, do not leave shared module
state changed.
In `@internal/module/lifecycle/bundles.go`:
- Around line 102-104: Update the TranslationTerm bundle-selection flow around
pickTranslationTermOwnerModule to derive candidate modules and ordered
applications from eligible installedMods rather than backendMods filtered by
non-empty ServiceEntryPoint, so empty-entry modules reach the supported
selection path and TranslationTerm is injected. Add a regression test covering
an application with no pre-existing service entry.
In `@modules/core/service/orm/model/translation_term_base_model.ts`:
- Around line 186-187: The MySQL path in the unique-index creation logic must
avoid retrying an already-existing index. Before executing the DDL built in the
mysql branch, query information_schema.statistics for the current database,
table, and indexName and skip creation when it exists; ensure the existing
ensuredUniqueIndexTables tracking is updated for both existing and newly created
indexes.
- Around line 288-313: Update the method’s query and processing flow so
computeTermHash receives every row for the requested language, without applying
module_names filtering first. Preserve module_names filtering and the
KIND_LITERAL check only when building termsByModule, while leaving the
unchanged-hash response behavior intact.
---
Nitpick comments:
In `@internal/module/artifact/build/injectappmodel/inject.go`:
- Line 216: Update the import assignment in the relevant injection function to
append path to out.Imports rather than replacing the slice, matching the
out.Files handling and preserving any imports added earlier.
In `@internal/module/artifact/build/injectappmodel/session.go`:
- Around line 169-174: Update the doc comment above
Session.RevertEnsuredServiceEntry to begin with the exact exported identifier
“RevertEnsuredServiceEntry”, while preserving the existing explanation of its
behavior.
In `@modules/core/service/orm/model/translation_term_base_model.ts`:
- Around line 176-201: Update ensureTermUniqueIndex to key
ensuredUniqueIndexTables by both the normalized dialect and table name, so
identical tables on different database dialects each execute their DDL. In the
catch block, log the DDL execution error with sufficient context instead of
silently swallowing it, while preserving the existing best-effort behavior.
- Around line 282-285: Update the catalog lookup around
TranslationTermBaseModel.Search so requests do not repeatedly read and hash
every language-scoped row. Reuse a cached catalog/hash and invalidate or refresh
it when translation terms change, or otherwise maintain the hash incrementally
through term mutations. Ensure normal request handling avoids full-catalog reads
while preserving correct hashes after updates.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: da3d615e-ca16-44a1-8d7c-6438ddacc003
📒 Files selected for processing (18)
internal/esbplugins/backendplugin/plugin.gointernal/module/artifact/build/backend/builder.gointernal/module/artifact/build/backend/builder_test.gointernal/module/artifact/build/backend/inject.gointernal/module/artifact/build/injectappmodel/bundle.gointernal/module/artifact/build/injectappmodel/coverage_test.gointernal/module/artifact/build/injectappmodel/effects.gointernal/module/artifact/build/injectappmodel/helpers.gointernal/module/artifact/build/injectappmodel/inject.gointernal/module/artifact/build/injectappmodel/injectappmodel_test.gointernal/module/artifact/build/injectappmodel/registry.gointernal/module/artifact/build/injectappmodel/session.gointernal/module/artifact/build/injectappmodel/source.gointernal/module/artifact/build/injectappmodel/spec.gointernal/module/lifecycle/bundles.gointernal/module/lifecycle/bundles_app_setting_test.gomodules/core/service/index.tsmodules/core/service/orm/model/translation_term_base_model.ts
- Adopt an on-disk service/index.ts instead of registering a virtual stub when Module.ServiceEntryPoint is empty. - Point unit-test Module.ServiceEntryPoint at the real entryPoint so Ensure does not run over live apps. - Keep ResolveDir at dirname(path) when a virtual source path also exists on disk so ./models imports resolve. Co-authored-by: Cursor <cursoragent@cursor.com>
- Align GetTranslations with Go: language-wide hash and empty module_names yields empty terms_by_module. - Revert Ensure'd ServiceEntryPoint from releaseInjectSchedules so build failures cannot leak a virtual entry. - Resolve relative service entries before Stat, treat only ErrNotExist as missing, and collect empty-entry apps for TranslationTerm bundle inject. Co-authored-by: Cursor <cursoragent@cursor.com>
- Cover GetTranslations, dialect DDL ensure, and hash/filter edge cases in translation_term_base_model. - Exercise Ensure/ServiceEntryPath adopt, Bundle empty-entry emit, helpers guards, and disk ResolveDir shadowing. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
@cursor review |
- Cover nullish/falsy normalize, default GetTranslations req, empty application, and DDL catch paths. - Extract compareTermHashKeys and pass raw Value into hashing so every branch is exercisable from unit tests. Co-authored-by: Cursor <cursoragent@cursor.com>
- Always set Effects.ServiceEntryPath when Ensure runs, including on-disk adopt, so applyInjectEffects can fill an empty builder entryPoint. - Add a regression covering inject with empty builder entry and an existing service/index.ts. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/module/lifecycle/bundles.go (1)
264-269: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPreserve application order when appending owners.
Go map iteration does not preserve order. This loop appends
translationTermOwnersin a nondeterministic order. That order affects virtual-import registration and bundle output.Track application order from
installed, or derive it with the existing stable application ordering logic before appending owners.🤖 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 `@internal/module/lifecycle/bundles.go` around lines 264 - 269, Update the owner-collection flow around pickTranslationTermOwnerModule to avoid iterating directly over the byApp map when appending owners. Iterate applications using the stable order from installed or the existing application-ordering logic, then look up each app’s modules in byApp and append owners in that deterministic order while preserving seenApp behavior.
🤖 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 `@internal/module/lifecycle/bundles.go`:
- Around line 106-111: The bundle target selection around orderedApps and
appendTranslationTermOwnersFromInstalled must include eligible applications that
own TranslationTerms, even when all non-core modules have empty
ServiceEntryPoint values. Add those applications to orderedApps, selecting a
valid representative module when no backend module exists, so bundle
construction and ensureBundleC2VirtualImports receive the owners; add an
integration test covering an installation containing only empty service entries.
In `@modules/core/service/orm/model/translation_term_base_model_coverage.test.ts`:
- Around line 192-218: Restore meta.tableName in the finally block using the
originalTable value captured before mutation, and remove reliance on the
success-path assignment after the assertions. Ensure cleanup occurs even when
either assertion or GetTranslations call fails, while preserving the existing
global cleanup.
---
Nitpick comments:
In `@internal/module/lifecycle/bundles.go`:
- Around line 264-269: Update the owner-collection flow around
pickTranslationTermOwnerModule to avoid iterating directly over the byApp map
when appending owners. Iterate applications using the stable order from
installed or the existing application-ordering logic, then look up each app’s
modules in byApp and append owners in that deterministic order while preserving
seenApp behavior.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c56ab916-0a55-4ab6-a870-530a8c41b081
📒 Files selected for processing (12)
internal/esbplugins/backendplugin/plugin_test.gointernal/module/artifact/build/backend/builder_test.gointernal/module/artifact/build/backend/inject.gointernal/module/artifact/build/backend/inject_test.gointernal/module/artifact/build/injectappmodel/bundle.gointernal/module/artifact/build/injectappmodel/helpers.gointernal/module/artifact/build/injectappmodel/inject.gointernal/module/artifact/build/injectappmodel/injectappmodel_test.gointernal/module/lifecycle/bundles.gointernal/module/lifecycle/bundles_translation_term_test.gomodules/core/service/orm/model/translation_term_base_model.tsmodules/core/service/orm/model/translation_term_base_model_coverage.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/module/artifact/build/injectappmodel/helpers.go
- internal/module/artifact/build/backend/inject.go
- internal/module/artifact/build/injectappmodel/bundle.go
- internal/module/artifact/build/backend/builder_test.go
- internal/module/artifact/build/injectappmodel/inject.go
- Fall back to a TranslationTerm owner as the multi-app bundle representative when orderedApps is empty. - Append Ensure-only owners in installed order, and restore mutated tableName in coverage tests on failure. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d7fd9d6. Configure here.
- Extract writeBackendBundleToDir and unit-test !ok, error wrap, and success. - Cover early no-representative return and successful buildBackendBundlesToDir via a test hook. Co-authored-by: Cursor <cursoragent@cursor.com>
This was referenced Aug 6, 2026
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
1 participant
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.
User description
Summary
TranslationTermas the first C2 Spec (EnsureServiceEntry: true,softDelete: false) and materializes a virtualservice/index.tsplus__generated__/translation_term.tswithout touchingpackage.jsonor the source tree.TranslationTermBaseModelwith ORM fields, unique-index ensure, andGetTranslations(Gateway still dials Go I18n until P3).Effects.ServiceEntryPathto the builder/plugin entry when empty; reverts Ensure'dModule.ServiceEntryPointafter build so DB stays package.json-sourced; app bundles can still host TranslationTerm for empty-entry apps (e.g.web).Test plan
go test ./internal/module/artifact/build/injectappmodel/ ./internal/module/artifact/build/backend/ ./internal/esbplugins/backendplugin/ ./internal/module/lifecycle/ -count=1./choysum test typecheck coreweb(noentryPoints.service) and confirm TranslationTerm injects; diskpackage.jsonunchangedMade with Cursor
PR Type
Enhancement
Description
Go Core: Register TranslationTerm C2 model with EnsureServiceEntry and virtual entry generation
Go Core: Revert in-memory ServiceEntryPoint mutations prior to database persistence
TS Modules: Add TranslationTermBaseModel in modules/core with GetTranslations and index ensure
Licensing & Tests: Include Apache-2.0 SPDX header on new model; expand Go unit tests across build packages
File Walkthrough
12 files
Expose SetEntryPoint and expand esbuild virtual resolve filterRevert ensured service entry on persist and register missing virtualsourcesAdopt virtual ServiceEntryPath in builder and plugin when entry pointis emptyMaterialize virtual service entries during multi-module bundlingAdd ServiceEntryPath field to Effects and preserve on mergeGenerate virtual service/index.ts for EnsureServiceEntry specsRegister TranslationTerm spec as first builtin C2 modelTrack and revert temporary ServiceEntryPoint mutations in sessionProvide virtual service entry path and stub source generatorsInclude empty-entry modules when resolving TranslationTerm bundleownersRe-export TranslationTermBaseModel and TranslationTermModelCtorAdd TranslationTermBaseModel with schema, hash computation, andGetTranslations4 files
Initialize empty inject registry in backend builder unit testsUpdate coverage tests for new builtin spec count and merged effectsAdd unit tests for TranslationTerm injection and entry point revertUpdate bundle C2 virtual import test assertions1 files
Update EnsureServiceEntry documentation commentSummary by CodeRabbit
New Features
Bug Fixes