refactor(platform): fix all React Doctor issues (82 → 100)#571
Conversation
Resolve 6 errors and 128 warnings flagged by react-doctor: - Replace fetch-in-useEffect with useReactQuery hooks for document previews - Replace derived-state useEffects with render-time sync (image, json-input) - Replace dangerouslySetInnerHTML with callback ref pattern - Replace motion with m + LazyMotion provider (~30kb bundle savings) - Extract useResizable hook and sub-components to reduce component size - Refactor document-preview-pdf to use useReducer - Fix array index keys, default prop allocations, unnecessary useMemo - Add accessibility roles for interactive elements
Greptile SummaryThis PR systematically addresses all React Doctor issues, improving the score from 82 to 100 through comprehensive refactoring focused on performance, maintainability, and best practices. Key Changes:
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| services/platform/app/routes/__root.tsx | Added LazyMotion provider with domAnimation features for bundle size optimization |
| services/platform/app/components/ui/data-display/image.tsx | Replaced derived state useEffect with render-time calculation and added key prop for reset |
| services/platform/app/components/ui/forms/json-input.tsx | Major refactor: extracted sub-components, replaced derived state with render-time sync, improved validation logic |
| services/platform/app/features/automations/components/automation-sidepanel.tsx | Extracted useResizable hook and sub-components (ValidationMessages, StepEditorContent), consolidated state, added accessibility role |
| services/platform/app/features/documents/hooks/use-document-preview.ts | New hook extracting fetch-in-useEffect logic to useReactQuery for docx, xlsx, and text previews |
| services/platform/app/features/documents/components/document-preview-pdf.tsx | Refactored to useReducer pattern for complex state management, improved render logic organization |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[React Doctor Issues<br/>82 → 100] --> B[Bundle Optimization]
A --> C[State Management]
A --> D[Code Quality]
B --> B1[Replace motion with m]
B --> B2[Add LazyMotion provider]
B --> B3[~30KB bundle savings]
C --> C1[fetch-in-useEffect → useReactQuery]
C --> C2[Derived state → Render-time sync]
C --> C3[Complex state → useReducer]
D --> D1[Extract sub-components]
D --> D2[Fix array index keys]
D --> D3[Add accessibility roles]
D --> D4[Replace dangerouslySetInnerHTML]
C1 --> E[useDocxPreview]
C1 --> F[useXlsxPreview]
C1 --> G[useTextPreview]
C2 --> H[Image component]
C2 --> I[JsonInput component]
C3 --> J[PDF Viewer]
D1 --> K[AutomationSidePanel]
D1 --> L[JsonInput]
D4 --> M[Callback ref pattern]
Last reviewed commit: b93a5b2
Update framer-motion mock to include m, LazyMotion, and domAnimation exports to match the motion → m migration.
📝 WalkthroughWalkthroughThis PR introduces documentation for a React Doctor agent, optimizes component state management through lazy initialization and ref-based tracking, extracts reusable constants for default values, refactors complex components with simplified state patterns (notably JSON input validation and PDF viewer reducer), migrates Framer Motion imports to shorthand notation (motion → m), replaces array-index React keys with stable identifiers, creates new custom hooks for document preview functionality (useDocxPreview, useXlsxPreview, useTextPreview), and removes unnecessary memoization (useMemo) in several UI components. Changes span layout, theming, data display, automation, chat, and document features. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
services/platform/app/features/automations/components/automation-sidepanel.tsx (1)
392-403:⚠️ Potential issue | 🟡 MinorResize handle has
role="separator"andtabIndex={0}but no keyboard interaction.The separator is focusable and has ARIA attributes, which is great, but there's no
onKeyDownhandler. Keyboard users who tab to it have no way to resize. Per WAI-ARIA, aseparatorwithtabIndex={0}is expected to respond to arrow keys.Either add an
onKeyDownthat adjusts width withArrowLeft/ArrowRight, or removetabIndex={0}to avoid implying interactivity that doesn't exist.♿ Proposed fix — add keyboard resize support in the hook
In
useResizable, add a keyboard handler and return it:+ const handleKeyDown = useCallback( + (e: React.KeyboardEvent) => { + const step = 20; + if (e.key === 'ArrowLeft') { + setWidth((w) => Math.min(MAX_WIDTH, w + step)); + } else if (e.key === 'ArrowRight') { + setWidth((w) => Math.max(MIN_WIDTH, w - step)); + } + }, + [], + ); + - return { width, handleMouseDown }; + return { width, handleMouseDown, handleKeyDown };Then on the handle element:
<div role="separator" aria-orientation="vertical" + aria-valuenow={width} + aria-valuemin={MIN_WIDTH} + aria-valuemax={MAX_WIDTH} tabIndex={0} onMouseDown={handleMouseDown} + onKeyDown={handleKeyDown}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/app/features/automations/components/automation-sidepanel.tsx` around lines 392 - 403, The separator div is focusable (tabIndex={0}) and has role="separator" but no keyboard handling; update the resizable logic in useResizable to expose an onKeyDown handler (or add an onKeyDown inline) that listens for ArrowLeft/ArrowRight (and optionally ArrowUp/ArrowDown) to call the same resize routine used by handleMouseDown/handleDrag, and attach that handler to the separator element (the div using handleMouseDown). Alternatively, if you do not want keyboard resize support, remove tabIndex={0} and role="separator" to avoid implying interactivity; prefer adding the onKeyDown in useResizable and reusing existing resize functions so keyboard and pointer resizing remain consistent.services/platform/app/features/automations/components/automation-assistant/thinking-animation.tsx (1)
29-64:⚠️ Potential issue | 🟡 Minor
exitanimations onm.div/m.spanwill not fire without anAnimatePresencewrapper.The
exitprops (lines 33, 44) require anAnimatePresenceancestor to function. Currently,ThinkingAnimationis rendered conditionally inMessageListbut neitherMessageListnor its parentAutomationAssistantContentwraps the component inAnimatePresence. When the component unmounts (when loading completes), the exit animations are skipped. To fix this, wrap<MessageList>or the conditional<ThinkingAnimation>in<AnimatePresence>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/app/features/automations/components/automation-assistant/thinking-animation.tsx` around lines 29 - 64, The exit animations on the motion elements inside ThinkingAnimation (the m.div and m.span using exit props) won't run unless the component is wrapped in Framer Motion's AnimatePresence; update the rendering so the conditional mount/unmount of ThinkingAnimation is wrapped by <AnimatePresence>—either wrap the conditional ThinkingAnimation itself or wrap the parent MessageList (or AutomationAssistantContent where MessageList is rendered) with AnimatePresence—so the exit animations on ThinkingAnimation will be triggered on unmount.services/platform/app/features/documents/components/document-preview-pdf.tsx (1)
110-175:⚠️ Potential issue | 🟠 MajorPending render requests can be dropped due stale state capture in async render flow.
Line 171 reads
state.pageNumPendingfrom the async function’s captured state, not guaranteed latest reducer state. Requests queued during render can be missed.🔧 Proposed fix (consume pending from reducer state in an effect)
- if (state.pageNumPending !== null) { - const pending = state.pageNumPending; - dispatch({ type: 'CONSUME_PENDING' }); - void renderPageRef.current?.(pending); - }// Add outside renderPageRef.current useEffect(() => { if (state.pageRendering || state.pageNumPending === null) return; const pending = state.pageNumPending; dispatch({ type: 'CONSUME_PENDING' }); void renderPageRef.current?.(pending); }, [state.pageRendering, state.pageNumPending]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/app/features/documents/components/document-preview-pdf.tsx` around lines 110 - 175, The async renderPageRef.current reads state.pageNumPending from a stale closure which can drop pending render requests; instead, stop consuming pending inside renderPageRef.current and add an effect that watches state.pageRendering and state.pageNumPending: when pageRendering is false and pageNumPending !== null, read pending from the reducer state, dispatch({ type: 'CONSUME_PENDING' }), and call renderPageRef.current(pending). Remove or avoid using state.pageNumPending inside renderPageRef.current to ensure the reducer state is the single source of truth; reference renderPageRef.current, state.pageNumPending, state.pageRendering, dispatch and the CONSUME_PENDING action when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/app/components/theme/theme-color-meta.tsx`:
- Around line 17-20: Remove the first-render guard so the effect that updates
the theme-color meta tag runs on initial mount: in the ThemeColorMeta component
(remove the hasMounted ref usage and the early return block that checks
hasMounted.current), let the useEffect that reads resolvedTheme execute on mount
and when resolvedTheme changes, and delete the hasMounted variable if it becomes
unused.
In `@services/platform/app/components/ui/data-display/image.tsx`:
- Around line 41-53: The component currently uses hasError/setHasError so any
load error permanently forces fallbackSrc even after src changes; replace this
with tracking the specific failed URL: add state failedSrc (and setFailedSrc),
compute currentSrc as (failedSrc === src ? fallbackSrc : src || fallbackSrc),
update handleError to call setFailedSrc(src), and add a useEffect that clears
failedSrc (setFailedSrc(null)) whenever src changes so new URLs are retried;
keep key={src} if you still want remounting but do not rely on it to reset
component state.
In `@services/platform/app/components/ui/forms/json-input.tsx`:
- Around line 313-333: The validation state is being initialized to a
always-valid value and only recomputed when `value` changes via the inline
`prevValueRef` check, and the cancel handler unconditionally clears validation;
update initialization and cancel logic to derive validation from real content
using `computeValidation(value, schema, t)` instead of forcing `isValid: true`.
Specifically, set the initial `validation` state by calling
`computeValidation(value, schema, t)` (or run that computation in a `useEffect`
that mirrors the existing `prevValueRef` logic), and change the
cancel/edit-reset code (the block that currently calls `setValidation` around
lines 395–399) to recompute and restore validation for the restored `value`
using `computeValidation` rather than clearing it. Ensure you reference and
update `validation`, `setValidation`, `computeValidation`, `prevValueRef`, and
the cancel/edit reset handler so initial and restored values reflect real
JSON/schema validation.
- Line 140: The div element that currently sets role="region" (the element with
className "p-3" and aria-describedby={describedBy}) creates an unlabeled
landmark; either remove the redundant role attribute or supply an accessible
name by adding aria-label or aria-labelledby that points to an existing
heading/label ID. Update the div in json-input.tsx so it no longer uses
role="region" without a label, or add a clear accessible name
(aria-label/aria-labelledby) tied to a visible label element.
- Around line 197-203: The component's textarea (json-input.tsx) currently
accepts the rows prop but then forces a fixed height via the h-[12.5rem] utility
in the className, effectively ignoring rows; update the rendering in the JSON
input component (where rows and className are applied) to stop forcing a fixed
height—either remove h-[12.5rem] or make height conditional (e.g., use min-h or
apply h only when rows is undefined) so consumer-provided rows take effect;
ensure the textarea still keeps desired styling (overflow-y-auto, resize-none)
while respecting the rows prop.
In
`@services/platform/app/features/automations/components/automation-sidepanel.tsx`:
- Around line 87-91: Add explicit role="list" attributes to the unordered lists
in the AutomationSidepanel component to restore list semantics for
VoiceOver/Safari; specifically, update the <ul> that renders
{uniqueErrors.map((error) => (...))} and the other <ul> at the later block
(lines referenced in the review) to include role="list" so Tailwind v4
Preflight's reset doesn't remove list semantics.
- Line 518: The prop usage redundantly null-coalesces stepOptions even though it
already has a default of EMPTY_STEP_OPTIONS in the component props
destructuring; remove the dead code by changing the JSX prop from
stepOptions={stepOptions ?? EMPTY_STEP_OPTIONS} to simply
stepOptions={stepOptions}, locating the usage in automation-sidepanel.tsx and
keeping the existing EMPTY_STEP_OPTIONS default in the component destructuring.
In
`@services/platform/app/features/documents/components/document-preview-pdf.tsx`:
- Around line 209-214: The inline call to renderPageRef.current inside the PDF
load promise should be removed because it runs before the state update from
dispatch({type: 'PDF_LOADED', doc}) completes; instead add a useEffect that
watches state.pdfDoc and, when it becomes non-null, calls
renderPageRef.current({ pageNum: 1, scale: initialState.scale }); specifically:
delete the immediate invocation in the .then callback that follows dispatch, add
a useEffect that returns early if !state.pdfDoc and otherwise calls
renderPageRef.current with pageNum 1 and initialState.scale; reference
renderPageRef, state.pdfDoc, dispatch('PDF_LOADED'), and initialState.scale when
applying the change.
- Around line 194-223: The PDF loader effect in document-preview-pdf.tsx should
be made reactive to url changes and cleaned up properly: change the effect that
creates the script (the useEffect that currently has an empty dependency array
and uses urlRef) to depend on url, remove the created script and its 'load'
listener in the effect cleanup, and revoke any loadingTask if present to avoid
leaked listeners; also ensure the initial page is rendered when a document is
loaded—either call renderPageRef.current({ pageNum: 1, scale: initialState.scale
}) inside the loadingTask.promise.then block after dispatch({ type:
'PDF_LOADED', doc }) or, if you prefer to keep the render call elsewhere, add a
separate effect that watches state.pdfDoc and invokes renderPageRef.current for
the initial render when state.pdfDoc becomes available.
In `@services/platform/app/features/documents/hooks/use-document-preview.ts`:
- Around line 10-13: The queryFn in use-document-preview.ts currently ignores
React Query's AbortSignal causing DOCX/XLSX fetches and parsing to continue
after cancellation; update the queryFn signature to accept the query context ({
signal }) and pass that signal into fetch (fetch(url, { signal })) and into any
downstream parsing calls (e.g., parseDocx, parseXlsx, parseOfficeDocument or
whichever functions handle the ArrayBuffer) so network and CPU work are aborted
when the query is cancelled; ensure you keep the same error handling for non-ok
responses and handle AbortError appropriately if needed.
In `@services/platform/app/features/products/components/product-view-dialog.tsx`:
- Around line 119-120: product.tags may contain duplicate strings so using
key={tag} on the Badge within the map (product.tags.map(...)) can produce
duplicate React keys; update the rendering to guarantee unique keys by either
deduplicating product.tags before mapping (e.g., create a Set or use
Array.from(new Set(product.tags)) and iterate that) or keep duplicates visually
but use a composite key such as `${tag}-${index}` (use the map index alongside
the tag) when rendering Badge to ensure each key is unique and avoid
reconciliation warnings.
---
Outside diff comments:
In
`@services/platform/app/features/automations/components/automation-assistant/thinking-animation.tsx`:
- Around line 29-64: The exit animations on the motion elements inside
ThinkingAnimation (the m.div and m.span using exit props) won't run unless the
component is wrapped in Framer Motion's AnimatePresence; update the rendering so
the conditional mount/unmount of ThinkingAnimation is wrapped by
<AnimatePresence>—either wrap the conditional ThinkingAnimation itself or wrap
the parent MessageList (or AutomationAssistantContent where MessageList is
rendered) with AnimatePresence—so the exit animations on ThinkingAnimation will
be triggered on unmount.
In
`@services/platform/app/features/automations/components/automation-sidepanel.tsx`:
- Around line 392-403: The separator div is focusable (tabIndex={0}) and has
role="separator" but no keyboard handling; update the resizable logic in
useResizable to expose an onKeyDown handler (or add an onKeyDown inline) that
listens for ArrowLeft/ArrowRight (and optionally ArrowUp/ArrowDown) to call the
same resize routine used by handleMouseDown/handleDrag, and attach that handler
to the separator element (the div using handleMouseDown). Alternatively, if you
do not want keyboard resize support, remove tabIndex={0} and role="separator" to
avoid implying interactivity; prefer adding the onKeyDown in useResizable and
reusing existing resize functions so keyboard and pointer resizing remain
consistent.
In
`@services/platform/app/features/documents/components/document-preview-pdf.tsx`:
- Around line 110-175: The async renderPageRef.current reads
state.pageNumPending from a stale closure which can drop pending render
requests; instead, stop consuming pending inside renderPageRef.current and add
an effect that watches state.pageRendering and state.pageNumPending: when
pageRendering is false and pageNumPending !== null, read pending from the
reducer state, dispatch({ type: 'CONSUME_PENDING' }), and call
renderPageRef.current(pending). Remove or avoid using state.pageNumPending
inside renderPageRef.current to ensure the reducer state is the single source of
truth; reference renderPageRef.current, state.pageNumPending,
state.pageRendering, dispatch and the CONSUME_PENDING action when making the
change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (32)
.agents/react-doctor/AGENTS.md.agents/react-doctor/SKILL.mdservices/platform/app/components/layout/sticky-header.tsxservices/platform/app/components/theme/theme-color-meta.tsxservices/platform/app/components/ui/data-display/image.tsxservices/platform/app/components/ui/data-table/data-table-filters.tsxservices/platform/app/components/ui/data-table/data-table.stories.tsxservices/platform/app/components/ui/forms/json-input.tsxservices/platform/app/features/automations/components/automation-assistant/collapsible-message.tsxservices/platform/app/features/automations/components/automation-assistant/thinking-animation.tsxservices/platform/app/features/automations/components/automation-edge.tsxservices/platform/app/features/automations/components/automation-sidepanel.tsxservices/platform/app/features/automations/components/step-create-dialog.tsxservices/platform/app/features/automations/triggers/components/collapsible-section.tsxservices/platform/app/features/chat/components/chat-header.tsxservices/platform/app/features/chat/components/chat-interface.tsxservices/platform/app/features/chat/components/message-bubble.tsxservices/platform/app/features/chat/components/thinking-animation.tsxservices/platform/app/features/custom-agents/components/test-chat-panel/test-message-list.tsxservices/platform/app/features/documents/components/breadcrumb-navigation.tsxservices/platform/app/features/documents/components/document-preview-docx.tsxservices/platform/app/features/documents/components/document-preview-pdf.tsxservices/platform/app/features/documents/components/document-preview-text.tsxservices/platform/app/features/documents/components/document-preview-xlsx.tsxservices/platform/app/features/documents/components/document-team-tags-dialog.tsxservices/platform/app/features/documents/hooks/use-document-preview.tsservices/platform/app/features/products/components/product-view-dialog.tsxservices/platform/app/features/settings/branding/components/branding-form.tsxservices/platform/app/features/settings/branding/components/image-upload-field.tsxservices/platform/app/features/settings/integrations/components/integration-details.tsxservices/platform/app/routes/__root.tsxservices/platform/app/routes/dashboard/$id/chat.tsx
| if (!hasMounted.current) { | ||
| hasMounted.current = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n services/platform/app/components/theme/theme-color-meta.tsxRepository: tale-project/tale
Length of output: 1149
🏁 Script executed:
find services/platform -name "*theme-provider*" -type fRepository: tale-project/tale
Length of output: 118
🏁 Script executed:
cat -n services/platform/app/components/theme/theme-provider.tsxRepository: tale-project/tale
Length of output: 3954
Remove first-render guard to ensure theme-color meta tag is set on initial mount
The guard on lines 17–20 prevents the meta tag update from running on the component's first mount. Since resolvedTheme is always initialized to a value ('light') and the effect only re-runs if it changes, the meta tag may never be set if the theme doesn't change after mount.
Remove the hasMounted guard entirely:
Suggested fix
-import { useEffect, useRef } from 'react';
+import { useEffect } from 'react';
...
- const hasMounted = useRef(false);
-
useEffect(() => {
- if (!hasMounted.current) {
- hasMounted.current = true;
- return;
- }
-
const color =
resolvedTheme === 'dark' ? THEME_COLORS.dark : THEME_COLORS.light;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/theme/theme-color-meta.tsx` around lines 17
- 20, Remove the first-render guard so the effect that updates the theme-color
meta tag runs on initial mount: in the ThemeColorMeta component (remove the
hasMounted ref usage and the early return block that checks hasMounted.current),
let the useEffect that reads resolvedTheme execute on mount and when
resolvedTheme changes, and delete the hasMounted variable if it becomes unused.
| const [hasError, setHasError] = useState(false); | ||
|
|
||
| // Reset state when src changes | ||
| useEffect(() => { | ||
| setCurrentSrc(src || fallbackSrc); | ||
| setHasError(false); | ||
| }, [src, fallbackSrc]); | ||
| const currentSrc = hasError ? fallbackSrc : src || fallbackSrc; | ||
|
|
||
| const handleError = () => { | ||
| if (!hasError && currentSrc !== fallbackSrc) { | ||
| if (!hasError) { | ||
| setHasError(true); | ||
| setCurrentSrc(fallbackSrc); | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| <img | ||
| key={src} | ||
| ref={ref} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n services/platform/app/components/ui/data-display/image.tsx | head -80Repository: tale-project/tale
Length of output: 1889
🏁 Script executed:
# Check if there's any useEffect in the component or a previous version with reset logic
git log --oneline -n 20 services/platform/app/components/ui/data-display/image.tsxRepository: tale-project/tale
Length of output: 128
🏁 Script executed:
# Show the diff for that specific commit on the image.tsx file
git show b93a5b23 -- services/platform/app/components/ui/data-display/image.tsxRepository: tale-project/tale
Length of output: 2606
hasError permanently locks the component to fallback when src changes.
After any load error, hasError becomes true and is never reset. When the src prop changes to a new URL, currentSrc still evaluates to fallbackSrc (line 42) because hasError remains true. The key={src} on the <img> element remounts the DOM node but does not reset component state, so new URLs are never attempted.
This is a regression from the intended refactoring to replace derived-state useEffects with render-time sync. The fix should track which source URL failed instead of using a global error flag:
Proposed fix
- const [hasError, setHasError] = useState(false);
- const currentSrc = hasError ? fallbackSrc : src || fallbackSrc;
+ const normalizedSrc = src || fallbackSrc;
+ const [failedSrc, setFailedSrc] = useState<string | null>(null);
+ const currentSrc = failedSrc === normalizedSrc ? fallbackSrc : normalizedSrc;
const handleError = () => {
- if (!hasError) {
- setHasError(true);
- }
+ if (normalizedSrc !== fallbackSrc) setFailedSrc(normalizedSrc);
};📝 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.
| const [hasError, setHasError] = useState(false); | |
| // Reset state when src changes | |
| useEffect(() => { | |
| setCurrentSrc(src || fallbackSrc); | |
| setHasError(false); | |
| }, [src, fallbackSrc]); | |
| const currentSrc = hasError ? fallbackSrc : src || fallbackSrc; | |
| const handleError = () => { | |
| if (!hasError && currentSrc !== fallbackSrc) { | |
| if (!hasError) { | |
| setHasError(true); | |
| setCurrentSrc(fallbackSrc); | |
| } | |
| }; | |
| return ( | |
| <img | |
| key={src} | |
| ref={ref} | |
| const normalizedSrc = src || fallbackSrc; | |
| const [failedSrc, setFailedSrc] = useState<string | null>(null); | |
| const currentSrc = failedSrc === normalizedSrc ? fallbackSrc : normalizedSrc; | |
| const handleError = () => { | |
| if (normalizedSrc !== fallbackSrc) setFailedSrc(normalizedSrc); | |
| }; | |
| return ( | |
| <img | |
| key={src} | |
| ref={ref} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/data-display/image.tsx` around lines 41 -
53, The component currently uses hasError/setHasError so any load error
permanently forces fallbackSrc even after src changes; replace this with
tracking the specific failed URL: add state failedSrc (and setFailedSrc),
compute currentSrc as (failedSrc === src ? fallbackSrc : src || fallbackSrc),
update handleError to call setFailedSrc(src), and add a useEffect that clears
failedSrc (setFailedSrc(null)) whenever src changes so new URLs are retried;
keep key={src} if you still want remounting but do not rely on it to reset
component state.
| onEdit, | ||
| }: JsonViewerDisplayProps) { | ||
| return ( | ||
| <div className="p-3" role="region" aria-describedby={describedBy}> |
There was a problem hiding this comment.
role="region" needs an accessible name (or should be removed).
Line 140 adds a landmark region without aria-label/aria-labelledby, creating an unlabeled landmark for assistive tech.
💡 Proposed fix (remove redundant role)
- <div className="p-3" role="region" aria-describedby={describedBy}>
+ <div className="p-3" aria-describedby={describedBy}>📝 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.
| <div className="p-3" role="region" aria-describedby={describedBy}> | |
| <div className="p-3" aria-describedby={describedBy}> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/forms/json-input.tsx` at line 140, The
div element that currently sets role="region" (the element with className "p-3"
and aria-describedby={describedBy}) creates an unlabeled landmark; either remove
the redundant role attribute or supply an accessible name by adding aria-label
or aria-labelledby that points to an existing heading/label ID. Update the div
in json-input.tsx so it no longer uses role="region" without a label, or add a
clear accessible name (aria-label/aria-labelledby) tied to a visible label
element.
| rows={rows} | ||
| id={inputId} | ||
| aria-describedby={describedBy} | ||
| className={cn( | ||
| 'w-full resize-none border-0 bg-transparent p-3 text-xs focus:outline-none focus:ring-0 h-[12.5rem] overflow-y-auto', | ||
| 'font-mono leading-relaxed', | ||
| 'placeholder:text-muted-foreground', |
There was a problem hiding this comment.
rows is currently overridden by fixed height styling.
Line 197 passes rows, but Line 201 forces h-[12.5rem], so consumer-provided row sizing is effectively ignored.
💡 Proposed fix
- 'w-full resize-none border-0 bg-transparent p-3 text-xs focus:outline-none focus:ring-0 h-[12.5rem] overflow-y-auto',
+ 'w-full resize-none border-0 bg-transparent p-3 text-xs focus:outline-none focus:ring-0 overflow-y-auto',📝 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.
| rows={rows} | |
| id={inputId} | |
| aria-describedby={describedBy} | |
| className={cn( | |
| 'w-full resize-none border-0 bg-transparent p-3 text-xs focus:outline-none focus:ring-0 h-[12.5rem] overflow-y-auto', | |
| 'font-mono leading-relaxed', | |
| 'placeholder:text-muted-foreground', | |
| rows={rows} | |
| id={inputId} | |
| aria-describedby={describedBy} | |
| className={cn( | |
| 'w-full resize-none border-0 bg-transparent p-3 text-xs focus:outline-none focus:ring-0 overflow-y-auto', | |
| 'font-mono leading-relaxed', | |
| 'placeholder:text-muted-foreground', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/forms/json-input.tsx` around lines 197 -
203, The component's textarea (json-input.tsx) currently accepts the rows prop
but then forces a fixed height via the h-[12.5rem] utility in the className,
effectively ignoring rows; update the rendering in the JSON input component
(where rows and className are applied) to stop forcing a fixed height—either
remove h-[12.5rem] or make height conditional (e.g., use min-h or apply h only
when rows is undefined) so consumer-provided rows take effect; ensure the
textarea still keeps desired styling (overflow-y-auto, resize-none) while
respecting the rows prop.
| const [validation, setValidation] = useState<ValidationState>({ | ||
| isValid: true, | ||
| error: '', | ||
| }); | ||
|
|
||
| // Validate JSON and schema | ||
| const validateJson = useCallback( | ||
| (jsonString: string) => { | ||
| if (!jsonString.trim()) { | ||
| setIsValid(true); | ||
| setError(''); | ||
| return true; | ||
| } | ||
| const prevValueRef = useRef(value); | ||
| const shakeRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
| const containerRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| try { | ||
| const parsed = JSON.parse(jsonString); | ||
|
|
||
| // If schema is provided, validate against it | ||
| if (schema) { | ||
| try { | ||
| schema.parse(parsed); | ||
| setIsValid(true); | ||
| setError(''); | ||
| return true; | ||
| } catch (err) { | ||
| if (err instanceof z.ZodError) { | ||
| const zodError = err; | ||
| const validationError = zodError.issues | ||
| .map((e) => `${e.path.join('.')}: ${e.message}`) | ||
| .join(', '); | ||
| setIsValid(false); | ||
| setError( | ||
| t('validation.schemaValidationFailed', { | ||
| error: validationError, | ||
| }), | ||
| ); | ||
| } else { | ||
| setIsValid(false); | ||
| setError(t('validation.schemaValidationFailed', { error: '' })); | ||
| } | ||
| return false; | ||
| } | ||
| } else { | ||
| setIsValid(true); | ||
| setError(''); | ||
| return true; | ||
| } | ||
| } catch (err) { | ||
| setIsValid(false); | ||
| setError( | ||
| err instanceof Error ? err.message : t('validation.invalidJson'), | ||
| ); | ||
| return false; | ||
| } | ||
| if (prevValueRef.current !== value) { | ||
| prevValueRef.current = value; | ||
| try { | ||
| const parsed = value.trim() ? JSON.parse(value) : {}; | ||
| setParsedValue(parsed); | ||
| } catch { | ||
| setParsedValue({}); | ||
| } | ||
| setTextValue(value); | ||
| setEditing({ isEditing: false, isDirty: false }); | ||
| setValidation(computeValidation(value, schema, t)); | ||
| } |
There was a problem hiding this comment.
Initialize and reset validation from real content instead of forcing a valid state.
Line 313 initializes validation as valid, and recomputation only happens when value changes (Line 322), so initial invalid JSON/schema state is silently accepted. Line 398 also clears validation unconditionally on cancel, which can hide schema errors for the restored value.
💡 Proposed fix
- const [validation, setValidation] = useState<ValidationState>({
- isValid: true,
- error: '',
- });
+ const [validation, setValidation] = useState<ValidationState>(() =>
+ computeValidation(value, schema, t),
+ );
...
const handleCancel = () => {
- setTextValue(JSON.stringify(parsedValue, null, 2));
+ const resetValue = JSON.stringify(parsedValue, null, 2);
+ setTextValue(resetValue);
setEditing({ isEditing: false, isDirty: false });
- setValidation({ isValid: true, error: '' });
+ setValidation(computeValidation(resetValue, schema, t));
};Also applies to: 395-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/forms/json-input.tsx` around lines 313 -
333, The validation state is being initialized to a always-valid value and only
recomputed when `value` changes via the inline `prevValueRef` check, and the
cancel handler unconditionally clears validation; update initialization and
cancel logic to derive validation from real content using
`computeValidation(value, schema, t)` instead of forcing `isValid: true`.
Specifically, set the initial `validation` state by calling
`computeValidation(value, schema, t)` (or run that computation in a `useEffect`
that mirrors the existing `prevValueRef` logic), and change the
cancel/edit-reset code (the block that currently calls `setValidation` around
lines 395–399) to recompute and restore validation for the restored `value`
using `computeValidation` rather than clearing it. Ensure you reference and
update `validation`, `setValidation`, `computeValidation`, `prevValueRef`, and
the cancel/edit reset handler so initial and restored values reflect real
JSON/schema validation.
| isValidating={isValidating} | ||
| errors={errors} | ||
| warnings={warnings} | ||
| stepOptions={stepOptions ?? EMPTY_STEP_OPTIONS} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant nullish coalescing — default parameter already handles this.
stepOptions is defaulted to EMPTY_STEP_OPTIONS at Line 263 via the destructuring default, so it can never be undefined here. The ?? EMPTY_STEP_OPTIONS is dead code.
🧹 Proposed fix
- stepOptions={stepOptions ?? EMPTY_STEP_OPTIONS}
+ stepOptions={stepOptions}📝 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.
| stepOptions={stepOptions ?? EMPTY_STEP_OPTIONS} | |
| stepOptions={stepOptions} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/automations/components/automation-sidepanel.tsx`
at line 518, The prop usage redundantly null-coalesces stepOptions even though
it already has a default of EMPTY_STEP_OPTIONS in the component props
destructuring; remove the dead code by changing the JSX prop from
stepOptions={stepOptions ?? EMPTY_STEP_OPTIONS} to simply
stepOptions={stepOptions}, locating the usage in automation-sidepanel.tsx and
keeping the existing EMPTY_STEP_OPTIONS default in the component destructuring.
| const urlRef = useRef(url); | ||
| urlRef.current = url; | ||
|
|
||
| useEffect(() => { | ||
| const script = document.createElement('script'); | ||
| script.src = | ||
| 'https://cdnjs.cloudflare.com/ajax/libs/pdf.js/3.11.174/pdf.min.js'; | ||
| script.addEventListener('load', () => { | ||
| if ('pdfjsLib' in window) { | ||
| // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- pdfjsLib is injected by the CDN script loaded above | ||
| const lib = (window as unknown as { pdfjsLib: PdfJsLib }).pdfjsLib; | ||
| lib.GlobalWorkerOptions.workerSrc = | ||
| 'https://cdnjs.cloudflare.com/ajax/libs/pdf.js/3.11.174/pdf.worker.min.js'; | ||
| const loadingTask = lib.getDocument(urlRef.current); | ||
| loadingTask.promise | ||
| .then((doc: PDFDocumentProxy) => { | ||
| dispatch({ type: 'PDF_LOADED', doc }); | ||
| void renderPageRef.current?.({ | ||
| pageNum: 1, | ||
| scale: initialState.scale, | ||
| }); | ||
| }) | ||
| .catch((error: unknown) => { | ||
| console.error('Error loading PDF:', error); | ||
| }); | ||
| } | ||
| }); | ||
| document.head.appendChild(script); | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file
find . -type f -name "document-preview-pdf.tsx" 2>/dev/nullRepository: tale-project/tale
Length of output: 139
🏁 Script executed:
# Check file existence and get total lines
wc -l services/platform/app/features/documents/components/document-preview-pdf.tsx 2>/dev/null || echo "File not found in expected location"Repository: tale-project/tale
Length of output: 141
🏁 Script executed:
# Search for the file more broadly if needed
find . -type f -name "*document-preview*pdf*" 2>/dev/null | head -20Repository: tale-project/tale
Length of output: 139
🏁 Script executed:
# Read the critical section plus surrounding context
sed -n '180,240p' services/platform/app/features/documents/components/document-preview-pdf.tsxRepository: tale-project/tale
Length of output: 1910
🏁 Script executed:
# Get more context - check component structure, props, and state
sed -n '1,100p' services/platform/app/features/documents/components/document-preview-pdf.tsxRepository: tale-project/tale
Length of output: 2482
🏁 Script executed:
# Check if there are other useEffects and how the component handles cleanup
sed -n '220,280p' services/platform/app/features/documents/components/document-preview-pdf.tsxRepository: tale-project/tale
Length of output: 1905
🏁 Script executed:
# Check the component props/interface to see how `url` is used
sed -n '100,150p' services/platform/app/features/documents/components/document-preview-pdf.tsxRepository: tale-project/tale
Length of output: 1763
🏁 Script executed:
# Also check if there's a cleanup pattern elsewhere or if useEffect dependencies are typically managed
grep -n "useEffect" services/platform/app/features/documents/components/document-preview-pdf.tsxRepository: tale-project/tale
Length of output: 185
🏁 Script executed:
# Check if there's any other effect or mechanism that triggers initial render after PDF_LOADED
sed -n '224,280p' services/platform/app/features/documents/components/document-preview-pdf.tsx | head -40Repository: tale-project/tale
Length of output: 1235
🏁 Script executed:
# Verify if there are any listeners on renderPageRef or effects that depend on pdfDoc state
rg -n "pdfDoc|renderPageRef" services/platform/app/features/documents/components/document-preview-pdf.tsx | head -30Repository: tale-project/tale
Length of output: 566
🏁 Script executed:
# Let me check if there's any effect that triggers when pdfDoc or url changes
sed -n '280,350p' services/platform/app/features/documents/components/document-preview-pdf.tsxRepository: tale-project/tale
Length of output: 2852
🏁 Script executed:
# Also verify if url is used anywhere else in the component
rg -n "url" services/platform/app/features/documents/components/document-preview-pdf.tsxRepository: tale-project/tale
Length of output: 316
PDF loader effect is not URL-reactive and lacks proper cleanup.
The effect uses an empty dependency array with urlRef to access the current URL. When the url prop changes, the effect won't re-run to load the new document. Additionally, the script tag and load listener are never removed, causing potential listener accumulation on component remount.
The proposed fix's approach is sound—making the effect reactive to url changes and adding cleanup—but it removes the initial page render call that occurs in the .then() callback. Without an additional effect watching state.pdfDoc to trigger initial render, removing that call breaks the initial display. Either restore the initial render in the then() block or add a separate effect watching state.pdfDoc.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/documents/components/document-preview-pdf.tsx`
around lines 194 - 223, The PDF loader effect in document-preview-pdf.tsx should
be made reactive to url changes and cleaned up properly: change the effect that
creates the script (the useEffect that currently has an empty dependency array
and uses urlRef) to depend on url, remove the created script and its 'load'
listener in the effect cleanup, and revoke any loadingTask if present to avoid
leaked listeners; also ensure the initial page is rendered when a document is
loaded—either call renderPageRef.current({ pageNum: 1, scale: initialState.scale
}) inside the loadingTask.promise.then block after dispatch({ type:
'PDF_LOADED', doc }) or, if you prefer to keep the render call elsewhere, add a
separate effect that watches state.pdfDoc and invokes renderPageRef.current for
the initial render when state.pdfDoc becomes available.
| .then((doc: PDFDocumentProxy) => { | ||
| dispatch({ type: 'PDF_LOADED', doc }); | ||
| void renderPageRef.current?.({ | ||
| pageNum: 1, | ||
| scale: initialState.scale, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's get the file and check its size
wc -l services/platform/app/features/documents/components/document-preview-pdf.tsxRepository: tale-project/tale
Length of output: 141
🏁 Script executed:
# Read the critical sections: around line 209-214 for the issue, and line 114 for the renderPageRef early-return
sed -n '200,225p' services/platform/app/features/documents/components/document-preview-pdf.tsxRepository: tale-project/tale
Length of output: 1129
🏁 Script executed:
# Also check around line 114 to see the early-return mentioned
sed -n '110,130p' services/platform/app/features/documents/components/document-preview-pdf.tsxRepository: tale-project/tale
Length of output: 874
🏁 Script executed:
# Let's also look for the renderPageRef definition and how it's used
rg -n 'renderPageRef' services/platform/app/features/documents/components/document-preview-pdf.tsx -B 2 -A 2Repository: tale-project/tale
Length of output: 944
🏁 Script executed:
# Check the reducer definition to understand if dispatch is synchronous
rg -n 'useReducer\|const.*reducer' services/platform/app/features/documents/components/document-preview-pdf.tsx -B 2 -A 5Repository: tale-project/tale
Length of output: 43
🏁 Script executed:
# Look for the state usage in renderPageRef callback
sed -n '1,120p' services/platform/app/features/documents/components/document-preview-pdf.tsx | head -120Repository: tale-project/tale
Length of output: 3227
Remove inline render call and defer to state update via useEffect.
The initial page render is skipped because renderPageRef.current is invoked immediately after dispatch, before the state update completes. The closure captures stale state.pdfDoc === null, triggering the early-return guard at line 114.
🔧 Proposed fix
loadingTask.promise
.then((doc: PDFDocumentProxy) => {
dispatch({ type: 'PDF_LOADED', doc });
- void renderPageRef.current?.({
- pageNum: 1,
- scale: initialState.scale,
- });
})
.catch((error: unknown) => {
console.error('Error loading PDF:', error);
});// Add after existing effects
useEffect(() => {
if (!state.pdfDoc) return;
void renderPageRef.current?.({ pageNum: 1, scale: initialState.scale });
}, [state.pdfDoc]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/documents/components/document-preview-pdf.tsx`
around lines 209 - 214, The inline call to renderPageRef.current inside the PDF
load promise should be removed because it runs before the state update from
dispatch({type: 'PDF_LOADED', doc}) completes; instead add a useEffect that
watches state.pdfDoc and, when it becomes non-null, calls
renderPageRef.current({ pageNum: 1, scale: initialState.scale }); specifically:
delete the immediate invocation in the .then callback that follows dispatch, add
a useEffect that returns early if !state.pdfDoc and otherwise calls
renderPageRef.current with pageNum 1 and initialState.scale; reference
renderPageRef, state.pdfDoc, dispatch('PDF_LOADED'), and initialState.scale when
applying the change.
| queryFn: async () => { | ||
| const res = await fetch(url); | ||
| if (!res.ok) throw new Error(`Failed to fetch document (${res.status})`); | ||
| const ab = await res.arrayBuffer(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "use-document-preview.ts" | head -20Repository: tale-project/tale
Length of output: 133
🏁 Script executed:
cat -n ./services/platform/app/features/documents/hooks/use-document-preview.tsRepository: tale-project/tale
Length of output: 2787
Propagate React Query AbortSignal for DOCX/XLSX fetches.
Lines 10-11 and 25-26 ignore cancellation, while Line 65 already implements it correctly for text previews. This allows large downloads and expensive parsing operations to continue running after component unmount or query-key changes, wasting bandwidth and CPU resources.
🔧 Proposed fix
export function useDocxPreview(url: string) {
return useReactQuery({
queryKey: ['docx-preview', url],
- queryFn: async () => {
- const res = await fetch(url);
+ queryFn: async ({ signal }) => {
+ const res = await fetch(url, { signal });
if (!res.ok) throw new Error(`Failed to fetch document (${res.status})`);
const ab = await res.arrayBuffer(); export function useXlsxPreview(url: string) {
return useReactQuery({
queryKey: ['xlsx-preview', url],
- queryFn: async () => {
- const res = await fetch(url);
+ queryFn: async ({ signal }) => {
+ const res = await fetch(url, { signal });
if (!res.ok)
throw new Error(`Failed to fetch spreadsheet (${res.status})`);📝 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.
| queryFn: async () => { | |
| const res = await fetch(url); | |
| if (!res.ok) throw new Error(`Failed to fetch document (${res.status})`); | |
| const ab = await res.arrayBuffer(); | |
| queryFn: async ({ signal }) => { | |
| const res = await fetch(url, { signal }); | |
| if (!res.ok) throw new Error(`Failed to fetch document (${res.status})`); | |
| const ab = await res.arrayBuffer(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/documents/hooks/use-document-preview.ts`
around lines 10 - 13, The queryFn in use-document-preview.ts currently ignores
React Query's AbortSignal causing DOCX/XLSX fetches and parsing to continue
after cancellation; update the queryFn signature to accept the query context ({
signal }) and pass that signal into fetch (fetch(url, { signal })) and into any
downstream parsing calls (e.g., parseDocx, parseXlsx, parseOfficeDocument or
whichever functions handle the ArrayBuffer) so network and CPU work are aborted
when the query is cancelled; ensure you keep the same error handling for non-ok
responses and handle AbortError appropriately if needed.
| {product.tags.map((tag) => ( | ||
| <Badge key={tag} variant="outline"> |
There was a problem hiding this comment.
Duplicate tags would produce duplicate keys — consider deduplicating or using a composite key.
product.tags is typed as string[], which permits duplicates. If two tags share the same value, key={tag} generates duplicate keys, triggering React reconciliation warnings and potential rendering bugs.
Either deduplicate before mapping or use a composite key that preserves uniqueness:
🛡️ Option A — deduplicate at render time
- {product.tags.map((tag) => (
- <Badge key={tag} variant="outline">
+ {[...new Set(product.tags)].map((tag) => (
+ <Badge key={tag} variant="outline">🛡️ Option B — composite key (retains duplicates visually, avoids key collision)
- {product.tags.map((tag) => (
- <Badge key={tag} variant="outline">
+ {product.tags.map((tag, index) => (
+ <Badge key={`${tag}-${index}`} variant="outline">📝 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.
| {product.tags.map((tag) => ( | |
| <Badge key={tag} variant="outline"> | |
| {[...new Set(product.tags)].map((tag) => ( | |
| <Badge key={tag} variant="outline"> |
| {product.tags.map((tag) => ( | |
| <Badge key={tag} variant="outline"> | |
| {product.tags.map((tag, index) => ( | |
| <Badge key={`${tag}-${index}`} variant="outline"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/products/components/product-view-dialog.tsx`
around lines 119 - 120, product.tags may contain duplicate strings so using
key={tag} on the Badge within the map (product.tags.map(...)) can produce
duplicate React keys; update the rendering to guarantee unique keys by either
deduplicating product.tags before mapping (e.g., create a Set or use
Array.from(new Set(product.tags)) and iterate that) or keep duplicates visually
but use a composite key such as `${tag}-${index}` (use the map index alongside
the tag) when rendering Badge to ensure each key is unique and avoid
reconciliation warnings.
Resolve 6 errors and 128 warnings flagged by react-doctor:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor