feat(build): inject PropertyDefinition per app and purge on uninstall - #270
Conversation
- Register PropertyDefinition as a C2 Spec alongside FieldDefault/AppSetting. - Wire per-app bundle owners and uninstall purge for gone TargetModel/ContainerModel rows. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe PR adds ChangesPropertyDefinition integration
PropertyDefinition uninstall cleanup
Vue component handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant buildBackendAppToDir
participant ensureBundleC2VirtualImports
participant injectappmodel
participant BackendPlugin
buildBackendAppToDir->>ensureBundleC2VirtualImports: pass PropertyDefinition owner modules
ensureBundleC2VirtualImports->>injectappmodel: inject PropertyDefinition sources
injectappmodel-->>ensureBundleC2VirtualImports: generate virtual model imports
ensureBundleC2VirtualImports->>BackendPlugin: resolve property_definition.ts
BackendPlugin-->>buildBackendAppToDir: return virtual source resolution
sequenceDiagram
participant ModuleUninstaller
participant applyPropertyDefinitionPurge
participant PropertyDefinitionTable
ModuleUninstaller->>applyPropertyDefinitionPurge: purge removed model definitions
applyPropertyDefinitionPurge->>PropertyDefinitionTable: probe table and count surviving models
PropertyDefinitionTable-->>applyPropertyDefinitionPurge: return table and model state
applyPropertyDefinitionPurge->>PropertyDefinitionTable: delete obsolete target or container rows
PropertyDefinitionTable-->>ModuleUninstaller: return purge result
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! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/lifecycle/modulemanager.go`:
- Line 208: Update BundleInjectAppModels to use the last eligible module,
matching pickPropertyDefinitionOwnerModule, when injecting PropertyDefinition
imports through ensureBundleC2VirtualImports. Add a test with two eligible
modules and assert that the generated PropertyDefinition source path belongs to
the last eligible module.
🪄 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: 1069d6fe-6496-47f3-ac33-14b330c5e45e
📒 Files selected for processing (13)
internal/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/registry.gointernal/module/artifact/build/injectappmodel/session.gointernal/module/artifact/build/injectappmodel/spec.gointernal/module/lifecycle/bundles.gointernal/module/lifecycle/bundles_app_setting_test.gointernal/module/lifecycle/bundles_property_definition_test.gointernal/module/lifecycle/modulemanager.gointernal/module/lifecycle/uninstaller.gointernal/module/lifecycle/uninstaller_property_definition_test.go
- Extend the backend OnResolve filter so property_definition.ts virtual injects load like FieldDefault/AppSetting. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/esbplugins/backendplugin/virtual_source_test.go (1)
119-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise PropertyDefinition through the virtual-source loader.
TestVirtualC2SourceOnResolveFilterchecks only the filter.TestBackendPluginDefinePluginsOnLoad_ServesVirtualSourceexercises the shared resolver and loader withfield_default.ts, but notproperty_definition.ts. Add a table-drivenproperty_definition.tscase that checks the resolved path and loaded contents.🤖 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/esbplugins/backendplugin/virtual_source_test.go` around lines 119 - 138, Extend TestBackendPluginDefinePluginsOnLoad_ServesVirtualSource with a table-driven property_definition.ts case, alongside the existing field_default.ts case. Assert that the virtual-source resolver returns the expected property_definition.ts path and that the loader serves its expected contents, covering both path resolution and loading behavior.Source: Coding guidelines
🤖 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/esbplugins/backendplugin/virtual_source_test.go`:
- Around line 119-138: Extend
TestBackendPluginDefinePluginsOnLoad_ServesVirtualSource with a table-driven
property_definition.ts case, alongside the existing field_default.ts case.
Assert that the virtual-source resolver returns the expected
property_definition.ts path and that the loader serves its expected contents,
covering both path resolution and loading behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22920e7c-518f-4ef2-94ae-2bcfa31d046c
📒 Files selected for processing (2)
internal/esbplugins/backendplugin/plugin.gointernal/esbplugins/backendplugin/virtual_source_test.go
- Make BundleInjectAppModels last-wins per app and pass pick* owners from per-app bundles. - Cover property_definition through the backend virtual OnResolve/OnLoad path. Co-authored-by: Cursor <cursoragent@cursor.com>
- Exercise cleanModels failure when UserFilter or PropertyDefinition purge fails. - Extract and unit-test dialect missing-table error matching. Co-authored-by: Cursor <cursoragent@cursor.com>
- Drive buildBackendAppToDir with Path-backed modules so FieldDefault/AppSetting/PropertyDefinition owners are appended. Co-authored-by: Cursor <cursoragent@cursor.com>
- markRaw searchView before dynamic component render to avoid reactive Component warns. - Replace boolean component mocks with no-op stubs so slot children are valid VNodes. Co-authored-by: Cursor <cursoragent@cursor.com>
- Unwrap and markRaw dynamic searchView/icon Components so prop proxies are not treated as reactive. - Accept singleton defaultFilters, give firstframe search sentinels a render function, and skip inject outside setup. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
modules/web/web/composables/useOnchange.ts (1)
513-516: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for both controller contexts.
The new branch has two observable paths. A setup caller must still receive the injected
'view-mode'. A non-component caller must avoidinjectand use the fallback controller path. Add or update tests inmodules/web/web/composables/useOnchange.test.tsfor both paths.As per coding guidelines, build the CLI and generate ignored embedded assets before installing modules or running the application, install required local modules, and run the applicable formatting, build, typecheck, unit-test, and E2E-test checks.
🤖 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/web/web/composables/useOnchange.ts` around lines 513 - 516, Add regression tests in useOnchange.test.ts covering both getOnchangeController contexts: verify setup callers receive the injected 'view-mode' Ref, and non-component callers skip inject and use the fallback controller path. Follow the repository workflow by generating required embedded assets and building the CLI before installing modules, then run applicable formatting, build, typecheck, unit, and E2E checks.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/module/lifecycle/bundles_property_definition_test.go`:
- Around line 126-143: Update the test around buildBackendAppToDir to capture
the registered virtual imports before dropping meta_raw_model and forcing
injection failure. Assert that the PropertyDefinition import is supplied only by
crm_last, rather than inferring ownership from the generated entry file, while
preserving the existing failure and entry-generation assertions.
In `@modules/web/web/components/view/OListView.vue`:
- Around line 270-274: Use the normalized resolvedSearchView identity for
first-frame deferral: update shouldDeferViewFirstFrame calls in
modules/web/web/components/view/OListView.vue (lines 270-274) and
modules/web/web/components/view/OKanbanView.vue (lines 214-218) to pass
resolvedSearchView.value instead of props.searchView. Update the tests asserting
these calls to expect the normalized value.
---
Nitpick comments:
In `@modules/web/web/composables/useOnchange.ts`:
- Around line 513-516: Add regression tests in useOnchange.test.ts covering both
getOnchangeController contexts: verify setup callers receive the injected
'view-mode' Ref, and non-component callers skip inject and use the fallback
controller path. Follow the repository workflow by generating required embedded
assets and building the CLI before installing modules, then run applicable
formatting, build, typecheck, unit, and E2E checks.
🪄 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: 4bb8567b-7bac-42ce-9389-d67bdf0684ee
📒 Files selected for processing (19)
internal/esbplugins/backendplugin/plugin_test.gointernal/module/artifact/build/injectappmodel/bundle.gointernal/module/artifact/build/injectappmodel/injectappmodel_test.gointernal/module/lifecycle/bundles_property_definition_test.gointernal/module/lifecycle/modulemanager.gointernal/module/lifecycle/uninstaller.gointernal/module/lifecycle/uninstaller_property_definition_test.gointernal/module/lifecycle/uninstaller_user_filter_test.gomodules/web/web/components/view/OChartView.vuemodules/web/web/components/view/OKanbanView.firstframe.test.tsmodules/web/web/components/view/OKanbanView.vuemodules/web/web/components/view/OListView.firstframe.test.tsmodules/web/web/components/view/OListView.test.tsmodules/web/web/components/view/OListView.vuemodules/web/web/components/view/OSearchView.vuemodules/web/web/components/view/OStatInfo.test.tsmodules/web/web/components/view/OStatInfo.vuemodules/web/web/composables/useOnchange.tsmodules/web/web/pages/TerminologyEditor.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/module/lifecycle/modulemanager.go
- internal/module/lifecycle/uninstaller.go
- internal/module/artifact/build/injectappmodel/injectappmodel_test.go
- internal/module/lifecycle/uninstaller_property_definition_test.go
- Spy console.warn when exercising the missing relationStore fallback so CI logs stay clean. 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 `@modules/web/web/components/field/relationTypeahead.NameSearch.test.ts`:
- Around line 248-252: Update the test workflow around the withStore and
withoutStore wrappers to declare both wrapper variables before an outer
try/finally, then unmount every defined wrapper in finally. Ensure cleanup still
runs when clickRemote or any assertion throws, and avoid duplicate unmounts from
the existing inner cleanup.
🪄 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: cd3ecb3c-f335-4bf7-b5f6-95f9bc161267
📒 Files selected for processing (1)
modules/web/web/components/field/relationTypeahead.NameSearch.test.ts
- Assert C2 PropertyDefinition/FieldDefault last-owner on the app-build fixture; virtual import capture is impossible when inject fails in dbLoadModels. - Defer first-frame load using resolvedSearchView identity so reactive prop proxies cannot skip OSearchView wait. - Make OManyToOneRefField typeahead wrapper cleanup exception-safe, and cover view-mode inject gating for getOnchangeController. Co-authored-by: Cursor <cursoragent@cursor.com>
- Add OChartView mount coverage for truthy and falsy searchView resolution. - Mount OKanbanView without searchView to cover the null resolvedSearchView branch. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
User description
Summary
{app}.PropertyDefinitionas a C2 inject Spec (EnsureServiceEntry: false), matching FieldDefault / AppSetting force-delivery for non-core apps with a service entry.pickPropertyDefinitionOwnerModuleinto multi-app / per-app bundle C2 virtual imports so each eligible app gets exactly one definition store.meta_model(TargetModelorContainerModelmatch); missing per-app tables are a no-op; probe/delete failures fail uninstall (PP5 / SF7 rigor).Test plan
go test ./internal/module/artifact/build/injectappmodel/... -count=1go test ./internal/module/lifecycle/... -count=1./choysum test unit core --beMade with Cursor
PR Type
Enhancement, Tests
Description
Add PropertyDefinition to C2 inject specs (mirrors FieldDefault/AppSetting, EnsureServiceEntry: false)
Wire pickPropertyDefinitionOwnerModule into multi-app and per-app bundle builds
Purge PropertyDefinition rows on uninstall when no live meta_model remains
Extend tests for inject, bundles, and uninstall behavior
File Walkthrough
5 files
Update spec count assertions to include PropertyDefinitionExtend test for PropertyDefinition spec and injection validationUpdate existing test to pass propertyDefinitionOwnersNew tests for PropertyDefinition bundle owner selection and errorpathsAdd comprehensive tests for PropertyDefinition uninstall purge4 files
Update package doc comment to mention PropertyDefinitionUpdate comment to include PropertyDefinitionUpdate comments for PropertyDefinitionUpdate Spec doc to include PropertyDefinition4 files
Register PropertyDefinition as a built-in inject specAdd pickPropertyDefinitionOwnerModule and wire into bundle buildsPass nil propertyDefinitionOwners in single-app bundle buildImplement PropertyDefinition purge on uninstall when models goneSummary by CodeRabbit
New Features
Bug Fixes