Skip to content

feat(docs): honor --tab when setting page layout#878

Merged
steipete merged 1 commit into
openclaw:mainfrom
atmasphere:docs-page-layout-tab
Jun 24, 2026
Merged

feat(docs): honor --tab when setting page layout#878
steipete merged 1 commit into
openclaw:mainfrom
atmasphere:docs-page-layout-tab

Conversation

@atmasphere

@atmasphere atmasphere commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

What

Add --tab support to docs page-layout so document-layout changes can target a specific tab, and fix docs write --pageless silently no-opping on secondary tabs of multi-tab documents.

Why

Document-style fields (documentMode/pageless, page size, margins) are per-tab in the Docs API. page-layout and write --pageless built an UpdateDocumentStyleRequest but never set TabId, so the change always applied to the document's default tab. On a multi-tab document, setting pageless on a non-default tab reported success while nothing actually changed — documentFormat.documentMode stayed PAGES on the targeted tab.

Changes

  • docs page-layout: add --tab (title or ID, with the existing deprecated --tab-id alias), resolved to a concrete tab ID via resolveDocsTabID.
  • docs_layout.go: thread TabID through docsDocumentStyleOptions onto UpdateDocumentStyleRequest.TabId.
  • docs write: resolve --tab in applyDocumentStyle so --pageless/layout flags land on the targeted tab across every write path, including the markdown/Drive-replace path that never resolved the tab.
  • Behaviour is unchanged when --tab is omitted (empty TabId → default tab).

Tests

  • New unit tests for the request builder (TabID set → request carries it; omitted → empty, default-tab behaviour preserved).
  • make fmt-check lint deadcode test all pass locally; regenerated docs/commands/gog-docs-page-layout.md and updated CHANGELOG.md.
  • Verified end-to-end against the live Docs API: page-layout --layout pageless --tab <id> flips documentFormat.documentMode to PAGELESS on the targeted secondary tab (confirmed by reading that tab back).

@clawsweeper

clawsweeper Bot commented Jun 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 23, 2026, 11:03 PM ET / 03:03 UTC.

Summary
The PR adds tab-targeted Docs page layout support by resolving --tab/--tab-id and passing TabId into Docs API document-style updates.

Reproducibility: yes. from source inspection: current main builds Docs document-style update requests without TabId, so page layout and pageless style changes cannot explicitly target a secondary tab. I did not run live Docs API calls in this read-only review.

Review metrics: 1 noteworthy metric.

  • Changed files: 7 files, +134/-0. The diff is a bounded additive Docs CLI change, but it spans command code, tests, generated docs, and a release-owned changelog entry.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted command output, logs, a screenshot, recording, or linked artifact showing page-layout --tab and a readback of the targeted tab's layout mode.
  • Remove the CHANGELOG.md entry and keep release-note context in the PR body or commit message.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body describes live verification but does not include inspectable after-fix output; add redacted terminal output, logs, media, or a linked artifact and update the PR body for re-review.

Risk before merge

  • [P1] Needs contributor-provided real behavior proof before merge: the PR should include redacted terminal output, logs, screenshot, recording, or linked artifact showing page-layout --tab and readback of the targeted tab's layout mode.
  • [P1] The PR adds a CHANGELOG.md entry even though this review workflow treats the changelog as release-owned; the release note context should stay in the PR body or commit message until landing.

Maintainer options:

  1. Decide the mitigation before merge
    Land the focused tab-aware document-style fix after proof is added, preserving default-tab behavior when no tab is provided and leaving final release notes to the release owner.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] The remaining blocker is contributor-provided real behavior proof; automation cannot prove the contributor's live Docs setup for them.

Security
Cleared: The diff only changes Docs command code, generated command docs, a changelog entry, and unit tests; no security or supply-chain concern was found.

Review findings

  • [P3] Remove the release-owned changelog entry — CHANGELOG.md:5
Review details

Best possible solution:

Land the focused tab-aware document-style fix after proof is added, preserving default-tab behavior when no tab is provided and leaving final release notes to the release owner.

Do we have a high-confidence way to reproduce the issue?

Yes from source inspection: current main builds Docs document-style update requests without TabId, so page layout and pageless style changes cannot explicitly target a secondary tab. I did not run live Docs API calls in this read-only review.

Is this the best way to solve the issue?

Yes for the code path: threading a resolved tab ID through the existing Docs tab helpers into UpdateDocumentStyleRequest is the narrow maintainable fix. The changelog line should be removed before merge because release notes are release-owned.

