Agentic Review: Baseline comparison#34926
Conversation
ghengeveld
force-pushed
the
review-baseline-comparison
branch
from
4be9f90 to
ad35795
Compare
📝 WalkthroughWalkthroughThis PR adds baseline-vs-latest preview comparison to the Review addon: an experimental devServer preset that proxies baseline requests via /__review-baseline, a refactored DetailsScreen with dual iframes, synchronized scrolling and 1-up/2-up preview modes, and story updates targeting the GuidePage. ChangesBaseline-vs-Latest Review Comparison
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 52 KB | 201 KB | 🚨 +149 KB 🚨 |
| Dependency size | 654 B | 654 B | 0 B |
| Bundle Size Analyzer | Link | Link |
|
Actionable comments posted: 0 |
1 similar comment
|
Actionable comments posted: 0 |
…screen with 1up/2up modes, view toggles, and readiness gating so latest stays visible while baseline loads. Also proxy baseline previews through a local __review-baseline dev-server route and keep the two iframes aligned by syncing URL and scroll position.
ghengeveld
force-pushed
the
review-baseline-comparison
branch
from
8f1fff5 to
b677c0a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/addons/review/src/preset.ts`:
- Around line 79-87: The proxy created in createProxyMiddleware (assigned to
proxyRequest) lacks timeout and deterministic error handling; update the options
passed to createProxyMiddleware (for the instance referenced as proxyRequest and
mounted via app.use(BASELINE_PROXY_PATH)) to include timeout and proxyTimeout
values (e.g., sensible millisecond limits), and add an on.error handler that
sends a deterministic HTTP failure (e.g., 502/504 with a short JSON/plain text
body) and ends the response to avoid hanging the request when
BASELINE_TARGET_ORIGIN is slow/unreachable; keep existing changeOrigin and
pathRewrite behavior and ensure the handler checks res.headersSent before
writing a response.
🪄 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: 2c695d1e-0cc1-4f59-a8c6-bba28ad492a6
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
code/addons/review/package.jsoncode/addons/review/src/preset.tscode/addons/review/src/screens/DetailsScreen.stories.tsxcode/addons/review/src/screens/DetailsScreen.tsxcode/core/src/manager/settings/GuidePage.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- code/core/src/manager/settings/GuidePage.tsx
- code/addons/review/src/screens/DetailsScreen.stories.tsx
- code/addons/review/src/screens/DetailsScreen.tsx
| const proxyRequest = createProxyMiddleware({ | ||
| target: BASELINE_TARGET_ORIGIN, | ||
| changeOrigin: true, | ||
| pathRewrite: (path) => | ||
| path.startsWith(BASELINE_PROXY_PATH) ? path.slice(BASELINE_PROXY_PATH.length) || '/' : path, | ||
| }) as unknown as Middleware; | ||
|
|
||
| app.use(BASELINE_PROXY_PATH, proxyRequest); | ||
| return app; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For http-proxy-middleware v4.0.0, what are the exact supported options for request timeout and error handling (e.g., timeout, proxyTimeout, and error hooks like onErrororon.error)?
💡 Result:
In http-proxy-middleware v4.0.0, configuration options for timeouts and error handling are explicitly defined to manage proxy behavior [1][2]. Timeout Options: These options accept a value in milliseconds [1][3]. timeout: Sets the timeout for incoming requests [1][2]. proxyTimeout: Sets the timeout for the proxy when it receives no response from the target server [1][2]. Error Handling: In v4.0.0, default plugins are used, and users are advised to register their own error handlers to prevent the server from crashing [1][2]. The library provides specific plugins for this purpose, such as debugProxyErrorsPlugin and errorResponsePlugin [1][2]. The primary mechanism for custom error handling is via the 'on' option, which subscribes to httpxy events [2][4]. on.error: This function allows you to subscribe to the underlying httpxy 'error' event for custom error handling [1][2]. It typically follows the signature (err, req, res, target) [4]. To implement custom error handling and prevent crashes, you can eject default plugins and manually add error-handling plugins: import { debugProxyErrorsPlugin, errorResponsePlugin, proxyEventsPlugin, } from 'http-proxy-middleware'; // Example of subscribing to error events createProxyMiddleware({ target: 'http://www.example.org', on: { error: (err, req, res) => { // Custom error handling logic }, }, }); The proxyEventsPlugin is specifically used to implement the 'on:' option for event subscriptions [1][2].
Citations:
- 1: https://registry.npmjs.org/http-proxy-middleware
- 2: https://github.com/chimurai/http-proxy-middleware
- 3: https://github.com/chimurai/http-proxy-middleware/blob/master/README.md?plain=1
- 4: https://github.com/chimurai/http-proxy-middleware/blob/master/README.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the proxy setup code
sed -n '60,105p' code/addons/review/src/preset.ts
# Determine which http-proxy-middleware version this repo uses
# (try root package.json first; fall back to searching for the dependency)
if [ -f package.json ]; then
rg -n "\"http-proxy-middleware\"" package.json || true
cat package.json | head -n 80
fi
# Search for any package.json containing the dependency
fd -a 'package.json' -t f . | while read -r f; do
if rg -q "\"http-proxy-middleware\"" "$f"; then
echo "---- $f ----"
rg -n "\"http-proxy-middleware\"" "$f"
fi
doneRepository: storybookjs/storybook
Length of output: 4399
🌐 Web query:
http-proxy-middleware v4 options timeout proxyTimeout on.error errorResponsePlugin error hooks
💡 Result:
In http-proxy-middleware v4, which migrated from the http-proxy library to httpxy [1][2], the configuration options for timeouts and error handling remain central to the middleware's functionality [3][4][5]. Timeout Configuration: - option.timeout: Specifies the timeout (in milliseconds) for incoming requests [3][4][5]. - option.proxyTimeout: Specifies the timeout (in milliseconds) for the proxy when it receives no response from the target server [3][4][5]. Error Handling and Plugins: - option.on.error: This option allows you to subscribe to the underlying proxy's error event for custom error handling [3][4][5]. The callback signature generally follows: function(err, req, res, target) [4]. - errorResponsePlugin: In v3 and v4, the middleware features a plugin system [6][7]. The errorResponsePlugin is a built-in plugin that provides default error response behavior [8]. If you implement custom error handling, ensure you register your own handlers to prevent the server from crashing [8]. - Other Plugins: The middleware includes other plugins such as debugProxyErrorsPlugin, loggerPlugin, and proxyEventsPlugin, which can be configured or ejected to suit specific needs [8][7]. Important v4 Changes: - ESM-only: v4 is an ESM-only package and no longer supports CommonJS require [1][2]. - Runtime: The minimum supported Node.js version is now 22.15.0 [1][2]. - Dependencies: The core proxy engine has been switched to httpxy, which brings updated behavior and fixes [1][2]. For further customization, such as manipulating proxy responses, developers typically use the selfHandleResponse: true option, which allows the developer to take full responsibility for returning the response by listening to the proxyRes event [3][4].
Citations:
- 1: https://github.com/chimurai/http-proxy-middleware/releases/tag/v4.0.0
- 2: https://newreleases.io/project/npm/http-proxy-middleware/release/4.0.0
- 3: https://registry.npmjs.org/http-proxy-middleware
- 4: https://github.com/chimurai/http-proxy-middleware/blob/master/README.md
- 5: https://github.com/chimurai/http-proxy-middleware
- 6: https://github.com/chimurai/http-proxy-middleware/blob/master/CHANGELOG.md
- 7: v3.0.0 chimurai/http-proxy-middleware#768
- 8: https://www.jsdocs.io/package/http-proxy-middleware
🌐 Web query:
http-proxy-middleware v4.0.0 createProxyMiddleware options list on error event signature
💡 Result:
In http-proxy-middleware v4.0.0, the recommended way to handle proxy events, including errors, is by using the on configuration object within the createProxyMiddleware options [1][2]. The signature for the error event handler in v4.0.0 is: (err, req, res) => { /* handle error */ } You configure this via the on property: import { createProxyMiddleware } from 'http-proxy-middleware'; const proxy = createProxyMiddleware({ target: 'http://www.example.org', on: { error: (err, req, res) => { // Your custom error handling logic here console.error(err); res.writeHead(500, { 'Content-Type': 'text/plain' }); res.end('Something went wrong.'); }, }, }); Key changes in v4.0.0: - Proxy logic has been migrated from the legacy http-proxy dependency to httpxy [3][4]. - While the previous versions allowed defining event handlers directly as options (e.g., options.onError), the modern and preferred approach is to group all event handlers within the on object [1][2]. - The package is now ESM-only and requires Node.js 22.15.0 or later [3][4].
Citations:
- 1: https://registry.npmjs.org/http-proxy-middleware
- 2: https://github.com/chimurai/http-proxy-middleware/blob/master/README.md
- 3: https://newreleases.io/project/github/chimurai/http-proxy-middleware/release/v4.0.0
- 4: https://github.com/chimurai/http-proxy-middleware/releases
Add explicit proxy timeouts and deterministic error handling (http-proxy-middleware v4)
createProxyMiddleware in code/addons/review/src/preset.ts sets only target/changeOrigin/pathRewrite; http-proxy-middleware@^4 supports timeout (incoming request), proxyTimeout (no response from target), and on: { error: (err, req, res) => ... }—add these to avoid slow/unreachable Chromatic stalling the review UI and to return a deterministic failure response.
🤖 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/addons/review/src/preset.ts` around lines 79 - 87, The proxy created in
createProxyMiddleware (assigned to proxyRequest) lacks timeout and deterministic
error handling; update the options passed to createProxyMiddleware (for the
instance referenced as proxyRequest and mounted via
app.use(BASELINE_PROXY_PATH)) to include timeout and proxyTimeout values (e.g.,
sensible millisecond limits), and add an on.error handler that sends a
deterministic HTTP failure (e.g., 502/504 with a short JSON/plain text body) and
ends the response to avoid hanging the request when BASELINE_TARGET_ORIGIN is
slow/unreachable; keep existing changeOrigin and pathRewrite behavior and ensure
the handler checks res.headersSent before writing a response.
1 participant
Closes #
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake 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.🦋 Canary release
This pull request has been released as version
0.0.0-pr-34926-sha-a0498164. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34926-sha-a0498164 sandboxor in an existing project withnpx storybook@0.0.0-pr-34926-sha-a0498164 upgrade.More information
0.0.0-pr-34926-sha-a0498164review-baseline-comparisona04981641779970666)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=34926Summary by CodeRabbit
New Features
Chores