Merge pull request #396 from opf/bug/69230-invalid-src-fallback

Bug/69230 Handle 404 errors in AvatarWithFallback with client-side fallback

Author: akabiru

.changeset/olive-eyes-juggle.md

@@ -0,0 +1,5 @@
+---
+'@openproject/primer-view-components': patch
+---
+
+Handle 404 errors in AvatarWithFallback with client-side fallback

app/components/primer/open_project/avatar_fallback.ts

@@ -4,16 +4,57 @@ import {attr, controller} from '@github/catalyst'
 export class AvatarFallbackElement extends HTMLElement {
   @attr uniqueId = ''
   @attr altText = ''
+  @attr fallbackSrc = ''
+
+  private img: HTMLImageElement | null = null
+  private boundErrorHandler?: () => void
 
   connectedCallback() {
+    this.img = this.querySelector<HTMLImageElement>('img') ?? null
+    if (!this.img) return
+
+    this.boundErrorHandler = () => this.handleImageError(this.img!)
+
+    // Handle image load errors (404, network failure, etc.)
+    this.img.addEventListener('error', this.boundErrorHandler)
+
+    // Check if image already failed (error event fired before listener attached)
+    if (this.isImageBroken(this.img)) {
+      this.handleImageError(this.img)
+    } else if (this.isFallbackImage(this.img)) {
+      this.applyColor(this.img)
+    }
+  }
+
+  disconnectedCallback() {
+    if (this.boundErrorHandler && this.img) {
+      this.img.removeEventListener('error', this.boundErrorHandler)
+    }
+    this.boundErrorHandler = undefined
+    this.img = null
+  }
+
+  private isImageBroken(img: HTMLImageElement): boolean {
+    // Image is broken if loading completed but no actual image data loaded
+    // Skip check for data URIs (fallback SVGs) as they're always valid
+    return img.complete && img.naturalWidth === 0 && !img.src.startsWith('data:')
+  }
+
+  private handleImageError(img: HTMLImageElement) {
+    // Prevent infinite loop if fallback also fails
+    if (this.isFallbackImage(img)) return
+
+    if (this.fallbackSrc) {
+      img.src = this.fallbackSrc
+      this.applyColor(img)
+    }
+  }
+
+  private applyColor(img: HTMLImageElement) {
     // If either uniqueId or altText is missing, skip color customization so the SVG
     // keeps its default gray fill defined in the source and no color override is applied.
     if (!this.uniqueId || !this.altText) return
 
-    const img = this.querySelector<HTMLImageElement>('img[src^="data:image/svg+xml"]')
-    if (!img) return
-
-    // Generate consistent color based on uniqueId and altText (hash must match OP Core)
     const text = `${this.uniqueId}${this.altText}`
     const hue = this.valueHash(text)
     const color = `hsl(${hue}, 50%, 30%)`
@@ -46,4 +87,8 @@ export class AvatarFallbackElement extends HTMLElement {
       // to avoid breaking the component.
     }
   }
+
+  private isFallbackImage(img: HTMLImageElement): boolean {
+    return img.src === this.fallbackSrc
+  }
 }

app/components/primer/open_project/avatar_with_fallback.rb

@@ -21,7 +21,7 @@ class AvatarWithFallback < Primer::Beta::Avatar
       #   - https://github.com/primer/css/blob/main/src/support/variables/typography.scss
       FONT_STACK = "-apple-system, BlinkMacSystemFont, 'Segoe UI', 'Noto Sans', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji'"
 
-      # @param src [String] The source url of the avatar image. When nil, renders a fallback with initials.
+      # @param src [String] The source url of the avatar image. When nil or a broken URL, it renders a fallback with initials.
       # @param alt [String] Alt text for the avatar. Used for accessibility and to generate initials when src is nil.
       # @param size [Integer] <%= one_of(Primer::Beta::Avatar::SIZE_OPTIONS) %>
       # @param shape [Symbol] Shape of the avatar. <%= one_of(Primer::Beta::Avatar::SHAPE_OPTIONS) %>
