-
-
Notifications
You must be signed in to change notification settings - Fork 98
Minor refactoring of ChildPanels composable #949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughReplaces the sealed PanelChild variant with a nullable-child Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI / Panels
participant Stack as PanelsStack
participant Helper as rememberBroadcastPredictiveBackParams
participant Renderer as PanelRenderer
UI->>Stack: read stack.items / active.instance
Note right of Stack: provides items, size, and first-item emptiness
UI->>Helper: call with key (panels) and params lambda
Helper->>Helper: invoke params() -> PredictiveBackParams?
alt stack size threshold (hardcoded 2) met
Helper->>UI: broadcast predictive onBack
else
Helper-->>UI: no broadcast
end
UI->>Renderer: pass PanelChild (nullable child)
alt child != null
Renderer->>UI: render child content
else
Renderer->>UI: render placeholder
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-28T22:32:21.630Z
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 880
File: docs/extensions/compose.md:429-431
Timestamp: 2025-05-28T22:32:21.630Z
Learning: In Decompose documentation, the correct import path for `subscribeAsState` in the Child Items navigation example is `com.arkivanov.decompose.extensions.compose.subscribeAsState`, not the jetbrains-specific variant. The import paths vary depending on the specific compose variant being used (general compose vs jetbrains/multiplatform compose).
Applied to files:
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build on Linux
- GitHub Check: Build on macOS
...monMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt
Outdated
Show resolved
Hide resolved
...monMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt
Outdated
Show resolved
Hide resolved
04dd6eb to
cb92f84
Compare
cb92f84 to
42e50b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt (1)
419-419: Consider slight Kotlin idiom enhancement (optional).The
isEmptyextension is clear and works well. For even more idiomatic Kotlin in the rendering blocks, you could optionally use?.letinstead of explicit null checks, though the current if-statements are equally readable:// Optional alternative at lines 283-286, 323-325, 367-369: it.instance.child?.let { child -> content(child) }This is purely stylistic—the current code is perfectly fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
extensions-compose-experimental/api/extensions-compose-experimental.klib.api(0 hunks)extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt(9 hunks)
💤 Files with no reviewable changes (1)
- extensions-compose-experimental/api/extensions-compose-experimental.klib.api
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-28T22:32:21.630Z
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 880
File: docs/extensions/compose.md:429-431
Timestamp: 2025-05-28T22:32:21.630Z
Learning: In Decompose documentation, the correct import path for `subscribeAsState` in the Child Items navigation example is `com.arkivanov.decompose.extensions.compose.subscribeAsState`, not the jetbrains-specific variant. The import paths vary depending on the specific compose variant being used (general compose vs jetbrains/multiplatform compose).
Applied to files:
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build on Linux
- GitHub Check: Build on macOS
🔇 Additional comments (5)
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt (5)
215-215: LGTM! Clean API simplification.Removing the
countparameter and hardcoding it to 2 (line 388) is a valid simplification since all call sites were using the same value. This reduces API surface area without losing functionality.
259-288: LGTM! MainPanel refactoring is sound.The signature change to
PanelChild<MC, MT>aligns with the new nullable-child design. The rendering logic correctly extracts and null-checks the child before rendering. The predictive back condition (items.size == 2) appropriately enables the back gesture when transitioning between panels.
293-329: LGTM! DetailsPanel refactoring correctly addresses previous critical issue.The switch from
stack.backStack.first()tostack.items.first()at line 315 is safe becauseitemsalways contains at least the active element, eliminating the crash risk when the back stack is empty. The predictive back logic correctly enables the gesture only when there are non-empty items to navigate back from. The nullable-child rendering pattern is properly implemented.
335-373: LGTM! ExtraPanel refactoring correctly addresses previous critical issue.Similar to DetailsPanel, the use of
stack.items.first()at line 359 safely avoids exceptions. The predictive back condition (stack.items.first() == EmptyChild1 && stack.size > 1) correctly enables the gesture only when the extra panel is actually present. The nullable-child rendering is properly handled.
412-417: LGTM! Elegant simplification of PanelChild design.Replacing the sealed interface with a simple nullable-child class reduces complexity while maintaining clear semantics. The
Emptycompanion object provides a clean way to represent absent panels.
Summary by CodeRabbit
Refactor
Breaking Changes