refactor(app): decompose widget-editor-modal into sub-components (#573) - #627
Merged
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (17)
WalkthroughRefactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
🚥 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. Review rate limit: 0/1 reviews remaining, refill in 37 minutes and 27 seconds.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: 6
🧹 Nitpick comments (3)
app/src/components/widget-editor/__tests__/lab-metadata-form.test.tsx (1)
64-67: Assert semantic required behavior, not just the asterisk.Consider validating the input has
requiredto lock in accessibility semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor/__tests__/lab-metadata-form.test.tsx` around lines 64 - 67, The test currently asserts only that an asterisk is rendered; instead assert the input has the semantic required attribute: render LabMetadataForm, query the "template name" input (e.g. via screen.getByLabelText('Template name') or screen.getByRole('textbox', { name: /template name/i }) to target the template name field in LabMetadataForm) and expect that element.toHaveAttribute('required') or element.required to be true; replace or augment the getByText("*") assertion with this required-attribute check to ensure accessibility semantics.app/src/components/widget-editor/lab-metadata-form.tsx (1)
17-25: Expose required semantics on Template Name input.The field is visually required but not semantically marked required.
💡 Suggested fix
<Input id="lab-template-name" value={labName} onChange={(e) => setLabName(e.target.value)} placeholder="My chart template" + required + aria-required="true" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor/lab-metadata-form.tsx` around lines 17 - 25, The Template Name input is only visually marked required but lacks semantic required attributes; update the Input usage in lab-metadata-form (the Input tied to labName / setLabName) to include required and aria-required="true" (or the component's equivalent prop) so the field is programmatically exposed as required to assistive tech and HTML validation.app/src/components/widget-editor/__tests__/advanced-caching-section.test.tsx (1)
94-121: Consider adding edge-case tests for TTL parser guardrails.Add cases for
>1440clamping and invalid numeric intermediates so this doesn’t regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor/__tests__/advanced-caching-section.test.tsx` around lines 94 - 121, Add tests in advanced-caching-section.test.tsx that exercise AdvancedCachingSection’s TTL parser guardrails: render AdvancedCachingSection with mockEnableCache = true and fireEvent.change on the "cache-ttl" input with a value >1440 (e.g., "2000") and assert mockSetCacheTtlMinutes was called with 1440, and add another test that sends invalid/intermediate numeric inputs (e.g., "", "abc", "1.5") to the same "cache-ttl" input and assert mockSetCacheTtlMinutes is clamped/normalized to the minimum (1) or expected safe value; reference the AdvancedCachingSection component, the "cache-ttl" test id, and mockSetCacheTtlMinutes when adding these tests.
🤖 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/advanced-caching-section.tsx`:
- Around line 38-40: The onChange handler for the TTL uses
Number(e.target.value) and Math.max but can save NaN and doesn't enforce the
upper bound; update the handler that calls setCacheTtlMinutes to parse the input
(e.g., parseInt/parseFloat), check isNaN and default to 1 if invalid, then clamp
the value between 1 and 1440 using Math.max/Math.min before passing to
setCacheTtlMinutes (refer to the onChange arrow function and the
setCacheTtlMinutes call).
In `@app/src/components/widget-editor/advanced-form-refresh-section.tsx`:
- Around line 14-17: The UI currently uses refreshWidgetIds.length and raw
includes() which counts/marks IDs that might not be visible; instead derive
selection state from the visible widget list (otherWidgets) before rendering and
when computing bulk/row state. Create a visibleSelectedIds/visibleSelectedSet by
filtering refreshWidgetIds to only IDs present in otherWidgets (e.g., const
visibleSelectedIds = refreshWidgetIds.filter(id => otherWidgets.some(w => w.id
=== id)); const visibleSelectedSet = new Set(visibleSelectedIds)); use
visibleSelectedIds.length for the "n of m selected" display and
visibleSelectedSet.has(id) for per-row checked state; update bulk toggle
handlers to add/remove only the visible widget ids to/from the global
refreshWidgetIds via setRefreshWidgetIds(prev => { const s = new Set(prev); if
(selecting) visibleIds.forEach(id => s.add(id)); else visibleIds.forEach(id =>
s.delete(id)); return Array.from(s); }) so store still holds other selections
but UI reflects only visible ones.
In `@app/src/components/widget-editor/modal-footer.tsx`:
- Around line 41-43: The lab error banner (labError) is being rendered
regardless of the current editor mode and can leak stale errors into "add" or
"edit" sessions; update the rendering in modal-footer.tsx (ModalFooter/its JSX)
to only show the paragraph when labError is set AND mode === "lab" (i.e., change
the condition from just labError to labError && mode === "lab"), ensuring the
component uses the passed-in mode prop so lab-only errors don't appear in other
modes.
- Around line 47-81: The save validation is inconsistent between the lab and
normal branches causing param-select templates to be unsavable in lab mode and
allowing empty connectionId in normal mode; extract the predicate into a single
helper (e.g., canSaveWidget or validateSaveGuard) that encapsulates the rules:
if isParamSelect then require paramWidgetName.trim() and if paramUIType ===
"select" require connectionId && String(chartOptions.seedQuery ?? "").trim();
else if isContentOnly allow save; else (non-content) require connectionId &&
query.trim(); then replace the duplicated disabled expressions in both
LoadingButton branches to call this helper and ensure onLabSave/onSave (and
buildWidgetForSave) are only invoked when the helper returns true. Use the
existing symbols isLabMode, isParamSelect, paramWidgetName, paramUIType,
connectionId, chartOptions.seedQuery, isContentOnly, query, onLabSave, onSave,
saveStatus and buildWidgetForSave to locate and update the logic.
In `@app/src/components/widget-editor/use-auto-preview.ts`:
- Around line 181-186: When the modal closes in the useEffect watching open,
also reset the save status to avoid a stale "Saved!" message: after clearing
savedTimerRef (in the useEffect that checks if (!open && savedTimerRef.current
!== null)), call setSaveStatus('idle') (or the hook's neutral state value) so
saveStatus is reset; reference the useAutoPreview hook's savedTimerRef,
saveStatus, setSaveStatus and the open variable when making this change.
- Around line 130-136: The run+save path in handleRunAndSave currently calls
previewQueryRef.current.mutate without sending the extracted query parameters,
so parameterized queries fail; update handleRunAndSave to include the same
extracted params payload that handlePreview sends (pass the params/parsedParams
or the same param object used by handlePreview) when calling
previewQueryRef.current.mutate and any subsequent save/mutate logic so
parameterized queries receive their params.
---
Nitpick comments:
In
`@app/src/components/widget-editor/__tests__/advanced-caching-section.test.tsx`:
- Around line 94-121: Add tests in advanced-caching-section.test.tsx that
exercise AdvancedCachingSection’s TTL parser guardrails: render
AdvancedCachingSection with mockEnableCache = true and fireEvent.change on the
"cache-ttl" input with a value >1440 (e.g., "2000") and assert
mockSetCacheTtlMinutes was called with 1440, and add another test that sends
invalid/intermediate numeric inputs (e.g., "", "abc", "1.5") to the same
"cache-ttl" input and assert mockSetCacheTtlMinutes is clamped/normalized to the
minimum (1) or expected safe value; reference the AdvancedCachingSection
component, the "cache-ttl" test id, and mockSetCacheTtlMinutes when adding these
tests.
In `@app/src/components/widget-editor/__tests__/lab-metadata-form.test.tsx`:
- Around line 64-67: The test currently asserts only that an asterisk is
rendered; instead assert the input has the semantic required attribute: render
LabMetadataForm, query the "template name" input (e.g. via
screen.getByLabelText('Template name') or screen.getByRole('textbox', { name:
/template name/i }) to target the template name field in LabMetadataForm) and
expect that element.toHaveAttribute('required') or element.required to be true;
replace or augment the getByText("*") assertion with this required-attribute
check to ensure accessibility semantics.
In `@app/src/components/widget-editor/lab-metadata-form.tsx`:
- Around line 17-25: The Template Name input is only visually marked required
but lacks semantic required attributes; update the Input usage in
lab-metadata-form (the Input tied to labName / setLabName) to include required
and aria-required="true" (or the component's equivalent prop) so the field is
programmatically exposed as required to assistive tech and HTML validation.
🪄 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: 86a5241f-ea07-47e0-a5f0-b67d484b8aaa
📒 Files selected for processing (15)
app/src/components/widget-editor-modal.tsxapp/src/components/widget-editor/__tests__/advanced-caching-section.test.tsxapp/src/components/widget-editor/__tests__/advanced-form-refresh-section.test.tsxapp/src/components/widget-editor/__tests__/advanced-interactivity-section.test.tsxapp/src/components/widget-editor/__tests__/advanced-styling-section.test.tsxapp/src/components/widget-editor/__tests__/lab-metadata-form.test.tsxapp/src/components/widget-editor/__tests__/modal-footer.test.tsxapp/src/components/widget-editor/advanced-caching-section.tsxapp/src/components/widget-editor/advanced-form-refresh-section.tsxapp/src/components/widget-editor/advanced-interactivity-section.tsxapp/src/components/widget-editor/advanced-styling-section.tsxapp/src/components/widget-editor/lab-metadata-form.tsxapp/src/components/widget-editor/modal-footer.tsxapp/src/components/widget-editor/use-auto-preview.tsapp/src/components/widget-editor/use-widget-save.ts
Comment on lines
+38
to
+40
| onChange={(e) => | ||
| setCacheTtlMinutes(Math.max(1, Number(e.target.value))) | ||
| } |
There was a problem hiding this comment.
TTL normalization allows invalid values (NaN) and ignores max bound.
Current conversion can store NaN, and values above 1440 are not clamped before writing to state.
💡 Suggested fix
<Input
id="cache-ttl"
type="number"
min={1}
max={1440}
value={cacheTtlMinutes}
- onChange={(e) =>
- setCacheTtlMinutes(Math.max(1, Number(e.target.value)))
- }
+ onChange={(e) => {
+ const parsed = Number.parseInt(e.target.value, 10);
+ if (Number.isNaN(parsed)) return;
+ setCacheTtlMinutes(Math.min(1440, Math.max(1, parsed)));
+ }}
className="w-24"
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onChange={(e) => | |
| setCacheTtlMinutes(Math.max(1, Number(e.target.value))) | |
| } | |
| onChange={(e) => { | |
| const parsed = Number.parseInt(e.target.value, 10); | |
| if (Number.isNaN(parsed)) return; | |
| setCacheTtlMinutes(Math.min(1440, Math.max(1, parsed))); | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/widget-editor/advanced-caching-section.tsx` around lines
38 - 40, The onChange handler for the TTL uses Number(e.target.value) and
Math.max but can save NaN and doesn't enforce the upper bound; update the
handler that calls setCacheTtlMinutes to parse the input (e.g.,
parseInt/parseFloat), check isNaN and default to 1 if invalid, then clamp the
value between 1 and 1440 using Math.max/Math.min before passing to
setCacheTtlMinutes (refer to the onChange arrow function and the
setCacheTtlMinutes call).
Comment on lines
+14
to
+17
| const refreshWidgetIds = useWidgetEditorStore((s) => s.refreshWidgetIds); | ||
| const setRefreshWidgetIds = useWidgetEditorStore( | ||
| (s) => s.setRefreshWidgetIds, | ||
| ); |
There was a problem hiding this comment.
Derive selection state from the visible widget list.
Lines 31-49 use refreshWidgetIds.length and raw includes() checks directly. If the store still contains ids for widgets that were deleted or moved out of otherWidgets, the UI can show n of m selected while no visible row is checked, and the bulk-action label becomes inconsistent.
Proposed fix
const refreshWidgetIds = useWidgetEditorStore((s) => s.refreshWidgetIds);
const setRefreshWidgetIds = useWidgetEditorStore(
(s) => s.setRefreshWidgetIds,
);
+ const visibleSelectedWidgetIds = otherWidgets
+ .map((w) => w.id)
+ .filter((id) => refreshWidgetIds.includes(id));
+ const allVisibleSelected =
+ otherWidgets.length > 0 &&
+ visibleSelectedWidgetIds.length === otherWidgets.length;
return (
@@
<span className="text-xs text-muted-foreground">
- {refreshWidgetIds.length} of {otherWidgets.length} selected
+ {visibleSelectedWidgetIds.length} of {otherWidgets.length} selected
</span>
<Button
@@
className="h-6 text-xs px-2"
onClick={() => {
- const allSelected = otherWidgets.every((w) =>
- refreshWidgetIds.includes(w.id),
- );
setRefreshWidgetIds(
- allSelected ? [] : otherWidgets.map((w) => w.id),
+ allVisibleSelected ? [] : otherWidgets.map((w) => w.id),
);
}}
>
- {otherWidgets.every((w) => refreshWidgetIds.includes(w.id))
+ {allVisibleSelected
? "Deselect all"
: "Select all"}
</Button>
@@
<Checkbox
id={`refresh-widget-${w.id}`}
- checked={refreshWidgetIds.includes(w.id)}
+ checked={visibleSelectedWidgetIds.includes(w.id)}
onCheckedChange={(checked) => {
if (checked) {
- setRefreshWidgetIds([...refreshWidgetIds, w.id]);
+ setRefreshWidgetIds(
+ Array.from(new Set([...visibleSelectedWidgetIds, w.id])),
+ );
} else {
setRefreshWidgetIds(
- refreshWidgetIds.filter((id: string) => id !== w.id),
+ visibleSelectedWidgetIds.filter((id) => id !== w.id),
);
}
}}
/>Also applies to: 27-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/widget-editor/advanced-form-refresh-section.tsx` around
lines 14 - 17, The UI currently uses refreshWidgetIds.length and raw includes()
which counts/marks IDs that might not be visible; instead derive selection state
from the visible widget list (otherWidgets) before rendering and when computing
bulk/row state. Create a visibleSelectedIds/visibleSelectedSet by filtering
refreshWidgetIds to only IDs present in otherWidgets (e.g., const
visibleSelectedIds = refreshWidgetIds.filter(id => otherWidgets.some(w => w.id
=== id)); const visibleSelectedSet = new Set(visibleSelectedIds)); use
visibleSelectedIds.length for the "n of m selected" display and
visibleSelectedSet.has(id) for per-row checked state; update bulk toggle
handlers to add/remove only the visible widget ids to/from the global
refreshWidgetIds via setRefreshWidgetIds(prev => { const s = new Set(prev); if
(selecting) visibleIds.forEach(id => s.add(id)); else visibleIds.forEach(id =>
s.delete(id)); return Array.from(s); }) so store still holds other selections
but UI reflects only visible ones.
Comment on lines
+41
to
+43
| {labError && ( | ||
| <p className="text-sm text-destructive mr-auto">{labError}</p> | ||
| )} |
There was a problem hiding this comment.
Only show labError in lab mode.
Lines 41-43 render the template-save error banner even when mode is "add" or "edit". Since the parent keeps labError in component state, a failed lab save can leak a stale error into a later widget-edit session.
Proposed fix
- {labError && (
+ {isLabMode && labError && (
<p className="text-sm text-destructive mr-auto">{labError}</p>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {labError && ( | |
| <p className="text-sm text-destructive mr-auto">{labError}</p> | |
| )} | |
| {isLabMode && labError && ( | |
| <p className="text-sm text-destructive mr-auto">{labError}</p> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/widget-editor/modal-footer.tsx` around lines 41 - 43, The
lab error banner (labError) is being rendered regardless of the current editor
mode and can leak stale errors into "add" or "edit" sessions; update the
rendering in modal-footer.tsx (ModalFooter/its JSX) to only show the paragraph
when labError is set AND mode === "lab" (i.e., change the condition from just
labError to labError && mode === "lab"), ensuring the component uses the
passed-in mode prop so lab-only errors don't appear in other modes.
Comment on lines
+47
to
+81
| {isLabMode ? ( | ||
| <LoadingButton | ||
| type="button" | ||
| disabled={!labName.trim() || (!isContentOnly && !query.trim())} | ||
| loading={labSaving} | ||
| loadingText="Saving..." | ||
| onClick={onLabSave} | ||
| > | ||
| {mode === "lab-edit" ? "Save Template" : "Create Template"} | ||
| </LoadingButton> | ||
| ) : ( | ||
| <LoadingButton | ||
| type="button" | ||
| disabled={ | ||
| isParamSelect | ||
| ? !paramWidgetName.trim() || | ||
| (paramUIType === "select" && | ||
| (!connectionId || | ||
| !String(chartOptions.seedQuery ?? "").trim())) | ||
| : isContentOnly | ||
| ? false | ||
| : isForm | ||
| ? !connectionId || !query.trim() | ||
| : !query.trim() | ||
| } | ||
| loading={saveStatus === "saving"} | ||
| loadingText="Saving..." | ||
| onClick={onSave} | ||
| > | ||
| {saveStatus === "saved" | ||
| ? "Saved!" | ||
| : mode === "edit" | ||
| ? "Save Changes" | ||
| : "Add Widget"} | ||
| </LoadingButton> |
There was a problem hiding this comment.
Unify the save guard across both branches.
Lines 50-71 validate different things depending on mode, and that breaks real flows. In lab mode, parameter-select templates are effectively unsavable because the predicate requires query, even though that editor path uses paramWidgetName/seedQuery instead. In normal mode, the default branch still allows saving non-content widgets with a query but no connectionId, and buildWidgetForSave() will persist that empty connection id.
Proposed fix
const isParamSelect = chartType === "parameter-select";
const isForm = chartType === "form";
const isLabMode = mode === "lab-edit" || mode === "lab-create";
+ const hasRequiredWidgetFields = isParamSelect
+ ? !!paramWidgetName.trim() &&
+ (paramUIType !== "select" ||
+ (!!connectionId && !!String(chartOptions.seedQuery ?? "").trim()))
+ : isContentOnly
+ ? true
+ : !!connectionId && !!query.trim();
+
+ const widgetSaveDisabled = !hasRequiredWidgetFields;
+ const labSaveDisabled = !labName.trim() || !hasRequiredWidgetFields;
return (
<DialogFooter>
@@
{isLabMode ? (
<LoadingButton
type="button"
- disabled={!labName.trim() || (!isContentOnly && !query.trim())}
+ disabled={labSaveDisabled}
loading={labSaving}
loadingText="Saving..."
onClick={onLabSave}
>
@@
) : (
<LoadingButton
type="button"
- disabled={
- isParamSelect
- ? !paramWidgetName.trim() ||
- (paramUIType === "select" &&
- (!connectionId ||
- !String(chartOptions.seedQuery ?? "").trim()))
- : isContentOnly
- ? false
- : isForm
- ? !connectionId || !query.trim()
- : !query.trim()
- }
+ disabled={widgetSaveDisabled}
loading={saveStatus === "saving"}
loadingText="Saving..."
onClick={onSave}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/widget-editor/modal-footer.tsx` around lines 47 - 81, The
save validation is inconsistent between the lab and normal branches causing
param-select templates to be unsavable in lab mode and allowing empty
connectionId in normal mode; extract the predicate into a single helper (e.g.,
canSaveWidget or validateSaveGuard) that encapsulates the rules: if
isParamSelect then require paramWidgetName.trim() and if paramUIType ===
"select" require connectionId && String(chartOptions.seedQuery ?? "").trim();
else if isContentOnly allow save; else (non-content) require connectionId &&
query.trim(); then replace the duplicated disabled expressions in both
LoadingButton branches to call this helper and ensure onLabSave/onSave (and
buildWidgetForSave) are only invoked when the helper returns true. Use the
existing symbols isLabMode, isParamSelect, paramWidgetName, paramUIType,
connectionId, chartOptions.seedQuery, isContentOnly, query, onLabSave, onSave,
saveStatus and buildWidgetForSave to locate and update the logic.
Comment on lines
+130
to
+136
| const handleRunAndSave = useCallback(() => { | ||
| if (chartType === "markdown" || chartType === "iframe") return; | ||
| if (!query.trim() || saveStatus === "saving") return; | ||
| setSaveStatus("saving"); | ||
| previewQueryRef.current.mutate( | ||
| { connectionId, query }, | ||
| { |
There was a problem hiding this comment.
Run+save path drops query params for parameterized queries.
handlePreview sends extracted params, but the keyboard run+save path does not. Queries with references can fail only on this shortcut.
💡 Suggested fix
const handleRunAndSave = useCallback(() => {
if (chartType === "markdown" || chartType === "iframe") return;
if (!query.trim() || saveStatus === "saving") return;
setSaveStatus("saving");
+ const referenced = extractReferencedParams(query, allParamValues);
+ const params =
+ Object.keys(referenced).length > 0 ? referenced : undefined;
previewQueryRef.current.mutate(
- { connectionId, query },
+ { connectionId, query, params },
{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/widget-editor/use-auto-preview.ts` around lines 130 - 136,
The run+save path in handleRunAndSave currently calls
previewQueryRef.current.mutate without sending the extracted query parameters,
so parameterized queries fail; update handleRunAndSave to include the same
extracted params payload that handlePreview sends (pass the params/parsedParams
or the same param object used by handlePreview) when calling
previewQueryRef.current.mutate and any subsequent save/mutate logic so
parameterized queries receive their params.
Comment on lines
+181
to
+186
| useEffect(() => { | ||
| if (!open && savedTimerRef.current !== null) { | ||
| clearTimeout(savedTimerRef.current); | ||
| savedTimerRef.current = null; | ||
| } | ||
| }, [open]); |
There was a problem hiding this comment.
Reset saveStatus when modal closes to avoid stale “Saved!” on reopen.
If the modal closes before the 1.5s timer completes, the timer is cleared but status can remain "saved".
💡 Suggested fix
useEffect(() => {
if (!open && savedTimerRef.current !== null) {
clearTimeout(savedTimerRef.current);
savedTimerRef.current = null;
}
+ if (!open) {
+ setSaveStatus("idle");
+ }
}, [open]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/widget-editor/use-auto-preview.ts` around lines 181 - 186,
When the modal closes in the useEffect watching open, also reset the save status
to avoid a stale "Saved!" message: after clearing savedTimerRef (in the
useEffect that checks if (!open && savedTimerRef.current !== null)), call
setSaveStatus('idle') (or the hook's neutral state value) so saveStatus is
reset; reference the useAutoPreview hook's savedTimerRef, saveStatus,
setSaveStatus and the open variable when making this change.
2 tasks
Extract save logic, auto-preview, and Advanced tab sections from the 1,405-line monolith into 8 focused sub-components with 52 unit tests. Modal reduced to 857 lines. Fixes buildClickAction missing layout arg. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
force-pushed
the
refactor/issue-573-extract-save-hook
branch
from
May 1, 2026 15:18
3b23be9 to
efb66ea
Compare
…e hooks Cover the two untested hooks extracted in #573 to meet SonarCloud's 80% new-code coverage gate. Tests verify: auto-preview on open with debounce, query-change debounce, CMD/Ctrl+Shift+Enter run-and-save shortcut, saveStatus lifecycle, widget payload construction for all chart types (bar, parameter-select, form, markdown, iframe), click action/styling/cache/transforms inclusion/exclusion, and query history recording. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
This was referenced May 2, 2026
4 tasks
5 tasks
7 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
use-widget-save.ts— encapsulates widget construction for all 3 save paths (handleSave, handleRunAndSave, handleLabSave)use-auto-preview.ts— auto-preview debouncing, run+save shortcut, keyboard handlerlab-metadata-form.tsx— template name/description/tagsmodal-footer.tsx— save/cancel buttons for lab + normal modesbuildClickAction()now correctly receiveslayoutargumentTest plan
tlsmodule resolution issue (not related to this PR)Closes #573
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Refactor
Tests