Codex/pagination e2e tests#435
Conversation
Adds the repo's first Playwright spec (tooling/e2e/pagination.spec.ts), locking in the user-visible behavior from the 2026-05-23 dogfood pass that the margin-aware-packing fix resolved but that has no automated coverage: - ISSUE-001: every break sits on/above the true 931px A4 boundary (no downward drift); per-page gaps stay ~one page tall and don't accumulate. - ISSUE-002: advisory break lines actually render on load. - ISSUE-003: a "Page 1 of N" marker plus "Page K of N" labels with a real total, numbered 2..N in document order. - ISSUE-004: at a 600px viewport every label box stays within [0, 600] (left-gutter placement), so page numbers never scroll off-screen. Plus two hand-verified invariants: the overlay keeps pointer-events:none (native editing untouched) and the load+resize recompute logs no console errors. Lives in tooling/e2e/ (pnpm e2e), outside the bun fast/slow lanes. Verified 6/6 on Chromium against apps/www /dev/pagination2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n notes - dogfood-output/report.md: add Resolution blocks (ISSUE-001/003/004 fixed, 002 improved) referencing fix e7be784 + the new e2e coverage; report had captured only the pre-fix state. - include dogfood screenshots/videos the report references. - carry forward untracked docs/plans/2026-05-22-pagination-*.md (in-flight planning notes) so they are not orphaned. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive documentation and implementation plans for a new pagination feature, covering research, architecture, and specific strategies like Approach B and Two-Mode. It also includes E2E tests for the pagination overlay. Feedback highlights the need to address a placeholder file in the documentation and suggests using the Playwright Locator type in the E2E tests to enhance type safety and code clarity.
| @@ -0,0 +1 @@ | |||
| PLACEHOLDER | |||
There was a problem hiding this comment.
This file is currently a placeholder. It should either be completed with the intended unified implementation plan or removed from the pull request to maintain documentation quality.
| @@ -0,0 +1,157 @@ | |||
| import { expect, test } from '@playwright/test'; | |||
There was a problem hiding this comment.
Importing the Locator type from @playwright/test allows for better type safety and cleaner function signatures in helper methods.
| import { expect, test } from '@playwright/test'; | |
| import { type Locator, expect, test } from '@playwright/test'; |
| const topOf = (handle: { | ||
| evaluate: <R>(fn: (el: SVGElement | HTMLElement) => R) => Promise<R>; | ||
| }) => handle.evaluate((el) => Number.parseFloat((el as HTMLElement).style.top)); |
There was a problem hiding this comment.
The topOf helper can be simplified by using the Locator type. This avoids the need for a manual structural type definition and makes the code more idiomatic for Playwright tests.
const topOf = (handle: Locator) =>
handle.evaluate((el) => Number.parseFloat((el as HTMLElement).style.top));![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/plans/2026-05-22-pagination-unified-plan.md (1)
1-1: ⚡ Quick winAdd context to the placeholder or remove the file until ready.
This file contains only "PLACEHOLDER" with no explanation. Readers won't understand what this placeholder represents, when it will be filled, or what decision it consolidates. Either add explanatory content describing this as the intended consolidation point for selecting/combining Plan B and Plan T approaches after design review, or remove this file until the unified decision is ready to document.
📝 Suggested placeholder content
-PLACEHOLDER +# Pagination — Unified Plan (Decision Pending) + +**Status:** Placeholder for unified decision after Plan B and Plan T evaluation. + +This document will consolidate the final pagination approach after: +1. Design review of Plan B (line-projection with page boxes) and Plan T (two-mode: continuous edit + print) +2. Initial implementation feedback from Phase 0 foundation PRs (F1-F5) +3. Evaluation of shared foundation performance and feasibility + +**Related documents:** +- `2026-05-22-pagination-research.md` — foundational research and alternatives +- `2026-05-22-pagination-B-plan.md` — Plan B specification +- `2026-05-22-pagination-B-line-projection.md` — Plan B implementation details +- `2026-05-22-pagination-twomode-plan.md` — Plan T specification + +**Next steps:** Document the selected approach (B, T, or hybrid) here after Phase 0 implementation and model review.🤖 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 `@docs/plans/2026-05-22-pagination-unified-plan.md` at line 1, The file currently contains only the token "PLACEHOLDER"; replace that token with a short explanatory paragraph that states this doc is the intended consolidation point for choosing/combining the Plan B and Plan T approaches after design review, including expected next steps, ownership, and a tentative timeline (or alternatively delete the file until the unified decision is ready to document); look for the "PLACEHOLDER" string to locate where to edit.tooling/e2e/pagination.spec.ts (1)
28-31: 💤 Low valueConsider inlining the
CONTAINERconstant.The
CONTAINERconstant is only used once (line 130). As per coding guidelines for TypeScript files, prefer inline selectors when used once; extract constants only when reused.♻️ Proposed refactor
const BREAK_LINE = '[data-slot="pagination-break-line"]'; const PAGE_MARKER = '[data-slot="pagination-page-marker"]'; const LABEL = '[data-slot="pagination-break-label"]'; -const CONTAINER = '[data-slot="pagination-break-lines"]';Then inline it at the usage site:
const pointerEvents = await page - .locator(CONTAINER) + .locator('[data-slot="pagination-break-lines"]') .evaluate((el) => getComputedStyle(el).pointerEvents);🤖 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 `@tooling/e2e/pagination.spec.ts` around lines 28 - 31, CONTAINER is declared but only used once; remove the CONTAINER constant and replace its single usage with the literal selector string '[data-slot="pagination-break-lines"]' so the selector is inlined in pagination.spec.ts (locate the usage where CONTAINER is referenced in the pagination tests alongside BREAK_LINE, PAGE_MARKER, and LABEL) to follow the guideline of extracting constants only when reused.
🤖 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.
Nitpick comments:
In `@docs/plans/2026-05-22-pagination-unified-plan.md`:
- Line 1: The file currently contains only the token "PLACEHOLDER"; replace that
token with a short explanatory paragraph that states this doc is the intended
consolidation point for choosing/combining the Plan B and Plan T approaches
after design review, including expected next steps, ownership, and a tentative
timeline (or alternatively delete the file until the unified decision is ready
to document); look for the "PLACEHOLDER" string to locate where to edit.
In `@tooling/e2e/pagination.spec.ts`:
- Around line 28-31: CONTAINER is declared but only used once; remove the
CONTAINER constant and replace its single usage with the literal selector string
'[data-slot="pagination-break-lines"]' so the selector is inlined in
pagination.spec.ts (locate the usage where CONTAINER is referenced in the
pagination tests alongside BREAK_LINE, PAGE_MARKER, and LABEL) to follow the
guideline of extracting constants only when reused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6008d447-7ace-4fb1-977e-1bdfd4509dec
⛔ Files ignored due to path filters (9)
dogfood-output/screenshots/after-fix-full.pngis excluded by!**/*.pngdogfood-output/screenshots/after-fix-narrow.pngis excluded by!**/*.pngdogfood-output/screenshots/initial.pngis excluded by!**/*.pngdogfood-output/screenshots/issue-002-flash.pngis excluded by!**/*.pngdogfood-output/screenshots/issue-004-narrow.pngis excluded by!**/*.pngdogfood-output/screenshots/issue-A-step1.pngis excluded by!**/*.pngdogfood-output/screenshots/issue-A-step2.pngis excluded by!**/*.pngdogfood-output/screenshots/issue-B-empty.pngis excluded by!**/*.pngdogfood-output/screenshots/issue-D-oversized.pngis excluded by!**/*.png
📒 Files selected for processing (6)
docs/plans/2026-05-22-pagination-B-line-projection.mddocs/plans/2026-05-22-pagination-B-plan.mddocs/plans/2026-05-22-pagination-research.mddocs/plans/2026-05-22-pagination-twomode-plan.mddocs/plans/2026-05-22-pagination-unified-plan.mdtooling/e2e/pagination.spec.ts
1 participant
Checklist
pnpm typecheckpnpm lint:fixbun testpnpm brlpnpm changesetSummary by CodeRabbit
Documentation
Tests