Review follow-up on the browser harness.
The exclusion of va-ddc-host now names the issue that tracks re-enabling it
(dc/dc#449), so the coverage gate cannot report a rotted exclusion as accounted
for. The comment also states the ordering constraint the spec carries: it clears
the DC_JSLIB filter tables per case, and every spec in that job shares one mock
estate, so it has to run last or be given its own.
The spec's comments and test name describe the behaviour and the harness, with
no ticket numbers, per AGENTS.md. The va-filter docstring says what the inference
is: a guess from the value's shape alone, which can disagree with the column in
either direction, and which costs the deferred call rather than the page because
the attempt suppresses the abort modal and falls back to the unfiltered load.
issue 444 was a month-name date from VA becoming a character literal, and it
only reproduced on a report refresh: the deferred initial load reads VA's
buffered message, which has no column metadata, so the SAS kind comes from the
value's shape. Posting a message into a running page exercises the other path -
it arrives after getdata, metadata in hand, so a date is formatted correctly
whatever the client does - which is why the unit spec could pin the fix but not
show the failure a report author sees.
The new spec frames the built app in sasjs/server's /ddc-host page, which mounts
it with VA's sandbox attribute set and, with timing=early, replays the message
from the moment the frame is created. That is the request and apply path a
refresh takes, with mock services instead of a VA estate.
Where the assertion sits: the mock public/validatefilter stores what it is sent,
so the clause is read back from the drive (mpe_filtersource.json) rather than
inferred from a request payload. The store is emptied per case because
mpFilterStore reuses an existing FILTER_RK for a clause it has already seen, and
a leftover row would answer for this run. The host page's own log is the second
witness: the message is captured before the frame finishes loading.
Three cases in one run - the WORDDATE shape from the issue, a DATE9 date that
always worked, and a character parameter. Against the unfixed service the first
stores 'August 02, 1971' and fails, while both controls pass; with the fix all
three store the expected literal. So the guard goes red for the defect and only
the defect.
The spec runs last, needs no SAS, and takes about a minute against the mock
estate the other specs already use.
mv_getgroupmembers declared the member name at $43, and a longer name was
truncated - or, on Viya, stopped the step with "Multiple lengths were
specified".
membername is declared at $256 in the same shape the macro now returns it: one
width for every group, so the tables the caller combines are never of differing
lengths, and a long active directory name is kept whole.
The stage that reports what the file is doing is meant to belong to the whole file, and it finished at "Extracting the data finished" whatever happened. A sheet whose parse rejected had already blanked its datasource and dropped itself from the submission in the per-sheet catch, so the user was told the file had extracted while a table was silently missing from the submission.
`allSettled` was already the right primitive, but its results said nothing: the catch swallowed the error, so every promise fulfilled. The catch now names the sheet on the component, the stage ends at a new `extractionFailed` state when anything was left out, and the pane lists what was left out and why.
The two reasons a sheet contributes nothing are kept apart, because they are not the same thing to the reader: a sheet the sheet utility could not read is a "could not extract" problem in red, while a sheet it parsed and found no rows in - which arrives as the rejection "No relevant data found" - is reported as "no rows found" without calling the file corrupt. Both leave the sheet out of the submission, and either one stops the stage reporting a clean finish.
The row derivation sits inside the try with the parse. Outside it, a throw on a row shape the sheet utility did not expect escaped as a rejection that `allSettled` swallows - the dataset stayed in the submission with `parsingTable` true and the stage reported a clean finish over a sheet that had been dropped.
The messages live in the no-table-selected pane, which is where the user is once extraction has finished; the file pane they were first put in unmounts as soon as the datasets are fetched, so they would never have been read there. The stage line keeps one id per state rather than two of the same: `file-loading-stage` in the file pane while the datasets are fetched, `file-extraction-stage` in the overlay while the sheets are extracted, and the Cypress spec asks for both.
The Cypress spec asks for both stage ids, because the element that carries the stage changes state, and asserts that a sheet left out of the submission is named in the pane - under whichever reason the sheet utility reported, since only the utility decides which of the two it is.
The spec builds the component through the test injector rather than with seven positional arguments, so a constructor that gains or reorders a dependency fails there (`NG0201: No provider found for ElementRef`) instead of binding a stub to the wrong collaborator and passing.
The comment claimed the sidebar animates open/closed. It does not: the
collapse is a plain margin-left on clr-vertical-nav.closed (styles.scss)
and margin is not in any transition there - Clarity's width transition on
.clr-vertical-nav does not apply, because the width never changes. What
does need the deferral is that Angular applies the class binding after the
event handler returns, so a synchronous measurement reads the pre-toggle
layout.
The spec now says what it covers: the scheduling contract, not whether a
measurement lands before or after a transition.
Unsubscribing stops future layout notifications but leaves a timer that
one already scheduled, so a sidebar toggle immediately followed by destroy
still ran calculateInputPosition() against a torn-down view. The timer id
is now held and cleared in ngOnDestroy, and the spec covers that window -
it fails if the clear is removed.
Also drops the EventService.dispatchEvent stub from the viewboxes spec,
which advertised a method this branch deletes.
The specs gated their assertions on a real setTimeout(..., 10) plus done,
which is wall-clock timing: a loaded runner can assert before the
component's own timer fires, and the negative case would then pass
vacuously. fakeAsync + tick order the assertion on the scheduler instead.
Also drops the stale half of the viewbox resize comment - the shared tail
no longer notifies listeners, it just persists and re-measures.
The synthetic resize dispatch was its only caller. Nothing else used it,
and a generic window-event dispatcher on a service invites exactly the
coupling this change removes.
The sidebar spec now asserts the behaviour directly - no resize event
reaches window listeners on a toggle - and fails when a dispatch is put
back, rather than asserting on the removed API.
A sidebar toggle and the viewbox data/resize paths dispatched a synthetic
window resize so other components would notice the layout moved. Two
thirds of that was unnecessary: Handsontable 18 attaches its own
ResizeObserver to the grid's parent, so a container change re-measures the
grid with no event at all (measured in Chromium: view.getLastSize().width
tracks a container resize 600 -> 380 -> 700 without any dispatch).
The one real consumer was the autocomplete dropdown, which follows its
input under the new layout. It now subscribes to
EventService.onSidebarToggle and re-runs calculateInputPosition(), and
unsubscribes on destroy; the window:resize listener stays for real
viewport changes.
Removed: sidebar.component.ts:53, viewboxes.component.ts:281, :705 and
:1571. The viewbox resize tail already re-measures each instance through
refreshTableAfterResize, and a data load renders the grid itself.
Scoping the pass to one grid left the page's other grids uncorrected: the
viewboxes each render their own Handsontable (shared/viewboxes), and the
multi-dataset preview renders two. Nothing called the pass for them, so
they kept role=treegrid and aria-rowcount=-1 - the failures this change
exists to remove. The old document-wide sweep happened to cover them.
observeGridAria(hot) runs the pass and keeps it running from the grid's
own afterRender, for grids whose component has no render hook of its own.
It defers by one microtask: Handsontable writes the row/column counts
after the render that follows a data load, so a pass inside the hook
still sees the previous frame.
Wired into getViewboxHotInstance (every viewbox that hands out an
instance) and into both multi-dataset instances.
The root `package.json` carries no install or postinstall script, so the
guidance to rely on "the composite install script ... which installs both
subprojects" sends the reader after something that does not exist, and
`client/.npmrc` told them the same thing in a comment.
The install section also contradicted itself: it opened by telling the reader
to write the SheetJS registry token into `client/.npmrc`, and closed by
explaining that `client/.npmrc` is a tracked policy file that must not carry a
token (the pre-commit hook rejects one). The token goes in the user config,
which is what the first block now says.
The install sequence itself is unchanged - root `npm ci`, then `client` and
`sas` explicitly - because that is what the repo actually needs.
Review follow-up. The table pass matched every table inside the grid root,
so a table a cell renderer owns picked up role=table and scope=col - which
contradicted the reason the wrapper loop avoids [role="treegrid"]. It now
matches table.htCore only (every HOT table carries that class, one per
view), and header cells are read with :scope > thead > tr > th so a
renderer's nested table is not touched. The separate aria-rowcount=-1 pass
is gone: the wrapper strip already removes that attribute from the grid's
own element, and matching it anywhere else was the same overreach.
The viewer also gets the editor's fallback chain, so a pass that runs
before hotInstance is assigned still finds the grid through
hotTableComponent.
Scoping the ARIA pass to the grid exposed a dependency the document-wide
sweep had been hiding: the sweep used to write aria-label from an
element's text, so cypress/e2e/full-table-search.cy.ts could select the
filter modal's submit button as aria-label="Ok" - a label only the sweep
ever wrote. Once the sweep stopped reaching the modal, that selector
matched nothing and the dev job failed on it.
An element's text already is its accessible name, so the synthesis is
gone from the pass, and the spec now selects the button by its text the
way a user would.
The helper-only spec passes even if the observer goes back to calling
updateSettings() - which is the regression the helper exists to prevent.
Move the observer into refreshGridAfterContainerResize.ts so a spec can
drive a real container resize through it, and assert that the resize
reaches refreshDimensions() without any settings call. Verified by
re-injecting the old wiring: the spec fails with 'Expected spy
updateSettings not to have been called', and passes once it is removed.
Also corrects the helper's claim about what refreshDimensions() measures:
it compares rootElement.getBoundingClientRect() with the last measured
size, and both grids take their height from a viewport-derived calc(), so
a resize that matters is a width or viewport change.
The sweep matched [role="treegrid"] inside the root, which would also
catch a treegrid a cell renderer put in a cell. The grid's own wrapper is
the root itself, so target that plus the overlay wrappers Handsontable
renders, and leave nested widgets alone.
Adds a live-grid spec: Handsontable writes role=treegrid/aria-rowcount
again on loadData, so the pass has to run after renders, not once.
The editor and viewer each ran fixAriaAccessibility() over the whole
document on every render, on a 3-second interval and from a
MutationObserver on document.body, and each call scheduled four further
copies of itself. The sweep removes the role/aria attributes that its own
observer watches, so it re-armed itself.
Measured with axe-core against Handsontable 18: the default markup fails
aria-required-children (critical) because the treegrid wrapper has no row
children, and disabling the tags with ariaTags:false only trades that for
empty-table-header while leaving the placeholder semantics missing. The
existing attribute sweep is therefore the right end state - it just has to
run against one grid's root, not the document.
Both components now call one shared helper with their own
hotInstance.rootElement, and the interval, the body observer and the four
deferred re-runs are gone; the afterRender/afterChange hooks and the init
calls are the only triggers.
The grid's container ResizeObserver (added in c7ba025 to replace the
afterRender window-resize dispatch of #246) refreshed by calling
updateSettingsSortSafe({ height }), and that wrapper's whole reason for
existing is that any updateSettings() while a multi-column sort is active
corrupts formula references into #REF!. It works around that by clearing the
sort, applying the settings and re-applying the sort - so every sidebar
toggle, modal open/close or window resize re-sorted the user's data, and an
open cell editor (anchored to visual coordinates) could end up writing to a
different row than the one being typed into.
refreshGridAfterContainerResize() replaces it with hot.refreshDimensions():
it compares the element's box with the last known size, re-measures the
overlays and redraws only when the size actually moved, touching no settings.
The decision lives in its own module so the spec can pin it - a real
Handsontable instance with an active sort and a formula column asserts that
the resize path calls refreshDimensions() and does not call updateSettings(),
clearSort() or sort(), that the sort config is unchanged, and that no cell
degrades to #REF!.
This is the narrow, independent piece of #246; the wider removal of the
remaining synthetic window resize dispatches (sidebar, viewboxes) is analysed
on the issue and is not part of this change.
Two findings from review.
downloadTablePdf() revoked the blob URL in the same task as the anchor click.
Firefox and Safari read the blob asynchronously, so that can cancel the
download and leave the user with no file; the revoke now runs on a later turn,
after the click has been handed to the download machinery.
getExportRange() clamped the selection's left edge up to skipLeadingCols but
then forced the right edge up to match, so a selection lying entirely inside
the leading housekeeping column(s) was shifted sideways into a column the user
never selected - the PDF contained column 1 when they had selected column 0.
The range is now clamped into the exportable columns and comes back empty
(right < left) when nothing is left, which the PDF path reports as "no table"
and the CSV/Excel path skips rather than exporting the wrong column.
The revoke change is only covered by the spec (it needs a browser to exercise);
the range change is covered by the spec and by the local harness, now 28 checks.
The modal decided what was wrong with two regular expressions of its own, and the answer disagreed with the log it was showing:
- an ODS HTML line was cut from the first capital `E` in it and lost its last character, so an error read `ERROR: File WORK.X.DATA does not exist.</font>`;
- a message SAS wrapped over several lines was reported once, truncated at the break, and the rest of it was not reported at all;
- the colon-less form SAS writes (`ERROR Libref X is not assigned.`) was missed on any line carrying markup;
- the notable notes - uninitialised variable, invalid data, a format too small - were not shown anywhere;
- clicking an issue searched the rendered text for its message, so a log that repeats a message (two `ERROR 22-322:` lines is ordinary) always jumped to the first of them.
`parseLog` from `@sasjs/utils` replaces all of it, the same function the VS Code extension and the SASjs server Studio use, so one log now reads identically in every host. Each request keeps the issues the parser reported, grouped by severity, and the tree gains a Notes list beside Errors and Warnings. The click scrolls to `issue.line` and flashes from that line to `issue.endLine`, and the lists no longer render at zero.
The modal is SAS Logs now, as are the menu entry, the deploy button and the abort modal's button; the request history is unchanged underneath.
`formatTimestamp` decided on `moment().format()`, which is 'Invalid date' - truthy - for anything it cannot parse, so its fallback could never fire. It asks `isValid()` now.
The warning flash was yellow where the warning text is ochre; both are ochre now, and the notes flash blue.
`@sasjs/utils` goes to 4.3.1, which is where the parser lives.
The new empty-grid spec built a grid from `data: []` alone, and Handsontable
reports zero columns when the data is empty and no `columns` setting defines
the schema - so buildGridPdfInput() correctly returned null and the spec failed
in CI while passing against the stub harness. The real grids declare their
columns, so the fixture does now too.
Two findings from the second review.
A row taller than the printable area was drawn straight through the footer and
off the page, losing the tail of the cell unannounced - MAX_COL_CHARS bounds a
column, not a value, so one long text field wraps to more lines than a page
holds. Rows are now clamped to the tallest row that fits the page, with ' ...'
marking the cut, and the height budget accounts for both margins so an
exactly-fitting row stays on the page it started on instead of leaving the
first page with headers only.
exportGridPdf() also returned silently for a grid with no rows, although the
writer renders an empty table as a valid header-only page. The input is now
built by buildGridPdfInput(), which returns null only when there are no
columns, so a rowless grid downloads a page saying "0 rows x N columns".
Tests: the tall-row spec asserts the marker, the single page and that no text
start falls outside the printable band; the empty-grid spec asserts the input
is built rather than skipped.
Two findings from review.
First, a regression in the CSV/Excel path: getExportRange() defaults `left`
to skipLeadingCols, so comparing it against skipLeadingCols made every
no-selection export look like a whole-grid export and the plugin put the
leading housekeeping column back in. The whole-grid test now compares the left
edge against visual column 0, so a skipped column stays out.
Second, the PDF row cap was applied after collectGridTable() had already read
every cell, so a very large grid still froze before the cap took effect.
collectGridTable() now takes a maxRows limit that clamps the row loop before
any cell is read, and exportGridPdf() still reports the true total in the
subtitle.
Both are covered: a spec asserts the CSV export receives range
[0, skipLeadingCols, lastRow, lastCol], and one asserts the cap stops cell
reads rather than collecting and slicing.
CI caught this on the real grid specs: getPlugin('hiddenColumns') returns the
plugin instance even when it is switched off, and isHidden() then dereferences
a map that enablePlugin() never created, so any grid without a hiddenColumns
setting threw instead of exporting.
Ask the plugin's enabled flag first and treat a failure as "visible" - an
export is better off with a column the user could not see than with an error
dialog. The spec adds a regression case for exactly the CI failure, and its
afterEach no longer destroys an already-destroyed instance when a test creates
no grid.
Adds a PDF entry to the right-click Export submenu, next to CSV and Excel,
so the grid as displayed can leave the browser as a printable document.
Implemented directly against PDF 1.4 rather than adding a PDF library: the
output is a text table, which needs no embedded fonts, no images and no
compression. The base-14 Courier font is used because its glyphs are exactly
600/1000 em wide, so column widths are arithmetic on character counts instead
of a font-metrics table - a 1200 x 12 grid is laid out in ~60 ms.
Layout: A4 landscape, bold header band repeated on every page with "Page n of
m" footers, numeric columns right-aligned, values word-wrapped inside their
column, and long tables paginated. Wide tables are fitted by shrinking the
body font first (7pt down to 5pt, so a 20-column table keeps full headers at
6pt) rather than squeezing columns; only at 5pt are columns scaled, and they
stop at 4 characters - past that trailing columns are dropped and the first
page says how many were kept, so nothing is omitted silently.
Encoding is WinAnsi, the widest set the base-14 fonts cover without embedding;
characters outside cp1252 print as '?'. A typeset, branded report is a
different feature and belongs on the server, where SAS formats and DC metadata
are available.
The export keeps the existing "what you see" contract: current selection (when
it spans more than one cell), otherwise the whole grid, hidden columns
omitted, leading housekeeping columns skipped. The range calculation that CSV
and Excel already used is now a shared getExportRange(), so the three formats
cannot drift apart. Rows are capped at 5000 and the subtitle says so.
Tests: pdf-export.util.spec.ts covers document structure (xref offsets, page
count, footer numbering), string escaping, WinAnsi mapping and fallback,
wrapping, alignment, multi-line values, pagination, font fitting and the
page-width invariant; hot-export.util.spec.ts covers grid extraction against a
real Handsontable instance.
A MIN_TOTAL_MB typo reached the integer test under set -e and aborted with "integer expression expected"; it now prints usage and exits 2. A kernel without MemAvailable no longer reports 0 MB available in the summary or the warning, so the oversubscription warning stays meaningful.
release.yaml (all three jobs), sites.yaml and release-assets.yaml run the same cold production build that the review jobs do, so they get the same pre-install check.
The header now says what the check rules out (an undersized box) and what it does not (two jobs sharing one box), so the MemAvailable warning is not read as a gate.
An undersized runner does not fail honestly: the kernel OOM killer takes
the build, so the job reports exitcode 137 at ng build, or Karma
disconnects mid-suite after twenty minutes. Both read as a flaky test
rather than as a box that cannot hold the job, and both cost a whole CI
slot.
The check runs first in the three client jobs (Build-and-ng-test,
Build-and-test-development, lighthouse), prints total and available RAM,
vCPU count and any cgroup limit, and exits 1 when the effective size is
under 8 GB - the supported minimum. A cold production build measures
3.8 GB peak on its own, so 8 GB leaves roughly 2x headroom.
An uploaded file's preview is validated in place, so the cells that fail
a rule carry Handsontable's invalid cell meta. Discarding the file
restores the loaded data, but cell meta is keyed by row/column and
survives that restore - without a re-validation the grid keeps painting
cells that are no longer invalid.
cancelEdit re-runs the validation (hot.validateCells()) after restoring
the data source, which is what clears them; this test drives the flow and
pins it: import a file that fails validation, discard it, then assert the
row count is back, no .htInvalid cell remains and the page is read-only
again. The manual-edit path (test 25) keeps the parsed values by design.
Refs #122
The multi-load page rendered the loading stage only inside a branch guarded by
`parsedDatasets.length`, and that collection is empty for the whole read and
sheet-scan phase - so two of the four stage labels ("Reading the file",
"Searching for the data in the file") were assigned but never reached the DOM,
and a large file sat on screen with no feedback at all.
The stage is now shown next to the file name and size from the moment a file is
attached, and again in the content area while the sheets are being parsed. The
labels cover every phase the user waits through: reading the file, searching
for the data in the file, file ready to load, fetching the dataset details (one
getdata call per dataset - the longest wait), extracting the data, and
extracting the data finished. A spinner accompanies the stages that have work
running; the idle stages and the finished state show the label alone.
Two corrections fall out of that: the stage only moves to `extracted` once
every sheet has been parsed rather than when the first one finishes, and the
read's continuation only advances the stage if the read is still the phase in
progress (the sheet scan runs in parallel and can finish first).
excel-multi-load.cy.ts gains test 4, which asserts the file size, the ready
stage, the fetching stage (the getdata calls are held open so it is on screen
long enough to assert) and the handover to the "select a dataset" hint.
Closes#121
Enables noMacroIfInOpenCode and promotes it to "error". Open-code %if is a SAS
9.4M5 feature, so the 19 sites it found in nine files were hiding a version
requirement the codebase does not document (the requirements page says 9.4M3).
Each is rewritten so the construct no longer appears in open code:
- Six blocks become a small macro called unconditionally, which is where the
conditional then lives: drop_cas_tables (demodata), add_derived_cols
(getdata), build_sasdata4 (getdata test 4), drop_derived (stagedata),
build_staged (getrawdata), and one per option in the 20261003 migration.
- Four blocks in the two makedata STPs get the same treatment: check_outfmt,
check_keepme, emit_response and self_destruct. Their `call symputx` writes
are now explicit about the global symbol table, because a %let or a plain
symput inside a wrapper would be local to it.
- Two guards in stagedata and getrawdata are removed rather than wrapped: the
SQL only selects into a list that is already %let blank, and with no derived
variables it returns no rows, so the guard was doing nothing. The trailing
drop keeps a conditional, now in a macro and tested with %trim, since a
zero-row select can leave a single blank in the variable.
- validatefilter's two %let branches become one data step with call symputx,
which writes to the global table the later %mp_filterstore reads.
Verified: `npm run lint:sas` clean with the rule as an error; `sasjs compile`
succeeds; on sas.4gl.io the full suite passes 64/64, including the derived
variable tests in getdata test 4, stagedata tests 3 and 4 and validatefilter,
which are the paths these wrappers touch.
The tree is clean under all of them, so the remaining warn-level rules can be
promoted without a single code change: hasDoxygenHeader, hasMacroNameInMend,
hasMacroParentheses, noNestedMacros, strictMacroDefinition, noSpacesInFileNames,
lowerCaseFileNames, noTabIndentation, noTrailingSpaces, noGremlins and
noNestedCommentsInHeader.
With that, every rule @sasjs/lint 4.9.0 can raise is an error - only
noEncodedPasswords was an error by default - so any new violation fails
`npm run lint:sas` and the CI step that runs it, instead of printing a warning
that leaves the exit code at 0.
Checked: a trailing space appended to a file reports
"Error Line contains trailing spaces" and a non-zero exit, where it used to be
a Warning.
sas/.sasjslint takes maxLineLength from 180 to 80 - the linter's own default -
and promotes it to "error".
53 of the 258 .sas files already hold a line longer than 80. Rewrapping them
would be a large, reviewable-only-in-theory diff, so each keeps its current
longest line as an exact ceiling via a per-file override, e.g.
@sasjslint {"maxLineLength": 95} in services/validations/tables_all.sas. A
file with no override is held to 80, and an overridden file cannot grow past
the length it has today: the ceiling is the file's longest line, not a round
number.
Both directions were checked: a 101 character line added to an
un-overridden file fails with "Line exceeds maximum length by 21 characters",
and a 114 character line added to macros/mpe_execute.sas (ceiling 101) fails
by 13. `npm run lint:sas` is clean with the setting in force.
Rung 6 of the #376 ratchet: `"lineEndings": "lf"` is enabled and promoted to
"error". Its 37 findings are cleared and nothing but line endings changed.
- 34 files ended without a newline; each now ends with exactly one LF.
- 3 files were stored with CRLF (services/public/getgroups.sas and the two
sas9 lineage services, getmetacols and getmetatables), against the repo's
own `* text=auto eol=lf` in .gitattributes. They are converted to LF.
Checked mechanically that this is whitespace only: for every changed file,
HEAD's content and the new content are identical once CRLF is folded to LF
and the trailing newline is stripped.
VERIFICATION
- `npm run lint:sas`: 0 warnings, 0 errors, exit 0.
- `sasjs compile`: succeeds.
- On sas.4gl.io, against a scratch appLoc seeded with makedata:
"Tests that pass: 64/64 (100%)".
dc
allan