Full review comments:

  • [P3] Remove the release-owned changelog entry — CHANGELOG.md:5
    CHANGELOG.md is release-owned for this review workflow. Please keep the release-note context in the PR body or commit message and let the release owner add the final changelog entry at landing.
    Confidence: 0.86

Overall correctness: patch is correct
Overall confidence: 0.82

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 7af5c68d93b3.

Label changes

Label justifications:

  • P2: The PR fixes a bounded Docs multi-tab command correctness issue with limited blast radius and no emergency signal.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body describes live verification but does not include inspectable after-fix output; add redacted terminal output, logs, media, or a linked artifact and update the PR body for re-review.
Evidence reviewed

What I checked:

  • Current main page-layout command lacks tab targeting: DocsPageLayoutCmd on current main exposes DocID, Layout, and layout flags only, then calls setDocumentStyle without resolving a tab. (internal/cmd/docs_set_page_layout.go:21, 7af5c68d93b3)
  • Current main request builder omits TabId: buildUpdateDocumentStyleRequest currently returns UpdateDocumentStyleRequest with DocumentStyle and Fields, but no TabId, so document-style updates cannot explicitly target a secondary tab. (internal/cmd/docs_layout.go:141, 7af5c68d93b3)
  • PR diff adds the narrow TabId plumbing: The PR adds a TabID option, resolves --tab/--tab-id in docs page-layout, passes the resolved ID to UpdateDocumentStyleRequest.TabId, and adds regression tests for set and omitted tab IDs. (internal/cmd/docs_layout.go:145, ffcf63137881)
  • Markdown style path has a Docs service when style is applied: PlanMarkdownWrite sets RequiresDocumentsService when ApplyDocumentStyle is true, so the PR's Drive-replace style application path has the Docs service it needs. (internal/docsedit/markdown.go:85, 7af5c68d93b3)
  • Real behavior proof remains uninspectable: The PR body says live Docs API verification was done, but the body and comments do not include redacted terminal output, logs, screenshots, recordings, or linked artifacts showing the after-fix command and readback. (ffcf63137881)
  • Feature history points to page-layout and tab helper owners: Search-string history shows the page-layout command was introduced in 1b8a7500, later Docs layout controls in b26e1900, and the --tab-id deprecation helper in 50f506dd. (internal/cmd/docs_set_page_layout.go:21, 1b8a750026df)

Likely related people:

  • sebsnyk: Search-string history shows the DocsPageLayoutCmd surface was introduced in the page-layout command commit. (role: feature introducer; confidence: high; commits: 1b8a750026df; files: internal/cmd/docs_set_page_layout.go, docs/commands/gog-docs-page-layout.md)
  • steipete: History search and blame point the current Docs layout/write implementation and recent layout-control work to this contributor. (role: recent area contributor; confidence: high; commits: 48db48b4f61f, 971c63788d39, b26e19003ebd; files: internal/cmd/docs_set_page_layout.go, internal/cmd/docs_layout.go, internal/cmd/docs_edit.go)
  • Ben Lewis: History search shows the --tab-id to --tab deprecation helper came from this contributor's tab-flag refactor. (role: adjacent owner; confidence: medium; commits: 50f506dd3aad; files: internal/cmd/docs_edit.go, internal/cmd/docs_helpers.go)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels Jun 23, 2026
@steipete
steipete force-pushed the docs-page-layout-tab branch from 9cddbf7 to ffcf631 Compare June 24, 2026 02:51
@steipete
steipete merged commit 57e3283 into openclaw:main Jun 24, 2026
5 checks passed
@steipete

steipete commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Landed as 57e3283a.

Validation:

  • go test ./internal/cmd -run 'TestDocsPageLayout|TestBuildUpdateDocumentStyleRequest'
  • make ci
  • Codex autoreview: clean; no accepted/actionable findings
  • PR CI: Linux test/lint/docs, Windows, macOS/cgo, worker, and Docker all green on the exact landed head
  • Added request-level coverage proving a tab title resolves to the secondary tab ID and UpdateDocumentStyleRequest.tabId carries that ID

Maintainer adjustments: normalized result output to the existing tabId convention, added command-level regression coverage, and completed the changelog attribution.

Live-proof caveat: the contributor reports successful secondary-tab Docs API readback. I could not independently repeat the live mutation because the available local test OAuth was revoked and the existing-Chrome bridge was unavailable; the local HTTP boundary test and official Docs API contract provide the independent request-shape proof.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@atmasphere @steipete