Open
Conversation
Benchmark ResultsHyperfine
Criterion |
Benchmark ResultsHyperfine
Criterion |
k88hudson-cfa
requested changes
Feb 12, 2026
Collaborator
k88hudson-cfa
left a comment
k88hudson-cfa
left a comment
There was a problem hiding this comment.
There are some security issues here I'm concerned about, if this is the approach you're taking it's critical to maintain principle of least privilege on giving the action permissions. In terms of the artifacts, you could consider this: https://docs.github.com/en/actions/tutorials/store-and-share-data
Benchmark ResultsHyperfine
Criterion |
Benchmark ResultsHyperfine
Criterion |
k88hudson-cfa
approved these changes
Feb 16, 2026
Collaborator
k88hudson-cfa
left a comment
k88hudson-cfa
left a comment
There was a problem hiding this comment.
R+ with a follow-up: I didn't do thorough comments on the front-end code, but please fix the innerHTML with the unescaped html (in general be careful about using innerHTML)
Some other minor notes that are optional but recommended
| - name: Create JSON results | ||
| if: ${{ github.event.pull_request.base.ref == 'main' }} | ||
| env: |
| } | ||
|
|
||
| const url = source?.url ?? DEFAULT_HISTORY_URL; | ||
| $("meta").innerHTML = `Source: <a href="${url}">${url}</a> · Runs: ${runs.length} · Latest: ${lastTxt}`; |
Collaborator
There was a problem hiding this comment.
This is XSS vulnerable, can you document.createElement("a") and set textContent instead?
| }); | ||
| } else { | ||
| const allWrap = $("tsAllWrap"); | ||
| allWrap.innerHTML = ""; |
Collaborator
There was a problem hiding this comment.
These are OK security wise but you can also use replaceChildren
| } | ||
|
|
||
| function main() { | ||
| const argv = process.argv.slice(2); |
Collaborator
There was a problem hiding this comment.
Modern node has https://nodejs.org/api/util.html#utilparseargsconfig
import {parseArgs} from "node:util";
const { values } = parseArgs({
options: {
repo: { type: "string" },
branch: { type: "string" },
help: { type: "boolean", short: "h" },
},
strict: false,
});| All args are optional except input files; missing inputs produce empty result arrays. | ||
| */ | ||
|
|
||
| 'use strict'; |
Collaborator
There was a problem hiding this comment.
For node 20+ you should use esms
import fs from "node:fs";
import path from "node:path";
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
2 participants
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.
No description provided.