Skip to content

chore: implement chromatic#933

Open
jakehwll wants to merge 18 commits intojakehwll/implement-storybookfrom
jakehwll/implement-chromatic
Open

chore: implement chromatic#933
jakehwll wants to merge 18 commits intojakehwll/implement-storybookfrom
jakehwll/implement-chromatic

Conversation

@jakehwll
Copy link
Copy Markdown

@jakehwll jakehwll commented May 5, 2026

🤖 This PR was modified by Coder Agents on behalf of Jake Howell.

Follow-up to #932. Adds Chromatic to the CI pipeline so we get visual regression testing and snapshot previews on PRs.

  • Added chromatic job to ci.yaml with separate steps for PR builds (review-gated) and mainline builds (auto-accept)
  • Uses TurboSnap (onlyChanged) and zip uploads for faster, leaner builds
  • Skips Renovate/Dependabot branches to avoid unnecessary snapshot churn
  • Added chromatic and storybook:ci scripts to package.json
  • Pinned chromatic dependency via the pnpm catalog

@jakehwll jakehwll marked this pull request as ready for review May 6, 2026 05:54
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1bcc7299a0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/workflows/ci.yaml Outdated
@jakehwll jakehwll requested a review from EhabY May 6, 2026 06:20
Comment thread pnpm-workspace.yaml
"@vscode-elements/react-elements": ^2.4.0
"@vscode/codicons": ^0.0.45
babel-plugin-react-compiler: ^1.0.0
chromatic: ^16.7.0
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: 16.8.0 is already out 😛 (also could be inline in package.json)

Comment thread .github/workflows/ci.yaml
run: ${{ runner.os == 'Linux' && 'xvfb-run -a' || '' }} pnpm test:integration --label "VS Code ${{ matrix.vscode-version }}"
shell: bash

# Reference guide:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like the source but the inner comments feel overly verbose to me, we can remove or adapt this to our use-case and make them a bit slimmer!

Comment thread .github/workflows/ci.yaml
Comment on lines +89 to +91
# 👇 Ensures Chromatic can read your full git history
fetch-depth: 0
# 👇 Tells the checkout which commit hash to reference
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Unnecessary comments IMO

Comment thread .github/workflows/ci.yaml
exitOnceUploaded: true
# This will prevent CI from failing when Chromatic detects visual changes
exitZeroOnChanges: true
# Prevent excessive build runs on minor version changes
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Misleading comment (I'd say remove most of these inline ones)

Comment thread .github/workflows/ci.yaml
# the check to pass. This is desired in PRs, but not in mainline.
- name: Publish to Chromatic (non-mainline)
if: github.ref != 'refs/heads/main' && github.repository_owner == 'coder'
uses: chromaui/action@latest
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should pin this like all other actions for security and reproducibility

Comment thread .github/workflows/ci.yaml
buildScriptName: "storybook:build"
# Run TurboSnap to trace file dependencies to related stories
# and tell chromatic to only take snapshots of relevant stories
onlyChanged: true
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should drop this on mainline because autoAcceptChanges is set to true (so we have a full baseline build)

Comment thread .github/workflows/ci.yaml
Comment on lines +122 to +127
# This is a separate step for mainline only that auto accepts and changes
# instead of holding CI up. Since we squash/merge, this is defensive to
# avoid the same changeset from requiring review once squashed into
# main. Chromatic is supposed to be able to detect that we use squash
# commits, but it's good to be defensive in case, otherwise CI remains
# infinitely "in progress" in mainline unless we re-review each build.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Very verbose imo

Copy link
Copy Markdown
Collaborator

@EhabY EhabY left a comment

Choose a reason for hiding this comment

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

Let's merge this after the storybook one lands and we have no conflicts!

Comment thread eslint.config.mjs
Comment on lines +1 to +2
// For more info, see https://github.com/storybookjs/eslint-plugin-storybook#configuration-flat-config-format
import storybook from "eslint-plugin-storybook";
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This seems to be no longer maintained, maybe use the monorepo one? https://github.com/storybookjs/storybook/tree/next/code/lib/eslint-plugin

Also don't we need to extend the rules?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants