Improve alertbox dismissal handling and Playwright CI coverage#74
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR splits the Playwright CI workflow from one job into three parallel per-browser jobs (chromium, firefox, webkit) with a ChangesAlertbox Dismissal Logic Fix
Per-browser CI Workflow Split
Sequence Diagram(s)sequenceDiagram
participant Banner
participant AlertboxManager
participant Storage
participant Test
Test->>AlertboxManager: addBanners(banners)
AlertboxManager->>AlertboxManager: `#containsDismissedBanner`(banner)
AlertboxManager->>Storage: Check dismissType for session/permanent
Storage-->>AlertboxManager: dismissed status
AlertboxManager-->>Test: Show or hide banner
Test->>Banner: Click close button
Banner->>AlertboxManager: `#handleClick`()
AlertboxManager->>Storage: Store dismissal (if session or permanent)
Storage-->>AlertboxManager: Stored
AlertboxManager-->>Test: Banner dismissed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 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 |
![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)
tests/alertbox-daterange.spec.js (1)
118-125:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClose the manually created browser context before test exit.
browser.newContext()is opened but never closed in this test, which can leak resources and increase flakiness across the suite.Suggested fix
const context = await browser.newContext(); const newPage = await context.newPage(); await newPage.clock.setFixedTime("2025-09-19"); await newPage.goto("/components/alertbox/date-range.html"); const newBanners = newPage.getByRole("status"); await expect(newBanners).toHaveCount(2); + await context.close(); });🤖 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/alertbox-daterange.spec.js` around lines 118 - 125, The browser context created by browser.newContext() in this test is never closed before the test ends, causing resource leaks. After the expect assertion on newBanners, add a cleanup step to close the context by calling await context.close() to ensure proper resource cleanup and prevent test flakiness.
🧹 Nitpick comments (1)
components/alertbox/js/alertbox-manager.js (1)
126-136: ⚡ Quick winAdd required JSDoc blocks for changed methods and class API surface.
The new/updated methods are missing the mandated JSDoc contract tags (
@param,@throws,@fires,@example; and class-level@class/ event docs as applicable).As per coding guidelines, "
**/*.js: Include JSDoc comments for all JavaScript classes and methods with@class,@param,@throws,@fires,@exampletags..." and "**/*.{js,ts}: Document API methods with complete JSDoc signatures including@paramtypes,@throwserror conditions,@firescustom events, and@exampleusage code."Also applies to: 155-192
🤖 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 `@components/alertbox/js/alertbox-manager.js` around lines 126 - 136, The method `#containsDismissedBanner` and other methods in the affected range are missing required JSDoc documentation blocks. Add JSDoc comments for each method that include `@param` tags documenting the parameters (including their types), `@throws` tags for any error conditions, and `@example` tags showing usage examples. Ensure the documentation accurately reflects what each method does, what parameters it accepts, and any exceptions it may throw according to the coding guidelines.Source: Coding guidelines
🤖 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 @.github/workflows/playwright.yml:
- Around line 34-37: The actions/checkout steps at lines 34, 62, and 90 are
using default credential persistence which increases token exposure risk when
jobs upload artifacts. Add persist-credentials: false to the with section of
each actions/checkout@v6 call to disable credential persistence. This
configuration change should be applied to all three checkout steps in the
workflow file to ensure credentials are not persisted in the CI environment.
- Around line 34-35: The GitHub Actions workflow uses tag-based references (such
as `actions/checkout@v6` and other `@v7` tags) instead of immutable full commit
SHAs, which weakens supply-chain security. Replace all tag-based action
references (on the lines specified: 34, 44, 62, 72, 90, 100) with their
corresponding full commit SHA hashes. For each action that currently uses a
version tag like `@v6` or `@v7`, look up the full commit SHA for that version
and replace the tag with the complete SHA hash format (similar to how the
pnpm/setup action is already pinned). Apply this change consistently across all
job definitions mentioned (checkout and other actions).
- Around line 20-21: The workflow jobs chromium and others starting at line 20
lack explicit permission restrictions for the GITHUB_TOKEN, causing them to
inherit overly permissive default scopes. Add a workflow-level permissions block
before the jobs section with minimal required permissions (such as contents:
read for read-only access). If any specific job or step requires elevated
permissions beyond the workflow-level minimum, add a job-level or step-level
permissions override for only those elevated permissions needed.
---
Outside diff comments:
In `@tests/alertbox-daterange.spec.js`:
- Around line 118-125: The browser context created by browser.newContext() in
this test is never closed before the test ends, causing resource leaks. After
the expect assertion on newBanners, add a cleanup step to close the context by
calling await context.close() to ensure proper resource cleanup and prevent test
flakiness.
---
Nitpick comments:
In `@components/alertbox/js/alertbox-manager.js`:
- Around line 126-136: The method `#containsDismissedBanner` and other methods
in the affected range are missing required JSDoc documentation blocks. Add JSDoc
comments for each method that include `@param` tags documenting the parameters
(including their types), `@throws` tags for any error conditions, and `@example`
tags showing usage examples. Ensure the documentation accurately reflects what
each method does, what parameters it accepts, and any exceptions it may throw
according to the coding guidelines.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a03ee91d-f664-4996-8f8a-b5f97bf485d9
📒 Files selected for processing (7)
.github/workflows/playwright.ymlcomponents/alertbox/js/alertbox-manager.jsplaywright.config.jstests/alertbox-daterange.spec.jstests/alertbox-permanent.spec.jstests/alertbox-session.spec.jstests/alertbox.spec.js
2 participants
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores