Skip to content

lisa/feat/impl-system-ssp-refactor#261

Merged
gusfcarvalho merged 6 commits into
mainfrom
lisa/feat/impl-system-ssp-refactor
Jun 11, 2026
Merged

lisa/feat/impl-system-ssp-refactor#261
gusfcarvalho merged 6 commits into
mainfrom
lisa/feat/impl-system-ssp-refactor

Conversation

@ccf-lisa

@ccf-lisa ccf-lisa Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

automated implementation by lisa.

@gusfcarvalho gusfcarvalho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/views/system/OverviewView.vue Outdated
Comment thread src/views/system/DiagramsView.vue
Comment thread src/views/system/UsersView.vue Outdated
Comment thread src/views/system/AuthorizationsView.vue
Comment thread src/views/system/AuthorizationsView.vue

@gusfcarvalho gusfcarvalho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ccf-lisa

ccf-lisa Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

PR approved. Marking as ready for e2e.

@gusfcarvalho gusfcarvalho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/views/system/DiagramsView.vue
Comment thread src/views/system/DiagramsView.vue Outdated
@ccf-lisa ccf-lisa Bot removed the ready-for-e2e label Jun 11, 2026

@gusfcarvalho gusfcarvalho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/views/system/__tests__/systemViews.spec.ts

@gusfcarvalho gusfcarvalho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ccf-lisa

ccf-lisa Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

PR approved. Marking as ready for e2e.

@gusfcarvalho gusfcarvalho merged commit 431a116 into main Jun 11, 2026
2 checks passed
@gusfcarvalho gusfcarvalho deleted the lisa/feat/impl-system-ssp-refactor branch June 11, 2026 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant