Skip to content

[PLAT-9142] Sonar issue cleanup - #832

Merged
jdm717 merged 8 commits into
masterfrom
sonar
Aug 5, 2026
Merged

[PLAT-9142] Sonar issue cleanup#832
jdm717 merged 8 commits into
masterfrom
sonar

Conversation

@jdm717

@jdm717 jdm717 commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Currently, Sonar analysis of the master branch is failing due to a number of miscellaneous existing issues. This PR attempts to address most or all of the issues causing some of the categories to be below the level needed to pass.

Test Plan

  • Unit tests / branch checks on this PR
  • Verify Sonar checks pass on this PR
  • Verify all workflow steps continue to work as expected (some script changes + workflow changes, but should all be fairly minor)
  • Verify Sonar checks pass on the master branch after merging

Release Notes

N/A

Possible Regressions

Github workflows + scripts

Dependencies

N/A

@jdm717 jdm717 changed the title Sonar fixes [PLAT-9142] Sonar issue cleanup Aug 5, 2026
@jdm717
jdm717 marked this pull request as ready for review August 5, 2026 14:53
@jdm717
jdm717 requested a review from a team as a code owner August 5, 2026 14:53

@amvertex amvertex left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing all of these!

</head>
<body>
<vertex-viewer id="viewer" class="viewer">
<vertex-viewer-toolbar data-viewer="viewer">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not need the toolbar anymore?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope! I've updated this to show the controls outside the viewer - the toolbar was really only used here for positioning, but taking a look at the example, these felt like they fit outside the viewer better anyway

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I like how it looks now.

Comment thread packages/viewer/src/components/scene-tree/lib/controller.ts Outdated
@sonarqubecloud

sonarqubecloud Bot commented Aug 5, 2026

Copy link
Copy Markdown

@aeischeid aeischeid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanups! Looks ready to go :shipit:

run: corepack enable
- name: "Install"
run: "yarn install"
run: "yarn install --ignore-scripts"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably should have noticed and added this when I upgraded Lerna a bit ago

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely an easy one to miss! The yarn install commands don't often get modified in PRs, so Sonar wasn't adding warnings to PRs frequently, and since the failure shows up post-merge, I've been missing this one as well 😅

@jdm717
jdm717 merged commit a49d9af into master Aug 5, 2026
8 checks passed
@jdm717
jdm717 deleted the sonar branch August 5, 2026 15:57
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.

3 participants