Feat/tactical telemetry redesign#61
Open
nazarli-shabnam wants to merge 9 commits into
Open
Conversation
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (3)
📝 WalkthroughWalkthroughThe app migrates icon usage to ChangesIcon migration and visual redesign
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
apps/ui/components/check-card.tsx (1)
61-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSuccess state uses hardcoded green instead of a design token.
The fail branch correctly uses the new
destructivesemantic token (border-destructive/25,text-destructive), but the pass branch still hardcodesgreen-500/green-400. This breaks the token-driven theming this redesign otherwise establishes and won't automatically adapt if the palette/dark-mode tokens change.♻️ Use a semantic success/accent token instead of hardcoded green
className={`bg-card border p-3.5 flex items-start gap-3 transition-colors duration-200 ease-(--ease-out) ${ - pass ? "border-green-500/20 hover:border-green-500/35" : "border-destructive/25 hover:border-destructive/45" + pass ? "border-accent/20 hover:border-accent/35" : "border-destructive/25 hover:border-destructive/45" }`} > {pass ? ( - <CheckCircle className="size-4 shrink-0 mt-0.5 text-green-400" /> + <CheckCircle className="size-4 shrink-0 mt-0.5 text-accent" /> ) : (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ui/components/check-card.tsx` around lines 61 - 68, The success state in CheckCard still hardcodes green values instead of using the new semantic token approach. Update the pass branch in check-card.tsx (the className and CheckCircle usage inside CheckCard) to use the appropriate semantic success/accent token classes rather than green-500/green-400 so it matches the token-driven styling used by the destructive branch.apps/ui/package.json (1)
15-18: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider enabling
optimizePackageImportsfor@phosphor-icons/react.The package's own docs recommend adding it to
experimental.optimizePackageImportsinnext.config.jsso Next.js only compiles the icon modules actually used, rather than the whole package.next.config.jsisn't part of this diff/cohort, so flagging as a forward-looking note rather than a required change here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ui/package.json` around lines 15 - 18, The package entry for `@phosphor-icons/react` should be reflected in Next.js import optimization settings; when you update next.config.js, add `@phosphor-icons/react` to experimental.optimizePackageImports so Next.js compiles only the icon modules actually used. Use the existing next.config.js configuration to locate the optimizePackageImports array and include this package there.apps/ui/app/globals.css (1)
342-359: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
.section-titleduplicates.section-labelverbatim.Both rules now have identical declarations. Since the comment states it's kept only as a transitional alias, consider merging the selectors to avoid drift if one gets edited later.
♻️ Suggested consolidation
- .section-label { - font-size: 0.6875rem; - font-weight: 500; - text-transform: uppercase; - letter-spacing: 0.1em; - color: var(--muted-foreground); - font-family: var(--font-mono); - } - - /* Keep old name as alias during the transition */ - .section-title { - font-size: 0.6875rem; - font-weight: 500; - text-transform: uppercase; - letter-spacing: 0.1em; - color: var(--muted-foreground); - font-family: var(--font-mono); - } + /* Keep old name as alias during the transition */ + .section-label, + .section-title { + font-size: 0.6875rem; + font-weight: 500; + text-transform: uppercase; + letter-spacing: 0.1em; + color: var(--muted-foreground); + font-family: var(--font-mono); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ui/app/globals.css` around lines 342 - 359, The .section-title rule is a verbatim duplicate of .section-label and should be consolidated to avoid future drift. Update the styling in globals.css so .section-title acts as a transitional alias by sharing the same declarations as .section-label, preferably through a combined selector or shared rule, and keep the unique symbols .section-label and .section-title easy to locate for the transition period.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ui/app/globals.css`:
- Around line 477-488: The .noise-overlay layer is currently above interactive
overlay components, so lower its stacking order in globals.css by adjusting the
.noise-overlay rule to sit beneath Sheet/Tooltip overlays used by the app.
Update the .noise-overlay z-index to a value below the overlay layers while
keeping the fixed, pointer-events-none grain effect in place.
In `@apps/ui/components/page-header.tsx`:
- Around line 16-23: The decorative /// marker in PageHeader should be hidden
from assistive technology so it is not announced before the page title. Update
the span that renders the marker in PageHeader to include aria-hidden="true"
while leaving the title heading unchanged.
In `@apps/ui/components/ui/button.tsx`:
- Around line 9-10: The Button class string in button.tsx uses an unsupported
Tailwind variant for the popup exclusion, so the active scale rule is not
applying correctly. Update the class definition in the button styling to use an
arbitrary selector-based :not() check for aria-haspopup instead of
not-aria-[haspopup], keeping the rule attached to the same button variant logic
in the Button component.
In `@apps/ui/components/ui/sidebar.tsx`:
- Line 682: The sidebar item icon color is always using the accent foreground,
which no longer matches the label’s inactive state. Update the class list in the
sidebar button styling so the `[&>svg]` color follows the same state modifiers
as the text (`hover:`, `active:`, and `data-active:`) in the relevant sidebar
component. Keep the icon styling aligned with the existing button state classes
in `sidebar.tsx` so both label and icon switch colors together.
---
Nitpick comments:
In `@apps/ui/app/globals.css`:
- Around line 342-359: The .section-title rule is a verbatim duplicate of
.section-label and should be consolidated to avoid future drift. Update the
styling in globals.css so .section-title acts as a transitional alias by sharing
the same declarations as .section-label, preferably through a combined selector
or shared rule, and keep the unique symbols .section-label and .section-title
easy to locate for the transition period.
In `@apps/ui/components/check-card.tsx`:
- Around line 61-68: The success state in CheckCard still hardcodes green values
instead of using the new semantic token approach. Update the pass branch in
check-card.tsx (the className and CheckCircle usage inside CheckCard) to use the
appropriate semantic success/accent token classes rather than
green-500/green-400 so it matches the token-driven styling used by the
destructive branch.
In `@apps/ui/package.json`:
- Around line 15-18: The package entry for `@phosphor-icons/react` should be
reflected in Next.js import optimization settings; when you update
next.config.js, add `@phosphor-icons/react` to experimental.optimizePackageImports
so Next.js compiles only the icon modules actually used. Use the existing
next.config.js configuration to locate the optimizePackageImports array and
include this package there.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e0347eb5-23eb-434c-9cc7-701c692350f2
⛔ Files ignored due to path filters (1)
apps/ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
apps/ui/app/automation/page.tsxapps/ui/app/collaborators/page.tsxapps/ui/app/globals.cssapps/ui/app/layout.tsxapps/ui/app/login/page.tsxapps/ui/app/page.tsxapps/ui/app/repos/[repo]/cache/page.tsxapps/ui/app/security/page.tsxapps/ui/app/settings/page.tsxapps/ui/app/setup/page.tsxapps/ui/components/activity-list.tsxapps/ui/components/app-sidebar.tsxapps/ui/components/check-card.tsxapps/ui/components/empty-state.tsxapps/ui/components/icon-provider.tsxapps/ui/components/page-header.tsxapps/ui/components/stat-card.tsxapps/ui/components/ui/badge.tsxapps/ui/components/ui/button.tsxapps/ui/components/ui/card.tsxapps/ui/components/ui/input.tsxapps/ui/components/ui/sheet.tsxapps/ui/components/ui/sidebar.tsxapps/ui/components/ui/skeleton.tsxapps/ui/components/ui/tooltip.tsxapps/ui/package.json
💤 Files with no reviewable changes (2)
- apps/ui/app/collaborators/page.tsx
- apps/ui/app/automation/page.tsx
Update stale page-header test assertion (title is the h1, not description), fix noise-overlay z-index over Sheet/Tooltip, add aria-hidden to the decorative marker, fix unsupported not-aria-[haspopup] variant, align sidebar icon color with label state, use accent token instead of hardcoded green, dedupe section-label/section-title, and enable optimizePackageImports for phosphor icons.
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
…ry-redesign # Conflicts: # apps/ui/package-lock.json # skills-lock.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit