🤖 fix: open goal tab from workspace target pill#3272
Conversation
|
/coder-agents-review |
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: e9497863d0
ℹ️ 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".
ThomasK33
force-pushed
the
target-interaction-6wbb
branch
from
e949786 to
d24086f
Compare
|
/coder-agents-review |
|
@codex review Please take another look after the goal-pill sidebar collapse fix. |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is sound: replacing a timing-dependent custom event with a persistence-based channel that survives workspace transitions is the right call. The workspaceSelection extraction is a clean DRY improvement, and the test verifies real behavioral outcomes rather than implementation details.
1 P2, 2 P3, 2 Nit, 1 Note.
The P2 is a regression: the old handleOpenGoalTab handler called setCollapsed(false) before selecting the tab. The new code persists the layout but never touches the collapsed state, so the Goal tab ends up selected behind a 20px collapsed sidebar. Ten of sixteen reviewers flagged this independently.
"User collapses sidebar, clicks a goal pill on the workspace they are already viewing, nothing visible happens. They click it again, still nothing. They file a bug or forget the feature exists." (Hisoka)
🤖 This review was automatically generated with Coder Agents.
ThomasK33
force-pushed
the
target-interaction-6wbb
branch
from
d24086f to
2611d5e
Compare
|
/coder-agents-review |
|
@codex review Please take another look after the coder-agents-review fixes. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
All six R1 findings addressed. The collapsed-sidebar regression (DEREM-1) is fixed with test coverage. The shared helper extraction (DEREM-3) is clean. The subtle improvement from using the dynamic tab fallback instead of hardcoding "costs" is a nice bonus.
1 new P3.
The DEREM-3 fix eliminated the duplication between instructionsTabFocus.ts and the goal-pill path, but the extracted helper re-created the same pattern that already exists in commands/sources.ts. Both files now have identical getRightSidebarTabFallback implementations and structurally equivalent layout-update sequences.
"The DEREM-3 fix correctly eliminated the duplication between instructionsTabFocus.ts and the goal-pill path. But the extracted helper re-created a pattern that already exists one directory over." (Robin)
🤖 This review was automatically generated with Coder Agents.
ThomasK33
force-pushed
the
target-interaction-6wbb
branch
from
2611d5e to
7d1b087
Compare
|
/coder-agents-review |
|
@codex review Please take another look after the shared right-sidebar layout helper follow-up. |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
All eight prior findings addressed. The DEREM-8 consolidation is clean: commands/sources.ts now imports the shared helpers from rightSidebarTabFocus.ts, and the type annotations improved as a side effect. The core goal-pill fix (DEREM-1) is solid with good test coverage.
1 P3, 1 Nit (both from Netero first pass).
The DEREM-3/DEREM-8 consolidation caught the copies in sources.ts and instructionsTabFocus.ts, but uiLayouts.ts retains its own identical private copies of the same helpers. Same class of improvement the author already agreed to do.
"The design is sound. The implicit assumption this PR eliminated (that the target workspace's RightSidebar is mounted when the goal pill fires) is replaced by a deterministic pre-seeding of localStorage before navigation." (Meruem, R2)
🤖 This review was automatically generated with Coder Agents.
ThomasK33
force-pushed
the
target-interaction-6wbb
branch
from
7d1b087 to
ddb7d0e
Compare
|
/coder-agents-review |
|
@codex review Please take another look after the uiLayouts shared-helper follow-up. |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
All 10 findings resolved across 4 rounds. No new findings from Netero or the panel.
The consolidation arc (DEREM-3, DEREM-8, DEREM-9) landed well: rightSidebarTabFocus.ts is now the single source of truth for sidebar layout helpers, with commands/sources.ts, instructionsTabFocus.ts, and uiLayouts.ts all importing from it. The uiLayouts.ts cleanup also corrected two hardcoded "costs" fallbacks to use the dynamic getRightSidebarTabFallback(), respecting the user's persisted tab preference.
The core fix is sound: replacing a timing-dependent custom event with deterministic localStorage pre-seeding before navigation. Test coverage is thorough.
"Each assertion proves a distinct side effect of the production click handler. Not fake, not fragile, not unrunnable." (Bisky)
🤖 This review was automatically generated with Coder Agents.
1 participant
Summary
Clicking a workspace target/goal pill now opens that workspace and primes its right sidebar to the Goal tab, instead of dispatching an event that is ignored when the workspace is not already selected.
Background
The target pill looked interactive but could be a no-op for inactive workspaces because only the selected workspace's RightSidebar listens for the open-goal event. Persisting the Goal tab selection before workspace navigation makes the destination deterministic even before the target workspace shell mounts.
Implementation
The AgentListItem goal pill now persists the target workspace's right-sidebar layout with the Goal tab selected, then reuses the existing workspace selection flow. A focused component test verifies both the navigation callback and persisted tab state.
Validation
bun test src/browser/components/AgentListItem/AgentListItem.test.tsxmake typecheckmake fmt-checkmake lintmake static-checkagent-browseragainst Storybook: opened the goal-pill story, clicked the target pill, and verified localStorage selected the Goal tab for the target workspace.Risks
Low. The change is scoped to the AgentListItem goal pill click path and uses the existing right-sidebar layout parser/selector helpers.
Generated with
mux• Model:openai:gpt-5.5• Thinking:high• Cost:$23.18