TML-2733: address migration list --graph PR #628 review findings#636
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 53 minutes and 14 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThe PR refactors the migration list rendering system to become topology-aware and support configurable glyph modes (Unicode or ASCII). It renames type identifiers for clarity, adds lookahead-based graph layout optimizations, and centralizes glyph-mode detection as a reusable utility. ChangesTopology-Aware Migration List Rendering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
Comment |
size-limit report 📦
|
@prisma-next/extension-author-tools
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/extension-arktype-json
@prisma-next/extension-cipherstash
@prisma-next/middleware-cache
@prisma-next/mongo
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/extension-postgis
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/sqlite
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract
@prisma-next/utils
@prisma-next/config
@prisma-next/errors
@prisma-next/framework-components
@prisma-next/operations
@prisma-next/ts-render
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/cli-telemetry
@prisma-next/emitter
@prisma-next/migration-tools
prisma-next
@prisma-next/vite-plugin-contract-emit
@prisma-next/mongo-codec
@prisma-next/mongo-contract
@prisma-next/mongo-value
@prisma-next/mongo-contract-psl
@prisma-next/mongo-contract-ts
@prisma-next/mongo-emitter
@prisma-next/mongo-schema-ir
@prisma-next/mongo-query-ast
@prisma-next/mongo-orm
@prisma-next/mongo-query-builder
@prisma-next/mongo-lowering
@prisma-next/mongo-wire
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/target-sqlite
@prisma-next/adapter-postgres
@prisma-next/adapter-sqlite
@prisma-next/driver-postgres
@prisma-next/driver-sqlite
commit: |
… docs Rename EdgeKind → MigrationEdgeKind and MigrationGraphTopology → MigrationListGraphTopology. Sort still-WHITE nodes lexically before the second DFS pass so disjoint pure cycles seed deterministically. Correct project docs: the tolerant classifier re-implements adjacency + DFS, it does not reuse MigrationGraph or detectCycles. Signed-off-by: wmadden-electric <286902546+wmadden-electric@users.noreply.github.com>
Move migration-list-graph-layout into the CLI formatter layer and drop the migration-tools export. Rename joinBelow → joinAbove, implement rule-5 unwoven degrade when a forward producer sorts above its consumer, right-trim rendered graph lines, and collapse assignProducerLane. Signed-off-by: wmadden-electric <286902546+wmadden-electric@users.noreply.github.com>
Move GlyphMode detection to glyph-mode.ts. Share kind-glyph tables across flat and graph renderers; thread glyph mode into the flat list. Keep kind glyphs bright in both views. Classify topology once in the command and pass it into both render paths. Add per-space classification and ASCII flat-list tests. Signed-off-by: wmadden-electric <286902546+wmadden-electric@users.noreply.github.com>
…ests Restore optional topology argument on computeMigrationListGraphLayout for the command handoff. Drop hints/labels from migration-list command test packages to match current manifest validation. Signed-off-by: wmadden-electric <286902546+wmadden-electric@users.noreply.github.com>
… and plan Signed-off-by: Will Madden <madden@prisma.io>
- F03/AC17: rewrite the per-space test as a cross-space spurious cycle so it fails if classification reverts to global (the old fixture cycle was internal to one space and did not discriminate). - Route the graph-gutter kind glyph through style.kind so the bright-in-both policy has a single source of truth (output byte-identical; style.kind is identity). - Docs (C): scrub the surviving "reuse MigrationGraph / mirrors detectCycles" claims in design-notes and spec; the tolerant classifier is independent. Signed-off-by: Will Madden <madden@prisma.io>
wmadden
force-pushed
the
tml-2733-migration-list-graph-review-followups
branch
from
7fb1b71 to
93deb57
Compare
This was referenced May 30, 2026
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.
Linked issue
Refs TML-2733. Follow-up to TML-2702 (PR #628,
migration list --graph), batching that PR's open architect + principal-engineer review findings into one change.At a glance
--asciinow governs the flat list too, not just--graph. Both renderers share one kind-glyph table and select the Unicode or ASCII row by glyph mode:Previously the flat list hard-coded the Unicode glyphs regardless of
--ascii. That was the one real behaviour gap among the review findings; the rest of this PR is structural cleanup that leaves output byte-identical.Decision
Address every open finding from PR #628's review in a single follow-up, with three operator-confirmed calls and the rest at implementer discretion:
migration-toolsinto the CLI (architect Finding A) — view-model geometry (laneIndex,passThroughLanes, fan/join connectors) is a rendering concern, so it now lives with the renderer that consumes it.migration-toolskeeps only the topology (edge-kind + degrees), which is a genuine domain fact.MigrationGraph/detectCycles; it runs an independent tolerant pass. The docs claimed reuse the code never performed. Docs-only, by decision — the strict/tolerant split is deliberate and stays.The remaining findings (B, D, E, N1–N3, F02–F06) are folded in: glyph-mode capability relocation, a single kind-glyph table, flat-list
--ascii, the renames, per-component cycle seeding, a discriminating per-space test, bright kind glyphs in both views, trailing-whitespace trimming, and small readability cleanups.How it fits together
migration-tools). RenamedEdgeKind→MigrationEdgeKindandMigrationGraphTopology→MigrationListGraphTopology(the latter no longer reads as the strictMigrationGraph's topology). The pure-cycle DFS now seeds the still-unvisited remainder in lexical order before its second pass, so a graph with both a rooted component and a disjoint cycle picks the documented back-edge.migration-list-graph-layout.tsand its tests move intocli/.../formatters/; themigration-toolssubpath export,package.jsonentry, andtsdownentry are removed. Import direction is now strictly CLI →migration-tools. TheConnectorKindmemberjoinBelowbecomesjoinAboveto match the design's node-relative "join above / fan below" vocabulary.GlyphMode/detectGlyphModemove to aglyph-mode.tsthatterminal-uiowns (it no longer imports up into a formatter). The Unicode kind-glyph table lives once inmigration-list-data-columnand both renderers consume it. The command classifies topology once (buildMigrationListTopologyBySpace) and passes it into both render paths.renderMigrationListWithStyletakes aGlyphMode; the command threads the resolved mode to the flat path as well as the graph, so--asciiis honoured everywhere.What lands in this PR
7306ce7eb29dacjoinAbove, rule-5 unwoven degrade (F01), line trimming (F05), layout cleanups (F06)6b7fbeaglyph-mode.ts(B), shared kind glyphs (D), flat--ascii(E), bright kind glyphs (F04), classify-once handoff6ff4b89topologyparam; align command test manifests0a7016df54e6ccReviewer notes
app: X→Y,ext: Y→X) that forms a 2-cycle only when merged — reverting to global classification flips one edge to a rollback and fails the test. Seecli/test/commands/migration-list.test.ts.hasLaterForwardDepartingFrom), so normal linear chains and legitimate tips keep their woven lanes. The PE pass judged this more correct than the literal rule-5 wording.--asciiglyphs; the kind glyph routed through the styler, which is currently identity so goldens are unchanged).Verification
climigration-list suites (migration-list-graph-render,migration-list-render,migration-list-styler,commands/migration-list,migration-list-graph-layout): 97/97.migration-toolstopology suite: 14/14, package typecheck clean.pnpm --filter @prisma-next/cli typecheck(src + test): clean.pnpm lint:deps: clean. Biome on touched files: clean.version/removed-verb-redirectscli suites (cold-spawn timeouts) and somemigration-toolstest-only type errors (hints/labelsonMigrationMetadatain files this PR doesn't touch).Follow-ups
buildMigrationListTopologyBySpacecurrently lives on the flat-render module; relocate to a neutral home if a third consumer appears.Alternatives considered
detectCyclesand the tolerant classifier (architect Finding C's deeper fix). Rejected for this PR by decision: tolerance (never throw, no genesis assumption) is the point of the second pass, and unifying the DFS would entangle the strict path's throwing semantics with the offline list view. Corrected the docs instead.migration-toolsand prefix its exported types. Rejected — the settled design always placed lane geometry in the CLI, and the smaller domain-package surface is the cleaner outcome.Checklist
git commit -s) per the DCO.migration list --graphannotated-tree mode #628 review follow-ups).--ascii).TML-NNNN: <sentence-case title>form.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor