Skip to content

give image cluster priority over the emoji for group conversations#1140

Merged
cameronvoell merged 2 commits into
devfrom
feature/cluster-image-priority-over-emoji
Jul 8, 2026
Merged

give image cluster priority over the emoji for group conversations#1140
cameronvoell merged 2 commits into
devfrom
feature/cluster-image-priority-over-emoji

Conversation

@cameronvoell

@cameronvoell cameronvoell commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.

Note

Give image clusters priority over emoji for group conversation avatars

  • Updates Conversation.avatarType in Conversation.swift to return a clustered member avatar when any member has a profile photo, falling back to the seeded emoji only when no member avatars are available.
  • Updates ConversationsViewController.diff-changed-items to include avatarType in the change-detection predicate, so cells refresh when the avatar type changes.
  • Behavioral Change: group conversations with a seeded emoji will now show a member photo cluster instead if any members have avatars.

Macroscope summarized 5b78b5b.

Copy link
Copy Markdown
Contributor Author

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Claude finished @cameronvoell's task —— View job


Code Review Complete

Summary

This PR successfully prioritizes member avatar clusters over seeded conversation emojis for group conversations. The implementation is clean and well-tested.


Code Quality: ✅ Excellent

Conversation.swift (lines 232-258):

  • Logic change is clear and correct: checks member avatars before conversation emoji
  • Comment at lines 248-252 excellently explains the design decision
  • Follows existing patterns and conventions

View Controller Change:

  • Line 186 in ConversationsViewController.swift adds avatarType comparison to trigger cell reconfiguration when member avatars change
  • This is the key fix for stale cluster rendering - essential for making the priority change visible in the UI

Test Coverage: ✅ Comprehensive

New Test File (ConversationsRepositoryAvatarObservationTests.swift):

  • Tests database reactivity when member avatars change (lines 88-118)
  • Includes control test to verify observation mechanism works (lines 120-145)
  • Excellent comments explaining the bug hypothesis being tested (lines 7-21)

Updated Test File (DefaultConversationDisplayTests.swift):

  • Line 550: New test avatarTypeClustersOverSeededConversationEmoji specifically validates the priority change
  • Lines 553-555: Comment references the bug (feat: stable conversation emoji in shared metadata + invite payloads #686) and explains the decision reversal
  • Test creates a group with member avatars AND a seeded emoji, then verifies the cluster wins
  • Lines 605-609: Assertion confirms clustered avatar type with correct profile count

Potential Issues: ⚠️ Minor

1. Observation Test May Be Flaky
ConversationsRepositoryAvatarObservationTests.swift:99 and 113

  • Uses hardcoded sleep durations (300ms, 800ms) for async observation
  • These timing-based tests can fail on slow CI machines
  • Recommendation: Consider using expectation-based waiting or increase timeouts with documentation

2. EmissionCounter Thread Safety
ConversationsRepositoryAvatarObservationTests.swift:148-161

  • Uses @unchecked Sendable with manual NSLock synchronization
  • Implementation is correct, but Swift 6 strict concurrency may flag this
  • Recommendation: Consider using OSAllocatedUnfairLock or actor-based counter for future-proofing

Performance: ✅ Good

  • hasAnyAvatar check (line 254) is efficient - short-circuits on first avatar found
  • sortedForCluster().prefix(7) limits cluster size appropriately
  • No performance concerns introduced

Security: ✅ No Concerns

No security implications in this change.


Suggestions for Improvement

Optional Enhancement:
Consider adding a test case for the scenario where:

  • A group starts with no member avatars (shows emoji)
  • A member adds an avatar
  • The UI should switch from emoji to clustered avatars

This would validate the full reactivity chain end-to-end.


Verdict: ✅ Approve

This PR successfully addresses the stated issue with clean code, comprehensive tests, and good documentation. The minor issues noted are not blockers.


@cameronvoell cameronvoell marked this pull request as ready for review July 7, 2026 15:40
@cameronvoell cameronvoell requested a review from yewreeka July 7, 2026 15:40
@macroscopeapp

macroscopeapp Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved

This PR reorders avatar display priority for group conversations (showing clustered member photos over auto-seeded emoji) and fixes a UI refresh issue. The changes are self-contained, well-tested, and limited to visual display logic with no broader runtime implications.

You can customize Macroscope's approvability policy. Learn more.

@cameronvoell cameronvoell merged commit 802c6b0 into dev Jul 8, 2026
12 checks passed
@cameronvoell cameronvoell deleted the feature/cluster-image-priority-over-emoji branch July 8, 2026 19:27
yewreeka added a commit that referenced this pull request Jul 8, 2026
Since member avatars outrank the seeded conversation emoji (#1140), the
empty-state mock's members - whose Profile.mock avatars are fake URLs
that never load - rendered the CTA as a cluster of gray initials instead
of the convo emoji. Build those members with avatar-less profiles so
avatarType falls through to the supplied emoji, and pin that with a
regression test.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant