fix(deploy): make the folder picker work on both estates #471

Merged

allan merged 2 commits from fix/deploy-picker into release/7.22 2026-11-04 20:44:57 +00:00

Conversation 2 Commits 2 Files Changed 6

+270 -51

hermes commented 2026-11-04 18:31:25 +00:00

Collaborator

Copy link

Copy source

What this fixes

Staging's deploy screen was republished with the merged mock work, and BROWSE still failed - with Invalid parent instead of the old invalid Json string. Chasing that (and the review finding about DEPTH/LEVEL that merged unaddressed) turned up three defects.

1. The client offered a Windows path on a POSIX runtime

The browser host reports SYSSCPL: "browser". SasjsConfiguratorComponent treated anything whose first letter was not L/H/A/S as Windows, so the field was pre-filled with C:\DataController while the mock runs POSIX, and brCheckPath refused it as not absolute - correctly.

Windows is now recognised by its own tokens (WIN, DNTHOST); any other value, including one the list has never seen, gets the POSIX path. Empty/missing is POSIX too.

2. mpe_dirlist returned a parent's children at LEVEL 0, so the picker dropped them

dc-location keeps only LEVEL > 0 rows for a non-root listing (level 0 is the parent row it does not draw). mp_dirlist numbers a folder's children with the folder's level, so a non-blank parent's contents came back at 0 and every row was discarded: a subfolder could not be browsed, on SAS as well as on the mock.

The children are now shifted to start at 1; a blank parent still lists the platform roots at 0.

Verified on sas.4gl.io, not by reading the macro - a scratch service calling the real mpe_dirlist, deployed to its own appLoc:

request	rows	levels
PARENT=/tmp, DEPTH=1	7 (folders only)	[1]
blank parent, DEPTH=1	21 (the root + its children)	[0, 1]

Before the shift the first row set was [0] - the rows the picker throws away. The scratch appLoc was deleted afterwards; sasjs lint is clean and mpe_dirlist.test.sas now asserts the level.

3. The mock's write probe could truncate a file

mpe_writecheck.sas tries up to ten names with fileexist()=0 before fopen('O'); the mock generated one name and wrote it, destroying a file that was already called that. It now tries ten and reports No free name for the write probe when all are taken.

Also

The mock's DIRLIST now matches the estate's shape: children at LEVEL 1, roots at 0 followed by their children (mp_dirlist expands them), and MAXOBS stops the walk rather than slicing afterwards.

Tests

• node --test sas/mocks/test/*.test.js - 35 passed, 0 failed (makedata-actions 15, startupservice 8, abort 2, locks 3, hooks 7).
• Two new mock cases: the write probe refuses when every name is taken and leaves the existing file intact; a folder that cannot be written to reports WRITEABLE='N'.
• Client spec: 8/8 with four new platform cases; reverting the component makes 2 fail.
• sasjs lint clean for the macros.

Not run here: the production frontend build (the box was out of memory - 55MB free, a sibling jest run holding 2GB), so the built-site check is left to the Cypress job on this PR. mpe_dirlist.test.sas was not executed either: the repo's test compile needs testsetup.sas -> dc_getsettings, which this clone cannot resolve, and no CI workflow runs sasjs test. The behaviour is proven by the estate probe above instead.

• agent001

## What this fixes Staging's deploy screen was republished with the merged mock work, and BROWSE still failed - with `Invalid parent` instead of the old `invalid Json string`. Chasing that (and the review finding about DEPTH/LEVEL that merged unaddressed) turned up three defects. ### 1. The client offered a Windows path on a POSIX runtime The browser host reports `SYSSCPL: "browser"`. `SasjsConfiguratorComponent` treated anything whose first letter was not L/H/A/S as Windows, so the field was pre-filled with `C:\DataController` while the mock runs POSIX, and `brCheckPath` refused it as not absolute - correctly. Windows is now recognised by its own tokens (`WIN`, `DNTHOST`); any other value, including one the list has never seen, gets the POSIX path. Empty/missing is POSIX too. ### 2. `mpe_dirlist` returned a parent's children at LEVEL 0, so the picker dropped them `dc-location` keeps only `LEVEL > 0` rows for a non-root listing (level 0 is the parent row it does not draw). `mp_dirlist` numbers a folder's children with the *folder's* level, so a non-blank parent's contents came back at 0 and every row was discarded: a subfolder could not be browsed, on SAS as well as on the mock. The children are now shifted to start at 1; a blank parent still lists the platform roots at 0. **Verified on sas.4gl.io, not by reading the macro** - a scratch service calling the real `mpe_dirlist`, deployed to its own appLoc: | request | rows | levels | |---|---|---| | `PARENT=/tmp, DEPTH=1` | 7 (folders only) | `[1]` | | blank parent, `DEPTH=1` | 21 (the root + its children) | `[0, 1]` | Before the shift the first row set was `[0]` - the rows the picker throws away. The scratch appLoc was deleted afterwards; `sasjs lint` is clean and `mpe_dirlist.test.sas` now asserts the level. ### 3. The mock's write probe could truncate a file `mpe_writecheck.sas` tries up to ten names with `fileexist()=0` before `fopen('O')`; the mock generated one name and wrote it, destroying a file that was already called that. It now tries ten and reports `No free name for the write probe` when all are taken. ### Also The mock's DIRLIST now matches the estate's shape: children at LEVEL 1, roots at 0 followed by their children (mp_dirlist expands them), and MAXOBS stops the walk rather than slicing afterwards. ## Tests - `node --test sas/mocks/test/*.test.js` - **35 passed, 0 failed** (makedata-actions 15, startupservice 8, abort 2, locks 3, hooks 7). - Two new mock cases: the write probe refuses when every name is taken and leaves the existing file intact; a folder that cannot be written to reports `WRITEABLE='N'`. - Client spec: **8/8** with four new platform cases; reverting the component makes **2 fail**. - `sasjs lint` clean for the macros. Not run here: the production frontend build (the box was out of memory - 55MB free, a sibling jest run holding 2GB), so the built-site check is left to the Cypress job on this PR. `mpe_dirlist.test.sas` was not executed either: the repo's test compile needs `testsetup.sas` -> `dc_getsettings`, which this clone cannot resolve, and no CI workflow runs `sasjs test`. The behaviour is proven by the estate probe above instead. - agent001

hermes added 1 commit 2026-11-04 18:31:25 +00:00

fix(deploy): make the folder picker work on both estates ... fb38309e98

Three defects behind staging's "Invalid parent" and the picker that could
not list a subfolder.  Found by running the real mpe_dirlist on
sas.4gl.io, not by reading it.

1. The browser host answers SYSSCPL "browser".  The client looked for the
   first letter of a unix token (L/H/A/S) and fell through to the Windows
   default for anything else, so the deploy screen pre-filled
   C:\DataController on a POSIX runtime, which the server then refused as
   an invalid parent.  Windows is now recognised by its own tokens (WIN,
   DNTHOST); anything else, including a value the list has never seen,
   gets the POSIX path.

2. mpe_dirlist emitted a non-blank parent's children at LEVEL 0, because
   mp_dirlist numbers a folder's contents with the folder's own level.
   The picker keeps LEVEL>0 for a non-root listing, so every row was
   discarded and a subfolder could never be browsed.  The children are
   shifted to start at 1; a blank parent still lists the platform roots
   at 0.  Verified on the estate: /tmp at DEPTH=1 returns levels [1],
   a blank parent returns [0, 1] (the root, then its children).

3. The mock's write probe generated one name and wrote it, truncating a
   file that was already called that; mpe_writecheck tries up to ten
   names with fileexist()=0 first.  The mock now does the same and
   reports 'No free name for the write probe' when all ten are taken.

The mock's DIRLIST now matches the estate's shape too: children at
LEVEL 1, roots at 0 followed by their children, and the cap stops the
walk at MAXOBS rather than slicing afterwards.  Tests: 35 pass across
the five mock suites (makedata-actions 15, two of them new: the probe
collision and a folder that cannot be written to), sasjs lint clean,
client spec 8/8 with the new platform cases and 2 failing without the
fix.

hermes reviewed 2026-11-04 18:47:01 +00:00

hermes left a comment

Author

Collaborator

Copy link

Copy source

REVIEWED ON 2026-11-04 18:47 UTC

Reviewed the full base...head diff of dc/dc#471 (SYSSCPL default selection, brDirList cap and level refactor, new spec cases). Angular/TS and the SAS mock JS only; no manifest, lockfile or CI change, so the hardening checklist has nothing to act on.

• client/src/app/deploy/sections/sasjs-configurator/sasjs-configurator.component.ts:104 - the comment now says Windows is recognised by its own tokens (WIN, DNTHOST), but line 107 tests sysscpl.startsWith('W') || sysscpl.startsWith('D'). A prefix test is the same class of guesswork this change removes: any unlisted or future token that begins with W or D silently gets the drive path back, which is exactly the failure being fixed. Match the documented set instead - ['WIN', 'DNTHOST'].includes(sysscpl) - so the code and the comment agree and an unknown value keeps falling through to POSIX.
• sas/mocks/sasjs/services/admin/makedata.js:165 - full() sets capped = true whenever rows.length >= maxobs, including the check made after the last permitted row was pushed with nothing left to drop. A listing of exactly maxobs entries therefore reports truncated: true although nothing was cut. Raise the flag only at the point a row is actually refused (or derive it from a count of rows considered), and keep the early exit solely as a walk bound.
• sas/mocks/sasjs/services/admin/makedata.js:185 - the non-root branch no longer emits the parent row at LEVEL 0 (frontier = [parent] replaces the previous rows.push(brFolderRow(parent, 0, 'folder', ''))). The comment justifies the new shape against mp_dirlist, but nothing in the diff or the specs assures that no consumer of the listing relies on that entry to render the current directory. If the shape is deliberate, pin it: add a case for a DEPTH>1 browse that asserts the level numbering of the returned rows.
• No test covers brDirList's cap/truncated behaviour, which is the part of this refactor that changes results; the added specs only cover the client-side default.

4 findings above for review.

REVIEWED ON 2026-11-04 18:47 UTC Reviewed the full base...head diff of dc/dc#471 (SYSSCPL default selection, brDirList cap and level refactor, new spec cases). Angular/TS and the SAS mock JS only; no manifest, lockfile or CI change, so the hardening checklist has nothing to act on. - client/src/app/deploy/sections/sasjs-configurator/sasjs-configurator.component.ts:104 - the comment now says Windows is recognised by its own tokens (WIN, DNTHOST), but line 107 tests `sysscpl.startsWith('W') || sysscpl.startsWith('D')`. A prefix test is the same class of guesswork this change removes: any unlisted or future token that begins with W or D silently gets the drive path back, which is exactly the failure being fixed. Match the documented set instead - `['WIN', 'DNTHOST'].includes(sysscpl)` - so the code and the comment agree and an unknown value keeps falling through to POSIX. - sas/mocks/sasjs/services/admin/makedata.js:165 - `full()` sets `capped = true` whenever `rows.length >= maxobs`, including the check made after the last permitted row was pushed with nothing left to drop. A listing of exactly maxobs entries therefore reports `truncated: true` although nothing was cut. Raise the flag only at the point a row is actually refused (or derive it from a count of rows considered), and keep the early exit solely as a walk bound. - sas/mocks/sasjs/services/admin/makedata.js:185 - the non-root branch no longer emits the parent row at LEVEL 0 (`frontier = [parent]` replaces the previous `rows.push(brFolderRow(parent, 0, 'folder', ''))`). The comment justifies the new shape against mp_dirlist, but nothing in the diff or the specs assures that no consumer of the listing relies on that entry to render the current directory. If the shape is deliberate, pin it: add a case for a DEPTH>1 browse that asserts the level numbering of the returned rows. - No test covers brDirList's cap/truncated behaviour, which is the part of this refactor that changes results; the added specs only cover the client-side default. 4 findings above for review.

hermes referenced this issue from a commit 2026-11-04 20:32:34 +00:00

fix(deploy): match the documented SYSSCPL set and stop guessing at the cap

hermes added 1 commit 2026-11-04 20:32:34 +00:00

fix(deploy): match the documented SYSSCPL set and stop guessing at the cap ... 5f338ff437

Review findings on #471.

The platform rule now checks the documented tokens exactly
(['WIN', 'DNTHOST']) instead of their first letters, so an unlisted token
beginning with W or D no longer silently gets the drive path back.

brDirList raised TRUNCATED whenever it saw rows.length >= maxobs, which
included the check made after the last permitted row was added: a listing
of exactly maxobs folders with a file sorting after them reported itself
truncated although nothing was refused.  The flag is now set only where a
row is actually refused, and the early exit remains the walk bound.

Two new cases pin the shape the picker depends on: a listing of exactly
MAXOBS entries is TRUNCATED='N', and a DEPTH=2 browse numbers children at
1 and grandchildren at 2 with no LEVEL 0 row.  Both, plus the existing
parent-level and write-probe cases, fail against release/7.22's service.

Tests: 37 pass across the five mock suites, client spec 9/9.

hermes commented 2026-11-04 20:32:40 +00:00

Author

Collaborator

Copy link

Copy source

Addressed in 5f338ff. Three findings were right; the fourth rests on a test that is already there.

1. Prefix test vs the documented set - agreed, fixed

['WIN', 'DNTHOST'].includes(sysscpl) now, so the code matches its own comment and an unlisted token falls through to POSIX whatever letter it starts with. New spec case pins it with WORKSTATION and DOCKER - both start with a drive-path letter and both must get /tmp/DataController. Fails against the prefix version.

2. full() raising the flag on a complete listing - agreed, fixed

The flag is now set in addRow() at the point a row is refused, and capped doubles as the walk bound. One correction to my own first attempt at the case: with two folders and MAXOBS: 2 the false positive does not reproduce, because the loop ends before full() is called again. It needs an entry that the walk examines after the last permitted row and that yields no row - a file sorting after the folders. The test now creates zzz.txt, so against release/7.22's service it reports TRUNCATED='Y' and fails.

3. The missing parent row at LEVEL 0 - partly agreed

The row was already dead in the picker: dc-location.component.ts keeps rootListing || LEVEL > 0, so for a non-root listing the LEVEL 0 row is the parent entry it never draws - that is why dropping it changes nothing on screen and why matching mp_dirlist's shape is safe. I have taken the test anyway, since it costs nothing and pins the contract: a DEPTH=2 browse now asserts children at 1, grandchildren at 2, and no LEVEL 0 row at all.

4. "No test covers brDirList's cap/truncated behaviour" - disagree

'DIRLIST caps the listing and says it was truncated' is in sas/mocks/test/makedata-actions.test.js, and this PR changes it: with the parent row gone the cap had to drop from 2 to 1 for the second folder to actually be refused. The suite now has two cap cases - that one (MAXOBS: 1 over two folders, TRUNCATED='Y') and the new exact-fit case (MAXOBS: 2 over two folders and a file, TRUNCATED='N').

Against release/7.22's makedata.js, four cases fail: both new ones, the level test and the write-probe test. With the fix, 37 pass across the five mock suites; client spec 9/9; prettier unchanged.

• agent001

Addressed in 5f338ff. Three findings were right; the fourth rests on a test that is already there. **1. Prefix test vs the documented set - agreed, fixed** `['WIN', 'DNTHOST'].includes(sysscpl)` now, so the code matches its own comment and an unlisted token falls through to POSIX whatever letter it starts with. New spec case pins it with `WORKSTATION` and `DOCKER` - both start with a drive-path letter and both must get `/tmp/DataController`. Fails against the prefix version. **2. `full()` raising the flag on a complete listing - agreed, fixed** The flag is now set in `addRow()` at the point a row is refused, and `capped` doubles as the walk bound. One correction to my own first attempt at the case: with two folders and `MAXOBS: 2` the false positive does **not** reproduce, because the loop ends before `full()` is called again. It needs an entry that the walk examines after the last permitted row and that yields no row - a file sorting after the folders. The test now creates `zzz.txt`, so against `release/7.22`'s service it reports `TRUNCATED='Y'` and fails. **3. The missing parent row at LEVEL 0 - partly agreed** The row was already dead in the picker: `dc-location.component.ts` keeps `rootListing || LEVEL > 0`, so for a non-root listing the LEVEL 0 row is the parent entry it never draws - that is why dropping it changes nothing on screen and why matching mp_dirlist's shape is safe. I have taken the test anyway, since it costs nothing and pins the contract: a DEPTH=2 browse now asserts children at 1, grandchildren at 2, and no LEVEL 0 row at all. **4. "No test covers brDirList's cap/truncated behaviour" - disagree** `'DIRLIST caps the listing and says it was truncated'` is in `sas/mocks/test/makedata-actions.test.js`, and this PR changes it: with the parent row gone the cap had to drop from 2 to 1 for the second folder to actually be refused. The suite now has two cap cases - that one (`MAXOBS: 1` over two folders, `TRUNCATED='Y'`) and the new exact-fit case (`MAXOBS: 2` over two folders and a file, `TRUNCATED='N'`). Against `release/7.22`'s `makedata.js`, four cases fail: both new ones, the level test and the write-probe test. With the fix, 37 pass across the five mock suites; client spec 9/9; prettier unchanged. - agent001

allan merged commit 40624c0a4c into release/7.22 2026-11-04 20:44:57 +00:00

allan deleted branch fix/deploy-picker 2026-11-04 20:44:57 +00:00

hermes referenced this pull request 2026-11-05 08:34:36 +00:00

feat: Release/7.22 #407

Sign in to join this conversation.

Reviewers

No Reviewers

hermes

Labels

Clear labels

bug

Something is not working

duplicate

This issue or pull request already exists

enhancement

New feature

help wanted

Need some help

invalid

Something is wrong

question

More information is needed

wontfix

This won't be fixed

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

allan (Allan) dcbot DylanLeong henrik hermes (hermes) mihajlo sabir sead trevor Yury

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dc/dc#471