Skip to content

Conversation

@devin-ai-integration
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Oct 29, 2025

Issue

This PR addresses critical functionality and accessibility issues found during code review of PR #3931.

Link to Devin run: https://app.devin.ai/sessions/e6b4e7ed91ed4ac298e6d03b67b1d395
Requested by: [email protected] (@MH4GF)

Why is this change needed?

During review of PR #3931 (which removes undefined CSS custom properties), I identified several CSS property deletions that negatively impact functionality and accessibility. While most of the removed properties were indeed dead code, some were critical:

Critical Fixes Applied

1. Radix UI Runtime Variables (High Priority)

  • File: Select.module.css
  • Issue: Removed --radix-select-trigger-width and --radix-select-content-available-height which are injected at runtime by Radix UI
  • Fix: Restored with stylelint-disable comments to suppress false positives
  • Impact: Without these, Select dropdown sizing/positioning would break

2. Accessibility - Focus Outlines (High Priority)

  • Files: LeftPane.module.css, Call.module.css
  • Issue: Removed focus outline colors, leaving only outline-offset
  • Fix: Restored with var(--primary-accent)
  • Impact: Keyboard navigation users couldn't see focus state

3. Visual Affordances (Medium Priority)

  • Resizable.module.css: Restored handle border and background for visibility
  • Multiple form components: Restored opacity: 0.6 for disabled states (replaced undefined variables like var(--opacity-disabled) with numeric values)
  • Impact: Users couldn't tell when UI elements were disabled or resizable

4. Layout - Sidebar Positioning (Medium Priority)

  • File: Sidebar.module.css
  • Issue: Removed offcanvas positioning
  • Fix: Restored with fallback value: left: calc(var(--sidebar-width, 16rem) * -1)
  • Impact: Mobile sidebar might not hide correctly

Review Checklist for Maintainers

⚠️ Critical Testing Needed:

  • Verify Select dropdown sizing and positioning works correctly
  • Test keyboard navigation - ensure focus outlines are visible on all interactive elements
  • Test disabled states - buttons and form inputs should appear visually disabled (with reduced opacity)
  • Test mobile view - sidebar should hide properly in offcanvas mode
  • Verify Resizable handle is visible and draggable

⚠️ Note: An unrelated auto-generated file schema.generated.ts appears in this diff. This should likely be removed from the PR or added to .gitignore.

Remaining Original Changes

The original PR's removal of truly undefined CSS variables remains intact in files I didn't modify. Those changes appear to be valid dead code elimination.

- Replace undefined opacity properties with numeric values (0.5, 0.6, 0.7)
- Replace undefined font-weight properties with numeric values (400, 500, 600)
- Replace undefined color properties with defined variables
- Replace undefined border-radius properties with defined variables
- Replace undefined spacing properties with defined variables or remove them
- Remove undefined Radix UI properties from animations
- Remove undefined shadow properties
- Rebuild CLI package to regenerate minified CSS

Fixes route06/liam-internal#5967

Co-Authored-By: [email protected] <[email protected]>
@devin-ai-integration
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI' or '@devin'.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@giselles-ai
Copy link

giselles-ai bot commented Oct 29, 2025

Finished running flow.

Step Status Updated(UTC)
1 Oct 29, 2025 8:13am
2 Oct 29, 2025 8:14am
3 Oct 29, 2025 8:14am

@vercel
Copy link

vercel bot commented Oct 29, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
liam-app Ready Ready Preview Comment Oct 29, 2025 9:15am
liam-assets Ready Ready Preview Comment Oct 29, 2025 9:15am
liam-docs Ready Ready Preview Comment Oct 29, 2025 9:15am
liam-erd-sample Ready Ready Preview Comment Oct 29, 2025 9:15am
liam-storybook Ready Ready Preview Comment Oct 29, 2025 9:15am

@supabase
Copy link

supabase bot commented Oct 29, 2025

Updates to Preview Branch (devin/1761724636-remove-undefined-css-properties) ↗︎

Deployments Status Updated
Database Wed, 29 Oct 2025 09:11:56 UTC
Services Wed, 29 Oct 2025 09:11:56 UTC
APIs Wed, 29 Oct 2025 09:11:56 UTC

Tasks are run on every commit but only new migration files are pushed.
Close and reopen this PR if you want to apply changes from existing seed or migration files.

Tasks Status Updated
Configurations Wed, 29 Oct 2025 09:11:57 UTC
Migrations Wed, 29 Oct 2025 09:11:57 UTC
Seeding Wed, 29 Oct 2025 09:11:57 UTC
Edge Functions Wed, 29 Oct 2025 09:11:57 UTC

View logs for this Workflow Run ↗︎.
Learn more about Supabase for Git ↗︎.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 29, 2025

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


Comment @coderabbitai help to get the list of available commands and usage tips.

@giselles-ai
Copy link

giselles-ai bot commented Oct 29, 2025

Check changeset necessity

Status: REQUIRED

Reason:

  • Changes touch target packages: @liam-hq/ui and @liam-hq/erd-core (both require changesets for user-facing changes).
  • CSS updates replace previously undefined custom properties with valid tokens/values, which can alter rendered styles (colors, borders, radii, focus outlines) — user-facing bug fixes/behavior changes.
  • No API surface changes; this qualifies as a patch bump, not a minor.
  • Modifications under frontend/apps/app pertain to @liam-hq/app (ignored package) and do not affect the necessity decision.

Changeset (copy & paste):

---
"@liam-hq/ui": patch
"@liam-hq/erd-core": patch
---
- 🐛 Remove undefined CSS custom property references and align styles to valid design tokens
  - Replace undefined variables with concrete values (e.g., opacity 0.6/0.7, font-weight 400/500/600/700)
  - Map deprecated tokens to current ones (e.g., --color-border → --pane-border, --radius-2 → --border-radius-sm)
  - Adjust focus/hover visuals to use --primary-accent; remove Radix-only sizing vars
  - No API changes; visual output may subtly change where prior values were ignored

- Delete all undefined CSS custom property references instead of replacing them
- These properties had no effect in production (browsers ignored them)
- Preserve Radix UI properties with stylelint disable comments (false positives)
- Radix UI injects --radix-* variables at runtime, so they are valid

Changes:
- Deleted opacity, font-weight, color, background-color, border-radius properties
- Deleted outline, border, spacing, and other undefined properties
- Added stylelint disable/enable comments for Radix UI collapsible animations
- All 27 CSS module files corrected

Verified: pnpm run lint:stylelint returns 0 errors
Co-Authored-By: [email protected] <[email protected]>
…ity and accessibility

- Restore Radix UI runtime variables (--radix-select-trigger-width, --radix-select-content-available-height) with stylelint suppression
- Restore focus outlines for keyboard navigation accessibility (LeftPane, Call components)
- Restore Resizable handle visibility (border and background)
- Restore disabled state opacity (0.6) across all form components and buttons
- Restore Sidebar offcanvas positioning with fallback value

These properties were incorrectly identified as undefined CSS variables but are actually:
1. Runtime-injected by Radix UI
2. Essential for accessibility (focus outlines)
3. Important for visual affordance (disabled states, handle visibility)
4. Critical for layout (offcanvas positioning)

Co-Authored-By: [email protected] <[email protected]>
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.

2 participants