Conversation
WalkthroughAdds maintainer_emails argument handling to Radio.Feed create action, switches seed action handlers to use feeds!(), introduces logging in seed feed changes, and adjusts UI seed page gating to default-enable in development when the env var is unset. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client as Client
participant API as Radio.Feed
participant DB as DB
Note over Client,API: Feed creation with maintainer_emails
Client->>API: create(feed params, maintainer_emails?)
rect rgba(230,245,255,0.5)
note right of API: Change callback reads maintainer_emails (default [])
API->>API: changeset.put_attr(:maintainer_emails, value)
end
API->>DB: insert feed with maintainer_emails
DB-->>API: result
API-->>Client: created feed or error
sequenceDiagram
autonumber
actor User as User
participant Next as Next.js Build
participant Env as Process Env
Note over User,Next: Seed page static generation
Next->>Env: read ENABLE_SEED_FROM_PROD, NODE_ENV
alt ENV var defined
Next->>Next: enabled = (ENV == "true")
else ENV var undefined
Next->>Next: enabled = (NODE_ENV == "development")
end
Next-->>User: { notFound: !enabled } or { props: {} }
sequenceDiagram
autonumber
participant Seed as Radio.Seed action
participant Feed as Feed provider
participant Logger as Logger
Note over Seed,Feed: Seed handlers now use feeds!()
Seed->>Feed: feeds!() (raises on failure)
alt Success
Seed-->>Seed: continue seeding
Logger-->>Seed: info("Seeded N feeds")
else Failure
Seed-->>Seed: exception propagates
Logger-->>Seed: error("...errors...")
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes broken seeding functionality that was failing due to maintainer emails returning nil from an API that requires authentication. The fix includes setting maintainer emails to an empty list by default and improving seeding error handling.
- Sets default empty list for maintainer emails when creating feeds
- Updates seeding to fail loudly with explicit error logging
- Enables seed page by default in development environment
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ui/src/pages/seed.tsx | Updates seed page visibility logic to default to enabled in development |
| server/lib/orcasite/radio/seed/changes/seed_feeds.ex | Adds logging for seeding success and error cases |
| server/lib/orcasite/radio/seed.ex | Changes feeds() to feeds!() for explicit error handling |
| server/lib/orcasite/radio/feed.ex | Adds maintainer_emails argument with empty array default |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/lib/orcasite/radio/feed.ex (1)
50-50: Mark PII as sensitive.Email addresses are PII; prevent accidental logging/inspection.
-attribute :maintainer_emails, {:array, :string}, allow_nil?: false, public?: true, default: [] +attribute :maintainer_emails, {:array, :string}, + allow_nil?: false, + public?: true, + default: [], + sensitive?: true
🧹 Nitpick comments (3)
server/lib/orcasite/radio/seed/changes/seed_feeds.ex (1)
22-23: Prefer lazy logging with structured metadata.Avoid interpolation cost and add useful fields.
-Logger.info("Successfully seeded #{count} feeds") +Logger.info(fn -> "seed_feeds: success" end, count: count, resource: :feed)server/lib/orcasite/radio/seed.ex (1)
93-93: Same here—intentional exception on failure.If you want a custom error message upstream, consider wrapping in try/rescue and re-raising with context.
-__MODULE__.feeds!() +try do + __MODULE__.feeds!() +rescue + e -> + require Logger + Logger.error(fn -> "seed.latest: feeds! failed" end, error: e) + reraise(e, __STACKTRACE__) +endui/src/pages/seed.tsx (1)
314-317: Build-time gating may require a redeploy to toggle. Consider SSR if you need runtime control.getStaticProps reads env at build time; switching ENABLE_SEED_FROM_PROD post-deploy won’t take effect until a rebuild.
-export async function getStaticProps() { +export async function getServerSideProps() { const enableSeedFromProd = process.env.ENABLE_SEED_FROM_PROD ? process.env.ENABLE_SEED_FROM_PROD === "true" : process.env.NODE_ENV === "development"; // default to enabled in dev return !enableSeedFromProd ? { notFound: true } : { props: {} }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/lib/orcasite/radio/feed.ex(1 hunks)server/lib/orcasite/radio/seed.ex(2 hunks)server/lib/orcasite/radio/seed/changes/seed_feeds.ex(2 hunks)ui/src/pages/seed.tsx(1 hunks)
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (3)
server/lib/orcasite/radio/seed/changes/seed_feeds.ex (2)
6-7: Logger import looks good.
27-32: Log with metadata and confirm add_error/2 input shapeUse structured logging and ensure the
errorsvalue passed to Ash.Changeset.add_error/2 is the form Ash expects (a single error or a list/keyword). IfAsh.bulk_create(..., return_errors?: true)returns a wrapper like%{errors: inner}, passinnertoadd_error/2.File: server/lib/orcasite/radio/seed/changes/seed_feeds.ex (lines 27–32)
-Logger.error("Error while seeding feeds: #{inspect(errors)}") +Logger.error(fn -> "seed_feeds: error" end, errors: errors, resource: :feed)Run to verify error shape (or temporarily log
inspect(errors)):
rg -n "Ash.bulk_create(" -C3 server/lib/orcasite/radio/seed/changes/seed_feeds.ex
rg -n "add_error(" server/lib/orcasite/radio/seed/changes/seed_feeds.ex -C2server/lib/orcasite/radio/seed.ex (1)
64-64: Good: fail fast with feeds!().Confirms seeding fails loudly as intended. Ensure callers (GraphQL/Oban) surface the raised error cleanly to operators.
| argument :maintainer_emails, {:array, :string}, default: [] | ||
|
|
||
| change &change_lat_lng/2 | ||
|
|
||
| change fn changeset, _context -> | ||
| maintainer_emails = Ash.Changeset.get_argument(changeset, :maintainer_emails) || [] | ||
| Ash.Changeset.change_attribute(changeset, :maintainer_emails, maintainer_emails) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure upserts can update maintainer_emails; also normalize values.
Currently not included in upsert_fields, so existing rows won’t update on conflict; also normalize to lower-case/unique.
Apply both diffs:
- Normalize on create:
- change fn changeset, _context ->
- maintainer_emails = Ash.Changeset.get_argument(changeset, :maintainer_emails) || []
- Ash.Changeset.change_attribute(changeset, :maintainer_emails, maintainer_emails)
- end
+ change fn changeset, _context ->
+ maintainer_emails =
+ (Ash.Changeset.get_argument(changeset, :maintainer_emails) || [])
+ |> Enum.map(&String.downcase/1)
+ |> Enum.uniq()
+
+ Ash.Changeset.change_attribute(changeset, :maintainer_emails, maintainer_emails)
+ end- Allow updates on conflict (outside this hunk; in upsert_fields):
upsert_fields [
:name,
:node_name,
:intro_html,
:image_url,
:visible,
:bucket,
:bucket_region,
:cloudfront_url,
:dataplicity_id,
:orcahello_id,
- :location_point
+ :location_point,
+ :maintainer_emails
]🏁 Script executed:
#!/bin/bash
# Verify DB migration exists for the new column
rg -n "maintainer_emails" -g "priv/**/migrations/*.exs" -C2 || echo "MIGRATION NOT FOUND"
# Verify GraphQL schema exposure matches policy (nullable, field policy)
rg -n "nullable_fields.*maintainer_emails" server/lib/orcasite/radio/feed.ex -C2Length of output: 273
Add missing DB migration and update upsert logic
- Create a new Ecto migration under priv/repo/migrations to add
:maintainer_emails(type{:array, :string}, default[]) to the feeds table. - In server/lib/orcasite/radio/feed.ex – inside the
upsert_fieldslist, append:maintainer_emails; in thechangehook, normalize withEnum.map(&String.downcase/1)andEnum.uniq().
GraphQL exposure (nullable_fields [:maintainer_emails]) is already in place.
🤖 Prompt for AI Agents
In server/lib/orcasite/radio/feed.ex around lines 264 to 271, the PR missed
adding a DB migration and updating upsert logic for the new maintainer_emails
field: add an Ecto migration file under priv/repo/migrations that alters the
feeds table to add a maintainer_emails column of type {:array, :string} with
default [] and null: false (or appropriate nullability), and run mix
ecto.migrate; then update server/lib/orcasite/radio/feed.ex by appending
:maintainer_emails to the upsert_fields list and modify the change hook to
normalize the incoming maintainer_emails via Enum.map(&String.downcase/1) |>
Enum.uniq() before calling Ash.Changeset.change_attribute (keeping
change_lat_lng/2 intact). Ensure the migration and schema/upsert changes remain
consistent and tests/GraphQL nullable exposure still pass.
skanderm
left a comment
skanderm
left a comment
There was a problem hiding this comment.
Small request to make this more idiomatic - you can remove the argument and change in favor of adding maintainer_emails to the accept list: https://hexdocs.pm/ash/dsl-ash-resource.html#actions-create-accept
| description "A comma-separated string of longitude and latitude" | ||
| end | ||
|
|
||
| argument :maintainer_emails, {:array, :string}, default: [] |
There was a problem hiding this comment.
If the argument is an attribute and you're setting the attribute to the argument, you can simply add it to the accept list above and skip the argument and change _attribute changeset. You could add it to the upsert_fields too, but it's less important since this is just for seeding.
There was a problem hiding this comment.
Hm I need it to default to empty list though, in case the API returns nil for maintainer_emails (which it will because it's an auth only field). So I guess I could remove the argument but I still need the change no? Do you have a better suggestion? Curious how you would tackle it, I don't really know what's idiomatic
There was a problem hiding this comment.
Ok yeah really good point. In prod we don't want that to be nil, but an empty list works.
skanderm
left a comment
skanderm
left a comment
There was a problem hiding this comment.
Fair point about non-nullability of maintainer_emails, so you can simplify the change with a builtin set_attribute
| description "A comma-separated string of longitude and latitude" | ||
| end | ||
|
|
||
| argument :maintainer_emails, {:array, :string}, default: [] |
There was a problem hiding this comment.
Ok yeah really good point. In prod we don't want that to be nil, but an empty list works.
| change fn changeset, _context -> | ||
| maintainer_emails = Ash.Changeset.get_argument(changeset, :maintainer_emails) || [] | ||
| Ash.Changeset.change_attribute(changeset, :maintainer_emails, maintainer_emails) | ||
| end |
There was a problem hiding this comment.
This can still be simplified to:
| change fn changeset, _context -> | |
| maintainer_emails = Ash.Changeset.get_argument(changeset, :maintainer_emails) || [] | |
| Ash.Changeset.change_attribute(changeset, :maintainer_emails, maintainer_emails) | |
| end | |
| change set_attribute(:maintainer_emails, arg(:maintainer_emails), set_when_nil?: false) |
See: https://hexdocs.pm/ash/Ash.Resource.Change.Builtins.html#set_attribute/3
nilfrom API since they require authSummary by CodeRabbit
New Features
Chores