Premium Analytics: Expand AGENTS.md with widget Storybook guidance and Overview clarifications - #49679
Conversation
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
| 4. **Title**: `Packages/Premium Analytics/Widgets Toolkit/Widgets/<WidgetName>`. This matches | ||
| the existing Storybook sidebar hierarchy — do not put new widgets at a different top-level | ||
| path. |
There was a problem hiding this comment.
I know this is an existing path, but in my PR, I'm placing it on the Packages/Premium Analytics/Widgets/<WidgetName> path because Widgets Toolkit is temporary and the items on the current path haven't been migrated yet.
| 4. **Title**: `Packages/Premium Analytics/Widgets Toolkit/Widgets/<WidgetName>`. This matches | |
| the existing Storybook sidebar hierarchy — do not put new widgets at a different top-level | |
| path. | |
| 4. **Title**: `Packages/Premium Analytics/Widgets/<WidgetName>`. |
There was a problem hiding this comment.
Thanks, I wasn't aware those were due to be migrated. Cursor updated the guidance based on your PR.
4124b3f to
70dea65
Compare
|
Pausing this until #49505 lands, as it now has conflicts and I'm unsure whether the stories in it are the final shape. |
…cations to AGENTS.md Add a new ## Widgets section anchored to the widget layout introduced in #49505 (Average items per order): documents the required widgets/<name>/{package.json,widget.json,widget.ts,render.tsx,stories/} folder structure, the WidgetDashboardWithWidget-based story template, the Packages/Premium Analytics/Widgets/<WidgetName> title path, registerReportMocks() / autodocs requirements, the projects/js-packages/storybook/storybook/projects.js registration step, and widget-specific pitfalls (including avoiding the legacy packages/widgets-toolkit/src/widgets/* path and the legacy withWidgetRoot() decorator). Also restructure the existing ## Overview into a bulleted list with more detail on each consolidated surface (Odyssey dashboard / apps/odyssey-stats / email stats, and a link to the private woocommerce/woocommerce-analytics repo). Co-authored-by: Jasper Kang <kangzjnet@gmail.com>
Rework the AGENTS.md story guidance so an agent can build all three required stories from the doc alone, without copying a specific widget's story file (which can drift). Inline snippets for each story — Default and WithComparison close-ups plus the WidgetDashboardWithWidget harness story — share one setup block. Reconcile the surrounding requirements: the dashboard story proves the in-product render while the close-ups use a plain canvas decorator, and the legacy note no longer points at an example widget as the thing to copy. Also corrects the WidgetRenderProps import, which pointed at a non-existent @automattic/jetpack-widget-primitives package; the real type comes from @wordpress/widget-primitives. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2vY2GxKEJurnaod7phrFo
a9931bc to
926c669
Compare
nerrad
left a comment
There was a problem hiding this comment.
A few additional comments on things that stand out to me. Pre-approving because I don't think you need to get an additional review, it can be iterated on as needed.
| New widgets live at the top of the package in `widgets/<widget-name>/` and are composed from | ||
| primitives in `packages/widgets-toolkit/`. Each widget is its own pnpm workspace package so | ||
| its render bundle can be lazy-loaded by the dashboard at runtime. |
There was a problem hiding this comment.
What about @automattic/charts? Should that be referenced here? I think we're generally moving toward not needing packages/widgets-toolkit and this is temporary while in development. So it's at risk of getting out of date?
There was a problem hiding this comment.
Good point — updated: the opening paragraph now notes the widgets-toolkit primitives are built on @automattic/charts, plus a caveat that the toolkit is an interim layer (expected to fold into @automattic/charts) so its module paths are provisional rather than long-term API. e8d15fd
| > reference implementation is the **Average items per order** widget (added in | ||
| > [#49505](https://github.com/Automattic/jetpack/pull/49505)). |
There was a problem hiding this comment.
I don't think we should explicitly reference this PR because it could get stale? Maybe just leave it out, keep the rest of the paragraph. The skill can always be added to in the future if needed.
There was a problem hiding this comment.
Done — dropped the explicit PR link from the legacy note and kept the rest of the paragraph. e8d15fd
| - `name` in both `widget.json` and `widget.ts` MUST use the `jpa/` prefix | ||
| (e.g. `jpa/<widget-name>`). |
There was a problem hiding this comment.
Are we all settled on this prefix (writing this knowing that we've already been creating PRs with this prefix 😄 )? I think an agent just picked it as the prefix/namespace. I'm wondering if we should be more explicit... (i.e. premium-analytics/*)
There was a problem hiding this comment.
The merged widgets already standardized on jpa/ (jpa/hello-world, jpa/locations, jpa/top-posts on trunk), so the doc reflects what's shipped. Moving to something like premium-analytics/* would mean renaming those, so I've left jpa/ here to keep the guidance matching reality — happy to track a rename as a separate decision if we want one.
| ### Widget pitfalls | ||
|
|
||
| - Putting new widgets under `packages/widgets-toolkit/src/widgets/*` — that path is for the | ||
| legacy widgets that haven't been migrated yet. | ||
| - Using the legacy `withWidgetRoot()` decorator for new stories — new widgets render via the | ||
| real `WidgetDashboard` through the shared story helper instead. | ||
| - Putting the story at the wrong title path (e.g. `Packages/Premium Analytics/Widgets | ||
| Toolkit/Widgets/<Name>` — that's the legacy path). | ||
| - Omitting `registerReportMocks()` at the top of the story file when the widget fetches | ||
| report data — the widget will render an error state. | ||
| - Forgetting to register `projects/packages/premium-analytics/widgets` in | ||
| `projects/js-packages/storybook/storybook/projects.js` — the story won't appear in | ||
| Storybook at all. | ||
| - Omitting `tags: [ 'autodocs' ]`, which silently drops the widget from the autogenerated | ||
| docs page. | ||
| - Reimplementing data fetching or chart wiring inside `render.tsx` instead of composing | ||
| primitives from `@jetpack-premium-analytics/widgets-toolkit`. |
There was a problem hiding this comment.
How much of this is needed? Is it not already clear enough in the above content?
There was a problem hiding this comment.
Agreed — trimmed to the two non-obvious legacy traps (the widgets-toolkit/src/widgets/* path and the withWidgetRoot() decorator). The other bullets just restated the REQUIRED folder-structure / story rules above, so they're gone. e8d15fd
From nerrad's review (#4524224327): - Note widgets-toolkit primitives are built on @automattic/charts, and flag the toolkit as an interim layer whose module paths are provisional. - Drop the explicit #49505 link from the legacy note so it can't go stale. - Trim the widget pitfalls to the two non-obvious legacy traps; the rest just restated the REQUIRED folder-structure / story rules above. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2vY2GxKEJurnaod7phrFo
Fixes WOOA7S-1555
Proposed changes
Two related updates to the package's
AGENTS.md(introduced in #49678):1. New
## WidgetssectionRequires every widget to ship with a Storybook story, anchored to the new widget layout introduced in #49505 (Average items per order), not the legacy
packages/widgets-toolkit/src/widgets/*widgets which are scheduled to be migrated. Specifically:widgets/<name>/{package.json, widget.json, widget.ts, render.tsx, stories/<name>-widget.stories.tsx}— and thejpa/<name>naming convention shared bywidget.jsonandwidget.ts.render.tsxshould stay thin: composeWidgetRoot+ a toolkit widget primitive rather than reimplement data fetching, chart wiring, or theming.Defaultand aWithComparisonclose-up, plus aWidgetDashboardWithWidgetstory that renders through the sharedWidgetDashboardWithWidgethelper fromwidgets/stories/widget-dashboard-with-widget.tsx(mounting the realWidgetDashboardso it inherits the size / edit-mode / host-environment controls). CallregisterReportMocks()at module level for data widgets, use thePackages/Premium Analytics/Widgets/<WidgetName>title path, and tag the stories[ 'autodocs' ].projects/packages/premium-analytics/widgetstoprojects/js-packages/storybook/storybook/projects.jsso per-widget folders get auto-discovered.packages/widgets-toolkit/src/widgets/*path" and "don't use the legacywithWidgetRoot()decorator for new stories").2. Overview clarifications (from review)
Per code-review suggestions from @kangzj, the
## Overviewsection now spells out each consolidated surface as a bullet rather than an inline parenthetical:stats-adminbackend and theapps/odyssey-statsCalypso frontend, and explicitly mentions email stats alongside traffic / posts / subscribers / WordAds.woocommerce/woocommerce-analyticsrepo so it's discoverable.Scoped narrowly per the issue's "start light, address missing stories rather than building a broader AI compatibility layer up front" framing.
Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
This PR only updates documentation (
AGENTS.md) and a changelog entry. No runtime code changes.## Widgetssection inprojects/packages/premium-analytics/AGENTS.md.render.tsx,widget.ts, andwidget.jsonagainst the Average items per order widget in Premium Analytics: port the Average items per order widget #49505 (projects/packages/premium-analytics/widgets/average-items-per-order/*) and confirm they line up. The story template is self-contained by design (it trims that widget's source-transform / preset boilerplate), so it won't match its story file line-for-line.## Overviewbullets and confirm they match how Stats and Woo Analytics are described elsewhere in the project.Linear Issue: WOOA7S-1555