feat: add mobile app release version in Sentry issues #3249
Conversation
Aniruddha-Shriwant
requested review from
Chethan-Fyle and
arjunaj5
as code owners
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request enhance the functionality of the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -46,12 +46,12 @@ module.exports = function (ctx) { | |||
| '/node_modules/@capacitor-community/camera-preview/android/src/main/java/com/ahm/capacitor/camera/preview/CameraPreview.java', | |||
| }; | |||
|
|
|||
| // Adding GIT_COMMIT_SHA for sentry | |||
| // Adding mobile app version for tracking the release in sentry | |||
There was a problem hiding this comment.
git commit sha wasn't working earlier, check release property
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
hooks/prebuild.js(1 hunks)src/main.ts(1 hunks)
🧰 Additional context used
🪛 Biome
hooks/prebuild.js
[error] 50-50: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.
(lint/style/noVar)
[error] 51-51: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.
(lint/style/noVar)
🔇 Additional comments (4)
src/main.ts (1)
21-21: Mind-blowing change, but let's verify the magic happens correctly!
Thalaiva says the change looks perfect for tracking mobile app versions in Sentry, but we must ensure the placeholder gets replaced properly during build time, machaan!
✅ Verification successful
Superstar approves this style, mind it! The placeholder replacement is working perfectly!
The verification shows the prebuild script is handling our placeholder like a well-choreographed action sequence, machaan! Here's what's happening:
- The placeholder
please-replace-your-mobile-app-versiongets replaced withFYLE_MOBILE_RELEASE_VERSIONenvironment variable - The same version gets synchronized across multiple files including build.gradle and pbxproj
- The script even performs version comparison checks to ensure everything is in perfect rhythm
This is a blockbuster implementation that will make Sentry tracking as powerful as Rajini's punch dialogues! 🌟
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that prebuild script handles this placeholder replacement
# Check if prebuild script exists and contains the replacement logic
rg -A 5 "please-replace-your-mobile-app-version" "hooks/prebuild.js"
# Also check for the environment variable that should provide the version
rg "FYLE_MOBILE_RELEASE_VERSION" "hooks/prebuild.js"
Length of output: 797
hooks/prebuild.js (3)
49-54: Ensure the path to 'main.ts' is correct
Just make sure the src/main.ts file exists at that location. We don't want any surprises later.
Check with this command:
#!/bin/bash
# Description: Verify that src/main.ts exists
if [ -f src/main.ts ]; then
echo "File exists. Proceed ahead!"
else
echo "Hold on! src/main.ts not found."
fi🧰 Tools
🪛 Biome
[error] 50-50: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.
(lint/style/noVar)
[error] 51-51: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.
(lint/style/noVar)
54-54: Confirm the Sentry release version is set correctly
Setting the correct release version in Sentry is crucial for tracking. Let's ensure it's updated properly.
Here's a script to double-check:
#!/bin/bash
# Description: Verify the Sentry release version in main.ts
grep 'Sentry\.init' src/main.ts -A 5 | grep 'release'Expect to see:
release: 'your-mobile-app-version',
Ensure 'your-mobile-app-version' matches ctx.env.FYLE_MOBILE_RELEASE_VERSION.
54-54: Double-check that the mobile app version is correctly replaced
Make sure 'please-replace-your-mobile-app-version' is properly swapped with the actual app version in main.ts.
Run this script to confirm:
| var mainPath = path.resolve(process.cwd(), 'src/main.ts'); | ||
| var mainPathContent = fs.readFileSync(mainPath).toString(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Listen, my friend! Let's use 'let' or 'const' instead of 'var'
Using 'let' or 'const' helps avoid hoisting issues and keeps your code tight and clean.
Here's how we can fix it:
- var mainPath = path.resolve(process.cwd(), 'src/main.ts');
- var mainPathContent = fs.readFileSync(mainPath).toString();
+ const mainPath = path.resolve(process.cwd(), 'src/main.ts');
+ const mainPathContent = fs.readFileSync(mainPath).toString();📝 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.
| var mainPath = path.resolve(process.cwd(), 'src/main.ts'); | |
| var mainPathContent = fs.readFileSync(mainPath).toString(); | |
| const mainPath = path.resolve(process.cwd(), 'src/main.ts'); | |
| const mainPathContent = fs.readFileSync(mainPath).toString(); |
🧰 Tools
🪛 Biome
[error] 50-50: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.
(lint/style/noVar)
[error] 51-51: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.
(lint/style/noVar)
|
Clickup
https://app.clickup.com/t/86cwz8r36
Easier to track issues based on Mobile app version
Summary by CodeRabbit
New Features
Bug Fixes
Refactor