chore(i18n): ImportPackaged facade, CRUD invalidate, sync design - #256
Conversation
- Extend $choysum.i18n with invalidateModule and upsertPackagedTerms via ScopeProvider. - Invalidate TermStore after TranslationTerm CRUD and add ImportPackaged facade. - Refresh core i18n README and auth_forward comments for TranslationTerm topology. Co-authored-by: Cursor <cursoragent@cursor.com>
|
We've triggered an ultrareview automatically — This PR rewires the i18n bridge with new cache invalidation and async packaged-term upserts spanning Go and TS; a missed invalidation or scope/lifecycle bug could leave stale translations or corrupt term rows in production, so it merits a deep multi-pass review.. I'll post findings when complete. An ultrareview is cubic's deepest review, catching hard-to-find bugs in the most critical PRs. It runs a longer, multi-pass analysis using cubic's most capable review models, and typically takes around 30 minutes. It consumes your team's reviewed-lines allowance at 3× the standard rate. Automated ultrareviews are disabled by default. We triggered this run as part of your trial. Want cubic to do this for every high-risk PR? Enable auto-ultrareview in your settings. |
📝 WalkthroughWalkthroughChangesThe PR connects the runtime i18n bridge to scope providers, adds module invalidation and packaged PO-term upserts, and integrates these operations with translation-term CRUD flows and cache handling. Terminology bridge and model integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TranslationTermModel
participant ChoysumI18nBridge
participant TerminologyProvider
TranslationTermModel->>ChoysumI18nBridge: upsertPackagedTerms(application, module, lang, poText)
ChoysumI18nBridge->>TerminologyProvider: import packaged PO terms
TerminologyProvider-->>ChoysumI18nBridge: import statistics and language
ChoysumI18nBridge-->>TranslationTermModel: resolved upsert result
Possibly related PRs
Suggested labels: 🚥 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:
|
|
Failed to generate code suggestions for PR |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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 (10)
modules/core/service/orm/model/_translation_term_cache.ts (3)
34-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecord the swallowed invalidation failure.
A failure here leaves the Go
TermStorewith stale terms and produces no signal. The write already succeeded, so throwing is not correct. Log the failure instead, so a stale cache can be diagnosed.♻️ Proposed logging of the failure
try { bridge.invalidateModule(app, mod); - } catch { - /* best-effort: write already succeeded */ + } catch (err) { + // best-effort: the write already succeeded; the cache stays stale until the next warm. + console.warn('invalidateTerminologyModule failed', app, mod, 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_cache.ts` around lines 34 - 38, Update the invalidation catch block around bridge.invalidateModule(app, mod) to log the caught failure while preserving best-effort behavior and not rethrowing after the successful write. Include enough context to diagnose stale TermStore terms.
52-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
modulesFromRowsandmodulesFromPayloadshave identical bodies.Both functions normalize the input to an array, read
Module, trim it, and keep non-empty values. Keep both export names for call-site intent, but share one implementation.♻️ Proposed consolidation
-export function modulesFromRows(rows: unknown): string[] { - const list = Array.isArray(rows) ? rows : rows != null ? [rows] : []; - const out: string[] = []; - for (const row of list) { - const mod = String((row as any)?.Module ?? '').trim(); - if (mod) out.push(mod); - } - return out; -} - -export function modulesFromPayloads(values: unknown): string[] { - const list = Array.isArray(values) ? values : values != null ? [values] : []; - const out: string[] = []; - for (const row of list) { - const mod = String((row as any)?.Module ?? '').trim(); - if (mod) out.push(mod); - } - return out; -} +function moduleNames(input: unknown): string[] { + const list = Array.isArray(input) ? input : input != null ? [input] : []; + const out: string[] = []; + for (const item of list) { + const mod = String((item as any)?.Module ?? '').trim(); + if (mod) out.push(mod); + } + return out; +} + +/** Module names from persisted rows returned by a repository call. */ +export const modulesFromRows = moduleNames; + +/** Module names from caller-supplied write payloads. */ +export const modulesFromPayloads = moduleNames;🤖 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_cache.ts` around lines 52 - 70, Consolidate the duplicated normalization logic in modulesFromRows and modulesFromPayloads into one shared helper, while retaining both exported functions as intent-specific wrappers that delegate to it. Preserve the current handling of nullish, scalar, array, trimmed, and empty Module values.
4-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the
$choysum.i18nbridge contract.The bridge contract is declared in three places:
_translation_term_cache.ts,translate.ts, and$choysum.d.ts. The local types also mark bridge methods optional, while$choysum.d.tsmarks them required.Place the shared type and accessor in a module under
service/i18n, then import them from both consumers. This preserves the existing dependency direction.🤖 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_cache.ts` around lines 4 - 25, The $choysum.i18n bridge contract is duplicated and inconsistent across the listed files. Create a shared bridge type and getChoysumI18nBridge accessor under service/i18n, make the bridge methods consistently required as defined by $choysum.d.ts, and import/reuse them from modules/core/service/orm/model/_translation_term_cache.ts and modules/core/service/i18n/translate.ts; update $choysum.d.ts to consume the shared contract if needed while preserving the existing dependency direction.modules/core/service/orm/model/translation_term_base_model.ts (2)
461-464: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
Updatedoes not forwardoptionsto the pre-writeSearch.
Deleteat lines 502-506 spreads...(options || {})into its pre-writeSearch.Updateomits it. The two pre-reads then apply different soft-delete visibility for the same kind of condition. Forwardoptionshere so both paths select the same rows.♻️ Proposed alignment
const before = await (this as any).Search(condition as any, { fields: ['Module'] as any, limit: 0, + ...(options || {}), });🤖 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 461 - 464, Update the pre-write Search call in Update to spread ...(options || {}) into its query options, matching Delete’s pre-read behavior so both paths apply the same soft-delete visibility for identical conditions.
460-470: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace the pre-write row scan with a grouped module read.
limit: 0does not limitSearch, so each write loads every matching row. UseReadGroup(['Module'], condition)and extractkeys.Moduleorlabels.Modulebefore invalidation. Apply the same change toDelete.🤖 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 460 - 470, In the update flow around the pre-write Search call, replace the full row scan with ReadGroup(['Module'], condition), extracting module names from each group's keys.Module or labels.Module for invalidateTerminologyModules. Apply the same grouped-module lookup before Delete, preserving invalidation of modules from the payload and affected rows where applicable.internal/i18n/bridge/terminology.go (1)
141-184: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGuard the packaged-term import against oversized PO input.
performUpsertPackagedTermsruns the full parse, table migration, transaction, and cache warm inline on the QuickJS execution thread.i18nimport.UpsertPackagedTermsalso warms every affected language after the write. A large PO payload blocks the engine for the whole operation, and the returned promise resolves only after the blocking work completes.Add an upper bound on
len(poText)and reject payloads above it. This keeps a single import from stalling the runtime.♻️ Proposed size guard
poText, err := poTextBytes(args[3]) if err != nil { return ctx.ThrowError(err) } + const maxPoBytes = 8 << 20 + if len(poText) > maxPoBytes { + return ctx.ThrowError(fmt.Errorf("upsertPackagedTerms: poText exceeds %d bytes", maxPoBytes)) + }🤖 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/i18n/bridge/terminology.go` around lines 141 - 184, Update performUpsertPackagedTerms to enforce an upper bound on len(poText) immediately after poTextBytes succeeds and before resolving the runtime scope or calling i18nimport.UpsertPackagedTerms. Reject oversized payloads with a QuickJS error, using the repository’s established maximum PO input size constant if available; preserve normal processing for payloads at or below the limit.modules/core/service/orm/model/translation_term_cache.test.ts (3)
142-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
BaseModeldirectly instead of walking the prototype chain.
Object.getPrototypeOf(TranslationTermBaseModel.prototype).constructorassumes thatTranslationTermBaseModelextendsBaseModelwith no intermediate class. If an intermediate class is added later, the patch targets that class instead, the override still calls the realsuper.Create, and the test gives a false result with no error.Import
BaseModelfrom./modeland patch it directly.Also applies to: 169-171
🤖 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_cache.test.ts` around lines 142 - 144, Import BaseModel directly from ./model and update the test’s Create override to target BaseModel rather than deriving it via Object.getPrototypeOf(TranslationTermBaseModel.prototype).constructor. Apply the same change to the additional override referenced in the comment, while preserving the existing originalCreate capture and restoration behavior.
129-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFour of the six CRUD overrides have no invalidation test.
The PR adds
Create,CreateMany,Update,UpdateById,Delete, andDeleteByIdoverrides inmodules/core/service/orm/model/translation_term_base_model.ts. This file testsCreateandUpdateByIdonly.The untested paths carry the more complex logic.
UpdateandDeleteboth run a pre-writeSearchand merge module names from several sources.CreateManymerges payload modules with row modules. A regression in any of them leaves the GoTermStorestale with no test signal.Add invalidation tests for
CreateMany,Update,Delete, andDeleteById.🤖 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_cache.test.ts` around lines 129 - 180, Add focused invalidation tests alongside the existing Create and UpdateById tests for the TtInvTerm.CreateMany, TtInvTerm.Update, TtInvTerm.Delete, and TtInvTerm.DeleteById overrides. Mock the relevant BaseModel methods and Browse/Search results to exercise each method’s module-merging logic, then assert global i18n.invalidateModule receives the expected app/module pairs; restore all mocks and global state in finally blocks.
73-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for an empty
lang.The guard in
ImportPackagedisif (!module || !lang). Only the emptymodulehalf is tested. Add one call withlang: ''so both halves of the condition are covered.🤖 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_cache.test.ts` around lines 73 - 81, Add a rejection test for TtInvTerm.ImportPackaged using a valid module and an empty lang value, and assert the TRANSLATION_TERM_IMPORT_ARGS error. Keep the existing empty-module and null-poText cases unchanged.internal/i18n/bridge/terminology_test.go (1)
166-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the
Uint8ArrayPO input.The test covers only the string form of
poText.poTextByteshas three additional branches forUint8Array,Uint8ClampedArray, and byte arrays, and none of them are exercised. The declared public type inmodules/core/types/$choysum.d.tsacceptsstring | Uint8Array, so the binary path is part of the supported contract.Add one upsert call that passes a
Uint8Arraybuilt in JavaScript.🤖 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/i18n/bridge/terminology_test.go` around lines 166 - 184, Extend the test around the existing upsertPackagedTerms evaluation to add a second upsert call whose PO input is a JavaScript Uint8Array. Use the same valid PO content and assertions as the string case, verifying the binary input is accepted and reports one upserted term; keep the existing string-path coverage intact.
🤖 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/i18n/bridge/terminology.go`:
- Around line 111-124: Update invalidateModuleFunc to use a read-only
existing-store lookup instead of reg.StoreFor, returning false when the
application has no registered store and avoiding cache mutation; add the lookup
to the registry’s existing-store API and update the test expectation for
invalidating before an auth store exists from true to false.
- Around line 128-139: Update performUpsertPackagedTerms and its NewPromise
callback so every error path returns ctx.NewError(err) rather than the
JS_EXCEPTION sentinel, allowing ret.IsError() to detect failures and
reject(ret). Preserve freeing ret after resolve or reject.
In `@modules/core/service/orm/model/translation_term_base_model.ts`:
- Around line 428-437: Update the static Create method to combine modules
derived from the input value with modules derived from the returned row before
calling invalidateTerminologyModules, matching the existing CreateMany behavior
so invalidation still occurs when returnFields omits Module.
In `@modules/core/service/orm/model/translation_term_cache.test.ts`:
- Around line 26-32: Update the test around invalidateTerminologyModule to
restore root.$choysum in a finally block, ensuring restoration occurs whether
the no-op assertion passes or throws; preserve the existing saved-value and
deletion setup.
- Around line 57-65: Update expectRejects so the assertion for a resolved
promise is not executed inside the try/catch: track whether awaiting promise
completes without rejection, validate that flag after the catch, and keep the
existing ChoysumError code checks only for actual rejections.
In `@modules/core/types/`$choysum.d.ts:
- Around line 249-265: Mark the i18n.invalidateModule and
i18n.upsertPackagedTerms members in the declaration optional, matching their
conditional installation in installI18nObject and the existing internal type
declarations. Preserve their current function signatures and return types while
allowing callers to guard against their absence.
---
Nitpick comments:
In `@internal/i18n/bridge/terminology_test.go`:
- Around line 166-184: Extend the test around the existing upsertPackagedTerms
evaluation to add a second upsert call whose PO input is a JavaScript
Uint8Array. Use the same valid PO content and assertions as the string case,
verifying the binary input is accepted and reports one upserted term; keep the
existing string-path coverage intact.
In `@internal/i18n/bridge/terminology.go`:
- Around line 141-184: Update performUpsertPackagedTerms to enforce an upper
bound on len(poText) immediately after poTextBytes succeeds and before resolving
the runtime scope or calling i18nimport.UpsertPackagedTerms. Reject oversized
payloads with a QuickJS error, using the repository’s established maximum PO
input size constant if available; preserve normal processing for payloads at or
below the limit.
In `@modules/core/service/orm/model/_translation_term_cache.ts`:
- Around line 34-38: Update the invalidation catch block around
bridge.invalidateModule(app, mod) to log the caught failure while preserving
best-effort behavior and not rethrowing after the successful write. Include
enough context to diagnose stale TermStore terms.
- Around line 52-70: Consolidate the duplicated normalization logic in
modulesFromRows and modulesFromPayloads into one shared helper, while retaining
both exported functions as intent-specific wrappers that delegate to it.
Preserve the current handling of nullish, scalar, array, trimmed, and empty
Module values.
- Around line 4-25: The $choysum.i18n bridge contract is duplicated and
inconsistent across the listed files. Create a shared bridge type and
getChoysumI18nBridge accessor under service/i18n, make the bridge methods
consistently required as defined by $choysum.d.ts, and import/reuse them from
modules/core/service/orm/model/_translation_term_cache.ts and
modules/core/service/i18n/translate.ts; update $choysum.d.ts to consume the
shared contract if needed while preserving the existing dependency direction.
In `@modules/core/service/orm/model/translation_term_base_model.ts`:
- Around line 461-464: Update the pre-write Search call in Update to spread
...(options || {}) into its query options, matching Delete’s pre-read behavior
so both paths apply the same soft-delete visibility for identical conditions.
- Around line 460-470: In the update flow around the pre-write Search call,
replace the full row scan with ReadGroup(['Module'], condition), extracting
module names from each group's keys.Module or labels.Module for
invalidateTerminologyModules. Apply the same grouped-module lookup before
Delete, preserving invalidation of modules from the payload and affected rows
where applicable.
In `@modules/core/service/orm/model/translation_term_cache.test.ts`:
- Around line 142-144: Import BaseModel directly from ./model and update the
test’s Create override to target BaseModel rather than deriving it via
Object.getPrototypeOf(TranslationTermBaseModel.prototype).constructor. Apply the
same change to the additional override referenced in the comment, while
preserving the existing originalCreate capture and restoration behavior.
- Around line 129-180: Add focused invalidation tests alongside the existing
Create and UpdateById tests for the TtInvTerm.CreateMany, TtInvTerm.Update,
TtInvTerm.Delete, and TtInvTerm.DeleteById overrides. Mock the relevant
BaseModel methods and Browse/Search results to exercise each method’s
module-merging logic, then assert global i18n.invalidateModule receives the
expected app/module pairs; restore all mocks and global state in finally blocks.
- Around line 73-81: Add a rejection test for TtInvTerm.ImportPackaged using a
valid module and an empty lang value, and assert the
TRANSLATION_TERM_IMPORT_ARGS error. Keep the existing empty-module and
null-poText cases unchanged.
🪄 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: c9df261b-344f-4bb3-a62d-b857ce3b0080
📒 Files selected for processing (10)
internal/defaultengine/init.gointernal/i18n/bridge/terminology.gointernal/i18n/bridge/terminology_test.gointernal/i18n/gateway/auth_forward.gomodules/core/i18n/README.mdmodules/core/service/i18n/translate.tsmodules/core/service/orm/model/_translation_term_cache.tsmodules/core/service/orm/model/translation_term_base_model.tsmodules/core/service/orm/model/translation_term_cache.test.tsmodules/core/types/$choysum.d.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultrareview completed in 15m 41s
All reported issues were addressed across 10 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
- Use ExistingStore for invalidateModule and NewError for upsert rejections. - Guard nullish bridge args; invalidate Create from payload when Module is projected away. - Align $choysum.i18n types and facade unit-test teardown/assertions with runtime. Co-authored-by: Cursor <cursoragent@cursor.com>
- Cover bridge error branches, poText variants, ExistingStore, and CRUD invalidate paths. - Drop unreachable ExecContext nil guard; add package-level hooks for upsert/marshal failures. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
- Pass upsert/marshal deps into performUpsertPackagedTerms instead of package globals. - Drop no-op void BaseModel and redundant restore from withInvalidateSpy. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
User description
Summary
$choysum.i18nwith syncinvalidateModuleand asyncupsertPackagedTerms(ScopeProvider), wired through the default engine plugin.TranslationTerm.ImportPackagedplus CRUD success-path TermStore invalidation; update bridge types and unit coverage.modules/core/i18n/README.mdandauth_forwardcomments for the TranslationTerm topology (local.devdesign rewrite is intentionally not in this PR).Test plan
go test ./internal/i18n/bridge/ ./internal/i18n/import/ ./internal/defaultengine/... -count=1./choysum test unit core --be(incl. ImportPackaged / invalidate helpers)_tsees new value after invalidateImportPackagedmatchesUpsertPackagedTerms(skip override, purge, invalidate)Made with Cursor
Summary by cubic
Add runtime i18n writes and cache invalidation with a provider-based bridge. Exposes
$choysum.i18n.invalidateModuleand async$choysum.i18n.upsertPackagedTerms, auto‑invalidates afterTranslationTermCRUD, and adds theTranslationTerm.ImportPackagedfacade.New Features
WithTerminologyProviderviaquickjsengine.NewRuntimePluginWithProvider; exposest,invalidateModule, and asyncupsertPackagedTerms.TranslationTerm.ImportPackagedfacade that forwards to$choysum.i18n.upsertPackagedTermsand returns import stats; TypeScript$choysum.i18nand model types updated.Bug Fixes
invalidateModuleusesRegistry.ExistingStoreand rejects empty/core apps or nullish args;upsertPackagedTermsvalidates required args and supportsstring/Uint8Array/ArrayBufferPO text, rejecting with JS errors.$choysum.i18n; inject upsert/marshal deps intoperformUpsertPackagedTerms(removed mutable test hooks) and drop a deadExecContextguard; tests cover error branches, poText variants,ExistingStore, and CRUD invalidation (patch coverage 100%).Written for commit d0c6731. Summary will update on new commits.
PR Type
Enhancement
Description
Core Go runtime i18n bridge enhancements
invalidateModuleandupsertPackagedTermsvia$choysum.i18nTypeScript ORM model cache invalidation
TermStoreautomatically onTranslationTermCRUDImportPackagedfacade method for PO translation importsLicense compliance and test coverage
File Walkthrough
6 files
Wire i18n runtime plugin using scope providerExpose invalidateModule and upsertPackagedTerms bridge functionsUpdate ChoysumI18n bridge type interface definitionsAdd terminology cache invalidation helpers with SPDX headerAdd ImportPackaged facade and post-CRUD cache invalidationUpdate global $choysum.i18n TypeScript declaration2 files
Test runtime i18n invalidation and upsert bridgeAdd unit tests for TranslationTerm cache invalidation2 files
Update RPC auth context forward comment referencesUpdate doc references to TranslationTerm model topologySummary by CodeRabbit
New Features
Documentation
Tests