Skip to content

fix(elixir-client): Improve elixir client parity - #4031

Merged
magnetised merged 3 commits into
mainfrom
magnetised/elixir-client-tweaks
Mar 19, 2026
Merged

magnetised merged 3 commits into
mainfrom
magnetised/elixir-client-tweaks

Conversation

@magnetised

Copy link
Copy Markdown
Contributor

Use full jitter strategy for retry backoff

Switch from multiplicative jitter (90-100% of delay) to full jitter (random between 1 and delay), matching the AWS-recommended strategy used by the TypeScript client. Also align max delay from 30s to 32s.

Add response header validation

Validate that required Electric headers are present on responses, raising a clear error mentioning proxy/CDN misconfiguration when they're missing. Previously, missing headers either caused unhelpful errors (electric-handle) or were silently ignored (electric-offset, electric-schema, electric-cursor).

@codecov

codecov Bot commented Mar 19, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.64%. Comparing base (e3b3134) to head (15c8f04).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4031      +/-   ##
==========================================
+ Coverage   85.56%   85.64%   +0.08%     
==========================================
  Files          51       51              
  Lines        3352     3365      +13     
  Branches      614      615       +1     
==========================================
+ Hits         2868     2882      +14     
+ Misses        482      481       -1     
  Partials        2        2              
Flag Coverage Δ
elixir 77.66% <100.00%> (+0.42%) ⬆️
elixir-client 77.66% <100.00%> (+0.42%) ⬆️
packages/experimental 87.73% <ø> (ø)
packages/react-hooks 86.48% <ø> (ø)
packages/start 82.83% <ø> (ø)
packages/typescript-client 93.81% <ø> (ø)
packages/y-electric 56.05% <ø> (ø)
typescript 88.67% <ø> (ø)
unit-tests 85.64% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@claude

claude Bot commented Mar 19, 2026 •

Copy link
Copy Markdown

Claude Code Review

Summary

This revision adds four targeted tests for the new header validation and makes the bypass_resp helper default electric-cursor to "1" when last_offset is present. The implementation is clean, correct, and the new tests directly verify each validation path.

What's Working Well

  • Four new tests cover every validation branch: all-headers-missing, missing electric-offset, missing electric-schema on non-live, missing electric-cursor on live. Each test is focused and has clear assertions.
  • bypass_resp cursor defaulting (if(opts[:last_offset], do: "1")) correctly handles the common case where existing bypass tests don't pass an explicit cursor — non-live responses get one included, live bypass responses that pass last_offset likewise get one.
  • validate_headers! correctness — the condition not is_live? and is_nil(state.schema) and is_nil(resp.schema) correctly implements first-write-wins (no error if schema was already cached in state.schema) and skips schema validation once live.
  • mock.ex updated to expose next_cursor so Mock-based tests can pass validation without raw HTTP.
  • CI workflow change (ELECTRIC_PORT env var + runtime.exs fix) correctly makes the port configurable to match the CI environment at 3100.

Issues Found

No new issues found.


Correction to Previous Review

My iteration 2 review flagged the "Unable to retrieve data" CI failures as likely new regressions. After tracing stream.ex:222-224 (handle_error/2 simply re-raises the error), it's clear those tests (streams an empty shape, streams a non empty shape, accepts a table name as a shape) are integration tests against a real Electric server — they fail because no Electric server is running in that CI job. These are pre-existing infrastructure failures, not regressions from this PR. The TypeScript test failure (should fall back to long polling...) is similarly pre-existing and unrelated (no TypeScript files changed in this PR).


Suggestions (Nice to Have)

validate_headers! guard status in 200..299 — unchanged from my previous suggestion: narrowing to status == 200 would be more precise, since Electric only ever returns 200 for valid shape responses. Still harmless as-is.


Issue Conformance

No linked issue — unchanged. Recommended to link a corresponding issue if one exists.


Previous Review Status

Issue Status
shape_handle!/1 dead code Fixed (iteration 2)
:rand.uniform/1 comment overstated range Fixed (iteration 2)
next_cursor: 0 typespec violation Fixed (iteration 2)
CI failures flagged as likely regressions Correction: pre-existing infrastructure failures, not regressions
Missing tests for header validation Fixed — 4 new tests added in this iteration

Review iteration: 3 | 2026-03-19

Switch from multiplicative jitter (90-100% of delay) to full jitter
(random between 1 and delay), matching the AWS-recommended strategy used
by the TypeScript client. Also align max delay from 30s to 32s.
Validate that required Electric headers are present on responses,
raising a clear error mentioning proxy/CDN misconfiguration when they're
missing. Previously, missing headers either caused unhelpful errors
(electric-handle) or were silently ignored (electric-offset,
electric-schema, electric-cursor).
@magnetised
magnetised force-pushed the magnetised/elixir-client-tweaks branch from d03ca2a to 6a69b46 Compare March 19, 2026 14:37
@magnetised
magnetised force-pushed the magnetised/elixir-client-tweaks branch from 6a69b46 to 15c8f04 Compare March 19, 2026 14:56
@magnetised
magnetised merged commit 19d6508 into main Mar 19, 2026
45 checks passed
@magnetised
magnetised deleted the magnetised/elixir-client-tweaks branch March 19, 2026 15:31
@github-actions

Copy link
Copy Markdown
Contributor

This PR has been released! 🚀

The following packages include changes from this PR:

  • @core/elixir-client@0.9.3
  • @core/elixir-client@0.9.3

Thanks for contributing to Electric!

balegas pushed a commit that referenced this pull request Apr 2, 2026
### Use full jitter strategy for retry backoff

Switch from multiplicative jitter (90-100% of delay) to full jitter
(random between 1 and delay), matching the AWS-recommended strategy used
by the TypeScript client. Also align max delay from 30s to 32s.

### Add response header validation

Validate that required Electric headers are present on responses,
raising a clear error mentioning proxy/CDN misconfiguration when they're
missing. Previously, missing headers either caused unhelpful errors
(electric-handle) or were silently ignored (electric-offset,
electric-schema, electric-cursor).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants