From 91938396905a44c62adab79aeb24c8f6eeec1f82 Mon Sep 17 00:00:00 2001 From: AdamMagued Date: Mon, 5 Oct 2026 06:18:33 +0000 Subject: [PATCH] d2sequence: account for self and sibling message height in note placement When positioning notes in sequence diagrams, vertical offsets only accounted for self-messages where source matched destination directly. Messages between spans of the same actor or descendant/sibling relations also consume extra vertical space for loop routing. Because note placement ignored that space, notes appearing after sibling messages were placed too high, causing subsequent groups to overlap preceding groups. Extract an isSelfOrSibling helper shared by routeMessages and placeNotes, and account for the full height of self and sibling messages during note positioning. Add tests covering group overlap prevention with notes. --- d2layouts/d2sequence/layout_test.go | 123 +++++++++++++++++++++++ d2layouts/d2sequence/sequence_diagram.go | 41 ++++---- 2 files changed, 147 insertions(+), 17 deletions(-) diff --git a/d2layouts/d2sequence/layout_test.go b/d2layouts/d2sequence/layout_test.go index ac79e510b4..415d7827f0 100644 --- a/d2layouts/d2sequence/layout_test.go +++ b/d2layouts/d2sequence/layout_test.go @@ -471,3 +471,126 @@ func TestSequenceToDescendant(t *testing.T) { t.Fatal("expected route to end at `a` lifeline") } } + +func TestGroupStartsWithNoteOverlap(t *testing.T) { + input := ` +shape: sequence_diagram + +job + +group1: { + job.t1 -> job.t2: one + job.t1 -> job.t3: two +} +group2: { + job.t1."overlap with group1" +} +` + ctx := log.With(context.Background(), testlog.New(t)) + g, _, err := d2compiler.Compile("", strings.NewReader(input), nil) + assert.Nil(t, err) + + for _, obj := range g.Objects { + if obj.Box == nil { + obj.Box = geo.NewBox(nil, 100, 100) + } + } + + err = d2sequence.Layout(ctx, g, func(ctx context.Context, g *d2graph.Graph) error { + return nil + }) + assert.Nil(t, err) + + group1, has := g.Root.HasChild([]string{"group1"}) + assert.True(t, has) + group2, has := g.Root.HasChild([]string{"group2"}) + assert.True(t, has) + + assert.GreaterOrEqual(t, group2.TopLeft.Y, group1.TopLeft.Y+group1.Height, + "group2 (top %v) must not overlap group1 (bottom %v)", group2.TopLeft.Y, group1.TopLeft.Y+group1.Height) +} + +func TestGroupStartsWithNoteFollowedByMessages(t *testing.T) { + input := ` +shape: sequence_diagram + +a +b + +group1: { + a."step one note" + a -> b: step one +} +group2: { + b."step two note" + b -> a: step two +} +` + ctx := log.With(context.Background(), testlog.New(t)) + g, _, err := d2compiler.Compile("", strings.NewReader(input), nil) + assert.Nil(t, err) + + for _, obj := range g.Objects { + if obj.Box == nil { + obj.Box = geo.NewBox(nil, 100, 100) + } + } + + err = d2sequence.Layout(ctx, g, func(ctx context.Context, g *d2graph.Graph) error { + return nil + }) + assert.Nil(t, err) + + group1, has := g.Root.HasChild([]string{"group1"}) + assert.True(t, has) + group2, has := g.Root.HasChild([]string{"group2"}) + assert.True(t, has) + + assert.GreaterOrEqual(t, group2.TopLeft.Y, group1.TopLeft.Y+group1.Height, + "group2 (top %v) must not overlap group1 (bottom %v)", group2.TopLeft.Y, group1.TopLeft.Y+group1.Height) +} + +func TestMultipleGroupsStartingWithNotes(t *testing.T) { + input := ` +shape: sequence_diagram + +srv + +g1: { + srv."init config" +} +g2: { + srv."start worker" +} +g3: { + srv."listen on port" +} +` + ctx := log.With(context.Background(), testlog.New(t)) + g, _, err := d2compiler.Compile("", strings.NewReader(input), nil) + assert.Nil(t, err) + + for _, obj := range g.Objects { + if obj.Box == nil { + obj.Box = geo.NewBox(nil, 100, 100) + } + } + + err = d2sequence.Layout(ctx, g, func(ctx context.Context, g *d2graph.Graph) error { + return nil + }) + assert.Nil(t, err) + + g1, has := g.Root.HasChild([]string{"g1"}) + assert.True(t, has) + g2, has := g.Root.HasChild([]string{"g2"}) + assert.True(t, has) + g3, has := g.Root.HasChild([]string{"g3"}) + assert.True(t, has) + + assert.GreaterOrEqual(t, g2.TopLeft.Y, g1.TopLeft.Y+g1.Height, + "g2 (top %v) must not overlap g1 (bottom %v)", g2.TopLeft.Y, g1.TopLeft.Y+g1.Height) + assert.GreaterOrEqual(t, g3.TopLeft.Y, g2.TopLeft.Y+g2.Height, + "g3 (top %v) must not overlap g2 (bottom %v)", g3.TopLeft.Y, g2.TopLeft.Y+g2.Height) +} + diff --git a/d2layouts/d2sequence/sequence_diagram.go b/d2layouts/d2sequence/sequence_diagram.go index 58711f9478..d217acc3c5 100644 --- a/d2layouts/d2sequence/sequence_diagram.go +++ b/d2layouts/d2sequence/sequence_diagram.go @@ -478,6 +478,27 @@ func IsLifelineEnd(obj *d2graph.Object) bool { return obj.ID == LifelineEndID(obj.ID[:markerIndex]) } +func isSelfOrSibling(message *d2graph.Edge) bool { + if message.Src == message.Dst { + return true + } + if strings.HasPrefix(message.Dst.AbsID(), message.Src.AbsID()+".") { + return true + } + if strings.HasPrefix(message.Src.AbsID(), message.Dst.AbsID()+".") { + return true + } + currSrc := message.Src + for !currSrc.Parent.IsSequenceDiagram() { + currSrc = currSrc.Parent + } + currDst := message.Dst + for !currDst.Parent.IsSequenceDiagram() { + currDst = currDst.Parent + } + return currSrc == currDst +} + func (sd *sequenceDiagram) placeNotes() { rankToX := make(map[int]float64) for _, actor := range sd.actors { @@ -490,8 +511,8 @@ func (sd *sequenceDiagram) placeNotes() { for _, msg := range sd.messages { if sd.verticalIndices[msg.AbsID()] < verticalIndex { - if msg.Src == msg.Dst { - // For self-messages, account for the full vertical space they occupy + if isSelfOrSibling(msg) { + // For self/sibling messages, account for the full vertical space they occupy y += sd.yStep + math.Max(float64(msg.LabelDimensions.Height), MIN_MESSAGE_DISTANCE)*1.5 } else { y += sd.yStep + float64(msg.LabelDimensions.Height) @@ -609,21 +630,7 @@ func (sd *sequenceDiagram) routeMessages() error { } else { return fmt.Errorf("could not find center of %s. Is it declared as an actor?", message.Dst.ID) } - isToDescendant := strings.HasPrefix(message.Dst.AbsID(), message.Src.AbsID()+".") - isFromDescendant := strings.HasPrefix(message.Src.AbsID(), message.Dst.AbsID()+".") - isSelfMessage := message.Src == message.Dst - - currSrc := message.Src - for !currSrc.Parent.IsSequenceDiagram() { - currSrc = currSrc.Parent - } - currDst := message.Dst - for !currDst.Parent.IsSequenceDiagram() { - currDst = currDst.Parent - } - isToSibling := currSrc == currDst - - if isSelfMessage || isToDescendant || isFromDescendant || isToSibling { + if isSelfOrSibling(message) { midX := startX + math.Max(SELF_MESSAGE_HORIZONTAL_TRAVEL, float64(message.LabelDimensions.Width)/2.+label.PADDING*2) startY := messageOffset + noteOffset endY := startY + math.Max(float64(message.LabelDimensions.Height), MIN_MESSAGE_DISTANCE)*1.5