AppConfig refactor #870
AppConfig refactor #870
Conversation
WalkthroughAdds JSONB-backed per-integration configuration, new namespaced config controllers and Turbo Frame edit views, integration-level accessors/validations, routes and migrations, component/UI updates to surface per-category setup, and updates App readiness logic to use integration-based readiness. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant Page as Integrations Page
participant Card as IntegrationCardComponent
participant Modal as ModalComponent
participant TF as Turbo Frame
participant C as ConfigsController
participant I as Integration(Model)
participant App as App
U->>Page: open app integrations
Page->>Card: render for each integration
Card->>I: query further_setup? / pre_open_category?
alt configure available
U->>Card: click Configure (cog)
Card->>Modal: open modal (light action)
Modal->>TF: load frame (category_config_turbo_frame_id)
TF->>C: GET edit (app + integration category)
C->>I: load config data (repos/projects/apps)
C-->>TF: render form partial
U->>TF: submit Update (PATCH)
TF->>C: update (parsed JSON params)
C->>I: persist config JSON
alt success
C-->>U: redirect to app_path with success notice
else failure
C-->>U: redirect back with error flash
end
else only disconnect
U->>Card: click Disconnect
Card->>I: send DELETE (Turbo default)
end
sequenceDiagram
autonumber
participant AppsCtrl as AppsController
participant App as App
participant Integrations as Integration
AppsCtrl->>App: call ready?
App->>Integrations: Integration.configured?
Integrations->>Integrations: further_setup_by_category (aggregate readiness)
alt ready
AppsCtrl-->>View: set @train_in_creation
else not ready
AppsCtrl-->>View: set @app_setup_instructions
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
Animesh-Ghosh
force-pushed
the
chore/app-config-refactor
branch
2 times, most recently
from
2453349 to
e0e39e8
Compare
Animesh-Ghosh
force-pushed
the
chore/app-config-refactor
branch
4 times, most recently
from
c7d159b to
2f007eb
Compare
…grations Update the read path on `AppConfig#further_setup_by_category?` for `code_repository`. This set of changes has a few TODO items that should be resolved once the remaining configurable integrations are brought under the fold of the refactor.
Bitbucket version control configuration requires the user to select a workspace to select the code repository. The workspace dropdown is supposed work by re-rendering the configuration form via Turbo stream. However, the partial for the TURBO_STREAM format was missing, which would break the interaction, along with raising a server-side error. These changes refactor the HTML partial to render the form in both the HTML and the TURBO_STREAM formats, to make the drop-down UX work.
Update read path on `AppConfig#further_setup_by_category?` for `ci_cd`. Fix config update translations.
Animesh-Ghosh
force-pushed
the
chore/app-config-refactor
branch
2 times, most recently
from
e5f05d8 to
be7cdaa
Compare
Update read path on `AppConfig#further_setup_by_category?` for `build_channel`.
Animesh-Ghosh
force-pushed
the
chore/app-config-refactor
branch
from
be7cdaa to
73ac1b9
Compare
The persisted config value, when serialized, could generate a JSON string that could be different from the one generated by the view helper for the drop-down options' values, which would make it so that the persisted config value is not pre-selected by default. Thus, we explicitly find the correct config and use that as the default, rather than using the persisted value.
Simplify some config read paths in `AppConfig`.
Update read paths for `monitoring` config in `AppConfig` for: - `#further_setup_by_category?` - bugsnag related attributes
…ntegrations Update read path for `project_management` config in `AppConfig#further_setup_by_category?`. Remove unnecessary code and markup from `IntegrationCard` and `IntegrationList` components. Remove unused strong params method in `Monitoring::BugsnagConfigsController` controller.
Removing model validations from `AppConfig` has to be done as part of deprecating the model.
…tions TODO: fix failing specs.
Animesh-Ghosh
force-pushed
the
chore/app-config-refactor
branch
from
d62d187 to
a96cd2b
Compare
Animesh-Ghosh
force-pushed
the
chore/app-config-refactor
branch
from
ca1259e to
397e9d7
Compare
Animesh-Ghosh
force-pushed
the
chore/app-config-refactor
branch
from
260b54d to
7d32eb4
Compare
Animesh-Ghosh
force-pushed
the
chore/app-config-refactor
branch
from
700cd34 to
d019bd2
Compare
Since factoring out the `code_repository` to individual integration providers, creating trains would fail since CI/CD integrations wouldn't have the necessary `repository_config` present. This was observed only for Github, Gitlab and Bitbucket integrations. Thus, we now allow CI/CD integrations to now be configurable.
Update the new integration config controllers to redirect to the app's show page. Separate integration connection-readiness and integration config-readiness checks so that the wizard isn't stuck at just showing the "Complete your app setup" section. Update CI/CD integration readiness check.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
app/views/linear_integration/_release_filter_form.html.erb (1)
4-10: Add nested attributes configuration to LinearIntegration model
TheLinearIntegrationmodel is missingaccepts_nested_attributes_for :release_filters, allow_destroy: true, so the_destroyhidden field in the form won’t work as intended.app/models/jira_integration.rb (1)
98-102: Fix undefined variable in complete_access (resource is nil).
resourceis not defined in this branch, leading to a NameError. Useresources.first.- if resources.length == 1 - self.cloud_id = resources.first["id"] - self.organization_url = resource["url"] - self.organization_name = resource["name"] - true + if resources.length == 1 + resource = resources.first + self.cloud_id = resource["id"] + self.organization_url = resource["url"] + self.organization_name = resource["name"] + trueapp/models/integration.rb (1)
177-215: Use connected. for provider lookups to avoid stale/disconnected picks*Provider helpers should align with readiness checks and only consider connected integrations.
- def vcs_provider - kept.version_control.first&.providable - end + def vcs_provider + connected.version_control.first&.providable + end - def ci_cd_provider - kept.ci_cd.first&.providable - end + def ci_cd_provider + connected.ci_cd.first&.providable + end - def bitrise_ci_cd_provider - kept.ci_cd.find(&:bitrise_integration?)&.providable - end + def bitrise_ci_cd_provider + connected.ci_cd.find(&:bitrise_integration?)&.providable + end - def monitoring_provider - kept.monitoring.first&.providable - end + def monitoring_provider + connected.monitoring.first&.providable + end - def android_store_provider - kept.build_channel.find(&:google_play_store_integration?)&.providable - end + def android_store_provider + connected.build_channel.find(&:google_play_store_integration?)&.providable + end - def ios_store_provider - kept.build_channel.find(&:app_store_integration?)&.providable - end + def ios_store_provider + connected.build_channel.find(&:app_store_integration?)&.providable + end - def slack_build_channel_provider - kept.build_channel.find(&:slack_integration?)&.providable - end + def slack_build_channel_provider + connected.build_channel.find(&:slack_integration?)&.providable + end - def firebase_build_channel_provider - kept.build_channel.find(&:google_firebase_integration?)&.providable - end + def firebase_build_channel_provider + connected.build_channel.find(&:google_firebase_integration?)&.providable + end - def project_management_provider - kept.project_management.first&.providable - end + def project_management_provider + connected.project_management.first&.providable + endapp/models/bitbucket_integration.rb (1)
218-220: Fix URL encoding: CGI.escapeURIComponent does not exist in RubyThis will raise at runtime. Use ERB::Util.url_encode (or CGI.escape) instead.
Apply this diff:
- def compare_url(to_branch, from_branch) - "https://bitbucket.org/#{code_repository_name}/branch/#{from_branch}?dest=#{CGI.escapeURIComponent(to_branch)}" - end + def compare_url(to_branch, from_branch) + "https://bitbucket.org/#{code_repository_name}/branch/#{from_branch}?dest=#{ERB::Util.url_encode(to_branch)}" + endapp/models/app_config.rb (1)
76-94: Project management readiness short-circuits when both Jira and Linear existIf Jira is present but not ready, Linear readiness isn’t checked. Either rely on the app’s project_management_provider or OR-combine readiness.
- if jira - return jira.project_config.present? && - jira.project_config["selected_projects"].present? && - jira.project_config["selected_projects"].any? && - jira.project_config["project_configs"].present? - end - - if linear - return linear.project_config.present? && - linear.project_config["selected_teams"].present? && - linear.project_config["selected_teams"].any? && - linear.project_config["team_configs"].present? - end + if jira || linear + jira_ready = jira && jira.project_config.present? && + jira.project_config["selected_projects"].present? && + jira.project_config["selected_projects"].any? && + jira.project_config["project_configs"].present? + + linear_ready = linear && linear.project_config.present? && + linear.project_config["selected_teams"].present? && + linear.project_config["selected_teams"].any? && + linear.project_config["team_configs"].present? + + return jira_ready || linear_ready + end
♻️ Duplicate comments (1)
app/views/version_control/gitlab_configs/edit.html+turbo_frame.erb (1)
8-15: Duplicate repository selection logic.This view duplicates the same complex repository selection logic from
app/views/ci_cd/github_configs/edit.html+turbo_frame.erb(lines 9-16).See the helper extraction suggestion in the GitHub config view review. The same helper method would simplify this view as well:
- <%= section.F.labeled_select :repository_config, - "Code Repository", - options_for_select( - display_channels(@code_repositories) { |repo| repo[:full_name] }, - @code_repositories.find { |repo| repo[:id] == @gitlab_integration.repository_config&.dig("id") }&.to_json - ) %> + <%= section.F.labeled_select :repository_config, + "Code Repository", + repository_selection_options(@code_repositories, @gitlab_integration.repository_config) %>
🧹 Nitpick comments (34)
app/components/integration_card_component.html.erb (1)
64-65: LGTM; consider i18n for the button label.Looks good. Optionally wrap "Connect" with I18n.
app/components/integration_card_component.rb (4)
83-90: Minor: consider i18n for the "Configure" prefix.Use a translation key so UI is localizable.
91-104: Base routing switch on @category for consistency.Avoid relying on integration.category if @category is the source of truth for this card:
-def edit_config_path - case integration.category +def edit_config_path + case @category.to_s when "version_control" then edit_app_version_control_config_path when "ci_cd" then edit_app_ci_cd_config_path when "build_channel" then edit_app_build_channel_config_path when "monitoring" then edit_app_monitoring_config_path when "project_management" then edit_app_project_management_config_path else unsupported_integration_category end endEnsure @category always matches integration.category in callers.
108-147: Route mapping via large case trees is brittle; push to integration/providable.Define an API on integration/providable (e.g., integration.edit_config_path_for(@category, app: @app)) and delegate here. This removes per-type branching from the component and centralizes ownership.
I can draft the interface and a stepwise refactor if helpful.
149-155: Use ArgumentError for unsupported inputs (optional).Raising ArgumentError better communicates invalid input than TypeError here:
-def unsupported_integration_category - raise TypeError, "Unsupported integration category: #{integration.category}" +def unsupported_integration_category + raise ArgumentError, "Unsupported integration category: #{integration.category}" end -def unsupported_integration_type - raise TypeError, "Unsupported integration type: #{integration.providable_type}" +def unsupported_integration_type + raise ArgumentError, "Unsupported integration type: #{integration.providable_type}" endapp/models/concerns/app_configurable.rb (1)
4-5: Acknowledge the noted refactoring opportunity.The comments correctly identify that
firebase_appcould be moved toGoogleFirebaseIntegrationto further align with the integration-specific configuration pattern established in this PR.Would you like me to help refactor this method into
GoogleFirebaseIntegration, or should this be tracked as a follow-up task?app/views/ci_cd/github_configs/edit.html+turbo_frame.erb (1)
9-16: Consider extracting repository selection logic into a helper.The repository selection logic (finding and preselecting the repository) is complex and will be duplicated across similar views (GitLab, Bitbucket).
Consider extracting this into a view helper method:
# app/helpers/integration_configs_helper.rb def repository_selection_options(repositories, selected_repository_config) options = display_channels(repositories) { |repo| repo[:full_name] } selected = repositories.find { |repo| repo[:id] == selected_repository_config&.dig("id") }&.to_json options_for_select(options, selected) endThen simplify the view to:
- <%= section.F.labeled_select :repository_config, - "Code Repository", - options_for_select( - display_channels(@code_repositories) { |repo| repo[:full_name] }, - @code_repositories.find { |repo| repo[:id] == @github_integration.repository_config&.dig("id") }&.to_json - ) %> + <%= section.F.labeled_select :repository_config, + "Code Repository", + repository_selection_options(@code_repositories, @github_integration.repository_config) %>app/controllers/ci_cd/bitrise_configs_controller.rb (1)
1-53: Consider extracting the common controller pattern.This controller follows the same structure as other CI/CD config controllers (GitHub, GitLab, Bitbucket). Consider extracting the common pattern into a concern or base class to reduce duplication while allowing integration-specific behavior.
For example, a concern could provide:
- Standard
set_appand redirect error handling- Parameter parsing pattern with safe_json_parse
- Standard update flow with redirect paths
Each controller would then only need to define integration-specific methods like
set_ci_cd_projects.spec/factories/apps.rb (1)
9-11: Track the TODO for removing app_config.The comment indicates this line can be removed, suggesting it's technical debt left during the gradual migration from AppConfig to per-integration configs.
Would you like me to open a new issue to track the removal of
app.configfrom the factory once all integration-specific configurations have been fully migrated?app/views/project_management/linear_configs/edit.html+turbo_frame.erb (1)
2-2: Prefer safe navigation for nil checks.The conditional
@linear_data && @linear_data[:teams].present?works but could be more idiomatic.Consider using safe navigation:
- <% if @linear_data && @linear_data[:teams].present? %> + <% if @linear_data&.dig(:teams).present? %>app/models/jira_integration.rb (1)
65-65: Release filters validation: solid, consider reuse with Linear.Validation logic is correct. Given identical logic in LinearIntegration, consider extracting a small concern/helper to DRY it up while keeping per‑integration VALID_FILTER_TYPES.
Also applies to: 268-274
app/models/bitrise_integration.rb (1)
64-68: Good: read project from project_config; add nil guards at call sites.Accessor is fine. When
project_configis unset,projectis nil; downstream calls likeworkflows,trigger_workflow_run!, andartifact_urlmay break.
- Add early returns (e.g.,
return [] if project.blank?) in read paths and clear errors/raise in write/trigger paths when not configured.- Verify current API methods safely handle
nilapp id.app/views/project_management/jira_configs/edit.html+turbo_frame.erb (1)
13-14: Name consistency: current_jira_config vs project_config.The partial now binds to project_config; consider renaming
current_jira_configtocurrent_project_configfor clarity.app/models/linear_integration.rb (1)
63-63: Same release_filters validator duplication as Jira.Works as intended; consider extracting to a small shared concern to avoid drift.
Also applies to: 246-252
app/views/monitoring/_bugsnag_project_form.html.erb (2)
10-10: Escape JSON for data attributeEmbed JSON safely to avoid breaking the attribute and to prevent injection. Use json_escape (or j) on to_json.
- data-nested-select-options-value="<%= projects.to_json %>" + data-nested-select-options-value="<%= json_escape(projects.to_json) %>"
14-21: *Avoid using full JSON as option value for _project_idThe select name suggests it expects an ID, but the option values look like JSON blobs. Prefer using the project id as the option value and keep the JSON in data-* attributes (or via the nested-select controller state). This simplifies params handling and reduces payload size.
config/locales/en.yml (1)
780-821: Reduce duplication via shared i18n keyThese per-provider update.success messages are identical. Consider referencing a shared message (e.g., config.update.success) to reduce duplication and future drift.
db/schema.rb (1)
382-390: Legacy app_id column still presentModel ignores google_firebase_integrations.app_id. Consider a follow-up migration to drop this column to reduce confusion and risk of stale data.
app/models/github_integration.rb (2)
138-141: Wrap external call in retries for resiliencyAlign with other providers (e.g., GitLab/Bitbucket) and wrap list_repos with with_api_retries.
-def repos(_ = nil) - installation.list_repos(REPOS_TRANSFORMATIONS) -end +def repos(_ = nil) + with_api_retries { installation.list_repos(REPOS_TRANSFORMATIONS) } +end
382-384: Handle missing namespace in namespaced_branchAvoid generating ":branch" when namespace is absent.
-def namespaced_branch(branch_name) - [code_repo_namespace, ":", branch_name].join -end +def namespaced_branch(branch_name) + return branch_name if code_repo_namespace.blank? + "#{code_repo_namespace}:#{branch_name}" +endapp/controllers/version_control/github_configs_controller.rb (1)
45-51: Make repository_config parsing type-safeHandle both String and Hash inputs to avoid NoMethodError on Hash.
- github_config_params = params.require(:github_integration) - .permit(:repository_config) - github_config_params.merge( - repository_config: github_config_params[:repository_config]&.safe_json_parse - ) + github_config_params = params.require(:github_integration).permit(:repository_config) + repo_cfg = github_config_params[:repository_config] + repo_cfg = repo_cfg.is_a?(String) ? repo_cfg.safe_json_parse : repo_cfg + github_config_params.merge(repository_config: repo_cfg)app/models/integration.rb (1)
164-171: Tighten configured? to connected scopeUse connected.exists? and avoid relying on Relation#none? semantics.
- def configured? - return false if none? # need at least one integration - - further_setup_by_category - .values - .pluck(:ready) - .all? - end + def configured? + return false unless connected.exists? + further_setup_by_category.values.pluck(:ready).all? + endapp/controllers/ci_cd/gitlab_configs_controller.rb (1)
45-51: Type-safe parsing for repository_configAccept both String and Hash inputs.
- gitlab_config_params = params.require(:gitlab_integration) - .permit(:repository_config) - gitlab_config_params.merge( - repository_config: gitlab_config_params[:repository_config]&.safe_json_parse - ) + gitlab_config_params = params.require(:gitlab_integration).permit(:repository_config) + repo_cfg = gitlab_config_params[:repository_config] + repo_cfg = repo_cfg.is_a?(String) ? repo_cfg.safe_json_parse : repo_cfg + gitlab_config_params.merge(repository_config: repo_cfg)app/controllers/ci_cd/github_configs_controller.rb (1)
12-16: Consider adding turbo_stream response for consistencyOther CI/CD config controllers support turbo_stream. Add if desired for uniform UX.
Apply this diff:
respond_to do |format| format.html do |variant| variant.turbo_frame { render :edit } end + format.turbo_stream { render :edit } endapp/controllers/project_management/linear_configs_controller.rb (1)
64-65: Side effects on GETPersisting defaults in edit (save! if changed?) writes on GET. Consider deferring to update or a background init.
app/controllers/ci_cd/bitbucket_configs_controller.rb (1)
42-46: Handle empty workspace list gracefullyIf there are no workspaces, repos(nil) may fail. Default to empty repos.
Apply this diff:
def set_code_repositories @workspaces = @bitbucket_integration.workspaces || [] - workspace = params[:workspace] || @workspaces.first - @code_repositories = @bitbucket_integration.repos(workspace) + workspace = params[:workspace] || @workspaces.first + @code_repositories = workspace.present? ? @bitbucket_integration.repos(workspace) : [] endapp/models/app_config.rb (4)
60-62: Prefer dig over fetch for project_config idSimpler and avoids potential KeyError semantics.
- app.ci_cd_provider&.project_config&.fetch("id", nil).present? + app.ci_cd_provider&.project_config&.dig("id").present?
64-68: Add safe-nav to Bugsnag readiness; keep provider access consistentMinor safety/consistency improvement.
- monitoring_provider = app.monitoring_provider - configs_ready?(monitoring_provider.ios_config, monitoring_provider.android_config) + monitoring_provider = app.monitoring_provider + configs_ready?(monitoring_provider&.ios_config, monitoring_provider&.android_config)
70-74: Deduplicate configs_ready? with Integration-level helperThis duplicates Integration.configs_ready? with a different signature, inviting drift. Centralize and delegate to the canonical helper.
Example:
def configs_ready?(ios, android) app.integrations.configs_ready?(app, android, ios) end
31-40: Move remaining validations to integration models and remove from AppConfigYou already left a TODO. Recommend migrating these to JiraIntegration/LinearIntegration and deleting the AppConfig counterparts to avoid split validation logic.
app/controllers/project_management/jira_configs_controller.rb (4)
61-65: Heuristic for “done” statuses may be too strictMatching name == "Done" likely misses custom “Done” categories. Prefer statusCategory key (e.g., status['statusCategory']['key'] == 'done') if available.
68-69: Avoid persisting defaults during GET editSaving in set_jira_projects causes side effects on GET. Prefer computing defaults in-memory and persisting only on update.
Possible approaches:
- Move the default-initialization into update if config blank.
- Or wrap save! in a non-GET guard.
2-2: Unused refinement importRefinedString isn’t used here. Consider removing to reduce surface area.
90-91: Compact blanks in selected_projectsPrevents empty strings sneaking in.
- selected_projects: Array(config_params[:selected_projects]), + selected_projects: Array(config_params[:selected_projects]).compact_blank,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (70)
app/components/integration_card_component.html.erb(2 hunks)app/components/integration_card_component.rb(3 hunks)app/components/integration_list_component.html.erb(1 hunks)app/components/integration_list_component.rb(0 hunks)app/components/live_release/finalize_component.rb(1 hunks)app/controllers/apps_controller.rb(1 hunks)app/controllers/build_channel/google_firebase_configs_controller.rb(1 hunks)app/controllers/ci_cd/bitbucket_configs_controller.rb(1 hunks)app/controllers/ci_cd/bitrise_configs_controller.rb(1 hunks)app/controllers/ci_cd/github_configs_controller.rb(1 hunks)app/controllers/ci_cd/gitlab_configs_controller.rb(1 hunks)app/controllers/monitoring/bugsnag_configs_controller.rb(1 hunks)app/controllers/project_management/jira_configs_controller.rb(1 hunks)app/controllers/project_management/linear_configs_controller.rb(1 hunks)app/controllers/version_control/bitbucket_configs_controller.rb(1 hunks)app/controllers/version_control/github_configs_controller.rb(1 hunks)app/controllers/version_control/gitlab_configs_controller.rb(1 hunks)app/libs/coordinators/webhooks/pull_request.rb(1 hunks)app/libs/coordinators/webhooks/push.rb(1 hunks)app/models/app.rb(2 hunks)app/models/app_config.rb(3 hunks)app/models/bitbucket_integration.rb(3 hunks)app/models/bitrise_integration.rb(2 hunks)app/models/bugsnag_integration.rb(4 hunks)app/models/concerns/app_configurable.rb(1 hunks)app/models/github_integration.rb(4 hunks)app/models/gitlab_integration.rb(5 hunks)app/models/google_firebase_integration.rb(3 hunks)app/models/integration.rb(3 hunks)app/models/jira_integration.rb(4 hunks)app/models/linear_integration.rb(4 hunks)app/views/build_channel/_firebase_integration_form.html.erb(1 hunks)app/views/build_channel/google_firebase_configs/edit.html+turbo_frame.erb(1 hunks)app/views/ci_cd/bitbucket_configs/_form.html.erb(1 hunks)app/views/ci_cd/bitbucket_configs/edit.html+turbo_frame.erb(1 hunks)app/views/ci_cd/bitbucket_configs/edit.turbo_stream.erb(1 hunks)app/views/ci_cd/bitrise_configs/edit.html+turbo_frame.erb(1 hunks)app/views/ci_cd/github_configs/edit.html+turbo_frame.erb(1 hunks)app/views/ci_cd/gitlab_configs/edit.html+turbo_frame.erb(1 hunks)app/views/jira_integration/_project_selection.html.erb(2 hunks)app/views/jira_integration/_release_filter_form.html.erb(1 hunks)app/views/linear_integration/_release_filter_form.html.erb(1 hunks)app/views/linear_integration/_team_selection.html.erb(2 hunks)app/views/monitoring/_bugsnag_project_form.html.erb(1 hunks)app/views/monitoring/bugsnag_configs/edit.html+turbo_frame.erb(1 hunks)app/views/project_management/jira_configs/edit.html+turbo_frame.erb(1 hunks)app/views/project_management/linear_configs/edit.html+turbo_frame.erb(1 hunks)app/views/version_control/bitbucket_configs/_form.html.erb(1 hunks)app/views/version_control/bitbucket_configs/edit.html+turbo_frame.erb(1 hunks)app/views/version_control/bitbucket_configs/edit.turbo_stream.erb(1 hunks)app/views/version_control/github_configs/edit.html+turbo_frame.erb(1 hunks)app/views/version_control/gitlab_configs/edit.html+turbo_frame.erb(1 hunks)config/locales/en.yml(1 hunks)config/routes.rb(1 hunks)db/migrate/20250908020424_add_repository_config_to_vcs_integrations.rb(1 hunks)db/migrate/20250909193500_add_project_config_to_bitrise_integration.rb(1 hunks)db/migrate/20250918025642_add_config_to_google_firebase_integration.rb(1 hunks)db/migrate/20250919124803_add_config_to_bugsnag_integration.rb(1 hunks)db/migrate/20250924035033_add_config_to_project_management_integrations.rb(1 hunks)db/schema.rb(6 hunks)spec/factories/apps.rb(1 hunks)spec/factories/bitbucket_integrations.rb(1 hunks)spec/factories/github_integrations.rb(1 hunks)spec/factories/gitlab_integrations.rb(1 hunks)spec/factories/integrations.rb(1 hunks)spec/factories/jira_integrations.rb(1 hunks)spec/libs/triggers/pull_request_spec.rb(5 hunks)spec/models/gitlab_integration_spec.rb(7 hunks)spec/models/jira_integration_spec.rb(3 hunks)spec/models/linear_integration_spec.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/components/integration_list_component.rb
🧰 Additional context used
🧬 Code graph analysis (31)
spec/models/jira_integration_spec.rb (2)
app/libs/installations/jira/api.rb (1)
include(2-150)app/models/jira_integration.rb (1)
fetch_tickets_for_release(160-176)
app/libs/coordinators/webhooks/push.rb (6)
app/models/integration.rb (1)
vcs_provider(177-179)app/models/bitbucket_integration.rb (1)
code_repository_name(33-35)app/models/github_integration.rb (1)
code_repository_name(113-115)app/models/gitlab_integration.rb (1)
code_repository_name(159-161)app/libs/coordinators/webhooks/bitbucket/pull_request.rb (1)
repository_name(25-27)app/libs/coordinators/webhooks/bitbucket/push.rb (2)
repository_name(35-37)branch_name(31-33)
app/models/app.rb (1)
app/models/integration.rb (3)
ready?(160-162)configured?(164-171)further_setup_by_category(227-267)
spec/models/gitlab_integration_spec.rb (1)
app/models/gitlab_integration.rb (2)
installation(334-336)code_repository_name(159-161)
app/controllers/monitoring/bugsnag_configs_controller.rb (4)
app/controllers/application_controller.rb (1)
current_organization(30-42)app/models/integration.rb (1)
monitoring_provider(189-191)app/models/bugsnag_integration.rb (1)
setup(83-85)app/refinements/refined_string.rb (1)
safe_json_parse(42-46)
app/models/github_integration.rb (2)
app/models/bitbucket_integration.rb (5)
code_repository_name(33-35)code_repo_url(37-39)code_repo_namespace(41-41)code_repo_name_only(43-43)repos(165-167)app/models/gitlab_integration.rb (5)
code_repository_name(159-161)code_repo_url(163-165)code_repo_namespace(167-167)code_repo_name_only(169-169)repos(198-200)
app/components/integration_card_component.rb (2)
app/components/integration_list_component.rb (2)
initialize(1-15)initialize(2-6)app/models/application_record.rb (1)
human_enum_name(21-24)
app/controllers/ci_cd/github_configs_controller.rb (5)
app/controllers/ci_cd/bitbucket_configs_controller.rb (5)
using(1-55)edit(9-18)set_code_repositories(42-46)update(20-27)set_app(31-33)app/controllers/ci_cd/gitlab_configs_controller.rb (5)
using(1-52)edit(9-17)set_code_repositories(41-43)update(19-26)set_app(30-32)app/models/integration.rb (1)
ci_cd_provider(181-183)app/models/github_integration.rb (1)
repos(138-140)app/refinements/refined_string.rb (1)
safe_json_parse(42-46)
spec/factories/apps.rb (1)
app/models/app_variant.rb (1)
config(34-34)
app/components/live_release/finalize_component.rb (5)
app/models/bitbucket_integration.rb (2)
tag_url(214-216)code_repo_url(37-39)app/models/github_integration.rb (2)
tag_url(189-191)code_repo_url(117-119)app/models/gitlab_integration.rb (2)
tag_url(322-324)code_repo_url(163-165)app/models/release.rb (1)
tag_url(433-435)app/models/integration.rb (1)
vcs_provider(177-179)
app/controllers/build_channel/google_firebase_configs_controller.rb (4)
app/controllers/application_controller.rb (1)
current_organization(30-42)app/models/integration.rb (1)
firebase_build_channel_provider(209-211)app/models/google_firebase_integration.rb (1)
setup(106-117)app/refinements/refined_string.rb (1)
safe_json_parse(42-46)
app/models/linear_integration.rb (1)
app/models/jira_integration.rb (1)
release_filters_are_valid(268-274)
app/models/jira_integration.rb (1)
app/models/linear_integration.rb (1)
release_filters_are_valid(246-252)
app/controllers/ci_cd/bitrise_configs_controller.rb (4)
app/controllers/application_controller.rb (1)
current_organization(30-42)app/models/integration.rb (1)
ci_cd_provider(181-183)app/models/bitrise_integration.rb (1)
setup(93-95)app/refinements/refined_string.rb (1)
safe_json_parse(42-46)
app/controllers/version_control/bitbucket_configs_controller.rb (4)
app/controllers/version_control/github_configs_controller.rb (5)
using(1-52)edit(9-17)set_code_repositories(41-43)update(19-26)set_app(30-32)app/models/integration.rb (1)
vcs_provider(177-179)app/models/bitbucket_integration.rb (2)
workspaces(96-98)repos(165-167)app/refinements/refined_string.rb (1)
safe_json_parse(42-46)
app/controllers/version_control/github_configs_controller.rb (5)
app/controllers/version_control/bitbucket_configs_controller.rb (5)
using(1-55)edit(9-18)set_code_repositories(42-46)update(20-27)set_app(31-33)app/controllers/version_control/gitlab_configs_controller.rb (5)
using(1-52)edit(9-17)set_code_repositories(41-43)update(19-26)set_app(30-32)app/models/integration.rb (1)
vcs_provider(177-179)app/models/github_integration.rb (1)
repos(138-140)app/refinements/refined_string.rb (1)
safe_json_parse(42-46)
app/models/google_firebase_integration.rb (1)
app/models/concerns/app_configurable.rb (1)
firebase_app(6-13)
app/controllers/project_management/linear_configs_controller.rb (4)
app/controllers/project_management/jira_configs_controller.rb (4)
using(1-109)edit(9-17)update(19-26)set_app(30-32)app/models/integration.rb (1)
project_management_provider(213-215)app/models/linear_integration.rb (1)
setup(116-132)app/libs/installations/linear/api.rb (1)
workflow_states(95-103)
spec/libs/triggers/pull_request_spec.rb (5)
app/models/integration.rb (1)
vcs_provider(177-179)app/models/bitbucket_integration.rb (3)
code_repository_name(33-35)code_repo_namespace(41-41)code_repo_name_only(43-43)app/models/github_integration.rb (3)
code_repository_name(113-115)code_repo_namespace(121-123)code_repo_name_only(125-127)app/models/gitlab_integration.rb (3)
code_repository_name(159-161)code_repo_namespace(167-167)code_repo_name_only(169-169)app/libs/triggers/pull_request.rb (1)
repo_integration(129-131)
app/controllers/ci_cd/bitbucket_configs_controller.rb (5)
app/controllers/ci_cd/github_configs_controller.rb (5)
using(1-52)edit(9-17)set_code_repositories(41-43)update(19-26)set_app(30-32)app/controllers/ci_cd/gitlab_configs_controller.rb (5)
using(1-52)edit(9-17)set_code_repositories(41-43)update(19-26)set_app(30-32)app/models/integration.rb (1)
ci_cd_provider(181-183)app/models/bitbucket_integration.rb (2)
workspaces(96-98)repos(165-167)app/refinements/refined_string.rb (1)
safe_json_parse(42-46)
app/controllers/project_management/jira_configs_controller.rb (3)
app/controllers/project_management/linear_configs_controller.rb (4)
using(1-100)edit(9-17)update(19-26)set_app(30-32)app/models/integration.rb (1)
project_management_provider(213-215)app/models/jira_integration.rb (1)
setup(129-146)
app/controllers/ci_cd/gitlab_configs_controller.rb (7)
app/controllers/ci_cd/bitbucket_configs_controller.rb (5)
using(1-55)edit(9-18)set_code_repositories(42-46)update(20-27)set_app(31-33)app/controllers/ci_cd/github_configs_controller.rb (5)
using(1-52)edit(9-17)set_code_repositories(41-43)update(19-26)set_app(30-32)app/models/integration.rb (1)
ci_cd_provider(181-183)app/models/bitbucket_integration.rb (1)
repos(165-167)app/models/github_integration.rb (1)
repos(138-140)app/models/gitlab_integration.rb (1)
repos(198-200)app/refinements/refined_string.rb (1)
safe_json_parse(42-46)
app/models/integration.rb (4)
app/models/app_config.rb (5)
firebase_ready?(53-57)bugsnag_ready?(64-68)project_management_ready?(76-97)bitrise_ready?(59-62)configs_ready?(70-74)app/models/app.rb (2)
bitrise_connected?(109-111)bugsnag_connected?(113-115)app/models/bitrise_integration.rb (1)
bitrise_project(64-66)app/models/concerns/integrable.rb (1)
firebase_connected?(14-16)
app/models/gitlab_integration.rb (3)
app/models/bitbucket_integration.rb (4)
code_repository_name(33-35)code_repo_url(37-39)with_api_retries(369-385)installation(60-62)app/libs/installations/github/api.rb (1)
get_commit(287-289)app/libs/installations/gitlab/api.rb (1)
get_commit(111-115)
app/libs/coordinators/webhooks/pull_request.rb (6)
app/models/integration.rb (1)
vcs_provider(177-179)app/models/bitbucket_integration.rb (1)
code_repository_name(33-35)app/models/github_integration.rb (1)
code_repository_name(113-115)app/models/gitlab_integration.rb (1)
code_repository_name(159-161)app/libs/coordinators/webhooks/bitbucket/pull_request.rb (1)
repository_name(25-27)app/libs/coordinators/webhooks/bitbucket/push.rb (1)
repository_name(35-37)
app/models/bitbucket_integration.rb (2)
app/models/github_integration.rb (4)
code_repository_name(113-115)code_repo_url(117-119)code_repo_namespace(121-123)code_repo_name_only(125-127)app/models/gitlab_integration.rb (4)
code_repository_name(159-161)code_repo_url(163-165)code_repo_namespace(167-167)code_repo_name_only(169-169)
app/controllers/apps_controller.rb (2)
app/models/app.rb (3)
ready?(129-131)train_in_creation(137-139)app_setup_instructions(171-203)app/models/integration.rb (1)
ready?(160-162)
app/controllers/version_control/gitlab_configs_controller.rb (6)
app/controllers/version_control/bitbucket_configs_controller.rb (5)
using(1-55)edit(9-18)set_code_repositories(42-46)update(20-27)set_app(31-33)app/controllers/version_control/github_configs_controller.rb (5)
using(1-52)edit(9-17)set_code_repositories(41-43)update(19-26)set_app(30-32)app/controllers/application_controller.rb (1)
current_organization(30-42)app/models/integration.rb (1)
vcs_provider(177-179)app/models/gitlab_integration.rb (1)
repos(198-200)app/refinements/refined_string.rb (1)
safe_json_parse(42-46)
app/models/app_config.rb (3)
app/models/integration.rb (7)
monitoring_provider(189-191)firebase_ready?(307-313)firebase_build_channel_provider(209-211)configs_ready?(346-350)bitrise_ready?(296-301)ci_cd_provider(181-183)bugsnag_ready?(315-321)app/models/concerns/integrable.rb (1)
firebase_connected?(14-16)app/models/app.rb (2)
bitrise_connected?(109-111)bugsnag_connected?(113-115)
app/models/bitrise_integration.rb (1)
app/models/integration.rb (1)
bitrise_project(303-305)
app/models/bugsnag_integration.rb (1)
app/models/app_config.rb (1)
set_bugsnag_config(45-51)
🪛 ast-grep (0.39.5)
spec/models/gitlab_integration_spec.rb
[warning] 16-16: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(installation).to have_received(:create_release!).with(gitlab_integration.code_repository_name, "v1.0.0", "main", "Release notes")
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 16-16: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: have_received(:create_release!).with(gitlab_integration.code_repository_name, "v1.0.0", "main", "Release notes")
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 64-64: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(installation).to have_received(:update_file!).with(gitlab_integration.code_repository_name, "main", "path/to/file.txt", "new content", "commit message", author_name: "Test User", author_email: "test@example.com")
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 64-64: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: have_received(:update_file!).with(gitlab_integration.code_repository_name, "main", "path/to/file.txt", "new content", "commit message", author_name: "Test User", author_email: "test@example.com")
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 91-91: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(installation).to have_received(:run_pipeline_with_job!).with(gitlab_integration.code_repository_name, "main", {key: "value"}, "ci_cd_channel", "abc123", GitlabIntegration::WORKFLOW_RUN_TRANSFORMATIONS)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 91-91: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: have_received(:run_pipeline_with_job!).with(gitlab_integration.code_repository_name, "main", {key: "value"}, "ci_cd_channel", "abc123", GitlabIntegration::WORKFLOW_RUN_TRANSFORMATIONS)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 115-115: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(installation).to have_received(:retry_job!).with(gitlab_integration.code_repository_name, "123", GitlabIntegration::JOB_RUN_TRANSFORMATIONS)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 115-115: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: have_received(:retry_job!).with(gitlab_integration.code_repository_name, "123", GitlabIntegration::JOB_RUN_TRANSFORMATIONS)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 123-123: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(installation).to have_received(:get_job).with(gitlab_integration.code_repository_name, "123")
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 123-123: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: have_received(:get_job).with(gitlab_integration.code_repository_name, "123")
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 131-131: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(installation).to have_received(:create_tag!).with(gitlab_integration.code_repository_name, "v1.0.0", "abcdef")
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 131-131: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: have_received(:create_tag!).with(gitlab_integration.code_repository_name, "v1.0.0", "abcdef")
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 161-161: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(installation).to have_received(:cherry_pick_pr).with(gitlab_integration.code_repository_name, "main", "abcdef", "patch-branch", "PR Title", "Custom description", GitlabIntegration::PR_TRANSFORMATIONS)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 161-161: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: have_received(:cherry_pick_pr).with(gitlab_integration.code_repository_name, "main", "abcdef", "patch-branch", "PR Title", "Custom description", GitlabIntegration::PR_TRANSFORMATIONS)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🪛 GitHub Actions: Lint
db/schema.rb
[warning] 1-1: Schema linting warnings: db/schema.rb changed in commits without an update in lib/tasks/anonymize.rake afterward.
[warning] 1-1: Schema linting warnings: db/schema.rb changed in commits without an update in lib/tasks/anonymize.rake afterward.
[warning] 1-1: Schema linting warnings: db/schema.rb changed in commits without an update in lib/tasks/anonymize.rake afterward.
[warning] 1-1: Schema linting warnings: db/schema.rb changed in commits without an update in lib/tasks/anonymize.rake afterward.
[warning] 1-1: Schema linting warnings: db/schema.rb changed in commits without an update in lib/tasks/anonymize.rake afterward.
| def initialize(app, integration, category, pre_open_category = nil) | ||
| @app = app | ||
| @integration = integration | ||
| @category = category | ||
| @pre_open_category = pre_open_category | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Use a keyword for pre_open_category to avoid positional arg mistakes.
Switch to a keyword to align with IntegrationListComponent and improve readability:
-def initialize(app, integration, category, pre_open_category = nil)
+def initialize(app, integration, category, pre_open_category: nil)
@app = app
@integration = integration
@category = category
- @pre_open_category = pre_open_category
+ @pre_open_category = pre_open_category
endUpdate callers accordingly.
Run to find/adjust all call sites:
🏁 Script executed:
#!/bin/bash
rg -nP --type=ruby 'IntegrationCardComponent\.new\(' -C2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search all files for direct instantiation
rg -nP -C2 'IntegrationCardComponent\.new\('
# Search for render calls in views
rg -nP -C2 'render\s+IntegrationCardComponent'Length of output: 993
Convert initializer to keyword argument and update its callers
-def initialize(app, integration, category, pre_open_category = nil)
+def initialize(app, integration, category, pre_open_category: nil)In app/components/integration_list_component.html.erb (line 8):
-<%= render IntegrationCardComponent.new(@app, integration, category, @pre_open_category) %>
+<%= render IntegrationCardComponent.new(@app, integration, category, pre_open_category: @pre_open_category) %>🤖 Prompt for AI Agents
In app/components/integration_card_component.rb around lines 13 to 18, the
initializer currently uses positional arguments; change it to keyword arguments
(e.g., app:, integration:, category:, pre_open_category: nil) and update every
caller to pass keyword names (notably
app/components/integration_list_component.html.erb at line 8) so parameters map
correctly; run the test suite and search/replace other instantiations to ensure
no positional calls remain and update any docs or specs accordingly.
| def parsed_google_firebase_config_params | ||
| google_firebase_config_params = params.require(:google_firebase_integration) | ||
| .permit(:android_config, :ios_config) | ||
| google_firebase_config_params.merge( | ||
| android_config: google_firebase_config_params[:android_config]&.safe_json_parse, | ||
| ios_config: google_firebase_config_params[:ios_config]&.safe_json_parse | ||
| ) | ||
| end |
There was a problem hiding this comment.
Prevent unintended config wipe when params are omitted
Merging unconditionally injects nil and can clear jsonb columns if a field isn’t submitted. Parse only when the key is present.
Apply this diff:
def parsed_google_firebase_config_params
- google_firebase_config_params = params.require(:google_firebase_integration)
- .permit(:android_config, :ios_config)
- google_firebase_config_params.merge(
- android_config: google_firebase_config_params[:android_config]&.safe_json_parse,
- ios_config: google_firebase_config_params[:ios_config]&.safe_json_parse
- )
+ p = params.require(:google_firebase_integration)
+ .permit(:android_config, :ios_config)
+ p[:android_config] = p[:android_config]&.safe_json_parse if p.key?(:android_config)
+ p[:ios_config] = p[:ios_config]&.safe_json_parse if p.key?(:ios_config)
+ p
end🤖 Prompt for AI Agents
In app/controllers/build_channel/google_firebase_configs_controller.rb around
lines 46 to 53, the current merge unconditionally sets android_config and
ios_config to nil when keys are omitted, which can wipe jsonb columns; change
the method to only parse and include a key when it is present in params (e.g.
build a result hash from google_firebase_config_params and conditionally assign
result[:android_config] = parsed value only if
google_firebase_config_params.key?(:android_config), similarly for :ios_config),
then return that result so absent keys are not merged and stored as nil.
| def parsed_bitbucket_config_params | ||
| bitbucket_config_params = params.require(:bitbucket_integration) | ||
| .permit(:repository_config, :workspace) | ||
| bitbucket_config_params.merge( | ||
| repository_config: bitbucket_config_params[:repository_config]&.safe_json_parse | ||
| ) | ||
| end |
There was a problem hiding this comment.
Guard against clearing repository_config on partial updates
Avoid merging a nil repository_config when the field isn’t submitted.
Apply this diff:
def parsed_bitbucket_config_params
- bitbucket_config_params = params.require(:bitbucket_integration)
- .permit(:repository_config, :workspace)
- bitbucket_config_params.merge(
- repository_config: bitbucket_config_params[:repository_config]&.safe_json_parse
- )
+ p = params.require(:bitbucket_integration).permit(:repository_config, :workspace)
+ if p.key?(:repository_config)
+ p[:repository_config] = p[:repository_config]&.safe_json_parse
+ end
+ p
end📝 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.
| def parsed_bitbucket_config_params | |
| bitbucket_config_params = params.require(:bitbucket_integration) | |
| .permit(:repository_config, :workspace) | |
| bitbucket_config_params.merge( | |
| repository_config: bitbucket_config_params[:repository_config]&.safe_json_parse | |
| ) | |
| end | |
| def parsed_bitbucket_config_params | |
| p = params.require(:bitbucket_integration).permit(:repository_config, :workspace) | |
| if p.key?(:repository_config) | |
| p[:repository_config] = p[:repository_config]&.safe_json_parse | |
| end | |
| p | |
| end |
🤖 Prompt for AI Agents
In app/controllers/ci_cd/bitbucket_configs_controller.rb around lines 48 to 54,
the current merge unconditionally sets repository_config to nil when the param
is omitted; change it to only merge repository_config when the incoming params
include that key and it is not nil/blank. Specifically, check
params[:bitbucket_integration].key?(:repository_config) (or use presence) before
calling safe_json_parse and merging, leaving repository_config untouched for
partial updates when the key is absent.
| def parsed_github_config_params | ||
| github_config_params = params.require(:github_integration) | ||
| .permit(:repository_config) | ||
| github_config_params.merge( | ||
| repository_config: github_config_params[:repository_config]&.safe_json_parse | ||
| ) | ||
| end |
There was a problem hiding this comment.
Do not clear repository_config when the param is absent
Unconditional merge sets repository_config to nil if the field isn’t submitted. Guard by key presence.
Apply this diff:
def parsed_github_config_params
- github_config_params = params.require(:github_integration)
- .permit(:repository_config)
- github_config_params.merge(
- repository_config: github_config_params[:repository_config]&.safe_json_parse
- )
+ p = params.require(:github_integration).permit(:repository_config)
+ if p.key?(:repository_config)
+ p[:repository_config] = p[:repository_config]&.safe_json_parse
+ end
+ p
end🤖 Prompt for AI Agents
In app/controllers/ci_cd/github_configs_controller.rb around lines 45 to 51, the
current parsed_github_config_params unconditionally merges repository_config
which sets it to nil when the param is absent; change the logic to only merge
repository_config when params[:github_integration] includes the
repository_config key (or github_config_params.key?(:repository_config)), and
otherwise return github_config_params unchanged so existing repository_config is
not cleared.
| def ready? | ||
| integrations.ready? and config&.ready? | ||
| integrations.ready? && integrations.configured? | ||
| end |
There was a problem hiding this comment.
❓ Verification inconclusive
Verify the readiness logic change is tested and intentional.
The change from a single config.ready? check to integrations.ready? && integrations.configured? introduces a stricter requirement: both the base integration categories (MINIMUM_REQUIRED_SET) and their configurations must be complete. This represents a potential behavior change from the previous implementation.
Verify that test coverage exists for this updated readiness logic:
🏁 Script executed:
#!/bin/bash
# Search for tests covering app.ready? with different integration states
rg -n -B5 -A10 'app\.ready\?' spec/models/app_spec.rb spec/requests/ spec/system/ --type=rbLength of output: 26
🏁 Script executed:
#!/bin/bash
# Search for tests covering App#ready? with different integration states
rg -n -B5 -A10 'ready\?' -g '*.rb' spec/Length of output: 0
Add tests for App#ready? covering new readiness logic
No specs currently exercise App#ready?. Introduce tests in spec/models/app_spec.rb for:
integrations.ready?false →ready?falseintegrations.ready?true &integrations.configured?false → false- both true → true
🤖 Prompt for AI Agents
In app/models/app.rb around lines 129–131, there are no specs covering the new
App#ready? logic; add tests in spec/models/app_spec.rb that create an App (or
build a minimal instance), stub or mock the app.integrations to return the three
combinations described, and assert ready? returns false when integrations.ready?
is false, false when integrations.ready? is true but integrations.configured? is
false, and true when both are true; ensure each example isolates the
integrations double and sets expectations on the boolean return values.
| def project(platform) | ||
| case platform | ||
| when "android" then android_config&.dig("project_id") | ||
| when "ios" then ios_config&.dig("project_id") | ||
| else | ||
| raise ArgumentError, "Invalid platform: #{platform}" | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix platform project getters: current shape mismatch will raise at runtime
project(platform) returns a String ID, but project_url/project_id expect a Hash and call fetch(), causing NoMethodError and nil IDs.
Proposed fix: read directly from android_config/ios_config and avoid fetch() on non-Hash.
- def project(platform)
- case platform
- when "android" then android_config&.dig("project_id")
- when "ios" then ios_config&.dig("project_id")
- else
- raise ArgumentError, "Invalid platform: #{platform}"
- end
- end
+ def project(platform)
+ # Kept for compatibility but returns the project_id String
+ project_id(platform)
+ end
- def project_url(platform)
- project(platform)&.fetch("url", nil)
- end
+ def project_url(platform)
+ case platform
+ when "android" then android_config&.dig("project_url")
+ when "ios" then ios_config&.dig("project_url")
+ else
+ raise ArgumentError, "Invalid platform: #{platform}"
+ end
+ end
- def project_id(platform)
- project(platform)&.fetch("id", nil)
- end
+ def project_id(platform)
+ case platform
+ when "android" then android_config&.dig("project_id")
+ when "ios" then ios_config&.dig("project_id")
+ else
+ raise ArgumentError, "Invalid platform: #{platform}"
+ end
+ endOptional: normalize platform with platform = platform.to_s to accept symbols.
Also applies to: 143-149
🤖 Prompt for AI Agents
In app/models/bugsnag_integration.rb around lines 116–124, project(platform)
currently returns a String ID while downstream methods expect a Hash and call
fetch(), causing NoMethodError; change project(platform) to return the config
Hash (e.g., android_config or ios_config) instead of digging the project_id so
callers can safely call fetch("project_id"), and normalize platform with
platform = platform.to_s to accept symbols; apply the same change to the similar
logic at lines 143–149 so both code paths return the config Hash rather than a
raw String ID.
| def get_commit(sha) | ||
| with_api_retries { installation.get_commit(app_config.code_repository["id"], sha, COMMITS_TRANSFORMATIONS) } | ||
| with_api_retries { installation.get_commit(repository_config["id"], sha, COMMITS_TRANSFORMATIONS) } | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Guard against missing repository_config in get_commit
Calling ["id"] on nil will raise; add a guard for clearer behavior.
- def get_commit(sha)
- with_api_retries { installation.get_commit(repository_config["id"], sha, COMMITS_TRANSFORMATIONS) }
- end
+ def get_commit(sha)
+ repo_id = repository_config&.fetch("id", nil)
+ return nil if repo_id.blank?
+ with_api_retries { installation.get_commit(repo_id, sha, COMMITS_TRANSFORMATIONS) }
+ endAlternatively, raise a domain error if you prefer explicit failure.
🏁 Script executed:
#!/bin/bash
set -e
# Locate the repository_config accessor or method definition
rg -n "def repository_config" -C2 --glob "app/models/gitlab_integration.rb"
# Find where repository_config is assigned or fetched
rg -n "repository_config" -C3 --glob "app/models/gitlab_integration.rb"
# Inspect get_commit context
sed -n '350,380p' app/models/gitlab_integration.rbLength of output: 0
🏁 Script executed:
#!/bin/bash
set -e
# Find the path of the file
find . -type f -name "gitlab_integration.rb"
# Inspect the file around the get_commit definition if found
sed -n '350,380p' app/models/gitlab_integration.rb 2>/dev/null || true
# Search globally for get_commit definitions
rg -n "def get_commit" -C2
# Search globally for repository_config usages
rg -n "repository_config" -C2Length of output: 19492
Guard against missing repository_config["id"] in get_commit
Replace direct indexing with a safe fetch and early return:
def get_commit(sha)
- with_api_retries { installation.get_commit(repository_config["id"], sha, COMMITS_TRANSFORMATIONS) }
+ repo_id = repository_config&.fetch("id", nil)
+ return nil if repo_id.blank?
+ with_api_retries { installation.get_commit(repo_id, sha, COMMITS_TRANSFORMATIONS) }
end🤖 Prompt for AI Agents
In app/models/gitlab_integration.rb around lines 361 to 363, get_commit
currently directly indexes repository_config["id"] which can be nil and raise;
change to safely fetch the repository id (e.g., repo_id =
repository_config&.fetch("id", nil) or repository_config["id"] rescue nil) and
return nil or appropriate early value if repo_id is blank before calling
with_api_retries; this prevents NoMethodError/KeyError and ensures get_commit
exits early when the repository id is missing.
| INVALID_PLATFORM_ERROR = "platform must be valid" | ||
|
|
||
| def firebase_app(platform) | ||
| case platform | ||
| when "android" then android_config["app_id"] | ||
| when "ios" then ios_config["app_id"] | ||
| else | ||
| raise ArgumentError, INVALID_PLATFORM_ERROR | ||
| end | ||
| end |
There was a problem hiding this comment.
Guard against nil configs in firebase_app (avoid NoMethodError)
android_config/ios_config can be nil (allow_blank: true). Indexing ["app_id"] will raise. Normalize platform, nil-safe access, and raise a clear error when app_id missing.
- def firebase_app(platform)
- case platform
- when "android" then android_config["app_id"]
- when "ios" then ios_config["app_id"]
- else
- raise ArgumentError, INVALID_PLATFORM_ERROR
- end
- end
+ def firebase_app(platform)
+ platform = platform.to_s
+ app_id =
+ case platform
+ when "android" then android_config&.fetch("app_id", nil)
+ when "ios" then ios_config&.fetch("app_id", nil)
+ else
+ raise ArgumentError, INVALID_PLATFORM_ERROR
+ end
+ raise ArgumentError, "app_id missing for #{platform}" if app_id.blank?
+ app_id
+ end📝 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.
| INVALID_PLATFORM_ERROR = "platform must be valid" | |
| def firebase_app(platform) | |
| case platform | |
| when "android" then android_config["app_id"] | |
| when "ios" then ios_config["app_id"] | |
| else | |
| raise ArgumentError, INVALID_PLATFORM_ERROR | |
| end | |
| end | |
| INVALID_PLATFORM_ERROR = "platform must be valid" | |
| def firebase_app(platform) | |
| platform = platform.to_s | |
| app_id = | |
| case platform | |
| when "android" then android_config&.fetch("app_id", nil) | |
| when "ios" then ios_config&.fetch("app_id", nil) | |
| else | |
| raise ArgumentError, INVALID_PLATFORM_ERROR | |
| end | |
| raise ArgumentError, "app_id missing for #{platform}" if app_id.blank? | |
| app_id | |
| end |
🤖 Prompt for AI Agents
In app/models/google_firebase_integration.rb around lines 59 to 68, firebase_app
currently indexes android_config/ios_config directly which can be nil; normalize
the incoming platform (e.g., to_s.downcase), select the correct config with safe
navigation (or fetch via dig) to avoid NoMethodError, retrieve the "app_id" in a
nil-safe way, and raise ArgumentError with a clear message (e.g., missing app_id
for platform) if the config or app_id is absent.
| <% f.with_action do %> | ||
| <%= f.F.authz_submit "Update", "plus.svg", size: :xs %> | ||
| <% end %> | ||
| <% end %> |
There was a problem hiding this comment.
Hide submit when not connected.
Submit renders even if Bugsnag isn’t connected (no fields shown). Gate it behind the same connectivity check.
- <% f.with_action do %>
- <%= f.F.authz_submit "Update", "plus.svg", size: :xs %>
- <% end %>
+ <% if @app.bugsnag_connected? %>
+ <% f.with_action do %>
+ <%= f.F.authz_submit "Update", "plus.svg", size: :xs %>
+ <% end %>
+ <% end %>📝 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.
| <% f.with_action do %> | |
| <%= f.F.authz_submit "Update", "plus.svg", size: :xs %> | |
| <% end %> | |
| <% end %> | |
| <% if @app.bugsnag_connected? %> | |
| <% f.with_action do %> | |
| <%= f.F.authz_submit "Update", "plus.svg", size: :xs %> | |
| <% end %> | |
| <% end %> | |
| <% end %> |
🤖 Prompt for AI Agents
In app/views/monitoring/bugsnag_configs/edit.html+turbo_frame.erb around lines
28 to 31, the "Update" submit button is rendered even when Bugsnag is not
connected; wrap the f.with_action block that renders f.F.authz_submit in the
same conditional used earlier to display the integration fields (i.e., only
render the submit when the connectivity check is true), so the submit button is
hidden when there is no Bugsnag connection.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (7)
app/components/integration_card_component.html.erb (1)
25-27: Add an accessible label to the icon-only config trigger.This button renders only an icon, so assistive tech users still get no name. Please pass visible text or an
aria-labelviahtml_options. Example fix:- <% button = modal.with_button(scheme: :light, type: :action, size: :xxs) %> + <% button = modal.with_button( + scheme: :light, + type: :action, + size: :xxs, + html_options: { aria: { label: category_title } } + ) %>app/controllers/ci_cd/bitbucket_configs_controller.rb (1)
48-54: Guard against clearing repository_config on partial updates.The previous review comment correctly identified that this merge will unconditionally set
repository_configtonilwhen the param is not submitted, which can clear existing configuration on partial updates.Apply this diff to only merge
repository_configwhen explicitly provided:def parsed_bitbucket_config_params - bitbucket_config_params = params.require(:bitbucket_integration) - .permit(:repository_config, :workspace) - bitbucket_config_params.merge( - repository_config: bitbucket_config_params[:repository_config]&.safe_json_parse - ) + p = params.require(:bitbucket_integration).permit(:repository_config, :workspace) + if p.key?(:repository_config) + p[:repository_config] = p[:repository_config]&.safe_json_parse + end + p endapp/controllers/version_control/gitlab_configs_controller.rb (1)
34-39: Type verification implemented.The type check using
.is_a?(GitlabIntegration)correctly addresses the concern raised in the previous review about verifying the VCS provider type. The nil case is also handled properly sincenil.is_a?(GitlabIntegration)returnsfalse.app/controllers/version_control/bitbucket_configs_controller.rb (1)
42-46: This issue was previously flagged and remains unaddressed.The past review identified that calling
repos(nil)can error when no workspaces exist, and the code does not prefer the persisted workspace to maintain user context. The suggested fix from the previous review should be applied:def set_code_repositories @workspaces = @bitbucket_integration.workspaces || [] - workspace = params[:workspace] || @workspaces.first - @code_repositories = @bitbucket_integration.repos(workspace) + workspace = params[:workspace] || @bitbucket_integration.workspace || @workspaces.first + @code_repositories = workspace.present? ? @bitbucket_integration.repos(workspace) : [] endapp/controllers/build_channel/google_firebase_configs_controller.rb (1)
46-53: Prevent unintended config wipe when params are omitted.This issue was previously flagged: merging unconditionally injects
nilfor keys not present in params, which clears JSONB columns if a field isn't submitted (e.g., editing onlyandroid_configwould wipeios_config). Parse only when the key is present.Apply this diff:
def parsed_google_firebase_config_params - google_firebase_config_params = params.require(:google_firebase_integration) - .permit(:android_config, :ios_config) - google_firebase_config_params.merge( - android_config: google_firebase_config_params[:android_config]&.safe_json_parse, - ios_config: google_firebase_config_params[:ios_config]&.safe_json_parse - ) + p = params.require(:google_firebase_integration) + .permit(:android_config, :ios_config) + p[:android_config] = p[:android_config]&.safe_json_parse if p.key?(:android_config) + p[:ios_config] = p[:ios_config]&.safe_json_parse if p.key?(:ios_config) + p endapp/models/app_config.rb (1)
48-52: Nil-guard monitoring_provider to avoid NPE.This issue was already flagged in a previous review. The
monitoring_providercan be nil if the app has no monitoring integration, and calling methods on nil will raise NoMethodError.app/controllers/monitoring/bugsnag_configs_controller.rb (1)
45-54: Avoid mass-assigning unknown attributes; return only ios_config/android_config.This issue was previously flagged: merging scalar params (
:ios_project_id,:ios_release_stage, etc.) into the update hash can raiseUnknownAttributeErrorif the model doesn't have those attributes. The model expects only the nested:ios_configand:android_configkeys.Apply this diff to return only the nested config:
def parsed_bugsnag_config_params - bugsnag_config_params = params.require(:bugsnag_integration) - .permit( - :ios_project_id, - :ios_release_stage, - :android_project_id, - :android_release_stage - ) - bugsnag_config_params.merge(bugsnag_config(bugsnag_config_params)) + p = params.require(:bugsnag_integration).permit( + :ios_project_id, :ios_release_stage, :android_project_id, :android_release_stage + ) + bugsnag_config(p) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/components/integration_card_component.html.erb(2 hunks)app/controllers/build_channel/google_firebase_configs_controller.rb(1 hunks)app/controllers/ci_cd/bitbucket_configs_controller.rb(1 hunks)app/controllers/ci_cd/bitrise_configs_controller.rb(1 hunks)app/controllers/ci_cd/github_configs_controller.rb(1 hunks)app/controllers/ci_cd/gitlab_configs_controller.rb(1 hunks)app/controllers/monitoring/bugsnag_configs_controller.rb(1 hunks)app/controllers/project_management/jira_configs_controller.rb(1 hunks)app/controllers/version_control/bitbucket_configs_controller.rb(1 hunks)app/controllers/version_control/github_configs_controller.rb(1 hunks)app/controllers/version_control/gitlab_configs_controller.rb(1 hunks)app/models/app_config.rb(3 hunks)app/models/concerns/app_configurable.rb(1 hunks)app/models/jira_integration.rb(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/models/concerns/app_configurable.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- app/controllers/version_control/github_configs_controller.rb
- app/controllers/project_management/jira_configs_controller.rb
- app/controllers/ci_cd/gitlab_configs_controller.rb
- app/controllers/ci_cd/github_configs_controller.rb
🧰 Additional context used
🧬 Code graph analysis (8)
app/controllers/version_control/bitbucket_configs_controller.rb (5)
app/controllers/ci_cd/bitbucket_configs_controller.rb (7)
using(1-55)edit(9-18)set_code_repositories(42-46)update(20-27)parsed_bitbucket_config_params(48-54)set_app(31-33)set_bitbucket_integration(35-40)app/controllers/version_control/github_configs_controller.rb (5)
using(1-52)edit(9-17)set_code_repositories(41-43)update(19-26)set_app(30-32)app/models/integration.rb (1)
vcs_provider(177-179)app/models/bitbucket_integration.rb (2)
workspaces(96-98)repos(165-167)app/refinements/refined_string.rb (1)
safe_json_parse(42-46)
app/controllers/version_control/gitlab_configs_controller.rb (5)
app/controllers/version_control/bitbucket_configs_controller.rb (5)
using(1-55)edit(9-18)set_code_repositories(42-46)update(20-27)set_app(31-33)app/controllers/version_control/github_configs_controller.rb (5)
using(1-52)edit(9-17)set_code_repositories(41-43)update(19-26)set_app(30-32)app/models/integration.rb (1)
vcs_provider(177-179)app/models/gitlab_integration.rb (1)
repos(198-200)app/refinements/refined_string.rb (1)
safe_json_parse(42-46)
app/models/jira_integration.rb (1)
app/models/linear_integration.rb (1)
release_filters_are_valid(246-252)
app/controllers/monitoring/bugsnag_configs_controller.rb (4)
app/controllers/build_channel/google_firebase_configs_controller.rb (4)
using(1-54)edit(9-17)update(19-26)set_app(30-32)app/models/integration.rb (1)
monitoring_provider(189-191)app/models/bugsnag_integration.rb (1)
setup(83-85)app/refinements/refined_string.rb (1)
safe_json_parse(42-46)
app/models/app_config.rb (4)
app/models/bugsnag_integration.rb (1)
set_bugsnag_config(136-141)app/models/integration.rb (7)
monitoring_provider(189-191)firebase_ready?(307-313)firebase_build_channel_provider(209-211)configs_ready?(346-350)bitrise_ready?(296-301)ci_cd_provider(181-183)bugsnag_ready?(315-321)app/models/concerns/integrable.rb (1)
firebase_connected?(14-16)app/models/app.rb (2)
bitrise_connected?(109-111)bugsnag_connected?(113-115)
app/controllers/ci_cd/bitrise_configs_controller.rb (5)
app/controllers/ci_cd/bitbucket_configs_controller.rb (4)
using(1-55)edit(9-18)update(20-27)set_app(31-33)app/controllers/ci_cd/github_configs_controller.rb (4)
using(1-52)edit(9-17)update(19-26)set_app(30-32)app/models/integration.rb (1)
ci_cd_provider(181-183)app/models/bitrise_integration.rb (1)
setup(93-95)app/refinements/refined_string.rb (1)
safe_json_parse(42-46)
app/controllers/build_channel/google_firebase_configs_controller.rb (4)
app/controllers/application_controller.rb (1)
current_organization(30-42)app/models/integration.rb (1)
firebase_build_channel_provider(209-211)app/models/google_firebase_integration.rb (1)
setup(106-117)app/refinements/refined_string.rb (1)
safe_json_parse(42-46)
app/controllers/ci_cd/bitbucket_configs_controller.rb (5)
app/controllers/version_control/bitbucket_configs_controller.rb (7)
using(1-55)edit(9-18)set_code_repositories(42-46)update(20-27)parsed_bitbucket_config_params(48-54)set_app(31-33)set_bitbucket_integration(35-40)app/controllers/application_controller.rb (1)
current_organization(30-42)app/models/integration.rb (1)
ci_cd_provider(181-183)app/models/bitbucket_integration.rb (2)
workspaces(96-98)repos(165-167)app/refinements/refined_string.rb (1)
safe_json_parse(42-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (18)
app/controllers/version_control/gitlab_configs_controller.rb (2)
1-26: LGTM!The controller structure, actions, and filters are implemented correctly and consistently with the GitHub and Bitbucket config controllers. The edit and update flows properly handle repository configuration with Turbo Frame support, parameter parsing, and error handling.
30-32: LGTM!The helper methods are correctly implemented:
set_appuses the standard friendly finder patternset_code_repositoriescalls the integration's repos method (which includes retry logic)parsed_gitlab_config_paramssafely parses JSON with proper nil-handling via the safe navigation operator andsafe_json_parseAlso applies to: 41-51
app/controllers/version_control/bitbucket_configs_controller.rb (5)
1-7: LGTM: Controller setup follows Rails conventions.The class setup with authentication guards, refinements, and time zone handling is well-structured.
9-18: LGTM: Edit action properly implements Turbo Frame pattern.The action correctly handles both turbo_frame and turbo_stream formats for editing Bitbucket configuration. Note that this action depends on
set_code_repositories, which has unresolved issues flagged in previous reviews.
20-27: LGTM: Update action handles success and failure appropriately.The update flow with proper redirects and user-friendly error messages follows Rails best practices.
31-40: LGTM: Helper methods properly scope and validate the integration.The private methods correctly load the app scoped to the current organization and validate that the integration is a BitbucketIntegration with appropriate error handling.
48-54: LGTM: Parameter parsing safely handles JSON and nil cases.The strong parameters with
safe_json_parseand safe navigation operator properly handle edge cases, returning an empty hash for invalid JSON.app/controllers/build_channel/google_firebase_configs_controller.rb (5)
1-7: LGTM!The controller setup follows the established pattern seen in other config controllers (CiCd, VersionControl). Access control, integration loading, and timezone handling are correctly configured.
9-17: LGTM!The edit action follows the standard Turbo Frame pattern used across other config controllers in this PR.
19-26: LGTM!The update action correctly handles success and failure cases with appropriate redirects and flash messages.
30-32: LGTM!Standard app loading pattern consistent with other controllers.
41-44: LGTM!The Firebase apps setup extraction is straightforward. Assuming the redirect fix in
set_google_firebase_integration, this will execute safely.app/models/jira_integration.rb (3)
100-101: Fix looks correct.The change to use
resources.firstinstead ofresource(singular) correctly fixes what appears to be an undefined variable bug. Since this code is in the branch wherecloud_idis NOT present andresources.length == 1, usingresources.firstis appropriate.Note: The AI summary incorrectly states this fix is "when cloud_id is already present." The fix is actually for the case when
cloud_idis blank and there's exactly one available resource (lines 98-102), not for the earlier branch at lines 90-96 wherecloud_idis already present.
161-164: Data source migration looks correct.The changes successfully migrate from the global
app.config.jira_configto the per-integrationproject_configfield. The early return guard (line 161) and safe navigation operators are properly placed.
10-10: Migration backfills existing project_config TheAddConfigToProjectManagementIntegrationsmigration setsdefault: {}andnull: false, so existing records are initialized correctly.app/controllers/monitoring/bugsnag_configs_controller.rb (3)
1-7: LGTM! Standard Rails controller setup.The controller follows established patterns with appropriate access control filters and time zone handling.
9-26: LGTM! Standard CRUD actions.Both
editandupdateactions follow Rails conventions with proper Turbo Frame support and error handling.
30-43: LGTM! Well-structured helper methods.The private helper methods properly load and validate the required objects with appropriate error handling.
| def set_google_firebase_integration | ||
| @google_firebase_integration = @app.integrations.firebase_build_channel_provider | ||
| unless @google_firebase_integration.is_a?(GoogleFirebaseIntegration) | ||
| redirect_to app_integrations_path(@app), flash: {error: "Firebase build channel integration not found."} | ||
| end | ||
| end |
There was a problem hiding this comment.
Add explicit return after redirect to prevent continued execution.
Rails redirects do not halt execution. If firebase_build_channel_provider returns nil or an unexpected type, the redirect executes but the before_action chain continues, leading to a NoMethodError when set_firebase_apps calls .setup on nil (line 42).
Apply this diff:
def set_google_firebase_integration
@google_firebase_integration = @app.integrations.firebase_build_channel_provider
unless @google_firebase_integration.is_a?(GoogleFirebaseIntegration)
- redirect_to app_integrations_path(@app), flash: {error: "Firebase build channel integration not found."}
+ redirect_to app_integrations_path(@app), flash: {error: "Firebase build channel integration not found."} and return
end
end📝 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.
| def set_google_firebase_integration | |
| @google_firebase_integration = @app.integrations.firebase_build_channel_provider | |
| unless @google_firebase_integration.is_a?(GoogleFirebaseIntegration) | |
| redirect_to app_integrations_path(@app), flash: {error: "Firebase build channel integration not found."} | |
| end | |
| end | |
| def set_google_firebase_integration | |
| @google_firebase_integration = @app.integrations.firebase_build_channel_provider | |
| unless @google_firebase_integration.is_a?(GoogleFirebaseIntegration) | |
| redirect_to app_integrations_path(@app), flash: {error: "Firebase build channel integration not found."} and return | |
| end | |
| end |
🤖 Prompt for AI Agents
In app/controllers/build_channel/google_firebase_configs_controller.rb around
lines 34 to 39, the before_action uses redirect_to when the integration is
missing but does not halt execution, allowing subsequent callbacks to run and
cause NoMethodError; after calling redirect_to with the error flash, immediately
return (e.g., use `redirect_to ... and return` or `redirect_to(...); return`) so
execution stops and downstream methods like set_firebase_apps do not run on a
nil integration.
| def set_bitrise_integration | ||
| @bitrise_integration = @app.ci_cd_provider | ||
| unless @bitrise_integration.is_a?(BitriseIntegration) | ||
| redirect_to app_integrations_path(@app), flash: {error: "CI/CD integration not found."} | ||
| end | ||
| end |
There was a problem hiding this comment.
Type verification implemented, consider improving error message.
The is_a?(BitriseIntegration) check now properly verifies the provider type, addressing the previous review concern. However, the error message "CI/CD integration not found." is generic and doesn't clearly indicate that a Bitrise-specific integration is required.
While this message is consistent with similar controllers (BitbucketConfigsController, GithubConfigsController), consider making it more specific to improve the user experience:
def set_bitrise_integration
@bitrise_integration = @app.ci_cd_provider
unless @bitrise_integration.is_a?(BitriseIntegration)
- redirect_to app_integrations_path(@app), flash: {error: "CI/CD integration not found."}
+ redirect_to app_integrations_path(@app), flash: {error: "Bitrise integration not found or incorrect CI/CD provider configured."}
end
end📝 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.
| def set_bitrise_integration | |
| @bitrise_integration = @app.ci_cd_provider | |
| unless @bitrise_integration.is_a?(BitriseIntegration) | |
| redirect_to app_integrations_path(@app), flash: {error: "CI/CD integration not found."} | |
| end | |
| end | |
| def set_bitrise_integration | |
| @bitrise_integration = @app.ci_cd_provider | |
| unless @bitrise_integration.is_a?(BitriseIntegration) | |
| redirect_to app_integrations_path(@app), flash: {error: "Bitrise integration not found or incorrect CI/CD provider configured."} | |
| end | |
| end |
🤖 Prompt for AI Agents
In app/controllers/ci_cd/bitrise_configs_controller.rb around lines 34 to 39,
the type check for BitriseIntegration is fine but the flash error is too
generic; change the flash message to explicitly mention Bitrise (e.g., "Bitrise
CI/CD integration not found." or the appropriate i18n key) so the user knows a
Bitrise-specific integration is required, keeping the redirect and behavior
unchanged and matching the style used in other controllers.
| def set_ci_cd_projects | ||
| @ci_cd_apps = @bitrise_integration.setup | ||
| @project_config = @bitrise_integration.project_config | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
Verify error handling for setup call.
The setup method is called without error handling. If setup or the underlying list_apps method fails or returns nil, @ci_cd_apps could be unset or invalid, potentially causing issues in the view.
🏁 Script executed:
#!/bin/bash
# Verify if BitriseIntegration#setup can fail or return nil
# and check how @ci_cd_apps is used in the view
# Find the setup/list_apps implementation
ast-grep --pattern 'def setup
$$$
end'
# Find the view that uses @ci_cd_apps
fd -e erb bitrise_configs | xargs rg -n '@ci_cd_apps' -A 3 -B 1Length of output: 6108
Inspect BitriseIntegration#list_apps implementation for error handling and return value:
🏁 Script executed:
#!/bin/bash
rg -n 'def list_apps' -n app/models/bitrise_integration.rb -A10 -B5Length of output: 482
Additional context from BitriseIntegration#list_apps:
🏁 Script executed:
#!/bin/bash
rg -n 'class BitriseIntegration' -A5 -B0 app/models/bitrise_integration.rb
rg -n 'def installation' -A5 -B5 app/models/bitrise_integration.rbLength of output: 362
Add error handling around BitriseIntegration#setup. The call to @bitrise_integration.setup (which invokes installation.list_apps) has no rescue and can raise, causing a 500. Wrap it in a rescue Installations::Error and default @ci_cd_apps to an empty array to prevent uncaught exceptions. [app/controllers/ci_cd/bitrise_configs_controller.rb:42]
🤖 Prompt for AI Agents
In app/controllers/ci_cd/bitrise_configs_controller.rb around lines 41 to 44,
the call to @bitrise_integration.setup can raise from installation.list_apps and
currently has no rescue; wrap the setup call in a begin/rescue block that
rescues Installations::Error, sets @ci_cd_apps = [] on error (so the controller
doesn't 500), and optionally log or notify the error (e.g. Rails.logger.warn
with the exception message); preserve the existing assignment of @project_config
after handling the potential exception.
| def parsed_bitrise_config_params | ||
| bitrise_config_params = params.require(:bitrise_integration) | ||
| .permit(:project_config) | ||
| bitrise_config_params.merge( | ||
| project_config: bitrise_config_params[:project_config]&.safe_json_parse | ||
| ) | ||
| end |
There was a problem hiding this comment.
JSON parsing silently fails on invalid input.
The safe_json_parse method returns an empty hash {} when JSON parsing fails. If a user submits invalid JSON for project_config, it will be silently replaced with an empty hash, potentially overwriting existing configuration without clear feedback.
Consider adding validation to detect and report JSON parsing failures, or at minimum, logging when parsing fails to aid debugging.
Note: This pattern is consistent with similar controllers (BitbucketConfigsController, GithubConfigsController), suggesting this might be intentional behavior across the codebase.
🤖 Prompt for AI Agents
In app/controllers/ci_cd/bitrise_configs_controller.rb around lines 46 to 52,
the call to safe_json_parse silently returns {} on invalid JSON which can
overwrite existing project_config without feedback; update
parsed_bitrise_config_params to detect parse failures by checking whether
project_config was valid JSON (e.g., have safe_json_parse return nil or raise,
or check raw string vs parsed result), and on failure either add a validation
error to the params/record (so the controller responds with a 4xx and an error
message) or at minimum log the parsing error with context (user/id/params)
before returning; ensure the controller path used by similar controllers is
followed for consistent behavior (reject invalid JSON rather than silently
replacing it).
| def bugsnag_config(config_params) | ||
| config = {} | ||
|
|
||
| if config_params[:ios_release_stage].present? | ||
| config[:ios_config] = { | ||
| project_id: config_params[:ios_project_id]&.safe_json_parse, | ||
| release_stage: config_params[:ios_release_stage] | ||
| } | ||
| end | ||
|
|
||
| if config_params[:android_release_stage].present? | ||
| config[:android_config] = { | ||
| project_id: config_params[:android_project_id]&.safe_json_parse, | ||
| release_stage: config_params[:android_release_stage] | ||
| } | ||
| end | ||
|
|
||
| config | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
Verify error handling for invalid JSON in project IDs.
Lines 61 and 68 use safe_json_parse, which returns an empty hash ({}) on parse errors. If project_id contains invalid JSON, it will silently be set to {} rather than raising an error or storing the raw value. This might not be the desired behavior.
Ensure the BugsnagIntegration model has appropriate validations to reject empty or invalid project_id values, or consider explicitly handling JSON parse errors in the controller to provide user feedback.
Run this script to check for model validations:
🏁 Script executed:
#!/bin/bash
# Check BugsnagIntegration model for validations on config fields
ast-grep --pattern $'class BugsnagIntegration
$$$
validate $$$
$$$
'
# Also check for any custom validation methods
rg -nP --type=ruby -A 5 'def (validate|valid_project_id|valid_config)' app/models/bugsnag_integration.rbLength of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Locate the BugsnagIntegration class definition
model_file=$(rg -l 'class BugsnagIntegration' -g '*.rb')
echo "Found model file: $model_file"
# Search for validation macros or custom validation methods
rg -n 'validates?' "$model_file"
rg -n 'def validate' "$model_file"
Length of output: 136
Add validation for parsed project_id in BugsnagIntegration. The model (app/models/bugsnag_integration.rb) currently only validates access_token and correct_key, so {} from safe_json_parse can be saved silently. Add validates or a custom validator for ios_config[:project_id] and android_config[:project_id], or surface JSON parse errors in the controller.
🤖 Prompt for AI Agents
In app/controllers/monitoring/bugsnag_configs_controller.rb around lines 56–74,
the controller currently assigns project_id using safe_json_parse which can
return {} and be saved silently; add a check after parsing to ensure the parsed
project_id is present and of the expected type (or not an empty hash), and if it
is invalid respond with a parameter error (or add to config.errors) so the bad
payload is rejected. Alternatively (or additionally), add validations in
app/models/bugsnag_integration.rb that require ios_config["project_id"] when
ios_release_stage is present and android_config["project_id"] when
android_release_stage is present (presence and format/type checks or a custom
validator to detect empty-hash from failed JSON parse), so invalid or empty
parsed project_id values fail model validation instead of being saved.
| firebase_build_channel = app.integrations.firebase_build_channel_provider | ||
| configs_ready?(firebase_build_channel.ios_config, firebase_build_channel.android_config) |
There was a problem hiding this comment.
Nil-guard firebase_build_channel to avoid NPE.
The firebase_build_channel_provider can return nil even when firebase_connected? is true (e.g., if the integration exists but has no providable, or in edge cases during deletion). Calling .ios_config on nil will raise NoMethodError.
Apply this diff to add a nil guard:
def firebase_ready?
return true unless app.firebase_connected?
firebase_build_channel = app.integrations.firebase_build_channel_provider
+ return false unless firebase_build_channel
configs_ready?(firebase_build_channel.ios_config, firebase_build_channel.android_config)
endAlternatively, use safe navigation:
def firebase_ready?
return true unless app.firebase_connected?
firebase_build_channel = app.integrations.firebase_build_channel_provider
- configs_ready?(firebase_build_channel.ios_config, firebase_build_channel.android_config)
+ configs_ready?(firebase_build_channel&.ios_config, firebase_build_channel&.android_config)
end📝 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.
| firebase_build_channel = app.integrations.firebase_build_channel_provider | |
| configs_ready?(firebase_build_channel.ios_config, firebase_build_channel.android_config) | |
| def firebase_ready? | |
| return true unless app.firebase_connected? | |
| firebase_build_channel = app.integrations.firebase_build_channel_provider | |
| return false unless firebase_build_channel | |
| configs_ready?(firebase_build_channel.ios_config, firebase_build_channel.android_config) | |
| end |
🤖 Prompt for AI Agents
In app/models/app_config.rb around lines 57 to 58, the code calls
firebase_build_channel.ios_config without guarding against
firebase_build_channel being nil; change the code to nil-guard the provider
(e.g., return false early if firebase_build_channel is nil) or use safe
navigation when passing configs (e.g., firebase_build_channel&.ios_config,
firebase_build_channel&.android_config) so NoMethodError is avoided and
configs_ready? receives nils safely.
| monitoring_provider = app.monitoring_provider | ||
| configs_ready?(monitoring_provider.ios_config, monitoring_provider.android_config) |
There was a problem hiding this comment.
Nil-guard monitoring_provider to avoid NPE.
The monitoring_provider can return nil even when bugsnag_connected? is true (e.g., if the integration exists but has no providable, or in edge cases during deletion). Calling .ios_config on nil will raise NoMethodError.
Apply this diff to add a nil guard:
def bugsnag_ready?
return true unless app.bugsnag_connected?
monitoring_provider = app.monitoring_provider
+ return false unless monitoring_provider
configs_ready?(monitoring_provider.ios_config, monitoring_provider.android_config)
endAlternatively, use safe navigation:
def bugsnag_ready?
return true unless app.bugsnag_connected?
monitoring_provider = app.monitoring_provider
- configs_ready?(monitoring_provider.ios_config, monitoring_provider.android_config)
+ configs_ready?(monitoring_provider&.ios_config, monitoring_provider&.android_config)
end🤖 Prompt for AI Agents
In app/models/app_config.rb around lines 68-69, calling
monitoring_provider.ios_config (and .android_config) can raise NoMethodError
when monitoring_provider is nil; guard against nil by checking
monitoring_provider before accessing its configs or use safe navigation (e.g.,
only call .ios_config and .android_config if monitoring_provider.present? or use
monitoring_provider&.ios_config, monitoring_provider&.android_config) so
configs_ready? receives nils instead of raising.
| delegate :app, to: :integration | ||
| delegate :cache, to: Rails | ||
| validates :cloud_id, presence: true | ||
| validate :release_filters_are_valid, if: -> { project_config&.dig("release_filters").present? } |
There was a problem hiding this comment.
Add array type check to validation condition.
The validation condition checks for presence but not type. If project_config["release_filters"] is set to a non-array value (e.g., a string or hash), the condition evaluates to true, triggering the validation method which calls .each on line 269, resulting in a NoMethodError.
Apply this diff to add an array type check:
- validate :release_filters_are_valid, if: -> { project_config&.dig("release_filters").present? }
+ validate :release_filters_are_valid, if: -> { project_config&.dig("release_filters").is_a?(Array) }Note: is_a?(Array) returns false for nil and non-array values, so it's both a presence and type check.
📝 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.
| validate :release_filters_are_valid, if: -> { project_config&.dig("release_filters").present? } | |
| validate :release_filters_are_valid, if: -> { project_config&.dig("release_filters").is_a?(Array) } |
🤖 Prompt for AI Agents
In app/models/jira_integration.rb around line 65, the validation guard uses
presence only and can be true for non-array values causing the
release_filters_are_valid method to call `.each` on a non-array and raise
NoMethodError; update the condition to both check presence and type (e.g.,
ensure project_config&.dig("release_filters").is_a?(Array)) so the validation
runs only when release_filters is an Array.
| def release_filters_are_valid | ||
| project_config["release_filters"].each do |filter| | ||
| unless filter.is_a?(Hash) && VALID_FILTER_TYPES.include?(filter["type"]) && filter["value"].present? | ||
| errors.add(:project_config, "release filters must contain valid type and value") | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Guard against non-array release_filters.
The method calls .each on project_config["release_filters"] without verifying it's an array. If release_filters is nil or a non-array type, this raises a NoMethodError. While the validation condition on line 65 checks for presence, it doesn't enforce array type, allowing this failure scenario.
Apply this diff to add a type guard:
def release_filters_are_valid
+ return unless project_config["release_filters"].is_a?(Array)
+
project_config["release_filters"].each do |filter|
unless filter.is_a?(Hash) && VALID_FILTER_TYPES.include?(filter["type"]) && filter["value"].present?
errors.add(:project_config, "release filters must contain valid type and value")
end
end
endConsider extracting duplicate validation logic.
This validation method is duplicated in LinearIntegration (see app/models/linear_integration.rb lines 245-251). Consider extracting this logic into a shared concern or module to follow the DRY principle and simplify maintenance.
For example, create a concern:
# app/models/concerns/release_filter_validatable.rb
module ReleaseFilterValidatable
extend ActiveSupport::Concern
VALID_FILTER_TYPES = %w[label fix_version].freeze
included do
validate :release_filters_are_valid, if: -> { project_config&.dig("release_filters").is_a?(Array) }
end
private
def release_filters_are_valid
project_config["release_filters"].each do |filter|
unless filter.is_a?(Hash) && VALID_FILTER_TYPES.include?(filter["type"]) && filter["value"].present?
errors.add(:project_config, "release filters must contain valid type and value")
end
end
end
endThen include it in both JiraIntegration and LinearIntegration:
class JiraIntegration < ApplicationRecord
include ReleaseFilterValidatable
# ...
endBased on learnings.
Safe-nav felt unnecessary since Integration depends on App.
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/models/integration.rb(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/models/integration.rb (5)
app/models/app_config.rb (5)
firebase_ready?(55-59)bugsnag_ready?(66-70)project_management_ready?(78-99)bitrise_ready?(61-64)configs_ready?(72-76)app/models/app.rb (2)
bitrise_connected?(109-111)bugsnag_connected?(113-115)app/models/bitrise_integration.rb (1)
bitrise_project(64-66)app/models/concerns/integrable.rb (1)
firebase_connected?(14-16)app/models/external_app.rb (2)
ios?(33-33)android?(35-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (1)
app/models/integration.rb (1)
182-184: Connected vs kept scope change is safe. Sincedisconnectsetsdiscarded_at, thekeptscope (non-discarded) will never include disconnected records. No action needed.
| def category_ready?(category) | ||
| # Does the following need to change to `ready.first.integrable` since app seems like an optional `belongs_to` association? | ||
| app = ready.first&.app |
There was a problem hiding this comment.
Address the existing concern about optional app association.
The comment on line 146 correctly identifies a potential issue. Since app is an optional association (line 25: belongs_to :app, optional: true), but integrable is required (via delegated_type), using integrable would be more robust.
However, integrable can be either an App or AppVariant. If this method needs specifically an App, you'll need to handle the case where integrable is an AppVariant:
-# Does the following need to change to `ready.first.integrable` since app seems like an optional `belongs_to` association?
-app = ready.first&.app
+app = ready.first&.integrable
+app = app.app if app.is_a?(AppVariant) # Get parent app if integrable is AppVariant
+return false unless appOr if the logic should work at the integrable level rather than app level, adjust the subsequent checks accordingly.
📝 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.
| def category_ready?(category) | |
| # Does the following need to change to `ready.first.integrable` since app seems like an optional `belongs_to` association? | |
| app = ready.first&.app | |
| def category_ready?(category) | |
| app = ready.first&.integrable | |
| app = app.app if app.is_a?(AppVariant) # Get parent app if integrable is AppVariant | |
| return false unless app | |
| … # rest of method unchanged |
🤖 Prompt for AI Agents
In app/models/integration.rb around lines 145 to 147, replace the fragile
ready.first&.app access with ready.first&.integrable because app is an optional
belongs_to while integrable is required; then handle the two possible integrable
types: if you specifically need an App, detect when integrable is an AppVariant
and obtain its parent App (or return false if no parent), otherwise proceed
using the integrable directly if your logic applies to either App or
AppVariant—alternatively, if the method should operate at the integrable level,
update the subsequent checks to reference integrable instead of app.
| def configured? | ||
| return false if none? # need at least one integration | ||
|
|
||
| further_setup_by_category | ||
| .values | ||
| .pluck(:ready) | ||
| .all? | ||
| end |
There was a problem hiding this comment.
Critical: configured? doesn't verify minimum required categories are present.
The method only checks that present categories are ready, not that the minimum required set (:version_control, :ci_cd, :build_channel) are actually connected. If a required category is missing, it won't appear in further_setup_by_category and configured? will incorrectly return true.
Compare with the ready? method (lines 161-163) which explicitly checks MINIMUM_REQUIRED_SET.
Apply this diff to ensure minimum categories are present:
def configured?
return false if none? # need at least one integration
+ return false unless ready? # ensure minimum required categories are connected
further_setup_by_category
.values
.pluck(:ready)
.all?
end🤖 Prompt for AI Agents
In app/models/integration.rb around lines 165 to 172, configured? currently only
checks that existing categories are ready but doesn't ensure the minimum
required categories (:version_control, :ci_cd, :build_channel) are present;
update configured? to first verify that MINIMUM_REQUIRED_SET is a subset of
further_setup_by_category.keys (or that all required category keys exist), then
ensure all those categories (or all values) report ready. Concretely: if none?
return false, compute the category map, return false if any required category is
missing, then check the ready flags (pluck(:ready).all?) and return that result.
| if connected_integrations.build_channel.present? | ||
| categories[:build_channel] = { | ||
| further_setup: connected_integrations.build_channel.map(&:providable).any?(&:further_setup?), | ||
| ready: firebase_ready? | ||
| } | ||
| end |
There was a problem hiding this comment.
Major: Provider-specific readiness check applied to entire category.
The build_channel category calls firebase_ready? for all build channel providers, but this category can include AppStoreIntegration, GooglePlayStoreIntegration, GoogleFirebaseIntegration, and SlackIntegration. Only Firebase requires the config checked by firebase_ready?.
If the build channel is AppStore or GooglePlayStore without Firebase, this incorrectly returns ready: true when those providers might need their own configuration checks.
Consider refactoring to check provider-specific readiness:
if connected_integrations.build_channel.present?
+ build_providers = connected_integrations.build_channel.map(&:providable)
categories[:build_channel] = {
- further_setup: connected_integrations.build_channel.map(&:providable).any?(&:further_setup?),
- ready: firebase_ready?
+ further_setup: build_providers.any?(&:further_setup?),
+ ready: build_providers.all? { |p| provider_ready?(p) }
}
endWhere provider_ready? dispatches to the appropriate readiness check per provider type.
Committable suggestion skipped: line range outside the PR's diff.
| if connected_integrations.monitoring.present? | ||
| categories[:monitoring] = { | ||
| further_setup: connected_integrations.monitoring.any?(&:further_setup?), | ||
| ready: bugsnag_ready? | ||
| } | ||
| end |
There was a problem hiding this comment.
Major: Provider-specific readiness check applied to entire category.
Similar to the build_channel issue, monitoring calls bugsnag_ready? for all monitoring providers, but this category can include both BugsnagIntegration and CrashlyticsIntegration. Only Bugsnag requires the config checked by bugsnag_ready?.
Apply the same provider-specific readiness pattern suggested for build_channel.
🤖 Prompt for AI Agents
In app/models/integration.rb around lines 253 to 258, the monitoring category
currently uses a single bugsnag_ready? check for all providers; change this to a
provider-specific readiness computation like in build_channel: inspect
connected_integrations.monitoring to detect provider classes (e.g.,
BugsnagIntegration, CrashlyticsIntegration), and set
categories[:monitoring][:ready] based on the appropriate readiness check for
each detected provider (call bugsnag_ready? only when BugsnagIntegration
instances exist, call the Crashlytics readiness helper when
CrashlyticsIntegration exists, or compute a combined boolean that aggregates
per-provider readiness).
| def bitrise_ready? | ||
| app = first.integrable | ||
| return true unless app.bitrise_connected? | ||
|
|
||
| bitrise_project.present? | ||
| end | ||
|
|
||
| def bitrise_project | ||
| bitrise_ci_cd_provider&.bitrise_project | ||
| end |
There was a problem hiding this comment.
Critical: Potential NoMethodError and confusing logic.
Two issues:
-
Line 298:
first.integrablewill raiseNoMethodErrorif the collection is empty. Add a guard:app = first&.integrable return false unless app
-
Line 299: The check
return true unless app.bitrise_connected?is confusing in this context.bitrise_ready?is only called fromci_cd_ready?whenci_cd_provideris aBitriseIntegration, which implies Bitrise is already connected. Returningtruewhen it's not connected seems contradictory.
If the intent is "Bitrise doesn't need configuration if not connected," this logic should be in ci_cd_ready?, not here.
Apply this diff to fix the nil safety issue:
def bitrise_ready?
- app = first.integrable
+ app = first&.integrable
+ return false unless app
return true unless app.bitrise_connected?
bitrise_project.present?
end🤖 Prompt for AI Agents
app/models/integration.rb around lines 297 to 306: fix nil-safety and clarify
the connectivity check by making app retrieval safe (use first&.integrable) and
return false if app is nil, and change the confusing early-true branch to
require the app to be connected (replace `return true unless
app.bitrise_connected?` with `return false unless app.bitrise_connected?`),
leaving the final result as `bitrise_project.present?`.
| def firebase_ready? | ||
| app = first.integrable | ||
| return true unless app.firebase_connected? | ||
|
|
||
| firebase_build_channel = firebase_build_channel_provider | ||
| configs_ready?(app, firebase_build_channel&.android_config, firebase_build_channel&.ios_config) | ||
| end | ||
|
|
||
| def bugsnag_ready? | ||
| app = first.integrable | ||
| return true unless app.bugsnag_connected? | ||
|
|
||
| monitoring = monitoring_provider | ||
| configs_ready?(app, monitoring&.android_config, monitoring&.ios_config) | ||
| end |
There was a problem hiding this comment.
Critical: Potential NoMethodError in both methods.
Both firebase_ready? (line 309) and bugsnag_ready? (line 317) call first.integrable without nil checks, which will raise NoMethodError if the collection is empty.
Apply this diff to add nil guards:
def firebase_ready?
- app = first.integrable
+ app = first&.integrable
+ return false unless app
return true unless app.firebase_connected?
firebase_build_channel = firebase_build_channel_provider
configs_ready?(app, firebase_build_channel&.android_config, firebase_build_channel&.ios_config)
end
def bugsnag_ready?
- app = first.integrable
+ app = first&.integrable
+ return false unless app
return true unless app.bugsnag_connected?
monitoring = monitoring_provider
configs_ready?(app, monitoring&.android_config, monitoring&.ios_config)
end📝 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.
| def firebase_ready? | |
| app = first.integrable | |
| return true unless app.firebase_connected? | |
| firebase_build_channel = firebase_build_channel_provider | |
| configs_ready?(app, firebase_build_channel&.android_config, firebase_build_channel&.ios_config) | |
| end | |
| def bugsnag_ready? | |
| app = first.integrable | |
| return true unless app.bugsnag_connected? | |
| monitoring = monitoring_provider | |
| configs_ready?(app, monitoring&.android_config, monitoring&.ios_config) | |
| end | |
| def firebase_ready? | |
| app = first&.integrable | |
| return false unless app | |
| return true unless app.firebase_connected? | |
| firebase_build_channel = firebase_build_channel_provider | |
| configs_ready?(app, firebase_build_channel&.android_config, firebase_build_channel&.ios_config) | |
| end | |
| def bugsnag_ready? | |
| app = first&.integrable | |
| return false unless app | |
| return true unless app.bugsnag_connected? | |
| monitoring = monitoring_provider | |
| configs_ready?(app, monitoring&.android_config, monitoring&.ios_config) | |
| end |
🤖 Prompt for AI Agents
In app/models/integration.rb around lines 308 to 322, both firebase_ready? and
bugsnag_ready? call first.integrable without guarding for an empty collection
which can raise NoMethodError; fix by capturing the first record (e.g.
first_record = first), return true if first_record is nil, then get app =
first_record.integrable (or use safe navigation first&.integrable) and guard if
app is nil before calling app.firebase_connected? / app.bugsnag_connected? so
the methods return safely when no first record or integrable exists.
1 participant
Why
Currently,
AppConfigholds a lot of integration-specific configuration, which feels like a data-modelling smell. This modelling also makes it harder to develop certain code paths forAppVariants.Moving the integration-specific configurations to the apt integration providable should simplify our data modelling and allow further features to be built on top of these changes.
This addresses
Enter the details of what all this covers, include screenshots, demos, thought processes etc.
Making
Integrationprovidables own their integration-specific configuration by adding schema migrations.We also enable dedicated write paths for each
Integration's configuration which closely modelsAppConfig's write path, while also modifying existing read paths to use persisted configuration from theIntegrations' providables, rather than the globalAppConfig. This involved the following UI changes on the app's Integrations tab:We also try to ensure the app setup wizard behaves as it did when the configuration values were being read from
AppConfig.App.Finishing.Up.Integration.Configuration.mp4
Due to the fact that
Integrationprovidables can now own their own config, an edge case had to be handled where if a CI/CD integration is attempted to be connected, rather than re-used, the repository config would not be present on the integration, and vice-versa for version control integrations. This would disallow train creation. Thus, the following CI/CD integrations were made to be further configurable:BitbucketIntegrationGithubIntegrationGitlabIntegrationThis edge-case should not surface if the integration is re-used instead.
Scenarios tested
Summary by CodeRabbit