New plugins - #10
Open
listepo wants to merge 16 commits into
Open
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:
📝 WalkthroughWalkthroughThe pull request adds the ChangesRepository tooling
ns-rspack package
NativeScript test app
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant NativeScriptApp
participant MainPage
participant WidgetsGalleryModel
participant TabsPage
NativeScriptApp->>MainPage: navigate to main page
MainPage->>WidgetsGalleryModel: create binding context
MainPage->>WidgetsGalleryModel: dispatch widget interaction
WidgetsGalleryModel->>TabsPage: open tab page
TabsPage->>MainPage: navigate back or close modal
``
</details>
<!-- walkthrough_end -->
<!-- pre_merge_checks_walkthrough_start -->
<details>
<summary>🚥 Pre-merge checks | ✅ 4 | ❌ 1</summary>
### ❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
| :----------------: | :--------- | :------------------------------------------------------------------------------------ | :--------------------------------------------------------------------------------- |
| Docstring Coverage | ⚠️ Warning | Docstring coverage is 61.90% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
<details>
<summary>✅ Passed checks (4 passed)</summary>
| Check name | Status | Explanation |
| :------------------------: | :------- | :------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately identifies the addition of new plugins, which is a major part of the changeset, although it does not specify the Rspack and WASM plugins. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
</details>
</details>
<!-- pre_merge_checks_walkthrough_end -->
<!-- finishing_touch_checkbox_start -->
<details>
<summary>✨ Finishing Touches 💡 1</summary>
<!-- finishing_touch_suggestion:fix_ci -->
<details open>
<summary>🛠️ Fix failing CI checks 💡</summary>
- [ ] <!-- {"checkboxId": "6d21cfe8-ec3f-40e2-9222-b8318b64d3b0", "radioGroupId": "fix-ci-output-choice-group-unknown_comment_id"} --> Create stacked PR
- [ ] <!-- {"checkboxId": "9f0d24fb-b419-4f01-baf0-8b26b6424f34", "radioGroupId": "fix-ci-output-choice-group-unknown_comment_id"} --> Commit on current branch
</details>
<details>
<summary>🧪 Generate unit tests (beta)</summary>
- [ ] <!-- {"checkboxId": "f47ac10b-58cc-4372-a567-0e02b2c3d479", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Create PR with unit tests
- [ ] <!-- {"checkboxId": "6ba7b810-9dad-11d1-80b4-00c04fd430c8", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Commit unit tests in branch `new-plugins`
</details>
</details>
<!-- finishing_touch_checkbox_end -->
<!-- tips_start -->
---
<sub>Comment `@coderabbitai help` to get the list of available commands.</sub>
<!-- tips_end -->
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
nx.json (1)
46-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the deprecated
servetarget unless compatibility requires it.Nx documents
serveTargetNameas deprecated anddevTargetNameas its replacement. The two names represent the same Vite development-server capability. (nx.dev)Remove
serveTargetNameunless existing scripts or projects still requirenx serve.Suggested configuration
"devTargetName": "dev", - "serveTargetName": "serve", "previewTargetName": "preview",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nx.json` around lines 46 - 50, Remove the deprecated serveTargetName entry from the Nx configuration, while retaining devTargetName as the development-server target. Preserve serveTargetName only if repository scripts or projects explicitly depend on nx serve.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nx.json`:
- Line 60: Update the preVersionCommand release hook in nx.json to invoke Nx
through the repository-pinned pnpm executable, replacing the npx command with
pnpm exec nx while preserving the existing run-many build arguments.
In `@packages/ns-wamr/package.json`:
- Around line 158-159: Update the typings.ios and typings.android scripts in
package.json to execute their respective ns typings commands from the
apps/ns-wasm-test working directory, so generated declarations are written to
that app’s typings directory.
---
Nitpick comments:
In `@nx.json`:
- Around line 46-50: Remove the deprecated serveTargetName entry from the Nx
configuration, while retaining devTargetName as the development-server target.
Preserve serveTargetName only if repository scripts or projects explicitly
depend on nx serve.
🪄 Autofix
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 94995fd9-0c17-4fed-8215-abffeff53652
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
.gitignorenx.jsonpackage.jsonpackages/ns-wamr/package.jsonpnpm-workspace.yaml
Fixes Applied SuccessfullyFixed 2 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Replaces the hello-world page with a live gallery of every
@nativescript/core UI widget: text, inputs, pickers, images,
HtmlView/WebView, ListView/Repeater, Placeholder, all layouts,
gestures, animations, dialogs, and Frame/Page/TabView navigation.
The app root is now a RootLayout wrapping the Frame so the popup
and shade-cover demo exercises the real API.
Verified on the iOS simulator. Four core quirks worked around:
- {{ $value }} over a primitive array is a boxed String, which
colour-typed properties cannot parse — item templates bind named
object properties instead
- TabView is controller-backed on iOS and fills the whole Page, so
it is the page's only content and Close lives inside a tab
- @nativescript/theme sets text-transform:capitalize on TabView and
it inherits into tab content — overridden on .gallery
- the template logo.png is white on transparent, so the Image demos
sit on a dark backdrop
SplitView is documented rather than demoed: at compact width the
collapsing UISplitViewController loops re-pushing nav controllers,
so it cannot be hosted in a Frame-pushed page.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/rspack-test-app/nativescript.config.ts (1)
3-11: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet the NativeScript CLI package manager explicitly.
If this app can be run through NativeScript CLI, add
cli.packageManager: 'pnpm'. The repository-localapps/ns-wasm-test/nativescript.config.tsuses this setting. NativeScript otherwise uses the CLI-wide default, ornpmwhen no default is set. A different default can produce a dependency tree that differs from CI. (docs.nativescript.org)Suggested configuration change
appResourcesPath: 'App_Resources', + cli: { + packageManager: 'pnpm' + }, android: {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/rspack-test-app/nativescript.config.ts` around lines 3 - 11, Update the exported NativeScript configuration object to include a cli.packageManager setting of 'pnpm', matching the existing configuration pattern in the repository and ensuring CLI installs use pnpm.apps/rspack-test-app/App_Resources/Android/src/main/AndroidManifest.xml (1)
11-12: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRemove legacy storage permissions when no storage feature requires them.
The manifest declares
READ_EXTERNAL_STORAGEandWRITE_EXTERNAL_STORAGE, but the supplied app objective is a widgets gallery and no storage access is shown. If omitted app code does not require these permissions, remove them to keep the permission surface minimal.Proposed cleanup
- <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE"/> - <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/rspack-test-app/App_Resources/Android/src/main/AndroidManifest.xml` around lines 11 - 12, Remove the READ_EXTERNAL_STORAGE and WRITE_EXTERNAL_STORAGE declarations from the Android manifest, provided no app code requires storage access. Keep the manifest focused on permissions needed by the widgets gallery.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/rspack-test-app/app/app.css`:
- Around line 25-27: Update the dark-mode card styles in .ns-dark .card and the
related title and hint selectors around the referenced card content to set
accessible light text colors against the `#212121` background. Preserve the
existing dark card background while ensuring both card titles and hint text meet
sufficient contrast.
In `@apps/rspack-test-app/app/tabs-page.xml`:
- Line 3: Update the Android NavigationButton in tabs-page to bind its tap
action to the existing onClose handler by adding the tap binding, while
preserving the current text and system icon attributes.
In `@apps/rspack-test-app/package.json`:
- Around line 6-15: Add the NativeScript 9.0-compatible `@nativescript/android`
package to the devDependencies in package.json, then regenerate or update the
corresponding package-lock.json entry while preserving the existing dependency
versions.
---
Nitpick comments:
In `@apps/rspack-test-app/App_Resources/Android/src/main/AndroidManifest.xml`:
- Around line 11-12: Remove the READ_EXTERNAL_STORAGE and WRITE_EXTERNAL_STORAGE
declarations from the Android manifest, provided no app code requires storage
access. Keep the manifest focused on permissions needed by the widgets gallery.
In `@apps/rspack-test-app/nativescript.config.ts`:
- Around line 3-11: Update the exported NativeScript configuration object to
include a cli.packageManager setting of 'pnpm', matching the existing
configuration pattern in the repository and ensuring CLI installs use pnpm.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ce3704b5-c38b-45f4-b949-8f93f89f703c
⛔ Files ignored due to path filters (40)
apps/rspack-test-app/App_Resources/Android/src/main/res/drawable-hdpi/background.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/Android/src/main/res/drawable-hdpi/logo.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/Android/src/main/res/drawable-ldpi/background.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/Android/src/main/res/drawable-ldpi/logo.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/Android/src/main/res/drawable-mdpi/background.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/Android/src/main/res/drawable-mdpi/logo.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/Android/src/main/res/drawable-xhdpi/background.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/Android/src/main/res/drawable-xhdpi/logo.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/Android/src/main/res/drawable-xxhdpi/background.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/Android/src/main/res/drawable-xxhdpi/logo.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/Android/src/main/res/drawable-xxxhdpi/background.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/Android/src/main/res/drawable-xxxhdpi/logo.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/Android/src/main/res/mipmap-hdpi/ic_launcher.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/Android/src/main/res/mipmap-mdpi/ic_launcher.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/Android/src/main/res/mipmap-xhdpi/ic_launcher.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/Android/src/main/res/mipmap-xxhdpi/ic_launcher.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/Android/src/main/res/mipmap-xxxhdpi/ic_launcher.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/AppIcon.appiconset/icon-1024.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/AppIcon.appiconset/icon-20.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/AppIcon.appiconset/icon-20@2x.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/AppIcon.appiconset/icon-20@3x.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/AppIcon.appiconset/icon-29.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/AppIcon.appiconset/icon-29@2x.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/AppIcon.appiconset/icon-29@3x.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/AppIcon.appiconset/icon-40.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/AppIcon.appiconset/icon-40@2x.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/AppIcon.appiconset/icon-40@3x.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/AppIcon.appiconset/icon-60@2x.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/AppIcon.appiconset/icon-60@3x.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/AppIcon.appiconset/icon-76.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/AppIcon.appiconset/icon-76@2x.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/AppIcon.appiconset/icon-83.5@2x.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/LaunchScreen.AspectFill.imageset/LaunchScreen-AspectFill.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/LaunchScreen.AspectFill.imageset/LaunchScreen-AspectFill@2x.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/LaunchScreen.AspectFill.imageset/LaunchScreen-AspectFill@3x.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/LaunchScreen.Center.imageset/LaunchScreen-Center.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/LaunchScreen.Center.imageset/LaunchScreen-Center@2x.pngis excluded by!**/*.pngapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/LaunchScreen.Center.imageset/LaunchScreen-Center@3x.pngis excluded by!**/*.pngapps/rspack-test-app/app/images/logo.pngis excluded by!**/*.pngapps/rspack-test-app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
apps/rspack-test-app/.editorconfigapps/rspack-test-app/.gitignoreapps/rspack-test-app/.vscode/extensions.jsonapps/rspack-test-app/App_Resources/Android/app.gradleapps/rspack-test-app/App_Resources/Android/before-plugins.gradleapps/rspack-test-app/App_Resources/Android/src/main/AndroidManifest.xmlapps/rspack-test-app/App_Resources/Android/src/main/res/drawable-nodpi/splash_screen.xmlapps/rspack-test-app/App_Resources/Android/src/main/res/drawable/ic_launcher_foreground.xmlapps/rspack-test-app/App_Resources/Android/src/main/res/mipmap-anydpi-v26/ic_launcher.xmlapps/rspack-test-app/App_Resources/Android/src/main/res/values-v21/colors.xmlapps/rspack-test-app/App_Resources/Android/src/main/res/values-v21/styles.xmlapps/rspack-test-app/App_Resources/Android/src/main/res/values-v29/styles.xmlapps/rspack-test-app/App_Resources/Android/src/main/res/values/colors.xmlapps/rspack-test-app/App_Resources/Android/src/main/res/values/ic_launcher_background.xmlapps/rspack-test-app/App_Resources/Android/src/main/res/values/styles.xmlapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/AppIcon.appiconset/Contents.jsonapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/Contents.jsonapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/LaunchScreen.AspectFill.imageset/Contents.jsonapps/rspack-test-app/App_Resources/iOS/Assets.xcassets/LaunchScreen.Center.imageset/Contents.jsonapps/rspack-test-app/App_Resources/iOS/Info.plistapps/rspack-test-app/App_Resources/iOS/LaunchScreen.storyboardapps/rspack-test-app/App_Resources/iOS/build.xcconfigapps/rspack-test-app/app/app-root.xmlapps/rspack-test-app/app/app.cssapps/rspack-test-app/app/app.tsapps/rspack-test-app/app/main-page.tsapps/rspack-test-app/app/main-page.xmlapps/rspack-test-app/app/main-view-model.tsapps/rspack-test-app/app/tabs-page.tsapps/rspack-test-app/app/tabs-page.xmlapps/rspack-test-app/nativescript.config.tsapps/rspack-test-app/package.jsonapps/rspack-test-app/references.d.tsapps/rspack-test-app/tsconfig.jsonapps/rspack-test-app/webpack.config.js
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
packages/ns-rspack/src/lib/compat.ts (2)
184-198: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGuard the entry-exports shim against an empty entry set.
If
entryPointsis empty,patternis an empty string. The generated test is/^()\.[cm]?js$/, which matches no real asset and still registersNsEntryExportsShim. Skip the plugin when there are no entries.♻️ Proposed guard
const entries = [...(config.entryPoints as unknown as ChainedMapLike).store.keys()] + + if (entries.length === 0) { + return config + } + const pattern = entries.map((name) => name.replace(/[.*+?^${}()|[\]\\/]/g, '\\$&')).join('|')Note: an early
returnhere would skip the laterWatchStatePluginand Terser steps. Wrap the block in anif (entries.length > 0)condition instead if you keep the flow linear.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ns-rspack/src/lib/compat.ts` around lines 184 - 198, Guard the NsEntryExportsShim registration using the entries collected from config.entryPoints in the non-outputModule path. Only create the pattern and register the BannerPlugin when entries.length is greater than zero; keep the surrounding flow linear so later WatchStatePlugin and Terser processing still executes.
68-72: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
toRspackGlobcovers only the+(...)extglob form.globby also supports
@(...),?(...),*(...), and!(...). A user copy rule that uses those forms silently copies nothing, the same failure this function exists to prevent. Extend the rewrite for@(a|b)and?(a|b), or document the limitation in the README.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ns-rspack/src/lib/compat.ts` around lines 68 - 72, The toRspackGlob function only rewrites +(…) extglobs, leaving supported @ (a|b) and ?(a|b) patterns unusable. Extend its rewrite logic to handle the requested @ and ? forms while preserving the existing +(…) conversion, or document this limitation in the README.packages/ns-rspack/src/index.ts (1)
71-76: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winA caller-supplied chain skips
adaptChain.
resolveConfigappliesadaptChainonly throughresolveChainableConfig(). If a caller builds a chain and passes it directly, the webpack-only plugins andts-loaderstay in place, and the build fails at compile time.adaptChainis effectively idempotent for an already-adapted chain, so apply it unconditionally.♻️ Proposed fix
export function resolveConfig(chainableConfig?: RspackChain): Configuration { - const chain = chainableConfig ?? resolveChainableConfig() + const chain = chainableConfig ? adaptChain(chainableConfig) : resolveChainableConfig() const resolved = nsWebpack.resolveConfig(asWebpackChain(chain))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ns-rspack/src/index.ts` around lines 71 - 76, Update resolveConfig to apply adaptChain unconditionally to the selected chain, including caller-supplied chainableConfig, before passing it to asWebpackChain and nsWebpack.resolveConfig. Preserve the existing resolveChainableConfig path while ensuring already-adapted chains remain supported.packages/ns-rspack/src/lib/watch-state-plugin.ts (1)
36-70: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCall
callback()after the hook work, not before.Both hooks signal completion first and then read state and send IPC. Two consequences follow. First, the compiler can continue past the hook while the plugin still reads
compilation.getStats(), so the reported state can belong to a compilation the compiler has already moved on from in watch mode. Second, any exception thrown aftercallback()escapes the hook and becomes an unhandled error instead of a compilation error. Move eachcallback()to the end of the hook.♻️ Proposed reordering for `afterEmit`
compiler.hooks.afterEmit.tapAsync(id, (compilation, callback) => { - callback() - if (this.options.stats) { @@ this.notify({ type: 'compilation', version: 1, hash: compilation.hash ?? undefined, data: { emittedAssets, staleAssets }, }) + + callback() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ns-rspack/src/lib/watch-state-plugin.ts` around lines 36 - 70, Move the callback() invocation to the end of both the watchRun and afterEmit handlers, after all state updates, logging, stats processing, and notify work completes. Preserve the existing hook logic, ensuring exceptions from that work occur before signaling completion and the compiler cannot proceed until each handler finishes.packages/ns-rspack/src/lib/compat.spec.ts (1)
30-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact replacement plugin.
not.toBe(FakeCustomPlugin)passes for any substitution, including a wrong one. Assert the exportedContextExclusionPlugininstead.♻️ Proposed stronger assertion
-import { adaptChain, adaptConfig } from './compat.js' +import { ContextExclusionPlugin, adaptChain, adaptConfig } from './compat.js'- expect(config.plugin('ContextExclusionPlugin|App_Resources').get('plugin')).not.toBe( - FakeCustomPlugin, - ) + expect(config.plugin('ContextExclusionPlugin|App_Resources').get('plugin')).toBe( + ContextExclusionPlugin, + ) + expect(config.plugin('ContextExclusionPlugin|App_Resources').get('args')).toEqual([/x/])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ns-rspack/src/lib/compat.spec.ts` around lines 30 - 40, Update the assertion in the plugin registration test to verify that the adapted plugin is exactly the exported ContextExclusionPlugin, replacing the broad not.toBe(FakeCustomPlugin) check while preserving the existing lookup key and adaptation flow.packages/ns-rspack/src/bin/index.ts (1)
104-112: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWatch mode never closes the compiler.
compiler.watchreturns aWatchinghandle. The CLI discards it, so no code path callswatching.close(). OnSIGINTorSIGTERMthe process exits with the file watchers and the output file system still active. Keep the handle and close it on termination signals.♻️ Proposed handling
- compiler.watch(configuration.watchOptions ?? {}, (err, stats) => report(env, err, stats)) + const watching = compiler.watch(configuration.watchOptions ?? {}, (err, stats) => + report(env, err, stats), + ) + + for (const signal of ['SIGINT', 'SIGTERM'] as const) { + process.once(signal, () => { + watching.close(() => process.exit(0)) + }) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ns-rspack/src/bin/index.ts` around lines 104 - 112, Update the watch-mode branch around compiler.watch to retain its returned Watching handle and register SIGINT and SIGTERM handlers that call watching.close() before termination. Preserve the existing report callback and normal watch-mode flow, ensuring cleanup occurs only for the active watch instance.apps/rspack-test-app/nativescript.config.ts (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the compatibility type.
bundler: stringaccepts invalid bundler names and weakens compile-time checking. Keep the NativeScript bundler type and add only'rspack'. Usesatisfiesinstead ofasso the remaining fields stay checked. The NativeScript CLI has a dedicated Rspack path; unknown values can fall through to webpack executable resolution. (github.com)This follows the supplied NativeScript CLI and package context.
Proposed type-safe declaration
-} as Omit<NativeScriptConfig, 'bundler'> & { bundler: string }; +} satisfies Omit<NativeScriptConfig, 'bundler'> & { + bundler: NativeScriptConfig['bundler'] | 'rspack'; +};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/rspack-test-app/nativescript.config.ts` at line 21, Update the NativeScript config declaration to preserve the existing NativeScript bundler type while extending it with only the literal 'rspack'. Replace the broad type assertion with satisfies so all remaining configuration fields continue to receive structural validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ns-rspack/src/bin/index.ts`:
- Around line 86-88: Update the environment setup around env.config and
process.env.NATIVESCRIPT_CONFIG_NAME so the variable is assigned only when
env.config is a string. Preserve the existing nullish assignment behavior when a
valid config name is provided, but avoid assigning when env.config is absent to
prevent the literal "undefined" value.
- Around line 27-28: Update parseEnvFlags to convert exact string values "true"
and "false" into boolean true and false while preserving all other values as
strings. Ensure the resulting env.stats value is a boolean so the existing
checks in report and the compatibility logic honor --env.stats=false.
In `@packages/ns-rspack/src/lib/compat.ts`:
- Around line 18-32: Update ContextExclusionPlugin.apply and its
contextModuleFactory hook integration so missing contextModuleFiles support
cannot silently disable exclusions: use a supported ContextModuleFactory hook
for filtering, or throw an explicit compatibility error when no supported hook
exists. Preserve negativeMatcher filtering for supported runtimes.
---
Nitpick comments:
In `@apps/rspack-test-app/nativescript.config.ts`:
- Line 21: Update the NativeScript config declaration to preserve the existing
NativeScript bundler type while extending it with only the literal 'rspack'.
Replace the broad type assertion with satisfies so all remaining configuration
fields continue to receive structural validation.
In `@packages/ns-rspack/src/bin/index.ts`:
- Around line 104-112: Update the watch-mode branch around compiler.watch to
retain its returned Watching handle and register SIGINT and SIGTERM handlers
that call watching.close() before termination. Preserve the existing report
callback and normal watch-mode flow, ensuring cleanup occurs only for the active
watch instance.
In `@packages/ns-rspack/src/index.ts`:
- Around line 71-76: Update resolveConfig to apply adaptChain unconditionally to
the selected chain, including caller-supplied chainableConfig, before passing it
to asWebpackChain and nsWebpack.resolveConfig. Preserve the existing
resolveChainableConfig path while ensuring already-adapted chains remain
supported.
In `@packages/ns-rspack/src/lib/compat.spec.ts`:
- Around line 30-40: Update the assertion in the plugin registration test to
verify that the adapted plugin is exactly the exported ContextExclusionPlugin,
replacing the broad not.toBe(FakeCustomPlugin) check while preserving the
existing lookup key and adaptation flow.
In `@packages/ns-rspack/src/lib/compat.ts`:
- Around line 184-198: Guard the NsEntryExportsShim registration using the
entries collected from config.entryPoints in the non-outputModule path. Only
create the pattern and register the BannerPlugin when entries.length is greater
than zero; keep the surrounding flow linear so later WatchStatePlugin and Terser
processing still executes.
- Around line 68-72: The toRspackGlob function only rewrites +(…) extglobs,
leaving supported @ (a|b) and ?(a|b) patterns unusable. Extend its rewrite logic
to handle the requested @ and ? forms while preserving the existing +(…)
conversion, or document this limitation in the README.
In `@packages/ns-rspack/src/lib/watch-state-plugin.ts`:
- Around line 36-70: Move the callback() invocation to the end of both the
watchRun and afterEmit handlers, after all state updates, logging, stats
processing, and notify work completes. Preserve the existing hook logic,
ensuring exceptions from that work occur before signaling completion and the
compiler cannot proceed until each handler finishes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fb9ac7c9-fa39-4ecc-b8d5-605083909380
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
apps/rspack-test-app/nativescript.config.tsapps/rspack-test-app/package.jsonapps/rspack-test-app/rspack.config.tsapps/rspack-test-app/tsconfig.jsonpackage.jsonpackages/ns-rspack/README.mdpackages/ns-rspack/package.jsonpackages/ns-rspack/src/bin/index.tspackages/ns-rspack/src/index.tspackages/ns-rspack/src/lib/compat.spec.tspackages/ns-rspack/src/lib/compat.tspackages/ns-rspack/src/lib/watch-state-plugin.tspackages/ns-rspack/tsconfig.jsonpackages/ns-rspack/tsconfig.lib.jsonpackages/ns-rspack/tsconfig.spec.jsonpackages/ns-rspack/vitest.config.mtspnpm-workspace.yamltsconfig.json
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/rspack-test-app/package.json
- apps/rspack-test-app/tsconfig.json
- package.json
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ns-rspack/AGENTS.md`:
- Line 25: Update the build command reference in AGENTS.md to invoke the
workspace-local Nx through pnpm, changing the `nx run ns-rspack:build` example
to `pnpm exec nx run ns-rspack:build` while preserving the surrounding
documentation.
- Line 19: Add language identifiers to both fenced code blocks in AGENTS.md:
mark the architecture listing as text and the command block as bash or console,
preserving their existing contents.
In `@README.md`:
- Around line 233-235: Update the shared WebAssembly documentation row in
README.md to accurately reflect the newly added `@cross-code/ns-wasm-edge`,
`@cross-code/ns-wasm-chicory`, and `@cross-code/ns-endive` packages, listing all six
plugins or explicitly limiting WASM.md’s scope to the original wasm3, WAMR, and
WasmKit plugins.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c5fedc77-d393-425c-a6db-7f594ccfd420
📒 Files selected for processing (3)
AGENTS.mdREADME.mdpackages/ns-rspack/AGENTS.md
- --env.<key>=true/false now yield real booleans, matching webpack's CLI - only set NATIVESCRIPT_CONFIG_NAME when a config name was actually passed - ContextExclusionPlugin throws when the bundler exposes no contextModuleFiles hook instead of silently bundling excluded files - test app: wire NavigationButton back to onClose, dark-theme card colors
- bump TypeScript ~5.4.0 → ~6.0.3 to match the workspace (the shared tsconfig.base.json uses noUncheckedSideEffectImports, a TS ≥5.6 option) - add @nativescript/types-android/types-ios as direct devDependencies — the umbrella @nativescript/types references them via ../ paths, and pnpm only links direct deps into node_modules/@NativeScript - tsconfig: ignoreDeprecations 6.0 (baseUrl), composite + outDir/tsBuildInfoFile so the nx typecheck target (tsc --build --emitDeclarationOnly) works - root tsconfig: nx sync registers the app as a project reference
… webpack
Both apps now set bundler: 'rspack' in nativescript.config.ts and ship a
rspack.config.ts; @nativescript/webpack, ts-loader and postcss-loader are
replaced by @nativescript/rspack (file: alias to @cross-code/ns-rspack,
the name the {N} CLI resolves the bundler by). The ns-wasm-test WASM
copy rules and Vitest test entry move from webpack.config.js into
rspack.config.ts.
- vitest-ns webpack.cjs helper is now bundler-agnostic: it calls
chainRspack when the module exposes it, chainWebpack otherwise — the
chain API is identical in both
- add @nativescript/types-android/types-ios devDeps to both apps so the
umbrella @nativescript/types references resolve (pnpm only links
direct deps)
- ns-wasm-test/nativescript.config.ts type widened for bundler: 'rspack'
- project.json typecheck inputs track rspack.config.ts
- CI: build ns-rspack before the app installs (file: snapshot), and
trigger app jobs when packages/ns-rspack changes
- rspack-test-app tsconfig: nx sync's TS6 rewrite (drop baseUrl,
relative paths, ignoreDeprecations no longer needed)
The standalone apps are their own pnpm workspace roots; they now expose the sibling packages they consume through the workspace: protocol (pnpm-workspace.yaml packages: ../../packages/...), replacing file: dependencies. The apps stay separate installs with their own lockfiles — only the listed packages are reachable, never the root workspace. - ns-wasm-test: ns-wasm-core override dropped; the plugins' workspace:* core reference now resolves against the app's own workspace - ns-wry-app: ns-wry and ns-rspack linked via workspace: - CI: reword the build-before-install comments (workspace: links, no file: snapshotting) - AGENTS.md: package wiring section updated
rspack 2.x's ContextModuleFactory exposes only beforeResolve/afterResolve and enumerates context files in Rust, so the contextModuleFiles hook never exists — the previous fail-loudly behavior broke every rspack build at plugin apply time (the wasm-test app bundle failed, ns run exited 127, and vitest then timed out tearing the device worker down). The exclusion is best-effort: the ~/ context is scoped to the app dir (App_Resources is a sibling), and platform-suffixed files are rare. Warn once instead of throwing; keep the hook tap for bundlers that do expose it. Verified end-to-end: the ns-wasm-test iOS bundle compiles, emits the vitest-ns worker entry, enumerates all 8 spec files via require.context, and copies both .wasm fixtures.
The app's hooks dir had two before-checkForChanges trampolines: nativescript-core.mjs (core 9.0.20, correct) and nativescript-core.js (stale, requiring a .js cli-hook that core 9.0.20 does not ship). The CLI loads every file in the hooks dir, so the stale .js one threw 'Cannot find module @nativescript/core/cli-hooks/before-checkForChanges.js' after the first rspack compilation and ns run exited 127. The .js trampoline is written by @nativescript/core 8.9.9's postinstall (@nativescript/hook 2.0.0). It entered the app's install because the app workspace now includes packages/* as members and pnpm installs their devDependencies — ns-wamr declared a vestigial @nativescript/core ^8.9.0 devDep that nothing imports. - remove the unused @nativescript/core devDep from packages/ns-wamr - regenerate root + app lockfiles (core 8.9.9 and hook 2.0.0 gone) - delete the stale nativescript-core.js trampolines in both apps (hooks/ is gitignored; core 9.0.20's postinstall regenerates the .mjs hook) - nx sync: apps' tsconfigs gain the ns-rspack project reference - ci.yml: drop continue-on-error from wasm-test-android per the never-continue-on-error policy (AGENTS.md)
Rstest has no custom-pool API, so @cross-code/ns-rstest owns its own Node host: runNativeScriptTests() globs the spec files, launches the NativeScript CLI (ns run --env.rstestNativeScript), round-robins files across device worker slots, and reports through Rstest's Reporter interface. On device it drives Rstest's own browser runtime (@rstest/core/internal/browser-runtime) inside a Worker; a shim on globalThis['@rstest/core'] forwards the per-file API Rstest publishes, and the optional results view ships from @cross-code/ns-rstest/ui. - new package packages/ns-rstest (Node host, protocol, worker runtime, shim, UI); deletes packages/vitest-ns and packages/vitest-ns-ui - app: vitest-ns.ts/.worker.ts -> ns-rstest.ts/.worker.ts; specs import from @rstest/core; host runners are ns-rstest.ios.mts / .android.mts run via node (type stripping); rspack.config.ts swaps the bundler hook for configureNativeScriptRstest (env.rstestNativeScript, Istanbul coverage) - nx-ns-app: drop the vitest pool option from NsTestOptions - CI/docs: vitest-ns references -> ns-rstest; device jobs build ns-rstest before installing the app - crlf -> lf normalization in ci.yml / code-review.yml Verified: 16-project test+typecheck+build all pass; the ns-rstest host launches the CLI, rspack emits the worker bundle with all 8 spec files enumerated, and the rstest entry swaps in under env.rstestNativeScript.
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
1 participant
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.
Summary by CodeRabbit
New Features
Documentation
Tests