feat(stages): customizable pipeline stages with colours, reorder, and migration-on-delete#6
Conversation
Schema:
- stages.color text (nullable). NULL means "use the existing default
style" via the legacy name-keyed lookup in StagePill.
- CHECK (NOT (is_won AND is_lost)) constraint on stages so a single
stage can't be both flags simultaneously. Seed data is compliant
(Won has only is_won=true; Lost has only is_lost=true), so the
constraint adds cleanly.
Palette:
- lib/stage-colors.ts: 10-swatch curated palette (slate, blue, indigo,
violet, pink, rose, amber, lime, emerald, teal) with oklch tokens
matching the project's design tokens. STAGE_COLORS const + StageColor
type + STAGE_COLOR_TOKENS map + isStageColor type guard.
UI:
- StagePill now accepts an optional color prop. Resolution order:
(1) explicit color → STAGE_COLOR_TOKENS lookup
(2) name-keyed STAGE_CONFIG (existing behavior, unchanged)
(3) prospecting default
Tier 2 keeps seeded stages (Lead/Qualified/Discovery/.../Won/Lost)
visually identical without a backfill migration.
Co-authored-by: Claude <81847+claude@users.noreply.github.com>
Pure functions taking (db, orgId, ...) so server actions and MCP tools can be thin shells, and tests can hit the lib directly without mocking Better Auth. - assertStageInOrg(db, stageId, orgId) → Stage | null. Single source of truth for the org-scoping check; INNER JOIN through pipelines. - createStage → always inserts at end (max(order)+1). Banning a position arg keeps the swap-reorder mechanic safe by preventing tied orders. - updateStage → patch semantics; color: null clears, omitted leaves alone. - reorderStage(direction "up" | "down") → swap order values with the adjacent sibling inside a transaction. Boundary call returns unchanged before/after. - deleteStage(stageId, destinationStageId) → tx that bulk-migrates deals to the destination then deletes the stage. Refuses self-as-destination, cross-pipeline destination, and the only-stage-in-pipeline case. - previewStageFlagToggle → count of deals on the stage; cheap pre-flight for the UI confirm dialog. - StageOpError discriminates the five failure cases by `code` so server actions and MCP tools can map them to user-facing messages without string-matching. Co-authored-by: Claude <81847+claude@users.noreply.github.com>
…line
lib/audit.ts: ENTITY_TYPES gains "stage" and "pipeline" so audit writes
for these entities pass Zod validation in recordAudit.
app/(app)/settings/pipelines/actions.ts: thin server-action shells over
lib/stages. Each requires session, delegates to lib, audits the result,
and revalidates /settings/pipelines + /deals + /companies (the latter
is a layout-level revalidate so deal-stage views inside company tabs
also refresh).
- addStageAction: parse FormData, createStage(...), audit "create"
- updateStageAction: patch semantics; only audits if a real change
occurred (diffChangedFields)
- reorderStageAction: boundary no-op skips audit + revalidate
- deleteStageAction: writes one "update" audit row per migrated deal
+ one "delete" audit row on the stage. The migrated-deal audit
rows give per-deal traceability ("why did my deal jump?") at the
cost of N+1 audit rows for one user action — acceptable since
deletes are rare and bulk migrations even rarer.
- previewStageFlagToggleAction: cheap pre-flight returning the deal
count for the UI confirm dialog.
Errors from lib/stages bubble up as StageOpError and are mapped to
{ ok: false, code, message } discriminated-union returns so the UI
can surface them inline without string-matching.
Co-authored-by: Claude <81847+claude@users.noreply.github.com>
Four MCP tools wrapping lib/stages, registered in server.ts:
- create_stage(pipelineId, name, color?, isWon?, isLost?) — appends to
the pipeline; no position arg by design (avoids tied-order bug in
swap-reorder).
- update_stage(id, name?, color?, isWon?, isLost?) — patch semantics;
color is z.enum.nullable().optional() so callers can omit (leave as-is)
or pass null (clear).
- reorder_stage(id, direction "up"|"down") — boundary calls return
{ moved: false } and skip audit.
- delete_stage(id, destinationStageId) — bulk-migrates deals then deletes;
writes one audit row per migrated deal + one delete audit row on the
stage so each deal's timeline shows the move and the workspace activity
feed sees the stage removal.
StageOpError from lib/stages is mapped to a structured
{ error: code, message } in jsonResult so Claude sees the failure
reason without parsing free-form text.
Co-authored-by: Claude <81847+claude@users.noreply.github.com>
Settings page replaces the read-only stage list with an interactive editor. The page server-component fetches via getStages (now with color in the select) and renders <StagesEditor /> per pipeline. Components: - stages-editor.tsx: list of <StageRow /> + an inline "Add stage" form that toggles into focus, blurs to add. useTransition keeps the form responsive while the server action runs. - stage-row.tsx: per-stage row with up/down arrow reorder buttons (boundary disabled), a colour-swatch button that opens an inline popover of the 10 palette swatches + a clear option, click-to-edit inline rename (Enter saves, Escape cancels, blur saves), Won/Lost toggle buttons that fire previewStageFlagToggleAction first and surface a window.confirm() if any deals would be affected, and a hover-revealed delete button that opens the destination-picker dialog. - delete-stage-dialog.tsx: modal with deal count + sibling-stage dropdown for migration. Disables Delete when no destination exists. Toggling a flag explicitly sends the opposite flag as `false` when the opposite was previously true, to avoid the DB CHECK rejecting a transient both-true state during the patch. lib/data.ts: getStages select now includes the new `color` column so downstream <StagePill> consumers (companies overview, deals list, deals kanban) get the explicit colour for free. Co-authored-by: Claude <81847+claude@users.noreply.github.com>
…ssertions tests/stages.test.ts: 14 tests against lib/stages directly. Covers assertStageInOrg cross-org isolation, createStage append-at-end + duplicate-name + won/lost conflict, updateStage patch semantics (including color: null clears), DB CHECK on both-flags, reorderStage up/down/boundary, deleteStage migration + cross-pipeline + same-stage + last-stage refusals, previewStageFlagToggle counts, and updateStage returning null for cross-org. tests/mcp-tools.test.ts: two stage-tool integration tests: - create_stage / update_stage / reorder_stage round-trip with audit row + color round-trip + boundary moved=false - delete_stage migrates a deal and writes per-deal audit rows + a "delete" audit row on the stage Bug fix in lib/stages.ts: postgres-js wraps unique-constraint errors with the postgres `code: "23505"` on `err.cause`, not on `err`. isUniqueViolation now checks both levels so the duplicate-name path returns a StageOpError(name_in_use) instead of leaking the raw postgres error. Test-style note: bun:test's `expect(...).rejects.toBeInstanceOf(...)` hangs with custom Error subclasses; switched to manual try/catch + toBeInstanceOf to avoid the framework quirk. 76/76 tests pass. Lint, typecheck, build all clean. Co-authored-by: Claude <81847+claude@users.noreply.github.com>
P1 from local code review: StagePill accepts an optional color prop, but only the settings page passes it. Result: a user picks a colour in /settings/pipelines, the swatch updates there but every other StagePill (deals list, deals kanban column header, company-detail overview, company-detail deals tab, cockpit) keeps falling back to the legacy name-keyed style — colour selection has no visible effect where it matters. Add stages.color to the SELECT in five consumers and pass it to StagePill: - app/(app)/deals/page.tsx (server) — pipeline stage rows include color - app/(app)/deals/deals-view.tsx — KanbanStage type + per-deal stageColor - app/(app)/deals/kanban.tsx — column header now renders <StagePill> with color (was a plain text span) - app/(app)/companies/[id]/page.tsx — active-deals select - app/(app)/companies/[id]/deals/page.tsx — deals tab table - app/(app)/page.tsx — cockpit at-risk deals Co-authored-by: Claude <81847+claude@users.noreply.github.com>
|
Working on this PR… |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
enkii · code review
Summary
This PR adds interactive pipeline stage editing — create, rename, reorder, recolor, toggle won/lost, and delete with deal migration — plus MCP tooling and audit trail. The core business logic (lib/stages.ts) and DB schema are solid. However, three issues need attention before merge: (1) P1: the kanban board view never receives stage colors because DealsView drops the color field in its mapping to KanbanBoard, making the color feature invisible in board view; (2) P1: StageRow's name state is never synced with prop changes from revalidatePath, so re-entering edit mode after a successful rename and blurring silently reverts the rename to the old value; (3) P2: the StagePill fallback path has a broken case-sensitivity comparison that shows "Prospecting" for any custom-named stage without a color. Address P1s before merge; P2 can ship but is a one-line fix.
Mergeability Score: 3/5
Not merge-ready until the flagged correctness issues are addressed.
Unanchored notes (1)
GitHub could not resolve these findings to changed diff lines, so enkii is preserving them in the summary instead of failing the review.
components/pills.tsx:151— StagePill shows "Prospecting" for custom stages without a color
| </td> | ||
| <td className="px-3.5 py-2.5 align-middle"> | ||
| <StagePill value={d.stageName} /> | ||
| <StagePill value={d.stageName} color={d.stageColor} /> |
There was a problem hiding this comment.
color stripped in staging mapping
DealsView receives stages with color from the server component (added in this PR), but the mapping stages.map((s) => ({ id: s.id, name: s.name })) passed to KanbanBoard drops the color field. KanbanBoard now renders <StagePill value={stage.name} color={stage.color} /> where stage.color is always undefined, so the kanban headers fall through to the name-based STAGE_CONFIG lookup. Custom colors set in Settings → Pipelines are invisible in board view.
Fix: add color to the mapping: stages.map((s) => ({ id: s.id, name: s.name, color: s.color })).
| const [editing, setEditing] = useState(false); | ||
| const [name, setName] = useState(stage.name); | ||
| const [colorPickerOpen, setColorPickerOpen] = useState(false); | ||
| const [deleteOpen, setDeleteOpen] = useState(false); |
There was a problem hiding this comment.
useState(stage.name) captures the initial prop value and never updates when stage.name changes via revalidatePath. After a successful rename, the server re-renders with the new name, but the name state remains stale. When the user clicks to edit again, the input shows the old name. Blurring (or pressing Enter without changes) triggers commitName, which compares the stale name state against the current stage.name prop, finds them different, and sends the old name back to the server — silently undoing the rename.
Fix: add useEffect(() => { setName(stage.name); }, [stage.name]); to sync the local state when the prop updates.
There was a problem hiding this comment.
enkii · security review
Summary
Security review of PR #6 (pipeline stages CRUD): Reviewed all new stage CRUD operations in lib/stages.ts, UI server actions in app/(app)/settings/pipelines/actions.ts, MCP tools in lib/mcp/tools/stages.ts, color rendering in components/pills.tsx, and the DB migration adding nullable stages.color with a CHECK constraint. All operations are properly scoped to the authenticated user's organization via requireOrgSession() / MCP context and assertStageInOrg() (pipeline-join org check). Inputs are validated with zod (UUIDs, name length, color enum). DB queries use Drizzle ORM with parameterized bindings — no SQL injection. No IDOR, no XSS, no privilege escalation, no information disclosure. Audit trail is complete for all mutations. No security findings. Safe to merge.
Mergeability Score: 5/5
Safe to merge from this review's perspective.
- P1: KanbanBoard never received stage colours because DealsView's stages mapping dropped the color field. Include it in the map so the kanban column header recolours. - P1: StageRow's local `name` state was initialized once from the prop and never re-synced. After a stage is renamed externally (revalidatePath, another tab, MCP, etc.) the input still held the old value; blurring would silently overwrite the new name with the old one. Fix: introduce startEdit() that explicitly resyncs from the prop every time edit mode opens. - P2: StagePill's name-keyed fallback was comparing cfg.label === key (string vs underscore-lowercased key — never equal). Every custom- named stage without an explicit color rendered as "Prospecting". Fix: check whether the lookup actually matched and render the matched label when it did, otherwise render the raw value. 76/76 tests pass; lint, typecheck clean. Co-authored-by: Claude <81847+claude@users.noreply.github.com>
# Conflicts: # db/migrations/meta/0004_snapshot.json # db/migrations/meta/_journal.json
Main landed 0004_add-org-invite-link and 0005_add-member-user-org-unique while this branch had 0004_stage-color-and-flag-check. Drizzle keys migrations by index; the slot collision had to be resolved by regenerating the stage migration as 0006 with identical SQL (ALTER TABLE add color column + add CHECK constraint). 90/90 tests pass against a freshly reset DB (16 new tests came in with main's invite/member work, on top of the stage tests already in this branch). Co-authored-by: Claude <81847+claude@users.noreply.github.com>
|
Working on this PR… |
There was a problem hiding this comment.
enkii · code review
Summary
This PR adds an interactive pipeline stage editor with customizable colors, reordering, won/lost toggles, and migration-on-delete — plus MCP tool wrappers for the same operations. Coverage includes schema changes (nullable color column + CHECK constraint), business logic in lib/stages.ts, UI server actions with audit, <StagePill> color propagation across the app, and 16 new tests.
Both P1 findings from a prior review pass (kanban color stripping, StageRow name state sync) are already fixed in the head SHA. One P2 finding remains: the "deleteStage refuses the only stage" test hits the self-as-destination guard instead of the last_stage check, leaving that error path untested. The production code is correct — this is a test coverage gap only.
Safe to merge after addressing the test gap.
Mergeability Score: 4/5
Likely mergeable after reviewing the flagged low-risk comments.
| expect(caught).toBeInstanceOf(StageOpError); | ||
| }); | ||
|
|
||
| test("previewStageFlagToggle counts deals on the stage", async () => { |
There was a problem hiding this comment.
last_stage error code — self-as-destination guard fires first
The test named "deleteStage refuses the only stage in a pipeline" creates a 2-stage pipeline "Lonely", deletes onlyStage (leaving otherStageId as the sole remaining stage), then calls deleteStage(db, orgId, otherStageId, otherStageId). This hits the stageId === destinationStageId early guard (error code "wrong_pipeline") and never reaches the siblingCount <= 1 check that throws "last_stage". The last_stage error path has no test coverage.
Fix: construct a separate destination stage ID in the same pipeline so the stageId === destinationStageId check passes, allowing the last_stage check to be reached. For example, seed a third stage in the pipeline, delete one of the first two, then attempt to delete another with the third as destination until only one remains.
There was a problem hiding this comment.
enkii · security review
Summary
Security surface reviewed: Full pipeline stages CRUD feature — server actions (app/(app)/settings/pipelines/actions.ts), business logic (lib/stages.ts), MCP tools (lib/mcp/tools/stages.ts), schema migration (color column + CHECK constraint), UI components (stage-row, stages-editor, delete-stage-dialog, StagePill), and audit logging.
Findings: No security vulnerabilities identified. All mutation paths are authenticated via requireOrgSession(). Every lib/stages.ts operation verifies organization ownership via assertStageInOrg / assertPipelineInOrg (join through pipelines to check organizationId), preventing cross-org IDOR. Input validation uses Zod schemas (UUID, string length, enum validation for colors). SQL queries use Drizzle ORM with parameterized statements — no injection surfaces. A DB CHECK constraint (stage_not_both_won_lost) provides defense-in-depth against won/lost mutual exclusion. All mutations write audit log rows attributed to the actor. No secrets, no dangerous rendering patterns (no dangerouslySetInnerHTML, no eval), no unauthenticated read paths.
Safe to merge. No security blockers.
Mergeability Score: 5/5
Safe to merge from this review's perspective.
1 participant
Summary
Settings → Pipelines goes from a read-only list with a "lands in Phase 4" footer to a fully interactive stage editor. Users can add, rename, reorder (up/down), recolour, toggle won/lost, and delete stages on their workspace pipeline. Deletes that have deals attached require picking a destination stage; deals are bulk-migrated atomically. The same operations are available via MCP so Claude can help configure pipelines on request. Every change writes an audit row attributed to the actor.
What's Included
stages.color textcolumn +stage_not_both_won_lostCHECK constraint.lib/stage-colors.ts— 10 curated swatches with oklch tokens.<StagePill>resolution: explicit color → existing name-keyed lookup → default. Tier 2 keeps seeded stages visually identical without a backfill.lib/stages.ts— pure functions taking(db, orgId, ...)covering create / update / reorder / delete / preview, plusassertStageInOrghelper that centralizes the pipeline-join org check.app/(app)/settings/pipelines/actions.ts— thin shells overlib/stageswith audit + revalidate.StageOpErrorcodes mapped to{ ok, code, message }so the UI surfaces inline errors without string-matching.lib/mcp/tools/stages.ts—create_stage,update_stage,reorder_stage,delete_stage.update_stagepatch semantics:color: nullclears, omitted leaves alone.stages-editor,stage-row,delete-stage-dialogclient components. Inline rename (Enter saves, Esc cancels, blur saves), colour-swatch popover with 10 colours + clear, up/down reorder buttons (boundary disabled), Won/Lost toggles withwindow.confirmif any deals would be reclassified, hover-revealed delete that opens the destination-picker dialog with the current deal count.<StagePill>consumers (deals list, deals kanban header, cockpit at-risk deals, company overview, company deals tab) all now selectstages.colorand pass it through, so colour selection is visible across the app — not just on the settings page.ENTITY_TYPESextends with"stage"and"pipeline". Every stage write records an audit row. Deletes record one row per migrated deal (per-deal traceability) plus one delete row on the stage.tests/stages.test.ts(14 tests) againstlib/stagesdirectly + 2 MCP wrapper integration tests intests/mcp-tools.test.tscovering audit row presence and shape.Plan:
artefacts/plans/pipeline-stages.md(gitignored).Tests
bun run lint: passedbun run typecheck: passedbun run build: passedbun testagainst a fresh test DB on port 5436: 76 pass / 0 fail / 8 filesTest-style note:
expect(...).rejects.toBeInstanceOf(...)hangs in bun:test with custom Error subclasses (framework quirk). Switched the affected stage tests to manual try/catch +toBeInstanceOfto avoid it.Test Plan
/settings/pipelines. Confirm the workspace's pipeline shows interactive controls.order.window.confirmshows the count → confirm → flag flips. Toggling a stage to BOTH won AND lost via two clicks → DB CHECK blocks the second; UI shows the error.create_stage,update_stage(with color),reorder_stage,delete_stagefrom a Claude session. Confirmlist_pipelinesreflects the changes including thecolorfield, and audit rows are written with the right actor.Local Review Findings
a4dc681—<StagePill>consumers were not passingcolor, so colour selection was invisible everywhere except the settings page. Updated five consumer queries + StagePill calls to thread the colour through.SELECT FOR UPDATEwould prevent it; deferred since realistic risk on a settings page is near-zero.🤖 Generated with Claude Code