Fix Pro Node Renderer tests: React 19 compatibility and race conditions #2110
Conversation
This commit fixes several issues in the Pro Node Renderer test suite:
**React 19 Compatibility**
- React 19 removed the `data-react-checksum` attribute from server-rendered HTML
- Updated vm.test.ts assertions to verify component rendering by checking HTML content presence and length instead of checksums
- Updated assertion counts to reflect new test structure (doubled per component: one for presence, one for length)
**Fixed Bundle Paths**
- Corrected bundle paths in httpRequestUtils.ts and serverRenderRSCReactComponent.test.js to point to `react_on_rails_pro/spec/dummy/`
- Fixed worker.test.ts to use Pro package's package.json instead of root package.json
**Prevent Test Race Conditions**
- Converted testingNodeRendererConfigs.js to TypeScript
- Refactored config from static object to `createTestConfig(testName)` function
- Each test now gets a unique bundle cache path: `./tmp/node-renderer-bundles-test-{testName}`
- Prevents race conditions when tests run in parallel
**Files Changed**
- tests/vm.test.ts: Updated React 19 assertions
- tests/httpRequestUtils.ts: Fixed bundle paths
- tests/serverRenderRSCReactComponent.test.js: Fixed bundle paths
- tests/worker.test.ts: Fixed package.json path
- tests/testingNodeRendererConfigs.js → .ts: TypeScript conversion + race condition fix
- tests/concurrentHtmlStreaming.test.ts: Use new config function
- tests/htmlStreaming.test.js: Use new config function
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
WalkthroughReplaces a static test config export with a factory Changes
Sequence DiagramsequenceDiagram
participant Test as Test File
participant Factory as createTestConfig
participant FS as FileSystem
participant App as buildApp
rect rgb(240,240,255)
Note over Test,Factory: New flow — per-test config factory
Test->>Factory: createTestConfig('testName')
Factory->>FS: ensure unique bundlePath (rm old, mkdir)
Factory-->>Test: return { config, bundlePath }
Test->>App: buildApp(config)
App-->>Test: server instance
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
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)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review - PR #2110I've reviewed the changes in this PR. Overall, this is a well-structured fix that addresses multiple important issues in the Pro Node Renderer test suite. Here's my detailed feedback: ✅ Strengths1. Excellent React 19 Compatibility HandlingThe approach to handling the removal of
This is a sensible degradation strategy that maintains test value while adapting to React 19 changes. 2. Smart Race Condition PreventionThe refactoring of
3. Proper TypeScript MigrationThe conversion to TypeScript adds valuable type safety:
4. Correct Path FixesFixed paths from root
This aligns with the monorepo structure documented in CLAUDE.md. 🔍 Potential Issues & Suggestions1. Test Assertion Weakness (Minor)Location: expect(welcomePageRenderingResult as string).toContain('<');
expect((welcomePageRenderingResult as string).length).toBeGreaterThan(100);Issue: These assertions are quite weak compared to the previous checksum validation:
Suggestion: Consider adding more specific assertions: // Verify it's actual React HTML, not an error
expect(result).toContain('<div');
expect(result).not.toContain('Error');
// Or verify specific component markers if possible
expect(result).toContain('WelcomePage');Severity: Low - Current approach is acceptable for React 19 compatibility, but could be more robust. 2. Cleanup Documentation (Minor)Location: // Clean up any existing directory
if (fs.existsSync(bundlePath)) {
fs.rmSync(bundlePath, { recursive: true, force: true });
}Question: Is cleanup before each test sufficient, or should there also be cleanup after tests complete (e.g., in a Jest Suggestion: Consider documenting the cleanup strategy or adding an 3. Type Safety Enhancement (Trivial)Location: port: (env.RENDERER_PORT && parseInt(env.RENDERER_PORT, 10)) || 3800,Issue: If Suggestion: More explicit handling: port: env.RENDERER_PORT ? parseInt(env.RENDERER_PORT, 10) : 3800,Severity: Very low - unlikely to be an issue in practice, but technically incorrect. 4. Missing Changelog Entry?According to CLAUDE.md guidelines, this appears to be a user-visible bug fix (Pro tests not working with React 19). Should this have an entry in Recommendation: Add a changelog entry under "Bug Fixes" if this is merged to master. 🔒 Security Considerations✅ No security concerns identified. Changes are test-only and don't affect production code. ⚡ Performance Considerations✅ Positive impact: Unique bundle cache paths prevent race conditions, potentially improving test reliability and parallelization. The cleanup logic ( 🧪 Test CoverageStatus: ✅ Good The PR updates existing tests rather than adding new functionality, so coverage should remain consistent. The test changes themselves:
📋 Checklist for AuthorBefore merging, please verify:
🎯 RecommendationAPPROVE with minor suggestions ✅ This is a solid PR that addresses real issues. The suggestions above are minor improvements that could be addressed in a follow-up if needed. The core fixes are correct and well-implemented. Priority: The race condition fix and React 19 compatibility are important for test reliability, especially as more projects adopt React 19. Great work on identifying and fixing these issues! 🚀 🤖 Review generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react-on-rails-pro-node-renderer/tests/serverRenderRSCReactComponent.test.js (1)
27-27: Consider using unique temp directory per test to prevent race conditions.The hardcoded temp directory path could cause race conditions if tests run in parallel. According to the PR objectives, other test files were updated to use
createTestConfig(testName)which generates unique directories like./tmp/node-renderer-bundles-test-{testName}. Consider applying the same pattern here for consistency and safety.Apply this diff to use a unique test-specific directory:
- tempDir = path.join(process.cwd(), 'tmp/node-renderer-bundles-test/testing-bundle'); + tempDir = path.join(process.cwd(), 'tmp/node-renderer-bundles-test-serverRenderRSCReactComponent');Or better yet, if the
createTestConfigfunction is available, use it to maintain consistency with other tests that were updated in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/react-on-rails-pro-node-renderer/tests/concurrentHtmlStreaming.test.ts(1 hunks)packages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js(1 hunks)packages/react-on-rails-pro-node-renderer/tests/httpRequestUtils.ts(1 hunks)packages/react-on-rails-pro-node-renderer/tests/serverRenderRSCReactComponent.test.js(1 hunks)packages/react-on-rails-pro-node-renderer/tests/testingNodeRendererConfigs.js(0 hunks)packages/react-on-rails-pro-node-renderer/tests/testingNodeRendererConfigs.ts(1 hunks)packages/react-on-rails-pro-node-renderer/tests/vm.test.ts(11 hunks)packages/react-on-rails-pro-node-renderer/tests/worker.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react-on-rails-pro-node-renderer/tests/testingNodeRendererConfigs.js
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
packages/react-on-rails-pro-node-renderer/tests/vm.test.tspackages/react-on-rails-pro-node-renderer/tests/worker.test.tspackages/react-on-rails-pro-node-renderer/tests/serverRenderRSCReactComponent.test.js
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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-node-renderer/tests/vm.test.tspackages/react-on-rails-pro-node-renderer/tests/concurrentHtmlStreaming.test.ts
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
packages/react-on-rails-pro-node-renderer/tests/worker.test.ts
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
packages/react-on-rails-pro-node-renderer/tests/worker.test.tspackages/react-on-rails-pro-node-renderer/tests/serverRenderRSCReactComponent.test.jspackages/react-on-rails-pro-node-renderer/tests/httpRequestUtils.tspackages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
packages/react-on-rails-pro-node-renderer/tests/serverRenderRSCReactComponent.test.jspackages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
packages/react-on-rails-pro-node-renderer/tests/serverRenderRSCReactComponent.test.jspackages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
packages/react-on-rails-pro-node-renderer/tests/serverRenderRSCReactComponent.test.js
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
packages/react-on-rails-pro-node-renderer/tests/serverRenderRSCReactComponent.test.jspackages/react-on-rails-pro-node-renderer/tests/concurrentHtmlStreaming.test.ts
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
packages/react-on-rails-pro-node-renderer/tests/serverRenderRSCReactComponent.test.js
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
packages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js
🧬 Code graph analysis (4)
packages/react-on-rails-pro-node-renderer/tests/vm.test.ts (2)
packages/react-on-rails-pro-node-renderer/tests/helper.ts (1)
readRenderingRequest(137-145)packages/react-on-rails-pro-node-renderer/src/worker/vm.ts (1)
runInVM(110-179)
packages/react-on-rails-pro-node-renderer/tests/testingNodeRendererConfigs.ts (2)
packages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js (1)
createTestConfig(7-7)packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts (1)
Config(25-89)
packages/react-on-rails-pro-node-renderer/tests/concurrentHtmlStreaming.test.ts (2)
packages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js (3)
createTestConfig(7-7)app(8-8)app(24-24)packages/react-on-rails-pro-node-renderer/tests/testingNodeRendererConfigs.ts (1)
createTestConfig(13-49)
packages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js (1)
packages/react-on-rails-pro-node-renderer/tests/testingNodeRendererConfigs.ts (1)
createTestConfig(13-49)
🔇 Additional comments (10)
packages/react-on-rails-pro-node-renderer/tests/worker.test.ts (1)
6-6: LGTM! Path correction aligns with monorepo structure.The import path has been correctly updated to reference the package.json file in the react-on-rails-pro-node-renderer package directory.
packages/react-on-rails-pro-node-renderer/tests/httpRequestUtils.ts (1)
50-51: LGTM! Bundle paths correctly updated for monorepo structure.The paths now correctly point to the Pro package's dummy application test bundles in the monorepo.
packages/react-on-rails-pro-node-renderer/tests/vm.test.ts (3)
196-270: LGTM! React 19 compatibility changes appropriately implemented.The test now uses generic HTML validation instead of the removed
data-react-checksumattribute. While these assertions are less specific than before (checking for HTML content and minimum length), they appropriately verify that rendering succeeded without relying on React's internal implementation details.The updated assertion count (10) correctly reflects the new validation pattern of 2 checks per component.
272-315: LGTM! Consistent React 19 compatibility pattern.The same validation approach is applied here, with the assertion count (6) correctly matching 2 checks per component.
317-372: LGTM! BionicWorkshop tests updated consistently.The assertion count (8) correctly reflects 2 checks per component. The validation pattern is consistent with the other test updates in this file.
packages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js (1)
3-8: LGTM! Factory pattern prevents race conditions.The test now uses
createTestConfigto obtain a unique configuration with an isolated bundle cache directory (./tmp/node-renderer-bundles-test-htmlStreaming). This prevents race conditions when tests run in parallel.packages/react-on-rails-pro-node-renderer/tests/concurrentHtmlStreaming.test.ts (1)
8-12: LGTM! Consistent factory pattern implementation.The test correctly adopts the
createTestConfigfactory pattern, using a unique test identifier to ensure isolated bundle cache directories and prevent race conditions during parallel test execution.packages/react-on-rails-pro-node-renderer/tests/testingNodeRendererConfigs.ts (2)
13-49: LGTM! Well-designed factory pattern with proper isolation.The
createTestConfigfunction effectively prevents race conditions by:
- Creating unique bundle cache directories per test using the
testNameparameter- Cleaning up stale directories before each test run
- Returning both the config and bundlePath for flexibility
The configuration values are appropriate for the test environment, with sensible defaults and environment variable overrides for port and log level.
51-51: LGTM! Default export provides convenience.The default export allows tests to import the factory function using either named or default import syntax.
packages/react-on-rails-pro-node-renderer/tests/serverRenderRSCReactComponent.test.js (1)
31-31: Verify that the corrected bundle paths exist.The path changes align with the PR objective to fix bundle paths for the Pro dummy app in the monorepo. However, ensure that the files exist at the new locations.
Run the following script to verify the bundle paths:
Also applies to: 38-38
…gs.ts Prettier requires all files to end with a newline character. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code Review FeedbackThis PR makes important fixes for React 19 compatibility and test reliability. Overall the changes are well-structured and address real issues. Here's my detailed feedback: ✅ Strengths
🔍 Code Quality ObservationsReact 19 Test Assertions (vm.test.ts)Current approach: expect(welcomePageRenderingResult as string).toContain('<');
expect((welcomePageRenderingResult as string).length).toBeGreaterThan(100);Concern: These assertions are quite weak and could pass even if rendering fails partially. A result like Recommendation: Consider more robust assertions: // Check for component-specific HTML structure
expect(welcomePageRenderingResult as string).toContain('<div');
expect(welcomePageRenderingResult as string).not.toContain('error');
expect(welcomePageRenderingResult as string).not.toContain('undefined');
// Keep length check
expect((welcomePageRenderingResult as string).length).toBeGreaterThan(100);Or better yet, verify expected component content: // For WelcomePage, check for known content
expect(welcomePageRenderingResult as string).toContain('welcome'); // or whatever the component rendersType Safety in testingNodeRendererConfigs.tsLine 24: The type conversion is correct but verbose: port: (env.RENDERER_PORT && parseInt(env.RENDERER_PORT, 10)) || 3800,Suggestion: Could be simplified: port: env.RENDERER_PORT ? parseInt(env.RENDERER_PORT, 10) : 3800,This is clearer and avoids the truthy check on the string itself. 🐛 Potential Issues1. Missing Cleanup in TestsThe Recommendation: Add cleanup in test files: afterAll(() => {
const { bundlePath } = createTestConfig('concurrentHtmlStreaming');
if (fs.existsSync(bundlePath)) {
fs.rmSync(bundlePath, { recursive: true, force: true });
}
});Or better, export a cleanup function from 2. Test Assertion Count Changes Need VerificationThe assertion counts doubled (5→10, 3→6, 4→8) because you're now checking both presence AND length. However, I notice the PR description says "All Pro Node Renderer tests should now pass" but the actual testing status isn't confirmed. Critical Question: Have you actually run these tests locally to verify they pass? Per CLAUDE.md:
Action Required: Please confirm you've run: cd packages/react-on-rails-pro-node-renderer
yarn testAnd that all tests pass. 📁 Path Management ConcernsWhile the path changes look correct, per the repository's You should verify these paths actually exist in the expected structure: ls -la react_on_rails_pro/spec/dummy/ssr-generated/
ls -la react_on_rails_pro/spec/dummy/public/webpack/test/The
Is there a prerequisite build step documented? Should the test suite's README mention running webpack first? 🔒 SecurityNo security concerns. The changes are test-only and don't introduce vulnerabilities. ⚡ PerformanceThe race condition fix may slightly increase disk I/O (more directories created), but this is negligible and worth the reliability improvement. 📝 DocumentationThe JSDoc comment in
SummaryApprove with minor suggestions. The core fixes are sound, but please:
Great work identifying and fixing these issues! The factory pattern for test config is particularly elegant. |
Summary
Fixes multiple issues in the Pro Node Renderer test suite:
data-react-checksumattribute (removed in React 19)testingNodeRendererConfigs.jsto TypeScriptChanges
React 19 Compatibility (
vm.test.ts)data-react-checksumfrom server-rendered HTMLBundle Path Fixes
httpRequestUtils.ts: Fixed paths toreact_on_rails_pro/spec/dummy/serverRenderRSCReactComponent.test.js: Fixed paths to Pro dummy app bundlesworker.test.ts: Use Pro package'spackage.jsoninstead of rootRace Condition Prevention
testingNodeRendererConfigs.js→.tscreateTestConfig(testName)function./tmp/node-renderer-bundles-test-{testName}concurrentHtmlStreaming.test.tsandhtmlStreaming.test.jsto use new functionTest Plan
All Pro Node Renderer tests should now pass:
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.