Adding snapshot tests #321
Conversation
✅ Deploy Preview for graypaper-reader ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds Playwright snapshot testing infrastructure (configs, tests, Docker, CI workflow), local and Docker test scripts, test artifacts ignores, small UI test hooks ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Shell as Shell
participant Docker as Docker Engine
participant Container as Test Container
participant Playwright as Playwright Runner
participant Server as Dev Server (Vite)
participant App as Web App
Dev->>Shell: npm run docker:test / docker:test:update
Shell->>Docker: build image (Dockerfile.snapshot-tests)
Docker->>Container: run container (env: CI, PLAYWRIGHT_*)
Container->>Playwright: npm run test (Playwright)
alt PLAYWRIGHT_START_SERVER=true
Playwright->>Server: start dev server (PORT)
Server->>App: serve built app
end
Playwright->>App: launch browser, capture snapshots (light/dark)
Playwright-->>Container: emit test-results & playwright-report
Container-->>Dev: exit code + mounted artifacts
sequenceDiagram
autonumber
actor GitHub as GH
participant WF as Actions Workflow
participant Docker as Docker
participant Container as Test Container
participant Playwright as Playwright
participant Artifacts as Artifacts Service
participant Bot as PR Commenter
GH->>WF: Push/PR to main
WF->>Docker: build playwright-test image
WF->>Container: run container, execute tests
Container->>Playwright: run snapshot tests
Playwright-->>Artifacts: upload reports (test-results, html)
WF->>Bot: post/update PR comment with report URL & status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
tools/snapshot-tests/package.json (1)
7-7: Module type mismatch with TypeScript config.The package is set to
"type": "commonjs", butplaywright.config.tsuses ES module syntax (import/export default). This works because TypeScript transpiles ES modules, but it creates inconsistency.For clarity and modern conventions, consider setting the type to
"module":- "type": "commonjs", + "type": "module",Alternatively, if you prefer CommonJS, convert the config to use
require()andmodule.exports.tools/snapshot-tests/tests/snaphosts.spec.ts (1)
89-91: Simplify overly complex locator fallbacks.The locators use multiple fallback selectors (e.g., combining
data-testid,:has-text(),aria-label, and class names). This creates fragile tests that are hard to maintain.Prefer a single, stable selector strategy:
- Best: Add
data-testidattributes to the components (similar to how you added them to Tabs and Note components)- Good: Use ARIA roles and labels
- Avoid: CSS classes and
:has-text()which can break with minor UI changesExample refactor for the GitHub button:
- const githubButton = page.locator( - '[data-testid="github-dropdown"], button:has-text("GitHub"), [aria-label*="github"], .github', - ); + const githubButton = page.locator('[data-testid="github-dropdown"]');If
data-testidisn't present, add it to the component. This makes tests more maintainable and aligns with the pattern you're already using for tabs and notes.Also applies to: 117-119, 134-136, 165-167
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (36)
package-lock.jsonis excluded by!**/package-lock.jsontools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/homepage-outline-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/homepage-outline-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/homepage-screenshot-1-chromium-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/homepage-screenshot-1-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/homepage-screenshot-1-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/notes-tab-edit-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/notes-tab-edit-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/notes-tab-initial-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/notes-tab-initial-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/search-initial-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/search-initial-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/search-with-query-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/search-with-query-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/search-with-results-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/search-with-results-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-github-after-hover-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-github-before-hover-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-github-expanded-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-github-expanded-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-github-hover-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-github-hover-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-github-option-hover-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-github-option-hover-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-more-clicked-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-more-clicked-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-more-settings-clicked-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-more-settings-clicked-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-notes-dropdown-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-notes-dropdown-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-notes-selected-off-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-notes-selected-off-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-notes-selected-on-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/topbar-notes-selected-on-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/workflow-start-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/snaphosts.spec.ts-snapshots/workflow-start-light-mode-darwin.pngis excluded by!**/*.png
📒 Files selected for processing (8)
README.md(1 hunks)src/components/NoteManager/components/Note.tsx(1 hunks)src/components/Tabs/Tabs.tsx(1 hunks)tools/snapshot-tests/.gitignore(1 hunks)tools/snapshot-tests/package.json(1 hunks)tools/snapshot-tests/playwright.config.ts(1 hunks)tools/snapshot-tests/tests/snaphosts.spec.ts(1 hunks)tools/snapshot-tests/tests/utils/add-notes-to-local-storage.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
⚙️ CodeRabbit configuration file
When reviewing Tailwind CSS classes, ensure they follow Tailwind 4.x conventions and suggest modern 4.x alternatives for deprecated patterns.
Files:
src/components/NoteManager/components/Note.tsxtools/snapshot-tests/tests/utils/add-notes-to-local-storage.tstools/snapshot-tests/playwright.config.tssrc/components/Tabs/Tabs.tsxtools/snapshot-tests/tests/snaphosts.spec.ts
🧬 Code graph analysis (1)
tools/snapshot-tests/tests/snaphosts.spec.ts (1)
tools/snapshot-tests/tests/utils/add-notes-to-local-storage.ts (1)
initialLocalStorage(34-43)
🪛 GitHub Actions: Node.js CI
tools/snapshot-tests/tests/snaphosts.spec.ts
[error] 21-21: Playwright Test did not expect test.describe() to be called here. This caused the test suite to fail during 'npm test'.
🪛 GitHub Check: build
tools/snapshot-tests/tests/snaphosts.spec.ts
[failure] 21-21: tools/snapshot-tests/tests/snaphosts.spec.ts
Error: Playwright Test did not expect test.describe() to be called here.
Most common reasons include:
- You are calling test.describe() in a configuration file.
- You are calling test.describe() in a file that is imported by the configuration file.
- You have two different versions of @playwright/test. This usually happens
when one of the dependencies in your package.json depends on @playwright/test.
❯ TestTypeImpl._currentSuite node_modules/playwright/lib/common/testType.js:74:13
❯ TestTypeImpl._describe node_modules/playwright/lib/common/testType.js:114:24
❯ Function.describe node_modules/playwright/lib/transform/transform.js:275:12
❯ tools/snapshot-tests/tests/snaphosts.spec.ts:21:6
🔇 Additional comments (3)
src/components/Tabs/Tabs.tsx (1)
118-118: LGTM! Clean test ID addition.The
data-testidattribute with the patterntab-${t.name}provides clear, predictable selectors for snapshot tests without affecting functionality.src/components/NoteManager/components/Note.tsx (1)
107-107: LGTM! Conditional test ID follows the button's behavior.The conditional
data-testidcorrectly reflects whether the button is in "save" or "edit" mode, making tests more readable and maintainable.tools/snapshot-tests/tests/utils/add-notes-to-local-storage.ts (1)
7-7: Verify the future timestamps in test data.The
datefields contain timestamps from October 2025 (e.g.,1759742271192). If the application performs date-based filtering, sorting, or validation, these future dates could cause unexpected test behavior or false negatives.Consider using timestamps relative to the current date or a fixed reference date from the past to ensure deterministic test behavior:
- date: 1759742271192, + date: Date.now() - 86400000, // 1 day agoOr use a fixed historical timestamp:
- date: 1759742271192, + date: 1704067200000, // 2024-01-01Also applies to: 17-17
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tools/snapshot-tests/tests/basic-snapshots.spec.ts (2)
7-19: Consider consistent test setup across all tests.The
getCommonContext()function is only used in some tests (lines 35, 112, 129) but not others. This creates inconsistent test environments where some tests have pre-loaded localStorage (notes and labels) while others start with a clean state.Consider whether all tests should use the same context setup for consistency, or document why certain tests require different setups.
89-91: Simplify locator strategy to reduce brittleness.The locator uses multiple fallback selectors chained together, which makes it brittle and hard to maintain. When UI changes, it's unclear which selector should be updated.
Consider:
- Adding consistent
data-testidattributes to UI elements (preferred)- Using a single, specific selector
- If multiple selectors are needed for different environments, use separate test configurations
Similar patterns appear throughout the file (lines 99-100, 117-119, 134-136, 165-167).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)tools/snapshot-tests/playwright.config.ts(1 hunks)tools/snapshot-tests/tests/basic-snapshots.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/snapshot-tests/playwright.config.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
⚙️ CodeRabbit configuration file
When reviewing Tailwind CSS classes, ensure they follow Tailwind 4.x conventions and suggest modern 4.x alternatives for deprecated patterns.
Files:
tools/snapshot-tests/tests/basic-snapshots.spec.ts
🧬 Code graph analysis (1)
tools/snapshot-tests/tests/basic-snapshots.spec.ts (1)
tools/snapshot-tests/tests/utils/add-notes-to-local-storage.ts (1)
initialLocalStorage(34-43)
🔇 Additional comments (1)
tools/snapshot-tests/tests/basic-snapshots.spec.ts (1)
21-31: LGTM!The homepage tests are well-structured. Good use of
waitUntil: "networkidle"anddocument.fonts.readyto ensure page stability before taking screenshots.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tools/snapshot-tests/tests/basic-snapshots.spec.ts (2)
4-5: Stop hardcoding the app origin.Baking
http://localhost:5173into the test makes the suite environment-specific. Read the target host from Playwright’sbaseURL(or an env var) so these tests run unchanged in CI/staging.Apply this diff:
-const origin = "http://localhost:5173"; -const hostname = "http://localhost:5173/#/ab2cdbd?v=0.7.2"; +const origin = test.info().project.use.baseURL ?? process.env.SNAPSHOT_BASE_URL ?? "http://localhost:5173"; +const hostname = `${origin}/#/ab2cdbd?v=0.7.2`;
144-149: Use a stable locator for the “More” button.
button:has-text("...")is ambiguous; any overflow button with ellipsis will match and make the test flaky. Add a dedicateddata-testid(or equivalent) and target that instead.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (26)
tools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/homepage-outline-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/homepage-outline-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-edit-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-edit-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-initial-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-initial-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-initial-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-initial-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-query-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-query-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-results-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-results-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-expanded-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-expanded-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-option-hover-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-option-hover-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-more-clicked-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-more-clicked-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-more-settings-clicked-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-more-settings-clicked-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-dropdown-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-dropdown-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-off-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-off-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-on-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-on-light-mode-darwin.pngis excluded by!**/*.png
📒 Files selected for processing (2)
src/components/Tabs/Tabs.tsx(2 hunks)tools/snapshot-tests/tests/basic-snapshots.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Tabs/Tabs.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
⚙️ CodeRabbit configuration file
When reviewing Tailwind CSS classes, ensure they follow Tailwind 4.x conventions and suggest modern 4.x alternatives for deprecated patterns.
Files:
tools/snapshot-tests/tests/basic-snapshots.spec.ts
🧬 Code graph analysis (1)
tools/snapshot-tests/tests/basic-snapshots.spec.ts (1)
tools/snapshot-tests/tests/utils/add-notes-to-local-storage.ts (1)
initialLocalStorage(34-43)
chmurson
force-pushed
the
adding-snapshot-tests
branch
from
fc671a4 to
b0a65e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
README.md (1)
38-38: Fix grammar in documentation."Visual snapshot tests checks" should be "Visual snapshot tests check" (verb agreement).
Apply this diff:
-Visual snapshot tests checks for visual regression. +Visual snapshot tests check for visual regression.tools/snapshot-tests/tests/basic-snapshots.spec.ts (4)
36-47: Close the browser context to prevent resource leaks.The test creates a browser context but never closes it, which can lead to resource leaks and test instability.
Apply this diff:
test("notes tab - initial state", async ({ browser }) => { const context = await browser.newContext(getCommonContext()); - - const page = await context.newPage(); - await page.goto(hostname, { waitUntil: "networkidle" }); - await page.evaluate(() => document.fonts.ready); - await page.click('[data-testid="tab-notes"]'); - await page.locator('[data-testid="tab-content-notes"]').waitFor({ state: "visible" }); - await expect(page).toHaveScreenshot("notes-tab-initial.png", { fullPage: true }); - await page.click('[data-testid="edit-button"]', { timeout: 2500 }); - await expect(page).toHaveScreenshot("notes-tab-edit.png", { fullPage: true }); + try { + const page = await context.newPage(); + await page.goto(hostname, { waitUntil: "networkidle" }); + await page.evaluate(() => document.fonts.ready); + await page.click('[data-testid="tab-notes"]'); + await page.locator('[data-testid="tab-content-notes"]').waitFor({ state: "visible" }); + await expect(page).toHaveScreenshot("notes-tab-initial.png", { fullPage: true }); + await page.click('[data-testid="edit-button"]', { timeout: 2500 }); + await expect(page).toHaveScreenshot("notes-tab-edit.png", { fullPage: true }); + } finally { + await context.close(); + } });
101-114: Close the browser context to prevent resource leaks.The test creates a browser context but never closes it, which can lead to resource leaks and test instability.
Apply this diff:
test("notes dropdown - initial state", async ({ browser }) => { const context = await browser.newContext(getCommonContext()); - const page = await context.newPage(); - await page.goto(hostname, { waitUntil: "networkidle" }); - await page.evaluate(() => document.fonts.ready); - - const notesDropdown = page.locator( - '[data-testid="notes-dropdown"], button:has-text("Notes"), [aria-label*="notes"], .notes-dropdown', - ); - if ((await notesDropdown.count()) > 0) { - await notesDropdown.first().click(); - - await expect(page).toHaveScreenshot("topbar-notes-dropdown.png", { fullPage: true }); + try { + const page = await context.newPage(); + await page.goto(hostname, { waitUntil: "networkidle" }); + await page.evaluate(() => document.fonts.ready); + + const notesDropdown = page.locator( + '[data-testid="notes-dropdown"], button:has-text("Notes"), [aria-label*="notes"], .notes-dropdown', + ); + if ((await notesDropdown.count()) > 0) { + await notesDropdown.first().click(); + + await expect(page).toHaveScreenshot("topbar-notes-dropdown.png", { fullPage: true }); + } + } finally { + await context.close(); } });
117-140: Close the browser context to prevent resource leaks.The test creates a browser context but never closes it, which can lead to resource leaks and test instability.
Apply this diff:
test("selecting note on and off", async ({ browser }) => { const context = await browser.newContext(getCommonContext()); - const page = await context.newPage(); - await page.goto(hostname, { waitUntil: "networkidle" }); - await page.evaluate(() => document.fonts.ready); - - const notesDropdown = page.locator( - '[data-testid="notes-dropdown"], button:has-text("Notes"), [aria-label*="notes"], .notes-dropdown', - ); - if ((await notesDropdown.count()) > 0) { - await notesDropdown.first().click(); - - const noteOptions = page.locator('[type="checkbox"], .note-option, [role="menuitemcheckbox"]'); - if ((await noteOptions.count()) > 0) { - await noteOptions.first().click(); - - await expect(page).toHaveScreenshot("topbar-notes-selected-on.png", { fullPage: true }); - - await noteOptions.first().click(); - - await expect(page).toHaveScreenshot("topbar-notes-selected-off.png", { fullPage: true }); + try { + const page = await context.newPage(); + await page.goto(hostname, { waitUntil: "networkidle" }); + await page.evaluate(() => document.fonts.ready); + + const notesDropdown = page.locator( + '[data-testid="notes-dropdown"], button:has-text("Notes"), [aria-label*="notes"], .notes-dropdown', + ); + if ((await notesDropdown.count()) > 0) { + await notesDropdown.first().click(); + + const noteOptions = page.locator('[type="checkbox"], .note-option, [role="menuitemcheckbox"]'); + if ((await noteOptions.count()) > 0) { + await noteOptions.first().click(); + + await expect(page).toHaveScreenshot("topbar-notes-selected-on.png", { fullPage: true }); + + await noteOptions.first().click(); + + await expect(page).toHaveScreenshot("topbar-notes-selected-off.png", { fullPage: true }); + } } + } finally { + await context.close(); } });
148-149: Use a more specific selector for the menu button.The generic selector
button:has-text("...")may match multiple elements. Use a data-testid or more specific selector to ensure the correct button is clicked.Apply this diff:
- const moreButton = page.locator('button:has-text("...")'); + const moreButton = page.locator('[data-testid="more-menu-button"]');Or if a data-testid isn't available:
- const moreButton = page.locator('button:has-text("...")'); + const moreButton = page.locator('[aria-label="More options"]');
🧹 Nitpick comments (1)
tools/snapshot-tests/package.json (1)
13-14: Ensure cross-platform compatibility for npm scripts.The inline environment variable syntax (e.g.,
PLAYWRIGHT_START_SERVER=true) won't work on Windows. Consider using thecross-envpackage for cross-platform compatibility or document that these scripts require Unix-like environments.Option 1: Use cross-env (recommended for cross-platform support):
First, install cross-env:
npm install --save-dev cross-envThen apply this diff:
- "test:with-server": "PLAYWRIGHT_START_SERVER=true playwright test", - "test:local": "PLAYWRIGHT_START_SERVER=true PLAYWRIGHT_PORT=5173 PLAYWRIGHT_HOST=localhost playwright test" + "test:with-server": "cross-env PLAYWRIGHT_START_SERVER=true playwright test", + "test:local": "cross-env PLAYWRIGHT_START_SERVER=true PLAYWRIGHT_PORT=5173 PLAYWRIGHT_HOST=localhost playwright test"Option 2: Document Unix-only requirement in README if Windows support is not needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (30)
package-lock.jsonis excluded by!**/package-lock.jsontools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/homepage-outline-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/homepage-outline-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-edit-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-edit-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-initial-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-initial-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-initial-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-initial-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-query-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-query-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-query-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-query-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-results-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-results-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-results-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-expanded-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-expanded-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-option-hover-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-option-hover-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-more-clicked-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-more-clicked-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-more-settings-clicked-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-more-settings-clicked-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-dropdown-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-dropdown-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-off-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-off-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-on-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-on-light-mode-linux.pngis excluded by!**/*.png
📒 Files selected for processing (12)
.dockerignore(1 hunks).github/workflows/node.js.yml(1 hunks)Dockerfile.playwright(1 hunks)README.md(1 hunks)docker-compose.yml(1 hunks)package.json(1 hunks)shared/links-metadata/package.json(1 hunks)src/components/NoteManager/NoteManager.tsx(1 hunks)tools/snapshot-tests/README.md(1 hunks)tools/snapshot-tests/package.json(1 hunks)tools/snapshot-tests/playwright.config.ts(1 hunks)tools/snapshot-tests/tests/basic-snapshots.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tools/snapshot-tests/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- Dockerfile.playwright
- shared/links-metadata/package.json
- package.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
⚙️ CodeRabbit configuration file
When reviewing Tailwind CSS classes, ensure they follow Tailwind 4.x conventions and suggest modern 4.x alternatives for deprecated patterns.
Files:
src/components/NoteManager/NoteManager.tsxtools/snapshot-tests/tests/basic-snapshots.spec.tstools/snapshot-tests/playwright.config.ts
🧬 Code graph analysis (1)
tools/snapshot-tests/tests/basic-snapshots.spec.ts (1)
tools/snapshot-tests/tests/utils/add-notes-to-local-storage.ts (1)
initialLocalStorage(34-43)
🪛 actionlint (1.7.7)
.github/workflows/node.js.yml
15-15: string should not be empty
(syntax-check)
51-51: the runner of "peaceiris/actions-gh-pages@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
.dockerignore (1)
1-82: LGTM!The .dockerignore file contains appropriate patterns for excluding unnecessary files from the Docker build context, following Node.js best practices.
tools/snapshot-tests/playwright.config.ts (1)
1-41: LGTM!The Playwright configuration is well-structured with conditional server startup, appropriate retry/worker settings for CI, and separate projects for light/dark mode testing.
chmurson
force-pushed
the
adding-snapshot-tests
branch
from
b0a65e8 to
91b4d28
Compare
Visual Regression Test Report ✅ PassedGithub run id: 18309305378 🔗 Artifacts: Download |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/NoteManager/NoteManager.tsx (1)
85-86: Remove inappropriate placeholder text.This inappropriate placeholder was already flagged in a previous review but remains unaddressed. The text "TWOJA STARA!!" is unprofessional and should be replaced with the original descriptive placeholder or another appropriate alternative.
Apply this diff to restore the original placeholder:
- // placeholder="Add a note to the selected fragment. Math typesetting is supported! Use standard delimiters such as $...$, \[...\] or \begin{equation}...\end{equation}." - placeholder="TWOJA STARA!!" + placeholder="Add a note to the selected fragment. Math typesetting is supported! Use standard delimiters such as $...$, \[...\] or \begin{equation}...\end{equation}."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (27)
package-lock.jsonis excluded by!**/package-lock.jsontools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/homepage-outline-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/homepage-outline-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-edit-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-edit-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-initial-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-initial-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-initial-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-initial-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-query-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-query-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-results-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-results-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-expanded-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-expanded-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-option-hover-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-option-hover-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-more-clicked-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-more-clicked-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-more-settings-clicked-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-more-settings-clicked-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-dropdown-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-dropdown-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-off-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-off-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-on-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-on-light-mode-linux.pngis excluded by!**/*.png
📒 Files selected for processing (9)
.dockerignore(1 hunks).github/workflows/node.js.yml(1 hunks).github/workflows/visual-regression.yml(1 hunks)Dockerfile.playwright(1 hunks)README.md(1 hunks)docker-compose.yml(1 hunks)package.json(1 hunks)shared/links-metadata/package.json(1 hunks)src/components/NoteManager/NoteManager.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- package.json
- .github/workflows/node.js.yml
- docker-compose.yml
- README.md
- Dockerfile.playwright
- .dockerignore
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
⚙️ CodeRabbit configuration file
When reviewing Tailwind CSS classes, ensure they follow Tailwind 4.x conventions and suggest modern 4.x alternatives for deprecated patterns.
Files:
src/components/NoteManager/NoteManager.tsx
🪛 actionlint (1.7.7)
.github/workflows/visual-regression.yml
48-48: the runner of "peaceiris/actions-gh-pages@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: visual-tests
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/visual-regression.yml(1 hunks)docker-compose.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: visual-tests
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
.github/workflows/visual-regression.yml (3)
22-29: Improve container stability for Chromium and simplify command.Add shared memory to avoid browser crashes and use -w instead of cd.
- - name: Run visual regression tests - run: | - docker run --rm \ + - name: Run visual regression tests + run: | + docker run --rm --shm-size=1g \ -v ${{ github.workspace }}/tools/snapshot-tests/test-results:/app/tools/snapshot-tests/test-results \ -v ${{ github.workspace }}/tools/snapshot-tests/playwright-report:/app/tools/snapshot-tests/playwright-report \ -e CI=true \ - playwright-test \ - sh -c "cd tools/snapshot-tests && npm run test" + -w /app/tools/snapshot-tests \ + playwright-test \ + npm run test
31-37: Also publish raw test-results (not only the HTML report).If you want junit/json diagnostics, add a guarded upload for tools/snapshot-tests/test-results.
- name: Upload Playwright Report uses: actions/upload-artifact@v4 if: ${{ always() && hashFiles('tools/snapshot-tests/playwright-report/**') != '' }} with: name: playwright-report-${{ github.run_number }} path: tools/snapshot-tests/playwright-report/ retention-days: 30 + + - name: Upload Test Results + uses: actions/upload-artifact@v4 + if: ${{ always() && hashFiles('tools/snapshot-tests/test-results/**') != '' }} + with: + name: test-results-${{ github.run_number }} + path: tools/snapshot-tests/test-results/ + retention-days: 30
43-51: Optional: include a direct link to the Playwright report artifact.You already link the run; adding a deep link improves UX.
- const body = `## Visual Regression Test Report ${testStatus} - - Github run id: ${{github.run_id}} - - 🔗 **Artifacts**: [Download](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}) - ` + const body = `## Visual Regression Test Report ${testStatus} + + Github run id: ${{github.run_id}} + + 🔗 **Artifacts**: [Run page](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}) + `Dockerfile.snapshot-tests (4)
32-35: Prefernpm cifor reproducible, faster installs in CI.Locks to package-lock and avoids mutation.
-RUN cd tools/snapshot-tests && npm install +RUN cd tools/snapshot-tests && npm ci
49-49: No-op RUN layer.
RUN cd shared/links-metadatadoes nothing in a separate layer; remove it to keep the image lean.-RUN cd shared/links-metadata
3-9: Verify necessity of installing Node 22 over Playwright base image.Mixing Node runtimes increases size and risk. If Node 22 isn’t required, drop this and use the Node bundled in the Playwright image.
-RUN apt-get update && apt-get install -y --no-install-recommends \ - git \ - curl \ - && rm -rf /var/lib/apt/lists/* \ - && curl -fsSL https://deb.nodesource.com/setup_22.x | bash - \ - && apt-get install -y nodejs \ - && rm -rf /var/lib/apt/lists/* +RUN apt-get update && apt-get install -y --no-install-recommends git curl \ + && rm -rf /var/lib/apt/lists/*If Node 22 is required, consider using a Playwright base tag that already bundles the desired Node version to avoid manual installs.
42-48: Avoid copying.gitinto the image.It bloats layers and may leak history. Only copy build inputs.
-COPY .git ./.git -COPY .gitmodules ./
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/visual-regression.yml(1 hunks)Dockerfile.snapshot-tests(1 hunks)docker-compose.yml(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- docker-compose.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: visual-tests
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/visual-regression.yml(1 hunks)Dockerfile.snapshot-tests(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile.snapshot-tests
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: visual-tests
| const body = `## Visual Regression Test Report ${testStatus} | ||
|
|
||
| Github run id: ${{github.run_id}} | ||
|
|
||
| 🔗 **Artifacts**: [Download](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}) | ||
| ` |
There was a problem hiding this comment.
Dedent the PR comment body to avoid code-block rendering
The template literal inherits the 12-space YAML indentation, so every line after the header is treated as a Markdown code block. The report link ends up inside that block and is no longer clickable, breaking the workflow’s UX. Build the string without leading spaces (e.g., join an array of lines) so the comment renders normally.
- const body = `## Visual Regression Test Report ${testStatus}
-
- Github run id: ${{github.run_id}}
-
- 🔗 **Artifacts**: [Download](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }})
- `
+ const body = [
+ `## Visual Regression Test Report ${testStatus}`,
+ '',
+ `GitHub run id: ${{ github.run_id }}`,
+ '',
+ `🔗 **Artifacts**: [Download](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }})`
+ ].join('\n');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const body = `## Visual Regression Test Report ${testStatus} | |
| Github run id: ${{github.run_id}} | |
| 🔗 **Artifacts**: [Download](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}) | |
| ` | |
| const body = [ | |
| `## Visual Regression Test Report ${testStatus}`, | |
| '', | |
| `GitHub run id: ${{ github.run_id }}`, | |
| '', | |
| `🔗 **Artifacts**: [Download](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }})` | |
| ].join('\n'); |
🤖 Prompt for AI Agents
.github/workflows/visual-regression.yml around lines 49 to 54: the template
literal for the PR comment body is indented in YAML so each line gains leading
spaces and renders as a Markdown code block; rebuild the body without leading
spaces (for example, construct it by joining an array of lines or using
unindented concatenation) so the header and subsequent lines are not prefixed by
spaces and the artifact link renders as a normal clickable link.
1 participant
Summary by CodeRabbit