docs: add Star History section to README and improve e2e test navigation strategy#179
docs: add Star History section to README and improve e2e test navigation strategy#179
Conversation
…ion strategy * Added a new Star History section to README.md for enhanced visibility of repository popularity. * Updated e2e tests to improve navigation reliability by using Promise.all for URL assertions, reducing flakiness in tests.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
📝 WalkthroughWalkthroughAdded client haptic feedback, updated share/download button text, moved a duplicate README "🌟 Star History" block, renamed tax labels/helper text to include "Rate", adjusted Playwright timeouts, and updated multiple E2E tests to match selector/text and navigation-wait patterns. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(135,206,235,0.5)
participant User
end
rect rgba(144,238,144,0.5)
participant UI as "Client UI\n(page.client / download-link)"
end
rect rgba(255,182,193,0.5)
participant Haptic as "haptic lib\n(src/lib/haptic.ts)"
end
rect rgba(221,160,221,0.5)
participant Analytics as "Analytics/Meta"
end
User->>UI: Click "Generate invoice link" / "Download PDF"
UI->>Haptic: call haptic()
Haptic-->>UI: returns (vibrate or noop)
UI->>Analytics: track event / update metadata
UI-->>User: show toast / start download / update UI state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/invoice-form.test.ts`:
- Around line 25-31: The test currently races
page.waitForURL("**/?template=default") with page.goto("/en/app") so waitForURL
can resolve from prior state; change the flow to navigate first then verify the
redirect target explicitly: call await page.goto("/en/app") (or keep Promise.all
but ensure navigation finishes), then await
page.waitForURL("**/?template=default") and add an assertion on page.url() or
use expect(page.url()).toContain("?template=default") to guarantee the redirect
target is validated (references: page.goto, page.waitForURL, and
page.url()/expect).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.mde2e/invoice-form.test.tse2e/stripe-invoice-template/stripe-invoice-template.test.ts
| // listener starts FIRST | ||
| // navigation starts SECOND | ||
| // should eliminate flakiness | ||
| await Promise.all([ | ||
| page.waitForURL("**/?template=default"), | ||
| page.goto("/en/app"), | ||
| ]); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Playwright page.waitForURL current URL already matches pattern resolution behavior
💡 Result:
In Playwright, page.waitForURL(urlOrPattern, { waitUntil }) is a navigation-style wait: it completes when the main frame’s URL matches and the navigation has reached the requested lifecycle event (load by default). (playwright.dev)
If the page’s current URL already matches the pattern, waitForURL does not need to observe a future navigation to satisfy the URL condition; in that case it effectively just waits for the page/frame to reach the requested load state (and if that load state was already reached, it returns immediately—same semantics as waitForLoadState). (playwright.dev)
Related nuance: Playwright considers navigation “committed” once response headers are parsed and session history is updated—this is when the URL is set; subsequent lifecycle events like domcontentloaded/load happen afterward. (playwright.dev)
Citations:
- 1: https://playwright.dev/docs/api/class-page
- 2: https://playwright.dev/docs/api/class-frame?utm_source=openai
- 3: https://playwright.dev/docs/next/navigations?utm_source=openai
Redirect test can pass without verifying the redirect target.
At line 29, waitForURL("**/?template=default") resolves immediately from the pre-existing URL state set in beforeEach, so this test passes even if /en/app no longer redirects. Assert the URL after navigation completes.
Suggested fix
- // listener starts FIRST
- // navigation starts SECOND
- // should eliminate flakiness
- await Promise.all([
- page.waitForURL("**/?template=default"),
- page.goto("/en/app"),
- ]);
+ await page.goto("/en/app");
+ await expect(page).toHaveURL("/?template=default");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/invoice-form.test.ts` around lines 25 - 31, The test currently races
page.waitForURL("**/?template=default") with page.goto("/en/app") so waitForURL
can resolve from prior state; change the flow to navigate first then verify the
redirect target explicitly: call await page.goto("/en/app") (or keep Promise.all
but ensure navigation finishes), then await
page.waitForURL("**/?template=default") and add an assertion on page.url() or
use expect(page.url()).toContain("?template=default") to guarantee the redirect
target is validated (references: page.goto, page.waitForURL, and
page.url()/expect).
…r clarity * Changed field names from to and to across various test files and components for consistency. * Updated helper messages in tests to enhance clarity and user guidance. * Adjusted timeout settings in Playwright configuration to improve navigation reliability.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/(app)/components/invoice-form/sections/invoice-items.tsx (1)
544-545: Consider localizing theRatesuffix.Line 544 hardcodes
Rate, which can produce mixed-language labels (e.g.,TVA Rate). Prefer deriving the suffix from the active language translations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(app)/components/invoice-form/sections/invoice-items.tsx around lines 544 - 545, The label currently concatenates a hardcoded "Rate" suffix ("{taxLabelText} Rate"), which can mix languages; replace the literal "Rate" with the app's translation lookup (use the same i18n hook used elsewhere in this file, e.g., useTranslations or t) and render "{taxLabelText} {t('rate')}" (or equivalent translation key) instead, adding the import/use of the translation hook in the InvoiceItems component (or wherever Label is rendered) if not already present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playwright.config.ts`:
- Line 61: The Playwright config sets navigationTimeout to 45_000 but the
overall test timeout (the config "timeout" used for local runs) is lower
(40_000), causing navigation waits to outlive the test and be cut off; update
navigationTimeout in the config (navigationTimeout) to be less than or equal to
the global/local test timeout (the config "timeout" value) or increase the
global "timeout" to at least match 45_000 so navigationTimeout and the
test-level timeout are consistent; modify the value near the navigationTimeout
setting and/or the global timeout setting to keep them aligned.
---
Nitpick comments:
In `@src/app/`(app)/components/invoice-form/sections/invoice-items.tsx:
- Around line 544-545: The label currently concatenates a hardcoded "Rate"
suffix ("{taxLabelText} Rate"), which can mix languages; replace the literal
"Rate" with the app's translation lookup (use the same i18n hook used elsewhere
in this file, e.g., useTranslations or t) and render "{taxLabelText}
{t('rate')}" (or equivalent translation key) instead, adding the import/use of
the translation hook in the InvoiceItems component (or wherever Label is
rendered) if not already present.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e/default-invoice-template/default-invoice-template.test.tse2e/generate-invoice-link.test.tse2e/invoice-form.test.tse2e/stripe-invoice-template/stripe-invoice-template.test.tsplaywright.config.tssrc/app/(app)/components/invoice-form/sections/invoice-items.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/invoice-form.test.ts
|
|
||
| // applies to: page.goto(), redirects, page.waitForURL(), clicking links that trigger navigation, form submits that navigate | ||
| navigationTimeout: 30_000, | ||
| navigationTimeout: 45_000, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n playwright.config.ts | head -80Repository: VladSez/easy-invoice-pdf
Length of output: 3613
navigationTimeout exceeds local test timeout.
Line 61 sets navigationTimeout to 45s, but Line 40 limits local test execution to 40s. When running locally, tests abort before navigation operations can complete their configured timeout, reducing the effectiveness of timeout-based flakiness mitigation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright.config.ts` at line 61, The Playwright config sets
navigationTimeout to 45_000 but the overall test timeout (the config "timeout"
used for local runs) is lower (40_000), causing navigation waits to outlive the
test and be cut off; update navigationTimeout in the config (navigationTimeout)
to be less than or equal to the global/local test timeout (the config "timeout"
value) or increase the global "timeout" to at least match 45_000 so
navigationTimeout and the test-level timeout are consistent; modify the value
near the navigationTimeout setting and/or the global timeout setting to keep
them aligned.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/invoice-form.test.ts (1)
1089-1104:⚠️ Potential issue | 🟡 MinorVAT helper-text expectation is inconsistent with the updated copy.
This block still uses the old helper message string, while Line 487 expects the new wording. Keep one canonical string to avoid brittle/failing assertions.
Suggested fix
- const helperText = `Tax rate must be a number between 0-100 or any text (i.e. NP, OO, etc).`; + const helperText = "Enter a number (0-100) or text (e.g., NP, OO, etc).";Based on learnings: Applies to **/*.test.{ts,tsx} : Use meaningful assertions that clearly document expected behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/invoice-form.test.ts` around lines 1089 - 1104, The test uses an outdated helper message string for the VAT input; update the assertion to use the canonical, current helper copy so both checks match. Replace the local helperText used with the canonical message (create or reuse a constant like VAT_HELPER_TEXT) and use that same constant for the invoiceItemsSection assertion and the notification assertion (the locations referencing vatInput, invoiceItemsSection and page.getByLabel("Notifications alt+T")). Ensure both expect(...) calls compare against the identical updated string.
♻️ Duplicate comments (1)
e2e/invoice-form.test.ts (1)
25-31:⚠️ Potential issue | 🟠 MajorRedirect test can pass without proving
/en/appactually redirected.At Line 29,
waitForURL("**/?template=default")can satisfy from pre-existing URL state, so this test may pass even if/en/appredirect breaks. Assert the redirect target after navigation.Suggested fix
- await Promise.all([ - page.waitForURL("**/?template=default"), - page.goto("/en/app"), - ]); + await page.goto("/en/app"); + await expect(page).toHaveURL("/?template=default");Based on learnings: Applies to **/*.test.{ts,tsx} : Use meaningful assertions that clearly document expected behavior.
Also applies to: 38-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/invoice-form.test.ts` around lines 25 - 31, The current Promise.all with page.waitForURL("**/?template=default") can be satisfied by an existing URL, so change the test to perform the navigation first (use page.goto("/en/app") or keep Promise.all but ensure you then assert the URL) and then explicitly assert the final redirect target using Playwright's URL assertion (e.g., call page.url() or expect(page).toHaveURL("**/?template=default")) to prove /en/app actually redirected; update the lines referencing page.goto and page.waitForURL accordingly to verify the redirect target after navigation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@e2e/invoice-form.test.ts`:
- Around line 1089-1104: The test uses an outdated helper message string for the
VAT input; update the assertion to use the canonical, current helper copy so
both checks match. Replace the local helperText used with the canonical message
(create or reuse a constant like VAT_HELPER_TEXT) and use that same constant for
the invoiceItemsSection assertion and the notification assertion (the locations
referencing vatInput, invoiceItemsSection and page.getByLabel("Notifications
alt+T")). Ensure both expect(...) calls compare against the identical updated
string.
---
Duplicate comments:
In `@e2e/invoice-form.test.ts`:
- Around line 25-31: The current Promise.all with
page.waitForURL("**/?template=default") can be satisfied by an existing URL, so
change the test to perform the navigation first (use page.goto("/en/app") or
keep Promise.all but ensure you then assert the URL) and then explicitly assert
the final redirect target using Playwright's URL assertion (e.g., call
page.url() or expect(page).toHaveURL("**/?template=default")) to prove /en/app
actually redirected; update the lines referencing page.goto and page.waitForURL
accordingly to verify the redirect target after navigation.
* Changed button label from Generate a link to invoice to Generate invoice link across multiple test files and components for consistency. * Adjusted timeout settings in Playwright configuration to improve test reliability, increasing the timeout for expect assertions based on the environment.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
playwright.config.ts (1)
61-61:⚠️ Potential issue | 🟡 Minor
navigationTimeoutexceeds local test timeout.The
navigationTimeout(45s) exceeds the local testtimeout(40s at line 40). When running locally, tests will abort before navigation operations can complete their configured timeout, which may cause misleading test failures.Consider aligning these values:
🔧 Suggested fix
- navigationTimeout: 45_000, + navigationTimeout: isLocal ? 35_000 : 45_000,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright.config.ts` at line 61, The configured navigationTimeout (navigationTimeout) is larger than the global test timeout (timeout), causing navigation waits to outlive the test run; update the navigationTimeout value in the Playwright config so it is less than or equal to the global timeout (e.g., set navigationTimeout to the same numeric value as timeout or a smaller value) to ensure navigation waits cannot exceed the test-level timeout.
🧹 Nitpick comments (3)
src/lib/haptic.ts (3)
49-54: Appending todocument.headis unconventional for DOM manipulation.While this iOS haptic trick works, appending interactive elements to
<head>is non-standard. Consider usingdocument.bodyinstead for better semantic correctness, though this is a minor concern given the element is immediately removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/haptic.ts` around lines 49 - 54, Replace appending the temporary `label` to document.head with appending to document.body for semantic correctness: determine a container (e.g., const container = document.body || document.head) and use container.appendChild(label) and container.removeChild(label) in the existing try/finally block (the `label` variable and the try/finally that calls appendChild/label.click()/removeChild). This preserves the fallback when body is not present while making document.body the preferred container.
3-6:supportsHapticmay incorrectly returnfalseon initial SSR hydration.The check runs at module evaluation time. While
"use client"ensures client-side execution, the module may still be evaluated during SSR wherewindowis undefined, causingsupportsHapticto befalseeven on devices that support haptics.Consider using a function or lazy evaluation:
♻️ Suggested refactor
-export const supportsHaptic = - typeof window !== "undefined" - ? window.matchMedia("(pointer: coarse)").matches - : false; +export function supportsHaptic(): boolean { + if (typeof window === "undefined") return false; + return window.matchMedia("(pointer: coarse)").matches; +}Then update usages in
haptic():- if (!supportsHaptic) return; + if (!supportsHaptic()) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/haptic.ts` around lines 3 - 6, supportsHaptic is evaluated at module load and can be false during SSR; change it from a top-level boolean to a runtime check (either export a function like supportsHaptic() or a getter) and update haptic() to call that function at invocation time. Specifically, replace the exported constant supportsHaptic with a function (e.g., function supportsHaptic(): boolean) that performs typeof window !== "undefined" && window.matchMedia("(pointer: coarse)").matches (and any fallback checks), then modify the haptic() implementation to call supportsHaptic() instead of reading a captured boolean so the correct value is determined on client hydration.
55-55: Empty catch block intentionally swallows errors.This is acceptable for haptic feedback since it's a non-critical enhancement. However, consider adding a brief comment to clarify the intent for future maintainers.
- } catch {} + } catch { + // Silently ignore - haptic feedback is non-critical + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/haptic.ts` at line 55, The empty catch block (the catch {} that follows the try invoking haptic feedback) currently swallows errors with no explanation; update that catch to include a brief explanatory comment indicating the swallow is intentional because haptic feedback is non-critical (e.g., "// intentionally ignore errors from haptic feedback - non‑critical UX enhancement"), so future maintainers understand the intent while leaving behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@playwright.config.ts`:
- Line 61: The configured navigationTimeout (navigationTimeout) is larger than
the global test timeout (timeout), causing navigation waits to outlive the test
run; update the navigationTimeout value in the Playwright config so it is less
than or equal to the global timeout (e.g., set navigationTimeout to the same
numeric value as timeout or a smaller value) to ensure navigation waits cannot
exceed the test-level timeout.
---
Nitpick comments:
In `@src/lib/haptic.ts`:
- Around line 49-54: Replace appending the temporary `label` to document.head
with appending to document.body for semantic correctness: determine a container
(e.g., const container = document.body || document.head) and use
container.appendChild(label) and container.removeChild(label) in the existing
try/finally block (the `label` variable and the try/finally that calls
appendChild/label.click()/removeChild). This preserves the fallback when body is
not present while making document.body the preferred container.
- Around line 3-6: supportsHaptic is evaluated at module load and can be false
during SSR; change it from a top-level boolean to a runtime check (either export
a function like supportsHaptic() or a getter) and update haptic() to call that
function at invocation time. Specifically, replace the exported constant
supportsHaptic with a function (e.g., function supportsHaptic(): boolean) that
performs typeof window !== "undefined" && window.matchMedia("(pointer:
coarse)").matches (and any fallback checks), then modify the haptic()
implementation to call supportsHaptic() instead of reading a captured boolean so
the correct value is determined on client hydration.
- Line 55: The empty catch block (the catch {} that follows the try invoking
haptic feedback) currently swallows errors with no explanation; update that
catch to include a brief explanatory comment indicating the swallow is
intentional because haptic feedback is non-critical (e.g., "// intentionally
ignore errors from haptic feedback - non‑critical UX enhancement"), so future
maintainers understand the intent while leaving behavior unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
e2e/generate-invoice-link.test.tse2e/invoice-form.test.tse2e/stripe-invoice-template/share-logic.test.tsplaywright.config.tssrc/app/(app)/components/index.tsxsrc/app/(app)/components/invoice-page-header.tsxsrc/app/(app)/components/invoice-pdf-download-link.tsxsrc/app/(app)/page.client.tsxsrc/lib/haptic.ts
✅ Files skipped from review due to trivial changes (1)
- src/app/(app)/components/index.tsx
1 participant
Summary by CodeRabbit
Documentation
UI
New Features
Tests
Chores