Stub Clipboard methods during tests #2392
Conversation
VasylMarchuk
added
bug
WalkthroughThe pull request introduces several modifications across multiple components to enhance clipboard functionality. Key changes include converting several Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (10)
app/config/environment.d.ts (1)
14-14: Add documentation and type constraints for copyConfirmationTimeout.While the type declaration is correct, consider adding documentation and constraints:
- copyConfirmationTimeout: number; + /** + * Duration (in milliseconds) to show the copy confirmation message + * Default: 1000ms in components, 10ms in tests + */ + copyConfirmationTimeout: number & { __brand: 'PositiveNumber' };This addition would:
- Document the expected unit (milliseconds)
- Clarify the different values used in components vs tests
- Use TypeScript's branded types to ensure positive numbers
app/components/copyable-code.ts (1)
Line range hint
34-36: Add error handling for the callbackThe callback execution should be wrapped in a try-catch to prevent silent failures:
if (this.args.onCopyButtonClick) { - this.args.onCopyButtonClick(); + try { + this.args.onCopyButtonClick(); + } catch (error) { + console.error('Callback execution failed:', error); + } }tests/support/stub-clipboard.ts (2)
5-15: Enhance return type documentation.The documentation is thorough, but the return type description could be more specific about how the returned cache array is used for testing purposes.
Consider updating the return type description to:
- * @returns {ClipboardItem[]} Fake clipboard cache + * @returns {ClipboardItem[]} Fake clipboard cache array that can be used to verify clipboard operations in tests
29-31: Optimize cache mutation and improve type safety.The cache mutation in
writeTextcould be simplified, and type safety could be enhanced.Consider these improvements:
- stub(navigator.clipboard, 'writeText').callsFake(async function (text = '') { - cache.splice(0, Infinity, ...[new ClipboardItem({ [TEXT_MIME_TYPE]: new Blob([text], { type: TEXT_MIME_TYPE }) })]); + stub(navigator.clipboard, 'writeText').callsFake(async function (text: string = '') { + cache.length = 0; + cache.push(new ClipboardItem({ [TEXT_MIME_TYPE]: new Blob([text], { type: TEXT_MIME_TYPE }) }));tests/support/stub-local-storage.ts (3)
3-14: Consider enhancing documentation with usage examples.The documentation is thorough, but adding usage examples would make it more developer-friendly and help prevent misuse.
Add example usage to the JSDoc:
* @returns {Map<string, string>} Fake localStorage cache + * + * @example + * // Basic usage + * const cache = stubLocalStorage(); + * + * // Usage with predefined cache + * const initialCache = new Map([['key', 'value']]); + * const cache = stubLocalStorage(initialCache); */
36-38: Optimize key() method performance.Creating a new array from Map keys on every call could be inefficient for large datasets.
Consider caching the keys array or using Array.from with index access:
stub(window.localStorage, 'key').callsFake(function (index) { - return [...cache.keys()][index] || null; + let i = 0; + for (const key of cache.keys()) { + if (i === index) return key; + i++; + } + return null; });
15-45: Enhance type safety and error handling.Consider adding stricter TypeScript types and error handling for edge cases.
Here are some suggested improvements:
interface StorageStub { cache: Map<string, string>; restore(): void; } export default function stubLocalStorage( initialCache: Map<string, string> = new Map() ): StorageStub { const stubs = { getItem: stub(window.localStorage, 'getItem'), setItem: stub(window.localStorage, 'setItem'), // ... other stubs }; // ... existing implementation ... return { cache, restore: () => { Object.values(stubs).forEach(stub => stub.restore()); } }; }This provides:
- Type-safe stub management
- A clean way to restore original methods
- Better IDE support
app/components/course-admin/code-example-page/comparison-card.ts (1)
72-72: Fix typo in method name: "Clipbard" → "Clipboard".The method name contains a typo:
handleCopyIdToClipbardButtonClickshould behandleCopyIdToClipboardButtonClick.app/components/course-page/share-progress-modal.ts (1)
67-67: Consider adding error handling for clipboard operations.The clipboard API can fail due to various reasons (permissions, browser support, etc.). Consider wrapping the operation in a try-catch block.
- await navigator.clipboard.writeText(this.copyableText); + try { + await navigator.clipboard.writeText(this.copyableText); + } catch (error) { + console.error('Failed to copy to clipboard:', error); + // Optionally: Set an error state or show a fallback message + return; + }app/components/course-page/repository-dropdown.ts (1)
Line range hint
63-77: Add error handling and proper async/await usage for clipboard operations.While the configurable timeout is a good improvement, there are some concerns with the clipboard operation handling:
- The clipboard operation should be properly awaited
- Error handling is missing for clipboard operations
- The timeout callback could execute even if the clipboard operation fails
Consider this improved implementation:
@action async handleCopyGitRepositoryURLClick() { // get('isNew') works around type bug if (this.args.activeRepository.get('isNew')) { return; } - await navigator.clipboard.writeText(this.args.activeRepository.cloneUrl); - this.gitRepositoryURLWasCopiedRecently = true; - - later( - this, - () => { - this.gitRepositoryURLWasCopiedRecently = false; - }, - config.x.copyConfirmationTimeout, - ); + try { + await navigator.clipboard.writeText(this.args.activeRepository.cloneUrl); + this.gitRepositoryURLWasCopiedRecently = true; + + later( + this, + () => { + this.gitRepositoryURLWasCopiedRecently = false; + }, + config.x.copyConfirmationTimeout, + ); + } catch (error) { + console.error('Failed to copy to clipboard:', error); + // Optionally: Show user feedback about the failure + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
app/components/copyable-code.ts(3 hunks)app/components/copyable-terminal-command.ts(3 hunks)app/components/course-admin/code-example-page/comparison-card.ts(1 hunks)app/components/course-admin/code-example-page/evaluation-card/evaluation-tab.ts(2 hunks)app/components/course-page/repository-dropdown.ts(2 hunks)app/components/course-page/share-progress-modal.ts(1 hunks)app/config/environment.d.ts(1 hunks)config/environment.js(2 hunks)tests/support/stub-clipboard.ts(1 hunks)tests/support/stub-local-storage.ts(1 hunks)tests/test-helper.js(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/components/course-admin/code-example-page/evaluation-card/evaluation-tab.ts
[warning] 33-34: app/components/course-admin/code-example-page/evaluation-card/evaluation-tab.ts#L33-L34
Added lines #L33 - L34 were not covered by tests
🔇 Additional comments (17)
app/components/copyable-code.ts (3)
5-5: LGTM: Import ordering follows conventions
The placement of the config import after framework imports follows good practices.
19-19: LGTM: Well-typed tracked property
The codeWasCopiedRecently property is properly typed and decorated.
31-31: Verify copyConfirmationTimeout configuration
Let's ensure the configuration is properly defined in the environment files.
✅ Verification successful
Let me check the actual configuration definition to ensure it's properly set up.
Configuration copyConfirmationTimeout is properly defined
The configuration is correctly defined in config/environment.js with a default value of 1000ms and can be overridden to 10ms in certain environments. The usage in the component is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify copyConfirmationTimeout configuration exists in environment files
# Test: Check if copyConfirmationTimeout is defined in environment config
rg -l "copyConfirmationTimeout" config/
Length of output: 61
Script:
#!/bin/bash
# Get the content of the environment.js file to verify the configuration
rg "copyConfirmationTimeout" config/environment.js -B 2 -A 2
Length of output: 349
tests/support/stub-clipboard.ts (1)
1-3: LGTM! Clean import and constant declaration.
The import and constant declaration follow best practices and use appropriate naming conventions.
tests/test-helper.js (2)
10-11: LGTM! Clean separation of test helpers.
The new imports for stub helpers follow the project's conventions and support the goal of modularizing test utilities.
21-22: Verify stub cleanup handling.
The stub initialization looks good, but we should ensure proper cleanup to prevent test pollution.
Let's verify the cleanup implementation in the stub helpers:
✅ Verification successful
Cleanup is automatically handled by Sinon
The stubs are implemented using Sinon's stub() function and the test setup uses setupSinon() from ember-sinon-qunit, which automatically handles cleanup of all stubs after each test. No additional cleanup code is needed because:
- Both stubs use Sinon's
stub()function which integrates with QUnit's test lifecycle - The
ember-sinon-qunitsetup in test-helper.js ensures all Sinon stubs are automatically restored after each test - The implementation returns clean caches (new Map/Array) by default for each test
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the stub helpers properly handle cleanup
# Look for cleanup patterns in stub-local-storage.js
echo "Checking localStorage stub cleanup..."
rg -A 5 "QUnit\.(testDone|moduleDone)" "tests/support/stub-local-storage.js"
# Look for cleanup patterns in stub-clipboard.js
echo "Checking clipboard stub cleanup..."
rg -A 5 "QUnit\.(testDone|moduleDone)" "tests/support/stub-clipboard.js"
# Look for Sinon sandbox usage which would handle cleanup automatically
echo "Checking Sinon sandbox usage..."
rg "sinon\.(sandbox|createSandbox)" "tests/support/stub-*.js"
Length of output: 662
Script:
#!/bin/bash
# Let's first locate the stub files
echo "Locating stub files..."
fd "stub.*\.js" tests/
# Once we find them, let's examine their content
echo -e "\nChecking cleanup patterns in all test files..."
rg -A 5 "QUnit\.(testDone|moduleDone)" tests/
# Look for Sinon sandbox usage in test files
echo -e "\nChecking Sinon sandbox usage..."
rg "sinon\.(sandbox|createSandbox)" tests/
# Check test-helper.js for cleanup patterns since it's where stubs are initialized
echo -e "\nChecking test-helper.js..."
cat tests/test-helper.js
Length of output: 1847
Script:
#!/bin/bash
# Let's check the actual implementation of stub files by their import paths
echo "Checking stub-local-storage implementation..."
rg -l "stub-local-storage" tests/
cat tests/support/stub-local-storage.js || cat app/tests/support/stub-local-storage.js
echo -e "\nChecking stub-clipboard implementation..."
rg -l "stub-clipboard" tests/
cat tests/support/stub-clipboard.js || cat app/tests/support/stub-clipboard.js
# Also check for any cleanup in QUnit.testDone across the entire codebase
echo -e "\nChecking for testDone handlers..."
rg "QUnit\.testDone" --type js
Length of output: 861
Script:
#!/bin/bash
# Let's try to find the actual location of these files by checking the import statements
echo "Checking for stub files in the entire codebase..."
fd -e js -e ts "stub-local-storage" .
fd -e js -e ts "stub-clipboard" .
# Let's also check the actual import paths from test-helper.js
echo -e "\nChecking resolved import paths..."
rg "import.*stub-(local-storage|clipboard)" --type js -A 1
# Check if these stubs might be in node_modules
echo -e "\nChecking package.json for potential test support packages..."
cat package.json | grep -A 5 "devDependencies"
Length of output: 1035
Script:
#!/bin/bash
# Now that we found the TypeScript files, let's examine their content
echo "Checking stub-local-storage.ts implementation..."
cat tests/support/stub-local-storage.ts
echo -e "\nChecking stub-clipboard.ts implementation..."
cat tests/support/stub-clipboard.ts
# Also check for any cleanup patterns in test teardown
echo -e "\nChecking for QUnit cleanup patterns..."
rg "QUnit\.(testDone|moduleDone|done)" tests/support/stub-*.ts
Length of output: 3044
app/components/course-admin/code-example-page/evaluation-card/evaluation-tab.ts (2)
6-6: LGTM: Clean import addition
The config import is correctly positioned and necessary for the timeout configuration.
33-34:
Add error handling and null checks
The clipboard operation needs additional safeguards:
- Missing null check for
logsFileContents - No error handling for clipboard operations
- Test coverage is missing
Here's a suggested improvement:
copyToClipboardTask = task({ keepLatest: true }, async (): Promise<void> => {
this.wasRecentlyCopied = true;
+ try {
+ if (!this.args.evaluation.logsFileContents) {
+ throw new Error('No logs content available to copy');
+ }
await navigator.clipboard.writeText(this.args.evaluation.logsFileContents!);
await timeout(config.x.copyConfirmationTimeout);
+ } catch (error) {
+ // Handle clipboard errors (e.g., permissions)
+ console.error('Failed to copy to clipboard:', error);
+ } finally {
this.wasRecentlyCopied = false;
+ }
});Let's verify the test coverage:
Would you like me to help create test cases for the clipboard operations? We should test:
- Successful copy operation
- Handling of null logs content
- Clipboard permission errors
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-34: app/components/course-admin/code-example-page/evaluation-card/evaluation-tab.ts#L33-L34
Added lines #L33 - L34 were not covered by tests
tests/support/stub-local-storage.ts (1)
16-22: LGTM! Correctly implements Storage.getItem spec.
The implementation properly handles both existing and non-existing keys, ensuring string return type as per the Storage API specification.
app/components/copyable-terminal-command.ts (3)
8-8: LGTM: Import statement repositioning
The config import is correctly positioned and needed for the timeout configuration.
38-39: LGTM: Proper async/await implementation
The clipboard operation is now correctly handled as an async operation with proper await statement.
48-48: Verify config.x.copyConfirmationTimeout exists
Let's ensure the config property is properly defined.
✅ Verification successful
Property config.x.copyConfirmationTimeout is properly defined and used consistently
The verification shows that:
- The property is defined in
config/environment.jswith a default value of 1000ms and overridden to 10ms in development - It's properly typed in
app/config/environment.d.ts - Used consistently across multiple components for copy confirmation timeouts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the config property exists and is used consistently
# Search for copyConfirmationTimeout definition
echo "Searching for copyConfirmationTimeout definition:"
rg -p "copyConfirmationTimeout.*=" config/
# Search for other usages to ensure consistency
echo -e "\nSearching for other usages:"
rg "copyConfirmationTimeout"
Length of output: 987
app/components/course-page/share-progress-modal.ts (1)
67-69: LGTM! Changes align with PR objectives.
The modifications correctly implement:
- Proper async/await for clipboard operations
- Configurable timeout from config
config/environment.js (2)
31-32: LGTM! Good choice for the default timeout.
The 1-second default timeout is a reasonable duration for production use, providing enough time for user feedback without being too intrusive.
88-88: LGTM! Appropriate test timeout configuration.
Setting a shorter timeout (10ms) for tests is a good optimization that will help speed up test execution while still maintaining the same behavior verification.
Let's verify the type definition exists:
✅ Verification successful
Type definition for copyConfirmationTimeout is properly defined
The verification confirms that the copyConfirmationTimeout property is correctly typed as a number in the TypeScript definition file, which aligns with the configuration value of 10 being set in config/environment.js.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the type definition for copyConfirmationTimeout exists
# Expected: Should find the type definition in the environment.d.ts file
fd -e d.ts environment | xargs rg "copyConfirmationTimeout"
Length of output: 96
app/components/course-page/repository-dropdown.ts (2)
10-10: LGTM! Import statement is correctly placed and necessary.
The addition of the config import enables the use of configurable timeouts as intended.
Line range hint 63-77: Verify removal of environment checks for clipboard operations.
Let's ensure all environment checks for clipboard operations have been removed as per PR objectives.
✅ Verification successful
Environment checks for clipboard operations have been successfully removed
After reviewing all clipboard-related code in the codebase, I can confirm that there are no remaining environment checks for clipboard operations. The clipboard operations are now being used directly without any environment-specific conditions:
- Direct clipboard usage in repository-dropdown.ts:
navigator.clipboard.writeText - Similar direct usage in other components like copyable-code.ts, copyable-terminal-command.ts, and share-progress-modal.ts
- Test stubs are properly maintained in tests/support/stub-clipboard.ts for testing purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining environment checks related to clipboard operations
rg -i "environment.*test.*clipboard" || rg -i "clipboard.*environment.*test"
Length of output: 77
Script:
#!/bin/bash
# Let's search for clipboard-related code more broadly to ensure we catch all cases
rg -i "clipboard" -A 3 -B 3
Length of output: 17401
app/components/course-admin/code-example-page/comparison-card.ts
Outdated
Show resolved
Hide resolved
app/components/course-admin/code-example-page/comparison-card.ts
Outdated
Show resolved
Hide resolved
… it to 10 in tests
VasylMarchuk
force-pushed
the
stub-clipboard-during-tests
branch
from
6e481c2 to
52edc45
Compare
Bundle ReportChanges will increase total bundle size by 1.88kB (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/components/course-page/share-progress-modal.ts (1)
69-69: Consider extracting the timeout duration to a constant.While the configurable timeout is a good improvement, consider extracting these magic numbers to named constants for better maintainability.
+const COPY_CONFIRMATION_TIMEOUT = { + TEST: 10, + PRODUCTION: 1000 +}; -await timeout(config.environment !== 'test' ? 1000 : 10); +await timeout(config.environment !== 'test' ? COPY_CONFIRMATION_TIMEOUT.PRODUCTION : COPY_CONFIRMATION_TIMEOUT.TEST);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
app/components/copyable-code.ts(3 hunks)app/components/copyable-terminal-command.ts(3 hunks)app/components/course-admin/code-example-page/comparison-card.hbs(1 hunks)app/components/course-admin/code-example-page/comparison-card.ts(1 hunks)app/components/course-admin/code-example-page/evaluation-card/evaluation-tab.ts(2 hunks)app/components/course-page/repository-dropdown.ts(2 hunks)app/components/course-page/share-progress-modal.ts(1 hunks)tests/support/stub-clipboard.ts(1 hunks)tests/support/stub-local-storage.ts(1 hunks)tests/test-helper.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- app/components/course-admin/code-example-page/comparison-card.ts
- app/components/course-admin/code-example-page/evaluation-card/evaluation-tab.ts
- tests/support/stub-local-storage.ts
- tests/test-helper.js
- app/components/copyable-terminal-command.ts
- tests/support/stub-clipboard.ts
- app/components/course-page/repository-dropdown.ts
- app/components/copyable-code.ts
🧰 Additional context used
📓 Learnings (1)
app/components/course-admin/code-example-page/comparison-card.hbs (1)
Learnt from: VasylMarchuk
PR: codecrafters-io/frontend#2392
File: app/components/course-admin/code-example-page/comparison-card.ts:72-73
Timestamp: 2024-11-12T07:09:58.518Z
Learning: In the `ComparisonCard` component in `app/components/course-admin/code-example-page/comparison-card.ts`, adding error handling for the clipboard operation in `handleCopyIdToClipboardButtonClick` is not required.
🔇 Additional comments (3)
app/components/course-page/share-progress-modal.ts (1)
67-67: LGTM! Proper async handling added.
The addition of await for the clipboard operation ensures proper asynchronous handling, preventing potential race conditions.
app/components/course-admin/code-example-page/comparison-card.hbs (2)
43-43: LGTM! Typo fix in the event handler name.
The correction from handleCopyIdToClipbardButtonClick to handleCopyIdToClipboardButtonClick aligns with the PR's objective of improving clipboard handling.
43-43: Verify async event handling.
Since the corresponding TypeScript method is now async, ensure the template properly handles the Promise resolution.
✅ Verification successful
No changes needed for async event handling
The {{on}} modifier in Ember.js automatically handles promises returned from event handlers. The implementation is correct as is, since Ember's event system will properly await the promise returned by handleCopyIdToClipboardButtonClick. No additional modifiers or error handling are required for this simple clipboard operation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the component uses proper async event handling modifiers
# Look for any ember-concurrency or similar async handling patterns in the component
ast-grep --pattern 'class ComparisonCard {
$$$
handleCopyIdToClipboardButtonClick = $_ => {
$$$
}
$$$
}'
# Check for perform/did-error modifiers in templates
rg -A 2 'perform' app/components/course-admin/code-example-page/comparison-card.hbs
Length of output: 202
Script:
#!/bin/bash
# Let's check the implementation of handleCopyIdToClipboardButtonClick in the component's TypeScript/JavaScript file
fd "comparison-card" --exec rg -l "handleCopyIdToClipboardButtonClick"
# Check the actual implementation
ast-grep --pattern 'handleCopyIdToClipboardButtonClick = async $_'
# Look for any async event handling patterns in the template
rg "on \"click\"" app/components/course-admin/code-example-page/comparison-card.hbs -A 2
Length of output: 761
Script:
#!/bin/bash
# Let's check the actual implementation in the TypeScript file
rg -A 5 "handleCopyIdToClipboardButtonClick" app/components/course-admin/code-example-page/comparison-card.ts
Length of output: 267
Extracted from #2365
Brief
Stub all clipboard methods when running tests using Sinon's fakes.
Details
stubClipboardtest helper and made run it before each testenvironment === 'test'checks from component methods callingnavigator.clipboard.writeTextcopyConfirmationTimeout: 1000to configcopyConfirmationTimeoutis set to10awaitstatements to allnavigator.clipboard.writeTextcallslocalStorage-stubbing logic into a test helperstubLocalStorageas welltest-helper.jsfilestubLocalStoragein TypeScriptstubClipboardandstubLocalStoragetest helpers can be called multiple times during a testArrayorMap) used to store temporary fake valuesChecklist
[percy]in the message to trigger)Summary by CodeRabbit
New Features
Bug Fixes
Tests