Conversation
There was a problem hiding this comment.
Code Review: Improve HTML Report Performance
No previous review comments to address.
Summary
This is a well-crafted PR with two distinct, focused performance improvements:
- CSS for smooth theme transitions — registers color tokens as typed properties so the browser interpolates them at level, eliminating per-element stagger.
- Lazy/deferred rendering of test detail panels — defers
renderDetail(t)until a panel is first expanded, dramatically reducing initial DOM size.
Both changes are architecturally sound and the code quality is high.
Strengths
Lazy rendering (biggest win):
The ensureDetailRendered pattern is clean and correct. The idempotency guard (det.dataset.rendered = '1') is simple and effective. All call sites have been updated consistently: row click, keyboard Enter, Expand All, hash navigation, and scrollToTest. This is the kind of change that makes a real difference for large test suites.
testById lookup map:
Using Object.create(null) is a good practice — no prototype chain, no risk of collisions with toString etc. The map is built once at startup, which is the right time.
content-visibility: auto on .grp-body-inner:
Good addition. The contain-intrinsic-size: auto 400px fallback with the auto keyword is the right choice since the browser remembers computed sizes after first render, avoiding layout thrash on repeated opens.
Theme transition refactor:
Moving the class from document.body to document.documentElement is the correct fix — the CSS selector .theme-transitioning * needs to be on the root element to suppress transitions for all descendants.
Concerns
1. Browser compatibility for @property (CSS Houdini)
@property is a CSS Houdini feature. Support matrix:
- Chrome/Edge 85+ ✅
- Safari 15.4+ ✅
- Firefox 128+ ✅ (relatively recent — July 2024)
For a dev/CI HTML report this is acceptable, but it's worth noting: in unsupported browsers (older Firefox, some mobile), the fallback is that the transition doesn't animate at all — the theme switch will snap. This is a degraded-but-functional experience. Consider adding a comment in the CSS explaining the browser requirement, so future maintainers understand why the approach was chosen.
2. Expand All still causes a synchronous DOM storm
The ensureDetailRendered pattern defers work nicely for individual opens, but Expand All still renders every detail panel synchronously in a single click handler:
container.querySelectorAll('.t-detail').forEach(function(d){ensureDetailRendered(d);d.classList.add('open');});For a report with 5,000+ tests this will block the main thread for a noticeable amount of time. A more resilient approach would be to chunk the work using requestIdleCallback or requestAnimationFrame batching. That said, Expand All is an intentionally heavy operation and this is a reasonable trade-off for now — the content-visibility: auto on .grp-body-inner will also help render performance after the DOM is built.
3. The @property initial values are dark-theme defaults
The initial-value declarations all reflect dark theme values (e.g. --bg { initial-value:#0b0d11 }). If a user has light theme saved in localStorage, the browser may briefly flash the dark values before the JS sets data-theme="light" on page load. This is a pre-existing concern with the report's theme initialisation, not introduced by this PR — but worth noting if you ever improve the FOUC handling.
Note on the Preact suggestion
The vanilla JS approach keeps the report self-contained (no CDN dependencies, works offline, no CSP concerns). Given the scope of the interactivity, I'd lean toward keeping it vanilla unless the complexity grows significantly. The lazy-rendering pattern introduced here proves vanilla can handle the perf concerns without a VDOM.
Verdict
The changes are focused, correct, and solve a real problem. The lazy-rendering architecture in particular is a solid improvement. The CSS @property approach is clever and well-commented. Happy with the direction — minor concerns noted above are not blockers.
There was a problem hiding this comment.
Code Review: Improve HTML Report Performance
No previous review comments to address.
Summary
This is a well-crafted PR with two distinct, focused performance improvements:
- CSS
@propertyfor smooth theme transitions - registers color tokens as typed properties so the browser interpolates them at:rootlevel, eliminating per-element stagger. - Lazy/deferred rendering of test detail panels - defers
renderDetail(t)until a panel is first expanded, dramatically reducing initial DOM size.
Both changes are architecturally sound and the code quality is high.
Strengths
Lazy rendering (biggest win):
The ensureDetailRendered pattern is clean and correct. The idempotency guard (det.dataset.rendered = '1') is simple and effective. All call sites have been updated consistently: row click, keyboard Enter, Expand All, hash navigation, and scrollToTest. This is the kind of change that makes a real difference for large test suites.
testById lookup map:
Using Object.create(null) is a good practice - no prototype chain, no risk of collisions with toString etc. The map is built once at startup, which is the right time.
content-visibility: auto on .grp-body-inner:
Good addition. The contain-intrinsic-size: auto 400px fallback with the auto keyword is the right choice since the browser remembers computed sizes after first render, avoiding layout thrash on repeated opens.
Theme transition refactor:
Moving the class from document.body to document.documentElement is the correct fix - the CSS selector .theme-transitioning * needs to be on the root element to suppress transitions for all descendants.
Concerns
1. Browser compatibility for @property (CSS Houdini)
@property is a CSS Houdini feature. Support matrix:
- Chrome/Edge 85+ - supported
- Safari 15.4+ - supported
- Firefox 128+ - supported (relatively recent, July 2024)
For a dev/CI HTML report this is acceptable, but in unsupported browsers the fallback is that the transition does not animate at all - the theme switch will snap. This is a degraded-but-functional experience. Consider adding a comment in the CSS explaining the browser requirement so future maintainers understand why the approach was chosen.
2. Expand All still causes a synchronous DOM storm
The ensureDetailRendered pattern defers work nicely for individual opens, but Expand All still renders every detail panel synchronously in a single click handler. For a report with 5,000+ tests this will block the main thread for a noticeable amount of time. A more resilient approach would be to chunk the work using requestIdleCallback or requestAnimationFrame batching. That said, Expand All is an intentionally heavy operation and this is a reasonable trade-off for now - the content-visibility: auto on .grp-body-inner will also help render performance after the DOM is built.
3. The @property initial values are dark-theme defaults
The initial-value declarations all reflect dark theme values (e.g. --bg { initial-value:#0b0d11 }). If a user has light theme saved in localStorage, the browser may briefly flash the dark values before the JS sets data-theme="light" on page load. This is a pre-existing concern with the report's theme initialisation, not introduced by this PR - but worth noting if you ever improve the FOUC handling.
Note on the Preact suggestion
The vanilla JS approach keeps the report self-contained (no CDN dependencies, works offline, no CSP concerns). Given the scope of the interactivity, I would lean toward keeping it vanilla unless the complexity grows significantly. The lazy-rendering pattern introduced here proves vanilla can handle the perf concerns without a VDOM.
Verdict
The changes are focused, correct, and solve a real problem. The lazy-rendering architecture in particular is a solid improvement. The CSS @property approach is clever and well-commented. Happy with the direction - minor concerns noted above are not blockers.
|
Ty! |
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.18.21 to 1.18.37. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.18.37 <!-- Release notes generated using configuration in .github/release.yml at v1.18.37 --> ## What's Changed ### Other Changes * Improve HTML report perf by @slang25 in thomhurst/TUnit#5077 * perf: remove `Action` allocation for `token.Register` by @TimothyMakkison in thomhurst/TUnit#5075 * fix(mocks): escape C# keyword parameter names in generated mock code by @thomhurst in thomhurst/TUnit#5091 * Support static abstract interface members in mock generation by @thomhurst in thomhurst/TUnit#5070 ### Dependencies * chore(deps): update tunit to 1.18.21 by @thomhurst in thomhurst/TUnit#5076 * chore(deps): update dependency polyfill to 9.16.0 by @thomhurst in thomhurst/TUnit#5080 * chore(deps): update dependency polyfill to 9.16.0 by @thomhurst in thomhurst/TUnit#5079 * chore(deps): update dependency polyfill to 9.17.0 by @thomhurst in thomhurst/TUnit#5082 * chore(deps): update dependency polyfill to 9.17.0 by @thomhurst in thomhurst/TUnit#5081 * chore(deps): update dependency polly to 8.6.6 by @thomhurst in thomhurst/TUnit#5084 * chore(deps): update dependency humanizer to 3.0.8 by @thomhurst in thomhurst/TUnit#5087 * chore(deps): update dependency polyfill to 9.18.0 by @thomhurst in thomhurst/TUnit#5090 * chore(deps): update dependency polyfill to 9.18.0 by @thomhurst in thomhurst/TUnit#5089 **Full Changelog**: thomhurst/TUnit@v1.18.21...v1.18.37 Commits viewable in [compare view](thomhurst/TUnit@v1.18.21...v1.18.37). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Description
This change is to address the stutteriness of the html report when toggling between dark and light mode. The biggest win is by deferring the test details to keep the dom elements/depth to a minimum.
That said, maybe we should embed a lightweight library for this kind of thing, like preact 🤔 Interested to hear your views.