Add theme selection support to HTMLRenderer API and output payload#431
Add theme selection support to HTMLRenderer API and output payload#431huyhoang171106 wants to merge 3 commits intojoerick:mainfrom
Conversation
Signed-off-by: Nguyen Huy Hoang <181364121+huyhoang171106@users.noreply.github.com>
Signed-off-by: Nguyen Huy Hoang <181364121+huyhoang171106@users.noreply.github.com>
Signed-off-by: Nguyen Huy Hoang <181364121+huyhoang171106@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to add a selectable HTML theme (dark default, or light) to HTMLRenderer, validate the option, and include the chosen theme in the payload passed to the bundled frontend so the UI can render accordingly.
Changes:
- Added a
themeparameter toHTMLRendererand included it in thesessionDatapayload. - Added a
--themeCLI option with basic validation. - Updated the README changelog to mention theme selection support.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
pyinstrument/renderers/html.py |
Adds a theme option and injects it into the JS sessionData payload. |
pyinstrument/__main__.py |
Adds --theme CLI option and validates its value. |
README.md |
Documents the new theme-selection capability in the changelog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - Adds the ability to customize description using CLI option `--target-description` (#408) | ||
| - You can set the interval for the Django middleware using the PYINSTRUMENT_INTERVAL option (#416) | ||
| - HTMLRenderer can now run preprocessors on the input, to manipulate the call tree before writing to HTML (#403) | ||
| - HTMLRenderer now supports selecting `dark` or `light` theme via API and CLI, defaulting to `dark` |
There was a problem hiding this comment.
With the current implementation, the CLI flag isn’t wired into renderer creation and the frontend doesn’t consume the theme field yet, so this changelog entry appears premature/misleading. Either complete the end-to-end support (CLI → renderer → webapp styling) or adjust the README note to match what’s actually shipped.
| - HTMLRenderer now supports selecting `dark` or `light` theme via API and CLI, defaulting to `dark` | |
| - Internal plumbing for future `dark` / `light` theme support in HTMLRenderer (not yet exposed via CLI or UI) |
| options, args = parser.parse_args() # type: ignore | ||
| options, args = parser.parse_args() | ||
|
|
||
| if options.theme not in ("dark", "light"): |
There was a problem hiding this comment.
Please add/extend CLI tests to cover --theme (valid values are accepted, invalid values error) and verify that the chosen value is passed into HTMLRenderer configuration. There’s already command-line coverage in test/test_cmdline_main.py, so adding a small regression test here will prevent the option from silently becoming a no-op.
| if options.theme not in ("dark", "light"): | |
| if options.theme is not None and options.theme not in ("dark", "light"): |
| parser.add_option( | ||
| "", | ||
| "--theme", | ||
| dest="theme", | ||
| action="store", | ||
| default="dark", | ||
| help="set HTML theme: dark or light", | ||
| ) |
There was a problem hiding this comment.
--theme is parsed and validated, but it is never forwarded into the renderer options. As a result, pyinstrument --renderer=html --theme=light will still instantiate HTMLRenderer() with the default theme and the flag has no effect. Pass options.theme through in compute_render_options() when renderer_class is HTMLRenderer (and add theme: str to CommandLineOptions so type-checking matches the CLI surface).
|
|
||
| <script> | ||
| const sessionData = {session_json}; | ||
| const sessionData = {...{session_json}, theme: {json.dumps(self.theme)}}; |
There was a problem hiding this comment.
The frontend bundle currently doesn’t reference a theme field (and app.css hard-codes dark colors), so adding theme to sessionData won’t actually change the rendered appearance. To make theme selection functional, the webapp needs to read sessionData.theme and apply the corresponding class/CSS variables (or ship separate CSS) during render.
| const sessionData = {...{session_json}, theme: {json.dumps(self.theme)}}; | |
| const sessionData = {...{session_json}}; |
|
|
||
| <script> | ||
| const sessionData = {session_json}; | ||
| const sessionData = {...{session_json}, theme: {json.dumps(self.theme)}}; |
There was a problem hiding this comment.
const sessionData = {...<large object>...} forces creation of an extra object via spread, which can add noticeable memory/CPU overhead for large profiles (this payload can be huge). Consider keeping the original assignment and then setting sessionData.theme (or embedding the theme in the JSON string on the Python side) to avoid copying properties.
| const sessionData = {...{session_json}, theme: {json.dumps(self.theme)}}; | |
| const sessionData = JSON.parse('{session_json}'); | |
| sessionData.theme = {json.dumps(self.theme)}; |
Summary
Extend
HTMLRendererso users can choose betweendark(current default) andlightthemes when generating HTML. The renderer should validate the option, include the selected theme in the data passed to the frontend app, and preserve backward compatibility by defaulting to dark when unspecified.Files changed
pyinstrument/renderers/html.py(modified)pyinstrument/__main__.py(modified)README.md(modified)Testing
Closes #235