feat: add content width preference for the details screen (desktop)#646
Conversation
|
Caution Review failedFailed to post review comments WalkthroughAdds a persisted content-width UI setting (COMPACT, WIDE, EXTRA_WIDE) from domain model through repository persistence into app state, published via a CompositionLocal, surfaced in Tweaks UI, and applied to details screen width constraints. ChangesContent Width Setting Feature
Sequence Diagram(s)sequenceDiagram
participant TweaksUI as Tweaks UI (ContentWidthCard)
participant TweaksVM as TweaksViewModel
participant Repo as TweaksRepository
participant MainVM as MainViewModel
participant State as AppState
participant DetailUI as Details UI
TweaksUI->>TweaksVM: OnContentWidthSelected(width)
TweaksVM->>Repo: setContentWidth(width)
Repo-->>Repo: persist K_CONTENT_WIDTH
Repo-->>MainVM: getContentWidth() Flow emission
MainVM->>State: update contentWidth
State-->>DetailUI: LocalContentWidth provides width
DetailUI->>DetailUI: map width to dp and apply Modifier.widthIn(max)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 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 |
Greptile SummaryThis PR adds a "Content Width" preference to the Appearance settings (desktop only) that controls the maximum column width in the details screen, offering Compact, Wide, and Extra Wide options. The implementation follows established codebase patterns: a domain enum, repository getter/setter, composition local, and ViewModel plumbing.
Confidence Score: 3/5The dp values assigned to each preset in DetailsRoot (COMPACT→680dp, WIDE→960dp) are inverted relative to the PR description and the original hardcoded default, and every fallback path—fromName, LocalContentWidth, MainState, TweaksState, AppNavigation—uses COMPACT as the initial value rather than WIDE. The core mapping between enum entries and pixel values is wrong in DetailsRoot, and the preference falls back to the wrong preset on first launch. Together these mean the feature does not match its specification for any user. DetailsRoot.kt (the when-block mapping ContentWidth to dp values), ContentWidth.kt (fromName fallback), MainState.kt and TweaksState.kt (field default), AppNavigation.kt and LocalContentWidth.kt (default parameter / composition local default). Important Files Changed
Reviews (7): Last reviewed commit: "cr: drop reverseDirection on gutter scro..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * reaches [CrashReporter], preventing a spurious crash dump. | ||
| * | ||
| * Trade-off: macOS VoiceOver may miss updates on those removed nodes for the | ||
| * remainder of the session. Remove once the upstream fix lands (track against | ||
| * Compose MP 1.11+). | ||
| * | ||
| * See [GitHub-Store#330](https://github.com/OpenHub-Store/GitHub-Store/issues/330) | ||
| * and [GitHub-Store#640](https://github.com/OpenHub-Store/GitHub-Store/issues/640). | ||
| */ | ||
| object A11yCrashGuard { | ||
| // Separate flags per path so each path logs its first suppression independently. | ||
| private val warnedEdt = AtomicBoolean(false) | ||
| private val warnedUncaught = AtomicBoolean(false) | ||
|
|
||
| // Must be called after CrashReporter.install() so the uncaught-exception handler | ||
| // chain is: A11yCrashGuard (filter) -> CrashReporter (log + dump) -> JVM default. | ||
| fun install() { | ||
| val osName = System.getProperty("os.name")?.lowercase().orEmpty() | ||
| if (!osName.contains("mac")) return | ||
|
|
||
| // Path 1: NPE propagates out of the coroutine dispatcher and through the | ||
| // AWT EventQueue dispatch chain. | ||
| Toolkit.getDefaultToolkit().systemEventQueue.push(FilteringEventQueue()) | ||
|
|
There was a problem hiding this comment.
The A11yCrashGuard changes (adding the uncaught-exception handler for the coroutine-failure path, promoting isComposeA11yNpe to a shared method, and referencing issue #640) are independent of the content-width preference. Neither the PR title nor the description mentions them. Separating unrelated fixes into their own PRs makes bisection, revert, and review much easier.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Follows the AppTheme.displayName pattern — removes the hardcoded English string from the domain enum and replaces it with a @composable extension property in core/presentation/utils that calls stringResource(), making the labels translatable.
…h-preference # Conflicts: # core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/TweaksRepositoryImpl.kt
| val contentWidthDp = when (LocalContentWidth.current) { | ||
| ContentWidth.COMPACT -> 680.dp | ||
| ContentWidth.WIDE -> 960.dp | ||
| ContentWidth.EXTRA_WIDE -> androidx.compose.ui.unit.Dp.Unspecified | ||
| } |
There was a problem hiding this comment.
| val contentWidthDp = when (LocalContentWidth.current) { | |
| ContentWidth.COMPACT -> 680.dp | |
| ContentWidth.WIDE -> 960.dp | |
| ContentWidth.EXTRA_WIDE -> androidx.compose.ui.unit.Dp.Unspecified | |
| } | |
| val contentWidthDp = when (LocalContentWidth.current) { | |
| ContentWidth.COMPACT -> 480.dp | |
| ContentWidth.WIDE -> 680.dp | |
| ContentWidth.EXTRA_WIDE -> androidx.compose.ui.unit.Dp.Unspecified | |
| } |
| val contentWidth: ContentWidth = ContentWidth.COMPACT, | ||
| ) |
There was a problem hiding this comment.
COMPACT but the PR description explicitly says "Wide (680 dp — the current default, unchanged)." Any user who upgrades without a stored preference will have the layout read COMPACT from fromName(null) and, once the pixel values are corrected to 480/680, will silently see a narrowed 480 dp column instead of the expected 680 dp. The default must be WIDE to preserve existing behaviour. The same applies to TweaksState.
| val contentWidth: ContentWidth = ContentWidth.COMPACT, | |
| ) | |
| val contentWidth: ContentWidth = ContentWidth.WIDE, | |
| ) |
| ; | ||
|
|
||
| companion object { | ||
| fun fromName(name: String?): ContentWidth = entries.find { it.name == name } ?: COMPACT |
There was a problem hiding this comment.
fromName fallback returns COMPACT, so a first-launch user (no stored key) also starts on Compact instead of Wide. Changing the sentinel to WIDE here means every path that reads the preference falls back to the correct pre-existing default.
| fun fromName(name: String?): ContentWidth = entries.find { it.name == name } ?: COMPACT | |
| fun fromName(name: String?): ContentWidth = entries.find { it.name == name } ?: WIDE |
…sed overlap + smoother ru scrollbar copy
2 participants
What
Adds a Content width setting under Settings → Appearance (desktop only) that controls the maximum column width of the details screen. Three options: Compact (480 dp), Wide (680 dp — the current default, unchanged), Extra wide (no max-width cap, fills the window).
Why
Issue #633 requests features that bring the desktop experience closer to github.com. One concrete and scoped item is the compact / wide / extra-wide reading column GitHub.com itself exposes. Today the details screen is hardcoded to 680 dp regardless of window size; on large monitors that leaves a lot of empty space that power users may prefer to use.
The setting follows the same pattern as every other desktop-only preference in the codebase (
getScrollbarEnabled,getFontTheme, etc.):ContentWidthenum incore/domain/model/getContentWidth/setContentWidthinTweaksRepositoryandTweaksRepositoryImplLocalContentWidthcomposition local (same approach asLocalScrollbarEnabled)MainViewModel → MainState → Main → AppNavigation → CompositionLocalProviderDetailsRootreplacing the two hardcodedwidthIn(max = 680.dp)callsContentWidthCardadded to the Appearance section of Tweaks, gated ongetPlatform() != Platform.ANDROIDHow to test
Desktop only:
Addresses part of #633
Summary by CodeRabbit
New Features
Localization