-
Notifications
You must be signed in to change notification settings - Fork 2
Show tags on the board and filter by them #1499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| # Task tags on the board | ||
|
|
||
| What shipped when tags stopped being write-only, what deviated, and what was | ||
| deliberately left out. The user-facing description lives in the Operations | ||
| Center guide ("Finding things"); this note is for whoever changes the code. | ||
|
|
||
| ## The gap | ||
|
|
||
| Tags (#212) were storable and queryable but invisible. The create form accepted | ||
| them, `models.Task` carried them, `TaskFilter.Tags` filtered on them, | ||
| `GET /tasks?tag=a&tag=b` narrowed to tasks carrying **both**, and | ||
| `GET /tasks/tags` returned the whole catalogue with per-tag counts — and no | ||
| surface in the web app ever rendered a tag again. So the one thing a tag is | ||
| for, finding the rest of its group, could not be done from the UI at all. | ||
|
|
||
| The gap was found while writing the user guide, which is worth recording: the | ||
| guide had to describe tags as "stored metadata rather than a control on that | ||
| screen", and a sentence that awkward is usually a defect wearing prose. | ||
|
|
||
| ## What shipped | ||
|
|
||
| - **Chips.** A task's tags render on its table row and its phone card, coloured | ||
| from the same hashed palette (`shared/lib/labelColors`) as the chat | ||
| conversation labels, so one tag reads the same everywhere it appears. | ||
| - **Every chip is a control.** Clicking one adds that tag to the board's | ||
| filter; clicking a selected one removes it. Tags AND server-side, so each | ||
| addition narrows and each removal widens. | ||
| - **A Tags group in the filter bar** — a select that *adds* a tag, plus a | ||
| removable chip per selected tag. The select never holds a value: the board is | ||
| filtered by every chip beside it, not by the last one chosen, and a select | ||
| reading `ops` while `ops + urgent` were applied would misstate the board. | ||
| - **Tags count as an active filter**, so **Clear filters** appears and clears | ||
| them. Without that the only way back to the full board was a page reload. | ||
| - **`/api/orchestrator/tasks/tags`**, a thin proxy to the existing catalogue | ||
| endpoint. The static `tags` segment wins over the sibling `[taskId]` route, | ||
| so it does not shadow `GET /tasks/{id}` — the same ordering `cmd/fleet/main.go` | ||
| spells out explicitly for the Go router. | ||
|
|
||
| Two things underneath had to change: | ||
|
|
||
| - **`passThroughQuery` forwards every value of a repeated parameter.** It read | ||
| only the first, which is right for every single-valued filter and wrong for | ||
| `tag`: dropping the second of `?tag=a&tag=b` *widens* the result instead of | ||
| narrowing it — the one direction a filter must never fail in. Single-valued | ||
| parameters behave exactly as before. | ||
| - **The phone card's box moved from its `<button>` to the enclosing `<li>`.** | ||
| The chips cannot live inside the card button (see below), so they render as | ||
| its sibling; moving the border, radius and background one level out is what | ||
| keeps them inside the visible card. | ||
|
|
||
| ## Two decisions worth keeping | ||
|
|
||
| **A chip is a `<button>`, and on the phone card it is NOT inside the card | ||
| button.** The card is itself a `<button>`, and a control nested inside a button | ||
| has invalid accessibility semantics however it is marked up — assistive | ||
| technology can expose only the outer "View task" control, or make the tag | ||
| action ambiguous. The first version dressed the chip as a `<span | ||
| role="button">`, which dodges the HTML parsing rule and keeps the actual | ||
| problem. Siblings, not children. `TasksTable.test.tsx` pins this. | ||
|
|
||
| (The table row is also `role="button"` and already nests real buttons — run | ||
| now, delete. That predates this change and was left alone rather than widening | ||
| the PR; it is a reasonable thing to revisit.) | ||
|
|
||
| **The catalogue has a TTL, not a fetch-once and not a fetch-every-reload.** | ||
| `GET /tasks/tags` is a `GROUP BY` over every task's tag array. Fetching it with | ||
| the dashboard's 30s refresh would pay for that constantly to catch a list that | ||
| changes only when somebody retags something; fetching it once per activation | ||
| left a tag created later, on a task not on the current page, unreachable until | ||
| a full reload, because `active` stays true for the whole signed-in session. | ||
| `TAG_CATALOGUE_TTL_MS` (5 minutes) bounds both. The gap it leaves is closed | ||
| from the other side: `tagOptions` is the catalogue **unioned with the tags on | ||
| the listed tasks**, so a brand-new tag is selectable the moment a task carrying | ||
| it appears, without waiting for the refresh. | ||
|
|
||
| ## Honest scope | ||
|
|
||
| - **The catalogue is deployment-wide; the board is not.** A non-admin sees only | ||
| their own tasks, so a tag a colleague uses can appear in the dropdown and | ||
| filter down to nothing. This is the same pre-existing property as the | ||
| dashboard counters, it is not introduced here, and the guide states it rather | ||
| than hiding it. | ||
| - **No tag counts in the UI.** The catalogue returns them, but "ops (12)" above | ||
| a board showing two of them is a number that is wrong for most readers, for | ||
| the reason above. Names only. | ||
| - **Not shipped:** tag management from the board (renaming or deleting a tag | ||
| across tasks — retagging is per-task, through the form or | ||
| `POST /tasks/{id}/tags`), and no tag filter on the Upcoming or Sleeping | ||
| panels. |
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { NextRequest } from "next/server"; | ||
|
|
||
| import { passThroughQuery } from "./proxy"; | ||
|
|
||
| const ORIGIN = "https://chat.example.com"; | ||
|
|
||
| function request(query: string): NextRequest { | ||
| return new NextRequest(`${ORIGIN}/api/orchestrator/tasks?${query}`); | ||
| } | ||
|
|
||
| // passThroughQuery is the allow-list between the browser and the orchestrator. | ||
| // It read only the FIRST value of each param, which is correct for every | ||
| // single-valued filter and wrong for `tag`: ?tag=a&tag=b means "carrying BOTH" | ||
| // (the server ANDs them), so dropping b WIDENED the result instead of | ||
| // narrowing it — the one direction a filter must never fail in. | ||
| describe("passThroughQuery", () => { | ||
| it("forwards every value of a repeated param", () => { | ||
| const qs = passThroughQuery(request("tag=ops&tag=urgent"), ["tag"]); | ||
| expect(new URLSearchParams(qs.slice(1)).getAll("tag")).toEqual(["ops", "urgent"]); | ||
| }); | ||
|
|
||
| it("passes a single-valued param through unchanged", () => { | ||
| expect(passThroughQuery(request("status=running"), ["status"])).toBe("?status=running"); | ||
| }); | ||
|
|
||
| it("drops params outside the allow-list", () => { | ||
| expect(passThroughQuery(request("status=running&secret=x"), ["status"])).toBe( | ||
| "?status=running", | ||
| ); | ||
| expect(passThroughQuery(request("tag=ops"), ["status"])).toBe(""); | ||
| }); | ||
|
|
||
| it("drops empty values rather than forwarding a blank filter", () => { | ||
| expect(passThroughQuery(request("status=&q=hello"), ["status", "q"])).toBe("?q=hello"); | ||
| // An empty value among repeated ones drops only itself. | ||
| const qs = passThroughQuery(request("tag=ops&tag=&tag=urgent"), ["tag"]); | ||
| expect(new URLSearchParams(qs.slice(1)).getAll("tag")).toEqual(["ops", "urgent"]); | ||
| }); | ||
|
|
||
| it("returns an empty string, not a bare '?', when nothing passes", () => { | ||
| expect(passThroughQuery(request("nope=1"), ["status"])).toBe(""); | ||
| }); | ||
|
|
||
| it("emits params in allow-list order, not the caller's", () => { | ||
| expect(passThroughQuery(request("q=hi&status=running"), ["status", "q"])).toBe( | ||
| "?status=running&q=hi", | ||
| ); | ||
| }); | ||
|
|
||
| it("keeps a comma-separated value intact for completed_status", () => { | ||
| // The Failed Today card sends two statuses in one value; splitting or | ||
| // truncating it here would silently halve the filter. | ||
| const qs = passThroughQuery( | ||
| request("completed_status=error%2Cdead_lettered"), | ||
| ["completed_status"], | ||
| ); | ||
| expect(new URLSearchParams(qs.slice(1)).get("completed_status")).toBe("error,dead_lettered"); | ||
| }); | ||
| }); |
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| import { NextRequest } from "next/server"; | ||
| import { proxyToOrchestrator } from "../../_lib/proxy"; | ||
|
|
||
| export const runtime = "nodejs"; | ||
|
|
||
| // GET /api/orchestrator/tasks/tags → orchestrator GET /tasks/tags (#212): the | ||
| // distinct tags in use, busiest first. Feeds the board's tag filter, which | ||
| // needs the tags that exist rather than only those on the page in front of you. | ||
| // | ||
| // The static `tags` segment wins over the sibling `[taskId]` route, so this | ||
| // does not shadow GET /tasks/{id} — the same ordering the Go router spells out | ||
| // explicitly in cmd/fleet/main.go. | ||
| export async function GET(request: NextRequest) { | ||
| return proxyToOrchestrator(request, "/tasks/tags"); | ||
| } |
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.