Repository navigation
refactor(article): WebView 분리 + trigger 패턴 적용 - #100
Conversation
- ArticleDetailView에서 FullWebView/HighlightableWebView → ArticleWebView.swift 분리 - FontSizeControlView/BorderedThumbSlider → FontSizeControlView.swift 분리 - webViewRef(@State WKWebView) 제거 → ArticleWebViewAction trigger 패턴 - fontSize UserDefaults 읽기/쓰기를 ViewModel didSet으로 이동 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors ArticleDetailView by extracting the web view and font size control components into separate files (ArticleWebView and FontSizeControlView) and moving the font size state to ArticleDetailViewModel. It also introduces a trigger pattern (ArticleWebViewAction) to handle web view actions instead of directly evaluating JavaScript. Feedback on the changes includes fixing a potential race condition in ArticleWebView when clearing pendingActions, preventing memory leaks by removing script message handlers in dismantleUIView, removing unused disableHighlight properties, and ensuring the Coordinator in BorderedThumbSlider does not hold a stale reference to its parent.
| if !pendingActions.isEmpty { | ||
| for action in pendingActions { | ||
| switch action { | ||
| case .scrollToTop: | ||
| uiView.scrollView.setContentOffset(.zero, animated: true) | ||
| case .scrollToHighlight(let text): | ||
| let script = ArticleHighlightJS.scrollToHighlightScript(text: text) | ||
| uiView.evaluateJavaScript(script, completionHandler: nil) | ||
| case .removeHighlight(let text): | ||
| let script = ArticleHighlightJS.removeHighlightScript(text: text) | ||
| uiView.evaluateJavaScript(script, completionHandler: nil) | ||
| } | ||
| } | ||
| DispatchQueue.main.async { self.pendingActions = [] } | ||
| } |
There was a problem hiding this comment.
pendingActions를 비동기적으로 비울 때 self.pendingActions = []와 같이 전체 배열을 초기화하면 레이스 컨디션(Race Condition)이 발생할 수 있습니다. updateUIView가 실행된 시점과 DispatchQueue.main.async 블록이 실행되는 시점 사이에 사용자의 새로운 액션이 추가되는 경우, 해당 새로운 액션이 유실될 위험이 있습니다. 대신 현재 처리한 액션의 개수만큼만 배열 앞부분에서 제거하는 방식(dropFirst)을 권장합니다.
| if !pendingActions.isEmpty { | |
| for action in pendingActions { | |
| switch action { | |
| case .scrollToTop: | |
| uiView.scrollView.setContentOffset(.zero, animated: true) | |
| case .scrollToHighlight(let text): | |
| let script = ArticleHighlightJS.scrollToHighlightScript(text: text) | |
| uiView.evaluateJavaScript(script, completionHandler: nil) | |
| case .removeHighlight(let text): | |
| let script = ArticleHighlightJS.removeHighlightScript(text: text) | |
| uiView.evaluateJavaScript(script, completionHandler: nil) | |
| } | |
| } | |
| DispatchQueue.main.async { self.pendingActions = [] } | |
| } | |
| if !pendingActions.isEmpty { | |
| let actions = pendingActions | |
| for action in actions { | |
| switch action { | |
| case .scrollToTop: | |
| uiView.scrollView.setContentOffset(.zero, animated: true) | |
| case .scrollToHighlight(let text): | |
| let script = ArticleHighlightJS.scrollToHighlightScript(text: text) | |
| uiView.evaluateJavaScript(script, completionHandler: nil) | |
| case .removeHighlight(let text): | |
| let script = ArticleHighlightJS.removeHighlightScript(text: text) | |
| uiView.evaluateJavaScript(script, completionHandler: nil) | |
| } | |
| } | |
| DispatchQueue.main.async { | |
| self.pendingActions = Array(self.pendingActions.dropFirst(actions.count)) | |
| } | |
| } |
| context.coordinator.lastFontSize = fontSize | ||
| uiView.evaluateJavaScript("adjustFontSize(\(fontSize));", completionHandler: nil) | ||
| } | ||
| } |
There was a problem hiding this comment.
WKUserContentController.add(_:name:)는 메시지 핸들러(여기서는 Coordinator)를 강하게 참조(Strong Reference)하므로 메모리 누수(Memory Leak)를 유발할 수 있습니다. UIViewRepresentable이 화면에서 사라질 때 이 참조를 해제할 수 있도록 dismantleUIView 메서드를 구현하여 등록된 스크립트 메시지 핸들러들을 제거해 주는 것이 안전합니다.
}
static func dismantleUIView(_ uiView: HighlightableWebView, coordinator: Coordinator) {
uiView.configuration.userContentController.removeScriptMessageHandler(forName: "textSelected")
uiView.configuration.userContentController.removeScriptMessageHandler(forName: "getSelectedText")
uiView.configuration.userContentController.removeScriptMessageHandler(forName: "consoleLog")
uiView.configuration.userContentController.removeScriptMessageHandler(forName: "highlightTypeChanged")
uiView.configuration.userContentController.removeScriptMessageHandler(forName: "highlightDeleted")
}| class HighlightableWebView: WKWebView { | ||
| var disableHighlight: Bool = false |
There was a problem hiding this comment.
HighlightableWebView 클래스 내의 disableHighlight 프로퍼티는 정의되어 있고 makeUIView에서 설정되지만, 실제 canPerformAction 메서드 내에서는 사용되지 않고 항상 false를 반환하고 있습니다. 불필요한 데드 코드(Dead Code)를 제거하여 코드를 더 깔끔하게 유지하는 것을 권장합니다.
| class HighlightableWebView: WKWebView { | |
| var disableHighlight: Bool = false | |
| class HighlightableWebView: WKWebView { |
| let webView = HighlightableWebView(frame: .zero, configuration: config) | ||
| webView.disableHighlight = disableHighlight | ||
| webView.uiDelegate = context.coordinator |
There was a problem hiding this comment.
HighlightableWebView에서 사용되지 않는 disableHighlight 프로퍼티 설정을 제거하여 코드를 간결하게 유지할 수 있습니다.
| let webView = HighlightableWebView(frame: .zero, configuration: config) | |
| webView.disableHighlight = disableHighlight | |
| webView.uiDelegate = context.coordinator | |
| let webView = HighlightableWebView(frame: .zero, configuration: config) | |
| webView.uiDelegate = context.coordinator |
| func updateUIView(_ uiView: UISlider, context: Context) { | ||
| let stepped = Float(round(value / step) * step) | ||
| if uiView.value != stepped { uiView.value = stepped } | ||
| } |
There was a problem hiding this comment.
BorderedThumbSlider 내의 updateUIView에서 context.coordinator.parent = self를 업데이트해 주지 않으면, Coordinator가 초기 생성 시점의 parent 구조체 복사본을 계속 참조하게 됩니다. 만약 step이나 range 같은 프로퍼티가 동적으로 변경될 경우, Coordinator 내에서 이전의 오래된(stale) 값을 참조하는 버그가 발생할 수 있습니다. updateUIView에서 parent를 최신 상태로 업데이트해 주는 것이 안전합니다.
| func updateUIView(_ uiView: UISlider, context: Context) { | |
| let stepped = Float(round(value / step) * step) | |
| if uiView.value != stepped { uiView.value = stepped } | |
| } | |
| func updateUIView(_ uiView: UISlider, context: Context) { | |
| context.coordinator.parent = self | |
| let stepped = Float(round(value / step) * step) | |
| if uiView.value != stepped { uiView.value = stepped } | |
| } |
| class Coordinator: NSObject { | ||
| let parent: BorderedThumbSlider | ||
| init(_ parent: BorderedThumbSlider) { self.parent = parent } |
There was a problem hiding this comment.
Coordinator 내의 parent 프로퍼티를 var로 변경하여 updateUIView에서 최신 parent 인스턴스를 전달받을 수 있도록 개선하는 것을 권장합니다.
| class Coordinator: NSObject { | |
| let parent: BorderedThumbSlider | |
| init(_ parent: BorderedThumbSlider) { self.parent = parent } | |
| class Coordinator: NSObject { | |
| var parent: BorderedThumbSlider | |
| init(_ parent: BorderedThumbSlider) { self.parent = parent } |
Summary
ArticleDetailView.swift618줄 → 170줄로 축소@State webViewRef: WKWebView?제거 → action trigger 패턴으로 대체Changes
파일 분리
Component/ArticleWebView.swift— HighlightableWebView + ArticleWebView + CoordinatorComponent/FontSizeControlView.swift— FontSizeControlView + BorderedThumbSlider + TrackSliderwebViewRef 제거
fontSize ViewModel 이동
Test plan
🤖 Generated with Claude Code