build: bundling configs for .node + proper import of setapp package #217
Conversation
WalkthroughAdds handling for Node native addons to the webpack build by adding a module rule for Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (1)
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 |
nsrCodes
force-pushed
the
draft-setapp-wrapper-support
branch
from
eaf00bf to
98b74d9
Compare
nsrCodes
force-pushed
the
draft-setapp-wrapper-support
branch
from
98b74d9 to
4858c06
Compare
nsrCodes
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.erb/configs/webpack.config.base.ts(1 hunks)package.json(3 hunks)release/app/package.json(3 hunks)src/main/main.ts(1 hunks)
🔇 Additional comments (5)
package.json (2)
4-4: LGTM: Version bump is consistent.The version bump to 25.11.10 matches the corresponding change in
release/app/package.json, maintaining version consistency across the project.
246-246: No issues found — node-loader version is compatible.The latest published version of node-loader is 2.1.0, which is exactly what's specified in the PR. node-loader is maintained for Webpack 5 and is compatible with Webpack v5.x, including the webpack 5.58.2 version in the project. No action needed.
release/app/package.json (2)
4-4: LGTM: Version consistency maintained.The version bump to 25.11.10 is consistent with the main
package.json, ensuring version alignment across the application.
27-27: LGTM: Minor patch update for Setapp framework.The patch update from ^4.3.3 to ^4.3.4 is appropriate. Note that as an
optionalDependency, this package may not be installed in all environments, which is relevant to the usage insrc/main/main.ts..erb/configs/webpack.config.base.ts (1)
23-26: LGTM: Appropriate configuration for native Node.js addons.The new webpack rule correctly handles
.nodenative addon files usingnode-loader. This configuration aligns with theasarUnpackpattern inpackage.json(line 44) and is essential for bundling native modules.
| if (process.env.IS_SETAPP_BUILD === "true") { | ||
| log.log("[SETAPP] build identified") | ||
| const setappFramework = require("@setapp/framework-wrapper"); | ||
| setappFramework.SetappManager.shared.reportUsageEvent(setappFramework.SETAPP_USAGE_EVENT.USER_INTERACTION); | ||
| log.log("[SETAPP] integration complete") | ||
| } |
There was a problem hiding this comment.
Critical: Add error handling for optional dependency.
The dynamic require("@setapp/framework-wrapper") at line 37 will crash the application if the optional dependency is not installed. Since @setapp/framework-wrapper is listed as an optionalDependency in release/app/package.json, it may not be present in all environments.
Apply this diff to add proper error handling:
if (process.env.IS_SETAPP_BUILD === "true") {
log.log("[SETAPP] build identified")
- const setappFramework = require("@setapp/framework-wrapper");
- setappFramework.SetappManager.shared.reportUsageEvent(setappFramework.SETAPP_USAGE_EVENT.USER_INTERACTION);
- log.log("[SETAPP] integration complete")
+ try {
+ const setappFramework = require("@setapp/framework-wrapper");
+ setappFramework.SetappManager.shared.reportUsageEvent(setappFramework.SETAPP_USAGE_EVENT.USER_INTERACTION);
+ log.log("[SETAPP] integration complete")
+ } catch (error) {
+ log.error("[SETAPP] Failed to initialize Setapp framework:", error);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (process.env.IS_SETAPP_BUILD === "true") { | |
| log.log("[SETAPP] build identified") | |
| const setappFramework = require("@setapp/framework-wrapper"); | |
| setappFramework.SetappManager.shared.reportUsageEvent(setappFramework.SETAPP_USAGE_EVENT.USER_INTERACTION); | |
| log.log("[SETAPP] integration complete") | |
| } | |
| if (process.env.IS_SETAPP_BUILD === "true") { | |
| log.log("[SETAPP] build identified") | |
| try { | |
| const setappFramework = require("@setapp/framework-wrapper"); | |
| setappFramework.SetappManager.shared.reportUsageEvent(setappFramework.SETAPP_USAGE_EVENT.USER_INTERACTION); | |
| log.log("[SETAPP] integration complete") | |
| } catch (error) { | |
| log.error("[SETAPP] Failed to initialize Setapp framework:", error); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/main.ts around lines 35 to 40 the dynamic require of
"@setapp/framework-wrapper" can throw if the optional dependency is not
installed; wrap the require and all usage of setappFramework in a try/catch
block so a missing module or runtime error won’t crash the app, log a clear
warning on failure (including the error message) and continue without calling
SetappManager; optionally guard calls with a truthy check (e.g., if
(setappFramework && setappFramework.SetappManager)) before invoking
reportUsageEvent.
🧩 Analysis chain
Verify webpack bundling of optional dependency.
The dynamic require() at line 37 may not work correctly with webpack bundling. Webpack might not include optional dependencies in the bundle, or may require additional configuration. Please verify that @setapp/framework-wrapper is correctly bundled and accessible at runtime in production builds.
Run the following script to check if the Setapp framework is properly bundled in production builds:
🏁 Script executed:
#!/bin/bash
# Description: Verify that @setapp/framework-wrapper is available in production builds
# Check if the package is present in release/app/node_modules
if [ -d "release/app/node_modules/@setapp/framework-wrapper" ]; then
echo "✓ @setapp/framework-wrapper found in release/app/node_modules"
ls -la release/app/node_modules/@setapp/framework-wrapper/
else
echo "✗ @setapp/framework-wrapper NOT found in release/app/node_modules"
fi
# Check webpack externals configuration
echo "Checking webpack externals configuration..."
rg -n "externals" .erb/configs/webpack.config.base.ts -A 5
# Check if @setapp/framework-wrapper is in the externals list
echo "Verifying if @setapp/framework-wrapper is externalized..."
rg -n "@setapp/framework-wrapper" release/app/package.jsonLength of output: 429
🏁 Script executed:
#!/bin/bash
# Check if @setapp/framework-wrapper is in root node_modules
echo "=== Checking root node_modules ==="
if [ -d "node_modules/@setapp/framework-wrapper" ]; then
echo "✓ @setapp/framework-wrapper found in root node_modules"
else
echo "✗ @setapp/framework-wrapper NOT found in root node_modules"
fi
# Check main package.json
echo ""
echo "=== Checking root package.json for @setapp/framework-wrapper ==="
rg -n "@setapp/framework-wrapper" package.json
# Check release/app package.json structure
echo ""
echo "=== Checking release/app/package.json dependencies section ==="
rg -n "dependencies" release/app/package.json -A 20 | head -30
# Check webpack config for how externals are handled in the actual build
echo ""
echo "=== Checking if there are webpack plugins or special handling for externals ==="
rg -n "externals|optional|@setapp" .erb/configs/ -B 2 -A 2Length of output: 1673
🏁 Script executed:
#!/bin/bash
# Check the actual code context around the require statement
echo "=== Checking code context for error handling ==="
rg -n "require.*@setapp" src/main/main.ts -B 10 -A 5
# Check if there's a try-catch or conditional check
echo ""
echo "=== Checking for error handling patterns ==="
rg -n "try|catch|optional|IS_SETAPP_BUILD" src/main/main.ts -B 2 -A 2
# Check webpack config to see if optionalDependencies should be included
echo ""
echo "=== Full webpack externals configuration ==="
rg -n "externals" .erb/configs/webpack.config.base.ts -B 5 -A 10Length of output: 2721
🏁 Script executed:
#!/bin/bash
# Check the actual code context around the require statement
echo "=== Checking code context for error handling ==="
rg -n "require.*@setapp" src/main/main.ts -B 10 -A 5
# Check if there's a try-catch or conditional check
echo ""
echo "=== Checking for error handling patterns ==="
rg -n "try|catch|optional|IS_SETAPP_BUILD" src/main/main.ts -B 2 -A 2
# Check webpack config to see if optionalDependencies should be included
echo ""
echo "=== Full webpack externals configuration ==="
rg -n "externals" .erb/configs/webpack.config.base.ts -B 5 -A 10Length of output: 2721
Wrap require() in try-catch or update webpack config to handle optional dependencies.
The code lacks error handling for a dynamic require of an optional dependency. The package @setapp/framework-wrapper is listed in release/app/package.json as an optionalDependency (line 27) but the webpack configuration at .erb/configs/webpack.config.base.ts only marks dependencies as externals (line 7-10), excluding optional ones. This means webpack will attempt to bundle the package, but since it's not installed in node_modules, the require at line 37 will fail at runtime without error handling.
Fix one of:
- Wrap the require and Setapp calls in a try-catch block, or
- Update webpack externals config to include
optionalDependenciesalongsidedependencies
🤖 Prompt for AI Agents
In src/main/main.ts around lines 35 to 40, the dynamic require of
"@setapp/framework-wrapper" is unprotected and will throw at runtime if the
optional dependency is absent; wrap the require and subsequent SetappManager
calls in a try-catch that safely logs or ignores the failure, ensuring the app
continues to run, or alternatively update .erb/configs/webpack.config.base.ts to
treat optionalDependencies as externals alongside dependencies so the module is
not bundled and runtime require behaves as expected.
| if (process.env.IS_SETAPP_BUILD === "true") { | ||
| log.log("[SETAPP] build identified") | ||
| const setappFramework = require("@setapp/framework-wrapper"); | ||
| setappFramework.SetappManager.shared.reportUsageEvent(setappFramework.SETAPP_USAGE_EVENT.USER_INTERACTION); |
There was a problem hiding this comment.
Major: Reconsider USER_INTERACTION event timing.
Reporting a USER_INTERACTION usage event immediately on app startup (before any actual user interaction) seems semantically incorrect. This event type typically signifies actual user engagement with the application. Consider either:
- Reporting a different event type more appropriate for app initialization (e.g.,
APP_LAUNCHor similar) - Deferring this event until an actual user interaction occurs
🤖 Prompt for AI Agents
In src/main/main.ts around line 38, calling
setappFramework.SetappManager.shared.reportUsageEvent(setappFramework.SETAPP_USAGE_EVENT.USER_INTERACTION)
at startup is semantically incorrect; change the reported event to an
initialization/launch event (e.g., SETAPP_USAGE_EVENT.APP_LAUNCH or similar) or
remove this startup call and move it into actual user interaction handlers
(mouse/keyboard/menu/action listeners) so USER_INTERACTION is only reported when
real user input occurs; update the single call accordingly and ensure any new
event constant exists and is imported/used consistently.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.