Unify all interactions with Clipboard & stub all Clipboard methods during tests #2365
Conversation
VasylMarchuk
added
bug
WalkthroughThe changes introduced in this pull request primarily focus on implementing a new 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 ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files📢 Thoughts on this report? Let us know! |
VasylMarchuk
force-pushed
the
stub-clipboard-during-tests
branch
from
537d794
to
4b52d81
Compare
Bundle ReportChanges will decrease total bundle size by 1.57kB (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
VasylMarchuk
force-pushed
the
stub-clipboard-during-tests
branch
from
c20229d
to
978803a
Compare
VasylMarchuk
force-pushed
the
stub-clipboard-during-tests
branch
from
978803a
to
ec58d16
Compare
VasylMarchuk
force-pushed
the
stub-clipboard-during-tests
branch
2 times, most recently
from
0824b63
to
89a48b7
Compare
VasylMarchuk
force-pushed
the
stub-clipboard-during-tests
branch
from
89a48b7
to
c7bac21
Compare
VasylMarchuk
force-pushed
the
stub-clipboard-during-tests
branch
from
f0cfa46
to
bf39b90
Compare
… it to 10 in tests
VasylMarchuk
force-pushed
the
stub-clipboard-during-tests
branch
from
8c78c7a
to
6a7f88f
Compare
VasylMarchuk
force-pushed
the
stub-clipboard-during-tests
branch
from
6a7f88f
to
2d1c80e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (17)
app/config/environment.d.ts (1)
14-14: Add JSDoc documentation for the new configuration property.The addition of
copyConfirmationTimeoutis well-typed and correctly placed. Consider adding JSDoc documentation to describe its purpose, expected values, and units (milliseconds).x: { backendUrl: string; + /** Duration in milliseconds to show the copy confirmation message. Default: 1000ms */ copyConfirmationTimeout: number; isCI: boolean;app/components/copyable-code.hbs (1)
12-21: Enhance accessibility of the copy button.The integration of CopyableAnything looks good and aligns with the PR objective of unifying clipboard interactions. However, consider adding accessibility attributes to improve user experience.
Apply these improvements to the button:
<button type="button" class="bg-teal-600 px-3 py-1 text-white flex items-center rounded-r rounded-l-none text-sm font-bold" {{on "click" performCopyTask}} data-test-copy-button + aria-label="Copy code to clipboard" + title="Copy code to clipboard" > {{if hasRecentlyCopied "COPIED" "COPY"}} </button>app/components/copyable-terminal-command.ts (1)
Line range hint
1-41: Well-structured component refactoringThe component has been successfully simplified by delegating clipboard functionality to
CopyableAnythingwhile maintaining its core responsibility of command text preparation. This separation of concerns improves maintainability and reduces code duplication.tests/support/stub-clipboard.ts (1)
5-15: Consider adding example usage to the documentation.The documentation is thorough, but it could be even more helpful with an example showing how to use this helper in tests.
Add example usage to the JSDoc:
* @param {ClipboardItem[]} [cache=new Array<ClipboardItem>()] Predefined contents of the fake clipboard cache * @returns {ClipboardItem[]} Fake clipboard cache + * + * @example + * ```typescript + * // In your test file + * const clipboardCache = stubClipboard(); + * + * // Pre-populate clipboard + * await navigator.clipboard.writeText('test content'); + * + * // Assert clipboard content + * expect(await navigator.clipboard.readText()).to.equal('test content'); + * ``` */tests/support/stub-local-storage.ts (1)
1-14: Enhance documentation with usage example.The documentation is thorough, but adding an example usage would make it more developer-friendly.
Add the following to the JSDoc:
/** * Stubs the following `window.localStorage` methods with sinon fakes * and returns the Map used for storing the fake localStorage cache: * - `setItem` * - `removeItem` * - `clear` * - `key` * - `get length` * @see {@link https://developer.mozilla.org/en-US/docs/Web/API/Storage} * @param {Map<string, string>} [cache=new Map<string, string>()] Predefined contents of the fake localStorage cache * @returns {Map<string, string>} Fake localStorage cache + * @example + * // Basic usage + * const cache = stubLocalStorage(); + * localStorage.setItem('key', 'value'); + * assert.equal(localStorage.getItem('key'), 'value'); + * assert.equal(cache.get('key'), 'value'); + * + * // With pre-populated cache + * const initialCache = new Map([['existingKey', 'existingValue']]); + * stubLocalStorage(initialCache); */app/components/copyable-anything.ts (1)
12-13: Consider adding runtime validation for nullable value.The
valueargument is typed as potentially null, but it's directly converted to String on line 32 without validation. While TypeScript ensures type safety at compile time, consider adding runtime validation for a better developer experience.async copy(): Promise<void> { if (this.args.isDisabled) { return; } + if (this.args.value == null) { + console.warn('CopyableAnything: Attempted to copy null or undefined value'); + return; + } await navigator.clipboard.writeText(String(this.args.value));app/components/course-page/share-progress-modal.hbs (1)
27-44: LGTM! Consider adding ARIA attributes for better accessibility.The implementation correctly uses the new CopyableAnything component and provides clear visual feedback. The animation and state management are well-implemented.
Consider adding aria-label to improve accessibility:
- <PrimaryButton {{on "click" performCopyTask}} data-test-copy-button> + <PrimaryButton + {{on "click" performCopyTask}} + data-test-copy-button + aria-label={{if hasRecentlyCopied "Copied to clipboard" "Copy to clipboard"}} + >app/components/course-page/share-progress-modal.ts (2)
Line range hint
33-41: Consider improving the copyableText implementation.The implementation could benefit from some refinements:
- The URL and platform-specific text could be extracted as constants
- Consider handling the case where language is undefined
Here's a suggested improvement:
+ private readonly BASE_URL = 'https://app.codecrafters.io/courses'; + private readonly PLATFORM_PREFIXES = { + twitter: '@codecraftersio', + default: 'CodeCrafters' + }; get copyableText() { + const platformPrefix = this.selectedSocialPlatform === 'twitter' + ? this.PLATFORM_PREFIXES.twitter + : this.PLATFORM_PREFIXES.default; + const language = this.args.repository.language?.name ?? 'selected language'; + const courseSlug = this.args.repository.course.slug; + return `I'm working on the ${platformPrefix} ${this.args.repository.course.name} challenge in ${language}.\n\n${this.BASE_URL}/${courseSlug}/overview`; }
Line range hint
1-65: Great job on centralizing clipboard functionality!The refactoring aligns well with the PR's objective of unifying clipboard interactions. The component now has a cleaner separation of concerns:
- UI state management (selected platform)
- Text content generation (copyableText)
- Analytics tracking
This makes the code more maintainable and testable.
app/components/course-admin/code-example-page/comparison-card.ts (2)
Line range hint
1-16: Consider adding JSDoc documentation.While the TypeScript interfaces provide good type safety, adding JSDoc comments would improve code maintainability by documenting:
- The component's purpose
- The significance of each argument
- Any important implementation details
Example:
/** * Renders a comparison card for displaying solution comparisons between two users. * * @component * @example * <CourseAdmin::CodeExamplePage::ComparisonCard * @comparison={{this.comparison}} * @firstUser={{this.user}} * /> */
Line range hint
20-77: Consider simplifying the user and solution getters.The component has multiple getters for handling user and solution comparisons. Consider refactoring to reduce complexity:
private getUserSolution(targetUser: UserModel) { const isFirstSolution = this.args.comparison.firstSolution.user.id === targetUser.id; return isFirstSolution ? this.args.comparison.firstSolution : this.args.comparison.secondSolution; } get solutionForFirstUser() { return this.getUserSolution(this.firstUser); } get solutionForSecondUser() { return this.getUserSolution(this.secondUser); }app/components/copyable-terminal-command.hbs (1)
34-48: Enhance accessibility for screen readers.Consider adding ARIA attributes to improve accessibility:
- <TertiaryButton @size="extra-small" {{on "click" performCopyTask}}> + <TertiaryButton + @size="extra-small" + aria-label="Copy command to clipboard" + {{on "click" performCopyTask}} + > <AnimatedContainer> {{#animated-if hasRecentlyCopied use=this.transition duration=200}} - <div class="flex items-center gap-1"> + <div class="flex items-center gap-1" role="status" aria-live="polite"> {{svg-jar "clipboard-check" class="h-4 w-4 text-teal-500"}} <span class="text-xs text-teal-500">copied!</span> </div>config/environment.js (1)
31-31: Consider organizing configuration properties and adding documentation.While the default timeout value is reasonable, consider:
- Grouping related UI configuration properties together
- Adding JSDoc comments to document the purpose and expected value range
x: { backendUrl: process.env.BACKEND_URL || 'https://test-backend.ngrok.io', + // UI Configuration + /** Duration (ms) to show "Copied!" confirmation message */ copyConfirmationTimeout: 1000, defaultMetaTags: {app/components/course-admin/code-example-page/comparison-card.hbs (1)
40-49: LGTM! Consider adding ARIA attributes for better accessibility.The integration of
CopyableAnythingis well implemented and aligns perfectly with the PR's objective of unifying clipboard interactions.Consider adding
aria-labelto improve screen reader experience:<button class="flex-shrink-0 border hover:bg-gray-100 hover:border-gray-300 transition-colors px-2 py-1.5 text-gray-700 font-bold text-xs rounded flex items-center" type="button" + aria-label={{if hasRecentlyCopied "ID copied to clipboard" "Copy ID to clipboard"}} {{on "click" performCopyTask}} >app/components/course-page/repository-dropdown.hbs (2)
45-81: Enhance accessibility of the copy button implementation.The implementation successfully integrates with the new
CopyableAnythingcomponent and maintains the existing UX. However, consider these accessibility improvements:
- Replace the
divwith a semanticbuttonelement:-<div +<button class="px-3 py-2 hover:bg-gray-50 dark:hover:bg-gray-700/50 flex group {{if @activeRepository.isNewAsBoolean 'text-gray-300 dark:text-gray-700'}}" role="button" {{on "click" performCopyTask}} data-test-dropdown-action ->
- Add
aria-labelto provide better context for screen readers:<button + aria-label="Copy Git Repository URL" class="px-3 py-2 hover:bg-gray-50 dark:hover:bg-gray-700/50 flex group
77-79: Move tooltip outside the clickable area.The tooltip is currently rendered inside the clickable area, which could interfere with the copy action. Consider moving it outside:
-{{#if @activeRepository.isNew}} - <EmberTooltip @text="Please select a language first" /> -{{/if}} </div> +{{#if @activeRepository.isNew}} + <EmberTooltip @text="Please select a language first" /> +{{/if}}app/models/repository.ts (1)
147-149: Add documentation for theisNewAsBooleangetter.While the implementation is correct, please add a JSDoc comment explaining:
- The purpose of this getter
- Its relationship to clipboard functionality
- The source of the
isNewproperty (inherited from Model)+ /** + * Converts the inherited `isNew` property to a boolean. + * Used by CopyableAnything component to control the disabled state of clipboard operations. + * + * @returns {boolean} True if the repository is new, false otherwise + */ get isNewAsBoolean() { return Boolean(this.isNew); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
- app/components/copyable-anything.hbs (1 hunks)
- app/components/copyable-anything.ts (1 hunks)
- app/components/copyable-code.hbs (1 hunks)
- app/components/copyable-code.ts (1 hunks)
- app/components/copyable-terminal-command.hbs (1 hunks)
- app/components/copyable-terminal-command.ts (1 hunks)
- app/components/course-admin/code-example-page/comparison-card.hbs (1 hunks)
- app/components/course-admin/code-example-page/comparison-card.ts (3 hunks)
- app/components/course-admin/code-example-page/evaluation-card/evaluation-tab.hbs (1 hunks)
- app/components/course-admin/code-example-page/evaluation-card/evaluation-tab.ts (1 hunks)
- app/components/course-admin/submissions-page/submission-details/header-container.hbs (3 hunks)
- app/components/course-admin/submissions-page/submission-details/header-container.ts (1 hunks)
- app/components/course-page/repository-dropdown.hbs (1 hunks)
- app/components/course-page/repository-dropdown.ts (3 hunks)
- app/components/course-page/share-progress-modal.hbs (1 hunks)
- app/components/course-page/share-progress-modal.ts (2 hunks)
- app/config/environment.d.ts (1 hunks)
- app/models/repository.ts (1 hunks)
- config/environment.js (2 hunks)
- tests/integration/components/copyable-anything-test.ts (1 hunks)
- tests/support/stub-clipboard.ts (1 hunks)
- tests/support/stub-local-storage.ts (1 hunks)
- tests/test-helper.js (2 hunks)
🔇 Additional comments (36)
app/components/copyable-anything.hbs (1)
1-1: LGTM! Consider adding TypeScript template validation.The implementation follows Ember.js best practices for component composition and async task handling. The yielded values align perfectly with the component's intended API.
Consider adding TypeScript template validation using
<template>tags to ensure type safety:+<template> {{yield (fn (perform this.copyTask)) this.hasRecentlyCopied}} +</template>Let's verify that all components using
CopyableAnythingare properly passing block content:app/components/copyable-code.ts (1)
13-13: Verify migration to CopyableAnything component.The component implementation has been removed as part of centralizing clipboard logic. Let's verify the corresponding template is using
CopyableAnything.✅ Verification successful
Migration to CopyableAnything component is properly implemented ✅
The template is correctly using the
CopyableAnythingcomponent for handling clipboard operations, with proper bindings for@valueand@onCopiedattributes, and appropriate usage of the yieldedperformCopyTaskandhasRecentlyCopiedvalues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the template is using CopyableAnything # Expect: CopyableAnything usage in the corresponding HBS template # Find and check the corresponding template fd --extension hbs copyable-code --exec cat {}Length of output: 987
app/components/course-admin/code-example-page/evaluation-card/evaluation-tab.ts (2)
2-3: LGTM! Clean import organization.The imports are now logically grouped and unnecessary imports have been removed, making the code more maintainable.
Line range hint
1-38: Verify template integration with CopyableAnything.The clipboard-related code has been correctly removed from this component. However, we should verify that the corresponding template file properly implements the new
CopyableAnythingcomponent.✅ Verification successful
Template correctly uses CopyableAnything component and has no direct clipboard interactions
The verification confirms that:
- The template file
evaluation-tab.hbsproperly implements theCopyableAnythingcomponent- No direct clipboard interactions (like
navigator.clipboardorcopyToClipboard) remain in the template🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the corresponding HBS template uses CopyableAnything # Test: Check if the template exists and uses CopyableAnything rg -g "*.hbs" "CopyableAnything" "app/components/course-admin/code-example-page/evaluation-card/" # Test: Ensure no direct clipboard interactions remain rg -g "*.hbs" "navigator\.clipboard|copyToClipboard" "app/components/course-admin/code-example-page/evaluation-card/"Length of output: 501
app/components/course-admin/code-example-page/evaluation-card/evaluation-tab.hbs (2)
3-7: Well-implemented clipboard interaction refactor!The integration of
CopyableAnythingis clean and maintains all the necessary functionality:
- Preserves the disabled state when no logs content is available
- Provides user feedback through the button text
- Maintains consistent UX with the existing button styling
3-7: Verify consistent clipboard implementation across components.Let's ensure this implementation follows the same pattern as other refactored components.
✅ Verification successful
Based on the search results, I can now generate the final response:
Implementation aligns with established clipboard patterns
The
CopyableAnythingcomponent usage inevaluation-tab.hbsfollows the consistent pattern found across the codebase:
- Uses the same
@valuebinding pattern- Yields
performCopyTaskandhasRecentlyCopiedconsistently- Implements similar button UI with copy state feedback
Other instances like
copyable-code.hbs,copyable-terminal-command.hbs, andshare-progress-modal.hbsfollow identical patterns, confirming this implementation is part of the unified clipboard interaction approach.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent CopyableAnything implementation pattern across components # Expected: Similar implementation pattern with @value, performCopyTask, and hasRecentlyCopied # Search for other CopyableAnything implementations echo "Checking CopyableAnything usage pattern across components:" rg -A 3 "@value=\{\{.*\}\}" --type hbs # Verify removal of old clipboard-related code echo -e "\nChecking for any remaining direct clipboard interactions:" rg -i "clipboard" --type tsLength of output: 21448
app/components/copyable-terminal-command.ts (2)
4-4: LGTM: Proper typing for DarkModeServiceThe explicit type import enhances type safety for the dark mode service.
11-11: LGTM: Enhanced type signature for async copy operationsThe updated signature properly supports both synchronous and asynchronous copy operations, which is essential for clipboard interactions.
Let's verify that all implementations of this callback are handling the async nature correctly:
tests/support/stub-clipboard.ts (2)
1-3: LGTM! Clean import and constant declaration.The import is appropriate, and the constant follows naming conventions.
16-34: Well-implemented clipboard stubbing with proper state management.The implementation correctly maintains clipboard state and handles all clipboard methods appropriately. The use of
splicefor cache updates ensures proper array mutation, and the optional chaining inreadTextprovides safe access to potentially undefined values.tests/test-helper.js (2)
9-11: LGTM! Import changes align with PR objectives.The new imports for
stubLocalStorageandstubClipboardhelpers, along with the switch toember-examfor test running, are well-structured changes that support the PR's goal of unifying clipboard interactions and improving test infrastructure.
21-22: Verify stub initialization order.The stubs are correctly placed in the
QUnit.testStarthook to ensure fresh stubs for each test. However, let's verify that there are no order-dependent side effects between these stubs.✅ Verification successful
No order dependency between localStorage and clipboard stubs
After examining the implementation of both stubs, I can confirm that they are completely independent of each other:
stubLocalStorageonly interacts withwindow.localStorageand its own cache MapstubClipboardonly interacts withnavigator.clipboardand its own cache Array- Neither stub references or depends on the other's functionality or state
The order of initialization in the
QUnit.testStarthook is safe as implemented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any order dependencies between localStorage and clipboard stubs # by looking for cross-references in their implementations echo "Checking stub implementations for dependencies..." # Look for clipboard references in localStorage stub rg -l "clipboard" "tests/support/stub-local-storage.ts" # Look for localStorage references in clipboard stub rg -l "localStorage" "tests/support/stub-clipboard.ts" # Look for test files that might rely on stub order rg -l "stubLocalStorage.*stubClipboard|stubClipboard.*stubLocalStorage" "tests"Length of output: 295
Script:
#!/bin/bash # Let's first check if these files exist and examine their content fd "stub-local-storage.ts|stub-clipboard.ts" tests/ # If not found with .ts extension, try .js fd "stub-local-storage.js|stub-clipboard.js" tests/ # Let's examine the actual test-helper.js content cat tests/test-helper.js # Look for any test files that use these stubs rg "stubLocalStorage|stubClipboard" tests/ -A 3 -B 3Length of output: 3801
Script:
#!/bin/bash # Let's examine the content of both stub files to understand their implementations cat tests/support/stub-local-storage.ts cat tests/support/stub-clipboard.ts # Let's also check if there are any test files that specifically test these stubs # or their interaction rg -l "describe.*stub.*Storage|describe.*stub.*Clipboard" tests/Length of output: 2697
tests/support/stub-local-storage.ts (1)
1-45: Implementation aligns well with PR objectives.The
stubLocalStoragehelper effectively complements the clipboard stubbing infrastructure mentioned in the PR objectives. The implementation is thorough and provides a solid foundation for testing components that interact with localStorage.app/components/copyable-anything.ts (4)
22-24: LGTM! Clean component setup.The component class is well-structured with proper typing and state tracking.
45-47: LGTM! Proper task configuration.The ember-concurrency task is well-configured with
keepLatestto handle rapid copy attempts gracefully.
50-54: LGTM! Proper Glint integration.The component is correctly registered with Glint for proper template type checking.
1-54: Verify complete migration to CopyableAnything.Let's ensure all direct clipboard.writeText usages are migrated to this new component.
✅ Verification successful
Let me verify the implementation details of the components using CopyableAnything to ensure they're using it correctly.
Based on the comprehensive review of the implementation, I can now provide the final response:
Migration to CopyableAnything component is complete and correct.
The verification shows that:
- The only direct
navigator.clipboard.writeTextusage is within the CopyableAnything component itself- All clipboard operations across the codebase are using the CopyableAnything component correctly
- The component is being used in various contexts with proper implementation:
- Terminal commands
- Code snippets
- Repository URLs
- Commit/Tree SHAs
- Progress sharing
- Evaluation logs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining direct clipboard API usage echo "Searching for direct clipboard API usage..." rg -l "navigator\.clipboard\.writeText" --type ts --type hbs # Search for CopyableAnything usage to verify adoption echo -e "\nVerifying CopyableAnything adoption..." rg -l "CopyableAnything" --type ts --type hbsLength of output: 838
Script:
#!/bin/bash # Check implementation of components using CopyableAnything echo "Checking copyable-terminal-command implementation..." cat app/components/copyable-terminal-command.hbs echo -e "\nChecking copyable-code implementation..." cat app/components/copyable-code.hbs echo -e "\nChecking repository-dropdown implementation..." cat app/components/course-page/repository-dropdown.hbs echo -e "\nChecking share-progress-modal implementation..." cat app/components/course-page/share-progress-modal.hbs echo -e "\nChecking comparison-card implementation..." cat app/components/course-admin/code-example-page/comparison-card.hbs echo -e "\nChecking header-container implementation..." cat app/components/course-admin/submissions-page/submission-details/header-container.hbs echo -e "\nChecking evaluation-tab implementation..." cat app/components/course-admin/code-example-page/evaluation-card/evaluation-tab.hbsLength of output: 26823
app/components/course-page/share-progress-modal.hbs (2)
27-27: Verify test coverage for clipboard interactions.Since this component handles clipboard operations, let's verify that the tests properly stub the clipboard API as mentioned in the PR objectives.
#!/bin/bash # Search for test files related to this component fd "share-progress-modal-test" "tests" # Check if the tests use the stubClipboard helper rg "stubClipboard" "tests" -A 5
30-30: Verify animation duration consistency across components.The animation duration is set to 100ms here. Let's verify this is consistent with other CopyableAnything implementations.
✅ Verification successful
Let me try a different search pattern to find animation duration usage across components.
Animation duration is consistent with similar copy-related components
The duration of 100ms in
share-progress-modalmatches other copy-related animations in the codebase:
dark-mode-toggle-optionuses 100msonboarding-survey-wizard/selectable-itemuses 100ms- Only
copyable-terminal-commanduses a slightly longer duration of 200msThe implementation appears to be consistent with the established patterns in the codebase, with most quick feedback animations using 100ms duration. Longer animations (300ms) are used for more complex transitions like expanding sections.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other animation duration settings in similar components rg -A 2 "duration=" "app/components" --type hbsLength of output: 1129
Script:
#!/bin/bash # Search for animated-if with duration parameter rg -A 2 "animated-if.*duration=" "app/components" --type hbs # Also search for other animation duration settings rg -A 2 "duration=" "app/components" --type js --type tsLength of output: 2334
app/components/course-page/share-progress-modal.ts (2)
3-7: LGTM! Import statements are well-organized.The imports are properly typed and follow best practices.
Line range hint
43-54: Verify CopyableAnything integration in the template.The action handlers look good, but let's ensure proper integration with CopyableAnything.
✅ Verification successful
CopyableAnything integration verified and properly implemented
The template shows correct integration with CopyableAnything component:
- Proper binding of
@value={{this.copyableText}}- Correct handler connection via
@onCopied={{this.handleCopyButtonClick}}- Appropriate usage of yielded properties
performCopyTaskandhasRecentlyCopied🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify CopyableAnything usage in the template # Expected: The template should use CopyableAnything with proper bindings rg -t hbs "CopyableAnything.*@value={{this.copyableText}}.*@onCopied={{this.handleCopyButtonClick}}" app/components/course-page/Length of output: 327
Script:
#!/bin/bash # Search for CopyableAnything usage in the template with proper escaping rg -t hbs "CopyableAnything" app/components/course-page/ -A 3 -B 3Length of output: 2505
app/components/course-admin/code-example-page/comparison-card.ts (2)
17-17: LGTM! Class renaming improves clarity.The renaming from
ComparisonCardtoComparisonCardComponentfollows better naming conventions by being more explicit about the component's nature. The change is consistently applied in both the class declaration and Glint registry.Also applies to: 81-81
Line range hint
1-81: Verify clipboard functionality migration.The removal of clipboard-related code aligns with the PR's objective to unify clipboard interactions. Let's verify that all clipboard functionality has been properly migrated to the
CopyableAnythingcomponent.✅ Verification successful
Let me check the actual template content to verify the clipboard functionality migration.
Clipboard functionality has been properly migrated to CopyableAnything
The template shows that clipboard functionality has been correctly migrated to use the
CopyableAnythingcomponent for copying the comparison ID. The only clipboard-related reference is to an icon (clipboard-check). No direct clipboard API usage remains in the component.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining direct clipboard API usage in the component's template # Test: Search for direct clipboard API usage rg -l "navigator\.clipboard" "app/components/course-admin/code-example-page/comparison-card" # Test: Verify CopyableAnything is properly imported in the template rg -l "CopyableAnything" "app/components/course-admin/code-example-page/comparison-card.hbs"Length of output: 351
Script:
#!/bin/bash # Check the template content to verify proper usage of CopyableAnything cat app/components/course-admin/code-example-page/comparison-card.hbs # Also search for any clipboard-related code in the component rg -g "comparison-card.*" "clipboard" app/components/course-admin/code-example-page/Length of output: 3235
app/components/copyable-terminal-command.hbs (1)
33-49: Implementation aligns well with PR objectives!The integration of CopyableAnything maintains the existing UX while standardizing clipboard interactions.
Let's verify the copyableText implementation:
✅ Verification successful
Implementation is correct and well-implemented
The
copyableTextgetter is properly implemented inapp/components/copyable-terminal-command.ts:get copyableText(): string { return this.args.commands.map((command) => command.replace(/# .*$/, '')).join('\n'); }It correctly processes the commands by stripping comments and joining them with newlines, making it suitable for clipboard operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of copyableText in the component's TypeScript file ast-grep --pattern 'copyableText = $$$'Length of output: 41
Script:
#!/bin/bash # Let's try to find the component's TypeScript/JavaScript file first fd "copyable-terminal-command" --type f # Then let's search for any getter or computed property definition for copyableText ast-grep --pattern 'get copyableText() { $$$ }' # Also search for class property definition ast-grep --pattern 'copyableText = $$$' # Backup search using ripgrep for any copyableText references rg "copyableText" -A 3 -B 3Length of output: 8761
app/components/course-admin/submissions-page/submission-details/header-container.ts (2)
4-4: LGTM! Modern service injection syntax.The update to use
import { service }instead ofimport { inject as service }aligns with current Ember.js best practices.
Line range hint
1-108: Verify template integration with CopyableAnything component.The removal of clipboard-related methods suggests that the corresponding template file now uses the
CopyableAnythingcomponent. Let's verify this integration:✅ Verification successful
Let me check the actual template content to verify the integration properly.
CopyableAnything component is properly integrated
The template shows correct integration of the
CopyableAnythingcomponent in multiple places:
- Git clone URL copying
- Commit SHA copying
- Tree SHA copying
Each integration follows the correct pattern using the component's block form with
performCopyTaskandhasRecentlyCopiedparameters. The clipboard icons (clipboard-list) are only used within theCopyableAnythingcontext for visual feedback, not for direct clipboard operations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the template file properly integrates with CopyableAnything # and that no direct clipboard operations remain. # Test 1: Check if the template uses CopyableAnything echo "Checking for CopyableAnything usage in the template:" rg -l "CopyableAnything" "app/components/course-admin/submissions-page/submission-details/header-container.hbs" # Test 2: Verify no direct clipboard operations remain echo "Checking for any remaining direct clipboard operations:" rg -i "clipboard" "app/components/course-admin/submissions-page/submission-details/header-container.hbs"Length of output: 783
Script:
#!/bin/bash # Get the template content to verify CopyableAnything integration echo "Template content:" cat "app/components/course-admin/submissions-page/submission-details/header-container.hbs"Length of output: 9645
config/environment.js (1)
88-88: Verify if 10ms is sufficient for test stability.While reducing the timeout in tests is good for performance, 10ms might be too aggressive and could lead to flaky tests if the copy operation or state updates take longer.
✅ Verification successful
Based on the search results, I can now provide a definitive response since we found the relevant test file that uses this timeout value.
10ms timeout for copy confirmation is sufficient based on test implementation
The
copyConfirmationTimeoutis only used intests/integration/components/copyable-anything-test.tsto verify the "COPIED!" text state changes. The test waits for half the timeout to check if "COPIED!" is shown, then waits for the full timeout to verify it reverts to "COPY". Since this is just for UI state changes and doesn't involve actual file operations or network calls, 10ms is adequate for this specific use case.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any timing-related test failures or flaky tests # Test: Search for timing-related test failures in git history rg -i "timeout|flaky|timing|race condition" -g "tests/**/*.{js,ts}" -A 5Length of output: 7714
app/components/course-page/repository-dropdown.ts (3)
3-3: LGTM! Clean import syntax.The simplified service import is more concise and follows modern Ember conventions.
23-23: LGTM! Clean component rename.The component rename and corresponding Glint registry update are well-aligned. The new name
RepositoryDropdownis more concise while maintaining clarity about its purpose.Also applies to: 100-100
Line range hint
1-102: Verify complete migration to CopyableAnything.The clipboard-related code has been removed from this component, but let's verify that the corresponding template properly implements the
CopyableAnythingcomponent for the repository URL copying functionality.✅ Verification successful
Migration to CopyableAnything is properly implemented
The template correctly implements the
CopyableAnythingcomponent for handling repository URL copying functionality. The implementation:
- Uses
CopyableAnythingcomponent with proper binding to@activeRepository.cloneUrl- Includes appropriate disabled state handling
- Shows copy feedback with animation
- Has removed all direct clipboard API usage
The migration is complete and correctly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the template implements CopyableAnything and old clipboard code is removed # Test 1: Check if the template uses CopyableAnything echo "Checking for CopyableAnything usage in template..." rg -l "CopyableAnything.*cloneUrl" "app/components/course-page/repository-dropdown.hbs" # Test 2: Verify no direct clipboard API usage remains echo "Checking for any remaining direct clipboard API usage..." rg -l "navigator\.clipboard|writeText" "app/components/course-page/repository-dropdown.hbs"Length of output: 413
Script:
#!/bin/bash # Let's first check if the template file exists and see its content echo "Checking if template file exists..." fd "repository-dropdown.hbs" app/components/course-page/ echo -e "\nChecking template content..." cat app/components/course-page/repository-dropdown.hbs # Let's also check for any clipboard-related components or helpers in the template echo -e "\nChecking for any clipboard-related patterns..." rg -p "copy|clipboard|CopyableAnything" app/components/course-page/repository-dropdown.hbsLength of output: 7557
tests/integration/components/copyable-anything-test.ts (2)
1-10: LGTM: Proper test setup with all necessary imports.The test file is properly configured with all required testing utilities and follows Ember testing best practices.
1-79: Verify test coverage for all clipboard interaction scenarios.While the test suite covers the core functionality well, consider adding these test cases based on the PR objectives:
- Test error handling when clipboard API is not available
- Test concurrent copy operations
- Test with various content types (text, rich text, etc.)
app/components/course-admin/submissions-page/submission-details/header-container.hbs (3)
69-74: LGTM! Clean implementation of repository URL copying.The integration of
CopyableAnythingwith the repository URL copy button is well-implemented, maintaining the existing UX while standardizing clipboard interactions.
105-110: LGTM! Consistent implementation of commit SHA copying.The implementation maintains consistency with other copy buttons and preserves test attributes.
120-133: 🛠️ Refactor suggestionConsider propagating the disabled state to CopyableAnything.
While the button is correctly disabled when the tree SHA is "Unknown", the
CopyableAnythingcomponent might still attempt to copy when clicked. Consider passing the disabled state toCopyableAnythingto prevent copying attempts when the value is unavailable.Let's verify if
CopyableAnythingsupports the disabled state:Consider updating the implementation to:
- <CopyableAnything @value={{@submission.treeSha}} as |performCopyTask hasRecentlyCopied|> + <CopyableAnything + @value={{@submission.treeSha}} + @isDisabled={{eq this.shortSubmissionTreeSha "Unknown"}} + as |performCopyTask hasRecentlyCopied|>✅ Verification successful
Based on the search results, I can now generate the final response since I found evidence that the
CopyableAnythingcomponent does support theisDisabledproperty and properly handles it:
- The interface in
app/components/copyable-anything.tsshows it accepts anisDisabledprop:export interface CopyableAnythingSignature { Args: { value?: string | null; isDisabled?: boolean; onCopied?: OnCopiedCallback; }; }
- The component implementation shows it respects the disabled state in its copy method:
async copy(): Promise<void> { if (this.args.isDisabled) { return; } // ... }
- The test file
tests/integration/components/copyable-anything-test.tsconfirms this behavior:test('it does nothing when `@isDisabled` is true', async function (assert) { // ... assert.ok(callback.notCalled, 'callback was not called when @isDisabled is true'); assert.strictEqual(await navigator.clipboard.readText(), '', 'clipboard is not written when @isDisabled is true'); });The review comment is correct. The disabled state should be propagated to CopyableAnything.
When the tree SHA is "Unknown", the button is correctly disabled but the
CopyableAnythingwrapper can still attempt to copy. The suggested fix to pass the disabled state toCopyableAnythingis the right approach to prevent any copy attempts when the value is unavailable.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if CopyableAnything component supports @isDisabled property ast-grep --pattern 'interface CopyableAnythingArgs { $$$ isDisabled?: boolean; $$$ }'Length of output: 93
Script:
#!/bin/bash # Let's check the implementation of CopyableAnything component fd "copyable-anything" --type f # Then examine its contents rg -l "CopyableAnything" --type ts --type js --type hbs # Look for any disabled state handling in the component rg "isDisabled|disabled" -A 3 -B 3 --type ts --type js --type hbsLength of output: 105458
app/models/repository.ts (1)
147-149: Verify the usage ofisNewAsBooleanin UI components.The implementation looks good. Let's verify its integration with
CopyableAnything.✅ Verification successful
Let me verify the implementation of CopyableAnything to ensure it's being used correctly.
Let me check the component implementation to understand how isDisabled is handled.
Integration of
isNewAsBooleanwithCopyableAnythingis correctly implementedThe implementation is properly integrated:
CopyableAnythingcomponent correctly handles theisDisabledprop in its implementation- The repository dropdown uses
isNewAsBooleanto disable the clone URL copying functionality when a new repository is being created- The disabled state is consistently reflected in both the functionality and UI styling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that isNewAsBoolean is used correctly with CopyableAnything # Test 1: Find all usages of isNewAsBoolean echo "Finding all usages of isNewAsBoolean..." rg "isNewAsBoolean" # Test 2: Specifically look for usage with CopyableAnything's isDisabled property echo "Checking usage with CopyableAnything..." rg -A 3 "CopyableAnything" | rg "isDisabled.*isNewAsBoolean"Length of output: 669
Script:
#!/bin/bash # Check the implementation of CopyableAnything component ast-grep --pattern 'class CopyableAnything { $$$ }' # Find the template for CopyableAnything fd -e hbs -e handlebars "copyable-anything" # If found, show its contents fd -e hbs -e handlebars "copyable-anything" -x cat {}Length of output: 254
Script:
#!/bin/bash # Check the component implementation fd -e ts -e js "copyable-anything" -x cat {} # Also check the repository-dropdown template where it's being used cat app/components/course-page/repository-dropdown.hbsLength of output: 12263
|
|
||
| interface Signature { | ||
| Element: HTMLDivElement; | ||
|
|
||
| Args: { | ||
| backgroundColor?: 'gray' | 'white'; | ||
| code: string; | ||
| onCopyButtonClick?: () => void; | ||
| onCopyButtonClick?: () => void | Promise<void>; |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent callback signatures found across copyable components
The Promise return type is not consistently implemented across copyable components:
CopyableCode:onCopyButtonClick?: () => void | Promise<void>CopyableTerminalCommand:onCopyButtonClick?: () => void | Promise<void>CopyableTerminalCommandWithVariants:onCopyButtonClick?: () => void(missing Promise)CopyableAnything: UsesOnCopiedCallback = () => void | Promise<void>type
The callback signature should be unified across all copyable components to maintain consistency, especially since this is part of a refactor to unify clipboard interactions.
🔗 Analysis chain
LGTM: Method signature update allows for async callbacks.
The change to allow Promise<void> return type is a good improvement that supports async operations while maintaining backward compatibility.
Let's verify this change is consistently applied across other copyable components:
The previous ast-grep pattern might have been too specific. Let's try a broader search to:
- Find all relevant files first
- Look for any copy-related callback signatures to verify consistency
The previous search was too broad. Let's focus specifically on the copyable components to verify the consistency of the callback signatures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other copyable components have similar callback signatures
# Expect: Consistent Promise<void> | void signatures across components
ast-grep --pattern 'interface $_ {
$$$
onCopyButtonClick?: () => $ret;
$$$
}'
Length of output: 85
Script:
#!/bin/bash
# Let's try a broader search for any interfaces or types with copy-related callbacks
# First, let's find all TypeScript files that might contain copyable components
fd -e ts -e tsx | xargs rg -l "copy|clipboard" --ignore-case
# Then let's search for method signatures related to copy operations
rg "(?:on[A-Z][a-zA-Z]*Copy|copy.*\?:.*=>.*)" -A 2 -B 2
Length of output: 2108
Script:
#!/bin/bash
# Let's specifically look at the copyable components implementations
rg -A 5 -B 5 "onCopyButtonClick|CopyableAnything|CopyableCode" app/components/copyable-*.ts
# Also check the interfaces/types in these files
ast-grep --pattern 'interface $_ {
$$$
}' app/components/copyable-*.ts
Length of output: 8455
| stub(navigator.clipboard, 'readText').callsFake(async function () { | ||
| return (await cache[0]?.getType(TEXT_MIME_TYPE))?.text() || ''; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for invalid clipboard items.
The readText implementation could benefit from explicit error handling for malformed clipboard items.
Consider this implementation:
stub(navigator.clipboard, 'readText').callsFake(async function () {
- return (await cache[0]?.getType(TEXT_MIME_TYPE))?.text() || '';
+ try {
+ const item = cache[0];
+ if (!item) return '';
+ const blob = await item.getType(TEXT_MIME_TYPE);
+ if (!blob) return '';
+ return await blob.text();
+ } catch (error) {
+ console.error('Error reading clipboard text:', error);
+ return '';
+ }
});📝 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.
| stub(navigator.clipboard, 'readText').callsFake(async function () { | |
| return (await cache[0]?.getType(TEXT_MIME_TYPE))?.text() || ''; | |
| }); | |
| stub(navigator.clipboard, 'readText').callsFake(async function () { | |
| try { | |
| const item = cache[0]; | |
| if (!item) return ''; | |
| const blob = await item.getType(TEXT_MIME_TYPE); | |
| if (!blob) return ''; | |
| return await blob.text(); | |
| } catch (error) { | |
| console.error('Error reading clipboard text:', error); | |
| return ''; | |
| } | |
| }); |
| export default function stubLocalStorage(cache: Map<string, string> = new Map<string, string>()): Map<string, string> { | ||
| stub(window.localStorage, 'getItem').callsFake(function (key) { | ||
| if (cache.has(key)) { | ||
| return String(cache.get(key)); | ||
| } else { | ||
| return null; | ||
| } | ||
| }); | ||
|
|
||
| stub(window.localStorage, 'setItem').callsFake(function (key, value) { | ||
| cache.set(key, value); | ||
| }); | ||
|
|
||
| stub(window.localStorage, 'removeItem').callsFake(function (key) { | ||
| cache.delete(key); | ||
| }); | ||
|
|
||
| stub(window.localStorage, 'clear').callsFake(function () { | ||
| cache.clear(); | ||
| }); | ||
|
|
||
| stub(window.localStorage, 'key').callsFake(function (index) { | ||
| return [...cache.keys()][index] || null; | ||
| }); | ||
|
|
||
| stub(window.localStorage['__proto__'], 'length').get(function () { | ||
| return cache.size; | ||
| }); | ||
|
|
||
| return cache; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding QuotaExceededError simulation.
The stub could be more realistic by simulating storage quota errors that can occur in real localStorage.
Consider adding a configurable storage limit:
export default function stubLocalStorage(
cache: Map<string, string> = new Map<string, string>(),
options: { quotaInBytes?: number } = {}
): Map<string, string> {
const calculateSize = () =>
[...cache.entries()].reduce((size, [k, v]) =>
size + k.length + v.length, 0);
stub(window.localStorage, 'setItem').callsFake(function (key, value) {
const newSize = calculateSize() + String(key).length + String(value).length;
if (options.quotaInBytes && newSize > options.quotaInBytes) {
throw new DOMException('QuotaExceededError', 'QuotaExceededError');
}
cache.set(String(key), String(value));
});
// ... rest of the implementation
}| stub(window.localStorage, 'setItem').callsFake(function (key, value) { | ||
| cache.set(key, value); | ||
| }); |
There was a problem hiding this comment.
Enforce string conversion in setItem stub.
The real localStorage.setItem automatically converts values to strings, but this stub doesn't replicate that behavior.
Apply this fix:
stub(window.localStorage, 'setItem').callsFake(function (key, value) {
- cache.set(key, value);
+ cache.set(String(key), String(value));
});📝 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.
| stub(window.localStorage, 'setItem').callsFake(function (key, value) { | |
| cache.set(key, value); | |
| }); | |
| stub(window.localStorage, 'setItem').callsFake(function (key, value) { | |
| cache.set(String(key), String(value)); | |
| }); |
| stub(window.localStorage, 'key').callsFake(function (index) { | ||
| return [...cache.keys()][index] || null; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize key method implementation for large datasets.
Using spread operator with cache.keys() creates a temporary array of all keys, which could be inefficient for large datasets.
Consider this more efficient implementation:
stub(window.localStorage, 'key').callsFake(function (index) {
- return [...cache.keys()][index] || null;
+ if (index < 0 || index >= cache.size) return null;
+ let i = 0;
+ for (const key of cache.keys()) {
+ if (i === index) return key;
+ i++;
+ }
+ return null;
});📝 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.
| stub(window.localStorage, 'key').callsFake(function (index) { | |
| return [...cache.keys()][index] || null; | |
| }); | |
| stub(window.localStorage, 'key').callsFake(function (index) { | |
| if (index < 0 || index >= cache.size) return null; | |
| let i = 0; | |
| for (const key of cache.keys()) { | |
| if (i === index) return key; | |
| i++; | |
| } | |
| return null; | |
| }); |
| await navigator.clipboard.writeText(String(this.args.value)); | ||
| this.hasRecentlyCopied = true; |
There was a problem hiding this comment.
Add error handling for clipboard API.
The clipboard API can fail in various scenarios (permissions, secure context). Consider adding try-catch and user feedback.
- await navigator.clipboard.writeText(String(this.args.value));
- this.hasRecentlyCopied = true;
+ try {
+ await navigator.clipboard.writeText(String(this.args.value));
+ this.hasRecentlyCopied = true;
+ } catch (error) {
+ console.error('Failed to copy to clipboard:', error);
+ // Optionally trigger error feedback
+ return;
+ }📝 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.
| await navigator.clipboard.writeText(String(this.args.value)); | |
| this.hasRecentlyCopied = true; | |
| try { | |
| await navigator.clipboard.writeText(String(this.args.value)); | |
| this.hasRecentlyCopied = true; | |
| } catch (error) { | |
| console.error('Failed to copy to clipboard:', error); | |
| // Optionally trigger error feedback | |
| return; | |
| } |
| test('it calls an optional `@onCopied` callback after copying', async function (assert) { | ||
| const callback = spy(); | ||
| this.set('onCopiedCallback', callback); | ||
| await render(hbs` | ||
| {{! @glint-expect-error it thinks onCopiedCallback is not defined}} | ||
| <CopyableAnything @onCopied={{this.onCopiedCallback}} as |performCopyTask|> | ||
| <button data-test-copy-button type="button" {{on "click" performCopyTask}} /> | ||
| </CopyableAnything> | ||
| `); | ||
| await click('[data-test-copy-button]'); | ||
| assert.ok(callback.calledOnce, 'callback was called once'); | ||
| }); |
There was a problem hiding this comment.
Fix the Glint type error instead of suppressing it.
The @glint-expect-error comment suggests a typing issue. Instead of suppressing it, consider fixing the root cause:
- Define proper types for the component's arguments
- Ensure the test context type includes the callback property
Add proper type definitions:
interface TestContext {
onCopiedCallback: () => void;
}
test('it calls an optional `@onCopied` callback after copying', async function (this: TestContext, assert) {
const callback = spy();
this.onCopiedCallback = callback;
// ... rest of the test
});| /** | ||
| * Requires browser to be in focus | ||
| */ | ||
| test('it does nothing when `@isDisabled` is true', async function (assert) { | ||
| const callback = spy(); | ||
| this.set('onCopiedCallback', callback); | ||
| await render(hbs` | ||
| <CopyableAnything | ||
| @value="Test value for copying" | ||
| @isDisabled={{true}} | ||
| {{! @glint-expect-error it thinks onCopiedCallback is not defined}} | ||
| @onCopied={{this.onCopiedCallback}} | ||
| as |performCopyTask hasRecentlyCopied|> | ||
| <button data-test-copy-button type="button" {{on "click" performCopyTask}} /> | ||
| <span data-test-copy-button-text>{{if hasRecentlyCopied "COPIED!" "COPY"}}</span> | ||
| </CopyableAnything> | ||
| `); | ||
| click('[data-test-copy-button]'); // Do not await | ||
| await timeout(config.x.copyConfirmationTimeout / 2); // Await for hasRecentlyCopied to propagate | ||
| assert.dom('[data-test-copy-button-text]').hasText('COPY'); | ||
| await timeout(config.x.copyConfirmationTimeout); // Await for hasRecentlyCopied to revert | ||
| assert.dom('[data-test-copy-button-text]').hasText('COPY'); | ||
| assert.ok(callback.notCalled, 'callback was not called when @isDisabled is true'); | ||
| assert.strictEqual(await navigator.clipboard.readText(), '', 'clipboard is not written when @isDisabled is true'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance disabled state test and maintain consistency.
The test is comprehensive but could be improved:
- Add assertion for button's disabled state
- Fix the Glint type error (same as previous test)
- Add aria-disabled attribute check
test('it does nothing when `@isDisabled` is true', async function (assert) {
const callback = spy();
this.set('onCopiedCallback', callback);
await render(hbs`
<CopyableAnything
@value="Test value for copying"
@isDisabled={{true}}
- {{! @glint-expect-error it thinks onCopiedCallback is not defined}}
@onCopied={{this.onCopiedCallback}}
as |performCopyTask hasRecentlyCopied|>
- <button data-test-copy-button type="button" {{on "click" performCopyTask}} />
+ <button
+ data-test-copy-button
+ type="button"
+ aria-disabled="true"
+ {{on "click" performCopyTask}}
+ />
<span data-test-copy-button-text>{{if hasRecentlyCopied "COPIED!" "COPY"}}</span>
</CopyableAnything>
`);
+ assert.dom('[data-test-copy-button]').hasAttribute('aria-disabled', 'true', 'Button should have aria-disabled attribute');
click('[data-test-copy-button]'); // Do not await
await timeout(config.x.copyConfirmationTimeout / 2); // Await for hasRecentlyCopied to propagate
assert.dom('[data-test-copy-button-text]').hasText('COPY');Committable suggestion was skipped due to low confidence.
| test('it copies passed `@value` to clipboard when yielded `performCopyTask` is called', async function (assert) { | ||
| await render(hbs` | ||
| <CopyableAnything @value="Test value for copying" as |performCopyTask|> | ||
| <button data-test-copy-button type="button" {{on "click" performCopyTask}} /> | ||
| </CopyableAnything> | ||
| `); | ||
| assert.strictEqual(await navigator.clipboard.readText(), '', 'clipboard is initially empty'); | ||
| await click('[data-test-copy-button]'); | ||
| assert.strictEqual(await navigator.clipboard.readText(), 'Test value for copying', 'clipboard contains expected text'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the basic copy test with additional checks.
While the test covers the basic functionality well, consider these improvements:
- Add assertion to verify button is enabled before clicking
- Add accessibility attributes (aria-label) to the button
- Add error handling for clipboard permission denial
test('it copies passed `@value` to clipboard when yielded `performCopyTask` is called', async function (assert) {
await render(hbs`
<CopyableAnything @value="Test value for copying" as |performCopyTask|>
- <button data-test-copy-button type="button" {{on "click" performCopyTask}} />
+ <button
+ data-test-copy-button
+ type="button"
+ aria-label="Copy to clipboard"
+ {{on "click" performCopyTask}}
+ />
</CopyableAnything>
`);
+ assert.dom('[data-test-copy-button]').isEnabled('Copy button should be enabled');
assert.strictEqual(await navigator.clipboard.readText(), '', 'clipboard is initially empty');
- await click('[data-test-copy-button]');
+ try {
+ await click('[data-test-copy-button]');
+ } catch (error) {
+ assert.notOk(true, 'Failed to copy: Clipboard permission denied');
+ }
assert.strictEqual(await navigator.clipboard.readText(), 'Test value for copying', 'clipboard contains expected text');
});📝 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.
| test('it copies passed `@value` to clipboard when yielded `performCopyTask` is called', async function (assert) { | |
| await render(hbs` | |
| <CopyableAnything @value="Test value for copying" as |performCopyTask|> | |
| <button data-test-copy-button type="button" {{on "click" performCopyTask}} /> | |
| </CopyableAnything> | |
| `); | |
| assert.strictEqual(await navigator.clipboard.readText(), '', 'clipboard is initially empty'); | |
| await click('[data-test-copy-button]'); | |
| assert.strictEqual(await navigator.clipboard.readText(), 'Test value for copying', 'clipboard contains expected text'); | |
| }); | |
| test('it copies passed `@value` to clipboard when yielded `performCopyTask` is called', async function (assert) { | |
| await render(hbs` | |
| <CopyableAnything @value="Test value for copying" as |performCopyTask|> | |
| <button | |
| data-test-copy-button | |
| type="button" | |
| aria-label="Copy to clipboard" | |
| {{on "click" performCopyTask}} | |
| /> | |
| </CopyableAnything> | |
| `); | |
| assert.dom('[data-test-copy-button]').isEnabled('Copy button should be enabled'); | |
| assert.strictEqual(await navigator.clipboard.readText(), '', 'clipboard is initially empty'); | |
| try { | |
| await click('[data-test-copy-button]'); | |
| } catch (error) { | |
| assert.notOk(true, 'Failed to copy: Clipboard permission denied'); | |
| } | |
| assert.strictEqual(await navigator.clipboard.readText(), 'Test value for copying', 'clipboard contains expected text'); | |
| }); |
| /** | ||
| * Requires browser to be in focus | ||
| */ | ||
| test('it yields a `hasRecentlyCopied` boolean, true for a configured interval after copying', async function (assert) { | ||
| await render(hbs` | ||
| <CopyableAnything as |performCopyTask hasRecentlyCopied|> | ||
| <button data-test-copy-button type="button" {{on "click" performCopyTask}} /> | ||
| <span data-test-copy-button-text>{{if hasRecentlyCopied "COPIED!" "COPY"}}</span> | ||
| </CopyableAnything> | ||
| `); | ||
| assert.dom('[data-test-copy-button-text]').hasText('COPY'); | ||
| click('[data-test-copy-button]'); // Do not await | ||
| await timeout(config.x.copyConfirmationTimeout / 2); // Await for hasRecentlyCopied to propagate | ||
| assert.dom('[data-test-copy-button-text]').hasText('COPIED!'); | ||
| await timeout(config.x.copyConfirmationTimeout); // Await for hasRecentlyCopied to revert | ||
| assert.dom('[data-test-copy-button-text]').hasText('COPY'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test reliability for copy indicator state changes.
The test could be flaky due to browser focus requirement and timing dependencies. Consider these improvements:
- Add retry logic for focus-dependent operations
- Add assertions to verify intermediate states
- Document why browser focus is required
- /**
- * Requires browser to be in focus
- */
+ /**
+ * This test requires browser focus because clipboard operations are only
+ * allowed when the document has focus for security reasons.
+ * @see https://w3c.github.io/clipboard-apis/#privacy-considerations
+ */
test('it yields a `hasRecentlyCopied` boolean, true for a configured interval after copying', async function (assert) {
await render(hbs`
<CopyableAnything as |performCopyTask hasRecentlyCopied|>
<button data-test-copy-button type="button" {{on "click" performCopyTask}} />
<span data-test-copy-button-text>{{if hasRecentlyCopied "COPIED!" "COPY"}}</span>
</CopyableAnything>
`);
assert.dom('[data-test-copy-button-text]').hasText('COPY');
+ assert.dom('[data-test-copy-button]').isEnabled('Copy button should be enabled');
click('[data-test-copy-button]'); // Do not await
await timeout(config.x.copyConfirmationTimeout / 2); // Await for hasRecentlyCopied to propagate
assert.dom('[data-test-copy-button-text]').hasText('COPIED!');
+ assert.dom('[data-test-copy-button]').isEnabled('Button should remain enabled after copy');
await timeout(config.x.copyConfirmationTimeout); // Await for hasRecentlyCopied to revert
assert.dom('[data-test-copy-button-text]').hasText('COPY');
});📝 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.
| /** | |
| * Requires browser to be in focus | |
| */ | |
| test('it yields a `hasRecentlyCopied` boolean, true for a configured interval after copying', async function (assert) { | |
| await render(hbs` | |
| <CopyableAnything as |performCopyTask hasRecentlyCopied|> | |
| <button data-test-copy-button type="button" {{on "click" performCopyTask}} /> | |
| <span data-test-copy-button-text>{{if hasRecentlyCopied "COPIED!" "COPY"}}</span> | |
| </CopyableAnything> | |
| `); | |
| assert.dom('[data-test-copy-button-text]').hasText('COPY'); | |
| click('[data-test-copy-button]'); // Do not await | |
| await timeout(config.x.copyConfirmationTimeout / 2); // Await for hasRecentlyCopied to propagate | |
| assert.dom('[data-test-copy-button-text]').hasText('COPIED!'); | |
| await timeout(config.x.copyConfirmationTimeout); // Await for hasRecentlyCopied to revert | |
| assert.dom('[data-test-copy-button-text]').hasText('COPY'); | |
| }); | |
| /** | |
| * This test requires browser focus because clipboard operations are only | |
| * allowed when the document has focus for security reasons. | |
| * @see https://w3c.github.io/clipboard-apis/#privacy-considerations | |
| */ | |
| test('it yields a `hasRecentlyCopied` boolean, true for a configured interval after copying', async function (assert) { | |
| await render(hbs` | |
| <CopyableAnything as |performCopyTask hasRecentlyCopied|> | |
| <button data-test-copy-button type="button" {{on "click" performCopyTask}} /> | |
| <span data-test-copy-button-text>{{if hasRecentlyCopied "COPIED!" "COPY"}}</span> | |
| </CopyableAnything> | |
| `); | |
| assert.dom('[data-test-copy-button-text]').hasText('COPY'); | |
| assert.dom('[data-test-copy-button]').isEnabled('Copy button should be enabled'); | |
| click('[data-test-copy-button]'); // Do not await | |
| await timeout(config.x.copyConfirmationTimeout / 2); // Await for hasRecentlyCopied to propagate | |
| assert.dom('[data-test-copy-button-text]').hasText('COPIED!'); | |
| assert.dom('[data-test-copy-button]').isEnabled('Button should remain enabled after copy'); | |
| await timeout(config.x.copyConfirmationTimeout); // Await for hasRecentlyCopied to revert | |
| assert.dom('[data-test-copy-button-text]').hasText('COPY'); | |
| }); |
|
Dismissing review, since our plan was to make a more minimal changeset |
Brief
This unifies all interactions with the Clipboard by adding a new component
CopyableAnything, and by using it in all components that previously interacted withnavigator.clipboardmanually (custom implementations in approximately 9 different places, each slightly different).In addition, when running the tests suite, all
navigator.clipboard.*API methods are stubbed with Sinon fakes before each test, and un-stubbed after, thus preventing any potential side-effects.The timeout of the "Copied!" message shown after copying is also reduced from
1000to10milliseconds in thetestenvironment.Details
CopyableAnythingcomponent:@valuevalue to copy to the clipboard@onCopiedcallback to call after copying@isDisabledwhentrue, calling yieldedperformCopyTaskdoes nothingperformCopyTask— call this to actually copy passed@valueto clipboardhasRecentlyCopied— will betruefor a configured time after copyingnavigator.clipboard.writeTextto useCopyableAnythingcomponent instead:CopyableCodeCopyableTerminalCommandComparisonCardEvaluationTabHeaderContainerRepositoryDropdownShareProgressModalstubClipboardtest helper:navigator.clipboard.writeTextintestenvironmentif (config.environment === 'test')in the codecopyConfirmationTimeout: 1000to config, set it to10in intestenvironmentlocalStorage-stubbing logic into a test helperstubLocalStorageChecklist:
[percy]in the message to trigger)Summary by CodeRabbit
Release Notes
New Features
CopyableAnythingcomponent for enhanced clipboard functionality across various components.copyConfirmationTimeoutfor customizable copy feedback duration.Bug Fixes
Refactor
CopyableAnything.Tests
CopyableAnythingcomponent to validate copy functionality and state management.