v9.5.2 maximize log table height and auditor log access

[tpurschke]

Two changes to the network modelling UI: the log data table below the connection editor now follows the browser window instead of a fixed height and page size, and auditors can open the modelling forms they were previously locked out of.

Log data table height

• Removes the viewport cap on the log data table: LogDataTable.razor.css, the WrapperCssClass property and kMinRowsForScrolling are gone. The table is no longer wrapped in a scrolling box, so a column's filter dropdown can no longer be clipped by it.
• Replaces the fixed page size of 25 with a measured one: wwwroot/js/logDataTable.js reports how many rows fit into the window. It takes the height of an actually rendered row and derives the non-row overhead as container height − tbody height, so the result stays correct when a column header wraps or the pager grows, and subtracts the sticky navbar plus a small reserve. The container therefore ends up about one window tall, so scrolling it under the sticky navbar shows the table together with its pager.
• Adds LogDataTableLayout.ResolvePageSize, the page-size decision kept out of the component so it is testable without a browser. It clamps the measurement to 5..100 rows and keeps the size the table already has when a measurement fails, so the table neither jumps back to the default nor renders the whole loaded result set in one DOM update.
• Re-measures when a new owner's rows arrive and when the window is resized. The measurement is otherwise skipped, so a keystroke in a column filter does not cost a browser round trip, and the resize path is debounced by 200 ms because the global resize listener in window.js is not.
• Degrades to the default page size of 15 if JS interop is unavailable, so a circuit without a browser still shows a usable table.

Auditor access to the modelling forms

• NetworkModelling.razor gains IsModellingInteractionAllowedForApp, which is true for an auditor as well as for anyone who may already write. It drives the add connection, add interface, add common service and edit controls and the Readonly flag of the connection tables, so an auditor can open a connection and inspect it in the same form everyone else uses instead of being locked out of it.
• Persistence is unchanged. Save stays behind ExecutionModeAuthorizeView Roles="Modeller" in EditConn.razor and Request firewall changes stays disabled, so an auditor can look but not commit — which is what the role is meant to allow. Hasura grants the auditor role select only on modelling.connection, so the data layer refuses a write regardless of the UI.

Tests

• 13 unit tests across UiLogDataTableLayoutTest.cs and UiLogDataTableTest.cs covering the page-size bounds, both clamps, the failed-measurement fallback, the re-measure triggers and a stubbed IJSRuntime. Drops the two WrapperCssClass tests and the reflection helper they were the last caller of.
• UiNetworkModellingPageTest.Render_AuditorCanModelButCannotSaveOrRequestChanges pins the auditor rule from both sides: add connection and edit enabled, save and request firewall changes disabled.

Known limitations

• The arithmetic inside logDataTable.js is not covered by tests — the repo has no JS test harness, so it needs a look in a real browser at a small and a large window size.
• A page size measured while the user has paged into the data is held back until the next measurement taken on the first page, because BlazorTable 1.17.0 exposes neither a page change event nor a public page number setter. Paging back to the first page does not re-render this component and so does not apply it on its own; a later resize or a different owner does. Tracked as F7 in the review thread.
• Neither change has a revision-history.md or whats_new_facts entry yet.

🤖 Generated with Claude Code

[tpurschke]

Review round 4 — PR #5280 (v9.5.2 maximize log table height)

⚠️ Conflict of interest. This branch was authored by me (Claude) across earlier sessions, so this is a self-review and weaker than an independent one. The prior rounds' findings F1–F9 are carried over and verified against the current diff rather than taken on trust from the commit messages — one of them did not survive that check.

Review environment: Claude Opus 5 — model id claude-opus-5[1m], 1M-context variant. No explicit reasoning-effort override was set or observable in this session. Review depth: standard — the diff is UI, JS and tests only, with no schema, migration, Hasura permission or JWT change, so none of the deep triggers apply.

Base: develop · Head: 7142ac9ca · 9 files, +626/−79 · state: draft

Prior history: three earlier review rounds (rounds 1–3), no submitted reviews, no review threads. Round 3 raised F7–F9; the head commit is titled "fix findings F7,F8,F9".

