Added a skill for the hyva commerce dashboard widget - #12
Conversation
Vinai
left a comment
There was a problem hiding this comment.
Critical
- README.md not updated. The repo convention is that every skill appears in a README
table with a one-line description. The PR touches only the new SKILL.md, so the skill
would be undiscoverable via the docs (it still shows up ininstall-hyva-skill.sh --list).
Observations
- PR description mismatch. The PR body says "Updated the hyva-widgets skill", but the
PR adds a new file. Together with thename: hyva-widgetsfrontmatter this suggests the
skill was ported from an internal repo and the rename to the public directory name was
missed in the frontmatter and PR text. getTrailingAction()PHP shape undocumented. The interface documents the return
shape asarray{title?: Phrase, href?: string, target?: string}[verified in source] —
note the keys differ from the XML element (label/route/target). The skill only
shows delegation to$ctx, which is fine, but a widget overriding it has no shape to
follow.- Spelling consistency. The body uses "Hyva" throughout while most repo prose uses
"Hyvä". - Section naming. "Source Of Truth" reads oddly for what is actually "API contract
requirements + doc links"; consider splitting or renaming.
Automated check results
- Cross-AI compatibility: Claude Code OK, Gemini CLI OK, Codex OK (but description at
485/500 chars, see line 3 comment). No AI-specific tools or hardcoded skill paths. - Token efficiency: 1,211 words (24% of the 5,000-word guideline), 11.9 KB (37% of Codex's
32 KB limit). Single-file skill is appropriate at this size; no extraction needed.
| @@ -0,0 +1,348 @@ | |||
| --- | |||
| name: hyva-widgets | |||
There was a problem hiding this comment.
The frontmatter name doesn't match the directory hyva-commerce-dashboard-widgets. All 12
existing skills in this repo have name: matching their directory, and
install-hyva-skill.sh resolves skills by directory name while agents register the skill
under the frontmatter name — so users would install hyva-commerce-dashboard-widgets but
invoke /hyva-widgets. The name hyva-widgets is also misleadingly generic: Magento has a
core storefront "widgets" concept, so this name invites exactly the mis-triggering the body
warns against (lines 12–14). Please rename to hyva-commerce-dashboard-widgets.
| @@ -0,0 +1,348 @@ | |||
| --- | |||
| name: hyva-widgets | |||
| description: Use when creating or modifying a Hyva Commerce Admin Dashboard widget for Magento Admin using the newer Hyva AdminDashboardApi V1 composition contract, including etc/adminhtml/hyva_dashboard_widget.xml registration, WidgetTypeInterface methods with WidgetContextInterface, configurable/display properties, display data, permissions, save hooks, built-in display types, and optional custom templates/scripts. Examples must be generic for app/code/Vendor/Module and not company-specific. | |||
There was a problem hiding this comment.
Three issues:
- "Examples must be generic for app/code/Vendor/Module and not company-specific." is an
instruction to the skill author, already duplicated in the body (lines 16–23). It doesn't
help trigger matching and wastes space in the description. - The description is 485 of the 500 characters Codex allows — any future edit tips it over.
Removing the sentence above solves this too. - Unlike every sibling skill, there is no explicit trigger-phrase list. Suggest following
the repo pattern: "This skill should be used when… Trigger phrases include 'dashboard
widget', 'admin dashboard widget', 'hyva commerce widget', …".
| Useful official docs: | ||
|
|
||
| - PHP implementation: https://docs.hyva.io/hyva-commerce/features/admin-dashboard/devdocs/widget-types/php.html | ||
| - XML configuration: https://docs.hyva.io/hyva-commerce/features/admin-dashboard/devdocs/widget-types/xml.html | ||
| - Configurable inputs: https://docs.hyva.io/hyva-commerce/features/admin-dashboard/devdocs/widget-types/configurable-inputs.html | ||
| - Available widget types: https://docs.hyva.io/hyva-commerce/features/admin-dashboard/devdocs/widget-types/available-types.html |
There was a problem hiding this comment.
All four URLs are dead redirects. Each …/devdocs/widget-types/*.html page returns only a
redirect stub ("You're being redirected…") that many agent fetch tools won't follow. This
matters doubly because the skill defers detail to these links (chart data shapes, input
types). Canonical URLs (verified live):
| Current | Replace with |
|---|---|
…/devdocs/widget-types/php.html |
…/devdocs/widget-php.html |
…/devdocs/widget-types/xml.html |
…/devdocs/widget-xml.html |
…/devdocs/widget-types/configurable-inputs.html |
…/devdocs/configurable-inputs.html |
…/devdocs/widget-types/available-types.html |
…/devdocs/available-widget-types.html |
Additionally, the API package bundles an offline reference the skill could point agents at:
vendor/hyva-themes/commerce-module-admin-dashboard-api/docs/implementing-a-widget.md
— it covers the same contract and works without network access.
| Only support the newer composition API: | ||
|
|
||
| - Implement `Hyva\AdminDashboardApi\Api\V1\WidgetTypeInterface`. | ||
| - Every widget method receives `WidgetContextInterface $ctx` as its first | ||
| argument. | ||
| - Do not extend `AbstractWidgetType` and do not implement older widget | ||
| interfaces. | ||
|
|
||
| This keeps custom widgets dependent on | ||
| `hyva-themes/commerce-module-admin-dashboard-api` rather than the full | ||
| dashboard runtime. |
There was a problem hiding this comment.
The skill never tells the agent to actually declare the dependency: require
hyva-themes/commerce-module-admin-dashboard-api in composer.json and add
Hyva_AdminDashboardApi to etc/module.xml <sequence>. The package and module names are
[verified in source] (composer.json, src/etc/module.xml); the interface docblock
itself states that only the api package is needed for setup:di:compile.
An agent
scaffolding a fresh module will otherwise produce one that doesn't load the XSD or
interfaces. Please add a short module-setup step.
| <widget id="my_widget"> | ||
| <class>Vendor\Module\Model\Widget\MyWidget</class> | ||
| <display_type>text</display_type> | ||
| </widget> | ||
| </config> |
There was a problem hiding this comment.
The minimal example (line 73) and the display_type table row (line 106) use text, but
text is not a mapped display type: the
displayTypeTemplateMap argument in the dashboard runtime's framework/etc/di.xml maps
exactly bar_chart, line_chart, number, pie_chart, table, and
date-interval-table (plus template, which is handled via the widget's own <template>).
No first-party widget registers display_type=text either — the built-in "text" widget
uses display_type=template. Following the minimal example verbatim therefore produces a
widget with no template mapping. Also note the skill's own built-ins list (lines 118–121)
omits number, which IS a built-in. Please use a real built-in type in the minimal example
and state the definitive list once: table, bar_chart, line_chart, pie_chart,
number, date-interval-table, template.
| Read saved values from the widget instance. Use constants from | ||
| `Hyva\AdminDashboardApi\Api\ConfigurationKeys`. |
There was a problem hiding this comment.
"Read saved values from the widget instance. Use constants from
Hyva\AdminDashboardApi\Api\ConfigurationKeys." names a class but no method. The API is:
$widgetInstance->getPropertyValue(ConfigurationKeys::CONFIGURABLE_PROPERTIES, 'limit');
$widgetInstance->getPropertyValues(ConfigurationKeys::DISPLAY_PROPERTIES);
$widgetInstance->getConfiguration();ConfigurationKeys exists with exactly CONFIGURABLE_PROPERTIES = 'configurable_properties'
and DISPLAY_PROPERTIES = 'display_properties'. As written, the agent must guess the single
most common operation a widget needs — please show a one-line example.
| For chart display types, inspect the installed templates for the exact shape | ||
| expected by `bar-chart.phtml`, `line-chart.phtml`, or `pie-chart.phtml`. | ||
| Marker interfaces live under `Hyva\AdminDashboardApi\Api\V1\ChartType\*`. |
There was a problem hiding this comment.
"Marker interfaces live under Hyva\AdminDashboardApi\Api\V1\ChartType\*" undersells them.
Five exist: BarChartWidgetTypeInterface,
LineChartWidgetTypeInterface, PieChartWidgetTypeInterface, NumberWidgetTypeInterface,
DateIntervalWidgetTypeInterface. Per the API package's bundled docs, implementing a
chart-type interface "tells the framework which display type the widget uses and wires the
matching defaults into $ctx" — i.e. they are the intended way to build chart widgets, not
just markers to be aware of. The number display type is missing from the skill entirely.
Also, "inspect the installed templates" only works when Hyvä Commerce is installed — with
the doc links fixed (lines 41–44) that's acceptable, otherwise it's a dead end.
| Use XML `acl` for normal access control. Add `isAllowed()` only for extra | ||
| per-instance rules. | ||
|
|
||
| Use `$ctx->isAllowed($widgetInstance)` as the default implementation, then add | ||
| extra checks only when the widget needs per-instance rules. |
There was a problem hiding this comment.
"Add isAllowed() only for extra per-instance rules" — the interface makes all eight
methods mandatory (and the skill forbids the abstract base on line
32), so it can't be "added" optionally. The next sentence (lines 258–259) then gives the
correct guidance and largely repeats the first. Suggest merging into one statement:
"Implement isAllowed() as a delegation to $ctx->isAllowed($widgetInstance); add extra
checks only when the widget needs per-instance rules. Use XML acl for normal access
control."
| <?php | ||
| use Hyva\AdminDashboardApi\Api\V1\WidgetInstanceInterface; | ||
|
|
||
| /** @var WidgetInstanceInterface|null $widgetInstance */ | ||
| $widgetInstance = $block->getData('widget_instance'); | ||
|
|
||
| if (!$widgetInstance || !($data = $widgetInstance->getDisplayData())) { | ||
| return; | ||
| } | ||
| ?> |
There was a problem hiding this comment.
The template example imports Hyva\AdminDashboardApi\Api\V1\WidgetInstanceInterface and
then calls $widgetInstance->getDisplayData() — but the API interface does not declare
getDisplayData(). That method lives on the runtime's extended
interface: Hyva\AdminDashboardFramework\Api\V1\WidgetInstance\WidgetInstanceInterface,
signature getDisplayData(bool $loadFromCache = true): mixed. The framework's own
widget/table.phtml docblock uses exactly that framework interface. Static analysis would
flag the skill's example, and it type-hints a contract the call doesn't exist on. Fix: use
the framework interface in the template docblock (templates only render when the dashboard
runtime is installed, so the API-only dependency rule isn't violated).
| should not have access. | ||
| - Save hooks always return the widget instance and handle recoverable failures | ||
| without throwing. | ||
| - Run Magento cache flush/setup steps required by the project, then confirm |
There was a problem hiding this comment.
The checklist asks to "Run Magento cache flush/setup steps required by the project" — this
is exactly what the hyva-exec-shell-cmd skill handles. Consider adding
requires: hyva-exec-shell-cmd to the frontmatter, matching sibling skills.
| This skill is for **Hyva Commerce Admin Dashboard widgets**: pluggable cards | ||
| merchants add to the Magento Admin Dashboard, such as KPIs, charts, tables, | ||
| links, and custom template widgets. |
There was a problem hiding this comment.
[Disambiguation] Since Magento Admin Dashboard and Hyvä Commerce Admin Dashboard are two distinct things, I would avoid mentioning the Magento Admin Dashboard; see the suggested change.
| This skill is for **Hyva Commerce Admin Dashboard widgets**: pluggable cards | |
| merchants add to the Magento Admin Dashboard, such as KPIs, charts, tables, | |
| links, and custom template widgets. | |
| This skill is for creating customised widgets for **Hyvä Commerce Admin Dashboard**, such as KPIs, charts, tables, links, and custom template widgets. |
|
Hello @tschallacka, thanks again for your contribution! |
Updated the hyva-widgets skill to target only the newer Hyva Admin Dashboard API composition contract.
Changes: