Merged
Conversation
crespofer
added
Minor
|
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 client-side Changes
Sequence DiagramsequenceDiagram
actor User
participant Page as IssueListPage (Server)
participant Auth as Auth System
participant RolesAPI as Roles/TRPC
participant Client as IssuesList (Client)
participant Pane as IssueFetcherPane
User->>Page: GET /issues/list
Page->>Auth: auth()
alt No session
Auth-->>Page: null
Page-->>User: Redirect to sign-in
else Session present
Auth-->>Page: session
Page->>RolesAPI: roles.hasPermission([READ_ISSUES,EDIT_ISSUES,EDIT_ISSUE_TEMPLATES,READ_ISSUE_TEMPLATES])
alt Permission denied
RolesAPI-->>Page: false
Page-->>User: 404 Not Found
else Permission granted
RolesAPI-->>Page: true
Page-->>Client: Render IssuesList
Client->>Pane: mount / initialize
Pane->>RolesAPI: fetch issues (filters)
RolesAPI-->>Pane: issues data
Pane-->>Client: paneData (issues, filters, roleNameById, refresh)
Client-->>User: Render issues table + controls
User->>Client: Click delete issue
Client->>RolesAPI: deleteIssue(id)
alt Deletion success
RolesAPI-->>Client: success
Client->>RolesAPI: invalidate issues query
Client->>Pane: paneData.refresh()
Client-->>User: Success toast
else Deletion failed
RolesAPI-->>Client: error
Client-->>User: Error toast
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/blade/src/app/_components/issue-list/issues-list.tsx (2)
193-193: Make edit button labels row-specific
aria-label="Edit issue"is repeated for every row; include the issue name so assistive tech users can distinguish actions quickly.💡 Suggested fix
- aria-label="Edit issue" + aria-label={`Edit issue ${issue.name}`}As per coding guidelines,
apps/blade/**requires attention to “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/issue-list/issues-list.tsx` at line 193, The edit button currently uses a static aria-label ("Edit issue") for every row; update the button in the issues list component (e.g., the Edit button inside IssuesList / IssueRow in apps/blade/src/app/_components/issue-list/issues-list.tsx) to include the row-specific issue name (e.g., use issue.title or issue.name) so the aria-label becomes unique per row (like "Edit issue: {issue.title}"), ensuring assistive tech users can distinguish actions.
118-124: Use semantic table markup for this tabular dataThis is structured as a 4-column data table but rendered with
divs, which weakens screen-reader navigation and header-cell associations. Prefer<table>/<thead>/<tbody>/<tr>/<th>/<td>.As per coding guidelines,
apps/blade/**requires attention to “Accessibility (alt text, ARIA, semantic HTML)”.Also applies to: 145-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/issue-list/issues-list.tsx` around lines 118 - 124, The current issues-list component renders a 4-column data grid using divs (the container with className "overflow-hidden rounded-lg border" and the header row using "hidden grid-cols-[1fr_auto_auto_auto] ... md:grid"); change this to proper semantic table markup by replacing the header row div with a <table> structure: <thead> containing a <tr> with four <th> elements ("Issue", "Status", "Due", "Edit") and the body rows (the section around lines 145-199) as <tbody> with <tr> and <td> cells, preserving existing styling classes (apply them to the table, thead, tr, th, td as appropriate) and ensure responsive variants (md:grid) are migrated to equivalent table styling or utility classes so screen readers get correct header-cell associations and keyboard accessibility.
🤖 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/issue-list/issues-list.tsx`:
- Around line 170-177: The form's initialValues in issues-list.tsx should not
default a missing due date to new Date(); change the date initializer used by
initialValues (the object passed into the form in the issues-list.tsx component,
specifically the date property in the initialValues block) from issue.date ??
new Date() to issue.date ?? null (or undefined) so an absent due date remains
empty in edit mode and won't be overwritten on save; also ensure the date form
field/handler in the same component accepts null/undefined as a valid empty
value so saving doesn't write today's date.
---
Nitpick comments:
In `@apps/blade/src/app/_components/issue-list/issues-list.tsx`:
- Line 193: The edit button currently uses a static aria-label ("Edit issue")
for every row; update the button in the issues list component (e.g., the Edit
button inside IssuesList / IssueRow in
apps/blade/src/app/_components/issue-list/issues-list.tsx) to include the
row-specific issue name (e.g., use issue.title or issue.name) so the aria-label
becomes unique per row (like "Edit issue: {issue.title}"), ensuring assistive
tech users can distinguish actions.
- Around line 118-124: The current issues-list component renders a 4-column data
grid using divs (the container with className "overflow-hidden rounded-lg
border" and the header row using "hidden grid-cols-[1fr_auto_auto_auto] ...
md:grid"); change this to proper semantic table markup by replacing the header
row div with a <table> structure: <thead> containing a <tr> with four <th>
elements ("Issue", "Status", "Due", "Edit") and the body rows (the section
around lines 145-199) as <tbody> with <tr> and <td> cells, preserving existing
styling classes (apply them to the table, thead, tr, th, td as appropriate) and
ensure responsive variants (md:grid) are migrated to equivalent table styling or
utility classes so screen readers get correct header-cell associations and
keyboard accessibility.
🪄 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: 267e93e4-d166-466a-aa68-8747d9197e95
📒 Files selected for processing (2)
apps/blade/src/app/_components/issue-list/issues-list.tsxapps/blade/src/app/issues/list/page.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/issue-list/issues-list.tsx`:
- Around line 45-47: The onError handler in issues-list.tsx (the onError:
(error) => { ... } block) and the UI that renders paneData.error should not
display raw backend messages to users; change both places to show a generic
user-facing message like "Something went wrong, please try again" while
retaining the detailed error for developers by sending the full error to your
observability/logging helper (e.g., call reportError(error) or
processLogger.error(error)) or console.error for now; update the onError
callback and the component that reads paneData.error (lines around the
paneData.error usage) to use the generic copy for UI and route the original
error to logs only.
- Around line 94-97: The Create/Edit controls are shown regardless of write
permission; add a write-permission check and hide/disable those controls when
not allowed: call the roles permission check (e.g., api.roles.hasPermission or a
new useWritePermission hook) from IssuesList (or inside CreateEditDialog) to
determine canWrite, then conditionally render or disable <CreateEditDialog
intent="create">, its trigger Button ("Create issue"), <IssueTemplateDialog>,
and the other dialog triggers referenced around lines 168-197 when canWrite is
false; ensure checks use the "EDIT_ISSUES" permission and propagate the boolean
down as a prop or read it inside the dialog components.
🪄 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: 0f454dd6-07d2-4540-9e2e-65f7e260a80c
📒 Files selected for processing (2)
apps/blade/src/app/_components/issue-list/issues-list.tsxapps/blade/src/app/issues/list/page.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/blade/src/app/issues/list/page.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/blade/src/app/_components/issue-list/issues-list.tsx (1)
54-58: Consider separating header totals from the rendered row set.
openCount/closedCountcome from the sameissuesarray used for the visible rows. Once a status filter narrows that list, the opposite bucket naturally drops to0, so the header stops behaving like the GitHub-style open/closed summary this page is aiming for. Prefer surfacing status totals fromIssueFetcherPanebefore the status filter is applied and render those here.Possible direction
- const openCount = useMemo( - () => issues.filter((issue) => issue.status !== "FINISHED").length, - [issues], - ); - const closedCount = issues.length - openCount; + const openCount = paneData?.statusCounts.open ?? 0; + const closedCount = paneData?.statusCounts.closed ?? 0;This needs a matching
statusCountsaddition inIssueFetcherPaneData/IssueFetcherPane, computed beforefilters.statusFiltertrims the list.Also applies to: 84-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/issue-list/issues-list.tsx` around lines 54 - 58, The header open/closed totals (openCount/closedCount) are being computed from the filtered issues list so they change with the status filter; compute and pass status totals separately from the visible rows: add a statusCounts (or similar) property to IssueFetcherPaneData in IssueFetcherPane that computes counts for each Issue.status before applying filters.statusFilter, then change issues-list.tsx to derive openCount/closedCount from that statusCounts instead of from the filtered issues array (also update any other places using openCount/closedCount around the 84-90 region to use the new statusCounts).
🤖 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/issue-list/issues-list.tsx`:
- Around line 118-123: The header containing "Status / Due / Edit" is hidden
below md (the div with className="hidden grid-cols-[1fr_auto_auto_auto] ...
md:grid"), leaving mobile users and assistive tech without column context;
either convert the component to semantic table markup or, if keeping the grid,
add per-cell labels (hidden-on-desktop visible-on-mobile) inside each row
cell—e.g., insert visually-hidden or mobile-visible <span> labels for "Status"
and "Due" before the cell values and give the edit button a descriptive label
like "Edit issue {title}" or an adjacent row-specific "Edit" text for screen
readers; update the row rendering logic in issues-list.tsx accordingly so every
Status/Due/Edit cell provides persistent context on small screens.
---
Nitpick comments:
In `@apps/blade/src/app/_components/issue-list/issues-list.tsx`:
- Around line 54-58: The header open/closed totals (openCount/closedCount) are
being computed from the filtered issues list so they change with the status
filter; compute and pass status totals separately from the visible rows: add a
statusCounts (or similar) property to IssueFetcherPaneData in IssueFetcherPane
that computes counts for each Issue.status before applying filters.statusFilter,
then change issues-list.tsx to derive openCount/closedCount from that
statusCounts instead of from the filtered issues array (also update any other
places using openCount/closedCount around the 84-90 region to use the new
statusCounts).
🪄 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: debe2d9b-d562-450c-a435-79af385844a2
📒 Files selected for processing (2)
apps/blade/src/app/_components/issue-list/issues-list.tsxapps/blade/src/app/issues/list/page.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/blade/src/app/issues/list/page.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/blade/src/app/_components/issue-list/issues-list.tsx (1)
28-31: RedundantDateconstructor.Since
valueis already typed asDatewhen reaching line 30 (null case returns early), wrapping it innew Date()is unnecessary.✨ Suggested simplification
function formatDate(value: Date | null) { if (!value) return "No due date"; - return new Date(value).toLocaleDateString(); + return value.toLocaleDateString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/issue-list/issues-list.tsx` around lines 28 - 31, The formatDate function wraps an already-typed Date in a redundant new Date() call; update the function (formatDate) to return value.toLocaleDateString() after the null check instead of new Date(value).toLocaleDateString() so the existing Date object is used directly.
🤖 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/issue-list/issues-list.tsx`:
- Around line 28-31: The formatDate function wraps an already-typed Date in a
redundant new Date() call; update the function (formatDate) to return
value.toLocaleDateString() after the null check instead of new
Date(value).toLocaleDateString() so the existing Date object is used directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5832bb42-ce58-4c35-8970-71b135e28a33
📒 Files selected for processing (1)
apps/blade/src/app/_components/issue-list/issues-list.tsx
crespofer
force-pushed
the
blade/issue-list-page
branch
from
aa96f20 to
6aa0b25
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/blade/src/app/_components/issue-list/issues-list.tsx (3)
73-74: Date filter tags display raw values—consider user-friendly formatting.
filters.dateFromandfilters.dateToare displayed directly. Depending on their format (e.g.,"2026-04-08"), this may not be the most readable for users. Consider using yourformatDatehelper ortoLocaleDateString()for consistency with the due date column.♻️ Optional improvement
- if (filters.dateFrom) tags.push("From " + filters.dateFrom); - if (filters.dateTo) tags.push("To " + filters.dateTo); + if (filters.dateFrom) + tags.push("From " + new Date(filters.dateFrom).toLocaleDateString()); + if (filters.dateTo) + tags.push("To " + new Date(filters.dateTo).toLocaleDateString());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/issue-list/issues-list.tsx` around lines 73 - 74, The tag builder currently pushes raw filters.dateFrom and filters.dateTo strings into tags (in IssuesList or the component building those tags); update that logic to format dates before pushing—use the existing formatDate helper (or Date.prototype.toLocaleDateString()) to convert filters.dateFrom and filters.dateTo into the same user-friendly format used in the due date column, then push "From " + formattedDate and "To " + formattedDate instead so displayed tags are readable and consistent.
28-31: Consider simplifying the Date constructor call.Since
valueis already typed asDate | nulland you've guarded againstnull, thenew Date(value)wrapper is technically redundant. However, if dates arrive JSON-serialized as strings from the API, this is a valid defensive pattern.If you're certain dates are always
Dateobjects (not strings), you can simplify:♻️ Optional simplification
function formatDate(value: Date | null) { if (!value) return "No due date"; - return new Date(value).toLocaleDateString(); + return value.toLocaleDateString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/issue-list/issues-list.tsx` around lines 28 - 31, The formatDate function currently does new Date(value) even though value is typed Date | null and null is guarded; if your API always supplies Date objects, simplify by returning value.toLocaleDateString() directly in formatDate; if dates may be JSON strings, instead coerce safely by detecting typeof value === "string" and parsing only then (or keep new Date(value) as a defensive parse) — update the formatDate function accordingly to either remove the redundant new Date() wrapper or add a typeof check to handle string inputs.
54-58: Minor inconsistency:openCountis memoized butclosedCountis not.This works correctly since subtraction is cheap, but for consistency you could either memoize both or neither. The current approach is fine for performance—just a style observation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/issue-list/issues-list.tsx` around lines 54 - 58, openCount is memoized via useMemo while closedCount is computed directly, creating a minor inconsistency in style; update the code for consistency by either memoizing closedCount as well (e.g., useMemo(() => issues.length - openCount, [issues, openCount]) or useMemo(() => issues.length - openCount, [issues]) depending on how you compute openCount) or remove the useMemo wrapper from openCount so both are computed directly — adjust the references to the affected symbols openCount, closedCount, and useMemo accordingly.
🤖 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/issue-list/issues-list.tsx`:
- Around line 73-74: The tag builder currently pushes raw filters.dateFrom and
filters.dateTo strings into tags (in IssuesList or the component building those
tags); update that logic to format dates before pushing—use the existing
formatDate helper (or Date.prototype.toLocaleDateString()) to convert
filters.dateFrom and filters.dateTo into the same user-friendly format used in
the due date column, then push "From " + formattedDate and "To " + formattedDate
instead so displayed tags are readable and consistent.
- Around line 28-31: The formatDate function currently does new Date(value) even
though value is typed Date | null and null is guarded; if your API always
supplies Date objects, simplify by returning value.toLocaleDateString() directly
in formatDate; if dates may be JSON strings, instead coerce safely by detecting
typeof value === "string" and parsing only then (or keep new Date(value) as a
defensive parse) — update the formatDate function accordingly to either remove
the redundant new Date() wrapper or add a typeof check to handle string inputs.
- Around line 54-58: openCount is memoized via useMemo while closedCount is
computed directly, creating a minor inconsistency in style; update the code for
consistency by either memoizing closedCount as well (e.g., useMemo(() =>
issues.length - openCount, [issues, openCount]) or useMemo(() => issues.length -
openCount, [issues]) depending on how you compute openCount) or remove the
useMemo wrapper from openCount so both are computed directly — adjust the
references to the affected symbols openCount, closedCount, and useMemo
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b2f805f1-dd3e-4390-9265-c7d25505b27f
📒 Files selected for processing (1)
apps/blade/src/app/_components/issue-list/issues-list.tsx
DVidal1205
requested changes
Apr 8, 2026
Contributor
DVidal1205
left a comment
DVidal1205
left a comment
There was a problem hiding this comment.
this is actually beautiful. so good. just want 2 more quick adds that i think will be huge for quality of life in this view:
- we need a quick status button. like, a quick way to go from in progress -> finished. maybe where u show the status as (In Progress) it's a select dropdown/dialog, you can set to finished, and click done. its redundant but big for usability
- can we have a definitive UI signal for overdue tasks? maybe like make the text a dark red?
- the alignment of the status, due, and edit text could use some work. right now the word "Status" is hardly over the status tag.
- no need to include the issue ID under the issue name, instead put the last updated date and time so we can see if things are "active" or live. then still keep the diff teams, just drop the word "Team". like it shows "Team Workshop Team" on the video you sent, should just say "Workshop"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Have a page to be able to view and manage issues/events similar to the github issues page.
What
Creates a component to manage actions related to issues (view/edit/create)
Creates a new page to view issues as a list
Closes: #396
Test Plan
Checklist
db:pushbefore mergingSummary by CodeRabbit