Skip to content

Refactor notifyShadowReady and add unit tests - #140

Closed
jannalu wants to merge 3 commits into
CMU-313:mainfrom
jannalu:main
Closed

jannalu wants to merge 3 commits into
CMU-313:mainfrom
jannalu:main

Conversation

@jannalu

@jannalu jannalu commented Sep 7, 2026

Copy link
Copy Markdown

P1B: Starter Task: Refactoring PR

1. Issue

Link to the associated GitHub issue: #25

Full path to the refactored file: packages/session-ui/src/pierre/file-runtime.ts

What do you think this file does?
I think that this file helps manages the file viewer's Shadow DOM, including finding the viewer, syncing its color scheme, and detecting when the viewer is ready to use.

What is the scope of your refactoring within that file?
I only refactored the notifyShadowReady function in file-runtime.ts. The other helper functions were left unchanged. I also added a new file-runtime.test.ts file to test the behavior of notifyShadowReady.

Which Qlty‑reported issue did you address?
I addressed Qlty's "Function with many returns" code smell in notifyShadowReady, which reported 10 return statements before the refactor.

2. Refactoring

How did the specific issue you chose impact the codebase’s maintainability?
Having many return statements made the control flow of notifyShadowReady more difficult to follow. This could make future changes cause errors due to the many different exit paths through the function.

What changes did you make to resolve the issue?
I refactored notifyShadowReady to reduce the number of early return statements, replacing the early returns with if/else conditions. I also added unit tests to verify its behavior.

How do your changes improve maintainability? Did you consider alternatives?
The refactor makes the control flow easier to understand and reduces the number of exit paths that developers need to reason about. I considered keeping the existing structure, but reorganizing the logic provided a cleaner solution while the added tests help ensure that the behavior remains correct.

3. Validation

How did you validate that the change is correct?
I added 4 tests that check the main behaviors of notifyShadowReady. I also ran the packages test suite and got 87 passes and 0 failures. Finally, I reran bun lint and Qlty smells to confirm there were no lint errors and that the original code smell was removed.

Attach a screenshot of the test coverage showing the lines were executed by the tests.

bun coverage
Screenshot 2026-09-07 at 4 27 50 PM

The uncovered lines are code that was unaffected by the refactoring of notifyShadowReady (lines 51-119)
Screenshot 2026-09-07 at 4 28 40 PM

Attach a screenshot showing the tests that cover the change passing during CI
Screenshot 2026-09-07 at 4 54 55 PM

bun test
Screenshot 2026-09-07 at 4 55 19 PM

bun lint
Screenshot 2026-09-07 at 6 02 28 PM
No new errors/warnings are introduced due to my changes

Attach a screenshot of showing fewer reported issues after the changes.
qlty smells --no-snippets packages/session-ui/src/pierre/file-runtime.ts
Before refactoring:
Screenshot 2026-09-07 at 4 31 21 PM

After refactoring:
Screenshot 2026-09-07 at 4 31 45 PM

@jannalu jannalu closed this Sep 7, 2026
@jannalu

jannalu commented Sep 7, 2026

Copy link
Copy Markdown
Author

Closing this PR because it was opened from the wrong compare branch. A new PR will be created from fix-code-smell.

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.

1 participant