layout: route non-chain flows perpendicular to aging chains#444
layout: route non-chain flows perpendicular to aging chains#444
Conversation
When a stock has a chain flow (stock-to-stock) going right, non-chain outflows (stock-to-cloud) now exit from the bottom of the stock instead of overlapping the chain flow to the right. Symmetrically, non-chain inflows enter from the top when a chain inflow comes from the left. The core of the change is classify_flow_sides(), which separates a stock's flows into chain vs side categories and assigns each to a stock edge (Right, Left, Bottom, Top). Multiple flows on the same edge are distributed evenly using the (i+1)/(n+1) spacing formula, matching the TypeScript editor's computeFlowOffsets logic. This fixes the hiring model diagram where all waste flows were invisible because they occupied the exact same position as their corresponding chain flows.
Code ReviewReviewed the full diff: No bugs found. The implementation is clean and well-tested. A few observations (non-blocking):
Verdict: Correct. Existing tests and code will not break. The patch cleanly separates chain flows from waste/cloud flows and the test coverage is thorough. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64bf43b634
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(&pos) = initial_positions.get(flow_ident) { | ||
| let uid = state.get_or_alloc_uid(flow_ident); | ||
| create_flow_view_element(&mut state, &config, &metadata, flow_ident, uid, pos)?; | ||
| create_flow_view_element( |
There was a problem hiding this comment.
In the incremental path (src/simlin-engine/src/layout/mod.rs), new flows are still created from initial_positions computed by compute_new_element_positions (which seeds flows to the right of connected elements), but this commit now lets create_flow_view_element build vertical top/bottom geometries for side flows. For a newly added stock↔cloud side flow on a stock with a chain neighbor, that means the flow center (pos) is far from its generated points, and apply_layout_positions later preserves that offset, so the valve/label remains visually detached from the pipe after settling. Full layout avoids this because layout_chain explicitly computes vertical flow positions, but incremental layout does not.
Useful? React with 👍 / 👎.
Address review feedback for the incremental layout path: When a new chain flow is added to a stock that already has a preserved cloud flow on the right, the existing flow must be rebuilt to exit from the bottom. The incremental layout now classifies sides for all flows on affected stocks (not just new flows), detects orientation mismatches in preserved flows, and deletes/rebuilds them with correct geometry. Adds two integration tests: one for incrementally adding a waste flow (verifies it goes below), one for adding a chain flow that forces an existing cloud flow to change sides.
Code ReviewSummaryThis PR adds perpendicular routing for non-chain (cloud) flows when a stock also has chain (stock-to-stock) flows, preventing visual overlap. The core logic in FindingsNo bugs found. The logic is sound:
Test coverage is thorough: unit tests for classification logic, layout behavior tests for geometry, integration test against the hiring model, and incremental layout tests for both adding a waste flow and adding a chain flow that triggers rebuilding. VerdictCorrect. Existing code and tests will not break. The patch is free of bugs and blocking issues. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6e56b9fff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for flow_ident in &new_elements.new_flows { | ||
| let (from_stock, to_stock) = metadata.connected_stocks(flow_ident); | ||
| if let Some(stock) = from_stock { | ||
| affected_stocks.insert(stock.to_string()); |
There was a problem hiding this comment.
The reclassification pass only seeds affected_stocks from new_elements.new_flows, so deleting a chain flow never marks its stock for side recomputation. In incremental edits where a stock previously had chain+cloud outflows (cloud flow routed to Bottom), removing the chain flow leaves the existing cloud flow vertical instead of moving it back to the Right as full layout would. This creates persistent incremental/full-layout divergence after common delete operations.
Useful? React with 👍 / 👎.
| // Right/Left: use existing horizontal position | ||
| Position::new( | ||
| stock_pos.x | ||
| + config.stock_width / 2.0 | ||
| + config.horizontal_spacing / 2.0, |
There was a problem hiding this comment.
When rebuilding preserved flows, the fallback branch for Right/Left attachments always computes x using stock_pos.x + stock_width/2 + ..., which is the right side. If a preserved inflow is reclassified from Top back to Left (e.g., chain inflow removed while other flow changes trigger rebuild), the valve is rebuilt on the wrong side of the stock, producing incorrect geometry and connector angles.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #444 +/- ##
==========================================
+ Coverage 81.24% 81.32% +0.08%
==========================================
Files 173 173
Lines 48185 48815 +630
==========================================
+ Hits 39147 39699 +552
- Misses 9038 9116 +78 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Fix four issues with the incremental layout path for side flows: - Seed new top/bottom flows from their attachment point rather than the generic horizontal position, so the valve sits on the vertical pipe instead of off to the side. - Rebuild preserved side flows when their offset changes (adding a sibling on the same face redistributes from 0.5 to 1/3 + 2/3). - Reclassify sibling cloud flows when chain flows are removed, by including stocks from UpdateStockFlows patch ops in affected_stocks. Move the reclassification logic before the is_empty() early return so it runs even on deletion-only patches. - Preserve off-center top/bottom attachment x-positions through resnap by clamping rather than centering. Also addresses minor review feedback: extract attachment_based_flow_position helper to share between rebuilt and new flows, precompute flow_attachments once in layout_chain instead of calling classify_flow_sides redundantly, use explicit match arms instead of wildcards, and narrow assign_side parameter from &mut Vec to &mut [String].
Code ReviewI reviewed all changes in this PR — the No bugs found. Key areas I verified:
Verdict: Correct. Existing code and tests will not break, and the patch is free of blocking issues. |
Two fixes for the incremental layout path: - Stock-to-stock (chain) flows now use the generic initial_positions seed instead of attachment_based_flow_position, avoiding nondeterministic valve placement when the HashMap merge picks the wrong stock's attachment. - Preserved side flows are re-sorted by their existing attachment position (x for top/bottom, y for left/right) rather than alphabetical ident, preventing visual swaps when a sibling is added to imported or manually-edited views.
Code ReviewI've reviewed the full diff across all four changed files. The implementation is well-structured: Key observations:
No bugs found. The test coverage is thorough — 6 unit tests for classification, 4 layout behavior tests, and 1 integration test for the hiring model. The logic is consistent between the Overall correctness: correct ✓ |
- Detect affected stocks from DeleteVariable ops by scanning the old view for the deleted flow's stock attachment points. This ensures sibling flows are reclassified even when the patch omits UpdateStockFlows. - Honor Right/Left attachment offsets for horizontal cloud flows. Multiple cloud flows on the same horizontal side now get distinct y-positions along the stock edge via the (i+1)/(n+1) formula, matching the Bottom/Top behavior. Updated attachment_based_flow_position, create_flow_view_element, layout_chain BFS, resnap, and the incremental offset-change detection to handle both axes.
Code ReviewI reviewed the full diff across all 4 files (~2,862 lines added, 50 removed). The changes implement perpendicular routing for non-chain flows when a stock has a chain flow, with even spacing via No blocking issues foundThe implementation is well-structured:
The Overall correctness: CorrectThe patch does not break existing code or tests. The new behavior is well-tested and the edge cases are handled properly. |
Chain flows connecting two stocks have their pipe geometry determined solely by the two stock positions, ignoring the attachment offset. The offset-change rebuild check was incorrectly triggering rebuilds for chain flows when a sibling was added (changing the offset from 0.5 to 1/3), which would recreate the flow with a single-stock-based position and an off-center valve y. Skip stock-to-stock flows in the offset rebuild check since their geometry is independent of the attachment offset.
Code ReviewReviewed the full diff: ~717 lines of production code in SummaryThe change introduces flow-side classification (
Analysis
VerdictCorrect. No bugs found. The patch is well-structured, the edge cases are handled, and existing tests should continue to pass. The code correctly solves the flow overlap problem described in the PR. |
Three fixes for the incremental side-flow logic: - Exclude stock-to-stock (chain) flows from the entire rebuild check, not just the offset branch. An imported chain that happens to be more vertical than horizontal would otherwise trigger an orientation rebuild via attachment_based_flow_position, which only knows one stock and places the valve beside it instead of between the pair. - Exclude chain flows from reorder_attachments_by_position. Because incr_flow_attachments collapses per-stock classifications into a single FlowAttachment per flow, a chain flow may appear on the wrong side of one stock and skew sibling offset redistribution. - Match deleted flows by UID rather than canonicalized display name. Labels go through format_label_with_line_breaks which strips quoting, so canonicalizing the label can produce a different ident for names like "a.b".
Code ReviewI reviewed the full diff across all 4 files (~3,000 lines). The changes implement a well-structured approach to routing non-chain flows perpendicular to aging chains. No bugs foundThe logic is sound throughout:
Overall correctness: ✅ CorrectThe patch is well-tested (11 new tests covering full layout, incremental add/rebuild, offset redistribution, and the hiring model integration scenario), the logic is consistent between the full layout and incremental paths, and existing behavior is preserved for the non-multi-flow case. |
…ookup - Only call resnap_flow_endpoints when flows were actually rebuilt, to avoid rewriting imported/manual flow geometry on patches that only change equations or documentation. - Build stock_uid_to_ident from the model's stock variables rather than from view element labels, since labels go through format_label_with_line_breaks and may not round-trip through canonicalize for quoted names like "a.b".
Code ReviewReviewed the full diff (~3300 lines across 4 files). The approach is well-structured: Key observations:
No bugs found. Test coverage is thorough with 6 unit tests for classification, 4 layout behavior tests, 3 integration tests (including the hiring model), and all existing tests preserved. Overall correctness: ✅ Correct — existing code and tests will not break, and the patch is free of bugs and blocking issues. |
When attachment_based_flow_position cannot resolve a stock UID (e.g. imported views with quoted identifiers that do not round-trip through canonicalize), the rebuilt flow was silently dropped. Save the old flow position before deletion and use it as a fallback, ensuring the flow is always recreated.
Code ReviewI reviewed the full diff across all 4 changed files ( SummaryThis PR adds perpendicular routing for non-chain (cloud) flows when a stock already has a chain (stock-to-stock) flow. The approach is sound: FindingsNo bugs found. Key areas verified:
VerdictCorrect. The patch introduces no bugs or breaking changes. The new behavior is well-tested with 11+ new tests covering unit classification, full layout, incremental add/rebuild, and the hiring model integration scenario. Existing 151 tests continue to pass per the PR description. |
Rebuilt flows already have correct geometry from create_flow_view_element and there is no SFDP settlement step on this path. Remove resnap to avoid rewriting unrelated manual or imported flow endpoints elsewhere in the diagram when only a subset of flows are rebuilt.
Review cycle summaryEight review iterations addressed incremental layout correctness across multiple edge cases. The key improvements beyond the original two commits: Incremental layout fixes:
Full layout improvements:
Robustness:
Two known limitations filed as issues: #445 (chain direction inference for non-standard layouts) and #446 (reorder axis when flows change faces). |
Code ReviewI reviewed the full diff across 4 files (~2,200 lines). This PR introduces perpendicular routing for non-chain (cloud) flows when a stock already has a chain (stock-to-stock) flow, preventing visual overlap in aging chain models like the hiring model. Summary of changesCore logic (
Key correctness observations:
FindingsNo bugs found. The implementation is well-structured, internally consistent, and thoroughly tested (6 unit tests for Verdict: Correct ✓The patch is free of bugs and blocking issues. Existing tests and code will not break. |
Summary
(i+1)/(n+1)spacing formula, matching the TS editor'scomputeFlowOffsetslogicTest plan
classify_flow_sides()covering all combinations (chain+side, multiple sides, no chain, etc.)