Conversation
Split the config page into two regions: core settings (as before) and a new "App settings" region for app-owned settings groups, labeling each with its owning app. App-owned groups whose owning app is disabled or not enabled are hidden entirely, and the App settings region (and its header) is omitted when no enabled app-owned groups remain. Search and filtering continue to apply across both regions. Disabled apps' setting classes are also excluded from the Class filter dropdown so it never offers a class that dead-ends in the empty state (addresses review feedback). Consumes the app metadata now carried on each SettingClassGroup in the settings LIST response (is_app_owned, app_id, app_display_name, app_enabled). Covered by component tests (region grouping, disabled-app hiding, core groups staying in the core region, empty-region omission, cross-region search, filtered dropdown) and a settings e2e test.
There was a problem hiding this comment.
Pull request overview
This PR updates the Settings UI to clearly separate core SEP settings from app-owned settings, hiding app-owned settings when their owning app is disabled/not enabled, and omitting the entire “App settings” section when nothing eligible remains.
Changes:
- Partition settings groups into core vs enabled app-owned groups, rendering app-owned groups under a dedicated “App settings” region.
- Add an optional per-app label chip on app-owned settings groups (using LIST-response app metadata).
- Extend component + e2e coverage for grouping, disabled-app hiding, class-filter behavior, and cross-region search.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/packages/shell/src/pages/SettingsPage.tsx | Partitions groups into core vs enabled app-owned; renders “App settings” section conditionally; adjusts Class filter options to exclude disabled app-owned groups. |
| frontend/packages/shell/src/components/settings/SettingsGroup.tsx | Adds optional appLabel chip to tag app-owned setting groups with their owning app. |
| frontend/packages/shell/src/components/settings/filters.ts | Adds partitionSettingsGroups and appLabelFor utilities to support grouping + labeling. |
| frontend/packages/shell/src/components/settings/tests/SettingsPage.test.tsx | Adds component tests for the new regions, labeling, disabled-app hiding, Class filter behavior, and cross-region search. |
| frontend/packages/shell/src/components/settings/tests/fixtures.tsx | Adds fixtures for enabled vs disabled app-owned groups. |
| frontend/packages/shell/src/components/settings/tests/filters.test.ts | Adds unit tests for partitionSettingsGroups and appLabelFor. |
| frontend/packages/e2e/tests/settings.spec.ts | Adds e2e coverage for app-owned grouping, disabled-app hiding, and search behavior. |
- filters.ts: drop "already filtered" from partitionSettingsGroups docstring since it's also called with the unfiltered group list. - SettingsPage.test.tsx: note that the combined LIST response now includes app-owned groups alongside core SEP + Tasks groups.
yyyyyyyan
left a comment
yyyyyyyan
left a comment
There was a problem hiding this comment.
@nachodd — clean, tightly-scoped implementation. partitionSettingsGroups expresses the core-vs-app split in a single pass, and the coverage is thorough end-to-end: the component tests pin enabled-app rendering, core-stays-out, disabled-app hiding, the Class-filter exclusion, and region-omitted-when-empty, and the Playwright spec asserts the disabled app's rows never reach the DOM. Good call keeping appLabelFor's trailing ?? undefined — it coerces a null app_id into the string | undefined the appLabel && guard expects, so it isn't dead code even though it reads like it.
One small style nit inline, plus a non-blocking note below.
No changelog fragment — this adds a user-visible "App settings" section, and user-facing frontend changes here usually drop a fragment via make changelog-add TICKET=SEP-1489 SECTION=added MSG="...". Not blocking: the backend sibling SEP-1488 landed without one too, so the whole feature may be getting documented at release — noting it only so it doesn't slip.
Approving — the only requested code change is the one-line doc-comment suggestion.
- Drop 'as before' from PartitionedSettingsGroups.core docstring
- Document the new App settings section on the Settings page
|
Added the changelog fragment in e38af0e ( |
What & why
Settings page now shows two distinct regions so admins aren't shown config for apps they aren't running:
App-owned groups whose owning app is disabled or not enabled are hidden entirely, and the App settings region (and its header) is omitted when no enabled app-owned groups remain. Search and filtering continue to apply across both regions.
The page consumes the app metadata now carried on each
SettingClassGroupin the settings LIST response (is_app_owned,app_id,app_display_name,app_enabled) — the backend sibling story.Acceptance criteria covered
Changes
SettingsPage.tsx— partition groups into core vs enabled app-owned regions; render an "App settings" section; keep empty-state / filter behavior intact; exclude disabled classes from the Class filter.components/settings/filters.ts— addpartitionSettingsGroupsandappLabelFor.components/settings/SettingsGroup.tsx— optional per-app label chip (additive, backward-compatible).SettingsPage.test.tsx,filters.test.ts) + fixtures, and a settings e2e test.Test plan
pnpm --filter @sep/shell test— settings component tests pass (26 in the two touched files).pnpm --filter @sep/e2e test:e2e settings.spec.ts— 11 passed, including the new region/disabled-app test.pnpm --filter @sep/shell type-checkandoxlint— clean.Notes