fix: prevent duplicate variable in tailwind #1671
Merged
Conversation
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
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 86bd186 in 39 seconds
More details
- Looked at
24lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. apps/studio/electron/main/assets/styles.ts:145
- Draft comment:
Ensure that extractTailwindCssVariables always returns a valid object even for empty/malformed CSS. This check prevents false negatives for duplicates. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. apps/studio/electron/main/assets/styles.ts:147
- Draft comment:
Good duplicate check. Consider logging a warning or providing guidance if a duplicate is found. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
3. apps/studio/electron/main/assets/styles.ts:144
- Draft comment:
Good safeguard: the added check using extractTailwindCssVariables prevents duplicate CSS variables (both in :root and .dark). Ensure that newCssVarName is generated consistently across contexts. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure consistency in the generation ofnewCssVarName. This is similar to asking the author to double-check or verify something, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue, making it not useful according to the guidelines.
4. apps/studio/electron/main/assets/styles.ts:149
- Draft comment:
For readability, consider refactoring this duplicate-check branch: an early return on duplicate (as done) followed by the subsequent addition might be extracted into a helper if similar logic is needed elsewhere. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
Workflow ID: wflow_sna2D1Atn5TP2SwZ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 8c0e4e4 in 1 minute and 49 seconds
More details
- Looked at
369lines of code in6files - Skipped
0files when reviewing. - Skipped posting
14drafted comments based on config settings.
1. apps/studio/electron/main/assets/helpers.ts:104
- Draft comment:
Removal of the local toCamelCase function is intentional. Ensure all callers now import from the utility module consistently. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/studio/electron/main/assets/styles.ts:56
- Draft comment:
In updateTailwindColorConfig, newName is converted with toCamelCase when an update is performed but createTailwindColorVariable uses newName directly. Please ensure the casing is applied consistently across both paths to truly prevent duplicate variables. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The comment appears to be incorrect. The code is actually handling casing consistently by:
- Converting newName to camelCase once at the start
- Passing that camelCased version to both code paths
- Using that consistently named variable throughout both functions
Could I be missing some deeper inconsistency in how the casing is handled within those called functions? Could there be edge cases where the casing matters differently in different contexts?
No - looking at the implementation of both functions, they use the passed camelCased name directly without any further casing transformations. The comment is simply incorrect in its assessment.
The comment should be deleted as it is factually incorrect - the code is already handling casing consistently by applying toCamelCase once and using that result throughout both code paths.
3. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:279
- Draft comment:
Usage of toNormalCase for displaying color names improves readability. Verify that all UI components consistently apply this transformation. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:35
- Draft comment:
Conversion with toCamelCase in handleSave is correctly used to check for name duplication. Confirm that the utility's implementation aligns with your naming expectations. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. packages/utility/src/string.ts:19
- Draft comment:
The implementations of toNormalCase and toCamelCase look robust. Consider edge cases such as strings with consecutive uppercase letters or numeric separators if needed in the future. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
6. apps/studio/tests/helpers.test.ts:1
- Draft comment:
Test cases for toNormalCase and toCamelCase comprehensively cover many scenarios. Great work ensuring utility correctness. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. apps/studio/electron/main/assets/helpers.ts:115
- Draft comment:
Using 'newName' directly here (instead of applying toCamelCase) prevents duplicate variable creation. Ensure that the caller passes a properly normalized name if needed. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
8. apps/studio/electron/main/assets/styles.ts:56
- Draft comment:
Consistently computing 'camelCaseName' via toCamelCase(newName) in updateTailwindColorConfig and then passing it to create/update functions appears correct. Verify that normalization is handled uniformly across both branches. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
9. apps/studio/electron/main/assets/styles.ts:213
- Draft comment:
Updating config keys without applying toCamelCase (using newName directly) is intentional. Just confirm that newName is already normalized upstream to avoid mismatches. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
10. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:105
- Draft comment:
Resetting the local error state with setLocalError(null) upon initiating rename is a good improvement to clear stale warnings. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
11. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:80
- Draft comment:
The added onKeyDown handler to trigger handleSave on 'Enter' improves usability. This ensures users can commit changes via keyboard. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
12. apps/studio/tests/helpers.test.ts:1
- Draft comment:
The tests for toNormalCase and toCamelCase cover a wide variety of cases including empty strings, mixed separators, numbers, etc. This provides confidence in the utility functions. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
13. packages/utility/src/string.ts:9
- Draft comment:
The implementations of toNormalCase and toCamelCase are clear and robust. The regex patterns effectively handle most common cases, though be mindful if edge cases arise in future. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
14. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:57
- Draft comment:
Typographical Note: The component and file are named 'ColorPalletGroup', which might be a misspelling of 'ColorPaletteGroup' if the intent is to refer to a group of colors. Please verify the intended spelling and consistency across the project. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_7dvoTATHrjaLWCYW
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5659eba in 1 minute and 12 seconds
More details
- Looked at
45lines of code in1files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:39
- Draft comment:
Removed theme parameter but still passing state variable: ensure this intentional change doesn't lead to inconsistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:56
- Draft comment:
Removed theme parameter from handleDuplicate; verifies usage of outer state variable 'theme'. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:65
- Draft comment:
Consistency in handleDefaultColorChange: theme parameter removed and outer state 'theme' is used. Confirm this meets all intended behavior. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:1
- Draft comment:
The PR description and linked issue mention adding better onboarding docs. However, changes relate to preventing duplicate variables. Please update the PR description to reflect the actual changes. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:39
- Draft comment:
Redundant 'theme' parameter removed from handleColorChange (and handleColorChangeEnd) to use the state 'theme'. This streamlines the signature, but note that the PR title mentions preventing duplicate tailwind variables – ensure that this refactor fully addresses the intended issue. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:56
- Draft comment:
Removed the extra 'theme' parameter from handleDuplicate – it now relies on the state variable. Verify that all duplicate operations are intended to use the current theme. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:69
- Draft comment:
Simplified handleDefaultColorChange by removing its extra 'theme' parameter; it now uses the component-level 'theme' for consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
8. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:9
- Draft comment:
There's an inconsistency in the naming: the imported component is named 'BrandPalletGroup' from './ColorPalletGroup', but the UI and comments refer to color 'palettes'. If 'BrandPaletteGroup' was intended, please update the naming for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_BiHVCCyRQ6UxUWNB
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Closed
6 tasks
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 91a78b0 in 51 seconds
More details
- Looked at
171lines of code in4files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. apps/studio/electron/main/assets/styles.ts:57
- Draft comment:
Review: Switching from custom toCamelCase to lodash's camelCase. Verify that the output format matches previous expectations for all inputs. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify that the output format matches previous expectations, which is against the rules. It doesn't provide a specific suggestion or point out a potential issue with the code change. Therefore, this comment should be removed.
2. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:36
- Draft comment:
Ensure that using lodash's camelCase for the color name in the validation logic preserves the intended behavior, especially for reserved names like 'DEFAULT'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that the use of lodash'scamelCasefunction does not alter the intended behavior, particularly for reserved names like 'DEFAULT'. This is a request for confirmation of intended behavior, which violates the rule against asking the author to confirm their intention or ensure behavior is intended.
3. apps/studio/tests/helpers.test.ts:38
- Draft comment:
Test updates: verify that all expected cases for camelCase conversion (formerly toCamelCase) still pass with lodash's implementation. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to verify that all expected cases for camelCase conversion still pass with lodash's implementation. This falls under the rule of not asking the PR author to ensure the behavior is intended or to make sure their change is tested. Therefore, this comment should be removed.
4. packages/utility/src/string.ts:9
- Draft comment:
Note: The custom toCamelCase function has been removed. Confirm that no consumers rely on its specific conversion logic which may differ from lodash's camelCase. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. apps/studio/electron/main/assets/styles.ts:10
- Draft comment:
Good to see the migration from a custom toCamelCase to lodash's camelCase. Please double-check that lodash’s implementation covers all edge cases expected by the previous version. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:81
- Draft comment:
Consider calling e.preventDefault() inside the onKeyDown handler when Enter is pressed to avoid any unintended default behavior. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:80
- Draft comment:
Ensure that disabling the input when editedName is 'DEFAULT' meets the intended UX. If a case‐insensitive check is needed, normalize the value before comparison. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%
None
8. apps/studio/tests/helpers.test.ts:38
- Draft comment:
The tests for camelCase are comprehensive. Just ensure that lodash’s camelCase behavior remains consistent with your naming conventions compared to the previous custom implementation. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. packages/utility/src/string.ts:9
- Draft comment:
The removal of toCamelCase from the utility module is acceptable given the switch to lodash's implementation. Ensure that all consumers of toCamelCase have been updated to use lodash's camelCase to avoid breaking changes. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_swH6FaFfxNSB759c
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Kitenite
approved these changes
Mar 25, 2025
| import { toNormalCase } from '@onlook/utility'; | ||
| import { camelCase } from 'lodash'; | ||
|
|
||
| describe('toNormalCase', () => { |
There was a problem hiding this comment.
Thanks for the unit tests :)
| import { Tooltip, TooltipContent, TooltipTrigger } from '@onlook/ui/tooltip'; | ||
| import { toNormalCase, type Color } from '@onlook/utility'; | ||
| import { camelCase } from 'lodash'; |
There was a problem hiding this comment.
Using lodash instead so we don't have repeat code
There was a problem hiding this comment.
❌ Changes requested. Incremental review on a4a0556 in 1 minute and 52 seconds
More details
- Looked at
105lines of code in4files - Skipped
0files when reviewing. - Skipped posting
10drafted comments based on config settings.
1. apps/studio/package.json:61
- Draft comment:
Switching from lodash to lodash-es: Ensure that all module imports are updated accordingly and that the ESM module is fully compatible with your codebase. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to ensure compatibility and update module imports, which falls under the rule of not asking the author to ensure or verify things. It doesn't provide a specific suggestion or point out a specific issue with the code.
2. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/index.tsx:8
- Draft comment:
Import of BrandPopoverPicker now replaces PopoverPicker. Verify that BrandPopoverPicker fully meets the requirements and props usage. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/index.tsx:200
- Draft comment:
Consider cleaning up commented out PopoverPicker code if it's no longer needed to reduce confusion. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. apps/studio/src/routes/editor/LayersPanel/index.tsx:156
- Draft comment:
Removal of the 'hidden' class on the button: Confirm that the button’s visibility logic is intended to change in all contexts. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm their intention regarding the visibility logic of a button. This violates the rule against asking the author to confirm their intention or ensure behavior is intended. The comment does not provide a specific code suggestion or point out a clear issue.
5. bun.lock:10
- Draft comment:
Lockfile updates: Ensure consistent dependency updates (e.g., bun-types and @types/bun versions) across the project. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. apps/studio/package.json:61
- Draft comment:
Switched from 'lodash' to 'lodash-es'. Ensure the codebase supports ES modules and that all import statements or usages are updated accordingly. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/index.tsx:200
- Draft comment:
The rendered component now uses BrandPopoverPicker while the PopoverPicker block is commented out. Consider deleting the obsolete commented-out PopoverPicker code for clarity. - Reason this comment was not posted:
Marked as duplicate.
8. apps/studio/src/routes/editor/LayersPanel/index.tsx:156
- Draft comment:
The 'hidden' class was removed from the button. Verify that this adjustment in visibility is intentional and that no additional logic is needed to control it. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
This appears to be a deliberate UI change to show the Brand tab button that was previously hidden. The code shows a complete and consistent implementation - the button has proper styling, click handlers, and integration with the tab system. There's no evidence of any issues or missing logic. Asking for verification of intentional UI changes violates our review rules.
The button could potentially need to be hidden under certain conditions that aren't visible in this file. The removal of 'hidden' might affect the overall layout or user experience.
The code shows this is a standard navigation button implemented like others in the file. If there were conditions requiring it to be hidden, they would likely be visible in the conditional rendering logic here. UI visibility changes are explicitly excluded from our review scope.
Delete this comment as it only asks for verification of an intentional UI change, which violates our rules about not asking authors to confirm their intentions or commenting on UI changes.
9. bun.lock:10
- Draft comment:
Versions for '@types/bun' and 'bun-types' have been bumped. Confirm that these updates are compatible with the rest of the project. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to confirm compatibility of dependency updates, which is against the rules. It does not provide a specific suggestion or point out a specific issue with the code.
10. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/index.tsx:1
- Draft comment:
The PR description mentions saving and closing the modal on Enter as well as onboarding documentation updates, but these changes aren’t evident in the diff. Ensure that all intended updates are included. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_GpGB6lP0CHmuIOI9
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -8,7 +8,7 @@ import { Color, isColorEmpty } from '@onlook/utility'; | |||
| import { observer } from 'mobx-react-lite'; | |||
| import { memo, useCallback, useEffect, useMemo, useState } from 'react'; | |||
| // import { BrandPopoverPicker } from './ColorBrandPicker'; | |||
| import { PopoverPicker } from './Popover'; | |||
| import { BrandPopoverPicker } from './ColorBrandPicker'; | |||
There was a problem hiding this comment.
Replaced the import of PopoverPicker with BrandPopoverPicker. Remove any outdated commented code to keep the file clean.
zongkelong
pushed a commit
to zongkelong/coolook
that referenced
this pull request
Mar 27, 2025
…slation_zh * 'main' of https://github.com/onlook-dev/onlook: Add brand tests (onlook-dev#1675) Disable elide lines for precommit hooks (onlook-dev#1674) App Panel, Installed Apps, App Details (onlook-dev#1608) Modifying configs for linux packages (onlook-dev#1664) fix: prevent duplicate variable in tailwind (onlook-dev#1671)
ml-delaurier
pushed a commit
to ml-delaurier/nolook
that referenced
this pull request
Apr 23, 2025
* fix: prevent duplicate variable in tailwind * fix color brand name * fix theme * Use lodash camel case
t1c1
pushed a commit
to t1c1/onlookbotcodes
that referenced
this pull request
Jun 5, 2025
* fix: prevent duplicate variable in tailwind * fix color brand name * fix theme * Use lodash camel case
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Screen.Recording.2025-03-25.at.17.26.04.mov
Additional Notes
Important
Fixes duplicate variable creation in Tailwind and refactors color name handling with added tests.
createTailwindColorVariable()instyles.tsby checking existing variables.ColorPopoverinColorPopover.tsx.toCamelCase()withcamelCase()fromlodashinhelpers.tsandstyles.ts.toNormalCase()inColorPalletGroup.tsxandColorPopover.tsx.toNormalCase()andcamelCase()inhelpers.test.ts.lodashtolodash-esinpackage.jsonandbun.lock.ColorInput/index.tsxto useBrandPopoverPicker.This description was created by
for a4a0556. It will automatically update as commits are pushed.