SCRUM-267 design: add search result screen#33
Merged
gdaegeun539 merged 11 commits intoApr 21, 2026
Merged
Conversation
- add NoteTopic enum - represent note topic category
|
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:
📝 WalkthroughWalkthroughAdds a NoteTopic enum, UI and navigation for a note search results screen, a ViewModel with pagination and topic filtering, view state types, and supporting presentation utilities and commit workflow docs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NoteSearchScreen as "NoteSearchScreen\n(UI)"
participant NavController as "NavController"
participant NoteSearchResultScreen as "NoteSearchResultScreen\n(UI)"
participant VM as "NoteSearchResultViewModel"
participant Data as "DummyDataSource"
User->>NoteSearchScreen: tap music item (onMusicClick)
NoteSearchScreen->>NavController: navigate("note_search_result")
NavController->>NoteSearchResultScreen: show screen
NoteSearchResultScreen->>VM: init -> loadInitial()
VM->>Data: request filtered notes (topic, page=0)
Data-->>VM: return notes list
VM-->>NoteSearchResultScreen: emit viewState (SUCCESS/EMPTY)
NoteSearchResultScreen->>User: render notes
NoteSearchResultScreen-->>VM: near-end scroll -> loadNextPage()
VM->>Data: request next page
Data-->>VM: return next page
VM-->>NoteSearchResultScreen: emit updated viewState (append notes)
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/src/main/java/com/lyrics/feelin/presentation/util/NoteTopicLabelExtension.kt (1)
5-10: Move these labels to string resources.Hardcoded Korean copy here makes localization and copy changes require code edits. A safer shape is mapping
NoteTopicto@StringResand resolving the text in the UI layer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/lyrics/feelin/presentation/util/NoteTopicLabelExtension.kt` around lines 5 - 10, Replace the hardcoded Korean strings in the NoteTopic.label extension with resource IDs: change the extension on NoteTopic (NoteTopic.label) to return an `@StringRes` Int and map each enum case (NoteTopic.ALL, INTERPRETATION, FREE, QUESTION) to R.string identifiers (e.g., R.string.note_topic_all, etc.), then update call sites in the UI layer to resolve the text with context.getString(noteTopic.label) or view.getString(...); ensure string entries are added to strings.xml for each label and adjust imports/annotations to use androidx.annotation.StringRes where appropriate.app/src/main/java/com/lyrics/feelin/core/domain/enum/NoteTopic.kt (1)
3-7: KeepALLout of the domain enum.
NoteTopiclives undercore.domain, butALLis a UI filter, not an actual note category. Keeping it here makes later API or persistence mappings easy to misuse once this enum is reused outside the screen. A separate presentation-layer filter type would keep the boundary cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/lyrics/feelin/core/domain/enum/NoteTopic.kt` around lines 3 - 7, Remove the UI-only value ALL from the domain enum NoteTopic and keep NoteTopic limited to actual categories (INTERPRETATION, FREE, QUESTION); introduce a separate presentation-layer filter type (e.g., NoteTopicFilter or PresentationNoteTopic) in the UI/presentation module that includes ALL and any other UI-only options, and add explicit mapping code between the new presentation filter and the domain NoteTopic where the UI interacts with the domain (update callers that relied on NoteTopic.ALL to use the new filter and convert to nullable/Optional or explicit domain values before passing into domain/persistence code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSearchResultViewModel.kt`:
- Around line 27-33: When switching to the LOADING state in
NoteSearchResultViewModel, also reset the previous total note count so
ExpandedSearchHeader doesn't show the old count for the new query; update the
_viewState.value.copy call that sets status = NoteSearchResultStatus.LOADING to
include totalNoteCount = null (or 0 if your UI expects an int) alongside notes =
emptyList(), nextCursor = null, isNextPageLoading = false, and errorMessage =
null.
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSearchResultViewState.kt`:
- Around line 30-44: The initial() companion factory for
NoteSearchResultViewState currently hardcodes a NoteSearchResultSongSummary (시퍼런
봄 / 쏜애플); change the factory to accept the selected song (e.g., a
NoteSearchResultSongSummary? or required param) instead of embedding a concrete
song, remove the hardcoded NoteSearchResultSongSummary from
NoteSearchResultViewState.initial, and update callers (such as
NoteSearchResultViewModel construction) to pass the actual selected song into
the new factory so viewState.songSummary reflects the real selection at startup.
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSerachResultScreen.kt`:
- Around line 130-133: The NoteSearchResultScreen currently hardcodes an inert
back action; add a parameter onBackClick: () -> Unit = {} to the
NoteSearchResultScreen signature and pass it into the app bar/back-action
composable so the screen API exposes navigation to callers (also update the
other NoteSearchResultScreen declaration/usage that mirrors this
implementation). Locate the NoteSearchResultScreen function and any duplicate
definitions/usages, add the onBackClick parameter, and forward it to the app
bar's onBackClick handler so callers can supply navigation behavior.
---
Nitpick comments:
In `@app/src/main/java/com/lyrics/feelin/core/domain/enum/NoteTopic.kt`:
- Around line 3-7: Remove the UI-only value ALL from the domain enum NoteTopic
and keep NoteTopic limited to actual categories (INTERPRETATION, FREE,
QUESTION); introduce a separate presentation-layer filter type (e.g.,
NoteTopicFilter or PresentationNoteTopic) in the UI/presentation module that
includes ALL and any other UI-only options, and add explicit mapping code
between the new presentation filter and the domain NoteTopic where the UI
interacts with the domain (update callers that relied on NoteTopic.ALL to use
the new filter and convert to nullable/Optional or explicit domain values before
passing into domain/persistence code).
In
`@app/src/main/java/com/lyrics/feelin/presentation/util/NoteTopicLabelExtension.kt`:
- Around line 5-10: Replace the hardcoded Korean strings in the NoteTopic.label
extension with resource IDs: change the extension on NoteTopic (NoteTopic.label)
to return an `@StringRes` Int and map each enum case (NoteTopic.ALL,
INTERPRETATION, FREE, QUESTION) to R.string identifiers (e.g.,
R.string.note_topic_all, etc.), then update call sites in the UI layer to
resolve the text with context.getString(noteTopic.label) or view.getString(...);
ensure string entries are added to strings.xml for each label and adjust
imports/annotations to use androidx.annotation.StringRes where appropriate.
🪄 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: 86c0ce97-9065-40a4-ac4f-978a4f02d145
📒 Files selected for processing (5)
app/src/main/java/com/lyrics/feelin/core/domain/enum/NoteTopic.ktapp/src/main/java/com/lyrics/feelin/presentation/util/NoteTopicLabelExtension.ktapp/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSearchResultViewModel.ktapp/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSearchResultViewState.ktapp/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSerachResultScreen.kt
1 task
Update commit command and workflow documents to use the repository's English-only `type: Subject` format and staged-only flow with explicit confirmation before commit.
Reset the note count when a new search result load starts so the header does not briefly show the previous topic's total. Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.opencode/command/suggest_commit.md (1)
1-132: Consider refactoring shared content withcommit.md.Steps 1–4 (lines 9–122) are nearly identical between
suggest_commit.mdandcommit.md. This duplication makes maintenance harder and increases the risk of inconsistencies when rules are updated.Consider extracting the shared workflow steps into a common reference document (e.g.,
.agent/rules/commit-message-generation.md) and having both command files reference it, leaving only the execution/presentation differences in each file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.opencode/command/suggest_commit.md around lines 1 - 132, The sections in suggest_commit.md and commit.md are duplicated; extract the shared Steps 1–4 content into a new shared reference file named commit-message-generation.md and replace the duplicated blocks in both suggest_commit.md and commit.md with a short include/reference line pointing to commit-message-generation.md, leaving only the command-specific differences (presentation/execution rules) in each original file; update any internal references in suggest_commit.md (e.g., "Step 1: Gather Context", "Step 2: Analyze Changes", "Step 3: Generate Commit Message", "Step 4: Present Options") to use the shared document so future edits are made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.opencode/command/commit.md:
- Around line 98-122: In the "Suggested Commits" section update the three fenced
code blocks (Option 1 (recommended), Option 2 (minimal), Option 3 (detailed)) to
include a language specifier (e.g., text) after the opening triple-backticks so
markdownlint warnings are resolved; locate the blocks by their headings and
replace ``` with ```text for the examples under those three options.
- Around line 58-91: The commit guideline is missing the capitalization rule:
add a short bullet to the commit message rules (e.g., under "1. 커밋 메시지는 영문으로 쓴다"
or near the title rules) stating that "The first letter of the subject (after
the `type:` prefix) must be capitalized," mirroring the rule in the referenced
standard; ensure the wording is concise and consistent with existing bullets and
update any nearby examples or notes if present to reflect this capitalization
requirement.
In @.opencode/command/suggest_commit.md:
- Around line 98-122: The three fenced commit-message blocks under the headings
"Option 1 (recommended)", "Option 2 (minimal)", and "Option 3 (detailed)" are
missing language specifiers and trigger markdownlint warnings; fix by adding a
language tag (e.g., text) to each opening triple-backtick so the blocks become
```text, ensuring all three commit examples include the language specifier.
- Around line 57-92: Add the missing capitalization rule into the "Core rules"
section (near "1. 커밋 메시지는 영문으로 쓴다") by stating that the first letter of the
subject (the first character after the "type: " prefix, e.g., in "feat: Add
feature") must be capitalized; update the numbered list to include this rule and
mirror the wording from the referenced git-commit-rules.md standard so readers
know to use "feat: Add ..." not "feat: add ...".
---
Nitpick comments:
In @.opencode/command/suggest_commit.md:
- Around line 1-132: The sections in suggest_commit.md and commit.md are
duplicated; extract the shared Steps 1–4 content into a new shared reference
file named commit-message-generation.md and replace the duplicated blocks in
both suggest_commit.md and commit.md with a short include/reference line
pointing to commit-message-generation.md, leaving only the command-specific
differences (presentation/execution rules) in each original file; update any
internal references in suggest_commit.md (e.g., "Step 1: Gather Context", "Step
2: Analyze Changes", "Step 3: Generate Commit Message", "Step 4: Present
Options") to use the shared document so future edits are made in one place.
🪄 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: 4b0c9002-7e78-4970-ab2e-3fc913928d53
📒 Files selected for processing (4)
.agent/workflows/commit.md.opencode/command/commit.md.opencode/command/suggest_commit.mdapp/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSearchResultViewModel.kt
✅ Files skipped from review due to trivial changes (1)
- .agent/workflows/commit.md
Register the note search result route and connect the search and result screens through navigation
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSerachResultScreen.kt (1)
248-253: Add akeyparameter toitems()for stable identity during pagination.Without a key, adding items during pagination causes unnecessary recomposition of existing items.
NoteComponentDatahas anid: Longfield that should be used.♻️ Suggested fix
- items(viewState.notes) { note -> + items( + items = viewState.notes, + key = { it.id } + ) { note -> NoteComponent( noteData = note, modifier = Modifier.padding(horizontal = 20.dp, vertical = 8.dp), ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSerachResultScreen.kt` around lines 248 - 253, The items(...) call that renders NoteComponent from viewState.notes lacks a stable key, causing unnecessary recompositions during pagination; update the call to supply a key that uses the note id (e.g., key = { note.id } or key = { it.id }) so each NoteComponent is identified by its NoteComponentData.id and preserves identity across list updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.kt`:
- Around line 185-197: The selected song data from NoteSearchScreen (supplied
via onMusicClick as MusicComponentData.SearchNoteByMusic) is currently
discarded; modify navigation so the click handler forwards the song info to
NoteSearchResultScreen (rather than just
navController.navigate(FeelinDestination.NoteSearchResult.route)). Either: add a
song identifier to MusicComponentData.SearchNoteByMusic and pass it as a
navigation argument to FeelinDestination.NoteSearchResult.route, or serialize
the song payload (title+artist or JSON of
imageUrl/songName/artistName/noteCount) and attach it as a safe nav argument;
then update the destination composable (NoteSearchResultScreen) and its
ViewModel to read that argument (or use a shared ViewModel) and populate
songSummary. Ensure updates touch the onMusicClick call site in
NoteSearchScreen, the navController.navigate invocation for
FeelinDestination.NoteSearchResult.route, and the NoteSearchResultScreen input
handling (or its ViewModel) to consume the passed data.
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSerachResultScreen.kt`:
- Line 1: The file name contains a typo—rename the file from
NoteSerachResultScreen.kt to NoteSearchResultScreen.kt and update the
corresponding class/compose function name (e.g., NoteSerachResultScreen ->
NoteSearchResultScreen) plus any imports/usages across the codebase to match the
corrected identifier; ensure package declaration remains the same and run a
build/IDE refactor to update all references to avoid broken imports.
---
Nitpick comments:
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSerachResultScreen.kt`:
- Around line 248-253: The items(...) call that renders NoteComponent from
viewState.notes lacks a stable key, causing unnecessary recompositions during
pagination; update the call to supply a key that uses the note id (e.g., key = {
note.id } or key = { it.id }) so each NoteComponent is identified by its
NoteComponentData.id and preserves identity across list updates.
🪄 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: fad99845-0517-4d00-99e8-e7ffd47fa3ad
📒 Files selected for processing (4)
app/src/main/java/com/lyrics/feelin/navigation/FeelinDestination.ktapp/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.ktapp/src/main/java/com/lyrics/feelin/presentation/view/note/search/NoteSearchScreen.ktapp/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSerachResultScreen.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSearchResultScreen.kt (1)
157-166: Pagination guards are already in place; consider defensive guards on Composable side as optional improvement.The
loadNextPage()function inNoteSearchResultViewModelalready has protection against duplicate calls: theisNextPageLoadingcheck at line 66 causes an early return if a load is already in progress. This guard prevents the concern about repeated pagination triggers.That said, the suggestion to add defensive checks in the Composable side (
distinctUntilChanged()on the flow,isNextPageLoadingstate check before callingloadNextPage()) is still valid as a belt-and-suspenders approach and can improve clarity of intent. However, this is an optional improvement rather than a required fix, as the critical deduplication is already handled by the ViewModel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSearchResultScreen.kt` around lines 157 - 166, The composable's pagination trigger can fire repeatedly even though NoteSearchResultViewModel.loadNextPage already guards via isNextPageLoading; add an extra defensive layer in the LaunchedEffect snapshotFlow used with listState by applying distinctUntilChanged() so identical (lastVisible, total) pairs don't re-emit, and check the ViewModel's isNextPageLoading (or expose an isLoading state) before calling viewModel.loadNextPage() to avoid redundant calls; update the LaunchedEffect tied to listState/PAGINATION_PREFETCH_THRESHOLD to perform these checks and only call loadNextPage when distinct and not already loading.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSearchResultScreen.kt`:
- Around line 78-79: The header expand/collapse threshold uses raw pixels
(HEADER_EXPAND_OFFSET_THRESHOLD = 50) but is compared against
firstVisibleItemScrollOffset (pixel-based); change
HEADER_EXPAND_OFFSET_THRESHOLD to a Dp value (e.g., 50.dp) and, inside
rememberHeaderState(), convert that Dp to pixels using LocalDensity.current
(density.dipToPx or density.run { HEADER_EXPAND_OFFSET_THRESHOLD.toPx() })
before comparing to firstVisibleItemScrollOffset so the breakpoint is
density-independent; update references in rememberHeaderState() and any
comparisons that use HEADER_EXPAND_OFFSET_THRESHOLD, leaving
COLLAPSED_TOP_BAR_HEIGHT as-is.
---
Nitpick comments:
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSearchResultScreen.kt`:
- Around line 157-166: The composable's pagination trigger can fire repeatedly
even though NoteSearchResultViewModel.loadNextPage already guards via
isNextPageLoading; add an extra defensive layer in the LaunchedEffect
snapshotFlow used with listState by applying distinctUntilChanged() so identical
(lastVisible, total) pairs don't re-emit, and check the ViewModel's
isNextPageLoading (or expose an isLoading state) before calling
viewModel.loadNextPage() to avoid redundant calls; update the LaunchedEffect
tied to listState/PAGINATION_PREFETCH_THRESHOLD to perform these checks and only
call loadNextPage when distinct and not already loading.
🪄 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: 28b0cc18-7fc6-450d-b47f-39457ce4cd1b
📒 Files selected for processing (1)
app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSearchResultScreen.kt
Member
Author
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58efa292a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Rename the header expansion threshold to reflect its behavior and compare it as a density-aware pixel value instead of a raw number.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSearchResultScreen.kt (1)
137-137: Preview currently depends on runtime ViewModel resolution.
NoteSearchResultScreenPreview()(Line 448-451) callsNoteSearchResultScreen()which resolvesviewModel()at Line 137. IfNoteSearchResultViewModelis injected (e.g., Hilt constructor deps), Preview can fail or be non-deterministic. Consider a pure UI-content composable for preview with static state.#!/bin/bash set -euo pipefail vm_file="$(fd 'NoteSearchResultViewModel.kt' app/src/main/java | head -n1)" echo "ViewModel file: ${vm_file:-NOT_FOUND}" if [[ -n "${vm_file:-}" ]]; then echo "---- ViewModel declaration context ----" cat -n "$vm_file" | sed -n '1,220p' echo "---- Hilt/injection markers ----" rg -n '@HiltViewModel|class\s+NoteSearchResultViewModel|constructor\(' "$vm_file" fi echo "---- NoteSearchResultScreen call sites ----" rg -n 'NoteSearchResultScreen\s*\(' app/src/main/java -C2 echo "---- Existing screen-entry patterns (hiltViewModel) ----" rg -n --type=kt 'hiltViewModel\s*\(' app/src/main/java/com/lyrics/feelin/presentation/view -C2Also applies to: 448-451
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSearchResultScreen.kt` at line 137, NoteSearchResultScreen currently calls viewModel() at runtime which breaks deterministic previews; instead extract the UI into a pure composable (e.g., NoteSearchResultContent(state: NoteSearchState, onEvent: (NoteSearchEvent)->Unit)) and make NoteSearchResultScreen a thin wrapper that obtains a NoteSearchResultViewModel and passes its state/events into NoteSearchResultContent; update NoteSearchResultScreen signature to accept an optional viewModel parameter or only call viewModel from the wrapper, then change NoteSearchResultScreenPreview to call NoteSearchResultContent with a static NoteSearchState and noop handlers so preview no longer depends on runtime ViewModel resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSearchResultScreen.kt`:
- Around line 134-138: Reorder the composable parameter lists so any lambda
parameters come last: for NoteSearchResultScreen change the signature so
onBackClick (the lambda) is the final parameter (e.g., fun
NoteSearchResultScreen(modifier: Modifier = Modifier, viewModel:
NoteSearchResultViewModel = viewModel(), onBackClick: () -> Unit = {}) ), and
apply the same reordering to the other composable signature mentioned (the one
with a trailing lambda currently after modifier/viewModel) so all lambda params
are placed last.
- Around line 161-170: The prefetch inside LaunchedEffect/snapshotFlow can fire
repeatedly while a page is already loading; guard the call to
viewModel.loadNextPage() with an in-flight/status check (e.g., expose and read a
ViewModel property such as isLoading or hasNextPage from the UI state) so you
only call loadNextPage() when not currently loading and when more pages exist;
update the snapshotFlow collector to check viewModel.isLoading (or
viewModel.uiState.isLoading / viewModel.uiState.hasMore) along with the existing
PAGINATION_PREFETCH_THRESHOLD condition before invoking
viewModel.loadNextPage().
---
Nitpick comments:
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSearchResultScreen.kt`:
- Line 137: NoteSearchResultScreen currently calls viewModel() at runtime which
breaks deterministic previews; instead extract the UI into a pure composable
(e.g., NoteSearchResultContent(state: NoteSearchState, onEvent:
(NoteSearchEvent)->Unit)) and make NoteSearchResultScreen a thin wrapper that
obtains a NoteSearchResultViewModel and passes its state/events into
NoteSearchResultContent; update NoteSearchResultScreen signature to accept an
optional viewModel parameter or only call viewModel from the wrapper, then
change NoteSearchResultScreenPreview to call NoteSearchResultContent with a
static NoteSearchState and noop handlers so preview no longer depends on runtime
ViewModel resolution.
🪄 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: e5b5685e-f9e6-49eb-bb11-bea46e3c6c88
📒 Files selected for processing (1)
app/src/main/java/com/lyrics/feelin/presentation/view/note/search/result/NoteSearchResultScreen.kt
Member
Author
|
CodeRabbit Potential issue 모두 수정 완료했으므로 병합하겠습니다. |
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)?
곡을 이용한 노트 검색의 상세 화면을 추가합니다.
기능 연동은 로그인/회원가입 연동 이후 진행합니다.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
노트 카테고리를 표현하는 Enum을 추가합니다.
ScreenShots (If needed)
Other information:
#32 병합 이후 라우팅 연결을 진행하고 Draft를 해제하겠습니다.
Summary by CodeRabbit
New Features
Documentation