@@ -32,20 +32,20 @@ def initialize(src: nil, alt: nil, size: DEFAULT_SIZE, shape: DEFAULT_SHAPE, hre
         require_src_or_alt_arguments(src, alt)
 
         @unique_id = unique_id
-        @use_fallback = src.blank?
-        final_src = @use_fallback ? generate_fallback_svg(alt, size) : src
+        @fallback_svg = generate_fallback_svg(alt, size)
+        final_src = src.blank? ? @fallback_svg : src
 
         super(src: final_src, alt: alt, size: size, shape: shape, href: href, **system_arguments)
       end
 
       def call
         render(
-          Primer::ConditionalWrapper.new(
-            condition: @use_fallback,
+          Primer::BaseComponent.new(
             tag: :"avatar-fallback",
             data: {
               unique_id: @unique_id,
-              alt_text: @system_arguments[:alt]
+              alt_text: @system_arguments[:alt],
+              fallback_src: @fallback_svg
             }
           )
         ) { super }

previews/primer/open_project/avatar_with_fallback_preview.rb

@@ -66,6 +66,26 @@ def fallback_as_link
       end
       #
       # @!endgroup
+
+      # @!group Error Handling (404 Fallback)
+      #
+      # @label Broken image (404)
+      # @snapshot
+      def broken_image_404
+        # Uses a non-existent URL - will trigger error handler and show fallback SVG
+        render(Primer::OpenProject::AvatarWithFallback.new(
+          src: "/non-existent-avatar.png",
+          alt: "User With Missing Avatar",
+          unique_id: 42
+        ))
+      end
+
+      # @label Multiple broken images
+      def multiple_broken_images
+        render_with_template(locals: {})
+      end
+      #
+      # @!endgroup
     end
   end
 end

previews/primer/open_project/avatar_with_fallback_preview/multiple_broken_images.html.erb

@@ -0,0 +1,10 @@
+<div class="d-flex gap-3 flex-items-center">
+  <p class="color-fg-muted">These avatars have broken image URLs and will fallback to SVG initials:</p>
+</div>
+<div class="d-flex gap-3 flex-items-center mt-2">
+  <%= render(Primer::OpenProject::AvatarWithFallback.new(src: "/missing/alice.png", alt: "Alice Johnson", unique_id: 10)) %>
+  <%= render(Primer::OpenProject::AvatarWithFallback.new(src: "/missing/bob.png", alt: "Bob Smith", unique_id: 20)) %>
+  <%= render(Primer::OpenProject::AvatarWithFallback.new(src: "/missing/charlie.png", alt: "Charlie Brown", unique_id: 30)) %>
+  <%= render(Primer::OpenProject::AvatarWithFallback.new(src: "https://example.com/404.png", alt: "Diana Prince", unique_id: 40)) %>
+  <%= render(Primer::OpenProject::AvatarWithFallback.new(src: "https://invalid-domain-12345.com/avatar.png", alt: "Eve Anderson", unique_id: 50)) %>
+</div>

static/arguments.json

@@ -5775,7 +5775,7 @@
         "name": "src",
         "type": "String",
         "default": "`nil`",
-        "description": "The source url of the avatar image. When nil, renders a fallback with initials."
+        "description": "The source url of the avatar image. When nil or a broken URL, it renders a fallback with initials."
       },
       {
         "name": "alt",

static/info_arch.json

@@ -18882,7 +18882,7 @@
         "name": "src",
         "type": "String",
         "default": "`nil`",
-        "description": "The source url of the avatar image. When nil, renders a fallback with initials."
+        "description": "The source url of the avatar image. When nil or a broken URL, it renders a fallback with initials."
       },
       {
         "name": "alt",
@@ -19040,6 +19040,32 @@
             "color-contrast"
           ]
         }
+      },
+      {
+        "preview_path": "primer/open_project/avatar_with_fallback/broken_image_404",
+        "name": "broken_image_404",
+        "snapshot": "true",
+        "skip_rules": {
+          "wont_fix": [
+            "region"
+          ],
+          "will_fix": [
+            "color-contrast"
+          ]
+        }
+      },
+      {
+        "preview_path": "primer/open_project/avatar_with_fallback/multiple_broken_images",
+        "name": "multiple_broken_images",
+        "snapshot": "false",
+        "skip_rules": {
+          "wont_fix": [
+            "region"
+          ],
+          "will_fix": [
+            "color-contrast"
+          ]
+        }
       }
     ],
     "subcomponents": []

static/previews.json

@@ -1633,6 +1633,32 @@
             "color-contrast"
           ]
         }
+      },
+      {
+        "preview_path": "primer/open_project/avatar_with_fallback/broken_image_404",
+        "name": "broken_image_404",
+        "snapshot": "true",
+        "skip_rules": {
+          "wont_fix": [
+            "region"
+          ],
+          "will_fix": [
+            "color-contrast"
+          ]
+        }
+      },
+      {
+        "preview_path": "primer/open_project/avatar_with_fallback/multiple_broken_images",
+        "name": "multiple_broken_images",
+        "snapshot": "false",
+        "skip_rules": {
+          "wont_fix": [
+            "region"
+          ],
+          "will_fix": [
+            "color-contrast"
+          ]
+        }
       }
     ]
   },

test/components/open_project/avatar_stack_test.rb

@@ -41,7 +41,8 @@ def test_renders_mixed_avatars
       assert_selector(".AvatarStack-body") do
         # 2 img tags total: 1 with remote src, 1 with data URI fallback
         assert_selector("img.avatar", count: 2)
-        assert_selector("avatar-fallback", count: 1)
+        # All avatars wrapped in avatar-fallback for 404 error handling
+        assert_selector("avatar-fallback", count: 2)
         assert_selector("img.avatar[src^='data:image/svg+xml;base64,']", count: 1)
       end
     end

test/components/open_project/avatar_with_fallback_test.rb

@@ -8,8 +8,27 @@ class PrimerOpenProjectAvatarWithFallbackTest < Minitest::Test
   def test_renders_image_avatar_with_src
     render_inline(Primer::OpenProject::AvatarWithFallback.new(src: "https://github.com/github.png", alt: "github"))
 
-    assert_selector("img.avatar")
-    refute_selector("avatar-fallback")
+    # Always wrapped in avatar-fallback for 404 error handling
+    assert_selector("avatar-fallback[data-fallback-src^='data:image/svg+xml;base64,']") do
+      assert_selector("img.avatar[src='https://github.com/github.png']")
+    end
+  end
+
+  def test_image_avatar_error_handling_setup
+    render_inline(Primer::OpenProject::AvatarWithFallback.new(src: "https://example.com/avatar.png", alt: "Alice Johnson", unique_id: 123))
+
+    # Original src is preserved (client-side JS handles error -> fallback swap)
+    assert_selector("avatar-fallback[data-unique-id='123'][data-alt-text='Alice Johnson']") do
+      assert_selector("img.avatar[src='https://example.com/avatar.png']")
+    end
+
+    # Verify fallback SVG is available and contains correct initials
+    fallback_wrapper = page.find("avatar-fallback")
+    fallback_src = fallback_wrapper["data-fallback-src"]
+
+    assert fallback_src.start_with?("data:image/svg+xml;base64,")
+    svg_content = Base64.decode64(fallback_src.sub("data:image/svg+xml;base64,", ""))
+    assert_includes svg_content, ">AJ<", "Fallback SVG should contain initials 'AJ'"
   end
 
   def test_renders_fallback_when_src_is_nil