Skip to content

APS Icon Fixes (Safer SVG Rendering) - #1345

Merged
Dhruv-0-Arora merged 5 commits into
darora1/142/desktop-ui-redesignfrom
alexey/claude-aps-icon-fixes
Jun 26, 2026
Merged

APS Icon Fixes (Safer SVG Rendering)#1345
Dhruv-0-Arora merged 5 commits into
darora1/142/desktop-ui-redesignfrom
alexey/claude-aps-icon-fixes

Conversation

@AlexD717

@AlexD717 AlexD717 commented Jun 25, 2026

Copy link
Copy Markdown
Member

Symptom

Logging in with APS caused some top bar icons to become invisible.

Solution

Reworked top bar icon rendering to keep inline SVG behavior while using a safer implementation:

  • Removed dangerouslySetInnerHTML usage in TopBarIcons.tsx.
  • Kept SVG inlining from local icon assets, but now parse SVG markup with DOMParser and append cloned SVG nodes.
  • Preserved SVG ID scoping to avoid cross-icon ID collisions.

Verification

  • Top bar icons render as inline SVG (not <img>).
  • Multiple icon instances render correctly without collisions.
  • Added targeted test coverage in src/test/ui/TopBarIcons.test.tsx.
  • Logging in with APS causes no icons to disappear.

Before merging, ensure the following criteria are met:

  • All acceptance criteria outlined in the ticket are met.
  • Necessary test cases have been added and updated.
  • A feature toggle or safe disable path has been added (if applicable).
  • User-facing polish:
    • Ask: "Is this ready-looking?"
  • Cross-linking between Jira and GitHub:
    • PR links to the relevant Jira issue.
    • Jira ticket has a comment referencing this PR.

@Dhruv-0-Arora
Dhruv-0-Arora marked this pull request as ready for review June 25, 2026 00:03
@Dhruv-0-Arora
Dhruv-0-Arora requested review from a team as code owners June 25, 2026 00:03
@Dhruv-0-Arora
Dhruv-0-Arora requested review from PepperLola, RoushilS and elliefre and removed request for a team June 25, 2026 00:03
@autodesk-chorus

Copy link
Copy Markdown

Chorus detected one or more security issues with this pull request. See the Checks tab for more details.

As a reminder, please follow the secure code review process as part of the Secure Coding Trust Commitment requirement.

@azaleacolburn azaleacolburn 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.

I think we need to talk about these changes before merging, as the solution seems pretty scuffed and I don't fully believe everything Claude wrote in the TopBarIcons.tsx comment.

Also, the usage of dangerouslySetHtml is questionable, even though it doesn't use user input.

@Dhruv-0-Arora

Copy link
Copy Markdown
Collaborator

@copilot what are your thoughts on the changes made in TopBarIcons.tsx
Do you think that there is a better and safer way to implement that fix?

Copilot AI commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

@copilot what are your thoughts on the changes made in TopBarIcons.tsx
Do you think that there is a better and safer way to implement that fix?

Yes — I replaced the dangerouslySetInnerHTML approach with a safer DOMParser + cloned-<svg> rendering path in TopBarIcons.tsx (commit 1d3a51a). This keeps inline SVG behavior (so icons stay visible) while removing direct HTML injection. I also added focused tests in 9bb35b5 to verify inline SVG rendering and multi-instance behavior.

Copilot AI changed the title APS Icon Fixes APS Icon Fixes (Safer SVG Rendering) Jun 25, 2026
Copilot AI requested a review from Dhruv-0-Arora June 25, 2026 00:35
@Dhruv-0-Arora
Dhruv-0-Arora marked this pull request as draft June 25, 2026 00:40
@Dhruv-0-Arora

Copy link
Copy Markdown
Collaborator

@AlexD717 could you take a look again and see if this still fixes it for you?

@AlexD717

Copy link
Copy Markdown
Member Author

@Dhruv-0-Arora Yes, this also fixed it for me.

@Dhruv-0-Arora
Dhruv-0-Arora marked this pull request as ready for review June 25, 2026 16:52
@Dhruv-0-Arora
Dhruv-0-Arora merged commit 37235a6 into darora1/142/desktop-ui-redesign Jun 26, 2026
10 checks passed
@Dhruv-0-Arora
Dhruv-0-Arora deleted the alexey/claude-aps-icon-fixes branch June 26, 2026 01:32
@PepperLola PepperLola mentioned this pull request Jul 16, 2026
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.

4 participants