Skip to content

Commit ebd2767

Browse files
waleedlatif1waleedlatif1
committed
improvement(settings): fix header regressions found in review
Six independent review passes over the diff. Three real defects, all introduced by the header-meta fallback or the loading boundary. - A denied organization section rendered the section's catalog heading, description and Docs link above a "you do not have access" body, because SettingsUnavailable renders its own centred heading and registers nothing. It now claims an empty header, which is how a body opts out of the meta fallback. Releasing the header is an explicit null rather than an EMPTY_CONFIG sentinel, so "no body owns this" is stated instead of implied. - The account credit-usage route resolves to its parent billing section, so the shell painted "Billing" in the server frame before hydration swapped in "Credit usage". The shell now only supplies meta for a section's own route, not for detail routes beneath it. - Adding loading.tsx put the page inside a Suspense boundary, where notFound() and redirect() can no longer set the response status: a legacy or unknown settings URL loaded directly answered 200 and redirected in a second round trip instead of 307/404. Segment-level routing moved into the layout, above the boundary, which is also where it belonged. Also from review: - Parallelize two pairs of independent awaits in the access gate. Every await there sits in front of the section body, so this shortens the exact wait the PR is about. - Note in settings-header that the layout effect is load-bearing: a passive effect would let the previous section's title show for a frame. - Correct the loading.tsx docs, which claimed the empty body matched every other route-level fallback. It is the only null one; the accurate statement is that the shell above it already renders the chrome. - Tests: cover resolveSettingsSection's alias table (previously untested on either side of the move) and the general-settings prefetch gate. Strengthen the wholesale-substitution test, which passed against a field-merge implementation because SettingsPanel always emits a description key. All four verified against mutants.
1 parent e012b7a commit ebd2767

13 files changed

Lines changed: 298 additions & 105 deletions

File tree

‎apps/sim/app/account/settings/[section]/loading.tsx‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,18 @@
11
/**
2-
* Route-transition fallback for a account settings section.
2+
* Route-transition fallback for the account settings sections.
33
*
4-
* Without a loading boundary the App Router holds the outgoing section on screen until the
5-
* incoming page's access gate resolves, so a click reads as a dead click. This commits the
6-
* navigation immediately — the URL changes and the shell's heading updates with it — and
7-
* lets the gate resolve behind an empty body.
4+
* Its job is to exist. Without a loading boundary the App Router holds the outgoing section on
5+
* screen until the incoming page's access gate resolves, so a click reads as a dead click; with
6+
* one, the navigation commits immediately and the heading changes with it. It is also what
7+
* makes the sidebar's `router.prefetch` worth anything — with no loading boundary in the
8+
* subtree the scheduler skips the segment request entirely, and an `AUTO` prefetch caches the
9+
* shell only as far as the nearest boundary.
810
*
9-
* The body is empty rather than a skeleton, matching every other route-level fallback in the
10-
* app: `ResourceChromeFallback` renders its real header and column headers over zero rows,
11-
* and the credit-usage fallback renders its real title and description over nothing. The
12-
* chrome is what signals arrival; placeholder rows would only add a shape that no section
13-
* actually has, and a layout shift when the real body replaces it.
11+
* It renders no body of its own because the shell that owns the header, heading and scroll
12+
* region renders above it and is already resolved by this point. That lands in the same place
13+
* as the two neighbouring settings fallbacks — credit-usage renders its title and description
14+
* over an empty body, `ResourceChromeFallback` renders a real header and column headers over
15+
* zero rows — without restating chrome this route already has.
1416
*/
1517
export default function AccountSettingsSectionLoading() {
1618
return null

‎apps/sim/app/organization/[organizationId]/settings/[section]/loading.tsx‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,18 @@
11
/**
2-
* Route-transition fallback for a organization settings section.
2+
* Route-transition fallback for the organization settings sections.
33
*
4-
* Without a loading boundary the App Router holds the outgoing section on screen until the
5-
* incoming page's access gate resolves, so a click reads as a dead click. This commits the
6-
* navigation immediately — the URL changes and the shell's heading updates with it — and
7-
* lets the gate resolve behind an empty body.
4+
* Its job is to exist. Without a loading boundary the App Router holds the outgoing section on
5+
* screen until the incoming page's access gate resolves, so a click reads as a dead click; with
6+
* one, the navigation commits immediately and the heading changes with it. It is also what
7+
* makes the sidebar's `router.prefetch` worth anything — with no loading boundary in the
8+
* subtree the scheduler skips the segment request entirely, and an `AUTO` prefetch caches the
9+
* shell only as far as the nearest boundary.
810
*
9-
* The body is empty rather than a skeleton, matching every other route-level fallback in the
10-
* app: `ResourceChromeFallback` renders its real header and column headers over zero rows,
11-
* and the credit-usage fallback renders its real title and description over nothing. The
12-
* chrome is what signals arrival; placeholder rows would only add a shape that no section
13-
* actually has, and a layout shift when the real body replaces it.
11+
* It renders no body of its own because the shell that owns the header, heading and scroll
12+
* region renders above it and is already resolved by this point. That lands in the same place
13+
* as the two neighbouring settings fallbacks — credit-usage renders its title and description
14+
* over an empty body, `ResourceChromeFallback` renders a real header and column headers over
15+
* zero rows — without restating chrome this route already has.
1416
*/
1517
export default function OrganizationSettingsSectionLoading() {
1618
return null

‎apps/sim/app/selfhost/settings/[section]/loading.tsx‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,18 @@
11
/**
2-
* Route-transition fallback for a self-hosted settings section.
2+
* Route-transition fallback for the self-host settings sections.
33
*
4-
* Without a loading boundary the App Router holds the outgoing section on screen until the
5-
* incoming page's access gate resolves, so a click reads as a dead click. This commits the
6-
* navigation immediately — the URL changes and the shell's heading updates with it — and
7-
* lets the gate resolve behind an empty body.
4+
* Its job is to exist. Without a loading boundary the App Router holds the outgoing section on
5+
* screen until the incoming page's access gate resolves, so a click reads as a dead click; with
6+
* one, the navigation commits immediately and the heading changes with it. It is also what
7+
* makes the sidebar's `router.prefetch` worth anything — with no loading boundary in the
8+
* subtree the scheduler skips the segment request entirely, and an `AUTO` prefetch caches the
9+
* shell only as far as the nearest boundary.
810
*
9-
* The body is empty rather than a skeleton, matching every other route-level fallback in the
10-
* app: `ResourceChromeFallback` renders its real header and column headers over zero rows,
11-
* and the credit-usage fallback renders its real title and description over nothing. The
12-
* chrome is what signals arrival; placeholder rows would only add a shape that no section
13-
* actually has, and a layout shift when the real body replaces it.
11+
* It renders no body of its own because the shell that owns the header, heading and scroll
12+
* region renders above it and is already resolved by this point. That lands in the same place
13+
* as the two neighbouring settings fallbacks — credit-usage renders its title and description
14+
* over an empty body, `ResourceChromeFallback` renders a real header and column headers over
15+
* zero rows — without restating chrome this route already has.
1416
*/
1517
export default function SelfHostSettingsSectionLoading() {
1618
return null

‎apps/sim/app/workspace/[workspaceId]/settings/[section]/layout.tsx‎

Lines changed: 37 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,32 +1,58 @@
1+
import { notFound, redirect } from 'next/navigation'
12
import {
23
SettingsHeaderProvider,
34
SettingsHeaderShell,
45
} from '@/app/workspace/[workspaceId]/settings/components/settings-header/settings-header'
56
import { resolveSettingsSection } from '@/app/workspace/[workspaceId]/settings/navigation'
67

78
/**
8-
* Persistent chrome for the settings panel pages. The header bar, title,
9-
* description, scroll region, and centered column live in the shell and stay
10-
* mounted across section navigation — only the body swaps. Scoped to `[section]`
11-
* so detail routes (e.g. `secrets/[credentialId]`) keep their own chrome.
9+
* Sections that were promoted out of settings into their own workspace routes. Kept as
10+
* segment-level rewrites so old links and bookmarks still land somewhere sensible.
11+
*/
12+
const TOP_LEVEL_REDIRECTS: Readonly<Record<string, (workspaceId: string) => string>> = {
13+
integrations: (workspaceId) => `/workspace/${workspaceId}/integrations`,
14+
skills: (workspaceId) => `/workspace/${workspaceId}/skills`,
15+
/** Cookie preferences moved into General. */
16+
privacy: (workspaceId) => `/workspace/${workspaceId}/settings/general?view=privacy`,
17+
}
18+
19+
/**
20+
* Persistent chrome for the settings panel pages: the header bar, title, description, scroll
21+
* region and centered column. Scoped to `[section]` so detail routes (e.g.
22+
* `secrets/[credentialId]`) keep their own chrome.
1223
*
13-
* The heading is resolved here rather than pushed up from the section body, so it
14-
* paints with the shell instead of waiting on the body's lazily-loaded chunk. An
15-
* unknown segment resolves to `null` and the page below it calls `notFound()`.
24+
* The heading is resolved here rather than pushed up from the section body, so it renders with
25+
* the shell instead of waiting on the body's lazily-loaded chunk.
26+
*
27+
* Whether a segment names a section at all is decided here too, above the sibling
28+
* `loading.tsx`. Inside that Suspense boundary a `notFound()` or `redirect()` can no longer set
29+
* the response status — React replays the boundary on the client and the shell still flushes
30+
* 200 — so a bad or legacy URL loaded directly would answer 200 and redirect in a second round
31+
* trip. Deciding it above the boundary keeps the 404 and the 307. Whether the *viewer* may open
32+
* a section is a different question and stays in the page, where it belongs; those checks need
33+
* the database and are reached almost entirely by client navigation.
34+
*
35+
* Authentication is already enforced by the ancestor workspace layout, so this runs only for a
36+
* signed-in viewer.
1637
*/
1738
export default async function SettingsSectionLayout({
1839
children,
1940
params,
2041
}: {
2142
children: React.ReactNode
22-
params: Promise<{ section: string }>
43+
params: Promise<{ workspaceId: string; section: string }>
2344
}) {
24-
const { section } = await params
25-
const meta = resolveSettingsSection(section)?.meta ?? null
45+
const { workspaceId, section } = await params
46+
47+
const topLevelHref = TOP_LEVEL_REDIRECTS[section]?.(workspaceId)
48+
if (topLevelHref) redirect(topLevelHref)
49+
50+
const resolved = resolveSettingsSection(section)
51+
if (!resolved) notFound()
2652

2753
return (
2854
<SettingsHeaderProvider>
29-
<SettingsHeaderShell meta={meta}>{children}</SettingsHeaderShell>
55+
<SettingsHeaderShell meta={resolved.meta}>{children}</SettingsHeaderShell>
3056
</SettingsHeaderProvider>
3157
)
3258
}

‎apps/sim/app/workspace/[workspaceId]/settings/[section]/loading.tsx‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,18 @@
11
/**
2-
* Route-transition fallback for a workspace settings section.
2+
* Route-transition fallback for the workspace settings sections.
33
*
4-
* Without a loading boundary the App Router holds the outgoing section on screen until the
5-
* incoming page's access gate resolves, so a click reads as a dead click. This commits the
6-
* navigation immediately — the URL changes and the shell's heading updates with it — and
7-
* lets the gate resolve behind an empty body.
4+
* Its job is to exist. Without a loading boundary the App Router holds the outgoing section on
5+
* screen until the incoming page's access gate resolves, so a click reads as a dead click; with
6+
* one, the navigation commits immediately and the heading changes with it. It is also what
7+
* makes the sidebar's `router.prefetch` worth anything — with no loading boundary in the
8+
* subtree the scheduler skips the segment request entirely, and an `AUTO` prefetch caches the
9+
* shell only as far as the nearest boundary.
810
*
9-
* The body is empty rather than a skeleton, matching every other route-level fallback in the
10-
* app: `ResourceChromeFallback` renders its real header and column headers over zero rows,
11-
* and the credit-usage fallback renders its real title and description over nothing. The
12-
* chrome is what signals arrival; placeholder rows would only add a shape that no section
13-
* actually has, and a layout shift when the real body replaces it.
11+
* It renders no body of its own because the shell that owns the header, heading and scroll
12+
* region renders above it and is already resolved by this point. That lands in the same place
13+
* as the two neighbouring settings fallbacks — credit-usage renders its title and description
14+
* over an empty body, `ResourceChromeFallback` renders a real header and column headers over
15+
* zero rows — without restating chrome this route already has.
1416
*/
1517
export default function WorkspaceSettingsSectionLoading() {
1618
return null

‎apps/sim/app/workspace/[workspaceId]/settings/[section]/page.test.tsx‎

Lines changed: 42 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -84,17 +84,28 @@ vi.mock('@/lib/workspaces/host-context', () => ({
8484
}))
8585

8686
vi.mock('@/app/_shell/providers/get-query-client', () => ({
87-
getQueryClient: vi.fn(),
87+
getQueryClient: mockGetQueryClient,
8888
}))
8989

90-
const { mockSections } = vi.hoisted(() => ({
91-
mockSections: ['general', 'billing', 'secrets', 'sessions'],
90+
const { mockGetQueryClient, mockPrefetchGeneralSettings } = vi.hoisted(() => ({
91+
mockGetQueryClient: vi.fn(),
92+
mockPrefetchGeneralSettings: vi.fn(),
93+
}))
94+
95+
const { mockSections, mockAliases } = vi.hoisted(() => ({
96+
mockSections: ['general', 'billing', 'secrets', 'sessions', 'admin'],
97+
/** Mirrors the real alias table so a legacy segment behaves here as it does in production. */
98+
mockAliases: {
99+
subscription: 'billing',
100+
team: 'organization',
101+
'api-keys': 'apikeys',
102+
domains: 'sso',
103+
} as Record<string, string>,
92104
}))
93105

94106
vi.mock('@/app/workspace/[workspaceId]/settings/navigation', () => ({
95-
allNavigationItems: mockSections.map((id) => ({ id })),
96107
resolveSettingsSection: vi.fn((section: string) => {
97-
const id = section === 'subscription' ? 'billing' : section
108+
const id = mockAliases[section] ?? section
98109
return mockSections.includes(id) ? { id, meta: { title: id } } : null
99110
}),
100111
getSettingsSectionMeta: vi.fn(() => null),
@@ -109,13 +120,14 @@ vi.mock('@/ee/workspace-forking/lib/lineage/authz', () => ({
109120
}))
110121

111122
vi.mock('@/app/workspace/[workspaceId]/settings/[section]/prefetch', () => ({
112-
prefetchGeneralSettings: vi.fn(),
123+
prefetchGeneralSettings: mockPrefetchGeneralSettings,
113124
}))
114125

115126
vi.mock('@/app/workspace/[workspaceId]/settings/[section]/settings', () => ({
116127
SettingsPage: vi.fn(() => null),
117128
}))
118129

130+
import { QueryClient } from '@tanstack/react-query'
119131
import WorkspaceSettingsSectionPage from '@/app/workspace/[workspaceId]/settings/[section]/page'
120132

121133
const PERSONAL_HOST_CONTEXT = {
@@ -152,6 +164,7 @@ describe('WorkspaceSettingsSectionPage unavailable sections', () => {
152164
mockCanOpenOrganizationSettingsSection.mockResolvedValue(false)
153165
mockIsOrganizationOnEnterprisePlan.mockResolvedValue(false)
154166
mockIsOrganizationSettingsSectionAvailable.mockReturnValue(true)
167+
mockGetQueryClient.mockReturnValue(new QueryClient())
155168
})
156169

157170
it('redirects an unavailable subscription section to General', async () => {
@@ -179,6 +192,29 @@ describe('WorkspaceSettingsSectionPage unavailable sections', () => {
179192
expect(mockGetWorkspaceHostContext).not.toHaveBeenCalled()
180193
})
181194

195+
it('hydrates general settings only for the sections whose body reads them', async () => {
196+
// The saving this gate exists for: the other ~25 sections no longer block on a query they
197+
// never touch. `general` still does, and so does an alias that resolves onto the set.
198+
mockResolveWorkspaceNavigation.mockReturnValue([{ id: 'secrets' }])
199+
200+
await WorkspaceSettingsSectionPage(pageProps('general'))
201+
expect(mockPrefetchGeneralSettings).toHaveBeenCalledTimes(1)
202+
203+
mockPrefetchGeneralSettings.mockClear()
204+
await WorkspaceSettingsSectionPage(pageProps('secrets'))
205+
expect(mockPrefetchGeneralSettings).not.toHaveBeenCalled()
206+
})
207+
208+
it('gates the hydration on the resolved section, not the raw segment', async () => {
209+
// `/settings/subscription` is a legacy link for billing, which does read the key. Billing on
210+
// a personal workspace is only reachable by the billed account owner.
211+
mockGetSession.mockResolvedValue({ user: { id: 'owner-b' } })
212+
213+
await WorkspaceSettingsSectionPage(pageProps('subscription'))
214+
215+
expect(mockPrefetchGeneralSettings).toHaveBeenCalledTimes(1)
216+
})
217+
182218
it('keeps inaccessible workspaces fail-fast', async () => {
183219
mockGetWorkspaceHostContext.mockResolvedValue(null)
184220

‎apps/sim/app/workspace/[workspaceId]/settings/[section]/page.tsx‎

Lines changed: 28 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,6 @@ interface WorkspaceSettingsSectionPageProps {
3232
params: Promise<{ workspaceId: string; section: string }>
3333
}
3434

35-
const TOP_LEVEL_REDIRECTS: Readonly<Record<string, (workspaceId: string) => string>> = {
36-
integrations: (workspaceId) => `/workspace/${workspaceId}/integrations`,
37-
skills: (workspaceId) => `/workspace/${workspaceId}/skills`,
38-
// Cookie preferences moved into General; keep old links working.
39-
privacy: (workspaceId) => `/workspace/${workspaceId}/settings/general?view=privacy`,
40-
}
41-
4235
const WORKSPACE_SECTION_MAP: Partial<Record<SettingsSection, WorkspaceSettingsSection>> = {
4336
teammates: 'teammates',
4437
secrets: 'secrets',
@@ -104,18 +97,23 @@ export default async function WorkspaceSettingsSectionPage({
10497
if (!session?.user) redirect('/login')
10598

10699
const { workspaceId, section } = await params
107-
const topLevelHref = TOP_LEVEL_REDIRECTS[section]?.(workspaceId)
108-
if (topLevelHref) redirect(topLevelHref)
100+
/** The layout already rejected an unknown segment; this narrows the type and fails safe. */
109101
const resolved = resolveSettingsSection(section)
110102
if (!resolved) notFound()
111103
const parsed = resolved.id
112104

113-
const hostContext = await getWorkspaceHostContextForViewer(workspaceId, session.user.id)
105+
/**
106+
* Independent given the session, and both gate the same render, so they overlap rather than
107+
* queue. Every await here sits in front of the section's body, so it is the length of this
108+
* chain that the user waits out.
109+
*/
110+
const requiresPlatformAdmin = parsed === 'admin' || parsed === 'mothership'
111+
const [hostContext, isViewerPlatformAdmin] = await Promise.all([
112+
getWorkspaceHostContextForViewer(workspaceId, session.user.id),
113+
requiresPlatformAdmin ? isPlatformAdmin(session.user.id) : Promise.resolve(false),
114+
])
114115
if (!hostContext) notFound()
115-
116-
if (parsed === 'admin' || parsed === 'mothership') {
117-
if (!(await isPlatformAdmin(session.user.id))) notFound()
118-
}
116+
if (requiresPlatformAdmin && !isViewerPlatformAdmin) notFound()
119117

120118
const workspaceSection = WORKSPACE_SECTION_MAP[parsed]
121119
if (workspaceSection) {
@@ -162,23 +160,30 @@ export default async function WorkspaceSettingsSectionPage({
162160
if (!hostContext.viewer.isHostOrganizationAdmin) {
163161
redirectToGeneralSettings(workspaceId)
164162
}
165-
if (
166-
!(await canOpenOrganizationSettingsSection(
163+
/**
164+
* Overlapped for the same reason: neither reads the other's result. The plan lookup is
165+
* skipped for the two sections that do not gate on it, so the only case that pays for a
166+
* lookup it does not use is one that was about to redirect anyway.
167+
*/
168+
const needsEnterprisePlan =
169+
organizationSection !== 'members' && organizationSection !== 'billing'
170+
const [canOpenSection, isEnterpriseOrganization] = await Promise.all([
171+
canOpenOrganizationSettingsSection(
167172
hostContext.hostOrganizationId,
168173
session.user.id,
169174
organizationSection
170-
))
171-
) {
175+
),
176+
needsEnterprisePlan
177+
? isOrganizationOnEnterprisePlan(hostContext.hostOrganizationId)
178+
: Promise.resolve(false),
179+
])
180+
if (!canOpenSection) {
172181
redirectToGeneralSettings(workspaceId)
173182
}
174-
const hasEnterprisePlan =
175-
organizationSection !== 'members' &&
176-
organizationSection !== 'billing' &&
177-
(await isOrganizationOnEnterprisePlan(hostContext.hostOrganizationId))
178183
if (
179184
!isOrganizationSettingsSectionAvailable(
180185
organizationSection,
181-
getOrganizationSettingsFeatures(hasEnterprisePlan)
186+
getOrganizationSettingsFeatures(needsEnterprisePlan && isEnterpriseOrganization)
182187
)
183188
) {
184189
redirectToGeneralSettings(workspaceId)

0 commit comments

Comments
 (0)