test: added tests for coverage #896
Conversation
WalkthroughThis PR adds comprehensive test coverage across unit tests for modal, upload, and utility modules, introduces multiple end-to-end tests for file uploader and cloud editor components, removes unused focus-visible polyfill code, adds test identifiers to UI components, and updates test configurations including coverage settings and TypeScript path mappings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import { beforeAll, beforeEach, describe, expect, it, test } from 'vitest'; | ||
| import '../types/jsx'; | ||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { renderer } from './utils/test-renderer'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 18 days ago
To fix the unused import error, remove the import declaration for renderer from ./utils/test-renderer on line 6. This change should be made only at line 6 of tests/file-uploader-minimal.e2e.test.tsx. No other code or imports need editing. This will ensure that the codebase is cleaner, avoids confusion, and any initialization side effects (if needed) should be handled elsewhere, explicitly, if required.
| @@ -3,7 +3,6 @@ | ||
| import '../types/jsx'; | ||
| import fi from '@/locales/file-uploader/fi'; | ||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { renderer } from './utils/test-renderer'; | ||
|
|
||
| beforeAll(async () => { | ||
| // biome-ignore lint/suspicious/noTsIgnore: Ignoring TypeScript error for CSS import |
![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
🧹 Nitpick comments (3)
src/utils/get-top-level-origin.test.ts (1)
4-9: Consider testing edge cases for iframe scenarios.The test validates the basic case but doesn't verify the "top-level" behavior in iframe contexts, which appears to be the function's primary purpose. Consider adding tests that verify behavior when the code is running in an iframe versus the top-level window.
tests/file-uploader-minimal.e2e.test.tsx (2)
31-35: Use more specific assertion.
toBeDefined()only checks if the element exists but doesn't verify visibility or usability. Consider usingtoBeInTheDocument()or a similar matcher that better reflects the test intention.Apply this diff:
it('should open file dialog on click', async () => { await page.getByText('Choose file', { exact: true }).click(); const startFrom = page.getByTestId('uc-start-from'); - await expect(startFrom).toBeDefined(); + await expect.element(startFrom).toBeInTheDocument(); });
37-46: Consider more realistic drag-and-drop test data.The test drags the "Powered by Uploadcare" copyright text onto the uploader, which seems like a test workaround rather than realistic user behavior. Consider using a dedicated test element or file-like data structure for more meaningful validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
package.json(1 hunks)src/abstract/managers/__tests__/ModalManager.test.ts(1 hunks)src/abstract/managers/__tests__/SecureUploadsManager.test.ts(1 hunks)src/utils/get-top-level-origin.test.ts(1 hunks)tests/file-uploader-minimal.e2e.test.tsx(1 hunks)vite.config.js(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
tests/file-uploader-minimal.e2e.test.tsx
[notice] 1-1: Unused variable, import, function or class
Unused import commands.
[notice] 2-2: Unused variable, import, function or class
Unused import test.
[notice] 5-5: Unused variable, import, function or class
Unused import renderer.
⏰ 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 (4)
package.json (1)
85-85: LGTM!The addition of
--coverageto the e2e dev script aligns with the coverage configuration changes in vite.config.js and enables coverage reporting during development testing.vite.config.js (1)
20-31: LGTM!The coverage configuration properly excludes test files and sets reasonable thresholds. The exclude patterns appropriately filter out tests, config files, and locales from coverage calculations, while the thresholds (80-85%) align with the PR's coverage improvement goals.
src/abstract/managers/__tests__/SecureUploadsManager.test.ts (1)
1-369: LGTM!Comprehensive test suite with excellent coverage of edge cases, error handling, caching behavior, and time-sensitive expiration logic. The use of fake timers, proper mocking, and thorough assertions demonstrates high-quality test engineering.
src/abstract/managers/__tests__/ModalManager.test.ts (1)
1-474: LGTM!Excellent test suite with comprehensive coverage of modal lifecycle, event system, error handling, and edge cases. The tests are well-structured with proper mocking and clear assertions that validate both happy paths and error scenarios.
| @@ -0,0 +1,48 @@ | |||
| import { commands, page, userEvent } from '@vitest/browser/context'; | |||
There was a problem hiding this comment.
Remove unused import.
The commands import is unused according to static analysis.
Apply this diff:
-import { commands, page, userEvent } from '@vitest/browser/context';
+import { page, userEvent } from '@vitest/browser/context';📝 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.
| import { commands, page, userEvent } from '@vitest/browser/context'; | |
| import { page, userEvent } from '@vitest/browser/context'; |
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 1-1: Unused variable, import, function or class
Unused import commands.
🤖 Prompt for AI Agents
In tests/file-uploader-minimal.e2e.test.tsx around line 1, the import includes
an unused identifier `commands`; remove it from the import list so only the used
symbols (page, userEvent) are imported to satisfy static analysis and avoid lint
errors.
| import { beforeAll, beforeEach, describe, expect, it, test } from 'vitest'; | ||
| import '../types/jsx'; | ||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { renderer } from './utils/test-renderer'; |
There was a problem hiding this comment.
Remove unused import.
The renderer import is unused according to static analysis.
Apply this diff:
-// biome-ignore lint/correctness/noUnusedImports: Used in JSX
-import { renderer } from './utils/test-renderer';Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 5-5: Unused variable, import, function or class
Unused import renderer.
🤖 Prompt for AI Agents
In tests/file-uploader-minimal.e2e.test.tsx around line 5, the import "renderer"
is unused; remove the line "import { renderer } from './utils/test-renderer';"
from the file (or if other tests need it, replace with a named import that is
used), then save the file and run the linter/tests to verify no unused-import
errors remain.
egordidenko
force-pushed
the
test/coverage-80
branch
from
e008ae3 to
05fb135
Compare
| import { commands, page, userEvent } from '@vitest/browser/context'; | ||
| import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; | ||
| import '../types/jsx'; | ||
| import fi from '@/locales/file-uploader/fi'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 18 days ago
The best way to fix this problem is to remove the unused import statement for fi on line 4 in tests/file-uploader-minimal.e2e.test.tsx. Only imports that are used elsewhere in this file (or intentionally included for side-effects, with a clear comment) should remain. Deleting this line will not affect any existing functionality, as there are no usages or references to this identifier. There are no follow-up changes or new dependencies required.
| @@ -1,9 +1,6 @@ | ||
| import { commands, page, userEvent } from '@vitest/browser/context'; | ||
| import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; | ||
| import '../types/jsx'; | ||
| import fi from '@/locales/file-uploader/fi'; | ||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { renderer } from './utils/test-renderer'; | ||
|
|
||
| beforeAll(async () => { | ||
| // biome-ignore lint/suspicious/noTsIgnore: Ignoring TypeScript error for CSS import |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/file-uploader-minimal.e2e.test.tsx (1)
5-6: Remove unused import.The
rendererimport is unused despite the biome-ignore comment claiming it's used in JSX. The comment is misleading sincerendererdoesn't appear anywhere in the file.Apply this diff:
-// biome-ignore lint/correctness/noUnusedImports: Used in JSX -import { renderer } from './utils/test-renderer';
🧹 Nitpick comments (2)
src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts (1)
1-5: Consider enhancing the deprecation notice with migration guidance.The
@deprecatedtag is properly added. For better developer experience, consider specifying:
- What developers should use instead (e.g., native
:focus-visibleCSS pseudo-class)- The target version for removal
/** * This module is deprecated and will be removed in future versions. - * @deprecated - * - * + * @deprecated Use the native `:focus-visible` CSS pseudo-class instead. + * * Helper function for legacy browsers and iframes which sometimes focus on elements like document, body, and * non-interactive SVG. */tests/file-uploader-minimal.e2e.test.tsx (1)
32-36: Use a more robust assertion for visibility testing.The test name indicates it should verify that a dialog opens (implying visibility), but
toBeDefined()only checks existence. Other tests in this file and related test files usetoBeVisible()for similar assertions.Apply this diff:
it('should open file dialog on click', async () => { await page.getByText('Choose file', { exact: true }).click(); const startFrom = page.getByTestId('uc-start-from'); - await expect(startFrom).toBeDefined(); + await expect.element(startFrom).toBeVisible(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
package.json(1 hunks)src/abstract/managers/__tests__/ModalManager.test.ts(1 hunks)src/abstract/managers/__tests__/SecureUploadsManager.test.ts(1 hunks)src/blocks/CloudImageEditor/src/lib/FocusVisible.ts(1 hunks)src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts(1 hunks)src/blocks/Config/initialConfig.ts(1 hunks)src/blocks/FileItem/FileItem.ts(1 hunks)src/utils/get-top-level-origin.test.ts(1 hunks)tests/api.e2e.test.tsx(1 hunks)tests/config.e2e.test.tsx(1 hunks)tests/file-uploader-minimal.e2e.test.tsx(1 hunks)tests/file-uploader-regular.e2e.test.tsx(1 hunks)tests/validation.e2e.test.tsx(1 hunks)vite.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/blocks/FileItem/FileItem.ts
- src/blocks/CloudImageEditor/src/lib/FocusVisible.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- vite.config.js
- src/utils/get-top-level-origin.test.ts
- src/abstract/managers/tests/ModalManager.test.ts
- package.json
- src/abstract/managers/tests/SecureUploadsManager.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
tests/file-uploader-minimal.e2e.test.tsx (1)
tests/utils/commands.ts (1)
commands(16-18)
🪛 GitHub Check: CodeQL
tests/file-uploader-minimal.e2e.test.tsx
[notice] 4-4: Unused variable, import, function or class
Unused import fi.
[notice] 6-6: Unused variable, import, function or class
Unused import renderer.
🔇 Additional comments (5)
src/blocks/Config/initialConfig.ts (1)
90-90: LGTM! Environment-aware quality insights configuration.The change from a hardcoded
truetoimport.meta.env.PRODmakes quality insights environment-aware, enabling it only in production builds while keeping it disabled during development and testing. This aligns with the test strategy across the PR where tests explicitly setqualityInsights={false}.tests/file-uploader-regular.e2e.test.tsx (1)
20-20: LGTM! Consistent test configuration.Explicitly disabling quality insights in the test environment is appropriate and aligns with the environment-aware configuration change in
initialConfig.tsand the consistent pattern across all test files in this PR.tests/validation.e2e.test.tsx (1)
23-23: LGTM! Consistent test configuration.The addition of
qualityInsights={false}maintains consistency with the test configuration pattern established across all test files in this PR.tests/api.e2e.test.tsx (1)
21-21: LGTM! Consistent test configuration.The test configuration aligns with the pattern established across the PR for explicitly disabling quality insights in test environments.
tests/config.e2e.test.tsx (1)
20-20: LGTM! Consistent test configuration.The test setup maintains the consistent pattern of explicitly disabling quality insights in test environments.
| import { commands, page, userEvent } from '@vitest/browser/context'; | ||
| import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; | ||
| import '../types/jsx'; | ||
| import fi from '@/locales/file-uploader/fi'; |
There was a problem hiding this comment.
Remove unused import.
The fi locale import is unused in this file.
Apply this diff:
-import fi from '@/locales/file-uploader/fi';📝 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.
| import fi from '@/locales/file-uploader/fi'; |
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 4-4: Unused variable, import, function or class
Unused import fi.
🤖 Prompt for AI Agents
In tests/file-uploader-minimal.e2e.test.tsx around line 4, there is an unused
import "import fi from '@/locales/file-uploader/fi';" which should be removed;
delete that import statement to eliminate the unused import and ensure no other
references to `fi` remain in the file.
| it('should drag and drop file', async () => { | ||
| const fileUploader = page.getByTestId('uc-file-uploader-minimal'); | ||
| const copyright = page.getByText('Powered by Uploadcare', { exact: true }); | ||
|
|
||
| const uploadList = page.getByTestId('uc-upload-list'); | ||
|
|
||
| await userEvent.dragAndDrop(copyright, fileUploader); | ||
|
|
||
| await expect.element(uploadList).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for drag/drop event handlers in the minimal uploader implementation
rg -nP -A5 -B5 '\b(drag|drop)\b' --type=ts --glob='**/file-uploader-minimal/**' --glob='**/FileItem/**'Repository: uploadcare/file-uploader
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Find files related to file-uploader-minimal
find . -type f -name '*file-uploader-minimal*' | head -20Repository: uploadcare/file-uploader
Length of output: 110
🏁 Script executed:
#!/bin/bash
# List the repository structure to understand the layout
ls -laRepository: uploadcare/file-uploader
Length of output: 2406
🏁 Script executed:
#!/bin/bash
# Search for the test file
find . -type f -name '*file-uploader-minimal.e2e.test.tsx'Repository: uploadcare/file-uploader
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Read the test file to understand the context
cat -n ./tests/file-uploader-minimal.e2e.test.tsxRepository: uploadcare/file-uploader
Length of output: 3058
🏁 Script executed:
#!/bin/bash
# Find components in src related to file-uploader-minimal
find ./src -type f \( -name '*minimal*' -o -name '*file-uploader*' \) | head -20Repository: uploadcare/file-uploader
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Search for drag/drop handlers broadly in the codebase
rg -n 'drag|drop' --type=ts --type=tsx | head -50Repository: uploadcare/file-uploader
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Search for drag/drop handlers more broadly
rg -n 'drag|drop' --type=ts | head -50Repository: uploadcare/file-uploader
Length of output: 28595
🏁 Script executed:
#!/bin/bash
# Find the minimal uploader implementation
find ./src -type d -name '*minimal*'
find ./src -path '*file-uploader*' -name '*.ts' -o -path '*file-uploader*' -name '*.tsx'Repository: uploadcare/file-uploader
Length of output: 426
🏁 Script executed:
#!/bin/bash
# Look at the solutions directory structure
ls -la ./src/solutions/file-uploader/ 2>/dev/null || echo "Directory not found"Repository: uploadcare/file-uploader
Length of output: 343
🏁 Script executed:
#!/bin/bash
# Read the drop area implementation
cat -n ./src/blocks/DropArea/addDropzone.tsRepository: uploadcare/file-uploader
Length of output: 5223
🏁 Script executed:
#!/bin/bash
# Read the getDropItems implementation
cat -n ./src/blocks/DropArea/getDropItems.tsRepository: uploadcare/file-uploader
Length of output: 5238
🏁 Script executed:
#!/bin/bash
# Search for any special handling of drop events or dataTransfer that might accept plain text
rg -n 'text/plain' --type=tsRepository: uploadcare/file-uploader
Length of output: 995
🏁 Script executed:
#!/bin/bash
# Check if there's any other drag/drop handling besides getDropItems
rg -n 'dataTransfer' --type=ts | head -30Repository: uploadcare/file-uploader
Length of output: 1198
🏁 Script executed:
#!/bin/bash
# Find where onItems is called and what happens with the items
rg -n 'onItems' --type=ts -A 5 -B 5 | head -80Repository: uploadcare/file-uploader
Length of output: 2999
Replace test to drag actual file data, not arbitrary text elements.
The test drags a text element ("Powered by Uploadcare") which produces text/plain drag data. However, the uploader's drop handler only processes items where kind === 'file' or kind === 'string' with type matching text/uri-list (see getDropItems.ts lines 101, 135). When neither condition is met, an empty array is returned, causing the onItems handler to exit early without processing any upload (see DropArea.ts line 107-108). The upload list becomes visible for reasons unrelated to drag-and-drop file handling. Drag an actual file or file-like object to properly test file upload via drag-and-drop.
🤖 Prompt for AI Agents
In tests/file-uploader-minimal.e2e.test.tsx around lines 38 to 47, the test
currently drags a text node ("Powered by Uploadcare") which yields text/plain
drag data and is ignored by the uploader; replace that with a real file upload
simulation by creating a File/Blob (e.g., new File([...], 'test.png', { type:
'image/png' })) and use a proper file-oriented API to simulate the drop — either
call the file input's setInputFiles / setFiles helper on the uploader's hidden
input or call userEvent.dragAndDrop with a dataTransfer that includes files:
[file] so the drop produces items with kind === 'file' and the uploader
processes the upload.
05fb135 to
04aaadf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/file-uploader-minimal.e2e.test.tsx (2)
4-4: Remove unused fi locale import
fiis imported but never used in this test; it just adds noise and triggers static analysis warnings.You can safely drop it:
-import fi from '@/locales/file-uploader/fi';
38-47: Drag-and-drop test is not actually dropping a fileThis test currently drags the “Powered by Uploadcare” text node:
const fileUploader = page.getByTestId('uc-file-uploader-minimal'); const copyright = page.getByText('Powered by Uploadcare', { exact: true }); await userEvent.dragAndDrop(copyright, fileUploader);That produces
text/plaindrag data rather than aFile/text/uri-listitem, so the uploader’s drop logic won’t treat it as a real file drop. The assertion that the upload list becomes visible is therefore not validating the drag‑and‑drop upload path.Consider rewriting this test to simulate an actual file drop, e.g. by constructing a
Fileand ensuring it’s present in the dropDataTransfer, or by reusing existing helpers that already cover file uploads, so the test truly exercises DnD behavior.
🧹 Nitpick comments (2)
vite.config.js (1)
20-31: Confirm coverage patterns and thresholds match desired scopeThe new coverage config is generally solid, but two quick checks might be worthwhile:
include: ['src/**/*.ts']skips any.tsxsources; if you have React/JSX components insrc, consider extending this to['src/**/*.{ts,tsx}']so they count toward coverage.- The thresholds (vars 85, funcs 85, branches 80, lines 85) will make coverage gating stricter; confirm the current suite can consistently meet these numbers to avoid noisy failures.
tests/file-uploader-minimal.e2e.test.tsx (1)
54-56: Verify whether waitFileChooserAndUpload should be awaited
commands.waitFileChooserAndUpload(['./fixtures/test_image.jpeg']);is called withoutawait. If this helper is async (it probably waits for the file chooser and uploads a file), the test may race and click “From device” before the upload wiring is ready.If
waitFileChooserAndUploadreturns aPromise, consider:- commands.waitFileChooserAndUpload(['./fixtures/test_image.jpeg']); + await commands.waitFileChooserAndUpload(['./fixtures/test_image.jpeg']);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
package.json(1 hunks)src/abstract/managers/__tests__/ModalManager.test.ts(1 hunks)src/abstract/managers/__tests__/SecureUploadsManager.test.ts(1 hunks)src/blocks/CloudImageEditor/src/lib/FocusVisible.ts(1 hunks)src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts(1 hunks)src/blocks/FileItem/FileItem.ts(1 hunks)src/utils/get-top-level-origin.test.ts(1 hunks)tests/api.e2e.test.tsx(1 hunks)tests/config.e2e.test.tsx(1 hunks)tests/file-uploader-minimal.e2e.test.tsx(1 hunks)tests/file-uploader-regular.e2e.test.tsx(1 hunks)tests/validation.e2e.test.tsx(1 hunks)vite.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/api.e2e.test.tsx
- package.json
- src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts
- src/blocks/CloudImageEditor/src/lib/FocusVisible.ts
- src/blocks/FileItem/FileItem.ts
- tests/file-uploader-regular.e2e.test.tsx
- src/utils/get-top-level-origin.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
tests/file-uploader-minimal.e2e.test.tsx (1)
tests/utils/commands.ts (1)
commands(16-18)
🪛 GitHub Check: CodeQL
tests/file-uploader-minimal.e2e.test.tsx
[notice] 4-4: Unused variable, import, function or class
Unused import fi.
[notice] 6-6: Unused variable, import, function or class
Unused import renderer.
⏰ 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 (4)
tests/validation.e2e.test.tsx (1)
20-25: qualityInsights flag in test setup looks goodAdding
qualityInsights={false}to<uc-config>in the e2e setup is consistent with the rest of the suite and should help keep tests deterministic by disabling quality insights side effects.tests/config.e2e.test.tsx (1)
18-22: Consistent config for qualityInsights in e2e testsAdding
qualityInsights={false}to<uc-config>here keeps this setup aligned with the other e2e tests and should avoid side effects from quality insights during config tests.src/abstract/managers/__tests__/ModalManager.test.ts (1)
1-470: Comprehensive ModalManager test coverageThis suite gives very thorough coverage of ModalManager: registration, lifecycle operations, back/closeAll semantics, active state tracking, subscription management (incl. error + telemetry paths), destroy behavior, and ModalEvents constants. The mocks and expectations all look consistent with a robust modal manager contract.
src/abstract/managers/__tests__/SecureUploadsManager.test.ts (1)
1-362: SecureUploadsManager tests thoroughly cover config and resolver flowsThe test suite does a good job specifying behavior for static secure config, resolver-based tokens (including caching, expiry thresholds, and precedence), and error/invalid-result handling with appropriate debug/telemetry logging. Timer usage and console spies are correctly set up and cleaned up.
egordidenko
force-pushed
the
test/coverage-80
branch
from
04aaadf to
dad1f1d
Compare
| @@ -0,0 +1,76 @@ | |||
| import { page, userEvent } from '@vitest/browser/context'; | |||
| import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
To fix the problem, we should remove vi from the destructured import statement on line 2. The change is simple: update the import line to remove vi, leaving the rest of the imports untouched. No other changes are necessary, as nothing in the code references vi. Only one region of the file needs to be changed.
| @@ -1,5 +1,5 @@ | ||
| import { page, userEvent } from '@vitest/browser/context'; | ||
| import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; | ||
| import '../types/jsx'; | ||
|
|
||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX |
| import '../types/jsx'; | ||
|
|
||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { renderer } from './utils/test-renderer'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
To fix the issue, we should remove the unused import statement import { renderer } from './utils/test-renderer'; from tests/cloud-image-editor.e2e.test.tsx. This means deleting both the import line and the comment immediately above it (// biome-ignore lint/correctness/noUnusedImports: Used in JSX), as neither is functionally required—no code in this file references renderer either directly or via JSX. No other code changes or substitutions are necessary.
| @@ -2,8 +2,6 @@ | ||
| import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import '../types/jsx'; | ||
|
|
||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { renderer } from './utils/test-renderer'; | ||
|
|
||
| beforeAll(async () => { | ||
| // biome-ignore lint/suspicious/noTsIgnore: Ignoring TypeScript error for CSS import |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/file-uploader-minimal.e2e.test.tsx (2)
4-4: Remove unusedfilocale import (or wire it into the config).
fiisn’t used anywhere in this test. Either remove the import, or, if you intend to test Finnish localization, pass it into<uc-config>(e.g.,locale={fi}) so it’s actually exercised.-import fi from '@/locales/file-uploader/fi';
38-47: Drag-and-drop test isn’t dropping a real file.This test drags the “Powered by Uploadcare” text node, which yields plain text drag data. The drop pipeline for the uploader only processes real files or supported URL types, so this doesn’t actually verify file DnD behavior—it only asserts that the upload list becomes visible for other reasons.
Consider changing the test to simulate a real file drop (e.g., via a
Fileobject in the drag data or by using your existingwaitFileChooserAndUpload/ file chooser helpers) so it exercises the same path actual users hit.
🧹 Nitpick comments (3)
vite.config.js (1)
20-31: Confirm whether coverage should also include TSX/JS sources
test.coverage.includeis limited tosrc/**/*.ts, so any logic in.tsxor.jsundersrcwon’t count toward the configured thresholds. If you also want UI/JS modules included, consider broadening the pattern, for example:- include: ['src/**/*.ts'], + include: ['src/**/*.{ts,tsx}'],(and adjust
excludesimilarly if needed).src/blocks/FileItem/FileItem.ts (1)
476-482: Consider making the edit button test id more specific
data-testid="edit"will be duplicated across multipleFileIteminstances; that’s fine functionally but can make selectors ambiguous in e2e tests. If you start relying heavily on this, consider a more specific id (e.g.file-item-edit) or using scopedgetByTestId/getAllByTestIdin tests.tests/cloud-image-editor.e2e.test.tsx (1)
35-39: Add an assertion to the “should select tunings tab” test.This test only clicks the control and never asserts resulting state, so it currently just verifies that the click doesn’t throw. Consider asserting the expected outcome (e.g., active tab, aria-selected, or visible tuning panel) to make the test meaningful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
package.json(1 hunks)src/abstract/managers/__tests__/ModalManager.test.ts(1 hunks)src/abstract/managers/__tests__/SecureUploadsManager.test.ts(1 hunks)src/blocks/CloudImageEditor/src/EditorToolbar.ts(1 hunks)src/blocks/CloudImageEditor/src/crop-utils.test.ts(1 hunks)src/blocks/CloudImageEditor/src/lib/FocusVisible.ts(1 hunks)src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts(1 hunks)src/blocks/FileItem/FileItem.ts(1 hunks)src/utils/get-top-level-origin.test.ts(1 hunks)tests/api.e2e.test.tsx(1 hunks)tests/cloud-image-editor.e2e.test.tsx(1 hunks)tests/config.e2e.test.tsx(1 hunks)tests/file-uploader-minimal.e2e.test.tsx(1 hunks)tests/file-uploader-regular.e2e.test.tsx(1 hunks)tests/validation.e2e.test.tsx(1 hunks)vite.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/api.e2e.test.tsx
- package.json
- src/utils/get-top-level-origin.test.ts
- src/abstract/managers/tests/ModalManager.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
tests/file-uploader-minimal.e2e.test.tsx (1)
tests/utils/commands.ts (1)
commands(16-18)
🪛 GitHub Check: CodeQL
tests/cloud-image-editor.e2e.test.tsx
[notice] 2-2: Unused variable, import, function or class
Unused import vi.
[notice] 6-6: Unused variable, import, function or class
Unused import renderer.
tests/file-uploader-minimal.e2e.test.tsx
[notice] 4-4: Unused variable, import, function or class
Unused import fi.
[notice] 6-6: Unused variable, import, function or class
Unused import renderer.
⏰ 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 (6)
src/abstract/managers/__tests__/SecureUploadsManager.test.ts (1)
1-362: SecureUploadsManager test suite looks comprehensive and well-structuredThe suite exercises static config, resolver flows (including caching, expiry, threshold behavior), error handling, and debug/telemetry side effects with good use of fake timers and spies; I don’t see gaps or flakiness risks in the current scenarios.
tests/validation.e2e.test.tsx (1)
18-27: Disabling qualityInsights in test config is a sensible defaultPassing
qualityInsights={false}in the shareduc-configsetup should help keep these validation e2e tests deterministic and free from any extra quality/insights side-effects without impacting the scenarios under test.src/blocks/CloudImageEditor/src/EditorToolbar.ts (1)
504-505: Slider controls now have consistent title metadataAdding
title-prop:cancel/title-prop:applyto the slider buttons brings them in line with the main toolbar buttons’ a11y/tooltip configuration without changing behavior.tests/file-uploader-regular.e2e.test.tsx (1)
15-23: uc-config setup with qualityInsights={false} is consistent with other e2e testsUpdating this test’s
uc-configto includequalityInsights={false}aligns it with the rest of the suite and should avoid any incidental insight-related behavior during runs.tests/config.e2e.test.tsx (1)
15-24: Explicitly disabling qualityInsights keeps config tests focusedAdding
qualityInsights={false}to the shareduc-confighere helps ensure these tests exercise only configuration behavior (e.g.,cdnCname) without being affected by quality insights logic.src/blocks/CloudImageEditor/src/crop-utils.test.ts (1)
1-719: crop-utils tests provide strong coverage of geometry and SVG helpersThis suite exercises the key code paths (SVG node helpers, cursor mapping, move/resize with constraints and aspect ratios, centered crop computation, rounding/clamping) with good edge-case coverage; the few exact-value expectations (e.g., specific 1:1 crop frame) are reasonable but will need revisiting if the underlying math is ever adjusted.
| /** | ||
| * This module is deprecated and will be removed in future versions. | ||
| * @deprecated | ||
| */ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "src/blocks/CloudImageEditor/src/lib/FocusVisible.ts"Repository: uploadcare/file-uploader
Length of output: 1431
🏁 Script executed:
rg -n "FocusVisible" --type=ts --type=js -B2 -A2Repository: uploadcare/file-uploader
Length of output: 7525
🏁 Script executed:
cat -n "src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts" 2>/dev/null || echo "File not found"Repository: uploadcare/file-uploader
Length of output: 7472
🏁 Script executed:
fd "FocusVisible" --type=fRepository: uploadcare/file-uploader
Length of output: 184
Deprecation notice needs migration guidance and accurate context.
The deprecation header marks this module for removal but doesn't provide:
- A reason for deprecation
- An alternative or migration path
- A target version for removal
Before enhancing this notice, clarify the actual deprecation rationale. The module's purpose extends beyond providing a :focus-visible selector—it applies custom outline styling based on focus visibility state, which requires more nuanced migration guidance than simply using native CSS :focus-visible. Document what developers should use instead and include a target removal version.
| @@ -0,0 +1,76 @@ | |||
| import { page, userEvent } from '@vitest/browser/context'; | |||
| import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; | |||
There was a problem hiding this comment.
Remove unused vi import.
vi is not used anywhere in this file, and there is no ignore directive for it. Dropping it will satisfy static analysis and keep imports clean.
-import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest';
+import { beforeAll, beforeEach, describe, expect, it } from 'vitest';📝 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.
| import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; | |
| import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; |
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 2-2: Unused variable, import, function or class
Unused import vi.
🤖 Prompt for AI Agents
In tests/cloud-image-editor.e2e.test.tsx around line 2, the imported symbol `vi`
is unused; remove `vi` from the import list (i.e., import only beforeAll,
beforeEach, describe, expect, it from 'vitest') to satisfy static analysis and
keep imports clean.
dad1f1d to
632f528
Compare
| @@ -0,0 +1,68 @@ | |||
| import { commands, page, userEvent } from '@vitest/browser/context'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
The best way to fix the problem is to remove the unused import commands from the import statement on line 1 in tests/file-uploader-inline.e2e.test.tsx. This change affects only the import statement itself — the rest of the code uses page and userEvent, so those should remain. No other changes are needed in the file, and no additional imports, methods, or definitions are required.
| @@ -1,4 +1,4 @@ | ||
| import { commands, page, userEvent } from '@vitest/browser/context'; | ||
| import { page, userEvent } from '@vitest/browser/context'; | ||
| import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; | ||
| import '../types/jsx'; | ||
| import { delay } from '@/utils/delay'; |
| import '../types/jsx'; | ||
| import { delay } from '@/utils/delay'; | ||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { renderer } from './utils/test-renderer'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
To fix this problem, you should remove the unused import statement from line 6 of tests/file-uploader-inline.e2e.test.tsx. Specifically, delete the line import { renderer } from './utils/test-renderer';. No other changes are required in the file, and no imports or code edits elsewhere are necessary, as the identifier is unused.
| @@ -3,7 +3,6 @@ | ||
| import '../types/jsx'; | ||
| import { delay } from '@/utils/delay'; | ||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { renderer } from './utils/test-renderer'; | ||
|
|
||
| beforeAll(async () => { | ||
| // biome-ignore lint/suspicious/noTsIgnore: Ignoring TypeScript error for CSS import |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
tests/cloud-image-editor.e2e.test.tsx (2)
2-2: Remove unusedviimport.The
viimport is not used anywhere in this file.Apply this diff:
-import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import { beforeAll, beforeEach, describe, expect, it } from 'vitest';
5-6: Remove unusedrendererimport despite biome-ignore comment.The biome-ignore comment claims
rendereris used in JSX, but the code only uses native JSX elements (uc-cloud-image-editoranduc-config), not therendererutility. This import should be removed along with its ignore comment.Apply this diff:
-// biome-ignore lint/correctness/noUnusedImports: Used in JSX -import { renderer } from './utils/test-renderer'; -
🧹 Nitpick comments (3)
src/blocks/CameraSource/__tests__/calcCameraModes.test.ts (1)
43-45: Align mixed-values test config shape with other casesIn the “mixed valid and invalid values” case you construct a minimal config via a type assertion:
const cfg = { cameraModes: 'video,unknown,photo' } as ConfigType;This works today, but it’s slightly brittle if
calcCameraModesever starts depending on additional fields fromConfigType. It would also be more consistent with the other tests to base it oninitialConfig.Consider:
- it('should handle mixed valid and invalid values', () => { - const cfg = { cameraModes: 'video,unknown,photo' } as ConfigType; + it('should handle mixed valid and invalid values', () => { + const cfg = { ...initialConfig, cameraModes: 'video,unknown,photo' } as ConfigType;This keeps the full config shape while still clearly focusing the test on the
cameraModesfield.src/blocks/CloudImageEditor/src/crop-utils.test.ts (1)
68-152: Consider verifying actual path coordinates, not just structure.The
cornerPathandsidePathtests only check that the result contains 'M' and 'L' commands and that the center has length 2. They don't verify that the paths are positioned correctly or that center coordinates are accurate. These tests would pass even if the paths were completely wrong.Consider adding assertions like:
it('should generate path for northwest corner', () => { const result = cornerPath(rect, 'nw', 1); expect(result.d).toContain('M'); expect(result.d).toContain('L'); expect(result.center).toHaveLength(2); + // Verify center is at the northwest corner + expect(result.center[0]).toBe(rect.x); + expect(result.center[1]).toBe(rect.y); });tests/file-uploader-inline.e2e.test.tsx (1)
34-34: Use a more robust assertion.
toBeDefined()only verifies the selector returned a value, not that the element is actually visible or in the DOM. Consider usingtoBeVisible()ortoBeInTheDocument()for stronger validation.Apply this diff:
- await expect(urlSource).toBeDefined(); + await expect(urlSource).toBeVisible();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
src/abstract/__tests__/loadFileUploaderFrom.test.ts(1 hunks)src/blocks/CameraSource/CameraSource.ts(5 hunks)src/blocks/CameraSource/__tests__/calcCameraModes.test.ts(1 hunks)src/blocks/CloudImageEditor/src/EditorToolbar.ts(1 hunks)src/blocks/CloudImageEditor/src/crop-utils.test.ts(1 hunks)src/blocks/CloudImageEditor/src/lib/FocusVisible.ts(0 hunks)src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts(0 hunks)tests/cloud-image-editor.e2e.test.tsx(1 hunks)tests/file-uploader-inline.e2e.test.tsx(1 hunks)tests/validation.e2e.test.tsx(8 hunks)
💤 Files with no reviewable changes (2)
- src/blocks/CloudImageEditor/src/lib/FocusVisible.ts
- src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/validation.e2e.test.tsx
- src/blocks/CloudImageEditor/src/EditorToolbar.ts
🧰 Additional context used
🪛 GitHub Check: CodeQL
tests/file-uploader-inline.e2e.test.tsx
[notice] 1-1: Unused variable, import, function or class
Unused import commands.
[notice] 6-6: Unused variable, import, function or class
Unused import renderer.
tests/cloud-image-editor.e2e.test.tsx
[notice] 2-2: Unused variable, import, function or class
Unused import vi.
[notice] 6-6: Unused variable, import, function or class
Unused import renderer.
⏰ 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 (14)
src/blocks/CameraSource/CameraSource.ts (1)
1031-1095: Template data-testid additions look good and non-invasiveThe added
data-testidattributes on camera tabs, recording timer, retake, microphone toggle, and audio select are purely additive and don’t interfere with existingsetbindings or refs. They provide stable hooks for tests and are appropriately scoped to the relevant controls.src/blocks/CameraSource/__tests__/calcCameraModes.test.ts (1)
6-51: Good coverage of cameraModes behavior spaceThe suite cleanly exercises default, single-mode, empty, and mixed cameraModes configurations and asserts both
isVideoRecordingEnabledandisPhotoEnabled, which should give solid regression coverage aroundcalcCameraModes.src/blocks/CloudImageEditor/src/crop-utils.test.ts (6)
22-66: LGTM!The tests for
setSvgNodeAttrsandcreateSvgNodeprovide adequate coverage for these basic SVG utility functions, verifying attribute setting for both strings and numbers.
154-190: LGTM!Comprehensive coverage of all cursor types for different resize directions.
192-232: LGTM!Good coverage of rectangle movement with proper boundary constraint testing in all directions.
234-272: LGTM!Thorough testing of rectangle constraint logic with all boundary cases covered.
274-441: Excellent test coverage for resize functionality!The
resizeRecttest suite is very thorough, covering all resize directions, aspect ratio preservation, boundary constraints, and minimum size enforcement. This comprehensive approach ensures the resize logic is well-tested.
443-718: LGTM!The remaining test suites (
rectContainsPoint,isRectInsideRect,isRectMatchesAspectRatio,rotateSize,calculateMaxCenteredCropFrame,roundRect, andclamp) all have comprehensive coverage with proper edge case and boundary condition testing.tests/cloud-image-editor.e2e.test.tsx (1)
8-76: LGTM!The e2e test suite is well-structured with proper setup in
beforeAllandbeforeEach, and the test cases effectively cover key editor functionality: rendering, tab switching, crop preset selection, and brightness operations. Good use of semantic selectors withgetByRoleandgetByTestId.src/abstract/__tests__/loadFileUploaderFrom.test.ts (1)
24-25: Top-level await in describe blocks is supported in Vitest 3.2.4.This pattern works correctly in the current Vitest version (^3.2.4). However, consider using
beforeAllorbeforeEachinstead for a more conventional test structure that better aligns with standard testing patterns.tests/file-uploader-inline.e2e.test.tsx (4)
8-14: LGTM!The beforeAll setup correctly imports the required CSS and defines the components for the inline uploader tests.
16-24: LGTM!The beforeEach setup correctly creates a unique context for each test and properly configures the inline uploader with test mode enabled.
27-29: LGTM!The rendering verification test correctly asserts the presence and visibility of the expected start area text.
54-55: The delay duration is appropriate for recording simulation.The 2-second delay is reasonable for testing the microphone recording functionality in an e2e test, even though it adds to test execution time.
| afterEach(() => { | ||
| const scripts = document.querySelectorAll('script[src]'); | ||
| for (const script of scripts) { | ||
| script.remove(); | ||
| } |
There was a problem hiding this comment.
Overly broad script cleanup risks test pollution.
The cleanup removes ALL script elements with a src attribute, not just those created by these tests. This could inadvertently remove scripts injected by the test framework, other concurrent tests, or test infrastructure.
Apply this diff to target only the scripts created by these tests:
afterEach(() => {
- const scripts = document.querySelectorAll('script[src]');
+ const scripts = document.querySelectorAll(`script[src="${testUrl}"]`);
for (const script of scripts) {
script.remove();
}
delete (window as any)[UC_WINDOW_KEY];
});However, this requires testUrl to be accessible in the afterEach scope. Consider moving the import to module level or tracking created scripts explicitly:
let createdScripts: HTMLScriptElement[] = [];
afterEach(() => {
createdScripts.forEach(script => script.remove());
createdScripts = [];
delete (window as any)[UC_WINDOW_KEY];
});
// In each test, after creating a script:
const script = document.querySelector(`script[src="${testUrl}"]`) as HTMLScriptElement;
if (script) createdScripts.push(script);🤖 Prompt for AI Agents
In src/abstract/__tests__/loadFileUploaderFrom.test.ts around lines 15-19, the
afterEach currently removes all script[src] tags which can delete unrelated
test/framework scripts; instead, track and remove only scripts this test creates
(e.g., push created HTMLScriptElement instances into a module-scoped
createdScripts array and in afterEach iterate over createdScripts to remove them
and reset the array), and ensure UC_WINDOW_KEY cleanup is done there; if testUrl
is needed in afterEach, make it module-scoped (move its import/definition to
top-level) or otherwise capture the created script references at creation time
so afterEach can remove only those specific elements.
| it('should reject on script error', async () => { | ||
| const loadPromise = loadFileUploaderFrom(testUrl); | ||
|
|
||
| const script = document.querySelector(`script[src="${testUrl}"]`) as HTMLScriptElement; | ||
| script.onerror?.(new Event('error')); | ||
|
|
||
| await expect(loadPromise).rejects.toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
Error rejection with undefined is poor practice.
The test expects the promise to reject with undefined, which provides no error information for debugging. Errors should typically reject with an Error object or descriptive message.
While this test accurately reflects the current implementation, consider updating the implementation to reject with a meaningful error:
// In loadFileUploaderFrom implementation:
script.onerror = () => {
reject(new Error(`Failed to load file uploader from ${url}`));
};Then update the test:
-await expect(loadPromise).rejects.toBeUndefined();
+await expect(loadPromise).rejects.toThrow('Failed to load file uploader');🤖 Prompt for AI Agents
In src/abstract/__tests__/loadFileUploaderFrom.test.ts around lines 125 to 132,
the test currently expects the load promise to reject with undefined; change the
implementation of loadFileUploaderFrom so script.onerror rejects with a
descriptive Error (e.g. new Error(`Failed to load file uploader from ${url}`)),
and update this test to assert the promise rejects with that Error or at least
with an instance of Error (e.g. use toRejectWith or toThrow matching the message
or Error constructor) instead of toBeUndefined.
| describe('non-browser environment', () => { | ||
| it('should resolve with null when document is not an object', async () => { | ||
| const originalDocument = global.document; | ||
|
|
||
| // @ts-expect-error - intentionally setting to non-object for test | ||
| global.document = undefined; | ||
|
|
||
| const result = await loadFileUploaderFrom(testUrl); | ||
|
|
||
| expect(result).toBeNull(); | ||
|
|
||
| global.document = originalDocument; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Non-browser environment test may be fragile.
Manipulating global.document may not work correctly in all test environments. In browser-like environments (jsdom, happy-dom) or real browsers, document may not be accessible via global and could be a non-configurable property.
Consider using a more robust approach with proper environment detection:
it('should resolve with null when document is not an object', async () => {
// Create a minimal test module that can be imported with document stubbed
// Or mock the document check at a higher level
// Alternative: Test the actual condition from loadFileUploaderFrom
const originalTypeOf = global.Object.prototype.toString;
vi.spyOn(Object.prototype, 'toString').mockReturnValueOnce('[object Undefined]');
// ... test logic
vi.restoreAllMocks();
});Alternatively, verify the test actually executes correctly by adding an assertion that the mock succeeded:
// @ts-expect-error - intentionally setting to non-object for test
global.document = undefined;
+ expect(typeof document).not.toBe('object'); // Verify mock worked
const result = await loadFileUploaderFrom(testUrl);🤖 Prompt for AI Agents
In src/abstract/__tests__/loadFileUploaderFrom.test.ts around lines 135 to 148,
the test mutates global.document which is fragile across environments
(jsdom/happy-dom/real browsers) because document may not be configurable;
instead, change the test to simulate the "document is not an object" condition
without redefining global.document — for example, spy/mocking the runtime check
used in loadFileUploaderFrom (mock Object.prototype.toString or the specific
helper that checks for document) to return an undefined-like result for the
duration of the test, assert the mock was applied, run loadFileUploaderFrom
expecting null, then restore all mocks; alternatively create a minimal test
module import where document is stubbed and import that module to exercise the
condition.
| @@ -0,0 +1,68 @@ | |||
| import { commands, page, userEvent } from '@vitest/browser/context'; | |||
There was a problem hiding this comment.
Remove unused imports.
The imports commands and renderer are not used in this file.
Apply this diff to remove the unused imports:
-import { commands, page, userEvent } from '@vitest/browser/context';
+import { page, userEvent } from '@vitest/browser/context';
import { beforeAll, beforeEach, describe, expect, it } from 'vitest';
import '../types/jsx';
import { delay } from '@/utils/delay';
-// biome-ignore lint/correctness/noUnusedImports: Used in JSX
-import { renderer } from './utils/test-renderer';Also applies to: 6-6
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 1-1: Unused variable, import, function or class
Unused import commands.
🤖 Prompt for AI Agents
In tests/file-uploader-inline.e2e.test.tsx around lines 1 and 6, remove the
unused imports `commands` and `renderer` from the import statements so only the
used identifiers (e.g., `page`, `userEvent`) remain; update the import lines
accordingly to eliminate the unused bindings and any trailing commas.
| const recordingTimer = cameraSource.getByTestId('uc-camera-source--recording-timer'); | ||
| await expect(recordingTimer).toBeVisible(); | ||
|
|
||
| const accptButton = cameraSource.getByTestId('uc-camera-source--accept'); |
There was a problem hiding this comment.
Fix typo in variable name.
The variable name accptButton should be acceptButton.
Apply this diff:
- const accptButton = cameraSource.getByTestId('uc-camera-source--accept');
+ const acceptButton = cameraSource.getByTestId('uc-camera-source--accept');And update the usage:
- await userEvent.click(accptButton);
+ await userEvent.click(acceptButton);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/file-uploader-inline.e2e.test.tsx around line 62, the variable name
`accptButton` is misspelled; rename it to `acceptButton` and update all usages
accordingly (e.g., any subsequent references or assertions that use
`accptButton` should be changed to `acceptButton`) so the test compiles and
reads correctly.
632f528 to
401928a
Compare
| @@ -0,0 +1,23 @@ | |||
| import { commands, page, userEvent } from '@vitest/browser/context'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
The best way to fix the problem is to remove the unused imports from line 1, while keeping the used page import. This does not affect any functionality as the variables commands and userEvent are not referenced in this file. The only change needed is to edit the import statement on line 1 in tests/adaptive-image.e2e.test.tsx, removing these two unused imports.
| @@ -1,4 +1,4 @@ | ||
| import { commands, page, userEvent } from '@vitest/browser/context'; | ||
| import { page } from '@vitest/browser/context'; | ||
| import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; | ||
| import '../types/jsx'; | ||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX |
| @@ -0,0 +1,23 @@ | |||
| import { commands, page, userEvent } from '@vitest/browser/context'; | |||
| import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
To fix the problem, the unused import expect should be removed from the import statement on line 2. This can be done by editing the import to exclude expect, leaving only the imports that are used: beforeAll, beforeEach, describe, and it. This edit should be performed only in the file shown, and only the import statement needs to change; no other changes are necessary at this time given the provided snippet. No new methods, definitions, or imports are needed.
| @@ -1,5 +1,5 @@ | ||
| import { commands, page, userEvent } from '@vitest/browser/context'; | ||
| import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; | ||
| import { beforeAll, beforeEach, describe, it } from 'vitest'; | ||
| import '../types/jsx'; | ||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { renderer } from './utils/test-renderer'; |
| import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; | ||
| import '../types/jsx'; | ||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { renderer } from './utils/test-renderer'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
The best fix is to remove the unused import of renderer from ./utils/test-renderer. This is done by deleting the line:
import { renderer } from './utils/test-renderer';No other changes are needed since renderer is not referenced elsewhere in the file. Ensure that no side effects are performed by this import, which given the named import style, should be safe.
Only the single line (line 5) needs to be removed from tests/adaptive-image.e2e.test.tsx. No additional imports, definitions, methods, or dependencies are required.
| @@ -2,7 +2,6 @@ | ||
| import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; | ||
| import '../types/jsx'; | ||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { renderer } from './utils/test-renderer'; | ||
|
|
||
| beforeAll(async () => { | ||
| await import('@/solutions/adaptive-image/index.js'); |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tests/cloud-image-editor.e2e.test.tsx (1)
2-2: Remove unused imports.The imports
viandrendererare not used in this file.Apply this diff:
-import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; import '../types/jsx'; -// biome-ignore lint/correctness/noUnusedImports: Used in JSX -import { renderer } from './utils/test-renderer'; - beforeAll(async () => {Also applies to: 6-6
tests/file-uploader-inline.e2e.test.tsx (2)
1-1: Remove unused imports.The imports
commandsandrendererare not used in this file.Apply this diff:
-import { commands, page, userEvent } from '@vitest/browser/context'; +import { page, userEvent } from '@vitest/browser/context'; import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; import '../types/jsx'; import { delay } from '@/utils/delay'; -// biome-ignore lint/correctness/noUnusedImports: Used in JSX -import { renderer } from './utils/test-renderer';Also applies to: 6-6
62-63: Fix typo in variable name.The variable name
accptButtonshould beacceptButton.Apply this diff:
- const accptButton = cameraSource.getByTestId('uc-camera-source--accept'); - await userEvent.click(accptButton); + const acceptButton = cameraSource.getByTestId('uc-camera-source--accept'); + await userEvent.click(acceptButton);
🧹 Nitpick comments (2)
src/blocks/CameraSource/__tests__/calcCameraModes.test.ts (2)
7-14: Avoid coupling the first test to implicit initialConfig defaultsThe description says “includes video and photo”, but the test relies on whatever
initialConfigcurrently sets forcameraModes. To make the intent future‑proof and self‑contained, consider setting it explicitly, e.g.:const cfg = { ...initialConfig, cameraModes: 'video,photo' } as ConfigType;This avoids accidental breakage if
initialConfig.cameraModeschanges later.
43-45: Align the mixed-values test config with real-world ConfigType shapeHere you construct
{ cameraModes: 'video,unknown,photo' } as ConfigType, whereas the other tests start frominitialConfig. For realism and to reduce the chance of calcCameraModes later depending on another config field, consider:const cfg = { ...initialConfig, cameraModes: 'video,unknown,photo' } as ConfigType;This keeps the test closer to how the function is used in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
src/abstract/__tests__/loadFileUploaderFrom.test.ts(1 hunks)src/blocks/CameraSource/CameraSource.ts(5 hunks)src/blocks/CameraSource/__tests__/calcCameraModes.test.ts(1 hunks)src/blocks/CloudImageEditor/src/EditorToolbar.ts(1 hunks)src/blocks/CloudImageEditor/src/crop-utils.test.ts(1 hunks)src/blocks/CloudImageEditor/src/lib/FocusVisible.ts(0 hunks)src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts(0 hunks)tests/adaptive-image.e2e.test.tsx(1 hunks)tests/cloud-image-editor.e2e.test.tsx(1 hunks)tests/file-uploader-inline.e2e.test.tsx(1 hunks)tests/validation.e2e.test.tsx(8 hunks)
💤 Files with no reviewable changes (2)
- src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts
- src/blocks/CloudImageEditor/src/lib/FocusVisible.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/abstract/tests/loadFileUploaderFrom.test.ts
- src/blocks/CloudImageEditor/src/crop-utils.test.ts
- src/blocks/CloudImageEditor/src/EditorToolbar.ts
- tests/validation.e2e.test.tsx
🧰 Additional context used
🪛 GitHub Check: CodeQL
tests/adaptive-image.e2e.test.tsx
[notice] 1-1: Unused variable, import, function or class
Unused imports commands, userEvent.
[notice] 2-2: Unused variable, import, function or class
Unused import expect.
[notice] 5-5: Unused variable, import, function or class
Unused import renderer.
tests/file-uploader-inline.e2e.test.tsx
[notice] 1-1: Unused variable, import, function or class
Unused import commands.
[notice] 6-6: Unused variable, import, function or class
Unused import renderer.
tests/cloud-image-editor.e2e.test.tsx
[notice] 2-2: Unused variable, import, function or class
Unused import vi.
[notice] 6-6: Unused variable, import, function or class
Unused import renderer.
⏰ 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 (3)
src/blocks/CameraSource/__tests__/calcCameraModes.test.ts (1)
1-51: Good coverage and clear expectations for calcCameraModesThe suite cleanly covers the main modes (video/photo/both/none/mixed) and uses explicit expectations for the two booleans. This should be solid for guarding regressions in calcCameraModes.
src/blocks/CameraSource/CameraSource.ts (2)
1026-1044: LGTM! Well-chosen test IDs for tab navigation.The
data-testidattributes for the photo and video tab buttons are descriptive and follow a consistent naming pattern, making them easy to select in automated tests.
1046-1097: LGTM! Comprehensive test coverage for camera controls.The added
data-testidattributes (recording-timer,retake,toggle-microphone,audio-select) provide solid hooks for testing all key camera interactions. The naming is clear and consistent across all elements.
| import { commands, page, userEvent } from '@vitest/browser/context'; | ||
| import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; | ||
| import '../types/jsx'; | ||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { renderer } from './utils/test-renderer'; | ||
|
|
There was a problem hiding this comment.
Remove unused imports.
The imports commands, userEvent, expect, and renderer are not used in this file.
Apply this diff to remove the unused imports:
-import { commands, page, userEvent } from '@vitest/browser/context';
-import { beforeAll, beforeEach, describe, expect, it } from 'vitest';
+import { page } from '@vitest/browser/context';
+import { beforeAll, beforeEach, describe, it } from 'vitest';
import '../types/jsx';
-// biome-ignore lint/correctness/noUnusedImports: Used in JSX
-import { renderer } from './utils/test-renderer';🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 1-1: Unused variable, import, function or class
Unused imports commands, userEvent.
[notice] 2-2: Unused variable, import, function or class
Unused import expect.
[notice] 5-5: Unused variable, import, function or class
Unused import renderer.
🤖 Prompt for AI Agents
In tests/adaptive-image.e2e.test.tsx around lines 1 to 6, several imports are
unused: remove `commands` and `userEvent` from the first import, remove `expect`
from the vitest import list (keep `beforeAll`, `beforeEach`, `describe`, `it`),
and delete the unused `import { renderer } from './utils/test-renderer';` line;
ensure the file still imports `page` and `../types/jsx` as needed and run
lint/tests to verify no remaining unused imports.
| it('should be rendered', async () => { | ||
| // await expect.element(page.getByTestId('uc-img')).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
Complete the test or remove the placeholder.
The test assertion is commented out, making this test non-functional. Either implement the test properly or remove the placeholder if it's not ready.
Apply this diff to enable the assertion:
it('should be rendered', async () => {
- // await expect.element(page.getByTestId('uc-img')).toBeVisible();
+ await expect.element(page.getByTestId('uc-img')).toBeVisible();
});Note: You'll need to restore the expect import if you uncomment this assertion.
📝 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.
| it('should be rendered', async () => { | |
| // await expect.element(page.getByTestId('uc-img')).toBeVisible(); | |
| }); | |
| it('should be rendered', async () => { | |
| await expect.element(page.getByTestId('uc-img')).toBeVisible(); | |
| }); |
🤖 Prompt for AI Agents
In tests/adaptive-image.e2e.test.tsx around lines 20 to 22, the test contains a
commented-out assertion which makes it a no-op; either restore and enable the
expectation or delete the placeholder test. Uncomment the expect assertion
calling page.getByTestId('uc-img') and toBeVisible(), and if necessary re-add
the expect import from the test runner at the top of the file so the assertion
works; alternatively, remove the entire it(...) block if this test isn't ready.
401928a to
546d03c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (10)
tests/file-uploader-inline.e2e.test.tsx (2)
1-1: Remove unused imports.The imports
commands(line 1) andrenderer(line 6) are not used anywhere in the test file.Also applies to: 6-6
62-63: Fix typo in variable name.The variable name
accptButtonshould beacceptButton.tests/adaptive-image.e2e.test.tsx (2)
1-6: Remove unused imports.The imports
commands,userEvent,expect, andrendererare not used in this file. These should be removed to satisfy static analysis.Apply this diff to remove the unused imports:
-import { commands, page, userEvent } from '@vitest/browser/context'; -import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { page } from '@vitest/browser/context'; +import { beforeAll, beforeEach, describe, it } from 'vitest'; import '../types/jsx'; -// biome-ignore lint/correctness/noUnusedImports: Used in JSX -import { renderer } from './utils/test-renderer';
19-23: Complete the test or remove the placeholder.The test assertion is commented out, making this test non-functional. Either implement the test properly or remove the placeholder if it's not ready.
Apply this diff to enable the assertion:
it('should be rendered', async () => { - // await expect.element(page.getByTestId('uc-img')).toBeVisible(); + await expect.element(page.getByTestId('uc-img')).toBeVisible(); });Note: You'll need to restore the
expectimport when uncommenting this assertion.tests/cloud-image-editor.e2e.test.tsx (2)
2-2: Remove unusedviimport.The
viimport is not used anywhere in this file. Remove it to satisfy static analysis.Apply this diff:
-import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import { beforeAll, beforeEach, describe, expect, it } from 'vitest';
30-30: Remove.skipto enable the test suite.The entire test suite is currently skipped with
describe.skip, which means none of these tests are running. This defeats the purpose of adding test coverage.Apply this diff to enable the tests:
-describe.skip('Cloud Image Editor', () => { +describe('Cloud Image Editor', () => {After enabling, verify all tests pass locally before pushing.
src/abstract/__tests__/loadFileUploaderFrom.test.ts (4)
135-148: Non‑browser test is fragile due to directglobal.documentreassignment.Overwriting
global.documentcan be brittle across environments (jsdom, happy-dom, real browser) and, if the test fails before the final line, may leavedocumentin a bad state for subsequent tests. Prefer simulating the “non-object document” condition via the same check used inloadFileUploaderFrom(e.g., mocking that helper or spying on the relevant property) and/or ensure restoration happens in atry/finally:- const originalDocument = global.document; - // @ts-expect-error - intentionally setting to non-object for test - global.document = undefined; - - const result = await loadFileUploaderFrom(testUrl); - - expect(result).toBeNull(); - - global.document = originalDocument; + const originalDocument = global.document; + try { + // @ts-expect-error - intentionally setting to non-object for test + global.document = undefined; + + const result = await loadFileUploaderFrom(testUrl); + expect(result).toBeNull(); + } finally { + global.document = originalDocument; + }This reduces the risk of leaking a broken
documentinto other tests.
15-22: Limit script cleanup to only scripts created by this test suite.
afterEachcurrently removes allscript[src]elements fromdocument, which can unintentionally delete scripts not created by these tests (e.g., shared setup, other helpers using script tags). Track and remove only the specific scripts you create (for example, by storing createdHTMLScriptElementinstances in a module-scoped array and cleaning those up here), instead of blanket-removing allscript[src]nodes.
125-132: Rejecting withundefinedis weak; prefer asserting a meaningful Error.This test encodes a rejection with
undefined, which makes failures harder to debug. It would be better forloadFileUploaderFromto reject with anError(or at least a descriptive value) and assert accordingly, e.g.rejects.toThroworrejects.toEqual(expect.any(Error)), once the implementation is updated.
24-26: Removeasyncfromdescribecallback and move the dynamic import tobeforeAll.The
describecallback must be synchronous for Vitest to properly collect tests. Moving the dynamic import to abeforeAllhook resolves this:-import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -describe('loadFileUploaderFrom', async () => { - const { default: testUrl } = await import('~/web/file-uploader.iife.min.js?url'); +describe('loadFileUploaderFrom', () => { + let testUrl: string; + + beforeAll(async () => { + ({ default: testUrl } = await import('~/web/file-uploader.iife.min.js?url')); + });
🧹 Nitpick comments (1)
tests/file-uploader-inline.e2e.test.tsx (1)
34-34: UsetoBeVisible()instead oftoBeDefined()for consistency.The assertion
toBeDefined()only verifies that the element query returned a value, not that the URL source is actually visible to the user. Other tests in this file usetoBeVisible()(lines 28, 48, 60, 66) for UI element verification.Apply this diff:
- await expect(urlSource).toBeDefined(); + await expect(urlSource).toBeVisible();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
src/abstract/__tests__/loadFileUploaderFrom.test.ts(1 hunks)src/blocks/CameraSource/CameraSource.ts(5 hunks)src/blocks/CameraSource/__tests__/calcCameraModes.test.ts(1 hunks)src/blocks/CloudImageEditor/src/EditorToolbar.ts(1 hunks)src/blocks/CloudImageEditor/src/crop-utils.test.ts(1 hunks)src/blocks/CloudImageEditor/src/lib/FocusVisible.ts(0 hunks)src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts(0 hunks)tests/adaptive-image.e2e.test.tsx(1 hunks)tests/cloud-image-editor.e2e.test.tsx(1 hunks)tests/file-uploader-inline.e2e.test.tsx(1 hunks)tests/validation.e2e.test.tsx(8 hunks)vite.config.js(1 hunks)
💤 Files with no reviewable changes (2)
- src/blocks/CloudImageEditor/src/lib/FocusVisible.ts
- src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/blocks/CloudImageEditor/src/crop-utils.test.ts
- tests/validation.e2e.test.tsx
- src/blocks/CameraSource/tests/calcCameraModes.test.ts
🧰 Additional context used
🪛 GitHub Check: CodeQL
tests/cloud-image-editor.e2e.test.tsx
[notice] 2-2: Unused variable, import, function or class
Unused import vi.
[notice] 6-6: Unused variable, import, function or class
Unused import renderer.
tests/adaptive-image.e2e.test.tsx
[notice] 1-1: Unused variable, import, function or class
Unused imports commands, userEvent.
[notice] 2-2: Unused variable, import, function or class
Unused import expect.
[notice] 5-5: Unused variable, import, function or class
Unused import renderer.
tests/file-uploader-inline.e2e.test.tsx
[notice] 1-1: Unused variable, import, function or class
Unused import commands.
[notice] 6-6: Unused variable, import, function or class
Unused import renderer.
⏰ 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 (8)
src/blocks/CameraSource/CameraSource.ts (4)
1025-1044: Tab test IDs look good and consistent
data-testid="tab-photo"anddata-testid="tab-video"follow the existing test-id style and clearly encode the mode. No issues; just keep these IDs stable since tests will depend on them.
1046-1054: Recording timer test ID is clear
data-testid="recording-timer"is descriptive and tied to the timer control used during recording; no behavioral change introduced.
1056-1071: Retake button test ID completes CTA coverageAdding
data-testid="retake"gives tests a stable hook for the retake path, matching theacceptandshotbuttons. Looks good.
1082-1097: Audio control test IDs are appropriately named
data-testid="toggle-microphone"anddata-testid="audio-select"are self-explanatory and align with existing test-id conventions in this template. No further changes needed.tests/file-uploader-inline.e2e.test.tsx (1)
37-67: Comprehensive camera source test coverage.The test effectively validates the camera source workflow including tab switching, microphone toggling, capture actions, and upload list rendering. The timing delays appropriately accommodate the recording flow.
src/blocks/CloudImageEditor/src/EditorToolbar.ts (1)
504-505: LGTM! Good accessibility enhancement.Adding
title-propattributes to the cancel and apply slider buttons improves accessibility and enables proper localization. The changes are purely metadata and don't affect control flow.vite.config.js (1)
24-25: LGTM! Appropriate coverage configuration.The expanded coverage configuration properly includes all TypeScript source files while excluding tests, configuration files, locales, and build artifacts. This aligns well with the PR objective of improving test coverage.
src/abstract/__tests__/loadFileUploaderFrom.test.ts (1)
27-124: Good coverage of UC_WINDOW_KEY and script loading behavior.The tests around existing
window[UC_WINDOW_KEY], script injection, resolve semantics, anddefineComponentsregistration cover the key branches and interactions cleanly and in an implementation-agnostic way. The structure here looks solid.
egordidenko
force-pushed
the
test/coverage-80
branch
from
546d03c to
a534c92
Compare
egordidenko
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
tests/adaptive-image.e2e.test.tsx (2)
1-5: Clean up unused imports and align with intended assertion usage
commandsanduserEventare unused; they can be dropped. Also,expectis currently unused because the only assertion is commented out—either removeexpector (preferably) re‑enable the assertion so the import is actually needed. Therendererimport looks intentionally kept for JSX; I’d leave it as is despite static analysis noise.
19-22: Enable the assertion so this test isn’t a no‑opRight now the test body does nothing because the
expect.element(...).toBeVisible()line is commented out. For meaningful coverage, uncomment the assertion (and keep theexpectimport) or remove the test entirely if it’s not ready yet.tests/file-uploader-inline.e2e.test.tsx (2)
1-6: Remove unusedcommandsimport; keeprendererif JSX truly depends on it
commandsisn’t used anywhere in this file and can be dropped. Therendererimport is still flagged by CodeQL, but given the// Used in JSXnote it’s likely required for the JSX setup—if so, keeping it and silencing the CodeQL rule in config is preferable to removing it.
62-63: Fix typo inaccptButtonvariable nameRename
accptButtontoacceptButton(and update the click call) for clarity and to avoid confusion when editing this test later:- const accptButton = cameraSource.getByTestId('uc-camera-source--accept'); - await userEvent.click(accptButton); + const acceptButton = cameraSource.getByTestId('uc-camera-source--accept'); + await userEvent.click(acceptButton);tests/cloud-image-editor.e2e.test.tsx (1)
1-6: Drop unusedviimport; keeprendereronly if required for JSX
viisn’t referenced in this file and can be removed from the Vitest import list. Therendererimport is again flagged as unused but appears intentionally kept for JSX; if that’s the case, prefer suppressing the CodeQL warning via configuration instead of deleting the import.
🧹 Nitpick comments (2)
tests/validation.e2e.test.tsx (1)
23-23: Explicitly disablingqualityInsightsin tests looks goodUsing
qualityInsights={false}on<uc-config>keeps test runs focused and deterministic. If this same config appears in many e2e files, consider a small shared render helper to reduce duplication.tests/cloud-image-editor.e2e.test.tsx (1)
41-45: Strengthen “tunings tab” test with an assertionThis test only clicks the element (
flip) without asserting any resulting state, so regressions might slip through. Consider checking for a visible tuning panel, active tab class, or any other UI change to confirm the interaction had the intended effect.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
src/abstract/__tests__/loadFileUploaderFrom.test.ts(1 hunks)src/blocks/CameraSource/CameraSource.ts(5 hunks)src/blocks/CameraSource/__tests__/calcCameraModes.test.ts(1 hunks)src/blocks/CloudImageEditor/src/EditorToolbar.ts(1 hunks)src/blocks/CloudImageEditor/src/crop-utils.test.ts(1 hunks)src/blocks/CloudImageEditor/src/lib/FocusVisible.ts(0 hunks)src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts(0 hunks)tests/adaptive-image.e2e.test.tsx(1 hunks)tests/cloud-image-editor.e2e.test.tsx(1 hunks)tests/file-uploader-inline.e2e.test.tsx(1 hunks)tests/validation.e2e.test.tsx(8 hunks)vite.config.js(1 hunks)
💤 Files with no reviewable changes (2)
- src/blocks/CloudImageEditor/src/lib/FocusVisible.ts
- src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts
✅ Files skipped from review due to trivial changes (1)
- src/abstract/tests/loadFileUploaderFrom.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/blocks/CloudImageEditor/src/EditorToolbar.ts
- vite.config.js
- src/blocks/CloudImageEditor/src/crop-utils.test.ts
- src/blocks/CameraSource/CameraSource.ts
- src/blocks/CameraSource/tests/calcCameraModes.test.ts
🧰 Additional context used
🪛 GitHub Check: CodeQL
tests/adaptive-image.e2e.test.tsx
[notice] 1-1: Unused variable, import, function or class
Unused imports commands, userEvent.
[notice] 2-2: Unused variable, import, function or class
Unused import expect.
[notice] 5-5: Unused variable, import, function or class
Unused import renderer.
tests/file-uploader-inline.e2e.test.tsx
[notice] 1-1: Unused variable, import, function or class
Unused import commands.
[notice] 6-6: Unused variable, import, function or class
Unused import renderer.
tests/cloud-image-editor.e2e.test.tsx
[notice] 2-2: Unused variable, import, function or class
Unused import vi.
[notice] 6-6: Unused variable, import, function or class
Unused import renderer.
⏰ 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 (1)
tests/validation.e2e.test.tsx (1)
210-210: Role‑based “Apply” button selectors are a solid improvementUsing
page.getByRole('button', { name: /apply/i })is more robust and aligned with accessibility than ad‑hoc selectors. Just ensure there’s only one visible “Apply” button per view and that the label isn’t localized in these test runs, otherwise this could become ambiguous or brittle.Also applies to: 256-256, 300-300, 449-449, 582-582, 595-595, 608-608
egordidenko
force-pushed
the
test/coverage-80
branch
from
a534c92 to
0c7eebe
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
tests/adaptive-image.e2e.test.tsx (2)
1-6: Remove unused imports.Static analysis correctly identifies
commands,userEvent,expect, andrendereras unused.Apply this diff:
-import { commands, page, userEvent } from '@vitest/browser/context'; -import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { page } from '@vitest/browser/context'; +import { beforeAll, beforeEach, describe, it } from 'vitest'; import '../types/jsx'; -// biome-ignore lint/correctness/noUnusedImports: Used in JSX -import { renderer } from './utils/test-renderer';
16-18: Complete the test or remove the placeholder.The commented-out assertion makes this test non-functional, defeating the purpose of adding test coverage.
Either restore the assertion (and re-add the
expectimport):it('should be rendered', async () => { - // await expect.element(page.getByTestId('uc-img')).toBeVisible(); + await expect.element(page.getByTestId('uc-img')).toBeVisible(); });Or remove the placeholder test entirely if it's not ready.
tests/file-uploader-inline.e2e.test.tsx (2)
1-6: Remove unused imports.Static analysis correctly identifies
commandsandrendereras unused.Apply this diff:
-import { commands, page, userEvent } from '@vitest/browser/context'; +import { page, userEvent } from '@vitest/browser/context'; import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; import '../types/jsx'; import { delay } from '@/utils/delay'; -// biome-ignore lint/correctness/noUnusedImports: Used in JSX -import { renderer } from './utils/test-renderer';
62-63: Fix typo in variable name.
accptButtonshould beacceptButton.Apply this diff:
- const accptButton = cameraSource.getByTestId('uc-camera-source--accept'); - await userEvent.click(accptButton); + const acceptButton = cameraSource.getByTestId('uc-camera-source--accept'); + await userEvent.click(acceptButton);tests/cloud-image-editor.e2e.test.tsx (1)
1-6: Remove unused imports.Static analysis correctly identifies
viandrendereras unused.Apply this diff:
import { page, userEvent } from '@vitest/browser/context'; -import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; import '../types/jsx'; -// biome-ignore lint/correctness/noUnusedImports: Used in JSX -import { renderer } from './utils/test-renderer'; - beforeAll(async () => {
🧹 Nitpick comments (5)
src/abstract/__tests__/loadFileUploaderFrom.test.ts (1)
24-25: Move the dynamic import to module level for better test framework compatibility.The async describe block with a dynamic import is non-standard and may cause test discovery issues or break with future test framework updates. Most test frameworks expect describe blocks to be synchronous.
Apply this diff to move the import to module level:
+const testUrlPromise = import('~/web/file-uploader.iife.min.js?url'); + -describe('loadFileUploaderFrom', async () => { - const { default: testUrl } = await import('~/web/file-uploader.iife.min.js?url'); +describe('loadFileUploaderFrom', () => { + let testUrl: string; + + beforeAll(async () => { + const imported = await testUrlPromise; + testUrl = imported.default; + });Alternatively, if the import can be synchronous:
+import testUrlImport from '~/web/file-uploader.iife.min.js?url'; + -describe('loadFileUploaderFrom', async () => { - const { default: testUrl } = await import('~/web/file-uploader.iife.min.js?url'); +describe('loadFileUploaderFrom', () => { + const testUrl = testUrlImport;src/blocks/CameraSource/__tests__/calcCameraModes.test.ts (2)
6-14: LGTM!The test correctly validates that
initialConfigenables both camera modes. Consider adding a comment documenting thatinitialConfig.cameraModesis expected to include both 'video' and 'photo' for clarity.
6-51: Consider additional edge case tests.The test suite covers the main scenarios well. To make it more robust, consider adding tests for:
nullorundefinedcameraModes values- Whitespace handling (e.g.,
'video, photo'or' video ')- Case sensitivity (e.g.,
'Video'vs'video')- Duplicate values (e.g.,
'video,video,photo')src/blocks/CloudImageEditor/src/crop-utils.test.ts (2)
71-108: Consider adding at least one test with exact expected output.The current tests only verify structural properties (
dcontains 'M' and 'L',centerhas length 2) but don't validate actual coordinate values. This means incorrect calculations could still pass. Consider adding at least one test per corner that asserts exact expected values:it('should generate correct path for northwest corner', () => { const result = cornerPath(rect, 'nw', 1); // Assert exact expected d string and center coordinates expect(result.d).toBe('M 100 ... expected path ...'); expect(result.center).toEqual([expectedX, expectedY]); });The same applies to
sidePathtests below.
490-498: Redundant test cases.These two tests are effectively identical - both test a rectangle that matches the outer rectangle exactly. Consider removing one or changing the second to test a rect that only touches some edges:
- it('should return true when inner rect touches edges', () => { - const touchingEdges: Rectangle = { x: 0, y: 0, width: 500, height: 400 }; - - expect(isRectInsideRect(touchingEdges, outerRect)).toBe(true); - }); + it('should return true when inner rect touches edges', () => { + const touchingLeft: Rectangle = { x: 0, y: 50, width: 200, height: 100 }; + const touchingRight: Rectangle = { x: 300, y: 50, width: 200, height: 100 }; + + expect(isRectInsideRect(touchingLeft, outerRect)).toBe(true); + expect(isRectInsideRect(touchingRight, outerRect)).toBe(true); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
src/abstract/__tests__/loadFileUploaderFrom.test.ts(1 hunks)src/blocks/CameraSource/CameraSource.ts(5 hunks)src/blocks/CameraSource/__tests__/calcCameraModes.test.ts(1 hunks)src/blocks/CloudImageEditor/src/EditorToolbar.ts(1 hunks)src/blocks/CloudImageEditor/src/crop-utils.test.ts(1 hunks)src/blocks/CloudImageEditor/src/lib/FocusVisible.ts(0 hunks)src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts(0 hunks)tests/adaptive-image.e2e.test.tsx(1 hunks)tests/cloud-image-editor.e2e.test.tsx(1 hunks)tests/file-uploader-inline.e2e.test.tsx(1 hunks)tests/validation.e2e.test.tsx(8 hunks)tsconfig.test.json(1 hunks)vite.config.js(1 hunks)
💤 Files with no reviewable changes (2)
- src/blocks/CloudImageEditor/src/lib/FocusVisible.ts
- src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/blocks/CameraSource/CameraSource.ts
- tests/validation.e2e.test.tsx
- vite.config.js
🧰 Additional context used
🪛 GitHub Check: CodeQL
tests/file-uploader-inline.e2e.test.tsx
[notice] 1-1: Unused variable, import, function or class
Unused import commands.
[notice] 6-6: Unused variable, import, function or class
Unused import renderer.
tests/adaptive-image.e2e.test.tsx
[notice] 1-1: Unused variable, import, function or class
Unused imports commands, userEvent.
[notice] 2-2: Unused variable, import, function or class
Unused import expect.
[notice] 5-5: Unused variable, import, function or class
Unused import renderer.
tests/cloud-image-editor.e2e.test.tsx
[notice] 2-2: Unused variable, import, function or class
Unused import vi.
[notice] 6-6: Unused variable, import, function or class
Unused import renderer.
⏰ 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 (9)
tsconfig.test.json (2)
8-8: Good addition for Vite type support.Adding
"vite/client"to the types array is the standard way to enable TypeScript support for Vite-specific APIs and globals in the test environment. This is appropriate and necessary.
14-17: Path aliases are properly configured and consistent.The path mappings in
tsconfig.test.json(@/*→./src/*and~/*→./*) correctly align with the Vite aliases defined invite.config.js(@→srcand~→ root). This configuration follows standard conventions and will facilitate cleaner imports in test files.src/blocks/CloudImageEditor/src/EditorToolbar.ts (1)
504-505: LGTM! Test-friendly metadata added.The
title-propattributes on the cancel and apply buttons provide accessible selectors for testing without affecting functionality.tests/cloud-image-editor.e2e.test.tsx (1)
36-82: LGTM! Comprehensive test coverage.The test suite provides good coverage of the Cloud Image Editor functionality, including rendering, tab navigation, crop preset selection, and tuning operations.
src/abstract/__tests__/loadFileUploaderFrom.test.ts (1)
1-149: Comprehensive test coverage with clear test cases.The test suite thoroughly covers all major scenarios including existing window.UC, successful script loading, error handling, and edge cases. The tests are well-organized, use appropriate mocking, and properly clean up after themselves.
src/blocks/CameraSource/__tests__/calcCameraModes.test.ts (3)
1-4: LGTM!The imports are appropriate for testing the
calcCameraModesfunction.
16-41: LGTM!The test cases for individual modes and empty configuration are well-structured and comprehensive.
43-50: The test's minimal config is valid.calcCameraModesonly accesses thecameraModesproperty from the config, which is provided in the minimal object. This is an intentional edge case test with no issues.src/blocks/CloudImageEditor/src/crop-utils.test.ts (1)
1-19: Well-structured test suite with comprehensive coverage.Good job covering all exported utilities from
crop-utils. The test organization using nesteddescribeblocks is clean, and the coverage of edge cases (boundary constraints, aspect ratios, negative values, decimal rounding) is thorough.
egordidenko
force-pushed
the
test/coverage-80
branch
from
0c7eebe to
2e1feb4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
tests/adaptive-image.e2e.test.tsx (2)
1-5: Remove unused imports.The imports
commands,userEvent,expect, andrendererare not used in this file. Thebiome-ignorecomment on line 4 incorrectly claimsrendereris used in JSX, but line 12 actually usespage.render().Apply this diff to remove the unused imports:
-import { commands, page, userEvent } from '@vitest/browser/context'; -import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { page } from '@vitest/browser/context'; +import { beforeAll, beforeEach, describe, it } from 'vitest'; import '../types/jsx'; -// biome-ignore lint/correctness/noUnusedImports: Used in JSX -import { renderer } from './utils/test-renderer';
16-18: Complete the test or remove the placeholder.The test assertion is commented out, making this test non-functional. Either implement the test properly or remove the placeholder if it's not ready.
Apply this diff to enable the assertion:
it('should be rendered', async () => { - // await expect.element(page.getByTestId('uc-img')).toBeVisible(); + await expect.element(page.getByTestId('uc-img')).toBeVisible(); });Note: You'll need to restore the
expectimport if you uncomment this assertion.tests/file-uploader-inline.e2e.test.tsx (2)
1-6: Clean up unusedcommandsimport; keeprendererif needed for JSX.
commandsis never used and can be dropped; this removes the CodeQL notice without changing behavior. Therendererimport appears to be intentionally kept for JSX side effects (per the biome-ignore comment), so no change there unless you switch to a pure side-effect import.-import { commands, page, userEvent } from '@vitest/browser/context'; +import { page, userEvent } from '@vitest/browser/context';
37-67: FixaccptButtontypo and consider avoiding fixed 2s delay.
- The variable name
accptButtonis misspelled, which hurts readability and can confuse future edits. Rename it and update the usage:- const accptButton = cameraSource.getByTestId('uc-camera-source--accept'); - await userEvent.click(accptButton); + const acceptButton = cameraSource.getByTestId('uc-camera-source--accept'); + await userEvent.click(acceptButton);
- The hardcoded
await delay(2000);may slow the suite and still be flaky on slower/faster environments. Where possible, prefer waiting on a concrete UI state (e.g., a recording indicator or specific class change) rather than a fixed sleep.tests/cloud-image-editor.e2e.test.tsx (1)
2-2: Remove unusedviimport.
viis not referenced in this file; dropping it will satisfy static analysis and keep imports minimal.-import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import { beforeAll, beforeEach, describe, expect, it } from 'vitest';
🧹 Nitpick comments (4)
src/blocks/CameraSource/CameraSource.ts (1)
1031-1094: LGTM! Test identifiers added appropriately.The
data-testidattributes are well-named, follow a consistent kebab-case convention, and are added to key interactive elements for test automation. These additions align perfectly with the PR objective of adding test coverage.For consistency and comprehensive test coverage, consider adding test identifiers to the remaining interactive elements:
<button type="button" class="uc-mini-btn" set="onclick: *historyBack" l10n="@title:back" + data-testid="back" ><button type="button" class="uc-mini-btn uc-close-btn" set="onclick: *closeModal" l10n="@title:a11y-activity-header-button-close;@aria-label:a11y-activity-header-button-close" + data-testid="close" ><uc-select class="uc-camera-select" set="$.options: cameraSelectOptions; @hidden: cameraSelectHidden; onchange: onCameraSelectChange" + data-testid="camera-select" ><button type="button" set="onclick: onRequestPermissions; @hidden: requestBtnHidden" l10n="camera-permissions-request" + data-testid="request-permissions" ></button>tests/validation.e2e.test.tsx (1)
210-210: Excellent refactoring to role-based selector.The migration from text-based to
getByRole('button', { name: /apply/i })follows testing best practices. Role-based selectors are more robust, accessible, and aligned with Testing Library principles.Also applies to: 256-256, 300-300, 449-449, 581-581, 594-594, 607-607
tests/file-uploader-inline.e2e.test.tsx (1)
31-35: Strengthen the URL source assertion.Right now the test effectively just checks that
getByTestId('uc-url-source')returns something (which it already guarantees by throwing on failure). Consider asserting visibility instead to better capture the behavior:- const urlSource = page.getByTestId('uc-url-source'); - await expect(urlSource).toBeDefined(); + const urlSource = page.getByTestId('uc-url-source'); + await expect(urlSource).toBeVisible();tests/cloud-image-editor.e2e.test.tsx (1)
41-45: Add an assertion to"should select tunings tab".This test only performs a click and never verifies that the tunings tab (or any related UI state) actually changed. Consider asserting that the expected tab/panel is active or visible after the click so the test meaningfully guards behavior rather than just acting as a smoke test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
src/blocks/CameraSource/CameraSource.ts(5 hunks)src/blocks/CameraSource/__tests__/calcCameraModes.test.ts(1 hunks)src/blocks/CloudImageEditor/src/EditorToolbar.ts(1 hunks)src/blocks/CloudImageEditor/src/crop-utils.test.ts(1 hunks)src/blocks/CloudImageEditor/src/lib/FocusVisible.ts(0 hunks)src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts(0 hunks)tests/adaptive-image.e2e.test.tsx(1 hunks)tests/cloud-image-editor.e2e.test.tsx(1 hunks)tests/file-uploader-inline.e2e.test.tsx(1 hunks)tests/validation.e2e.test.tsx(8 hunks)tsconfig.test.json(1 hunks)vite.config.js(1 hunks)
💤 Files with no reviewable changes (2)
- src/blocks/CloudImageEditor/src/lib/applyFocusVisiblePolyfill.ts
- src/blocks/CloudImageEditor/src/lib/FocusVisible.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/blocks/CameraSource/tests/calcCameraModes.test.ts
- src/blocks/CloudImageEditor/src/EditorToolbar.ts
- tsconfig.test.json
- vite.config.js
🧰 Additional context used
🪛 GitHub Check: CodeQL
tests/adaptive-image.e2e.test.tsx
[notice] 1-1: Unused variable, import, function or class
Unused imports commands, userEvent.
[notice] 2-2: Unused variable, import, function or class
Unused import expect.
[notice] 5-5: Unused variable, import, function or class
Unused import renderer.
tests/file-uploader-inline.e2e.test.tsx
[notice] 1-1: Unused variable, import, function or class
Unused import commands.
[notice] 6-6: Unused variable, import, function or class
Unused import renderer.
tests/cloud-image-editor.e2e.test.tsx
[notice] 2-2: Unused variable, import, function or class
Unused import vi.
[notice] 6-6: Unused variable, import, function or class
Unused import renderer.
⏰ 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 (3)
tests/validation.e2e.test.tsx (1)
23-23: Good test isolation withqualityInsights={false}.Disabling quality insights in test setup helps ensure tests remain deterministic and isolated from external telemetry features. The
qualityInsightsprop is properly supported in theuc-configcomponent and is consistently set tofalseacross all e2e test files.tests/file-uploader-inline.e2e.test.tsx (1)
8-30: Test setup and basic render assertion look solid.One‑time component definition in
beforeAllplus a freshctx-nameper test is a good isolation pattern, and the initial “Drop files here” visibility check exercises the main entry UI without over-specifying layout.src/blocks/CloudImageEditor/src/crop-utils.test.ts (1)
21-719: Thorough and well‑targetedcrop-utilstest coverage.This suite exercises the helpers across nominal cases, boundaries, aspect‑ratio constraints, and numeric edge conditions; expectations look consistent with typical crop/geometry behavior. No issues from this review.
Description
Checklist
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.