-
Notifications
You must be signed in to change notification settings - Fork 2
Add local likes & history to cloud after signing in #242
base: main
Are you sure you want to change the base?
Conversation
Deploying with Cloudflare Pages
|
Codecov Report
@@ Coverage Diff @@
## main #242 +/- ##
==========================================
- Coverage 60.74% 60.56% -0.19%
==========================================
Files 75 75
Lines 670 672 +2
Branches 200 200
==========================================
Hits 407 407
- Misses 250 252 +2
Partials 13 13
Continue to review full report at Codecov.
|
@@ -1,5 +1,8 @@ | |||
import { snackbar } from '$lib/shared/ui/snackbar'; | |||
import { supabaseClient } from '$lib/shared/api'; | |||
import { likesStore, addCloudLike } from '$lib/features/like-episode'; | |||
import { listeningHistory, addToCloudListeningHistory } from '$lib/features/listening-history'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: FSD, cross-imports of features
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as #241 (comment)
@@ -14,5 +17,7 @@ export function signInSnackbar() { | |||
}); | |||
url.searchParams.delete('snackbar'); | |||
window.history.replaceState(null, '', url.toString()); | |||
get(likesStore).forEach(addCloudLike); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: why did you decide to do it here, of all the places?)
suggestion: the same as in that other PR, let's do it here:
cast/src/lib/features/like-episode/model/like.ts
Lines 13 to 18 in bd8fc73
user.subscribe(async ($user) => { | |
if ($user) { | |
const ids = await fetchLikes(); | |
likesStore.set(ids ?? []); | |
} | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I didn't want to add the like to Supabase every time the user loads the page (I'm not sure if there are any uniqueness constraints set in the backend). This seemed like the only function where I guarantee the user has just signed in after being logged out before and having added some episode to the favorites.
I agree it's not the best solution, especially that it only works if the user signed in through the snackbar that appears after liking an episode unauthenticated, but I also couldn't think of another quick solution that doesn't involve querying the backend likes/history and diffing the data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are uniqueness constraints on the data, so uploading a duplicate like/history won't really change anything except for the created_at
timestamp, which we don't use anyway, except for ordering.
Technically, we could have another derived
store that tracks the previous value of the user
store, and expose to the public yet another derived
store that provides access to them both, however, that is slightly janky, so proceed at will
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, I would prefer not to depend on constraints in the backend, especially that the function name literally says "add". It's better to validate the data on the client side first and consider the backend validation as a safety measure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, then diffing on the client it is. Shouldn't be too expensive/hard
Closes #235