Fix dark mode styling #19392
Fix dark mode styling #19392
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
dannyroosevelt
moved this to Ready for PR Review
in Component (Source and Action) Backlog
WalkthroughRelease 2.3.0: adds dark-mode support for select components via new theme tokens (optionHover, optionSelected, optionSelectedHover, appIconBackground), centralizes select styling with createBaseSelectStyles/resolveSelectColors, and introduces memoization across select components to reduce recomputation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
packages/connect-react/CHANGELOG.md(1 hunks)packages/connect-react/package.json(1 hunks)packages/connect-react/src/components/ControlSelect.tsx(9 hunks)packages/connect-react/src/utils/select-styles.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-27T16:47:38.177Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/components/ControlSelect.tsx:138-145
Timestamp: 2025-08-27T16:47:38.177Z
Learning: In the connect-react package, changes should be non-behavioral during the SDK migration. The LoadMore functionality in ControlSelect should maintain its current runtime behavior.
Applied to files:
packages/connect-react/package.jsonpackages/connect-react/CHANGELOG.mdpackages/connect-react/src/components/ControlSelect.tsxpackages/connect-react/src/utils/select-styles.ts
📚 Learning: 2025-08-27T16:48:48.776Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/components/Errors.tsx:16-19
Timestamp: 2025-08-27T16:48:48.776Z
Learning: In the connect-react package refactoring PR, maintain current runtime behavior even when type patterns are not ideal - prioritize behavioral consistency over type safety improvements when explicitly requested by the maintainer.
Applied to files:
packages/connect-react/package.json
📚 Learning: 2025-08-27T17:25:10.425Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/utils/type-guards.ts:23-33
Timestamp: 2025-08-27T17:25:10.425Z
Learning: In the connect-react package, the isOptionWithLabel type guard intentionally restricts value types to string|number for runtime filtering purposes, even though LabelValueOption<T> allows any T. This runtime behavior should be preserved over type safety improvements.
Applied to files:
packages/connect-react/src/components/ControlSelect.tsx
🧬 Code graph analysis (2)
packages/connect-react/src/components/ControlSelect.tsx (2)
packages/connect-react/src/utils/select-styles.ts (2)
resolveSelectColors(28-53)createBaseSelectStyles(59-131)packages/connect-react/src/types.ts (1)
LabelValueOption(40-42)
packages/connect-react/src/utils/select-styles.ts (1)
packages/connect-react/src/theme.ts (1)
Colors(8-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
🔇 Additional comments (6)
packages/connect-react/package.json (1)
3-3: LGTM!Version bump to 2.3.0 is appropriate for the new dark mode feature addition. Follows semantic versioning correctly.
packages/connect-react/CHANGELOG.md (1)
5-10: LGTM!Changelog entry accurately documents the dark mode additions and new theme color tokens. Format is consistent with existing entries.
packages/connect-react/src/utils/select-styles.ts (1)
104-128: LGTM! Option styling logic is well-structured.The background color priority (selected+focused → selected → focused → surface) and the
:activeoverride to replace default blue with theme colors are correctly implemented for dark mode support.packages/connect-react/src/components/ControlSelect.tsx (3)
170-195: LGTM!The simplified
finalComponentslogic correctly uses the caller'sMenuListoverride when provided, falling back to the default. Using refs forshowLoadMoreButtonandonLoadMorekeeps the dependency array minimal while ensuring callbacks stay current.
284-295: LGTM!Style merging order is correct: base dark mode styles (
selectStyles) can be overridden by user-provided styles (selectProps?.styles), with final inline overrides forcontainerandmenuPortal. This ensures dark mode support while maintaining customization flexibility.
105-107: Good documentation of design decision.The comment clearly explains why styles are applied directly rather than through
getProps, helping future maintainers understand the intentional separation for dark mode support.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
packages/connect-react/src/components/ControlApp.tsx(2 hunks)packages/connect-react/src/components/ControlSelect.tsx(7 hunks)packages/connect-react/src/components/SelectApp.tsx(4 hunks)packages/connect-react/src/components/SelectComponent.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-27T16:47:38.177Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/components/ControlSelect.tsx:138-145
Timestamp: 2025-08-27T16:47:38.177Z
Learning: In the connect-react package, changes should be non-behavioral during the SDK migration. The LoadMore functionality in ControlSelect should maintain its current runtime behavior.
Applied to files:
packages/connect-react/src/components/ControlApp.tsxpackages/connect-react/src/components/ControlSelect.tsxpackages/connect-react/src/components/SelectComponent.tsxpackages/connect-react/src/components/SelectApp.tsx
📚 Learning: 2025-08-27T17:25:10.425Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/utils/type-guards.ts:23-33
Timestamp: 2025-08-27T17:25:10.425Z
Learning: In the connect-react package, the isOptionWithLabel type guard intentionally restricts value types to string|number for runtime filtering purposes, even though LabelValueOption<T> allows any T. This runtime behavior should be preserved over type safety improvements.
Applied to files:
packages/connect-react/src/components/ControlSelect.tsx
🧬 Code graph analysis (3)
packages/connect-react/src/components/ControlApp.tsx (1)
packages/connect-react/src/utils/select-styles.ts (2)
resolveSelectColors(28-53)createBaseSelectStyles(59-131)
packages/connect-react/src/components/SelectComponent.tsx (1)
packages/connect-react/src/utils/select-styles.ts (2)
resolveSelectColors(28-53)createBaseSelectStyles(59-131)
packages/connect-react/src/components/SelectApp.tsx (2)
packages/connect-react/src/utils/select-styles.ts (2)
resolveSelectColors(28-53)createBaseSelectStyles(59-131)packages/connect-react/src/hooks/customization-context.tsx (1)
BaseReactSelectProps(133-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (5)
packages/connect-react/src/components/ControlApp.tsx (1)
58-78: Memoized select colors/styles are correct and preserve control behavior.Color resolution and base select styles are now only recomputed when
theme.colorsortheme.boxShadowchange, and thecontrolstyle override still delegates to the base style before settinggridArea: "control". This matches the pattern used by the other select components and is a clean perf win with no behavior change.Also applies to: 99-102
packages/connect-react/src/components/SelectApp.tsx (1)
101-121: SelectApp theming and icon background integration look solid.Resolved colors and base select styles are now memoized off
theme.colors/theme.boxShadow, and the app icon background usesresolvedColors.appIconBgwith correct dependencies incustomComponents. This keeps styling consistent with other select components while limiting recalculation.Also applies to: 124-124, 142-142, 166-166, 200-200
packages/connect-react/src/components/SelectComponent.tsx (1)
53-73: Consistent memoization of select styles in SelectComponent.Color resolution and base select styles are now memoized exactly like in the other select components, and those styles are fed through
select.getPropsviabaseSelectProps.styles. This reduces recalculation without altering load-more or selection behavior.Also applies to: 98-98
packages/connect-react/src/components/ControlSelect.tsx (2)
60-80: Memoized select styles and LoadMore wrapper preserve existing behavior.Color resolution and base select styles are now memoized off
theme.colors/theme.boxShadow, and those styles are fed intoselect.getProps("controlSelect", …)viacustomizationProps, matching the pattern in other select components. The newfinalComponentsmerge (customizationProps.components→componentsOverride→CustomMenuListwrapper) still delegates to the parentMenuListand appendsLoadMoreButtonbased onshowLoadMoreButtonRef, so the LoadMore runtime behavior remains unchanged while gaining customization support. Based on learnings, this keeps ControlSelect’s LoadMore semantics intact.Also applies to: 147-201
147-155: Consider forwarding more ofselect.getPropsto keep ControlSelect aligned with other select components.Here
select.getProps("controlSelect", …)is only used to pullclassNamePrefix,classNames,theme,styles, andcomponentsintocustomizationProps, and only those are forwarded toMaybeCreatableSelect. InSelectApp,SelectComponent, andControlApp, the full result ofselect.getPropsis spread into the underlyingSelect, so any future additions to the customization contract (e.g., defaultmenuPlacement,filterOption, or other props) would apply automatically.For consistency and to future‑proof customization, consider either:
- Spreading
customizationPropsontoMaybeCreatableSelect(with care for precedence vs.selectPropsand your explicit props), or- Explicitly forwarding any additional keys that
select.getProps("controlSelect", …)is expected to supply beyond theme/classNamePrefix/classNames/styles/components.This would keep ControlSelect behavior in line with the other select wrappers if the customization API expands.
Also applies to: 171-201, 280-283
⛔ Skipped due to learnings
Learnt from: jverce Repo: PipedreamHQ/pipedream PR: 18187 File: packages/connect-react/src/components/ControlSelect.tsx:138-145 Timestamp: 2025-08-27T16:47:38.177Z Learning: In the connect-react package, changes should be non-behavioral during the SDK migration. The LoadMore functionality in ControlSelect should maintain its current runtime behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/connect-react/src/components/ControlSelect.tsx (1)
292-303: LGTM! Guard forselectProps?.stylescorrectly applied.The
?? {}guard addresses the previous review feedback. Note that the explicitcontainerandmenuPortaldefinitions will override any user-provided styles for these keys—this appears intentional for enforcing layout (gridArea) and stacking context (zIndex) requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/connect-react/src/components/ControlSelect.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-27T16:47:38.177Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/components/ControlSelect.tsx:138-145
Timestamp: 2025-08-27T16:47:38.177Z
Learning: In the connect-react package, changes should be non-behavioral during the SDK migration. The LoadMore functionality in ControlSelect should maintain its current runtime behavior.
Applied to files:
packages/connect-react/src/components/ControlSelect.tsx
📚 Learning: 2025-08-27T16:48:48.776Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/components/Errors.tsx:16-19
Timestamp: 2025-08-27T16:48:48.776Z
Learning: In the connect-react package refactoring PR, maintain current runtime behavior even when type patterns are not ideal - prioritize behavioral consistency over type safety improvements when explicitly requested by the maintainer.
Applied to files:
packages/connect-react/src/components/ControlSelect.tsx
📚 Learning: 2025-08-27T17:25:10.425Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/utils/type-guards.ts:23-33
Timestamp: 2025-08-27T17:25:10.425Z
Learning: In the connect-react package, the isOptionWithLabel type guard intentionally restricts value types to string|number for runtime filtering purposes, even though LabelValueOption<T> allows any T. This runtime behavior should be preserved over type safety improvements.
Applied to files:
packages/connect-react/src/components/ControlSelect.tsx
🧬 Code graph analysis (1)
packages/connect-react/src/components/ControlSelect.tsx (2)
packages/connect-react/src/utils/select-styles.ts (2)
resolveSelectColors(28-53)createBaseSelectStyles(59-131)packages/connect-react/src/types.ts (1)
LabelValueOption(40-42)
🔇 Additional comments (4)
packages/connect-react/src/components/ControlSelect.tsx (4)
28-31: LGTM!The new utility imports align with the centralized select styling approach, consistent with the pattern in SelectComponent and SelectApp.
59-80: LGTM! Memoization correctly implemented.The two-tier memoization creates a proper dependency chain:
theme.colors→resolvedColors→baseSelectStyles. This prevents unnecessary style object recreation on every render.
147-155: LGTM!The customization props pattern correctly passes
baseSelectStylesas the default, allowing user customizations to merge on top while preserving dark mode styling.
169-201: LGTM!The component merge order (context → caller overrides → MenuList wrapper) is correct, and the dependency array properly includes
customizationProps.components.
| // Apply customization context values as defaults | ||
| classNamePrefix={customizationProps.classNamePrefix || "react-select"} | ||
| classNames={customizationProps.classNames} | ||
| theme={customizationProps.theme} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using ?? instead of || for classNamePrefix.
Using || treats empty string as falsy, so customizationProps.classNamePrefix = "" (intentionally no prefix) would fall back to "react-select". Using ?? would only fallback for null/undefined, preserving explicit empty string customization.
- classNamePrefix={customizationProps.classNamePrefix || "react-select"}
+ classNamePrefix={customizationProps.classNamePrefix ?? "react-select"}🤖 Prompt for AI Agents
In packages/connect-react/src/components/ControlSelect.tsx around lines 279 to
282, the code uses the || operator for classNamePrefix which treats an explicit
empty string as falsy and will wrongly fall back to "react-select"; change this
to the nullish coalescing operator (??) so that only null or undefined trigger
the default while preserving an intentional empty string, i.e., replace the ||
usage with ?? for classNamePrefix.
github-project-automation
bot
moved this from Ready for PR Review
to Done
in Component (Source and Action) Backlog
* improving dark mode (esp for highlighted and selected states) * linting * PR feedback * PR feedback * Update ControlSelect.tsx
WHY
Summary by CodeRabbit
New Features
Version
✏️ Tip: You can customize this high-level summary in your review settings.