Skip to content

ref(snapshots): Make the snapshots toolbar presentational and de-duplicate#116477

Open
mtopo27 wants to merge 1 commit into
masterfrom
max/snapshots-toolbar-presentational
Open

ref(snapshots): Make the snapshots toolbar presentational and de-duplicate#116477
mtopo27 wants to merge 1 commit into
masterfrom
max/snapshots-toolbar-presentational

Conversation

@mtopo27
Copy link
Copy Markdown
Contributor

@mtopo27 mtopo27 commented May 29, 2026

First of a 3-PR stack splitting #115893. PR 1 of 3 — base: `master`.

Refactors the preprod snapshots toolbar so it can be rendered in isolation, without touching production behavior.

What changes

  • Extracts the toolbar components into a dedicated `snapshotsToolbar.tsx` module and makes them purely presentational.
  • `DiffModeToggle` now takes a `showSplit` prop instead of calling `useBreakpoints()` itself. The view-mode / diff-mode toggles no longer call `useOrganization()` / `trackAnalytics` — that wiring moves up into `SnapshotMainContent`, which passes `breakpoints.sm` and fires analytics from its `onChange` handlers.
  • `snapshotMainContent.tsx` imports the shared components instead of defining its own copies, removing the duplication.
  • Drops the now-unused `export` on `TRANSPARENT_COLOR` in `diffImageDisplay.tsx` (still used internally; no external importer remains after the de-dupe).

Behavior

No production behavior change intended. The old `DiffModeToggle` did `const breakpoints = useBreakpoints(); breakpoints.sm ? …`, which is exactly what `showSplit={breakpoints.sm}` now passes.

Follow-ups in the stack

  • PR 2: snapshot-harness plumbing to render real design-system components under SSR.
  • PR 3: the actual toolbar snapshot tests.

Extract the preprod snapshots toolbar into a dedicated snapshotsToolbar.tsx
module and make the components purely presentational. DiffModeToggle now
takes a showSplit prop instead of calling useBreakpoints(), and the toggles
no longer call useOrganization()/trackAnalytics — that wiring moves up into
SnapshotMainContent, which passes breakpoints.sm and fires analytics from
its onChange handlers. snapshotMainContent.tsx imports the shared components
instead of defining its own copies, eliminating the duplication.

Also drop the now-unused export on TRANSPARENT_COLOR in diffImageDisplay.tsx
(still used internally, no longer imported elsewhere after the de-dupe).

const handleViewModeChange = useCallback(
(mode: ViewMode) => {
onViewModeChange(mode);
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.

Bug: The handleOpenSnapshot function calls onViewModeChange directly, bypassing the handleViewModeChange wrapper and skipping analytics tracking when a snapshot is opened from the list view.
Severity: MEDIUM

Suggested Fix

In handleOpenSnapshot, replace the direct call to onViewModeChange('single') with a call to the wrapper function handleViewModeChange('single'). This will ensure the analytics event is correctly tracked when a user opens a snapshot.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: static/app/views/preprod/snapshots/main/snapshotMainContent.tsx#L160

Potential issue: When a user opens a snapshot from the list view, the
`handleOpenSnapshot` callback is triggered. This function directly calls the
`onViewModeChange('single')` prop instead of the new `handleViewModeChange` wrapper. As
a result, the `trackAnalytics('preprod.snapshots.details.view_mode_changed', ...)`
event, which is located inside `handleViewModeChange`, is not fired. This leads to a
loss of analytics data for a primary user interaction, contradicting the PR's stated
goal of centralizing analytics wiring within the `SnapshotMainContent` component.

Did we get this right? 👍 / 👎 to inform future reviews.

@github-actions
Copy link
Copy Markdown
Contributor

📊 Type Coverage Diff

Metric Before After Delta
Coverage 93.59% 93.59% ±0%
Typed 133,253 133,254 🟢 +1
Untyped 9,128 9,128 ±0
🔍 1 new type safety issue introduced

Type assertions (as) (1 new)

File Line Detail
static/app/views/preprod/snapshots/main/snapshotsToolbar.tsx 163 as Nodee.target as Node

This is informational only and does not block the PR.

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

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants