Replace streaming flaky tests with Playwright E2E tests #1869
Conversation
…he source of the failure
… check the source of the failure" This reverts commit 67b2ffc.
WalkthroughAdds Playwright-based E2E tests and Redis-backed test fixtures for the dummy app, a RedisReceiver streaming component with Rails endpoints/routes/views, Playwright config and snapshots, CI job to run tests, ESLint/TS adjustments, and removal of legacy streaming system tests. Changes
Sequence Diagram(s)sequenceDiagram
participant PW as Playwright Test
participant Browser as Browser
participant Rails as Rails Server
participant Redis as Redis
participant Component as RedisReceiver
PW->>+Rails: GET /redis_receiver?request_id=RID
Rails->>+Redis: create stream:RID (background thread)
Rails-->>-Browser: SSR HTML (fallbacks) + streaming endpoint
Browser->>Component: mount with requestId
Component->>Redis: listenToRequestData(requestId)
loop items 1..5
Redis-->>Component: send ItemN
Component-->>Browser: stream incremental update
PW->>Browser: capture snapshot stage N
end
PW->>Redis: cleanup stream:RID
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (1)
Comment |
Pull Request Review: Fix streaming flaky testsThank you for working on fixing the flaky streaming tests! This is a valuable improvement to the test infrastructure. Below is my detailed review: 🎯 SummaryThis PR migrates flaky Capybara-based streaming tests to Playwright with controlled Redis streaming, improving test reliability and coverage. The changes include:
✅ Strengths
🐛 Potential IssuesCritical Issues
Code Quality Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-on-rails-pro/src/RSCRoute.tsx (1)
78-81: Correct the reference directive fromreact/experimentaltoreact.The CI failures stem from an incorrect reference directive path. The correct triple‑slash directive should be
/// <reference types="react" />, notreact/experimental. React.use is defined in @types/react v18.3.18 with proper function overloads, and your installed versions are correct. The root tsconfig already includes"typeRoots": ["./node_modules/@types"], so the directive can be omitted entirely or corrected to:-/// <reference types="react/experimental" /> +/// <reference types="react" />Alternatively, remove the reference directive entirely since your tsconfig automatically resolves @types. Either approach resolves the TypeScript compilation errors.
🧹 Nitpick comments (9)
react_on_rails_pro/spec/dummy/e2e-tests/dummy.spec.ts (2)
1-12: Consider removing commented code.The commented-out test code spanning lines 1-12 should either be removed or uncommented if it's needed. Keeping large blocks of commented code reduces maintainability.
19-21: Remove console.log from test code.The test contains a console.log statement (line 20) that appears to be for debugging. Consider removing it or converting it to a proper assertion if this is production test code. Also, line 21 is missing a semicolon for consistency.
Apply this diff if this is production test code:
test('eee', ({ h2 }) => { - console.log('TEst', h2); -}) + // Add proper assertions here + expect(h2).toBeDefined(); +});Or remove the file entirely if it's just a temporary fixture test.
react_on_rails_pro/spec/dummy/e2e-tests/streaming.spec.ts (1)
16-21: Consider extracting test configuration to improve maintainability.The test configuration array uses
as constand forEach to generate multiple test cases. While this works, consider extracting it to a separate constant for better readability and reusability.+const TEST_CONFIGURATIONS = [ + ['RedisReceiver', redisReceiverPageTest], + ['RedisReceiver inside router page', redisReceiverInsideRouterPageTest], + ['RedisReceiver inside router after navigation', redisReceiverPageAfterNavigationTest], + ['RedisReceiver with Async Toggle Container Client Component', redisReceiverPageWithAsyncClientComponentTest], +] as const; + -([ - ['RedisReceiver', redisReceiverPageTest], - ['RedisReceiver inside router page', redisReceiverInsideRouterPageTest], - ['RedisReceiver inside router after navigation', redisReceiverPageAfterNavigationTest], - ['RedisReceiver with Async Toggle Container Client Component', redisReceiverPageWithAsyncClientComponentTest], -] as const).forEach(([pageName, test]) => { +TEST_CONFIGURATIONS.forEach(([pageName, test]) => {react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb (1)
89-93: Don’t raise after streaming response; return an HTTP status insteadRaising after the stream may cause noisy 500s. Prefer an explicit status.
- Rails.logger.error "Redis thread timed out" - raise "Redis thread timed out" + Rails.logger.error "Redis thread timed out" + head :gateway_timeoutreact_on_rails_pro/spec/dummy/e2e-tests/dummt-fixture.ts (2)
12-16: Fix “Unexpected empty object pattern” in fixturesReplace empty destructuring with an unused arg to satisfy linters.
- h: [async({}, use) => { + h: [async (_args, use) => { console.log('F1') await use('F1'); console.log('F1 end'); }, { auto: true }]- h: [async({}, use) => { + h: [async (_args, use) => { console.log('Fm') await use('Fm'); console.log('Fm end'); }, { auto: true }]Also applies to: 20-24
1-1: Optional: file name typoConsider renaming dummt-fixture.ts to dummy-fixture.ts for clarity.
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx (2)
21-24: Prefer JSX children over a children propPass children via JSX to satisfy linters and React conventions.
-const AsyncToggleContainer = async ({ children, childrenTitle, getValue }) => { +const AsyncToggleContainer = async ({ children, childrenTitle, getValue }) => { await getValue('ToggleContainer'); - return <ToggleContainer children={children} childrenTitle={childrenTitle} /> + return <ToggleContainer childrenTitle={childrenTitle}>{children}</ToggleContainer> }
41-47: Avoid passing unused props to ToggleContainerWhen asyncToggleContainer is false, getValue is unnecessary. Keep prop surface minimal.
- const toggleContainerGetValueParam = asyncToggleContainer ? getValue : undefined; + const toggleContainerGetValueParam = asyncToggleContainer ? getValue : undefined; // consider only spreading when defined ... - <UsedToggleContainer childrenTitle="Redis Items" getValue={toggleContainerGetValueParam}> + <UsedToggleContainer childrenTitle="Redis Items" {...(toggleContainerGetValueParam ? { getValue: toggleContainerGetValueParam } : {})}>react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts (1)
27-27: Fix “Unexpected empty object pattern” in fixturesReplace empty destructuring with an unused arg name.
- redisClient: [async ({}, use, workerInfo) => { + redisClient: [async (_args, use, workerInfo) => {- redisRequestId: async ({}, use) => { + redisRequestId: async (_args, use) => {Also applies to: 36-36
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
react_on_rails_pro/spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockreact_on_rails_pro/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (23)
.circleci/config.yml(2 hunks)packages/react-on-rails-pro/src/RSCRoute.tsx(1 hunks)react_on_rails_pro/spec/dummy/.gitignore(1 hunks)react_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage0.aria.yml(1 hunks)react_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage1.aria.yml(1 hunks)react_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage2.aria.yml(1 hunks)react_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage3.aria.yml(1 hunks)react_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage4.aria.yml(1 hunks)react_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage5.aria.yml(1 hunks)react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb(1 hunks)react_on_rails_pro/spec/dummy/app/views/pages/redis_receiver.html.erb(1 hunks)react_on_rails_pro/spec/dummy/app/views/pages/server_router.html.erb(1 hunks)react_on_rails_pro/spec/dummy/client/app/components/RSCPostsPage/ToggleContainer.jsx(1 hunks)react_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsx(2 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx(1 hunks)react_on_rails_pro/spec/dummy/config/routes.rb(1 hunks)react_on_rails_pro/spec/dummy/e2e-tests/dummt-fixture.ts(1 hunks)react_on_rails_pro/spec/dummy/e2e-tests/dummy.spec.ts(1 hunks)react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts(1 hunks)react_on_rails_pro/spec/dummy/e2e-tests/streaming.spec.ts(1 hunks)react_on_rails_pro/spec/dummy/package.json(2 hunks)react_on_rails_pro/spec/dummy/playwright.config.ts(1 hunks)react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb(0 hunks)
💤 Files with no reviewable changes (1)
- react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
react_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage2.aria.ymlreact_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage5.aria.ymlreact_on_rails_pro/spec/dummy/client/app/components/RSCPostsPage/ToggleContainer.jsxreact_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage1.aria.ymlpackages/react-on-rails-pro/src/RSCRoute.tsxreact_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsxreact_on_rails_pro/spec/dummy/package.jsonreact_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage3.aria.ymlreact_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage4.aria.ymlreact_on_rails_pro/spec/dummy/e2e-tests/dummt-fixture.tsreact_on_rails_pro/spec/dummy/playwright.config.tsreact_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage0.aria.ymlreact_on_rails_pro/spec/dummy/e2e-tests/streaming.spec.tsreact_on_rails_pro/spec/dummy/e2e-tests/dummy.spec.tsreact_on_rails_pro/spec/dummy/e2e-tests/fixture.tsreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/config/routes.rb
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
react_on_rails_pro/spec/dummy/client/app/components/RSCPostsPage/ToggleContainer.jsxpackages/react-on-rails-pro/src/RSCRoute.tsxreact_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsxreact_on_rails_pro/spec/dummy/e2e-tests/dummt-fixture.tsreact_on_rails_pro/spec/dummy/playwright.config.tsreact_on_rails_pro/spec/dummy/e2e-tests/streaming.spec.tsreact_on_rails_pro/spec/dummy/e2e-tests/dummy.spec.tsreact_on_rails_pro/spec/dummy/e2e-tests/fixture.tsreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx
🧬 Code graph analysis (5)
react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb (1)
react_on_rails_pro/lib/react_on_rails_pro/concerns/stream.rb (1)
stream_view_containing_react_components(33-47)
react_on_rails_pro/spec/dummy/playwright.config.ts (1)
react_on_rails_pro/spec/dummy/client/node-renderer.js (1)
process(5-5)
react_on_rails_pro/spec/dummy/e2e-tests/dummy.spec.ts (1)
react_on_rails_pro/spec/dummy/e2e-tests/dummt-fixture.ts (1)
test2(27-33)
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts (1)
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx (1)
value(7-7)
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx (2)
react_on_rails_pro/spec/dummy/client/app/components/RSCPostsPage/ToggleContainer.jsx (1)
ToggleContainer(3-19)react_on_rails_pro/spec/dummy/client/app/components/ErrorBoundary.tsx (1)
ErrorBoundary(7-9)
🪛 Biome (2.1.2)
react_on_rails_pro/spec/dummy/e2e-tests/dummt-fixture.ts
[error] 12-12: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 20-20: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts
[error] 27-27: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 36-36: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx
[error] 23-23: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
🪛 GitHub Actions: JS unit tests for Renderer package
packages/react-on-rails-pro/src/RSCRoute.tsx
[error] 80-80: TS2339: Property 'use' does not exist on type 'typeof React'.
🪛 GitHub Actions: Lint JS and Ruby
packages/react-on-rails-pro/src/RSCRoute.tsx
[error] 80-80: TypeScript error: Property 'use' does not exist on type 'typeof React'.
🪛 GitHub Actions: Main test
packages/react-on-rails-pro/src/RSCRoute.tsx
[error] 80-80: TS2339: Property 'use' does not exist on type 'typeof React'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: build-and-test
🔇 Additional comments (14)
react_on_rails_pro/spec/dummy/.gitignore (1)
3-9: LGTM! Playwright test artifacts properly ignored.The Playwright-related ignore patterns are standard and complete, covering test results, reports, cache, and authentication state files. This aligns well with the PR's introduction of Playwright-based end-to-end testing infrastructure.
react_on_rails_pro/spec/dummy/client/app/components/RSCPostsPage/ToggleContainer.jsx (1)
13-13: LGTM! Test selector added.The
classNameaddition provides a stable selector for e2e tests and aligns with the PR's test infrastructure improvements.react_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage0.aria.yml (1)
1-9: ARIA snapshot test fixture approved.This initial stage snapshot properly captures the loading state for streaming ARIA tests.
react_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage1.aria.yml (1)
1-9: ARIA snapshot shows correct incremental progression.Stage1 properly captures the first item received while others remain pending.
react_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage2.aria.yml (1)
1-9: ARIA snapshot progression is realistic.Stage2 correctly captures non-sequential item arrival, simulating real streaming scenarios.
react_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage3.aria.yml (1)
1-9: ARIA snapshot consistent with progression.Stage3 properly advances item delivery while maintaining accessibility structure.
react_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage4.aria.yml (1)
1-9: ARIA snapshot validates near-complete streaming state.Stage4 correctly represents most items received with final item pending.
react_on_rails_pro/spec/dummy/__snapshots__/streaming.spec.ts/stage5.aria.yml (1)
1-9: ARIA snapshot captures complete streaming terminal state.Stage5 correctly represents all items successfully received from Redis.
.circleci/config.yml (2)
350-350: Verify Redis service image version.Redis 6.2.6 was released in 2021. Consider whether you intentionally target 6.2 for compatibility testing or if upgrading to Redis 7.x or 8.x is appropriate. Verify this aligns with your production environment expectations.
423-426: Workflow integration looks correct.The e2e test job is properly added to the workflow with correct dependency ordering.
react_on_rails_pro/spec/dummy/package.json (1)
100-100: Script addition properly supports e2e test workflow.The "e2e-test" script command correctly maps to
playwright test, enabling the CircleCI job to execute end-to-end tests.react_on_rails_pro/spec/dummy/e2e-tests/streaming.spec.ts (1)
41-69: Verify early hydration test behavior.The early hydration test (lines 41-69) performs UI interactions before completing incremental rendering. Ensure this accurately reflects the intended user behavior and that the
waitForConsoleMessageon line 42 doesn't cause timing issues that could contribute to test flakiness.Consider adding explicit wait conditions or timeouts to make the test more deterministic:
test(`early hydration of page: ${pageName}`, async ({ page, waitForConsoleMessage, matchPageSnapshot, sendRedisItemValue }) => { - waitForConsoleMessage('ToggleContainer with title'); + await waitForConsoleMessage('ToggleContainer with title'); await page.click('.toggle-button');Note: Verify if
waitForConsoleMessageis intentionally not awaited or if this is a missingawait.react_on_rails_pro/spec/dummy/app/views/pages/redis_receiver.html.erb (1)
1-5: No action required. @request_id initialization verified.The verification confirms that both controller actions properly initialize
@request_id:
redis_receiver(line 73): Sets@request_id = SecureRandom.uuidredis_receiver_for_testing(line 96): Sets@request_id = params[:request_id]with validationThe view dependency is satisfied.
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts (1)
49-54: Confirm stream field naming matches client expectationsYou’re writing fields as : (colon-prefixed). Ensure listenToRequestData/getValue expects this shape.
If not required, drop the colon:
- await redisClient.xAdd(`stream:${redisRequestId}`, '*', { [`:${key}`]: JSON.stringify(value) }); + await redisClient.xAdd(`stream:${redisRequestId}`, '*', { [key]: JSON.stringify(value) });
| # TODO: DRY with previous job | ||
| dummy-app-node-renderer-e2-tests: | ||
| docker: | ||
| - image: *docker_image | ||
| - image: cimg/redis:6.2.6 | ||
| steps: | ||
| - checkout | ||
| - run: *print-system-info | ||
| - restore_cache: *restore-package-gem-cache | ||
| - restore_cache: *restore-package-node-modules-cache | ||
| - restore_cache: *restore-dummy-app-node-modules-cache | ||
| - restore_cache: *restore-dummy-app-gem-cache | ||
| - run: rm -rf react_on_rails_pro/spec/dummy/public/webpack | ||
| - run: rm -rf react_on_rails_pro/spec/dummy/ssr-generated | ||
| - restore_cache: *restore-dummy-app-webpack-bundle-cache | ||
| - run: *install-dummy-app-ruby-gems | ||
| - run: *install-package-node-modules | ||
| - run: *install-latest-chrome | ||
| - run: *install-dummy-app-node-modules | ||
| - run: | ||
| name: Generate file-system based entrypoints (Pro) | ||
| working_directory: react_on_rails_pro | ||
| command: cd spec/dummy && bundle exec rake react_on_rails:generate_packs | ||
| - run: | ||
| name: Run Pro Node renderer in a background | ||
| working_directory: react_on_rails_pro | ||
| command: cd spec/dummy && yarn run node-renderer | ||
| background: true | ||
| - run: | ||
| name: run rails server in background (Pro dummy app) | ||
| working_directory: react_on_rails_pro | ||
| command: cd spec/dummy && RAILS_ENV=test rails server | ||
| background: true | ||
| - run: | ||
| name: wait for rails server to start | ||
| command: | | ||
| while ! curl -s http://localhost:3000 > /dev/null; do sleep 1; done | ||
| - run: | ||
| name: Run playwright tests (Pro dummy app) | ||
| working_directory: react_on_rails_pro/spec/dummy | ||
| command: yarn e2e-test | ||
| - store_test_results: | ||
| path: react_on_rails_pro/spec/dummy/results.xml | ||
| - store_artifacts: | ||
| path: react_on_rails_pro/spec/dummy/playwright-report |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor to eliminate job duplication via YAML anchors.
The dummy-app-node-renderer-e2-tests job is nearly identical to rspec-dummy-app-node-renderer (lines 289–344), with only the Redis service addition and test command differing. The TODO comment acknowledges this.
Use YAML anchors to extract common steps and compose the jobs to follow DRY principles:
- &e2e-test-steps
- checkout
- run: *print-system-info
# ... (shared steps)
- run:
name: Run tests
working_directory: react_on_rails_pro/spec/dummy
command: << parameters.test_command >>Then define both jobs by extending shared steps with job-specific parameters.
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx
Outdated
Show resolved
Hide resolved
| }, | ||
| "devDependencies": { | ||
| "@babel/preset-typescript": "^7.23.2", | ||
| "@playwright/test": "^1.56.1", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Playwright version is latest or intentional.
Playwright ^1.56.1 is relatively recent. Confirm this version is current and compatible with your Node.js/React environment as of October 2025.
🌐 Web query:
What is the latest version of Playwright test runner as of October 2025?
💡 Result:
As of October 20, 2025 the latest stable Playwright Test runner release is @playwright/test 1.56.0 (Playwright 1.56). [Oct 6, 2025]. [1][2]
Note: canary/alpha builds for 1.57.x are available (1.57.0-alpha) if you need the very latest pre-release changes. [2][3]
Sources:
[1] Playwright release notes — Version 1.56. (playwright.dev)
[2] npm/yarn release listings showing 1.56.0 (stable) and 1.57.0-alpha (canary). (classic.yarnpkg.com)
Citations:
- 1: https://playwright.dev/docs/release-notes?utm_source=openai
- 2: https://classic.yarnpkg.com/en/package/%40playwright/test?utm_source=openai
Update Playwright version to available stable release.
The specified version ^1.56.1 references a patch version that doesn't exist in stable releases. As of October 20, 2025, the latest stable Playwright Test runner release is @playwright/test 1.56.0. Update to ^1.56.0 to match the actual latest stable release.
🤖 Prompt for AI Agents
In react_on_rails_pro/spec/dummy/package.json at line 80, the Playwright
dependency is pinned to ^1.56.1 which is not a valid stable release; change the
version specifier to ^1.56.0 so it matches the actual latest stable
@playwright/test release. Update the package.json entry to use "^1.56.0", run
npm/yarn install to update lockfile, and commit the updated package.json and
lockfile.
PR Review: Fix streaming flaky testsSummaryThis PR replaces flaky RSpec system tests with Playwright-based end-to-end tests to verify React Server Components streaming behavior. The changes include new test infrastructure, CI integration, and test components. ✅ Positive Aspects
🔴 Critical Issues1. Test files committed that should be development only
|
Pull Request Review: Fix streaming flaky testsSummaryThis PR replaces the old Capybara/Selenium system tests with a modern Playwright-based E2E testing framework for testing streaming behavior. The implementation adds comprehensive tests for React Server Components (RSC) streaming with Redis, including a new demo component and well-structured test fixtures. ✅ Strengths1. Excellent Test Architecture
2. Clean Component Design
3. CI/CD Integration
🐛 Potential Issues1. Critical: Incomplete cleanup in fixture.tsLocation: redisRequestId: async ({ redisClient }, use) => {
await use(randomUUID());
redisClient.del // ⚠️ INCOMPLETE LINE - no cleanup happening
},Issue: This line appears to be incomplete - it references Fix needed: redisRequestId: async ({ redisClient }, use) => {
const requestId = randomUUID();
await use(requestId);
// Clean up the stream after the test
await redisClient.del(`stream:${requestId}`);
},2. Resource Management: Redis Connection Leak RiskLocation: Issues:
Concerns:
Suggestions: // Use delete instead of setting to undefined
delete activeListeners[requestId];
delete pendingPromises[key];3. Thread Safety in ControllerLocation: Issue: Using
Note: This appears to be demo/test code, so it's acceptable for that purpose, but should not be used as a pattern for production code. 4. Missing Newline at EOFMultiple snapshot files are missing trailing newlines, which violates the CLAUDE.md requirement: Files affected:
Action required: Add newlines to pass linting 5. Hardcoded Timeout ValuesLocations:
Suggestion: Consider extracting these to constants or configuration for easier tuning. 🔒 Security Considerations1. No Major Security Issues ✅The code is defensive-oriented test infrastructure, which is acceptable. 2. Minor: Request ID ValidationLocation: @request_id = params[:request_id]
raise "request_id is required at the url" if @request_id.blank?Suggestion: Consider validating the UUID format to prevent potential injection or abuse: raise "Invalid request_id format" unless @request_id.match?(/\A[0-9a-f-]{36}\z/i)🎯 Performance Considerations1. Redis Stream PollingThe 2. Message DeletionMessages are deleted after processing (lines 149-161), which is good for preventing unbounded growth. 3. Test ParallelizationCI config sets 📋 Test CoverageExcellent Coverage ✅
Potential Additions
📝 Documentation & CompletenessMissing (per PR checklist):
The PR body still contains the template placeholder text. Please update it with:
🔧 Minor Issues
✅ RecommendationsMust Fix:
Should Fix:
Nice to Have:
🎉 ConclusionThis is a solid improvement to the testing infrastructure. The Playwright migration is well-executed with thoughtful test design. The main concerns are:
Once the critical issues are addressed (particularly the incomplete Overall Assessment: Approve pending fixes for critical issues Review generated with attention to the project's CLAUDE.md guidelines, focusing on RuboCop compliance, formatting requirements, and defensive security practices. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts (3)
33-34: Use quit()/disconnect() with node-redis 5.x, not close()- await use(client as RedisClientType); - await client.close(); + await use(client as RedisClientType); + try { + await client.quit(); + } catch { + await client.disconnect(); + }
67-75: Use supported Playwright API to wait for console messagesRemove
page.consoleMessages(); rely onwaitForEvent.- waitForConsoleMessage: async({ page }, use) =>{ + waitForConsoleMessage: async({ page }, use) =>{ await use(async(msg) => { - if ((await page.consoleMessages()).find(consoleMsg => consoleMsg.text().includes(msg))) { - return; - } - await page.waitForEvent('console', { predicate: (consoleMsg) => consoleMsg.text().includes(msg), }) }) },
78-82: Capture network requests via event listener;page.requests()doesn’t existBuffer requests with
page.on('request').- getNetworkRequests: async({ page }, use) => { - await use(async(requestUrlPattern) => { - return (await page.requests()).filter(request => request.url().match(requestUrlPattern)) - }) - } + getNetworkRequests: async({ page }, use) => { + const captured: Request[] = []; + const onReq = (r: Request) => captured.push(r); + page.on('request', onReq); + await use(async (requestUrlPattern) => captured.filter(r => requestUrlPattern.test(r.url()))); + page.off('request', onReq); + }
🧹 Nitpick comments (3)
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx (2)
21-24: Don’t pass children via prop (Biome noChildrenProp)Use JSX children instead.
-const AsyncToggleContainer = async ({ children, childrenTitle, getValue }) => { +const AsyncToggleContainer = async ({ children, childrenTitle, getValue }) => { await getValue('ToggleContainer'); - return <ToggleContainer children={children} childrenTitle={childrenTitle} /> + return <ToggleContainer childrenTitle={childrenTitle}>{children}</ToggleContainer> }
33-35: Avoid passing unused props to ToggleContainerOnly AsyncToggleContainer needs
getValue. Pass conditionally.- const UsedToggleContainer = asyncToggleContainer ? AsyncToggleContainer : ToggleContainer; - const toggleContainerGetValueParam = asyncToggleContainer ? getValue : undefined; + const UsedToggleContainer = asyncToggleContainer ? AsyncToggleContainer : ToggleContainer; @@ - <UsedToggleContainer childrenTitle="Redis Items" getValue={toggleContainerGetValueParam}> + <UsedToggleContainer + childrenTitle="Redis Items" + {...(asyncToggleContainer ? { getValue } : {})} + >Also applies to: 41-41
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts (1)
27-31: Replace empty object pattern param (Biome noEmptyPattern)Avoid
{}destructuring; use_placeholder.- redisClient: [async ({}, use, workerInfo) => { + redisClient: [async (_opts, use, workerInfo) => { console.log(`Creating Redis Client at Worker ${workerInfo.workerIndex}`) const url = process.env.REDIS_URL || 'redis://localhost:6379'; const client = createClient({ url }); await client.connect();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.circleci/config.yml(2 hunks)react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb(1 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx(1 hunks)react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts(1 hunks)react_on_rails_pro/spec/dummy/e2e-tests/streaming.spec.ts(1 hunks)react_on_rails_pro/spec/dummy/playwright.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb
- react_on_rails_pro/spec/dummy/playwright.config.ts
- .circleci/config.yml
- react_on_rails_pro/spec/dummy/e2e-tests/streaming.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
react_on_rails_pro/spec/dummy/e2e-tests/fixture.tsreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
react_on_rails_pro/spec/dummy/e2e-tests/fixture.tsreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx
🧬 Code graph analysis (2)
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts (1)
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx (1)
value(7-7)
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx (2)
react_on_rails_pro/spec/dummy/client/app/components/RSCPostsPage/ToggleContainer.jsx (1)
ToggleContainer(3-19)react_on_rails_pro/spec/dummy/client/app/components/ErrorBoundary.tsx (1)
ErrorBoundary(7-9)
🪛 Biome (2.1.2)
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts
[error] 27-27: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx
[error] 23-23: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: build
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: claude-review
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: build-and-test
| const RedisItem = async ({ getValue, itemIndex }) => { | ||
| const value = await getValue(`Item${itemIndex}`); | ||
| return <li className={`redis-item${itemIndex}`}>Value of "Item{itemIndex + 1}": {value}</li> | ||
| } |
There was a problem hiding this comment.
Fix JSX interpolation: avoid braces inside quotes
Renders literal Item{itemIndex + 1}. Interpolate via template or split nodes.
-const RedisItem = async ({ getValue, itemIndex }) => {
- const value = await getValue(`Item${itemIndex}`);
- return <li className={`redis-item${itemIndex}`}>Value of "Item{itemIndex + 1}": {value}</li>
-}
+const RedisItem = async ({ getValue, itemIndex }) => {
+ const value = await getValue(`Item${itemIndex}`);
+ return (
+ <li className={`redis-item${itemIndex}`}>
+ {`Value of "Item${itemIndex + 1}": ${String(value)}`}
+ </li>
+ );
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const RedisItem = async ({ getValue, itemIndex }) => { | |
| const value = await getValue(`Item${itemIndex}`); | |
| return <li className={`redis-item${itemIndex}`}>Value of "Item{itemIndex + 1}": {value}</li> | |
| } | |
| const RedisItem = async ({ getValue, itemIndex }) => { | |
| const value = await getValue(`Item${itemIndex}`); | |
| return ( | |
| <li className={`redis-item${itemIndex}`}> | |
| {`Value of "Item${itemIndex + 1}": ${String(value)}`} | |
| </li> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx
around lines 6 to 9, the JSX is rendering the literal text Item{itemIndex + 1}
because the braces are inside a quoted string; change the node so the
interpolation is evaluated — e.g. replace the quoted "Item{itemIndex + 1}" with
a JSX expression such as {`Item${itemIndex + 1}`} or {"Item" + (itemIndex + 1)}
so the correct item number is displayed (keep the rest of the element as-is).
| const RedisItemWithWrapper = ({ getValue, itemIndex }) => ( | ||
| <section className={`redis-item${itemIndex}-container`}> | ||
| <Suspense fallback={<p className={`redis-item${itemIndex}-fallback`}>Waiting for the key "Item{itemIndex + 1}"</p>}> | ||
| <RedisItem getValue={getValue} itemIndex={itemIndex} /> | ||
| </Suspense> | ||
| </section> |
There was a problem hiding this comment.
Fix Suspense fallback text interpolation
Same issue; braces are inside quotes.
- <section className={`redis-item${itemIndex}-container`}>
- <Suspense fallback={<p className={`redis-item${itemIndex}-fallback`}>Waiting for the key "Item{itemIndex + 1}"</p>}>
+ <section className={`redis-item${itemIndex}-container`}>
+ <Suspense
+ fallback={
+ <p className={`redis-item${itemIndex}-fallback`}>
+ {`Waiting for the key "Item${itemIndex + 1}"`}
+ </p>
+ }
+ >
<RedisItem getValue={getValue} itemIndex={itemIndex} />
</Suspense>
</section>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const RedisItemWithWrapper = ({ getValue, itemIndex }) => ( | |
| <section className={`redis-item${itemIndex}-container`}> | |
| <Suspense fallback={<p className={`redis-item${itemIndex}-fallback`}>Waiting for the key "Item{itemIndex + 1}"</p>}> | |
| <RedisItem getValue={getValue} itemIndex={itemIndex} /> | |
| </Suspense> | |
| </section> | |
| const RedisItemWithWrapper = ({ getValue, itemIndex }) => ( | |
| <section className={`redis-item${itemIndex}-container`}> | |
| <Suspense | |
| fallback={ | |
| <p className={`redis-item${itemIndex}-fallback`}> | |
| {`Waiting for the key "Item${itemIndex + 1}"`} | |
| </p> | |
| } | |
| > | |
| <RedisItem getValue={getValue} itemIndex={itemIndex} /> | |
| </Suspense> | |
| </section> | |
| ) |
🤖 Prompt for AI Agents
In
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx
around lines 11 to 16, the Suspense fallback text uses literal braces inside a
quoted string ("Item{itemIndex + 1}") so it renders the braces instead of
interpolating the expression; change the fallback to use a JSX expression that
interpolates itemIndex (for example wrap a template literal or concatenation in
curly braces) so it renders the evaluated Item number while keeping the existing
className template literal unchanged.
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx
Show resolved
Hide resolved
PR Review: Fix streaming flaky testsSummaryThis PR adds comprehensive Playwright-based E2E tests for streaming functionality with Redis, introducing a robust testing infrastructure for React Server Components streaming behavior. The implementation includes CI/CD integration and well-structured test fixtures. ✅ Code Quality & Best PracticesStrengths
Areas for ImprovementCritical Issues:
Code Quality Issues:
🐛 Potential Bugs
🔒 Security Concerns
⚡ Performance Considerations
🧪 Test CoverageExcellent Coverage:
Consider Adding:
📋 Action ItemsMust Fix Before Merge:
Should Fix:
Consider:
💭 Additional Notes
Overall Assessment: This is a well-architected addition to the test suite that significantly improves confidence in streaming functionality. The critical bugs (undefined variable and incomplete cleanup) must be fixed before merge, but the overall approach is sound. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx (3)
6-9: Fix JSX interpolation: text renders literal braces.Line 8 displays
"Item{itemIndex + 1}"as literal text because the braces are inside a string. Users will seeItem{itemIndex + 1}instead ofItem1,Item2, etc.Apply this diff to interpolate the expression:
const RedisItem = async ({ getValue, itemIndex }) => { const value = await getValue(`Item${itemIndex}`); - return <li className={`redis-item${itemIndex}`}>Value of "Item{itemIndex + 1}": {value}</li> + return <li className={`redis-item${itemIndex}`}>{`Value of "Item${itemIndex + 1}": ${value}`}</li> }
11-17: Fix Suspense fallback interpolation.Line 13 has the same issue:
"Item{itemIndex + 1}"renders as literal text in the fallback message.Apply this diff:
const RedisItemWithWrapper = ({ getValue, itemIndex }) => ( <section className={`redis-item${itemIndex}-container`}> - <Suspense fallback={<p className={`redis-item${itemIndex}-fallback`}>Waiting for the key "Item{itemIndex + 1}"</p>}> + <Suspense fallback={<p className={`redis-item${itemIndex}-fallback`}>{`Waiting for the key "Item${itemIndex + 1}"`}</p>}> <RedisItem getValue={getValue} itemIndex={itemIndex} /> </Suspense> </section> )
29-31: Guard railsContext before usinginoperator.If
railsContextis undefined or null, line 29 throws a TypeError. Past review marked this as addressed, but the guard is still missing.Apply this diff:
- if ('addPostSSRHook' in railsContext) { + if (railsContext && 'addPostSSRHook' in railsContext) { railsContext.addPostSSRHook(close); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.circleci/config.yml(2 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .circleci/config.yml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx
🧬 Code graph analysis (1)
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx (2)
react_on_rails_pro/spec/dummy/client/app/components/RSCPostsPage/ToggleContainer.jsx (1)
ToggleContainer(3-19)react_on_rails_pro/spec/dummy/client/app/components/ErrorBoundary.tsx (1)
ErrorBoundary(7-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: build
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: build-and-test
🔇 Additional comments (2)
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx (2)
1-4: LGTM: Imports are clean.All imports are properly structured and relevant to the component's functionality.
19-24: LGTM: AsyncToggleContainer implementation is correct.The async wrapper pattern properly awaits the getValue call before rendering the ToggleContainer.
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx
Outdated
Show resolved
Hide resolved
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx
Outdated
Show resolved
Hide resolved
Pull Request Review - Fix Streaming Flaky TestsThank you for working on fixing the flaky streaming tests! This is an important improvement to the test suite. I've reviewed the changes and have the following feedback: 🎯 Overall AssessmentPositives:
Critical Issues to Address:
🐛 Bugs & IssuesCritical: Undefined Variable in RedisReceiver.jsx (Line 42)The variable toggleContainerGetValueParam is referenced but never defined, which will cause a ReferenceError at runtime. Location: react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx:42 Fix: This should likely use the asyncToggleContainer prop instead, or be removed entirely. Question: "use client" directive in RSCRoute.tsxWhy is the "use client" directive being added to RSCRoute.tsx? This seems counterintuitive for a component dealing with RSC (React Server Components). Please clarify if this is intentional and document the reasoning. Location: packages/react-on-rails-pro/src/RSCRoute.tsx:16
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts (3)
36-38: Add cleanup for Redis stream keys.The fixture generates a request ID but doesn't clean up the associated Redis stream key (
stream:${id}). Over time, this will accumulate test data in Redis.Apply this diff:
redisRequestId: async ({ redisClient }, use) => { - await use(randomUUID()); + const id = randomUUID(); + try { + await use(id); + } finally { + // Clean up the stream for this test + await redisClient.del(`stream:${id}`); + } },
77-81: Replace invalid Playwright API with event listener.
page.requests()is not a Playwright API and will cause runtime errors. Network requests must be captured via an event listener.Apply this diff:
getNetworkRequests: async({ page }, use) => { + const captured: Request[] = []; + const onRequest = (r: Request) => captured.push(r); + page.on('request', onRequest); await use(async(requestUrlPattern) => { - return (await page.requests()).filter(request => request.url().match(requestUrlPattern)) + return captured.filter(request => request.url().match(requestUrlPattern)); }) + page.off('request', onRequest); }
66-75: Remove invalid Playwright API call.
page.consoleMessages()is not a Playwright API and will cause runtime errors. The pre-check is unnecessary sincewaitForEventwill resolve immediately if the event has already fired.Apply this diff:
waitForConsoleMessage: async({ page }, use) =>{ await use(async(msg) => { - if ((await page.consoleMessages()).find(consoleMsg => consoleMsg.text().includes(msg))) { - return; - } - await page.waitForEvent('console', { predicate: (consoleMsg) => consoleMsg.text().includes(msg), }) }) },
🧹 Nitpick comments (1)
react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb (1)
72-99: Consider capturing @request_id locally in the thread.While the current implementation is safe (the thread is joined before the action completes), it's clearer to capture instance variables as local variables when passing them to threads to avoid potential confusion.
Apply this diff:
def redis_receiver @request_id = SecureRandom.uuid - + request_id = @request_id + redis_thread = Thread.new do redis = ::Redis.new 5.times do |index| sleep 1 - redis.xadd("stream:#{@request_id}", { ":Item#{index}" => "Value of Item#{index + 1}".to_json }) + redis.xadd("stream:#{request_id}", { ":Item#{index}" => "Value of Item#{index + 1}".to_json }) end rescue StandardError => e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb(1 hunks)react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts
🧬 Code graph analysis (2)
react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb (1)
react_on_rails_pro/lib/react_on_rails_pro/concerns/stream.rb (1)
stream_view_containing_react_components(33-47)
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts (1)
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx (1)
value(7-7)
🪛 Biome (2.1.2)
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts
[error] 27-27: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: build
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: claude-review
- GitHub Check: build-and-test
PR Review: Fix streaming flaky testsOverviewThis PR adds Playwright-based end-to-end tests for streaming functionality using Redis, replacing flaky integration tests. The implementation includes a new Strengths✅ Excellent Test Architecture
✅ Good Code Organization
✅ CI/CD Integration
Issues Found🔴 Critical Issues1. Missing File in Repository
2. Missing Newline at EOF
3. Incomplete PR Description
🟡 Major Issues4. Potential Redis Connection Leak async function closeRedisClient() {
if (sharedRedisClient && isClientConnected) {
await sharedRedisClient.quit();
isClientConnected = false;
// Missing: sharedRedisClient = null;
}
}
5. Missing Error Boundary Import
6. JSX Type Safety Issue return <li>Value of "Item{itemIndex + 1}": {value}</li>Should be: return <li>Value of "Item{itemIndex + 1}": {value}</li>The issue is 7. React Server Component Pattern Issue
8. Missing "use client" Directive 🟢 Minor Issues9. Inconsistent Naming
10. Missing PropTypes/TypeScript interface RedisReceiverProps {
requestId: string;
asyncToggleContainer?: boolean;
}11. Hard-coded Array Mapping 12. Magic Numbers
13. Console.log in Production Code 14. TODO Comment in CI Config Performance Considerations
Security Concerns🔒 Redis Connection String
🔒 Request ID Validation
Test Coverage Assessment✅ Well Covered:
❌ Missing Coverage:
RecommendationsBefore Merging:
Post-Merge Improvements:
VerdictChanges Requested The test infrastructure is excellent and the approach is sound, but there are several critical issues that need to be addressed:
Once these issues are resolved, this will be a solid improvement to test reliability. Great work on the test architecture! The use of Playwright fixtures and ARIA snapshots is particularly well done. The incremental testing approach is exactly what's needed for validating streaming behavior. |
PR Review: Fix streaming flaky testsSummaryThis PR replaces legacy RSpec integration tests with modern Playwright end-to-end tests for streaming server rendering. The implementation adds Redis-backed streaming demos and comprehensive test coverage using ARIA snapshot testing. StrengthsTest Architecture (Excellent)
Test Coverage (Comprehensive)Covers critical scenarios:
Code Quality
Areas for Improvement1. Missing PR DescriptionThe PR body still contains template placeholders. Please update to include:
2. Missing CHANGELOG EntryPer CLAUDE.md, add a CHANGELOG entry at the top of the file. 3. Potential Race Conditionpages_controller.rb:72-99 - redis_receiver starts streaming immediately after starting thread. Add comment explaining if intentional. 4. Hardcoded Test DataMagic number 5 and [0,1,2,3,4] hardcoded in multiple places. Extract to TEST_ITEM_COUNT constant. 5. Missing Error HandlingAdd test case for Redis failure scenarios to verify error boundary behavior. 6. TypeScript Configurationtsconfig.json uses allowDefaultProject workaround. Consider separate tsconfig in e2e-tests directory. 7. Test Coverage ComparisonRemoved 421 lines from integration_spec.rb. Please confirm all coverage is maintained in new tests. 8. Multiple Browser TestingRunning on Chromium, Firefox, AND WebKit triples CI time. Consider Chromium-only for faster feedback. SecurityNo concerns - proper UUID usage, namespaced Redis streams, cleanup prevents leaks. Before Merging
AssessmentCode Quality: 4.5/5 Excellent test architecture! The fixture-based approach is exemplary. Main issues are process-related (CHANGELOG, PR description) rather than code quality. Ready to merge once documentation is complete. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts (2)
77-86: Unsupported Playwright API: replace page.consoleMessages() and make it robust to messages logged before awaitingpage.consoleMessages() doesn’t exist. Also, if the message was logged before awaiting, the current approach misses it. Capture console history via a one‑time listener and consult it before waiting.
- waitForConsoleMessage: async ({ page }, use) => { - await use(async (msg) => { - if ((await page.consoleMessages()).find((consoleMsg) => consoleMsg.text().includes(msg))) { - return; - } - - await page.waitForEvent('console', { - predicate: (consoleMsg) => consoleMsg.text().includes(msg), - }); - }); - }, + // Auto-captured console history (see fixture added below) + waitForConsoleMessage: async ({ page, consoleHistory }, use) => { + await use(async (msg) => { + if (consoleHistory.some((t) => t.includes(msg))) return; + await page.waitForEvent('console', { predicate: (c) => c.text().includes(msg) }); + }); + },Add these auto fixtures to capture history early:
const redisControlledTest = base.extend<RedisRequestIdFixture, RedisClientFixture>({ @@ }, + // Capture console history early to avoid missing already-logged messages + consoleHistory: [ + async ({ page }, use) => { + const history: string[] = []; + const onConsole = (m: any) => history.push(m.text()); + page.on('console', onConsole); + try { + await use(history); + } finally { + page.off('console', onConsole); + } + }, + { auto: true }, + ],And extend the type:
type RedisRequestIdFixture = { redisRequestId: string; nonBlockingNavigateWithRequestId: (path: string) => Promise<Response | null>; + consoleHistory: string[]; };
88-92: Unsupported Playwright API and possible false negatives: replace page.requests() and start capture before navigationpage.requests() doesn’t exist. Use a request listener and ensure it’s attached before page navigation to avoid missing SSR requests.
- getNetworkRequests: async ({ page }, use) => { - await use(async (requestUrlPattern) => { - return (await page.requests()).filter((request) => request.url().match(requestUrlPattern)); - }); - }, + getNetworkRequests: async ({ requestsHistory }, use) => { + await use(async (requestUrlPattern) => + requestsHistory.filter((r) => requestUrlPattern.test(r.url())), + ); + },Add an auto fixture to record requests:
const redisControlledTest = base.extend<RedisRequestIdFixture, RedisClientFixture>({ @@ }, + // Capture all network requests from the start of the test + requestsHistory: [ + async ({ page }, use) => { + const captured: Request[] = []; + const onReq = (r: Request) => captured.push(r); + page.on('request', onReq); + try { + await use(captured); + } finally { + page.off('request', onReq); + } + }, + { auto: true }, + ],Update type to include the history fixture:
type RedisRequestIdFixture = { redisRequestId: string; nonBlockingNavigateWithRequestId: (path: string) => Promise<Response | null>; + requestsHistory: Request[]; };
🧹 Nitpick comments (4)
react_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsx (1)
77-80: Optional: DRY the path to avoid driftPath literal appears in both Link and Route. Consider extracting a local const to keep them in sync.
export default function App({ basePath = '/server_router', ...props }: { basePath?: string }) { return ( <ErrorBoundary> + {/* + Keep Link and Route in sync + */} + const redisReceiverPath = `${basePath}/redis-receiver-for-testing`; <nav> <ul style={{ listStyle: 'none', padding: 0, margin: 0 }}> <li> - <Link to={`${basePath}/redis-receiver-for-testing`}>Redis Receiver For Testing</Link> + <Link to={redisReceiverPath}>Redis Receiver For Testing</Link> </li> ... - <Route - path={`${basePath}/redis-receiver-for-testing`} - element={<RSCRoute componentName="RedisReceiver" componentProps={props} />} - /> + <Route path={redisReceiverPath} element={<RSCRoute componentName="RedisReceiver" componentProps={props} />} />react_on_rails_pro/spec/dummy/e2e-tests/streaming.spec.ts (2)
26-27: Avoid shadowing Playwright’stestRename the tuple element to reduce confusion and potential lint noise.
-).forEach(([pageName, test]) => { - test(`incremental rendering of page: ${pageName}`, async ({ matchPageSnapshot, sendRedisItemValue }) => { +).forEach(([pageName, pageTest]) => { + pageTest(`incremental rendering of page: ${pageName}`, async ({ matchPageSnapshot, sendRedisItemValue }) => { ... - test(`early hydration of page: ${pageName}`, async ({ + pageTest(`early hydration of page: ${pageName}`, async ({
82-87: Network request assertions may be timing‑sensitiveIf flakes persist, switch to polling to wait until the request set stabilizes.
- await expect(await getNetworkRequests(/rsc_payload/)).toHaveLength(0); + await expect.poll(() => getNetworkRequests(/rsc_payload/)).toHaveLength(0); - await expect(await getNetworkRequests(/rsc_payload/)).toHaveLength(1); + await expect.poll(() => getNetworkRequests(/rsc_payload/)).toHaveLength(1); - await expect(await getNetworkRequests(/rsc_payload/)).toHaveLength(1); + await expect.poll(() => getNetworkRequests(/rsc_payload/)).toHaveLength(1);Also applies to: 89-94, 96-105
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts (1)
49-55: Optional: add a timeout to nonBlocking navigationPrevent tests from hanging indefinitely on navigation.
- return page.goto(fullPath, { waitUntil: 'commit' }); + return page.goto(fullPath, { waitUntil: 'commit', timeout: 15000 });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/react-on-rails-pro/src/RSCRoute.tsx(1 hunks)packages/react-on-rails-pro/tsconfig.json(1 hunks)react_on_rails_pro/spec/dummy/client/app/components/RSCPostsPage/ToggleContainer.jsx(1 hunks)react_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsx(2 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx(1 hunks)react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts(1 hunks)react_on_rails_pro/spec/dummy/e2e-tests/streaming.spec.ts(1 hunks)react_on_rails_pro/spec/dummy/playwright.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- react_on_rails_pro/spec/dummy/playwright.config.ts
- react_on_rails_pro/spec/dummy/client/app/components/RSCPostsPage/ToggleContainer.jsx
- react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
packages/react-on-rails-pro/src/RSCRoute.tsxpackages/react-on-rails-pro/tsconfig.jsonreact_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsxreact_on_rails_pro/spec/dummy/e2e-tests/fixture.tsreact_on_rails_pro/spec/dummy/e2e-tests/streaming.spec.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
packages/react-on-rails-pro/src/RSCRoute.tsxreact_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsxreact_on_rails_pro/spec/dummy/e2e-tests/fixture.tsreact_on_rails_pro/spec/dummy/e2e-tests/streaming.spec.ts
🧠 Learnings (3)
📚 Learning: 2025-06-09T07:58:02.646Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
Applied to files:
packages/react-on-rails-pro/src/RSCRoute.tsx
📚 Learning: 2025-06-11T12:34:58.182Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCProvider.tsx:0-0
Timestamp: 2025-06-11T12:34:58.182Z
Learning: `RSCProvider` intentionally keeps failed `getServerComponent` promises cached (even if rejected) to avoid repeated fetch attempts; callers must use `refetchComponent` to retry.
Applied to files:
packages/react-on-rails-pro/src/RSCRoute.tsx
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
packages/react-on-rails-pro/src/RSCRoute.tsx
🧬 Code graph analysis (2)
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts (1)
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx (1)
value(7-7)
react_on_rails_pro/spec/dummy/e2e-tests/streaming.spec.ts (1)
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts (5)
redisReceiverPageTest(149-149)redisReceiverInsideRouterPageTest(150-150)redisReceiverPageAfterNavigationTest(151-151)redisReceiverPageWithAsyncClientComponentTest(152-152)redisReceiverPageController(148-148)
🪛 Biome (2.1.2)
react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts
[error] 28-28: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: build
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: claude-review
- GitHub Check: build-and-test
🔇 Additional comments (3)
packages/react-on-rails-pro/tsconfig.json (1)
7-7: File not found in repository.The file
packages/react-on-rails-pro/tsconfig.jsondoes not exist at the specified path. The review comment cannot be verified against actual file content.Additionally,
allowDefaultProjectis not a tsconfig.json option—it's an ESLint/typescript-eslint parser option for the ProjectService, not a TypeScript compiler configuration field. If this code change exists elsewhere, ensureallowDefaultProjectis placed in ESLint configuration (parserOptions.projectService) rather than in tsconfig.json.Please confirm:
- Is the file path correct?
- Has this file been moved or deleted in this PR?
- Should the review comment target a different configuration file (e.g., .eslintrc, eslint.config.js)?
packages/react-on-rails-pro/src/RSCRoute.tsx (1)
17-18: LGTM! The "use client" directive correctly marks RSCRoute as a client component.This change makes the client boundary explicit, which is correct since RSCRoute uses client-side hooks (
useRSC) and maintains state (error boundary). The explicit directive helps bundlers and the React compiler consistently treat this as a client component, which may address the flaky test behavior mentioned in the PR objectives by ensuring deterministic client/server boundary detection during SSR, hydration, and client-side navigation.react_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsx (1)
44-46: LGTM: link matches route and test selectorThe nav entry is correct and matches the test click text. No issues.
| async ({}, use, workerInfo) => { | ||
| console.log(`Creating Redis Client at Worker ${workerInfo.workerIndex}`); | ||
| const url = process.env.REDIS_URL || 'redis://localhost:6379'; | ||
| const client = createClient({ url }); | ||
| await client.connect(); | ||
| await use(client as RedisClientType); | ||
| await client.quit(); | ||
| }, |
There was a problem hiding this comment.
Fix Biome error and harden Redis client lifecycle
- Replace empty object pattern (Biome error).
- Ensure teardown runs even on failures, prefer quit() with disconnect() fallback, and attach an error handler.
- redisClient: [
- async ({}, use, workerInfo) => {
+ redisClient: [
+ async (_testInfo, use, workerInfo) => {
console.log(`Creating Redis Client at Worker ${workerInfo.workerIndex}`);
const url = process.env.REDIS_URL || 'redis://localhost:6379';
const client = createClient({ url });
- await client.connect();
- await use(client as RedisClientType);
- await client.quit();
+ client.on('error', (err) => console.error('Redis client error in tests:', err));
+ await client.connect();
+ try {
+ await use(client as RedisClientType);
+ } finally {
+ try {
+ await client.quit();
+ } catch {
+ await client.disconnect();
+ }
+ }
},
{ scope: 'worker' },
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async ({}, use, workerInfo) => { | |
| console.log(`Creating Redis Client at Worker ${workerInfo.workerIndex}`); | |
| const url = process.env.REDIS_URL || 'redis://localhost:6379'; | |
| const client = createClient({ url }); | |
| await client.connect(); | |
| await use(client as RedisClientType); | |
| await client.quit(); | |
| }, | |
| async (_testInfo, use, workerInfo) => { | |
| console.log(`Creating Redis Client at Worker ${workerInfo.workerIndex}`); | |
| const url = process.env.REDIS_URL || 'redis://localhost:6379'; | |
| const client = createClient({ url }); | |
| client.on('error', (err) => console.error('Redis client error in tests:', err)); | |
| await client.connect(); | |
| try { | |
| await use(client as RedisClientType); | |
| } finally { | |
| try { | |
| await client.quit(); | |
| } catch { | |
| await client.disconnect(); | |
| } | |
| } | |
| }, |
🧰 Tools
🪛 Biome (2.1.2)
[error] 28-28: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
| const redisReceiverPageTest = redisReceiverPageController.extend<RedisReceiverPageFixture>({ | ||
| pagePath: [ | ||
| async ({ nonBlockingNavigateWithRequestId }, use) => { | ||
| const pagePath = '/redis_receiver_for_testing'; | ||
| await nonBlockingNavigateWithRequestId(pagePath); | ||
| await use(pagePath); | ||
| }, | ||
| { auto: true }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Ensure listeners attach before navigation to avoid missing events
Force initialization order by depending on the auto fixtures (even if unused).
- async ({ nonBlockingNavigateWithRequestId }, use) => {
+ async ({ nonBlockingNavigateWithRequestId, consoleHistory: _ch, requestsHistory: _rh }, use) => {
const pagePath = '/redis_receiver_for_testing';
await nonBlockingNavigateWithRequestId(pagePath);
await use(pagePath);
},
@@
- async ({ page, nonBlockingNavigateWithRequestId, sendRedisValue }, use) => {
+ async ({ page, nonBlockingNavigateWithRequestId, sendRedisValue, consoleHistory: _ch, requestsHistory: _rh }, use) => {
const pagePath = '/redis_receiver_for_testing?async_toggle_container=true';
await nonBlockingNavigateWithRequestId(pagePath);
@@
- async ({ nonBlockingNavigateWithRequestId }, use) => {
+ async ({ nonBlockingNavigateWithRequestId, consoleHistory: _ch, requestsHistory: _rh }, use) => {
const pagePath = '/server_router/redis-receiver-for-testing';
await nonBlockingNavigateWithRequestId(pagePath);
await use(pagePath);
},
@@
- async ({ nonBlockingNavigateWithRequestId, page }, use) => {
+ async ({ nonBlockingNavigateWithRequestId, page, consoleHistory: _ch, requestsHistory: _rh }, use) => {
await nonBlockingNavigateWithRequestId('/server_router/simple-server-component');
await expect(page.getByText('Post 1')).toBeVisible({ timeout: 3000 });
await page.getByText('Redis Receiver For Testing').click();
await use('/server_router/redis-receiver-for-testing');
},Also applies to: 106-122, 124-133, 135-145
🤖 Prompt for AI Agents
In react_on_rails_pro/spec/dummy/e2e-tests/fixture.ts around lines 95-104 (also
apply the same fix to 106-122, 124-133, 135-145), the page fixture navigates
before the auto fixtures have been initialized which can miss event listeners;
fix this by making the async fixture depend on the auto fixtures (add them to
the destructured parameter list or include them in the fixture tuple) and
reference them (even as a no-op) before calling nonBlockingNavigateWithRequestId
so the auto fixtures are initialized and listeners are attached prior to
navigation.
Code Review: Fix streaming flaky testsThank you for this PR! This is a significant improvement that replaces flaky integration tests with a comprehensive Playwright E2E test suite. Here's my detailed review: ✅ Strengths1. Excellent Test Architecture
2. Strong Code Quality
3. CI/CD Integration
|
AbanoubGhadban
changed the title
Pull Request Review: Replace streaming flaky tests with Playwright E2E testsThank you for this comprehensive PR! The move from flaky integration tests to Playwright E2E tests is a great improvement. Here's my detailed review: ✅ Strengths1. Excellent Test Architecture
2. Good Code Organization
3. Strong Test Coverage
|
PR Review: Replace streaming flaky tests with Playwright E2E testsGreat work on migrating from flaky integration tests to Playwright E2E tests! This is a solid improvement for test reliability. Here's my detailed review: ✅ Strengths
|
Code Review: PR #1869 - Replace streaming flaky tests with Playwright E2E testsSummaryThis PR successfully replaces flaky RSpec integration tests with robust Playwright E2E tests for streaming server rendering functionality. The new tests use Redis-backed incremental rendering to verify streaming behavior across multiple scenarios. ✅ Strengths1. Excellent Test Architecture
2. Good Code Quality
3. CI Integration
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.circleci/config.yml(2 hunks)react_on_rails_pro/eslint.config.mjs(1 hunks)react_on_rails_pro/spec/dummy/e2e-tests/streaming.spec.ts(1 hunks)react_on_rails_pro/spec/dummy/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- react_on_rails_pro/spec/dummy/e2e-tests/streaming.spec.ts
- .circleci/config.yml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
react_on_rails_pro/spec/dummy/tsconfig.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: claude-review
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: build
🔇 Additional comments (2)
react_on_rails_pro/eslint.config.mjs (1)
200-200: The review comment is incorrect; the current glob pattern is appropriate for the codebase.The
e2e-tests/directory contains only two files at the top level (fixture.tsandstreaming.spec.ts) with no nested subdirectories. The current patternspec/dummy/e2e-tests/*correctly matches both files. The suggested patternspec/dummy/e2e-tests/**/*is unnecessary and inconsistent with the actual directory structure.Likely an incorrect or invalid review comment.
react_on_rails_pro/spec/dummy/tsconfig.json (1)
10-16: Verify thee2e-testsglob pattern will match all test files.The
e2e-testsdirectory doesn't exist yet, but the patterne2e-tests/*on line 14 only matches top-level files. If E2E tests will be organized in subdirectories, update the pattern toe2e-tests/**/*for recursive matching:"include": [ "client/**/*.ts", "client/**/*.tsx", "client/**/*.d.ts", - "e2e-tests/*", + "e2e-tests/**/*", "./playwright.config.ts" ]
| { | ||
| files: ['spec/dummy/e2e-tests/*'], | ||
|
|
||
| rules: { | ||
| 'no-empty-pattern': [ | ||
| 'error', | ||
| { | ||
| allowObjectPatternsAsParameters: true, | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| { | ||
| files: ['spec/dummy/e2e-tests/*'], | ||
| rules: { | ||
| 'react-hooks/rules-of-hooks': ['off'], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Combine duplicate configuration blocks.
Both blocks target the same file pattern. Merge them into a single configuration to reduce duplication and improve maintainability.
Apply this diff to consolidate the blocks:
{
files: ['spec/dummy/e2e-tests/*'],
rules: {
'no-empty-pattern': [
'error',
{
allowObjectPatternsAsParameters: true,
},
],
- },
- },
- {
- files: ['spec/dummy/e2e-tests/*'],
- rules: {
'react-hooks/rules-of-hooks': ['off'],
},
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| files: ['spec/dummy/e2e-tests/*'], | |
| rules: { | |
| 'no-empty-pattern': [ | |
| 'error', | |
| { | |
| allowObjectPatternsAsParameters: true, | |
| }, | |
| ], | |
| }, | |
| }, | |
| { | |
| files: ['spec/dummy/e2e-tests/*'], | |
| rules: { | |
| 'react-hooks/rules-of-hooks': ['off'], | |
| }, | |
| }, | |
| { | |
| files: ['spec/dummy/e2e-tests/*'], | |
| rules: { | |
| 'no-empty-pattern': [ | |
| 'error', | |
| { | |
| allowObjectPatternsAsParameters: true, | |
| }, | |
| ], | |
| 'react-hooks/rules-of-hooks': ['off'], | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In react_on_rails_pro/eslint.config.mjs around lines 199 to 216, there are two
separate override objects targeting the same files pattern
('spec/dummy/e2e-tests/*'); merge them into a single override object that
contains both rules ('no-empty-pattern' with allowObjectPatternsAsParameters:
true, and 'react-hooks/rules-of-hooks' set to 'off') to remove duplication and
keep configuration consolidated.
1 participant
Summary
Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.
Pull Request checklist
Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by
~.Add the CHANGELOG entry at the top of the file.
Other Information
Remove this paragraph and mention any other important and relevant information such as benchmarks.
This change is
Summary by CodeRabbit
New Features
Tests
Chores