Add copy to clipboard as shared service - #2507
Conversation
…iately after success.
…ce-usein-publisheddata-details' into add-copy-to-clipboard-as-shared-service
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- ClipboardService calls
navigator.clipboard.writeTextdirectly without feature detection or a non-HTTPS fallback, which may break in unsupported or insecure browsing contexts; consider guarding withif (navigator.clipboard?.writeText)and providing a graceful fallback or message when unavailable. UsersServiceis imported intoconfigurable-action.component.tsbut never used; it can be removed to avoid unused dependency warnings and keep the component lean.- The Cypress tests for published data actions now rely on
configurable-action buttonwith visible text instead ofdata-cyattributes, which is more brittle against UI/content changes; consider keeping stabledata-cyhooks on the relevant action buttons in the configurable-actions component or wrapper.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- ClipboardService calls `navigator.clipboard.writeText` directly without feature detection or a non-HTTPS fallback, which may break in unsupported or insecure browsing contexts; consider guarding with `if (navigator.clipboard?.writeText)` and providing a graceful fallback or message when unavailable.
- `UsersService` is imported into `configurable-action.component.ts` but never used; it can be removed to avoid unused dependency warnings and keep the component lean.
- The Cypress tests for published data actions now rely on `configurable-action button` with visible text instead of `data-cy` attributes, which is more brittle against UI/content changes; consider keeping stable `data-cy` hooks on the relevant action buttons in the configurable-actions component or wrapper.
## Individual Comments
### Comment 1
<location path="src/app/_layout/app-footer/app-footer.component.scss" line_range="19" />
<code_context>
+
+ .toplink {
+ padding: 0.5rem 1rem;
+ font: bold;
+ font-size: 11pt;
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Replace invalid `font: bold;` with a proper font-weight declaration.
`font` is a shorthand that expects a full font definition, so `font: bold;` is invalid and may be ignored. If you only need bold text, use `font-weight: bold;` instead for reliable rendering.
</issue_to_address>
### Comment 2
<location path="src/app/shared/services/clipboard.service.ts" line_range="24" />
<code_context>
+ successMessage: string = "Copied to clipboard",
+ duration: number = 5000,
+ ): void {
+ navigator.clipboard.writeText(text).then(
+ () => {
+ const message = new Message(
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against missing `navigator.clipboard` to avoid runtime errors in unsupported environments.
Because `navigator.clipboard.writeText` can throw synchronously when the Clipboard API isn’t available (e.g. some test runners, non-secure origins, older browsers), that error bypasses your Promise handlers and can crash the app. You can feature-detect and show a message instead:
```ts
if (!navigator.clipboard || !navigator.clipboard.writeText) {
const errorMessage = new Message(
"Clipboard not supported",
MessageType.Error,
duration,
);
this.store.dispatch(showMessageAction({ message: errorMessage }));
return;
}
navigator.clipboard.writeText(text).then(…)
```
</issue_to_address>
### Comment 3
<location path="docs/contributors/configurable_actions_technical.md" line_range="152" />
<code_context>
| #userIsAdmin | boolean | True if the user is an admin |
+| #userIsLoggedIn | boolean | True if the user is logged in (negate with a leading `!`, e.g. `!#userIsLoggedIn`) |
| #uuid | string | A v4 uuid generated on the fly |
| @variable | any | replace the string with the valu eof the variable defined in the action variable |
</code_context>
<issue_to_address>
**issue (typo):** Fix the typo "valu eof" to "value of" in the description.
Consider rephrasing this to: "replace the string with the value of the variable defined in the action variable."
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| .toplink { | ||
| padding: 0.5rem 1rem; | ||
| font: bold; |
There was a problem hiding this comment.
issue (bug_risk): Replace invalid font: bold; with a proper font-weight declaration.
font is a shorthand that expects a full font definition, so font: bold; is invalid and may be ignored. If you only need bold text, use font-weight: bold; instead for reliable rendering.
| successMessage: string = "Copied to clipboard", | ||
| duration: number = 5000, | ||
| ): void { | ||
| navigator.clipboard.writeText(text).then( |
There was a problem hiding this comment.
issue (bug_risk): Guard against missing navigator.clipboard to avoid runtime errors in unsupported environments.
Because navigator.clipboard.writeText can throw synchronously when the Clipboard API isn’t available (e.g. some test runners, non-secure origins, older browsers), that error bypasses your Promise handlers and can crash the app. You can feature-detect and show a message instead:
if (!navigator.clipboard || !navigator.clipboard.writeText) {
const errorMessage = new Message(
"Clipboard not supported",
MessageType.Error,
duration,
);
this.store.dispatch(showMessageAction({ message: errorMessage }));
return;
}
navigator.clipboard.writeText(text).then(…)| | #userIsAdmin | boolean | True if the user is an admin | | ||
| | #userIsLoggedIn | boolean | True if the user is logged in (negate with a leading `!`, e.g. `!#userIsLoggedIn`) | | ||
| | #uuid | string | A v4 uuid generated on the fly | | ||
| | @variable | any | replace the string with the valu eof the variable defined in the action variable | |
There was a problem hiding this comment.
issue (typo): Fix the typo "valu eof" to "value of" in the description.
Consider rephrasing this to: "replace the string with the value of the variable defined in the action variable."
Description
a new copy service is introduced as shared service and used in published data details.
Motivation
More convenience: Goal was to make the doi copyable just like the pids in dataset details.
Changes:
introduce new service at central place (app/shared/service) to use in published data for doi field.
Other
For maintainability reasons one can follow up replacements of other places (see screenshot) where this functionality was used. Main bottleneck is testing that functionality remains in all cases the same.
Screenshot from github bot

Tests included
Backend version
Summary by Sourcery
Centralize clipboard functionality and expand configurable actions across published data and application layout.
New Features:
Enhancements:
Documentation:
Tests: