Conversation
- 이미지 컴포넌트를 프리뷰에서 확인할 수 있도록 로컬 모드일 때만 배경색을 반환하는 확장 함수 구현
- 검색 결과에서 "더보기" 클릭 시 카테고리별 데이터 확인 가능 - 카테고리 태그 포맷팅 처리 위치 변경 (Mapper -> UI)
📝 WalkthroughWalkthroughThis pull request introduces a "load more" feature for search results, renames the GraphDetailScreen parameter from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@feature/detail/src/main/java/com/metasearch/android/feature/detail/photo/PhotoDetailPresenter.kt:
- Around line 74-80: PhotoDetailPresenter currently uses
galleryRepository.getFileName(...) and falls back to an empty string which
causes invalid navigation to GraphDetailScreen; change the scope.launch handler
for PhotoDetailUiEvent.OnGraphButtonClick to check the result of
galleryRepository.getFileName(screen.imageUriString.toUri()) and if it's null or
blank do not call navigator.goTo but instead emit an error UI event or call the
presenter/view error handler (e.g., show an error toast/state), otherwise pass
the non-empty fileName into GraphDetailScreen; update references in
PhotoDetailPresenter to use this guard around
navigator.goTo(GraphDetailScreen(...)).
🧹 Nitpick comments (3)
feature/search/src/main/java/com/metasearch/android/feature/search/focusing/component/MoreButton.kt (1)
25-30: Add semantic role for accessibility.Using
clickablewithout semantics means screen readers won't announce this as a button. Consider addingrole = Role.Buttonfor proper accessibility support.♻️ Suggested fix
+import androidx.compose.ui.semantics.Role Box( modifier = Modifier .size(100.dp) .clip(RoundedCornerShape(MetaSearchTheme.radius.sm)) .background(Neutral800) - .clickable { onClick() }, + .clickable(role = Role.Button) { onClick() }, contentAlignment = Alignment.Center, ) {feature/search/src/main/java/com/metasearch/android/feature/search/focusing/component/SearchResultList.kt (2)
40-40: Consider extractingdisplayLimitas a named constant or parameter.The magic number
3could be made more self-documenting and easier to adjust if requirements change.♻️ Suggested refactor
+private const val PHOTO_DISPLAY_LIMIT = 3 + @Composable internal fun SearchResultList( modifier: Modifier = Modifier, result: SearchResult, onImageClick: (String) -> Unit, onMoreClick: (String) -> Unit, ) { LazyColumn( // ... ) { - val displayLimit = 3 + val displayLimit = PHOTO_DISPLAY_LIMIT
63-71: Modifier order may cause preview placeholder to not be clipped.The
previewPlaceholder()is applied beforeclip(), which means in previews the placeholder won't have rounded corners. Consider reordering for visual consistency.♻️ Suggested fix
modifier = Modifier .size(100.dp) - .previewPlaceholder() .clip(RoundedCornerShape(MetaSearchTheme.radius.sm)) + .previewPlaceholder() .clickable { onImageClick(photoName) },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
core/common/src/main/java/com/metasearch/android/core/common/extensions/ModifierExt.ktcore/data/api/src/main/java/com/metasearch/android/core/data/api/repository/GraphRepository.ktcore/data/impl/src/main/java/com/metasearch/android/core/data/impl/mapper/ResponseToModel.ktcore/data/impl/src/main/java/com/metasearch/android/core/data/impl/repository/GraphRepositoryImpl.ktfeature/detail/src/main/java/com/metasearch/android/feature/detail/graph/GraphDetailPresenter.ktfeature/detail/src/main/java/com/metasearch/android/feature/detail/photo/PhotoDetailPresenter.ktfeature/screens/src/main/java/com/metasearch/android/feature/screens/Screens.ktfeature/search/src/main/java/com/metasearch/android/feature/search/focusing/FocusingSearchPresenter.ktfeature/search/src/main/java/com/metasearch/android/feature/search/focusing/FocusingSearchUi.ktfeature/search/src/main/java/com/metasearch/android/feature/search/focusing/FocusingSearchUiState.ktfeature/search/src/main/java/com/metasearch/android/feature/search/focusing/component/MoreButton.ktfeature/search/src/main/java/com/metasearch/android/feature/search/focusing/component/SearchResultList.ktfeature/search/src/main/res/values/strings.xml
🧰 Additional context used
🧬 Code graph analysis (2)
feature/search/src/main/java/com/metasearch/android/feature/search/focusing/component/MoreButton.kt (1)
core/designsystem/src/main/java/com/metasearch/android/core/designsystem/theme/Theme.kt (1)
MetaSearchTheme(14-19)
feature/search/src/main/java/com/metasearch/android/feature/search/focusing/component/SearchResultList.kt (1)
feature/search/src/main/java/com/metasearch/android/feature/search/focusing/component/MoreButton.kt (1)
MoreButton(21-39)
🔇 Additional comments (15)
feature/search/src/main/res/values/strings.xml (1)
14-14: LGTM!The new string resource follows the existing naming convention and appropriately supports the "load more" feature.
core/data/impl/src/main/java/com/metasearch/android/core/data/impl/mapper/ResponseToModel.kt (1)
19-24: LGTM!The formatting change to use comma-separated category names (without hashtag prefixes) is applied consistently for related categories. This aligns with the change on line 33 for individual category groups.
feature/search/src/main/java/com/metasearch/android/feature/search/focusing/FocusingSearchUiState.kt (1)
25-30: LGTM!The new
OnMoreClickevent follows the established pattern and is well-documented. ThecategoryNameparameter correctly supports navigation to category-specific detail views.core/common/src/main/java/com/metasearch/android/core/common/extensions/ModifierExt.kt (1)
15-23: Missingthis.breaks modifier chaining.The
background()calls on line 20 are not chained tothis, meaning any modifiers applied beforepreviewPlaceholderwill be lost in preview mode.🐛 Proposed fix
@Composable fun Modifier.previewPlaceholder( color: Color = LightGray, shape: Shape? = null, ): Modifier = if (LocalInspectionMode.current) { - if (shape != null) background(color, shape) else background(color) + if (shape != null) this.background(color, shape) else this.background(color) } else { this }Likely an incorrect or invalid review comment.
core/data/api/src/main/java/com/metasearch/android/core/data/api/repository/GraphRepository.kt (1)
7-7: Parameter rename improves semantic clarity.The rename from
imageUriStringtoentityNamecorrectly reflects that this parameter represents an entity name rather than an image URI. All call sites have been updated, and the implementation properly URL-encodes the parameter before building the detail graph URL.core/data/impl/src/main/java/com/metasearch/android/core/data/impl/repository/GraphRepositoryImpl.kt (2)
27-31: URL construction logic looks correct.The refactoring to accept
entityNamedirectly and useURLEncoderfor proper URL encoding is a clean approach.
14-18: ThegalleryRepositorydependency is actively used in thefindMatchedUrimethod on line 41 and should not be removed. The original review comment incorrectly identifies it as unused after the refactoring.Likely an incorrect or invalid review comment.
feature/screens/src/main/java/com/metasearch/android/feature/screens/Screens.kt (1)
36-39: LGTM!The rename from
imageUriStringtoentityNameproperly reflects the semantic change in how GraphDetailScreen is now used—it represents a named entity rather than an image URI reference.feature/search/src/main/java/com/metasearch/android/feature/search/focusing/FocusingSearchPresenter.kt (1)
123-124: LGTM!The
OnMoreClickevent handling correctly navigates toGraphDetailScreenusingcategoryNameas the entity identifier. This aligns with the PR's goal of adding "load more" functionality for search results.feature/detail/src/main/java/com/metasearch/android/feature/detail/graph/GraphDetailPresenter.kt (1)
47-49: LGTM!The change correctly uses
screen.entityNameto fetch the detail graph URL, aligning with the updatedGraphDetailScreencontract andGraphRepositoryAPI.feature/search/src/main/java/com/metasearch/android/feature/search/focusing/component/MoreButton.kt (1)
41-49: LGTM!The preview is properly structured with
MetaSearchThemewrapper and appropriate annotation.feature/search/src/main/java/com/metasearch/android/feature/search/focusing/FocusingSearchUi.kt (2)
182-184: LGTM!The
onMoreClickcallback is correctly wired to emitFocusingSearchUiEvent.OnMoreClickwith the category name, consistent with the event model defined inFocusingSearchUiEvent.kt.
193-207: LGTM!The preview data provides good coverage for both scenarios: groups with more than 3 photos (showing the MoreButton) and groups with fewer photos. The
categoryNameformat aligns with the split logic inSearchResultList.feature/search/src/main/java/com/metasearch/android/feature/search/focusing/component/SearchResultList.kt (2)
44-49: LGTM!The tag transformation logic correctly formats category names for display while preserving the original
categoryNamefor theonMoreClickcallback.
56-80: LGTM!The display limit and
MoreButtonlogic are well-implemented:
- Photos are correctly limited to 3 items
hasMoreproperly determines when to show the buttonMoreButtonis rendered as aLazyRowitem maintaining consistent horizontal scrolling- The original
categoryNameis correctly passed toonMoreClickfor downstream navigation
| is PhotoDetailUiEvent.OnGraphButtonClick -> { | ||
| scope.launch { | ||
| val fileName = galleryRepository.getFileName(screen.imageUriString.toUri()) ?: "" | ||
|
|
||
| navigator.goTo(GraphDetailScreen(entityName = fileName)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Empty string fallback may cause invalid navigation.
When getFileName returns null, the code navigates to GraphDetailScreen with an empty entityName, which would result in an invalid URL (e.g., .../entityTripleGraph/dbName/). Consider showing an error or preventing navigation when the filename cannot be resolved.
🐛 Proposed fix
is PhotoDetailUiEvent.OnGraphButtonClick -> {
scope.launch {
val fileName = galleryRepository.getFileName(screen.imageUriString.toUri())
-
- navigator.goTo(GraphDetailScreen(entityName = fileName))
+ if (fileName != null) {
+ navigator.goTo(GraphDetailScreen(entityName = fileName))
+ } else {
+ toastMessage = "Unable to retrieve file information"
+ }
}
}🤖 Prompt for AI Agents
In
@feature/detail/src/main/java/com/metasearch/android/feature/detail/photo/PhotoDetailPresenter.kt
around lines 74 - 80, PhotoDetailPresenter currently uses
galleryRepository.getFileName(...) and falls back to an empty string which
causes invalid navigation to GraphDetailScreen; change the scope.launch handler
for PhotoDetailUiEvent.OnGraphButtonClick to check the result of
galleryRepository.getFileName(screen.imageUriString.toUri()) and if it's null or
blank do not call navigator.goTo but instead emit an error UI event or call the
presenter/view error handler (e.g., show an error toast/state), otherwise pass
the non-empty fileName into GraphDetailScreen; update references in
PhotoDetailPresenter to use this guard around
navigator.goTo(GraphDetailScreen(...)).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
New Features
UI Improvements
✏️ Tip: You can customize this high-level summary in your review settings.
카테고리별 더보기