refactor(web): collapse TaskAuxiliaryPanel into a single scrolling view - #2307
Conversation
Replace the 7-tab configurator (Details/Workflow/Agents/Gates/Artifacts/ Timeline/Log) plus the dual flat-cards-vs-tabs layout with one scrolling detail panel. Removes the Agents tab and all per-agent model-override machinery (AgentConfigCard, WorkflowModelSelect, override state, node-executions fetch), the Gates tab + GateCard, the Log tab + WorkflowExecutionLogFeed, and the workflow-detail fetch/node-list. Result order: header -> Description -> Details (now includes the workflow selector row, depends-on, and schedule) -> Result -> Timeline -> Artifacts. The right-panel and middle-column now share one layout path; only the middle-column back button differs. The status actions menu is available in both header variants. TaskAuxiliaryPanel no longer takes a tab prop, so RightPanel stops forwarding it; the signal's tab field stays for SpaceTaskPane.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM / Zhipu)
Model: glm-5.1 | Client: NeoKai | Provider: GLM (Zhipu)
Recommendation: APPROVE. (Posted as COMMENT because GitHub rejects APPROVE on the author's own PR.) Clean, well-scoped refactor (-871/+213) that does exactly what Task #792 asked.
Verified:
- typecheck (web
tsc --noEmit) clean;oxlintclean on the touched files; knip unaffected (its config ignorespackages/web/src/**). TaskAuxiliaryPanel.test.tsx(12) +TaskArtifactsPanel.test.tsx(18) pass. Tests were rewritten sensibly — tab-switching + model-override cases removed; single-view layout, back-button (middle column only), right-panel actions menu, not-found, depends-on, autosave, and workflow-select cases kept/added.- Scope items all addressed: tab bar +
TASK_PANEL_TABS/availableTabs/normalizeTab/selectTabeffect gone; Agents tab + all per-agent model-override machinery (AgentConfigCard,WorkflowModelSelect,overrideKey,findExecution,resolvedAgentModel, node-executions ensure/fetch effect,pendingOverridesRef, etc.) gone; Gates/GateCardgone; Log tab gone; Workflow folded into a Details row; one unified layout path (only the back button differs); Artifacts section wired via existingTaskArtifactsPanel; existingTaskTimelineFeedretained. - Hooks all called before the
if (!task)early return — no Rules-of-Hooks issue. ActorMessageProjectionFeedhas its ownh-full overflow-y-auto, so theh-96timeline box scrolls internally (no clipping).- Error handling actually improved: status-transition errors (
transitionError) now render at the top of the scroll in both layouts, whereas the old sharedoverrideErroronly surfaced on the now-removed Agents tab.
Non-blocking P3 notes (optional cleanup, not required to merge):
WorkflowExecutionLogFeed.tsxis now an orphan — zero importers repo-wide after this PR. knip won't catch it (web/src is ignored), and the task scoped removing only the usage, so this is fine to defer — but the file is genuinely dead. Either delete it or confirm the curated-timeline follow-up will reuse it.rightPanelTargetSignal'stabfield is now write-only:RightPanel.tsx:106andSpaceTaskPane.tsx:260,298still write it, andTaskRightPanelTabstill enumerates the removedagents/gates/log. Nothing readstarget.tabin production anymore (the only.tabreads are unrelated URL-query tabs inrouter.ts). Pre-existing in untouched files and out of scope here — worth a small follow-up. (Minor nit: the PR description says the field "stays for SpaceTaskPane," but SpaceTaskPane only writes it.)- Timeline
bottomInsetPxdropped from16→0(theActorMessageProjectionFeeddefault), so the last entry hugs the bottom of the 384px scroller. Trivial, and the timeline is explicitly interim UI pending the curated follow-up.
No P0–P3 blocking findings. Ship it.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
HyperNeo/packages/web/src/components/space/TaskAuxiliaryPanel.tsx
Lines 216 to 219 in 7f85e80
Hide approval transitions from the auxiliary action menu
When a task is in review—particularly with a task_completion or gate checkpoint—this newly exposed right-panel menu includes the unfiltered Approve and Cancel actions. Selecting Approve calls the generic spaceTask.update path, bypassing approvePendingCompletion and PostApprovalRouter, so a workflow configured to run an auto-merge or another post-approval step can instead be marked done without dispatching that step. Apply the same checkpoint-aware filtering used by SpaceTaskPane and TaskStatusActions before constructing these menu items.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…itch Two follow-ups to the panel collapse, raised in review (#2307): - Legacy /timeline and /artifacts task URLs: SpaceTaskPane still records the requested section in rightPanelTargetSignal.tab (then rewrites the URL to /thread). The panel no longer reads tab for routing, so pass it as focusSection and scrollIntoView the matching section, restoring the deep-link target now that tabs are sections in one scroll. - TaskArtifactsPanel was rendered unkeyed; its drill-down view state is not reset when runId/taskId change, so retargeting tasks mid-drill-down could show task A's file/commit against task B's run. Key it by workflowRunId so it remounts (and resets local state) when the run changes.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Re-review by glm-5.1 (GLM / Zhipu)
Model: glm-5.1 | Client: NeoKai | Provider: GLM (Zhipu)
Recommendation: APPROVE (COMMENT because GitHub rejects self-APPROVE). Re-reviewing commit b37dffd96, which folds in both P2 follow-ups. Both are correctly implemented.
Fix 1 — legacy deep-link scroll (focusSection): Verified. RightPanel forwards renderedTarget.tab as the optional focusSection prop; TaskAuxiliaryPanel maps timeline→task-timeline-section / artifacts→task-artifacts-section and scrollIntoViews it inside the scroll container. The call is optional-chained (?.scrollIntoView?.()) so it's jsdom-safe; the testid values are hardcoded literals (no selector-injection surface); it no-ops cleanly when the section is absent (e.g. artifacts without a run — and SpaceTaskPane only writes tab:'artifacts' when a run exists, so that path is handled upstream). Hook is declared before the if (!task) early return, so hook order is fine. Two new tests cover the scroll + the no-focus no-op.
- P3 nit (non-blocking): the effect deps are
[focusSection, taskId], so if the task loads after the panel's first render (latespaceStore.taskspopulation), the scroll container mounts but the effect won't re-run and the scroll is missed. Narrow (only legacy bookmark deep-links, and the task is usually already in the store when the panel opens). If you want belt-and-suspenders, add task presence (e.g.task?.idor ahasTaskflag) to the deps.
Fix 2 — key={task.workflowRunId} on TaskArtifactsPanel: Verified. Remount-on-run-change correctly resets the drill-down view state (the actual defect) without touching the component internals. Addresses the exact cross-run retarget concern raised.
-
P3 nit (non-blocking): two different tasks sharing the same
workflowRunId(rare) wouldn't remount on task switch. If that matters,key={${task.workflowRunId}:${task.id}}is more thorough — but for the same run, keeping the view is arguably correct anyway.
Checks: tsc --noEmit clean; oxlint clean on the touched files; TaskAuxiliaryPanel (14) + TaskArtifactsPanel (18) = 32 targeted tests pass.
Biome note (P3, non-blocking): biome is not a CI gate here (check runs oxlint, not biome check; biome only appears as format --write). That said, the "biome clean" claim is loose — biome check flags mostly pre-existing items (noNonNullAssertion on the forge-scope !, import order, svg a11y) plus one newly-introduced noLabelWithoutControl on the Workflow <label>. Harmless since ungated, but pairing the label with htmlFor/id would clear it.
Threads: both resolved (one shows outdated because the key edit shifted the anchored line — benign).
No P0–P3 blocking findings. Ship it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b37dffd96a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The collapse dropped the workflow-run status row, so a workflow-backed task no longer showed which workflow ran or its execution status — distinct from the task's own status (e.g. a task marked Done vs a run marked Succeeded). For Auto-select tasks the Workflow row just read 'Auto-select' with no indication of what executed. Re-adds, in Details and gated on task.workflowRunId (store reads only, no restored workflow-definition fetch): - Run status DetailRow via getWorkflowRunExecutionStatusLabel - Executed workflow name when it differs from preferredWorkflowId (the Auto-select case), avoiding duplication otherwise Reviewed in #2307.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Final re-review by glm-5.1 (GLM / Zhipu)
Model: glm-5.1 | Client: NeoKai | Provider: GLM (Zhipu)
Recommendation: APPROVE (COMMENT because GitHub rejects self-APPROVE). Reviewing commit 1452ef141, which restores the workflow run-status info (P2-A).
P2-A fix — verified correct and well-tested. getWorkflowRunExecutionStatusLabel is re-imported; Details now shows a "Run status" row plus an "Executed workflow" row, both via store-only reads (spaceStore.workflowRuns / spaceStore.workflows — no restored workflow-definition fetch), gated on task.workflowRunId. The executed-workflow row is suppressed when it matches task.preferredWorkflowId, so the Auto-select case surfaces the actually-run workflow without duplicating the selector. This restores the only view of run status for a workflow-backed task, exactly addressing the regression. Tests re-add mockWorkflowRuns and assert both the "Succeeded" label and the suppression when the executed workflow matches the configured one.
P2-B / P2-C — agreed defer. P2-B (re-scroll on an identical repeated legacy nav) is near-zero-frequency and the source doesn't re-fire either. P2-C (eager artifacts/Timeline mount) is an enhancement over what Task #792 item 8/9 explicitly specified ("render as a section") — a lazy/IntersectionObserver follow-up is the right home if RPC cost on task-open ever matters. Both are correctly P3 follow-ups, not blockers.
Final state: tsc clean, oxlint clean, targeted tests pass; all 5 review threads resolved (0 unresolved); PR OPEN/MERGEABLE. The structural simplification in Task #792 is complete, and the two integration regressions surfaced during review (legacy deep-link landing, cross-task artifacts state) plus the run-status information loss are all addressed.
No P0–P3 blocking findings. Ship it.
TaskAuxiliaryPanel keeps its bespoke header (it does not use InspectPanelHeader), so the badges fragment needs its own mt-2 flex flex-wrap gap-2 row — otherwise the three header badges touch and can clip at the 280px min panel width. Regression from the rebase onto #2307.
…2311) * refactor(web): unify third-column inspect panels behind shared shell Extract the duplicated inner chrome of the git/task/goal/scope inspect panels into shared primitives so all four render through the same header + scroll-body machinery. - InspectPanel: outer column shell with header/body/empty-state slots - InspectPanelHeader: sticky 88px detail header (title/badges/actions) - InspectBadge: rectangular header badge driven by indicator tones - SectionCard: rounded bordered section wrapper with uppercase label - priority-tokens: map task/goal priority to a unified indicator tone GitPanel keeps its bespoke 52px header + master-detail body (renders identically) but adopts the InspectPanel outer shell. Goal and Scope chrome is normalized onto the shared header + SectionCard; priority and status badges now derive from the #777 indicator palette instead of per-panel ad-hoc classes. * refactor(web): drop unused inspect-primitive surface + test priority tones Address review feedback on PR #2311: - Remove unused InspectPanelHeader.titleNode, SectionCard.action, and SectionCard.bodyClass — no migrated panel passes them (Simplicity first). - Add getPriorityIndicatorTone unit test (low/normal→neutral, high→warning, urgent→danger). * fix(web): restore TaskAuxiliaryPanel badge-row container TaskAuxiliaryPanel keeps its bespoke header (it does not use InspectPanelHeader), so the badges fragment needs its own mt-2 flex flex-wrap gap-2 row — otherwise the three header badges touch and can clip at the 280px min panel width. Regression from the rebase onto #2307.
…2310) * fix(space): make root-repo sync an explicit post-approval merge step Step 5 of PR_MERGE_POST_APPROVAL_INSTRUCTIONS claimed root-repo sync is 'handled outside the isolated worktree' — false, since the reviewer post-approval session is the only actor that runs it. The disclaim caused a reviewer to skip syncing the root repo after merging PR #2307. Replace it with an explicit 'git -C <mainRepoPath> pull --ff-only origin dev' (the banner names that path as 'Main repository' and permits writes under post-approval). Keep the worktree-safe 'git fetch origin dev'; mirror step 4's best-effort cleanup_warning handling if the ff-only fails. * fix(space): derive root-repo path from git in post-approval sync step Round-1 step 5 referenced the worktree-isolation banner and a literal <mainRepoPath> to name the root path — both broken for the post-approval reviewer session, which has no session.worktree (worker sub-session via createCustomAgentInit), so QueryOptionsBuilder never appends the banner and <mainRepoPath> is not an interpolation token. The reviewer would run it verbatim, fail, and leave the root stale — the exact regression intended. Derive the root from git instead: ROOT from the worktree's shared common dir parent. Add guards: refuse a root not on dev (pull --ff-only moves the current branch), refuse to claim sync when local dev is ahead of origin/dev ('Already up to date' hides stray commits), keep best-effort cleanup_warning on pull failure. Also run the sync on step 1's already-merged path so a restart after merge still syncs the root. * fix(space): make post-approval merge template branch-agnostic These are product built-ins (Coding/Research/QA) that run against arbitrary user repos, but the merge template hard-coded `dev` everywhere — breaking any repo whose base branch is main/master or a release branch. Derive the base up front from the PR's baseRefName (the branch it merges INTO, which can differ from the repo default, e.g. a release branch): BASE=$(gh pr view {{pr_url}} --json baseRefName --jq .baseRefName) and sweep every literal dev/origin/dev in steps 3–5 to $BASE/origin/$BASE (fetch, merge-tree, base_branch artifacts, rebase target, root sync, guards). Round-2 work preserved: git-derived root path, non-base + ahead-of-origin guards, already-merged→step-5 routing, best-effort cleanup_warning. Updated the dev-pinning tests and added a branch-agnostic guard (baseRefName present, no origin/dev). * fix(space): sync the configured Space checkout, not a git-inferred root Round 2 derived the sync path from `git rev-parse --git-common-dir`, but that resolves to the shared main-repo .git — whose parent is a DIFFERENT checkout when the Space workspace is itself a linked worktree. createTaskWorktree bases future task worktrees on the workspace's HEAD (`git worktree add … HEAD` with cwd=space.workspacePath), so inferring the main repo leaves the actual branching base stale. The configured workspace path is already threaded into the post-approval context as {{workspace_path}} (= space.workspacePath). Use it directly: SPACE_WS="{{workspace_path}}" and fast-forward THAT checkout (with the existing $BASE / ahead-of-origin / pull-failure guards). Renames $ROOT → $SPACE_WS to reflect that it is the Space checkout, not the repo root. No new token plumbing needed. * fix(space): single-quote workspace path; resolve $BASE in tool-call payloads Two round-5 P2s on the merge template: 1. Shell-quote the interpolated workspace path. The interpolator renders values verbatim, so SPACE_WS="{{workspace_path}}" (double quotes) would expand $/backtick/ $() in a workspace path. Single-quote it: SPACE_WS='{{workspace_path}}'. 2. $BASE is a shell variable, but step 3's base_branch fields live inside send_message / save_artifact (MCP/JSON, not shell), so "$BASE" there would be sent literally and the coder would be told to rebase onto literal origin/$BASE. Use the resolved-value placeholder <base branch> in those tool-call fields (and the coder message body), and clarify in step 1 that tool-call payloads take the actual value, not the variable. Added negative regression guards for both. Shell-command uses of $BASE (git fetch/pull, the guard) are unchanged. * fix(space): re-derive $BASE in each post-approval Bash block Each Bash tool call runs in a fresh shell, so the $BASE assigned in step 1 does not persist — step 3a's conflict inspection (git fetch origin "$BASE") and step 5's sync/guard would receive an empty value, breaking conflict inspection and leaving the Space checkout stale after a successful merge. Re-derive BASE at the top of each dependent command block (3a, 5a, 5b) — the gh pr view baseRefName call is idempotent — and note the statelessness next to the step-1 definition.
1 participant
Collapses
TaskAuxiliaryPanelfrom a 7-tab configurator (~769 lines) into one scrolling detail panel: header → Description → Details → Result → Timeline → Artifacts, with one layout path for both the right panel and middle column (only the back button differs).Removes the Agents tab and all per-agent model-override machinery (state, node-executions fetch,
WorkflowModelSelect), the Gates tab +GateCard, the Log tab +WorkflowExecutionLogFeed, and the workflow-detail fetch/node-list. The Workflow selector folds into a Details row. The component no longer takes atabprop, soRightPanelstops forwarding it; the signal'stabfield stays forSpaceTaskPane.Tests rewritten to cover the single-view layout and kept sections; tab-switching and model-override tests removed.