[#398] COMPONENTS - Added reusable issue fetcher pane#429
[#398] COMPONENTS - Added reusable issue fetcher pane#429
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new client-side React component Changes
Sequence DiagramsequenceDiagram
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
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
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
📒 Files selected for processing (2)
apps/blade/src/app/_components/issues/issue-fetcher-pane-playground.tsxapps/blade/src/app/_components/issues/issue-fetcher-pane.tsx
apps/blade/src/app/_components/issues/issue-fetcher-pane-playground.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx (2)
85-118:⚠️ Potential issue | 🟠 MajorUnify 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 | 🟠 MajorAdd 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
📒 Files selected for processing (1)
apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx (1)
92-125:⚠️ Potential issue | 🟠 MajorDestructure query fields before memoizing
refreshto prevent unnecessary parent re-renders.The
refreshcallback depends on fullrolesQueryandissuesQueryobjects (line 179). Whenfilterschange, thequeryInputchanges (line 113), causingissuesQueryto be a new object reference. This recreatesrefresh, which then recreates thedataobject (line 188), triggeringonDataChange(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
📒 Files selected for processing (2)
apps/blade/src/app/_components/issues/issue-fetcher-pane-playground.tsxapps/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
There was a problem hiding this comment.
🧹 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, usingpushis 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
📒 Files selected for processing (2)
apps/blade/src/app/_components/issues/issue-fetcher-pane.tsxpackages/consts/src/issue.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx (1)
120-137:blockedParentIdscan 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
📒 Files selected for processing (1)
apps/blade/src/app/_components/issues/issue-fetcher-pane.tsx
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
Checklist
db:pushbefore mergingSummary by CodeRabbit