Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a Vercel React best-practices skill (docs/metadata/rules), many UI primitives and layout refactors (sidebar decomposition, shared dialogs, FullPageMessage, PreviewLoader/TextLoop), broad component memoization, chat message type migration to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 54
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/contacts/contact-import-dialog.tsx (1)
187-194: 🛠️ Refactor suggestion | 🟠 MajorUse
cn()for conditional class merging in this dropzone block.Please replace template-string class concatenation with
cn()to match project conventions and avoid fragile class composition.♻️ Proposed refactor
+import { cn } from "@/lib/utils"; ... - <label + <label htmlFor={fileInputId} onDrop={handleDrop} onDragOver={handleDragOver} onDragLeave={handleDragLeave} - className={`relative block cursor-pointer rounded-lg border-2 border-dashed p-8 transition-all duration-200 ease-in-out${ - isDragging - ? "scale-[1.02] border-primary bg-primary/5" - : "border-muted-foreground/25 hover:border-muted-foreground/50 hover:bg-muted/50" - } - ${selectedFile ? "border-success/50 bg-success/5" : ""} - ${isLoading ? "pointer-events-none opacity-60" : ""} - `} + className={cn( + "relative block cursor-pointer rounded-lg border-2 border-dashed p-8 transition-all duration-200 ease-in-out", + isDragging + ? "scale-[1.02] border-primary bg-primary/5" + : "border-muted-foreground/25 hover:border-muted-foreground/50 hover:bg-muted/50", + selectedFile && "border-success/50 bg-success/5", + isLoading && "pointer-events-none opacity-60", + )} >As per coding guidelines, "Use
cn()utility function (clsx + tailwind-merge) for class merging".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/contacts/contact-import-dialog.tsx` around lines 187 - 194, The dropzone's className currently concatenates classes with a template string; replace that with the project's cn() utility to safely merge conditional classes—locate the className on the element inside ContactImportDialog (references: isDragging, selectedFile, isLoading) and change the template-string expression to a cn(...) call that passes the static classes and conditional objects/strings (e.g., { "scale-[1.02] border-primary bg-primary/5": isDragging, "border-success/50 bg-success/5": selectedFile, "pointer-events-none opacity-60": isLoading }) so tailwind-merge and clsx handle merging consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/vercel-react-best-practices/README.md:
- Line 122: The acknowledgment sentence uses the redundant phrase "Originally
created"; update that sentence in the README acknowledgement line to replace
"Originally created" with "created" so the phrasing reads more concise (e.g.,
change "Originally created by..." to "created by...") — locate the
acknowledgment sentence in .agents/skills/vercel-react-best-practices/README.md
and make this single-word replacement.
In
@.agents/skills/vercel-react-best-practices/rules/advanced-event-handler-refs.md:
- Around line 40-53: Clarify the React version requirement for useEffectEvent in
the alternative: update the text around the useWindowEvent example to state that
useEffectEvent is only available and stable in React 19.2+ (not in React 18),
and advise readers to use the ref-based pattern (as shown in the useWindowEvent
ref approach / handler ref pattern in lines 25–38) for compatibility with React
18 and earlier; mention the specific symbols useEffectEvent and useWindowEvent
so reviewers can find and update the example and wording.
In @.agents/skills/vercel-react-best-practices/rules/advanced-init-once.md:
- Line 10: The "Correct" example currently recommends a module-level mutable
guard (didInit) and useEffect([]) for one-time init; update the guidance to
explicitly scope that pattern to client-only/browser environments and warn it is
unsafe in Node/SSR where module-level state is shared across requests; add
alternative recommendations: for server-side init use per-request setup in
Server Components, loaders, or middleware, and for app-level browser init prefer
top-level entry-module initialization guarded by a client-only check instead of
relying on component useEffect([]) or shared module state like didInit.
In @.agents/skills/vercel-react-best-practices/rules/async-parallel.md:
- Around line 20-27: Update the explanatory text that claims Promise.all reduces
three independent requests to "1 round trip": replace the inaccurate wording
with a precise statement that Promise.all runs fetchUser, fetchPosts, and
fetchComments concurrently so their waiting times overlap, making total latency
roughly the maximum of the individual request latencies rather than the sum,
rather than implying it collapses multiple network calls into a single round
trip.
- Around line 22-27: The TypeScript example block using Promise.all is
space-indented; update the code fence so its interior uses tabs for indentation
(preserve the same lines and semantics for the array destructuring and the calls
to fetchUser(), fetchPosts(), fetchComments()) to comply with the
tab-indentation rule configured in biome.json.
In
@.agents/skills/vercel-react-best-practices/rules/async-suspense-boundaries.md:
- Around line 79-86: Update the example using use(dataPromise) to state that the
use() API requires React 19+ and that DataDisplay and DataSummary must be Client
Components (e.g., annotated as client) while the Promise should originate from a
Server Component; explicitly mention these constraints alongside the
DataDisplay, DataSummary, and use(dataPromise) symbols so readers know to only
use this pattern in React 19+ and with client-side components consuming
server-provided promises.
In @.agents/skills/vercel-react-best-practices/rules/bundle-barrel-imports.md:
- Around line 27-35: Update the Next.js config by moving optimizePackageImports
out of the experimental object into the top-level export (i.e., set
optimizePackageImports: ['lucide-react','@mui/material'] on the root
module.exports) so it matches Next.js v16.2+ usage, and edit the doc text that
cites performance numbers (lines referenced around the optimizePackageImports
example and the other metric at line ~56): either remove the quantitative
figures or replace them with explicit citations to official Vercel/Next.js
benchmarks; keep qualitative statements (e.g., tree-shaking reduces bundle size)
but ensure any numeric claims include a verifiable source.
In @.agents/skills/vercel-react-best-practices/rules/bundle-conditional.md:
- Around line 19-20: The doc line that says "typeof window !== 'undefined'
prevents bundling" is misleading; update the text near the if check (if (enabled
&& !frames && typeof window !== 'undefined') and the
import('./animation-frames.js') mention) to state that the typeof window guard
only prevents server-side execution at runtime but does not stop modern bundlers
(Next.js, Webpack, Vite, Rollup) from including the module in client bundles;
recommend clarifying that dynamic import with runtime-only guards prevents
execution on the server but additional bundler-specific configuration (e.g.,
dynamic import conditions, webpack ignore plugins, or Next.js dynamic import
options) is required to avoid including the module in the bundle.
In
@.agents/skills/vercel-react-best-practices/rules/bundle-defer-third-party.md:
- Around line 14-49: The example using `@vercel/analytics` as a blocking
third-party is misleading; update the document to either replace the
Analytics/RootLayout example with a more representative heavy library (e.g., use
Chart/Dashboard from a fictitious heavy-charting-lib and show dynamic import
with next/dynamic and ssr: false) or add a clarifying note near
Analytics/RootLayout that `@vercel/analytics` is already async/optimized and is
shown only as a pattern; ensure you reference the same symbols (Analytics,
RootLayout, dynamic) when editing and if replacing, introduce Chart and
Dashboard as the new example symbols.
In @.agents/skills/vercel-react-best-practices/rules/bundle-dynamic-imports.md:
- Around line 14-34: The TSX examples (the CodePanel component and the
MonacoEditor dynamic import) use space indentation but the repo requires tabs;
update both embedded snippets so each indentation level uses tabs instead of
spaces, i.e., change the leading spaces before import, const/ function
declarations, JSX return and dynamic import lines to tabs for the MonacoEditor
and CodePanel examples; keep the exact tokens (MonacoEditor, CodePanel, dynamic,
import('./monaco-editor')) and maintain existing line breaks and punctuation
while only converting leading spaces to tabs.
In @.agents/skills/vercel-react-best-practices/rules/bundle-preload.md:
- Line 50: Update the assertion in the rule that claims the `typeof window !==
'undefined'` guard prevents bundling; instead change the text to state that this
guard only prevents runtime execution in the browser but does not stop Webpack
from resolving and including the module in build-time bundles (the module can
still be present in chunks and only removed by later dead-code
elimination/minification). Replace the incorrect guidance with recommended
approaches for true bundle separation and SSR optimization such as using
file-level conventions (e.g., .server.js/.client.js), next/dynamic({ ssr: false
}), or "use server"/"use client" directives, and keep a mention of `typeof
window !== 'undefined'` as a runtime guard only.
In @.agents/skills/vercel-react-best-practices/rules/client-event-listeners.md:
- Around line 57-65: The useSWRSubscription callback currently omits the
required parameters and next() call; update the subscription callback for
useSWRSubscription('global-keydown', ...) to accept (key, { next }) and call
next(null, e) inside the handler after invoking keyCallbacks so events are
published to subscribers; ensure the handler variable and the returned cleanup
still add/remove the same window 'keydown' listener and keep using
keyCallbacks.get(e.key)!.forEach(cb => cb()) before calling next.
In @.agents/skills/vercel-react-best-practices/rules/client-swr-dedup.md:
- Around line 14-53: The embedded TSX examples use spaces for indentation;
update each block to use tabs instead to match the repo's biome.json formatting
rules—specifically adjust the examples containing the UserList function (both
fetch/useSWR variants), the StaticContent function using useImmutableSWR, and
the UpdateButton function using useSWRMutation so all JSX/TSX lines are
tab-indented rather than space-indented.
- Around line 38-52: Replace the non-official custom hook import/use in the
immutable example: change the import of useImmutableSWR and its usage to the
official SWR immutable API by importing useSWRImmutable from 'swr/immutable' and
calling useSWRImmutable('/api/config', fetcher) inside StaticContent; leave the
useSWRMutation import/usage for UpdateButton unchanged.
In
@.agents/skills/vercel-react-best-practices/rules/js-cache-function-results.md:
- Line 10: Update the guidance that currently recommends a module-level Map for
caching function results to explicitly warn against unbounded module-level state
in SSR and instead instruct using bounded caches (mention using an LRU
implementation like the lru-cache package with size limits, eviction policies,
and TTLs), recommend Next.js built-ins (reference unstable_cache for the App
Router and cacheHandlers config with LRU), advise external shared stores
(Redis/Upstash with LRU) for multi-instance deployments, clarify per-invocation
isolation in Vercel serverless (so module state does not persist across
invocations) and restrict this pattern to pure, deterministic functions only to
avoid stale/shared state and security/race issues.
In
@.agents/skills/vercel-react-best-practices/rules/js-cache-property-access.md:
- Around line 12-18: The claim "3 lookups × N iterations" is overstated and
needs clarification: update the text around the for-loop example (references:
arr.length, obj.config.settings.value, process) to explain that modern JS
engines (e.g., V8) optimize property access and often cache arr.length, so the
performance gain from hoisting obj.config.settings.value into a local variable
only matters in true hot paths (tight loops executed millions of times). Replace
the absolute assertion with a note that shows the alternative (cache nested
property into a local before the loop) and add guidance when to apply this
optimization (benchmark/profile first, large N, performance-critical code)
rather than as a general rule.
In @.agents/skills/vercel-react-best-practices/rules/js-length-check-first.md:
- Around line 36-41: The manual element-by-element comparison can be simplified:
replace the for-loop that checks currentSorted and originalSorted with a single
comparison using their string representations (e.g., join) so the function
returns the result of currentSorted.join(...) !== originalSorted.join(...);
update the return accordingly and keep using the existing variables
currentSorted and originalSorted to locate the change.
- Around line 34-35: The code uses the ES2023 method calls current.toSorted()
and original.toSorted(), which have limited browser support; replace these with
non-mutating, compatible equivalents by creating shallow copies and sorting them
(e.g., use [...current].sort(...) and [...original].sort(...), preserving any
comparator used) so variables currentSorted and originalSorted keep the same
semantics without relying on toSorted().
In
@.agents/skills/vercel-react-best-practices/rules/js-request-idle-callback.md:
- Line 86: The module-level assignment to scheduleIdleWork accesses window
directly and will throw during SSR; update the assignment to guard against
server rendering by checking typeof window !== 'undefined' before using
window.requestIdleCallback (e.g., pick window.requestIdleCallback only when
window exists, otherwise use the setTimeout fallback), or move the pattern into
a client-only location such as a useEffect or a file/component marked 'use
client' so no module-scope window access occurs; reference the scheduleIdleWork
constant when making the change.
In @.agents/skills/vercel-react-best-practices/rules/js-set-map-lookups.md:
- Around line 15-23: Replace the invalid placeholder array syntax used for
allowedIds in both examples with concrete sample values or a clear comment
placeholder; specifically update the const allowedIds in the array example (used
by items.filter and includes) and the Set example (used by items.filter and has)
so they no longer use "['a', 'b', 'c', ...]" but instead contain real example
strings (e.g., ['a','b','c']) or a comment like ['a', 'b', 'c' /* more ids */]
to prevent copy-paste errors while keeping the demonstrated usage of allowedIds,
items.filter, includes, Set, and has intact.
In @.agents/skills/vercel-react-best-practices/rules/rendering-activity.md:
- Around line 15-21: Update the rule documentation to state the React version
requirement by adding a note that this rule (which references the Activity API
used in the Dropdown component and the Activity element wrapping ExpensiveMenu)
requires React 19.2 or later; edit the rendering-activity.md content to include
a short "Version requirement" or "Compatibility" line indicating "Requires React
19.2+" near the top or alongside the example so readers know Scope.
In
@.agents/skills/vercel-react-best-practices/rules/rendering-animate-svg-wrapper.md:
- Line 10: Replace the outdated blanket recommendation in
rendering-animate-svg-wrapper.md (the line "Wrap SVG in a `<div>` and animate
the wrapper instead.") with a modern, qualified statement: state that current
browsers (Chrome/Firefox/Safari) generally support GPU-accelerated CSS
transforms and opacity on SVG elements so prefer animating transform/opacity
directly on the SVG; if you keep the wrapper recommendation, limit it to
specific cases (e.g., heavy filter effects, complex path morphing with
<animate>, or known browser bugs) and add a short rationale and example for
those edge cases.
In
@.agents/skills/vercel-react-best-practices/rules/rendering-content-visibility.md:
- Line 38: The sentence "For 1000 messages, browser skips layout/paint for ~990
off-screen items (10× faster initial render)" is too specific; change it to
directional guidance and remove hardcoded multipliers and exact counts. Update
that line to something like "For large lists, browsers can skip layout/paint for
many off-screen items, which can significantly improve initial render
performance" and add a brief caveat noting results vary by device and browser so
numbers are not guaranteed.
In @.agents/skills/vercel-react-best-practices/rules/rendering-hoist-jsx.md:
- Around line 14-42: The examples reference an undefined loading variable;
update both Container definitions to accept a loading prop (e.g., change
function Container() to function Container({ loading }: { loading: boolean }))
so the conditional uses a passed-in boolean, and keep the LoadingSkeleton and
loadingSkeleton examples otherwise unchanged; update both occurrences of the
Container function accordingly.
In
@.agents/skills/vercel-react-best-practices/rules/rendering-hydration-no-flicker.md:
- Around line 62-74: The inline script using dangerouslySetInnerHTML that sets
the 'theme-wrapper' class from localStorage is not CSP-safe; update the
documentation near this script example to add a short note advising CSP-aware
alternatives: state that apps enforcing CSP should avoid inline scripts or
include a nonce/hash, and recommend using a nonce/hash-compatible approach or
framework-supported pre-hydration script APIs (or server-rendered classes)
instead of raw dangerouslySetInnerHTML. Mention the specific snippet (the
immediately-invoked function that reads localStorage and sets
document.getElementById('theme-wrapper').className) so readers know which code
needs the CSP guidance.
In
@.agents/skills/vercel-react-best-practices/rules/rendering-hydration-suppress-warning.md:
- Line 10: Update the guidance for using suppressHydrationWarning to explicitly
state its one-level-deep scope: note that suppressHydrationWarning only
suppresses hydration warnings for the direct element’s attributes and text
content and does not suppress mismatches in child elements or deeper
descendants; update the documentation around the suppressHydrationWarning
recommendation (the prop name "suppressHydrationWarning") to include this
limitation and give the example that placing suppressHydrationWarning on a
parent will not prevent warnings triggered by mismatches inside child elements.
In
@.agents/skills/vercel-react-best-practices/rules/rendering-resource-hints.md:
- Around line 12-20: The doc is missing a React version and framework note:
update the resource hint APIs section to state that prefetchDNS, preconnect,
preload, preloadModule, preinit, and preinitModule require React 19+ for stable
support (they were experimental before React 19) and add a brief caveat that
frameworks like Next.js often manage resource loading internally so direct calls
to these APIs may be unnecessary or redundant in framework contexts; explicitly
mention React 19+ and include the framework caveat near the top of the list so
readers on older React versions or using Next.js know not to copy unsupported
code.
In
@.agents/skills/vercel-react-best-practices/rules/rendering-usetransition-loading.md:
- Line 10: The rule text incorrectly advises replacing explicit async loading
state with useTransition and claims it "automatically manages transitions" and
implicit error/reset behavior; update the markdown in
rendering-usetransition-loading.md to remove that recommendation, explicitly
state that useTransition only gives an isPending render-priority flag and does
not cancel network requests or manage request lifecycle, mention that aborting
requires AbortController and that for fetch/error/loading management libraries
like React Query or explicit useState/error handling remain recommended, and
apply this correction to the other referenced sections (lines ~51-55 and ~70-74)
so all claims about automatic cancellation/error/reset are removed or corrected.
In @.agents/skills/vercel-react-best-practices/rules/rerender-dependencies.md:
- Around line 39-44: The effect only enables mobile mode on isMobile becoming
true and never disables it; update the useEffect that reads isMobile (derived
from width) to handle both transitions by calling enableMobileMode() when
isMobile is true and calling a corresponding disableMobileMode() (or revert
function) when isMobile is false, keeping the dependency array on [isMobile] so
the effect runs whenever the derived boolean changes; reference the isMobile
constant, the useEffect block, enableMobileMode(), and add or call
disableMobileMode() (or an appropriate teardown) to ensure two-way state
handling.
In @.agents/skills/vercel-react-best-practices/rules/rerender-transitions.md:
- Around line 14-40: Add usage of the tracked state to both examples so the
benefit of non-blocking updates is visible: in the ScrollTracker component
(which defines scrollY and setScrollY) return some UI that consumes scrollY,
e.g. render an ExpensiveVisualization component with a prop like
scrollPosition={scrollY}; do this for both the blocking example (handler calls
setScrollY) and the non-blocking example (handler wraps setScrollY in
startTransition) so readers can see how startTransition prevents jank when
updating scroll-driven expensive renders.
In
@.agents/skills/vercel-react-best-practices/rules/rerender-use-deferred-value.md:
- Around line 30-49: The example is missing React imports used in the Search
component; add an import for useDeferredValue, useMemo and useState from 'react'
at the top of the file so the hooks used in Search (useDeferredValue, useMemo,
useState) resolve correctly when rendering the Search function and computing
filtered/isStale.
In
@.agents/skills/vercel-react-best-practices/rules/server-after-nonblocking.md:
- Around line 44-49: The after() callback is calling logUserAction(...) without
awaiting it; update the async after() body to await logUserAction(...) (e.g.,
await logUserAction({ sessionCookie, userAgent })) so the promise is properly
handled, and optionally wrap that call in a try/catch to surface/log errors from
logUserAction (while still leveraging Next.js waitUntil behavior from headers()
and cookies()).
In @.agents/skills/vercel-react-best-practices/rules/server-auth-actions.md:
- Around line 74-81: The authorization block uses a raw throw new Error('Can
only update own profile') while the earlier example uses the dedicated
unauthorized helper; update this code so both checks use the same helper
(instead of throw new Error) — replace the second throw in the session/user id
check (the code referencing session and validated.userId) with the same
unauthorized helper call used in the prior example (passing a concise message or
no message to match the other usage) so the examples are consistent.
In @.agents/skills/vercel-react-best-practices/rules/server-cache-lru.md:
- Around line 23-25: The cache hit check uses truthiness and therefore treats
cached falsy values (like null from findUnique) as misses; replace the truthy
check with an existence check by verifying the cache entry exists (e.g., use
cache.has(id) or check cached !== undefined) and return the cached value when
present so falsy cached values are honored; update the logic around
cache.get(id) and the variable cached accordingly.
In
@.agents/skills/vercel-react-best-practices/rules/server-parallel-fetching.md:
- Line 67: The TSX example uses ReactNode in the Layout component signature but
doesn't import it; update the example to import ReactNode from React at the top
of the code block so the type annotation in function Layout({ children }: {
children: ReactNode }) compiles (add "import { ReactNode } from 'react'").
Ensure the import appears alongside any existing React imports in that snippet.
In @.agents/skills/vercel-react-best-practices/rules/server-serialization.md:
- Line 10: Update the rule text that currently claims "The React Server/Client
boundary serializes all object properties into strings..." to accurately
describe the RSC mechanism: replace that sentence with language stating that
props crossing the server→client boundary are serialized via the RSC Payload
format (a structured binary/text format with tagged rows) which is embedded in
the initial HTML and fetched via RSC requests, and then add the recommended
guidance "Only pass fields that the client actually uses." Reference the
existing phrase "React Server/Client boundary" and the term "RSC Payload" when
editing so the change is applied in the same sentence/block.
In `@apps/web/src/components/auth/signin-form.tsx`:
- Line 19: Replace the relative import of the AuthFormDivider with the
app-relative alias form per project convention: locate the import statement that
brings in AuthFormDivider (the import referencing "./auth-form-divider") and
change it to use the "@/..." alias that maps to src (e.g., import
AuthFormDivider from '@/components/auth/auth-form-divider' or the correct path
under src) so the component import follows the app-relative alias rule.
In `@apps/web/src/components/auth/signup-form.tsx`:
- Line 19: The import for AuthFormDivider uses a relative path; change it to the
app-relative alias import using the `@/` mapping (replace the current relative
import of AuthFormDivider with an alias import like `@/...` that resolves to the
same component under src), ensuring the import statement referencing
AuthFormDivider (component name) follows the project's `@/*` -> `src/*`
convention.
In `@apps/web/src/components/brands/brand-avatar.tsx`:
- Around line 32-45: The current BrandAvatar component always chooses the <img>
branch when logoUrl is truthy, causing broken images to display; add an image
error state (e.g., hasImageError via useState) and an onError handler on the
<img> that sets that state so the component treats image load failures the same
as a missing logoUrl and falls back to rendering initials. Ensure you also reset
the error state when the logoUrl prop changes (useEffect) so new URLs are
retried, and keep existing identifiers (BrandAvatar, logoUrl, sizeClasses, dim,
className) when updating the render conditional to check !logoUrl ||
hasImageError before showing the <img>.
- Around line 6-22: Replace the handcrafted sizeClasses with a cva-based
variant: create a cva (e.g., avatarClasses or brandAvatarClasses) that includes
the base classes and a variants block for size mapping the same strings (xs, sm,
md, lg) to their respective class strings, and set defaultVariants.size to "md";
then update the component to call that cva with avatarClasses({ size, className
}) instead of using sizeClasses, while keeping imgDimensions as-is for image
pixel values.
In `@apps/web/src/components/brands/create/brand-create-page.tsx`:
- Around line 17-32: The resolveLogoUrl function lacks error handling around the
fetch(dataUrl) call; wrap the fetch(...).then(r => r.blob()) logic in a
try/catch inside resolveLogoUrl, catch any thrown errors, log or surface the
error (e.g., console.error or a passed logger) with context (include filename
and dataUrl) and then return the fallback (or rethrow with augmented message if
you prefer the caller handle it); ensure you still call uploadFn(formData) only
when the fetch/blob succeeded and reference resolveLogoUrl, fetch, uploadFn and
handleSave in your changes.
In `@apps/web/src/components/brands/create/types.ts`:
- Line 1: The import for the ToneOfVoice type currently uses a relative path
("./tone-of-voice"); update that import to use the project alias (@"/*" mapping
to src/*) so the types file imports ToneOfVoice via the app-relative alias
instead of the relative "./tone-of-voice" path (locate the import of ToneOfVoice
in types.ts and replace the relative import with the corresponding "@/..." alias
import).
In `@apps/web/src/components/contacts/contact-details.tsx`:
- Around line 24-27: The status class mappings in ContactDetails duplicate the
styles already defined in StatusBadge; remove the hard-coded mapping and instead
import and render the shared StatusBadge (or call its shared style helper if
available) from StatusBadge inside ContactDetails so each status (subscribed,
unsubscribed, bounced, complained) uses the centralized styling; update the
render where those classes are applied to pass the status to StatusBadge (or use
getStatusClasses/statusClassForStatus helper from StatusBadge) and delete the
duplicated class strings.
In `@apps/web/src/components/contacts/status-badge.tsx`:
- Around line 4-9: Replace the manual STATUS_STYLES mapping with a
class-variance-authority (cva) variant definition and consume that variant
inside the StatusBadge component: import cva, define a statusBadge =
cva(baseClass, { variants: { variant: { subscribed: "...", unsubscribed: "...",
bounced: "...", complained: "..." } }, defaultVariants: { variant: "subscribed"
} }), then update the component to call statusBadge({ variant: status }) instead
of STATUS_STYLES[status]; ensure ContactStatus is used as the variant key type
and remove the STATUS_STYLES constant.
In `@apps/web/src/components/layout/sidebar-brand-list.tsx`:
- Around line 19-25: The current active-brand check is using substring-matching
against location.search (from useRouterState), which can misidentify brands when
one id is a substring of another or when values are encoded; instead parse the
brandId query param using URLSearchParams(location.search).get('brandId') and
compare that exact value to the brand id (e.g. String(brand.id) ===
activeBrandId) when rendering the brands from brands (and keep using
setOpenMobile in handleNavClick unchanged); ensure you handle null/undefined
activeBrandId gracefully so no brand is marked active if the param is absent.
In `@apps/web/src/components/layout/sidebar-chat-list.tsx`:
- Around line 104-106: The optional chaining on chats is redundant because the
ternary guard already ensures chats.length > 0; locate the rendering block that
uses chats?.map inside the fragment (the map producing <SidebarMenuItem
key={chat.id}> items) and change chats?.map to chats.map so the map call is
direct (keep the existing chat => (...) callback and SidebarMenuItem usage
unchanged).
In `@apps/web/src/components/layout/sidebar-user-menu.tsx`:
- Around line 33-36: The AvatarImage components are receiving an empty string
for src when the user has no image, causing browsers to request the current
document URL; update the places that render AvatarImage (where you access user
image via authClient.useSession() / data?.user?.image) to pass undefined instead
of "" when the image is falsy (e.g., use a conditional that passes
data?.user?.image ?? undefined or data?.user?.image ? data.user.image :
undefined) and apply the same change to the other AvatarImage usages referenced
in the diff so the fallback renders without a broken-image request.
In `@apps/web/src/components/preview/preview-header.tsx`:
- Around line 55-58: The callback handleCopy currently calls onCopyHtml() and
unconditionally shows toast.success; change it to await or handle the returned
promise from onCopyHtml (since it may be async) and only call
toast.success("Code copied to clipboard!") after onCopyHtml resolves
successfully, wrapping the call in try/catch (or .then/.catch) to show an error
toast on rejection or thrown errors; update the useCallback accordingly to
handle async flow and keep the dependency on onCopyHtml.
- Around line 135-195: The icon-only mobile buttons lack accessible names; add
aria-label attributes (or visually hidden text) to each interactive control: the
Preview and Code buttons that call setViewMode("preview") / setViewMode("code"),
the Copy button that calls handleCopy, the Export DropdownTrigger button and its
DropdownMenuItem actions onExportHtml and onExport, and the Send Test button
that calls onSendTest so screen readers can identify them (use concise labels
like "Preview", "Code", "Copy", "Export", "Send Test").
In `@apps/web/src/components/preview/preview-loader.tsx`:
- Around line 157-161: The current motion.rect elements use a non-unique key
string built from line.tx and line.ty (key={`${line.tx}-${line.ty}`}) which
collides for two entries in codeLines; change the key to be unique per item —
for example include the loop index or a unique identifier (e.g.,
key={`${line.tx}-${line.ty}-${i}`}` or use a stable line.id) when mapping over
codeLines in the component that calls fadeProps and renders motion.rect so React
can correctly reconcile the animated rectangles.
In `@apps/web/src/components/ui/text-loop.tsx`:
- Around line 36-48: The useEffect in TextLoop (text-loop.tsx) currently creates
a timer that computes (current + 1) % items.length which yields NaN when
items.length is 0; fix by guarding against an empty items/children array before
starting the interval (e.g., only set up the interval when items.length > 0 and
trigger is true), and inside the state updater for setCurrentIndex avoid the
modulo when items.length === 0 (return 0 or keep current), only call
onIndexChange when the next index is valid, and ensure the cleanup still clears
the timer; update the useEffect conditions and the setCurrentIndex callback in
the TextLoop component accordingly.
In `@apps/web/src/hooks/email-compiler.ts`:
- Around line 36-37: The membership test for MODULE_CACHE currently uses the in
operator which inspects the prototype chain; change the check in the
email-compiler hook to use an own-property test (e.g.
Object.prototype.hasOwnProperty.call(MODULE_CACHE, moduleName) or
Object.hasOwn(MODULE_CACHE, moduleName)) so only direct keys on MODULE_CACHE
(referenced as MODULE_CACHE and moduleName in the code) are considered when
returning MODULE_CACHE[moduleName].
In `@apps/web/src/lib/utils.ts`:
- Around line 8-14: The getInitials function can produce empty fragments with
irregular whitespace; update getInitials to trim the input, split on a
whitespace regex (e.g., /\s+/), optionally filter out empty strings, then map to
the first character uppercased and join/slice to two characters so initials are
stable for names with extra spaces; locate and modify the getInitials function
to implement these changes.
In `@apps/web/src/router.tsx`:
- Line 2: Change the app-relative import of the Loader component in router.tsx
to use the repository path alias: replace the current import of Loader (symbol:
Loader) from "./components/loader" with "@/components/loader" so it follows the
app alias mapping to src/* and remains consistent with project import rules.
---
Outside diff comments:
In `@apps/web/src/components/contacts/contact-import-dialog.tsx`:
- Around line 187-194: The dropzone's className currently concatenates classes
with a template string; replace that with the project's cn() utility to safely
merge conditional classes—locate the className on the element inside
ContactImportDialog (references: isDragging, selectedFile, isLoading) and change
the template-string expression to a cn(...) call that passes the static classes
and conditional objects/strings (e.g., { "scale-[1.02] border-primary
bg-primary/5": isDragging, "border-success/50 bg-success/5": selectedFile,
"pointer-events-none opacity-60": isLoading }) so tailwind-merge and clsx handle
merging consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bfbe7fe9-9314-443e-ab9a-3f3a814e47e7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (141)
.agents/skills/vercel-react-best-practices/AGENTS.md.agents/skills/vercel-react-best-practices/README.md.agents/skills/vercel-react-best-practices/SKILL.md.agents/skills/vercel-react-best-practices/metadata.json.agents/skills/vercel-react-best-practices/rules/_sections.md.agents/skills/vercel-react-best-practices/rules/_template.md.agents/skills/vercel-react-best-practices/rules/advanced-effect-event-deps.md.agents/skills/vercel-react-best-practices/rules/advanced-event-handler-refs.md.agents/skills/vercel-react-best-practices/rules/advanced-init-once.md.agents/skills/vercel-react-best-practices/rules/advanced-use-latest.md.agents/skills/vercel-react-best-practices/rules/async-api-routes.md.agents/skills/vercel-react-best-practices/rules/async-cheap-condition-before-await.md.agents/skills/vercel-react-best-practices/rules/async-defer-await.md.agents/skills/vercel-react-best-practices/rules/async-dependencies.md.agents/skills/vercel-react-best-practices/rules/async-parallel.md.agents/skills/vercel-react-best-practices/rules/async-suspense-boundaries.md.agents/skills/vercel-react-best-practices/rules/bundle-barrel-imports.md.agents/skills/vercel-react-best-practices/rules/bundle-conditional.md.agents/skills/vercel-react-best-practices/rules/bundle-defer-third-party.md.agents/skills/vercel-react-best-practices/rules/bundle-dynamic-imports.md.agents/skills/vercel-react-best-practices/rules/bundle-preload.md.agents/skills/vercel-react-best-practices/rules/client-event-listeners.md.agents/skills/vercel-react-best-practices/rules/client-localstorage-schema.md.agents/skills/vercel-react-best-practices/rules/client-passive-event-listeners.md.agents/skills/vercel-react-best-practices/rules/client-swr-dedup.md.agents/skills/vercel-react-best-practices/rules/js-batch-dom-css.md.agents/skills/vercel-react-best-practices/rules/js-cache-function-results.md.agents/skills/vercel-react-best-practices/rules/js-cache-property-access.md.agents/skills/vercel-react-best-practices/rules/js-cache-storage.md.agents/skills/vercel-react-best-practices/rules/js-combine-iterations.md.agents/skills/vercel-react-best-practices/rules/js-early-exit.md.agents/skills/vercel-react-best-practices/rules/js-flatmap-filter.md.agents/skills/vercel-react-best-practices/rules/js-hoist-regexp.md.agents/skills/vercel-react-best-practices/rules/js-index-maps.md.agents/skills/vercel-react-best-practices/rules/js-length-check-first.md.agents/skills/vercel-react-best-practices/rules/js-min-max-loop.md.agents/skills/vercel-react-best-practices/rules/js-request-idle-callback.md.agents/skills/vercel-react-best-practices/rules/js-set-map-lookups.md.agents/skills/vercel-react-best-practices/rules/js-tosorted-immutable.md.agents/skills/vercel-react-best-practices/rules/rendering-activity.md.agents/skills/vercel-react-best-practices/rules/rendering-animate-svg-wrapper.md.agents/skills/vercel-react-best-practices/rules/rendering-conditional-render.md.agents/skills/vercel-react-best-practices/rules/rendering-content-visibility.md.agents/skills/vercel-react-best-practices/rules/rendering-hoist-jsx.md.agents/skills/vercel-react-best-practices/rules/rendering-hydration-no-flicker.md.agents/skills/vercel-react-best-practices/rules/rendering-hydration-suppress-warning.md.agents/skills/vercel-react-best-practices/rules/rendering-resource-hints.md.agents/skills/vercel-react-best-practices/rules/rendering-script-defer-async.md.agents/skills/vercel-react-best-practices/rules/rendering-svg-precision.md.agents/skills/vercel-react-best-practices/rules/rendering-usetransition-loading.md.agents/skills/vercel-react-best-practices/rules/rerender-defer-reads.md.agents/skills/vercel-react-best-practices/rules/rerender-dependencies.md.agents/skills/vercel-react-best-practices/rules/rerender-derived-state-no-effect.md.agents/skills/vercel-react-best-practices/rules/rerender-derived-state.md.agents/skills/vercel-react-best-practices/rules/rerender-functional-setstate.md.agents/skills/vercel-react-best-practices/rules/rerender-lazy-state-init.md.agents/skills/vercel-react-best-practices/rules/rerender-memo-with-default-value.md.agents/skills/vercel-react-best-practices/rules/rerender-memo.md.agents/skills/vercel-react-best-practices/rules/rerender-move-effect-to-event.md.agents/skills/vercel-react-best-practices/rules/rerender-no-inline-components.md.agents/skills/vercel-react-best-practices/rules/rerender-simple-expression-in-memo.md.agents/skills/vercel-react-best-practices/rules/rerender-split-combined-hooks.md.agents/skills/vercel-react-best-practices/rules/rerender-transitions.md.agents/skills/vercel-react-best-practices/rules/rerender-use-deferred-value.md.agents/skills/vercel-react-best-practices/rules/rerender-use-ref-transient-values.md.agents/skills/vercel-react-best-practices/rules/server-after-nonblocking.md.agents/skills/vercel-react-best-practices/rules/server-auth-actions.md.agents/skills/vercel-react-best-practices/rules/server-cache-lru.md.agents/skills/vercel-react-best-practices/rules/server-cache-react.md.agents/skills/vercel-react-best-practices/rules/server-dedup-props.md.agents/skills/vercel-react-best-practices/rules/server-hoist-static-io.md.agents/skills/vercel-react-best-practices/rules/server-no-shared-module-state.md.agents/skills/vercel-react-best-practices/rules/server-parallel-fetching.md.agents/skills/vercel-react-best-practices/rules/server-parallel-nested-fetching.md.agents/skills/vercel-react-best-practices/rules/server-serialization.mdapps/web/src/components/auth/auth-form-divider.tsxapps/web/src/components/auth/forgot-password-form.tsxapps/web/src/components/auth/reset-password-form.tsxapps/web/src/components/auth/signin-form.tsxapps/web/src/components/auth/signup-form.tsxapps/web/src/components/brands/brand-avatar.tsxapps/web/src/components/brands/brand-card.tsxapps/web/src/components/brands/brand-selector.tsxapps/web/src/components/brands/brands-delete-dialog.tsxapps/web/src/components/brands/brands-dialog.tsxapps/web/src/components/brands/brands-view-dialog.tsxapps/web/src/components/brands/create/brand-create-page.tsxapps/web/src/components/brands/create/email-preview.tsxapps/web/src/components/brands/create/section-sidebar.tsxapps/web/src/components/brands/create/sections/brand-details-section.tsxapps/web/src/components/brands/create/sections/colors-section.tsxapps/web/src/components/brands/create/sections/legal-section.tsxapps/web/src/components/brands/create/sections/logos-section.tsxapps/web/src/components/brands/create/sections/socials-section.tsxapps/web/src/components/brands/create/types.tsapps/web/src/components/chat/chat-list.tsxapps/web/src/components/chat/chat-message.tsxapps/web/src/components/chat/chat-options.tsxapps/web/src/components/chat/reasoning-with-tools.tsxapps/web/src/components/chat/thinking-section.tsxapps/web/src/components/chat/tool-call-section.tsxapps/web/src/components/common/confirm-delete-dialog.tsxapps/web/src/components/common/full-page-message.tsxapps/web/src/components/common/prompt-input.tsxapps/web/src/components/contact-lists/list-delete-dialog.tsxapps/web/src/components/contacts/contact-delete-dialog.tsxapps/web/src/components/contacts/contact-details.tsxapps/web/src/components/contacts/contact-import-dialog.tsxapps/web/src/components/contacts/contacts-table.tsxapps/web/src/components/contacts/status-badge.tsxapps/web/src/components/dashboard/dashboard-chat-panel.tsxapps/web/src/components/dashboard/dashboard-preview-panel.tsxapps/web/src/components/error.tsxapps/web/src/components/landing/hero.tsxapps/web/src/components/layout/app-sidebar.tsxapps/web/src/components/layout/sidebar-brand-list.tsxapps/web/src/components/layout/sidebar-chat-list.tsxapps/web/src/components/layout/sidebar-nav.tsxapps/web/src/components/layout/sidebar-user-menu.tsxapps/web/src/components/loader.tsxapps/web/src/components/not-found.tsxapps/web/src/components/preview/email-preview.tsxapps/web/src/components/preview/monaco-editor.tsxapps/web/src/components/preview/preview-header.tsxapps/web/src/components/preview/preview-loader.tsxapps/web/src/components/skeletons/chat-page-skeleton.tsxapps/web/src/components/ui/section-header.tsxapps/web/src/components/ui/text-loop.tsxapps/web/src/contexts/chat-context.tsxapps/web/src/functions/brand.tsapps/web/src/hooks/email-compiler.tsapps/web/src/hooks/use-code-animation.tsapps/web/src/index.cssapps/web/src/lib/utils.tsapps/web/src/router.tsxapps/web/src/routes/__root.tsxapps/web/src/routes/api/chat.tsapps/web/src/routes/index.tsxapps/web/src/types/chat.tspackage.jsonskills-lock.json
💤 Files with no reviewable changes (5)
- apps/web/src/components/chat/thinking-section.tsx
- apps/web/src/hooks/use-code-animation.ts
- apps/web/src/components/brands/brands-view-dialog.tsx
- apps/web/src/components/chat/tool-call-section.tsx
- apps/web/src/components/brands/brands-dialog.tsx
| 6. Rules are automatically sorted by title - no need to manage numbers! | ||
|
|
||
| ## Acknowledgments | ||
|
|
Contributor
There was a problem hiding this comment.
Tighten wording in acknowledgment sentence.
“Originally created” is redundant here; “created” reads cleaner.
🧰 Tools
🪛 LanguageTool
[style] ~122-~122: This phrase is redundant. Consider writing “created”.
Context: ...to manage numbers! ## Acknowledgments Originally created by @shuding at...
(ORIGINALLY_CREATED)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/vercel-react-best-practices/README.md at line 122, The
acknowledgment sentence uses the redundant phrase "Originally created"; update
that sentence in the README acknowledgement line to replace "Originally created"
with "created" so the phrasing reads more concise (e.g., change "Originally
created by..." to "created by...") — locate the acknowledgment sentence in
.agents/skills/vercel-react-best-practices/README.md and make this single-word
replacement.
Comment on lines
+40
to
+53
| **Alternative: use `useEffectEvent` if you're on latest React:** | ||
|
|
||
| ```tsx | ||
| import { useEffectEvent } from 'react' | ||
|
|
||
| function useWindowEvent(event: string, handler: (e) => void) { | ||
| const onEvent = useEffectEvent(handler) | ||
|
|
||
| useEffect(() => { | ||
| window.addEventListener(event, onEvent) | ||
| return () => window.removeEventListener(event, onEvent) | ||
| }, [event]) | ||
| } | ||
| ``` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the current availability and status of React's useEffectEvent hook? Is it available in stable React 18 or only in experimental/Canary builds?
💡 Result:
The useEffectEvent hook is currently available and stable in React 19.2 and later versions (released October 1, 2025, with subsequent patches like 19.2.4 on January 26, 2026). It is documented in the official React documentation at react.dev/reference/react/useEffectEvent and was introduced as a new stable feature in the React 19.2 release notes. It is not available in stable React 18 (latest 18.3.1), which predates this feature and lacks any mention of useEffectEvent in its changelog. Earlier mentions described it as experimental in pre-19.2 builds.
Citations:
- 1: https://react.dev/reference/react/useEffectEvent
- 2: https://github.com/facebook/react/releases/tag/v19.2.0
- 3: https://github.com/facebook/react/blob/main/CHANGELOG.md
- 4: https://react.dev/blog/2025/10/01/react-19-2
Specify React version requirement for useEffectEvent.
useEffectEvent is stable as of React 19.2+ but is not available in React 18. The phrase "if you're on latest React" should be clarified to specify React 19.2 or later. Alternatively, recommend the ref pattern shown in lines 25–38, which is compatible with all React versions including React 18.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
@.agents/skills/vercel-react-best-practices/rules/advanced-event-handler-refs.md
around lines 40 - 53, Clarify the React version requirement for useEffectEvent
in the alternative: update the text around the useWindowEvent example to state
that useEffectEvent is only available and stable in React 19.2+ (not in React
18), and advise readers to use the ref-based pattern (as shown in the
useWindowEvent ref approach / handler ref pattern in lines 25–38) for
compatibility with React 18 and earlier; mention the specific symbols
useEffectEvent and useWindowEvent so reviewers can find and update the example
and wording.
|
|
||
| ## Initialize App Once, Not Per Mount | ||
|
|
||
| Do not put app-wide initialization that must run once per app load inside `useEffect([])` of a component. Components can remount and effects will re-run. Use a module-level guard or top-level init in the entry module instead. |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to current React and Next.js guidance, is a module-level mutable guard (e.g., let didInit = false) safe as a general pattern for one-time initialization when SSR/server rendering is involved?
💡 Result:
No, a module-level mutable guard like let didInit = false is not safe as a general pattern for one-time initialization when SSR/server rendering is involved in React and Next.js. Module-level variables in JavaScript are shared across all requests in a Node.js server environment due to module caching. In SSR scenarios, multiple user requests share the same process and thus the same module instance. If one request sets didInit = true, subsequent requests see the already-initialized state, leading to skipped initialization for other users. This causes data leakage, inconsistent rendering, or incorrect behavior across requests. This is explicitly warned against in state management libraries like Zustand used with Next.js Server Components: "any global singleton, basically any mutable object declared at the module-level, will persist on the server but not on the client" and "Server-side components are meant to be stateless". Similar issues apply to Jotai and other global/module state solutions, which recommend per-request isolation via Providers or hydration. Official React docs emphasize stateless server rendering with renderToString etc., and Server Components are designed to be stateless, passing data as props to Client Components. Next.js reinforces this by recommending request-scoped QueryClients and avoiding shared state. Safe alternatives for one-time initialization: - Use React hooks like useEffect with browser-only checks (typeof window !== 'undefined') in Client Components ('use client'). - For server-side, perform initialization per-request in Server Components or loaders (e.g., getServerSideProps, async Server Components). - Use per-request contexts or providers for state isolation. - For global-like init (e.g., DB connections), use connection pools or singletons with proper request scoping, not mutable flags.
Citations:
- 1: Using Zustand in React Server Components - misguided misinformation and misuse? pmndrs/zustand#2200
- 2: https://beta.reactjs.org/reference/rsc/server-components
- 3: https://tanstack.com/query/latest/docs/framework/react/guides/advanced-ssr
- 4: https://tanstack.com/query/v4/docs/framework/react/guides/ssr
- 5: https://react.dev/reference/react-dom/server/renderToString
🏁 Script executed:
cat -n ".agents/skills/vercel-react-best-practices/rules/advanced-init-once.md" | head -60Repository: Blackmamoth/scribe
Length of output: 1315
The "Correct" example is unsafe as general guidance in SSR/server environments.
The file shows a module-level mutable didInit flag as the recommended pattern. In Node.js server environments (relevant for Vercel/Next.js), module-level variables are shared across all requests. If one request sets didInit = true, subsequent requests see the already-initialized state, causing skipped initialization for other users. This pattern is explicitly warned against by React, Next.js, Zustand, and TanStack Query documentation for SSR scenarios.
The guidance should be scoped to client-only initialization or explicitly call out that this pattern only works in single-process, browser-based environments. For server-side one-time initialization, use per-request setup in Server Components or loaders instead.
✏️ Suggested doc correction
-Use a module-level guard or top-level init in the entry module instead.
+For client-only initialization, use a module-level guard. For server environments (SSR/Next.js), use per-request initialization in Server Components or loaders instead.-**Correct (once per app load):**
+**Client-only (once per browser app load):**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/vercel-react-best-practices/rules/advanced-init-once.md at
line 10, The "Correct" example currently recommends a module-level mutable guard
(didInit) and useEffect([]) for one-time init; update the guidance to explicitly
scope that pattern to client-only/browser environments and warn it is unsafe in
Node/SSR where module-level state is shared across requests; add alternative
recommendations: for server-side init use per-request setup in Server
Components, loaders, or middleware, and for app-level browser init prefer
top-level entry-module initialization guarded by a client-only check instead of
relying on component useEffect([]) or shared module state like didInit.
Comment on lines
+20
to
+27
| **Correct (parallel execution, 1 round trip):** | ||
|
|
||
| ```typescript | ||
| const [user, posts, comments] = await Promise.all([ | ||
| fetchUser(), | ||
| fetchPosts(), | ||
| fetchComments() | ||
| ]) |
Contributor
There was a problem hiding this comment.
Correct the networking claim in the explanation.
Promise.all overlaps wait time, but it does not reduce three independent requests to “1 round trip.” Rephrase to avoid technical inaccuracy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/vercel-react-best-practices/rules/async-parallel.md around
lines 20 - 27, Update the explanatory text that claims Promise.all reduces three
independent requests to "1 round trip": replace the inaccurate wording with a
precise statement that Promise.all runs fetchUser, fetchPosts, and fetchComments
concurrently so their waiting times overlap, making total latency roughly the
maximum of the individual request latencies rather than the sum, rather than
implying it collapses multiple network calls into a single round trip.
Comment on lines
+22
to
+27
| ```typescript | ||
| const [user, posts, comments] = await Promise.all([ | ||
| fetchUser(), | ||
| fetchPosts(), | ||
| fetchComments() | ||
| ]) |
Contributor
There was a problem hiding this comment.
Use tabs in the TypeScript snippet indentation.
The example block is space-indented.
As per coding guidelines, **/*.{ts,tsx,js,jsx,json,md}: Use tab indentation (configured in biome.json).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/vercel-react-best-practices/rules/async-parallel.md around
lines 22 - 27, The TypeScript example block using Promise.all is space-indented;
update the code fence so its interior uses tabs for indentation (preserve the
same lines and semantics for the array destructuring and the calls to
fetchUser(), fetchPosts(), fetchComments()) to comply with the tab-indentation
rule configured in biome.json.
Comment on lines
+17
to
+32
| async function resolveLogoUrl( | ||
| dataUrl: string | null | undefined, | ||
| fallback: string | null, | ||
| filename: string, | ||
| uploadFn: (formData: FormData) => Promise<string>, | ||
| ): Promise<string | null> { | ||
| if (dataUrl?.startsWith("data:")) { | ||
| const blob = await fetch(dataUrl).then((r) => r.blob()); | ||
| const file = new File([blob], filename, { type: blob.type }); | ||
| const formData = new FormData(); | ||
| formData.append("logo", file); | ||
| return uploadFn(formData); | ||
| } | ||
| if (dataUrl) return dataUrl; | ||
| return fallback; | ||
| } |
Contributor
There was a problem hiding this comment.
Consider adding error handling for the fetch call on data URLs.
The fetch(dataUrl).then((r) => r.blob()) pattern works for data URLs but lacks explicit error handling. If the data URL is malformed, this will throw and bubble up to the handleSave catch block, which swallows the error silently (line 204-206).
🛡️ Optional: Add explicit error handling
async function resolveLogoUrl(
dataUrl: string | null | undefined,
fallback: string | null,
filename: string,
uploadFn: (formData: FormData) => Promise<string>,
): Promise<string | null> {
if (dataUrl?.startsWith("data:")) {
- const blob = await fetch(dataUrl).then((r) => r.blob());
+ const response = await fetch(dataUrl);
+ if (!response.ok) {
+ throw new Error(`Failed to process logo: ${filename}`);
+ }
+ const blob = await response.blob();
const file = new File([blob], filename, { type: blob.type });
const formData = new FormData();
formData.append("logo", file);
return uploadFn(formData);
}
if (dataUrl) return dataUrl;
return fallback;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function resolveLogoUrl( | |
| dataUrl: string | null | undefined, | |
| fallback: string | null, | |
| filename: string, | |
| uploadFn: (formData: FormData) => Promise<string>, | |
| ): Promise<string | null> { | |
| if (dataUrl?.startsWith("data:")) { | |
| const blob = await fetch(dataUrl).then((r) => r.blob()); | |
| const file = new File([blob], filename, { type: blob.type }); | |
| const formData = new FormData(); | |
| formData.append("logo", file); | |
| return uploadFn(formData); | |
| } | |
| if (dataUrl) return dataUrl; | |
| return fallback; | |
| } | |
| async function resolveLogoUrl( | |
| dataUrl: string | null | undefined, | |
| fallback: string | null, | |
| filename: string, | |
| uploadFn: (formData: FormData) => Promise<string>, | |
| ): Promise<string | null> { | |
| if (dataUrl?.startsWith("data:")) { | |
| const response = await fetch(dataUrl); | |
| if (!response.ok) { | |
| throw new Error(`Failed to process logo: ${filename}`); | |
| } | |
| const blob = await response.blob(); | |
| const file = new File([blob], filename, { type: blob.type }); | |
| const formData = new FormData(); | |
| formData.append("logo", file); | |
| return uploadFn(formData); | |
| } | |
| if (dataUrl) return dataUrl; | |
| return fallback; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/brands/create/brand-create-page.tsx` around lines 17
- 32, The resolveLogoUrl function lacks error handling around the fetch(dataUrl)
call; wrap the fetch(...).then(r => r.blob()) logic in a try/catch inside
resolveLogoUrl, catch any thrown errors, log or surface the error (e.g.,
console.error or a passed logger) with context (include filename and dataUrl)
and then return the fallback (or rethrow with augmented message if you prefer
the caller handle it); ensure you still call uploadFn(formData) only when the
fetch/blob succeeded and reference resolveLogoUrl, fetch, uploadFn and
handleSave in your changes.
| @@ -1,3 +1,5 @@ | |||
| import type { ToneOfVoice } from "./tone-of-voice"; | |||
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use @/ alias for app-relative import.
Line 1 should use the project alias instead of a relative path to match repository import conventions.
Suggested change
-import type { ToneOfVoice } from "./tone-of-voice";
+import type { ToneOfVoice } from "@/components/brands/create/tone-of-voice";As per coding guidelines, "Use path alias @/* mapping to src/* for app-relative imports".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import type { ToneOfVoice } from "./tone-of-voice"; | |
| import type { ToneOfVoice } from "@/components/brands/create/tone-of-voice"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/brands/create/types.ts` at line 1, The import for the
ToneOfVoice type currently uses a relative path ("./tone-of-voice"); update that
import to use the project alias (@"/*" mapping to src/*) so the types file
imports ToneOfVoice via the app-relative alias instead of the relative
"./tone-of-voice" path (locate the import of ToneOfVoice in types.ts and replace
the relative import with the corresponding "@/..." alias import).
Comment on lines
+55
to
+58
| const handleCopy = useCallback(() => { | ||
| onCopyHtml(); | ||
| toast.success("Code copied to clipboard!"); | ||
| }; | ||
| }, [onCopyHtml]); |
Contributor
There was a problem hiding this comment.
Don’t show success toast before copy is confirmed.
toast.success("Code copied to clipboard!") runs unconditionally. If onCopyHtml throws/rejects, users still get a success message.
🛠️ Suggested fix
- const handleCopy = useCallback(() => {
- onCopyHtml();
- toast.success("Code copied to clipboard!");
- }, [onCopyHtml]);
+ const handleCopy = useCallback(async () => {
+ try {
+ await Promise.resolve(onCopyHtml());
+ toast.success("Code copied to clipboard!");
+ } catch {
+ toast.error("Failed to copy code.");
+ }
+ }, [onCopyHtml]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleCopy = useCallback(() => { | |
| onCopyHtml(); | |
| toast.success("Code copied to clipboard!"); | |
| }; | |
| }, [onCopyHtml]); | |
| const handleCopy = useCallback(async () => { | |
| try { | |
| await Promise.resolve(onCopyHtml()); | |
| toast.success("Code copied to clipboard!"); | |
| } catch { | |
| toast.error("Failed to copy code."); | |
| } | |
| }, [onCopyHtml]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/preview/preview-header.tsx` around lines 55 - 58, The
callback handleCopy currently calls onCopyHtml() and unconditionally shows
toast.success; change it to await or handle the returned promise from onCopyHtml
(since it may be async) and only call toast.success("Code copied to clipboard!")
after onCopyHtml resolves successfully, wrapping the call in try/catch (or
.then/.catch) to show an error toast on rejection or thrown errors; update the
useCallback accordingly to handle async flow and keep the dependency on
onCopyHtml.
Comment on lines
+135
to
+195
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| className={cn( | ||
| "h-7 px-2 text-xs", | ||
| viewMode === "preview" && "bg-background shadow-sm", | ||
| )} | ||
| onClick={() => setViewMode("preview")} | ||
| > | ||
| <Eye className="mr-1.5 h-3.5 w-3.5" /> | ||
| <span className="hidden md:inline">Preview</span> | ||
| </Button> | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| className={cn( | ||
| "h-7 px-2 text-xs", | ||
| viewMode === "code" && "bg-background shadow-sm", | ||
| )} | ||
| onClick={() => setViewMode("code")} | ||
| > | ||
| <Code2 className="mr-1.5 h-3.5 w-3.5" /> | ||
| <span className="hidden md:inline">Code</span> | ||
| </Button> | ||
| </div> | ||
|
|
||
| <div className="mx-1 h-4 w-px bg-border" /> | ||
|
|
||
| <Button | ||
| variant="ghost" | ||
| variant="outline" | ||
| size="sm" | ||
| className={cn( | ||
| "h-7 px-2 text-xs", | ||
| viewMode === "code" && "bg-background shadow-sm", | ||
| )} | ||
| onClick={() => setViewMode("code")} | ||
| className="h-8 text-xs" | ||
| onClick={handleCopy} | ||
| > | ||
| <Code2 className="mr-1.5 h-3.5 w-3.5" /> | ||
| <span className="hidden md:inline">Code</span> | ||
| <Copy className="h-3.5 w-3.5 md:mr-1.5" /> | ||
| <span className="hidden md:inline">Copy</span> | ||
| </Button> | ||
| </div> | ||
|
|
||
| <div className="mx-1 h-4 w-px bg-border" /> | ||
|
|
||
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| className="h-8 text-xs" | ||
| onClick={handleCopy} | ||
| > | ||
| <Copy className="h-3.5 w-3.5 md:mr-1.5" /> | ||
| <span className="hidden md:inline">Copy</span> | ||
| </Button> | ||
|
|
||
| <DropdownMenu> | ||
| <DropdownMenuTrigger asChild> | ||
| <Button variant="outline" size="sm" className="h-8 text-xs"> | ||
| <Download className="h-3.5 w-3.5 md:mr-1.5" /> | ||
| <span className="hidden md:inline">Export</span> | ||
| </Button> | ||
| </DropdownMenuTrigger> | ||
| <DropdownMenuContent align="end"> | ||
| <DropdownMenuItem onClick={onExportHtml}> | ||
| <Code2 className="mr-2 h-4 w-4" /> | ||
| Export HTML | ||
| </DropdownMenuItem> | ||
| <DropdownMenuItem onClick={onExport}> | ||
| <Download className="mr-2 h-4 w-4" /> | ||
| Export React | ||
| </DropdownMenuItem> | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
| <DropdownMenu> | ||
| <DropdownMenuTrigger asChild> | ||
| <Button variant="outline" size="sm" className="h-8 text-xs"> | ||
| <Download className="h-3.5 w-3.5 md:mr-1.5" /> | ||
| <span className="hidden md:inline">Export</span> | ||
| </Button> | ||
| </DropdownMenuTrigger> | ||
| <DropdownMenuContent align="end"> | ||
| <DropdownMenuItem onClick={onExportHtml}> | ||
| <Code2 className="mr-2 h-4 w-4" /> | ||
| Export HTML | ||
| </DropdownMenuItem> | ||
| <DropdownMenuItem onClick={onExport}> | ||
| <Download className="mr-2 h-4 w-4" /> | ||
| Export React | ||
| </DropdownMenuItem> | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
|
|
||
| <Button size="sm" className="h-8 text-xs" onClick={onSendTest}> | ||
| <Send className="h-3.5 w-3.5 md:mr-1.5" /> | ||
| <span className="hidden md:inline">Send Test</span> | ||
| </Button> | ||
| <Button size="sm" className="h-8 text-xs" onClick={onSendTest}> | ||
| <Send className="h-3.5 w-3.5 md:mr-1.5" /> | ||
| <span className="hidden md:inline">Send Test</span> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Add accessible names for icon-only mobile action buttons.
On small screens, these buttons render icons only (Preview, Code, Copy, Export, Send Test) and lose an accessible name because text is hidden with hidden md:inline. Add aria-label (or sr-only text) so screen readers can identify actions.
♿ Suggested fix
<Button
variant="ghost"
size="sm"
+ aria-label="Preview"
className={cn(
"h-7 px-2 text-xs",
viewMode === "preview" && "bg-background shadow-sm",
)}
onClick={() => setViewMode("preview")}
>
<Eye className="mr-1.5 h-3.5 w-3.5" />
- <span className="hidden md:inline">Preview</span>
+ <span className="hidden md:inline" aria-hidden="true">Preview</span>
</Button>
<Button
variant="ghost"
size="sm"
+ aria-label="Code"
className={cn(
"h-7 px-2 text-xs",
viewMode === "code" && "bg-background shadow-sm",
)}
onClick={() => setViewMode("code")}
>
<Code2 className="mr-1.5 h-3.5 w-3.5" />
- <span className="hidden md:inline">Code</span>
+ <span className="hidden md:inline" aria-hidden="true">Code</span>
</Button>
- <Button
+ <Button
variant="outline"
size="sm"
+ aria-label="Copy"
className="h-8 text-xs"
onClick={handleCopy}
>
<Copy className="h-3.5 w-3.5 md:mr-1.5" />
- <span className="hidden md:inline">Copy</span>
+ <span className="hidden md:inline" aria-hidden="true">Copy</span>
</Button>
- <Button variant="outline" size="sm" className="h-8 text-xs">
+ <Button variant="outline" size="sm" className="h-8 text-xs" aria-label="Export">
<Download className="h-3.5 w-3.5 md:mr-1.5" />
- <span className="hidden md:inline">Export</span>
+ <span className="hidden md:inline" aria-hidden="true">Export</span>
</Button>
- <Button size="sm" className="h-8 text-xs" onClick={onSendTest}>
+ <Button size="sm" className="h-8 text-xs" aria-label="Send Test" onClick={onSendTest}>
<Send className="h-3.5 w-3.5 md:mr-1.5" />
- <span className="hidden md:inline">Send Test</span>
+ <span className="hidden md:inline" aria-hidden="true">Send Test</span>
</Button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Button | |
| variant="ghost" | |
| size="sm" | |
| className={cn( | |
| "h-7 px-2 text-xs", | |
| viewMode === "preview" && "bg-background shadow-sm", | |
| )} | |
| onClick={() => setViewMode("preview")} | |
| > | |
| <Eye className="mr-1.5 h-3.5 w-3.5" /> | |
| <span className="hidden md:inline">Preview</span> | |
| </Button> | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| className={cn( | |
| "h-7 px-2 text-xs", | |
| viewMode === "code" && "bg-background shadow-sm", | |
| )} | |
| onClick={() => setViewMode("code")} | |
| > | |
| <Code2 className="mr-1.5 h-3.5 w-3.5" /> | |
| <span className="hidden md:inline">Code</span> | |
| </Button> | |
| </div> | |
| <div className="mx-1 h-4 w-px bg-border" /> | |
| <Button | |
| variant="ghost" | |
| variant="outline" | |
| size="sm" | |
| className={cn( | |
| "h-7 px-2 text-xs", | |
| viewMode === "code" && "bg-background shadow-sm", | |
| )} | |
| onClick={() => setViewMode("code")} | |
| className="h-8 text-xs" | |
| onClick={handleCopy} | |
| > | |
| <Code2 className="mr-1.5 h-3.5 w-3.5" /> | |
| <span className="hidden md:inline">Code</span> | |
| <Copy className="h-3.5 w-3.5 md:mr-1.5" /> | |
| <span className="hidden md:inline">Copy</span> | |
| </Button> | |
| </div> | |
| <div className="mx-1 h-4 w-px bg-border" /> | |
| <Button | |
| variant="outline" | |
| size="sm" | |
| className="h-8 text-xs" | |
| onClick={handleCopy} | |
| > | |
| <Copy className="h-3.5 w-3.5 md:mr-1.5" /> | |
| <span className="hidden md:inline">Copy</span> | |
| </Button> | |
| <DropdownMenu> | |
| <DropdownMenuTrigger asChild> | |
| <Button variant="outline" size="sm" className="h-8 text-xs"> | |
| <Download className="h-3.5 w-3.5 md:mr-1.5" /> | |
| <span className="hidden md:inline">Export</span> | |
| </Button> | |
| </DropdownMenuTrigger> | |
| <DropdownMenuContent align="end"> | |
| <DropdownMenuItem onClick={onExportHtml}> | |
| <Code2 className="mr-2 h-4 w-4" /> | |
| Export HTML | |
| </DropdownMenuItem> | |
| <DropdownMenuItem onClick={onExport}> | |
| <Download className="mr-2 h-4 w-4" /> | |
| Export React | |
| </DropdownMenuItem> | |
| </DropdownMenuContent> | |
| </DropdownMenu> | |
| <DropdownMenu> | |
| <DropdownMenuTrigger asChild> | |
| <Button variant="outline" size="sm" className="h-8 text-xs"> | |
| <Download className="h-3.5 w-3.5 md:mr-1.5" /> | |
| <span className="hidden md:inline">Export</span> | |
| </Button> | |
| </DropdownMenuTrigger> | |
| <DropdownMenuContent align="end"> | |
| <DropdownMenuItem onClick={onExportHtml}> | |
| <Code2 className="mr-2 h-4 w-4" /> | |
| Export HTML | |
| </DropdownMenuItem> | |
| <DropdownMenuItem onClick={onExport}> | |
| <Download className="mr-2 h-4 w-4" /> | |
| Export React | |
| </DropdownMenuItem> | |
| </DropdownMenuContent> | |
| </DropdownMenu> | |
| <Button size="sm" className="h-8 text-xs" onClick={onSendTest}> | |
| <Send className="h-3.5 w-3.5 md:mr-1.5" /> | |
| <span className="hidden md:inline">Send Test</span> | |
| </Button> | |
| <Button size="sm" className="h-8 text-xs" onClick={onSendTest}> | |
| <Send className="h-3.5 w-3.5 md:mr-1.5" /> | |
| <span className="hidden md:inline">Send Test</span> | |
| </Button> | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| aria-label="Preview" | |
| className={cn( | |
| "h-7 px-2 text-xs", | |
| viewMode === "preview" && "bg-background shadow-sm", | |
| )} | |
| onClick={() => setViewMode("preview")} | |
| > | |
| <Eye className="mr-1.5 h-3.5 w-3.5" /> | |
| <span className="hidden md:inline" aria-hidden="true">Preview</span> | |
| </Button> | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| aria-label="Code" | |
| className={cn( | |
| "h-7 px-2 text-xs", | |
| viewMode === "code" && "bg-background shadow-sm", | |
| )} | |
| onClick={() => setViewMode("code")} | |
| > | |
| <Code2 className="mr-1.5 h-3.5 w-3.5" /> | |
| <span className="hidden md:inline" aria-hidden="true">Code</span> | |
| </Button> | |
| </div> | |
| <div className="mx-1 h-4 w-px bg-border" /> | |
| <Button | |
| variant="outline" | |
| size="sm" | |
| aria-label="Copy" | |
| className="h-8 text-xs" | |
| onClick={handleCopy} | |
| > | |
| <Copy className="h-3.5 w-3.5 md:mr-1.5" /> | |
| <span className="hidden md:inline" aria-hidden="true">Copy</span> | |
| </Button> | |
| <DropdownMenu> | |
| <DropdownMenuTrigger asChild> | |
| <Button variant="outline" size="sm" className="h-8 text-xs" aria-label="Export"> | |
| <Download className="h-3.5 w-3.5 md:mr-1.5" /> | |
| <span className="hidden md:inline" aria-hidden="true">Export</span> | |
| </Button> | |
| </DropdownMenuTrigger> | |
| <DropdownMenuContent align="end"> | |
| <DropdownMenuItem onClick={onExportHtml}> | |
| <Code2 className="mr-2 h-4 w-4" /> | |
| Export HTML | |
| </DropdownMenuItem> | |
| <DropdownMenuItem onClick={onExport}> | |
| <Download className="mr-2 h-4 w-4" /> | |
| Export React | |
| </DropdownMenuItem> | |
| </DropdownMenuContent> | |
| </DropdownMenu> | |
| <Button size="sm" className="h-8 text-xs" aria-label="Send Test" onClick={onSendTest}> | |
| <Send className="h-3.5 w-3.5 md:mr-1.5" /> | |
| <span className="hidden md:inline" aria-hidden="true">Send Test</span> | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/preview/preview-header.tsx` around lines 135 - 195,
The icon-only mobile buttons lack accessible names; add aria-label attributes
(or visually hidden text) to each interactive control: the Preview and Code
buttons that call setViewMode("preview") / setViewMode("code"), the Copy button
that calls handleCopy, the Export DropdownTrigger button and its
DropdownMenuItem actions onExportHtml and onExport, and the Send Test button
that calls onSendTest so screen readers can identify them (use concise labels
like "Preview", "Code", "Copy", "Export", "Send Test").
Comment on lines
+36
to
+37
| if (moduleName in MODULE_CACHE) { | ||
| return MODULE_CACHE[moduleName]; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all MODULE_CACHE membership checks use own-property checks.
rg -nP --type=ts -C2 '\bmoduleName\s+in\s+MODULE_CACHE\b|\bhasOwnProperty\.call\(MODULE_CACHE,\s*moduleName\)'Repository: Blackmamoth/scribe
Length of output: 398
Use own-property checks for the module cache membership test.
Line 36 uses the in operator, which checks the prototype chain and could allow prototype-chain keys (like constructor) to bypass the allowlist.
Suggested fix
- if (moduleName in MODULE_CACHE) {
- return MODULE_CACHE[moduleName];
+ if (Object.prototype.hasOwnProperty.call(MODULE_CACHE, moduleName)) {
+ return MODULE_CACHE[moduleName];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (moduleName in MODULE_CACHE) { | |
| return MODULE_CACHE[moduleName]; | |
| if (Object.prototype.hasOwnProperty.call(MODULE_CACHE, moduleName)) { | |
| return MODULE_CACHE[moduleName]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/hooks/email-compiler.ts` around lines 36 - 37, The membership
test for MODULE_CACHE currently uses the in operator which inspects the
prototype chain; change the check in the email-compiler hook to use an
own-property test (e.g. Object.prototype.hasOwnProperty.call(MODULE_CACHE,
moduleName) or Object.hasOwn(MODULE_CACHE, moduleName)) so only direct keys on
MODULE_CACHE (referenced as MODULE_CACHE and moduleName in the code) are
considered when returning MODULE_CACHE[moduleName].
Comment on lines
+79
to
+86
| function DataDisplay({ dataPromise }: { dataPromise: Promise<Data> }) { | ||
| const data = use(dataPromise) // Unwraps the promise | ||
| return <div>{data.content}</div> | ||
| } | ||
|
|
||
| function DataSummary({ dataPromise }: { dataPromise: Promise<Data> }) { | ||
| const data = use(dataPromise) // Reuses the same promise | ||
| return <div>{data.summary}</div> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is the React use(promise) API stable in React 19, and what are the constraints for using it in Server Components vs Client Components?
💡 Result:
Yes, the React use(promise) API (imported as use from 'react') is stable in React 19 for both Server Components and Client Components. React 19 was released stable on December 5, 2024. Constraints: - In Server Components: Prefer async/await over use(promise) for data fetching, as await resumes rendering from the await point while use re-renders the entire component after resolution. use can still be used but is not the primary recommendation. - In Client Components: Fully supported. Create promises in Server Components and pass them as props to Client Components for stability across re-renders (Client-created promises recreate on every render, causing repeated suspensions unless cached via Suspense-compatible libraries/frameworks). Must wrap in for loading fallbacks. Resolved values must be serializable when passed server-to-client. Cannot create uncached promises in render (warns: "Creating promises inside a Client Component or hook is not yet supported"). Handle rejections with Error Boundaries or promise.catch (not try/catch). use also reads context conditionally/loops (unlike hooks).
Citations:
- 1: https://react.dev/blog/2024/04/25/react-19
- 2: https://github.com/facebook/react/releases/tag/v19.0.0
- 3: https://react.dev/blog/2024/12/05/react-19
- 4: https://react.dev/reference/react/use
🏁 Script executed:
cd .agents/skills/vercel-react-best-practices/rules && \
head -n 90 async-suspense-boundaries.md | tail -n 20Repository: Blackmamoth/scribe
Length of output: 691
🏁 Script executed:
cat -n .agents/skills/vercel-react-best-practices/rules/async-suspense-boundaries.md | sed -n '1,100p'Repository: Blackmamoth/scribe
Length of output: 3264
🏁 Script executed:
grep -n "React" .agents/skills/vercel-react-best-practices/rules/async-suspense-boundaries.mdRepository: Blackmamoth/scribe
Length of output: 44
🏁 Script executed:
cat .agents/skills/vercel-react-best-practices/rules/async-suspense-boundaries.mdRepository: Blackmamoth/scribe
Length of output: 2571
Add React version and component type requirements to the use(dataPromise) example.
The example on lines 79–86 uses React's use() API without documenting that it requires React 19+ and must be used in Client Components with promises passed from Server Components. Add a brief note clarifying these constraints so readers don't encounter runtime errors when copying this pattern into projects with older React versions or incorrect component contexts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
@.agents/skills/vercel-react-best-practices/rules/async-suspense-boundaries.md
around lines 79 - 86, Update the example using use(dataPromise) to state that
the use() API requires React 19+ and that DataDisplay and DataSummary must be
Client Components (e.g., annotated as client) while the Promise should originate
from a Server Component; explicitly mention these constraints alongside the
DataDisplay, DataSummary, and use(dataPromise) symbols so readers know to only
use this pattern in React 19+ and with client-side components consuming
server-provided promises.
Comment on lines
+27
to
+35
| **Correct - Next.js 13.5+ (recommended):** | ||
|
|
||
| ```js | ||
| // next.config.js - automatically optimizes barrel imports at build time | ||
| module.exports = { | ||
| experimental: { | ||
| optimizePackageImports: ['lucide-react', '@mui/material'] | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Next.js (latest stable), is optimizePackageImportsstill configured underexperimentalinnext.config.js? Also, are the documented performance gains (15–70% dev boot, 28% builds, 40% cold starts) stated as general guarantees or benchmark-specific in official Vercel/Next.js sources?
💡 Result:
No, optimizePackageImports is no longer configured under experimental in next.config.js for the latest stable Next.js (v16.2.x as of 2026-04-04). It is now a top-level configuration option. The documented performance gains of 15–70% dev boot, 28% builds, and 40% cold starts are not found in official Vercel/Next.js sources. No official documentation or release notes mention these specific figures as general guarantees or benchmarks for optimizePackageImports. Performance improvements from package optimization are discussed qualitatively (e.g., reducing bundle sizes by tree-shaking unused exports from barrel files), but no quantitative claims matching the query are present in searched official sources.
Citations:
- 1: https://nextjs.org/docs/api-reference/next.config.js/introduction
- 2: https://nextjs.net.cn/docs/app/api-reference/config/next-config-js/optimizePackageImports
- 3: https://www.reflowhub.dev/en/docs/nextjs/app-api-reference/app-api-reference-config/app-api-reference-config-next-config-js/app-api-reference-config-next-config-js-optimizePackageImports
- 4: https://nextjs.org/docs/pages/api-reference/config/next-config-js/optimizePackageImports
- 5: https://nextjs.org/docs/14/app/api-reference/next-config-js
- 6: https://nextjs.org/blog/next-16-1
- 7: https://nextjs.org/blog/next-16-2-turbopack
- 8: https://nextjs.org/blog/next-16
- 9: https://nextjs.org/blog/next-16-2
Update Next.js configuration to match current release; remove or source performance metrics.
Lines 27–35: optimizePackageImports is no longer under experimental in Next.js v16.2.x (current). Move it to the top-level config object.
Lines 27–35 and 56: The performance gains (15–70% dev boot, 28% builds, 40% cold starts) are not documented in official Vercel or Next.js sources. Either remove these figures or add explicit citations. Qualitative statements (e.g., "reduces bundle size by tree-shaking unused exports") are acceptable without citations; quantitative claims require documentation.
Example correction for lines 27–35:
// next.config.js
module.exports = {
optimizePackageImports: ['lucide-react', '@mui/material']
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/vercel-react-best-practices/rules/bundle-barrel-imports.md
around lines 27 - 35, Update the Next.js config by moving optimizePackageImports
out of the experimental object into the top-level export (i.e., set
optimizePackageImports: ['lucide-react','@mui/material'] on the root
module.exports) so it matches Next.js v16.2+ usage, and edit the doc text that
cites performance numbers (lines referenced around the optimizePackageImports
example and the other metric at line ~56): either remove the quantitative
figures or replace them with explicit citations to official Vercel/Next.js
benchmarks; keep qualitative statements (e.g., tree-shaking reduces bundle size)
but ensure any numeric claims include a verifiable source.
| return <nav>{items.map(renderItem)}</nav> | ||
| } | ||
|
|
||
| function Layout({ children }: { children: ReactNode }) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if file exists and read the content around line 67
if [ -f ".agents/skills/vercel-react-best-practices/rules/server-parallel-fetching.md" ]; then
echo "File found. Reading content around line 67:"
sed -n '60,75p' ".agents/skills/vercel-react-best-practices/rules/server-parallel-fetching.md"
else
echo "File not found at specified path. Searching for similar files:"
find . -type f -name "*server-parallel-fetching*" 2>/dev/null | head -20
fiRepository: Blackmamoth/scribe
Length of output: 354
🏁 Script executed:
# Read more context to see the full example block
sed -n '40,80p' ".agents/skills/vercel-react-best-practices/rules/server-parallel-fetching.md"Repository: Blackmamoth/scribe
Length of output: 733
🏁 Script executed:
# Read from the start of the file to understand the structure better
sed -n '1,80p' ".agents/skills/vercel-react-best-practices/rules/server-parallel-fetching.md" | head -100Repository: Blackmamoth/scribe
Length of output: 1607
Add ReactNode import in the TSX example.
The third code example at line 67 uses ReactNode in the type annotation without importing it, which will cause a TypeScript error if copied directly. Add the import at the top of that code block.
Suggested doc snippet fix
+import type { ReactNode } from 'react'
+
async function Header() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function Layout({ children }: { children: ReactNode }) { | |
| import type { ReactNode } from 'react' | |
| function Layout({ children }: { children: ReactNode }) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/vercel-react-best-practices/rules/server-parallel-fetching.md
at line 67, The TSX example uses ReactNode in the Layout component signature but
doesn't import it; update the example to import ReactNode from React at the top
of the code block so the type annotation in function Layout({ children }: {
children: ReactNode }) compiles (add "import { ReactNode } from 'react'").
Ensure the import appears alongside any existing React imports in that snippet.
Comment on lines
+6
to
+22
| size?: "xs" | "sm" | "md" | "lg"; | ||
| className?: string; | ||
| } | ||
|
|
||
| const sizeClasses = { | ||
| xs: "size-4 text-[8px]", | ||
| sm: "size-6 text-[10px]", | ||
| md: "size-8 text-xs", | ||
| lg: "size-10 text-sm", | ||
| } as const; | ||
|
|
||
| const imgDimensions = { | ||
| xs: 16, | ||
| sm: 24, | ||
| md: 32, | ||
| lg: 40, | ||
| } as const; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Model the size prop with cva.
This component now has an explicit size variant, but the styling is still hand-maintained in sizeClasses. Please move that variant to cva so it follows the repo pattern and doesn't drift as more sizes/styles are added.
Based on learnings: Applies to **/*.{tsx,jsx} : Use class-variance-authority (cva) for component variants in React components
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/brands/brand-avatar.tsx` around lines 6 - 22, Replace
the handcrafted sizeClasses with a cva-based variant: create a cva (e.g.,
avatarClasses or brandAvatarClasses) that includes the base classes and a
variants block for size mapping the same strings (xs, sm, md, lg) to their
respective class strings, and set defaultVariants.size to "md"; then update the
component to call that cva with avatarClasses({ size, className }) instead of
using sizeClasses, while keeping imgDimensions as-is for image pixel values.
Comment on lines
+32
to
+45
| if (logoUrl) { | ||
| return ( | ||
| <img | ||
| src={logoUrl} | ||
| alt={name} | ||
| width={dim} | ||
| height={dim} | ||
| className={cn( | ||
| "rounded-full object-cover", | ||
| sizeClasses[size], | ||
| className, | ||
| )} | ||
| /> | ||
| ); |
Contributor
There was a problem hiding this comment.
Fall back to initials when the logo request fails.
Any truthy logoUrl permanently selects the <img> branch, so a 404/blocked image shows a broken avatar instead of the initials fallback. Please treat image load failures the same way as a missing logoUrl.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/brands/brand-avatar.tsx` around lines 32 - 45, The
current BrandAvatar component always chooses the <img> branch when logoUrl is
truthy, causing broken images to display; add an image error state (e.g.,
hasImageError via useState) and an onError handler on the <img> that sets that
state so the component treats image load failures the same as a missing logoUrl
and falls back to rendering initials. Ensure you also reset the error state when
the logoUrl prop changes (useEffect) so new URLs are retried, and keep existing
identifiers (BrandAvatar, logoUrl, sizeClasses, dim, className) when updating
the render conditional to check !logoUrl || hasImageError before showing the
<img>.
Comment on lines
+33
to
+36
| const { data } = authClient.useSession(); | ||
| const isAnonymous = data?.user?.isAnonymous; | ||
| const userInitials = data?.user?.name ? getInitials(data.user.name) : ""; | ||
|
|
Contributor
There was a problem hiding this comment.
Don't pass src="" to AvatarImage.
When the user has no image, both avatars render with an empty src. Browsers resolve that against the current document URL, which can trigger pointless requests and flash a broken-image state before the fallback kicks in. Pass undefined instead.
🐛 Proposed fix
const { data } = authClient.useSession();
const isAnonymous = data?.user?.isAnonymous;
const userInitials = data?.user?.name ? getInitials(data.user.name) : "";
+ const avatarSrc = data?.user?.image || undefined;
@@
- <AvatarImage src={data?.user?.image || ""} alt="User" />
+ <AvatarImage src={avatarSrc} alt="User" />
@@
- <AvatarImage src={data?.user?.image || ""} alt="User" />
+ <AvatarImage src={avatarSrc} alt="User" />Also applies to: 45-45, 71-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/layout/sidebar-user-menu.tsx` around lines 33 - 36,
The AvatarImage components are receiving an empty string for src when the user
has no image, causing browsers to request the current document URL; update the
places that render AvatarImage (where you access user image via
authClient.useSession() / data?.user?.image) to pass undefined instead of ""
when the image is falsy (e.g., use a conditional that passes data?.user?.image
?? undefined or data?.user?.image ? data.user.image : undefined) and apply the
same change to the other AvatarImage usages referenced in the diff so the
fallback renders without a broken-image request.
Comment on lines
+157
to
+161
| {codeLines.map((line, i) => { | ||
| const props = fadeProps(0.8 + i * 0.04, 0.3); | ||
| return ( | ||
| <motion.rect | ||
| key={`${line.tx}-${line.ty}`} |
Contributor
There was a problem hiding this comment.
Fix the duplicate key in codeLines.
Two items in codeLines share the same tx/ty pair, so key={${line.tx}-${line.ty}} collides here. That makes the animated list unstable because React reconciles those rectangles as the same child.
🐛 Proposed fix
- key={`${line.tx}-${line.ty}`}
+ key={`${line.tx}-${line.ty}-${line.width}-${line.height}`}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {codeLines.map((line, i) => { | |
| const props = fadeProps(0.8 + i * 0.04, 0.3); | |
| return ( | |
| <motion.rect | |
| key={`${line.tx}-${line.ty}`} | |
| {codeLines.map((line, i) => { | |
| const props = fadeProps(0.8 + i * 0.04, 0.3); | |
| return ( | |
| <motion.rect | |
| key={`${line.tx}-${line.ty}-${line.width}-${line.height}`} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/preview/preview-loader.tsx` around lines 157 - 161,
The current motion.rect elements use a non-unique key string built from line.tx
and line.ty (key={`${line.tx}-${line.ty}`}) which collides for two entries in
codeLines; change the key to be unique per item — for example include the loop
index or a unique identifier (e.g., key={`${line.tx}-${line.ty}-${i}`}` or use a
stable line.id) when mapping over codeLines in the component that calls
fadeProps and renders motion.rect so React can correctly reconcile the animated
rectangles.
Comment on lines
+36
to
+48
| useEffect(() => { | ||
| if (!trigger) return; | ||
|
|
||
| const intervalMs = interval * 1000; | ||
| const timer = setInterval(() => { | ||
| setCurrentIndex((current) => { | ||
| const next = (current + 1) % items.length; | ||
| onIndexChange?.(next); | ||
| return next; | ||
| }); | ||
| }, intervalMs); | ||
| return () => clearInterval(timer); | ||
| }, [items.length, interval, onIndexChange, trigger]); |
Contributor
There was a problem hiding this comment.
Guard against empty children array to prevent NaN index.
If children is empty, items.length is 0 and the modulo operation (current + 1) % items.length produces NaN, causing undefined behavior. Additionally, items[currentIndex] would be undefined.
🛡️ Proposed fix
useEffect(() => {
- if (!trigger) return;
+ if (!trigger || items.length === 0) return;
const intervalMs = interval * 1000;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (!trigger) return; | |
| const intervalMs = interval * 1000; | |
| const timer = setInterval(() => { | |
| setCurrentIndex((current) => { | |
| const next = (current + 1) % items.length; | |
| onIndexChange?.(next); | |
| return next; | |
| }); | |
| }, intervalMs); | |
| return () => clearInterval(timer); | |
| }, [items.length, interval, onIndexChange, trigger]); | |
| useEffect(() => { | |
| if (!trigger || items.length === 0) return; | |
| const intervalMs = interval * 1000; | |
| const timer = setInterval(() => { | |
| setCurrentIndex((current) => { | |
| const next = (current + 1) % items.length; | |
| onIndexChange?.(next); | |
| return next; | |
| }); | |
| }, intervalMs); | |
| return () => clearInterval(timer); | |
| }, [items.length, interval, onIndexChange, trigger]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/ui/text-loop.tsx` around lines 36 - 48, The useEffect
in TextLoop (text-loop.tsx) currently creates a timer that computes (current +
1) % items.length which yields NaN when items.length is 0; fix by guarding
against an empty items/children array before starting the interval (e.g., only
set up the interval when items.length > 0 and trigger is true), and inside the
state updater for setCurrentIndex avoid the modulo when items.length === 0
(return 0 or keep current), only call onIndexChange when the next index is
valid, and ensure the cleanup still clears the timer; update the useEffect
conditions and the setCurrentIndex callback in the TextLoop component
accordingly.
Comment on lines
+8
to
+14
| export function getInitials(name: string): string { | ||
| return name | ||
| .split(" ") | ||
| .map((word) => word.charAt(0).toUpperCase()) | ||
| .join("") | ||
| .slice(0, 2); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Harden getInitials for irregular whitespace.
Consider trimming and splitting on \s+ to avoid empty word fragments and make initials generation more robust.
Suggested patch
export function getInitials(name: string): string {
return name
- .split(" ")
+ .trim()
+ .split(/\s+/)
+ .filter(Boolean)
.map((word) => word.charAt(0).toUpperCase())
.join("")
.slice(0, 2);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function getInitials(name: string): string { | |
| return name | |
| .split(" ") | |
| .map((word) => word.charAt(0).toUpperCase()) | |
| .join("") | |
| .slice(0, 2); | |
| } | |
| export function getInitials(name: string): string { | |
| return name | |
| .trim() | |
| .split(/\s+/) | |
| .filter(Boolean) | |
| .map((word) => word.charAt(0).toUpperCase()) | |
| .join("") | |
| .slice(0, 2); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/utils.ts` around lines 8 - 14, The getInitials function can
produce empty fragments with irregular whitespace; update getInitials to trim
the input, split on a whitespace regex (e.g., /\s+/), optionally filter out
empty strings, then map to the first character uppercased and join/slice to two
characters so initials are stable for names with extra spaces; locate and modify
the getInitials function to implement these changes.
| @@ -1,5 +1,5 @@ | |||
| import { createRouter as createTanStackRouter } from "@tanstack/react-router"; | |||
| import Loader from "./components/loader"; | |||
| import { Loader } from "./components/loader"; | |||
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the app alias import on Line 2.
Please switch this app-relative import to @/components/loader for consistency with repository import rules.
Suggested patch
-import { Loader } from "./components/loader";
+import { Loader } from "@/components/loader";As per coding guidelines, "Use path alias @/* mapping to src/* for app-relative imports".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { Loader } from "./components/loader"; | |
| import { Loader } from "@/components/loader"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/router.tsx` at line 2, Change the app-relative import of the
Loader component in router.tsx to use the repository path alias: replace the
current import of Loader (symbol: Loader) from "./components/loader" with
"@/components/loader" so it follows the app alias mapping to src/* and remains
consistent with project import rules.
Contributor
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 `@apps/web/src/components/layout/sidebar-nav.tsx`:
- Around line 55-56: Route checks like the isActive callbacks in sidebar-nav.tsx
use pathname.startsWith("/templates") which causes false positives (e.g.,
"/templates-old"); update each isActive to use exact-or-child matching by
comparing pathname === base || pathname.startsWith(base + "/") where base is the
route string (e.g., "/templates", "/contacts"); apply this change to the
isActive/open checks found around the other occurrences mentioned (the blocks at
lines 62-63, 91-93, 95-99) so all nav items use the same exact-or-child matching
logic.
- Line 10: The code uses React.ElementType but doesn't import the React
namespace; change the import statement to import ElementType from "react" and
replace React.ElementType occurrences with the imported ElementType type (e.g.,
update the import line that currently imports
memo/useCallback/useEffect/useState to also import ElementType and update the
type usage in the component props or variables where React.ElementType is
referenced, such as the sidebar nav prop/type declarations).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3b363bb-b48d-440d-b880-d97557ce441c
📒 Files selected for processing (1)
apps/web/src/components/layout/sidebar-nav.tsx
| List, | ||
| Users, | ||
| } from "lucide-react"; | ||
| import { memo, useCallback, useEffect, useState } from "react"; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify React namespace type usage patterns in TS/TSX files.
rg -nP --type=ts --type=tsx '\bReact\.(ElementType|FC|ReactNode|ComponentType|RefObject)\b' -C2Repository: Blackmamoth/scribe
Length of output: 89
🏁 Script executed:
cat -n apps/web/src/components/layout/sidebar-nav.tsx | head -40Repository: Blackmamoth/scribe
Length of output: 1101
🏁 Script executed:
rg -n "React\." apps/web/src/components/layout/sidebar-nav.tsx -A 2 -B 2Repository: Blackmamoth/scribe
Length of output: 187
🏁 Script executed:
cat apps/web/src/components/layout/sidebar-nav.tsx | wc -lRepository: Blackmamoth/scribe
Length of output: 65
🏁 Script executed:
rg "import.*React" apps/web/src/components/layout/sidebar-nav.tsxRepository: Blackmamoth/scribe
Length of output: 44
🏁 Script executed:
rg "React\." apps/web/src/components/layout/sidebar-nav.tsxRepository: Blackmamoth/scribe
Length of output: 87
🏁 Script executed:
cat -n apps/web/src/components/layout/sidebar-nav.tsx | sed -n '26,35p'Repository: Blackmamoth/scribe
Length of output: 341
Import ElementType type directly from React instead of using React.ElementType namespace.
Line 29 uses React.ElementType but the file does not import the React namespace. This will cause a TypeScript compilation error. Import the type directly from the react module.
Suggested fix
-import { memo, useCallback, useEffect, useState } from "react";
+import {
+ type ElementType,
+ memo,
+ useCallback,
+ useEffect,
+ useState,
+} from "react";Then update line 29:
- icon: React.ElementType;
+ icon: ElementType;This aligns with TypeScript strict mode requirements in the coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { memo, useCallback, useEffect, useState } from "react"; | |
| import { | |
| type ElementType, | |
| memo, | |
| useCallback, | |
| useEffect, | |
| useState, | |
| } from "react"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/layout/sidebar-nav.tsx` at line 10, The code uses
React.ElementType but doesn't import the React namespace; change the import
statement to import ElementType from "react" and replace React.ElementType
occurrences with the imported ElementType type (e.g., update the import line
that currently imports memo/useCallback/useEffect/useState to also import
ElementType and update the type usage in the component props or variables where
React.ElementType is referenced, such as the sidebar nav prop/type
declarations).
Comment on lines
+55
to
+56
| isActive: (pathname) => pathname.startsWith("/templates"), | ||
| }, |
Contributor
There was a problem hiding this comment.
Make route checks segment-aware to avoid false active/open states.
startsWith("/contacts") and startsWith("/templates") also match unrelated paths like /contacts-foo or /templates-old. Use exact-or-child matching (=== base || startsWith(base + "/")).
Suggested fix
+const isRouteOrChild = (pathname: string, base: string) =>
+ pathname === base || pathname.startsWith(`${base}/`);
@@
- isActive: (pathname) => pathname.startsWith("/templates"),
+ isActive: (pathname) => isRouteOrChild(pathname, "/templates"),
@@
- isActive: (pathname) => pathname.startsWith("/contacts"),
+ isActive: (pathname) => isRouteOrChild(pathname, "/contacts"),
@@
- const [contactsOpen, setContactsOpen] = useState(
- location.pathname.startsWith("/contacts"),
- );
+ const [contactsOpen, setContactsOpen] = useState(
+ isRouteOrChild(location.pathname, "/contacts"),
+ );
@@
- if (location.pathname.startsWith("/contacts")) {
+ if (isRouteOrChild(location.pathname, "/contacts")) {
setContactsOpen(true);
}Also applies to: 62-63, 91-93, 95-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/layout/sidebar-nav.tsx` around lines 55 - 56, Route
checks like the isActive callbacks in sidebar-nav.tsx use
pathname.startsWith("/templates") which causes false positives (e.g.,
"/templates-old"); update each isActive to use exact-or-child matching by
comparing pathname === base || pathname.startsWith(base + "/") where base is the
route string (e.g., "/templates", "/contacts"); apply this change to the
isActive/open checks found around the other occurrences mentioned (the blocks at
lines 62-63, 91-93, 95-99) so all nav items use the same exact-or-child matching
logic.
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
UI/UX Improvements
Performance
Documentation