feat: Tradingview Lightweight Charts - #1402
Open
jnumainville wants to merge 44 commits into
Open
Conversation
… restore correctly.
Co-authored-by: Copilot <copilot@github.com>
# Conflicts: # package-lock.json # package.json # tests/app.d/tests.app
# Conflicts: # package-lock.json # tests/app.d/tests.app
*_series constructors and chart-level kwargs on per-type constructors were removed; compose via tvl.chart(...) instead. Fixes server startup crash in e2e docker container. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s them Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # tests/app.d/tests.app
Fix pre-existing lint errors that fail `npm run test:ci` (test:ci:lint): - Move @deephaven/dashboard-core-plugins from devDependencies to dependencies (imported by non-test source; import/no-extraneous-deps) - strict-boolean-expressions: explicit null/empty checks - no-shadow: rename shadowing pendingDs locals to pendingResample - no-param-reassign: house-style inline disables for intentional mutation - no-nested-ternary: hoist chart-type formatter selection to if/else - drop unused error/isLoading state value bindings - normalize isDownsampleSwap to a definite boolean - consistent-type-imports, no-empty-function, react/destructuring-assignment, jsx-props-no-spreading, max-classes-per-file, and prettier formatting Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove 6 of the 7 eslint-disable comments added when first fixing lint, addressing the root causes instead: - no-explicit-any: pendingDownsample is a public field; drop the `as any` - no-param-reassign (x2): mutate via array-index locals, not forEach params - jsx-props-no-spreading (x2): destructure and pass props explicitly in tests - max-classes-per-file: make MockResizeObserver a mock factory (one class left) The one remaining disable (react-hooks/exhaustive-deps on the DOM-derived useDHChartTheme memo) is kept intentionally — it matches the established convention in the ui plugin for memos that depend on non-scope DOM state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The tvl plugin lagged the rest of the repo (e.g. dashboard-core-plugins ^0.85.42, components ^1.2.0, plugin ^1.2.0). Bump the shared @deephaven/* deps to the versions the sibling plugins (pivot / pivot-builder / plotly- express) pin, which are already hoisted at the workspace root: - components ^1.22.1, dashboard ^1.24.0, dashboard-core-plugins ^1.24.0, jsapi-bootstrap ^1.23.0, log ^1.8.0, plugin ^1.24.0, redux ^1.23.0, utils ^1.10.0, jsapi-types ^1.0.0-dev0.39.6 This dedupes the previously-nested dashboard-core-plugins@0.85.x tree onto the root 1.24.0 install (removing ~167 stale packages). Verified: repo-wide tsc, full build, and test:ci (unit + lint) all pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cut the tradingview-lightweight plugin's eslint-disable comments from 72 to 4 by fixing the underlying issues instead of suppressing them: Production source: - no-explicit-any (33): use real @deephaven/jsapi-types / lightweight-charts types (Widget.sendMessage, PartitionedTable, plot.Downsample, DateWrapper, Event<T>, IRange/LogicalRange), access now-public model fields directly, and add a declaration.d.ts for the `*.css?inline` import (drops @ts-ignore) - no-param-reassign (2): resolve colors into locals, not the params - no-continue (3): wrap loop bodies in positive conditions - no-non-null-assertion (1): get-or-create map entry - class-methods-use-this (1): make unwrapValue static - import/prefer-default-export (1): default-export the plugin Tests: - no-new (16): assign construction to a var + assert defined (house pattern) - no-underscore-dangle (9) + no-var-requires (1): expose mock internals under non-underscore names, import the same mock instance via `import * as` - no-explicit-any (2): `as never` / typed casts The 4 remaining are deliberate and match repo convention: react-hooks/ exhaustive-deps on two DOM-derived effects/memos (as the ui plugin does), and no-underscore-dangle on the intentional `__tvlTestHook` global debug hook. Verified: repo-wide tsc, build, and test:ci (unit + lint, 332 suites) all pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
ui docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
tradingview-lightweight docs preview (Available for 14 days) |
|
ui docs preview (Available for 14 days) |
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces the beta TradingView Lightweight Charts plugin, including Python APIs, JavaScript rendering, documentation, tests, and snapshot infrastructure.
Changes:
- Adds chart types, continuous time rendering, themes, tooltips, events, and downsampling.
- Adds Python, Jest, Playwright, and documentation coverage.
- Extends Docker-based documentation snapshot generation.
Reviewed changes
Copilot reviewed 54 out of 623 changed files in this pull request and generated 10 comments.
Show a summary per file
| File(s) | Description |
|---|---|
plugins/tradingview-lightweight/** |
New plugin implementation, packaging, tests, fixtures, docs, and snapshots. |
tools/image-snapshotter/** |
New chart-image snapshot generation and validation tooling. |
tests/** |
Adds TVL fixtures and stabilizes visual tests. |
plugins/ui/.../useDebouncedOnChange* |
Prevents callback replacement from dropping pending changes. |
sphinx_ext/deephaven_autodoc.py |
Handles raises sections and MDX brace escaping. |
docker-compose.docs*.yml, docker/python*/** |
Integrates TVL into documentation containers. |
tools/run_docker.sh, package.json |
Updates snapshot orchestration and diagnostics. |
.github/actions/get-changes/action.yml |
Adds TVL change detection. |
.gitignore |
Adds Python artifact exclusions. |
Suppressed comments (3)
plugins/tradingview-lightweight/README.md:38
chart()deliberately hard-broke these flat kwargs. The runnable example must use the groupedcrosshairandtime_scaleobjects or it raisesTypeError.
plugins/tradingview-lightweight/docs/README.md:60- This terminology entry repeats the nonexistent
pane_indexparameter; the constructors select a pane withpane=.
plugins/tradingview-lightweight/README.md:134 plugin_builder.pyhas no--pluginoption; plugins are positional arguments, and starting the separate server requires--server. This documented local workflow exits with an unknown-option error.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+24
to
+25
| tooltip_visible=True, | ||
| tooltip_value_precision=2, |
| # Simple candlestick chart | ||
| chart = tvl.candlestick( | ||
| ohlc_table, | ||
| time="Timestamp", |
| ```python order=my_chart | ||
| from deephaven.plot import tradingview_lightweight as tvl | ||
| t = tvl.data.values() | ||
| my_chart = tvl.line(t, time="Timestamp", value="Value") |
| - **Live Table Support**: Direct integration with real-time Deephaven tables, so charts update as the underlying data ticks. | ||
| - **Viewport-Aware Downsampling**: Pixel-accurate, whitespace-based downsampling that keeps panning and zooming smooth on multi-million-row series. | ||
| - **Server-Side Autobinning**: Histograms compute bin widths and counts directly in the Deephaven query engine, avoiding round-trips of raw data. | ||
| - **Multi-Pane Stacks**: Price, volume, and indicator series can be stacked into separate panes with independent height ratios via `pane_index`. |
Comment on lines
+20
to
+27
| setupFilesAfterEach: [ | ||
| ...(baseConfig.setupFilesAfterEach ?? []), | ||
| path.join(__dirname, 'jest.setup.ts'), | ||
| ], | ||
| setupFiles: [ | ||
| ...(baseConfig.setupFiles ?? []), | ||
| path.join(__dirname, 'jest.setup.ts'), | ||
| ], |
Comment on lines
+33
to
+34
| cp -rf "$PLUGIN_ROOT/src/js/dist/." "$DEST"/ | ||
| echo "[e2e] synced freshly built JS bundle into the installed package" |
| "update-dh-packages:ui": "npm run update-dh-packages -- --scope=@deephaven/js-plugin-ui --", | ||
| "validate-marketplace": "node tools/validate-marketplace.mjs", | ||
| "update-doc-snapshots": "./tools/run_docker.sh ./docker-compose.docs-snapshots.yml deephaven-plugins-docs-snapshotter", | ||
| "update-doc-snapshots": "docker compose -f ./docker/python/docker-compose.yml build server && (./tools/run_docker.sh ./docker-compose.docs-snapshots.yml deephaven-plugins-docs-image-snapshotter-tvl; ec=$?; ./tools/run_docker.sh ./docker-compose.docs-snapshots.yml deephaven-plugins-docs-error-reporter || true; exit $ec)", |
Comment on lines
+152
to
+160
| if ! node /work/dist/validate-assets-cli.js "${SNAPSHOTS_HOST_DIR}"; then | ||
| VALIDATE_EXIT=$? | ||
| echo "[entrypoint] snapshot asset validation failed (exit ${VALIDATE_EXIT})." >&2 | ||
| # Prefer reporting the playwright failure if there was one, otherwise | ||
| # surface the validator's exit code. | ||
| if [ "${PLAYWRIGHT_EXIT}" = "0" ]; then | ||
| exit "${VALIDATE_EXIT}" | ||
| fi | ||
| fi |
Comment on lines
+28
to
+39
| let raw: string; | ||
| try { | ||
| raw = readFileSync(full, 'utf8'); | ||
| } catch { | ||
| continue; | ||
| } | ||
| let parsed: { objects?: Record<string, ObjEntry> }; | ||
| try { | ||
| parsed = JSON.parse(raw); | ||
| } catch { | ||
| continue; | ||
| } |
|
|
||
| ### Build the docs locally | ||
|
|
||
| From the repo root: |
mofojed
requested changes
Aug 27, 2026
mofojed
left a comment
mofojed
left a comment
Member
There was a problem hiding this comment.
I think this needs to be split up into at least a couple PRs:
- TVL plugin. Should just be the changes for the TVL plugin and anything to wire that up; shouldn't include changes to other tests (like dialog/table/plotly - why? I don't think we want to check those changes in)
- image-snapshotter - I think this should be it's own PR. We could use the screenshotter in deephaven.ui as well.
I don't think we need the TVL plugin to be gated on the image-snapshotter or vice versa... @dsmmcken any opinions?
Member
There was a problem hiding this comment.
This isn't what we do with the plotly tests, we just pass a seed into the data generator. Not sure I like changing up our CI for the trading view plugin...
| @@ -447,7 +447,7 @@ def t_selection_component(): | |||
|
|
|||
| t_programmatic_sort_abs_desc = ui.table( | |||
| _programmatic_sort_data, | |||
| sorts=ui.TableSort(column="Name", direction="DESC", is_abs=True), | |||
| sorts=ui.TableSort(column="SepalLength", direction="DESC", is_abs=True), | |||
Contributor
There was a problem hiding this comment.
It doesn't belong in this PR, but this test is actually currently wrong, identified by claude. It's supposed to test is_abs, but actually errors to console during run (still produces a screenshot so it is not failing). is_abs on a string column doesn't work, should have been on a valid column.
Member
There was a problem hiding this comment.
This file doesn't seem to be referenced by the tests.app...
Member
There was a problem hiding this comment.
This file doesn't seem to be referenced either... what is it for? Maybe remove it or put it somewhere more relevant.
Contributor
There was a problem hiding this comment.
just delete it, was probably transitory during development.
Member
There was a problem hiding this comment.
Why are all these other tests updated? Should only have new TVL tests.
Member
There was a problem hiding this comment.
This image-snapshotter tool should really be it's own PR.
Also, we should probably call it screenshotter instead. So it's clear it's taking a screenshot, instead of conflating it with other snapshot terminology in tests (which generally refer to data snapshots).
Comment on lines
+64
to
+76
| SNAPSHOTTER_BASE_URL: 'http://server:10000/ide/' | ||
| SNAPSHOTTER_PLUGIN_ROOT: '/work' | ||
| SNAPSHOTTER_PLUGIN: 'tradingview-lightweight' | ||
| SNAPSHOTTER_TARGET_SELECTOR: '.dh-tvl-chart' | ||
| SNAPSHOTTER_WIDGET_TYPE: 'deephaven.plot.tradingview_lightweight.TvlChart' | ||
| SNAPSHOTTER_APP_ID: 'tvl.docs.examples' | ||
| SNAPSHOTTER_APP_NAME: 'TVL Docs Examples' | ||
| SNAPSHOTTER_COMPOSE_FILE: '/workspace/docker/python-snapshotter/docker-compose.yml' | ||
| SNAPSHOTTER_SERVER_CONTAINER: 'tvl-snapshotter-server' | ||
| # Pass through the host's SNAPSHOTTER_FORCE so callers can opt in to | ||
| # cache-busting recapture without editing this file. | ||
| SNAPSHOTTER_FORCE: ${SNAPSHOTTER_FORCE:-} | ||
| CI: 'true' |
Member
There was a problem hiding this comment.
With how many env vars there are, might make sense to make a config file instead that gets passed in. Would be easier to understand IMO
| - ./plugins/tradingview-lightweight/docs/snapshots:/results/tradingview-lightweight | ||
| - ./snapshot-results/errors:/errors | ||
|
|
||
| # Pass 2: capture widget PNGs for the tradingview-lightweight docs. Runs |
Member
There was a problem hiding this comment.
So this pass 2 should be part of the screenshotter PR.
Contributor
Yeah, at one point I let claude just go ham with the instruction to fix all flaky e2e tests to get it to solve all issues with the TVL tests that had various failures. Which ended up fixing far more than just TVL... but no they don't belong in this PR. Should have claude move them all to an e2e flakiness fix pr. |
These fixes had nothing to do with the TradingView plugin — they were picked up along the way while getting the e2e suite green. They now live in deephaven#1406 so they can land and be reviewed on their own: - ag_grid / express / theme / ui_plotly / ui_table screenshot waits - the ui_dialog dialog-scoped screenshots and their baselines - the `t_programmatic_sort_abs_desc` numeric-column fixture and baselines - the `useDebouncedOnChange` pending-call fix and its unit tests - the Plotly wait helpers and the `waitForGridRender` band sampling `tests/utils.ts` is left byte-identical to deephaven#1406 plus `waitForTvlSettled`, since the tvl specs call `openPanel` without a locator and so depend on its tab-title path. Once deephaven#1406 merges, merging main here collapses that file's diff down to `waitForTvlSettled` alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Split the unrelated flaky e2e fixes out of this PR into #1406 (
PR now depends on #1406 landing first. Also dropped |
|
ui docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
tradingview-lightweight docs preview (Available for 14 days) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 605 changed files in this pull request and generated 8 comments.
Suppressed comments (6)
plugins/tradingview-lightweight/README.md:13
- The public constructor accepts
timestamp=, nottime=, so the first copyable usage example raisesTypeError.
plugins/tradingview-lightweight/README.md:51 - These commands use plugin-relative paths (
../../sphinx_ext, localdist, and localmake_docs.py), so following the stated repository-root instruction fails. Identify the plugin directory as the working directory.
plugins/tradingview-lightweight/docs/README.md:36 - The constructors expose
pane=, notpane_index=, so this feature summary points users to a nonexistent API parameter.
plugins/tradingview-lightweight/docs/README.md:60 - The terminology entry repeats the nonexistent
pane_indexparameter; series select their pane withpane=.
plugins/tradingview-lightweight/docs/example-data.md:23 stocks()creates a 360-row seed and symbol-specific seeded random walks, not a 120-row sine-wave table. The documented shape and generation model are both incorrect.
plugins/tradingview-lightweight/docs/example-data.md:37volume()uses seeded lognormal noise scaled by weekday factors, not overlapping sine waves, so this description does not match the generated example data.
| return syms[seed % 3] | ||
|
|
||
| def sym_step(sym: str, seed: int) -> float: | ||
| return Random(seed * 7 + hash(sym) % 1000).gauss(0.0, sym_vols[sym]) |
Comment on lines
+37
to
+38
| crosshair_mode="magnet", | ||
| time_visible=True, |
| needs to match CI. | ||
|
|
||
| ```shell | ||
| python tools/plugin_builder.py --plugin tradingview-lightweight # separate terminal |
| @@ -0,0 +1,103 @@ | |||
| # Example data | |||
|
|
|||
| The Deephaven TradingView Lightweight package includes several built-in, deterministically generated tables for testing and example code. Each function returns a fresh `deephaven.table.Table` whose values are produced by closed-form expressions (no random data), so snapshot tests stay stable across runs. | |||
| renderer: TradingViewChartRenderer, | ||
| model: TradingViewChartModel | ||
| ): boolean { | ||
| if (model.isResampling()) return true; |
Comment on lines
+46
to
+48
| if (!existsSync(snapshotsDir) || !statSync(snapshotsDir).isDirectory()) { | ||
| return { missing, envelopesScanned, imageRefsScanned }; | ||
| } |
Comment on lines
+140
to
+143
| const seen = new Map<string, Block>(); | ||
| for (const b of nonSkip) { | ||
| if (!seen.has(b.md5)) seen.set(b.md5, b); | ||
| } |
Comment on lines
+872
to
+874
| options surface). Chart-level styling (background_color, | ||
| watermark_text, crosshair_mode, time_visible, width, height, etc.) | ||
| lives on :func:`chart` — wrap this result in ``tvl.chart(...)`` to |
disconnect_test.py and pv_isolate.py were debugging scratch files. Neither is listed in tests.app, so nothing ever loaded them, and nothing else in the repo references them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
ui docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
tradingview-lightweight docs preview (Available for 14 days) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 603 changed files in this pull request and generated no new comments.
Suppressed comments (3)
package.json:34
|| truediscards failures reported by the error-reporter service, and the recipe always returns the snapshotter's status. Consequently, a docs example can fail during app.d startup while the snapshotter exits zero after skipping its missing chart, and this command still succeeds. Preserve the snapshotter status when it failed, but otherwise return the reporter status.
plugins/tradingview-lightweight/README.md:14- The public quickstart calls
candlestickwith an unsupportedtimekeyword; the API parameter istimestamp. Running this example raisesTypeErrorbefore a chart is created.
plugins/tradingview-lightweight/README.md:38 - These flat chart keywords were removed by the grouped-options API, so the advertised multi-series example raises
TypeError. Pass the corresponding factory results throughcrosshairandtime_scale.
dsmmcken
added a commit
that referenced
this pull request
Aug 27, 2026
Split out of #1402, which picked up a handful of e2e fixes that have nothing to do with the TradingView plugin. Most of the flakiness is the same bug in different places: a screenshot gets taken before the widget's data has painted. The loading spinner clears when the widget *mounts*, not when it has data, and Playwright's auto-stabilization doesn't help — a blank plot is static, so two frames match and it locks onto the empty state. - `waitForPlotlyData` / `waitForPlotlyIndicator` in `tests/utils`, used by the express, ui_plotly and theme specs. Checks Plotly's data model rather than the DOM so WebGL traces are covered too. Indicators get their own helper because their value is a scalar, not an array. - ag_grid waits for a row, ui_table waits for `waitForGridRender`. - `waitForGridRender` samples bands instead of reading back the whole canvas — the full `getImageData` is ~8MB at 1080p and was wedging the page's main thread under a parallel run. - Page-load and Panels-button timeouts up to 45s; three browsers against one server regularly blow past the default 15s. - `openPanel` only counts `.dh-panel` when the caller passed a specific locator — the generic selector also matches the server's default panels, which race the count. Otherwise it checks the tab title. Two others while in here: - **ui_dialog** screenshots the dialog instead of the page. The modal and fullscreen dialogs cover the viewport, so the full-page shots were also capturing IDE chrome that drifts run to run (console log lines, heap indicator), which made the baselines ping-pong. Popover and tray were already scoped and are untouched. - **`useDebouncedOnChange`** was dropping keystrokes. The server hands down a new `on_change` on every re-render, and `useDebouncedCallback` cancels its pending trailing call when the callback identity changes — so typing "world" quickly sends "wor", the re-render lands, and "world" never reaches Python. Now keeps one debounced instance and reads the latest callable from a ref. `t_programmatic_sort_abs_desc` also had `is_abs=True` on a string column, which means nothing; pointed it at `SepalLength`, which has negatives. ## Testing `tsc` and eslint/prettier clean, `useDebouncedOnChange` unit tests pass. The regenerated baselines need a CI e2e run to confirm. --------- Co-authored-by: Don McKenzie <donmckenzie@illumon.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A record of most changes I made on top of Don's work is in
plans/DH-1376-tradingview-lightweight-review.mdI also added a
continuousargument (defaultTrue) to charts. Generally, this means data gaps aren't erased by default. This will make it so histograms are truly bar-to-bar, for a specific example. Setting toFalseswitches back to the default ordinal rendering mode for those desiring it.Since it is impossible to pin down everything without taking significant developer effort, I think our best bet is noting this as a beta plugin. If further work is highly desirable once initially surfaced we can dedicate more resources.