perf(details): drop BoxWithConstraints subcompose + lazy keys#636
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDetailsRoot measures container height with onSizeChanged+LocalDensity instead of BoxWithConstraints. About/WhatsNew preprocess markdown off-main-thread, produce preview + chunked full content, and render progressively via ProgressiveMarkdown. Syntax highlighting and heavy markdown work run on Dispatchers.Default. Logs list items use stable keys; markdown components factory is non-@composable. New truncate/split utilities added. ChangesDetails Presentation Rendering Improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/Logs.kt`:
- Line 34: The key generation for the lazy list currently concatenates
it.timeIso + it.assetName which can produce ambiguous keys; update the key
lambda in Logs.kt (the key = { ... } expression) to combine timeIso and
assetName with an explicit delimiter (for example a pipe or null-safe separator)
so each item key is unambiguous (reference the properties timeIso and assetName
in the key lambda).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28229445-f0fb-4a21-974d-b45530e5026a
📒 Files selected for processing (2)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsRoot.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/Logs.kt
Greptile SummaryThis PR moves several expensive composition-time operations off the Main thread: markdown image pre-processing, syntax tokenization, and AST parsing are now dispatched to
Confidence Score: 4/5Safe to merge if the previously-flagged issues (named("test") qualifier, probeCache thread safety, isDark/remember key spinner flash) are tracked and resolved before or shortly after landing. The core async-offloading mechanics are sound and the ANR fix is well-targeted. Several issues identified in earlier review rounds remain unaddressed in this diff — most notably the production use of a named("test") DI qualifier (which crashes at runtime when that binding is absent) and the unsynchronised HashMap accessed from both Main and IO. About.kt and WhatsNew.kt carry the named("test") HttpClient injection and the isDark-keyed remember that resets content to null on theme toggle. MarkdownImageTransformer.kt has the unsynchronised probeCache HashMap. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Main Thread (Composition)
participant Def as Dispatchers.Default
participant IO as Dispatchers.IO
UI->>UI: "ExpandableMarkdownContent composed, fullChunks = null → show spinner"
UI->>Def: LaunchedEffect: applyThemeAwareImages(raw, isDark)
Def-->>UI: processed string
UI->>Def: splitMarkdownIntoChunks(processed, 4000)
Def-->>UI: List chunks
UI->>UI: "fullChunks = chunks → recompose, renderedCount = 1 → chunk[0] visible"
alt "isExpanded = true"
loop "while renderedCount < chunks.size"
UI->>UI: yield() → renderedCount++
UI->>UI: chunk[n] composed via rememberMarkdownState
end
end
UI->>UI: onSizeChanged fires → onMeasured(px)
UI->>UI: "needsExpansion = true → show Read more"
note over UI,IO: Image probing (per URL, once per session)
UI->>IO: LaunchedEffect: probeOnce(url) via probeClient.head()
IO-->>UI: ProbeResult.Allowed / Skipped
UI->>UI: "probeCache[url] = result"
note over UI,Def: Syntax highlighting (per code fence)
UI->>UI: SyntaxHighlightedCode: render plain code immediately
UI->>Def: LaunchedEffect: buildHighlighted(code, lang, isDark)
Def-->>UI: AnnotatedString with spans
UI->>UI: "highlighted = result → recompose with colours"
Reviews (7): Last reviewed commit: "feat(details): clickable markdown images..." | Re-trigger Greptile |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/markdown/GithubStoreMarkdownComponents.kt (1)
8-10: ⚡ Quick winInclude
imageTransformerin the memoization guidance.The returned lambdas close over both
isDarkandimageTransformer, so telling callers toremember(isDark)is incomplete. A caller following that literally can keep a stale transformer bound in the cachedMarkdownComponents.Suggested wording
-// Plain (non-@Composable) factory so callers can wrap in `remember(isDark)` +// Plain (non-@Composable) factory so callers can wrap in `remember(isDark, imageTransformer)`🤖 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 `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/markdown/GithubStoreMarkdownComponents.kt` around lines 8 - 10, The memoization guidance for the plain factory is incomplete: the returned lambdas close over both isDark and imageTransformer, so callers must remember both to avoid closing over a stale transformer. Update the comment/documentation for GithubStoreMarkdownComponents (the Plain factory) to instruct callers to use remember(isDark, imageTransformer) (or equivalent) so the cached MarkdownComponents capture both the current isDark and imageTransformer values.
🤖 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
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/About.kt`:
- Around line 161-167: The current remember call resets displayContent to null
as soon as rawMarkdown or isDark changes, which drops the existing Markdown tree
and shows the spinner; fix this by making the mutable state persistent across
key changes (so the old rendered markdown remains until the new processed string
is ready): replace remember(rawMarkdown, isDark) { mutableStateOf<String?>(null)
} with a key-less remember { mutableStateOf<String?>(null) } (or otherwise
ensure the state is not recreated on rawMarkdown/isDark changes), keep the
LaunchedEffect(rawMarkdown, isDark) that computes processed via
applyThemeAwareImages and assigns displayContent when done, and apply the same
change to the other identical block referenced (lines ~209-232) so
displayContent isn't cleared mid-flight.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/WhatsNew.kt`:
- Around line 167-173: The current remember keyed as remember(raw, isDark)
causes displayContent to reset to null before applyThemeAwareImages finishes;
change the state so it is not cleared on key changes—either replace the
remember(raw, isDark) call with an unkeyed remember {
mutableStateOf<String?>(null) } and keep LaunchedEffect(raw, isDark) to update
displayContent after withContext(Dispatchers.Default) {
applyThemeAwareImages(raw, isDark) }, or use produceState(initialValue = null,
raw, isDark) { value = withContext(Dispatchers.Default) {
applyThemeAwareImages(raw, isDark) } } so the Markdown/retainState is not torn
down while preprocessing runs (refer to displayContent, remember,
LaunchedEffect, applyThemeAwareImages, produceState).
---
Nitpick comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/markdown/GithubStoreMarkdownComponents.kt`:
- Around line 8-10: The memoization guidance for the plain factory is
incomplete: the returned lambdas close over both isDark and imageTransformer, so
callers must remember both to avoid closing over a stale transformer. Update the
comment/documentation for GithubStoreMarkdownComponents (the Plain factory) to
instruct callers to use remember(isDark, imageTransformer) (or equivalent) so
the cached MarkdownComponents capture both the current isDark and
imageTransformer values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d959550f-8f3f-434d-a732-f99e190d42f5
📒 Files selected for processing (3)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/About.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/WhatsNew.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/markdown/GithubStoreMarkdownComponents.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/WhatsNew.kt (1)
167-179:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid clearing the rendered markdown while preprocessing.
Lines 167-178 still key
displayContentandpreviewContenttoraw/isDark, so both reset tonullon theme or translation changes and the UI falls back to the spinner until the background work finishes. That reintroduces the flicker this refactor was trying to remove.Proposed fix
- var displayContent by remember(raw, isDark) { mutableStateOf<String?>(null) } - var previewContent by remember(raw, isDark) { mutableStateOf<String?>(null) } + var displayContent by remember { mutableStateOf<String?>(null) } + var previewContent by remember { mutableStateOf<String?>(null) } LaunchedEffect(raw, isDark) { val processed = withContext(Dispatchers.Default) { applyThemeAwareImages(raw, isDark) } val preview = withContext(Dispatchers.Default) { zed.rainxch.details.presentation.utils .truncateMarkdownPreview(processed, maxChars = 6000) } previewContent = preview displayContent = processed }🤖 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 `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/WhatsNew.kt` around lines 167 - 179, The current remember calls for displayContent and previewContent include raw and isDark as keys, causing them to reset to null (spinner) whenever those inputs change; update the remember usage so displayContent and previewContent are initialized without raw/isDark as keys (e.g., remember { mutableStateOf<String?>(null) }) so their previous values are preserved while LaunchedEffect(raw, isDark) does background work, and continue to assign previewContent and displayContent inside the LaunchedEffect after processing (functions: applyThemeAwareImages, truncateMarkdownPreview).
🧹 Nitpick comments (1)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownTruncate.kt (1)
3-18: ⚡ Quick winDrop the added KDoc/comment block here.
This helper is straightforward enough to read without the new prose, and the repo rules only allow KDoc/inline comments when they capture a non-obvious invariant, workaround, or concurrency detail. As per coding guidelines, "Do not add KDoc or inline comments unless explicitly requested; only add inline comments for non-obvious invariants, tricky concurrency, or workarounds".
🤖 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 `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownTruncate.kt` around lines 3 - 18, Remove the added KDoc/comment block above the truncateMarkdownPreview function; keep the function signature and implementation intact but delete the multi-line descriptive comment so only the code and any necessary inline comments for non-obvious invariants remain, targeting the truncateMarkdownPreview function and its surrounding comment block.
🤖 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
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/WhatsNew.kt`:
- Around line 249-257: The onSizeChanged handler currently early-returns when
"decisive" is true so only increases above effectiveHeight are reported; remove
that early-return and instead report any significant change (use the existing
abs(measured - lastReportedPx) < 1f threshold) so decreases are propagated too.
In practice, update lastReportedPx when the change passes the threshold and call
onMeasured(measured) regardless of whether measured is greater or smaller than
effectiveHeight; adjust the logic around effectiveHeight, collapsedHeightPx,
measured, lastReportedPx, onSizeChanged and onMeasured to ensure shrinks update
the parent state as well.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownTruncate.kt`:
- Around line 11-13: The function truncateMarkdownPreview doesn't validate
maxChars and will crash on substring(0, maxChars) if maxChars is negative;
update the function (truncateMarkdownPreview) to either call require(maxChars >=
0) at the start or clamp negative values to zero (e.g., val safeMax = maxOf(0,
maxChars)) before using substring, ensuring all subsequent uses (window =
content.substring(0, safeMax)) use the validated/clamped value and preserving
existing behavior when maxChars >= content.length.
---
Duplicate comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/WhatsNew.kt`:
- Around line 167-179: The current remember calls for displayContent and
previewContent include raw and isDark as keys, causing them to reset to null
(spinner) whenever those inputs change; update the remember usage so
displayContent and previewContent are initialized without raw/isDark as keys
(e.g., remember { mutableStateOf<String?>(null) }) so their previous values are
preserved while LaunchedEffect(raw, isDark) does background work, and continue
to assign previewContent and displayContent inside the LaunchedEffect after
processing (functions: applyThemeAwareImages, truncateMarkdownPreview).
---
Nitpick comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownTruncate.kt`:
- Around line 3-18: Remove the added KDoc/comment block above the
truncateMarkdownPreview function; keep the function signature and implementation
intact but delete the multi-line descriptive comment so only the code and any
necessary inline comments for non-obvious invariants remain, targeting the
truncateMarkdownPreview function and its surrounding comment block.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e471639f-3f8b-4b41-a071-d4634d041b09
📒 Files selected for processing (4)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/About.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/WhatsNew.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/markdown/SyntaxHighlightedCode.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownTruncate.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/About.kt
Comment on lines
+11
to
+13
| fun truncateMarkdownPreview(content: String, maxChars: Int): String { | ||
| if (content.length <= maxChars) return content | ||
| val window = content.substring(0, maxChars) |
Contributor
There was a problem hiding this comment.
Validate maxChars before slicing.
A negative maxChars reaches substring(0, maxChars) and crashes immediately. Since this helper is now reusable, it's worth making the contract explicit with a require(maxChars >= 0) or by clamping to zero.
Proposed fix
fun truncateMarkdownPreview(content: String, maxChars: Int): String {
+ require(maxChars >= 0) { "maxChars must be >= 0" }
if (content.length <= maxChars) return content
val window = content.substring(0, maxChars)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fun truncateMarkdownPreview(content: String, maxChars: Int): String { | |
| if (content.length <= maxChars) return content | |
| val window = content.substring(0, maxChars) | |
| fun truncateMarkdownPreview(content: String, maxChars: Int): String { | |
| require(maxChars >= 0) { "maxChars must be >= 0" } | |
| if (content.length <= maxChars) return content | |
| val window = content.substring(0, maxChars) |
🤖 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
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownTruncate.kt`
around lines 11 - 13, The function truncateMarkdownPreview doesn't validate
maxChars and will crash on substring(0, maxChars) if maxChars is negative;
update the function (truncateMarkdownPreview) to either call require(maxChars >=
0) at the start or clamp negative values to zero (e.g., val safeMax = maxOf(0,
maxChars)) before using substring, ensuring all subsequent uses (window =
content.substring(0, safeMax)) use the validated/clamped value and preserving
existing behavior when maxChars >= content.length.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownTruncate.kt (1)
61-62: 💤 Low valueFence toggle doesn't track opener type.
A
~~~line inside a```block (or vice versa) incorrectly togglesinFence, potentially splitting mid-fence. CommonMark requires matching fence types. This is rare in practice (mostly affects markdown-about-markdown READMEs).Optional fix to track fence opener
- var inFence = false + var fenceOpener: String? = null // "```" or "~~~" or null val lines = content.split('\n') ... for (line in lines) { val trimmed = line.trimStart() - if (trimmed.startsWith("```") || trimmed.startsWith("~~~")) { - inFence = !inFence + val opener = when { + trimmed.startsWith("```") -> "```" + trimmed.startsWith("~~~") -> "~~~" + else -> null + } + if (opener != null) { + fenceOpener = if (fenceOpener == opener) null else fenceOpener ?: opener } ... - if (!inFence && + if (fenceOpener == null &&🤖 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 `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownTruncate.kt` around lines 61 - 62, The fence toggling currently flips inFence on any line starting with "```" or "~~~", which mis-handles mixed fence types; change the logic to track the actual opener string instead: introduce a nullable String fenceOpener (initially null), compute opener via when { trimmed.startsWith("```") -> "```"; trimmed.startsWith("~~~") -> "~~~"; else -> null }, and when opener != null set fenceOpener = if (fenceOpener == opener) null else fenceOpener ?: opener; then replace checks against inFence with fenceOpener != null (or fenceOpener == null where appropriate) so only matching fence closers toggle the fence state (update all uses in MarkdownTruncate logic that referenced inFence).
🤖 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
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/WhatsNew.kt`:
- Line 45: Remove the duplicate import of androidx.compose.ui.unit.dp from the
WhatsNew.kt file: locate the redundant import statement (the one importing dp at
line 45) and delete it so only the original import (already present at line 38)
remains; ensure no other references are changed and that the file still compiles
with the single remaining dp import.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownTruncate.kt`:
- Around line 44-45: The function splitMarkdownIntoChunks should validate that
targetChunkChars is positive to avoid infinite/incorrect flushing when
targetChunkChars is zero or negative; add a precondition (e.g., use
require(targetChunkChars > 0) or throw IllegalArgumentException) at the start of
splitMarkdownIntoChunks (and any caller like truncateMarkdownPreview if present)
so the contract is explicit and the later check current.length >=
targetChunkChars behaves correctly.
---
Nitpick comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownTruncate.kt`:
- Around line 61-62: The fence toggling currently flips inFence on any line
starting with "```" or "~~~", which mis-handles mixed fence types; change the
logic to track the actual opener string instead: introduce a nullable String
fenceOpener (initially null), compute opener via when {
trimmed.startsWith("```") -> "```"; trimmed.startsWith("~~~") -> "~~~"; else ->
null }, and when opener != null set fenceOpener = if (fenceOpener == opener)
null else fenceOpener ?: opener; then replace checks against inFence with
fenceOpener != null (or fenceOpener == null where appropriate) so only matching
fence closers toggle the fence state (update all uses in MarkdownTruncate logic
that referenced inFence).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7be79dee-ed15-4d1f-b578-7a9aeb5a5248
📒 Files selected for processing (3)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/About.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/WhatsNew.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownTruncate.kt
| import com.mikepenz.markdown.model.rememberMarkdownState | ||
| import zed.rainxch.core.domain.util.applyThemeAwareImages | ||
| import zed.rainxch.details.presentation.markdown.githubStoreMarkdownComponents | ||
| import androidx.compose.ui.unit.dp |
Contributor
There was a problem hiding this comment.
Remove duplicate import.
androidx.compose.ui.unit.dp is already imported at line 38.
-import androidx.compose.ui.unit.dp📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import androidx.compose.ui.unit.dp |
🤖 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
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/WhatsNew.kt`
at line 45, Remove the duplicate import of androidx.compose.ui.unit.dp from the
WhatsNew.kt file: locate the redundant import statement (the one importing dp at
line 45) and delete it so only the original import (already present at line 38)
remains; ensure no other references are changed and that the file still compiles
with the single remaining dp import.
Comment on lines
+44
to
+45
| fun splitMarkdownIntoChunks(content: String, targetChunkChars: Int): List<String> { | ||
| if (content.length <= targetChunkChars) return listOf(content) |
Contributor
There was a problem hiding this comment.
Validate targetChunkChars to be positive.
If targetChunkChars is zero or negative, the condition at line 67 (current.length >= targetChunkChars) always passes, causing a flush on every blank line. Adding a precondition like truncateMarkdownPreview makes the contract explicit.
Proposed fix
fun splitMarkdownIntoChunks(content: String, targetChunkChars: Int): List<String> {
+ require(targetChunkChars > 0) { "targetChunkChars must be > 0" }
if (content.length <= targetChunkChars) return listOf(content)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fun splitMarkdownIntoChunks(content: String, targetChunkChars: Int): List<String> { | |
| if (content.length <= targetChunkChars) return listOf(content) | |
| fun splitMarkdownIntoChunks(content: String, targetChunkChars: Int): List<String> { | |
| require(targetChunkChars > 0) { "targetChunkChars must be > 0" } | |
| if (content.length <= targetChunkChars) return listOf(content) |
🤖 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
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownTruncate.kt`
around lines 44 - 45, The function splitMarkdownIntoChunks should validate that
targetChunkChars is positive to avoid infinite/incorrect flushing when
targetChunkChars is zero or negative; add a precondition (e.g., use
require(targetChunkChars > 0) or throw IllegalArgumentException) at the start of
splitMarkdownIntoChunks (and any caller like truncateMarkdownPreview if present)
so the contract is explicit and the later check current.length >=
targetChunkChars behaves correctly.
Comment on lines
+123
to
+128
| val probeClient = org.koin.compose.koinInject<io.ktor.client.HttpClient>( | ||
| qualifier = org.koin.core.qualifier.named("test"), | ||
| ) | ||
| val imageTransformer = remember(probeClient) { | ||
| MarkdownImageTransformer(probeClient) | ||
| } |
There was a problem hiding this comment.
named("test") qualifier injected in production UI
Both About.kt (here) and WhatsNew.kt (line 184) inject HttpClient under qualifier = named("test"). If this binding is only registered in a test DI module (which the name strongly implies), every user who opens a repo's detail screen will get a NoBeanDefinitionFoundException crash at runtime. Even if it is currently present in the production SharedModule, using a "test"-qualified dependency in production code is fragile — a future cleanup of test bindings would silently break this. The binding should either be registered under a production-stable qualifier (e.g. named("head_probe")) or a dedicated HttpClient parameter should be added to the DI graph.
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
Hard fixes for the README/release-notes lag + ANR observed on a Galaxy S25. All heavy work moved off Main.
1.
BoxWithConstraints→Box+onSizeChangedDetails previously nested
BoxWithConstraintsinsideScaffold. Both subcompose during the measure pass — nesting multiplies the cost on rotation, IME show, or any parent size change. TheBoxWithConstraintswas only consumingmaxHeightto derivecollapsedSectionHeight = maxHeight * 0.7f. Single-shot size read →Modifier.onSizeChanged. Cites: recomposition/avoiding-subcomposition-pitfalls.2.
items(state.installLogs)keyed + contentTypeLazy item cache could not reuse composables across insertions / scroll. Cites: lists/optimizing-lazy-layouts.
3. Async markdown pre-process + loader + measure debounce
applyThemeAwareImages(raw, isDark)(regex-heavy) ran insideremember { ... }— on the composition thread. Moved toLaunchedEffect + withContext(Dispatchers.Default), with aCircularProgressIndicatorclamped tocollapsedHeightwhile running.Switched from
Markdown(content = ...)toMarkdown(markdownState = rememberMarkdownState(..., retainState = true)).retainState = truekeeps the previously-parsed AST on screen while the next parse runs on Default — no flash to blank Loading on theme toggle.Hoisted
MarkdownParser,GFMFlavourDescriptor, andgithubStoreMarkdownComponents(isDark, transformer)intoremember(...). Dropped the unnecessary@Composableannotation on the components factory so callers can memoize.Debounced the
onSizeChanged → onMeasuredcallback. Each forwarded write recopies the fullDetailsState; the old code fired on every layout tick during the initial measure cascade — the "flicker after scroll completes" and "flicker on expand".4. Syntax highlighting moved to
Dispatchers.Default+ 16KB cutoff (the ANR fix)SyntaxHighlightedCode.buildHighlighted(...)was tokenizing the entire code block via theHighlightslibrary insideremember { ... }— every code fence parsed on Main during composition. For a README with N fences, that is N synchronous tokenizations on the UI thread. ABackground concurrent mark compact GC freed 49MBevent with ANR followed exactly this pattern.Fix: render plain code text immediately; tokenize in a
LaunchedEffect + withContext(Dispatchers.Default); swap the styledAnnotatedStringin once ready. Code blocks larger than 16 000 chars (embedded JSON dumps, generated YAML) are not highlighted at all — the tokenizer is super-linear, the readability payoff drops fast.5. Preview truncation when collapsed
The Markdown library composes every element on Main once given an AST. Even with parsing async, a kubernetes/kubernetes-sized README produces hundreds of
MarkdownText/MarkdownHeader/MarkdownCodeFencecomposables on first frame.Fix: compute a truncated preview (first ~6000 chars at a paragraph boundary) on
Dispatchers.Defaultalongside the full content. Render the preview when!isExpanded; render the full tree only when the user taps "Read more". First-frame composition cost falls by an order of magnitude on big READMEs.Test plan
Follow-ups (separate PRs)
List<T>fields inDetailsStateviakotlinx.collections.immutable.ImmutableList.DetailsStateinto focused sub-states (header / releases / readme / logs).Summary by CodeRabbit