Skip to content

Refactor (packages/ui/src/theme/v2/mapping.ts:34): Found 64 lines of similar code in 2 locations - #66

Open
relghali wants to merge 4 commits into
CMU-17313Q:mainfrom
relghali:refactor-v2-mapping-duplication
Open

Refactor (packages/ui/src/theme/v2/mapping.ts:34): Found 64 lines of similar code in 2 locations#66
relghali wants to merge 4 commits into
CMU-17313Q:mainfrom
relghali:refactor-v2-mapping-duplication

Conversation

@relghali

@relghali relghali commented Sep 4, 2026

Copy link
Copy Markdown

Issue for this PR

Closes #60

Type of change

  • Refactor / code improvement

What does this PR do?

Initially, Qlty had flagged 64 lines to have similar code in two locations in the 'packages/ui/src/theme/v2/mapping.ts' file. The 'light' and 'dark' token tables had the same exact keys in the same order and only differed in their values. This meant every new token had to be added twice, which is what caused duplicated code.

To fix this, I merged each pair of tokens into a single table of ['light', 'dark'] tuples, then used the helper function 'pick' to construct the 'light' and 'dark' with less repetition by indexing sets of tokens with 0 for 'light' and 1 for 'dark'. This works because the token order is unchanged and every value is the same, it is just the tables are reorganized so that each token is written once.

This PR also includes my  .devcontainer changes, since the bun image doesn't come with curl, git, or xz-utils (which Qlty installer needs)

How did you verify your code works?

There was actually no tests for this file at all and it didn't show up in the coverage report, so I added 'packages/ui/src/theme/v2/mapping.test.ts' to test the changes. It checks the exact values (from the original file) for both themes, covers each kind of value in the file (e.g. var() refs, hex, rgba(), etc.), and that 'light' and 'dark' contain the same tokens just as the original did.

To make sure I did not accidentally mistype/miscopy a value while fixing the code, I dumped the full output of mapV2Semantics(false) and mapV2Semantics(true) to JSON before and after the change and diffed them, and found the diff to be empty.

I also made sure these tests worked by writing incorrect code (e.g. swapping a 'dark' value with a 'light' value) and having the tests prove that there is a failure.

bun lint and bun test pass locally. The CI config already runs packages/ui's tests and the test script picks up everything under src, so the new test file runs on this branch without any CI changes.

Screenshots / recordings

Note that while I changed a file in the UI, there was no UI changed as all the token values remained the exact same, I simply re-structured the code to do the same thing.

Evidences:

  1. Test Coverage Results
image (mapping.ts did not appear in the coverage report before this PR, it is now covered by mapping.test.ts.)
  1. Qlty before & after

Before:
image

After:
Screenshot 2026-09-04 204804

  1. Bun lint passing
image
  1. Test output passing
image

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

P1B: Refactor (packages/ui/src/theme/v2/mapping.ts:34): Found 64 lines of similar code in 2 locations (mass = 405)

1 participant