Skip to content

feat: pluggable widget engine v2#68

Open
engalar wants to merge 3 commits intomendixlabs:mainfrom
engalar:feat/widget-engine-v2
Open

feat: pluggable widget engine v2#68
engalar wants to merge 3 commits intomendixlabs:mainfrom
engalar:feat/widget-engine-v2

Conversation

@engalar
Copy link
Copy Markdown
Contributor

@engalar engalar commented Apr 1, 2026

Stacked on #67. PLUGGABLEWIDGET syntax, auto datasource/child slots, 20 widget templates, widget CLI.

engalar and others added 3 commits April 1, 2026 09:58
…D reuse

BSON int32→int64:
- All property values in writer serialization changed from int32 to int64
  to match Studio Pro's expected BSON types
- Array version markers (int32(2), int32(3)) preserved as int32
- Fixes Studio Pro crash: "Sequence contains no matching element" in
  MprProperty..ctor when type cache expects int64 but finds int32

CREATE OR REPLACE PAGE:
- Reuses existing page UUID and calls UpdatePage instead of DeletePage+
  CreatePage, producing git "Modified" diff instead of "Deleted+Added"
- Prevents RevStatusCache crash when parsing deleted mxunit base files

BSON roundtrip test baseline:
- Add golden mxunit files for roundtrip testing (pages, microflows, enums)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
diag --check-units:
- New command to detect orphan units (DB record, no file) and stale
  mxunit files (file exists, no DB record)
- --fix flag auto-removes stale files
- bson dump --format bson output format

Grammar fixes for MDL baseline scripts:
- Add DISPLAY, STRUCTURE to enumValueName rule
- Support String(unlimited) syntax in dataType
- Add ATTRIBUTE to attributeName rule
- Support parenthesized CREATE ASSOCIATION with optional colons
- Add SEND, REQUEST, TABLETWIDTH, PHONEWIDTH keywords

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PLUGGABLEWIDGET syntax with explicit properties, auto datasource,
auto child slots, TextTemplate attribute binding, and keyword properties.

Core changes:
- PLUGGABLEWIDGET 'widget.id' name (key: value) syntax
- CUSTOMWIDGET/TABCONTAINER/TABPAGE grammar tokens
- Auto datasource ordering (step 4.1, before child slots)
- Auto child slot matching by container name
- Object list auto-populate with Required filter
- TextTemplate {AttrName} parameter binding
- Widget lifecycle CLI (widget init/docs)
- DESCRIBE PLUGGABLEWIDGET output for generic widgets
- 20 new widget templates (mendix-11.6)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@ako ako left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

51K additions — this is a very large PR. It stacks on #67 (which stacks on #66), so all issues from both prior reviews carry forward. The new commit adds ~39K lines, of which ~28K are widget template JSON files.

Scope

This should be at least 3 separate PRs:

  1. Widget template JSON files (20 new templates) — bulk addition, easy to review separately
  2. Pluggable widget engine v2 (widget_engine.go, augment.go, loader.go, visitor/executor changes)
  3. Widget CLI commands (cmd_widget.go)

Bundling makes this effectively unreviewable at 51K lines.

Code quality issues

1. Silent nil return in deepCloneMap (augment.go)

deepCloneMap serializes to JSON then deserializes back. On error, it returns nil without signaling the caller. This causes cascading nils — cloned properties silently become nil, and downstream code gets mysterious failures instead of a clear error.

2. Unsafe type assertions throughout

Multiple places assume BSON structure shapes without checking. For example, createAttributeObject silently returns nil AttributeRef when the attribute path has fewer than 2 dots, instead of returning an error for invalid paths.

3. Entity context mutation (widget_engine.go)

entityContext is saved/restored around Build() but mutated in-place during DataSource mapping. If an error occurs partway through, the context is left corrupted. Should use an immutable context stack or pass context as a parameter.

4. Placeholder ID generation (augment.go)

Uses atomic.AddInt64 for a counter but ResetPlaceholderCounter() exists for testing — this is a race condition if tests run in parallel. The deterministic format also means IDs may collide across concurrent augmentations.

5. Mode selection swallows ambiguity (widget_engine.go)

When multiple modes have no condition, the first fallback is silently chosen with no warning. This hides accidental duplicate mode definitions that could produce wrong widget output.

6. Debug logging still present from PR #66

The log.Printf("SERIALIZE CHECK: timelineCustom Widgets: ...") debug code in writer_widgets_custom.go is still here from the base commits.

Missing error handling

  • buildAttributeObjects: No validation that the resolved attribute path actually exists in the domain model before creating a reference
  • Operations in widget_engine.go (opAssociation, etc.): Check if source is empty but don't validate the resolved context (e.g., EntityName being set)
  • containsPlaceholderID gives generic error messages — doesn't indicate which field has the unreplaced placeholder

Design concerns

JSON round-trip for deep clone

Using JSON marshal/unmarshal for deep cloning BSON structures is fragile and has performance implications. Custom BSON types or binary data won't survive the round-trip. A proper BSON deep-clone would be more robust.

Tight coupling to BSON

PluggableWidgetEngine receives and returns bson.D directly with no abstraction. This makes the engine untestable without real BSON structures and means any serialization format changes require rewriting the entire module.

Testing

  • sdk/mpr/roundtrip_test.go is a good addition (+212 lines) — this addresses the gap flagged in PR #66 review
  • However: no tests for deepCloneMap error paths, no tests for partial operation failures, no tests for mode fallback ambiguity, no tests for placeholder ID uniqueness under concurrency

Design docs committed

docs/plans/2026-03-30-widget-docs-and-customwidget.md and docs/plans/2026-03-30-widget-docs-generation-design.md are committed. Are these intended to live permanently, or should they be removed post-implementation? They describe planned work rather than documenting what was built.

Template files

20 new widget templates added under sdk/widgets/templates/mendix-11.6/. These are extracted from Studio Pro which is the right approach per the repo's guidelines. However:

  • Were all templates verified to include both type AND object fields as required by sdk/widgets/templates/README.md?
  • timeline.json shows +0 -0 (empty file?) — needs verification

Summary

The core widget engine design is reasonable, but the PR needs:

  1. Splitting into reviewable units
  2. Error handling hardened (especially deepCloneMap, attribute validation, context mutation)
  3. Debug logging removed
  4. Tests for error paths
  5. All issues from PR #66 and #67 reviews resolved first

@github-actions github-actions bot mentioned this pull request Apr 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants