Pull request overview

Refactors the Agents Settings UX by splitting the former monolithic “Behavior” settings into dedicated routes and introducing a nested admin-only settings sub-panel, reducing unnecessary admin-scoped queries for non-admin users.

Changes:

• Split Behavior into dedicated destinations (General, Compaction, Instructions, Experiments, Lifecycle) and added backward-compatible redirects.
• Reworked Agents sidebar navigation into a two-level settings drill-down with an admin sub-panel.
• Updated Storybook interaction coverage and renamed “API Keys” UI copy to “Secrets (API keys)”.

Reviewed changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Clean refactor that breaks a monolithic settings page into focused destinations with proper permission gating. The drill-down sidebar and RequirePermission wrappers look well-structured.

This is a first-pass review only: these are mechanical findings from Netero. The full review panel has not yet reviewed this PR. The panel will review after these findings are addressed.

1 P2, 2 P3.

"Existing bookmarks, shared links, and browser history entries pointing to /agents/settings/behavior will render a blank settings outlet with no active sidebar item." (Netero)

🤖 This review was automatically generated with Coder Agents.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8a7527a51d

The split from a monolithic Behavior page into five focused destinations is well-motivated: non-admins no longer fire deployment-scoped queries, and the authorization boundary moved from component-level conditional rendering to route-level RequirePermission wrappers. The sidebar drill-down is clean on desktop. Story migration was systematic; every old story was accounted for, and new coverage (compaction threshold stories, user prompt save flow) was added beyond what was asked.

DEREM-1 (redirect for removed /behavior route): the panel unanimously closed this (18/18 accept). The maintainer explicitly accepted this breakage for an unstable feature.

2 P2, 7 P3, 1 P4, 3 Nit.

The two P2s are the mobile admin navigation regression (the drill-down sidebar architecture makes admin pages unreachable on mobile viewports) and the Lifecycle stories testing only half the page (RetentionPeriodSettings has zero play-function coverage).

"Before this PR, all admin items were flat siblings in the settings nav. Mobile admins could reach any admin page by tapping it from the settings index. After this PR, the only admin page reachable on mobile via the UI is the one 'Manage Agents' hardcodes." (Hisoka)

site/src/pages/AgentsPage/components/Sidebar/AgentsSidebar.tsx:136

P3 [DEREM-11] sidebarViewFromPath is a pure function that now drives panel selection for three consumers. It has a Set.has() check, a nullable coalescing fallback, and a regex, but no unit tests. The project convention reserves vitest for pure logic. The existing test file (AgentsSidebar.test.tsx) does not cover it.

The function is exercised through the SettingsViewResets integration story, which provides adequate end-to-end coverage. Dedicated unit tests would be faster to run and protect against regressions in the classification logic if the set of admin sections grows.

(Meruem P3, Mafuuu, Bisky)

🤖

site/src/pages/AgentsPage/AgentsPageView.stories.tsx:664

P3 [DEREM-14] OpensSettingsForNonAdmins verifies the General page renders for non-admins but never asserts that admin-only sidebar items are absent. If the {isAdmin && ...} guard on the "Manage Agents" nav item were accidentally removed, this test would still pass. Adding expect(screen.queryByRole("link", { name: "Manage Agents" })).not.toBeInTheDocument() would guard the admin/non-admin boundary.

(Kite)

🤖

🤖 This review was automatically generated with Coder Agents.

DEREM-4 (mobile admin navigation) was addressed in 7743b8c, nice fix with the dedicated /agents/settings/admin index URL.

Further review is blocked until the remaining 12 open findings receive a response (fix, acknowledgment, or contest). The panel cannot re-review while prior findings are silent.

Open findings awaiting response:

• DEREM-5 (P2) RetentionPeriodSettings has zero play-function coverage in Lifecycle stories
• DEREM-6 (P3) isSettingsPanel check duplicated in three files
• DEREM-7 (P3) ADMIN_SETTINGS_SECTIONS is a second source of truth
• DEREM-8 (P3) Non-admin on admin URL sees misleading sidebar state
• DEREM-9 (P3) Plan mode instructions save flow untested
• DEREM-10 (P3) ChatFullWidthSettings untested in General page
• DEREM-11 (P3) sidebarViewFromPath has no unit tests
• DEREM-12 (Nit) "MCP servers" title-case inconsistency
• DEREM-14 (P3) Non-admin story missing negative assertion
• DEREM-15 (P4) "Manage Agents" active prop is dead code
• DEREM-16 (Nit) File name SystemInstructions doesn't match route slug
• DEREM-17 (Nit) UsersIcon for personal-settings page

A reply on each thread (even "acknowledged, won't fix" or "deferred to follow-up with ticket") unblocks the next round.

🤖 This review was automatically generated with Coder Agents.

Great progress. 10 of 12 open findings addressed or acknowledged across 5175fa0 and 9961e42. The isSettingsView helper, retention stories, plan-mode story, non-admin fallback fix, file renames, and icon/label corrections all look clean.

DEREM-7 (ADMIN_SETTINGS_SECTIONS coupling): author acknowledged, bounded risk, follow-up offered. Accepted.

Two findings remain without a response. These were folded into the review body (not inline threads) because they fell outside the diff hunks, so they may have been easy to miss:

• DEREM-11 (P3) sidebarViewFromPath has new branching logic with no unit tests. The function now handles "admin" as a special section, user/admin panel classification via ADMIN_SETTINGS_SECTIONS, and the isSettingsView guard. A vitest file covering the key cases would be fast to add.
• DEREM-14 (P3) OpensSettingsForNonAdmins story is missing a negative assertion for admin sidebar items. Adding expect(screen.queryByRole("link", { name: "Manage Agents" })).not.toBeInTheDocument() guards the admin/non-admin boundary.

A reply on each (fix, acknowledge, or contest) unblocks the next round.

🤖 This review was automatically generated with Coder Agents.

All prior findings addressed or acknowledged. The fix commits are clean: isSettingsView type guard extracted, retention and plan-mode stories added, sidebarViewFromPath unit-tested with 17 cases, non-admin fallback fixed, file renames applied, icon and label corrections landed. Mafu-san noted that the correction arc across rounds shows systematic, non-regressing work.

2 P3.

"Pure-logic unit test for sidebarViewFromPath and isSettingsView. 17 cases, all passing, no mocks, no DOM, no fragility." (Bisky)

site/src/pages/AgentsPage/AgentSettingsPage.tsx:17

P3 [DEREM-19] mobileBack always targets /agents/settings regardless of whether the current section is user-level or admin-level. On mobile, an admin on /agents/settings/providers who taps the "Settings" back link lands on the user settings sidebar (General, Compaction, Secrets, Manage Agents), not the admin sidebar they came from. Returning to the admin context requires two extra taps.

Fix: detect admin sections and route to /agents/settings/admin:

const sidebarView = sidebarViewFromPath(location.pathname);
const mobileBack = section
    ? sidebarView.panel === "settings-admin"
        ? { to: "/agents/settings/admin", label: "Manage Agents" }
        : { to: "/agents/settings", label: "Settings" }
    : undefined;

(Meruem)

🤖

🤖 This review was automatically generated with Coder Agents.

No new commits since R5. Both open findings are still awaiting a response:

• DEREM-18 (P3) Retention toggle-off, save-error, and load-error story coverage. Inline thread on AgentSettingsLifecyclePageView.stories.tsx:236.
• DEREM-19 (P3) Mobile back navigation from admin content pages skips admin sidebar. In the R5 review body (AgentSettingsPage.tsx:17).

A reply on each (fix, acknowledge, or contest) unblocks the next round.

🤖 This review was automatically generated with Coder Agents.

All 19 findings across 7 rounds are closed: 15 fixed in code, 1 acknowledged with bounded-risk defense (DEREM-7, follow-up offered), 1 panel-closed (DEREM-1, 18/18 accept), 1 dropped by orchestrator (DEREM-13, pre-existing gap). Netero R7 and the R7 panel (Bisky, Mafu-san, Mafuuu) found no new issues.

The 24620b1 fix correctly addresses both R5 findings: mobile back navigation now routes to /agents/settings/admin for admin sections, and retention stories have full parity with autostop coverage.

PS. The PR description icon table still says General uses UsersIcon; the code now uses UserIcon (fixed in DEREM-17). Worth updating the description before merge. (Mafu-san)

🤖 This review was automatically generated with Coder Agents.