Skip to content

fix: sidebar labels not appearing#91

Open
MarkVillacampa wants to merge 1 commit into
steipete:mainfrom
MarkVillacampa:fix/sidebar-nav-label-visibility
Open

fix: sidebar labels not appearing#91
MarkVillacampa wants to merge 1 commit into
steipete:mainfrom
MarkVillacampa:fix/sidebar-nav-label-visibility

Conversation

@MarkVillacampa

Copy link
Copy Markdown

Now below the breakpoint the label is screen-reader-only (icon rail stays accessible) but at/above it the label is simply its default visible state. Applying sr-only one-directionally avoids relying on a not-sr-only override beating the base rule, which is order/cache fragile.

image

Below the breakpoint the label is screen-reader-only (icon rail stays accessible); at/above it the label is simply its default visible state. Applying sr-only one-directionally avoids relying on a not-sr-only override beating the base rule, which is order/cache fragile.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@clawsweeper

clawsweeper Bot commented Jul 4, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 5, 2026, 12:08 PM ET / 16:08 UTC.

Summary
The PR changes navLinkLabelClass from sr-only min-[1100px]:not-sr-only to max-[1100px]:sr-only in src/lib/ui.ts.

Reproducibility: no. high-confidence live reproduction was established in this read-only review. Source inspection shows the normal AppNav path still depends on the old responsive class, and the attached screenshot shows the reported missing-label state rather than a verified fix.

Review metrics: 2 noteworthy metrics.

  • Diff scope: 1 file changed, 1 class string updated. The patch is localized to the shared sidebar label utility, so review can focus on responsive CSS behavior and proof.
  • Visual proof: 1 screenshot attached; 0 screenshots show labels visible. The PR changes visible UI and the supplied image does not demonstrate the repaired wide-sidebar state.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add an after-fix screenshot or short recording at a viewport width above 1100px showing sidebar nav labels visible.
  • Also show the below-1100px or compact route state if possible, with private details redacted; updating the PR body should trigger a fresh ClawSweeper review, or a maintainer can comment @clawsweeper re-review.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The screenshot is viewable, but it shows an icon-only wide sidebar and does not demonstrate the repaired label-visible state; the contributor should add an after-fix screenshot or recording with private details redacted. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A browser visual proof would directly show whether sidebar labels appear at desktop width and stay hidden in the narrow or compact state. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis visual task: verify sidebar nav labels are visible at desktop width and screen-reader-only below 1100px.

Risk before merge

  • [P1] The attached screenshot does not show the claimed after-fix state, so maintainers still lack positive browser proof that labels appear at desktop width.
  • [P1] Current tests only assert compact labels stay sr-only; they do not cover the normal responsive label class or a real browser breakpoint.

Maintainer options:

  1. Decide the mitigation before merge
    Land the small responsive-class fix only after visible browser proof shows labels at desktop width and confirms labels remain hidden below the breakpoint or in compact routes.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] The remaining blocker is contributor or maintainer visual proof, not a concrete code repair that ClawSweeper should attempt automatically.

Security
Cleared: Cleared; the diff only changes a Tailwind class string in UI code and introduces no dependency, workflow, secret, or code-execution change.

Review details

Best possible solution:

Land the small responsive-class fix only after visible browser proof shows labels at desktop width and confirms labels remain hidden below the breakpoint or in compact routes.

Do we have a high-confidence way to reproduce the issue?

No high-confidence live reproduction was established in this read-only review. Source inspection shows the normal AppNav path still depends on the old responsive class, and the attached screenshot shows the reported missing-label state rather than a verified fix.

Is this the best way to solve the issue?

Yes, the proposed direction is narrow and maintainable: make labels visible by default and apply sr-only only below the breakpoint. The remaining blocker is visual after-fix proof, not a broader implementation change.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 66523e1dcb32.

Label changes

Label justifications:

  • P2: This is a normal-priority user-facing web UI regression with a small localized fix and limited blast radius.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The screenshot is viewable, but it shows an icon-only wide sidebar and does not demonstrate the repaired label-visible state; the contributor should add an after-fix screenshot or recording with private details redacted. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence. The screenshot is viewable, but it shows an icon-only wide sidebar and does not demonstrate the repaired label-visible state; the contributor should add an after-fix screenshot or recording with private details redacted.
Evidence reviewed

What I checked:

  • Target repository policy: No AGENTS.md exists inside the target git repository; the only discovered AGENTS.md was outside the birdclaw checkout and was not applied as target policy.
  • Current main still has the old label class: At current main, non-compact nav labels are still sr-only min-[1100px]:not-sr-only, so the central issue is not already implemented on main. (src/lib/ui.ts:46, 66523e1dcb32)
  • PR patch scope: The submitted patch changes exactly the shared nav label class and leaves compact-label behavior unchanged. (src/lib/ui.ts:46, db377444a52b)
  • Class usage is scoped through AppNav: AppNav applies navLinkLabelClass only for normal navigation and uses navLinkLabelCompactClass when the nav is explicitly compact. (src/components/AppNav.tsx:101, 66523e1dcb32)
  • Proof screenshot inspected: The attached PNG is viewable at 1767x989, but it shows the Discuss route with a wide sidebar containing icons only; it does not demonstrate nav labels visible after the fix.
  • Feature history: Blame and file-history inspection point to Peter Steinberger as the recent owner of the sidebar UI area. (src/lib/ui.ts:46, ef10b93e91af)

Likely related people:

  • steipete: git blame on the current sidebar label class and GitHub file-history queries for src/lib/ui.ts, src/components/AppNav.tsx, and src/routes/__root.tsx all point to Peter Steinberger as the person most connected to the sidebar UI behavior. (role: introduced behavior and recent area contributor; confidence: high; commits: a933d72f5e00, 8390d3f50587, 68ab69279291; files: src/lib/ui.ts, src/components/AppNav.tsx, src/routes/__root.tsx)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (1 earlier review cycle)
  • reviewed 2026-07-04T20:45:44.509Z sha db37744 :: needs real behavior proof before merge. :: none

@clawsweeper clawsweeper Bot added proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels Jul 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant