fix: make tab bar horizontally scrollable on mobile#66
Conversation
Agent-Logs-Url: https://github.com/nitrocode/token-deathclock/sessions/5d0c22bc-ae00-4524-8a86-eec25691c0a8 Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
|
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 (2)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughListen here, fool! This PR makes some righteous changes to get that tab bar scrolling smooth on mobile viewports, adds some E2E tests to verify the behavior, trims down some build configuration, and updates the documentation. I pity the fool who don't appreciate mobile optimization, sucka! Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Listen here, fool! You got yourself multiple file changes across different domains—styling modifications that repeat across two files, new test logic with specific assertions, documentation tweaks, and build script adjustments. I pity the fool who tries to review this without paying attention to both the CSS consistency and the test verification details, sucka! The changes ain't overly complex, but they span enough variety to demand separate consideration for each cohort. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@copilot resolve the merge conflicts in this pull request |
Agent-Logs-Url: https://github.com/nitrocode/token-deathclock/sessions/bda51ada-db65-44dc-832e-7fcea0043e27 Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
Done — merged |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/death-clock.spec.js (1)
319-331: Add dashboard to the activation-flow loop.Line 320 currently skips clicking
dashboard, so the “each tab activates its panel” flow is only partially covered.♻️ Suggested test hardening
- test('each tab button is clickable and activates the correct panel', async ({ page }) => { - const tabs = [ - { btn: '#tab-btn-news', panel: '#tab-news' }, - { btn: '#tab-btn-about', panel: '#tab-about' }, - { btn: '#tab-btn-changelog', panel: '#tab-changelog' }, - ]; - for (const { btn, panel } of tabs) { - // Scroll the button into view inside the tab bar before clicking - await page.locator(btn).scrollIntoViewIfNeeded(); - await page.locator(btn).click(); - await expect(page.locator(panel)).not.toHaveAttribute('hidden', /.*/); - } - }); + test('each tab button is clickable and activates the correct panel', async ({ page }) => { + const tabs = ['dashboard', 'news', 'about', 'changelog']; + for (const tab of tabs) { + const btn = page.locator(`#tab-btn-${tab}`); + await btn.scrollIntoViewIfNeeded(); + await btn.click(); + const panelId = await btn.getAttribute('aria-controls'); + expect(panelId).toBeTruthy(); + await expect(page.locator(`#${panelId}`)).not.toHaveAttribute('hidden', /.*/); + } + });Based on learnings, "Applies to tests/e2e/death-clock.spec.js : E2E tests in tests/e2e/death-clock.spec.js must validate DOM behaviour and visual flows; add E2E assertions when modifying script.js or index.html".
🤖 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 - 331, The test's tabs array omits the dashboard tab so the activation loop in test('each tab button is clickable and activates the correct panel', ...) doesn't verify dashboard behavior; add an entry for the dashboard (e.g. { btn: '#tab-btn-dashboard', panel: '#tab-dashboard' }) to the tabs array used by the loop, keep the existing scrollIntoViewIfNeeded/click/assert sequence, and ensure the test continues to use the same btn and panel variables so the loop covers all tab activation flows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/death-clock.spec.js`:
- Around line 319-331: The test's tabs array omits the dashboard tab so the
activation loop in test('each tab button is clickable and activates the correct
panel', ...) doesn't verify dashboard behavior; add an entry for the dashboard
(e.g. { btn: '#tab-btn-dashboard', panel: '#tab-dashboard' }) to the tabs array
used by the loop, keep the existing scrollIntoViewIfNeeded/click/assert
sequence, and ensure the test continues to use the same btn and panel variables
so the loop covers all tab activation flows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 514518f4-a84a-49e4-a91c-197f820c207a
📒 Files selected for processing (3)
styles.cssstyles/hero-tabs.csstests/e2e/death-clock.spec.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/build-js.js (1)
80-90: Yo fool, this esbuild implementation is solid!I pity the fool who doesn't appreciate clean minification code! The
transformSyncusage is correct, sucka. But listen here — thatglobalName: undefinedon line 84 is redundant, fool!undefinedis already the default value. You can drop that line and the code still works the same.💪 Mr. T's clean-up suggestion
const result = esbuild.transformSync(unminified, { minify: true, - // Keep the IIFE wrapper intact; tell esbuild the code is already IIFE-wrapped. - globalName: undefined, // Preserve the leading banner comment so tools can still identify the file. banner: '/* AI DEATH CLOCK — browser/DOM layer (minified) */', // Target all modern browsers; no transpilation needed. target: ['es2018'], loader: 'js', });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build-js.js` around lines 80 - 90, The esbuild.transformSync call includes an unnecessary option assignment; remove the redundant globalName: undefined property from the options object passed to esbuild.transformSync (the code around the esbuild.transformSync(...) invocation and its option keys like minify, banner, target, loader) so the behavior remains identical while cleaning up the config.docs/prd/bundler.md (1)
94-94: Minor inconsistency with the version spec, fool!The PRD says
^0.28.0butpackage.jsonpins to exact0.28.0without the caret. This ain't a big deal since the PRD is a planning doc and the implementation is what matters, but you might wanna update this to match reality, sucka!✏️ Proposed alignment
-1. **`esbuild` devDependency** added (`^0.28.0`). +1. **`esbuild` devDependency** added (`0.28.0`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/prd/bundler.md` at line 94, Update the PRD entry that currently lists the esbuild devDependency as `^0.28.0` to match the actual implementation by changing it to `0.28.0` (or alternatively update package.json to `^0.28.0` if you prefer caret behavior); edit the PRD text where the esbuild devDependency is declared so the version spec in the document matches the package.json, ensuring consistency between the planning doc and implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/prd/bundler.md`:
- Around line 99-100: The PRD claim that the `build` script runs "all six build
steps" is wrong — the `package.json` `build` script only chains five tasks
(`build:milestones`, `build:changelog`, `build:project-stats`, `build:js`,
`build:css`), so update the text in bundler.md to say it runs five build steps
(or if a sixth task was intended, add the missing script to `package.json` and
include its name); reference the `build` script in package.json and the sentence
in docs/prd/bundler.md that currently says "all six build steps" and make the
wording consistent with the actual `build` chain.
---
Nitpick comments:
In `@docs/prd/bundler.md`:
- Line 94: Update the PRD entry that currently lists the esbuild devDependency
as `^0.28.0` to match the actual implementation by changing it to `0.28.0` (or
alternatively update package.json to `^0.28.0` if you prefer caret behavior);
edit the PRD text where the esbuild devDependency is declared so the version
spec in the document matches the package.json, ensuring consistency between the
planning doc and implementation.
In `@scripts/build-js.js`:
- Around line 80-90: The esbuild.transformSync call includes an unnecessary
option assignment; remove the redundant globalName: undefined property from the
options object passed to esbuild.transformSync (the code around the
esbuild.transformSync(...) invocation and its option keys like minify, banner,
target, loader) so the behavior remains identical while cleaning up the config.
🪄 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: 1e672a99-3eb8-4525-a465-a993d8bb8557
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.github/workflows/e2e-tests.yml.github/workflows/unit-tests.ymlAGENTS.mdCLAUDE.mddocs/prd/README.mddocs/prd/bundler.mdpackage.jsonrenovate.jsonscript.jsscripts/build-css.jsscripts/build-js.jsstyles.css
✅ Files skipped from review due to trivial changes (5)
- CLAUDE.md
- renovate.json
- docs/prd/README.md
- .github/workflows/unit-tests.yml
- .github/workflows/e2e-tests.yml
Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
…edundant globalName from build-js.js Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
👁️ PR Preview
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66 +/- ##
=========================================
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:
|
docs/prd/bundler.mdline 94:^0.28.0→0.28.0(match actual package.json)docs/prd/bundler.mdline 99: "six" → "five" build steps (matchesbuildscript in package.json)globalName: undefinedfromscripts/build-js.jsesbuild.transformSync callnpm run build:js— script.js rebuilt successfullynpm run test:ci— 253 unit tests pass, 100% coverageSummary by CodeRabbit
New Features
Bug Fixes
Tests