refactor(i18n): lookup-only TermStore and remove Go I18n - #255
Conversation
- Make UpsertPackagedTerms the sole packaged write path and trim TermStore to Lookup/warm/invalidate.
- Dial TranslationTerm Search/Count/GetTranslations via loader; drop Go {app}.I18n handlers and Ensure*.
- Seed terminology.editor ACL on TranslationTerm Search/Browse/Update/Count for Editor and PO export.
Co-authored-by: Cursor <cursoragent@cursor.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2ea4ce03-d7e4-4011-ae67-0393f4d47c93) |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 1 minute 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 (8)
📝 WalkthroughWalkthroughThe PR replaces legacy I18n RPC usage with dynamic TranslationTerm Count, Search, and GetTranslations calls. It updates PO export pagination, packaged-term migration, term-store tests, authorization records, and service registration. ChangesTranslationTerm migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Gateway
participant DescriptorLoader
participant TranslationTermRPC
participant POExporter
Gateway->>DescriptorLoader: load TranslationTerm descriptors
Gateway->>TranslationTermRPC: Count terms
TranslationTermRPC-->>Gateway: return total
Gateway->>TranslationTermRPC: Search terms by page
TranslationTermRPC-->>Gateway: return term rows
POExporter->>Gateway: collect terms for PO export
Gateway-->>POExporter: return paginated terms
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 |
Codecov Report❌ Patch coverage is
📢 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/i18n/store/term_store_internal_test.go (1)
82-98: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftInvalidate
TermStoreafter directTranslationTermmutations commit.Generated
TranslationTermmodels use the generic ORM mutation path, which does not callInvalidateModule. An editor update can leave stale translations cached and allowWarmLanguageto install a stale snapshot. Cover every reachable create, update, and delete 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/i18n/store/term_store_internal_test.go` around lines 82 - 98, Update the generic TranslationTerm mutation flow so every committed create, update, and delete operation calls TermStore.InvalidateModule for the affected module. Ensure invalidation occurs only after a successful database mutation and covers all reachable ORM mutation paths, allowing WarmLanguage to discard stale snapshots.
🧹 Nitpick comments (4)
internal/i18n/gateway/rpc_client_test.go (1)
96-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore deterministic dial-failure coverage.
fetchAppSearchTermsandfetchAppTranslationsresolve a descriptor before they callclient.Dial. A descriptor-load failure now satisfies these tests, so neither test proves thatdial boomreachesclient.ToStatusError.Set up a valid descriptor fixture before each call. Then assert the returned error contains the dial failure. Keep descriptor-load failure in a separate test.
🤖 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/gateway/rpc_client_test.go` around lines 96 - 112, Update TestFetchAppSearchTermsDialFailure and TestFetchAppTranslationsDialFailure to register or provide a valid descriptor fixture before invoking the fetch functions, ensuring execution reaches client.Dial and client.ToStatusError. Assert the returned errors contain “dial boom” rather than only checking for any error, and add separate coverage for descriptor-load failures.internal/i18n/import/import.go (1)
76-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the missing-table fallback.
All displayed
UpsertPackagedTermstests callMigrateTranslationTermTablebefore importing. They do not execute this branch. Add a fresh-scope test that imports one PO entry without a table, then asserts table creation and the imported row.🤖 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/import/import.go` around lines 76 - 82, Add a fresh-scope test for UpsertPackagedTerms that skips MigrateTranslationTermTable, imports one PO entry, and verifies the missing-table fallback creates the translation-term table and persists the imported row. Keep existing migrated-table tests unchanged and assert both table existence and stored term data after the import.internal/module/evolution/schema/migrator_test.go (1)
46-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd a positive ACL-seeding test.
Both tests cover only no-op paths.
TestMigratorMigrateCallsTerminologyEditorAllowshas no effective catalog or role tables, soEnsureTerminologyEditorAllowsreturns before it creates an allow row. Removing the call frommigrator.Migratewould still pass this test.
internal/module/evolution/schema/migrator_test.go#L46-L60: create the catalog,terminology.editorrole, TranslationTerm services, and role-access table; then assert thatMigrateseeds Search, Browse, Update, and Count.internal/i18n/models/terminology_acl_test.go#L8-L25: add a direct unit test that asserts one allow row for each TranslationTerm method.🤖 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/evolution/schema/migrator_test.go` around lines 46 - 60, Add positive ACL-seeding coverage: in internal/module/evolution/schema/migrator_test.go lines 46-60, configure the catalog, terminology.editor role, TranslationTerm services, and role-access table, then assert Migrate seeds Search, Browse, Update, and Count; in internal/i18n/models/terminology_acl_test.go lines 8-25, add a direct test asserting one allow row for each TranslationTerm method. Use the existing migrator and EnsureTerminologyEditorAllows test helpers and preserve the no-table-creation assertion.internal/i18n/store/term_store_test.go (1)
312-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the same
TermStoreafter the table is created.The first
WarmLanguagecall exercises the missing-table branch. Replacingtswith a new store bypasses the transition from the empty cache to the migrated table. Remove the reassignment and warm the original store again. This ensures the test detects regressions in recovery for an existingTermStore.Proposed test adjustment
- ts = store.NewTermStore(rs, "auth") if err := ts.WarmLanguage("zh_CN"); err != nil {🤖 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/store/term_store_test.go` around lines 312 - 333, Update TestTermStoreWarmMissingTableAndBumpInvalidate to keep the original TermStore instance after migrating and populating the table; remove the reassignment to store.NewTermStore and call WarmLanguage on the existing ts to exercise recovery from the missing-table cache state.
🤖 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.
Outside diff comments:
In `@internal/i18n/store/term_store_internal_test.go`:
- Around line 82-98: Update the generic TranslationTerm mutation flow so every
committed create, update, and delete operation calls TermStore.InvalidateModule
for the affected module. Ensure invalidation occurs only after a successful
database mutation and covers all reachable ORM mutation paths, allowing
WarmLanguage to discard stale snapshots.
---
Nitpick comments:
In `@internal/i18n/gateway/rpc_client_test.go`:
- Around line 96-112: Update TestFetchAppSearchTermsDialFailure and
TestFetchAppTranslationsDialFailure to register or provide a valid descriptor
fixture before invoking the fetch functions, ensuring execution reaches
client.Dial and client.ToStatusError. Assert the returned errors contain “dial
boom” rather than only checking for any error, and add separate coverage for
descriptor-load failures.
In `@internal/i18n/import/import.go`:
- Around line 76-82: Add a fresh-scope test for UpsertPackagedTerms that skips
MigrateTranslationTermTable, imports one PO entry, and verifies the
missing-table fallback creates the translation-term table and persists the
imported row. Keep existing migrated-table tests unchanged and assert both table
existence and stored term data after the import.
In `@internal/i18n/store/term_store_test.go`:
- Around line 312-333: Update TestTermStoreWarmMissingTableAndBumpInvalidate to
keep the original TermStore instance after migrating and populating the table;
remove the reassignment to store.NewTermStore and call WarmLanguage on the
existing ts to exercise recovery from the missing-table cache state.
In `@internal/module/evolution/schema/migrator_test.go`:
- Around line 46-60: Add positive ACL-seeding coverage: in
internal/module/evolution/schema/migrator_test.go lines 46-60, configure the
catalog, terminology.editor role, TranslationTerm services, and role-access
table, then assert Migrate seeds Search, Browse, Update, and Count; in
internal/i18n/models/terminology_acl_test.go lines 8-25, add a direct test
asserting one allow row for each TranslationTerm method. Use the existing
migrator and EnsureTerminologyEditorAllows test helpers and preserve the
no-table-creation assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4b91f7e-bebc-4570-af7e-d7487adf1c39
📒 Files selected for processing (33)
internal/i18n/gateway/client.gointernal/i18n/gateway/po_export.gointernal/i18n/gateway/rpc_client_test.gointernal/i18n/gateway/term_search.gointernal/i18n/gateway/terms_client.gointernal/i18n/gateway/terms_client_test.gointernal/i18n/gateway/translations.gointernal/i18n/import/import.gointernal/i18n/import/import_test.gointernal/i18n/models/i18n_meta_test.gointernal/i18n/models/terminology_acl.gointernal/i18n/models/terminology_acl_test.gointernal/i18n/models/translation_term.gointernal/i18n/models/translation_term_test.gointernal/i18n/service/parse_int32_test.gointernal/i18n/service/proto.gointernal/i18n/service/search.gointernal/i18n/service/service.gointernal/i18n/service/service_test.gointernal/i18n/service/update.gointernal/i18n/store/term_store.gointernal/i18n/store/term_store_internal_test.gointernal/i18n/store/term_store_test.gointernal/module/evolution/schema/migrator.gointernal/module/evolution/schema/migrator_test.gointernal/server/middleware/auth/grpcauth/interceptor.gointernal/server/middleware/auth/grpcauth/interceptor_test.gointernal/server/server_service_registration_test.gointernal/service/i18n.gointernal/service/service.gointernal/service/service_test.gomodules/auth/data/bootstrap.jsonmodules/base/web/route/routes.ts
💤 Files with no reviewable changes (12)
- internal/server/server_service_registration_test.go
- internal/service/i18n.go
- internal/i18n/service/proto.go
- internal/i18n/service/parse_int32_test.go
- internal/server/middleware/auth/grpcauth/interceptor_test.go
- internal/i18n/service/search.go
- internal/i18n/service/update.go
- internal/i18n/service/service.go
- internal/server/middleware/auth/grpcauth/interceptor.go
- internal/i18n/service/service_test.go
- internal/service/service.go
- internal/i18n/models/i18n_meta_test.go
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨ |
- Cover TranslationTerm Search/Count/GetTranslations dial paths via bufconn fixtures. - Exercise missing-table migrate fallback, positive terminology.editor ACL seeding, and TermStore warm recovery. Co-authored-by: Cursor <cursoragent@cursor.com>
- Remove EnsureTerminologyEditorAllows and its migrator hook so installs no longer seed TranslationTerm allows. - Clarify PermissionDenied on PO/Search gateway to mention the terminology.editor role. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/i18n/import/import_test.go (1)
105-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the error content, not only that an error occurred.
The test passes if any stage of
UpsertPackagedTermsfails. A future change that fails earlier, for example inparsePoTermsor the transactor, would keep the test green while the migration path is no longer covered. Assert the migration wrap text.💚 Proposed change
_, err = i18nimport.UpsertPackagedTerms(rs, nil, "auth", "auth", "zh_CN", poText) - if err == nil { - t.Fatal("expected migrate error for missing table on closed DB") + if err == nil || !strings.Contains(err.Error(), "migrate auth_translation_term") { + t.Fatalf("expected migrate error for missing table on closed DB, got %v", err) }Add the
stringsimport if it is not already present.🤖 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/import/import_test.go` around lines 105 - 108, Update the test around UpsertPackagedTerms to assert that the returned error contains the migration wrap text indicating the missing table, rather than only checking err is non-nil; add the strings import if needed and retain the existing failure assertion.internal/i18n/gateway/terms_rpc_fixture_test.go (2)
487-534: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared bufconn server setup.
newListValueResponseDialerduplicates the listener, server start, cleanup, and lazy-connection logic ofnewTranslationTermDialer. Extract a helper that takes thegrpc.StreamHandlerand returns thegrpcclient.ServiceDialer. This also removes the inconsistency wherenewListValueResponseDialerdoes not validateserviceName.🤖 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/gateway/terms_rpc_fixture_test.go` around lines 487 - 534, Extract the shared bufconn listener, gRPC server lifecycle, cleanup, and lazy connection logic from newListValueResponseDialer and newTranslationTermDialer into a helper that accepts a grpc.StreamHandler and returns a grpcclient.ServiceDialer. Update both dialers to use this helper, preserving each handler’s behavior and ensuring the shared dialer validates serviceName consistently.
181-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace deprecated
grpc.DialContextwithgrpc.NewClient.Pass
"passthrough:///bufnet"as the target. Apply the same change tonewListValueResponseDialer.🤖 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/gateway/terms_rpc_fixture_test.go` around lines 181 - 186, Replace deprecated grpc.DialContext with grpc.NewClient in the fixture dialer, using "passthrough:///bufnet" as the target while preserving the existing context dialer and insecure transport credentials. Apply the same target and constructor change to newListValueResponseDialer.
🤖 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/models/translation_term_test.go`:
- Around line 176-191: Update TestMigrateTranslationTermTableUniqueIndexError to
pin the database pool to one connection by calling SetMaxOpenConns(1) before
enabling PRAGMA query_only, ensuring the subsequent migration uses the read-only
connection and deterministically exercises the unique-index error path.
---
Nitpick comments:
In `@internal/i18n/gateway/terms_rpc_fixture_test.go`:
- Around line 487-534: Extract the shared bufconn listener, gRPC server
lifecycle, cleanup, and lazy connection logic from newListValueResponseDialer
and newTranslationTermDialer into a helper that accepts a grpc.StreamHandler and
returns a grpcclient.ServiceDialer. Update both dialers to use this helper,
preserving each handler’s behavior and ensuring the shared dialer validates
serviceName consistently.
- Around line 181-186: Replace deprecated grpc.DialContext with grpc.NewClient
in the fixture dialer, using "passthrough:///bufnet" as the target while
preserving the existing context dialer and insecure transport credentials. Apply
the same target and constructor change to newListValueResponseDialer.
In `@internal/i18n/import/import_test.go`:
- Around line 105-108: Update the test around UpsertPackagedTerms to assert that
the returned error contains the migration wrap text indicating the missing
table, rather than only checking err is non-nil; add the strings import if
needed and retain the existing failure assertion.
🪄 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: ba0527e6-881b-4b1c-953d-c9d53164e715
📒 Files selected for processing (10)
internal/i18n/gateway/po_export.gointernal/i18n/gateway/term_search.gointernal/i18n/gateway/terms_client_test.gointernal/i18n/gateway/terms_rpc_fixture_test.gointernal/i18n/import/import_test.gointernal/i18n/models/translation_term_test.gointernal/i18n/store/term_store_test.gointernal/module/evolution/schema/migrator.gointernal/module/evolution/schema/migrator_test.gomodules/base/web/route/routes.ts
💤 Files with no reviewable changes (3)
- internal/i18n/gateway/po_export.go
- internal/module/evolution/schema/migrator_test.go
- internal/i18n/store/term_store_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/base/web/route/routes.ts
- internal/i18n/gateway/terms_client_test.go
- Set MaxOpenConns(1) so PRAGMA query_only applies to the migrate connection. - Assert UpsertPackagedTerms missing-table failures include the migrate wrap text. Co-authored-by: Cursor <cursoragent@cursor.com>
- Extract migrateTranslationTermTableIfMissing and stub it in-package to force migrate failures. - Split the err check so patch coverage no longer leaves a partial if-init line. Co-authored-by: Cursor <cursoragent@cursor.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 34 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Fix all with cubic | Re-trigger cubic
- Correct UpsertPackagedTerms docs: MetaModel owns schema; missing-table create is fallback only. - Keep paging when probe Total is unset, and backfill Total from later pages. - Stop inferring fuzzy from Comments; those fields store PO file refs only. 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).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
- When search hooks omit Total and export stops at poExportMaxItems, set truncated so clients are not silently capped. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
User description
Summary
UpsertPackagedTermsthe sole packaged-term write path and trim TermStore to Lookup/warm/invalidate/termHash for_t.TranslationTermSearch/Count/GetTranslations vialoader.GetMethodDescriptor; delete Go{app}.I18nhandlers, EnsureI18nMeta, EnsureTranslationTermTable, and ServiceDescs I18n injection.terminology.editorACL to TranslationTerm Search/Browse/Update/Count (Editor + PO export paging).Test plan
go test ./internal/i18n/... ./internal/service/... ./internal/server/... ./internal/module/lifecycle/... ./internal/module/evolution/schema/... -count=1_tstill resolves via Go Lookup cacheterminology.editor:GET /web/i18n/translationsandGET /web/i18n/po?module=…&application=…&lang=…succeed{app}.I18nServiceDesc / nogo://i18n/<app>meta after installMade with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Access
PR Type
Enhancement, Refactoring
Description
Go Core: Removed legacy
{app}.I18ngRPC handlers and trimmedTermStoreto a lookup-only cache for_t.Go Core: Re-routed I18n gateway RPCs and PO exports to
{app}.TranslationTermSearch/Count via dynamic descriptors.Go Core: Updated schema migration and ACL seeding to grant
terminology.editoraccess toTranslationTermmethods.TypeScript Modules: Rebound
terminology.editorACL inmodules/authbootstrap JSON toTranslationTermSearch/Browse/Update/Count.SPDX & Licensing: New file
internal/i18n/models/terminology_acl_test.goincludes SPDX LGPL-3.0 header; no license boundary impact.Test Coverage: Cleaned up obsolete Go
i18nservicetests and updated unit tests across gateway, models, store, schema migrator, and server middleware.File Walkthrough
9 files
Fetch app translations via TranslationTerm GetTranslations descriptorRename packaged write helper to UpsertPackagedTermsReplace EnsureI18nMeta with EnsureTerminologyEditorAllows forTranslationTermRename table helper to MigrateTranslationTermTable for test fixturesRemove legacy Go I18n service implementationTrim TermStore to lookup-only cache for hot path translationsUpdate schema migrator to seed terminology editor ACL allowancesRemove legacy I18n GetTranslations from internal auth method listsRemove automatic injection of I18n descriptors into ApplicationService2 files
Update PO export collection using TranslationTerm Count and SearchDial TranslationTerm Count and Search RPCs with query conditions1 files
Add unit tests for EnsureTerminologyEditorAllows early returns1 files
Rebind terminology.editor role permissions to TranslationTermSearch/Browse/Update/Count1 files
Update terminology editor route comment to reflect new ACL methods19 files