-
Notifications
You must be signed in to change notification settings - Fork 22
fix: example #610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: onekey
Are you sure you want to change the base?
fix: example #610
Conversation
WalkthroughThe refactor removes the shared-constants workspace and consolidates its functionality. Axios support is added to expo-example via a compatibility shim. The CONNECT_SRC constant now resolves directly from environment variables instead of a shared function. Changes
Sequence DiagramsequenceDiagram
participant App as App/Bundler
participant Webpack as Webpack
participant Shim as shim-axios.js
participant Axios as axios
App->>Webpack: import axios
Webpack->>Webpack: alias to shim
Webpack->>Shim: resolve
Shim->>Axios: require('axios')
Axios-->>Shim: axios object
Shim->>Shim: ensure default export
Shim->>Shim: ensure interceptors mock
Shim-->>Webpack: augmented axios
Webpack-->>App: shimmed module
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
package.json(1 hunks)packages/connect-examples/expo-example/package.json(1 hunks)packages/connect-examples/expo-example/shim-axios.js(1 hunks)packages/connect-examples/expo-example/src/constants/connect.ts(1 hunks)packages/connect-examples/expo-example/webpack.config.js(1 hunks)packages/connect-examples/shared-constants/constants.js(0 hunks)packages/connect-examples/shared-constants/package.json(0 hunks)
💤 Files with no reviewable changes (2)
- packages/connect-examples/shared-constants/constants.js
- packages/connect-examples/shared-constants/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: deploy_preview
- GitHub Check: lint (22)
- GitHub Check: build (22)
🔇 Additional comments (4)
packages/connect-examples/expo-example/src/constants/connect.ts (1)
3-3: Environment-based config is cleaner than the old shared function.The fallback URL version (1.1.17) matches the package version. Logic is clear.
packages/connect-examples/expo-example/webpack.config.js (1)
58-63: Axios alias configuration looks correct.Using
require.resolveensures the shim path is resolved correctly. Spreading existing aliases prevents conflicts.packages/connect-examples/expo-example/shim-axios.js (1)
9-12: The .default property fix handles Rollup interop correctly.This ensures
axios__default["default"]resolves properly when Rollup compiles the code.packages/connect-examples/expo-example/package.json (1)
42-42: Axios 1.12.2 is valid and has no known vulnerabilities.Version 1.12.2 exists on npm (released Sep 14, 2025) and is only one minor version behind the latest (1.13.2). None of the known HIGH or MODERATE severity advisories affect this version—all reported vulnerabilities were patched in earlier updates that 1.12.2 includes.
| "@rollup/plugin-babel": "^5.3.1", | ||
| "@rollup/plugin-commonjs": "^22.0.0", | ||
| "@rollup/plugin-json": "^4.1.0", | ||
| "@rollup/plugin-node-resolve": "^16.0.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Verify @rollup/plugin-node-resolve is needed at root level.
Check if this plugin is used by packages other than expo-example. If only expo-example needs it, move it there.
🏁 Script executed:
#!/bin/bash
# Search for rollup config files and @rollup/plugin-node-resolve usage
echo "=== Finding rollup config files ==="
fd -e js -e ts -e mjs "rollup.config" --type f
echo ""
echo "=== Searching for @rollup/plugin-node-resolve usage ==="
rg -n --type=js --type=ts "plugin-node-resolve" -C 2
echo ""
echo "=== Checking package.json files for rollup dependencies ==="
fd "package.json" --type f --exec grep -l "rollup" {}Length of output: 624
🏁 Script executed:
#!/bin/bash
# Check for @rollup/plugin-node-resolve in all package.json files
echo "=== Checking root package.json ==="
grep -n "@rollup/plugin-node-resolve" ./package.json
echo ""
echo "=== Checking expo-example package.json ==="
grep -n "@rollup/plugin-node-resolve" ./packages/expo-example/package.json 2>/dev/null || echo "File not found or no match"
echo ""
echo "=== Searching all package.json files for @rollup/plugin-node-resolve ==="
fd "package.json" --type f --exec grep -l "@rollup/plugin-node-resolve" {}
echo ""
echo "=== Checking build scripts in root package.json ==="
grep -A 5 '"scripts"' ./package.json | head -20Length of output: 514
🏁 Script executed:
#!/bin/bash
# Check which packages have rollup and how they use it
echo "=== Packages with rollup dependencies ==="
fd "package.json" --type f --exec grep -l "rollup" {} | xargs grep -l '"rollup"' | while read f; do
echo "File: $f"
grep -E '"rollup"|"@rollup' "$f" | head -3
echo ""
done
echo ""
echo "=== Build scripts in packages with rollup ==="
fd "package.json" --type f -path "*/packages/*/package.json" --exec grep -l "rollup" {} | while read f; do
echo "=== $f ==="
grep -A 10 '"scripts"' "$f" 2>/dev/null | grep -E '"scripts"|build|rollup' || echo "No build script found"
echo ""
doneLength of output: 396
🏁 Script executed:
#!/bin/bash
# Check build scripts that reference rollup
echo "=== Build scripts in root package.json ==="
grep -A 20 '"scripts"' ./package.json | grep -E 'rollup|build'
echo ""
echo "=== All package.json files mentioning rollup in scripts ==="
fd "package.json" -type f --exec grep -l "rollup" {} \; | while read f; do
if grep -q '"scripts"' "$f"; then
echo "=== $f ==="
grep -A 30 '"scripts"' "$f" | grep -B 2 -A 2 "rollup"
fi
done
echo ""
echo "=== Direct check: Does expo-example exist? ==="
ls -la packages/ | grep expoLength of output: 609
🏁 Script executed:
#!/bin/bash
# Final verification: check for any indirect rollup usage
echo "=== Check for rollup in root devDependencies ==="
grep -A 100 '"devDependencies"' ./package.json | grep -E 'rollup|@rollup' | head -10
echo ""
echo "=== Look for rollup in any config files ==="
find . -maxdepth 2 -type f \( -name "*.config.js" -o -name "*.config.ts" -o -name "tsconfig.json" -o -name "webpack.config.js" \) 2>/dev/null | xargs grep -l "rollup" 2>/dev/null || echo "No rollup refs in config files"
echo ""
echo "=== Check if packages directory has expo-related packages ==="
ls -1 packages/ | grep -i expo || echo "No expo packages found"
echo ""
echo "=== List all packages ==="
ls -1 packages/Length of output: 717
🏁 Script executed:
#!/bin/bash
# Verify rollup.config.js uses plugin-node-resolve
echo "=== Content of ./build/rollup.config.js ==="
cat ./build/rollup.config.jsLength of output: 620
Remove @rollup/plugin-node-resolve from root devDependencies—it's unused.
The plugin doesn't appear in ./build/rollup.config.js or anywhere else in the codebase. The rollup config only uses typescript, json, dts, and commonjs plugins. Also, expo-example doesn't exist as a package, so the original assumption was incorrect. This is dead code that should be removed.
🤖 Prompt for AI Agents
In package.json around line 81, the devDependency "@rollup/plugin-node-resolve"
is unused and should be removed; delete the entry from the root devDependencies,
run your package manager to update lockfile (npm install or yarn install) and
verify no references remain (search build/rollup.config.js and the repo) before
committing.
| // Also ensure interceptors exist | ||
| if (!axios.interceptors) { | ||
| // This shouldn't happen, but just in case, create a mock | ||
| axios.interceptors = { | ||
| request: { | ||
| use: fn => { | ||
| console.warn('axios.interceptors.request.use mock called'); | ||
| return 0; | ||
| }, | ||
| }, | ||
| response: { | ||
| use: fn => { | ||
| console.warn('axios.interceptors.response.use mock called'); | ||
| return 0; | ||
| }, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mock interceptors hide real problems instead of failing fast.
If axios.interceptors is missing, something is broken. Mock functions return 0 and log warnings, but code expecting real interceptor behavior will fail silently.
Replace the mock with an error:
-// Also ensure interceptors exist
-if (!axios.interceptors) {
- // This shouldn't happen, but just in case, create a mock
- axios.interceptors = {
- request: {
- use: fn => {
- console.warn('axios.interceptors.request.use mock called');
- return 0;
- },
- },
- response: {
- use: fn => {
- console.warn('axios.interceptors.response.use mock called');
- return 0;
- },
- },
- };
-}
+// Verify interceptors exist (they should always be present in real axios)
+if (!axios.interceptors) {
+ throw new Error('axios.interceptors is missing - axios module may be corrupted');
+}📝 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.
| // Also ensure interceptors exist | |
| if (!axios.interceptors) { | |
| // This shouldn't happen, but just in case, create a mock | |
| axios.interceptors = { | |
| request: { | |
| use: fn => { | |
| console.warn('axios.interceptors.request.use mock called'); | |
| return 0; | |
| }, | |
| }, | |
| response: { | |
| use: fn => { | |
| console.warn('axios.interceptors.response.use mock called'); | |
| return 0; | |
| }, | |
| }, | |
| }; | |
| } | |
| // Verify interceptors exist (they should always be present in real axios) | |
| if (!axios.interceptors) { | |
| throw new Error('axios.interceptors is missing - axios module may be corrupted'); | |
| } |
🤖 Prompt for AI Agents
In packages/connect-examples/expo-example/shim-axios.js around lines 14 to 31,
the current code silently provides mock axios.interceptors that return 0 and log
warnings; this hides real failures — replace the mock with a fail-fast behavior
by removing the mock interceptor objects and instead throw a clear, descriptive
Error when axios.interceptors is missing (e.g., "axios.interceptors missing —
ensure axios is properly installed/initialized"), or at minimum log the problem
and immediately throw so callers cannot continue with broken behavior.
| @@ -1,4 +1,3 @@ | |||
| // eslint-disable-next-line import/no-relative-packages | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove orphaned eslint-disable comment.
No relative imports exist in this file. The comment serves no purpose.
-// eslint-disable-next-line import/no-relative-packages
-
export const CONNECT_SRC = process.env.CONNECT_SRC || `https://jssdk.onekey.so/1.1.17/`;🤖 Prompt for AI Agents
In packages/connect-examples/expo-example/src/constants/connect.ts around line
1, there is an orphaned ESLint disable comment "// eslint-disable-next-line
import/no-relative-packages" that is not needed because no relative-package
imports exist in this file; remove that comment line to clean up the code and
let ESLint rules apply normally.
Summary by CodeRabbit
New Features
Refactor
Chores