You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "Joob1n (via GitHub)" <gi...@apache.org> on 2026/08/31 13:38:02 UTC
[PR] feat(desktop): select several tasks in the Session rail [maka]
Joob1n opened a new pull request, #4365:
URL: https://github.com/apache/maka/pull/4365
## Summary
The Session rail could act on one task at a time. Clearing out a run of finished tasks meant opening the ⋯ menu, confirming, and repeating — once per task.
⌘/Ctrl-click marks a row instead of opening it, Shift-click marks a run, and a bar in the rail's sticky chrome offers **archive** and **delete** over the marked set. Escape clears the selection. Delete asks for the marked set rather than the focused row: deleting one of several rows a user marked is the shape of an unrecoverable surprise. Nothing renders until something is marked, so the rail at rest is unchanged, and a surface that never wires the selection up keeps plain clicks and gains no chrome.
Three decisions are worth stating, because each is a constraint rather than a preference.
**A range never leaves its group.** A project group's collapsed state lives inside Astryx's `SideNavItem` (`collapsible={{ defaultIsCollapsed: false }}`, uncontrolled) and is not readable from the rail, so a range spanning groups could quietly include rows nobody can see. Within one group the question does not arise: both endpoints had to be clicked, and a row that can be clicked is a row that is on screen.
**The selection is a third rail context**, for the reason the chrome is a second one. It changes on every modified click while the list does not, and folding it into `SessionRailData` would give that value a new identity per click — the ~1,000-fiber render the split exists to prevent (#4109). Rows read it directly rather than receiving it through the memoized row renderer, so a selection change re-renders the rows on screen and nothing above them.
**Both sweeps reuse the purge machinery instead of copying its accounting.** `purgeSessions` becomes `sweepSessions`, parameterized by how each id decides the archived premise: Settings asserts it for every target; the rail reads it off the task exactly as single-row delete does, because the rail lists unarchived tasks and asserting it there would refuse every deletion the rail can ask for. Bulk archive accounts by count and first failure instead of reusing `SessionPurgeOutcome`, which would carry a `restored` field that can never be anything but empty. Neither raises a toast per task: one sweep is one message.
### On the existing bulk pattern
The project's other multi-item surface, Settings › archived tasks, is filter-then-sweep: narrow with the search box, then purge everything visible. That is deliberate and its reasoning is sound ("Frozen at the click"), but it does not transfer here. The rail has no search box, its rows are grouped rather than listed, and the set a user wants to clear out is usually a handful of scattered rows rather than a prefix of a filtered list. Marking is the only way to name that set. The two patterns now coexist and share one sweep underneath.
## Behavior change
An unmodified click on a rail row still opens the task; nothing about navigation changes. With rows marked, `Delete`/`Backspace` in the rail deletes the marked set (behind a confirm) instead of the focused row — the only change to an existing keyboard behaviour, and only while a selection exists.
## Verification
```
packages/ui 285 tests, 285 pass, 0 fail (9 new)
apps/desktop 1752 tests, 1752 pass, 0 fail (17 new)
typecheck (desktop × 4 tsconfigs, ui) clean
biome check (packages/ui/src, apps/desktop/src) clean
npm run smoke:storybook 162 stories, passed — the AX audit included
```
The UI cases drive real clicks through `react-dom/client` + `act` rather than asserting markup: the branch under test is chosen inside the row's click handler, and markup cannot say which branch ran.
Each change fails a test when reverted individually:
| reverted | result |
|---|---|
| the modifier branch in the row handler | ui 279 pass / 3 fail |
| `data-selected` on the row | ui 281 pass / 1 fail |
| the selection model's toggle/range/prune | desktop 14 cases across 4 suites |
| per-task archived premise in the sweep | desktop `deleteSessions` fails |
| bulk archive's failure accounting | desktop `archiveSessions` fails |
The feature-boundary test earned its keep during this work: it rejected a test that imported the selection model by deep path, and the model is now exported from `features/session-navigation/testing.ts` as the README requires.
## Self-review
Reviewing the diff before opening this turned up four defects, all fixed here.
1. **A per-row array allocation that defeated the row memo.** `ProjectNavRow` built the group's id array inside the row loop, so every row got a fresh array identity — precisely what `SessionNavRow`'s `memo` exists to prevent, and O(n²) besides. Hoisted and memoized on `props.sessions`.
2. **A misplaced comment.** My first fix inserted a `useMemo` between the `#4109` comment ("Two dependencies, not eight…") and the `renderSessionRow` it describes, so the comment then read as if it described the new code. The `useMemo` turned out to be useless anyway — `SessionHistoryList` rebuilds its group array every render, so it never hit — and the only real problem was the per-row allocation in (1). Removed; the flat branch computes one array per group, which is what it did before.
3. **`preventDefault()` justified by the wrong phase.** I called it in the click handler with a comment about stopping native drag-select. The browser starts a text selection on `mousedown`, before any click exists, so the call could not have done what its comment claimed. Removed, and the real fix — `user-select: none` on the row — is in `sidebar.css` with an accurate comment.
4. **Escape and Delete jumped the text-entry guard.** Both new branches ran before the `input, textarea, [contenteditable]` check, so Escape in the rename field would have cleared the selection behind the open dialog as a second, invisible effect of one keypress. The guard now runs first for all three keys.
A fifth was considered and dismissed with evidence: `getShellCopy(locale)` returns a frozen module-level constant, so the copy object's identity is stable and the sweep callbacks do not change identity per render.
## AI use
Select exactly one:
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: Claude Opus 5 via Claude Code — design, implementation, tests, and this description. Reviewed and verified locally by me; the commit carries a `Generated-by` trailer.
## Checklist
- [x] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes — described under Summary above
- [ ] No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): select several tasks in the Session rail [maka]
Posted by "likun666661 (via GitHub)" <gi...@apache.org>.
likun666661 merged PR #4365:
URL: https://github.com/apache/maka/pull/4365
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): select several tasks in the Session rail [maka]
Posted by "Joob1n (via GitHub)" <gi...@apache.org>.
Joob1n commented on PR #4365:
URL: https://github.com/apache/maka/pull/4365#issuecomment-5488045802
Both findings were real; both are fixed in `90625c38`, and each has a test that fails when the fix is reverted. Rebased onto `9249bf3f` first, which is where `previewRemoval` and `archivedSubtaskCount` arrived.
**[P2] Old sweep clearing a newer selection.** Reproduced exactly as described. The `finally` replaced the whole set rather than the one it submitted, so A's completion discarded B. It now removes only the submitted ids and leaves the rest alone; the mode still stays on afterwards. `session-selection-sweep-race.test.ts` drives leave → re-enter → mark B → settle A with a deferred command; reverting the fix fails it (1 pass / 1 fail).
**[P2] Linked-subtask warning in bulk delete.** Also real. `deleteSelected` now asks the Host for a `previewRemoval` per selected task before confirming, and a single preview failure makes the warning uncertain rather than silently under-reporting a destructive set — the renderer's projection cannot answer this, so estimating it here would over-promise. The sweep already accumulates `archivedSubtasks`; the toast reports that executed total, not the estimate. Four cases cover the warning, the uncertain fallback, the silent case, and a declined confirm; reverting fails two of them.
One deliberate cost worth naming: this is N previews before a destructive confirm, one round trip per selected task. I kept it because the number the confirm shows has to be one the user can act on, and a set is exactly where a wrong number is least recoverable.
The conflict resolution kept both halves of the line it landed on — `main`'s destructured `archivedSubtaskCount` and this branch's per-id archived premise — so `sweepSessions` carries the count for the Settings purge and the rail alike.
**Description rewritten.** It advertised the ⌘/Shift-click gestures that `d8af780` removed. It now describes the checkbox mode, states why the modifier gestures went (they were one of two causes of a #4109 render-contract regression this branch introduced and then fixed), and reports verification against the current head — including that the one remaining e2e failure, `composer-plus-menu-stability`, fails twice in three runs on `main` and is not this branch's.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): select several tasks in the Session rail [maka]
Posted by "hqhq1025 (via GitHub)" <gi...@apache.org>.
hqhq1025 commented on code in PR #4365:
URL: https://github.com/apache/maka/pull/4365#discussion_r3897915854
##########
apps/desktop/src/renderer/features/session-navigation/controller/session-row-actions.ts:
##########
@@ -298,12 +328,156 @@ export function createSessionNavigationRowActions(deps: {
};
}
+ /**
+ * Settings › archived tasks. Every target is archived by definition, and the
+ * premise is asserted anyway so a task restored between the confirm and the
+ * write is kept rather than removed.
+ */
+ async function purgeSessions(sessionIds: readonly string[]): Promise<SessionPurgeOutcome> {
+ return sweepSessions(sessionIds, () => true);
+ }
+
+ /**
+ * The rail's multi-select delete. The rail lists unarchived tasks, so the
+ * archived premise is read per task exactly as single-row delete reads it:
+ * asserting it for a task that was never archived would refuse every
+ * deletion the rail can actually ask for.
+ *
+ * No confirm here either — one sweep is one question, and only the caller
+ * knows how many tasks it is about to name.
+ */
+ async function deleteSessions(sessionIds: readonly string[]): Promise<SessionPurgeOutcome> {
+ return sweepSessions(
+ sessionIds,
+ (sessionId) =>
+ sessionsRef.current.find((entry) => entry.id === sessionId)?.isArchived === true,
+ );
+ }
+
+ /**
+ * The rail's multi-select archive.
+ *
+ * Archiving has no disposition to report — a task is archived or the call
+ * failed — so this accounts by count and first failure rather than reusing
+ * the delete sweep's shape, which would carry a `restored` field that can
+ * never be anything but empty.
+ *
+ * Like the sweep, it raises no toast per task: one action is one message, and
+ * a run of them is what a sweep exists to avoid.
+ */
+ async function archiveSessions(sessionIds: readonly string[]): Promise<SessionArchiveOutcome> {
+ const failed: string[] = [];
+ let firstFailure: SessionArchiveOutcome['firstFailure'];
+ let archived = 0;
+ for (const sessionId of sessionIds) {
+ const key = `${sessionId}:archive`;
+ if (
+ Array.from(pendingSessionRowActionsRef.current).some((pending) =>
+ pending.startsWith(`${sessionId}:`),
+ )
+ ) {
+ failed.push(sessionId);
+ continue;
+ }
+ pendingSessionRowActionsRef.current.add(key);
+ try {
+ const familyIds = revisionFamilySessionIds(sessionsRef.current, sessionId);
+ await service.archive(sessionId, { revisionFamily: true });
+ if (activeIdRef.current && familyIds.includes(activeIdRef.current)) {
+ setActiveId(undefined);
+ clearActiveMessages();
+ }
+ for (const id of familyIds) clearSessionRendererState(id);
+ archived += 1;
+ } catch (error) {
+ failed.push(sessionId);
+ firstFailure ??= { error, sessionId };
+ } finally {
+ pendingSessionRowActionsRef.current.delete(key);
+ }
+ }
+ // Once, after the whole sweep. Refreshing per task would re-render the rail
+ // under the user's cursor for every id in the selection.
+ await refreshSessions();
+ return { archived, failed, firstFailure };
+ }
+
+ /**
+ * The rail's own bulk archive, wording included.
+ *
+ * The sweeps below it stay silent on purpose — Settings' purge phrases its
+ * own confirm — but the rail's phrasing belongs to the rail, and this module
+ * is where the feature already holds its copy. Putting it in the selection
+ * hook instead would have made that hook the feature's second importer of
+ * renderer legacy copy, which the architecture check refuses.
+ */
+ async function archiveSelected(sessionIds: readonly string[]): Promise<void> {
+ if (sessionIds.length === 0) return;
+ const ok = await toastApi.confirm({
+ title: copy.bulkArchiveTitle(sessionIds.length),
+ description: copy.bulkArchiveDescription,
+ confirmLabel: copy.bulkArchiveLabel,
+ cancelLabel: copy.cancelLabel,
+ });
+ if (!ok) return;
+ const outcome = await archiveSessions(sessionIds);
+ if (outcome.failed.length === 0) {
+ toastApi.success(copy.bulkArchivedTitle(outcome.archived));
+ return;
+ }
+ toastApi.error(
+ copy.bulkArchiveFailedTitle,
+ outcome.firstFailure
+ ? localizedShellErrorMessage(outcome.firstFailure.error, copy.actionFallback, uiLocale)
+ : copy.bulkFailedBody(outcome.failed.length),
+ undefined,
+ outcome.firstFailure ? { sessionId: outcome.firstFailure.sessionId } : undefined,
+ );
+ }
+
+ /** The rail's own bulk delete. See `archiveSelected` for why the wording is here. */
+ async function deleteSelected(sessionIds: readonly string[]): Promise<void> {
+ if (sessionIds.length === 0) return;
+ const ok = await toastApi.confirm({
+ title: copy.bulkDeleteTitle(sessionIds.length),
+ description: copy.bulkDeleteDescription,
Review Comment:
**[P2] Preserve the linked-subtask warning in bulk delete**
This confirmation only says that the selected tasks and their revisions are permanently deleted, and the success path only reports `removed`. However, the existing Host removal contract archives ordinary linked subagent tasks when their parent is deleted; the exact-head retirement test `archives direct subagent Sessions when their parent family is removed` passes and demonstrates that behavior. Current `main` now exposes `previewRemoval` plus `archivedSubtaskCount` specifically so destructive confirmations warn about these survivors and completion reports how many moved to Archived. Because this new bulk path bypasses both, deleting a selected parent can make its subtasks unexpectedly reappear under Archived with no warning or explanation. During the required rebase, aggregate the Host previews (including the uncertain fallback) before confirming and carry the executed archived-subtask counts through the sweep/result toast.
##########
apps/desktop/src/renderer/features/session-navigation/controller/use-session-selection.ts:
##########
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from 'react';
+import type { SessionSummary } from '@maka/core/session';
+import type { SessionRailRowSelection, SessionRailSelection } from '@maka/ui';
+import {
+ EMPTY_SESSION_SELECTION,
+ enterSessionSelection,
+ exitSessionSelection,
+ pruneSessionSelection,
+ setAllSessionsSelected,
+} from '../model/session-selection.js';
+import type { SessionNavigationRowActions } from './session-row-actions.js';
+
+/**
+ * The rail's multi-select: which rows are marked, and the two sweeps they feed.
+ *
+ * The selection is reconciled against the catalog on every change to it, not
+ * only after a sweep. Another window deleting a task, a Host going away, or a
+ * grouping change that drops rows all leave ids behind, and a count that
+ * includes them is a count that does not match what the confirm names.
+ */
+export function useSessionSelection(input: {
+ sessions: readonly SessionSummary[];
+ commands: SessionNavigationRowActions;
+}): { selection: SessionRailSelection; rowSelection: SessionRailRowSelection } {
+ const { sessions, commands } = input;
+ const [selection, setSelection] = useState(EMPTY_SESSION_SELECTION);
+ const [busy, setBusy] = useState(false);
+
+ const listedIds = useMemo(() => new Set(sessions.map((session) => session.id)), [sessions]);
+ useEffect(() => {
+ // `pruneSessionSelection` returns its input untouched when nothing was
+ // dropped, so this settles after one pass instead of looping on a new Set.
+ setSelection((current) => pruneSessionSelection(current, listedIds));
+ }, [listedIds]);
+
+ // The sweep reads the ids at the moment it runs, and the state it reads is
+ // the one the confirm was built from — held in a ref so the callbacks below
+ // do not change identity per selection change and re-render the bar's buttons.
+ const selectionRef = useRef(selection);
+ const busyRef = useRef(busy);
+ // Published on commit, not during render: a render React throws away must not
+ // leave a ref pointing at a selection that was never shown.
+ useLayoutEffect(() => {
+ selectionRef.current = selection;
+ busyRef.current = busy;
+ listedRef.current = listedSessionIds;
+ });
+
+ const listedSessionIds = useMemo(() => sessions.map((session) => session.id), [sessions]);
+ const listedRef = useRef(listedSessionIds);
+
+ const onToggleRow = useCallback<SessionRailSelection['onToggleRow']>((sessionId, selected) => {
+ setSelection((current) => {
+ if (current.selectedIds.has(sessionId) === selected) return current;
+ const selectedIds = new Set(current.selectedIds);
+ if (selected) selectedIds.add(sessionId);
+ else selectedIds.delete(sessionId);
+ return { active: true, selectedIds };
+ });
+ }, []);
+
+ const onEnter = useCallback<SessionRailSelection['onEnter']>((sessionId) => {
+ setSelection((current) => {
+ const entered = enterSessionSelection(current);
+ if (sessionId === undefined) return entered;
+ return { active: true, selectedIds: new Set([...entered.selectedIds, sessionId]) };
+ });
+ }, []);
+
+ const onExit = useCallback(() => setSelection(exitSessionSelection()), []);
+
+ const onToggleAll = useCallback<SessionRailSelection['onToggleAll']>((selected) => {
+ setSelection((current) => setAllSessionsSelected(current, listedRef.current, selected));
+ }, []);
+
+ /**
+ * One sweep at a time, over the ids the user could see when they pressed.
+ *
+ * Frozen at the click for the reason the archived-task purge freezes its own
+ * set: a confirm names a number to a person, and a set re-read after the
+ * dialog can be a different one. The confirm, the sweep and the report all
+ * live in `session-row-actions`, which is where this feature keeps its copy.
+ */
+ const runSweep = useCallback(
+ async (run: (sessionIds: readonly string[]) => Promise<void>) => {
+ if (busyRef.current) return;
+ const sessionIds = [...selectionRef.current.selectedIds];
+ if (sessionIds.length === 0) return;
+ setBusy(true);
+ try {
+ await run(sessionIds);
+ } finally {
+ setBusy(false);
+ // Whatever survived is reconciled by the catalog refresh the sweep
+ // already ran; clearing here is about the request, which is answered
+ // either way. Leaving a marked set behind after a destructive sweep
+ // invites a second press on rows that may no longer exist.
+ //
+ // The MODE stays on. The person was in the middle of tidying up, and
+ // taking the checkboxes away after each sweep would make them re-enter
+ // for the next one.
+ setSelection((current) => ({ active: current.active, selectedIds: new Set() }));
Review Comment:
**[P2] Do not let an old sweep clear a newer selection**
`Done` intentionally remains enabled while a bulk operation is running, but this `finally` clears whichever selection exists when the old request settles. A reachable sequence is: select A, start archive/delete, click Done after confirming, re-enter selection mode from B’s row menu, and select B before the Host operation finishes. When A’s request resolves, this updater replaces B with an empty set. I reproduced that sequence with a deferred `archiveSelected` command; the final selected set was `[]` instead of `[B]`. Track a selection/request generation (or clear only the submitted snapshot when it is still current) and add an async regression so completion from an earlier mode instance cannot mutate a later one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): select several tasks in the Session rail [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #4365:
URL: https://github.com/apache/maka/pull/4365#issuecomment-5493348379
@Joob1n — I've opened #4455 on top of this, and I'd rather you heard it here than found it in the commit log. Requesting your review there; nothing in it is settled.
The short version: **#4455 is a continuation of this PR, not a reversal of it.** Its first revision was the interaction #4455 now ships, and the description above records why it was given up:
> Rows were handed their group's session ids so a Shift-click range could know where it may reach. […] The prop is gone, and with it the modifier-click gestures it existed for — the mode is checkboxes now and nothing in it needs a range.
The gestures weren't judged worse than a mode; they cost the #4109 budget 12 of 12, and no memoisation fixes that, because every candidate key derives from `rail.sessions`. #4455 removes the constraint instead of living with it: the rendered order is never a prop at all. It is read off the DOM at the moment of the click — one query per click, nothing per render, and it is the true order, groups and collapsed projects included. With that, a range costs the budget nothing and the mode is no longer the only shape available.
What #4455 keeps from your work, because it was right and is not specific to the mode:
- **A sweep unmarks the set it submitted, never whatever is picked when it lands** — the P2 from this review. Carried over with its regression test.
- **Pruning the selection against the catalog**, extended to the range anchor.
- **The two-cause diagnosis of the render budget.** Both causes are why the picked set reaches rows as props in #4455: a plain click now changes the selection, so a context the rows subscribed to would redraw ~1,000 fibers for a switch that moved two. Your analysis is what made that obvious.
What it deliberately narrows, so it is on the record rather than buried in a diff: the rail no longer offers bulk delete, select-all, or `Delete`/`Backspace` on the marked set. Delete moves to Settings › 已归档任务, which can only reach a task archived first; select-all is not rebuilt because a range is one Shift-click. Your linked-subtask contract survives that move — `previewRemoval` still runs for the Settings purge, which now owns every deletion. What went away is the rail's entrance to it, not the accounting behind it.
Doing this a day after your PR landed is the cheapest moment, not a comment on the work: nothing depends on the mode's API yet and no user has built a habit on it. But you know this code better than anyone right now, and the six interaction decisions in #4455 were each settled separately — any of them can be reopened, including the one that matters most to me, that picked rows and the open task share a single ground.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org