fix: add validation when create new color group #1703
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
spartan-vutrannguyen
force-pushed
the
fix/add-validation-when-create-new-color-group
branch
from
d0d1040 to
5ad78bb
Compare
![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! Incremental review on 5ad78bb in 1 minute and 11 seconds
More details
- Looked at
163lines of code in2files - Skipped
0files when reviewing. - Skipped posting
10drafted comments based on config settings.
1. apps/studio/electron/main/assets/styles.ts:349
- Draft comment:
Good use of async formatting. Confirm that formatContent handles error cases. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
2. apps/studio/electron/main/assets/styles.ts:363
- Draft comment:
Consistent async file updates for config file. Ensure writeFile handles errors similarly to fs.writeFileSync. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
3. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/GroupNameInput.tsx:41
- Draft comment:
Empty input check moved above regex validation. This is a better UX ordering. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%
None
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/GroupNameInput.tsx:106
- Draft comment:
Added 'max-w-xs' to tooltip content for improved responsiveness. Verify that tooltip width is as expected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/GroupNameInput.tsx:47
- Draft comment:
Validation now uses camelCase(input) for comparison; ensure that transforming the input does not undesirably alter names in other parts of the app. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
6. apps/studio/electron/main/assets/styles.ts:49
- Draft comment:
Consider validating the format of 'originalKey' before splitting to ensure it contains '-' and avoid potential runtime errors. - Reason this comment was not posted:
Comment was on unchanged code.
7. apps/studio/electron/main/assets/styles.ts:349
- Draft comment:
Good job replacing synchronous fs.writeFileSync calls with async formatting and writeFile; this improves consistency and error handling. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, as it praises the change without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest improvements.
8. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/GroupNameInput.tsx:41
- Draft comment:
Nice improvement: the early empty-check ensures that a blank group name returns a clear error message. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply acknowledges a positive change without offering any specific guidance or questions.
9. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/GroupNameInput.tsx:106
- Draft comment:
Adding 'max-w-xs' to TooltipContent is a good touch to control tooltip width for better UI consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, as it just states that addingmax-w-xstoTooltipContentcontrols tooltip width for better UI consistency. It doesn't provide any actionable feedback or suggestions for improvement.
10. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/GroupNameInput.tsx:1
- Draft comment:
The PR title and description mention onboarding docs updates, but the changes only address validation and file formatting. Ensure that the documentation updates are included if required. - 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_wWGyVuAEiuPFJZC9
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! Reviewed everything up to d0d1040 in 2 minutes and 25 seconds
More details
- Looked at
512lines of code in6files - Skipped
0files when reviewing. - Skipped posting
15drafted comments based on config settings.
1. apps/studio/electron/main/assets/helpers.ts:64
- Draft comment:
Good enhancement: key extraction now handles Identifier, NumericLiteral, and StringLiteral. Consider logging unsupported key types if needed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/studio/electron/main/assets/styles.ts:349
- Draft comment:
Switched synchronous file writes to async; ensure formatContent and writeFile handle errors gracefully. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:41
- Draft comment:
Added validation for special characters in color names. Confirm that disallowing all special characters (except spaces) meets design requirements. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/GroupNameInput.tsx:42
- Draft comment:
Regex in group name validation allows spaces; ensure trimming or feedback for unintended trailing spaces if needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:26
- Draft comment:
The useEffect calling themeManager.scanConfig has an empty dependency array; ensure that scanConfig doesn't need to rerun if themeManager changes. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:106
- Draft comment:
Renaming now uses GroupNameInput component; verify that error messaging and state resets are consistent with UX. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. apps/studio/electron/main/assets/helpers.ts:62
- Draft comment:
Good enhancement: now handling Identifier, NumericLiteral, and StringLiteral keys in extractObject. Consider logging or a comment for unsupported key types in case future key types are encountered. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
8. apps/studio/electron/main/assets/styles.ts:349
- Draft comment:
Switching from synchronous fs.writeFileSync to asynchronous writeFile with formatContent improves consistency. Consider adding error handling (or try/catch) for potential rejections from formatContent. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
9. apps/studio/electron/main/assets/styles.ts:51
- Draft comment:
When splitting 'originalKey' by '-' in updateTailwindColorConfig, ensure that group names do not include dashes unexpectedly. If dashes are allowed, the logic may need to be revisited. - Reason this comment was not posted:
Comment looked like it was already resolved.
10. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:130
- Draft comment:
Nice refactor: extracting GroupNameInput simplifies the component and centralizes name validation logic. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
11. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:41
- Draft comment:
The added validation disallowing special characters in color names is good. Consider also trimming the input to avoid errors from leading/trailing spaces. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
12. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/GroupNameInput.tsx:33
- Draft comment:
The GroupNameInput component’s validation logic is clear and reusable. For very large themes, you might consider memoizing or caching the computed list of existing group names from the theme manager. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
13. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:132
- Draft comment:
Using GroupNameInput for adding new groups improves consistency and reusability compared to inline inputs. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
14. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:59
- Draft comment:
There is an inconsistency in naming: the component is named 'BrandPalletGroup' (and the file is 'ColorPalletGroup.tsx') but there is a prop 'isDefaultPalette' that uses 'Palette'. Consider renaming the component and file to 'BrandPaletteGroup' (and 'ColorPaletteGroup') for consistency if that is the intended meaning. - 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.
15. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:75
- Draft comment:
The variable 'existedName' (line 75) might be a typographical error. Consider renaming it to 'existingNames' or a similarly clear term for better readability and consistency. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_Ttd6W5CiMCxE3ebE
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 a7164d1 in 1 minute and 28 seconds
More details
- Looked at
248lines of code in4files - Skipped
0files when reviewing. - Skipped posting
11drafted comments based on config settings.
1. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:47
- Draft comment:
Consider converting initialName to camelCase (or vice versa) when comparing, to avoid potential case mismatches. - 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.
2. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:69
- Draft comment:
The handleSubmit logic triggers onCancel if name is unchanged; confirm this is the intended UX behavior. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:32
- Draft comment:
The handleNameChange callback uses the potentially stale 'editedColor' state; consider ensuring it’s up-to-date. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:97
- Draft comment:
Consider updating the placeholder from 'Enter group name' to 'Enter color name' for consistency with validation messages. - Reason this comment was not posted:
Comment was on unchanged code.
5. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:47
- Draft comment:
Compare normalized names for the unchanged check. Consider using camelCase(initialName) instead of initialName directly for a consistent comparison. - 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.
6. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:47
- Draft comment:
The onOpenChange callback immediately submits the current editedName on popover close. Consider verifying if this auto-submission is desired to avoid premature or unintended name changes. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment is asking to verify intention, which violates our rules. It's not pointing out a clear bug or suggesting a specific improvement. The code appears to be intentionally designed this way - when popover closes, it commits the changes. This is a common pattern in UI components. The validation is now handled by ColorNameInput component.
Maybe there could be a real issue with auto-submission if the user accidentally clicks outside the popover? Maybe this is a UX concern worth addressing?
Even if there are UX considerations, the comment isn't making a clear suggestion for improvement. It's just asking to verify intention, which our rules explicitly say not to do.
Delete the comment because it only asks to verify intention without suggesting a specific improvement, which violates our review rules.
7. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:74
- Draft comment:
Consider normalizing the existing color names (e.g., mapping them with camelCase) before passing them to ColorNameInput for duplicate detection. - 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.
8. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:97
- Draft comment:
Typographical error: The placeholder text on the input still uses "group name" instead of "color name". Please update the placeholder to "Enter color name" for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
9. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:58
- Draft comment:
Typo issue: The component and filename use 'Pallet' (as seen in 'BrandPalletGroup' and the file name 'ColorPalletGroup.tsx'). Consider using 'Palette' if this was intended to represent a group of colors. - 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.
10. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:74
- Draft comment:
Typographical suggestion: The variable 'existedName' might be better named 'existingNames' to more clearly represent the collection derived from 'colors.map(...)'. - 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.
11. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:9
- Draft comment:
Minor typo suggestion: The component and file 'ColorPalletGroup' might be intended as 'ColorPaletteGroup' if referring to color palettes. Please verify if this naming is intentional. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_75Hr5JEVi1NwDXBu
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 22d5ac0 in 57 seconds
More details
- Looked at
75lines of code in2files - Skipped
0files when reviewing. - Skipped posting
11drafted comments based on config settings.
1. apps/studio/electron/main/assets/styles.ts:168
- Draft comment:
Good switch to async formatting and writing. Consider adding error handling around 'formatContent' and 'writeFile' calls. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. apps/studio/electron/main/assets/styles.ts:195
- Draft comment:
Removal of the 'retainLines' option in generate() is intentional? Double-check if line retention is needed for clarity in diffs. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. apps/studio/electron/main/assets/styles.ts:847
- Draft comment:
Using an empty options object in generate() for deleteColorGroup and updateDefaultTailwindColor may affect formatting consistency. Confirm format requirements. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:34
- Draft comment:
Removing trim() from the empty check in validateName is acceptable since trimmed value is passed from onChange. Ensure consistent trimming across usage. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/studio/electron/main/assets/styles.ts:168
- Draft comment:
Good shift from synchronous fs.writeFileSync to async writeFile with formatting. Ensure that writeFile properly handles errors. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. apps/studio/electron/main/assets/styles.ts:191
- Draft comment:
Consistently using async file writes and formatting for the config file update. Removal of 'retainLines' appears intentional; ensure downstream formatting meets expectations. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. apps/studio/electron/main/assets/styles.ts:307
- Draft comment:
Removal of 'retainLines: true' in the generate() call is consistent with later formatting. Verify this change preserves the intended code style. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
8. apps/studio/electron/main/assets/styles.ts:847
- Draft comment:
In deleteColorGroup, switching to default generator options (empty options object) is acceptable if formatContent standardizes the output. Confirm that no formatting anomalies arise. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. apps/studio/electron/main/assets/styles.ts:1008
- Draft comment:
Similarly, in updateDefaultTailwindColor the generator options were removed in favor of post-formatting. Ensure this doesn’t affect any intended inline formatting. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
10. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:34
- Draft comment:
Since the input value is trimmed before validation in handleChange, checking for an empty string (value === '') is sufficient. This makes the intent clear. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
11. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:66
- Draft comment:
Applying .trim() to newValue before passing it to validateName ensures consistent and correct validation of empty or whitespace-only input. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_UtOCOClJJzqPs0EX
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Kitenite
reviewed
Apr 2, 2025
| ); | ||
| }; | ||
|
|
||
| export default ColorNameInput; |
There was a problem hiding this comment.
Let's start using direct export instead of default export
There was a problem hiding this comment.
i.e.
export const ColorNameInput = ({
| disabled?: boolean; | ||
| } | ||
|
|
||
| const ColorNameInput = ({ |
There was a problem hiding this comment.
Thanks for refactoring this. Much better.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 2a4e676 in 1 minute and 51 seconds
More details
- Looked at
61lines of code in3files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. apps/studio/electron/main/assets/styles.ts:549
- Draft comment:
Regex update: simplified pattern may over-match. Confirm that removing the prefix check won’t affect intended utility class targeting. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The regex is used to find Tailwind utility classes that reference a color name. The original pattern was more restrictive, only matching lowercase letters and hyphens before the color name. The new pattern will match any characters before the hyphen. However, this is being used in a very specific context - matching within @apply rules in Tailwind CSS, where utility class names follow a strict format. The looser pattern is unlikely to cause issues in practice since invalid class names wouldn't be valid Tailwind utilities anyway.
The comment raises a valid technical point about pattern matching becoming less precise. In theory, this could lead to false positives.
While technically correct, the context makes this a non-issue - we're only matching within Tailwind @apply rules where class names follow strict conventions. The build system would catch any invalid class names.
The comment should be deleted. While technically accurate, it raises a theoretical concern that isn't practically relevant in this specific context of Tailwind CSS class matching.
2. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:95
- Draft comment:
onBlur now calls the provided callback with the current value instead of auto-submitting. Verify that this behavior meets the UX requirements. - 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 if the behavior meets UX requirements, which is against the rules. It does not provide a specific code suggestion or point out a potential issue that needs confirmation. Therefore, it should be removed.
3. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:137
- Draft comment:
onBlur handler in the ColorNameInput for renaming now calls onRename and resets renaming state. Confirm that triggering rename on blur is the desired flow. - 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 onBlur handler behavior. This violates the rule against asking for confirmation of intention. The comment does not provide a specific suggestion or point out a clear issue, so it should be removed.
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:40
- Draft comment:
Numeric validation: The logic allows fully numeric names but disallows names starting with numbers mixed with letters. Verify this meets the requirement for allowing numeric variables in sub-colors. - 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:549
- Draft comment:
Regex updated to/-${originalName}\b/now omits the preceding character class. Ensure this change correctly supports numeric sub-colors without causing unintended matches. - 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 is speculative - it says "ensure this change correctly supports..." without providing evidence of an actual issue. The original regex was likely overly restrictive by requiring letters/hyphens before the hyphen. The new regex simply matches any hyphen followed by the color name, which seems reasonable for matching Tailwind utility classes. The comment is asking for verification rather than pointing out a clear problem.
The regex change could potentially match unintended cases where the hyphen is preceded by numbers or other characters. The original regex may have been intentionally restrictive.
While the regex is now more permissive, there's no clear evidence this would cause issues. Tailwind utility classes commonly use hyphens as separators, so matching on the hyphen boundary is a reasonable approach. The comment is speculative and asks for verification rather than demonstrating a concrete problem.
The comment should be deleted as it is speculative and asks for verification without providing evidence of an actual issue. The regex change appears reasonable for the use case.
6. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:137
- Draft comment:
Added an onBlur handler for ColorNameInput that calls onRename and resets renaming state. Verify that the input is sufficiently validated and trimmed consistently with onSubmit. - 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 input is validated and trimmed consistently, which violates the rule against asking the author to ensure behavior is intended or tested. It does not provide a specific suggestion or ask for a specific test to be written.
7. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:99
- Draft comment:
Typographical error: The placeholder text on line 99 says 'Enter group name', which seems inconsistent with the rest of the component that refers to colors (e.g., error messages like 'Color name cannot be empty'). Please consider updating it to 'Enter color name' 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.
8. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:1
- Draft comment:
It appears that the file and component are named 'ColorPalletGroup' and 'BrandPalletGroup'. If the intention was to refer to a group of colors, you might consider renaming them to 'ColorPaletteGroup' and 'BrandPaletteGroup' to avoid confusion with 'pallet', which typically means a platform for moving things. - 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.
9. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:74
- Draft comment:
Consider renaming 'existedName' (line 74) to something like 'existingNames' for clarity and to better convey that these are names that already exist. - 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_kRyrLtJ3uwSkWk5t
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.
apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f61f400 in 1 minute and 7 seconds
More details
- Looked at
55lines of code in3files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:72
- Draft comment:
In handleSubmit, use consistent camelCase conversion for initialName. Compare against camelCase(initialName) instead of initialName for consistency with validateName. - 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.
2. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:72
- Draft comment:
Use a consistent normalization check: compare camelCase(inputValue) with camelCase(initialName) (not raw initialName) to avoid mismatches. - 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.
3. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:17
- Draft comment:
Exporting as a named export (instead of default) is a good change. Ensure all import statements across the project are updated accordingly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:1
- Draft comment:
Typographical inconsistency: The component and file are named 'ColorPalletGroup' while other parts (e.g., CSS classes with 'palette' and prop names like 'isDefaultPalette') use 'palette'. Consider renaming to 'ColorPaletteGroup' for consistency and 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.
5. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:74
- Draft comment:
Minor typographical suggestion: The variable name 'existedName' might be better as 'existingNames' (or similar) for clarity, as it represents a list of already existing color names. - 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.
6. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:9
- Draft comment:
Typographical suggestion: Consider checking if 'BrandPalletGroup' (and the associated file name 'ColorPalletGroup') is intentionally spelled with 'Pallet'. If it's meant to represent a group of colors (a color palette), it might be clearer as 'BrandPaletteGroup'. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_oAiqeTeBKR4V92x6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
spartan-vutrannguyen
force-pushed
the
fix/add-validation-when-create-new-color-group
branch
from
f61f400 to
ffcff67
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on ffcff67 in 1 minute and 18 seconds
More details
- Looked at
68lines of code in4files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:72
- Draft comment:
Consider comparing normalized names for consistency. On line 49, the check usescamelCase(initialName)while here it compares againstinitialNamedirectly. UsecamelCase(initialName)in the condition. - 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.
2. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:72
- Draft comment:
Ensure consistent comparison by also normalizing initialName in handleSubmit. In validateName (line 49) you compare using camelCase(initialName), but in handleSubmit you compare camelCase(inputValue) against initialName directly. - 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.
3. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:74
- Draft comment:
Consider renaming the variable 'existedName' to 'existingNames' for clarity and consistency with prop naming (as used in ColorNameInput). - 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.
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:95
- Draft comment:
Consider passing the trimmed input value in the onBlur callback (e.g. onBlur(inputValue.trim())) to ensure consistent validation. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:47
- Draft comment:
Review the behavior where onOpenChange triggers handleNameChange on popover close, which in turn calls onColorChangeEnd. Confirm that this mechanism is intentional and does not lead to duplicate updates. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:1
- Draft comment:
It looks like the file and component are named 'ColorPalletGroup', but in the context of color collections, the correct term is usually 'Palette'. Consider renaming it (and updating references) to 'ColorPaletteGroup' if this isn't intentional. - 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.
7. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:74
- Draft comment:
The variable name 'existedName' might be a typo. Consider renaming it to 'existingNames' (and adjust its usage accordingly) to more clearly indicate that it represents the names that currently exist. - 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.
8. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:15
- Draft comment:
Typo: The property 'existedName' appears to be a typo. It would be clearer to rename it to 'existingNames' (and update its type accordingly) to match the prop passed to ColorNameInput at line 67. - 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_YHoyRgVUo5DM6JAG
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 81c3fe0 in 1 minute and 46 seconds
More details
- Looked at
125lines of code in3files - Skipped
0files when reviewing. - Skipped posting
12drafted comments based on config settings.
1. apps/studio/electron/main/assets/styles.ts:92
- Draft comment:
Good refactor: using isValidTailwindConfigProperty reduces duplicated key checks. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/studio/electron/main/assets/styles.ts:818
- Draft comment:
Verify that changing from startsWith to endsWith for CSS variable deletion is intentional. - 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 comment violates the rule about not asking authors to verify their intentions. The comment starts with "Verify that..." which is explicitly called out as likely not useful. Additionally, this appears to be a deliberate change - when deleting a specific color, we want to match the full variable name at the end to avoid partial matches. For example, --primary-light should not match when deleting --primary.
The change from startsWith to endsWith could potentially break existing functionality if there was a specific reason for using startsWith before. Maybe there are edge cases I'm not seeing.
The code change appears intentional and logical - endsWith provides more precise matching for specific color variables. If there were edge cases requiring startsWith, they should be raised with concrete examples, not speculative verification requests.
Delete this comment. It violates the rule about asking for verification, and the code change appears to be an intentional improvement to the variable matching logic.
3. apps/studio/electron/main/assets/styles.ts:1028
- Draft comment:
Nice addition of helper isValidTailwindConfigProperty. Ensure all property types are properly handled. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:44
- Draft comment:
Regex updated to allow numeric names with spaces. Confirm that /^[0-9\s]+$/ suits all valid cases. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:96
- Draft comment:
Updated placeholder text to 'Enter color name'—clearer for end users. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:350
- Draft comment:
BrandColor prop now uses dynamic text (group title and count); please confirm this meets the intended UI behavior. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. apps/studio/electron/main/assets/styles.ts:92
- Draft comment:
Nice refactoring using isValidTailwindConfigProperty for property matching. Please verify that numeric keys are handled correctly in all cases. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. apps/studio/electron/main/assets/styles.ts:818
- Draft comment:
Changing the CSS line filter from startsWith to endsWith. Confirm this logic correctly targets only the intended CSS variable without false positives. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
9. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:44
- Draft comment:
Updated regex now allows numbers and spaces. Ensure that the error message ('Color name cannot start with a number') aligns with the new validation logic for numeric-only names. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
10. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:99
- Draft comment:
Updated placeholder text to 'Enter color name' makes intent clearer. Looks good. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
11. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:349
- Draft comment:
Changed brandColor value to include title and color count. Verify that this dynamic naming meets UI/UX requirements consistently. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
12. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:58
- Draft comment:
Typographical Issue: The component and file name use 'Pallet', which may be a typo. Consider renaming to 'Palette' for consistency with typical terminology in color-related features. - 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_qPErKYNYOIkuGLD4
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 11c20d1 in 1 minute and 39 seconds
More details
- Looked at
17lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. apps/studio/src/lib/editor/engine/theme/index.ts:453
- Draft comment:
Consider edge cases: if colorName is numeric, using group.length + 1 may produce duplicate names if numbers are already present. Ensure that the new name is unique. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. apps/studio/src/lib/editor/engine/theme/index.ts:453
- Draft comment:
Document behavior: Explain why numeric color names get a suffix using group length and whether that reliably avoids collisions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking for an explanation of a specific behavior in the code, which is not allowed according to the rules. It does not provide a specific code suggestion or ask for a test to be written. Therefore, it should be removed.
3. apps/studio/src/lib/editor/engine/theme/index.ts:453
- Draft comment:
Ensure that 'group' is defined before accessing 'group.length' in the newName generation. If 'group' is undefined, this could lead to a runtime error. - 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 code is actually well protected against undefinedgroup. Ifgroupwas undefined, the code would throw earlier at line 448 with thefind()call. Additionally, ifcolorToDuplicateis not found, there's an explicit error thrown on line 451. By the time we reach line 456, we knowgroupexists and is defined.
Could there be edge cases wheregroupbecomes undefined between lines 447 and 456? Could race conditions affect this?
This is a synchronous code block with no async operations between the group definition and usage. The code is within a try-catch block and has proper error handling. Race conditions are not possible here.
The comment should be deleted. The code already has proper null checks and error handling, making this warning unnecessary.
4. apps/studio/src/lib/editor/engine/theme/index.ts:453
- Draft comment:
Consider extracting the duplicate color name generation logic (handling numeric vs non-numeric names) into a helper function for better clarity and consistency. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
5. apps/studio/src/lib/editor/engine/theme/index.ts:353
- Draft comment:
Typo: The comment on this line says 'Base on the class name, find the styles to update'. It would be clearer if it was written as 'Based on the class name, find the styles to update'. - 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_kzpV7xgPyr6MhJfk
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 4d773f1 in 1 minute and 44 seconds
More details
- Looked at
70lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. apps/studio/electron/main/assets/styles.ts:222
- Draft comment:
Consider refactoring the repeated identifier/numeric literal checks into a helper function. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
2. apps/studio/electron/main/assets/styles.ts:222
- Draft comment:
When renaming, converting a numeric literal key to an Identifier may change its type. Confirm that this transformation preserves the intended behavior and consider extracting this logic into a helper to avoid duplication. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment raises a valid point about type conversion, but it's speculative ("may change its type") and asks for confirmation ("Confirm that..."). The code already handles both types explicitly and converts between them deliberately. The suggestion to extract logic is vague and not clearly necessary.
The type conversion appears intentional and well-handled in the code. The comment doesn't point to any specific issue or bug.
While the comment identifies a real code change, it's asking for confirmation rather than pointing out a clear problem, and the code already handles the type conversion explicitly.
Delete this comment as it violates the rules by asking for confirmation and making speculative suggestions without identifying a clear issue.
3. apps/studio/electron/main/assets/styles.ts:244
- Draft comment:
The inline extraction of the key value for nested properties (using a ternary check for Identifier vs NumericLiteral) is repeated. Consider refactoring this logic into a helper function (e.g., getKeyValue) for improved readability and maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. apps/studio/electron/main/assets/styles.ts:336
- Draft comment:
Debug console.log statements are present. Remove or replace them with a proper logging mechanism before merging to production. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_CSlpk4sLF7WdGALG
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.
spartan-vutrannguyen
force-pushed
the
fix/add-validation-when-create-new-color-group
branch
from
4d773f1 to
4793a3f
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4793a3f in 2 minutes and 6 seconds
More details
- Looked at
61lines of code in1files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. apps/studio/electron/main/assets/styles.ts:222
- Draft comment:
Refactor suggestion: Extract a helper to obtain a key’s string value from either an Identifier or NumericLiteral. This would avoid duplicate logic and improve readability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. apps/studio/electron/main/assets/styles.ts:232
- Draft comment:
Review update: When renaming numeric literal keys, the code creates a new Identifier object. Confirm that it’s acceptable to lose the original NumericLiteral type. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. apps/studio/electron/main/assets/styles.ts:245
- Draft comment:
Consistency note: The nestedProp key conversion uses similar logic. Consider using the same helper function for both parent and nested keys to reduce duplication. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. apps/studio/electron/main/assets/styles.ts:222
- Draft comment:
Ensure that converting a NumericLiteral key to an Identifier when renaming is intentional. If preserving the numeric literal is desired, consider handling it differently. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. apps/studio/electron/main/assets/styles.ts:267
- Draft comment:
When building dynamic RegExp patterns, escape any special regex characters in derived strings (e.g. parentKey) to avoid unintended matches. - Reason this comment was not posted:
Comment was on unchanged code.
6. apps/studio/electron/main/assets/styles.ts:252
- Draft comment:
Consider refactoring the repeated logic for extracting a key’s string value from either an Identifier or NumericLiteral into a utility function for improved maintainability. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
7. apps/studio/electron/main/assets/styles.ts:941
- Draft comment:
Typo in comment: The phrase "Update the specific shade base on tailwinds color scale" should be reworded to "Update the specific shade based on Tailwind's color scale". - 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_6T7k0ax8vSHRbeWm
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Kitenite
reviewed
Apr 3, 2025
|
|
||
| const newName = `${colorName}Copy`; | ||
| // If the color name is a number, we need to add a suffix to the new color name | ||
| const newName = isNaN(Number(colorName)) |
There was a problem hiding this comment.
This doesn't handle if the group color + group.length already exist.
Reproduce: have 3 colors, name one "Test 3", create a new color , it'll make it "Test 3", doesn't validate. If we create it, nothing happens
There was a problem hiding this comment.
Built-in.Retina.Display.mp4
Kitenite
reviewed
Apr 3, 2025
| return 'Color name cannot be empty'; | ||
| } | ||
|
|
||
| // Allow full numbers (e.g. "123") but not allow names starting with numbers (e.g. "1abc") |
There was a problem hiding this comment.
Comment no longer relevant
There was a problem hiding this comment.
❌ Changes requested. Incremental review on b428d75 in 2 minutes and 1 seconds
More details
- Looked at
34lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:128
- Draft comment:
Consider memoizing generateUniqueColorName or storing its result in state when adding a new color, to avoid generating different names on every render. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The function is only called when rendering the ColorPopover for a new color. Even if it generates a different name on each render, this doesn't matter because: 1) The user will be actively choosing a name anyway, this is just an initial value 2) The ColorPopover component likely maintains its own internal state once mounted 3) The random name is just a temporary placeholder until the user finalizes the color.
I could be wrong about the ColorPopover's internal state management. Maybe generating different names causes visual flickering or affects the component's ability to maintain state between renders.
Even if the ColorPopover doesn't maintain internal state, generating different random names during the brief period when adding a new color is not a significant issue that impacts functionality. The user will immediately be choosing their own name anyway.
This comment suggests an optimization that would add complexity without solving any real problem. The current implementation is fine.
2. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:358
- Draft comment:
Using generateUniqueColorName() directly in the prop can lead to inconsistent values if the component re-renders. Consider caching the generated name. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The ColorPopover component is only rendered when isAddingNewColor is true. When the color is saved, isAddingNewColor is set to false. If the user cancels, isAddingNewColor is also set to false. So this component is unmounted after each use. When adding a new color again, we actually want a new random name. Having different random names on re-render during the component's brief lifetime doesn't cause any issues.
I could be wrong about the component lifecycle. Maybe there are cases where ColorPopover persists longer than I think and the changing name causes UX issues.
Even if the component does persist longer, having a changing random name while picking a new color isn't a serious issue - the user will type their own name anyway. This seems like premature optimization.
The comment raises a theoretical issue but in practice, the changing random name doesn't cause any real problems. The comment should be removed.
3. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:19
- Draft comment:
The import of VALID_DATA_ATTR_CHARS appears unused. Consider removing it to keep the code clean. - Reason this comment was not posted:
Marked as duplicate.
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:128
- Draft comment:
generateUniqueColorName is called during render, which generates a new random ID on every render. Consider storing the value in state or memoizing it to ensure the placeholder remains consistent while creating a new color. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The random name is only used as an initial placeholder value for a new color being added. The user will likely change this name anyway. Even if it changes between renders, it doesn't affect functionality since it's just a temporary placeholder. The ColorPopover component handles the actual name input and state management.
The comment correctly identifies a technical issue - the random name will change on each render. But am I being too dismissive of the UX impact this could have?
While technically correct, this is a minor UX issue at worst since it only affects the initial placeholder name shown briefly when adding a new color. The actual color naming functionality works correctly.
The comment should be deleted because while technically accurate, the issue is too minor to warrant a code change. The random name is just a temporary placeholder and doesn't affect core functionality.
5. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:60
- Draft comment:
The component name and file name use 'Pallet' (e.g., BrandPalletGroup, ColorPalletGroup.tsx). If the intended meaning is a collection of colors, it might be better to rename to 'Palette' (e.g., BrandPaletteGroup) to avoid confusion. - 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.
6. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:76
- Draft comment:
The variable name 'existedName' may be a typographical error. Consider renaming it to 'existingNames' (or 'existingName') for clarity and consistency. - 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_08674u4LUgFPbJQE
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.
| import { camelCase } from 'lodash'; | ||
| import { ColorNameInput } from './ColorNameInput'; | ||
| import { customAlphabet } from 'nanoid/non-secure'; | ||
| import { VALID_DATA_ATTR_CHARS } from '/common/helpers/ids'; |
There was a problem hiding this comment.
Unused import: VALID_DATA_ATTR_CHARS is imported but not used. Remove if unnecessary.
Suggested change
| import { VALID_DATA_ATTR_CHARS } from '/common/helpers/ids'; |
spartan-vutrannguyen
force-pushed
the
fix/add-validation-when-create-new-color-group
branch
from
b428d75 to
a235314
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a235314 in 33 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:18
- Draft comment:
Removed unused import for VALID_DATA_ATTR_CHARS. Good cleanup, but ensure the required validation logic for new color groups is implemented elsewhere. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:18
- Draft comment:
Removed unused import 'VALID_DATA_ATTR_CHARS'. This cleanup is fine if no validation logic depends on it. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:59
- Draft comment:
The component is named 'BrandPalletGroup', but the file is named 'ColorPalletGroup.tsx' and props use 'isDefaultPalette'. This mix of 'Pallet' and 'Palette' can be confusing. If 'Palette' is the intended word, please update the component and file names to be consistent. - 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.
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:75
- Draft comment:
The variable name 'existedName' might be clearer if renamed to 'existingNames' to better reflect that it stores an array of color names. Consider updating it for improved readability. - 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_fSC1KhwNeUCz2Y9D
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 f0ef2f8 in 2 minutes and 1 seconds
More details
- Looked at
39lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. apps/studio/src/lib/editor/engine/theme/index.ts:455
- Draft comment:
The duplicate logic appends a random numeric suffix for numeric color names. If variable names cannot start with a number, consider adding a non-numeric prefix instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment assumes there could be issues with variable names starting with numbers. However, this is a color name in a theme configuration, not a JavaScript variable name. Looking at the rest of the file, there's no evidence that color names must follow variable naming rules. The code already handles numeric color names throughout without issues. The randomId addition seems to be just for uniqueness, not for syntax validity.
I could be wrong about whether color names have syntax restrictions in the theme configuration. There might be downstream processing that requires valid JavaScript identifiers.
The code base already handles numeric color names throughout without adding prefixes, and there's no evidence in the file of any requirement for color names to be valid JavaScript identifiers. The change appears to be working as intended.
The comment should be deleted as it makes assumptions about naming requirements without clear evidence, and the current code appears to be working as intended.
2. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:127
- Draft comment:
The generateUniqueColorName function differentiates between text and numeric titles using isNaN(Number(title)). Verify that this logic properly prevents names that start with a digit, or adjust the validation to check the first character. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The current code checks if the title can be converted to a number using isNaN(Number(title)), while the suggestion checks if it starts with a digit. These are different checks - the current code would treat "123abc" and "abc123" differently than the suggested code. However, there's no clear evidence that either approach is better. The code appears to be working as intended to generate unique names.
I don't see any evidence that the current approach is causing problems. The suggestion seems like a matter of preference rather than fixing a real issue.
Without evidence of actual problems with the current implementation, suggesting a different validation approach isn't helpful.
This comment should be deleted as it suggests a change without clear evidence that the current implementation is problematic.
3. apps/studio/src/lib/editor/engine/theme/index.ts:455
- Draft comment:
Consider using a regex (e.g. /^[0-9]+$/) to check if the entire colorName is numeric instead of isNaN(Number(colorName)). This would more accurately distinguish names that are fully numeric from those that merely start with a digit. - Reason this comment was not posted:
Comment was on unchanged code.
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:128
- Draft comment:
The randomId generation logic here is similar to that in the theme manager. Consider extracting it to a shared helper and using a consistent numeric check (e.g. using a regex) for determining if the title is numeric. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While code reuse is generally good, this suggestion requires knowledge of the theme manager code which isn't shown. The logic here is quite simple and specific to color name generation. Without seeing the theme manager code, I can't verify if extraction would actually improve things. The comment also suggests using regex which may be unnecessary complexity.
I could be undervaluing the benefits of consistency across the codebase. Maybe the theme manager code is very similar and this really would reduce duplication.
Even if there is some duplication, the current code is simple and serves its specific purpose well. Without seeing the other code, extracting it could create unnecessary coupling.
The comment should be removed since we can't verify the similarity with theme manager code, and the suggested changes may add complexity without clear benefit.
5. apps/studio/src/lib/editor/engine/theme/index.ts:3
- Draft comment:
Typo: The imported module name 'ColorPalletGroup' might be a typo and is likely intended to be 'ColorPaletteGroup'. Please confirm and update if necessary. - 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_Wukd8jebJzTl4qW5
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 a049c87 in 1 minute and 7 seconds
More details
- Looked at
56lines of code in3files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. apps/studio/src/lib/editor/engine/theme/index.ts:455
- Draft comment:
Using randomText via customAlphabet is a good fix to avoid duplicate names. Ensure the randomness is sufficient for your use case. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:55
- Draft comment:
Validation now checks provided 'existingNames' before falling back to themeManager names. This improves clarity in error messaging. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:128
- Draft comment:
Changed generateUniqueColorName to use alphabet-only random text for non-numeric titles. Confirm that title being numeric is handled as intended. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. apps/studio/src/lib/editor/engine/theme/index.ts:453
- Draft comment:
Appending a random text suffix for non-numeric color names improves uniqueness. Please verify that this approach meets the requirements and that potential name collisions are handled appropriately. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorNameInput.tsx:55
- Draft comment:
The validation now checks for duplicate names using the provided 'existingNames' list only when it’s non-empty, bypassing the theme manager check. Confirm that this logic covers all duplicate scenarios as intended. - 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 that the logic covers all duplicate scenarios, which violates the rule against asking for confirmation of intention or behavior. It does not provide a specific code suggestion or point out a specific issue.
6. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:127
- Draft comment:
The unique name generator now uses only letters for the random text part, which is a change from including numbers. Ensure that this is consistent with the duplicate naming logic and that numeric titles properly use the numeric suffix. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:59
- Draft comment:
The component's name (BrandPalletGroup) and the file name (ColorPalletGroup.tsx) use 'Pallet', whereas the CSS class uses 'palette' (e.g. 'group/palette'). This inconsistency suggests that 'Palette' might be the intended spelling. Please double-check and correct if necessary. - 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_N4RKVdxANeDmTgdd
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
zongkelong
pushed a commit
to zongkelong/coolook
that referenced
this pull request
Apr 3, 2025
…slation_zh * 'main' of https://github.com/onlook-dev/onlook: fix: add validation when create new color group (onlook-dev#1703) Disable web when running dev (onlook-dev#1718) feat: font panel config (onlook-dev#1666) Add web app (onlook-dev#1702)
ml-delaurier
pushed a commit
to ml-delaurier/nolook
that referenced
this pull request
Apr 23, 2025
* add validation when create new color group * validate input color name * update write file * use common component * fix format file * fix error when edit color * change import * update validate with number name * fix logic when duplicate color * fix error when rename number variable * generate unique color name * rebase main * generate unique id for numeric variable * update random color name
t1c1
pushed a commit
to t1c1/onlookbotcodes
that referenced
this pull request
Jun 5, 2025
* add validation when create new color group * validate input color name * update write file * use common component * fix format file * fix error when edit color * change import * update validate with number name * fix logic when duplicate color * fix error when rename number variable * generate unique color name * rebase main * generate unique id for numeric variable * update random color name
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)
Additional Notes
Important
Adds validation for color group and color creation, ensuring valid and unique names, and updates UI components for color management.
ColorNameInputcomponent for validating color names inColorNameInput.tsx.themeManagerand provided list.ThemeManagerinindex.tsto handle color group creation, renaming, deletion, and duplication.updateTailwindColorConfiginstyles.tsto support numeric variables.generateUniqueColorNameinColorPalletGroup.tsxfor unique color naming.ColorNameInputinColorPalletGroup.tsxandColorPopover.tsxfor name input.ColorNameInput.tsx.ColorPanelinindex.tsxto useColorNameInputfor adding new groups.This description was created by
for a049c87. It will automatically update as commits are pushed.