feat(workflow-form): Form View page shell, load, and the shared spec harness - #8376
feat(workflow-form): Form View page shell, load, and the shared spec harness#8376yangzhang75 wants to merge 1 commit into
Conversation
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8376 +/- ##
============================================
- Coverage 94.02% 93.97% -0.05%
Complexity 4819 4819
============================================
Files 1202 1207 +5
Lines 48925 49093 +168
Branches 5942 5960 +18
============================================
+ Hits 46002 46136 +134
- Misses 1459 1486 +27
- Partials 1464 1471 +7
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
69507ad to
ea2e7ae
Compare
|
/request-review @mengw15 |
There was a problem hiding this comment.
🟡 Changes recommended
The new page is unreachable without route registration, and its feature-flag and error-handling behavior must be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds the initial read-only workflow Form View shell and reusable test harness.
Changes:
- Loads Form View workflows behind a feature flag.
- Adds navigation, collaborator display, and service cleanup.
- Adds shared mocks and unit tests.
File summaries
| File | Review |
|---|---|
frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts |
Adds reusable test mocks and fixtures. No issues found. |
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts |
Implements loading, redirects, read-only state, and cleanup. Critical: the component lacks route registration. Moderate: check the feature flag before requests and avoid misreporting generic failures as access errors. |
frontend/src/app/workspace/component/workflow-form/workflow-form.component.spec.ts |
Tests loading, redirects, read-only behavior, and cleanup. No issues found. |
frontend/src/app/workspace/component/workflow-form/workflow-form.component.scss |
Styles the shell and top bar. No issues found. |
frontend/src/app/workspace/component/workflow-form/workflow-form.component.html |
Defines the page shell and view switch. Nit: add required visual evidence to the PR description. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
mengw15
left a comment
mengw15
left a comment
There was a problem hiding this comment.
Two asks:
-
Please register the
workflow/:id/formroute here rather than in the final slice — the shell already guards it fully (flag off redirects to the canvas, and even with the flag on nothing can setdefault_view = FORMfrom the UI yet), so it exposes nothing while making this and every later slice testable at the real URL. Note it must sit aboveworkflow/:idso the segment isn't swallowed. -
With the route in, a screenshot (or short recording) of the shell in the description becomes cheap — and since the spec deliberately skips template rendering, it would be the review's only evidence of the rendered page, as #8318's screenshots were.
…harness The read-only shell of the Form View page: behind the feature flag it loads the workflow the URL names, redirects to the operator canvas when the workflow does not open in the form (or the flag is off), shows it read-only, hands back to the canvas, and releases the shared singleton services on the way out. The title bar's rename/save, the read-only preview, the inputs, running and results are added on top by later PRs. Also adds the shared spec harness (workflow-form.spec-harness.ts) these specs build the component from; later slices extend it with the mocks they introduce. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ea2e7ae to
a3ed02c
Compare
|
Thanks @mengw15. Both addressed in a3ed02c:
|
mengw15
left a comment
mengw15
left a comment
There was a problem hiding this comment.
Left one comment, plus a coverage ask: codecov has the patch at 74.5%, with 25 of the 26 missing lines in the template (the direct-construction specs never render it). Could you add a TestBed-rendered spec covering the template — the name/avatar row, the Canvas button actually triggering the switch, and the loading/body swap — and bring the patch as close to 100% as the v8-ignored navigation allows? That also replaces the screenshot as rendering evidence going forward.
| next: ({ workflow }) => { | ||
| // Even with the flag on, a workflow that opens on the operator canvas is sent | ||
| // there: reaching this URL for it lands on the canvas, not an empty page. | ||
| if (workflow.defaultView !== DefaultView.FORM) { |
There was a problem hiding this comment.
The #8011 closing note settled that the form renders for any workflow while the flag is on ("nothing redirects away from the form" — the default_view bit only picks the landing), yet this condition redirects a canvas-default workflow (undefined included) away, which quietly reintroduces the per-workflow gate: pr11's view switch would bounce straight back for any workflow whose default is the canvas. If it's temporary scaffolding while the body is empty, worth a comment saying which slice removes it; if it's meant to stay, it contradicts the settled model.
What changes were proposed in this PR?
The base of the Form View page (parent issue #8011): a new
/user/workflow/:id/formpage, of which this PR lays down the read-only shell. Frontend only.This PR is independent of the property-editor "choose exposed" PR (#8318) and builds directly on the merged foundation (feature flag #8013,
default_view#8014, shared types #8015, form-binding service #8016). The Form View is two parallel tracks -- the authoring affordance (#8318) and the reader page -- and this is the first slice of the reader-page track; the later page slices (title bar/save, read-only preview, rendering the inputs, running, results, authoring) all build on this component.Behind the
formViewEnabledflag it loads the workflow the URL names, and redirects to the operator canvas when the workflow does not open in the form (itsdefault_viewis notFORM) or the flag is off.Shows the workflow read-only (the graph and its properties are not edited here) and offers a switch back to the operator canvas via a full page load.
Releases the shared singleton services (graph, shared model, computing-unit connection, console, results) on destroy, so nothing left bound here follows the user to the next page.
Registers the
workflow/:id/formroute (above the canvasworkflow/:idroute, so the trailing segment is not swallowed), making the page reachable at its real URL. The page still guards itself, so nothing is exposed while the flag is off.It also adds
workflow-form.spec-harness.ts, the shared mock/factory the later page slices build their specs from; each slice extends it with the dependencies it introduces.Screenshot
Screenshot
The read-only page shell at
/user/workflow/:id/form(behind the flag, a form-default workflow). Since the spec deliberately skips template rendering, this is the rendered-page evidence for the view.Any related issues, documentation, discussions?
Closes #8018.
Part of the Form View feature (parent issue #8011); builds on the merged #8013/#8014/#8015/#8016. Independent of #8318.
How was this PR tested?
Unit tests (vitest):
workflow-form.component.spec.ts(7 tests). The page component injects ~14 services and heavy child components, so its spec constructs it directly with the shared harness's mocks rather than through TestBed; template rendering is therefore not exercised, but the component and the harness are at 100% of the TypeScript lines this PR adds. The template compiles cleanly underng build.Was this PR authored or co-authored using generative AI tooling?
Yes. Co-authored with Claude (Anthropic), reviewed line by line by the author before submission.