Resolves packages/ui/src/theme/v2/mapping.ts line duplication - #42
Draft
ariaf7 wants to merge 3 commits into
Draft
Resolves packages/ui/src/theme/v2/mapping.ts line duplication#42ariaf7 wants to merge 3 commits into
ariaf7 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: Refactoring PR
Use this pull request template to briefly answer the questions below in one to two sentences each.
Feel free to delete this text at the top after filling out the template.
1. Issue
Link to the associated GitHub issue:
#20
Full path to the refactored file:
packages/ui/src/theme/v2/mapping.ts
What do you think this file does?
This file is responsible for mapping token colors for both the light and dark themes of the UI. It assigns colors for text, borders, states, etc, and returns the mapping depending on whether the app is in light or dark mode.
What is the scope of your refactoring within that file?
This refactoring joined the duplicate token definitions into shared dictionaries that store both the light and dark values, and then added logic that is in charge of choosing which values from the dictionary to assign to the variables depending on which mode is being used. The regions touched were the semantic token mappings, the agent token mappings, and the construction of the final light and dark mappings used by mapV2Semantics (mapV2Semantics was not changed though)
Which Qlty‑reported issue did you address?
Found 64 lines of similar code in 2 locations (mass = 405)
2. Refactoring
How did the specific issue you chose impact the codebase’s maintainability?
The issue was duplicated semantic token mapping, which made the file much longer than necessary, and would mean updating multiple sections if changing or adding a token needed to happen.
What changes did you make to resolve the issue?
I made one shared token structure to store both the light and dark values, and added logic to handle that combination.
How do your changes improve maintainability? Did you consider alternatives?
The refactor removes duplication which makes the values of the tokens easier to update, compare, and makes it less likely that one theme would be updated and not the other. I debated removing smaller chunks of the duplicated sections and leaving the rest of the of the structure the same, but that still meant a lot of the duplication would remain, which would defeat the purpose of the refactor.
3. Validation
How did you validate that the change is correct?



My test file mapping.test.ts compares the new mappings from mapV2Semantics against the literal original mappings from before the refactor. This ensures that the refactor kept the existing semantic token values for both light and dark themes correctly. There was also a 99.6% line coverage, with the lines that were not covered being lines I did not change, and that did not have tests beforehand.
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
bun test:
bun lint:
Attach a screenshot of


qlty smells --no-snippets <full/path/to/file.ts>showing fewer reported issues after the changes.Before:
After: