Skip to content

Conversation

@emir-karabeg
Copy link
Collaborator

Summary

Brief description of what this PR does and why.

Fixes #(issue)

Type of Change

  • New feature

Testing

How has this been tested? What should reviewers focus on?

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

Screenshots/Videos

@vercel
Copy link

vercel bot commented Dec 18, 2025

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

1 Skipped Deployment
Project Deployment Review Updated (UTC)
docs Skipped Skipped Dec 24, 2025 0:22am

@emir-karabeg
Copy link
Collaborator Author

@greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 20, 2025

Greptile Summary

This PR implements a comprehensive design system refresh with light/dark theme support, note block enhancements, and UI/UX improvements across 181 files.

Key Changes

  • Theme System: Refactored from stone-only palette to warm light theme + dark neutral theme with new CSS variable system
  • Note Block: Added YouTube embed support, removed format dropdown (always markdown), refined spacing
  • EMCN Components: Enhanced Combobox with grouped options, added size variants to Popover and Badge, updated color tokens
  • Subscription UI: Redesigned layout with consolidated usage components and improved skeleton states
  • OAuth Modal: Simplified scope descriptions for consistency
  • Tool Input: Refactored to use grouped Combobox component for better organization
  • Zoom Prevention: New usePreventZoom hook for overlay components

Issues Found

  • Theme utility bug (line 19 in theme.ts): oldValue is retrieved after setting newValue, causing incorrect event data
  • Custom instruction violation (globals.css): Large-scale edits to globals.css file violate custom instruction c3b5e4b0 which states to avoid editing this file unless absolutely necessary

Confidence Score: 4/5

  • Safe to merge with one logic bug that needs fixing
  • Comprehensive design system refactor with mostly safe changes. The theme.ts logic bug needs fixing before merge, but it's a straightforward fix. The globals.css changes violate style guide but are intentional for the design system refresh.
  • Pay attention to apps/sim/lib/core/utils/theme.ts for the oldValue bug fix

Important Files Changed

Filename Overview
apps/sim/app/_styles/globals.css Major refactor of color system from stone palette to warm light/dark themes, removed workflow z-index rules, increased panel width
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/note-block/note-block.tsx Added YouTube embed support, removed format dropdown (always uses markdown), refined spacing and styling
apps/sim/components/emcn/components/combobox/combobox.tsx Added grouped options support, custom select handlers, disabled state, empty message, and maxHeight prop
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.tsx Refactored tool selection UI to use new grouped Combobox component, improved organization and layout
apps/sim/components/emcn/components/popover/popover.tsx Added size variants (sm/md), renamed primary to secondary variant, updated spacing and prevented auto-focus flickering
apps/sim/lib/core/utils/theme.ts Modified theme sync logic, but oldValue retrieval happens after setting new value which always returns new value

Sequence Diagram

sequenceDiagram
    participant User
    participant Component
    participant ThemeUtil
    participant LocalStorage
    participant NextThemes
    participant DOM

    User->>Component: Select theme (light/dark/system)
    Component->>ThemeUtil: syncThemeToNextThemes(theme)
    ThemeUtil->>LocalStorage: setItem('sim-theme', theme)
    Note over ThemeUtil,LocalStorage: Bug: oldValue retrieved AFTER setting
    ThemeUtil->>LocalStorage: getItem('sim-theme')
    LocalStorage-->>ThemeUtil: Returns NEW value (not old)
    ThemeUtil->>NextThemes: dispatchEvent(StorageEvent)
    Note over ThemeUtil,NextThemes: Incorrect oldValue in event
    ThemeUtil->>DOM: classList.remove('light', 'dark')
    ThemeUtil->>DOM: classList.add(resolvedTheme)
    DOM-->>User: UI updates with new theme
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (2)

  1. apps/sim/app/invite/components/status-card.tsx, line 7 (link)

    style: Loader2 import still present but no longer used in the component

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  2. apps/sim/components/ui/tag-input.tsx, line 79 (link)

    style: Redundant conditional logic - both branches set the same pl-[4px] value

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

133 files reviewed, 10 comments

Edit Code Review Agent Settings | Greptile


const textareaVariants = cva(
'flex w-full rounded-[4px] border border-[var(--surface-11)] bg-[var(--surface-6)] dark:bg-[var(--surface-9)] px-[8px] py-[8px] font-medium font-sans text-sm text-[var(--text-primary)] transition-colors placeholder:text-[var(--text-muted)] dark:placeholder:text-[var(--text-muted)] outline-none focus:outline-none focus-visible:outline-none focus-visible:ring-0 focus-visible:ring-offset-0 resize-none overflow-auto disabled:cursor-not-allowed disabled:opacity-50',
'flex w-full rounded-[4px] border border-[var(--border-1)] bg-[var(--surface-5)] dark:bg-[var(--surface-5)] px-[8px] py-[8px] font-medium font-sans text-sm text-[var(--text-primary)] transition-colors placeholder:text-[var(--text-muted)] dark:placeholder:text-[var(--text-muted)] outline-none focus:outline-none focus-visible:outline-none focus-visible:ring-0 focus-visible:ring-offset-0 resize-none overflow-auto disabled:cursor-not-allowed disabled:opacity-50',
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Redundant dark class - both light and dark use same value --surface-5

Suggested change
'flex w-full rounded-[4px] border border-[var(--border-1)] bg-[var(--surface-5)] dark:bg-[var(--surface-5)] px-[8px] py-[8px] font-medium font-sans text-sm text-[var(--text-primary)] transition-colors placeholder:text-[var(--text-muted)] dark:placeholder:text-[var(--text-muted)] outline-none focus:outline-none focus-visible:outline-none focus-visible:ring-0 focus-visible:ring-offset-0 resize-none overflow-auto disabled:cursor-not-allowed disabled:opacity-50',
'flex w-full rounded-[4px] border border-[var(--border-1)] bg-[var(--surface-5)] px-[8px] py-[8px] font-medium font-sans text-sm text-[var(--text-primary)] transition-colors placeholder:text-[var(--text-muted)] dark:placeholder:text-[var(--text-muted)] outline-none focus:outline-none focus-visible:outline-none focus-visible:ring-0 focus-visible:ring-offset-0 resize-none overflow-auto disabled:cursor-not-allowed disabled:opacity-50',

Context Used: Context from dashboard - Tailwind CSS and styling conventions (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/components/emcn/components/textarea/textarea.tsx
Line: 6:6

Comment:
**style:** Redundant dark class - both light and dark use same value `--surface-5`

```suggestion
  'flex w-full rounded-[4px] border border-[var(--border-1)] bg-[var(--surface-5)] px-[8px] py-[8px] font-medium font-sans text-sm text-[var(--text-primary)] transition-colors placeholder:text-[var(--text-muted)] dark:placeholder:text-[var(--text-muted)] outline-none focus:outline-none focus-visible:outline-none focus-visible:ring-0 focus-visible:ring-offset-0 resize-none overflow-auto disabled:cursor-not-allowed disabled:opacity-50',
```

**Context Used:** Context from `dashboard` - Tailwind CSS and styling conventions ([source](https://app.greptile.com/review/custom-context?memory=e1e9ca56-9819-463f-be67-0d2ba849ec4f))

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

key: 'sim-theme',
newValue: forcedTheme,
newValue: theme,
oldValue: localStorage.getItem('sim-theme'),
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: retrieving the old value after setting the new value will always return the new value, creating incorrect event data

Suggested change
oldValue: localStorage.getItem('sim-theme'),
const oldValue = localStorage.getItem('sim-theme')
localStorage.setItem('sim-theme', theme)
window.dispatchEvent(
new StorageEvent('storage', {
key: 'sim-theme',
newValue: theme,
oldValue: oldValue,
storageArea: localStorage,
url: window.location.href,
})
)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/lib/core/utils/theme.ts
Line: 19:19

Comment:
**logic:** retrieving the old value after setting the new value will always return the new value, creating incorrect event data

```suggestion
      const oldValue = localStorage.getItem('sim-theme')
  localStorage.setItem('sim-theme', theme)

  window.dispatchEvent(
    new StorageEvent('storage', {
      key: 'sim-theme',
      newValue: theme,
      oldValue: oldValue,
      storageArea: localStorage,
      url: window.location.href,
    })
  )
```

How can I resolve this? If you propose a fix, please make it concise.

secondary: 'bg-[var(--brand-secondary)] text-[var(--text-primary)]',
tertiary:
'bg-[var(--brand-tertiary)] dark:bg-[var(--brand-tertiary)] dark:text-[var(--text-primary)] text-[var(--text-primary)] hover:bg-[var(--brand-tertiary)] hover:dark:bg-[var(--brand-tertiary)] hover:text-[var(--text-primary)] hover:dark:text-[var(--text-primary)]',
'!bg-[var(--brand-tertiary-2)] !text-[var(--text-inverse)] hover:brightness-110 hover:!text-[var(--text-inverse)] ![transition-property:background-color,border-color,fill,stroke]',
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Using !important overrides suggests this variant has styling conflicts. Consider if the base styles can be adjusted to avoid needing !important. Is there a specific reason this variant needs to override the transition property with !important?

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/components/emcn/components/button/button.tsx
Line: 21:21

Comment:
**style:** Using !important overrides suggests this variant has styling conflicts. Consider if the base styles can be adjusted to avoid needing !important. Is there a specific reason this variant needs to override the transition property with !important?

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines 490 to 491
{Array.from({ length: 12 }).map((_, i) => {
const filledCount = Math.ceil((Math.min(storageInfo.percentUsed, 100) / 100) * 16)
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Progress bar calculation mismatch: using 12 segments in the loop but calculating filledCount based on 16 segments

Suggested change
{Array.from({ length: 12 }).map((_, i) => {
const filledCount = Math.ceil((Math.min(storageInfo.percentUsed, 100) / 100) * 16)
{Array.from({ length: 12 }).map((_, i) => {
const filledCount = Math.ceil((Math.min(storageInfo.percentUsed, 100) / 100) * 12)

Should the calculation use 12 segments to match the loop, or should the loop create 16 segments to match the calculation?

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/settings-modal/components/files/files.tsx
Line: 490:491

Comment:
**logic:** Progress bar calculation mismatch: using 12 segments in the loop but calculating filledCount based on 16 segments

```suggestion
            {Array.from({ length: 12 }).map((_, i) => {
              const filledCount = Math.ceil((Math.min(storageInfo.percentUsed, 100) / 100) * 12)
```

 Should the calculation use 12 segments to match the loop, or should the loop create 16 segments to match the calculation?

How can I resolve this? If you propose a fix, please make it concise.

onMouseEnter={() => setIsHoveringMessage(true)}
onMouseLeave={() => setIsHoveringMessage(false)}
className='group relative w-full cursor-pointer rounded-[4px] border border-[var(--surface-11)] bg-[var(--surface-6)] px-[6px] py-[6px] transition-all duration-200 hover:border-[var(--surface-14)] hover:bg-[var(--surface-9)] dark:bg-[var(--surface-9)] dark:hover:border-[var(--surface-13)] dark:hover:bg-[var(--surface-11)]'
className='group relative w-full cursor-pointer rounded-[4px] border border-[var(--border-1)] bg-[var(--surface-5)] px-[6px] py-[6px] transition-all duration-200 hover:border-[var(--surface-14)] hover:bg-[var(--surface-5)] dark:bg-[var(--surface-5)] dark:hover:border-[var(--surface-13)] dark:hover:bg-[var(--border-1)]'
Copy link
Contributor

Choose a reason for hiding this comment

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

style: The dark mode hover state uses dark:hover:bg-[var(--border-1)] which seems inconsistent - using a border color for background might create unexpected visual results. Is using --border-1 as a background color in dark mode hover state intentional, or should this be a surface color?

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/copilot/components/copilot-message/copilot-message.tsx
Line: 314:314

Comment:
**style:** The dark mode hover state uses `dark:hover:bg-[var(--border-1)]` which seems inconsistent - using a border color for background might create unexpected visual results. Is using `--border-1` as a background color in dark mode hover state intentional, or should this be a surface color?

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

{/* Gradient fade when truncated - applies to entire message box */}
{!isExpanded && needsExpansion && (
<div className='pointer-events-none absolute right-0 bottom-0 left-0 h-6 bg-gradient-to-t from-0% from-[var(--surface-6)] via-40% via-[var(--surface-6)]/70 to-100% to-transparent group-hover:from-[var(--surface-9)] group-hover:via-[var(--surface-9)]/70 dark:from-[var(--surface-9)] dark:via-[var(--surface-9)]/70 dark:group-hover:from-[var(--surface-11)] dark:group-hover:via-[var(--surface-11)]/70' />
<div className='pointer-events-none absolute right-0 bottom-0 left-0 h-6 bg-gradient-to-t from-0% from-[var(--surface-5)] via-40% via-[var(--surface-5)]/70 to-100% to-transparent group-hover:from-[var(--surface-5)] group-hover:via-[var(--surface-5)]/70 dark:from-[var(--surface-5)] dark:via-[var(--surface-5)]/70 dark:group-hover:from-[var(--border-1)] dark:group-hover:via-[var(--border-1)]/70' />
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Similar inconsistency in gradient overlay where dark mode hover uses --border-1 for gradient colors instead of surface colors

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/copilot/components/copilot-message/copilot-message.tsx
Line: 360:360

Comment:
**style:** Similar inconsistency in gradient overlay where dark mode hover uses `--border-1` for gradient colors instead of surface colors

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +428 to +430
<span className='truncate font-base text-[14px] text-[var(--text-primary)] dark:text-[var(--white)]'>
{activeWorkspace?.name || 'Loading...'}
</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

style: inconsistent dark mode text styling - active popover button uses text-[var(--text-primary)] (line 284) while disabled fallback uses dark:text-[var(--white)]

Suggested change
<span className='truncate font-base text-[14px] text-[var(--text-primary)] dark:text-[var(--white)]'>
{activeWorkspace?.name || 'Loading...'}
</span>
<span className='truncate font-base text-[14px] text-[var(--text-primary)]'>
{activeWorkspace?.name || 'Loading...'}
</span>

Context Used: Context from dashboard - Tailwind CSS and styling conventions (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.tsx
Line: 428:430

Comment:
**style:** inconsistent dark mode text styling - active popover button uses `text-[var(--text-primary)]` (line 284) while disabled fallback uses `dark:text-[var(--white)]`

```suggestion
            <span className='truncate font-base text-[14px] text-[var(--text-primary)]'>
              {activeWorkspace?.name || 'Loading...'}
            </span>
```

**Context Used:** Context from `dashboard` - Tailwind CSS and styling conventions ([source](https://app.greptile.com/review/custom-context?memory=e1e9ca56-9819-463f-be67-0d2ba849ec4f))

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines 451 to 486
<Button
className='h-[28px] truncate px-[8px] py-[5px] text-[12.5px] hover:bg-[var(--surface-9)] hover:text-[var(--text-primary)]'
className={`h-[28px] truncate rounded-[6px] border px-[8px] py-[5px] text-[12.5px] ${
_hasHydrated && activeTab === 'copilot'
? 'border-[var(--border-1)]'
: 'border-transparent hover:border-[var(--border-1)] hover:bg-[var(--surface-5)] hover:text-[var(--text-primary)]'
}`}
variant={_hasHydrated && activeTab === 'copilot' ? 'active' : 'ghost'}
onClick={() => handleTabClick('copilot')}
data-tab-button='copilot'
>
Copilot
</Button>
<Button
className='h-[28px] px-[8px] py-[5px] text-[12.5px] hover:bg-[var(--surface-9)] hover:text-[var(--text-primary)]'
className={`h-[28px] rounded-[6px] border px-[8px] py-[5px] text-[12.5px] ${
_hasHydrated && activeTab === 'toolbar'
? 'border-[var(--border-1)]'
: 'border-transparent hover:border-[var(--border-1)] hover:bg-[var(--surface-5)] hover:text-[var(--text-primary)]'
}`}
variant={_hasHydrated && activeTab === 'toolbar' ? 'active' : 'ghost'}
onClick={() => handleTabClick('toolbar')}
data-tab-button='toolbar'
>
Toolbar
</Button>
<Button
className='h-[28px] px-[8px] py-[5px] text-[12.5px] hover:bg-[var(--surface-9)] hover:text-[var(--text-primary)]'
className={`h-[28px] rounded-[6px] border px-[8px] py-[5px] text-[12.5px] ${
_hasHydrated && activeTab === 'editor'
? 'border-[var(--border-1)]'
: 'border-transparent hover:border-[var(--border-1)] hover:bg-[var(--surface-5)] hover:text-[var(--text-primary)]'
}`}
variant={_hasHydrated && activeTab === 'editor' ? 'active' : 'ghost'}
onClick={() => handleTabClick('editor')}
data-tab-button='editor'
>
Editor
</Button>
Copy link
Contributor

Choose a reason for hiding this comment

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

style: The custom conditional styling with template literals duplicates similar logic across three tab buttons, creating maintenance overhead.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/panel.tsx
Line: 451:486

Comment:
**style:** The custom conditional styling with template literals duplicates similar logic across three tab buttons, creating maintenance overhead.

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

@emir-karabeg
Copy link
Collaborator Author

@greptile

@emir-karabeg emir-karabeg marked this pull request as ready for review December 24, 2025 00:22
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