Fixed an issue where internal settings for the separate tools tab were not being restored during application relaunch. #8988

[pravesh-sharma]

Summary by CodeRabbit

• New Features

  • ERD toolbar preferences (notation, cardinality display, SQL formatting) are now saved, restored, and applied automatically; toolbar reflects saved choices.

• Improvements

  • Schema Diff compare and filter options are persisted and restored; filter selections and compare params now sync across sessions for a more consistent workflow.

[coderabbitai]

Walkthrough

Stores and propagates toolbar preferences: ERDTool reads toolbarPrefs from restored connectionInfo.preferences, includes it in DIRTY/save payloads, and passes it to MainToolBar; MainToolBar accepts toolbarPrefs, applies and updates notation/cardinality/sql_with_drop; schema_diff components load and persist compare/filter preferences.

Changes

Cohort / File(s)	Summary
ERDTool Preferences Integration web/pgadmin/tools/erd/static/js/erd_tool/components/ERDTool.jsx	Adds toolbarPrefs instance property, sets it from toolContent.connectionInfo.preferences during restore, extends DIRTY event payload to include toolbarPrefs, and passes toolbarPrefs to MainToolBar.
MainToolBar Preferences State Management web/pgadmin/tools/erd/static/js/erd_tool/components/MainToolBar.jsx	Accepts new toolbarPrefs prop and PropTypes entry; introduces notationRef for stable notation, applies toolbarPrefs (sql_with_drop, cardinality) on mount/update, adds onCardinalityNotationChange handler, routes cardinality UI through notationRef, and includes toolbarPrefs in debounced save payload.
Schema Diff Preferences Flow web/pgadmin/tools/schema_diff/static/js/components/SchemaDiffButtonComponent.jsx, web/pgadmin/tools/schema_diff/static/js/components/SchemaDiffCompare.jsx	SchemaDiffButtonComponent gains filters prop (removed default for filterParams); SchemaDiffCompare stores full response, initializes compareOptions/filterOptions from connectionInfo.preferences, switches to oldSchemaDiffData.current?.data access, and passes filters={filterOptions}. Save payload now includes preferences (compare/filter).
Manifest/Package package.json	Minor line changes recorded.

Sequence Diagram(s)

sequenceDiagram
    participant ERDTool as ERDTool
    participant Backend as Backend
    participant EventBus as EventBus
    participant MainToolBar as MainToolBar

    ERDTool->>Backend: restoreToolContent(request)
    Backend-->>ERDTool: toolContent (includes connectionInfo.preferences)
    ERDTool->>ERDTool: set toolbarPrefs = connectionInfo.preferences
    ERDTool->>EventBus: emit DIRTY(true, toolContent.data, null, toolbarPrefs)
    EventBus->>MainToolBar: DIRTY event (includes toolbarPrefs)
    MainToolBar->>MainToolBar: apply toolbarPrefs (notation/cardinality/sql_with_drop)
    MainToolBar->>EventBus: user changes -> emit save (includes toolbarPrefs)
    EventBus->>Backend: saveToolData(payload includes preferences)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and accurately describes the main objective of the pull request: fixing the restoration of internal settings for separate tools tabs during application relaunch.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)

web/pgadmin/tools/schema_diff/static/js/components/SchemaDiffButtonComponent.jsx (2)

67-74: ⚠️ Potential issue | 🟡 Minor

filterParams lost its default value — risk of TypeError if passed undefined.

Previously filterParams had a default of [DIFFERENT, SOURCE_ONLY, TARGET_ONLY]. Now it has none, so selectedFilters initializes to undefined if filterParams is ever undefined. Downstream .includes() calls on selectedFilters (lines 215–222) would throw.

Currently SchemaDiffCompare.jsx always passes an array via getFilterParams(), so this won't crash today, but the removed safety net makes the component fragile.

Proposed fix — restore a safe default

-export function SchemaDiffButtonComponent({ sourceData, targetData, selectedRowIds, onServerSchemaChange, rows, compareParams, filterParams, filters }) {
+export function SchemaDiffButtonComponent({ sourceData, targetData, selectedRowIds, onServerSchemaChange, rows, compareParams, filterParams = [], filters }) {

---

228-236: ⚠️ Potential issue | 🟡 Minor

Missing filters in PropTypes declaration.

The filters prop is accepted and used in the component but not declared in the PropTypes block.

Proposed fix

 SchemaDiffButtonComponent.propTypes = {
   sourceData: PropTypes.object,
   targetData: PropTypes.object,
   selectedRowIds: PropTypes.array,
   onServerSchemaChange:PropTypes.func,
   rows: PropTypes.array,
   compareParams: PropTypes.object,
-  filterParams: PropTypes.array
+  filterParams: PropTypes.array,
+  filters: PropTypes.array,
 };

web/pgadmin/tools/schema_diff/static/js/components/SchemaDiffCompare.jsx (1)

157-165: ⚠️ Potential issue | 🟡 Minor

Inconsistent null-safety on oldSchemaDiffData.current.data access.

Line 159 accesses .data without optional chaining, while lines 685 and 709 use oldSchemaDiffData.current?.data. If getToolContent returns an object without a data property (e.g. empty or malformed response), this will throw.

Proposed fix — add optional chaining for consistency

-      _.each(oldSchemaDiffData.current.data,(d)=>{
+      _.each(oldSchemaDiffData.current?.data,(d)=>{

web/pgadmin/tools/erd/static/js/erd_tool/components/ERDTool.jsx (1)

229-236: ⚠️ Potential issue | 🔴 Critical

Existing DIRTY event fires omit toolbarPrefs, causing downstream data loss.

linksUpdated and nodesUpdated (lines 231, 235) fire the DIRTY event with only 3 arguments, leaving toolbarPrefs as undefined in the MainToolBar handler. As noted in the MainToolBar review, this overwrites previously saved toolbar preferences.

To fix this at the source, either pass this.toolbarPrefs in all DIRTY events, or (preferably) fix the MainToolBar handler to preserve existing toolbarPrefs when none are provided — see the MainToolBar comment.

🤖 Fix all issues with AI agents

In `@web/pgadmin/tools/erd/static/js/erd_tool/components/MainToolBar.jsx`:
- Around line 90-98: The checkMenuClick and onCardinalityNotationChange handlers
call setSaveERDData with a functional updater that spreads prev (which can be
null before any DIRTY event), producing an incomplete save payload and causing
useDelayDebounce to invoke saveToolData with undefined data; update those
handlers (the setSaveERDData updater used in checkMenuClick and
onCardinalityNotationChange) to guard against prev === null by either
initializing a full default saveERDData shape (including data, fileName,
isDirty, toolbarPrefs) when prev is null or by early-returning / skipping the
debounce when the existing payload is incomplete, so
useDelayDebounce/saveToolData always receives a complete payload.
- Around line 163-168: The DIRTY event handler currently writes toolbarPrefs
directly from the event args, which can be undefined and overwrite persisted
prefs; update the handler (the ERD_EVENTS.DIRTY callback in MainToolBar.jsx that
calls isDirtyRef.current, setDisableButton('save', ...),
isSaveToolDataEnabled('ERD') and setSaveERDData) to only replace toolbarPrefs
when the incoming toolbarPrefs argument is defined—otherwise preserve the
existing toolbarPrefs from the previous saveERDData state (or a ref) and pass
that into setSaveERDData so undefined event args do not erase stored toolbar
preferences.
- Around line 123-148: The useEffect uses lodash helpers (_.isUndefined,
_.isNull) but lodash is not imported, causing ReferenceError; fix by either
adding an import for lodash (e.g. import _ from 'lodash') at the top of
MainToolBar.jsx so _.isUndefined/_.isNull are defined, or replace those calls
with native checks (e.g. toolbarPrefs !== undefined && toolbarPrefs !== null or
using Object.prototype.hasOwnProperty/typeof) in the useEffect that references
toolbarPrefs, preferences, checkedMenuItems, notationRef and onNotationChange.

🧹 Nitpick comments (2)

web/pgadmin/tools/schema_diff/static/js/components/SchemaDiffCompare.jsx (1)

807-809: Both filterParams and filters props passed — verify intent of dual-prop design.

SchemaDiffButtonComponent receives both filterParams={getFilterParams()} (used to initialize selectedFilters state) and filters={filterOptions} (used in a useEffect to override selectedFilters when restored). This works but is somewhat confusing — two props that ultimately set the same state. Consider consolidating into a single prop in a follow-up to reduce cognitive overhead.

web/pgadmin/tools/erd/static/js/erd_tool/components/ERDTool.jsx (1)

180-180: toolbarPrefs is a plain class property, not React state.

Since this.toolbarPrefs isn't state, changes to it from MainToolBar (via checkMenuClick / onCardinalityNotationChange) won't flow back to ERDTool. Currently that's acceptable because MainToolBar manages persistence independently. However, if future code in ERDTool needs the current toolbar prefs (e.g., for additional DIRTY events), it would read stale data.

Consider promoting toolbarPrefs to component state or using a ref, but this is not blocking for the current scope.

Also applies to: 1088-1092

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@web/pgadmin/tools/erd/static/js/erd_tool/components/MainToolBar.jsx`:
- Around line 124-149: The effect in MainToolBar.jsx (the useEffect that reads
toolbarPrefs/preferences and updates notationRef and calls onNotationChange) is
missing onNotationChange and notation in its dependency array; update the
dependency array to include onNotationChange and notation so the effect re-runs
when the notation prop or the onNotationChange callback changes, ensuring
notationRef.current and the onNotationChange call use the latest values (keep
existing checks for toolbarPrefs/preferences and still update checkedMenuItems
via setCheckedMenuItems as before).

🧹 Nitpick comments (1)

web/pgadmin/tools/erd/static/js/erd_tool/components/MainToolBar.jsx (1)

388-389: Ref read during render — menu checked state won't update without a parent re-render.

notationRef.current is read during render to derive the checked prop. Since mutating a ref doesn't trigger a re-render, the checked state only updates when something else (e.g., onNotationChange propagating a state change in the parent) forces a re-render. This works today because onNotationChange does trigger a parent re-render, but it's fragile — if the parent ever optimizes away that re-render, the menu items will show stale checked states.

Consider using a state variable instead of a ref for the notation value, or accept the coupling and add a comment noting the dependency on the parent re-render.