Android CI/CD #583
Android CI/CD #583
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a new GitHub Actions workflow named Changes
Possibly related PRs
Poem
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 (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
🚀 The APK is ready for download (As per the last commit of this PR): Download APK |
![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
🧹 Nitpick comments (4)
.github/workflows/pr-check.yml (3)
73-73: Optimize file writes by combining echo statements.ShellCheck (SC2129) flags repeated echo statements. Consider using a block for multiple lines:
- echo "apiKommunicateIo=${{ secrets.APIKOMMUNICATEIO }}" >> release.secrets.properties - echo "apiEuKommunicateIo=${{ secrets.APIEUKOMMUNICATEIO }}" >> release.secrets.properties + { + echo "apiKommunicateIo=${{ secrets.APIKOMMUNICATEIO }}" + echo "apiEuKommunicateIo=${{ secrets.APIEUKOMMUNICATEIO }}" + } >> release.secrets.propertiesAlso applies to: 166-166, 255-255
🧰 Tools
🪛 actionlint (1.7.4)
73-73: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
278-278: Double quote to prevent word splitting (SC2086).ShellCheck suggests quoting variables in commands like “echo $GITHUB_SHA” to avoid unintended glob expansion or splitting.
🧰 Tools
🪛 actionlint (1.7.4)
278-278: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
4-4: Remove trailing spaces and add a newline at EOF.YAML linting has flagged trailing spaces and missing newline. Removing them helps maintain code cleanliness.
- pull_request: + pull_request: ... - - name: Clean the project before app build + - name: Clean the project before app build ... - path: app/build/outputs/apk/debug/app-debug.apk + path: app/build/outputs/apk/debug/app-debug.apk ... - body: "@coderabbit review" \ No newline at end of file + body: "@coderabbit review" +Also applies to: 107-107, 195-195, 283-283, 286-286, 323-323, 344-344
🧰 Tools
🪛 yamllint (1.35.1)
[error] 4-4: trailing spaces
(trailing-spaces)
app/lint-baseline.xml (1)
1-779: Regularly update lint baseline.This file lists current lint issues. Keep it updated so that newly introduced errors are caught by lint instead of being silently ignored.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/pr-check.yml(1 hunks)app/build.gradle(1 hunks)app/lint-baseline.xml(1 hunks)gradle/wrapper/gradle-wrapper.properties(1 hunks)gradlew(1 hunks)gradlew.bat(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- gradle/wrapper/gradle-wrapper.properties
- gradlew.bat
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pr-check.yml
73-73: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
166-166: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
255-255: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
278-278: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
290-290: the runner of "actions/github-script@v6" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
310-310: the runner of "actions/github-script@v6" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
336-336: the runner of "actions/github-script@v6" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/pr-check.yml
[error] 4-4: trailing spaces
(trailing-spaces)
[error] 107-107: trailing spaces
(trailing-spaces)
[error] 195-195: trailing spaces
(trailing-spaces)
[error] 283-283: trailing spaces
(trailing-spaces)
[error] 286-286: trailing spaces
(trailing-spaces)
[error] 323-323: trailing spaces
(trailing-spaces)
[error] 344-344: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (2)
gradlew (1)
1-252: No major issues detected in the Gradle wrapper script.
This is a standard Gradle wrapper script with extensive POSIX compatibility handling, along with robust environment checks. No code-level changes are needed at this time.
.github/workflows/pr-check.yml (1)
290-290: GitHub Script action version may be outdated.
The runner “actions/github-script@v6” is flagged as too old. Updating to a recent version (e.g., v6.3.0 or higher if available) may resolve potential incompatibilities.
Also applies to: 310-310, 336-336
🧰 Tools
🪛 actionlint (1.7.4)
290-290: the runner of "actions/github-script@v6" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| lintOptions { | ||
| baseline file("lint-baseline.xml") | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Lint baseline contains several critical issues that need attention
The lint baseline file contains multiple important issues that should be addressed:
- Security concerns:
- Exported service without permission requirements
- Missing POST_NOTIFICATIONS permission for Android 13+
- Missing Selected Photos Access handling for Android 14+
- Outdated dependencies with potential security/performance implications:
- Firebase Messaging (20.1.0 → 24.1.0)
- Play Services Location (17.0.0 → 21.3.0)
- AndroidX Core KTX (1.13.1 → 1.15.0)
- UI/UX issues:
- Custom splash screen implementation (should use the platform's splash screen API)
- Missing content descriptions for accessibility
- Hardcoded strings instead of using string resources
- Missing autofill hints on text inputs
Consider addressing these issues rather than suppressing them in the baseline, especially the security-related ones.
🔗 Analysis chain
Ensure lint baseline alignment.
You've introduced a lint baseline. Verify that the baseline file is correctly maintained to avoid ignoring newly introduced critical issues. It's a best practice to periodically update the baseline to keep issues in check.
Let me verify the existence and content of the lint baseline file.
Let me check the content of the lint baseline file found in the app directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check if the lint baseline file exists and show its content
if [ -f "lint-baseline.xml" ]; then
echo "Lint baseline file exists. Content:"
cat "lint-baseline.xml"
else
echo "Lint baseline file not found at repository root"
# Check if it exists in other directories
fd -t f "lint-baseline.xml"
fi
Length of output: 185
Script:
#!/bin/bash
# Show content of the lint baseline file in the app directory
cat "app/lint-baseline.xml"
Length of output: 29623
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
🚀 The APK is ready for download (As per the last commit of this PR): Download APK |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
.github/workflows/temp.yml (2)
3-3: Fix YAML formatting issuesThere are several YAML formatting issues that should be addressed:
- Trailing spaces on lines 3 and 18
- Missing newline at end of file
-on: +on: - steps: + steps: # Add newline at end of fileAlso applies to: 18-18, 39-39
🧰 Tools
🪛 yamllint (1.35.1)
[error] 3-3: trailing spaces
(trailing-spaces)
28-29: Review retry configurationThe current retry configuration might not be sufficient for network issues:
- 3 retries with 500ms delay might be too aggressive
- Consider exponential backoff
- retry-count: 3 - retry-delay: 500 + retry-count: 5 + retry-delay: 1000.github/workflows/pr-check.yml (3)
4-16: LGTM! Well-configured workflow triggers and concurrency settings.The workflow is properly configured to:
- Trigger on relevant PR events (opened, reopened, synchronize)
- Target important branches (development, master)
- Handle concurrent runs efficiently
Remove the redundant comment on line 8 as it duplicates the configuration itself.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 4-4: trailing spaces
(trailing-spaces)
288-305: Improve error handling for artifact URL retrieval.The current implementation could fail silently if the artifact is not found immediately after upload.
Add retry logic and better error handling:
script: | + const MAX_RETRIES = 3; + const RETRY_DELAY = 5000; // 5 seconds + + async function sleep(ms) { + return new Promise(resolve => setTimeout(resolve, ms)); + } + + async function getArtifact(artifactName, attempt = 1) { const artifacts = await github.rest.actions.listArtifactsForRepo({ owner: context.repo.owner, repo: context.repo.repo, }); - console.log(`Artifacts: $artifacts`); + console.log(`Attempt ${attempt}: Found ${artifacts.data.artifacts.length} artifacts`); const artifact = artifacts.data.artifacts.find(a => a.name === artifactName); if (artifact) { return artifact; + } else if (attempt < MAX_RETRIES) { + console.log(`Artifact not found, retrying in ${RETRY_DELAY/1000} seconds...`); + await sleep(RETRY_DELAY); + return getArtifact(artifactName, attempt + 1); } else { - core.setFailed(`Artifact ${artifactName} not found.`); + throw new Error(`Artifact ${artifactName} not found after ${MAX_RETRIES} attempts.`); } + } + const artifactName = `Agent-App-${process.env.commit_hash}`; - const artifact = artifacts.data.artifacts.find(a => a.name === artifactName); - if (artifact) { + try { + const artifact = await getArtifact(artifactName); core.setOutput('artifact_url', artifact.archive_download_url); console.log(`Artifact URL: ${artifact.archive_download_url}`); - } else { - core.setFailed(`Artifact ${artifactName} not found.`); + } catch (error) { + core.setFailed(error.message); }🧰 Tools
🪛 actionlint (1.7.4)
290-290: the runner of "actions/github-script@v6" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
1-343: Address static analysis warnings.Several issues were identified by the static analysis tools:
- Shell script improvements needed for file redirects
- GitHub Script action version needs updating
- YAML formatting issues present
Apply the following fixes:
- Update GitHub Script action version:
- uses: actions/github-script@v6 + uses: actions/github-script@v7
- Fix shell script redirects (apply to all similar instances):
- echo "apiKommunicateIo=${{ secrets.APIKOMMUNICATEIO }}" >> release.secrets.properties - echo "apiEuKommunicateIo=${{ secrets.APIEUKOMMUNICATEIO }}" >> release.secrets.properties + { + echo "apiKommunicateIo=${{ secrets.APIKOMMUNICATEIO }}" + echo "apiEuKommunicateIo=${{ secrets.APIEUKOMMUNICATEIO }}" + } >> release.secrets.properties
- Fix YAML formatting:
- Remove trailing spaces
- Add newline at end of file
🧰 Tools
🪛 actionlint (1.7.4)
73-73: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
166-166: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
255-255: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
278-278: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
290-290: the runner of "actions/github-script@v6" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
309-309: the runner of "actions/github-script@v6" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
335-335: the runner of "actions/github-script@v6" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[error] 4-4: trailing spaces
(trailing-spaces)
[error] 107-107: trailing spaces
(trailing-spaces)
[error] 195-195: trailing spaces
(trailing-spaces)
[error] 283-283: trailing spaces
(trailing-spaces)
[error] 286-286: trailing spaces
(trailing-spaces)
[error] 322-322: trailing spaces
(trailing-spaces)
[error] 343-343: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pr-check.yml(1 hunks).github/workflows/temp.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/temp.yml
[error] 3-3: trailing spaces
(trailing-spaces)
[error] 18-18: trailing spaces
(trailing-spaces)
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/pr-check.yml
[error] 4-4: trailing spaces
(trailing-spaces)
[error] 107-107: trailing spaces
(trailing-spaces)
[error] 195-195: trailing spaces
(trailing-spaces)
[error] 283-283: trailing spaces
(trailing-spaces)
[error] 286-286: trailing spaces
(trailing-spaces)
[error] 322-322: trailing spaces
(trailing-spaces)
[error] 343-343: no new line character at the end of file
(new-line-at-end-of-file)
🪛 actionlint (1.7.4)
.github/workflows/pr-check.yml
73-73: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
166-166: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
255-255: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
278-278: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
290-290: the runner of "actions/github-script@v6" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
309-309: the runner of "actions/github-script@v6" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
335-335: the runner of "actions/github-script@v6" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (1)
.github/workflows/temp.yml (1)
26-27: Review authentication method
The workflow uses a token for authentication. Ensure that:
- The
DOWNLOAD_TOKENsecret has minimal required permissions - The token is regularly rotated
- Consider using GITHUB_TOKEN if possible for automatic token management
✅ Verification successful
Let me check the workflow file to understand the context of token usage.
Consider using GITHUB_TOKEN for artifact download
Based on the verification:
- The workflow is downloading an artifact from the same repository (Kommunicate-Android-Chat-SDK)
- The API test confirms that GITHUB_TOKEN has sufficient permissions to access artifacts
- GITHUB_TOKEN is automatically created and rotated by GitHub Actions
Replace the custom DOWNLOAD_TOKEN with GITHUB_TOKEN:
Authorization: Bearer ${{ github.token }}This change will:
- Eliminate the need to manage a custom token
- Provide automatic token rotation
- Follow the principle of least privilege as GITHUB_TOKEN has repository-scoped access
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if GITHUB_TOKEN could be used instead of custom token
gh api \
-H "Accept: application/vnd.github+json" \
-H "X-GitHub-Api-Version: 2022-11-28" \
"/repos/Kommunicate-io/Kommunicate-Android-Chat-SDK/actions/artifacts" \
--jq 'if .total_count > 0 then "GITHUB_TOKEN might be sufficient for artifact access" else "Custom token might be required" end'
Length of output: 338
Script:
#!/bin/bash
# Check the workflow file to understand the token usage
cat .github/workflows/temp.yml
Length of output: 1438
.github/workflows/temp.yml
Outdated
| @@ -0,0 +1,39 @@ | |||
| name: dasd | |||
There was a problem hiding this comment.
Workflow name requires revision
The current workflow name "dasd" is non-descriptive and appears to be a placeholder. Consider using a meaningful name that reflects the workflow's purpose, such as "Download Artifact Workflow" or "Artifact Retrieval".
-name: dasd
+name: Download Artifact Workflow📝 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.
| name: dasd | |
| name: Download Artifact Workflow |
.github/workflows/temp.yml
Outdated
| steps: | ||
| - name: Send HTTP request to download artifact | ||
| id: request | ||
| uses: tyrrrz/action-http-request@master |
There was a problem hiding this comment.
Security concern: Avoid using @master reference
Using the @master reference for actions is a security risk as it can lead to unexpected behavior if the action is compromised. Pin to a specific version hash for security.
- uses: tyrrrz/action-http-request@master
+ uses: tyrrrz/[email protected]📝 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.
| uses: tyrrrz/action-http-request@master | |
| uses: tyrrrz/action-http-request@v1.6.1 |
.github/workflows/temp.yml
Outdated
| id: request | ||
| uses: tyrrrz/action-http-request@master | ||
| with: | ||
| url: https://api.github.com/repos/Kommunicate-io/Kommunicate-Android-Chat-SDK/actions/artifacts/2355441843 |
There was a problem hiding this comment.
Security concern: Hardcoded artifact ID
The artifact ID (2355441843) is hardcoded in the URL. This is not maintainable and could pose security risks if the artifact is sensitive.
Consider:
- Parameterizing the artifact ID using workflow inputs or environment variables
- Implementing proper artifact lifecycle management
- url: https://api.github.com/repos/Kommunicate-io/Kommunicate-Android-Chat-SDK/actions/artifacts/2355441843
+ url: https://api.github.com/repos/Kommunicate-io/Kommunicate-Android-Chat-SDK/actions/artifacts/${{ inputs.artifact_id }}📝 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.
| url: https://api.github.com/repos/Kommunicate-io/Kommunicate-Android-Chat-SDK/actions/artifacts/2355441843 | |
| url: https://api.github.com/repos/Kommunicate-io/Kommunicate-Android-Chat-SDK/actions/artifacts/${{ inputs.artifact_id }} |
.github/workflows/temp.yml
Outdated
| - name: Print outputs | ||
| run: | | ||
| echo "Status: ${{ steps.request.outputs.status }}" | ||
| echo "Success: ${{ steps.request.outputs.success }}" | ||
| echo "Headers: ${{ steps.request.outputs.headers }}" | ||
| echo "Body: ${{ steps.request.outputs.body }}" | ||
| echo "Artifact downloaded successfully as artifact.zip" | ||
| # Optional: Save the body (artifact) to a file if it is binary content | ||
| echo "${{ steps.request.outputs.body }}" > artifact.zip |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve artifact handling and validation
The current implementation saves the response body directly to a file without proper validation or error handling.
Consider:
- Add validation for the response status
- Verify the content type and size
- Implement proper error handling
- name: Print outputs
+ if: steps.request.outputs.success == 'true'
run: |
- echo "Status: ${{ steps.request.outputs.status }}"
- echo "Success: ${{ steps.request.outputs.success }}"
- echo "Headers: ${{ steps.request.outputs.headers }}"
- echo "Body: ${{ steps.request.outputs.body }}"
- echo "Artifact downloaded successfully as artifact.zip"
- # Optional: Save the body (artifact) to a file if it is binary content
- echo "${{ steps.request.outputs.body }}" > artifact.zip
+ if [[ "${{ steps.request.outputs.status }}" == "200" ]]; then
+ echo "Downloading artifact..."
+ echo "${{ steps.request.outputs.body }}" > artifact.zip
+ if [[ -f "artifact.zip" ]]; then
+ echo "Artifact downloaded successfully"
+ else
+ echo "Failed to save artifact"
+ exit 1
+ fi
+ else
+ echo "Failed to download artifact. Status: ${{ steps.request.outputs.status }}"
+ exit 1
+ fi📝 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.
| - name: Print outputs | |
| run: | | |
| echo "Status: ${{ steps.request.outputs.status }}" | |
| echo "Success: ${{ steps.request.outputs.success }}" | |
| echo "Headers: ${{ steps.request.outputs.headers }}" | |
| echo "Body: ${{ steps.request.outputs.body }}" | |
| echo "Artifact downloaded successfully as artifact.zip" | |
| # Optional: Save the body (artifact) to a file if it is binary content | |
| echo "${{ steps.request.outputs.body }}" > artifact.zip | |
| - name: Print outputs | |
| if: steps.request.outputs.success == 'true' | |
| run: | | |
| if [[ "${{ steps.request.outputs.status }}" == "200" ]]; then | |
| echo "Downloading artifact..." | |
| echo "${{ steps.request.outputs.body }}" > artifact.zip | |
| if [[ -f "artifact.zip" ]]; then | |
| echo "Artifact downloaded successfully" | |
| else | |
| echo "Failed to save artifact" | |
| exit 1 | |
| fi | |
| else | |
| echo "Failed to download artifact. Status: ${{ steps.request.outputs.status }}" | |
| exit 1 | |
| fi | |
🧰 Tools
🪛 yamllint (1.35.1)
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/pr-check.yml
Outdated
| build: | ||
| name: "Build Project" | ||
| runs-on: ubuntu-latest | ||
| needs: lint |
There was a problem hiding this comment.
Fix job dependencies to ensure correct execution order.
The current job dependency chain is problematic:
- Build job depends on lint job, but lint checks might need generated files from the build
- Both debug-apk and comment-on-pr jobs correctly depend on build
Modify the job dependencies to ensure correct execution order:
build:
name: "Build Project"
runs-on: ubuntu-latest
- needs: lint
lint:
name: "Run Lint Checks"
runs-on: ubuntu-latest
+ needs: buildAlso applies to: 204-204, 325-325
There was a problem hiding this comment.
@prateek-kommunicate, thank you for addressing the fix! Looks good.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| - name: Create local.properties | ||
| run: | | ||
| echo "sdk.dir=$ANDROID_HOME" > local.properties | ||
| echo "GOOGLE_KEY=${{ secrets.GOOGLE_MAPS_API }}" >> local.properties | ||
|
|
||
| # Create release.secrets.properties | ||
| - name: Create release.secrets.properties | ||
| run: | | ||
| echo "apiKommunicateIo=${{ secrets.APIKOMMUNICATEIO }}" >> release.secrets.properties | ||
| echo "apiEuKommunicateIo=${{ secrets.APIEUKOMMUNICATEIO }}" >> release.secrets.properties | ||
| echo "apiCaKommunicateIo=${{ secrets.APICAKOMMUNICATEIO }}" >> release.secrets.properties | ||
| echo "apiTestKommuncateIo=${{ secrets.APITESTKOMMUNCATEIO }}" >> release.secrets.properties | ||
| echo "kommunicateIo=${{ secrets.KOMMUNICATEIO }}" >> release.secrets.properties | ||
| echo "apiInKommunicateIo=${{ secrets.APIINKOMMUNICATEIO }}" >> release.secrets.properties | ||
| echo "kommunicateAWS=${{ secrets.KOMMUNICATEAWS }}" >> release.secrets.properties | ||
| echo "SENTRY_DSN=${{ secrets.SENTRY_DSN }}" >> release.secrets.properties | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated configuration setup into a reusable composite action.
The same configuration setup (creating properties files with secrets) is duplicated across three jobs. This increases maintenance overhead and the risk of inconsistencies.
Create a composite action to handle the configuration setup:
# .github/actions/setup-properties/action.yml
name: 'Setup Properties Files'
description: 'Creates necessary properties files with secrets'
runs:
using: "composite"
steps:
- name: Create local.properties
shell: bash
run: |
{
echo "sdk.dir=$ANDROID_HOME"
echo "GOOGLE_KEY=${{ secrets.GOOGLE_MAPS_API }}"
} > local.properties
- name: Create release.secrets.properties
shell: bash
run: |
{
echo "apiKommunicateIo=${{ secrets.APIKOMMUNICATEIO }}"
echo "apiEuKommunicateIo=${{ secrets.APIEUKOMMUNICATEIO }}"
echo "apiCaKommunicateIo=${{ secrets.APICAKOMMUNICATEIO }}"
echo "apiTestKommuncateIo=${{ secrets.APITESTKOMMUNCATEIO }}"
echo "kommunicateIo=${{ secrets.KOMMUNICATEIO }}"
echo "apiInKommunicateIo=${{ secrets.APIINKOMMUNICATEIO }}"
echo "kommunicateAWS=${{ secrets.KOMMUNICATEAWS }}"
echo "SENTRY_DSN=${{ secrets.SENTRY_DSN }}"
} > release.secrets.propertiesThen use it in each job:
- name: Setup Properties
uses: ./.github/actions/setup-propertiesAlso applies to: 159-175, 248-264
🧰 Tools
🪛 actionlint (1.7.4)
73-73: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
🚀 The APK is ready for download (As per the last commit of this PR): Download APK |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/workflows/temp.yml (4)
7-7: Remove redundant commentThe comment is redundant as the branch targeting is already clear from the configuration.
- - master # Run only for pull requests targeting the develop and master branch + - master
49-52: Enhance test execution reliabilityThe test execution step could benefit from better error handling and timeout configuration.
- name: Build and Run Tests + timeout-minutes: 30 run: | + # Ensure Gradle wrapper has execute permissions + chmod +x ./gradlew + # Build with detailed logging on failure ./gradlew assembleDebug assembleAndroidTest - ./gradlew connectedAndroidTest + ./gradlew connectedAndroidTest --info --stacktrace + continue-on-error: false
55-59: Improve test results artifact configurationThe artifact upload configuration could benefit from additional settings for better maintainability and resource management.
- name: Upload Test Results uses: actions/upload-artifact@v4 with: name: test-results path: app/build/reports/androidTests/ + retention-days: 14 + if-no-files-found: error + compression-level: 9Also, add a newline at the end of the file to fix the YAML formatting issue.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 59-59: no new line character at the end of file
(new-line-at-end-of-file)
1-59: Consider splitting the workflow for better maintainabilityThe current workflow combines environment setup, test execution, and artifact handling. Consider splitting it into reusable workflows:
- A reusable environment setup workflow
- A main test execution workflow
- A reusable artifact handling workflow
This would improve maintainability and allow for better reuse across different Android projects.
Would you like me to provide an example of how to structure these reusable workflows?
🧰 Tools
🪛 actionlint (1.7.4)
20-20: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
24-24: the runner of "actions/setup-java@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[error] 3-3: trailing spaces
(trailing-spaces)
[error] 53-53: trailing spaces
(trailing-spaces)
[error] 59-59: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/temp.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/temp.yml
20-20: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
24-24: the runner of "actions/setup-java@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/temp.yml
[error] 3-3: trailing spaces
(trailing-spaces)
[error] 53-53: trailing spaces
(trailing-spaces)
[error] 59-59: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/temp.yml (1)
1-1: Workflow name requires revision
The current workflow name "dasd" is non-descriptive. Consider using a meaningful name that reflects the workflow's purpose.
-name: dasd
+name: Android Instrumentation Tests
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
🚀 The APK is ready for download (As per the last commit of this PR): Download APK |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (18)
.github/workflows/temp.yml (6)
3-11: Consider adding path filters to the workflow triggerThe workflow runs instrumentation tests which can be time-consuming. Consider adding path filters to only trigger the workflow when relevant files are changed.
on: pull_request: branches: - development - master + paths: + - 'app/**' + - 'kommunicatesdk/**' + - 'build.gradle' + - 'gradle.properties' types: - synchronize - opened - reopened🧰 Tools
🪛 yamllint (1.35.1)
[error] 3-3: trailing spaces
(trailing-spaces)
17-21: Enhance test coverage with multiple API levelsConsider testing against multiple API levels to ensure compatibility across different Android versions. Also, add a name to the matrix combination for better visibility in the GitHub UI.
strategy: + fail-fast: false matrix: - api-level: [ 30 ] + api-level: [ 29, 30, 31 ] target: [ google_apis ] + name: Instrumentation Tests (API ${{ matrix.api-level }})
41-46: Improve cache key specificityThe current cache key only includes the API level. Consider adding more variables to make the cache key more specific and prevent cache conflicts.
with: path: | ~/.android/avd/* ~/.android/adb* - key: avd-${{ matrix.api-level }} + key: avd-${{ matrix.api-level }}-${{ matrix.target }}-${{ hashFiles('build.gradle') }} + restore-keys: | + avd-${{ matrix.api-level }}-${{ matrix.target }}- + avd-${{ matrix.api-level }}-
57-57: Optimize emulator performance settingsThe emulator options could be optimized for CI environment. Consider adding memory-related options for better performance.
- emulator-options: -no-window -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none + emulator-options: -no-window -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none -memory 2048 -no-snapshot -cores 2Also applies to: 86-86
69-77: Optimize file creation and enhance security
- Use a single redirection for better performance
- Consider using environment files for sensitive data
- - name: Create release.secrets.properties - run: | - echo "apiKommunicateIo=${{ secrets.APIKOMMUNICATEIO }}" >> release.secrets.properties - echo "apiEuKommunicateIo=${{ secrets.APIEUKOMMUNICATEIO }}" >> release.secrets.properties - echo "apiCaKommunicateIo=${{ secrets.APICAKOMMUNICATEIO }}" >> release.secrets.properties - echo "apiTestKommuncateIo=${{ secrets.APITESTKOMMUNCATEIO }}" >> release.secrets.properties - echo "kommunicateIo=${{ secrets.KOMMUNICATEIO }}" >> release.secrets.properties - echo "apiInKommunicateIo=${{ secrets.APIINKOMMUNICATEIO }}" >> release.secrets.properties - echo "kommunicateAWS=${{ secrets.KOMMUNICATEAWS }}" >> release.secrets.properties - echo "SENTRY_DSN=${{ secrets.SENTRY_DSN }}" >> release.secrets.properties + - name: Create release.secrets.properties + run: | + { + echo "apiKommunicateIo=${{ secrets.APIKOMMUNICATEIO }}" + echo "apiEuKommunicateIo=${{ secrets.APIEUKOMMUNICATEIO }}" + echo "apiCaKommunicateIo=${{ secrets.APICAKOMMUNICATEIO }}" + echo "apiTestKommuncateIo=${{ secrets.APITESTKOMMUNCATEIO }}" + echo "kommunicateIo=${{ secrets.KOMMUNICATEIO }}" + echo "apiInKommunicateIo=${{ secrets.APIINKOMMUNICATEIO }}" + echo "kommunicateAWS=${{ secrets.KOMMUNICATEAWS }}" + echo "SENTRY_DSN=${{ secrets.SENTRY_DSN }}" + } > release.secrets.properties + shell: bash🧰 Tools
🪛 actionlint (1.7.4)
69-69: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
3-3: Fix formatting issues
- Remove trailing spaces on lines 3, 78, and 91
- Add newline at end of file
Also applies to: 78-78, 91-91, 97-97
🧰 Tools
🪛 yamllint (1.35.1)
[error] 3-3: trailing spaces
(trailing-spaces)
app/src/androidTest/java/kommunicate/io/sample/KMUserDataValidationTest.kt (1)
97-105: Consider asserting specific exception details instead of swallowing.Catching a general
Exceptionwithout re-throwing or logging it might lose information about what actually went wrong. If the test aims to confirm certain exceptions, verifying the exception type or message in detail can help avoid false positives.🧰 Tools
🪛 detekt (1.23.7)
[warning] 102-102: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
app/src/androidTest/java/kommunicate/io/sample/utils/Utils.kt (1)
86-94: Optional improvement: replace arbitrary sleeps with test synchronization.This new
waitAfterLatchparameter can help reduce flakiness by giving extra time for UI stabilization. However, be cautious that arbitrary waits may slow your tests. When feasible, consider using a more deterministic synchronization mechanism (e.g., IdlingResource or a known event) instead of time-based waits.app/src/androidTest/java/kommunicate/io/sample/ConversationWithPreChatTest.kt (3)
163-165: Consider event-based synchronization.Waiting for 5 seconds might ensure stability but can unnecessarily increase test run times. When possible, prefer event-based synchronization (e.g., IdlingResource) to detect actual completion.
328-343: Verify exception details in negative tests.Catching a general
Exceptionand only checking the message can mask unexpected failures. To improve test accuracy, consider verifying the specific exception subtype or re-throwing it after logging.
379-380: Reduce or replace long waits.Another 5-second wait to stabilize the test might be necessary, but like earlier comments, consider a more targeted event-based approach to avoid maintaining large arbitrary sleep durations.
app/src/androidTest/java/kommunicate/io/sample/ConversationTest.kt (2)
206-208: Consider avoiding static waits for stability.Relying on a fixed wait time (
waitFor(3000)) can introduce flakiness if the operation completes sooner or later. Consider using an Espresso Idling Resource or a latch-based approach to synchronize the test more reliably.
290-290: Validate large wait times.A 5-second wait may be appropriate for network operations, but it can slow tests. Confirm that this longer wait is necessary and consider an event-based synchronization when possible.
kommunicate/src/main/java/io/kommunicate/KmConversationHelper.java (1)
622-622: Passing the exception object as the callback failure parameter.Switching from
callback.onFailure(registrationResponse)tocallback.onFailure(exception)provides more accurate error information. However, confirm whether theRegistrationResponsecontains additional details that might still be valuable. If so, consider including those details in error logs or bundling them in a custom error object.app/src/androidTest/java/kommunicate/io/sample/PseudoNameTest.kt (4)
51-55: Initialization ofchatAPIand tokensWhile the initialization flow looks good, consider verifying or logging if the retrieved tokens are valid to prevent unexpected runtime issues.
56-58: Kommunicate initialization inonActivityHardcoding the app ID here is okay for testing, but extracting it to a constant or configuration can improve maintainability.
84-85: Performing an explicit delayWhile
waitFor(3000)might work, using an IdlingResource or more precise synchronization mechanism can reduce test flakiness.
86-87: Asserting dashboard user name
assertNotNullis a good baseline. Optionally, add a direct check to confirm it’s the same user name as generated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/temp.yml(1 hunks)app/src/androidTest/java/kommunicate/io/sample/ConversationTest.kt(6 hunks)app/src/androidTest/java/kommunicate/io/sample/ConversationWithPreChatTest.kt(5 hunks)app/src/androidTest/java/kommunicate/io/sample/KMUserDataValidationTest.kt(2 hunks)app/src/androidTest/java/kommunicate/io/sample/PseudoNameTest.kt(3 hunks)app/src/androidTest/java/kommunicate/io/sample/utils/KmTestHelper.kt(1 hunks)app/src/androidTest/java/kommunicate/io/sample/utils/Utils.kt(1 hunks)kommunicate/src/main/AndroidManifest.xml(1 hunks)kommunicate/src/main/java/io/kommunicate/KmConversationHelper.java(1 hunks)
🧰 Additional context used
🪛 detekt (1.23.7)
app/src/androidTest/java/kommunicate/io/sample/KMUserDataValidationTest.kt
[warning] 102-102: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🪛 actionlint (1.7.4)
.github/workflows/temp.yml
69-69: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
🪛 yamllint (1.35.1)
.github/workflows/temp.yml
[error] 3-3: trailing spaces
(trailing-spaces)
[error] 78-78: trailing spaces
(trailing-spaces)
[error] 91-91: trailing spaces
(trailing-spaces)
[error] 97-97: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (21)
kommunicate/src/main/AndroidManifest.xml (1)
27-28: Reconsider forcing android:allowBackup in library manifest
The addition of tools:replace="android:allowBackup" forces the library's allowBackup=false setting to override the hosting application's backup settings. While disabling backup can be good for security, this decision should typically be left to the application developer.
Consider these alternatives:
- Remove the
tools:replaceand document the recommended backup settings in your library's documentation - Use
tools:node="remove"instead to let the app explicitly set this value - If backup must be disabled, clearly document this behavior in the SDK documentation
This change might break existing applications that rely on backup functionality for their data.
Let's verify if any applications are affected:
.github/workflows/temp.yml (1)
1-1: Workflow name requires revision
The current workflow name "dasd" is non-descriptive and appears to be a placeholder.
app/src/androidTest/java/kommunicate/io/sample/KMUserDataValidationTest.kt (1)
110-112: Looks good for wait synchronization.
The call to waitForLatch(latch, 100) ensures a short post-latch delay, which can stabilize asynchronous test flows. This is fine if the extra wait does not overly increase your test duration.
app/src/androidTest/java/kommunicate/io/sample/ConversationWithPreChatTest.kt (1)
349-351: Latch-based wait approach is acceptable.
This usage of waitForLatch is consistent. Ensure that the latch truly reflects test readiness to avoid extraneous waits.
app/src/androidTest/java/kommunicate/io/sample/ConversationTest.kt (4)
218-218: Verify the newly added bot ID.
You’ve introduced "richmessagetest-apbah" to the bot IDs list. Ensure any required bot configuration or references exist in the system to avoid failures during runtime.
338-338: Ensure consistency with bot ID updates.
You’ve updated the botIds again to include "richmessagetest-apbah". Double-check that the usage in this method and the previous test is consistent—mismatched lists can cause inconsistencies when verifying bot participation in a conversation.
470-470: Confirm the updated assertion text.
The test now checks for the text "trgfdgfd". Ensure this change is intentional and the text is not a placeholder.
513-513: Improved logging context.
Adding groupId to the failure message is beneficial for debugging. This looks good.
app/src/androidTest/java/kommunicate/io/sample/PseudoNameTest.kt (13)
4-5: New imports for lifecycle usage
No issues. These imports are essential for instrumented tests with ActivityScenario and lifecycleScope.
7-8: Added Espresso imports
The added Espresso imports are standard for UI testing and look proper.
17-17: Imported additional utility methods
These helpers (launchConversation, getRandomString, sendMessageAsUser, waitFor, waitForLatch) are clearly used within the test. No issues found.
Also applies to: 19-19, 20-20, 21-21, 22-22
23-23: Coroutine launch import
No concerns. The kotlinx.coroutines.launch import is consistent with the usage in line 73.
27-27: Importing assertNotNull
No problems with adding this import, as it's used to verify that objects are not null.
33-33: Importing CountDownLatch
Using a latch for test synchronization is standard practice in Android tests.
40-40: ActivityScenario for MainActivity
Instantiating ActivityScenario here is a suitable approach for instrumentation testing of MainActivity.
43-43: New property: chatAPI
Declaring private lateinit var chatAPI clearly communicates that it’ll be initialized before usage.
63-64: Using CountDownLatch for synchronization
Creating the latch and counting it down upon conversation readiness is a solid and straightforward concurrency solution.
72-79: Launching conversation within lifecycleScope
This asynchronous call is clean. However, consider capturing and asserting exceptions in invokeOnCompletion to avoid false positives.
80-81: Waiting on the latch
The waitForLatch(latch) approach is correct and ensures the test doesn’t proceed prematurely.
82-82: Sending a random user message
No issues here. Generating random strings for tests is a common approach to ensure uniqueness.
137-160: New function getUserFromDashboardWithUserName
This function clearly retrieves the user via the chatAPI. Failing fast is acceptable in test code, though returning an optional or custom result object could improve clarity in larger test suites.
app/src/androidTest/java/kommunicate/io/sample/KMUserDataValidationTest.kt
Show resolved
Hide resolved
app/src/androidTest/java/kommunicate/io/sample/ConversationWithPreChatTest.kt
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
lint-baseline.xmlfile to define lint issues and their resolutions.Updates