_____DELETE__THIS__RECORD_____ (the editor's Delete? column) is now created by the frontend, and editors/getdata no longer sends it.
Why
getdata.sas synthesised the column itself, in four places:
where
what it did
the two filter data steps
_____DELETE__THIS__RECORD_____="No"; on every row
the outdata build
hoisted it in front of the PKs, attribed it, set it to "No "
the column metadata
ctrlOptions='["No","Yes"]' for it
maxvarlengths
forced MAXLEN=3
In the editor that column is a constant - it is only ever "No" on load. The only part that mattered, the Yes/No dropdown rule, is already injected client-side (deleteRecordColumnRule.ts, since #253's vertical-array fix), because %mp_getcols never has a row for it. So the backend was sending a column the client already owns, and its value was never read.
The column is meaningful elsewhere - postdata / getstagetable carry real Yes values, and file upload legitimately accepts the name (excel.md, files.md). None of that changes.
Changes
SAS - sas/sasjs/services/editors/getdata.sas
No longer emits the column: the two data-step assignments, the outdata hoist, the ctrlOptions branch and the maxvarlengths override are gone. (With the delete column gone, ctrlOptions had no other producer in this service, so the whole plumbing went with it.)
Frontend - deleteRecordColumn.ts (new)
prepareDeleteRecordColumn(response) asserts the response does not carry the name, then prepends it to COLHEADERS. Everything downstream (DcValidator's rules, columnHeader, headerArray) is keyed off COLHEADERS order, so the delete column is still index 0 and the grid is unchanged.
seedDeleteRecordColumn(rows) sets every row to 'No' before the validator or HyperFormula sees it.
A response that does carry the name throws, rather than rendering a table whose delete column would shadow a real column of that name. The throw lands in the existing getdata.catch, which sets getdataError and shows the error state - so the failure is visible, not silent.
multi-dataset.component.ts builds its own DcValidator from the same responses, so it goes through the same helper.
Mocks - the server and sas9 getdata mocks are updated to the new contract. postdata, getstagetable and makedata are untouched.
Screenshot
The editor, on the mocked estate, with the client-owned Delete? column first and its dropdown open. The spec asserts the header text and that the dropdown contains exactly No / Yes before it shoots.
Testing
client unit tests 570/570 (npm run test:headless), including 7 new cases for deleteRecordColumn.ts
npm run build (production) clean
sasjs lint clean on the changed SAS; sasjs compile -t server succeeds
Cypress against the mocked estate: coltype-delete-record.cy.ts2/2, editor.cy.ts55/55, excel.cy.ts23/23, row-cell-limits.cy.ts5/5
coltype-delete-record.cy.ts is the direct regression guard: it opens the editor on MPE_X_TEST and asserts the first column renders its hardcoded Yes/No dropdown end to end.
Deploy notes
The name is reserved. A table that contains a real column called _____DELETE__THIS__RECORD_____ will now fail to open in the editor, with the error state rather than a grid. That is the intended behaviour - the name would otherwise shadow the delete flag - and the docs PR adds the note.
No migration and no configuration change. The response is one column smaller, and the grid is identical.
Docs: docs.datacontroller.io PR updates the Developer Guide's pre-edit hook example (which mirrored the old data step) and adds the reserved-name note.
Closes #309.
## What
`_____DELETE__THIS__RECORD_____` (the editor's **Delete?** column) is now created by the frontend, and `editors/getdata` no longer sends it.
## Why
`getdata.sas` synthesised the column itself, in four places:
| where | what it did |
|---|---|
| the two filter data steps | `_____DELETE__THIS__RECORD_____="No";` on every row |
| the `outdata` build | hoisted it in front of the PKs, `attrib`ed it, set it to `"No "` |
| the column metadata | `ctrlOptions='["No","Yes"]'` for it |
| `maxvarlengths` | forced `MAXLEN=3` |
In the editor that column is a **constant** - it is only ever `"No"` on load. The only part that mattered, the Yes/No dropdown rule, is already injected client-side (`deleteRecordColumnRule.ts`, since #253's vertical-array fix), because `%mp_getcols` never has a row for it. So the backend was sending a column the client already owns, and its value was never read.
The column *is* meaningful elsewhere - `postdata` / `getstagetable` carry real `Yes` values, and file upload legitimately accepts the name (`excel.md`, `files.md`). None of that changes.
## Changes
**SAS - `sas/sasjs/services/editors/getdata.sas`**
- No longer emits the column: the two data-step assignments, the `outdata` hoist, the `ctrlOptions` branch and the `maxvarlengths` override are gone. (With the delete column gone, `ctrlOptions` had no other producer in this service, so the whole plumbing went with it.)
**Frontend - `deleteRecordColumn.ts` (new)**
- `prepareDeleteRecordColumn(response)` asserts the response does **not** carry the name, then prepends it to `COLHEADERS`. Everything downstream (`DcValidator`'s rules, `columnHeader`, `headerArray`) is keyed off `COLHEADERS` order, so the delete column is still index 0 and the grid is unchanged.
- `seedDeleteRecordColumn(rows)` sets every row to `'No'` before the validator or HyperFormula sees it.
- A response that *does* carry the name **throws**, rather than rendering a table whose delete column would shadow a real column of that name. The throw lands in the existing `getdata` `.catch`, which sets `getdataError` and shows the error state - so the failure is visible, not silent.
- `multi-dataset.component.ts` builds its own `DcValidator` from the same responses, so it goes through the same helper.
**Mocks** - the server and sas9 `getdata` mocks are updated to the new contract. `postdata`, `getstagetable` and `makedata` are untouched.
## Screenshot
The editor, on the mocked estate, with the client-owned **Delete?** column first and its dropdown open. The spec asserts the header text and that the dropdown contains exactly `No` / `Yes` before it shoots.

## Testing
- client unit tests **570/570** (`npm run test:headless`), including 7 new cases for `deleteRecordColumn.ts`
- `npm run build` (production) clean
- `sasjs lint` clean on the changed SAS; `sasjs compile -t server` succeeds
- Cypress against the mocked estate: `coltype-delete-record.cy.ts` **2/2**, `editor.cy.ts` **55/55**, `excel.cy.ts` **23/23**, `row-cell-limits.cy.ts` **5/5**
`coltype-delete-record.cy.ts` is the direct regression guard: it opens the editor on `MPE_X_TEST` and asserts the first column renders its hardcoded Yes/No dropdown end to end.
## Deploy notes
- **The name is reserved.** A table that contains a real column called `_____DELETE__THIS__RECORD_____` will now fail to open in the editor, with the error state rather than a grid. That is the intended behaviour - the name would otherwise shadow the delete flag - and the docs PR adds the note.
- No migration and no configuration change. The response is one column smaller, and the grid is identical.
- Docs: `docs.datacontroller.io` PR updates the Developer Guide's pre-edit hook example (which mirrored the old data step) and adds the reserved-name note.
Reviewed the full diff at 56025cc (the editor takes ownership of the delete-record column; getdata.sas and the mocks stop emitting it), the head tree for the stagedata/mpe_loader positional contract, and the PR CI state.
client/src/app/multi-dataset/multi-dataset.component.ts:1084 - prepareDeleteRecordColumn(response) throws inside buildDatasetsObjects(), which runs in the Promise.allSettled(...).then(...) callback at line 359 with no .catch anywhere on that chain (the only .catch in the block, line 419, is the per-dataset excel-parse one). The new module's own doc comment says callers must surface the throw in a promise-chain .catch, and the editor does (editor.component.ts:3527 sets getdataError), but in the multi-dataset flow a getdata response carrying the reserved column becomes an unhandled promise rejection: the loop aborts, parsedDatasets stays empty, and no error state or modal is shown. Add a .catch to the allSettled chain (or wrap the buildDatasetsObjects call) that surfaces the error the way fetchDataset failures are surfaced.
client/src/app/multi-dataset/multi-dataset.component.ts:1111 - this rename block still uses the literal '_____DELETE__THIS__RECORD_____' while the same PR moves the identical block in editor.component.ts:3945 to DELETE_RECORD_COLUMN_NAME. Use the exported constant here too so the reserved name has a single definition.
2 findings above for review.
Reviewed the full diff at 56025cc (the editor takes ownership of the delete-record column; getdata.sas and the mocks stop emitting it), the head tree for the stagedata/mpe_loader positional contract, and the PR CI state.
- client/src/app/multi-dataset/multi-dataset.component.ts:1084 - `prepareDeleteRecordColumn(response)` throws inside `buildDatasetsObjects()`, which runs in the `Promise.allSettled(...).then(...)` callback at line 359 with no `.catch` anywhere on that chain (the only `.catch` in the block, line 419, is the per-dataset excel-parse one). The new module's own doc comment says callers must surface the throw in a promise-chain `.catch`, and the editor does (editor.component.ts:3527 sets `getdataError`), but in the multi-dataset flow a getdata response carrying the reserved column becomes an unhandled promise rejection: the loop aborts, `parsedDatasets` stays empty, and no error state or modal is shown. Add a `.catch` to the allSettled chain (or wrap the `buildDatasetsObjects` call) that surfaces the error the way `fetchDataset` failures are surfaced.
- client/src/app/multi-dataset/multi-dataset.component.ts:1111 - this rename block still uses the literal `'_____DELETE__THIS__RECORD_____'` while the same PR moves the identical block in editor.component.ts:3945 to `DELETE_RECORD_COLUMN_NAME`. Use the exported constant here too so the reserved name has a single definition.
2 findings above for review.
1. Unhandled rejection in the multi-dataset flow - wrapped the buildDatasetsObjects(datasets) call in a try/catch rather than adding a .catch to the allSettled chain. Chaining after that .then() re-indented its whole 60-line body (142 changed lines for a two-line fix); the wrap confines the diff to the call site. On a throw it logs and drops the parse, the same as fetchDataset's own .catch does for a failed getdata.
2. The name constant - the rename-to-Delete? block in multi-dataset.component.ts now uses DELETE_RECORD_COLUMN_NAME, matching the same block in editor.component.ts.
Re-verified after the change: client unit tests 570/570, npm run build clean. The full Cypress suite re-runs on this head in CI.
On the remaining literal occurrences of _____DELETE__THIS__RECORD_____ in these two components (the Excel-preview default, the submit-time reorder and the cell-change handler): those predate this PR and are untouched, so the diff stays on the sites the change actually moves. Happy to sweep them to the constant in a follow-up if you would rather the name had exactly one definition everywhere.
Both addressed in 2b8a2fa.
**1. Unhandled rejection in the multi-dataset flow** - wrapped the `buildDatasetsObjects(datasets)` call in a `try`/`catch` rather than adding a `.catch` to the `allSettled` chain. Chaining after that `.then()` re-indented its whole 60-line body (142 changed lines for a two-line fix); the wrap confines the diff to the call site. On a throw it logs and drops the parse, the same as `fetchDataset`'s own `.catch` does for a failed getdata.
**2. The name constant** - the rename-to-`Delete?` block in `multi-dataset.component.ts` now uses `DELETE_RECORD_COLUMN_NAME`, matching the same block in `editor.component.ts`.
Re-verified after the change: client unit tests 570/570, `npm run build` clean. The full Cypress suite re-runs on this head in CI.
On the remaining literal occurrences of `_____DELETE__THIS__RECORD_____` in these two components (the Excel-preview default, the submit-time reorder and the cell-change handler): those predate this PR and are untouched, so the diff stays on the sites the change actually moves. Happy to sweep them to the constant in a follow-up if you would rather the name had exactly one definition everywhere.
Re-reviewed the full diff at 2b8a2fa against the previous review at 56025cc (the editor takes ownership of the delete-record column; getdata.sas and the mocks stop emitting it). Scope of this pass: the delta commit (surfacing the reserved-column guard in the multi-dataset flow, using the name constant) plus a re-read of the full base...head diff against the head tree. Both previous findings (the unhandled rejection from buildDatasetsObjects in the allSettled chain, and the literal reserved name in the rename block) are addressed at this head and not re-raised. Verified against the head tree: the mpe_targetloader/bitemporal_dataloader macros that consume the delete flag on submit are untouched by this PR; stagedata.sas still synthesizes its own delete column server-side; the SAS delta leaves no dangling ctrloptions reference (the vars3 keep-list was updated in step with the ctrlOptions block's removal); the gitleaks pre-commit hook, .npmrc (ignore-scripts + save-exact) and the strict npm audit steps all stand.
client/src/app/multi-dataset/multi-dataset.component.ts:377 - the new catch logs and returns, but unlike fetchDataset's failures (which resolve to undefined and let the remaining datasets parse) it drops the whole parse with nothing shown to the user: uploadLoading is already false, parsedDatasets stays empty, and the only trace is a console.warn. A response carrying the reserved column means a real table column of that name - the editor flow refuses the load with an on-screen error state (editor.component.ts:3529 sets getdataError), so this flow should surface it the same way (eventService.showInfoModal is the pattern this file already uses elsewhere) rather than silently showing an empty "select tables" state with the file still attached.
client/src/app/editor/editor.component.ts:3894 - prepareDeleteRecordColumn(response) throws inside initSetup, and the comment at 3892 says "A throw here lands in the caller's .catch" - that only holds on the direct path. initSetup retries via setTimeout(() => this.initSetup(response, attempt + 1), 50) (line 3885) when hotInstance is not ready yet, and a throw from that retry runs outside the promise chain: the .catch at 3523 is not re-entered, so the reserved-column refusal becomes an unhandled rejection and the user is left on the loading spinner (getdataError never set, because tableTrue is only assigned at 3928, after the throw point). Guard for it: set this.getdataError = true (or route through the same catch) before rethrowing inside the retry branch too, so the refusal is surfaced whichever attempt it lands on.
2 findings above for review.
Re-reviewed the full diff at 2b8a2fa against the previous review at 56025cc (the editor takes ownership of the delete-record column; getdata.sas and the mocks stop emitting it). Scope of this pass: the delta commit (surfacing the reserved-column guard in the multi-dataset flow, using the name constant) plus a re-read of the full base...head diff against the head tree. Both previous findings (the unhandled rejection from buildDatasetsObjects in the allSettled chain, and the literal reserved name in the rename block) are addressed at this head and not re-raised. Verified against the head tree: the mpe_targetloader/bitemporal_dataloader macros that consume the delete flag on submit are untouched by this PR; stagedata.sas still synthesizes its own delete column server-side; the SAS delta leaves no dangling ctrloptions reference (the vars3 keep-list was updated in step with the ctrlOptions block's removal); the gitleaks pre-commit hook, .npmrc (ignore-scripts + save-exact) and the strict npm audit steps all stand.
- client/src/app/multi-dataset/multi-dataset.component.ts:377 - the new catch logs and returns, but unlike fetchDataset's failures (which resolve to undefined and let the remaining datasets parse) it drops the whole parse with nothing shown to the user: uploadLoading is already false, parsedDatasets stays empty, and the only trace is a console.warn. A response carrying the reserved column means a real table column of that name - the editor flow refuses the load with an on-screen error state (editor.component.ts:3529 sets getdataError), so this flow should surface it the same way (eventService.showInfoModal is the pattern this file already uses elsewhere) rather than silently showing an empty "select tables" state with the file still attached.
- client/src/app/editor/editor.component.ts:3894 - `prepareDeleteRecordColumn(response)` throws inside initSetup, and the comment at 3892 says "A throw here lands in the caller's .catch" - that only holds on the direct path. initSetup retries via `setTimeout(() => this.initSetup(response, attempt + 1), 50)` (line 3885) when hotInstance is not ready yet, and a throw from that retry runs outside the promise chain: the .catch at 3523 is not re-entered, so the reserved-column refusal becomes an unhandled rejection and the user is left on the loading spinner (getdataError never set, because tableTrue is only assigned at 3928, after the throw point). Guard for it: set this.getdataError = true (or route through the same catch) before rethrowing inside the retry branch too, so the refusal is surfaced whichever attempt it lands on.
2 findings above for review.
1. The retry path in initSetup - you are right that a throw from the setTimeout retry is outside the promise chain. The retry call is now wrapped and routes through a new private handleGetdataFailure(err), which is also what the callService.catch now calls - so the refusal sets getdataError/tableTrue and shows the error state whichever attempt it lands on.
2. The multi-dataset catch - now surfaces eventService.showInfoModal('Upload error', err.message), the pattern this file already uses for the other upload problems, instead of logging and returning.
Re-verified after the change: client unit tests 570/570, npm run build clean. The Cypress suite re-runs on this head.
One note on the previous head: Build-and-test-development went red on 2b8a2fa, on excel.cy.ts test 18 (openTableFromTree waiting on .app-loading), which the same spec passed on 56025cc. It is the home-page boot spinner, not the upload path this PR touches, and the neighbouring tests in the same run passed - I have re-run the job. If it fails again on this head I will treat it as real and dig in.
Both addressed in b26f557.
**1. The retry path in `initSetup`** - you are right that a throw from the `setTimeout` retry is outside the promise chain. The retry call is now wrapped and routes through a new private `handleGetdataFailure(err)`, which is also what the `callService` `.catch` now calls - so the refusal sets `getdataError`/`tableTrue` and shows the error state whichever attempt it lands on.
**2. The multi-dataset catch** - now surfaces `eventService.showInfoModal('Upload error', err.message)`, the pattern this file already uses for the other upload problems, instead of logging and returning.
Re-verified after the change: client unit tests 570/570, `npm run build` clean. The Cypress suite re-runs on this head.
One note on the previous head: `Build-and-test-development` went red on 2b8a2fa, on `excel.cy.ts` test 18 (`openTableFromTree` waiting on `.app-loading`), which the same spec passed on 56025cc. It is the home-page boot spinner, not the upload path this PR touches, and the neighbouring tests in the same run passed - I have re-run the job. If it fails again on this head I will treat it as real and dig in.
Re-reviewed the full base...head diff at b26f557, focused on the delta commit (surfacing the reserved-column refusal on every getdata path) plus a re-read of the earlier commits against the head tree. Both findings from the review at 2b8a2fa are addressed at this head and not re-raised: the initSetup retry branch (editor.component.ts:3899) now wraps the recursive call in a try/catch routed through handleGetdataFailure, so a throw from any retry attempt lands in the error state instead of an unhandled rejection; and the multi-dataset catch (multi-dataset.component.ts:380) now surfaces the refusal on screen via eventService.showInfoModal rather than only a console.warn. Verified in the delta and head tree: initSetup is synchronous so the try/catch spans the retry correctly; the cypress childNodes[N+1] position math matches the new mock COLHEADERS with the client-prepended column; the %length(&pk) guard around the attrib statement is needed now that the delete name no longer pads it.
No blocking issues.
Re-reviewed the full base...head diff at b26f557, focused on the delta commit (surfacing the reserved-column refusal on every getdata path) plus a re-read of the earlier commits against the head tree. Both findings from the review at 2b8a2fa are addressed at this head and not re-raised: the initSetup retry branch (editor.component.ts:3899) now wraps the recursive call in a try/catch routed through handleGetdataFailure, so a throw from any retry attempt lands in the error state instead of an unhandled rejection; and the multi-dataset catch (multi-dataset.component.ts:380) now surfaces the refusal on screen via eventService.showInfoModal rather than only a console.warn. Verified in the delta and head tree: initSetup is synchronous so the try/catch spans the retry correctly; the cypress childNodes[N+1] position math matches the new mock COLHEADERS with the client-prepended column; the %length(&pk) guard around the attrib statement is needed now that the delete name no longer pads it.
No blocking issues.
getdata.sas synthesised _____DELETE__THIS__RECORD_____ itself - it added
the column to work.out, hoisted it in front of the PKs, set it to "No" on
every row, gave it a No/Yes dropdown in the metadata and forced MAXLEN=3
in maxvarlengths. In the editor that column is a constant: it is only ever
"No" on load, and its Yes/No rule is already injected client-side
(deleteRecordColumnRule.ts, since #253's vertical-array fix). So the
backend was sending a column the client already owns.
The editor now creates the column, the same way EDIT_STATUS / dc.row_status
is client-only:
- getdata.sas no longer emits it (data steps, the outdata hoist, the
ctrlOptions dropdown, the maxvarlengths override).
- deleteRecordColumn.ts asserts the response does NOT carry the name, then
prepends it to COLHEADERS. Everything downstream (DcValidator's rules,
columnHeader, headerArray) is keyed off COLHEADERS order, so the delete
column is still index 0 and the grid is unchanged.
- Every row is seeded to 'No' before the validator or HyperFormula sees it.
- A response that does carry the name now throws instead of rendering a
table whose delete column would shadow a real column of that name. The
throw lands in the existing getdata .catch, which sets getdataError and
shows the error state. The name is reserved; the docs note is in the
sibling docs PR.
The multi-dataset loader (multi-dataset.component.ts) builds its own
DcValidator from the same responses, so it goes through the same helper.
Mocks updated to the new contract (server + sas9 getdata); postdata,
getstagetable and makedata are untouched - they consume or seed the flag
legitimately, and the upload contract in excel.md / files.md is unchanged.
Verified: client unit tests 570/570, npm run build clean, sasjs lint clean
on the changed SAS, sasjs compile -t server succeeds, and against the
mocked estate coltype-delete-record.cy.ts 2/2, editor.cy.ts 55/55,
excel.cy.ts 23/23 and row-cell-limits.cy.ts 5/5.
Review on #343:
- buildDatasetsObjects() runs inside the Promise.allSettled(...).then(...)
chain in onStartParsingFile, which had no .catch. prepareDeleteRecordColumn
throws on a response carrying the reserved delete-record column, so in this
flow that became an unhandled rejection: the loop aborted, no dataset was
parsed and no state was shown. Wrap the call and drop the parse, the way
fetchDataset's own catch handles a failed getdata.
- The rename-to-Delete? block still used the literal name while the same
block in editor.component.ts uses DELETE_RECORD_COLUMN_NAME. Use the
constant here too.
Review on #343:
- initSetup's retry branch called itself from a bare setTimeout, outside the
callService promise chain, so the reserved-column throw on a retry never
reached the .catch: getdataError was never set and the user was left on the
loading spinner. Wrap the retry call and route it through the same failure
handler the .catch uses (now a private handleGetdataFailure, shared by
both).
- The multi-dataset catch logged and returned, leaving an empty table
selection with the file still attached and only a console.warn. Surface it
with eventService.showInfoModal, the pattern this file already uses for
other upload problems.
allan
merged commit d6e0662b16 into feat/release-7.182026-10-16 13:01:43 +00:00
allan
deleted branch fix/getdata-delete-column-client-side2026-10-16 13:01:43 +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.
Closes #309.
What
_____DELETE__THIS__RECORD_____(the editor's Delete? column) is now created by the frontend, andeditors/getdatano longer sends it.Why
getdata.sassynthesised the column itself, in four places:_____DELETE__THIS__RECORD_____="No";on every rowoutdatabuildattribed it, set it to"No "ctrlOptions='["No","Yes"]'for itmaxvarlengthsMAXLEN=3In the editor that column is a constant - it is only ever
"No"on load. The only part that mattered, the Yes/No dropdown rule, is already injected client-side (deleteRecordColumnRule.ts, since #253's vertical-array fix), because%mp_getcolsnever has a row for it. So the backend was sending a column the client already owns, and its value was never read.The column is meaningful elsewhere -
postdata/getstagetablecarry realYesvalues, and file upload legitimately accepts the name (excel.md,files.md). None of that changes.Changes
SAS -
sas/sasjs/services/editors/getdata.sasoutdatahoist, thectrlOptionsbranch and themaxvarlengthsoverride are gone. (With the delete column gone,ctrlOptionshad no other producer in this service, so the whole plumbing went with it.)Frontend -
deleteRecordColumn.ts(new)prepareDeleteRecordColumn(response)asserts the response does not carry the name, then prepends it toCOLHEADERS. Everything downstream (DcValidator's rules,columnHeader,headerArray) is keyed offCOLHEADERSorder, so the delete column is still index 0 and the grid is unchanged.seedDeleteRecordColumn(rows)sets every row to'No'before the validator or HyperFormula sees it.getdata.catch, which setsgetdataErrorand shows the error state - so the failure is visible, not silent.multi-dataset.component.tsbuilds its ownDcValidatorfrom the same responses, so it goes through the same helper.Mocks - the server and sas9
getdatamocks are updated to the new contract.postdata,getstagetableandmakedataare untouched.Screenshot
The editor, on the mocked estate, with the client-owned Delete? column first and its dropdown open. The spec asserts the header text and that the dropdown contains exactly
No/Yesbefore it shoots.Testing
npm run test:headless), including 7 new cases fordeleteRecordColumn.tsnpm run build(production) cleansasjs lintclean on the changed SAS;sasjs compile -t serversucceedscoltype-delete-record.cy.ts2/2,editor.cy.ts55/55,excel.cy.ts23/23,row-cell-limits.cy.ts5/5coltype-delete-record.cy.tsis the direct regression guard: it opens the editor onMPE_X_TESTand asserts the first column renders its hardcoded Yes/No dropdown end to end.Deploy notes
_____DELETE__THIS__RECORD_____will now fail to open in the editor, with the error state rather than a grid. That is the intended behaviour - the name would otherwise shadow the delete flag - and the docs PR adds the note.docs.datacontroller.ioPR updates the Developer Guide's pre-edit hook example (which mirrored the old data step) and adds the reserved-name note.Reviewed the full diff at
56025cc(the editor takes ownership of the delete-record column; getdata.sas and the mocks stop emitting it), the head tree for the stagedata/mpe_loader positional contract, and the PR CI state.client/src/app/multi-dataset/multi-dataset.component.ts:1084 -
prepareDeleteRecordColumn(response)throws insidebuildDatasetsObjects(), which runs in thePromise.allSettled(...).then(...)callback at line 359 with no.catchanywhere on that chain (the only.catchin the block, line 419, is the per-dataset excel-parse one). The new module's own doc comment says callers must surface the throw in a promise-chain.catch, and the editor does (editor.component.ts:3527 setsgetdataError), but in the multi-dataset flow a getdata response carrying the reserved column becomes an unhandled promise rejection: the loop aborts,parsedDatasetsstays empty, and no error state or modal is shown. Add a.catchto the allSettled chain (or wrap thebuildDatasetsObjectscall) that surfaces the error the wayfetchDatasetfailures are surfaced.client/src/app/multi-dataset/multi-dataset.component.ts:1111 - this rename block still uses the literal
'_____DELETE__THIS__RECORD_____'while the same PR moves the identical block in editor.component.ts:3945 toDELETE_RECORD_COLUMN_NAME. Use the exported constant here too so the reserved name has a single definition.2 findings above for review.
Both addressed in
2b8a2fa.1. Unhandled rejection in the multi-dataset flow - wrapped the
buildDatasetsObjects(datasets)call in atry/catchrather than adding a.catchto theallSettledchain. Chaining after that.then()re-indented its whole 60-line body (142 changed lines for a two-line fix); the wrap confines the diff to the call site. On a throw it logs and drops the parse, the same asfetchDataset's own.catchdoes for a failed getdata.2. The name constant - the rename-to-
Delete?block inmulti-dataset.component.tsnow usesDELETE_RECORD_COLUMN_NAME, matching the same block ineditor.component.ts.Re-verified after the change: client unit tests 570/570,
npm run buildclean. The full Cypress suite re-runs on this head in CI.On the remaining literal occurrences of
_____DELETE__THIS__RECORD_____in these two components (the Excel-preview default, the submit-time reorder and the cell-change handler): those predate this PR and are untouched, so the diff stays on the sites the change actually moves. Happy to sweep them to the constant in a follow-up if you would rather the name had exactly one definition everywhere.Re-reviewed the full diff at
2b8a2faagainst the previous review at56025cc(the editor takes ownership of the delete-record column; getdata.sas and the mocks stop emitting it). Scope of this pass: the delta commit (surfacing the reserved-column guard in the multi-dataset flow, using the name constant) plus a re-read of the full base...head diff against the head tree. Both previous findings (the unhandled rejection from buildDatasetsObjects in the allSettled chain, and the literal reserved name in the rename block) are addressed at this head and not re-raised. Verified against the head tree: the mpe_targetloader/bitemporal_dataloader macros that consume the delete flag on submit are untouched by this PR; stagedata.sas still synthesizes its own delete column server-side; the SAS delta leaves no dangling ctrloptions reference (the vars3 keep-list was updated in step with the ctrlOptions block's removal); the gitleaks pre-commit hook, .npmrc (ignore-scripts + save-exact) and the strict npm audit steps all stand.client/src/app/multi-dataset/multi-dataset.component.ts:377 - the new catch logs and returns, but unlike fetchDataset's failures (which resolve to undefined and let the remaining datasets parse) it drops the whole parse with nothing shown to the user: uploadLoading is already false, parsedDatasets stays empty, and the only trace is a console.warn. A response carrying the reserved column means a real table column of that name - the editor flow refuses the load with an on-screen error state (editor.component.ts:3529 sets getdataError), so this flow should surface it the same way (eventService.showInfoModal is the pattern this file already uses elsewhere) rather than silently showing an empty "select tables" state with the file still attached.
client/src/app/editor/editor.component.ts:3894 -
prepareDeleteRecordColumn(response)throws inside initSetup, and the comment at 3892 says "A throw here lands in the caller's .catch" - that only holds on the direct path. initSetup retries viasetTimeout(() => this.initSetup(response, attempt + 1), 50)(line 3885) when hotInstance is not ready yet, and a throw from that retry runs outside the promise chain: the .catch at 3523 is not re-entered, so the reserved-column refusal becomes an unhandled rejection and the user is left on the loading spinner (getdataError never set, because tableTrue is only assigned at 3928, after the throw point). Guard for it: set this.getdataError = true (or route through the same catch) before rethrowing inside the retry branch too, so the refusal is surfaced whichever attempt it lands on.2 findings above for review.
Both addressed in
b26f557.1. The retry path in
initSetup- you are right that a throw from thesetTimeoutretry is outside the promise chain. The retry call is now wrapped and routes through a new privatehandleGetdataFailure(err), which is also what thecallService.catchnow calls - so the refusal setsgetdataError/tableTrueand shows the error state whichever attempt it lands on.2. The multi-dataset catch - now surfaces
eventService.showInfoModal('Upload error', err.message), the pattern this file already uses for the other upload problems, instead of logging and returning.Re-verified after the change: client unit tests 570/570,
npm run buildclean. The Cypress suite re-runs on this head.One note on the previous head:
Build-and-test-developmentwent red on2b8a2fa, onexcel.cy.tstest 18 (openTableFromTreewaiting on.app-loading), which the same spec passed on56025cc. It is the home-page boot spinner, not the upload path this PR touches, and the neighbouring tests in the same run passed - I have re-run the job. If it fails again on this head I will treat it as real and dig in.Re-reviewed the full base...head diff at
b26f557, focused on the delta commit (surfacing the reserved-column refusal on every getdata path) plus a re-read of the earlier commits against the head tree. Both findings from the review at2b8a2faare addressed at this head and not re-raised: the initSetup retry branch (editor.component.ts:3899) now wraps the recursive call in a try/catch routed through handleGetdataFailure, so a throw from any retry attempt lands in the error state instead of an unhandled rejection; and the multi-dataset catch (multi-dataset.component.ts:380) now surfaces the refusal on screen via eventService.showInfoModal rather than only a console.warn. Verified in the delta and head tree: initSetup is synchronous so the try/catch spans the retry correctly; the cypress childNodes[N+1] position math matches the new mock COLHEADERS with the client-prepended column; the %length(&pk) guard around the attrib statement is needed now that the delete name no longer pads it.No blocking issues.