fix(web): split-only note links and disjoint heading ids of split note copies
4 files · +14 -8
M internal/adapters/web/jstest/mr.test.mjs +2 -2
| @@ -63,7 +63,7 @@ test('quote opens the thread reply and prefills and focuses its textarea', () => | |||
| 63 | 63 | assert.equal(focused,true); | |
| 64 | 64 | }); | |
| 65 | 65 | ||
| 66 | - | test('reveal selects the split note, including after lazy loading', async () => { | |
| 66 | + | for (const dataset of [{diff:'split'}, {view:'split'}]) test(`reveal selects the split note (${JSON.stringify(dataset)}), including after lazy loading`, async () => { | |
| 67 | 67 | const saved = globalThis.document; | |
| 68 | 68 | const savedRAF = globalThis.requestAnimationFrame; | |
| 69 | 69 | let scrolled = ''; | |
| @@ -72,7 +72,7 @@ test('reveal selects the split note, including after lazy loading', async () => | |||
| 72 | 72 | const nodes = new Map([['c12',node('c12')],['c12-s',node('c12-s')]]); | |
| 73 | 73 | const ph = {dataset:{notes:'c12'}}; | |
| 74 | 74 | globalThis.document = { | |
| 75 | - | querySelector: sel => sel === '#diffs' ? {dataset:{diff:'split'}} : null, | |
| 75 | + | querySelector: sel => sel === '#diffs' ? {dataset} : null, | |
| 76 | 76 | querySelectorAll: () => [ph], | |
| 77 | 77 | getElementById: id => nodes.get(id), | |
| 78 | 78 | }; | |
| @@ -63,7 +63,7 @@ test('quote opens the thread reply and prefills and focuses its textarea', () => | |||||
| 63 | assert.equal(focused,true); | 63 | assert.equal(focused,true); | ||
| 64 | }); | 64 | }); | ||
| 65 | 65 | ||||
| 66 | - | test('reveal selects the split note, including after lazy loading', async () => { | 66 | + | for (const dataset of [{diff:'split'}, {view:'split'}]) test(`reveal selects the split note (${JSON.stringify(dataset)}), including after lazy loading`, async () => { |
| 67 | const saved = globalThis.document; | 67 | const saved = globalThis.document; | ||
| 68 | const savedRAF = globalThis.requestAnimationFrame; | 68 | const savedRAF = globalThis.requestAnimationFrame; | ||
| 69 | let scrolled = ''; | 69 | let scrolled = ''; | ||
| @@ -72,7 +72,7 @@ test('reveal selects the split note, including after lazy loading', async () => | |||||
| 72 | const nodes = new Map([['c12',node('c12')],['c12-s',node('c12-s')]]); | 72 | const nodes = new Map([['c12',node('c12')],['c12-s',node('c12-s')]]); | ||
| 73 | const ph = {dataset:{notes:'c12'}}; | 73 | const ph = {dataset:{notes:'c12'}}; | ||
| 74 | globalThis.document = { | 74 | globalThis.document = { | ||
| 75 | - | querySelector: sel => sel === '#diffs' ? {dataset:{diff:'split'}} : null, | 75 | + | querySelector: sel => sel === '#diffs' ? {dataset} : null, |
| 76 | querySelectorAll: () => [ph], | 76 | querySelectorAll: () => [ph], | ||
| 77 | getElementById: id => nodes.get(id), | 77 | getElementById: id => nodes.get(id), | ||
| 78 | }; | 78 | }; | ||
M internal/adapters/web/mr.go +1 -1
| @@ -813,7 +813,7 @@ func (s *Server) placeComments(p *MRPage, repo string, n int64, comments []domai | |||
| 813 | 813 | if err != nil { | |
| 814 | 814 | body = "" | |
| 815 | 815 | } | |
| 816 | - | bodySplit, err := s.md.renderNote([]byte(c.Text.String()), repo, head, "c"+cid+"-s-", exists) | |
| 816 | + | bodySplit, err := s.md.renderNote([]byte(c.Text.String()), repo, head, "c"+cid+"s-", exists) | |
| 817 | 817 | if err != nil { | |
| 818 | 818 | bodySplit = "" | |
| 819 | 819 | } | |
| @@ -813,7 +813,7 @@ func (s *Server) placeComments(p *MRPage, repo string, n int64, comments []domai | |||||
| 813 | if err != nil { | 813 | if err != nil { | ||
| 814 | body = "" | 814 | body = "" | ||
| 815 | } | 815 | } | ||
| 816 | - | bodySplit, err := s.md.renderNote([]byte(c.Text.String()), repo, head, "c"+cid+"-s-", exists) | 816 | + | bodySplit, err := s.md.renderNote([]byte(c.Text.String()), repo, head, "c"+cid+"s-", exists) |
| 817 | if err != nil { | 817 | if err != nil { | ||
| 818 | bodySplit = "" | 818 | bodySplit = "" | ||
| 819 | } | 819 | } | ||
M internal/adapters/web/mr_notes_test.go +7 -4
| @@ -141,7 +141,9 @@ func TestDeletedNoteRendering(t *testing.T) { | |||
| 141 | 141 | ||
| 142 | 142 | func TestInlineNoteHeadingIDs(t *testing.T) { | |
| 143 | 143 | t.Parallel() | |
| 144 | - | text, err := domain.ParsePlainText("# H\n[jump](#h)") | |
| 144 | + | // "S H" would get the id the split copy of "H" gets if the prefixes of the | |
| 145 | + | // two copies overlapped. | |
| 146 | + | text, err := domain.ParsePlainText("# H\n\n# S H\n\n[jump](#h)") | |
| 145 | 147 | require.NoError(t, err) | |
| 146 | 148 | s := &Server{cfg: Config{Now: time.Now}, md: newMarkdown()} | |
| 147 | 149 | p := MRPage{Files: []DiffFile{{Key: "a", ID: "f1", Hunks: []DiffHunk{{Lines: []DiffLine{{Kind: "add", newN: 1}}}}}}} | |
| @@ -152,8 +154,9 @@ func TestInlineNoteHeadingIDs(t *testing.T) { | |||
| 152 | 154 | require.NoError(t, commentView(notes[0], "").Render(context.Background(), &b)) | |
| 153 | 155 | require.NoError(t, commentView(notes[0], "-s").Render(context.Background(), &b)) | |
| 154 | 156 | html := b.String() | |
| 155 | - | assert.Equal(t, 1, strings.Count(html, `id="c12-h"`)) | |
| 156 | - | assert.Equal(t, 1, strings.Count(html, `id="c12-s-h"`)) | |
| 157 | + | for _, id := range []string{"c12-h", "c12-s-h", "c12s-h", "c12s-s-h"} { | |
| 158 | + | assert.Equal(t, 1, strings.Count(html, `id="`+id+`"`), id) | |
| 159 | + | } | |
| 157 | 160 | assert.Contains(t, html, `href="#c12-h"`) | |
| 158 | - | assert.Contains(t, html, `href="#c12-s-h"`) | |
| 161 | + | assert.Contains(t, html, `href="#c12s-h"`) | |
| 159 | 162 | } | |
| @@ -141,7 +141,9 @@ func TestDeletedNoteRendering(t *testing.T) { | |||||
| 141 | 141 | ||||
| 142 | func TestInlineNoteHeadingIDs(t *testing.T) { | 142 | func TestInlineNoteHeadingIDs(t *testing.T) { | ||
| 143 | t.Parallel() | 143 | t.Parallel() | ||
| 144 | - | text, err := domain.ParsePlainText("# H\n[jump](#h)") | 144 | + | // "S H" would get the id the split copy of "H" gets if the prefixes of the |
| 145 | + | // two copies overlapped. | |||
| 146 | + | text, err := domain.ParsePlainText("# H\n\n# S H\n\n[jump](#h)") | |||
| 145 | require.NoError(t, err) | 147 | require.NoError(t, err) | ||
| 146 | s := &Server{cfg: Config{Now: time.Now}, md: newMarkdown()} | 148 | s := &Server{cfg: Config{Now: time.Now}, md: newMarkdown()} | ||
| 147 | p := MRPage{Files: []DiffFile{{Key: "a", ID: "f1", Hunks: []DiffHunk{{Lines: []DiffLine{{Kind: "add", newN: 1}}}}}}} | 149 | p := MRPage{Files: []DiffFile{{Key: "a", ID: "f1", Hunks: []DiffHunk{{Lines: []DiffLine{{Kind: "add", newN: 1}}}}}}} | ||
| @@ -152,8 +154,9 @@ func TestInlineNoteHeadingIDs(t *testing.T) { | |||||
| 152 | require.NoError(t, commentView(notes[0], "").Render(context.Background(), &b)) | 154 | require.NoError(t, commentView(notes[0], "").Render(context.Background(), &b)) | ||
| 153 | require.NoError(t, commentView(notes[0], "-s").Render(context.Background(), &b)) | 155 | require.NoError(t, commentView(notes[0], "-s").Render(context.Background(), &b)) | ||
| 154 | html := b.String() | 156 | html := b.String() | ||
| 155 | - | assert.Equal(t, 1, strings.Count(html, `id="c12-h"`)) | 157 | + | for _, id := range []string{"c12-h", "c12-s-h", "c12s-h", "c12s-s-h"} { |
| 156 | - | assert.Equal(t, 1, strings.Count(html, `id="c12-s-h"`)) | 158 | + | assert.Equal(t, 1, strings.Count(html, `id="`+id+`"`), id) |
| 159 | + | } | |||
| 157 | assert.Contains(t, html, `href="#c12-h"`) | 160 | assert.Contains(t, html, `href="#c12-h"`) | ||
| 158 | - | assert.Contains(t, html, `href="#c12-s-h"`) | 161 | + | assert.Contains(t, html, `href="#c12s-h"`) |
| 159 | } | 162 | } | ||
M internal/adapters/web/static/js/mr.js +4 -1
| @@ -127,7 +127,10 @@ window.ermiteMR = (() => { | |||
| 127 | 127 | // Any navigation, loaded or not, makes the reveals still loading stale. | |
| 128 | 128 | const gen = ++revealGen; | |
| 129 | 129 | const target = () => { | |
| 130 | - | if (/^c\d+$/.test(id) && $('#diffs')?.dataset.diff === 'split') { | |
| 130 | + | // A page with both layouts says which shows in data-diff; a large diff | |
| 131 | + | // rendered in one layout only says it in data-view. | |
| 132 | + | const diffs = $('#diffs')?.dataset; | |
| 133 | + | if (/^c\d+$/.test(id) && (diffs?.diff || diffs?.view) === 'split') { | |
| 131 | 134 | const split = document.getElementById(id + '-s'); | |
| 132 | 135 | if (split) return split; | |
| 133 | 136 | } | |
| @@ -127,7 +127,10 @@ window.ermiteMR = (() => { | |||||
| 127 | // Any navigation, loaded or not, makes the reveals still loading stale. | 127 | // Any navigation, loaded or not, makes the reveals still loading stale. | ||
| 128 | const gen = ++revealGen; | 128 | const gen = ++revealGen; | ||
| 129 | const target = () => { | 129 | const target = () => { | ||
| 130 | - | if (/^c\d+$/.test(id) && $('#diffs')?.dataset.diff === 'split') { | 130 | + | // A page with both layouts says which shows in data-diff; a large diff |
| 131 | + | // rendered in one layout only says it in data-view. | |||
| 132 | + | const diffs = $('#diffs')?.dataset; | |||
| 133 | + | if (/^c\d+$/.test(id) && (diffs?.diff || diffs?.view) === 'split') { | |||
| 131 | const split = document.getElementById(id + '-s'); | 134 | const split = document.getElementById(id + '-s'); | ||
| 132 | if (split) return split; | 135 | if (split) return split; | ||
| 133 | } | 136 | } | ||