fix(request-state): remove data race on early state load #179
Conversation
This patch fixes condition where client already hydated and expects server data from naostores but nanostores not populated with server data yet Signed-off-by: Максим Тарасов <[email protected]>
|
@MWGuy is attempting to deploy a commit to the Fryuni Pro Team on Vercel. A member of the Team first needs to authorize it. |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request modify the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/request-state/src/runtime/middleware.ts (1)
29-37: Consider adding error handling for missing head tag.While moving the injection point to
</head>fixes the race condition, we should handle cases where the head tag isn't found to maintain robustness.Consider this enhancement:
const headCloseIndex = chunk.indexOf('</head>'); if (headCloseIndex > -1) { const state = getState(); if (state) { const stateScript = `<script id="it-astro-state" type="application/json+devalue">${state}</script>`; chunk = chunk.slice(0, headCloseIndex) + stateScript + chunk.slice(headCloseIndex); } injected = true; +} else { + // Log warning if head tag not found in first chunk + console.warn('Warning: </head> tag not found in chunk, state injection may be delayed'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/request-state/src/runtime/middleware.ts(1 hunks)
🔇 Additional comments (1)
packages/request-state/src/runtime/middleware.ts (1)
29-30: LGTM! Moving state injection to head addresses the data race.
The change to inject state before </head> instead of </body> is a good solution to prevent the data race condition where client-side hydration occurs before state is available.
Let's verify the HTML structure assumptions:
Also applies to: 35-35
✅ Verification successful
Moving state injection to </head> is safe and well-tested
The implementation and test analysis confirms:
- All test fixtures have proper HTML structure with single
<head>tags - E2E tests verify state injection and hydration works correctly
- View transitions test confirms state persistence across page navigations
- The middleware correctly handles streaming HTML transformation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify HTML structure assumptions for state injection
# Test: Check if there are any HTML files with multiple </head> tags or missing </head>
# which could affect the state injection logic
# Search for multiple </head> tags in same file
echo "Checking for multiple </head> tags..."
rg -l "</head>.*</head>" -g "*.{html,astro}"
# Search for files with opening <head> but missing closing tag
echo "Checking for unclosed <head> tags..."
rg -l "<head>(?![\s\S]*</head>)" -g "*.{html,astro}"
Length of output: 503
Script:
#!/bin/bash
# Let's try a different approach using simpler patterns and PCRE2 mode
# First, let's find all HTML/Astro files
echo "Finding all HTML/Astro files..."
fd -e html -e astro
# Then check content of these files for HTML structure
echo "Checking HTML structure in template files..."
rg --pcre2 "<head>.*</head>" -l
# Also check for any potential template files that might generate HTML
echo "Checking other potential template files..."
fd -e jsx -e tsx -e vue -e svelte
# Look for head-related patterns in middleware tests
echo "Checking test files for head-related patterns..."
rg "head" -l packages/request-state/src/runtime/middleware.test.ts
Length of output: 5022
Script:
#!/bin/bash
# Let's check the actual test file location and its content
echo "Finding middleware test file..."
fd middleware.test
# Let's examine the actual middleware implementation to understand the HTML handling
echo "Checking middleware implementation..."
cat packages/request-state/src/runtime/middleware.ts
# Let's also check if there are any other tests that might verify HTML handling
echo "Checking for related test files..."
fd -e test.ts -e spec.ts -e test.js -e spec.js -p request-state
# Look for any HTML transformation tests in e2e tests
echo "Checking e2e tests..."
rg -l "head" packages/request-state/e2e
Length of output: 2068
Script:
#!/bin/bash
# Let's examine the e2e tests to understand how HTML transformation is tested
echo "Checking e2e test content..."
cat packages/request-state/e2e/basic.spec.ts
cat packages/request-state/e2e/dev.spec.ts
cat packages/request-state/e2e/view-transitions.spec.ts
# Let's also check the test fixtures to see the HTML structure
echo "Checking test fixture content..."
cat packages/request-state/e2e/fixture/basic/src/pages/index.astro
cat packages/request-state/e2e/fixture/dev/src/pages/index.astro
cat packages/request-state/e2e/fixture/view-transitions/src/pages/index.astro
Length of output: 5735
|
Hoping that @Fryuni merge this PR as fast as possible 🙏 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…t-state and request-nanostores packages
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
docs/src/content/docs/request-nanostores.mdx (2)
120-122: Improve grammar and clarity in the Caveats section.The current text has some grammatical issues. Consider this revision:
-Enabling request-nanostores disables [response streaming](https://docs.astro.build/en/recipes/streaming-improve-page-performance/). This behavior caused by [request-state](/request-state) package that prevents race condition where nanostores not initialized before client components starts hydrating. +Enabling request-nanostores disables [response streaming](https://docs.astro.build/en/recipes/streaming-improve-page-performance/). This behavior is caused by the [request-state](/request-state) package to prevent race conditions where nanostores are not initialized before client components start hydrating.
120-122: Consider expanding the Caveats section with additional details.The section would benefit from:
- Explaining the performance implications of disabling response streaming
- Providing examples of scenarios where this limitation might be significant
- Suggesting potential workarounds or best practices for performance-critical applications
Would you like me to help draft this additional content?
docs/src/content/docs/request-state.mdx (2)
133-135: Improve grammar in the Caveats sectionThe technical content accurately describes the streaming limitation. However, the text could be clearer with proper articles.
Apply this diff to improve readability:
-Enabling request-state disables [response streaming](https://docs.astro.build/en/recipes/streaming-improve-page-performance/). The entire application state must be generated before shipping closing head tag to the client. This behavior prevents data race when client components start hydrating before entire document is loaded. +Enabling request-state disables [response streaming](https://docs.astro.build/en/recipes/streaming-improve-page-performance/). The entire application state must be generated before shipping the closing head tag to the client. This behavior prevents data races when client components start hydrating before the entire document is loaded.🧰 Tools
🪛 LanguageTool
[uncategorized] ~135-~135: Possible missing article found.
Context: ...state must be generated before shipping closing head tag to the client. This behavior p...(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~135-~135: Possible missing article found.
Context: ...lient components start hydrating before entire document is loaded. ## License Astro ...(AI_HYDRA_LEO_MISSING_THE)
133-136: Consider adding more context about the data raceWhile the caveat section correctly explains the limitation, it would be helpful to provide more context about the data race scenario that this prevents.
Consider expanding the section with:
## Caveats Enabling request-state disables [response streaming](https://docs.astro.build/en/recipes/streaming-improve-page-performance/). The entire application state must be generated before shipping the closing head tag to the client. This behavior prevents data races when client components start hydrating before the entire document is loaded. + +This limitation is necessary because client-side hydration could begin as soon as the component's script is loaded, potentially before the server state is available in nanostores. By injecting the state in the head section, we ensure that all required data is available before any client-side hydration occurs.🧰 Tools
🪛 LanguageTool
[uncategorized] ~135-~135: Possible missing article found.
Context: ...state must be generated before shipping closing head tag to the client. This behavior p...(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~135-~135: Possible missing article found.
Context: ...lient components start hydrating before entire document is loaded. ## License Astro ...(AI_HYDRA_LEO_MISSING_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
docs/src/content/docs/request-nanostores.mdx(1 hunks)docs/src/content/docs/request-state.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/content/docs/request-state.mdx
[uncategorized] ~135-~135: Possible missing article found.
Context: ...state must be generated before shipping closing head tag to the client. This behavior p...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~135-~135: Possible missing article found.
Context: ...lient components start hydrating before entire document is loaded. ## License Astro ...
(AI_HYDRA_LEO_MISSING_THE)
|
Hey @MWGuy, sorry for taking so long to get back to this. Without having to worry about streaming, the code looks a little different (it runs after the entire response is done instead of as part of the response stream), that was the missing part. Gonna merge this now! Thanks for the PR and for the patience 😄 |
Fryuni
changed the title
The problem
Sometimes i got condition where client ready to hydate conponents and expects server data loaded to nanostores in context but nanostores not populated with server data yet because document not yet downloaded from server.
1115.mp4
Solution
This patch fixes middleware logic that inserts hydration data into end of head rather than end of body with dynamic scripts imports.
Summary by CodeRabbit
New Features
</head>tag, enhancing the structure of the response.Bug Fixes
Documentation
request-nanostoresand@inox-tools/request-state, highlighting limitations regarding response streaming and state generation. Adjustments made for clarity and coherence.