[FIX] Removed the outer border, radius, and surface background#52
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves how member avatars/names are resolved and displayed on the Modules page by hardening the Avatar component’s fallback behavior, introducing a more robust workspace-member lookup helper, and updating ModulesPage to use these utilities while refining some layout styling.
Changes:
- Refactors
Avatarto gracefully fall back to initials on image load failure and to generate initials more robustly. - Adds
findWorkspaceMemberByUserId(with ID normalization + legacy field support) and expandsgetImageUrlhandling. - Updates Modules page lead lookup/display logic and adjusts list layout container styling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ui/src/pages/ModulesPage.tsx | Uses robust member lookup + improved lead name/avatar selection; UI tweaks for list/gallery/table lead display. |
| ui/src/lib/utils.ts | Adds member lookup helper and hardens getImageUrl normalization/accepted schemes. |
| ui/src/components/ui/Avatar.tsx | Adds image-error handling and improved initials fallback logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (id === '') return null; | ||
| const m = findWorkspaceMemberByUserId(members, id); | ||
| const fromDisplay = m?.member_display_name?.trim() ?? ''; | ||
| const fromEmail = m?.member_email?.trim().split('@')[0]?.trim() ?? ''; |
There was a problem hiding this comment.
fromEmail can throw when member_email is undefined or null: m?.member_email?.trim() can evaluate to undefined, but .split('@') is then called unconditionally. Use optional chaining on split (e.g., m?.member_email?.trim()?.split('@')[0]) or normalize with a default empty string before splitting.
| const fromEmail = m?.member_email?.trim().split('@')[0]?.trim() ?? ''; | |
| const fromEmail = m?.member_email?.trim()?.split('@')[0]?.trim() ?? ''; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( | ||
| t.startsWith('http://') || | ||
| t.startsWith('https://') || | ||
| t.startsWith('data:') || | ||
| t.startsWith('blob:') | ||
| ) { | ||
| return t; | ||
| } |
There was a problem hiding this comment.
getImageUrl now passes through data: and blob: URLs unchanged. If member_avatar/logo values can be influenced by users or external sources, allowing data: URLs can bypass URL validation and may enable oversized payloads or SVG-based attack surface. Consider restricting to a safer allowlist (e.g., only http/https + relative paths, or data:image/* only if explicitly required) and documenting the intended sources for these URLs.
| if ( | |
| t.startsWith('http://') || | |
| t.startsWith('https://') || | |
| t.startsWith('data:') || | |
| t.startsWith('blob:') | |
| ) { | |
| return t; | |
| } | |
| // Allow only http/https URLs and, if needed, inline image data URLs. | |
| if (t.startsWith('http://') || t.startsWith('https://')) { | |
| return t; | |
| } | |
| if (t.startsWith('data:image/')) { | |
| return t; | |
| } |
| export function Avatar({ name, src, size = 'md', className }: AvatarProps) { | ||
| const resolvedSrc = src?.trim() ?? ''; | ||
| const remountKey = resolvedSrc !== '' ? resolvedSrc : `initials:${name}`; | ||
|
|
||
| return ( | ||
| <AvatarInner | ||
| key={remountKey} | ||
| name={name} | ||
| resolvedSrc={resolvedSrc} | ||
| size={size} | ||
| className={className} |
There was a problem hiding this comment.
Avatar uses the full resolvedSrc string as a React key to force a remount. This can become very large if a data: URL is ever used (base64), increasing reconciliation overhead and memory churn. Prefer resetting imgFailed via an effect on resolvedSrc (or using a short derived key/hash) instead of remounting with the entire URL as the key.
| @@ -24,21 +31,52 @@ const sizeStyles: Record<AvatarSize, string> = { | |||
| lg: 'h-10 w-10 text-base', | |||
| }; | |||
|
|
|||
| export function Avatar({ name, src, size = 'md', className }: AvatarProps) { | |||
| type AvatarInnerProps = { | |||
| name: string; | |||
| resolvedSrc: string; | |||
| size: AvatarSize; | |||
| className?: string; | |||
| }; | |||
|
|
|||
| /** Holds image error state; parent remounts via `key` when `resolvedSrc` changes. */ | |||
| function AvatarInner({ name, resolvedSrc, size, className }: AvatarInnerProps) { | |||
| const [imgFailed, setImgFailed] = useState(false); | |||
| const showImg = resolvedSrc !== '' && !imgFailed; | |||
|
|
|||
| return ( | |||
| <span | |||
| className={cn( | |||
| 'inline-flex shrink-0 items-center justify-center rounded-full bg-(--bg-accent-primary) font-medium text-(--txt-on-color)', | |||
| 'inline-flex shrink-0 items-center justify-center overflow-hidden rounded-full bg-(--bg-accent-primary) font-medium text-(--txt-on-color)', | |||
| sizeStyles[size], | |||
| className, | |||
| )} | |||
| title={name} | |||
| > | |||
| {src ? ( | |||
| <img src={src} alt={name} className="h-full w-full rounded-full object-cover" /> | |||
| {showImg ? ( | |||
| <img | |||
| src={resolvedSrc} | |||
| alt={name} | |||
| className="h-full w-full object-cover" | |||
| onError={() => setImgFailed(true)} | |||
| /> | |||
| ) : ( | |||
| getInitials(name) | |||
| )} | |||
There was a problem hiding this comment.
When name is empty/whitespace, the fallback initials now render as ?, but the <img alt>/title will still be empty when an image exists, and the text fallback isn't an accessible name. Consider deriving a non-empty label (e.g., const label = name.trim() || 'User') and using it consistently for alt, title, and initials generation so screen readers don't announce just ?/initials.
| {rowLead ? ( | ||
| <Avatar | ||
| name={rowLead.name} | ||
| src={getImageUrl(rowLead.avatarUrl) ?? undefined} | ||
| size="sm" | ||
| className="size-5 shrink-0 text-[9px]" | ||
| /> | ||
| ) : ( | ||
| <span className="flex size-4 shrink-0 items-center justify-center text-(--txt-icon-tertiary)"> | ||
| <svg | ||
| width="12" | ||
| height="12" | ||
| viewBox="0 0 24 24" | ||
| fill="none" | ||
| stroke="currentColor" | ||
| strokeWidth="2" | ||
| aria-hidden | ||
| > | ||
| <circle cx="12" cy="12" r="9" /> | ||
| <path d="M12 6v6l4 2" /> | ||
| </svg> | ||
| </span> | ||
| )} | ||
| <span className="min-w-0 truncate">{mod.name}</span> | ||
| </Link> |
There was a problem hiding this comment.
PR description says list and table layouts "always show the lead's avatar and name", but the updated timeline table only adds the lead avatar next to the module name (no lead name is rendered). Either update the UI here to include the lead name (if that's the intent) or adjust the PR description to match the actual behavior.
martian56
changed the title
This pull request improves the display and robustness of user avatars and workspace member lookups, particularly for module leads in the modules page. The changes enhance avatar fallback logic, ensure more reliable member matching, and refine the UI to consistently show lead avatars and names across different layouts.
Avatar handling improvements:
Avatarcomponent to handle image load failures gracefully, show initials as fallback, and improve how initials are generated (e.g., handling single-word names and empty names more robustly). [1] [2] [3]Workspace member lookup enhancements:
findWorkspaceMemberByUserIdutility to reliably match workspace members by user ID, supporting bothmember_idand legacyidfields, and improved normalization of IDs for matching. [1] [2]Modules page updates:
Avatarcomponent, with appropriate fallbacks if data is missing. [1] [2] [3] [4]These changes collectively improve the reliability and user experience of displaying member avatars and names throughout the application.