Skip to content

fix: Shape comparable should include log mode to avoid collisions - #3486

Merged
balegas merged 4 commits into
mainfrom
msfstef/add-log-mode
Nov 24, 2025
Merged

balegas merged 4 commits into
mainfrom
msfstef/add-log-mode

Conversation

@msfstef

@msfstef msfstef commented Nov 20, 2025 •

Copy link
Copy Markdown
Contributor

Fixes #3484

What I was worried about happened - the comparable was not updated as we updated the shape. I think we should have a better more solid way to define the comparable.

Also updating this will kind of invalidate existing stored shapes if using shape status backup, but not quite sure what the best way to do that is.

I've invalidated it by bumping the shape status backup version so this works now.

@msfstef
msfstef requested a review from icehaunter November 20, 2025 17:58
@codecov

codecov Bot commented Nov 20, 2025 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 67.78%. Comparing base (a672d2a) to head (b6a3ff5).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
packages/sync-service/lib/electric/shapes/shape.ex 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3486      +/-   ##
==========================================
+ Coverage   67.74%   67.78%   +0.04%     
==========================================
  Files         184      184              
  Lines        9943     9943              
  Branches      408      404       -4     
==========================================
+ Hits         6736     6740       +4     
+ Misses       3205     3201       -4     
  Partials        2        2              
Flag Coverage Δ
elixir 63.94% <0.00%> (+0.04%) ⬆️
elixir-client 74.47% <ø> (+0.52%) ⬆️
packages/experimental 87.73% <ø> (ø)
packages/react-hooks 86.48% <ø> (ø)
packages/typescript-client 93.07% <ø> (ø)
packages/y-electric 55.12% <ø> (ø)
postgres-140000 62.55% <0.00%> (ø)
postgres-150000 62.75% <0.00%> (+0.07%) ⬆️
postgres-170000 62.71% <0.00%> (+0.06%) ⬆️
postgres-180000 62.71% <0.00%> (+0.06%) ⬆️
sync-service 62.88% <0.00%> (ø)
typescript 87.45% <ø> (ø)
unit-tests 67.78% <0.00%> (+0.04%) ⬆️

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.

@magnetised magnetised left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wat no test? Could we have some property test that mutates all the keys and asserts the comparable-ness?

I'm also really nervous about this need to keep updating @backup_version... At least some scary comment in Shape.comparable about the dependency (or could we automate it? Calculate the version from the comparable fields or something?)

@balegas

balegas commented Nov 21, 2025

Copy link
Copy Markdown
Contributor

Tests are already broken so they are doing their work :D. Claude-generated fix incoming

@pkg-pr-new

pkg-pr-new Bot commented Nov 21, 2025 •

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@electric-sql/react@3486
npm i https://pkg.pr.new/@electric-sql/client@3486
npm i https://pkg.pr.new/@electric-sql/y-electric@3486

commit: b6a3ff5

@netlify

netlify Bot commented Nov 21, 2025 •

Copy link
Copy Markdown

✅ Deploy Preview for electric-next ready!

Name Link
🔨 Latest commit b6a3ff5
🔍 Latest deploy log https://app.netlify.com/projects/electric-next/deploys/692422d250fa550009d211f2
😎 Deploy Preview https://deploy-preview-3486--electric-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@balegas

balegas commented Nov 21, 2025

Copy link
Copy Markdown
Contributor

I've pushed a fix for the tests. When deleting a shape we were always passing table, but now since we also have the log mode in the comparison the shape would not match. We could pass the log mode explicitly, but it seems that when we have a shape handle there is no good reason for passing the shape definition in the delete, so I just stopped passing it.

@msfstef

msfstef commented Nov 22, 2025

Copy link
Copy Markdown
Contributor Author

Wat no test? Could we have some property test that mutates all the keys and asserts the comparable-ness?

Sorry yeah opened the PR last thing before leaving haha just for reference of what the fix is - property test was exactly what I was thinking as well

I'm also really nervous about this need to keep updating @backup_version... At least some scary comment in Shape.comparable about the dependency (or could we automate it? Calculate the version from the comparable fields or something?)

I'm updating it even without releasing electric just for the sake of consistency of the commits and the need to update it. I agree that automating it somehow would be better. Either way recovery right now is broken anyway (see #3482)

I have an idea to not rely on shape comparable in general but will elaborate when back

@balegas
balegas merged commit 2e19332 into main Nov 24, 2025
47 checks passed
@balegas
balegas deleted the msfstef/add-log-mode branch November 24, 2025 11: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.

Bug Report: Shape Cache Collision Between Different log_mode Values

3 participants