mirror of
https://github.com/tiennm99/ccs.git
synced 2026-07-15 22:21:20 +00:00
fix(ui/design-system): address PR-Agent round 2 on PR #1109
Three more issues raised by upstream review after the v1.7 fix push.
Each is encoded both in code and in spec so future drift can't
reintroduce them:
1. Required `storageKey` was the wrong fix for cross-page state bleed.
Forcing every callsite to pass `storageKey` made future
production-page migrations a build-breaking event for no real
safety benefit. Better engineering: keep the prop optional and
default to a key derived from `window.location.pathname` so each
route gets its own localStorage slot automatically. Pathname-
derived defaults are unique by construction; the cross-page bleed
risk only exists for hardcoded shared keys, which the doc warns
against. SSR-safe (falls back to a stable string when window is
unavailable). Pages can still pass an explicit `storageKey` to opt
out of pathname coupling (e.g. when sub-routes should share state).
2. `MaskedInput` `type` was overridable by callers, which defeated
the component's purpose for credentials. Two-layer fix:
- Compile-time: `MaskedInputProps` now extends
`Omit<InputHTMLAttributes, 'type'>` so callers literally cannot
pass `type`. The TS surface enforces the contract.
- Runtime: the `type` attribute on `<Input>` is now placed AFTER
the spread so even an `as`-cast bypass can't override it.
3. The reveal toggle was keyboard-inaccessible. Both `MaskedInput`
and `Field` had `tabIndex={-1}` on the eye-icon button, removing
it from the tab order. Keyboard-only users couldn't show or hide
the secret. Resolution:
- Drop `tabIndex={-1}` so the button joins the natural tab order.
- Add `aria-pressed={revealed}` so screen readers announce toggle
state.
- Add a focus-visible accent ring so the focused state is visible
without a mouse.
Decisions log: v1.8 entry records the rationale for each correction
and explicitly calls out that v1.7's "required storageKey" fix was
worse than the problem it tried to solve.
Validation: typecheck + lint + format + build clean. Tests 519/521
pass (2 pre-existing account-visual-groups failures on dev,
unrelated). Styleguide demos still pass storageKey explicitly so
behavior is unchanged for the only existing callsites.
This commit is contained in:
@@ -52,6 +52,25 @@ After the v1.1 restructure, the `health` page received a separate, focused redes
|
||||
|
||||
---
|
||||
|
||||
## v1.8 revision (2026-04-26) — PR-Agent round 2: storageKey default, MaskedInput type-safety, keyboard-accessible reveal toggle
|
||||
|
||||
Three more substantive issues from upstream review on PR #1109 after the v1.7 fix push. Each is encoded in code + spec:
|
||||
|
||||
**Required `storageKey` was the wrong fix.** v1.7 made `storageKey: string` REQUIRED to prevent cross-page state bleed, but that turned every future production-page migration into a build-breaking event. Better engineering: keep the prop optional and **default to a key derived from `window.location.pathname`** so each route gets its own localStorage slot automatically. The cross-page bleed concern still holds for hardcoded shared keys, but a pathname-derived default is unique by construction. SSR-safe (falls back to a stable string when `window` is unavailable). Pages can still pass an explicit `storageKey` to opt out of pathname coupling (e.g. when sub-routes should share split state).
|
||||
|
||||
**`MaskedInput` type was overridable by callers.** The spread `{...props}` came AFTER the hardcoded `type={revealed ? 'text' : 'password'}`, so a caller passing `type="text"` would silently render a credential in plaintext — a real leakage risk that defeated the component's purpose. Two-layer fix:
|
||||
- Compile-time: `MaskedInputProps` now extends `Omit<InputHTMLAttributes, 'type'>` so callers literally cannot pass `type`. Type system enforces the contract.
|
||||
- Runtime: the `type` attribute on `<Input>` is now placed AFTER the spread so even if the compile-time check is bypassed (e.g. via `as`-cast), the component still wins. Belt-and-braces.
|
||||
|
||||
**Reveal toggle was keyboard-inaccessible.** Both `MaskedInput` and `Field` had `tabIndex={-1}` on the eye-icon button, which removed it from the tab order — keyboard-only users couldn't show or hide the secret. Resolution:
|
||||
- Drop `tabIndex={-1}` so the button joins the natural tab order.
|
||||
- Add `aria-pressed={revealed}` so screen readers announce toggle state.
|
||||
- Add a focus-visible accent ring so the focused state is visible without a mouse.
|
||||
|
||||
These are accessibility regressions that should never have shipped; the new tests-of-record for §5g are: (a) `type` cannot be overridden, (b) the toggle is reachable via Tab and announces state.
|
||||
|
||||
---
|
||||
|
||||
## v1.7 revision (2026-04-26) — PR-Agent feedback: enforce mins, require storageKey, broaden sensitive heuristic
|
||||
|
||||
Three substantive issues surfaced by upstream PR review on PR #1109. Each is encoded in code + spec:
|
||||
|
||||
@@ -42,7 +42,7 @@ The **middle (form) and right (json) panes of `ConfigLayout` MUST be horizontall
|
||||
|
||||
- The left rail is **unified across pages but content-fit** — same envelope, same primitives, but the actual width adapts to its header content. Constraints: `min-w-[240px]` (floor — header controls never squeeze into a wrap) and `max-w-[360px]` (cap — rail can't dominate the body even when an entity label is unusually long; per-item `truncate` inside `ListPane` handles labels beyond that). The rail is NOT user-resizable — only the form↔json split is.
|
||||
- The form ↔ json divider is the **only adjustable split**. Users frequently want a wide form for entering env values OR a wide json pane for reading effective config — never both at the design-time defaults.
|
||||
- **Default ratio: form ~45% / json ~55%** of the body width remaining after the rail. The json pane is **slightly larger by default** because the canonical cliproxy reference shows users spend more time reading effective configuration than editing one field at a time. Persist the user's chosen ratio **per page** via `react-resizable-panels` `autoSaveId` — `ConfigLayout`'s `storageKey` prop is **required**, no default, so each page must pass a stable identifier (e.g. `storageKey="config-layout.cliproxy"`). A shared default would bleed split-ratio state across unrelated pages, so the type system enforces explicit keys.
|
||||
- **Default ratio: form ~45% / json ~55%** of the body width remaining after the rail. The json pane is **slightly larger by default** because the canonical cliproxy reference shows users spend more time reading effective configuration than editing one field at a time. Persist the user's chosen ratio **per page** via `react-resizable-panels` `autoSaveId`. `ConfigLayout`'s `storageKey` prop is optional and **defaults to a key derived from `window.location.pathname`** (so each route gets its own localStorage slot automatically and split state never bleeds across unrelated Config pages). Pass an explicit key only when sub-routes should share state, or to opt out of pathname coupling.
|
||||
- Each pane has an enforced **minimum size of 30%** of the resizable container (the body width after the rail), via `react-resizable-panels`' percentage-based `minSize`. On a 1280px viewport with a 280px rail this lands around 300px per pane — readable but not generous; on standard 1500px+ viewports it lands at 360px+, the practical comfort floor. The library's `minSize` API does NOT support pixel values in v3; if hard pixel floors become necessary later, layer a `onResize` clamp on top.
|
||||
- When the json pane is omitted (`json={undefined}`), the form expands to fill the remaining width — no divider rendered.
|
||||
- Below the `<1024px` breakpoint the layout collapses to tabs (Browse / Configure / JSON) — resizing is irrelevant in tab mode.
|
||||
@@ -389,4 +389,4 @@ These remain bespoke and are out of scope:
|
||||
|
||||
## 7. Decisions
|
||||
|
||||
See [`design-decisions.md`](./design-decisions.md) for the resolved open questions and the v1.1 / v1.2 / v1.3 / v1.4 / v1.5 / v1.6 / v1.7 revision rationale.
|
||||
See [`design-decisions.md`](./design-decisions.md) for the resolved open questions and the v1.1 / v1.2 / v1.3 / v1.4 / v1.5 / v1.6 / v1.7 / v1.8 revision rationale.
|
||||
|
||||
@@ -37,12 +37,14 @@ interface ConfigLayoutProps {
|
||||
/** Right pane: raw JSON / effective config. Omit to hide. */
|
||||
json?: ReactNode;
|
||||
/**
|
||||
* Persistence key for the form↔json split ratio (per §0e). REQUIRED.
|
||||
* Use a stable per-page id like "config-layout.cliproxy" so each page
|
||||
* persists its own ratio in localStorage. A shared/default key would
|
||||
* cause split-ratio state to bleed across unrelated Config pages.
|
||||
* Persistence key for the form↔json split ratio (per §0e). Optional —
|
||||
* defaults to a key derived from `window.location.pathname` so each
|
||||
* route gets its own localStorage slot automatically. Pass an explicit
|
||||
* key only when sub-routes should share split state, or to opt out of
|
||||
* pathname coupling. Avoid hardcoded shared keys: they cause split
|
||||
* ratios to bleed across unrelated Config pages.
|
||||
*/
|
||||
storageKey: string;
|
||||
storageKey?: string;
|
||||
className?: string;
|
||||
}
|
||||
|
||||
@@ -64,6 +66,16 @@ interface ConfigLayoutProps {
|
||||
*/
|
||||
export function ConfigLayout({ left, form, json, storageKey, className }: ConfigLayoutProps) {
|
||||
const isDesktop = useIsDesktop();
|
||||
// Per-route default so pages get unique localStorage slots automatically
|
||||
// (prevents the cross-page state bleed that a hardcoded shared default
|
||||
// would cause). SSR-safe: falls back to a stable string when window is
|
||||
// unavailable. Pages can still pass an explicit storageKey to opt out of
|
||||
// pathname coupling (e.g. when sub-routes should share split state).
|
||||
const effectiveKey =
|
||||
storageKey ??
|
||||
(typeof window === 'undefined'
|
||||
? 'config-layout.default'
|
||||
: `config-layout${window.location.pathname.replace(/\//g, '.')}`);
|
||||
|
||||
// CRITICAL: render exactly one layout at a time. Rendering both and
|
||||
// toggling visibility via Tailwind `hidden lg:grid` would mount two copies
|
||||
@@ -85,7 +97,7 @@ export function ConfigLayout({ left, form, json, storageKey, className }: Config
|
||||
{left}
|
||||
</aside>
|
||||
)}
|
||||
<PanelGroup direction="horizontal" autoSaveId={storageKey} className="min-w-0 flex-1">
|
||||
<PanelGroup direction="horizontal" autoSaveId={effectiveKey} className="min-w-0 flex-1">
|
||||
<Panel defaultSize={json ? 45 : 100} minSize={json ? 30 : 100} order={1}>
|
||||
<main className="h-full overflow-hidden rounded-xl border bg-card">{form}</main>
|
||||
</Panel>
|
||||
|
||||
@@ -44,8 +44,9 @@ export function Field({
|
||||
<button
|
||||
type="button"
|
||||
onClick={() => setRevealed((v) => !v)}
|
||||
className="absolute right-1.5 top-1/2 flex size-7 -translate-y-1/2 items-center justify-center rounded text-muted-foreground transition-colors hover:bg-muted hover:text-accent"
|
||||
aria-label={revealed ? 'Hide value' : 'Reveal value'}
|
||||
aria-pressed={revealed}
|
||||
className="absolute right-1.5 top-1/2 flex size-7 -translate-y-1/2 items-center justify-center rounded text-muted-foreground transition-colors hover:bg-muted hover:text-accent focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-accent/60"
|
||||
>
|
||||
{revealed ? <EyeOff className="size-3.5" /> : <Eye className="size-3.5" />}
|
||||
</button>
|
||||
|
||||
@@ -3,7 +3,10 @@ import { Eye, EyeOff, Lock } from 'lucide-react';
|
||||
import { Input } from '@/components/ui/input';
|
||||
import { cn } from '@/lib/utils';
|
||||
|
||||
interface MaskedInputProps extends React.InputHTMLAttributes<HTMLInputElement> {
|
||||
// `type` is intentionally omitted from the prop surface so callers cannot
|
||||
// override the `password`/`text` toggle and accidentally render a credential
|
||||
// in plaintext. The component is the sole authority on the input type.
|
||||
interface MaskedInputProps extends Omit<React.InputHTMLAttributes<HTMLInputElement>, 'type'> {
|
||||
label?: string;
|
||||
/**
|
||||
* Hide the "sensitive" pill on the label row. Defaults to false (pill shown).
|
||||
@@ -48,20 +51,23 @@ export function MaskedInput({
|
||||
)}
|
||||
<div className="relative">
|
||||
<Input
|
||||
{...props}
|
||||
// `type` MUST come after the spread so it cannot be overridden by
|
||||
// a caller-supplied attribute. Belt-and-braces with the `Omit` on
|
||||
// MaskedInputProps so this is enforced at compile time too.
|
||||
type={revealed ? 'text' : 'password'}
|
||||
className={cn(
|
||||
'pr-9 font-mono transition-all',
|
||||
'focus-visible:ring-1 focus-visible:ring-accent/40 focus-visible:border-accent/50',
|
||||
className
|
||||
)}
|
||||
{...props}
|
||||
/>
|
||||
<button
|
||||
type="button"
|
||||
onClick={() => setRevealed((v) => !v)}
|
||||
tabIndex={-1}
|
||||
aria-label={revealed ? 'Hide value' : 'Reveal value'}
|
||||
className="absolute right-1.5 top-1/2 flex size-7 -translate-y-1/2 items-center justify-center rounded text-muted-foreground transition-colors hover:bg-muted hover:text-accent"
|
||||
aria-pressed={revealed}
|
||||
className="absolute right-1.5 top-1/2 flex size-7 -translate-y-1/2 items-center justify-center rounded text-muted-foreground transition-colors hover:bg-muted hover:text-accent focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-accent/60"
|
||||
>
|
||||
{revealed ? <EyeOff className="size-3.5" /> : <Eye className="size-3.5" />}
|
||||
</button>
|
||||
|
||||
@@ -210,7 +210,7 @@ function Intro() {
|
||||
</div>
|
||||
<p className="text-xs text-muted-foreground">
|
||||
Full spec: <code>ui/docs/design-system.md</code> · revision history:{' '}
|
||||
<code>ui/docs/design-decisions.md</code> (current: v1.7, 2026-04-26). Color rules: §5.
|
||||
<code>ui/docs/design-decisions.md</code> (current: v1.8, 2026-04-26). Color rules: §5.
|
||||
</p>
|
||||
</header>
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user