perf(web): improve frontend dialog layout and sizing - #5346
Merged
Conversation
- introduce a reusable dialog component for consistent header, body, and footer layout. - support per-dialog sizing, trigger rendering, initial focus, and close button controls. - preserve base dialog open and close motion classes while allowing content-specific styling.
- migrate frontend dialogs to the shared footer API so actions stay separated from scrollable body content. - tune dialog dimensions for model analytics, prefill groups, billing history, channel model sync, and related workflows. - update channel terminology and dialog action translations across supported locales.
Contributor
WalkthroughAdds a new shared Dialog component and migrates all existing dialogs across the app to its prop-driven API. Updates multiple locale JSON files with new/moved keys. ChangesUnified Dialog component and refactors
Sequence Diagram(s)sequenceDiagram
participant Feature as rgba(53, 132, 228, 0.5) Feature
participant Dialog as rgba(0, 148, 133, 0.5) Dialog (`@/components/dialog`)
participant Radix as rgba(153, 102, 255, 0.5) Radix Primitives
Feature->>Dialog: title, description, footer, classNames, contentHeight
Dialog->>Radix: Root/Trigger/Content/Header/Body/Footer
Radix-->>Feature: onOpenChange(open)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
web/default/src/features/dashboard/components/overview/announcement-detail-dialog.tsx (1)
37-42: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRefactor to avoid destructuring component props.
The function signature destructures props, which violates the project's coding guideline. As per coding guidelines, component props should be accessed via
props.xxxdirectly instead of destructuring.♻️ Proposed refactor
-export function AnnouncementDetailModal({ - open, - onOpenChange, - announcement, -}: AnnouncementDetailModalProps) { +export function AnnouncementDetailModal( + props: AnnouncementDetailModalProps +) { const { t } = useTranslation() return ( <Dialog - open={open} - onOpenChange={onOpenChange} + open={props.open} + onOpenChange={props.onOpenChange} title={t('Announcement Details')} description={ - announcement?.publishDate - ? `${t('Published:')} ${formatDateTimeObject(new Date(announcement.publishDate))}` + props.announcement?.publishDate + ? `${t('Published:')} ${formatDateTimeObject(new Date(props.announcement.publishDate))}` : undefined } contentClassName='sm:max-w-lg' contentHeight='auto' bodyClassName='space-y-4' > <ScrollArea className='max-h-[min(58vh,520px)] pr-4'> <div className='space-y-4'> - {announcement?.content && ( + {props.announcement?.content && ( <div> <h4 className='mb-2 font-medium'>{t('Content')}</h4> - <Markdown>{announcement.content}</Markdown> + <Markdown>{props.announcement.content}</Markdown> </div> )} - {announcement?.extra && ( + {props.announcement?.extra && ( <div> <h4 className='mb-2 font-medium'> {t('Additional Information')} </h4> <Markdown className='text-muted-foreground'> - {announcement.extra} + {props.announcement.extra} </Markdown> </div> )} </div> </ScrollArea> </Dialog> ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/dashboard/components/overview/announcement-detail-dialog.tsx` around lines 37 - 42, The component AnnouncementDetailModal currently destructures its props in the function signature (open, onOpenChange, announcement); change the signature to accept a single props parameter (props: AnnouncementDetailModalProps) and update all internal references to use props.open, props.onOpenChange, and props.announcement instead of the destructured names so the component follows the project's no-destructuring guideline.Source: Coding guidelines
web/default/src/features/system-settings/models/channel-selector-dialog.tsx (1)
81-90: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRefactor to avoid destructuring component props.
The function signature destructures all props, which violates the project's coding guideline. As per coding guidelines, component props should be accessed via
props.xxxdirectly instead of destructuring.♻️ Proposed refactor
-export function ChannelSelectorDialog({ - open, - onOpenChange, - channels, - selectedChannelIds, - onSelectedChannelIdsChange, - channelEndpoints, - onChannelEndpointsChange, - onConfirm, -}: ChannelSelectorDialogProps) { +export function ChannelSelectorDialog( + props: ChannelSelectorDialogProps +) { const { t } = useTranslation() const [search, setSearch] = useState('') const [rowSelection, setRowSelection] = useState<RowSelectionState>({}) useEffect(() => { - if (!selectedChannelIds.length) { + if (!props.selectedChannelIds.length) { setRowSelection({}) return } - const availableChannelIds = new Set(channels.map((channel) => channel.id)) + const availableChannelIds = new Set( + props.channels.map((channel) => channel.id) + ) const newSelection: RowSelectionState = {} - selectedChannelIds.forEach((id) => { + props.selectedChannelIds.forEach((id) => { if (availableChannelIds.has(id)) { newSelection[id.toString()] = true } }) setRowSelection(newSelection) - }, [selectedChannelIds, channels]) + }, [props.selectedChannelIds, props.channels]) const updateEndpoint = useCallback( (channelId: number, endpoint: string) => { - onChannelEndpointsChange({ - ...channelEndpoints, + props.onChannelEndpointsChange({ + ...props.channelEndpoints, [channelId]: endpoint, }) }, - [channelEndpoints, onChannelEndpointsChange] + [props.channelEndpoints, props.onChannelEndpointsChange] ) // ... continue updating all references to use props.xxx patternApply similar changes throughout the component for all destructured props:
props.open,props.onOpenChange,props.channels,props.selectedChannelIds,props.onSelectedChannelIdsChange,props.channelEndpoints,props.onChannelEndpointsChange, andprops.onConfirm.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/system-settings/models/channel-selector-dialog.tsx` around lines 81 - 90, The component ChannelSelectorDialog currently destructures props in its function signature; change the signature to accept a single props parameter (e.g., function ChannelSelectorDialog(props: ChannelSelectorDialogProps)) and update all internal references to use props.open, props.onOpenChange, props.channels, props.selectedChannelIds, props.onSelectedChannelIdsChange, props.channelEndpoints, props.onChannelEndpointsChange, and props.onConfirm instead of the destructured identifiers so the component follows the project's guideline of not destructuring props at the top level.Source: Coding guidelines
web/default/src/features/system-settings/maintenance/update-checker-section.tsx (1)
136-174:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
contentClassNamebreaks Dialog overflow model.Line 154 sets
overflow-y-autoandmax-h-[80vh]incontentClassName, which override the Dialog's baseoverflow-hiddenandmax-h-[calc(100vh-2rem)]classes. This breaks the Dialog's scroll behavior.🔧 Recommended fix
Remove
overflow-y-autoandmax-h-[80vh]fromcontentClassName:- contentClassName='max-h-[80vh] overflow-y-auto' + contentClassName=''Or omit
contentClassNameentirely if no custom styling is needed. The Dialog handles overflow internally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/system-settings/maintenance/update-checker-section.tsx` around lines 136 - 174, The Dialog usage in update-checker-section (the Dialog component with props dialogOpen/onOpenChange/setDialogOpen and title/description/footer) currently passes contentClassName='max-h-[80vh] overflow-y-auto' which overrides the Dialog's internal overflow and max-height behavior; remove those classes (or remove the contentClassName prop entirely) so the Dialog keeps its built-in overflow-hidden and max-h-[calc(100vh-2rem)] behavior, and if you need additional styling, move safe styles into an inner wrapper inside the Dialog body rather than overriding contentClassName.web/default/src/features/profile/components/dialogs/wechat-bind-dialog.tsx (1)
28-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused
onSuccessprop.The
onSuccesscallback is declared inWeChatBindDialogProps(line 31) but is never used in the component body. Since this appears to be a placeholder dialog (line 65 says "This feature requires server-side WeChat configuration"), the unused prop should either be removed or a comment should explain why it's reserved for future implementation.🧹 Proposed fix
If the prop is truly unused and not needed for future implementation:
interface WeChatBindDialogProps { open: boolean onOpenChange: (open: boolean) => void - onSuccess: () => void }Or add a comment if it's reserved for future use:
interface WeChatBindDialogProps { open: boolean onOpenChange: (open: boolean) => void + // Reserved for future WeChat binding success callback onSuccess: () => void }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/profile/components/dialogs/wechat-bind-dialog.tsx` around lines 28 - 32, The WeChatBindDialogProps interface currently declares an unused onSuccess callback; remove the onSuccess property from the WeChatBindDialogProps interface and from the component's props/destructuring (references to onSuccess in the WeChatBindDialog component), or if you intend to reserve it, replace it with a short comment explaining it's intentionally unused for now; also scan for and remove any now-unneeded prop forwarding/usages of onSuccess where this dialog is instantiated to keep prop types consistent.
🧹 Nitpick comments (18)
web/default/src/features/auth/sign-in/components/user-auth-form.tsx (1)
407-469: ⚖️ Poor tradeoffConsider extracting shared WeChat dialog component.
The WeChat dialog implementation (QR code display, verification code input, confirm/cancel buttons) is duplicated between
user-auth-form.tsxandsign-up-form.tsx(lines 380-442). Extracting this into a shared component would reduce duplication and improve maintainability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/auth/sign-in/components/user-auth-form.tsx` around lines 407 - 469, Extract the duplicated WeChat dialog UI into a reusable component (e.g., WeChatDialog) and replace the inline Dialog blocks in user-auth-form.tsx and sign-up-form.tsx with that component; the new component should accept props for open, onOpenChange (handleWeChatDialogChange), onConfirm (handleWeChatLogin), isSubmitting (isWeChatSubmitting), qrCodeUrl (wechatQrCodeUrl), code (wechatCode), onCodeChange (setWeChatCode), requiresLegalConsent, agreedToLegal, and i18n strings so it can render the QR image, verification Input, and Cancel/Confirm Buttons with the same enable/disable logic and loader display. Ensure you keep the same aria/ids (e.g., htmlFor='wechat-code') and pass through className/content props like contentClassName/contentHeight/bodyClassName so styling/behavior is preserved, and update both call sites to use this single WeChatDialog component.web/default/src/features/models/components/data-table-bulk-actions.tsx (1)
183-207: ⚡ Quick winRemove unnecessary whitespace child.
Line 206 contains
{' '}as the only child of the Dialog. For a confirmation dialog with no body content, you can either pass no children or omit the whitespace entirely.♻️ Proposed fix
footer={ <> <Button variant='outline' onClick={() => setShowDeleteConfirm(false)} > {t('Cancel')} </Button> <Button variant='destructive' onClick={handleDeleteAll}> {t('Delete')} </Button> </> } - > - {' '} - </Dialog> + />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/models/components/data-table-bulk-actions.tsx` around lines 183 - 207, The Dialog component instance (Dialog in data-table-bulk-actions.tsx) currently includes an unnecessary whitespace child {' '} which is redundant; remove that child so the Dialog has no children (i.e., delete the {' '} node or omit any children between <Dialog ...> and </Dialog>) to avoid extraneous whitespace rendering and satisfy the reviewer comment.web/default/src/features/system-settings/general/channel-affinity/index.tsx (1)
85-92: ⚡ Quick winAvoid destructuring component props.
As per coding guidelines, component props should not be destructured. Use
props.open,props.onOpenChange, etc., directly instead.-function ChannelAffinityConfirmDialog(props: { - open: boolean - onOpenChange: (open: boolean) => void - title: ReactNode - desc: ReactNode - handleConfirm: () => void - destructive?: boolean -}) { +type ChannelAffinityConfirmDialogProps = { + open: boolean + onOpenChange: (open: boolean) => void + title: ReactNode + desc: ReactNode + handleConfirm: () => void + destructive?: boolean +} + +function ChannelAffinityConfirmDialog(props: ChannelAffinityConfirmDialogProps) { const { t } = useTranslation()Then reference
props.*in the Dialog JSX.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/system-settings/general/channel-affinity/index.tsx` around lines 85 - 92, ChannelAffinityConfirmDialog currently destructures its props; update the component to use the props object directly (e.g., props.open, props.onOpenChange, props.title, props.desc, props.handleConfirm, props.destructive) instead of destructuring in the function signature or body, and update all references inside the Dialog JSX to reference props.* so the component follows the guideline against destructuring props.Source: Coding guidelines
web/default/src/features/system-settings/content/faq-section.tsx (1)
88-88: ⚡ Quick winAvoid destructuring component props.
As per coding guidelines, component props should not be destructured. Use
props.enabledandprops.datadirectly instead.-export function FAQSection({ enabled, data }: FAQSectionProps) { +export function FAQSection(props: FAQSectionProps) { const { t } = useTranslation()Then reference
props.enabledandprops.datathroughout the component body.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/system-settings/content/faq-section.tsx` at line 88, The component FAQSection currently destructures props in its signature; change the signature to accept a single props parameter (e.g., function FAQSection(props: FAQSectionProps)) and replace all uses of the destructured names with props.enabled and props.data inside the component body, ensuring any references to enabled or data (including in JSX and any helper calls) are updated accordingly.Source: Coding guidelines
web/default/src/features/system-settings/content/api-info-section.tsx (1)
113-113: ⚡ Quick winAvoid destructuring component props.
As per coding guidelines, component props should not be destructured. Use
props.enabledandprops.datadirectly instead.-export function ApiInfoSection({ enabled, data }: ApiInfoSectionProps) { +export function ApiInfoSection(props: ApiInfoSectionProps) { const { t } = useTranslation() const updateOption = useUpdateOption()Then reference
props.enabledandprops.datathroughout the component body.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/system-settings/content/api-info-section.tsx` at line 113, The ApiInfoSection component currently destructures its props in the signature (export function ApiInfoSection({ enabled, data }: ApiInfoSectionProps)); change the function to accept a single props parameter (e.g., export function ApiInfoSection(props: ApiInfoSectionProps)) and replace all usages of enabled and data inside the component with props.enabled and props.data respectively so the component follows the no-destructuring guideline; ensure any nested references (handlers, JSX, or helpers) are updated to use props.enabled and props.data.Source: Coding guidelines
web/default/src/features/system-settings/integrations/waffo-settings-section.tsx (1)
77-82: ⚡ Quick winAvoid destructuring component props.
As per coding guidelines, component props should not be destructured. Use
props.values,props.onValueChange, etc., directly instead.-export function WaffoSettingsSection({ - values, - onValueChange, - payMethods, - onPayMethodsChange, -}: Props) { +export function WaffoSettingsSection(props: Props) { const { t } = useTranslation()Then reference
props.*throughout the component body.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/system-settings/integrations/waffo-settings-section.tsx` around lines 77 - 82, The WaffoSettingsSection component currently destructures its props (values, onValueChange, payMethods, onPayMethodsChange); update the component signature and all usages to accept a single props parameter and reference props.values, props.onValueChange, props.payMethods, and props.onPayMethodsChange instead. Specifically modify the WaffoSettingsSection function parameter from ({ values, onValueChange, payMethods, onPayMethodsChange }: Props) to (props: Props) and replace every occurrence of the destructured identifiers inside the component body with the corresponding props.* accessors to comply with the guideline.Source: Coding guidelines
web/default/src/features/system-settings/content/chat-dialog.tsx (1)
59-64: ⚡ Quick winAvoid destructuring component props.
As per coding guidelines, component props should not be destructured. Use
props.open,props.onOpenChange, etc., directly instead.-export function ChatDialog({ - open, - onOpenChange, - onSave, - editData, -}: ChatDialogProps) { +export function ChatDialog(props: ChatDialogProps) { const { t } = useTranslation() - const isEditMode = !!editData + const isEditMode = !!props.editDataThen reference
props.*throughout the component body.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/system-settings/content/chat-dialog.tsx` around lines 59 - 64, The ChatDialog component currently destructures props in its function signature; change the signature to accept a single props parameter (i.e., function ChatDialog(props: ChatDialogProps)) and stop destructuring open, onOpenChange, onSave, editData there; then update all internal references to use props.open, props.onOpenChange, props.onSave, props.editData (and any other prop usages) so the component consistently accesses props via the props object rather than via destructured variables.Source: Coding guidelines
web/default/src/features/system-settings/content/uptime-kuma-section.tsx (1)
95-95: ⚡ Quick winAvoid destructuring component props.
As per coding guidelines, component props should not be destructured. Use
props.enabledandprops.datadirectly instead.-export function UptimeKumaSection({ enabled, data }: UptimeKumaSectionProps) { +export function UptimeKumaSection(props: UptimeKumaSectionProps) { const { t } = useTranslation()Then reference
props.enabledandprops.datathroughout the component body.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/system-settings/content/uptime-kuma-section.tsx` at line 95, The component currently destructures props in the function signature (UptimeKumaSection({ enabled, data }: UptimeKumaSectionProps)); change the signature to accept a single props parameter (e.g., UptimeKumaSection(props: UptimeKumaSectionProps)) and update all internal references from enabled and data to props.enabled and props.data so the component follows the non-destructuring guideline; ensure any usages in JSX and helper calls within UptimeKumaSection are updated accordingly.Source: Coding guidelines
web/default/src/features/channels/components/dialogs/fetch-models-dialog.tsx (1)
401-507: ⚡ Quick winReplace multi-level ternaries with explicit branch helpers.
Line 401 and Line 437 use nested ternaries with 2+ levels, which makes this render path harder to maintain and violates the TS/TSX guideline.
♻️ Suggested refactor
+ const getDefaultTab = (): 'new' | 'removed' | 'existing' => { + if (newModels.length > 0) return 'new' + if (removedModels.length > 0) return 'removed' + return 'existing' + } + + const renderDialogBody = () => { + if (!activeChannel && !customFetcher) { + return ( + <div className='text-muted-foreground py-8 text-center'> + {t('No channel selected')} + </div> + ) + } + if (isFetching) { + return ( + <div className='flex items-center justify-center py-12'> + <Loader2 className='text-muted-foreground h-8 w-8 animate-spin' /> + </div> + ) + } + if (fetchedModels.length === 0 && removedModels.length === 0) { + return ( + <div className='text-muted-foreground py-8 text-center'> + <p>{t('No models fetched yet.')}</p> + <Button className='mt-4' onClick={handleFetchModels} disabled={isFetching}> + {t('Fetch Models')} + </Button> + </div> + ) + } + return ( + // existing main content block + ) + } ... - defaultValue={ - newModels.length > 0 - ? 'new' - : removedModels.length > 0 - ? 'removed' - : 'existing' - } + defaultValue={getDefaultTab()} ... - {!activeChannel && !customFetcher ? ( - ... - ) : isFetching ? ( - ... - ) : fetchedModels.length === 0 && removedModels.length === 0 ? ( - ... - ) : ( - ... - )} + {renderDialogBody()}As per coding guidelines,
web/default/**/*.{ts,tsx}prohibits nested ternary expressions with 2 or more levels.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/channels/components/dialogs/fetch-models-dialog.tsx` around lines 401 - 507, The top-level nested ternary in the FetchModelsDialog render (the block using {!activeChannel && !customFetcher ? ... : isFetching ? ... : fetchedModels.length === 0 && removedModels.length === 0 ? ... : ...}) should be replaced with explicit branch helper functions or early-return conditionals to avoid multi-level ternaries; extract each branch into small render helpers such as renderNoChannelState(), renderLoadingState(), renderEmptyFetchedState() and renderModelsState() and call them in the JSX, reusing existing helpers like handleFetchModels, renderModelCategory, and getSortedCategoryEntries/newModelsByCategory/removedModels to build each branch. Ensure the Tabs block and selection summary remain inside renderModelsState() so logic stays unchanged but the top-level conditional is flattened and readable.Source: Coding guidelines
web/default/src/features/channels/components/dialogs/param-override-editor-dialog.tsx (1)
1805-2049: ⚡ Quick winFlatten the nested render ternary into explicit condition blocks.
Line 1805 introduces a multi-level ternary (
editMode+visualMode) that reduces readability and violates the TS/TSX guideline.As per coding guidelines,
web/default/**/*.{ts,tsx}prohibits nested ternary expressions with 2 or more levels.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/channels/components/dialogs/param-override-editor-dialog.tsx` around lines 1805 - 2049, The nested ternary based on editMode and visualMode (the large JSX starting with "{editMode === 'visual' ? ( visualMode === 'legacy' ? ... ) : ... }") should be flattened into explicit condition blocks or extracted render helper components; replace the two-level ternary with clear if/else branches (or small functions like renderVisualLegacy(), renderVisualEditor(), renderJsonEditor()) and call them from the main JSX, keeping the same props and callbacks (e.g., legacyValue/setLegacyValue, operations, selectedOperationId, addOperation, handleDragStart/Over/Drop, RuleEditor usage, jsonText/handleJsonChange/formatJson) so behavior is unchanged while removing the nested ternary.Source: Coding guidelines
web/default/src/features/profile/components/dialogs/access-token-dialog.tsx (1)
38-41: ⚡ Quick winAvoid destructuring component props.
The coding guidelines specify: "Do not destructure component props; use
props.xxxdirectly instead for clarity." This applies toopenandonOpenChangeon lines 38-41.♻️ Recommended refactor to use props directly
export function AccessTokenDialog({ - open, - onOpenChange, -}: AccessTokenDialogProps) { + props +}: { props: AccessTokenDialogProps }) { const { t } = useTranslation() const { token, generating, generate } = useAccessToken() // Auto-generate token when dialog opens if no token exists useEffect(() => { - if (open && !token) { + if (props.open && !token) { generate() } - }, [open, token, generate]) + }, [props.open, token, generate]) return ( <Dialog - open={open} - onOpenChange={onOpenChange} + open={props.open} + onOpenChange={props.onOpenChange} title={t('Access Token')}Alternatively, a simpler pattern:
-export function AccessTokenDialog({ - open, - onOpenChange, -}: AccessTokenDialogProps) { +export function AccessTokenDialog(props: AccessTokenDialogProps) { const { t } = useTranslation() const { token, generating, generate } = useAccessToken() // Auto-generate token when dialog opens if no token exists useEffect(() => { - if (open && !token) { + if (props.open && !token) { generate() } - }, [open, token, generate]) + }, [props.open, token, generate]) return ( <Dialog - open={open} - onOpenChange={onOpenChange} + open={props.open} + onOpenChange={props.onOpenChange}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/profile/components/dialogs/access-token-dialog.tsx` around lines 38 - 41, The component AccessTokenDialog is destructuring props (open, onOpenChange); change it to use the props object directly to match guidelines by replacing the parameter destructuring with a single props parameter and update all usages to props.open and props.onOpenChange (e.g., in the function signature for AccessTokenDialog and any internal references) so the component reads from props.xxx instead of direct destructured variables.Source: Coding guidelines
web/default/src/features/profile/components/dialogs/wechat-bind-dialog.tsx (1)
34-37: ⚡ Quick winAvoid destructuring component props.
Per coding guidelines: "Do not destructure component props; use
props.xxxdirectly instead for clarity." This applies toopenandonOpenChangeon lines 34-37.Note that
onSuccessis declared in the props interface but never destructured or used, which is flagged separately below.♻️ Recommended refactor
-export function WeChatBindDialog({ - open, - onOpenChange, -}: WeChatBindDialogProps) { +export function WeChatBindDialog(props: WeChatBindDialogProps) { const { t } = useTranslation() return ( <Dialog - open={open} - onOpenChange={onOpenChange} + open={props.open} + onOpenChange={props.onOpenChange} title={t('Bind WeChat Account')}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/profile/components/dialogs/wechat-bind-dialog.tsx` around lines 34 - 37, The WeChatBindDialog component currently destructures props (open, onOpenChange) in its signature; update the component to accept a single props parameter and reference values as props.open and props.onOpenChange inside the body (i.e., change export function WeChatBindDialog({ ... }) to export function WeChatBindDialog(props: WeChatBindDialogProps) and replace usages accordingly). Also address the unused onSuccess declared on WeChatBindDialogProps by either wiring props.onSuccess where appropriate or removing it from the WeChatBindDialogProps interface to eliminate the dead prop.Source: Coding guidelines
web/default/src/features/profile/components/dialogs/change-password-dialog.tsx (1)
39-43: ⚡ Quick winAvoid destructuring component props.
As per coding guidelines: "Do not destructure component props; use
props.xxxdirectly instead for clarity." This applies toopen,onOpenChange, andusernameon lines 39-43.♻️ Recommended refactor
-export function ChangePasswordDialog({ - open, - onOpenChange, - username, -}: ChangePasswordDialogProps) { +export function ChangePasswordDialog(props: ChangePasswordDialogProps) { const { t } = useTranslation() const [loading, setLoading] = useState(false) // ... other state const handleSubmit = async (e: React.FormEvent) => { // ... validation if (response.success) { toast.success(t('Password changed successfully')) - onOpenChange(false) + props.onOpenChange(false) setFormData({ originalPassword: '', newPassword: '', confirmPassword: '', }) return ( <Dialog - open={open} - onOpenChange={onOpenChange} + open={props.open} + onOpenChange={props.onOpenChange} title={t('Change Password')} description={ <> - {t('Update your password for account:')} <strong>{username}</strong> + {t('Update your password for account:')} <strong>{props.username}</strong> </> }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/profile/components/dialogs/change-password-dialog.tsx` around lines 39 - 43, The component ChangePasswordDialog currently destructures its props (open, onOpenChange, username); update it to accept a single props parameter and replace all uses of the destructured identifiers with props.open, props.onOpenChange, and props.username throughout the function and any nested scopes (including JSX and handlers) to follow the project's "do not destructure props" guideline.Source: Coding guidelines
web/default/src/features/profile/components/dialogs/two-fa-setup-dialog.tsx (2)
136-143: ⚡ Quick winConsider using a single i18n key for the step description.
The description concatenates multiple translated fragments (
t('Step'),step + 1,t('of 3:'),stepLabels[step]), which may not work well for languages with different word orders or grammar rules.♻️ Recommended approach
Consider using a parameterized translation key:
description={ - <> - {t('Step')} - {step + 1} - {t('of 3:')} - {stepLabels[step]} - </> + t('Step {{current}} of {{total}}: {{label}}', { + current: step + 1, + total: 3, + label: stepLabels[step], + }) }Then add to locale files:
{ "Step {{current}} of {{total}}: {{label}}": "Step {{current}} of {{total}}: {{label}}" }This allows translators to reorder the elements as needed for their language.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/profile/components/dialogs/two-fa-setup-dialog.tsx` around lines 136 - 143, Replace the concatenated translations in the description prop with a single parameterized i18n key so translators can reorder parts; call t with one key (e.g. 'twoFa.stepDescription') and pass an object like { current: step + 1, total: 3, label: stepLabels[step] } instead of using t('Step') and t('of 3:') separately, update locale files to include the new key ("twoFa.stepDescription": "Step {{current}} of {{total}}: {{label}}") and ensure any usages in two-fa-setup-dialog.tsx use that single interpolated translation.
43-47: ⚡ Quick winAvoid destructuring component props.
As per coding guidelines: "Do not destructure component props; use
props.xxxdirectly instead for clarity." This applies toopen,onOpenChange, andonSuccesson lines 43-47.♻️ Recommended refactor
-export function TwoFASetupDialog({ - open, - onOpenChange, - onSuccess, -}: TwoFASetupDialogProps) { +export function TwoFASetupDialog(props: TwoFASetupDialogProps) { const { t } = useTranslation() const [loading, setLoading] = useState(false) // ... other state const handleSetup = useCallback(async () => { // ... setup logic toast.error(response.message || t('Failed to setup 2FA')) - onOpenChange(false) + props.onOpenChange(false) } - }, [onOpenChange, t]) + }, [props.onOpenChange, t]) const handleEnable = async () => { // ... enable logic if (response.success) { toast.success(t('Two-factor authentication enabled successfully!')) - onOpenChange(false) - onSuccess() + props.onOpenChange(false) + props.onSuccess() // ... reset const handleOpenChange = (open: boolean) => { if (!loading && !initializing) { if (open && !setupData) { handleSetup() } if (!open) { // ... reset } - onOpenChange(open) + props.onOpenChange(open) } } useEffect(() => { - if (open && !setupData && !initializing) { + if (props.open && !setupData && !initializing) { handleSetup() } - }, [open, setupData, initializing, handleSetup]) + }, [props.open, setupData, initializing, handleSetup]) return ( <Dialog - open={open} + open={props.open} onOpenChange={handleOpenChange}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/profile/components/dialogs/two-fa-setup-dialog.tsx` around lines 43 - 47, The component TwoFASetupDialog currently destructures its props (open, onOpenChange, onSuccess); change it to accept a single props parameter and reference properties via props.open, props.onOpenChange, and props.onSuccess inside the function and JSX to comply with the no-destructuring guideline—update the function signature (keep name TwoFASetupDialog) and replace every use of the former local variables with props.<property> so the component consistently uses props for access.Source: Coding guidelines
web/default/src/features/profile/components/dialogs/delete-account-dialog.tsx (1)
43-47: ⚡ Quick winAvoid destructuring component props.
Per coding guidelines: "Do not destructure component props; use
props.xxxdirectly instead for clarity." This applies toopen,onOpenChange, andusernameon lines 43-47.♻️ Recommended refactor
-export function DeleteAccountDialog({ - open, - onOpenChange, - username, -}: DeleteAccountDialogProps) { +export function DeleteAccountDialog(props: DeleteAccountDialogProps) { const { t } = useTranslation() const navigate = useNavigate() const { reset } = useAuthStore((state) => state.auth) const [loading, setLoading] = useState(false) const [confirmation, setConfirmation] = useState('') const handleDelete = async () => { - if (confirmation !== username) { + if (confirmation !== props.username) { toast.error(t('Username confirmation does not match')) return } const handleOpenChange = (open: boolean) => { if (!loading) { - onOpenChange(open) + props.onOpenChange(open) if (!open) { setConfirmation('') } return ( <Dialog - open={open} + open={props.open} onOpenChange={handleOpenChange} // ... other props description={ <> - {t('Type')} <strong>{username}</strong> {t('to confirm')} + {t('Type')} <strong>{props.username}</strong> {t('to confirm')} </> }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/profile/components/dialogs/delete-account-dialog.tsx` around lines 43 - 47, The component DeleteAccountDialog currently destructures props (open, onOpenChange, username); change its signature to receive a single props object (e.g., DeleteAccountDialog(props: DeleteAccountDialogProps)) and update all internal references to use props.open, props.onOpenChange, and props.username instead of the destructured variables so the component conforms to the "do not destructure component props" guideline.Source: Coding guidelines
web/default/src/features/users/components/dialogs/user-binding-dialog.tsx (1)
385-387: ⚡ Quick winPrefer
cn()for dynamic class merging.The template literal should be replaced with the
cn()utility to align with the coding guidelines and maintain consistency across the codebase.♻️ Refactor to use cn()
Import
cnfrom@/lib/utils:+import { cn } from '`@/lib/utils`'Then update the className:
- className={`flex items-center justify-between rounded-md border px-3 py-2.5 ${ - !binding.isBound ? 'opacity-50' : '' - }`} + className={cn( + 'flex items-center justify-between rounded-md border px-3 py-2.5', + !binding.isBound && 'opacity-50' + )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/users/components/dialogs/user-binding-dialog.tsx` around lines 385 - 387, Replace the dynamic template literal used for the className on the binding row with the project's classname helper: import cn from '`@/lib/utils`' and use cn(...) instead of the string template so conditional 'opacity-50' is merged properly; update the element that currently references binding.isBound in its className (the JSX that builds `className={`flex items-center justify-between rounded-md border px-3 py-2.5 ${!binding.isBound ? 'opacity-50' : ''}`}`) to call cn with the static classes and a conditional object/array for '!binding.isBound' so the class merging follows project conventions.Source: Coding guidelines
web/default/src/features/subscriptions/components/dialogs/subscription-purchase-dialog.tsx (1)
262-262: ⚡ Quick winAdd
aria-hidden="true"to decorative icons.The Crown, CalendarClock, and Package icons are decorative (they accompany text that conveys the same information). Per coding guidelines, decorative icons should have
aria-hidden="true"to prevent unnecessary verbosity for screen reader users.♻️ Proposed fix
title={ <> - <Crown className='h-5 w-5' /> + <Crown className='h-5 w-5' aria-hidden='true' /> {t('Purchase Subscription')} </> }<span className='flex items-center gap-1 text-sm'> - <CalendarClock className='h-3.5 w-3.5' /> + <CalendarClock className='h-3.5 w-3.5' aria-hidden='true' /> {formatDuration(plan, t)} </span><span className='flex items-center gap-1 text-sm'> - <Package className='h-3.5 w-3.5' /> + <Package className='h-3.5 w-3.5' aria-hidden='true' /> {totalAmount > 0 ? formatQuota(totalAmount) : t('Unlimited')} </span>Also applies to: 286-286, 303-303
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/subscriptions/components/dialogs/subscription-purchase-dialog.tsx` at line 262, The decorative icons Crown, CalendarClock, and Package in subscription-purchase-dialog.tsx are missing aria-hidden, so add aria-hidden="true" to each icon component instance (e.g., the <Crown />, <CalendarClock />, and <Package /> usages) so screen readers skip them; update all occurrences (the instances around the current Crown usage and the other two occurrences noted) to include aria-hidden="true".Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3feacbd8-2a0e-4671-8973-1a51288d9f4d
📒 Files selected for processing (80)
web/default/src/components/dialog.tsxweb/default/src/components/layout/components/public-header.tsxweb/default/src/features/auth/secure-verification/components/secure-verification-dialog.tsxweb/default/src/features/auth/sign-in/components/user-auth-form.tsxweb/default/src/features/auth/sign-up/components/sign-up-form.tsxweb/default/src/features/channels/components/data-table-bulk-actions.tsxweb/default/src/features/channels/components/dialogs/balance-query-dialog.tsxweb/default/src/features/channels/components/dialogs/channel-test-dialog.tsxweb/default/src/features/channels/components/dialogs/codex-oauth-dialog.tsxweb/default/src/features/channels/components/dialogs/codex-usage-dialog.tsxweb/default/src/features/channels/components/dialogs/copy-channel-dialog.tsxweb/default/src/features/channels/components/dialogs/edit-tag-dialog.tsxweb/default/src/features/channels/components/dialogs/fetch-models-dialog.tsxweb/default/src/features/channels/components/dialogs/multi-key-manage-dialog.tsxweb/default/src/features/channels/components/dialogs/ollama-models-dialog.tsxweb/default/src/features/channels/components/dialogs/param-override-editor-dialog.tsxweb/default/src/features/channels/components/dialogs/status-code-risk-dialog.tsxweb/default/src/features/channels/components/dialogs/tag-batch-edit-dialog.tsxweb/default/src/features/channels/components/dialogs/upstream-update-dialog.tsxweb/default/src/features/dashboard/components/models/models-chart-preferences.tsxweb/default/src/features/dashboard/components/models/models-filter-dialog.tsxweb/default/src/features/dashboard/components/overview/announcement-detail-dialog.tsxweb/default/src/features/keys/components/dialogs/cc-switch-dialog.tsxweb/default/src/features/models/components/data-table-bulk-actions.tsxweb/default/src/features/models/components/dialogs/description-dialog.tsxweb/default/src/features/models/components/dialogs/extend-deployment-dialog.tsxweb/default/src/features/models/components/dialogs/missing-models-dialog.tsxweb/default/src/features/models/components/dialogs/prefill-group-management-dialog.tsxweb/default/src/features/models/components/dialogs/rename-deployment-dialog.tsxweb/default/src/features/models/components/dialogs/sync-wizard-dialog.tsxweb/default/src/features/models/components/dialogs/update-config-dialog.tsxweb/default/src/features/models/components/dialogs/upstream-conflict-dialog.tsxweb/default/src/features/models/components/dialogs/vendor-mutate-dialog.tsxweb/default/src/features/models/components/dialogs/view-details-dialog.tsxweb/default/src/features/models/components/dialogs/view-logs-dialog.tsxweb/default/src/features/profile/components/checkin-calendar-card.tsxweb/default/src/features/profile/components/dialogs/access-token-dialog.tsxweb/default/src/features/profile/components/dialogs/change-password-dialog.tsxweb/default/src/features/profile/components/dialogs/delete-account-dialog.tsxweb/default/src/features/profile/components/dialogs/email-bind-dialog.tsxweb/default/src/features/profile/components/dialogs/telegram-bind-dialog.tsxweb/default/src/features/profile/components/dialogs/two-fa-backup-dialog.tsxweb/default/src/features/profile/components/dialogs/two-fa-disable-dialog.tsxweb/default/src/features/profile/components/dialogs/two-fa-setup-dialog.tsxweb/default/src/features/profile/components/dialogs/wechat-bind-dialog.tsxweb/default/src/features/subscriptions/components/dialogs/subscription-purchase-dialog.tsxweb/default/src/features/system-settings/auth/custom-oauth/components/provider-form-dialog.tsxweb/default/src/features/system-settings/content/announcements-section.tsxweb/default/src/features/system-settings/content/api-info-section.tsxweb/default/src/features/system-settings/content/chat-dialog.tsxweb/default/src/features/system-settings/content/faq-section.tsxweb/default/src/features/system-settings/content/uptime-kuma-section.tsxweb/default/src/features/system-settings/general/channel-affinity/cache-stats-dialog.tsxweb/default/src/features/system-settings/general/channel-affinity/index.tsxweb/default/src/features/system-settings/general/channel-affinity/rule-editor-dialog.tsxweb/default/src/features/system-settings/integrations/amount-discount-dialog.tsxweb/default/src/features/system-settings/integrations/creem-product-dialog.tsxweb/default/src/features/system-settings/integrations/payment-method-dialog.tsxweb/default/src/features/system-settings/integrations/waffo-settings-section.tsxweb/default/src/features/system-settings/maintenance/update-checker-section.tsxweb/default/src/features/system-settings/models/channel-selector-dialog.tsxweb/default/src/features/system-settings/models/group-ratio-visual-editor.tsxweb/default/src/features/system-settings/request-limits/rate-limit-dialog.tsxweb/default/src/features/usage-logs/components/dialogs/audio-preview-dialog.tsxweb/default/src/features/usage-logs/components/dialogs/details-dialog.tsxweb/default/src/features/usage-logs/components/dialogs/fail-reason-dialog.tsxweb/default/src/features/usage-logs/components/dialogs/image-dialog.tsxweb/default/src/features/usage-logs/components/dialogs/prompt-dialog.tsxweb/default/src/features/usage-logs/components/dialogs/user-info-dialog.tsxweb/default/src/features/users/components/dialogs/user-binding-dialog.tsxweb/default/src/features/users/components/user-quota-dialog.tsxweb/default/src/features/wallet/components/dialogs/billing-history-dialog.tsxweb/default/src/features/wallet/components/dialogs/creem-confirm-dialog.tsxweb/default/src/features/wallet/components/dialogs/transfer-dialog.tsxweb/default/src/i18n/locales/en.jsonweb/default/src/i18n/locales/fr.jsonweb/default/src/i18n/locales/ja.jsonweb/default/src/i18n/locales/ru.jsonweb/default/src/i18n/locales/vi.jsonweb/default/src/i18n/locales/zh.json
Comment on lines
+51
to
+66
| export function Dialog({ | ||
| title, | ||
| description, | ||
| children, | ||
| trigger, | ||
| footer, | ||
| contentHeight = 'auto', | ||
| contentClassName, | ||
| headerClassName, | ||
| titleClassName, | ||
| descriptionClassName, | ||
| bodyClassName, | ||
| footerClassName, | ||
| initialFocus, | ||
| showCloseButton, | ||
| ...dialogProps |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Refactor to avoid props destructuring.
The coding guidelines require using props.xxx directly instead of destructuring component props for clarity. As per coding guidelines, "Do not destructure component props; use props.xxx directly instead for clarity".
♻️ Refactor to use props directly
-export function Dialog({
- title,
- description,
- children,
- trigger,
- footer,
- contentHeight = 'auto',
- contentClassName,
- headerClassName,
- titleClassName,
- descriptionClassName,
- bodyClassName,
- footerClassName,
- initialFocus,
- showCloseButton,
- ...dialogProps
-}: DialogProps) {
+export function Dialog(props: DialogProps) {
+ const {
+ title,
+ description,
+ children,
+ trigger,
+ footer,
+ contentHeight = 'auto',
+ contentClassName,
+ headerClassName,
+ titleClassName,
+ descriptionClassName,
+ bodyClassName,
+ footerClassName,
+ initialFocus,
+ showCloseButton,
+ ...dialogProps
+ } = props
+
return (
- <DialogRoot {...dialogProps}>
- {trigger ? <DialogTrigger render={trigger} /> : null}
+ <DialogRoot {...dialogProps}>
+ {props.trigger ? <DialogTrigger render={props.trigger} /> : null}
<DialogContent
className={cn(
'flex max-h-[calc(100vh-2rem)] w-full flex-col gap-4 overflow-hidden p-4 sm:max-w-2xl sm:p-6',
- contentClassName,
+ props.contentClassName,
dialogContentMotionClassName
)}
- initialFocus={initialFocus}
- showCloseButton={showCloseButton}
+ initialFocus={props.initialFocus}
+ showCloseButton={props.showCloseButton}
style={
{
- '--dialog-content-height': contentHeight,
+ '--dialog-content-height': props.contentHeight ?? 'auto',
} as React.CSSProperties
}
>
<DialogHeader
- className={cn('flex-shrink-0 text-start', headerClassName)}
+ className={cn('flex-shrink-0 text-start', props.headerClassName)}
>
- <DialogTitle className={titleClassName}>{title}</DialogTitle>
- {description ? (
- <DialogDescription className={descriptionClassName}>
- {description}
+ <DialogTitle className={props.titleClassName}>{props.title}</DialogTitle>
+ {props.description ? (
+ <DialogDescription className={props.descriptionClassName}>
+ {props.description}
</DialogDescription>
) : null}
</DialogHeader>
<div
className={cn(
'-mx-1 min-h-0 overflow-x-hidden overflow-y-auto overscroll-contain',
'h-[var(--dialog-content-height)] max-h-[calc(100vh-14rem)]'
)}
>
<div
className={cn(
'min-w-0 px-1 py-1',
'[&_form]:overflow-x-visible',
'[&_[data-slot=scroll-area-viewport]]:px-1 [&_[data-slot=scroll-area-viewport]]:py-1',
- bodyClassName
+ props.bodyClassName
)}
>
- {children}
+ {props.children}
</div>
</div>
- {footer ? (
+ {props.footer ? (
<DialogFooter
className={cn(
'flex-shrink-0 gap-2 sm:-mx-6 sm:-mb-6 sm:justify-end sm:p-6',
- footerClassName
+ props.footerClassName
)}
>
- {footer}
+ {props.footer}
</DialogFooter>
) : null}
</DialogContent>
</DialogRoot>
)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/default/src/components/dialog.tsx` around lines 51 - 66, The Dialog
component currently destructures its props in the function signature (symbols:
Dialog, title, description, children, trigger, footer, contentHeight,
contentClassName, headerClassName, titleClassName, descriptionClassName,
bodyClassName, footerClassName, initialFocus, showCloseButton, dialogProps);
refactor it to accept a single props parameter and replace all uses of the
destructured names with props.xxx (e.g., props.title, props.description,
props.initialFocus, etc.), keeping default values (like contentHeight = 'auto')
applied inside the function body (e.g., const contentHeight =
props.contentHeight ?? 'auto') and leaving the rest of the implementation and
exported name unchanged.
Source: Coding guidelines
| <Checkbox | ||
| checked={checked} | ||
| onCheckedChange={(v) => toggleSelected(m.id, !!v)} | ||
| aria-label={`Select model ${m.id}`} |
Contributor
There was a problem hiding this comment.
Localize the checkbox aria-label.
Line 527 hardcodes English assistive text, so screen-reader users won’t get localized output.
🌐 Suggested fix
- aria-label={`Select model ${m.id}`}
+ aria-label={t('Select model {{model}}', { model: m.id })}As per coding guidelines, all user-facing text content in web/default/**/*.{tsx,ts} should be localized via t().
📝 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
| aria-label={`Select model ${m.id}`} | |
| aria-label={t('Select model {{model}}', { model: m.id })} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@web/default/src/features/channels/components/dialogs/ollama-models-dialog.tsx`
at line 527, Replace the hardcoded aria-label={`Select model ${m.id}`} in the
OllamaModelsDialog component with a localized string using the t() i18n helper
(e.g. t('selectModel', { id: m.id })) so assistive text is translated; update
the translation key (selectModel) in the relevant locales and ensure any
interpolation uses m.id; locate the attribute on the checkbox element where
aria-label is currently set and swap the literal for the t(...) call.
Source: Coding guidelines
Comment on lines
+80
to
+83
| <Dialog | ||
| open={open} | ||
| onOpenChange={onOpenChange} | ||
| title={ |
Contributor
There was a problem hiding this comment.
Reset checklist/input state on all close paths.
Line 82 bypasses handleCancel, so Escape/backdrop close keeps previous confirmation state. On reopen, users can inherit prior checks/text in a high-risk flow.
🛡️ Suggested fix
- onOpenChange={onOpenChange}
+ onOpenChange={(nextOpen) => {
+ if (!nextOpen) {
+ handleCancel()
+ return
+ }
+ onOpenChange(true)
+ }}📝 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
| <Dialog | |
| open={open} | |
| onOpenChange={onOpenChange} | |
| title={ | |
| <Dialog | |
| open={open} | |
| onOpenChange={(nextOpen) => { | |
| if (!nextOpen) { | |
| handleCancel() | |
| return | |
| } | |
| onOpenChange(true) | |
| }} | |
| title={ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@web/default/src/features/channels/components/dialogs/status-code-risk-dialog.tsx`
around lines 80 - 83, The dialog currently closes via the Dialog onOpenChange
path (Escape/backdrop) without calling handleCancel, leaving checklist/input
state populated; update the close handling so all close paths reset state by
either wrapping onOpenChange to call handleCancel when newOpen is false or add a
useEffect watching open that invokes the same reset logic (e.g.,
resetConfirmationState or handleCancel) whenever open becomes false; reference
the Dialog's open/onOpenChange props and the existing
handleCancel/resetConfirmationState functions to ensure the checklist and input
are cleared on every close path.
Comment on lines
+184
to
+191
| <Button | ||
| size='sm' | ||
| className='flex-shrink-0 gap-1' | ||
| onClick={() => handleConfigureModel(modelName)} | ||
| > | ||
| <Plus className='h-4 w-4' /> | ||
| Configure | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Missing i18n for button text.
Line 190 contains hardcoded English text Configure that should be wrapped in t() for internationalization support. As per coding guidelines, all user-facing text content must support i18n.
🌐 Proposed fix
<Button
size='sm'
className='flex-shrink-0 gap-1'
onClick={() => handleConfigureModel(modelName)}
>
<Plus className='h-4 w-4' />
- Configure
+ {t('Configure')}
</Button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Button | |
| size='sm' | |
| className='flex-shrink-0 gap-1' | |
| onClick={() => handleConfigureModel(modelName)} | |
| > | |
| <Plus className='h-4 w-4' /> | |
| Configure | |
| </Button> | |
| <Button | |
| size='sm' | |
| className='flex-shrink-0 gap-1' | |
| onClick={() => handleConfigureModel(modelName)} | |
| > | |
| <Plus className='h-4 w-4' /> | |
| {t('Configure')} | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/default/src/features/models/components/dialogs/missing-models-dialog.tsx`
around lines 184 - 191, The button label is hardcoded ("Configure") in the JSX
for the Button component; update the text to use the i18n helper by replacing
the literal with t('Configure') (or the appropriate key like
t('models.configure') if your app uses keys) so the Button inside the
missing-models-dialog component uses internationalized text; locate the Button
with onClick={() => handleConfigureModel(modelName)} and change its children to
call t(...) instead of the raw string.
Source: Coding guidelines
Comment on lines
+172
to
+203
| <Dialog | ||
| open={props.open} | ||
| onOpenChange={props.onOpenChange} | ||
| title={isEditing ? t('Edit OAuth Provider') : t('Add OAuth Provider')} | ||
| description={ | ||
| isEditing | ||
| ? t('Update the configuration for this custom OAuth provider.') | ||
| : t('Configure a new custom OAuth provider for user authentication.') | ||
| } | ||
| contentClassName='max-h-[85vh] overflow-y-auto sm:max-w-2xl' | ||
| contentHeight='auto' | ||
| bodyClassName='space-y-4' | ||
| footer={ | ||
| <> | ||
| <Button | ||
| type='button' | ||
| variant='outline' | ||
| onClick={() => props.onOpenChange(false)} | ||
| disabled={isPending} | ||
| > | ||
| {t('Cancel')} | ||
| </Button> | ||
| <Button type='submit' form={PROVIDER_FORM_ID} disabled={isPending}> | ||
| {isPending | ||
| ? t('Saving...') | ||
| : isEditing | ||
| ? t('Update Provider') | ||
| : t('Create Provider')} | ||
| </Button> | ||
| </> | ||
| } | ||
| > |
Contributor
There was a problem hiding this comment.
Critical: contentClassName breaks Dialog overflow model.
Line 181 sets overflow-y-auto and max-h-[85vh] in contentClassName, which override the Dialog's base overflow-hidden and max-h-[calc(100vh-2rem)] classes. The Dialog component expects overflow-hidden on DialogContent with scrolling handled by the inner body container. Overriding these classes breaks the layout and scroll behavior.
🔧 Recommended fix
Remove overflow-y-auto and max-h-[85vh] from contentClassName:
- contentClassName='max-h-[85vh] overflow-y-auto sm:max-w-2xl'
+ contentClassName='sm:max-w-2xl'The Dialog already handles overflow and max-height internally. Use contentHeight to control the body scroll area if needed.
📝 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
| <Dialog | |
| open={props.open} | |
| onOpenChange={props.onOpenChange} | |
| title={isEditing ? t('Edit OAuth Provider') : t('Add OAuth Provider')} | |
| description={ | |
| isEditing | |
| ? t('Update the configuration for this custom OAuth provider.') | |
| : t('Configure a new custom OAuth provider for user authentication.') | |
| } | |
| contentClassName='max-h-[85vh] overflow-y-auto sm:max-w-2xl' | |
| contentHeight='auto' | |
| bodyClassName='space-y-4' | |
| footer={ | |
| <> | |
| <Button | |
| type='button' | |
| variant='outline' | |
| onClick={() => props.onOpenChange(false)} | |
| disabled={isPending} | |
| > | |
| {t('Cancel')} | |
| </Button> | |
| <Button type='submit' form={PROVIDER_FORM_ID} disabled={isPending}> | |
| {isPending | |
| ? t('Saving...') | |
| : isEditing | |
| ? t('Update Provider') | |
| : t('Create Provider')} | |
| </Button> | |
| </> | |
| } | |
| > | |
| <Dialog | |
| open={props.open} | |
| onOpenChange={props.onOpenChange} | |
| title={isEditing ? t('Edit OAuth Provider') : t('Add OAuth Provider')} | |
| description={ | |
| isEditing | |
| ? t('Update the configuration for this custom OAuth provider.') | |
| : t('Configure a new custom OAuth provider for user authentication.') | |
| } | |
| contentClassName='sm:max-w-2xl' | |
| contentHeight='auto' | |
| bodyClassName='space-y-4' | |
| footer={ | |
| <> | |
| <Button | |
| type='button' | |
| variant='outline' | |
| onClick={() => props.onOpenChange(false)} | |
| disabled={isPending} | |
| > | |
| {t('Cancel')} | |
| </Button> | |
| <Button type='submit' form={PROVIDER_FORM_ID} disabled={isPending}> | |
| {isPending | |
| ? t('Saving...') | |
| : isEditing | |
| ? t('Update Provider') | |
| : t('Create Provider')} | |
| </Button> | |
| </> | |
| } | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@web/default/src/features/system-settings/auth/custom-oauth/components/provider-form-dialog.tsx`
around lines 172 - 203, Dialog's contentClassName is overriding the
DialogContent's overflow and max-height (remove the conflicting classes to
restore Dialog's overflow-hidden and max-h calc behavior): in
provider-form-dialog.tsx remove `overflow-y-auto` and `max-h-[85vh]` from the
Dialog's contentClassName prop and instead rely on the Dialog's built-in
sizing/scrolling (use the existing contentHeight prop and bodyClassName/inner
container to control scrollable area if you need a specific height); ensure you
only keep layout/styling classes that don't change overflow/max-height so
Dialog, DialogContent, and the inner body container handle scrolling as
intended.
| <Copy className='size-4' /> | ||
| )} | ||
| </Button> | ||
| <p className='overflow-wrap-anywhere pr-10 text-sm leading-relaxed break-all whitespace-pre-wrap text-red-600'> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if overflow-wrap-anywhere is a custom Tailwind class
# Search for custom class definition in Tailwind config or CSS files
rg -n "overflow-wrap-anywhere" --type=css --type=js --type=ts -g '!node_modules' -g '!dist' -g '!build'Repository: QuantumNous/new-api
Length of output: 270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Broader search for any definition/usage of the class name, including config files that might not be .ts/.js/.css
rg -n "overflow-wrap-anywhere" --hidden --glob '!*node_modules/**' -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' -g '!**/.next/**' -g '!**/coverage/**'Repository: QuantumNous/new-api
Length of output: 270
🌐 Web query:
Tailwind CSS v4 "overflow-wrap-anywhere" utility class
💡 Result:
Tailwind CSS does not provide an "overflow-wrap-anywhere" utility class. Instead, the functionality for overflow-wrap: anywhere; is provided by the wrap-anywhere utility class [1][2]. This class is part of the overflow-wrap documentation [1]. It behaves similarly to wrap-break-word (overflow-wrap: break-word;), with the key distinction that the browser considers mid-word line breaks when calculating the intrinsic size of the element, which is particularly useful for preventing layout issues within flex containers [1][2]. For reference, the available utilities under the overflow-wrap category are: - wrap-normal (overflow-wrap: normal;) [1][2] - wrap-break-word (overflow-wrap: break-word;) [1][2] - wrap-anywhere (overflow-wrap: anywhere;) [1][2]
Citations:
- 1: https://tailwindcss.com/docs/overflow-wrap
- 2: https://github.com/tailwindlabs/tailwindcss.com/blob/main/src/docs/overflow-wrap.mdx
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Broader search for any definition/usage of the class name, including config files that might not be .ts/.js/.css
rg -n "overflow-wrap-anywhere" --hidden --glob '!*node_modules/**' -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' -g '!**/.next/**' -g '!**/coverage/**'Repository: QuantumNous/new-api
Length of output: 270
🌐 Web query:
Tailwind CSS v4 "overflow-wrap-anywhere" utility class
💡 Result:
There is no Tailwind CSS v4 utility class named exactly “overflow-wrap-anywhere”. Instead, Tailwind provides an overflow-wrap utility named “wrap-anywhere”. It sets CSS overflow-wrap: anywhere (used for wrapping within words, including in flex layouts). Answer: - Tailwind class: wrap-anywhere - CSS it applies: overflow-wrap: anywhere [1][2] Supporting details: - The Tailwind overflow-wrap docs list wrap-anywhere alongside wrap-break-word and wrap-normal, and explicitly describe “Wrapping anywhere” behavior for mid-word breaks and intrinsic sizing [1]. - The Tailwind v4.1 release notes describe “wrap-anywhere” as similar to wrap-break-word but allowing mid-word line breaks when calculating intrinsic size, and position it as useful inside flex containers [3].
Citations:
- 1: https://tailwindcss.com/docs/overflow-wrap
- 2: https://github.com/tailwindlabs/tailwindcss.com/blob/main/src/docs/overflow-wrap.mdx
- 3: https://tailwindcss.com/blog/tailwindcss-v4-1
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate potential Tailwind config files (common extensions)
fd -t f "tailwind.config.*" -a . -0 | xargs -0 -I{} echo "FOUND_TAILWIND_CONFIG: {}"
# Search for any mention/override in common config/style file types
rg -n "overflow-wrap-anywhere|overflow-wrap:\s*anywhere|wrap-anywhere" --hidden --glob '!*node_modules/**' -g '!**/dist/**' -g '!**/build/**' -g '!**/.next/**' -g '!**/coverage/**'Repository: QuantumNous/new-api
Length of output: 437
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect Tailwind config for any custom utility or theme extension related to overflow-wrap
tailwind_config="/home/jailuser/git/web/classic/tailwind.config.js"
# Print relevant sections only around matches
rg -n "overflow-wrap-anywhere|overflow-wrap:\s*anywhere|wrap-anywhere" "$tailwind_config" || true
# Also search for any custom utilities/plugins that might generate the class
rg -n "addUtilities|matchUtilities|theme\.extend|plugins\s*:" "$tailwind_config" || trueRepository: QuantumNous/new-api
Length of output: 82
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tailwind_config="/home/jailuser/git/web/classic/tailwind.config.js"
# Show the whole file only if it's small; otherwise sample key sections.
wc -l "$tailwind_config"
# Print sections likely relevant: theme.extend and safelist/plugins
nl -ba "$tailwind_config" | sed -n '1,220p'Repository: QuantumNous/new-api
Length of output: 178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tailwind_config="/home/jailuser/git/web/classic/tailwind.config.js"
# Print the config file with line numbers (using cat -n)
cat -n "$tailwind_config" | sed -n '1,220p'Repository: QuantumNous/new-api
Length of output: 8996
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "wrap-anywhere|\\[overflow-wrap:anywhere\\]" web/default/src -SRepository: QuantumNous/new-api
Length of output: 364
Fix invalid Tailwind class overflow-wrap-anywhere (line 71)
Tailwind provides wrap-anywhere (or the arbitrary form [overflow-wrap:anywhere]), and the repo doesn’t define a custom overflow-wrap-anywhere utility in web/classic/tailwind.config.js—so this class is likely a no-op.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@web/default/src/features/usage-logs/components/dialogs/fail-reason-dialog.tsx`
at line 71, The Tailwind utility used in the paragraph inside the
FailReasonDialog component is invalid: replace the non-existent class
overflow-wrap-anywhere with a valid Tailwind utility such as wrap-anywhere (or
the arbitrary variant [overflow-wrap:anywhere]) in the <p> element within
fail-reason-dialog.tsx so the text wrapping behavior is applied correctly;
update the className string on that <p> (the element containing 'text-red-600'
and 'whitespace-pre-wrap') to use wrap-anywhere or [overflow-wrap:anywhere].
Comment on lines
+413
to
+420
| <Button | ||
| variant='ghost' | ||
| size='sm' | ||
| className='text-destructive hover:text-destructive h-7 w-7 shrink-0 p-0' | ||
| onClick={() => setUnbindTarget(binding)} | ||
| > | ||
| <Unlink className='h-3.5 w-3.5' /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Add aria-label to icon-only button for screen reader users.
The Unlink button contains only an icon without text content. As per coding guidelines, ARIA attributes should be used when necessary to provide text equivalents for important information. Screen reader users need an accessible label to understand the button's purpose.
♿ Add accessible label
{binding.isBound && (
<Button
variant='ghost'
size='sm'
className='text-destructive hover:text-destructive h-7 w-7 shrink-0 p-0'
onClick={() => setUnbindTarget(binding)}
+ aria-label={t('Unbind {{provider}}', { provider: binding.label })}
>
<Unlink className='h-3.5 w-3.5' />
</Button>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Button | |
| variant='ghost' | |
| size='sm' | |
| className='text-destructive hover:text-destructive h-7 w-7 shrink-0 p-0' | |
| onClick={() => setUnbindTarget(binding)} | |
| > | |
| <Unlink className='h-3.5 w-3.5' /> | |
| </Button> | |
| <Button | |
| variant='ghost' | |
| size='sm' | |
| className='text-destructive hover:text-destructive h-7 w-7 shrink-0 p-0' | |
| onClick={() => setUnbindTarget(binding)} | |
| aria-label={t('Unbind {{provider}}', { provider: binding.label })} | |
| > | |
| <Unlink className='h-3.5 w-3.5' /> | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/default/src/features/users/components/dialogs/user-binding-dialog.tsx`
around lines 413 - 420, The Unlink icon-only Button (component Button wrapping
<Unlink />) is missing an accessible name; update the Button where onClick={()
=> setUnbindTarget(binding)} is used to include an aria-label (e.g.,
aria-label="Unbind user" or similar descriptive text) so screen readers can
announce the button purpose; locate the Button and add the aria-label prop while
keeping existing props and handler (setUnbindTarget) intact.
Source: Coding guidelines
11 tasks
OuYang-HX
pushed a commit
to OuYang-HX/new-api
that referenced
this pull request
Jun 13, 2026
Merge pull request QuantumNous#5346 from QuantumNous/perf/ui-dialog
zhaodechao2008
pushed a commit
to zhaodechao2008/new-api
that referenced
this pull request
Jul 27, 2026
Merge pull request QuantumNous#5346 from QuantumNous/perf/ui-dialog
330079598
pushed a commit
to 330079598/new-api
that referenced
this pull request
Aug 19, 2026
Merge pull request QuantumNous#5346 from QuantumNous/perf/ui-dialog
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
2 participants
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.
Important
📝 变更描述 / Description
(简述：做了什么？为什么这样改能生效？请基于你对代码逻辑的理解来写，避免粘贴未经整理的内容)
概述
改动说明
效果
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
(请在此粘贴截图、关键日志或测试报告，以证明变更生效)
Summary by CodeRabbit
Refactor
Localization