logs.html → {js,html} split + cleanup + lit + tsc + biome - #9340
allisonkarlitskaya wants to merge 5 commits into
Conversation
|
Predicted feedback from @Venefilyn: use an import map and integrity :) |
|
@Venefilyn should we also give the PF CSS the integrity treatment? In both cases this means we'd need to pin to a specific version... |
Yes!
We don't need integrity treatment for CSS, but we'd need to pin the version |
85d990e to
bcc2d88
Compare
Venefilyn
left a comment
Venefilyn
left a comment
There was a problem hiding this comment.
Overall missing comments throughout.
I kinda like having raw logs available like we have in main atm, it is just a lot faster to scroll through etc. but seems fine here so sure lets go with it - means it's faster to switch between
Nitpicks about Lit in general. Performance wise it is noticeably slower, I don't know how Lit works in regards to re-rendering and performance optimizations or how Web Components handle performance when re-rendering. Maybe there is something we're missing?
I checked performance recording for main vs this PR and there is a lot more happening. When going from full list to smaller (only failures) it is going from 20ms to 50ms. Going from only failures to full list goes from 130ms to 190ms.
These test/verify/check-selinux TestSelinuxEnforcing.test run green 3 times, just retried as the test is touched. So looks weird to show that - we shouldn't show it IMO and keep it to how we had it before. See how we have it live atm
I'm missing the reason for the retries, they're missing completely
| render_filter_toggle(has_failed) { | ||
| return html` | ||
| <div> | ||
| <label class="pf-v6-c-switch"> | ||
| <input class="pf-v6-c-switch__input" type="checkbox" role="switch" | ||
| .checked=${has_failed ? this.show_only_failed : false} | ||
| @change=${(/** @type {Event} */ e) => { | ||
| this.show_only_failed = /** @type {HTMLInputElement} */ (e.target).checked; | ||
| }}> | ||
| <span class="pf-v6-c-switch__toggle"></span> | ||
| <span class="pf-v6-c-switch__label" aria-hidden="true">Show only failed</span> | ||
| </label> | ||
| </div>`; | ||
| } |
There was a problem hiding this comment.
This doesn't have animation when switching, maybe rerendered each time? Only happens on Firefox
Kooha-2026-08-14-16-23-37.webm
| If you need to test the streaming code itself, it's also possible to run CI | ||
| jobs locally using `job-runner run cockpit-project/starter-kit` or something | ||
| similar. In the default configuration, `job-runner` will write to the local | ||
| disk instead of uploading to S3, but the s3streamer algorithm is still used and | ||
| live updates should work in the same way. In that case you should run the | ||
| webserver like this: |
There was a problem hiding this comment.
I would like to make it possible to use logs from an existing run. That way you can compare with live code. I could do that with current main by just modifying log.html
There was a problem hiding this comment.
I tried to follow your instructions and it failed with a CORS message in the debugger...
|
|
||
| customElements.define("test-entry", TestEntry); | ||
|
|
||
| class LogViewer extends LitElement { |
There was a problem hiding this comment.
This is the main component but in the middle of the file, could we move to the bottom of the file
| static properties = { | ||
| href: { type: String }, | ||
| content: { state: true }, | ||
| raw: { state: true }, | ||
| show_only_failed: { state: true }, | ||
| }; |
bcc2d88 to
2c845aa
Compare
Move from importing lit via URL in the .js file to using an import map in JS. This lets us specify integrity information as part of the map, effectively pinning our lit version. Improve the type declarations: I had Claude go through the actual type declarations and copy over the parts relevant to our code. Add a licence header accordingly, since this is now a derived work of the original. Rename our lit.d.ts file and update it to `declare module "lit"` now that we call it that way from the import statement.
This speeds things up from ~1.3s to ~0.5s on no changes. It's minor, but it's a win.
This got added by Claude for some unknown reason: we always have biome.jsonc present, so we should run biome whenever it's installed, without further conditions.
Move lib/s3-html/log.html to lib/html/ alongside the dashboard and rewrite it substantially to use Web Components with Lit. This fixes some bugs: - due to internal component state, subsections no longer snap shut during updates - updates no longer occasionally delete chunks of the previous page content (owing to side-effects related to our use of .exec on regexps, which is stateful). - the TAP parsing in general (which has been incrementally extended over the last decade) has been rewritten in simplified form and actually possible to understand and work on now - all the logic is in a separate .js file now, with types, and under Biome, and All of this means that it should be much easier to make changes to the log page going forward.
2c845aa to
e962da7
Compare
This section was inaccurately named and offered advice that I couldn't get to work (because of the CORS settings on the Python webserver being unhappy about accesses to different hosts). Update the information more generally and provide some new ideas for testing.
|
Re-request review when my previous review is addressed and I'll take a look :) |
2 participants
This simplifies the code substantially, fixes several bugs, and makes this code a lot easier to work on going forward.