Conversation
ako
left a comment
There was a problem hiding this comment.
Review
This PR adds one commit on top of #68: MDL baseline scripts for a WidgetDemo module (5 files, ~717 lines of new content). All issues from PRs #66/#67/#68 carry forward.
What's new in this commit
| File | Lines | Purpose |
|---|---|---|
01-domain-model.mdl |
53 | 3 enums, 2 entities, 1 association, security grant |
01b-snippet.mdl |
6 | Button showcase snippet |
02-showcase-page.mdl |
456 | 95-widget showcase page |
03-seed-and-navigate.mdl |
98 | Seed microflow + navigation grant |
WIDGET_ANALYSIS.md |
104 | Widget classification document |
Overall impression
The MDL scripts are well-structured and serve as a comprehensive integration test for the widget engine. The showcase page exercises 62 native and 33 pluggable widgets across 8 sections, which is good coverage. The scripts are self-contained (no external module dependencies).
Issues
1. Does this actually work?
The test plan checkbox "Studio Pro opens project without crash" is unchecked in PR #66 (which this stacks on), and this PR has no test plan at all. A 95-widget showcase page that depends on unreleased features from PRs #66-#68 needs validation. Has mxcli check been run against these scripts? Has the resulting project been opened in Studio Pro?
2. WIDGET_ANALYSIS.md documents known bugs without fixing them
The analysis identifies 2 "misclassified" widgets (BadgeButton, Fieldset) that DESCRIBE renders with native-style names instead of PLUGGABLEWIDGET syntax:
This is a DESCRIBE bug — the widget type mapping in
customWidgetTypeShortNamesproduces a native-looking name.
This should be filed as an issue or fixed, not documented as known-broken in a committed analysis file.
3. Hardcoded placeholder URLs in seed data
ImageUrl = 'https://placehold.co/200x200?text=Headphones'
The seed microflow hardcodes placehold.co URLs. These are external dependencies that will break if the service goes down. For demo/test data this is probably acceptable, but worth noting — if these scripts are used in CI, the images will be broken in an air-gapped environment.
4. LanguageSelector datasource looks wrong
PLUGGABLEWIDGET '...languageselector.LanguageSelector' langSel1 (
DataSource: DATABASE FROM WidgetDemo.DemoItem,
languageCaption: '''en_US'''
)
A LanguageSelector backed by DemoItem entities seems semantically wrong — this widget is meant for selecting application languages, not arbitrary entities. This will likely produce a working but nonsensical widget at runtime.
5. Seed microflow uses SHOW PAGE with entity parameter
SHOW PAGE WidgetDemo.WidgetDemo_Showcase(DemoItem: $Existing);
After RETRIEVE ... LIMIT 1, if multiple items exist, only the first is passed. The page's Gallery uses a database datasource anyway (not the parameter), so the DemoItem parameter seems unnecessary for the gallery section. This works but may confuse readers about the page's data flow.
6. File naming convention
01b-snippet.mdl breaks the numeric sequence convention established by 01, 02, 03. Consider 01-domain-model.mdl, 02-snippet.mdl, 03-showcase-page.mdl, 04-seed-and-navigate.mdl — or document why the snippet must be created between domain model and page (if there's an ordering dependency).
Minor observations
- The triple-quoted strings (
'''en_US''','''https://...''') are correct MDL escaping for strings containing single quotes — good GRANT EXECUTE ON MICROFLOWandGRANT VIEW ON PAGEare properly placed after their respective CREATE statements- The showcase page exercises most of the new PLUGGABLEWIDGET syntax features: explicit properties, child widget slots, DataSource, TextTemplate
{Param}binding, mode switching (customVisualization: true/falseon Timeline) WIDGET_ANALYSIS.mdis useful for understanding coverage but reads more like a working document than permanent documentation — consider whether it belongs in the repo or in a PR description
Recommendation
This commit is reasonable on its own — it's focused (example scripts + analysis doc) and well-organized. However, it can't be reviewed or merged independently because it depends on the unreleased PLUGGABLEWIDGET syntax from PR #68, which in turn depends on #67 and #66. The entire 4-PR stack needs to be reviewed as a unit, which at 51K+ lines is not practical.
Suggest: resolve the issues in #66 first, then rebase the stack. Each PR should be independently mergeable.
MDL baseline scripts for reproducible WidgetDemo module generation: - 01-domain-model.mdl: 3 enums, 2 entities, 1 association - 01b-snippet.mdl: ButtonShowcase snippet - 02-showcase-page.mdl: 95-widget showcase (62 native, 33 pluggable) - 03-seed-and-navigate.mdl: seed data microflow + navigation WIDGET_ANALYSIS.md: classification of all 95 widgets by type. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove WIDGET_ANALYSIS.md (working document, not permanent docs) - Renumber files sequentially: 01b→02, 02→03, 03→04 - Remove nonsensical LanguageSelector datasource (DemoItem entity)
bc977ca to
0b0c1c0
Compare
Review ResponseRebased on updated Fixed
Evaluated but not changed
|
AI Code ReviewCritical Issues
What Looks Good
Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
on #68