mirror of
https://github.com/multica-ai/multica.git
synced 2026-07-26 04:25:46 +02:00
fix(properties): address MUL-4463 review round 2 — desc sort, option bucketing, archived-state reconciliation
- sort: direction now applies to value comparison only; issues without a value sort last in BOTH directions (the whole-array reverse flipped them to the front on desc). Test covers the desc+missing case. - board: values referencing an option removed from the definition bucket into the No-value column instead of vanishing (unmatched column ids dropped the issue entirely). Defense-in-depth behind the new server-side in-use guard; drag-utils test locks both behaviors. - controller: persisted propertyFilters keyed by archived/deleted definitions are stripped before reaching the filter predicates, and a persisted property sort on a non-active definition degrades to manual order — previously both kept silently applying while the header claimed otherwise. The filter badge counts only active-definition filters. Co-authored-by: multica-agent <github@multica.ai>
This commit is contained in:
@@ -193,6 +193,13 @@ export function BoardView({
|
||||
// deleted, other workspace) falls back to status columns.
|
||||
const grouping: IssueGrouping =
|
||||
groupingPropertyId && !groupingProperty ? "status" : storeGrouping;
|
||||
const groupingOptionIds = useMemo(
|
||||
() =>
|
||||
groupingProperty
|
||||
? new Set((groupingProperty.config.options ?? []).map((option) => option.id))
|
||||
: undefined,
|
||||
[groupingProperty],
|
||||
);
|
||||
const setIssuePropertyMutation = useSetIssueProperty();
|
||||
const unsetIssuePropertyMutation = useUnsetIssueProperty();
|
||||
const applyPropertyGroupValue = useCallback(
|
||||
@@ -303,13 +310,13 @@ export function BoardView({
|
||||
recentlyMovedRef,
|
||||
settleVersion,
|
||||
beginSettle,
|
||||
} = useDragSettle(() => buildColumns(groupedIssues, groups, grouping));
|
||||
} = useDragSettle(() => buildColumns(groupedIssues, groups, grouping, groupingOptionIds));
|
||||
|
||||
useEffect(() => {
|
||||
if (!isDraggingRef.current && !isSettlingRef.current) {
|
||||
setColumns(buildColumns(groupedIssues, groups, grouping));
|
||||
setColumns(buildColumns(groupedIssues, groups, grouping, groupingOptionIds));
|
||||
}
|
||||
}, [groupedIssues, groups, grouping, settleVersion, setColumns, isDraggingRef, isSettlingRef]);
|
||||
}, [groupedIssues, groups, grouping, groupingOptionIds, settleVersion, setColumns, isDraggingRef, isSettlingRef]);
|
||||
|
||||
// --- Issue map ---
|
||||
// Frozen during drag so BoardColumn/DraggableBoardCard props stay
|
||||
@@ -374,7 +381,7 @@ export function BoardView({
|
||||
setActiveIssue(null);
|
||||
|
||||
const resetColumns = () =>
|
||||
setColumns(buildColumns(groupedIssues, groups, grouping));
|
||||
setColumns(buildColumns(groupedIssues, groups, grouping, groupingOptionIds));
|
||||
|
||||
if (!over) {
|
||||
resetColumns();
|
||||
@@ -469,7 +476,7 @@ export function BoardView({
|
||||
onMoveIssue(activeId, getMoveUpdates(finalGroup, newPosition), beginSettle());
|
||||
applyPropertyGroupValue(finalGroup, activeId);
|
||||
},
|
||||
[groupedIssues, groups, grouping, onMoveIssue, groupIds, groupMap, sortBy, beginSettle, columnsRef, isDraggingRef, setColumns, applyPropertyGroupValue],
|
||||
[groupedIssues, groups, grouping, groupingOptionIds, onMoveIssue, groupIds, groupMap, sortBy, beginSettle, columnsRef, isDraggingRef, setColumns, applyPropertyGroupValue],
|
||||
);
|
||||
|
||||
return (
|
||||
|
||||
@@ -923,10 +923,20 @@ export function IssueDisplayControls({
|
||||
const counts = useIssueCounts(scopedIssues);
|
||||
const showDateFilter = !!onDateFilterChange;
|
||||
|
||||
// Only count filters whose definition is still active — a filter pinned to
|
||||
// an archived definition is stripped by the surface controller and must
|
||||
// not light the badge for an effect that no longer exists.
|
||||
const effectivePropertyFilters = useMemo(() => {
|
||||
const activeIds = new Set(filterableProperties.map((p) => p.id));
|
||||
return Object.fromEntries(
|
||||
Object.entries(propertyFilters).filter(([id, selected]) => selected.length > 0 && activeIds.has(id)),
|
||||
);
|
||||
}, [filterableProperties, propertyFilters]);
|
||||
|
||||
const activeFilterCount = getActiveFilterCount({
|
||||
statusFilters,
|
||||
priorityFilters,
|
||||
propertyFilters,
|
||||
propertyFilters: effectivePropertyFilters,
|
||||
assigneeFilters,
|
||||
includeNoAssignee,
|
||||
creatorFilters,
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
"use client";
|
||||
|
||||
import { useEffect, useMemo } from "react";
|
||||
import { useQuery } from "@tanstack/react-query";
|
||||
import type { QueryKey } from "@tanstack/react-query";
|
||||
import type {
|
||||
Issue,
|
||||
@@ -22,6 +23,7 @@ import {
|
||||
import type { IssueScope } from "@multica/core/issues/surface/scope";
|
||||
import type { IssueDateFilter, SortField } from "@multica/core/issues/stores/view-store";
|
||||
import { sortIssues } from "../utils/sort";
|
||||
import { propertyListOptions } from "@multica/core/properties";
|
||||
import { propertyIdFromViewKey } from "@multica/core/issues/stores/view-store";
|
||||
import { useViewStore } from "@multica/core/issues/stores/view-store-context";
|
||||
import type { IssueFilters } from "../utils/filter";
|
||||
@@ -158,10 +160,29 @@ export function useIssueSurfaceController({
|
||||
() => issueDateFilterToApiParams(dateFilter),
|
||||
[dateFilter],
|
||||
);
|
||||
// Active property catalog. Persisted view state can outlive definitions
|
||||
// (archive/delete): filters keyed by a non-active definition are stripped
|
||||
// before they reach the predicates, and a sort on a non-active definition
|
||||
// degrades to manual order — matching what the header already shows.
|
||||
const { data: workspaceProperties = [] } = useQuery(propertyListOptions(wsId));
|
||||
const activePropertyIds = useMemo(
|
||||
() => new Set(workspaceProperties.map((p) => p.id)),
|
||||
[workspaceProperties],
|
||||
);
|
||||
const effectivePropertyFilters = useMemo(() => {
|
||||
const entries = Object.entries(propertyFilters).filter(
|
||||
([propertyId, selected]) => selected.length > 0 && activePropertyIds.has(propertyId),
|
||||
);
|
||||
if (entries.length === Object.keys(propertyFilters).length) return propertyFilters;
|
||||
return Object.fromEntries(entries);
|
||||
}, [activePropertyIds, propertyFilters]);
|
||||
|
||||
// Custom-property sorts (`property:<id>`) are client-side only — the
|
||||
// server sort enum is fixed, so the query falls back to position order and
|
||||
// the surface lists re-sort below via sortIssues.
|
||||
const propertySortId = propertyIdFromViewKey(sortBy);
|
||||
const rawPropertySortId = propertyIdFromViewKey(sortBy);
|
||||
const propertySortId =
|
||||
rawPropertySortId && activePropertyIds.has(rawPropertySortId) ? rawPropertySortId : null;
|
||||
const sort = useMemo<IssueSortParam>(
|
||||
() => ({
|
||||
sort_by: propertySortId
|
||||
@@ -208,7 +229,7 @@ export function useIssueSurfaceController({
|
||||
projectFilters: viewProjectFilters,
|
||||
includeNoProject: viewIncludeNoProject,
|
||||
labelFilters,
|
||||
propertyFilters,
|
||||
propertyFilters: effectivePropertyFilters,
|
||||
agentRunningFilter,
|
||||
showSubIssues,
|
||||
loadProjects:
|
||||
@@ -225,19 +246,19 @@ export function useIssueSurfaceController({
|
||||
const propertySortedIssues = useMemo(
|
||||
() =>
|
||||
propertySortId
|
||||
? sortIssues(data.issues, sortBy, sortDirection)
|
||||
? sortIssues(data.issues, `property:${propertySortId}`, sortDirection)
|
||||
: data.issues,
|
||||
[data.issues, propertySortId, sortBy, sortDirection],
|
||||
[data.issues, propertySortId, sortDirection],
|
||||
);
|
||||
const propertySortedAssigneeGroups = useMemo(
|
||||
() =>
|
||||
propertySortId && data.assigneeGroups
|
||||
? data.assigneeGroups.map((group) => ({
|
||||
...group,
|
||||
issues: sortIssues(group.issues, sortBy, sortDirection),
|
||||
issues: sortIssues(group.issues, `property:${propertySortId}`, sortDirection),
|
||||
}))
|
||||
: data.assigneeGroups,
|
||||
[data.assigneeGroups, propertySortId, sortBy, sortDirection],
|
||||
[data.assigneeGroups, propertySortId, sortDirection],
|
||||
);
|
||||
|
||||
return {
|
||||
|
||||
@@ -94,6 +94,18 @@ describe("property grouping", () => {
|
||||
expect(issueMatchesGroup(withoutValue, noneColumn)).toBe(true);
|
||||
});
|
||||
|
||||
it("unknown option values bucket into the none column when the catalog is known", () => {
|
||||
const stale = { id: "C", properties: { [propertyId]: "opt-deleted" } } as unknown as Issue;
|
||||
const known = new Set(["opt-staging"]);
|
||||
expect(getIssueGroupId(stale, `property:${propertyId}`, known)).toBe(
|
||||
propertyGroupId(propertyId, null),
|
||||
);
|
||||
// Without the catalog, the raw bucket is preserved (caller may still map it).
|
||||
expect(getIssueGroupId(stale, `property:${propertyId}`)).toBe(
|
||||
propertyGroupId(propertyId, "opt-deleted"),
|
||||
);
|
||||
});
|
||||
|
||||
it("getMoveUpdates for property columns only carries position", () => {
|
||||
expect(getMoveUpdates({ id: "c1", title: "Staging", propertyId, propertyOptionId: "opt-staging" }, 5)).toEqual({ position: 5 });
|
||||
});
|
||||
|
||||
@@ -42,12 +42,24 @@ export function assigneeGroupId(
|
||||
return type && id ? `assignee:${type}:${id}` : UNASSIGNED_GROUP_ID;
|
||||
}
|
||||
|
||||
export function getIssueGroupId(issue: Issue, grouping: IssueGrouping): string {
|
||||
export function getIssueGroupId(
|
||||
issue: Issue,
|
||||
grouping: IssueGrouping,
|
||||
knownOptionIds?: ReadonlySet<string>,
|
||||
): string {
|
||||
if (grouping === "status") return statusGroupId(issue.status);
|
||||
const propertyId = propertyIdFromViewKey(grouping);
|
||||
if (propertyId) {
|
||||
const value = issue.properties?.[propertyId];
|
||||
return propertyGroupId(propertyId, typeof value === "string" ? value : null);
|
||||
let optionId = typeof value === "string" ? value : null;
|
||||
// A value referencing an option no longer in the definition (removed
|
||||
// before the in-use guard existed, or by a newer server) must bucket
|
||||
// into the No-value column — an unmatched column id would silently drop
|
||||
// the issue from the board.
|
||||
if (optionId !== null && knownOptionIds && !knownOptionIds.has(optionId)) {
|
||||
optionId = null;
|
||||
}
|
||||
return propertyGroupId(propertyId, optionId);
|
||||
}
|
||||
return assigneeGroupId(issue.assignee_type, issue.assignee_id);
|
||||
}
|
||||
@@ -56,11 +68,12 @@ export function buildColumns(
|
||||
issues: Issue[],
|
||||
groups: BoardColumnGroup[],
|
||||
grouping: IssueGrouping,
|
||||
knownOptionIds?: ReadonlySet<string>,
|
||||
): Record<string, string[]> {
|
||||
const cols: Record<string, string[]> = {};
|
||||
for (const group of groups) cols[group.id] = [];
|
||||
for (const issue of issues) {
|
||||
const gid = getIssueGroupId(issue, grouping);
|
||||
const gid = getIssueGroupId(issue, grouping, knownOptionIds);
|
||||
if (cols[gid]) cols[gid].push(issue.id);
|
||||
}
|
||||
return cols;
|
||||
|
||||
@@ -22,13 +22,13 @@ describe("sortIssues property sorts", () => {
|
||||
expect(sorted.map((i) => i.id)).toEqual(["small", "big", "none"]);
|
||||
});
|
||||
|
||||
it("desc reverses values but keeps semantics", () => {
|
||||
it("desc reverses values but keeps missing values last", () => {
|
||||
const sorted = sortIssues(
|
||||
[issueWith("small", 2), issueWith("big", 10)],
|
||||
[issueWith("none"), issueWith("small", 2), issueWith("big", 10)],
|
||||
`property:${propertyId}`,
|
||||
"desc",
|
||||
);
|
||||
expect(sorted.map((i) => i.id)).toEqual(["big", "small"]);
|
||||
expect(sorted.map((i) => i.id)).toEqual(["big", "small", "none"]);
|
||||
});
|
||||
|
||||
it("sorts date-only strings chronologically via lexical compare", () => {
|
||||
|
||||
@@ -14,12 +14,23 @@ export function sortIssues(
|
||||
): Issue[] {
|
||||
// `property:<id>` sorts by the custom-property value. Number values sort
|
||||
// numerically; date values are date-only "YYYY-MM-DD" strings, which sort
|
||||
// correctly lexically. Issues without a value always sort last regardless
|
||||
// of direction (matching start_date/due_date semantics).
|
||||
// correctly lexically. Direction applies to the VALUE comparison only —
|
||||
// issues without a value sort last in both directions (a whole-array
|
||||
// reverse would flip them to the front on desc).
|
||||
const propertyId = propertyIdFromViewKey(field);
|
||||
if (propertyId) {
|
||||
const sorted = issues.toSorted((a, b) => comparePropertyValues(a, b, propertyId));
|
||||
return direction === "desc" ? sorted.reverse() : sorted;
|
||||
const dir = direction === "desc" ? -1 : 1;
|
||||
return issues.toSorted((a, b) => {
|
||||
const av = a.properties?.[propertyId];
|
||||
const bv = b.properties?.[propertyId];
|
||||
const aMissing = av === undefined || Array.isArray(av);
|
||||
const bMissing = bv === undefined || Array.isArray(bv);
|
||||
if (aMissing && bMissing) return 0;
|
||||
if (aMissing) return 1;
|
||||
if (bMissing) return -1;
|
||||
if (typeof av === "number" && typeof bv === "number") return dir * (av - bv);
|
||||
return dir * String(av).localeCompare(String(bv));
|
||||
});
|
||||
}
|
||||
|
||||
const sorted = issues.toSorted((a, b) => {
|
||||
@@ -58,15 +69,3 @@ export function sortIssues(
|
||||
});
|
||||
return direction === "desc" ? sorted.reverse() : sorted;
|
||||
}
|
||||
|
||||
function comparePropertyValues(a: Issue, b: Issue, propertyId: string): number {
|
||||
const av = a.properties?.[propertyId];
|
||||
const bv = b.properties?.[propertyId];
|
||||
const aMissing = av === undefined || Array.isArray(av);
|
||||
const bMissing = bv === undefined || Array.isArray(bv);
|
||||
if (aMissing && bMissing) return 0;
|
||||
if (aMissing) return 1;
|
||||
if (bMissing) return -1;
|
||||
if (typeof av === "number" && typeof bv === "number") return av - bv;
|
||||
return String(av).localeCompare(String(bv));
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user