SCRUM-276 feature: Add global loading overlay in FeelinNavHost#50
Conversation
Add global blocking loading overlay to FeelinNavHost to prevent interactions and back gestures during async loading states. Delegate SettingScreen's logout loading overlay to this unified global implementation.
|
No actionable comments were generated in the recent review. π βΉοΈ Recent review infoβοΈ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: π Files selected for processing (2)
π WalkthroughWalkthroughThe PR centralizes the blocking loading overlay in MainScaffold (new isBlockingLoading flag), removes the overlay and loading parameter from SettingScreen, and updates the Settings route to pass isBlockingLoading; also adds macOS entries to .gitignore. ChangesBlocking Loading Overlay Refactor
Repository ignores
Sequence Diagram(s) sequenceDiagram
participant NavHost as FeelinNavHost
participant Scaffold as MainScaffold
participant Screen as SettingScreen
participant Overlay as BlockingOverlay
NavHost->>Scaffold: create with isBlockingLoading = (logoutStatus == LOADING)
Scaffold->>Screen: render content underlay
alt isBlockingLoading == true
Scaffold->>Overlay: show overlay, register BackHandler
Overlay->>Overlay: clear semantics + consume pointer input
Overlay->>Overlay: display CircularProgressIndicator
end
Estimated code review effortπ― 4 (Complex) | β±οΈ ~40 minutes Possibly related issues
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 |
gdaegeun539
changed the title
There was a problem hiding this comment.
Pull request overview
This PR moves the logout βblockingβ loading UI from SettingScreen into the shared MainScaffold (inside FeelinNavHost) so that the overlay covers both the main content and bottom navigation, and consumes system back events during logout loading.
Changes:
- Remove the screen-local logout loading overlay from
SettingScreenand itsisLogoutLoadingparameter. - Add
isBlockingLoadingtoMainScaffoldand render a full-screen dim + progress overlay above the scaffold when enabled. - Consume system back via
BackHandlerwhile blocking loading is active.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/src/main/java/com/lyrics/feelin/presentation/view/mypage/setting/SettingScreen.kt | Removes the local logout loading overlay and related parameter/imports. |
| app/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.kt | Adds isBlockingLoading to MainScaffold and implements a global overlay + back consumption during loading. |
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. π βΉοΈ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with π. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![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
π€ 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 `@app/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.kt`:
- Around line 443-444: In MainScaffold within FeelinNavHost (the layout block
setting start/end paddings), replace the incorrect use of
paddingValues.calculateEndPadding(LayoutDirection.Ltr) for the start inset with
paddingValues.calculateStartPadding(LayoutDirection.Ltr) so the start padding
uses the start calculation; update the call that currently references
calculateEndPadding for the start parameter to calculateStartPadding while
leaving the end parameter as calculateEndPadding.
πͺ 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 Plus
Run ID: 8285e6e7-8cb5-4ea6-b987-5c6b1c6b99b8
π Files selected for processing (2)
app/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.ktapp/src/main/java/com/lyrics/feelin/presentation/view/mypage/setting/SettingScreen.kt
π€ Files with no reviewable changes (1)
- app/src/main/java/com/lyrics/feelin/presentation/view/mypage/setting/SettingScreen.kt
- add antigravity cli generated files - add macos generated files
| Log.d("FeelinNavHost", "MainScaffold: bottomPadding=$bottomPadding") | ||
|
|
There was a problem hiding this comment.
μ΄λ² κΈ°λ₯μ ꡬν λ²μλ₯Ό λμ΄μλ λ³κ²½μ΄μ΄μ λ°μνμ§ μμ΅λλ€.
|
κΈ°λ₯ ꡬνμ΄ μλ£λμμμΌλ‘ λ³ν©νκ² μ΅λλ€. κ²°ν©λμ λ°λ₯Έ 리ν©ν λ§μ #51 λ‘ λ³λλ‘ λ±λ‘νμ΅λλ€. μΆκ° κΈ°λ₯ ꡬνμ μ°μ μ§ννκ³ μ΄λ₯Ό κ΄λ¦¬ν μμ μ λλ€. |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
What is the current behavior?
λ‘κ·Έμμ λ‘λ© μ€λ²λ μ΄κ°
SettingScreenλ΄λΆ content μμμλ§ νμλμ΄ νλ¨ νκ³Ό μμ€ν λ€λ‘κ°κΈ°λ‘ νλ©΄μ μ΄νν μ μμ΅λλ€.What is the new behavior (if this is a feature change)?
MainScaffoldμisBlockingLoadingμ μΆκ°ν΄ λ‘κ·Έμμ λ‘λ© μ€ main contentμ bottom navigation μμ λμΌν dim + progress overlayλ₯Ό νμν©λλ€. λ‘λ© μ€ μμ€ν back μ΄λ²€νΈλ μλΉν©λλ€.Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No.
ScreenShots (If needed)
User
λ‘λ© μ€λ²λ μ΄κ° νμλκ² νλ‘μ°λ₯Ό λ Ήνν΄ μ²¨λΆνμ΅λλ€. 5~6μ΄κ²½ μ μ μ€λ²λ μ΄κ° νμλ©λλ€.
Screen_Recording_20260523_102608_Feelin.mp4
Other information:
Verification:
cmd.exe /c gradlew.bat detektcmd.exe /c gradlew.bat :app:assembleDebugcmd.exe /c gradlew.bat testDebugUnitTestgit diff --checkManual device QA was not run because no Android device/emulator was attached in the local environment.
Summary by CodeRabbit
Improvements
Chores