Skip to content

Core: Add ai-review observability - #35300

Merged
yannbf merged 3 commits into
nextfrom
yann/agentic-review-observability
Jul 2, 2026
Merged

yannbf merged 3 commits into
nextfrom
yann/agentic-review-observability

Conversation

@yannbf

@yannbf yannbf commented Jun 26, 2026 •

Copy link
Copy Markdown
Member

Closes #

What I did

Added "pageview"-style observability events for the Storybook Review UI, so we can see how the summary and detail surfaces are used. When a user lands on the review summary overlay or opens a specific reviewed story, the manager emits a channel event that core-server forwards as an ai-review observability event ({ action: 'pageview', page, reviewCreatedAt }).

Follows the existing manager-emits → server-forwards pattern (same shape as share, sidebar-filter, and ai-prompt-nudge):

  • Channel event + payload contract — core/src/shared/review/events.ts: new REVIEW_EVENTS.PAGEVIEW (tab → core-server) plus ReviewPage / ReviewPageviewPayload types, re-exported from shared/review/index.ts.
  • Manager emitter — core/src/manager/components/review/ReviewProvider.tsx: a useEffect fires a pageview when the active review surface changes (summary when the overlay is visible, detail when a reviewed story is open in review mode). Keyed via a ref (summary / detail:<storyId>) so re-renders that don't change the surface or story don't re-fire. The review's createdAt rides along as a correlation id.
  • Server forwarder — core/src/core-server/server-channel/telemetry-channel.ts: listens for REVIEW_EVENTS.PAGEVIEW and emits the ai-review observability event.
  • Event type — core/src/telemetry/types.ts: added ai-review to the EventType union.

These are usage/observability events only — no change to how the Review UI renders or behaves.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

  1. Run a sandbox, e.g. yarn task --task sandbox --start-from auto --template react-vite/default-ts
  2. Set STORYBOOK_TELEMETRY_DEBUG=1 when starting Storybook so observability events are logged to the terminal
  3. Trigger a review via the addon-mcp display-review flow so the review summary appears
  4. Confirm an ai-review event with { action: 'pageview', page: 'summary' } is logged when the summary overlay shows
  5. Open one of the reviewed stories and confirm an ai-review event with { action: 'pageview', page: 'detail' } is logged
  6. Re-render / re-navigate to the same story and confirm no duplicate detail event fires; navigating to a different reviewed story fires a new detail event

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Declare whether manual QA will be needed for this PR during the next release, through qa:needed or qa:skip

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

Summary by CodeRabbit

  • New Features

    • Review pages now send pageview telemetry for summary and detail views.
  • Bug Fixes

    • Improved telemetry tracking so repeated re-renders no longer create duplicate review pageview events.
    • Review pageview events now include the viewed page and review creation time for better reporting.

@yannbf
yannbf requested a review from ghengeveld June 26, 2026 11:06
@yannbf yannbf added bug core ci:normal Run our default set of CI jobs (choose this for most PRs). qa:needed Pull Requests that will need manual QA prior to release. labels Jun 26, 2026

@shilman shilman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good! Did not test

@coderabbitai

coderabbitai Bot commented Jun 26, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a new PAGEVIEW review event with ReviewPage and ReviewPageviewPayload types, extends EventType with 'review', wires a telemetry-channel listener forwarding this event to telemetry, and adds ReviewProvider logic to emit deduplicated pageview events based on surface changes.

Changes

Review pageview telemetry

Layer / File(s) Summary
Review pageview event contract
code/core/src/shared/review/events.ts, code/core/src/shared/review/index.ts, code/core/src/telemetry/types.ts
Adds REVIEW_EVENTS.PAGEVIEW, ReviewPage and ReviewPageviewPayload types, re-exports them, and extends EventType with 'review'.
ReviewProvider pageview emission
code/core/src/manager/components/review/components/ReviewProvider.tsx
Adds a ref to track the last emitted surface key and a useEffect that emits EVENTS.PAGEVIEW when the summary/detail surface changes, resetting on null state.
Telemetry forwarding
code/core/src/core-server/server-channel/telemetry-channel.ts, code/core/src/core-server/server-channel/telemetry-channel.test.ts
Adds a REVIEW_EVENTS.PAGEVIEW channel listener forwarding to telemetry('review', ...) with tests covering summary and detail pageviews.

Estimated code review effort: 2 (Simple) | ~12 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ReviewProvider
  participant Channel
  participant TelemetryChannel
  participant Telemetry

  ReviewProvider->>Channel: emit REVIEW_EVENTS.PAGEVIEW (page, reviewCreatedAt)
  Channel->>TelemetryChannel: REVIEW_EVENTS.PAGEVIEW listener
  TelemetryChannel->>Telemetry: telemetry('review', {action: 'pageview', source: 'mcp-review', page, reviewCreatedAt})
Loading

Possibly related PRs

  • storybookjs/storybook#35223: Both PRs modify code/core/src/shared/review/events.ts, with the retrieved PR establishing REVIEW_EVENTS and this PR adding the PAGEVIEW event and payload typing.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@code/core/src/core-server/server-channel/telemetry-channel.test.ts`:
