refactor: implement mutation backends and migrate handlers#237
refactor: implement mutation backends and migrate handlers#237ako merged 5 commits intomendixlabs:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR continues the shared-types refactor by moving Mendix project value types and mutation/serialization responsibilities into backend interfaces, reducing direct sdk/mpr / BSON coupling in the executor and improving testability via mock backends.
Changes:
- Introduces
mdl/types(WASM-safe shared types) and migrates multiple handlers/readers fromsdk/mprtypes tomdl/types. - Adds backend interfaces for page/workflow mutation and widget serialization/building, and wires the executor to use them.
- Adds extensive mock-based tests for executor commands and updates widget engine/registry validation accordingly.
Reviewed changes
Copilot reviewed 117 out of 137 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| mdl/types/doc.go | Adds package docs clarifying purpose and conversion layer location. |
| mdl/types/asyncapi.go | Adds AsyncAPI 2.x parsing into WASM-safe shared types. |
| mdl/linter/rules/page_navigation_security.go | Switches navigation menu traversal to mdl/types menu item types. |
| mdl/executor/widget_templates.go | Removes executor-side BSON widget template helpers (moved behind backend). |
| mdl/executor/widget_registry_test.go | Updates tests to match new operation validation approach. |
| mdl/executor/widget_registry.go | Replaces OperationRegistry with a fixed knownOperations set for validation. |
| mdl/executor/widget_operations.go | Removes executor-side operation registry and operation implementations. |
| mdl/executor/widget_engine_test.go | Updates tests to validate knownOperations and removes registry tests. |
| mdl/executor/widget_defaults.go | Removes executor-side object-list default population (moved behind backend). |
| mdl/executor/mock_test_helpers_test.go | Adds shared mock context/model factories + assertion helpers for executor tests. |
| mdl/executor/helpers.go | Migrates folder info + ID generation to mdl/types. |
| mdl/executor/executor.go | Migrates cached unit/folder info to mdl/types. |
| mdl/executor/cmd_workflows_write.go | Migrates workflow ID generation to mdl/types. |
| mdl/executor/cmd_workflows_mock_test.go | Adds mock-based tests for workflow show/describe. |
| mdl/executor/cmd_settings_mock_test.go | Adds mock-based tests for settings show/describe. |
| mdl/executor/cmd_security_write.go | Migrates security member-access and revocation types to mdl/types. |
| mdl/executor/cmd_security_mock_test.go | Adds mock-based tests for multiple security commands. |
| mdl/executor/cmd_rest_clients_mock_test.go | Adds mock-based tests for REST client show/describe. |
| mdl/executor/cmd_rename.go | Migrates rename hit types to mdl/types. |
| mdl/executor/cmd_published_rest_mock_test.go | Adds mock-based tests for published REST services. |
| mdl/executor/cmd_pages_mock_test.go | Adds mock-based tests for pages/snippets/layouts listing. |
| mdl/executor/cmd_pages_create_v3.go | Wires widgetBackend into the page builder for V3 creation. |
| mdl/executor/cmd_pages_builder_v3_pluggable.go | Uses bsonutil IDs and backend widget serialization hook. |
| mdl/executor/cmd_pages_builder_v3_layout.go | Migrates layout/widget ID generation to mdl/types. |
| mdl/executor/cmd_pages_builder_input_filters.go | Uses bsonutil and mdl/types IDs for filter widget BSON creation. |
| mdl/executor/cmd_pages_builder_input_cloning_test.go | Updates cloning tests to use bsonutil IDs. |
| mdl/executor/cmd_pages_builder_input_cloning.go | Replaces sdk/mpr ID helpers with bsonutil/mdl/types. |
| mdl/executor/cmd_pages_builder_input.go | Removes executor-side datasource serialization and uses bsonutil IDs for refs. |
| mdl/executor/cmd_pages_builder.go | Adds widget backend dependency and migrates folder info caching to mdl/types. |
| mdl/executor/cmd_odata_mock_test.go | Adds mock-based tests for OData commands. |
| mdl/executor/cmd_odata.go | Switches EDMX parsing to mdl/types and ID generation to mdl/types. |
| mdl/executor/cmd_notconnected_mock_test.go | Adds mock-based “not connected” tests for many commands. |
| mdl/executor/cmd_navigation_mock_test.go | Adds mock-based tests for navigation show/describe. |
| mdl/executor/cmd_navigation.go | Migrates navigation document/spec types to mdl/types. |
| mdl/executor/cmd_modules_mock_test.go | Adds mock-based tests for modules listing. |
| mdl/executor/cmd_misc_mock_test.go | Adds mock-based tests for version output. |
| mdl/executor/cmd_microflows_mock_test.go | Adds mock-based tests for microflows/nanoflows show/describe. |
| mdl/executor/cmd_microflows_create.go | Migrates microflow IDs to mdl/types. |
| mdl/executor/cmd_microflows_builder_workflow.go | Migrates workflow-related microflow builder IDs to mdl/types. |
| mdl/executor/cmd_microflows_builder_graph.go | Migrates graph builder IDs to mdl/types. |
| mdl/executor/cmd_microflows_builder_flows.go | Migrates sequence flow IDs to mdl/types. |
| mdl/executor/cmd_microflows_builder_control.go | Migrates split/merge/loop IDs to mdl/types. |
| mdl/executor/cmd_microflows_builder_annotations.go | Migrates annotation and event IDs to mdl/types. |
| mdl/executor/cmd_mermaid_mock_test.go | Adds mock-based test for Mermaid domain model rendering. |
| mdl/executor/cmd_jsonstructures_mock_test.go | Adds mock-based tests for JSON structures listing. |
| mdl/executor/cmd_jsonstructures.go | Migrates JSON structure types/helpers to mdl/types. |
| mdl/executor/cmd_javascript_actions_mock_test.go | Adds mock-based tests for JavaScript actions. |
| mdl/executor/cmd_javaactions_mock_test.go | Adds mock-based tests for Java actions. |
| mdl/executor/cmd_javaactions.go | Migrates Java action ID generation to mdl/types. |
| mdl/executor/cmd_import_mappings_mock_test.go | Adds mock-based tests for import mappings listing. |
| mdl/executor/cmd_import_mappings.go | Migrates JSON element types (schema alignment) to mdl/types. |
| mdl/executor/cmd_imagecollections_mock_test.go | Adds mock-based tests for image collections show/describe. |
| mdl/executor/cmd_imagecollections.go | Migrates image collection types to mdl/types. |
| mdl/executor/cmd_fragments_mock_test.go | Adds mock-based tests for fragments listing. |
| mdl/executor/cmd_folders.go | Migrates folder info usage to mdl/types. |
| mdl/executor/cmd_export_mappings_mock_test.go | Adds mock-based tests for export mappings listing. |
| mdl/executor/cmd_export_mappings.go | Migrates JSON element types (schema alignment) to mdl/types. |
| mdl/executor/cmd_enumerations_mock_test.go | Refactors enumeration tests to common mock helpers and adds describe tests. |
| mdl/executor/cmd_entities_mock_test.go | Adds mock-based tests for entities listing. |
| mdl/executor/cmd_entities.go | Migrates entity-related ID generation to mdl/types. |
| mdl/executor/cmd_diff_local.go | Moves microflow parsing from sdk/mpr to backend method. |
| mdl/executor/cmd_dbconnection_mock_test.go | Adds mock-based tests for database connections show/describe. |
| mdl/executor/cmd_datatransformer_mock_test.go | Adds mock-based tests for data transformers list/describe. |
| mdl/executor/cmd_constants_mock_test.go | Adds mock-based tests for constants show/describe. |
| mdl/executor/cmd_businessevents_mock_test.go | Adds mock-based tests for business event service commands. |
| mdl/executor/cmd_businessevents.go | Migrates ID generation to mdl/types. |
| mdl/executor/cmd_associations_mock_test.go | Adds mock-based tests for association listing. |
| mdl/executor/cmd_agenteditor_mock_test.go | Adds mock-based tests for agent editor commands. |
| mdl/catalog/builder_references.go | Migrates navigation menu item reference extraction to mdl/types. |
| mdl/catalog/builder_navigation.go | Migrates navigation indexing functions to mdl/types. |
| mdl/catalog/builder_contract.go | Migrates EDMX/AsyncAPI parsing to mdl/types. |
| mdl/catalog/builder.go | Updates CatalogReader interface to use mdl/types shared structs. |
| mdl/bsonutil/bsonutil.go | Adds BSON-safe UUID helpers without sdk/mpr dependency. |
| mdl/backend/workflow.go | Migrates image collection types to mdl/types. |
| mdl/backend/security.go | Migrates member access + revocation types to mdl/types. |
| mdl/backend/navigation.go | Migrates navigation types/specs to mdl/types. |
| mdl/backend/mutation.go | Adds mutation + widget builder/serialization backend interfaces. |
| mdl/backend/mock/mock_workflow.go | Updates mock backend to use mdl/types image collection types. |
| mdl/backend/mock/mock_security.go | Updates mock backend to use mdl/types revocation types. |
| mdl/backend/mock/mock_navigation.go | Updates mock backend to use mdl/types navigation types/specs. |
| mdl/backend/mock/mock_mutation.go | Adds mock methods for new mutation/serialization/builder interfaces. |
| mdl/backend/mock/mock_module.go | Updates folder listing mocks to mdl/types. |
| mdl/backend/mock/mock_microflow.go | Adds mock hook for backend microflow parsing from raw BSON maps. |
| mdl/backend/mock/mock_mapping.go | Updates JSON structure types to mdl/types. |
| mdl/backend/mock/mock_java.go | Updates Java/JavaScript action types to mdl/types. |
| mdl/backend/mock/mock_infrastructure.go | Updates raw unit + rename + widget type mocks to mdl/types; makes agent editor create/delete overrideable. |
| mdl/backend/mock/mock_connection.go | Migrates version/project version types to mdl/types. |
| mdl/backend/mock/backend.go | Extends mock backend surface to cover new interfaces and mdl/types types. |
| mdl/backend/microflow.go | Adds ParseMicroflowFromRaw to backend API. |
| mdl/backend/mapping.go | Migrates mapping backend JSON structure types to mdl/types. |
| mdl/backend/java.go | Migrates Java backend list/read API types to mdl/types. |
| mdl/backend/infrastructure.go | Migrates rename/raw unit/metadata/widget type APIs to mdl/types. |
| mdl/backend/doc.go | Updates docs to reflect mdl/types extraction. |
| mdl/backend/connection.go | Migrates connection + folder backend types to mdl/types. |
| mdl/backend/backend.go | Extends FullBackend to include mutation + serialization + widget builder backends. |
| cmd/mxcli/project_tree.go | Migrates menu tree building to mdl/types.NavMenuItem. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
retran
force-pushed
the
feature/mutation-backends
branch
2 times, most recently
from
0adddbc to
55b21eb
Compare
Code ReviewOverviewThis is the largest PR in the stack (+12,710/-5,703 lines, 28 files) and delivers the actual implementation behind the interfaces defined in PR #236. The executor's Note: This PR is currently in DRAFT state. Positive Aspects
Issues & Concerns1. Potential method visibility issue in The analysis flagged that the 2. Workflow mutation operations are undertested
These are the most complex mutation operations and the most likely to have edge case bugs (e.g., position handling, sub-flow construction, deduplication). The corresponding 3. BSON helpers are duplicated across three files
4. No
5. The parameter remapping logic when changing a page's layout is the most complex method in the mutator. It correctly handles old/new layout qualified names and argument list remapping, but the complexity makes it a candidate for dedicated tests — none exist currently. Checklist Against Project Standards
VerdictStrong work overall — the refactoring goal is achieved. The executor is genuinely decoupled from BSON, the implementations are thorough, and the page-side tests are solid. The main gaps before this moves out of DRAFT are: (1) verify the |
Implement PageMutator, WorkflowMutator, and WidgetBuilderBackend in mdl/backend/mpr/. Rewrite ALTER PAGE (1721→256 lines) and ALTER WORKFLOW (887→178 lines) as thin orchestrators using mutator sessions. Implement PluggableWidgetEngine with WidgetObjectBuilder interface, eliminating all BSON from widget_engine.go. - Create mdl/backend/mpr/page_mutator.go (1554 lines) - Create mdl/backend/mpr/workflow_mutator.go (771 lines) - Create mdl/backend/mpr/widget_builder.go (1007 lines) - Migrate SerializeWidget/ClientAction/DataSource to backend interface - Add ParseMicroflowFromRaw to MicroflowBackend interface - Delete widget_operations.go, widget_templates.go, widget_defaults.go - Move ALTER PAGE/WORKFLOW tests to backend/mpr/ package
retran
force-pushed
the
feature/mutation-backends
branch
2 times, most recently
from
96b12d4 to
a2d0752
Compare
AI Code ReviewCritical Issues
Moderate Issues
Minor Issues
What Looks Good
RecommendationApprove. This PR is a well-executed refactor that successfully implements the mutation backends as described in PR #236, moves BSON manipulation logic behind the backend abstraction, simplifies executors to thin orchestrators, relocates tests appropriately, and adds comprehensive test coverage for the conversion layer. The changes are focused, consistent with project patterns, and maintain all existing functionality while improving architecture. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
|
Thanks for the thorough review! Addressing each point: #1 (Method visibility): Verified — no issue. #2 (Workflow mutation tests): Fixed in Also discovered and documented a #3 (BSON helpers dedup): Already addressed in PR #238 — #4 (MockPageMutator/MockWorkflowMutator): Deferred. These ~30-method FuncField mock types would enable executor-level isolation tests for #5 (SetLayout tests): Fixed in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AI Code ReviewLet me review this pull request against the checklist. First, I need to understand what this PR does. From the PR description:
Now let me go through the checklist: Overlap & duplication
MDL syntax designThis PR doesn't appear to add or modify MDL syntax. It's refactoring the implementation behind existing MDL syntax (ALTER PAGE, ALTER WORKFLOW). The PR description says it's implementing the mutation interfaces defined in PR #236. So no new MDL syntax is being added here. Full-stack consistencySince this PR doesn't add new MDL syntax or features (it's implementing existing interfaces), the full-stack consistency check doesn't apply. The grammar, AST, visitor, etc. would have been handled in PR #236 which defined the interfaces. Test coverage
Security & robustness
Scope & atomicity
Code quality
Bugs & correctness
Now let me look at specific concerns from the visible diff:
One potential issue I notice: In But looking at the ContainerKind type (from PR #236 which we don't see here), I wonder if it's comparing string to enum correctly. However, in the OpenPageForMutation function, I see: And then later: So the comparison in SetLayout should be: Not: This looks like a bug! It's comparing a backend.ContainerKind enum value to a string literal. Let me double-check this. In the SetLayout function: But m.containerType is of type backend.ContainerKind, which is likely an enum/constant type defined in backend.go. Looking at the diff for backend.go: This doesn't show the ContainerKind definition. But from the OpenPageForMutation function in mpr/backend.go, we can see it's being set to backend.ContainerPage, backend.ContainerSnippet, etc. So yes, this is a bug. It should be comparing to the enum constant, not a string. Let me see if I can find where ContainerKind is defined. It's not in the visible diff, but it's referenced. Actually, looking more carefully at the OpenPageForMutation function in mpr/backend.go: And then: So ContainerKind is definitely a type in the backend package, and ContainerSnippet is a value of that type. Therefore, in SetLayout, it should be: Not: This is a critical bug that would cause the SET Layout check to always fail (since an enum value will never equal a string), allowing SET Layout on snippets when it should be prohibited. Let me check if there are other similar issues. In the same file, I see: This logic seems okay for extracting the module name from a qualified name like "Module.LayoutName" -> "Module". But then later: This comparison might be problematic if newLayout is expected to be a qualified name but oldLayoutQN is just the module name. Let me see how it's used. Actually, looking at the usage: So newLayout is expected to be a module name (since it's concatenated with "." and placeholder), and oldLayoutQN was extracted as the module name from the parameter (e.g., from "Module.LayoutName" we get "Module"). So the comparison But the initial check for snippet should definitely be fixed. Another thing: In the SetLayout function, there's: But newLayout here should be the qualified name (e.g., "MyModule.MyLayout"), not just the module name. Let me check. Actually, looking at the parameter: This suggests that newLayout is expected to be just the module name, and the layout name (placeholder) is appended to it. But then the early return check However, the Form field should store the qualified name, not just the module name. Let me check how it's stored originally. In the detection loop: Here, s is the full qualified name (e.g., "MyModule.MyLayout"), but then: So oldLayoutQN becomes just the module name ("MyModule"). Then when updating: If newLayout is just the module name (as used later in This is another bug. Let me trace through:
Actually, wait. The check is: But newLayout parameter is supposed to be the new layout reference. If the user says "SET Layout TO MyModule.NewLayout", then newLayout should be "MyModule.NewLayout". But in the code, when updating the Form field, it does: And when updating parameters: This suggests that newLayout is expected to be just the module name, and the layout-specific part comes from the parameter mapping. But that doesn't make sense for the Form field, which should store the qualified layout name. Let me look at how this is called. I don't see the caller in the diff, but presumably from the executor. Actually, re-reading the SetLayout function signature and usage: The function is: And in the Form field update: In the parameter update: This implies that newLayout is the module name only. But then the detection: Here s is the full qualified name from the Form field. Then: So oldLayoutQN becomes the module name. Then the check: But if the user changes from "MyModule.MyLayout" to "MyModule.NewLayout", the module name doesn't change, so oldLayoutQN == newLayout would be true, and it would return early without making any changes - which is wrong because the layout name did change. Ah, I see the issue. The check should be comparing the full qualified names, not just the module names. Actually, no. Let's think about what SET Layout does. In Mendix, when you SET Layout, you're changing which layout the page uses. The layout is identified by a qualified name like "Module.LayoutName". The Form field in the page's FormCall stores the qualified name of the layout. When you change layouts, you might also need to update parameter mappings if the new layout has different parameters. The function receives:
Looking at the parameter update logic: This strongly suggests that newLayout is expected to be just the module name, and placeholder is the layout name (e.g., "MyLayout"). But then what should go in the Form field? It should be the qualified layout name, which would be newLayout + "." + (some layout name). But which layout name? It should be determined from the parameter mappings or something. Actually, looking more carefully at the detection of oldLayoutQN: For the Form field: So oldLayoutQN gets the full qualified name from the Form field (e.g., "MyModule.MyLayout"). Then for parameters: This is trying to extract the module name from a parameter value that might be a qualified name like "Module.Something". But there's a bug here: This looks wrong. Let me re-read: The condition Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
|
Thanks for the thorough review. Addressing the two findings: 1. 2. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AI Code ReviewWe are given a diff of a pull request that refactors the mutation backends and migrates handlers. Since the diff is very long (truncated to 80k characters, total 387634 bytes), we must focus on the key aspects. However, note that the PR description says: We are to check:
Let's analyze the provided diff: The diff shows changes in:
We note that the PR also mentions:
However, the diff provided does not show the executor files (cmd_alter_page.go, cmd_alter_workflow.go) or the widget_builder.go. Since the diff is truncated, we must rely on what we see and the description. Let's break down by checklist:
Given the above, we can summarize: Critical Issues: None found.
Minor Issues:
What Looks Good:
Recommendation: Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- 33 round-trip and wrapper tests for convert/unconvert layer covering all forward conversions, reverse (unconvert) paths, nil handling, and error passthrough - 48+ workflow mutator tests: InsertAfterActivity, ReplaceActivity, InsertOutcome, DropOutcome, InsertPath, DropPath, InsertBranch, DropBranch, InsertBoundaryEvent, SetActivityProperty, SetPropertyWithEntity - 6 SetLayout tests: basic layout change, explicit param mappings, same-layout no-op, snippet rejection, missing FormCall, empty Form - Widget engine test updates for applyChildSlots operation validation
- Add NewWidgetRegistryWithOps(extraOps) for extending known operations - Move knownOperations from package-level var to WidgetRegistry field to eliminate global mutable state race - Convert validateDefinitionOperations and validateMappings to WidgetRegistry methods - Use backend.ContainerSnippet constant in SetLayout check
- Fix SetPropertyWithEntity for OVERVIEW_PAGE: pass entity value instead of empty string (was clearing AdminPage instead of setting it) - Split OVERVIEW_PAGE and PARAMETER handling in cmd_alter_workflow.go - Reorder Build(): auto-datasource before applyChildSlots so entityContext is available for child widgets - Validate slot.Operation in applyChildSlots (must be "widgets") - setWidgetCaption returns validation error when Caption missing (matching setWidgetContent behavior) - extractBinaryIDFromDoc handles []byte in addition to primitive.Binary
retran
force-pushed
the
feature/mutation-backends
branch
from
8d8017f to
10f460d
Compare
|
Thanks for the review. Addressing each point: Moderate — Minor — PR size: Acknowledged. This is the largest PR in a 5-PR stack (#235→#236→#237→#238→#239). Each PR is scoped to a single concern, and this one is the implementation that backs the interfaces defined in #236. The size is a consequence of moving ~50 handler functions from executor to backend — splitting further would create partial migrations that don't compile independently. Minor — |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
mdl/executor/cmd_diff_local.go:503
microflowBsonToMDLclaims it "Falls back to a header-only stub if parsing fails", but the current implementation unconditionally callsrenderMicroflowMDLon the result ofParseMicroflowFromRaw. If the backend returns nil (or in future if parsing can fail), this will panic. Add a nil check (and/or recover) and return a minimal stub MDL when parsing fails to match the comment and prevent crashes in diff-local.
// microflowBsonToMDL converts a microflow BSON to MDL using the same
// renderer as DESCRIBE MICROFLOW, so diffs include activity bodies.
// Falls back to a header-only stub if parsing fails.
func microflowBsonToMDL(ctx *ExecContext, raw map[string]any, qualifiedName string) string {
qn := splitQualifiedName(qualifiedName)
mf := ctx.Backend.ParseMicroflowFromRaw(raw, model.ID(qn.Name), "")
entityNames, microflowNames := buildNameLookups(ctx)
return renderMicroflowMDL(ctx, mf, qn, entityNames, microflowNames, nil)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // InsertWidget inserts serialized widgets at the given position | ||
| // relative to the target widget. | ||
| InsertWidget(targetWidget string, position InsertPosition, widgets []pages.Widget) error | ||
| // relative to the target widget or column. Position is "before" or "after". | ||
| // columnRef is "" for widget targeting; non-empty for column targeting. | ||
| InsertWidget(widgetRef string, columnRef string, position InsertPosition, widgets []pages.Widget) error | ||
|
|
||
| // DropWidget removes widgets by name from the tree. | ||
| DropWidget(widgetRefs []string) error | ||
| // DropWidget removes widgets by ref from the tree. | ||
| DropWidget(refs []WidgetRef) error | ||
|
|
||
| // ReplaceWidget replaces the target widget with the given widgets. | ||
| ReplaceWidget(targetWidget string, widgets []pages.Widget) error | ||
| // ReplaceWidget replaces the target widget or column with the given widgets. | ||
| // columnRef is "" for widget targeting. | ||
| ReplaceWidget(widgetRef string, columnRef string, widgets []pages.Widget) error |
There was a problem hiding this comment.
The PageMutator API mixes WidgetRef (used by DropWidget) with separate widgetRef/columnRef string parameters (used by InsertWidget/ReplaceWidget). This makes call sites error-prone (easy to swap/forget columnRef) and undermines the purpose of introducing WidgetRef. Consider updating InsertWidget/ReplaceWidget to accept a single WidgetRef (or consistently use separate strings everywhere) to keep the API coherent and harder to misuse.
There was a problem hiding this comment.
Valid API design concern. The current split exists because InsertWidget/ReplaceWidget need to target both widget-level and column-level positions (columnRef disambiguates), while DropWidget only needs widget identification. Unifying to WidgetRef would require WidgetRef to carry an optional column field — tracked for a follow-up API cleanup.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
mdl/executor/widget_engine.go:155
- Auto-datasource selection is based on ranging over
propertyTypeIDs(a Go map). Since map iteration order is randomized, the chosen "first" DataSource property can vary between runs, leading to non-deterministic widget output and potentially differententityContextresolution. Consider selecting deterministically (e.g., collect matching keys, sort, then pick the first) or using template-defined property order if available.
if ds := w.GetDataSource(); ds != nil {
for propKey, entry := range propertyTypeIDs {
if entry.ValueType == "DataSource" {
dataSource, entityName, err := e.pageBuilder.buildDataSourceV3(ds)
if err != nil {
return nil, mdlerrors.NewBackend("auto datasource for "+propKey, err)
}
builder.SetDataSource(propKey, dataSource)
if entityName != "" {
e.pageBuilder.entityContext = entityName
}
break
}
mdl/backend/mpr/backend.go:762
MprBackendstill implements severalWidgetSerializationBackendmethods viapanic("not yet implemented"). Sincebackend.FullBackendincludes this interface, any call into these methods will hard-crash at runtime. Consider either implementing these using the existingmpr.Serialize*helpers (as done forSerializeDataSource) or removing them fromFullBackendif they are no longer part of the supported backend contract.
func (b *MprBackend) SerializeWidget(w pages.Widget) (any, error) {
panic("MprBackend.SerializeWidget not yet implemented") // TODO: implement in PR #237
}
func (b *MprBackend) SerializeClientAction(a pages.ClientAction) (any, error) {
panic("MprBackend.SerializeClientAction not yet implemented") // TODO: implement in PR #237
}
func (b *MprBackend) SerializeDataSource(ds pages.DataSource) (any, error) {
return mpr.SerializeCustomWidgetDataSource(ds), nil
}
func (b *MprBackend) SerializeWorkflowActivity(a workflows.WorkflowActivity) (any, error) {
panic("MprBackend.SerializeWorkflowActivity not yet implemented") // TODO: implement in PR #237
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor IssuesNone found. What Looks Good
RecommendationApprove - This PR represents a successful refactor that improves code organization while maintaining all existing functionality. The changes are well-scoped, properly tested, and align with the architectural goals outlined in the PR description and previous PRs in the stack. No blocking issues were identified during review. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Why
PR #236 defined the mutation interfaces — this PR provides the MPR-backed implementations that actually perform the BSON manipulation. The goal is to move all BSON mutation logic out of the executor and behind the backend abstraction, so the executor becomes a thin orchestrator.
What changed (incremental from PR #236)
28 files changed, +4,511/-3,523
New backend implementations:
mdl/backend/mpr/page_mutator.go(1,554 lines) — fullPageMutatorimplementation: widget tree traversal, property updates, widget add/drop/move, layout changes, pluggable widget operations, all operating on BSON documentsmdl/backend/mpr/workflow_mutator.go(771 lines) — fullWorkflowMutatorimplementation: activity insertion/removal, outcome management, path operations, boundary eventsmdl/backend/mpr/widget_builder.go(1,007 lines) — widget construction and serialization:WidgetObjectBuilder, property type resolution, default widget scaffoldingExecutor simplification:
cmd_alter_page.go: 1,721 → 256 lines — rewritten as thin orchestrator callingPageMutatormethodscmd_alter_workflow.go: 887 → 178 lines — same pattern withWorkflowMutatorwidget_engine.gorefactored to useWidgetObjectBuilderinterface instead of direct BSON constructionwidget_defaults.go,widget_operations.go,widget_templates.go(logic moved to backend)Interface integration from #236:
ContainerKind,InsertPosition,PluggablePropertyOpconstants defined in PR refactor: define mutation backend interfaces #236InsertWidgetsignature usesInsertPositiontyped parameter (case-insensitive comparison in implementation)Tests relocated: ALTER PAGE and ALTER WORKFLOW tests moved from executor to
mdl/backend/mpr/where the implementation now lives.Review-driven improvements (post-PR creation):
NewWidgetRegistryWithOps(extraOps)— reintroduces extensibility injection point for user-defined widget operations that was lost whenOperationRegistrywas replaced by a fixedknownOperationsset.NewWidgetRegistry()remains the zero-config default.convert/unconvertlayer inmdl/backend/mpr/convert_roundtrip_test.go— 33 tests covering all forward conversion, reverse unconvert, nil/error passthrough, and Slice/Ptr wrapper functions.Stack
PR 3/5 in the shared-types refactoring stack. This is the largest PR in the stack.
Merge order: #232 → #235 → #236 → this → #238 → #239