feat: support per-lane workflow heights - #255
Conversation
|
Hello |
|
Thanks for the PR. The overall direction is good: adding per-lane height is appropriately scoped and is preferable to modeling one logical container as multiple lanes.
|
c5cae54 to
72ea590
Compare
|
Rebased this PR onto current Updated head: Validated:
|
tt-a1i
left a comment
There was a problem hiding this comment.
Thanks for addressing the earlier fixed-v1 group-containment and default-equivalence issues. The code-level boundary tests now look solid, but the feature still fails its intended browser acceptance case.
Using the Issue #250 shape as a readable-v2 workflow — one lane with height: 104, one group, stageA -> stageB -> stageC in the same column with yOffset: 0 / 90 / 180, bottom-to-top edges, and no explicit meta.viewBox — validate passes 9/9 and deliver passes with 0 errors / 0 warnings. However, visual-check fails every required desktop viewport due to vertical overflow:
- 1440x900 -> scrollHeight 1231 (Stage C is below the first screen)
- 1600x1000 -> scrollHeight 1359
- 1920x1080 -> scrollHeight 1359
- 2048x1320 -> scrollHeight 1377
This is the exact product shape the PR is meant to unlock, so artifact validation alone is not sufficient. Please make the tall-lane output fit the required desktop viewports without clipping or an internal scroller, either through height-aware Viewer scaling or an appropriate intrinsic canvas/aspect-ratio solution.
After fixing it, please regenerate the same three-stage artifact and attach:
- a corrected 1440x900 light screenshot corresponding to the currently failing view, with all three stages and the full group visible;
- the matching dark screenshot and 2048x1320 evidence;
- the
visual-check --jsonreceipt showing no overflow at 1440x900, 1600x1000, 1920x1080, and 2048x1320; - a browser regression covering this exact Issue #250 case;
- a rebuilt canonical
archify.zipand green exact-head CI.
Please keep deliver, automated browser containment, and perceptual visual review as separate evidence. Once the corrected artifact and receipt are available, I will re-review the new exact head.
| const groupHeaderHeights = groupLaneReserves.map(({ header }) => header); | ||
| const groupFooterHeights = groupLaneReserves.map(({ footer }) => footer); | ||
| const laneHeights = groupLaneReserves.map(({ header, footer }) => laneH + header + footer); | ||
| const laneHeights = groupLaneReserves.map(({ header, footer }, index) => ( |
There was a problem hiding this comment.
Blocking visual acceptance issue: the new per-lane height can produce a tall portrait viewBox while the Viewer still expands it primarily from the available width. The canonical Issue #250 three-stage stack passes validate and deliver, but visual-check reports vertical overflow at every required desktop viewport; at 1440x900 the page is 1231px tall and Stage C is below the first screen. Please make the resulting artifact height-aware (Viewer fit or intrinsic aspect ratio), add a browser regression for the exact three-stage repro, and attach corrected light/dark screenshots plus a passing four-viewport visual-check receipt.
tt-a1i
left a comment
There was a problem hiding this comment.
Thanks for addressing the earlier tall-workflow Viewer issue. I re-reviewed exact head 95635b1700199afe9afe4869bf549f9067111105, and the previous desktop-overflow finding is resolved in the tested reproduction.
On macOS with Node 22.19.0 and real Chrome, the following suites passed together: workflow-compiler.test.mjs, v1-compatibility.test.mjs, workflow-compiler-hard-contract.test.mjs, visual-check.test.mjs, and desktop-reader-browser.test.mjs — 152 passed, 0 failed, 0 skipped. This includes both the three-stage and five-stage browser regressions.
I also independently delivered the three-stage Issue #250 fixture and ran visual-check --json. At 1440x900, 1600x1000, 1920x1080, and 2048x1320, the page has no horizontal or vertical overflow, no internal diagram scroll range, and no clipped nodes or group frames. The receipt passes with no diagnostics. I inspected the 1440x900 light and dark screenshots: all three stages and the complete group are visible. There is still substantial blank space above the stack; this is a composition observation, not a renewed overflow blocker or a claim of complete perceptual acceptance.
One packaging defect still blocks merging:
[P1] Preserve the CLI executable bit in archify.zip. The committed archive records archify/bin/archify.mjs as 100644, while rebuilding the same head with Node 22.19.0 produces 100755. All 77 archived file contents match; the differing entry metadata is this executable mode. Consequently, cmp against the canonical rebuild fails, and the extracted entry point lacks the Unix executable bit. Please regenerate the archive with the tracked executable mode preserved and verify direct execution of the extracted CLI, in addition to Node-prefixed invocations.
Please sync with current main, resolve the binary archify.zip conflict by rebuilding from the final sources, verify byte-for-byte package freshness, and rerun the relevant checks and full suite. CI on this head is currently action_required, with no passing remote checks. Update the PR description with the current scope and validation evidence, including the Viewer and visual-check changes.
I did not run the full local npm test suite in this re-review. The earlier overflow objection is superseded by the successful browser evidence above; the remaining change request is for the package defect and outstanding integration/CI gates.
# Conflicts: # archify.zip
|
Addressed the latest P1 at
The PR description now follows the repository template and separates automated/browser evidence from perceptual review. GitHub Actions was triggered for this head but is |
|
Resolved the approved-run failures at exact head Root cause: the previous WSL2 packaging preparation preserved The replacement archive was built from a real WSL2 Git clone of head WSL2 validation for the corrected artifact:
The new exact-head workflow run is currently |
# Conflicts: # archify.zip
|
Synced this branch with current The only merge conflict was the binary Exact-head validation includes:
GitHub now reports the PR as mergeable with no content conflicts. Please approve/run the fork CI for |
tt-a1i
left a comment
There was a problem hiding this comment.
I compared current main ed7f4d4 with this PR at ab4908b, using the public CLI, real Chrome, and the delivered screenshots. The results narrow the product need, so I would like to pause merging and agree on scope before asking for another implementation round.
First, the earlier containment and Viewer fixes are acknowledged. The archive-mode defect was also verified fixed at a82c3e2. This comment is a design/scope discussion, not a renewed report of those resolved defects or an approval of all integration checks on the latest head.
What the comparison showed:
- The single-container, three-stage vertical workflow is already possible on main: use schema v2, omit the fixed
meta.viewBox, and center the authoredyOffsetvalues at-90 / 0 / 90. This preserves the group, stage order, spacing, and vertical connections.deliverpassed 9/9 checks, and the delivered artifact passed browser checks at 1440x900, 1600x1000, 1920x1080, and 2048x1320. I also inspected the screenshot. The original v1 JSON still fails unchanged on both main and this PR. - The clear new benefit is independent sizing in a mixed-lane workflow. In our three-lane reproduction, main produced lane heights
[276, 270, 270]and a768x1032canvas. Setting the stacked lane'sheightto104in this PR produced[276, 104, 104]and a768x700canvas, reducing canvas height by about 32%. Main overflowed the tested desktop viewports; the PR artifact passed all four. This is a useful, demonstrated improvement.
My proposed direction is to focus on independent automatic lane sizing in v2, rather than adding an authored-height API to solve the original single-lane example:
- Document the existing v2 solution for the original example, including the fact that
yOffsetis relative to the lane's content center. - Retain the independent-measurement work, but evaluate a v2 layout change without introducing
lane.heightin this iteration. Currentlyheight: 104acts as both a minimum and an opt-in to a different sizing policy, and can also enable Viewer fitting. That is a broader public contract than a height value alone suggests. - Establish the compatibility boundary before changing the default v2 behavior. Independent sizing can change existing geometry; explicit offsets, routes, fixed viewBoxes, and checked-in examples need before/after evidence. We should not silently reinterpret authored constraints to obtain a smaller canvas.
- Base Viewer fitting on the measured artifact and readability constraints, rather than the presence of an authored height field. Keep this work only as broad as the demonstrated layout need requires.
- Leave fixed-v1 unchanged for this feature. The general visual-check improvements for scrolling, clipping, and theme verification are useful independently and can be proposed separately.
If a real use case requires authors to specify a minimum lane height, we can evaluate that API separately with a single, explicit meaning.
Please start with feedback on this scope and the compatibility tradeoffs; there is no need to do another broad rewrite before we agree on the direction. We should have made this product-scope decision earlier in the review process. Passing tests and resolving the implementation findings are valuable, but we also need to agree on the public behavior we are committing to maintain before merging.
Refine readable-v2 independent lane measurement without adding public lane.height, preserve fixed-v1 and authored-geometry compatibility, and fit the Viewer from compiled geometry.
|
Addressed the latest review in What changed after the review:
Compatibility evidence now locked by tests:
Validation:
Ready for re-review. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (34)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 SummarySummaryAdds optional per-lane workflow heights with a 104px minimum. Readable-v2 now measures stacked nodes independently, sizes lane and group frames from authored or measured heights, validates containment, and reports structured diagnostics when members exceed rendered bounds. Fixed-v1 preserves existing geometry when Viewer fitting now supports measured intrinsic-height workflows. Generated examples, gallery artifacts, receipts, hashes, and the canonical ZIP were refreshed. Archive permissions now mark only ValidationAuthor-reported validation includes targeted and full test suites, real-Chrome viewport regressions, artifact freshness checks, executable archive extraction tests, and reproducible ZIP builds. The provided evidence does not include independently observed checks at the current head. Static test changes do not establish browser or perceptual acceptance. The latest GitHub Actions run was awaiting maintainer approval. WalkthroughReadable-v2 workflows now measure vertically stacked lanes independently. The compiler marks oversized implicit stacks as intrinsic-height diagrams. Adaptive reader shells use measured text size to select desktop widths. Tests, examples, documentation, and artifact receipts were regenerated. ChangesIntrinsic-height workflow layout
Priority: ➖ Normal — Schedule the per-lane height change because it broadly affects readable-v2 workflow layout, group containment, viewer fitting, and generated artifacts while addressing the medium-severity stacked-stage issue. Estimated code review effort: 4 (Complex) | ~60 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The intrinsic-height workflow path is covered through compilation, reader sizing, generated artifacts, and regressions, with no remaining merge-blocking issue identified. 🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
Full details: Contribution ScopeExplanation The PR description is complete in structure: it explains issue Resolution Align the PR description with the final implementation: describe inferred per-lane measurement, omit claims about the public Full details: Validation EvidenceExplanation Required validation evidence is incomplete, not a reproduced code defect. The PR adds appropriate compiler, Viewer, fixed-v1, containment, and Issue Resolution A maintainer must inspect the workflow and proposed code, approve the pending fork CI run, and rerun/complete all required final-head jobs for Warning Some tools did not complete. Review the errors below. 🔧 ast-grep (0.45.2)archify/examples/dataflow-product-analytics.htmlast-grep timed out on this file docs/gallery/artifacts/deployment-release.lifecycle.htmlast-grep timed out on this file archify/examples/lifecycle-agent-run.htmlast-grep timed out on this file
🔧 SkillSpector (2.9.5)SkillSpector batch scan produced no output 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 |
Problem and value
Closes #250 by allowing a single workflow lane to contain a vertical stage stack without introducing a multi-lane-group model. The result preserves fixed-v1 compatibility while readable-v2 can measure the affected lane independently and keep unrelated lanes compact.
Scope
lanes[].heightwith a 104px minimum.main(ed7f4d4, including fix(cli): prevent non-HTML output overwrites without restricting directories #322) in merge commitab4908band resolved the binary package conflict by rebuildingarchify.zipfrom the final combined source.archify/bin/archify.mjsis the single100755entry and the other 76 tracked files remain100644.755and run directly on Unix, in addition to Node-prefixed invocation.Stability impact
lanes[].heightretain their established behavior. Explicitheight: 104remains geometry-equivalent to omission in fixed-v1. Invalid authored heights fail with structured diagnostics rather than silently clipping group members.Tests run
Latest exact head
ab4908b, Windows development host, Node v22.23.2:node --test test/workflow-compiler.test.mjs test/v1-compatibility.test.mjs test/workflow-compiler-hard-contract.test.mjs test/visual-check.test.mjs test/desktop-reader-browser.test.mjs— all 149 non-browser tests and both Issue workflow: per-lane height / tall lanes for a vertically-stacked group (one container, N stacked stages) #250 browser acceptance tests passed; one production Chrome inspection timed out while four heavy suites were running concurrently.node --test test/desktop-reader-browser.test.mjs— 3 passed, 0 failed, 0 skipped. This covers the production reader plus the Issue workflow: per-lane height / tall lanes for a vertically-stacked group (one container, N stacked stages) #250 three-stage and five-stage workflows at all required desktop viewports.node --test test/cli-output-types.test.mjs— 9 passed, 0 failed, 4 symlink-permission skips. All non-symlink fix(cli): prevent non-HTML output overwrites without restricting directories #322 output-type cases passed.node --test test/cursor-onboarding.test.mjs— 2 passed, 0 failed; the extracted zero-dependency CLI runs directly and through Node.node scripts/generate-brand-marks.mjs --check,node scripts/generate-validators.mjs --check,node ../scripts/check-release-identity.mjs, andnode test/golden.mjs— passed.node --test test/gallery.test.mjs— 1 passed, 0 failed.CB9483FB06550FCEBDFCC1C2FBA95247400E057C32B6CA1D2D6FE807FC92A76D.0755mapping, MSYS conversion of Windows temp paths, and symlinkEPERM); the direct extracted-CLI smoke above passed.Prior feature head
a82c3e2was also validated in Ubuntu WSL2 with Node 22:npm testreported 1050 passed, 0 failed, and 4 conditional skips. Its remote CI run was green across Node 18/20/22/24, WebM/browser, ZIP freshness, and Ubuntu/macOS/Windows package smoke. Exact-head remote CI forab4908bis requested below.Visual evidence
visual-checkat all required desktop viewports; the generated Viewer and Gallery artifacts remain included in the PR.height: 104lane, a vertical stack in one group, default intrinsic viewBox, light and dark endpoint captures, and 1440x900 / 1600x1000 / 1920x1080 / 2048x1320 containment checks.Generated artifacts
archify.zip, with one executable CLI entry and 76 regular-file entries.archify/examples/*,examples/*,docs/gallery.html,docs/gallery/manifest.json,docs/gallery/artifacts/*, anddocs/assets/archify-live-proof.{gif,json}.Remote CI
a82c3e2: all required CI and package-smoke jobs passed.ab4908b: GitHub reportsMERGEABLE; CI run 34018218223 isaction_requiredand awaits maintainer approval before jobs can start.Checklist
npm testinarchify/.