refactor(platform): improve UI components and extract feature subcomponents#480
Merged
Israeltheminer merged 4 commits intoFeb 16, 2026
Merged
Conversation
Add comprehensive Storybook stories for data-table, entity, and form components. Add unit tests for search-input, file-upload, and shared UI components. Improve date-range-picker, file-upload, json-input, and search-input with better accessibility and enhanced functionality.
Break down monolithic components into focused subcomponents with dedicated files, extracted hooks, and shared types. Affects approvals, automations, chat, conversations, custom-agents, documents, and settings/integrations features.
…pages Simplify data-table component, enhance customer/product/vendor table skeletons and import forms, update knowledge page routes, and add empty state translations.
Israeltheminer
force-pushed
the
refactor/platform-ui-and-component-extraction
branch
from
e365407 to
6c0f09e
Compare
📝 WalkthroughWalkthroughThis PR introduces a large-scale refactoring across multiple features to adopt a modular architecture. Major changes include extracting inline UI and state management from monolithic components into dedicated hooks (useApprovalColumns, useAssistantChat, useTestChat, useBulkActions, useIntegrationManage, useSsoConfigForm) and new subcomponents. Form components receive new accessibility and UX features (label, description, errorMessage, required props). Chat and messaging features are restructured with better separation of concerns (new file display, markdown rendering, and image preview components). Approvals, conversations, automations, custom-agents, and integration management features are refactored to delegate orchestration to hooks and composed subcomponents. Storybook stories are added for multiple UI components, and comprehensive test suites cover newly modularized components. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR implements a comprehensive refactoring that improves code maintainability and component quality across the platform service. The changes break down large monolithic components into focused, reusable pieces while enhancing UI components with better accessibility and error handling. Key Improvements:
Architecture Pattern: Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| services/platform/app/components/ui/forms/search-input.tsx | Enhanced with label, description, error handling, accessibility features, and shake animation |
| services/platform/app/components/ui/forms/file-upload.tsx | Refactored to compound component pattern with Root, DropZone, Overlay, added accessibility and error handling |
| services/platform/app/components/ui/data-table/data-table.tsx | Simplified component with improved error boundary integration and better pagination handling |
| services/platform/app/features/automations/hooks/use-assistant-chat.ts | Extracted chat logic from component with duplicate send prevention and optimistic updates |
| services/platform/app/features/automations/components/automation-assistant.tsx | Reduced from 900+ lines to minimal orchestration component by extracting subcomponents and hooks |
| services/platform/app/features/conversations/hooks/use-bulk-actions.ts | Extracted bulk operations logic (resolve, reopen, send) into dedicated hook |
| services/platform/app/features/conversations/components/conversations-client.tsx | Simplified from 500+ lines to focused orchestration by extracting hooks and subcomponents |
| services/platform/app/features/settings/integrations/components/integration-manage-dialog.tsx | Reduced from 1600+ lines to orchestration component by extracting subcomponents and hooks |
| services/platform/app/features/custom-agents/components/test-chat-panel.tsx | Reduced from 800+ lines by extracting subcomponents (input, message list, thinking dots) |
| services/platform/app/features/documents/components/onedrive-import-dialog.tsx | Reduced from 1000+ lines by extracting stage components and file/drive tables |
Flowchart
flowchart TD
A[Large Monolithic Components<br/>900-1600+ lines] --> B[Extract Custom Hooks]
A --> C[Extract Subcomponents]
A --> D[Extract Shared Types]
B --> E[use-assistant-chat.ts<br/>use-bulk-actions.ts<br/>use-conversation-selection.ts<br/>use-integration-manage.ts]
C --> F[Message List Components<br/>Input Components<br/>Dialog Stages<br/>Action Bars]
D --> G[types.ts files<br/>selection.ts]
E --> H[Slim Orchestration Component<br/>50-200 lines]
F --> H
G --> H
H --> I[Improved Maintainability<br/>Better Testability<br/>Code Reusability]
J[UI Form Components] --> K[Add Accessibility]
J --> L[Add Error Handling]
J --> M[Add Label/Description]
K --> N[Enhanced Components<br/>search-input<br/>file-upload<br/>date-range-picker<br/>json-input]
L --> N
M --> N
N --> O[Comprehensive Tests<br/>Storybook Stories]
Last reviewed commit: 6c0f09e
Add missing imports for describe, it, expect, vi, afterEach, and beforeEach from vitest. Use @/test/utils/render instead of importing directly from @testing-library/react, matching the project convention.
Israeltheminer
deleted the
refactor/platform-ui-and-component-extraction
branch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 52
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
services/platform/app/features/conversations/components/message-editor.tsx (1)
289-346: 🧹 Nitpick | 🔵 TrivialConsider extracting inline styles to a separate stylesheet.
The inline
<style>block is large and static. Extracting it to a CSS file (e.g.,message-editor.css) would improve readability and allow better caching. However, this is a stylistic preference and the current approach works correctly.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@services/platform/app/features/conversations/components/message-editor.tsx` around lines 289 - 346, Extract the large inline <style> block from message-editor.tsx into a dedicated CSS file (e.g., message-editor.css) and import it in the MessageEditor component; move all rules under the .milkdown, .milkdown .editor, and .milkdown .ProseMirror selectors into that stylesheet, remove the inline <style> tag from the JSX, and ensure any CSS custom properties and class names used by the editor remain unchanged so styles continue to apply at runtime.services/platform/app/components/ui/forms/date-range-picker.tsx (1)
179-430:⚠️ Potential issue | 🟠 MajorWire label/description/error IDs to the picker control.
LabeluseshtmlFor={id}, but the actual interactive element (custom button) never receives thatidor anyaria-describedby/aria-errormessageattributes. Screen readers won’t associate the label/description/error with the control.✅ Suggested fix
interface CustomInputProps { value?: string; onClick?: () => void; isLoading?: boolean; placeholder?: string; presetLabel?: string; presetOptions: { key: DatePreset; label: string }[]; onPresetSelect: (preset: DatePreset) => void; + id?: string; + describedBy?: string; + errorId?: string; + hasError?: boolean; + required?: boolean; } const CustomInput = forwardRef<HTMLButtonElement, CustomInputProps>( ( { value, onClick, isLoading, placeholder, presetLabel, presetOptions, onPresetSelect, + id, + describedBy, + errorId, + hasError, + required, }, ref, ) => ( <div className="ring-border flex divide-x rounded-lg ring-1"> <Button ref={ref} type="button" variant="secondary" disabled={isLoading} onClick={onClick} + id={id} + aria-invalid={hasError || undefined} + aria-describedby={describedBy} + aria-errormessage={hasError ? errorId : undefined} + aria-required={required || undefined} className={cn( 'w-auto justify-start text-sm text-left font-normal space-x-2 px-2.5 rounded-r-none border-r-0 ring-0', !value && 'text-muted-foreground', isLoading && 'opacity-50 cursor-not-allowed', )} > @@ export function DatePickerWithRange({ @@ const errorId = `${id}-error`; const descriptionId = `${id}-description`; const hasError = !!errorMessage; + const describedBy = + [description && descriptionId, hasError && errorId] + .filter(Boolean) + .join(' ') || undefined; @@ customInput={ <CustomInput isLoading={isLoading} placeholder={t('upload.pickADate')} presetLabel={presetLabel} presetOptions={presetOptions} onPresetSelect={handlePresetSelect} + id={id} + hasError={hasError} + errorId={errorId} + describedBy={describedBy} + required={required} /> }🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@services/platform/app/components/ui/forms/date-range-picker.tsx` around lines 179 - 430, The label/description/error aren’t wired to the interactive control: extend CustomInputProps to accept id, ariaDescribedBy (or aria-describedby), ariaErrormessage (or aria-errormessage) and ariaInvalid (or a boolean hasError), then forward those attributes onto the primary interactive button inside CustomInput (the first Button that opens the datepicker) and optionally the dropdown trigger button; finally, when creating the CustomInput in DatePickerWithRange (the ReactDatePicker customInput prop) pass id, ariaDescribedBy: descriptionId, ariaErrormessage: errorId, and ariaInvalid: hasError so screen readers can associate Label, Description and Error with the control (use the existing id/descriptionId/errorId/hasError symbols).services/platform/app/features/documents/components/onedrive-import-dialog.tsx (1)
138-209: 🧹 Nitpick | 🔵 TrivialConsider extracting
collectAllFilesoutside the component or memoizing it.This async function is recreated on every render. While it captures several state values (
sourceTab,selectedSite,selectedDrive), the function references stable mutation functions. Consider wrapping withuseCallbackor extracting to a module-level helper that accepts dependencies as parameters to improve performance and prevent potential stale closure issues during long-running recursive operations.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@services/platform/app/features/documents/components/onedrive-import-dialog.tsx` around lines 138 - 209, The recursive async function collectAllFiles is recreated on every render and closes over component state (sourceTab, selectedSite, selectedDrive), risking stale closures and wasted work; fix by either extracting collectAllFiles to a module-level helper that accepts the dynamic deps (sourceTab, selectedSite, selectedDrive, listSharePointFiles, listOneDriveFiles, toast, t) as parameters, or wrap it in useCallback inside the component with an explicit dependency array ([sourceTab, selectedSite, selectedDrive, listSharePointFiles, listOneDriveFiles, toast, t]) so the function identity is stable during long-running recursion and uses correct values. Ensure any local helpers (e.g., isFile/isFolder) remain accessible or are passed in as arguments if you move the function out.services/platform/app/features/conversations/components/conversations-client.tsx (1)
50-62:⚠️ Potential issue | 🟠 MajorSearch can’t be cleared when
initialSearchis set.
searchQuery || initialSearchre-applies the initial term when the user clears the input, so the list stays filtered. UsesearchQuerydirectly (it’s already initialized frominitialSearch).🐛 Suggested fix
- const searchTerm = searchQuery || initialSearch; + const searchTerm = searchQuery;🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@services/platform/app/features/conversations/components/conversations-client.tsx` around lines 50 - 62, filteredConversations' useMemo uses `searchQuery || initialSearch`, which re-applies the initial term when the user clears the input; change the memo to use `searchQuery` directly (e.g., `const searchTerm = searchQuery`) and remove `initialSearch` from the dependency array so the filter relies solely on the current `searchQuery`; keep the existing call to `filterByTextSearch(paginatedResult.results, searchTerm, ['title','description','subject','externalMessageId'])` and retain `paginatedResult.results` and `searchQuery` as the memo dependencies.services/platform/app/components/ui/forms/file-upload.tsx (1)
38-116:⚠️ Potential issue | 🟠 MajorLabel/error/description aren’t wired to the interactive element.
Root generates IDs and renders Label/Description/Error, but DropZone keeps its own staticinputIdand never appliesaria-describedby/aria-errormessage. This breaks label association, error announcement, and creates duplicate IDs if multiple FileUpload instances render on a page.🛠️ Suggested wiring fix
-interface FileUploadContextValue { - isDragOver: boolean; - setIsDragOver: (value: boolean) => void; -} +interface FileUploadContextValue { + isDragOver: boolean; + setIsDragOver: (value: boolean) => void; + inputId: string; + labelId?: string; + descriptionId?: string; + errorId?: string; + hasError: boolean; +} @@ - const errorId = `${id}-error`; - const descriptionId = `${id}-description`; + const labelId = label ? `${id}-label` : undefined; + const errorId = errorMessage ? `${id}-error` : undefined; + const descriptionId = description ? `${id}-description` : undefined; @@ - const value = useMemo( - () => ({ - isDragOver, - setIsDragOver, - }), - [isDragOver], - ); + const value = useMemo( + () => ({ + isDragOver, + setIsDragOver, + inputId: id, + labelId, + descriptionId, + errorId, + hasError, + }), + [isDragOver, id, labelId, descriptionId, errorId, hasError], + ); @@ - {label && ( - <Label htmlFor={id} required={required} error={hasError}> + {label && ( + <Label id={labelId} htmlFor={id} required={required} error={hasError}> {label} </Label> )} @@ -function DropZone({ ..., inputId = 'file-upload', ... }: DropZoneProps) { - const { setIsDragOver } = useFileUploadContext(); +function DropZone({ ..., inputId, ... }: DropZoneProps) { + const { + setIsDragOver, + inputId: contextInputId, + labelId, + descriptionId, + errorId, + hasError, + } = useFileUploadContext(); + const resolvedInputId = inputId ?? contextInputId; + const ariaDescribedBy = + [descriptionId, hasError ? errorId : undefined].filter(Boolean).join(' ') || + undefined; @@ - aria-label={clickable ? ariaLabel : undefined} + aria-label={clickable ? ariaLabel : undefined} + aria-labelledby={!ariaLabel ? labelId : undefined} + aria-describedby={ariaDescribedBy} + aria-errormessage={hasError ? errorId : undefined} + aria-invalid={hasError || undefined} @@ - id={inputId} + id={resolvedInputId}Also applies to: 119-240
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@services/platform/app/components/ui/forms/file-upload.tsx` around lines 38 - 116, Root generates unique ids (generatedId -> id, errorId, descriptionId) but DropZone uses its static inputId, so Label/Description/Error aren't associated and duplicate IDs occur; update DropZone to accept an id prop (or use the id from FileUpload Root via context) instead of its hardcoded inputId and wire aria-describedby and aria-errormessage on the interactive element to descriptionId and errorId respectively; ensure the label's htmlFor uses the same id (Root -> Label already uses id) and remove any hardcoded duplicate ids in DropZone so multiple FileUpload instances get unique associations (check functions/components: Root, FileUploadContext, DropZone, inputId, errorId, descriptionId, Label, Description).services/platform/app/features/chat/components/message-bubble.tsx (1)
101-113:⚠️ Potential issue | 🟡 MinorUse stable keys for attachments and file parts.
Index keys can cause stale UI when items are inserted/removed; prefer fileId/url for stable identity.🔧 Use stable keys
- {message.attachments.map((attachment, index) => ( - <FileAttachmentDisplay key={index} attachment={attachment} /> - ))} + {message.attachments.map((attachment) => ( + <FileAttachmentDisplay + key={attachment.fileId} + attachment={attachment} + /> + ))} ... - {message.fileParts.map((part, index) => ( - <FilePartDisplay key={index} filePart={part} /> - ))} + {message.fileParts.map((part) => ( + <FilePartDisplay key={part.url} filePart={part} /> + ))}🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@services/platform/app/features/chat/components/message-bubble.tsx` around lines 101 - 113, The map calls in MessageBubble that render FileAttachmentDisplay and FilePartDisplay use index keys (key={index}) which can cause stale UI; update the keys to use a stable unique identifier from each item (e.g., use attachment.fileId || attachment.url for the FileAttachmentDisplay map and part.fileId || part.url for the FilePartDisplay map), falling back to a guaranteed unique property if present, and ensure the components accept those props unchanged (refer to the message.attachments mapping and message.fileParts mapping and the FileAttachmentDisplay/FilePartDisplay usages).
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@services/platform/app/components/ui/forms/date-range-picker.tsx`:
- Around line 179-430: The label/description/error aren’t wired to the
interactive control: extend CustomInputProps to accept id, ariaDescribedBy (or
aria-describedby), ariaErrormessage (or aria-errormessage) and ariaInvalid (or a
boolean hasError), then forward those attributes onto the primary interactive
button inside CustomInput (the first Button that opens the datepicker) and
optionally the dropdown trigger button; finally, when creating the CustomInput
in DatePickerWithRange (the ReactDatePicker customInput prop) pass id,
ariaDescribedBy: descriptionId, ariaErrormessage: errorId, and ariaInvalid:
hasError so screen readers can associate Label, Description and Error with the
control (use the existing id/descriptionId/errorId/hasError symbols).
In `@services/platform/app/components/ui/forms/file-upload.test.tsx`:
- Around line 80-96: The test currently only checks the presence of the input
and never simulates a file selection, so update the test for FileUpload.DropZone
to simulate a user uploading files and assert onFilesSelected was called: create
a mock handler (handleFiles = vi.fn()), find the input element by id
"test-upload", use userEvent.upload(input, [file]) to trigger change, await any
async updates, then expect(handleFiles).toHaveBeenCalledWith(...) or
toHaveBeenCalled(); ensure the test imports and uses userEvent and constructs a
File object to pass into the upload call.
In `@services/platform/app/components/ui/forms/file-upload.tsx`:
- Around line 65-71: The showShake animation can remain true if hasError flips
to false before the timeout; update the useEffect that watches
hasError/errorMessage so the cleanup always clears the pending timer and resets
showShake to false when the effect re-runs or unmounts. Concretely, in the
useEffect containing hasError -> setShowShake(true) use a local timer id, set
the timeout only when hasError is true, and return a cleanup that
clearsTimeout(timer) and calls setShowShake(false) to ensure the shake state is
cleared if the error is removed early (referencing useEffect, hasError,
setShowShake, errorMessage).
- Around line 38-116: Root generates unique ids (generatedId -> id, errorId,
descriptionId) but DropZone uses its static inputId, so Label/Description/Error
aren't associated and duplicate IDs occur; update DropZone to accept an id prop
(or use the id from FileUpload Root via context) instead of its hardcoded
inputId and wire aria-describedby and aria-errormessage on the interactive
element to descriptionId and errorId respectively; ensure the label's htmlFor
uses the same id (Root -> Label already uses id) and remove any hardcoded
duplicate ids in DropZone so multiple FileUpload instances get unique
associations (check functions/components: Root, FileUploadContext, DropZone,
inputId, errorId, descriptionId, Label, Description).
In `@services/platform/app/components/ui/forms/json-input.tsx`:
- Around line 265-271: The effect on React.useEffect is re-triggering when the
error text (displayError) changes even though hasAnyError stays true; update the
effect to only run when hasAnyError changes by removing displayError from the
dependency list or, better, detect the transition from false→true by tracking
previous hasAnyError (e.g., a ref storing prevHasAnyError) and only call
setShowShake(true)/start the timeout when prevHasAnyError is false and
hasAnyError is true; ensure you still clear the timeout in the cleanup and keep
setShowShake(false) after 400ms (references: React.useEffect, hasAnyError,
displayError, setShowShake).
- Line 361: Replace the non-semantic <div className="p-3" role="region"
aria-describedby={describedBy}> with a semantic <section> and provide an
accessible name: remove role="region", use <section className="p-3"> and add
either aria-labelledby pointing to an existing visible label/id or aria-label
(e.g., aria-labelledby={labelId} or aria-label={labelText}) so screen readers
announce the region; reference the describedBy variable to keep the existing
aria-describedby behavior and update the JsonInput/JsonInputForm component
markup to use section + accessible name.
- Around line 323-331: Replace the wrapper div that uses role="group" with a
semantic <fieldset> to provide native grouping for the form controls: in the
component where the div currently has className={cn(...)} and role="group" (and
uses aria-describedby={describedBy}, hasAnyError, showShake, disabled flags),
switch to a fieldset element, preserve all className logic and aria-describedby,
remove role="group", and map the disabled boolean to the fieldset's disabled
attribute; also ensure default fieldset styles are reset in your CSS/Tailwind
(Preflight) so the visual appearance remains the same.
In `@services/platform/app/features/approvals/components/approvals-client.tsx`:
- Around line 233-240: The dialog closes because selectedApprovalDetail is
derived from the filtered allApprovals; change the lookup to use the unfiltered
paginatedResult.results so the selected item remains available when searches
hide it: inside the useMemo for selectedApprovalDetail, replace allApprovals
with paginatedResult?.results (or an empty array fallback) when calling
.find((a: ApprovalItem) => a._id === selectedApprovalId) and then pass that
approval into getApprovalDetail; also update the hook dependencies from
[selectedApprovalId, allApprovals] to [selectedApprovalId,
paginatedResult?.results] to keep the memo correct.
In
`@services/platform/app/features/approvals/components/approvals-client/get-approval-detail.ts`:
- Around line 20-23: The recommended product name fallback only reads
'productName' (const name = safeGetString(product, 'productName', '')) which can
produce blanks when metadata uses 'name' or 'product_name'; update the name
assignment in get-approval-detail.ts so it mirrors other
renderers/previousPurchases by falling back through keys (e.g., const name =
safeGetString(product, 'productName', '') || safeGetString(product, 'name', '')
|| safeGetString(product, 'product_name', '')); keep using safeGetString and the
same variable name so all consumers of name remain unchanged.
In
`@services/platform/app/features/approvals/components/approvals-client/use-approval-columns.tsx`:
- Around line 225-233: The cell renderer for the 'approval' column currently
hardcodes t('types.recommendProduct') which mislabels resolved approvals;
replace the hardcoded label by calling getApprovalTypeLabel(row.original, t)
inside the cell for the 'approval' column (and the other occurrence around line
338) and ensure getApprovalTypeLabel is imported/available and added to the
hook/component dependency list so the cell updates when its inputs change.
- Around line 64-75: getConfidencePercent currently picks the first entry from
approval.metadata.recommendedProducts without sorting, which can mismatch the
top product shown by RecommendationProductList; update getConfidencePercent (and
its use of ApprovalItem.metadata and recommendedProducts) to compute the
confidence from the highest-confidence recommended product (e.g., take max over
recs' confidence) or reuse the same sorted list used by
RecommendationProductList so the percent always corresponds to the displayed top
recommendation.
In
`@services/platform/app/features/automations/components/automation-assistant/automation-details-collapse.tsx`:
- Around line 19-31: The button in automation-details-collapse.tsx currently
uses a bare <button> with onClick={() => setIsOpen(!isOpen)} which defaults to
type="submit" inside forms; update that button to explicitly set type="button"
to prevent accidental form submissions while preserving the existing onClick
toggle behavior (references: setIsOpen, isOpen, the button element rendering the
ChevronDown/ChevronRight icons).
In
`@services/platform/app/features/automations/components/automation-assistant/chat-input.tsx`:
- Around line 85-92: The image preview rendering uses attachment.previewUrl
which is optional; update the rendering in the component that maps over
attachments (the attachments array/map block) to skip or fallback when
previewUrl is missing by either extending the filter to attachments.filter(att
=> att.fileType.startsWith('image/') && att.previewUrl) or conditionally
returning null inside the .map when attachment.previewUrl is falsy; keep the
same key (attachment.fileId) and alt (attachment.fileName) usage to avoid
key/prop regressions.
- Around line 88-123: The icon-only remove buttons lack accessible names; update
the two button elements that call removeAttachment(attachment.fileId) (the image
preview remove button and the non-image document remove button) to include an
aria-label prop that uses the existing i18n copy (or add new keys) such as
aria-label={t('attachments.remove', { fileName: attachment.fileName })} so
screen readers announce which file will be removed; ensure both <button ...
onClick={() => removeAttachment(attachment.fileId)}> instances are updated
consistently.
In
`@services/platform/app/features/automations/components/automation-assistant/collapsible-message.tsx`:
- Around line 61-84: The toggle button in the CollapsibleMessage component (the
button that uses shouldTruncate, isExpanded, and setIsExpanded) doesn't expose
state to assistive tech; update that button to include
aria-expanded={isExpanded} and add aria-controls pointing at the id of the
collapsible content element (generate a stable id with useId or a prop if one
doesn't exist) and ensure the collapsible content container has that same id and
appropriate role (e.g., region) so screen readers can associate the button with
the expanded/collapsed region.
In
`@services/platform/app/features/automations/components/automation-assistant/thinking-animation.tsx`:
- Around line 11-25: The effect and rendering can break when steps becomes empty
or shorter: clamp currentStep and guard rendering; inside the useEffect that
references currentStep and steps.length (and in the component where
steps[currentStep] is used) ensure you early-return when steps.length === 0 and
whenever steps.length changes call setCurrentStep(Math.min(currentStep,
Math.max(0, steps.length - 1))) (or reset to 0) so currentStep never indexes out
of bounds; update references to currentStep, setCurrentStep and steps.length
accordingly and only render steps[currentStep] when steps.length > 0.
In `@services/platform/app/features/automations/hooks/use-assistant-chat.ts`:
- Around line 25-40: The duplicate-send guard implemented by recentSends and
canSendMessage (using DUPLICATE_WINDOW_MS) currently keys only on threadId +
text which causes false blocks for attachment-only sends and collisions before
threadId is assigned; update canSendMessage and the place invoking it (the call
site around lines referenced in the comment) to either bypass the guard when
attachments are present OR expand the dedupe key to include automationId and
organizationId and a stable identifier for attachments (e.g., attachment IDs or
a fingerprint), and ensure a per-assistant-instance identifier (or automationId)
is used instead of the nullable threadId so starts-before-threadId don't
collide; also apply the same change to the other guard invocation noted in the
comment.
- Around line 282-285: Currently clearAttachments() is called before invoking
chatWithWorkflowAssistant which causes attachments to be lost if the mutation
fails; change the flow so you compute attachmentsToSend from clearAttachments()
but do not call clearAttachments() (or mutate stored attachments) until after
chatWithWorkflowAssistant resolves successfully, and on error either restore the
previous attachments or avoid clearing them—specifically update the logic around
attachmentsToSend/clearedAttachments and move the clearAttachments() side-effect
to the success branch (or add a restore step in the catch block) so that
chatWithWorkflowAssistant and its error handling keep access to the original
attachments.
In `@services/platform/app/features/automations/hooks/use-automation-layout.ts`:
- Around line 78-153: The recursive calculateLoopWidth function recomputes
widths for the same loop slugs repeatedly; add a memoization cache (e.g., a
Map<string, number>) keyed by loop stepSlug inside use-automation-layout and
consult it at the start of calculateLoopWidth and store computed widths before
returning; ensure calls that pass '' or invalid slugs are handled (skip caching
or return BASE_WIDTH) and that recursive calls
(calculateLoopWidth(loop.stepSlug)) use the cache to avoid duplicate work.
- Around line 238-416: The code repeatedly calls sortedSteps.find(...) during
edge construction which is O(n) per lookup; create a Map keyed by step.stepSlug
(e.g., stepBySlug = new Map(sortedSteps.map(s => [s.stepSlug, s]))) before the
forEach and replace all sortedSteps.find(...) uses (for sourceStep,
targetStepData, targetIsLoop checks, and any later lookups) with
stepBySlug.get(stepSlug); also compute targetIsLoop and sourceStep once from the
map, remove duplicate lookups inside the nested Object.entries loop, and keep
existing logic that uses loopBodyMap, edges.push and MarkerType unchanged.
- Around line 417-480: The bug is that edge.sourceHandle/edge.targetHandle
non-top handles are treated as bottom by default, causing left/right handles to
be counted as bottom; update the handle classification in the loop that
populates topHandlesUsed and bottomHandlesUsed so you only add to
bottomHandlesUsed when the handle startsWith('bottom-') (and otherwise ignore
handles that startWith('left-') or 'right-'), applying this change for both the
sourceHandle and targetHandle branches; after that the hasBidirectionalBottom
map computed from bottomHandlesUsed and the nodesWithFullConnectionData usage
will reflect only actual bottom handles.
- Around line 155-236: The issue is that parentLoopId selection picks a middle
loop when multiple candidate loops exist; change the selection to pick the
innermost loop (the candidate that does NOT contain any other candidate loop in
its body). In the node-building code where candidateLoops is computed, replace
the current candidate selection (the block computing parentLoopId when
candidateLoops.length > 1) with logic that picks the candidateId for which
loopBodyMap.get(candidateId) does not contain any other candidateId; if none
match, fall back to the last candidate in candidateLoops. Update references to
candidateLoops, parentLoopId, and loopBodyMap in use-automation-layout.ts
accordingly.
In `@services/platform/app/features/chat/components/message-bubble.tsx`:
- Around line 101-113: The map calls in MessageBubble that render
FileAttachmentDisplay and FilePartDisplay use index keys (key={index}) which can
cause stale UI; update the keys to use a stable unique identifier from each item
(e.g., use attachment.fileId || attachment.url for the FileAttachmentDisplay map
and part.fileId || part.url for the FilePartDisplay map), falling back to a
guaranteed unique property if present, and ensure the components accept those
props unchanged (refer to the message.attachments mapping and message.fileParts
mapping and the FileAttachmentDisplay/FilePartDisplay usages).
In
`@services/platform/app/features/chat/components/message-bubble/code-block.tsx`:
- Around line 39-43: The useEffect calling highlightCode(code, lang, shikiTheme)
can reject and currently has no error handling; update the effect in
message-bubble/code-block.tsx to catch failures from highlightCode (either by
using an async IIFE with try/catch or appending .catch()) and handle errors
(e.g., log via console.error or a logger and avoid calling setHtml when
cancelled), ensuring you still respect the cancelled flag before calling
setHtml(extractShikiCodeContent(result)); keep references to highlightCode,
setHtml, extractShikiCodeContent and the cancelled variable when adding the
error handling.
- Around line 106-117: Add an accessible label to the icon-only Button in
message-bubble/code-block.tsx: update the Button (the one using
onClick={handleCopy} and rendering CheckIcon/CopyIcon based on isCopied) to
include an aria-label prop that conveys the action/state (for example
aria-label={isCopied ? 'Copied' : 'Copy code'}), so screen readers can announce
the button purpose.
In
`@services/platform/app/features/chat/components/message-bubble/image-preview-dialog.tsx`:
- Around line 53-67: The current handleWheel prevents default on every wheel
event, blocking page scroll and panning; update the handler used in the
useEffect so wheel-based zoom only runs when a modifier key is held (e.g.,
e.ctrlKey || e.metaKey || e.altKey) or when the image is already zoomed and the
user explicitly intends to zoom; otherwise do not call e.preventDefault() and
allow normal scrolling/panning. Specifically, modify handleWheel (the function
passed to container.addEventListener) to check modifier keys before calling
e.preventDefault() and setZoom (using ZOOM_STEP, MAX_ZOOM, MIN_ZOOM), and
alternatively consider adding drag-to-pan handlers
(pointerdown/pointermove/pointerup) on containerRef to enable panning when zoom
> 1.
In
`@services/platform/app/features/chat/components/message-bubble/markdown-renderer.tsx`:
- Around line 103-121: The regex used to detect the code language only matches
word characters and thus ignores hyphenated ids; update the match call in the
anonymous renderer (the code: ({ node, className, children, ...props }) => { ...
}) to use a pattern that allows hyphens (replace the current
className.match(/language-(\w+)/) usage with a pattern that captures hyphenated
names) so HighlightedCode receives correct lang values (the lang prop passed to
HighlightedCode should come from the new capture group).
In
`@services/platform/app/features/conversations/components/bulk-send-dialog.tsx`:
- Around line 23-30: The modal overlay in bulk-send-dialog.tsx is missing dialog
semantics; update the outer dialog container (the inner div with classes
"bg-background mx-4 w-full max-w-md rounded-lg border p-6") to include
role="dialog" and aria-modal="true", add an id to the title element (h3) such as
"bulk-send-title" and reference it via aria-labelledby on the dialog, add an id
to the description paragraph (p) such as "bulk-send-desc" and reference it via
aria-describedby, and ensure the ids are used consistently so screen readers
announce the dialog (adjust component-level attributes in the
BulkSendDialog/related component accordingly).
- Around line 32-37: Replace the manual spinner/ARIA pattern with Button's
built-in loading support: in the bulk-send dialog's action buttons (the Button
with onConfirm and the secondary Button with onCancel) remove the inline
Loader2Icon and any manual aria-busy/animation logic and instead pass
isLoading={isSending} to the primary Button (and to secondary if desired) so the
Button component handles the spinner and aria attributes; keep the existing
label usages tConversations('bulkSend.send') and tCommon('actions.cancel') and
the disabled prop only if still needed for visual/behavior parity.
In
`@services/platform/app/features/conversations/components/conversations-client.tsx`:
- Around line 50-62: filteredConversations' useMemo uses `searchQuery ||
initialSearch`, which re-applies the initial term when the user clears the
input; change the memo to use `searchQuery` directly (e.g., `const searchTerm =
searchQuery`) and remove `initialSearch` from the dependency array so the filter
relies solely on the current `searchQuery`; keep the existing call to
`filterByTextSearch(paginatedResult.results, searchTerm,
['title','description','subject','externalMessageId'])` and retain
`paginatedResult.results` and `searchQuery` as the memo dependencies.
In `@services/platform/app/features/conversations/components/message-editor.tsx`:
- Around line 289-346: Extract the large inline <style> block from
message-editor.tsx into a dedicated CSS file (e.g., message-editor.css) and
import it in the MessageEditor component; move all rules under the .milkdown,
.milkdown .editor, and .milkdown .ProseMirror selectors into that stylesheet,
remove the inline <style> tag from the JSX, and ensure any CSS custom properties
and class names used by the editor remain unchanged so styles continue to apply
at runtime.
In
`@services/platform/app/features/conversations/components/message-editor/editor-action-bar.tsx`:
- Around line 104-123: The send Button rendered when !isImproveMode is icon-only
and lacks an accessible name; update the Button in editor-action-bar.tsx (the
Button with onClick={onSend} and children Send/LoaderCircleIcon) to include an
aria-label (e.g., aria-label="Send message" or similar) so screen readers can
announce its purpose while leaving existing props like disabled, size, and
className intact.
- Around line 69-83: The file input and attachment Button are missing accessible
labels; add an explicit accessible name to both by giving the <input>
(referenced by fileInputRef and handled by handleFileChange) either an
aria-label or an associated <label htmlFor="..."> and ensure the Button (which
triggers handleFileInputClick and shows PaperclipIcon) has an aria-label (e.g.,
"Attach file") and/or aria-describedby if additional context is needed so screen
readers can describe their purpose; ensure disabled state still conveys the
control is unavailable and that the input id matches any label's htmlFor.
- Around line 131-144: The improve submit Button in editor-action-bar.tsx is an
icon-only control (uses onImproveSubmit, isImproving,
LoaderCircleIcon/WandSparklesIcon inside Tooltip) and needs an accessible label;
add an aria-label (e.g., "Improve message") to the Button element so screen
readers can announce its purpose, matching the pattern used for other icon-only
buttons.
In
`@services/platform/app/features/conversations/components/message-editor/file-attachments-list.tsx`:
- Around line 54-60: The remove button in file-attachments-list.tsx contains
only an XIcon and lacks accessible text; update the button that calls
onRemove(file.id) to include an accessible label (e.g., add aria-label="Remove
attachment" or aria-label={`Remove ${file.name}`} or include visually hidden
text) so screen readers can convey its purpose while keeping the XIcon for
visual users.
- Around line 17-31: The formatFileSize function can index past the units array
for very large bytes; clamp the computed exponent i to the last valid unit index
before using it. In function formatFileSize(bytes, tCommon) compute i as now,
then set i = Math.min(i, units.length - 1) (or equivalent) so units[i] is always
defined; ensure the divisor uses the clamped i when calculating the display
value and keep the existing zero-case handling.
In
`@services/platform/app/features/conversations/components/message-editor/improve-mode.tsx`:
- Around line 39-43: The back button in improve-mode.tsx is icon-only and needs
an accessible label: add an aria-label to the Button (the one using
onClick={onClose} and rendering <ChevronLeft />) using the same translation
string tConversations('editor.backToEditor') so screen readers announce the
button purpose independently of the Tooltip; ensure the aria-label value matches
the tooltip text for consistency.
In
`@services/platform/app/features/conversations/hooks/use-conversation-selection.ts`:
- Around line 13-34: The selection logic in handleConversationCheck builds
selectedIds from conversations.map(c => c.id) but other code uses c._id, causing
mismatched membership checks; update handleConversationCheck to consistently use
the same identifier across the selection flow (use c._id everywhere or c.id
everywhere), i.e., when creating newSelectedIds from conversations use
conversations.map(c => c._id) (or change other checks to .id) and ensure
setSelectionState(selectedIds) stores the same id key as the rest of the code
that reads selectionState.selectedIds.
In
`@services/platform/app/features/custom-agents/components/test-chat-panel/test-chat-input.tsx`:
- Around line 95-123: The icon-only buttons in test-chat-input.tsx (the
removeAttachment handlers inside the attachments.map for images and non-images,
and the other icon-only attach/send buttons noted around the second block) lack
accessible names; update each <button> that only contains an <X> or other icon
to include a descriptive aria-label (e.g., for the remove button use
aria-label={`Remove ${attachment.fileName}`} or similar to include the
attachment.fileName, and for generic attach/send buttons use aria-label="Attach
file" or aria-label="Send message"); ensure you add the aria-label prop on the
same elements that call removeAttachment and on the icon-only attach/send
buttons so screen readers receive meaningful labels.
In `@services/platform/app/features/custom-agents/hooks/use-test-chat.ts`:
- Around line 418-425: handleClearChat currently only deletes the server thread
and calls onReset, leaving local threadId, messages, pending state, and
attachments stale; update handleClearChat to reset local state as well by (1)
awaiting or handling the deleteChatThread promise and then clearing local
variables/state: setThreadId(null) (or equivalent), setMessages([]),
setIsPending(false), and clearAttachments() (or whatever local attachment reset
exists), and (2) ensure onReset is still invoked (or folded into these resets)
so the client UI cannot attempt to send to the deleted thread; reference
handleClearChat, deleteChatThread, threadId, and onReset when locating where to
add these resets.
In
`@services/platform/app/features/documents/components/onedrive-import-dialog.tsx`:
- Line 267: filteredItems is redundantly assigned from currentItems in the
OnedriveImportDialog component; remove the unnecessary const filteredItems =
currentItems and replace uses of filteredItems with currentItems (e.g., where
filteredItems is referenced in rendering, handlers, or effects) so the component
uses currentItems directly and the unused filteredItems binding is eliminated.
- Around line 451-525: The code is calling OneDrivePickerStage (and
OneDriveSettingsStage) as plain functions which breaks React hooks; change these
to be rendered as JSX components (e.g. <OneDrivePickerStage ... /> and
<OneDriveSettingsStage ... />) and update those components to return JSX (or
expose render props) so you can use their provided header/content via props
(instead of reading picker.customHeader/picker.content from a function result);
ensure all props listed (sourceTab, searchQuery, selectedItems, onSpFolderClick,
onProceedToSettings, etc.) are passed as component props and that any
previously-returned customHeader/content are implemented as children or props on
the component for use by Dialog.
- Around line 138-209: The recursive async function collectAllFiles is recreated
on every render and closes over component state (sourceTab, selectedSite,
selectedDrive), risking stale closures and wasted work; fix by either extracting
collectAllFiles to a module-level helper that accepts the dynamic deps
(sourceTab, selectedSite, selectedDrive, listSharePointFiles, listOneDriveFiles,
toast, t) as parameters, or wrap it in useCallback inside the component with an
explicit dependency array ([sourceTab, selectedSite, selectedDrive,
listSharePointFiles, listOneDriveFiles, toast, t]) so the function identity is
stable during long-running recursion and uses correct values. Ensure any local
helpers (e.g., isFile/isFolder) remain accessible or are passed in as arguments
if you move the function out.
In
`@services/platform/app/features/documents/components/onedrive-import/onedrive-file-table.tsx`:
- Around line 35-46: The component OneDriveFileTable declares a selectedItems
prop but doesn't use it and the useMemo that computes emptyState should include
selectedItems in its dependency array if selectedItems affects rendering; either
remove selectedItems from the OneDriveFileTableProps if unused, or add
selectedItems to the useMemo dependency list for emptyState (and reference it
inside the memo if needed) to prevent stale renders — search for
OneDriveFileTable, the selectedItems prop, and the emptyState useMemo to make
the change.
In
`@services/platform/app/features/documents/components/onedrive-import/onedrive-picker-stage.tsx`:
- Around line 111-134: Wrap the two tab buttons in a container with
role="tablist" and update each button to include role="tab", aria-selected
(e.g., aria-selected={sourceTab === 'onedrive'}), an id (e.g., "tab-onedrive" /
"tab-sharepoint"), aria-controls pointing to the corresponding panel ids (e.g.,
"panel-onedrive" / "panel-sharepoint"), and a keyboard-focusable tabindex (0 for
selected, -1 for non-selected) while still using onTabChange to switch tabs;
also ensure the corresponding content panels use role="tabpanel" with matching
ids so aria-controls links work.
In
`@services/platform/app/features/documents/components/onedrive-import/onedrive-settings-stage.tsx`:
- Around line 74-184: The current render-object export (returning {title,
description, footer, customHeader, content}) in the Onedrive settings stage
should be refactored because the parent should not call a React component as a
function; change this file to either (A) export a custom hook (e.g.,
useOnedriveSettingsConfig) that returns the config object (title, description,
footer, customHeader, content) so the parent can call a hook, or (B) convert
this into a proper React component (e.g., OnedriveSettingsStage) that renders
the Dialog/UI directly instead of returning an object; update usages in the
parent to call the hook or render the component accordingly and ensure the
unique keys from the diff (title, description, customHeader, content, footer,
RadioGroup, onImportTypeChange, selectedTeams, onToggleTeam) are preserved and
wired into the new hook/component API.
- Around line 162-174: The team row is visually clickable but only the Checkbox
is interactive; make the whole row activate the same toggle by adding an onClick
on the surrounding div that calls onToggleTeam(team.id) (respecting the
isImporting guard) so clicking anywhere toggles selection, and avoid
double-toggling by early-returning from that handler when the event originates
from an interactive element (e.g. input/button/a) — reference the surrounding
div that wraps Checkbox (id `onedrive-team-${team.id}`), the Checkbox component,
the onToggleTeam function, selectedTeams and isImporting.
In
`@services/platform/app/features/settings/integrations/components/integration-manage-dialog.tsx`:
- Around line 60-63: The component currently uses inline handlers that directly
call hook setters (e.g. setParsedUpdate, setUpdateParseError, setCredentials,
setSqlConfig, setOAuth2Fields, setSelectedAuthMethod, setTestResult) which
should be encapsulated inside useIntegrationManage; add methods like
handleClearUpdate, handleAuthMethodChange, handleCredentialChange,
handleSqlConfigChange and handleOAuth2FieldsChange to the hook (they should call
the existing setters and perform related resets like clearing test results),
return these handlers from useIntegrationManage, and replace the inline arrow
functions in integration-manage-dialog.tsx (the onClearUpdate and the handlers
referenced around lines 103–119 in the review) with the corresponding hook
methods to reduce allocations and centralize state transitions.
In
`@services/platform/app/features/settings/integrations/components/integration-manage/integration-delete-section.tsx`:
- Around line 31-61: The Button elements for delete/confirm/cancel in
integration-delete-section.tsx currently rely on default button behavior which
can submit surrounding forms; update each Button (the ones rendering Trash2 and
calling onDelete and onConfirmToggle) to set an explicit type prop of "button"
to prevent accidental form submits and keep existing props (variant, size,
onClick, disabled, className) unchanged.
In
`@services/platform/app/features/settings/integrations/components/integration-manage/integration-icon-upload.tsx`:
- Around line 75-79: The code lowercases the translated label via
t('integrations.upload.operations').toLowerCase() which is locale-insensitive;
remove the .toLowerCase() call and either use a dedicated translation key for
the lowercased/variant text (e.g. add a new key like
'integrations.upload.operationsLabel' or
'integrations.upload.operations_lowercase' and call t(...) directly) or apply
casing via styling (e.g. CSS/text-transform on the Badge). Update the component
where operationCount and t('integrations.upload.operations') are used (in
integration-icon-upload.tsx, inside the HStack/Badge) to use the new translation
key or styled Badge instead of mutating the translated string.
In
`@services/platform/app/features/settings/integrations/components/integration-manage/integration-update-section.tsx`:
- Around line 112-122: The rendered line count can be misleading when
connectorCode contains only empty/whitespace lines; update the logic in
integration-update-section.tsx around parsedUpdate.connectorCode to compute
nonEmptyLines by splitting on '\n' and filtering out lines where line.trim() is
empty, then use that nonEmptyLines value both for the conditional render and for
the displayed count (and avoid using trim().split('\n').length); reference
parsedUpdate.connectorCode and the HStack block that currently shows
newCodeLines.
In
`@services/platform/app/features/settings/integrations/components/sso-config/role-mapping-section.tsx`:
- Around line 59-68: The role-mapping source options currently only include
'jobTitle' and 'appRole', which prevents group-based mappings; update the
options passed to the Select (in role-mapping-section.tsx / the options array)
to include an entry with value 'group' and label
t('integrations.sso.sourceGroup') so group-based mappings can be created and
existing group rules edited; ensure the new option follows the same shape as the
others and use the same i18n key pattern for the label.
- Around line 49-101: Add accessible names to the two Selects, the Input, and
the icon-only delete Button: update the Select component usage (first Select for
source, second Select for targetRole) to pass an accessible name/label prop (or
extend the Select component to accept and forward aria-label/aria-labelledby to
its trigger), add a visible or sr-only label element tied to the Input for
pattern (rule.pattern) and ensure onChange still calls onUpdate, and add an
aria-label (e.g. "Delete role mapping") to the Button that calls onRemove(index)
so the Trash2 icon is announced; keep existing props (value, onValueChange,
disabled, options) unchanged and only add accessible labeling.
In
`@services/platform/app/features/settings/integrations/hooks/use-integration-manage.ts`:
- Around line 216-235: The handleIconUpload callback currently returns early on
invalid size/type but leaves the file input value set, preventing onChange when
a user reselects the same file; update handleIconUpload to clear the file input
(e.g., set e.currentTarget.value = '' or e.target.value = '') immediately before
each early return for the MAX_ICON_SIZE and ACCEPTED_ICON_TYPES checks so the
input is reset and the same file can be selected again.
In
`@services/platform/app/features/settings/integrations/hooks/use-sso-config-form.ts`:
- Around line 31-160: The role-mapping filter in useSsoConfigForm (inside the
useEffect handling getFullConfig) incorrectly drops rules with source 'group' by
only allowing 'jobTitle' and 'appRole'; update that filter to preserve 'group'
as well and explicitly exclude only 'claim' so existing group mappings are not
lost, and ensure DEFAULT_MAPPING_RULES and any UI defaults/validation referring
to roleMappingRules still support 'group' as a valid source (identify symbols:
useSsoConfigForm, DEFAULT_MAPPING_RULES, roleMappingRules, getFullConfig).
- Around line 207-321: The current handlers send an empty clientSecret and treat
"use existing config" purely by isConnected & empty clientSecret, which can
overwrite or incorrectly test the stored secret; update both handleSave and
handleTest to only include clientSecret in the payload when clientSecret is
non-empty and to treat useExistingConfig as true only when the form has no
changes vs the stored provider (e.g., compare issuer and clientId and any edited
fields against the existing provider object you already have, such as
existingProvider or ssoProvider). Specifically: in handleSave
(upsertSSOProvider) build the payload so clientSecret is omitted when
clientSecret is falsy rather than sent as an empty string; in handleTest compute
useExistingConfig = isConnected && !clientSecret &&
noFormChangesAgainstExistingProvider (compare issuer/clientId/other relevant
form fields to existingProvider.issuer, existingProvider.clientId, etc.); and in
handleTest call testExistingSSOConfig only when that stricter useExistingConfig
is true, otherwise call testSSOConfig with a payload that includes clientSecret
only if non-empty.
In `@services/platform/app/routes/dashboard/`$id/_knowledge/customers.tsx:
- Around line 55-58: The active-filters check is missing the search.query field
so pages with only a text query still show the "no customers yet" empty state;
update the hasActiveFilters boolean (the const hasActiveFilters defined near the
top of this component) to include search.query alongside search.status,
search.source and search.locale so that any non-empty query flips
hasActiveFilters to true and triggers the "no results" path.
In `@services/platform/app/routes/dashboard/`$id/_knowledge/products.tsx:
- Around line 53-56: The active-filter flag misses query-only filters: update
the hasActiveFilters computation (where hasActiveFilters is defined) to include
search.query alongside search.status and search.category so that a non-empty
search.query counts as an active filter and triggers the “no results” empty
state instead of the “no products yet” copy.
Comment on lines
+80
to
+96
| it('calls onFilesSelected when file input changes', async () => { | ||
| const handleFiles = vi.fn(); | ||
| render( | ||
| <FileUpload.Root> | ||
| <FileUpload.DropZone | ||
| onFilesSelected={handleFiles} | ||
| inputId="test-upload" | ||
| aria-label="Upload files" | ||
| > | ||
| <p>Drop files here</p> | ||
| </FileUpload.DropZone> | ||
| </FileUpload.Root>, | ||
| ); | ||
|
|
||
| const input = document.getElementById('test-upload'); | ||
| expect(input).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the test file
find . -name "file-upload.test.tsx" -type f | head -5Repository: tale-project/tale
Length of output: 125
🏁 Script executed:
# Check the file path to ensure we find it
find . -path "*services/platform/app/components/ui/forms/file-upload.test.tsx" -type fRepository: tale-project/tale
Length of output: 125
🏁 Script executed:
# Broader search for the test file
find . -name "*file-upload*" -type f | grep -E "test|spec"Repository: tale-project/tale
Length of output: 125
🏁 Script executed:
# Read the test file with line numbers
cat -n ./services/platform/app/components/ui/forms/file-upload.test.tsxRepository: tale-project/tale
Length of output: 6799
🏁 Script executed:
# Check the test utils to see what's available
cat -n ./services/platform/app/test/utils/render.ts | head -30Repository: tale-project/tale
Length of output: 137
🏁 Script executed:
# Look at the FileUpload component implementation
cat -n ./services/platform/app/components/ui/forms/file-upload.tsx | head -100Repository: tale-project/tale
Length of output: 2942
🏁 Script executed:
# Continue reading the FileUpload component to see the DropZone implementation
cat -n ./services/platform/app/components/ui/forms/file-upload.tsx | tail -200Repository: tale-project/tale
Length of output: 6192
🏁 Script executed:
# Find test utils directory
find . -path "*test/utils*" -type dRepository: tale-project/tale
Length of output: 91
🏁 Script executed:
# Check test utils directory
ls -la ./services/platform/test/utils/Repository: tale-project/tale
Length of output: 288
🏁 Script executed:
# Check if fireEvent is used elsewhere in the codebase
rg "fireEvent" ./services/platform --type tsx --type ts | head -20Repository: tale-project/tale
Length of output: 88
🏁 Script executed:
# Check if userEvent is used instead
rg "userEvent" ./services/platform --type tsx --type ts | head -20Repository: tale-project/tale
Length of output: 88
🏁 Script executed:
# Check if fireEvent is used elsewhere in the codebase
rg "fireEvent" ./services/platform -t ts -t js | head -20Repository: tale-project/tale
Length of output: 43
🏁 Script executed:
# Check if userEvent is used instead
rg "userEvent" ./services/platform -t ts -t js | head -20Repository: tale-project/tale
Length of output: 579
🏁 Script executed:
# Read the render.tsx file to understand the test setup
cat -n ./services/platform/test/utils/render.tsxRepository: tale-project/tale
Length of output: 899
🏁 Script executed:
# Look for other file input or upload tests to see patterns
find ./services/platform -name "*.test.ts*" -type f -exec grep -l "HTMLInputElement\|file.*input\|upload" {} \; | head -5Repository: tale-project/tale
Length of output: 357
🏁 Script executed:
# Check if there are other tests that interact with file inputs
rg "onChange.*input|files\[" ./services/platform/app/components -t ts | head -20Repository: tale-project/tale
Length of output: 209
🏁 Script executed:
# Look for how userEvent is used in other test files
rg "user\..*input\|user\..*change" ./services/platform/app/components -B2 -A2 | head -40Repository: tale-project/tale
Length of output: 43
Test doesn't verify the onFilesSelected callback is invoked.
The test only asserts that the input element exists but never simulates a file change or verifies that the handler was called. This won't catch regressions in the file selection logic.
Consider using userEvent.upload() to simulate file selection and assert the callback was invoked:
it('calls onFilesSelected when file input changes', async () => {
const handleFiles = vi.fn();
const { user } = render(
<FileUpload.Root>
<FileUpload.DropZone
onFilesSelected={handleFiles}
inputId="test-upload"
aria-label="Upload files"
>
<p>Drop files here</p>
</FileUpload.DropZone>
</FileUpload.Root>,
);
const input = document.getElementById('test-upload') as HTMLInputElement;
expect(input).toBeInTheDocument();
const file = new File(['hello'], 'test.csv', { type: 'text/csv' });
await user.upload(input, file);
expect(handleFiles).toHaveBeenCalledWith([file]);
});🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@services/platform/app/components/ui/forms/file-upload.test.tsx` around lines
80 - 96, The test currently only checks the presence of the input and never
simulates a file selection, so update the test for FileUpload.DropZone to
simulate a user uploading files and assert onFilesSelected was called: create a
mock handler (handleFiles = vi.fn()), find the input element by id
"test-upload", use userEvent.upload(input, [file]) to trigger change, await any
async updates, then expect(handleFiles).toHaveBeenCalledWith(...) or
toHaveBeenCalled(); ensure the test imports and uses userEvent and constructs a
File object to pass into the upload call.
Comment on lines
+65
to
+71
| useEffect(() => { | ||
| if (hasError) { | ||
| setShowShake(true); | ||
| const timer = setTimeout(() => setShowShake(false), 400); | ||
| return () => clearTimeout(timer); | ||
| } | ||
| }, [hasError, errorMessage]); |
There was a problem hiding this comment.
Shake state can stick after clearing an error.
If the error is removed before the timeout, showShake stays true and keeps the animation class.
🧹 Suggested fix
useEffect(() => {
- if (hasError) {
- setShowShake(true);
- const timer = setTimeout(() => setShowShake(false), 400);
- return () => clearTimeout(timer);
- }
+ if (!hasError) {
+ setShowShake(false);
+ return;
+ }
+ setShowShake(true);
+ const timer = setTimeout(() => setShowShake(false), 400);
+ return () => clearTimeout(timer);
}, [hasError, errorMessage]);🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@services/platform/app/components/ui/forms/file-upload.tsx` around lines 65 -
71, The showShake animation can remain true if hasError flips to false before
the timeout; update the useEffect that watches hasError/errorMessage so the
cleanup always clears the pending timer and resets showShake to false when the
effect re-runs or unmounts. Concretely, in the useEffect containing hasError ->
setShowShake(true) use a local timer id, set the timeout only when hasError is
true, and return a cleanup that clearsTimeout(timer) and calls
setShowShake(false) to ensure the shake state is cleared if the error is removed
early (referencing useEffect, hasError, setShowShake, errorMessage).
Comment on lines
+265
to
+271
| React.useEffect(() => { | ||
| if (hasAnyError) { | ||
| setShowShake(true); | ||
| const timer = setTimeout(() => setShowShake(false), 400); | ||
| return () => clearTimeout(timer); | ||
| } | ||
| }, [hasAnyError, displayError]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Shake animation may re-trigger unnecessarily when error text changes.
The effect triggers on [hasAnyError, displayError], so if the error message text changes while hasAnyError remains true, the shake animation will re-trigger. If only new errors should shake, consider removing displayError from dependencies or tracking the previous error state.
♻️ Suggested fix to trigger shake only on new errors
React.useEffect(() => {
if (hasAnyError) {
setShowShake(true);
const timer = setTimeout(() => setShowShake(false), 400);
return () => clearTimeout(timer);
}
- }, [hasAnyError, displayError]);
+ }, [hasAnyError]);📝 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
| React.useEffect(() => { | |
| if (hasAnyError) { | |
| setShowShake(true); | |
| const timer = setTimeout(() => setShowShake(false), 400); | |
| return () => clearTimeout(timer); | |
| } | |
| }, [hasAnyError, displayError]); | |
| React.useEffect(() => { | |
| if (hasAnyError) { | |
| setShowShake(true); | |
| const timer = setTimeout(() => setShowShake(false), 400); | |
| return () => clearTimeout(timer); | |
| } | |
| }, [hasAnyError]); |
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@services/platform/app/components/ui/forms/json-input.tsx` around lines 265 -
271, The effect on React.useEffect is re-triggering when the error text
(displayError) changes even though hasAnyError stays true; update the effect to
only run when hasAnyError changes by removing displayError from the dependency
list or, better, detect the transition from false→true by tracking previous
hasAnyError (e.g., a ref storing prevHasAnyError) and only call
setShowShake(true)/start the timeout when prevHasAnyError is false and
hasAnyError is true; ensure you still clear the timeout in the cleanup and keep
setShowShake(false) after 400ms (references: React.useEffect, hasAnyError,
displayError, setShowShake).
Comment on lines
323
to
+331
| <div | ||
| className={cn( | ||
| 'border rounded-md overflow-hidden bg-card', | ||
| !isValid && 'border-destructive', | ||
| hasAnyError && 'border-destructive', | ||
| showShake && 'animate-shake', | ||
| disabled && 'opacity-50 cursor-not-allowed', | ||
| )} | ||
| role="group" | ||
| aria-describedby={describedBy} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a <fieldset> element instead of role="group".
Static analysis flagged that semantic HTML elements are preferred over ARIA roles. A <fieldset> would provide better native semantics for grouping form controls, though it requires resetting default styling.
♻️ Suggested refactor using fieldset
- <div
+ <fieldset
className={cn(
- 'border rounded-md overflow-hidden bg-card',
+ 'border rounded-md overflow-hidden bg-card appearance-none m-0 p-0 min-w-0',
hasAnyError && 'border-destructive',
showShake && 'animate-shake',
disabled && 'opacity-50 cursor-not-allowed',
)}
- role="group"
aria-describedby={describedBy}
+ disabled={disabled}
>Note: Ensure fieldset default styles are reset in your CSS if not already handled by Tailwind Preflight.
📝 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
| <div | |
| className={cn( | |
| 'border rounded-md overflow-hidden bg-card', | |
| !isValid && 'border-destructive', | |
| hasAnyError && 'border-destructive', | |
| showShake && 'animate-shake', | |
| disabled && 'opacity-50 cursor-not-allowed', | |
| )} | |
| role="group" | |
| aria-describedby={describedBy} | |
| <fieldset | |
| className={cn( | |
| 'border rounded-md overflow-hidden bg-card appearance-none m-0 p-0 min-w-0', | |
| hasAnyError && 'border-destructive', | |
| showShake && 'animate-shake', | |
| disabled && 'opacity-50 cursor-not-allowed', | |
| )} | |
| aria-describedby={describedBy} | |
| disabled={disabled} | |
| > |
🧰 Tools
🪛 Biome (2.3.14)
[error] 330-330: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@services/platform/app/components/ui/forms/json-input.tsx` around lines 323 -
331, Replace the wrapper div that uses role="group" with a semantic <fieldset>
to provide native grouping for the form controls: in the component where the div
currently has className={cn(...)} and role="group" (and uses
aria-describedby={describedBy}, hasAnyError, showShake, disabled flags), switch
to a fieldset element, preserve all className logic and aria-describedby, remove
role="group", and map the disabled boolean to the fieldset's disabled attribute;
also ensure default fieldset styles are reset in your CSS/Tailwind (Preflight)
so the visual appearance remains the same.
| role="region" | ||
| aria-describedby={description ? descriptionId : undefined} | ||
| > | ||
| <div className="p-3" role="region" aria-describedby={describedBy}> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use <section> instead of role="region" and add an accessible name.
Per static analysis, prefer semantic elements. Additionally, landmarks (role="region") require an accessible name to be announced by screen readers.
♻️ Suggested refactor
- <div className="p-3" role="region" aria-describedby={describedBy}>
+ <section className="p-3" aria-label={label ?? 'JSON viewer'} aria-describedby={describedBy}>
<ReactJsonView
...
/>
- </div>
+ </section>📝 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
| <div className="p-3" role="region" aria-describedby={describedBy}> | |
| <section className="p-3" aria-label={label ?? 'JSON viewer'} aria-describedby={describedBy}> | |
| <ReactJsonView | |
| ... | |
| /> | |
| </section> |
🧰 Tools
🪛 Biome (2.3.14)
[error] 361-361: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@services/platform/app/components/ui/forms/json-input.tsx` at line 361,
Replace the non-semantic <div className="p-3" role="region"
aria-describedby={describedBy}> with a semantic <section> and provide an
accessible name: remove role="region", use <section className="p-3"> and add
either aria-labelledby pointing to an existing visible label/id or aria-label
(e.g., aria-labelledby={labelId} or aria-label={labelText}) so screen readers
announce the region; reference the describedBy variable to keep the existing
aria-describedby behavior and update the JsonInput/JsonInputForm component
markup to use section + accessible name.
Comment on lines
+74
to
+184
| return { | ||
| title: t('onedrive.importSettings'), | ||
| description: t('onedrive.settingsDescription', { | ||
| count: selectedItemCount, | ||
| }), | ||
| footer, | ||
| footerClassName: 'border-t border-border p-4', | ||
| customHeader: ( | ||
| <div className="border-border flex items-start justify-between border-b px-6 py-5"> | ||
| <div className="space-y-1"> | ||
| <h2 className="text-foreground text-base leading-none font-semibold tracking-tight"> | ||
| {t('onedrive.importSettings')} | ||
| </h2> | ||
| <p className="text-muted-foreground text-sm"> | ||
| {t('onedrive.settingsDescription', { | ||
| count: selectedItemCount, | ||
| })} | ||
| </p> | ||
| </div> | ||
| </div> | ||
| ), | ||
| content: ( | ||
| <div className="px-6 py-2"> | ||
| <RadioGroup | ||
| value={importType} | ||
| onValueChange={(value: string) => | ||
| // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- Radix RadioGroup onValueChange returns string | ||
| onImportTypeChange(value as ImportType) | ||
| } | ||
| className="space-y-2" | ||
| > | ||
| <div className="border-border hover:bg-muted rounded-lg border p-3"> | ||
| <div className="flex items-center gap-3"> | ||
| <RadioGroupItem value="one-time" id="one-time" /> | ||
| <div className="flex-1"> | ||
| <label | ||
| htmlFor="one-time" | ||
| className="cursor-pointer text-base font-medium" | ||
| > | ||
| {t('onedrive.oneTimeImport')} | ||
| </label> | ||
| <div className="text-muted-foreground text-sm"> | ||
| {t('onedrive.oneTimeDescription')} | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div className="border-border hover:bg-muted rounded-lg border p-3"> | ||
| <div className="flex items-center gap-3"> | ||
| <RadioGroupItem value="sync" id="sync" /> | ||
| <div className="flex-1"> | ||
| <label | ||
| htmlFor="sync" | ||
| className="cursor-pointer text-base font-medium" | ||
| > | ||
| {t('onedrive.syncImport')} | ||
| </label> | ||
| <div className="text-muted-foreground text-sm"> | ||
| {t('onedrive.syncDescription')} | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </RadioGroup> | ||
|
|
||
| <div className="border-border mt-4 border-t pt-4"> | ||
| <p className="mb-2 text-sm font-medium">{t('upload.selectTeams')}</p> | ||
| <p className="text-muted-foreground mb-3 text-xs"> | ||
| {t('upload.selectTeamsDescription')} | ||
| </p> | ||
|
|
||
| {isLoadingTeams ? ( | ||
| <div className="flex items-center justify-center py-4"> | ||
| <span className="text-muted-foreground text-sm"> | ||
| {tCommon('actions.loading')} | ||
| </span> | ||
| </div> | ||
| ) : !teams || teams.length === 0 ? ( | ||
| <div className="flex flex-col items-center justify-center py-4 text-center"> | ||
| <Users className="text-muted-foreground/50 mb-2 size-6" /> | ||
| <p className="text-muted-foreground text-sm"> | ||
| {t('upload.noTeamsAvailable')} | ||
| </p> | ||
| </div> | ||
| ) : ( | ||
| <Stack gap={2}> | ||
| {teams.map((team) => ( | ||
| <div | ||
| key={team.id} | ||
| className="bg-card hover:bg-accent/50 flex cursor-pointer items-center gap-3 rounded-lg border p-3 transition-colors" | ||
| > | ||
| <Checkbox | ||
| id={`onedrive-team-${team.id}`} | ||
| checked={selectedTeams.has(team.id)} | ||
| onCheckedChange={() => onToggleTeam(team.id)} | ||
| disabled={isImporting} | ||
| label={team.name} | ||
| /> | ||
| </div> | ||
| ))} | ||
| </Stack> | ||
| )} | ||
|
|
||
| <p className="text-muted-foreground mt-3 text-xs"> | ||
| {t('upload.allMembersHint')} | ||
| </p> | ||
| </div> | ||
| </div> | ||
| ), | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The render-object pattern requires proper consumption in the parent.
This component returns an object containing {title, description, footer, customHeader, content} rather than JSX. This pattern works only if the parent calls this as a regular function (not as a React component). However, as noted in the parent file review, calling React components as functions is an anti-pattern.
Consider refactoring to either:
- A compound component pattern where this renders the Dialog itself
- A custom hook that returns the configuration object
- Direct JSX composition in the parent
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In
`@services/platform/app/features/documents/components/onedrive-import/onedrive-settings-stage.tsx`
around lines 74 - 184, The current render-object export (returning {title,
description, footer, customHeader, content}) in the Onedrive settings stage
should be refactored because the parent should not call a React component as a
function; change this file to either (A) export a custom hook (e.g.,
useOnedriveSettingsConfig) that returns the config object (title, description,
footer, customHeader, content) so the parent can call a hook, or (B) convert
this into a proper React component (e.g., OnedriveSettingsStage) that renders
the Dialog/UI directly instead of returning an object; update usages in the
parent to call the hook or render the component accordingly and ensure the
unique keys from the diff (title, description, customHeader, content, footer,
RadioGroup, onImportTypeChange, selectedTeams, onToggleTeam) are preserved and
wired into the new hook/component API.
Comment on lines
+162
to
+174
| <div | ||
| key={team.id} | ||
| className="bg-card hover:bg-accent/50 flex cursor-pointer items-center gap-3 rounded-lg border p-3 transition-colors" | ||
| > | ||
| <Checkbox | ||
| id={`onedrive-team-${team.id}`} | ||
| checked={selectedTeams.has(team.id)} | ||
| onCheckedChange={() => onToggleTeam(team.id)} | ||
| disabled={isImporting} | ||
| label={team.name} | ||
| /> | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Team row div is clickable but only the checkbox is interactive.
The outer div has cursor-pointer styling but clicking anywhere on it doesn't toggle the checkbox - only the checkbox itself responds. Consider making the entire row clickable for better UX.
♻️ Proposed fix
<div
key={team.id}
className="bg-card hover:bg-accent/50 flex cursor-pointer items-center gap-3 rounded-lg border p-3 transition-colors"
+ onClick={() => !isImporting && onToggleTeam(team.id)}
>
<Checkbox
id={`onedrive-team-${team.id}`}
checked={selectedTeams.has(team.id)}
- onCheckedChange={() => onToggleTeam(team.id)}
+ onCheckedChange={(e) => {
+ e.stopPropagation?.();
+ onToggleTeam(team.id);
+ }}
disabled={isImporting}
label={team.name}
/>
</div>🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In
`@services/platform/app/features/documents/components/onedrive-import/onedrive-settings-stage.tsx`
around lines 162 - 174, The team row is visually clickable but only the Checkbox
is interactive; make the whole row activate the same toggle by adding an onClick
on the surrounding div that calls onToggleTeam(team.id) (respecting the
isImporting guard) so clicking anywhere toggles selection, and avoid
double-toggling by early-returning from that handler when the event originates
from an interactive element (e.g. input/button/a) — reference the surrounding
div that wraps Checkbox (id `onedrive-team-${team.id}`), the Checkbox component,
the onToggleTeam function, selectedTeams and isImporting.
Comment on lines
+75
to
+79
| {operationCount > 0 && ( | ||
| <HStack gap={2} className="ml-auto flex-wrap"> | ||
| <Badge variant="outline" className="text-xs"> | ||
| {operationCount} {t('integrations.upload.operations').toLowerCase()} | ||
| </Badge> |
There was a problem hiding this comment.
Avoid lowercasing translated labels in code.
Lowercasing a translated string is locale-insensitive and can produce incorrect casing in some languages. Prefer a dedicated translation key or styling instead of mutating the text.
✏️ Suggested change
- {operationCount} {t('integrations.upload.operations').toLowerCase()}
+ {operationCount} {t('integrations.upload.operations')}📝 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
| {operationCount > 0 && ( | |
| <HStack gap={2} className="ml-auto flex-wrap"> | |
| <Badge variant="outline" className="text-xs"> | |
| {operationCount} {t('integrations.upload.operations').toLowerCase()} | |
| </Badge> | |
| {operationCount > 0 && ( | |
| <HStack gap={2} className="ml-auto flex-wrap"> | |
| <Badge variant="outline" className="text-xs"> | |
| {operationCount} {t('integrations.upload.operations')} | |
| </Badge> |
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In
`@services/platform/app/features/settings/integrations/components/integration-manage/integration-icon-upload.tsx`
around lines 75 - 79, The code lowercases the translated label via
t('integrations.upload.operations').toLowerCase() which is locale-insensitive;
remove the .toLowerCase() call and either use a dedicated translation key for
the lowercased/variant text (e.g. add a new key like
'integrations.upload.operationsLabel' or
'integrations.upload.operations_lowercase' and call t(...) directly) or apply
casing via styling (e.g. CSS/text-transform on the Badge). Update the component
where operationCount and t('integrations.upload.operations') are used (in
integration-icon-upload.tsx, inside the HStack/Badge) to use the new translation
key or styled Badge instead of mutating the translated string.
Comment on lines
+55
to
+58
| const hasActiveFilters = !!(search.status || search.source || search.locale); | ||
|
|
||
| const isInitialLoading = | ||
| paginatedResult.status === 'LoadingFirstPage' && !hasServerFilters; | ||
| paginatedResult.status === 'LoadingFirstPage' && !paginatedResult.results; |
There was a problem hiding this comment.
Include search.query in active-filter detection.
When only the query is set, the empty state still renders the “no customers yet” copy instead of “no results.” Add search.query to the hasActiveFilters check.
🔧 Suggested fix
- const hasActiveFilters = !!(search.status || search.source || search.locale);
+ const hasActiveFilters = !!(
+ search.query ||
+ search.status ||
+ search.source ||
+ search.locale
+ );📝 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 hasActiveFilters = !!(search.status || search.source || search.locale); | |
| const isInitialLoading = | |
| paginatedResult.status === 'LoadingFirstPage' && !hasServerFilters; | |
| paginatedResult.status === 'LoadingFirstPage' && !paginatedResult.results; | |
| const hasActiveFilters = !!( | |
| search.query || | |
| search.status || | |
| search.source || | |
| search.locale | |
| ); | |
| const isInitialLoading = | |
| paginatedResult.status === 'LoadingFirstPage' && !paginatedResult.results; |
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@services/platform/app/routes/dashboard/`$id/_knowledge/customers.tsx around
lines 55 - 58, The active-filters check is missing the search.query field so
pages with only a text query still show the "no customers yet" empty state;
update the hasActiveFilters boolean (the const hasActiveFilters defined near the
top of this component) to include search.query alongside search.status,
search.source and search.locale so that any non-empty query flips
hasActiveFilters to true and triggers the "no results" path.
Comment on lines
+53
to
+56
| const hasActiveFilters = !!(search.status || search.category); | ||
|
|
||
| const isInitialLoading = | ||
| paginatedResult.status === 'LoadingFirstPage' && !hasServerFilters; | ||
| paginatedResult.status === 'LoadingFirstPage' && !paginatedResult.results; |
There was a problem hiding this comment.
Include search.query in active-filter detection.
With a query-only filter, the empty state will still show the “no products yet” copy instead of “no results.” Add search.query to hasActiveFilters.
🔧 Suggested fix
- const hasActiveFilters = !!(search.status || search.category);
+ const hasActiveFilters = !!(search.query || search.status || search.category);📝 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 hasActiveFilters = !!(search.status || search.category); | |
| const isInitialLoading = | |
| paginatedResult.status === 'LoadingFirstPage' && !hasServerFilters; | |
| paginatedResult.status === 'LoadingFirstPage' && !paginatedResult.results; | |
| const hasActiveFilters = !!(search.query || search.status || search.category); | |
| const isInitialLoading = | |
| paginatedResult.status === 'LoadingFirstPage' && !paginatedResult.results; |
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@services/platform/app/routes/dashboard/`$id/_knowledge/products.tsx around
lines 53 - 56, The active-filter flag misses query-only filters: update the
hasActiveFilters computation (where hasActiveFilters is defined) to include
search.query alongside search.status and search.category so that a non-empty
search.query counts as an active filter and triggers the “no results” empty
state instead of the “no products yet” copy.
Comment on lines
+78
to
+153
| const calculateLoopWidth = (loopStepSlug: string): number => { | ||
| const BASE_WIDTH = 640; | ||
| const NODE_WIDTH = 300; | ||
| const HORIZONTAL_PADDING = 32; | ||
| const NODE_SEP = 120; | ||
| const EXTRA_MARGIN = 64; | ||
|
|
||
| const children = sortedSteps.filter((s) => { | ||
| const childParentSlug = Array.from(loopBodyMap.entries()).find( | ||
| ([, bodies]) => bodies.has(s.stepSlug), | ||
| )?.[0]; | ||
| return childParentSlug === loopStepSlug; | ||
| }); | ||
|
|
||
| if (children.length === 0) { | ||
| return BASE_WIDTH; | ||
| } | ||
|
|
||
| const nestedLoops = children.filter((child) => child.stepType === 'loop'); | ||
|
|
||
| if (nestedLoops.length === 0) { | ||
| return BASE_WIDTH; | ||
| } | ||
|
|
||
| const nestedLoopWidths = nestedLoops.map((loop) => | ||
| calculateLoopWidth(loop.stepSlug), | ||
| ); | ||
|
|
||
| const maxNestedWidth = Math.max(...nestedLoopWidths); | ||
|
|
||
| const conditionalNodes = children.filter( | ||
| (child) => child.stepType === 'condition', | ||
| ); | ||
|
|
||
| let maxBranchWidth = 0; | ||
| conditionalNodes.forEach((condNode) => { | ||
| const targets = Object.values(condNode.nextSteps); | ||
| const targetNodes = targets | ||
| .map((targetSlug) => children.find((c) => c.stepSlug === targetSlug)) | ||
| .filter(Boolean); | ||
|
|
||
| const hasLoopBranch = targetNodes.some((t) => t?.stepType === 'loop'); | ||
| const hasNonLoopBranch = targetNodes.some( | ||
| (t) => t?.stepType !== 'loop', | ||
| ); | ||
|
|
||
| if (hasLoopBranch && hasNonLoopBranch) { | ||
| const loopWidth = targetNodes | ||
| .filter((t) => t?.stepType === 'loop') | ||
| .map((t) => calculateLoopWidth(t?.stepSlug ?? '')) | ||
| .reduce((max, w) => Math.max(max, w), 0); | ||
|
|
||
| const nonLoopWidth = NODE_WIDTH; | ||
| const branchWidth = loopWidth + NODE_SEP + nonLoopWidth; | ||
| maxBranchWidth = Math.max(maxBranchWidth, branchWidth); | ||
| } | ||
| }); | ||
|
|
||
| if (maxBranchWidth > 0) { | ||
| return Math.min(maxBranchWidth + HORIZONTAL_PADDING, 1920); | ||
| } | ||
|
|
||
| if (nestedLoops.length === 1) { | ||
| return maxNestedWidth + HORIZONTAL_PADDING + EXTRA_MARGIN; | ||
| } | ||
|
|
||
| const totalNestedWidth = nestedLoopWidths.reduce((sum, w) => sum + w, 0); | ||
| const spacingBetween = (nestedLoops.length - 1) * NODE_SEP; | ||
|
|
||
| const widthForBranching = | ||
| totalNestedWidth + spacingBetween + HORIZONTAL_PADDING; | ||
| const widthForStacking = | ||
| maxNestedWidth + HORIZONTAL_PADDING + EXTRA_MARGIN; | ||
|
|
||
| return Math.min(Math.max(widthForBranching, widthForStacking), 1920); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider memoizing loop width calculations.
calculateLoopWidth is recursive and can be called repeatedly for nested loops; caching by loop slug would avoid repeated work on larger graphs.
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@services/platform/app/features/automations/hooks/use-automation-layout.ts`
around lines 78 - 153, The recursive calculateLoopWidth function recomputes
widths for the same loop slugs repeatedly; add a memoization cache (e.g., a
Map<string, number>) keyed by loop stepSlug inside use-automation-layout and
consult it at the start of calculateLoopWidth and store computed widths before
returning; ensure calls that pass '' or invalid slugs are handled (skip caching
or return BASE_WIDTH) and that recursive calls
(calculateLoopWidth(loop.stepSlug)) use the cache to avoid duplicate work.
Comment on lines
+155
to
+236
| const nodes: Node[] = sortedSteps.map((step) => { | ||
| const isLoopNode = step.stepType === 'loop'; | ||
|
|
||
| let parentLoopId: string | undefined; | ||
| const candidateLoops: string[] = []; | ||
|
|
||
| for (const [loopId, bodyNodes] of loopBodyMap.entries()) { | ||
| if (bodyNodes.has(step.stepSlug)) { | ||
| candidateLoops.push(loopId); | ||
| } | ||
| } | ||
|
|
||
| if (candidateLoops.length > 0) { | ||
| if (candidateLoops.length === 1) { | ||
| parentLoopId = candidateLoops[0]; | ||
| } else { | ||
| parentLoopId = candidateLoops.find((candidateId) => { | ||
| return candidateLoops.some((otherId) => { | ||
| return ( | ||
| otherId !== candidateId && | ||
| loopBodyMap.get(otherId)?.has(candidateId) | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| if (!parentLoopId) { | ||
| parentLoopId = candidateLoops[candidateLoops.length - 1]; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const nodeConfig: Partial<Node> = { | ||
| id: step.stepSlug, | ||
| type: isLoopNode ? 'loopContainer' : 'custom', | ||
| position: { x: 0, y: 0 }, | ||
| zIndex: parentLoopId ? 10 : 1, | ||
| data: { | ||
| label: step.name, | ||
| stepType: step.stepType, | ||
| stepSlug: step.stepSlug, | ||
| actionType: | ||
| step.stepType === 'action' && 'type' in step.config | ||
| ? String(step.config.type) | ||
| : undefined, | ||
| isLeafNode: leafStepSlugs.has(step.stepSlug), | ||
| isTerminalNode: leafStepSlugs.has(step.stepSlug), | ||
| rank: step.order, | ||
| isLoopBodyNode: !!parentLoopId, | ||
| }, | ||
| }; | ||
|
|
||
| if (isLoopNode) { | ||
| const loopWidth = calculateLoopWidth(step.stepSlug); | ||
| const topPadding = 80; | ||
| const bottomPadding = 30; | ||
| const estimatedHeight = topPadding + 200 + bottomPadding; | ||
|
|
||
| nodeConfig.width = loopWidth; | ||
| nodeConfig.height = estimatedHeight; | ||
| nodeConfig.style = { | ||
| width: loopWidth, | ||
| height: estimatedHeight, | ||
| }; | ||
| } else { | ||
| nodeConfig.width = 300; | ||
| nodeConfig.height = 80; | ||
| nodeConfig.style = { | ||
| width: 300, | ||
| height: 80, | ||
| }; | ||
| } | ||
|
|
||
| if (parentLoopId) { | ||
| nodeConfig.parentId = parentLoopId; | ||
| nodeConfig.extent = 'parent'; | ||
| nodeConfig.draggable = true; | ||
| nodeConfig.position = { x: 0, y: 0 }; | ||
| } | ||
|
|
||
| // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- ReactFlow Node requires all fields, but we build incrementally with conditional properties | ||
| return nodeConfig as Node; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd services/platform && find . -name "use-automation-layout.ts" 2>/dev/nullRepository: tale-project/tale
Length of output: 118
🏁 Script executed:
cd services/platform && wc -l ./app/features/automations/hooks/use-automation-layout.tsRepository: tale-project/tale
Length of output: 122
🏁 Script executed:
cd services/platform && sed -n '1,160p' ./app/features/automations/hooks/use-automation-layout.ts | head -100Repository: tale-project/tale
Length of output: 2836
🏁 Script executed:
cd services/platform && sed -n '155,240p' ./app/features/automations/hooks/use-automation-layout.tsRepository: tale-project/tale
Length of output: 2772
🏁 Script executed:
cd services/platform && sed -n '100,155p' ./app/features/automations/hooks/use-automation-layout.tsRepository: tale-project/tale
Length of output: 1964
🏁 Script executed:
cd services/platform && cat -n ./app/features/automations/hooks/use-automation-layout.ts | sed -n '25,75p'Repository: tale-project/tale
Length of output: 1785
🏁 Script executed:
cd services/platform && find . -name "*use-automation-layout*" -o -name "*automation*" -type f | grep -i testRepository: tale-project/tale
Length of output: 192
🏁 Script executed:
cd services/platform && rg -i "loopBodyMap\|parentLoopId" -A 3 -B 3 --type ts --type tsxRepository: tale-project/tale
Length of output: 88
🏁 Script executed:
cd services/platform && rg -i "loopBodyMap\|parentLoopId" -A 3 -B 3 --type tsRepository: tale-project/tale
Length of output: 43
🏁 Script executed:
python3 << 'EOF'
# Scenario: 3-level nested loops A > B > C with node X in all their bodies
# loopBodyMap represents what nodes are directly in each loop's body
loopBodyMap = {
'A': {'B', 'C', 'X', 'other_nodes_A'},
'B': {'C', 'X', 'other_nodes_B'},
'C': {'X', 'other_nodes_C'}
}
candidateLoops = ['A', 'B', 'C'] # All three contain X
print("=== CURRENT LOGIC ===")
print("Find first candidateId where some otherId contains candidateId")
print()
for candidateId in candidateLoops:
print(f"Checking {candidateId}:")
found = False
for otherId in candidateLoops:
if otherId != candidateId:
# Check if otherId contains candidateId
if candidateId in loopBodyMap.get(otherId, set()):
print(f" {otherId} contains {candidateId}? YES")
found = True
break
else:
print(f" {otherId} contains {candidateId}? No")
if found:
print(f" → Result: MATCH - Would return {candidateId}")
current_result = candidateId
break
else:
print(f" → Result: No match")
print(f"\nCurrent logic selects: {current_result}")
print(f"Problem: X should be in C (innermost), but gets parented to {current_result}")
print()
print("=== PROPOSED LOGIC ===")
print("Find first candidateId where every otherId equals candidateId OR candidateId doesn't contain otherId")
print()
for candidateId in candidateLoops:
print(f"Checking {candidateId}:")
passes = True
for otherId in candidateLoops:
if otherId == candidateId:
print(f" {otherId} === {candidateId}? YES")
else:
# Check if candidateId contains otherId
if otherId in loopBodyMap.get(candidateId, set()):
print(f" {otherId} in {candidateId}? YES - FAILS condition")
passes = False
break
else:
print(f" {otherId} in {candidateId}? No")
if passes:
print(f" → Result: MATCH - Would return {candidateId}")
proposed_result = candidateId
break
else:
print(f" → Result: No match")
print(f"\nProposed logic selects: {proposed_result}")
print(f"✓ Correct: X is properly nested in {proposed_result} (innermost loop)")
EOFRepository: tale-project/tale
Length of output: 840
🏁 Script executed:
python3 << 'EOF'
# Test additional scenarios to validate the proposed fix
print("=== TEST CASE 2: Two parallel loops at same level ===")
# Both A and B are direct children of root, C is in both
loopBodyMap = {
'A': {'C', 'other_A'},
'B': {'C', 'other_B'}
}
candidateLoops = ['A', 'B']
print(f"candidateLoops: {candidateLoops}")
print("Current: finds first loop that is nested - neither A nor B are nested → fallback to last = B")
print("Proposed: finds loop that doesn't contain other candidates")
for candidateId in candidateLoops:
passes = all(otherId == candidateId or otherId not in loopBodyMap.get(candidateId, set())
for otherId in candidateLoops)
print(f" {candidateId}: {passes}")
print("Result: Would return A (first that passes) - but this is ambiguous")
print("However, this scenario shouldn't happen in valid automation flow structure\n")
print("=== TEST CASE 3: 4-level nested loops ===")
loopBodyMap = {
'A': {'B', 'C', 'D', 'X'},
'B': {'C', 'D', 'X'},
'C': {'D', 'X'},
'D': {'X'}
}
candidateLoops = ['A', 'B', 'C', 'D']
print(f"candidateLoops: {candidateLoops}")
print("Proposed logic - find innermost:")
for candidateId in candidateLoops:
passes = all(otherId == candidateId or otherId not in loopBodyMap.get(candidateId, set())
for otherId in candidateLoops)
if passes:
print(f" {candidateId}: MATCH ✓")
break
else:
print(f" {candidateId}: contains other candidates - skip")
print("Result: Correctly selects D (innermost)\n")
print("=== Proposed fix syntax check ===")
print("The nullish coalescing (??) ensures fallback to last element")
print("even though the find should always succeed in valid nested loop structure")
EOFRepository: tale-project/tale
Length of output: 905
🏁 Script executed:
cd services/platform && sed -n '165,190p' ./app/features/automations/hooks/use-automation-layout.ts | cat -nRepository: tale-project/tale
Length of output: 1014
Nested loops can be attached to the wrong parent in deeply nested structures.
Lines 171–181 select the first candidate loop that is itself nested inside another loop, which picks a middle loop when there are 3+ nested loops. This mis-parents nodes and leaves the innermost loop container empty in the flow. For a 3-level nested loop scenario (A → B → C with node X in all bodies), the current logic incorrectly selects B as the parent when C (the innermost loop) should be selected, leaving C's body empty.
🔧 Suggested fix (pick the innermost loop)
- parentLoopId = candidateLoops.find((candidateId) => {
- return candidateLoops.some((otherId) => {
- return (
- otherId !== candidateId &&
- loopBodyMap.get(otherId)?.has(candidateId)
- );
- });
- });
-
- if (!parentLoopId) {
- parentLoopId = candidateLoops[candidateLoops.length - 1];
- }
+ parentLoopId =
+ candidateLoops.find((candidateId) => {
+ return candidateLoops.every((otherId) => {
+ return (
+ otherId === candidateId ||
+ !loopBodyMap.get(candidateId)?.has(otherId)
+ );
+ });
+ }) ?? candidateLoops[candidateLoops.length - 1];🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@services/platform/app/features/automations/hooks/use-automation-layout.ts`
around lines 155 - 236, The issue is that parentLoopId selection picks a middle
loop when multiple candidate loops exist; change the selection to pick the
innermost loop (the candidate that does NOT contain any other candidate loop in
its body). In the node-building code where candidateLoops is computed, replace
the current candidate selection (the block computing parentLoopId when
candidateLoops.length > 1) with logic that picks the candidateId for which
loopBodyMap.get(candidateId) does not contain any other candidateId; if none
match, fall back to the last candidate in candidateLoops. Update references to
candidateLoops, parentLoopId, and loopBodyMap in use-automation-layout.ts
accordingly.
Comment on lines
+238
to
+416
| const edges: Edge[] = []; | ||
| sortedSteps.forEach((step) => { | ||
| if (step.nextSteps && typeof step.nextSteps === 'object') { | ||
| Object.entries(step.nextSteps).forEach(([key, targetStepSlug]) => { | ||
| if (sortedSteps.find((s) => s.stepSlug === targetStepSlug)) { | ||
| const keyLower = key.toLowerCase(); | ||
|
|
||
| let targetIsChildOfSource = false; | ||
| for (const [loopStepSlug, bodyNodes] of loopBodyMap.entries()) { | ||
| if ( | ||
| loopStepSlug === step.stepSlug && | ||
| bodyNodes.has(targetStepSlug) | ||
| ) { | ||
| targetIsChildOfSource = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (targetIsChildOfSource) { | ||
| return; | ||
| } | ||
|
|
||
| const isLoopExit = [ | ||
| 'done', | ||
| 'complete', | ||
| 'finished', | ||
| 'exit', | ||
| ].includes(keyLower); | ||
|
|
||
| const isNegativePath = [ | ||
| 'reject', | ||
| 'false', | ||
| 'no', | ||
| 'failure', | ||
| 'error', | ||
| ].includes(keyLower); | ||
|
|
||
| const isPositivePath = [ | ||
| 'approve', | ||
| 'true', | ||
| 'yes', | ||
| 'success', | ||
| 'default', | ||
| ].includes(keyLower); | ||
|
|
||
| let edgeColor = '#9CA3AF'; | ||
| let edgeLabel: string | undefined = undefined; | ||
| let edgeStyle: React.CSSProperties = { | ||
| strokeWidth: 1.5, | ||
| stroke: edgeColor, | ||
| }; | ||
|
|
||
| if (step.stepType === 'loop') { | ||
| if (isLoopExit) { | ||
| edgeColor = 'hsl(var(--chart-2))'; | ||
| edgeLabel = undefined; | ||
| edgeStyle = { strokeWidth: 2, stroke: edgeColor }; | ||
| } else { | ||
| edgeLabel = undefined; | ||
| } | ||
| } else if (step.stepType === 'condition') { | ||
| if (isNegativePath) { | ||
| edgeColor = 'hsl(var(--destructive))'; | ||
| edgeLabel = 'false'; | ||
| } else if (isPositivePath) { | ||
| edgeColor = 'hsl(var(--chart-2))'; | ||
| edgeLabel = 'true'; | ||
| } | ||
| edgeStyle = { strokeWidth: 1.5, stroke: edgeColor }; | ||
| } else if (step.stepType === 'action' || step.stepType === 'llm') { | ||
| if (isNegativePath) { | ||
| edgeColor = 'hsl(var(--destructive))'; | ||
| } else if (isPositivePath) { | ||
| edgeColor = 'hsl(var(--chart-2))'; | ||
| } | ||
| edgeStyle = { strokeWidth: 1.5, stroke: edgeColor }; | ||
| } | ||
|
|
||
| const targetIsLoop = | ||
| sortedSteps.find((s) => s.stepSlug === targetStepSlug) | ||
| ?.stepType === 'loop'; | ||
|
|
||
| const sourceStep = sortedSteps.find( | ||
| (s) => s.stepSlug === step.stepSlug, | ||
| ); | ||
| const targetStepData = sortedSteps.find( | ||
| (s) => s.stepSlug === targetStepSlug, | ||
| ); | ||
|
|
||
| let sourceHandle = 'bottom-source'; | ||
| let targetHandle = 'top-target'; | ||
| let edgeType: 'smoothstep' | 'default' = 'smoothstep'; | ||
|
|
||
| const isBackwardConnection = | ||
| targetStepData && | ||
| sourceStep && | ||
| targetStepData.order < sourceStep.order; | ||
|
|
||
| if (isBackwardConnection) { | ||
| sourceHandle = 'right-source'; | ||
| targetHandle = 'left-target'; | ||
| edgeType = 'smoothstep'; | ||
| } | ||
|
|
||
| let bothAreChildNodes = false; | ||
| for (const [, bodyNodes] of loopBodyMap.entries()) { | ||
| if ( | ||
| bodyNodes.has(step.stepSlug) && | ||
| bodyNodes.has(targetStepSlug) | ||
| ) { | ||
| bothAreChildNodes = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| const sourceIsLoop = step.stepType === 'loop'; | ||
| const involvesLoopNode = sourceIsLoop || targetIsLoop; | ||
|
|
||
| if (targetIsLoop && (isBackwardConnection || isNegativePath)) { | ||
| return; | ||
| } | ||
|
|
||
| edges.push({ | ||
| id: `e${step.stepSlug}-${targetStepSlug}-${key}`, | ||
| type: edgeType, | ||
| source: step.stepSlug, | ||
| target: targetStepSlug, | ||
| sourceHandle, | ||
| targetHandle, | ||
| zIndex: bothAreChildNodes | ||
| ? 10 | ||
| : isBackwardConnection | ||
| ? -3 | ||
| : involvesLoopNode | ||
| ? -1 | ||
| : isNegativePath || isPositivePath | ||
| ? -1 | ||
| : -2, | ||
| markerEnd: { | ||
| type: MarkerType.Arrow, | ||
| strokeWidth: 1.5, | ||
| color: edgeColor, | ||
| }, | ||
| style: { | ||
| ...edgeStyle, | ||
| ...(isBackwardConnection | ||
| ? { | ||
| strokeDasharray: '5,5', | ||
| opacity: 0.7, | ||
| strokeWidth: 2, | ||
| } | ||
| : {}), | ||
| }, | ||
| animated: !isBackwardConnection, | ||
| data: { | ||
| isBackward: isBackwardConnection, | ||
| label: edgeLabel, | ||
| labelStyle: edgeLabel | ||
| ? { | ||
| fill: edgeColor, | ||
| fontSize: '11px', | ||
| fontWeight: 600, | ||
| } | ||
| : undefined, | ||
| labelBgStyle: edgeLabel | ||
| ? { | ||
| fill: 'hsl(var(--background))', | ||
| stroke: edgeColor, | ||
| strokeWidth: 1.5, | ||
| } | ||
| : undefined, | ||
| isBackwardConnection, | ||
| }, | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reduce repeated linear lookups during edge construction.
This block calls sortedSteps.find(...) multiple times per edge; building a Map<stepSlug, step> once would drop lookups to O(1) and simplify the flow for larger automations.
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@services/platform/app/features/automations/hooks/use-automation-layout.ts`
around lines 238 - 416, The code repeatedly calls sortedSteps.find(...) during
edge construction which is O(n) per lookup; create a Map keyed by step.stepSlug
(e.g., stepBySlug = new Map(sortedSteps.map(s => [s.stepSlug, s]))) before the
forEach and replace all sortedSteps.find(...) uses (for sourceStep,
targetStepData, targetIsLoop checks, and any later lookups) with
stepBySlug.get(stepSlug); also compute targetIsLoop and sourceStep once from the
map, remove duplicate lookups inside the nested Object.entries loop, and keep
existing logic that uses loopBodyMap, edges.push and MarkerType unchanged.
Comment on lines
+417
to
+480
| const incomingCounts = new Map<string, number>(); | ||
| const outgoingCounts = new Map<string, number>(); | ||
| const topHandlesUsed = new Map<string, Set<string>>(); | ||
| const bottomHandlesUsed = new Map<string, Set<string>>(); | ||
|
|
||
| edges.forEach((edge) => { | ||
| incomingCounts.set( | ||
| edge.target, | ||
| (incomingCounts.get(edge.target) || 0) + 1, | ||
| ); | ||
| outgoingCounts.set( | ||
| edge.source, | ||
| (outgoingCounts.get(edge.source) || 0) + 1, | ||
| ); | ||
|
|
||
| if (edge.sourceHandle) { | ||
| const nodeHandles = edge.sourceHandle.startsWith('top-') | ||
| ? topHandlesUsed | ||
| : bottomHandlesUsed; | ||
| if (!nodeHandles.has(edge.source)) { | ||
| nodeHandles.set(edge.source, new Set()); | ||
| } | ||
| nodeHandles.get(edge.source)?.add(edge.sourceHandle); | ||
| } | ||
|
|
||
| if (edge.targetHandle) { | ||
| const nodeHandles = edge.targetHandle.startsWith('top-') | ||
| ? topHandlesUsed | ||
| : bottomHandlesUsed; | ||
| if (!nodeHandles.has(edge.target)) { | ||
| nodeHandles.set(edge.target, new Set()); | ||
| } | ||
| nodeHandles.get(edge.target)?.add(edge.targetHandle); | ||
| } | ||
| }); | ||
|
|
||
| const hasBidirectionalTop = new Map<string, boolean>(); | ||
| const hasBidirectionalBottom = new Map<string, boolean>(); | ||
|
|
||
| nodes.forEach((node: Node) => { | ||
| const topHandles = topHandlesUsed.get(node.id) || new Set(); | ||
| hasBidirectionalTop.set( | ||
| node.id, | ||
| topHandles.has('top-target') && topHandles.has('top-source'), | ||
| ); | ||
|
|
||
| const bottomHandles = bottomHandlesUsed.get(node.id) || new Set(); | ||
| hasBidirectionalBottom.set( | ||
| node.id, | ||
| bottomHandles.has('bottom-target') && | ||
| bottomHandles.has('bottom-source'), | ||
| ); | ||
| }); | ||
|
|
||
| const nodesWithFullConnectionData = nodes.map((node: Node) => ({ | ||
| ...node, | ||
| data: { | ||
| ...node.data, | ||
| incomingCount: incomingCounts.get(node.id) || 0, | ||
| outgoingCount: outgoingCounts.get(node.id) || 0, | ||
| hasBidirectionalTop: hasBidirectionalTop.get(node.id) || false, | ||
| hasBidirectionalBottom: hasBidirectionalBottom.get(node.id) || false, | ||
| }, | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=tsx -C3 'hasBidirectionalTop|hasBidirectionalBottom' services/platform/appRepository: tale-project/tale
Length of output: 88
🏁 Script executed:
# Also search for left/right handle definitions and usage
rg -n 'left-|right-' services/platform/app --type=tsx --type=ts -C2Repository: tale-project/tale
Length of output: 88
🏁 Script executed:
# Check the file context around the problematic code
cat -n services/platform/app/features/automations/hooks/use-automation-layout.ts | sed -n '410,485p'Repository: tale-project/tale
Length of output: 2907
🏁 Script executed:
rg -n 'hasBidirectionalTop|hasBidirectionalBottom' services/platform/app -t ts -C3Repository: tale-project/tale
Length of output: 7400
🏁 Script executed:
rg -n 'left-|right-' services/platform/app -t ts -C2Repository: tale-project/tale
Length of output: 39310
🏁 Script executed:
# Check for handle definitions to understand what handle types are used
rg -n "Handle|handle" services/platform/app/features/automations/hooks/use-automation-layout.ts -C1 | head -80Repository: tale-project/tale
Length of output: 2028
Left/right handles are miscounted as bottom handles, affecting render positioning.
Lines 433/443 treat any non-top handle as bottom, so left-target and right-source (used for backward connections) contribute to hasBidirectionalBottom. Since these flags control handle positioning in automation-step.tsx (lines 41-44) and automation-loop-container.tsx (lines 30-33), nodes with only horizontal edges will have incorrect offsets.
Explicitly check for 'bottom-' prefix instead of defaulting to it:
Suggested fix
if (edge.sourceHandle) {
- const nodeHandles = edge.sourceHandle.startsWith('top-')
- ? topHandlesUsed
- : bottomHandlesUsed;
- if (!nodeHandles.has(edge.source)) {
- nodeHandles.set(edge.source, new Set());
- }
- nodeHandles.get(edge.source)?.add(edge.sourceHandle);
+ const isTop = edge.sourceHandle.startsWith('top-');
+ const isBottom = edge.sourceHandle.startsWith('bottom-');
+ if (isTop || isBottom) {
+ const nodeHandles = isTop ? topHandlesUsed : bottomHandlesUsed;
+ if (!nodeHandles.has(edge.source)) {
+ nodeHandles.set(edge.source, new Set());
+ }
+ nodeHandles.get(edge.source)?.add(edge.sourceHandle);
+ }
}
if (edge.targetHandle) {
- const nodeHandles = edge.targetHandle.startsWith('top-')
- ? topHandlesUsed
- : bottomHandlesUsed;
- if (!nodeHandles.has(edge.target)) {
- nodeHandles.set(edge.target, new Set());
- }
- nodeHandles.get(edge.target)?.add(edge.targetHandle);
+ const isTop = edge.targetHandle.startsWith('top-');
+ const isBottom = edge.targetHandle.startsWith('bottom-');
+ if (isTop || isBottom) {
+ const nodeHandles = isTop ? topHandlesUsed : bottomHandlesUsed;
+ if (!nodeHandles.has(edge.target)) {
+ nodeHandles.set(edge.target, new Set());
+ }
+ nodeHandles.get(edge.target)?.add(edge.targetHandle);
+ }
}📝 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 incomingCounts = new Map<string, number>(); | |
| const outgoingCounts = new Map<string, number>(); | |
| const topHandlesUsed = new Map<string, Set<string>>(); | |
| const bottomHandlesUsed = new Map<string, Set<string>>(); | |
| edges.forEach((edge) => { | |
| incomingCounts.set( | |
| edge.target, | |
| (incomingCounts.get(edge.target) || 0) + 1, | |
| ); | |
| outgoingCounts.set( | |
| edge.source, | |
| (outgoingCounts.get(edge.source) || 0) + 1, | |
| ); | |
| if (edge.sourceHandle) { | |
| const nodeHandles = edge.sourceHandle.startsWith('top-') | |
| ? topHandlesUsed | |
| : bottomHandlesUsed; | |
| if (!nodeHandles.has(edge.source)) { | |
| nodeHandles.set(edge.source, new Set()); | |
| } | |
| nodeHandles.get(edge.source)?.add(edge.sourceHandle); | |
| } | |
| if (edge.targetHandle) { | |
| const nodeHandles = edge.targetHandle.startsWith('top-') | |
| ? topHandlesUsed | |
| : bottomHandlesUsed; | |
| if (!nodeHandles.has(edge.target)) { | |
| nodeHandles.set(edge.target, new Set()); | |
| } | |
| nodeHandles.get(edge.target)?.add(edge.targetHandle); | |
| } | |
| }); | |
| const hasBidirectionalTop = new Map<string, boolean>(); | |
| const hasBidirectionalBottom = new Map<string, boolean>(); | |
| nodes.forEach((node: Node) => { | |
| const topHandles = topHandlesUsed.get(node.id) || new Set(); | |
| hasBidirectionalTop.set( | |
| node.id, | |
| topHandles.has('top-target') && topHandles.has('top-source'), | |
| ); | |
| const bottomHandles = bottomHandlesUsed.get(node.id) || new Set(); | |
| hasBidirectionalBottom.set( | |
| node.id, | |
| bottomHandles.has('bottom-target') && | |
| bottomHandles.has('bottom-source'), | |
| ); | |
| }); | |
| const nodesWithFullConnectionData = nodes.map((node: Node) => ({ | |
| ...node, | |
| data: { | |
| ...node.data, | |
| incomingCount: incomingCounts.get(node.id) || 0, | |
| outgoingCount: outgoingCounts.get(node.id) || 0, | |
| hasBidirectionalTop: hasBidirectionalTop.get(node.id) || false, | |
| hasBidirectionalBottom: hasBidirectionalBottom.get(node.id) || false, | |
| }, | |
| })); | |
| const incomingCounts = new Map<string, number>(); | |
| const outgoingCounts = new Map<string, number>(); | |
| const topHandlesUsed = new Map<string, Set<string>>(); | |
| const bottomHandlesUsed = new Map<string, Set<string>>(); | |
| edges.forEach((edge) => { | |
| incomingCounts.set( | |
| edge.target, | |
| (incomingCounts.get(edge.target) || 0) + 1, | |
| ); | |
| outgoingCounts.set( | |
| edge.source, | |
| (outgoingCounts.get(edge.source) || 0) + 1, | |
| ); | |
| if (edge.sourceHandle) { | |
| const isTop = edge.sourceHandle.startsWith('top-'); | |
| const isBottom = edge.sourceHandle.startsWith('bottom-'); | |
| if (isTop || isBottom) { | |
| const nodeHandles = isTop ? topHandlesUsed : bottomHandlesUsed; | |
| if (!nodeHandles.has(edge.source)) { | |
| nodeHandles.set(edge.source, new Set()); | |
| } | |
| nodeHandles.get(edge.source)?.add(edge.sourceHandle); | |
| } | |
| } | |
| if (edge.targetHandle) { | |
| const isTop = edge.targetHandle.startsWith('top-'); | |
| const isBottom = edge.targetHandle.startsWith('bottom-'); | |
| if (isTop || isBottom) { | |
| const nodeHandles = isTop ? topHandlesUsed : bottomHandlesUsed; | |
| if (!nodeHandles.has(edge.target)) { | |
| nodeHandles.set(edge.target, new Set()); | |
| } | |
| nodeHandles.get(edge.target)?.add(edge.targetHandle); | |
| } | |
| } | |
| }); | |
| const hasBidirectionalTop = new Map<string, boolean>(); | |
| const hasBidirectionalBottom = new Map<string, boolean>(); | |
| nodes.forEach((node: Node) => { | |
| const topHandles = topHandlesUsed.get(node.id) || new Set(); | |
| hasBidirectionalTop.set( | |
| node.id, | |
| topHandles.has('top-target') && topHandles.has('top-source'), | |
| ); | |
| const bottomHandles = bottomHandlesUsed.get(node.id) || new Set(); | |
| hasBidirectionalBottom.set( | |
| node.id, | |
| bottomHandles.has('bottom-target') && | |
| bottomHandles.has('bottom-source'), | |
| ); | |
| }); | |
| const nodesWithFullConnectionData = nodes.map((node: Node) => ({ | |
| ...node, | |
| data: { | |
| ...node.data, | |
| incomingCount: incomingCounts.get(node.id) || 0, | |
| outgoingCount: outgoingCounts.get(node.id) || 0, | |
| hasBidirectionalTop: hasBidirectionalTop.get(node.id) || false, | |
| hasBidirectionalBottom: hasBidirectionalBottom.get(node.id) || false, | |
| }, | |
| })); |
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@services/platform/app/features/automations/hooks/use-automation-layout.ts`
around lines 417 - 480, The bug is that edge.sourceHandle/edge.targetHandle
non-top handles are treated as bottom by default, causing left/right handles to
be counted as bottom; update the handle classification in the loop that
populates topHandlesUsed and bottomHandlesUsed so you only add to
bottomHandlesUsed when the handle startsWith('bottom-') (and otherwise ignore
handles that startWith('left-') or 'right-'), applying this change for both the
sourceHandle and targetHandle branches; after that the hasBidirectionalBottom
map computed from bottomHandlesUsed and the nodesWithFullConnectionData usage
will reflect only actual bottom handles.
Comment on lines
+60
to
+63
| onClearUpdate={() => { | ||
| manage.setParsedUpdate(null); | ||
| manage.setUpdateParseError(null); | ||
| }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider encapsulating inline handlers in the hook.
Multiple inline handlers directly manipulate hook state (setParsedUpdate, setCredentials, setSqlConfig, setOAuth2Fields). This exposes internal state management to the component and creates multiple arrow function allocations on each render.
Moving these handlers into useIntegrationManage would:
- Improve encapsulation
- Reduce re-render allocations
- Keep state transitions in one place
♻️ Example: Move handlers to hook
In use-integration-manage.ts:
// Add these to the hook's return object
handleClearUpdate: () => {
setParsedUpdate(null);
setUpdateParseError(null);
},
handleAuthMethodChange: (value: string) => {
const method = supportedMethods.find((m) => m === value);
if (method) {
setSelectedAuthMethod(method);
setCredentials({});
setTestResult(null);
}
},
handleCredentialChange: (key: string, value: string) => {
setCredentials((prev) => ({ ...prev, [key]: value }));
},
// ... similar for other handlersThen in the dialog:
- onClearUpdate={() => {
- manage.setParsedUpdate(null);
- manage.setUpdateParseError(null);
- }}
+ onClearUpdate={manage.handleClearUpdate}Also applies to: 103-109, 111-119
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In
`@services/platform/app/features/settings/integrations/components/integration-manage-dialog.tsx`
around lines 60 - 63, The component currently uses inline handlers that directly
call hook setters (e.g. setParsedUpdate, setUpdateParseError, setCredentials,
setSqlConfig, setOAuth2Fields, setSelectedAuthMethod, setTestResult) which
should be encapsulated inside useIntegrationManage; add methods like
handleClearUpdate, handleAuthMethodChange, handleCredentialChange,
handleSqlConfigChange and handleOAuth2FieldsChange to the hook (they should call
the existing setters and perform related resets like clearing test results),
return these handlers from useIntegrationManage, and replace the inline arrow
functions in integration-manage-dialog.tsx (the onClearUpdate and the handlers
referenced around lines 103–119 in the review) with the corresponding hook
methods to reduce allocations and centralize state transitions.
Comment on lines
+49
to
+101
| <Select | ||
| value={rule.source} | ||
| onValueChange={(value) => | ||
| onUpdate(index, { | ||
| // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- Radix Select onValueChange returns string | ||
| source: value as 'jobTitle' | 'appRole', | ||
| }) | ||
| } | ||
| disabled={disabled} | ||
| className="w-28 shrink-0" | ||
| options={[ | ||
| { | ||
| value: 'jobTitle', | ||
| label: t('integrations.sso.sourceJobTitle'), | ||
| }, | ||
| { | ||
| value: 'appRole', | ||
| label: t('integrations.sso.sourceAppRole'), | ||
| }, | ||
| ]} | ||
| /> | ||
|
|
||
| <Input | ||
| placeholder="*developer*" | ||
| value={rule.pattern} | ||
| onChange={(e) => onUpdate(index, { pattern: e.target.value })} | ||
| disabled={disabled} | ||
| className="min-w-32 flex-1" | ||
| /> | ||
|
|
||
| <Select | ||
| value={rule.targetRole} | ||
| onValueChange={(value) => | ||
| onUpdate(index, { | ||
| // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- Radix Select onValueChange returns string | ||
| targetRole: value as PlatformRole, | ||
| }) | ||
| } | ||
| disabled={disabled} | ||
| className="w-28 shrink-0" | ||
| options={platformRoles} | ||
| /> | ||
|
|
||
| <Button | ||
| type="button" | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={() => onRemove(index)} | ||
| disabled={disabled} | ||
| className="shrink-0" | ||
| > | ||
| <Trash2 className="h-4 w-4" /> | ||
| </Button> |
There was a problem hiding this comment.
Add accessible labels for rule controls and the icon-only delete button.
The selects and pattern input have no accessible name, and the Trash-only action lacks an aria-label, which is a WCAG blocker for screen readers. Add visible or sr-only labels (you may need to extend Select to pass an accessible name to the trigger) and label the delete action.
✏️ Possible starting point
- <Input
+ <Input
+ aria-label={t('integrations.sso.rulePattern')}
placeholder="*developer*"
value={rule.pattern}
onChange={(e) => onUpdate(index, { pattern: e.target.value })}
disabled={disabled}
className="min-w-32 flex-1"
/>
@@
- <Button
+ <Button
+ aria-label={t('integrations.sso.removeRule')}
type="button"
variant="ghost"
size="icon"
onClick={() => onRemove(index)}
disabled={disabled}
className="shrink-0"
>🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In
`@services/platform/app/features/settings/integrations/components/sso-config/role-mapping-section.tsx`
around lines 49 - 101, Add accessible names to the two Selects, the Input, and
the icon-only delete Button: update the Select component usage (first Select for
source, second Select for targetRole) to pass an accessible name/label prop (or
extend the Select component to accept and forward aria-label/aria-labelledby to
its trigger), add a visible or sr-only label element tied to the Input for
pattern (rule.pattern) and ensure onChange still calls onUpdate, and add an
aria-label (e.g. "Delete role mapping") to the Button that calls onRemove(index)
so the Trash2 icon is announced; keep existing props (value, onValueChange,
disabled, options) unchanged and only add accessible labeling.
Comment on lines
+59
to
+68
| options={[ | ||
| { | ||
| value: 'jobTitle', | ||
| label: t('integrations.sso.sourceJobTitle'), | ||
| }, | ||
| { | ||
| value: 'appRole', | ||
| label: t('integrations.sso.sourceAppRole'), | ||
| }, | ||
| ]} |
There was a problem hiding this comment.
Expose group as a role‑mapping source.
Limiting sources to jobTitle/appRole blocks group-based mappings and prevents editing existing group rules.
✏️ Suggested change
options={[
{
value: 'jobTitle',
label: t('integrations.sso.sourceJobTitle'),
},
{
value: 'appRole',
label: t('integrations.sso.sourceAppRole'),
},
+ {
+ value: 'group',
+ label: t('integrations.sso.sourceGroup'),
+ },
]}Based on learnings: In services/platform/convex/sso_providers/entra_id/role_mapping.ts, the 'claim' source type is reserved for future work; the initial release supports jobTitle, appRole, and group role mapping.
📝 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
| options={[ | |
| { | |
| value: 'jobTitle', | |
| label: t('integrations.sso.sourceJobTitle'), | |
| }, | |
| { | |
| value: 'appRole', | |
| label: t('integrations.sso.sourceAppRole'), | |
| }, | |
| ]} | |
| options={[ | |
| { | |
| value: 'jobTitle', | |
| label: t('integrations.sso.sourceJobTitle'), | |
| }, | |
| { | |
| value: 'appRole', | |
| label: t('integrations.sso.sourceAppRole'), | |
| }, | |
| { | |
| value: 'group', | |
| label: t('integrations.sso.sourceGroup'), | |
| }, | |
| ]} |
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In
`@services/platform/app/features/settings/integrations/components/sso-config/role-mapping-section.tsx`
around lines 59 - 68, The role-mapping source options currently only include
'jobTitle' and 'appRole', which prevents group-based mappings; update the
options passed to the Select (in role-mapping-section.tsx / the options array)
to include an entry with value 'group' and label
t('integrations.sso.sourceGroup') so group-based mappings can be created and
existing group rules edited; ensure the new option follows the same shape as the
others and use the same i18n key pattern for the label.
Comment on lines
+216
to
+235
| const handleIconUpload = useCallback( | ||
| async (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const file = e.target.files?.[0]; | ||
| if (!file) return; | ||
|
|
||
| if (file.size > MAX_ICON_SIZE) { | ||
| toast({ | ||
| title: t('integrations.upload.iconTooLarge'), | ||
| variant: 'destructive', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (!ACCEPTED_ICON_TYPES.has(file.type)) { | ||
| toast({ | ||
| title: t('integrations.upload.invalidIconFormat'), | ||
| variant: 'destructive', | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Clear the file input on rejected uploads so the same file can be reselected.
Early returns for size/type errors leave the input value intact, which can prevent onChange from firing when the user picks the same file again.
✏️ Suggested change
if (file.size > MAX_ICON_SIZE) {
toast({
title: t('integrations.upload.iconTooLarge'),
variant: 'destructive',
});
+ if (iconInputRef.current) {
+ iconInputRef.current.value = '';
+ }
return;
}
@@
if (!ACCEPTED_ICON_TYPES.has(file.type)) {
toast({
title: t('integrations.upload.invalidIconFormat'),
variant: 'destructive',
});
+ if (iconInputRef.current) {
+ iconInputRef.current.value = '';
+ }
return;
}🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In
`@services/platform/app/features/settings/integrations/hooks/use-integration-manage.ts`
around lines 216 - 235, The handleIconUpload callback currently returns early on
invalid size/type but leaves the file input value set, preventing onChange when
a user reselects the same file; update handleIconUpload to clear the file input
(e.g., set e.currentTarget.value = '' or e.target.value = '') immediately before
each early return for the MAX_ICON_SIZE and ACCEPTED_ICON_TYPES checks so the
input is reset and the same file can be selected again.
Comment on lines
+31
to
+160
| const DEFAULT_MAPPING_RULES: RoleMappingRule[] = [ | ||
| { source: 'jobTitle', pattern: '*admin*', targetRole: 'admin' }, | ||
| { source: 'jobTitle', pattern: '*manager*', targetRole: 'admin' }, | ||
| { source: 'jobTitle', pattern: '*developer*', targetRole: 'developer' }, | ||
| { source: 'jobTitle', pattern: '*engineer*', targetRole: 'developer' }, | ||
| { source: 'jobTitle', pattern: '*editor*', targetRole: 'editor' }, | ||
| ]; | ||
|
|
||
| interface UseSsoConfigFormParams { | ||
| open?: boolean; | ||
| onOpenChange?: (open: boolean) => void; | ||
| organizationId: string; | ||
| existingProvider?: SsoProvider | null; | ||
| } | ||
|
|
||
| export function useSsoConfigForm({ | ||
| open, | ||
| onOpenChange, | ||
| organizationId, | ||
| existingProvider, | ||
| }: UseSsoConfigFormParams) { | ||
| const { t } = useT('settings'); | ||
| const { t: tCommon } = useT('common'); | ||
|
|
||
| const platformRoles: { value: PlatformRole; label: string }[] = useMemo( | ||
| () => [ | ||
| { value: 'admin', label: t('integrations.sso.roleAdmin') }, | ||
| { value: 'developer', label: t('integrations.sso.roleDeveloper') }, | ||
| { value: 'editor', label: t('integrations.sso.roleEditor') }, | ||
| { value: 'member', label: t('integrations.sso.roleMember') }, | ||
| { value: 'disabled', label: t('integrations.sso.roleDisabled') }, | ||
| ], | ||
| [t], | ||
| ); | ||
|
|
||
| const [issuer, setIssuer] = useState(''); | ||
| const [clientId, setClientId] = useState(''); | ||
| const [clientSecret, setClientSecret] = useState(''); | ||
| const [autoProvisionTeam, setAutoProvisionTeam] = useState(true); | ||
| const [excludeGroups, setExcludeGroups] = useState(''); | ||
| const [autoProvisionRole, setAutoProvisionRole] = useState(true); | ||
| const [roleMappingRules, setRoleMappingRules] = useState<RoleMappingRule[]>( | ||
| DEFAULT_MAPPING_RULES, | ||
| ); | ||
| const [defaultRole, setDefaultRole] = useState<PlatformRole>('member'); | ||
| const [enableOneDriveAccess, setEnableOneDriveAccess] = useState(false); | ||
| const [testResult, setTestResult] = useState<{ | ||
| valid: boolean; | ||
| error?: string; | ||
| } | null>(null); | ||
|
|
||
| const originalConfigRef = useRef<{ | ||
| issuer: string; | ||
| clientId: string; | ||
| autoProvisionTeam: boolean; | ||
| excludeGroups: string; | ||
| autoProvisionRole: boolean; | ||
| roleMappingRules: RoleMappingRule[]; | ||
| defaultRole: PlatformRole; | ||
| enableOneDriveAccess: boolean; | ||
| } | null>(null); | ||
|
|
||
| const { mutateAsync: upsertSSOProvider, isPending: isUpserting } = | ||
| useUpsertSsoProvider(); | ||
| const { mutateAsync: removeSSOProvider, isPending: isRemoving } = | ||
| useRemoveSsoProvider(); | ||
| const { mutateAsync: getFullConfig, isPending: isLoadingConfig } = | ||
| useSsoFullConfig(); | ||
| const { mutateAsync: testSSOConfig, isPending: isTestingNew } = | ||
| useTestSsoConfig(); | ||
| const { mutateAsync: testExistingSSOConfig, isPending: isTestingExisting } = | ||
| useTestExistingSsoConfig(); | ||
|
|
||
| const isSubmitting = isUpserting || isRemoving; | ||
| const isTesting = isTestingNew || isTestingExisting; | ||
| const isConnected = !!existingProvider; | ||
|
|
||
| const hasChanges = useMemo(() => { | ||
| if (clientSecret) return true; | ||
| if (!originalConfigRef.current) return false; | ||
|
|
||
| const orig = originalConfigRef.current; | ||
| const basicFieldsChanged = | ||
| issuer !== orig.issuer || | ||
| clientId !== orig.clientId || | ||
| autoProvisionTeam !== orig.autoProvisionTeam || | ||
| excludeGroups !== orig.excludeGroups || | ||
| autoProvisionRole !== orig.autoProvisionRole || | ||
| defaultRole !== orig.defaultRole || | ||
| enableOneDriveAccess !== orig.enableOneDriveAccess; | ||
|
|
||
| if (basicFieldsChanged) return true; | ||
| if (roleMappingRules.length !== orig.roleMappingRules.length) return true; | ||
|
|
||
| return roleMappingRules.some((curr, i) => { | ||
| const origRule = orig.roleMappingRules[i]; | ||
| return ( | ||
| curr.source !== origRule.source || | ||
| curr.pattern !== origRule.pattern || | ||
| curr.targetRole !== origRule.targetRole | ||
| ); | ||
| }); | ||
| }, [ | ||
| issuer, | ||
| clientId, | ||
| clientSecret, | ||
| autoProvisionTeam, | ||
| excludeGroups, | ||
| autoProvisionRole, | ||
| roleMappingRules, | ||
| defaultRole, | ||
| enableOneDriveAccess, | ||
| ]); | ||
|
|
||
| useEffect(() => { | ||
| if (open && existingProvider) { | ||
| getFullConfig({}) | ||
| .then((config) => { | ||
| if (config) { | ||
| const entraFeatures = config.providerFeatures?.entraId; | ||
| const excludeGroupsStr = (entraFeatures?.excludeGroups || []).join( | ||
| ', ', | ||
| ); | ||
| const rules = | ||
| config.roleMappingRules.length > 0 | ||
| ? config.roleMappingRules.filter( | ||
| (r: RoleMappingRule) => | ||
| r.source === 'jobTitle' || r.source === 'appRole', | ||
| ) | ||
| : DEFAULT_MAPPING_RULES; |
There was a problem hiding this comment.
Preserve supported role‑mapping sources (including group) to avoid dropping existing rules.
Filtering to jobTitle/appRole discards group rules; saving then loses them. Keep group (and only exclude claim) and update defaults/UI accordingly.
✏️ Suggested change
- const rules =
- config.roleMappingRules.length > 0
- ? config.roleMappingRules.filter(
- (r: RoleMappingRule) =>
- r.source === 'jobTitle' || r.source === 'appRole',
- )
- : DEFAULT_MAPPING_RULES;
+ const rules =
+ config.roleMappingRules.length > 0
+ ? config.roleMappingRules.filter(
+ (r: RoleMappingRule) =>
+ r.source === 'jobTitle' ||
+ r.source === 'appRole' ||
+ r.source === 'group',
+ )
+ : DEFAULT_MAPPING_RULES;Based on learnings: In services/platform/convex/sso_providers/entra_id/role_mapping.ts, the 'claim' source type is reserved for future work; the initial release supports jobTitle, appRole, and group role mapping.
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In
`@services/platform/app/features/settings/integrations/hooks/use-sso-config-form.ts`
around lines 31 - 160, The role-mapping filter in useSsoConfigForm (inside the
useEffect handling getFullConfig) incorrectly drops rules with source 'group' by
only allowing 'jobTitle' and 'appRole'; update that filter to preserve 'group'
as well and explicitly exclude only 'claim' so existing group mappings are not
lost, and ensure DEFAULT_MAPPING_RULES and any UI defaults/validation referring
to roleMappingRules still support 'group' as a valid source (identify symbols:
useSsoConfigForm, DEFAULT_MAPPING_RULES, roleMappingRules, getFullConfig).
Comment on lines
+207
to
+321
| const handleSave = useCallback(async () => { | ||
| const requiresSecret = !isConnected; | ||
| if (!issuer || !clientId || (requiresSecret && !clientSecret)) { | ||
| toast({ | ||
| title: t('integrations.sso.validationError'), | ||
| description: t('integrations.sso.allFieldsRequired'), | ||
| variant: 'destructive', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await upsertSSOProvider({ | ||
| organizationId, | ||
| providerId: 'entra-id', | ||
| issuer, | ||
| clientId, | ||
| clientSecret, | ||
| scopes: DEFAULT_SCOPES, | ||
| autoProvisionRole, | ||
| roleMappingRules, | ||
| defaultRole, | ||
| providerFeatures: { | ||
| entraId: { | ||
| enableOneDriveAccess, | ||
| autoProvisionTeam, | ||
| excludeGroups: excludeGroups | ||
| .split(',') | ||
| .map((g) => g.trim()) | ||
| .filter(Boolean), | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| toast({ | ||
| title: isConnected | ||
| ? t('integrations.sso.updateSuccessful') | ||
| : t('integrations.sso.configureSuccessful'), | ||
| description: t('integrations.sso.ssoConfigured'), | ||
| variant: 'success', | ||
| }); | ||
|
|
||
| onOpenChange?.(false); | ||
| } catch (error) { | ||
| toast({ | ||
| title: t('integrations.sso.configureFailed'), | ||
| description: | ||
| error instanceof Error | ||
| ? error.message | ||
| : t('integrations.sso.configureError'), | ||
| variant: 'destructive', | ||
| }); | ||
| } | ||
| }, [ | ||
| isConnected, | ||
| issuer, | ||
| clientId, | ||
| clientSecret, | ||
| organizationId, | ||
| autoProvisionRole, | ||
| roleMappingRules, | ||
| defaultRole, | ||
| enableOneDriveAccess, | ||
| autoProvisionTeam, | ||
| excludeGroups, | ||
| upsertSSOProvider, | ||
| onOpenChange, | ||
| t, | ||
| ]); | ||
|
|
||
| const handleDisconnect = useCallback(async () => { | ||
| try { | ||
| await removeSSOProvider({ organizationId }); | ||
|
|
||
| toast({ | ||
| title: t('integrations.sso.disconnected'), | ||
| description: t('integrations.sso.ssoDisconnected'), | ||
| }); | ||
|
|
||
| onOpenChange?.(false); | ||
| } catch (error) { | ||
| toast({ | ||
| title: t('integrations.sso.disconnectFailed'), | ||
| description: | ||
| error instanceof Error | ||
| ? error.message | ||
| : t('integrations.sso.disconnectError'), | ||
| variant: 'destructive', | ||
| }); | ||
| } | ||
| }, [removeSSOProvider, organizationId, onOpenChange, t]); | ||
|
|
||
| const handleTest = useCallback(async () => { | ||
| const useExistingConfig = isConnected && !clientSecret; | ||
|
|
||
| if (!useExistingConfig && (!issuer || !clientId || !clientSecret)) { | ||
| toast({ | ||
| title: t('integrations.sso.validationError'), | ||
| description: t('integrations.sso.allFieldsRequired'), | ||
| variant: 'destructive', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| setTestResult(null); | ||
|
|
||
| try { | ||
| const result = useExistingConfig | ||
| ? await testExistingSSOConfig({}) | ||
| : await testSSOConfig({ | ||
| issuer, | ||
| clientId, | ||
| clientSecret, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Avoid sending empty client secrets on updates/tests.
When connected and the secret is blank, save/test can overwrite the stored secret or test the old config even after edits. Only include clientSecret when provided and gate “use existing config” on having no field changes.
✏️ Suggested change
const handleSave = useCallback(async () => {
const requiresSecret = !isConnected;
- if (!issuer || !clientId || (requiresSecret && !clientSecret)) {
+ const trimmedSecret = clientSecret.trim();
+ if (!issuer || !clientId || (requiresSecret && !trimmedSecret)) {
toast({
title: t('integrations.sso.validationError'),
description: t('integrations.sso.allFieldsRequired'),
variant: 'destructive',
});
return;
}
@@
await upsertSSOProvider({
organizationId,
providerId: 'entra-id',
issuer,
clientId,
- clientSecret,
+ ...(trimmedSecret ? { clientSecret: trimmedSecret } : {}),
scopes: DEFAULT_SCOPES,
autoProvisionRole,
roleMappingRules,
defaultRole,
@@
const handleTest = useCallback(async () => {
- const useExistingConfig = isConnected && !clientSecret;
+ const useExistingConfig =
+ isConnected && !clientSecret.trim() && !hasChanges;- t,
+ t,
+ hasChanges,🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In
`@services/platform/app/features/settings/integrations/hooks/use-sso-config-form.ts`
around lines 207 - 321, The current handlers send an empty clientSecret and
treat "use existing config" purely by isConnected & empty clientSecret, which
can overwrite or incorrectly test the stored secret; update both handleSave and
handleTest to only include clientSecret in the payload when clientSecret is
non-empty and to treat useExistingConfig as true only when the form has no
changes vs the stored provider (e.g., compare issuer and clientId and any edited
fields against the existing provider object you already have, such as
existingProvider or ssoProvider). Specifically: in handleSave
(upsertSSOProvider) build the payload so clientSecret is omitted when
clientSecret is falsy rather than sent as an empty string; in handleTest compute
useExistingConfig = isConnected && !clientSecret &&
noFormChangesAgainstExistingProvider (compare issuer/clientId/other relevant
form fields to existingProvider.issuer, existingProvider.clientId, etc.); and in
handleTest call testExistingSSOConfig only when that stricter useExistingConfig
is true, otherwise call testSSOConfig with a payload that includes clientSecret
only if non-empty.
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
Test plan
Summary by CodeRabbit