mirror of
https://github.com/block/buzz.git
synced 2026-11-11 11:51:13 +00:00
fix(mobile): steady New DM picker rows and animate picks (#9249)
On iOS, the New DM (and Add Members) picker is a native UIKit sheet that Flutter drives by pushing the picker's whole state on every rebuild. Each push called `reloadData`, which rebuilt every visible row in a fresh, unreused cell showing the initials fallback, then fetched each avatar again over the platform channel. So whenever anything refreshed (several refreshes land right after foregrounding), every avatar blinked to its initial for a frame or two. Picks also jumped instantly from the results to the chosen section. Now the picker: - ignores a pushed state identical to the one it shows; - applies the rest as a diffable snapshot, so unchanged rows keep their cells and changed rows are redrawn in place, in reused cells; - keeps loaded avatars by avatar key for the life of the sheet, so a redrawn row shows its avatar at once; - animates a member's row moving between the chosen and result sections when it's picked or removed (no animation with Reduce Motion). Flutter's toggle handlers in both pickers now read the current selection rather than the one captured at build time. A fast second tap could otherwise add the same person twice, which the diffed table would reject. ### Validation - Native member picker test (`scripts/test-mobile-member-picker.sh`), extended: after an avatar loads, a changed row is redrawn with that avatar and requests nothing more. The avatar check fails on `main` and passes here; the existing identity and accessibility checks still pass with the diffable table. - Added a New DM test: two toggles before Flutter rebuilds never list a person twice. It fails without the toggle fix. - Full mobile Flutter suite and analyzer pass in CI on the current head, which includes `main`'s fix for the right-drag link tests. - Simulator runs of the app (iPhone 17 Pro, iOS 27, signed in to a test community): opened New DM, backgrounded and foregrounded the app, searched, picked from the list and from a search, and removed a pick. Without this change, every avatar flashed to its initial after a foreground. With it, no avatar flashed at any point, and picks and removals animate between sections. Recordings show real community members, so they're shared in Buzz rather than here. #### Full-app comparison, `main` vs this PR The same scripted New DM sequence on a clone of the same signed-in simulator (iPhone 17 Pro, iOS 27), first on `main` (65e864301d) and then on this PR (330805cfc4): pick george, then glm; search "ju", pick judge, clear the search; remove george; background and reopen the app. The list shows only agents, no people. Both builds end in the same state: each pick moves into the selected section with its check mark, search narrows to one result and clearing it restores the full list, the removed pick returns to the list, and every avatar keeps rendering after the app reopens. Sampled at 6 frames per second around the reopen, neither build fell back to initials in this run. The visible difference is motion: on `main` a picked row jumps into the selected section, and here it animates there. Before (`main`): https://github.com/user-attachments/assets/12d88f47-0c10-4e78-a12c-18235e068f88 After (this PR): https://github.com/user-attachments/assets/b3d8edb1-94ad-44dd-b6b3-3db22d8311b0 ### Before merge - [x] Update onto `main` once main's link swipe tests are fixed (they were fixed by [#9255 Solo chat settings](https://github.com/block/buzz/pull/9255)), so Mobile Checks can pass. buzz-review-completed 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Tom Brow <tomb@squareup.com> Co-authored-by: Tom Brow <tomb@squareup.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: studio <dc2fc481343672df31a689840c09299972e704dac592e0da6f18486646a7ee3e@buzz.block.builderlab.xyz>
This commit is contained in:
Tom Brow
@@ -25,7 +25,7 @@ struct AddMembersIdentityTests {
|
||||
picker.loadViewIfNeeded()
|
||||
for index in keys.indices {
|
||||
let path = IndexPath(row: index, section: 1)
|
||||
let cell = picker.tableView(picker.tableView, cellForRowAt: path)
|
||||
let cell = picker.tableView.dataSource!.tableView(picker.tableView, cellForRowAt: path)
|
||||
let content = cell.contentConfiguration as? UIListContentConfiguration
|
||||
precondition(content?.text == labels[index], "Visible identity must retain its qualifier")
|
||||
precondition(cell.isAccessibilityElement, "The row owns accessibility")
|
||||
@@ -37,8 +37,16 @@ struct AddMembersIdentityTests {
|
||||
picker.tableView(picker.tableView, didSelectRowAt: path)
|
||||
}
|
||||
precondition(selected == keys, "Each distinct label must select its own pubkey")
|
||||
avatarReplies.removeAll()
|
||||
let renamed = [users[0].merging(["detail": "Renamed"]) { _, new in new }, users[1]]
|
||||
picker.update(["users": renamed, "selected": [], "query": "", "loading": false])
|
||||
let rebuilt = picker.tableView.dataSource!.tableView(picker.tableView, cellForRowAt: IndexPath(row: 0, section: 1))
|
||||
let rebuiltContent = rebuilt.contentConfiguration as? UIListContentConfiguration
|
||||
precondition(rebuiltContent?.secondaryText == "Renamed", "A changed row is redrawn")
|
||||
precondition(avatarReplies.isEmpty, "A loaded avatar is not fetched again on update")
|
||||
precondition(rebuiltContent?.image?.pngData() == UIImage(data: avatar!)?.pngData(), "A rebuilt row draws its loaded avatar at once")
|
||||
picker.update(["users": users, "selected": [users[1]], "query": "", "loading": false])
|
||||
let chosen = picker.tableView(picker.tableView, cellForRowAt: IndexPath(row: 0, section: 0))
|
||||
let chosen = picker.tableView.dataSource!.tableView(picker.tableView, cellForRowAt: IndexPath(row: 0, section: 0))
|
||||
precondition(chosen.accessibilityLabel == "\(labels[1]), Same owner", "Selected identity retains its qualifier")
|
||||
precondition(chosen.accessibilityTraits.contains(.selected), "Selection is exposed to VoiceOver")
|
||||
print("Native member picker identity and accessibility passed")
|
||||
|
||||
@@ -97,6 +97,15 @@ final class NativeAddMembersViewController: UITableViewController, UISearchResul
|
||||
private var state: [String: Any]
|
||||
private let send: (String, [String: Any]) -> Void
|
||||
private let loadAvatar: (String, String, @escaping (Data?) -> Void) -> Void
|
||||
/// Loaded avatars by key. Every state update rebuilds the cells, so a row
|
||||
/// draws its known avatar at once instead of blinking to its initial while
|
||||
/// the image comes back over the channel. Bounded like the Dart-side
|
||||
/// cache, so browsing a large community doesn't grow it without limit.
|
||||
private let avatars: NSCache<NSString, UIImage> = {
|
||||
let cache = NSCache<NSString, UIImage>()
|
||||
cache.countLimit = 64
|
||||
return cache
|
||||
}()
|
||||
private let search = UISearchController(searchResultsController: nil)
|
||||
private let pageColor: UIColor
|
||||
private let rowColor: UIColor
|
||||
@@ -160,6 +169,13 @@ final class NativeAddMembersViewController: UITableViewController, UISearchResul
|
||||
navigationItem.rightBarButtonItem?.accessibilityLabel = "Create message"
|
||||
}
|
||||
NavigationTopEdge.configure(tableView, item: navigationItem)
|
||||
tableView.register(UITableViewCell.self, forCellReuseIdentifier: Self.cellIdentifier)
|
||||
rows = UITableViewDiffableDataSource(tableView: tableView) { [weak self] tableView, indexPath, item in
|
||||
let cell = tableView.dequeueReusableCell(withIdentifier: Self.cellIdentifier, for: indexPath)
|
||||
self?.configure(cell, item: item)
|
||||
return cell
|
||||
}
|
||||
rows.defaultRowAnimation = .fade
|
||||
applyState()
|
||||
}
|
||||
|
||||
@@ -177,6 +193,8 @@ final class NativeAddMembersViewController: UITableViewController, UISearchResul
|
||||
func update(_ data: [String: Any]) {
|
||||
// A queued render from before the latest keystroke must not replace results.
|
||||
guard (data["query"] as? String ?? "") == (search.searchBar.text ?? "") else { return }
|
||||
// Flutter sends the whole state on every rebuild, often unchanged.
|
||||
guard !NSDictionary(dictionary: data).isEqual(to: state) else { return }
|
||||
state = data
|
||||
if isViewLoaded { applyState() }
|
||||
}
|
||||
@@ -191,7 +209,7 @@ final class NativeAddMembersViewController: UITableViewController, UISearchResul
|
||||
navigationItem.rightBarButtonItem?.title = busy ? "Adding…" : selected.isEmpty ? "Add" : "Add (\(selected.count))"
|
||||
}
|
||||
search.searchBar.isUserInteractionEnabled = !busy
|
||||
tableView.reloadData()
|
||||
applySnapshot()
|
||||
let error = state["error"] as? String ?? ""
|
||||
if error.isEmpty {
|
||||
tableView.tableFooterView = nil
|
||||
@@ -241,11 +259,56 @@ final class NativeAddMembersViewController: UITableViewController, UISearchResul
|
||||
return "No matching people or agents."
|
||||
}
|
||||
private var shownUsers: [[String: Any]] { atLimit ? [] : users }
|
||||
private func isStatus(_ indexPath: IndexPath) -> Bool {
|
||||
indexPath.section == 1 && indexPath.row >= shownUsers.count
|
||||
|
||||
/// A row: a member by pubkey, or the status row after the results. A
|
||||
/// member keeps its identity across sections, so a pick moves its row.
|
||||
private enum Item: Hashable {
|
||||
case member(String)
|
||||
case status
|
||||
}
|
||||
private static let cellIdentifier = "member"
|
||||
private var rows: UITableViewDiffableDataSource<Int, Item>!
|
||||
/// What each shown row was last drawn from, to redraw only changed rows.
|
||||
private var drawn: [Item: NSDictionary] = [:]
|
||||
|
||||
private func member(_ pubkey: String) -> [String: Any]? {
|
||||
(selected + shownUsers).first { $0["pubkey"] as? String == pubkey }
|
||||
}
|
||||
|
||||
private func drawing(_ item: Item) -> NSDictionary {
|
||||
switch item {
|
||||
case .member(let pubkey):
|
||||
return ["user": member(pubkey) ?? [:], "busy": busy, "selected": selected.contains { $0["pubkey"] as? String == pubkey }]
|
||||
case .status:
|
||||
return ["text": status ?? "", "retry": loadError && !atLimit, "busy": busy]
|
||||
}
|
||||
}
|
||||
|
||||
/// Applies the state as a diff: unchanged rows keep their cells, changed
|
||||
/// ones are redrawn in place, and a pick or removal animates its row
|
||||
/// between the chosen and result sections.
|
||||
private func applySnapshot() {
|
||||
let chosen = selected.compactMap { $0["pubkey"] as? String }
|
||||
var results: [Item] = shownUsers.compactMap { ($0["pubkey"] as? String).map(Item.member) }
|
||||
if status != nil { results.append(.status) }
|
||||
var snapshot = NSDiffableDataSourceSnapshot<Int, Item>()
|
||||
snapshot.appendSections([0, 1])
|
||||
snapshot.appendItems(chosen.map(Item.member), toSection: 0)
|
||||
snapshot.appendItems(results, toSection: 1)
|
||||
let previous = rows.snapshot()
|
||||
let wasChosen = previous.numberOfSections > 0 ? previous.itemIdentifiers(inSection: 0) : []
|
||||
let next = Dictionary(uniqueKeysWithValues: snapshot.itemIdentifiers.map { ($0, drawing($0)) })
|
||||
let changed = snapshot.itemIdentifiers.filter { item in
|
||||
drawn[item].map { $0 != next[item] } ?? false
|
||||
}
|
||||
let shown = Set(previous.itemIdentifiers)
|
||||
snapshot.reconfigureItems(changed.filter(shown.contains))
|
||||
drawn = next
|
||||
let animate = view.window != nil && wasChosen != chosen.map(Item.member)
|
||||
&& !UIAccessibility.isReduceMotionEnabled
|
||||
rows.apply(snapshot, animatingDifferences: animate)
|
||||
}
|
||||
|
||||
override func numberOfSections(in tableView: UITableView) -> Int { 2 }
|
||||
// No section titles, and no grouped-table header bands: a small gap
|
||||
// above the first shown card and between the two.
|
||||
override func tableView(_ tableView: UITableView, heightForHeaderInSection section: Int) -> CGFloat {
|
||||
@@ -258,77 +321,76 @@ final class NativeAddMembersViewController: UITableViewController, UISearchResul
|
||||
override func tableView(_ tableView: UITableView, viewForHeaderInSection section: Int) -> UIView? { nil }
|
||||
override func tableView(_ tableView: UITableView, viewForFooterInSection section: Int) -> UIView? { nil }
|
||||
private static let cardGap: CGFloat = 8
|
||||
override func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
|
||||
section == 0 ? selected.count : shownUsers.count + (status == nil ? 0 : 1)
|
||||
}
|
||||
override func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
|
||||
let cell = UITableViewCell(style: .subtitle, reuseIdentifier: nil)
|
||||
|
||||
private func configure(_ cell: UITableViewCell, item: Item) {
|
||||
cell.backgroundColor = rowColor
|
||||
var content = cell.defaultContentConfiguration()
|
||||
var avatarRequest: (String, String)?
|
||||
var memberLabel: String?
|
||||
if isStatus(indexPath) {
|
||||
cell.accessoryType = .none
|
||||
cell.selectionStyle = .default
|
||||
cell.accessibilityTraits = .none
|
||||
cell.isAccessibilityElement = false
|
||||
cell.accessibilityLabel = nil
|
||||
cell.accessibilityIdentifier = nil
|
||||
cell.contentView.accessibilityElementsHidden = false
|
||||
cell.isUserInteractionEnabled = !busy
|
||||
var content = UIListContentConfiguration.subtitleCell()
|
||||
guard case .member(let pubkey) = item else {
|
||||
content.text = status
|
||||
content.textProperties.color = loadError && !atLimit ? Self.errorColor : .secondaryLabel
|
||||
content.textProperties.numberOfLines = 0
|
||||
cell.selectionStyle = loadError && !atLimit ? .default : .none
|
||||
if loadError && !atLimit { cell.accessibilityTraits.insert(.button) }
|
||||
} else {
|
||||
let user = indexPath.section == 0 ? selected[indexPath.row] : shownUsers[indexPath.row]
|
||||
let unavailable = user["unavailable"] as? Bool == true
|
||||
content.text = user["name"] as? String
|
||||
content.secondaryText = user["detail"] as? String
|
||||
memberLabel = [content.text, content.secondaryText]
|
||||
.compactMap { $0 }.filter { !$0.isEmpty }.joined(separator: ", ")
|
||||
content.textProperties.numberOfLines = 0
|
||||
content.secondaryTextProperties.numberOfLines = 1
|
||||
content.image = fallbackAvatar(user)
|
||||
content.imageProperties.maximumSize = CGSize(width: 40, height: 40)
|
||||
content.imageProperties.reservedLayoutSize = CGSize(width: 40, height: 40)
|
||||
if let pubkey = user["pubkey"] as? String, let key = user["avatarKey"] as? String {
|
||||
avatarRequest = (pubkey, key)
|
||||
}
|
||||
cell.accessoryType = indexPath.section == 0 ? .checkmark : .none
|
||||
if unavailable {
|
||||
content.textProperties.color = .secondaryLabel
|
||||
cell.selectionStyle = .none
|
||||
cell.accessibilityTraits.insert(.notEnabled)
|
||||
}
|
||||
cell.accessibilityTraits.insert(.button)
|
||||
if indexPath.section == 0 { cell.accessibilityTraits.insert(.selected) }
|
||||
cell.contentConfiguration = content
|
||||
return
|
||||
}
|
||||
guard let user = member(pubkey) else { preconditionFailure("A shown member row must be in the state") }
|
||||
let chosen = selected.contains { $0["pubkey"] as? String == pubkey }
|
||||
let key = user["avatarKey"] as? String
|
||||
content.text = user["name"] as? String
|
||||
content.secondaryText = user["detail"] as? String
|
||||
content.textProperties.numberOfLines = 0
|
||||
content.secondaryTextProperties.numberOfLines = 1
|
||||
content.image = key.flatMap { avatars.object(forKey: $0 as NSString) } ?? fallbackAvatar(user)
|
||||
content.imageProperties.maximumSize = CGSize(width: 40, height: 40)
|
||||
content.imageProperties.reservedLayoutSize = CGSize(width: 40, height: 40)
|
||||
cell.accessoryType = chosen ? .checkmark : .none
|
||||
if user["unavailable"] as? Bool == true {
|
||||
content.textProperties.color = .secondaryLabel
|
||||
cell.selectionStyle = .none
|
||||
cell.accessibilityTraits.insert(.notEnabled)
|
||||
}
|
||||
cell.accessibilityTraits.insert(.button)
|
||||
if chosen { cell.accessibilityTraits.insert(.selected) }
|
||||
cell.contentConfiguration = content
|
||||
if let memberLabel {
|
||||
cell.isAccessibilityElement = true
|
||||
cell.accessibilityLabel = memberLabel
|
||||
cell.isAccessibilityElement = true
|
||||
cell.accessibilityLabel = [content.text, content.secondaryText]
|
||||
.compactMap { $0 }.filter { !$0.isEmpty }.joined(separator: ", ")
|
||||
cell.contentView.accessibilityElementsHidden = true
|
||||
guard let key else { return }
|
||||
cell.accessibilityIdentifier = key
|
||||
guard avatars.object(forKey: key as NSString) == nil else { return }
|
||||
loadAvatar(pubkey, key) { [weak self, weak cell] data in
|
||||
guard let data, let image = UIImage(data: data) else { return }
|
||||
let avatar = image.withRenderingMode(.alwaysOriginal)
|
||||
self?.avatars.setObject(avatar, forKey: key as NSString)
|
||||
guard let cell, cell.accessibilityIdentifier == key,
|
||||
var configuration = cell.contentConfiguration as? UIListContentConfiguration else { return }
|
||||
configuration.image = avatar
|
||||
let label = cell.accessibilityLabel
|
||||
cell.contentConfiguration = configuration
|
||||
cell.accessibilityLabel = label
|
||||
cell.contentView.accessibilityElementsHidden = true
|
||||
}
|
||||
if let (pubkey, key) = avatarRequest {
|
||||
cell.accessibilityIdentifier = key
|
||||
loadAvatar(pubkey, key) { [weak cell] data in
|
||||
guard let cell, cell.accessibilityIdentifier == key,
|
||||
let data, let image = UIImage(data: data),
|
||||
var configuration = cell.contentConfiguration as? UIListContentConfiguration else { return }
|
||||
configuration.image = image.withRenderingMode(.alwaysOriginal)
|
||||
let label = cell.accessibilityLabel
|
||||
cell.contentConfiguration = configuration
|
||||
cell.accessibilityLabel = label
|
||||
cell.contentView.accessibilityElementsHidden = true
|
||||
}
|
||||
}
|
||||
cell.isUserInteractionEnabled = !busy
|
||||
return cell
|
||||
}
|
||||
|
||||
override func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) {
|
||||
tableView.deselectRow(at: indexPath, animated: true)
|
||||
guard !busy else { return }
|
||||
if isStatus(indexPath) {
|
||||
guard !busy, let item = rows.itemIdentifier(for: indexPath) else { return }
|
||||
guard case .member(let pubkey) = item else {
|
||||
if loadError && !atLimit { send("retry", [:]) }
|
||||
return
|
||||
}
|
||||
let user = indexPath.section == 0 ? selected[indexPath.row] : shownUsers[indexPath.row]
|
||||
guard let user = member(pubkey) else { preconditionFailure("A shown member row must be in the state") }
|
||||
if user["unavailable"] as? Bool == true { return }
|
||||
guard let pubkey = user["pubkey"] as? String else { return }
|
||||
send("toggle", ["pubkey": pubkey])
|
||||
// Where asked (New DM), a pick ends its query, as an inserted mention
|
||||
// does, so the next name starts fresh.
|
||||
|
||||
@@ -161,7 +161,11 @@ class AddChannelMembersSheet extends HookConsumerWidget {
|
||||
void toggleUser(MentionCandidate user) {
|
||||
if (isSubmitting.value) return;
|
||||
final normalized = user.pubkey.toLowerCase();
|
||||
if (selectedPubkeys.contains(normalized)) {
|
||||
// The current selection, not this build's: a second tap can land
|
||||
// before the rebuild that reflects the first.
|
||||
if (selectedUsers.value.any(
|
||||
(selected) => selected.pubkey.toLowerCase() == normalized,
|
||||
)) {
|
||||
selectedUsers.value = [
|
||||
for (final selected in selectedUsers.value)
|
||||
if (selected.pubkey.toLowerCase() != normalized) selected,
|
||||
|
||||
@@ -466,7 +466,9 @@ class _NewDirectMessageSheet extends HookConsumerWidget {
|
||||
},
|
||||
onToggle: (pubkey) {
|
||||
if (isSubmitting.value) return;
|
||||
if (selectedPubkeys.contains(pubkey)) {
|
||||
// The current selection, not this build's: a second tap can land
|
||||
// before the rebuild that reflects the first.
|
||||
if (selectedUsers.value.any((user) => user.pubkey == pubkey)) {
|
||||
selectedUsers.value = selectedUsers.value
|
||||
.where((user) => user.pubkey != pubkey)
|
||||
.toList();
|
||||
|
||||
@@ -64,6 +64,22 @@ void registerNativeCreationTests(Widget Function(List<Override>) buildPage) {
|
||||
expect(found['detail'], isNull);
|
||||
await event('toggle', {'pubkey': _dmKey(2)});
|
||||
await event('query', {'value': 'Person'});
|
||||
// Two taps before Flutter rebuilds still never list a person twice.
|
||||
for (var tap = 0; tap < 2; tap++) {
|
||||
await messenger.handlePlatformMessage(
|
||||
bridge.name,
|
||||
const StandardMethodCodec().encodeMethodCall(
|
||||
MethodCall('toggle', {'session': session, 'pubkey': _dmKey(3)}),
|
||||
),
|
||||
(_) {},
|
||||
);
|
||||
}
|
||||
await tester.pumpAndSettle();
|
||||
final picked = [
|
||||
for (final user in states.last['selected'] as List)
|
||||
(user as Map)['pubkey'],
|
||||
];
|
||||
expect(picked.toSet(), hasLength(picked.length));
|
||||
for (final key in people.keys) {
|
||||
await event('toggle', {'pubkey': key});
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user