π :: (#914) μμ€ν ν λ§ μ¬μ©μ μ ν μ 곡#915
Merged
π :: (#914) μμ€ν
ν
λ§ μ¬μ©μ μ ν μ 곡#915
Conversation
|
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 (3)
π§ Files skipped from review as they are similar to previous changes (1)
π WalkthroughWalkthroughAdds a new core:theme module with theme persistence, DI bindings, and UI integration; wires PreferencesDataStore for theme, exposes ThemeMode flows, persists selections, and integrates theme state into MainActivity and Settings. Changes
Sequence Diagram(s)sequenceDiagram
actor User as User
participant Setting as SettingScreen
participant SVM as SettingViewModel
participant TSDS as ThemeDataStoreDataSource
participant Store as ThemeStore
participant DS as PreferencesDataStore
participant MAViewModel as MainActivityViewModel
participant MainActivity as MainActivity
participant Theme as DmsTheme
User->>Setting: choose ThemeMode
Setting->>SVM: setThemeMode(mode)
SVM->>TSDS: setThemeMode(mode)
TSDS->>Store: setThemeMode(mode)
Store->>DS: write THEME_MODE = mode.name
rect rgba(0,200,100,0.5)
note over DS,Store: Persistence saved
end
DS->>Store: emit preferences update
Store->>TSDS: Flow emits ThemeMode
TSDS->>MAViewModel: themeMode StateFlow updates
MAViewModel->>MainActivity: themeMode collected
MainActivity->>MainActivity: resolve isDark (SYSTEM -> isSystemInDarkTheme())
MainActivity->>Theme: DmsTheme(isDarkTheme=isDark)
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~25 minutes Possibly related PRs
Suggested reviewers
π₯ 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
π§Ή Nitpick comments (7)
core/theme/src/main/java/team/aliens/dms/android/core/theme/datastore/store/ThemeStoreImpl.kt (1)
25-30: Minor: redundant null fallback insiderunCatching.
preferences[THEME_MODE] ?: ThemeMode.SYSTEM.nameis unnecessary β when the key is absent,ThemeMode.valueOf(null)would throw andrunCatching { ... }.getOrDefault(ThemeMode.SYSTEM)already handles that, producing the same result. The double fallback slightly obscures intent.β»οΈ Optional simplification
- override fun getThemeModeFlow(): Flow<ThemeMode> = - themeDataStore.data.map { preferences -> - runCatching { - ThemeMode.valueOf(preferences[THEME_MODE] ?: ThemeMode.SYSTEM.name) - }.getOrDefault(ThemeMode.SYSTEM) - } + override fun getThemeModeFlow(): Flow<ThemeMode> = + themeDataStore.data.map { preferences -> + val stored = preferences[THEME_MODE] ?: return@map ThemeMode.SYSTEM + runCatching { ThemeMode.valueOf(stored) }.getOrDefault(ThemeMode.SYSTEM) + }π€ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/theme/src/main/java/team/aliens/dms/android/core/theme/datastore/store/ThemeStoreImpl.kt` around lines 25 - 30, The null-coalescing fallback inside getThemeModeFlow is redundant: inside themeDataStore.data.map you're already wrapping ThemeMode.valueOf(...) with runCatching and calling .getOrDefault(ThemeMode.SYSTEM), so remove the ?: ThemeMode.SYSTEM.name and pass the raw preferences[THEME_MODE] into ThemeMode.valueOf; keep the runCatching and .getOrDefault as-is to handle absent/invalid values. Reference: getThemeModeFlow, ThemeMode.valueOf, THEME_MODE, themeDataStore.data, runCatching.core/theme/src/main/java/team/aliens/dms/android/core/theme/datastore/ThemeDataStoreDataSource.kt (1)
6-9: Preferinterfacefor this stateless contract.Same rationale as
ThemeStore: no state, no constructor, only used as a contract. Aninterfaceis more idiomatic and plays nicely with@Binds.β»οΈ Proposed refactor
-abstract class ThemeDataStoreDataSource { - abstract suspend fun setThemeMode(mode: ThemeMode) - abstract fun getThemeModeFlow(): Flow<ThemeMode> -} +interface ThemeDataStoreDataSource { + suspend fun setThemeMode(mode: ThemeMode) + fun getThemeModeFlow(): Flow<ThemeMode> +}See the comment on
ThemeStore.ktregarding the overall redundancy between these two sibling abstractions.π€ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/theme/src/main/java/team/aliens/dms/android/core/theme/datastore/ThemeDataStoreDataSource.kt` around lines 6 - 9, The file defines an abstract class ThemeDataStoreDataSource but itβs a stateless contract; replace it with an interface to be idiomatic and compatible with `@Binds`. Change ThemeDataStoreDataSource from abstract class to interface and keep the signatures suspend fun setThemeMode(mode: ThemeMode) and fun getThemeModeFlow(): Flow<ThemeMode> unchanged so implementations can be bound via `@Binds` and to match the existing ThemeStore pattern.core/theme/src/main/java/team/aliens/dms/android/core/theme/datastore/store/ThemeStore.kt (2)
6-9: Preferinterfaceover statelessabstract class.
ThemeStorehas no state or constructor logic and is used only as a pure contract (bound via@BindsinStoreModule). Aninterfaceis the idiomatic Kotlin choice and avoids forcing subclasses to call a superclass constructor.β»οΈ Proposed refactor
-internal abstract class ThemeStore { - abstract suspend fun setThemeMode(mode: ThemeMode) - abstract fun getThemeModeFlow(): Flow<ThemeMode> -} +internal interface ThemeStore { + suspend fun setThemeMode(mode: ThemeMode) + fun getThemeModeFlow(): Flow<ThemeMode> +}If you apply this, also update
ThemeStoreImplto use: ThemeStore(without parentheses).π€ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/theme/src/main/java/team/aliens/dms/android/core/theme/datastore/store/ThemeStore.kt` around lines 6 - 9, Replace the stateless abstract class ThemeStore with a Kotlin interface: declare ThemeStore as an interface exposing suspend fun setThemeMode(mode: ThemeMode) and fun getThemeModeFlow(): Flow<ThemeMode>, then update the implementation class ThemeStoreImpl to implement the interface using ": ThemeStore" (remove any superclass constructor call) and ensure the `@Binds` binding in StoreModule still binds ThemeStore to ThemeStoreImpl.
6-9: Redundant abstraction layer betweenThemeStoreandThemeDataStoreDataSource?
ThemeStoreandThemeDataStoreDataSourceexpose the exact same two members (setThemeMode,getThemeModeFlow), and per the AI summaryThemeDataStoreDataSourceImplsimply forwards every call toThemeStore. Unless there is a planned divergence (e.g., caching, additional data sources, extra members), one of the two layers can be collapsed β either haveThemeDataStoreDataSourceImpltalk to DataStore directly, or exposeThemeStoreitself to consumers and dropThemeDataStoreDataSource. Keeping both only increases DI wiring and boilerplate without behavioral value.π€ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/theme/src/main/java/team/aliens/dms/android/core/theme/datastore/store/ThemeStore.kt` around lines 6 - 9, The two-layer abstraction is redundant: either remove ThemeStore and let ThemeDataStoreDataSourceImpl (class ThemeDataStoreDataSourceImpl) talk directly to DataStore, or remove ThemeDataStoreDataSource (interface/class) and expose ThemeStore to consumers; pick one and collapse the other, update DI bindings to provide the remaining implementation, and remove the forwarding methods (setThemeMode, getThemeModeFlow) from the dropped layer so callers use the single remaining type (ThemeStore or ThemeDataStoreDataSourceImpl) directly.feature/src/main/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt (1)
23-56: LGTM!Theme mode observation in
initandsetThemeModecorrectly delegate to the data source. State synchronization viasetStateis straightforward.Minor note (optional): Wrapping
themeDataStoreDataSource.setThemeMode(mode)withDispatchers.IOis not strictly required since DataStore internally usesDispatchers.IOfor its write operations. It's harmless but can be omitted for consistency withobserveThemeMode()which doesn't specify a dispatcher.π€ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/src/main/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt` around lines 23 - 56, Remove the explicit Dispatchers.IO dispatcher in setThemeMode and let DataStore manage threading: change internal fun setThemeMode(mode: ThemeMode) { viewModelScope.launch(Dispatchers.IO) { themeDataStoreDataSource.setThemeMode(mode) } } to launch without specifying a dispatcher so it matches observeThemeMode() and relies on DataStoreβs internal IO handling; update the method signature/usage of setThemeMode and references to themeDataStoreDataSource.setThemeMode(mode) accordingly.feature/src/main/kotlin/team/aliens/dms/android/feature/setting/ui/SettingScreen.kt (2)
220-251: Consider a radio-selection pattern for the theme dialog.Rendering each
ThemeModeas a fullDmsButton(with a"β "prefix hack to denote selection) works but isn't the most idiomatic Material pattern for single-choice selection. ARadioButton+ label row per option would:
- Convey selection semantics to accessibility services (TalkBack announces "selected"/"not selected") instead of relying on a check glyph in the label text.
- Be more compact visually and easier to scan.
- Avoid the string concatenation for the checkmark, which complicates localization.
Not a blocker β current implementation is functional.
π€ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/src/main/kotlin/team/aliens/dms/android/feature/setting/ui/SettingScreen.kt` around lines 220 - 251, The ThemeSelectDialog currently renders each ThemeMode as a full DmsButton and uses a prefixed "β " string to indicate selection; change this to an idiomatic radio-selection pattern by replacing the DmsButton list inside ThemeSelectDialog with a Column of selectable rows that contain a RadioButton (selected = mode == currentThemeMode) and a Text label (mode.toDisplayName()), wire the row's clickable/onClick to call onThemeSelected(mode), and remove the checkmark string and buttonColor logic; ensure you iterate ThemeMode.entries and keep onDismissRequest and dismissButton behavior the same so accessibility (TalkBack selection state) and localization are preserved.
253-257: Consider movingtoDisplayName()alongsideThemeModeor using string resources.Hardcoded Korean strings ("μμ€ν κΈ°λ³Έ", "λΌμ΄νΈ", "λ€ν¬") in a
.ktfile bypass Android's string-resource system, which prevents translation, per-locale formatting, and runtime locale switching. If the app targets only Korean this is acceptable, but consider moving these labels intostrings.xmland resolving viastringResource(...)in the composable, keeping the enum free of display concerns.π€ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/src/main/kotlin/team/aliens/dms/android/feature/setting/ui/SettingScreen.kt` around lines 253 - 257, The extension ThemeMode.toDisplayName() currently returns hardcoded Korean strings which bypass Android string resources; move these labels into strings.xml and replace usage of toDisplayName() in the composable (e.g., SettingScreen) with calls to stringResource(R.string.some_key) mapped from the enum (e.g., when(themeMode) -> R.string.theme_system / R.string.theme_light / R.string.theme_dark), or alternatively relocate the toDisplayName() implementation next to the ThemeMode declaration only if it returns resource IDs instead of raw strings so the enum stays UI-agnostic.
π€ Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/team/aliens/dms/android/app/MainActivityViewModel.kt`:
- Around line 34-48: The startup coroutine flips _isStartupResolved before the
persisted theme from themeDataStoreDataSource.getThemeModeFlow() is observed,
causing a flicker; modify init so the startup-resolution coroutine first obtains
the initial theme (e.g., call getThemeModeFlow().first()/collect the first
emission) and set _themeMode.value to that before setting _isStartupResolved =
true (then after that, continue to collect subsequent emissions in a separate
collector or re-launch a continuous collector) so that _themeMode is resolved
synchronously during startup; update references to _themeMode,
themeDataStoreDataSource.getThemeModeFlow(), and _isStartupResolved in
MainActivityViewModel to implement this ordering.
---
Nitpick comments:
In
`@core/theme/src/main/java/team/aliens/dms/android/core/theme/datastore/store/ThemeStore.kt`:
- Around line 6-9: Replace the stateless abstract class ThemeStore with a Kotlin
interface: declare ThemeStore as an interface exposing suspend fun
setThemeMode(mode: ThemeMode) and fun getThemeModeFlow(): Flow<ThemeMode>, then
update the implementation class ThemeStoreImpl to implement the interface using
": ThemeStore" (remove any superclass constructor call) and ensure the `@Binds`
binding in StoreModule still binds ThemeStore to ThemeStoreImpl.
- Around line 6-9: The two-layer abstraction is redundant: either remove
ThemeStore and let ThemeDataStoreDataSourceImpl (class
ThemeDataStoreDataSourceImpl) talk directly to DataStore, or remove
ThemeDataStoreDataSource (interface/class) and expose ThemeStore to consumers;
pick one and collapse the other, update DI bindings to provide the remaining
implementation, and remove the forwarding methods (setThemeMode,
getThemeModeFlow) from the dropped layer so callers use the single remaining
type (ThemeStore or ThemeDataStoreDataSourceImpl) directly.
In
`@core/theme/src/main/java/team/aliens/dms/android/core/theme/datastore/store/ThemeStoreImpl.kt`:
- Around line 25-30: The null-coalescing fallback inside getThemeModeFlow is
redundant: inside themeDataStore.data.map you're already wrapping
ThemeMode.valueOf(...) with runCatching and calling
.getOrDefault(ThemeMode.SYSTEM), so remove the ?: ThemeMode.SYSTEM.name and pass
the raw preferences[THEME_MODE] into ThemeMode.valueOf; keep the runCatching and
.getOrDefault as-is to handle absent/invalid values. Reference:
getThemeModeFlow, ThemeMode.valueOf, THEME_MODE, themeDataStore.data,
runCatching.
In
`@core/theme/src/main/java/team/aliens/dms/android/core/theme/datastore/ThemeDataStoreDataSource.kt`:
- Around line 6-9: The file defines an abstract class ThemeDataStoreDataSource
but itβs a stateless contract; replace it with an interface to be idiomatic and
compatible with `@Binds`. Change ThemeDataStoreDataSource from abstract class to
interface and keep the signatures suspend fun setThemeMode(mode: ThemeMode) and
fun getThemeModeFlow(): Flow<ThemeMode> unchanged so implementations can be
bound via `@Binds` and to match the existing ThemeStore pattern.
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/setting/ui/SettingScreen.kt`:
- Around line 220-251: The ThemeSelectDialog currently renders each ThemeMode as
a full DmsButton and uses a prefixed "β " string to indicate selection; change
this to an idiomatic radio-selection pattern by replacing the DmsButton list
inside ThemeSelectDialog with a Column of selectable rows that contain a
RadioButton (selected = mode == currentThemeMode) and a Text label
(mode.toDisplayName()), wire the row's clickable/onClick to call
onThemeSelected(mode), and remove the checkmark string and buttonColor logic;
ensure you iterate ThemeMode.entries and keep onDismissRequest and dismissButton
behavior the same so accessibility (TalkBack selection state) and localization
are preserved.
- Around line 253-257: The extension ThemeMode.toDisplayName() currently returns
hardcoded Korean strings which bypass Android string resources; move these
labels into strings.xml and replace usage of toDisplayName() in the composable
(e.g., SettingScreen) with calls to stringResource(R.string.some_key) mapped
from the enum (e.g., when(themeMode) -> R.string.theme_system /
R.string.theme_light / R.string.theme_dark), or alternatively relocate the
toDisplayName() implementation next to the ThemeMode declaration only if it
returns resource IDs instead of raw strings so the enum stays UI-agnostic.
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt`:
- Around line 23-56: Remove the explicit Dispatchers.IO dispatcher in
setThemeMode and let DataStore manage threading: change internal fun
setThemeMode(mode: ThemeMode) { viewModelScope.launch(Dispatchers.IO) {
themeDataStoreDataSource.setThemeMode(mode) } } to launch without specifying a
dispatcher so it matches observeThemeMode() and relies on DataStoreβs internal
IO handling; update the method signature/usage of setThemeMode and references to
themeDataStoreDataSource.setThemeMode(mode) accordingly.
πͺ 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: 277d68a3-09dd-48de-93f4-4e5ab808287e
π Files selected for processing (23)
app/build.gradle.ktsapp/src/main/kotlin/team/aliens/dms/android/app/MainActivity.ktapp/src/main/kotlin/team/aliens/dms/android/app/MainActivityViewModel.ktbuildSrc/src/main/kotlin/ProjectPaths.ktcore/datastore/src/main/java/team/aliens/dms/android/core/datastore/DataStore.ktcore/datastore/src/main/java/team/aliens/dms/android/core/datastore/Qualifier.ktcore/datastore/src/main/java/team/aliens/dms/android/core/datastore/di/DataStoreModule.ktcore/theme/.gitignorecore/theme/build.gradle.ktscore/theme/consumer-rules.procore/theme/proguard-rules.procore/theme/src/main/AndroidManifest.xmlcore/theme/src/main/java/team/aliens/dms/android/core/theme/ThemeMode.ktcore/theme/src/main/java/team/aliens/dms/android/core/theme/datastore/ThemeDataStoreDataSource.ktcore/theme/src/main/java/team/aliens/dms/android/core/theme/datastore/ThemeDataStoreDataSourceImpl.ktcore/theme/src/main/java/team/aliens/dms/android/core/theme/datastore/store/ThemeStore.ktcore/theme/src/main/java/team/aliens/dms/android/core/theme/datastore/store/ThemeStoreImpl.ktcore/theme/src/main/java/team/aliens/dms/android/core/theme/di/DataSourceModule.ktcore/theme/src/main/java/team/aliens/dms/android/core/theme/di/StoreModule.ktfeature/build.gradle.ktsfeature/src/main/kotlin/team/aliens/dms/android/feature/setting/ui/SettingScreen.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.ktsettings.gradle.kts
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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
2 participants
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 by CodeRabbit