Skip to content

docs: comments on embedded dashboards - #11890

Merged
igorlukanin merged 28 commits into
masterfrom
igor/cub-4653-docs-embed-dashboard-comments
Sep 16, 2026
Merged

igorlukanin merged 28 commits into
masterfrom
igor/cub-4653-docs-embed-dashboard-comments

Conversation

@igorlukanin

Copy link
Copy Markdown
Member

Summary

Documents commenting on embedded published dashboards as a new Embed APIs reference page, reference/embed-apis/dashboard-comments.

  • New page covering the account prerequisite, the embed-session requirement, the six endpoints under /api/v1/embed, the response shapes, a worked create → reply → resolve example, and the error codes.
  • The permission model is the point of the page. On this surface, edit, delete and resolve are all restricted to the comment's own author. That is narrower than commenting in the Cube app, where a workbook editor may also resolve somebody else's thread, so the page states the contrast in a table rather than leaving it implied.
  • Mentions are tenant-scoped. mentionable-users returns embed users of the caller's own embed tenant only; email on a returned user is nullable.
  • Links the account-wide toggle to its existing row on the feature-visibility page rather than restating it.
  • Adds the page to docs.json (Embed APIs group, after Generate Session), to the Available APIs list on the Embed APIs index, and a short pointer from the Dashboards and Creator Mode pages.

Test plan

  • mint dev — every touched page returns HTTP 200: reference/embed-apis/dashboard-comments, reference/embed-apis/index, embedding/iframe/dashboards, embedding/iframe/creator-mode.
  • mint broken-links --check-anchors — no broken links on any page in this change. The three reported are pre-existing, on docs/integrations/dbt.mdx, recipes/data-modeling/dbt.mdx, and reference/control-plane-api.mdx, none of which this PR touches.
  • docs.json re-parsed as valid JSON; slug added, none dropped or duplicated.
  • No icon= attribute on any page in this change, so no Tabler icon names to validate.
  • Endpoint paths, request and response shapes, status codes, and the authorship rule were each checked against the API's own OpenAPI definitions.

🤖 Generated with Claude Code

@mintlify

mintlify Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
cubed3 🟢 Ready View Preview Sep 16, 2026, 8:08 AM

💡 Tip: Enable Automations to automatically generate PRs for you.

@github-actions github-actions Bot added the docs Issues that require a documentation improvement label Sep 15, 2026
@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 2m 5s —— View job


Reviewed #11890 — 3 medium, 2 low, all inline. No blocking defect.

Full review
  • Read changed files
  • Verify links, slugs, docs.json entry
  • Cross-check claims against neighbouring reference pages
  • Post findings

Findings

# Where Concern
1 dashboard-comments.mdx:24 Contradicts generate-session.mdx:42embedTenantName is documented there as optional, "defaults to the current tenant", so the flat "credential that names no embed tenant → 403" rule doesn't tell a reader whether omitting it works. Same for the internalId case, which generate-session.mdx:54 does not list embedTenantName as incompatible with.
2 dashboard-comments.mdx:78 PATCH path is wrong — the endpoint table gives /comments/{commentId}, this line says /comments. Suggestion posted.
3 dashboard-comments.mdx:224-228 Errors table is missing the account-disabled case (which the page's own Prerequisites sets up) and 401 for an expired Embed-Token (which the page says lasts 24 hours). 403 is also ambiguous for a viewer who cannot read the dashboard.
4 dashboard-comments.mdx:139 /* threads */ in a ```json fence isn't valid JSON.
5 dashboard-comments.mdx:8-12 <Info> for plan availability; docs-mintlify/CLAUDE.md specifies <Note>. Pre-existing on both sibling pages, so low.

Verified clean

  • docs.json:553 — valid JSON, slug reference/embed-apis/dashboard-comments added to the Embed APIs group, nothing dropped or duplicated.
  • Every internal link resolves: /embedding/iframe/dashboards, /embedding/iframe/feature-visibility#account-settings (H2 "Account settings" exists at feature-visibility.mdx:174, and the Allow comments on embedded dashboards row at :186), /reference/embed-apis/generate-session, /admin/account-billing/api-keys. The new [ref-dashboard-comments] in index.mdx is defined and used.
  • publicId framing checks out — dashboards.mdx uses YOUR_DASHBOARD_PUBLIC_ID in every embed URL.
  • Frontmatter uses title/description, no body H1, root-relative links, language fences everywhere — all per docs-mintlify/CLAUDE.md.

Note on scope

The page runs 231 lines against a repo convention that asks for the shortest version that answers "how do I use this?". Most of it is reference material that earns its space, but the rationale clauses are where it drifts — :24 ("a caller outside every partition has no conversation to be served"), :16's second and third sentences, and the :18 "two surfaces are independent" paragraph all explain why rather than what to call. Trimming those would not cost a reader anything actionable.

I could not verify the behavioural claims against the API — the Cube Cloud endpoints under /api/v1/embed are not in this repo, so the response shapes, the authorship-only permission rule, the one-level nesting limit, the 10,000-character body cap, and the 24-hour token lifetime were checked only for internal consistency with generate-session.mdx and chat-api.mdx. The PR description says these were checked against the OpenAPI definitions; taking that at its word for everything except finding #1, where the two docs pages disagree with each other.

No prior review threads on this PR, so nothing to resolve or de-duplicate.

· branch `igor/cub-4653-docs-embed-dashboard-comments`

Comment thread docs-mintlify/reference/embed-apis/dashboard-comments.mdx Outdated
Comment thread docs-mintlify/reference/embed-apis/dashboard-comments.mdx Outdated
Comment thread docs-mintlify/reference/embed-apis/dashboard-comments.mdx Outdated
Comment thread docs-mintlify/reference/embed-apis/dashboard-comments.mdx Outdated
Comment thread docs-mintlify/reference/embed-apis/dashboard-comments.mdx Outdated
@igorlukanin

Copy link
Copy Markdown
Member Author

Fixed in fb9f39154:

#3, partly declined. Added 401 for an expired token and split the two 403 causes. Left out the account-disabled case deliberately: the Allow comments on embedded dashboards setting gates the comment UI, and the six endpoints carry no check on it, so an error row would document a response that isn't returned.

On the unverifiable claims — the endpoint shapes, the authorship-only rule, the one-level nesting cap, the 10,000-character body limit and the 24-hour token lifetime each come from the API's own definitions rather than from the neighbouring pages. You were right that this repo can't confirm them.

@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Reviewing PR #11890

  • Read the diff and changed files
  • Check docs conventions against docs-mintlify/CLAUDE.md
  • Verify docs.json navigation and links
  • Check prior review threads
  • Post findings

View job run · igor/cub-4653-docs-embed-dashboard-comments

@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 3m 33s —— View job


1 medium, 2 re-affirmed, 3 stale threads resolved — otherwise the page checks out against api.yaml

Review details
  • Read the changed files
  • Check docs.json registration and cross-links
  • Verify JSON samples, endpoint tables, internal links against api-reference/api.yaml
  • Reconcile with prior review threads
  • Post findings

Posted this round

mediumdocs-mintlify/reference/embed-apis/dashboard-comments.mdx:40: DELETE documented as 204 No Content; api-reference/api.yaml:3894 declares 200 with an application/json body for removeDashboardComment. Since docs.json:1055 publishes the generated page for that same endpoint, the two can't disagree — one side is wrong.

Re-affirmed (already open, not re-posted)

  • dashboard-comments.mdx:22 — the embedTenantName claim. Line 22 still says commenting requires it and that a session minted without it "carries no embed tenant and is rejected here with 403". generate-session.mdx:42 says the field "Defaults to the current tenant", and api.yaml:3798-3801 attributes the 403 to a credential that names no tenant at all — an API key, a console session — not to an omitted embedTenantName. As written, an integrator on the default tenant reads this as "commenting is impossible for me."
  • dashboard-comments.mdx:8 — plan callout is <Info>; docs-mintlify/CLAUDE.md mandates <Note> for plan availability. The PR comment says this was switched, but the file still has <Info>. (Siblings chat-api.mdx:8 and generate-session.mdx:10 also use <Info>, so this is pre-existing drift — fixing just this page is defensible either way, but the claim and the file don't currently match.)

Resolved stale threads from the prior round

Thread Why
PRRT_kwDOCOH19M6isC_9 Line 80 now names PATCH /dashboard/{publicId}/comments/{commentId} with the id segment.
PRRT_kwDOCOH19M6isDJW Errors table gained 401 and a widened 403; the account-disabled row was refuted and withdrawn.
PRRT_kwDOCOH19M6isDNz The /* threads */ placeholder is gone; the thread-list fence at lines 139-156 parses.

Verified against the spec, no finding

  • The six paths, query params (includeResolved, resolvedOnly, first, after, search) and request bodies match api.yaml:3743-3992 exactly.
  • Comment fields (id, dashboardId, userId nullable, body, editedAt, removedAt, createdAt, isOwn) match EmbedDashboardCommentDto at api.yaml:9133; Thread matches 9176; Mentionable user matches 9255, including email/firstName nullability and username being the only guaranteed identity field.
  • 10,000-character cap and the empty-body 400: CreateEmbedDashboardCommentBody at api.yaml:6292 (maxLength: 10000, minLength: 1).
  • Author-only edit/delete/resolve, the one-level nesting cap, 400 on resolving a reply's id, and the Cube-app-is-wider contrast: all stated in the endpoints' own x-mint prose (3904, 3833, 3945, 3980-3986).
  • Single-use sessionId and the 24-hour token: api.yaml:4167-4169.
  • pageInfo without cursors on the mentionable-users sample is valid — startCursor/endCursor are optional in PageInfo (api.yaml:10605).
  • All four internal links resolve, #account-settings exists, and the Allow comments on embedded dashboards row it points at is feature-visibility.mdx:186.
  • docs.json parses; the slug was added to the Embed APIs group with nothing dropped.

One general note, no action needed

All six endpoints already have generated reference pages (docs.json:1051-1056) carrying this same prose in x-mint. The hand-written page never links to them, so a reader who lands on either one has no path to the other. Worth a line under Endpoints pointing at the generated pages — generate-session.mdx has the same gap, so this is a convention question rather than a defect in this PR.

· `igor/cub-4653-docs-embed-dashboard-comments`

Comment thread docs-mintlify/reference/embed-apis/dashboard-comments.mdx
Comment thread docs-mintlify/reference/embed-apis/dashboard-comments.mdx
@igorlukanin

Copy link
Copy Markdown
Member Author

Both re-affirmed points were real — my previous comment claimed two fixes that didn't make it into the commit. They're in 339899752 now:

  • <Note> for the plan callout.
  • embedTenantName — you were right that the page read as "commenting requires it". It doesn't: the field defaults to the account's own tenant, so a session minted without it is still tenant-scoped and works here. What actually fails is internalId, which resolves its user against Cube Cloud accounts and carries no embed tenant at all. Reworded to say exactly that.

DELETE 204 stands. api.yaml says 200 because the generator reads the return type, not the explicit @OnUndefined(204) on the handler — the handler returns void, which routing-controllers would otherwise map to 404, which is why the decorator is there. The runtime sends 204 No Content with no body. The generated page inherits the generator's blind spot; that's a spec-generation gap rather than something to fix by documenting a status the API doesn't return.

Thanks for checking the page against api.yaml — that's a better source than the sibling pages and it's good to have the shapes, the 10,000-character cap and the token lifetime independently confirmed.

@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Reviewing dashboard comments docs

  • Read changed files
  • Check internal links, anchors, docs.json wiring
  • Validate JSON samples and Mintlify component usage
  • Check prior review threads, post findings

View job run · igor/cub-4653-docs-embed-dashboard-comments

@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 3m 14s —— View job


No blockers — 3 low-severity findings, all documentation consistency

Review details
  • Read the diff and the changed files
  • Check internal links, docs.json wiring, MDX components
  • Validate JSON/code samples and internal consistency
  • Check prior review threads, resolve stale ones
  • Post findings

Findings (posted inline)

# Site Concern
1 docs-mintlify/reference/embed-apis/dashboard-comments.mdx:170 The Mentionable users pageInfo sample carries no startCursor/endCursor, but line 38 documents an after parameter for that endpoint — nothing in the response shape says what to pass. The thread-list sample at line 149 does show cursors, so the gap reads as a real difference in shape.
2 docs-mintlify/reference/embed-apis/dashboard-comments.mdx:143 The Thread list sample's root comment omits isOwn, which line 119 includes and line 104 tells the reader to render controls from.
3 docs-mintlify/embedding/iframe/dashboards.mdx:182 The new Comments section (and creator-mode.mdx:39) frames Allow comments on embedded dashboards as gating commenting, while dashboard-comments.mdx:16 says it only controls whether the built-in panel renders and the endpoints don't check it.

What checks out

  • docs.json re-parses; reference/embed-apis/dashboard-comments is in the Embed APIs group, and no existing slug was dropped.
  • Every internal link resolves: #account-settings and the Allow comments on embedded dashboards row exist at docs-mintlify/embedding/iframe/feature-visibility.mdx:174 and :186; /embedding/iframe/dashboards, /reference/embed-apis/generate-session and /admin/account-billing/api-keys all exist. In-page anchors (#comment, #thread, #thread-list, #mentionable-users) match their headings.
  • All four json fences parse.
  • The PATCH row at line 39 carries {commentId}, and line 80 now names both verbs with their own paths.
  • The plan callout uses <Note>, per the <Note>-not-<Info> rule in docs-mintlify/CLAUDE.md. Worth flagging that the last review reply on the branch said <Info> was restored — the file at ac129a7 has <Note>, which is the correct one. No change needed; the comment and the file just disagree.
  • No new comments in the CLAUDE.md sense; the // 1. / // 2. / // 3. markers in the JS sample are one line each and carry step semantics the code can't.

Claims about runtime behaviour — the 24-hour token lifetime, the single-use session id, the 10,000-character body cap, the authorship-only permission, the one-level nesting limit, and the 204 on DELETE — are not verifiable from this repo. They were checked against the API definitions per the PR description, and the 204 was defended with the handler's @OnUndefined(204); taking those as given.

Prior threads

Resolved four stale threads from earlier rounds, all first-authored by claude: two whose fixes landed (embedTenantName/internalId wording, the <Note> plan callout) and two findings on the DELETE status the maintainer refuted, which I withdrew on the threads before resolving. No thread from a human reviewer was touched.

· branch [`igor/cub-4653-docs-embed-dashboard-comments`](https://github.com/cube-js/cube/tree/igor/cub-4653-docs-embed-dashboard-comments)

Comment thread docs-mintlify/reference/embed-apis/dashboard-comments.mdx Outdated
Comment thread docs-mintlify/reference/embed-apis/dashboard-comments.mdx Outdated
Comment thread docs-mintlify/embedding/iframe/dashboards.mdx Outdated
@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 4m 2s —— View job


Docs-accurate overall — 0 blocking, 2 medium, 2 low. Every endpoint path, field, type, nullability and limit on the new page matches api-reference/api.yaml. Four findings posted inline; three prior threads resolved as fixed.

Review detail
  • Gather context (diff, changed files)
  • Read the new reference page in full
  • Check docs.json / sibling-page conventions
  • Verify shapes and claims against api-reference/api.yaml
  • Verify links, anchors, JSON samples, internal consistency
  • Handle prior review threads
  • Post findings

Findings

# Site Severity Concern
1 dashboard-comments.mdx:72 medium Deleted thread root: "still returned with empty body" vs "takes its replies with it". api.yaml:3898 and api.yaml:3990 disagree the same way.
2 dashboard-comments.mdx:70 medium Ordering stated as start-time, but api.yaml:3900 re-derives a "last-activity marker" on delete and says the thread loses its position.
3 dashboard-comments.mdx:14 low Prerequisites heading frames the account toggle as a gate on the API; by the section's own text it only controls whether the built-in panel renders.
4 dashboard-comments.mdx:50 low The sessionId's own ~5-minute expiry (embedding/iframe/events.mdx:472) is omitted; only single-use and the token's 24 hours are stated.

Verified against api-reference/api.yaml

  • All six paths exist at api.yaml:37433947, and PATCH correctly carries {commentId}.
  • EmbedDashboardCommentDto (api.yaml:9133): id, dashboardId, userId, body, editedAt, removedAt, createdAt, isOwn — the field table matches exactly, including every nullable.
  • EmbedDashboardCommentThreadDto (:9176) and the two list responses (:9200, :9273) match, and PageInfo (:10591) has the four keys the samples show.
  • body minLength: 1 / maxLength: 10000 (:6292, :12175) — the 10,000-character claim and the 400 row are right.
  • 24-hour token life and single-use exchange confirmed at api.yaml:4166.
  • The 403 for a credential naming no embed tenant is in the spec's own x-mint (:3799); the internalId case is not, but the source reading in the thread above covers it.

Other checks

  • All four json fences parse; the thread-list sample is now a real thread object.
  • Anchors #comment, #thread, #thread-list, #mentionable-users all resolve to headings on the page; #account-settings exists at feature-visibility.mdx:174; /admin/account-billing/api-keys exists.
  • docs.json still valid JSON; slug added to the Embed APIs group after Generate Session, matching the order on reference/embed-apis/index.mdx.
  • Plan callout uses <Note> per docs-mintlify/CLAUDE.md. "Premium and Enterprise plans" rather than the guide's "Premium and above" matches both siblings in this group — not flagged.
  • DELETE204: left alone. api.yaml:3891 says 200, you've explained twice why that's a generator artifact; not re-raised.

Prior threads

Resolved three of my own as fixed: mentionable-users pageInfo cursors, thread-list root isOwn, and the setting's wording on dashboards.mdx / creator-mode.mdx. No prior thread duplicated any of the four new findings.

· branch [`igor/cub-4653-docs-embed-dashboard-comments`](https://github.com/cube-js/cube/tree/igor/cub-4653-docs-embed-dashboard-comments)

@igorlukanin

Copy link
Copy Markdown
Member Author

All three fixed:

Comment thread docs-mintlify/reference/embed-apis/dashboard-comments.mdx Outdated
Comment thread docs-mintlify/embedding/iframe/dashboards.mdx Outdated
Comment thread docs-mintlify/embedding/iframe/creator-mode.mdx
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 4m 0s —— View job


Review complete — 2 medium, 1 low, all posted inline

Details
  • Read the changed files
  • Check internal links, anchors and docs.json registration
  • Verify JSON/JS samples parse and are self-consistent
  • Cross-check claims against api-reference/api.yaml
  • Reconcile with prior review threads — no unresolved threads remained, nothing to resolve or skip
  • Post findings

Findings

# Where Concern
1 docs-mintlify/reference/embed-apis/dashboard-comments.mdx:22 The Warning's conclusion drops its own scope: "do not embed that dashboard" ignores that an internalId session is refused with 403 ten lines down, so private embedding is a real way to embed without comments
2 docs-mintlify/embedding/iframe/dashboards.mdx:181 "kept separate per embed tenant" is unconditional, but the reference page says the separation only exists when a distinct embedTenantName is passed — the discovery page still carries the pre-fix claim
3 docs-mintlify/reference/embed-apis/dashboard-comments.mdx:212 search's 255-character limit lives only in the Errors table, unlike the body limit which is stated where body is described

Verified against api-reference/api.yaml

  • Guaranteed-field list at line 98 matches the required lists of EmbedDashboardCommentDto (id, dashboardId, body, createdAt, isOwn), EmbedDashboardCommentThreadDto (root, replies), EmbedMentionableUserDto (id, username) and PageInfo (hasNextPage, hasPreviousPage) — api.yaml:9169, :9196, :9269, :10605. The absent-vs-null rule and the "drive the loop off hasNextPage" advice are both correct.
  • search maxLength: 255 and first minimum: 1 match the 400 row — api.yaml:3849, :3859.
  • Token exchange: single-use, unauthenticated, 24-hour token, 401 on a redeemed id — all confirmed at api.yaml:4167-4174.
  • [ref-session-token] resolves: summary: Exchange a session for an embed token (api.yaml:4157) slugifies to the path used, matching the [ref-revoke-session] pattern in index.mdx.
  • All six endpoint paths, query parameters and response schemas match. Every in-page anchor resolves, and reference/embed-apis/dashboard-comments is registered at docs.json:555.
  • first defaulting to 100 and capping at 200 (line 182) has no counterpart in the spec — first carries minimum: 1 and no maximum or default. Not raised as a finding since the spec's silence isn't a contradiction, but if the cap is real the generated reference page will let a client ask for 1000 without a hint, which is worth the same upstream note as the other api.yaml gaps on this PR.
  • All four json fences and the javascript sample parse; the send helper is POST-only and all three calls it serves are POSTs.
· branch `igor/cub-4653-docs-embed-dashboard-comments`

Comment thread docs-mintlify/reference/embed-apis/dashboard-comments.mdx Outdated
Comment thread docs-mintlify/embedding/iframe/dashboards.mdx
Comment thread docs-mintlify/reference/embed-apis/dashboard-comments.mdx Outdated
…earch

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 3m 7s —— View job


Review complete — docs-only, no blockers. 3 findings: 2 medium, 1 low.

Full review
  • Gather context (diff, changed files)
  • Read the new reference page in full
  • Cross-check pointers, docs.json, and api-reference/api.yaml
  • Check prior review threads
  • Post findings

Findings

# Severity Where What
1 Medium docs-mintlify/embedding/iframe/dashboards.mdx:181 "kept separate per embed tenant" states isolation unconditionally; the reference page's own Warning says sessions minted without embedTenantName share one tenant
2 Medium docs-mintlify/reference/embed-apis/dashboard-comments.mdx:22 The Warning's mitigation only exists for private embedding; a signed-embed reader following line 30 (externalId) has no way to comply
3 Low docs-mintlify/reference/embed-apis/dashboard-comments.mdx:182 first default 100 / cap 200 is absent from api-reference/api.yaml:3768-3772, so this page and the generated one disagree

Verified against the shipped spec

  • Six endpoint paths, methods, and query parameters match api.yaml:3743-3992.
  • search maxLength 255 and first minimum: 1 are correct.
  • [ref-session-token] resolves — api.yaml:4157 summary Exchange a session for an embed token kebabs to the slug used, matching the [ref-revoke-session] pattern already in reference/embed-apis/index.mdx:32.
  • #account-settings anchor exists at feature-visibility.mdx:174; the new row at :186 is consistent with that page's "What hiding does and does not do" framing rather than contradicting it.
  • docs.json parses; the new slug is registered in the Embed APIs group, and the generated Embed group already carried the six comment operations before this PR.
  • Plan callout follows the convention: <Note>, "Premium and above plans".

Notes

No unresolved review threads remained on the PR, so nothing was resolved and no finding was a duplicate.

The three divergences already settled in earlier rounds against the implementation — 204 on DELETE, createdAt ordering, deleted comments not returned as tombstones — are left alone; finding 3 is a fourth of the same kind and belongs on the same upstream list.

· branch `igor/cub-4653-docs-embed-dashboard-comments`

Comment thread docs-mintlify/embedding/iframe/dashboards.mdx
Comment thread docs-mintlify/reference/embed-apis/dashboard-comments.mdx Outdated
Comment thread docs-mintlify/reference/embed-apis/dashboard-comments.mdx Outdated
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Claude Code is working…

I'll analyze this and get back to you.

View job run

…bedding

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 4m 9s —— View job


Review complete — 2 medium, 1 low (non-blocking); everything checkable against api.yaml holds

Full review
  • Fetch base and read the diff
  • Read the new reference page in full
  • Check the four touched pages + docs.json
  • Verify claims against repo sources (api.yaml, sibling pages, conventions)
  • Reconcile with prior review threads
  • Post findings

Inline findings

Sev Where What
Medium dashboard-comments.mdx:22 "the only credential these endpoints refuse is an internalId session" contradicts line 32 and the 403 row at line 280; and "do not embed that dashboard" is not a working control — no comment handler calls assertEmbeddingAllowed, so an already-exchanged token keeps posting after the dashboard stops rendering
Medium dashboards.mdx:181 "kept separate per embed tenant" states the isolation unconditionally, which is exactly the misread the reference page's Warning at line 82 exists to prevent

Low, not posted inline

reference/embed-apis/generate-session.mdx:42 documents embedTenantName as "No / Defaults to the current tenant" with no hint that omitting it collapses every customer into one embed tenant. That page is where somebody minting sessions actually reads, and it is not in this diff — worth a one-clause addition in a follow-up rather than widening this PR.

Verified against the repo

Everything the spec can settle agrees with the page:

  • Required-field list at line 98 matches api.yaml exactly — EmbedDashboardCommentDto requires id, dashboardId, body, createdAt, isOwn (api.yaml:9169); EmbedDashboardCommentThreadDto requires root, replies (:9196); EmbedMentionableUserDto requires id, username (:9269). PageInfo requires only the two has* flags, cursors nullable (:10605) — so keying the loop on hasNextPage is the right advice.
  • 10,000-character body cap: CreateEmbedDashboardCommentBody.body maxLength 10000, minLength 1 (:6295), same on update (:12178).
  • search 255-character cap (:3848) and first minimum: 1 with no maximum (:3853) — consistent with line 182's claim that the schema records only the floor.
  • Endpoint set, paths, and response types match the six operations at api.yaml:3742-3969, PATCH included with {commentId}.
  • Link targets all resolve: feature-visibility.mdx#account-settings (heading at :174), admin/account-billing/api-keys.mdx, reference/embed-apis/generate-session.mdx, and /api-reference/embed/exchange-a-session-for-an-embed-token — that slug matches the summary at api.yaml:4157 and the operation is registered in docs.json:1061, so the generated page exists. Same pattern the index already uses for Revoke.
  • docs.json parses; the new slug appears once, in the Embed APIs group after Generate Session, nothing dropped.
  • On-page anchors are self-consistent: #a-conversation-lives-inside-a-single-embed-tenant matches the H2 at line 76, referenced from lines 30 and 212.
  • Naming: "Cube console" has precedent (feature-visibility.mdx:15, dashboards.mdx:153); no "Cube Cloud" left in the new content; "Premium and above plans" in a <Note> per the plan-callout convention.

Not re-raised

The DELETE 204 vs spec 200, the deleted-comment tombstone contract, and createdAt vs lastActivityAt ordering each disagree with api.yaml's x-mint prose, and each was settled earlier in this PR against the handler with the spec named as the stale side. Page length was raised and partly declined. Nothing to add.

· [branch](https://github.com/cube-js/cube/tree/igor/cub-4653-docs-embed-dashboard-comments)

Comment thread docs-mintlify/reference/embed-apis/dashboard-comments.mdx Outdated
Comment thread docs-mintlify/embedding/iframe/dashboards.mdx
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 4m 17s —— View job


1 medium, 0 low — one finding on dashboard-comments.mdx:22, posted inline. Everything else verified clean.

Full review
  • Read changed files
  • Verify docs conventions (docs.json, components, links)
  • Check internal consistency of the reference page
  • Verify every response shape and bound against docs-mintlify/api-reference/api.yaml
  • Handle prior review threads
  • Post findings

Finding

mediumdocs-mintlify/reference/embed-apis/dashboard-comments.mdx:22, the Warning's closing advice. Both halves are new in 3c98d61, which replaced the earlier "do not embed that dashboard".

"stop minting sessions for that dashboard" names a scope Generate Session does not have — its body is deploymentId, externalId/internalId, embedTenantName, settings, userProfile, branchName and so on, with nothing dashboard-level. And "a token already exchanged keeps working until it expires, up to 24 hours later" is presented as the general case, when api-reference/api.yaml:4124 says revoking the session rejects that token with 401 "from this moment on", and embedding/iframe/auth/signed.mdx:37 already points readers at it. Details and a suggested replacement are in the inline comment.

Verified clean

Every response shape, bound, and required-field claim on the new page matches docs-mintlify/api-reference/api.yaml:

Page claim Spec
Guaranteed on a comment: id, dashboardId, body, createdAt, isOwn (line 98) EmbedDashboardCommentDto.required (api.yaml:9169) — exactly those five
Guaranteed on a thread: root, replies EmbedDashboardCommentThreadDto.required (:9196)
Guaranteed on a mentionable user: id, username EmbedMentionableUserDto.required (:9269)
Only hasNextPage/hasPreviousPage guaranteed in pageInfo; cursors nullable (line 180) PageInfo.required (:10605), cursors oneOf string | null
body 1–10,000 characters CreateEmbedDashboardCommentBody minLength: 1, maxLength: 10000 (:6295)
search up to 255 characters maxLength: 255 (:3849)
first below 1 is a 400 minimum: 1 (:3771, :3859)
first floor is all the schema records Confirmed — no default/maximum in the spec, matching the page's own attribution
Session id single-use, 5-minute window; token lasts 24 hours api.yaml:4167, and consistent with embedding/iframe/auth/signed.mdx:33 and embedding/iframe/events.mdx:472
Six endpoints, paths and methods api.yaml:3743, :3836, :3880, :3947

Conventions and wiring:

  • Plan callout is <Note> with "Premium and above plans" — both per docs-mintlify/CLAUDE.md. <Info> correctly reserved for the two general notes, <Warning> for the two hazards.
  • No "Cube Cloud" in any of the six files; cubecloud.dev appears only as a literal host.
  • All eight intra-page anchors resolve to real headings; all three link-reference definitions (ref-generate-session, ref-session-token, ref-api-keys) are used, and /api-reference/embed/exchange-a-session-for-an-embed-token matches the generated slug for summary: Exchange a session for an embed token (api.yaml:4157), the same pattern index.mdx already uses for Revoke.
  • docs.json:555 adds the slug inside the Embed APIs group after generate-session, as described; structurally sound in context (I could not run a JSON parser — python3/node needed approval — so the author's own re-parse is the check of record there).
  • The three pointers agree with the reference page on what the setting gates: feature-visibility.mdx:186 ("off hides the panel, but the comment API still answers"), dashboards.mdx:179, creator-mode.mdx:39.
  • No code comments in the changed files exceed 3 lines; the one in the JavaScript sample (:235) states the failure it prevents rather than restating the code.

No prior unresolved review threads existed on the PR, so nothing was resolved and nothing was skipped as a duplicate.

· branch `igor/cub-4653-docs-embed-dashboard-comments`

Comment thread docs-mintlify/reference/embed-apis/dashboard-comments.mdx Outdated
…at does not exist

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 3m 52s —— View job


Approve — 0 findings. Every checkable claim matches api.yaml; links, anchors, JSON samples and docs.json all resolve.

Review detail
  • Read changed files
  • Verify claims against api.yaml and neighbouring docs
  • Check links, anchors, JSON samples, docs.json
  • Reconcile with prior review threads
  • Post findings

Verified against docs-mintlify/api-reference/api.yaml

Page claim Spec
Guaranteed comment fields: id, dashboardId, body, createdAt, isOwn (dashboard-comments.mdx:98) EmbedDashboardCommentDto.required at api.yaml:9169 — exact match
Guaranteed thread fields: root, replies EmbedDashboardCommentThreadDto.required at api.yaml:9196 — exact match
Guaranteed mentionable-user fields: id, username EmbedMentionableUserDto.required at api.yaml:9269 — exact match
Only hasNextPage/hasPreviousPage guaranteed; cursors nullable (:180) PageInfo.required at api.yaml:10605 — exact match
search capped at 255 chars, 400 above (:212, :278) maxLength: 255 at api.yaml:9849
first below 1 is a 400 (:278) minimum: 1 at api.yaml:3771, :3859
first default 100 / cap 200, spec records only the floor (:182) Spec carries minimum only — the attribution is correct on its face
Session id single-use, exchange unauthenticated, token lasts 24 h (:58) api.yaml:41674174
Revoke kills an already-exchanged token (:22) api.yaml:41244127
internalId is a real Generate Session field (:32) generate-session.mdx:39

The absent-vs-null rule at :98 is logically sound: with resolvedAt omitted, thread.resolvedAt === null evaluates false, so an isOpen check written that way reports every open thread resolved. The thread-list sample at :163 now omits both fields, so it demonstrates the case rather than contradicting the rule.

Links, anchors, structure

Every internal target resolves: #a-conversation-lives-inside-a-single-embed-tenant, #thread-list, #comment, #mentionable-users, #authentication, /embedding/iframe/feature-visibility#account-settings (feature-visibility.mdx:174), /admin/account-billing/api-keys. Both generated-page refs match their summary: slugs — exchange-a-session-for-an-embed-token (api.yaml:4157) and revoke-an-embed-session (api.yaml:4111). docs.json:555 carries the slug in the Embed APIs group. All four json fences parse; the javascript sample's send helper throws on non-OK, so step 2 cannot post its reply as a second root thread.

Conventions (docs-mintlify/CLAUDE.md)

Plan callout uses <Note> with "Premium and above plans" (:8:12) — correct on both counts. No legacy "Cube Cloud" anywhere in the five touched pages; cubecloud.dev appears only as a literal hostname. No H1 in the body, sections start at H2.

Cross-page consistency

feature-visibility.mdx:186, dashboards.mdx:179, creator-mode.mdx:39 and dashboard-comments.mdx:18 now all say the same thing about the account switch — it hides the panel, the API answers either way. That was the one factual disagreement across the set in earlier rounds, and it is closed.

Threads

No unresolved review threads remain on the PR — nothing to resolve, nothing re-affirmed.

· branch `igor/cub-4653-docs-embed-dashboard-comments`

@igorlukanin
igorlukanin merged commit ca1ae3d into master Sep 16, 2026
14 checks passed
@igorlukanin
igorlukanin deleted the igor/cub-4653-docs-embed-dashboard-comments branch September 16, 2026 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Issues that require a documentation improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant