fix: fix mobile layout clipping for GitHub corner, Share Doom button, and grim reaper#67
Merged
Merged
Conversation
… and grim reaper Agent-Logs-Url: https://github.com/nitrocode/token-deathclock/sessions/012d6c7c-483f-47de-98b3-791d1efc2d47 Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughI pity the fool — this PR adds screenshot automation (new script and npm command), a Playwright mobile project, mobile-focused E2E assertions, and small CSS and .gitignore updates to support captured screenshots and prevent horizontal overflow. Changes
Sequence DiagramsequenceDiagram
actor User
participant NPM as "npm run screenshots"
participant Script as "scripts/screenshots.js"
participant Server as "Static Server\n(npx serve)"
participant Playwright as "Playwright / Chromium"
participant Desktop as "Desktop Context"
participant Mobile as "Mobile Context (Pixel 5)"
User->>NPM: invoke
NPM->>Script: start
Script->>Script: ensure screenshots/ exists
Script->>Script: resolve BASE_URL (SCREENSHOT_URL or default)
alt BASE_URL is default
Script->>Server: spawn (npx serve)
Script->>Script: wait 2s
end
Script->>Playwright: launch browser
Playwright->>Desktop: create desktop context
Desktop->>Playwright: goto BASE_URL (wait networkidle)
Desktop->>Script: save desktop.png
Playwright->>Mobile: create mobile context (Pixel 5)
Mobile->>Playwright: goto BASE_URL (wait networkidle)
Mobile->>Script: save mobile.png
Script->>Server: kill (if spawned)
Script->>NPM: exit (status)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
Copilot created this pull request from a session on behalf of
nitrocode
April 24, 2026 13:05
View session
Contributor
👁️ PR Preview
|
Agent-Logs-Url: https://github.com/nitrocode/token-deathclock/sessions/48247342-5af0-4491-985a-6a26eccb7b65 Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/screenshots.js (1)
48-97: Overall structure looks good, but watch that server cleanup!The main function logic is solid - spawning the server, capturing screenshots, and cleaning up. But I pity the fool who don't handle edge cases! If
serverProcess.kill()fails (e.g., process already exited), you might swallow an error silently. Consider a safety check.🛡️ Optional: safer server cleanup
} finally { if (serverProcess) { - serverProcess.kill(); + try { + serverProcess.kill(); + } catch { + // Server may have already exited — ignore + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/screenshots.js` around lines 48 - 97, In main(), the finally block calls serverProcess.kill() without guarding against the case where the child already exited or kill throws; wrap the shutdown in a safe check and error handler: verify serverProcess exists and is not already killed (use serverProcess.killed or serverProcess.exitCode), call serverProcess.kill() inside a try/catch, and log any failure instead of allowing an exception to propagate (reference symbols: main, serverProcess, spawn, and the finally block containing serverProcess.kill()).tests/e2e/death-clock.spec.js (1)
319-328: Unused variable, fool!I pity the fool who leaves dead code lying around! The
vpvariable on line 320 is declared but never used in this test - unlike the other tests where it's needed for viewport boundary checks.🧹 Proposed cleanup
test('grim reaper is not excessively cut off on the left on mobile', async ({ page }) => { - const vp = page.viewportSize(); const bbox = await page.locator('#grim-reaper').boundingBox(); expect(bbox).not.toBeNull();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/death-clock.spec.js` around lines 319 - 328, The variable vp declared in the test function "grim reaper is not excessively cut off on the left on mobile" is unused; remove the unused declaration const vp = page.viewportSize() (or if viewport info is intended use it in assertions) to eliminate dead code—edit the test in tests/e2e/death-clock.spec.js by deleting the vp variable declaration in that test or replacing its usage appropriately within the same test function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/screenshots.js`:
- Around line 87-89: The catch block in scripts/screenshots.js accesses
err.message even though TypeScript types the caught value as unknown; change the
catch handling in that try/catch so you narrow err first (e.g., use a type guard
like `if (err instanceof Error) { console.error('[screenshots] Error:',
err.message); } else { console.error('[screenshots] Error:', String(err)); }`)
and then set exitCode = 1; update the catch around the screenshot logic (the
`catch (err) { ... }` block) to perform this narrowing before accessing
.message.
- Around line 44-46: The failing typecheck is caused by the untyped parameter in
function waitMs; update the function signature or add a JSDoc annotation so
TypeScript knows ms is a number (e.g., add a JSDoc `@param` {number} ms above
async function waitMs or convert to async function waitMs(ms: number) if using
.ts), and ensure the Promise resolve callback matches that type to satisfy
strict mode.
---
Nitpick comments:
In `@scripts/screenshots.js`:
- Around line 48-97: In main(), the finally block calls serverProcess.kill()
without guarding against the case where the child already exited or kill throws;
wrap the shutdown in a safe check and error handler: verify serverProcess exists
and is not already killed (use serverProcess.killed or serverProcess.exitCode),
call serverProcess.kill() inside a try/catch, and log any failure instead of
allowing an exception to propagate (reference symbols: main, serverProcess,
spawn, and the finally block containing serverProcess.kill()).
In `@tests/e2e/death-clock.spec.js`:
- Around line 319-328: The variable vp declared in the test function "grim
reaper is not excessively cut off on the left on mobile" is unused; remove the
unused declaration const vp = page.viewportSize() (or if viewport info is
intended use it in assertions) to eliminate dead code—edit the test in
tests/e2e/death-clock.spec.js by deleting the vp variable declaration in that
test or replacing its usage appropriately within the same test function.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8dd48b00-ab9a-43d5-b306-0dc53e235341
📒 Files selected for processing (8)
.gitignorepackage.jsonplaywright.config.jsscripts/screenshots.jsstyles.cssstyles/base.cssstyles/scary-features.csstests/e2e/death-clock.spec.js
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 233 233
Branches 107 107
=========================================
Hits 233 233
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Owner
|
@copilot resolve the merge conflicts in this pull request |
…-issues # Conflicts: # styles.css # tests/e2e/death-clock.spec.js Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
Contributor
Author
Resolved in 1dc5a6a. Merged
All 253 unit tests and typecheck pass. |
This was referenced Apr 26, 2026
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
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.
npm run typecheck(tsc) failing onscripts/screenshots.jsTS7006: add@type {number}JSDoc annotation towaitMsparameterTS18046: narrowerrtoErrorbefore accessing.messagetsc --noEmitexits 0)Summary by CodeRabbit
New Features
Bug Fixes
Tests