Skip to content

fix: support associations without an id - #6

Merged
michaelst merged 2 commits into
mainfrom
fix-associations-without-id
Aug 3, 2026
Merged

fix: support associations without an id#6
michaelst merged 2 commits into
mainfrom
fix-associations-without-id

Conversation

@michaelst

@michaelst michaelst commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Problem

maybe_remove_from_association/4 dereferenced .id on every member of a loaded has_many:

Enum.filter(list, &(is_map(&1) && &1.id not in records_to_remove))

That raises a KeyError for join style schemas with a composite primary key and no id field. Worse, the filter runs even when records_to_remove is empty, so any sync message crashes a LiveView that has such an association loaded — unrelated inserts, updates, or deletes on some other table are enough.

Real crash from a LiveView watching a record whose child has a composite primary key:

** (KeyError) key :id not found in:

    %Dishbooks.Expenses.Schemas.BillItemLocation{
      __meta__: #Ecto.Schema.Metadata<:loaded, "bill_item_locations">,
      amount: Decimal.new("1200.00"),
      bill_item_id: "bli_01kxpdj8879rk6gh7eyjsvg2zr",
      location_id: "loc_01kw2phfp16vje31k3y3fvfar7",
      ...
    }

    (live_sync 0.1.11) lib/live_sync/socket.ex:211: anonymous fn/2 in LiveSync.Socket.maybe_remove_from_association/4
    (elixir 1.19.5) lib/enum.ex:4445: Enum.filter_list/2
    (live_sync 0.1.11) lib/live_sync/socket.ex:168: anonymous fn/6 in LiveSync.Socket.traverse_associations/3
    ...

Fix

Compare records with LiveSync.lookup_info/1 — the same lookup that already keys the updates map — instead of raw .id. Schemas with no Watch impl look up as nil and are never removed, which is the correct behavior for a child schema that isn't synced.

sync/4 had the same assumption baked in (record.__struct__ == schema and record.id == id) and would raise for any schema deriving Watch with a custom id: option, so it uses the lookup too.

Tests

New works with associations without an id reproduces the stack frame above, via a LiveSync.ExampleTag support schema with a composite primary key. Verified red before the fix, green after.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved live synchronization for associations whose records do not have a standalone ID.
    • Association items now remain correctly synchronized when parent records are updated or removed.
  • New Features

    • Added support for displaying and synchronizing example tags associated through composite keys.
  • Tests

    • Added coverage validating initial tag rendering and continued visibility after updates.
    • Expanded test setup for tag associations.

`maybe_remove_from_association/4` dereferenced `.id` on every member of a
loaded has_many, which raises a KeyError for join style schemas with a
composite primary key. The filter ran even when nothing was being removed,
so any sync message crashed a LiveView with such an association loaded.

Compare records using `LiveSync.lookup_info/1` instead, which is also what
keys the updates map. Schemas without a `Watch` impl look up as `nil` and are
never removed. `sync/4` had the same assumption baked in and would raise for
any schema deriving `Watch` with a custom `id` option.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because your workspace is out of credits. Ask your workspace admin to add credits to resume reviews. Manage billing

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 14e763c3-75c5-4c01-96f0-e9a4114a2832

📥 Commits

Reviewing files that changed from the base of the PR and between e4ddabf and 6b6edf0.

📒 Files selected for processing (1)
  • test/support/live_page.ex

📝 Walkthrough

Walkthrough

The change adds an ID-less ExampleTag association, uses lookup values for synchronization matching, renders tags, adds regression coverage, and updates the project version to 0.1.12.

Changes

ID-less association synchronization

Layer / File(s) Summary
Composite-key association fixture
test/support/example_tag.ex, test/support/example.ex, lib/test_helper.exs
Adds the composite-key LiveSync.ExampleTag schema, the example_tags association, and database setup and cleanup.
Lookup-based synchronization matching
lib/live_sync/socket.ex
sync/4 and association removal compare records through LiveSync.lookup_info/1 instead of direct id fields.
Rendering and regression validation
test/support/live_page.ex, lib/live_sync/socket_test.exs, lib/live_sync/replication_test.exs, mix.exs
Preloads and renders tags, verifies tag persistence after a parent update, cleans up the new table, and updates the project version.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Test
  participant LivePage
  participant LiveSync
  participant Database
  Test->>Database: Create ExampleTag
  LivePage->>Database: Preload example_tags
  LivePage->>Test: Render tag name
  Test->>LiveSync: Update parent Example
  LiveSync->>LiveSync: Compare lookup_info values
  LiveSync->>Test: Synchronize updated parent and tag
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: supporting associations without an id field.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-associations-without-id

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/support/live_page.ex`:
- Line 26: Update the preload list in sync/3 to include the :ignored association
alongside :parent, :children, and :example_tags, ensuring render/1 can safely
enumerate `@data.ignored` when it is not already loaded.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 17959656-9051-43e9-b8ee-eca879c2093d

📥 Commits

Reviewing files that changed from the base of the PR and between cb8777b and e4ddabf.

📒 Files selected for processing (8)
  • lib/live_sync/replication_test.exs
  • lib/live_sync/socket.ex
  • lib/live_sync/socket_test.exs
  • lib/test_helper.exs
  • mix.exs
  • test/support/example.ex
  • test/support/example_tag.ex
  • test/support/live_page.ex

Comment thread test/support/live_page.ex Outdated
Already loaded associations survive the traversal, so the test page does not
need to preload them again. Not re-preloading also makes the regression test
stronger: it now proves the composite primary key association is carried
through traverse_associations/3 rather than being reloaded after it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@michaelst
michaelst merged commit 7f7b863 into main Aug 3, 2026
17 of 19 checks passed
@michaelst
michaelst deleted the fix-associations-without-id branch August 3, 2026 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant