Extract TranscriptionHistoryViewModel; remove dead UsageDashboardView (A2)#21
Merged
Conversation
After the dashboard redesign, DashboardView's Overview tab switched to DashboardHomeView and UsageDashboardView was left referenced by nothing but its own test. Deleting the dead view (and its test) also removes four direct store-singleton accesses. (grade-report A2) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
TranscriptionHistoryView called DataManager.shared directly for record fetching, pagination, and deletion. That logic and state now live in a @mainactor @observable TranscriptionHistoryViewModel, constructor- injected with a DataManagerProtocol (default DataManager.shared) so it is unit-testable with MockDataManager. The view keeps only UI state and no longer touches the store. Adds 10 ViewModel tests. (grade-report A2) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR extracts pagination and record-management logic from ChangesTranscription History View Model Extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Views/Transcription/TranscriptionHistoryView.swift (1)
22-97: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winSplit
bodyinto smaller view-builder units to satisfy function-length rule.
bodyis still substantially over the 40-line limit, which makes this file harder to maintain despite the VM extraction.As per coding guidelines,
Keep functions to 40 lines or fewer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Views/Transcription/TranscriptionHistoryView.swift` around lines 22 - 97, The body of TranscriptionHistoryView is overlong; split it into smaller view-builder units by extracting logical sections into private computed properties or funcs (e.g., makeHeader(), makeSearchBar(), makeContentView(), makeAlertsAndModifiers()) and call those from body to keep each function under 40 lines. Move the TranscriptionHistoryHeader, TranscriptionSearchBar, the conditional records/list block, and the alert/modifier chains into their respective helpers (preserve bindings like $searchText, $isSearchFocused, expandedRecords and callbacks such as toggleExpansion(for:), confirmDelete, copyToClipboard, handleEscapeKey, handleCommandF) and ensure .task and .onChange remain applied to the main container or appropriate helper so behavior is unchanged. Ensure helpers return some View and are marked private to keep class scope clean.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/ViewModels/TranscriptionHistoryViewModel.swift`:
- Around line 42-43: The guard that returns when isLoading is true discards
incoming reset requests; instead add a queued-reset mechanism on
TranscriptionHistoryViewModel: introduce a property like pendingReset (and
optionally pendingQuery or pendingParameters) and, in the method that currently
contains "guard !isLoading else { return }", if isLoading is true set
pendingReset = true (and store the latest query params) and return without
dropping the request; after the current fetch completes (where you set isLoading
= false) check pendingReset and if set clear it and replay the load call with
reset: true (using the stored params if applicable) so the latest reset is
executed when the in-flight load finishes.
---
Outside diff comments:
In `@Sources/Views/Transcription/TranscriptionHistoryView.swift`:
- Around line 22-97: The body of TranscriptionHistoryView is overlong; split it
into smaller view-builder units by extracting logical sections into private
computed properties or funcs (e.g., makeHeader(), makeSearchBar(),
makeContentView(), makeAlertsAndModifiers()) and call those from body to keep
each function under 40 lines. Move the TranscriptionHistoryHeader,
TranscriptionSearchBar, the conditional records/list block, and the
alert/modifier chains into their respective helpers (preserve bindings like
$searchText, $isSearchFocused, expandedRecords and callbacks such as
toggleExpansion(for:), confirmDelete, copyToClipboard, handleEscapeKey,
handleCommandF) and ensure .task and .onChange remain applied to the main
container or appropriate helper so behavior is unchanged. Ensure helpers return
some View and are marked private to keep class scope clean.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a2e447d-0a74-4ca9-a67b-82ec768b78d4
📒 Files selected for processing (5)
Sources/ViewModels/TranscriptionHistoryViewModel.swiftSources/Views/Dashboard/UsageDashboardView.swiftSources/Views/Transcription/TranscriptionHistoryView.swiftTests/ViewModels/TranscriptionHistoryViewModelTests.swiftTests/Views/Dashboard/UsageDashboardViewTests.swift
💤 Files with no reviewable changes (2)
- Tests/Views/Dashboard/UsageDashboardViewTests.swift
- Sources/Views/Dashboard/UsageDashboardView.swift
Comment on lines
+42
to
+43
| guard !isLoading else { return } | ||
| isLoading = true |
There was a problem hiding this comment.
Reset search requests can be dropped while loading, causing stale results.
At Line 42, guard !isLoading else { return } discards any reset: true call fired during an in-flight fetch. With rapid search updates, the latest query may never load.
💡 Suggested fix (queue latest reset and replay after current load)
final class TranscriptionHistoryViewModel {
+ private var pendingResetSearch: String?
@@
func loadRecords(reset: Bool = false, search: String = "") async {
- guard !isLoading else { return }
+ if isLoading {
+ if reset { pendingResetSearch = search }
+ return
+ }
isLoading = true
defer {
isLoading = false
hasLoadedOnce = true
}
@@
do {
@@
} catch {
errorMessage = "Failed to load transcription history: \(error.localizedDescription)"
showError = true
hasMore = false
}
+
+ if let queuedSearch = pendingResetSearch {
+ pendingResetSearch = nil
+ Task { [weak self] in
+ await self?.loadRecords(reset: true, search: queuedSearch)
+ }
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/ViewModels/TranscriptionHistoryViewModel.swift` around lines 42 - 43,
The guard that returns when isLoading is true discards incoming reset requests;
instead add a queued-reset mechanism on TranscriptionHistoryViewModel: introduce
a property like pendingReset (and optionally pendingQuery or pendingParameters)
and, in the method that currently contains "guard !isLoading else { return }",
if isLoading is true set pendingReset = true (and store the latest query params)
and return without dropping the request; after the current fetch completes
(where you set isLoading = false) check pendingReset and if set clear it and
replay the load call with reset: true (using the stored params if applicable) so
the latest reset is executed when the in-flight load finishes.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Grade-report item A2 — views bypassing the ViewModel layer to call store singletons directly. Done as its own focused PR.
UsageDashboardView. After the dashboard redesign,DashboardView's Overview tab switched toDashboardHomeView;UsageDashboardViewwas left referenced by nothing but its own test. Deleting the dead view + test also eliminates four direct store-singleton accesses.TranscriptionHistoryViewModel.TranscriptionHistoryViewcalledDataManager.shareddirectly for fetch / pagination / delete. That logic and state now live in a@MainActor @ObservableViewModel, constructor-injected with aDataManagerProtocol(defaultDataManager.shared) — mirroring the existingRecordingViewModel/DashboardHomeViewinjection pattern. The view keeps only UI state and no longer touches the store. Adds 10 ViewModel tests usingMockDataManager.The remaining
.sharedreferences the audit noted (inDashboardView/DashboardHomeView/DashboardCategoriesView) are already injectable-default seams (x ?? .shared), not true layering violations — left as-is.Test plan
swift build+swift build --build-testscleanswiftlint --strictcleanUsageDashboardViewtests were removed)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Refactor
Removals