Only show user avatars if organizer_signed_in?

[sampoder]

I think this will reduce the amount of requests we get as each image is a new request

[sampoder]

@claude can you review?

[claude]

Claude finished @sampoder's task —— View job

---

Code Review ✅

Tasks:

• Check for CLAUDE.md repo guidelines
• Read the modified file
• Analyze the changes
• Provide review feedback

---

Summary

This PR conditionally hides the user avatar column when an organizer is not signed in, which should reduce the number of HTTP requests for avatar images. The implementation is consistent and correct.

What Changed

The PR modifies app/views/events/ledger.html.erb:17-19,25,27 to:

1. Wrap the table header column in a conditional (lines 17-19)
2. Pass organizer_signed_in? to the show_author_column local instead of hardcoded true (lines 25, 27)

This ensures that both the header cell and the data cells are only rendered when an organizer is signed in.

Review Feedback

✅ Strengths

1. Consistency: The implementation correctly handles both places where the author column appears:

   • Table header <th> element (line 17-19)
   • Both partial renders that use show_author_column local (lines 25, 27)

2. Correct conditional logic: Both partials (_canonical_transaction.html.erb:142-154 and _canonical_pending_transaction.html.erb:135-147) already check if local_assigns[:show_author_column], so changing the passed value from true to organizer_signed_in? will properly hide/show the column.

3. Performance benefit: This should indeed reduce HTTP requests since:

   • Avatar images use avatar_for helper (line 141/148 in partials)
   • Fallback avatars use image_tag with loading: "lazy" (line 143/150 in partials)
   • Not rendering these cells = no image requests for non-organizers

4. Pattern consistency: The same organizer_signed_in? check pattern is already used elsewhere in the same file (line 87, 89, etc.)

🤔 Potential Considerations

1. User experience: Non-organizers will no longer see who created transactions. This seems intentional based on the PR description, but worth confirming this is the desired UX change (not just a performance optimization).

2. Table alignment: When the column is hidden for non-organizers but shown for organizers, this could create a slight visual difference between the two views. This is likely acceptable, but just noting it.

3. No test coverage visible: The PR doesn't include test updates. Consider adding a test to verify the column is hidden when organizer_signed_in? returns false and shown when it returns true.

Recommendation

✅ Approve with minor suggestion: The code changes are correct and should work as intended. The only suggestion is to add test coverage for this conditional rendering behavior.

Example test case (if you want to add it):

# In a view spec or system spec
context "when organizer is not signed in" do
  it "does not render author avatar column" do
    # ... setup ...
    expect(page).not_to have_css("th", text: /user avatar/i)
  end
end

context "when organizer is signed in" do
  it "renders author avatar column" do
    # ... setup ...
    expect(page).to have_css("th", text: /user avatar/i)
  end
end

---

[Luke-Oldenburg]

I think this is good to improve performance for now.