Upgrade elixir 1.17 #713
Upgrade elixir 1.17 #713
Conversation
WalkthroughThe pull request includes updates to various components of the Orcasite application. Key changes involve upgrading the Elixir and Erlang versions in the Dockerfile and Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
Dockerfile (1)
4-4: Consider documenting upgrade stepsFor future reference and to help other developers, consider adding upgrade notes to the repository's documentation.
Add a section in the README or create an UPGRADING.md file with:
- Required Elixir/OTP versions
- Breaking changes
- Migration steps
- Verification steps
Would you like me to help create this documentation?
server/test/orcasite_web/graphql/moderator_test.exs (1)
81-87: LGTM! Added comprehensive notification verification.Good addition of detailed assertions that verify:
- The notification can be retrieved using the returned ID
- The notification's meta data contains the correct message
- The notification is associated with the correct candidate
Consider adding assertions for additional notification properties like timestamps or status if they are relevant to the business logic. For example:
assert %{ meta: %{"message" => "Test notification", "candidate_id" => ^candidate_id}, inserted_at: %DateTime{}, # verify timestamp exists status: :sent # verify expected status } = notificationui/src/components/auth/SignInForm.tsx (2)
Line range hint
1-90: Consider enhancing form validation.While the current implementation is functional, consider these improvements:
- Add client-side validation for minimum password length
- Prevent form submission when fields are empty
- Add loading state during submission
Example enhancement for password validation:
<TextField label="Password" variant="outlined" margin="normal" fullWidth type="password" + inputProps={{ minLength: 8 }} + required value={password} onChange={(event) => setPassword(event.target.value)}
The error string "Authentication failed" needs to be updated to match the backend
The backend code shows that authentication failures return an error with code "invalid_credentials", not "Authentication failed". The error handling in SignInForm.tsx should be updated to match this.
- Update the error check in
ui/src/components/auth/SignInForm.tsxfrom:to:if (error === "Authentication failed")if (error === "invalid_credentials")🔗 Analysis chain
Line range hint
142-148: LGTM! Verify the exact error string from Ash.The error handling has been updated to match the new Ash authentication error format, with clear user-friendly messages. The implementation properly handles all cases.
Let's verify the exact error string from Ash:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for authentication error definitions in Elixir files rg -t elixir "authentication.*failed" -B 2 -A 2Length of output: 49
Script:
#!/bin/bash # Let's try different variations of authentication error patterns in Elixir files rg -t elixir "(:authentication|authentication|auth.*failed|invalid.*credentials)" -B 2 -A 2 # Also search for error definitions in authentication-related Elixir files fd -e ex "(auth|authentication)" --exec rg -l "error" {}Length of output: 2715
Script:
#!/bin/bash # Let's check the auth controller for error definitions cat ./server/lib/orcasite_web/controllers/auth_controller.ex # Let's also check the GraphQL accounts type implementation since we found "invalid_credentials" there cat ./server/lib/orcasite_web/graphql/types/accounts.exLength of output: 4209
server/lib/orcasite/radio/audio_image.ex (1)
Line range hint
1-222: Consider enhancing error handling in spectrogram generationWhile reviewing the context around the GraphQL changes, I noticed potential improvements in error handling:
- The
generate_spectrogramaction usesTask.Supervisor.async_nolinkwith a 2-minute timeout, but there's no explicit handling of timeout errors.- The error message stored in
last_erroruses rawinspect(error)which might not be ideal for client consumption.Consider these improvements:
change after_action( fn _change, image, _context -> # Only one feed segment at a time for now [feed_segment] = image |> Ash.load!(:feed_segments) |> Map.get(:feed_segments) timeout = :timer.minutes(2) Task.Supervisor.async_nolink( Orcasite.TaskSupervisor, fn -> %{ image_id: image.id, audio_bucket: feed_segment.bucket, audio_key: feed_segment.segment_path, image_bucket: image.bucket, image_key: image.object_path } |> Orcasite.Radio.AwsClient.generate_spectrogram() |> case do {:ok, %{file_size: image_size}} -> image |> Ash.Changeset.for_update(:update, %{ status: :complete, image_size: image_size }) |> Ash.Changeset.force_change_attribute(:last_error, nil) |> Ash.update(authorize?: false) {:error, error} -> image |> Ash.Changeset.for_update(:update, %{ status: :failed }) - |> Ash.Changeset.force_change_attribute(:last_error, inspect(error)) + |> Ash.Changeset.force_change_attribute(:last_error, format_error_message(error)) |> Ash.update(authorize?: false) end end, - timeout: timeout + timeout: timeout + ) + |> Task.await(timeout) + |> case do + {:ok, result} -> result + {:exit, :timeout} -> + image + |> Ash.Changeset.for_update(:update, %{status: :failed}) + |> Ash.Changeset.force_change_attribute(:last_error, "Spectrogram generation timed out") + |> Ash.update(authorize?: false) ) {:ok, image} end, prepend?: true )Add this helper function:
defp format_error_message(error) when is_binary(error), do: error defp format_error_message(error) when is_atom(error), do: to_string(error) defp format_error_message(error), do: "Error generating spectrogram: #{inspect(error)}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
server/mix.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Dockerfile(1 hunks)server/.tool-versions(1 hunks)server/config/prod.exs(0 hunks)server/lib/orcasite/logger.ex(1 hunks)server/lib/orcasite/notifications/resources/notification.ex(1 hunks)server/lib/orcasite/notifications/resources/notification_instance.ex(0 hunks)server/lib/orcasite/radio/audio_image.ex(1 hunks)server/lib/orcasite/radio/audio_image_feed_segment.ex(1 hunks)server/lib/orcasite/radio/candidate.ex(1 hunks)server/lib/orcasite/radio/detection.ex(1 hunks)server/lib/orcasite/radio/feed_segment.ex(1 hunks)server/lib/orcasite_web/endpoint.ex(0 hunks)server/lib/orcasite_web/graphql/errors/authentication.ex(0 hunks)server/mix.exs(2 hunks)server/test/orcasite_web/graphql/moderator_test.exs(1 hunks)ui/src/components/auth/SignInForm.tsx(1 hunks)
💤 Files with no reviewable changes (4)
- server/config/prod.exs
- server/lib/orcasite/notifications/resources/notification_instance.ex
- server/lib/orcasite_web/endpoint.ex
- server/lib/orcasite_web/graphql/errors/authentication.ex
✅ Files skipped from review due to trivial changes (1)
- server/.tool-versions
🔇 Additional comments (18)
Dockerfile (2)
1-4: Excellent security practice with SHA256 digests!
Good job adding SHA256 digests to both Node and Elixir base images. This ensures image integrity and prevents supply chain attacks.
4-4: Verify compatibility with Elixir 1.17 and OTP 27
The upgrade involves significant version changes:
- Elixir: 1.16 → 1.17
- OTP: 26 → 27
These are major version upgrades that might introduce breaking changes.
Let's verify the compatibility:
✅ Verification successful
The upgrade to Elixir 1.17 and OTP 27 appears safe to proceed
The analysis shows favorable conditions for the upgrade:
- The project specifies a flexible Elixir version requirement (
~> 1.12) - Uses Phoenix 1.7.2 which is compatible with newer Elixir versions
- Limited GenServer usage (only in rate_limiter.ex)
- No direct Erlang code (.erl files) or custom Erlang build configurations
- Standard runtime configurations without version-specific features
The conservative version constraints and minimal use of low-level OTP features suggest the upgrade should work without issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential compatibility issues with the upgrade
# Check mix.exs for dependency versions
echo "Checking mix.exs for dependency constraints..."
rg -A 5 "deps do" server/mix.exs
# Look for OTP-specific configurations
echo "Checking for OTP-specific configurations..."
rg -l "otp|erlang" server/config/
# Check for deprecated features usage
echo "Checking for potentially deprecated Elixir 1.16 features..."
rg -l "GenServer.handle_call|GenServer.handle_cast|GenServer.handle_info" server/lib/
# Look for version-specific compiler options
echo "Checking for compiler options..."
rg "elixirc_options|erlc_options" server/mix.exs
Length of output: 761
Script:
#!/bin/bash
# Let's check for more specific compatibility indicators
# Check mix.exs for Elixir version requirement
echo "Checking Elixir version requirement in mix.exs..."
rg "elixir: " server/mix.exs
# Look for Phoenix version and other major dependencies
echo "Checking Phoenix and major dependencies..."
rg -A 10 "deps do" server/mix.exs
# Check for any custom GenServer implementations
echo "Checking for GenServer implementations..."
rg -l "use GenServer" server/lib/
# Look for any Erlang-specific code or NIFs
echo "Looking for Erlang-specific code..."
fd -e erl
# Check for any .erlang.mk files or rebar configs
echo "Checking for Erlang build configurations..."
fd "rebar|erlang.mk"
# Look for Runtime configuration
echo "Checking runtime configurations..."
rg "config :" server/config/runtime.exs
Length of output: 1458
server/test/orcasite_web/graphql/moderator_test.exs (2)
76-80: LGTM! Improved test assertion specificity.
The change from using a wildcard pattern match to explicitly capturing the notification_id is a good improvement. This allows for further verification of the notification details in subsequent assertions.
Line range hint 1-1: Verify Elixir 1.17 test features compatibility
Since this PR includes an upgrade to Elixir 1.17, let's verify if the test module is using the latest testing features and patterns.
✅ Verification successful
Let me gather more information about the test helpers and ExUnit configuration to ensure we have a complete picture.
Test implementation is already following best practices, no Elixir 1.17 specific changes needed
The test module is already well-structured and follows current best practices:
- Uses appropriate test case templates and helpers
- Properly sets up async testing capabilities
- Implements clear test organization with describe blocks
- Uses proper assertion patterns
The mix.exs shows the project is using modern testing dependencies including:
- ExUnit (built into Elixir)
- mix_test_watch for development
- ex_unit_notifier for test notifications
While Elixir 1.17 introduces some new features, none of them impact the current test implementation or require changes to maintain compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if mix.exs specifies ExUnit version and any test-related dependencies
rg -A 5 "ExUnit" mix.exs
# Look for any version-specific test helpers or configurations
fd -e exs -e ex | xargs rg "ExUnit|use.*Case"
Length of output: 1060
Script:
#!/bin/bash
# Check test helper implementations for any version-specific features
cat server/test/test_helper.exs
cat server/test/support/data_case.ex
cat server/test/support/conn_case.ex
# Check the specific test file for any test patterns
cat server/test/orcasite_web/graphql/moderator_test.exs
# Look for mix.exs in the server directory
cat server/mix.exs
Length of output: 11606
server/mix.exs (5)
91-91: Good improvement: Moving to official ash_uuid release
Switching from a GitHub tag to an official release version (~> 1.1.2) is a good practice for:
- Better version control
- Easier dependency management
- More reliable dependency resolution
89-89: Review MJML 4.0 email template compatibility
The upgrade from MJML 3.0 to 4.0 is a major version bump. Please ensure all email templates are compatible with the new version.
#!/bin/bash
# Find and check all MJML templates
fd -e mjml | while read -r template; do
echo "=== $template ==="
cat "$template" | head -n 10
done69-69: Verify compatibility with Poison 6.0
The upgrade from Poison 5.0 to 6.0 is a major version bump that may include breaking changes. Please ensure compatibility with other JSON-dependent libraries in the project, particularly:
- Absinthe GraphQL (which uses Poison for JSON encoding)
- Phoenix JSON responses
#!/bin/bash
# Search for Poison.encode/decode usage that might need updates
rg -l 'Poison\.(encode|decode)' | while read -r file; do
echo "=== $file ==="
rg 'Poison\.(encode|decode)' "$file"
done74-74: Check reverse_proxy_plug 3.0 breaking changes
The upgrade to reverse_proxy_plug 3.0 is a major version bump that could affect the NextJS frontend integration. Please verify:
- Any configuration changes required
- Changes to proxy behavior
#!/bin/bash
# Search for reverse_proxy configuration
rg -A 5 'reverse_proxy' config/Line range hint 1-124: Verify Elixir 1.17 compatibility across all dependencies
Since this PR aims to upgrade to Elixir 1.17, please ensure all dependencies are compatible with this version. Pay special attention to:
- Phoenix and its ecosystem
- Ash framework and its extensions
- Database adapters
ui/src/components/auth/SignInForm.tsx (1)
Line range hint 65-73: LGTM! Error display implementation is robust.
The error display logic is well-implemented with proper mapping, spacing, and type safety. Each error is clearly presented to the user through distinct Alert components.
server/lib/orcasite/radio/candidate.ex (1)
187-187: LGTM! Verify consistent GraphQL type usage across related modules.
The addition of feed_id: :id type is appropriate for the UUID field and aligns with the non-nullable belongs_to relationship.
Let's verify consistent GraphQL type usage across related modules:
✅ Verification successful
GraphQL type usage is consistent across all related modules
The verification shows that all modules with feed relationships consistently use :id type for feed_id in their GraphQL configurations:
radio/feed_segment.ex:feed_id: :idradio/audio_image.ex:feed_id: :idradio/candidate.ex:feed_id: :idradio/detection.ex:feed_id: :id
This consistent typing aligns with the non-nullable belongs_to relationships defined in these modules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent GraphQL type definitions for feed_id across related modules
# Expected: All feed_id fields should be typed as :id in GraphQL configurations
# Search for GraphQL type definitions of feed_id
rg -A 5 "attribute_types.*feed_id" server/lib/orcasite/
# Search for feed relationships to cross-reference
rg -A 5 "belongs_to.*:feed" server/lib/orcasite/
Length of output: 4845
server/lib/orcasite/radio/feed_segment.ex (2)
215-215: LGTM! Good improvement to GraphQL type safety.
The explicit type definitions for feed_id and feed_stream_id as :id align with the relationships defined in the module and improve the GraphQL schema's type safety.
215-215: Verify type consistency across related resources.
Let's ensure this type definition pattern is consistently applied across other related Ash resources.
✅ Verification successful
Based on the results, I can now generate the final response as we have sufficient information about the GraphQL attribute type definitions across related resources.
GraphQL attribute type definitions are consistently applied across resources
The type definitions are correctly and consistently defined across all related resources in the Radio context:
feed_segment.ex:[feed_id: :id, feed_stream_id: :id]candidate.ex:[feed_id: :id]audio_image_feed_segment.ex:[feed_segment_id: :id]detection.ex:[candidate_id: :id, feed_id: :id]audio_image.ex:[feed_id: :id]
All ID-type attributes are properly typed as :id in their respective GraphQL definitions, maintaining consistency throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent GraphQL attribute type definitions across related resources
# Look for other resource files that might need similar type definitions
# Find all Ash resource files in the Radio context
echo "=== Checking Radio context resources for GraphQL attribute types ==="
rg -A 3 "graphql do" server/lib/orcasite/radio/
Length of output: 2312
server/lib/orcasite/radio/audio_image.ex (1)
222-222: LGTM: GraphQL type definition is correct and consistent
The addition of explicit type definition for feed_id as :id in GraphQL configuration is appropriate and aligns with the relationship definition. This change maintains consistency with similar changes across other Radio modules.
server/lib/orcasite/notifications/resources/notification.ex (2)
Line range hint 230-270: Consider improving async task reliability and error handling
The async task implementation has several potential issues:
- Using
async_nolinkmeans task failures won't propagate to the parent process - No error handling or retry mechanism for failed subscription counts
- Potential race condition between notification creation and target_count update
- No timeout specified for the async task
Consider applying these improvements:
change fn changeset, _context ->
changeset
|> Ash.Changeset.after_action(fn _, %{id: notification_id} = notification ->
- Task.Supervisor.async_nolink(Orcasite.TaskSupervisor, fn ->
+ Task.Supervisor.start_child(Orcasite.TaskSupervisor, fn ->
+ try do
target_count =
Orcasite.Notifications.Subscription
|> Ash.Query.for_read(:available_for_notification, %{
notification_id: notification_id,
event_type: notification.event_type
})
|> Ash.stream!()
|> Stream.map(fn subscription ->
Orcasite.Notifications.NotificationInstance
|> Ash.Changeset.for_create(:create_with_relationships, %{
notification: notification,
subscription: subscription
})
|> Ash.create!()
end)
|> Enum.reduce(0, fn _, sum -> sum + 1 end)
notification
|> Ash.Changeset.for_update(:update, %{target_count: target_count})
|> Ash.update(authorize?: false)
+ rescue
+ error ->
+ Logger.error("Failed to update target_count for notification #{notification_id}: #{inspect(error)}")
+ # Consider implementing a retry mechanism or alerting
+ end
end)
{:ok, notification}
end)
endAdditionally, consider:
- Adding telemetry or monitoring for task failures
- Implementing a retry mechanism for failed updates
- Setting appropriate timeouts
- Adding tests to verify error handling
#!/bin/bash
# Description: Check for existing error handling patterns and tests
# Look for similar async task patterns
rg "Task.Supervisor" --type elixir
# Check for existing error handling tests
rg "test.*async.*error" --type elixir test/157-159: Verify the implications of using force_change_attribute
The change from change_attribute to force_change_attribute for the meta field bypasses attribute validation checks. While this ensures the meta field is always updated, it could potentially override important existing data.
✅ Verification successful
Using force_change_attribute is appropriate in this context
The use of force_change_attribute for the meta field is justified because:
- The meta field is designed as a flexible map with a default empty value (
default: %{}) - Other resources in the codebase (AudioImage) use
force_change_attributefor similar update scenarios - The meta field is properly handled in the notification workflow:
- It's indexed using gin for efficient querying
- The data is properly formatted using JSON encoding
- The worker correctly processes the meta information for email notifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing meta field usage that might be affected
# Search for meta field references
rg -A 5 "meta.*candidate_id" --type elixir
# Look for any tests that might need updating
rg -A 5 "notify_confirmed_candidate.*meta" --type elixir test/
Length of output: 999
Script:
#!/bin/bash
# Let's try a different approach to find meta field usage and related tests
# Search for meta field usage in notification context
rg -A 5 "meta.*:" server/lib/orcasite/notifications/
# Search for notify_confirmed_candidate action usage
rg -A 5 "notify_confirmed_candidate" server/lib/orcasite/
# Look for any notification tests
fd "notification.*test.exs$"
# Check if there are any existing force_change_attribute usages
rg "force_change_attribute" server/lib/orcasite/
Length of output: 14054
server/lib/orcasite/radio/detection.ex (2)
339-339: LGTM! Explicit GraphQL attribute types improve type safety.
The addition of explicit types for candidate_id and feed_id aligns with best practices and matches the belongs_to relationships defined in the module.
339-339: Verify GraphQL query compatibility and consider additional fields.
Please verify that existing GraphQL queries continue to work as expected with these explicit type definitions. Also, consider if other relationship ID fields in the module should have explicit type definitions for consistency.
✅ Verification successful
Based on the comprehensive analysis of the codebase, I can now provide a final response:
GraphQL queries are compatible with the explicit type definitions
The explicit type definitions [candidate_id: :id, feed_id: :id] in the GraphQL configuration are consistent with:
- The existing GraphQL queries found in the codebase that use these fields
- The resource's relationships (
belongs_to :candidateandbelongs_to :feed) - The schema definitions where these fields are used as IDs
All other relationship ID fields in the module are properly handled through the relationships block and don't require explicit type definitions in the GraphQL configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for GraphQL queries using these fields to verify compatibility
# Search for GraphQL queries using candidate_id or feed_id
rg -t graphql "candidate_id|feed_id" --type-add 'graphql:*.{graphql,gql}'
# Search for other potential ID fields in GraphQL queries that might need type definitions
ast-grep --pattern 'belongs_to :$_, $_' server/lib/orcasite/radio/detection.ex
Length of output: 156
Script:
#!/bin/bash
# Let's try a broader search strategy
# Search for GraphQL schema definitions and type definitions
fd ".graphql$|.gql$" --exec cat {}
# Look for GraphQL-related code in Elixir files
rg "schema|query|mutation" --type elixir -A 5
# Check the Detection schema and related files
rg "schema.*Detection" --type elixir -A 10
# Look for belongs_to relationships in the Detection module
cat server/lib/orcasite/radio/detection.ex
Length of output: 37937
Upgrades Elixir to 1.17, dependencies, some moderator tests.
Summary by CodeRabbit
Release Notes
New Features
feed_idandfeed_segment_id.Bug Fixes
Chores
Refactor
Tests