fix(i18n): gateway GetTranslations catalog for multi-app terminology - #258
Conversation
- Encode lang/module_names/hash under the generated req field and unwrap result so catalog reads match TranslationTerm_GetTranslations_* shapes. Co-authored-by: Cursor <cursoragent@cursor.com>
- Include application web in app-stage codegen and stop treating it as SPA-only. - Load api/web/proto plus bundle scripts on ApplicationService(web) while keeping dist/web static assets. - Persist missing TranslationTerm IR before GenerateApp so EnsureServiceEntry hosts emit descriptors. Co-authored-by: Cursor <cursoragent@cursor.com>
- Prefix ServiceDescs RegisterProto paths as {app}/{file}.proto so web works without JS bundle registration.
- Cover api/<app>/proto layout and loaderRegisterPath helpers in unit tests.
Co-authored-by: Cursor <cursoragent@cursor.com>
- Wrap gateway catalog Search with withRepositoryAuthzRuleBypass so non-meta hosts return terms under internal identity. - Cover the bypass path in TranslationTerm GetTranslations unit tests. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change aligns translation RPC envelopes, includes ChangesWeb application and translation runtime
Estimated code review effort: 4 (Complex) | ~60 minutes 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 🔍(Review updated until commit 97e67f6)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 97e67f6
Previous suggestionsSuggestions up to commit 5ad02a1
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
- Treat web as TranslationTerm backend in tests by seeding bundles and api/web/proto. - Keep application-mode bootstrap readiness covering apps/web index + proto. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Persistent review updated to latest commit 97e67f6 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/core/service/orm/model/translation_term_base_model_coverage.test.ts (1)
63-65: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert bypass-depth restoration after
GetTranslations.The test verifies that both bypass depths are enabled during
Search. It does not verify that the awaited call restores them afterward. A leaked bypass can authorize later repository operations in the same request.Add zero-depth assertions after
GetTranslationsresolves.Proposed fix
const out = await TtCovTerm.GetTranslations({ lang: 'zh_CN', module_names: ['auth'] }); expect(sawBypass).toBe(true); + expect(getRepositoryRecordRuleBypassDepth()).toBe(0); + expect(getRepositoryFieldRuleBypassDepth()).toBe(0); expect(out.terms_by_module).toEqual({ auth: { ui: { Hi: '你好' } } });🤖 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_coverage.test.ts` around lines 63 - 65, Extend the test around TtCovTerm.GetTranslations to assert that both bypass-depth values are zero after the awaited call resolves. Keep the existing assertions confirming bypass during Search and the returned translations unchanged, and use the test’s existing bypass-depth symbols.
🤖 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/gateway/client.go`:
- Around line 77-82: Update the response handling before parseAppTranslations so
the legacy fallback occurs only when out lacks the "result" key. If "result" is
present but cannot be decoded as map[string]any, return a decode error instead
of parsing an empty catalog; preserve normal parsing for valid envelopes and
absent-result legacy responses.
---
Nitpick comments:
In `@modules/core/service/orm/model/translation_term_base_model_coverage.test.ts`:
- Around line 63-65: Extend the test around TtCovTerm.GetTranslations to assert
that both bypass-depth values are zero after the awaited call resolves. Keep the
existing assertions confirming bypass during Search and the returned
translations unchanged, and use the test’s existing bypass-depth symbols.
🪄 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: dc5170a4-fbc9-4f11-9ced-8969b18cbd45
📒 Files selected for processing (21)
internal/i18n/gateway/client.gointernal/i18n/gateway/terms_rpc_fixture_test.gointernal/module/artifact/pipeline/pipeline.gointernal/module/artifact/pipeline/pipeline_test.gointernal/module/artifact/runtimeapi/sync.gointernal/module/artifact/runtimeapi/sync_test.gointernal/module/lifecycle/modulemanager.gointernal/module/lifecycle/modulemanager_coverage_test.gointernal/module/plan/planner.gointernal/module/plan/planner_test.gointernal/server/runplan/dist_validation.gointernal/server/runplan/dist_validation_test.gointernal/server/runplan/plan_test.gointernal/server/server_bootstrap_orchestration_test.gointernal/server/server_serve_test.gointernal/server/server_service_registration_bootstrap_test.gointernal/server/server_test_fixtures_test.gointernal/service/service.gointernal/service/service_test.gomodules/core/service/orm/model/translation_term_base_model.tsmodules/core/service/orm/model/translation_term_base_model_coverage.test.ts
There was a problem hiding this comment.
All reported issues were addressed across 21 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
- Reject a present non-object GetTranslations result instead of legacy empty-catalog fallback. - Assert authz bypass depths restore after GetTranslations Search. - Clarify that catalog-wide bypass is intentional (FieldDefault.GetEffective pattern). Co-authored-by: Cursor <cursoragent@cursor.com>
- Cover malformed GetTranslations result unwrap via gateway RPC fixture. - Cover web application-mode dist validation and default-target dedupe. - Cover web application-mode scriptDistPath and empty loaderRegisterPath. - Cover ensureInjectedAppModelsForCodegen branches via testable builder hook. - Cover Ensure-only stub entry inject errors and core upgrade listInstalledApps. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
- Assert core upgrade plan apps include web via listInstalledApps, not lease entry. - Check protoImportPaths for application-mode web ApplicationService. - Reuse commitStubSplitBuilder for ensureInjected codegen tests. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
- Keep entrySeen/buildErr on the package stub used by ensureInjected coverage. - Avoid looking like dead fields inside installer_commit_test.go alone. Co-authored-by: Cursor <cursoragent@cursor.com>
User description
Summary
Wrap gateway
GetTranslationsRPC args as protobufValue reqso catalog fetches match generated service stubs.Enable
webTranslationTerm codegen/runtime (api/web/proto, bundle scripts) and register app protos under{app}/paths for ProtoLoader.Bypass RecordRule/FieldRule on
TranslationTerm.GetTranslationscatalog Search so non-meta hosts (e.g. base) return terms under the gateway's internal identity.Test plan
go test ./internal/i18n/gateway/... ./internal/service/... ./internal/module/plan/... ./internal/server/runplan/... -count=1./choysum test unit core --be --pattern 'GetTranslations'Restart server after upgrade;
GET /web/i18n/translations?lang=zh_CNincludesbase(and other apps), not onlymeta/coreUI at
/web/base/companiesshows Chinese field/menu labels when locale iszh_CNBy submitting this pull request, you agree to the Contributor License Agreement (CLA) of this project. If you have not yet signed it, please follow the instructions provided by @cla-bot below.
Made with Cursor
PR Type
Bug fix, Enhancement, Tests
Description
Go core changes for web i18n
GetTranslationsRPC args inreq.webapp proto generation and script routing.{app}/{file}.protofor loader.TypeScript module changes for catalog reads
TranslationTermcatalogSearch.License compliance and test coverage
File Walkthrough
2 files
wrap dynamic GetTranslations RPC arguments and unwrap responsebypass repository authz rules during GetTranslations catalog search11 files
update RPC fixtures to reflect wrapped request and response valuesadd pipeline test covering web module generationupdate proto sync test to verify web app restorationupdate installed app listing tests to include web appupdate build plan tests for affected apps with webupdate dist validation unit tests for web proto requirementseed web bundle and proto dist in server bootstrap testsseed web application dist in bootstrap registration testadd helpers to seed web backend and bundle dist assetstest loader register path logic and web service scriptsadd unit test for GetTranslations authz rule bypass6 files
include web application in app-stage module generationinclude web application in missing proto synchronizationrebuild TranslationTerm IR before app generation and include webinclude web application in affected apps planningvalidate dist assets for web backend proto targetsadd scriptDistPath and register app protos under app path2 files
Summary by CodeRabbit
New Features
Bug Fixes