Thulasizwe/en/3540#3869
Conversation
WalkthroughConsolidates read-only rendering by removing ReadOnlyDisplayFormItem branches for checkbox, radio, and switch; introduces a checkbox styling system (useStyles + defaultStyles) and applies computed finalStyle to native controls; propagates incoming style to ReadOnlyDisplayFormItem root; tightens border-width check in columns. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Designer as Designer Component
participant Style as Style Computation
participant UI as Native Control
User->>Designer: Render component (model, style, readOnly flag)
Designer->>Style: Compute finalStyle (useMemo: merge font+dimensions if readOnly && !enableStyleOnReadonly)
Style-->>Designer: finalStyle
Note over Designer,UI #DDEBF7: ReadOnlyDisplayFormItem path removed for checkbox/radio/switch
Designer->>UI: Render native control (Checkbox/Radio/Switch) with finalStyle
sequenceDiagram
autonumber
participant Columns as Columns Component
participant Logic as Border Logic
Columns->>Logic: isValidBorderWidth(width)
Logic-->>Columns: evaluate width !== 0 (strict)
alt strictly non-zero
Columns->>Columns: hasBorder = true
else
Columns->>Columns: hasBorder = false
end
Note right of Columns #FFF2CC: string "0"/"0px" no longer === 0
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–30 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 3
🧹 Nitpick comments (5)
shesha-reactjs/src/designer-components/switch/switch.tsx (1)
45-53: Don’t mix controlled and uncontrolled props; add ARIA hint
checkedmakes the Switch controlled; keepdefaultChecked/defaultValueoff to avoid React warnings. Consideraria-readonlyfor accessibility.Apply this diff:
- return ( <Switch + return ( <Switch className="sha-switch" disabled={model.readOnly} + aria-readonly={model.readOnly || undefined} style={finalStyle} size={model.size as SwitchSize} checked={value} - defaultChecked={model.defaultChecked} - defaultValue={model.defaultValue} onChange={onChangeInternal} /> );shesha-reactjs/src/designer-components/checkbox/checkbox.tsx (1)
63-63: Optional: expose read-only semantics to ATAdd
aria-readonlyso assistive tech can differentiate disabled vs read-only policy.Apply this diff:
- return <Checkbox className={styles.checkbox} disabled={model.readOnly} checked={value} {...events} />; + return <Checkbox className={styles.checkbox} disabled={model.readOnly} aria-readonly={model.readOnly || undefined} checked={value} {...events} />;shesha-reactjs/src/designer-components/checkbox/styles.ts (3)
8-23: Handle numeric/keyword fontWeight; add typing.Current switch only matches string numerals; numeric weights and keywords fall through.
- const borderWidthFromWeight = (weight) => { - switch (weight) { - case '100': - return '1px'; - case '400': - return '2px'; - case '500': - return '3px'; - case '700': - return '4px'; - case '900': - return '5px'; - default: - return '2px'; - }; - }; + const borderWidthFromWeight = (weight?: number | string) => { + const asNum = typeof weight === 'number' ? weight : Number.parseInt(String(weight ?? ''), 10); + if (Number.isFinite(asNum)) { + if (asNum >= 900) return '5px'; + if (asNum >= 700) return '4px'; + if (asNum >= 500) return '3px'; + if (asNum >= 400) return '2px'; + return '1px'; + } + switch (String(weight)) { + case 'bold': return '3px'; + case 'bolder': return '4px'; + case 'lighter': return '1px'; + default: return '2px'; + } + };
40-43: Remove empty nested selectors.These blocks are no‑ops and add noise.
- .${prefixCls}-checkbox { - - } + /* removed empty selector */ @@ - .${prefixCls}-checkbox { - - } + /* removed empty selector */Also applies to: 51-54
2-2: Use a type‑only import for React.Avoid an unused value import.
-import React from 'react'; +import type React from 'react';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
shesha-reactjs/src/components/readOnlyDisplayFormItem/index.tsx(1 hunks)shesha-reactjs/src/designer-components/checkbox/checkbox.tsx(5 hunks)shesha-reactjs/src/designer-components/checkbox/styles.ts(1 hunks)shesha-reactjs/src/designer-components/checkbox/utils.ts(1 hunks)shesha-reactjs/src/designer-components/columns/columns.tsx(1 hunks)shesha-reactjs/src/designer-components/radio/radioGroup.tsx(0 hunks)shesha-reactjs/src/designer-components/switch/switch.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- shesha-reactjs/src/designer-components/radio/radioGroup.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-12T16:55:57.638Z
Learnt from: teboho
PR: shesha-io/shesha-framework#3397
File: shesha-reactjs/src/designer-components/charts/bar.tsx:49-52
Timestamp: 2025-06-12T16:55:57.638Z
Learning: For the chart components’ migrators (e.g., BarChartComponent in shesha-reactjs/src/designer-components/charts/bar.tsx), the version 5 step intentionally spreads `{ ...prev, ...defaultConfigFiller }` so that values from `defaultConfigFiller` override any existing properties in `prev`. This reset to new defaults is by design and should not be flagged as an issue.
Applied to files:
shesha-reactjs/src/designer-components/checkbox/checkbox.tsx
📚 Learning: 2025-06-26T19:59:22.872Z
Learnt from: teboho
PR: shesha-io/shesha-framework#3461
File: shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts:1280-1280
Timestamp: 2025-06-26T19:59:22.872Z
Learning: In shesha-reactjs chart settings forms, the styling panels (border, shadow, background) should not include buttonType-based hidden conditions since charts are not button components. The border configuration should follow the standard component pattern without button-specific restrictions.
Applied to files:
shesha-reactjs/src/designer-components/columns/columns.tsx
🔇 Additional comments (8)
shesha-reactjs/src/designer-components/switch/switch.tsx (1)
29-33: finalStyle memoization makes senseGood conditional merge for read-only styling; dependencies look correct.
shesha-reactjs/src/components/readOnlyDisplayFormItem/index.tsx (1)
156-159: Style passthrough to wrapper is OKApplying
styleon the wrapper improves consistency with other branches. No issues spotted.shesha-reactjs/src/designer-components/checkbox/checkbox.tsx (3)
44-47: finalStyle logic for read-only is soundClear, minimal merge of font + dimensions when styles are disabled on read-only.
49-50: useStyles integration looks goodUsing a class for styling is appropriate; keeps inline style minimal on the control.
81-84: Verify migration doesn’t clobber user stylesrg returned no hits for migratePrevStyles nor for shesha-reactjs/src/designer-components/checkbox/checkbox.tsx in this branch — confirm migratePrevStyles performs a non-destructive merge (prefers existing non-empty values over defaults) when called as migratePrevStyles(prev, defaultStyles()); if the function is missing, paste its implementation or update the migrator to use a deep-preserve merge.
shesha-reactjs/src/designer-components/checkbox/utils.ts (1)
1-33: Baseline styles look reasonableSane defaults for font, border, radius, and 16px control size. No blockers.
shesha-reactjs/src/designer-components/checkbox/styles.ts (2)
36-36: Confirm object interpolation support for ${rest}.Ensure your
csshelper accepts object interpolation inside template strings (Emotion does; others may not). If not, build a style object and pass it viacss({ ... }).
25-61: Good direction: centralized, token‑driven styling.Hook‑based styling with a single
checkboxclass aligns with the PR’s consolidation goals.Please confirm Checkbox consumes
styles.checkboxon both editable and read‑only paths as intended.
| export const useStyles = createStyles(({ css, cx, prefixCls }, { style }: { style: React.CSSProperties }) => { | ||
|
|
||
| const { fontWeight, backgroundImage, backgroundColor, ...rest } = style; |
There was a problem hiding this comment.
Guard against missing style prop (avoid runtime crash).
style is required by the type but could be omitted by callers. Destructuring on Line 6 would then throw. Make style optional with a safe default.
-export const useStyles = createStyles(({ css, cx, prefixCls }, { style }: { style: React.CSSProperties }) => {
+export const useStyles = createStyles(
+ ({ css, cx, prefixCls },
+ { style = {} as React.CSSProperties }: { style?: React.CSSProperties }
+ ) => {
@@
- const { fontWeight, backgroundImage, backgroundColor, ...rest } = style;
+ const { backgroundImage, backgroundColor, width, height, fontSize, color, ...rest } = style;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const useStyles = createStyles(({ css, cx, prefixCls }, { style }: { style: React.CSSProperties }) => { | |
| const { fontWeight, backgroundImage, backgroundColor, ...rest } = style; | |
| export const useStyles = createStyles( | |
| ({ css, cx, prefixCls }, | |
| { style = {} as React.CSSProperties }: { style?: React.CSSProperties } | |
| ) => { | |
| const { backgroundImage, backgroundColor, width, height, fontSize, color, ...rest } = style; |
🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/checkbox/styles.ts around lines 4 to
6, the function destructures properties from a required style param which can be
omitted at runtime and cause a crash; make the style parameter optional (change
its type to { style?: React.CSSProperties }) and provide a safe default empty
object when destructuring (e.g., destructure from style = {}), then proceed to
extract fontWeight, backgroundImage, backgroundColor, ...rest from that
defaulted object.
|
|
||
| const checkbox = cx("sha-checkbox", css` | ||
| .${prefixCls}-checkbox { | ||
| .${prefixCls}-checkbox-inner { | ||
| --ant-control-interactive-size: ${style?.fontSize}; | ||
| --ant-line-width-bold: ${borderWidthFromWeight(style?.fontWeight)} !important; | ||
| --ant-color-white: ${style.color || '#fff'} !important; | ||
| --ant-color-primary-hover: ${backgroundColor}; | ||
| width: ${style?.width}; | ||
| height: ${style?.height}; | ||
| display: flex; | ||
| justify-content: center; | ||
| ${rest} |
There was a problem hiding this comment.
Avoid unitless numbers and “undefined” leaking into CSS; add a unit normalizer and conditional declarations.
Numbers like fontSize/width/height will render without units inside template strings, and undefined values will print as “undefined”.
@@
- const checkbox = cx("sha-checkbox", css`
+ const toUnit = (v?: string | number) => (v == null ? undefined : typeof v === 'number' ? `${v}px` : v);
+ const checkbox = cx("sha-checkbox", css`
.${prefixCls}-checkbox {
.${prefixCls}-checkbox-inner {
- --ant-control-interactive-size: ${style?.fontSize};
- --ant-line-width-bold: ${borderWidthFromWeight(style?.fontWeight)} !important;
- --ant-color-white: ${style.color || '#fff'} !important;
- --ant-color-primary-hover: ${backgroundColor};
- width: ${style?.width};
- height: ${style?.height};
+ ${fontSize != null ? `--ant-control-interactive-size: ${toUnit(fontSize)};` : ''}
+ --ant-line-width-bold: ${borderWidthFromWeight(style?.fontWeight)} !important;
+ --ant-color-white: ${(color ?? '#fff')} !important;
+ ${backgroundColor ? `--ant-color-primary-hover: ${backgroundColor};` : ''}
+ ${width != null ? `width: ${toUnit(width)};` : ''}
+ ${height != null ? `height: ${toUnit(height)};` : ''}
display: flex;
justify-content: center;
${rest}
@@
.${prefixCls}-checkbox-checked {
.${prefixCls}-checkbox-inner {
- background: ${backgroundImage || backgroundColor};
- ${rest}
+ ${(backgroundImage || backgroundColor) ? `background: ${backgroundImage || backgroundColor};` : ''}Also applies to: 46-49
🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/checkbox/styles.ts around lines 24 to
36 (also applies to lines 46-49), numeric or undefined values like fontSize,
width, height and border widths are being interpolated directly into the CSS
template, which can emit unitless numbers or the string "undefined"; add a small
unit normalizer (e.g., coerce numbers to px, pass-through strings that already
contain units, allow zero) and use conditional string fragments so each CSS
custom property or rule is only added when the normalized value is not
undefined, replacing direct interpolations with normalizedValue && `property:
${normalizedValue};` patterns or a helper that returns an empty string when the
input is null/undefined.
| // Add padding when border is configured to prevent border from touching components | ||
| const isValidBorderWidth = (width: string | number | undefined): boolean => { | ||
| return !!(width && width !== '0px' && width != 0); | ||
| return !!(width && width !== '0px' && width !== 0); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Fix zero-width detection: strings like "0" currently count as a border
isValidBorderWidth treats '0' (and '0rem', '0em', etc.) as non‑zero due to strict checks. This inflates hasBorder, adding unintended padding/boxSizing. Parse numeric part and handle common zero tokens.
Apply this diff:
- const isValidBorderWidth = (width: string | number | undefined): boolean => {
- return !!(width && width !== '0px' && width !== 0);
- };
+ const isValidBorderWidth = (width: string | number | undefined): boolean => {
+ if (width == null) return false;
+ if (typeof width === 'number') return !Number.isNaN(width) && width !== 0;
+ const w = String(width).trim();
+ if (!w) return false;
+ // Handle common unitized zeros and numeric strings
+ const n = parseFloat(w);
+ if (!Number.isNaN(n)) return n !== 0;
+ // Non-numeric (e.g., CSS vars). Consider no-border only for explicit "0".
+ return w !== '0';
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Add padding when border is configured to prevent border from touching components | |
| const isValidBorderWidth = (width: string | number | undefined): boolean => { | |
| return !!(width && width !== '0px' && width != 0); | |
| return !!(width && width !== '0px' && width !== 0); | |
| }; | |
| // Add padding when border is configured to prevent border from touching components | |
| const isValidBorderWidth = (width: string | number | undefined): boolean => { | |
| if (width == null) return false; | |
| if (typeof width === 'number') return !Number.isNaN(width) && width !== 0; | |
| const w = String(width).trim(); | |
| if (!w) return false; | |
| // Handle common unitized zeros and numeric strings | |
| const n = parseFloat(w); | |
| if (!Number.isNaN(n)) return n !== 0; | |
| // Non-numeric (e.g., CSS vars). Consider no-border only for explicit "0". | |
| return w !== '0'; | |
| }; |
🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/columns/columns.tsx around lines 121
to 125, isValidBorderWidth currently treats strings like "0" or "0rem" as
non-zero because it only checks for exact '0px' and 0; change it to: if width is
a number, return width !== 0; if width is a string, trim it and parse the
leading numeric portion (or use a regex to capture an optional sign and numeric
value) and return false when that numeric part equals 0, otherwise true; also
explicitly treat empty/undefined/null as false so common tokens like "0", "0px",
"0rem", "0em", "0%" are recognized as zero widths.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
shesha-reactjs/src/designer-components/timeField/timePickerWrapper.tsx (4)
10-10: Allow nulls in RangeValue to match picker semantics.Current
RangeValueforbids nulls, but the component deals with missing ends. Update the alias.-type RangeValue = [moment.Moment, moment.Moment]; +type RangeValue = [Moment | null, Moment | null];
82-88: Emit nulls (not undefined) in range onChange and accept null entries.
getTotalSecondscan produceundefined; propagatenullinstead for clearer API contracts and JSON-serializable payloads. Also allow nulls in the incoming values tuple.- const handleRangePicker = (values: Moment[], timeString: [string, string]) => { - if (onChange){ - const seconds = values?.map(value => getTotalSeconds(value)); - (onChange as RangePickerChangeEvent)(seconds, timeString); - } - }; + const handleRangePicker = (values: [Moment | null, Moment | null], timeString: [string, string]) => { + if (onChange) { + const seconds = values.map((v) => (v ? getTotalSeconds(v) : null)) as [number | null, number | null]; + (onChange as RangePickerChangeEvent)(seconds, timeString); + } + };
14-25: Return type of getMoment should include null; tighten parsing.Function currently promises
Momentbut can returnundefined, leading to type unsoundness and casts elsewhere.-const getMoment = (value: any, dateFormat: string): Moment => { - if (value === null || value === undefined) return undefined; - const values = [ - isMoment(value) ? value : null, - typeof (value) === 'number' ? moment.utc(value * 1000) : null, // time in millis - typeof (value) === 'string' ? moment(value as string) : null, - typeof (value) === 'string' ? moment(value as string, dateFormat) : null, - ]; - const parsed = values.find((i) => isMoment(i) && i.isValid()); - - return parsed; -}; +const getMoment = (value: unknown, dateFormat: string): Moment | null => { + if (value === null || value === undefined) return null; + const candidates: Array<Moment | null> = [ + isMoment(value) ? (value as Moment) : null, + typeof value === 'number' ? moment.utc((value as number) * 1000) : null, // seconds → ms + typeof value === 'string' ? moment(value as string, dateFormat, true) : null, + typeof value === 'string' ? moment(value as string) : null, + ]; + return candidates.find((m) => m && m.isValid()) ?? null; +};
27-37: Align getTotalSeconds return type with nulls.It returns
undefinedon invalid input but is typed asnumber. Prefernumber | nullto pair with above changes.-const getTotalSeconds = (value: Moment): number => { - if (!isMoment(value) || !value.isValid()) - return undefined; - const timeOnly = moment.duration({ - hours: value.hours(), - minutes: value.minutes(), - seconds: value.seconds() - }); - return timeOnly.asSeconds(); -}; +const getTotalSeconds = (value: Moment | null | undefined): number | null => { + if (!value || !isMoment(value) || !value.isValid()) return null; + return value.hours() * 3600 + value.minutes() * 60 + value.seconds(); +};
🧹 Nitpick comments (3)
shesha-reactjs/src/designer-components/timeField/timePickerWrapper.tsx (3)
76-80: Guard is good; normalize undefined to null for single onChange.Minor consistency tweak with range branch.
- if (onChange){ - const seconds = getTotalSeconds(newValue); - (onChange as TimePickerChangeEvent)(seconds, timeString); - } + if (onChange) { + const seconds = getTotalSeconds(newValue); + (onChange as TimePickerChangeEvent)(seconds ?? null, timeString); + }
98-106: Forward disabled to the interactive pickers.
disabledis plucked but never forwarded toTimeRangePicker/TimePicker(only used in read-only). Users can’t disable the input otherwise.<TimeRangePicker variant={hideBorder ? 'borderless' : undefined } onChange={handleRangePicker} format={format} value={getRangePickerValues(value || defaultValue) as RangeValue} {...steps} style={style} className={styles.shaTimepicker} + disabled={disabled} showNow={showNow} allowClear={allowClear} - {...rest} - placeholder={[placeholder, placeholder]} + placeholder={Array.isArray(placeholder) ? placeholder : [placeholder, placeholder]} + {...rest} /><TimePicker variant={hideBorder ? 'borderless' : undefined } onChange={handleTimePickerChange} format={format} value={evaluatedValue|| (defaultValue && moment(defaultValue))} {...steps} style={style} className={styles.shaTimepicker} placeholder={placeholder} + disabled={disabled} showNow={showNow} allowClear={allowClear} {...rest} />Also applies to: 115-126
107-108: Range placeholder: support array input and keep rest last.Ensure callers can pass distinct placeholders and maintain predictable prop override order.
- {...rest} - placeholder={[placeholder, placeholder]} + placeholder={Array.isArray(placeholder) ? placeholder : [placeholder, placeholder]} + {...rest}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
shesha-reactjs/src/designer-components/timeField/timePickerWrapper.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-12T16:55:57.638Z
Learnt from: teboho
PR: shesha-io/shesha-framework#3397
File: shesha-reactjs/src/designer-components/charts/bar.tsx:49-52
Timestamp: 2025-06-12T16:55:57.638Z
Learning: For the chart components’ migrators (e.g., BarChartComponent in shesha-reactjs/src/designer-components/charts/bar.tsx), the version 5 step intentionally spreads `{ ...prev, ...defaultConfigFiller }` so that values from `defaultConfigFiller` override any existing properties in `prev`. This reset to new defaults is by design and should not be flagged as an issue.
Applied to files:
shesha-reactjs/src/designer-components/timeField/timePickerWrapper.tsx
🔇 Additional comments (3)
shesha-reactjs/src/designer-components/timeField/timePickerWrapper.tsx (3)
97-97: LGTM: borderless variant hook-up (range).Consistent with
hideBorderusage elsewhere.
115-115: LGTM: borderless variant hook-up (single).Matches range branch.
8-8: Verification blocked — missing timeField filesCouldn't locate shesha-reactjs/src/designer-components/timeField/timePickerWrapper.tsx or its ./models (ITimePickerProps, RangePickerChangeEvent, TimePickerChangeEvent) in the repo. Paste the models/type declarations or confirm that:
- ITimePickerProps.placeholder is string | [string,string]
- RangePickerChangeEvent and TimePickerChangeEvent payloads are number | null
| const getRangePickerValues = (value: string | [string, string]) => | ||
| Array.isArray(value) && value?.length === 2 ? value?.map((v) => getMoment(v, format)) : [null, null]; | ||
| Array.isArray(value) && value?.length === 2 ? value?.map((v) => getMoment(v, format)) : [null, null]; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize range values to Moment|null and widen accepted input.
getRangePickerValues can yield undefined entries and only accepts string | [string,string], while callers pass value ?? defaultValue that may include Moments/numbers. Normalize to [Moment|null, Moment|null] and accept unknown input.
- const getRangePickerValues = (value: string | [string, string]) =>
- Array.isArray(value) && value?.length === 2 ? value?.map((v) => getMoment(v, format)) : [null, null];
+ const getRangePickerValues = (val: unknown): [Moment | null, Moment | null] =>
+ Array.isArray(val) && val.length === 2
+ ? (val as unknown[]).map((v) => getMoment(v, format) ?? null) as [Moment | null, Moment | null]
+ : [null, null];Additionally, drop the cast at usage and rely on the function’s return type:
- value={getRangePickerValues(value || defaultValue) as RangeValue}
+ value={getRangePickerValues(value ?? defaultValue)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getRangePickerValues = (value: string | [string, string]) => | |
| Array.isArray(value) && value?.length === 2 ? value?.map((v) => getMoment(v, format)) : [null, null]; | |
| Array.isArray(value) && value?.length === 2 ? value?.map((v) => getMoment(v, format)) : [null, null]; | |
| const getRangePickerValues = (val: unknown): [Moment | null, Moment | null] => | |
| Array.isArray(val) && val.length === 2 | |
| ? (val as unknown[]).map((v) => getMoment(v, format) ?? null) as [Moment | null, Moment | null] | |
| : [null, null]; |
🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/timeField/timePickerWrapper.tsx around
lines 72-74, getRangePickerValues currently types input as string |
[string,string] and can return undefined entries; update it to accept unknown
(or any) input, normalize into an explicit [Moment|null, Moment|null] tuple by:
if value is an array of length 2 map each entry to a Moment or null (if entry is
already a Moment return it, if number/string parse with getMoment, otherwise
null), and for non-array inputs return [null, null]; update the function
signature return type accordingly and remove the downstream cast where it’s used
so callers rely on the normalized [Moment|null, Moment|null].
| onChange={handleTimePickerChange} | ||
| format={format} | ||
| value={evaluatedValue || (defaultValue && moment(defaultValue))} | ||
| value={evaluatedValue|| (defaultValue && moment(defaultValue))} |
There was a problem hiding this comment.
Fix defaultValue parsing (seconds vs ms) and use nullish coalescing.
moment(defaultValue) treats numbers as milliseconds, while getMoment treats numbers as seconds×1000. This creates an inconsistent default-value interpretation. Use getMoment(defaultValue, format) and coalesce to null.
- value={evaluatedValue|| (defaultValue && moment(defaultValue))}
+ value={evaluatedValue ?? getMoment(defaultValue, format) ?? null}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| value={evaluatedValue|| (defaultValue && moment(defaultValue))} | |
| value={evaluatedValue ?? getMoment(defaultValue, format) ?? null} |
🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/timeField/timePickerWrapper.tsx around
line 118, the code uses moment(defaultValue) which treats numeric inputs as
milliseconds causing inconsistency with getMoment (which expects seconds*1000);
replace that call with getMoment(defaultValue, format) and use nullish
coalescing so the value prop becomes evaluatedValue ?? getMoment(defaultValue,
format) (or null if getMoment returns undefined) to ensure consistent parsing
and correct fallback handling.
2 participants
Summary by CodeRabbit