Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate local thread mutes #4523

Merged
merged 7 commits into from
Jun 18, 2024
Merged

Migrate local thread mutes #4523

merged 7 commits into from
Jun 18, 2024

Conversation

mozzius
Copy link
Member

@mozzius mozzius commented Jun 14, 2024

Stacked on #4518

Simple. Uploads existing thread mutes then deletes them.

Does not account for multi-accounts, but I'm not sure if there is a neat solution here without making this far more complex than necessary

Copy link

render bot commented Jun 14, 2024

Copy link

github-actions bot commented Jun 14, 2024

Old size New size Diff
6.29 MB 6.29 MB 0 B (0.00%)

function migrateThreadMutes(agent: BskyAgent) {
const threadMutes = persisted.get('mutedThreads')
if (threadMutes.length > 0) {
console.log('migrating', threadMutes.length, 'thread mutes')
Copy link
Collaborator

Choose a reason for hiding this comment

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

log

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

if you have a thousand (as some older heavy users might), i think this version will fire off a thousand concurrent requests. would that result in a rate limit?

not sure how to solve but let's think about what we want to happen in this case

@mary-ext
Copy link
Contributor

mary-ext commented Jun 15, 2024

Might be best to migrate the newest mutes first, and to update the array as the migration progresses? (if there's a lot of mutes then it's possible that the migration won't be finished by the time the user's done with the app and closes it)

There's some concern around web being able to open multiple tabs...

@mozzius mozzius force-pushed the samuel/mute-thread-migration branch from ccf6a4f to 8990cc8 Compare June 17, 2024 15:15
// not a big deal if this fails, since the post might have been deleted
.catch(console.error)

persisted.write('mutedThreads', threads)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are multiple browser tabs going to fight with each other and try to sync this value with each other? I wonder if we should just skip the "broadcast" here. cc @estrattonbailey for thoughts

src/state/persisted/schema.ts Outdated Show resolved Hide resolved
src/state/queries/post.ts Outdated Show resolved Hide resolved
Comment on lines 320 to 323
onSuccess(finalIsMuted) {
// finalize
setThreadMute(rootUri, finalIsMuted)
},
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to handle the failure case and reset, or nah?

Copy link
Member Author

Choose a reason for hiding this comment

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

This resets it, just without feedback. Should I add feedback?


if (!root) break

persisted.write('mutedThreads', threads)
Copy link
Member

Choose a reason for hiding this comment

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

We could add a param here to not emit an update. But also, if a user reloads the app with 4 tabs open, this migration is going to fire 4 times, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

The list will be processed in parallel by each active tab. It's fine if there's data races though, nothing happens if you mute the same thread twice. Also, writing to persisted doesn't refire the effect, so it doesn't cause multiple migrations or something

cancelled = true
}
}
}, [agent, currentAccount, setThreadMute])
Copy link
Member

Choose a reason for hiding this comment

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

I don't think agent or currentAccount can change here, but we might want to protect against this hook being called more than once?

Copy link
Member Author

Choose a reason for hiding this comment

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

The loop cancels itself, so it's fine for the effect to fire multiple times

@mozzius mozzius force-pushed the samuel/mute-thread-migration branch from c7779f1 to a5e7cfb Compare June 18, 2024 14:21
@mozzius mozzius force-pushed the samuel/mute-thread-migration branch from 59ce2b8 to 28d52a8 Compare June 18, 2024 20:11
Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

lgtm

@gaearon gaearon merged commit 0012d12 into main Jun 18, 2024
6 checks passed
@gaearon gaearon deleted the samuel/mute-thread-migration branch June 18, 2024 21:06
estrattonbailey added a commit that referenced this pull request Jun 18, 2024
* origin/main: (35 commits)
  Bump labeler limit to 20 (#4565)
  Migrate local thread mutes (#4523)
  Disable newskie dialog tap in hover card web (#4562)
  Implement thread locking (#4545)
  Prevent unecessary calls (#4561)
  Force callers of `getTimeAgo` to pass in the value for "now" (#4560)
  Fix: only apply self-thread load-more behavior on the outer edge of the reply tree (#4559)
  Server-side thread mutes (#4518)
  Explore fixes (#4540)
  Is it "newskie" or "newsky" 🤔  (#4557)
  fix keyboard overlaying onboarding inputs (#4558)
  Add `useGetTimeAgo` and utils (#4556)
  Unconditionally polyfill Intl.PluralRules for native (#4554)
  Dedupe Zod installation (#4551)
  Use exact imports for icons (#4549)
  Fix Android startup perf regression (#4544)
  Explore feed cards (#4521)
  Onboarding fixes (#4508)
  Add `native_pwi_disabled` feature gate experiment (#4507)
  Select, don't mutate (#4541)
  ...
estrattonbailey added a commit that referenced this pull request Jun 20, 2024
* origin/main: (62 commits)
  Rework "Who can reply" to blend more nicely into the UI (#4578)
  Fix threadgate read after write (#4577)
  Convert button to use forwardRef (#4576)
  use 1000x1000 for image height in avatar cropper (#4453)
  fix for autofill covering border (#4573)
  Update HomeHeaderLayoutMobile.tsx (#4572)
  Option for large alt badges (#4571)
  Truncate post metrics and fix truncation on native (#4575)
  Fix avi placeholder layout (#4570)
  add support for `ListEmptyComponent`, allow `undefined` data (#4403)
  GIF previews in notifications (#4447)
  [Session] Convert account to session data explicitly (#4446)
  Move onboarding start to after successfull account creation (#4381)
  Collection of moderation fixes (#4566)
  Fix undefined block (#4479)
  fix gap between tab bar and its border (#4538)
  Better handling of blocks in `KnownFollowers` (#4563)
  Verify email reminders (#4510)
  Bump labeler limit to 20 (#4565)
  Migrate local thread mutes (#4523)
  ...
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.

4 participants