refactor(app): consolidate widget-editor-modal state into store (#573) - #584
Merged
alfredo1996 merged 6 commits intoApr 20, 2026
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 17 minutes and 22 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughConsolidates widget-editor-modal local state into Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Modal as WidgetEditorModal
participant Store as useWidgetEditorStore
participant Templates as TemplateBrowser
participant Preview as WidgetPreviewPanel
User->>Modal: open(editor mode)
Modal->>Store: loadFromWidget() / resetForAdd() / setLabMeta()
User->>Modal: open Templates step
Modal->>Templates: render(templates, loading, connectorType)
User->>Templates: select template
Templates->>Store: applyTemplate(template)
Store-->>Modal: update store fields (templateId, chart/query/title, connection)
Store->>Modal: set dialogStep("main")
User->>Preview: click Run
Preview->>Store: requestPreview(query, connectionId)
Store-->>Preview: previewQuery (pending → data / error)
Preview-->>User: render preview (cards/markdown/iframe/form)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/components/widget-editor-modal.tsx (1)
326-358:⚠️ Potential issue | 🔴 Critical🔴
setTemplateSearchis undefined — this will fail at runtime.Line 356 calls
setTemplateSearch(""), but there's no declaration or import for it anywhere in this file. TemplateBrowser manages its own search state internally (const [search, setSearch]at template-browser.tsx:33), so the setter was never passed to the modal. This is aReferenceErrorthe moment a user clicks a template card.Since TemplateBrowser unmounts when
dialogStep !== "templates", search clears automatically—just delete the orphaned line.Fix
} else if (t.connectorType) { // Fall back to first connection of matching type const match = connections.find((c) => c.type === t.connectorType); if (match) store.setConnectionId(match.id); } } - setTemplateSearch(""); store.setDialogStep("main"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor-modal.tsx` around lines 326 - 358, In applyTemplate (function applyTemplate) there's an invalid call to setTemplateSearch("") which is not defined in this file and causes a ReferenceError; remove the setTemplateSearch("") line from applyTemplate so the modal no longer references an external search setter (TemplateBrowser manages its own search and clears on unmount), and ensure no other references to setTemplateSearch remain in widget-editor-modal.tsx.
🧹 Nitpick comments (6)
app/src/components/widget-editor-modal.tsx (4)
204-271: Comment lies a little —refreshWidgetIdsis store-backed.The
// ── Local-only state (not in store) ────banner on line 204 is immediately contradicted by lines 268-271 pullingrefreshWidgetIds/setRefreshWidgetIdsfrom the store. Either move those selectors up next to the other store-backed block, or relabel the banner to "Derived / local-only and refs". Purely cosmetic, but future readers will trip on it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor-modal.tsx` around lines 204 - 271, The comment banner "Local-only state (not in store)" is misleading because refreshWidgetIds and setRefreshWidgetIds are pulled from the store; update the code so the banner accurately reflects contents: either move the useWidgetEditorStore selectors (refreshWidgetIds, setRefreshWidgetIds) up into the existing store-backed section where other selectors are used, or change the banner text to something like "Derived / local-only and refs" so it correctly groups refs and store-backed selectors (ensure you update the comment near editInitialChartTypeRef and the selectors for refreshWidgetIds/setRefreshWidgetIds).
174-202: LGTM — single initialization path is exactly what#573asked for.Clean consolidation:
loadFromWidgetfor edit, seeded reset for lab-edit, plainresetForAddfor add/lab-create. Thelab-createbranch's redundantsetLabName("")/etc. afterresetForAdd()is harmless but unnecessary sincegetInitialState()already seeds them to""(seewidget-editor-store.ts:250-252). Feel free to drop the branch entirely.♻️ Optional simplification
} else { // add or lab-create store.resetForAdd(); - if (mode === "lab-create") { - store.setLabName(""); - store.setLabDescription(""); - store.setLabTagsInput(""); - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor-modal.tsx` around lines 174 - 202, The lab-create branch after calling store.resetForAdd() redundantly calls store.setLabName(""), store.setLabDescription(""), and store.setLabTagsInput("") even though resetForAdd() already seeds those values from getInitialState(); remove the inner if (mode === "lab-create") block (or at minimum delete the three setLab... calls) inside the useEffect so resetForAdd() is the single initialization path for add/lab-create; reference useEffect in widget-editor-modal.tsx and the methods resetForAdd, setLabName, setLabDescription, setLabTagsInput and getInitialState in widget-editor-store.ts to locate the code.
1316-1322: Nit: inline object creates a newpreviewQueryreference every render.
WidgetPreviewPanelis not memoized today so there's no observable cost, but if you ever wrap it inReact.memothis will defeat it. Either passpreviewQuerythrough as-is (its shape already matches the prop — just widen the component's prop type) oruseMemothe literal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor-modal.tsx` around lines 1316 - 1322, The inline object passed as the previewQuery prop recreates a new reference each render and will break future memoization of WidgetPreviewPanel; instead, pass the existing previewQuery object directly (adjust WidgetPreviewPanel prop type to accept the wider shape) or wrap the constructed object in useMemo to stabilize its identity. Locate where previewQuery is passed into WidgetPreviewPanel in widget-editor-modal (the previewQuery prop and initialPreviewData usage), and either change the prop to previewQuery={previewQuery} or memoize the object with useMemo surrounding the literal so its reference is stable across renders.
397-412:handleChartTypeChangehas incomplete dependencies for recommended consistency.The useCallback references
setClickActionEnabledandsetStylingEnabledbut doesn't list them in the deps array. While Zustand setters are referentially stable and safe,react-hooks/exhaustive-depswill flag this on the next lint run. For consistency with other similar functions in this file (lines 424, 444, 462), either add both setters to the deps array or add aneslint-disable-next-line react-hooks/exhaustive-depscomment with a rationale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor-modal.tsx` around lines 397 - 412, The handleChartTypeChange useCallback references setClickActionEnabled and setStylingEnabled but they are missing from the dependency array; update the callback for consistency by adding setClickActionEnabled and setStylingEnabled to the deps array of handleChartTypeChange (alongside setChartType), or if you intentionally rely on stable setters, add an eslint-disable-next-line react-hooks/exhaustive-deps with a short rationale; locate handleChartTypeChange and adjust its dependency list accordingly.app/src/components/widget-editor/widget-preview-panel.tsx (1)
64-91: Prop surface is wide — consider grouping when you extend this next time.22 props is a lot to thread through on every render. Not a blocker for this PR (it's mirroring the modal's state shape), but when you add the next flag, a small
flags/paramState/previewStategrouping would make this much easier to memoize and test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor/widget-preview-panel.tsx` around lines 64 - 91, The WidgetPreviewPanel props list is too large (22 props) which makes extending and memoizing hard; refactor the component signature (WidgetPreviewPanel) to group related props into cohesive objects (e.g., flags: { isParamSelect, isForm, isContentOnly, isMarkdown, isIframe }, previewState: { seedPreviewOptions, seedQueryPending, seedQueryError, previewQuery, initialPreviewData }, and paramState: { paramUIType, multiSelect, paramWidgetName, formFields }) and accept those grouped objects instead of individual primitives, then update all usages/call sites to pass the new grouped objects and adjust internal accesses to use the new property paths; this keeps external API backward-compatible by optionally accepting the old props temporarily or by updating all imports in the codebase to the new signature and will make memoization and testing easier.app/src/components/widget-editor/template-browser.tsx (1)
82-86: Addtype="button"to defensively guard against future form nesting.Currently the dialog is not a
<form>, so the default"submit"is harmless. If this ever ends up inside one (easy to miss when composing dialogs), clicking a card would submit it. One-character fix, zero risk.♻️ Proposed fix
{filtered.map((t) => ( <button key={t.id} + type="button" onClick={() => onApply(t)} className="text-left rounded-lg border p-2 hover:bg-accent transition-colors flex flex-col gap-1.5" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor/template-browser.tsx` around lines 82 - 86, The button rendered for each template card (key={t.id}, onClick={() => onApply(t)}) lacks an explicit type and will default to "submit" if this component is ever rendered inside a form; add type="button" to that <button> element in template-browser (the template card button using onApply and t.id) to defensively prevent accidental form submissions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/widget-editor/template-browser.tsx`:
- Around line 42-49: The search input is hidden when loading is true because the
JSX is gated by "!loading && templates && templates.length > 0"; instead render
the <Input> regardless of loading so the user's typed search isn't lost during
refetch. Update template-browser.tsx to move the Input out of the "!loading"
guard (keep using value={search} and onChange={e => setSearch(e.target.value)}),
and only use the loading flag to control loader/empty-state UI for the templates
list (e.g., show spinner or "no results" messages when loading is true or
templates is empty) so the Input remains visible while templates are refetching.
---
Outside diff comments:
In `@app/src/components/widget-editor-modal.tsx`:
- Around line 326-358: In applyTemplate (function applyTemplate) there's an
invalid call to setTemplateSearch("") which is not defined in this file and
causes a ReferenceError; remove the setTemplateSearch("") line from
applyTemplate so the modal no longer references an external search setter
(TemplateBrowser manages its own search and clears on unmount), and ensure no
other references to setTemplateSearch remain in widget-editor-modal.tsx.
---
Nitpick comments:
In `@app/src/components/widget-editor-modal.tsx`:
- Around line 204-271: The comment banner "Local-only state (not in store)" is
misleading because refreshWidgetIds and setRefreshWidgetIds are pulled from the
store; update the code so the banner accurately reflects contents: either move
the useWidgetEditorStore selectors (refreshWidgetIds, setRefreshWidgetIds) up
into the existing store-backed section where other selectors are used, or change
the banner text to something like "Derived / local-only and refs" so it
correctly groups refs and store-backed selectors (ensure you update the comment
near editInitialChartTypeRef and the selectors for
refreshWidgetIds/setRefreshWidgetIds).
- Around line 174-202: The lab-create branch after calling store.resetForAdd()
redundantly calls store.setLabName(""), store.setLabDescription(""), and
store.setLabTagsInput("") even though resetForAdd() already seeds those values
from getInitialState(); remove the inner if (mode === "lab-create") block (or at
minimum delete the three setLab... calls) inside the useEffect so resetForAdd()
is the single initialization path for add/lab-create; reference useEffect in
widget-editor-modal.tsx and the methods resetForAdd, setLabName,
setLabDescription, setLabTagsInput and getInitialState in widget-editor-store.ts
to locate the code.
- Around line 1316-1322: The inline object passed as the previewQuery prop
recreates a new reference each render and will break future memoization of
WidgetPreviewPanel; instead, pass the existing previewQuery object directly
(adjust WidgetPreviewPanel prop type to accept the wider shape) or wrap the
constructed object in useMemo to stabilize its identity. Locate where
previewQuery is passed into WidgetPreviewPanel in widget-editor-modal (the
previewQuery prop and initialPreviewData usage), and either change the prop to
previewQuery={previewQuery} or memoize the object with useMemo surrounding the
literal so its reference is stable across renders.
- Around line 397-412: The handleChartTypeChange useCallback references
setClickActionEnabled and setStylingEnabled but they are missing from the
dependency array; update the callback for consistency by adding
setClickActionEnabled and setStylingEnabled to the deps array of
handleChartTypeChange (alongside setChartType), or if you intentionally rely on
stable setters, add an eslint-disable-next-line react-hooks/exhaustive-deps with
a short rationale; locate handleChartTypeChange and adjust its dependency list
accordingly.
In `@app/src/components/widget-editor/template-browser.tsx`:
- Around line 82-86: The button rendered for each template card (key={t.id},
onClick={() => onApply(t)}) lacks an explicit type and will default to "submit"
if this component is ever rendered inside a form; add type="button" to that
<button> element in template-browser (the template card button using onApply and
t.id) to defensively prevent accidental form submissions.
In `@app/src/components/widget-editor/widget-preview-panel.tsx`:
- Around line 64-91: The WidgetPreviewPanel props list is too large (22 props)
which makes extending and memoizing hard; refactor the component signature
(WidgetPreviewPanel) to group related props into cohesive objects (e.g., flags:
{ isParamSelect, isForm, isContentOnly, isMarkdown, isIframe }, previewState: {
seedPreviewOptions, seedQueryPending, seedQueryError, previewQuery,
initialPreviewData }, and paramState: { paramUIType, multiSelect,
paramWidgetName, formFields }) and accept those grouped objects instead of
individual primitives, then update all usages/call sites to pass the new grouped
objects and adjust internal accesses to use the new property paths; this keeps
external API backward-compatible by optionally accepting the old props
temporarily or by updating all imports in the codebase to the new signature and
will make memoization and testing easier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b31a4404-4137-4e13-aadc-6dd6ab25dd29
📒 Files selected for processing (3)
app/src/components/widget-editor-modal.tsxapp/src/components/widget-editor/template-browser.tsxapp/src/components/widget-editor/widget-preview-panel.tsx
Comment on lines
+42
to
+49
| {!loading && templates && templates.length > 0 && ( | ||
| <Input | ||
| placeholder="Search by name..." | ||
| value={search} | ||
| onChange={(e) => setSearch(e.target.value)} | ||
| className="mb-3 max-w-xs" | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Search input disappears while re-fetching even when results existed.
Since the filter guard is !loading && templates && templates.length > 0, any refetch (e.g., re-opening after a connector change) flips loading back to true and blows away the user's typed search. Probably low-impact for this UI, but consider gating the loader/empty-states instead of the input.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/widget-editor/template-browser.tsx` around lines 42 - 49,
The search input is hidden when loading is true because the JSX is gated by
"!loading && templates && templates.length > 0"; instead render the <Input>
regardless of loading so the user's typed search isn't lost during refetch.
Update template-browser.tsx to move the Input out of the "!loading" guard (keep
using value={search} and onChange={e => setSearch(e.target.value)}), and only
use the loading flag to control loader/empty-state UI for the templates list
(e.g., show spinner or "no results" messages when loading is true or templates
is empty) so the Input remains visible while templates are refetching.
alfredo1996
force-pushed
the
refactor/issue-573-widget-editor-consolidation
branch
from
April 20, 2026 10:30
6475e81 to
4f660bf
Compare
Replaces 18 duplicated useState hooks with Zustand store selectors, removes the redundant 160-line initialization effect, and delegates buildClickAction/buildStylingConfig to the store. 1842 → 1616 lines (-226 lines, 12% reduction). Eliminates the class of bugs where local state and store state drift out of sync. Closes #573 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Moves the template browser (~100 lines) and preview panel (~160 lines) into dedicated sub-components under widget-editor/, reducing the modal from 1616 to ~1390 lines. Cleans up unused imports. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
force-pushed
the
refactor/issue-573-widget-editor-consolidation
branch
from
April 20, 2026 13:01
4f660bf to
18396ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/src/components/widget-editor/template-browser.tsx (1)
42-49:⚠️ Potential issue | 🟡 MinorKeep the search input mounted during refetch.
Line 42 hides the input whenever
loadingflips true, so template refetches make the search control disappear. Move the<Input>outside the loading/results guard and only gate the list/empty states.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor/template-browser.tsx` around lines 42 - 49, The search input is currently conditional on {!loading && templates && templates.length > 0}, causing it to unmount during refetch; move the <Input> so it is always rendered (i.e., outside that guard) and only keep the templates list/empty-state behind the loading/results condition; update the JSX around Input (references: Input component, search state and setSearch handler, and templates/loading checks) so the input remains mounted during loading/refetch while the results area still reflects loading/empty states.
🧹 Nitpick comments (1)
app/src/components/widget-editor-modal.tsx (1)
102-1382: Finish or rescope the modal decomposition objective.This modal is still 1,382 lines, so the “files under 500 lines” acceptance criterion is not met yet despite the new extracted panels. Either continue extracting the remaining editor sections or update the issue scope before closing
#573.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor-modal.tsx` around lines 102 - 1382, WidgetEditorModal is still ~1,382 lines and needs further decomposition per `#573`; split the remaining large responsibilities into focused sub-files or shrink the scope before closing the ticket. Specifically: extract modal initialization and store wiring (the useEffect blocks that load/reset the store and the chartType reset) into a custom hook (e.g., useWidgetEditorInitialization); move preview/autopreview/previewQuery logic and handlePreview into useWidgetAutoPreview; move keyboard shortcut and save-status logic including handleRunAndSave/handleSave/handleLabSave and savedTimerRef into useWidgetSaveHandlers; extract large JSX chunks (lab metadata inputs, the left-column content inside ChartSettingsPanel props/dataTab/styleTab/advancedTab and the DialogFooter buttons) into presentational components (e.g., LabMetadata, MainSettings, AdvancedSettings, FooterActions) that receive props and call the above handlers; keep applyTemplate, handleConnectionChange, buildClickAction/buildStylingConfig as small exported helpers or pass them as props to the new components so WidgetEditorModal becomes a coordinator under ~500 lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/widget-editor-modal.tsx`:
- Around line 179-192: Replace the manual per-field hydration in the lab-edit
branch (the block using store.resetForAdd(), store.setChartType,
store.setChartOptions, etc.) with the store's full template/widget hydration
routine used by edit mode (call the same method the edit path uses to fully
hydrate stylingConfig, clickAction, transforms, conditionalFormatting, etc. from
templateProp), and ensure you apply the same chart-type guard used in edit mode
so the initial lab-edit chartType isn't overwritten by the chart-type reset. Use
the existing identifiers (mode, templateProp, store.resetForAdd,
store.setChartType, store.setChartOptions) to locate the code and swap to the
edit-mode hydration function and chart-type guard.
- Line 25: Restore the missing FlaskConical import by adding it to the existing
lucide-react import where AlertTriangle and Info are imported (i.e., update the
import statement that currently reads import { AlertTriangle, Info } from
"lucide-react"; to include FlaskConical) so the JSX usage of <FlaskConical />
type-checks and renders correctly.
- Around line 356-357: The call to setTemplateSearch("") is stale and causes a
compile error because setTemplateSearch was removed when search state moved into
TemplateBrowser; remove the setTemplateSearch("") invocation from the
widget-editor-modal component (leave store.setDialogStep("main") as-is) or
replace it with a call that updates the TemplateBrowser's search state if such
an API exists (e.g., a prop or store method); locate usages around the dialog
step transition where setTemplateSearch is referenced and delete or redirect
them to the new TemplateBrowser search mechanism.
---
Duplicate comments:
In `@app/src/components/widget-editor/template-browser.tsx`:
- Around line 42-49: The search input is currently conditional on {!loading &&
templates && templates.length > 0}, causing it to unmount during refetch; move
the <Input> so it is always rendered (i.e., outside that guard) and only keep
the templates list/empty-state behind the loading/results condition; update the
JSX around Input (references: Input component, search state and setSearch
handler, and templates/loading checks) so the input remains mounted during
loading/refetch while the results area still reflects loading/empty states.
---
Nitpick comments:
In `@app/src/components/widget-editor-modal.tsx`:
- Around line 102-1382: WidgetEditorModal is still ~1,382 lines and needs
further decomposition per `#573`; split the remaining large responsibilities into
focused sub-files or shrink the scope before closing the ticket. Specifically:
extract modal initialization and store wiring (the useEffect blocks that
load/reset the store and the chartType reset) into a custom hook (e.g.,
useWidgetEditorInitialization); move preview/autopreview/previewQuery logic and
handlePreview into useWidgetAutoPreview; move keyboard shortcut and save-status
logic including handleRunAndSave/handleSave/handleLabSave and savedTimerRef into
useWidgetSaveHandlers; extract large JSX chunks (lab metadata inputs, the
left-column content inside ChartSettingsPanel props/dataTab/styleTab/advancedTab
and the DialogFooter buttons) into presentational components (e.g., LabMetadata,
MainSettings, AdvancedSettings, FooterActions) that receive props and call the
above handlers; keep applyTemplate, handleConnectionChange,
buildClickAction/buildStylingConfig as small exported helpers or pass them as
props to the new components so WidgetEditorModal becomes a coordinator under
~500 lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 276e5478-ae1e-4f7d-bb89-63c7b1a76708
📒 Files selected for processing (3)
app/src/components/widget-editor-modal.tsxapp/src/components/widget-editor/template-browser.tsxapp/src/components/widget-editor/widget-preview-panel.tsx
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/src/components/widget-editor-modal.tsx (2)
174-202:⚠️ Potential issue | 🟠 MajorLab-edit init: partial template hydration + chartOptions gets clobbered.
Two related problems in this branch:
- Only
chartType,connectionId,query,title,chartOptions, and lab metadata are restored. Templates persiststylingConfig,clickAction,transforms, andconditionalFormattingtoo (seeloadFromWidgetbehavior inwidget-editor-store.ts), so opening an existing template in the lab and saving it will silently drop those settings.- Unlike the
editbranch,lab-editdoes not seteditInitialChartTypeRef.current. ThesetChartType(templateProp.chartType)call here will trigger the chart-type reset effect at lines 450–462, which then overwrites thechartOptionsyou just applied withgetDefaultChartSettings(...). Either seteditInitialChartTypeRef.current = templateProp.chartTypebeforesetChartType, or setapplyingTemplateRef.current = true(same guardapplyTemplateuses).Best fix: add a
loadFromTemplatehelper to the store that mirrorsloadFromWidgetfor the template shape, and call it here and fromapplyTemplate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor-modal.tsx` around lines 174 - 202, The lab-edit branch only partially hydrates templates and then clobbers chartOptions via the chart-type reset; add a loadFromTemplate(template) method to the widget editor store (mirroring loadFromWidget) that sets stylingConfig, clickAction, transforms, conditionalFormatting, chartType, connectionId, query, title, chartOptions, and lab metadata; replace the inline template population in widget-editor-modal's useEffect and in applyTemplate to call store.loadFromTemplate(templateProp); additionally, ensure you prevent the chart-type reset from overwriting chartOptions by setting editInitialChartTypeRef.current = templateProp.chartType (or setting applyingTemplateRef.current = true) before calling store.setChartType(templateProp.chartType) so chartOptions are preserved.
326-357:⚠️ Potential issue | 🟠 Major
applyTemplateonly restores a subset of template settings.Same hydration gap as
lab-edit:stylingConfig,clickAction,transforms, andconditionalFormattingont.settingsare ignored. If a user picks a template that relies on any of those, the applied widget will be missing them. Route this through a shared store hydration method so all template-derived fields are written in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor-modal.tsx` around lines 326 - 357, The applyTemplate function only restores chartType, query, title, chartOptions and connection info but ignores stylingConfig, clickAction, transforms, and conditionalFormatting; update applyTemplate to delegate full hydration to a single store method (e.g., useWidgetEditorStore.getState().hydrateFromTemplate or setFromTemplate) that writes templateId/templateSyncedAt plus all fields from t.settings (title, chartOptions, stylingConfig, clickAction, transforms, conditionalFormatting) and preserves existing fallback/default logic for chart options and connection selection; if such a hydrate method does not exist, implement it on the widget editor store (name it clearly like hydrateFromTemplate or setTemplateFromTemplate) and call that from applyTemplate so all template-derived fields are written in one place.
🧹 Nitpick comments (1)
app/src/components/widget-editor-modal.tsx (1)
413-424: Double-effect onopen: consider merging.This effect and the init effect at lines 174–202 share the same trigger set (
open,mode,widget,templateProp). Splitting them means the chart-type guard for edit mode lives away from the init that actually sets the chart type, which is exactly why the lab-edit gap above is easy to miss. Folding theeditInitialChartTypeRefassignment and preview resets into the init effect (and setting the ref forlab-edit+applyTemplate-initialized paths too) would make the invariant hard to break.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor-modal.tsx` around lines 413 - 424, The second useEffect that resets previewQuery/seedQueryExecution and sets editInitialChartTypeRef when open is redundant and can desynchronize invariants; merge its logic into the existing initialization effect that handles field initialization (the init useEffect that runs on [open, mode, widget, templateProp]) so that editInitialChartTypeRef.current is set there for mode === "edit" (and also set for the lab-edit + applyTemplate initialization paths), and call seedQueryExecution.reset() and previewQuery.reset() from that same init effect instead of a separate useEffect; update references to useEffect, editInitialChartTypeRef, seedQueryExecution.reset, and previewQuery.reset accordingly and remove the duplicate effect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/components/widget-editor-modal.tsx`:
- Around line 174-202: The lab-edit branch only partially hydrates templates and
then clobbers chartOptions via the chart-type reset; add a
loadFromTemplate(template) method to the widget editor store (mirroring
loadFromWidget) that sets stylingConfig, clickAction, transforms,
conditionalFormatting, chartType, connectionId, query, title, chartOptions, and
lab metadata; replace the inline template population in widget-editor-modal's
useEffect and in applyTemplate to call store.loadFromTemplate(templateProp);
additionally, ensure you prevent the chart-type reset from overwriting
chartOptions by setting editInitialChartTypeRef.current = templateProp.chartType
(or setting applyingTemplateRef.current = true) before calling
store.setChartType(templateProp.chartType) so chartOptions are preserved.
- Around line 326-357: The applyTemplate function only restores chartType,
query, title, chartOptions and connection info but ignores stylingConfig,
clickAction, transforms, and conditionalFormatting; update applyTemplate to
delegate full hydration to a single store method (e.g.,
useWidgetEditorStore.getState().hydrateFromTemplate or setFromTemplate) that
writes templateId/templateSyncedAt plus all fields from t.settings (title,
chartOptions, stylingConfig, clickAction, transforms, conditionalFormatting) and
preserves existing fallback/default logic for chart options and connection
selection; if such a hydrate method does not exist, implement it on the widget
editor store (name it clearly like hydrateFromTemplate or
setTemplateFromTemplate) and call that from applyTemplate so all
template-derived fields are written in one place.
---
Nitpick comments:
In `@app/src/components/widget-editor-modal.tsx`:
- Around line 413-424: The second useEffect that resets
previewQuery/seedQueryExecution and sets editInitialChartTypeRef when open is
redundant and can desynchronize invariants; merge its logic into the existing
initialization effect that handles field initialization (the init useEffect that
runs on [open, mode, widget, templateProp]) so that
editInitialChartTypeRef.current is set there for mode === "edit" (and also set
for the lab-edit + applyTemplate initialization paths), and call
seedQueryExecution.reset() and previewQuery.reset() from that same init effect
instead of a separate useEffect; update references to useEffect,
editInitialChartTypeRef, seedQueryExecution.reset, and previewQuery.reset
accordingly and remove the duplicate effect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fa354b8b-61b7-456a-b61c-33bbbeddc126
📒 Files selected for processing (1)
app/src/components/widget-editor-modal.tsx
The store defaults chartType to "table", but the old modal's init effect (removed in the consolidation) explicitly set "bar" for add mode. E2E tests rely on bar as the default. Restored via explicit setChartType after resetForAdd. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds 16 unit tests covering loading, empty, render, search, filter, click handlers, and all render branches (markdown, iframe, param-select, form, CardContainer, error state). Closes coverage gap on extracted sub-components for #573. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
This was referenced Apr 22, 2026
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
useStatehooks with Zustand store selectorsuseEffect(store'sloadFromWidget/resetForAddalready handles this)buildClickActionandbuildStylingConfigto the store instead of local duplicates1842 → 1616 lines (-226 lines, 12% reduction)
Why
The modal had ~18 local state fields that duplicated fields already in
widget-editor-store.ts. Two initialization effects fired on open, with the longer one overriding the store-backed approach.buildClickActionwas duplicated between both files with ~50 lines of identical validation logic.This eliminates the class of bugs where local state and store state drift out of sync, and makes the component more testable.
What stays local
templateSearch,saveStatus,labError— these are UI-only state not needed by sub-editors.Test plan
Closes #573
🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Refactor