Conversation
|
|
📝 WalkthroughWalkthroughRemoves truncation styling from list items, introduces utility functions for text-selection-aware drag behavior and expand/collapse state management, updates the block component with selection guards and restructured DOM layout, enhances ControlLink with a selectable prop and guards, adds comprehensive tests, and updates TypeScript configuration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 5
🧹 Nitpick comments (1)
apps/web/core/components/issues/issue-layouts/list/block.logic.ts (1)
5-35: Please routeIssueBlockthrough these helpers before relying on the new tests.
packages/codemods/tests/block.logic.test.tsexercises this module, butapps/web/core/components/issues/issue-layouts/list/block.tsxstill duplicates the same selection, drag, expand, and toast decisions inline instead of importing these functions. That leaves you with green tests over helper code while the live component can drift separately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/core/components/issues/issue-layouts/list/block.logic.ts` around lines 5 - 35, The IssueBlock component in block.tsx is still computing selection, drag, expand and toast logic inline; instead import and use the helper functions shouldSuppressEvent, canDragBasedOnSelection, nextExpandState, and getDragDisallowedToast from block.logic.ts so the component behavior matches the tests—replace inline checks for selectionText, drag permission, nesting/expanded state toggling, and building the drag-disallowed toast with calls to these helpers (use shouldSuppressEvent(selectionText) for suppressing events, canDragBasedOnSelection(selectionText, isAllowed) for drag gating, nextExpandState(nestingLevel, isExpanded) for toggling expansion, and getDragDisallowedToast(canDrag, canEdit) for the toast).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/core/components/issues/issue-layouts/list/block.tsx`:
- Around line 121-127: The draggable() is being applied to an invisible absolute
element (the variable element) so the drag hotspot spans the whole row; instead,
limit dragging to the visible handle by attaching draggable() to the actual
handle node or by guarding canDrag to only allow drag when the event target is
the handle (e.g., check event.target or a handleRef/class) and keep checking
window.getSelection() and isDraggingAllowed; apply the same fix where
draggable(...) is used later (the other combine call around the second element
usage) so only the handle can initiate drags.
- Around line 324-329: The epic stats are shown when displayProperties is true
even if the actual count is zero; update the WithDisplayPropertiesHOC guard so
it also checks the runtime sub-issue count. Change the shouldRenderProperty
callback used with displayPropertyKey="sub_issue_count" to return a boolean that
combines the display property and the local sub-issue count (e.g.,
shouldRenderProperty = (properties) => !!properties.sub_issue_count &&
!!subIssuesCount) so IssueStats stays hidden when subIssuesCount is 0.
In `@packages/codemods/tsconfig.json`:
- Line 13: The tsconfig "include" currently pulls in "../../apps/web" which
forces the codemods package to type-check unrelated TSX files and path-aliases;
update the tsconfig for the codemods package (the "include" array in
packages/codemods/tsconfig.json) to remove "../../apps/web" so the program is
scoped to only "src" and "tests" (optionally add an "exclude" if you want to be
explicit); ensure no other cross-package project references are added so tests
still transitively pick up block.logic.ts without importing the whole web app.
In `@packages/ui/src/control-link/control-link.tsx`:
- Around line 54-60: The fallback branches for ControlLink break link semantics:
the disabled branch still renders an href so disabled links remain navigable,
and the selectable branch replaces the anchor with a span removing href/onClick
and keyboard semantics. Fix by changing the disabled branch (where code checks
disabled && (ref || className)) to render an <a> without an href (remove href or
set href={undefined}) and add aria-disabled="true" and tabIndex={-1} so it is
non-navigable but still an anchor; and change the selectable branch (the branch
that currently returns a <span>) to render an <a> that preserves href and
onClick/keyboard handlers (or if no href is provided, render an <a role="button"
tabIndex={0} onKeyDown={...} onClick={onClick}) so selectable links keep
keyboard and link semantics. Ensure these changes reference the component props
href, onClick, selectable, disabled, ref, className, and children.
- Around line 37-40: The guard inside handleOnClick that checks
window.getSelection()?.toString() should call event.preventDefault() (and
event.stopPropagation() if you want to fully mirror the other guard) before
returning so the anchor's default navigation is suppressed when the user has a
text selection; update the handleOnClick function (the
React.MouseEvent<HTMLAnchorElement, MouseEvent> handler) to call
event.preventDefault() (and optionally event.stopPropagation()) then return.
---
Nitpick comments:
In `@apps/web/core/components/issues/issue-layouts/list/block.logic.ts`:
- Around line 5-35: The IssueBlock component in block.tsx is still computing
selection, drag, expand and toast logic inline; instead import and use the
helper functions shouldSuppressEvent, canDragBasedOnSelection, nextExpandState,
and getDragDisallowedToast from block.logic.ts so the component behavior matches
the tests—replace inline checks for selectionText, drag permission,
nesting/expanded state toggling, and building the drag-disallowed toast with
calls to these helpers (use shouldSuppressEvent(selectionText) for suppressing
events, canDragBasedOnSelection(selectionText, isAllowed) for drag gating,
nextExpandState(nestingLevel, isExpanded) for toggling expansion, and
getDragDisallowedToast(canDrag, canEdit) for the toast).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19095843-1c50-418b-86c1-d1a8c1fbdad4
📒 Files selected for processing (6)
apps/web/core/components/core/list/list-item.tsxapps/web/core/components/issues/issue-layouts/list/block.logic.tsapps/web/core/components/issues/issue-layouts/list/block.tsxpackages/codemods/tests/block.logic.test.tspackages/codemods/tsconfig.jsonpackages/ui/src/control-link/control-link.tsx
| return combine( | ||
| draggable({ | ||
| element, | ||
| canDrag: () => isDraggingAllowed, | ||
| canDrag: () => { | ||
| if (window.getSelection()?.toString()) return false; | ||
| return isDraggingAllowed; | ||
| }, |
There was a problem hiding this comment.
This makes the drag hotspot effectively row-wide again.
draggable() is now attached to an invisible absolute element that spans the main content area, so dragging can start from almost any blank space in the row instead of a dedicated handle. That reintroduces the same click/selection conflicts this change is trying to eliminate.
Also applies to: 223-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/core/components/issues/issue-layouts/list/block.tsx` around lines
121 - 127, The draggable() is being applied to an invisible absolute element
(the variable element) so the drag hotspot spans the whole row; instead, limit
dragging to the visible handle by attaching draggable() to the actual handle
node or by guarding canDrag to only allow drag when the event target is the
handle (e.g., check event.target or a handleRef/class) and keep checking
window.getSelection() and isDraggingAllowed; apply the same fix where
draggable(...) is used later (the other combine call around the second element
usage) so only the handle can initiate drags.
| {isEpic && displayProperties && ( | ||
| <WithDisplayPropertiesHOC | ||
| displayProperties={displayProperties} | ||
| displayPropertyKey="sub_issue_count" | ||
| shouldRenderProperty={(properties) => !!properties.sub_issue_count} | ||
| > |
There was a problem hiding this comment.
Keep epic stats hidden when the sub-issue count is zero.
This guard now only checks the display flag, so IssueStats renders for every epic even when subIssuesCount is 0. The Kanban block keeps the old behavior by also gating on !!subIssuesCount.
Suggested fix
{isEpic && displayProperties && (
<WithDisplayPropertiesHOC
displayProperties={displayProperties}
displayPropertyKey="sub_issue_count"
- shouldRenderProperty={(properties) => !!properties.sub_issue_count}
+ shouldRenderProperty={(properties) =>
+ !!properties.sub_issue_count && !!subIssuesCount
+ }
>📝 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.
| {isEpic && displayProperties && ( | |
| <WithDisplayPropertiesHOC | |
| displayProperties={displayProperties} | |
| displayPropertyKey="sub_issue_count" | |
| shouldRenderProperty={(properties) => !!properties.sub_issue_count} | |
| > | |
| {isEpic && displayProperties && ( | |
| <WithDisplayPropertiesHOC | |
| displayProperties={displayProperties} | |
| displayPropertyKey="sub_issue_count" | |
| shouldRenderProperty={(properties) => | |
| !!properties.sub_issue_count && !!subIssuesCount | |
| } | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/core/components/issues/issue-layouts/list/block.tsx` around lines
324 - 329, The epic stats are shown when displayProperties is true even if the
actual count is zero; update the WithDisplayPropertiesHOC guard so it also
checks the runtime sub-issue count. Change the shouldRenderProperty callback
used with displayPropertyKey="sub_issue_count" to return a boolean that combines
the display property and the local sub-issue count (e.g., shouldRenderProperty =
(properties) => !!properties.sub_issue_count && !!subIssuesCount) so IssueStats
stays hidden when subIssuesCount is 0.
| "noFallthroughCasesInSwitch": true | ||
| } | ||
| }, | ||
| "include": ["src", "tests", "../../apps/web"] |
There was a problem hiding this comment.
Keep the codemods TS program scoped to codemods sources.
Line 13 pulls the entire apps/web project into a node-library tsconfig that does not carry the web app's JSX/path-alias settings. That turns one cross-package test import into type-checking for unrelated TSX files and @/* imports, which is likely to break linting/type-checking for this package. The test already brings block.logic.ts into the program transitively, so the broad include is unnecessary.
Suggested fix
- "include": ["src", "tests", "../../apps/web"]
+ "include": ["src", "tests"]📝 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.
| "include": ["src", "tests", "../../apps/web"] | |
| "include": ["src", "tests"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/codemods/tsconfig.json` at line 13, The tsconfig "include" currently
pulls in "../../apps/web" which forces the codemods package to type-check
unrelated TSX files and path-aliases; update the tsconfig for the codemods
package (the "include" array in packages/codemods/tsconfig.json) to remove
"../../apps/web" so the program is scoped to only "src" and "tests" (optionally
add an "exclude" if you want to be explicit); ensure no other cross-package
project references are added so tests still transitively pick up block.logic.ts
without importing the whole web app.
| const handleOnClick = (event: React.MouseEvent<HTMLAnchorElement, MouseEvent>) => { | ||
| if (window.getSelection()?.toString()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Prevent the anchor's default navigation when selection exists.
This guard returns early, but it never calls event.preventDefault(). On an <a>, that means the browser can still follow href, which defeats the text-selection fix and bypasses the explicit preventDefault/stopPropagation guard already added in apps/web/core/components/issues/issue-layouts/list/block.tsx.
Suggested fix
const handleOnClick = (event: React.MouseEvent<HTMLAnchorElement, MouseEvent>) => {
if (window.getSelection()?.toString()) {
+ event.preventDefault();
+ event.stopPropagation();
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.
| const handleOnClick = (event: React.MouseEvent<HTMLAnchorElement, MouseEvent>) => { | |
| if (window.getSelection()?.toString()) { | |
| return; | |
| } | |
| const handleOnClick = (event: React.MouseEvent<HTMLAnchorElement, MouseEvent>) => { | |
| if (window.getSelection()?.toString()) { | |
| event.preventDefault(); | |
| event.stopPropagation(); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/control-link/control-link.tsx` around lines 37 - 40, The
guard inside handleOnClick that checks window.getSelection()?.toString() should
call event.preventDefault() (and event.stopPropagation() if you want to fully
mirror the other guard) before returning so the anchor's default navigation is
suppressed when the user has a text selection; update the handleOnClick function
(the React.MouseEvent<HTMLAnchorElement, MouseEvent> handler) to call
event.preventDefault() (and optionally event.stopPropagation()) then return.
| // if disabled but still has a ref or a className then it has to be rendered without a href | ||
| if (disabled && (ref || className)) | ||
| return ( | ||
| <a ref={ref} className={className}> | ||
| <a ref={ref as React.Ref<HTMLAnchorElement>} className={className} href={href ?? "#"}> | ||
| {children} | ||
| </a> | ||
| ); |
There was a problem hiding this comment.
The new fallback branches no longer preserve ControlLink semantics.
The disabled branch still renders an href, so "disabled" links remain navigable. The selectable branch switches to a span and drops both href and onClick, so the same component stops navigating and loses keyboard/link semantics entirely. This is already reachable today from apps/web/core/components/issues/issue-layouts/list/block.tsx:180-195, which passes both disabled and className.
Also applies to: 65-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/control-link/control-link.tsx` around lines 54 - 60, The
fallback branches for ControlLink break link semantics: the disabled branch
still renders an href so disabled links remain navigable, and the selectable
branch replaces the anchor with a span removing href/onClick and keyboard
semantics. Fix by changing the disabled branch (where code checks disabled &&
(ref || className)) to render an <a> without an href (remove href or set
href={undefined}) and add aria-disabled="true" and tabIndex={-1} so it is
non-navigable but still an anchor; and change the selectable branch (the branch
that currently returns a <span>) to render an <a> that preserves href and
onClick/keyboard handlers (or if no href is provided, render an <a role="button"
tabIndex={0} onKeyDown={...} onClick={onClick}) so selectable links keep
keyboard and link semantics. Ensure these changes reference the component props
href, onClick, selectable, disabled, ref, className, and children.
2 participants
Description
This PR fixes an issue where users were unable to select or copy text inside issue rows in the list view. Previously, any attempt to highlight text triggered navigation or drag behavior, preventing users from copying work item IDs, titles, and other inline text. The update introduces selection‑safe regions, adjusts event‑handling logic, and ensures that drag and navigation behaviors no longer override text selection.
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
To verify the fix, the following tests were performed:
Confirmed that text inside issue rows (ID, title, metadata) can be selected and copied without triggering navigation.
Verified that clicking a row still navigates correctly when no text is selected.
Ensured drag‑and‑drop behavior remains unchanged and still works exclusively through the drag handle.
Added unit tests for updated event‑handling logic where applicable.
Performed manual interaction testing across Chrome, Firefox, and Safari to validate consistent behavior.
References
#8627
Summary by CodeRabbit
New Features
selectableoption to control links for better interaction handling.Bug Fixes
Tests