Sync iOS configuration with project template and update Fastlane build scripts#3122
Open
Aryan-Baglane wants to merge 20 commits intoopenMF:developmentfrom
Open
Sync iOS configuration with project template and update Fastlane build scripts#3122Aryan-Baglane wants to merge 20 commits intoopenMF:developmentfrom
Aryan-Baglane wants to merge 20 commits intoopenMF:developmentfrom
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR refactors build and deployment configuration to use a centralized ProjectConfig module, updates GitHub Actions workflows and Fastlane to v1.0.11, enhances secrets management tooling with automated syncing capabilities, and adds comprehensive documentation for CI/CD and Fastlane workflows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (7)
Gemfile (1)
5-12: Fix gem ordering to satisfy RuboCop'sBundler/OrderedGems.Two violations flagged by RuboCop:
bigdecimal(line 9) should precedemutex_m(line 8).cocoapods(line 12) should precedefastlane(line 11).♻️ Proposed fix
# Add compatibility gems for Ruby 3.3+ gem "abbrev" gem "base64" +gem "bigdecimal" gem "mutex_m" -gem "bigdecimal" -gem "fastlane" gem "cocoapods", "~> 1.16" +gem "fastlane"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gemfile` around lines 5 - 12, RuboCop's Bundler/OrderedGems requires gems in alphabetical order; reorder the entries so "bigdecimal" appears before "mutex_m" and "cocoapods" appears before "fastlane". Update the Gemfile by swapping the positions of the pairs (ensure the lines with gem "bigdecimal" and gem "mutex_m" are sorted, and the lines with gem "cocoapods" and gem "fastlane" are sorted) so the gem names are alphabetically ordered..github/workflows/CLAUDE.md (1)
26-34: Fenced code block on line 26 is missing a language specifier.markdownlint (MD040) flags this. Adding
textorplainsilences the warning and improves renderer behaviour.-``` +```text Local Workflows (.github/workflows/)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/CLAUDE.md around lines 26 - 34, The fenced code block in the CLAUDE.md content (the triple-backtick block that lists Local Workflows → Reusable Workflows → Custom Actions → Fastlane Lanes) is missing a language specifier; update that opening fence from ``` to ```text (or ```plain) so markdownlint MD040 is silenced and rendering is consistent, i.e., locate the triple-backtick block in the file and prepend the chosen language after the opening backticks.keystore-manager.sh (2)
604-678:update_secrets_env_with_filesrelies on bash dynamic scoping to readENCODED_SECRETSfrom its caller — fragile and undocumented.The comment at line 610 acknowledges this:
# Access the ENCODED_SECRETS array from parent scope. This works only as long as the function is called from withinencode_secrets_directory_files's stack frame. Moving, extracting, or testing either function in isolation will silently use an unset/empty array. Pass the data explicitly or restructure to keep both in the same function scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keystore-manager.sh` around lines 604 - 678, The function update_secrets_env_with_files currently reads ENCODED_SECRETS from its caller's scope (fragile); change it to accept the encoded-secrets associative array explicitly (e.g., add a parameter like encoded_array_name) and inside update_secrets_env_with_files create a nameref with declare -n encoded_secrets_ref="$1" (or accept the output of declare -p and eval if supporting older Bash), then replace all uses of ENCODED_SECRETS with encoded_secrets_ref so the function no longer relies on dynamic parent-scope variables; update callers (like encode_secrets_directory_files) to call update_secrets_env_with_files "ENCODED_SECRETS" and keep function name references update_secrets_env_with_files and encode_secrets_directory_files consistent.
68-69:IOS_STRING_SECRETSglobal declaration is misleadingly indented at script scope.The
declare -g -A IOS_STRING_SECRETSon lines 68–69 sits at script (top) level but is indented with two spaces, making it look like it belongs to an enclosing block. The companionMACOS_PASSWORD_SECRETSdeclaration at line 426 is correctly at column 0. Aligning them avoids confusion about scope.♻️ Proposed fix
- # Global associative array for iOS string secrets - declare -g -A IOS_STRING_SECRETS +# Global associative array for iOS string secrets +declare -g -A IOS_STRING_SECRETS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keystore-manager.sh` around lines 68 - 69, The global associative array declaration for IOS_STRING_SECRETS is incorrectly indented making it appear to be inside a block; move the line declaring IOS_STRING_SECRETS (declare -g -A IOS_STRING_SECRETS) to column 0 (no leading spaces) so it is visually and actually at script/top scope like the existing MACOS_PASSWORD_SECRETS declaration, ensuring consistent alignment for global declarations.fastlane/FastFile (1)
475-475: Pass options through tosetup_ci_if_needed.These calls drop lane options, so
ci_provideroverrides are ignored.Suggested fix
- setup_ci_if_needed + setup_ci_if_needed(options) @@ - setup_ci_if_needed + setup_ci_if_needed(options) @@ - setup_ci_if_needed + setup_ci_if_needed(options)Also applies to: 556-556, 682-682
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fastlane/FastFile` at line 475, The calls to setup_ci_if_needed drop the lane options and thus ignore overrides like ci_provider; update each invocation of setup_ci_if_needed (e.g., the calls currently at the same spots you found) to forward the lane options by passing the options hash (e.g., setup_ci_if_needed(options)), so the method receives and respects overrides such as ci_provider.fastlane-config/extract_config.rb (1)
1-1: Resolve shebang vs file permission mismatch.This file has a script shebang but is not executable per static analysis. Either mark it executable or remove the shebang to avoid lint noise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fastlane-config/extract_config.rb` at line 1, Either make the script executable or remove the shebang line to resolve the shebang vs file permission mismatch: either run chmod +x on the file so the "#!/usr/bin/env ruby" line is valid, or delete that shebang from extract_config.rb to stop lint warnings for a non-executable file.fastlane-config/project_config.rb (1)
274-274: Make required-file validation independent of current working directory.Using
Dir.pwdhere is fragile and can point outside the repo depending on invocation path. Resolve relative to this config file instead.Suggested fix
- missing_files = required_files.reject { |file| File.exist?(File.join(Dir.pwd, '..', file)) } + repo_root = File.expand_path('..', __dir__) + missing_files = required_files.reject { |file| File.exist?(File.join(repo_root, file)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fastlane-config/project_config.rb` at line 274, The required-file check uses Dir.pwd which can be outside the repo; update the path resolution so it is relative to this config file instead: change the File.exist? call that builds the candidate path (referenced in the missing_files computation and the required_files array) to resolve files against the directory of this config (use __dir__ or File.dirname(__FILE__) and File.expand_path to join the parent directory) rather than Dir.pwd, so existence is tested consistently relative to the repository/config file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/CLAUDE.md:
- Line 4: Update the stale workflow version string
`openMF/mifos-x-actionhub@v1.0.8` to `openMF/mifos-x-actionhub@v1.0.11` wherever
it appears in the document (notably the occurrences around the examples at lines
referenced in the review); search for the exact token `@v1.0.8` and replace each
with `@v1.0.11` so the documentation matches the actual workflow files (ensure
all occurrences including the ones cited in the review are changed).
In @.github/workflows/multi-platform-build-and-publish.yml:
- Around line 127-128: The workflow is passing undefined inputs
distribute_macos_testflight and distribute_macos_appstore into the reusable
workflow, so macOS distribution is effectively disabled; fix by either adding
these two inputs to the workflow_dispatch inputs block (declare inputs named
distribute_macos_testflight and distribute_macos_appstore with suitable boolean
defaults) or replace the passed expressions at the call site with explicit
boolean literals (true/false) until you intend to support them; update
references to inputs.distribute_macos_testflight and
inputs.distribute_macos_appstore accordingly so the reusable workflow receives a
defined value.
- Around line 122-126: The workflow currently omits passing the
distribute_ios_testflight and distribute_ios_appstore inputs to the reusable
job, effectively disabling iOS TestFlight/App Store delivery; update the job
invocation to either (A) forward exactly one of distribute_ios_testflight or
distribute_ios_appstore based on the calling inputs (so if
inputs.distribute_ios_appstore is true pass distribute_ios_appstore: ${{
inputs.distribute_ios_appstore }} and pass distribute_ios_testflight: false, and
vice‑versa) or (B) introduce a new ios_delivery_target input (values:
none/testflight/appstore) and map it to the reusable workflow by setting the two
boolean inputs accordingly—change references to the inputs
distribute_ios_testflight and distribute_ios_appstore in the job invocation to
implement the chosen mutual‑exclusive mapping.
- Line 74: Update the workflow's target_branch default from 'dev' to
'development' so the default matches the existing repository branch;
specifically change the default value for the target_branch input/variable (the
"target_branch" key) to 'development' to align with other workflows like
android-release.yml and prevent silent failures when the default is accepted.
In `@fastlane-config/ios_config.rb`:
- Around line 24-25: The BUILD_CONFIG's ios_config is missing the
provisioning_profile_appstore key used by fastlane/FastFile
(ios_config[:provisioning_profile_appstore] on lines 606 and 741); add a new key
provisionin_profile_appstore (named provisioning_profile_appstore) to the
ios_config block and set it to the App Store profile value from
ProjectConfig::IOS_SHARED[:code_signing][:provisioning_profiles][:appstore] so
Fastfile picks the correct App Store/TestFlight profile; update the ios_config
in ios_config.rb near the existing provisioning_profile_name and git_url entries
to include this new key.
- Around line 37-41: BUILD_CONFIG currently contains invalid constant assignment
syntax — the constants TESTFLIGHT_CONFIG and APPSTORE_CONFIG are being assigned
inside the hash literal; move those two assignments out of the BUILD_CONFIG hash
and place them as standalone constant assignments (e.g., TESTFLIGHT_CONFIG =
ProjectConfig::IOS_SHARED[:testflight and APPSTORE_CONFIG =
ProjectConfig::IOS_SHARED[:appstore]) above or below the BUILD_CONFIG definition
so BUILD_CONFIG remains a valid Hash and the constants are defined separately.
In `@fastlane-config/project_config.rb`:
- Around line 34-39: The keystore block currently hardcodes secrets (password
and key_password) and must be replaced to read from secure environment/secret
manager: remove literal values for password and key_password in the keystore
hash and instead fetch them from environment variables or a secrets API (e.g.,
ENV['KEYSTORE_PASSWORD'], ENV['KEY_PASSWORD']) and validate presence at startup,
raising a clear error if missing; keep key_alias configurable too
(ENV['KEY_ALIAS'] or default) and ensure these values are never printed to logs
or committed to repo.
In `@fastlane/AppFile`:
- Line 12: The call team_id(FastlaneConfig::ProjectConfig::IOS[:team_id]) uses
the wrong config key and returns nil; update the reference to pull the iOS team
id from FastlaneConfig::ProjectConfig::IOS_SHARED (e.g. replace the
FastlaneConfig::ProjectConfig::IOS[:team_id] usage with
FastlaneConfig::ProjectConfig::IOS_SHARED[:team_id] where team_id(...) is
invoked in AppFile) so the correct team id is provided for iOS account
resolution.
In `@fastlane/CLAUDE.md`:
- Line 5: Update the header line that currently reads "Total Lanes: 12 (7
Android + 5 iOS)" to reflect the actual counts in the document — change it to
"Total Lanes: 13 (8 Android + 5 iOS)"; ensure the visible header string in
CLAUDE.md exactly matches the new totals so the summary aligns with the lane
list.
- Around line 34-42: Fenced code blocks in the CLAUDE.md document are missing
language tags (triggering MD040); add an explicit language identifier (e.g.,
"text") to each triple-backtick fence covering the ASCII diagram, the Android
gradle properties block (the lines starting with -Pandroid.injected.signing.*),
and the fastlane-config directory tree, and likewise update the other blocks
referenced (around the other ranges) so every ``` becomes ```text to satisfy
markdownlint.
In `@fastlane/FastFile`:
- Around line 45-47: In the bundleReleaseApks lane, the build step calls
buildAndSignApp with taskName set to "assemble" (which produces APKs); change
the taskName to the Gradle bundle task "bundleRelease" (i.e., set taskName:
"bundleRelease") so buildAndSignApp produces an AAB; update the call in the
bundleReleaseApks lane where buildAndSignApp is invoked.
- Line 364: The hardcoded readonly: false allows mutations during CI; change the
declaration that sets readonly to instead choose true when running in CI and
false locally by checking the CI environment set by setup_ci_if_needed (e.g.,
inspect ENV['CI'] or a flag/variable that setup_ci_if_needed establishes) and
use that boolean for the readonly option so provisioning/signing assets are
read-only in CI but mutable for local development.
In `@keystore-manager.sh`:
- Around line 392-396: In parse_shared_keys_env, split combined declarations
like "local APPSTORE_KEY_ID=$(...)" into two statements: declare the local
(e.g., local APPSTORE_KEY_ID) on one line, then assign from the command
substitution on the next line and immediately check the command's exit status
($?) so failures are detected and handled (log/error/return). Do the same for
APPSTORE_ISSUER_ID, NOTARIZATION_TEAM_ID, NOTARIZATION_APPLE_ID, and
NOTARIZATION_PASSWORD in parse_shared_keys_env, and apply the same pattern in
encode_secrets_directory_files (and the other reported locations) so no
command-substitution failures are masked by a combined local+assignment.
- Around line 882-884: validate_sync_result is failing because it expects the
literal header "# GitHub Secrets Environment File" in SECRETS_FILE but
update_secrets_env only writes that header when creating a new file, not when
updating an existing one; modify update_secrets_env so that when it rewrites an
existing SECRETS_FILE it ensures the header line is present (prepend or replace
the top with "# GitHub Secrets Environment File" if missing) and avoid
duplicating the header on repeated updates, referencing the functions/variables
update_secrets_env and SECRETS_FILE and the header string used by
validate_sync_result.
- Around line 1259-1276: The awk script inside update_fastlane_config has an
unmatched closing brace; remove the extra `}` that follows the range/block close
so the block starting with `/keystore:.*\{/,/\}/ {` properly balances its
braces, and then add a defensive check after running awk to verify it succeeded
and the temp file is non-empty before moving it over the original config (e.g.,
test the exit status `$?` or use `if [ -s "$temp_file" ] && awk_exit_code=0`
pattern), so you only `mv "$temp_file" "$config_file"` when awk completed
without error to avoid clobbering project_config.rb.
---
Nitpick comments:
In @.github/workflows/CLAUDE.md:
- Around line 26-34: The fenced code block in the CLAUDE.md content (the
triple-backtick block that lists Local Workflows → Reusable Workflows → Custom
Actions → Fastlane Lanes) is missing a language specifier; update that opening
fence from ``` to ```text (or ```plain) so markdownlint MD040 is silenced and
rendering is consistent, i.e., locate the triple-backtick block in the file and
prepend the chosen language after the opening backticks.
In `@fastlane-config/extract_config.rb`:
- Line 1: Either make the script executable or remove the shebang line to
resolve the shebang vs file permission mismatch: either run chmod +x on the file
so the "#!/usr/bin/env ruby" line is valid, or delete that shebang from
extract_config.rb to stop lint warnings for a non-executable file.
In `@fastlane-config/project_config.rb`:
- Line 274: The required-file check uses Dir.pwd which can be outside the repo;
update the path resolution so it is relative to this config file instead: change
the File.exist? call that builds the candidate path (referenced in the
missing_files computation and the required_files array) to resolve files against
the directory of this config (use __dir__ or File.dirname(__FILE__) and
File.expand_path to join the parent directory) rather than Dir.pwd, so existence
is tested consistently relative to the repository/config file.
In `@fastlane/FastFile`:
- Line 475: The calls to setup_ci_if_needed drop the lane options and thus
ignore overrides like ci_provider; update each invocation of setup_ci_if_needed
(e.g., the calls currently at the same spots you found) to forward the lane
options by passing the options hash (e.g., setup_ci_if_needed(options)), so the
method receives and respects overrides such as ci_provider.
In `@Gemfile`:
- Around line 5-12: RuboCop's Bundler/OrderedGems requires gems in alphabetical
order; reorder the entries so "bigdecimal" appears before "mutex_m" and
"cocoapods" appears before "fastlane". Update the Gemfile by swapping the
positions of the pairs (ensure the lines with gem "bigdecimal" and gem "mutex_m"
are sorted, and the lines with gem "cocoapods" and gem "fastlane" are sorted) so
the gem names are alphabetically ordered.
In `@keystore-manager.sh`:
- Around line 604-678: The function update_secrets_env_with_files currently
reads ENCODED_SECRETS from its caller's scope (fragile); change it to accept the
encoded-secrets associative array explicitly (e.g., add a parameter like
encoded_array_name) and inside update_secrets_env_with_files create a nameref
with declare -n encoded_secrets_ref="$1" (or accept the output of declare -p and
eval if supporting older Bash), then replace all uses of ENCODED_SECRETS with
encoded_secrets_ref so the function no longer relies on dynamic parent-scope
variables; update callers (like encode_secrets_directory_files) to call
update_secrets_env_with_files "ENCODED_SECRETS" and keep function name
references update_secrets_env_with_files and encode_secrets_directory_files
consistent.
- Around line 68-69: The global associative array declaration for
IOS_STRING_SECRETS is incorrectly indented making it appear to be inside a
block; move the line declaring IOS_STRING_SECRETS (declare -g -A
IOS_STRING_SECRETS) to column 0 (no leading spaces) so it is visually and
actually at script/top scope like the existing MACOS_PASSWORD_SECRETS
declaration, ensuring consistent alignment for global declarations.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.github/workflows/CLAUDE.md.github/workflows/monthly-release.yaml.github/workflows/multi-platform-build-and-publish.yml.github/workflows/pr-check.yml.github/workflows/promote-to-production.ymlGemfilefastlane-config/android_config.rbfastlane-config/extract_config.rbfastlane-config/ios_config.rbfastlane-config/project_config.rbfastlane/AppFilefastlane/CLAUDE.mdfastlane/FastFilefastlane/age_rating.jsonkeystore-manager.shkeystores/original_keystore.keystoresecrets.env.template
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
keystore-manager.sh (1)
343-354:⚠️ Potential issue | 🟠 MajormacOS
base64 -iproduces line-wrapped output; Linuxbase64 -w 0does not — this inconsistency causes multi-line base64 values on macOS.On macOS,
base64 -i "$file_path"wraps output at 60 characters (no equivalent of-w 0). The resulting multi-line string is stored inENCODED_SECRETSand then written verbatim inside heredoc blocks. Invalidate_sync_result(line 1066), lines are concatenated without a newline separator (value+="$line"), producing a stripped single-line string that then gets decoded. Whilebase64 -Dtolerates this, the double-encoding/decoding asymmetry between platforms is a latent bug and will produce different env-file content on macOS vs Linux.Add macOS de-wrap step to normalise output:
🐛 Proposed fix
if [[ "$OSTYPE" == "darwin"* ]]; then - # macOS requires -i flag for input file - # Linux accepts positional argument and -w 0 for no wrapping - base64 -i "$file_path" + # macOS base64 wraps at 60 chars; strip newlines to match Linux behaviour + base64 -i "$file_path" | tr -d '\n' else # Linux base64 -w 0 "$file_path" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keystore-manager.sh` around lines 343 - 354, The macOS branch uses `base64 -i "$file_path"` which produces wrapped lines; update the macOS path where `base64` is invoked so its output is normalized to a single-line base64 string (matching Linux `base64 -w 0`) before storing in ENCODED_SECRETS and writing heredocs — e.g. post-process the `base64 -i` output to remove newline characters; ensure this change also addresses how `validate_sync_result` consumes `ENCODED_SECRETS` (function name validate_sync_result and the variable ENCODED_SECRETS) so decoding is platform-consistent and heredoc content is identical across macOS and Linux.
♻️ Duplicate comments (3)
keystore-manager.sh (3)
392-396: SC2155:local+ command-substitution on the same line masks non-zero exit codes inparse_shared_keys_env.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keystore-manager.sh` around lines 392 - 396, The assignments using "local VAR=$(...)" in parse_shared_keys_env mask non-zero exit codes; split declaration and command-substitution so failures are not hidden: declare the locals first (e.g., local APPSTORE_KEY_ID APPSTORE_ISSUER_ID NOTARIZATION_TEAM_ID NOTARIZATION_APPLE_ID NOTARIZATION_PASSWORD) and then assign each with separate lines (APPSTORE_KEY_ID=$(grep ...), etc.), optionally checking each command's exit status after assignment so parse_shared_keys_env can handle errors correctly.
573-574: SC2155:local encoded=$(encode_base64 ...)masks the function's exit code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keystore-manager.sh` around lines 573 - 574, Replace the current command that assigns encoded with a construct that preserves the command's exit status instead of masking it; specifically, stop using "local encoded=$(encode_base64 "$file_path")" followed by checking "$?" and instead run the capture inside an if-condition like "if encoded=$(encode_base64 "$file_path"); then ..." (referencing the encode_base64 function, the encoded variable and file_path) so the shell uses the command's actual exit status to drive the conditional and you can remove the separate "$?" check.
882-884:validate_sync_resultalways fails the header check for files updated (not freshly created) byupdate_secrets_env.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keystore-manager.sh` around lines 882 - 884, The header check in validate_sync_result currently uses grep -q "^# GitHub Secrets Environment File" against $SECRETS_FILE and thus fails when update_secrets_env updates an existing file where the header may not be at column 1 (or may have leading whitespace); change the check to be more lenient by matching optional leading whitespace and the exact header text (e.g. grep -qP '^\s*# GitHub Secrets Environment File' "$SECRETS_FILE") or use grep -qF without the start-anchor to search anywhere in the file; ensure you update the validate_sync_result function (and keep the header string "# GitHub Secrets Environment File" and the format_errors array usage) so updated files pass the header test.
🧹 Nitpick comments (2)
keystore-manager.sh (2)
68-69:declare -gis redundant at top level; fix indentation.
declare -gis only needed inside functions to make a variable global. At script top-level,declare -Ais already global. Additionally, the 2-space indent here is inconsistent with the rest of the top-level code.♻️ Proposed fix
- # Global associative array for iOS string secrets - declare -g -A IOS_STRING_SECRETS +# Global associative array for iOS string secrets +declare -A IOS_STRING_SECRETSApply the same fix to the
MACOS_PASSWORD_SECRETSdeclaration at line 426:-# Global associative array for macOS password secrets -declare -g -A MACOS_PASSWORD_SECRETS +declare -A MACOS_PASSWORD_SECRETS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keystore-manager.sh` around lines 68 - 69, Remove the unnecessary -g from the top-level associative array declarations and fix their indentation to match other top-level statements: change "declare -g -A IOS_STRING_SECRETS" to a top-level "declare -A IOS_STRING_SECRETS" (no extra leading spaces) and apply the same change to the MACOS_PASSWORD_SECRETS declaration so both are declared as "declare -A" with consistent top-level indentation.
1999-2073:encode-secretsandsynccase branches are inconsistently indented relative to the rest of thecasestatement.All other branches (
generate,view,add,list,delete,delete-all,help) use 4-space indentation. The two new branches use 6 spaces for the label and 14 spaces for the body, and the closing;;forsyncsits at 10 spaces. This is a readability nit but stands out in a file that is otherwise consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keystore-manager.sh` around lines 1999 - 2073, The case labels "encode-secrets" and "sync" (and their bodies, including the closing ";;") are mis-indented compared to the rest of the case branches; reformat them to match the file's 4-space indentation convention—align the label "encode-secrets)" and "sync)" with 4 spaces, indent all lines inside their bodies by 4 spaces (not 14), and place the closing ";;" at the same 4-space indentation as other branch terminators so the "encode-secrets", "sync", their bodies, and their ";;" match the existing branches (e.g., "generate", "view", "add").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keystore-manager.sh`:
- Line 1255: In update_fastlane_config, avoid combining "local
temp_file=$(mktemp)" and guard the final mv to prevent clobbering config_file:
declare local temp_file first (e.g., local temp_file), then create it with
temp_file=$(mktemp) and immediately check mktemp's exit status; run awk
redirecting to "$temp_file", capture awk's exit code, ensure the temp file
exists and is non-empty (or awk exited 0) before running mv "$temp_file"
"$config_file"; also add a trap to remove "$temp_file" on exit/failure and
handle errors (return non-zero) if mktemp or awk fail. Ensure the same pattern
is applied to the other occurrence at lines shown (the second temp_file usage).
- Line 721: In update_ios_section, avoid masking command failures by splitting
combined "local var=$(cmd)" declarations: declare variables (e.g.,
escaped_value, section_line, another_var) with local first, then assign with
separate command substitutions and immediately test their exit status (check $?
or use || return/exit) after running printf/grep/sed/head/tail; if a command
fails, handle the error (return non-zero or log and exit) instead of proceeding
to the sed/head/tail calls that assume valid output. Ensure you reference and
update the existing uses of escaped_value and section_line in update_ios_section
so failures are caught and handled.
- Line 1345: In update_gradle_config (and likewise update_fastlane_config) avoid
SC2155 by declaring temp_file in one statement and assigning it in the next,
ensure the awk/sed command succeeded before moving the temp file, and use a safe
mv (mv -- "$temp_file" "$target") plus a trap to remove the temp_file on
failure; specifically: change "local temp_file=$(mktemp)" to "local temp_file;
temp_file=$(mktemp)" (or ensure POSIX compatibility), capture the awk exit code,
only run mv to overwrite "cmp-android/build.gradle.kts" when awk returned 0 and
the temp file is non-empty, and add cleanup/trap logic so the original file is
preserved on errors.
- Around line 99-114: When both secrets files exist, the interactive read in
resolve_env_file can hang in CI; detect non-interactive stdin (use [[ -t 0 ]])
before calling read and if not a TTY, automatically pick the default (set
ENV_FILE="$SECRETS_DIR_ENV_FILE" and call print_info "Non-interactive: using
$SECRETS_DIR_ENV_FILE") instead of prompting; otherwise keep the existing read
-r -p and case logic that may set ENV_FILE="$ROOT_ENV_FILE" or
"$SECRETS_DIR_ENV_FILE".
---
Outside diff comments:
In `@keystore-manager.sh`:
- Around line 343-354: The macOS branch uses `base64 -i "$file_path"` which
produces wrapped lines; update the macOS path where `base64` is invoked so its
output is normalized to a single-line base64 string (matching Linux `base64 -w
0`) before storing in ENCODED_SECRETS and writing heredocs — e.g. post-process
the `base64 -i` output to remove newline characters; ensure this change also
addresses how `validate_sync_result` consumes `ENCODED_SECRETS` (function name
validate_sync_result and the variable ENCODED_SECRETS) so decoding is
platform-consistent and heredoc content is identical across macOS and Linux.
---
Duplicate comments:
In `@keystore-manager.sh`:
- Around line 392-396: The assignments using "local VAR=$(...)" in
parse_shared_keys_env mask non-zero exit codes; split declaration and
command-substitution so failures are not hidden: declare the locals first (e.g.,
local APPSTORE_KEY_ID APPSTORE_ISSUER_ID NOTARIZATION_TEAM_ID
NOTARIZATION_APPLE_ID NOTARIZATION_PASSWORD) and then assign each with separate
lines (APPSTORE_KEY_ID=$(grep ...), etc.), optionally checking each command's
exit status after assignment so parse_shared_keys_env can handle errors
correctly.
- Around line 573-574: Replace the current command that assigns encoded with a
construct that preserves the command's exit status instead of masking it;
specifically, stop using "local encoded=$(encode_base64 "$file_path")" followed
by checking "$?" and instead run the capture inside an if-condition like "if
encoded=$(encode_base64 "$file_path"); then ..." (referencing the encode_base64
function, the encoded variable and file_path) so the shell uses the command's
actual exit status to drive the conditional and you can remove the separate "$?"
check.
- Around line 882-884: The header check in validate_sync_result currently uses
grep -q "^# GitHub Secrets Environment File" against $SECRETS_FILE and thus
fails when update_secrets_env updates an existing file where the header may not
be at column 1 (or may have leading whitespace); change the check to be more
lenient by matching optional leading whitespace and the exact header text (e.g.
grep -qP '^\s*# GitHub Secrets Environment File' "$SECRETS_FILE") or use grep
-qF without the start-anchor to search anywhere in the file; ensure you update
the validate_sync_result function (and keep the header string "# GitHub Secrets
Environment File" and the format_errors array usage) so updated files pass the
header test.
---
Nitpick comments:
In `@keystore-manager.sh`:
- Around line 68-69: Remove the unnecessary -g from the top-level associative
array declarations and fix their indentation to match other top-level
statements: change "declare -g -A IOS_STRING_SECRETS" to a top-level "declare -A
IOS_STRING_SECRETS" (no extra leading spaces) and apply the same change to the
MACOS_PASSWORD_SECRETS declaration so both are declared as "declare -A" with
consistent top-level indentation.
- Around line 1999-2073: The case labels "encode-secrets" and "sync" (and their
bodies, including the closing ";;") are mis-indented compared to the rest of the
case branches; reformat them to match the file's 4-space indentation
convention—align the label "encode-secrets)" and "sync)" with 4 spaces, indent
all lines inside their bodies by 4 spaces (not 14), and place the closing ";;"
at the same 4-space indentation as other branch terminators so the
"encode-secrets", "sync", their bodies, and their ";;" match the existing
branches (e.g., "generate", "view", "add").
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
keystore-manager.sh
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
fastlane-config/ios_config.rb (1)
23-25:⚠️ Potential issue | 🟠 MajorAdd explicit App Store provisioning profile key to
BUILD_CONFIG.
BUILD_CONFIGstill exposes only the adhoc profile on Line 24. App Store/TestFlight lanes that readios_config[:provisioning_profile_appstore]can still fall back incorrectly.Suggested fix
app_identifier: ProjectConfig::IOS[:app_identifier], provisioning_profile_name: ProjectConfig::IOS_SHARED[:code_signing][:provisioning_profiles][:adhoc], + provisioning_profile_appstore: ProjectConfig::IOS_SHARED[:code_signing][:provisioning_profiles][:appstore], git_url: ProjectConfig::IOS_SHARED[:code_signing][:match_git_url],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fastlane-config/ios_config.rb` around lines 23 - 25, BUILD_CONFIG currently only sets the adhoc provisioning profile which can cause App Store/TestFlight lanes to fall back incorrectly; add an explicit provisioning_profile_appstore key (e.g., provisioning_profile_appstore) to the same BUILD_CONFIG/ios_config hash next to app_identifier, provisioning_profile_name and git_url and populate it from ProjectConfig::IOS_SHARED[:code_signing][:provisioning_profiles][:appstore] (or the equivalent appstore key) so lanes that read ios_config[:provisioning_profile_appstore] get the correct profile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@fastlane-config/ios_config.rb`:
- Around line 23-25: BUILD_CONFIG currently only sets the adhoc provisioning
profile which can cause App Store/TestFlight lanes to fall back incorrectly; add
an explicit provisioning_profile_appstore key (e.g.,
provisioning_profile_appstore) to the same BUILD_CONFIG/ios_config hash next to
app_identifier, provisioning_profile_name and git_url and populate it from
ProjectConfig::IOS_SHARED[:code_signing][:provisioning_profiles][:appstore] (or
the equivalent appstore key) so lanes that read
ios_config[:provisioning_profile_appstore] get the correct profile.
biplab1
reviewed
Mar 2, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/multi-platform-build-and-publish.yml (1)
123-126:⚠️ Potential issue | 🟠 MajorRestore iOS delivery input mapping instead of only documenting the conflict.
At Line 123, the comment is correct, but both iOS delivery flags are still omitted from the reusable workflow call, so operators cannot trigger TestFlight/App Store distribution from this workflow UI.
💡 Minimal fix (mutually exclusive mapping)
distribute_ios_firebase: ${{ inputs.distribute_ios_firebase }} + distribute_ios_testflight: ${{ !inputs.distribute_ios_appstore && inputs.distribute_ios_testflight }} + distribute_ios_appstore: ${{ inputs.distribute_ios_appstore }} # When App Store is enabled, skip TestFlight — deliver() already uploads # the IPA to App Store Connect which makes the build available in TestFlight. # Running both in parallel causes version conflicts since the upstream # reusable workflow has no job dependency between the two.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/multi-platform-build-and-publish.yml around lines 123 - 126, Re-add the iOS delivery input mappings to the reusable workflow call so operators can trigger TestFlight or App Store distribution from the workflow UI; specifically restore the two inputs (e.g., enable_testflight / enable_app_store or testflight_delivery / app_store_delivery) on the reusable-workflow `uses:` step and wire them to the existing upstream inputs, and enforce mutual exclusivity with an expression (set app_store = github.event.inputs.app_store && !github.event.inputs.testflight, or vice-versa) so only one delivery target is true at a time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/multi-platform-build-and-publish.yml:
- Around line 123-126: Re-add the iOS delivery input mappings to the reusable
workflow call so operators can trigger TestFlight or App Store distribution from
the workflow UI; specifically restore the two inputs (e.g., enable_testflight /
enable_app_store or testflight_delivery / app_store_delivery) on the
reusable-workflow `uses:` step and wire them to the existing upstream inputs,
and enforce mutual exclusivity with an expression (set app_store =
github.event.inputs.app_store && !github.event.inputs.testflight, or vice-versa)
so only one delivery target is true at a time.
biplab1
reviewed
Mar 2, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/CLAUDE.md (1)
4-4:⚠️ Potential issue | 🟡 MinorStale reusable-workflow version references remain unresolved.
@v1.0.8is still documented at Line 4, Line 23, Line 50, Line 107, and Line 147, which conflicts with the PR’s move to@v1.0.11.Also applies to: 23-23, 50-50, 107-107, 147-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/CLAUDE.md at line 4, The file still references the old reusable-workflow version string `openMF/mifos-x-actionhub@v1.0.8` in multiple places; update every occurrence of that exact token in .github/CLAUDE.md to `openMF/mifos-x-actionhub@v1.0.11` so the document matches the PR change (search for `openMF/mifos-x-actionhub@v1.0.8` and replace all instances).
🧹 Nitpick comments (1)
.github/CLAUDE.md (1)
26-34: Add a language identifier to the fenced code block.The architecture block starts at Line 26 without a language tag, which triggers markdownlint MD040.
✏️ Suggested edit
-``` +```text Local Workflows (.github/workflows/) ↓ Reusable Workflows (mifos-x-actionhub/.github/workflows/) ↓ Custom Actions (mifos-x-actionhub-*/) ↓ Fastlane Lanes (fastlane/Fastfile)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.github/CLAUDE.md around lines 26 - 34, The fenced code block in
.github/CLAUDE.md that contains the architecture diagram (the four lines
starting with "Local Workflows (.github/workflows/)") is missing a language
identifier and triggers markdownlint MD040; update the opening fence to include
a language tag (for example "text") so it becomes ```text to silence the linter
and preserve formatting of the block.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/CLAUDE.md:
- Line 570: The document uses two different App Store auth key filenames;
replace the inconsistentAuth_key.p8occurrence with the canonical
AuthKey.p8so all references match; update the stringAuth_key.p8to
AuthKey.p8(and verify any other occurrences ofAuthKey.p8orAuth_key.p8
in the doc are made consistent) to avoid operator confusion.- Line 553: The Firebase secrets row in the CLAUDE.md table shows a count of 3
but only lists FIREBASECREDS and GOOGLESERVICES; update the row so the count
matches the listed secrets by either changing the count from 3 to 2 or adding
the missing secret name to the list (referencing FIREBASECREDS and
GOOGLESERVICES to locate the row and ensure consistency).
Duplicate comments:
In @.github/CLAUDE.md:
- Line 4: The file still references the old reusable-workflow version string
openMF/mifos-x-actionhub@v1.0.8in multiple places; update every occurrence of
that exact token in .github/CLAUDE.md toopenMF/mifos-x-actionhub@v1.0.11so
the document matches the PR change (search foropenMF/mifos-x-actionhub@v1.0.8
and replace all instances).
Nitpick comments:
In @.github/CLAUDE.md:
- Around line 26-34: The fenced code block in .github/CLAUDE.md that contains
the architecture diagram (the four lines starting with "Local Workflows
(.github/workflows/)") is missing a language identifier and triggers
markdownlint MD040; update the opening fence to include a language tag (for
example "text") so it becomes ```text to silence the linter and preserve
formatting of the block.</details> --- <details> <summary>ℹ️ Review info</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 9120c47bb4dfd2bb06bf32d3a4012b4e4272b06c and a9b500059e06e5dba21eb4dacc79f0fc8528a98e. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `.github/CLAUDE.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
biplab1
reviewed
Mar 4, 2026
biplab1
reviewed
Mar 4, 2026
biplab1
reviewed
Mar 4, 2026
Contributor
|
@Aryan-Baglane Can you please tell us the steps taken to implement this PR? |
2 tasks
Contributor
|
@Aryan-Baglane Please refer this PR review comments and address them in your PR: openMF/mifos-x-field-officer-app#2639 |
niyajali
requested changes
Mar 18, 2026
Collaborator
|
@Aryan-Baglane It looks like just one workflow file has been modified; have the others already updated? and copy the Gemfile and run the |
Member
Author
Yes sir there was one pr before this pr which was got merged so some file were got updated and for gemfile.lock file i have ran the bundle update to update that file |
niyajali
requested changes
Apr 1, 2026
Collaborator
niyajali
left a comment
niyajali
left a comment
There was a problem hiding this comment.
do these changes and verify everything, and lemme know we'll proceed with merge
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
4 participants
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.
Fixes - MM-554
Didn't create a Jira ticket, click here to create new.
Please Add Screenshots If there are any UI changes.
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit