Skip to content

fix(desktop): contain deeply nested task rows in the task panel - #4215

Closed
liuxiaocs7 wants to merge 4 commits into
apache:mainfrom
liuxiaocs7:liuxiaocs7/task-panel-failure-states
Closed

fix(desktop): contain deeply nested task rows in the task panel#4215
liuxiaocs7 wants to merge 4 commits into
apache:mainfrom
liuxiaocs7:liuxiaocs7/task-panel-failure-states

Conversation

@liuxiaocs7

@liuxiaocs7 liuxiaocs7 commented Aug 29, 2026

Copy link
Copy Markdown
Member

Summary

Extends the real TaskLedgerPanel Storybook surface with task states that the existing Tasks / TasksEmpty / TasksLoadFailed stories did not cover:

  • TasksRecentlyFinished covers failed and cancelled outcomes, failure detail, and the three-item recent-terminal cap.
  • TasksDeepNesting covers an eight-level task chain and asserts that the deepest visible row remains contained with no horizontal overflow.
  • The task-tree layout now keeps recursive groups at panel width and applies the clamped depth indentation only to each row’s visible content.

Refs #3944, #3893

Verification

  • npm --workspace @maka/ui run typecheck
  • npm --workspace @maka/desktop run typecheck:stories
  • npm --workspace @maka/ui run test — 275 tests passed
  • npx biome check apps/desktop/stories/session-workbar.stories.tsx apps/desktop/src/renderer/styles/task-ledger.css packages/ui/src/task-ledger-panel.tsx
  • npm --workspace @maka/desktop run build-storybook
  • npm --workspace @maka/desktop run smoke:storybook — 207 stories passed, including product-session-workbar--tasks-deep-nesting

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code authored the original Storybook stories. Codex resolved the merge conflict, implemented the nested-task layout fix, added the overflow regression assertion, and ran verification. Affected commits retain Generated-by trailers.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — deeply nested task rows now remain within the task panel.
  • No

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Aug 29, 2026
@liuxiaocs7
liuxiaocs7 force-pushed the liuxiaocs7/task-panel-failure-states branch from 2d63592 to 8f4c92b Compare August 29, 2026 21:36

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex-assisted review performed under the maintainer-approved review workflow.

I found one P2 coverage gap. This PR adds two real-component Storybook states for recent terminal tasks and an eight-level task chain; I inspected the complete diff plus TaskLedgerPanel, its model tests, CSS, and the latest-main synthetic merge.

Validation: hosted test is green; exact-head Storybook typecheck, build, 198-story interaction/AX smoke, Biome, and the 264-test UI suite passed. The synthetic merge 8340a25894e60f2e1d74613ca091c57a466239ae also passed Storybook typecheck/build and the 203-story interaction/AX smoke, with ASF headers clean.

I could not validate native Electron-only behavior beyond the real Storybook component path, but the reported overflow is deterministic DOM geometry in both wide and narrow canvases.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

Comment thread apps/desktop/stories/session-workbar.stories.tsx Outdated
@liuxiaocs7 liuxiaocs7 changed the title test(desktop): story-cover task panel terminal and deep-nesting states fix(desktop): contain deeply nested task rows in the task panel Aug 30, 2026
Eighth surface under apache#3944 (one surface per PR): extend the 任务 panel
(TaskLedgerPanel, via Product/Session Workbar) with the states the existing
Tasks / TasksEmpty / TasksLoadFailed stories never reach.

- TasksRecentlyFinished — the 最近结束 section with a failed task (+ reason) and
  a cancelled task, plus four finished tasks so the three-item cap drops the
  oldest.
- TasksDeepNesting — an eight-level subtask chain; the play asserts the deepest
  row renders (reachability). The indent clamp (`--task-depth`) is a
  computed-style contract left to focused tests (review feedback).

pending / in_progress / blocked / completed, owners, and shallow nesting were
already covered by Tasks; failed, cancelled, the recent-finished cap, and deep
nesting were not. stale / responding / unread / project-grouping have no UI in
this panel and are left out.

Refs apache#3944, apache#3893

Generated-by: Claude Code
Keep nested task groups at the panel width, apply depth indentation only to row content, and assert the deepest task has no horizontal overflow.

Generated-by: Codex
@liuxiaocs7
liuxiaocs7 force-pushed the liuxiaocs7/task-panel-failure-states branch from da68dc2 to 0f2e1f7 Compare August 30, 2026 09:00
@liuxiaocs7
liuxiaocs7 requested a review from hqhq1025 August 30, 2026 09:16

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding focused coverage for the deep-nesting state and for moving depth padding off the recursive group wrapper. I found one remaining bounded case at the supported minimum Workbar width below. This is a suggestion from an outside review rather than a Request Changes review; please push back if I have misunderstood the panel-width contract.\n\nAI-assisted review disclosure: Codex ran independent code/UI and test-quality lanes; Astro-Han independently verified the exact-head CSS, production width authority, reachability, CI, and severity and owns this review.

Comment thread apps/desktop/src/renderer/styles/task-ledger.css
The deep-nesting containment fix collapsed to the compact three-column
row via a viewport @media (max-width: 620px). But the right workbar
resizes down to SESSION_WORKBAR_MIN_WIDTH (320px) inside an otherwise
wide window, where that viewport query never fires — so at clamped
depth 6 the 128px indent plus key/subject/status columns overflowed the
320px panel horizontally.

Make .maka-task-ledger-panel a named inline-size container and select
the compact layout with @container (max-width: 420px), matching the
inspector/chat-header container-query precedent. 420px keeps the 480px
default wide while covering the 320px floor and the common 400px resize
width. Pin TasksDeepNesting at width=320 so its containment assertion
exercises the supported minimum panel width.
@Astro-Han Astro-Han self-assigned this Aug 31, 2026
@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for turning both review rounds around properly — I want to be clear up front that what follows is not a problem with your work.

This PR has been superseded by #4351, which merged on 2026-09-01. That change retired the Task Ledger demand chain and replaced it with a Host-owned, flat SessionTodo document. packages/ui/src/task-ledger-panel.tsx is gone from main (hence the modify/delete conflict), and the panel is now packages/ui/src/session-todo-panel.tsx, which renders:

<li className="maka-task-ledger-row" key={...}>
  <StatusIcon size={ICON_SIZE.control} aria-hidden="true" />
  <span>{item.content}</span>
</li>

Only the class names carried over. Every premise this PR builds on is gone from main: there is no recursive role="group" nesting, no --task-depth or aria-level, no four-column key/subject/status/meta row, and SessionTodoStatus is only pending | in_progress | completed — no failed or cancelled, and no recent-finished section. So the deep-nesting overflow this PR fixes can no longer occur, and neither TasksDeepNesting nor TasksRecentlyFinished can be constructed against the new panel. This is not a rebase away; the target was removed.

For the record, both rounds of feedback were resolved correctly before that landed. The .maka-task-ledger-row-content wrapper did contain the recursive width, and 2cce8f259 was the right answer to the 320px question — the compact layout should key off the panel's own inline size rather than the viewport, and the container-query approach matched the inspector.css / chat-header.css precedent. That judgement was sound and it is worth keeping.

Suggest converting this to draft (or closing it) and re-deciding what coverage is still worth having against SessionTodoPanel. My read is that most of it no longer applies, and the one idea worth carrying forward is separate from this PR: the dead CSS #4351 left behind in apps/desktop/src/renderer/styles/task-ledger.css, where the viewport-vs-container point you found still holds independently of nesting. I have filed that as its own issue and left it unassigned, so please pick it up if you want it.

Reviewed at head 9cbf19bd.

AI use: Claude Code traced the supersede against main and drafted this comment; I verified the file removal, the new panel's structure, and the status enum myself.

@liuxiaocs7

Copy link
Copy Markdown
Member Author

Closing this PR: it has been obsoleted by #4351 (6b9d16fed, merged into main), which retired the hierarchical Task Ledger and cut the panel over to a flat, Host-owned SessionTodo document.

The nested rendering this PR fixes no longer exists on main:

  • packages/ui/src/task-ledger-panel.tsx (where the depth clamp lived) was deleted and replaced by session-todo-panel.tsx, a read-only flat <ol>.
  • SessionTodoItem is now strictly { content, status } — no parentId, no aria-level, no --task-depth, no tree. So there are no deeply nested rows to contain, and the .maka-task-ledger-row-content grid / @media compact block this PR touched are no longer rendered.

Because the whole premise is gone, rebasing onto main produces a modify/delete conflict against a deleted file rather than a resolvable merge, so continuing this PR would mean re-introducing retired code. Closing instead. Thanks @hqhq1025 and @Astro-Han for the reviews — the 320px containment concern is moot under the flat panel. If the leftover dead rules in task-ledger.css are worth removing, that's a small standalone follow-up.

@liuxiaocs7 liuxiaocs7 closed this Aug 31, 2026
@liuxiaocs7

Copy link
Copy Markdown
Member Author

Thanks @Astro-Han and @hqhq1025 for the reviewing, close it now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants