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

suggestions #7382

Merged
merged 2 commits into from
Jan 7, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
62 changes: 32 additions & 30 deletions src/components/MediaPreview.tsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
import React from 'react'
import {StyleProp, StyleSheet, View, ViewStyle} from 'react-native'
import {Image} from 'expo-image'
import {AppBskyFeedDefs} from '@atproto/api'
import {
AppBskyEmbedExternal,
AppBskyEmbedImages,
AppBskyEmbedRecordWithMedia,
AppBskyEmbedVideo,
} from '@atproto/api'
import {$Typed} from '@atproto/api/dist/client/util'
import {Trans} from '@lingui/macro'

import {parseTenorGif} from '#/lib/strings/embed-player'
import {isTenorGifUri} from '#/lib/strings/embed-player'
import {atoms as a, useTheme} from '#/alf'
import {MediaInsetBorder} from '#/components/MediaInsetBorder'
import {Text} from '#/components/Typography'
import {PlayButtonIcon} from '#/components/video/PlayButtonIcon'
import {parseEmbed} from '#/types/atproto/post'

/**
* Streamlined MediaPreview component which just handles images, gifs, and videos
Expand All @@ -18,17 +23,22 @@ export function Embed({
embed,
style,
}: {
embed: AppBskyFeedDefs.PostView['embed']
embed?:
| $Typed<AppBskyEmbedRecordWithMedia.View>
| $Typed<AppBskyEmbedImages.View>
| $Typed<AppBskyEmbedVideo.View>
| $Typed<AppBskyEmbedExternal.View>
| {$type: string}
Copy link
Member

Choose a reason for hiding this comment

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

I get why this makes sense and is probably the most academic way of doing this, but a few things I don't like:

  • We lose the relationship between the prop embed and the origin of the data in PostView['embed']. The way it was typed before makes it very obvious how this component is used
  • Having to remember to duplicate the forwards compat via {$type: string} in components like this is boilerplate I don't think we want. If we adopt this pattern, we'll be doing this in hundreds of places.
  • Having to reconstruct these types exactly but understanding exactly what can be passed in here (plus needing the $Type util) requires understanding of the data on both sides of this component. This makes it more cumbersome to make "dumb" components that can be composed and reused throughout the app.

style?: StyleProp<ViewStyle>
}) {
const e = parseEmbed(embed)

if (!e) return null

if (e.type === 'images') {
if (!embed) {
return null
} else if (AppBskyEmbedRecordWithMedia.isView(embed)) {
return <Embed embed={embed.media} style={style} />
} else if (AppBskyEmbedImages.isView(embed)) {
return (
<Outer style={style}>
{e.view.images.map(image => (
{embed.images.map(image => (
<ImageItem
key={image.thumb}
thumbnail={image.thumb}
Expand All @@ -37,32 +47,24 @@ export function Embed({
))}
</Outer>
)
} else if (e.type === 'link' && e.view.external.thumb) {
let url: URL | undefined
try {
url = new URL(e.view.external.uri)
} catch {}
if (url) {
const {success} = parseTenorGif(url)
if (success) {
return (
<Outer style={style}>
<GifItem
thumbnail={e.view.external.thumb}
alt={e.view.external.title}
/>
</Outer>
)
}
}
} else if (e.type === 'video') {
} else if (AppBskyEmbedExternal.isView(embed)) {
if (!embed.external.thumb) return null
if (!isTenorGifUri(embed.external.uri)) return null

return (
<Outer style={style}>
<GifItem thumbnail={embed.external.thumb} alt={embed.external.title} />
</Outer>
)
} else if (AppBskyEmbedVideo.isView(embed)) {
return (
<Outer style={style}>
<VideoItem thumbnail={e.view.thumbnail} alt={e.view.alt} />
<VideoItem thumbnail={embed.thumbnail} alt={embed.alt} />
</Outer>
)
}

// media is {$type: string}
return null
}

Expand Down
38 changes: 18 additions & 20 deletions src/components/interstitials/Trending.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -59,26 +59,24 @@ export function Inner() {
) : !trending?.topics ? null : (
<>
{trending.topics.map(topic => (
<>
<TrendingTopicLink
key={topic.link}
topic={topic}
onPress={() => {
logEvent('trendingTopic:click', {context: 'interstitial'})
}}>
<View style={[a.py_lg]}>
<Text
style={[
t.atoms.text,
a.text_sm,
a.font_bold,
{opacity: 0.7}, // NOTE: we use opacity 0.7 instead of a color to match the color of the home pager tab bar
]}>
{topic.topic}
</Text>
</View>
</TrendingTopicLink>
</>
<TrendingTopicLink
key={topic.link}
topic={topic}
onPress={() => {
logEvent('trendingTopic:click', {context: 'interstitial'})
}}>
<View style={[a.py_lg]}>
<Text
style={[
t.atoms.text,
a.text_sm,
a.font_bold,
{opacity: 0.7}, // NOTE: we use opacity 0.7 instead of a color to match the color of the home pager tab bar
]}>
{topic.topic}
</Text>
</View>
</TrendingTopicLink>
))}
<Button
label={_(msg`Hide trending topics`)}
Expand Down
9 changes: 9 additions & 0 deletions src/lib/strings/embed-player.ts
Original file line number Diff line number Diff line change
Expand Up @@ -568,3 +568,12 @@ export function parseTenorGif(urlp: URL):
dimensions,
}
}

export function isTenorGifUri(url: URL | string) {
try {
return parseTenorGif(typeof url === 'string' ? new URL(url) : url).success
} catch {
// Invalid URL
return false
}
}
6 changes: 1 addition & 5 deletions src/screens/Messages/components/MessageInputEmbed.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,7 @@ export function MessageInputEmbed({
)

const {rt, record} = useMemo(() => {
if (
post &&
AppBskyFeedPost.isRecord(post.record) &&
AppBskyFeedPost.validateRecord(post.record).success
) {
if (post && AppBskyFeedPost.isValidRecord(post.record)) {
return {
rt: new RichTextAPI({
text: post.record.text,
Expand Down
4 changes: 3 additions & 1 deletion src/screens/StarterPack/Wizard/State.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ interface State {
currentStep: Step
name?: string
description?: string
profiles: AppBskyActorDefs.ProfileViewBasic[]
profiles: Array<
AppBskyActorDefs.ProfileViewBasic | AppBskyActorDefs.ProfileView
>
feeds: GeneratorView[]
processing: boolean
error?: string
Expand Down
22 changes: 10 additions & 12 deletions src/state/queries/notifications/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {QueryClient} from '@tanstack/react-query'
import chunk from 'lodash.chunk'

import {labelIsHideableOffense} from '#/lib/moderation'
import * as atp from '#/types/atproto'
import {precacheProfile} from '../profile'
import {FeedNotification, FeedPage, NotificationType} from './types'

Expand Down Expand Up @@ -256,18 +255,17 @@ function getSubjectUri(
return notif.uri
} else if (type === 'post-like' || type === 'repost') {
if (
atp.fastIsType<AppBskyFeedRepost.Record>(
notif.record,
AppBskyFeedRepost.isRecord,
) ||
atp.fastIsType<AppBskyFeedLike.Record>(
notif.record,
AppBskyFeedLike.isRecord,
)
AppBskyFeedRepost.isRecord(notif.record) ||
AppBskyFeedLike.isRecord(notif.record)
) {
return typeof notif.record.subject?.uri === 'string'
? notif.record.subject?.uri
: undefined
// Type casting here is actually safe because here we check for the
// actual type bellow
const record = notif.record as
| AppBskyFeedRepost.Record
| AppBskyFeedLike.Record
Copy link
Member

Choose a reason for hiding this comment

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

Totally! This is actually what fastIsType is doing as well, just casting the values without validation. I decided to make the util so that it's clear what's happening when that util is used, instead of feeling the need to leave a comment on each type cast as you did here. That seem reasonable to you?


const uri = record.subject?.uri
return typeof uri === 'string' ? uri : undefined
}
} else if (type === 'feedgen-like') {
return notif.reasonSubject
Expand Down
19 changes: 8 additions & 11 deletions src/state/queries/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,9 @@ import {
AppBskyFeedPost,
AtUri,
} from '@atproto/api'
import {$Typed} from '@atproto/api/dist/client/util'
import {InfiniteData, QueryClient, QueryKey} from '@tanstack/react-query'

import * as atp from '#/types/atproto'

export async function truncateAndInvalidate<T = any>(
queryClient: QueryClient,
queryKey: QueryKey,
Expand Down Expand Up @@ -44,22 +43,20 @@ export function didOrHandleUriMatches(
}

export function getEmbeddedPost(
v: unknown,
v:
| undefined
| $Typed<AppBskyEmbedRecord.View>
| $Typed<AppBskyEmbedRecordWithMedia.View>
| {$type: string},
): AppBskyEmbedRecord.ViewRecord | undefined {
if (atp.fastIsType<AppBskyEmbedRecord.View>(v, AppBskyEmbedRecord.isView)) {
if (AppBskyEmbedRecord.isView(v)) {
if (
AppBskyEmbedRecord.isViewRecord(v.record) &&
AppBskyFeedPost.isRecord(v.record.value)
) {
return v.record
}
}
if (
atp.fastIsType<AppBskyEmbedRecordWithMedia.View>(
v,
AppBskyEmbedRecordWithMedia.isView,
)
) {
} else if (AppBskyEmbedRecordWithMedia.isView(v)) {
if (
AppBskyEmbedRecord.isViewRecord(v.record.record) &&
AppBskyFeedPost.isRecord(v.record.record.value)
Expand Down
4 changes: 2 additions & 2 deletions src/types/atproto/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ export * as profile from '#/types/atproto/profile'
* }
* ```
*/
export function fastIsType<R>(
export function fastIsType<R extends {$type?: string}>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change prevents using fastIsType with a type param R that does not match the predicate function.

record: unknown,
identity: <V>(v: V) => boolean,
identity: <V>(v: V) => v is V & {$type: NonNullable<R['$type']>},
Comment on lines +19 to +21
Copy link
Member

Choose a reason for hiding this comment

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

Oh v nice!

): record is R {
return identity(record)
}
Loading
Loading