-
Notifications
You must be signed in to change notification settings - Fork 3k
feat: light, note block, ui/ux #2457
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
e057c34 to
7deeb50
Compare
Greptile SummaryThis PR implements a comprehensive design system refresh with light/dark theme support, note block enhancements, and UI/UX improvements across 181 files. Key Changes
Issues Found
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (2)
-
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!
-
apps/sim/components/ui/tag-input.tsx, line 79 (link)style: Redundant conditional logic - both branches set the same
pl-[4px]valueNote: 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
|
|
||
| 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', |
There was a problem hiding this comment.
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
| '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'), |
There was a problem hiding this comment.
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
| 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]', |
There was a problem hiding this comment.
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.| {Array.from({ length: 12 }).map((_, i) => { | ||
| const filledCount = Math.ceil((Math.min(storageInfo.percentUsed, 100) / 100) * 16) |
There was a problem hiding this comment.
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
| {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)]' |
There was a problem hiding this comment.
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' /> |
There was a problem hiding this comment.
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.| <span className='truncate font-base text-[14px] text-[var(--text-primary)] dark:text-[var(--white)]'> | ||
| {activeWorkspace?.name || 'Loading...'} | ||
| </span> |
There was a problem hiding this comment.
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)]
| <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.| <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> |
There was a problem hiding this comment.
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.359fec3 to
db74927
Compare
db74927 to
93c9cfc
Compare
Summary
Brief description of what this PR does and why.
Fixes #(issue)
Type of Change
Testing
How has this been tested? What should reviewers focus on?
Checklist
Screenshots/Videos