Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR updates docs to require a 24+ char remote token for non-loopback binds, standardizes Playwright CI test commands (port 3016 for CI-style runs), refactors and hardens the README screenshot script to use authenticated seeding and chart-readiness detection, adds chart-rendering helpers, and adds unit and e2e tests for docs, auth, seeding, and chart readiness. ChangesDocumentation, Authentication & Screenshot Infrastructure
Sequence Diagram(s)sequenceDiagram
participant Script as Screenshot Script
participant Server as Test Server
participant API as /api/usage
participant Browser as Browser
Script->>Server: start server with runtime root & local auth token
Server-->>Script: exposes bootstrap URL & accepts auth token
Script->>API: waitForServer(/api/usage) with Authorization header
API-->>Script: returns OK (authSession reachable)
Script->>Server: seedSampleUsageFile (writes data.json/settings.json)
Server-->>Script: seed success
Script->>Browser: launch & navigate to authSession.bootstrapUrl
Browser->>Server: load dashboard (renders charts)
Script->>Browser: waitForRenderedChartData -> countRenderedChartDataShapes polling
Browser-->>Script: chart shapes >= minShapes
Script->>Browser: capture screenshot
Script->>Server: stopServer (SIGTERM, escalate to SIGKILL if needed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/capture-readme-screenshots.js (1)
146-185:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake browser and child-process teardown exception-safe.
If anything throws after
chromium.launch(), neither the browser nor the context is guaranteed to close. Also, the currentserver.once('close')wait can hang if the child has already emittedclosebefore thefinallyblock runs. Move browser cleanup into a nestedfinally, and only awaitclosewhen the child is still alive.♻️ Cleanup shape to avoid leaked Chromium processes and stuck shutdowns
- try { + let browser; + let context; + try { const authSession = await waitForServer(baseUrl); await seedSampleUsage({ authSession }); - const browser = await chromium.launch(); - const context = await browser.newContext({ + browser = await chromium.launch(); + context = await browser.newContext({ viewport: { width: 1600, height: 1400 }, colorScheme: 'dark', reducedMotion: 'reduce', }); - const page = await context.newPage(); + const page = await context.newPage(); // ... - - await context.close(); - await browser.close(); } finally { - server.kill('SIGTERM'); - await new Promise((resolve) => server.once('close', resolve)); + await context?.close().catch(() => {}); + await browser?.close().catch(() => {}); + + if (server.exitCode === null && server.signalCode === null) { + const closed = new Promise((resolve) => server.once('close', resolve)); + server.kill('SIGTERM'); + await closed; + } }🤖 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 `@scripts/capture-readme-screenshots.js` around lines 146 - 185, The try/finally currently can leak Chromium if an exception occurs before context/browser are closed and can hang awaiting server.once('close') if the child already closed; refactor so that after calling chromium.launch() you have a nested try/finally that always attempts context.close() and browser.close() (guarding for undefined browser/context) in its finally block, and in the outer finally only call server.kill('SIGTERM') and await server.once('close') conditionally (e.g., check server.killed or server.exitCode / an isAlive flag) so you only await close when the child is still running; update code references around chromium.launch(), context.close(), browser.close(), server.kill('SIGTERM') and server.once('close') accordingly.
🤖 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 `@scripts/capture-readme-screenshots.js`:
- Around line 66-79: The polling loop in waitForServer can exceed the intended
timeout because fetchImpl and sleepImpl may run past the remaining budget;
compute remainingTimeoutMs at the top of each iteration (using startedAt and
timeoutMs) and if <= 0 immediately throw, create an AbortController and pass its
signal to fetchImpl (and to any internal fetch timeout), start a timer to abort
the controller when remainingTimeoutMs elapses so in-flight fetches are
cancelled, and cap the sleepImpl duration to Math.min(pollMs,
remainingTimeoutMs) so you never sleep past the budget; use the existing symbols
readAuthStatusImpl, fetchImpl, createAuthHeaders, sleepImpl, authStatusFile and
the waitForServer loop to locate the changes.
In `@SECURITY.md`:
- Around line 38-40: Update the curl example in SECURITY.md to use a concrete
reachable host instead of the bind address 0.0.0.0; replace the request URL
shown in the snippet (the curl command that currently targets
http://0.0.0.0:3000/api/usage) with a concrete host such as
http://127.0.0.1:3000/api/usage for local examples or indicate using the
server's LAN/public IP for remote access so readers know to target a reachable
address.
In `@tests/unit/documentation-contract.test.ts`:
- Around line 22-25: The SECURITY.md contract test is missing an assertion for
the X-TTDash-Remote-Token header, so update the test in
tests/unit/documentation-contract.test.ts to also assert that
docs['SECURITY.md'] contains 'X-TTDash-Remote-Token' (similar to the existing
README.md checks); locate the test that references docs['README.md'] and
docs['SECURITY.md'] and add the additional expect(...) assertion for the
SECURITY.md content to ensure the header isn't accidentally removed.
---
Outside diff comments:
In `@scripts/capture-readme-screenshots.js`:
- Around line 146-185: The try/finally currently can leak Chromium if an
exception occurs before context/browser are closed and can hang awaiting
server.once('close') if the child already closed; refactor so that after calling
chromium.launch() you have a nested try/finally that always attempts
context.close() and browser.close() (guarding for undefined browser/context) in
its finally block, and in the outer finally only call server.kill('SIGTERM') and
await server.once('close') conditionally (e.g., check server.killed or
server.exitCode / an isAlive flag) so you only await close when the child is
still running; update code references around chromium.launch(), context.close(),
browser.close(), server.kill('SIGTERM') and server.once('close') accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e15cf00f-236b-43da-a412-97583917fab3
⛔ Files ignored due to path filters (3)
docs/ttdash-dashboard-analytics.pngis excluded by!**/*.pngdocs/ttdash-dashboard-settings.pngis excluded by!**/*.pngdocs/ttdash-dashboard.pngis excluded by!**/*.png
📒 Files selected for processing (8)
CONTRIBUTING.mdREADME.mdSECURITY.mdscripts/capture-readme-screenshots.jsscripts/rendered-chart-data.jstests/e2e/dashboard-load-upload.spec.tstests/unit/documentation-contract.test.tstests/unit/readme-screenshots-script.test.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/dashboard-load-upload.spec.ts (1)
62-74:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScope the shape-count assertion to the two charts this test names.
countRenderedChartDataShapes(costAnalysisSection)will still pass if either “Cumulative cost per provider” or “Cost by model over time” stops rendering while any other chart inside#chartsstill draws enough SVG paths. Please assert rendered data under each chart container instead of the whole section.🤖 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 `@tests/e2e/dashboard-load-upload.spec.ts` around lines 62 - 74, The assertion currently calls countRenderedChartDataShapes(costAnalysisSection) for the whole `#charts` section which can hide a failure of either "Cumulative cost per provider" or "Cost by model over time"; instead locate each chart container via costAnalysisSection.getByText(/Cumulative cost per provider|Kumulative Kosten pro Anbieter/) and costAnalysisSection.getByText(/Cost by model over time|Kosten nach Modell im Zeitverlauf/) (or their parent/chart wrapper), then call countRenderedChartDataShapes() on each individual chart container and replace the single poll assertion with two polls asserting each chart's shape count is > 0 (using the same expect.poll(...).toBeGreaterThan(...) pattern).
🤖 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 `@scripts/capture-readme-screenshots.js`:
- Around line 221-235: The stopServer helper can hang waiting for 'close' if the
child ignores SIGTERM; update stopServer to add a shutdown deadline by wrapping
the existing promise with a timeout (use setTimeout) that will resolve if the
child hasn't closed within N ms, and on timeout attempt a stronger termination
(e.g., server.kill('SIGKILL')) and cleanup the 'close' listener and timeout.
Reference stopServer, isChildProcessRunning, server.kill, and the
server.once('close', ...) listener when making the changes so the promise always
resolves on success, error, or deadline and no listeners/timeouts are leaked.
In `@SECURITY.md`:
- Around line 38-43: Update the guidance that currently suggests substituting a
“public host name” for 127.0.0.1 when using the TTDASH_ALLOW_REMOTE,
TTDASH_REMOTE_TOKEN, HOST and ttdash example and the curl Authorization call:
explicitly restrict the example to LAN/VPN/SSH-tunneled access or require an
HTTPS reverse proxy/valid TLS termination before exposing the bearer token on
any public hostname, and mirror the same wording change in README.md to keep
both docs consistent.
In `@tests/unit/documentation-contract.test.ts`:
- Around line 14-20: The test's regression checks
(remoteBindWithoutTokenPattern, the expect on URL, and the TTDASH_REMOTE_TOKEN
assertion) are too narrow and miss permutations like different env-var order or
different ports; update the pattern and assertions to (1) use a regex that
detects TTDASH_ALLOW_REMOTE=1 and HOST=0.0.0.0 in any order without an
intervening TTDASH_REMOTE_TOKEN (e.g. look for both keys with a negative
lookahead for the token), (2) replace the exact URL check with a regex matching
http://0\.0\.0\.0(:\d+)?/api/usage so any port is caught, and (3) keep/assert
the TTDASH_REMOTE_TOKEN placeholder string
(TTDASH_REMOTE_TOKEN=<long-random-token>) still exists in docs entries; apply
these changes where the test iterates over docs and references
remoteBindWithoutTokenPattern, the URL expect, and the token expect.
In `@tests/unit/readme-screenshots-script.test.ts`:
- Around line 77-95: This test file mixes coverage for the chart-render helper
with the README screenshot script; extract all tests that exercise
renderedChartDataSelector and waitForRenderedChartData (the
require('../../scripts/rendered-chart-data.js') usage and any assertions around
those helpers) into a new focused unit test file (e.g.
tests/unit/rendered-chart-data.test.ts), update imports to
require('../../scripts/rendered-chart-data.js') and keep the existing typed
shape, and remove those chart-helper tests (lines exercising the helper, roughly
the block covering the helper behavior referenced in the review) from
tests/unit/readme-screenshots-script.test.ts so the README screenshot test only
covers capture-readme-screenshots.js.
---
Outside diff comments:
In `@tests/e2e/dashboard-load-upload.spec.ts`:
- Around line 62-74: The assertion currently calls
countRenderedChartDataShapes(costAnalysisSection) for the whole `#charts` section
which can hide a failure of either "Cumulative cost per provider" or "Cost by
model over time"; instead locate each chart container via
costAnalysisSection.getByText(/Cumulative cost per provider|Kumulative Kosten
pro Anbieter/) and costAnalysisSection.getByText(/Cost by model over time|Kosten
nach Modell im Zeitverlauf/) (or their parent/chart wrapper), then call
countRenderedChartDataShapes() on each individual chart container and replace
the single poll assertion with two polls asserting each chart's shape count is >
0 (using the same expect.poll(...).toBeGreaterThan(...) pattern).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 397b09ca-7032-4fb9-a208-091489866716
⛔ Files ignored due to path filters (1)
docs/ttdash-dashboard.pngis excluded by!**/*.png
📒 Files selected for processing (7)
README.mdSECURITY.mdscripts/capture-readme-screenshots.jsscripts/rendered-chart-data.jstests/e2e/dashboard-load-upload.spec.tstests/unit/documentation-contract.test.tstests/unit/readme-screenshots-script.test.ts
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.
Summary by CodeRabbit