refactor(meta): dual-store follow-up with injectappmodel and FlushEffective - #249
Conversation
…ective - Slim Entities() to effective catalog only, unexport migrate helpers, and remove unused metaeff. - Unify FieldDefault/AppSetting C2 inject into injectappmodel with InjectAppModels, SupersedeInjectAppModels, and BundleInjectAppModels. - Flush effective once at persist and i18n boundaries; add EnsureAbstractModel and ReplaceModuleDeclarations facades. - Unexport Raw* types and route tests through declaration helpers and table-name APIs. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR introduces a unified application-model injection system with registry-based scheduling, generated sources, validation, supersession, and bundle support. It also adds metadata facades, internalizes raw persistence APIs, adds effective flushing, and updates lifecycle database handling. ChangesDual-store metadata and application-model injection
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ModuleBuilder
participant InjectionSession
participant Registry
participant MetadataStore
ModuleBuilder->>InjectionSession: Decide and inject application models
InjectionSession->>Registry: Claim application ownership
InjectionSession->>MetadataStore: Load declarations and generated paths
InjectionSession->>InjectionSession: Generate virtual source and record imports
ModuleBuilder->>MetadataStore: Replace module declarations and flush effective keys
ModuleBuilder->>Registry: Release schedules on failure or cleanup
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:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (14)
internal/module/artifact/build/injectappmodel/injectappmodel_test.go (3)
46-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset the process-wide claims after each test.
newTestSessioncallsResetScheduledForTestat setup only.spec.scheduledis process-wide, so a claim from one test stays visible to any later test that does not usenewTestSession. Register a cleanup to keep tests independent.♻️ Proposed fix
t.Helper() ResetScheduledForTest() + t.Cleanup(ResetScheduledForTest)🤖 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/injectappmodel_test.go` around lines 46 - 63, Update newTestSession to register t.Cleanup that calls ResetScheduledForTest after the test completes, while retaining the existing setup reset so process-wide spec.scheduled claims cannot leak between tests.
189-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHandle the lookup result at Line 198.
spec, _ := specByName("FieldDefault")discardsok. If the spec name changes,specis nil and the next line panics with an unclear failure. Other tests in this file already fail fast witht.Fatal. UsespecByNameOrPanichere.🤖 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/injectappmodel_test.go` around lines 189 - 209, Update TestReleaseSchedules_ClearsClaim to use specByNameOrPanic("FieldDefault") instead of discarding the lookup result from specByName, so a missing spec fails fast with the established test error rather than panicking on spec.scheduled.
250-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the entry-point imports, not only the virtual source.
fakeHostrecordsentryImportsandsetImportCalls, but no test reads them.InjectAppModelsruns both specs and each spec appends the full cumulative path set, sohost.entryImportscan hold duplicates. Add an assertion onhost.entryImportshere. That test then pins the duplication defect flagged ininject.goLine 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 `@internal/module/artifact/build/injectappmodel/injectappmodel_test.go` around lines 250 - 266, Extend TestInjectRegistersVirtualSource to assert host.entryImports contains the expected entry-point import paths after InjectAppModels, accounting for the cumulative duplicate entries produced across both specs. Use the recorded setImportCalls or existing module/spec path helpers to verify the expected duplicated imports and preserve the current virtual-source assertions.internal/module/artifact/build/injectappmodel/bundle.go (1)
45-87: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
bundleSpecruns one DB query per application per spec.
dbLoadModelsexecutes inside the module loop for every registered spec. With N applications and M specs, the bundle path issues N×M queries. Consider loading declarations once per spec with a single query filtered by model name, then grouping by application in memory.🤖 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/bundle.go` around lines 45 - 87, Update bundleSpec to load all existing declarations for the current spec with one dbLoadModels query filtered by spec.ModelName, then group the returned models by application in memory before iterating modules. Use each module’s application group for handwrittenModels and generatedModels decisions, preserving path registration and skipping behavior while eliminating the per-application database query.internal/module/artifact/build/injectappmodel/source.go (1)
15-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidate
Specfields at registration instead of trusting them here.
applicationis escaped withstrconv.Quote, which is correct.spec.ModelNameis interpolated raw at Line 35 inside a single-quoted literal and at Line 36 as the class name and base import name. TodayRegisteris called only fromregister_builtin.gowith literal names, so no external input reaches this template. A future registration with a quote, a space, or a non-identifier character would emit source that fails to parse, and the failure would appear as an esbuild error far from the cause. Add a validation check inRegisterforModelName,GeneratedRelPath, andBaseModelFile.Also note Line 16:
filepath.Clean("")returns".", so an emptymodulesPathproduces relative imports. Both current callers substitute a fallback before the call, so this is defensive only.🤖 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/source.go` around lines 15 - 37, Add validation in Register for Spec.ModelName, Spec.GeneratedRelPath, and Spec.BaseModelFile, rejecting empty or invalid values before registration succeeds; ensure ModelName is a valid identifier suitable for both generated string literals and class/import-name positions, and the path fields are non-empty valid relative paths. Also defensively reject an empty modulesPath before generatedSource constructs imports, rather than allowing filepath.Clean("") to produce ".".internal/module/artifact/build/backend/app_setting.go (2)
208-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
appSettingDuplicateCodeinstead of repeating the literal.Line 19 defines
appSettingDuplicateCode = "APP_SETTING_DUPLICATE", andSpec.DuplicateCodecarries the same value. The literal in the format string can drift from the constant that tests assert on.♻️ Proposed fix
- return fmt.Errorf("APP_SETTING_DUPLICATE: application %q build produced multiple AppSetting models", app) + return fmt.Errorf("%s: application %q build produced multiple AppSetting models", appSettingDuplicateCode, app)🤖 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/backend/app_setting.go` at line 208, Update the duplicate AppSetting error construction in the relevant build flow to use the existing appSettingDuplicateCode constant instead of the repeated "APP_SETTING_DUPLICATE" literal, while preserving the current error message and formatting.
220-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
resetAppSettingScheduledAppsForTestnow clears every registered spec.
injectappmodel.ResetScheduledForTestiterates all specs. The function name states AppSetting scope, but the call also clears FieldDefault claims. A test that resets AppSetting state between phases now also drops a FieldDefault claim it intended to keep.Add a per-spec reset in
injectappmodel, or rename these helpers to state the global scope.🤖 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/backend/app_setting.go` around lines 220 - 222, Update resetAppSettingScheduledAppsForTest to reset only the AppSetting-related spec instead of calling injectappmodel.ResetScheduledForTest, which clears all registered specs; add and use a per-spec reset API in injectappmodel, preserving unrelated FieldDefault claims. Alternatively, rename the helper and its callers to explicitly reflect global reset scope if that is the intended behavior.internal/module/artifact/build/injectappmodel/spec.go (2)
83-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ScheduledAppsreturns a throwaway map for an unknownmodelName.If
modelNameis not registered, the function returns a fresh&sync.Map{}. Callers then store claims that no other code can observe, and the dedup silently stops working. A typo in a caller string literal produces no error.Consider returning
(*sync.Map, bool), or panicking in this test-only helper so a wrongmodelNamefails loudly.🤖 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/spec.go` around lines 83 - 88, The ScheduledApps helper silently returns a throwaway map for unknown model names, causing stored claims to be lost. Change ScheduledApps to fail loudly for unregistered modelName values, preferably by panicking in this test-only helper, while preserving the registered spec.scheduled map behavior.
19-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep deduplication state out of
Spec.Speccontains a namedsync.Map, which must not be copied after first use.Specs()copies each registeredSpecat line 47, andRegistercopies the by-value parameter at line 35;go vetflags these copies. Store one registry-owned map perModelNameand route all accesses, includingScheduledApps, through an accessor.🤖 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/spec.go` around lines 19 - 20, Remove the named sync.Map field from Spec and maintain deduplication state in registry-owned storage keyed by ModelName. Update Register and Specs to avoid copying synchronization state, and route all scheduled-state reads and writes—including ScheduledApps and NeedInject-related accesses—through a dedicated accessor that returns the map for the relevant model.internal/module/artifact/build/backend/field_default_test.go (1)
558-567: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the effective count before the flush as well.
The comment states the new contract: supersede does not flush, and the persist boundary does. The test proves only the post-flush state. If a future change reintroduces a flush inside supersede, this test still passes.
💚 Proposed addition
// EDS-opt-2: supersede does not FlushEffective; persist boundary does. + var before int64 + if err := db.Model(&meta.Model{}).Where("name = ?", "FieldDefault").Count(&before).Error; err != nil { + t.Fatalf("count effective before flush: %v", err) + } + if before != 2 { + t.Fatalf("expected effective rows untouched before flush, count=%d", before) + } if err := meta.FlushEffective(db, []meta.LogicalKey{{Application: "partner", Name: "FieldDefault"}}); err != nil { t.Fatalf("flush: %v", err) }Adjust the expected
beforevalue to match whatseedFieldDefaultDeclarationwrites into the effective table.🤖 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/backend/field_default_test.go` around lines 558 - 567, Update the test around FlushEffective to count the FieldDefault effective row before flushing and assert the expected pre-flush count based on seedFieldDefaultDeclaration. Keep the existing post-flush count assertion, ensuring the test verifies supersede leaves the effective table unchanged until the explicit FlushEffective call.internal/module/artifact/build/backend/field_default.go (1)
26-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
field_default.goandapp_setting.goare now near-identical wrappers.Both files repeat the same plan converter, sync helper, release helper, and six
*ForTestshims, and they differ only by the model-name literal. The PR moves the real logic intoinjectappmodel, so this remaining layer can collapse into one generic helper set parameterized bymodelName. Doing so removes the risk that a later fix lands in one file only.This is a follow-up cleanup, not a blocker for this PR.
🤖 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/backend/field_default.go` around lines 26 - 104, Collapse the duplicated FieldDefault wrapper logic in fieldDefaultPlanFrom, toInject, syncFieldDefaultFromSession, rememberFieldDefaultInjectPath, releaseFieldDefaultSchedule, and the six test shim helpers by introducing a shared model-name parameterized helper path, matching the existing app_setting.go pattern. Keep the current FieldDefault behavior intact while replacing the repeated literal-based wrappers with one reusable implementation so future changes only land in one place.internal/module/artifact/build/backend/declaration_test_helpers.go (1)
21-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the positional string parameters with a struct.
seedVirtualDeclarationTreetakes 11stringparameters andseedVirtualDeclarationErrorBranchTreetakes 9. Call sites pass bare literals such as"arg1", "tp1", "p1". If two arguments are swapped, the code still compiles and the test still passes with wrong identifiers.Pass an options struct with named fields, and let the helper default any empty field.
Also applies to: 67-71
🤖 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/backend/declaration_test_helpers.go` around lines 21 - 25, Replace the positional string arguments of seedVirtualDeclarationTree and seedVirtualDeclarationErrorBranchTree with an options struct using named fields. Update all call sites to construct the struct, preserving existing values while allowing omitted fields to remain empty. Add defaults inside both helpers for any empty option fields, and remove the long positional parameter lists.internal/module/artifact/build/backend/inject_host.go (1)
13-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a compile-time
Hostassertion and name the repeated plugin interfaces.
injectHostmust satisfyinjectappmodel.Host. Today a signature change inHostsurfaces only at the call site inensureInjectSession. A package-level assertion documents the contract and fails at the definition.The two anonymous interfaces are each written twice, once per plugin field.
♻️ Proposed refactor
// injectHost adapts ModuleBuilder to injectappmodel.Host. type injectHost struct { b *ModuleBuilder } + +var _ injectappmodel.Host = injectHost{} + +type entryPointImportsSetter interface { + SetEntryPointImports([]string) +} + +type virtualSourceRegistrar interface { + RegisterVirtualSource(path string, contents string) +}Then assert against
entryPointImportsSetterandvirtualSourceRegistrarin both methods.Also applies to: 45-71
🤖 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/backend/inject_host.go` around lines 13 - 15, Add a package-level compile-time assertion that injectHost implements injectappmodel.Host so any Host signature change fails at the type definition instead of only at ensureInjectSession. Also extract the repeated anonymous plugin interfaces used in injectHost’s methods into named interfaces, and reuse entryPointImportsSetter and virtualSourceRegistrar in both places rather than duplicating the same interface literals.internal/module/artifact/build/backend/app_setting_coverage_test.go (1)
429-429: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove redundant
meta.EnsureDualStoreTables()calls.
meta.CatalogEntities()already includes both effective and raw dual-store entities. The explicit calls at Lines 492–495 and 640–643 are redundant.🤖 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/backend/app_setting_coverage_test.go` at line 429, Remove the redundant meta.EnsureDualStoreTables() calls near the AutoMigrate setup and other referenced locations. Keep db.AutoMigrate(meta.CatalogEntities()...) as the sole initialization path for both effective and raw dual-store entities, without changing unrelated setup logic.
🤖 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/backend/app_setting_coverage_test.go`:
- Around line 281-290: The blank-Id fixtures are normalized before the filter
runs, so both tests miss the intended branch. In
internal/module/artifact/build/backend/app_setting_coverage_test.go lines
281-290 and
internal/module/artifact/build/backend/field_default_coverage_test.go lines
281-290, either assert after persistDeclaration that the stored row received a
generated Id, or insert the whitespace-only Id through a raw database helper
that bypasses ensureBaseModelID, while preserving the existing
supersedeVirtualAppSettings assertions.
In `@internal/module/artifact/build/backend/builder.go`:
- Around line 891-900: Update the key-enumeration block in the builder to
iterate over all registered specs from injectappmodel.Specs() instead of
hardcoding FieldDefault and AppSetting, appending each superseded spec’s
LogicalKey only when the application name is non-empty. Before using this loop,
fix Specs() so it snapshots the registry without copying sync.Map per element,
then continue checking each spec through
injectSession.Plan(name).SupersedeInject.
In `@internal/module/artifact/build/backend/declaration_test_helpers.go`:
- Around line 4-12: Rename declaration_test_helpers.go to
declaration_test_helpers_test.go so the Go toolchain treats its testing helpers
and testing import as test-only; leave the existing call sites in
app_setting_coverage_test.go and field_default_coverage_test.go unchanged.
In `@internal/module/artifact/build/backend/field_default.go`:
- Line 181: The append in the relevant field-default build flow must not mutate
the cached slice returned by entryPointImports(). Create an independent copy
before appending b.fieldDefaultInjectPaths, and apply the same protection to the
identical append pattern in app_setting.go so cached imports remain unchanged
across builder calls.
In `@internal/module/artifact/build/injectappmodel/inject.go`:
- Around line 171-172: Prevent duplicate imports and mutation of borrowed host
slices when applying inject paths. In
internal/module/artifact/build/injectappmodel/inject.go lines 171-172, update
applyInject to construct a fresh import slice and deduplicate the merged
EntryPointImports and allInjectPaths results before SetEntryPointImports. In
internal/module/artifact/build/injectappmodel/bundle.go lines 26-29, likewise
copy EntryPointImports into a new slice, deduplicate the merged imports before
SetEntryPointImports, and preserve each unique path only once.
- Around line 157-167: Enforce zero-value Session safety across all affected
entry points: in internal/module/artifact/build/injectappmodel/inject.go lines
157-167, update applyInject to return before calling sess.host.Module() when
host is nil; in internal/module/artifact/build/injectappmodel/inject.go lines
36-46, replace direct plans writes in DecideAndInjectOne and DecideOne with
SetPlan; in internal/module/artifact/build/injectappmodel/session.go lines
85-97, initialize lastInjectPath and injectPaths in rememberInjectPath before
writing to them.
- Around line 36-46: Update DecideAndInjectOne and DecideOne to store plans
through sess.SetPlan instead of assigning directly to sess.plans. Preserve the
existing plan values and error flow while relying on SetPlan’s lazy map
initialization so zero-value Session instances do not panic.
In `@internal/module/artifact/build/injectappmodel/session.go`:
- Around line 85-97: Update Session.rememberInjectPath to lazily initialize both
lastInjectPath and injectPaths before writing, matching SetPlan’s zero-value
handling. Preserve the existing nil-session, empty-path, deduplication, and
append behavior, and ensure a zero-value Session cannot panic on either map
write.
In `@pkg/meta/facade.go`:
- Around line 103-115: Make the module replacement flow atomic by wrapping the
previous-key reads, DeleteRawModelsForModule, and every PersistModelTreeAsRaw
call in a single database transaction. Update the relevant logic around the
shown replacement function to use the transaction handle consistently, commit
only after all trees persist successfully, and roll back on any failure. Add a
failure-path test that makes a later tree persistence fail and verifies the
original raw declarations remain unchanged.
---
Nitpick comments:
In `@internal/module/artifact/build/backend/app_setting_coverage_test.go`:
- Line 429: Remove the redundant meta.EnsureDualStoreTables() calls near the
AutoMigrate setup and other referenced locations. Keep
db.AutoMigrate(meta.CatalogEntities()...) as the sole initialization path for
both effective and raw dual-store entities, without changing unrelated setup
logic.
In `@internal/module/artifact/build/backend/app_setting.go`:
- Line 208: Update the duplicate AppSetting error construction in the relevant
build flow to use the existing appSettingDuplicateCode constant instead of the
repeated "APP_SETTING_DUPLICATE" literal, while preserving the current error
message and formatting.
- Around line 220-222: Update resetAppSettingScheduledAppsForTest to reset only
the AppSetting-related spec instead of calling
injectappmodel.ResetScheduledForTest, which clears all registered specs; add and
use a per-spec reset API in injectappmodel, preserving unrelated FieldDefault
claims. Alternatively, rename the helper and its callers to explicitly reflect
global reset scope if that is the intended behavior.
In `@internal/module/artifact/build/backend/declaration_test_helpers.go`:
- Around line 21-25: Replace the positional string arguments of
seedVirtualDeclarationTree and seedVirtualDeclarationErrorBranchTree with an
options struct using named fields. Update all call sites to construct the
struct, preserving existing values while allowing omitted fields to remain
empty. Add defaults inside both helpers for any empty option fields, and remove
the long positional parameter lists.
In `@internal/module/artifact/build/backend/field_default_test.go`:
- Around line 558-567: Update the test around FlushEffective to count the
FieldDefault effective row before flushing and assert the expected pre-flush
count based on seedFieldDefaultDeclaration. Keep the existing post-flush count
assertion, ensuring the test verifies supersede leaves the effective table
unchanged until the explicit FlushEffective call.
In `@internal/module/artifact/build/backend/field_default.go`:
- Around line 26-104: Collapse the duplicated FieldDefault wrapper logic in
fieldDefaultPlanFrom, toInject, syncFieldDefaultFromSession,
rememberFieldDefaultInjectPath, releaseFieldDefaultSchedule, and the six test
shim helpers by introducing a shared model-name parameterized helper path,
matching the existing app_setting.go pattern. Keep the current FieldDefault
behavior intact while replacing the repeated literal-based wrappers with one
reusable implementation so future changes only land in one place.
In `@internal/module/artifact/build/backend/inject_host.go`:
- Around line 13-15: Add a package-level compile-time assertion that injectHost
implements injectappmodel.Host so any Host signature change fails at the type
definition instead of only at ensureInjectSession. Also extract the repeated
anonymous plugin interfaces used in injectHost’s methods into named interfaces,
and reuse entryPointImportsSetter and virtualSourceRegistrar in both places
rather than duplicating the same interface literals.
In `@internal/module/artifact/build/injectappmodel/bundle.go`:
- Around line 45-87: Update bundleSpec to load all existing declarations for the
current spec with one dbLoadModels query filtered by spec.ModelName, then group
the returned models by application in memory before iterating modules. Use each
module’s application group for handwrittenModels and generatedModels decisions,
preserving path registration and skipping behavior while eliminating the
per-application database query.
In `@internal/module/artifact/build/injectappmodel/injectappmodel_test.go`:
- Around line 46-63: Update newTestSession to register t.Cleanup that calls
ResetScheduledForTest after the test completes, while retaining the existing
setup reset so process-wide spec.scheduled claims cannot leak between tests.
- Around line 189-209: Update TestReleaseSchedules_ClearsClaim to use
specByNameOrPanic("FieldDefault") instead of discarding the lookup result from
specByName, so a missing spec fails fast with the established test error rather
than panicking on spec.scheduled.
- Around line 250-266: Extend TestInjectRegistersVirtualSource to assert
host.entryImports contains the expected entry-point import paths after
InjectAppModels, accounting for the cumulative duplicate entries produced across
both specs. Use the recorded setImportCalls or existing module/spec path helpers
to verify the expected duplicated imports and preserve the current
virtual-source assertions.
In `@internal/module/artifact/build/injectappmodel/source.go`:
- Around line 15-37: Add validation in Register for Spec.ModelName,
Spec.GeneratedRelPath, and Spec.BaseModelFile, rejecting empty or invalid values
before registration succeeds; ensure ModelName is a valid identifier suitable
for both generated string literals and class/import-name positions, and the path
fields are non-empty valid relative paths. Also defensively reject an empty
modulesPath before generatedSource constructs imports, rather than allowing
filepath.Clean("") to produce ".".
In `@internal/module/artifact/build/injectappmodel/spec.go`:
- Around line 83-88: The ScheduledApps helper silently returns a throwaway map
for unknown model names, causing stored claims to be lost. Change ScheduledApps
to fail loudly for unregistered modelName values, preferably by panicking in
this test-only helper, while preserving the registered spec.scheduled map
behavior.
- Around line 19-20: Remove the named sync.Map field from Spec and maintain
deduplication state in registry-owned storage keyed by ModelName. Update
Register and Specs to avoid copying synchronization state, and route all
scheduled-state reads and writes—including ScheduledApps and NeedInject-related
accesses—through a dedicated accessor that returns the map for the relevant
model.
🪄 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: e8f72984-989c-42d8-9bf7-4cff1f423d06
📒 Files selected for processing (71)
internal/i18n/models/i18n_meta.gointernal/i18n/models/i18n_meta_test.gointernal/module/artifact/build/backend/app_setting.gointernal/module/artifact/build/backend/app_setting_coverage_test.gointernal/module/artifact/build/backend/app_setting_test.gointernal/module/artifact/build/backend/builder.gointernal/module/artifact/build/backend/builder_test.gointernal/module/artifact/build/backend/declaration_test_helpers.gointernal/module/artifact/build/backend/field_default.gointernal/module/artifact/build/backend/field_default_coverage_test.gointernal/module/artifact/build/backend/field_default_test.gointernal/module/artifact/build/backend/inject_app_models.gointernal/module/artifact/build/backend/inject_host.gointernal/module/artifact/build/injectappmodel/bundle.gointernal/module/artifact/build/injectappmodel/compat.gointernal/module/artifact/build/injectappmodel/doc.gointernal/module/artifact/build/injectappmodel/helpers.gointernal/module/artifact/build/injectappmodel/host.gointernal/module/artifact/build/injectappmodel/inject.gointernal/module/artifact/build/injectappmodel/injectappmodel_test.gointernal/module/artifact/build/injectappmodel/plan.gointernal/module/artifact/build/injectappmodel/register_builtin.gointernal/module/artifact/build/injectappmodel/session.gointernal/module/artifact/build/injectappmodel/source.gointernal/module/artifact/build/injectappmodel/spec.gointernal/module/artifact/build/injectappmodel/supersede.gointernal/module/lifecycle/bundles.gointernal/module/lifecycle/bundles_app_setting_test.gointernal/module/lifecycle/bundles_field_default_test.gointernal/module/lifecycle/install_module_test.gointernal/module/lifecycle/install_prefetch_test.gointernal/module/lifecycle/module_index_sync_test.gointernal/module/lifecycle/modulemanager.gointernal/module/lifecycle/modulemanager_coverage_test.gointernal/module/lifecycle/modulemanager_fastfail_test.gointernal/module/lifecycle/uninstaller_clean_models_test.gointernal/module/lifecycle/uninstaller_model_data_test.gointernal/module/lifecycle/upgrade_uninstall_commit_test.gointernal/module/metaeff/recompute.gointernal/module/metaeff/recompute_test.gopkg/meta/acl_remap.gopkg/meta/acl_remap_coverage_test.gopkg/meta/acl_remap_test.gopkg/meta/declaration.gopkg/meta/declaration_test.gopkg/meta/dual_store_migrate.gopkg/meta/dual_store_migrate_coverage_test.gopkg/meta/dual_store_migrate_test.gopkg/meta/effective_merge.gopkg/meta/effective_merge_coverage_test.gopkg/meta/effective_merge_test.gopkg/meta/extends_expand.gopkg/meta/extends_expand_coverage_test.gopkg/meta/extends_expand_test.gopkg/meta/facade.gopkg/meta/facade_test.gopkg/meta/meta_model.gopkg/meta/meta_raw_argument.gopkg/meta/meta_raw_decorator.gopkg/meta/meta_raw_field.gopkg/meta/meta_raw_field_test.gopkg/meta/meta_raw_model.gopkg/meta/meta_raw_parameter.gopkg/meta/meta_raw_service.gopkg/meta/meta_raw_typeparameter.gopkg/meta/model.gopkg/meta/model_test.gopkg/meta/raw_facade.gopkg/meta/recompute.gopkg/meta/recompute_coverage_test.gopkg/meta/recompute_test.go
💤 Files with no reviewable changes (2)
- internal/module/metaeff/recompute_test.go
- internal/module/metaeff/recompute.go
- Drop field_default.go and app_setting.go so ModuleBuilder only keeps injectSession and the three injectappmodel APIs. - Move decide/bundle/dedup matrix coverage into injectappmodel and keep a thin backend lifecycle integration test. - Wire lifecycle bundles to BundleInjectAppModels only and drop Ensure*VirtualImports stubs. Co-authored-by: Cursor <cursoragent@cursor.com>
- Deduplicate entry-point inject imports on a fresh slice and harden Session zero-value map writes via SetPlan. - Store Spec.scheduled as *sync.Map and drive persist flush keys from the Specs registry. - Wrap ReplaceModuleDeclarations delete+persist in one transaction and add a rollback regression test. Co-authored-by: Cursor <cursoragent@cursor.com>
- Add injectappmodel and backend inject coverage suites that exercise nil guards, decide/bundle/supersede branches, and BuildWithoutPersist failure releases. - Cover facade/raw_facade APIs including Prefer failures and replace rollback paths, and surface empty-entryPoint build plugin GetParserResults errors. Co-authored-by: Cursor <cursoragent@cursor.com>
- Exercise nil and empty name/path owners in appendUnique so bundles.go patch coverage is complete. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (4)
internal/module/artifact/build/backend/inject_coverage_test.go (3)
235-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe variable named
prebuildPluginholds the build plugin.
newInjectTestBuilderreturnsbuildPluginas the second value (line 48). Line 235 binds that value to a variable namedprebuildPlugin. Line 238 then assigns the build plugin tobuilder.prebuildPlugin, so both plugin fields reference one object. The test still passes, because it only asserts that prebuild fails on the missing entry file. Remove the misleading assignment.♻️ Proposed cleanup
- builder, prebuildPlugin, _ := newInjectTestBuilder(t, mod, nil) + builder, _, _ := newInjectTestBuilder(t, mod, nil) // Force prebuild esbuild path then fail: set entryPoint to missing file. builder.entryPoint = filepath.Join(t.TempDir(), "missing-entry.ts") - builder.prebuildPlugin = prebuildPlugin🤖 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/backend/inject_coverage_test.go` around lines 235 - 238, Remove the redundant builder.prebuildPlugin = prebuildPlugin assignment in the test setup around newInjectTestBuilder, since the returned plugin is already assigned to the builder’s plugin field and the variable name is misleading. Leave the missing-entry setup and failure assertions unchanged.
166-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the flush keys, not only the absence of an error.
Both tests are named for supersede flush-key behavior, but they only check that
persistModuleModelsreturns nil. A regression that computes the wrong flush keys, or computes none, would still pass. Add an assertion on the observable result, for example the raw declaration rows forFieldDefaultandAppSettingafter the call.Also applies to: 244-256
🤖 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/backend/inject_coverage_test.go` around lines 166 - 181, Update TestPersistModuleModels_SupersedeFlushKeys and the corresponding test around the second supersede case to assert the observable flush-key results after persistModuleModels succeeds. Inspect the raw declaration rows for both FieldDefault and AppSetting and verify they contain the expected flush keys, rather than only checking that no error was returned.
32-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck the
AutoMigrateerror.Line 32 fails the test when
EnsureDualStoreTablesfails, but line 35 discards theAutoMigrateerror. If theApplicationorModuletable is not created, later tests fail with unrelated query errors instead of a clear setup failure. Make the setup consistent.♻️ Proposed change
- _ = db.AutoMigrate(&meta.Application{}, &meta.Module{}) + if err := db.AutoMigrate(&meta.Application{}, &meta.Module{}); err != nil { + t.Fatal(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 `@internal/module/artifact/build/backend/inject_coverage_test.go` around lines 32 - 35, Handle the error returned by db.AutoMigrate in the test setup, alongside the existing EnsureDualStoreTables check. Fail the test immediately with t.Fatal when migration of meta.Application or meta.Module fails, rather than discarding the error.internal/module/artifact/build/injectappmodel/coverage_test.go (1)
346-351: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo assertions have empty bodies and cannot fail. Both sites evaluate a condition and place only a comment inside the block, so the check has no effect. Remove the dead blocks and keep the real assertions.
internal/module/artifact/build/injectappmodel/coverage_test.go#L346-L351: delete theisGeneratedPathblock with the empty body; line 353 already covers exact rel-path equality.internal/module/artifact/build/injectappmodel/coverage_test.go#L381-L391: delete the emptystrconvQuoteFallbackblock and thestrconvQuoteFallbackhelper; line 386 already checks the default application literal.🤖 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/coverage_test.go` around lines 346 - 351, Remove the empty isGeneratedPath assertion block in internal/module/artifact/build/injectappmodel/coverage_test.go lines 346-351; the exact relative-path assertion already provides coverage. Also remove the empty strconvQuoteFallback block and its helper in lines 381-391, preserving the existing assertion that checks the default application literal.
🤖 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.
Nitpick comments:
In `@internal/module/artifact/build/backend/inject_coverage_test.go`:
- Around line 235-238: Remove the redundant builder.prebuildPlugin =
prebuildPlugin assignment in the test setup around newInjectTestBuilder, since
the returned plugin is already assigned to the builder’s plugin field and the
variable name is misleading. Leave the missing-entry setup and failure
assertions unchanged.
- Around line 166-181: Update TestPersistModuleModels_SupersedeFlushKeys and the
corresponding test around the second supersede case to assert the observable
flush-key results after persistModuleModels succeeds. Inspect the raw
declaration rows for both FieldDefault and AppSetting and verify they contain
the expected flush keys, rather than only checking that no error was returned.
- Around line 32-35: Handle the error returned by db.AutoMigrate in the test
setup, alongside the existing EnsureDualStoreTables check. Fail the test
immediately with t.Fatal when migration of meta.Application or meta.Module
fails, rather than discarding the error.
In `@internal/module/artifact/build/injectappmodel/coverage_test.go`:
- Around line 346-351: Remove the empty isGeneratedPath assertion block in
internal/module/artifact/build/injectappmodel/coverage_test.go lines 346-351;
the exact relative-path assertion already provides coverage. Also remove the
empty strconvQuoteFallback block and its helper in lines 381-391, preserving the
existing assertion that checks the default application literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f598bf5d-1da5-48f8-bf3e-6a900adcbbfd
📒 Files selected for processing (15)
internal/module/artifact/build/backend/builder.gointernal/module/artifact/build/backend/inject_coverage_test.gointernal/module/artifact/build/backend/inject_host.gointernal/module/artifact/build/injectappmodel/bundle.gointernal/module/artifact/build/injectappmodel/compat.gointernal/module/artifact/build/injectappmodel/coverage_test.gointernal/module/artifact/build/injectappmodel/helpers.gointernal/module/artifact/build/injectappmodel/inject.gointernal/module/artifact/build/injectappmodel/injectappmodel_test.gointernal/module/artifact/build/injectappmodel/session.gointernal/module/artifact/build/injectappmodel/spec.gointernal/module/lifecycle/bundles_app_setting_test.gopkg/meta/facade.gopkg/meta/facade_coverage_test.gopkg/meta/facade_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/module/artifact/build/injectappmodel/inject.go
- internal/module/artifact/build/injectappmodel/helpers.go
- internal/module/lifecycle/bundles_app_setting_test.go
- internal/module/artifact/build/backend/builder.go
- internal/module/artifact/build/injectappmodel/bundle.go
- internal/module/artifact/build/injectappmodel/session.go
- pkg/meta/facade.go
- internal/module/artifact/build/injectappmodel/injectappmodel_test.go
- internal/module/artifact/build/injectappmodel/compat.go
- Add Registry with DefaultRegistry lazy builtins and package API forwards. - Bind NewSession to a registry and drive Decide/Inject/Bundle/Supersede from it. - Add WithInjectRegistry on ModuleBuilder and flush supersede keys via the session registry. - Migrate tests to NewRegistryWithDefaults/ResetClaims and deprecate package ResetScheduledForTest/ScheduledApps. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/registry.go`:
- Around line 123-129: Synchronize the complete claim-map operations in
TryClaim, ReleaseClaim, and ClaimOwner with ResetClaims using r.mu, so no claim
can be inserted, removed, or read concurrently with reset processing. Ensure
ResetClaims cannot finish before a concurrent TryClaim operation completes,
preserving the existing return behavior and claim ownership semantics.
🪄 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: 3e72fe08-2bc0-475f-9ad2-064b005bf425
📒 Files selected for processing (16)
internal/module/artifact/build/backend/builder.gointernal/module/artifact/build/backend/builder_test.gointernal/module/artifact/build/backend/inject_coverage_test.gointernal/module/artifact/build/backend/inject_host.gointernal/module/artifact/build/backend/inject_integration_test.gointernal/module/artifact/build/injectappmodel/bundle.gointernal/module/artifact/build/injectappmodel/compat.gointernal/module/artifact/build/injectappmodel/coverage_test.gointernal/module/artifact/build/injectappmodel/doc.gointernal/module/artifact/build/injectappmodel/inject.gointernal/module/artifact/build/injectappmodel/injectappmodel_test.gointernal/module/artifact/build/injectappmodel/register_builtin.gointernal/module/artifact/build/injectappmodel/registry.gointernal/module/artifact/build/injectappmodel/session.gointernal/module/artifact/build/injectappmodel/spec.gointernal/module/artifact/build/injectappmodel/supersede.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/module/artifact/build/injectappmodel/doc.go
- internal/module/artifact/build/backend/inject_integration_test.go
- internal/module/artifact/build/injectappmodel/bundle.go
- internal/module/artifact/build/injectappmodel/supersede.go
- internal/module/artifact/build/injectappmodel/inject.go
- internal/module/artifact/build/backend/inject_host.go
- internal/module/artifact/build/injectappmodel/injectappmodel_test.go
- internal/module/artifact/build/backend/builder.go
- internal/module/artifact/build/backend/builder_test.go
- internal/module/artifact/build/backend/inject_coverage_test.go
- Make injectappmodel return Effects (virtual files + imports) instead of writing through Host. - Bind Session to read-only BuildCtx; backend applies Effects onto esbuild plugins. - Migrate unit tests to assert Effects and mutate BuildCtx via Session.Context(). Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/inject.go`:
- Around line 26-28: The effects returned by these functions must contain
imports only for their current virtual files. In
internal/module/artifact/build/injectappmodel/inject.go lines 26-28 and
internal/module/artifact/build/injectappmodel/bundle.go lines 26-28, merge each
current fx.Imports into out alongside fx.Files instead of assigning session-wide
imports from sess.effectsImports(); in
internal/module/artifact/build/injectappmodel/inject.go lines 83-88, preserve
and return the imports produced by materializeInject without replacing them with
session-wide imports.
🪄 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: 4d677d50-daa8-43ea-93bf-a5b210194b46
📒 Files selected for processing (13)
internal/module/artifact/build/backend/inject_app_models.gointernal/module/artifact/build/backend/inject_coverage_test.gointernal/module/artifact/build/backend/inject_host.gointernal/module/artifact/build/injectappmodel/bundle.gointernal/module/artifact/build/injectappmodel/context.gointernal/module/artifact/build/injectappmodel/coverage_test.gointernal/module/artifact/build/injectappmodel/doc.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/session.gointernal/module/artifact/build/injectappmodel/supersede.go
💤 Files with no reviewable changes (1)
- internal/module/artifact/build/injectappmodel/helpers.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/module/artifact/build/injectappmodel/doc.go
- internal/module/artifact/build/injectappmodel/supersede.go
- internal/module/artifact/build/injectappmodel/injectappmodel_test.go
- internal/module/artifact/build/injectappmodel/coverage_test.go
- internal/module/artifact/build/backend/inject_coverage_test.go
- Merge inject_host and inject_app_models into inject.go; drop stale Host naming. - Merge coverage and integration tests into inject_test.go; remove leftover sessionDB helper. Co-authored-by: Cursor <cursoragent@cursor.com>
- Hold the registry mutex for the full TryClaim/ReleaseClaim/ClaimOwner path and replace claim maps on ResetClaims to avoid stale post-reset stores. - Return Effects.Imports only for files produced by the current inject/bundle call. Co-authored-by: Cursor <cursoragent@cursor.com>
- Re-extract meta.pot and sync zh_CN.po for new Meta*Raw model fields. - Fill zh_CN msgstr from existing non-Raw Meta* field translations. Co-authored-by: Cursor <cursoragent@cursor.com>
- Exercise nil BuildCtx, mergeImportPaths edge cases, and BundleInjectAppModels errors. - Cover lazy session maps, package specsList, and claimMapLocked nil-map paths. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@cursor review |
- Drop remembered inject paths if Effects were not applied, so reused builders cannot import stale generated files via buildOptions. - Cover partial FieldDefault success followed by AppSetting Decide failure. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@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 1eca959. Configure here.
User description
Summary
meta.Entities(), unexport dual-store migrate helpers, delete unusedmetaeff, and addFlushEffective/CatalogEntities.injectappmodel(InjectAppModels/SupersedeInjectAppModels/BundleInjectAppModels); supersede deletes declarations only.EnsureAbstractModel/ReplaceModuleDeclarations; flush effective once at persist (and i18n ACL) boundaries.Raw*types; tests seed/query via declaration facades and raw table helpers.Test plan
go test ./pkg/meta/ -count=1go test ./internal/module/artifact/build/injectappmodel/ ./internal/module/artifact/build/backend/ -count=1go test ./internal/module/lifecycle/ ./internal/i18n/models/ ./internal/module/evolution/schema/ -count=1__generated__declaration without double Flush mid-supersedemeta_raw_*viaCatalogEntities/DualStoreRawEntitiesMade with Cursor
PR Type
Enhancement
Description
Go Core: Unify FieldDefault and AppSetting inject logic into a new
injectappmodelpackage with a centralSpecregistry andSession.Go Core: Unexport raw dual-store types and migrate helpers in
pkg/meta, introducing clean declaration facades (EnsureAbstractModel,ReplaceModuleDeclarations,FlushEffective).Go Core: Optimize effective projection recomputation by flushing once at persist/install boundaries, and remove the redundant
metaeffpackage.TS Modules & Compliance: No TypeScript changes in
modules/; all 19 new Go files include SPDX LGPL-3.0 headers and are covered by unit tests.File Walkthrough
8 files
Use EnsureAbstractModel facade and FlushEffective for I18n ACL seedingDelegate AppSetting C2 inject and plan management to injectappmodelUnify app model inject and declaration flushing during module buildsDelegate FieldDefault C2 inject and plan management to injectappmodelInclude DualStoreRawEntities in default entity list for AutoMigrateUnexport RawModel struct and adjust ListDeclarations to rawModelAdd FlushEffective alias and unexport internal tree deletion helpersExpose CatalogEntities and DualStoreRawEntities for schema migrations2 files
Add declaration test helpers for raw model seeding and assertionAdd unit tests for injectappmodel session, plan, and supersede9 files
Add builder methods adapting ModuleBuilder to injectappmodel sessionImplement injectappmodel Host interface adapter for ModuleBuilderAdd multi-app bundle source registration for inject modelsImplement Decide, ApplyInject, and Validate for injectable app modelsManage per-build inject plans, schedules, and virtual import pathsDefine Spec registry for injectable application modelsImplement declaration tree deletion for superseded generated modelsAdd public EnsureAbstractModel and ReplaceModuleDeclarations facadesExpose raw table constants and helper queries without exporting Raw*1 files
Remove obsolete metaeff package in favor of pkg/meta APIs51 files
Summary by CodeRabbit
New Features
Improvements