Refactor (packages/ui/src/v2/components/text-shimmer-v2.tsx): Dedupe text shimmer logic - #111
Open
GioChkhaidze wants to merge 3 commits into
Open
GioChkhaidze wants to merge 3 commits into
GioChkhaidze wants to merge 3 commits into
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
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.
P1B: Starter Task: Dedupe text shimmer logic
1. Issue
Link: Closes #102
Full Path: packages/ui/src/v2/components/text-shimmer-v2.tsx, packages/ui/src/components/text-shimmer.tsx and newly added packages/ui/src/components/text-shimmer-core.tsx and its test.
What file does: it's a responsible for SolidJS component rendering for text rendering.
Scope: replaced TextShimmerV2 & TextShimmer components with wrapper which has shared core logic from both of them.
Qlty-report: Found 59 lines of similar code in 2 locations (mass = 290) at text-shimmer-v2.tsx:5 in components/text-shimmer.tsx
2. Refactoring
Maintainability: Each update must have been applied to both shimmers and might lead to drift, so with this fix I centralized logic so that it's easier to maintain (boths shimmers are live in session-turn and in basic-tool-v2).
Change: Extracted shared logic from both shimmers to single core shared code and reused it in both (centralized the logic). TextShimmerCore now owns logic and TextShimmer and TextShimmerV2 both reuse it.
Alternative: TextShimmerCore acts as a source of truth and is better for maintainability in long term. I considered removing v1 and using v2 to wrap v1 which would be smaller code change but rather making the codebase dirty in terms of understanding why one version reuses second one. Another reason why my approach is better is that it is cleaner foundation for newer future versions of shimmer reusing the same logic and acting as a source of truth.
3. Validation
Validation: I ran,
bun run typecheck,qlty smells --no-snippets(on both wrappers reporting no deduplication), andbun test --coverage --coverage-dir=./coverageshowing 33 tests pass (27 + 6 new test) in opencode/packages/ui. The new test-shimmer-core.test.ts imported helpers and configs and checked the mappings, v1 & v2 selectors, executing shared path for both wrappers.The test coverage showing the lines were executed by the tests.
Lints, typechecks and tests were executed successfully.
CI checks pass
My additional tests (27 + 6 new tests all pass in github actions CI in packages/ui)
qlty smells --no-snippets packages/ui/src/v2/components/text-shimmer-v2.tsx showing issues BEFORE changes.
qlty smells --no-snippets packages/ui/src/v2/components/text-shimmer-v2.tsx showing no reported issues AFTER changes.