---

Findings

#	Criticality	Status	Subject
F1	🟠 medium	✅ fixed	Dead reflection helpers left behind after test removal
F2	🔵 low	✅ fixed	kPageSize comment claimed a fit it did not have
F3	🔵 low	✅ fixed	Wrapper div with no class or attribute
F4	🟠 medium	✅ fixed	Page size change preserved page index, not position
F5	🟠 medium	✅ fixed	Zero conflated "could not measure" with "not one row fits"
F6	🔵 low	✅ fixed	New members lacked XML doc comments
F7	🟠 medium	⚠️ still open	Deferred page size is not applied when the user pages back to the first page
F8	🟠 medium	✅ fixed	Race in OnWindowResized over a disposed token source
F9	🟠 medium	✅ fixed	Dispose(bool) public with a finalizer pattern but no finalizer
F10	🟠 medium	🆕 new	IsOwner is now true for auditors, so the handler's own save/delete guards no longer mean "may persist"
F11	🔵 low	✅ fixed	Title and description covered only the table height, not the auditor change
F12	🔵 low	🆕 new	No revision-history.md or whats_new_facts entry for either user-visible change
F13	🟠 medium	🆕 new	Test API stub exceeds the complexity limit; pre-existing, extended by this PR

No rating from an earlier round was lowered.

---

F7 — still open, not fixed

The head commit is titled "fix findings F7,F8,F9". F8 and F9 are genuinely fixed. F7 is not: it was documented rather than resolved. ApplyMeasuredPageSize now carries the comment

"Paging back to the first page does not render this component and therefore does not apply it on its own."

which restates exactly the behaviour F7 reported. The explanation given is real and worth keeping — BlazorTable 1.17.0 exposes neither a page change event nor a public page number setter, so the position cannot be carried across a size change and holding the size back is the safer of the two options. But the user-visible effect is unchanged: resize the window while on page 3, page back to page 1, and the table still uses the old page size until a later resize or a different owner triggers a measurement.

Options, none of them free: poll the page number from the existing render path, subscribe to a click on the pager, or accept it and say so. If it is being accepted, mark it accepted explicitly — a finding closed by a commit message that says "fix" while the behaviour stands is the one state a later reviewer cannot distinguish from a real fix. I have kept it at medium and open rather than reclassifying it myself.

F10 — IsOwner no longer means "may persist"

IsModellingInteractionAllowedForApp returns true for Roles.Auditor, and its value is passed as the isOwner constructor argument of ModellingAppHandler, reaching ModellingHandlerBase.IsOwner. That flag is the sole guard on two write paths:

• ModellingConnectionHandler.Save() — if (!IsOwner) { deny }, no role check
• ModellingHandlerBase.DeleteConnection() — same shape

For an auditor those guards are now open. No current path lets an auditor persist, and I verified both remaining layers:

• the Save button is rendered inside ExecutionModeAuthorizeView Roles="Modeller", so an auditor gets the disabled branch — Render_AuditorCanModelButCannotSaveOrRequestChanges pins this;
• Hasura grants auditor select only on modelling.connection (insert, update and delete are modeller, plus implementer/middleware-server for update), so a mutation is refused at the data layer.

So this is not a live authorization hole, and I am not rating it high. What changed is that the middle layer stopped carrying meaning: IsOwner now reads "may interact" while its two call sites still treat it as "may persist", and the name says neither. A future caller that guards a new write on IsOwner — the obvious thing to do, since two existing ones do — would be wrong for auditors and would depend entirely on Hasura to catch it. That is the maintainability trap the medium bucket names.

Cheapest fix that keeps the intent: leave IsOwner meaning ownership and add a separate flag for "may open the form", so the two questions stop sharing an answer.

F11 — fixed in this round

The title and description described only the log table height, while NetworkModelling.razor carried an unrelated auditor RBAC change from commit ebd3de2e6. I have rewritten the PR description to cover both, state that persistence is unchanged and name the two layers that enforce it, and record the F7 limitation. The title still reads "v9.5.2 maximize log table height" and no longer matches the whole change — worth a second's thought before merge, though splitting the branch at this point is probably not worth it.

F12 — no release documentation

Neither change has a revision-history.md entry, and whats_new_facts in fworch-texts.sql is untouched. The auditor change in particular is user-visible behaviour: a role that could not open these forms now can. Per the repository standards both belong in the release notes, and whats_new_facts has to be updated in every language version at once.

F13 — complexity, pre-existing and extended

NetworkModellingPageTestApiConn.SendQueryAsync in UiNetworkModellingPageTest.cs is a dispatch chain of if (query == …) return …. Measured branch count: 19 on develop, 21 on this branch — the PR adds the getRuleActions and getTracking arms. The CODING_GUIDELINES.md limit is 10. The violation is pre-existing and not introduced here, and the two new arms follow the established pattern, so this is a "while you are here" item rather than a blocker. A dictionary from query to handler would take it to one branch.

---

Checks that passed

• CODING_GUIDELINES.md — otherwise clean: no file over 1000 lines (largest changed: NetworkModelling.razor at 737), no method over 100 lines, none over 7 parameters, no magic numbers, no inline array arguments, no missing XML docs on the new public members.
• Localization — every user-visible string in NetworkModelling.razor and LogDataTable.razor goes through userConfig.GetText(...). No hardcoded UI text.
• Security — no schema, migration, Hasura permission, JWT or LDAP change. The only authorization-relevant change is F10, analysed above. logDataTable.js reads geometry only — no innerHTML, no eval, no user data reaches the DOM through it, so the new script tag in _Host.cshtml adds no injection surface. No secrets, no new network calls, no tenant-scoped query touched.
• Disposal and events — Dispose unsubscribes from OnGlobalResize and sets the disposed flag that both the resize path and ApplyMeasuredPageSize check, so a settling resize on a component that has left the page does nothing.
• LogDataTableLayout — bounds, the fail-versus-zero distinction and the first-page guard all hold; ResolvePageSize keeps the current size only on a negative measurement and clamps otherwise.
• Pre-existing hits dropped — a commented-out line in _Host.cshtml was reported by the mechanical scan but exists in develop and is untouched here (this PR adds exactly one line to that file), so it is not a finding for this PR.

Residual risk

• The JavaScript is untested. logDataTable.js holds the actual arithmetic and the repo has no JS test harness. The C# tests cover the decision around it, not the measurement itself. It needs a look in a real browser at a small and a large window, and with a wrapped column header.
• CI is not green yet: Test install on ubuntu-latest and the ansible-core 2.18 variant are both still pending. SonarQube, GitGuardian, Python Code Check and the launcher guard tests pass.
• F7's effect is reachable in normal use, not only at the edges: resize while paged in, then page back.

---

Recommendations

Decide before merge:

• F7 — either close it properly or mark it accepted; do not leave a "fix" commit standing over unchanged behaviour.
• F10 — split "may interact" from "may persist" so IsOwner stops meaning two things.

Should do:

• F12 — add the revision-history.md entry and the whats_new_facts text in every language.
• Browser-check the JS at two window sizes; it is the one part of this change nothing verifies.

Nice to have:

• F13 — replace the stub's if chain with a dictionary.
• Retitle the PR, or accept that the title names only half of it.

Review method

• Depth: standard. Capability: correctness and security passes performed directly on the primary model against the fwo-review-pr checklists; the verification of F1–F9 was done by reading the current code for each, not by trusting the commit messages — which is what caught F7.
• Delegated to a reduced tier (haiku): prior-review-history collection, and the mechanical CODING_GUIDELINES.md and localization scan. Both returned evidence only; every hit was re-verified on the primary model, and one was dropped as pre-existing.
• Usage budget: this environment exposes no usage indicator to the agent, so the 25 % ceiling could not be measured. Proxy limits were applied and respected: 2 sub-agent dispatches (limit 6), no escalation to deep, reads confined to the changed files plus the handlers, permissions and markup they reach.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud