[REFACTOR] 레이아웃 팩토리 개선 및 빌더 도입 #151
Merged
Conversation
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
…pPool/iOS into refactor/#150-layout-abstract # Conflicts: # .gitignore
- SearchResult Section을 유동적으로 사용하던것을 분리함 - 이로 인해 dataSource의 의존성을 줄임
…pPool/iOS into refactor/#150-layout-abstract
## Walkthrough
This update introduces a compositional layout builder and protocol-driven layout providers for collection views, refactors the search feature's layout factory and reactor to use these abstractions, and unifies section management with a new `PopupSearchSection` enum. It also updates cell and enum naming, improves modularity, and enhances snapshot and empty state handling.
## Changes
| File(s) | Change Summary |
|---------|---------------|
| `.gitignore` | Added rules to ignore `**/buildServer.json` files and all files in `.vscode/` directories. |
| `Poppool/CoreLayer/Infrastructure/Infrastructure/Extension/Collection+.swift` | Added a public extension to `Collection` with a safe subscript for bounds-checked access. |
| `Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/CollectionLayoutBuilder.swift` | Introduced `CollectionLayoutBuilder` class for fluent compositional layout construction, with chainable methods for item/group configuration, headers, spacing, and insets. |
| `Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/CollectionLayoutProvidable.swift` | Added `CollectionLayoutProvidable` protocol requiring a `makeLayout()` method returning a layout section. |
| `Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/GridCollectionLayoutProvider.swift` | Added `GridCollectionLayoutProvider` struct implementing `CollectionLayoutProvidable` for grid layouts. |
| `Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/HeaderLayoutProvidable.swift` | Added `HeaderLayoutProvidable` protocol for providing header layout supplementary items. |
| `Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/TagCollectionLayoutProvider.swift` | Added `TagCollectionLayoutProvider` struct implementing both layout provider protocols for tag and header layouts. |
| `Poppool/PresentationLayer/SearchFeature/SearchFeature.xcodeproj/project.pbxproj` | Updated RxCocoa Swift package references and identifiers in the Xcode project file. |
| `Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/Factory/PopupSearchLayoutFactory.swift` | Refactored to a struct with injected section provider, delegated layout creation to provider structs and builder, removed direct data source dependency, and added a new section for empty search results. |
| `Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/Reactor/PopupSearchReactor.swift` | Refactored mutation logic into handler methods, replaced empty title mutation with a section update mutation, and unified search result loading logic. |
| `Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/SectionType/Section.swift` | Introduced `PopupSearchSection` enum with cases for each UI section. |
| `Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/View/Component/Cell/SearchResultEmptyCollectionViewCell.swift` | Renamed class and references from `SearchResultEmptyTitleCollectionViewCell` to `SearchResultEmptyCollectionViewCell`. |
| `Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/View/PopupSearchView.swift` | Replaced internal section enum with `PopupSearchSection`, updated data source and snapshot logic, renamed empty item case, and refined layout factory usage. |
| `Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/View/PopupSearchViewController.swift` | Added handling for `.searchResultEmpty` section, updated state binding to use new pulse property, and simplified snapshot update logic. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant ViewController
participant View
participant LayoutFactory
participant TagLayoutProvider
participant GridLayoutProvider
participant Builder
ViewController->>View: Configure collectionView
View->>LayoutFactory: Set sectionProvider
View->>LayoutFactory: makeCollectionViewLayout()
LayoutFactory->>LayoutFactory: sectionProvider(sectionIndex)
alt recentSearch/category
LayoutFactory->>TagLayoutProvider: makeLayout()/makeHeaderLayout()
TagLayoutProvider->>Builder: configure layout
Builder-->>TagLayoutProvider: NSCollectionLayoutSection/BoundarySupplementaryItem
TagLayoutProvider-->>LayoutFactory: section/header
else searchResult
LayoutFactory->>GridLayoutProvider: makeLayout()
GridLayoutProvider->>Builder: configure layout
Builder-->>GridLayoutProvider: NSCollectionLayoutSection
GridLayoutProvider-->>LayoutFactory: section
else searchResultEmpty
LayoutFactory->>Builder: configure empty layout
Builder-->>LayoutFactory: NSCollectionLayoutSection
end
LayoutFactory-->>View: UICollectionViewLayoutsequenceDiagram
participant Reactor
participant Handler
participant State
Reactor->>Handler: mutate(action)
Handler->>Handler: handleAction()
Handler-->>Reactor: Observable<Mutation>
Reactor->>State: reduce(state, mutation)
State-->>Reactor: new state
Poem
|
![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
🔭 Outside diff range comments (2)
Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/View/Component/Cell/SearchResultEmptyCollectionViewCell.swift (2)
37-42: 🛠️ Refactor suggestionUse
contentViewinstead ofselffor subviews
By convention, subviews of aUICollectionViewCellshould be added tocontentViewto ensure proper reuse and layout margins:- [emptyLabel].forEach { - self.addSubview($0) - } + [emptyLabel].forEach { + contentView.addSubview($0) + }
45-49: 🛠️ Refactor suggestionConstrain within
contentView
Adjust constraints to referencecontentViewinstead of the cell itself to avoid layout issues when the cell’s bounds change:- emptyLabel.snp.makeConstraints { make in - make.top.equalToSuperview().inset(145) - make.horizontalEdges.equalToSuperview() - make.height.equalTo(42) - } + emptyLabel.snp.makeConstraints { make in + make.top.equalTo(contentView).inset(145) + make.horizontalEdges.equalTo(contentView) + make.height.equalTo(42) + }
🧹 Nitpick comments (10)
.gitignore (1)
45-47: Simplify and strengthen Git ignore rules
**/buildServer.jsoncan be reduced tobuildServer.jsonsince Git matches files by name across all directories by default.- Use
.vscode/(with a trailing slash) to recursively ignore the entire.vscodedirectory rather than.vscode/*.Poppool/CoreLayer/Infrastructure/Infrastructure/Extension/Collection+.swift (1)
3-7: Doc comment suggestion for maintainability
Consider adding a documentation comment above the subscript to explain its purpose and usage, for example:/// Returns the element at `index` if it is within bounds, otherwise `nil`. /// - Parameter index: The position of the element to retrieve. /// - Returns: An optional element.Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/CollectionLayoutProvidable.swift (1)
3-5: Documentation for public API
As this is a public protocol, consider adding a docstring to guide implementors, for example:/// A type that can build and return a compositional layout section. public protocol CollectionLayoutProvidable { … }Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/View/Component/Cell/SearchResultEmptyCollectionViewCell.swift (1)
55-59: OverrideprepareForReuseto reset content
Ensure that when cells are reused, theemptyLabeltext is cleared:override func prepareForReuse() { super.prepareForReuse() emptyLabel.text = nil }Adding this improves robustness under rapid scrolling or data changes.
Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/SectionType/Section.swift (1)
3-9: Add documentation for clarity
Consider adding a brief doc comment explaining each case’s UI role, which will help future contributors understand the enum’s purpose at a glance.Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/HeaderLayoutProvidable.swift (1)
3-5: Documentation for public protocol
Since this is a public protocol, adding a doc comment will improve discoverability and usage guidance:/// Types conforming to this protocol can provide a boundary supplementary item (header) for collection layouts. public protocol HeaderLayoutProvidable { … }Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/View/PopupSearchViewController.swift (1)
210-222: Consider improving variable naming for clarity.The variable
isEmptyis confusing because it's actually checking ifupdateSearchResultSectionis nil, but the presence of a value inupdateSearchResultSectionindicates an empty search state (the opposite of what the name suggests).- let isEmpty = state.updateSearchResultSection == nil - let emptyCaseTitle = state.updateSearchResultSection + let hasEmptyState = state.updateSearchResultSection != nil + let emptyStateMessage = state.updateSearchResultSection owner.mainView.updateSearchResultSectionSnapshot( with: state.searchResultItems.map(PopupSearchView.SectionItem.searchResultItem), header: PopupSearchView.SectionItem.searchResultHeaderItem(state.searchResultHeader), - empty: isEmpty ? nil : PopupSearchView.SectionItem.searchResultEmptyItem(emptyCaseTitle!) + empty: hasEmptyState ? PopupSearchView.SectionItem.searchResultEmptyItem(emptyStateMessage!) : nil )Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/CollectionLayoutBuilder.swift (2)
74-107: Consider simplifying the default case in the axis switch.Since
UIAxisonly has.verticaland.horizontalcases, the default case seems unnecessary and could be simplified.switch axis { case .vertical: group = NSCollectionLayoutGroup.vertical( layoutSize: groupSize, subitems: Array(repeating: item, count: numberOfItemsPerGroup) ) case .horizontal: group = NSCollectionLayoutGroup.horizontal( layoutSize: groupSize, subitems: Array(repeating: item, count: numberOfItemsPerGroup) ) - - default: fatalError("Can't compose section to selected axis") }
176-184: Consider adding more descriptive error messages.The fatal error messages could be more specific to help developers understand what went wrong during development.
public func build() -> NSCollectionLayoutSection { - guard let section else { fatalError("Section must be created before building") } + guard let section else { fatalError("Section must be created using composeSection(_:) before calling build()") } return section } public func buildHeader() -> NSCollectionLayoutBoundarySupplementaryItem { - guard let headerItem else { fatalError("Header must be created before building") } + guard let headerItem else { fatalError("Header must be created using header(...) method before calling buildHeader()") } return headerItem }Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/Factory/PopupSearchLayoutFactory.swift (1)
32-36: Avoid re-creating the grid layout on every call
self.gridLayoutProvider.makeLayout()is executed for every.searchResultsection query.
IfmakeLayout()builds a fullNSCollectionLayoutSectioneach time, this closure will allocate new layout objects repeatedly while scrolling.Cache the section once, or let
GridCollectionLayoutProviderexpose an immutablesectionproperty.private lazy var searchResultSection = gridLayoutProvider.makeLayout() ... case .searchResult: return searchResultSection
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (14)
.gitignore(1 hunks)Poppool/CoreLayer/Infrastructure/Infrastructure/Extension/Collection+.swift(1 hunks)Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/CollectionLayoutBuilder.swift(1 hunks)Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/CollectionLayoutProvidable.swift(1 hunks)Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/GridCollectionLayoutProvider.swift(1 hunks)Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/HeaderLayoutProvidable.swift(1 hunks)Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/TagCollectionLayoutProvider.swift(1 hunks)Poppool/PresentationLayer/SearchFeature/SearchFeature.xcodeproj/project.pbxproj(4 hunks)Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/Factory/PopupSearchLayoutFactory.swift(1 hunks)Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/Reactor/PopupSearchReactor.swift(6 hunks)Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/SectionType/Section.swift(1 hunks)Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/View/Component/Cell/SearchResultEmptyCollectionViewCell.swift(3 hunks)Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/View/PopupSearchView.swift(7 hunks)Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/View/PopupSearchViewController.swift(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/CollectionLayoutProvidable.swift (2)
Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/GridCollectionLayoutProvider.swift (1)
makeLayout(6-16)Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/TagCollectionLayoutProvider.swift (1)
makeLayout(6-14)
Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/HeaderLayoutProvidable.swift (1)
Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/TagCollectionLayoutProvider.swift (1)
makeHeaderLayout(16-20)
Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/TagCollectionLayoutProvider.swift (2)
Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/GridCollectionLayoutProvider.swift (1)
makeLayout(6-16)Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/CollectionLayoutBuilder.swift (9)
item(17-28)group(30-41)composeSection(74-107)withScrollingBehavior(134-139)groupSpacing(141-146)build(176-179)header(109-132)header(167-174)buildHeader(181-184)
Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/View/PopupSearchViewController.swift (1)
Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/View/PopupSearchView.swift (1)
updateSearchResultSectionSnapshot(278-299)
Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/GridCollectionLayoutProvider.swift (2)
Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/TagCollectionLayoutProvider.swift (1)
makeLayout(6-14)Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/CollectionLayoutBuilder.swift (8)
item(17-28)group(30-41)numberOfItemsPerGroup(43-48)itemSpacing(50-55)composeSection(74-107)withContentInsets(57-72)groupSpacing(141-146)build(176-179)
Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/View/PopupSearchView.swift (2)
Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/Factory/PopupSearchLayoutFactory.swift (2)
setSectionProvider(12-14)makeCollectionViewLayout(16-38)Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/CollectionLayoutBuilder.swift (2)
header(109-132)header(167-174)
🪛 SwiftLint (0.57.0)
Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/View/PopupSearchView.swift
[Error] 216-216: Force casts should be avoided
(force_cast)
🔇 Additional comments (17)
Poppool/PresentationLayer/SearchFeature/SearchFeature.xcodeproj/project.pbxproj (4)
10-10: PBXBuildFile entry updated for RxCocoa
The new PBXBuildFile ID (0506BE882DD79A6C006CDEDE) correctly references the XCSwiftPackageProductDependency ID (0506BE872DD79A6C006CDEDE) for RxCocoa, ensuring the framework is now pulled in via Swift Package Manager.
181-181: Frameworks build phase reference updated
ThePBXFrameworksBuildPhasenow includes the updated RxCocoa build file ID (0506BE882DD79A6C006CDEDE), aligning with the new PBXBuildFile entry above.
314-314: SearchFeature target SPM dependency updated
ThepackageProductDependencieslist for the SearchFeature target now correctly includes the new RxCocoa dependency ID (0506BE872DD79A6C006CDEDE), matching the Swift package product entry.
941-945: XCSwiftPackageProductDependency entry for RxCocoa
The XCSwiftPackageProductDependency block declares the RxCocoa product under the RxSwift package reference with the correct new ID. This completes the SPM integration for RxCocoa.Poppool/CoreLayer/Infrastructure/Infrastructure/Extension/Collection+.swift (1)
3-7: Safety-first: Safe subscript implementation looks solid
The newsubscript(safe:)correctly guards against out-of-bounds access by leveragingindices.contains(_:). This will prevent crashes when indexing. Consider adding unit tests for Arrays, Dictionaries, and Strings to exercise this extension.Would you like me to scaffold a set of XCTest cases covering different
Collectiontypes?Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/CollectionLayoutProvidable.swift (1)
3-5: Protocol design looks good
TheCollectionLayoutProvidableprotocol cleanly abstracts the creation ofNSCollectionLayoutSection. It enables you to swap providers easily and aligns well with SOLID principles.Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/SectionType/Section.swift (1)
3-9: Verify access level for cross-module use
Theenum PopupSearchSectionis currently internal. If you plan to use it from other modules (e.g., layout factories in the DesignSystem), consider marking itpublic. Otherwise, internal access is sufficient within the SearchFeature target.Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/HeaderLayoutProvidable.swift (1)
3-5: Protocol abstraction is solid
TheHeaderLayoutProvidableprotocol cleanly separates header layout concerns, complementingCollectionLayoutProvidable. This adds modularity to your layout-building approach.Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/GridCollectionLayoutProvider.swift (1)
3-17: LGTM! Well-structured grid layout provider implementation.The implementation correctly uses the builder pattern to create a consistent grid layout with proper spacing and insets. The two-column grid with fixed item height and appropriate spacing will create a visually balanced layout.
Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/View/PopupSearchViewController.swift (1)
106-107: LGTM! Proper handling for empty search results section.Correctly returning nil prevents actions when selecting items in the empty search result section, consistent with the existing pattern for the header section.
Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/TagCollectionLayoutProvider.swift (2)
3-14: LGTM! Clean implementation for tag collection layout.The implementation properly configures a vertical layout with continuous scrolling behavior, which is ideal for displaying horizontally scrollable tags. The estimated width with fixed height accommodates variable text lengths.
16-20: LGTM! Well-implemented header layout provider.The header implementation with a fixed height of 24 points is appropriate for section headers in a tag collection.
Poppool/PresentationLayer/DesignSystem/DesignSystem/Components/Layout/CollectionLayoutBuilder.swift (4)
3-16: LGTM! Well-structured builder class initialization.The class effectively implements the builder pattern with appropriate property storage and initialization options. The option to initialize with an existing section provides good flexibility.
17-72: LGTM! Great implementation of chainable configuration methods.The methods for configuring item sizes, group sizes, item count, spacing, and content insets are well-designed with proper return types for chaining and
@discardableResultannotation.
109-146: LGTM! Well-implemented header configuration and scrolling behavior.The methods for configuring headers and scrolling behavior are appropriately implemented with good default values and proper return types for method chaining.
148-174: LGTM! Good flexibility with section and header modification methods.The custom modifiers for sections and headers provide additional flexibility for scenarios not covered by the standard builder methods.
Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/Factory/PopupSearchLayoutFactory.swift (1)
10-17:sectionProviderbeing optional can silently break the layout at runtime
makeCollectionViewLayout()returns a compositional layout that assumessectionProviderhas been injected beforehand.
If a caller forgets to callsetSectionProvider, every section will resolve toniland nothing will be shown – no compile-time hint, no assert.Consider making the dependency explicit by:
- Initialising the factory with a non-optional provider, or
- Throwing an assertion / precondition failure when the provider is missing.
- private var sectionProvider: ((Int) -> PopupSearchSection?)? + private let sectionProvider: (Int) -> PopupSearchSection? - mutating func setSectionProvider(_ provider: @escaping (Int) -> PopupSearchSection?) { - self.sectionProvider = provider - } + init(sectionProvider: @escaping (Int) -> PopupSearchSection?) { + self.sectionProvider = sectionProvider + }This fails fast and prevents silent UI breakage.
Comment on lines
+212
to
217
| case .searchResultEmptyItem(let title): | ||
| let cell = collectionView.dequeueReusableCell( | ||
| withReuseIdentifier: SearchResultEmptyTitleCollectionViewCell.identifiers, | ||
| withReuseIdentifier: SearchResultEmptyCollectionViewCell.identifiers, | ||
| for: indexPath | ||
| ) as! SearchResultEmptyTitleCollectionViewCell | ||
| ) as! SearchResultEmptyCollectionViewCell | ||
|
|
There was a problem hiding this comment.
Replace force-cast with safe down-cast to prevent runtime crashes
SwiftLint already flags this line.
If the cell class ever changes in Interface Builder or the identifier is mistyped, the app will crash at runtime.
- let cell = collectionView.dequeueReusableCell(
- withReuseIdentifier: SearchResultEmptyCollectionViewCell.identifiers,
- for: indexPath
- ) as! SearchResultEmptyCollectionViewCell
+ guard let cell = collectionView.dequeueReusableCell(
+ withReuseIdentifier: SearchResultEmptyCollectionViewCell.identifiers,
+ for: indexPath
+ ) as? SearchResultEmptyCollectionViewCell else {
+ assertionFailure("Failed to dequeue SearchResultEmptyCollectionViewCell")
+ return nil
+ }Apply the same pattern to the other as! casts in this method to remove all force_cast warnings.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 216-216: Force casts should be avoided
(force_cast)
🤖 Prompt for AI Agents
In
Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/View/PopupSearchView.swift
around lines 212 to 217, replace the force-cast (as!) when dequeuing collection
view cells with a safe down-cast (as?) to prevent runtime crashes. Use optional
binding (if let or guard let) to safely unwrap the cell and handle the failure
case gracefully. Apply this pattern consistently to all force-casts in this
method to eliminate all force_cast warnings.
Comment on lines
+297
to
299
| func makeSearchResultEmpty(state: State) -> String? { | ||
| if !currentState.searchResultItems.isEmpty { return nil } else if currentState.isSearching { return "검색 결과가 없어요 :(\n다른 키워드로 검색해주세요" } else { return "검색 결과가 없어요 :(\n다른 옵션을 선택해주세요" } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Dead code – parameter state never used
Same issue as above: the parameter is never referenced, so Swift will emit a warning and the logic is misleading.
Once the fix above is applied, this discrepancy disappears.
🤖 Prompt for AI Agents
In
Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/Reactor/PopupSearchReactor.swift
around lines 297 to 299, the function makeSearchResultEmpty has an unused
parameter 'state' which causes a warning and confusion. Remove the 'state'
parameter from the function signature and update any calls to this function
accordingly to eliminate the dead code and clarify the logic.
Comment on lines
+201
to
203
| case .updateSearchResultSection: | ||
| newState.updateSearchResultSection = makeSearchResultEmpty(state: newState) | ||
|
|
There was a problem hiding this comment.
updateSearchResultSection uses stale state – empty/keyword messages may be wrong
reduce sets
newState.updateSearchResultSection = makeSearchResultEmpty(state: newState)However, makeSearchResultEmpty(state:) completely ignores its parameter and instead reads currentState, which still contains the old values.
Consequences:
- After mutations like
setupSearchResultthe empty-state message is evaluated against the previous snapshot. - The UI can show an out-of-date empty/keyword message.
Fix by using the argument:
- func makeSearchResultEmpty(state: State) -> String? {
- if !currentState.searchResultItems.isEmpty { return nil }
- else if currentState.isSearching { … }
- else { … }
- }
+ func makeSearchResultEmpty(state: State) -> String? {
+ if !state.searchResultItems.isEmpty { return nil }
+ else if state.isSearching {
+ return "검색 결과가 없어요 :(\n다른 키워드로 검색해주세요"
+ } else {
+ return "검색 결과가 없어요 :(\n다른 옵션을 선택해주세요"
+ }
+ }Remove the now-unused state parameter from the call sites if you prefer to access state directly.
🤖 Prompt for AI Agents
In
Poppool/PresentationLayer/SearchFeature/SearchFeature/PopupSearch/Reactor/PopupSearchReactor.swift
around lines 201 to 203, the call to makeSearchResultEmpty(state:) passes
newState but the function ignores this parameter and uses currentState instead,
causing stale data usage. Fix this by modifying makeSearchResultEmpty(state:) to
use the passed-in state parameter rather than currentState, ensuring it
evaluates the correct, updated state. Optionally, remove the state parameter
from all call sites if you prefer to always use the currentState inside the
function.
dongglehada
approved these changes
May 25, 2025
Member
dongglehada
left a comment
dongglehada
left a comment
There was a problem hiding this comment.
공유해주신 빌더 패턴을 적용하여 layout을 생성하는 protocol을 설계해주신것 같아서 좋습니다! 앞으로 계속 이 설계를 기반으로 발전시켜보면 좋을 것 같아요 :> 수고 많으셨습니다 !!
Member
There was a problem hiding this comment.
공유주신 코드리뷰의 역할을 하는 기능인가 보네요!!
| self.section = existingSection | ||
| } | ||
|
|
||
| @discardableResult |
Member
There was a problem hiding this comment.
컴파일러에서 경고 메시지를 이렇게 제한을 할 수도 있군요..!
Member
There was a problem hiding this comment.
protocol 을 분리하여 header를 정의하는 부분도 따로 채택할 수 있는 구조군요..!
zzangzzangguy
approved these changes
May 26, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
📌 이슈
✅ 작업 사항
🚀 테스트 방식
SearchFeature 데모를 실행하여 검색 화면이 이전과 동일한 레이아웃을 제공해주는지 테스트
👀 ETC (추후 개발해야 할 것, 참고자료 등)
새롭게 도입한 빌더 패턴은 기존의 Sectionable이 제공해주었던 CollectionLayout이 만들기 쉽지 않다는 문제점을 개선하기 위해 도입했던 부분에 충분히 공감하였기에 새롭게 제시하는 방식입니다.
현재 검색화면을 만드는데에는 충분히 커버를 해주고있지만, 팀원분들의 화면을 커버 가능한지 확인이 필요합니다.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
.gitignore업데이트.