refactor: backend perf/fixes, frontend dedup, frappe-ui dialog reuse#614
Merged
Conversation
update_page_folder and delete_folder issued one db.set_value per page in a loop. Collapse each into a single filtered UPDATE. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
arrayFunctions.ts patched Array.prototype.add/remove globally, but every .add()/.remove() call site is a Set, DOMTokenList, document.fonts or DOM element - the array methods had zero users. Delete the file and its side-effect import. Also regenerate components.d.ts to drop references to components removed in an earlier commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ambient `declare module "~icons/*"` in shims-vue.d.ts already types these virtual modules (other files import them without suppression), so the 14 @ts-ignore in PropsEditor.vue and 1 in BuilderToolbar.vue were dead. Removing them restores type checking on those imports. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
helpers.ts had grown to 1620 lines mixing unrelated concerns. Extract three cohesive groups into their own files and re-export them from helpers.ts (barrel pattern), so no call sites change: - colors.ts - hex/HSV/rgb conversions - cssUtils.ts - px/spacing/background/unit normalization - scriptSandbox.ts - user block-script execution (restricted + unrestricted) helpers.ts: 1620 -> 1009 lines. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Remove unused exports throttle, stripExtension and logObjectDiff (no callers; throttling is done via @vueuse useThrottleFn elsewhere). - Stop re-exporting RGBToHex; it has no external users and stays an internal helper of getRGB in colors.ts. - Extract the duplicated wawoff2 load+decompress block shared by getFontArrayBuffer and getFontNameFromFile into decompressFontIfWoff2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the hand-rolled setTimeout/clearTimeout debounce and its debounceTimer bookkeeping with useDebounceFn from @vueuse/core (already a dependency). Same 50ms batching behaviour, less state to manage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- NumberArrows: replace raw setTimeout/setInterval hold-to-repeat with useTimeoutFn + useIntervalFn. Fixes a leak where an active hold kept firing timers (and emitting) after the component unmounted. - GlobalDomains: replace the manual pollInterval + onUnmounted bookkeeping with useIntervalFn driven by the hasPendingDomains watcher. Both helpers auto-dispose on scope teardown, so no manual unmount cleanup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- blockController.isBlockSelected: `length || 0 > 0` parsed as `length || (0 > 0)` due to operator precedence, returning a number rather than a boolean; use `(length ?? 0) > 0`. - useAnalytics.getFormattedRoute: the exact/else branches returned the same value, so collapse them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `elif block.get("children"): return is_component_used(...)` returned
inside the loop on the first child-bearing block, so a component used
only in a *later* sibling subtree was reported as unused. This drives
clear_page_cache and Builder Component sync, so it caused stale published
pages and missed component syncs. Recurse without returning False early.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Drop the unused get_dummy_blocks helper (no callers anywhere). - builder_page: remove the `try/except TemplateSyntaxError: raise` no-op around render_template, plus its now-unused import. - Fix a duplicate-variable typo (`public_path, public_path = None, None`) in get_builder_page_preview_file_paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
getDynamicValues aliased this.dynamicValues and pushed the parent component's values straight into it, mutating reactive block state from what callers treat as a getter (it runs during render in several components). Merge into a copy instead so the block's own array is left untouched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The shared MutationObserver was created through useMutationObserver in the first BlockEditor's scope, so it was torn down when that editor unmounted while window.observer stayed truthy and was never recreated — silently killing block tracking for later editors. The module-global updateList also grew unbounded with stale updaters. Use a single native MutationObserver reference-counted against the live target Set: it is created on first registration, every target removes itself on scope dispose, and the observer disconnects once the last one is gone (so it is recreated cleanly next time). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Extract the recursive block-tree search into one exported findBlockInTree (block.ts); the two composable findBlock wrappers (useCanvasUtils, useBlockSelection) and Block's internal lookup now delegate to it instead of each re-implementing the walk. - Collapse the duplicate generateId (Block.generateId + helpers) into the single helpers.generateId and replace the deprecated substr(2, 9) with slice(2, 11). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
savingPage was set true only by the editor watch in PageBuilder, so the direct savePage() callers (copy/paste, keyboard events, route changes) left it false and waitTillPageIsSaved() resolved after its 300ms delay without waiting for the in-flight save. savePage now sets the flag itself; the watch keeps its optimistic pre-set for the debounce window. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- sync_component scanned every Builder Page (get_cached_doc + is_component_used per page). Add the same blocks/draft_blocks LIKE pre-filter that replace_component already uses, so only candidate pages are loaded; the precise is_component_used() check still runs. - Builder Page.replace_component issued two db_set(commit=True) calls per page (2N commits across N pages). Combine them into a single dict db_set so each page commits once. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
MarginHandler and PaddingHandler each re-implemented the same blockStyles breakpoint merge, handleBorderWidth, handle-size clamps, Position enum and the updating flag. Move those byte-identical pieces into a useSpacingHandler composable. Each component keeps its own template, per-side handle offsets and drag handler, because those genuinely differ (margin sits outside the box and drags outward; padding sits inside and inverts bottom/right) — so there is no change to drag behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The builder's hand-rolled imperative-dialog stack (showDialog + appDialogs store array + AlertDialog.vue + the App.vue v-for renderer) duplicated frappe-ui's `dialog` namespace + Dialogs renderer. FrappeUIProvider already mounts <Dialogs />, so showDialog can delegate to dialog.confirm with no other plumbing. - showDialog now wraps dialog.confirm and resolves its Promise<void> on every close path (action click, cancel, dismiss). The 3 imperative callers (the confirm/alert helpers, builderBlockCopyPaste, and useBuilderEvents) keep the same API — no call-site changes. - alert() previously called h(AlertDialog) without mounting the vnode, so router.ts "no permission" and componentStore "component in use" silently displayed nothing. It now routes through showDialog with a single Ok action and actually shows. - Delete AlertDialog.vue, the appDialogs store field + its Dialog type import, and the App.vue v-for renderer; drop the now-unused imports (defineComponent, h, markRaw, useBuilderStore, Controls/Dialog) in helpers.ts. Dialog interaction is visual and esbuild can't verify it — a manual check of the confirm / alert / paste-page / copy-page dialogs is the remaining behavior check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both modals were thin single-form components — open via v-model, render
one or two BuilderInputs, call one action. They're a perfect fit for
frappe-ui's dialog.prompt({ fields, onConfirm }).
- New util utils/dialogs.ts exposes promptCreateFolder() and
promptCreateComponent(block); each opens a prompt and auto-closes on
successful onConfirm (errors render inline via the prompt's setError).
- DashboardSidebar: drop the NewFolder import, mount and ref; the "+"
button now calls promptCreateFolder() directly.
- BlockContextMenu: drop the NewComponent import, mount and ref; the
"Save As Component" action now calls promptCreateComponent(block.value).
- Delete NewFolder.vue and NewComponent.vue.
NewBlockTemplate (FileUploader slot), NewBuilderVariable (custom
ColorInput) and SelectFolder (cross-component flow + visual list)
deliberately left as components — dialog.prompt's PromptField union
doesn't support custom slots, and SelectFolder's setFolder logic is
entangled with dashboard state.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SelectFolder was a styled list with a global ref (showFolderSelectorDialog in useDashboardState) opened by DashboardHead and closed by DashboardContent, whose setFolder applied the chosen folder to the selected pages. Collapse that cross-component flow into a single promptSelectFolder() that owns the whole select-and-apply operation: - New utils/dialogs.ts function uses a select field (Home + each folder), runs update_page_folder on confirm, and clears the dashboard selection state. Same end state as setFolder; errors now render inline instead of silently leaving the dialog open (latent bug in the prior .then-only chain). - DashboardHead's "Move To Folder" button calls promptSelectFolder() directly. - DashboardContent drops the <SelectFolder> mount, the setFolder function, and the createResource import. - useDashboardState removes showFolderSelectorDialog entirely (no more cross-component dialog ref). Delete SelectFolder.vue. UX note: the picker is now a select dropdown rather than a styled list with home/folder icons — the cost of going imperative; the function is the same. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #614 +/- ##
===========================================
- Coverage 57.93% 57.87% -0.06%
===========================================
Files 29 29
Lines 3202 3198 -4
===========================================
- Hits 1855 1851 -4
Misses 1347 1347 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
surajshetty3416
changed the title
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
1 participant
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
update_page_folder/delete_folder;sync_componentgets a blocks LIKE pre-filter (no more full-table scan);Builder Page.replace_componentcollapses 2 commits/page into 1;is_component_usednow traverses all sibling subtrees (was returning early on the first child-bearing block → stale cache + missed component syncs).trackTargetobserver/updateList leak across BlockEditor remounts (single refcounted nativeMutationObserver);getDynamicValuesno longer mutates the reactive array during render;savingPagenow owned bysavePage()sowaitTillPageIsSaved()is reliable for all callers (was only set in the editor watch — 4 direct callers slipped through).helpers.tssplit intocolors.ts/cssUtils.ts/scriptSandbox.ts; onefindBlockInTreefor the three duplicated tree-walks; onegenerateId(substr→slice);useSpacingHandlerfor the byte-identical pieces of Margin/Padding handlers (drag math, offsets, defaults stay per-component);@vueusefor timer / debounce auto-cleanup; misc dead-code removal.showDialog/appDialogs/AlertDialogstack routed throughdialog.confirm(FrappeUIProvideralready mounts<Dialogs />); fixes a brokenalert()that was creating a vnode without mounting it. Three simple modals (NewFolder,NewComponent,SelectFolder) converted todialog.prompt({ fields }).