lisa/feat/impl-system-ssp-refactor#261
Conversation
gusfcarvalho
left a comment
There was a problem hiding this comment.
Findings inline (1 medium, 4 low). Headline: the Overview systemImplementationStats loader this PR null-proofed is dead code — never rendered — and should be removed instead (saves 4 API calls per mount); plus three smaller robustness/consistency items (phantom seeded diagram on non-404 failures, empty states masking fetch errors, tooltip-less TooltipTitle).
Verified: make reviewable green locally (669 tests), CI green. Router duplicate-name resolution, Diagrams delete styling (matches CatalogListView), UsersView style-block removal (admin views carry their own copies), and the ssp-id route-param fix were all checked and are correct.
gusfcarvalho
left a comment
There was a problem hiding this comment.
All five review findings verified addressed in f37c80f/af079d5: dead stats loader removed entirely (ref, four useDataApi handles, allSettled block, its test, and unused type imports), authorization-boundary initialData no longer seeds a phantom diagram, Users/Authorizations empty states now gate on a load-failed flag with catch + error toast, and both TooltipTitles got real tooltip keys in src/config/tooltips.ts. Verified locally with make reviewable (format/typecheck/lint + 668 tests green) and CI is green. Approving.
Optional, non-blocking: toErrorDetail is now copy-pasted in three views (ComponentsView, UsersView, AuthorizationsView) — a future cleanup could hoist it into a shared util.
|
PR approved. Marking as ready for e2e. |
gusfcarvalho
left a comment
There was a problem hiding this comment.
Re-opening after the earlier approval: manual verification on the dev stack surfaced one bug and one requested enhancement in the Diagrams section, both inline on DiagramsView.vue.
Headline: (1) diagrams created after the 404-reset never render until refresh — addDiagram/deleteDiagram mutate a plain object inside useDataApi's shallowRef; fix with immutable reassignment + regression test. (2) Give each diagram a card header: title from the OSCAL caption field (with inline edit that round-trips props — the drawio XML lives there), description subtitle, and per-diagram hide/show using v-show (v-if destroys unsaved drawio state).
Note: 'Add Diagram' on Network Architecture / Data Flow also fails server-side today because the API refuses to create diagrams in a missing grouping — that is fixed separately in compliance-framework/api#415 (auto-create grouping on first diagram POST). The UI changes here are independent of that PR landing, but manual e2e of Add-Diagram-on-empty-grouping needs it.
gusfcarvalho
left a comment
There was a problem hiding this comment.
d0ae3d5 implements both prior findings correctly — the addDiagram/deleteDiagram immutable reassignment, the caption title/edit with props round-tripping, and the v-show collapse all check out, and the caption-edit test genuinely asserts props preservation in the PUT body.
One finding inline: the Add-Diagram regression test doesn't actually pin the shallowRef bug — the test mock uses a deep ref() for data, under which the old push code also re-renders. I verified the test passes against the reverted buggy code. One-line fix: make the mock's data a shallowRef, mirroring the real useDataApi; verified that makes the test fail against the bug and pass against the fix with no collateral damage to the other 9 tests.
gusfcarvalho
left a comment
There was a problem hiding this comment.
All diagram findings addressed and verified. 1ce30b9 applies the exact prescribed fix — the useDataApi test mock now uses shallowRef, matching the production composable — which I had already verified both directions (test fails against the old push mutation, passes against the committed immutable-reassignment fix). make reviewable green locally (672 tests) and CI green. Approving.
Reminder for merge sequencing: 'Add Diagram' on a missing network-architecture/data-flow grouping also needs compliance-framework/api#415 (grouping auto-create) deployed to work end-to-end; the UI side is complete and independent.
|
PR approved. Marking as ready for e2e. |
automated implementation by lisa.