Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PM-16847: Update inline loading indicators #1258

Merged
merged 3 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class MockSendRepository: SendRepository {

var fetchSyncCalled = false
var fetchSyncForceSync: Bool?
var fetchSyncHandler: (() -> Void)?
var fetchSyncResult: Result<Void, Error> = .success(())

var searchSendSearchText: String?
Expand Down Expand Up @@ -84,6 +85,7 @@ class MockSendRepository: SendRepository {
func fetchSync(forceSync: Bool) async throws {
fetchSyncCalled = true
fetchSyncForceSync = forceSync
fetchSyncHandler?()
try fetchSyncResult.get()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ struct LoadingView<T: Equatable & Sendable, Contents: View>: View {
var body: some View {
switch state {
case .loading:
ProgressView()
CircularActivityIndicator()
.frame(maxWidth: .infinity, maxHeight: .infinity)
case let .data(data):
contents(data)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@ struct CircularActivityIndicator: View {
.trim(from: 0, to: 0.65)
.stroke(Asset.Colors.strokeBorder.swiftUIColor, style: strokeStyle)
.rotationEffect(Angle(degrees: isSpinning ? 360 : 0))
.animation(.linear(duration: 1).repeatForever(autoreverses: false), value: isSpinning)
.onAppear {
isSpinning = true
withAnimation(.linear(duration: 1).repeatForever(autoreverses: false)) {
isSpinning = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐Ÿค” For my own edification, what's the salient difference between these two?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the previous implicit animation, when the vault list would load I think the search bar ends up moving the spinner as it appears and that was getting picked up in the animation. So the spinner would not only rotate, but it would also move vertically ๐Ÿ™ƒ. Switching to using withAnimation fixed that.

loading.animation.mp4

}
}
}
.frame(width: 56, height: 56)
Expand Down
1 change: 1 addition & 0 deletions BitwardenShared/UI/Tools/Send/Send/SendCoordinator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ final class SendCoordinator: Coordinator, HasStackNavigator {
& HasPasteboardService
& HasPolicyService
& HasSendRepository
& HasVaultRepository

// MARK: - Private Properties

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,17 @@ import Foundation

extension SendListState {
static var empty: SendListState {
SendListState(loadingState: .data([]))
}

static var loading: SendListState {
SendListState()
}

static var content: SendListState {
let date = Date(year: 2023, month: 11, day: 5, hour: 9, minute: 41)
return SendListState(
sections: [
loadingState: .data([
SendListSection(
id: "1",
isCountDisplayed: false,
Expand Down Expand Up @@ -96,14 +100,14 @@ extension SendListState {
],
name: "All sends"
),
]
])
)
}

static var contentTextType: SendListState {
let date = Date(year: 2023, month: 11, day: 5, hour: 9, minute: 41)
return SendListState(
sections: [
loadingState: .data([
SendListSection(
id: "text",
isCountDisplayed: false,
Expand Down Expand Up @@ -174,7 +178,7 @@ extension SendListState {
],
name: nil
),
],
]),
type: .text
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ final class SendListProcessor: StateProcessor<SendListState, SendListAction, Sen
& HasPasteboardService
& HasPolicyService
& HasSendRepository
& HasVaultRepository

// MARK: Private properties

Expand Down Expand Up @@ -174,21 +175,34 @@ final class SendListProcessor: StateProcessor<SendListState, SendListAction, Sen
if let type = state.type {
for try await sends in try await services.sendRepository.sendTypeListPublisher(type: type) {
if sends.isEmpty {
state.sections = []
state.loadingState = .data([])
} else {
state.sections = [
state.loadingState = .data([
SendListSection(
id: type.localizedName,
isCountDisplayed: false,
items: sends,
name: nil
),
]
])
}
}
} else {
for try await sections in try await services.sendRepository.sendListPublisher() {
state.sections = sections
let needsSync = try await services.vaultRepository.needsSync()

if needsSync, sections.isEmpty {
// If a sync is needed and there's no sends in the user's vault, it could
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โ›๏ธ Technically, it would be "there are no sends"

// mean the initial sync hasn't completed so sync first.
do {
try await services.sendRepository.fetchSync(forceSync: false)
state.loadingState = .data(sections)
} catch {
services.errorReporter.log(error: error)
}
} else {
state.loadingState = .data(sections)
}
}
}
} catch {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ class SendListProcessorTests: BitwardenTestCase { // swiftlint:disable:this type
var policyService: MockPolicyService!
var sendRepository: MockSendRepository!
var subject: SendListProcessor!
var vaultRepository: MockVaultRepository!

override func setUp() {
super.setUp()
Expand All @@ -25,14 +26,16 @@ class SendListProcessorTests: BitwardenTestCase { // swiftlint:disable:this type
pasteboardService = MockPasteboardService()
policyService = MockPolicyService()
sendRepository = MockSendRepository()
vaultRepository = MockVaultRepository()

subject = SendListProcessor(
coordinator: coordinator.asAnyCoordinator(),
services: ServiceContainer.withMocks(
errorReporter: errorReporter,
pasteboardService: pasteboardService,
policyService: policyService,
sendRepository: sendRepository
sendRepository: sendRepository,
vaultRepository: vaultRepository
),
state: SendListState()
)
Expand All @@ -46,6 +49,7 @@ class SendListProcessorTests: BitwardenTestCase { // swiftlint:disable:this type
policyService = nil
sendRepository = nil
subject = nil
vaultRepository = nil
}

// MARK: Tests
Expand Down Expand Up @@ -276,7 +280,7 @@ class SendListProcessorTests: BitwardenTestCase { // swiftlint:disable:this type

/// `perform(_:)` with `.streamSendList` updates the state's send list whenever it changes.
@MainActor
func test_perform_streamSendList_nilType() {
func test_perform_streamSendList_nilType() throws {
let sendListItem = SendListItem(id: "1", itemType: .group(.file, 42))
sendRepository.sendListSubject.send([
SendListSection(id: "1", isCountDisplayed: true, items: [sendListItem], name: "Name"),
Expand All @@ -286,11 +290,12 @@ class SendListProcessorTests: BitwardenTestCase { // swiftlint:disable:this type
await subject.perform(.streamSendList)
}

waitFor(!subject.state.sections.isEmpty)
waitFor(subject.state.loadingState.data != nil)
task.cancel()

XCTAssertEqual(subject.state.sections.count, 1)
XCTAssertEqual(subject.state.sections[0].items, [sendListItem])
let sections = try XCTUnwrap(subject.state.loadingState.data)
XCTAssertEqual(sections.count, 1)
XCTAssertEqual(sections[0].items, [sendListItem])
}

/// `perform(_:)` with `.streamSendList` records any errors.
Expand All @@ -307,9 +312,65 @@ class SendListProcessorTests: BitwardenTestCase { // swiftlint:disable:this type
XCTAssertEqual(errorReporter.errors.last as? BitwardenTestError, .example)
}

/// `perform(_:)` with `.streamSendList` updates the state's send list whenever it changes,
/// syncing first if a sync is needed and the vault is empty.
@MainActor
func test_perform_streamSendList_nilType_needsSync() throws {
let sendListItem = SendListItem(id: "1", itemType: .group(.file, 42))
sendRepository.fetchSyncHandler = { [weak self] in
guard let self else { return }
// Update `sendListSubject` after `fetchSync` is called to simulate an initially empty
// vault, syncing, and then sends in the list.
sendRepository.sendListSubject.send([
SendListSection(id: "1", isCountDisplayed: true, items: [sendListItem], name: "Name"),
])
}
vaultRepository.needsSyncResult = .success(true)

let task = Task {
await subject.perform(.streamSendList)
}

waitFor(subject.state.loadingState.data?.count == 1)
task.cancel()

let sections = try XCTUnwrap(subject.state.loadingState.data)
XCTAssertEqual(sections.count, 1)
XCTAssertEqual(sections[0].items, [sendListItem])
XCTAssertTrue(vaultRepository.needsSyncCalled)
}

/// `perform(_:)` with `.streamSendList` logs an error if sync is needed but it fails, but still
/// receives any updates from the publisher if the send list changes.
@MainActor
func test_perform_streamSendList_nilType_needsSync_error() throws {
sendRepository.fetchSyncResult = .failure(BitwardenTestError.example)
vaultRepository.needsSyncResult = .success(true)

let task = Task {
await subject.perform(.streamSendList)
}

waitFor(!errorReporter.errors.isEmpty)

let sendListItem = SendListItem(id: "1", itemType: .group(.file, 42))
sendRepository.sendListSubject.send([
SendListSection(id: "1", isCountDisplayed: true, items: [sendListItem], name: "Name"),
])

waitFor(subject.state.loadingState.data?.count == 1)
task.cancel()

XCTAssertEqual(errorReporter.errors as? [BitwardenTestError], [.example])
let sections = try XCTUnwrap(subject.state.loadingState.data)
XCTAssertEqual(sections.count, 1)
XCTAssertEqual(sections[0].items, [sendListItem])
XCTAssertTrue(vaultRepository.needsSyncCalled)
}

/// `perform(_:)` with `.streamSendList` updates the state's send list whenever it changes.
@MainActor
func test_perform_streamSendList_textType() {
func test_perform_streamSendList_textType() throws {
let sendListItem = SendListItem.fixture()
sendRepository.sendTypeListSubject.send([
sendListItem,
Expand All @@ -321,12 +382,29 @@ class SendListProcessorTests: BitwardenTestCase { // swiftlint:disable:this type
await subject.perform(.streamSendList)
}

waitFor(!subject.state.sections.isEmpty)
waitFor(subject.state.loadingState.data != nil)
task.cancel()

let sections = try XCTUnwrap(subject.state.loadingState.data)
XCTAssertEqual(sendRepository.sendTypeListPublisherType, .text)
XCTAssertEqual(subject.state.sections.count, 1)
XCTAssertEqual(subject.state.sections[0].items, [sendListItem])
XCTAssertEqual(sections.count, 1)
XCTAssertEqual(sections[0].items, [sendListItem])
}

/// `perform(_:)` with `.streamSendList` updates the state's send list whenever it changes.
@MainActor
func test_perform_streamSendList_textType_empty() throws {
subject.state.type = .text

let task = Task {
await subject.perform(.streamSendList)
}

waitFor(subject.state.loadingState.data != nil)
task.cancel()

let sections = try XCTUnwrap(subject.state.loadingState.data)
XCTAssertTrue(sections.isEmpty)
}

/// `receive(_:)` with `.addItemPressed` navigates to the `.addItem` route.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ struct SendListState: Sendable {
/// A flag indicating if the info button should be hidden.
var isInfoButtonHidden: Bool { type != nil }

/// The loading state of the send list screen.
var loadingState: LoadingState<[SendListSection]> = .loading(nil)

/// The navigation title for this screen.
var navigationTitle: String { type?.localizedName ?? Localizations.sends }

Expand All @@ -26,9 +29,6 @@ struct SendListState: Sendable {
/// An array of results matching the ``searchText``.
var searchResults: [SendListItem] = []

/// The sections displayed in the send list.
var sections: [SendListSection] = []

/// A toast message to show in the view.
var toast: Toast?

Expand Down
Loading
Loading