Skip to content

fix: [M3-9085] - Miscellaneous masked data bugs and enhancements - #12003

Merged
mjac0bs merged 12 commits into
linode:developfrom
mjac0bs:M3-9085-fix-misc-masked-data-bugs
Apr 14, 2025
Merged

fix: [M3-9085] - Miscellaneous masked data bugs and enhancements#12003
mjac0bs merged 12 commits into
linode:developfrom
mjac0bs:M3-9085-fix-misc-masked-data-bugs

Conversation

@mjac0bs

@mjac0bs mjac0bs commented Apr 9, 2025

Copy link
Copy Markdown
Contributor

Description 📝

This PR fixes a few known bugs:

  • In the LKE node pool node row, the visibility icon is unexpectedly small. Note the difference in size from the copy icon and the visibility icon up in the specs section of the cluster details page.
  • On the Longview Processes tab, the User column width is so narrow that the default length of masked dots wraps to the next line.
  • On the primary domain details page, we're not masking SOA email or A/AAAA record IP addresses when we should be.
  • On the IAM landing page, there is a console error about DOM nesting: <p> cannot appear as a descendant of <p> in MaskableText.

This PR also provides one enhancement, which allows the user to toggle the masked data visibility icon for IP addresses on the Linode details page without having to scroll when the addresses are too long for the component. This was especially visible with IPv6 and doing so required a little reworking of how the CopyTooltip was handling masking.

Changes 🔄

  • Allow the MaskableText length prop to be any number (in addition to predefined values)
  • Add MaskableText to the Email and IP Addresses fields on Domains Details page
  • Fix the nesting in the IAM UserRow
  • In the LKE node pool row, use a StyledCopyTooltip to resize that svg specifically without resizing the VisibilityIcon, which was a regression
  • Update the CopyTooltip props and logic to handle visibility being toggled in an uncontrolled (state managed within CopyTooltip, as before) or controlled (visibility tooltip state managed outside the CopyTooltip) way

Target release date 🗓️

4/22

Preview 📷

Before After
Screenshot 2025-04-09 at 1 11 38 PM Screenshot 2025-04-09 at 1 11 43 PM
Screenshot 2025-04-09 at 1 20 52 PM Screenshot 2025-04-09 at 1 20 37 PM
Screenshot 2025-04-09 at 1 19 59 PM Screenshot 2025-04-09 at 1 19 48 PM
Screenshot 2025-04-09 at 1 34 40 PM
Screen.Recording.2025-04-09.at.1.19.03.PM.mov
Screenshot 2025-04-09 at 1 17 05 PM Screenshot 2025-04-09 at 1 12 10 PM

How to test 🧪

Prerequisites

(How to setup test environment)

  • To see the Longview Processes table, you'll need Longview running on a Linode (Docs)
  • Enable the 'mask sensitive data' setting via your Profile settings

Reproduction steps

(How to reproduce the issue, if applicable)

  • Visit the places mentioned in the description to observe the issues in the Preview "Before" section

Verification steps

(How to verify changes)

  • LKE node pool node row: visibility icon is a consistent size and the copy tooltip size is not impacted
  • Longview Processes: the masked User data doesn't wrap in the column
  • Domain records: email and IP addresses are masked
  • No console error about invalid DOM nesting is fixed for MaskableText when it handles JSX children. The IAM UserRow was the only place where I saw this happening.
  • Linode details: IP addresses are now a standard length and the visibility icon displays next to the copy icon
  • Confirm that there are no regressions to other places the CopyTooltip is used (for example: the IP addresses in the Network tab)
Author Checklists

As an Author, to speed up the review process, I considered 🤔

👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@mjac0bs mjac0bs added the Bug Fixes for regressions or bugs label Apr 9, 2025
@mjac0bs mjac0bs self-assigned this Apr 9, 2025
/**
* Optionally specifies the length of the masked text to depending on data type (e.g. 'ipv4', 'ipv6', 'plaintext'); if not provided, will use a default length.
*/
maskedTextLength?: MaskableTextLength | number;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The change here was to add the number type to remain consistent with the change to MaskableText.tsx's length prop.

Comment on lines +28 to +31
* If true, the component is in controlled mode for text masking, meaning the parent component handles the visibility toggle.
* @default false
*/
masked?: boolean;
isMaskingControlled?: boolean;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

New prop to allow us to use a VisibilityIcon in a parent component to toggle the CopyTooltip's masked state. This was needed for the AccessRow IP addresses, where we have a Stack to group the StyledCopyIcon and the VisibilityIcon next to each other, visible even when there is a scroll bar.

* Optionally specifies the length of the masked text to depending on data type (e.g. 'ipv4', 'ipv6', 'plaintext'); if not provided, will use a default length.
*/
length?: MaskableTextLength;
length?: MaskableTextLength | number;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed the issue described for the Longview ProcessesTable.

@mjac0bs
mjac0bs marked this pull request as ready for review April 9, 2025 20:31
@mjac0bs
mjac0bs requested a review from a team as a code owner April 9, 2025 20:31
@mjac0bs
mjac0bs requested review from cpathipa and pmakode-akamai and removed request for a team April 9, 2025 20:31

@pmakode-akamai pmakode-akamai 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.

Looking good ✅

Comment thread packages/manager/src/features/Linodes/AccessTable.tsx Outdated
@pmakode-akamai pmakode-akamai added the Add'tl Approval Needed Waiting on another approval! label Apr 11, 2025
@github-project-automation github-project-automation Bot moved this from Review to Approved in Cloud Manager Apr 14, 2025
@cpathipa cpathipa added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Apr 14, 2025
@linode-gh-bot

Copy link
Copy Markdown

Cloud Manager UI test results

🎉 540 passing tests on test run #5 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing540 Passing4 Skipped110m 39s

@mjac0bs
mjac0bs merged commit 3688d82 into linode:develop Apr 14, 2025
@github-project-automation github-project-automation Bot moved this from Approved to Merged in Cloud Manager Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved Multiple approvals and ready to merge! Bug Fixes for regressions or bugs

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants