oficina / sheets Public

Show comment text in Sheets threads

Each comment sat in rst-tblock, whose body rastrillo hides until a
toggle is checked, and the page had no toggle, so every thread showed
only author and time (B6). Comments are now plain always-open blocks
with token-only styling. Reviewed by Fable 5.1 on discussion #32.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Squashed from fix/r1-comment-bodies into main.

Paul Campbell pushed by claude 3 weeks ago 94be6b6664eb6154bf3958f2c858f09b880b5439 parent 336aa0d

5 files changed, +129 −6

internal/sheets/browser_comments_test.go +44 −0
internal/sheets/handlers_comments_test.go +19 −0
internal/sheets/static/sheets.css +22 −0
internal/sheets/templates/comments.html +7 −6
tools/drive/comments.mjs +37 −0

CI cancelled — superseded by a newer push — run details

diff --git a/internal/sheets/browser_comments_test.go b/internal/sheets/browser_comments_test.go

new file mode 100644

index 0000000..a4e3eee

--- /dev/null

+++ b/internal/sheets/browser_comments_test.go

@@ -0,0 +1,44 @@

+package sheets

+import (

+ "encoding/json"

+ "net/http"

+ "net/http/httptest"

+ "os"

+ "regexp"

+ "testing"

+// Opt-in fixture for tools/drive/comments.mjs: the real server-rendered

+// comments page and its stylesheets, with a synthetic owner, sheet and

+// comment. No session cookie is exported.

+func TestBrowserCommentsFixture(t *testing.T) {

+ path := os.Getenv("SHEETS_COMMENTS_FIXTURE")

+ if path == "" {

+ t.Skip("set SHEETS_COMMENTS_FIXTURE to render the browser fixture")

+ }

+ app, cookies := signIn(t)

+ s, _ := newSheet(t, app, cookies, "Budget")

+ addComment(t, app, s, cookies, 0, 0, "Check this total")

+ addComment(t, app, s, cookies, 0, 0, "Checked, it matches the ledger")

+ page := do(t, app, as(httptest.NewRequest(http.MethodGet, commentsPath(s), nil), cookies))

+ if page.Code != http.StatusOK {

+ t.Fatalf("comments page: %d", page.Code)

+ }

+ assets := map[string]string{}

+ for _, match := range regexp.MustCompile(`(?:src|href)="(/static/[^" ]+)"`).FindAllStringSubmatch(page.Body.String(), -1) {

+ asset := do(t, app, as(httptest.NewRequest(http.MethodGet, match[1], nil), cookies))

+ if asset.Code != http.StatusOK {

+ t.Fatalf("asset %s: %d", match[1], asset.Code)

+ }

+ assets[match[1]] = asset.Body.String()

+ }

+ data, err := json.Marshal(map[string]any{"html": page.Body.String(), "assets": assets,

+ "bodies": []string{"Check this total", "Checked, it matches the ledger"}})

+ if err != nil {

+ t.Fatal(err)

+ }

+ if err := os.WriteFile(path, data, 0600); err != nil {

+ t.Fatal(err)

+ }

diff --git a/internal/sheets/handlers_comments_test.go b/internal/sheets/handlers_comments_test.go

index c94f15f..82b9e5b 100644

--- a/internal/sheets/handlers_comments_test.go

+++ b/internal/sheets/handlers_comments_test.go

@@ -6,6 +6,7 @@ import (

"net/http"

"net/http/httptest"

"net/url"

+ "regexp"

"strings"

"testing"

)

@@ -243,3 +244,21 @@ func TestTheResolveFormPostsPositionsNotIDs(t *testing.T) {

body[max(0, strings.Index(body, "resolve")-300):])

}

+// B6: comment text rendered inside rst-tblock, a toggle-block whose body

+// is display:none until a switch in its head is checked. Comments have no

+// switch, so every thread showed an author, a time and an empty box. The

+// visible-in-a-browser proof is tools/drive/comments.mjs; this keeps the

+// markup from drifting back into a collapsed component in make test.

+func TestCommentTextIsNotInACollapsedToggleBlock(t *testing.T) {

+ app, cookies := signIn(t)

+ s, _ := newSheet(t, app, cookies, "Budget")

+ addComment(t, app, s, cookies, 0, 0, "Check this total")

+ body := commentsBody(t, app, s, cookies)

+ if strings.Contains(body, "rst-tblock") {

+ t.Error("comment entries render inside rst-tblock, whose body is hidden without a checked switch")

+ }

+ if !regexp.MustCompile(`<div data-comment>\s*<p><strong>[^<]+</strong>[\s\S]*?<p>Check this total</p>`).MatchString(body) {

+ t.Error("the comment text is not rendered as an always-open entry")

+ }

diff --git a/internal/sheets/static/sheets.css b/internal/sheets/static/sheets.css

index 294690c..c3e940a 100644

--- a/internal/sheets/static/sheets.css

+++ b/internal/sheets/static/sheets.css

@@ -1437,3 +1437,25 @@ form[data-doc-title]:not(:focus-within) button {

outline: 2px solid var(--rst-accent);

outline-offset: -2px;

}

+/* One entry in a comment thread (comments.html): author and time, then

+ the text, always shown. Entries after the first get a rule between. */

+[data-comment] > p {

+ margin: 0;

+[data-comment] > p + p {

+ margin-block-start: var(--rst-sp-1);

+ white-space: pre-line;

+[data-comment] + [data-comment] {

+ border-block-start: 1px solid var(--rst-line);

+ margin-block-start: var(--rst-sp-3);

+ padding-block-start: var(--rst-sp-3);

+[data-comment] [rst-row-sub] {

+ display: inline;

+ margin-inline-start: var(--rst-sp-2);

diff --git a/internal/sheets/templates/comments.html b/internal/sheets/templates/comments.html

index fe86cb4..d912de7 100644

--- a/internal/sheets/templates/comments.html

+++ b/internal/sheets/templates/comments.html

@@ -44,13 +44,14 @@ addresses them: row 0, column 0 is A1.</p>

{{if .Resolved}}<span rst-badge="neutral">resolved</span>{{end}}

</div>

+{{/* Always open. Not rst-tblock: that is a toggle-block whose body

+ stays display:none until a switch in its head is checked, and a

+ comment has no switch, so its text never showed. */}}

-<div rst-tblock>

-<div rst-tblock-head>

-<span rst-tblock-title>{{.Author}}</span>

-<span rst-row-sub>{{.When.Format "2 Jan 2006, 15:04"}}</span>

-</div>

-<div rst-tblock-body><p>{{.Body}}</p></div>

+<div data-comment>

+<p><strong>{{.Author}}</strong>

+<span rst-row-sub>{{.When.Format "2 Jan 2006, 15:04"}}</span></p>

+<p>{{.Body}}</p>

</div>

diff --git a/tools/drive/comments.mjs b/tools/drive/comments.mjs

new file mode 100644

index 0000000..de29a60

--- /dev/null

+++ b/tools/drive/comments.mjs

@@ -0,0 +1,37 @@

+// Comment bodies are visible on the comments page (B6: they rendered inside a

+// rastrillo toggle-block, whose body is display:none without a checked switch).

+// Real server-rendered page and stylesheets, mocked transport; no live data.

+// SHEETS_COMMENTS_FIXTURE=/tmp/comments.json go test ./internal/sheets -run TestBrowserCommentsFixture -count=1

+// PLAYWRIGHT=/path/to/playwright/index.mjs SHEETS_COMMENTS_FIXTURE=/tmp/comments.json node tools/drive/comments.mjs

+import assert from 'node:assert/strict';

+import fs from 'node:fs';

+const pw = await import(process.env.PLAYWRIGHT || 'playwright');

+const fixture = JSON.parse(fs.readFileSync(process.env.SHEETS_COMMENTS_FIXTURE));

+const origin = 'https://sheets.test';

+for (const name of ['chromium', 'webkit']) {

+ const options = name === 'webkit' && process.env.WEBKIT_EXECUTABLE_PATH ? {executablePath:process.env.WEBKIT_EXECUTABLE_PATH} : {};

+ const browser = await pw[name].launch(options);

+ try {

+ for (const viewport of [{width:1440,height:900},{width:390,height:844}]) {

+ const page = await browser.newPage({viewport});

+ await page.route(origin+'/**', async route => {

+ const path = new URL(route.request().url()).pathname;

+ if (path in fixture.assets) return route.fulfill({body:fixture.assets[path],contentType:

+ path.endsWith('.js')?'application/javascript':path.endsWith('.css')?'text/css':'image/svg+xml'});

+ return route.fulfill({body:fixture.html,contentType:'text/html'});

+ });

+ await page.goto(origin+'/sheets/1/comments');

+ for (const text of fixture.bodies) {

+ const body = page.getByText(text, {exact:true});

+ assert.equal(await body.count(), 1, `${name}: "${text}" rendered once`);

+ assert.ok(await body.isVisible(), `${name} ${viewport.width}px: "${text}" is visible`);

+ const box = await body.boundingBox();

+ assert.ok(box && box.width > 0 && box.height > 0, `${name}: "${text}" has a painted box`);

+ }

+ await page.close();

+ }

+ console.log(`${name}: comment bodies visible`);

+ } finally {

+ await browser.close();

+ }