SCRUM-231 design: add note write, edit form screen#55
Merged
gdaegeun539 merged 25 commits intoJun 10, 2026
Merged
Conversation
- except some todos
Set minimum height for note form text field instead of fixed height. Change modifier order in LyricsActionButton to apply click ripple correctly
Redesign the lyrics background selection sheet inside NoteFormScreen to match the UI specification, replacing the grid layout with a single-column list previewing the lyrics text on each background item. - Replace LazyVerticalGrid with LazyColumn to preview lyrics on backgrounds - Add a close button and update sheet header layout - Add checkbox icons to indicate the selected background - Enable JavaScript and DOM storage on Melon search WebView - Suppress related lint warnings (LongMethod, MultipleEmitters)
Update the layout of the lyrics search bottom sheet to include a header with a title and close button. Resolve the touch conflict inside the WebView by invoking requestDisallowInterceptTouchEvent on touch down. Additionally, adjust the lyrics background sheet height constraint and configure the search sheet to hide its drag handle and use the modal container color.
Add a border-styled row with SongListIcon and "곡" text to the bottom bar of NoteFormScreen. This button layout is prepared to let users select songs, which should be visible only for specific categories.
- Update placeholder text dynamically based on the selected note topic - Support toggle logic for song section visibility and no-song dialog - Preserve form inputs when switching topics to prevent data loss - Adapt text color dynamically for dark lyric backgrounds (Black/Red) - Add comprehensive unit tests for UI states and ViewModel behaviors
- Show song section when switching to interpretation topic - Keep song section visible when switching from interpretation if a song is already selected, otherwise hide it - Allow song delete button to be visible based on section visibility and edit mode status, regardless of song existence - Update unit tests for UI state and view model logic accordingly
- add gajae-code generated files
Break down the massive NoteFormScreen file into dedicated components (NoteFormContent, NoteFormSections, and NoteFormSheets) to improve code readability and maintainability.
Update the enable condition of the bottom song button to depend on the visibility of the song section instead of checking if the selected song is null. Add corresponding unit tests to verify the updated behavior of NoteFormUiState and NoteFormViewModel.
- Add NoteFormSongDeleteDialog to verify song deletion. - Clear lyrics and display a no-song dialog when trying to type lyrics without selecting a song first. - Update NoteFormViewModelTest to verify these behavior additions
|
Warning Review limit reached
More reviews will be available in 7 minutes and 44 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces a complete note creation and editing feature. It adds navigation routes for note form screens (create, edit, search song), state management via NoteFormViewModel, form UI components with conditional sections and modals, and integration with the existing community screen via a floating action button. A song search feature enables users to select songs during note creation. ChangesNote Form Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 |
Member
Author
|
Github Copilot 코드리뷰는 요청하지 않겠습니다. |
Member
Author
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ 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: 2
🧹 Nitpick comments (5)
app/src/main/java/com/lyrics/feelin/presentation/view/note/form/NoteFormSections.kt (1)
157-259: ⚡ Quick winConsider using LocalFeelinColors for all color references.
Lines 193, 209 reference hard-coded theme colors (
LightGray00,LightGray04,LightGray09,LightSystemDisable) directly. As per coding guidelines, prefer accessing colors viaLocalFeelinColorsto ensure proper theme handling. Verify these colors are available throughLocalFeelinColors.currentand use that accessor instead.🤖 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 `@app/src/main/java/com/lyrics/feelin/presentation/view/note/form/NoteFormSections.kt` around lines 157 - 259, In NoteFormLyricsSection replace direct usages of theme constants (LightGray00, LightGray09, LightGray04, LightSystemDisable) with values from the already-obtained LocalFeelinColors (the colors variable) so the component respects theming; update the textStyle color and the placeholder Text color checks to use colors.* properties (pick the closest equivalents in LocalFeelinColors) and ensure any conditional branches (e.g., lyricsBackground.isNoteFormDark()) map to the appropriate colors from colors rather than the hard-coded constants.Source: Coding guidelines
app/src/main/java/com/lyrics/feelin/presentation/view/note/form/NoteFormSheets.kt (2)
195-198: ⚡ Quick winUse
LocalFeelinColorshere instead of direct color constants.
LightGray00/LightGray08bypass the active palette while the rest of this file already reads fromLocalFeelinColors. Pull these foreground colors from the theme so this sheet stays aligned with future palette changes. As per coding guidelines, presentation composables should use project theme tokens viaLocalFeelinColorsinstead of ad-hoc colors.🤖 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 `@app/src/main/java/com/lyrics/feelin/presentation/view/note/form/NoteFormSheets.kt` around lines 195 - 198, Replace the hardcoded LightGray00/LightGray08 usage in the Text composable inside NoteFormSheets (the Text that uses background.isNoteFormDark()) with colors pulled from the theme via LocalFeelinColors; locate the Text call in NoteFormSheets.kt that currently sets color = if (background.isNoteFormDark()) LightGray00 else LightGray08 and change it to reference the appropriate foreground color tokens from LocalFeelinColors so the sheet follows the active palette.Source: Coding guidelines
59-63: ⚡ Quick winStandardize the new note-form composable signatures before they spread further.
The same root cause appears in both files:
modifieris missing or not the first optional parameter, and callbacks are placed before it. Normalizing these APIs now will keep the screen/sheet layer consistent and avoid churn once more call sites land. As per coding guidelines, UI composables inpresentationshould exposemodifier: Modifier = Modifier, keep it as the first optional parameter, and place lambda parameters last.🤖 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 `@app/src/main/java/com/lyrics/feelin/presentation/view/note/form/NoteFormSheets.kt` around lines 59 - 63, The NoteFormCategorySheet signature places modifier after callback parameters; per presentation UI guidelines, make modifier: Modifier = Modifier the first optional parameter and move lambda callbacks (onDismissRequest, onCategorySelect) to the end so that NoteFormCategorySheet(hasVisibleFlag, modifier: Modifier = Modifier, onDismissRequest: () -> Unit, onCategorySelect: (NoteTopic) -> Unit) — update the function declaration and all call sites accordingly; apply the same reordering to any other new note-form composables to keep APIs consistent.Source: Coding guidelines
app/src/test/java/com/lyrics/feelin/presentation/view/note/form/searchsong/SearchSongScreenSourceTest.kt (1)
33-40: ⚖️ Poor tradeoffHardcoded relative path may fail depending on working directory.
The path
"src/main/java/..."assumes the test runs from the project root. When executed from an IDE or different Gradle task context, the working directory may differ and causeFileNotFoundException.Additionally, source-level string matching is fragile—formatting changes, whitespace adjustments, or comments can break these assertions without changing behavior.
Consider these alternatives:
- If validating wiring behavior, use Compose UI tests or runtime assertions
- If enforcing code style, add a detekt custom rule
- If the behavior is already covered by integration tests, remove this test
More robust path resolution
If you must keep this approach, resolve the path relative to the source root:
private fun searchSongScreenSource(): String { - return File(SEARCH_SONG_SCREEN_PATH).readText() + val projectRoot = File(System.getProperty("user.dir")) + return File(projectRoot, SEARCH_SONG_SCREEN_PATH).readText() }However, this still won't address the brittleness of string matching.
🤖 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 `@app/src/test/java/com/lyrics/feelin/presentation/view/note/form/searchsong/SearchSongScreenSourceTest.kt` around lines 33 - 40, The test uses a brittle hardcoded path in searchSongScreenSource() via SEARCH_SONG_SCREEN_PATH which can break depending on the working directory and the whole approach of string-matching source is fragile; either remove/replace this test with a behavior-level assertion (e.g., Compose UI test or runtime wiring assertion) or, if you must keep it, change searchSongScreenSource() to resolve the source file robustly (derive the project/source-root at runtime instead of using "src/main/...", e.g., compute an absolute path from the current project root or locate the file via the module/sourceRoot) and consider normalizing whitespace before assertions to reduce fragility. Ensure the fix targets searchSongScreenSource() and the SEARCH_SONG_SCREEN_PATH usage.app/src/main/java/com/lyrics/feelin/presentation/view/note/form/searchsong/SearchSongScreen.kt (1)
41-46: ⚡ Quick winMove lambda parameters to the end of both composable signatures.
SearchSongRouteandSearchSongScreencurrently place lambda parameters before the end of the parameter list. Please align both APIs with the project’s composable parameter-order rule.Suggested refactor
`@Composable` fun SearchSongRoute( - onBackClick: () -> Unit, - onSongSelect: (MusicComponentData.NoteWriteMusicExist) -> Unit, modifier: Modifier = Modifier, viewModel: SearchSongViewModel = hiltViewModel(), + onBackClick: () -> Unit, + onSongSelect: (MusicComponentData.NoteWriteMusicExist) -> Unit, ) { ... } `@Composable` fun SearchSongScreen( uiState: SearchSongUiState, - onBackClick: () -> Unit, - onSearchQueryChange: (String) -> Unit, - onSongSelect: (MusicComponentData.NoteWriteMusicExist) -> Unit, modifier: Modifier = Modifier, + onBackClick: () -> Unit, + onSearchQueryChange: (String) -> Unit, + onSongSelect: (MusicComponentData.NoteWriteMusicExist) -> Unit, ) { ... }As per coding guidelines, in
app/src/main/java/com/lyrics/feelin/presentation/**/*.kt: “Keep lambda parameters as the last parameter in Composable functions” and “Keepmodifieras the first optional parameter in Composable functions”.Also applies to: 59-65
🤖 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 `@app/src/main/java/com/lyrics/feelin/presentation/view/note/form/searchsong/SearchSongScreen.kt` around lines 41 - 46, SearchSongRoute and SearchSongScreen place lambda parameters before the end of their parameter lists; reorder their signatures so that modifier remains the first optional parameter and all lambda parameters (onBackClick, onSongSelect, etc.) appear last in the parameter list for each Composable. Update both function declarations (SearchSongRoute and SearchSongScreen) and any call-sites to match the new parameter order so compilation and call semantics remain correct.Source: Coding guidelines
🤖 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/core/designsystem/component/FeelinModalBottomSheet.kt`:
- Line 55: The drag handle is always supplied which leaves reserved spacing;
update FeelinModalBottomSheet so the ModalBottomSheet's dragHandle parameter is
null when showDragHandle is false by passing dragHandle = if (showDragHandle) {
{ FeelinDragHandle() } } else null (i.e., supply FeelinDragHandle() only when
showDragHandle is true), ensuring the dragHandle is nullable and removing the
empty composable that causes the reserved area.
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/note/form/NoteFormScreen.kt`:
- Around line 35-39: The NoteFormScreen currently wires onCompleteClick to
onCloseClick which falsely signals success and drops user input; change this so
onCompleteClick is not the plain close action: either (A) create a dedicated
handler (e.g., onCompleteClick = { viewModel.saveDraft() ; onCloseClick() } or
onCompleteClick = viewModel::submitNote when submit exists) to persist or queue
the note before closing, or (B) block the complete action and surface an
explicit temporary behavior (e.g., show a confirmation/toast like "Draft not
saved" or open a confirmation dialog) so users are not misled; update the call
site that currently passes onCloseClick into NoteFormScreen and add the new
viewModel methods (saveDraft/submitNote) or a UI-only confirmation flow
accordingly.
---
Nitpick comments:
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/note/form/NoteFormSections.kt`:
- Around line 157-259: In NoteFormLyricsSection replace direct usages of theme
constants (LightGray00, LightGray09, LightGray04, LightSystemDisable) with
values from the already-obtained LocalFeelinColors (the colors variable) so the
component respects theming; update the textStyle color and the placeholder Text
color checks to use colors.* properties (pick the closest equivalents in
LocalFeelinColors) and ensure any conditional branches (e.g.,
lyricsBackground.isNoteFormDark()) map to the appropriate colors from colors
rather than the hard-coded constants.
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/note/form/NoteFormSheets.kt`:
- Around line 195-198: Replace the hardcoded LightGray00/LightGray08 usage in
the Text composable inside NoteFormSheets (the Text that uses
background.isNoteFormDark()) with colors pulled from the theme via
LocalFeelinColors; locate the Text call in NoteFormSheets.kt that currently sets
color = if (background.isNoteFormDark()) LightGray00 else LightGray08 and change
it to reference the appropriate foreground color tokens from LocalFeelinColors
so the sheet follows the active palette.
- Around line 59-63: The NoteFormCategorySheet signature places modifier after
callback parameters; per presentation UI guidelines, make modifier: Modifier =
Modifier the first optional parameter and move lambda callbacks
(onDismissRequest, onCategorySelect) to the end so that
NoteFormCategorySheet(hasVisibleFlag, modifier: Modifier = Modifier,
onDismissRequest: () -> Unit, onCategorySelect: (NoteTopic) -> Unit) — update
the function declaration and all call sites accordingly; apply the same
reordering to any other new note-form composables to keep APIs consistent.
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/note/form/searchsong/SearchSongScreen.kt`:
- Around line 41-46: SearchSongRoute and SearchSongScreen place lambda
parameters before the end of their parameter lists; reorder their signatures so
that modifier remains the first optional parameter and all lambda parameters
(onBackClick, onSongSelect, etc.) appear last in the parameter list for each
Composable. Update both function declarations (SearchSongRoute and
SearchSongScreen) and any call-sites to match the new parameter order so
compilation and call semantics remain correct.
In
`@app/src/test/java/com/lyrics/feelin/presentation/view/note/form/searchsong/SearchSongScreenSourceTest.kt`:
- Around line 33-40: The test uses a brittle hardcoded path in
searchSongScreenSource() via SEARCH_SONG_SCREEN_PATH which can break depending
on the working directory and the whole approach of string-matching source is
fragile; either remove/replace this test with a behavior-level assertion (e.g.,
Compose UI test or runtime wiring assertion) or, if you must keep it, change
searchSongScreenSource() to resolve the source file robustly (derive the
project/source-root at runtime instead of using "src/main/...", e.g., compute an
absolute path from the current project root or locate the file via the
module/sourceRoot) and consider normalizing whitespace before assertions to
reduce fragility. Ensure the fix targets searchSongScreenSource() and the
SEARCH_SONG_SCREEN_PATH usage.
🪄 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: ef3293e2-b3a0-4139-b9be-ecdf993262b1
📒 Files selected for processing (21)
.gitignoreapp/src/main/java/com/lyrics/feelin/core/designsystem/component/FeelinModalBottomSheet.ktapp/src/main/java/com/lyrics/feelin/core/designsystem/icon/Icons.ktapp/src/main/java/com/lyrics/feelin/navigation/FeelinDestination.ktapp/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.ktapp/src/main/java/com/lyrics/feelin/navigation/NoteFormNavigation.ktapp/src/main/java/com/lyrics/feelin/presentation/view/community/CommunityMainScreen.ktapp/src/main/java/com/lyrics/feelin/presentation/view/note/form/NoteFormContent.ktapp/src/main/java/com/lyrics/feelin/presentation/view/note/form/NoteFormContentActions.ktapp/src/main/java/com/lyrics/feelin/presentation/view/note/form/NoteFormScreen.ktapp/src/main/java/com/lyrics/feelin/presentation/view/note/form/NoteFormSections.ktapp/src/main/java/com/lyrics/feelin/presentation/view/note/form/NoteFormSheets.ktapp/src/main/java/com/lyrics/feelin/presentation/view/note/form/NoteFormUiState.ktapp/src/main/java/com/lyrics/feelin/presentation/view/note/form/NoteFormViewModel.ktapp/src/main/java/com/lyrics/feelin/presentation/view/note/form/searchsong/SearchSongScreen.ktapp/src/main/java/com/lyrics/feelin/presentation/view/note/form/searchsong/SearchSongUiState.ktapp/src/main/java/com/lyrics/feelin/presentation/view/note/form/searchsong/SearchSongViewModel.ktapp/src/main/res/drawable/image_gallery.xmlapp/src/test/java/com/lyrics/feelin/presentation/view/note/form/NoteFormUiStateTest.ktapp/src/test/java/com/lyrics/feelin/presentation/view/note/form/NoteFormViewModelTest.ktapp/src/test/java/com/lyrics/feelin/presentation/view/note/form/searchsong/SearchSongScreenSourceTest.kt
Fix an issue where the drag handle area is still rendered even when FeelinModalDialog's showDragHandle is false. By passing null instead of an empty lambda, the underlying ModalBottomSheet properly handles the omission of the drag handle.
Member
Author
|
병합하겠습니다 |
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.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
What is the current behavior?
노트 작성 및 수정 기능에 대한 화면 디자인이 구현되지 않은 상태입니다.
What is the new behavior (if this is a feature change)?
노트 작성 및 수정 기능에 대한 화면 디자인을 구현합니다.
동시에 서버 연동 이전에 추가할 수 있는 노트 작성에 대한 기능 요구사항을 일부 구현합니다.
노트 작성 화면과 수정 화면에 대한 라우트를 등록하고, 노트 작성 화면은 아티스트 메인 화면의 FAB에 라우트를 연결합니다.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No breaking changes.
ScreenShots
Light mode
Dark mode
Other information:
노트 수정화면은 노트 수정 기능을 연결할때 동작을 같이 검증해야 합니다. 현재 구현으로는 진입점이 없어 해당 라우트의 로직을 검증하지 못했습니다.
표현해야 하는 상태가 많아서, 스크린샷이 많아 스크린샷 폭을 줄여서 업로드합니다.
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores