From 011763223169b9a6e9b755b682ee99a35bb95ce5 Mon Sep 17 00:00:00 2001 From: ruzaqiarkan-eng <289478799+ruzaqiarkan-eng@users.noreply.github.com> Date: Thu, 9 Jul 2026 14:54:34 +0000 Subject: [PATCH 1/2] refactor: align Rule Builder with Frappe v15 reactive architecture - Implemented centralized `watchEffect` in `RuleBuilder` controller for reactive Page API synchronization. - Decoupled Pinia stores from Frappe Page API (moved breadcrumbs to controller). - Removed manual DOM manipulation for title badges in favor of standard `set_indicator`. - Implemented dynamic primary actions (Save, Activate, Edit) based on document state. - Brokered all Page chrome interactions through the controller. - Produced architecture report and migration plan in `report/architecture_report_v1.md`. --- .../js/flexirule/rule_builder/rule_builder.js | 225 ++++++------------ .../rule_builder/stores/useRuleStore.js | 25 -- report/architecture_report_v1.md | 97 ++++++++ 3 files changed, 172 insertions(+), 175 deletions(-) create mode 100644 report/architecture_report_v1.md diff --git a/flexirule/public/js/flexirule/rule_builder/rule_builder.js b/flexirule/public/js/flexirule/rule_builder/rule_builder.js index ae4b9bd5..02ab085c 100644 --- a/flexirule/public/js/flexirule/rule_builder/rule_builder.js +++ b/flexirule/public/js/flexirule/rule_builder/rule_builder.js @@ -1,4 +1,4 @@ -import { createApp, watch } from "vue"; +import { createApp, watchEffect } from "vue"; import { createPinia } from "pinia"; // Import FlexiRule Utilities @@ -23,58 +23,16 @@ class RuleBuilder { init() { this.setup_app(); this.setup_page(); + this.watch_changes(); } setup_page() { - // Set page title - this.page.set_title(__(this.rule)); - // Clear existing actions this.page.clear_actions(); this.page.clear_menu(); this.page.clear_custom_actions(); - // Primary action - Save button - const translatedSaveLabel = __("Save Rule"); - this.save_button_label = - typeof translatedSaveLabel === "string" && translatedSaveLabel.trim() - ? translatedSaveLabel - : "Save Rule"; - this.save_btn = this.page.set_primary_action(this.save_button_label, () => - this.ruleStore.save_changes() - ); - - // Secondary button - Reset - this.reset_btn = this.page.add_button( - __("Reset Changes"), - () => { - this.ruleStore.fetch(); - }, - { icon: "refresh" } - ); - this.reset_btn.hide(); - - // Status Toggle - this.status_btn = this.page.add_inner_button(__("Draft"), async () => { - await this.toggle_rule_active(); - }); - - // Debug - this.test_btn = this.page.add_inner_button(__("Debug Rule"), () => { - flexirule.debug.show_dialog(); - }); - - // Clear visualization if any - this.clear_test_btn = this.page.add_inner_button(__("Clear Debug Path"), () => { - this.uiStore.clear_test_result(); - this.update_test_ui([]); - }); - this.clear_test_btn.hide(); - - // Custom status area beside title - this.setup_custom_header(); - - // Menu items + // Static menu items this.page.add_menu_item(__("Go to Rule"), () => { frappe.set_route("Form", "Rule", this.rule); }); @@ -84,6 +42,25 @@ class RuleBuilder { }); } + setup_breadcrumbs() { + if (frappe.breadcrumbs && frappe.breadcrumbs.$breadcrumbs) { + let breadcrumbs = ` +
  • ${__("Rule")}
    • +
  • ${__(this.ruleStore.rule_doc?.rule_name || this.rule)}
    • +
  • ${__("Builder")}
    • + `; + frappe.breadcrumbs.clear(); + frappe.breadcrumbs.$breadcrumbs.append(breadcrumbs); + } else if (frappe.breadcrumbs && frappe.breadcrumbs.add) { + // Modern Frappe 15 fallback + frappe.breadcrumbs.add({ + type: "Custom", + module: "Rule", + workspace: "RuleFlow", + }); + } + } + show_keyboard_shortcuts() { if (this.uiStore) { this.uiStore.show_shortcuts_help = true; @@ -111,50 +88,68 @@ class RuleBuilder { this.uiStore = useUIStore(pinia); this.ruleStore.rule_name = this.rule; - // Initial sync - this.update_test_ui(this.uiStore.test_execution_path); + this.setup_debug_api(); - // Watch for dirty state changes (computed) - watch( - () => this.ruleStore.is_dirty, - (is_dirty) => { - this.update_save_button(is_dirty); - if (this.reset_btn) { - is_dirty ? this.reset_btn.show() : this.reset_btn.hide(); - } - } - ); + // Mount app + this.$rule_builder = app.mount(this.$wrapper.get(0)); + } - // Watch for active status changes - watch( - () => this.ruleStore.rule_doc?.is_active, - (is_active) => { - this.update_status_button(is_active); + watch_changes() { + watchEffect(() => { + const is_dirty = this.ruleStore.is_dirty; + const is_active = this.ruleStore.is_active; + const has_test_path = this.uiStore.has_test_path; + + // 1. Update Title & Breadcrumbs + this.page.set_title(__(this.ruleStore.rule_doc?.rule_name || this.rule)); + this.setup_breadcrumbs(); + + // 2. Indicators + this.page.clear_indicator(); + if (is_dirty) { + this.page.set_indicator(__("Not Saved"), "orange"); + } else if (is_active) { + this.page.set_indicator(__("Active"), "green"); + } else { + this.page.set_indicator(__("Draft"), "orange"); } - ); - this.uiStore.$subscribe((mutation, state) => { - this.update_test_ui(state.test_execution_path); + // 3. Toolbar Actions + this.refresh_toolbar(is_dirty, is_active, has_test_path); }); + } - // Initial status update after fetch - // We might need to wait for fetch, but store.$subscribe handles mutations. - // We can also watch rule_doc specifically if needed, but the main subscribe is usually enough for state changes. - // Also manual call after mount if data is already there (it fetches async) + refresh_toolbar(is_dirty, is_active, has_test_path) { + this.page.clear_actions(); + this.page.clear_custom_actions(); - // Use a watcher on rule_doc specifically - const unwatch = this.ruleStore.$onAction(({ name, after }) => { - if (name === "fetch") { - after(() => { - this.update_status_button(this.ruleStore.rule_doc?.is_active); - }); - } - }); + // Primary Action: Save or Activate + if (is_dirty) { + this.page.set_primary_action(__("Save Rule"), () => this.ruleStore.save_changes()); + } else if (!is_active) { + this.page.set_primary_action(__("Activate Rule"), () => this.toggle_rule_active()); + } else { + // If active and not dirty, primary action is to Unlock + this.page.set_primary_action(__("Edit Rule"), () => this.toggle_rule_active()); + } - this.setup_debug_api(); + // Secondary Actions + if (is_dirty) { + this.page.add_button(__("Reset Changes"), () => this.ruleStore.fetch(), { + icon: "refresh", + }); + } - // Mount app - this.$rule_builder = app.mount(this.$wrapper.get(0)); + // Inner Buttons + this.page.add_inner_button(__("Debug Rule"), () => { + flexirule.debug.show_dialog(); + }); + + if (has_test_path) { + this.page.add_inner_button(__("Clear Debug Path"), () => { + this.uiStore.clear_test_result(); + }); + } } async toggle_rule_active() { @@ -180,76 +175,6 @@ class RuleBuilder { } finally { frappe.dom.unfreeze(); } - - this.update_status_button(this.ruleStore.rule_doc?.is_active); - } - - update_status_button(is_active) { - const status_text = is_active ? __("Active") : __("Draft"); - const indicator_color = is_active ? "green" : "orange"; - - // 1. Update Title with Badges - // We use a container to avoid overwriting the whole title if possible - if (!this.page.$title_area.find(".flexirule-status-badges").length) { - this.page.$title_area.find(".title-text, .page-title").first().append(` - - `); - } - const $badges = this.page.$title_area.find(".flexirule-status-badges"); - - let badges_html = ` - ${status_text} - `; - - if ( - this.ruleStore.rule_doc?.trigger_type === "Callable Event" && - this.ruleStore.rule_doc?.exposed_as_subrule - ) { - badges_html += ` - ${__("Sub-Rule")} - `; - } - $badges.html(badges_html); - - // 2. Update Toggle Button (on the right) - if (this.status_btn) { - this.status_btn.show().removeClass("hide"); - if (is_active) { - this.status_btn - .text(__("Unlock for Editing")) - .removeClass("btn-default btn-primary btn-success") - .addClass("btn-warning"); - } else { - this.status_btn - .text(__("Set to Active")) - .removeClass("btn-warning btn-success") - .addClass("btn-default"); - } - } - } - - update_save_button(is_dirty) { - if (this.save_btn?.text()?.trim() !== this.save_button_label) { - this.save_btn.text(this.save_button_label); - } - if (is_dirty) { - this.save_btn.removeClass("btn-primary-light").addClass("btn-primary"); - this.page.set_indicator(__("Not Saved"), "orange"); - } else { - this.save_btn.removeClass("btn-primary").addClass("btn-primary-light"); - this.page.clear_indicator(); - } - } - - update_test_ui(test_path) { - const show = !!(test_path && test_path.length > 0); - if (this.clear_test_btn) { - if (show) { - this.clear_test_btn.show().removeClass("hide"); - } else { - this.clear_test_btn.hide().addClass("hide"); - } - } } setup_debug_api() { diff --git a/flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js b/flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js index 4e8041e9..60974d47 100644 --- a/flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js +++ b/flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js @@ -184,8 +184,6 @@ export const useRuleStore = defineStore("rule-builder-rule", () => { }); } - setup_breadcrumbs(); - // We wait for nextTick to ensure all reactive changes from // graphStore sync and normalize have settled before we capture the baseline. await nextTick(); @@ -913,29 +911,6 @@ export const useRuleStore = defineStore("rule-builder-rule", () => { } } - // ── Breadcrumbs (Frappe pattern) ── - function setup_breadcrumbs() { - if (frappe.breadcrumbs && frappe.breadcrumbs.$breadcrumbs) { - let breadcrumbs = ` -
  • ${__("Rule")}
    • -
  • ${__( - rule_doc.value?.rule_name || rule_name.value - )}
    • -
  • ${__("Builder")}
    • - `; - frappe.breadcrumbs.clear(); - frappe.breadcrumbs.$breadcrumbs.append(breadcrumbs); - } else if (frappe.breadcrumbs && frappe.breadcrumbs.add) { - // Modern Frappe 15 fallback (usually handled automatically by standard views, - // but we can add the workspace breadcrumb if needed) - frappe.breadcrumbs.add({ - type: "Custom", - module: "Rule", - workspace: "RuleFlow", - }); - } - } - return { // State rule_name, diff --git a/report/architecture_report_v1.md b/report/architecture_report_v1.md new file mode 100644 index 00000000..90e779b8 --- /dev/null +++ b/report/architecture_report_v1.md @@ -0,0 +1,97 @@ +# FlexiRule Architecture Report & Migration Plan (v1.0) + +## Phase 1: Analysis of Current Implementation + +### 1. Application Initialization & Mounting +- **Page Entry**: The `rule-builder` Frappe Page (`rule_builder.js`) acts as the entry point. It uses `frappe.ui.make_app_page` to initialize the chrome and then dynamically requires `rule_builder.bundle.js`. +- **Controller**: The `frappe.ui.RuleBuilder` class in the bundle is the "Controller". It: + - Receives the `page` object and `wrapper`. + - Initializes Pinia and mounts the Vue 3 app (`App.vue`). + - Holds references to `ruleStore` and `uiStore`. +- **Vue Mounting**: The app is mounted to `.layout-main-section`. + +### 2. Page API & Toolbar Implementation +- **Access**: The `RuleBuilder` class accesses the Page API via `this.page`. +- **Toolbar Buttons**: Primary (Save) and secondary (Reset, Draft/Active toggle, Debug) buttons are created in `setup_page()`. +- **State Coupling**: + - The controller uses manual Vue `watch` calls on `ruleStore.is_dirty` and `ruleStore.rule_doc.is_active`. + - It also uses Pinia `$subscribe` and `$onAction` to trigger UI updates like `update_status_button` and `update_test_ui`. +- **DOM Manipulation**: The status indicators (Active/Draft/Sub-Rule) are currently injected into the title area using jQuery DOM manipulation (`this.page.$title_area.find(...)`). + +### 3. Event & Data Flow +1. **User Action**: User clicks a button in a Vue component. +2. **Store Action**: Component calls a Pinia store action (e.g., `graphStore.addNode`). +3. **State Change**: Store updates state, triggering `is_dirty` computed property. +4. **Controller Watcher**: `RuleBuilder` class detects change via `watch`. +5. **Page Update**: Controller calls `page.set_indicator` or `page.set_primary_action`. + +--- + +## Phase 2: Frappe Best Practices (Source of Truth) + +### 1. Form Builder / Workflow Builder Patterns +- **Standardization**: Frappe uses a JS class (e.g., `WorkflowBuilder`) as a wrapper for the Vue app. +- **Reactivity**: + - They use `watchEffect` inside the controller to keep the Frappe Page synchronized with the store. + - **Dirty State**: `watchEffect(() => { if (store.dirty) frm.dirty(); })`. +- **Toolbar**: + - Primary actions are set once or updated reactively. + - They avoid complex manual DOM manipulation for titles, preferring standard `page.set_title` and `page.set_indicator`. + +### 2. Key Findings +- **Single Source of Truth**: The Pinia store is the source of truth for the *application state*, while the Controller is the source of truth for the *integration*. +- **No Direct API in Vue**: Vue components are "dumb" regarding the Frappe Page; they only know about the store. + +--- + +## Phase 3: Architecture Comparison + +| Feature | Current FlexiRule | Frappe Standard (v15) | Recommendation | +| :--- | :--- | :--- | :--- | +| **Toolbar Sync** | Fragmented `watch` + `$subscribe` | Centralized `watchEffect` | **Adopt `watchEffect`** | +| **Status Display** | Manual jQuery DOM injection | `page.set_indicator` | **Use Standard Indicators** | +| **Primary Action** | Fixed label ("Save Rule") | Dynamic based on context | **Make Dynamic** | +| **Breadcrumbs** | Handled in Pinia Store | Handled in Controller | **Move to Controller** | + +--- + +## Phase 4: Migration Design + +### 1. Reactive Integration Strategy +The `RuleBuilder` controller will implement a single `watch_changes()` method using Vue's `watchEffect`. This effect will track: +- `ruleStore.is_dirty` +- `ruleStore.is_active` +- `uiStore.has_test_path` +- `ruleStore.rule_doc.rule_name` + +When any of these change, the controller will call a `refresh_page()` method that updates the title, indicators, and toolbar in one pass. + +### 2. State Ownership +- **Pinia**: Owns the "Dirty" flag, the "Active" status, and the "Has Results" state. +- **Controller**: Owns the mapping of that state to Frappe Page API calls. + +--- + +## Phase 5: Implementation Plan + +### 1. Files to Modify +- `flexirule/public/js/flexirule/rule_builder/rule_builder.js`: + - Remove fragmented watchers. + - Implement `watch_changes()` and `refresh_toolbar()`. + - Clean up `setup_page()`. +- `flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js`: + - Remove `setup_breadcrumbs` and any other Page API calls. + +### 2. New Update Flow +1. **Change** happens in Store. +2. **`watchEffect`** in `RuleBuilder` is triggered automatically. +3. **`refresh_toolbar()`** clears and re-adds actions based on current store state. +4. **`set_indicator()`** updates the status (Draft/Active) and dirty state. + +### 3. Risks & Testing +- **Risk**: Frequent toolbar clearing might cause a slight flicker. + - *Mitigation*: Only clear/re-add if the relevant state actually changed, or rely on Frappe's efficient DOM handling. +- **Testing**: + - Verify "Save" button appears/disappears correctly. + - Verify "Activate" button replaces "Save" when not dirty. + - Verify "Edit" (Unlock) button appears when Active. From 510f948437f038025c376c0b65fbdee72bbc1acf Mon Sep 17 00:00:00 2001 From: ruzaqiarkan-eng <289478799+ruzaqiarkan-eng@users.noreply.github.com> Date: Thu, 9 Jul 2026 15:04:29 +0000 Subject: [PATCH 2/2] refactor: fix reactive UI updates for rule lifecycle transitions - Fixed bug where UI (indicators/toolbar) wouldn't update after Activation/Deactivation without a hard refresh. - Removed blocking `is_loading` guard in `fetch()` to allow forced refreshes from lifecycle actions. - Enhanced `watchEffect` in `RuleBuilder` controller to track `ruleStore.rule_doc` directly. - Ensured `toggle_rule_active` is the single source of truth for Activation/Deactivation flows in the UI. - Maintained architectural separation between stores (data) and controller (integration). --- flexirule/public/js/flexirule/rule_builder/rule_builder.js | 3 +++ .../public/js/flexirule/rule_builder/stores/useRuleStore.js | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/flexirule/public/js/flexirule/rule_builder/rule_builder.js b/flexirule/public/js/flexirule/rule_builder/rule_builder.js index 02ab085c..d52c35ce 100644 --- a/flexirule/public/js/flexirule/rule_builder/rule_builder.js +++ b/flexirule/public/js/flexirule/rule_builder/rule_builder.js @@ -100,6 +100,9 @@ class RuleBuilder { const is_active = this.ruleStore.is_active; const has_test_path = this.uiStore.has_test_path; + // Access rule_doc to ensure re-run on full doc refresh + const _doc = this.ruleStore.rule_doc; + // 1. Update Title & Breadcrumbs this.page.set_title(__(this.ruleStore.rule_doc?.rule_name || this.rule)); this.setup_breadcrumbs(); diff --git a/flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js b/flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js index 60974d47..2dbca3d9 100644 --- a/flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js +++ b/flexirule/public/js/flexirule/rule_builder/stores/useRuleStore.js @@ -80,8 +80,8 @@ export const useRuleStore = defineStore("rule-builder-rule", () => { const historyStore = useHistoryStore(); const uiStore = useUIStore(); - // Guard to prevent redundant fetches - if (is_loading.value) return; + // Guard to prevent redundant fetches, but allow if it's a forced refresh + // (is_loading is often set by the caller like save_changes or activate_rule) is_loading.value = true; uiStore.is_initializing = true;