Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unify all interactions with Clipboard & stub all Clipboard methods during tests #2365

Closed
wants to merge 17 commits into from
Closed

Unify all interactions with Clipboard & stub all Clipboard methods during tests #2365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
2ecd63a
Extract `localStorage`-stubbing logic into a test helper `stubLocalSt…
VasylMarchuk Oct 27, 2024
81adb5f
Add `stubClipboard` test helper and run it before each test
VasylMarchuk Oct 27, 2024
6906d33
Call `navigator.clipboard.writeText` even when environment is `test`
VasylMarchuk Oct 27, 2024
2c47331
Add `copyConfirmationTimeout: 1000` to config, use it everywhere, set…
VasylMarchuk Oct 27, 2024
5342288
Rewrite all actions using `navigator.clipboard.writeText` as tasks
VasylMarchuk Oct 27, 2024
2a59e05
Add a new service `clipboard` to unify interactions with the clipboard
VasylMarchuk Oct 28, 2024
fcd7f4f
Modify `CopyableCodeComponent` to use `ClipboardService`
VasylMarchuk Oct 28, 2024
8292b6e
Modify `CopyableTerminalCommandComponent` to use `ClipboardService`
VasylMarchuk Oct 28, 2024
ee17ef8
Modify `ComparisonCardComponent` to use `ClipboardService`
VasylMarchuk Oct 28, 2024
8711660
Modify `EvaluationTabComponent` to use `ClipboardService`
VasylMarchuk Oct 28, 2024
5127156
Modify `HeaderContainerComponent` to use `ClipboardService`
VasylMarchuk Oct 28, 2024
9042935
Modify `RepositoryDropdownComponent` to use `ClipboardService`
VasylMarchuk Oct 28, 2024
806d563
Modify `ShareProgressModalComponent` to use `ClipboardService`
VasylMarchuk Oct 28, 2024
bd60c2e
Replace `clipboard` service with `CopyableAnything` component
VasylMarchuk Oct 28, 2024
6cd77fe
Inline a const in `stubClipboard` test helper
VasylMarchuk Oct 28, 2024
2bb6017
Add links to Clipboard & Storage API documentation
VasylMarchuk Oct 28, 2024
2d1c80e
Add tests for `CopyableAnything` component
VasylMarchuk Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/components/copyable-anything.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{{yield (fn (perform this.copyTask)) this.hasRecentlyCopied}}
54 changes: 54 additions & 0 deletions app/components/copyable-anything.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import Component from '@glimmer/component';
import { action } from '@ember/object';
import { waitFor } from '@ember/test-waiters';
import { tracked } from '@glimmer/tracking';
import { task, timeout } from 'ember-concurrency';
import config from 'codecrafters-frontend/config/environment';

export type OnCopiedCallback = () => void | Promise<void>;

export interface CopyableAnythingSignature {
Args: {
value?: string | null;
isDisabled?: boolean;
onCopied?: OnCopiedCallback;
};
Blocks: {
default: [() => void, boolean];
};
Element: null;
}

export default class CopyableAnythingComponent extends Component<CopyableAnythingSignature> {
@tracked hasRecentlyCopied: boolean = false;

@action
@waitFor
async copy(): Promise<void> {
if (this.args.isDisabled) {
return;
}

await navigator.clipboard.writeText(String(this.args.value));
this.hasRecentlyCopied = true;
Comment on lines +32 to +33
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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;
}


try {
if (this.args.onCopied) {
await this.args.onCopied();
}
} finally {
await timeout(config.x.copyConfirmationTimeout);
this.hasRecentlyCopied = false;
}
}
Comment on lines +35 to +43
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Protect timeout from callback errors.

If onCopied throws an error, the finally block's timeout could create a race condition with subsequent copy attempts.

     try {
       if (this.args.onCopied) {
         await this.args.onCopied();
       }
+    } catch (error) {
+      console.error('onCopied callback failed:', error);
     } finally {
-      await timeout(config.x.copyConfirmationTimeout);
-      this.hasRecentlyCopied = false;
+      try {
+        await timeout(config.x.copyConfirmationTimeout);
+      } finally {
+        this.hasRecentlyCopied = false;
+      }
     }
📝 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.

Suggested change
try {
if (this.args.onCopied) {
await this.args.onCopied();
}
} finally {
await timeout(config.x.copyConfirmationTimeout);
this.hasRecentlyCopied = false;
}
}
try {
if (this.args.onCopied) {
await this.args.onCopied();
}
} catch (error) {
console.error('onCopied callback failed:', error);
} finally {
try {
await timeout(config.x.copyConfirmationTimeout);
} finally {
this.hasRecentlyCopied = false;
}
}
}


copyTask = task({ keepLatest: true }, async (): Promise<void> => {
await this.copy();
});
}

declare module '@glint/environment-ember-loose/registry' {
export default interface Registry {
CopyableAnything: typeof CopyableAnythingComponent;
}
}
18 changes: 10 additions & 8 deletions app/components/copyable-code.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,16 @@
</div>

<div class="relative flex items-stretch">
<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" this.handleCopyButtonClick}}
data-test-copy-button
>
{{if this.codeWasCopiedRecently "COPIED" "COPY"}}
</button>
<CopyableAnything @value={{@code}} @onCopied={{@onCopyButtonClick}} as |performCopyTask hasRecentlyCopied|>
<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
>
{{if hasRecentlyCopied "COPIED" "COPY"}}
</button>
</CopyableAnything>
<div class="absolute top-px bottom-px -left-6 w-6 bg-gray-300 border-text-mask">
</div>
</div>
Expand Down
31 changes: 2 additions & 29 deletions app/components/copyable-code.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,43 +1,16 @@
import Component from '@glimmer/component';
import config from 'codecrafters-frontend/config/environment';
import { action } from '@ember/object';
import { later } from '@ember/runloop';
import { tracked } from '@glimmer/tracking';

interface Signature {
Element: HTMLDivElement;

Args: {
backgroundColor?: 'gray' | 'white';
code: string;
onCopyButtonClick?: () => void;
onCopyButtonClick?: () => void | Promise<void>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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: Uses OnCopiedCallback = () => 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:

  1. Find all relevant files first
  2. 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

};
}

export default class CopyableCodeComponent extends Component<Signature> {
@tracked codeWasCopiedRecently: boolean = false;

@action
handleCopyButtonClick() {
if (config.environment !== 'test') {
navigator.clipboard.writeText(this.args.code);
}

this.codeWasCopiedRecently = true;

later(
this,
() => {
this.codeWasCopiedRecently = false;
},
1000,
);

if (this.args.onCopyButtonClick) {
this.args.onCopyButtonClick();
}
}
}
export default class CopyableCodeComponent extends Component<Signature> {}

declare module '@glint/environment-ember-loose/registry' {
export default interface Registry {
Expand Down
32 changes: 17 additions & 15 deletions app/components/copyable-terminal-command.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,21 +30,23 @@
{{/if}}
</div>

<TertiaryButton @size="extra-small" {{on "click" this.handleCopyButtonClick}}>
<AnimatedContainer>
{{#animated-if this.wasCopiedRecently use=this.transition duration=200}}
<div class="flex items-center gap-1">
{{svg-jar "clipboard-check" class="h-4 w-4 text-teal-500"}}
<span class="text-xs text-teal-500">copied!</span>
</div>
{{else}}
<div class="flex items-center gap-1">
{{svg-jar "clipboard-copy" class="h-4 w-4"}}
<span class="text-xs">copy</span>
</div>
{{/animated-if}}
</AnimatedContainer>
</TertiaryButton>
<CopyableAnything @value={{this.copyableText}} @onCopied={{@onCopyButtonClick}} as |performCopyTask hasRecentlyCopied|>
<TertiaryButton @size="extra-small" {{on "click" performCopyTask}}>
<AnimatedContainer>
{{#animated-if hasRecentlyCopied use=this.transition duration=200}}
<div class="flex items-center gap-1">
{{svg-jar "clipboard-check" class="h-4 w-4 text-teal-500"}}
<span class="text-xs text-teal-500">copied!</span>
</div>
{{else}}
<div class="flex items-center gap-1">
{{svg-jar "clipboard-copy" class="h-4 w-4"}}
<span class="text-xs">copy</span>
</div>
{{/animated-if}}
</AnimatedContainer>
</TertiaryButton>
</CopyableAnything>
</div>

<div class="w-full text-xs overflow-x-auto" data-test-copyable-text>
Expand Down
31 changes: 2 additions & 29 deletions app/components/copyable-terminal-command.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,14 @@
import Component from '@glimmer/component';
import config from 'codecrafters-frontend/config/environment';
import { action } from '@ember/object';
import { later } from '@ember/runloop';
import { tracked } from '@glimmer/tracking';
import fade from 'ember-animated/transitions/fade';
import type DarkModeService from 'codecrafters-frontend/services/dark-mode';
import { service } from '@ember/service';
import type DarkModeService from 'codecrafters-frontend/services/dark-mode';

interface Signature {
Element: HTMLDivElement;

Args: {
commands: string[];
onCopyButtonClick?: () => void;
onCopyButtonClick?: () => void | Promise<void>;
onVariantLabelClick?: (variant: string) => void;
selectedVariantLabel?: string;
variantLabels?: string[];
Expand All @@ -24,36 +20,13 @@ export default class CopyableTerminalCommandComponent extends Component<Signatur

@service declare darkMode: DarkModeService;

@tracked wasCopiedRecently: boolean = false;

get codeForHighlighting(): string {
return this.args.commands.join('\n');
}

get copyableText(): string {
return this.args.commands.map((command) => command.replace(/# .*$/, '')).join('\n');
}

@action
handleCopyButtonClick() {
if (config.environment !== 'test') {
navigator.clipboard.writeText(this.copyableText);
}

this.wasCopiedRecently = true;

later(
this,
() => {
this.wasCopiedRecently = false;
},
1000,
);

if (this.args.onCopyButtonClick) {
this.args.onCopyButtonClick();
}
}
}

declare module '@glint/environment-ember-loose/registry' {
Expand Down
18 changes: 10 additions & 8 deletions app/components/course-admin/code-example-page/comparison-card.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,16 @@
</div>
<div class="flex items-center gap-3">
{{#if this.isExpanded}}
<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"
{{on "click" this.handleCopyIdToClipbardButtonClick}}
>
{{svg-jar "clipboard-check" class="w-3 mr-1 fill-current"}}
Copy ID
</button>
<CopyableAnything @value={{@comparison.id}} as |performCopyTask hasRecentlyCopied|>
<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"
{{on "click" performCopyTask}}
>
{{svg-jar "clipboard-check" class="w-3 mr-1 fill-current"}}
{{if hasRecentlyCopied "Copied!" "Copy ID"}}
</button>
</CopyableAnything>
<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"
Expand Down
11 changes: 3 additions & 8 deletions app/components/course-admin/code-example-page/comparison-card.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import Component from '@glimmer/component';
import { action } from '@ember/object';
import { next } from '@ember/runloop';
import Component from '@glimmer/component';
import { tracked } from '@glimmer/tracking';
import type SolutionComparisonModel from 'codecrafters-frontend/models/solution-comparison';
import type UserModel from 'codecrafters-frontend/models/user';
Expand All @@ -14,7 +14,7 @@ export interface Signature {
};
}

export default class ComparisonCard extends Component<Signature> {
export default class ComparisonCardComponent extends Component<Signature> {
@tracked isExpanded = false;

get firstUser() {
Expand Down Expand Up @@ -68,11 +68,6 @@ export default class ComparisonCard extends Component<Signature> {
});
}

@action
handleCopyIdToClipbardButtonClick() {
navigator.clipboard.writeText(this.args.comparison.id);
}

@action
handleExpandButtonClick() {
next(() => {
Expand All @@ -83,6 +78,6 @@ export default class ComparisonCard extends Component<Signature> {

declare module '@glint/environment-ember-loose/registry' {
export default interface Registry {
'CourseAdmin::CodeExamplePage::ComparisonCard': typeof ComparisonCard;
'CourseAdmin::CodeExamplePage::ComparisonCard': typeof ComparisonCardComponent;
}
}
12 changes: 5 additions & 7 deletions app/components/course-admin/code-example-page/evaluation-card/evaluation-tab.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
<div>
<div class="flex items-center mb-4 gap-4 flex-wrap">
<TertiaryButton class="mb-4" {{on "click" this.handleCopyToClipboardButtonClick}} @isDisabled={{not @evaluation.logsFileContents}}>
{{#if this.wasRecentlyCopied}}
Copied!
{{else}}
Copy to Clipboard
{{/if}}
</TertiaryButton>
<CopyableAnything @value={{@evaluation.logsFileContents}} as |performCopyTask hasRecentlyCopied|>
<TertiaryButton class="mb-4" {{on "click" performCopyTask}} @isDisabled={{not @evaluation.logsFileContents}}>
{{if hasRecentlyCopied "Copied!" "Copy to Clipboard"}}
</TertiaryButton>
</CopyableAnything>

<TertiaryButtonWithSpinner
class="mb-4"
Expand Down
19 changes: 2 additions & 17 deletions app/components/course-admin/code-example-page/evaluation-card/evaluation-tab.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import { action } from '@ember/object';
import Component from '@glimmer/component';
import { action } from '@ember/object';
import { task } from 'ember-concurrency';
import type CommunitySolutionEvaluationModel from 'codecrafters-frontend/models/community-solution-evaluation';
import { task, timeout } from 'ember-concurrency';
import { tracked } from 'tracked-built-ins';

export interface Signature {
Element: HTMLDivElement;
Expand All @@ -14,26 +13,12 @@ export interface Signature {
}

export default class EvaluationTabComponent extends Component<Signature> {
@tracked wasRecentlyCopied = false;

@action
handleCopyToClipboardButtonClick() {
this.copyToClipboardTask.perform();
}

@action
handleRegenerateButtonClick() {
this.regenerateTask.perform();
this.args.onRegenerate();
}

copyToClipboardTask = task({ keepLatest: true }, async (): Promise<void> => {
this.wasRecentlyCopied = true;
navigator.clipboard.writeText(this.args.evaluation.logsFileContents!);
await timeout(1000);
this.wasRecentlyCopied = false;
});

regenerateTask = task({ keepLatest: true }, async (): Promise<void> => {
await this.args.evaluation.regenerate({});
});
Expand Down
Loading
Loading