fix: respect dashboard task column filters#1177
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR enforces dashboard task-list column filters in the ChangesTask column filtering feature
Desktop release test tweak
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for filtering the dashboard task list endpoint by persisted column value, validating the input against allowed columns.
Changes:
- Adds a
columnquery parameter toGET /api/tasks, validated viaisColumnand forwarded toscopedStore.listTasks. - Adds tests covering valid column filtering and rejection of invalid column values.
- Documents the column enum contract and adds a changeset.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/dashboard/src/routes/register-task-workflow-routes.ts | Parses, validates, and forwards column query param to the store. |
| packages/dashboard/src/tests/routes-tasks.test.ts | Adds tests for valid/invalid column filter behavior. |
| docs/task-management.md | Clarifies persisted column enum values vs. UI labels. |
| .changeset/dashboard-column-filter.md | Adds patch changeset entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const res = await GET(buildApp(), `/api/tasks?column=${column}&limit=20`); | ||
|
|
||
| expect(res.status).toBe(200); | ||
| expect(res.body).toHaveLength(1); |
Greptile SummaryThis PR wires up the
Confidence Score: 5/5The change is narrow and additive — it wires up an existing store capability that was already well-exercised elsewhere in the codebase. The listTasks store method already accepted column?: Column and used it correctly in many call sites; this PR simply routes the HTTP query parameter through the same validated path. The type guard isColumn is shared with other validated inputs and the 400 path is covered by a dedicated test. No data-mutation path is touched. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Route as GET /api/tasks
participant Store as TaskStore
Client->>Route: "?column=triage&limit=20"
Note over Route: columnParam = "triage"<br/>isColumn("triage") true<br/>column = "triage"
Route->>Store: "listTasks({ limit:20, slim:true, includeArchived:false, column:"triage" })"
Store-->>Route: Task[] filtered to "triage"
Route-->>Client: 200 [ ...triage tasks ]
Client->>Route: "?column=Planning"
Note over Route: columnParam = "Planning"<br/>isColumn("Planning") false<br/>column = undefined
Note over Route: columnParam && !column 400
Route-->>Client: "400 { error: "column must be one of: ..." }"
Client->>Route: "?q=search&limit=10"
Note over Route: q branch taken<br/>column not forwarded
Route->>Store: "searchTasks("search", { limit:10, ... })"
Store-->>Route: Task[]
Route-->>Client: 200 [ ...results ]
Reviews (3): Last reviewed commit: "fix: respect dashboard task column filte..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -764,7 +769,8 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork | |||
| // Board-view list: omit the heavy agent log payload and exclude | |||
| // archived tasks unless explicitly requested. Full task detail still loads via | |||
| // GET /api/tasks/:id. Without this, every dashboard load shipped tens of MB of agent logs. | |||
| tasks = await scopedStore.listTasks({ limit, offset, slim: true, includeArchived }); | |||
| const listOptions = { limit, offset, slim: true, includeArchived, ...(column ? { column } : {}) }; | |||
| tasks = await scopedStore.listTasks(listOptions); | |||
There was a problem hiding this comment.
q is also provided
The column validation runs unconditionally before the q branch, but column is never forwarded to searchTasks. This creates two inconsistent outcomes for callers who pass both parameters: ?q=foo&column=Planning returns 400 (misleading, since column has no effect during search), and ?q=foo&column=triage returns 200 but the column restriction is silently dropped — the caller receives unfiltered search results. Either the validation should be gated behind !q, or column should be forwarded to searchTasks (which also lacks the parameter in its signature at packages/core/src/store.ts:4819).
![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)
packages/dashboard/src/routes/register-task-workflow-routes.ts (1)
766-773:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
columnfilter is dropped whenqis presentLine 766 routes search requests through
searchTasks(...)without applyingcolumn, soGET /api/tasks?q=...&column=triagecan return non-triagetasks. Apply the validated column filter in the search path too.Suggested fix
- if (q && q.length > 0) { - tasks = await scopedStore.searchTasks(q, { limit, offset, slim: true, includeArchived }); + if (q && q.length > 0) { + const searchResults = await scopedStore.searchTasks(q, { limit, offset, slim: true, includeArchived }); + tasks = column ? searchResults.filter((task) => task.column === column) : searchResults; } else { // Board-view list: omit the heavy agent log payload and exclude // archived tasks unless explicitly requested. Full task detail still loads via // GET /api/tasks/:id. Without this, every dashboard load shipped tens of MB of agent logs. const listOptions = { limit, offset, slim: true, includeArchived, ...(column ? { column } : {}) }; tasks = await scopedStore.listTasks(listOptions); }🤖 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 `@packages/dashboard/src/routes/register-task-workflow-routes.ts` around lines 766 - 773, The search path omits the validated column filter so requests with q plus column (e.g., column=triage) can return wrong tasks; update the call to scopedStore.searchTasks(q, ...) to include the same options used for listing (limit, offset, slim, includeArchived and conditionally column) — mirror the listOptions construction (or reuse listOptions) and pass it into searchTasks so the validated column filter is applied in both paths.
🧹 Nitpick comments (1)
packages/dashboard/src/__tests__/routes-tasks.test.ts (1)
332-366: ⚡ Quick winAdd one regression test for
q + columntogetherPlease add a case for
/api/tasks?q=<term>&column=<valid-column>so the filter contract is covered on the search code path too.Suggested test
+ it("applies column filter when q and column are both provided", async () => { + (store.searchTasks as ReturnType<typeof vi.fn>).mockResolvedValueOnce([ + { ...FAKE_TASK_DETAIL, id: "FN-TRIAGE", column: "triage" as const }, + { ...FAKE_TASK_DETAIL, id: "FN-DONE", column: "done" as const }, + ]); + + const res = await GET(buildApp(), "/api/tasks?q=FN&column=triage"); + + expect(res.status).toBe(200); + expect(res.body).toEqual([ + expect.objectContaining({ id: "FN-TRIAGE", column: "triage" }), + ]); + });🤖 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 `@packages/dashboard/src/__tests__/routes-tasks.test.ts` around lines 332 - 366, Add a regression test that verifies the /api/tasks endpoint accepts both q and column together: in packages/dashboard/src/__tests__/routes-tasks.test.ts add an it(...) similar to the existing column-only test but call GET(buildApp(), `/api/tasks?q=TERM&column=${column}&limit=20`) (use the same task fixtures and vi mock for store.listTasks) and assert response status 200, returned rows match column and/or query, and that store.listTasks was called with the combined options including q, column, limit, offset: undefined, slim: true, and includeArchived: false; reuse GET, buildApp, FAKE_TASK_DETAIL and store.listTasks identifiers to locate code.
🤖 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.
Outside diff comments:
In `@packages/dashboard/src/routes/register-task-workflow-routes.ts`:
- Around line 766-773: The search path omits the validated column filter so
requests with q plus column (e.g., column=triage) can return wrong tasks; update
the call to scopedStore.searchTasks(q, ...) to include the same options used for
listing (limit, offset, slim, includeArchived and conditionally column) — mirror
the listOptions construction (or reuse listOptions) and pass it into searchTasks
so the validated column filter is applied in both paths.
---
Nitpick comments:
In `@packages/dashboard/src/__tests__/routes-tasks.test.ts`:
- Around line 332-366: Add a regression test that verifies the /api/tasks
endpoint accepts both q and column together: in
packages/dashboard/src/__tests__/routes-tasks.test.ts add an it(...) similar to
the existing column-only test but call GET(buildApp(),
`/api/tasks?q=TERM&column=${column}&limit=20`) (use the same task fixtures and
vi mock for store.listTasks) and assert response status 200, returned rows match
column and/or query, and that store.listTasks was called with the combined
options including q, column, limit, offset: undefined, slim: true, and
includeArchived: false; reuse GET, buildApp, FAKE_TASK_DETAIL and
store.listTasks identifiers to locate code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: af18b703-ab48-4031-869e-07a75a710648
📒 Files selected for processing (4)
.changeset/dashboard-column-filter.mddocs/task-management.mdpackages/dashboard/src/__tests__/routes-tasks.test.tspackages/dashboard/src/routes/register-task-workflow-routes.ts
plarson
force-pushed
the
fix/fn202-dashboard-column-filter
branch
from
906ba8f to
179a488
Compare
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
plarson
force-pushed
the
fix/fn202-dashboard-column-filter
branch
from
179a488 to
6ba3cbf
Compare
|
Thank you for your contributions! They are great and much appreciated |
Summary
Verification
Summary by CodeRabbit
New Features
Documentation
Tests
Chores