Expandable code blocks #1683
Expandable code blocks #1683
Conversation
|
WalkthroughThe CodeBlock component now supports an optional modal view for displaying code. A new property, Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CB as CodeBlock
participant D as Dialog
U->>CB: Click "Open in Modal" button
CB-->>CB: Set isModalOpen = true
CB->>D: Render modal with code and copy button
U->>D: Click modal copy button
D-->>CB: Trigger onModalCopied callback
CB-->>CB: Set modalCopied = true
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/webapp/app/components/code/CodeBlock.tsx (4)
203-204: Consider combining copy statesThe component now maintains two separate copy states (
copiedandmodalCopied) that serve the same purpose. Consider unifying them into a single state to reduce complexity.-const [copied, setCopied] = useState(false); -const [modalCopied, setModalCopied] = useState(false); +const [copyState, setCopyState] = useState({ main: false, modal: false });
219-230: Consolidate copy handlersThe
onModalCopiedfunction is nearly identical toonCopied. Consider creating a single reusable copy handler.-const onCopied = useCallback( +const createCopyHandler = useCallback( + (setState: (value: boolean) => void) => (event: React.MouseEvent<HTMLButtonElement>) => { event.preventDefault(); event.stopPropagation(); navigator.clipboard.writeText(code); - setCopied(true); + setState(true); setTimeout(() => { - setCopied(false); + setState(false); }, 1500); }, - [code] + [code] ); -const onModalCopied = useCallback(/* ... */);
413-433: Consider accessibility improvements for the modalThe modal implementation looks good, but could benefit from some accessibility enhancements:
- Add
aria-labelto the modal for screen readers- Consider trapping focus within the modal when open
- Add keyboard shortcuts for common actions
<Dialog open={isModalOpen} onOpenChange={setIsModalOpen}> - <DialogContent className="flex flex-col gap-0 p-0 pt-[2.9rem] sm:h-[80vh] sm:max-h-[80vh] sm:max-w-[80vw]"> + <DialogContent + className="flex flex-col gap-0 p-0 pt-[2.9rem] sm:h-[80vh] sm:max-h-[80vh] sm:max-w-[80vw]" + aria-label="Code block expanded view" + >
444-449: Consider preserving text size preferenceThe modal view forces a larger text size (
text-base) compared to the inline view (text-xs). Consider making this configurable or preserving the user's text size preference.<pre className={cn( - "relative mr-2 font-mono text-base leading-relaxed", + "relative mr-2 font-mono leading-relaxed", + modalTextSize ?? "text-base", inheritedClassName )} style={inheritedStyle} dir="ltr" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/components/code/CodeBlock.tsx(6 hunks)apps/webapp/app/components/primitives/Dialog.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/webapp/app/components/primitives/Dialog.tsx (1)
97-97: LGTM! Style adjustment for dialog titlesThe removal of
leading-noneandtracking-tightclasses improves readability of dialog titles, particularly beneficial for the new code block modal view.apps/webapp/app/components/code/CodeBlock.tsx (2)
1-8: LGTM! Well-organized importsThe new imports are properly organized and include all necessary components for the modal functionality.
58-59: LGTM! Clear prop definitionThe
showOpenInModalprop is well-documented and follows the existing pattern of boolean flags.
| @@ -193,6 +200,9 @@ export const CodeBlock = forwardRef<HTMLDivElement, CodeBlockProps>( | |||
| ) => { | |||
| const [mouseOver, setMouseOver] = useState(false); | |||
| const [copied, setCopied] = useState(false); | |||
| const [modalCopied, setModalCopied] = useState(false); | |||
There was a problem hiding this comment.
Can't we use the existing copied state?
There was a problem hiding this comment.
I tried this at first but when you do, it triggers the copied UI to animated below the modal. You can see it through the modal's overlay and it looks like a bug.
| @@ -206,6 +216,19 @@ export const CodeBlock = forwardRef<HTMLDivElement, CodeBlockProps>( | |||
| [code] | |||
| ); | |||
|
|
|||
| const onModalCopied = useCallback( | |||
There was a problem hiding this comment.
Can't we use the existing copy callback?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/webapp/app/components/code/CodeBlock.tsx (1)
219-230: 🛠️ Refactor suggestionAvoid duplicating copy logic.
The
onModalCopiedcallback duplicates the logic fromonCopied. Consider creating a reusable copy function.+const copyToClipboard = useCallback( + (event: React.MouseEvent<HTMLButtonElement>, setStateFn: (value: boolean) => void) => { + event.preventDefault(); + event.stopPropagation(); + navigator.clipboard.writeText(code); + setStateFn(true); + setTimeout(() => { + setStateFn(false); + }, 1500); + }, + [code] +); -const onCopied = useCallback( - (event: React.MouseEvent<HTMLButtonElement>) => { - event.preventDefault(); - event.stopPropagation(); - navigator.clipboard.writeText(code); - setCopied(true); - setTimeout(() => { - setCopied(false); - }, 1500); - }, - [code] -); -const onModalCopied = useCallback( - (event: React.MouseEvent<HTMLButtonElement>) => { - event.preventDefault(); - event.stopPropagation(); - navigator.clipboard.writeText(code); - setModalCopied(true); - setTimeout(() => { - setModalCopied(false); - }, 1500); - }, - [code] -); +const onCopied = useCallback( + (event: React.MouseEvent<HTMLButtonElement>) => copyToClipboard(event, setCopied), + [copyToClipboard] +); +const onModalCopied = useCallback( + (event: React.MouseEvent<HTMLButtonElement>) => copyToClipboard(event, setModalCopied), + [copyToClipboard] +);
🧹 Nitpick comments (3)
apps/webapp/app/components/code/CodeBlock.tsx (3)
58-59: Consider adding JSDoc comment for the new prop.Add documentation for the
showOpenInModalprop to maintain consistency with other props.+ /** Whether to show the expand button that opens the code in a modal */ showOpenInModal?: boolean;
203-204: Consider using a single state object for related states.The component now has multiple copy-related states (
copied,modalCopied) and modal state. Consider combining them into a single state object for better maintainability.-const [copied, setCopied] = useState(false); -const [modalCopied, setModalCopied] = useState(false); -const [isModalOpen, setIsModalOpen] = useState(false); +const [state, setState] = useState({ + copied: false, + modalCopied: false, + isModalOpen: false, +});
330-370: Consider accessibility improvements for the modal.The modal implementation looks good, but could benefit from some accessibility improvements:
- Add
aria-labelto the copy button- Add keyboard shortcuts for closing the modal (ESC key)
<Button variant="tertiary/small" onClick={onModalCopied} className="absolute right-4 top-16 z-50" LeadingIcon={modalCopied ? undefined : Clipboard} leadingIconClassName="size-3 -ml-1" + aria-label={modalCopied ? "Code copied" : "Copy code"} > {modalCopied ? "Copied" : "Copy"} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/components/code/CodeBlock.tsx(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/webapp/app/components/code/CodeBlock.tsx (3)
1-1: LGTM! Clean import organization.The new imports are logically grouped and follow the component's dependencies.
Also applies to: 7-8
290-302: LGTM! Clean implementation of the expand button.The expand button implementation follows the same pattern as the copy button and uses appropriate tooltips for better UX.
404-517: LGTM! Clean extraction of highlighting logic.The highlighting logic has been cleanly extracted into a separate component with proper prop types. This improves code organization and reusability.
This adds a new "Expand" icon next to the copy button on all code blocks
It opens the code in a large modal so you can view it more clearly
It's default visible for all code blocks but can be turned off.
Summary by CodeRabbit