Add randomized README demos - #46
Merged
Merged
Conversation
Deploying jst with
|
| Latest commit: |
02cb0e6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9f165d1c.jst-5mg.pages.dev |
| Branch Preview URL: | https://agent-add-readme-demos.jst-5mg.pages.dev |
|
The macOS CLI preview for this PR is ready. With GitHub CLI authenticated, copy this one command to download and run it: preview_dir="$(mktemp -d)" && gh run download 32019075924 --repo yoavf/jst --name jst-pr-preview-macos-universal --dir "$preview_dir" && chmod +x "$preview_dir/jst" && "$preview_dir/jst" --dry list files in the current directoryReplace the example arguments after |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Hide | ||
| Type `unset NO_COLOR; export COLUMNS=80; export PS1=$'\n❯ '` | ||
| Enter | ||
| Type "mkdir -p /tmp/jst-demo-zip/folder/node_modules /tmp/jst-demo-zip/folder/src; cd /tmp/jst-demo-zip; mkfile 1m folder/src/app.js; mkfile 2m folder/node_modules/bundle.js" |
There was a problem hiding this comment.
The tape creates its fixture exclusively with the macOS-specific mkfile utility, so contributors regenerating the GIF on other platforms receive mkfile: command not found and produce an empty or misleading archive demonstration. Use a portable fixture command or explicitly declare the recording environment.
PR Summary by QodoRandomize README demo GIF via scoped Cloudflare Worker redirect
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Code Review by Qodo
1. Undefined redirect target
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+8
to
+10
| export function demoFileAt(index) { | ||
| return DEMO_FILES[index % DEMO_FILES.length]; | ||
| } |
There was a problem hiding this comment.
1. Undefined redirect target 🐞 Bug ≡ Correctness
demoFileAt uses a raw modulo result as an array index, so negative/NaN/non-integer inputs (or a future empty DEMO_FILES) return undefined and demoRedirect will emit Location: https://jst.sh/demos/undefined. That produces broken redirects and makes the exported helpers unsafe for reuse beyond the current Math.floor(Math.random()) callsite.
Agent Prompt
### Issue description
`demoFileAt(index)` can return `undefined` for invalid indices (negative, NaN, non-integer) or if the list is ever emptied, and `demoRedirect` will then generate a broken `Location` header.
### Issue Context
These helpers are exported and already imported by unit tests, so they are part of a reusable API surface.
### Fix Focus Areas
- workers/demo-gif.js[1-20]
### Suggested change
- Guard against an empty `DEMO_FILES`.
- Coerce `index` to a finite integer and normalize negatives before indexing.
- Optionally throw (or default to 0) when `index` is invalid so bad calls fail loudly instead of redirecting to `/undefined`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+22
to
+25
| export default { | ||
| fetch() { | ||
| return demoRedirect(Math.floor(Math.random() * DEMO_FILES.length)); | ||
| }, |
There was a problem hiding this comment.
2. No method/path guarding 🐞 Bug ☼ Reliability
The Worker fetch handler ignores the incoming Request, so it returns the same 302 redirect for any HTTP method and for any path the Worker is routed to. This increases blast radius if the route is ever broadened/misconfigured, because unrelated requests would start redirecting to demo GIFs.
Agent Prompt
### Issue description
The Worker responds with a demo redirect unconditionally; it does not validate HTTP method or pathname.
### Issue Context
Even if deployment is intended to be scoped to `/demo.gif`, adding an in-code check is a cheap defense-in-depth measure against accidental route changes.
### Fix Focus Areas
- workers/demo-gif.js[22-26]
### Suggested change
- Accept `request` in `fetch(request)`.
- If `request.method` is not `GET`/`HEAD`, return `405`.
- If `new URL(request.url).pathname` is not `/demo.gif`, return `404` (or similar).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+3
| import assert from "node:assert/strict"; | ||
| import test from "node:test"; | ||
| import { demoFileAt, demoRedirect } from "../workers/demo-gif.js"; |
There was a problem hiding this comment.
3. Js tests not in ci 🐞 Bug ⚙ Maintainability
This PR adds a Node test file for the demo-gif Worker, but the GitHub Actions workflows shown do not run any Node/npm test command, so these new tests won’t execute in CI and regressions to the worker behavior can merge undetected.
Agent Prompt
### Issue description
The newly-added Node tests under `site/*.test.js` are not exercised by existing GitHub Actions workflows.
### Issue Context
`package.json` defines `test:demo` as `node --test site/*.test.js`, but `.github/workflows/ci.yml` only runs Rust checks.
### Fix Focus Areas
- site/demo-gif.test.js[1-18]
- package.json[5-9]
- .github/workflows/ci.yml[16-30]
### Suggested change
- Add a CI job/steps to install Node (actions/setup-node), run `npm ci`, then `npm run test:demo`.
- Optionally cache npm to keep CI fast.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
What changed
https://jst.sh/demo.gifat the top of the README.workers/demo-gif.js; it randomly chooses a GIF and responds with ano-storeredirect.Why
GitHub README content cannot run JavaScript. The scoped Cloudflare Worker route,
jst.sh/demo.gif, supplies the random selection while the normal site and the GIF assets remain static GitHub Pages content.Validation
npm run test:demo(33 passing)vhs validate docs/demos/*.tapegit diff --checkThe deployed worker is intentionally restricted to that exact route. GitHub's image proxy may cache a selected GIF between fresh fetches.
Greptile Summary
The PR adds four recorded terminal demos, embeds a randomized demo in the README, and introduces a small redirecting Cloudflare Worker with unit coverage.
docs/demos/.jst.sh/demo.gifembed to the README.Cache-Control: no-store.Confidence Score: 4/5
The PR appears safe to merge, with only a non-blocking portability issue in the zip demo’s regeneration source.
The randomized redirect and static asset mapping are internally consistent, while the zip demo tape’s undeclared dependency on macOS
mkfilemakes that checked-in artifact difficult to reproduce on other development environments.Files Needing Attention: docs/demos/zip-folder.tape
Important Files Changed
mkfile.Reviews (1): Last reviewed commit: "Remove faulty largest-files demo" | Re-trigger Greptile