Refactor (packages/web/src/components/Share.tsx): Function with high complexity (count = 136): Share - #67
Open
EvuhLi wants to merge 6 commits into
Open
Refactor (packages/web/src/components/Share.tsx): Function with high complexity (count = 136): Share#67EvuhLi wants to merge 6 commits into
EvuhLi wants to merge 6 commits into
Conversation
The filteredParts predicate was an anonymous closure nested three levels deep inside Share (For -> filter callback), which qlty attributed to Share's own complexity/return-count score. Hoisting it into a standalone function is behavior-preserving and lets the smells report attribute its 8 early returns to the predicate itself.
The data() memo's cost/token/model reducer loop was inlined directly inside Share, adding to its complexity score. Extracting it into a standalone summarizeSession(info, msgs) function is behavior- preserving (same accumulation logic, iterated with for-of instead of an indexed loop since the index was never used).
…tton
The floating scroll-to-bottom button's signals, IntersectionObserver
sentinel, checkScrollNeed logic, and its onMount/onCleanup pair were
all inlined in Share, driving most of its remaining complexity.
useScrollButton() encapsulates the same logic unchanged and returns
{ visible, scrollToBottom, onMouseEnter, onMouseLeave } for the JSX
to wire up directly.
Exports the two functions extracted from Share out of Share.tsx and adds unit tests exercising every branch: each isVisiblePart filter rule (plus a realistic mixed-part integration case) and summarizeSession's cost/ token/model aggregation, last-write-wins fields, and edge cases around missing session info or messages.
…seScrollButton useScrollButton bundled its scroll-visibility decision and repeated "clear this if it's set" cleanup checks inside one closure, which Qlty flagged as high complexity (28) after the earlier Share extractions moved it there. Pulling the pure show/hide predicate and a shared disposeIfSet(value, dispose) helper out to top-level functions drops useScrollButton out of the Qlty report entirely (file total complexity 144 -> 126) without introducing a new flagged function. Behavior is unchanged; only where the branches live changed.
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.
P1B: Starter Task: Refactoring PR
1. Issue
#44
/opencode/packages/web/src/components/Share.tsx
What do you think this file does?
Share.tsx opens up a socket to handle incoming messages using the OpenCode chat. When it receives an input from the user, it checks if the type is info, message, or part, and handles it accordingly. The file renders the connection and displays any content that is marked as isVisiblepart. It also handles closing the websocket.
What is the scope of your refactoring within that file?
My refactoring in this file is done within the Share component and the useScrollButton. I moved isVisiblePart, summarizeSessionInfo, shouldShowScrollButton, and disposeIfSet into their own independently testable functions, outside of the main Share component. This greatly reduced the complexity of the share component. All four of these have their behavior preserved.
Which Qlty‑reported issue did you address?
Function with high complexity (count = 136): Share
2. Refactoring
How did the specific issue you chose impact the codebase’s maintainability?
The specific issue I chose was impacting the codebases's maintainability because Share had high complexity. This was because there were four complex systems all within Share's own body, which could be extracted. This was an issue because none of these systems could be unit tested in isolation, since they were all bundled in the same function, and editing one of these could potentially break another function in the component. The body of the share function was also extremely long and difficult to step through.
What changes did you make to resolve the issue?
In order to resolve this issue, I extracted the four pieces into their own standalone functions. I preserved the behavior, but relocated the logic.
I moved isVisiblePart out of the callback inside Share's render loop into its own function. In Share, it now calls msg.parts.filter(isVisiblePart).
I moved summarizeSession out of the data() memo, which looped through messages in Share. Now, Share calls on summarizeSession(store.info, messages()) to achieve the same functionality.
I moved the show/hide logic in shouldShowScrollButton out of the component.
I also moved disposeIfSet, which was a method to cleanup any remaining components. Originally, this was five repeated if statements, and I extracted them into a single helper.
How do your changes improve maintainability? Did you consider alternatives?
Share's complexity dropped significantly, because the branching logic is now separated into 4 independent functions, promoting abstraction and clean code. All four of the functions have direct test coverage as well, which wasn't possible before because they were all nested within the main component. I considered splitting the Share component into sub components, like a MessageList component, isntead of extracting the functions. However, this could potentially impact styling and props, and required a much more significant change the could introduce bugs in the logic.
3. Validation
How did you validate that the change is correct?
I ran both bun lint and bun test. I also created tests for all of the extracted functions. The tests effectively covered every line of the four extracted functions and 29/29 tests passed.
Attach a screenshot showing the tests that cover the change passing during CI
Attach a screenshot of
qlty smells --no-snippets <full/path/to/file.ts>showing fewer reported issues after the changes.