Split bill - #30
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR refactors split-bill from percentage/lock-based allocation to transaction-tag-based allocation. Transactions emit signed amounts and parsed tags in a new splitBillUpdate payload; the component matches transaction tags to people and evenly distributes matched amounts, rendering per-person allocations and an unallocated remainder row. ChangesSplit Bill Tag-Based Allocation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 11
🧹 Nitpick comments (6)
test-split.spec.ts (1)
1-13: ⚡ Quick winConsider removing one of the two exploratory files or marking it as skipped.
test-split.jsandtest-split.spec.tsoverlap heavily and add duplicate coverage/noise in the E2E suite.🤖 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 `@test-split.spec.ts` around lines 1 - 13, There are duplicate exploratory E2E tests (test-split.spec.ts and test-split.js) creating noise; pick one to keep and either delete the other file or mark it as skipped—e.g., in test-split.spec.ts locate the test block named "test split bill" and change it to a skipped test (use the test.skip variant) or remove the file entirely, ensuring only one canonical test for the split-bill flow remains in the suite.static_src/js/transactions.ts (1)
508-512: ⚡ Quick winUse proper type instead of
any[]for transaction arrays.The
checkedTxanduncheckedTxarrays holdSplitBillTransactionobjects but are typed asany[], which loses type safety. Consider using the imported type or inline type.♻️ Suggested type fix
- const checkedTx: any[] = []; - const uncheckedTx: any[] = []; + const checkedTx: { amount: number; tags: string[] }[] = []; + const uncheckedTx: { amount: number; tags: string[] }[] = [];Or import and use
SplitBillTransactionfrom./split_billif it's exported.🤖 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 `@static_src/js/transactions.ts` around lines 508 - 512, The arrays checkedTx and uncheckedTx are typed as any[] losing type safety; change their declarations to use the proper SplitBillTransaction[] type (e.g., const checkedTx: SplitBillTransaction[] = []; const uncheckedTx: SplitBillTransaction[] = []), and if SplitBillTransaction is not in scope import or reference it from its module (for example import { SplitBillTransaction } from './split_bill' or use an inline type alias) so all usages of checkedTx/uncheckedTx are strongly typed.JustAnotherExpenseManager/templates/transactions_list.html (1)
7-9: 💤 Low valueButton text "Reset selection" may confuse users.
The button toggles visibility of checkboxes and resets them when hiding. When checkboxes are hidden, clicking "Reset selection" actually shows them (not a reset action). Consider "Toggle selection" or separate show/reset controls for clarity.
🤖 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 `@JustAnotherExpenseManager/templates/transactions_list.html` around lines 7 - 9, The button with id "split-select-toggle" in transactions_list.html has misleading text; change its label to "Toggle selection" (or similar) to reflect that it toggles visibility, or split into two controls: keep "Show/Hide selection" toggle (id "split-select-toggle") to control checkbox visibility and add a separate "Reset selection" button that clears all checkboxes when clicked; ensure the toggle updates accessible state (e.g., aria-pressed) and the reset button's handler explicitly unchecks inputs with the selection class.JustAnotherExpenseManager/static/js/split_bill.js (1)
101-101: 🏗️ Heavy liftReplace dynamic
innerHTMLrendering with DOM node creation.These assignments are exactly where OSSAR is flagging unsafe DOM writes. Even with partial escaping, moving to
createElement/textContentremoves this class of issue and aligns with the security lint policy.Also applies to: 106-106, 128-128, 142-142
🤖 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 `@JustAnotherExpenseManager/static/js/split_bill.js` at line 101, Replace unsafe innerHTML assignments on the tableBody element with explicit DOM creation: instead of setting tableBody.innerHTML to a string (e.g., the row with class "split-empty" and other similar strings at the other flagged sites), create the tr and td elements via document.createElement, set attributes/classes (e.g., td.className = "split-empty"), set text via textContent, and append the nodes to tableBody; update all occurrences where tableBody.innerHTML is used (the current instance and the ones referenced at the other flagged locations) and ensure colspan is set via setAttribute or td.colSpan so no raw HTML strings are injected.tests/11-split-bill.spec.ts (2)
127-127: ⚡ Quick winRemove temporary console logging from the test path.
Line 127 adds ad-hoc debug output that can clutter CI logs and make failures harder to scan.
Suggested cleanup
- transactionsPage.page.on('console', msg => console.log('PAGE LOG:', msg.text()));🤖 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/11-split-bill.spec.ts` at line 127, Remove the temporary ad-hoc console logging added to the test by deleting the transactionsPage.page.on('console', msg => console.log('PAGE LOG:', msg.text())); line; if you need console capture for debugging keep it behind a configurable flag or use the test framework's logging utilities instead so CI logs remain clean.
311-312: ⚡ Quick winAvoid clicking
Reset selectionbefore any row is selected.Line 311 is not needed for this test’s intent and can become flaky if the button is disabled/hidden in the initial state.
Suggested simplification
- await transactionsPage.split.resetSelection.click(); await transactionsPage.table.getByRole('row').filter({ hasText: 'Coffee' }).locator('.split-select-checkbox').check();🤖 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/11-split-bill.spec.ts` around lines 311 - 312, The test unnecessarily clicks transactionsPage.split.resetSelection before any selection, which can be flaky if that control is disabled/hidden; remove the await transactionsPage.split.resetSelection.click() call and simply perform the selection via transactionsPage.table.getByRole('row').filter({ hasText: 'Coffee' }).locator('.split-select-checkbox').check() so the test directly selects the intended row without interacting with the reset button.
🤖 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 `@JustAnotherExpenseManager/static/js/split_bill.js`:
- Around line 45-49: The event handler for splitBillUpdate should normalize
incoming transactions so calculateSplits() never sees a missing/non-array tags;
when processing e.detail in the splitBillUpdate handler (where you set
this.total and this.transactions), coerce detail.transactions to an array
default and map each transaction to ensure a tags property that is an array
(e.g., if tx.tags is missing or not an array replace with []), then assign that
normalized array to this.transactions before calling this.renderTotalAndTable();
this prevents tx.tags.some(...) in calculateSplits() from throwing.
- Line 57: The duplicate-check when adding a person uses a case-sensitive
comparison (this.people.some((p) => p.name === name.trim())), causing "Alice"
and "alice" to both be added; update the check in the add-person logic to
normalize both sides (e.g., trim and toLowerCase) before comparing so it matches
the case-insensitive tag matching used elsewhere (also update the corresponding
tag-matching spot around the tag handling at the other check to use the same
normalization strategy) to prevent case-variant duplicates.
- Line 121: The remove button markup rendering the element with class
"split-remove-btn" and data attributes (data-action="remove", data-id="${p.id}")
currently omits an explicit button type, which can default to submit inside
forms; update the template/JS that returns this string (the code building the
`<button class="split-remove-btn" ...">×</button>` HTML) to include
type="button" on that element so clicks won't trigger form submission.
In `@JustAnotherExpenseManager/templates/split_bill_component.html`:
- Around line 13-15: The tag selector (<select> with class "split-name-input"
and data-action="name-input") lacks an accessible label; add a visible <label>
associated with the select (give the select an id and use <label
for="...">Tag</label>) or, if a visible label is not desired, provide an
aria-label or aria-labelledby that describes the control (e.g., "Tag selector")
so screen readers and keyboard users can identify the control.
- Around line 16-17: The Add Person button (class="btn btn-primary
split-add-btn", data-action="add") is missing an explicit type and will submit
any surrounding form; modify that button element to include type="button" to
prevent accidental form submission while keeping its click behavior for the add
action.
In `@test-split.js`:
- Line 3: Replace the hardcoded absolute URL in the test's call to page.goto
with a relative path that uses the Playwright-configured baseURL (i.e., change
the call to use '/transactions' instead of
'http://localhost:5000/transactions'); locate the page.goto invocation in
test-split.js and update it to use the relative route so the test respects
CI/baseURL configuration.
- Line 4: Replace the brittle fixed sleep (page.waitForTimeout(1000)) with a
deterministic wait for the UI change: locate where the table is populated and
use page.waitForSelector (e.g., wait for a table row selector like 'table tbody
tr' or a specific cell text) or page.waitForResponse (matching the fetch
endpoint that populates the table) instead of waitForTimeout; update the test to
await page.waitForSelector(...) or await page.waitForResponse(...) before
asserting table contents so the test only proceeds once the async fetch/render
completes.
- Around line 7-12: The test currently only logs tagsStr from page.evaluate and
has no assertion; replace the console.log with an assertion that verifies the
expected behavior: use the tagsStr result from the page.evaluate block (or
re-evaluate a query like document.querySelectorAll('.btn-edit[data-tags]')) and
assert either that tagsStr is not 'no button' / not empty (for the non-empty
case) or explicitly assert the empty-state behavior when no .btn-edit exists;
update the test to fail on regressions by asserting the presence (or explicit
absence) of a data-tags attribute instead of only logging it.
In `@test-split.spec.ts`:
- Line 3: Replace the hardcoded host in the Playwright navigation call: locate
the page.goto invocation (the await
page.goto('http://localhost:5000/transactions') call) and change it to use a
relative path so Playwright's baseURL/CI routing is honored (e.g., navigate to
'/transactions' instead of including 'http://localhost:5000'); ensure any
related tests rely on Playwright's configured baseURL rather than an absolute
URL.
- Line 4: Replace the fixed sleep (page.waitForTimeout) with an explicit DOM
wait: locate where page.waitForTimeout(1000) is used and instead wait for a
concrete selector or condition that indicates the transactions list is populated
(e.g. page.waitForSelector('[data-testid="transactions-list"]') or
page.waitForFunction(() =>
document.querySelectorAll('[data-testid="transaction-item"]').length > 0)); this
removes flakiness by waiting for the actual DOM state rather than a fixed
timeout.
- Around line 6-13: The test currently only logs the computed `results` from the
`page.evaluate` call (which selects rows via
`document.querySelectorAll('tr[data-amount]')` and maps each row's edit button
`data-tags`) and never asserts anything; replace the console.log with real
assertions that validate `results` (for example check expected array length,
that no entry equals 'NO_BTN' if every row should have an edit button, and/or
that specific expected tag strings are present). Update the test block that
defines `results` to use your test runner's assertion helpers (e.g.,
`expect(results).toEqual(...)`, `expect(results).not.toContain('NO_BTN')`, or
similar) so the spec fails when the DOM is incorrect.
---
Nitpick comments:
In `@JustAnotherExpenseManager/static/js/split_bill.js`:
- Line 101: Replace unsafe innerHTML assignments on the tableBody element with
explicit DOM creation: instead of setting tableBody.innerHTML to a string (e.g.,
the row with class "split-empty" and other similar strings at the other flagged
sites), create the tr and td elements via document.createElement, set
attributes/classes (e.g., td.className = "split-empty"), set text via
textContent, and append the nodes to tableBody; update all occurrences where
tableBody.innerHTML is used (the current instance and the ones referenced at the
other flagged locations) and ensure colspan is set via setAttribute or
td.colSpan so no raw HTML strings are injected.
In `@JustAnotherExpenseManager/templates/transactions_list.html`:
- Around line 7-9: The button with id "split-select-toggle" in
transactions_list.html has misleading text; change its label to "Toggle
selection" (or similar) to reflect that it toggles visibility, or split into two
controls: keep "Show/Hide selection" toggle (id "split-select-toggle") to
control checkbox visibility and add a separate "Reset selection" button that
clears all checkboxes when clicked; ensure the toggle updates accessible state
(e.g., aria-pressed) and the reset button's handler explicitly unchecks inputs
with the selection class.
In `@static_src/js/transactions.ts`:
- Around line 508-512: The arrays checkedTx and uncheckedTx are typed as any[]
losing type safety; change their declarations to use the proper
SplitBillTransaction[] type (e.g., const checkedTx: SplitBillTransaction[] = [];
const uncheckedTx: SplitBillTransaction[] = []), and if SplitBillTransaction is
not in scope import or reference it from its module (for example import {
SplitBillTransaction } from './split_bill' or use an inline type alias) so all
usages of checkedTx/uncheckedTx are strongly typed.
In `@test-split.spec.ts`:
- Around line 1-13: There are duplicate exploratory E2E tests
(test-split.spec.ts and test-split.js) creating noise; pick one to keep and
either delete the other file or mark it as skipped—e.g., in test-split.spec.ts
locate the test block named "test split bill" and change it to a skipped test
(use the test.skip variant) or remove the file entirely, ensuring only one
canonical test for the split-bill flow remains in the suite.
In `@tests/11-split-bill.spec.ts`:
- Line 127: Remove the temporary ad-hoc console logging added to the test by
deleting the transactionsPage.page.on('console', msg => console.log('PAGE LOG:',
msg.text())); line; if you need console capture for debugging keep it behind a
configurable flag or use the test framework's logging utilities instead so CI
logs remain clean.
- Around line 311-312: The test unnecessarily clicks
transactionsPage.split.resetSelection before any selection, which can be flaky
if that control is disabled/hidden; remove the await
transactionsPage.split.resetSelection.click() call and simply perform the
selection via transactionsPage.table.getByRole('row').filter({ hasText: 'Coffee'
}).locator('.split-select-checkbox').check() so the test directly selects the
intended row without interacting with the reset button.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d86a109b-a073-4f3e-9350-eee32f8e6802
📒 Files selected for processing (13)
JustAnotherExpenseManager/static/css/split_bill.cssJustAnotherExpenseManager/static/js/split_bill.jsJustAnotherExpenseManager/static/js/transactions.jsJustAnotherExpenseManager/templates/split_bill_component.htmlJustAnotherExpenseManager/templates/transactions_list.htmlREADME.mdstatic_src/js/split_bill.tsstatic_src/js/transactions.tstest-split.jstest-split.spec.tstests/11-split-bill.spec.tstests/pages/SplitBillComponent.tstests/pages/TransactionsPage.ts
Comment on lines
+7
to
+12
| const tagsStr = await page.evaluate(() => { | ||
| const editBtn = document.querySelector('.btn-edit'); | ||
| return editBtn ? editBtn.getAttribute('data-tags') : 'no button'; | ||
| }); | ||
| console.log('tagsStr:', tagsStr); | ||
| }); |
There was a problem hiding this comment.
This test has no assertion, so it cannot fail on regressions.
Right now it only logs output; convert this into a real check (e.g., ensure at least one row has a data-tags attribute or explicitly assert empty-state behavior).
Suggested change
- console.log('tagsStr:', tagsStr);
+ expect(tagsStr).not.toBe('no button');
+ expect(tagsStr).not.toBeNull();📝 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.
Suggested change
| const tagsStr = await page.evaluate(() => { | |
| const editBtn = document.querySelector('.btn-edit'); | |
| return editBtn ? editBtn.getAttribute('data-tags') : 'no button'; | |
| }); | |
| console.log('tagsStr:', tagsStr); | |
| }); | |
| const tagsStr = await page.evaluate(() => { | |
| const editBtn = document.querySelector('.btn-edit'); | |
| return editBtn ? editBtn.getAttribute('data-tags') : 'no button'; | |
| }); | |
| expect(tagsStr).not.toBe('no button'); | |
| expect(tagsStr).not.toBeNull(); | |
| }); |
🤖 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 `@test-split.js` around lines 7 - 12, The test currently only logs tagsStr from
page.evaluate and has no assertion; replace the console.log with an assertion
that verifies the expected behavior: use the tagsStr result from the
page.evaluate block (or re-evaluate a query like
document.querySelectorAll('.btn-edit[data-tags]')) and assert either that
tagsStr is not 'no button' / not empty (for the non-empty case) or explicitly
assert the empty-state behavior when no .btn-edit exists; update the test to
fail on regressions by asserting the presence (or explicit absence) of a
data-tags attribute instead of only logging it.
Comment on lines
+6
to
+13
| const results = await page.evaluate(() => { | ||
| return Array.from(document.querySelectorAll('tr[data-amount]')).map(row => { | ||
| const editBtn = row.querySelector('.btn-edit'); | ||
| return editBtn ? editBtn.getAttribute('data-tags') : 'NO_BTN'; | ||
| }); | ||
| }); | ||
| console.log('Results:', results); | ||
| }); |
There was a problem hiding this comment.
Add assertions; logging alone is not a test.
results is computed but never validated, so this test always passes unless it crashes.
Suggested change
const results = await page.evaluate(() => {
return Array.from(document.querySelectorAll('tr[data-amount]')).map(row => {
const editBtn = row.querySelector('.btn-edit');
return editBtn ? editBtn.getAttribute('data-tags') : 'NO_BTN';
});
});
- console.log('Results:', results);
+ expect(results.length).toBeGreaterThan(0);
+ expect(results).not.toContain('NO_BTN');
+ expect(results.some(v => v !== null)).toBeTruthy();📝 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.
Suggested change
| const results = await page.evaluate(() => { | |
| return Array.from(document.querySelectorAll('tr[data-amount]')).map(row => { | |
| const editBtn = row.querySelector('.btn-edit'); | |
| return editBtn ? editBtn.getAttribute('data-tags') : 'NO_BTN'; | |
| }); | |
| }); | |
| console.log('Results:', results); | |
| }); | |
| const results = await page.evaluate(() => { | |
| return Array.from(document.querySelectorAll('tr[data-amount]')).map(row => { | |
| const editBtn = row.querySelector('.btn-edit'); | |
| return editBtn ? editBtn.getAttribute('data-tags') : 'NO_BTN'; | |
| }); | |
| }); | |
| expect(results.length).toBeGreaterThan(0); | |
| expect(results).not.toContain('NO_BTN'); | |
| expect(results.some(v => v !== null)).toBeTruthy(); | |
| }); |
🤖 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 `@test-split.spec.ts` around lines 6 - 13, The test currently only logs the
computed `results` from the `page.evaluate` call (which selects rows via
`document.querySelectorAll('tr[data-amount]')` and maps each row's edit button
`data-tags`) and never asserts anything; replace the console.log with real
assertions that validate `results` (for example check expected array length,
that no entry equals 'NO_BTN' if every row should have an edit button, and/or
that specific expected tag strings are present). Update the test block that
defines `results` to use your test runner's assertion helpers (e.g.,
`expect(results).toEqual(...)`, `expect(results).not.toContain('NO_BTN')`, or
similar) so the spec fails when the DOM is incorrect.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 53-56: The workflow run step currently calls "apt-get update" and
"apt-get upgrade -y ..." without sudo and uses upgrade which won't install
missing libraries; update the Playwright job's run block (the step that runs
"npx playwright install chromium" and the subsequent apt commands) to invoke
privileged commands and install the specific packages instead of upgrading —
call "sudo apt-get update" followed by "sudo apt-get install -y libnss3
libatk-bridge2.0-0 libdrm2 libxcomposite1 libxdamage1 libxrandr2 libgbm1" so the
runner can perform the actions and the required browser dependencies are
actually installed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9044ef45-8885-4262-9fe6-9501ff86f8a1
📒 Files selected for processing (1)
.github/workflows/playwright.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 52-54: The PLAYWRIGHT_DOWNLOAD_CONNECTION_TIMEOUT environment
variable is set too low (1000) causing browser download failures; update the
workflow by either removing the PLAYWRIGHT_DOWNLOAD_CONNECTION_TIMEOUT env entry
entirely so Playwright uses its default, or increase it to a much larger value
(e.g., 300000) near the env block that precedes the run step using "npx
playwright install chromium --with-deps" so downloads have sufficient time to
complete.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 829a18ec-23a0-4571-8f47-596960fa57b5
📒 Files selected for processing (1)
.github/workflows/playwright.yml
Comment on lines
+52
to
+54
| env: | ||
| PLAYWRIGHT_DOWNLOAD_CONNECTION_TIMEOUT: 1000 | ||
| run: npx playwright install chromium --with-deps |
There was a problem hiding this comment.
The connection timeout is too short and will cause browser installation failures.
PLAYWRIGHT_DOWNLOAD_CONNECTION_TIMEOUT: 1000 sets a 1-second connection timeout for downloading Playwright browser binaries, which are typically 100+ MB. This will cause frequent CI failures, especially on moderate or slower network connections where downloads routinely take 30-60+ seconds.
⏱️ Proposed fix
Either remove the timeout entirely to use Playwright's default, or set a much higher value:
- name: Install Playwright browsers
- env:
- PLAYWRIGHT_DOWNLOAD_CONNECTION_TIMEOUT: 1000
run: npx playwright install chromium --with-depsOr, if you need an explicit timeout, use at least 300 seconds (5 minutes):
- name: Install Playwright browsers
env:
- PLAYWRIGHT_DOWNLOAD_CONNECTION_TIMEOUT: 1000
+ PLAYWRIGHT_DOWNLOAD_CONNECTION_TIMEOUT: 300000
run: npx playwright install chromium --with-deps📝 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.
Suggested change
| env: | |
| PLAYWRIGHT_DOWNLOAD_CONNECTION_TIMEOUT: 1000 | |
| run: npx playwright install chromium --with-deps | |
| - name: Install Playwright browsers | |
| run: npx playwright install chromium --with-deps |
🤖 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 @.github/workflows/playwright.yml around lines 52 - 54, The
PLAYWRIGHT_DOWNLOAD_CONNECTION_TIMEOUT environment variable is set too low
(1000) causing browser download failures; update the workflow by either removing
the PLAYWRIGHT_DOWNLOAD_CONNECTION_TIMEOUT env entry entirely so Playwright uses
its default, or increase it to a much larger value (e.g., 300000) near the env
block that precedes the run step using "npx playwright install chromium
--with-deps" so downloads have sufficient time to complete.
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
1 participant
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.
Summary by CodeRabbit
New Features
Improvements
Documentation