Add Async Props documentation with animated SVG diagrams#2265
Add Async Props documentation with animated SVG diagrams#2265AbanoubGhadban wants to merge 58 commits intoupcoming-v16.3.0from
Conversation
- Introduced `IncrementalRenderRequestManager` to handle streaming NDJSON requests, managing state and processing of incremental render requests. - Added `validateBundlesExist` utility function to check for the existence of required bundles, improving error handling for missing assets. - Refactored the incremental render endpoint to utilize the new request manager, enhancing the response lifecycle and error management. - Updated tests to cover scenarios for missing bundles and validate the new request handling logic.
- Replaced the `IncrementalRenderRequestManager` with `handleIncrementalRenderStream` to manage streaming NDJSON requests more efficiently. - Enhanced error handling and validation during the rendering process. - Updated the `run` function to utilize the new stream handler, improving the response lifecycle and overall performance. - Removed the deprecated `IncrementalRenderRequestManager` class to streamline the codebase.
- Introduced improved error handling for malformed JSON chunks during the incremental rendering process. - Added logging and reporting for errors in subsequent chunks while allowing processing to continue. - Updated tests to verify behavior for malformed JSON in both initial and update chunks, ensuring robust error management.
…inability - Introduced helper functions to reduce redundancy in test setup, including `getServerAddress`, `createHttpRequest`, and `createInitialObject`. - Streamlined the handling of HTTP requests and responses in tests, enhancing clarity and organization. - Updated tests to utilize new helper functions, ensuring consistent structure and easier future modifications.
- Replaced inline wait functions with a new `waitFor` utility to improve test reliability and readability. - Updated tests to utilize `waitFor` for asynchronous expectations, ensuring proper handling of processing times. - Simplified the test structure by removing redundant wait logic, enhancing maintainability.
…processing - Introduced `createBasicTestSetup` and `createStreamingTestSetup` helper functions to streamline test initialization and improve readability. - Added `sendChunksAndWaitForProcessing` to handle chunk sending and processing verification, reducing redundancy in test logic. - Updated existing tests to utilize these new helpers, enhancing maintainability and clarity in the test structure.
- Added detailed error reporting in the `waitFor` function to include the last encountered error message when a timeout occurs. - Refactored the `createStreamingResponsePromise` function to improve clarity and maintainability by renaming variables and returning received chunks alongside the promise. - Updated tests to utilize the new structure, ensuring robust handling of streaming responses and error scenarios.
…ment - Removed unnecessary bundle validation checks from the incremental render request flow. - Enhanced the `handleIncrementalRenderRequest` function to directly call `handleRenderRequest`, streamlining the rendering process. - Updated the `IncrementalRenderInitialRequest` type to support a more flexible structure for dependency timestamps. - Improved error handling to capture unexpected errors during the rendering process, ensuring robust responses. - Added cleanup logic in tests to restore mocks after each test case.
- Removed individual protocol version and authentication checks from the request handling flow. - Introduced a new `performRequestPrechecks` function to streamline the validation process for incoming requests. - Updated the `authenticate` and `checkProtocolVersion` functions to accept request bodies directly, enhancing modularity. - Improved error handling by ensuring consistent response structures across precheck validations.
- Updated the `/upload-assets` endpoint to differentiate between assets and bundles, allowing for more flexible uploads. - Introduced logic to extract bundles prefixed with 'bundle_' and handle them separately. - Integrated the `handleNewBundlesProvided` function to manage the processing of new bundles. - Added comprehensive tests to verify the correct handling of uploads with various combinations of assets and bundles, including edge cases for empty requests and duplicate bundle hashes.
- Added tests to verify directory structure and file presence for uploaded bundles and assets. - Implemented checks for scenarios with empty requests and duplicate bundle hashes, ensuring correct behavior without overwriting existing files. - Improved coverage of the `/upload-assets` endpoint to handle various edge cases effectively.
- Implemented a new test case for the `/upload-assets` endpoint to verify that bundles are correctly placed in their own hash directories rather than the targetBundles directory. - Ensured that the test checks for the existence of the bundle in the appropriate directory and confirms that the target bundle directory remains empty, enhancing coverage for asset upload scenarios.
- Implemented a suite of tests for the `/bundles/:bundleTimestamp/incremental-render/:renderRequestDigest` endpoint to verify successful rendering under various conditions, including pre-uploaded bundles and assets. - Added scenarios to test failure cases, such as missing bundles, incorrect passwords, and invalid JSON payloads. - Enhanced coverage for handling multiple dependency bundles and processing NDJSON chunks, ensuring robust error management and response validation.
- Simplified test structure by introducing helper functions to reduce code duplication for creating worker apps and uploading bundles. - Improved test cases for the `/bundles/:bundleTimestamp/incremental-render/:renderRequestDigest` endpoint, ensuring robust validation of successful renders and error handling for various scenarios. - Added tests for handling invalid JSON and missing required fields, enhancing coverage for edge cases in the rendering process. - Updated tests to ensure proper handling of multiple dependency bundles and improved response validation for different payload conditions.
- Replaced the `runInVM` function with a new `ExecutionContext` class to manage VM contexts more effectively. - Updated the `handleRenderRequest` function to utilize the new `ExecutionContext`, improving the handling of rendering requests. - Enhanced error management by introducing `VMContextNotFoundError` for better clarity when VM contexts are missing. - Refactored tests to align with the new execution context structure, ensuring consistent behavior across rendering scenarios.
…andling - Updated the parameters for the `runOnOtherBundle` function to ensure correct execution order. - Introduced a reference to `globalThis.runOnOtherBundle` in the server rendering code for better accessibility. - Enhanced the test fixture to align with the changes in the global context, ensuring consistent behavior across rendering requests.
- Introduced `IncrementalRenderSink` type to manage streaming updates more effectively. - Updated `handleIncrementalRenderRequest` to return an optional sink and handle execution context errors gracefully. - Refactored the `run` function to utilize the new sink for processing updates, enhancing error logging for unexpected chunks. - Simplified test setup by removing unused sink methods, ensuring tests focus on relevant functionality.
- Updated the `setResponse` call in the `run` function to correctly use `result.response`. - Expanded the incremental render tests to cover new scenarios, including basic updates, multi-bundle interactions, and error handling for malformed update chunks. - Introduced new helper functions in test fixtures to streamline the creation of async values and streams, enhancing the robustness of the tests. - Improved the secondary bundle's functionality to support async value resolution and streaming, ensuring consistent behavior across bundles.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| normalized_value = raw_value.strip.downcase | ||
| return PRERENDER_OVERRIDE_VALUES[normalized_value] if PRERENDER_OVERRIDE_VALUES.key?(normalized_value) | ||
|
|
||
| Rails.logger.warn( |
There was a problem hiding this comment.
Rails.logger.warn is called while PRERENDER_OVERRIDE_CACHE_MUTEX is held. Rails logger is thread-safe in isolation, but calling any external logger inside a mutex can cause a deadlock if the logger itself ever acquires a lock that another thread holds while also waiting for PRERENDER_OVERRIDE_CACHE_MUTEX.
In practice this is very unlikely here since the mutex is only held briefly and Rails' logger doesn't call back into prerender_env_override. But the risk can be eliminated entirely by extracting the warn call to outside the synchronize block (build the log message inside, log it outside). Given the value is cached after the first parse, this is a one-time cost anyway.
| @@ -0,0 +1,123 @@ | |||
| # Async Props: Streaming Server-Side Rendering | |||
There was a problem hiding this comment.
Async Props is a React on Rails Pro feature requiring the Node renderer, but this documentation lives in docs/async-props/ (alongside OSS docs) rather than docs/pro/. This could confuse OSS users who try to use this feature.
Consider either:
- Moving to
docs/pro/async-props/ - Adding a prominent Pro-only notice at the top: "
⚠️ This feature requires React on Rails Pro."
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
test |
ReviewOverall: The PR bundles async props documentation (SVGs, 4 doc pages) with several unrelated runtime changes: a breaking upload protocol migration, an Breaking changesTwo breaking changes are documented in the CHANGELOG but worth flagging in review:
Security: path traversal in extractBundlesAndAssetsThe Minor
|
|
Test comment posting |
|
Review: Async Props Documentation - please see full review in separate comment |
|
Review: Async Props Documentation Thanks for the documentation effort. However, there are several accuracy issues that need to be addressed before this can merge. CRITICAL: Fictional API Surface 1. useAsyncProp hook does not exist. 2. async_prop(timeout:, on_error:) options are shown as working code but are not implemented. 3. Several Rails config options in api-reference.md do not exist in ReactOnRailsPro::Configuration:
Verified against react_on_rails_pro/lib/react_on_rails_pro/configuration.rb. Minor Issues 4. Animated SVGs will not render on GitHub. 5. PR scope does not match the title. Whats Good
|
PR ReviewScope / Title Mismatch The title says Add Async Props documentation, but this PR bundles several distinct changes: new async-props docs with animated SVGs, a path-traversal security fix in validateBundleTimestamp, a breaking /upload-assets protocol change (bundle_ keys replace targetBundles), an async gem version bump from >=2.6 to >=2.29, moving copyUploadedAssets outside the per-bundle lock, Async::Variable to Async::Promise migration, master-to-main renames across 20+ files, and a version bump to 16.5.0. Consider splitting out the documentation as a standalone PR. Security - validateBundleTimestamp: Good fix. The regex blocks path separators and traversal sequences. Test coverage is present and correct. Minor: getRequestBundleFilePath calls validateBundleTimestamp twice on the same input (once inside getBundleDirectory and once directly). The second call is redundant. Protocol Change - /upload-assets (breaking): The endpoint now rejects requests lacking a bundle_ field. The Ruby side still sends targetBundles for backward compat, but older node renderer versions will get a 400 because they never sent bundle_ keys. The CHANGELOG and TODO in request.rb note this. Please verify the migration docs make clear that gem and node renderer must be upgraded atomically. Async::Variable to Async::Promise - logic is correct: The resolved? + reject sequence in consumer_stream_async is safe under cooperative scheduling since no fiber switch occurs between the two calls. The contract canary in promise_spec.rb is a nice addition. Eager-loading require async/promise at module load level is the right call. copyUploadedAssets moved outside the lock: Asset copy now runs after unlock, so two concurrent workers targeting the same bundle directory can copy assets simultaneously. The comment explains this is intentional (overwrite: true, idempotent). That holds for identical files. If two workers upload different assets with the same filename (unlikely but possible), the last write wins silently. Worth confirming this invariant is acceptable. Documentation quality: The async-props docs are thorough and the animated SVGs are a clear win. The performance table (1800ms to 50ms TTFB/TTI) uses specific numbers that users might treat as benchmarks - consider noting these are illustrative. SMIL animations render in GitHub web UI but not VS Code preview; the test plan GIF conversion item is worth completing before merge. |
| return path.join(bundleDirectory, filename); | ||
| } | ||
|
|
||
| const SAFE_BUNDLE_TIMESTAMP_PATTERN = /^(?!\.{1,2}$)[A-Za-z0-9_.-]+$/; |
There was a problem hiding this comment.
The regex is correct for preventing path traversal — no path separators are allowed so no component can escape serverBundleCachePath. One note: the negative lookahead only rejects the strings "." and ".." exactly. Values like "..foo" pass the check, which is fine (they are safe) but unusual for a timestamp. If you want to be stricter you could anchor the first character: /^[A-Za-z0-9][A-Za-z0-9_.-]*$/, which also removes the need for the lookahead.
| export function getRequestBundleFilePath(bundleTimestamp: string | number) { | ||
| const bundleDirectory = getBundleDirectory(bundleTimestamp); | ||
| return path.join(bundleDirectory, `${bundleTimestamp}.js`); | ||
| const safeBundleTimestamp = validateBundleTimestamp(bundleTimestamp); | ||
| return path.join(bundleDirectory, `${safeBundleTimestamp}.js`); |
There was a problem hiding this comment.
validateBundleTimestamp is called twice here: once inside getBundleDirectory (line 189) and once directly on line 190. Since getBundleDirectory already validates and returns the safe string, you can reuse its return value:
| export function getRequestBundleFilePath(bundleTimestamp: string | number) { | |
| const bundleDirectory = getBundleDirectory(bundleTimestamp); | |
| return path.join(bundleDirectory, `${bundleTimestamp}.js`); | |
| const safeBundleTimestamp = validateBundleTimestamp(bundleTimestamp); | |
| return path.join(bundleDirectory, `${safeBundleTimestamp}.js`); | |
| export function getRequestBundleFilePath(bundleTimestamp: string | number) { | |
| const bundleDirectory = getBundleDirectory(bundleTimestamp); | |
| const safeBundleTimestamp = bundleDirectory.slice(bundleDirectory.lastIndexOf('/') + 1); | |
| return path.join(bundleDirectory, `${safeBundleTimestamp}.js`); |
Or more simply, just accept that getBundleDirectory validates and derive the filename from the validated value returned by the first call rather than re-validating.
| // Always copy assets to the bundle directory — even if the bundle was | ||
| // already present (e.g., from a prior upload or another worker). | ||
| // copyUploadedAssets uses overwrite:true, so this is idempotent. | ||
| if (assetsToCopy) { | ||
| await copyUploadedAssets(assetsToCopy, bundleDirectory); | ||
| } |
There was a problem hiding this comment.
Moving asset copy outside the lock is intentional (so assets are always applied even when the bundle was already present), and overwrite: true makes it safe for the normal case. One edge case to confirm: two concurrent workers uploading different assets under the same filename to the same bundle directory will race silently — the last writer wins. This is an acceptable invariant if assets for a given bundle are always content-identical across uploads, but if that assumption can ever be violated a log.info or log.warn noting the overwrite would help with debugging.
| # TODO: targetBundles is only kept for backward compatibility with older node renderers | ||
| # (protocol 2.0.0) that require it. The new node renderer derives target directories from | ||
| # the bundle_<hash> form keys and ignores this field. Remove at the next breaking version. | ||
| # Note: it's not mandatory to keep this until then — users are expected to upgrade the | ||
| # node renderer and react_on_rails gem to the same version together — but it's an easy | ||
| # backward compatibility safeguard. | ||
| form["targetBundles"] = target_bundles |
There was a problem hiding this comment.
The backward-compat comment is clear, but note the asymmetry: the Ruby gem sends both targetBundles (for old renderers) and the new bundle_<hash> fields (for the new renderer). An old node renderer that sees bundle_<hash> keys will treat them as unrecognised form fields and silently ignore them while reading targetBundles as before — so the Ruby-new / renderer-old direction is safe. The reverse (Ruby-old / renderer-new) is the breaking direction: old Ruby sends only targetBundles, new renderer rejects with 400. The atomic-upgrade requirement is therefore one-directional: renderer can be upgraded first, gem second, but not gem first. Worth documenting this order explicitly if it is not already in the upgrade guide.
PR ReviewThis PR has a misleading title. The title says "Add Async Props documentation with animated SVG diagrams" but the actual changes are much broader - this is a v16.5.0 release PR containing:
The PR title/description should accurately reflect this scope. |
| config_path = "config/initializers/react_on_rails_pro.rb" | ||
| return false unless File.exist?(config_path) | ||
|
|
||
| File.read(config_path).match?(/server_renderer\s*=\s*["']NodeRenderer["']/) |
There was a problem hiding this comment.
The regex /server_renderer\s*=\s*["']NodeRenderer["']/ will match commented-out lines (e.g., # config.server_renderer = "NodeRenderer"), producing a false positive. Use a negative lookbehind or filter to commented lines first:
| File.read(config_path).match?(/server_renderer\s*=\s*["']NodeRenderer["']/) | |
| File.read(config_path).each_line.any? do |line| | |
| !line.match?(/^\s*#/) && line.match?(/server_renderer\s*=\s*["']NodeRenderer["']/) | |
| end |
| # so `import ... from 'react-on-rails'` resolves silently — loading the base package instead | ||
| # of Pro. Components registered through the base package won't have Pro features (streaming, | ||
| # caching, RSC), and may cause "component not registered" errors at runtime. | ||
| BASE_PACKAGE_IMPORT_PATTERN = %r{\bfrom\s+['"]react-on-rails(?:/[^'"]*)?['"]} |
There was a problem hiding this comment.
This pattern will match import statements inside single-line JS comments (// import ReactOnRails from 'react-on-rails'), generating false-positive warnings. The file-level check (content.match?) has no way to distinguish commented imports from live ones. Consider using a line-by-line filter that skips lines starting with // or * before running the regex.
| # Returns true if environment was loaded successfully, false otherwise. | ||
| def ensure_rails_environment_loaded | ||
| return true if @rails_environment_loaded | ||
| return false if @rails_environment_attempted |
There was a problem hiding this comment.
@rails_environment_attempted is used here but never initialized in initialize (unlike @rails_environment_loaded = false). Ruby treats uninitialized ivar as nil (falsy), so the guard works, but it's inconsistent. Add @rails_environment_attempted = false to initialize for symmetry.
|
Substantive issues 1. pro_initializer_has_node_renderer? matches commented-out config - The regex matches commented-out lines like a disabled config.server_renderer assignment, giving a false positive where check_server_rendering_engine reports Pro uses NodeRenderer even when it is not active. 2. BASE_PACKAGE_IMPORT_PATTERN matches imports inside JS comments - The pattern fires on single-line JS comment import statements, generating spurious warnings for intentionally-commented migration notes in JS/TS files. 3. @rails_environment_attempted not initialized in initialize - @rails_environment_loaded is set to false in initialize, but @rails_environment_attempted is not. Ruby nil is falsy so the guard works, but the inconsistency could confuse future contributors. 4. asset_exists_on_vm_renderer? uses old targetBundles without a compat comment - request.rb line 170 sets form_data targetBundles for the /asset-exists endpoint with no explanation of its compatibility status relative to the new bundle_hash protocol. The upload_assets method has a TODO tracking this; this endpoint does not. Minor observations - docs/async-props/ references Pro streaming APIs and RSC heavily but lives in the OSS docs/ tree with no Pro-only callout. The animated SVGs use SMIL, which the PR own test plan flags as potentially incompatible with GitHub rendering. |
| # the bundle_<hash> form keys and ignores this field. Remove at the next breaking version. | ||
| # Note: it's not mandatory to keep this until then — users are expected to upgrade the | ||
| # node renderer and react_on_rails gem to the same version together — but it's an easy | ||
| # backward compatibility safeguard. | ||
| form["targetBundles"] = target_bundles | ||
|
|
There was a problem hiding this comment.
This still sends targetBundles to the node renderer with no comment explaining its compatibility status relative to the new bundle_<hash> protocol. The upload_assets method (line 148–154) has a TODO tracking backward compatibility, but this endpoint does not. Please add a matching comment here so it doesn't get missed when the deprecated field is eventually removed.
AbanoubGhadban
force-pushed
the
upcoming-v16.3.0
branch
from
2c96039 to
c186056
Compare
2 participants
Summary
Documentation Structure
Test plan
🤖 Generated with Claude Code