diff --git a/ui/docs/design-decisions.md b/ui/docs/design-decisions.md index 639c2e65..e3b64052 100644 --- a/ui/docs/design-decisions.md +++ b/ui/docs/design-decisions.md @@ -52,6 +52,132 @@ 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` so callers literally cannot pass `type`. Type system enforces the contract. +- Runtime: the `type` attribute on `` 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: + +**Width floor — pixel claim was unenforceable.** The previous spec wording said "form ≥ 360px / json ≥ 320px" but `react-resizable-panels` v3 only accepts percentage `minSize`. On a 1280px viewport this could let a user drag a pane down to ~250px — well below the documented floor. Resolution: bump `minSize` from 25 to **30**, restate the floor as percentage (≥ 30% of body width after the rail), document the actual 300–360px range across realistic viewports, note the v3 API constraint, and leave the door open for a future `onResize` clamp if hard pixel floors become necessary. + +**`storageKey` was no longer optional.** The previous default `storageKey="ccs.config-layout"` meant any `` without an explicit key would share localStorage state with every other Config page — split ratios bleeding across unrelated pages. Resolution: make `storageKey` REQUIRED in the `ConfigLayout` props (no default). TypeScript now enforces explicit per-page keys. Pages MUST pass e.g. `storageKey="config-layout.cliproxy"` — this is checked at compile time, not at runtime. + +**Sensitive-field heuristic was too narrow.** The previous regex `AUTH_TOKEN|API_KEY|SECRET|PASSWORD|PRIVATE_KEY` missed common secret names: `ACCESS_TOKEN`, `REFRESH_TOKEN`, `BEARER_TOKEN`, `CLIENT_SECRET`, `CLIENT_ID`, `AWS_ACCESS_KEY_ID`, `AWS_SECRET_ACCESS_KEY`, GCP/Azure/GitHub/OpenAI/Anthropic variants, `JWT`, `OAUTH`, `CREDENTIAL`, `PAT`, `WEBHOOK_SECRET`, `HMAC_KEY`, `SIGNING_KEY`, `SSH_KEY`. Resolution: extract the heuristic to `src/lib/sensitive-label.ts` (`isSensitiveLabel(label)`) and broaden the pattern to cover all of those; case-insensitive; tolerates `_`/`-` separators. The Field component imports the shared helper. Adding a new pattern means editing one regex; every consumer inherits. + +The shared helper is the Single Source of Truth so future drift can't reintroduce per-component heuristics that disagree with each other. + +--- + +## v1.6 revision (2026-04-26) — content-fit rail (unified envelope, not fixed width) + +Live review of the rail-anchored pattern on the API Profiles page surfaced a regression: at the previously-mandated fixed `260px` rail width, the rail header "API Profiles" wrapped onto two lines, the description band wrapped, and the action buttons crowded each other — i.e. the rail was overflowing its own content even though the system mandated that exact width. + +**Decision:** the left rail is **unified across pages but content-fit**, not fixed-width. Same primitives, same style treatment (§5), same vertical structure (header → search → list → footer) — but the *width* adapts to its header content within a unified envelope: + +- `w-fit` — natural width grows to the largest atomic header element (title + buttons stay on one row) +- `min-w-[240px]` — floor so a sparse rail (e.g. 1-section SectionRail with no badges) doesn't squeeze controls +- `max-w-[360px]` — cap so the rail never dominates the body when an entity label is unusually long; per-item `truncate` inside `ListPane` handles labels beyond that + +Pages MUST NOT override this envelope. If a page legitimately needs a wider rail, that is a system-wide envelope change (one PR raises the cap for everyone — uniformity is the point). + +**Why not user-resizable?** The form↔json split *is* user-resizable (§0e) because users have different reading-vs-editing preferences mid-task. The rail isn't, because rail content is page-author-controlled and should be sized by the page author at design time. Letting users drag the rail width adds drag affordances inside an identity strip (visual noise) without solving a real recurring need. + +**Encoded as §0a** in `design-system.md` (mandatory layout invariant; the prior "fixed minimum width" wording in §0e was rewritten to reference the new envelope). `ConfigLayout` ships the envelope as `