Repository navigation
UN-4361 [FEAT] Add v1 toasts, inline alerts, progress and skeletons behind one notification entry point - #2426
Merged
Merged
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]>
Signed-off-by: Rahul Johny <[email protected]>
johnyrahul
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…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
Contributor
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).
- 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
Contributor
Author
|
@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
vishnuszipstack
approved these changes
Nov 5, 2026
Contributor
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
chandrasekharan-zipstack
added
the
main-only
This was referenced Nov 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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