You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The logic for handling hue shifts in the execute method should be reviewed to ensure that all harmony types produce the correct and expected results. Specifically, verify that the hue adjustments and modulo operations are accurate for edge cases.
lethueShifts: number[]=[];switch(harmonyType){case'analogous':
hueShifts=[-30,30];break;case'complementary':
hueShifts=[180];break;case'splitComplementary':
hueShifts=[150,-150];break;case'triadic':
hueShifts=[120,-120];break;case'tetradic':
hueShifts=[90,180,-90];break;}// Start with the base colorharmonies.push(color);// Generate harmony colorsfor(constshiftofhueShifts){letnewHue=(baseHue+shift)%360;if(newHue<0)newHue+=360;constnewColor=newColor('hsl',[newHue,saturation,lightness],color.alpha);harmonies.push(toColorObject(newColor));}// Ensure we return the requested number of colorsif(harmonies.length>numberOfColors){harmonies=harmonies.slice(0,numberOfColors);}elseif(harmonies.length<numberOfColors){constoriginalLength=harmonies.length;for(leti=originalLength;i<numberOfColors;i++){harmonies.push(harmonies[i%originalLength]);}
The logic for repeating colors to meet the numberOfColors requirement should be reviewed to ensure it handles edge cases correctly, such as when the requested number of colors is less than or equal to the base harmonies.
Add validation for numberOfColors to ensure it is a positive integer
Validate that numberOfColors is a positive integer before using it to slice or repeat the harmonies array to avoid unexpected behavior or infinite loops.
-if (harmonies.length > numberOfColors) {- harmonies = harmonies.slice(0, numberOfColors);-} else if (harmonies.length < numberOfColors) {- const originalLength = harmonies.length;- for (let i = originalLength; i < numberOfColors; i++) {- harmonies.push(harmonies[i % originalLength]);+if (numberOfColors > 0 && Number.isInteger(numberOfColors)) {+ if (harmonies.length > numberOfColors) {+ harmonies = harmonies.slice(0, numberOfColors);+ } else if (harmonies.length < numberOfColors) {+ const originalLength = harmonies.length;+ for (let i = originalLength; i < numberOfColors; i++) {+ harmonies.push(harmonies[i % originalLength]);+ }
}
+} else {+ throw new Error('numberOfColors must be a positive integer');
}
Suggestion importance[1-10]: 10
Why: Validating numberOfColors as a positive integer is essential to prevent logical errors or infinite loops. This suggestion significantly improves the reliability and safety of the code by handling invalid input gracefully.
10
Add a fallback for the alpha value to prevent runtime errors if it is undefined or null
Ensure that the color.alpha property is checked for undefined or null values before using it in the new Color constructor to avoid potential runtime errors.
Why: This suggestion addresses a potential runtime error by providing a default value for the alpha channel when it is undefined or null. It ensures robustness and prevents unexpected crashes, which is critical for the functionality of the code.
9
General
Add a test case to ensure invalid numberOfColors values are handled correctly
Add a test case to verify the behavior when numberOfColors is set to zero or a negative value to ensure the function handles invalid input gracefully.
-test('handles custom number of colors by repeating', async () => {+test('handles invalid numberOfColors gracefully', async () => {
const graph = new Graph();
const node = new Node({ graph });
node.inputs.harmonyType.setValue('triadic');
- node.inputs.numberOfColors.setValue(5);+ node.inputs.numberOfColors.setValue(-1);- await node.execute();- const colors = node.outputs.colors.value as ColorType[];-- expect(colors).toHaveLength(5);- // Should repeat the pattern: base, +120, -120, base, +120- const expectedHues = [0, 120, 240, 0, 120];- colors.forEach((color, index) => {- const hslColor = toColor(color).to('hsl');- expect(Math.round(hslColor.coords[0])).toBe(expectedHues[index]);- });+ await expect(node.execute()).rejects.toThrow('numberOfColors must be a positive integer');
});
Suggestion importance[1-10]: 8
Why: Adding a test case for invalid numberOfColors values enhances test coverage and ensures the function behaves as expected under edge cases. This is important for maintaining code quality and reliability.
8
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
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.
User description
PR Type
Enhancement, Tests
Description
Introduced a new
Color Harmoniesnode for generating harmonious color combinations.Supports multiple harmony types: analogous, complementary, split-complementary, triadic, and tetradic.
Added comprehensive tests for all harmony types and edge cases.
Ensures preservation of saturation, lightness, and alpha values in generated colors.
Changes walkthrough 📝
harmonies.ts
Introduced `Color Harmonies` node with harmony generation logicpackages/graph-engine/src/nodes/color/harmonies.ts
Color Harmoniesnode for generating harmonious colorcombinations.
index.ts
Registered `Color Harmonies` node in indexpackages/graph-engine/src/nodes/color/index.ts
Color Harmoniesnode in the color nodes index.harmonies.test.ts
Added tests for `Color Harmonies` node functionalitypackages/graph-engine/tests/suites/nodes/color/harmonies.test.ts
split-complementary, triadic, and tetradic.
strong-rules-push.md
Added changeset for `Color Harmonies` node.changeset/strong-rules-push.md
Color Harmoniesnode.