Skip to content

Commit 0efb9d6

Browse files
jasonlongjasonlong
committed
Stabilize selection after bulk triage actions
1 parent f77bcfd commit 0efb9d6

2 files changed

Lines changed: 194 additions & 10 deletions

File tree

‎Octodot/App/AppState.swift‎

Lines changed: 65 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -457,14 +457,20 @@ final class AppState {
457457

458458
func done() {
459459
if let batch = checkedNotificationsBatch() {
460+
let originalVisibleOrder = filteredNotifications
461+
let originalSelectionID = selectedNotificationID
460462
clearChecked()
461463
for notification in batch {
462464
if notification.source == .dependabotAlert {
463-
dismissSecurityAlert(notification)
465+
dismissSecurityAlert(notification, updatesSelection: false)
464466
} else {
465-
startThreadAction(.done, target: notification)
467+
startThreadAction(.done, target: notification, updatesSelection: false)
466468
}
467469
}
470+
restoreSelectionAfterBulkMutation(
471+
originalSelectionID: originalSelectionID,
472+
originalVisibleOrder: originalVisibleOrder
473+
)
468474
presentActionToast(verb: .done, items: batch)
469475
return
470476
}
@@ -482,15 +488,21 @@ final class AppState {
482488

483489
func unsubscribeFromThread() {
484490
if let batch = checkedNotificationsBatch() {
491+
let originalVisibleOrder = filteredNotifications
492+
let originalSelectionID = selectedNotificationID
485493
clearChecked()
486494
for notification in batch {
487495
if notification.source == .dependabotAlert {
488-
dismissSecurityAlert(notification)
496+
dismissSecurityAlert(notification, updatesSelection: false)
489497
} else {
490498
inboxStore.muteThread(notification.threadId)
491-
startThreadAction(.unsubscribe, target: notification)
499+
startThreadAction(.unsubscribe, target: notification, updatesSelection: false)
492500
}
493501
}
502+
restoreSelectionAfterBulkMutation(
503+
originalSelectionID: originalSelectionID,
504+
originalVisibleOrder: originalVisibleOrder
505+
)
494506
presentActionToast(verb: .unsub, items: batch)
495507
return
496508
}
@@ -866,7 +878,8 @@ final class AppState {
866878
private func startThreadAction(
867879
_ kind: ThreadActionStore.ActionKind,
868880
target explicitTarget: GitHubNotification? = nil,
869-
delayNanosecondsOverride: UInt64? = nil
881+
delayNanosecondsOverride: UInt64? = nil,
882+
updatesSelection: Bool = true
870883
) {
871884
guard let client = apiClient else { return }
872885
guard let target = explicitTarget ?? selectedNotification else { return }
@@ -893,7 +906,7 @@ final class AppState {
893906

894907
let visibleBeforeMutation = filteredNotifications
895908

896-
if kind.hidesNotification {
909+
if updatesSelection, kind.hidesNotification {
897910
let removeIndex = visibleBeforeMutation.firstIndex(where: { $0.id == target.id }) ?? selectedIndexStorage
898911
selectedThreadID = selectionAfterRemoving(threadId: target.id, from: visibleBeforeMutation)
899912
selectedIndexStorage = min(removeIndex, max(0, visibleBeforeMutation.count - 2))
@@ -922,16 +935,21 @@ final class AppState {
922935
return true
923936
}
924937

925-
private func dismissSecurityAlert(_ target: GitHubNotification) {
938+
private func dismissSecurityAlert(
939+
_ target: GitHubNotification,
940+
updatesSelection: Bool = true
941+
) {
926942
guard target.source == .dependabotAlert else { return }
927943

928944
let visibleBeforeMutation = filteredNotifications
929945
inboxStore.dismissSecurityAlert(target)
930946
errorMessage = nil
931947

932-
let removeIndex = visibleBeforeMutation.firstIndex(where: { $0.id == target.id }) ?? selectedIndexStorage
933-
selectedThreadID = selectionAfterRemoving(threadId: target.id, from: visibleBeforeMutation)
934-
selectedIndexStorage = min(removeIndex, max(0, visibleBeforeMutation.count - 2))
948+
if updatesSelection {
949+
let removeIndex = visibleBeforeMutation.firstIndex(where: { $0.id == target.id }) ?? selectedIndexStorage
950+
selectedThreadID = selectionAfterRemoving(threadId: target.id, from: visibleBeforeMutation)
951+
selectedIndexStorage = min(removeIndex, max(0, visibleBeforeMutation.count - 2))
952+
}
935953
clampSelection()
936954
}
937955

@@ -1264,6 +1282,43 @@ final class AppState {
12641282
return list[removeIndex - 1].id
12651283
}
12661284

1285+
private func restoreSelectionAfterBulkMutation(
1286+
originalSelectionID: String?,
1287+
originalVisibleOrder: [GitHubNotification]
1288+
) {
1289+
guard let originalSelectionID,
1290+
let originalIndex = originalVisibleOrder.firstIndex(where: { $0.id == originalSelectionID }) else {
1291+
clampSelection()
1292+
return
1293+
}
1294+
1295+
let currentNotifications = filteredNotifications
1296+
let currentIndexByID = Dictionary(
1297+
uniqueKeysWithValues: currentNotifications.enumerated().map { ($1.id, $0) }
1298+
)
1299+
1300+
if let currentIndex = currentIndexByID[originalSelectionID] {
1301+
applySelection(index: currentIndex, in: currentNotifications)
1302+
return
1303+
}
1304+
1305+
for notification in originalVisibleOrder.dropFirst(originalIndex + 1) {
1306+
if let currentIndex = currentIndexByID[notification.id] {
1307+
applySelection(index: currentIndex, in: currentNotifications)
1308+
return
1309+
}
1310+
}
1311+
1312+
for notification in originalVisibleOrder[..<originalIndex].reversed() {
1313+
if let currentIndex = currentIndexByID[notification.id] {
1314+
applySelection(index: currentIndex, in: currentNotifications)
1315+
return
1316+
}
1317+
}
1318+
1319+
clearSelection()
1320+
}
1321+
12671322
private func actionDelay(for kind: ThreadActionStore.ActionKind) -> UInt64 {
12681323
switch kind {
12691324
case .markRead, .done, .unsubscribe:

‎OctodotTests/AppStateTests.swift‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,25 @@ struct AppStateTests {
6969
)
7070
}
7171

72+
fileprivate static func makeAuthedState(
73+
notifications: [GitHubNotification],
74+
results: [Result<(Data, HTTPURLResponse), Error>] = []
75+
) -> (AppState, StubNetworkSession) {
76+
let defaults = makeIsolatedUserDefaults()
77+
let session = StubNetworkSession(results: results)
78+
let client = GitHubAPIClient(token: "ghp_secret", session: session, useGraphQLForSubjectMetadata: false)
79+
let state = AppState(
80+
notifications: notifications,
81+
authStatus: .signedIn(username: "octodot"),
82+
apiClient: client,
83+
actionDispatchDelayNanoseconds: 0,
84+
backgroundRefreshEnabled: false,
85+
sleepHandler: { _ in },
86+
userDefaults: defaults
87+
)
88+
return (state, session)
89+
}
90+
7291
fileprivate static func makeAuthedState(
7392
results: [Result<(Data, HTTPURLResponse), Error>] = [],
7493
count: Int = 5,
@@ -3207,6 +3226,116 @@ struct AppStateTests {
32073226
}
32083227
}
32093228

3229+
@Test func bulkDonePreservesASelectedRowThatSurvives() {
3230+
let notifications = (0..<4).map { Self.makeNotification(id: $0) }
3231+
let responses = (0..<2).map { id in
3232+
Result<(Data, HTTPURLResponse), Error>.success((
3233+
Data(),
3234+
Self.httpResponse(url: "https://api.github.com/notifications/threads/\(id)", statusCode: 204)
3235+
))
3236+
}
3237+
let (state, _) = Self.makeAuthedState(notifications: notifications, results: responses)
3238+
state.groupByRepo = false
3239+
state.selectNotification(id: "3")
3240+
state.toggleChecked(id: "0")
3241+
state.toggleChecked(id: "1")
3242+
3243+
state.done()
3244+
3245+
#expect(state.selectedNotificationID == "3")
3246+
#expect(state.selectedIndex == 1)
3247+
}
3248+
3249+
@Test func bulkDoneAdvancesPastAllRemovedRows() {
3250+
let notifications = (0..<5).map { Self.makeNotification(id: $0) }
3251+
let responses = [1, 2].map { id in
3252+
Result<(Data, HTTPURLResponse), Error>.success((
3253+
Data(),
3254+
Self.httpResponse(url: "https://api.github.com/notifications/threads/\(id)", statusCode: 204)
3255+
))
3256+
}
3257+
let (state, _) = Self.makeAuthedState(notifications: notifications, results: responses)
3258+
state.groupByRepo = false
3259+
state.selectNotification(id: "1")
3260+
state.toggleChecked(id: "1")
3261+
state.toggleChecked(id: "2")
3262+
3263+
state.done()
3264+
3265+
#expect(state.selectedNotificationID == "3")
3266+
}
3267+
3268+
@Test func bulkDoneFallsBackToNearestPrecedingSurvivorAtEnd() {
3269+
let notifications = (0..<4).map { Self.makeNotification(id: $0) }
3270+
let responses = [1, 3].map { id in
3271+
Result<(Data, HTTPURLResponse), Error>.success((
3272+
Data(),
3273+
Self.httpResponse(url: "https://api.github.com/notifications/threads/\(id)", statusCode: 204)
3274+
))
3275+
}
3276+
let (state, _) = Self.makeAuthedState(notifications: notifications, results: responses)
3277+
state.groupByRepo = false
3278+
state.selectNotification(id: "3")
3279+
state.toggleChecked(id: "1")
3280+
state.toggleChecked(id: "3")
3281+
3282+
state.done()
3283+
3284+
#expect(state.selectedNotificationID == "2")
3285+
}
3286+
3287+
@Test func bulkDoneClearsSelectionWhenAllRowsAreRemoved() {
3288+
let notifications = (0..<3).map { Self.makeNotification(id: $0) }
3289+
let responses = (0..<3).map { id in
3290+
Result<(Data, HTTPURLResponse), Error>.success((
3291+
Data(),
3292+
Self.httpResponse(url: "https://api.github.com/notifications/threads/\(id)", statusCode: 204)
3293+
))
3294+
}
3295+
let (state, _) = Self.makeAuthedState(notifications: notifications, results: responses)
3296+
state.groupByRepo = false
3297+
state.selectNotification(id: "1")
3298+
for notification in notifications {
3299+
state.toggleChecked(id: notification.id)
3300+
}
3301+
3302+
state.done()
3303+
3304+
#expect(state.selectedNotificationID == nil)
3305+
#expect(state.selectedIndex == 0)
3306+
}
3307+
3308+
@Test func groupedBulkUnsubscribeAdvancesAcrossRepositoryBoundary() {
3309+
let notifications = [
3310+
Self.makeNotification(id: 0, repo: "acme/alpha"),
3311+
Self.makeNotification(id: 1, repo: "acme/beta"),
3312+
Self.makeNotification(id: 2, repo: "acme/alpha"),
3313+
Self.makeNotification(id: 3, repo: "acme/beta"),
3314+
]
3315+
let responses = [2, 3].flatMap { id in
3316+
[
3317+
Result<(Data, HTTPURLResponse), Error>.success((
3318+
Data(),
3319+
Self.httpResponse(url: "https://api.github.com/notifications/threads/\(id)/subscription", statusCode: 204)
3320+
)),
3321+
Result<(Data, HTTPURLResponse), Error>.success((
3322+
Data(),
3323+
Self.httpResponse(url: "https://api.github.com/notifications/threads/\(id)", statusCode: 204)
3324+
)),
3325+
]
3326+
}
3327+
let (state, _) = Self.makeAuthedState(notifications: notifications, results: responses)
3328+
state.groupByRepo = true
3329+
state.selectNotification(id: "2")
3330+
state.toggleChecked(id: "2")
3331+
state.toggleChecked(id: "3")
3332+
3333+
state.unsubscribeFromThread()
3334+
3335+
#expect(state.selectedNotificationID == "1")
3336+
#expect(state.selectedNotification?.repository == "acme/beta")
3337+
}
3338+
32103339
@Test func bulkUnsubscribeRunsOnAllCheckedRows() async {
32113340
let (state, session) = Self.makeAuthedState(
32123341
results: [

0 commit comments

Comments
 (0)