docs: assets generation automation - #12
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 24 minutes and 5 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces a test-bridge package to share Playwright test infrastructure and builds a comprehensive documentation asset generator (assets-cli). E2E tests migrate to the new package, the app's bridge is enhanced to seed CV content via JSON, and CI is optimized to exclude asset CLI dependencies from standard builds. ChangesTest Bridge & Assets CLI Implementation
E2E Test Migration & App Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
AxelUser
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
assets-cli/src/recipes/editor.ts (1)
106-111: ⚖️ Poor tradeoffGIF conversion in
finallymasks failures and runs on the unhappy path.If the recorded flow throws before
rec.stop()(e.g. a selector miss or timeout), thisfinallystill callsvideoToGifwith a window that was never closed, which both emits a misleading/partial GIF and can throw, masking the original error. The samefinallyblock is duplicated indashboard-search.ts,export.ts, andtailoring.ts; consider extracting a sharedwithGifRecording(ctx, fn)helper that only converts on success and re-raises the underlying error otherwise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets-cli/src/recipes/editor.ts` around lines 106 - 111, The finally block in editor.ts calls videoToGif unconditionally which can run when rec.stop() never completed and thus mask the original error; extract a shared helper like withGifRecording(ctx, fn) that encapsulates starting/stopping the recorder and converting the video (use page.video(), rec.stop(), context.close(), and videoToGif), ensure the helper only invokes videoToGif when the recording completed successfully, propagate/re-throw the original error if fn throws (and catch conversion errors separately so they don't overwrite the original), and replace the duplicated finally logic in editor.ts, dashboard-search.ts, export.ts, and tailoring.ts with calls to this helper.assets-cli/src/recipes/tailoring.ts (1)
72-72: 💤 Low valueBrittle decision-card locator.
.rounded-lg.border.p-4couples the recipe to Tailwind utility classes; a styling tweak will silently break the camera framing. Adata-testidon the decision card would be more durable if one can be added to the app.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets-cli/src/recipes/tailoring.ts` at line 72, The locator used to find decision cards (the expression assigning items via page.getByRole('dialog').locator('.rounded-lg.border.p-4')) is brittle because it relies on Tailwind classes; change the selector to a durable attribute-based selector (e.g., a data-testid like data-testid="decision-card") or another semantic selector (role/text) if data attributes cannot be added, update the locator call to use that selector, and coordinate with the app to add the data-testid to the decision card component so the test targets page.getByRole('dialog').locator('[data-testid="decision-card"]') (or equivalent semantic locator) instead of the Tailwind classes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@assets-cli/src/cli.ts`:
- Around line 48-51: The preview shutdown is not guaranteed because
killTree()/stop() returns immediately after sending SIGTERM and the inherited
stdio child can keep the process alive; update the implementation of killTree
(and server.stop which calls it) to wait for the child process to actually exit
(e.g., attach an exit handler or use childProcess.wait/Promise) and if it does
not exit within a short timeout escalate to SIGKILL and then resolve; ensure
main()/generateAll still closes resources (browser.close(), server.stop()) and
rely on process.exitCode instead of forcing process.exit so the CLI can
terminate cleanly once the preview child has truly exited.
- Around line 34-37: The CLI currently allows --help because CONFIG_OPTIONS
includes help, so ensure the help branch in the main entry (the if (values.help
|| command === 'help') check in assets-cli/src/cli.ts) remains and simply prints
HELP and exits early; no further action needed there. To tighten the hang risk,
update assets-cli/src/server.ts: in the non-Windows killTree() path have it send
SIGTERM then wait for child process 'exit' (use a Promise with a reasonable
timeout) and if the process hasn't exited within that timeout send SIGKILL and
await final exit before resolving stop(); also ensure assets-cli/src/runner.ts's
finally still calls server.stop() so the improved stop() behavior prevents
lingering processes.
In `@assets-cli/src/helpers/gif.ts`:
- Line 71: The code uses a non-null assertion on ffmpegPath (const bin =
ffmpegPath!) before calling execFile which can be null; update the logic in
assets-cli/src/helpers/gif.ts to check ffmpegPath for null/undefined before
assigning to bin and before calling execFile, and handle the missing binary by
throwing a clear Error or returning/rejecting early with a helpful message
(e.g., "ffmpeg binary not found"), so references such as ffmpegPath, the const
bin, and the execFile call are guarded and won't receive a null path at runtime.
- Around line 15-20: TrimWindow is duplicated; update gif.ts to use the
TrimWindow type exported from recorder.ts instead of redefining it: remove the
local TrimWindow interface in assets-cli/src/helpers/gif.ts and import the
TrimWindow type from the recorder module that defines rec.window(); ensure
videoToGif and any uses in gif.ts reference the imported TrimWindow so the
recorder.ts definition is the single source of truth.
In `@assets-cli/src/recipes/export.ts`:
- Around line 42-45: The download waiter is registered after hoverAndClick
triggers the click, which can miss the download event; change the sequence in
the export flow so you create the download promise with
page.waitForEvent('download') before calling hoverAndClick/export button click
(the code that eventually calls locator.click()), then trigger
hoverAndClick(exportBtn) and await the previously-created download promise;
adjust references in this function to use the prepared download promise rather
than calling waitForEvent after the click.
In `@assets-cli/src/runner.ts`:
- Around line 19-51: The preview server started by startPreview is leaked if
chromium.launch() throws because both are called before the try; move the
chromium.launch() call into the try block (so browser is created after entering
try) and ensure the finally block safely closes the browser only if it exists
(use a guard around browser.close(), e.g., browser?.close()) before always
calling server.stop(); update references in runner.ts to use startPreview,
chromium.launch, browser.close, and server.stop accordingly.
In `@assets-cli/src/server.ts`:
- Around line 51-79: The POSIX teardown can orphan the actual vite process
because killTree() only sends SIGTERM to the direct pnpm child and resolves
immediately; fix by spawning the preview in startPreview with a detached process
group (set detached: true on the spawn call for 'pnpm preview') and update
killTree(child) to kill the whole group on non-Windows by calling
process.kill(-child.pid, 'SIGTERM') (or SIGKILL as fallback), and wait for the
child's 'exit' event before resolving; keep Windows behavior using taskkill
as-is.
In `@test-bridge/src/index.ts`:
- Line 76: The call to page.waitForFunction currently passes the options object
as the second positional argument, so the timeout is treated as the function arg
and ignored; update the call to pass undefined as the second argument so the
options (e.g., { timeout: 5000 }) become the third argument and the timeout is
applied when waiting for (window as any).__hhTest inside page.waitForFunction.
---
Nitpick comments:
In `@assets-cli/src/recipes/editor.ts`:
- Around line 106-111: The finally block in editor.ts calls videoToGif
unconditionally which can run when rec.stop() never completed and thus mask the
original error; extract a shared helper like withGifRecording(ctx, fn) that
encapsulates starting/stopping the recorder and converting the video (use
page.video(), rec.stop(), context.close(), and videoToGif), ensure the helper
only invokes videoToGif when the recording completed successfully,
propagate/re-throw the original error if fn throws (and catch conversion errors
separately so they don't overwrite the original), and replace the duplicated
finally logic in editor.ts, dashboard-search.ts, export.ts, and tailoring.ts
with calls to this helper.
In `@assets-cli/src/recipes/tailoring.ts`:
- Line 72: The locator used to find decision cards (the expression assigning
items via page.getByRole('dialog').locator('.rounded-lg.border.p-4')) is brittle
because it relies on Tailwind classes; change the selector to a durable
attribute-based selector (e.g., a data-testid like data-testid="decision-card")
or another semantic selector (role/text) if data attributes cannot be added,
update the locator call to use that selector, and coordinate with the app to add
the data-testid to the decision card component so the test targets
page.getByRole('dialog').locator('[data-testid="decision-card"]') (or equivalent
semantic locator) instead of the Tailwind classes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c8ed354-06e4-4ea1-a6f6-af5ee7283c72
⛔ Files ignored due to path filters (14)
docs/assets/dashboard-search.dark.gifis excluded by!**/*.gifdocs/assets/dashboard-search.light.gifis excluded by!**/*.gifdocs/assets/dashboard.dark.pngis excluded by!**/*.pngdocs/assets/dashboard.light.pngis excluded by!**/*.pngdocs/assets/editor.dark.gifis excluded by!**/*.gifdocs/assets/editor.dark.pngis excluded by!**/*.pngdocs/assets/editor.light.gifis excluded by!**/*.gifdocs/assets/editor.light.pngis excluded by!**/*.pngdocs/assets/editor.pngis excluded by!**/*.pngdocs/assets/export.dark.gifis excluded by!**/*.gifdocs/assets/export.light.gifis excluded by!**/*.gifdocs/assets/tailoring.dark.gifis excluded by!**/*.gifdocs/assets/tailoring.light.gifis excluded by!**/*.gifpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (37)
.github/workflows/ci.yml.gitignoreREADME.mdassets-cli/README.mdassets-cli/bin/docs-assets.mjsassets-cli/package.jsonassets-cli/src/cli.tsassets-cli/src/config.tsassets-cli/src/helpers/context.tsassets-cli/src/helpers/dwight.tsassets-cli/src/helpers/gif.tsassets-cli/src/helpers/mouse.tsassets-cli/src/helpers/recorder.tsassets-cli/src/helpers/zoom.tsassets-cli/src/paths.tsassets-cli/src/recipe.tsassets-cli/src/recipes/dashboard-search.tsassets-cli/src/recipes/dashboard.tsassets-cli/src/recipes/editor.tsassets-cli/src/recipes/export.tsassets-cli/src/recipes/tailoring.tsassets-cli/src/registry.tsassets-cli/src/runner.tsassets-cli/src/server.tsassets-cli/tsconfig.jsondocs/decisions/ADR-004-e2e-test-strategy.mde2e/dashboard.spec.tse2e/editor.spec.tse2e/export.spec.tse2e/helpers/bridge.tse2e/tailoring.spec.tspackage.jsonpnpm-workspace.yamlsrc/lib/e2e-bridge.tstest-bridge/package.jsontest-bridge/src/index.tstest-bridge/tsconfig.json
💤 Files with no reviewable changes (1)
- e2e/helpers/bridge.ts
Comment on lines
+48
to
+51
| main().catch((err) => { | ||
| process.stderr.write(`${err instanceof Error ? (err.stack ?? err.message) : String(err)}\n`); | ||
| process.exitCode = 1; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect generateAll cleanup on error paths.
fd -t f 'runner.ts' assets-cli | xargs -r catRepository: AxelUser/HumbleHire
Length of output: 1949
🏁 Script executed:
#!/bin/bash
# Inspect CLI and preview server stop/teardown.
fd -t f 'cli.ts' assets-cli | xargs -r cat
fd -t f 'server.ts' assets-cli | xargs -r catRepository: AxelUser/HumbleHire
Length of output: 4645
Finalize preview shutdown so exitCode is sufficient.
generateAll already tears down resources in finally (await browser.close(); await server.stop();). The remaining risk is server.stop() on non-Windows: killTree() sends SIGTERM and resolves immediately without waiting for the preview child to actually exit. Because the preview is spawned with stdio: 'inherit', the CLI process can stay alive on failure. Update killTree/stop() to wait for the child to exit (and escalate to a stronger kill if it doesn’t).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@assets-cli/src/cli.ts` around lines 48 - 51, The preview shutdown is not
guaranteed because killTree()/stop() returns immediately after sending SIGTERM
and the inherited stdio child can keep the process alive; update the
implementation of killTree (and server.stop which calls it) to wait for the
child process to actually exit (e.g., attach an exit handler or use
childProcess.wait/Promise) and if it does not exit within a short timeout
escalate to SIGKILL and then resolve; ensure main()/generateAll still closes
resources (browser.close(), server.stop()) and rely on process.exitCode instead
of forcing process.exit so the CLI can terminate cleanly once the preview child
has truly exited.
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.
Summary by CodeRabbit