Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds rspack as a supported bundler alongside webpack, refactors bundling CLI and dev server paths, consolidates CI cross‑browser testing into a unified platform, extracts route and title-duplication helpers, centralizes workspace ID utilities, and adds/updates multiple integration tests and type/css tweaks. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/build-test.yml (1)
1345-1380:⚠️ Potential issue | 🟡 MinorAdd
bundler-matrixto thetest-donejob'sneedsarray, or document why it's intentionally excluded.The
test-donejob does not listbundler-matrixin itsneedsarray, meaning bundler-matrix failures won't block PR merges. If this is intentional (e.g., rspack support is experimental), add a comment explaining the exclusion. Otherwise, addbundler-matrixto the needs list.tools/cli/src/bundle.ts (1)
235-263:⚠️ Potential issue | 🟠 Major
buildWithWebpackisasyncbutcompiler.run()uses a callback—the returned promise resolves before the build completes.
await BundleCommand.build(pkg)inexecute()resolves immediately, not when the build finishes. Additionally,compiler.close()is never called afterrun(), which is required by webpack to allow plugins to clean up.The same issue exists in
buildWithRspack(lines 318–349).Suggested fix (wrap in a promise and close the compiler)
static async buildWithWebpack(pkg: Package) { process.env.NODE_ENV = 'production'; const logger = new Logger('bundle'); logger.info(`Packing package ${pkg.name} with webpack...`); logger.info('Cleaning old output...'); rmSync(pkg.distPath.value, { recursive: true, force: true }); const config = getWebpackBundleConfigs(pkg); config.parallelism = cpus().length; const compiler = webpack(config); if (!compiler) { throw new Error('Failed to create webpack compiler'); } - compiler.run((error, stats) => { - if (error) { - console.error(error); - process.exit(1); - } - if (stats) { - if (stats.hasErrors()) { - console.error(stats.toString('errors-only')); - process.exit(1); - } else { - console.log(stats.toString('minimal')); + return new Promise<void>((resolve, reject) => { + compiler.run((error, stats) => { + if (error) { + compiler.close(() => {}); + reject(error); + return; } - } + if (stats?.hasErrors()) { + console.error(stats.toString('errors-only')); + compiler.close(() => {}); + reject(new Error('Build failed with errors')); + return; + } + if (stats) { + console.log(stats.toString('minimal')); + } + compiler.close(closeErr => { + if (closeErr) reject(closeErr); + else resolve(); + }); + }); }); }
🤖 Fix all issues with AI agents
In @.github/workflows/build-test.yml:
- Around line 226-231: The BROWSER environment variable in the "Run playwright
tests" step references a matrix variable that no longer exists in the job
definition, causing it to resolve to an empty string. Either remove the BROWSER
environment variable from this step entirely if the tests no longer need it, or
replace the empty matrix.browser reference with an explicit browser value if the
BROWSER variable is still required by the test code.
In `@tools/cli/src/bundle.ts`:
- Around line 273-278: The unsafe cast occurs in the rspack→webpack fallback
path: when bundler === 'rspack' but !isRspackSupportedPackageName(pkg.name) we
call BundleCommand.devWithWebpack(pkg, devServerConfig as
WebpackDevServerConfiguration) which can pass an RspackDevServerConfiguration to
webpack. Fix by removing the unsafe cast: call BundleCommand.devWithWebpack(pkg)
(omit devServerConfig) and add a clear warning (e.g., logger.warn or
console.warn) right before the fallback that the requested 'rspack' bundler is
unsupported for this package and webpack is being used instead; modify the
branch containing bundler, isRspackSupportedPackageName, and the
BundleCommand.devWithWebpack call accordingly.
In `@tools/cli/src/rspack/index.ts`:
- Around line 10-11: Update the rspack integration to enforce or document
minimum compatible versions and a dev split-chunks workaround: check/validate
that Rspack is >=0.3.3 when using sentryWebpackPlugin and ensure
`@sentry/webpack-plugin` is >=1.20.1 before registering sentryWebpackPlugin;
likewise note/vet the `@vanilla-extract/webpack-plugin` usage and add a
development-time configuration or guard to avoid using splitChunks with
VanillaExtractPlugin (or provide a dedicated chunk-splitting config) to avoid
HMR issues. Locate the plugin registrations for sentryWebpackPlugin and
VanillaExtractPlugin and add runtime/version checks or clear comments and a
conditional config branch that applies split-chunks only when safe.
- Around line 225-232: The plugins array currently passes
require(pkg.join('tailwind.config.js').value) as an option to the
'@tailwindcss/postcss' plugin (see the plugins: [...] block and the
pkg.join('tailwind.config.js').exists() check); update this so the
'@tailwindcss/postcss' entry is configured with an empty object (i.e., plugin
tuple becomes ['@tailwindcss/postcss', {}]) instead of passing the JS config,
and ensure any Tailwind configuration is loaded via the CSS `@config` directive
(e.g., add documentation or comments to instruct using '@config
"./tailwind.config.js";' in the CSS entry) rather than supplying the config as
the PostCSS plugin option.
🧹 Nitpick comments (6)
packages/frontend/core/src/desktop/router.tsx (1)
123-141: Inconsistent URL construction in/template/previewloader.Line 134 still manually constructs
/workspace/${workspaceId}/${docId}instead of reusinggetWorkspaceDocPath. Consider using it here too for consistency:Suggested change
return redirect( - `/workspace/${workspaceId}/${docId}?${new URLSearchParams({ + `${getWorkspaceDocPath(workspaceId ?? '', docId ?? '')}?${new URLSearchParams({ isTemplate: 'true',packages/frontend/core/src/modules/workspace-engine/impls/local.ts (1)
167-174: Double deduplication is harmless but redundant.
dedupeWorkspaceIdsis called here at line 168, butsetLocalWorkspaceIdsalready deduplicates the result of the updater function (line 113). The extra call is idempotent so it won't cause issues — just noting it makes the intent explicit at the call site, which is fine.tools/cli/src/webpack/html-plugin.ts (1)
206-206: Return typeWebpackPluginInstance[]may mislead when used with rspack.The function returns
WebpackPluginInstance[], but the plugins now acceptCompilerLike(not the full webpackCompiler). This works via structural subtyping at runtime, but the import ofWebpackPluginInstancefrom'webpack'could cause confusion for maintainers seeing this used in an rspack context.tools/cli/src/rspack/index.ts (1)
608-610:process.env.NODE_ENVis hardcoded to"production"regardless of dev mode.In
createNodeTargetConfig,DefinePluginalways setsprocess.env.NODE_ENVto'"production"'(Line 609), even when thedevvariable (Line 504) istrue. This may be intentional for the server target, but could cause confusion if this config is used in development workflows. Consider gating it ondevor adding a comment explaining the rationale.tools/cli/src/bundle.ts (2)
34-35:WorkerConfigtype is too narrow for how the values are actually used.The objects returned by
createWebpackWorkerTargetConfig/createRspackWorkerTargetConfigare spread intoMultiConfiguration/MultiRspackOptionsarrays (e.g., line 96, 166), meaning they must be full bundler configuration objects—not just{ name: string }. Theascasts silence the type checker, but the generic type erases useful type information ingetBaseWorkerConfigs.Consider parameterizing the return type:
Suggested improvement
-type WorkerConfig = { name: string }; -type CreateWorkerTargetConfig = (pkg: Package, entry: string) => WorkerConfig; +type CreateWorkerTargetConfig<T extends { name: string } = { name: string }> = ( + pkg: Package, + entry: string +) => T; -function getBaseWorkerConfigs( +function getBaseWorkerConfigs<T extends { name: string }>( pkg: Package, - createWorkerTargetConfig: CreateWorkerTargetConfig + createWorkerTargetConfig: CreateWorkerTargetConfig<T> ) {This preserves the full config type through inference, potentially eliminating the need for
ascasts at the call sites.
67-198: Substantial duplication betweengetWebpackBundleConfigsandgetRspackBundleConfigs.These two functions share identical switch-case structures, entry points, and options—differing only in which
create*TargetConfighelpers they call. If a new package or worker entry is added, both functions must be updated in lockstep.Consider a generic
getBundleConfigsthat accepts the create helpers as parameters (similar to howgetBaseWorkerConfigsalready does for workers). This would consolidate the switch logic into one place.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## canary #14434 +/- ##
==========================================
+ Coverage 53.44% 53.81% +0.37%
==========================================
Files 2831 2832 +1
Lines 153338 153319 -19
Branches 22969 22937 -32
==========================================
+ Hits 81945 82513 +568
+ Misses 68170 67407 -763
- Partials 3223 3399 +176
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/build-test.yml (1)
1344-1372:⚠️ Potential issue | 🟡 Minor
bundler-matrixis not included in thetest-donegate job.The new
bundler-matrixjob is absent from theneedslist oftest-done. This means bundler build failures won't block the "3, 2, 1 Launch" status check. If this is intentional (informational-only), consider adding a comment to document it. If bundler regressions should block merges, add it to the list.Proposed fix (if blocking is desired)
needs: - analyze - lint - typecheck - lint-rust - check-git-status - check-yarn-binary + - bundler-matrix - e2e-test - e2e-blocksuite-testtools/cli/src/bundle.ts (1)
241-269:⚠️ Potential issue | 🟠 Major
asyncmethods never awaitcompiler.run— callers get an immediately-resolved promise.Both
buildWithWebpackandbuildWithRspackare declaredasync, butcompiler.run(callback)is fire-and-forget from the promise's perspective.await BundleCommand.build(...)inexecute()(line 224) resolves before compilation finishes.This works today only because
process.exit(1)in the callback terminates on failure, and Node stays alive for the pending callback on success. But any programmatic caller (tests, orchestration scripts) that relies on theawaitwill get a false "done" signal.Wrap
compiler.runin a promise:Proposed fix (shown for webpack; apply same pattern to rspack)
- compiler.run((error, stats) => { - if (error) { - console.error(error); - process.exit(1); - } - if (stats) { - if (stats.hasErrors()) { - console.error(stats.toString('errors-only')); - process.exit(1); - } else { - console.log(stats.toString('minimal')); - } - } - }); + await new Promise<void>((resolve, reject) => { + compiler.run((error, stats) => { + if (error) { + return reject(error); + } + if (stats?.hasErrors()) { + return reject(new Error(stats.toString('errors-only'))); + } + if (stats) { + console.log(stats.toString('minimal')); + } + compiler.close((closeErr) => { + if (closeErr) return reject(closeErr); + resolve(); + }); + }); + });This also avoids the
process.exitcalls, which skip cleanup and prevent proper error propagation.Also applies to: 324-355
🤖 Fix all issues with AI agents
In `@tools/cli/src/bundle.ts`:
- Around line 228-238: In BundleCommand.build, when bundler === 'rspack' but
isRspackSupportedPackageName(pkg.name) is false, add a logger.warn before
returning BundleCommand.buildWithWebpack(pkg) to surface the silent fallback;
the warning should mention that rspack was requested, the package name
(pkg.name), and that webpack will be used instead so callers can see the
implicit fallback.
- Around line 375-378: DEFAULT_DEV_SERVER_CONFIG is currently typed as
WebpackDevServerConfiguration but is passed into RspackDevServer (via merge(...,
DEFAULT_DEV_SERVER_CONFIG, ...)), creating a type mismatch; fix by either
(preferred) adding a separate DEFAULT_RSPACK_DEV_SERVER_CONFIG constant typed as
RspackDevServerConfiguration and use that when constructing RspackDevServer
(update the RspackDevServer instantiation to merge({},
DEFAULT_RSPACK_DEV_SERVER_CONFIG, devServerConfig)), or define a shared subset
type (e.g., DevServerCommonConfig or Partial<WebpackDevServerConfiguration &
RspackDevServerConfiguration>) and retype DEFAULT_DEV_SERVER_CONFIG to that
shared type and use it in both WebpackDevServer and RspackDevServer
constructions; ensure you update the symbol DEFAULT_DEV_SERVER_CONFIG usage in
the WebpackDevServer and RspackDevServer instantiations and adjust imports/types
to reference WebpackDevServerConfiguration and RspackDevServerConfiguration as
needed.
🧹 Nitpick comments (3)
blocksuite/integration-test/src/__tests__/edgeless/shape-dom.spec.ts (1)
8-14: Defaultprecision=2gives a tolerance of only ±0.005 — very tight for pixel values.
toBeCloseTo(expected, 2)checks that|actual - expected| < 0.005. For pixel-based assertions this is effectively exact equality, making the helper's "CloseTo" semantics misleading and the tests potentially flaky if the renderer introduces any sub-pixel rounding.Consider using
precision = 0(~0.5 px tolerance) orprecision = 1(~0.05 px) as the default, which better matches the intent of an approximate pixel comparison.Suggested change
function expectPxCloseTo( value: string, expected: number, - precision: number = 2 + precision: number = 0 ) { expect(Number.parseFloat(value)).toBeCloseTo(expected, precision); }tools/cli/src/bundle.ts (2)
38-40: Thin wrapper adds indirection without value.
assertRspackSupportedPackagejust forwardspkg.nametoassertRspackSupportedPackageName. Consider calling the shared function directly at call sites (lines 137, 326, 362) instead of adding this wrapper.
136-204: Near-identical duplication withgetWebpackBundleConfigs.
getRspackBundleConfigsmirrorsgetWebpackBundleConfigs(lines 68–134) almost line-for-line — the only differences are the imported config creators and the return type. Consider a generic helper parameterized by the config creators to eliminate the duplication, e.g.:function getBundleConfigs<T>( pkg: Package, createHTML: typeof createWebpackHTMLTargetConfig, createNode: typeof createWebpackNodeTargetConfig, createWorker: CreateWorkerTargetConfig ): T[] { /* shared switch logic */ }This would keep the bundler-specific wiring at the call sites while centralizing the package→entry mapping.
darkskygit
force-pushed
the
darksky/improve-test-and-bundle
branch
from
7ee128d to
84dfe9f
Compare
1 participant
PR Dependency Tree
This tree was auto-generated by Charcoal
Summary by CodeRabbit
New Features
Tests & Quality
Bug Fixes
Other