Repository navigation
UN-4361 [FEAT] Add v1 toasts, inline alerts, progress and skeletons behind one notification entry point - #2426
johnyrahul wants to merge 45 commits into
Conversation
…N-4270) Adds frontend/src/v1/ as an isolated tree for the v1 frontend (P0-T01): - Folder taxonomy from the spec, a public entry (src/v1/index.ts, imported by v0 as `@v1`) and a cloud overlay placeholder. - src/v1/tsconfig.json: strict, no JS, unused locals/params are errors, `@v1` alias only. v0's tsconfig/jsconfig exclude src/v1. - Biome overrides for the two-way import boundary and v1 bans (no `../`, `@/`, prop-types, antd, component CSS; noExplicitAny). - scripts/check-v1.mjs for what Biome cannot express: TypeScript-only files, the folder taxonomy and import contract, PropTypes, `ant-`. - typecheck:v1, lint:v1 and test:v1 scripts; v0 scripts are unchanged. - test-fixtures/v1-boundary/ proves each rule fails and passes as specified, run with node --test so the v0 vitest suite never sees it. - .gitignore re-includes src/v1/lib/ (the Python `lib/` rule matched it). v0 build output, test counts, typecheck and Biome results are unchanged.
…270) check-v1.mjs: - Scan comments with a string/template-aware tokenizer; a `/*` or `//` inside a string or line comment no longer hides the imports after it. - Read template-literal specifiers, import.meta.glob and `import(..., opts)`. - Flag specifiers that leave src/v1 (`/`, `..`, `@/`, `src/`) itself, not only via Biome, which skips template literals. - Check `@import`/`url()` and `ant-` in styles/ CSS; report symlinks; catch built `ant-` names; treat .test/.spec .mts/.cts as test files; ignore .DS_Store; drop the unused --root option. biome.json: also ban `..`, `./../**`, `/**`, `antd/**`, `prop-types/**`. tsconfig: skipLibCheck off so v1's own .d.ts files are checked. .gitignore: re-include lib/ at any depth under src/v1. Fixtures: a generated matrix over every folder-contract pair, every import form, every outside spelling, a symlink case, and cases for each new rule; tsc cases assert error codes; output matching excludes the command text and spawn errors fail loudly. Comment and README corrections.
… (UN-4270) - ci-frontend-lint: the typecheck job also runs `typecheck:v1` and `lint:v1`, so the strict v1 tsconfig and the scripts/check-v1.mjs checks (folder contract, TypeScript-only files, PropTypes, `ant-`) gate PRs now rather than from P0-T10. - Pages may no longer import app/, test/ or the v1 entry, only the shared folders (components, shell, services, stores, hooks, lib, styles, plugins). Test files in a page may still import test/. Contract table, README and the fixture matrix updated together.
…(UN-4270) check-v1.mjs: split the comment scanner into small helpers (cognitive complexity 40 -> under 15), drop the nested ternary and nested template literal, use String.raw for the import regexes, keep CLI flags in a Set. Fixture runner: - Cases and the expected folder contract are parsed text tables instead of repeated object literals (the 10% duplication). - Sorts use an explicit localeCompare comparator. - The contract matrix is built by a helper. - Spawns /bin/sh by absolute path and no longer needs `git init` (Biome only needs the ignore file). The git check-ignore call keeps a PATH lookup, with the reason and a NOSONAR marker. Fixture cases: re-exports rewritten as small functions and read-only props, without changing what each case exercises.
…nnot see (UN-4270) Addresses Greptile review on #2380: - import.meta.glob([...]): every pattern in the array is checked, not only the first, so an allowed first pattern no longer hides a forbidden one. - check-v1 --code also scans v0 (src/ outside v1, including the cloud src/plugins overlay) for template-literal imports, globs, require(), @/v1, src/v1, relative paths and CSS @import that reach into v1 other than through `@v1`. Biome still covers plain imports. Fixtures: Greptile's example as a case, a glob array with a forbidden later pattern, and a generated test of every v0 -> v1 form (denied and allowed). The v0 scan reports nothing on the current tree.
…i 5.2 both declare containSubset Co-Authored-By: Claude Opus 5.5 <[email protected]>
… per-browser preference Co-Authored-By: Claude Opus 5.5 <[email protected]>
…tore Co-Authored-By: Claude Opus 5.5 <[email protected]>
…in three modes Co-Authored-By: Claude Opus 5.5 <[email protected]>
… match the v1 Tailwind root Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
…t it Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ercase letters only Co-Authored-By: Claude Opus 5.5 <[email protected]>
…heme mapping and the focus utility Co-Authored-By: Claude Opus 5.5 <[email protected]>
…so biome ci passes src/v1/styles/tokens.css is written by scripts/build-v1-tokens.mjs and checked byte-for-byte by styles/tokens.test.ts; the formatter would rewrap it and break the check. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
…sts and on missing variants Co-Authored-By: Claude Opus 5.5 <[email protected]>
…and portal sync Co-Authored-By: Claude Opus 5.5 <[email protected]>
…tEffect Co-Authored-By: Claude Opus 5.5 <[email protected]>
…line styles, CSS imports and !important fail lint Co-Authored-By: Claude Opus 5.5 <[email protected]>
… arbitrary values Co-Authored-By: Claude Opus 5.5 <[email protected]>
… and the fixtures v0's root auto-detected sources across the whole frontend, so the v1 tree, scripts/check-v1-styles.mjs and test-fixtures/ added 12 unprefixed utilities and 8 theme variables to v0's stylesheet. With these three @source not lines the v0 build is byte-identical to the base again (239/239 files). Co-Authored-By: Claude Opus 5.5 <[email protected]>
…longer cancel each other Co-Authored-By: Claude Opus 5.5 <[email protected]>
…our arbitrary values and !important after a variant Co-Authored-By: Claude Opus 5.5 <[email protected]>
…cope variants, shadows and the root fill to v1 Co-Authored-By: Claude Opus 5.5 <[email protected]>
…the theme test exercises the mode effect Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ge, the pixel escape and the current unprefixed-class rationale Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Fable 5.1 <[email protected]>
… from review
S3 let a unit followed by `_` through (`ui:grid-cols-[200px_1fr]`,
`ui:m-[2rem_auto]`): the lookahead after the unit rejected `\w`, which
includes Tailwind's in-bracket separator. It now rejects letters and `-`
only, and fixture v1-raw-pixel-arbitrary-separator pins both forms.
S8 read every literal inside cva() as a class list, so a normal
`defaultVariants: { variant: "outline" }` failed because outline is also a
utility name. classSiteLiterals now tracks the property chain inside cva()
and treats as classes only the base string, the values under variants and
a compound `class`/`className`; a property name, a defaultVariants value or
a compoundVariants matcher is a choice, not a class. The passing fixture
carries a full cva config; v1-cva-unprefixed-variant proves a variant
value is still checked.
S6 reported every nested `& + x` as leaving the root, but under `.v1 .menu`
it resolves to `.v1 .menu + x`, inside v1. selectorPreludes now records
whether every enclosing rule is one compound on the root, and only then
does a `&` sibling leave. The nested fixture is rewritten to the leaving
forms (directly under `.v1`, and under `&[data-theme="dark"]`), and the
passing fixture gains the safe ones.
Co-Authored-By: Claude Fable 5.1 <[email protected]>
…narCloud findings
SonarCloud's duplication check ignores literal values, so the 75 same-shaped
entries in styles/tokens.ts read as copies of each other (91% of the file,
16.8% of the PR's new lines, gate threshold 3%). The values now live in
styles/tokens.json, one array per family in board order; tokens.ts keeps the
Token type and builds TOKENS from it, deriving cssVar from name (true for
every entry) and expanding a single string to all three modes. tokens.css is
regenerated (header only) and the lint no longer needs a raw-values exemption
for tokens.ts.
Also, from Ritwik's pass over the remaining findings:
- CSS_UNIT_RE: `\d+(?:\.\d*)?|\.\d+` matches the same strings without the
overlapping alternation (javascript:S8786).
- The stem escape `s.replace("-", "\\-")` was a no-op: `-` is literal outside
a character class.
- The file-type dispatch uses `endsWith(".css")` (javascript:S6557).
- `.sonarcloud.properties` excludes frontend/test-fixtures/**: the boundary
fixtures break the v1 rules on purpose.
Co-Authored-By: Claude Fable 5.1 <[email protected]>
…on entry point Toasts (Radix Toast), InlineAlert with retry, ProgressBar, StepProgress, Skeleton primitives and Spinner under components/ui/feedback/, plus the notify() contract in lib/notify.ts that the Toaster implements and P0-T05's error normaliser can call. lint:v1 rejects toast libraries outside the feedback folder, with guardrail fixtures. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01HzRGGSAhDA4adWPAfcbhpt
 [Low impact] The PR appears safe to merge; the reported focus problem is fixed.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- The single-entry-point lint now also catches `await import("radix-ui")`
(namespace or destructured Toast) and `Radix["Toast"]`, with fixtures.
- ProgressBar: an invalid or zero max shows an empty bar instead of NaN,
and the indeterminate pulse respects reduced motion.
- The replace-by-id test replaces an already-open toast.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01HzRGGSAhDA4adWPAfcbhpt
|
[Edit: #2399 has merged, and this PR now targets Generated by Claude Code |
Signed-off-by: Rahul Johny <[email protected]>
johnyrahul
changed the title
…container changes - Import Radix Progress as RadixProgress: main's shim-completeness test scans .tsx for <Foo.Bar> and read <Progress.Root> as the antd Progress shim, which has no Root. - Toaster now re-targets its viewport when the container prop changes (Greptile finding). Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01HzRGGSAhDA4adWPAfcbhpt
vishnuszipstack
left a comment
vishnuszipstack
left a comment
There was a problem hiding this comment.
Review of PR 2426 — UN-4361 v1 feedback components
Mode: INITIAL. Reviewed head c0dd48955812eb33f2d8b4dc162f2f3a7f72a8a7, merge base 332b4e3c0e5cb8237180f95f350d7c1aa7401bdb, base freshened (base_fetch: ok), 24 files, 1665 lines (matches GitHub). Rubric: unstract plugin v2.1.80.
Signals: lens 13 and component lifecycle raised (tests changed, React effects). facts.sh also raised the attack-surface and auth triggers; the only matching added lines I found use the word "token" for design tokens.
Security deep-dive: done
boundary-sweep: 1 sinks traced, clean (the notify() console fallback carries title, description and request id to the viewer's own browser console).
Verdict — REQUEST CHANGES
Summary — Critical: 0 · High: 1 · Medium: 8 · Low: 3 · Lenses run: 19/19 (1 not covered, 6 N/A; see checklist).
No v1 route renders these components and nothing calls notify() yet, so no user is affected today. Severities rate the contract that P1-T11, P1-T14 and P1-T18 will build on.
Findings are inline below, numbered as in the summary counts.
Unanchored findings
- The PR body is out of date: "Five guardrail fixtures" (the diff adds seven rows) and "63 vitest tests and 94 guardrail fixture rows" (the run here gave 64 and 96). It also still says "Stacked on #2399. Retarget to
mainonce it merges", and a PR comment repeats the merge-order warning, though the base is alreadymain. - The CI report's
uirow is red, but it is not this PR's: the same row fails on PRs 2430, 2424 and 2422, and the group is declared optional scaffolding intests/groups.yaml. Thee2echeck passes.
Lens checklist
- Spec & intent — See findings #6
- Architectural fit & precedent — Clean
- Correctness & edge cases — See findings #1, #2, #3, #5, #6, #7, #8, #10
- Security — Clean
- Data integrity & migrations — N/A — no persisted data or schema
- Concurrency — Clean
- API & contract compatibility — See findings #1, #4, #11
- Reliability & resilience — N/A — no external calls
- Performance & cost — Clean
- Observability — See findings #4
- Operational safety — N/A — no route renders v1, and no infra or rollout config changed
- LLM/agent-specific — N/A — no model, prompt or agent path
- Testing — See findings #6, #9
- Dependencies & build — Clean
- Code quality — Clean
- Doc & comment accuracy — See findings #1, #2, #4, #6, #7, #8, #12
- Cross-document rule consistency — Not covered — merge-result sweep unavailable: git 2.34 here, the script needs 2.38
- On-prem deployment experience — N/A — no chart, values or on-prem surface touched
- Scaling & workload lifecycle — N/A — no worker or scaling config touched
Assessed by me rather than an agent: lenses 2 (with code-reviewer), 4, 5, 6, 8, 11, 12, 14, 15, 17, 18, 19.
- Lens 4: the security deep-dive's find pass reported no candidates, so its filters had nothing to run on. It rests on that one pass plus my own read of the diff, and it excludes DoS and rate limiting by design.
- Lens 15:
biome ci src/v1,lint:v1andtypecheck:v1all pass here. - Lens 17: branch-side grep of the changed rules found no disagreement beyond finding 12. The one commit
origin/maingained since the merge base touches none of these paths.
Open questions for the author
- Does
aria-busy="true"on therole="status"skeleton (Skeleton.tsx:L29-L36) suppress the "Loading projects" label? The region is never set non-busy, it just unmounts, androle=statustakes no name from content, though the test atfeedback.test.tsx:L96-L113is titled "one busy, named region". UNVERIFIED — an NVDA or VoiceOver pass would settle it. - Should a
persistenttoast outrank the three-toast cap? The frames or spec decide finding 2. - Should the request id be visible in the toast? That decides finding 4.
Assumptions made
- Nothing in the design spec or frames was opened; the Confluence and Figma links were not read, so spec conformance in lens 1 is judged against the PR body and the code's own comments.
- The console is treated as a safe sink because it is the viewer's own browser; I did not check whether a v1 shell will forward console output to a third party.
- Probes ran under happy-dom. Layout, real pointer timing and screen-reader output were not exercised in a browser.
Dropped after checking: the "invisible click-blocking strip" from the cold read (Radix sets pointerEvents: none on the wrapper with no toasts, dist/index.mjs:L180), and the v0 svg.lucide rule collapsing icon sizes (it sits in @layer base, so ui:size-* utilities win; layer order only, not checked in a browser).
| setEntries((all) => | ||
| [...all.filter((e) => e.id !== id), { ...input, id }].slice( | ||
| -MAX_TOASTS, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
[High] [Lens 3, 7, 16] — Replacing a toast by id keeps the old Radix instance, so the timer, persistent and the announcement stay stale (finding 1)
- Location:
frontend/src/v1/components/ui/feedback/Toaster.tsx:L53-L57,:L79; contract atfrontend/src/v1/lib/notify.ts:L24,:L28;frontend/src/v1/README.md:L75 - Failure mode:
key={entry.id}reuses the mounted Root. "Uploading" replaced by a persistent error "Upload failed" still closes on the first toast's timer. A replacement sent at 5.9s of 6s is visible for 0.1s. Screen readers never hear the replacement, so an error that replaces an info toast is not announced at all once the first second has passed. Progress then outcome is whatidexists for. - Evidence: Radix 1.2.23
dist/index.mjs:L326returns onInfinitybeforeclearTimeout, so the armed timer survives;:L355-L356has deps[open, duration, ...], so an equal duration does not restart it;:L366-L368memoises the announcement on[node]. Replace test atToaster.test.tsx:L168-L181checks only text anddata-kind. - Suggested fix: add a per-
showrevision toEntryand use it as the Reactkey, keepingidfor lookup. That remounts on replace and also fixes finding 3 if close removes by revision. Move focus to the viewport first if the old toast held it. Add fake-timer tests and a live-region assertion. - Confidence: High.
- Verification: VERIFIED — source read, and I re-ran a happy-dom probe importing the real
Toaster.tsx: a timed-to-persistent replacement was gone 400ms later, and the announcer still read "Notification one" after replacing with "two", with nothing announced for a replace after 1s. A separate fake-timer test in a private copy agreed. No real screen reader was used.
There was a problem hiding this comment.
Fixed in bb4ea8b. Each show() now gets a revision. It is the React key, so a replacement mounts a fresh Radix Root with its own timer, persistent and announcement, and id is used only for lookup. If the old toast held focus, focus moves to the viewport first. New tests: a timed toast replaced by a persistent one at 900ms of 1000 is still open after 5s, and a later replacement gets a full fresh timer (fake timers); a replacement is announced, assertively for an error. Both fail with key={entry.id}.
Generated by Claude Code
| [...all.filter((e) => e.id !== id), { ...input, id }].slice( | ||
| -MAX_TOASTS, | ||
| ), |
There was a problem hiding this comment.
[Medium] [Lens 3, 16] — The three-toast cap evicts persistent toasts silently and drops keyboard focus to <body> (finding 2)
- Location:
frontend/src/v1/components/ui/feedback/Toaster.tsx:L54-L56; contract atfrontend/src/v1/lib/notify.ts:L24,frontend/src/v1/README.md:L77 - Failure mode: three later "Saved" toasts remove an open persistent error along with its action, despite "Stays until the user dismisses it". Nothing is logged. A keyboard user focused in the evicted toast lands on
<body>, because eviction bypasses Radix'shandleClose, the only code that moves focus to the viewport. - Evidence:
.slice(-MAX_TOASTS)ignorespersistent;Toaster.test.tsx:L159-L167pins eviction using persistent toasts, while the prose says the opposite. The PR body does state "oldest drops first", so the eviction is deliberate and the contradiction is between it and thepersistentdocs. - Suggested fix: evict the oldest non-persistent toast first and decide what happens when all are persistent, or reword both docs to say the cap wins. Move focus to the viewport before removing a focused toast.
- Confidence: High on behaviour. Design intent for the frames would settle which side changes.
- Verification: VERIFIED — probe:
["a","b","c"], active elementBODY. SameBODYresult fordismissNotificationwith focus inside.
There was a problem hiding this comment.
Fixed in bb4ea8b. At the cap, the oldest timed toast goes first (the newest always stays). Only when every other toast is persistent does the oldest persistent one go, so the stack stays bounded. The notify.ts and README docs now say exactly that. Any removal of a focused toast (eviction, replacement or dismissNotification) moves focus to the viewport first, so it no longer drops to <body>. New tests: "drops timed toasts before persistent ones at the limit" and "moves focus to the viewport when a focused toast is removed".
Generated by Claude Code
| onOpenChange={(open) => { | ||
| if (!open) { | ||
| onClose(); | ||
| } | ||
| }} |
There was a problem hiding this comment.
[Medium] [Lens 3] — An action that re-notifies its own id has the replacement removed immediately (finding 3)
- Location:
frontend/src/v1/components/ui/feedback/Toaster.tsx:L97-L101,:L129 - Failure mode:
onSelect: () => notify({ id: sameId, title: "Retrying…" })queues the replacement, then close runsremove(entry.id)and deletes it. A Retry button shows nothing and the message is lost with no log. - Evidence: Radix
dist/index.mjs:L532iscomposeEventHandlers(props.onClick, interactiveContext.onClose). - Suggested fix: remove by the per-
showrevision from finding 1. - Confidence: High.
- Verification: VERIFIED — probe:
[]after clicking Retry.
There was a problem hiding this comment.
Fixed in bb4ea8b. A toast's close now removes its own revision rather than its id, so a replacement queued by the action survives. Test: "keeps a replacement that the toast's own action sends" (a Retry that re-notifies the same id leaves "Retrying" on screen). It fails when close removes by id.
Generated by Claude Code
| /** The backend request id, kept with the message so support can trace it. */ | ||
| requestId?: string; |
There was a problem hiding this comment.
[Medium] [Lens 7, 10, 16] — requestId is dropped whenever a Toaster is mounted (finding 4)
- Location:
frontend/src/v1/lib/notify.ts:L26-L27;frontend/src/v1/components/ui/feedback/Toaster.tsx:L91-L144 - Failure mode: documented as "kept with the message so support can trace it", but only the no-notifier fallback reads it. On the production path it is not rendered or logged, so a user reporting an error toast has no id to give. Radix also sets inline
user-select: noneon the toast, so the text cannot be copied. - Evidence: outside tests,
requestIdappears only atnotify.ts:L27and:L67;document.body.innerHTML.includes("req-42")was false under a real Toaster. - Suggested fix: render the id, or log it in
notify()on both paths, and test it through the real Toaster. If neither is wanted yet, correct the comment. - Confidence: High on behaviour. Medium on intent; the P1-T13 spec would settle whether the id is visible.
- Verification: VERIFIED — grep and a throwaway test in a private copy.
There was a problem hiding this comment.
Fixed in bb4ea8b. When requestId is set, the toast renders "Request ID: …" as a caption line, with select-text so it can be copied despite Radix's user-select: none on the root. The doc comment now says it is shown. Test: "shows the request id where it can be copied", through the real Toaster. If design later prefers it hidden, it is one line to drop.
Generated by Claude Code
| onOpenChange={(open) => { | ||
| if (!open) { | ||
| onClose(); | ||
| } | ||
| }} | ||
| duration={entry.persistent ? Number.POSITIVE_INFINITY : undefined} |
There was a problem hiding this comment.
[Medium] [Lens 3] — After the last toast is closed under the pointer, the next timed toast never times out (finding 5)
- Location:
frontend/src/v1/components/ui/feedback/Toaster.tsx:L44-L46,:L97-L102; contract atfrontend/src/v1/lib/notify.ts:L24 - Failure mode: hover sets
isClosePausedRef. Removing the last toast tears down the viewport listeners without resuming, so the ref stays true and the next toast's timer never starts. It stays until the user hovers and leaves it or the window regains focus. This is stock Radix behaviour, but this PR ships it as v1's only toast region. - Evidence: Radix
dist/index.mjs:L94-L131(listeners only whilehasToasts) and:L355-L356(!context.isClosePausedRef.currentgate). - Suggested fix: unsure which is best. Either key
RadixToast.Provideron a generation that bumps whenentriesempties (then recheck F8 and focus), or resume before removing the last entry. - Confidence: Medium. A manual run in a real browser would raise it: hover, click X on the only toast, move away, fire a new toast.
- Verification: VERIFIED — probe under happy-dom with a synthetic pointer:
["second"]still open 600ms after a 300ms duration, gone after a windowfocus. Not run in a real browser.
There was a problem hiding this comment.
Fixed in bb4ea8b with the first option. A generation counter bumps when the last toast goes, and RadixToast.Provider is keyed on it, so the stale isClosePausedRef is discarded with the old provider. F8 still works (its test passes), and if the removed toast held focus, focus is restored to the new viewport after the remount. Test: "times out the next toast after the last one closes under the pointer" (hover, close the only toast with X, fire a 1000ms toast, gone at 1500ms). It fails without the key. I have not run it in a real browser yet.
Generated by Claude Code
| // Radix rejects a max that is not a positive number; an empty batch (0 of 0) | ||
| // shows as an empty bar rather than NaN. | ||
| const validMax = Number.isFinite(max) && max > 0; | ||
| const total = validMax ? max : 100; | ||
| const clamped = | ||
| indeterminate || !validMax ? 0 : Math.min(Math.max(value, 0), total); |
There was a problem hiding this comment.
[Medium] [Lens 3, 16] — ProgressBar guards an empty batch through max but not through value (finding 8)
- Location:
frontend/src/v1/components/ui/feedback/ProgressBar.tsx:L25-L30,:L43 - Failure mode: the comment promises an empty batch "shows as an empty bar rather than NaN". The common form,
value={done / total * 100}with the defaultmax, passesNaN, which yieldsstyle="--progress: NaN",data-state="indeterminate"and a Radixconsole.error, while the component's ownindeterminateis false, so there is no pulse. - Evidence:
Math.min(Math.max(NaN, 0), total)staysNaN. The test atfeedback.test.tsx:L49-L60covers only a badmax. - Suggested fix: treat a non-finite
valueas 0 or as indeterminate, and test that. - Confidence: High on output and ARIA. Medium on how
calc(NaN*100%)paints; a browser render would settle it. - Verification: VERIFIED — the output above came from a throwaway test in a private copy.
There was a problem hiding this comment.
Fixed in bb4ea8b. A non-finite value is treated as 0, so it gives an empty bar, aria-valuenow="0", not indeterminate, and no NaN in the style. The test now covers value={Number.NaN} alongside the bad-max cases.
Generated by Claude Code
| }, []); | ||
|
|
||
| useLayoutEffect(() => { | ||
| setTarget(container ?? getPortalContainer()); |
There was a problem hiding this comment.
[Medium] [Lens 13] — New logic that survives mutation, including the Greptile container fix (finding 9)
- Location:
frontend/src/v1/components/ui/feedback/Toaster.tsx:L49,:L147-L149;frontend/src/v1/components/ui/feedback/ProgressBar.tsx:L29-L30,:L43; tests atToaster.test.tsx:L47-L64,feedback.test.tsx:L37-L47,:L135-L140 - Failure mode: these product changes leave vitest green at 64/64: reverting commit c0dd489's
setTarget(container ?? getPortalContainer());assertivereturning true for success and warning (the test fires only error and info); removing the lower clamp; computing the fill against 100 rather thantotal, sovalue=3 max=4draws 3%. Separately, the test named for "no large size" never readssize. - Suggested fix: rerender with a new
container; loopNOTIFY_KINDSforaria-live; addvalue={-5}and assert--progress: 0.75for 3 of 4; drop that clause from the Spinner test's name or pin it with@ts-expect-error. - Confidence: High.
- Verification: VERIFIED — mutants applied in private copies; the
sizeitem was read from the code and not mutated.
There was a problem hiding this comment.
Fixed in bb4ea8b:
container: new test "follows a new container" rerenders with another host. It fails if c0dd489 is reverted.aria-live: the test now loopsNOTIFY_KINDSand expects assertive only forerror.- Lower clamp: added
value={-5}→aria-valuenow="0". - Fill: new test asserts
--progress: 0.75for 3 of 4. - Spinner: dropped "has no large size" from the test name.
I also mutation-checked the new Toaster tests: key={entry.id}, close-by-id, the old container effect and dropping the provider remount each fail at least one test.
Generated by Claude Code
| return ( | ||
| <RadixToast.Provider | ||
| duration={duration} | ||
| swipeDirection="right" |
There was a problem hiding this comment.
[Low] [Lens 3] — Swipe-to-dismiss is on with no visual feedback (finding 10) — frontend/src/v1/components/ui/feedback/Toaster.tsx:L74. A 50px mouse drag closes a toast, including a persistent error, while it does not visibly move; nothing in src/v1 reads --radix-toast-swipe-*. Add the swipe classes or disable the gesture. Confidence: High. VERIFIED — read Radix dist/index.mjs:L451-L452 and grepped src/v1.
There was a problem hiding this comment.
Fixed in bb4ea8b by switching the gesture off: swipeThreshold={Number.POSITIVE_INFINITY}, with a comment saying why (there are no swipe styles in the design). It can come back together with swipe classes if design asks for it.
Generated by Claude Code
| {onRetry ? ( | ||
| <button | ||
| type="button" | ||
| onClick={onRetry} |
There was a problem hiding this comment.
[Low] [Lens 7] — Zero-argument callbacks receive the click event (finding 11) — frontend/src/v1/components/ui/feedback/InlineAlert.tsx:L59, frontend/src/v1/components/ui/feedback/Toaster.tsx:L129. () => void accepts load(force?: boolean), so onRetry={load} compiles and then gets a truthy MouseEvent as force. Use onClick={() => onRetry()}. Confidence: High. VERIFIED — read both call sites.
There was a problem hiding this comment.
Fixed in bb4ea8b. Both are now onClick={() => onRetry()} and onClick={() => entry.action?.onSelect()}, and the tests assert each callback is called with no arguments.
Generated by Claude Code
| * The notification contract: the one way v1 tells the user something happened. | ||
| * | ||
| * It lives in `lib/` so every folder can reach it, including `services/`, which | ||
| * may import only `lib/`: the P0-T05 error normaliser calls `notify()`, and the |
There was a problem hiding this comment.
[Low] [Lens 16] — Two stale statements (finding 12) — frontend/src/v1/lib/notify.ts:L5 says the P0-T05 normaliser calls notify() in the present tense, and no such caller exists; frontend/src/v1/README.md:L110, the lint:v1 row of the Scripts table, omits the new rule. Confidence: High. VERIFIED — grep and read.
There was a problem hiding this comment.
Fixed in bb4ea8b. notify.ts now says the P0-T05 normaliser "is meant to call" notify(), and the lint:v1 row in the README's Scripts table names the single-notification-entry-point check.
Generated by Claude Code
- Toaster: key and remove toasts by a per-show revision, so a replacement gets its own timer, persistence and announcement, and an action that re-notifies its own id is kept. At the cap, timed toasts drop before persistent ones. Focus moves to the viewport when a focused toast is removed. The provider remounts when the last toast goes, clearing Radix's stuck hover pause. The request id is shown and selectable. Swipe-to-dismiss is off (no swipe styles in the design). Actions and onRetry are called with no arguments. - lint:v1: radix-ui is allowed outside components/ui/feedback/ only as a static named import that does not name Toast; every other reference is reported. Five new fixtures. - ProgressBar: a non-finite value shows an empty bar. - Skeleton: no aria-busy on the status region, so its label is read. - notify/README: describe the no-notifier fallback and replacement accurately; lint:v1 row names the new rule. - Tests for each behaviour, checked against mutants. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01HzRGGSAhDA4adWPAfcbhpt
|
@vishnuszipstack thanks for the thorough review. All 12 findings are addressed in bb4ea8b, with replies on each thread. On the rest of your review:
Generated by Claude Code |
…pties the list Removing a focused toast while others stayed open left the refocus flag set, so the next provider remount (when the last toast timed out) pulled focus to the viewport, away from wherever the user had gone. Set the flag only when the removal itself empties the list. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01HzRGGSAhDA4adWPAfcbhpt
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
Unstract test resultsPer-group results
Critical paths
|
Requested by Johny · project thread
What
frontend/src/v1/components/ui/feedback/:Toaster: the single toast region, on Radix Toast. Success, error, warning and info, with an optional action and an optionalpersistentflag. Toasts stack bottom-right, three at most; timed toasts drop before persistent ones. They pause while hovered or focused, and F8 moves focus to them. Reusing anidreplaces a toast with a fresh one, which gets a new timer and is announced again. ArequestIdis shown and can be copied. Swipe-to-dismiss is off.InlineAlert: the error state from dev spec section 8, "inline, above the content it concerns, with a retry".ProgressBar(Radix Progress, determinate and indeterminate, name required; a badmaxorvaluegives an empty bar) andStepProgress(done, current, upcoming;aria-current="step").SkeletonwithSkeletonText(12 then 10 tall, widths 100%, 78%, 46%),SkeletonBlock,SkeletonAvatar,SkeletonTableRows.Spinner: button and inline sizes only, so it cannot become a page loader.lib/notify.ts: the notification contract.notify()andtoast.*; theToasterregisters itself withsetNotifier. With nothing mounted, the message is only logged to the console with its request id; it is not shown or queued.lint:v1:check-v1.mjsrejects sonner, react-hot-toast, react-toastify and notistack outsidecomponents/ui/feedback/. There, radix-ui is allowed only as a static named import that does not nameToast; any other reference to it is reported. antd is already rejected by Biome. 12 guardrail fixtures.lint:v1row.Why
How
lib/, not in the componentservices/may import onlylib/, and P0-T05's error normaliser is meant to call it. Proposed to Kirtiman on UN-4313.assertive/politeannouncements.show()revision, not byidpersistentand announcement. An action that re-notifies its own id must not be removed by its own close.type="foreground"), everything else politelyToastto chase them one by one.aria-busy__snapshots__/markup.mdviatoMatchFileSnapshottypecheck:v1rejects.snapfiles under v1.src/v1/tsconfig.jsonhas no jest-dom types; adding them belongs to P0-T08's harness.ui:box-borderon padded full-width boxesw-fullplus padding overflowed. In the browser, toasts ran off the screen edge.infotoken, so info usesprimary. Nodestructive-mutedtoken, so the InlineAlert fill ismuted, as in frame 622:1320. Skeleton radius 4 and bar colour #e4e4e7 are not tokens, so they userounded-smandmuted. Raised with design.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
frontend/src/v1/, which no v0 route renders, plus a v1-only lint rule and its fixtures. No v0 file or dependency changes.Relevant Docs
Related Issues or PRs
main.Dependencies Versions / Env Variables
radix-ui,lucide-reactandclass-variance-authorityare already dependencies.Notes on Testing
bun run typecheck:v1,bun run lint:v1andbiome ci srcare clean.bun run test:v1passes 73 vitest tests and 101 guardrail fixture rows, and the full frontend vitest suite passes 728 tests.Screenshots
The Light and Dark renders are attached in the project thread. v1 has no route yet, so there is no in-app page to visit.
Checklist
I have read and understood the Contribution Guidelines.
Generated by Claude Code