refactor: flatten Domain, move Entity logic to Impl#176
Conversation
Remove intermediate Dependencies/ directory. Protocol files are now
directly under Sources/Domain/{DataSource,Handler,UseCase,...}.
Part of #175
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- NowPlaying.elapsed → PlaybackUseCaseImpl.elapsedTime(for:) - ConfigWriteError: remove unused LocalizedError/errorDescription - Remove unnecessary variable aliases in TrackHandlerImpl - Update CLAUDE.md for flattened Domain structure Part of #175 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Protocol definitions + fatalError stubs are not testable code. Part of #175 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRefactors elapsed-time computation from the Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as TrackHandler / Interactor
participant UseCase as PlaybackUseCase
participant Entity as NowPlaying
participant Clock as SystemClock
Handler->>UseCase: fetchNowPlaying()
UseCase-->>Handler: NowPlaying (rawElapsed, timestamp, playbackRate)
Handler->>UseCase: elapsedTime(for: NowPlaying)
Note right of UseCase: compute using rawElapsed + playbackRate × (Clock.now - timestamp)
UseCase->>Clock: Date()
UseCase-->>Handler: TimeInterval?
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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)
Comment |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (1)
Tests/TrackHandlerTests/TrackHandlerImplTests.swift (1)
168-168: Keep stub elapsed behavior closer to production to avoid timing blind spots.Line 168 always returns
rawElapsed, so tests won’t exercise timestamp/playbackRate interpolation paths. Consider matchingPlaybackUseCaseImpl.elapsedTime(for:)(or documenting this as intentionally simplified) for better timing-path confidence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/TrackHandlerTests/TrackHandlerImplTests.swift` at line 168, The test stub elapsedTime(for:) in TrackHandlerImplTests always returns np.rawElapsed which bypasses interpolation logic; update the stubbed function elapsedTime(for:) to mirror production behavior from PlaybackUseCaseImpl.elapsedTime(for:) by computing elapsed using np.timestamp and np.playbackRate (and falling back to rawElapsed when appropriate) so tests exercise timestamp/playbackRate interpolation paths, or explicitly document why the simplified return is intentional if you choose not to implement it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TrackHandler/TrackHandlerImpl.swift`:
- Around line 69-74: Compute elapsedTime once and reuse it for both the
elapsedTime field and currentLyric computation to avoid small time-based
divergence: call playbackUseCase.elapsedTime(for: nowPlaying) a single time
(store in a local variable, e.g. elapsed) before building the returned object,
then use that variable for the elapsedTime property and inside the currentLyric
closure when evaluating timedLines.last { $0.time <= t }?.text; update
references to playbackUseCase.elapsedTime(for: nowPlaying) in TrackHandlerImpl
(the construction that uses elapsedTime, currentLyric, timedLines,
result.plainLyrics, nowPlaying) to use the stored value.
---
Nitpick comments:
In `@Tests/TrackHandlerTests/TrackHandlerImplTests.swift`:
- Line 168: The test stub elapsedTime(for:) in TrackHandlerImplTests always
returns np.rawElapsed which bypasses interpolation logic; update the stubbed
function elapsedTime(for:) to mirror production behavior from
PlaybackUseCaseImpl.elapsedTime(for:) by computing elapsed using np.timestamp
and np.playbackRate (and falling back to rawElapsed when appropriate) so tests
exercise timestamp/playbackRate interpolation paths, or explicitly document why
the simplified return is intentional if you choose not to implement it.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42e49564-bf67-49f7-a45a-f18874542ff5
📒 Files selected for processing (43)
.claude/CLAUDE.mdSources/Domain/DataSource/ConfigDataSource.swiftSources/Domain/DataSource/LyricsDataSource.swiftSources/Domain/DataSource/MediaRemoteDataSource.swiftSources/Domain/DataSource/MetadataDataSource.swiftSources/Domain/DataSource/WallpaperDataSource.swiftSources/Domain/DataStore/LyricsDataStore.swiftSources/Domain/DataStore/MetadataDataStore.swiftSources/Domain/DataStore/WallpaperCacheStore.swiftSources/Domain/Handler/ConfigHandler.swiftSources/Domain/Handler/HealthHandler.swiftSources/Domain/Handler/ProcessHandler.swiftSources/Domain/Handler/ProcessLockable.swiftSources/Domain/Handler/ProcessManaging.swiftSources/Domain/Handler/ServiceHandler.swiftSources/Domain/Handler/TrackHandler.swiftSources/Domain/Handler/VersionHandler.swiftSources/Domain/Interactor/ScreenInteractor.swiftSources/Domain/Interactor/TrackInteractor.swiftSources/Domain/Interactor/WallpaperInteractor.swiftSources/Domain/Misc/FontMetricsProvider.swiftSources/Domain/Misc/HealthCheckable.swiftSources/Domain/Misc/StandardOutput.swiftSources/Domain/Misc/UserNotifier.swiftSources/Domain/Repository/ConfigRepository.swiftSources/Domain/Repository/LyricsRepository.swiftSources/Domain/Repository/MetadataRepository.swiftSources/Domain/Repository/NowPlayingRepository.swiftSources/Domain/Repository/WallpaperRepository.swiftSources/Domain/UseCase/ConfigUseCase.swiftSources/Domain/UseCase/LyricsUseCase.swiftSources/Domain/UseCase/MetadataUseCase.swiftSources/Domain/UseCase/PlaybackUseCase.swiftSources/Domain/UseCase/WallpaperUseCase.swiftSources/Entity/Config/ConfigWriteError.swiftSources/Entity/NowPlaying.swiftSources/PlaybackUseCase/PlaybackUseCaseImpl.swiftSources/TrackHandler/TrackHandlerImpl.swiftSources/TrackInteractor/TrackInteractorImpl.swiftSources/VersionHandler/Resources/version.txtTests/TrackHandlerTests/TrackHandlerImplTests.swiftTests/TrackInteractorTests/TrackInteractorRaceTests.swiftcodecov.yml
💤 Files with no reviewable changes (1)
- Sources/Entity/NowPlaying.swift
There was a problem hiding this comment.
Pull request overview
Domain モジュールのディレクトリ構造をフラット化しつつ、Entity に残っていたロジック（NowPlaying.elapsed など）を UseCase 実装側へ移して「Entity は純データ」のルールに寄せる PR です。
Changes:
- Domain の
Dependencies/配下を廃止し、各レイヤ（UseCase/Repository/DataSource…）をSources/Domain/直下のサブディレクトリに再配置 NowPlaying.elapsedの計算をPlaybackUseCase.elapsedTime(for:)とPlaybackUseCaseImplに移植し、呼び出し側を更新ConfigWriteErrorのLocalizedError準拠削除、および codecov の ignore 設定更新
Reviewed changes
Copilot reviewed 11 out of 43 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/TrackInteractorTests/TrackInteractorRaceTests.swift | PlaybackUseCase 追加APIに合わせてスタブを更新 |
| Tests/TrackHandlerTests/TrackHandlerImplTests.swift | PlaybackUseCase 追加APIに合わせてスタブを更新 |
| Sources/VersionHandler/Resources/version.txt | バージョンを 2.5.21 に更新 |
| Sources/TrackInteractor/TrackInteractorImpl.swift | NowPlaying.elapsed 参照を playbackUseCase.elapsedTime(for:) に置換 |
| Sources/TrackHandler/TrackHandlerImpl.swift | NowPlaying.elapsed 参照を playbackUseCase.elapsedTime(for:) に置換、不要な別名を整理 |
| Sources/PlaybackUseCase/PlaybackUseCaseImpl.swift | 経過時間算出ロジックを UseCase 実装へ移植 |
| Sources/Entity/NowPlaying.swift | elapsed computed property を削除（純データ化） |
| Sources/Entity/Config/ConfigWriteError.swift | LocalizedError 準拠を削除し Error に変更、Sendable 追加 |
| Sources/Domain/UseCase/WallpaperUseCase.swift | Domain 直下へ移動した UseCase 定義（新規追加） |
| Sources/Domain/UseCase/PlaybackUseCase.swift | elapsedTime(for:) を protocol に追加 |
| Sources/Domain/UseCase/MetadataUseCase.swift | Domain 直下へ移動した UseCase 定義（新規追加） |
| Sources/Domain/UseCase/LyricsUseCase.swift | Domain 直下へ移動した UseCase 定義（新規追加） |
| Sources/Domain/UseCase/ConfigUseCase.swift | Domain 直下へ移動した UseCase 定義（新規追加） |
| Sources/Domain/Repository/WallpaperRepository.swift | Domain 直下へ移動した Repository 定義（新規追加） |
| Sources/Domain/Repository/NowPlayingRepository.swift | Domain 直下へ移動した Repository 定義（新規追加） |
| Sources/Domain/Repository/MetadataRepository.swift | Domain 直下へ移動した Repository 定義（新規追加） |
| Sources/Domain/Repository/LyricsRepository.swift | Domain 直下へ移動した Repository 定義（新規追加） |
| Sources/Domain/Repository/ConfigRepository.swift | Domain 直下へ移動した Repository 定義（新規追加） |
| Sources/Domain/Misc/UserNotifier.swift | ユーザー通知の依存定義＋実装を Domain 側に追加 |
| Sources/Domain/Misc/StandardOutput.swift | CLI 出力の依存定義を Domain 側に追加 |
| Sources/Domain/Misc/HealthCheckable.swift | HealthCheck 依存定義を Domain 側に追加 |
| Sources/Domain/Misc/FontMetricsProvider.swift | FontMetrics 依存定義を Domain 側に追加 |
| Sources/Domain/Interactor/WallpaperInteractor.swift | Domain 直下へ移動した Interactor 定義（新規追加） |
| Sources/Domain/Interactor/TrackInteractor.swift | Domain 直下へ移動した Interactor 定義（新規追加） |
| Sources/Domain/Interactor/ScreenInteractor.swift | Domain 直下へ移動した Interactor 定義（新規追加） |
| Sources/Domain/Handler/VersionHandler.swift | Domain 直下へ移動した Handler 定義（新規追加） |
| Sources/Domain/Handler/TrackHandler.swift | Domain 直下へ移動した Handler 定義（新規追加） |
| Sources/Domain/Handler/ServiceHandler.swift | Domain 直下へ移動した Handler 定義（新規追加） |
| Sources/Domain/Handler/ProcessManaging.swift | Domain 直下へ移動した Handler 定義（新規追加） |
| Sources/Domain/Handler/ProcessLockable.swift | Domain 直下へ移動した Handler 定義（新規追加） |
| Sources/Domain/Handler/ProcessHandler.swift | Domain 直下へ移動した Handler 定義（新規追加） |
| Sources/Domain/Handler/HealthHandler.swift | Domain 直下へ移動した Handler 定義（新規追加） |
| Sources/Domain/Handler/ConfigHandler.swift | Domain 直下へ移動した Handler 定義（新規追加） |
| Sources/Domain/DataStore/WallpaperCacheStore.swift | Domain 直下へ移動した DataStore 定義（新規追加） |
| Sources/Domain/DataStore/MetadataDataStore.swift | Domain 直下へ移動した DataStore 定義（新規追加） |
| Sources/Domain/DataStore/LyricsDataStore.swift | Domain 直下へ移動した DataStore 定義（新規追加） |
| Sources/Domain/DataSource/WallpaperDataSource.swift | Domain 直下へ移動した DataSource 定義（新規追加） |
| Sources/Domain/DataSource/MetadataDataSource.swift | Domain 直下へ移動した DataSource 定義（新規追加） |
| Sources/Domain/DataSource/MediaRemoteDataSource.swift | Domain 直下へ移動した DataSource 定義（新規追加） |
| Sources/Domain/DataSource/LyricsDataSource.swift | Domain 直下へ移動した DataSource 定義（新規追加） |
| Sources/Domain/DataSource/ConfigDataSource.swift | Domain 直下へ移動した DataSource 定義（新規追加） |
| codecov.yml | Sources/Domain/** を ignore に追加 |
| .claude/CLAUDE.md | Domain 構成の説明を更新 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public enum ConfigWriteError: Error { | ||
| case alreadyExists(path: String) | ||
| case encodingFailed | ||
|
|
||
| public var errorDescription: String? { | ||
| switch self { | ||
| case .alreadyExists(let path): "Config file already exists at \(path). Use --force to overwrite." | ||
| case .encodingFailed: "Failed to encode config template." | ||
| } | ||
| } | ||
| } | ||
|
|
||
| extension ConfigWriteError: Sendable {} |
There was a problem hiding this comment.
Removing LocalizedError.errorDescription eliminates the user-facing error messages for config init/write failures. Since callers typically surface error.localizedDescription (or map errors into CLI output), this change will likely degrade the CLI error detail unless the messaging is reintroduced elsewhere. Consider keeping LocalizedError here or adding a dedicated mapping in the handler/output layer so .alreadyExists / .encodingFailed still produce actionable messages.
There was a problem hiding this comment.
The callers use try? which discards the error entirely. errorDescription was dead code.
| elapsedTime: playbackUseCase.elapsedTime(for: nowPlaying), | ||
| lyrics: result.plainLyrics, | ||
| syncedLyrics: timedLines, | ||
| currentLyric: timedLines.flatMap { lines in | ||
| nowPlaying.elapsed.flatMap { t in lines.last { $0.time <= t }?.text } | ||
| playbackUseCase.elapsedTime(for: nowPlaying).flatMap { t in lines.last { $0.time <= t }?.text } | ||
| } |
There was a problem hiding this comment.
elapsedTime(for:) is computed twice when building NowPlayingInfo (elapsedTime: and again for currentLyric). Because it depends on Date(), these values can diverge slightly within a single response, causing inconsistent UI/output. Consider computing elapsed once (e.g., let elapsed = ...) and reusing it for both fields.
| ignore: | ||
| - "Sources/DependencyInjection/**" | ||
| - "Sources/Domain/**" |
There was a problem hiding this comment.
Ignoring Sources/Domain/** in coverage now excludes files that contain real runtime logic (e.g., Domain/Misc/UserNotifier.swift includes an OSAScriptNotifier implementation and DI liveValue). If the intent is only to ignore protocol + stub definitions, consider narrowing the ignore patterns (or moving implementations out of Domain) so meaningful logic remains covered.
There was a problem hiding this comment.
Valid observation. UserNotifier has liveValue + OSAScriptNotifier in Domain, which is an architecture violation (should be in DependencyInjection). Will address in a follow-up. Domain ignore is still correct overall — it's 99% protocol + stub.
| **Wallpaper async resolution**: `WallpaperPresenter.start()` resolves wallpaper via `WallpaperInteractor` in a background Task. `WallpaperPresenter` also manages AVPlayer lifecycle (create, seek, loop, pause/play) and owns sleep/wake monitoring via `observeSleepWake()`. | ||
|
|
||
| **Domain Dependencies organization**: `Dependencies/` is organized by layer subdirectories (`Interactor/`, `UseCase/`, `Repository/`, `DataSource/`, `DataStore/`, `Misc/`) matching the architecture. Each file contains a protocol + `TestDependencyKey` + `DependencyValues` extension. | ||
| **Domain organization**: Domain module root is organized by layer subdirectories (`Interactor/`, `UseCase/`, `Repository/`, `DataSource/`, `DataStore/`, `Handler/`, `Misc/`) matching the architecture. Each file contains a protocol + `TestDependencyKey` + `DependencyValues` extension. |
There was a problem hiding this comment.
This statement says every Domain file contains a TestDependencyKey, but Domain/Misc/UserNotifier.swift currently defines UserNotifierKey: DependencyKey with a liveValue. Either adjust the implementation to match the documented convention, or update this documentation to reflect the exception.
| **Domain organization**: Domain module root is organized by layer subdirectories (`Interactor/`, `UseCase/`, `Repository/`, `DataSource/`, `DataStore/`, `Handler/`, `Misc/`) matching the architecture. Each file contains a protocol + `TestDependencyKey` + `DependencyValues` extension. | |
| **Domain organization**: Domain module root is organized by layer subdirectories (`Interactor/`, `UseCase/`, `Repository/`, `DataSource/`, `DataStore/`, `Handler/`, `Misc/`) matching the architecture. Most files follow the pattern of a protocol + `TestDependencyKey` + `DependencyValues` extension, but `Domain/Misc/UserNotifier.swift` is an exception and defines `UserNotifierKey: DependencyKey` with a `liveValue`. |
There was a problem hiding this comment.
Fair point. UserNotifierKey uses DependencyKey (not TestDependencyKey) because it provides liveValue inline. This is the exception, not the rule.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review
|
| Auto-apply | Compact |
|
|
Important
Your trial ends in 6 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.
Was this helpful? React with 👍 / 👎 | Gitar
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
概要
Domain の構造をフラット化し、Entity に残っていたロジックを Impl に移植。
変更内容
Domain フラット化
Sources/Domain/Dependencies/を削除、サブディレクトリを Domain 直下に移動Entity ロジック移植
NowPlaying.elapsed→PlaybackUseCaseImpl.elapsedTime(for:)ConfigWriteError.errorDescription→ 削除（未使用の LocalizedError 準拠）codecov.yml
Sources/Domain/**を ignore に追加（protocol + fatalError stub）その他
Closes #175
テスト計画
swift test全463テスト合格Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Chores
Tests