Skip to content

Add optional Mermaid flow-graph visualization to the PR comment - #81

Merged
sfc-gh-pvillard merged 4 commits into
mainfrom
add-flow-graph-visualization
Aug 8, 2026
Merged

Add optional Mermaid flow-graph visualization to the PR comment#81
sfc-gh-pvillard merged 4 commits into
mainfrom
add-flow-graph-visualization

Conversation

@sfc-gh-pvillard

Copy link
Copy Markdown
Collaborator

Summary

Adds an optional per-process-group Mermaid flow-graph to the PR comment, giving reviewers a visual of the structural changes alongside the existing textual description.

  • New flow-graph action input (default false — no change to existing behaviour when unset).
  • When enabled, each process group with structural changes (components/connections added or removed, or a connection rerouted) gets a collapsible flowchart overlaying the diff: 🟩 added / 🟥 removed / 🟧 modified / ⬜ unchanged. Node shapes distinguish processor (rectangle), port (rounded), funnel (circle); a processor's type shows as <Type> and a child-group port is tagged [group name].
  • Config-only groups keep the text description with no graph; a group whose graph would exceed a legibility/size limit falls back to text.
  • Graph logic lives in a new com.snowflake.openflow.graph.FlowGraph class, mirroring the checkstyle package layout — FlowDiff only wires it in.
  • Also makes the diff bullet ordering deterministic (content-based instead of the JVM identity hashCode), so the comment no longer reorders on unrelated code changes.

Example

flowchart TB
classDef added fill:#d1fae5,stroke:#16a34a,color:#166534;
classDef removed fill:#fee2e2,stroke:#dc2626,color:#991b1b,stroke-dasharray: 5 5;
classDef modified fill:#fef3c7,stroke:#d97706,color:#92400e;
classDef context fill:#e5e7eb,stroke:#6b7280,color:#374151;
n0["Generate<br>&#60;GenerateFlowFile&#62;"]:::modified
n1(["Ingest In"]):::added
n2["Old Sink"]:::removed
n0 -->|"success"| n1
n0 -.->|"success"| n2
Loading

Test plan

  • mvn clean package — build passes, 53 tests (added FlowGraphTest + 4 fixtures covering cross-group ports, funnel, reroute, oversize suppression, escaping, and config-only groups).
  • Verified on a real production connector PR: graph-enabled output renders correctly; graph-disabled output is unchanged in content.
  • Reviewer: sanity-check the rendered graph on a live PR comment.

Add a `flow-graph` action input (default false) that renders a per
process-group Mermaid flowchart of structural changes alongside the
existing textual diff. Graph logic lives in a new
com.snowflake.openflow.graph.FlowGraph class, mirroring the checkstyle
package layout; FlowDiff only wires it in.

Also make the diff bullet ordering deterministic (content-based instead
of the JVM identity hashCode), so the comment no longer reorders on
unrelated changes.

@sfc-gh-dchaffelson sfc-gh-dchaffelson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM — nicely self-contained (FlowGraph mirrors the checkstyle package layout, FlowDiff just wires it), safely default-off, thorough Mermaid escaping, and good test coverage of the graph behaviour incl. per-graph oversize suppression and config-only groups. Built + ran the suite locally on JDK 21: clean build, 54 tests pass. The deterministic-ordering change (dropping the JVM identity-hashCode tiebreaker) is a good fix on its own.

One non-blocking thing worth flagging before enabling this widely: the size guard is per-graph only (MAX_GRAPH_RENDERED_CHARS = 45000, plus the 20-node/40-edge caps). Since the comment emits one graph per structurally-changed process group on top of the existing text, there's no total-comment budget — a multi-group or large flow with flow-graph enabled could push the comment past GitHub's 65,536-char body limit. That's the same overflow as issue #63; this feature can make it worse when turned on. Default-off means no regression today, so not a blocker — just suggest coordinating enablement with a total-comment-size strategy (budget across graphs+text, or the multi-comment split from #63).

Minor: branch predates the recent main merges so it'll want a rebase; and the PR text says 'collapsible' while the code emits '

Details' (expanded by default).

@sfc-gh-pvillard
sfc-gh-pvillard merged commit b89c51b into main Aug 8, 2026
4 checks passed
@sfc-gh-pvillard
sfc-gh-pvillard deleted the add-flow-graph-visualization branch August 8, 2026 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants