Repository navigation
chore(frontend): upgrade Console UI dependencies to latest - #2661
malinskibeniamin wants to merge 7 commits into
Conversation
🚨 Registry drift detectedApp:
Components needing attention
Refresh command: bunx shadcn@latest add @redpanda/code-block-dynamic @redpanda/combobox @redpanda/stat --overwriteGenerated by lookout audit-changes. |
malinskibeniamin
left a comment
malinskibeniamin
left a comment
There was a problem hiding this comment.
Automated /review: 8 finding(s).
These findings could not be anchored to a changed line:
- P3
frontend/tests/federation/remote-adapter.cjs:9— The guard now checks only that the export hasget/initfunctions, but the error message still reads "Federation build did not export the rp_console container". After thecommonjs-modulechange the container name is no longer verified anywhere, so renaming the remote (rp_console) would leave the federation test green while consumers break.
Correction: keep the duck-type check and assert the name from the build output, or reword the message to match what is actually verified ("remoteEntry did not export a federation container").
Verify: bun run test:federation after temporarily renaming the remote in the module-federation config — it should fail.
| "format": "biome format src/* --write", | ||
| "format:check": "biome format src/*", | ||
| "install:chromium": "playwright install chromium", | ||
| "lint": "ultracite fix || true", |
There was a problem hiding this comment.
Priority: P1
Lint failures can no longer fail any gate.
"lint": "ultracite fix || true" always exits 0. Consequences, all introduced by this line:
"quality:gate": "bun run lint && bun run doctor && ..."(line 26) can never fail on lint.- The frontend CI lint job runs
bun run lintand then checks only for a dirty tree. Non-fixable diagnostics (noConsole,noMisusedPromises,useExhaustiveSwitchCases, type-aware rules) produce no file modification, so they leave a clean tree and a zero exit — a false success. frontend/lefthook.yml:3gained the same|| true, so pre-commit is advisory too.
This compounds with the Biome 2.5 migration in the same commit: biome.jsonc moves noReactForwardRef, noDeprecatedImports, and noImportCycles out of nursery into suspicious, and switches "recommended": true to "preset": "recommended". If any of those keys is wrong for 2.5.14, Biome exits non-zero with a configuration error — and || true swallows it, leaving the linter silently disabled repo-wide with a green CI.
Correction: drop || true from both places. If CI must not mutate files, point the CI job at the existing lint:check (line 20) and keep lint as the local autofix entrypoint:
"lint": "ultracite fix",Verify: bun run lint:check exits non-zero on a deliberately introduced console.log, and bun run lint surfaces (not swallows) a bad rule key — e.g. temporarily rename noImportCycles to noImportCyclesXYZ and confirm a non-zero exit.
Automated /review.
| uses: actions/checkout@v6 | ||
| with: | ||
| # `--scope changed` diffs against origin/master to report only new issues. | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
Priority: P2
React Doctor's changed-file scope can't resolve its base on this trigger.
frontend/package.json:11 now runs react-doctor . -y --blocking error --scope changed --base origin/master, and this step adds fetch-depth: 0 to support it. Two problems with the workflow as written (on: push only, no pull_request trigger):
- Feature-branch pushes:
actions/checkoutfetches a branch-scoped refspec (+refs/heads/<branch>*:refs/remotes/origin/<branch>*) —fetch-depth: 0removes the depth limit but does not widen the refspec. Sorefs/remotes/origin/masterdoes not exist on a non-master push, and--base origin/masterhas nothing to diff against. - Master pushes: checked-out
HEADisorigin/master, so the changed set is empty and newly merged issues are never scanned.
Either way the job is not doing the work its name implies, and because --blocking error only trips on findings, an empty or unresolvable scope reports green.
Correction: fetch the base explicitly and diff against the real merge base. For a push event that is the event's before-SHA, not the branch tip:
- name: Checkout
uses: actions/checkout@v6
with:
fetch-depth: 0
- name: Fetch base
run: git fetch --no-tags origin master:refs/remotes/origin/masterand run the unscoped full scan when github.ref == 'refs/heads/master'. Adding a pull_request trigger would make --scope changed meaningful in the place it is actually useful.
Verify: on a branch push, git rev-parse origin/master inside the job; then bun run doctor and confirm the log lists a non-empty changed-file set.
Automated /review.
| 'M22.2819 9.8211a5.9847 5.9847 0 0 0-.5157-4.9108 6.0462 6.0462 0 0 0-6.5098-2.9A6.0651 6.0651 0 0 0 4.9807 4.1818a5.9847 5.9847 0 0 0-3.9977 2.9 6.0462 6.0462 0 0 0 .7427 7.0966 5.98 5.98 0 0 0 .511 4.9107 6.051 6.051 0 0 0 6.5146 2.9001A5.9847 5.9847 0 0 0 13.2599 24a6.0557 6.0557 0 0 0 5.7718-4.2058 5.9894 5.9894 0 0 0 3.9977-2.9001 6.0557 6.0557 0 0 0-.7475-7.0729zm-9.022 12.6081a4.4755 4.4755 0 0 1-2.8764-1.0408l.1419-.0804 4.7783-2.7582a.7948.7948 0 0 0 .3927-.6813v-6.7369l2.02 1.1686a.071.071 0 0 1 .038.052v5.5826a4.504 4.504 0 0 1-4.4945 4.4944zm-9.6607-4.1254a4.4708 4.4708 0 0 1-.5346-3.0137l.142.0852 4.783 2.7582a.7712.7712 0 0 0 .7806 0l5.8428-3.3685v2.3324a.0804.0804 0 0 1-.0332.0615L9.74 19.9502a4.4992 4.4992 0 0 1-6.1408-1.6464zM2.3408 7.8956a4.485 4.485 0 0 1 2.3655-1.9728V11.6a.7664.7664 0 0 0 .3879.6765l5.8144 3.3543-2.0201 1.1685a.0757.0757 0 0 1-.071 0l-4.8303-2.7865A4.504 4.504 0 0 1 2.3408 7.872zm16.5963 3.8558L13.1038 8.364 15.1192 7.2a.0757.0757 0 0 1 .071 0l4.8303 2.7913a4.4944 4.4944 0 0 1-.6765 8.1042v-5.6772a.79.79 0 0 0-.407-.667zm2.0107-3.0231l-.142-.0852-4.7735-2.7818a.7759.7759 0 0 0-.7854 0L9.409 9.2297V6.8974a.0662.0662 0 0 1 .0284-.0615l4.8303-2.7866a4.4992 4.4992 0 0 1 6.6802 4.66zM8.3065 12.863l-2.02-1.1638a.0804.0804 0 0 1-.038-.0567V6.0742a4.4992 4.4992 0 0 1 7.3757-3.4537l-.142.0805L8.704 5.459a.7948.7948 0 0 0-.3927.6813zm1.0976-2.3654l2.602-1.4998 2.6069 1.4998v2.9994l-2.5974 1.4997-2.6067-1.4997Z' | ||
| ); | ||
|
|
||
| export const SalesforceIcon = createBrandIcon( |
There was a problem hiding this comment.
Priority: P2
Re-vendoring brand marks that upstream withdrew for trademark reasons needs a named owner.
The file's own header (line 4) states these are "Brand marks that @icons-pack/react-simple-icons dropped at the trademark owner's request (removed in 13.15)", and the fix is to copy the 13.8.0 path data into the repo so componentLogoMap keeps rendering them. That converts an upstream removal into a first-party redistribution of the same asset — the trademark exposure moves from the dependency to this repository, for three marks (Slack, OpenAI, Salesforce) shipped in a product UI.
This is a legal/brand decision, not an engineering one, and the diff makes it silently as part of a dependency bump. It is not mine to overrule, but it should be an explicit, attributed choice rather than a side effect of bun update.
Correction: get sign-off from whoever owns brand/legal for the console, and record it in the header comment (approver + date) so the next upgrade does not re-litigate it. If sign-off isn't available, the dependency-free fallback is a neutral glyph — component-logo-map.tsx already falls back to lucide icons (Package, FileJson) for unmapped components, so openai_* and salesforce_* can point at a generic mark without a code-shape change.
Separately, brand-icons.test.tsx:20 only asserts d is truthy, so it cannot detect a truncated or wrong path — the Salesforce path here is a single unclosed subpath, unlike the other two. A snapshot of d would at least pin what ships.
Verify: render /connect with an openai_* and a salesforce_* component and compare the glyphs against the 13.8.0 originals.
Automated /review.
| expect(packageJson.devDependencies?.['@rstest/core']).toBe('0.11.11'); | ||
| expect(packageJson.devDependencies?.['@rstest/coverage-v8']).toBe('0.11.11'); | ||
| // Exact pins: the Rstest packages move in lockstep and pre-1.0 minors break. | ||
| expect(packageJson.devDependencies?.['@module-federation/rstest']).toMatch(EXACT_VERSION); |
There was a problem hiding this comment.
Priority: P3
The comment on line 48 says "Exact pins: the Rstest packages move in lockstep and pre-1.0 minors break", but the new assertions only enforce shape, not lockstep: @module-federation/rstest and @rstest/core are each checked against /^\d+\.\d+\.\d+$/ independently, so @module-federation/rstest: 2.9.2 with @rstest/core: 0.99.0 passes. Only @rstest/coverage-v8 is tied to @rstest/core (line 51).
Relaxing the literal pins is reasonable — they forced a test edit on every bump — but the test now guards less than its comment claims. Either tie the @module-federation/rstest minor to @rstest/core explicitly, or narrow the comment to "exact, non-range pins" so the guarantee matches the assertion.
Verify: set @rstest/core to a mismatched minor in a scratch checkout and confirm the test fails.
Automated /review.
| switch (label) { | ||
| case 'json': | ||
| return new Worker(new URL('monaco-editor/esm/vs/language/json/json.worker', import.meta.url)); | ||
| return new Worker(new URL('monaco-editor/languages/features/json/json.worker', import.meta.url)); |
There was a problem hiding this comment.
Priority: P3
These worker specifiers moved to monaco 0.57's new layout but stayed extensionless (monaco-editor/languages/features/json/json.worker, .../typescript/ts.worker, monaco-editor/editor/editor.worker on line 480), while the aliases added in rsbuild.config.ts:55 and test.shared.ts:15 use the .js form. Under an exports map only the literally-listed subpaths resolve, and extensionless variants are commonly absent — pre-0.56 these deep paths worked because they were not gated by exports at all.
If they don't resolve, new Worker(new URL(...)) fails at bundle time (rspack resolves these statically), so bun run build would catch it — I could not run it here (frontend/node_modules is absent and this review is read-only). Worth confirming rather than assuming, because a partial failure mode is worse than a build break: a wrong worker path degrades JSON/YAML validation and the TypeScript filter editor at runtime with no build error.
Verify: bun install && bun run build, then load a topic's filter editor and a pipeline YAML editor and confirm no Could not create web worker / Unexpected usage in the console. Add .js to all three specifiers if resolution fails.
Automated /review.
| "@testing-library/user-event": "^14.6.7", | ||
| "@types/json-bigint": "^1.0.4", | ||
| "@types/node": "^22.19.1", | ||
| "@types/node": "^26.6.3", |
There was a problem hiding this comment.
Priority: P3
@types/node jumps ^22.19.1 → ^26.6.3 while engines.node (bottom of this file) still declares >=22.22 and CI pins Bun via .bun-version. Node 26 typings advertise APIs that don't exist on a Node 22 runtime, so build scripts and tests/**/*.mjs can typecheck clean and fail at runtime on the declared-minimum engine.
Correction: keep the major of @types/node aligned with engines.node (^22), or raise engines.node to >=26 if that's the intended floor — the two should not disagree.
Verify: bun run type:check, then run node tests/scripts/run-all-variants.mjs under Node 22 and confirm no is not a function on newly-typed APIs.
Automated /review.
| } catch { | ||
| return; | ||
| } | ||
| } catch {} |
There was a problem hiding this comment.
Priority: P3
The autofix turned catch { return; } into catch {} here and in three other places (node-inspector.tsx, utils/yaml.ts ×3). Behavior is identical — the function still yields undefined — but the explicit return; was the only signal that returning undefined was the intended contract for a parse failure; a bare catch {} reads as an accidentally-swallowed error and is the exact shape reviewers are trained to flag.
Not a defect, so no change is required. If you want the intent to survive the next reader, a one-line comment (// malformed schema — callers treat undefined as "no schema") costs nothing and matches the density of the surrounding comments in this file.
Verify: n/a — behavior-preserving.
Automated /review.
Bump every direct dependency to its latest stable release (release-age window overridden). Registry-owned packages stay within the majors the UI registry declares (motion 12, shiki 3, react-day-picker 9, react-dropzone 15, stepperize 5, cel 0.4). Adaptations: - TypeScript 7 stable: `tsc` replaces @typescript/native-preview; rsbuild ambient types cover side-effect CSS/SCSS imports - ultracite 7 / Biome 2.5: new config paths, migrated schema, rstest globals declared, svg/html excluded, lint keeps exit-0 semantics - monaco-editor 0.57: top-level `typescript` namespace, new worker subpaths, alias for monaco-worker-manager's legacy deep import - connect-query 2.3: wrapper reuses UseInfiniteQueryOptions and MessagePageParamKey - react-data-grid beta.61 named export, pretty-ms 9 Options type - simple-icons 13.15 dropped Slack/OpenAI/Salesforce: inline brand icons - module federation 2.9 commonjs container shape in federation tests - happy-dom 20.14 Animation.cancel unhandled rejection shim - react-doctor 0.9: --blocking, ratchet on changed scope Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Output of `bun run lint` with Biome 2.5.14 / ultracite 7 (formatter updates, useBlockStatements, negated-condition flips, redundant returns). No behavior change; two comments merged or dropped by the fixer restored. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Runners default to Node 20, below the engines floor (>=22.22). isolated-vm 6 (via @builder.io/sdk-react 5) ships prebuilds only for Node 22/24, so the Node 20 source build broke bun install. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
TanStack router-core 1.171.30 (#8321) now applies our trailingSlash: 'never' consistently, so navigating to the schema list lands on /schema-registry. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The named `lib-monaco-editor` cache group (enforce, chunks: all) merged every monaco module into one chunk set, so monaco's own lazy language modes and the worker-only TypeScript compiler loaded on the main thread wherever monaco is imported. Unnaming it cuts each federation expose's synchronous load from 3.4 MB to 1.6 MB gzip (master: 2.9 MB). Also drop MonacoWebpackPlugin: MonacoEnvironment.getWorker (src/config.ts) already creates every worker, so the plugin's worker copies were never used (-1.9 MB gzip build output). Both YAML editors call configureMonacoYaml directly, so its monaco-yaml injection was redundant. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Patch/minor releases since the initial bump (protobuf 2.16, TanStack Query 5.104.1 / Router 1.170.41 / Table 9.2.6, lucide-react 1.53, rsbuild 2.2.12, rstest 0.12.3, Playwright 1.64, Biome 2.5.15, ultracite 7.12.4, and others). Registry-owned majors stay held. Bun 1.4.2 in .bun-version and packageManager so the generated yarn.lock matches CI. Remove the now-unused monaco-editor-webpack-plugin. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
23c5569 to
c717e8e
Compare
|
@claude review |
|
Claude finished @malinskibeniamin's task in 1m 40s —— View job Review: PR #2661, Console UI dependency upgrade
Overall: The PR is in good shape. Nearly all of the ~90 I did not run anything. Should fix1. The PR description explains why the exit code is masked, and that's fair for one PR. The problem is how wide the mask is. It also swallows:
A narrower ratchet that keeps CI green: drop 2. React Doctor scans nothing when master is pushed: The workflow runs only Suggested fix: on Worth tightening (non-blocking)3. 4. Federation adapter error message: 5. The rstest pin test claims more than it checks: 6. Monaco worker specifiers are extensionless: 7. Verified / looks good
Minor: the PR description says Biome is 2.5.14 in the table and 2.5.15 under "Others". |
1 participant
Summary
Bumps every direct Console UI dependency to its latest stable release, overriding the 72h release-age window, and adapts the code, config and CI so each upgrade actually takes effect rather than just sitting in the lockfile.
Registry-owned packages stay within the majors the UI registry declares: motion 12.43, shiki/@shikijs 3.23, react-day-picker 9, react-dropzone 15, @stepperize/react 5 and @bufbuild/cel 0.4. calendar and stepper break on the newer majors, so the registry repo needs to migrate first.
Notable upgrades and what we adopt
type:checkrunstsc.@typescript/native-previewis dropped./// <reference types="@rsbuild/core/types" />fixes the side-effect CSS imports that TS 7 now checks by defaultultracite/biome/*and thepresetschema. Rstest globals declared, which clears 1,707 falsenoUndeclaredVariables.noJsxPropsBindoff, since React Compiler memoizes inline callbacks. SVG and HTML excluded from the new formatters. Autofix sweep is in its own commitexportsmap, top-leveltypescriptnamespacelanguages.typescript. Worker URLs use the new subpaths. Aliasedmonaco-worker-manager's legacy deep import in the app and test builds; its latest release still uses the old pathuseInfiniteQueryWithAllPagesreuses the exportedUseInfiniteQueryOptions,MessagePageParamKeyandMessageInitWithPageParaminstead of hand-rolled typescommonjs-moduleexports the container directlycomponents/icons/brand-icons.tsx, tested)--fail-on→--blocking, plus a--scope changed --base origin/masterratchet. The workflow checks out full historyKiBinstead ofkiB, which is the correct IEC symboltrailingSlashconsistentlytrailingSlash: 'never'is now honoured, so the schema list URL is/schema-registry?…. E2E expectations updatedactions/setup-nodewithfrontend/.nvmrc. Runners defaulted to Node 20, below ourenginesfloor of ≥22.22DataGridexportAnimation.cancel(), which fires unhandled rejections (capricorn86/happy-dom#2412)nameon thelib-monaco-editorcache group merged every Monaco module, including its lazy language modes and the worker-only TypeScript compiler, into one chunk set that loaded wherever Monaco is imported.monaco-editor-webpack-pluginbuilt worker copies thatMonacoEnvironment.getWorkeroverridesconfigureMonacoYamlthemselves.bun-versionandpackageManager), and moreLint and doctor gates
Ultracite 6's
fixalways exited 0, so CI's lint job only checked for an autofix diff while about 2,500 diagnostics went unreported. Ultracite 7 propagates the exit code. To keep this PR's CI green,lintand the pre-commit hook keep the old exit-0 behaviour. CI still fails on any autofix diff, andultracite doctorstill guards the config. The 1,334 remaining diagnostics and the 24 react-doctor errors go to a follow-up PR.Bundle impact
Gzip, measured on production builds of master and this branch:
App,EmbeddedApp, …)index)Monaco 0.57 itself ships more code (an LSP client and reorganised language services). Before the cache-group fix the federated synchronous load had grown to 3,375 KB. The fix also takes the TypeScript compiler and CSS language service off the main thread, where they had been loading on master too. Monaco is still imported at app start through
src/config.ts; moving it behind the first editor mount is a follow-up.Test plan
bun run type:checkbun run lint: a second run produces no diff;ultracite doctor5/5bun run buildbun run test:unit: 1,065 passedbun run test:integration: 1,612 passed. Warnings match master: 9 existing yamlmapAsMapnoticesbun run test:federation: passedbun run doctor: no new issues againstorigin/masteruser-error-handlingALREADY_EXISTS anduser-delete-error) failed on the first attempt and passed on retryFollow-ups
loader.config({ monaco })and worker setup out ofsrc/config.ts)src/protogenwith current plugins (buf.build/bufbuild/esv2.2.5 → v2.16,connectrpc/query-esv2.0.1 → v2.3.1) through the proto workflow🤖 Generated with Claude Code