A PR review flagged two blocking correctness bugs: String.replace's special $-pattern handling could silently corrupt a formula's DC.ORIG_VALUE/DC.USER_NAME substitution whenever the underlying cell data or username contained a literal $, and afterChange was indexing dataSource with Handsontable's visual row instead of translating to physical first, desyncing the EDIT_STATUS cell and "overwritten" comment sync on a sorted grid. Manually reproducing the second issue surfaced a third, unrelated Handsontable library bug — updateSettings() corrupts formula cell references (#REF!) whenever a sort is active, independent of what the call actually changes — which needed its own fix to unblock verification.
Implementation
$-substitution corruption: substituteBoundedToken's text.replace(pattern, replacement) used a string replacement, letting String.replace interpret $&/$`/$'/$<digit> in arbitrary cell data. Switched to a function replacement (() => replacement), whose return value is used verbatim — the single shared fix point, since substituteColumnReferences.ts calls the same function.
Sorted-grid row desync: afterChange now translates the visual row via hot.toPhysicalRow() before calling syncOverwrittenCommentForCell/updateEditStatusForRow (matching beforeChange's existing pattern). Those two methods, whose established contract is a physical row, now translate back to visual via hot.toVisualRow() for every Handsontable/comments-plugin API call — fixing only the call site would have moved the bug rather than closed it.
updateSettings() formula corruption: reproduced directly in Karma — a bare {} settings object, called while a sort is active, is enough to corrupt formula cells. A new updateSettingsSortSafe() wrapper clears the sort, calls updateSettings(), and restores it immediately after. Applied to every updateSettings() call site reachable during an active edit session (editTable, cancelEdit, getPendingExcelPreview, discardPendingExcel, cancelSubmit, checkSave, closeRecordEdit, confirmRecordEdit, applyDisplayColHeaders, grid-resize) except the very first call at initial table load, where nothing can be sorted yet.
Minor: the native paste listener added on hot.rootElement is now stored and removed in ngOnDestroy, closing a listener leak the same review flagged.
editor.component.ts — afterChange row translation, syncOverwrittenCommentForCell/updateEditStatusForRow visual-row translation, new updateSettingsSortSafe() wrapper applied across all reachable updateSettings() call sites, pasteListener field + cleanup in ngOnDestroy.
sortedGridRowSync.integration.spec.ts (new) — reproduces the visual/physical row desync and the updateSettings-while-sorted formula corruption against real Handsontable + HyperFormula instances, and proves both fixes.
editor.cy.ts — test 53: sorts a table, edits the row that moved, asserts DC.ROW_STATUS updates on the correct row and no other row's status changes.
## Intent
A PR review flagged two blocking correctness bugs: `String.replace`'s special `$`-pattern handling could silently corrupt a formula's `DC.ORIG_VALUE`/`DC.USER_NAME` substitution whenever the underlying cell data or username contained a literal `$`, and `afterChange` was indexing `dataSource` with Handsontable's visual row instead of translating to physical first, desyncing the `EDIT_STATUS` cell and "overwritten" comment sync on a sorted grid. Manually reproducing the second issue surfaced a third, unrelated Handsontable library bug — `updateSettings()` corrupts formula cell references (`#REF!`) whenever a sort is active, independent of what the call actually changes — which needed its own fix to unblock verification.
## Implementation
- **`$`-substitution corruption**: `substituteBoundedToken`'s `text.replace(pattern, replacement)` used a string replacement, letting `String.replace` interpret `$&`/`` $` ``/`$'`/`$<digit>` in arbitrary cell data. Switched to a function replacement (`() => replacement`), whose return value is used verbatim — the single shared fix point, since `substituteColumnReferences.ts` calls the same function.
- **Sorted-grid row desync**: `afterChange` now translates the visual row via `hot.toPhysicalRow()` before calling `syncOverwrittenCommentForCell`/`updateEditStatusForRow` (matching `beforeChange`'s existing pattern). Those two methods, whose established contract is a physical row, now translate back to visual via `hot.toVisualRow()` for every Handsontable/comments-plugin API call — fixing only the call site would have moved the bug rather than closed it.
- **`updateSettings()` formula corruption**: reproduced directly in Karma — a bare `{}` settings object, called while a sort is active, is enough to corrupt formula cells. A new `updateSettingsSortSafe()` wrapper clears the sort, calls `updateSettings()`, and restores it immediately after. Applied to every `updateSettings()` call site reachable during an active edit session (`editTable`, `cancelEdit`, `getPendingExcelPreview`, `discardPendingExcel`, `cancelSubmit`, `checkSave`, `closeRecordEdit`, `confirmRecordEdit`, `applyDisplayColHeaders`, grid-resize) except the very first call at initial table load, where nothing can be sorted yet.
- **Minor**: the native `paste` listener added on `hot.rootElement` is now stored and removed in `ngOnDestroy`, closing a listener leak the same review flagged.
## Changes
- `parseFormulaRule.ts` / `.spec.ts`, `substituteColumnReferences.spec.ts` — function-replacement fix plus `$&`/`` $` ``/`$'`/`$<digit>` regression tests.
- `editor.component.ts` — `afterChange` row translation, `syncOverwrittenCommentForCell`/`updateEditStatusForRow` visual-row translation, new `updateSettingsSortSafe()` wrapper applied across all reachable `updateSettings()` call sites, `pasteListener` field + cleanup in `ngOnDestroy`.
- `sortedGridRowSync.integration.spec.ts` (new) — reproduces the visual/physical row desync and the `updateSettings`-while-sorted formula corruption against real Handsontable + HyperFormula instances, and proves both fixes.
- `editor.cy.ts` — test 53: sorts a table, edits the row that moved, asserts `DC.ROW_STATUS` updates on the correct row and no other row's status changes.
Yury
self-assigned this 2026-09-02 14:22:17 +00:00
String.replace interpreted $&/$`/$'/$<digit> specially in DC.ORIG_VALUE/DC.USER_NAME substitutions, corrupting formulas whose cell data happened to contain a literal $. Separately, afterChange indexed dataSource with Handsontable's visual row instead of translating to physical (as beforeChange already does), desyncing EDIT_STATUS/overwritten-comment writes on a sorted grid. Investigating that surfaced a third, unrelated Handsontable bug: updateSettings() corrupts formula cell references whenever a sort is active, regardless of what it changes - fixed by clearing the sort around every updateSettings() call.
Yury
requested review from allan 2026-09-02 14:22:17 +00:00
Summary: Three well-reasoned bug fixes (formula $-substitution corruption, visual/physical row desync on sorted grids, and updateSettings+sort formula #REF! corruption) backed by thorough integration and e2e tests. High-quality PR.
Issues
editor.component.ts:1790 — if (visualRow === null) return is dead code. Handsontable.toVisualRow() returns number, never null. The TypeScript type wouldn't allow this check to ever fire. Not harmful, but misleading — a future reader may think there's a null-return path to handle. Same in updateEditStatusForRow at line 1982.
Suggestions
editor.component.ts — updateSettingsSortSafe() is used for 7 call sites, but editTable() (line 1268) and cancelEdit() (line 1379) use an inline if (sortConfigs.length > 0) columnSorting.clearSort() pattern instead of the wrapper. The inline approach is correct for those cases (the sort is restored later in the method, not immediately after), but a one-line comment in updateSettingsSortSafe noting "editTable/cancelEdit manage sort clear/restore inline because their restore point is later in the method, not immediately after updateSettings" would prevent a future maintainer from "fixing" the inconsistency.
sortedGridRowSync.integration.spec.ts:731 — const sortConfigs = Array.isArray(sortConfig) ? sortConfig : sortConfig ? [sortConfig] : [] duplicates getCurrentSortConfigs() logic from editor.component.ts. If that method is exported/testable, the test should use it directly to avoid drift.
Looks Good
The String.replace → function-replacement fix (() => replacement) is the correct fix for the $&/$`/$' corruption. Verified: node -e confirms string replacement interprets $& as the matched substring and $` as the preceding text, while function replacement preserves them verbatim.
Integration spec uses a real Handsontable + multiColumnSorting + HyperFormula instance to demonstrate both failure modes and fixes — not mocks. This is the right approach for visual/physical divergence bugs.
The Cypress e2e test (test 53) snapshots every other row's status and asserts none changed, which correctly distinguishes "edit landed on the right row" from "edit landed on whatever moved into the visual slot."
The pasteListener cleanup in ngOnDestroy fixes a real listener leak (previously attached a new closure per edit session with no removal).
The updateSettingsSortSafe workaround for the Handsontable sort+formulas #REF! bug is pragmatic and well-documented.
Reviewed by 4gl-reviewer
## Code Review — 4gl-reviewer
**Summary:** Three well-reasoned bug fixes (formula `$`-substitution corruption, visual/physical row desync on sorted grids, and `updateSettings`+sort formula `#REF!` corruption) backed by thorough integration and e2e tests. High-quality PR.
### Issues
- **editor.component.ts:1790** — `if (visualRow === null) return` is dead code. `Handsontable.toVisualRow()` returns `number`, never `null`. The TypeScript type wouldn't allow this check to ever fire. Not harmful, but misleading — a future reader may think there's a null-return path to handle. Same in `updateEditStatusForRow` at line 1982.
### Suggestions
- **editor.component.ts** — `updateSettingsSortSafe()` is used for 7 call sites, but `editTable()` (line 1268) and `cancelEdit()` (line 1379) use an inline `if (sortConfigs.length > 0) columnSorting.clearSort()` pattern instead of the wrapper. The inline approach is correct for those cases (the sort is restored later in the method, not immediately after), but a one-line comment in `updateSettingsSortSafe` noting "editTable/cancelEdit manage sort clear/restore inline because their restore point is later in the method, not immediately after updateSettings" would prevent a future maintainer from "fixing" the inconsistency.
- **sortedGridRowSync.integration.spec.ts:731** — `const sortConfigs = Array.isArray(sortConfig) ? sortConfig : sortConfig ? [sortConfig] : []` duplicates `getCurrentSortConfigs()` logic from editor.component.ts. If that method is exported/testable, the test should use it directly to avoid drift.
### Looks Good
- The `String.replace` → function-replacement fix (`() => replacement`) is the correct fix for the `$&`/`` $` ``/`$'` corruption. Verified: `node -e` confirms string replacement interprets `$&` as the matched substring and `` $` `` as the preceding text, while function replacement preserves them verbatim.
- Integration spec uses a real Handsontable + multiColumnSorting + HyperFormula instance to demonstrate both failure modes and fixes — not mocks. This is the right approach for visual/physical divergence bugs.
- The Cypress e2e test (test 53) snapshots every other row's status and asserts none changed, which correctly distinguishes "edit landed on the right row" from "edit landed on whatever moved into the visual slot."
- The `pasteListener` cleanup in `ngOnDestroy` fixes a real listener leak (previously attached a new closure per edit session with no removal).
- The `updateSettingsSortSafe` workaround for the Handsontable sort+formulas `#REF!` bug is pragmatic and well-documented.
*Reviewed by 4gl-reviewer*
Summary: Three well-reasoned bug fixes (formula $-substitution corruption, visual/physical row desync on sorted grids, and updateSettings+sort formula #REF! corruption) backed by thorough integration and e2e tests. High-quality PR.
Issues
editor.component.ts:1790 — if (visualRow === null) return is dead code. Handsontable.toVisualRow() returns number, never null. The TypeScript type wouldn't allow this check to ever fire. Not harmful, but misleading — a future reader may think there's a null-return path to handle. Same in updateEditStatusForRow at line 1982.
Suggestions
editor.component.ts — updateSettingsSortSafe() is used for 7 call sites, but editTable() (line 1268) and cancelEdit() (line 1379) use an inline if (sortConfigs.length > 0) columnSorting.clearSort() pattern instead of the wrapper. The inline approach is correct for those cases (the sort is restored later in the method, not immediately after), but a one-line comment in updateSettingsSortSafe noting "editTable/cancelEdit manage sort clear/restore inline because their restore point is later in the method, not immediately after updateSettings" would prevent a future maintainer from "fixing" the inconsistency.
sortedGridRowSync.integration.spec.ts:731 — const sortConfigs = Array.isArray(sortConfig) ? sortConfig : sortConfig ? [sortConfig] : [] duplicates getCurrentSortConfigs() logic from editor.component.ts. If that method is exported/testable, the test should use it directly to avoid drift.
Looks Good
The String.replace → function-replacement fix (() => replacement) is the correct fix for the $&/$`/$' corruption. Verified: node -e confirms string replacement interprets $& as the matched substring and $` as the preceding text, while function replacement preserves them verbatim.
Integration spec uses a real Handsontable + multiColumnSorting + HyperFormula instance to demonstrate both failure modes and fixes — not mocks. This is the right approach for visual/physical divergence bugs.
The Cypress e2e test (test 53) snapshots every other row's status and asserts none changed, which correctly distinguishes "edit landed on the right row" from "edit landed on whatever moved into the visual slot."
The pasteListener cleanup in ngOnDestroy fixes a real listener leak (previously attached a new closure per edit session with no removal).
The updateSettingsSortSafe workaround for the Handsontable sort+formulas #REF! bug is pragmatic and well-documented.
Reviewed by 4gl-reviewer
Applied both suggestions: updateSettingsSortSafe() now documents why editTable()/cancelEdit() manage sort clear/restore inline instead of using it, and the sort-config normalization is extracted into a shared normalizeSortConfig() used by both the component and the test.
On the visualRow === null check: I don't think it's dead code — Handsontable's public .d.ts declares toVisualRow() as returning number, but the underlying IndexMapper.getVisualFromPhysicalIndex() it calls is declared number | null, and this file already has a pre-existing example (rowHeaders/afterGetRowHeader) treating toPhysicalRow()'s return the same way. tsc also compiles clean on the comparison, which it wouldn't if TS considered it unreachable. Keeping the guard.
> ## Code Review — 4gl-reviewer
>
> **Summary:** Three well-reasoned bug fixes (formula `$`-substitution corruption, visual/physical row desync on sorted grids, and `updateSettings`+sort formula `#REF!` corruption) backed by thorough integration and e2e tests. High-quality PR.
>
> ### Issues
>
> - **editor.component.ts:1790** — `if (visualRow === null) return` is dead code. `Handsontable.toVisualRow()` returns `number`, never `null`. The TypeScript type wouldn't allow this check to ever fire. Not harmful, but misleading — a future reader may think there's a null-return path to handle. Same in `updateEditStatusForRow` at line 1982.
>
> ### Suggestions
>
> - **editor.component.ts** — `updateSettingsSortSafe()` is used for 7 call sites, but `editTable()` (line 1268) and `cancelEdit()` (line 1379) use an inline `if (sortConfigs.length > 0) columnSorting.clearSort()` pattern instead of the wrapper. The inline approach is correct for those cases (the sort is restored later in the method, not immediately after), but a one-line comment in `updateSettingsSortSafe` noting "editTable/cancelEdit manage sort clear/restore inline because their restore point is later in the method, not immediately after updateSettings" would prevent a future maintainer from "fixing" the inconsistency.
>
> - **sortedGridRowSync.integration.spec.ts:731** — `const sortConfigs = Array.isArray(sortConfig) ? sortConfig : sortConfig ? [sortConfig] : []` duplicates `getCurrentSortConfigs()` logic from editor.component.ts. If that method is exported/testable, the test should use it directly to avoid drift.
>
> ### Looks Good
>
> - The `String.replace` → function-replacement fix (`() => replacement`) is the correct fix for the `$&`/`` $` ``/`$'` corruption. Verified: `node -e` confirms string replacement interprets `$&` as the matched substring and `` $` `` as the preceding text, while function replacement preserves them verbatim.
> - Integration spec uses a real Handsontable + multiColumnSorting + HyperFormula instance to demonstrate both failure modes and fixes — not mocks. This is the right approach for visual/physical divergence bugs.
> - The Cypress e2e test (test 53) snapshots every other row's status and asserts none changed, which correctly distinguishes "edit landed on the right row" from "edit landed on whatever moved into the visual slot."
> - The `pasteListener` cleanup in `ngOnDestroy` fixes a real listener leak (previously attached a new closure per edit session with no removal).
> - The `updateSettingsSortSafe` workaround for the Handsontable sort+formulas `#REF!` bug is pragmatic and well-documented.
>
> *Reviewed by 4gl-reviewer*
Applied both suggestions: updateSettingsSortSafe() now documents why editTable()/cancelEdit() manage sort clear/restore inline instead of using it, and the sort-config normalization is extracted into a shared normalizeSortConfig() used by both the component and the test.
On the visualRow === null check: I don't think it's dead code — Handsontable's public .d.ts declares toVisualRow() as returning number, but the underlying IndexMapper.getVisualFromPhysicalIndex() it calls is declared number | null, and this file already has a pre-existing example (rowHeaders/afterGetRowHeader) treating toPhysicalRow()'s return the same way. tsc also compiles clean on the comparison, which it wouldn't if TS considered it unreachable. Keeping the guard.
Summary: Solid PR fixing three real bugs: $-substitution corruption in parseFormulaRule (string-replace interpreting $&/$1 etc.), visual/physical row desync in the afterChange hook and its callee methods on sorted grids, and a Handsontable updateSettings()-while-sorted formula #REF! corruption. Fixes are correct, well-documented, and well-tested. One inconsistency to address.
Issues:
editor.component.ts:1351 (cancelEdit) — Still uses the inline Array.isArray(columnSortConfig) ? columnSortConfig : [columnSortConfig] pattern instead of the new getCurrentSortConfigs()/normalizeSortConfig() helper. This misses the third branch (falsy/undefined): if getSortConfig() returns falsy, sortConfigs becomes [undefined] (length 1), so clearSort() runs (harmless), but the restore loop calls columnSorting.sort(undefined) which may throw or misbehave. editTable() at line 1264 correctly uses getCurrentSortConfigs() — cancelEdit() should too.
Suggestions:
editor.component.ts:4479,1798,1989 — The === null guard for toPhysicalRow/toVisualRow is good, but some Handsontable type definitions also permit undefined. Consider if (row == null) (loose equality) to catch both, for robustness against version differences.
editor.component.ts:1349-1352 — Since cancelEdit() deliberately can't use updateSettingsSortSafe (later restore point), consider extracting just the normalizeSortConfig call here too, to keep the sort-config normalization in one place even when the full wrapper can't be used.
Looks good:
The () => replacement function-replacement fix in parseFormulaRule.ts is the correct, idiomatic fix for String.replace's $-pattern interpretation. Excellent edge-case test coverage ($&, $', $`, $1).
updateSettingsSortSafe is a clean wrapper with a thorough comment explaining why editTable()/cancelEdit() can't use it (their restore point is later). Good separation of concerns.
The integration spec (sortedGridRowSync.integration.spec.ts) using a real Handsontable + multiColumnSorting instance to demonstrate both the buggy and fixed behavior is exactly the right testing strategy — unit mocks couldn't prove visual/physical divergence.
Cypress test 53 is well-designed: sorting first, then verifying other rows' statuses didn't change (not just that the edited row got 'M') proves the edit landed on the correct row.
Storing the paste listener as this.pasteListener for proper ngOnDestroy cleanup is correct.
## Code Review — 4gl-reviewer
**Summary:** Solid PR fixing three real bugs: `$`-substitution corruption in `parseFormulaRule` (string-replace interpreting `$&`/`$1` etc.), visual/physical row desync in the `afterChange` hook and its callee methods on sorted grids, and a Handsontable `updateSettings()`-while-sorted formula `#REF!` corruption. Fixes are correct, well-documented, and well-tested. One inconsistency to address.
**Issues:**
- `editor.component.ts:1351` (cancelEdit) — Still uses the inline `Array.isArray(columnSortConfig) ? columnSortConfig : [columnSortConfig]` pattern instead of the new `getCurrentSortConfigs()`/`normalizeSortConfig()` helper. This misses the third branch (falsy/`undefined`): if `getSortConfig()` returns falsy, `sortConfigs` becomes `[undefined]` (length 1), so `clearSort()` runs (harmless), but the restore loop calls `columnSorting.sort(undefined)` which may throw or misbehave. `editTable()` at line 1264 correctly uses `getCurrentSortConfigs()` — `cancelEdit()` should too.
**Suggestions:**
- `editor.component.ts:4479,1798,1989` — The `=== null` guard for `toPhysicalRow`/`toVisualRow` is good, but some Handsontable type definitions also permit `undefined`. Consider `if (row == null)` (loose equality) to catch both, for robustness against version differences.
- `editor.component.ts:1349-1352` — Since `cancelEdit()` deliberately can't use `updateSettingsSortSafe` (later restore point), consider extracting just the `normalizeSortConfig` call here too, to keep the sort-config normalization in one place even when the full wrapper can't be used.
**Looks good:**
- The `() => replacement` function-replacement fix in `parseFormulaRule.ts` is the correct, idiomatic fix for `String.replace`'s `$`-pattern interpretation. Excellent edge-case test coverage (`$&`, `$'`, `` $` ``, `$1`).
- `updateSettingsSortSafe` is a clean wrapper with a thorough comment explaining why `editTable()`/`cancelEdit()` can't use it (their restore point is later). Good separation of concerns.
- The integration spec (`sortedGridRowSync.integration.spec.ts`) using a real Handsontable + multiColumnSorting instance to demonstrate both the buggy and fixed behavior is exactly the right testing strategy — unit mocks couldn't prove visual/physical divergence.
- Cypress test 53 is well-designed: sorting first, then verifying *other rows'* statuses didn't change (not just that the edited row got 'M') proves the edit landed on the correct row.
- Storing the paste listener as `this.pasteListener` for proper `ngOnDestroy` cleanup is correct.
Summary: Solid, well-tested fix for two genuine bugs: visual/physical row-index confusion in the afterChange → EDIT_STATUS/overwritten-comment sync path on sorted grids, and a HOT multiColumnSorting+formulas #REF! corruption triggered by updateSettings() while sorted. The substituteBoundedToken$-escaping fix is correct and well-covered.
Issues:
editor.component.ts (cancelEdit, ~L1350) — cancelEdit() inlines Array.isArray(columnSortConfig) ? columnSortConfig : [columnSortConfig] instead of reusing this.getCurrentSortConfigs() / normalizeSortConfig(). If getSortConfig() returns undefined (no active sort), this yields [undefined], so the guard if (sortConfigs.length > 0) columnSorting.clearSort() passes (length 1) and the restore loop calls columnSorting.sort(undefined). editTable() was already migrated to the defensive helper; cancelEdit() should be too, for both consistency and correctness.
editor.component.ts (cancelEdit, ~L1350) — same call calls columnSorting.getSortConfig() directly with no try/catch, but getCurrentSortConfigs() was specifically written to wrap that call because it throws when the plugin's internal state is null (VA embed / early load). cancelEdit() is now the one un-guarded caller of the same throwing path.
editor.component.ts (cancelEdit, ~L1376-1382) — getFormulaCellsToPreserveOnCancel's callbacks pass the iterating physical rowIndex straight into commentsPlugin.getCommentAtCell(rowIndex, ...) and hot.getDataAtRowProp(rowIndex, prop) — both HOT APIs expect a visual row. On a sorted grid this reads the wrong cell, the same class of bug this PR fixes in syncOverwrittenCommentForCell/updateEditStatusForRow. Pre-existing, but since this PR is specifically about that translation it's worth closing here.
Suggestions:
editor.component.ts (cancelEdit) — replace the inline normalization with const sortConfigs = this.getCurrentSortConfigs() so it gets the undefined→[] handling and the try/catch guard for free.
editor.component.ts (updateSettingsSortSafe) — consider try/finally around hot.updateSettings(settings, render) so a thrown settings error still restores the sort (otherwise the grid is left unsorted on failure).
editor.component.ts (afterChange hook) — the new const row = hot.toPhysicalRow(visualRow); if (row === null) continue is good; consider logging/skipping silently is fine, but note changedRows.add(null) is avoided by the guard — verify changedRows is typed Set<number> so a stray null can't slip in elsewhere.
Looks good:
substituteBoundedToken switch from string to function replacement (() => replacement) is the correct fix for the $&/$`/$'/$1 corruption — and the new spec cases (a$&b, a$'b, a$`b, a$1b) lock it in.
normalizeSortConfig extraction is clean, and the integration spec (sortedGridRowSync.integration.spec.ts) reproduces all three failure modes (afterChange row, EDIT_STATUS write, comment placement, and the #REF! corruption) against a real HOT instance — exactly the right level of proof.
The Cypress e2e (test 53) snapshots every other row's status rather than asserting a specific other row, correctly avoiding seed-data coincidence.
pasteListener stored on the instance and removed in ngOnDestroy is a clean listener-leak fix.
## Code Review — 4gl-reviewer
**Summary:** Solid, well-tested fix for two genuine bugs: visual/physical row-index confusion in the afterChange → EDIT_STATUS/overwritten-comment sync path on sorted grids, and a HOT multiColumnSorting+formulas `#REF!` corruption triggered by `updateSettings()` while sorted. The `substituteBoundedToken` `$`-escaping fix is correct and well-covered.
**Issues:**
- `editor.component.ts` (cancelEdit, ~L1350) — `cancelEdit()` inlines `Array.isArray(columnSortConfig) ? columnSortConfig : [columnSortConfig]` instead of reusing `this.getCurrentSortConfigs()` / `normalizeSortConfig()`. If `getSortConfig()` returns `undefined` (no active sort), this yields `[undefined]`, so the guard `if (sortConfigs.length > 0) columnSorting.clearSort()` passes (length 1) and the restore loop calls `columnSorting.sort(undefined)`. `editTable()` was already migrated to the defensive helper; `cancelEdit()` should be too, for both consistency and correctness.
- `editor.component.ts` (cancelEdit, ~L1350) — same call calls `columnSorting.getSortConfig()` directly with no try/catch, but `getCurrentSortConfigs()` was specifically written to wrap that call because it throws when the plugin's internal state is null (VA embed / early load). `cancelEdit()` is now the one un-guarded caller of the same throwing path.
- `editor.component.ts` (cancelEdit, ~L1376-1382) — `getFormulaCellsToPreserveOnCancel`'s callbacks pass the iterating physical `rowIndex` straight into `commentsPlugin.getCommentAtCell(rowIndex, ...)` and `hot.getDataAtRowProp(rowIndex, prop)` — both HOT APIs expect a *visual* row. On a sorted grid this reads the wrong cell, the same class of bug this PR fixes in `syncOverwrittenCommentForCell`/`updateEditStatusForRow`. Pre-existing, but since this PR is specifically about that translation it's worth closing here.
**Suggestions:**
- `editor.component.ts` (cancelEdit) — replace the inline normalization with `const sortConfigs = this.getCurrentSortConfigs()` so it gets the `undefined`→`[]` handling and the try/catch guard for free.
- `editor.component.ts` (updateSettingsSortSafe) — consider `try/finally` around `hot.updateSettings(settings, render)` so a thrown settings error still restores the sort (otherwise the grid is left unsorted on failure).
- `editor.component.ts` (afterChange hook) — the new `const row = hot.toPhysicalRow(visualRow); if (row === null) continue` is good; consider logging/skipping silently is fine, but note `changedRows.add(null)` is avoided by the guard — verify `changedRows` is typed `Set<number>` so a stray null can't slip in elsewhere.
**Looks good:**
- `substituteBoundedToken` switch from string to function replacement (`() => replacement`) is the correct fix for the `$&`/`` $` ``/`$'`/`$1` corruption — and the new spec cases (`a$&b`, `a$'b`, `` a$`b ``, `a$1b`) lock it in.
- `normalizeSortConfig` extraction is clean, and the integration spec (`sortedGridRowSync.integration.spec.ts`) reproduces all three failure modes (afterChange row, EDIT_STATUS write, comment placement, and the `#REF!` corruption) against a real HOT instance — exactly the right level of proof.
- The Cypress e2e (test 53) snapshots *every other row's* status rather than asserting a specific other row, correctly avoiding seed-data coincidence.
- `pasteListener` stored on the instance and removed in `ngOnDestroy` is a clean listener-leak fix.
Summary: Well-executed PR fixing three distinct but related bugs in the editor: $-substitution corruption in formula rules, visual/physical row desync on sorted grids, and formula #REF! corruption from updateSettings() during active sort. Excellent test coverage and documentation throughout.
Issues:
[editor.component.ts:1228] — updateSettingsSortSafe clears sort before updateSettings and restores after, but if updateSettings throws, the sort is lost (not restored). A try/finally wrapping the updateSettings call would guarantee sort restoration even on exception.
Suggestions:
[editor.component.ts:3642] — The resize observer's updateSettingsSortSafe({ height }, false) will clear/restore sort on every window resize while sorted. Since the render: false flag signals an intent to avoid unnecessary rendering, the forced sort clear/restore partially defeats that. Consider whether the height-only path truly needs the wrapper or could guard on sortConfigs.length > 0 && hasFormulaColumns to skip the overhead for non-formula grids. Minor — correctness is fine, just a performance consideration for large sorted grids during rapid resize.
[normalizeSortConfig.ts:9] — Using any for both param and return is pragmatic given SortConfig isn't publicly exported, but a generic signature export const normalizeSortConfig = <T>(cfg: T | T[] | undefined): T[] => ... would preserve type info for callers without needing the deep import.
Looks good:
The () => replacement function-replacement fix in substituteBoundedToken is the correct approach — string replacement's $&/$`/$'/$<n> interpretation is a classic footgun, and the test cases for each special pattern are thorough.
Integration spec (sortedGridRowSync.integration.spec.ts) is exemplary: each describe block demonstrates the failure mode first, then the fix, against a real Handsontable + multiColumnSorting instance. This proves the bugs are real and the fixes close them, not just that unsorted grids still work.
The decision to keep editTable()/cancelEdit() with inline clear/restore (rather than using the wrapper) is correctly justified — their restore point is later in the method, and the comment explicitly warns against "simplifying" them.
Paste listener cleanup in ngOnDestroy properly prevents the accumulated-listener leak.
quoteLiteral already doubles embedded " characters, so the function-replacement fix handles the full round-trip for arbitrary user data correctly.
## Code Review — 4gl-reviewer
**Summary:** Well-executed PR fixing three distinct but related bugs in the editor: `$`-substitution corruption in formula rules, visual/physical row desync on sorted grids, and formula `#REF!` corruption from `updateSettings()` during active sort. Excellent test coverage and documentation throughout.
**Issues:**
- [`editor.component.ts:1228`] — `updateSettingsSortSafe` clears sort before `updateSettings` and restores after, but if `updateSettings` throws, the sort is lost (not restored). A `try/finally` wrapping the `updateSettings` call would guarantee sort restoration even on exception.
**Suggestions:**
- [`editor.component.ts:3642`] — The resize observer's `updateSettingsSortSafe({ height }, false)` will clear/restore sort on every window resize while sorted. Since the `render: false` flag signals an intent to avoid unnecessary rendering, the forced sort clear/restore partially defeats that. Consider whether the height-only path truly needs the wrapper or could guard on `sortConfigs.length > 0 && hasFormulaColumns` to skip the overhead for non-formula grids. Minor — correctness is fine, just a performance consideration for large sorted grids during rapid resize.
- [`normalizeSortConfig.ts:9`] — Using `any` for both param and return is pragmatic given SortConfig isn't publicly exported, but a generic signature `export const normalizeSortConfig = <T>(cfg: T | T[] | undefined): T[] => ...` would preserve type info for callers without needing the deep import.
**Looks good:**
- The `() => replacement` function-replacement fix in `substituteBoundedToken` is the correct approach — string replacement's `$&`/`` $` ``/`$'`/`$<n>` interpretation is a classic footgun, and the test cases for each special pattern are thorough.
- Integration spec (`sortedGridRowSync.integration.spec.ts`) is exemplary: each `describe` block demonstrates the failure mode first, then the fix, against a real Handsontable + multiColumnSorting instance. This proves the bugs are real and the fixes close them, not just that unsorted grids still work.
- The decision to keep `editTable()`/`cancelEdit()` with inline clear/restore (rather than using the wrapper) is correctly justified — their restore point is later in the method, and the comment explicitly warns against "simplifying" them.
- Paste listener cleanup in `ngOnDestroy` properly prevents the accumulated-listener leak.
- `quoteLiteral` already doubles embedded `"` characters, so the function-replacement fix handles the full round-trip for arbitrary user data correctly.
Summary: Well-crafted fix for three interrelated Handsontable issues: formula $-substitution corruption via String.replace, sorted-grid visual/physical row desync in afterChange/updateEditStatusForRow/syncOverwrittenCommentForCell, and updateSettings corrupting formula refs while a sort is active. Thorough test coverage including integration tests against real Handsontable instances.
Issues:
[editor.component.ts ~L4590] — this.pasteListener is reassigned on each call without removing the previous listener. If the registration code runs more than once (e.g. switching tables mid-session), the old listener leaks since only the latest reference is stored. Add a guard: if (this.pasteListener) { hot.rootElement.removeEventListener('paste', this.pasteListener) } before re-assigning, or guard registration with if (!this.pasteListener).
Suggestions:
[normalizeSortConfig.ts:10] — any[] return type loses type safety. Consider Record<string, unknown>[] at minimum, or the Handsontable sort config type if reachable.
[package.json] — The fast-uri devDependency addition and readdir-glob nested override seem unrelated to the formula/sort bug fixes. Consider splitting into a separate PR for cleaner history.
[editor.component.ts] — Pinning handsontable to exact 18.0.0 (removing caret) is reasonable but a brief comment explaining why would help future maintainers understand the lock.
Looks good:
Excellent test design: each integration test demonstrates the failure mode first, then the fix — making the bug and resolution self-documenting.
The updateSettingsSortSafe wrapper is a clean abstraction, and the comment explaining why editTable()/cancelEdit() deliberately don't use it (different restore point) prevents future "simplification" regressions.
The $-substitution fix (string → function replacement in substituteBoundedToken) is the correct, minimal change with targeted test cases for each special $ pattern ($&, $`, $', $1).
Cypress test 53 is thorough: snapshotting every other row's status before/after the edit proves the write landed on the correct row, not just "some row changed".
## Code Review — 4gl-reviewer
**Summary:** Well-crafted fix for three interrelated Handsontable issues: formula `$`-substitution corruption via `String.replace`, sorted-grid visual/physical row desync in `afterChange`/`updateEditStatusForRow`/`syncOverwrittenCommentForCell`, and `updateSettings` corrupting formula refs while a sort is active. Thorough test coverage including integration tests against real Handsontable instances.
**Issues:**
- [editor.component.ts ~L4590] — `this.pasteListener` is reassigned on each call without removing the previous listener. If the registration code runs more than once (e.g. switching tables mid-session), the old listener leaks since only the latest reference is stored. Add a guard: `if (this.pasteListener) { hot.rootElement.removeEventListener('paste', this.pasteListener) }` before re-assigning, or guard registration with `if (!this.pasteListener)`.
**Suggestions:**
- [normalizeSortConfig.ts:10] — `any[]` return type loses type safety. Consider `Record<string, unknown>[]` at minimum, or the Handsontable sort config type if reachable.
- [package.json] — The `fast-uri` devDependency addition and `readdir-glob` nested override seem unrelated to the formula/sort bug fixes. Consider splitting into a separate PR for cleaner history.
- [editor.component.ts] — Pinning `handsontable` to exact `18.0.0` (removing caret) is reasonable but a brief comment explaining why would help future maintainers understand the lock.
**Looks good:**
- Excellent test design: each integration test demonstrates the failure mode first, then the fix — making the bug and resolution self-documenting.
- The `updateSettingsSortSafe` wrapper is a clean abstraction, and the comment explaining why `editTable()`/`cancelEdit()` deliberately don't use it (different restore point) prevents future "simplification" regressions.
- The `$`-substitution fix (string → function replacement in `substituteBoundedToken`) is the correct, minimal change with targeted test cases for each special `$` pattern (`$&`, `` $` ``, `$'`, `$1`).
- Cypress test 53 is thorough: snapshotting every other row's status before/after the edit proves the write landed on the correct row, not just "some row changed".
Summary: Solid fix for two real bugs — formula $-substitution corruption via String.replace pattern interpretation, and sorted-grid visual/physical row desync causing wrong-cell writes. Excellent test coverage demonstrating both failure modes and fixes. Two consistency issues in cancelEdit() worth addressing.
Issues:
[editor.component.ts:1349-1352] — cancelEdit() inlines Array.isArray(columnSortConfig) ? columnSortConfig : [columnSortConfig] instead of calling this.getCurrentSortConfigs(). When getSortConfig() returns undefined (no sort active), this produces [undefined] (length 1), which triggers clearSort() (harmless) but then calls columnSorting.sort(undefined) in the restore loop at line 1414 — potentially throwing or behaving unpredictably. editTable() correctly uses getCurrentSortConfigs() which normalizes to [] via normalizeSortConfig. These should be consistent.
[editor.component.ts:1366-1374] — The getFormulaCellsToPreserveOnCancel callbacks (getCommentAtCell(rowIndex, ...) and getDataAtRowProp(rowIndex, prop)) run beforeclearSort() is called (line 1393), so the sort is still active. The function iterates 0..dataSource.length (physical indices), but both Handsontable APIs expect visual rows. On a sorted grid, the wrong cells would be checked for comments/values — the same class of desync bug this PR fixes in afterChange/updateEditStatusForRow/syncOverwrittenCommentForCell. Consider translating via hot.toVisualRow(rowIndex) in the callbacks, or moving clearSort() before this block.
Suggestions:
[normalizeSortConfig.ts:9] — The any return type is understandable given SortConfig isn't publicly exported in this Handsontable version, but consider a structural type (e.g. { column: number; sortOrder: string }[]) for minimal type safety on the fields actually used by callers.
[editor.component.ts:1240] — updateSettingsSortSafe calls columnSorting.clearSort() without checking if the plugin is initialized. getCurrentSortConfigs() already handles the null-plugin case defensively, but columnSorting.clearSort() itself would throw if the plugin is null. Consider a null-check or try/catch matching the defensive style of getCurrentSortConfigs().
Looks good:
The substituteBoundedToken fix — switching from string to function replacement (() => replacement) — is the correct way to prevent String.replace from interpreting $&/$`/$'/$<digit> in arbitrary user data. The spec tests covering $&, $', $`, and $1 are thorough.
updateSettingsSortSafe is a clean abstraction for the Handsontable sort+formulas #REF! corruption workaround, with an excellent comment explaining why editTable()/cancelEdit() can't use it directly.
The integration spec (sortedGridRowSync.integration.spec.ts) is outstanding — it reproduces each failure mode against a real Handsontable instance, then proves the fix, rather than mocking.
The paste listener refactor to a stored reference with proper removeEventListener cleanup in ngOnDestroy prevents listener leaks across repeated edit sessions.
Cypress e2e test #53 is well-designed — snapshotting all other rows' statuses and asserting none changed is a more robust proof than asserting a single expected value.
## Code Review — 4gl-reviewer
**Summary:** Solid fix for two real bugs — formula `$`-substitution corruption via `String.replace` pattern interpretation, and sorted-grid visual/physical row desync causing wrong-cell writes. Excellent test coverage demonstrating both failure modes and fixes. Two consistency issues in `cancelEdit()` worth addressing.
**Issues:**
- [editor.component.ts:1349-1352] — `cancelEdit()` inlines `Array.isArray(columnSortConfig) ? columnSortConfig : [columnSortConfig]` instead of calling `this.getCurrentSortConfigs()`. When `getSortConfig()` returns `undefined` (no sort active), this produces `[undefined]` (length 1), which triggers `clearSort()` (harmless) but then calls `columnSorting.sort(undefined)` in the restore loop at line 1414 — potentially throwing or behaving unpredictably. `editTable()` correctly uses `getCurrentSortConfigs()` which normalizes to `[]` via `normalizeSortConfig`. These should be consistent.
- [editor.component.ts:1366-1374] — The `getFormulaCellsToPreserveOnCancel` callbacks (`getCommentAtCell(rowIndex, ...)` and `getDataAtRowProp(rowIndex, prop)`) run *before* `clearSort()` is called (line 1393), so the sort is still active. The function iterates `0..dataSource.length` (physical indices), but both Handsontable APIs expect visual rows. On a sorted grid, the wrong cells would be checked for comments/values — the same class of desync bug this PR fixes in `afterChange`/`updateEditStatusForRow`/`syncOverwrittenCommentForCell`. Consider translating via `hot.toVisualRow(rowIndex)` in the callbacks, or moving `clearSort()` before this block.
**Suggestions:**
- [normalizeSortConfig.ts:9] — The `any` return type is understandable given `SortConfig` isn't publicly exported in this Handsontable version, but consider a structural type (e.g. `{ column: number; sortOrder: string }[]`) for minimal type safety on the fields actually used by callers.
- [editor.component.ts:1240] — `updateSettingsSortSafe` calls `columnSorting.clearSort()` without checking if the plugin is initialized. `getCurrentSortConfigs()` already handles the null-plugin case defensively, but `columnSorting.clearSort()` itself would throw if the plugin is null. Consider a null-check or try/catch matching the defensive style of `getCurrentSortConfigs()`.
**Looks good:**
- The `substituteBoundedToken` fix — switching from string to function replacement (`() => replacement`) — is the correct way to prevent `String.replace` from interpreting `$&`/`` $` ``/`$'`/`$<digit>` in arbitrary user data. The spec tests covering `$&`, `$'`, `` $` ``, and `$1` are thorough.
- `updateSettingsSortSafe` is a clean abstraction for the Handsontable sort+formulas `#REF!` corruption workaround, with an excellent comment explaining why `editTable()`/`cancelEdit()` can't use it directly.
- The integration spec (`sortedGridRowSync.integration.spec.ts`) is outstanding — it reproduces each failure mode against a real Handsontable instance, then proves the fix, rather than mocking.
- The paste listener refactor to a stored reference with proper `removeEventListener` cleanup in `ngOnDestroy` prevents listener leaks across repeated edit sessions.
- Cypress e2e test #53 is well-designed — snapshotting all other rows' statuses and asserting none changed is a more robust proof than asserting a single expected value.
Summary: Well-structured fix for two real Handsontable integration bugs: formula $-substitution corruption via String.replace's special pattern interpretation, and sorted-grid row/formula desync caused by visual/physical row confusion. The integration spec and cypress test provide solid coverage of the failure modes.
Issues:
[editor.component.ts:1376-1383] — In cancelEdit(), the getFormulaCellsToPreserveOnCancel callbacks call commentsPlugin.getCommentAtCell(rowIndex, ...) and hot.getDataAtRowProp(rowIndex, prop)beforeclearSort() runs (line 1399), while the sort is still active. rowIndex here iterates 0..dataSource.length as a physical index, but getCommentAtCell/getDataAtRowProp expect visual rows. On a sorted grid these calls will read the wrong cell/comment. The sort clear happens 14 lines later (after toPreserve is already computed). Consider moving the clearSort() call above the getFormulaCellsToPreserveOnCancel block, or translating rowIndex to visual within the callbacks.
Suggestions:
[editor.component.ts:1357-1359] — cancelEdit() still uses the inline normalization (Array.isArray(columnSortConfig) ? columnSortConfig : [columnSortConfig]) instead of the new normalizeSortConfig helper, and doesn't handle the undefined case (if no sort is active, getSortConfig() returns undefined, [undefined] won't be empty, so clearSort() runs unnecessarily). Consider using this.getCurrentSortConfigs() like editTable() does, which delegates to normalizeSortConfig and handles undefined.
Looks good:
Excellent test coverage: the integration spec reproduces each failure mode against a real Handsontable instance, and the cypress test proves the fix end-to-end with the sort-then-edit sequence.
The substituteBoundedToken fix (using a function replacement to avoid $&/$\``/$'/$` corruption) is correct and well-documented, with targeted spec coverage for each special pattern.
The updateSettingsSortSafe wrapper is a clean, well-commented abstraction, and the explicit note about why editTable/cancelEdit don't use it (their restore point is later) shows good awareness of the control flow.
The paste listener cleanup in ngOnDestroy properly prevents the listener leak, and removing any previous listener before re-registering is a nice defensive touch.
Pinning handsontable to exact 18.0.0 with the explanatory comment about version-dependent workarounds is a pragmatic choice.
## Code Review — 4gl-reviewer
**Summary:** Well-structured fix for two real Handsontable integration bugs: formula `$`-substitution corruption via `String.replace`'s special pattern interpretation, and sorted-grid row/formula desync caused by visual/physical row confusion. The integration spec and cypress test provide solid coverage of the failure modes.
**Issues:**
- [editor.component.ts:1376-1383] — In `cancelEdit()`, the `getFormulaCellsToPreserveOnCancel` callbacks call `commentsPlugin.getCommentAtCell(rowIndex, ...)` and `hot.getDataAtRowProp(rowIndex, prop)` **before** `clearSort()` runs (line 1399), while the sort is still active. `rowIndex` here iterates 0..dataSource.length as a physical index, but `getCommentAtCell`/`getDataAtRowProp` expect visual rows. On a sorted grid these calls will read the wrong cell/comment. The sort clear happens 14 lines later (after `toPreserve` is already computed). Consider moving the `clearSort()` call above the `getFormulaCellsToPreserveOnCancel` block, or translating `rowIndex` to visual within the callbacks.
**Suggestions:**
- [editor.component.ts:1357-1359] — `cancelEdit()` still uses the inline normalization (`Array.isArray(columnSortConfig) ? columnSortConfig : [columnSortConfig]`) instead of the new `normalizeSortConfig` helper, and doesn't handle the `undefined` case (if no sort is active, `getSortConfig()` returns `undefined`, `[undefined]` won't be empty, so `clearSort()` runs unnecessarily). Consider using `this.getCurrentSortConfigs()` like `editTable()` does, which delegates to `normalizeSortConfig` and handles `undefined`.
**Looks good:**
- Excellent test coverage: the integration spec reproduces each failure mode against a real Handsontable instance, and the cypress test proves the fix end-to-end with the sort-then-edit sequence.
- The `substituteBoundedToken` fix (using a function replacement to avoid `$&`/`$\``/`$'`/`$<n>` corruption) is correct and well-documented, with targeted spec coverage for each special pattern.
- The `updateSettingsSortSafe` wrapper is a clean, well-commented abstraction, and the explicit note about why `editTable`/`cancelEdit` don't use it (their restore point is later) shows good awareness of the control flow.
- The paste listener cleanup in `ngOnDestroy` properly prevents the listener leak, and removing any previous listener before re-registering is a nice defensive touch.
- Pinning `handsontable` to exact `18.0.0` with the explanatory comment about version-dependent workarounds is a pragmatic choice.
cancelEdit()'s getFormulaCellsToPreserveOnCancel callbacks passed a
physical rowIndex straight into visual-row-expecting Handsontable APIs
(getCommentAtCell, getDataAtRowProp) while the grid could still be
sorted, since clearSort() only ran later, right before updateSettings().
On a sorted grid this read the wrong row's comment/value, corrupting
which formula cells got preserved across cancel. Move clearSort() (via
getCurrentSortConfigs()) ahead of those callbacks instead.
Summary: Well-executed PR fixing three interrelated bugs: $-substitution corruption in formula parsing, visual/physical row index desync on sorted grids, and formula #REF! corruption when updateSettings runs during an active sort. The fixes are correct, thoroughly tested with integration specs demonstrating both failure modes and fixes, and well-documented.
Issues:
None blocking.
Suggestions:
editor.component.ts (updateSettingsSortSafe, ~line 1218) — Multi-column sort restoration uses a loop of individual columnSorting.sort(sortConfig) calls. Single-column sort is covered by existing tests but multi-column sort restoration is not. Consider adding a test case to confirm Handsontable's sort() accumulates rather than replaces across successive calls after clearSort().
package.json:57,72 — Pinning handsontable and @handsontable/angular-wrapper to exact 18.0.0 (removing ^) prevents automatic patch updates including potential security fixes. Worth documenting the rationale in release notes or a comment.
normalizeSortConfig.ts:13 — The any return type is well-justified in the comment. If feasible in a future cleanup, a type SortConfig = Record<string, unknown> alias would add marginal type safety without fighting the type system.
Looks good:
Excellent test coverage — sortedGridRowSync.integration.spec.ts demonstrates both the failure mode and the fix for each bug against a real Handsontable instance, making the regression value clear.
The pasteListener memory leak fix (storing the reference, removing in ngOnDestroy, defensive remove-before-attach) is clean and correct.
The $-substitution fix (string replacement → function replacement () => replacement) is the textbook correct approach for String.replace with untrusted replacement values.
The inline sort clear/restore in editTable()/cancelEdit() is correctly NOT migrated to updateSettingsSortSafe — the JSDoc clearly explains why the restore point must be later in the method.
Null checks on toVisualRow/toPhysicalRow return values are good defensive programming.
The cypress e2e test (test 53) verifies the end-to-end sorted-grid edit scenario, including snapshotting all other rows' statuses to prove the edit landed on the correct row.
## Code Review — 4gl-reviewer
**Summary:** Well-executed PR fixing three interrelated bugs: `$`-substitution corruption in formula parsing, visual/physical row index desync on sorted grids, and formula `#REF!` corruption when `updateSettings` runs during an active sort. The fixes are correct, thoroughly tested with integration specs demonstrating both failure modes and fixes, and well-documented.
**Issues:**
- None blocking.
**Suggestions:**
- `editor.component.ts` (`updateSettingsSortSafe`, ~line 1218) — Multi-column sort restoration uses a loop of individual `columnSorting.sort(sortConfig)` calls. Single-column sort is covered by existing tests but multi-column sort restoration is not. Consider adding a test case to confirm Handsontable's `sort()` accumulates rather than replaces across successive calls after `clearSort()`.
- `package.json:57,72` — Pinning `handsontable` and `@handsontable/angular-wrapper` to exact `18.0.0` (removing `^`) prevents automatic patch updates including potential security fixes. Worth documenting the rationale in release notes or a comment.
- `normalizeSortConfig.ts:13` — The `any` return type is well-justified in the comment. If feasible in a future cleanup, a `type SortConfig = Record<string, unknown>` alias would add marginal type safety without fighting the type system.
**Looks good:**
- Excellent test coverage — `sortedGridRowSync.integration.spec.ts` demonstrates both the failure mode and the fix for each bug against a real Handsontable instance, making the regression value clear.
- The `pasteListener` memory leak fix (storing the reference, removing in `ngOnDestroy`, defensive remove-before-attach) is clean and correct.
- The `$`-substitution fix (string replacement → function replacement `() => replacement`) is the textbook correct approach for `String.replace` with untrusted replacement values.
- The inline sort clear/restore in `editTable()`/`cancelEdit()` is correctly NOT migrated to `updateSettingsSortSafe` — the JSDoc clearly explains why the restore point must be later in the method.
- Null checks on `toVisualRow`/`toPhysicalRow` return values are good defensive programming.
- The cypress e2e test (test 53) verifies the end-to-end sorted-grid edit scenario, including snapshotting all other rows' statuses to prove the edit landed on the correct row.
allan
merged commit f797b1130a into version7-132026-09-03 09:21:57 +00:00
allan
deleted branch version7-13-fix2026-09-03 09:21:57 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
allan
hermes
Intent
A PR review flagged two blocking correctness bugs:
String.replace's special$-pattern handling could silently corrupt a formula'sDC.ORIG_VALUE/DC.USER_NAMEsubstitution whenever the underlying cell data or username contained a literal$, andafterChangewas indexingdataSourcewith Handsontable's visual row instead of translating to physical first, desyncing theEDIT_STATUScell and "overwritten" comment sync on a sorted grid. Manually reproducing the second issue surfaced a third, unrelated Handsontable library bug —updateSettings()corrupts formula cell references (#REF!) whenever a sort is active, independent of what the call actually changes — which needed its own fix to unblock verification.Implementation
$-substitution corruption:substituteBoundedToken'stext.replace(pattern, replacement)used a string replacement, lettingString.replaceinterpret$&/$`/$'/$<digit>in arbitrary cell data. Switched to a function replacement (() => replacement), whose return value is used verbatim — the single shared fix point, sincesubstituteColumnReferences.tscalls the same function.afterChangenow translates the visual row viahot.toPhysicalRow()before callingsyncOverwrittenCommentForCell/updateEditStatusForRow(matchingbeforeChange's existing pattern). Those two methods, whose established contract is a physical row, now translate back to visual viahot.toVisualRow()for every Handsontable/comments-plugin API call — fixing only the call site would have moved the bug rather than closed it.updateSettings()formula corruption: reproduced directly in Karma — a bare{}settings object, called while a sort is active, is enough to corrupt formula cells. A newupdateSettingsSortSafe()wrapper clears the sort, callsupdateSettings(), and restores it immediately after. Applied to everyupdateSettings()call site reachable during an active edit session (editTable,cancelEdit,getPendingExcelPreview,discardPendingExcel,cancelSubmit,checkSave,closeRecordEdit,confirmRecordEdit,applyDisplayColHeaders, grid-resize) except the very first call at initial table load, where nothing can be sorted yet.pastelistener added onhot.rootElementis now stored and removed inngOnDestroy, closing a listener leak the same review flagged.Changes
parseFormulaRule.ts/.spec.ts,substituteColumnReferences.spec.ts— function-replacement fix plus$&/$`/$'/$<digit>regression tests.editor.component.ts—afterChangerow translation,syncOverwrittenCommentForCell/updateEditStatusForRowvisual-row translation, newupdateSettingsSortSafe()wrapper applied across all reachableupdateSettings()call sites,pasteListenerfield + cleanup inngOnDestroy.sortedGridRowSync.integration.spec.ts(new) — reproduces the visual/physical row desync and theupdateSettings-while-sorted formula corruption against real Handsontable + HyperFormula instances, and proves both fixes.editor.cy.ts— test 53: sorts a table, edits the row that moved, assertsDC.ROW_STATUSupdates on the correct row and no other row's status changes.4gl-reviewer test — this is a connectivity check. I will post a full review shortly.
Code Review — 4gl-reviewer
Summary: Three well-reasoned bug fixes (formula
$-substitution corruption, visual/physical row desync on sorted grids, andupdateSettings+sort formula#REF!corruption) backed by thorough integration and e2e tests. High-quality PR.Issues
if (visualRow === null) returnis dead code.Handsontable.toVisualRow()returnsnumber, nevernull. The TypeScript type wouldn't allow this check to ever fire. Not harmful, but misleading — a future reader may think there's a null-return path to handle. Same inupdateEditStatusForRowat line 1982.Suggestions
editor.component.ts —
updateSettingsSortSafe()is used for 7 call sites, buteditTable()(line 1268) andcancelEdit()(line 1379) use an inlineif (sortConfigs.length > 0) columnSorting.clearSort()pattern instead of the wrapper. The inline approach is correct for those cases (the sort is restored later in the method, not immediately after), but a one-line comment inupdateSettingsSortSafenoting "editTable/cancelEdit manage sort clear/restore inline because their restore point is later in the method, not immediately after updateSettings" would prevent a future maintainer from "fixing" the inconsistency.sortedGridRowSync.integration.spec.ts:731 —
const sortConfigs = Array.isArray(sortConfig) ? sortConfig : sortConfig ? [sortConfig] : []duplicatesgetCurrentSortConfigs()logic from editor.component.ts. If that method is exported/testable, the test should use it directly to avoid drift.Looks Good
String.replace→ function-replacement fix (() => replacement) is the correct fix for the$&/$`/$'corruption. Verified:node -econfirms string replacement interprets$&as the matched substring and$`as the preceding text, while function replacement preserves them verbatim.pasteListenercleanup inngOnDestroyfixes a real listener leak (previously attached a new closure per edit session with no removal).updateSettingsSortSafeworkaround for the Handsontable sort+formulas#REF!bug is pragmatic and well-documented.Reviewed by 4gl-reviewer
Applied both suggestions: updateSettingsSortSafe() now documents why editTable()/cancelEdit() manage sort clear/restore inline instead of using it, and the sort-config normalization is extracted into a shared normalizeSortConfig() used by both the component and the test.
On the visualRow === null check: I don't think it's dead code — Handsontable's public .d.ts declares toVisualRow() as returning number, but the underlying IndexMapper.getVisualFromPhysicalIndex() it calls is declared number | null, and this file already has a pre-existing example (rowHeaders/afterGetRowHeader) treating toPhysicalRow()'s return the same way. tsc also compiles clean on the comparison, which it wouldn't if TS considered it unreachable. Keeping the guard.
Code Review — 4gl-reviewer
Summary: Solid PR fixing three real bugs:
$-substitution corruption inparseFormulaRule(string-replace interpreting$&/$1etc.), visual/physical row desync in theafterChangehook and its callee methods on sorted grids, and a HandsontableupdateSettings()-while-sorted formula#REF!corruption. Fixes are correct, well-documented, and well-tested. One inconsistency to address.Issues:
editor.component.ts:1351(cancelEdit) — Still uses the inlineArray.isArray(columnSortConfig) ? columnSortConfig : [columnSortConfig]pattern instead of the newgetCurrentSortConfigs()/normalizeSortConfig()helper. This misses the third branch (falsy/undefined): ifgetSortConfig()returns falsy,sortConfigsbecomes[undefined](length 1), soclearSort()runs (harmless), but the restore loop callscolumnSorting.sort(undefined)which may throw or misbehave.editTable()at line 1264 correctly usesgetCurrentSortConfigs()—cancelEdit()should too.Suggestions:
editor.component.ts:4479,1798,1989— The=== nullguard fortoPhysicalRow/toVisualRowis good, but some Handsontable type definitions also permitundefined. Considerif (row == null)(loose equality) to catch both, for robustness against version differences.editor.component.ts:1349-1352— SincecancelEdit()deliberately can't useupdateSettingsSortSafe(later restore point), consider extracting just thenormalizeSortConfigcall here too, to keep the sort-config normalization in one place even when the full wrapper can't be used.Looks good:
() => replacementfunction-replacement fix inparseFormulaRule.tsis the correct, idiomatic fix forString.replace's$-pattern interpretation. Excellent edge-case test coverage ($&,$',$`,$1).updateSettingsSortSafeis a clean wrapper with a thorough comment explaining whyeditTable()/cancelEdit()can't use it (their restore point is later). Good separation of concerns.sortedGridRowSync.integration.spec.ts) using a real Handsontable + multiColumnSorting instance to demonstrate both the buggy and fixed behavior is exactly the right testing strategy — unit mocks couldn't prove visual/physical divergence.this.pasteListenerfor properngOnDestroycleanup is correct.Code Review — 4gl-reviewer
Summary: Solid, well-tested fix for two genuine bugs: visual/physical row-index confusion in the afterChange → EDIT_STATUS/overwritten-comment sync path on sorted grids, and a HOT multiColumnSorting+formulas
#REF!corruption triggered byupdateSettings()while sorted. ThesubstituteBoundedToken$-escaping fix is correct and well-covered.Issues:
editor.component.ts(cancelEdit, ~L1350) —cancelEdit()inlinesArray.isArray(columnSortConfig) ? columnSortConfig : [columnSortConfig]instead of reusingthis.getCurrentSortConfigs()/normalizeSortConfig(). IfgetSortConfig()returnsundefined(no active sort), this yields[undefined], so the guardif (sortConfigs.length > 0) columnSorting.clearSort()passes (length 1) and the restore loop callscolumnSorting.sort(undefined).editTable()was already migrated to the defensive helper;cancelEdit()should be too, for both consistency and correctness.editor.component.ts(cancelEdit, ~L1350) — same call callscolumnSorting.getSortConfig()directly with no try/catch, butgetCurrentSortConfigs()was specifically written to wrap that call because it throws when the plugin's internal state is null (VA embed / early load).cancelEdit()is now the one un-guarded caller of the same throwing path.editor.component.ts(cancelEdit, ~L1376-1382) —getFormulaCellsToPreserveOnCancel's callbacks pass the iterating physicalrowIndexstraight intocommentsPlugin.getCommentAtCell(rowIndex, ...)andhot.getDataAtRowProp(rowIndex, prop)— both HOT APIs expect a visual row. On a sorted grid this reads the wrong cell, the same class of bug this PR fixes insyncOverwrittenCommentForCell/updateEditStatusForRow. Pre-existing, but since this PR is specifically about that translation it's worth closing here.Suggestions:
editor.component.ts(cancelEdit) — replace the inline normalization withconst sortConfigs = this.getCurrentSortConfigs()so it gets theundefined→[]handling and the try/catch guard for free.editor.component.ts(updateSettingsSortSafe) — considertry/finallyaroundhot.updateSettings(settings, render)so a thrown settings error still restores the sort (otherwise the grid is left unsorted on failure).editor.component.ts(afterChange hook) — the newconst row = hot.toPhysicalRow(visualRow); if (row === null) continueis good; consider logging/skipping silently is fine, but notechangedRows.add(null)is avoided by the guard — verifychangedRowsis typedSet<number>so a stray null can't slip in elsewhere.Looks good:
substituteBoundedTokenswitch from string to function replacement (() => replacement) is the correct fix for the$&/$`/$'/$1corruption — and the new spec cases (a$&b,a$'b,a$`b,a$1b) lock it in.normalizeSortConfigextraction is clean, and the integration spec (sortedGridRowSync.integration.spec.ts) reproduces all three failure modes (afterChange row, EDIT_STATUS write, comment placement, and the#REF!corruption) against a real HOT instance — exactly the right level of proof.pasteListenerstored on the instance and removed inngOnDestroyis a clean listener-leak fix.Code Review — 4gl-reviewer
Summary: Well-executed PR fixing three distinct but related bugs in the editor:
$-substitution corruption in formula rules, visual/physical row desync on sorted grids, and formula#REF!corruption fromupdateSettings()during active sort. Excellent test coverage and documentation throughout.Issues:
editor.component.ts:1228] —updateSettingsSortSafeclears sort beforeupdateSettingsand restores after, but ifupdateSettingsthrows, the sort is lost (not restored). Atry/finallywrapping theupdateSettingscall would guarantee sort restoration even on exception.Suggestions:
editor.component.ts:3642] — The resize observer'supdateSettingsSortSafe({ height }, false)will clear/restore sort on every window resize while sorted. Since therender: falseflag signals an intent to avoid unnecessary rendering, the forced sort clear/restore partially defeats that. Consider whether the height-only path truly needs the wrapper or could guard onsortConfigs.length > 0 && hasFormulaColumnsto skip the overhead for non-formula grids. Minor — correctness is fine, just a performance consideration for large sorted grids during rapid resize.normalizeSortConfig.ts:9] — Usinganyfor both param and return is pragmatic given SortConfig isn't publicly exported, but a generic signatureexport const normalizeSortConfig = <T>(cfg: T | T[] | undefined): T[] => ...would preserve type info for callers without needing the deep import.Looks good:
() => replacementfunction-replacement fix insubstituteBoundedTokenis the correct approach — string replacement's$&/$`/$'/$<n>interpretation is a classic footgun, and the test cases for each special pattern are thorough.sortedGridRowSync.integration.spec.ts) is exemplary: eachdescribeblock demonstrates the failure mode first, then the fix, against a real Handsontable + multiColumnSorting instance. This proves the bugs are real and the fixes close them, not just that unsorted grids still work.editTable()/cancelEdit()with inline clear/restore (rather than using the wrapper) is correctly justified — their restore point is later in the method, and the comment explicitly warns against "simplifying" them.ngOnDestroyproperly prevents the accumulated-listener leak.quoteLiteralalready doubles embedded"characters, so the function-replacement fix handles the full round-trip for arbitrary user data correctly.Code Review — 4gl-reviewer
Summary: Well-crafted fix for three interrelated Handsontable issues: formula
$-substitution corruption viaString.replace, sorted-grid visual/physical row desync inafterChange/updateEditStatusForRow/syncOverwrittenCommentForCell, andupdateSettingscorrupting formula refs while a sort is active. Thorough test coverage including integration tests against real Handsontable instances.Issues:
this.pasteListeneris reassigned on each call without removing the previous listener. If the registration code runs more than once (e.g. switching tables mid-session), the old listener leaks since only the latest reference is stored. Add a guard:if (this.pasteListener) { hot.rootElement.removeEventListener('paste', this.pasteListener) }before re-assigning, or guard registration withif (!this.pasteListener).Suggestions:
any[]return type loses type safety. ConsiderRecord<string, unknown>[]at minimum, or the Handsontable sort config type if reachable.fast-uridevDependency addition andreaddir-globnested override seem unrelated to the formula/sort bug fixes. Consider splitting into a separate PR for cleaner history.handsontableto exact18.0.0(removing caret) is reasonable but a brief comment explaining why would help future maintainers understand the lock.Looks good:
updateSettingsSortSafewrapper is a clean abstraction, and the comment explaining whyeditTable()/cancelEdit()deliberately don't use it (different restore point) prevents future "simplification" regressions.$-substitution fix (string → function replacement insubstituteBoundedToken) is the correct, minimal change with targeted test cases for each special$pattern ($&,$`,$',$1).Code Review — 4gl-reviewer
Summary: Solid fix for two real bugs — formula
$-substitution corruption viaString.replacepattern interpretation, and sorted-grid visual/physical row desync causing wrong-cell writes. Excellent test coverage demonstrating both failure modes and fixes. Two consistency issues incancelEdit()worth addressing.Issues:
cancelEdit()inlinesArray.isArray(columnSortConfig) ? columnSortConfig : [columnSortConfig]instead of callingthis.getCurrentSortConfigs(). WhengetSortConfig()returnsundefined(no sort active), this produces[undefined](length 1), which triggersclearSort()(harmless) but then callscolumnSorting.sort(undefined)in the restore loop at line 1414 — potentially throwing or behaving unpredictably.editTable()correctly usesgetCurrentSortConfigs()which normalizes to[]vianormalizeSortConfig. These should be consistent.getFormulaCellsToPreserveOnCancelcallbacks (getCommentAtCell(rowIndex, ...)andgetDataAtRowProp(rowIndex, prop)) run beforeclearSort()is called (line 1393), so the sort is still active. The function iterates0..dataSource.length(physical indices), but both Handsontable APIs expect visual rows. On a sorted grid, the wrong cells would be checked for comments/values — the same class of desync bug this PR fixes inafterChange/updateEditStatusForRow/syncOverwrittenCommentForCell. Consider translating viahot.toVisualRow(rowIndex)in the callbacks, or movingclearSort()before this block.Suggestions:
anyreturn type is understandable givenSortConfigisn't publicly exported in this Handsontable version, but consider a structural type (e.g.{ column: number; sortOrder: string }[]) for minimal type safety on the fields actually used by callers.updateSettingsSortSafecallscolumnSorting.clearSort()without checking if the plugin is initialized.getCurrentSortConfigs()already handles the null-plugin case defensively, butcolumnSorting.clearSort()itself would throw if the plugin is null. Consider a null-check or try/catch matching the defensive style ofgetCurrentSortConfigs().Looks good:
substituteBoundedTokenfix — switching from string to function replacement (() => replacement) — is the correct way to preventString.replacefrom interpreting$&/$`/$'/$<digit>in arbitrary user data. The spec tests covering$&,$',$`, and$1are thorough.updateSettingsSortSafeis a clean abstraction for the Handsontable sort+formulas#REF!corruption workaround, with an excellent comment explaining whyeditTable()/cancelEdit()can't use it directly.sortedGridRowSync.integration.spec.ts) is outstanding — it reproduces each failure mode against a real Handsontable instance, then proves the fix, rather than mocking.removeEventListenercleanup inngOnDestroyprevents listener leaks across repeated edit sessions.Code Review — 4gl-reviewer
Summary: Well-structured fix for two real Handsontable integration bugs: formula
$-substitution corruption viaString.replace's special pattern interpretation, and sorted-grid row/formula desync caused by visual/physical row confusion. The integration spec and cypress test provide solid coverage of the failure modes.Issues:
cancelEdit(), thegetFormulaCellsToPreserveOnCancelcallbacks callcommentsPlugin.getCommentAtCell(rowIndex, ...)andhot.getDataAtRowProp(rowIndex, prop)beforeclearSort()runs (line 1399), while the sort is still active.rowIndexhere iterates 0..dataSource.length as a physical index, butgetCommentAtCell/getDataAtRowPropexpect visual rows. On a sorted grid these calls will read the wrong cell/comment. The sort clear happens 14 lines later (aftertoPreserveis already computed). Consider moving theclearSort()call above thegetFormulaCellsToPreserveOnCancelblock, or translatingrowIndexto visual within the callbacks.Suggestions:
cancelEdit()still uses the inline normalization (Array.isArray(columnSortConfig) ? columnSortConfig : [columnSortConfig]) instead of the newnormalizeSortConfighelper, and doesn't handle theundefinedcase (if no sort is active,getSortConfig()returnsundefined,[undefined]won't be empty, soclearSort()runs unnecessarily). Consider usingthis.getCurrentSortConfigs()likeeditTable()does, which delegates tonormalizeSortConfigand handlesundefined.Looks good:
substituteBoundedTokenfix (using a function replacement to avoid$&/$\``/$'/$` corruption) is correct and well-documented, with targeted spec coverage for each special pattern.updateSettingsSortSafewrapper is a clean, well-commented abstraction, and the explicit note about whyeditTable/cancelEditdon't use it (their restore point is later) shows good awareness of the control flow.ngOnDestroyproperly prevents the listener leak, and removing any previous listener before re-registering is a nice defensive touch.handsontableto exact18.0.0with the explanatory comment about version-dependent workarounds is a pragmatic choice.Code Review — 4gl-reviewer
Summary: Well-executed PR fixing three interrelated bugs:
$-substitution corruption in formula parsing, visual/physical row index desync on sorted grids, and formula#REF!corruption whenupdateSettingsruns during an active sort. The fixes are correct, thoroughly tested with integration specs demonstrating both failure modes and fixes, and well-documented.Issues:
Suggestions:
editor.component.ts(updateSettingsSortSafe, ~line 1218) — Multi-column sort restoration uses a loop of individualcolumnSorting.sort(sortConfig)calls. Single-column sort is covered by existing tests but multi-column sort restoration is not. Consider adding a test case to confirm Handsontable'ssort()accumulates rather than replaces across successive calls afterclearSort().package.json:57,72— Pinninghandsontableand@handsontable/angular-wrapperto exact18.0.0(removing^) prevents automatic patch updates including potential security fixes. Worth documenting the rationale in release notes or a comment.normalizeSortConfig.ts:13— Theanyreturn type is well-justified in the comment. If feasible in a future cleanup, atype SortConfig = Record<string, unknown>alias would add marginal type safety without fighting the type system.Looks good:
sortedGridRowSync.integration.spec.tsdemonstrates both the failure mode and the fix for each bug against a real Handsontable instance, making the regression value clear.pasteListenermemory leak fix (storing the reference, removing inngOnDestroy, defensive remove-before-attach) is clean and correct.$-substitution fix (string replacement → function replacement() => replacement) is the textbook correct approach forString.replacewith untrusted replacement values.editTable()/cancelEdit()is correctly NOT migrated toupdateSettingsSortSafe— the JSDoc clearly explains why the restore point must be later in the method.toVisualRow/toPhysicalRowreturn values are good defensive programming.