refactor(ui): replace obsolete TextCopy with CodeSnippet in TwoFactorTOTP#39957
refactor(ui): replace obsolete TextCopy with CodeSnippet in TwoFactorTOTP#39957thekishandev wants to merge 1 commit intoRocketChat:developfrom
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughThis pull request removes the obsolete Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR completes the migration away from the deprecated internal TextCopy component by updating the TwoFactorTOTP security view to use Fuselage’s CodeSnippet, and then deleting the now-unused TextCopy component implementation.
Changes:
- Replaced
TextCopyusage inTwoFactorTOTPwithCodeSnippet+useClipboard. - Removed the legacy
apps/meteor/client/components/TextCopy.tsxcomponent file.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/meteor/client/views/account/security/TwoFactorTOTP.tsx | Swaps deprecated copy UI for Fuselage CodeSnippet and wires clipboard behavior. |
| apps/meteor/client/components/TextCopy.tsx | Deletes the obsolete component now that it has no consumers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { copy, hasCopied } = useClipboard(totpSecret || ''); | ||
|
|
There was a problem hiding this comment.
This refactor changes copy feedback: the previous TextCopy used useClipboardWithToast, which dispatches a success toast on copy. The new useClipboard usage only updates hasCopied/button label, so the toast is lost. If the intent is to preserve existing UX (and match the PR description), switch to useClipboardWithToast(totpSecret ?? '') or pass onCopySuccess/onCopyError options to useClipboard to keep the toast behavior.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/views/account/security/TwoFactorTOTP.tsx (1)
157-159:CodeSnippetprops are correct — no changes needed.The
onClickprop is the correct handler for the copy button action in the fuselageCodeSnippetcomponent, confirmed across multiple usages in the codebase (Installation.tsx, SaveE2EPasswordModal.tsx, BackupCodesModal.tsx).Note: While
onClick={copy}would simplify the code, theonClick={() => copy()}pattern is consistently used throughout the codebase for this component, so either form is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/account/security/TwoFactorTOTP.tsx` around lines 157 - 159, The CodeSnippet usage is fine: keep the onClick prop as provided (onClick={() => copy()}) and leave buttonText and buttonDisabled unchanged; no code change required for CodeSnippet in TwoFactorTOTP.tsx — if you prefer you may optionally simplify to onClick={copy} for consistency with other usages but it's not necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/client/views/account/security/TwoFactorTOTP.tsx`:
- Around line 157-159: The CodeSnippet usage is fine: keep the onClick prop as
provided (onClick={() => copy()}) and leave buttonText and buttonDisabled
unchanged; no code change required for CodeSnippet in TwoFactorTOTP.tsx — if you
prefer you may optionally simplify to onClick={copy} for consistency with other
usages but it's not necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2678c773-f6c0-49f0-99e4-b91a8ebb4eed
📒 Files selected for processing (2)
apps/meteor/client/components/TextCopy.tsxapps/meteor/client/views/account/security/TwoFactorTOTP.tsx
💤 Files with no reviewable changes (1)
- apps/meteor/client/components/TextCopy.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/account/security/TwoFactorTOTP.tsx
🧠 Learnings (4)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/account/security/TwoFactorTOTP.tsx
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.
Applied to files:
apps/meteor/client/views/account/security/TwoFactorTOTP.tsx
📚 Learning: 2026-01-19T18:08:13.423Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38169
File: packages/ui-client/src/hooks/useGoToDirectMessage.ts:20-27
Timestamp: 2026-01-19T18:08:13.423Z
Learning: In React hooks, including custom hooks like useUserSubscriptionByName, must always be called unconditionally and in the same order on every render, per the Rules of Hooks. Passing empty strings or fallback values to hooks is the correct approach when dealing with potentially undefined values, rather than conditionally calling the hook based on whether the value exists.
Applied to files:
apps/meteor/client/views/account/security/TwoFactorTOTP.tsx
🔇 Additional comments (2)
apps/meteor/client/views/account/security/TwoFactorTOTP.tsx (2)
1-2: LGTM!Imports are correctly sourced from the fuselage design system packages, aligning with the goal of using standardized components.
38-39: LGTM!Hook is called unconditionally following React's Rules of Hooks, with appropriate fallback for potentially undefined
totpSecret. Based on learnings: "Passing empty strings or fallback values to hooks is the correct approach when dealing with potentially undefined values."
Proposed changes
This PR removes the absolute final usage of the deprecated
<TextCopy>component in the.tsxcodebase, replacing it with the globally supported@rocket.chat/fuselage<CodeSnippet>. Since it was the last consumer, the obsoleteTextCopycomponent file itself has been permanently deleted from the codebase to sanitize the/componentsdirectory and reduce bundle payload.Root Cause / Problem
The
TwoFactorTOTPview component was still relying on the obsolete internal<TextCopy>component instead of the design system.Solution / Behavior
Replaced with official Fuselage , leveraging useClipboard for identical UX copy feedback. Safely deleted apps/meteor/client/components/TextCopy.tsx entirely.
Issue(s)
Closes #39956
Validation & Testing
Tested locally (Standard copy-to-clipboard functionality verified)
Confirmed TextCopy does not explicitly exist anywhere else
Type of change
refactor (non-breaking change replacing legacy UI architecture)
Summary by CodeRabbit
Refactor