Skip to content

Add demo bisect seed history - #161

Draft
rameziophobia wants to merge 8 commits into
ramez/canonical-bisect-statefrom
ramez/demo-bisect-seed-history
Draft

Add demo bisect seed history#161
rameziophobia wants to merge 8 commits into
ramez/canonical-bisect-statefrom
ramez/demo-bisect-seed-history

Conversation

@rameziophobia

Copy link
Copy Markdown
Contributor

Summary

  • add a documented commit history for exercising compare-bisect against the demo storefront
  • introduce homepage visual-regression and performance-regression checkpoints as separate commits
  • document clean checkpoints, route coverage, and fixture conventions

Stack

This PR is stacked on #160 so its review shows only the eight demo seed-history commits. Once #160 merges, this PR can be rebased or retargeted to main.

Review note

The granular commit sequence is part of the feature: compare-bisect needs distinct historical checkpoints to locate the seeded regressions. The final net diff is two files.

@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review

Verified the actual net diff (2 files, +42/-1) matches the PR description. Both changes look intentional and internally consistent with the stated bisect-fixture goal, but one maintainability concern stands out:

runMerchandisingWarmup reads as production code, not a fixture (demo-ecommerce/app/javascript/components/pages/HomePage.tsx)

The codebase already has a convention for this exact situation — ExperimentA11yRegressions.tsx names itself unambiguously and carries a doc comment: "Intentional experiment-only accessibility regressions used to exercise the compare report's 'new in experiment' UI." It's also visually marked (data-cy="experiment-a11y-regressions", dashed red border).

The new runMerchandisingWarmup() function has none of that: the name implies real business logic ("merchandising warmup"), there's no comment flagging it as a synthetic perf-regression fixture for git-bisect, and it produces no visible/DOM marker. A future contributor skimming HomePage.tsx (without having read docs/git-bisect-seed-history.md) could easily mistake this 450ms main-thread busy-loop for real logic worth "fixing" (which would defeat the bisect fixture) or worth optimizing/removing without realizing it's load-bearing for a specific historical checkpoint.

Suggest renaming to something self-evidently synthetic (e.g. simulateHomepagePerfRegression) and adding a one-line comment pointing at docs/git-bisect-seed-history.md, matching the ExperimentA11yRegressions precedent.

Everything else — the hero gradient color swap (visual-regression fixture) and the new docs file — is low-risk and consistent with the described bisect-seed-history design.

import LazySection from '../shared/LazySection';
import ExperimentA11yRegressions from '../shared/ExperimentA11yRegressions';

const runMerchandisingWarmup = () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a deliberate 450ms main-thread busy-loop planted as a git-bisect perf-regression fixture (per docs/git-bisect-seed-history.md), but nothing here signals that. The name reads as real merchandising logic, and there's no comment or marker (unlike ExperimentA11yRegressions.tsx, which is clearly named/commented/tagged as an intentional fixture).

Recommend renaming to something self-evidently synthetic (e.g. simulateHomepagePerfRegression) and adding a comment pointing at the bisect-seed-history doc, so a future reader doesn't mistake this for a real bug or delete it without realizing it's load-bearing for a specific checkpoint.

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.

1 participant