feat: add fetch cache tags support and refactor for cognitive complexity#154
Merged
feat: add fetch cache tags support and refactor for cognitive complexity#154
Conversation
- Update clap from 4.5.60 to 4.6.0 - Update tokio-tungstenite and tungstenite from 0.28.0 to 0.29.0 - Update image from 0.25.9 to 0.25.10 - Update tracing-subscriber from 0.3.22 to 0.3.23 - Update tempfile from 3.26.0 to 3.27.0 - Update @antfu/eslint-config from 7.7.2 to 7.7.3 - Update @typescript-eslint/utils from 8.57.0 to 8.57.1 - Update eslint-plugin-oxlint from 1.55.0 to 1.56.0 - Update knip from 5.86.0 to 5.87.0 - Update @sentry/react from 10.43.0 to 10.44.0 - Update @typescript/native-preview to 7.0.0-dev.20260317.1 - Update vite-plus from 0.1.11 to 0.1.12 - Add oxlint disable comment for nextTick function in init_node.js
- Rename REACT_INIT_SCRIPT to STREAMING_REACT_SETUP_SCRIPT for clarity - Rename react_init.js to streaming_react_setup.js to match constant naming - Extract Railway deployment logic into focused helper functions (updatePackageJsonForRailway, createRailwayToml, updateGitignoreForRailway, updateReadmeForRailway, printRailwaySuccessMessage) - Extract Render deployment logic into focused helper functions (updatePackageJsonForRender, createRenderYaml, updateGitignoreForRender, updateReadmeForRender, printRenderSuccessMessage) - Improve code organization by separating concerns and reducing function complexity - Update all references to use new constant and file names across renderer and deployment modules
📝 WalkthroughWalkthroughThis PR removes legacy React global shims, streaming and module-reload runtime extensions, and HMR stats; centralizes many resolvers/utilities; adds tag-aware fetch caching and proxy result processing; refactors router, deploy, Vite/image scanning, and proxy subsystems; and updates multiple dependency versions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
packages/deploy/src/railway.ts (1)
7-44: 🧹 Nitpick | 🔵 TrivialConsider extracting shared deployment logic to reduce duplication.
updatePackageJsonForRailwayis nearly identical toupdatePackageJsonForRenderinrender.ts. The same applies tocreateRailwayToml/createRenderYaml,updateGitignoreForRailway/updateGitignoreForRender, etc.Consider extracting shared logic into parameterized helpers in
utils.ts:// Example shared helper signature function updatePackageJsonForProvider(cwd: string, options: { providerName: string deployScript: string }) { ... }This would reduce maintenance burden and ensure consistent behavior across providers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/deploy/src/railway.ts` around lines 7 - 44, The two functions updatePackageJsonForRailway and updatePackageJsonForRender (and the parallel createRailwayToml/createRenderYaml and updateGitignoreForRailway/updateGitignoreForRender) duplicate deployment logic; extract the shared behavior into parameterized helpers in utils.ts (e.g., updatePackageJsonForProvider(cwd, { providerName, deployScript, startScript, dependency })) and refactor each provider-specific function to call these helpers, moving common steps such as reading/writing package.json, backing up existing start script, adding scripts/dependencies, and ensureMinimumNodeEngine into the shared helpers to eliminate duplication and keep provider files thin.packages/rari/src/proxy/runtime-executor.ts (1)
167-171:⚠️ Potential issue | 🟡 MinorFire-and-forget
Promise.allSettledhas unreachable.catch().Same issue as in
execute-proxy.ts.Promise.allSettlednever rejects.Proposed fix
if (waitUntilPromises.length > 0) { - Promise.allSettled(waitUntilPromises).catch((error) => { - console.error('[rari] Proxy: waitUntil promise failed:', error) - }) + Promise.allSettled(waitUntilPromises).then((results) => { + for (const result of results) { + if (result.status === 'rejected') { + console.error('[rari] Proxy: waitUntil promise failed:', result.reason) + } + } + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/proxy/runtime-executor.ts` around lines 167 - 171, The code calls Promise.allSettled(waitUntilPromises).catch(...), but allSettled never rejects so the .catch is unreachable; update the logic in runtime-executor.ts to remove the .catch and instead handle outcomes by calling Promise.allSettled(waitUntilPromises).then(results => { inspect results for status === 'rejected' and log errors }), referencing the waitUntilPromises variable so any rejected promise reasons are logged (or otherwise handled) inside the .then callback.packages/rari/src/proxy/execute-proxy.ts (1)
168-172:⚠️ Potential issue | 🟡 MinorFire-and-forget
Promise.allSettledhas unreachable.catch().
Promise.allSettlednever rejects—it always fulfills with an array of settlement objects. The.catch()handler will never execute.Proposed fix - remove unreachable catch or handle individual rejections
if (waitUntilPromises.length > 0) { - Promise.allSettled(waitUntilPromises).catch((error) => { - console.error('[rari] Proxy: waitUntil promise failed:', error) - }) + Promise.allSettled(waitUntilPromises).then((results) => { + for (const result of results) { + if (result.status === 'rejected') { + console.error('[rari] Proxy: waitUntil promise failed:', result.reason) + } + } + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/proxy/execute-proxy.ts` around lines 168 - 172, The call to Promise.allSettled inside the execute-proxy logic uses a .catch on a promise that never rejects; remove the unreachable .catch and either (A) simply await or call Promise.allSettled(waitUntilPromises) without .catch, or (B) handle individual failures by inspecting the returned array of settlement objects and logging any entries with status === 'rejected'; update the code in execute-proxy.ts where waitUntilPromises is processed (look for the Promise.allSettled(...) call) to implement one of these approaches and ensure rejected promises are logged or handled appropriately.crates/rari/src/runtime/ops.rs (1)
292-300: 🧹 Nitpick | 🔵 TrivialReturn
tagsconsistently fromop_fetch_with_cache.Line 298 adds
tagsonly in one branch. Keep the response shape stable by includingtagsin all branches (empty array when unavailable).Diff suggestion
Ok(serde_json::json!({ "ok": true, "status": result.status, "statusText": http_status_text(result.status), "body": body_str, "headers": headers_obj, "cached": result.was_cached, "tags": result.tags })) } Err(e) => { error!("Fetch failed for {}: {}", url, e); Ok(serde_json::json!({ "ok": false, "status": 500, "statusText": "Internal Server Error", "error": e.to_string(), - "cached": false + "cached": false, + "tags": [] })) } } } else { match perform_simple_fetch(&url, &options).await { Ok((status, body, headers)) => Ok(serde_json::json!({ "ok": (200..300).contains(&status), "status": status, "statusText": http_status_text(status), "body": body, "headers": headers, - "cached": false + "cached": false, + "tags": [] })), Err(e) => { error!("Fetch failed for {}: {}", url, e); Ok(serde_json::json!({ "ok": false, "status": 500, "statusText": "Internal Server Error", "error": e, - "cached": false + "cached": false, + "tags": [] })) } } }Also applies to: 304-331
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/runtime/ops.rs` around lines 292 - 300, The JSON response from op_fetch_with_cache currently includes "tags" only in one branch (using result.tags) which makes the response shape inconsistent; update all branches of the op_fetch_with_cache response (including the alternate branches around lines 304-331) to always emit a "tags" field—use result.tags when available and an empty array (e.g., Vec::new() / [] equivalent) when not—to ensure a stable response shape across all Ok(...) and Err/... return paths.crates/rari/src/server/middleware/request_context.rs (1)
117-131:⚠️ Potential issue | 🟠 MajorTags are dropped on cache hits, so tag coverage becomes incomplete.
On Line 122-131, a valid cached entry is returned immediately. The new tags parsed on Line 117-118 are only applied on Line 166-168 (fresh fetch path), so later requests with additional tags never update the cached entry.
Diff suggestion
{ let mut cache = self.fetch_cache.lock(); if let Some(cached) = cache.get(&cache_key) { let ttl_ms = options.get("cacheTTLMs").and_then(|t| t.parse::<u64>().ok()).unwrap_or(60_000); let elapsed_ms = cached.cached_at.elapsed().as_millis(); if elapsed_ms < ttl_ms as u128 { let mut result = cached.clone(); + if !tags.is_empty() { + for tag in &tags { + if !result.tags.contains(tag) { + result.tags.push(tag.clone()); + } + } + cache.put(cache_key.clone(), result.clone()); + } result.was_cached = true; return Ok(result); } cache.pop(&cache_key); } }Also applies to: 166-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/server/middleware/request_context.rs` around lines 117 - 131, Cached responses returned from the fetch_cache currently ignore newly-parsed tags (variable tags) so tag coverage never grows; update the cache-hit path in the request context (where fetch_cache.lock(), cache.get(&cache_key), cached.clone() and result.was_cached are used) to merge/union the newly-parsed tags into the cached entry before returning, and likewise ensure the fresh-fetch path (the code that applies tags on reply) also unions incoming tags with stored tags so cache entries are updated with any new tags instead of dropping them.packages/rari/src/router/robots-generator.ts (1)
197-204:⚠️ Potential issue | 🟡 MinorValidate that the dynamic robots file has a default export.
If the dynamic robots file lacks a default export,
module.defaultwill beundefined, causinggenerateRobotsTxtto fail with an unclear error. Add explicit validation.🛡️ Proposed fix
const module = await import(dataUrl) + if (module.default === undefined) { + throw new Error(`Robots file at ${robotsFile.path} must have a default export`) + } + let robotsData: Robots if (typeof module.default === 'function') { const robotsResult = module.default() robotsData = robotsResult instanceof Promise ? await robotsResult : robotsResult } else { robotsData = module.default }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/robots-generator.ts` around lines 197 - 204, Check that the imported dynamic robots module actually has a default export before using it: in the robots-generator logic that assigns robotsData from module.default, add an explicit validation that module && module.default !== undefined; if missing, throw a clear error (or return a rejected Promise) explaining the robots file must export a default (either an object or a function). Keep the existing handling for a function export (call it and await if it returns a Promise) and for a non-function default (assign directly to robotsData), but ensure you surface a descriptive error immediately when module.default is absent so generateRobotsTxt doesn't fail with an unclear message.packages/rari/src/image/Image.tsx (2)
97-129: 🧹 Nitpick | 🔵 Trivial
useImageLazyLoadis currently a no-op for network deferral.The observer never gates
src/srcSet; requests are still initiated immediately by render. This adds complexity without changing lazy-loading behavior. Either remove the hook and rely on nativeloading="lazy", or have it return visibility state and bindsrc/srcSetonly after intersection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/image/Image.tsx` around lines 97 - 129, The useImageLazyLoad hook currently observes but doesn't prevent network requests; change it to return a visibility boolean (e.g., isVisible) that becomes true when the IntersectionObserver reports intersection and remains true thereafter, ensure it respects shouldPreload, unoptimized, and loading === 'eager' (return true immediately in those cases), clean up the observer on unmount, and update the Image component to only set img src/srcSet when isVisible is true (or when preload/unoptimized/eager require immediate load) so requests are deferred until visible.
299-314:⚠️ Potential issue | 🟠 MajorGuard AVIF/WEBP
<source>emission whenloaderis set.
buildSrcSetStringcannot encodeformatthroughloader, so Line 301-Line 314 can emittype="image/avif"/type="image/webp"sources that are not truly format-specific. This can lead to incorrect source selection behavior.💡 Proposed fix
function OptimizedImage({ @@ }) { const defaultWidth = imgWidth || 1920 const sizesArray = imgWidth ? [imgWidth] : DEFAULT_DEVICE_SIZES + const canEmitFormatSources = !loader @@ return ( <picture ref={pictureRef}> - {DEFAULT_FORMATS.includes('avif') && ( + {canEmitFormatSources && DEFAULT_FORMATS.includes('avif') && ( <source type="image/avif" srcSet={buildSrcSetString(sizesArray, finalSrc, quality, 'avif', loader)} sizes={sizes} /> )} - {DEFAULT_FORMATS.includes('webp') && ( + {canEmitFormatSources && DEFAULT_FORMATS.includes('webp') && ( <source type="image/webp" srcSet={buildSrcSetString(sizesArray, finalSrc, quality, 'webp', loader)} sizes={sizes} /> )} {imgElement} </picture> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/image/Image.tsx` around lines 299 - 314, The AVIF/WEBP <source> elements are emitted even when a custom loader is provided (loader), but buildSrcSetString cannot force format through a loader; update the conditional around the DEFAULT_FORMATS checks in the JSX (where pictureRef is used and buildSrcSetString is called with sizesArray, finalSrc, quality, 'avif'/'webp', loader) to only render those <source> tags when no custom loader is set (e.g., loader is null/undefined) or when the loader explicitly supports a format query; in short, add a guard like "&& !loader" (or use a capability flag on the loader) to the DEFAULT_FORMATS.includes('avif') and DEFAULT_FORMATS.includes('webp') conditions so format-specific sources are only emitted when buildSrcSetString can actually produce that format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/deploy/src/railway.ts`:
- Around line 46-60: The function createRailwayDeployment is marked async but
contains no await calls (it only invokes updatePackageJsonForRailway,
createRailwayToml, updateGitignoreForRailway, updateReadmeForRailway, and
printRailwaySuccessMessage), so remove the unnecessary async keyword from the
createRailwayDeployment declaration to make it a regular synchronous function;
alternatively, if any of those helper functions are meant to be async, update
the calls to await them and keep async—prefer removing async here for the
current synchronous calls.
In `@packages/deploy/src/render.ts`:
- Around line 46-60: The function createRenderDeployment is marked async but
contains no await or asynchronous work; remove the async keyword from its
declaration to avoid misleading callers (or alternatively convert helper
functions like updatePackageJsonForRender, createRenderYaml,
updateGitignoreForRender, updateReadmeForRender and printRenderSuccessMessage to
return Promises and use await inside createRenderDeployment). Locate
createRenderDeployment and either drop the async modifier or update the
referenced helper functions to async equivalents and add awaits accordingly so
the function's signature matches its implementation.
In `@packages/rari/src/cli.ts`:
- Around line 82-84: The empty catch {} silently swallows errors; replace it
with catch (err) { /* explain why errors are ignored (e.g., best-effort read of
package.json) */ } and optionally emit a low-verbosity diagnostic (e.g.,
console.debug or the module logger) such as console.debug("Failed to read
package metadata:", err) so maintainers see malformed/permission errors while
preserving the fallback behavior in the surrounding code that reads package
metadata.
In `@packages/rari/src/proxy/execute-proxy.ts`:
- Around line 96-102: The empty catch in the body extraction block (in
execute-proxy.ts where result.text(), result.body and variable body are handled)
silently swallows errors; modify the catch to log the caught error at
debug/verbose level (using the module's existing logger such as processLogger or
logger, or console.debug if no logger exists) while still allowing execution to
continue, so failures extracting the response body are recorded for
troubleshooting without breaking proxy flow.
In `@packages/rari/src/proxy/matcher.ts`:
- Around line 72-73: Change the loose condition shape to a discriminated union
for condition.type (use 'header' | 'query' | 'cookie') and update the resolver
logic in the matcher functions that accept the parameter (the function whose
signature includes condition: { type: string, key: string, value?: string }) so
each branch is explicitly handled; replace any string-based checks with a switch
on condition.type, handle 'header', 'query', and 'cookie' cases, and add an
explicit default/unreachable path that throws an error so unknown types (e.g.,
'missing') cannot silently pass; also ensure the code that reads condition.value
is covered per-branch (or validated) so value resolution is exhaustive and
type-safe.
- Around line 83-86: The matching logic in matcher.ts incorrectly treats empty
string as falsy: change the checks that use if (!actualValue) and if
(condition.value && actualValue !== condition.value) to explicit null/undefined
checks so empty strings are allowed; specifically, replace the falsy test on
actualValue with a null/undefined check (e.g., actualValue == null) and replace
the truthy test on condition.value with an explicit undefined check (e.g.,
condition.value !== undefined) before comparing actualValue and condition.value,
updating the relevant code paths in the function that references actualValue and
condition.value so has/missing and value:'' cases behave correctly.
In `@packages/rari/src/proxy/runtime-executor.ts`:
- Around line 29-139: The functions and interfaces duplicated between
runtime-executor.ts and execute-proxy.ts (extractProxyHeaders, checkForRewrite,
checkForRedirect, handleContinueWithHeaders, handleDirectResponse,
processProxyResult, and the SimpleRequest/SimpleProxyResult interfaces) should
be moved into a new shared module (e.g., proxy-helpers.ts) that exports the
interfaces and those helper functions; update runtime-executor.ts and
execute-proxy.ts to import these symbols from the shared module and remove the
local duplicates so both files reuse the single implementation for consistency
and easier maintenance.
- Around line 97-104: The empty catch silently swallows errors when extracting
the response body (the block that assigns to the local variable `body` using
`result.text()` or `result.body`); update the catch to capture the thrown error
(e.g., `err`) and emit a debug-level log including context (e.g., "failed to
extract body from proxy result") using the project's logger used elsewhere
(mirroring the same fix from `execute-proxy.ts`), then preserve the current
fallback behavior so `body` stays undefined on failure.
In `@packages/rari/src/router/ClientRouter.tsx`:
- Around line 320-343: The history entry is being written too early in
updateHistoryState; change call sites so updateHistoryState (and any direct
window.history.replaceState/pushState at the other referenced locations) are
invoked only after the navigation fetch completes successfully. Specifically,
move the creation/persistence of HistoryState (route, navigationId,
scrollPosition, timestamp, key) and the pushState/replaceState call to the
success branch of the fetch/route-loading logic that triggers navigation
(instead of before the request), and ensure on fetch failure you do not modify
history or the address bar so currentRoute and rendered content remain
consistent.
- Around line 197-210: The updateDocumentMetadata function currently only
updates metadata keys that are present, causing stale head tags to persist when
a route omits keys; modify updateDocumentMetadata to explicitly clear/remove
metadata for keys that are absent by calling the appropriate cleanup routines
(e.g., remove or reset title/description via updateBasicMetadata, remove
canonical via updateCanonicalLink when metadata.canonical is undefined, remove
robots via updateRobotsMetadata when metadata.robots is undefined, and similarly
clear OG and Twitter tags via updateOpenGraphMetadata and updateTwitterMetadata
when their objects are missing); ensure each helper function
(updateBasicMetadata, updateCanonicalLink, updateRobotsMetadata,
updateOpenGraphMetadata, updateTwitterMetadata) supports removal semantics so
omitted fields remove the corresponding <meta>/<link> tags from <head>.
- Around line 515-519: completeNavigation() updates currentRoute and clears
error but doesn't persist the new navigationId, so subsequent navigate() calls
keep reusing the old id; update the setNavigationState call in
completeNavigation() (and the similar updates around the other occurrences) to
also write the new navigationId (the id you compute/derive for this successful
navigation, e.g., nextId or computedNavigationId) into
navigationState.navigationId so the next navigate() will increment from the
persisted value and emitted events/history entries remain uniquely correlated.
- Around line 395-430: The streaming UTF-8 decoder's internal buffer isn't
flushed after the read loop, so trailing partial multi-byte bytes can be lost;
after the while loop that reads from response.body.getReader() (where reader,
decoder, buffer are used and abort handling calls
cleanupAbortedNavigation(actualTargetPath, navigationId)), call decoder.decode()
with no arguments and append the result to buffer (i.e., buffer +=
decoder.decode()) before the final if (buffer.trim()) check so any remaining
bytes are emitted.
In `@packages/rari/src/router/navigation-error-handler.ts`:
- Around line 137-143: The network-error detection currently only checks error
instanceof TypeError && error.message.includes('fetch'), which misses
browser-specific fetch failure messages (e.g., Firefox "NetworkError...", Safari
"Load failed"). Update the condition that decides to call handleNetworkError to
match a broader set of failure messages (for example by testing error.message
against a case-insensitive regex covering "failed to fetch", "networkerror",
"load failed" and similar tokens) while keeping the TypeError check; change the
conditional in navigation-error-handler.ts (the branch that invokes
handleNetworkError(error, url)) to use that broader message test so Safari and
other browsers' fetch failures are routed to handleNetworkError instead of
handleUnknownError.
In `@packages/rari/src/router/props-extractor.ts`:
- Around line 181-193: The current mergeTitleField collapses a parent title
template into a plain string when an intermediate segment provides a string,
losing the template for deeper segments; update mergeTitleField so that if
parentTitle is an object with a template and childTitle is a string you do not
replace the template with the interpolated string but instead return the parent
title object (preserving parentTitle.template) so deeper merges can still apply
the template (i.e., in mergeTitleField, when typeof childTitle === 'string' and
typeof parentTitle === 'object' && parentTitle.template, return parentTitle
rather than parentTitle.template.replace('%s', childTitle)).
- Around line 132-136: The default ServerSidePropsResult object currently
materializes optional fields (e.g., notFound, revalidate, redirect) on the
success path; change the initial state in props-extractor.ts so it only sets
props: {} (do not set notFound/redirect/revalidate) and let the existing helpers
(the functions that mutate/return the ServerSidePropsResult) add those optional
fields when needed; locate the const state: ServerSidePropsResult and remove the
extra optional keys so the shape remains sparse unless the helpers explicitly
set them.
In `@packages/rari/src/router/robots-generator.ts`:
- Line 28: The generateRuleLines function currently types its parameter as any;
replace that with a concrete Robots rule type to restore type safety by either
extracting the rule type from the imported Robots type (e.g., use
Robots["rules"] or the appropriate member/type alias) or by declaring an
explicit interface (e.g., RobotsRule with userAgent, allow, disallow,
crawlDelay) and changing the signature to function generateRuleLines(rule:
RobotsRule): string[]; update any internal references to match the chosen type
so TypeScript validates property access.
In `@packages/rari/src/router/sitemap-generator.ts`:
- Around line 313-328: The generateMultipleSitemaps function uses untrusted
sitemap IDs directly in file paths; validate or sanitize each id returned by
module.generateSitemaps() before writing. Ensure you build outputPath using the
safe sitemapDir (not outDir) and normalize/resolve the final path, then verify
the resolved path starts with sitemapDir; if it does not, throw an error. Also
sanitize the id (e.g., strip path separators or allow only a safe pattern)
before using it in filenames so module.default({ id }) and the written file name
cannot perform path traversal.
- Around line 112-125: In addVideoComplexFields, the relationship attribute
values for video.restriction and video.platform are interpolated unescaped;
update the template strings to escape those attribute values (e.g., call
escapeXml on video.restriction.relationship and video.platform.relationship) so
special XML characters are encoded before being placed inside the
relationship="..." attribute; keep the existing escapeXml usage for content and
uploader.info as-is.
- Around line 157-185: Replace the unsafe any types on the sitemap helper
functions with the concrete sitemap entry type exported by your Sitemap
definition (e.g., import and use the SitemapEntry or SitemapItem
interface/type). Update the signatures of addSitemapEntryFields,
addAlternateLanguages, and addMediaContent to accept that specific type, and
adjust any internal property accesses (entry.lastModified,
entry.alternates.languages, entry.images, entry.videos) to reflect the real type
shape so the compiler can verify presence/optional properties instead of relying
on any.
In `@packages/rari/src/runtime/shared/get-client-component.ts`:
- Around line 123-124: The code currently uses a truthy check "if (component)
return component" which incorrectly treats valid falsy exports (e.g., 0, '',
false) as unresolved; update the check to a nullish check so only null/undefined
are treated as missing (for example replace with "if (component != null) return
component" or "if (component !== null && component !== undefined) return
component"), referencing the local variable "component" in get-client-component
to ensure valid falsy exports are returned.
- Around line 141-143: The sync resolver in get-client-component.ts normalizes
leading "./" before calling resolveByPath (using normalizedPath), but
getClientComponentAsync still uses the raw path causing mismatches; update
getClientComponentAsync to perform the same normalization (e.g., compute
normalizedPath = path.startsWith('./') ? path.slice(2) : path) and use that
normalizedPath when calling resolveByPath with clientComponents and
clientComponentPaths so sync and async lookups are consistent.
In `@packages/rari/src/vite/image-scanner.ts`:
- Around line 199-211: The fallback path in extractImageUsages silently drops
aliased imports from 'rari/image' because extractImageUsagesWithRegex only
searches for literal <Image ...> tags; compute the set of aliases from the
original source (e.g., by parsing import statements for "rari/image" or adding a
helper extractImageAliases(content)) before the transform attempt, and pass that
alias set into extractImageUsagesWithRegex so the regex fallback also matches
usages like <HeroImage ...> or named-import aliases; update the
extractImageUsagesWithRegex signature and all other fallback call sites (the
other try/catch around lines 215-220) to accept and use the alias set, and keep
existing behavior for when no aliases are found.
- Around line 184-191: The current regex in processImageIdentifiers
(createElementRegex) fails on nested prop objects because `([^}]+)` stops at the
first `}`; replace the flat-regex approach with brace-balanced extraction or
proper AST traversal: either (A) after locating `React.createElement(
<identifier> ,` find the `{` index in transformedCode and scan forward counting
braces until the matching `}` to extract the full propsString, then pass that to
parseImageProps, or (B) parse transformedCode into an AST and walk for
CallExpressions whose callee is `React.createElement` and whose first argument
matches the identifier, then extract the ObjectExpression props node and adapt
parseImageProps to accept the AST node; update processImageIdentifiers, remove
reliance on ESCAPE_REGEX-based createElementRegex, and ensure parseImageProps
can handle the extracted full props content.
In `@web/src/components/Sidebar.tsx`:
- Around line 439-459: The pure helpers shouldExpandSection and shouldExpandItem
are defined inside the Sidebar component and get recreated each render; move
them to top-level (outside the Sidebar function) as plain functions so they
aren’t reallocated on every render. Extract both functions (preserving their
signatures that reference NavItem and pathname: string | null), place them above
the Sidebar component, and export them if needed elsewhere; update any
imports/types used by these functions so they compile after relocation.
- Line 145: The check using the hardcoded string 'Getting Started' in the
expression that sets showSectionChevron is fragile; replace it with a stable
identifier by adding an id or collapsible flag to the navigation data and use
that instead (e.g., read section.id === 'getting-started' or
section.collapsible). Update the NavItem type to include the chosen property and
change the logic in the Sidebar where showSectionChevron, hasSectionItems, and
section.label are referenced so the chevron decision uses section.id or
section.collapsible rather than the literal label.
---
Outside diff comments:
In `@crates/rari/src/runtime/ops.rs`:
- Around line 292-300: The JSON response from op_fetch_with_cache currently
includes "tags" only in one branch (using result.tags) which makes the response
shape inconsistent; update all branches of the op_fetch_with_cache response
(including the alternate branches around lines 304-331) to always emit a "tags"
field—use result.tags when available and an empty array (e.g., Vec::new() / []
equivalent) when not—to ensure a stable response shape across all Ok(...) and
Err/... return paths.
In `@crates/rari/src/server/middleware/request_context.rs`:
- Around line 117-131: Cached responses returned from the fetch_cache currently
ignore newly-parsed tags (variable tags) so tag coverage never grows; update the
cache-hit path in the request context (where fetch_cache.lock(),
cache.get(&cache_key), cached.clone() and result.was_cached are used) to
merge/union the newly-parsed tags into the cached entry before returning, and
likewise ensure the fresh-fetch path (the code that applies tags on reply) also
unions incoming tags with stored tags so cache entries are updated with any new
tags instead of dropping them.
In `@packages/deploy/src/railway.ts`:
- Around line 7-44: The two functions updatePackageJsonForRailway and
updatePackageJsonForRender (and the parallel createRailwayToml/createRenderYaml
and updateGitignoreForRailway/updateGitignoreForRender) duplicate deployment
logic; extract the shared behavior into parameterized helpers in utils.ts (e.g.,
updatePackageJsonForProvider(cwd, { providerName, deployScript, startScript,
dependency })) and refactor each provider-specific function to call these
helpers, moving common steps such as reading/writing package.json, backing up
existing start script, adding scripts/dependencies, and ensureMinimumNodeEngine
into the shared helpers to eliminate duplication and keep provider files thin.
In `@packages/rari/src/image/Image.tsx`:
- Around line 97-129: The useImageLazyLoad hook currently observes but doesn't
prevent network requests; change it to return a visibility boolean (e.g.,
isVisible) that becomes true when the IntersectionObserver reports intersection
and remains true thereafter, ensure it respects shouldPreload, unoptimized, and
loading === 'eager' (return true immediately in those cases), clean up the
observer on unmount, and update the Image component to only set img src/srcSet
when isVisible is true (or when preload/unoptimized/eager require immediate
load) so requests are deferred until visible.
- Around line 299-314: The AVIF/WEBP <source> elements are emitted even when a
custom loader is provided (loader), but buildSrcSetString cannot force format
through a loader; update the conditional around the DEFAULT_FORMATS checks in
the JSX (where pictureRef is used and buildSrcSetString is called with
sizesArray, finalSrc, quality, 'avif'/'webp', loader) to only render those
<source> tags when no custom loader is set (e.g., loader is null/undefined) or
when the loader explicitly supports a format query; in short, add a guard like
"&& !loader" (or use a capability flag on the loader) to the
DEFAULT_FORMATS.includes('avif') and DEFAULT_FORMATS.includes('webp') conditions
so format-specific sources are only emitted when buildSrcSetString can actually
produce that format.
In `@packages/rari/src/proxy/execute-proxy.ts`:
- Around line 168-172: The call to Promise.allSettled inside the execute-proxy
logic uses a .catch on a promise that never rejects; remove the unreachable
.catch and either (A) simply await or call Promise.allSettled(waitUntilPromises)
without .catch, or (B) handle individual failures by inspecting the returned
array of settlement objects and logging any entries with status === 'rejected';
update the code in execute-proxy.ts where waitUntilPromises is processed (look
for the Promise.allSettled(...) call) to implement one of these approaches and
ensure rejected promises are logged or handled appropriately.
In `@packages/rari/src/proxy/runtime-executor.ts`:
- Around line 167-171: The code calls
Promise.allSettled(waitUntilPromises).catch(...), but allSettled never rejects
so the .catch is unreachable; update the logic in runtime-executor.ts to remove
the .catch and instead handle outcomes by calling
Promise.allSettled(waitUntilPromises).then(results => { inspect results for
status === 'rejected' and log errors }), referencing the waitUntilPromises
variable so any rejected promise reasons are logged (or otherwise handled)
inside the .then callback.
In `@packages/rari/src/router/robots-generator.ts`:
- Around line 197-204: Check that the imported dynamic robots module actually
has a default export before using it: in the robots-generator logic that assigns
robotsData from module.default, add an explicit validation that module &&
module.default !== undefined; if missing, throw a clear error (or return a
rejected Promise) explaining the robots file must export a default (either an
object or a function). Keep the existing handling for a function export (call it
and await if it returns a Promise) and for a non-function default (assign
directly to robotsData), but ensure you surface a descriptive error immediately
when module.default is absent so generateRobotsTxt doesn't fail with an unclear
message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7dfa4fb2-e2f1-4dc6-b117-1f9c21e5d517
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
Cargo.tomlcrates/rari/Cargo.tomlcrates/rari/src/rsc/rendering/streaming/constants.rscrates/rari/src/rsc/rendering/streaming/js/streaming_react_setup.jscrates/rari/src/rsc/rendering/streaming/renderer.rscrates/rari/src/runtime/ext/node/init_node.jscrates/rari/src/runtime/ext/web/init_fetch.jscrates/rari/src/runtime/ops.rscrates/rari/src/server/middleware/request_context.rspackages/create-rari-app/templates/default/package.jsonpackages/deploy/src/railway.tspackages/deploy/src/render.tspackages/rari/src/cli.tspackages/rari/src/image/Image.tsxpackages/rari/src/proxy/execute-proxy.tspackages/rari/src/proxy/matcher.tspackages/rari/src/proxy/runtime-executor.tspackages/rari/src/router/ClientRouter.tsxpackages/rari/src/router/navigation-error-handler.tspackages/rari/src/router/navigation-utils.tspackages/rari/src/router/props-extractor.tspackages/rari/src/router/robots-generator.tspackages/rari/src/router/sitemap-generator.tspackages/rari/src/runtime/shared/get-client-component.tspackages/rari/src/vite/image-scanner.tspnpm-workspace.yamlweb/package.jsonweb/src/components/Sidebar.tsx
- Extract shared deployment logic into reusable provider-agnostic utilities - Create http-utils.ts and timer-utils.ts for common HTTP and timing operations - Add alias-resolver.ts and file-resolver.ts to vite plugin for improved module resolution - Refactor railway.ts and render.ts to use updatePackageJsonForProvider and updateGitignoreForProvider - Implement createOrBackupConfigFile utility for consistent config file handling across providers - Add cache tag accumulation logic in request_context.rs to preserve tags across cache hits - Update fetch response objects to include empty tags array for consistency - Consolidate navigation and routing error handling in ClientRouter and navigation-error-handler - Improve HMR coordination and error handling in vite plugin - Reduce code duplication across deployment providers by centralizing common patterns
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/rari/src/vite/hmr-error-handler.ts (1)
24-25: 🧹 Nitpick | 🔵 TrivialInconsistent timer clearing pattern.
recordError()still uses the inlineif (this.resetTimer) clearTimeout(this.resetTimer)pattern whilereset()anddispose()use the newclearTimerutility. Consider usingclearTimerhere as well for consistency.♻️ Suggested refactor for consistency
recordError(error: Error): void { this.errorCount++ this.lastError = error - if (this.resetTimer) - clearTimeout(this.resetTimer) + this.resetTimer = clearTimer(this.resetTimer) this.resetTimer = setTimeout(() => { this.reset()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/vite/hmr-error-handler.ts` around lines 24 - 25, recordError currently clears the timeout with an inline check ("if (this.resetTimer) clearTimeout(this.resetTimer)"); replace that with the shared utility clearTimer to match reset() and dispose(). Update the recordError method to call clearTimer(this.resetTimer) (and ensure any resulting value handling matches how reset() and dispose() manage this.resetTimer) so timer clearing is consistent across the HMR error handler.packages/rari/src/proxy/execute-proxy.ts (1)
29-31: 🧹 Nitpick | 🔵 TrivialConsider typing the executor interface to avoid
anycast.The
(executor as any).proxyFnassertion bypasses type checking. If feasible, consider extending the executor's type definition to includeproxyFn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/proxy/execute-proxy.ts` around lines 29 - 31, The code is bypassing type-checking by using (executor as any).proxyFn; update the executor's type instead: extend or create an interface/type (e.g., ExecutorWithProxy or augment the existing Executor) that includes proxyFn?: Function (or the specific function signature) and change the function parameter or local variable type so you can access proxyFn without casting; alternatively add a narrow type guard that asserts executor has proxyFn before using it—replace the (executor as any).proxyFn access with a typed access to the proxyFn property on the newly typed executor so TypeScript enforces the shape and signature of proxyFn.packages/deploy/src/render.ts (1)
48-122: 🧹 Nitpick | 🔵 TrivialConsider extracting shared provider README/message generation to utilities.
This block duplicates the Railway implementation pattern almost verbatim. A small provider-templating helper would reduce future drift and cognitive complexity.
Also applies to: 124-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/deploy/src/render.ts` around lines 48 - 122, The Render README generation duplicates logic used for other providers; extract the shared templating into a utility (e.g., a new function like createProviderReadmeSection(providerName: string, body: string) or formatProviderReadme(provider: 'render'|'railway', contentTemplate: string) and move the provider-specific block (currently renderReadmeSection inside updateReadmeForRender) into a provider-specific template passed to that helper; then update updateReadmeForRender to call the utility (use the same helper from the Railway implementation) to generate the final README content and reuse the helper for create-or-update logic so file existence, append vs create, and logSuccess calls are handled consistently.packages/deploy/src/utils.ts (1)
123-133:⚠️ Potential issue | 🟠 Major
ensureMinimumNodeEnginecurrently under-enforces the declared minimum version.
MIN_NODE_VERSIONis>=22.12.0, but the sufficiency logic effectively checks only major version in many cases. For example,engines.node: ">=22.0.0"can pass even though it allows unsupported22.0–22.11.Suggested fix (enforce full semver floor, not only major)
export const MIN_NODE_VERSION = '>=22.12.0' +const LOWER_BOUND_SEMVER_REGEX = /^>=?\s*(\d+)\.(\d+)\.(\d+)/ + +function parseLowerBoundSemver(range: string): [number, number, number] | null { + const m = range.trim().match(LOWER_BOUND_SEMVER_REGEX) + if (!m) return null + return [Number.parseInt(m[1], 10), Number.parseInt(m[2], 10), Number.parseInt(m[3], 10)] +} + +function compareSemver(a: [number, number, number], b: [number, number, number]): number { + if (a[0] !== b[0]) return a[0] - b[0] + if (a[1] !== b[1]) return a[1] - b[1] + return a[2] - b[2] +} export function ensureMinimumNodeEngine(packageJson: any, minVersion: string = MIN_NODE_VERSION): boolean { packageJson.engines = packageJson.engines || {} @@ if (packageJson.engines.node) { - if (!isNodeVersionSufficient(packageJson.engines.node, minMajor)) { + const required = parseLowerBoundSemver(minVersion) + const current = parseLowerBoundSemver(packageJson.engines.node) + const isSufficient = required && current + ? compareSemver(current, required) >= 0 + : isNodeVersionSufficient(packageJson.engines.node, minMajor) + + if (!isSufficient) { logWarn(`Current engines.node value "${packageJson.engines.node}" may not meet the required minimum of ${minVersion}`) logWarn(`Updating to ${minVersion} for deployment compatibility`) packageJson.engines.node = minVersion return true }Also applies to: 125-145
♻️ Duplicate comments (3)
packages/rari/src/router/props-extractor.ts (1)
178-190:⚠️ Potential issue | 🟠 MajorTitle template is collapsed for intermediate segments, breaking deeper inheritance.
This issue was previously flagged: when an intermediate layout provides a string title and a parent provides a template, the template is interpolated into a plain string. Subsequent page titles then lose access to the template.
Example chain:
- Root:
{ template: '%s | Site' }- Docs layout:
'Docs'→ merged:'Docs | Site'(now a string)- API page:
'API'→ merged:'API'(parent is string, no template)Expected final:
'API | Site'To preserve the template for deeper segments, return an object that keeps the template intact:
,
Proposed fix
function mergeTitleField(parentTitle: MetadataResult['title'], childTitle: MetadataResult['title']): MetadataResult['title'] { if (childTitle === undefined) return parentTitle if (typeof childTitle === 'string') { - if (typeof parentTitle === 'object' && parentTitle?.template) - return parentTitle.template.replace('%s', childTitle) + if (typeof parentTitle === 'object' && parentTitle?.template) { + return { + ...parentTitle, + default: parentTitle.template.replace('%s', childTitle), + } + } return childTitle } return childTitle }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/props-extractor.ts` around lines 178 - 190, The mergeTitleField function collapses templates when childTitle is a string by returning the interpolated string; instead, when parentTitle is an object with a template and childTitle is a string, return an object that preserves parentTitle.template and also contains the interpolated/rendered title (so deeper segments can still inherit the template). Update mergeTitleField (handling of parentTitle and childTitle) to return an object with the original template plus a rendered/title field (using parentTitle.template.replace('%s', childTitle)) rather than returning the plain string.packages/rari/src/router/robots-generator.ts (1)
36-51: 🧹 Nitpick | 🔵 TrivialConsider grouping User-Agent lines before directives.
The current implementation emits Allow/Disallow lines for each User-Agent separately. While functionally correct, the standard robots.txt convention is to group multiple User-Agent lines together before their shared directives, resulting in a more compact output.
Current output for
{userAgent: ['*', 'Googlebot'], allow: '/public'}:User-Agent: * Allow: /public User-Agent: Googlebot Allow: /publicConventional format:
User-Agent: * User-Agent: Googlebot Allow: /public♻️ Proposed refactor
function generateRuleLines(rule: RobotsRule): string[] { const lines: string[] = [] const userAgents = normalizeUserAgents(rule.userAgent) - for (const userAgent of userAgents) { + for (const userAgent of userAgents) lines.push(`User-Agent: ${userAgent}`) - const allows = normalizeArray(rule.allow) - for (const allow of allows) - lines.push(`Allow: ${allow}`) + const allows = normalizeArray(rule.allow) + for (const allow of allows) + lines.push(`Allow: ${allow}`) - const disallows = normalizeArray(rule.disallow) - for (const disallow of disallows) - lines.push(`Disallow: ${disallow}`) + const disallows = normalizeArray(rule.disallow) + for (const disallow of disallows) + lines.push(`Disallow: ${disallow}`) - if (rule.crawlDelay !== undefined) - lines.push(`Crawl-delay: ${rule.crawlDelay}`) + if (rule.crawlDelay !== undefined) + lines.push(`Crawl-delay: ${rule.crawlDelay}`) - lines.push('') - } + lines.push('') return lines }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/robots-generator.ts` around lines 36 - 51, The generator currently iterates userAgents and emits directives per-agent; instead change the loop to emit all User-Agent lines first and then the shared directives to group them: for each rule (the loop using userAgents and rule), first push each `User-Agent: ${userAgent}` for all entries in `userAgents`, then push the normalized `Allow` lines from `normalizeArray(rule.allow)` and `Disallow` lines from `normalizeArray(rule.disallow)`, then conditionally push `Crawl-delay: ${rule.crawlDelay}` if defined, and finally a blank line into `lines`; keep using the same `normalizeArray`, `rule.crawlDelay`, and `lines` identifiers so behavior remains the same but output is grouped.packages/rari/src/runtime/shared/get-client-component.ts (1)
137-142:⚠️ Potential issue | 🟠 MajorGenerate
./path aliases from one shared helper in both resolvers.The sync branch still never tries
./${path}for bare ids, and the async branch's multiline template literal currently produces\n./${path}instead of./${path}. As written,foo#Barstill misses whenever~clientComponentPathsis keyed as./foo.Suggested fix
+function getPathCandidates(path: string): string[] { + return path.startsWith('./') ? [path, path.slice(2)] : [path, `./${path}`] +} + function resolveByPathWithExport( id: string, clientComponents: Record<string, ComponentInfo>, clientComponentPaths: Record<string, string>, ): any { const [path, exportName] = id.split('#') - - const result = resolveByPath(path, exportName, clientComponents, clientComponentPaths) - if (result !== null) - return result - - const normalizedPath = path.startsWith('./') ? path.slice(2) : path - return resolveByPath(normalizedPath, exportName, clientComponents, clientComponentPaths) + for (const candidate of getPathCandidates(path)) { + const result = resolveByPath(candidate, exportName, clientComponents, clientComponentPaths) + if (result !== null) + return result + } + + return null }- const candidates = path.startsWith('./') - ? [path, path.slice(2)] - : [path, ` -./${path}`] + const candidates = getPathCandidates(path)Run this read-only check to confirm the async candidate literal currently spans two lines:
#!/bin/bash python - <<'PY' from pathlib import Path lines = Path("packages/rari/src/runtime/shared/get-client-component.ts").read_text().splitlines() for lineno in range(196, 200): print(f"{lineno}: {lines[lineno - 1]!r}") PYAlso applies to: 196-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/runtime/shared/get-client-component.ts` around lines 137 - 142, The sync resolver misses bare-id lookups like './foo' and the async resolver builds a multiline template yielding '\n./${path}'; add a small shared helper (e.g., getPathVariants(path)) that returns [path, `./${path}`] and use its results in both resolution sites instead of the current normalizedPath logic, and replace the async template literal with a single-line join/iteration using those variants so resolveByPath is invoked for both the raw and './' prefixed keys (refer to resolveByPath, normalizedPath, clientComponents, clientComponentPaths).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rari/src/server/middleware/request_context.rs`:
- Around line 132-136: Replace the O(n*m) dedup loop that checks
result.tags.contains(tag) with a HashSet-based approach: import
std::collections::HashSet, build a HashSet from result.tags, extend/insert each
entry from tags into that set, then replace result.tags with the set's contents
(or collect into a Vec if a Vec is required); apply this change to both the
block that currently iterates over tags (the shown loop over "for tag in &tags {
if !result.tags.contains(tag) { result.tags.push(tag.clone()); } }") and the
similar block at the other location (lines 176–180) to ensure O(1) lookups and
deduplication.
- Around line 131-140: The fetch_cache is merging request tags into cached
entries (see result.tags push loop and cache.put usage in fetch_cache) but
there's no tag-based invalidation (no tag_index or invalidate_by_tag for
fetch_cache) so accumulated tags are orphaned; either remove the tag
accumulation from fetch_cache (delete the loop that mutates result.tags and stop
returning merged tags to callers) or implement a tag-invalidation system
analogous to response_cache (add a tag_index for fetch_cache, record tags on
cache.put, and provide invalidate_by_tag logic) — pick one approach and update
the fetch_cache functions (and any callers expecting result.tags) to match the
chosen design.
In `@packages/deploy/src/utils.ts`:
- Around line 147-161: The function getRariVersion currently falls back to the
mutable string 'latest'; change it to return a reproducible pinned version
instead by introducing a constant fallback (e.g. RARI_FALLBACK_VERSION prefixed
with ^ or exact semver) and return that when the package.json cannot be read;
update the other occurrence noted (the similar fallback at lines 194-198) to use
the same RARI_FALLBACK_VERSION constant so both getRariVersion and the other
lookup return a deterministic pinned version rather than 'latest'.
- Around line 219-221: The current check using
gitignoreContent.includes(providerFolder) is too broad; instead read the
.gitignore via readFileSync, split into lines and check for an exact ignore
entry match (compare trimmed lines against the intended entry or each line in
providerGitignoreEntries) so substrings/comments don't falsely satisfy the
check, and only call writeFileSync to append providerGitignoreEntries when no
exact ignore-line exists; update the logic around gitignoreContent,
providerFolder, and providerGitignoreEntries to perform per-line/exact-match
validation before writing.
In `@packages/rari/src/image/Image.tsx`:
- Around line 223-227: The current img elements (e.g., the one using imgRef,
finalImgSrc, showAltText, alt, imgWidth, fill) conditionally set alt to an empty
string when showAltText is false, which makes images incorrectly decorative;
always render the alt attribute with the provided alt prop (or a guaranteed
non-empty fallback if the component exposes a prop indicating decorative intent)
instead of toggling it on load/error. Update the Image component instances
(including those using imgRef/finalImgSrc and the other occurrences referenced)
to always include alt={alt} (or alt={decorative ? '' : alt} if a dedicated
decorative prop exists), remove logic that clears alt based on showAltText, and
ensure any accessibility tests or props reflect the new behavior.
- Around line 103-108: The problem is that isVisible is only initialized from
shouldLoadImmediately and isn't updated if props later change to make
shouldLoadImmediately true; update the state when that computed value flips by
adding a useEffect that depends on shouldLoadImmediately and calls
setIsVisible(true) when shouldLoadImmediately becomes true (apply the same fix
for the other occurrence around lines referencing isVisible/setIsVisible at
134-135); reference the existing shouldLoadImmediately, isVisible and
setIsVisible symbols and ensure the effect returns nothing (no-op) when
shouldLoadImmediately is false.
In `@packages/rari/src/proxy/shared/utils.ts`:
- Around line 93-99: The empty catch block in the body extraction logic (around
result.text()/result.body that sets the local variable body) silently swallows
errors; update the catch to log the caught error at debug/trace level (including
context like result or a brief message) using the module's logger or a passed-in
logger so failures in result.text() are visible while still allowing execution
to continue, and ensure the catch does not rethrow so fault tolerance remains
intact.
In `@packages/rari/src/router/ClientRouter.tsx`:
- Around line 416-421: handleRedirect is a no-op that just returns finalPath;
either remove the function and use finalPath directly at call sites, or keep it
and add a clear comment explaining its intended future behavior and contract
(e.g., why _targetPath is accepted, expected return semantics, and when it will
mutate or validate finalPath). Update all callers of handleRedirect (references
to handleRedirect) accordingly if you remove it, or add the explanatory comment
above the handleRedirect declaration if you retain it.
- Around line 423-433: The empty catch in processMetadata hides decode/parse
errors; update it to catch the exception and log a warning instead of swallowing
it. In processMetadata (where you call response.headers.get('x-rari-metadata')
and then decode/JSON.parse to call updateDocumentMetadata), catch the error and
call a logger or console.warn with a clear message that includes the header
value and the error (e.g., "Failed to parse x-rari-metadata", metadataHeader,
error) so metadata parse failures are visible for debugging.
In `@packages/rari/src/router/navigation-error-handler.ts`:
- Around line 54-87: In handleHttpError, treat HTTP 408 (Request Timeout) and
429 (Too Many Requests) as retryable: update the logic in the generic return
(and/or introduce an isRetryable boolean used in the 404, 5xx, and fetch-error
branches) so that retryable is true when status >= 500 OR status === 408 OR
status === 429; leave 404 as non-retryable and keep the server-error branch
unchanged. Ensure originalError, statusCode, url, and timestamp remain populated
as before.
In `@packages/rari/src/router/sitemap-generator.ts`:
- Around line 259-313: The virtual module id string is duplicated as
virtualModuleId in both createSitemapPlugin and buildSitemapModule; extract it
to a single module-level constant (e.g., const VIRTUAL_SITEMAP_ID =
'\0virtual:sitemap') and replace the local virtualModuleId declarations inside
createSitemapPlugin and buildSitemapModule with references to that constant so
both resolveId, load, and the build input use the same identifier.
- Around line 284-295: The function extractChunkCode currently types its result
and callbacks as any; update it to use rolldown's exported RolldownBuild type
for the result parameter and use that type for any related callbacks to improve
type safety (import type { RolldownBuild } from 'rolldown' and change the
function signature to accept result: RolldownBuild), leaving the internal logic
(checks for result.output, finding entryChunk, and returning entryChunk.code)
unchanged and ensuring the entryChunk variable retains its narrowed type checks.
In `@packages/rari/src/runtime/shared/get-client-component.ts`:
- Around line 163-181: The async loader getClientComponentAsync currently lacks
the new name-fallback logic used by resolveClientComponent, causing
getClientComponentAsync('Button') to return null while the sync resolver
succeeds; update getClientComponentAsync to mirror resolveClientComponent's
resolution order: read globalAccessor['~clientComponents'],
['~clientComponentPaths'], and ['~clientComponentNames'], attempt
resolveById(id) first, if id contains '#' call resolveByPathWithExport(id,
clientComponents, clientComponentPaths), and finally fall back to
resolveByName(id, clientComponents, clientComponentNames); also ensure any
loading/enqueue behavior used when starting async loads is applied when
resolving via the name map so behavior matches the sync path.
- Around line 93-96: The sync prefetch path in tryLoadComponent currently calls
triggerComponentLoad(componentInfo) which invokes executeLoader that rethrows
errors, causing unhandled promise rejections; change tryLoadComponent to use the
same error-swallowing behavior as loadClientComponent by either calling
loadClientComponent(componentInfo) or invoking
triggerComponentLoad(componentInfo).catch(() => {/* swallow/log like
loadClientComponent */}); ensure the promise rejection is handled rather than
left unawaited so loader errors are logged/swallowed consistently.
In `@packages/rari/src/shared/http-utils.ts`:
- Around line 1-4: The helper throwIfNotOk currently throws a plain Error with
only a message, losing the HTTP status metadata; update throwIfNotOk to create
an Error object for the response (inside the if (!response.ok) block), set
properties on that Error such as status = response.status and statusText =
response.statusText (and optionally body or errorText) before throwing it, so
callers can inspect error.status (reference: function throwIfNotOk and the
Response object used there).
In `@packages/rari/src/vite/alias-resolver.ts`:
- Around line 8-13: The alias resolution currently iterates
Object.entries(aliases) in insertion order which causes prefix shadowing (e.g.,
"@" matching before "@/components"); update the logic in alias-resolver so
aliases are checked from most specific to least specific by sorting the alias
keys/entries by alias length descending before the for-loop that examines
source.startsWith(`${alias}/`) || source === alias; this ensures longer/more
specific aliases are matched first and then use the existing replacement,
path.join and path.resolve(projectRoot, ...) behavior unchanged.
In `@packages/rari/src/vite/file-resolver.ts`:
- Around line 8-25: The resolver appends extensions unconditionally causing
already-qualified imports like "./Foo.tsx" to become "./Foo.tsx.tsx"; update
both resolveFile (the loop that builds pathWithExt) and resolveIndexFile to
first detect if resolvedPath already ends with one of the provided extensions
and if fs.existsSync(resolvedPath) return it immediately; otherwise continue
with the existing extension-appending logic (use the resolvedPath and extensions
parameters and the existing fs.existsSync checks).
In `@packages/rari/src/vite/image-scanner.ts`:
- Around line 229-246: The regex built from variable identifiers in
processImageIdentifiers poses a ReDoS risk; validate each identifier against a
safe JS identifier pattern (e.g., /^[A-Za-z_$][A-Za-z0-9_$]*$/) before
constructing escapedIdentifier and createElementPattern, and skip or handle
identifiers that don't match (log or ignore) so only validated identifiers are
used to build the RegExp; keep the existing ESCAPE_REGEX usage for escaping
after validation and then proceed to extractBalancedBraces, parseImageProps, and
addImageToMap as before.
- Around line 91-128: The transformCode function's build call should mark the
"rari/image" import as external so Rollup/Rollup-based build won't try to
resolve it during the virtual module transform; update the build(...) invocation
inside transformCode (the call that uses virtualModuleId and plugins) to include
an external config that lists "rari/image" (matching the pattern used in
server-build.ts) so missing runtime-only imports won't trigger the fallback path
in extractImageUsages.
In `@web/src/components/Sidebar.tsx`:
- Line 132: Remove the redundant key prop on the inner <li> rendering nested
items in Sidebar.tsx: the key `${itemKey}-${nestedItem.href ||
nestedItem.label}` should be removed from the <li> inside the mapped component
(where nestedItem is used) because React keys must be applied at the map call
site (where the mapped component is created, using itemKey). Keep the existing
key at the map invocation that supplies itemKey and ensure no duplicate keys
exist inside the rendered element.
- Around line 16-21: Remove the duplicate NavItem type in Sidebar.tsx and
instead import the shared NavItem interface from the docs-navigation module:
export the interface (export interface NavItem { label: string; href?: string;
items?: NavItem[]; collapsible?: boolean }) from the docs-navigation module and
replace the local declaration in Sidebar.tsx with an import of NavItem; update
any references in Sidebar component to use the imported NavItem type.
---
Outside diff comments:
In `@packages/deploy/src/render.ts`:
- Around line 48-122: The Render README generation duplicates logic used for
other providers; extract the shared templating into a utility (e.g., a new
function like createProviderReadmeSection(providerName: string, body: string) or
formatProviderReadme(provider: 'render'|'railway', contentTemplate: string) and
move the provider-specific block (currently renderReadmeSection inside
updateReadmeForRender) into a provider-specific template passed to that helper;
then update updateReadmeForRender to call the utility (use the same helper from
the Railway implementation) to generate the final README content and reuse the
helper for create-or-update logic so file existence, append vs create, and
logSuccess calls are handled consistently.
In `@packages/rari/src/proxy/execute-proxy.ts`:
- Around line 29-31: The code is bypassing type-checking by using (executor as
any).proxyFn; update the executor's type instead: extend or create an
interface/type (e.g., ExecutorWithProxy or augment the existing Executor) that
includes proxyFn?: Function (or the specific function signature) and change the
function parameter or local variable type so you can access proxyFn without
casting; alternatively add a narrow type guard that asserts executor has proxyFn
before using it—replace the (executor as any).proxyFn access with a typed access
to the proxyFn property on the newly typed executor so TypeScript enforces the
shape and signature of proxyFn.
In `@packages/rari/src/vite/hmr-error-handler.ts`:
- Around line 24-25: recordError currently clears the timeout with an inline
check ("if (this.resetTimer) clearTimeout(this.resetTimer)"); replace that with
the shared utility clearTimer to match reset() and dispose(). Update the
recordError method to call clearTimer(this.resetTimer) (and ensure any resulting
value handling matches how reset() and dispose() manage this.resetTimer) so
timer clearing is consistent across the HMR error handler.
---
Duplicate comments:
In `@packages/rari/src/router/props-extractor.ts`:
- Around line 178-190: The mergeTitleField function collapses templates when
childTitle is a string by returning the interpolated string; instead, when
parentTitle is an object with a template and childTitle is a string, return an
object that preserves parentTitle.template and also contains the
interpolated/rendered title (so deeper segments can still inherit the template).
Update mergeTitleField (handling of parentTitle and childTitle) to return an
object with the original template plus a rendered/title field (using
parentTitle.template.replace('%s', childTitle)) rather than returning the plain
string.
In `@packages/rari/src/router/robots-generator.ts`:
- Around line 36-51: The generator currently iterates userAgents and emits
directives per-agent; instead change the loop to emit all User-Agent lines first
and then the shared directives to group them: for each rule (the loop using
userAgents and rule), first push each `User-Agent: ${userAgent}` for all entries
in `userAgents`, then push the normalized `Allow` lines from
`normalizeArray(rule.allow)` and `Disallow` lines from
`normalizeArray(rule.disallow)`, then conditionally push `Crawl-delay:
${rule.crawlDelay}` if defined, and finally a blank line into `lines`; keep
using the same `normalizeArray`, `rule.crawlDelay`, and `lines` identifiers so
behavior remains the same but output is grouped.
In `@packages/rari/src/runtime/shared/get-client-component.ts`:
- Around line 137-142: The sync resolver misses bare-id lookups like './foo' and
the async resolver builds a multiline template yielding '\n./${path}'; add a
small shared helper (e.g., getPathVariants(path)) that returns [path,
`./${path}`] and use its results in both resolution sites instead of the current
normalizedPath logic, and replace the async template literal with a single-line
join/iteration using those variants so resolveByPath is invoked for both the raw
and './' prefixed keys (refer to resolveByPath, normalizedPath,
clientComponents, clientComponentPaths).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 199660ef-e57f-4310-a764-860241b8b3bc
📒 Files selected for processing (29)
crates/rari/src/runtime/ops.rscrates/rari/src/server/middleware/request_context.rspackages/deploy/src/railway.tspackages/deploy/src/render.tspackages/deploy/src/utils.tspackages/rari/src/image/Image.tsxpackages/rari/src/proxy/execute-proxy.tspackages/rari/src/proxy/matcher.tspackages/rari/src/proxy/runtime-executor.tspackages/rari/src/proxy/shared/utils.tspackages/rari/src/router/ClientRouter.tsxpackages/rari/src/router/navigation-error-handler.tspackages/rari/src/router/props-extractor.tspackages/rari/src/router/robots-generator.tspackages/rari/src/router/sitemap-generator.tspackages/rari/src/runtime-client.tspackages/rari/src/runtime/ErrorBoundaryWrapper.tsxpackages/rari/src/runtime/shared/get-client-component.tspackages/rari/src/shared/http-utils.tspackages/rari/src/shared/timer-utils.tspackages/rari/src/vite/alias-resolver.tspackages/rari/src/vite/file-resolver.tspackages/rari/src/vite/hmr-coordinator.tspackages/rari/src/vite/hmr-error-handler.tspackages/rari/src/vite/image-scanner.tspackages/rari/src/vite/index.tspackages/rari/src/vite/server-build.tsweb/src/components/Sidebar.tsxweb/src/lib/docs-navigation.ts
- Remove separate react_globals_setup.js and consolidate React initialization into component_eval_setup.js - Simplify React global object setup by removing redundant Component, Fragment, and Suspense assignments - Add null checks in jsx/jsxs functions to handle missing React context gracefully - Integrate spinner keyframes initialization directly into LoadingSpinner setup - Remove module_reload extension and associated initialization logic - Remove callbacks.rs and streaming module from runtime extensions - Consolidate streaming React setup into init_react.js - Update fetch initialization to use simplified web extension setup - Streamline runtime extension module structure by removing unused modules - Reduce boilerplate in RSC rendering core by eliminating duplicate React setup code
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/rari/src/runtime/ext/rsc_renderer/mod.rs (1)
4-14:⚠️ Potential issue | 🔴 CriticalAdd
init_reactback to thedepslist.The ESM files in this extension (
rsc_traversal.jsandclient_registry.js) extensively use React symbols (React.Fragment, React.Suspense, Symbol.for('react.transitional.element'), etc.). Withoutinit_reactin the dependency list, Deno's extension system offers no guarantee that React will be initialized before this extension runs, potentially causing runtime errors when those React symbols are accessed. Althoughreact::extensions()is added to the extensions vector beforersc_renderer::extensions(), Deno respects explicitdepsdeclarations for ordering, not Vec insertion order. Restore the dependency:deps = [rari, init_react, init_rsc_modules].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/runtime/ext/rsc_renderer/mod.rs` around lines 4 - 14, The extension init_rsc_renderer's deps list is missing init_react which can cause React symbols used in rsc_traversal.js and client_registry.js to be uninitialized at runtime; update the extension!macro invocation named init_rsc_renderer to include init_react in the deps array (i.e., change deps = [rari, init_rsc_modules] to include init_react so it becomes deps = [rari, init_react, init_rsc_modules]) so Deno guarantees React initialization before this extension runs (the referenced files are rsc_traversal.js and client_registry.js and React is provided by the init_react extension).packages/rari/src/proxy/execute-proxy.ts (1)
22-27:⚠️ Potential issue | 🟠 MajorMove
waitUntilpromise handling to afinallyblock to ensure async errors are reported even ifproxyFnthrows.If
proxyFnthrows at line 33 afterevent.waitUntil(...)is called, execution jumps to thecatchblock and skips thePromise.allSettledblock at lines 35-43. This drops error reporting for background tasks that were already scheduled.Proposed fix
export async function executeProxy(simpleRequest: SimpleRequest): Promise<SimpleProxyResult> { + const waitUntilPromises: Promise<unknown>[] = [] + const flushWaitUntil = () => { + if (waitUntilPromises.length === 0) + return + void Promise.allSettled(waitUntilPromises).then((results) => { + results.forEach((result, index) => { + if (result.status === 'rejected') { + console.error(`[rari] Proxy: waitUntil promise ${index} failed:`, result.reason) + } + }) + }) + } + try { @@ - const waitUntilPromises: Promise<unknown>[] = [] const event = { waitUntil: (promise: Promise<unknown>) => { waitUntilPromises.push(promise) }, } @@ - if (waitUntilPromises.length > 0) { - void Promise.allSettled(waitUntilPromises).then((results) => { - results.forEach((result, index) => { - if (result.status === 'rejected') { - console.error(`[rari] Proxy: waitUntil promise ${index} failed:`, result.reason) - } - }) - }) - } - return await processProxyResult(result) } catch (error) { console.error('[rari] Proxy: executeProxy failed:', error) return { continue: true } } + finally { + flushWaitUntil() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/proxy/execute-proxy.ts` around lines 22 - 27, The current pattern collects background tasks into waitUntilPromises via event.waitUntil but calls Promise.allSettled after proxyFn execution, so if proxyFn throws those background errors aren’t reported; update executeProxy to run the Promise.allSettled(waitUntilPromises) reporting logic inside a finally block (after try/catch) so that regardless of proxyFn throwing the waitUntilPromises array is awaited/settled and any rejections are logged; ensure you reference and use the existing waitUntilPromises array and event.waitUntil behavior and keep existing error reporting logic around Promise.allSettled unchanged.
♻️ Duplicate comments (3)
packages/rari/src/image/Image.tsx (1)
105-114:⚠️ Potential issue | 🟠 MajorMove
isVisibleprop-sync out of render to avoid render-phase updates.
setIsVisible(true)at Line 109 (and mutatingprevShouldLoadImmediatelyRefat Line 108) runs during render. This is fragile under Strict/Concurrent rendering and should be effect-driven.💡 Proposed fix
function useImageLazyLoad( imgRef: React.RefObject<HTMLImageElement | null>, shouldPreload: boolean, unoptimized: boolean, loading: 'lazy' | 'eager', ) { const shouldLoadImmediately = shouldPreload || unoptimized || loading === 'eager' const [isVisible, setIsVisible] = useState(shouldLoadImmediately) - const prevShouldLoadImmediatelyRef = useRef(shouldLoadImmediately) - - if (shouldLoadImmediately && !prevShouldLoadImmediatelyRef.current) { - prevShouldLoadImmediatelyRef.current = true - setIsVisible(true) - } - - useEffect(() => { - prevShouldLoadImmediatelyRef.current = shouldLoadImmediately - }, [shouldLoadImmediately]) + useEffect(() => { + if (shouldLoadImmediately) + setIsVisible(true) + }, [shouldLoadImmediately]) useEffect(() => { if (shouldLoadImmediately) returnIn React 19, what is the recommended pattern for syncing state from props when a boolean flips (e.g., shouldLoadImmediately), and is calling setState during render discouraged?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/image/Image.tsx` around lines 105 - 114, The code is calling setIsVisible(true) and mutating prevShouldLoadImmediatelyRef during render when shouldLoadImmediately flips, which can cause render-phase updates; move this sync into an effect: in a useEffect that depends on shouldLoadImmediately, compare prevShouldLoadImmediatelyRef.current to shouldLoadImmediately, if it changed and shouldLoadImmediately is true call setIsVisible(true) and update prevShouldLoadImmediatelyRef.current accordingly, and keep the existing effect that sets prevShouldLoadImmediatelyRef to shouldLoadImmediately for other updates to avoid any setState during render.packages/rari/src/router/ClientRouter.tsx (1)
90-96:⚠️ Potential issue | 🟠 MajorRemove stale viewport metadata when the next route omits it.
Line 90 updates viewport only when present; when absent, the previous route’s viewport tag remains in
<head>.Proposed fix
if (metadata.viewport) { updateOrCreateMetaTag('meta[name="viewport"]', { name: 'viewport', content: metadata.viewport, }) } + else { + removeMetaTag('meta[name="viewport"]') + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/ClientRouter.tsx` around lines 90 - 96, The current code only calls updateOrCreateMetaTag('meta[name="viewport"]', ...) when metadata.viewport exists, leaving a stale <meta name="viewport"> from the previous route; update the route-change logic in ClientRouter to explicitly remove the existing viewport meta tag when metadata.viewport is absent by querying for document.querySelector('meta[name="viewport"]') and removing it (or calling the existing removeMetaTag helper if one exists) instead of doing nothing; keep the update path using updateOrCreateMetaTag('meta[name="viewport"]', { name: 'viewport', content: metadata.viewport }) when metadata.viewport is present.packages/rari/src/router/props-extractor.ts (1)
182-186:⚠️ Potential issue | 🟠 MajorPreserve template metadata instead of collapsing to a string.
Line 183–186 still reduces
{ template: ... }to a plain string whenchildTitleis a string, which drops template inheritance for deeper segments.Proposed fix
if (typeof childTitle === 'string') { if (typeof parentTitle === 'object' && parentTitle?.template) { - const hasPlaceholder = parentTitle.template.includes('%s') - return hasPlaceholder ? parentTitle.template.replace('%s', childTitle) : childTitle + return { + ...parentTitle, + default: childTitle, + } } return childTitle }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/props-extractor.ts` around lines 182 - 186, When childTitle is a string and parentTitle is an object with a template, don't return a raw string (which drops the template metadata); instead return an object that preserves the parent template and includes the resolved string (e.g. keep parentTitle.template and add the computed text or value). Update the branch handling parentTitle/template and childTitle to return an object (preserving parentTitle.template) rather than collapsing to childTitle so deeper segments can inherit the template; reference the variables parentTitle and childTitle in props-extractor.ts to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rari/src/rsc/rendering/core/utils.rs`:
- Around line 41-43: The HMR bootstrap scripts (JSX_RUNTIME_SETUP_SCRIPT and
COMPONENT_EVAL_SETUP_SCRIPT) and transform_imports_for_hmr reference
globalThis.React but never initialize it; add an initialization of
globalThis.React before any transformed code runs (or, inside
transform_imports_for_hmr branches that inject "var React = globalThis.React;",
fall back to defining globalThis.React = globalThis.React || { createElement: /*
shim */, Fragment: /* shim */, Suspense: /* noop or shim */ }) so classic JSX
usages (React.createElement/Fragment/Suspense) and globals like
globalThis.LoadingSpinner will not throw; update the JSX_RUNTIME_SETUP_SCRIPT
and COMPONENT_EVAL_SETUP_SCRIPT to ensure globalThis.React is defined prior to
executing transformed imports.
In `@crates/rari/src/runtime/ext/rari/mod.rs`:
- Around line 10-14: The impl block for ExtensionTrait<()> for rari (fn init(():
()) -> Extension { rari::init() }) is not used because runtime initialization
calls extensions(is_snapshot) which delegates to rari::build(); either remove
this unused impl to avoid dead code, or explicitly document and suppress
warnings by adding a comment explaining it's intentionally unused and annotate
it with #[allow(dead_code)] (or #[allow(dead_code, unused)]), ensuring you
reference the rari::init(), rari::build(), and extensions(is_snapshot) code
paths so future maintainers understand why the trait impl was retained or
removed.
In `@crates/rari/src/runtime/ext/web/init_fetch.js`:
- Around line 116-122: The dedupe logic (requestDedupeMap) currently ignores
tags so concurrent requests with identical base keys but different
init.rari.tags result in followers cloning the leader's response without
propagating tags into fetchWithRustCache; update the keying so tags are
included: when building options.tags from init?.rari?.tags or init?.next?.tags
(the tags variable), normalize the tag array (filter strings, trim, sort) and
either append JSON.stringify(normalizedTags) to the dedupe key or use that
normalized string as part of the dedupeMap key; ensure fetch paths that set
options.tags and call fetchWithRustCache (the block using options.tags and the
code path referenced in the comment) use the same normalized-tag-aware key
(apply the same change to the other tagged-fetch block noted in the review).
In `@crates/rari/src/server/middleware/request_context.rs`:
- Around line 117-118: The fast-return path (guard.as_ref()) returns the
leader’s cached result without merging the follower’s tags parsed from
options.get("tags") (serde_json::from_str), so concurrent followers with
different tags lose their tags; before returning the shared in-flight result in
that branch, parse the follower tags into `tags: Vec<String>` (same parsing as
other branches), merge them into the result's tag list (append and deduplicate
or use the existing merge routine used in the leader/cache-hit code) and
persist/update the shared result so both the returned value and the stored
in-flight result include the combined tags; apply the same fix to the other
similar locations noted (the blocks around the other occurrences referenced in
the comment).
In `@packages/deploy/src/utils.ts`:
- Around line 189-195: The backup logic currently compares
packageJson.scripts.start to the literal 'rari start', which will mis-backup
generated scripts if config.startScript is different; compute the intended new
start value (e.g., const newStart = config.startScript || 'rari start') and only
create packageJson.scripts['start:original'] when packageJson.scripts.start
exists and packageJson.scripts.start !== newStart and
packageJson.scripts['start:original'] is not already set; then set
packageJson.scripts.start = newStart. This uses the symbols
packageJson.scripts.start, packageJson.scripts['start:original'], and
config.startScript to locate and fix the code.
In `@packages/rari/src/image/Image.tsx`:
- Around line 54-56: The prop extraction uses || which treats empty strings and
zero like missing values; change the fallbacks for imgWidth, imgHeight and
imgBlurDataURL to use nullish coalescing (??) so explicit values like
blurDataURL = "" or width = 0 are respected; update the expressions that
reference width, height, and blurDataURL (and the typeof src checks) to use ??
instead of || to prevent unintended fallback to src metadata.
- Around line 287-299: When imgWidth is provided the current logic sets
sizesArray = [imgWidth] and shouldUseSrcSet = false so the imgElement omits
srcSet and loses DPR candidates; update the rendering logic in Image.tsx to
always generate a DPR-based srcSet for the fixed-width path by calling
buildSrcSetString (or the loader) with density descriptors when imgWidth is
defined (i.e., treat the fixed-width branch specially: compute a srcSet with 1x,
2x, 3x candidates from finalSrc and quality and pass it into the imgElement's
srcSet), while keeping sizes undefined for the fixed-width case; modify the
conditions around shouldUseSrcSet / srcSet assignment so buildSrcSetString is
used for both responsive sizesArray and fixed imgWidth (refer to imgWidth,
sizesArray, shouldUseSrcSet, buildSrcSetString, and imgElement).
In `@packages/rari/src/proxy/matcher.ts`:
- Around line 70-75: checkHostCondition currently returns the input key when the
request hostname matches (returning key) which is inconsistent with other
checkers that return the actual matched value; update checkHostCondition
(function name) to return request.rariUrl.hostname when request.rariUrl.hostname
=== key and null otherwise so callers receive the actual hostname value instead
of the key.
In `@packages/rari/src/proxy/shared/utils.ts`:
- Around line 50-51: The current checks use truthiness on result.status (e.g.,
if (!result || !result.status)) which treats a valid status of 0 as absent;
update these checks to use explicit null/undefined comparisons (e.g.,
result.status == null / result.status != null) so a status of 0 is handled
correctly—replace occurrences that test result.status truthiness (the if
(!result || !result.status) check and the analogous check later in the file)
with nullish comparisons while keeping the existing early-return behavior when
result itself is falsy.
- Around line 16-19: The response.headers type and how headers are set in
handleDirectResponse currently overwrite duplicate header names (losing multiple
Set-Cookie), so change the header representation to allow multiple values (e.g.,
update response?: { headers: Record<string, string | string[]> } or a
string[]-capable type) and update the forEach in handleDirectResponse to append
instead of replace: when a header key already exists, convert the stored value
to an array if needed and push the new value (or merge into an array) so
duplicate header names are preserved; update any consumers of response.headers
accordingly (look for response.headers usage and the handleDirectResponse
function name to apply the changes).
In `@packages/rari/src/router/navigation-utils.ts`:
- Around line 24-31: The function matchDynamicSegment currently treats a missing
segmentParam as a successful match which allows malformed manifest entries to
pass; change the behavior to fail closed by returning false when segmentParam is
undefined so the route does not match and parameters aren't silently dropped.
Update matchDynamicSegment and any analogous checks for catch-all segments (the
same conditional branches around segmentParam in the dynamic/catch-all handling)
to return false instead of true, and ensure callers relying on params extraction
(the params Record<string,string|string[]>) only proceed when a valid
segmentParam exists. Verify all occurrences (the dynamic segment check and the
catch-all branch mentioned in the diff) are updated consistently.
In `@packages/rari/src/router/robots-generator.ts`:
- Around line 202-204: The current runtime check in robots-generator.ts only
tests module.default === undefined and therefore misses the case where
module.default is explicitly null; update the validation in the code that throws
the Error (the branch checking module and module.default) to treat nullish
values as invalid (e.g., use module.default == null or module.default ===
undefined || module.default === null) so that null defaults also trigger the
same error message, unless you intentionally want null to be allowed—in which
case add a comment explaining that null is treated as valid.
In `@packages/rari/src/router/sitemap-generator.ts`:
- Around line 308-319: The loop in sitemap-generator.ts that computes
sanitizedId (via SANITIZE_ID_REGEX) and then writes to disk can silently
overwrite files when two different raw ids normalize to the same sanitizedId; to
fix, track seen sanitized names (e.g., a Set seenSanitizedIds) inside the
sitemap generation function and after computing sanitizedId check
seenSanitizedIds.has(sanitizedId); if present, either throw/log a clear error
identifying both the conflicting raw ids (use the original id value) or generate
a deterministic unique filename (e.g., append an index or a short hash derived
from String(id)) before calling fs.writeFile; ensure you update the Set with the
final filename/sanitizedId and reference the variables sanitizedId, sitemapIds,
and the fs.writeFile call when making the change.
- Around line 253-259: The resolveId hook currently lacks the importer parameter
and always resolves relative paths against sitemapFile.path, breaking nested
relative imports and deviating from Rollup conventions; update the function
signature to accept (source: string, importer?: string) and when source
startsWith('.') resolve using path.resolve(path.dirname(importer ??
sitemapFile.path), source), keep the special-case return of VIRTUAL_SITEMAP_ID
when source === VIRTUAL_SITEMAP_ID, and return null for unresolved cases so it
behaves consistently with other resolveId implementations (e.g., the one using
the importer parameter).
In `@packages/rari/src/runtime/shared/get-client-component.ts`:
- Around line 96-97: The truthy checks on componentInfo.component in
getComponentFromInfo and resolveById incorrectly treat valid falsy values (e.g.,
0, '', false) as missing; replace those checks with nullish checks (test for
null or undefined) so only absent values are rejected — update the conditions
that currently read like "if (!componentInfo.component)" to use a null/undefined
check (e.g., componentInfo.component == null) in both getComponentFromInfo and
resolveById to match the check used later.
- Line 37: The code currently returns componentInfo.component || null which
incorrectly treats valid falsy exports (0, '', false) as null; update both
occurrences that return componentInfo.component (replace the `|| null` usage
with a nullish check such as `?? null` or an explicit undefined check) so that
falsy-but-valid values are preserved (apply this change where
componentInfo.component is returned and make it consistent with the existing
`(component !== null && component !== undefined)` checks).
In `@packages/rari/src/shared/http-utils.ts`:
- Line 10: The current thrown Error interpolates the full response body
(errorText) which may leak sensitive data and produce huge logs; change the
construction so the Error.message uses only bounded/safe info (e.g., `HTTP
${response.status}: ${response.statusText}`) and attach a capped preview of the
body (e.g., first N characters plus ellipsis) to the error as a separate
property (use the existing `error` variable and set
`error.status`/`error.statusText` as before, plus `error.bodyPreview`), ensuring
you do NOT include the full `errorText` in the message or any top-level logable
fields.
In `@packages/rari/src/vite/image-scanner.ts`:
- Around line 83-89: The scanDirectory function currently swallows
missing-directory errors (await fs.access(dir) catch { return }), which hides
typos in the required primary scan root; update the logic so missing primary
roots raise an error while still permitting silent returns for optional
additional dirs — e.g., add a boolean parameter like required (or throw from
scanDirectory when a new flag primary=true is passed) and change the caller that
invokes scanDirectory for the main srcDir to call scanDirectory(srcDir, images,
true) (keep additionalDirs calling with false), or alternatively perform an
explicit fs.access check before calling scanDirectory for the required srcDir
and throw a descriptive Error if absent; ensure you update references to
scanDirectory and any call sites around the current primary scan invocation (and
similarly address the same pattern referenced around lines 115-123 for
additionalDirs).
- Around line 33-39: Remove the brittle exact-string import prefilter in
processFile and make file reads resilient: call fs.readFile inside a try/catch
so a read error doesn't reject the whole scan, and after successfully reading
call extractImageUsages(content, fullPath, images) unconditionally (let
extractImageUsages handle alias/import detection); reference processFile and
extractImageUsages when updating the logic.
---
Outside diff comments:
In `@crates/rari/src/runtime/ext/rsc_renderer/mod.rs`:
- Around line 4-14: The extension init_rsc_renderer's deps list is missing
init_react which can cause React symbols used in rsc_traversal.js and
client_registry.js to be uninitialized at runtime; update the extension!macro
invocation named init_rsc_renderer to include init_react in the deps array
(i.e., change deps = [rari, init_rsc_modules] to include init_react so it
becomes deps = [rari, init_react, init_rsc_modules]) so Deno guarantees React
initialization before this extension runs (the referenced files are
rsc_traversal.js and client_registry.js and React is provided by the init_react
extension).
In `@packages/rari/src/proxy/execute-proxy.ts`:
- Around line 22-27: The current pattern collects background tasks into
waitUntilPromises via event.waitUntil but calls Promise.allSettled after proxyFn
execution, so if proxyFn throws those background errors aren’t reported; update
executeProxy to run the Promise.allSettled(waitUntilPromises) reporting logic
inside a finally block (after try/catch) so that regardless of proxyFn throwing
the waitUntilPromises array is awaited/settled and any rejections are logged;
ensure you reference and use the existing waitUntilPromises array and
event.waitUntil behavior and keep existing error reporting logic around
Promise.allSettled unchanged.
---
Duplicate comments:
In `@packages/rari/src/image/Image.tsx`:
- Around line 105-114: The code is calling setIsVisible(true) and mutating
prevShouldLoadImmediatelyRef during render when shouldLoadImmediately flips,
which can cause render-phase updates; move this sync into an effect: in a
useEffect that depends on shouldLoadImmediately, compare
prevShouldLoadImmediatelyRef.current to shouldLoadImmediately, if it changed and
shouldLoadImmediately is true call setIsVisible(true) and update
prevShouldLoadImmediatelyRef.current accordingly, and keep the existing effect
that sets prevShouldLoadImmediatelyRef to shouldLoadImmediately for other
updates to avoid any setState during render.
In `@packages/rari/src/router/ClientRouter.tsx`:
- Around line 90-96: The current code only calls
updateOrCreateMetaTag('meta[name="viewport"]', ...) when metadata.viewport
exists, leaving a stale <meta name="viewport"> from the previous route; update
the route-change logic in ClientRouter to explicitly remove the existing
viewport meta tag when metadata.viewport is absent by querying for
document.querySelector('meta[name="viewport"]') and removing it (or calling the
existing removeMetaTag helper if one exists) instead of doing nothing; keep the
update path using updateOrCreateMetaTag('meta[name="viewport"]', { name:
'viewport', content: metadata.viewport }) when metadata.viewport is present.
In `@packages/rari/src/router/props-extractor.ts`:
- Around line 182-186: When childTitle is a string and parentTitle is an object
with a template, don't return a raw string (which drops the template metadata);
instead return an object that preserves the parent template and includes the
resolved string (e.g. keep parentTitle.template and add the computed text or
value). Update the branch handling parentTitle/template and childTitle to return
an object (preserving parentTitle.template) rather than collapsing to childTitle
so deeper segments can inherit the template; reference the variables parentTitle
and childTitle in props-extractor.ts to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a8d2475-3c1f-4e93-a74f-2f52d59c550c
📒 Files selected for processing (41)
crates/rari/src/rsc/rendering/core/constants.rscrates/rari/src/rsc/rendering/core/js/component_eval_setup.jscrates/rari/src/rsc/rendering/core/js/react_globals_setup.jscrates/rari/src/rsc/rendering/core/renderer.rscrates/rari/src/rsc/rendering/core/utils.rscrates/rari/src/rsc/rendering/layout/route_composer.rscrates/rari/src/rsc/rendering/streaming/js/streaming_init.jscrates/rari/src/rsc/rendering/streaming/js/streaming_react_setup.jscrates/rari/src/runtime/ext/mod.rscrates/rari/src/runtime/ext/module_reload/init_module_reload.jscrates/rari/src/runtime/ext/module_reload/mod.rscrates/rari/src/runtime/ext/rari/callbacks.rscrates/rari/src/runtime/ext/rari/mod.rscrates/rari/src/runtime/ext/rari/rari.jscrates/rari/src/runtime/ext/react/init_react.jscrates/rari/src/runtime/ext/rsc_renderer/mod.rscrates/rari/src/runtime/ext/streaming/mod.rscrates/rari/src/runtime/ext/web/init_fetch.jscrates/rari/src/runtime/mod.rscrates/rari/src/runtime/module_loader/core.rscrates/rari/src/runtime/module_loader/node_stubs.rscrates/rari/src/server/middleware/request_context.rspackages/deploy/src/utils.tspackages/rari/src/image/Image.tsxpackages/rari/src/proxy/execute-proxy.tspackages/rari/src/proxy/matcher.tspackages/rari/src/proxy/shared/utils.tspackages/rari/src/router/ClientRouter.tsxpackages/rari/src/router/navigation-error-handler.tspackages/rari/src/router/navigation-utils.tspackages/rari/src/router/props-extractor.tspackages/rari/src/router/robots-generator.tspackages/rari/src/router/sitemap-generator.tspackages/rari/src/runtime/shared/get-client-component.tspackages/rari/src/shared/http-utils.tspackages/rari/src/vite/alias-resolver.tspackages/rari/src/vite/file-resolver.tspackages/rari/src/vite/hmr-error-handler.tspackages/rari/src/vite/image-scanner.tsweb/src/components/Sidebar.tsxweb/src/lib/docs-navigation.ts
💤 Files with no reviewable changes (13)
- crates/rari/src/runtime/ext/streaming/mod.rs
- crates/rari/src/runtime/mod.rs
- crates/rari/src/rsc/rendering/core/constants.rs
- crates/rari/src/rsc/rendering/layout/route_composer.rs
- crates/rari/src/runtime/ext/rari/callbacks.rs
- crates/rari/src/runtime/ext/mod.rs
- crates/rari/src/runtime/ext/module_reload/init_module_reload.js
- crates/rari/src/rsc/rendering/streaming/js/streaming_react_setup.js
- crates/rari/src/rsc/rendering/core/js/react_globals_setup.js
- crates/rari/src/runtime/module_loader/node_stubs.rs
- crates/rari/src/rsc/rendering/streaming/js/streaming_init.js
- crates/rari/src/runtime/ext/module_reload/mod.rs
- crates/rari/src/runtime/ext/rari/rari.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/rari/src/runtime/module_reload/manager.rs (3)
237-241:⚠️ Potential issue | 🟠 MajorReload failures are silently ignored in parallel mode.
handle.awaitreturnsResult<Result<(), RariError>, JoinError>. This code only handles the outerJoinError(task panic/cancel), but the innerResult<(), RariError>fromreload_moduleis discarded. Actual reload failures go unreported, unlike the sequential path which propagates errors.🐛 Proposed fix to handle inner result
for handle in handles { - if let Err(e) = handle.await { - error!(error = %e, "Batch reload task failed"); + match handle.await { + Ok(Ok(_)) => {} + Ok(Err(e)) => { + error!(error = %e, "Module reload failed in batch"); + } + Err(e) => { + error!(error = %e, "Batch reload task panicked"); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/runtime/module_reload/manager.rs` around lines 237 - 241, The current loop over handles only inspects the outer JoinError from handle.await and drops the inner Result from reload_module, so reload errors are ignored; update the loop in manager.rs (the for handle in handles block) to match the nested Result<Result<(), RariError>, JoinError> returned by handle.await, and when the inner Result is Err(e) (the RariError from reload_module) log or propagate that error (similar to the sequential path) instead of discarding it; ensure you reference handle.await and use both match arms for the JoinError and the inner Result to report the reload failure via error!(...) or return the error as appropriate.
195-212: 🧹 Nitpick | 🔵 TrivialUnused
_file_pathparameter is a code smell.The
_file_pathparameter is passed throughreload_module→reload_with_retry→reload_module_internalbut never used (note the underscore prefix). If file path information isn't needed for the reload logic, consider removing it from the internal method signature to avoid confusion. If it's intended for future use, a brief comment would clarify the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/runtime/module_reload/manager.rs` around lines 195 - 212, The _file_path parameter on reload_module_internal is unused; either remove it from reload_module_internal's signature and from its callers (reload_module and reload_with_retry) so the API reflects actual usage, or keep the parameter but document its intended future purpose with a concise comment in reload_module_internal and mark it as intentionally unused (e.g., keep the leading underscore). Update all references to the function (reload_module → reload_with_retry → reload_module_internal) to match the chosen approach to avoid unused-parameter warnings and confusion.
125-136:⚠️ Potential issue | 🔴 CriticalFix race condition in
reload_module_debounced: abort old task before registering new one.When
add_pendingis called with acomponent_idthat already has a pending task, the oldJoinHandleis dropped without being aborted. In Rust, dropping aJoinHandledoes not cancel the task—it continues running in the background. This causes both the old and new tasks to complete after their respective delays, resulting in duplicate reloads and defeating the debounce purpose.Change line 136 to cancel any existing pending task before adding the new one:
if let Some(_) = self.debounce_manager.cancel_pending(component_id).await { // Old task was cancelled } self.debounce_manager.add_pending(component_id.clone(), request, handle).await;Or alternatively, modify
add_pendingto abort the old handle internally before inserting:pub async fn add_pending( &self, component_id: String, request: ModuleReloadRequest, handle: tokio::task::JoinHandle<()>, ) { let mut pending = self.pending.lock().await; if let Some((_, old_handle)) = pending.remove(&component_id) { old_handle.abort(); } pending.insert(component_id, (request, handle)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/runtime/module_reload/manager.rs` around lines 125 - 136, The debounce logic currently drops previous JoinHandles without cancelling them, causing duplicate reloads; before calling debounce_manager.add_pending from reload_module_debounced, ensure any existing pending task for component_id is aborted—either call an existing debounce_manager.cancel_pending(component_id).await and handle/abort its JoinHandle, or update debounce_manager.add_pending to check pending.contains_key(&component_id) and call old_handle.abort() (use the ModuleReloadRequest and tokio::task::JoinHandle types from the signature) before inserting the new (request, handle) pair so only the newest task runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/rari/src/runtime/module_reload/manager.rs`:
- Around line 237-241: The current loop over handles only inspects the outer
JoinError from handle.await and drops the inner Result from reload_module, so
reload errors are ignored; update the loop in manager.rs (the for handle in
handles block) to match the nested Result<Result<(), RariError>, JoinError>
returned by handle.await, and when the inner Result is Err(e) (the RariError
from reload_module) log or propagate that error (similar to the sequential path)
instead of discarding it; ensure you reference handle.await and use both match
arms for the JoinError and the inner Result to report the reload failure via
error!(...) or return the error as appropriate.
- Around line 195-212: The _file_path parameter on reload_module_internal is
unused; either remove it from reload_module_internal's signature and from its
callers (reload_module and reload_with_retry) so the API reflects actual usage,
or keep the parameter but document its intended future purpose with a concise
comment in reload_module_internal and mark it as intentionally unused (e.g.,
keep the leading underscore). Update all references to the function
(reload_module → reload_with_retry → reload_module_internal) to match the chosen
approach to avoid unused-parameter warnings and confusion.
- Around line 125-136: The debounce logic currently drops previous JoinHandles
without cancelling them, causing duplicate reloads; before calling
debounce_manager.add_pending from reload_module_debounced, ensure any existing
pending task for component_id is aborted—either call an existing
debounce_manager.cancel_pending(component_id).await and handle/abort its
JoinHandle, or update debounce_manager.add_pending to check
pending.contains_key(&component_id) and call old_handle.abort() (use the
ModuleReloadRequest and tokio::task::JoinHandle types from the signature) before
inserting the new (request, handle) pair so only the newest task runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 481a4919-bafd-4de0-84b8-d990f5fe599e
📒 Files selected for processing (6)
crates/rari/src/runtime/module_reload/config.rscrates/rari/src/runtime/module_reload/manager.rscrates/rari/src/runtime/module_reload/mod.rscrates/rari/src/runtime/module_reload/stats.rscrates/rari/src/server/config/mod.rscrates/rari/src/server/core/mod.rs
💤 Files with no reviewable changes (5)
- crates/rari/src/runtime/module_reload/config.rs
- crates/rari/src/runtime/module_reload/mod.rs
- crates/rari/src/server/config/mod.rs
- crates/rari/src/server/core/mod.rs
- crates/rari/src/runtime/module_reload/stats.rs
- Add React global initialization with createElement, Fragment, and Suspense in component eval setup - Extract tag validation logic into reusable extractValidTags function for fetch operations - Include normalized tags in fetch cache key generation for proper cache differentiation - Update RSC renderer extension to depend on init_react for proper React setup - Improve module reload error handling with better match patterns for task results - Remove unused file_path parameter from reload_module_internal function - Enhance fetch cache merging to combine tags from multiple requests and maintain sorted order - Update deploy utils to support configurable start scripts with backup preservation - Improve navigation utils and props extractor with better type handling and edge cases
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/rari/src/runtime/module_reload/manager.rs (1)
221-249:⚠️ Potential issue | 🟡 MinorInconsistent error handling between sequential and parallel modes.
In sequential mode (line 223), the first error is propagated via
?and stops processing. In parallel mode (lines 237-244), errors are only logged and the function always returnsOk(()), even if all reloads fail.This asymmetry means callers cannot rely on the return value to detect batch failures when
parallel_reloadsis enabled. If this is intentional (fire-and-forget semantics for parallel), consider documenting it. Otherwise, consider collecting errors and returning a summary error:💡 Optional: Collect and report parallel failures
for handle in handles { match handle.await { Ok(Ok(())) => {} Ok(Err(e)) => { error!(error = %e, "Module reload failed in batch"); + // Optionally collect: errors.push(e); } Err(e) => { error!(error = %e, "Batch reload task failed"); } } } + // If collecting errors, return an aggregate error here }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/runtime/module_reload/manager.rs` around lines 221 - 249, Sequential mode currently propagates the first error via ? but the parallel branch just logs task errors and always returns Ok(()); change the parallel branch in the method to collect results from the spawned handles (the tokio::spawn handles created with manager.clone_for_task() calling manager.reload_module(&request.component_id, &request.file_path).await), aggregate any Err(e) results (both JoinError and inner Err from reload_module) into a single error (or a Vec of errors) and return Err(...) if any task failed so the function's return semantics match the sequential path; ensure you still log individual errors (using error!(error=%e,...)) while accumulating them and return Ok(()) only when all handles succeeded.packages/rari/src/proxy/execute-proxy.ts (1)
12-16:⚠️ Potential issue | 🟠 MajorSerialize proxy bootstrap across concurrent requests.
This
isInitialized()/initializeProxyFromManifest()pair is non-atomic. Inpackages/rari/src/proxy/executor.ts, initialization is only guarded by a boolean flag, so two cold-start requests can both callloadProxy()on the singleton at the same time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/proxy/execute-proxy.ts` around lines 12 - 16, The initialization race happens because executor.isInitialized() / initializeProxyFromManifest() are non-atomic; update the bootstrap to serialize concurrent initialization by adding an in-flight initialization promise or mutex in the executor singleton (e.g., in packages/rari/src/proxy/executor.ts) so multiple callers share the same initializeProxyFromManifest('./dist/proxy-manifest.json') promise instead of calling loadProxy() twice; change initializeProxyFromManifest to set and return a single Promise (and have isInitialized reflect completion) so callers in execute-proxy.ts that call initializeProxyFromManifest(...) will await the same promise and only one loadProxy() runs.
♻️ Duplicate comments (4)
packages/rari/src/image/Image.tsx (2)
103-119:⚠️ Potential issue | 🟠 Major
isVisiblecan still stayfalseafter props switch to immediate load.Line [115] initializes visibility once, but Lines [117-123] never set
isVisibletotruewhenshouldLoadImmediatelyflips fromfalsetotrue, sosrccan remain unset.💡 Proposed fix
function useImageLazyLoad( imgRef: React.RefObject<HTMLImageElement | null>, shouldPreload: boolean, unoptimized: boolean, loading: 'lazy' | 'eager', ) { const shouldLoadImmediately = shouldPreload || unoptimized || loading === 'eager' - const prevShouldLoadImmediatelyRef = useRef(shouldLoadImmediately) - - const getInitialVisibility = () => { - if (shouldLoadImmediately && !prevShouldLoadImmediatelyRef.current) { - prevShouldLoadImmediatelyRef.current = true - return true - } - - return shouldLoadImmediately - } - - const [isVisible, setIsVisible] = useState(getInitialVisibility) - - useEffect(() => { - prevShouldLoadImmediatelyRef.current = shouldLoadImmediately - }, [shouldLoadImmediately]) + const [isVisible, setIsVisible] = useState(shouldLoadImmediately) + + useEffect(() => { + if (shouldLoadImmediately) + setIsVisible(true) + }, [shouldLoadImmediately]) useEffect(() => { if (shouldLoadImmediately) returnAlso applies to: 121-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/image/Image.tsx` around lines 103 - 119, The isVisible state may remain false when shouldLoadImmediately flips true because getInitialVisibility only runs once; update the useEffect that watches shouldLoadImmediately to detect the transition (prevShouldLoadImmediatelyRef.current === false && shouldLoadImmediately === true) and call setIsVisible(true) before updating prevShouldLoadImmediatelyRef.current; this preserves the initial getInitialVisibility logic while ensuring isVisible is set when shouldLoadImmediately changes to immediate load (refer to shouldLoadImmediately, prevShouldLoadImmediatelyRef, getInitialVisibility, isVisible, setIsVisible, and the existing useEffect).
292-304:⚠️ Potential issue | 🟠 MajorFixed-width rendering still drops DPR candidates (
srcSet).Line [297] makes
shouldUseSrcSetfalse for fixed width, so Line [303] omitssrcSet; high-density screens lose sharper variants.💡 Proposed fix
const defaultWidth = imgWidth || 1920 const sizesArray = imgWidth ? [imgWidth] : DEFAULT_DEVICE_SIZES @@ const shouldUseSrcSet = !imgWidth || sizesArray.length > 1 || sizesArray[0] !== defaultWidth + const densitySrcSet = imgWidth + ? [ + `${loader ? loader({ src: finalSrc, width: imgWidth, quality }) : buildImageUrl(finalSrc, imgWidth, quality)} 1x`, + `${loader ? loader({ src: finalSrc, width: imgWidth * 2, quality }) : buildImageUrl(finalSrc, imgWidth * 2, quality)} 2x`, + ].join(', ') + : undefined const imgElement = ( <img ref={imgRef} src={isVisible ? mainSrc : undefined} - srcSet={isVisible && shouldUseSrcSet ? buildSrcSetString(sizesArray, finalSrc, quality, undefined, loader) : undefined} - sizes={shouldUseSrcSet ? sizes : undefined} + srcSet={isVisible ? (densitySrcSet || (shouldUseSrcSet ? buildSrcSetString(sizesArray, finalSrc, quality, undefined, loader) : undefined)) : undefined} + sizes={densitySrcSet ? undefined : (shouldUseSrcSet ? sizes : undefined)}Also applies to: 318-319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/image/Image.tsx` around lines 292 - 304, The component currently sets shouldUseSrcSet = false when a fixed imgWidth is provided, which prevents rendering DPR (density) candidates and yields low-res images on high-density screens; update the logic around defaultWidth, sizesArray and shouldUseSrcSet so that even for a single fixed width you still build a density-based srcSet via buildSrcSetString (i.e., allow srcSet when imgWidth is present by detecting the single-size case and generating DPR descriptors), and mirror the same change where srcSet/sizes are set in the imgElement and the corresponding alternate branch later (the references are defaultWidth, sizesArray, shouldUseSrcSet, buildSrcSetString, and imgElement).packages/rari/src/router/sitemap-generator.ts (1)
310-318:⚠️ Potential issue | 🟡 MinorValidate that
sanitizedIdis not empty after sanitization.If the original
idconsists entirely of characters that matchSANITIZE_ID_REGEX(e.g.,"@#$%"), thesanitizedIdwill be an empty string, resulting in a file named.xml. This could cause unexpected behavior or overwrite issues.🛡️ Proposed fix
const sanitizedId = String(id).replace(SANITIZE_ID_REGEX, '_') + if (!sanitizedId) { + throw new Error(`Invalid sitemap id "${id}": results in empty string after sanitization`) + } + const existingId = seenSanitizedIds.get(sanitizedId) if (existingId !== undefined) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/sitemap-generator.ts` around lines 310 - 318, After creating sanitizedId from String(id).replace(SANITIZE_ID_REGEX, '_'), validate that sanitizedId is not the empty string; if it is, either assign a safe default (e.g., '_') or throw a descriptive error to avoid generating a ".xml" filename. Update the sitemap generation logic that uses sanitizedId (and the duplicate check via seenSanitizedIds) to operate on the validated/normalized value so collisions and file writes are safe; reference the variables sanitizedId, SANITIZE_ID_REGEX, sitemapIds, and seenSanitizedIds when making the change.packages/rari/src/proxy/shared/utils.ts (1)
15-15:⚠️ Potential issue | 🟠 MajorPreserve duplicate
responseHeadersvalues in continue flows.
handleDirectResponse()now keeps repeated header names, butextractProxyHeaders()still flattensresponseHeadersto a single string. In thex-rari-proxy-continuepath, multipleSet-Cookievalues will still collapse to the last one.💡 Suggested fix
export interface SimpleProxyResult { continue: boolean redirect?: { @@ } rewrite?: string requestHeaders?: Record<string, string> - responseHeaders?: Record<string, string> + responseHeaders?: Record<string, string | string[]> response?: { status: number headers: Record<string, string | string[]> @@ -export function extractProxyHeaders(headers: ResponseLike['headers']): { requestHeaders?: Record<string, string>, responseHeaders?: Record<string, string> } { +export function extractProxyHeaders(headers: ResponseLike['headers']): { requestHeaders?: Record<string, string>, responseHeaders?: Record<string, string | string[]> } { const requestHeaders: Record<string, string> = {} - const responseHeaders: Record<string, string> = {} + const responseHeaders: Record<string, string | string[]> = {} @@ if (key.startsWith('x-rari-proxy-request-')) { const headerName = key.replace('x-rari-proxy-request-', '') requestHeaders[headerName] = value } else if (!key.startsWith('x-rari-proxy-')) { - responseHeaders[key] = value + const existing = responseHeaders[key] + responseHeaders[key] = existing === undefined + ? value + : Array.isArray(existing) ? [...existing, value] : [existing, value] } }) }Also applies to: 68-80, 90-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/proxy/shared/utils.ts` at line 15, extractProxyHeaders currently flattens responseHeaders into a Record<string,string>, which loses duplicate header values (e.g. multiple Set-Cookie) used by the x-rari-proxy-continue path; update extractProxyHeaders (and any callers that build responseHeaders in the x-rari-proxy-continue flow) to preserve duplicate headers by using Record<string,string[]> or an array of header tuples, mirror the behavior of handleDirectResponse, and ensure the downstream serialization for the continue flow emits repeated header entries instead of joining them into one string; reference extractProxyHeaders, handleDirectResponse, responseHeaders, and x-rari-proxy-continue when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rari/src/rsc/rendering/core/js/component_eval_setup.js`:
- Around line 45-50: The code assumes document.head exists before calling
document.head.appendChild, which can throw in partial DOM environments; update
the block that creates the style with id 'spinner-keyframes' to first ensure a
head element is available (check document.head and fall back to
document.getElementsByTagName('head')[0] or skip appending if none), and only
call appendChild on that validated head reference so the creation of the style
node and setting style.textContent remains safe in non-browser contexts.
- Around line 5-12: The element creation currently collapses falsy keys (e.g., 0
or '') to null and leaves the original key inside props; update the logic in the
element construction (the block that builds element with $typeof, type, props
and key) to: 1) compute key using a hasOwnProperty check (so valid falsy values
are preserved) rather than `props?.key || null`, and 2) create a shallow copy of
props with the `key` property removed (do not mutate the incoming props) and use
that copy for element.props before you merge children (the code that currently
spreads props into element.props and later merges children should use the
props-without-key copy).
- Around line 21-24: The current null no-op fallbacks for _jsx and _jsxs will
drop output; update their fallback factories so they delegate to the existing
runtime renderers instead of returning null: when setting _jsx and _jsxs, use
globalThis['~react']?.jsxRuntime?.jsx || globalThis.jsx ||
((...args)=>globalThis.React.createElement(...args)) and similarly for jsxs
(referencing _jsx, _jsxs, globalThis.jsx, globalThis.jsxs, and
globalThis.React.createElement) so the variables delegate to the working
implementations rather than returning null.
In `@crates/rari/src/runtime/ext/web/init_fetch.js`:
- Around line 134-140: Remove the redundant outer check that inspects `tags`
(the lines referencing `const tags = init?.rari?.tags ?? init?.next?.tags` and
`if (tags && Array.isArray(tags))`) because `extractValidTags(init)` already
returns an empty array for missing/invalid tags; instead call
`extractValidTags(init)`, check its length, and only then set `options.tags =
JSON.stringify(validTags)` — update references in this block around
`extractValidTags`, `validTags`, and `options.tags` so the code relies solely on
`extractValidTags(init)` for validation.
In `@packages/deploy/src/utils.ts`:
- Around line 147-168: getRariVersion is declared as getRariVersion(cwd: string)
but railway.ts and render.ts call it without arguments, causing a TS error;
change the function signature in utils.ts (getRariVersion) to accept an optional
cwd with a default (e.g., cwd = process.cwd()) so callers can omit it, and keep
existing behavior when a cwd is provided; update only the signature in
getRariVersion and leave all callers (railway.ts, render.ts, and existing
getRariVersion(cwd) usage) unchanged.
In `@packages/rari/src/proxy/execute-proxy.ts`:
- Around line 24-27: The event.waitUntil handler currently pushes raw promises
into waitUntilPromises which can trigger unhandled-rejection before the final
Promise.allSettled; update the waitUntil implementation (the event object and
any similar block at lines 42-50) to attach a rejection handler when registering
the promise (e.g., add .catch(err => store/log the error or noop) or wrap it so
rejections are observed) while still pushing the original promise (or its
wrapped variant) into waitUntilPromises so the existing Promise.allSettled logic
remains valid.
In `@packages/rari/src/proxy/shared/utils.ts`:
- Around line 103-107: The merge logic in result.headers.forEach incorrectly
treats falsy values (like an empty string) as absent because it uses if
(headers[lowerKey]); change that presence check to an explicit existence
test—e.g. use Object.prototype.hasOwnProperty.call(headers, lowerKey) or typeof
headers[lowerKey] !== 'undefined'—so when merging into headers (lowerKey,
existing, value) you correctly accumulate multiple header values instead of
overwriting when the first value is an empty string.
In `@test/unit/router/props-extractor.test.ts`:
- Around line 25-28: MetadataResult.title can be an object but
PageMetadata.title (used in ClientRouter.tsx) is typed/treated as string; update
the code that assigns metadata to document.title and that serializes the
x-rari-metadata header to coerce title objects to a string by using the object's
default property (or a sensible fallback like template applied or empty string)
before assignment/serialization. Locate the mergeMetadata output
(MetadataResult.title) and the places in ClientRouter.tsx where
PageMetadata.title is read/assigned (document.title assignment and metadata
header serialization) and add a small conversion helper that accepts string | {
default?: string; template?: string } and returns the proper string to use.
---
Outside diff comments:
In `@crates/rari/src/runtime/module_reload/manager.rs`:
- Around line 221-249: Sequential mode currently propagates the first error via
? but the parallel branch just logs task errors and always returns Ok(());
change the parallel branch in the method to collect results from the spawned
handles (the tokio::spawn handles created with manager.clone_for_task() calling
manager.reload_module(&request.component_id, &request.file_path).await),
aggregate any Err(e) results (both JoinError and inner Err from reload_module)
into a single error (or a Vec of errors) and return Err(...) if any task failed
so the function's return semantics match the sequential path; ensure you still
log individual errors (using error!(error=%e,...)) while accumulating them and
return Ok(()) only when all handles succeeded.
In `@packages/rari/src/proxy/execute-proxy.ts`:
- Around line 12-16: The initialization race happens because
executor.isInitialized() / initializeProxyFromManifest() are non-atomic; update
the bootstrap to serialize concurrent initialization by adding an in-flight
initialization promise or mutex in the executor singleton (e.g., in
packages/rari/src/proxy/executor.ts) so multiple callers share the same
initializeProxyFromManifest('./dist/proxy-manifest.json') promise instead of
calling loadProxy() twice; change initializeProxyFromManifest to set and return
a single Promise (and have isInitialized reflect completion) so callers in
execute-proxy.ts that call initializeProxyFromManifest(...) will await the same
promise and only one loadProxy() runs.
---
Duplicate comments:
In `@packages/rari/src/image/Image.tsx`:
- Around line 103-119: The isVisible state may remain false when
shouldLoadImmediately flips true because getInitialVisibility only runs once;
update the useEffect that watches shouldLoadImmediately to detect the transition
(prevShouldLoadImmediatelyRef.current === false && shouldLoadImmediately ===
true) and call setIsVisible(true) before updating
prevShouldLoadImmediatelyRef.current; this preserves the initial
getInitialVisibility logic while ensuring isVisible is set when
shouldLoadImmediately changes to immediate load (refer to shouldLoadImmediately,
prevShouldLoadImmediatelyRef, getInitialVisibility, isVisible, setIsVisible, and
the existing useEffect).
- Around line 292-304: The component currently sets shouldUseSrcSet = false when
a fixed imgWidth is provided, which prevents rendering DPR (density) candidates
and yields low-res images on high-density screens; update the logic around
defaultWidth, sizesArray and shouldUseSrcSet so that even for a single fixed
width you still build a density-based srcSet via buildSrcSetString (i.e., allow
srcSet when imgWidth is present by detecting the single-size case and generating
DPR descriptors), and mirror the same change where srcSet/sizes are set in the
imgElement and the corresponding alternate branch later (the references are
defaultWidth, sizesArray, shouldUseSrcSet, buildSrcSetString, and imgElement).
In `@packages/rari/src/proxy/shared/utils.ts`:
- Line 15: extractProxyHeaders currently flattens responseHeaders into a
Record<string,string>, which loses duplicate header values (e.g. multiple
Set-Cookie) used by the x-rari-proxy-continue path; update extractProxyHeaders
(and any callers that build responseHeaders in the x-rari-proxy-continue flow)
to preserve duplicate headers by using Record<string,string[]> or an array of
header tuples, mirror the behavior of handleDirectResponse, and ensure the
downstream serialization for the continue flow emits repeated header entries
instead of joining them into one string; reference extractProxyHeaders,
handleDirectResponse, responseHeaders, and x-rari-proxy-continue when making the
change.
In `@packages/rari/src/router/sitemap-generator.ts`:
- Around line 310-318: After creating sanitizedId from
String(id).replace(SANITIZE_ID_REGEX, '_'), validate that sanitizedId is not the
empty string; if it is, either assign a safe default (e.g., '_') or throw a
descriptive error to avoid generating a ".xml" filename. Update the sitemap
generation logic that uses sanitizedId (and the duplicate check via
seenSanitizedIds) to operate on the validated/normalized value so collisions and
file writes are safe; reference the variables sanitizedId, SANITIZE_ID_REGEX,
sitemapIds, and seenSanitizedIds when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75adc588-a5e2-492c-bd6d-13f79be47fb0
📒 Files selected for processing (18)
crates/rari/src/rsc/rendering/core/js/component_eval_setup.jscrates/rari/src/runtime/ext/web/init_fetch.jscrates/rari/src/runtime/module_reload/manager.rscrates/rari/src/server/middleware/request_context.rspackages/deploy/src/utils.tspackages/rari/src/image/Image.tsxpackages/rari/src/proxy/execute-proxy.tspackages/rari/src/proxy/matcher.tspackages/rari/src/proxy/shared/utils.tspackages/rari/src/router/navigation-utils.tspackages/rari/src/router/props-extractor.tspackages/rari/src/router/robots-generator.tspackages/rari/src/router/sitemap-generator.tspackages/rari/src/runtime/shared/get-client-component.tspackages/rari/src/shared/http-utils.tspackages/rari/src/vite/image-scanner.tstest/unit/router/navigation-utils.test.tstest/unit/router/props-extractor.test.ts
- Refactor React.createElement to explicitly handle key prop separation from props object - Update _jsx and _jsxs fallbacks to use globalThis.jsx/jsxs or createElement as final fallback - Add defensive head element check before appending spinner keyframes style - Simplify fetch cache tags extraction logic and remove redundant array check - Fix timeout option handling to properly coalesce undefined values with OR operator - Enhance module reload error collection to capture and return first error from batch - Add default parameter to getRariVersion function for process.cwd() fallback - Simplify image lazy load state management by removing ref-based visibility tracking - Add useDprDescriptors parameter to buildSrcSetString for device pixel ratio support - Update image srcset generation to support both width and DPR descriptor formats - Improve props extractor and sitemap generator type safety and error handling
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/rari/src/rsc/rendering/core/js/component_eval_setup.js (2)
52-63: 🧹 Nitpick | 🔵 TrivialConsider guarding
globalThis.ReactinsideLoadingSpinner.If a consumer somehow unsets or replaces
globalThis.Reactafter initial setup, this function would throw. A defensive check would improve resilience:Suggested fix
globalThis.LoadingSpinner = function () { + if (!globalThis.React?.createElement) + return null return globalThis.React.createElement('div', {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/rsc/rendering/core/js/component_eval_setup.js` around lines 52 - 63, The LoadingSpinner function currently assumes globalThis.React exists and will throw if it's unset; update the globalThis.LoadingSpinner implementation to first check that globalThis.React and React.createElement are defined (e.g., if (!globalThis.React || typeof globalThis.React.createElement !== 'function') return null) and only call React.createElement when safe; reference the LoadingSpinner function to add this defensive guard and return a safe fallback (null or a no-op) when React is unavailable.
66-78: 🧹 Nitpick | 🔵 TrivialConsider guarding
globalThis.ReactinsideDefaultLoadingfor consistency.Same defensive consideration as
LoadingSpinner— ifglobalThis.Reactis unset after setup, this throws.Suggested fix
globalThis.DefaultLoading = function () { + if (!globalThis.React?.createElement) + return null return globalThis.React.createElement('div', {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/rsc/rendering/core/js/component_eval_setup.js` around lines 66 - 78, DefaultLoading currently assumes globalThis.React exists and will throw if React is unset; update the DefaultLoading function to guard access to globalThis.React (same pattern used for LoadingSpinner) — e.g., check if globalThis.React is defined before calling createElement and return a safe fallback (null or a plain DOM string/div) when React is missing; ensure the function still returns the React element when globalThis.React and globalThis.LoadingSpinner are present, and reference DefaultLoading, globalThis.React, and globalThis.LoadingSpinner when making the change.crates/rari/src/server/middleware/request_context.rs (1)
78-108:⚠️ Potential issue | 🟠 MajorCache key inconsistency enables unintended tag accumulation.
The Rust cache excludes
tagsfrom the cache key (line 81), but the JavaScript deduplication layer ininit_fetch.jsincludes tags in the deduplication key via:tags:<json>suffix. This inconsistency creates problematic behavior:
- JS deduplication: requests with different tags generate different dedup keys
- Rust cache: requests with different tags reuse the same cache entry
Consequence: when multiple requests access the same cached URL with different tags, the cached entry accumulates all tags from every request that hits it (lines 137, 162, 191 perform tag merging and write back to cache). This means:
- A cached response originally fetched with tag [A] will be mutated to include tag [B] when accessed by a different request
- Later requests for the same URL may receive tags they never requested
- Tag values become unpredictable across requests
Either align cache key generation to include tags (matching JS behavior) or avoid mutating cached entries with request-specific tags.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/server/middleware/request_context.rs` around lines 78 - 108, The cache key generation in generate_cache_key currently excludes "tags" (cache_relevant_options filter) causing Rust cache entries to be shared across different tag sets and enabling tag accumulation; fix by including tags in the cache-relevant options (remove "tags" from the exclusion) and ensure the tags value is serialized deterministically to match the JS deduplication suffix (e.g., use a stable JSON/string representation or sorted list) so that generate_cache_key produces distinct keys for different tag sets and prevents mutation/merging of tags into a shared cache entry.crates/rari/src/runtime/module_reload/manager.rs (1)
125-139:⚠️ Potential issue | 🟠 MajorDebounce scheduling still has a race window before pending registration.
Because the task is spawned at Line 125 before Line 137 cancellation and Line 139 registration, execution can interleave in a way that allows stale/untracked reloads.
Suggested ordering fix
- let handle = tokio::spawn(async move { - tokio::time::sleep(debounce_delay).await; - - manager.debounce_manager.remove_pending(&component_id_clone).await; - - if let Err(e) = manager.reload_module(&component_id_clone, &file_path_clone).await { - error!("Failed to reload module {}: {}", component_id_clone, e); - } - }); - let request = ModuleReloadRequest::new(component_id.clone(), file_path.clone()); - - self.debounce_manager.cancel_pending(&component_id).await; - - self.debounce_manager.add_pending(component_id.clone(), request, handle).await; + self.debounce_manager.cancel_pending(&component_id).await; + + let handle = tokio::spawn(async move { + tokio::time::sleep(debounce_delay).await; + manager.debounce_manager.remove_pending(&component_id_clone).await; + if let Err(e) = manager.reload_module(&component_id_clone, &file_path_clone).await { + error!("Failed to reload module {}: {}", component_id_clone, e); + } + }); + + self.debounce_manager + .add_pending(component_id.clone(), request, handle) + .await;Run this read-only check to confirm whether
remove_pending/cancel_pendingare keyed only by component id and whetherdebounce_delay_mscan be zero:#!/bin/bash set -euo pipefail fd '.*\.rs$' crates/rari/src/runtime/module_reload rg -n -C3 'struct DebounceManager|fn add_pending|fn cancel_pending|fn remove_pending' crates/rari/src/runtime/module_reload -g '*.rs' rg -n -C3 'struct ReloadConfig|debounce_delay_ms' crates/rari/src/runtime/module_reload -g '*.rs'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/runtime/module_reload/manager.rs` around lines 125 - 139, The background task is spawned before the pending request is cancelled/registered, creating a race between the spawned task's remove_pending call and the subsequent cancel_pending/add_pending; reorder operations so you first construct the ModuleReloadRequest, call debounce_manager.cancel_pending(&component_id).await, then spawn the tokio task (the code that sleeps and calls remove_pending/reload_module) and finally call debounce_manager.add_pending(component_id.clone(), request, handle).await; ensure you reference debounce_manager, cancel_pending, add_pending, remove_pending, ModuleReloadRequest, debounce_delay (or debounce_delay_ms) and the spawned task's handle when applying this change.
♻️ Duplicate comments (6)
packages/rari/src/image/Image.tsx (2)
292-304:⚠️ Potential issue | 🟠 MajorFixed-width rendering still drops DPR
srcSetcandidates.When
imgWidthis provided,shouldUseSrcSetbecomes false and the<img>is rendered withoutsrcSet, which degrades sharpness on high-density displays.💡 Proposed fix (add density srcSet for fixed-width path)
const defaultWidth = imgWidth || 1920 const sizesArray = imgWidth ? [imgWidth] : DEFAULT_DEVICE_SIZES @@ - const shouldUseSrcSet = !imgWidth || sizesArray.length > 1 || sizesArray[0] !== defaultWidth + const shouldUseSrcSet = !imgWidth || sizesArray.length > 1 || sizesArray[0] !== defaultWidth + const densitySrcSet = imgWidth + ? [ + `${loader ? loader({ src: finalSrc, width: imgWidth, quality }) : buildImageUrl(finalSrc, imgWidth, quality)} 1x`, + `${loader ? loader({ src: finalSrc, width: imgWidth * 2, quality }) : buildImageUrl(finalSrc, imgWidth * 2, quality)} 2x`, + ].join(', ') + : undefined @@ - srcSet={isVisible && shouldUseSrcSet ? buildSrcSetString(sizesArray, finalSrc, quality, undefined, loader) : undefined} - sizes={shouldUseSrcSet ? sizes : undefined} + srcSet={isVisible ? (densitySrcSet || (shouldUseSrcSet ? buildSrcSetString(sizesArray, finalSrc, quality, undefined, loader) : undefined)) : undefined} + sizes={densitySrcSet ? undefined : (shouldUseSrcSet ? sizes : undefined)} @@ - if (!shouldUseSrcSet) + if (!shouldUseSrcSet && !densitySrcSet) return imgElementAlso applies to: 318-319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/image/Image.tsx` around lines 292 - 304, The image component currently disables srcSet when a fixed imgWidth is passed (because shouldUseSrcSet becomes false), which drops DPR candidates and reduces sharpness on high-density screens; update the logic around shouldUseSrcSet and the img rendering so that when imgWidth is provided you still generate a density-based srcSet (using buildSrcSetString) for DPR variants (e.g., 1x/2x/3x) and pass that into the imgElement srcSet prop while keeping sizes appropriate; specifically adjust calculation of shouldUseSrcSet (or add a new flag) and ensure buildSrcSetString is invoked for the fixed-width path (referencing imgWidth, sizesArray, shouldUseSrcSet, buildSrcSetString, mainSrc, and imgElement) so fixed-width images include DPR srcSet candidates.
103-123:⚠️ Potential issue | 🟠 Major
isVisibleis still not synchronized when immediate-load conditions change.
isVisibleis initialized once (Line 115). IfshouldLoadImmediatelyflips fromfalsetotruelater, state is never updated, sosrccan remain unset.💡 Proposed fix
- const prevShouldLoadImmediatelyRef = useRef(shouldLoadImmediately) - - const getInitialVisibility = () => { - if (shouldLoadImmediately && !prevShouldLoadImmediatelyRef.current) { - prevShouldLoadImmediatelyRef.current = true - return true - } - - return shouldLoadImmediately - } - - const [isVisible, setIsVisible] = useState(getInitialVisibility) - - useEffect(() => { - prevShouldLoadImmediatelyRef.current = shouldLoadImmediately - }, [shouldLoadImmediately]) + const [isVisible, setIsVisible] = useState(shouldLoadImmediately) + + useEffect(() => { + if (shouldLoadImmediately) + setIsVisible(true) + }, [shouldLoadImmediately])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/image/Image.tsx` around lines 103 - 123, isVisible is only initialized once via getInitialVisibility so when shouldLoadImmediately flips true later the state isn't updated; add an effect that watches shouldLoadImmediately (and optionally prevShouldLoadImmediatelyRef.current) and calls setIsVisible(true) when shouldLoadImmediately transitions to true, updating prevShouldLoadImmediatelyRef.current accordingly; reference the existing symbols getInitialVisibility, shouldLoadImmediately, prevShouldLoadImmediatelyRef, isVisible, and setIsVisible to locate where to add this synchronization.packages/rari/src/router/navigation-utils.ts (1)
79-82:⚠️ Potential issue | 🟠 MajorOptional catch-all still matches malformed segments at end-of-path.
At Line 79-82,
optional-catch-allreturns success even whensegment.paramis missing. This still allows malformed manifest entries to match and silently drop params; it should fail closed like the dynamic/catch-all paths.Proposed fix
if (actualIndex >= actualSegments.length) { if (segment.type === 'optional-catch-all') { + if (!segment.param) + return { success: false, newIndex: actualIndex } handleOptionalCatchAll(actualIndex, actualSegments.length, segment.param, params) return { success: true, newIndex: actualIndex } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/navigation-utils.ts` around lines 79 - 82, The optional-catch-all branch currently returns success without validating that segment.param exists, allowing malformed manifests to match; update the branch around handleOptionalCatchAll(actualIndex, actualSegments.length, segment.param, params) to first check that segment.param is present (non-empty) and, if missing, return { success: false } (same failure behavior as dynamic/catch-all handling); otherwise call handleOptionalCatchAll and return the success result as before so missing param cases fail closed.packages/rari/src/proxy/shared/utils.ts (1)
103-107:⚠️ Potential issue | 🟡 MinorUse explicit key-existence checks when merging response headers.
Line 105 still uses truthiness (
if (headers[lowerKey])), so an existing empty-string value is treated as absent and can be overwritten instead of merged.Proposed fix
result.headers.forEach((value: string, key: string) => { const lowerKey = key.toLowerCase() - if (headers[lowerKey]) { + if (Object.prototype.hasOwnProperty.call(headers, lowerKey)) { const existing = headers[lowerKey] headers[lowerKey] = Array.isArray(existing) ? [...existing, value] : [existing, value] } else { headers[lowerKey] = value } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/proxy/shared/utils.ts` around lines 103 - 107, The merge logic uses a truthiness check (if (headers[lowerKey])) which treats empty-string or other falsy values as missing; change that to an explicit existence check such as Object.prototype.hasOwnProperty.call(headers, lowerKey) or (lowerKey in headers) so keys with falsy values are correctly merged; update the block in the result.headers.forEach loop referencing headers, lowerKey and existing to use that explicit check before deciding whether to append to existing or create a new array.packages/rari/src/proxy/execute-proxy.ts (1)
24-27:⚠️ Potential issue | 🟠 MajorObserve
waitUntilrejections when registered, not only infinally.Line 26 stores the raw promise. If it rejects before
finallyruns, it can still hit unhandled-rejection paths.Proposed fix
const event = { waitUntil: (promise: Promise<unknown>) => { - waitUntilPromises.push(promise) + const observedPromise = promise.catch((error) => { + throw error + }) + waitUntilPromises.push(observedPromise) }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/proxy/execute-proxy.ts` around lines 24 - 27, The event.waitUntil implementation currently pushes the raw promise (waitUntilPromises.push(promise)) which can reject before finally handling and cause unhandled rejections; wrap the incoming promise before storing so its rejection is observed immediately — e.g., create a wrapped = promise.catch(err => { /* record/log the error or push to a waitUntilErrors array */ throw err; }); and push that wrapped promise into waitUntilPromises inside the waitUntil handler (referencing event.waitUntil and waitUntilPromises) so rejections are captured when registered rather than only in the later finally.packages/rari/src/router/sitemap-generator.ts (1)
253-259:⚠️ Potential issue | 🟠 MajorResolve relative imports from the real sitemap file, not the virtual module ID.
On Line 257, imports coming from the virtual entry use
VIRTUAL_SITEMAP_IDasimporter, so./fooresolves from the virtual module instead ofsitemapFile.path. Returningpath.resolve(...)here also bypasses normal extension resolution, so common extensionless imports can still fail.🔧 Proposed fix
function createSitemapPlugin(sitemapFile: SitemapFile, sourceCode: string) { return { name: 'virtual-sitemap', - resolveId(id: string, importer?: string) { + async resolveId(id: string, importer?: string) { if (id === VIRTUAL_SITEMAP_ID) return id - if (id.startsWith('.')) - return path.resolve(path.dirname(importer ?? sitemapFile.path), id) - - return null + if (!id.startsWith('.')) + return null + + const baseImporter = !importer || importer === VIRTUAL_SITEMAP_ID + ? sitemapFile.path + : importer + + return this.resolve(id, baseImporter, { skipSelf: true }) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/sitemap-generator.ts` around lines 253 - 259, The resolveId implementation currently resolves relative imports with path.resolve which treats the virtual module as the importer and bypasses normal extension resolution; update resolveId (the function named resolveId and references to VIRTUAL_SITEMAP_ID and sitemapFile.path) so that when id.startsWith('.') you call this.resolve(id, correctedImporter) instead of path.resolve: if importer === VIRTUAL_SITEMAP_ID use sitemapFile.path as the importer, otherwise use the provided importer, then return the resolved result (e.g., result?.id) or null so Vite/Rollup can perform normal extension resolution and plugin chaining.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rari/src/runtime/module_reload/manager.rs`:
- Around line 237-245: The parallel batch branch currently logs errors from each
task (matching on handle.await) but still returns Ok(()) so callers can't detect
failures; modify the join loop in manager.rs that processes each handle.await
(the code matching Ok(Ok(())), Ok(Err(e)), Err(e)) to collect errors (e.g., the
first encountered error or aggregate them) into a local variable and after the
loop return Err(...) when any error was seen instead of always returning Ok(());
ensure you propagate an appropriate Error type consistent with the function's
signature so the caller receives a failure result.
In `@packages/deploy/src/utils.ts`:
- Around line 170-175: The exported function updatePackageJsonForProvider has a
parameter typed as ProviderConfig but the ProviderConfig interface is not
exported; fix by either exporting the interface (add export before interface
ProviderConfig) so the function's public signature is valid, or if
updatePackageJsonForProvider is meant to be internal, remove the export keyword
from updatePackageJsonForProvider (and optionally keep it out of the barrel
index.ts); update whichever choice you make so signatures and re-exports are
consistent.
In `@packages/rari/src/proxy/shared/utils.ts`:
- Around line 120-124: The current truthy check drops empty-string response
bodies; in the response handling in packages/rari/src/proxy/shared/utils.ts
adjust the guards so empty strings are preserved by removing the `result.body
&&` from the first condition and ensuring the other branch uses a null/undefined
check (`result.body != null`) instead of a truthy check; locate the logic that
assigns `body` from `result.body` and update those conditions accordingly to
allow `''` through while still treating null/undefined as absent.
In `@packages/rari/src/router/sitemap-generator.ts`:
- Around line 228-246: determineModuleType advertises support for .mjs, .cjs and
.json but findSitemapFiles() only defaults to ['.ts','.tsx','.js','.jsx'], so
those formats will be skipped; update findSitemapFiles() to include '.mjs',
'.cjs', and '.json' in its default extensions array (or alternatively remove
those extensions from determineModuleType) so both functions agree, and ensure
any callers relying on the default behavior pick up the new extensions; refer to
the determineModuleType and findSitemapFiles identifiers when making the change.
- Around line 320-322: The shard invocation currently coerces id to a string
before calling module.default (see generateSitemaps() loop where sitemapData is
created), which can change semantics for numeric IDs; change the call to pass
the original id value (do not use String(id)) when calling module.default so the
shard receives the original type, and only apply String(id) where you build
filenames or diagnostic messages (e.g., sitemap filename construction or
logging) to avoid changing lookup/pagination behavior.
In `@packages/rari/src/runtime/shared/get-client-component.ts`:
- Around line 146-161: resolveByPathWithExport currently only tries the original
path and a stripped './' variant, which is asymmetric to
getClientComponentAsync's use of getPathVariants; update resolveByPathWithExport
to mirror getClientComponentAsync by trying both adding and removing the './'
prefix (i.e., produce the same path variants as getPathVariants) and call
resolveByPath for each variant until one returns non-null, ensuring lookup works
whether components are registered as 'foo' or './foo'; reference
resolveByPathWithExport, resolveByPath, and getPathVariants when making the
change.
In `@packages/rari/src/vite/image-scanner.ts`:
- Around line 244-245: The cache-key normalization currently uses logical OR
which treats explicit 0 as falsy; in the const key construction (using
imageUsage.src, imageUsage.width, imageUsage.quality and DEFAULT_QUALITY) and
the other occurrence around the same logic near the image scanning logic,
replace the `||` fallbacks with nullish coalescing `??` so only null/undefined
get the defaults (e.g., use imageUsage.width ?? 'auto' and imageUsage.quality ??
DEFAULT_QUALITY); update both places referencing those symbols to preserve
explicit zero values.
- Around line 293-306: The brace tracking for template expressions in
extractBalancedBraces is sharing braceCount with the outer props-object, which
can desynchronize extraction when nested braces appear inside `${...}`;
introduce a separate templateBraceCount (or templateDepth) to track braces
specifically inside template expressions and update that variable when you
detect the `${` start (where currently you do `braceCount++`) and when you see
matching `}` inside template mode (where you currently do `braceCount--`), keep
the outer braceCount untouched except for object-level `{`/`}` handling, and
ensure string/template start/end logic (stringChar, inString) references the new
template-specific counter so balanced extraction returns correct ranges.
---
Outside diff comments:
In `@crates/rari/src/rsc/rendering/core/js/component_eval_setup.js`:
- Around line 52-63: The LoadingSpinner function currently assumes
globalThis.React exists and will throw if it's unset; update the
globalThis.LoadingSpinner implementation to first check that globalThis.React
and React.createElement are defined (e.g., if (!globalThis.React || typeof
globalThis.React.createElement !== 'function') return null) and only call
React.createElement when safe; reference the LoadingSpinner function to add this
defensive guard and return a safe fallback (null or a no-op) when React is
unavailable.
- Around line 66-78: DefaultLoading currently assumes globalThis.React exists
and will throw if React is unset; update the DefaultLoading function to guard
access to globalThis.React (same pattern used for LoadingSpinner) — e.g., check
if globalThis.React is defined before calling createElement and return a safe
fallback (null or a plain DOM string/div) when React is missing; ensure the
function still returns the React element when globalThis.React and
globalThis.LoadingSpinner are present, and reference DefaultLoading,
globalThis.React, and globalThis.LoadingSpinner when making the change.
In `@crates/rari/src/runtime/module_reload/manager.rs`:
- Around line 125-139: The background task is spawned before the pending request
is cancelled/registered, creating a race between the spawned task's
remove_pending call and the subsequent cancel_pending/add_pending; reorder
operations so you first construct the ModuleReloadRequest, call
debounce_manager.cancel_pending(&component_id).await, then spawn the tokio task
(the code that sleeps and calls remove_pending/reload_module) and finally call
debounce_manager.add_pending(component_id.clone(), request, handle).await;
ensure you reference debounce_manager, cancel_pending, add_pending,
remove_pending, ModuleReloadRequest, debounce_delay (or debounce_delay_ms) and
the spawned task's handle when applying this change.
In `@crates/rari/src/server/middleware/request_context.rs`:
- Around line 78-108: The cache key generation in generate_cache_key currently
excludes "tags" (cache_relevant_options filter) causing Rust cache entries to be
shared across different tag sets and enabling tag accumulation; fix by including
tags in the cache-relevant options (remove "tags" from the exclusion) and ensure
the tags value is serialized deterministically to match the JS deduplication
suffix (e.g., use a stable JSON/string representation or sorted list) so that
generate_cache_key produces distinct keys for different tag sets and prevents
mutation/merging of tags into a shared cache entry.
---
Duplicate comments:
In `@packages/rari/src/image/Image.tsx`:
- Around line 292-304: The image component currently disables srcSet when a
fixed imgWidth is passed (because shouldUseSrcSet becomes false), which drops
DPR candidates and reduces sharpness on high-density screens; update the logic
around shouldUseSrcSet and the img rendering so that when imgWidth is provided
you still generate a density-based srcSet (using buildSrcSetString) for DPR
variants (e.g., 1x/2x/3x) and pass that into the imgElement srcSet prop while
keeping sizes appropriate; specifically adjust calculation of shouldUseSrcSet
(or add a new flag) and ensure buildSrcSetString is invoked for the fixed-width
path (referencing imgWidth, sizesArray, shouldUseSrcSet, buildSrcSetString,
mainSrc, and imgElement) so fixed-width images include DPR srcSet candidates.
- Around line 103-123: isVisible is only initialized once via
getInitialVisibility so when shouldLoadImmediately flips true later the state
isn't updated; add an effect that watches shouldLoadImmediately (and optionally
prevShouldLoadImmediatelyRef.current) and calls setIsVisible(true) when
shouldLoadImmediately transitions to true, updating
prevShouldLoadImmediatelyRef.current accordingly; reference the existing symbols
getInitialVisibility, shouldLoadImmediately, prevShouldLoadImmediatelyRef,
isVisible, and setIsVisible to locate where to add this synchronization.
In `@packages/rari/src/proxy/execute-proxy.ts`:
- Around line 24-27: The event.waitUntil implementation currently pushes the raw
promise (waitUntilPromises.push(promise)) which can reject before finally
handling and cause unhandled rejections; wrap the incoming promise before
storing so its rejection is observed immediately — e.g., create a wrapped =
promise.catch(err => { /* record/log the error or push to a waitUntilErrors
array */ throw err; }); and push that wrapped promise into waitUntilPromises
inside the waitUntil handler (referencing event.waitUntil and waitUntilPromises)
so rejections are captured when registered rather than only in the later
finally.
In `@packages/rari/src/proxy/shared/utils.ts`:
- Around line 103-107: The merge logic uses a truthiness check (if
(headers[lowerKey])) which treats empty-string or other falsy values as missing;
change that to an explicit existence check such as
Object.prototype.hasOwnProperty.call(headers, lowerKey) or (lowerKey in headers)
so keys with falsy values are correctly merged; update the block in the
result.headers.forEach loop referencing headers, lowerKey and existing to use
that explicit check before deciding whether to append to existing or create a
new array.
In `@packages/rari/src/router/navigation-utils.ts`:
- Around line 79-82: The optional-catch-all branch currently returns success
without validating that segment.param exists, allowing malformed manifests to
match; update the branch around handleOptionalCatchAll(actualIndex,
actualSegments.length, segment.param, params) to first check that segment.param
is present (non-empty) and, if missing, return { success: false } (same failure
behavior as dynamic/catch-all handling); otherwise call handleOptionalCatchAll
and return the success result as before so missing param cases fail closed.
In `@packages/rari/src/router/sitemap-generator.ts`:
- Around line 253-259: The resolveId implementation currently resolves relative
imports with path.resolve which treats the virtual module as the importer and
bypasses normal extension resolution; update resolveId (the function named
resolveId and references to VIRTUAL_SITEMAP_ID and sitemapFile.path) so that
when id.startsWith('.') you call this.resolve(id, correctedImporter) instead of
path.resolve: if importer === VIRTUAL_SITEMAP_ID use sitemapFile.path as the
importer, otherwise use the provided importer, then return the resolved result
(e.g., result?.id) or null so Vite/Rollup can perform normal extension
resolution and plugin chaining.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ee91ef8-f338-4bc3-96cc-b7e9b8fd34f2
📒 Files selected for processing (18)
crates/rari/src/rsc/rendering/core/js/component_eval_setup.jscrates/rari/src/runtime/ext/web/init_fetch.jscrates/rari/src/runtime/module_reload/manager.rscrates/rari/src/server/middleware/request_context.rspackages/deploy/src/utils.tspackages/rari/src/image/Image.tsxpackages/rari/src/proxy/execute-proxy.tspackages/rari/src/proxy/matcher.tspackages/rari/src/proxy/shared/utils.tspackages/rari/src/router/navigation-utils.tspackages/rari/src/router/props-extractor.tspackages/rari/src/router/robots-generator.tspackages/rari/src/router/sitemap-generator.tspackages/rari/src/runtime/shared/get-client-component.tspackages/rari/src/shared/http-utils.tspackages/rari/src/vite/image-scanner.tstest/unit/router/navigation-utils.test.tstest/unit/router/props-extractor.test.ts
- Add null checks for React globals in LoadingSpinner and DefaultLoading components - Remove "tags" from cache key generation filter in request context - Export ProviderConfig type from deploy utils - Improve null coalescing checks in proxy response body handling - Add validation for optional-catch-all segment param before processing - Expand sitemap file extensions to include .mjs, .cjs, and .json - Fix sitemap module invocation to pass id directly instead of wrapped object - Refactor client component path resolution to check both relative and absolute variants - Replace logical OR operators with nullish coalescing operators in image scanner - Update test description to reflect actual behavior for missing param names
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/rari/src/image/Image.tsx (3)
146-153:⚠️ Potential issue | 🟠 MajorFill mode currently overrides consumer
objectFit.On Line 152, hardcoding
objectFit: 'cover'ignores a caller-providedstyle.objectFit, which makes fill images harder to customize.💡 Proposed fix
...(fill && { position: 'absolute', inset: 0, width: '100%', height: '100%', - objectFit: 'cover', + objectFit: style?.objectFit ?? 'cover', }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/image/Image.tsx` around lines 146 - 153, The fill branch in Image.tsx is forcing objectFit: 'cover' which overrides any consumer-provided style.objectFit; update the spread so the consumer style wins by merging style after the fill defaults (or only set objectFit if not provided) within the component that builds the element's style (the code using the fill && { position, inset, width, height, objectFit } object) so that style.objectFit passed in props.style is preserved; adjust the merge order or conditional to check for style?.objectFit before assigning objectFit in the fill block.
293-315: 🧹 Nitpick | 🔵 TrivialRemove constant-true branch to reduce noise.
shouldUseSrcSetis alwaystrue, so theif (!shouldUseSrcSet)branch is unreachable and adds unnecessary complexity.💡 Proposed cleanup
- const shouldUseSrcSet = true - const imgElement = ( <img ref={imgRef} src={isVisible ? mainSrc : undefined} - srcSet={isVisible && shouldUseSrcSet ? buildSrcSetString(sizesArray, finalSrc, quality, undefined, loader, hasFixedWidth) : undefined} + srcSet={isVisible ? buildSrcSetString(sizesArray, finalSrc, quality, undefined, loader, hasFixedWidth) : undefined} sizes={hasFixedWidth ? undefined : sizes} alt={alt} @@ - if (!shouldUseSrcSet) - return imgElement - return (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/image/Image.tsx` around lines 293 - 315, The file defines a redundant constant shouldUseSrcSet = true causing the unreachable if (!shouldUseSrcSet) return imgElement branch; remove the constant and the dead conditional and simplify the img JSX to always compute srcSet via buildSrcSetString(sizesArray, finalSrc, quality, undefined, loader, hasFixedWidth) (or guard with isVisible only as currently done), ensuring references to imgElement, shouldUseSrcSet, and buildSrcSetString are updated/removed so no dead code remains.
104-136:⚠️ Potential issue | 🟠 MajorVisibility can regress when immediate-load props flip off.
On Line 104,
hasIntersectedalways startsfalse. IfshouldLoadImmediatelyis initiallytrueand later becomesfalse,isVisiblecan drop tofalseand clearsrcuntil intersection occurs again.💡 Proposed fix
function useImageLazyLoad( imgRef: React.RefObject<HTMLImageElement | null>, shouldPreload: boolean, unoptimized: boolean, loading: 'lazy' | 'eager', ) { const shouldLoadImmediately = shouldPreload || unoptimized || loading === 'eager' - const [hasIntersected, setHasIntersected] = useState(false) + const [hasIntersected, setHasIntersected] = useState(shouldLoadImmediately) + + useEffect(() => { + if (shouldLoadImmediately) + setHasIntersected(true) + }, [shouldLoadImmediately]) useEffect(() => { if (shouldLoadImmediately) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/image/Image.tsx` around lines 104 - 136, The visibility flag resets incorrectly when shouldLoadImmediately goes from true to false; initialize and preserve intersection state instead of always starting false. Change the useState for hasIntersected to derive its initial value from shouldLoadImmediately (or setHasIntersected(true) when shouldLoadImmediately is true) and ensure nothing in useEffect or elsewhere ever sets hasIntersected back to false so the returned expression (shouldLoadImmediately || hasIntersected) won't drop visibility when shouldLoadImmediately flips off; key symbols: hasIntersected, setHasIntersected, shouldLoadImmediately, imgRef, useEffect, observer.packages/deploy/src/utils.ts (1)
123-133:⚠️ Potential issue | 🟠 Major
ensureMinimumNodeEnginedoes not enforce the declared>=22.12.0floor.Line 123 declares
MIN_NODE_VERSIONas>=22.12.0, but Lines 128-129 reduce validation to major-only (22). This lets ranges like>=22.0.0pass unchanged even though they are below the stated minimum.Please align validation with the full minimum version, or change the declared minimum to major-only if that was the actual intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/deploy/src/utils.ts` around lines 123 - 133, The current ensureMinimumNodeEngine uses EXTRACT_MAJOR_REGEX and isNodeVersionSufficient to check only the major version against MIN_NODE_VERSION (>=22.12.0), letting ranges like >=22.0.0 slip through; update ensureMinimumNodeEngine to compare full semver ranges instead of extracting the major: use the semver library (e.g., semver.minVersion or semver.satisfies/semver.intersects) to derive the concrete minimum from MIN_NODE_VERSION and verify packageJson.engines.node satisfies that full minimum (reference symbols: MIN_NODE_VERSION, ensureMinimumNodeEngine, isNodeVersionSufficient, EXTRACT_MAJOR_REGEX, MIN_SUPPORTED_NODE_MAJOR); alternatively, if the intent was to enforce a major-only floor, change MIN_NODE_VERSION to a major-only value (e.g., ">=22.0.0") and keep the existing major-only logic.crates/rari/src/runtime/module_reload/manager.rs (1)
125-139:⚠️ Potential issue | 🟠 MajorMove task cancellation before task spawn to prevent race condition with debounce state.
The task spawned at line 125 can execute before the debounce state is registered via
add_pending(line 139). Ifdebounce_delay_msis 0 or very low, the spawned task'sremove_pendingcall executes beforeadd_pending, leaving a stale orphan entry in the pending map that corrupts subsequent operations on that component_id.Additionally,
ReloadConfig.validate()exists but is never called in the codebase, meaning the debounce delay has no minimum validation and can be set to 0.Reorder operations to register the pending state before the task can run:
Suggested fix
- let manager = self.clone_for_task(); - let component_id_clone = component_id.clone(); - let file_path_clone = file_path.clone(); - - let handle = tokio::spawn(async move { + self.debounce_manager.cancel_pending(&component_id).await; + + let manager = self.clone_for_task(); + let component_id_clone = component_id.clone(); + let file_path_clone = file_path.clone(); + + let handle = tokio::spawn(async move { tokio::time::sleep(debounce_delay).await; manager.debounce_manager.remove_pending(&component_id_clone).await; if let Err(e) = manager.reload_module(&component_id_clone, &file_path_clone).await { error!("Failed to reload module {}: {}", component_id_clone, e); } }); let request = ModuleReloadRequest::new(component_id.clone(), file_path.clone()); - - self.debounce_manager.cancel_pending(&component_id).await; - self.debounce_manager.add_pending(component_id.clone(), request, handle).await;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/runtime/module_reload/manager.rs` around lines 125 - 139, Move the debounce cancellation before you spawn the background task and ensure the debounce config is validated: call self.debounce_manager.cancel_pending(&component_id).await before creating the tokio::spawn handle so you remove any prior pending state before the new task can run, then create ModuleReloadRequest::new(...) and call debounce_manager.add_pending(..., handle).await after that; additionally, invoke ReloadConfig::validate() where the debounce_delay is derived so debounce_delay cannot be 0 (use the validated value when constructing the spawned task and calling remove_pending in manager.debounce_manager.remove_pending and manager.reload_module).
♻️ Duplicate comments (6)
packages/rari/src/router/props-extractor.ts (1)
182-186:⚠️ Potential issue | 🟠 MajorTemplate is still collapsed to a string, which breaks deeper title inheritance.
At Line 185, a parent templated title is converted into a plain string, so downstream merges can no longer apply the inherited template. This is the same concern previously raised for this area and appears reintroduced.
💡 Proposed fix
if (typeof childTitle === 'string') { if (typeof parentTitle === 'object' && parentTitle?.template) { const hasPlaceholder = parentTitle.template.includes('%s') - return hasPlaceholder ? parentTitle.template.replace('%s', childTitle) : childTitle + return { + ...parentTitle, + default: hasPlaceholder + ? parentTitle.template.replace('%s', childTitle) + : childTitle, + } } return childTitle }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/props-extractor.ts` around lines 182 - 186, The current branch returns a plain string when childTitle is a string and parentTitle has a template, which collapses parentTitle.template and prevents deeper inheritance; instead, keep parentTitle as an object and return a merged object that preserves parentTitle.template and stores the child string in a separate field (e.g., { template: parentTitle.template, value: childTitle }) so downstream merges can still apply the template; update the code path referencing childTitle/parentTitle in props-extractor.ts to return this merged object rather than collapsing to a string.packages/rari/src/proxy/shared/utils.ts (1)
137-142:⚠️ Potential issue | 🟡 MinorPreserve empty-string response bodies.
The truthy guard
result.body &&at line 137 drops empty-string bodies (result.body === ''). An empty body is semantically valid (e.g., 204 No Content responses that still havebodyset to'').Proposed fix
- else if (result.body && typeof result.body === 'string') { + else if (typeof result.body === 'string') { body = result.body } - else if (result.body) { + else if (result.body != null) { console.warn('[rari] Proxy: Response body is not extractable as text') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/proxy/shared/utils.ts` around lines 137 - 142, The current truthy guard drops empty-string bodies; update the checks around result.body so empty strings are preserved: use typeof result.body === 'string' to set body when result.body is a string (so '' is kept), and change the subsequent existence check to a null/undefined check (e.g., result.body != null) before logging the warning, keeping the variable name body and the console.warn('[rari] Proxy: Response body is not extractable as text') behavior intact.packages/deploy/src/utils.ts (1)
170-177:⚠️ Potential issue | 🟡 MinorExported function uses a non-exported type in its public signature.
Line 177 exports
updatePackageJsonForProvider, whileProviderConfigat Line 170 is not exported. This can cause API/declaration friction and makes the public contract less explicit.#!/bin/bash set -euo pipefail echo "=== Symbol visibility in utils.ts ===" rg -nP '^(export\s+)?interface\s+ProviderConfig|^export function updatePackageJsonForProvider' packages/deploy/src/utils.ts echo echo "=== Declaration-related compiler options under packages/deploy ===" python - <<'PY' import json from pathlib import Path for path in Path("packages/deploy").rglob("tsconfig*.json"): try: data = json.loads(path.read_text()) except Exception as e: print(f"--- {path} ---") print(f"Failed to parse JSON: {e}") continue co = data.get("compilerOptions", {}) print(f"--- {path} ---") print({ "declaration": co.get("declaration"), "emitDeclarationOnly": co.get("emitDeclarationOnly"), "composite": co.get("composite") }) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/deploy/src/utils.ts` around lines 170 - 177, The exported function updatePackageJsonForProvider currently references a non-exported type ProviderConfig in its public signature; export the interface ProviderConfig (add export to the ProviderConfig declaration) so the function's public API surface is explicit and type declarations can be generated and consumed correctly, ensuring symbols ProviderConfig and updatePackageJsonForProvider remain consistent in the module's public types.packages/rari/src/router/sitemap-generator.ts (3)
228-246:⚠️ Potential issue | 🟡 MinorKeep extension discovery aligned with the advertised module types.
This helper now treats
.mjs,.cjs, and.jsonas supported, butfindSitemapFiles()still defaults to['.ts', '.tsx', '.js', '.jsx'], and the current caller inpackages/rari/src/vite/server-build.ts:1851-1865passes that same list. If these formats are meant to work end-to-end, they need to be added to discovery as well; otherwise this branch is unreachable unless every caller opts in manually.♻️ Minimal fix
export async function findSitemapFiles( appDir: string, - extensions: string[] = ['.ts', '.tsx', '.js', '.jsx'], + extensions: string[] = ['.ts', '.tsx', '.js', '.jsx', '.mjs', '.cjs', '.json'], ): Promise<SitemapFile[]> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/sitemap-generator.ts` around lines 228 - 246, The determineModuleType function lists .mjs, .cjs, and .json as supported module types but discovery still uses a narrower set; update sitemap discovery so those extensions are actually found by adding '.mjs', '.cjs', and '.json' to the default extensions array used by findSitemapFiles (and ensure the caller in server-build.ts uses that updated list rather than the hardcoded ['.ts','.tsx','.js','.jsx']), or centralize the allowedExtensions constant and reference it from determineModuleType, findSitemapFiles, and the caller to keep discovery and type handling in sync.
323-325:⚠️ Potential issue | 🟠 MajorPass the original shard ID into the module function.
Stringifying here changes semantics for any non-string ID returned by
generateSitemaps(). The filename still needs a sanitized string, but the shard function should receive the originalidvalue.🔧 Proposed fix
const sitemapData = typeof module.default === 'function' - ? await module.default({ id: String(id) }) + ? await module.default({ id }) : module.default🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/sitemap-generator.ts` around lines 323 - 325, The code currently stringifies the shard id when calling the module function (const sitemapData = typeof module.default === 'function' ? await module.default({ id: String(id) }) : module.default), which alters semantics for non-string ids; change the call to pass the original id value (await module.default({ id })) and only stringify/sanitize when constructing filenames for the sitemap file output so the shard function receives the original id type while filenames still use a sanitized string.
253-259:⚠️ Potential issue | 🟠 MajorDelegate relative imports back to Rolldown's resolver.
resolveId()receives the fully resolved importer, returningnulldefers to the rest of the resolver chain, andthis.resolve(...)exists specifically to re-use that chain. This branch short-circuits all relative imports throughpath.resolve(...), so imports from the virtual entry skip theVIRTUAL_SITEMAP_IDbase case and, in practice, can miss normal extension resolution for common./helperimports. (rolldown.rs)🔧 Proposed fix
- resolveId(id: string, importer?: string) { + async resolveId(id: string, importer?: string) { if (id === VIRTUAL_SITEMAP_ID) return id - if (id.startsWith('.')) - return path.resolve(path.dirname(importer ?? sitemapFile.path), id) - - return null + if (!id.startsWith('.')) + return null + + const baseImporter = importer === VIRTUAL_SITEMAP_ID || !importer + ? sitemapFile.path + : importer + + return this.resolve(id, baseImporter, { skipSelf: true }) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/sitemap-generator.ts` around lines 253 - 259, The resolveId branch that handles relative imports currently short-circuits resolution by returning path.resolve(...), which bypasses Rollup/Rolldown's resolver chain; change the id.startsWith('.') branch in resolveId to delegate to the resolver chain by returning this.resolve(id, importer ?? sitemapFile.path) instead of calling path.resolve, so relative imports from the virtual sitemap entry (VIRTUAL_SITEMAP_ID) go through normal extension and plugin resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rari/src/rsc/rendering/core/js/component_eval_setup.js`:
- Line 10: The element factory currently sets the property named $typeof but
downstream checks expect $$typeof; update the created element object so the key
is $$typeof (with value Symbol.for('react.transitional.element')) instead of
$typeof so component identity and error handling in component_render.js and
streaming_react_setup.js will match; ensure any place constructing transitional
elements uses the $$typeof symbol key consistently.
- Around line 25-28: The current fallbacks for _jsx and _jsxs use a variadic
arrow that forwards all args into globalThis.React.createElement, which treats a
third arg (key) as a child and corrupts element identity; replace the fallback
with a small wrapper that accepts (type, props, key) (or checks args.length) and
calls globalThis.React.createElement(type, props, key) so the key is passed as
the third parameter; apply this change to both _jsx and _jsxs references in the
file.
In `@crates/rari/src/runtime/module_reload/manager.rs`:
- Around line 246-249: The task join failures are being mislabeled as
ModuleReloadError::RuntimeNotAvailable; add a new enum variant (e.g.,
ModuleReloadError::TaskJoinFailure or BatchTaskFailure) to the ModuleReloadError
type and use it when wrapping tokio join errors instead of RuntimeNotAvailable;
update the error construction call (currently using
RariError::module_reload(ModuleReloadError::RuntimeNotAvailable { message: ...
})) to RariError::module_reload(ModuleReloadError::TaskJoinFailure { message:
format!("Task join error: {}", e) }) and adjust any Display/From/serialization
implementations and tests that generate or match module-reload errors so the new
variant is handled consistently.
In `@packages/rari/src/proxy/execute-proxy.ts`:
- Around line 33-35: The code reads the private field proxyFn via a cast
((executor as any).proxyFn), which is fragile; add a public accessor on
ProxyExecutor (e.g., getProxyFunction(): ProxyFunction | null) in executor.ts to
return the underlying proxy function, then update execute-proxy.ts to call
executor.getProxyFunction() and validate its type instead of using the any cast;
reference ProxyExecutor, getProxyFunction, and the local proxyFn usage in
execute-proxy.ts when making the change.
- Around line 24-31: The current waitUntil implementation swallows rejections by
using .catch(err => return err) so handledPromise always resolves and
Promise.allSettled cannot detect rejections; change the handler in
event.waitUntil (around the event/waitUntil/waitUntilPromises code) to attach a
catch that rethrows the error (or use .then(undefined, err => { throw err })) so
the promise remains rejected while still having a handler to avoid
unhandledRejection warnings, allowing the later Promise.allSettled/status ===
'rejected' logic to work correctly.
---
Outside diff comments:
In `@crates/rari/src/runtime/module_reload/manager.rs`:
- Around line 125-139: Move the debounce cancellation before you spawn the
background task and ensure the debounce config is validated: call
self.debounce_manager.cancel_pending(&component_id).await before creating the
tokio::spawn handle so you remove any prior pending state before the new task
can run, then create ModuleReloadRequest::new(...) and call
debounce_manager.add_pending(..., handle).await after that; additionally, invoke
ReloadConfig::validate() where the debounce_delay is derived so debounce_delay
cannot be 0 (use the validated value when constructing the spawned task and
calling remove_pending in manager.debounce_manager.remove_pending and
manager.reload_module).
In `@packages/deploy/src/utils.ts`:
- Around line 123-133: The current ensureMinimumNodeEngine uses
EXTRACT_MAJOR_REGEX and isNodeVersionSufficient to check only the major version
against MIN_NODE_VERSION (>=22.12.0), letting ranges like >=22.0.0 slip through;
update ensureMinimumNodeEngine to compare full semver ranges instead of
extracting the major: use the semver library (e.g., semver.minVersion or
semver.satisfies/semver.intersects) to derive the concrete minimum from
MIN_NODE_VERSION and verify packageJson.engines.node satisfies that full minimum
(reference symbols: MIN_NODE_VERSION, ensureMinimumNodeEngine,
isNodeVersionSufficient, EXTRACT_MAJOR_REGEX, MIN_SUPPORTED_NODE_MAJOR);
alternatively, if the intent was to enforce a major-only floor, change
MIN_NODE_VERSION to a major-only value (e.g., ">=22.0.0") and keep the existing
major-only logic.
In `@packages/rari/src/image/Image.tsx`:
- Around line 146-153: The fill branch in Image.tsx is forcing objectFit:
'cover' which overrides any consumer-provided style.objectFit; update the spread
so the consumer style wins by merging style after the fill defaults (or only set
objectFit if not provided) within the component that builds the element's style
(the code using the fill && { position, inset, width, height, objectFit }
object) so that style.objectFit passed in props.style is preserved; adjust the
merge order or conditional to check for style?.objectFit before assigning
objectFit in the fill block.
- Around line 293-315: The file defines a redundant constant shouldUseSrcSet =
true causing the unreachable if (!shouldUseSrcSet) return imgElement branch;
remove the constant and the dead conditional and simplify the img JSX to always
compute srcSet via buildSrcSetString(sizesArray, finalSrc, quality, undefined,
loader, hasFixedWidth) (or guard with isVisible only as currently done),
ensuring references to imgElement, shouldUseSrcSet, and buildSrcSetString are
updated/removed so no dead code remains.
- Around line 104-136: The visibility flag resets incorrectly when
shouldLoadImmediately goes from true to false; initialize and preserve
intersection state instead of always starting false. Change the useState for
hasIntersected to derive its initial value from shouldLoadImmediately (or
setHasIntersected(true) when shouldLoadImmediately is true) and ensure nothing
in useEffect or elsewhere ever sets hasIntersected back to false so the returned
expression (shouldLoadImmediately || hasIntersected) won't drop visibility when
shouldLoadImmediately flips off; key symbols: hasIntersected, setHasIntersected,
shouldLoadImmediately, imgRef, useEffect, observer.
---
Duplicate comments:
In `@packages/deploy/src/utils.ts`:
- Around line 170-177: The exported function updatePackageJsonForProvider
currently references a non-exported type ProviderConfig in its public signature;
export the interface ProviderConfig (add export to the ProviderConfig
declaration) so the function's public API surface is explicit and type
declarations can be generated and consumed correctly, ensuring symbols
ProviderConfig and updatePackageJsonForProvider remain consistent in the
module's public types.
In `@packages/rari/src/proxy/shared/utils.ts`:
- Around line 137-142: The current truthy guard drops empty-string bodies;
update the checks around result.body so empty strings are preserved: use typeof
result.body === 'string' to set body when result.body is a string (so '' is
kept), and change the subsequent existence check to a null/undefined check
(e.g., result.body != null) before logging the warning, keeping the variable
name body and the console.warn('[rari] Proxy: Response body is not extractable
as text') behavior intact.
In `@packages/rari/src/router/props-extractor.ts`:
- Around line 182-186: The current branch returns a plain string when childTitle
is a string and parentTitle has a template, which collapses parentTitle.template
and prevents deeper inheritance; instead, keep parentTitle as an object and
return a merged object that preserves parentTitle.template and stores the child
string in a separate field (e.g., { template: parentTitle.template, value:
childTitle }) so downstream merges can still apply the template; update the code
path referencing childTitle/parentTitle in props-extractor.ts to return this
merged object rather than collapsing to a string.
In `@packages/rari/src/router/sitemap-generator.ts`:
- Around line 228-246: The determineModuleType function lists .mjs, .cjs, and
.json as supported module types but discovery still uses a narrower set; update
sitemap discovery so those extensions are actually found by adding '.mjs',
'.cjs', and '.json' to the default extensions array used by findSitemapFiles
(and ensure the caller in server-build.ts uses that updated list rather than the
hardcoded ['.ts','.tsx','.js','.jsx']), or centralize the allowedExtensions
constant and reference it from determineModuleType, findSitemapFiles, and the
caller to keep discovery and type handling in sync.
- Around line 323-325: The code currently stringifies the shard id when calling
the module function (const sitemapData = typeof module.default === 'function' ?
await module.default({ id: String(id) }) : module.default), which alters
semantics for non-string ids; change the call to pass the original id value
(await module.default({ id })) and only stringify/sanitize when constructing
filenames for the sitemap file output so the shard function receives the
original id type while filenames still use a sanitized string.
- Around line 253-259: The resolveId branch that handles relative imports
currently short-circuits resolution by returning path.resolve(...), which
bypasses Rollup/Rolldown's resolver chain; change the id.startsWith('.') branch
in resolveId to delegate to the resolver chain by returning this.resolve(id,
importer ?? sitemapFile.path) instead of calling path.resolve, so relative
imports from the virtual sitemap entry (VIRTUAL_SITEMAP_ID) go through normal
extension and plugin resolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1bba13d9-616f-4801-b885-12a7035e302b
📒 Files selected for processing (10)
crates/rari/src/rsc/rendering/core/js/component_eval_setup.jscrates/rari/src/runtime/ext/web/init_fetch.jscrates/rari/src/runtime/module_reload/manager.rspackages/deploy/src/utils.tspackages/rari/src/image/Image.tsxpackages/rari/src/proxy/execute-proxy.tspackages/rari/src/proxy/executor.tspackages/rari/src/proxy/shared/utils.tspackages/rari/src/router/props-extractor.tspackages/rari/src/router/sitemap-generator.ts
- Fix React element symbol from $typeof to $$typeof for proper element detection - Replace spread operator with explicit parameters in _jsx and _jsxs fallbacks - Optimize image lazy loading with immediate state initialization based on shouldLoadImmediately - Add ref tracking to handle shouldLoadImmediately state changes without extra renders - Reorder style application in buildImageStyle to allow custom styles to override fill styles - Remove unnecessary shouldUseSrcSet variable and conditional logic in OptimizedImage - Improve error handling in executeProxy by throwing caught errors instead of returning them - Add getProxyFunction() method to ProxyExecutor for cleaner proxy function access - Simplify history state management in ClientRouter by removing redundant path comparison
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
crates/rari/src/rsc/rendering/core/js/component_eval_setup.js (2)
10-10:⚠️ Potential issue | 🔴 CriticalUse
$$typeoffor element identity (Line 10).Downstream render paths match
$$typeof, not$typeof; this mismatch can break element recognition.Suggested patch
const element = { - $typeof: Symbol.for('react.transitional.element'), + $$typeof: Symbol.for('react.transitional.element'), type, props: propsWithoutKey, key, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/rsc/rendering/core/js/component_eval_setup.js` at line 10, The object currently uses the property "$typeof" which downstream renderers expect as "$$typeof"; update the key from "$typeof" to "$$typeof" so it sets Symbol.for('react.transitional.element') on the expected identity property (i.e., replace the "$typeof" property assignment with "$$typeof: Symbol.for('react.transitional.element')").
25-28:⚠️ Potential issue | 🟠 MajorFix
_jsx/_jsxsfallback signature mismatch withReact.createElement(Lines 25-28).The current variadic fallback can pass JSX runtime
keyas a child argument. Wrap fallbacks as(type, props, key)and mergekeyinto props only when defined.Suggested patch
if (typeof _jsx === 'undefined') - var _jsx = globalThis['~react']?.jsxRuntime?.jsx || globalThis.jsx || ((...args) => globalThis.React.createElement(...args)) + var _jsx = globalThis['~react']?.jsxRuntime?.jsx + || globalThis.jsx + || ((type, props, key) => globalThis.React.createElement(type, key === undefined ? props : { ...(props || {}), key })) if (typeof _jsxs === 'undefined') - var _jsxs = globalThis['~react']?.jsxRuntime?.jsxs || globalThis.jsxs || ((...args) => globalThis.React.createElement(...args)) + var _jsxs = globalThis['~react']?.jsxRuntime?.jsxs + || globalThis.jsxs + || ((type, props, key) => globalThis.React.createElement(type, key === undefined ? props : { ...(props || {}), key }))#!/bin/bash set -euo pipefail echo "1) Inspect current fallback implementations:" rg -nP "var _jsx|var _jsxs|createElement\\(\\.\\.\\.args\\)" crates/rari/src/rsc/rendering/core/js/component_eval_setup.js -C2 echo echo "2) Find transpiled callsites passing a 3rd argument (potential key):" rg -nP '\b_jsxs?\s*\([^,]+,[^,]+,[^)]+\)' crates/rari/src/rsc/rendering/core/js -g '!**/node_modules/**' echo echo "Expected:" echo "- If step (2) returns matches, current variadic fallback risks treating key as child."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/rsc/rendering/core/js/component_eval_setup.js` around lines 25 - 28, The fallback implementations for _jsx and _jsxs currently use a variadic signature that may pass the JSX-runtime key as a child to React.createElement; change the fallbacks for _jsx and _jsxs to accept (type, props, key) and, when key is defined, copy or set props.key = key (or create a shallow props copy with key) before calling globalThis.React.createElement(type, props) so the third argument is merged into props only when present; update the fallback expressions that reference globalThis['~react']?.jsxRuntime?.jsx or globalThis.jsx to follow this (type, props, key) -> merge key -> call React.createElement to match the JSX runtime signature for both _jsx and _jsxs.packages/rari/src/router/sitemap-generator.ts (1)
273-284: 🧹 Nitpick | 🔵 TrivialConsider using rolldown's exported types for better type safety.
The
resultparameter and array callbacks still useany. As noted in a previous review, rolldown exports theRolldownBuildtype which could improve type safety here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/sitemap-generator.ts` around lines 273 - 284, Replace the use of any in extractChunkCode by importing and using rolldown's exported build/output types: change the parameter type of result to RolldownBuild (or the exact exported build type) and type the output items as the Rolldown output chunk type (e.g., RolldownOutputChunk or the exact exported output item type) so the find callbacks use properly typed items instead of any; keep the same runtime checks for entryChunk and return type string from extractChunkCode but update signatures and local variables (result, entryChunk, and the callback parameter) to use the rolldown types for full type safety.crates/rari/src/server/middleware/request_context.rs (1)
153-167:⚠️ Potential issue | 🟠 MajorPersist merged tags back into the in-flight shared result.
Line 153 returns a merged follower result, but only updates
fetch_cache(Line 164).guardis left unchanged, so subsequent followers still merge from stale leader tags and can lose previously merged follower tags.🔧 Proposed fix
if let Some(result) = guard.as_ref() { let mut cloned_result = result.clone()?; let mut tag_set: FxHashSet<String> = cloned_result.tags.iter().cloned().collect(); tag_set.extend(tags.iter().cloned()); let mut merged_tags: Vec<String> = tag_set.into_iter().collect(); merged_tags.sort(); cloned_result.tags = merged_tags; { let mut cache = self.fetch_cache.lock(); cache.put(cache_key.clone(), cloned_result.clone()); } + *guard = Some(Ok(cloned_result.clone())); return Ok(cloned_result); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/server/middleware/request_context.rs` around lines 153 - 167, The merged tags are only written to fetch_cache (cloned_result) but not persisted back into the in-flight shared `guard`, so subsequent followers will read stale tags; update the in-flight shared result referenced by `guard` (the same object behind `guard.as_ref()` / `result`) to store `merged_tags` (or replace its tags with `cloned_result.tags`) before releasing the lock and returning, ensuring both `self.fetch_cache` (cache_key) and the in-flight `guard` reflect the merged tag set.packages/rari/src/runtime/shared/get-client-component.ts (1)
155-155: 🧹 Nitpick | 🔵 TrivialConsider using
getPathVariantsto reduce duplication.Line 155 duplicates the exact logic defined in
getPathVariants(lines 209-211). Consider calling the helper function for better maintainability.♻️ Suggested refactor
- const variants = path.startsWith('./') ? [path, path.slice(2)] : [path, `./${path}`] + const variants = getPathVariants(path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/runtime/shared/get-client-component.ts` at line 155, The line creating `variants` duplicates logic found in `getPathVariants`; replace the inline ternary with a call to that helper by setting `variants = getPathVariants(path)` in `get-client-component.ts` (where `variants` and `path` are used), ensuring the helper `getPathVariants` is in scope so you remove the duplicated logic and centralize path-variant generation.packages/rari/src/vite/image-scanner.ts (1)
250-322:⚠️ Potential issue | 🟠 MajorTemplate expression brace tracking still conflates counters.
The fix for nested objects introduced
templateDepthbut the issue persists:braceCountis used for both${...}template expression delimiters (lines 294, 305) and object literal braces (lines 311, 314). Objects inside template expressions cause extraction to fail.Example that breaks:
{ src: \/images/${getPath({type: 'foo'})}.jpg` }`
- The inner object's
}decrementsbraceCountvia line 305, causing the outer object's closing}to drivebraceCountnegative.🐛 Proposed fix: separate template expression depth from object brace count
function extractBalancedBraces(code: string, startIndex: number): string | null { let braceCount = 0 let inString = false let stringChar = '' let escaped = false let templateDepth = 0 + let templateExprDepth = 0 for (let i = startIndex; i < code.length; i++) { // ... escape and string start handling unchanged ... - if (inString && stringChar === '`' && char === '$' && i + 1 < code.length && code[i + 1] === '{') { - braceCount++ + if (inString && stringChar === '`' && templateExprDepth === 0 && char === '$' && i + 1 < code.length && code[i + 1] === '{') { + templateExprDepth++ i++ continue } - if (inString && stringChar === '`' && braceCount > 0 && char === '`') { - templateDepth++ - continue + if (inString && stringChar === '`' && templateExprDepth > 0) { + if (char === '{') + templateExprDepth++ + else if (char === '}') { + templateExprDepth-- + continue + } } - - if (inString && stringChar === '`' && char === '}' && braceCount > 0) { - braceCount-- - continue - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/vite/image-scanner.ts` around lines 250 - 322, The extractBalancedBraces function conflates template-expression braces with object-literal braces using a single braceCount; split these concerns by introducing two counters (e.g., objBraceCount and templateExprCount) and only decrement/increment the appropriate counter depending on context: use templateExprCount for tracking ${...} expressions inside backtick strings (increment when seeing ${ and decrement on matching } inside that expression) and use objBraceCount for top-level { } object literals (increment/decrement only when not inside a template expression or string). Update the logic around inString/stringChar/templateDepth to (1) increment templateExprCount when you encounter '${' while in a backtick string, (2) route '}' handling to decrement templateExprCount when templateExprCount>0, otherwise decrement objBraceCount, and (3) return the substring when objBraceCount reaches zero; ensure escaped chars and nested backtick/template backtracking still respect these separate counters (symbols: extractBalancedBraces, braceCount -> replace uses with objBraceCount/templateExprCount, inString, stringChar, templateDepth).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rari/src/server/middleware/request_context.rs`:
- Around line 132-136: The repeated dedup+sort logic that builds tag_set,
extends with tags, collects to merged_tags, sorts and assigns to result.tags
should be extracted into a helper (e.g., fn merge_and_sort_tags(existing: impl
IntoIterator<Item=String>, extra: impl IntoIterator<Item=String>) ->
Vec<String>) which returns a deduplicated, sorted Vec<String>; replace the three
inline blocks (the ones manipulating tag_set, merged_tags and assigning
result.tags) with calls to this helper, and update call sites to pass
result.tags and tags (or other tag sources) into merge_and_sort_tags and assign
the return back to result.tags to centralize behavior and reduce duplication.
In `@packages/rari/src/proxy/shared/utils.ts`:
- Around line 78-95: Duplicate header append/arrayify logic in
packages/rari/src/proxy/shared/utils.ts (the blocks that mutate requestHeaders
and responseHeaders) should be extracted into a single helper (e.g., mergeHeader
or appendOrArrayifyHeader) that accepts the target headers object, the
lower-cased key and the value and performs: if Object.hasOwn(headers, key) then
set headers[key] = Array.isArray(existing) ? [...existing, value] : [existing,
value] else headers[key] = value; replace the two duplicated blocks (the
requestHeaders and responseHeaders branches and the similar code at lines
~122-127) with calls to this helper so behavior is consistent and duplication
removed.
- Around line 73-87: The proxy header detection uses the raw header key casing
so headers like X-Rari-Proxy-Request-... can be missed; in extractProxyHeaders
normalize the incoming header key to lower-case first (e.g., const lowerKey =
key.toLowerCase()) and then perform the prefix checks against lowerKey (use
lowerKey.startsWith('x-rari-proxy-request-') and
lowerKey.startsWith('x-rari-proxy-')), compute headerName from lowerKey (strip
the 'x-rari-proxy-request-' prefix) and continue updating
requestHeaders/response path using the normalized key to ensure consistent
behavior regardless of original header casing.
In `@packages/rari/src/router/navigation-utils.ts`:
- Around line 20-22: matchStaticSegment currently compares the raw path token to
segment.value which fails for encoded characters (e.g., "/caf%C3%A9"); update
matchStaticSegment to decode the incoming token (and decode segment.value for
safety) using decodeURIComponent before doing the equality check so static
routes with percent-encoded characters match correctly — locate the
matchStaticSegment function and replace the direct === comparison with a
comparison of decoded values.
---
Duplicate comments:
In `@crates/rari/src/rsc/rendering/core/js/component_eval_setup.js`:
- Line 10: The object currently uses the property "$typeof" which downstream
renderers expect as "$$typeof"; update the key from "$typeof" to "$$typeof" so
it sets Symbol.for('react.transitional.element') on the expected identity
property (i.e., replace the "$typeof" property assignment with "$$typeof:
Symbol.for('react.transitional.element')").
- Around line 25-28: The fallback implementations for _jsx and _jsxs currently
use a variadic signature that may pass the JSX-runtime key as a child to
React.createElement; change the fallbacks for _jsx and _jsxs to accept (type,
props, key) and, when key is defined, copy or set props.key = key (or create a
shallow props copy with key) before calling globalThis.React.createElement(type,
props) so the third argument is merged into props only when present; update the
fallback expressions that reference globalThis['~react']?.jsxRuntime?.jsx or
globalThis.jsx to follow this (type, props, key) -> merge key -> call
React.createElement to match the JSX runtime signature for both _jsx and _jsxs.
In `@crates/rari/src/server/middleware/request_context.rs`:
- Around line 153-167: The merged tags are only written to fetch_cache
(cloned_result) but not persisted back into the in-flight shared `guard`, so
subsequent followers will read stale tags; update the in-flight shared result
referenced by `guard` (the same object behind `guard.as_ref()` / `result`) to
store `merged_tags` (or replace its tags with `cloned_result.tags`) before
releasing the lock and returning, ensuring both `self.fetch_cache` (cache_key)
and the in-flight `guard` reflect the merged tag set.
In `@packages/rari/src/router/sitemap-generator.ts`:
- Around line 273-284: Replace the use of any in extractChunkCode by importing
and using rolldown's exported build/output types: change the parameter type of
result to RolldownBuild (or the exact exported build type) and type the output
items as the Rolldown output chunk type (e.g., RolldownOutputChunk or the exact
exported output item type) so the find callbacks use properly typed items
instead of any; keep the same runtime checks for entryChunk and return type
string from extractChunkCode but update signatures and local variables (result,
entryChunk, and the callback parameter) to use the rolldown types for full type
safety.
In `@packages/rari/src/runtime/shared/get-client-component.ts`:
- Line 155: The line creating `variants` duplicates logic found in
`getPathVariants`; replace the inline ternary with a call to that helper by
setting `variants = getPathVariants(path)` in `get-client-component.ts` (where
`variants` and `path` are used), ensuring the helper `getPathVariants` is in
scope so you remove the duplicated logic and centralize path-variant generation.
In `@packages/rari/src/vite/image-scanner.ts`:
- Around line 250-322: The extractBalancedBraces function conflates
template-expression braces with object-literal braces using a single braceCount;
split these concerns by introducing two counters (e.g., objBraceCount and
templateExprCount) and only decrement/increment the appropriate counter
depending on context: use templateExprCount for tracking ${...} expressions
inside backtick strings (increment when seeing ${ and decrement on matching }
inside that expression) and use objBraceCount for top-level { } object literals
(increment/decrement only when not inside a template expression or string).
Update the logic around inString/stringChar/templateDepth to (1) increment
templateExprCount when you encounter '${' while in a backtick string, (2) route
'}' handling to decrement templateExprCount when templateExprCount>0, otherwise
decrement objBraceCount, and (3) return the substring when objBraceCount reaches
zero; ensure escaped chars and nested backtick/template backtracking still
respect these separate counters (symbols: extractBalancedBraces, braceCount ->
replace uses with objBraceCount/templateExprCount, inString, stringChar,
templateDepth).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c95941b-2241-47c1-8bff-646c0bd18ca3
📒 Files selected for processing (9)
crates/rari/src/rsc/rendering/core/js/component_eval_setup.jscrates/rari/src/server/middleware/request_context.rspackages/deploy/src/utils.tspackages/rari/src/proxy/shared/utils.tspackages/rari/src/router/navigation-utils.tspackages/rari/src/router/sitemap-generator.tspackages/rari/src/runtime/shared/get-client-component.tspackages/rari/src/vite/image-scanner.tstest/unit/router/navigation-utils.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rari/src/router/ClientRouter.tsx (1)
382-384:⚠️ Potential issue | 🔴 CriticalUse requested-path key for abort cleanup to avoid stale pending navigations.
pendingNavigationsRef.currentis stored undertargetPath, but abort cleanup sometimes deletes withactualTargetPath. On redirects these differ, leaving stale map entries and causing later navigations to short-circuit to an old promise.🔧 Proposed fix
- const cleanupAbortedNavigation = (path: string, navigationId: number) => { - pendingNavigationsRef.current.delete(path) + const cleanupAbortedNavigation = (pendingPath: string, navigationId: number) => { + pendingNavigationsRef.current.delete(pendingPath) @@ - const handleStreamingResponse = async ( + const handleStreamingResponse = async ( response: Response, abortController: AbortController, + pendingPath: string, actualTargetPath: string, navigationId: number, @@ if (abortController.signal.aborted) { await reader.cancel() - cleanupAbortedNavigation(actualTargetPath, navigationId) + cleanupAbortedNavigation(pendingPath, navigationId) return } @@ if (abortController.signal.aborted) { - cleanupAbortedNavigation(actualTargetPath, navigationId) + cleanupAbortedNavigation(targetPath, navigationId) return } @@ await handleStreamingResponse( response, abortController, + targetPath, actualTargetPath, navigationId, @@ if (abortController.signal.aborted) { - cleanupAbortedNavigation(actualTargetPath, navigationId) + cleanupAbortedNavigation(targetPath, navigationId) return }Also applies to: 449-450, 671-672, 710-711, 724-729
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/ClientRouter.tsx` around lines 382 - 384, The cleanup function cleanupAbortedNavigation is deleting pendingNavigationsRef.current entries using the resolved/actual path (variable named path) but the map is keyed by the originally requested targetPath, causing stale entries on redirects; update cleanupAbortedNavigation (and the other cleanup spots that call pendingNavigationsRef.current.delete) to delete using the requested/requestPath/requestedTargetPath key that was used when the entry was created (use the same identifier stored when creating the pending navigation promise instead of actualTargetPath) so redirects remove the correct map entry.
♻️ Duplicate comments (3)
crates/rari/src/rsc/rendering/core/js/component_eval_setup.js (1)
25-28:⚠️ Potential issue | 🔴 Critical
_jsx/_jsxsfallbacks incorrectly passkeyas a child instead of merging into props.The JSX runtime signature is
jsx(type, props, key), butcreateElementat line 4 uses(type, props, ...children). Passingkeyas the third argument treats it as a child, corrupting the element. Note thatglobalThis.jsxat line 35 correctly merges key into props—apply the same pattern here.Suggested fix
if (typeof _jsx === 'undefined') - var _jsx = globalThis['~react']?.jsxRuntime?.jsx || globalThis.jsx || ((type, props, key) => globalThis.React.createElement(type, props, key)) + var _jsx = globalThis['~react']?.jsxRuntime?.jsx || globalThis.jsx || ((type, props, key) => globalThis.React.createElement(type, key === undefined ? props : { ...(props || {}), key })) if (typeof _jsxs === 'undefined') - var _jsxs = globalThis['~react']?.jsxRuntime?.jsxs || globalThis.jsxs || ((type, props, key) => globalThis.React.createElement(type, props, key)) + var _jsxs = globalThis['~react']?.jsxRuntime?.jsxs || globalThis.jsxs || ((type, props, key) => globalThis.React.createElement(type, key === undefined ? props : { ...(props || {}), key }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/rsc/rendering/core/js/component_eval_setup.js` around lines 25 - 28, The fallback definitions for _jsx and _jsxs pass the third param `key` directly to globalThis.React.createElement as a child, which corrupts the element; change the fallback to merge the key into the props object (e.g., if key is not undefined, create a new propsWithKey = { ...props, key } and pass that as the second argument) so that _jsx and _jsxs behave like the real runtime; update the _jsx and _jsxs fallback expressions that reference globalThis.React.createElement to merge key into props before calling createElement.packages/rari/src/router/ClientRouter.tsx (1)
66-69:⚠️ Potential issue | 🟠 MajorClear stale
<title>andviewportmetadata when omitted.
updateBasicMetadatanow removes some absent fields, but missingtitle/viewportvalues still leave previous-route values in<head>, so metadata leakage is only partially fixed.💡 Proposed fix
function updateBasicMetadata(metadata: PageMetadata): void { - if (metadata.title) - document.title = metadata.title + if (metadata.title !== undefined) + document.title = metadata.title + else + document.title = '' @@ if (metadata.viewport) { updateOrCreateMetaTag('meta[name="viewport"]', { name: 'viewport', content: metadata.viewport, }) } + else { + removeMetaTag('meta[name="viewport"]') + } }Also applies to: 90-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/ClientRouter.tsx` around lines 66 - 69, updateBasicMetadata currently only sets document.title/viewport when provided but doesn't clear stale values; modify updateBasicMetadata so that if metadata.title is absent it clears document.title (e.g., set to empty string) and if metadata.viewport is absent it removes any existing <meta name="viewport"> or clears its content; similarly apply the same clear/remove behavior in the other metadata updater that handles title/viewport (the analogous metadata update function around lines 90-96) so omitted fields no longer leak previous-route values.packages/rari/src/proxy/execute-proxy.ts (1)
24-30: 🧹 Nitpick | 🔵 TrivialSimpler pattern avoids potential timing edge case.
The current
catch((err) => { throw err })creates a new promise that rejects, whichallSettledwill observe. However, there's a window between whenhandledPromiserejects and whenfinallycallsallSettledwhere Node could emit an unhandledRejection warning.The simpler pattern attaches a no-op handler to suppress the warning while pushing the original promise to preserve its rejection state:
Suggested simplification
const event = { waitUntil: (promise: Promise<unknown>) => { - const handledPromise = promise.catch((err) => { - throw err - }) - waitUntilPromises.push(handledPromise) + promise.catch(() => {}) // suppress unhandled rejection warning + waitUntilPromises.push(promise) }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/proxy/execute-proxy.ts` around lines 24 - 30, The waitUntil implementation currently wraps the incoming promise into handledPromise via .catch(err => { throw err }), which can cause an unhandledRejection race; instead attach a noop rejection handler to suppress warnings and push the original promise to waitUntilPromises so its rejection state is preserved. Locate the event.waitUntil function and replace the creation of handledPromise with adding a .catch(() => {}) (or equivalent no-op) on the original promise for warning suppression, then push the original promise (not the wrapped one) onto waitUntilPromises while preserving behavior when later awaited with Promise.allSettled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rari/src/image/Image.tsx`:
- Around line 85-86: The preload link only sets imagesizes but not imagesrcset,
so responsive preloading won't work; update the logic in Image.tsx where the
preload <link> is created (the block calling link.setAttribute('imagesizes',
sizes)) to also set the imagesrcset attribute when sizes is provided by
computing or passing the same srcSet value used for the <img> (e.g., the
component's srcSet/computeSrcSet function or prop) and calling
link.setAttribute('imagesrcset', srcSet) so the browser can choose the correct
image candidate.
---
Outside diff comments:
In `@packages/rari/src/router/ClientRouter.tsx`:
- Around line 382-384: The cleanup function cleanupAbortedNavigation is deleting
pendingNavigationsRef.current entries using the resolved/actual path (variable
named path) but the map is keyed by the originally requested targetPath, causing
stale entries on redirects; update cleanupAbortedNavigation (and the other
cleanup spots that call pendingNavigationsRef.current.delete) to delete using
the requested/requestPath/requestedTargetPath key that was used when the entry
was created (use the same identifier stored when creating the pending navigation
promise instead of actualTargetPath) so redirects remove the correct map entry.
---
Duplicate comments:
In `@crates/rari/src/rsc/rendering/core/js/component_eval_setup.js`:
- Around line 25-28: The fallback definitions for _jsx and _jsxs pass the third
param `key` directly to globalThis.React.createElement as a child, which
corrupts the element; change the fallback to merge the key into the props object
(e.g., if key is not undefined, create a new propsWithKey = { ...props, key }
and pass that as the second argument) so that _jsx and _jsxs behave like the
real runtime; update the _jsx and _jsxs fallback expressions that reference
globalThis.React.createElement to merge key into props before calling
createElement.
In `@packages/rari/src/proxy/execute-proxy.ts`:
- Around line 24-30: The waitUntil implementation currently wraps the incoming
promise into handledPromise via .catch(err => { throw err }), which can cause an
unhandledRejection race; instead attach a noop rejection handler to suppress
warnings and push the original promise to waitUntilPromises so its rejection
state is preserved. Locate the event.waitUntil function and replace the creation
of handledPromise with adding a .catch(() => {}) (or equivalent no-op) on the
original promise for warning suppression, then push the original promise (not
the wrapped one) onto waitUntilPromises while preserving behavior when later
awaited with Promise.allSettled.
In `@packages/rari/src/router/ClientRouter.tsx`:
- Around line 66-69: updateBasicMetadata currently only sets
document.title/viewport when provided but doesn't clear stale values; modify
updateBasicMetadata so that if metadata.title is absent it clears document.title
(e.g., set to empty string) and if metadata.viewport is absent it removes any
existing <meta name="viewport"> or clears its content; similarly apply the same
clear/remove behavior in the other metadata updater that handles title/viewport
(the analogous metadata update function around lines 90-96) so omitted fields no
longer leak previous-route values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76b5c7b8-e5ab-47cc-b0a0-dd596ed3bdd4
📒 Files selected for processing (5)
crates/rari/src/rsc/rendering/core/js/component_eval_setup.jspackages/rari/src/image/Image.tsxpackages/rari/src/proxy/execute-proxy.tspackages/rari/src/proxy/executor.tspackages/rari/src/router/ClientRouter.tsx
- Fix JSX runtime to properly handle key prop by merging with props object - Extract merge_and_sort_tags helper to reduce duplication in fetch cache tag handling - Update fetch cache to persist guard state after tag merging - Add imagesrcset attribute generation for image preloading optimization - Simplify waitUntil promise handling by removing unnecessary error wrapping - Extract mergeHeader helper to consolidate header merging logic in proxy utilities - Normalize header key comparison to lowercase for consistent proxy header extraction - Improve code maintainability by reducing repeated tag merging and header processing logic
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/rari/src/server/middleware/request_context.rs (1)
89-93:⚠️ Potential issue | 🟠 MajorExclude
tagsfrom cache key generation to prevent cache fragmentation.At Line 92,
tagsare still treated as cache-relevant options. Requests that differ only by tags will miss the same cache entry/in-flight lock, which defeats cross-request tag accumulation and causes unnecessary upstream fetches.💡 Proposed fix
- .filter(|(k, _)| !matches!(k.as_str(), "cacheTTLMs" | "timeout")) + .filter(|(k, _)| !matches!(k.as_str(), "cacheTTLMs" | "timeout" | "tags"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rari/src/server/middleware/request_context.rs` around lines 89 - 93, The cache key generator generate_cache_key currently treats "tags" as cache-relevant and thus includes it in cache_relevant_options; update the filter inside generate_cache_key (the closure building cache_relevant_options) to also exclude the "tags" key (i.e., add "tags" to the matches! pattern alongside "cacheTTLMs" and "timeout") so that requests differing only by tags map to the same cache/in-flight lock and avoid cache fragmentation.packages/rari/src/proxy/execute-proxy.ts (1)
12-17: 🧹 Nitpick | 🔵 TrivialRace condition during concurrent initialization is mitigated by internal synchronization.
Multiple requests may simultaneously see
isInitialized()returnfalseand callinitializeProxyFromManifest(). This causes the manifest file to be read redundantly (minor overhead). However, the proxy loading itself is already synchronized:ProxyExecutor.loadProxy()caches its initialization promise, so only the first caller executesdoLoadProxy()while others await the same promise.The suggested module-level caching won't prevent this race condition in practice—concurrent requests will still see
isInitialized()asfalse—and introduces issues in serverless environments where module state resets between invocations. No immediate action needed; redundant manifest reads are acceptable given the internal synchronization already in place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/proxy/execute-proxy.ts` around lines 12 - 17, Do not add module-level caching; the race where multiple callers see executor.isInitialized() === false is harmless because ProxyExecutor.loadProxy() caches the initialization promise so only the first caller runs doLoadProxy() while others await it; leave the existing initializeProxyFromManifest('./dist/proxy-manifest.json') call as-is and add a brief inline comment near isInitialized/initializeProxyFromManifest explaining that ProxyExecutor.loadProxy() handles synchronization (references: isInitialized, initializeProxyFromManifest, ProxyExecutor.loadProxy, doLoadProxy).packages/rari/src/router/ClientRouter.tsx (1)
382-390:⚠️ Potential issue | 🟡 MinorState update is a no-op that triggers unnecessary re-render.
The
setNavigationStatecall at lines 386-388 spreadsprevwithout changing any values, creating a new object that's functionally identical. This triggers an unnecessary React re-render.Either remove the state update if no changes are needed, or add the intended state changes.
Proposed fix: remove no-op state update or clarify intent
const cleanupAbortedNavigation = (pendingPath: string, navigationId: number) => { pendingNavigationsRef.current.delete(pendingPath) - - if (isMountedRef.current && navigationState.navigationId === navigationId) { - setNavigationState(prev => ({ - ...prev, - })) - } }If a re-render is intentionally needed to sync state, add a comment explaining why:
const cleanupAbortedNavigation = (pendingPath: string, navigationId: number) => { pendingNavigationsRef.current.delete(pendingPath) if (isMountedRef.current && navigationState.navigationId === navigationId) { + // Force re-render to ensure UI reflects aborted navigation state setNavigationState(prev => ({ ...prev, })) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/ClientRouter.tsx` around lines 382 - 390, The setNavigationState call inside cleanupAbortedNavigation is a no-op (it spreads prev without changes) and causes an unnecessary re-render; either remove the setNavigationState invocation entirely or replace it with the actual state mutation you intended (e.g., updating navigationState properties) and ensure pendingNavigationsRef.current.delete(pendingPath) is preserved; if you intentionally relied on forcing a re-render, replace the call with an explicit intent (e.g., toggle a dedicated renderKey in navigationState) or add a clear comment explaining why the no-op update is required. Ensure changes are made in the cleanupAbortedNavigation function and reference navigationState, pendingNavigationsRef, isMountedRef, and setNavigationState accordingly.
♻️ Duplicate comments (5)
packages/rari/src/runtime/shared/get-client-component.ts (1)
50-53:⚠️ Potential issue | 🟠 MajorUse nullish checks for loaded component guards.
Line 52 and Line 67 still use truthy checks, so valid falsy exports (
0,'',false) are treated as missing. This can returnnullafter a successful load and can also re-enter the loader path unnecessarily.Suggested fix
if (componentInfo.loader) { const loadedComponent = await executeLoader(componentInfo) - if (!loadedComponent) + if (loadedComponent == null) return null @@ function triggerComponentLoad(componentInfo: LazyComponentInfo): Promise<any> { - if (!componentInfo.loader || componentInfo.loading || componentInfo.component || componentInfo.loadPromise) + if (!componentInfo.loader || componentInfo.loading || componentInfo.component != null || componentInfo.loadPromise) return Promise.resolve(null)Also applies to: 66-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/runtime/shared/get-client-component.ts` around lines 50 - 53, The current truthy guards around the loader result treat valid falsy exports (0, '', false) as missing; in the loader flow (when componentInfo.loader is present) and the subsequent guard use nullish checks instead of truthy checks: replace checks like `if (!loadedComponent)` and any other `!` truthy guard for the result with a nullish check (e.g., `if (loadedComponent == null)` or `if (loadedComponent === null || loadedComponent === undefined)`) so executeLoader's return values (see executeLoader and the loadedComponent variable in getClientComponent) are preserved when they are valid falsy values and to avoid re-entering the loader path unnecessarily.packages/rari/src/router/ClientRouter.tsx (2)
66-68: 🧹 Nitpick | 🔵 TrivialConsider clearing or resetting document.title when metadata.title is absent.
The function sets
document.titlewhen provided but doesn't handle the case when it's absent. Unlike meta tags that can be removed,document.titlealways exists. If a page doesn't specify a title, the previous page's title persists.Consider setting a default fallback title when
metadata.titleis undefined.Proposed fix
function updateBasicMetadata(metadata: PageMetadata): void { if (metadata.title) document.title = metadata.title + // Optionally: else document.title = 'Default Site Title'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/ClientRouter.tsx` around lines 66 - 68, The updateBasicMetadata function only sets document.title when metadata.title exists, leaving the previous title if absent; modify updateBasicMetadata (and reference PageMetadata) to set document.title to a sensible default (e.g., an app-level fallback string or empty string) when metadata.title is undefined or empty so the title is cleared/reset for pages that don't provide one. Ensure the fallback is a constant or config value used elsewhere so it's easy to change.
659-671:⚠️ Potential issue | 🟠 MajorHistory entry is committed before response body is fully processed.
The history state is pushed/replaced (lines 668-671) after the fetch response headers are received but before the response body is fully processed. If streaming or response parsing fails after this point, the address bar will show the new URL while the content remains on the previous page.
Consider deferring the history update until after
handleStreamingResponseorhandleNonStreamingResponsecompletes successfully, or use a two-phase approach: update history optimistically but restore it on failure.Proposed fix: defer history update to completeNavigation
- const urlWithHash = hash ? `${actualTargetPath}#${hash}` : actualTargetPath - const historyState: HistoryState = { - route: actualTargetPath, - navigationId, - scrollPosition: { x: window.scrollX, y: window.scrollY }, - timestamp: Date.now(), - key: historyKey, - } - - if (options.replace) - window.history.replaceState(historyState, '', urlWithHash) - else - window.history.pushState(historyState, '', urlWithHash) - processMetadata(response) // ... streaming/non-streaming handling ... - completeNavigation(actualTargetPath, hash, options, navigationId) + completeNavigation(actualTargetPath, hash, options, navigationId, historyKey) // Then in completeNavigation, add history update: const completeNavigation = ( actualTargetPath: string, hash: string, options: NavigationOptions, navigationId: number, + historyKey: string, ) => { // ... existing code ... + const urlWithHash = hash ? `${actualTargetPath}#${hash}` : actualTargetPath + const historyState: HistoryState = { + route: actualTargetPath, + navigationId, + scrollPosition: { x: window.scrollX, y: window.scrollY }, + timestamp: Date.now(), + key: historyKey, + } + if (options.replace) + window.history.replaceState(historyState, '', urlWithHash) + else + window.history.pushState(historyState, '', urlWithHash) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/router/ClientRouter.tsx` around lines 659 - 671, The history update (window.history.pushState/replaceState using urlWithHash and historyState) is happening before the response body is fully processed; move that logic into the navigation completion path so the URL is only committed after handleStreamingResponse or handleNonStreamingResponse finishes successfully. Specifically, remove or defer the push/replace calls near urlWithHash/historyState and instead call them from completeNavigation (or the function that runs after the response handlers complete), using the same historyState and honoring options.replace; if processing fails, restore the previous history or avoid mutating it (implement an optimistic two‑phase approach only if you also roll back on failure).packages/rari/src/image/Image.tsx (2)
113-115: 🧹 Nitpick | 🔵 TrivialAvoid render-phase state updates in
useImageLazyLoad.Line 113 updates state during render. This works conditionally, but it makes render impure and is harder to reason about in Strict Mode. Prefer effect-based synchronization.
💡 Proposed refactor
- const prevShouldLoadImmediatelyRef = useRef(shouldLoadImmediately) - - if (shouldLoadImmediately && !prevShouldLoadImmediatelyRef.current && !hasIntersected) - setHasIntersected(true) - prevShouldLoadImmediatelyRef.current = shouldLoadImmediately + useEffect(() => { + if (shouldLoadImmediately) + setHasIntersected(true) + }, [shouldLoadImmediately])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/image/Image.tsx` around lines 113 - 115, The render currently mutates state in useImageLazyLoad by calling setHasIntersected(true) when shouldLoadImmediately changes, which is impure; move that logic into an effect: create a useEffect that depends on shouldLoadImmediately and hasIntersected, and inside it check if shouldLoadImmediately && !prevShouldLoadImmediatelyRef.current && !hasIntersected then call setHasIntersected(true); then update prevShouldLoadImmediatelyRef.current = shouldLoadImmediately inside the same effect (or a separate effect) to keep render pure; reference the useImageLazyLoad hook and the variables prevShouldLoadImmediatelyRef, shouldLoadImmediately, hasIntersected, and setHasIntersected when making the change.
85-92:⚠️ Potential issue | 🟠 MajorPreload candidate generation diverges from rendered image candidates.
At Line 85-Line 89,
imagesrcsetis skipped whenloaderis provided and is always generated withwdescriptors. But fixed-width rendering at Line 308 uses DPR descriptors (1x/2x/3x), so preload can select a different resource than the eventual<img>, reducing cache hit probability and wasting bandwidth.💡 Proposed fix
- if (!unoptimized && !loader) { - const sizesArray = imgWidth ? [imgWidth, imgWidth * 2, imgWidth * 3] : DEFAULT_DEVICE_SIZES - const srcSet = sizesArray.map(w => `${buildImageUrl(finalSrc, w, quality)} ${w}w`).join(', ') - link.setAttribute('imagesrcset', srcSet) - } - - if (sizes) - link.setAttribute('imagesizes', sizes) + if (!unoptimized) { + const hasFixedWidth = !!imgWidth + const sizesArray = imgWidth ? [imgWidth, imgWidth * 2, imgWidth * 3] : DEFAULT_DEVICE_SIZES + const srcSet = buildSrcSetString(sizesArray, finalSrc, quality, undefined, loader, hasFixedWidth) + link.setAttribute('imagesrcset', srcSet) + + if (!hasFixedWidth && sizes) + link.setAttribute('imagesizes', sizes) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/image/Image.tsx` around lines 85 - 92, The preload imagesrcset generation currently skips when loader exists and always uses width (w) descriptors, which mismatches the rendered image's DPR descriptors; update the block that sets srcSet so when the rendered image will use DPR descriptors (i.e., fixed-width rendering path), produce imagesrcset with DPR descriptors (1x/2x/3x) by mapping desired DPRs to buildImageUrl(finalSrc, Math.round(imgWidth * dpr), quality) and joining as `${url} ${dpr}x`; ensure this logic lives alongside the existing unoptimized/loader checks and updates the same variables (sizesArray/srcSet/imagesrcset) so preload candidates match the eventual <img> candidates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rari/src/rsc/rendering/core/js/component_eval_setup.js`:
- Around line 31-35: The wrappers globalThis.jsx and globalThis.jsxs currently
always add a key property (creating key: undefined) when building props; change
them to only materialize key when it's provided by checking if key is !==
undefined and, if so, return React.createElement(type, { ...props, key });
otherwise return React.createElement(type, props) so unkeyed elements do not
gain an own-property key; update both globalThis.jsx and globalThis.jsxs
accordingly.
In `@packages/rari/src/router/ClientRouter.tsx`:
- Around line 90-96: The viewport meta handling should mirror
description/keywords removal: if metadata.viewport is present call
updateOrCreateMetaTag('meta[name="viewport"]', { name: 'viewport', content:
metadata.viewport }), otherwise remove the tag so a previous page's viewport
doesn't persist — add an else branch that calls the same removal helper used for
description/keywords (e.g., removeMetaTag('meta[name="viewport"]')) next to the
existing updateOrCreateMetaTag usage and reference metadata.viewport and the
removeMetaTag/updateOrCreateMetaTag helpers.
- Around line 571-603: handleNavigationError currently results in two different
custom events for the same failure—errorHandlerRef.current.handleError(...)
emits 'rari:navigation-error' with only navError while handleNavigationError
emits 'rari:navigate-error' with full {from,to,error,navigationId}; consolidate
them by emitting a single event (preferably 'rari:navigate-error') containing
the full shape from handleNavigationError and remove or disable the event
emission inside errorHandlerRef.current.handleError, or alternatively document
both usages and make their detail shapes consistent; update the code paths
around handleNavigationError and errorHandlerRef.current.handleError to ensure
only one event name/shape is dispatched (or clearly documented) and adjust any
consumers to the unified format.
In `@packages/rari/src/runtime/shared/get-client-component.ts`:
- Around line 220-221: The early return when componentInfo is truthy calls
ensureComponentLoaded(componentInfo) but doesn't forward the exportName, so
direct-id hits containing a "#" can return the whole module instead of the named
export; update the call in get-client-component to forward the exportName (e.g.,
ensureComponentLoaded(componentInfo, exportName) or otherwise include exportName
on componentInfo) so ensureComponentLoaded receives the exportName and returns
the correct exported member.
---
Outside diff comments:
In `@crates/rari/src/server/middleware/request_context.rs`:
- Around line 89-93: The cache key generator generate_cache_key currently treats
"tags" as cache-relevant and thus includes it in cache_relevant_options; update
the filter inside generate_cache_key (the closure building
cache_relevant_options) to also exclude the "tags" key (i.e., add "tags" to the
matches! pattern alongside "cacheTTLMs" and "timeout") so that requests
differing only by tags map to the same cache/in-flight lock and avoid cache
fragmentation.
In `@packages/rari/src/proxy/execute-proxy.ts`:
- Around line 12-17: Do not add module-level caching; the race where multiple
callers see executor.isInitialized() === false is harmless because
ProxyExecutor.loadProxy() caches the initialization promise so only the first
caller runs doLoadProxy() while others await it; leave the existing
initializeProxyFromManifest('./dist/proxy-manifest.json') call as-is and add a
brief inline comment near isInitialized/initializeProxyFromManifest explaining
that ProxyExecutor.loadProxy() handles synchronization (references:
isInitialized, initializeProxyFromManifest, ProxyExecutor.loadProxy,
doLoadProxy).
In `@packages/rari/src/router/ClientRouter.tsx`:
- Around line 382-390: The setNavigationState call inside
cleanupAbortedNavigation is a no-op (it spreads prev without changes) and causes
an unnecessary re-render; either remove the setNavigationState invocation
entirely or replace it with the actual state mutation you intended (e.g.,
updating navigationState properties) and ensure
pendingNavigationsRef.current.delete(pendingPath) is preserved; if you
intentionally relied on forcing a re-render, replace the call with an explicit
intent (e.g., toggle a dedicated renderKey in navigationState) or add a clear
comment explaining why the no-op update is required. Ensure changes are made in
the cleanupAbortedNavigation function and reference navigationState,
pendingNavigationsRef, isMountedRef, and setNavigationState accordingly.
---
Duplicate comments:
In `@packages/rari/src/image/Image.tsx`:
- Around line 113-115: The render currently mutates state in useImageLazyLoad by
calling setHasIntersected(true) when shouldLoadImmediately changes, which is
impure; move that logic into an effect: create a useEffect that depends on
shouldLoadImmediately and hasIntersected, and inside it check if
shouldLoadImmediately && !prevShouldLoadImmediatelyRef.current &&
!hasIntersected then call setHasIntersected(true); then update
prevShouldLoadImmediatelyRef.current = shouldLoadImmediately inside the same
effect (or a separate effect) to keep render pure; reference the
useImageLazyLoad hook and the variables prevShouldLoadImmediatelyRef,
shouldLoadImmediately, hasIntersected, and setHasIntersected when making the
change.
- Around line 85-92: The preload imagesrcset generation currently skips when
loader exists and always uses width (w) descriptors, which mismatches the
rendered image's DPR descriptors; update the block that sets srcSet so when the
rendered image will use DPR descriptors (i.e., fixed-width rendering path),
produce imagesrcset with DPR descriptors (1x/2x/3x) by mapping desired DPRs to
buildImageUrl(finalSrc, Math.round(imgWidth * dpr), quality) and joining as
`${url} ${dpr}x`; ensure this logic lives alongside the existing
unoptimized/loader checks and updates the same variables
(sizesArray/srcSet/imagesrcset) so preload candidates match the eventual <img>
candidates.
In `@packages/rari/src/router/ClientRouter.tsx`:
- Around line 66-68: The updateBasicMetadata function only sets document.title
when metadata.title exists, leaving the previous title if absent; modify
updateBasicMetadata (and reference PageMetadata) to set document.title to a
sensible default (e.g., an app-level fallback string or empty string) when
metadata.title is undefined or empty so the title is cleared/reset for pages
that don't provide one. Ensure the fallback is a constant or config value used
elsewhere so it's easy to change.
- Around line 659-671: The history update (window.history.pushState/replaceState
using urlWithHash and historyState) is happening before the response body is
fully processed; move that logic into the navigation completion path so the URL
is only committed after handleStreamingResponse or handleNonStreamingResponse
finishes successfully. Specifically, remove or defer the push/replace calls near
urlWithHash/historyState and instead call them from completeNavigation (or the
function that runs after the response handlers complete), using the same
historyState and honoring options.replace; if processing fails, restore the
previous history or avoid mutating it (implement an optimistic two‑phase
approach only if you also roll back on failure).
In `@packages/rari/src/runtime/shared/get-client-component.ts`:
- Around line 50-53: The current truthy guards around the loader result treat
valid falsy exports (0, '', false) as missing; in the loader flow (when
componentInfo.loader is present) and the subsequent guard use nullish checks
instead of truthy checks: replace checks like `if (!loadedComponent)` and any
other `!` truthy guard for the result with a nullish check (e.g., `if
(loadedComponent == null)` or `if (loadedComponent === null || loadedComponent
=== undefined)`) so executeLoader's return values (see executeLoader and the
loadedComponent variable in getClientComponent) are preserved when they are
valid falsy values and to avoid re-entering the loader path unnecessarily.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73d45c38-a92d-48df-a69c-400d43a53892
📒 Files selected for processing (8)
crates/rari/src/rsc/rendering/core/js/component_eval_setup.jscrates/rari/src/server/middleware/request_context.rspackages/rari/src/image/Image.tsxpackages/rari/src/proxy/execute-proxy.tspackages/rari/src/proxy/shared/utils.tspackages/rari/src/router/ClientRouter.tsxpackages/rari/src/router/navigation-utils.tspackages/rari/src/runtime/shared/get-client-component.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/rari/src/runtime/shared/get-client-component.ts (1)
66-68:⚠️ Potential issue | 🟡 MinorUse a nullish guard when checking loaded state in
triggerComponentLoad.Line 67 still uses a truthy check (
componentInfo.component), which can treat valid falsy values as “not loaded” and re-trigger loading.Suggested fix
function triggerComponentLoad(componentInfo: LazyComponentInfo): Promise<any> { - if (!componentInfo.loader || componentInfo.loading || componentInfo.component || componentInfo.loadPromise) + if (!componentInfo.loader || componentInfo.loading || componentInfo.component != null || componentInfo.loadPromise) return Promise.resolve(null) return executeLoader(componentInfo) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/runtime/shared/get-client-component.ts` around lines 66 - 68, The current guard in triggerComponentLoad uses a truthy check for componentInfo.component which will re-trigger loading for valid falsy components; update the conditional in triggerComponentLoad to use a nullish check (e.g., check componentInfo.component != null or componentInfo.component !== null && componentInfo.component !== undefined) alongside the existing checks for componentInfo.loader, componentInfo.loading, and componentInfo.loadPromise so that already-loaded falsy components are treated as loaded and the function returns Promise.resolve(null) only when truly not loaded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rari/src/rsc/rendering/core/js/component_eval_setup.js`:
- Around line 2-3: The React shim should guard against partial globals by
checking for required APIs instead of truthiness; update the initialization
checks (currently using if (!globalThis.React) and similar blocks) to also
verify required members like createElement (e.g., if (!globalThis.React ||
typeof globalThis.React.createElement !== 'function')) so the shim runs when a
partial React global exists and prevents crashes in calls that rely on
React.createElement (referenced around the wrapper calls near lines where
createElement is used). Apply the same pattern to the other shim checks
mentioned (the blocks at the other two occurrences) to ensure required methods
exist before skipping initialization.
In `@crates/rari/src/server/middleware/request_context.rs`:
- Around line 159-171: The code unnecessarily clones tags when calling
Self::merge_and_sort_tags inside the guard branch; since this branch returns
immediately, move tags instead of cloning to avoid the allocation—replace the
tags.clone() argument with tags (ensuring ownership is moved) when calling
Self::merge_and_sort_tags for cloned_result.tags and keep the rest of the logic
(cache put, setting *guard, and returning cloned_result) unchanged.
In `@packages/rari/src/router/navigation-error-handler.ts`:
- Around line 136-139: The code assumes (error as any).status is numeric;
validate and coerce it before calling handleHttpError to avoid silent failures.
In navigation-error-handler.ts, where you extract status from error, create a
statusNumber: if typeof status === 'number' use it; else if typeof status ===
'string' and it matches a numeric pattern (e.g., /^\d+$/) parseInt to a number;
otherwise set statusNumber to undefined (or null) and pass that to
handleHttpError(error, statusNumber, url). This ensures handleHttpError always
receives a proper number or a clear empty value for non-numeric statuses.
---
Duplicate comments:
In `@packages/rari/src/runtime/shared/get-client-component.ts`:
- Around line 66-68: The current guard in triggerComponentLoad uses a truthy
check for componentInfo.component which will re-trigger loading for valid falsy
components; update the conditional in triggerComponentLoad to use a nullish
check (e.g., check componentInfo.component != null or componentInfo.component
!== null && componentInfo.component !== undefined) alongside the existing checks
for componentInfo.loader, componentInfo.loading, and componentInfo.loadPromise
so that already-loaded falsy components are treated as loaded and the function
returns Promise.resolve(null) only when truly not loaded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f93077c-5609-4cad-bd96-2c7a039ef110
📒 Files selected for processing (5)
crates/rari/src/rsc/rendering/core/js/component_eval_setup.jscrates/rari/src/server/middleware/request_context.rspackages/rari/src/router/navigation-error-handler.tspackages/rari/src/runtime/shared/get-client-component.tstest/unit/router/navigation-error-handler.test.ts
💤 Files with no reviewable changes (1)
- test/unit/router/navigation-error-handler.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/rari/src/runtime/shared/get-client-component.ts (1)
114-123:⚠️ Potential issue | 🟠 MajorDirect
#exportIDs still bypass export resolution in sync path.On Line 119,
resolveById()returnscomponentInfo.componentdirectly. For direct IDs that include#namedExport, this can return the wrong value instead of the requested export.Suggested fix
function resolveById(id: string, clientComponents: Record<string, ComponentInfo>): any { const componentInfo = clientComponents[id] as LazyComponentInfo if (!componentInfo) return null - if (componentInfo.component != null) - return componentInfo.component + const hashIndex = id.indexOf('#') + const exportName = hashIndex === -1 ? '' : id.slice(hashIndex + 1) + const component = getComponentFromInfo(componentInfo, exportName) + if (component !== null && component !== undefined) + return component tryLoadComponent(componentInfo) return null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/runtime/shared/get-client-component.ts` around lines 114 - 123, resolveById currently returns componentInfo.component directly which bypasses resolving named exports for direct IDs that include a "#exportName"; update resolveById to detect if id contains a "#" (parse the export name), and when componentInfo.component is already loaded return the requested export (e.g., componentInfo.component[exportName]) instead of the whole module; if no "#" return componentInfo.component as before; keep the existing lazy load behavior (tryLoadComponent(componentInfo)) when component isn't loaded. Ensure you reference resolveById, componentInfo.component, and tryLoadComponent in your changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rari/src/rsc/rendering/core/js/component_eval_setup.js`:
- Around line 25-28: The duplication between _jsx/_jsxs and
globalThis.jsx/globalThis.jsxs should be consolidated: add a single helper
(e.g., createJsxDelegate or getJsxFactory) that encapsulates the
delegation/search order and key-merging logic, then assign _jsx =
createJsxDelegate('jsx', globalThis['~react']?.jsxRuntime?.jsx, globalThis.jsx,
globalThis.React) and similarly _jsxs using the same helper; update the existing
_jsx/_jsxs initialization and the globalThis aliasing (also the similar block
around lines 31-50) to call this helper so both factories share identical
key-handling and fallback behavior.
In `@crates/rari/src/server/middleware/request_context.rs`:
- Around line 189-193: The code currently calls result.tags.clone() when merging
tags after perform_fetch, causing an unnecessary allocation; replace that clone
with std::mem::take(&mut result.tags) so you move out the existing Vec (leaving
an empty Vec in its place) and pass ownership into Self::merge_and_sort_tags,
then assign the merged Vec back to result.tags; update the block around
fetch_result handling (the
fetch_result/perform_fetch/result.tags/merge_and_sort_tags usage) to use
mem::take instead of clone.
In `@packages/rari/src/runtime/shared/get-client-component.ts`:
- Around line 196-202: The path-based resolution is incorrectly skipped by the
id.includes('#') guard, making resolveByPathWithExport()'s non-# path handling
unreachable; remove the id.includes('#') check and always call
resolveByPathWithExport(id, clientComponents, clientComponentPaths) first,
returning its non-null result, and only then fall back to resolveByName(id,
clientComponents, clientComponentNames) if path resolution returns null.
---
Duplicate comments:
In `@packages/rari/src/runtime/shared/get-client-component.ts`:
- Around line 114-123: resolveById currently returns componentInfo.component
directly which bypasses resolving named exports for direct IDs that include a
"#exportName"; update resolveById to detect if id contains a "#" (parse the
export name), and when componentInfo.component is already loaded return the
requested export (e.g., componentInfo.component[exportName]) instead of the
whole module; if no "#" return componentInfo.component as before; keep the
existing lazy load behavior (tryLoadComponent(componentInfo)) when component
isn't loaded. Ensure you reference resolveById, componentInfo.component, and
tryLoadComponent in your changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4f0bf55a-ea16-437f-8f7d-7cce9431a42a
📒 Files selected for processing (5)
crates/rari/src/rsc/rendering/core/js/component_eval_setup.jscrates/rari/src/server/middleware/request_context.rspackages/rari/src/router/navigation-error-handler.tspackages/rari/src/runtime/shared/get-client-component.tstest/unit/router/navigation-error-handler.test.ts
💤 Files with no reviewable changes (1)
- test/unit/router/navigation-error-handler.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/rari/src/runtime/shared/get-client-component.ts (1)
226-253:⚠️ Potential issue | 🟠 MajorAsync resolver still diverges from sync lookup semantics.
Line 226 uses
clientComponents[id]directly, soidvalues likefoo#Barwon’t hit a base-id registry key (foo).
Also, Line 233 gates path lookup on#, so non-hash path ids that sync resolution supports are skipped in async mode.Proposed fix
export async function getClientComponentAsync(id: string): Promise<any> { const clientComponents = (globalThis as unknown as GlobalWithRari)['~clientComponents'] || {} const clientComponentPaths = (globalThis as unknown as GlobalWithRari)['~clientComponentPaths'] || {} const clientComponentNames = (globalThis as unknown as GlobalWithRari)['~clientComponentNames'] || {} - let componentInfo = clientComponents[id] as LazyComponentInfo - - if (componentInfo) { - const exportName = id.includes('#') ? id.slice(id.indexOf('#') + 1) : undefined - return await ensureComponentLoaded(componentInfo, exportName) - } - - if (id.includes('#')) { - const hashIndex = id.indexOf('#') - const path = id.slice(0, hashIndex) - const exportName = id.slice(hashIndex + 1) - const variants = getPathVariants(path) - - for (const variant of variants) { - const componentId = clientComponentPaths[variant] - if (componentId) { - componentInfo = clientComponents[componentId] as LazyComponentInfo - if (componentInfo) - return await ensureComponentLoaded(componentInfo, exportName) - } - } - } + const hashIndex = id.indexOf('#') + const baseId = hashIndex === -1 ? id : id.slice(0, hashIndex) + const exportName = hashIndex === -1 ? undefined : id.slice(hashIndex + 1) + + let componentInfo = clientComponents[baseId] as LazyComponentInfo + if (componentInfo) + return await ensureComponentLoaded(componentInfo, exportName) + + const variants = getPathVariants(baseId) + for (const variant of variants) { + const componentId = clientComponentPaths[variant] + if (componentId) { + componentInfo = clientComponents[componentId] as LazyComponentInfo + if (componentInfo) + return await ensureComponentLoaded(componentInfo, exportName) + } + } const componentId = clientComponentNames[id] if (componentId) { componentInfo = clientComponents[componentId] as LazyComponentInfo if (componentInfo) - return await ensureComponentLoaded(componentInfo) + return await ensureComponentLoaded(componentInfo, exportName) } return null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/runtime/shared/get-client-component.ts` around lines 226 - 253, The async resolver in get-client-component.ts incorrectly only checks clientComponents[id] and only runs path-variant lookup when id contains '#', causing mismatches with sync lookup; update the logic in the block using clientComponents, clientComponentPaths, clientComponentNames and ensureComponentLoaded so that when id contains a '#', you also try the base id (slice before '#') against clientComponents and clientComponentNames, and run getPathVariants(path) and check clientComponentPaths for variants regardless of whether id includes a '#', returning await ensureComponentLoaded(componentInfo, exportName) when found; ensure exportName is derived correctly (part after '#') and preserve existing return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rari/src/rsc/rendering/core/js/component_eval_setup.js`:
- Around line 25-37: The current createJsxDelegate(runtimeJsx, globalJsx) can
choose the runtime jsx/jsxs that normalize keys (turning 0/'' into null); update
createJsxDelegate to accept an additional reactCreateElement parameter and
prefer a wrapper around globalThis.React.createElement (if typeof
reactCreateElement === 'function') before falling back to runtimeJsx or
globalJsx so falsy keys like 0 or '' are preserved; ensure the wrapper uses the
existing key handling (key !== undefined ? { ...props, key } : props) and then
change the _jsx/_jsxs initializers to call
createJsxDelegate(globalThis['~react']?.jsxRuntime?.jsx, globalThis.jsx,
globalThis.React?.createElement).
---
Duplicate comments:
In `@packages/rari/src/runtime/shared/get-client-component.ts`:
- Around line 226-253: The async resolver in get-client-component.ts incorrectly
only checks clientComponents[id] and only runs path-variant lookup when id
contains '#', causing mismatches with sync lookup; update the logic in the block
using clientComponents, clientComponentPaths, clientComponentNames and
ensureComponentLoaded so that when id contains a '#', you also try the base id
(slice before '#') against clientComponents and clientComponentNames, and run
getPathVariants(path) and check clientComponentPaths for variants regardless of
whether id includes a '#', returning await ensureComponentLoaded(componentInfo,
exportName) when found; ensure exportName is derived correctly (part after '#')
and preserve existing return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6a8bce9-5585-4e4a-af38-c20704a1d654
📒 Files selected for processing (4)
crates/rari/src/rsc/rendering/core/js/component_eval_setup.jscrates/rari/src/server/middleware/request_context.rspackages/rari/src/proxy/runtime-executor.tspackages/rari/src/runtime/shared/get-client-component.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rari/src/rsc/rendering/core/js/component_eval_setup.js`:
- Around line 50-51: The current check assumes document.getElementById exists
and can throw in partial DOM runtimes; update the guard in the initialization
block to ensure document is defined and document.getElementById is callable
(e.g. typeof document.getElementById === 'function' or 'getElementById' in
document) before calling document.getElementById('spinner-keyframes'), and keep
the existing fallback use of document.head ||
document.getElementsByTagName('head')[0] (referencing document.getElementById,
document.head, getElementsByTagName('head')[0], and the 'spinner-keyframes' id).
In `@packages/rari/src/runtime/shared/get-client-component.ts`:
- Around line 244-249: The name-based fallback currently looks up
clientComponentNames[id] and calls ensureComponentLoaded(componentInfo) without
preserving named-export semantics, so ids like "Button#Icon" lose the
exportName; update the fallback to parse the id for an exportName (split on '#'
to get baseName and exportName), resolve clientComponentNames using the baseName
if the full id key is missing, then pass the exportName through to
ensureComponentLoaded (or set it on the LazyComponentInfo before calling) so
ensureComponentLoaded receives the named export to load; reference
clientComponentNames, clientComponents, LazyComponentInfo,
ensureComponentLoaded, id and exportName when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a91aad1-05d8-4759-8330-8bb55ec0d47a
📒 Files selected for processing (2)
crates/rari/src/rsc/rendering/core/js/component_eval_setup.jspackages/rari/src/runtime/shared/get-client-component.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rari/src/runtime/shared/get-client-component.ts (1)
73-93: 🧹 Nitpick | 🔵 TrivialConsider extracting shared loading logic to reduce duplication.
loadClientComponentandexecuteLoader(lines 11-28) duplicate the component loading logic with different error handling strategies. While the divergent error handling (swallow vs rethrow) is intentional, the success path logic is identical.♻️ Potential consolidation approach
function executeLoaderCore( componentInfo: LazyComponentInfo, onError: (error: Error) => void, ): Promise<any> { componentInfo.loading = true componentInfo.loadPromise = componentInfo.loader!() .then((module: any) => { const component = module.default || module componentInfo.component = component componentInfo.registered = true componentInfo.loading = false return component }) .catch((error: Error) => { componentInfo.loading = false componentInfo.loadPromise = undefined onError(error) }) return componentInfo.loadPromise } // Then executeLoader rethrows and loadClientComponent swallows🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rari/src/runtime/shared/get-client-component.ts` around lines 73 - 93, The success-path of component loading in loadClientComponent and executeLoader is duplicated; extract that shared logic into a helper (e.g., executeLoaderCore) which accepts (componentInfo: LazyComponentInfo, onError: (err: Error) => void) and performs componentInfo.loading = true, sets componentInfo.loadPromise = componentInfo.loader!().then(module => { const c = module.default || module; componentInfo.component = c; componentInfo.registered = true; componentInfo.loading = false; return c }), and in .catch calls onError after resetting componentInfo.loading and componentInfo.loadPromise; then replace executeLoader to call executeLoaderCore with an onError that rethrows the error, and replace loadClientComponent to call executeLoaderCore with an onError that logs/swallow the error (maintain current behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rari/src/rsc/rendering/core/js/component_eval_setup.js`:
- Around line 55-60: The DOM-injection path uses getElementsByTagName,
createElement and appendChild without checking they exist, which can break in
partial-DOM environments; update the block around the head variable and spinner
injection (the code creating style with id 'spinner-keyframes' and assigning
textContent) to first verify document.getElementsByTagName,
document.createElement and head.appendChild are functions (and that head is
truthy), and bail out early if any are missing so you only call createElement,
set style.id/style.textContent, and appendChild when those helpers are present.
In `@packages/rari/src/runtime/shared/get-client-component.ts`:
- Around line 203-205: When the globalAccessor-backed maps (clientComponents,
clientComponentPaths, clientComponentNames) fall back to {} add a one-time debug
log to make this visible without changing behavior: detect the condition where
all three are empty after assignment and emit a single debug-level message (e.g.
via existing logger or console.debug) that includes which maps are missing and
context (e.g. file/function name). Implement the one-time suppression with a
module-level boolean sentinel (e.g. clientMapsWarned) so the log only appears
once and otherwise leave return values and logic unchanged; reference the
variables clientComponents, clientComponentPaths, clientComponentNames and
globalAccessor to locate where to add the check.
---
Outside diff comments:
In `@packages/rari/src/runtime/shared/get-client-component.ts`:
- Around line 73-93: The success-path of component loading in
loadClientComponent and executeLoader is duplicated; extract that shared logic
into a helper (e.g., executeLoaderCore) which accepts (componentInfo:
LazyComponentInfo, onError: (err: Error) => void) and performs
componentInfo.loading = true, sets componentInfo.loadPromise =
componentInfo.loader!().then(module => { const c = module.default || module;
componentInfo.component = c; componentInfo.registered = true;
componentInfo.loading = false; return c }), and in .catch calls onError after
resetting componentInfo.loading and componentInfo.loadPromise; then replace
executeLoader to call executeLoaderCore with an onError that rethrows the error,
and replace loadClientComponent to call executeLoaderCore with an onError that
logs/swallow the error (maintain current behavior).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2443a521-9e09-45b8-9568-fc44d2ac5ea2
📒 Files selected for processing (2)
crates/rari/src/rsc/rendering/core/js/component_eval_setup.jspackages/rari/src/runtime/shared/get-client-component.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores