fix(client): guard the settings binding against hostile values and dead scopes
Three review passes (standards, contract, over-engineering) over the whole settings surface left four follow-ups worth taking: - sync() wraps its snapshot parsing in the compatibility.md parser gate's bounded catch: a served value that throws on property access keeps the current state instead of throwing into the mirror's fan-out, with the mandated hostile fixture beside the happy path. - the attach disposer now clears the scope, so a set() after disposal never writes into a dead form; a superseded attach keeps the newer scope bound. - dead surface trimmed: six type re-exports nothing imported through the module, a single-caller query helper, and an orphaned export on the settings-card props interface.
This commit is contained in:
1 parent
fd782119e8
commit
f0b0e12e58
4 files changed
+60
-12
No files matched your search
@@ -7,7 +7,7 @@ import { IconChevronDown } from '../primitives'
|
||||
import type { SettingsField, SettingsState } from '../settings'
|
||||
import type { ViewKit } from '../viewkit'
|
||||
|
||||
export interface SettingsCardProps {
|
||||
interface SettingsCardProps {
|
||||
useContextSettings?: <T>(selector: (state: SettingsState) => T) => T
|
||||
set?: (field: SettingsField, value: string) => void
|
||||
}
|
||||
|
||||
+15
-6
@@ -3,7 +3,7 @@
|
||||
|
||||
import type { DefaultDeltaBase, DefaultFileSort, DefaultGranularity, DefaultDurationCurve, DefaultPlacement, DefaultToolSort, DefaultTrendMode, FleetTab, InsightsEntry, SettingsField } from '../shared/types'
|
||||
|
||||
export type { DefaultDeltaBase, DefaultFileSort, DefaultGranularity, DefaultDurationCurve, DefaultPlacement, DefaultToolSort, DefaultTrendMode, FleetTab, InsightsEntry, SettingsField } from '../shared/types'
|
||||
export type { DefaultDeltaBase, DefaultFileSort, DefaultToolSort, SettingsField } from '../shared/types'
|
||||
|
||||
/** The bound settings form (`ctx.configForms.get`), as consumed. */
|
||||
export interface SettingsScopeLike {
|
||||
@@ -100,12 +100,16 @@ export function createContextSettings(): ContextSettings {
|
||||
}
|
||||
const sync = (bound: SettingsScopeLike): { placement?: DefaultPlacement; insightsEntry?: InsightsEntry; fleetTab?: FleetTab } => {
|
||||
const snap = bound.getSnapshot()
|
||||
const prefs = prefsOf(snap.value)
|
||||
// Bounded catch (the compatibility.md parser gate): a value that throws on property access
|
||||
// keeps the current state instead of throwing into the mirror's fan-out.
|
||||
let prefs: Prefs = {}
|
||||
let raw: Record<string, unknown> | undefined
|
||||
try {
|
||||
prefs = prefsOf(snap.value)
|
||||
raw = snap.value !== null && typeof snap.value === 'object' ? snap.value as Record<string, unknown> : undefined
|
||||
} catch { /* hostile value */ }
|
||||
// Fail open: a config problem must never leave an entry hidden. A value the plugin cannot understand
|
||||
// degrades to the field's default; a section without the field (older Host half) keeps the current state.
|
||||
const raw = snap.value !== null && typeof snap.value === 'object'
|
||||
? snap.value as Record<string, unknown>
|
||||
: undefined
|
||||
publish({
|
||||
status: snap.status === 'ready' || snap.status === 'unavailable' ? snap.status : 'loading',
|
||||
placement: prefs.placement ?? (raw?.defaultPlacement === undefined ? state.placement : 'all'),
|
||||
@@ -143,7 +147,12 @@ export function createContextSettings(): ContextSettings {
|
||||
attach(bound) {
|
||||
scope = bound
|
||||
sync(bound)
|
||||
return bound.subscribe(() => { sync(bound) })
|
||||
const unsubscribe = bound.subscribe(() => { sync(bound) })
|
||||
return () => {
|
||||
unsubscribe()
|
||||
// A set() after disposal must not write into a dead form.
|
||||
if (scope === bound) scope = undefined
|
||||
}
|
||||
},
|
||||
set(field, value) {
|
||||
publish({ ...state, ...prefsOf({ [field]: value }) })
|
||||
|
||||
@@ -11,12 +11,9 @@ const PACKAGE_KEY = 'dsh-context'
|
||||
/** 40 ticks at the 100ms tick: enough for the page's package load. */
|
||||
const POLL_TICKS = 40
|
||||
|
||||
function buttonsOf(doc: Document): HTMLButtonElement[] {
|
||||
return [...doc.querySelectorAll<HTMLButtonElement>('button')]
|
||||
}
|
||||
|
||||
function findPanelEntry(doc: Document): HTMLButtonElement | undefined {
|
||||
return buttonsOf(doc).find(b => PLUGIN_LABELS.has(b.getAttribute('aria-label') ?? ''))
|
||||
return [...doc.querySelectorAll<HTMLButtonElement>('button')]
|
||||
.find(b => PLUGIN_LABELS.has(b.getAttribute('aria-label') ?? ''))
|
||||
}
|
||||
|
||||
/** The bundle card's open control: the one button inside the card that is not the enable switch (the
|
||||
|
||||
@@ -438,4 +438,46 @@ describe('attach', () => {
|
||||
assert.equal(calls, 0)
|
||||
assert.equal(s.defaultGranularity(), 'step')
|
||||
})
|
||||
|
||||
test('a set after the disposer runs never reaches the dead scope', () => {
|
||||
const s = createContextSettings()
|
||||
const scope = new TestSettingsScope({ status: 'ready', value: {}, writable: true })
|
||||
const detach = s.attach(scope)
|
||||
detach()
|
||||
s.set('defaultTrendMode', 'delta')
|
||||
assert.deepEqual(scope.sets, [])
|
||||
})
|
||||
|
||||
test('disposing a superseded attach keeps the newer scope bound', () => {
|
||||
const s = createContextSettings()
|
||||
const first = new TestSettingsScope({ status: 'ready', value: {}, writable: true })
|
||||
const second = new TestSettingsScope({ status: 'ready', value: {}, writable: true })
|
||||
const detachFirst = s.attach(first)
|
||||
s.attach(second)
|
||||
detachFirst()
|
||||
s.set('defaultTrendMode', 'delta')
|
||||
assert.deepEqual(first.sets, [])
|
||||
assert.deepEqual(second.sets, [{ field: 'defaultTrendMode', value: 'delta' }])
|
||||
})
|
||||
|
||||
test('a snapshot value that throws on property access keeps the current state', () => {
|
||||
const s = createContextSettings()
|
||||
const hostile = new Proxy({}, { get() { throw new Error('hostile') } })
|
||||
s.attach(new TestSettingsScope({ status: 'ready', value: hostile, writable: true }))
|
||||
assert.deepEqual(s.store.getSnapshot(), {
|
||||
status: 'ready',
|
||||
placement: 'all',
|
||||
granularity: 'step',
|
||||
mode: 'total',
|
||||
deltaBase: 'step',
|
||||
toolSort: 'count',
|
||||
fileSort: 'count',
|
||||
insightsEntry: 'show',
|
||||
durationCurve: 'show',
|
||||
fleetTab: 'show',
|
||||
promptPrefix: '',
|
||||
promptSuffix: '',
|
||||
writable: true,
|
||||
})
|
||||
})
|
||||
})
|
||||
Reference in new issue
Block a user