feat(Canvas): Filter placeholder nodes in screenshots and docs#3031
Merged
lordrip merged 1 commit intoKaotoIO:mainfrom Mar 16, 2026
Merged
feat(Canvas): Filter placeholder nodes in screenshots and docs#3031lordrip merged 1 commit intoKaotoIO:mainfrom
lordrip merged 1 commit intoKaotoIO:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR threads a new Changes
Sequence DiagramsequenceDiagram
participant User
participant ExportDoc as ExportDocument
participant HiddenCanvas as HiddenCanvas
participant FlowService as FlowService
participant DocService as DocumentationService
participant Browser as BrowserDownload
User->>ExportDoc: Click "Export"
ExportDoc->>HiddenCanvas: Render (isGeneratingImage=true)
HiddenCanvas->>FlowService: getFlowDiagram(entityVizNode, { removePlaceholder: true })
FlowService-->>HiddenCanvas: CanvasNodesAndEdges (placeholders removed)
HiddenCanvas->>HiddenCanvas: render & toBlob()
HiddenCanvas->>ExportDoc: onBlobGenerated(blob)
ExportDoc->>DocService: generateMarkdown(blob, filename)
DocService-->>ExportDoc: markdown
ExportDoc->>DocService: generateDocumentationZip(...)
DocService-->>ExportDoc: zipBlob
ExportDoc->>Browser: trigger download(zipBlob URL)
Browser-->>User: download kaoto-export.zip
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable the changed files summary in the walkthrough.Disable the |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3031 +/- ##
==========================================
+ Coverage 89.73% 89.99% +0.25%
==========================================
Files 564 564
Lines 21030 21038 +8
Branches 4926 4924 -2
==========================================
+ Hits 18872 18933 +61
+ Misses 2156 1980 -176
- Partials 2 125 +123 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/ui/src/components/Visualization/ContextToolbar/ExportDocument/ExportDocumentPreviewModal.test.tsx (1)
121-135: Assert that the export path enables placeholder filtering.These tests verify the async blob flow, but they never check that
FlowService.getFlowDiagramis called in placeholder-removal mode. That is the behavior this PR is meant to protect, so a regression there would still leave this suite green. Add an expectation on the mocked flow-service call afterGRAPH_LAYOUT_END_EVENT.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/ContextToolbar/ExportDocument/ExportDocumentPreviewModal.test.tsx` around lines 121 - 135, The test misses asserting that FlowService.getFlowDiagram is invoked in placeholder-removal mode; after triggering the GRAPH_LAYOUT_END_EVENT (i.e., after calling eventListenerCallback) add an expectation that FlowService.getFlowDiagram was called with the placeholder-filtering flag (for example verifying a call containing { removePlaceholders: true } or the specific arg your mock expects) so the test ensures the export path enables placeholder filtering; locate this in ExportDocumentPreviewModal.test.tsx around the GRAPH_LAYOUT_END_EVENT trigger and add the expect for FlowService.getFlowDiagram accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/components/Visualization/Canvas/flow.service.ts`:
- Around line 48-57: Filtering out placeholders can leave a group with zero
visible children while the original pre-filter structure still implies fallback
edges, causing missing prev->group edges; to fix, capture the original children
list before filtering (via vizNodeParam.getChildren()), then when
removePlaceholder is true and filtered children is empty but original
children.length > 0 and vizNodeParam.data.isGroup, still derive/append the
fallback edges for the group (call getEdgesFromVizNode() / appendEdges or invoke
appendNodesAndEdges on the group itself) so the prev->group edge is preserved;
apply the same change pattern wherever children are filtered (the blocks around
appendNodesAndEdges, getEdgesFromVizNode references) so empty-after-filter
groups retain their fallback edges.
In
`@packages/ui/src/components/Visualization/ContextToolbar/ExportDocument/ExportDocument.test.tsx`:
- Around line 49-57: The two waitFor blocks in ExportDocument.test.tsx that call
wrapper.findAllByTestId('export-document-preview-h1') and
wrapper.findAllByTestId('export-document-preview-table') drop their promises so
assertions may run after the test ends; fix by removing the outer waitFor and
directly awaiting the findAllByTestId calls (e.g., const headers = await
wrapper.findAllByTestId('export-document-preview-h1');
expect(headers).toHaveLength(3); and similarly for tables) so the test properly
awaits the assertions.
In
`@packages/ui/src/components/Visualization/ContextToolbar/ExportDocument/ExportDocumentPreviewModal.tsx`:
- Around line 68-74: The code creates object URLs with URL.createObjectURL in
handleBlobGenerated and in the download logic but never revokes them; update
handleBlobGenerated (and the download URL creation path) to revoke any previous
object URL before creating a new one (keep the current image URL in state, e.g.
flowImageUrl) and call URL.revokeObjectURL(prevUrl), revoke the generated
preview URL when the modal closes/unmounts, and revoke the temporary download
URL immediately after triggering the download (or after a short timeout) so
blobs are not retained; touch handleBlobGenerated, the download handler (where
downloadUrl is created), and the modal cleanup/unmount logic to implement this
revoke behavior.
---
Nitpick comments:
In
`@packages/ui/src/components/Visualization/ContextToolbar/ExportDocument/ExportDocumentPreviewModal.test.tsx`:
- Around line 121-135: The test misses asserting that FlowService.getFlowDiagram
is invoked in placeholder-removal mode; after triggering the
GRAPH_LAYOUT_END_EVENT (i.e., after calling eventListenerCallback) add an
expectation that FlowService.getFlowDiagram was called with the
placeholder-filtering flag (for example verifying a call containing {
removePlaceholders: true } or the specific arg your mock expects) so the test
ensures the export path enables placeholder filtering; locate this in
ExportDocumentPreviewModal.test.tsx around the GRAPH_LAYOUT_END_EVENT trigger
and add the expect for FlowService.getFlowDiagram accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 537caf28-35a6-45de-a62f-a34e81e658b1
⛔ Files ignored due to path filters (1)
packages/ui/src/components/Visualization/Canvas/__snapshots__/flow.service.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
packages/ui/src/components/Visualization/Canvas/flow.service.test.tspackages/ui/src/components/Visualization/Canvas/flow.service.tspackages/ui/src/components/Visualization/ContextToolbar/ExportDocument/ExportDocument.test.tsxpackages/ui/src/components/Visualization/ContextToolbar/ExportDocument/ExportDocument.tsxpackages/ui/src/components/Visualization/ContextToolbar/ExportDocument/ExportDocumentPreviewModal.test.tsxpackages/ui/src/components/Visualization/ContextToolbar/ExportDocument/ExportDocumentPreviewModal.tsxpackages/ui/src/components/Visualization/ContextToolbar/FlowExportImage/FlowExportImage.tsxpackages/ui/src/components/Visualization/ContextToolbar/FlowExportImage/HiddenCanvas.test.tsxpackages/ui/src/components/Visualization/ContextToolbar/FlowExportImage/HiddenCanvas.tsxpackages/ui/src/models/placeholder.constants.tspackages/ui/src/services/documentation.service.test.tspackages/ui/src/services/documentation.service.ts
💤 Files with no reviewable changes (2)
- packages/ui/src/services/documentation.service.ts
- packages/ui/src/services/documentation.service.test.ts
lordrip
force-pushed
the
feat/filter-placeholders
branch
from
585b224 to
17c2a29
Compare
igarashitm
previously approved these changes
Mar 12, 2026
Currently, there are multiple placeholder nodes across the Canvas making interacting with the flows easier. The downside is that when exporting screenshots or documentation about the flow, all these placeholders are included, making the picture more complicated than it needs to be. The fix is to filter all placeholders before drawing the graph. In addition to that, both `ExportDocumentPreviewModal` and `FlowExportImage` components should use the same `<HiddenCanvas>` component to have a consistent image. A follow-up is needed to consolidate all `placeholder-related` logic, because in some cases, the `placeholder` wording gets added to the path, while in others not. fix: KaotoIO#3029
lordrip
force-pushed
the
feat/filter-placeholders
branch
from
17c2a29 to
dac90f6
Compare
|
lordrip
commented
Mar 16, 2026
|
|
||
| Cypress.Commands.add('documentationTableCompare', (routeName: string, expectedTableData: string[][]) => { | ||
| // Wait until the loading distractor is no longer in the document | ||
| cy.get('[data-testid="Loading markdown preview"]', { timeout: 30_000 }).should('not.exist'); |
Member
Author
There was a problem hiding this comment.
@tplevko I added this timeout here to wait a bit longer because the doc feature takes more time in firefox
igarashitm
approved these changes
Mar 16, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/components/Visualization/ContextToolbar/ExportDocument/ExportDocumentPreviewModal.tsx (1)
91-107: Download URL cleanup is functional but could be more robust.The setTimeout-based revocation (lines 103-106) works but the timeout handle isn't stored/cleared if the component unmounts during the 100ms window. This is a minor issue since the revocation will still occur, but a warning may appear if the component unmounts very quickly after download.
♻️ Optional: Track and clear the timeout on unmount
+ const downloadTimeoutRef = useRef<ReturnType<typeof setTimeout>>(); + + useEffect(() => { + return () => { + if (downloadTimeoutRef.current) { + clearTimeout(downloadTimeoutRef.current); + } + }; + }, []); const onDownload = async () => { // ... link.click(); // Revoke the temporary download URL after a short timeout - setTimeout(() => { + downloadTimeoutRef.current = setTimeout(() => { URL.revokeObjectURL(downloadUrl); }, 100); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/ContextToolbar/ExportDocument/ExportDocumentPreviewModal.tsx` around lines 91 - 107, The onDownload handler revokes the temporary URL via setTimeout which can fire after the component unmounts; store the timeout ID (e.g., in a ref) when you call setTimeout inside ExportDocumentPreviewModal.onDownload and clear it in a cleanup (useEffect return) when the component unmounts, and also call URL.revokeObjectURL(downloadUrl) immediately if possible after link.click() to be safe; ensure the cleanup clears the stored timeout and revokes any remaining downloadUrl to avoid post-unmount operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/ui/src/components/Visualization/ContextToolbar/ExportDocument/ExportDocumentPreviewModal.tsx`:
- Around line 91-107: The onDownload handler revokes the temporary URL via
setTimeout which can fire after the component unmounts; store the timeout ID
(e.g., in a ref) when you call setTimeout inside
ExportDocumentPreviewModal.onDownload and clear it in a cleanup (useEffect
return) when the component unmounts, and also call
URL.revokeObjectURL(downloadUrl) immediately if possible after link.click() to
be safe; ensure the cleanup clears the stored timeout and revokes any remaining
downloadUrl to avoid post-unmount operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 420368b6-340c-4493-9247-315c411b7f58
⛔ Files ignored due to path filters (1)
packages/ui/src/components/Visualization/Canvas/__snapshots__/flow.service.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
packages/ui-tests/cypress/support/next-commands/nodeConfiguration.tspackages/ui/src/components/Visualization/Canvas/flow.service.test.tspackages/ui/src/components/Visualization/Canvas/flow.service.tspackages/ui/src/components/Visualization/ContextToolbar/ExportDocument/ExportDocument.test.tsxpackages/ui/src/components/Visualization/ContextToolbar/ExportDocument/ExportDocument.tsxpackages/ui/src/components/Visualization/ContextToolbar/ExportDocument/ExportDocumentPreviewModal.test.tsxpackages/ui/src/components/Visualization/ContextToolbar/ExportDocument/ExportDocumentPreviewModal.tsxpackages/ui/src/components/Visualization/ContextToolbar/FlowExportImage/FlowExportImage.tsxpackages/ui/src/components/Visualization/ContextToolbar/FlowExportImage/HiddenCanvas.test.tsxpackages/ui/src/components/Visualization/ContextToolbar/FlowExportImage/HiddenCanvas.tsxpackages/ui/src/models/placeholder.constants.tspackages/ui/src/services/documentation.service.test.tspackages/ui/src/services/documentation.service.ts
💤 Files with no reviewable changes (2)
- packages/ui/src/services/documentation.service.ts
- packages/ui/src/services/documentation.service.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/ui/src/components/Visualization/ContextToolbar/FlowExportImage/FlowExportImage.tsx
- packages/ui/src/models/placeholder.constants.ts
- packages/ui/src/components/Visualization/Canvas/flow.service.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Currently, there are multiple placeholder nodes across the Canvas making interacting with the flows easier. The downside is that when exporting screenshots or documentation about the flow, all these placeholders are included, making the picture more complicated than it needs to be.
Changes
The fix is to filter all placeholders before drawing the graph. In addition to that, both
ExportDocumentPreviewModalandFlowExportImagecomponents should use the same<HiddenCanvas>component to have a consistent image.Note
A follow-up is needed to consolidate all
placeholder-relatedlogic, because in some cases, theplaceholderwording gets added to the path, while in others not.Screenshots
Canvas view
fix: #3029
Summary by CodeRabbit
New Features
Improvements
Tests