Skip to content

PPLT-5844: Integrate intelliStory feature into percy-storybook#1343

Open
RaghavsBrowserStack wants to merge 5 commits into
masterfrom
PPLT-5844
Open

PPLT-5844: Integrate intelliStory feature into percy-storybook#1343
RaghavsBrowserStack wants to merge 5 commits into
masterfrom
PPLT-5844

Conversation

@RaghavsBrowserStack

@RaghavsBrowserStack RaghavsBrowserStack commented Jul 16, 2026

Copy link
Copy Markdown

This PR introduces the intelliStory feature to storybook. It doesn't implement any of the business logic itself. The business logic is implemented in CLI under percy/cli#2339. This PR just

  1. Attaches storybook path to snapshots if intelliStory is available
  2. Forwards the config params to applyIntelliStory in CLI.
  3. Writes the result of intelliStory.

@RaghavsBrowserStack RaghavsBrowserStack changed the title Start IntelliStory PPLT-5844: Integrate intelliStory feature into percy-storybook Jul 17, 2026
@RaghavsBrowserStack
RaghavsBrowserStack marked this pull request as ready for review July 17, 2026 08:11
@RaghavsBrowserStack
RaghavsBrowserStack requested a review from a team as a code owner July 17, 2026 08:11
@RaghavsBrowserStack

Copy link
Copy Markdown
Author

Claude Code PR Review

PR: #1343Head: 4e7f9e8Reviewers: stack:percy-cli-code-review

Summary

Adds a storybook.intelliStory config block and wires server-side snapshot selection into takeStorybookSnapshots — stamps importPath onto stories browser-side, creates the Percy build up front, filters snapshots via applyIntelliStory, and summarizes/traces the outcome after finalize.

Note: Per the PR author, CI is intentionally red until a major @percy/cli ships the IntelliStory exports (applyIntelliStory, writeIntelliStoryTrace, IntelliStoryBailError); the API contract is fixed. Failing tests and not-yet-resolvable imports were therefore not treated as defects.

Review Table

Priority Category Check Status Notes
High Security No hardcoded secrets or credentials Pass No PERCY_TOKEN/credentials added
High Security Authentication/authorization checks present N/A No auth surface touched
High Security Input validation and sanitization Pass New config validated via JSON schema
High Security No IDOR — resource ownership validated N/A No resource-ownership surface
High Security No SQL injection (parameterized queries) N/A No SQL
High Correctness Logic is correct, handles edge cases Pass applyIntelliStoryFilter gates on enabled, swallows bail/error, honors failBuildOnFailure; importPath stripped before snapshot
High Correctness Error handling is explicit, no swallowed exceptions Pass trace failure intentionally debug-logged; filter failures logged + fall back
High Correctness No race conditions or concurrency issues Pass Build created up front before uploads; generator ordering preserved
Medium Testing New code has corresponding tests Pass Core units (applyIntelliStoryFilter, processStory, evalStorybookStorySnapshots) covered; orchestration wiring in takeStorybookSnapshots untested (see F1)
Medium Testing Error paths and edge cases tested Pass Bail / generic-error / failBuildOnFailure all covered; post-finalize path not
Medium Testing Existing tests still pass (no regressions) N/A CI intentionally red pending CLI release (author-confirmed)
Medium Performance No N+1 queries or unbounded data fetching Pass
Medium Performance Long-running tasks use background jobs N/A
Medium Quality Follows existing codebase patterns Pass Matches percy generator/yield idioms
Medium Quality Changes are focused (single concern) Pass Scoped to IntelliStory
Low Quality Meaningful names, no dead code Pass
Low Quality Comments explain why, not what Pass Good "why" comments on build ordering / trace swallow
Low Quality No unnecessary dependencies added Pass No new deps; two existing deps re-pinned (see F5)

Findings

  • File: src/storybook.js:44

  • Severity: Medium

  • Reviewer: stack:percy-cli-code-review

  • Issue: buildDir: args.serve is undefined in hosted-URL mode (a http(s):// value routes to args.url, leaving args.serve unset). A user who enables intelliStory against a hosted Storybook silently passes buildDir === undefined into applyIntelliStory/writeIntelliStoryTrace with no warning, degrading selection/trace silently if the CLI relies on it.

  • Suggestion: When intelliStory.enabled and no buildDir resolves, emit a clear log.warn/error, or document an explicit fallback.

  • File: src/snapshots.js:461-463

  • Severity: Low

  • Reviewer: stack:percy-cli-code-review

  • Issue: kept = stats.graphKept + stats.forcedKept / total = kept + stats.skipped prints NaN if intelliStoryStats exists but any field is missing. (Verified the "fails a good build" concern is not reachable — property read + arithmetic + log.info don't throw here — so this is cosmetic, not a build-failure risk.)

  • Suggestion: Default the numeric fields with ?? 0.

  • File: src/snapshots.js:368,374

  • Severity: Low

  • Reviewer: stack:percy-cli-code-review

  • Issue: snapshots = yield applyIntelliStoryFilter(...) is followed by while (snapshots.length); if the CLI's applyIntelliStory ever resolves a non-array, this throws and fails the build.

  • Suggestion: Defensive Array.isArray guard that falls back to the original set.

  • File: test/snapshots.test.js (missing coverage) / src/snapshots.js:329-331,456-474

  • Severity: Medium

  • Reviewer: stack:percy-cli-code-review

  • Issue: The orchestration glue in takeStorybookSnapshots — up-front percy.yield.startBuild() gating, the post-finalize stats summary, and the trace-failure-is-swallowed path — has no direct test. The extracted units are well covered; the wiring is not.

  • Suggestion: Add a spec driving takeStorybookSnapshots with a fake percy.yield/percy.client asserting: startBuild only when enabled & not dry-run, trace failure swallowed to debug, filtered set replaces snapshots. (Would run once the CLI exports land.)

  • File: src/config.js:210-242

  • Severity: Low

  • Reviewer: stack:percy-cli-code-review

  • Issue: intelliStory sets unevaluatedProperties: false while sibling nested objects don't; baseline/untraced/bailOnChanges lack descriptions.

  • Suggestion: Align schema style with siblings; add field descriptions.

  • File: package.json:90-91Severity: Low

  • Reviewer: stack:percy-cli-code-review

  • Issue: Caret dropped — @percy/cli-command and @percy/config now exact-pinned to 1.32.4, freezing patch updates for these two while other deps keep caret ranges.

  • Suggestion: Intentional (locks to the IntelliStory-shipping version); confirm this is desired long-term or restore a caret once the contract is public.

  • File: (diff) • Severity: Low

  • Reviewer: stack:percy-cli-code-review

  • Issue: New user-facing config (storybook.intelliStory.*) has no changelog/docs entry.

  • Suggestion: Add a changelog line and a docs entry before the user-facing release.

Verified non-issues

  • importPath never reaches percy.snapshot — merged for filtering, then destructured out in processStory (test-confirmed).
  • importPath/diagnostics correctly gated on intelliStory and absent when disabled; __STORYBOOK_STORY_STORE__.raw fallback preserves prior behavior.
  • .__intelliStoryDiagnostics on the stories array is read in-browser before serialization, so it survives the page.eval boundary.
  • yielding (not yield*) the promise-returning helpers is correct for the percy generator runner; failBuildOnFailure propagates correctly.
  • No secrets, no eval/Function, no console.log, no new HTTP in this package.

Verdict: PASS — no Critical/High issues; two Mediums (silent buildDir in hosted-URL mode, orchestration test gap) worth addressing before the user-facing release.

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