apps/main/e2e/navigation.spec.ts (2)

27-50: Responsive tests provide minimal coverage.

These tests only verify that body is visible after viewport changes, which doesn't validate actual responsive behavior. Consider adding assertions for layout-specific elements (e.g., mobile menu visibility, sidebar collapse state).

---

64-86: Accessibility tests are placeholder-level.

These tests verify only that something receives focus, not that focus indicators meet WCAG contrast requirements or that keyboard navigation follows expected tab order. Consider using @axe-core/playwright for more comprehensive accessibility testing.

apps/main/e2e/customers.spec.ts (2)

15-67: All tests are skipped placeholders.

These tests are marked with test.skip() and won't run in CI. Additionally, line 36 uses waitForTimeout(500) which is a Playwright anti-pattern—prefer waiting for specific conditions like waitForResponse or element visibility.

Consider either implementing these tests or tracking them as TODOs in an issue.

♻️ Replace waitForTimeout with explicit wait

-				// Wait for results
-				await page.waitForTimeout(500);
+				// Wait for search results to load
+				await page.waitForResponse(resp => 
+					resp.url().includes('/wp-json/wc/v3/customers') && resp.status() === 200
+				);

Would you like me to open an issue to track implementing these skipped tests?

---

69-145: Excessive use of .catch(() => false) silently swallows test failures.

The pattern if (await element.isVisible({ timeout: 5000 }).catch(() => false)) causes tests to pass silently when elements don't exist. This defeats the purpose of e2e testing since UI regressions won't be caught.

When tests are unskipped, consider restructuring to fail explicitly when expected elements are missing.

apps/main/e2e/fixtures/mock-api.ts (2)

169-197: Non-deterministic order ID may cause flaky tests.

Math.floor(Math.random() * 10000) generates unpredictable IDs. If any test asserts on the created order's ID, it will be flaky.

♻️ Use deterministic ID generation

+let orderIdCounter = 1000;
+
 export async function setupApiMocks(page: Page) {
+	// Reset counter for each test
+	orderIdCounter = 1000;
+
 	// ... other routes ...

 	await page.route('**/wp-json/wc/v3/orders*', async (route: Route) => {
 		const method = route.request().method();

 		if (method === 'POST') {
 			const body = route.request().postDataJSON();
 			const newOrder = {
-				id: Math.floor(Math.random() * 10000),
+				id: ++orderIdCounter,
 				status: 'pending',
 				...body,
 			};

---

143-219: Consider mocking individual resource endpoints.

Currently only list endpoints (/products*, /customers*) are mocked. Tests that fetch individual items (e.g., GET /products/1) will fail. Add routes for single-resource fetches if needed.

apps/main/e2e/products.spec.ts (2)

42-42: Prefer event-based waiting over waitForTimeout.

Using page.waitForTimeout(500) for debounce delays is fragile and can cause flaky tests. When these tests are unskipped, consider replacing with more deterministic waits.

Example improvement

-await page.waitForTimeout(500); // Debounce delay
+// Wait for network idle or specific response after debounce
+await page.waitForLoadState('networkidle');

Alternatively, wait for the specific API response:

await page.waitForResponse(resp => resp.url().includes('/products') && resp.status() === 200);

Also applies to: 67-67

---

22-26: Visibility guards may silently pass when elements are missing.

The pattern if (await element.isVisible().catch(() => false)) will skip the test body if the element isn't found, which could mask actual failures. Consider adding explicit assertions or logging when elements aren't found, especially once these tests are unskipped.

Also applies to: 37-47

apps/main/e2e/pos-checkout.spec.ts (2)

50-63: Test assumes pre-existing cart state without setup.

The "should update cart item quantity" test navigates to / and immediately looks for a quantity input, but there's no prior step to add a product to the cart. When unskipped, this test may fail or be unreliable.

Suggested approach

 test.skip('should update cart item quantity', async ({ page }) => {
   await page.goto('/');

-  // Assuming a product is already in cart
+  // First add a product to cart
+  const productCard = page.locator(`[data-testid="product-card"]`).first();
+  if (await productCard.isVisible({ timeout: 5000 }).catch(() => false)) {
+    await productCard.click();
+  }
+
   const quantityInput = page.locator('[data-testid="quantity-input"], input[type="number"]');

---

65-80: Same issue: "remove item" test lacks cart setup.

This test also assumes a cart item exists without adding one first. The test setup should ensure the cart has at least one item before attempting removal.

packages/hooks/jest.config.js (1)

1-1: Consider centralizing TEST_REGEX constant.

The TEST_REGEX constant is duplicated identically across all package Jest configs (components, core, database, hooks, utils). Consider extracting to a shared config or the root jest.config.js to reduce duplication.

.github/workflows/deploy.yml (1)

57-71: URL extraction may silently fail without indication.

If the EAS deploy output doesn't match either the JSON format or the regex pattern, $URL will be empty and silently passed through. Consider adding validation or failing the step if no URL is captured.

Suggested improvement

          echo "Deployment URL: $URL"
+         if [ -z "$URL" ]; then
+           echo "::warning::Could not extract deployment URL from EAS output"
+         fi
          echo "preview_url=$URL" >> $GITHUB_OUTPUT

apps/main/e2e/orders.spec.ts (1)

61-61: Same waitForTimeout pattern as other test files.

These hardcoded delays should be replaced with event-based waiting when the tests are unskipped, as noted in the products.spec.ts review.

Also applies to: 79-79, 182-182

.github/workflows/e2e.yml (1)

1-26: Add explicit permissions for PR comments.

The workflow writes PR comments via actions/github-script@v7 (lines 109–163), which requires write access to issues. Without explicit permissions, the default GITHUB_TOKEN may be read-only, causing the step to fail. Add this permissions block:

🔧 Suggested permissions block

+permissions:
+  contents: read
+  actions: read
+  pull-requests: write
+  issues: write

.github/workflows/deploy.yml (2)

82-91: Minor duplication of production URL.

The production URL https://wcpos.expo.app is hardcoded in both line 80 and line 89. Consider defining it as an environment variable at the workflow level for easier maintenance.

Optional: Define production URL as workflow env variable

 env:
   EXPO_USE_FAST_RESOLVER: true # Use the faster Metro resolver in SDK 51
+  PRODUCTION_URL: https://wcpos.expo.app

Then reference ${{ env.PRODUCTION_URL }} in both places.

---

106-144: PR comment logic is well-implemented with idempotent updates.

Good implementation pattern: finding and updating existing comments prevents duplicate comments on PR synchronize events.

One minor note: the URL is directly interpolated into a JS string on line 111. If an attacker could somehow control the EAS output to include a URL with a single quote, it could break the script. This is low risk since URLs come from EAS output, but for defense-in-depth, consider using JSON.stringify:

Optional: Safer string interpolation

          script: |
-           const url = '${{ steps.deploy-preview.outputs.preview_url }}';
+           const url = ${{ toJson(steps.deploy-preview.outputs.preview_url) }};
            const body = `## 🚀 Preview Deployment

packages/hooks/src/use-hotkeys/validate.test.ts (1)

47-47: Duplicate assertion for space character.

The space character ' ' is tested at line 47 (in "special characters") and again at line 51 (in "whitespace"). Consider removing one to avoid redundancy.

♻️ Suggested fix

 		describe('invalid inputs', () => {
 			it('should return false for special characters', () => {
 				expect(isAlphaNumeric('!')).toBe(false);
 				expect(isAlphaNumeric('@')).toBe(false);
 				expect(isAlphaNumeric('#')).toBe(false);
 				expect(isAlphaNumeric('$')).toBe(false);
 				expect(isAlphaNumeric('-')).toBe(false);
 				expect(isAlphaNumeric('_')).toBe(false);
 				expect(isAlphaNumeric('.')).toBe(false);
-				expect(isAlphaNumeric(' ')).toBe(false);
 			});

 			it('should return false for whitespace', () => {
 				expect(isAlphaNumeric(' ')).toBe(false);
 				expect(isAlphaNumeric('\t')).toBe(false);
 				expect(isAlphaNumeric('\n')).toBe(false);
 			});
 		});

Also applies to: 50-51

packages/utils/src/logger/constants.test.ts (1)

22-25: Consider whether empty error codes should be handled differently.

The test documents that an empty string input produces a URL with a trailing slash (/error-codes/). This is the current behavior, but you may want to consider whether the implementation should throw an error or return a fallback for empty inputs, depending on how this function is used in practice.

packages/hooks/src/use-hotkeys/index.test.ts (1)

61-68: Consider using it.each for parameterized tests.

Using forEach inside a single test case means all key assertions run as one test. If any key fails, the failure message won't clearly indicate which specific key caused the issue. Using Jest's it.each provides better test isolation and failure reporting.

♻️ Suggested refactor using it.each

-		it('should handle various key values', () => {
-			const testKeys = ['Escape', 'Tab', 'ArrowUp', 'ArrowDown', ' ', 'a', '1', 'F1'];
-
-			testKeys.forEach((key) => {
-				const event = { key } as RNKeyboardEvent;
-				expect(getKeyFromEvent(event)).toBe(key);
-			});
-		});
+		it.each(['Escape', 'Tab', 'ArrowUp', 'ArrowDown', ' ', 'a', '1', 'F1'])(
+			'should handle key value: %s',
+			(key) => {
+				const event = { key } as RNKeyboardEvent;
+				expect(getKeyFromEvent(event)).toBe(key);
+			}
+		);

packages/components/src/numpad/reducer.test.ts (1)

131-152: Inconsistent payload structure for EVALUATE action.

The division tests omit payload: { decimalSeparator: '.' } in the EVALUATE actions (lines 138, 149), while other EVALUATE tests in this file (lines 47, 63, 79) include it. If the reducer relies on decimalSeparator for formatting, these tests may behave differently or rely on a default value.

For consistency, consider adding the payload:

Suggested fix

 		it('divides numbers', () => {
 			const state = {
 				currentOperand: '2',
 				previousOperand: '10',
 				operation: '÷',
 			};
-			const action = { type: ACTIONS.EVALUATE };
+			const action = { type: ACTIONS.EVALUATE, payload: { decimalSeparator: '.' } };
 			const newState = reducer(state, action);
 			expect(newState.currentOperand).toBe('5');
 		});

 		it('handles division with decimals', () => {
 			const state = {
 				currentOperand: '3',
 				previousOperand: '10',
 				operation: '÷',
 			};
-			const action = { type: ACTIONS.EVALUATE };
+			const action = { type: ACTIONS.EVALUATE, payload: { decimalSeparator: '.' } };
 			const newState = reducer(state, action);
 			expect(newState.currentOperand).toBe('3.333333');
 		});

packages/database/src/collections/utils.ts (2)

1-1: Rename import to avoid shadowing the global isNaN.

The static analysis tool correctly flags that importing isNaN shadows the global property. Rename to avoid confusion.

♻️ Suggested fix

-import isNaN from 'lodash/isNaN';
+import lodashIsNaN from 'lodash/isNaN';

Then update line 25:

-	if (isNaN(num)) return 0;
+	if (lodashIsNaN(num)) return 0;

---

23-27: Avoid using any type; prefer a stricter union type.

As per coding guidelines, any should not be used. Consider using a union type that reflects the expected inputs, or use unknown with proper type narrowing.

♻️ Suggested fix

-export const toSortableInteger = (value: any): number => {
+export const toSortableInteger = (value: string | number | null | undefined): number => {
 	const num = toNumber(value);
-	if (isNaN(num)) return 0;
+	if (lodashIsNaN(num)) return 0;
 	return Math.round(num * 1000000);
 };

If broader input types are genuinely needed for edge cases (e.g., booleans, arrays as tested), unknown with explicit handling would be preferable to any.

packages/core/src/hooks/use-wcpos-auth/utils.test.ts (1)

6-9: Unused mock declaration.

The expo-auth-session mock is declared but none of the imported functions (generateState, buildAuthUrl, parseAuthResult) use makeRedirectUri. Based on the relevant code snippets from utils.ts, these are pure utility functions without expo dependencies. Consider removing this unused mock to avoid confusion.

🧹 Proposed fix

 /**
  * `@jest-environment` node
  */
 import { buildAuthUrl, generateState, parseAuthResult } from './utils';
 
-// Mock expo-auth-session for getRedirectUri
-jest.mock('expo-auth-session', () => ({
-	makeRedirectUri: jest.fn(() => 'wcpos://callback'),
-}));
-
 describe('use-wcpos-auth utils', () => {

packages/database/src/plugins/parse-rest-response.test.ts (1)

794-813: Document intentional deviation from type-safe fallback.

The test correctly documents that when uuid is not found in meta_data, it remains undefined rather than falling back to an empty string (the normal type-safe default for strings). This is because the uuid extraction logic uses continue after checking, skipping the fallback logic.

Consider adding a brief comment in the implementation (parse-rest-response.ts) explaining this intentional behavior, or enhance the test description to clarify this is expected/desired behavior—otherwise future maintainers might view this as a bug.

Suggested test description enhancement

-	it('should not extract uuid if meta_data does not contain it', () => {
+	it('should leave uuid undefined (not empty string) when not found in meta_data - intentional special case', () => {

packages/utils/src/logger/error-codes.test.ts (1)

71-77: Refactor: avoid asserting “not frozen”; it’s a brittle implementation detail.
The test name says “immutable” but it currently asserts not frozen, which could block a future Object.freeze improvement. Prefer focusing on the type-level contract only.

♻️ Proposed update

-		it('should be immutable (const assertion)', () => {
-			// TypeScript enforces this at compile time, but we can verify at runtime
-			expect(Object.isFrozen(ERROR_CODES)).toBe(false); // as const doesn't freeze
-			// But it should be readonly at type level - this is a type check
-			const code: ErrorCode = ERROR_CODES.CONNECTION_TIMEOUT;
-			expect(typeof code).toBe('string');
-		});
+		it('should be readonly at the type level (const assertion)', () => {
+			const code: ErrorCode = ERROR_CODES.CONNECTION_TIMEOUT;
+			expect(typeof code).toBe('string');
+		});

packages/components/src/combobox/utils/fuzzy-search.test.ts (1)

71-77: Assert the preference implied by the test name.

The test says primary label should be preferred, but it only asserts both scores are > 0. Consider comparing the scores to lock in the behavior.

🧪 Suggested assertion

 			const labelMatch = commandScore('product', 'prod', ['alias']);
 			const aliasMatch = commandScore('item', 'prod', ['product']);
 			// Both should match but potentially with different scores
 			expect(labelMatch).toBeGreaterThan(0);
 			expect(aliasMatch).toBeGreaterThan(0);
+			expect(labelMatch).toBeGreaterThan(aliasMatch);

packages/components/src/combobox/utils/filter.test.ts (1)

15-20: Make the empty-query test assert a concrete outcome.

result.length >= 0 always passes, so the test doesn’t guard behavior. If empty query is intended to return all items, assert that; otherwise update the test name/expectation.

🧪 Example assertion (if empty query should return all items)

 			const result = defaultFilter(sampleOptions, '');
 			// The result depends on the implementation - empty string might score all items
-			expect(result.length).toBeGreaterThanOrEqual(0);
+			expect(result.length).toBe(sampleOptions.length);

packages/database/src/plugins/ajv-formats/formats.test.ts (1)

5-8: Consider creating a typed helper to extract validators instead of repeated as any casts.

The repeated pattern of extracting validators with as any casts violates the coding guideline against using any. A typed helper function could centralize this logic and improve type safety.

💡 Suggested helper approach

// At the top of the file, add a helper type and function:
type FormatDefinition = RegExp | ((data: string) => boolean) | {
  validate: RegExp | ((data: string) => boolean);
  type?: string;
};

function getValidator(format: FormatDefinition): (data: string) => boolean {
  if (format instanceof RegExp) {
    return (data: string) => format.test(data);
  }
  if (typeof format === 'function') {
    return format;
  }
  if (typeof format.validate === 'function') {
    return format.validate;
  }
  return (data: string) => (format.validate as RegExp).test(data);
}

// Then usage becomes:
const dateValidate = getValidator(fullFormats.date);

As per coding guidelines: "Do not use 'any' type; use strict types and generics instead".

Also applies to: 46-49, 81-84, 100-103, 113-116, 241-244, 296-299

packages/database/src/plugins/find-one-fix.test.ts (1)

1-1: Avoid any in test scaffolding.
Use minimal typed interfaces for the mock collection/prototype to keep type safety intact.

♻️ Suggested typing refinement

-import { of, firstValueFrom } from 'rxjs';
+import { of, firstValueFrom, type Observable } from 'rxjs';

+type FindOneResult<T = unknown> = {
+	exec: () => Promise<T | null>;
+	$: Observable<T | null>;
+};
+type FindOne = (query: unknown) => FindOneResult;
+type MockCollection = { findOneFix: (query: unknown) => FindOneResult };
+
 describe('findOneFixPlugin', () => {
 	describe('findOneFix behavior', () => {
-		let mockCollection: any;
-		let originalFindOne: jest.Mock;
-		let proto: any;
+		let mockCollection: MockCollection;
+		let originalFindOne: jest.Mock<FindOneResult, [unknown]>;
+		let proto: { findOne: FindOne };
@@
 	describe('edge cases', () => {
-		let mockCollection: any;
-		let originalFindOne: jest.Mock;
-		let proto: any;
+		let mockCollection: MockCollection;
+		let originalFindOne: jest.Mock<FindOneResult, [unknown]>;
+		let proto: { findOne: FindOne };

As per coding guidelines: Do not use 'any' type; use strict types and generics instead.

Also applies to: 27-30, 109-112