SchemaView: DataGridView audit + deferredDepChange protocol#10005
Draft
asheshv wants to merge 3 commits into
Draft
SchemaView: DataGridView audit + deferredDepChange protocol#10005asheshv wants to merge 3 commits into
asheshv wants to merge 3 commits into
Conversation
Four upstream bugs that silently no-op'd, exposed by aggressive review of the SchemaView/DataGridView code: - Feature priority comparator typo: `f1.priorty` is undefined, so the < comparison was always false and features never sorted. - Row className: `classList.join[' ']` bracket-accesses `Array.prototype.join` (returns undefined), so every grid row rendered with no CSS classes. - Reorder hover class: `classList?.append(...)` — Arrays have no `.append` method; the optional-chain silently no-op'd, so the hover row highlight was never applied during drag-reorder. - Inherited-column lock: `inSchemaWithColumnCheck` short-circuited on `isNew(state)` before reading `inheritedfrom`. In a child table CREATE flow, inherited columns arrive with `attnum` undefined (so `isNew` returns true) — the function returned false (not disabled), making Name and datatype editable on columns that should mirror the parent table. New regression tests pin each fix.
Add the queue-based deferredDepChange protocol so schemas can run async work (confirm dialogs, server fetches) atomically across a state change, with first-class error surfacing. Core infrastructure: - `SchemaState/reducer.js`: compose actionObj (type/path/value/ depChange + _.cloneDeep(oldState)) and append resolved callbacks to data.__deferred__. - `hooks/useSchemaState.js`: `drainDeferredQueue` processes queued promises in order; rejections surface via pgAdmin.Browser.notifier.error (falls back to console.error when the notifier is unavailable). Drain useEffect depends on the __deferred__ ref identity rather than .length so replaced entries reliably re-trigger. - `DepListener.js`: prefix-match guard uses delimiter-terminated joins so 'shared' doesn't match 'shared_username'. Caches joined source/dest keys for the hot match path. addDepListener deep-copies the source path so a caller mutating its own array later doesn't poison the listener. - `utils/listenDepChanges.js`: full JSDoc contract for depChange and deferredDepChange callbacks — return shape, when to opt out (return undefined), side-effect rule, actionObj fields, drain error-surfacing contract. Tests: - dep_listener.spec.js: prefix-match isolation, defensive snapshot, listener lifecycle. - reducer_deferred.spec.js: queue mechanism, actionObj composition. - deferred_drain.spec.js: serial processing, callback delta application, notifier.error on rejection, console.error fallback. - drain_useeffect_race.spec.jsx: deterministic race regression test for the ref-vs-length useEffect dependency fix.
Five schemas adopt the queue-based deferredDepChange contract,
fixing real bugs along the way:
- exclusion_constraint.ui.js (amname): opt out when amname is
unchanged or columns is empty — pre-fix code queued a
'clear columns' confirm dialog unconditionally, warning about
clearing an empty collection on the very first selection.
- index.ui.js (amname): explicit `return undefined` opt-out in
place of the no-op Promise.resolve(()=>{}) round-trip.
- table.ui.js (typname + coll_inherits): typname opt-out when
unchanged with stale-ofTypeTables guard
(`typeTable?.oftype_columns ?? []`); changeColumnOptions
hoisted out of the resolved callback per protocol JSDoc;
coll_inherits handles same-length swap (previous add/remove
branches missed it); stale-OID guard in REMOVE branch prevents
the `col.inheritedid != undefined` filter from dropping
local user-added columns.
- foreign_table.ui.js (inherits): same swap + stale-OID guard.
- azure_schema.ui.js (is_authenticating): orphan-Promise fix —
the previous version returned a Promise that never resolved
in the non-match branch, leaving a permanent __deferred__
entry. Source path compared against
`source[source.length - 1]` (arrays, not strings). Auth
failure surfaces to the user via notifier and resets
is_authenticating + clears stale auth_code on failure.
New `*.deferred.spec.js` for each schema, plus contract updates
to the existing exclusion_constraint.ui.spec.js and
table.ui.spec.js.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Fixes multiple silent UI failures in SchemaView’s DataGridView, and introduces a queue-based deferredDepChange protocol so schema dependency updates can safely perform async work (confirm dialogs, fetches) without leaking or losing pending work across batched React updates.
Changes:
- Fix DataGridView silent no-ops (feature priority sort, row className join, reorder hover class, inherited-column edit lock).
- Add a deferred dependency-change queue, reducer accumulation, and a drain mechanism with error surfacing; strengthen DepListener path-matching behavior.
- Update multiple schemas/tests to follow the new
deferredDepChangeopt-out + “resolve to callback” contract.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/pgadmin/static/js/SchemaView/DataGridView/features/feature.js | Fixes feature priority comparator typo so feature ordering works. |
| web/pgadmin/static/js/SchemaView/DataGridView/row.jsx | Fixes row className construction so CSS classes actually apply. |
| web/pgadmin/static/js/SchemaView/DataGridView/features/reorder.jsx | Fixes reorder-hover class application (Array .push vs .append). |
| web/pgadmin/static/js/SchemaView/SchemaState/reducer.js | Accumulates deferred queue entries instead of overwriting them. |
| web/pgadmin/static/js/SchemaView/hooks/useSchemaState.js | Adds deferred queue drain helper + ref-based effect dependency. |
| web/pgadmin/static/js/SchemaView/DepListener.js | Improves prefix matching, caches keys, adds defCallback short-circuit. |
| web/pgadmin/static/js/SchemaView/utils/listenDepChanges.js | Documents the authoritative deferredDepChange protocol contract. |
| web/pgadmin/misc/cloud/static/js/azure_schema.ui.js | Adopts new deferred contract; fixes non-resolving Promise + better error surfacing. |
| web/pgadmin/browser/server_groups/servers/databases/schemas/tables/static/js/table.ui.js | Adopts deferred contract; fixes inherits/oftype edge cases and mutation issues. |
| web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js | Adopts deferred contract; avoids input-state mutation; clean opt-out. |
| web/pgadmin/browser/server_groups/servers/databases/schemas/tables/constraints/exclusion_constraint/static/js/exclusion_constraint.ui.js | Adopts deferred contract; adds opt-out and avoids unnecessary confirms. |
| web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js | Fixes inherited-column lock ordering so CREATE-mode inherited cols are read-only. |
| web/pgadmin/browser/server_groups/servers/databases/schemas/foreign_tables/static/js/foreign_table.ui.js | Adopts deferred contract; fixes swap/remove edge cases and mutation. |
| web/regression/javascript/SchemaView/reducer_deferred.spec.js | Tests reducer queue accumulation behavior. |
| web/regression/javascript/SchemaView/deferred_drain.spec.js | Tests drain protocol error surfacing + non-function guard behavior. |
| web/regression/javascript/SchemaView/drain_useeffect_race.spec.jsx | Tests the React-batching dependency-array race fix. |
| web/regression/javascript/SchemaView/dep_listener.spec.js | Tests DepListener prefix matching + deferred behavior + removal semantics. |
| web/regression/javascript/SchemaView/no_bracket_on_prototype_method.spec.js | Guards against bracket-on-prototype-method typo across SchemaView tree. |
| web/regression/javascript/SchemaView/feature_register.spec.js | Tests feature registration ordering by priority. |
| web/regression/javascript/schema_ui_files/table.ui.spec.js | Updates existing table tests for opt-out behavior. |
| web/regression/javascript/schema_ui_files/table.ui.deferred.spec.js | Adds table deferredDepChange contract/edge-case tests. |
| web/regression/javascript/schema_ui_files/index.ui.deferred.spec.js | Adds index deferredDepChange contract/behavior tests. |
| web/regression/javascript/schema_ui_files/exclusion_constraint.ui.spec.js | Updates existing exclusion-constraint tests for new actionObj signature/inputs. |
| web/regression/javascript/schema_ui_files/exclusion_constraint.deferred.spec.js | Adds exclusion-constraint deferredDepChange contract/behavior tests. |
| web/regression/javascript/schema_ui_files/foreign_table.deferred.spec.js | Adds foreign-table inherits deferredDepChange contract/edge-case tests. |
| web/regression/javascript/schema_ui_files/column.ui.spec.js | Adds regression test for inherited-column lock in CREATE mode. |
| web/regression/javascript/schema_ui_files/azure_schema.deferred.spec.js | Adds azure deferredDepChange contract/behavior tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+42
to
+45
| export const drainDeferredQueue = (items, dispatch) => { | ||
| items.forEach((item) => { | ||
| Promise.resolve(item.promise).then( | ||
| (resFunc) => { |
Comment on lines
51
to
+53
| removeDepListener(dest) { | ||
| this._depListeners = _.filter(this._depListeners, (l)=>!_.join(l.dest, '|').startsWith(_.join(dest, '|'))); | ||
| this._hasDefCallback = this._depListeners.some((l) => l.defCallback); |
Comment on lines
+35
to
+38
| register(HighPriorityFeature); | ||
| register(LowPriorityFeature); | ||
|
|
||
| const fs = new FeatureSet(); |
Contributor
Author
Manual Test PlanPrerequisites
Section A — DataGridView mechanical fixesA1. Feature priority ordering (
|
Comment on lines
+42
to
+86
| export const drainDeferredQueue = (items, dispatch) => { | ||
| items.forEach((item) => { | ||
| Promise.resolve(item.promise).then( | ||
| (resFunc) => { | ||
| if (typeof resFunc !== 'function') { | ||
| // Protocol violation: the schema author resolved with | ||
| // something other than a (tmpstate) => deltaObj callback. | ||
| // Loud console.error so it trips dev/QA test suites; not a | ||
| // notifier toast because this is a code bug, not a runtime | ||
| // failure the end user can act on. | ||
| console.error( | ||
| 'deferredDepChange promise must resolve to a callback function; ' | ||
| + 'got %o. The dispatch is skipped — see useSchemaState ' | ||
| + 'drainDeferredQueue for the protocol.', | ||
| resFunc, | ||
| ); | ||
| return; | ||
| } | ||
| dispatch({ | ||
| type: SCHEMA_STATE_ACTIONS.DEFERRED_DEPCHANGE, | ||
| path: item.action.path, | ||
| depChange: item.action.depChange, | ||
| listener: { | ||
| ...item.listener, | ||
| callback: resFunc, | ||
| }, | ||
| }); | ||
| }, | ||
| (err) => { | ||
| const msg = err?.message || String(err) || 'unknown error'; | ||
| const userMsg = gettext('Dependent update failed: ') + msg; | ||
| const notifier = pgAdmin?.Browser?.notifier; | ||
| if (typeof notifier?.error === 'function') { | ||
| notifier.error(userMsg); | ||
| } else { | ||
| // Notifier unavailable (very early init, isolated test | ||
| // harness, etc.). Surface to console.error rather than | ||
| // silently dropping the rejection — the latter is the bug | ||
| // this drainer exists to prevent. | ||
| console.error('deferredDepChange:', userMsg, err); | ||
| } | ||
| }, | ||
| ); | ||
| }); | ||
| }; |
Comment on lines
+30
to
+33
| * 1. The resolved value MUST be a function (the callback that returns | ||
| * the data delta). If it's anything else, we warn and skip — this | ||
| * catches the common protocol mistake of resolving with a data | ||
| * object directly. |
Comment on lines
51
to
54
| removeDepListener(dest) { | ||
| this._depListeners = _.filter(this._depListeners, (l)=>!_.join(l.dest, '|').startsWith(_.join(dest, '|'))); | ||
| this._hasDefCallback = this._depListeners.some((l) => l.defCallback); | ||
| } |
Comment on lines
51
to
+53
| removeDepListener(dest) { | ||
| this._depListeners = _.filter(this._depListeners, (l)=>!_.join(l.dest, '|').startsWith(_.join(dest, '|'))); | ||
| this._hasDefCallback = this._depListeners.some((l) => l.defCallback); |
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
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
Three commits addressing silent-failure bugs in
DataGridViewand introducing a queue-baseddeferredDepChangeprotocol forSchemaView, with five schemas adopting the new contract.1.
fix(datagridview): correct silent-failure bugs in DataGridViewFour bugs that silently no-op'd:
f1.priortyis undefined, so<was always false and features never sorted.classList.join[' ']bracket-accessesArray.prototype.join(returns undefined). Every grid row rendered with no CSS classes.classList?.append(...)— Arrays have no.append; the optional chain silently no-op'd, so the hover-row highlight never applied during drag-reorder.inSchemaWithColumnCheckshort-circuited onisNew(state)before readinginheritedfrom. Inherited columns in CREATE mode (noattnum) bypassed the lock, making Name and datatype editable on columns that should mirror the parent.2.
feat(schemaview): introduce deferredDepChange protocolNew queue-based protocol so schemas can run async work (confirm dialogs, server fetches) atomically across a state change:
SchemaState/reducer.js— appends resolved callbacks todata.__deferred__with the canonicalactionObjshape.hooks/useSchemaState.js—drainDeferredQueueprocesses promises in order; rejections surface viapgAdmin.Browser.notifier.error(falls back toconsole.errorwhen the notifier is unavailable). DrainuseEffectkeyed on the__deferred__ref identity, not.length, so replaced entries reliably re-trigger.DepListener.js— prefix-match guard using delimiter-terminated joins (soshareddoesn't matchshared_username), cached source/dest keys, defensive deep-copy of source path on registration.utils/listenDepChanges.js— full JSDoc contract: return shape, opt-out viareturn undefined, side-effect-before-resolve rule,actionObjfields, drain error-surfacing.3.
fix(schemas): align deferredDepChange callbacks with new protocolFive schemas adopt the contract — real bugs fixed along the way:
exclusion_constraint.ui.js(amname) — opt out whenamnameunchanged orcolumnsempty; pre-fix code warned about clearing an empty collection on the very first selection.index.ui.js(amname) — explicitreturn undefinedopt-out in place ofPromise.resolve(()=>{})round-trip.table.ui.js(typname + coll_inherits) —typeTable?.oftype_columns ?? []guard against staleofTypeTables;changeColumnOptionshoisted out of the resolved callback (per protocol); same-length inherits swap handling; stale-OID guard preventingcol.inheritedid != undefinedfrom dropping local user-added columns.foreign_table.ui.js(inherits) — same swap + stale-OID guard.azure_schema.ui.js(is_authenticating) — orphan-Promise fix (was never resolving in the non-match branch, leaving a permanent__deferred__entry); source-path comparison againstsource[source.length - 1](arrays, not strings); auth failure surfaced via notifier with staleauth_codecleared.Test plan
cd web && yarn run jest— 151 suites / 884 tests passingcd web && yarn linter— cleanNotes
master.feature_register.spec.js,no_bracket_on_prototype_method.spec.js,dep_listener.spec.js,reducer_deferred.spec.js,deferred_drain.spec.js,drain_useeffect_race.spec.jsx, plus*.deferred.spec.jsfor each of the five affected schemas.deferredDepChangeJSDoc contract inutils/listenDepChanges.jsis the authoritative reference for schema authors.