Skip to content

docs: correct HTML chart Handlebars bindings and sandbox limitations - #11388

Open
igorlukanin wants to merge 6 commits into
masterfrom
igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs
Open

docs: correct HTML chart Handlebars bindings and sandbox limitations#11388
igorlukanin wants to merge 6 commits into
masterfrom
igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs

Conversation

@igorlukanin

Copy link
Copy Markdown
Member

The HTML chart docs page describes a template context and a JavaScript capability that don't match what the product does. Every code example on the page currently fails if copy-pasted.

Handlebars bindings. The page documents result._first and {{#each result.data}}. There's no result key in the template context — rows are a flat data array. Templates compile in strict mode, so these don't render blank, they throw and break the chart. Corrected to data.0.<member> and {{#each data}}, matching the Handlebars reference panel in the chart editor.

JavaScript. The page says <script> tags are supported and shows a Chart.js CDN example. HTML charts render in a sandboxed iframe under a restrictive CSP, so scripts don't execute and external resources don't load. Replaced that section with an accurate description of what the sandbox allows, and pointed readers at built-in chart types or custom Vega-Lite specs for visualizations.

Also documents the rest of the template context (columns, rowCount, columnCount, pivot) and the available helpers, which previously only existed in the in-app panel.

@mintlify

mintlify Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

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

Project Status Preview Updated (UTC)
cubed3 🟢 Ready View Preview Jul 28, 2026, 3:13 AM

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

@igorlukanin
igorlukanin marked this pull request as ready for review July 30, 2026 18:09
@igorlukanin
igorlukanin requested a review from keydunov as a code owner July 30, 2026 18:09
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


Review: docs-only change, correction is the right call — 1 medium, 3 low. 2 inline comments posted.

Full review
  • Gather context (diff, page, docs conventions)
  • Verify technical claims against the repo where possible
  • Check docs conventions (docs-mintlify/CLAUDE.md), links, formatting
  • Post review

Scope

Single .mdx file under docs-mintlify/ — the active docs site per CLAUDE.md. No code, no tests, no security/performance surface. Review is about accuracy and clarity.

The core correction is well-founded: the removed section told readers to load Chart.js from a CDN inside an iframe that can't run scripts or reach the network, and the removed bindings (result._first, result.data) don't exist. Frontmatter description and the intro were both updated to drop the JavaScript claim — consistent. No other page in docs-mintlify/docs/explore-analyze/ still claims <script> support (grepped for Chart.js, D3, <script>, JavaScript).

Verifiability caveat

The chart renderer isn't in this repo (Handlebars appears only in yarn.lock and docs), so I could not empirically confirm the template context, the helper list, or the CSP behavior. Everything below is reasoning about internal consistency, not a contradiction of the product. Two claims worth double-checking against the implementation before merge:

  • formatDate / formatMonth tokens. "yyyy-LL-dd" and "LLL yyyy" are Luxon tokens. If the renderer uses date-fns or Day.js instead, LL/LLL mean different things (date-fns LLL is a standalone month name, so "yyyy-LL-dd" would not produce 2026-07-30). Numbers use d3-format, which is a different library family, so the mix is worth confirming.
  • data: URLs for images and fonts. Allowed only if the CSP includes data: in img-src/font-src. Worth confirming against the actual policy header — this is the one "supported" bullet a reader would build a branded card around.

Findings

# Severity Location Issue
1 Medium html.mdx:18 vs :42 Unqualified (data.0.status) vs qualified (order_items.count) key form — both can't hold for the same result set, and guessing wrong reproduces the strict-mode throw this PR fixes. Inline comment posted.
2 Low html.mdx:59 pivot row is too vague to write a path against. Inline comment posted.
3 Low html.mdx:46 The <Tip> uses {{json data}} before the json helper is introduced at line 70. Fine as a forward reference, but a [json](#helpers) link would close it.
4 Low html.mdx:6 and :91 "evaluated before the chart is rendered" and "evaluated before the iframe is rendered" say the same thing twice. docs-mintlify/CLAUDE.md asks for "say it once" — the sandbox bullet could just be "Handlebars expressions".

Optional

kpi.mdx documents an HTML block inside KPI tiles with no mention of whether it shares this sandbox and template context. If it does, a one-line cross-link from that section to this page would save a reader the same failed copy-paste. Out of scope for this PR.

Conventions

Checked and clean: title/description frontmatter, no body H1, body starts at H2, all fences tagged html, callouts (<Warning>, <Tip>) are standard Mintlify, internal links are root-relative and resolve (charts/chart-types/index.mdx, charts/custom.mdx both exist), no new page so no docs.json change needed.

· Branch [`igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs`](https://github.com/cube-js/cube/tree/igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs)

Comment thread docs-mintlify/docs/explore-analyze/charts/chart-types/html.mdx Outdated
Comment thread docs-mintlify/docs/explore-analyze/charts/chart-types/html.mdx Outdated
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 54s —— View job


Re-review of 925d4d7: all four findings addressed, no new blockers. 1 low nit, no inline comments.

Full review
  • Diff the new commit against the previously reviewed state
  • Verify each prior finding is actually resolved
  • Check the date-fns claim against the token examples
  • Resolve stale review threads, post review

What changed in 925d4d7

Prior finding Status
1 — Medium: unqualified vs qualified key form Resolved. Line 18 now states the key form follows the query and points to the Handlebars reference panel; the <Warning> covers the loop form ({{this.[order_items.count]}}) as well as the indexed one.
2 — Low: pivot unactionable Resolved. Line 59 names rows, columns, cellMap, grandTotals, config and points at {{json pivot}}.
3 — Low: forward reference to json Resolved. Line 46 links [json helper](#helpers).
4 — Low: "before the chart is rendered" said twice Resolved. The sandbox bullet is now just "Handlebars expressions"; the statement survives once at line 6.

Two threads I opened are now outdated and resolved.

On the key-form correction

Igor's read is right and mine was too narrow — multiple aliases per value means the two forms usually coexist, so "both can't be true" was wrong as stated; the real hazard is the qualified-only row, which the new wording covers. The fix is better than either option I proposed: it names the mechanism (follows the query), gives the reader a deterministic way to find the answer (the reference panel), and generalizes the bracket rule to both access shapes.

formatDate / date-fns

The library is now named, and the tokens check out against it: date-fns treats L/LL/LLL as stand-alone month, so "yyyy-LL-dd"2026-07-30 and "LLL yyyy"Jul 2026. yyyy is the calendar year (not the YYYY that date-fns warns on). No contradiction with formatNumber using d3-format — different value types, different libraries is normal.

The renderer still isn't in this repo (formatChartData, Handlebars helper registration, and the CSP header are all outside it), so the library attribution and the data:-URL allowance remain unverifiable from here. Both are now stated precisely enough to be checked against the implementation by anyone who has it.

Remaining nit (low)

docs-mintlify/docs/explore-analyze/charts/chart-types/html.mdx:68-69formatDate names date-fns but formatMonth on the next row doesn't. Adjacency makes the shared library obvious enough that adding a second link would be noise; flagging only in case formatMonth accepts a different pattern set.

Conventions

Unchanged and clean: frontmatter title/description, no body H1, fences tagged html, standard callouts, root-relative internal links that resolve, no new page so no docs.json change.

· Branch igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs ·

@igorlukanin

Copy link
Copy Markdown
Member Author

Checked the two caveats against the implementation, both hold. The engine formats dates with date-fns, not Luxon — but yyyy-LL-dd and LLL yyyy are also the engine's own defaults and render as 2026-07-30 / Jul 2026 there, so the examples are right; added the date-fns link so the token family is unambiguous. The CSP is img-src data:; font-src data:, so the data: URL bullet is accurate.

Also took findings 3 and 4 in 925d4d77.

@igorlukanin
igorlukanin force-pushed the igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs branch from 925d4d7 to e43eed4 Compare July 30, 2026 18:32
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 48s —— View job


Re-review of e43eed4: no content change since my last pass — still clean, 1 low nit carried over. No new inline comments.

Full review
  • Diff the pushed commit against the previously reviewed state
  • Confirm prior findings remain resolved
  • Re-check links, frontmatter, and Mintlify conventions
  • Check existing review threads for dedup / stale resolution

What this push contains

e43eed4 ("docs: state that HTML chart bindings follow the query's column naming") carries exactly the content I reviewed as 925d4d7 — the SHA changed but the tree didn't. git diff d47e6f7..e43eed4 is the same six-line delta: the key-form paragraph at line 18, the loop form in the <Warning>, the pivot sub-keys, the json helper link, the date-fns link, and the trimmed sandbox bullet.

So the verdict from the last pass stands unchanged. All four prior findings remain resolved:

Prior finding Status
1 — Medium: unqualified vs qualified key form Resolved (line 18 + <Warning> at 42)
2 — Low: pivot unactionable Resolved (line 59 names rows, columns, cellMap, grandTotals, config)
3 — Low: forward reference to json Resolved (line 46)
4 — Low: duplicated "before the chart is rendered" Resolved (line 91)

Threads

Both of my inline threads are already resolved and marked outdated — nothing to resolve, nothing to re-affirm. No human-authored threads on this PR.

Carried-over nit (low, unchanged)

docs-mintlify/docs/explore-analyze/charts/chart-types/html.mdx:69formatMonth still doesn't name date-fns while formatDate on the line above does. Adjacency makes it obvious; only worth a change if formatMonth takes a different pattern set.

Verifiability

Unchanged from last pass: the renderer (formatChartData, helper registration, the CSP header) lives outside this repo, so the date-fns attribution and the img-src data: / font-src data: allowance rest on Igor's check against the implementation, which he reported holds. The date-fns tokens are self-consistent — L/LL/LLL are stand-alone month in that library, giving 2026-07-30 and Jul 2026.

Conventions

Re-checked and clean: title/description frontmatter, no body H1, body opens at H2, all fences tagged html, <Warning>/<Tip> are standard Mintlify, both internal links resolve (chart-types/index.mdx, charts/custom.mdx present on disk), no new page so no docs.json change.

Nothing blocking. Ready to merge from my side.

· Branch igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs ·

@igorlukanin

Copy link
Copy Markdown
Member Author

Took the nit in 6c691ee. Both helpers go through the same formatDateUtc wrapper over date-fns/format and differ only in their default (yyyy-LL-dd vs LLL yyyy), so naming the library on one row and not the adjacent one was just an inconsistency.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 31s —— View job


Re-review of 6c691ee: the last nit is closed. No findings — clean, ready to merge.

Full review
  • Diff the new commit against the previously reviewed state
  • Confirm the carried-over nit is resolved
  • Re-check tokens, links, frontmatter, and Mintlify conventions
  • Check review threads for dedup / stale resolution

What changed in 6c691ee

A one-line edit to the helpers table, closing the low nit from my last two passes:

-| `formatMonth value "format"` | Format a date as a month, e.g. `"LLL yyyy"` |
+| `formatMonth value "format"` | Format a date as a month using the same [date-fns](https://date-fns.org/docs/format) patterns, e.g. `"LLL yyyy"` |

Nothing else in the tree moved. formatDate (line 68) and formatMonth (line 69) now both name the library, and "the same … patterns" states the shared-formatDateUtc-wrapper relationship Igor described rather than just repeating the link — a reader who learns yyyy-LL-dd on one row knows it works on the other. That's the right call over a bare second link.

The token examples stay consistent with the named library: date-fns treats L/LL/LLL as stand-alone month, so "yyyy-LL-dd"2026-07-30 and "LLL yyyy"Jul 2026. yyyy is calendar year, not the YYYY date-fns warns about.

Findings

None. All four original findings remain resolved:

Prior finding Status
1 — Medium: unqualified vs qualified key form Resolved (line 18 + <Warning> at 42)
2 — Low: pivot unactionable Resolved (line 59 names rows, columns, cellMap, grandTotals, config)
3 — Low: forward reference to json Resolved (line 46)
4 — Low: duplicated "before the chart is rendered" Resolved (line 91)
5 — Low: formatMonth didn't name date-fns Resolved in this commit (line 69)

Threads

Both threads I opened are already resolved and outdated — nothing to resolve. No human-authored threads. No new inline comments this pass.

Verifiability

Unchanged: the renderer (formatDateUtc, helper registration, the CSP header) lives outside this repo, so the date-fns attribution and the img-src data: / font-src data: allowance rest on Igor's check against the implementation, which he reported holds. Both claims are stated precisely enough to re-check there.

Conventions

Re-checked and clean: title/description frontmatter, no body H1, body opens at H2, all fences tagged html, <Warning>/<Tip> are standard Mintlify, both internal links resolve (chart-types/index.mdx, charts/custom.mdx on disk), no new page so no docs.json change. Docs-only — no code, tests, security, or performance surface.

· Branch igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs ·

@igorlukanin
igorlukanin force-pushed the igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs branch from 6c691ee to 7f8ed7c Compare August 4, 2026 23:33
@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

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


Re-review of 7f8ed7c: content unchanged since 6c691ee, but a fresh pass over the helpers example turned up 1 medium — now used where Handlebars won't invoke it. 1 inline comment posted.

Full review
  • Diff the pushed commit against the previously reviewed state
  • Check existing review threads for dedup / stale resolution
  • Re-check helper examples against Handlebars strict-mode semantics
  • Post review

What this push contains

7f8ed7c carries the same tree I reviewed as 6c691ee — the four-commit history was rewritten (77a9f48, 102e34a, e4c9e17, 7f8ed7c), but git diff origin/master...HEAD is byte-for-byte the diff from my last pass: 66 insertions, 21 deletions, one file. So the synchronize is a rebase, not new content.

That means everything I signed off on last time still stands. What I did this pass is re-read the helpers section rather than just re-diffing it — and that surfaced something my earlier passes walked past.

New finding (medium)

html.mdx:80{{formatDate now "yyyy-LL-dd"}} likely throws under the strict mode this page documents.

now is listed as a helper at line 76, but line 80 uses it as a bare parameter. Handlebars resolves parameter paths against the context, not the helper registry — helpers in argument position require a subexpression, (now). Combined with strict mode (line 39), a context miss on now raises instead of rendering empty.

The fix depends on which now actually is:

  • Helper{{formatDate (now) "yyyy-LL-dd"}}
  • Context value the renderer injects → example is correct, but the row belongs in the Context table (line 52), not Helpers

Either way the two placements are currently inconsistent. Inline comment posted at line 80.

This has the same verifiability shape as the date-fns and CSP claims: helper registration is outside this repo, so it needs a check against the implementation. Worth doing before merge, since a broken example in the "here's how to use helpers" block undercuts a PR whose thesis is "every example on this page fails if copy-pasted."

Low nit

html.mdx:74coalesce value fallback is described as covering null or undefined, but under strict mode the undefined-from-a-missing-key case never reaches the helper; the lookup throws first. So coalesce guards null values in columns that exist, not absent columns — which is probably how a reader will try to use it after reading the strict-mode paragraph. Half a sentence would close the gap. Not blocking.

Prior findings

All five remain resolved — re-verified against the current file, not just carried forward:

Prior finding Status
1 — Medium: unqualified vs qualified key form Resolved (line 18 + <Warning> at 42)
2 — Low: pivot unactionable Resolved (line 59 names rows, columns, cellMap, grandTotals, config)
3 — Low: forward reference to json Resolved (line 46)
4 — Low: duplicated "before the chart is rendered" Resolved (line 91)
5 — Low: formatMonth didn't name date-fns Resolved (line 69)

Threads

Both threads I opened earlier are already resolved and outdated — nothing to resolve, no duplicates to skip. No human-authored threads.

Conventions

Re-checked and clean: title/description frontmatter, no body H1, body opens at H2, all fences tagged html, <Warning>/<Tip> are standard Mintlify, both internal links resolve (chart-types/index.mdx, charts/custom.mdx on disk), no new page so no docs.json change. Docs-only — no code, tests, security, or performance surface.

· Branch [`igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs`](https://github.com/cube-js/cube/tree/igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs)

Comment thread docs-mintlify/docs/explore-analyze/charts/chart-types/html.mdx Outdated
@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

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


Re-review of 25d4dcc: the now subexpression fix is correct — 2 low follow-ups on the explanation around it, none blocking. 1 inline comment posted. My earlier coalesce nit was wrong; retracted below.

Full review
  • Diff the new commit against the previously reviewed state
  • Verify the now subexpression fix against Handlebars compilation semantics
  • Re-check the coalesce nit and all prior findings
  • Check review threads for dedup / stale resolution, post review

What changed in 25d4dcc

Three lines, all addressing the now finding from my last pass:

-| `now` | The current date |
+| `now` | The current date. Wrap it in parentheses when passing it to another helper: `(now)` |
-<p>as of {{formatDate now "yyyy-LL-dd"}}</p>
+<p>as of {{formatDate (now) "yyyy-LL-dd"}}</p>
+
+A helper used as an argument to another helper has to be a parenthesized subexpression. A bare
+`{{formatDate now "yyyy-LL-dd"}}` reads `now` as a context lookup rather than the helper, so it
+passes nothing to `formatDate` and renders an empty string.

Right resolution, and the better of the two I offered — now really is a helper, so the example needed the subexpression, and the added paragraph teaches the rule rather than just patching the one call site. The helpers-table row carries the same note, which is where a reader scanning for now will actually look.

Correction to my last review

I said a bare now in argument position would throw under strict mode. That's wrong, and the new paragraph has it right. Handlebars only guards the terminal segment with container.strict(...) for paths in statement / simple-mustache position — path.strict is set in simpleSexpr and ambiguousSexpr, not in pushParam. An argument path compiles to a plain lookupProperty chain and yields undefined. So the mechanism is "passes nothing", not "raises".

That also retracts the low nit I raised last pass about coalesce value fallback (line 74). I argued the undefined case couldn't reach the helper because strict mode would throw on the lookup first. It can — argument lookups don't throw, so {{coalesce missingColumn "n/a"}} does reach coalesce with undefined. "null or undefined" is accurate as written; nothing to change there.

Findings (2 low)

Both on the new paragraph, both inline at line 83:

# Severity Location Issue
1 Low html.mdx:83 vs :39 Line 39 says a missing property "raises an error instead of rendering an empty string"; line 83 says a missing now renders an empty string. Both are true — strict mode covers rendered expressions, not helper arguments — but the page never states the distinction, so the two paragraphs read as contradicting each other. A half-clause at line 39 closes it.
2 Low html.mdx:83 "renders an empty string" describes formatDate's behavior, not Handlebars'. Handlebars passes undefined; date-fns' format(undefined, …) throws RangeError: Invalid time value, so unless formatDateUtc guards its input the visible result is a broken chart. "…so formatDate doesn't receive the current date" is true either way and needs no verification.

Prior findings

All six remain resolved — re-verified against the current file:

Prior finding Status
1 — Medium: unqualified vs qualified key form Resolved (line 18 + <Warning> at 42)
2 — Low: pivot unactionable Resolved (line 59 names rows, columns, cellMap, grandTotals, config)
3 — Low: forward reference to json Resolved (line 46)
4 — Low: duplicated "before the chart is rendered" Resolved (line 93)
5 — Low: formatMonth didn't name date-fns Resolved (line 69)
6 — Medium: now in parameter position Resolved in this commit (lines 76, 80, 83)
— Low: coalesce undefined case Retracted — my reasoning was wrong, see above

Threads

All three threads I opened are resolved and outdated — nothing to resolve, no duplicates to skip. No human-authored threads.

Verifiability

Unchanged: the renderer (formatDateUtc, helper registration, the CSP header) lives outside this repo, so the date-fns attribution and the img-src data: / font-src data: allowance rest on Igor's check against the implementation, which he reported holds. Handlebars isn't installed in this checkout either, so the strict-mode / argument-position reasoning above is from the compiler's source (javascript-compiler.js resolvePath/strictLookup, compiler.js simpleSexpr), not an executed test — worth a 30-second handlebars.compile(s, {strict: true}) check locally if you want it nailed down before rewording line 39.

Conventions

Re-checked and clean: title/description frontmatter, no body H1, body opens at H2, all fences tagged html, <Warning>/<Tip> are standard Mintlify, both internal links resolve (chart-types/index.mdx, charts/custom.mdx on disk), no new page so no docs.json change. Docs-only — no code, tests, security, or performance surface.

· Branch [`igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs`](https://github.com/cube-js/cube/tree/igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs)

Comment thread docs-mintlify/docs/explore-analyze/charts/chart-types/html.mdx Outdated
@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 44s —— View job


Re-review of ccce9f7: both low findings from my last pass are closed. No findings — clean, ready to merge.

Full review
  • Diff the new commit against the previously reviewed state
  • Verify the two low findings from last pass are addressed
  • Re-check prior findings and conventions
  • Check review threads for dedup / stale resolution, post review

What changed in ccce9f7

Two lines, both from the pair of low findings I raised on 25d4dcc:

-...raises an error instead of rendering an empty string. If a chart fails to render...
+...raises an error instead of rendering an empty string. Helper arguments are the one
+exception — a path that misses there is passed to the helper as `undefined`. If a chart
+fails to render...
-...reads `now` as a context lookup rather than the helper, so it passes nothing to
-`formatDate` and renders an empty string.
+...reads `now` as a context lookup rather than the helper, so `formatDate` never receives
+the current date.

Both land where I'd hoped. The exception clause at line 39 is the right place for it — it's stated once, at the point the strict-mode rule is introduced, so line 83 doesn't have to re-litigate it. And "never receives the current date" is the claim that holds regardless of how formatDateUtc treats undefined, so the sentence no longer depends on a wrapper this repo can't see.

A side effect worth noting: the new clause also retroactively makes the coalesce value fallback row (line 74) coherent. "Use fallback when value is null or undefined" previously looked like it contradicted the strict-mode paragraph; now the page explains why a helper can see undefined at all. That was the nit I raised and then retracted two passes ago — it's now not just accurate but explained.

Findings

None.

Prior finding Status
1 — Medium: unqualified vs qualified key form Resolved (line 18 + <Warning> at 42)
2 — Low: pivot unactionable Resolved (line 59 names rows, columns, cellMap, grandTotals, config)
3 — Low: forward reference to json Resolved (line 46)
4 — Low: duplicated "before the chart is rendered" Resolved (line 93)
5 — Low: formatMonth didn't name date-fns Resolved (line 69)
6 — Medium: now in parameter position Resolved (lines 76, 80, 83)
7 — Low: line 39 vs line 83 tension Resolved in this commit (line 39)
8 — Low: "renders an empty string" was a formatDate claim Resolved in this commit (line 83)
— Low: coalesce undefined case Retracted earlier; the wording is correct and now explained

Threads

All four threads I opened are resolved and outdated — nothing to resolve, no duplicates to skip. No human-authored threads. No new inline comments this pass.

Verifiability

Unchanged and unaffected by this commit: the renderer (formatDateUtc, helper registration, the CSP header) lives outside this repo, so the date-fns attribution and the img-src data: / font-src data: allowance rest on Igor's checks against the implementation, which he reported hold. The strict-mode / argument-position claim now documented at line 39 matches the Handlebars compiler source (path.strict set in simpleSexpr/ambiguousSexpr only; argument paths compile to a plain lookupProperty chain), and Igor confirmed the runtime path through toDate at htmlTemplateEngine.ts:504.

Conventions

Re-checked and clean: title/description frontmatter, no body H1, body opens at H2, all fences tagged html, <Warning>/<Tip> are standard Mintlify, both internal links resolve (chart-types/index.mdx, charts/custom.mdx on disk), no new page so no docs.json change. Docs-only — no code, tests, security, or performance surface.

· Branch [`igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs`](https://github.com/cube-js/cube/tree/igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs)

@igorlukanin
igorlukanin force-pushed the igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs branch from ccce9f7 to bd0da8c Compare August 13, 2026 12:26
@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

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


Re-review of bd0da8c: rebase only — content identical to the state I signed off on. No findings; 1 cosmetic nit. No inline comments.

Full review
  • Diff the pushed commits against the previously reviewed state
  • Verify prior findings remain resolved
  • Re-check conventions, links, and examples
  • Check review threads for dedup / stale resolution, post review

What this push contains

The six-commit history was rewritten (7d8325f, 85f0ce0, 00497a3, 95c9511, f2ef278, bd0da8c), but git diff origin/master...HEAD is the same 68/21 single-file delta I reviewed as ccce9f7. The tip commit message ("note that helper arguments are exempt from strict mode") is the same change that closed my last two findings; the tree carries both of them:

  • html.mdx:39 — "Helper arguments are the one exception — a path that misses there is passed to the helper as undefined."
  • html.mdx:83 — "so formatDate never receives the current date."

So this is a rebase, not new content, and the prior verdict stands.

Findings

None. All eight prior findings remain resolved — re-verified against the current file, not carried forward:

Prior finding Status
1 — Medium: unqualified vs qualified key form Resolved (line 18 + <Warning> at 42)
2 — Low: pivot unactionable Resolved (line 59 names rows, columns, cellMap, grandTotals, config)
3 — Low: forward reference to json Resolved (line 46)
4 — Low: duplicated "before the chart is rendered" Resolved (line 93)
5 — Low: formatMonth didn't name date-fns Resolved (line 69)
6 — Medium: now in parameter position Resolved (lines 76, 80, 83)
7 — Low: line 39 vs line 83 tension Resolved (line 39)
8 — Low: "renders an empty string" was a formatDate claim Resolved (line 83)
— Low: coalesce undefined case Retracted earlier; correct as written and now explained by line 39

Cosmetic nit (not worth a commit on its own)

html.mdx:51-55 — the sentence introduces the table as "Alongside data, the following are available", and then data is the table's first row. Harmless, and the row is genuinely useful as the place the "keyed by column name" shape is stated in table form; "The following bindings are available" would remove the small self-contradiction if you touch the file again for another reason.

Verifiability

Unchanged and unaffected by a rebase. The renderer (formatDateUtc, toDate, helper registration, the CSP header) lives outside this repo, so the date-fns attribution, the img-src data: / font-src data: allowance, and the now-is-a-helper claim rest on Igor's checks against htmlTemplateEngine.ts, which he reported hold. The strict-mode / argument-position rule now documented at line 39 matches the Handlebars compiler source (path.strict set in simpleSexpr/ambiguousSexpr only; argument paths compile to a plain lookupProperty chain).

Threads

All four threads I opened are resolved and outdated — nothing to resolve, no duplicates to skip. No human-authored threads.

Conventions

Re-checked and clean: title/description frontmatter, no body H1, body opens at H2, all fences tagged html, <Warning>/<Tip> are standard Mintlify, both internal links resolve (chart-types/index.mdx, charts/custom.mdx on disk), page already registered in docs.json:79 so no navigation change needed. Docs-only — no code, tests, security, or performance surface.

Nothing blocking. Ready to merge from my side.

· Branch [`igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs`](https://github.com/cube-js/cube/tree/igor/cub-2467-clarify-html-chart-js-support-and-handlebars-docs)

@igorlukanin

Copy link
Copy Markdown
Member Author

Nit's right, and I'm leaving it: the Rust jobs this push triggered take ~an hour, and a wording commit now would cancel them mid-flight. Folded into the next commit if the file gets touched for another reason.

On those jobs, since it'll look odd on a docs-only PR: Rust fires on the push event, whose path filter GitHub matches against the pushed commit range rather than the PR diff, so re-parenting onto 26 master commits pulls rust/cubestore/** in. Same thing happened on the two earlier pushes here and went green both times.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant