Skip to content

fix(ui): show avatar initials for single-segment usernames; tooltip truncated chart names#577

Open
eliran-ops wants to merge 5 commits into
mainfrom
feat/fix-cosmetic-batch-avatar-stop-claude-truncation-tooltips
Open

fix(ui): show avatar initials for single-segment usernames; tooltip truncated chart names#577
eliran-ops wants to merge 5 commits into
mainfrom
feat/fix-cosmetic-batch-avatar-stop-claude-truncation-tooltips

Conversation

@eliran-ops
Copy link
Copy Markdown
Contributor

@eliran-ops eliran-ops commented Apr 29, 2026

Summary

SKY-825 cosmetic batch:

  • 41 ✅ — Account menu avatar showed a generic silhouette while another circle in the header (radar-hub-web's avatar) showed correctly-computed initials.
  • 43 ✅ — Long chart names in the Helm catalogue were truncated with no way to read the full string.
  • 42 ⛔ Out of scope — the "Stop Claude" floating pill is the Cursor IDE agent UI, not radar.
  • 60 ⛔ Out of scope — the resource toolbar `+` button `onClick` is wired to `handleCreateResource` and `` renders correctly in radar OSS; could not reproduce.

Fixes

Bug 41 — silhouette avatar

The previous `UserMenu` initials computation only looked at separator-split segments of the username's local-part, so a username like `mkohli` (no `.`, `_`, `-`) produced no initials and the silhouette fallback kicked in.

Extracted `computeUserInitials` into `packages/k8s-ui/src/utils/user-initials.ts` with a clearer rule:

  • 2+ segments → first letter of each (max 2). `mary.kohli` → `MK`
  • otherwise → first 1-2 letters of the whole local-part. `mkohli` → `MK`
  • always uppercased; returns `''` for empty/null inputs

9 unit tests pin the contract including the bug 41 fixture verbatim, separator variants, the 2-cap, the @-domain stripping, edge cases (consecutive separators, single-character, empty/null).

Bug 43 — truncated chart names

Wrapped both `ChartBrowser` card variants' `

` in `` so users can hover to reveal the full name. Reuses the existing Tooltip primitive (itself fixed in PR #570 to anchor correctly and dismiss on click).

Test plan

  • `cd packages/k8s-ui && npm test` — 77 passing (9 new for `computeUserInitials`)
  • `cd web && npm run tsc` — clean
  • `cd web && npm run build` — clean

Made with Cursor


Note

Low Risk
Low risk UI/utility changes: adds a small pure helper for initials and adjusts tooltip class merging, with unit tests to pin behavior.

Overview
Fixes two UI papercuts: user avatar initials now come from a shared computeUserInitials helper that handles single-segment usernames/emails and filters non-letter glyphs, and the Helm ChartBrowser now wraps truncated chart titles in Tooltip so the full name is visible on hover.

Updates Tooltip to use tailwind-merge when combining its default wrapper classes with wrapperClassName, allowing callers (e.g. chart cards) to override display utilities correctly; adds targeted unit tests for both the tooltip class-merge contract and the new initials helper.

Reviewed by Cursor Bugbot for commit 7175703. Bugbot is set up for automated code reviews on this repo. Configure here.

Comment thread web/src/components/helm/ChartBrowser.tsx
Comment thread packages/k8s-ui/src/utils/user-initials.ts Outdated
@nadaverell
Copy link
Copy Markdown
Contributor

🤖 This review is the output of /pr-review-toolkit:review-pr — four Claude agents (code review, test coverage, comment accuracy, silent failures) on the diff. I've triaged their findings and stripped the noise; what's below is what survived.

Worth fixing before merge

1. Docstring contradicts the implementation. computeUserInitials.ts:16-17 claims it returns '' for "inputs with no usable letters." It doesn't:

  • computeUserInitials('..')'..'
  • computeUserInitials('123')'12'
  • computeUserInitials('_')'_'

Plus the rule list says "if the local-part contains separators, use the first letter of each segment" — but the code branches on segments.length >= 2 after filter(Boolean). So 'mary.' returns 'MA', not 'M'. The inline comment at the if is right; only the top-of-file docstring is wrong.

Fix either way: tighten the impl with a final letter-check (if (!/[a-z]/i.test(initials)) return '') or rewrite the docstring to match reality. Add one test pinning whichever you pick.

2. Whitespace bug. computeUserInitials(' alice') returns ' ' (two uppercased spaces — truthy). The UserMenu fallback {initials || <silhouette>} then renders two blank chars instead of falling back. Trim the local-part: username.trim().split('@')[0].

3. The twMerge override claim has no test. The whole point of the second commit is "callers can override inline-flex with block." But every existing test passes if you revert twMerge(clsx(...)) back to clsx(...). The regression only surfaces visually in ChartBrowser. Five-line render test in Tooltip.test.tsx closes it — render with wrapperClassName="block", assert the className resolves to block max-w-full (no inline-flex).

Style nits (same as #579)

4. (SKY-825 bug 41/43) refs across 5 inline spots and the test file's preamble — including the '... (the SKY-825 bug 41 fix)' test name. Same rule from CLAUDE.md as #579: ticket context belongs in the PR description, not in code that outlives the ticket.

5. The docstring's last paragraph references radar-hub-web's avatar. This util lives in @skyhook-io/k8s-ui and is consumed cross-repo. Embedding the rationale in the docstring will rot the moment radar-hub-web's header changes. One-liner WHY is enough: "Single-segment usernames must still produce a 2-letter label so avatars don't degrade to a generic icon."

Considered but skipping

  • Tooltip comment's "stylesheet ordering wins" framing — agent argued mechanism is "twMerge removes the conflicting utility." Substantively right but pedantic; the comment reads clearly enough.
  • wrapperClassName="min-w-0 flex-1" vs "block" inconsistency between the two ChartBrowser cards — both work; parent contexts may differ.
  • New user-initials.ts file vs colocating in format.ts — defensible either way, not worth churning.

Net: docstring/impl coherence + whitespace fix can be one commit; twMerge test is its own; ticket-ref cleanup is mechanical. Otherwise the PR is in good shape — twMerge swap was verified to regress nothing, peer-dep call is right, the substantive Tailwind comment is keeper material.

eliran-ops pushed a commit that referenced this pull request Apr 30, 2026
Address PR-review-toolkit findings on PR #577.

The v2 docstring promised:
  - returns '' for inputs with no usable letters
  - if local-part contains separators, use the first letter of each
    segment

The v2 implementation didn't honour either:
  - computeUserInitials('..')  → '..'   (returned the raw localPart)
  - computeUserInitials('123') → '12'   (digits leaked through)
  - computeUserInitials('_')   → '_'    (separator leaked through)
  - computeUserInitials('.user') → '.U' (separator leaked through)
  - computeUserInitials('mary.') → 'MA' (whole-localPart fallback
                                          ignored the separator
                                          contract for single-segment)

Rewrite to actually match the contract:
  1. Strip @-domain.
  2. Split on ./_/-, then drop non-letters per segment.
  3. 0 segments after cleanup → '' (caller draws silhouette).
  4. >= 2 → first letter of first 2 segments, uppercased.
  5. 1   → first 1-2 letters of THAT segment.

Tests pin all five v2 leaks plus digit / interleaved-punct cases.

Also strip the SKY-825 / bug-43 trailer from ChartBrowser per
CLAUDE.md (Tooltip already uses twMerge so the underlying
Tailwind-merge concern is already handled in the component).

Made-with: Cursor
@eliran-ops
Copy link
Copy Markdown
Contributor Author

@nadaverell @hisco — ready for review. Cursor bugbot found 2 issues: tailwind class conflict on Tooltip wrapper and avatar initials including separator chars. Both addressed: 75a43ae (twMerge for tooltip wrapper) and 0ab023e (drop non-letters in single-segment fallback). Cursor Bugbot SUCCESS on current HEAD.

@eliran-ops
Copy link
Copy Markdown
Contributor Author

Visual test

Tested locally on gke_koalabackend_us-east1-b_nonprod-cluster-us-east1.

Bug 43 - long chart name tooltip ✓

Helm Catalog → hovered `prometheus-yet-another-cloudwatch-exporter` (40-char name). Tooltip renders above the card with the full name (shown even when the card itself manages to fit it - the tooltip is the truncation safety-net at narrower viewports):

tooltip

DOM check: the <h4 class="...truncate"> is wrapped in <span class="inline-flex max-w-full min-w-0 flex-1"> (the Tooltip primitive), and tailwind-merge lets the caller's flex-1 override the default inline-flex. No layout regressions on the card grid.

Bug 41 - avatar initials computation

Could not exercise - radar OSS launched here has no authenticated UserMenu rendering (/home shows no avatar in the header). The computeUserInitials extraction lands the helper into shared utils with 9 unit tests pinning the mkohli → MK fixture; verifiable end-to-end only in radar-hub-web where the menu is shown.

Verdict

Bug 43 visually verified. Bug 41 helper + tests are correct by inspection; needs a follow-up check in cloud where the avatar actually renders.

eliran-mic added 3 commits May 6, 2026 13:34
…runcated chart names

SKY-825 cosmetic batch — addresses bugs 41 and 43 from the survey;
bugs 42 (Stop-Claude pill overlap — that pill is the Cursor IDE
agent UI, not radar) and 60 (no-op '+' button — could not reproduce
in OSS, the create-resource handler is wired and the dialog
renders) are out of scope here.

41. Account menu avatar showed a generic silhouette while another
    circle in the header (radar-hub-web's own avatar) showed
    correctly-computed initials — duplicated identity affordance.
    Root cause: the initials computation only looked at separator-
    split segments of the username's local-part, so a username like
    "mkohli" (no '.', '_', '-') produced no initials and the
    silhouette fallback kicked in.

    Fix: extract `computeUserInitials` into
    `packages/k8s-ui/src/utils/user-initials.ts`. New rule: if the
    local-part splits into 2+ segments, use the first letter of each
    (max 2). Otherwise fall back to the leading 1-2 letters of the
    whole local-part. Always uppercased. 9 unit tests pin the
    contract including the bug 41 fixture verbatim.

43. Long chart names (e.g. "super-long-chart-nam…") in the Helm
    catalogue were truncated with no way to read the full string.

    Fix: wrap both card variants' `<h4 truncate>` in `<Tooltip
    content={chart.name}>` so the user can hover to reveal the full
    name. Reuses the existing Tooltip primitive (which itself was
    fixed in PR #570 to anchor correctly and dismiss on click).

Linear: SKY-825
Made-with: Cursor
Cursor Bugbot caught: the `Tooltip` wrapper used `clsx('inline-flex
max-w-full', wrapperClassName)`. When a caller passed a conflicting
display utility (e.g. `wrapperClassName="block"` from ChartBrowser,
which needs the wrapper to fill its `flex-1 min-w-0` parent so the
child's `truncate` triggers), `clsx` simply concatenated and Tailwind's
generated stylesheet ordering — not className order — decided which
display won. `inline-flex` always won, and `block` was silently
dropped, leaving truncated chart names without their tooltip.

Fix:

- Add `tailwind-merge` as a peerDep of @skyhook-io/k8s-ui (web
  already has it as a dep).
- Wrap the wrapper className in `twMerge(clsx(...))`. tailwind-merge
  is Tailwind-aware and resolves utility-group conflicts in favour
  of the later (caller-supplied) value.

No call-site changes — `wrapperClassName="block"` in ChartBrowser
now actually overrides the default.

Made-with: Cursor
Address PR-review-toolkit findings on PR #577.

The v2 docstring promised:
  - returns '' for inputs with no usable letters
  - if local-part contains separators, use the first letter of each
    segment

The v2 implementation didn't honour either:
  - computeUserInitials('..')  → '..'   (returned the raw localPart)
  - computeUserInitials('123') → '12'   (digits leaked through)
  - computeUserInitials('_')   → '_'    (separator leaked through)
  - computeUserInitials('.user') → '.U' (separator leaked through)
  - computeUserInitials('mary.') → 'MA' (whole-localPart fallback
                                          ignored the separator
                                          contract for single-segment)

Rewrite to actually match the contract:
  1. Strip @-domain.
  2. Split on ./_/-, then drop non-letters per segment.
  3. 0 segments after cleanup → '' (caller draws silhouette).
  4. >= 2 → first letter of first 2 segments, uppercased.
  5. 1   → first 1-2 letters of THAT segment.

Tests pin all five v2 leaks plus digit / interleaved-punct cases.

Also strip the SKY-825 / bug-43 trailer from ChartBrowser per
CLAUDE.md (Tooltip already uses twMerge so the underlying
Tailwind-merge concern is already handled in the component).

Made-with: Cursor
@eliran-ops eliran-ops force-pushed the feat/fix-cosmetic-batch-avatar-stop-claude-truncation-tooltips branch from 0ab023e to a760e41 Compare May 6, 2026 10:35
@cursor
Copy link
Copy Markdown

cursor Bot commented May 6, 2026

You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace.

To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard.

Lock down two regression vectors flagged on PR #577:

- Tooltip wrapperClassName: assert that a caller-supplied display
  utility wins over the default `inline-flex`. Without twMerge the
  wrapper silently kept inline-flex and ChartBrowser truncation
  stopped engaging.
- computeUserInitials: assert leading/trailing whitespace doesn't
  leak as blank glyphs into the avatar circle and defeat the
  UserMenu's silhouette fallback.

Also strip ticket/PR/diff-history references from existing user-
initials test comments per CLAUDE.md.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 6bbaf88. Configure here.

Comment thread packages/k8s-ui/package.json
Every other entry in peerDependencies has a matching devDependencies
pin so the package can be type-checked, tested, and built standalone.
tailwind-merge was added to peerDependencies in this branch but the
devDependencies mirror was missed; Tooltip.test.tsx transitively
depends on it, so running vitest against the package outside the
monorepo (or under different hoisting rules) would fail.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@eliran-ops
Copy link
Copy Markdown
Contributor Author

@nadaverell a reminder

@eliran-ops
Copy link
Copy Markdown
Contributor Author

@nadaverell — final review-comment items from #issuecomment-4350877739 addressed.

  • 3. twMerge override testpackages/k8s-ui/src/components/ui/Tooltip.test.tsx (commit 6bbaf88) pins the regression directly: render with wrapperClassName="block" asserts block lands on the wrapper and inline-flex does NOT. Three cases: caller override wins, default stays when no override, non-conflicting utilities (min-w-0 flex-1) merge alongside defaults. Reverting twMerge(clsx(...)) back to clsx(...) fails the first case.
  • 4. SKY-825 refs in user-initials.test.ts preamble — same commit. The two diff-narration comments ("Bugbot regression: the v2 fallback…", "Reviewer regression: the v2 docstring said…") rewritten to state the WHY without referencing prior versions, per CLAUDE.md. Added one more test pinning leading-whitespace handling (' alice''AL').
  • Items 1 (docstring/impl coherence), 2 (whitespace trim — handled by the non-letter strip), and 5 (docstring's radar-hub-web framing) were addressed in the earlier 0ab023e / a760e41 pass; rationale in the file comments stands on its own without ticket refs.

Test suite: packages/k8s-ui — 161 passed across 15 files. npm run tsc clean. Cursor Bugbot SUCCESS on current HEAD (7175703).

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