Skip to content

[#398] COMPONENTS - Added reusable issue fetcher pane#429

Open
Jomak-x wants to merge 7 commits intomainfrom
jakob/issue-fetcher-clean
Open

[#398] COMPONENTS - Added reusable issue fetcher pane#429
Jomak-x wants to merge 7 commits intomainfrom
jakob/issue-fetcher-clean

Conversation

@Jomak-x
Copy link
Copy Markdown

@Jomak-x Jomak-x commented Mar 25, 2026

Why

One shared issue filter/fetch component that can be reused across different Blade views later.

What

Issue(s): #398

Added a reusable IssueFetcherPane component that handles shared issue filtering and fetching.

Also added a small IssueFetcherPanePlayground example to show how a parent view can receive the filtered issues and use them for later views like list, kanban, or calendar.

Test Plan

image

Checklist

  • Database: No schema changes, OR I have contacted the Development Lead to run db:push before merging
  • Environment Variables: No environment variables changed, OR I have contacted the Development Lead to modify them on Coolify BEFORE merging.

Summary by CodeRabbit

  • New Features
    • New Issue Fetcher panel: modal overlay to browse and select issues (supports keyboard and background close, controlled or standalone visibility)
    • Multi-criteria filtering: status, team, type, date range, root-only, and free-text search
    • Refresh control and loading/error indicators
    • Emits selected issue data and exposes a refresh/update flow
    • Highlights parents with unfinished child issues and maps role names for display

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 25, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a new client-side React component IssueFetcherPane for fetching/filtering issues and role links, plus new issue-related types and default filter values in the shared constants package. (Controlled/uncontrolled open state, TRPC queries, filter UI, and data emission.)

Changes

Cohort / File(s) Summary
Issue Fetcher Component
apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx
New React component exported as IssueFetcherPane. Implements controlled/uncontrolled visibility, portal dialog, TRPC queries for issues and role links, local filter state (status, team, date range, search, kind, root-only), blocked-parent computation, filtering logic, refresh, and emits onDataChange / setIssues.
Issue Constants & Types
packages/consts/src/issue.ts
Added types and interfaces: StatusFilter, IssueKindFilter, IssueFilters, IssueFetcherPaneIssue, IssueFetcherPaneData, and DEFAULT_ISSUE_FILTERS with initial values.

Sequence Diagram

sequenceDiagram
    participant Parent
    participant IssueFetcherPane
    participant TRPC_API
    participant RoleService

    Parent->>IssueFetcherPane: mount / open (props)
    IssueFetcherPane->>TRPC_API: query issues (input from filters)
    IssueFetcherPane->>TRPC_API: query role links
    TRPC_API->>IssueFetcherPane: return issues
    TRPC_API->>RoleService: fetch role details
    RoleService->>IssueFetcherPane: return roles
    IssueFetcherPane->>IssueFetcherPane: compute blockedParentIds & roleNameById
    IssueFetcherPane->>IssueFetcherPane: apply filters (search, status, team, kind, root-only, date)
    IssueFetcherPane->>Parent: call onDataChange / setIssues with filtered data
    Parent->>IssueFetcherPane: user changes filters/UI -> rerun queries/filters
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

Blade, Constants, Feature, Minor

Suggested reviewers

  • DVidal1205
🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title includes the required issue number [#398], follows the format guidelines with a clear description of the component addition, and stays well under the 72-character limit at 53 characters.
No Hardcoded Secrets ✅ Passed Both modified files contain no hardcoded API keys, passwords, tokens, or secrets. All external API communication uses TRPC abstraction layer; constants file only defines type exports and default UI state values.
Validated Env Access ✅ Passed Search results confirm neither modified file (issue-fetcher-pane.tsx, issue.ts) contains direct process.env usage, respecting codebase conventions.
No Typescript Escape Hatches ✅ Passed Pull request contains no TypeScript escape hatches (any, @ts-ignore, @ts-expect-error, non-null assertions) and demonstrates strong type safety practices.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jakob/issue-fetcher-clean

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai bot previously requested changes Mar 25, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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/blade/src/app/_components/issues/issue-fetcher-pane-playground.tsx`:
- Around line 13-15: The header/body currently render as if there are zero
results when fetcherData is null; change the component to treat fetcherData ===
null as "not ready" by gating the header and body rendering on fetcherData !==
null (i.e., only render counts, "No issues match…" messaging, and result UI when
fetcherData is non-null), and ensure any places referencing fetcherData (the
useState [fetcherData, setFetcherData] and the IssueFetcherPaneData-dependent
sections) early-return or show a loading/placeholder until fetcherData is
populated so the pane shows a single coherent loading/error/result state.

In `@apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx`:
- Around line 84-118: The pane currently treats rolesQuery and issuesQuery
independently (rolesQuery, issuesQuery, fetchedIssues, issuesError, isLoading,
refetch, roles, roleNameById, issueErrorMessage) which allows partial renders
when one query fails; instead create a single combined query state: compute
combinedIsLoading = rolesQuery.isLoading || issuesQuery.isLoading, combinedError
= rolesQuery.error ?? issuesQuery.error (or aggregate both), and a
combinedRefetch that calls both refetch functions (e.g.
Promise.all([rolesQuery.refetch(), issuesQuery.refetch()])); use
combinedIsLoading/combinedError/combinedRefetch for all rendering, error
reporting, and the parent refresh API so the component only renders "ready" when
both queries succeed and the Refresh button retries both queries.
- Around line 219-352: Add id/htmlFor associations for each labeled control:
give the Status SelectTrigger an id (e.g., "status-select") and set the
corresponding Label htmlFor to that id; do the same for Team ("team-select"),
Type ("type-select"), Search Input ("search-input"), Date From
("date-from-input"), Date To ("date-to-input"), and the Checkbox
("rootOnly-checkbox"). Update the Label components to include htmlFor and pass
matching id props into SelectTrigger / Input / Checkbox so the Label, Select
(SelectTrigger/SelectValue), Input, and Checkbox components (used in this file)
are properly associated and remain controlled via filters/setFilters (leave
existing onValueChange/onChange/onCheckedChange handlers intact); no logic
changes to DEFAULT_ISSUE_FILTERS required.
🪄 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: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 7ea50fb7-1a8f-4ddc-9675-15fe5eff5fe6

📥 Commits

Reviewing files that changed from the base of the PR and between a2a5348 and 14b9eb1.

📒 Files selected for processing (2)
  • apps/blade/src/app/_components/issues/issue-fetcher-pane-playground.tsx
  • apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx (2)

85-118: ⚠️ Potential issue | 🟠 Major

Unify pane loading/error/refresh across both required queries.

Line 114/117 and Line 173 only track issuesQuery, so the pane can report “ready” while roles are still loading/failed, and Refresh won’t retry roles. That breaks the single-source contract for parent views.

Suggested fix
-  const rolesQuery = api.roles.getAllLinks.useQuery(undefined, {
+  const {
+    data: rolesData,
+    error: rolesError,
+    isLoading: rolesLoading,
+    refetch: refetchRoles,
+  } = api.roles.getAllLinks.useQuery(undefined, {
     refetchOnWindowFocus: false,
   });

@@
-  const {
-    data: fetchedIssues,
-    error: issuesError,
-    isLoading,
-    refetch,
-  } = issuesQuery;
-  const issueErrorMessage = issuesError?.message ?? null;
+  const {
+    data: fetchedIssues,
+    error: issuesError,
+    isLoading: issuesLoading,
+    refetch: refetchIssues,
+  } = issuesQuery;
+  const isLoading = rolesLoading || issuesLoading;
+  const errorMessage = rolesError?.message ?? issuesError?.message ?? null;

-  const roles = useMemo(() => rolesQuery.data ?? [], [rolesQuery.data]);
+  const roles = useMemo(() => rolesData ?? [], [rolesData]);

   const refresh = useCallback(() => {
-    void refetch();
-  }, [refetch]);
+    void Promise.all([refetchRoles(), refetchIssues()]);
+  }, [refetchIssues, refetchRoles]);

@@
-      error: issueErrorMessage,
+      error: errorMessage,
@@
-      issueErrorMessage,
+      errorMessage,

Based on learnings: "gating rendering should occur only when all required data fetches succeed. Do not render partial success when some queries fail; instead, implement a unified loading/state or error handling that surfaces a single, coherent state once all data is ready or failed."

Also applies to: 172-183, 224-229

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx` around lines 85
- 118, The pane currently only observes issuesQuery
(fetchedIssues/issuesError/isLoading/refetch) so rolesQuery can be
loading/failed while the UI reports "ready"; update the component to derive a
unified state from both rolesQuery and issuesQuery (e.g., combine
rolesQuery.data && issuesQuery.data for ready, rolesQuery.isLoading ||
issuesQuery.isLoading for loading, and prefer a merged error message from
rolesQuery.error or issuesQuery.error), and change the refresh handling to call
both rolesQuery.refetch() and issuesQuery.refetch() so Refresh retries both;
locate the rolesQuery and issuesQuery variables in issue-fetcher-pane.tsx to
implement this unified loading/error/refresh behavior.

234-366: ⚠️ Potential issue | 🟠 Major

Add explicit Label ↔ control associations (htmlFor/id).

The filter labels are visually present but not programmatically bound to inputs/select triggers/checkbox. This hurts screen-reader labeling and label click behavior.

Suggested fix
-          <Label>Status</Label>
+          <Label htmlFor="issue-filter-status">Status</Label>
@@
-            <SelectTrigger>
+            <SelectTrigger id="issue-filter-status">
@@
-          <Label>Team</Label>
+          <Label htmlFor="issue-filter-team">Team</Label>
@@
-            <SelectTrigger>
+            <SelectTrigger id="issue-filter-team">
@@
-          <Label>Type</Label>
+          <Label htmlFor="issue-filter-type">Type</Label>
@@
-            <SelectTrigger>
+            <SelectTrigger id="issue-filter-type">
@@
-          <Label>Search</Label>
+          <Label htmlFor="issue-filter-search">Search</Label>
           <Input
+            id="issue-filter-search"
             placeholder="Search name/description/id..."
@@
-          <Label>Date From</Label>
+          <Label htmlFor="issue-filter-date-from">Date From</Label>
           <Input
+            id="issue-filter-date-from"
             type="date"
@@
-          <Label>Date To</Label>
+          <Label htmlFor="issue-filter-date-to">Date To</Label>
           <Input
+            id="issue-filter-date-to"
             type="date"
@@
         <Checkbox
+          id="issue-filter-root-only"
           checked={filters.rootOnly}
@@
-        <Label>Show root issues only (hide subtasks)</Label>
+        <Label htmlFor="issue-filter-root-only">
+          Show root issues only (hide subtasks)
+        </Label>

As per coding guidelines: "apps/blade/** ... Accessibility (alt text, ARIA, semantic HTML)."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx` around lines
234 - 366, Labels in the filter pane are not associated with their form controls
(Label vs Input/Select/Checkbox), breaking accessibility; add explicit id
attributes to each control and set the corresponding Label htmlFor to match.
Specifically, assign unique ids for the Status Select (tie to Select or
SelectTrigger depending on the Select implementation), Team Select, Type Select,
Search/Input, Date From, Date To, and the rootOnly Checkbox, and update each
Label's htmlFor to those ids; keep ids unique (e.g., status-filter, team-filter,
type-filter, search-filter, date-from-filter, date-to-filter, root-only-filter)
and ensure any custom Select component uses the id in the actionable trigger
element so clicking the Label focuses the control.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx`:
- Around line 85-118: The pane currently only observes issuesQuery
(fetchedIssues/issuesError/isLoading/refetch) so rolesQuery can be
loading/failed while the UI reports "ready"; update the component to derive a
unified state from both rolesQuery and issuesQuery (e.g., combine
rolesQuery.data && issuesQuery.data for ready, rolesQuery.isLoading ||
issuesQuery.isLoading for loading, and prefer a merged error message from
rolesQuery.error or issuesQuery.error), and change the refresh handling to call
both rolesQuery.refetch() and issuesQuery.refetch() so Refresh retries both;
locate the rolesQuery and issuesQuery variables in issue-fetcher-pane.tsx to
implement this unified loading/error/refresh behavior.
- Around line 234-366: Labels in the filter pane are not associated with their
form controls (Label vs Input/Select/Checkbox), breaking accessibility; add
explicit id attributes to each control and set the corresponding Label htmlFor
to match. Specifically, assign unique ids for the Status Select (tie to Select
or SelectTrigger depending on the Select implementation), Team Select, Type
Select, Search/Input, Date From, Date To, and the rootOnly Checkbox, and update
each Label's htmlFor to those ids; keep ids unique (e.g., status-filter,
team-filter, type-filter, search-filter, date-from-filter, date-to-filter,
root-only-filter) and ensure any custom Select component uses the id in the
actionable trigger element so clicking the Label focuses the control.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 89eb541b-0f9e-4a80-8a50-93db5a3a4812

📥 Commits

Reviewing files that changed from the base of the PR and between 14b9eb1 and 14ecd0b.

📒 Files selected for processing (1)
  • apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx

@Jomak-x Jomak-x self-assigned this Mar 25, 2026
@Jomak-x Jomak-x added the UI Changes modifies code in the global UI package label Mar 25, 2026
@Jomak-x Jomak-x changed the title add reusable issue fetcher pane COMPONENTS - Added reusable issue fetcher pane Mar 25, 2026
@Jomak-x Jomak-x changed the title COMPONENTS - Added reusable issue fetcher pane [#398] COMPONENTS - Added reusable issue fetcher pane Mar 25, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx (1)

92-125: ⚠️ Potential issue | 🟠 Major

Destructure query fields before memoizing refresh to prevent unnecessary parent re-renders.

The refresh callback depends on full rolesQuery and issuesQuery objects (line 179). When filters change, the queryInput changes (line 113), causing issuesQuery to be a new object reference. This recreates refresh, which then recreates the data object (line 188), triggering onDataChange (line 208) even when nothing meaningful changed.

Destructuring individual fields (refetch, isLoading, error, data) makes dependencies explicit and prevents cascading re-renders—the component already partially does this elsewhere (lines 124, 131).

Suggested fix
-  const rolesQuery = api.roles.getAllLinks.useQuery(undefined, {
+  const {
+    data: rolesData,
+    error: rolesError,
+    isLoading: rolesLoading,
+    refetch: refetchRoles,
+  } = api.roles.getAllLinks.useQuery(undefined, {
     refetchOnWindowFocus: false,
   });
@@
-  const issuesQuery = api.issues.getAllIssues.useQuery(queryInput, {
+  const {
+    data: fetchedIssues,
+    error: issuesError,
+    isLoading: issuesLoading,
+    refetch: refetchIssues,
+  } = api.issues.getAllIssues.useQuery(queryInput, {
     refetchOnWindowFocus: false,
   });
-  const { data: fetchedIssues } = issuesQuery;
-  const combinedIsLoading = rolesQuery.isLoading || issuesQuery.isLoading;
-  const combinedError = rolesQuery.error ?? issuesQuery.error;
+  const combinedIsLoading = rolesLoading || issuesLoading;
+  const combinedError = rolesError ?? issuesError;
@@
-  const roles = useMemo(() => rolesQuery.data ?? [], [rolesQuery.data]);
+  const roles = useMemo(() => rolesData ?? [], [rolesData]);
@@
   const refresh = useCallback(() => {
-    void Promise.all([rolesQuery.refetch(), issuesQuery.refetch()]);
-  }, [issuesQuery, rolesQuery]);
+    void Promise.all([refetchRoles(), refetchIssues()]);
+  }, [refetchIssues, refetchRoles]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx` around lines 92
- 125, Destructure the needed fields (refetch, isLoading, error, data) out of
rolesQuery and issuesQuery before creating the useMemo that defines refresh and
the derived data object; update the useMemo dependencies to reference those
destructured variables (e.g., rolesRefetch, rolesIsLoading, rolesError,
rolesData, issuesRefetch, issuesIsLoading, issuesError, issuesData) instead of
the whole rolesQuery/issuesQuery objects so refresh and the produced data object
only change when actual query results or refetch functions change; ensure
isReady, combinedError/combinedErrorMessage, and the object passed to
onDataChange use the destructured values and that refresh calls the destructured
refetch functions.
🤖 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/blade/src/app/_components/issues/issue-fetcher-pane.tsx`:
- Line 3: The component IssueFetcherPane currently uses hardcoded string IDs for
form controls (e.g., statusSelectId and related id/htmlFor pairs), causing
duplicate IDs across multiple instances; replace those hardcoded IDs by
importing and calling React's useId() inside IssueFetcherPane to generate
per-instance IDs, assign the generated ids to the controls' id attributes and
update every htmlFor and any client-side references to use those variables (keep
the existing variable names like statusSelectId but initialize them via useId())
so each instance has unique control IDs.

---

Duplicate comments:
In `@apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx`:
- Around line 92-125: Destructure the needed fields (refetch, isLoading, error,
data) out of rolesQuery and issuesQuery before creating the useMemo that defines
refresh and the derived data object; update the useMemo dependencies to
reference those destructured variables (e.g., rolesRefetch, rolesIsLoading,
rolesError, rolesData, issuesRefetch, issuesIsLoading, issuesError, issuesData)
instead of the whole rolesQuery/issuesQuery objects so refresh and the produced
data object only change when actual query results or refetch functions change;
ensure isReady, combinedError/combinedErrorMessage, and the object passed to
onDataChange use the destructured values and that refresh calls the destructured
refetch functions.
🪄 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: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: b78b738a-6df4-4408-b882-212a1a05b998

📥 Commits

Reviewing files that changed from the base of the PR and between 14ecd0b and 8631236.

📒 Files selected for processing (2)
  • apps/blade/src/app/_components/issues/issue-fetcher-pane-playground.tsx
  • apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx
✅ Files skipped from review due to trivial changes (1)
  • apps/blade/src/app/_components/issues/issue-fetcher-pane-playground.tsx

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx (1)

104-108: Minor: Avoid repeated array spreads in the loop.

Each [...current, issue] creates a new array, resulting in O(n²) allocations for issues with the same parent. For larger datasets, using push is more efficient:

♻️ Suggested optimization
   for (const issue of allIssues) {
     if (!issue.parent) continue;
-    const current = childrenByParent.get(issue.parent) ?? [];
-    childrenByParent.set(issue.parent, [...current, issue]);
+    let current = childrenByParent.get(issue.parent);
+    if (!current) {
+      current = [];
+      childrenByParent.set(issue.parent, current);
+    }
+    current.push(issue);
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx` around lines
104 - 108, The loop building childrenByParent is creating new arrays each
iteration with [...current, issue], causing extra allocations; update the loop
that iterates over allIssues so that for each issue with a parent you get or
create the array in childrenByParent (using childrenByParent.get(issue.parent)
or childrenByParent.has/ set) and call .push(issue) onto that existing array
instead of replacing it with a spread, ensuring you initialize an empty array
the first time you encounter a parent.
🤖 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/blade/src/app/_components/issues/issue-fetcher-pane.tsx`:
- Around line 104-108: The loop building childrenByParent is creating new arrays
each iteration with [...current, issue], causing extra allocations; update the
loop that iterates over allIssues so that for each issue with a parent you get
or create the array in childrenByParent (using
childrenByParent.get(issue.parent) or childrenByParent.has/ set) and call
.push(issue) onto that existing array instead of replacing it with a spread,
ensuring you initialize an empty array the first time you encounter a parent.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: a8decf8a-85e8-4c4e-926f-9ef8491ca048

📥 Commits

Reviewing files that changed from the base of the PR and between 8631236 and 505e977.

📒 Files selected for processing (2)
  • apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx
  • packages/consts/src/issue.ts

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx (1)

120-137: blockedParentIds can be derived in one pass.

This only needs to answer “does this parent have any unfinished child?”. The current group-then-scan approach allocates a new array on every insert and walks the data twice.

Suggested simplification
   const blockedParentIds = useMemo(() => {
-    const childrenByParent = new Map<string, ISSUE.IssueFetcherPaneIssue[]>();
-
-    for (const issue of allIssues) {
-      if (!issue.parent) continue;
-      const current = childrenByParent.get(issue.parent) ?? [];
-      childrenByParent.set(issue.parent, [...current, issue]);
-    }
-
     const blockedParents = new Set<string>();
-    for (const [parentId, children] of childrenByParent.entries()) {
-      if (children.some((child) => child.status !== "FINISHED")) {
-        blockedParents.add(parentId);
-      }
+    for (const issue of allIssues) {
+      if (issue.parent && issue.status !== "FINISHED") {
+        blockedParents.add(issue.parent);
+      }
     }
 
     return blockedParents;
   }, [allIssues]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx` around lines
120 - 137, The current useMemo for blockedParentIds builds childrenByParent and
then scans it, allocating arrays on each insert and doing two passes; change it
to a single-pass algorithm inside the same useMemo: create a Set
(blockedParents), iterate once over allIssues, for each issue with issue.parent
if issue.status !== "FINISHED" add issue.parent to blockedParents (no array
allocations), then return blockedParents; update/remove references to
childrenByParent and the two-loop logic in the useMemo that defines
blockedParentIds.
🤖 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/blade/src/app/_components/issues/issue-fetcher-pane.tsx`:
- Around line 76-93: The queryInput useMemo currently allows dateFrom > dateTo
(see parseLocalDate usage and assignment to input.dateFrom/input.dateTo), so
update the logic that builds queryInput (and the equivalent logic near the other
occurrences) to detect an inverted range: after parsing parsedDateFrom and
parsedDateTo, if both exist and parsedDateFrom > parsedDateTo, do not include
the invalid range in the returned input and instead set a validation error
flag/message (e.g., set an inline validation state like dateRangeError) and
ensure the component's isReady remains false until corrected; apply the same
inversion check and validation behavior for the other similar blocks noted
(lines ~104-107 and ~385-412) so the UI surfaces an inline validation error
rather than treating the pane as ready.
- Around line 58-64: The dialog lacks an accessible name/description and does
not manage focus on open/close; inside the IssueFetcherPane component, give the
modal element an explicit aria-labelledby that points to the pane header's id
(add an id to the header element) and aria-describedby to a short description
element if present, then implement focus management: store
document.activeElement in a ref before opening, move focus into the dialog
(either to the dialog container with tabIndex={-1} or the first focusable
control) when the pane opens, and on cleanup restore focus to the previously
focused element; update any existing effect that runs on open/close (the effect
around the modal open logic) to perform these steps and use the existing
useId-generated ids (e.g., statusSelectId, teamSelectId, typeSelectId,
searchInputId, dateFromInputId, dateToInputId, rootOnlyCheckboxId) only for form
controls while adding a new headerId/descriptionId for
aria-labelledby/aria-describedby and ensure the element with role="dialog"
references those ids.

---

Nitpick comments:
In `@apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx`:
- Around line 120-137: The current useMemo for blockedParentIds builds
childrenByParent and then scans it, allocating arrays on each insert and doing
two passes; change it to a single-pass algorithm inside the same useMemo: create
a Set (blockedParents), iterate once over allIssues, for each issue with
issue.parent if issue.status !== "FINISHED" add issue.parent to blockedParents
(no array allocations), then return blockedParents; update/remove references to
childrenByParent and the two-loop logic in the useMemo that defines
blockedParentIds.
🪄 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: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 74aa563b-569f-4693-9b77-558b96d4ed2b

📥 Commits

Reviewing files that changed from the base of the PR and between 505e977 and bc1e8c8.

📒 Files selected for processing (1)
  • apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

UI Changes modifies code in the global UI package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant