feat(ui): show pinned tasks by project - #4613
Conversation
Pinning appeared to do nothing in the project view because only the time view exposed the global pinned section. Keep the presentation consistent while preserving each task's project membership. CLOSES apache#4612 Generated-by: Codex Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>
|
The problem is real and the direction is right: a pinned task should stay at the top in both grouping modes. That is what option 1 in #4453 cannot give you, since a task that stays inside its project still sits wherever that project falls. So the flattening is the price this design pays, not a flaw in it. Sorry that thread sat unanswered until you had to build something to get a reply. The presentation is where I disagree. In the project view this puts a Pinned is a small grey section heading, The shape your screenshots do not reach is where it shows most:
What I would rather seeTwo sibling I built it rather than describing it, so you can disagree with something concrete. Branch Production is +33/−20 across two files, a net removal: the fix for the count is to delete the Tests and stories are +141/−5 including the fixture for the shape above, and both new assertions fail against this PR as it stands: the description comes out as 1 task where the test wants 0, and the top-level By time is untouched: The red check is not yours
Three small simplifications are inline, none behavioural. Take the branch, take part of it, or push back on it. Evidence boundary: rendered both implementations through the desktop Storybook from one worktree, same fixtures and viewport, light and dark; ran the AI-assisted review: drafted with Maka. The proposal branch, the renders and the pixel comparison are mine. 简体中文问题是真的,方向也对:置顶的任务在两种分组模式下都该稳定在最上面。这正是 #4453 里方案一给不了的,任务留在项目里,它的绝对位置就取决于那个项目排在第几。所以层级被打平是这个设计要付的代价,不是它的缺陷。那个帖子一直没人回,让你只能先做出来才拿得到答复,抱歉。 我不同意的是呈现方式。项目视图里,这次改动把一个 「置顶」是小号灰字的分区标题, 你截图没截到的形状最能说明问题: 我更希望的样子两个平级的 我把它做出来了而不是只描述,这样你能对着具体的东西反驳。分支 生产代码 +33/−20,两个文件,是净删:计数那个问题的修法是把 测试和 story 是 +141/−5,包含上面那个形状的 fixture,两条新断言对着现在的实现都会红:描述会是 1 task 而测试要 0,顶层的 按时间视图一行没动, CI 那项红不是你的问题挂的是 行内还有三条小的化简,都不改行为。分支整个拿走、拿一部分、或者直接反驳都行。 |
Astro-Han
left a comment
There was a problem hiding this comment.
The three simplifications I mentioned in the comment above, inline. None change behaviour, and they apply whichever structure this ends up with.
简体中文
上面那条评论里提到的三处化简,写在行内了。都不改行为,而且不管最后用哪种结构都适用。
| const activeGroups = props.groups.filter((group) => group.project?.archivedAt === undefined); | ||
| const archivedGroups = props.groups.filter((group) => group.project?.archivedAt !== undefined); | ||
| const pinnedSessions = groupSessionsForHistory( | ||
| activeGroups.flatMap((group) => group.sessions.filter((session) => session.isFlagged)), |
There was a problem hiding this comment.
P3: the pin predicate is applied twice in one expression. groupSessionsForHistory already splits on isFlagged and puts the result in the pinned bucket, so this pre-filter is doing work the next line undoes the need for. Passing the sessions through unfiltered leaves one place that decides what pinned means:
| activeGroups.flatMap((group) => group.sessions.filter((session) => session.isFlagged)), | |
| activeGroups.flatMap((group) => group.sessions), |
There was a problem hiding this comment.
Applied. The full active-session list now goes through groupSessionsForHistory, leaving the pinned predicate in one place.
| const pinnedSessions = groupSessionsForHistory( | ||
| activeGroups.flatMap((group) => group.sessions.filter((session) => session.isFlagged)), | ||
| locale, | ||
| ).find((group) => group.id === 'pinned')?.sessions ?? []; |
There was a problem hiding this comment.
P3: the group you just built already carries its own label, so copy.pinned further down is a second place naming the same section. Keeping the group and reading .label off it in the JSX removes that split, and it also makes the length > 0 check unnecessary since groupSessionsForHistory only emits the bucket when it is non-empty:
| ).find((group) => group.id === 'pinned')?.sessions ?? []; | |
| ).find((group) => group.id === 'pinned'); |
Then {pinnedGroup && <SideNavSection title={pinnedGroup.label} ...>} at the render site.
There was a problem hiding this comment.
Applied. The render now consumes pinnedGroup directly, including its label and sessions.
| label: string; | ||
| project?: ProjectRecord; | ||
| sessions: SessionSummary[]; | ||
| summarySessions?: SessionSummary[]; |
There was a problem hiding this comment.
P3: the only call site always passes this, so the optional marker and the ?? props.sessions fallback below are a branch that cannot be reached. Making it required states the contract that the project branch actually relies on, which is that the summary counts full membership and never the filtered subtree:
| summarySessions?: SessionSummary[]; | |
| summarySessions: SessionSummary[]; |
There was a problem hiding this comment.
Resolved by the two-section change: summarySessions was removed entirely, and the project summary now reads the same filtered sessions that its subtree renders.
| props.streamingSessionIds, | ||
| ), | ||
| [props.project, props.sessions, props.streamingSessionIds], | ||
| [props.project, props.sessions, props.streamingSessionIds, props.summarySessions], |
There was a problem hiding this comment.
P3: props.sessions is no longer read inside this memo, so listing it here recomputes the summary whenever the filtered subtree changes identity even though the summary cannot have changed:
| [props.project, props.sessions, props.streamingSessionIds, props.summarySessions], | |
| [props.project, props.streamingSessionIds, props.summarySessions], |
There was a problem hiding this comment.
Resolved with the same removal: the memo now depends only on project, sessions, and streamingSessionIds; summarySessions no longer exists.
The pinned zone and the project rows are two different Astryx primitives laid out as siblings. SideNavSection is a group of navigation items, not one of them: it renders role="group" with aria-labelledby, and its documented shape is a Section containing Items. Flattening one Section next to a row of SideNavItems puts a group heading and navigation items on the same level, and the seam shows. The pinned heading is small grey text where the project rows are full items with a folder icon; the project rows collapse and the pinned zone cannot; the project rows carry a row menu and the pinned zone does not. The one thing the two do share is the indent under them, so the layout promises a hierarchy it gives no way to operate. Render the two as sibling sections instead, "Pinned" and "Projects". The containment relationship is restored, project rows stay items inside a section with their icon, disclosure and row menu untouched, and the two grouping modes end up structurally symmetric: Pinned / Recent by time, Pinned / Projects by project. Archived projects stay the last item of the Projects section, because what that row holds is projects. Hoisting the pinned tasks out of a project also left the project row describing rows it no longer shows. The row derived its disclosure and its menu placement from the filtered list while the hover summary read the unfiltered one, so a project whose only visible task was pinned drew no chevron and no subtree while its accessible description still announced 1 task. Drop the second list: the summary reads the same sessions the row renders, so the count, the chevron and the menu placement have one source. A story fixture covers that shape. Generated-by: Claude Code
The scroll E2E recurred because moving to zero was treated as the request even though a settled scroller cannot move farther. Dispatching the upward wheel matches the production trigger and removes timing dependence on a scroll event. Signed-off-by: Jiawei Zhao <Phoenix500526@163.com> Generated-by: OpenAI Codex
The history grouping helper already owns the pinned predicate and label. Using its result directly keeps that definition in one place. Signed-off-by: Jiawei Zhao <Phoenix500526@163.com> Generated-by: OpenAI Codex
|
Thanks — I took the two-section proposal, including the pinned-only project fixture and the accessible-count correction, and applied the remaining lookup simplifications from the inline comments. I also included the explicit wheel trigger for the unrelated transcript-scroll flake; the affected UI suite is green and that E2E passed 10/10 locally. |
Astro-Han
left a comment
There was a problem hiding this comment.
Approving on af54c482d. You took the two-section shape, and the follow-up you did on top of it is better than what I handed you.
7c23d373a puts Pinned and Projects at the same level as two SideNavSections, so Section-contains-Item comes back and the project rows keep their folder icon, disclosure and row menu untouched. Both grouping modes now share one structure.
af54c482d is the part I did not think of. Instead of filtering isFlagged first and then finding the pinned group in the result, you feed the whole set to groupSessionsForHistory and take the group it produces, title included. That removes the project view's separate reference to copy.pinned, so what counts as pinned, how it is ordered and what it is called all resolve in one place (session-history-list.tsx:1484). One authority instead of two that agree by hand.
With summarySessions gone, hasSessions, the row menu position and the accessible count read the same list again. The story fixture and the two assertions pin the shape rather than describing it: the top-level [role="group"] list is exactly Pinned and Projects, and a project whose only visible task is pinned reports zero tasks instead of one.
The single line in transcript-scroll.spec.ts is unrelated to this change and I read it as correct. That spec's own comment says it is testing the wheel-to-load path, and setting scrollTop = 0 alone was missing that step; dispatching the wheel event is supplying the driver the case always needed, not papering over the failure. request top history explicitly describes it accurately.
Thanks for pushing back with an implementation rather than a description. Merging.
简体中文
在 af54c482d 上批准。两个 section 的形状你采纳了,而且在它上面又做的那一步比我给你的版本更好。
7c23d373a 把「置顶」和「项目」放成两个平级的 SideNavSection,Section 装 Item 的关系回来了,项目行的文件夹图标、折叠箭头、行内菜单一行没动,两种分组模式现在结构相同。
af54c482d 是我没想到的那一步。你不再先按 isFlagged 过滤、再去结果里找置顶分组,而是把整份列表喂给 groupSessionsForHistory,直接取它产出的那个分组,连标题一起用。这样项目视图里对 copy.pinned 的单独引用就没了,「哪些算置顶、怎么排、叫什么」全都落在 session-history-list.tsx:1484 一处。一个权威,而不是两个靠人手对齐的地方。
summarySessions 删掉之后,hasSessions、行内菜单的位置和无障碍计数又读同一份列表了。story fixture 和那两条断言是把形状钉住而不是描述它:顶层的 [role="group"] 恰好是「置顶」和「项目」,而唯一可见任务被置顶的项目报的是 0 条任务而不是 1 条。
transcript-scroll.spec.ts 那一行和这次改动无关,我看了一下是对的。那条测试自己的注释就写着它测的是 wheel 触发加载那条路径,而只设 scrollTop = 0 少了这一步;派发 wheel 事件是把这个用例本来就缺的驱动补上,不是把失败盖过去。commit 名 request top history explicitly 说得很准。
谢谢你用一个实现而不是一段描述来反驳我。合了。
…#4613) Pinned tasks were only pinned within their own group. Grouping by project put a pinned task wherever its project happened to fall, so the one list position a user pins something to reach was not stable across the two grouping modes. The project view now renders two sibling `SideNavSection`s, Pinned and Projects, the same shape the by-time view already had as Pinned and Recent. Project rows stay `SideNavItem`s inside the Projects section, so Section-contains-Item holds in both modes and the rows keep their folder icon, disclosure and row menu. Archived projects remain the last item of the Projects section, since what that section holds is projects. The pinned rows and their heading come from the group `groupSessionsForHistory` already produces, rather than from a separate `isFlagged` filter and a second reference to the copy. What counts as pinned, how it is ordered and what it is called now resolve in one place for both modes. No prop carries an unfiltered copy of the list alongside the filtered one, so `hasSessions`, the row menu position and the hover summary's task count read the same sessions. A project whose only visible task is pinned no longer reports a task it does not show. The by-time view is unchanged. Generated-by: Claude Code Generated-by: Codex Generated-by: OpenAI Codex Generated-by: GLM-5.3-Flash (ZCode)




Summary
Pinning a task in By project previously changed the menu action to
Unpin, but left the task in place with no visible indication that Pin had
taken effect. By time already presents pinned tasks in a dedicated global
section, so the same state looked different across the two grouping modes.
Add the global Pinned section above project groups in By project.
Pinned tasks appear there once instead of being duplicated in their project,
while their underlying project membership and project summary counts remain
unchanged. Tasks from archived projects stay inside the archived section.
This implements the behavior discussed in
Discussion #4453.
Fixes #4612
Verification
npm run build -w @maka/uinode --test packages/ui/dist/__tests__/session-history-row-actions.test.js(10 passed)
npm run typecheck:stories -w @maka/desktopnpm exec -- biome check packages/ui/src/session-history-list.tsx packages/ui/src/__tests__/session-history-row-actions.test.tsx packages/ui/stories/session-list-panel.stories.tsxproduction sidebar components.
Before Change
After Change
AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex implemented the UI behavior, regression test,
Storybook fixture, and verification with the contributor's review.
Checklist
Does this PR entail a change in behavior?