chore: remove unmaintained color2k dependency and replace with local utility #6885
Conversation
WalkthroughAdds a default-exported helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @app/containers/message/Components/Attachments/CollapsibleQuote/index.tsx:
- Around line 147-149: Remove the two debug console.log calls that print
attachment.color and backgroundColor in the CollapsibleQuote component;
specifically delete the console.log(attachment.color) and
console.log(backgroundColor) lines around the backgroundColor =
withAlpha(attachment.color, 0.2) assignment, or replace them with an appropriate
logger call if structured logging is required (e.g., use the existing app logger
at debug level), but do not leave raw console.log statements in production code.
🧹 Nitpick comments (5)
app/containers/message/Components/Attachments/CollapsibleQuote/index.tsx (1)
153-153: Remove redundant no-op assignment.The statement
fontSecondaryInfo = fontSecondaryInfo;assigns the variable to itself and serves no purpose.🔎 Proposed fix
strokeExtraLight = attachment.color; strokeLight = attachment.color; strokeMedium = attachment.color; - fontSecondaryInfo = fontSecondaryInfo; }app/lib/helpers/withAlpha.test.ts (1)
1-23: Good test coverage for core functionality.The tests comprehensively cover the main supported color formats (rgba, rgb, and hex variants). The alpha conversion values are mathematically correct (0.8 × 255 = 204 = 0xcc).
Consider adding edge case tests to improve robustness:
- Alpha boundary values:
withAlpha('#ff0000', 0)andwithAlpha('#ff0000', 1)- Invalid/malformed inputs:
withAlpha('invalid', 0.5),withAlpha('', 0.5)- Unsupported formats that should return unchanged:
withAlpha('red', 0.5),withAlpha('hsl(0, 100%, 50%)', 0.5)- Case sensitivity:
withAlpha('RGB(255, 0, 0)', 0.8)🔎 Example additional tests
test('returns color unchanged for named colors', () => { expect(withAlpha('red', 0.8)).toBe('red'); }); test('returns color unchanged for hsl format', () => { expect(withAlpha('hsl(0, 100%, 50%)', 0.8)).toBe('hsl(0, 100%, 50%)'); }); test('handles alpha of 0', () => { expect(withAlpha('#ff0000', 0)).toBe('#ff000000'); }); test('handles alpha of 1', () => { expect(withAlpha('#ff0000', 1)).toBe('#ff0000ff'); });app/lib/helpers/withAlpha.ts (3)
35-44: Consider case-insensitive matching for rgb format.Similar to the rgba case, the rgb format should also handle case-insensitive input for better robustness.
🔎 Proposed fix for case-insensitive matching
// case rgb - if (color.startsWith('rgb')) { - const match = color.match(/rgb\(\s*(\d+)\s*,\s*(\d+)\s*,\s*(\d+)\s*\)/); + if (color.toLowerCase().startsWith('rgb')) { + const match = color.match(/rgb\(\s*(\d+)\s*,\s*(\d+)\s*,\s*(\d+)\s*\)/i);
47-70: Consider validating hex characters and alpha range.The current implementation doesn't validate:
- Whether the hex string contains only valid hex characters ([0-9a-fA-F])
- Whether the alpha value is within the expected 0-1 range
While the current implementation works for valid inputs, these validations would make it more robust against invalid data.
🔎 Proposed enhancements
// case hex if (color.startsWith('#')) { let hex = color.slice(1); // #rgb → #rrggbb if (hex.length === 3) { hex = hex .split('') .map(c => c + c) .join(''); } if (hex.length === 8) { hex = hex.substring(0, 6); } if (hex.length !== 6) { return color; } + + // Validate hex characters + if (!/^[0-9a-fA-F]{6}$/.test(hex)) { + return color; + } + // Clamp alpha to valid range + const clampedAlpha = Math.max(0, Math.min(1, alpha)); - const alphaHex = Math.round(alpha * 255) + const alphaHex = Math.round(clampedAlpha * 255) .toString(16) .padStart(2, '0'); return `#${hex}${alphaHex}`; }
23-32: Consider case-insensitive matching for rgba and rgb formats if handling external/user input.While CSS color function names are technically case-insensitive per specification, the current implementation only matches lowercase
rgbaandrgb. The codebase currently uses only lowercase variants, and tests cover only lowercase patterns. However, if this function is designed to handle color strings from external or user input sources, case-insensitive matching would make it more robust.To implement: Use
.toLowerCase()on thestartsWith()checks and add theiflag to both regex patterns for case-insensitive matching.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
app/containers/message/Components/Attachments/CollapsibleQuote/index.tsxapp/lib/helpers/withAlpha.test.tsapp/lib/helpers/withAlpha.tspackage.json
💤 Files with no reviewable changes (1)
- package.json
🔇 Additional comments (4)
app/containers/message/Components/Attachments/CollapsibleQuote/index.tsx (1)
15-15: LGTM!The import statement correctly brings in the new local
withAlphautility that replaces the externalcolor2kdependency.app/lib/helpers/withAlpha.ts (3)
1-16: LGTM!The JSDoc is clear, comprehensive, and properly documents supported and unsupported formats with examples.
17-20: LGTM!The function signature and empty color guard are appropriate. The default alpha value of 1 is sensible for full opacity.
76-76: LGTM!The default export follows the module pattern used elsewhere in the codebase.
app/containers/message/Components/Attachments/CollapsibleQuote/index.tsx
Outdated
Show resolved
Hide resolved
divyanshu-patil
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
Really cool! Thanks for your contribution, @divyanshu-patil! |
divyanshu-patil
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
divyanshu-patil
had a problem deploying
to
official_android_build
GitHub Actions
Error
divyanshu-patil
had a problem deploying
to
official_ios_build
GitHub Actions
Error
divyanshu-patil
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
|
sure ! |
app/containers/message/Components/Attachments/CollapsibleQuote/index.tsx
Outdated
Show resolved
Hide resolved
divyanshu-patil
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/containers/message/Components/Attachments/CollapsibleQuote/index.tsx (1)
145-155: Incorrect alpha value causes visual regression.Based on your own explanation in the PR comments, the alpha value should be
0.2, not0.8:
transparentize(color, 0.8)= 80% transparent → 20% opacitywithAlpha(color, 0.8)= 80% opacity (current code - incorrect)withAlpha(color, 0.2)= 20% opacity (correct replacement)The current implementation will render backgrounds at 80% opacity instead of the intended 20%, making them significantly more opaque than before.
🐛 Proposed fix
try { if (attachment.color) { - backgroundColor = withAlpha(attachment.color, 0.8); // works for rgba, rgb, hex (`#rgb`, `#rrggbb`) + backgroundColor = withAlpha(attachment.color, 0.2); // works for rgba, rgb, hex (`#rgb`, `#rrggbb`) strokeExtraLight = attachment.color; strokeLight = attachment.color; strokeMedium = attachment.color;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/containers/message/Components/Attachments/CollapsibleQuote/index.tsxapp/lib/helpers/withAlpha.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/lib/helpers/withAlpha.ts
🔇 Additional comments (1)
app/containers/message/Components/Attachments/CollapsibleQuote/index.tsx (1)
15-15: LGTM!The import correctly references the new local
withAlphahelper, replacing the externalcolor2kdependency.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
3 participants
Proposed changes
Removes the unmaintained
color2kdependency and replaces its only usage with a small, local utility function for applying alpha to colors.The new utility supports
rgb,rgba, andhexcolor formats and preserves existing behavior while improving maintainability.Issue(s)
How to test or reproduce
yarn test withAlphaScreenshots after changes
App size comparision
Data
Types of changes
Checklist
Further comments
~2.9 KB.Summary by CodeRabbit
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.