feat: add repositories & volumes column sorting - #808
Conversation
|
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Hello @antoine-sh, thank you for your contribution! I’m not a big fan of adding more table-specific logic directly inside the business components. To be fair, the existing search filtering probably should not have lived there either. I initially kept it simple, but with sorting and more advanced filtering being added, I think it’s a good time to revisit the approach. Rather than extending this implementation here, I’d prefer to introduce a reusable table abstraction based on TanStack Table, so sorting/filtering/pagination can live in an isolated component and be reused consistently across the app. I’ll take care of that part myself so we avoid duplicating this logic in every table. Your PR is still helpful in clarifying the desired behavior, so thanks again for pushing this forward. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/client/modules/notifications/routes/notifications.tsx (1)
124-133:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing "generic" notification type in filter dropdown.
The
NotificationRowtype includes"generic"in the type union (line 29), but the filter dropdown doesn't include it as an option. Users won't be able to filter by this type.🔧 Proposed fix
<SelectContent> <SelectItem value="email">Email</SelectItem> <SelectItem value="slack">Slack</SelectItem> <SelectItem value="discord">Discord</SelectItem> <SelectItem value="gotify">Gotify</SelectItem> <SelectItem value="ntfy">Ntfy</SelectItem> <SelectItem value="pushover">Pushover</SelectItem> <SelectItem value="telegram">Telegram</SelectItem> <SelectItem value="custom">Custom</SelectItem> + <SelectItem value="generic">Generic</SelectItem> </SelectContent>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/modules/notifications/routes/notifications.tsx` around lines 124 - 133, The filter dropdown in the notifications UI is missing the "generic" option even though NotificationRow's type union includes "generic"; update the SelectContent options in the notifications route component by adding a SelectItem with value "generic" (label "Generic") alongside the existing values so the UI can filter NotificationRow items of type "generic".
🧹 Nitpick comments (2)
app/client/modules/repositories/routes/repositories.tsx (2)
100-100: ⚡ Quick winUnsafe type assertion bypasses type checking.
Same issue as in
volumes.tsx—castingdata as RepositoryRow[]assumes the API response matchesRepositoryRow. Consider using proper response typing from the generated API client.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/modules/repositories/routes/repositories.tsx` at line 100, Replace the unsafe assertion "const repositories = data as RepositoryRow[]" by using the generated API client's proper response typing (or a typed helper) so the compiler verifies the shape instead of bypassing it; update the call that produces `data` to return a typed response (e.g., the generated method that yields RepositoryRow[] or use a generic on your fetch helper), then assign `repositories` from that strongly-typed result (referencing `data`, `repositories`, and the `RepositoryRow` type and the generated API client method used to fetch repositories) and remove the direct "as RepositoryRow[]" cast.
73-87: Add explicit handling for "doctor" status in both filter and column variant.The
RepositoryStatustype includes "doctor" (and "cancelled") values, and "doctor" is actively used in the codebase (e.g.,repository.status === "doctor"inrepository-details.tsx). However, the status filter dropdown (around lines 150–160) only includes "healthy", "error", and "unknown", and theStatusDotcomponent treats "doctor" as "warning" via fallback rather than a distinct visual representation.If "doctor" represents a transient state and shouldn't be filterable, add a comment explaining this. If it should be filterable and visually distinct, add it to both the filter options and the
StatusDotvariant logic. Similarly, consider whether "cancelled" deserves explicit handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/modules/repositories/routes/repositories.tsx` around lines 73 - 87, The status column and filter need explicit handling for the "doctor" (and optionally "cancelled") RepositoryStatus values: update the column cell logic in the column with id "status" (where StatusDot is rendered) to map "doctor" to a distinct StatusDot variant (e.g., "info" or "doctor") instead of relying on the fallback, and update the status filter options (the filter dropdown code that feeds filterFn) to include "doctor" (and "cancelled" if applicable) so filterFn (row.getValue(id) === value) can match them; if "doctor" is intentionally not filterable, add a clear comment near the filter options explaining that decision. Ensure any new variant name used for StatusDot is supported by the StatusDot component or add a corresponding mapping before changing the column cell.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/client/modules/notifications/routes/notifications.tsx`:
- Around line 124-133: The filter dropdown in the notifications UI is missing
the "generic" option even though NotificationRow's type union includes
"generic"; update the SelectContent options in the notifications route component
by adding a SelectItem with value "generic" (label "Generic") alongside the
existing values so the UI can filter NotificationRow items of type "generic".
---
Nitpick comments:
In `@app/client/modules/repositories/routes/repositories.tsx`:
- Line 100: Replace the unsafe assertion "const repositories = data as
RepositoryRow[]" by using the generated API client's proper response typing (or
a typed helper) so the compiler verifies the shape instead of bypassing it;
update the call that produces `data` to return a typed response (e.g., the
generated method that yields RepositoryRow[] or use a generic on your fetch
helper), then assign `repositories` from that strongly-typed result (referencing
`data`, `repositories`, and the `RepositoryRow` type and the generated API
client method used to fetch repositories) and remove the direct "as
RepositoryRow[]" cast.
- Around line 73-87: The status column and filter need explicit handling for the
"doctor" (and optionally "cancelled") RepositoryStatus values: update the column
cell logic in the column with id "status" (where StatusDot is rendered) to map
"doctor" to a distinct StatusDot variant (e.g., "info" or "doctor") instead of
relying on the fallback, and update the status filter options (the filter
dropdown code that feeds filterFn) to include "doctor" (and "cancelled" if
applicable) so filterFn (row.getValue(id) === value) can match them; if "doctor"
is intentionally not filterable, add a clear comment near the filter options
explaining that decision. Ensure any new variant name used for StatusDot is
supported by the StatusDot component or add a corresponding mapping before
changing the column cell.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81af03cf-b02f-4e2e-adc0-9f4e0b899edc
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
app/client/components/data-table-sort-header.tsxapp/client/components/status-dot.tsxapp/client/modules/notifications/routes/notifications.tsxapp/client/modules/repositories/routes/repositories.tsxapp/client/modules/volumes/routes/volumes.tsxpackage.json
3 participants
A proposition to solve #788 issue by adding sorting to columns on Volumes & Repositories pages.
Summary by CodeRabbit
New Features
Style