Refactor (packages/web/src/components/share/part.tsx): Complex binary expression - #172
Open
parishijainn wants to merge 3 commits into
Open
Conversation
…CompletedAt Resolves Qlty smell at part.tsx:142. Adds unit tests covering all branches of the extracted condition.
Adds a test script to packages/web and registers @opencode-ai/web#test in turbo.json so the new part.tsx unit tests run in CI.
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
Link to the associated GitHub issue:
#126
Full path to the refactored file:
packages/web/src/components/share/part.tsx
What do you think this file does?
This file renders a single part of a chat message in the shared view of a session. It handles different message types like user text, assistant text, reasoning, file attachments, and tool calls, and shows the right UI for each one.
What is the scope of your refactoring within that file?
I refactored the JSX condition inside the Part component around line 142 that decides whether to show a completed at timestamp footer under an assistant's message.
Which Qlty reported issue did you address?
Complex binary expression at packages/web/src/components/share/part.tsx:142.
2. Refactoring
How did the specific issue you chose impact the codebase's maintainability?
The condition props.last && props.message.role === "assistant" && props.message.time.completed was written inline in the JSX and checked three things at once. It was also repeated in two spots, the footer's title and its content, so any change to this logic had to be made in two places.
What changes did you make to resolve the issue?
I moved the condition into a function called getCompletedAt that returns the completed timestamp if all three conditions are true, or undefined if not. The JSX now just checks completedAt() !== undefined, wrapped in a Solid createMemo so it stays reactive. This removes the duplication and puts the logic in one named function instead of being repeated inline.
How do your changes improve maintainability? Did you consider alternatives?
I thought about keeping it as a createMemo with the same inline condition inside it, but that leaves the complex expression itself unchanged so Qlty would still flag it. I also thought about using if statements directly in the JSX, but Solid doesn't support that in the render tree. Pulling it into a named function was the simplest option, and it also let me test the logic on its own without rendering the component.
3. Validation
How did you validate that the change is correct?
I added packages/web/test/part.test.ts with 4 tests that call getCompletedAt directly, covering the case where all conditions are true and one test for each condition failing on its own. I ran bun test in packages/web and all 4 pass. bun lint is clean for the file, and I re-ran qlty smells on part.tsx and confirmed the complex binary expression warning at line 142 is gone with no new smells. While writing the tests I found a bug in my own code, I had typed & instead of && between two conditions, which crashed on user messages since & doesn't short circuit. My test for that case failed and caught it right away.
Attach a screenshot of the test coverage showing the lines were executed by the tests.
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.