- Around line 59-63: The telemetry test expectations use the wrong event name,
causing the assertions in initTelemetryChannel to fail. Update the calls in
telemetry-channel.test.ts to match the actual event type forwarded by
initTelemetryChannel, which is the ai-review literal in the EventType union, and
apply the same fix to both pageview assertions so they verify the correct
telemetry arguments.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 524a457d-db90-406f-b6d3-084eaaa6afdb

📥 Commits

Reviewing files that changed from the base of the PR and between 77e6430 and 676e241.

📒 Files selected for processing (6)
  • code/core/src/core-server/server-channel/telemetry-channel.test.ts
  • code/core/src/core-server/server-channel/telemetry-channel.ts
  • code/core/src/manager/components/review/ReviewProvider.tsx
  • code/core/src/shared/review/events.ts
  • code/core/src/shared/review/index.ts
  • code/core/src/telemetry/types.ts

Comment thread code/core/src/core-server/server-channel/telemetry-channel.test.ts
Comment thread code/core/src/telemetry/types.ts Outdated
yannbf and others added 2 commits July 2, 2026 12:29
…bservability

# Conflicts:
#	code/core/src/manager/components/review/components/ReviewProvider.tsx
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@yannbf
yannbf enabled auto-merge July 2, 2026 10:38

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
code/core/src/manager/components/review/components/ReviewProvider.tsx (1)

199-221: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Dedup key ignores review identity, so a new review can be silently missed by telemetry.

key is derived only from the surface ('summary' or detail:${storyId}), not from state.createdAt. If DISPLAY_REVIEW delivers a new review (different createdAt) while the user stays on the same summary overlay or the same story's detail view — which is possible since the handler calls setState(next) directly when isDeferredReviewUpdate/isSameReviewPayload are both false — the surface key is unchanged, so no pageview fires for the new review even though reviewCreatedAt would differ. This undercounts pageviews for the "arrives while viewing" case.

Include state.createdAt in the dedup key so a new review on an unchanged surface still reports a pageview:

🐛 Proposed fix
     if (isSummaryVisible) {
       page = 'summary';
-      key = 'summary';
+      key = `summary:${state.createdAt}`;
     } else if (isInReviewMode && activeEntry) {
       page = 'detail';
-      key = `detail:${activeEntry.storyId}`;
+      key = `detail:${activeEntry.storyId}:${state.createdAt}`;
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@code/core/src/manager/components/review/components/ReviewProvider.tsx` around
lines 199 - 221, The pageview dedup logic in ReviewProvider’s useEffect is only
keyed by the visible surface and story, so a new review with a different
state.createdAt can be skipped when the user remains on the same summary or
detail view. Update the lastPageviewKeyRef comparison to include state.createdAt
in the key used for deduping, while keeping the existing page selection logic
and emit(EVENTS.PAGEVIEW, ...) call in sync with the current review identity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@code/core/src/manager/components/review/components/ReviewProvider.tsx`:
- Around line 199-221: The pageview dedup logic in ReviewProvider’s useEffect is
only keyed by the visible surface and story, so a new review with a different
state.createdAt can be skipped when the user remains on the same summary or
detail view. Update the lastPageviewKeyRef comparison to include state.createdAt
in the key used for deduping, while keeping the existing page selection logic
and emit(EVENTS.PAGEVIEW, ...) call in sync with the current review identity.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8917f68a-71f9-4075-b8a3-b642a88241e1

📥 Commits

Reviewing files that changed from the base of the PR and between 676e241 and 6139d90.

📒 Files selected for processing (5)
  • code/core/src/core-server/server-channel/telemetry-channel.test.ts
  • code/core/src/core-server/server-channel/telemetry-channel.ts
  • code/core/src/manager/components/review/components/ReviewProvider.tsx
  • code/core/src/shared/review/index.ts
  • code/core/src/telemetry/types.ts
✅ Files skipped from review due to trivial changes (1)
  • code/core/src/shared/review/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • code/core/src/telemetry/types.ts
  • code/core/src/core-server/server-channel/telemetry-channel.test.ts

This was referenced Jul 13, 2026
@JReinhold JReinhold added qa:success Pull Requests that were successfully QA'ed by the release team. and removed qa:needed Pull Requests that will need manual QA prior to release. labels Aug 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor
Fails
🚫

node failed.

Log

Details
Error:  Error: Could not find the Dangerfile at scripts/dangerfile.ts - if it is local, perhaps you have a typo? If it's using a remote file, it doesn't have a repo reference.
    at /usr/src/danger/dist/platforms/GitHub.js:161:27
    at step (/usr/src/danger/dist/platforms/GitHub.js:44:23)
    at Object.next (/usr/src/danger/dist/platforms/GitHub.js:25:53)
    at /usr/src/danger/dist/platforms/GitHub.js:19:71
    at new Promise (<anonymous>)
    at __awaiter (/usr/src/danger/dist/platforms/GitHub.js:15:12)
    at Object.executeRuntimeEnvironment (/usr/src/danger/dist/platforms/GitHub.js:144:88)
    at /usr/src/danger/dist/commands/danger-runner.js:101:47
    at step (/usr/src/danger/dist/commands/danger-runner.js:34:23)
    at Object.next (/usr/src/danger/dist/commands/danger-runner.js:15:53)
danger-results://tmp/danger-results-138aab1f.json

Generated by 🚫 dangerJS against 6139d90

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug ci:normal Run our default set of CI jobs (choose this for most PRs). core qa:success Pull Requests that were successfully QA'ed by the release team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants