fixed issue for muliple tap were nedded to view sub issue.#9139
fixed issue for muliple tap were nedded to view sub issue.#9139notHuman9504 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe issue list block component now fetches sub-issues asynchronously before expanding, replacing the prior synchronous state updater pattern. The actions wrapper gains accessibility semantics and keyboard event handling with preventDefault and stopPropagation on key presses. ChangesIssue List Block UI Interactions
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/core/components/issues/issue-layouts/list/block.tsx`:
- Around line 331-334: The current onKeyDown handler in the IssueList block (the
inline onKeyDown prop defined around the ControlLink/child element) calls
e.preventDefault() and e.stopPropagation() for every key which breaks keyboard
navigation; change it so you only preventDefault()/stopPropagation() for
specific activation keys (e.g., e.key === 'Enter' or e.key === ' ' / 'Spacebar')
and return early for other keys (allowing Tab, Escape, arrow keys to behave
normally), or alternatively only call stopPropagation() without preventDefault()
when you need to stop parent handlers but preserve native focus movement.
- Around line 146-156: The handleToggleExpand handler can fire multiple
concurrent fetchSubIssues calls and flip setExpanded unpredictably; add a
loading guard (e.g., local isLoading state or reuse subIssuesStore loader)
inside handleToggleExpand to early-return while a fetch is in progress, set the
loader true before awaiting subIssuesStore.fetchSubIssues and false in a finally
block, and update the expand/collapse button to be disabled or show a loading
indicator while isLoading so rapid clicks won't trigger multiple fetchSubIssues
calls or race setExpanded; reference handleToggleExpand, fetchSubIssues,
setExpanded, nestingLevel, and subIssuesStore when making the changes.
- Around line 146-156: In handleToggleExpand, wrap the await
subIssuesStore.fetchSubIssues(...) call in a try-catch and only call setExpanded
when the fetch succeeds (or fall back to a defined error state), so an API
failure doesn't leave the chevron expanded with no data; catch the error, log it
via your logger (or console.error) with context including issue.id and
workspaceSlug, and remove the redundant .toString() on workspaceSlug when
passing it to fetchSubIssues. Ensure the catch block prevents toggling expansion
on failure and surfaces or records the error appropriately.
- Line 322: The wrapper div in IssueActions (the element with
role="presentation" that also has onClick and onKeyDown handlers) is marked
non-interactive while handling events; remove the contradictory
role="presentation" attribute so assistive tech sees its interactive behavior,
or if you intend it purely as a grouping element, replace role="presentation"
with role="group" and add an appropriate aria-label; ensure the event handlers
on that div (onClick/onKeyDown) remain or are removed consistently with your
choice so ControlLink's semantics are not broken.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8c5f686-b4cd-41ae-a7da-2aec22c6b2b7
📒 Files selected for processing (1)
apps/web/core/components/issues/issue-layouts/list/block.tsx
| const handleToggleExpand = async (e: MouseEvent<HTMLButtonElement>) => { | ||
| e.stopPropagation(); | ||
| e.preventDefault(); | ||
| if (nestingLevel >= 3) { | ||
| handleIssuePeekOverview(issue); | ||
| } else { | ||
| setExpanded((prevState) => { | ||
| if (!prevState && workspaceSlug && issue && issue.project_id) | ||
| subIssuesStore.fetchSubIssues(workspaceSlug.toString(), issue.project_id, issue.id); | ||
| return !prevState; | ||
| }); | ||
| if (!isExpanded && workspaceSlug && issue.project_id) | ||
| await subIssuesStore.fetchSubIssues(workspaceSlug.toString(), issue.project_id, issue.id); | ||
| setExpanded((prevState) => !prevState); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Guard against concurrent expand/collapse operations.
The async handler has no protection against rapid successive clicks. If a user clicks the expand button multiple times while fetchSubIssues is pending, multiple API calls will be triggered and the expanded state will toggle unpredictably.
Consider adding a loading flag or disabling the button during the fetch operation.
🔒 Proposed fix with loading guard
Add a local state or use the store's loader:
+const [isTogglingExpand, setIsTogglingExpand] = useState(false);
+
const handleToggleExpand = async (e: MouseEvent<HTMLButtonElement>) => {
e.stopPropagation();
e.preventDefault();
+
+ if (isTogglingExpand) return;
+
if (nestingLevel >= 3) {
handleIssuePeekOverview(issue);
} else {
if (!isExpanded && workspaceSlug && issue.project_id) {
+ setIsTogglingExpand(true);
try {
await subIssuesStore.fetchSubIssues(workspaceSlug, issue.project_id, issue.id);
setExpanded((prevState) => !prevState);
} catch (error) {
// ... error handling
+ } finally {
+ setIsTogglingExpand(false);
}
} else {
setExpanded((prevState) => !prevState);
}
}
};And update the button to show loading state:
<button
type="button"
className="grid size-4 place-items-center rounded-xs text-placeholder hover:text-tertiary"
onClick={handleToggleExpand}
+ disabled={isTogglingExpand}
>
+ {isTogglingExpand ? (
+ <Spinner className="size-4" />
+ ) : (
<ChevronRightIcon
className={cn("size-4", {
"rotate-90": isExpanded,
})}
strokeWidth={2.5}
/>
+ )}
</button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/core/components/issues/issue-layouts/list/block.tsx` around lines
146 - 156, The handleToggleExpand handler can fire multiple concurrent
fetchSubIssues calls and flip setExpanded unpredictably; add a loading guard
(e.g., local isLoading state or reuse subIssuesStore loader) inside
handleToggleExpand to early-return while a fetch is in progress, set the loader
true before awaiting subIssuesStore.fetchSubIssues and false in a finally block,
and update the expand/collapse button to be disabled or show a loading indicator
while isLoading so rapid clicks won't trigger multiple fetchSubIssues calls or
race setExpanded; reference handleToggleExpand, fetchSubIssues, setExpanded,
nestingLevel, and subIssuesStore when making the changes.
Add error handling for the async fetch operation.
The async fetchSubIssues call has no try-catch block. If the API call fails, the error will propagate as an unhandled promise rejection, and the expansion state will still toggle, leaving the UI in an inconsistent state (expanded chevron with no sub-issues loaded).
As per coding guidelines, async operations should use try-catch with proper error types and log errors appropriately.
🛡️ Proposed fix with error handling
const handleToggleExpand = async (e: MouseEvent<HTMLButtonElement>) => {
e.stopPropagation();
e.preventDefault();
if (nestingLevel >= 3) {
handleIssuePeekOverview(issue);
} else {
- if (!isExpanded && workspaceSlug && issue.project_id)
- await subIssuesStore.fetchSubIssues(workspaceSlug.toString(), issue.project_id, issue.id);
- setExpanded((prevState) => !prevState);
+ if (!isExpanded && workspaceSlug && issue.project_id) {
+ try {
+ await subIssuesStore.fetchSubIssues(workspaceSlug, issue.project_id, issue.id);
+ setExpanded((prevState) => !prevState);
+ } catch (error) {
+ setToast({
+ type: TOAST_TYPE.ERROR,
+ title: "Failed to load sub-issues",
+ message: error instanceof Error ? error.message : "An error occurred while fetching sub-issues",
+ });
+ }
+ } else {
+ setExpanded((prevState) => !prevState);
+ }
}
};Note: Also removed redundant .toString() on workspaceSlug (already converted to string on line 82).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/core/components/issues/issue-layouts/list/block.tsx` around lines
146 - 156, In handleToggleExpand, wrap the await
subIssuesStore.fetchSubIssues(...) call in a try-catch and only call setExpanded
when the fetch succeeds (or fall back to a defined error state), so an API
failure doesn't leave the chevron expanded with no data; catch the error, log it
via your logger (or console.error) with context including issue.id and
workspaceSlug, and remove the redundant .toString() on workspaceSlug when
passing it to fetchSubIssues. Ensure the catch block prevents toggling expansion
on failure and surfaces or records the error appropriately.
| isEpic={isEpic} | ||
| /> | ||
| <div | ||
| role="presentation" |
There was a problem hiding this comment.
Remove role="presentation" from interactive wrapper.
The role="presentation" attribute tells assistive technologies to ignore this element's semantic role, treating it as purely decorative. However, this element has onClick and onKeyDown handlers, making it functionally interactive (even if only to stop event propagation).
This creates a contradiction: the element is marked as non-interactive but has interactive behavior. Either:
- Remove
role="presentation"if the div needs event handlers, or - Remove the event handlers and
roleif the child buttons already handle all interaction
Since the div appears to be a click/keyboard event trap to prevent bubbling to the parent ControlLink, consider using a more appropriate ARIA role or no role at all.
♻️ Suggested alternatives
Option 1: Remove the role attribute (simplest):
<div
- role="presentation"
className={cn("hidden", {Option 2: If the div groups related actions, use role="group" with an accessible label:
<div
- role="presentation"
+ role="group"
+ aria-label="Quick actions"
className={cn("hidden", {📝 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.
| role="presentation" | |
| <div | |
| className={cn("hidden", { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/core/components/issues/issue-layouts/list/block.tsx` at line 322,
The wrapper div in IssueActions (the element with role="presentation" that also
has onClick and onKeyDown handlers) is marked non-interactive while handling
events; remove the contradictory role="presentation" attribute so assistive tech
sees its interactive behavior, or if you intend it purely as a grouping element,
replace role="presentation" with role="group" and add an appropriate aria-label;
ensure the event handlers on that div (onClick/onKeyDown) remain or are removed
consistently with your choice so ControlLink's semantics are not broken.
| onKeyDown={(e) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| }} |
There was a problem hiding this comment.
Prevent default on all keyboard events breaks accessibility.
The onKeyDown handler calls e.preventDefault() and e.stopPropagation() for every keyboard event. This will block essential keyboard navigation including Tab (focus movement), Enter/Space (activation), Escape (closing dialogs), and arrow keys.
If the intent is to prevent specific keys from bubbling (e.g., only Enter or Space to avoid double-triggering the parent link), you should check e.key and only prevent default for those specific keys.
♿ Proposed fix for selective event handling
Only prevent activation keys that might trigger the parent ControlLink:
onKeyDown={(e) => {
- e.preventDefault();
- e.stopPropagation();
+ // Only prevent Enter/Space to avoid triggering parent link
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ e.stopPropagation();
+ }
}}Alternatively, if you want to stop all propagation but still allow default browser behavior (like Tab navigation):
onKeyDown={(e) => {
- e.preventDefault();
e.stopPropagation();
}}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/core/components/issues/issue-layouts/list/block.tsx` around lines
331 - 334, The current onKeyDown handler in the IssueList block (the inline
onKeyDown prop defined around the ControlLink/child element) calls
e.preventDefault() and e.stopPropagation() for every key which breaks keyboard
navigation; change it so you only preventDefault()/stopPropagation() for
specific activation keys (e.g., e.key === 'Enter' or e.key === ' ' / 'Spacebar')
and return early for other keys (allowing Tab, Escape, arrow keys to behave
normally), or alternatively only call stopPropagation() without preventDefault()
when you need to stop parent handlers but preserve native focus movement.
1 participant
Description
fixes the issue #9124
component was rendering before we were getting data from api, so we have to make it re render or make it asyncronus.
Type of Change
Screenshots and Media (if applicable)
Screen.Recording.2026-05-26.at.2.14.09.AM.mov
Test Scenarios
did the manual testing for behaviour of opening and closing of sub issues,
References
#9124
Summary by CodeRabbit
Bug Fixes
Accessibility