QF-2769 - Ensure copying the Arabic verse from 'Quran in a Year' section add spaces between words #2553
Conversation
…paces between words
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!selectedNodes.length) { | ||
| return; | ||
| } | ||
|
|
||
| event.preventDefault(); |
There was a problem hiding this comment.
The new handleCopy prevents the default copy action whenever any [data-word-index] element is selected and replaces the clipboard contents with Arabic words. Word-by-word translations and transliterations are rendered inside these same containers, so selecting only the translation text now also triggers preventDefault() and copies Arabic instead. This regression makes it impossible to copy translation/transliteration text in views like the word-by-word modal. Consider guarding the copy interception the way onCopyQuranWords does—only override the clipboard when the selection actually includes Arabic word nodes.
Useful? React with 👍 / 👎.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds word indexing to verse-word elements and a clipboard copy handler on the verse container. Each Changes
Sequence DiagramsequenceDiagram
participant User
participant VerseContainer as Verse Container (DOM)
participant OnCopy as onCopy Handler
participant Clipboard as Clipboard API
User->>VerseContainer: Select text (one or more words) + Copy
VerseContainer->>OnCopy: onCopy event
activate OnCopy
OnCopy->>VerseContainer: querySelectorAll('[data-word-index]') within selection
VerseContainer-->>OnCopy: list of word elements with indices
OnCopy->>OnCopy: sort elements by data-word-index
OnCopy->>OnCopy: map each element -> word[textFieldName] (font-specific) or fallback
OnCopy->>Clipboard: clipboardCopy(joinedWords)
Clipboard-->>User: clipboard contains selected text
deactivate OnCopy
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (13)**/*.*📄 CodeRabbit inference engine (.cursor/rules/bug-handling-with-todo-comments.mdc)
Files:
**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/next-js-conventions.mdc)
Files:
src/components/**/*.tsx📄 CodeRabbit inference engine (.cursor/rules/react-functional-components.mdc)
Files:
**/*.{tsx,jsx}📄 CodeRabbit inference engine (.cursor/rules/react.mdc)
Files:
**/*.tsx📄 CodeRabbit inference engine (.cursor/rules/react.mdc)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/react.mdc)
Files:
**/*.{ts,tsx,d.ts}📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Files:
{src,types}/**/*.ts?(x)📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/*.tsx📄 CodeRabbit inference engine (CLAUDE.md)
Files:
{src,types}/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/components/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx,scss}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (17)📓 Common learnings📚 Learning: 2025-10-19T11:34:07.609ZApplied to files:
📚 Learning: 2025-07-31T15:31:22.667ZApplied to files:
📚 Learning: 2025-10-22T20:33:55.176ZApplied to files:
📚 Learning: 2025-10-07T08:38:28.343ZApplied to files:
📚 Learning: 2025-10-20T15:16:53.776ZApplied to files:
📚 Learning: 2025-10-07T08:38:28.343ZApplied to files:
📚 Learning: 2025-08-01T08:50:54.518ZApplied to files:
📚 Learning: 2025-08-01T08:50:54.518ZApplied to files:
📚 Learning: 2025-08-01T08:50:54.518ZApplied to files:
📚 Learning: 2025-08-01T08:51:05.924ZApplied to files:
📚 Learning: 2025-08-01T08:50:54.518ZApplied to files:
📚 Learning: 2025-10-18T11:22:53.139ZApplied to files:
📚 Learning: 2025-08-01T08:50:22.602ZApplied to files:
📚 Learning: 2025-10-07T08:37:58.213ZApplied to files:
📚 Learning: 2025-08-01T08:50:54.518ZApplied to files:
📚 Learning: 2025-10-26T10:22:52.381ZApplied to files:
🧬 Code graph analysis (1)src/components/Verse/PlainVerseText/index.tsx (1)
🔇 Additional comments (3)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/Verse/PlainVerseText/index.tsx (1)
68-72: Copy interception prevents copying translations/transliterations.The current implementation prevents default copy behavior whenever any
[data-word-index]element is in the selection. Since word-by-word translations and transliterations render inside these same containers (see PlainVerseTextWord component lines 26-31), selecting only translation text also triggerspreventDefault()and copies Arabic instead. This makes it impossible to copy translation/transliteration text.Consider checking whether the selection actually includes Arabic word nodes (e.g., by checking the selection's text content or target element classes) before preventing default copy behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsx(1 hunks)src/components/Verse/PlainVerseText/index.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
**/*.*
📄 CodeRabbit inference engine (.cursor/rules/bug-handling-with-todo-comments.mdc)
If you encounter a bug in existing code, or the instructions lead to suboptimal or buggy code, add comments starting with "TODO:" outlining the problems.
**/*.*: Utilize Early Returns: Use early returns to avoid nested conditions and improve readability.
Conditional Classes: Prefer conditional classes over ternary operators for class attributes.
**/*.*: Use comments sparingly, and when you do, make them meaningful.
Don't comment on obvious things. Excessive or unclear comments can clutter the codebase and become outdated.
Use comments to convey the 'why' behind specific actions or explain unusual behavior and potential pitfalls.
Provide meaningful information about the function's behavior and explain unusual behavior and potential pitfalls.
**/*.*: Write short functions that only do one thing.
Follow the single responsibility principle (SRP), which means that a function should have one purpose and perform it effectively.
If a function becomes too long or complex, consider breaking it into smaller, more manageable functions.Order functions with those that are composing other functions appearing earlier in the file. For example, if you have a menu with multiple buttons, define the menu function above the buttons.
**/*.*: Always add helpful comments to the code explaining what you are doing.
Never delete old comments, unless they are no longer relevant because the code has been rewritten or deleted.
**/*.*: Choose names for variables, functions, and classes that reflect their purpose and behavior.
A name should tell you why it exists, what it does, and how it is used. If a name requires a comment, then the name does not reveal its intent.
Use specific names that provide a clearer understanding of what the variables represent and how they are used.
Files:
src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsxsrc/components/Verse/PlainVerseText/index.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/next-js-conventions.mdc)
**/*.{js,jsx,ts,tsx}: Rely on Next.js Pages Router for state changes.
Minimize 'use client' usage: Prefer server components and Next.js SSR features.
Minimize 'use client' usage: Use 'use client' only for Web API access in small components.
Minimize 'use client' usage: Avoid using 'use client' for data fetching or state management.
**/*.{js,jsx,ts,tsx}: Optimize Web Vitals (LCP, CLS, FID)
Use dynamic loading for non-critical components using @src/components/dls/Spinner/Spinner.tsx
Files:
src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsxsrc/components/Verse/PlainVerseText/index.tsx
src/components/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/react-functional-components.mdc)
src/components/**/*.tsx: Always use React functional components with hooks.
Use React.FC for functional components with props.
Files:
src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsxsrc/components/Verse/PlainVerseText/index.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react.mdc)
**/*.{tsx,jsx}: Use functional components over class components
Keep components small and focused
Extract reusable logic into custom hooks
Use composition over inheritance
Split large components into smaller, focused ones
Follow the Rules of Hooks
Use custom hooks for reusable logic
Use appropriate dependency arrays in useEffect
Implement cleanup in useEffect when needed
Avoid nested hooks
Use useState for local component state
Avoid prop drilling through proper state management
Implement proper memoization (useMemo, useCallback)
Use React.memo for expensive components
Avoid unnecessary re-renders
Implement proper lazy loading
Use proper key props in lists
Profile and optimize render performance
Show appropriate loading and error states
Handle async errors properly
Show user-friendly error messages
Implement proper fallback UI
Log errors appropriately
Handle edge cases gracefully
Use semantic HTML elements
Implement proper ARIA attributes
Ensure keyboard navigation
Handle focus management
Provide proper alt text for images
Use proper imports/exports
Document complex component logic
Files:
src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsxsrc/components/Verse/PlainVerseText/index.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/react.mdc)
Implement proper prop types with TypeScript
**/*.tsx: Prefix interfaces for React props with 'Props' (e.g., ButtonProps)
Implement proper error boundariesReact: use functional components only; extract reusable logic into custom hooks; apply proper memoization (React.memo/useMemo/useCallback)
Files:
src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsxsrc/components/Verse/PlainVerseText/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/react.mdc)
Use xstate for complex state logic
**/*.{ts,tsx}: TypeScript: prefer interfaces over type aliases for object shapes; avoid any; add explicit return types for public functions
Error handling: define and use custom error types; use async/await correctly; show user-friendly error messages
Use Redux Toolkit for state management and XState for complex state machines
Implement i18n with next-translate in components and utilities
Files:
src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsxsrc/components/Verse/PlainVerseText/index.tsx
**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx,d.ts}: Prefer interfaces over types for object definitions
Use type for unions, intersections, and mapped types
Avoid usingany, preferunknownfor unknown types
Leverage TypeScript's built-in utility types
Use generics for reusable type patterns
Use PascalCase for type names and interfaces
Use camelCase for variables and functions
Use UPPER_CASE for constants
Use descriptive names with auxiliary verbs (e.g., isLoading, hasError)
Use explicit return types for public functions
Use arrow functions for callbacks and methods
Implement proper error handling with custom error types
Use function overloads for complex type scenarios
Prefer async/await over Promises
Use readonly for immutable properties
Leverage discriminated unions for type safety
Use type guards for runtime type checking
Implement proper null checking
Avoid type assertions unless necessary
Create custom error types for domain-specific errors
Use Result types for operations that can fail
Use try-catch blocks with typed catch clauses
Handle Promise rejections properly
Files:
src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsxsrc/components/Verse/PlainVerseText/index.tsx
{src,types}/**/*.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript throughout the codebase with strict configuration
Files:
src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsxsrc/components/Verse/PlainVerseText/index.tsx
src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer React functional components for UI implementation
Files:
src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsxsrc/components/Verse/PlainVerseText/index.tsx
{src,types}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use path aliases @/ for src and @/dls/* for design system imports
Files:
src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsxsrc/components/Verse/PlainVerseText/index.tsx
src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Organize React components by feature and place DLS components under src/components/dls
Files:
src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsxsrc/components/Verse/PlainVerseText/index.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Order imports: React first, then external, then internal; alphabetize within groups with blank lines between groups
Naming: camelCase for variables/functions; PascalCase for types, interfaces, and React components; UPPER_CASE for constants
Functions should be <= 30 lines, prefer early returns, single responsibility, and descriptive names
Files:
src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsxsrc/components/Verse/PlainVerseText/index.tsx
**/*.{ts,tsx,js,jsx,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use Prettier formatting: single quotes, 2-space indentation, 100-character line width, and trailing commas
Files:
src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsxsrc/components/Verse/PlainVerseText/index.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-19T11:34:07.609Z
Learnt from: afifvdin
PR: quran/quran.com-frontend-next#2540
File: src/components/Verse/PlainVerseText/index.tsx:4-4
Timestamp: 2025-10-19T11:34:07.609Z
Learning: In src/components/Verse/PlainVerseText/index.tsx, the translation for verse titles should be wired directly to the component using next-translate rather than passed as props from parent components, as this ensures the wording from locales is centralized and only needs to be changed in one place.
Applied to files:
src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsxsrc/components/Verse/PlainVerseText/index.tsx
🧬 Code graph analysis (1)
src/components/Verse/PlainVerseText/index.tsx (1)
src/utils/word.ts (1)
getWordTextFieldNameByFont(29-37)
🔇 Additional comments (2)
src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsx (1)
10-10: LGTM! Clean integration of word indexing.The addition of
wordIndexas a prop and its exposure via thedata-word-indexattribute is straightforward and enables the parent component's copy functionality without modifying existing logic.Also applies to: 18-18, 24-24
src/components/Verse/PlainVerseText/index.tsx (1)
118-122: Clean integration of copy handler and word indexing.The
onCopyhandler is correctly wired to the verse container, andwordIndexis properly passed to eachPlainVerseTextWordcomponent instance. The integration is straightforward and consistent.Also applies to: 123-123, 129-129, 148-148
| const handleCopy = useCallback( | ||
| // eslint-disable-next-line react-func/max-lines-per-function | ||
| (event: React.ClipboardEvent<HTMLDivElement>) => { | ||
| const selection = document.getSelection(); | ||
| if (!selection || selection.isCollapsed) { | ||
| return; | ||
| } | ||
|
|
||
| const verseElement = event.currentTarget; | ||
| const selectedNodes = Array.from( | ||
| verseElement.querySelectorAll<HTMLElement>('[data-word-index]'), | ||
| ).filter((node) => selection.containsNode(node, true)); | ||
|
|
||
| if (!selectedNodes.length) { | ||
| return; | ||
| } | ||
|
|
||
| event.preventDefault(); | ||
|
|
||
| const textFieldName = getWordTextFieldNameByFont(quranFont); | ||
| const copiedText = selectedNodes | ||
| .sort( | ||
| (firstNode, secondNode) => | ||
| Number(firstNode.getAttribute('data-word-index')) - | ||
| Number(secondNode.getAttribute('data-word-index')), | ||
| ) | ||
| .map((node) => { | ||
| const index = Number(node.getAttribute('data-word-index')); | ||
| if (Number.isNaN(index)) { | ||
| return ''; | ||
| } | ||
| const word = words[index]; | ||
| if (!word) { | ||
| return ''; | ||
| } | ||
|
|
||
| const wordText = | ||
| word[textFieldName] ?? word.text ?? word.textUthmani ?? word.qpcUthmaniHafs ?? ''; | ||
|
|
||
| return typeof wordText === 'string' ? wordText.trim() : ''; | ||
| }) | ||
| .filter((wordText) => wordText.length > 0); | ||
|
|
||
| if (!copiedText.length) { | ||
| return; | ||
| } | ||
|
|
||
| clipboardCopy(copiedText.join(' ')); | ||
| }, | ||
| [quranFont, words], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor handleCopy into smaller, focused functions.
The handleCopy function is 50 lines and performs multiple responsibilities: selection handling, node filtering, text extraction, and clipboard copying. This violates the single responsibility principle and the 30-line function guideline. Additionally, the complex selection logic lacks explanatory comments.
As per coding guidelines.
Consider breaking it down:
+// Extracts selected word nodes from the verse container
+const getSelectedWordNodes = (
+ verseElement: HTMLDivElement,
+ selection: Selection,
+): HTMLElement[] => {
+ return Array.from(
+ verseElement.querySelectorAll<HTMLElement>('[data-word-index]'),
+ ).filter((node) => selection.containsNode(node, true));
+};
+
+// Builds copied text from selected word nodes based on font settings
+const buildCopiedText = (
+ selectedNodes: HTMLElement[],
+ words: Word[],
+ quranFont: QuranFont,
+): string[] => {
+ const textFieldName = getWordTextFieldNameByFont(quranFont);
+ return selectedNodes
+ .sort(
+ (firstNode, secondNode) =>
+ Number(firstNode.getAttribute('data-word-index')) -
+ Number(secondNode.getAttribute('data-word-index')),
+ )
+ .map((node) => {
+ const index = Number(node.getAttribute('data-word-index'));
+ if (Number.isNaN(index)) return '';
+
+ const word = words[index];
+ if (!word) return '';
+
+ const wordText =
+ word[textFieldName] ?? word.text ?? word.textUthmani ?? word.qpcUthmaniHafs ?? '';
+ return typeof wordText === 'string' ? wordText.trim() : '';
+ })
+ .filter((wordText) => wordText.length > 0);
+};
+
const handleCopy = useCallback(
(event: React.ClipboardEvent<HTMLDivElement>) => {
const selection = document.getSelection();
- if (!selection || selection.isCollapsed) {
- return;
- }
+ if (!selection || selection.isCollapsed) return;
- const verseElement = event.currentTarget;
- const selectedNodes = Array.from(
- verseElement.querySelectorAll<HTMLElement>('[data-word-index]'),
- ).filter((node) => selection.containsNode(node, true));
+ const selectedNodes = getSelectedWordNodes(event.currentTarget, selection);
+ if (!selectedNodes.length) return;
- if (!selectedNodes.length) {
- return;
- }
-
event.preventDefault();
-
- const textFieldName = getWordTextFieldNameByFont(quranFont);
- const copiedText = selectedNodes
- .sort(
- (firstNode, secondNode) =>
- Number(firstNode.getAttribute('data-word-index')) -
- Number(secondNode.getAttribute('data-word-index')),
- )
- .map((node) => {
- const index = Number(node.getAttribute('data-word-index'));
- if (Number.isNaN(index)) {
- return '';
- }
- const word = words[index];
- if (!word) {
- return '';
- }
-
- const wordText =
- word[textFieldName] ?? word.text ?? word.textUthmani ?? word.qpcUthmaniHafs ?? '';
-
- return typeof wordText === 'string' ? wordText.trim() : '';
- })
- .filter((wordText) => wordText.length > 0);
-
- if (!copiedText.length) {
- return;
- }
-
+ const copiedText = buildCopiedText(selectedNodes, words, quranFont);
+ if (!copiedText.length) return;
+
clipboardCopy(copiedText.join(' '));
},
[quranFont, words],
);
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where copying Arabic verses from the 'Quran in a Year' section resulted in words being concatenated without spaces. The solution implements a custom copy handler that intercepts the copy event and reconstructs the selected text with proper spacing.
Key changes:
- Added custom
onCopyhandler to intelligently copy Arabic text with spaces between words based on the selected font - Introduced
data-word-indexattributes to track word positions for accurate text reconstruction during copy operations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/components/Verse/PlainVerseText/index.tsx | Implements the custom handleCopy function that intercepts copy events, extracts selected words based on their indices, retrieves the appropriate text field based on the current font, and joins them with spaces before copying to clipboard |
| src/components/Verse/PlainVerseText/PlainVerseTextWord/index.tsx | Adds wordIndex prop and data-word-index attribute to word containers to enable word identification during the copy operation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const wordText = | ||
| word[textFieldName] ?? word.text ?? word.textUthmani ?? word.qpcUthmaniHafs ?? ''; |
There was a problem hiding this comment.
The fallback chain for wordText may cause inconsistent behavior across different fonts. The similar implementation in onCopyQuranWords.ts (line 67) directly returns selectedWord[getWordTextFieldNameByFont(quranFont)] without fallbacks.
Consider simplifying this to match the existing pattern:
const wordText = word[textFieldName] ?? '';The current fallback chain (word.text ?? word.textUthmani ?? word.qpcUthmaniHafs) could result in copying text from a different font than what the user is viewing, which may not match their intention.
| const wordText = | |
| word[textFieldName] ?? word.text ?? word.textUthmani ?? word.qpcUthmaniHafs ?? ''; | |
| const wordText = word[textFieldName] ?? ''; |
Summary
Fixes #QF-2769
Ensure copying the Arabic verse from 'Quran in a Year' section add spaces between words.
Added a custom onCopy handler that gathers the selected Arabic words and copies them as a space-separated string.
Type of change
Test plan
I've added a playwright test in this commit to ensure the verse is copied with spaces.
Checklist
Screenshots or videos
Summary by CodeRabbit