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.

Sign up for GitHub

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

Appview v2: don't apply missing modlists #2122

Merged
merged 5 commits into from
Feb 2, 2024
+67 −21
Merged

Appview v2: don't apply missing modlists #2122

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2457391
dont apply missing mod lists
dholms Feb 2, 2024
add0cc8
update mock dataplane
dholms Feb 2, 2024
ad9233e
Update packages/bsky/src/hydration/hydrator.ts
dholms Feb 2, 2024
9cdc882
refactor & document a bit better
dholms Feb 2, 2024
b86ba6e
fix up other routes
dholms Feb 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions packages/bsky/src/data-plane/server/routes/relationships.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ export default (db: Database): Partial<ServiceImpl<typeof Service>> => ({
db.db
.selectFrom('list_item')
.innerJoin('list_mute', 'list_mute.listUri', 'list_item.listUri')
.innerJoin('list', 'list.uri', 'list_item.listUri')
.where('list.purpose', '=', 'app.bsky.graph.defs#modlist')
.where('list_mute.mutedByDid', '=', actorDid)
.whereRef('list_item.subjectDid', '=', ref('actor.did'))
.select('list_item.listUri')
Expand All @@ -47,17 +45,13 @@ export default (db: Database): Partial<ServiceImpl<typeof Service>> => ({
db.db
.selectFrom('list_item')
.innerJoin('list_block', 'list_block.subjectUri', 'list_item.listUri')
.innerJoin('list', 'list.uri', 'list_item.listUri')
.where('list.purpose', '=', 'app.bsky.graph.defs#modlist')
.where('list_block.creator', '=', actorDid)
.whereRef('list_item.subjectDid', '=', ref('actor.did'))
.select('list_item.listUri')
.as('blockingByList'),
db.db
.selectFrom('list_item')
.innerJoin('list_block', 'list_block.subjectUri', 'list_item.listUri')
.innerJoin('list', 'list.uri', 'list_item.listUri')
.where('list.purpose', '=', 'app.bsky.graph.defs#modlist')
.where('list_item.subjectDid', '=', actorDid)
.whereRef('list_block.creator', '=', ref('actor.did'))
.select('list_item.listUri')
Expand Down
32 changes: 32 additions & 0 deletions packages/bsky/src/hydration/hydrator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ export class Hydrator {
listUris.push(...listUrisFromProfileViewer(item))
})
const listState = await this.hydrateListsBasic(listUris, viewer)
// if a list no longer exists or is not a mod list, then remove from viewer state
profileViewers?.forEach((item) => {
removeNonModListsFromProfileViewer(item, listState)
})
Comment on lines +106 to +109
Copy link
Collaborator

@devinivy devinivy Feb 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm a little torn on modifying the ProfileViewers in this way. There are implicit dependencies between getProfileViewerStates(), this logic, and mute/block evaluation. We could also handle it on the flip side, when blocks are being evaluated, in viewerBlockExists() and viewerMuteExists(), and in some ways I think that fits better into how hydration vs. view logic currently works. Either case introduces some dependencies, so I'd be for leaving another comment or two to document those. That would be either:

  • documenting in getProfileViewerStates() the dependence on a later call to removeNonModListsFromProfileViewer() for evaluation of blocks/mutes or
  • documenting in viewerMute/BlockExists() that referenced lists must be hydrated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah i thought about doing it in the view layer

but I think there's a decent argument that this should be handled in hydration. in v1, we were doing this in hydration & the lists on ProfileViewers don't actually "exist". Imo this isn't "view logic", it's an additional piece of logic on hydration that would ideally be handled in the hydration query but isn't because everything is denormalized & this is expensive to materialize 🤔

Def down to rework logic/document it better

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree it would be more ideal for this to be in getProfileViewerStates but then it mixes it up with list logic which is kinda pretty strange because we have to jam a list hydration call in there...

maybe we could add a higher level getProfilerViewerStates to the hydrator that handles this logic

and have a lower level getUncheckedProfileViewerStates on the profile hydrator that calls out the response is incomplete

return mergeStates(listState, {
actors,
labels,
Expand Down Expand Up @@ -563,9 +567,37 @@ const listUrisFromProfileViewer = (item: ProfileViewerState | null) => {
if (item?.blockingByList) {
listUris.push(item.blockingByList)
}
// blocked-by list does not appear in views, but will be used to evaluate the existence of a block between users.
if (item?.blockedByList) {
listUris.push(item.blockedByList)
}
dholms marked this conversation as resolved.
Show resolved Hide resolved
return listUris
}

const removeNonModListsFromProfileViewer = (
item: ProfileViewerState | null,
state: HydrationState,
) => {
if (!isModList(item?.mutedByList, state)) {
delete item?.mutedByList
}
if (!isModList(item?.blockingByList, state)) {
delete item?.blockingByList
}
if (!isModList(item?.blockedByList, state)) {
delete item?.blockedByList
}
}

const isModList = (
listUri: string | undefined,
state: HydrationState,
): boolean => {
if (!listUri) return false
const list = state.lists?.get(listUri)
return list?.record.purpose === 'app.bsky.graph.defs#modlist'
}

const labelSubjectsForDid = (dids: string[]) => {
return [
...dids,
Expand Down
Loading