Conversation
WalkthroughAdds canvas zoom (manual, pinch, wheel) and auto-zoom; introduces canvas utilities and SVG assets; extends canvas provider with autoZoom state/action; makes dimension calculations canvas-aware; exposes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Toolbar as Toolbar (DeviceOptions / CanvasConfig)
participant CanvasCtx as Canvas Context (useCanvas)
participant Sidebar as SidebarContainer
participant Utils as canvasUtils / usePinchZoom
User->>Toolbar: select device width
Toolbar->>CanvasCtx: setCanvasWidth(width, deviceType)
CanvasCtx-->>Sidebar: designerWidth updated
User->>Toolbar: toggle Auto
Toolbar->>CanvasCtx: setCanvasAutoZoom()
CanvasCtx-->>CanvasCtx: autoZoom toggled
User->>Toolbar: Zoom In/Out
Toolbar->>CanvasCtx: setCanvasZoom(clamped)
rect rgba(200,230,255,0.12)
User->>Sidebar: pinch gesture / ctrl+wheel
Sidebar->>Utils: compute zoom (usePinchZoom / wheel handler)
Utils-->>Sidebar: computedZoom (clamped)
Sidebar->>CanvasCtx: setCanvasZoom(computedZoom)
end
CanvasCtx->>Utils: calculateAutoZoom() when autoZoom
Utils-->>CanvasCtx: optimalZoom
CanvasCtx->>Sidebar: setCanvasZoom(optimalZoom)
Sidebar-->>User: render canvas with width & zoom
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shesha-reactjs/src/designer-components/_settings/utils/CustomDropdown.tsx (2)
45-48: Bug: adding multiple custom options drops earlier ones.
setCustomOptions(() => [...options, customOption]);reuses the propsoptions, discarding previously added customs.Preserve prior state and notify parent if requested:
- const addCustomOption = () => { - setCustomOptions(() => [...options, customOption]); - clearInputs(); - }; + const addCustomOption = () => { + setCustomOptions(prev => [...prev, customOption]); + onAddCustomOption?.(customOption); + clearInputs(); + };
38-44: State not synced whenoptionsprop changes.Local
customOptionswon’t reflect upstream updates.Add a sync effect:
const [customOptions, setCustomOptions] = useState(options); + + React.useEffect(() => { + setCustomOptions(options); + }, [options]);
🧹 Nitpick comments (27)
shesha-reactjs/src/components/mainLayout/styles/styles.ts (2)
170-173: Fix typo in comment (“for the sake of of”).Tidy up the wording for clarity.
- top: 0 !important; // By default, the trigger is at the bottom. But we want it to be at the top for the sake of of + top: 0 !important; // By default, the trigger is at the bottom. We want it at the top for UX consistency.
9-10: Prefer theme tokens over hard-coded colors.Aligns with theming/dark mode and reduces maintenance.
- const backgroundColor = '#f0f2f5'; // @background-color - const shaBorder = '1px solid #d3d3d3'; + const backgroundColor = token.colorBgLayout; // @background-color + const shaBorder = `1px solid ${token.colorBorderSecondary ?? token.colorBorder}`;- background: ${backgroundColor}; + background: ${backgroundColor};- background: ${backgroundColor}; + background: ${backgroundColor};Also applies to: 139-140, 218-219
shesha-reactjs/src/components/sidebarContainer/styles/svg/dropHint.tsx (1)
3-3: Add accessibility attributes to decorative SVG.Mark as decorative to keep it out of the accessibility tree.
-export const dropHint = <svg xmlns="http://www.w3.org/2000/svg" width="280" height="180" viewBox="0 0 280 180" fill="none"> +export const dropHint = <svg xmlns="http://www.w3.org/2000/svg" width="280" height="180" viewBox="0 0 280 180" fill="none" aria-hidden="true" focusable="false" role="img">shesha-reactjs/src/components/formDesigner/styles/styles.ts (1)
392-397: DRY the drop-hint artwork (duplicated SVG).The inline data-URL duplicates the new dropHint asset. Prefer a single source of truth:
- Store the SVG once (e.g., as
drop-hint.svgin an assets folder).- Use bundler asset URLs for CSS backgrounds and import the same file as a ReactComponent for inline usage.
- This aligns with our prior learning about immediate background initialization while avoiding drift.
shesha-reactjs/src/providers/canvas/utils.ts (1)
11-16: Desktop width changed to fixed 1024px — consider responsive clamp to avoid unintended horizontal scroll.Hardcoding 1024px can overflow narrower viewports or containers. Consider a responsive cap.
Apply one of these:
- return deviceType === 'desktop' - ? '1024px' + return deviceType === 'desktop' + ? 'min(1024px, 100%)' : deviceType === 'tablet' ? '724px' : '599px';or, if you prefer CSS math:
- ? '1024px' + ? 'clamp(724px, 100%, 1024px)'shesha-reactjs/src/components/sidebarContainer/styles/styles.ts (1)
36-41: Conflicting heights:height: 100%vsheight: 85vh.Both are set on the same block; later
85vhwins but this is confusing and brittle. Also, other descendants (e.g.,.sidebar-body-content.open) setheight: 85vhtoo, risking double scrollbars.Consider removing the redundant line or using a single calc based on header height:
- height: 100%; + /* Single source of truth for vertical sizing */ - height: 85vh; + height: calc(100vh - 102px); /* aligns with sidebars' min-height */shesha-reactjs/src/designer-components/_settings/utils/CustomDropdown.tsx (1)
24-31: Controlled vs defaulted Select: pick one.
valueis required (controlled), yetdefaultValueis also passed (ignored). Either makevalueoptional, or dropdefaultValueto avoid confusion.- defaultValue?: string; + // defaultValue unnecessary if component is controlled @@ - defaultValue={defaultValue}Also applies to: 93-94
shesha-reactjs/src/components/sidebarContainer/canvasUtils.ts (3)
34-53: SSR safety and magic numbers.
windowusage will throw during SSR. Also55and16are magic numbers.export function calculateAutoZoom(params: IAutoZoomParams): number { - const { designerWidth = '1024px', sizes = [20, 60, 20] } = params; + const { designerWidth = '1024px', sizes = DEFAULT_OPTIONS.defaultSizes } = params; + if (typeof window === 'undefined') return 100; - const availableWidthPercent = sizes[1]; - const availableWidth = (availableWidthPercent / 100) * (window.innerWidth - 55 - 16); + const availableWidthPercent = sizes[1]; + const SIDEBAR_GUTTER = 55; + const CANVAS_PADDING = 16; + const availableWidth = (availableWidthPercent / 100) * (window.innerWidth - SIDEBAR_GUTTER - CANVAS_PADDING);Also,
currentZoominIAutoZoomParamsis unused—either remove it from the type or document intent.
55-63: Align pinch zoom defaults with global defaults.
usePinchZoomusesminZoom=10,maxZoom=200, whileDEFAULT_OPTIONSsetsminZoom=25. Prefer consistency.- minZoom: number = 10, - maxZoom: number = 200, + minZoom: number = DEFAULT_OPTIONS.minZoom, + maxZoom: number = DEFAULT_OPTIONS.maxZoom,
99-103: Minor: normalize rounding behavior.Wheel zoom returns non-integers while pinch zoom rounds. Pick one for consistency.
- const newZoom = Math.max(minZoom, Math.min(maxZoom, currentZoom + delta)); - onZoomChange(newZoom); + const newZoom = Math.max(minZoom, Math.min(maxZoom, currentZoom + delta)); + onZoomChange(Math.round(newZoom));shesha-reactjs/src/components/formDesigner/designerMainArea/index.tsx (1)
73-80: Extraneous wrapper<div>aroundConfigurableFormRenderer.If not used for refs or styling, prefer a fragment to avoid extra DOM. If it will host pinch-zoom ref later, add a clear comment/class.
- <div> - <ConfigurableFormRenderer form={form} className={formMode === 'designer' ? styles.designerWorkArea : undefined} > + <> + <ConfigurableFormRenderer form={form} className={formMode === 'designer' ? styles.designerWorkArea : undefined}> {isDebug && ( <DebugPanel /> )} - </ConfigurableFormRenderer> - </div> + </ConfigurableFormRenderer> + </>shesha-reactjs/src/providers/canvas/contexts.ts (1)
7-7: Align types and firm up the new auto-zoom API
- Make
autoZoomnon-optional in state (it has a defined initial value).- Keep action name camelCase across the codebase (see actions.ts/index.tsx).
- Use
IDeviceTypesforICanvasWidthProps.deviceTypeto remove casts downstream.Apply:
export interface ICanvasStateContext { zoom?: number; - autoZoom?: boolean; + autoZoom: boolean; designerWidth?: string; designerDevice?: IDeviceTypes; physicalDevice?: IDeviceTypes; activeDevice?: IDeviceTypes; } export interface ICanvasWidthProps { width: number | string; - deviceType: string; + deviceType: IDeviceTypes; }Also applies to: 22-22, 28-28
shesha-reactjs/src/providers/canvas/index.tsx (4)
3-3: Use camelCase action name in imports for consistencyRename the creator to camelCase and import it accordingly. Also type it to accept no payload (see actions.ts diff).
-import { SetCanvasAutoZoomAction, setCanvasWidthAction, setCanvasZoomAction, setDesignerDeviceAction, setScreenWidthAction } from './actions'; +import { setCanvasAutoZoomAction, setCanvasWidthAction, setCanvasZoomAction, setDesignerDeviceAction, setScreenWidthAction } from './actions';
57-59: Dispatch the camelCase, payload-less actionPrevents accidental payload passing and aligns with existing naming.
- const setCanvasAutoZoom = () => { - dispatch(SetCanvasAutoZoomAction()) - } + const setCanvasAutoZoom = () => { + dispatch(setCanvasAutoZoomAction()); + };
62-67: Add trailing comma and fix setCanvasWidth signature to match context
- Add a trailing comma to ease future insertions.
- Signature should accept
number | stringandIDeviceTypes(removes reducer casts).- const setCanvasWidth = (width: number, deviceType: string) => { + const setCanvasWidth = (width: number | string, deviceType: IDeviceTypes) => { dispatch(setCanvasWidthAction({ width, deviceType })); };const actions ={ setDesignerDevice, setCanvasWidth, - setCanvasZoom, - setCanvasAutoZoom + setCanvasZoom, + setCanvasAutoZoom, /* NEW_ACTION_GOES_HERE */ };
26-34: Expose autoZoom in the DataContext metadataWithout it,
autoZoomisn’t discoverable/consumable via the context API tooling.properties: [ { path: 'zoom', dataType: DataTypes.number }, { path: 'designerWidth', dataType: DataTypes.string }, { path: 'designerDevice', dataType: DataTypes.string }, { path: 'physicalDevice', dataType: DataTypes.string }, { path: 'activeDevice', dataType: DataTypes.string }, + { path: 'autoZoom', dataType: DataTypes.boolean }, ],shesha-reactjs/src/providers/canvas/actions.ts (1)
9-9: CamelCase the creator and type it as payload-lessKeeps API consistent with existing creators and prevents accidental payloads.
-export const SetCanvasAutoZoomAction = createAction(CanvasConfigActionEnums.SetCanvasAutoZoom); +export const setCanvasAutoZoomAction = createAction<void>(CanvasConfigActionEnums.SetCanvasAutoZoom);Also applies to: 21-22
shesha-reactjs/src/providers/canvas/reducer.ts (2)
22-22: Remove cast by tightening types at the sourceOnce
ICanvasWidthProps.deviceTypeisIDeviceTypes, this cast is unnecessary.- designerDevice: deviceType as IDeviceTypes, + designerDevice: deviceType,
44-50: Toggle logic is fine; consider zoom sync when (re)enabling auto-zoomIf auto-zoom implies recalculating
zoomto fit, wire that recalculation here or via a side-effect. Otherwise users might toggle auto-zoom on and see no change until another event fires.shesha-reactjs/src/designer-components/_settings/utils/dimensions/utils.tsx (1)
9-11: Edge case: subtracting 'auto' margins yields invalid CSS calc.If margins are 'auto', calc(... - auto) is invalid. Consider ignoring 'auto' (treat as 0) or handling it upstream.
shesha-reactjs/src/components/formDesigner/toolbar/canvasConfig.tsx (2)
21-27: AntD Tooltip + disabled Button: wrap Button with a span or Tooltip won’t show.Tooltips don’t trigger on disabled buttons. Wrap the Button with a span for both zoom out/in buttons.
Example:
-<Tooltip title={`${zoom}%`}><Button size='small' disabled={autoZoom} type='text' icon={<MinusOutlined />} title='Zoom out' onClick={() => setCanvasZoom(zoom - (zoom > DEFAULT_OPTIONS.minZoom ? 2 : 0))} /></Tooltip> +<Tooltip title={`${zoom}%`}> + <span> + <Button size='small' disabled={autoZoom} type='text' icon={<MinusOutlined />} title='Zoom out' + onClick={() => setCanvasZoom(zoom - (zoom > DEFAULT_OPTIONS.minZoom ? 2 : 0))} /> + </span> +</Tooltip>Repeat for the Zoom in button.
21-23: Icon size prop is not supported; use style={{ fontSize }}.@ant-design/icons ignore non-standard size prop. Use style.
- icon={<ExpandOutlined size={14}/>} + icon={<ExpandOutlined style={{ fontSize: 14 }} />}shesha-reactjs/src/components/sidebarContainer/index.tsx (3)
52-55: Effect deps may be stale w.r.t. zoom.You pass currentZoom: zoom into calculateAutoZoom but omit zoom from deps. Either add zoom to deps or remove it from the call if unused internally.
- }, [isOpenRight, isOpenLeft, autoZoom, designerDevice, designerWidth, currentSizes]); + }, [isOpenRight, isOpenLeft, autoZoom, designerDevice, designerWidth, currentSizes, zoom]);
11-12: Inconsistent useCanvas import paths.This file imports from '@/index', others from '@/providers'. Prefer one path to avoid multiple contexts or circular deps.
113-113: CSS zoom is non-standard; consider a transform fallback.zoom is unsupported in Firefox. Consider transform: scale with transformOrigin: 'top center' as a progressive enhancement.
shesha-reactjs/src/components/formDesigner/toolbar/mobileDropdown.tsx (2)
62-65: labelRender fallback for custom widths.If value isn’t in options, option is undefined and the icon disappears. Provide a fallback icon by width range.
- labelRender={({ label, value }) => { - const option = options.find(opt => opt.value === value); - return <Tooltip title={value}>{option?.icon} {label}</Tooltip> - } + labelRender={({ label, value }) => { + const option = options.find(opt => opt.value === value); + const num = parseInt(String(value), 10); + const fallbackIcon = num > 850 ? <DesktopOutlined/> : num > 430 ? <TabletOutlined/> : <MobileOutlined/>; + return <Tooltip title={value}>{option?.icon ?? fallbackIcon} {label ?? value}</Tooltip>; + }
10-10: Align useCanvas import path with other files.Other files import from '@/providers'. Standardize.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
shesha-reactjs/src/components/formDesigner/designerMainArea/index.tsx(2 hunks)shesha-reactjs/src/components/formDesigner/styles/styles.ts(3 hunks)shesha-reactjs/src/components/formDesigner/toolbar/canvasConfig.tsx(1 hunks)shesha-reactjs/src/components/formDesigner/toolbar/mobileDropdown.tsx(1 hunks)shesha-reactjs/src/components/mainLayout/styles/styles.ts(1 hunks)shesha-reactjs/src/components/sidebarContainer/canvasUtils.ts(1 hunks)shesha-reactjs/src/components/sidebarContainer/index.tsx(5 hunks)shesha-reactjs/src/components/sidebarContainer/styles/styles.ts(1 hunks)shesha-reactjs/src/components/sidebarContainer/styles/svg/dropHint.tsx(1 hunks)shesha-reactjs/src/designer-components/_settings/utils/CustomDropdown.tsx(3 hunks)shesha-reactjs/src/designer-components/_settings/utils/dimensions/utils.tsx(1 hunks)shesha-reactjs/src/hooks/formComponentHooks.ts(3 hunks)shesha-reactjs/src/providers/canvas/actions.ts(2 hunks)shesha-reactjs/src/providers/canvas/contexts.ts(2 hunks)shesha-reactjs/src/providers/canvas/index.tsx(2 hunks)shesha-reactjs/src/providers/canvas/reducer.ts(2 hunks)shesha-reactjs/src/providers/canvas/utils.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-05-28T07:55:21.036Z
Learnt from: teboho
PR: shesha-io/shesha-framework#3312
File: shesha-reactjs/src/hooks/formComponentHooks.ts:183-192
Timestamp: 2025-05-28T07:55:21.036Z
Learning: In shesha-reactjs formComponentHooks.ts, the background style initialization deliberately sets valid values from properties panel immediately to provide better UX, rather than waiting for the useDeepCompareEffect to run. This intentional pattern prevents a flash of no background and shows the background image immediately when the component renders.
Applied to files:
shesha-reactjs/src/components/formDesigner/designerMainArea/index.tsxshesha-reactjs/src/hooks/formComponentHooks.tsshesha-reactjs/src/components/formDesigner/styles/styles.ts
📚 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/components/formDesigner/designerMainArea/index.tsxshesha-reactjs/src/components/formDesigner/toolbar/canvasConfig.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/hooks/formComponentHooks.ts
🪛 GitHub Actions: shesha-reactjs-build
shesha-reactjs/src/components/formDesigner/toolbar/mobileDropdown.tsx
[error] 2-2: Rollup/TypeScript TS6133: 'Select' is declared but its value is never read.
🔇 Additional comments (9)
shesha-reactjs/src/components/mainLayout/styles/styles.ts (1)
157-160: Removed stray token — good catch.Eliminates an invalid character inside the sider children block; avoids CSS parse noise. No behavioral change.
shesha-reactjs/src/components/formDesigner/styles/styles.ts (3)
178-178: Toolbar cross-axis centering — looks good.Aligning items vertically improves the toolbar’s visual balance.
379-380: Drop hint height increase acknowledged.The taller 55px hint should improve drop-target affordance. No functional concerns.
386-397: Empty-drop-area styling: streamline background, verify:has()support, and check scroll impact.Minor cleanup and verifications:
- Remove the redundant
repeatin the background shorthand since you override it withbackground-repeat: no-repeat;.- Confirm that using the CSS relational selector
:has()is acceptable for your supported browsers—add a safe fallback (e.g. a[data-empty]attribute or:empty) if some enterprise environments disable it.- Verify that
min-height: 85vhdoesn’t introduce unwanted double-scrollbars in your quick-edit modals or on very short viewports.Proposed diff:
- background: url("data:image/svg+xml,...") repeat; - background-size: 25vw; - background-repeat: no-repeat; - background-position: 50% 50%; + background-image: url("data:image/svg+xml,..."); + background-size: 25vw; + background-repeat: no-repeat; + background-position: 50% 50%;Please double-check your browserslist config and scroll behavior manually.
shesha-reactjs/src/designer-components/_settings/utils/CustomDropdown.tsx (1)
33-37: ForwardinglabelRender,popupMatchSelectWidth, andstylelooks good.
labelRenderis supported by rc-select (the underlying Select) and is passed through by AntD v5. (npmjs.com)shesha-reactjs/src/components/sidebarContainer/canvasUtils.ts (1)
2-18:widthRelativeToCanvasOK for simplevwinputs.Covers numeric
vwand numbers; defers all else. Consider extending to handle complex calc() expressions in the future if needed.shesha-reactjs/src/hooks/formComponentHooks.ts (1)
13-14: Confirm provider presence for useCanvas()
useCanvas()throws when not underCanvasProvider(defaultrequire = true). Ensure all consumers ofuseFormComponentStylesare always wrapped.Also applies to: 181-181
shesha-reactjs/src/components/formDesigner/toolbar/canvasConfig.tsx (1)
25-26: Zoom clamping logic looks good.Nice, prevents overflow/underflow without extra state churn.
shesha-reactjs/src/components/sidebarContainer/index.tsx (1)
53-55: Verify presence and type ofdesignerWidthusageI wasn’t able to locate any occurrences of
designerWidthorsetCanvasWidthin the codebase. Please confirm that:
- The file
shesha-reactjs/src/components/sidebarContainer/index.tsxis present and up-to-date in the branch.- The
designerWidthprop and thesetCanvasWidthAPI are defined and consumed consistently in your Canvas provider.Assuming the code exists as shown, the original suggestion applies: standardize the type of
designerWidthacross your API—either always pass a number (e.g.1024) or always pass a string with a unit (e.g.'1024px')—to avoid potential UI mismatches.
shesha-reactjs/src/components/formDesigner/toolbar/mobileDropdown.tsx
Outdated
Show resolved
Hide resolved
shesha-reactjs/src/components/formDesigner/toolbar/mobileDropdown.tsx
Outdated
Show resolved
Hide resolved
shesha-reactjs/src/designer-components/_settings/utils/CustomDropdown.tsx
Outdated
Show resolved
Hide resolved
shesha-reactjs/src/designer-components/_settings/utils/dimensions/utils.tsx
Outdated
Show resolved
Hide resolved
shesha-reactjs/src/designer-components/_settings/utils/dimensions/utils.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
shesha-reactjs/src/hooks/formComponentHooks.ts (1)
198-198: Stale memo fix looks correct (deps include designerWidth).This resolves the earlier memoization issue on canvas width changes.
Also, confirm
designerWidthis numeric as expected bygetDimensionsStyle; otherwise coerce before passing.#!/bin/bash # Inspect designerWidth usage/type at source rg -nP -C3 '\bdesignerWidth\b'shesha-reactjs/src/components/formDesigner/styles/styles.ts (1)
240-240: Fix CSS typo: justifyCoontent → justify-content.Rule is a no-op right now.
Apply:
- justifyCoontent: center; + justify-content: center;shesha-reactjs/src/components/formDesigner/toolbar/mobileDropdown.tsx (2)
2-2: Build fix retained.Unused
Selectimport removed; avoids TS6133.
26-26: Normalize controlled value to match options (px string).Prevents selection highlight mismatch.
Apply:
- value={designerWidth} + value={typeof designerWidth === 'number' ? `${designerWidth}px` : designerWidth}shesha-reactjs/src/components/sidebarContainer/canvasUtils.ts (2)
34-53: Unify defaults with DEFAULT_OPTIONS, clamp via constants, and add SSR safety.Also replace magic numbers with named locals. This resolves the earlier “inconsistent defaults” feedback.
export function calculateAutoZoom(params: IAutoZoomParams): number { - const { designerWidth = '1024px', sizes = [20, 60, 20] } = params; - - const availableWidthPercent = sizes[1]; - const availableWidth = (availableWidthPercent / 100) * (window.innerWidth - 55 - 16); - - let canvasWidth: number; - if (designerWidth.includes('px')) { - canvasWidth = parseFloat(designerWidth.replace('px', '')); - } else if (designerWidth.includes('vw')) { - const vwValue = parseFloat(designerWidth.replace('vw', '')); - canvasWidth = (vwValue / 100) * window.innerWidth; - } else { - canvasWidth = parseFloat(designerWidth) || 1024; - } - - const optimalZoom = (availableWidth / canvasWidth) * 100; - - return Math.max(25, Math.min(200, Math.round(optimalZoom))); + const { designerWidth = '1024px', sizes = DEFAULT_OPTIONS.defaultSizes } = params; + + // SSR-safe window usage and name the layout constants + const innerWidth = typeof window !== 'undefined' ? window.innerWidth : 1024; + const NAV_WIDTH = 55; // TODO: import from layout constants + const GUTTER_X = 16; // TODO: import from layout constants + + const mid = Array.isArray(sizes) ? Number(sizes[1]) : NaN; + const availableWidthPercent = Number.isFinite(mid) ? mid : DEFAULT_OPTIONS.defaultSizes[1]; + const availableWidth = (availableWidthPercent / 100) * (innerWidth - NAV_WIDTH - GUTTER_X); + + const normalized = String(designerWidth).trim().toLowerCase(); + let canvasWidth: number; + if (normalized.endsWith('px')) { + canvasWidth = parseFloat(normalized); + } else if (normalized.endsWith('vw')) { + const vwValue = parseFloat(normalized); + canvasWidth = (vwValue / 100) * innerWidth; + } else { + canvasWidth = parseFloat(normalized) || 1024; + } + + const optimalZoom = (availableWidth / canvasWidth) * 100; + const { minZoom, maxZoom } = DEFAULT_OPTIONS; + return Math.min(maxZoom, Math.max(minZoom, Math.round(optimalZoom))); }
111-119: Pinch listeners aren’t attached and null ref is not guarded — touch zoom won’t work.Add touchstart/touchmove/touchend with proper passive flags and cleanup; guard null.
useEffect(() => { - const element = elementRef?.current; - - element.addEventListener('wheel', handleWheel, { passive: false }); - - return () => { - element.removeEventListener('wheel', handleWheel); - }; + const element = elementRef.current; + if (!element) return; + + // Touch listeners for pinch zoom + element.addEventListener('touchstart', handleTouchStart as EventListener, { passive: false }); + element.addEventListener('touchmove', handleTouchMove as EventListener, { passive: false }); + element.addEventListener('touchend', handleTouchEnd as EventListener, { passive: true }); + // Ctrl + wheel zoom + element.addEventListener('wheel', handleWheel, { passive: false }); + + return () => { + element.removeEventListener('touchstart', handleTouchStart as EventListener); + element.removeEventListener('touchmove', handleTouchMove as EventListener); + element.removeEventListener('touchend', handleTouchEnd as EventListener); + element.removeEventListener('wheel', handleWheel); + }; }, [handleTouchStart, handleTouchMove, handleTouchEnd, handleWheel]);
🧹 Nitpick comments (7)
shesha-reactjs/src/components/formDesigner/utils/svgConstants.ts (1)
39-116: Inline SVG data URL: acceptable trade-off.Large but one-time constant; fine for a designer-only surface.
If bundle size becomes a concern, consider loading this via asset file or lazy getter.
shesha-reactjs/src/components/formDesigner/styles/styles.ts (2)
380-381: Drop hint height tweak: fine.Explicit 55px is acceptable here. Consider using token or rems later for consistency.
388-403: Background shorthand conflicts and :has() compatibility.
- Shorthand sets
repeatbut later you setbackground-repeat: no-repeat;. Drop therepeattoken to avoid confusion.:has()is widely supported in Chromium/WebKit but only relatively recently in Firefox. If designers use Firefox ESR, consider a JS-toggled class fallback.Apply:
- > div:not(.sha-drop-hint):not(:has(.sha-component)) { - background: url("${getFormDesignerBackgroundSvg()}") repeat; - background-size: 25vw; - background-repeat: no-repeat; - background-position: 50% 50%; - } + > div:not(.sha-drop-hint):not(:has(.sha-component)) { + background: url("${getFormDesignerBackgroundSvg()}"); + background-size: 25vw; + background-repeat: no-repeat; + background-position: 50% 50%; + }If a fallback is needed, add an
empty-canvasclass from the container when no.sha-componentexists and target that instead of:has()./* fallback */ > div.empty-canvas { background: url("..."); /* same as above */ }To check Firefox versions in your user base, please confirm with analytics or QA matrix.
shesha-reactjs/src/components/formDesigner/toolbar/mobileDropdown.tsx (2)
17-17: Controlled component: drop defaultValue.You pass
value, sodefaultValueis ignored and misleading.Apply:
- defaultValue={'1024px'}
23-25: Type-safety and centralizing device type thresholds.
- Cast to string before
parseIntto avoid surprises.- Consider centralizing breakpoints to a util to keep Designer consistent.
Apply:
- const value = parseInt(val, 10); + const value = parseInt(String(val), 10); - setCanvasWidth(value, value > 850 ? 'desktop' : value > 430 ? 'tablet' : 'mobile'); + setCanvasWidth( + value, + value > 850 ? 'desktop' : value > 430 ? 'tablet' : 'mobile' + );Optionally, move the breakpoint logic to a
getDeviceType(width)helper used across the designer.shesha-reactjs/src/components/sidebarContainer/canvasUtils.ts (2)
66-73: Minor: use Math.hypot for clarity.- return Math.sqrt( - Math.pow(touch2.clientX - touch1.clientX, 2) + - Math.pow(touch2.clientY - touch1.clientY, 2) - ); + return Math.hypot( + touch2.clientX - touch1.clientX, + touch2.clientY - touch1.clientY + );
96-104: Normalize wheel zoom to integer steps.Match pinch behavior to avoid fractional drift.
- const newZoom = Math.max(minZoom, Math.min(maxZoom, currentZoom + delta)); - onZoomChange(newZoom); + const newZoom = Math.max(minZoom, Math.min(maxZoom, Math.round(currentZoom + delta))); + onZoomChange(newZoom);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
shesha-reactjs/src/components/formDesigner/styles/styles.ts(4 hunks)shesha-reactjs/src/components/formDesigner/toolbar/mobileDropdown.tsx(1 hunks)shesha-reactjs/src/components/formDesigner/utils/svgConstants.ts(1 hunks)shesha-reactjs/src/components/sidebarContainer/canvasUtils.ts(1 hunks)shesha-reactjs/src/components/sidebarContainer/index.tsx(5 hunks)shesha-reactjs/src/hooks/formComponentHooks.ts(3 hunks)shesha-reactjs/src/providers/canvas/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- shesha-reactjs/src/providers/canvas/index.tsx
- shesha-reactjs/src/components/sidebarContainer/index.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-28T07:55:21.036Z
Learnt from: teboho
PR: shesha-io/shesha-framework#3312
File: shesha-reactjs/src/hooks/formComponentHooks.ts:183-192
Timestamp: 2025-05-28T07:55:21.036Z
Learning: In shesha-reactjs formComponentHooks.ts, the background style initialization deliberately sets valid values from properties panel immediately to provide better UX, rather than waiting for the useDeepCompareEffect to run. This intentional pattern prevents a flash of no background and shows the background image immediately when the component renders.
Applied to files:
shesha-reactjs/src/hooks/formComponentHooks.tsshesha-reactjs/src/components/formDesigner/styles/styles.ts
📚 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/hooks/formComponentHooks.ts
⏰ 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: build-attempt
🔇 Additional comments (11)
shesha-reactjs/src/hooks/formComponentHooks.ts (2)
13-17: Good addition: canvas context wired in.Importing
useCanvashere makes the styles hook canvas-aware without leaking concerns into callers.
181-182: Confirm safeuseCanvasfallback outside designer runtime
I wasn’t able to locate theuseCanvasimplementation or its provider via automated search. Please manually verify:
- The
useCanvashook returns sensible default values (e.g., no-op or zero dimensions) when noCanvasProvideris in scope.- Runtime forms that invoke
useFormComponentStyles(and thus calluseCanvas) will not crash in production.Affected location:
- shesha-reactjs/src/hooks/formComponentHooks.ts (lines 181–182) —
const {designerWidth} = useCanvas();shesha-reactjs/src/components/formDesigner/utils/svgConstants.ts (2)
3-37: Device options data: OK.Consistent labels/px-string values and icon refs will render cleanly in consumers.
118-118: Accessor helper is fine.Simple export keeps import sites tidy.
shesha-reactjs/src/components/formDesigner/styles/styles.ts (2)
3-3: Import of background helper: OK.Keeps assets centralized.
179-179: Toolbar vertical centering: LGTM.
align-items: center;improves toolbar alignment.shesha-reactjs/src/components/formDesigner/toolbar/mobileDropdown.tsx (1)
10-11: Canvas integration: LGTM.
setCanvasWidthanddesignerWidthwiring aligns with the new canvas config.shesha-reactjs/src/components/sidebarContainer/canvasUtils.ts (4)
2-18: widthRelativeToCanvas: straightforward and correct.Covers numeric, vw, and passthrough cases cleanly. No issues.
28-33: DEFAULT_OPTIONS look good.Single source for min/max and defaultSizes is helpful.
55-61: Unable to locatecanvasUtils.tsand related identifiers—please verify manually
I wasn’t able to find the referenced file or any occurrences ofDEFAULT_OPTIONSorisAutoWidthin the codebase. It’s possible the path has changed, the file is in a submodule, or the identifiers have been renamed.• Confirm that
shesha-reactjs/src/components/sidebarContainer/canvasUtils.tsexists on this branch
• Ensure theDEFAULT_OPTIONSconstant is imported and used for default zoom values
• Check whether the flagisAutoWidth(or its intended new nameisAutoZoom) appears elsewhere and update all call sites if renaming
22-26: IAutoZoomParams.currentZoom usage needs verificationI wasn’t able to locate
canvasUtils.tsor any usages ofIAutoZoomParams/currentZoomin the repo. To avoid unintended breakage, please manually confirm:
- That
src/components/sidebarContainer/canvasUtils.tsstill exists.- That no code consumes
IAutoZoomParams.currentZoom.You can run a full-text search such as:
grep -R "IAutoZoomParams" -n . grep -R "currentZoom" -n .If you verify that
currentZoomis indeed unused (or only reserved for future smoothing), then making it optional is safe:export interface IAutoZoomParams { - currentZoom: number; + currentZoom?: number; designerWidth?: string; sizes?: number[]; }
shesha-reactjs/src/components/formDesigner/toolbar/mobileDropdown.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
shesha-reactjs/src/components/sidebarContainer/index.tsx (1)
11-11: Avoid drift: reuse shared zoom bounds (DEFAULT_OPTIONS) instead of hard-coded 25/200.This keeps pinch/wheel zoom consistent with the toolbar’s config.
-import { calculateAutoZoom, usePinchZoom } from './canvasUtils'; +import { calculateAutoZoom, usePinchZoom, DEFAULT_OPTIONS } from './canvasUtils'; @@ const canvasRef = usePinchZoom( handleZoomChange, zoom, - 25, - 200, + DEFAULT_OPTIONS.minZoom, + DEFAULT_OPTIONS.maxZoom, autoZoom );Also applies to: 46-52
shesha-reactjs/src/components/sidebarContainer/canvasUtils.ts (2)
35-36: Single source of truth for defaults: use DEFAULT_OPTIONS in auto-zoom.Align defaultSizes and clamp bounds with DEFAULT_OPTIONS.
export function calculateAutoZoom(params: IAutoZoomParams): number { - const { designerWidth = '1024px', sizes = [20, 60, 20], isSidebarCollapsed } = params; + const { designerWidth = '1024px', sizes = DEFAULT_OPTIONS.defaultSizes, isSidebarCollapsed } = params; @@ - return Math.max(25, Math.min(200, Math.round(optimalZoom))); + return Math.max( + DEFAULT_OPTIONS.minZoom, + Math.min(DEFAULT_OPTIONS.maxZoom, Math.round(optimalZoom)) + ); }Also applies to: 54-55
57-63: Pinch zoom currently inoperative; attach touch listeners and add null guard.The hook only binds wheel; pinch never fires. Also round wheel zoom for consistency.
export const usePinchZoom = ( onZoomChange: (zoom: number) => void, currentZoom: number, - minZoom: number = 10, - maxZoom: number = 200, + minZoom: number = DEFAULT_OPTIONS.minZoom, + maxZoom: number = DEFAULT_OPTIONS.maxZoom, isAutoWidth: boolean = false ) => { @@ e.preventDefault(); - const delta = e.deltaY > 0 ? -5 : 5; - const newZoom = Math.max(minZoom, Math.min(maxZoom, currentZoom + delta)); - onZoomChange(newZoom); + const delta = e.deltaY > 0 ? -5 : 5; + const newZoom = Math.max(minZoom, Math.min(maxZoom, currentZoom + delta)); + onZoomChange(Math.round(newZoom)); }, [onZoomChange, currentZoom, minZoom, maxZoom, isAutoWidth]); @@ - useEffect(() => { - const element = elementRef?.current; - - element.addEventListener('wheel', handleWheel, { passive: false }); - - return () => { - element.removeEventListener('wheel', handleWheel); - }; - }, [handleTouchStart, handleTouchMove, handleTouchEnd, handleWheel]); + useEffect(() => { + const element = elementRef.current; + if (!element) return; + // Touch listeners for pinch zoom + element.addEventListener('touchstart', handleTouchStart as any, { passive: false }); + element.addEventListener('touchmove', handleTouchMove as any, { passive: false }); + element.addEventListener('touchend', handleTouchEnd as any, { passive: true }); + // Ctrl + wheel zoom + element.addEventListener('wheel', handleWheel, { passive: false }); + + return () => { + element.removeEventListener('touchstart', handleTouchStart as any); + element.removeEventListener('touchmove', handleTouchMove as any); + element.removeEventListener('touchend', handleTouchEnd as any); + element.removeEventListener('wheel', handleWheel); + }; + }, [handleTouchStart, handleTouchMove, handleTouchEnd, handleWheel]);Also applies to: 101-105, 113-121
🧹 Nitpick comments (4)
shesha-reactjs/src/components/sidebarContainer/index.tsx (2)
54-57: Side effect dependencies look intentional; minor improvement suggestion.No need to add zoom to deps since you set it. Consider computing isSidebarCollapsed inside this effect if you expect it to change outside React.
116-116: CSS zoom has cross-browser quirks; consider transform: scale for better support.Optional, but improves fidelity and avoids layout bugs in Firefox.
- <div ref={canvasRef} className={styles.sidebarContainerMainAreaBody} style={{ width: designerWidth, zoom: `${zoom}%`, overflow: 'auto', margin: '0 auto' }}>{children}</div> + <div + ref={canvasRef} + className={styles.sidebarContainerMainAreaBody} + style={{ + width: designerWidth, + transform: `scale(${zoom / 100})`, + transformOrigin: 'center top', + overflow: 'auto', + margin: '0 auto' + }} + > + {children} + </div>shesha-reactjs/src/components/sidebarContainer/canvasUtils.ts (2)
22-27: currentZoom in IAutoZoomParams is unused.Either remove it or use it to add hysteresis/smoothing; keep the interface minimal.
37-41: Magic numbers (55, 206, 16) obscure intent.Consider moving these to DEFAULT_OPTIONS with names (e.g., gutterWidth, sideMenuOpenWidth, sideMenuCollapsedWidth).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
shesha-reactjs/src/components/sidebarContainer/canvasUtils.ts(1 hunks)shesha-reactjs/src/components/sidebarContainer/index.tsx(5 hunks)
⏰ 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: build-attempt
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…sha-framework into thulasizwe/en/canvas
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
shesha-reactjs/src/components/sidebarContainer/index.tsx (1)
51-57: Avoid drift: use shared min/max zoom constants instead of hard-coded 25/200.Reuse DEFAULT_OPTIONS to keep bounds consistent with toolbar and utils.
-import { calculateAutoZoom, usePinchZoom } from './canvasUtils'; +import { calculateAutoZoom, usePinchZoom, DEFAULT_OPTIONS } from './canvasUtils'; @@ const canvasRef = usePinchZoom( handleZoomChange, zoom, - 25, - 200, + DEFAULT_OPTIONS.minZoom, + DEFAULT_OPTIONS.maxZoom, autoZoom );shesha-reactjs/src/components/sidebarContainer/canvasUtils.ts (1)
113-121: Touch pinch listeners missing and null-ref risk on mount.
- Pinch never works because touch handlers aren’t attached.
- elementRef.current may be null; guard add/remove.
- useEffect(() => { - const element = elementRef?.current; - - element.addEventListener('wheel', handleWheel, { passive: false }); - - return () => { - element.removeEventListener('wheel', handleWheel); - }; - }, [handleTouchStart, handleTouchMove, handleTouchEnd, handleWheel]); + useEffect(() => { + const element = elementRef.current; + if (!element) return; + + // Touch listeners for pinch + element.addEventListener('touchstart', handleTouchStart as any, { passive: false }); + element.addEventListener('touchmove', handleTouchMove as any, { passive: false }); + element.addEventListener('touchend', handleTouchEnd as any, { passive: true }); + // Ctrl + wheel zoom + element.addEventListener('wheel', handleWheel, { passive: false }); + + return () => { + element.removeEventListener('touchstart', handleTouchStart as any); + element.removeEventListener('touchmove', handleTouchMove as any); + element.removeEventListener('touchend', handleTouchEnd as any); + element.removeEventListener('wheel', handleWheel); + }; + }, [handleTouchStart, handleTouchMove, handleTouchEnd, handleWheel]);
🧹 Nitpick comments (5)
shesha-reactjs/src/components/sidebarContainer/index.tsx (3)
36-38: Make sidebar collapse state reactive; don’t read localStorage during render.Current value won’t update when the user toggles collapse. Use state + effect.
- const isSidebarCollapsed = - typeof window !== 'undefined' && - localStorage.getItem('SIDEBAR_COLLAPSE') === 'true'; + const [isSidebarCollapsed, setIsSidebarCollapsed] = useState(false);Add this effect (outside the range shown):
useEffect(() => { if (typeof window === 'undefined') return; const read = () => setIsSidebarCollapsed(localStorage.getItem('SIDEBAR_COLLAPSE') === 'true'); read(); const onStorage = (e: StorageEvent) => { if (e.key === 'SIDEBAR_COLLAPSE') read(); }; window.addEventListener('storage', onStorage); return () => window.removeEventListener('storage', onStorage); }, []);
59-63: Add renderSource to dependencies to keep auto-zoom in sync.Prevents stale zoom when switching modal vs page contexts.
- }, [isOpenRight, isOpenLeft, autoZoom, designerDevice, designerWidth, currentSizes, isSidebarCollapsed]); + }, [isOpenRight, isOpenLeft, autoZoom, designerDevice, designerWidth, currentSizes, isSidebarCollapsed, renderSource]);
123-123: Prefer CSS transform over non-standard zoom.Improves cross-browser behavior (Firefox) and layout consistency.
- <div ref={canvasRef} className={styles.sidebarContainerMainAreaBody} style={isDesigner ? { width: designerWidth, zoom: `${zoom}%`, overflow: 'auto', margin: '0 auto' } : {}}>{children}</div> + <div + ref={canvasRef} + className={styles.sidebarContainerMainAreaBody} + style={ + isDesigner + ? { + width: designerWidth, + transform: `scale(${zoom / 100})`, + transformOrigin: 'top center', + overflow: 'auto', + margin: '0 auto' + } + : {} + } + > + {children} + </div>shesha-reactjs/src/components/sidebarContainer/canvasUtils.ts (2)
57-63: Align pinch defaults with shared constants.Keep defaults consistent across the app.
-export const usePinchZoom = ( +export const usePinchZoom = ( onZoomChange: (zoom: number) => void, currentZoom: number, - minZoom: number = 10, - maxZoom: number = 200, + minZoom: number = DEFAULT_OPTIONS.minZoom, + maxZoom: number = DEFAULT_OPTIONS.maxZoom, isAutoWidth: boolean = false ) => {
2-18: Handle unitless numeric strings in widthRelativeToCanvas.Prevents invalid CSS like "width: '100'".
export function widthRelativeToCanvas(width: string | number, canvasWidth: string = '100vw'): string { if (typeof width === 'number') { return `${width}px`; } const trimmed = String(width).trim(); + // If it's a pure number string, treat as px + if (/^\d+(\.\d+)?$/.test(trimmed)) { + return `${trimmed}px`; + } const vwMatch = trimmed.match(/^([\d.]+)\s*vw$/i);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
shesha-reactjs/src/components/formDesigner/designerMainArea/index.tsx(5 hunks)shesha-reactjs/src/components/formDesigner/quickEdit/quickEditDialog.tsx(1 hunks)shesha-reactjs/src/components/formDesigner/styles/styles.ts(4 hunks)shesha-reactjs/src/components/sidebarContainer/canvasUtils.ts(1 hunks)shesha-reactjs/src/components/sidebarContainer/index.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- shesha-reactjs/src/components/formDesigner/styles/styles.ts
⏰ 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: build-attempt
🔇 Additional comments (4)
shesha-reactjs/src/components/formDesigner/quickEdit/quickEditDialog.tsx (1)
43-44: Prop passthrough LGTM.Passing renderSource='modal' keeps auto-zoom/layout logic context-aware.
shesha-reactjs/src/components/formDesigner/designerMainArea/index.tsx (3)
15-17: Public prop addition LGTM.Non-breaking and clearly scoped ("modal" | "designer-page").
61-62: Prop forwarding LGTM.SidebarContainer now receives renderSource for correct canvas calculations.
75-82: Wrapper addition LGTM.Neutral for layout; enables future hooks/styles without touching renderer.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
shesha-reactjs/src/components/sidebarContainer/index.tsx (1)
49-55: Avoid drift: reuse shared zoom bounds (DEFAULT_OPTIONS).Pinch-zoom still hard-codes 25/200. Import and use shared constants to keep toolbar and gestures in sync.
- import { calculateAutoZoom, usePinchZoom } from './canvasUtils'; + import { calculateAutoZoom, usePinchZoom, DEFAULT_OPTIONS } from './canvasUtils'; const canvasRef = usePinchZoom( handleZoomChange, zoom, - 25, - 200, + DEFAULT_OPTIONS.minZoom, + DEFAULT_OPTIONS.maxZoom, autoZoom );
🧹 Nitpick comments (4)
shesha-reactjs/src/components/sidebarContainer/index.tsx (4)
39-43: Remove any-cast; type sizes from getPanelSizes.Aligns with SizableColumns and avoids leaks.
-const [currentSizes, setCurrentSizes] = useState(getPanelSizes(isOpenLeft, isOpenRight, leftSidebarProps, rightSidebarProps, allowFullCollapse).sizes); +type PanelSizes = ReturnType<typeof getPanelSizes>['sizes']; +const [currentSizes, setCurrentSizes] = useState<PanelSizes>( + getPanelSizes(isOpenLeft, isOpenRight, leftSidebarProps, rightSidebarProps, allowFullCollapse).sizes +); -const handleDragSizesChange = useCallback((sizes: number[]) => { - setCurrentSizes(sizes as any); +const handleDragSizesChange = useCallback((sizes: PanelSizes) => { + setCurrentSizes(sizes); }, []);
121-121: Prefer transform over non‑standard CSS zoom; also default width.CSS zoom is non-standard and inconsistent. Use transform scale; ensure width fallback matches setCanvasWidth.
-<div ref={canvasRef} className={styles.sidebarContainerMainAreaBody} style={isDesigner ? { width: designerWidth, zoom: `${zoom}%`, overflow: 'auto', margin: '0 auto' } : {}}>{children}</div> +<div + ref={canvasRef} + className={styles.sidebarContainerMainAreaBody} + style={ + isDesigner + ? { + width: designerWidth ?? '1024px', + transform: `scale(${zoom / 100})`, + transformOrigin: 'top center', + overflow: 'auto', + margin: '0 auto', + } + : {} + } +> + {children} +</div>
97-99: Avoid redundant state updates during drag.onDrag already updates sizes; drop onDragEnd to cut renders.
- onDrag={handleDragSizesChange} - onDragEnd={handleDragSizesChange} + onDrag={handleDragSizesChange}
35-38: Safer read of global flag.Guard undefined to avoid accidental collapse when context not ready.
-const isSidebarCollapsed = !globalVariables.isSideBarExpanded; +const isSidebarCollapsed = !(globalVariables?.isSideBarExpanded ?? true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
shesha-reactjs/src/components/sidebarContainer/index.tsx(5 hunks)
⏰ 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: build-attempt
🔇 Additional comments (1)
shesha-reactjs/src/components/sidebarContainer/index.tsx (1)
16-16: Obsolete review comment:sidebarContainer/index.tsxnot found, nosideBarWidthreferencesLikely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
shesha-reactjs/src/components/sidebarContainer/index.tsx (4)
67-69: Complete dependency list for sizes refresh.Sizes also depend on sidebar props and
allowFullCollapse.- },[isOpenRight, isOpenLeft]); + }, [isOpenRight, isOpenLeft, leftSidebarProps, rightSidebarProps, allowFullCollapse]);
12-20: Type gap: addnoPaddingto the props interface.Prop is used (Line 121) but not typed.
export interface ISidebarContainerProps extends PropsWithChildren<any> { leftSidebarProps?: ISidebarProps; rightSidebarProps?: ISidebarProps; header?: ReactNode | (() => ReactNode); - sideBarWidth?: number; + sideBarWidth?: number; allowFullCollapse?: boolean; renderSource?: 'modal' | 'designer-page'; canZoom?: boolean; + noPadding?: boolean; }
51-57: Avoid drift: use shared min/max zoom constants.Replace hard-coded 25/200 with toolbar’s
DEFAULT_OPTIONS.-import { calculateAutoZoom, usePinchZoom } from './canvasUtils'; +import { calculateAutoZoom, usePinchZoom, DEFAULT_OPTIONS } from './canvasUtils'; const canvasRef = usePinchZoom( handleZoomChange, zoom, - 25, - 200, + DEFAULT_OPTIONS.minZoom, + DEFAULT_OPTIONS.maxZoom, autoZoom );
59-65: Fix stale deps in auto-zoom effect.Effect reads
zoomandrenderSourcebut doesn’t depend on them.- }, [canZoom, isOpenRight, isOpenLeft, autoZoom, designerDevice, designerWidth, currentSizes, isSidebarCollapsed]); + }, [canZoom, isOpenRight, isOpenLeft, autoZoom, designerDevice, designerWidth, currentSizes, isSidebarCollapsed, zoom, renderSource]);
🧹 Nitpick comments (3)
shesha-reactjs/src/components/formDesigner/designerMainArea/index.tsx (2)
25-26: DefaultrenderSourceto stabilize auto-zoom logic.If
renderSourceis undefined,calculateAutoZoommay behave inconsistently. Default to'designer-page'.-export const DesignerMainArea: FC<IDesignerMainAreaProps> = ({ renderSource }) => { +export const DesignerMainArea: FC<IDesignerMainAreaProps> = ({ renderSource = 'designer-page' }) => {
76-83: Wrapper div: keep only if needed for layout/scale.If this wrapper isn’t used for scaling/overflow, consider removing to reduce DOM depth. If you adopt transform-scale (see sidebarContainer comment), this wrapper is a good place to apply it.
shesha-reactjs/src/components/sidebarContainer/index.tsx (1)
125-125: Prefertransform: scaleover non-standardzoom.
zoomis non-standard and can behave inconsistently. Considertransformwith a fixed-width wrapper.- <div ref={canvasRef} className={styles.sidebarContainerMainAreaBody} style={isDesigner && canZoom ? { width: designerWidth, zoom: `${zoom}%`, overflow: 'auto', margin: '0 auto' } : {}}>{children}</div> + <div + ref={canvasRef} + className={styles.sidebarContainerMainAreaBody} + style={ + isDesigner && canZoom + ? { + width: designerWidth, + transform: `scale(${zoom / 100})`, + transformOrigin: 'top center', + overflow: 'auto', + margin: '0 auto' + } + : {} + } + > + {children} + </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
shesha-reactjs/src/components/formDesigner/designerMainArea/index.tsx(5 hunks)shesha-reactjs/src/components/sidebarContainer/index.tsx(5 hunks)
⏰ 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: build-attempt
🔇 Additional comments (3)
shesha-reactjs/src/components/formDesigner/designerMainArea/index.tsx (2)
34-41: Effect deps: include structure if markup must stay in sync.If markup should update when
flatStructurechanges, add it to deps. Otherwise, confirm current behavior is intended.- },[formSettings, shaForm]); + }, [formSettings, shaForm, shaForm?.flatStructure]);
61-63: Plumbing looks good.Passing
renderSourceand enablingcanZoomaligns with the new canvas behavior.shesha-reactjs/src/components/sidebarContainer/index.tsx (1)
12-20: ConfirmsideBarWidthremoval is safe
No occurrences ofsideBarWidthwere found in the repo; please manually verify there’s no indirect usage before deleting.
shesha-reactjs/src/components/formDesigner/designerMainArea/index.tsx
Outdated
Show resolved
Hide resolved
IvanIlyichev
left a comment
IvanIlyichev
left a comment
There was a problem hiding this comment.
Hi @czwe-01. Please do the following
- Make sure that all siggestions from coderabbit are resolved
- Review usages of
window(anddocumentif any) all usages of browser-specific environment can be done conditionally only to prevent issues with SSR. After your changes build of admin portal fails
| overflow: auto; | ||
| margin: 0 auto; | ||
|
|
||
| position: absolute; |
There was a problem hiding this comment.
@czwe-01 please review styles and make sure that your changes don't breake anything. Absolute positioning should be used in specific cases only. This change (and maybe some other ones as well) breaks visibility of table on index pages
3 participants
Enhancements to Form Builder Canvas #3558
Summary by CodeRabbit
New Features
UI Improvements
API/Props