From a75d3f0bdf40198c6eb0f0154e53f193e7c98850 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 24 Aug 2026 15:16:13 -0400 Subject: [PATCH 01/17] feat(chat): preview link unfurls before sending Paste a link in the composer and its unfurl card now appears above the input before the message is sent. Dismissing a card with the X suppresses that link's unfurl for that send. New chat1.local.unfurlPreviewLocal RPC runs the existing extractor, scraper and packager over unsent text and returns display-ready unfurls, so the preview reuses the same scrape and asset caches the post-send unfurl path already fills via Prefetch. Only whitelisted domains preview, and only generic unfurls; maps are excluded to match the message card. Suppression rides along on postTextNonblock as unfurlSuppress. The service stashes it against the message's outbox ID in a bounded LRU (200 entries, 5 minute TTL) and UnfurlAndSend skips those URLs. The entry is read without consuming so a second UnfurlAndSend, as happens when the user resolves an unfurl prompt on another link in the same message, still honors the dismissal. The composer snapshots the dismissed URLs before clearing the input rather than reading them at send time: clearing the input synchronously fires the draft throttle's leading edge, which drops the dismissed set before a setTimeout-scheduled send would have read it. A stellar payment cancel restores the set along with the text. Desktop only for now; the service side is platform neutral. --- go/chat/server.go | 14 ++ go/chat/types/interfaces.go | 3 + go/chat/types/types.go | 8 + go/chat/unfurl/cache.go | 31 +++- go/chat/unfurl/unfurler.go | 97 +++++++++- go/chat/unfurl/unfurler_test.go | 115 ++++++++++++ go/protocol/chat1/local.go | 39 ++++ protocol/avdl/chat1/local.avdl | 10 +- protocol/bin/enabled-calls.json | 1 + protocol/json/chat1/local.json | 38 ++++ .../input-area/input-state.test.tsx | 111 +++++++++++- .../conversation/input-area/input-state.tsx | 5 +- .../conversation/input-area/normal/index.tsx | 48 +++-- .../input-area/unfurl-preview.test.tsx | 68 +++++++ .../input-area/unfurl-preview.tsx | 60 +++++++ .../text/unfurl/unfurl-list/generic-view.tsx | 167 ++++++++++++++++++ .../text/unfurl/unfurl-list/generic.tsx | 161 ++--------------- shared/chat/conversation/send-actions.tsx | 29 ++- .../unfurl-preview-state.test.tsx | 135 ++++++++++++++ .../conversation/unfurl-preview-state.tsx | 149 ++++++++++++++++ shared/constants/rpc/rpc-chat-gen.tsx | 10 +- shared/tsconfig.native.json | 1 + 22 files changed, 1114 insertions(+), 186 deletions(-) create mode 100644 shared/chat/conversation/input-area/unfurl-preview.test.tsx create mode 100644 shared/chat/conversation/input-area/unfurl-preview.tsx create mode 100644 shared/chat/conversation/messages/text/unfurl/unfurl-list/generic-view.tsx create mode 100644 shared/chat/conversation/unfurl-preview-state.test.tsx create mode 100644 shared/chat/conversation/unfurl-preview-state.tsx diff --git a/go/chat/server.go b/go/chat/server.go index 9e3f9ac1c931..801cf11aa9c2 100644 --- a/go/chat/server.go +++ b/go/chat/server.go @@ -1005,6 +1005,10 @@ func (h *Server) PostTextNonblock(ctx context.Context, arg chat1.PostTextNonbloc }) } + if len(arg.UnfurlSuppress) > 0 && arg.OutboxID != nil { + h.G().Unfurler.SetSuppressed(ctx, *arg.OutboxID, arg.UnfurlSuppress) + } + var parg chat1.PostLocalNonblockArg parg.SessionID = arg.SessionID parg.ClientPrev = arg.ClientPrev @@ -1611,6 +1615,16 @@ func (h *Server) UpdateUnsentText(ctx context.Context, arg chat1.UpdateUnsentTex return nil } +func (h *Server) UnfurlPreviewLocal(ctx context.Context, arg chat1.UnfurlPreviewLocalArg) (res []chat1.UnfurlPreviewInfo, err error) { + ctx = globals.ChatCtx(ctx, h.G(), keybase1.TLFIdentifyBehavior_CHAT_GUI, nil, h.identNotifier) + defer h.Trace(ctx, &err, "UnfurlPreviewLocal")() + uid, err := utils.AssertLoggedInUID(ctx, h.G()) + if err != nil { + return nil, err + } + return h.G().Unfurler.PreviewURLs(ctx, uid, arg.ConvID, arg.Text), nil +} + func (h *Server) UpdateTyping(ctx context.Context, arg chat1.UpdateTypingArg) (err error) { var identBreaks []keybase1.TLFIdentifyFailure ctx = globals.ChatCtx(ctx, h.G(), keybase1.TLFIdentifyBehavior_CHAT_GUI, diff --git a/go/chat/types/interfaces.go b/go/chat/types/interfaces.go index b19baf5f3901..3482edf0318a 100644 --- a/go/chat/types/interfaces.go +++ b/go/chat/types/interfaces.go @@ -506,6 +506,9 @@ type Unfurler interface { UnfurlAndSend(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, msg chat1.MessageUnboxed) Prefetch(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, msgText string) int + PreviewURLs(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, + text string) []chat1.UnfurlPreviewInfo + SetSuppressed(ctx context.Context, outboxID chat1.OutboxID, urls []string) Status(ctx context.Context, outboxID chat1.OutboxID) (UnfurlerTaskStatus, *chat1.UnfurlResult, error) Retry(ctx context.Context, outboxID chat1.OutboxID) Complete(ctx context.Context, outboxID chat1.OutboxID) diff --git a/go/chat/types/types.go b/go/chat/types/types.go index c9121540a9ef..65f027e7cd2b 100644 --- a/go/chat/types/types.go +++ b/go/chat/types/types.go @@ -588,6 +588,14 @@ func (d DummyUnfurler) Prefetch(ctx context.Context, uid gregor1.UID, convID cha return 0 } +func (d DummyUnfurler) PreviewURLs(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, + text string, +) []chat1.UnfurlPreviewInfo { + return nil +} + +func (d DummyUnfurler) SetSuppressed(ctx context.Context, outboxID chat1.OutboxID, urls []string) {} + func (d DummyUnfurler) Status(ctx context.Context, outboxID chat1.OutboxID) (UnfurlerTaskStatus, *chat1.UnfurlResult, error) { return UnfurlerTaskStatusFailed, nil, nil } diff --git a/go/chat/unfurl/cache.go b/go/chat/unfurl/cache.go index e45cb8d990fc..bc258b625d60 100644 --- a/go/chat/unfurl/cache.go +++ b/go/chat/unfurl/cache.go @@ -12,6 +12,15 @@ import ( const ( defaultCacheLifetime = 10 * time.Minute defaultCacheSize = 1000 + + // suppressedCacheLifetime/Size bound the store of per-send unfurl + // suppressions: a message about to send holds at most a handful of + // dismissed URLs, and the entry is only relevant for as long as the + // send is in flight, so a small cap and a short TTL are enough to + // prevent an unconsumed entry (failed/aborted send) from leaking for + // the life of the process. + suppressedCacheLifetime = 5 * time.Minute + suppressedCacheSize = 200 ) type cacheItem struct { @@ -21,18 +30,24 @@ type cacheItem struct { type unfurlCache struct { sync.Mutex - cache *lru.Cache - clock clockwork.Clock + cache *lru.Cache + clock clockwork.Clock + lifetime time.Duration } func newUnfurlCache() *unfurlCache { - cache, err := lru.New(defaultCacheSize) + return newUnfurlCacheWithLimits(defaultCacheSize, defaultCacheLifetime) +} + +func newUnfurlCacheWithLimits(size int, lifetime time.Duration) *unfurlCache { + cache, err := lru.New(size) if err != nil { panic(err) } return &unfurlCache{ - cache: cache, - clock: clockwork.NewRealClock(), + cache: cache, + clock: clockwork.NewRealClock(), + lifetime: lifetime, } } @@ -40,8 +55,8 @@ func (c *unfurlCache) setClock(clock clockwork.Clock) { c.clock = clock } -// get determines if the item is in the cache and newer than 10 -// minutes. We don't want to cache this value indefinitely in case the page +// get determines if the item is in the cache and newer than the cache's +// lifetime. We don't want to cache this value indefinitely in case the page // content changes. func (c *unfurlCache) get(key string) (res cacheItem, ok bool) { c.Lock() @@ -55,7 +70,7 @@ func (c *unfurlCache) get(key string) (res cacheItem, ok bool) { if !ok { return res, false } - valid := c.clock.Now().Sub(cacheItem.ctime.Time()) <= defaultCacheLifetime + valid := c.clock.Now().Sub(cacheItem.ctime.Time()) <= c.lifetime if !valid { c.cache.Remove(key) } diff --git a/go/chat/unfurl/unfurler.go b/go/chat/unfurl/unfurler.go index 34e128dde240..17dab1859eda 100644 --- a/go/chat/unfurl/unfurler.go +++ b/go/chat/unfurl/unfurler.go @@ -13,6 +13,7 @@ import ( "github.com/keybase/client/go/chat/s3" "github.com/keybase/client/go/chat/storage" "github.com/keybase/client/go/chat/types" + "github.com/keybase/client/go/chat/unfurl/display" "github.com/keybase/client/go/chat/utils" "github.com/keybase/client/go/libkb" "github.com/keybase/client/go/protocol/chat1" @@ -52,12 +53,13 @@ type Unfurler struct { globals.Contextified utils.DebugLabeler - unfurlMap map[string]bool - extractor *Extractor - scraper *Scraper - packager *Packager - settings *Settings - sender UnfurlMessageSender + unfurlMap map[string]bool + suppressed *unfurlCache + extractor *Extractor + scraper *Scraper + packager *Packager + settings *Settings + sender UnfurlMessageSender // testing unfurlCh chan *chat1.Unfurl @@ -77,6 +79,7 @@ func NewUnfurler(g *globals.Context, store attachments.Store, s3signer s3.Signer Contextified: globals.NewContextified(g), DebugLabeler: utils.NewDebugLabeler(g.ExternalG(), "Unfurler", false), unfurlMap: make(map[string]bool), + suppressed: newUnfurlCacheWithLimits(suppressedCacheSize, suppressedCacheLifetime), extractor: extractor, scraper: scraper, packager: packager, @@ -88,6 +91,7 @@ func NewUnfurler(g *globals.Context, store attachments.Store, s3signer s3.Signer func (u *Unfurler) SetClock(clock clockwork.Clock) { u.scraper.cache.setClock(clock) u.packager.cache.setClock(clock) + u.suppressed.setClock(clock) } func (u *Unfurler) SetTestingRetryCh(ch chan struct{}) { @@ -234,6 +238,40 @@ func (u *Unfurler) makeBaseUnfurlMessage(ctx context.Context, fromMsg chat1.Mess return msg, nil } +// SetSuppressed records URLs the sender chose not to unfurl for the message +// with this outbox ID. The entry is not consumed by UnfurlAndSend: a message +// can be unfurled more than once (accepting an unfurl prompt re-runs +// UnfurlAndSend on the same message), and a consumed entry would let a +// dismissed URL unfurl on that second pass. It expires after +// suppressedCacheLifetime instead, which also keeps an entry left behind by +// a send that never reaches UnfurlAndSend from lingering for the life of the +// process. +func (u *Unfurler) SetSuppressed(ctx context.Context, outboxID chat1.OutboxID, urls []string) { + if len(urls) == 0 { + return + } + m := make(map[string]bool, len(urls)) + for _, url := range urls { + m[url] = true + } + u.suppressed.put(outboxID.String(), m) +} + +func (u *Unfurler) getSuppressed(outboxID *chat1.OutboxID) map[string]bool { + if outboxID == nil { + return nil + } + item, valid := u.suppressed.get(outboxID.String()) + if !valid { + return nil + } + m, ok := item.data.(map[string]bool) + if !ok { + return nil + } + return m +} + func (u *Unfurler) UnfurlAndSend(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, msg chat1.MessageUnboxed, ) { @@ -248,6 +286,7 @@ func (u *Unfurler) UnfurlAndSend(ctx context.Context, uid gregor1.UID, convID ch if len(hits) == 0 { return } + suppressed := u.getSuppressed(msg.Valid().ClientHeader.OutboxID) // get a map for all the URLs we have already unfurled prevUnfurled := make(map[string]bool) for _, u := range msg.Valid().Unfurls { @@ -255,6 +294,10 @@ func (u *Unfurler) UnfurlAndSend(ctx context.Context, uid gregor1.UID, convID ch } // for each hit, either prompt the user for action, or generate a new message for _, hit := range hits { + if suppressed[hit.URL] { + u.Debug(ctx, "UnfurlAndSend: skipping suppressed URL") + continue + } if prevUnfurled[hit.URL] { u.Debug(ctx, "UnfurlAndSend: skipping prev unfurled") continue @@ -333,6 +376,48 @@ func (u *Unfurler) Prefetch(ctx context.Context, uid gregor1.UID, convID chat1.C return numPrefetched } +// PreviewURLs scrapes and packages the whitelisted URLs in text and returns +// display-ready unfurls, so a client can show a preview before sending. Only +// generic unfurls are returned; failures are skipped rather than returned. +func (u *Unfurler) PreviewURLs(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, + text string, +) (res []chat1.UnfurlPreviewInfo) { + defer u.Trace(ctx, nil, "PreviewURLs")() + hits, err := u.extractor.Extract(ctx, uid, convID, 0, text, u.settings) + if err != nil { + u.Debug(ctx, "PreviewURLs: failed to extract: %s", err) + return nil + } + seen := make(map[string]bool) + for _, hit := range hits { + if hit.Typ != ExtractorHitUnfurl || seen[hit.URL] { + continue + } + seen[hit.URL] = true + unfurl, err := u.scrapeAndPackage(ctx, uid, convID, hit.URL) + if err != nil { + u.Debug(ctx, "PreviewURLs: unable to scrapeAndPackage: %s", err) + continue + } + typ, err := unfurl.UnfurlType() + if err != nil || typ != chat1.UnfurlType_GENERIC { + continue + } + // a map unfurl is a generic unfurl with MapInfo set, and the message + // view refuses to render those, so don't preview them either + if unfurl.Generic().MapInfo != nil { + continue + } + disp, err := display.DisplayUnfurl(ctx, u.G().AttachmentURLSrv, convID, unfurl) + if err != nil { + u.Debug(ctx, "PreviewURLs: failed to display: %s", err) + continue + } + res = append(res, chat1.UnfurlPreviewInfo{Url: hit.URL, Unfurl: disp}) + } + return res +} + func (u *Unfurler) checkAndSetUnfurling(ctx context.Context, outboxID chat1.OutboxID) (inprogress bool) { u.Lock() defer u.Unlock() diff --git a/go/chat/unfurl/unfurler_test.go b/go/chat/unfurl/unfurler_test.go index 596da8a9165d..9ba10827237f 100644 --- a/go/chat/unfurl/unfurler_test.go +++ b/go/chat/unfurl/unfurler_test.go @@ -9,11 +9,13 @@ import ( "github.com/keybase/client/go/chat/attachments" "github.com/keybase/client/go/chat/globals" + "github.com/keybase/client/go/chat/storage" "github.com/keybase/client/go/chat/types" "github.com/keybase/client/go/externalstest" "github.com/keybase/client/go/libkb" "github.com/keybase/client/go/protocol/chat1" "github.com/keybase/client/go/protocol/gregor1" + "github.com/keybase/clockwork" "github.com/stretchr/testify/require" ) @@ -176,3 +178,116 @@ func TestUnfurler(t *testing.T) { require.ErrorAs(t, err, new(libkb.NotFoundError)) require.Equal(t, types.UnfurlerTaskStatusFailed, status) } + +func TestUnfurlerPreviewURLs(t *testing.T) { + tc := externalstest.SetupTest(t, "unfurler", 0) + defer tc.Cleanup() + g := globals.NewContext(tc.G, &globals.ChatContext{}) + + store := attachments.NewStoreTesting(g, nil) + s3signer := &ptsigner{} + g.ActivityNotifier = makeDummyActivityNotifier() + g.MessageDeliverer = dummyDeliverer{} + g.AttachmentURLSrv = types.DummyAttachmentHTTPSrv{} + sender := makeDummySender() + ri := func() chat1.RemoteInterface { return paramsRemote{} } + storage := newMemConversationBackedStorage() + unfurler := NewUnfurler(g, store, s3signer, storage, sender, ri) + + uid := gregor1.UID([]byte{0, 1}) + convID := chat1.ConversationID([]byte{0, 1, 2}) + srv := createTestCaseHTTPSrv(t) + addr := srv.Start() + defer srv.Stop() + + url := fmt.Sprintf("http://%s/?name=%s", addr, "wsj0.html") + require.NoError(t, unfurler.WhitelistAdd(context.TODO(), uid, "127.0.0.1")) + + res := unfurler.PreviewURLs(context.TODO(), uid, convID, "check this out "+url) + require.Len(t, res, 1) + require.Equal(t, url, res[0].Url) + typ, err := res[0].Unfurl.UnfurlType() + require.NoError(t, err) + require.Equal(t, chat1.UnfurlType_GENERIC, typ) + require.NotZero(t, res[0].Unfurl.Generic().Title) + + // duplicate URLs collapse to one entry + res = unfurler.PreviewURLs(context.TODO(), uid, convID, url+" and again "+url) + require.Len(t, res, 1) + + // text with no links does no work + require.Empty(t, unfurler.PreviewURLs(context.TODO(), uid, convID, "no links here")) +} + +func makeTextMsgWithOutboxID(msgBody string, outboxID chat1.OutboxID) chat1.MessageUnboxed { + return chat1.NewMessageUnboxedWithValid(chat1.MessageUnboxedValid{ + ClientHeader: chat1.MessageClientHeaderVerified{ + TlfName: "mike", + MessageType: chat1.MessageType_TEXT, + OutboxID: &outboxID, + }, + ServerHeader: chat1.MessageServerHeader{ + MessageID: 4, + }, + MessageBody: chat1.NewMessageBodyWithText(chat1.MessageText{ + Body: msgBody, + }), + }) +} + +func TestUnfurlerSuppress(t *testing.T) { + tc := externalstest.SetupTest(t, "unfurler", 0) + defer tc.Cleanup() + g := globals.NewContext(tc.G, &globals.ChatContext{}) + + store := attachments.NewStoreTesting(g, nil) + s3signer := &ptsigner{} + g.ActivityNotifier = makeDummyActivityNotifier() + g.MessageDeliverer = dummyDeliverer{} + sender := makeDummySender() + ri := func() chat1.RemoteInterface { return paramsRemote{} } + memStorage := newMemConversationBackedStorage() + unfurler := NewUnfurler(g, store, s3signer, memStorage, sender, ri) + suppressedClock := clockwork.NewFakeClock() + unfurler.suppressed.setClock(suppressedClock) + + uid := gregor1.UID([]byte{0, 1}) + convID := chat1.ConversationID([]byte{0, 1, 2}) + srv := createTestCaseHTTPSrv(t) + addr := srv.Start() + defer srv.Stop() + + url := fmt.Sprintf("http://%s/?name=%s", addr, "wsj0.html") + require.NoError(t, unfurler.WhitelistAdd(context.TODO(), uid, "127.0.0.1")) + + outboxID, err := storage.NewOutboxID() + require.NoError(t, err) + msg := makeTextMsgWithOutboxID("check out this link! "+url, outboxID) + + unfurler.SetSuppressed(context.TODO(), outboxID, []string{url}) + unfurler.UnfurlAndSend(context.TODO(), uid, convID, msg) + select { + case <-sender.ch: + require.Fail(t, "should not have sent a suppressed unfurl") + case <-time.After(2 * time.Second): + } + + // suppression is not consumed: accepting an unfurl prompt re-runs + // UnfurlAndSend on the same message, and the dismissed URL must stay + // suppressed on that second pass + unfurler.UnfurlAndSend(context.TODO(), uid, convID, msg) + select { + case <-sender.ch: + require.Fail(t, "should not have sent a suppressed unfurl on the second pass") + case <-time.After(2 * time.Second): + } + + // the entry expires rather than being consumed + suppressedClock.Advance(suppressedCacheLifetime + time.Minute) + unfurler.UnfurlAndSend(context.TODO(), uid, convID, msg) + select { + case <-sender.ch: + case <-time.After(20 * time.Second): + require.Fail(t, "no unfurl message sent") + } +} diff --git a/go/protocol/chat1/local.go b/go/protocol/chat1/local.go index 66afa09eb337..6fccec09f1df 100644 --- a/go/protocol/chat1/local.go +++ b/go/protocol/chat1/local.go @@ -5844,6 +5844,18 @@ func (o UnfurlPromptResult) DeepCopy() UnfurlPromptResult { } } +type UnfurlPreviewInfo struct { + Url string `codec:"url" json:"url"` + Unfurl UnfurlDisplay `codec:"unfurl" json:"unfurl"` +} + +func (o UnfurlPreviewInfo) DeepCopy() UnfurlPreviewInfo { + return UnfurlPreviewInfo{ + Url: o.Url, + Unfurl: o.Unfurl.DeepCopy(), + } +} + type GalleryItemTyp int const ( @@ -6847,6 +6859,7 @@ type PostTextNonblockArg struct { OutboxID *OutboxID `codec:"outboxID,omitempty" json:"outboxID,omitempty"` IdentifyBehavior keybase1.TLFIdentifyBehavior `codec:"identifyBehavior" json:"identifyBehavior"` EphemeralLifetime *gregor1.DurationSec `codec:"ephemeralLifetime,omitempty" json:"ephemeralLifetime,omitempty"` + UnfurlSuppress []string `codec:"unfurlSuppress" json:"unfurlSuppress"` } type PostDeleteNonblockArg struct { @@ -7234,6 +7247,11 @@ type SaveUnfurlSettingsArg struct { Whitelist []string `codec:"whitelist" json:"whitelist"` } +type UnfurlPreviewLocalArg struct { + ConvID ConversationID `codec:"convID" json:"convID"` + Text string `codec:"text" json:"text"` +} + type ToggleMessageCollapseArg struct { ConvID ConversationID `codec:"convID" json:"convID"` MsgID MessageID `codec:"msgID" json:"msgID"` @@ -7553,6 +7571,7 @@ type LocalInterface interface { ResolveUnfurlPrompt(context.Context, ResolveUnfurlPromptArg) error GetUnfurlSettings(context.Context) (UnfurlSettingsDisplay, error) SaveUnfurlSettings(context.Context, SaveUnfurlSettingsArg) error + UnfurlPreviewLocal(context.Context, UnfurlPreviewLocalArg) ([]UnfurlPreviewInfo, error) ToggleMessageCollapse(context.Context, ToggleMessageCollapseArg) error BulkAddToConv(context.Context, BulkAddToConvArg) error BulkAddToManyConvs(context.Context, BulkAddToManyConvsArg) error @@ -8777,6 +8796,21 @@ func LocalProtocol(i LocalInterface) rpc.Protocol { return }, }, + "unfurlPreviewLocal": { + MakeArg: func() any { + var ret [1]UnfurlPreviewLocalArg + return &ret + }, + Handler: func(ctx context.Context, args any) (ret any, err error) { + typedArgs, ok := args.(*[1]UnfurlPreviewLocalArg) + if !ok { + err = rpc.NewTypeError((*[1]UnfurlPreviewLocalArg)(nil), args) + return + } + ret, err = i.UnfurlPreviewLocal(ctx, typedArgs[0]) + return + }, + }, "toggleMessageCollapse": { MakeArg: func() any { var ret [1]ToggleMessageCollapseArg @@ -9921,6 +9955,11 @@ func (c LocalClient) SaveUnfurlSettings(ctx context.Context, __arg SaveUnfurlSet return } +func (c LocalClient) UnfurlPreviewLocal(ctx context.Context, __arg UnfurlPreviewLocalArg) (res []UnfurlPreviewInfo, err error) { + err = c.Cli.Call(ctx, "chat.1.local.unfurlPreviewLocal", []any{__arg}, &res, 15000*time.Millisecond) + return +} + func (c LocalClient) ToggleMessageCollapse(ctx context.Context, __arg ToggleMessageCollapseArg) (err error) { err = c.Cli.Call(ctx, "chat.1.local.toggleMessageCollapse", []any{__arg}, nil, 0*time.Millisecond) return diff --git a/protocol/avdl/chat1/local.avdl b/protocol/avdl/chat1/local.avdl index 3312374e5fbf..24872ca01416 100644 --- a/protocol/avdl/chat1/local.avdl +++ b/protocol/avdl/chat1/local.avdl @@ -841,7 +841,7 @@ protocol local { PostLocalNonblockRes forwardMessageNonblock(int sessionID, ConversationID srcConvID, ConversationID dstConvID, MessageID msgID, keybase1.TLFIdentifyBehavior identifyBehavior, string title); @timeout_msec(30000) // 30 seconds - PostLocalNonblockRes postTextNonblock(int sessionID, ConversationID conversationID, string tlfName, boolean tlfPublic, string body, MessageID clientPrev, union { null, MessageID } replyTo, union { null, OutboxID } outboxID, keybase1.TLFIdentifyBehavior identifyBehavior, union {null, gregor1.DurationSec} ephemeralLifetime); + PostLocalNonblockRes postTextNonblock(int sessionID, ConversationID conversationID, string tlfName, boolean tlfPublic, string body, MessageID clientPrev, union { null, MessageID } replyTo, union { null, OutboxID } outboxID, keybase1.TLFIdentifyBehavior identifyBehavior, union {null, gregor1.DurationSec} ephemeralLifetime, array unfurlSuppress); @timeout_msec(30000) // 30 seconds PostLocalNonblockRes postDeleteNonblock(ConversationID conversationID, string tlfName, boolean tlfPublic, MessageID supersedes,MessageID clientPrev, union { null, OutboxID } outboxID, keybase1.TLFIdentifyBehavior identifyBehavior); @@ -1243,6 +1243,14 @@ protocol local { UnfurlSettingsDisplay getUnfurlSettings(); void saveUnfurlSettings(UnfurlMode mode, array whitelist); + record UnfurlPreviewInfo { + string url; + UnfurlDisplay unfurl; + } + + @timeout_msec(15000) + array unfurlPreviewLocal(ConversationID convID, string text); + void toggleMessageCollapse(ConversationID convID, MessageID msgID, boolean collapse); void bulkAddToConv(ConversationID convID, array usernames); diff --git a/protocol/bin/enabled-calls.json b/protocol/bin/enabled-calls.json index 664a2f886ccb..35ebd092896e 100644 --- a/protocol/bin/enabled-calls.json +++ b/protocol/bin/enabled-calls.json @@ -147,6 +147,7 @@ "chat.1.local.toggleMessageCollapse": {"promise":true}, "chat.1.local.trackGiphySelect": {"promise":true}, "chat.1.local.unboxMobilePushNotification": {"promise":true}, + "chat.1.local.unfurlPreviewLocal": {"promise":true}, "chat.1.local.unpinMessage": {"promise":true}, "chat.1.local.updateTyping": {"promise":true}, "chat.1.local.updateUnsentText": {"promise":true}, diff --git a/protocol/json/chat1/local.json b/protocol/json/chat1/local.json index f8008d79a0b2..1d995048f883 100644 --- a/protocol/json/chat1/local.json +++ b/protocol/json/chat1/local.json @@ -3672,6 +3672,20 @@ } ] }, + { + "type": "record", + "name": "UnfurlPreviewInfo", + "fields": [ + { + "type": "string", + "name": "url" + }, + { + "type": "UnfurlDisplay", + "name": "unfurl" + } + ] + }, { "type": "enum", "name": "GalleryItemTyp", @@ -4756,6 +4770,13 @@ null, "gregor1.DurationSec" ] + }, + { + "name": "unfurlSuppress", + "type": { + "type": "array", + "items": "string" + } } ], "response": "PostLocalNonblockRes", @@ -5918,6 +5939,23 @@ ], "response": null }, + "unfurlPreviewLocal": { + "request": [ + { + "name": "convID", + "type": "ConversationID" + }, + { + "name": "text", + "type": "string" + } + ], + "response": { + "type": "array", + "items": "UnfurlPreviewInfo" + }, + "timeout_msec": 15000 + }, "toggleMessageCollapse": { "request": [ { diff --git a/shared/chat/conversation/input-area/input-state.test.tsx b/shared/chat/conversation/input-area/input-state.test.tsx index ba6abdef4184..59d42a478f8b 100644 --- a/shared/chat/conversation/input-area/input-state.test.tsx +++ b/shared/chat/conversation/input-area/input-state.test.tsx @@ -4,14 +4,36 @@ import * as Message from '@/constants/chat/message' import type * as React from 'react' import * as T from '@/constants/types' import HiddenString from '@/util/hidden-string' -import {act, cleanup, renderHook} from '@testing-library/react' +import {act, cleanup, render, renderHook} from '@testing-library/react' import {notifyEngineActionListeners} from '@/engine/action-listener' import {resetAllStores} from '@/util/zustand' import {useCurrentUserState} from '@/stores/current-user' +import Input from './normal' +import type {PlatformInputProps} from './normal/input.shared' import {ConversationInputProvider, useConversationInput} from './input-state' import {ConversationThreadProvider, useConversationThreadActions} from '../thread-context' +import {getSuppressedURLs, useUnfurlPreviewState} from '../unfurl-preview-state' let mockRouteParams: Record = {} +let mockPlatformInputProps: PlatformInputProps | undefined +// stand in for the real composer input: it only has to hand back a ref whose clear() +// fires onChangeText('') the way the desktop input does, which is what races the send +jest.mock('./normal/input', () => ({ + __esModule: true, + default: function MockPlatformInput(p: PlatformInputProps) { + mockPlatformInputProps = p + p.setInputRef({ + blur: () => {}, + clear: () => mockPlatformInputProps?.onChangeText(''), + focus: () => {}, + getSelection: () => undefined, + isFocused: () => false, + transformText: () => {}, + value: '', + }) + return null + }, +})) // useChatThreadRouteParams only honors params on the chat thread routes, so the mock needs a matching name jest.mock('@react-navigation/native', () => ({ useRoute: () => ({name: 'chatConversation', params: mockRouteParams}), @@ -86,6 +108,10 @@ const renderInput = (id = convID) => wrapper: wrapperFor(id), }) +function renderComposer(id = convID) { + return render(, {wrapper: wrapperFor(id)}) +} + const renderInputWithThreadActions = (id = convID) => renderHook( () => ({ @@ -112,6 +138,7 @@ beforeEach(() => { }) afterEach(() => { + mockPlatformInputProps = undefined cleanup() jest.restoreAllMocks() resetAllStores() @@ -396,6 +423,88 @@ test('giphy engine events and send path update the input owner', async () => { expect(result.current.input.unsentText).toBe('') }) +test('sendComposerText sends dismissed unfurl urls as unfurlSuppress', async () => { + const getLastPost = mockPostText() + useUnfurlPreviewState.getState().dispatch.dismiss(convID, ['http://a.com']) + const {result} = renderInput() + + act(() => { + result.current.dispatch.sendComposerText('hi http://a.com') + }) + await flushPromises() + + expect(getLastPost()?.params.unfurlSuppress).toEqual(['http://a.com']) + expect(getLastPost()?.params.outboxID).toBeTruthy() + expect(getSuppressedURLs(convID)).toEqual([]) +}) + +test('onSubmit sends dismissed unfurl urls even though clearing the composer drops them', async () => { + jest.useFakeTimers() + try { + const getLastPost = mockPostText() + jest.spyOn(T.RPCChat, 'localUpdateTypingRpcPromise').mockResolvedValue(undefined) + jest.spyOn(T.RPCChat, 'localUpdateUnsentTextRpcPromise').mockResolvedValue(undefined) + jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise').mockResolvedValue([ + { + unfurl: {generic: {siteName: 'a', title: 'a', url: 'http://a.com'}, unfurlType: T.RPCChat.UnfurlType.generic}, + url: 'http://a.com', + } as T.RPCChat.UnfurlPreviewInfo, + ]) + renderComposer() + + const text = 'look at http://a.com' + act(() => { + mockPlatformInputProps?.onChangeText(text) + }) + // let the preview debounce fire so the hook holds a preview for this url + await act(async () => { + jest.advanceTimersByTime(600) + await flushPromises() + }) + + // the card's X + act(() => { + useUnfurlPreviewState.getState().dispatch.dismiss(convID, ['http://a.com']) + }) + expect(getSuppressedURLs(convID)).toEqual(['http://a.com']) + + // more than the 200ms draft throttle since the last keystroke, so clearing the composer + // runs updateDraft's leading edge synchronously and the hook drops the dismissal + act(() => { + mockPlatformInputProps?.onSubmit(text) + }) + expect(getSuppressedURLs(convID)).toEqual([]) + + await act(async () => { + jest.advanceTimersByTime(1) + await flushPromises() + }) + + expect(getLastPost()?.params.body).toBe(text) + expect(getLastPost()?.params.unfurlSuppress).toEqual(['http://a.com']) + } finally { + jest.useRealTimers() + } +}) + +test('a canceled stellar send restores the dismissed unfurl urls for the resend', async () => { + // the composer clears its dismissals before the send resolves, so a cancel has to put + // the snapshot back or the restored text re-unfurls what the user dismissed + jest.spyOn(T.RPCChat, 'localPostTextNonblockRpcListener').mockImplementation(async p => { + p.incomingCallMap['chat.1.chatUi.chatStellarDone']?.({canceled: true}) + await Promise.resolve() + return {outboxID: makeRpcOutboxID('posted-outbox')} + }) + const {result} = renderInput() + + act(() => { + result.current.dispatch.sendComposerText('hi http://a.com', ['http://a.com']) + }) + await flushPromises() + + expect(getSuppressedURLs(convID)).toEqual(['http://a.com']) +}) + test('toggleGiphyPrefill toggles the slash command text', () => { const {result} = renderInput() diff --git a/shared/chat/conversation/input-area/input-state.tsx b/shared/chat/conversation/input-area/input-state.tsx index c58d2da73965..42255ce62902 100644 --- a/shared/chat/conversation/input-area/input-state.tsx +++ b/shared/chat/conversation/input-area/input-state.tsx @@ -22,7 +22,7 @@ type ConversationInputStore = T.Immutable<{ type ConversationInputDispatch = { injectIntoInput: (text?: string, focus?: boolean) => void resetState: () => void - sendComposerText: (text: string) => void + sendComposerText: (text: string, unfurlSuppress?: ReadonlyArray) => void sendGiphyResult: (result: T.RPCChat.GiphySearchResult) => void setCommandMarkdown: (md?: T.RPCChat.UICommandMarkdown) => void setCommandStatusInfo: (info?: T.Chat.CommandStatusInfo) => void @@ -174,11 +174,12 @@ export const ConversationInputProvider = (p: React.PropsWithChildren<{id: T.Chat }) } }) - const sendComposerText = React.useEffectEvent((text: string) => { + const sendComposerText = React.useEffectEvent((text: string, unfurlSuppress?: ReadonlyArray) => { sendMessage(text, { editingOrdinal: state.editing, onRestoreText: injectIntoInput, replyToOrdinal: state.replyTo, + unfurlSuppress, }) dispatchState({type: 'afterSend'}) }) diff --git a/shared/chat/conversation/input-area/normal/index.tsx b/shared/chat/conversation/input-area/normal/index.tsx index 4201177d775a..aaf466d5f6e3 100644 --- a/shared/chat/conversation/input-area/normal/index.tsx +++ b/shared/chat/conversation/input-area/normal/index.tsx @@ -7,6 +7,7 @@ import Giphy from '../../giphy' import * as InputState from '../input-state' import PlatformInput from './input' import ReplyPreview from '../../reply-preview' +import UnfurlPreview from '../unfurl-preview' import * as T from '@/constants/types' import {indefiniteArticle} from '@/util/string' import {infoPanelWidthTablet} from '../../info-panel/common' @@ -26,6 +27,7 @@ import {useConversationParticipantsSelector} from '../../data-hooks' import {useCurrentUserState} from '@/stores/current-user' import {useRoute} from '@react-navigation/native' import {metasReceived, unboxRows, useInboxMetadataState} from '@/chat/inbox/metadata' +import {getSuppressedURLs} from '@/chat/conversation/unfurl-preview-state' const useHintText = (p: { isExploding: boolean @@ -207,9 +209,13 @@ const ConnectedPlatformInput = function ConnectedPlatformInput() { // A timeout rather than requestAnimationFrame: this callback owns the only copy of the text, and // frames stop in a hidden or backgrounded window, which would drop the message with the composer // already emptied. + // Snapshot the dismissed unfurls before clearing: the clear runs onChangeText('') + // synchronously, and the preview hook drops every dismissal for empty text, which + // would beat the deferred send to the store and unfurl a card the user dismissed. + const unfurlSuppress = getSuppressedURLs(conversationIDKey) injectText('', true) setTimeout(() => { - sendComposerText(text) + sendComposerText(text, unfurlSuppress) if (hasCenter) { toggleThreadSearch(true) jumpToRecent() @@ -228,6 +234,10 @@ const ConnectedPlatformInput = function ConnectedPlatformInput() { } const sendTyping = C.useThrottledCallback(sendTypingRaw, 1000) + // Low-frequency (throttled) copy of the composer text for the unfurl preview, which + // already debounces 500ms downstream. textValueRef is a ref (no re-render), so previews + // ride along on the existing throttled draft-save path instead of a per-keystroke state. + const [previewText, setPreviewText] = React.useState('') const updateDraftRaw = (text: string) => { // Immediately update local meta.draft so switching back to this thread // before the async unbox completes won't re-inject the old stale draft. @@ -236,6 +246,9 @@ const ConnectedPlatformInput = function ConnectedPlatformInput() { if (currentMeta) { metasReceived([{...currentMeta, draft: text}], undefined, {force: true}) } + if (!isMobile) { + setPreviewText(text) + } const f = async () => { await T.RPCChat.localUpdateUnsentTextRpcPromise({ conversationID: convoID, @@ -310,21 +323,24 @@ const ConnectedPlatformInput = function ConnectedPlatformInput() { }, [setInputRef]) return ( - + <> + {isMobile ? null : } + + ) } diff --git a/shared/chat/conversation/input-area/unfurl-preview.test.tsx b/shared/chat/conversation/input-area/unfurl-preview.test.tsx new file mode 100644 index 000000000000..938ce887b4ff --- /dev/null +++ b/shared/chat/conversation/input-area/unfurl-preview.test.tsx @@ -0,0 +1,68 @@ +/** @jest-environment jsdom */ +/// +import * as T from '@/constants/types' +import {render} from '@testing-library/react' +import UnfurlPreview from './unfurl-preview' + +const mockDismiss = jest.fn() +let mockPreviews: ReadonlyArray = [] + +jest.mock('@/chat/conversation/unfurl-preview-state', () => ({ + useUnfurlPreviews: () => ({dismiss: mockDismiss, previews: mockPreviews}), +})) + +const convID = T.Chat.conversationIDToKey(new Uint8Array([1, 2, 3, 4])) + +const nonGenericInfo: T.RPCChat.UnfurlPreviewInfo = { + unfurl: {unfurlType: T.RPCChat.UnfurlType.youtube, youtube: {}}, + url: 'http://youtube.com/watch', +} as T.RPCChat.UnfurlPreviewInfo + +const genericInfo: T.RPCChat.UnfurlPreviewInfo = { + unfurl: {generic: {siteName: 'a', title: 'a', url: 'http://a.com'}, unfurlType: T.RPCChat.UnfurlType.generic}, + url: 'http://a.com', +} as T.RPCChat.UnfurlPreviewInfo + +const mapInfo: T.RPCChat.UnfurlPreviewInfo = { + unfurl: { + generic: {mapInfo: {isLiveLocationDone: true}, siteName: 'Google Maps', title: 'here', url: 'http://map.com'}, + unfurlType: T.RPCChat.UnfurlType.generic, + }, + url: 'http://map.com', +} as T.RPCChat.UnfurlPreviewInfo + +describe('UnfurlPreview', () => { + afterEach(() => { + mockPreviews = [] + }) + + it('renders nothing when every preview is non-generic', () => { + mockPreviews = [nonGenericInfo] + const {container} = render() + expect(container.firstChild).toBeNull() + }) + + it('renders a card for a generic preview', () => { + mockPreviews = [genericInfo] + const {container} = render() + expect(container.firstChild).not.toBeNull() + }) + + it('renders nothing for a map unfurl', () => { + mockPreviews = [mapInfo] + const {container} = render() + expect(container.firstChild).toBeNull() + }) + + it('renders nothing on mobile even with a generic preview', () => { + mockPreviews = [genericInfo] + const originalIsMobile = global.isMobile + global.isMobile = true + try { + const {container} = render() + expect(container.firstChild).toBeNull() + } finally { + global.isMobile = originalIsMobile + } + }) +}) diff --git a/shared/chat/conversation/input-area/unfurl-preview.tsx b/shared/chat/conversation/input-area/unfurl-preview.tsx new file mode 100644 index 000000000000..2a3fda31f7fc --- /dev/null +++ b/shared/chat/conversation/input-area/unfurl-preview.tsx @@ -0,0 +1,60 @@ +import * as Kb from '@/common-adapters' +import * as T from '@/constants/types' +import UnfurlGenericView from '@/chat/conversation/messages/text/unfurl/unfurl-list/generic-view' +import {useUnfurlPreviews} from '@/chat/conversation/unfurl-preview-state' + +type Props = { + conversationIDKey: T.Chat.ConversationIDKey + text: string +} + +const UnfurlPreview = (p: Props) => { + const {conversationIDKey, text} = p + const styles = useStyles() + const {dismiss, previews} = useUnfurlPreviews(conversationIDKey, text) + const genericPreviews = previews.flatMap(preview => { + const {unfurl} = preview + if (unfurl.unfurlType !== T.RPCChat.UnfurlType.generic || unfurl.generic.mapInfo) { + // a map unfurl is a generic unfurl with mapInfo set, and the message card refuses + // to render those, so the preview must not show one either + return [] + } + return [{generic: unfurl.generic, preview}] + }) + if (isMobile || !genericPreviews.length) { + return null + } + return ( + + {genericPreviews.map(({preview, generic}) => ( + dismiss(preview.url)} + publishTime={generic.publishTime ?? undefined} + siteName={generic.siteName} + title={generic.title} + url={generic.url} + /> + ))} + + ) +} + +const useStyles = Kb.Styles.createStyleHook( + theme => + ({ + container: Kb.Styles.platformStyles({ + isElectron: { + backgroundColor: theme.blueGrey, + maxHeight: 200, + overflowY: 'auto', + padding: Kb.Styles.globalMargins.tiny, + }, + }), + }) as const +) + +export default UnfurlPreview diff --git a/shared/chat/conversation/messages/text/unfurl/unfurl-list/generic-view.tsx b/shared/chat/conversation/messages/text/unfurl/unfurl-list/generic-view.tsx new file mode 100644 index 000000000000..5f5976f9cd8f --- /dev/null +++ b/shared/chat/conversation/messages/text/unfurl/unfurl-list/generic-view.tsx @@ -0,0 +1,167 @@ +import * as Kb from '@/common-adapters/index' +import type * as T from '@/constants/types' +import UnfurlImage from './image' +import {formatTimeForMessages} from '@/util/timestamp' + +export type UnfurlGenericViewProps = { + description?: string + favicon?: T.RPCChat.UnfurlImageDisplay + isCollapsed?: boolean + media?: T.RPCChat.UnfurlImageDisplay + onClose?: () => void + onToggleCollapse?: () => void + publishTime?: number + siteName: string + title: string + url: string +} + +export default function UnfurlGenericView(p: UnfurlGenericViewProps) { + const {description, favicon, isCollapsed, media, onClose, onToggleCollapse, publishTime, siteName, title, url} = p + const styles = useStyles() + const theme = Kb.Styles.useTheme() + const titleUrlProps = Kb.useClickURL(url) + const {height, width, isVideo, url: mediaUrl} = media || {height: 0, isVideo: false, url: '', width: 0} + const showImageOnSide = + !isMobile && height >= width && !isVideo && (title.length > 0 || !!description) + const imageLocation = isCollapsed ? 'collapsed' : showImageOnSide ? 'side' : width > 0 && height > 0 ? 'bottom' : 'none' + + const publisher = ( + + {favicon?.url ? : null} + + + {siteName} + {publishTime ? ( + • Published {formatTimeForMessages(publishTime * 1000)} + ) : null} + + + {onClose ? ( + + ) : null} + + ) + + const snippet = description ? ( + + {description} + {(imageLocation === 'collapsed' || imageLocation === 'bottom') && ( + <> + {' '} + + + )} + + ) : null + + const bottomImage = + imageLocation === 'bottom' ? ( + + + + ) : null + + const rightImage = + imageLocation === 'side' && mediaUrl ? ( + + + + ) : null + + return ( + + {!isMobile && } + + {publisher} + + {title} + + {snippet} + {bottomImage} + + {rightImage} + + ) +} + +const useStyles = Kb.Styles.createStyleHook( + theme => + ({ + bottomImage: Kb.Styles.platformStyles({ + common: {marginTop: Kb.Styles.globalMargins.xtiny}, + isMobile: {alignSelf: 'center'}, + }), + closeBox: Kb.Styles.platformStyles({ + isElectron: { + alignSelf: 'flex-start', + marginLeft: 'auto', + }, + }), + collapseBox: Kb.Styles.platformStyles({ + isElectron: {display: 'inline'}, + }), + container: Kb.Styles.platformStyles({ + isElectron: {maxWidth: 500}, + isTablet: {maxWidth: 500}, + }), + + favicon: Kb.Styles.platformStyles({ + common: { + borderRadius: Kb.Styles.borderRadius, + ...Kb.Styles.size(16), + }, + }), + innerContainer: Kb.Styles.platformStyles({ + common: { + minWidth: 150, + }, + isMobile: { + borderColor: theme.grey, + borderRadius: Kb.Styles.borderRadius, + borderWidth: 1, + padding: Kb.Styles.globalMargins.xtiny, + }, + }), + quoteContainer: Kb.Styles.platformStyles({ + common: { + backgroundColor: theme.grey, + paddingLeft: Kb.Styles.globalMargins.xtiny, + }, + }), + sideImage: Kb.Styles.platformStyles({ + isElectron: { + ...Kb.Styles.size(80), + }, + }), + siteNameContainer: Kb.Styles.platformStyles({ + isElectron: {minHeight: 16}, + isMobile: {minHeight: 21}, + }), + url: { + ...Kb.Styles.globalStyles.fontSemibold, + }, + }) as const +) diff --git a/shared/chat/conversation/messages/text/unfurl/unfurl-list/generic.tsx b/shared/chat/conversation/messages/text/unfurl/unfurl-list/generic.tsx index 2878b06d2edf..75b8a4d304eb 100644 --- a/shared/chat/conversation/messages/text/unfurl/unfurl-list/generic.tsx +++ b/shared/chat/conversation/messages/text/unfurl/unfurl-list/generic.tsx @@ -1,18 +1,12 @@ -import * as Kb from '@/common-adapters/index' import * as T from '@/constants/types' -import UnfurlImage from './image' -import {formatTimeForMessages} from '@/util/timestamp' +import UnfurlGenericView from './generic-view' import {useActions} from './use-state' function UnfurlGeneric(p: { - author: string - conversationIDKey: T.Chat.ConversationIDKey ordinal: T.Chat.Ordinal unfurlInfo: T.RPCChat.UIMessageUnfurlInfo youAreAuthor: boolean }) { - const styles = useStyles() - const theme = Kb.Styles.useTheme() const {ordinal, unfurlInfo, youAreAuthor} = p const {isCollapsed, unfurl, unfurlMessageID} = unfurlInfo const {onClose, onToggleCollapse} = useActions( @@ -21,154 +15,25 @@ function UnfurlGeneric(p: { ordinal ) const generic = unfurl.unfurlType === T.RPCChat.UnfurlType.generic ? unfurl.generic : undefined - const titleUrlProps = Kb.useClickURL(generic?.mapInfo ? '' : (generic?.url ?? '')) if (!generic || generic.mapInfo) { return null } const {description, publishTime, favicon, media, siteName, title, url} = generic - const {height, width, isVideo, url: mediaUrl} = media || {height: 0, isVideo: false, url: '', width: 0} - const showImageOnSide = - !isMobile && height >= width && !isVideo && (title.length > 0 || !!description) - const imageLocation = isCollapsed ? 'collapsed' : showImageOnSide ? 'side' : width > 0 && height > 0 ? 'bottom' : 'none' - - const publisher = ( - - {favicon?.url ? : null} - - - {siteName} - {publishTime ? ( - • Published {formatTimeForMessages(publishTime * 1000)} - ) : null} - - - {onClose ? ( - - ) : null} - - ) - - const snippet = description ? ( - - {description} - {(imageLocation === 'collapsed' || imageLocation === 'bottom') && ( - <> - {' '} - - - )} - - ) : null - - const bottomImage = - imageLocation === 'bottom' ? ( - - - - ) : null - - const rightImage = - imageLocation === 'side' && mediaUrl ? ( - - - - ) : null return ( - - {!isMobile && } - - {publisher} - - {title} - - {snippet} - {bottomImage} - - {rightImage} - + ) } -const useStyles = Kb.Styles.createStyleHook( - theme => - ({ - bottomImage: Kb.Styles.platformStyles({ - common: {marginTop: Kb.Styles.globalMargins.xtiny}, - isMobile: {alignSelf: 'center'}, - }), - closeBox: Kb.Styles.platformStyles({ - isElectron: { - alignSelf: 'flex-start', - marginLeft: 'auto', - }, - }), - collapseBox: Kb.Styles.platformStyles({ - isElectron: {display: 'inline'}, - }), - container: Kb.Styles.platformStyles({ - isElectron: {maxWidth: 500}, - isTablet: {maxWidth: 500}, - }), - - favicon: Kb.Styles.platformStyles({ - common: { - borderRadius: Kb.Styles.borderRadius, - ...Kb.Styles.size(16), - }, - }), - innerContainer: Kb.Styles.platformStyles({ - common: { - minWidth: 150, - }, - isMobile: { - borderColor: theme.grey, - borderRadius: Kb.Styles.borderRadius, - borderWidth: 1, - padding: Kb.Styles.globalMargins.xtiny, - }, - }), - quoteContainer: Kb.Styles.platformStyles({ - common: { - backgroundColor: theme.grey, - paddingLeft: Kb.Styles.globalMargins.xtiny, - }, - }), - sideImage: Kb.Styles.platformStyles({ - isElectron: { - ...Kb.Styles.size(80), - }, - }), - siteNameContainer: Kb.Styles.platformStyles({ - isElectron: {minHeight: 16}, - isMobile: {minHeight: 21}, - }), - url: { - ...Kb.Styles.globalStyles.fontSemibold, - }, - }) as const -) - export default UnfurlGeneric diff --git a/shared/chat/conversation/send-actions.tsx b/shared/chat/conversation/send-actions.tsx index 75ab4c1302ac..ea3c0cc9340e 100644 --- a/shared/chat/conversation/send-actions.tsx +++ b/shared/chat/conversation/send-actions.tsx @@ -4,6 +4,7 @@ import logger from '@/logger' import {RPCError} from '@/util/errors' import {ignorePromise} from '@/constants/utils' import {getClientPrevFromThread} from './attachment-actions' +import {getSuppressedURLs, removeSuppressedURLs, restoreSuppressedURLs} from './unfurl-preview-state' import {useInboxMetadataState} from '../inbox/metadata-store' import { useConversationThreadActions, @@ -16,15 +17,20 @@ type SendTextParams = { conversationIDKey: T.Chat.ConversationIDKey ephemeralLifetime: number onRestoreText?: (text: string) => void + onSent?: () => void replyTo?: T.Chat.MessageID text: string tlfName: string + unfurlSuppress?: ReadonlyArray waitingKey?: string } const sendTextMessageStoreless = (p: SendTextParams) => { const f = async () => { const ephemeralData = p.ephemeralLifetime !== 0 ? {ephemeralLifetime: p.ephemeralLifetime} : {} + // a canceled stellar confirm resolves the rpc normally but posts nothing, so it is + // not a send and must not be treated as one + const sendState = {stellarCanceled: false} try { await T.RPCChat.localPostTextNonblockRpcListener({ customResponseIncomingCallMap: { @@ -38,6 +44,7 @@ const sendTextMessageStoreless = (p: SendTextParams) => { incomingCallMap: { 'chat.1.chatUi.chatStellarDone': ({canceled}) => { if (canceled) { + sendState.stellarCanceled = true p.onRestoreText?.(p.text) } }, @@ -49,13 +56,17 @@ const sendTextMessageStoreless = (p: SendTextParams) => { clientPrev: p.clientPrev, conversationID: T.Chat.keyToConversationID(p.conversationIDKey), identifyBehavior: T.RPCGen.TLFIdentifyBehavior.chatGui, - outboxID: undefined, + outboxID: Common.generateOutboxID(), replyTo: p.replyTo, tlfName: p.tlfName, tlfPublic: false, + unfurlSuppress: p.unfurlSuppress ? [...p.unfurlSuppress] : [], }, waitingKey: p.waitingKey, }) + if (!sendState.stellarCanceled) { + p.onSent?.() + } logger.info('success') } catch { logger.info('error') @@ -127,6 +138,7 @@ export const useConversationSendActions = () => { editingOrdinal?: T.Chat.Ordinal onRestoreText?: (text: string) => void replyToOrdinal?: T.Chat.Ordinal + unfurlSuppress?: ReadonlyArray } ) => { const editOrdinal = context?.editingOrdinal @@ -136,14 +148,27 @@ export const useConversationSendActions = () => { } const replyToOrdinal = context?.replyToOrdinal const replyTo = threadStore.getState().messageMap.get(replyToOrdinal ?? T.Chat.numberToOrdinal(0))?.id + // the caller passes a snapshot taken before it cleared the composer: clearing runs + // synchronously and the preview hook drops every dismissal once the text is empty + const unfurlSuppress = context?.unfurlSuppress ?? getSuppressedURLs(conversationIDKey) + const onRestoreText = context?.onRestoreText sendTextMessageStoreless({ clientPrev: getClientPrev(), conversationIDKey, ephemeralLifetime: threadStore.getState().explodingMode, - onRestoreText: context?.onRestoreText, + onRestoreText: onRestoreText + ? (restored: string) => { + restoreSuppressedURLs(conversationIDKey, unfurlSuppress) + onRestoreText(restored) + } + : undefined, + onSent: () => { + removeSuppressedURLs(conversationIDKey, unfurlSuppress) + }, replyTo, text, tlfName: getTlfName(), + unfurlSuppress, }) } diff --git a/shared/chat/conversation/unfurl-preview-state.test.tsx b/shared/chat/conversation/unfurl-preview-state.test.tsx new file mode 100644 index 000000000000..438e7825ac25 --- /dev/null +++ b/shared/chat/conversation/unfurl-preview-state.test.tsx @@ -0,0 +1,135 @@ +/** @jest-environment jsdom */ +/// +import * as T from '@/constants/types' +import {act, render, waitFor} from '@testing-library/react' +import {useUnfurlPreviews, getSuppressedURLs, useUnfurlPreviewState} from './unfurl-preview-state' + +// stringToConversationIDKey('conv1') is not valid hex and would throw inside +// T.Chat.keyToConversationID (used to build the RPC's convID param), so build +// the fixture the way input-state.test.tsx does: round-trip through bytes. +const convID = T.Chat.conversationIDToKey(new Uint8Array([1, 2, 3, 4])) +const otherConvID = T.Chat.conversationIDToKey(new Uint8Array([5, 6, 7, 8])) +const info = (url: string): T.RPCChat.UnfurlPreviewInfo => + ({unfurl: {generic: {title: url, url}, unfurlType: T.RPCChat.UnfurlType.generic}, url}) as T.RPCChat.UnfurlPreviewInfo + +const Harness = (p: { + text: string + id?: T.Chat.ConversationIDKey + onRender: (r: ReturnType) => void +}) => { + const r = useUnfurlPreviews(p.id ?? convID, p.text) + p.onRender(r) + return null +} + +describe('unfurl previews', () => { + beforeEach(() => { + jest.useFakeTimers() + useUnfurlPreviewState.getState().dispatch.resetState() + }) + afterEach(() => { + jest.useRealTimers() + jest.restoreAllMocks() + }) + + it('does not call the rpc for text with no link', () => { + const spy = jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise').mockResolvedValue([]) + render( {}} />) + act(() => { + jest.advanceTimersByTime(1000) + }) + expect(spy).not.toHaveBeenCalled() + }) + + it('debounces and returns previews', async () => { + const spy = jest + .spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise') + .mockResolvedValue([info('http://a.com')]) + let last: ReturnType | undefined + render( (last = r)} />) + expect(spy).not.toHaveBeenCalled() + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(last?.previews.length).toBe(1)) + expect(spy).toHaveBeenCalledTimes(1) + }) + + it('drops a stale response', async () => { + let resolveFirst: ((v: Array) => void) | undefined + jest + .spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise') + .mockImplementationOnce( + async () => new Promise>(resolve => (resolveFirst = resolve)) + ) + .mockResolvedValueOnce([info('http://b.com')]) + let last: ReturnType | undefined + const {rerender} = render( (last = r)} />) + act(() => { + jest.advanceTimersByTime(500) + }) + rerender( (last = r)} />) + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(last?.previews[0]?.url).toBe('http://b.com')) + act(() => resolveFirst?.([info('http://a.com')])) + expect(last?.previews[0]?.url).toBe('http://b.com') + }) + + it('dismiss hides the card and records the url for send', async () => { + jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise').mockResolvedValue([info('http://a.com')]) + let last: ReturnType | undefined + render( (last = r)} />) + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(last?.previews.length).toBe(1)) + act(() => last?.dismiss('http://a.com')) + await waitFor(() => expect(last?.previews.length).toBe(0)) + expect(getSuppressedURLs(convID)).toEqual(['http://a.com']) + }) + + it('forgets a dismissal once the url leaves the text', async () => { + jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise').mockResolvedValue([]) + let last: ReturnType | undefined + const {rerender} = render( (last = r)} />) + act(() => { + jest.advanceTimersByTime(500) + }) + act(() => last?.dismiss('http://a.com')) + expect(getSuppressedURLs(convID)).toEqual(['http://a.com']) + rerender( (last = r)} />) + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(getSuppressedURLs(convID)).toEqual([])) + }) + + it('does not flash the previous conversation preview after switching conversations', async () => { + const spy = jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise') + spy.mockResolvedValueOnce([info('http://a.com')]) + let resolveSecond: ((v: Array) => void) | undefined + spy.mockImplementationOnce( + async () => new Promise>(resolve => (resolveSecond = resolve)) + ) + let last: ReturnType | undefined + const {rerender} = render( (last = r)} />) + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(last?.previews[0]?.url).toBe('http://a.com')) + + // switch to a different conversation whose draft also contains a link + rerender( (last = r)} />) + // conv A's preview must be gone immediately, before the new conversation's debounce even fires + expect(last?.previews).toEqual([]) + act(() => { + jest.advanceTimersByTime(500) + }) + // still no preview: the new conversation's fetch is in flight but unresolved + expect(last?.previews).toEqual([]) + act(() => resolveSecond?.([info('http://c.com')])) + await waitFor(() => expect(last?.previews[0]?.url).toBe('http://c.com')) + }) +}) diff --git a/shared/chat/conversation/unfurl-preview-state.tsx b/shared/chat/conversation/unfurl-preview-state.tsx new file mode 100644 index 000000000000..98b896441289 --- /dev/null +++ b/shared/chat/conversation/unfurl-preview-state.tsx @@ -0,0 +1,149 @@ +import * as React from 'react' +import * as T from '@/constants/types' +import * as Z from '@/util/zustand' +import {ignorePromise} from '@/constants/utils' +import logger from '@/logger' + +type State = T.Immutable<{ + dismissed: Map> + dispatch: { + dismiss: (conversationIDKey: T.Chat.ConversationIDKey, urls: ReadonlyArray) => void + keepOnly: (conversationIDKey: T.Chat.ConversationIDKey, urls: ReadonlyArray) => void + remove: (conversationIDKey: T.Chat.ConversationIDKey, urls: ReadonlyArray) => void + resetState: () => void + } +}> + +export const useUnfurlPreviewState = Z.createZustand('unfurl-preview', set => ({ + dismissed: new Map(), + dispatch: { + dismiss: (conversationIDKey, urls) => { + if (!urls.length) return + set(s => { + const existing = s.dismissed.get(conversationIDKey) ?? new Set() + for (const url of urls) existing.add(url) + s.dismissed.set(conversationIDKey, existing) + }) + }, + keepOnly: (conversationIDKey, urls) => { + set(s => { + const existing = s.dismissed.get(conversationIDKey) + if (!existing) return + for (const url of [...existing]) { + if (!urls.includes(url)) existing.delete(url) + } + if (!existing.size) s.dismissed.delete(conversationIDKey) + }) + }, + remove: (conversationIDKey, urls) => { + set(s => { + const existing = s.dismissed.get(conversationIDKey) + if (!existing) return + for (const url of urls) existing.delete(url) + if (!existing.size) s.dismissed.delete(conversationIDKey) + }) + }, + resetState: Z.defaultReset, + }, +})) + +export const getSuppressedURLs = (conversationIDKey: T.Chat.ConversationIDKey) => [ + ...(useUnfurlPreviewState.getState().dismissed.get(conversationIDKey) ?? []), +] + +// dropped once the send they belong to lands; a targeted remove rather than a +// whole-conversation clear so a dismissal made while that send was in flight survives +export const removeSuppressedURLs = ( + conversationIDKey: T.Chat.ConversationIDKey, + urls: ReadonlyArray +) => { + useUnfurlPreviewState.getState().dispatch.remove(conversationIDKey, urls) +} + +// put back the snapshot a send took when that send never posted, so the composer the +// user gets back still has those urls dismissed +export const restoreSuppressedURLs = ( + conversationIDKey: T.Chat.ConversationIDKey, + urls: ReadonlyArray +) => { + useUnfurlPreviewState.getState().dispatch.dismiss(conversationIDKey, urls) +} + +const debounceMS = 500 + +// kept outside the hook body: try/catch inside a hook trips the react-compiler bailout check +const fetchPreviews = async ( + conversationIDKey: T.Chat.ConversationIDKey, + text: string, + requestID: number, + requestIDRef: {current: number}, + onSuccess: (conversationIDKey: T.Chat.ConversationIDKey, infos: ReadonlyArray) => void +) => { + try { + const res = await T.RPCChat.localUnfurlPreviewLocalRpcPromise({ + convID: T.Chat.keyToConversationID(conversationIDKey), + text, + }) + if (requestID !== requestIDRef.current) return + onSuccess(conversationIDKey, res ?? []) + } catch (e) { + // best-effort preview: an RPC failure just means no card shows, nothing for the user to act on + logger.info('unfurl preview failed', e) + } +} + +type FetchedPreviews = { + conversationIDKey: T.Chat.ConversationIDKey + previews: ReadonlyArray +} + +export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, text: string) => { + const [fetched, setFetched] = React.useState({conversationIDKey, previews: []}) + const dismissedSet = useUnfurlPreviewState(s => s.dismissed.get(conversationIDKey)) + const {dismiss: dismissURL, keepOnly} = useUnfurlPreviewState(s => s.dispatch) + const requestIDRef = React.useRef(0) + const hasLink = text.includes('http') + + const onFetched = React.useCallback( + (fetchedConversationIDKey: T.Chat.ConversationIDKey, infos: ReadonlyArray) => { + setFetched({conversationIDKey: fetchedConversationIDKey, previews: infos}) + keepOnly( + fetchedConversationIDKey, + infos.map(i => i.url) + ) + }, + [keepOnly] + ) + + React.useEffect(() => { + const id = ++requestIDRef.current + if (!text.includes('http')) { + keepOnly(conversationIDKey, []) + return + } + const timeoutID = setTimeout(() => { + ignorePromise(fetchPreviews(conversationIDKey, text, id, requestIDRef, onFetched)) + }, debounceMS) + return () => { + clearTimeout(timeoutID) + } + }, [conversationIDKey, keepOnly, text, onFetched]) + + const dismiss = React.useCallback( + (url: string) => { + dismissURL(conversationIDKey, [url]) + }, + [conversationIDKey, dismissURL] + ) + + // mask stale previews from a since-switched conversation the same way `hasLink` masks + // text that no longer has a link, so a switch never flashes the previous conversation's card + const visible = React.useMemo( + () => + hasLink && fetched.conversationIDKey === conversationIDKey + ? fetched.previews.filter(p => !dismissedSet?.has(p.url)) + : [], + [hasLink, fetched, conversationIDKey, dismissedSet] + ) + return {dismiss, previews: visible} +} diff --git a/shared/constants/rpc/rpc-chat-gen.tsx b/shared/constants/rpc/rpc-chat-gen.tsx index ef96791cd603..f29c77a4b23a 100644 --- a/shared/constants/rpc/rpc-chat-gen.tsx +++ b/shared/constants/rpc/rpc-chat-gen.tsx @@ -496,7 +496,7 @@ export type MessageTypes = { outParam: PostLocalNonblockRes, }, 'chat.1.local.postTextNonblock': { - inParam: {readonly conversationID: ConversationID,readonly tlfName: string,readonly tlfPublic: boolean,readonly body: string,readonly clientPrev: MessageID,readonly replyTo?: MessageID | null,readonly outboxID?: OutboxID | null,readonly identifyBehavior: Keybase1.TLFIdentifyBehavior,readonly ephemeralLifetime?: Gregor1.DurationSec | null}, + inParam: {readonly conversationID: ConversationID,readonly tlfName: string,readonly tlfPublic: boolean,readonly body: string,readonly clientPrev: MessageID,readonly replyTo?: MessageID | null,readonly outboxID?: OutboxID | null,readonly identifyBehavior: Keybase1.TLFIdentifyBehavior,readonly ephemeralLifetime?: Gregor1.DurationSec | null,readonly unfurlSuppress?: ReadonlyArray | null}, outParam: PostLocalNonblockRes, }, 'chat.1.local.previewConversationByIDLocal': { @@ -607,6 +607,10 @@ export type MessageTypes = { inParam: {readonly payload: string,readonly convID: string,readonly membersType: ConversationMembersType}, outParam: string, }, + 'chat.1.local.unfurlPreviewLocal': { + inParam: {readonly convID: ConversationID,readonly text: string}, + outParam: ReadonlyArray | null, + }, 'chat.1.local.unpinMessage': { inParam: {readonly convID: ConversationID}, outParam: PinMessageRes, @@ -628,7 +632,7 @@ export type MessageKey = keyof MessageTypes export type RpcIn = MessageTypes[M]['inParam'] export type RpcOut = MessageTypes[M]['outParam'] export type RpcResponse = {error: IncomingErrorCallback, result: (res: RpcOut) => void} -type PromiseMethod = 'chat.1.local.addBotConvSearch' | 'chat.1.local.addBotMember' | 'chat.1.local.addEmojiAlias' | 'chat.1.local.addEmojis' | 'chat.1.local.addTeamMemberAfterReset' | 'chat.1.local.archiveChat' | 'chat.1.local.archiveChatDelete' | 'chat.1.local.archiveChatList' | 'chat.1.local.archiveChatPause' | 'chat.1.local.archiveChatResume' | 'chat.1.local.bulkAddToConv' | 'chat.1.local.bulkAddToManyConvs' | 'chat.1.local.cancelActiveInboxSearch' | 'chat.1.local.cancelActiveSearch' | 'chat.1.local.CancelPost' | 'chat.1.local.cancelUploadTempFile' | 'chat.1.local.ConfigureFileAttachmentDownloadLocal' | 'chat.1.local.deleteConversationLocal' | 'chat.1.local.dismissJourneycard' | 'chat.1.local.DownloadFileAttachmentLocal' | 'chat.1.local.findConversationsLocal' | 'chat.1.local.findGeneralConvFromTeamID' | 'chat.1.local.forwardMessageConvSearch' | 'chat.1.local.forwardMessageNonblock' | 'chat.1.local.getBotMemberSettings' | 'chat.1.local.getDefaultTeamChannelsLocal' | 'chat.1.local.getGlobalAppNotificationSettingsLocal' | 'chat.1.local.getInboxAndUnboxUILocal' | 'chat.1.local.getLastActiveAtMultiLocal' | 'chat.1.local.getLastActiveForTeams' | 'chat.1.local.getMutualTeamsLocal' | 'chat.1.local.getNextAttachmentMessageLocal' | 'chat.1.local.getRecentJoinsLocal' | 'chat.1.local.getStaticConfig' | 'chat.1.local.getTeamRetentionLocal' | 'chat.1.local.getTeamRoleInConversation' | 'chat.1.local.getTLFConversationsLocal' | 'chat.1.local.getUnfurlSettings' | 'chat.1.local.getUnreadline' | 'chat.1.local.getUploadTempFile' | 'chat.1.local.getWelcomeMessage' | 'chat.1.local.ignorePinnedMessage' | 'chat.1.local.joinConversationByIDLocal' | 'chat.1.local.leaveConversationLocal' | 'chat.1.local.listPublicBotCommandsLocal' | 'chat.1.local.locationUpdate' | 'chat.1.local.makeAudioPreview' | 'chat.1.local.makeUploadTempFile' | 'chat.1.local.markAsReadLocal' | 'chat.1.local.markTLFAsReadLocal' | 'chat.1.local.newConversationLocal' | 'chat.1.local.pinMessage' | 'chat.1.local.postDeleteHistoryByAge' | 'chat.1.local.postDeleteNonblock' | 'chat.1.local.postEditNonblock' | 'chat.1.local.postFileAttachmentLocalNonblock' | 'chat.1.local.postHeadline' | 'chat.1.local.postHeadlineNonblock' | 'chat.1.local.postMetadata' | 'chat.1.local.postReactionNonblock' | 'chat.1.local.previewConversationByIDLocal' | 'chat.1.local.putReacjiSkinTone' | 'chat.1.local.refreshParticipants' | 'chat.1.local.removeBotMember' | 'chat.1.local.removeEmoji' | 'chat.1.local.removeFromConversationLocal' | 'chat.1.local.requestInboxLayout' | 'chat.1.local.requestInboxSmallIncrease' | 'chat.1.local.requestInboxSmallReset' | 'chat.1.local.requestInboxUnbox' | 'chat.1.local.resolveMaybeMention' | 'chat.1.local.resolveUnfurlPrompt' | 'chat.1.local.RetryPost' | 'chat.1.local.saveUnfurlSettings' | 'chat.1.local.setAppNotificationSettingsLocal' | 'chat.1.local.setBotMemberSettings' | 'chat.1.local.SetConversationStatusLocal' | 'chat.1.local.setConvMinWriterRoleLocal' | 'chat.1.local.setConvRetentionLocal' | 'chat.1.local.setDefaultTeamChannelsLocal' | 'chat.1.local.setGlobalAppNotificationSettingsLocal' | 'chat.1.local.setTeamRetentionLocal' | 'chat.1.local.setWelcomeMessage' | 'chat.1.local.simpleSearchInboxConvNames' | 'chat.1.local.toggleEmojiAnimations' | 'chat.1.local.toggleMessageCollapse' | 'chat.1.local.trackGiphySelect' | 'chat.1.local.unboxMobilePushNotification' | 'chat.1.local.unpinMessage' | 'chat.1.local.updateTyping' | 'chat.1.local.updateUnsentText' | 'chat.1.local.userEmojis' +type PromiseMethod = 'chat.1.local.addBotConvSearch' | 'chat.1.local.addBotMember' | 'chat.1.local.addEmojiAlias' | 'chat.1.local.addEmojis' | 'chat.1.local.addTeamMemberAfterReset' | 'chat.1.local.archiveChat' | 'chat.1.local.archiveChatDelete' | 'chat.1.local.archiveChatList' | 'chat.1.local.archiveChatPause' | 'chat.1.local.archiveChatResume' | 'chat.1.local.bulkAddToConv' | 'chat.1.local.bulkAddToManyConvs' | 'chat.1.local.cancelActiveInboxSearch' | 'chat.1.local.cancelActiveSearch' | 'chat.1.local.CancelPost' | 'chat.1.local.cancelUploadTempFile' | 'chat.1.local.ConfigureFileAttachmentDownloadLocal' | 'chat.1.local.deleteConversationLocal' | 'chat.1.local.dismissJourneycard' | 'chat.1.local.DownloadFileAttachmentLocal' | 'chat.1.local.findConversationsLocal' | 'chat.1.local.findGeneralConvFromTeamID' | 'chat.1.local.forwardMessageConvSearch' | 'chat.1.local.forwardMessageNonblock' | 'chat.1.local.getBotMemberSettings' | 'chat.1.local.getDefaultTeamChannelsLocal' | 'chat.1.local.getGlobalAppNotificationSettingsLocal' | 'chat.1.local.getInboxAndUnboxUILocal' | 'chat.1.local.getLastActiveAtMultiLocal' | 'chat.1.local.getLastActiveForTeams' | 'chat.1.local.getMutualTeamsLocal' | 'chat.1.local.getNextAttachmentMessageLocal' | 'chat.1.local.getRecentJoinsLocal' | 'chat.1.local.getStaticConfig' | 'chat.1.local.getTeamRetentionLocal' | 'chat.1.local.getTeamRoleInConversation' | 'chat.1.local.getTLFConversationsLocal' | 'chat.1.local.getUnfurlSettings' | 'chat.1.local.getUnreadline' | 'chat.1.local.getUploadTempFile' | 'chat.1.local.getWelcomeMessage' | 'chat.1.local.ignorePinnedMessage' | 'chat.1.local.joinConversationByIDLocal' | 'chat.1.local.leaveConversationLocal' | 'chat.1.local.listPublicBotCommandsLocal' | 'chat.1.local.locationUpdate' | 'chat.1.local.makeAudioPreview' | 'chat.1.local.makeUploadTempFile' | 'chat.1.local.markAsReadLocal' | 'chat.1.local.markTLFAsReadLocal' | 'chat.1.local.newConversationLocal' | 'chat.1.local.pinMessage' | 'chat.1.local.postDeleteHistoryByAge' | 'chat.1.local.postDeleteNonblock' | 'chat.1.local.postEditNonblock' | 'chat.1.local.postFileAttachmentLocalNonblock' | 'chat.1.local.postHeadline' | 'chat.1.local.postHeadlineNonblock' | 'chat.1.local.postMetadata' | 'chat.1.local.postReactionNonblock' | 'chat.1.local.previewConversationByIDLocal' | 'chat.1.local.putReacjiSkinTone' | 'chat.1.local.refreshParticipants' | 'chat.1.local.removeBotMember' | 'chat.1.local.removeEmoji' | 'chat.1.local.removeFromConversationLocal' | 'chat.1.local.requestInboxLayout' | 'chat.1.local.requestInboxSmallIncrease' | 'chat.1.local.requestInboxSmallReset' | 'chat.1.local.requestInboxUnbox' | 'chat.1.local.resolveMaybeMention' | 'chat.1.local.resolveUnfurlPrompt' | 'chat.1.local.RetryPost' | 'chat.1.local.saveUnfurlSettings' | 'chat.1.local.setAppNotificationSettingsLocal' | 'chat.1.local.setBotMemberSettings' | 'chat.1.local.SetConversationStatusLocal' | 'chat.1.local.setConvMinWriterRoleLocal' | 'chat.1.local.setConvRetentionLocal' | 'chat.1.local.setDefaultTeamChannelsLocal' | 'chat.1.local.setGlobalAppNotificationSettingsLocal' | 'chat.1.local.setTeamRetentionLocal' | 'chat.1.local.setWelcomeMessage' | 'chat.1.local.simpleSearchInboxConvNames' | 'chat.1.local.toggleEmojiAnimations' | 'chat.1.local.toggleMessageCollapse' | 'chat.1.local.trackGiphySelect' | 'chat.1.local.unboxMobilePushNotification' | 'chat.1.local.unfurlPreviewLocal' | 'chat.1.local.unpinMessage' | 'chat.1.local.updateTyping' | 'chat.1.local.updateUnsentText' | 'chat.1.local.userEmojis' export type RpcFn = [RpcIn] extends [undefined] ? (params?: undefined, waitingKey?: WaitingKey) => Promise> : (params: RpcIn, waitingKey?: WaitingKey) => Promise> @@ -1574,6 +1578,7 @@ export type UnfurlGiphyDisplay = {readonly favicon?: UnfurlImageDisplay | null,r export type UnfurlGiphyRaw = {readonly imageUrl?: string | null,readonly video?: UnfurlVideo | null,readonly faviconUrl?: string | null,} export type UnfurlImageDisplay = {readonly url: string,readonly height: number,readonly width: number,readonly isVideo: boolean,} export type UnfurlMapsRaw = {readonly title: string,readonly url: string,readonly siteName: string,readonly imageUrl: string,readonly historyImageUrl?: string | null,readonly description: string,readonly coord: Coordinate,readonly time: Gregor1.Time,readonly liveLocationEndTime?: Gregor1.Time | null,readonly liveLocationDone: boolean,} +export type UnfurlPreviewInfo = {readonly url: string,readonly unfurl: UnfurlDisplay,} export type UnfurlPromptResult ={ actionType: UnfurlPromptAction.always } | { actionType: UnfurlPromptAction.never } | { actionType: UnfurlPromptAction.notnow } | { actionType: UnfurlPromptAction.accept, accept: string } | { actionType: UnfurlPromptAction.onetime, onetime: string } export type UnfurlRaw ={ unfurlType: UnfurlType.generic, generic: UnfurlGenericRaw } | { unfurlType: UnfurlType.youtube, youtube: UnfurlYoutubeRaw } | { unfurlType: UnfurlType.giphy, giphy: UnfurlGiphyRaw } | { unfurlType: UnfurlType.maps, maps: UnfurlMapsRaw } export type UnfurlResult = {readonly unfurl: Unfurl,readonly url: string,} @@ -1699,6 +1704,7 @@ export const localToggleEmojiAnimationsRpcPromise = createRpc('chat.1.local.togg export const localToggleMessageCollapseRpcPromise = createRpc('chat.1.local.toggleMessageCollapse') export const localTrackGiphySelectRpcPromise = createRpc('chat.1.local.trackGiphySelect') export const localUnboxMobilePushNotificationRpcPromise = createRpc('chat.1.local.unboxMobilePushNotification') +export const localUnfurlPreviewLocalRpcPromise = createRpc('chat.1.local.unfurlPreviewLocal') export const localUnpinMessageRpcPromise = createRpc('chat.1.local.unpinMessage') export const localUpdateTypingRpcPromise = createRpc('chat.1.local.updateTyping') export const localUpdateUnsentTextRpcPromise = createRpc('chat.1.local.updateUnsentText') diff --git a/shared/tsconfig.native.json b/shared/tsconfig.native.json index 399783d67966..3ac69ddc1e79 100644 --- a/shared/tsconfig.native.json +++ b/shared/tsconfig.native.json @@ -18,6 +18,7 @@ "./common-adapters/icon.constants-gen.native.tsx", "./common-adapters/icon.constants-gen.shared.tsx", "./chat/conversation/normal/container.test.tsx", + "./chat/conversation/input-area/unfurl-preview.test.tsx", "./chat/conversation/messages/system-users-added-to-conv/container.test.tsx", "./chat/conversation/messages/text/coinflip/results.test.tsx", "./common-adapters/markdown/index.test.tsx", From 02bd7fdbafc8b087d88d64716919608c6c19788f Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 24 Aug 2026 15:32:27 -0400 Subject: [PATCH 02/17] feat(chat): float the link preview and page through multiple unfurls The preview was a full-width strip in flow above the composer, so showing one shifted the whole message list up. It is now an absolutely positioned panel anchored to the input's top edge: left aligned, shrink to fit, rounded border and a shadow, with a small gap above the input. Nothing in the thread moves when a preview appears or goes away. A message can contain several links, so the panel shows one card at a time with arrows in its top left to page between them. Every card renders into the same CSS grid cell, which keeps the panel sized to the largest one so paging never resizes it; the inactive cards are only hidden, which also keeps them out of the tab order and unclickable. The grid stacking is desktop only. Mobile still renders nothing here, and when it lands it needs the cross platform equivalent: measure each card with onLayout and hold the container at the max. gridArea joins the desktop style allowlist, which was only missing it. --- .../conversation/input-area/normal/index.tsx | 45 ++++---- .../input-area/unfurl-preview.tsx | 101 +++++++++++++++--- shared/styles/css.d.ts | 1 + 3 files changed, 113 insertions(+), 34 deletions(-) diff --git a/shared/chat/conversation/input-area/normal/index.tsx b/shared/chat/conversation/input-area/normal/index.tsx index aaf466d5f6e3..df165d77e432 100644 --- a/shared/chat/conversation/input-area/normal/index.tsx +++ b/shared/chat/conversation/input-area/normal/index.tsx @@ -322,25 +322,34 @@ const ConnectedPlatformInput = function ConnectedPlatformInput() { setInputRef(inputRef.current) }, [setInputRef]) + const input = ( + + ) + + if (isMobile) { + return input + } + + // the preview floats out of this box, so it needs a positioned ancestor return ( - <> - {isMobile ? null : } - - + + + {input} + ) } diff --git a/shared/chat/conversation/input-area/unfurl-preview.tsx b/shared/chat/conversation/input-area/unfurl-preview.tsx index 2a3fda31f7fc..c43751e2501c 100644 --- a/shared/chat/conversation/input-area/unfurl-preview.tsx +++ b/shared/chat/conversation/input-area/unfurl-preview.tsx @@ -1,4 +1,5 @@ import * as Kb from '@/common-adapters' +import * as React from 'react' import * as T from '@/constants/types' import UnfurlGenericView from '@/chat/conversation/messages/text/unfurl/unfurl-list/generic-view' import {useUnfurlPreviews} from '@/chat/conversation/unfurl-preview-state' @@ -11,7 +12,9 @@ type Props = { const UnfurlPreview = (p: Props) => { const {conversationIDKey, text} = p const styles = useStyles() + const theme = Kb.Styles.useTheme() const {dismiss, previews} = useUnfurlPreviews(conversationIDKey, text) + const [index, setIndex] = React.useState(0) const genericPreviews = previews.flatMap(preview => { const {unfurl} = preview if (unfurl.unfurlType !== T.RPCChat.UnfurlType.generic || unfurl.generic.mapInfo) { @@ -21,24 +24,64 @@ const UnfurlPreview = (p: Props) => { } return [{generic: unfurl.generic, preview}] }) - if (isMobile || !genericPreviews.length) { + // dismissing the last card, or the text losing a link, shrinks the list under us + const clamped = Math.min(index, Math.max(genericPreviews.length - 1, 0)) + const shown = genericPreviews[clamped] + if (isMobile || !shown) { return null } - return ( - - {genericPreviews.map(({preview, generic}) => ( - dismiss(preview.url)} - publishTime={generic.publishTime ?? undefined} - siteName={generic.siteName} - title={generic.title} - url={generic.url} + const onPrevious = () => { + setIndex(clamped - 1) + } + const onNext = () => { + setIndex(clamped + 1) + } + const pager = + genericPreviews.length > 1 ? ( + + 0 ? onPrevious : undefined} + color={clamped > 0 ? undefined : theme.black_20} + padding="xtiny" /> - ))} + {`${clamped + 1}/${genericPreviews.length}`} + + + ) : null + return ( + + {pager} + + {genericPreviews.map(({generic, preview}, i) => ( + // every card occupies the same grid cell, so the stack is always as big as the + // largest one and paging never resizes the panel. the inactive ones are only + // hidden, which also keeps them out of the tab order and unclickable. + + dismiss(preview.url)} + publishTime={generic.publishTime ?? undefined} + siteName={generic.siteName} + title={generic.title} + url={generic.url} + /> + + ))} + ) } @@ -48,12 +91,38 @@ const useStyles = Kb.Styles.createStyleHook( ({ container: Kb.Styles.platformStyles({ isElectron: { - backgroundColor: theme.blueGrey, + ...Kb.Styles.desktopStyles.boxShadow, + // floats over the thread instead of taking flow space, so showing a preview + // never shifts the message list + backgroundColor: theme.white, + borderColor: theme.black_10, + borderRadius: Kb.Styles.borderRadius, + borderStyle: 'solid', + borderWidth: 1, + bottom: '100%', + left: Kb.Styles.globalMargins.small, + marginBottom: Kb.Styles.globalMargins.xtiny, maxHeight: 200, + maxWidth: 500, overflowY: 'auto', padding: Kb.Styles.globalMargins.tiny, + position: 'absolute', }, }), + cell: Kb.Styles.platformStyles({ + // grid items stretch by default, which would drop a short card into the middle of + // the tallest one's space. the track still sizes to the tallest either way. + isElectron: {alignSelf: 'start', gridArea: '1 / 1'}, + }), + cellHidden: Kb.Styles.platformStyles({ + isElectron: {visibility: 'hidden'}, + }), + pager: Kb.Styles.platformStyles({ + isElectron: {alignItems: 'center'}, + }), + stack: Kb.Styles.platformStyles({ + isElectron: {display: 'grid'}, + }), }) as const ) diff --git a/shared/styles/css.d.ts b/shared/styles/css.d.ts index 398830bd51c2..311423bf5651 100644 --- a/shared/styles/css.d.ts +++ b/shared/styles/css.d.ts @@ -66,6 +66,7 @@ type StyleKeys = | 'fontStyle' | 'fontVariant' | 'fontWeight' + | 'gridArea' | 'height' | 'inset' | 'justifyContent' From 17836b50c6b50aa5f78e8e761d663f9b1b7273e3 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 24 Aug 2026 16:43:02 -0400 Subject: [PATCH 03/17] feat(chat): show the link preview on mobile, fix the panel sizing Mobile rendered nothing at all: the hook was never called and the composer never fed it text. It now renders in flow above the input, inside KeyboardStickyView so it rides the keyboard, the way the reply preview already sits. The card area is a fixed size instead of hugging its content, so paging between cards of different shapes cannot resize the panel. That replaces the css grid that was holding the panel at the largest card: the grid sized the stack to the tallest card, which left a short card overflowing its own box and showing a scrollbar it did not need. It also drops the gridArea style allowlist entry, so css.d.ts is back to what master has. Only the card area scrolls now; the pager sits above it and stays put. Native clips at a fixed height rather than scrolling, so it gets a real ScrollView; desktop scrolls with overflowY. Both axes are set explicitly because the overflow shorthand would also set the y axis and beat the scroll depending on emission order. Drops the test that claimed to cover the mobile branch. Kb.Box2 renders a react-native Pressable when isMobile is set, which produces no DOM under jsdom, so it passed whether or not the guard it was testing existed. The mobile layout is verified on a device. --- .../conversation/input-area/normal/index.tsx | 16 ++- .../input-area/unfurl-preview.test.tsx | 15 +-- .../input-area/unfurl-preview.tsx | 100 +++++++++--------- shared/styles/css.d.ts | 1 - 4 files changed, 66 insertions(+), 66 deletions(-) diff --git a/shared/chat/conversation/input-area/normal/index.tsx b/shared/chat/conversation/input-area/normal/index.tsx index df165d77e432..ac55cadb5620 100644 --- a/shared/chat/conversation/input-area/normal/index.tsx +++ b/shared/chat/conversation/input-area/normal/index.tsx @@ -246,9 +246,7 @@ const ConnectedPlatformInput = function ConnectedPlatformInput() { if (currentMeta) { metasReceived([{...currentMeta, draft: text}], undefined, {force: true}) } - if (!isMobile) { - setPreviewText(text) - } + setPreviewText(text) const f = async () => { await T.RPCChat.localUpdateUnsentTextRpcPromise({ conversationID: convoID, @@ -340,14 +338,22 @@ const ConnectedPlatformInput = function ConnectedPlatformInput() { /> ) + const preview = + if (isMobile) { - return input + // in flow above the composer; it rides KeyboardStickyView with the input + return ( + <> + {preview} + {input} + + ) } // the preview floats out of this box, so it needs a positioned ancestor return ( - + {preview} {input} ) diff --git a/shared/chat/conversation/input-area/unfurl-preview.test.tsx b/shared/chat/conversation/input-area/unfurl-preview.test.tsx index 938ce887b4ff..c37eb939380c 100644 --- a/shared/chat/conversation/input-area/unfurl-preview.test.tsx +++ b/shared/chat/conversation/input-area/unfurl-preview.test.tsx @@ -31,6 +31,9 @@ const mapInfo: T.RPCChat.UnfurlPreviewInfo = { url: 'http://map.com', } as T.RPCChat.UnfurlPreviewInfo +// these render the desktop tree only: Kb.Box2 renders a react-native Pressable when +// isMobile is set, which produces no DOM under jsdom, so flipping that global here would +// assert nothing. the mobile layout is verified on a device. describe('UnfurlPreview', () => { afterEach(() => { mockPreviews = [] @@ -53,16 +56,4 @@ describe('UnfurlPreview', () => { const {container} = render() expect(container.firstChild).toBeNull() }) - - it('renders nothing on mobile even with a generic preview', () => { - mockPreviews = [genericInfo] - const originalIsMobile = global.isMobile - global.isMobile = true - try { - const {container} = render() - expect(container.firstChild).toBeNull() - } finally { - global.isMobile = originalIsMobile - } - }) }) diff --git a/shared/chat/conversation/input-area/unfurl-preview.tsx b/shared/chat/conversation/input-area/unfurl-preview.tsx index c43751e2501c..ddb9cf36be87 100644 --- a/shared/chat/conversation/input-area/unfurl-preview.tsx +++ b/shared/chat/conversation/input-area/unfurl-preview.tsx @@ -27,61 +27,63 @@ const UnfurlPreview = (p: Props) => { // dismissing the last card, or the text losing a link, shrinks the list under us const clamped = Math.min(index, Math.max(genericPreviews.length - 1, 0)) const shown = genericPreviews[clamped] - if (isMobile || !shown) { + if (!shown) { return null } + const {generic, preview} = shown const onPrevious = () => { setIndex(clamped - 1) } const onNext = () => { setIndex(clamped + 1) } + const atStart = clamped === 0 + const atEnd = clamped === genericPreviews.length - 1 const pager = genericPreviews.length > 1 ? ( 0 ? onPrevious : undefined} - color={clamped > 0 ? undefined : theme.black_20} + onClick={atStart ? undefined : onPrevious} + color={atStart ? theme.black_20 : undefined} padding="xtiny" /> {`${clamped + 1}/${genericPreviews.length}`} ) : null + const card = ( + dismiss(preview.url)} + publishTime={generic.publishTime ?? undefined} + siteName={generic.siteName} + title={generic.title} + url={generic.url} + /> + ) return ( {pager} - - {genericPreviews.map(({generic, preview}, i) => ( - // every card occupies the same grid cell, so the stack is always as big as the - // largest one and paging never resizes the panel. the inactive ones are only - // hidden, which also keeps them out of the tab order and unclickable. - - dismiss(preview.url)} - publishTime={generic.publishTime ?? undefined} - siteName={generic.siteName} - title={generic.title} - url={generic.url} - /> - - ))} - + {/* the card area is a fixed size, so paging between cards never resizes the panel. + only this part scrolls; the pager above it stays put */} + {isMobile ? ( + // native clips at a fixed height rather than scrolling, so it needs a real scroller + {card} + ) : ( + + {card} + + )} ) } @@ -89,39 +91,41 @@ const UnfurlPreview = (p: Props) => { const useStyles = Kb.Styles.createStyleHook( theme => ({ + cardArea: Kb.Styles.platformStyles({ + common: {height: 200}, + // per axis rather than the shorthand: `overflow: hidden` would also set the y axis + // and beat the scroll depending on emission order + isElectron: {overflowX: 'hidden', overflowY: 'auto', width: 420}, + isMobile: {flexGrow: 0, flexShrink: 0}, + }), container: Kb.Styles.platformStyles({ + common: { + backgroundColor: theme.white, + borderColor: theme.black_10, + borderRadius: Kb.Styles.borderRadius, + borderWidth: 1, + padding: Kb.Styles.globalMargins.tiny, + }, isElectron: { ...Kb.Styles.desktopStyles.boxShadow, // floats over the thread instead of taking flow space, so showing a preview // never shifts the message list - backgroundColor: theme.white, - borderColor: theme.black_10, - borderRadius: Kb.Styles.borderRadius, borderStyle: 'solid', - borderWidth: 1, bottom: '100%', left: Kb.Styles.globalMargins.small, marginBottom: Kb.Styles.globalMargins.xtiny, - maxHeight: 200, - maxWidth: 500, - overflowY: 'auto', - padding: Kb.Styles.globalMargins.tiny, position: 'absolute', }, - }), - cell: Kb.Styles.platformStyles({ - // grid items stretch by default, which would drop a short card into the middle of - // the tallest one's space. the track still sizes to the tallest either way. - isElectron: {alignSelf: 'start', gridArea: '1 / 1'}, - }), - cellHidden: Kb.Styles.platformStyles({ - isElectron: {visibility: 'hidden'}, + isMobile: { + // in flow above the composer, the way the reply preview already sits + alignSelf: 'stretch', + marginBottom: Kb.Styles.globalMargins.xtiny, + marginLeft: Kb.Styles.globalMargins.tiny, + marginRight: Kb.Styles.globalMargins.tiny, + }, }), pager: Kb.Styles.platformStyles({ - isElectron: {alignItems: 'center'}, - }), - stack: Kb.Styles.platformStyles({ - isElectron: {display: 'grid'}, + common: {alignItems: 'center'}, }), }) as const ) diff --git a/shared/styles/css.d.ts b/shared/styles/css.d.ts index 311423bf5651..398830bd51c2 100644 --- a/shared/styles/css.d.ts +++ b/shared/styles/css.d.ts @@ -66,7 +66,6 @@ type StyleKeys = | 'fontStyle' | 'fontVariant' | 'fontWeight' - | 'gridArea' | 'height' | 'inset' | 'justifyContent' From e71cb0901a6ab0c26fe40fc246b78a4d998353a3 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 24 Aug 2026 17:01:49 -0400 Subject: [PATCH 04/17] fix(chat): keep dismissed link previews across a conversation switch The input subtree is keyed on the conversation, so returning to a conversation mounts a fresh hook whose first render has empty text, before the draft is restored. The effect read that as "no links" and cleared every dismissed url for the conversation, so dismissing a card, switching away and switching back brought the card straight back and the link unfurled on send. Only prune once real text has been seen. The existing conversation-switch test could not catch this: it re-renders one mounted instance, where the real switch remounts. The new test drives an unmount and remount instead, and fails without the fix. Also from review: postTextNonblock now says so when it is handed unfurlSuppress with no outboxID. Suppression is keyed by outbox ID, and the deliverer generates its own when the caller supplies none, so that combination silently unfurled a link the sender had dismissed. No current caller does this. The pager had no coverage; it gets the counter, the arrows being inert at each end, and re-clamping when the shown card is dismissed away. Editing a message drops unfurlSuppress because postEditNonblock has no such parameter, so the dismiss control does nothing there. Noted at the call site rather than left for someone to rediscover. --- go/chat/server.go | 11 +++- .../input-area/unfurl-preview.test.tsx | 54 ++++++++++++++++++- shared/chat/conversation/send-actions.tsx | 3 ++ .../unfurl-preview-state.test.tsx | 21 ++++++++ .../conversation/unfurl-preview-state.tsx | 11 +++- 5 files changed, 95 insertions(+), 5 deletions(-) diff --git a/go/chat/server.go b/go/chat/server.go index 801cf11aa9c2..24ba107ce759 100644 --- a/go/chat/server.go +++ b/go/chat/server.go @@ -1005,8 +1005,15 @@ func (h *Server) PostTextNonblock(ctx context.Context, arg chat1.PostTextNonbloc }) } - if len(arg.UnfurlSuppress) > 0 && arg.OutboxID != nil { - h.G().Unfurler.SetSuppressed(ctx, *arg.OutboxID, arg.UnfurlSuppress) + if len(arg.UnfurlSuppress) > 0 { + // suppression is keyed by outbox ID, and without one from the caller the deliverer + // generates its own, which we never see here. say so rather than dropping the + // caller's intent silently and unfurling a link they asked us not to. + if arg.OutboxID == nil { + h.Debug(ctx, "PostTextNonblock: ignoring unfurlSuppress, no outboxID supplied") + } else { + h.G().Unfurler.SetSuppressed(ctx, *arg.OutboxID, arg.UnfurlSuppress) + } } var parg chat1.PostLocalNonblockArg diff --git a/shared/chat/conversation/input-area/unfurl-preview.test.tsx b/shared/chat/conversation/input-area/unfurl-preview.test.tsx index c37eb939380c..cc37366f8edf 100644 --- a/shared/chat/conversation/input-area/unfurl-preview.test.tsx +++ b/shared/chat/conversation/input-area/unfurl-preview.test.tsx @@ -1,7 +1,7 @@ /** @jest-environment jsdom */ /// import * as T from '@/constants/types' -import {render} from '@testing-library/react' +import {fireEvent, render} from '@testing-library/react' import UnfurlPreview from './unfurl-preview' const mockDismiss = jest.fn() @@ -18,8 +18,13 @@ const nonGenericInfo: T.RPCChat.UnfurlPreviewInfo = { url: 'http://youtube.com/watch', } as T.RPCChat.UnfurlPreviewInfo +const genericInfo2: T.RPCChat.UnfurlPreviewInfo = { + unfurl: {generic: {siteName: 'b', title: 'Bravo', url: 'http://b.com'}, unfurlType: T.RPCChat.UnfurlType.generic}, + url: 'http://b.com', +} as T.RPCChat.UnfurlPreviewInfo + const genericInfo: T.RPCChat.UnfurlPreviewInfo = { - unfurl: {generic: {siteName: 'a', title: 'a', url: 'http://a.com'}, unfurlType: T.RPCChat.UnfurlType.generic}, + unfurl: {generic: {siteName: 'a', title: 'Alpha', url: 'http://a.com'}, unfurlType: T.RPCChat.UnfurlType.generic}, url: 'http://a.com', } as T.RPCChat.UnfurlPreviewInfo @@ -56,4 +61,49 @@ describe('UnfurlPreview', () => { const {container} = render() expect(container.firstChild).toBeNull() }) + + it('shows no pager for a single preview', () => { + mockPreviews = [genericInfo] + const {queryByText} = render() + expect(queryByText('1/1')).toBeNull() + }) + + it('pages between previews and disables the arrows at each end', () => { + mockPreviews = [genericInfo, genericInfo2] + const {getByText, container} = render( + + ) + expect(getByText('1/2')).toBeTruthy() + // the card shown is the first one + expect(container.textContent).toContain('Alpha') + + const left = container.querySelector('.icon-gen-iconfont-arrow-left') as Element + const right = container.querySelector('.icon-gen-iconfont-arrow-right') as Element + + // at the start the left arrow does nothing + fireEvent.click(left) + expect(getByText('1/2')).toBeTruthy() + + fireEvent.click(right) + expect(getByText('2/2')).toBeTruthy() + expect(container.textContent).toContain('Bravo') + + // at the end the right arrow does nothing + fireEvent.click(right) + expect(getByText('2/2')).toBeTruthy() + }) + + it('re-clamps the index when the shown card is dismissed away', () => { + mockPreviews = [genericInfo, genericInfo2] + const {getByText, container, rerender} = render( + + ) + fireEvent.click(container.querySelector('.icon-gen-iconfont-arrow-right') as Element) + expect(getByText('2/2')).toBeTruthy() + + // the second preview goes away; the index must fall back rather than blank the panel + mockPreviews = [genericInfo] + rerender() + expect(container.textContent).toContain('Alpha') + }) }) diff --git a/shared/chat/conversation/send-actions.tsx b/shared/chat/conversation/send-actions.tsx index ea3c0cc9340e..4047a93c853d 100644 --- a/shared/chat/conversation/send-actions.tsx +++ b/shared/chat/conversation/send-actions.tsx @@ -143,6 +143,9 @@ export const useConversationSendActions = () => { ) => { const editOrdinal = context?.editingOrdinal if (editOrdinal) { + // unfurlSuppress is dropped here on purpose: postEditNonblock has no such param, so + // dismissing a preview card while editing cannot reach the service. carrying it + // would need the protocol change, not just plumbing on this side. editMessage(editOrdinal, text) return } diff --git a/shared/chat/conversation/unfurl-preview-state.test.tsx b/shared/chat/conversation/unfurl-preview-state.test.tsx index 438e7825ac25..09baec607dbe 100644 --- a/shared/chat/conversation/unfurl-preview-state.test.tsx +++ b/shared/chat/conversation/unfurl-preview-state.test.tsx @@ -132,4 +132,25 @@ describe('unfurl previews', () => { act(() => resolveSecond?.([info('http://c.com')])) await waitFor(() => expect(last?.previews[0]?.url).toBe('http://c.com')) }) + + it('keeps dismissals when the conversation is left and returned to', async () => { + jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise').mockResolvedValue([info('http://a.com')]) + let last: ReturnType | undefined + const first = render( (last = r)} />) + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(last?.previews.length).toBe(1)) + act(() => last?.dismiss('http://a.com')) + expect(getSuppressedURLs(convID)).toEqual(['http://a.com']) + + // switching conversations unmounts this subtree: the provider is keyed on the + // conversation, so coming back mounts a fresh hook whose first render has no text yet + first.unmount() + render( (last = r)} />) + act(() => { + jest.advanceTimersByTime(500) + }) + expect(getSuppressedURLs(convID)).toEqual(['http://a.com']) + }) }) diff --git a/shared/chat/conversation/unfurl-preview-state.tsx b/shared/chat/conversation/unfurl-preview-state.tsx index 98b896441289..3a7203d7077d 100644 --- a/shared/chat/conversation/unfurl-preview-state.tsx +++ b/shared/chat/conversation/unfurl-preview-state.tsx @@ -102,6 +102,11 @@ export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, t const dismissedSet = useUnfurlPreviewState(s => s.dismissed.get(conversationIDKey)) const {dismiss: dismissURL, keepOnly} = useUnfurlPreviewState(s => s.dispatch) const requestIDRef = React.useRef(0) + // the input subtree remounts per conversation (key={conversationIDKey} on the provider), + // so the first render of a conversation we return to always has empty text before the + // draft is restored. clearing dismissals on that would throw away what the user + // dismissed before switching away, so only prune once real text has been seen. + const sawTextRef = React.useRef(false) const hasLink = text.includes('http') const onFetched = React.useCallback( @@ -118,9 +123,13 @@ export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, t React.useEffect(() => { const id = ++requestIDRef.current if (!text.includes('http')) { - keepOnly(conversationIDKey, []) + if (sawTextRef.current) { + keepOnly(conversationIDKey, []) + } + sawTextRef.current = sawTextRef.current || !!text return } + sawTextRef.current = true const timeoutID = setTimeout(() => { ignorePromise(fetchPreviews(conversationIDKey, text, id, requestIDRef, onFetched)) }, debounceMS) From d154395d9be3ff299da9b78678501e17d19c1cbc Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 24 Aug 2026 17:11:06 -0400 Subject: [PATCH 05/17] fix(chat): only show a preview while its link is still in the composer A card stayed on screen after the link it described was edited out. The fetch that would have replaced it can fail or still be in flight, and nothing reset the previous result, so the composer could read one url while the card above it described another. Clicking that card's X then suppressed a link that was not in the message, and the link that was about to send never got offered a dismiss at all. Previews are now filtered against the current composer text, so a card survives only as long as its url does. That also makes the failure path honest: a fetch that throws shows nothing rather than leaving the last success on screen. This replaces the conversation masking that used to guard the same memo. Two reviewers independently found it unreachable: the input subtree is keyed on the conversation, so a hook instance never sees a second conversation, and the test defending it drove a rerender where the real app remounts. Filtering on the text covers the case that can actually happen. Adds the coverage those reviews found missing: clicking the close icon dismisses the url of the card on screen rather than the first one, and the generic-only/no-maps rule the frontend relies on is now a named predicate with a test, instead of an inline condition asserted nowhere. --- go/chat/unfurl/unfurler.go | 20 +++++++++----- go/chat/unfurl/unfurler_test.go | 16 ++++++++++++ .../input-area/unfurl-preview.test.tsx | 15 +++++++++++ .../unfurl-preview-state.test.tsx | 20 +++++--------- .../conversation/unfurl-preview-state.tsx | 26 ++++++++----------- 5 files changed, 62 insertions(+), 35 deletions(-) diff --git a/go/chat/unfurl/unfurler.go b/go/chat/unfurl/unfurler.go index 17dab1859eda..362ab17455ed 100644 --- a/go/chat/unfurl/unfurler.go +++ b/go/chat/unfurl/unfurler.go @@ -379,6 +379,18 @@ func (u *Unfurler) Prefetch(ctx context.Context, uid gregor1.UID, convID chat1.C // PreviewURLs scrapes and packages the whitelisted URLs in text and returns // display-ready unfurls, so a client can show a preview before sending. Only // generic unfurls are returned; failures are skipped rather than returned. +// previewable reports whether an unfurl can be shown in the pre-send preview. only generic +// unfurls render there, and a map unfurl is a generic unfurl with MapInfo set, which the +// message view itself refuses to render. the frontend filters on the same rule, so keep the +// two in step. +func previewable(unfurl chat1.Unfurl) bool { + typ, err := unfurl.UnfurlType() + if err != nil || typ != chat1.UnfurlType_GENERIC { + return false + } + return unfurl.Generic().MapInfo == nil +} + func (u *Unfurler) PreviewURLs(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, text string, ) (res []chat1.UnfurlPreviewInfo) { @@ -399,13 +411,7 @@ func (u *Unfurler) PreviewURLs(ctx context.Context, uid gregor1.UID, convID chat u.Debug(ctx, "PreviewURLs: unable to scrapeAndPackage: %s", err) continue } - typ, err := unfurl.UnfurlType() - if err != nil || typ != chat1.UnfurlType_GENERIC { - continue - } - // a map unfurl is a generic unfurl with MapInfo set, and the message - // view refuses to render those, so don't preview them either - if unfurl.Generic().MapInfo != nil { + if !previewable(unfurl) { continue } disp, err := display.DisplayUnfurl(ctx, u.G().AttachmentURLSrv, convID, unfurl) diff --git a/go/chat/unfurl/unfurler_test.go b/go/chat/unfurl/unfurler_test.go index 9ba10827237f..9d8aa4bc9172 100644 --- a/go/chat/unfurl/unfurler_test.go +++ b/go/chat/unfurl/unfurler_test.go @@ -291,3 +291,19 @@ func TestUnfurlerSuppress(t *testing.T) { require.Fail(t, "no unfurl message sent") } } + +func TestPreviewable(t *testing.T) { + generic := chat1.NewUnfurlWithGeneric(chat1.UnfurlGeneric{Title: "t"}) + require.True(t, previewable(generic)) + + // a map is a generic unfurl carrying MapInfo; the message view will not render one, + // so the preview must not either + mapped := chat1.NewUnfurlWithGeneric(chat1.UnfurlGeneric{ + Title: "here", + MapInfo: &chat1.UnfurlGenericMapInfo{IsLiveLocationDone: true}, + }) + require.False(t, previewable(mapped)) + + require.False(t, previewable(chat1.NewUnfurlWithYoutube(chat1.UnfurlYoutube{}))) + require.False(t, previewable(chat1.NewUnfurlWithGiphy(chat1.UnfurlGiphy{}))) +} diff --git a/shared/chat/conversation/input-area/unfurl-preview.test.tsx b/shared/chat/conversation/input-area/unfurl-preview.test.tsx index cc37366f8edf..f96b5bd85f17 100644 --- a/shared/chat/conversation/input-area/unfurl-preview.test.tsx +++ b/shared/chat/conversation/input-area/unfurl-preview.test.tsx @@ -106,4 +106,19 @@ describe('UnfurlPreview', () => { rerender() expect(container.textContent).toContain('Alpha') }) + + it('dismisses the shown card by its url when the close icon is clicked', () => { + mockPreviews = [genericInfo, genericInfo2] + const {container} = render( + + ) + fireEvent.click(container.querySelector('.icon-gen-iconfont-close') as Element) + expect(mockDismiss).toHaveBeenCalledWith('http://a.com') + + // and after paging it dismisses the one actually on screen, not the first + mockDismiss.mockClear() + fireEvent.click(container.querySelector('.icon-gen-iconfont-arrow-right') as Element) + fireEvent.click(container.querySelector('.icon-gen-iconfont-close') as Element) + expect(mockDismiss).toHaveBeenCalledWith('http://b.com') + }) }) diff --git a/shared/chat/conversation/unfurl-preview-state.test.tsx b/shared/chat/conversation/unfurl-preview-state.test.tsx index 09baec607dbe..343d95e112b8 100644 --- a/shared/chat/conversation/unfurl-preview-state.test.tsx +++ b/shared/chat/conversation/unfurl-preview-state.test.tsx @@ -8,7 +8,6 @@ import {useUnfurlPreviews, getSuppressedURLs, useUnfurlPreviewState} from './unf // T.Chat.keyToConversationID (used to build the RPC's convID param), so build // the fixture the way input-state.test.tsx does: round-trip through bytes. const convID = T.Chat.conversationIDToKey(new Uint8Array([1, 2, 3, 4])) -const otherConvID = T.Chat.conversationIDToKey(new Uint8Array([5, 6, 7, 8])) const info = (url: string): T.RPCChat.UnfurlPreviewInfo => ({unfurl: {generic: {title: url, url}, unfurlType: T.RPCChat.UnfurlType.generic}, url}) as T.RPCChat.UnfurlPreviewInfo @@ -106,13 +105,10 @@ describe('unfurl previews', () => { await waitFor(() => expect(getSuppressedURLs(convID)).toEqual([])) }) - it('does not flash the previous conversation preview after switching conversations', async () => { + it('drops a card once its url leaves the composer, even if the next fetch fails', async () => { const spy = jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise') spy.mockResolvedValueOnce([info('http://a.com')]) - let resolveSecond: ((v: Array) => void) | undefined - spy.mockImplementationOnce( - async () => new Promise>(resolve => (resolveSecond = resolve)) - ) + spy.mockRejectedValueOnce(new Error('scrape failed')) let last: ReturnType | undefined const {rerender} = render( (last = r)} />) act(() => { @@ -120,17 +116,15 @@ describe('unfurl previews', () => { }) await waitFor(() => expect(last?.previews[0]?.url).toBe('http://a.com')) - // switch to a different conversation whose draft also contains a link - rerender( (last = r)} />) - // conv A's preview must be gone immediately, before the new conversation's debounce even fires + // the user replaces the link; the fetch for the new one fails, so nothing ever + // overwrites the previous result. the old card must not stay on screen, or its X would + // suppress a url that is no longer in the message while the new one goes out unfurled + rerender( (last = r)} />) expect(last?.previews).toEqual([]) act(() => { jest.advanceTimersByTime(500) }) - // still no preview: the new conversation's fetch is in flight but unresolved - expect(last?.previews).toEqual([]) - act(() => resolveSecond?.([info('http://c.com')])) - await waitFor(() => expect(last?.previews[0]?.url).toBe('http://c.com')) + await waitFor(() => expect(last?.previews).toEqual([])) }) it('keeps dismissals when the conversation is left and returned to', async () => { diff --git a/shared/chat/conversation/unfurl-preview-state.tsx b/shared/chat/conversation/unfurl-preview-state.tsx index 3a7203d7077d..79b59416c984 100644 --- a/shared/chat/conversation/unfurl-preview-state.tsx +++ b/shared/chat/conversation/unfurl-preview-state.tsx @@ -17,6 +17,7 @@ type State = T.Immutable<{ export const useUnfurlPreviewState = Z.createZustand('unfurl-preview', set => ({ dismissed: new Map(), dispatch: { + // also the restore path after a canceled send: both mean "these urls are suppressed" dismiss: (conversationIDKey, urls) => { if (!urls.length) return set(s => { @@ -87,18 +88,14 @@ const fetchPreviews = async ( if (requestID !== requestIDRef.current) return onSuccess(conversationIDKey, res ?? []) } catch (e) { - // best-effort preview: an RPC failure just means no card shows, nothing for the user to act on + // best-effort preview: nothing is shown for a url whose fetch failed, since `visible` + // only surfaces previews whose url is still in the composer text logger.info('unfurl preview failed', e) } } -type FetchedPreviews = { - conversationIDKey: T.Chat.ConversationIDKey - previews: ReadonlyArray -} - export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, text: string) => { - const [fetched, setFetched] = React.useState({conversationIDKey, previews: []}) + const [fetched, setFetched] = React.useState>([]) const dismissedSet = useUnfurlPreviewState(s => s.dismissed.get(conversationIDKey)) const {dismiss: dismissURL, keepOnly} = useUnfurlPreviewState(s => s.dispatch) const requestIDRef = React.useRef(0) @@ -111,7 +108,7 @@ export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, t const onFetched = React.useCallback( (fetchedConversationIDKey: T.Chat.ConversationIDKey, infos: ReadonlyArray) => { - setFetched({conversationIDKey: fetchedConversationIDKey, previews: infos}) + setFetched(infos) keepOnly( fetchedConversationIDKey, infos.map(i => i.url) @@ -145,14 +142,13 @@ export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, t [conversationIDKey, dismissURL] ) - // mask stale previews from a since-switched conversation the same way `hasLink` masks - // text that no longer has a link, so a switch never flashes the previous conversation's card + // a card is only shown while its url is still in the composer. the fetch that would + // replace these previews can fail or still be in flight, and showing a card for a url the + // user has since deleted is worse than showing nothing: its X would suppress a link that + // is not in the message, while the link that is about to send never gets offered one. const visible = React.useMemo( - () => - hasLink && fetched.conversationIDKey === conversationIDKey - ? fetched.previews.filter(p => !dismissedSet?.has(p.url)) - : [], - [hasLink, fetched, conversationIDKey, dismissedSet] + () => (hasLink ? fetched.filter(p => text.includes(p.url) && !dismissedSet?.has(p.url)) : []), + [hasLink, fetched, text, dismissedSet] ) return {dismiss, previews: visible} } From 3116ba9b8253e2934e168f6d2f66c2e91af64e7c Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 24 Aug 2026 17:26:27 -0400 Subject: [PATCH 06/17] fix(chat): tie unfurl suppression to the message, not to a clock Dismissed urls were held in a five minute cache keyed by outbox id. A message that waited longer than that to send - offline, airplane mode, a flaky connection - lost them, and unfurled links the sender had explicitly dismissed. The bound existed to stop entries from a send that never happened piling up for the life of the process, so it was memory hygiene doubling as a correctness cliff. The urls now ride SenderSendOptions. PostLocalNonblock puts them there, Queue persists them on the outbox record, and the deliverer hands them back to BlockingSender.Send, which is already the caller of UnfurlAndSend. So they live exactly as long as the message they belong to, survive a restart, and need no cleanup: the outbox record going away takes them with it. UnfurlAndSend runs again when the user resolves an unfurl prompt for another url in the same message, and that pass has no outbox record left to read. The first pass writes a marker at the url's existing deterministic task key, and later passes skip on that, so a dismissal holds however long the prompt sits unanswered. Giphy and maps are untouched. They are never previewed, so never dismissed, and the send path still does not filter by unfurl type. Text sends go back to letting the deliverer assign the outbox id, since suppression no longer keys on it. That reverts an unannounced change to every text send, and removes the case where suppression was dropped in silence because no id had been supplied. --- go/chat/livelocation_test.go | 2 +- go/chat/maps/livelocation.go | 2 +- go/chat/sender.go | 5 +- go/chat/server.go | 23 +++-- go/chat/types/interfaces.go | 3 +- go/chat/types/types.go | 4 +- go/chat/unfurl/cache.go | 31 ++----- go/chat/unfurl/unfurler.go | 90 ++++++++++--------- go/chat/unfurl/unfurler_test.go | 31 +++---- go/protocol/chat1/extras.go | 7 ++ go/protocol/chat1/local.go | 13 +++ protocol/avdl/chat1/local.avdl | 5 +- protocol/json/chat1/local.json | 14 +++ .../input-area/input-state.test.tsx | 1 - shared/chat/conversation/send-actions.tsx | 2 +- shared/constants/rpc/rpc-chat-gen.tsx | 2 +- 16 files changed, 127 insertions(+), 108 deletions(-) diff --git a/go/chat/livelocation_test.go b/go/chat/livelocation_test.go index 459a8b0e3c70..eda8490ef58a 100644 --- a/go/chat/livelocation_test.go +++ b/go/chat/livelocation_test.go @@ -81,7 +81,7 @@ func (m *mockUnfurler) Prefetch(ctx context.Context, uid gregor1.UID, convID cha } func (m *mockUnfurler) UnfurlAndSend(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, - msg chat1.MessageUnboxed, + msg chat1.MessageUnboxed, suppress []string, ) { require.True(m.t, msg.IsValid()) body := msg.Valid().MessageBody diff --git a/go/chat/maps/livelocation.go b/go/chat/maps/livelocation.go index 76e24d886e23..54dd586b89bf 100644 --- a/go/chat/maps/livelocation.go +++ b/go/chat/maps/livelocation.go @@ -225,7 +225,7 @@ func (l *LiveLocationTracker) updateMapUnfurl(ctx context.Context, t *locationTr unfurlDoneCh := make(chan struct{}, 10) outboxID := storage.GetOutboxIDFromURL(body, t.convID, newMsg) listenerID := l.G().NotifyRouter.AddListener(newUnfurlNotifyListener(l.G(), outboxID, unfurlDoneCh)) - l.G().Unfurler.UnfurlAndSend(ctx, l.uid, t.convID, newMsg) + l.G().Unfurler.UnfurlAndSend(ctx, l.uid, t.convID, newMsg, nil) select { case <-unfurlDoneCh: case <-time.After(time.Minute): diff --git a/go/chat/sender.go b/go/chat/sender.go index 653934c17696..ad09cc28c5dd 100644 --- a/go/chat/sender.go +++ b/go/chat/sender.go @@ -1337,9 +1337,10 @@ func (s *BlockingSender) Send(ctx context.Context, convID chat1.ConversationID, chat1.ChatActivitySource_LOCAL) } if conv.GetTopicType() == chat1.TopicType_CHAT { - // Unfurl + // Unfurl. the suppressed urls travel with the message on its outbox record, so a + // send that waited offline still honours what the sender dismissed go s.G().Unfurler.UnfurlAndSend(globals.BackgroundChatCtx(ctx, s.G()), boxed.ClientHeader.Sender, - convID, unboxedMsg) + convID, unboxedMsg, sendOpts.GetUnfurlSuppress()) // Start tracking any live location sends if unboxedMsg.IsValid() && unboxedMsg.GetMessageType() == chat1.MessageType_TEXT && unboxedMsg.Valid().MessageBody.Text().LiveLocation != nil { diff --git a/go/chat/server.go b/go/chat/server.go index 24ba107ce759..40410126f538 100644 --- a/go/chat/server.go +++ b/go/chat/server.go @@ -1005,18 +1005,8 @@ func (h *Server) PostTextNonblock(ctx context.Context, arg chat1.PostTextNonbloc }) } - if len(arg.UnfurlSuppress) > 0 { - // suppression is keyed by outbox ID, and without one from the caller the deliverer - // generates its own, which we never see here. say so rather than dropping the - // caller's intent silently and unfurling a link they asked us not to. - if arg.OutboxID == nil { - h.Debug(ctx, "PostTextNonblock: ignoring unfurlSuppress, no outboxID supplied") - } else { - h.G().Unfurler.SetSuppressed(ctx, *arg.OutboxID, arg.UnfurlSuppress) - } - } - var parg chat1.PostLocalNonblockArg + parg.UnfurlSuppress = arg.UnfurlSuppress parg.SessionID = arg.SessionID parg.ClientPrev = arg.ClientPrev parg.ConversationID = arg.ConversationID @@ -1209,14 +1199,18 @@ func (h *Server) PostLocalNonblock(ctx context.Context, arg chat1.PostLocalNonbl // Create non block sender var prepareOpts chat1.SenderPrepareOptions + var sendOpts chat1.SenderSendOptions sender := NewBlockingSender(h.G(), h.boxer, h.remoteClient) nonblockSender := NewNonblockingSender(h.G(), sender) prepareOpts.ReplyTo = arg.ReplyTo + // rides the outbox record, so a message that waits offline still knows which urls the + // sender dismissed by the time it actually goes out + sendOpts.UnfurlSuppress = arg.UnfurlSuppress if arg.Msg.ClientHeader.Conv.TopicType == chat1.TopicType_NONE { arg.Msg.ClientHeader.Conv.TopicType = chat1.TopicType_CHAT } obid, _, err := nonblockSender.Send(ctx, arg.ConversationID, arg.Msg, arg.ClientPrev, arg.OutboxID, - nil, &prepareOpts) + &sendOpts, &prepareOpts) if err != nil { return res, fmt.Errorf("PostLocalNonblock: unable to send message: err: %s", err.Error()) } @@ -2687,7 +2681,10 @@ func (h *Server) ResolveUnfurlPrompt(ctx context.Context, arg chat1.ResolveUnfur if len(msgs) != 1 { return errors.New("message not found") } - h.G().Unfurler.UnfurlAndSend(ctx, uid, arg.ConvID, msgs[0]) + // no suppress list on this pass: the message is already sent, so its outbox record + // is gone. urls dismissed at send time were marked then, and UnfurlAndSend reads + // those markers, so a dismissal still holds here + h.G().Unfurler.UnfurlAndSend(ctx, uid, arg.ConvID, msgs[0], nil) return nil } atyp, err := arg.Result.ActionType() diff --git a/go/chat/types/interfaces.go b/go/chat/types/interfaces.go index 3482edf0318a..cff757760b2e 100644 --- a/go/chat/types/interfaces.go +++ b/go/chat/types/interfaces.go @@ -504,11 +504,10 @@ type WhitelistExemption interface { type Unfurler interface { UnfurlAndSend(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, - msg chat1.MessageUnboxed) + msg chat1.MessageUnboxed, suppress []string) Prefetch(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, msgText string) int PreviewURLs(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, text string) []chat1.UnfurlPreviewInfo - SetSuppressed(ctx context.Context, outboxID chat1.OutboxID, urls []string) Status(ctx context.Context, outboxID chat1.OutboxID) (UnfurlerTaskStatus, *chat1.UnfurlResult, error) Retry(ctx context.Context, outboxID chat1.OutboxID) Complete(ctx context.Context, outboxID chat1.OutboxID) diff --git a/go/chat/types/types.go b/go/chat/types/types.go index 65f027e7cd2b..ef7c7e36414e 100644 --- a/go/chat/types/types.go +++ b/go/chat/types/types.go @@ -581,7 +581,7 @@ type DummyUnfurler struct{} var _ Unfurler = (*DummyUnfurler)(nil) func (d DummyUnfurler) UnfurlAndSend(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, - msg chat1.MessageUnboxed) { + msg chat1.MessageUnboxed, suppress []string) { } func (d DummyUnfurler) Prefetch(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, msgText string) int { @@ -594,8 +594,6 @@ func (d DummyUnfurler) PreviewURLs(ctx context.Context, uid gregor1.UID, convID return nil } -func (d DummyUnfurler) SetSuppressed(ctx context.Context, outboxID chat1.OutboxID, urls []string) {} - func (d DummyUnfurler) Status(ctx context.Context, outboxID chat1.OutboxID) (UnfurlerTaskStatus, *chat1.UnfurlResult, error) { return UnfurlerTaskStatusFailed, nil, nil } diff --git a/go/chat/unfurl/cache.go b/go/chat/unfurl/cache.go index bc258b625d60..e45cb8d990fc 100644 --- a/go/chat/unfurl/cache.go +++ b/go/chat/unfurl/cache.go @@ -12,15 +12,6 @@ import ( const ( defaultCacheLifetime = 10 * time.Minute defaultCacheSize = 1000 - - // suppressedCacheLifetime/Size bound the store of per-send unfurl - // suppressions: a message about to send holds at most a handful of - // dismissed URLs, and the entry is only relevant for as long as the - // send is in flight, so a small cap and a short TTL are enough to - // prevent an unconsumed entry (failed/aborted send) from leaking for - // the life of the process. - suppressedCacheLifetime = 5 * time.Minute - suppressedCacheSize = 200 ) type cacheItem struct { @@ -30,24 +21,18 @@ type cacheItem struct { type unfurlCache struct { sync.Mutex - cache *lru.Cache - clock clockwork.Clock - lifetime time.Duration + cache *lru.Cache + clock clockwork.Clock } func newUnfurlCache() *unfurlCache { - return newUnfurlCacheWithLimits(defaultCacheSize, defaultCacheLifetime) -} - -func newUnfurlCacheWithLimits(size int, lifetime time.Duration) *unfurlCache { - cache, err := lru.New(size) + cache, err := lru.New(defaultCacheSize) if err != nil { panic(err) } return &unfurlCache{ - cache: cache, - clock: clockwork.NewRealClock(), - lifetime: lifetime, + cache: cache, + clock: clockwork.NewRealClock(), } } @@ -55,8 +40,8 @@ func (c *unfurlCache) setClock(clock clockwork.Clock) { c.clock = clock } -// get determines if the item is in the cache and newer than the cache's -// lifetime. We don't want to cache this value indefinitely in case the page +// get determines if the item is in the cache and newer than 10 +// minutes. We don't want to cache this value indefinitely in case the page // content changes. func (c *unfurlCache) get(key string) (res cacheItem, ok bool) { c.Lock() @@ -70,7 +55,7 @@ func (c *unfurlCache) get(key string) (res cacheItem, ok bool) { if !ok { return res, false } - valid := c.clock.Now().Sub(cacheItem.ctime.Time()) <= c.lifetime + valid := c.clock.Now().Sub(cacheItem.ctime.Time()) <= defaultCacheLifetime if !valid { c.cache.Remove(key) } diff --git a/go/chat/unfurl/unfurler.go b/go/chat/unfurl/unfurler.go index 362ab17455ed..b7343f1d1b60 100644 --- a/go/chat/unfurl/unfurler.go +++ b/go/chat/unfurl/unfurler.go @@ -53,13 +53,12 @@ type Unfurler struct { globals.Contextified utils.DebugLabeler - unfurlMap map[string]bool - suppressed *unfurlCache - extractor *Extractor - scraper *Scraper - packager *Packager - settings *Settings - sender UnfurlMessageSender + unfurlMap map[string]bool + extractor *Extractor + scraper *Scraper + packager *Packager + settings *Settings + sender UnfurlMessageSender // testing unfurlCh chan *chat1.Unfurl @@ -79,7 +78,6 @@ func NewUnfurler(g *globals.Context, store attachments.Store, s3signer s3.Signer Contextified: globals.NewContextified(g), DebugLabeler: utils.NewDebugLabeler(g.ExternalG(), "Unfurler", false), unfurlMap: make(map[string]bool), - suppressed: newUnfurlCacheWithLimits(suppressedCacheSize, suppressedCacheLifetime), extractor: extractor, scraper: scraper, packager: packager, @@ -91,7 +89,6 @@ func NewUnfurler(g *globals.Context, store attachments.Store, s3signer s3.Signer func (u *Unfurler) SetClock(clock clockwork.Clock) { u.scraper.cache.setClock(clock) u.packager.cache.setClock(clock) - u.suppressed.setClock(clock) } func (u *Unfurler) SetTestingRetryCh(ch chan struct{}) { @@ -119,6 +116,13 @@ func (u *Unfurler) statusKey(outboxID chat1.OutboxID) libkb.DbKey { } } +func (u *Unfurler) suppressedKey(outboxID chat1.OutboxID) libkb.DbKey { + return libkb.DbKey{ + Typ: libkb.DBUnfurler, + Key: fmt.Sprintf("s|%s", outboxID), + } +} + func (u *Unfurler) taskKey(outboxID chat1.OutboxID) libkb.DbKey { return libkb.DbKey{ Typ: libkb.DBUnfurler, @@ -238,42 +242,32 @@ func (u *Unfurler) makeBaseUnfurlMessage(ctx context.Context, fromMsg chat1.Mess return msg, nil } -// SetSuppressed records URLs the sender chose not to unfurl for the message -// with this outbox ID. The entry is not consumed by UnfurlAndSend: a message -// can be unfurled more than once (accepting an unfurl prompt re-runs -// UnfurlAndSend on the same message), and a consumed entry would let a -// dismissed URL unfurl on that second pass. It expires after -// suppressedCacheLifetime instead, which also keeps an entry left behind by -// a send that never reaches UnfurlAndSend from lingering for the life of the -// process. -func (u *Unfurler) SetSuppressed(ctx context.Context, outboxID chat1.OutboxID, urls []string) { - if len(urls) == 0 { - return - } - m := make(map[string]bool, len(urls)) - for _, url := range urls { - m[url] = true - } - u.suppressed.put(outboxID.String(), m) +// markSuppressed records that this URL was dismissed for this message, at the same +// deterministic key the unfurl task would use. UnfurlAndSend runs again whenever the user +// resolves an unfurl prompt for another URL in the same message, and that pass carries no +// suppress list, so the marker is what keeps the dismissal honoured. It lives with the +// message's other unfurl bookkeeping rather than on a clock, so a send that waited offline +// is treated the same as one that went out immediately. +func (u *Unfurler) markSuppressed(ctx context.Context, outboxID chat1.OutboxID) error { + return u.G().GetKVStore().PutObj(u.suppressedKey(outboxID), nil, true) } -func (u *Unfurler) getSuppressed(outboxID *chat1.OutboxID) map[string]bool { - if outboxID == nil { - return nil - } - item, valid := u.suppressed.get(outboxID.String()) - if !valid { - return nil - } - m, ok := item.data.(map[string]bool) - if !ok { - return nil +func (u *Unfurler) isSuppressed(ctx context.Context, outboxID chat1.OutboxID) bool { + var found bool + ok, err := u.G().GetKVStore().GetInto(&found, u.suppressedKey(outboxID)) + if err != nil { + u.Debug(ctx, "isSuppressed: failed to read: %s", err) + return false } - return m + return ok && found } +// UnfurlAndSend unfurls the URLs in msg. suppress carries the URLs the sender chose not to +// unfurl; it arrives from the message's outbox record on the send that posts the message, +// and is empty on later passes over the same message (resolving an unfurl prompt calls this +// again), which read the marker left behind by the first pass instead. func (u *Unfurler) UnfurlAndSend(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, - msg chat1.MessageUnboxed, + msg chat1.MessageUnboxed, suppress []string, ) { defer u.Trace(ctx, nil, "UnfurlAndSend")() // early out for errors @@ -286,7 +280,10 @@ func (u *Unfurler) UnfurlAndSend(ctx context.Context, uid gregor1.UID, convID ch if len(hits) == 0 { return } - suppressed := u.getSuppressed(msg.Valid().ClientHeader.OutboxID) + suppressed := make(map[string]bool, len(suppress)) + for _, url := range suppress { + suppressed[url] = true + } // get a map for all the URLs we have already unfurled prevUnfurled := make(map[string]bool) for _, u := range msg.Valid().Unfurls { @@ -294,10 +291,6 @@ func (u *Unfurler) UnfurlAndSend(ctx context.Context, uid gregor1.UID, convID ch } // for each hit, either prompt the user for action, or generate a new message for _, hit := range hits { - if suppressed[hit.URL] { - u.Debug(ctx, "UnfurlAndSend: skipping suppressed URL") - continue - } if prevUnfurled[hit.URL] { u.Debug(ctx, "UnfurlAndSend: skipping prev unfurled") continue @@ -313,6 +306,15 @@ func (u *Unfurler) UnfurlAndSend(ctx context.Context, uid gregor1.UID, convID ch u.G().ActivityNotifier.PromptUnfurl(ctx, uid, convID, msg.GetMessageID(), domain) case ExtractorHitUnfurl: outboxID := storage.GetOutboxIDFromURL(hit.URL, convID, msg) + if suppressed[hit.URL] || u.isSuppressed(ctx, outboxID) { + // remember it: this runs again when the user resolves a prompt for another + // URL in the same message, and that pass has no suppress list of its own + u.Debug(ctx, "UnfurlAndSend: skipping suppressed URL: outboxID: %s", outboxID) + if err := u.markSuppressed(ctx, outboxID); err != nil { + u.Debug(ctx, "UnfurlAndSend: failed to mark suppressed: %s", err) + } + continue + } if _, err := u.getTask(ctx, outboxID); err == nil { u.Debug(ctx, "UnfurlAndSend: skipping URL hit, task exists: outboxID: %s", outboxID) continue diff --git a/go/chat/unfurl/unfurler_test.go b/go/chat/unfurl/unfurler_test.go index 9d8aa4bc9172..768936ad68e7 100644 --- a/go/chat/unfurl/unfurler_test.go +++ b/go/chat/unfurl/unfurler_test.go @@ -15,7 +15,6 @@ import ( "github.com/keybase/client/go/libkb" "github.com/keybase/client/go/protocol/chat1" "github.com/keybase/client/go/protocol/gregor1" - "github.com/keybase/clockwork" "github.com/stretchr/testify/require" ) @@ -111,7 +110,7 @@ func TestUnfurler(t *testing.T) { numPrefetched := unfurler.Prefetch(context.TODO(), uid, convID, msgBody) require.Equal(t, 0, numPrefetched) - unfurler.UnfurlAndSend(context.TODO(), uid, convID, fromMsg) + unfurler.UnfurlAndSend(context.TODO(), uid, convID, fromMsg, nil) select { case <-sender.ch: require.Fail(t, "no send here") @@ -130,7 +129,7 @@ func TestUnfurler(t *testing.T) { require.Equal(t, 1, numPrefetched) for range 5 { - unfurler.UnfurlAndSend(context.TODO(), uid, convID, fromMsg) + unfurler.UnfurlAndSend(context.TODO(), uid, convID, fromMsg, nil) } var outboxID chat1.OutboxID select { @@ -248,8 +247,6 @@ func TestUnfurlerSuppress(t *testing.T) { ri := func() chat1.RemoteInterface { return paramsRemote{} } memStorage := newMemConversationBackedStorage() unfurler := NewUnfurler(g, store, s3signer, memStorage, sender, ri) - suppressedClock := clockwork.NewFakeClock() - unfurler.suppressed.setClock(suppressedClock) uid := gregor1.UID([]byte{0, 1}) convID := chat1.ConversationID([]byte{0, 1, 2}) @@ -264,31 +261,35 @@ func TestUnfurlerSuppress(t *testing.T) { require.NoError(t, err) msg := makeTextMsgWithOutboxID("check out this link! "+url, outboxID) - unfurler.SetSuppressed(context.TODO(), outboxID, []string{url}) - unfurler.UnfurlAndSend(context.TODO(), uid, convID, msg) + unfurler.UnfurlAndSend(context.TODO(), uid, convID, msg, []string{url}) select { case <-sender.ch: require.Fail(t, "should not have sent a suppressed unfurl") case <-time.After(2 * time.Second): } - // suppression is not consumed: accepting an unfurl prompt re-runs - // UnfurlAndSend on the same message, and the dismissed URL must stay - // suppressed on that second pass - unfurler.UnfurlAndSend(context.TODO(), uid, convID, msg) + // resolving an unfurl prompt for another URL re-runs UnfurlAndSend on the same message + // with no suppress list of its own. the marker left by the first pass is what keeps the + // dismissal honoured, and it is not on a clock: however long the message waited to + // send, this pass must still skip the URL + unfurler.UnfurlAndSend(context.TODO(), uid, convID, msg, nil) select { case <-sender.ch: require.Fail(t, "should not have sent a suppressed unfurl on the second pass") case <-time.After(2 * time.Second): } - // the entry expires rather than being consumed - suppressedClock.Advance(suppressedCacheLifetime + time.Minute) - unfurler.UnfurlAndSend(context.TODO(), uid, convID, msg) + // an unsuppressed URL in the same conversation still unfurls, so the checks above are + // not passing because the pipeline is dead + otherURL := fmt.Sprintf("http://%s/?name=%s", addr, "nytimes0.html") + otherOutboxID, err := storage.NewOutboxID() + require.NoError(t, err) + otherMsg := makeTextMsgWithOutboxID("and this one "+otherURL, otherOutboxID) + unfurler.UnfurlAndSend(context.TODO(), uid, convID, otherMsg, nil) select { case <-sender.ch: case <-time.After(20 * time.Second): - require.Fail(t, "no unfurl message sent") + require.Fail(t, "no unfurl message sent for the unsuppressed URL") } } diff --git a/go/protocol/chat1/extras.go b/go/protocol/chat1/extras.go index df39d55e7d2b..f4277c96ac20 100644 --- a/go/protocol/chat1/extras.go +++ b/go/protocol/chat1/extras.go @@ -3092,6 +3092,13 @@ func (o *SenderSendOptions) GetJoinMentionsAs() *ConversationMemberStatus { return o.JoinMentionsAs } +func (o *SenderSendOptions) GetUnfurlSuppress() []string { + if o == nil { + return nil + } + return o.UnfurlSuppress +} + func (c Coordinate) IsZero() bool { return c.Lat == 0 && c.Lon == 0 } diff --git a/go/protocol/chat1/local.go b/go/protocol/chat1/local.go index 6fccec09f1df..b46e2b575249 100644 --- a/go/protocol/chat1/local.go +++ b/go/protocol/chat1/local.go @@ -1757,6 +1757,7 @@ func (o SenderPrepareOptions) DeepCopy() SenderPrepareOptions { type SenderSendOptions struct { JoinMentionsAs *ConversationMemberStatus `codec:"joinMentionsAs,omitempty" json:"joinMentionsAs,omitempty"` + UnfurlSuppress []string `codec:"unfurlSuppress" json:"unfurlSuppress"` } func (o SenderSendOptions) DeepCopy() SenderSendOptions { @@ -1768,6 +1769,17 @@ func (o SenderSendOptions) DeepCopy() SenderSendOptions { tmp := x.DeepCopy() return &tmp })(o.JoinMentionsAs), + UnfurlSuppress: (func(x []string) []string { + if x == nil { + return nil + } + ret := make([]string, len(x)) + for i, v := range x { + vCopy := v + ret[i] = vCopy + } + return ret + })(o.UnfurlSuppress), } } @@ -6828,6 +6840,7 @@ type PostLocalNonblockArg struct { ReplyTo *MessageID `codec:"replyTo,omitempty" json:"replyTo,omitempty"` IdentifyBehavior keybase1.TLFIdentifyBehavior `codec:"identifyBehavior" json:"identifyBehavior"` SkipInChatPayments bool `codec:"skipInChatPayments" json:"skipInChatPayments"` + UnfurlSuppress []string `codec:"unfurlSuppress" json:"unfurlSuppress"` } type ForwardMessageArg struct { diff --git a/protocol/avdl/chat1/local.avdl b/protocol/avdl/chat1/local.avdl index 24872ca01416..52cbfb2cc52a 100644 --- a/protocol/avdl/chat1/local.avdl +++ b/protocol/avdl/chat1/local.avdl @@ -272,6 +272,9 @@ protocol local { record SenderSendOptions { union { null, ConversationMemberStatus } joinMentionsAs; + // urls the sender chose not to unfurl; rides the outbox record so a message that + // waits offline still carries them when it finally sends + array unfurlSuppress; } enum OutboxStateType { @@ -829,7 +832,7 @@ protocol local { OutboxID generateOutboxID(); @timeout_msec(30000) // 30 seconds - PostLocalNonblockRes postLocalNonblock(int sessionID, ConversationID conversationID, MessagePlaintext msg, MessageID clientPrev, union { null, OutboxID } outboxID, union { null, MessageID } replyTo, keybase1.TLFIdentifyBehavior identifyBehavior, boolean skipInChatPayments); + PostLocalNonblockRes postLocalNonblock(int sessionID, ConversationID conversationID, MessagePlaintext msg, MessageID clientPrev, union { null, OutboxID } outboxID, union { null, MessageID } replyTo, keybase1.TLFIdentifyBehavior identifyBehavior, boolean skipInChatPayments, array unfurlSuppress); record PostLocalNonblockRes { array rateLimits; OutboxID outboxID; diff --git a/protocol/json/chat1/local.json b/protocol/json/chat1/local.json index 1d995048f883..d0c4b0673b2d 100644 --- a/protocol/json/chat1/local.json +++ b/protocol/json/chat1/local.json @@ -996,6 +996,13 @@ "ConversationMemberStatus" ], "name": "joinMentionsAs" + }, + { + "type": { + "type": "array", + "items": "string" + }, + "name": "unfurlSuppress" } ] }, @@ -4656,6 +4663,13 @@ { "name": "skipInChatPayments", "type": "boolean" + }, + { + "name": "unfurlSuppress", + "type": { + "type": "array", + "items": "string" + } } ], "response": "PostLocalNonblockRes", diff --git a/shared/chat/conversation/input-area/input-state.test.tsx b/shared/chat/conversation/input-area/input-state.test.tsx index 59d42a478f8b..7ecc294f1154 100644 --- a/shared/chat/conversation/input-area/input-state.test.tsx +++ b/shared/chat/conversation/input-area/input-state.test.tsx @@ -434,7 +434,6 @@ test('sendComposerText sends dismissed unfurl urls as unfurlSuppress', async () await flushPromises() expect(getLastPost()?.params.unfurlSuppress).toEqual(['http://a.com']) - expect(getLastPost()?.params.outboxID).toBeTruthy() expect(getSuppressedURLs(convID)).toEqual([]) }) diff --git a/shared/chat/conversation/send-actions.tsx b/shared/chat/conversation/send-actions.tsx index 4047a93c853d..ef2592e2166f 100644 --- a/shared/chat/conversation/send-actions.tsx +++ b/shared/chat/conversation/send-actions.tsx @@ -56,7 +56,7 @@ const sendTextMessageStoreless = (p: SendTextParams) => { clientPrev: p.clientPrev, conversationID: T.Chat.keyToConversationID(p.conversationIDKey), identifyBehavior: T.RPCGen.TLFIdentifyBehavior.chatGui, - outboxID: Common.generateOutboxID(), + outboxID: undefined, replyTo: p.replyTo, tlfName: p.tlfName, tlfPublic: false, diff --git a/shared/constants/rpc/rpc-chat-gen.tsx b/shared/constants/rpc/rpc-chat-gen.tsx index f29c77a4b23a..7e73f07ab61c 100644 --- a/shared/constants/rpc/rpc-chat-gen.tsx +++ b/shared/constants/rpc/rpc-chat-gen.tsx @@ -1476,7 +1476,7 @@ export type SearchOpts = {readonly isRegex: boolean,readonly sentBy: string,read export type SearchRegexpRes = {readonly offline: boolean,readonly hits?: ReadonlyArray | null,readonly rateLimits?: ReadonlyArray | null,readonly identifyFailures?: ReadonlyArray | null,} export type SendRes = {readonly message: string,readonly messageID?: MessageID | null,readonly outboxID?: OutboxID | null,readonly identifyFailures?: ReadonlyArray | null,readonly rateLimits?: ReadonlyArray | null,} export type SenderPrepareOptions = {readonly skipTopicNameState: boolean,readonly replyTo?: MessageID | null,} -export type SenderSendOptions = {readonly joinMentionsAs?: ConversationMemberStatus | null,} +export type SenderSendOptions = {readonly joinMentionsAs?: ConversationMemberStatus | null,readonly unfurlSuppress?: ReadonlyArray | null,} export type ServerCacheVers = {readonly inboxVers: number,readonly bodiesVers: number,} export type ServerNowRes = {readonly rateLimit?: RateLimit | null,readonly now: Gregor1.Time,} export type SetAppNotificationSettingsInfo = {readonly convID: ConversationID,readonly settings: ConversationNotificationInfo,} From f3b711cb969c26ef43d63b2460ed4902f1d88028 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 24 Aug 2026 17:37:22 -0400 Subject: [PATCH 07/17] fix(chat): stop the suppression marker colliding with the task status key suppressedKey was copied from statusKey and the prefix never changed, so both produced "s|" in the DBUnfurler namespace. markSuppressed writes a bool there and setStatus writes a task status enum, so whichever ran last for an outbox ID clobbered the other, and reading back the wrong type just fails - isSuppressed swallows that and reports not suppressed, un-dismissing a link. Nothing hits it today only because a suppressed URL never gets a task and so never gets a status, which is an invariant nothing states or enforces. The marker gets its own prefix. Suppression is also checked for every hit again, not only for the unfurl case. A queued message is classified against the whitelist at send time, so a URL dismissed while its domain was whitelisted can arrive as a prompt hit instead, and prompting for a link the sender declined breaks the same promise as unfurling it. Both paths now have a test, and each carries a positive control so a passing run cannot mean the pipeline is simply dead. The comment claiming the marker needs no cleanup was wrong: it is keyed per URL per message, nothing deletes it, and Complete only clears state for URLs that actually unfurled. Says so now, with what it costs. Also corrects the previewText comment, which described a ref it has nothing to do with. --- go/chat/unfurl/unfurler.go | 48 +++++++++++------ go/chat/unfurl/unfurler_test.go | 52 +++++++++++++++++++ .../conversation/input-area/normal/index.tsx | 6 +-- 3 files changed, 86 insertions(+), 20 deletions(-) diff --git a/go/chat/unfurl/unfurler.go b/go/chat/unfurl/unfurler.go index b7343f1d1b60..7f9f13bd5f0b 100644 --- a/go/chat/unfurl/unfurler.go +++ b/go/chat/unfurl/unfurler.go @@ -119,7 +119,8 @@ func (u *Unfurler) statusKey(outboxID chat1.OutboxID) libkb.DbKey { func (u *Unfurler) suppressedKey(outboxID chat1.OutboxID) libkb.DbKey { return libkb.DbKey{ Typ: libkb.DBUnfurler, - Key: fmt.Sprintf("s|%s", outboxID), + // not "s|", which is statusKey + Key: fmt.Sprintf("sup|%s", outboxID), } } @@ -242,12 +243,17 @@ func (u *Unfurler) makeBaseUnfurlMessage(ctx context.Context, fromMsg chat1.Mess return msg, nil } -// markSuppressed records that this URL was dismissed for this message, at the same -// deterministic key the unfurl task would use. UnfurlAndSend runs again whenever the user -// resolves an unfurl prompt for another URL in the same message, and that pass carries no -// suppress list, so the marker is what keeps the dismissal honoured. It lives with the -// message's other unfurl bookkeeping rather than on a clock, so a send that waited offline -// is treated the same as one that went out immediately. +// markSuppressed records that this URL was dismissed for this message, keyed by the same +// deterministic per-URL outbox ID the unfurl task would use. UnfurlAndSend runs again +// whenever the user resolves an unfurl prompt for another URL in the same message, and that +// pass carries no suppress list, so the marker is what keeps the dismissal honoured, on a +// clock or not. +// +// Nothing deletes these. Complete() clears the task and status keys for a URL that actually +// unfurled, but a suppressed URL never gets a task, so it has no such moment: the marker +// would have to be cleaned when the message itself goes away, which the unfurler is not +// told about. Each is one bool per dismissed URL per message, so the footprint is small and +// grows only with links the user explicitly declined. func (u *Unfurler) markSuppressed(ctx context.Context, outboxID chat1.OutboxID) error { return u.G().GetKVStore().PutObj(u.suppressedKey(outboxID), nil, true) } @@ -296,6 +302,23 @@ func (u *Unfurler) UnfurlAndSend(ctx context.Context, uid gregor1.UID, convID ch continue } prevUnfurled[hit.URL] = true // only one action per unique URL + // checked for every hit type, not just the unfurl one: a queued message can sit for + // a long time, and the whitelist it is classified against is read at send time, so a + // url the sender dismissed may have become a prompt hit in the meantime. prompting + // for a link they declined is the same broken promise as unfurling it + urlOutboxID := storage.GetOutboxIDFromURL(hit.URL, convID, msg) + marked := u.isSuppressed(ctx, urlOutboxID) + if suppressed[hit.URL] || marked { + u.Debug(ctx, "UnfurlAndSend: skipping suppressed URL: outboxID: %s", urlOutboxID) + if !marked { + // remember it: this runs again when the user resolves a prompt for another + // url in the same message, and that pass has no suppress list of its own + if err := u.markSuppressed(ctx, urlOutboxID); err != nil { + u.Debug(ctx, "UnfurlAndSend: failed to mark suppressed: %s", err) + } + } + continue + } switch hit.Typ { case ExtractorHitPrompt: domain, err := GetDomain(hit.URL) @@ -305,16 +328,7 @@ func (u *Unfurler) UnfurlAndSend(ctx context.Context, uid gregor1.UID, convID ch } u.G().ActivityNotifier.PromptUnfurl(ctx, uid, convID, msg.GetMessageID(), domain) case ExtractorHitUnfurl: - outboxID := storage.GetOutboxIDFromURL(hit.URL, convID, msg) - if suppressed[hit.URL] || u.isSuppressed(ctx, outboxID) { - // remember it: this runs again when the user resolves a prompt for another - // URL in the same message, and that pass has no suppress list of its own - u.Debug(ctx, "UnfurlAndSend: skipping suppressed URL: outboxID: %s", outboxID) - if err := u.markSuppressed(ctx, outboxID); err != nil { - u.Debug(ctx, "UnfurlAndSend: failed to mark suppressed: %s", err) - } - continue - } + outboxID := urlOutboxID if _, err := u.getTask(ctx, outboxID); err == nil { u.Debug(ctx, "UnfurlAndSend: skipping URL hit, task exists: outboxID: %s", outboxID) continue diff --git a/go/chat/unfurl/unfurler_test.go b/go/chat/unfurl/unfurler_test.go index 768936ad68e7..3e12e46441c2 100644 --- a/go/chat/unfurl/unfurler_test.go +++ b/go/chat/unfurl/unfurler_test.go @@ -308,3 +308,55 @@ func TestPreviewable(t *testing.T) { require.False(t, previewable(chat1.NewUnfurlWithYoutube(chat1.UnfurlYoutube{}))) require.False(t, previewable(chat1.NewUnfurlWithGiphy(chat1.UnfurlGiphy{}))) } + +func TestUnfurlerSuppressPrompt(t *testing.T) { + tc := externalstest.SetupTest(t, "unfurler", 0) + defer tc.Cleanup() + g := globals.NewContext(tc.G, &globals.ChatContext{}) + + store := attachments.NewStoreTesting(g, nil) + s3signer := &ptsigner{} + notifier := makeDummyActivityNotifier() + g.ActivityNotifier = notifier + g.MessageDeliverer = dummyDeliverer{} + sender := makeDummySender() + ri := func() chat1.RemoteInterface { return paramsRemote{} } + memStorage := newMemConversationBackedStorage() + unfurler := NewUnfurler(g, store, s3signer, memStorage, sender, ri) + + uid := gregor1.UID([]byte{0, 1}) + convID := chat1.ConversationID([]byte{0, 1, 2}) + srv := createTestCaseHTTPSrv(t) + addr := srv.Start() + defer srv.Stop() + + // no WhitelistAdd here, so this url classifies as a prompt hit rather than an unfurl + // one. a queued message is classified at send time, so a url the sender dismissed while + // the domain was whitelisted can arrive here as a prompt: prompting for a link they + // declined breaks the same promise as unfurling it + url := fmt.Sprintf("http://%s/?name=%s", addr, "wsj0.html") + outboxID, err := storage.NewOutboxID() + require.NoError(t, err) + msg := makeTextMsgWithOutboxID("check out this link! "+url, outboxID) + + unfurler.UnfurlAndSend(context.TODO(), uid, convID, msg, []string{url}) + select { + case n := <-notifier.ch: + require.Failf(t, "prompted for a suppressed URL", "domain: %s", n.domain) + case <-time.After(2 * time.Second): + } + + // and an unsuppressed one still prompts, so the check above is not passing because + // prompting is broken. it has to be a different url: the marker is keyed by url, so + // re-sending the same one would be skipped by the marker the first pass just wrote + otherURL := fmt.Sprintf("http://%s/?name=%s", addr, "nytimes0.html") + otherOutboxID, err := storage.NewOutboxID() + require.NoError(t, err) + unfurler.UnfurlAndSend(context.TODO(), uid, convID, + makeTextMsgWithOutboxID("and this one "+otherURL, otherOutboxID), nil) + select { + case <-notifier.ch: + case <-time.After(20 * time.Second): + require.Fail(t, "no prompt for the unsuppressed URL") + } +} diff --git a/shared/chat/conversation/input-area/normal/index.tsx b/shared/chat/conversation/input-area/normal/index.tsx index ac55cadb5620..0650ca0be9f4 100644 --- a/shared/chat/conversation/input-area/normal/index.tsx +++ b/shared/chat/conversation/input-area/normal/index.tsx @@ -234,9 +234,9 @@ const ConnectedPlatformInput = function ConnectedPlatformInput() { } const sendTyping = C.useThrottledCallback(sendTypingRaw, 1000) - // Low-frequency (throttled) copy of the composer text for the unfurl preview, which - // already debounces 500ms downstream. textValueRef is a ref (no re-render), so previews - // ride along on the existing throttled draft-save path instead of a per-keystroke state. + // Low-frequency copy of the composer text for the unfurl preview, set from the already + // throttled draft-save path rather than from onChangeText, so the composer does not + // re-render on every keystroke. The preview debounces another 500ms downstream anyway. const [previewText, setPreviewText] = React.useState('') const updateDraftRaw = (text: string) => { // Immediately update local meta.draft so switching back to this thread From bf100321f9f2d7683104d305f77e11b6a5b8e135 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 24 Aug 2026 17:41:36 -0400 Subject: [PATCH 08/17] test(chat): let the unfurl tests express distinct messages The message helper hardcoded a message id, so the outbox id a caller passed had no effect on the suppression key, which derives from the url, the conversation and the message id. Both suppression tests were leaning on a different url to separate their cases while reading as though the outbox id did it, and the scenario that matters most could not be written at all: dismissing a link in one message must not suppress the same link in a later one. The helper takes a message id now, both tests use it for their positive control, and the message-scoping case is covered. Verified it fails when the key stops depending on the message. --- go/chat/unfurl/unfurler_test.go | 34 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/go/chat/unfurl/unfurler_test.go b/go/chat/unfurl/unfurler_test.go index 3e12e46441c2..a30986362c42 100644 --- a/go/chat/unfurl/unfurler_test.go +++ b/go/chat/unfurl/unfurler_test.go @@ -218,7 +218,7 @@ func TestUnfurlerPreviewURLs(t *testing.T) { require.Empty(t, unfurler.PreviewURLs(context.TODO(), uid, convID, "no links here")) } -func makeTextMsgWithOutboxID(msgBody string, outboxID chat1.OutboxID) chat1.MessageUnboxed { +func makeTextMsgWithMsgID(msgBody string, outboxID chat1.OutboxID, msgID chat1.MessageID) chat1.MessageUnboxed { return chat1.NewMessageUnboxedWithValid(chat1.MessageUnboxedValid{ ClientHeader: chat1.MessageClientHeaderVerified{ TlfName: "mike", @@ -226,7 +226,9 @@ func makeTextMsgWithOutboxID(msgBody string, outboxID chat1.OutboxID) chat1.Mess OutboxID: &outboxID, }, ServerHeader: chat1.MessageServerHeader{ - MessageID: 4, + // the suppression key derives from this, not from the outbox id, so tests that + // mean "a different message" have to vary it + MessageID: msgID, }, MessageBody: chat1.NewMessageBodyWithText(chat1.MessageText{ Body: msgBody, @@ -259,7 +261,7 @@ func TestUnfurlerSuppress(t *testing.T) { outboxID, err := storage.NewOutboxID() require.NoError(t, err) - msg := makeTextMsgWithOutboxID("check out this link! "+url, outboxID) + msg := makeTextMsgWithMsgID("check out this link! "+url, outboxID, 4) unfurler.UnfurlAndSend(context.TODO(), uid, convID, msg, []string{url}) select { @@ -279,17 +281,15 @@ func TestUnfurlerSuppress(t *testing.T) { case <-time.After(2 * time.Second): } - // an unsuppressed URL in the same conversation still unfurls, so the checks above are - // not passing because the pipeline is dead - otherURL := fmt.Sprintf("http://%s/?name=%s", addr, "nytimes0.html") - otherOutboxID, err := storage.NewOutboxID() - require.NoError(t, err) - otherMsg := makeTextMsgWithOutboxID("and this one "+otherURL, otherOutboxID) - unfurler.UnfurlAndSend(context.TODO(), uid, convID, otherMsg, nil) + // the dismissal is scoped to that message: the same url sent again, undismissed, must + // still unfurl. the suppression key derives from the message id, so this needs a + // genuinely different message rather than just a different outbox id + laterMsg := makeTextMsgWithMsgID("sending it again "+url, outboxID, 5) + unfurler.UnfurlAndSend(context.TODO(), uid, convID, laterMsg, nil) select { case <-sender.ch: case <-time.After(20 * time.Second): - require.Fail(t, "no unfurl message sent for the unsuppressed URL") + require.Fail(t, "no unfurl message sent for the same url in a later message") } } @@ -337,7 +337,7 @@ func TestUnfurlerSuppressPrompt(t *testing.T) { url := fmt.Sprintf("http://%s/?name=%s", addr, "wsj0.html") outboxID, err := storage.NewOutboxID() require.NoError(t, err) - msg := makeTextMsgWithOutboxID("check out this link! "+url, outboxID) + msg := makeTextMsgWithMsgID("check out this link! "+url, outboxID, 4) unfurler.UnfurlAndSend(context.TODO(), uid, convID, msg, []string{url}) select { @@ -346,14 +346,10 @@ func TestUnfurlerSuppressPrompt(t *testing.T) { case <-time.After(2 * time.Second): } - // and an unsuppressed one still prompts, so the check above is not passing because - // prompting is broken. it has to be a different url: the marker is keyed by url, so - // re-sending the same one would be skipped by the marker the first pass just wrote - otherURL := fmt.Sprintf("http://%s/?name=%s", addr, "nytimes0.html") - otherOutboxID, err := storage.NewOutboxID() - require.NoError(t, err) + // the same url in a later message still prompts, so the check above is not passing + // because prompting is broken unfurler.UnfurlAndSend(context.TODO(), uid, convID, - makeTextMsgWithOutboxID("and this one "+otherURL, otherOutboxID), nil) + makeTextMsgWithMsgID("sending it again "+url, outboxID, 5), nil) select { case <-notifier.ch: case <-time.After(20 * time.Second): From 6b1ff14116934765fb1f131555d04ce757cfa79a Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 24 Aug 2026 17:52:22 -0400 Subject: [PATCH 09/17] fix(chat): do not offer a dismiss the edit path cannot honour Editing a message shows the preview card with a working looking X. The edit rpc carries no unfurlSuppress, so clicking it hid the card and changed nothing: the link still unfurled on save. A control that removes itself and silently does nothing is worse than no control, and worse than the missing card a failed scrape already produces, because it actively promises something. The card still renders while editing, since it is useful to see what will unfurl. It just has no X there. Closing the gap properly needs unfurlSuppress on postEditNonblock; this only stops lying about it in the meantime. --- .../conversation/input-area/normal/index.tsx | 6 ++++- .../input-area/unfurl-preview.test.tsx | 27 +++++++++++++------ .../input-area/unfurl-preview.tsx | 7 +++-- 3 files changed, 29 insertions(+), 11 deletions(-) diff --git a/shared/chat/conversation/input-area/normal/index.tsx b/shared/chat/conversation/input-area/normal/index.tsx index 0650ca0be9f4..cda7d6a8c568 100644 --- a/shared/chat/conversation/input-area/normal/index.tsx +++ b/shared/chat/conversation/input-area/normal/index.tsx @@ -338,7 +338,11 @@ const ConnectedPlatformInput = function ConnectedPlatformInput() { /> ) - const preview = + // no dismiss while editing: postEditNonblock carries no unfurlSuppress, so the X would + // hide the card and change nothing about what the edit posts + const preview = ( + + ) if (isMobile) { // in flow above the composer; it rides KeyboardStickyView with the input diff --git a/shared/chat/conversation/input-area/unfurl-preview.test.tsx b/shared/chat/conversation/input-area/unfurl-preview.test.tsx index f96b5bd85f17..bacd0a473d41 100644 --- a/shared/chat/conversation/input-area/unfurl-preview.test.tsx +++ b/shared/chat/conversation/input-area/unfurl-preview.test.tsx @@ -46,32 +46,32 @@ describe('UnfurlPreview', () => { it('renders nothing when every preview is non-generic', () => { mockPreviews = [nonGenericInfo] - const {container} = render() + const {container} = render() expect(container.firstChild).toBeNull() }) it('renders a card for a generic preview', () => { mockPreviews = [genericInfo] - const {container} = render() + const {container} = render() expect(container.firstChild).not.toBeNull() }) it('renders nothing for a map unfurl', () => { mockPreviews = [mapInfo] - const {container} = render() + const {container} = render() expect(container.firstChild).toBeNull() }) it('shows no pager for a single preview', () => { mockPreviews = [genericInfo] - const {queryByText} = render() + const {queryByText} = render() expect(queryByText('1/1')).toBeNull() }) it('pages between previews and disables the arrows at each end', () => { mockPreviews = [genericInfo, genericInfo2] const {getByText, container} = render( - + ) expect(getByText('1/2')).toBeTruthy() // the card shown is the first one @@ -96,21 +96,21 @@ describe('UnfurlPreview', () => { it('re-clamps the index when the shown card is dismissed away', () => { mockPreviews = [genericInfo, genericInfo2] const {getByText, container, rerender} = render( - + ) fireEvent.click(container.querySelector('.icon-gen-iconfont-arrow-right') as Element) expect(getByText('2/2')).toBeTruthy() // the second preview goes away; the index must fall back rather than blank the panel mockPreviews = [genericInfo] - rerender() + rerender() expect(container.textContent).toContain('Alpha') }) it('dismisses the shown card by its url when the close icon is clicked', () => { mockPreviews = [genericInfo, genericInfo2] const {container} = render( - + ) fireEvent.click(container.querySelector('.icon-gen-iconfont-close') as Element) expect(mockDismiss).toHaveBeenCalledWith('http://a.com') @@ -121,4 +121,15 @@ describe('UnfurlPreview', () => { fireEvent.click(container.querySelector('.icon-gen-iconfont-close') as Element) expect(mockDismiss).toHaveBeenCalledWith('http://b.com') }) + + it('offers no dismiss while editing, since an edit cannot carry suppression', () => { + mockPreviews = [genericInfo] + const {container} = render( + + ) + // the card still shows what will unfurl; it just does not offer a control that would + // hide it and change nothing about the posted edit + expect(container.textContent).toContain('Alpha') + expect(container.querySelector('.icon-gen-iconfont-close')).toBeNull() + }) }) diff --git a/shared/chat/conversation/input-area/unfurl-preview.tsx b/shared/chat/conversation/input-area/unfurl-preview.tsx index ddb9cf36be87..57c7aa129520 100644 --- a/shared/chat/conversation/input-area/unfurl-preview.tsx +++ b/shared/chat/conversation/input-area/unfurl-preview.tsx @@ -5,12 +5,15 @@ import UnfurlGenericView from '@/chat/conversation/messages/text/unfurl/unfurl-l import {useUnfurlPreviews} from '@/chat/conversation/unfurl-preview-state' type Props = { + // false while editing a message: the edit rpc cannot carry suppression, so offering a + // dismiss there would be a control that does nothing + canDismiss: boolean conversationIDKey: T.Chat.ConversationIDKey text: string } const UnfurlPreview = (p: Props) => { - const {conversationIDKey, text} = p + const {canDismiss, conversationIDKey, text} = p const styles = useStyles() const theme = Kb.Styles.useTheme() const {dismiss, previews} = useUnfurlPreviews(conversationIDKey, text) @@ -64,7 +67,7 @@ const UnfurlPreview = (p: Props) => { description={generic.description ?? undefined} favicon={generic.favicon ?? undefined} media={generic.media ?? undefined} - onClose={() => dismiss(preview.url)} + onClose={canDismiss ? () => dismiss(preview.url) : undefined} publishTime={generic.publishTime ?? undefined} siteName={generic.siteName} title={generic.title} From a6226dbf941398be15e32962e2460fa56b044ccd Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 24 Aug 2026 17:54:15 -0400 Subject: [PATCH 10/17] perf(chat): collapse concurrent preview scrapes of the same url Prefetch holds prefetchLock for its whole body, so the scrape, package and asset upload it does are serialized. PreviewURLs took no lock at all, and it is a synchronous rpc rather than a goroutine fired and forgotten, so it can be entered again while an earlier call is still running. The composer does exactly that whenever a link is edited before its fetch comes back: the client discards the stale response by request id, but the service had already fetched the page, downloaded the image and uploaded the assets for a card nobody would see. Preview scrapes now go through a singleflight keyed by uid, conversation and url, so overlapping calls for the same link share one fetch. Uses the groupcache singleflight already in the tree rather than adding a second implementation. This does not bound a caller that ignores the client and asks for many distinct urls at once. That would want a semaphore, and it only matters to something that already has the local socket. --- go/chat/unfurl/unfurler.go | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/go/chat/unfurl/unfurler.go b/go/chat/unfurl/unfurler.go index 7f9f13bd5f0b..90dbfb4c22e9 100644 --- a/go/chat/unfurl/unfurler.go +++ b/go/chat/unfurl/unfurler.go @@ -8,6 +8,7 @@ import ( "net/url" "sync" + "github.com/golang/groupcache/singleflight" "github.com/keybase/client/go/chat/attachments" "github.com/keybase/client/go/chat/globals" "github.com/keybase/client/go/chat/s3" @@ -54,11 +55,16 @@ type Unfurler struct { utils.DebugLabeler unfurlMap map[string]bool - extractor *Extractor - scraper *Scraper - packager *Packager - settings *Settings - sender UnfurlMessageSender + // collapses concurrent preview scrapes of the same url. Prefetch gets this for free + // from prefetchLock, which serializes it; PreviewURLs is a synchronous rpc that can be + // called again while an earlier call is still in flight, which the composer does + // whenever the user edits a link before its fetch comes back + previewGroup singleflight.Group + extractor *Extractor + scraper *Scraper + packager *Packager + settings *Settings + sender UnfurlMessageSender // testing unfurlCh chan *chat1.Unfurl @@ -422,11 +428,19 @@ func (u *Unfurler) PreviewURLs(ctx context.Context, uid gregor1.UID, convID chat continue } seen[hit.URL] = true - unfurl, err := u.scrapeAndPackage(ctx, uid, convID, hit.URL) + scraped, err := u.previewGroup.Do(fmt.Sprintf("%s:%s:%s", uid, convID, hit.URL), + func() (any, error) { + return u.scrapeAndPackage(ctx, uid, convID, hit.URL) + }) if err != nil { u.Debug(ctx, "PreviewURLs: unable to scrapeAndPackage: %s", err) continue } + unfurl, ok := scraped.(chat1.Unfurl) + if !ok { + u.Debug(ctx, "PreviewURLs: unexpected scrape result type: %T", scraped) + continue + } if !previewable(unfurl) { continue } From c90de6e3f34174513dcab2a31e4e2b9dec6b3667 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Tue, 25 Aug 2026 16:24:06 -0400 Subject: [PATCH 11/17] test(chat): assert the unfurl title is non-empty, not non-zero --- go/chat/unfurl/unfurler_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/chat/unfurl/unfurler_test.go b/go/chat/unfurl/unfurler_test.go index a30986362c42..212f8d81e948 100644 --- a/go/chat/unfurl/unfurler_test.go +++ b/go/chat/unfurl/unfurler_test.go @@ -208,7 +208,7 @@ func TestUnfurlerPreviewURLs(t *testing.T) { typ, err := res[0].Unfurl.UnfurlType() require.NoError(t, err) require.Equal(t, chat1.UnfurlType_GENERIC, typ) - require.NotZero(t, res[0].Unfurl.Generic().Title) + require.NotEmpty(t, res[0].Unfurl.Generic().Title) // duplicate URLs collapse to one entry res = unfurler.PreviewURLs(context.TODO(), uid, convID, url+" and again "+url) From f40da9d5c2fa681ca745f7a12f454898980aa5e4 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Tue, 25 Aug 2026 16:36:28 -0400 Subject: [PATCH 12/17] fix(chat): do not let an abandoned preview caller sink the shared scrape PreviewURLs collapses concurrent scrapes of the same url through a singleflight, but the shared work ran on whichever caller happened to win the group. That caller is the one most likely to go away: the composer issues a new PreviewURLs on every edit and abandons the one in flight, so its cancellation would surface as an error for every other caller waiting on the same url. Run the scrape on a context detached from cancellation and let each caller wait on its own context instead, so a caller that leaves stops waiting rather than taking the result away from everyone else. Bail out of the remaining hits once the caller is gone. Also key the group on hex rather than the raw bytes of the uid and convID, either of which can contain the separator. --- go/chat/unfurl/unfurler.go | 57 ++++++++++++++++++++++++++++++++------ 1 file changed, 48 insertions(+), 9 deletions(-) diff --git a/go/chat/unfurl/unfurler.go b/go/chat/unfurl/unfurler.go index 90dbfb4c22e9..28327026ca4f 100644 --- a/go/chat/unfurl/unfurler.go +++ b/go/chat/unfurl/unfurler.go @@ -413,6 +413,50 @@ func previewable(unfurl chat1.Unfurl) bool { return unfurl.Generic().MapInfo == nil } +// previewScrape scrapes and packages one url, collapsing concurrent calls for the same +// url into a single scrape. +// +// the shared scrape deliberately does not run on the calling context. the composer calls +// PreviewURLs again on every edit and abandons the call in flight, so whichever caller +// happens to win the singleflight is also the one most likely to go away: running the +// scrape on its context would fail every other caller waiting on the same url. callers +// still honour their own cancellation, they just stop waiting rather than kill the work. +func (u *Unfurler) previewScrape(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, + url string, +) (chat1.Unfurl, error) { + // %x, not %s: uid and convID are raw bytes and can contain the separator + key := fmt.Sprintf("%x:%x:%s", uid, convID, url) + // keeps the caller's log tags and identify behaviour and drops only the cancellation. + // the scraper bounds the work with its own request timeout + scrapeCtx := context.WithoutCancel(ctx) + type scrapeRes struct { + unfurl chat1.Unfurl + err error + } + ch := make(chan scrapeRes, 1) + go func() { + scraped, err := u.previewGroup.Do(key, func() (any, error) { + return u.scrapeAndPackage(scrapeCtx, uid, convID, url) + }) + if err != nil { + ch <- scrapeRes{err: err} + return + } + unfurl, ok := scraped.(chat1.Unfurl) + if !ok { + ch <- scrapeRes{err: fmt.Errorf("unexpected scrape result type: %T", scraped)} + return + } + ch <- scrapeRes{unfurl: unfurl} + }() + select { + case res := <-ch: + return res.unfurl, res.err + case <-ctx.Done(): + return chat1.Unfurl{}, ctx.Err() + } +} + func (u *Unfurler) PreviewURLs(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, text string, ) (res []chat1.UnfurlPreviewInfo) { @@ -428,17 +472,12 @@ func (u *Unfurler) PreviewURLs(ctx context.Context, uid gregor1.UID, convID chat continue } seen[hit.URL] = true - scraped, err := u.previewGroup.Do(fmt.Sprintf("%s:%s:%s", uid, convID, hit.URL), - func() (any, error) { - return u.scrapeAndPackage(ctx, uid, convID, hit.URL) - }) + unfurl, err := u.previewScrape(ctx, uid, convID, hit.URL) if err != nil { u.Debug(ctx, "PreviewURLs: unable to scrapeAndPackage: %s", err) - continue - } - unfurl, ok := scraped.(chat1.Unfurl) - if !ok { - u.Debug(ctx, "PreviewURLs: unexpected scrape result type: %T", scraped) + if ctx.Err() != nil { + return nil + } continue } if !previewable(unfurl) { From af341b883fde52d4a89384c5a149aa29caca0e07 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Tue, 25 Aug 2026 16:36:29 -0400 Subject: [PATCH 13/17] test(chat): pin the preview scrape shared by a cancelled caller Gates the test server mid-scrape so a second caller joins the first one's singleflight, then cancels the first. Asserts the cancelled caller returns nothing, the survivor still gets its preview, and one scrape served both. This pins the collapse and the cancelled caller's own return value. It does not discriminate the detached context on its own: colly takes no context, so cancellation never reaches the scrape stage, and the packaging stage that does honour it is stubbed out here. --- go/chat/unfurl/unfurler_test.go | 82 +++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/go/chat/unfurl/unfurler_test.go b/go/chat/unfurl/unfurler_test.go index 212f8d81e948..ff3ceeba9f41 100644 --- a/go/chat/unfurl/unfurler_test.go +++ b/go/chat/unfurl/unfurler_test.go @@ -1,9 +1,15 @@ package unfurl import ( + "bytes" "context" "fmt" + "io" + "net/http" + "os" + "path/filepath" "strings" + "sync/atomic" "testing" "time" @@ -15,6 +21,7 @@ import ( "github.com/keybase/client/go/libkb" "github.com/keybase/client/go/protocol/chat1" "github.com/keybase/client/go/protocol/gregor1" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -356,3 +363,78 @@ func TestUnfurlerSuppressPrompt(t *testing.T) { require.Fail(t, "no prompt for the unsuppressed URL") } } + +// an abandoned preview caller must not take the scrape down with it: the composer calls +// PreviewURLs again on every edit, so the caller that wins the singleflight is often the +// one that goes away first +func TestUnfurlerPreviewURLsCallerCancel(t *testing.T) { + tc := externalstest.SetupTest(t, "unfurler", 0) + defer tc.Cleanup() + g := globals.NewContext(tc.G, &globals.ChatContext{}) + + store := attachments.NewStoreTesting(g, nil) + s3signer := &ptsigner{} + g.ActivityNotifier = makeDummyActivityNotifier() + g.MessageDeliverer = dummyDeliverer{} + g.AttachmentURLSrv = types.DummyAttachmentHTTPSrv{} + sender := makeDummySender() + ri := func() chat1.RemoteInterface { return paramsRemote{} } + storage := newMemConversationBackedStorage() + unfurler := NewUnfurler(g, store, s3signer, storage, sender, ri) + + uid := gregor1.UID([]byte{0, 1}) + convID := chat1.ConversationID([]byte{0, 1, 2}) + + var scrapes int64 + started := make(chan struct{}, 1) + release := make(chan struct{}) + srv := newDummyHTTPSrv(t, func(w http.ResponseWriter, r *http.Request) { + name := r.URL.Query().Get("name") + if name == "wsj0.html" && atomic.AddInt64(&scrapes, 1) == 1 { + started <- struct{}{} + <-release + } + w.WriteHeader(200) + dat, err := os.ReadFile(filepath.Join("testcases", name)) + assert.NoError(t, err) + _, err = io.Copy(w, bytes.NewBuffer(dat)) + assert.NoError(t, err) + }) + addr := srv.Start() + defer srv.Stop() + + url := fmt.Sprintf("http://%s/?name=%s", addr, "wsj0.html") + require.NoError(t, unfurler.WhitelistAdd(context.TODO(), uid, "127.0.0.1")) + + // the first caller wins the singleflight and is then abandoned mid-scrape + firstCtx, cancelFirst := context.WithCancel(context.TODO()) + firstCh := make(chan []chat1.UnfurlPreviewInfo, 1) + go func() { firstCh <- unfurler.PreviewURLs(firstCtx, uid, convID, "check this out "+url) }() + select { + case <-started: + case <-time.After(20 * time.Second): + require.Fail(t, "scrape never started") + } + + // the second caller joins the same scrape, then the first one goes away + secondCh := make(chan []chat1.UnfurlPreviewInfo, 1) + go func() { secondCh <- unfurler.PreviewURLs(context.TODO(), uid, convID, "check this out "+url) }() + cancelFirst() + select { + case res := <-firstCh: + require.Empty(t, res, "a cancelled caller should not return a preview") + case <-time.After(20 * time.Second): + require.Fail(t, "cancelled caller never returned") + } + + close(release) + select { + case res := <-secondCh: + require.Len(t, res, 1, "the surviving caller lost its preview to the cancelled one") + require.Equal(t, url, res[0].Url) + require.NotEmpty(t, res[0].Unfurl.Generic().Title) + case <-time.After(20 * time.Second): + require.Fail(t, "surviving caller never returned") + } + require.Equal(t, int64(1), atomic.LoadInt64(&scrapes), "the two callers did not share one scrape") +} From 5fface64b74c0086a156b7ebc5d3469489d87736 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Tue, 25 Aug 2026 20:51:55 -0400 Subject: [PATCH 14/17] fix(chat): suppress urls the composer could not preview PreviewURLs dropped a url it failed to scrape or package, so the composer showed no card for it and the send unfurled it anyway: UnfurlAndSend queues and retries that url for minutes, landing a card the user never saw and had no way to decline. Those urls now come back with a nil unfurl (the avdl field becomes a union with null). The client keeps them in a `failed` set, separate from the dismissed set so a url that starts scraping again returns as a card instead of staying suppressed, and getSuppressedURLs unions both so the message unfurls exactly what the composer offered. Also puts focus back in the composer after a dismiss, since the X takes it. --- go/chat/unfurl/unfurler.go | 15 ++++-- go/chat/unfurl/unfurler_test.go | 37 ++++++++++++++ go/protocol/chat1/local.go | 14 ++++-- protocol/avdl/chat1/local.avdl | 4 +- protocol/json/chat1/local.json | 5 +- .../input-area/unfurl-preview.test.tsx | 21 ++++++++ .../input-area/unfurl-preview.tsx | 12 ++++- .../unfurl-preview-state.test.tsx | 35 +++++++++++++ .../conversation/unfurl-preview-state.tsx | 49 ++++++++++++++++--- shared/constants/rpc/rpc-chat-gen.tsx | 2 +- 10 files changed, 176 insertions(+), 18 deletions(-) diff --git a/go/chat/unfurl/unfurler.go b/go/chat/unfurl/unfurler.go index 28327026ca4f..250b9103e86b 100644 --- a/go/chat/unfurl/unfurler.go +++ b/go/chat/unfurl/unfurler.go @@ -399,8 +399,9 @@ func (u *Unfurler) Prefetch(ctx context.Context, uid gregor1.UID, convID chat1.C } // PreviewURLs scrapes and packages the whitelisted URLs in text and returns -// display-ready unfurls, so a client can show a preview before sending. Only -// generic unfurls are returned; failures are skipped rather than returned. +// display-ready unfurls, so a client can show a preview before sending. Only generic +// unfurls carry a display; a url that cannot be previewed at all comes back with a nil +// unfurl so the client can suppress it on send. // previewable reports whether an unfurl can be shown in the pre-send preview. only generic // unfurls render there, and a map unfurl is a generic unfurl with MapInfo set, which the // message view itself refuses to render. the frontend filters on the same rule, so keep the @@ -478,17 +479,25 @@ func (u *Unfurler) PreviewURLs(ctx context.Context, uid gregor1.UID, convID chat if ctx.Err() != nil { return nil } + // reported rather than dropped: UnfurlAndSend would still queue this url and + // retry it in the background for minutes after the send, so a card the user + // never saw could land in the sent message with no way to have declined it. + // telling the client makes it suppress the url instead + res = append(res, chat1.UnfurlPreviewInfo{Url: hit.URL}) continue } if !previewable(unfurl) { + // a giphy or a map: no card in the composer, but the send unfurls it the same + // as it always has, so this is not something the client should suppress continue } disp, err := display.DisplayUnfurl(ctx, u.G().AttachmentURLSrv, convID, unfurl) if err != nil { u.Debug(ctx, "PreviewURLs: failed to display: %s", err) + res = append(res, chat1.UnfurlPreviewInfo{Url: hit.URL}) continue } - res = append(res, chat1.UnfurlPreviewInfo{Url: hit.URL, Unfurl: disp}) + res = append(res, chat1.UnfurlPreviewInfo{Url: hit.URL, Unfurl: &disp}) } return res } diff --git a/go/chat/unfurl/unfurler_test.go b/go/chat/unfurl/unfurler_test.go index ff3ceeba9f41..56d15c51f0de 100644 --- a/go/chat/unfurl/unfurler_test.go +++ b/go/chat/unfurl/unfurler_test.go @@ -212,6 +212,7 @@ func TestUnfurlerPreviewURLs(t *testing.T) { res := unfurler.PreviewURLs(context.TODO(), uid, convID, "check this out "+url) require.Len(t, res, 1) require.Equal(t, url, res[0].Url) + require.NotNil(t, res[0].Unfurl) typ, err := res[0].Unfurl.UnfurlType() require.NoError(t, err) require.Equal(t, chat1.UnfurlType_GENERIC, typ) @@ -225,6 +226,41 @@ func TestUnfurlerPreviewURLs(t *testing.T) { require.Empty(t, unfurler.PreviewURLs(context.TODO(), uid, convID, "no links here")) } +// a url the scraper cannot fetch still comes back, with no unfurl on it: the send path +// would queue an unfurl for it anyway, so the client needs to know to suppress it +func TestUnfurlerPreviewURLsScrapeFailure(t *testing.T) { + tc := externalstest.SetupTest(t, "unfurler", 0) + defer tc.Cleanup() + g := globals.NewContext(tc.G, &globals.ChatContext{}) + + store := attachments.NewStoreTesting(g, nil) + s3signer := &ptsigner{} + g.ActivityNotifier = makeDummyActivityNotifier() + g.MessageDeliverer = dummyDeliverer{} + g.AttachmentURLSrv = types.DummyAttachmentHTTPSrv{} + sender := makeDummySender() + ri := func() chat1.RemoteInterface { return paramsRemote{} } + storage := newMemConversationBackedStorage() + unfurler := NewUnfurler(g, store, s3signer, storage, sender, ri) + + uid := gregor1.UID([]byte{0, 1}) + convID := chat1.ConversationID([]byte{0, 1, 2}) + srv := newDummyHTTPSrv(t, func(w http.ResponseWriter, r *http.Request) { + // what wsj.com does to the scraper + w.WriteHeader(http.StatusUnauthorized) + }) + addr := srv.Start() + defer srv.Stop() + + url := fmt.Sprintf("http://%s/?name=%s", addr, "wsj0.html") + require.NoError(t, unfurler.WhitelistAdd(context.TODO(), uid, "127.0.0.1")) + + res := unfurler.PreviewURLs(context.TODO(), uid, convID, "check this out "+url) + require.Len(t, res, 1) + require.Equal(t, url, res[0].Url) + require.Nil(t, res[0].Unfurl) +} + func makeTextMsgWithMsgID(msgBody string, outboxID chat1.OutboxID, msgID chat1.MessageID) chat1.MessageUnboxed { return chat1.NewMessageUnboxedWithValid(chat1.MessageUnboxedValid{ ClientHeader: chat1.MessageClientHeaderVerified{ @@ -432,6 +468,7 @@ func TestUnfurlerPreviewURLsCallerCancel(t *testing.T) { case res := <-secondCh: require.Len(t, res, 1, "the surviving caller lost its preview to the cancelled one") require.Equal(t, url, res[0].Url) + require.NotNil(t, res[0].Unfurl) require.NotEmpty(t, res[0].Unfurl.Generic().Title) case <-time.After(20 * time.Second): require.Fail(t, "surviving caller never returned") diff --git a/go/protocol/chat1/local.go b/go/protocol/chat1/local.go index b46e2b575249..16d40c1171d1 100644 --- a/go/protocol/chat1/local.go +++ b/go/protocol/chat1/local.go @@ -5857,14 +5857,20 @@ func (o UnfurlPromptResult) DeepCopy() UnfurlPromptResult { } type UnfurlPreviewInfo struct { - Url string `codec:"url" json:"url"` - Unfurl UnfurlDisplay `codec:"unfurl" json:"unfurl"` + Url string `codec:"url" json:"url"` + Unfurl *UnfurlDisplay `codec:"unfurl,omitempty" json:"unfurl,omitempty"` } func (o UnfurlPreviewInfo) DeepCopy() UnfurlPreviewInfo { return UnfurlPreviewInfo{ - Url: o.Url, - Unfurl: o.Unfurl.DeepCopy(), + Url: o.Url, + Unfurl: (func(x *UnfurlDisplay) *UnfurlDisplay { + if x == nil { + return nil + } + tmp := x.DeepCopy() + return &tmp + })(o.Unfurl), } } diff --git a/protocol/avdl/chat1/local.avdl b/protocol/avdl/chat1/local.avdl index 52cbfb2cc52a..ee55421303bb 100644 --- a/protocol/avdl/chat1/local.avdl +++ b/protocol/avdl/chat1/local.avdl @@ -1248,7 +1248,9 @@ protocol local { record UnfurlPreviewInfo { string url; - UnfurlDisplay unfurl; + // null when the url could not be scraped or packaged. the client suppresses those + // urls on send, so the preview it showed is exactly what the message unfurls + union { null, UnfurlDisplay } unfurl; } @timeout_msec(15000) diff --git a/protocol/json/chat1/local.json b/protocol/json/chat1/local.json index d0c4b0673b2d..6188aa6d0cd8 100644 --- a/protocol/json/chat1/local.json +++ b/protocol/json/chat1/local.json @@ -3688,7 +3688,10 @@ "name": "url" }, { - "type": "UnfurlDisplay", + "type": [ + null, + "UnfurlDisplay" + ], "name": "unfurl" } ] diff --git a/shared/chat/conversation/input-area/unfurl-preview.test.tsx b/shared/chat/conversation/input-area/unfurl-preview.test.tsx index bacd0a473d41..d431500afc69 100644 --- a/shared/chat/conversation/input-area/unfurl-preview.test.tsx +++ b/shared/chat/conversation/input-area/unfurl-preview.test.tsx @@ -3,6 +3,7 @@ import * as T from '@/constants/types' import {fireEvent, render} from '@testing-library/react' import UnfurlPreview from './unfurl-preview' +import {ThreadRefsContext} from '@/chat/conversation/normal/context' const mockDismiss = jest.fn() let mockPreviews: ReadonlyArray = [] @@ -122,6 +123,26 @@ describe('UnfurlPreview', () => { expect(mockDismiss).toHaveBeenCalledWith('http://b.com') }) + it('puts focus back in the composer after a dismiss', () => { + mockPreviews = [genericInfo] + const focusInput = jest.fn() + const refs = { + focusInput, + scrollDown: () => {}, + scrollToBottom: () => {}, + scrollUp: () => {}, + setInputRef: () => {}, + setScrollRef: () => {}, + } + const {container} = render( + + + + ) + fireEvent.click(container.querySelector('.icon-gen-iconfont-close') as Element) + expect(focusInput).toHaveBeenCalled() + }) + it('offers no dismiss while editing, since an edit cannot carry suppression', () => { mockPreviews = [genericInfo] const {container} = render( diff --git a/shared/chat/conversation/input-area/unfurl-preview.tsx b/shared/chat/conversation/input-area/unfurl-preview.tsx index 57c7aa129520..955fe823d999 100644 --- a/shared/chat/conversation/input-area/unfurl-preview.tsx +++ b/shared/chat/conversation/input-area/unfurl-preview.tsx @@ -3,6 +3,7 @@ import * as React from 'react' import * as T from '@/constants/types' import UnfurlGenericView from '@/chat/conversation/messages/text/unfurl/unfurl-list/generic-view' import {useUnfurlPreviews} from '@/chat/conversation/unfurl-preview-state' +import {ThreadRefsContext} from '@/chat/conversation/normal/context' type Props = { // false while editing a message: the edit rpc cannot carry suppression, so offering a @@ -17,6 +18,7 @@ const UnfurlPreview = (p: Props) => { const styles = useStyles() const theme = Kb.Styles.useTheme() const {dismiss, previews} = useUnfurlPreviews(conversationIDKey, text) + const {focusInput} = React.useContext(ThreadRefsContext) const [index, setIndex] = React.useState(0) const genericPreviews = previews.flatMap(preview => { const {unfurl} = preview @@ -67,7 +69,15 @@ const UnfurlPreview = (p: Props) => { description={generic.description ?? undefined} favicon={generic.favicon ?? undefined} media={generic.media ?? undefined} - onClose={canDismiss ? () => dismiss(preview.url) : undefined} + onClose={ + canDismiss + ? () => { + dismiss(preview.url) + // the X takes focus on the way out, and the composer is where the user was + focusInput() + } + : undefined + } publishTime={generic.publishTime ?? undefined} siteName={generic.siteName} title={generic.title} diff --git a/shared/chat/conversation/unfurl-preview-state.test.tsx b/shared/chat/conversation/unfurl-preview-state.test.tsx index 343d95e112b8..ee647ea8e14c 100644 --- a/shared/chat/conversation/unfurl-preview-state.test.tsx +++ b/shared/chat/conversation/unfurl-preview-state.test.tsx @@ -11,6 +11,9 @@ const convID = T.Chat.conversationIDToKey(new Uint8Array([1, 2, 3, 4])) const info = (url: string): T.RPCChat.UnfurlPreviewInfo => ({unfurl: {generic: {title: url, url}, unfurlType: T.RPCChat.UnfurlType.generic}, url}) as T.RPCChat.UnfurlPreviewInfo +// a url the service could not scrape: reported so it can be suppressed, no unfurl on it +const failedInfo = (url: string): T.RPCChat.UnfurlPreviewInfo => ({url}) as T.RPCChat.UnfurlPreviewInfo + const Harness = (p: { text: string id?: T.Chat.ConversationIDKey @@ -89,6 +92,38 @@ describe('unfurl previews', () => { expect(getSuppressedURLs(convID)).toEqual(['http://a.com']) }) + it('suppresses a url the service could not preview, and shows no card for it', async () => { + jest + .spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise') + .mockResolvedValue([failedInfo('http://wsj.com'), info('http://a.com')]) + let last: ReturnType | undefined + render( (last = r)} />) + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(last?.previews.map(p => p.url)).toEqual(['http://a.com'])) + // the send path would unfurl wsj minutes later otherwise, with no card to decline + expect(getSuppressedURLs(convID)).toEqual(['http://wsj.com']) + }) + + it('offers the card again once a url that failed starts previewing', async () => { + const spy = jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise') + spy.mockResolvedValueOnce([failedInfo('http://a.com')]) + spy.mockResolvedValueOnce([info('http://a.com')]) + let last: ReturnType | undefined + const {rerender} = render( (last = r)} />) + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(getSuppressedURLs(convID)).toEqual(['http://a.com'])) + rerender( (last = r)} />) + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(last?.previews.length).toBe(1)) + expect(getSuppressedURLs(convID)).toEqual([]) + }) + it('forgets a dismissal once the url leaves the text', async () => { jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise').mockResolvedValue([]) let last: ReturnType | undefined diff --git a/shared/chat/conversation/unfurl-preview-state.tsx b/shared/chat/conversation/unfurl-preview-state.tsx index 79b59416c984..863cbcecb288 100644 --- a/shared/chat/conversation/unfurl-preview-state.tsx +++ b/shared/chat/conversation/unfurl-preview-state.tsx @@ -6,16 +6,23 @@ import logger from '@/logger' type State = T.Immutable<{ dismissed: Map> + // urls the service could not preview. they suppress on send like a dismissal does, but + // are kept apart from one: the next fetch replaces this set wholesale, and a url that + // starts scraping again must come back as a card, which it could not do if a failure + // had been recorded as something the user dismissed + failed: Map> dispatch: { dismiss: (conversationIDKey: T.Chat.ConversationIDKey, urls: ReadonlyArray) => void keepOnly: (conversationIDKey: T.Chat.ConversationIDKey, urls: ReadonlyArray) => void remove: (conversationIDKey: T.Chat.ConversationIDKey, urls: ReadonlyArray) => void + setFailed: (conversationIDKey: T.Chat.ConversationIDKey, urls: ReadonlyArray) => void resetState: () => void } }> export const useUnfurlPreviewState = Z.createZustand('unfurl-preview', set => ({ dismissed: new Map(), + failed: new Map(), dispatch: { // also the restore path after a canceled send: both mean "these urls are suppressed" dismiss: (conversationIDKey, urls) => { @@ -36,6 +43,15 @@ export const useUnfurlPreviewState = Z.createZustand('unfurl-preview', se if (!existing.size) s.dismissed.delete(conversationIDKey) }) }, + setFailed: (conversationIDKey, urls) => { + set(s => { + if (!urls.length) { + s.failed.delete(conversationIDKey) + return + } + s.failed.set(conversationIDKey, new Set(urls)) + }) + }, remove: (conversationIDKey, urls) => { set(s => { const existing = s.dismissed.get(conversationIDKey) @@ -48,9 +64,14 @@ export const useUnfurlPreviewState = Z.createZustand('unfurl-preview', se }, })) -export const getSuppressedURLs = (conversationIDKey: T.Chat.ConversationIDKey) => [ - ...(useUnfurlPreviewState.getState().dismissed.get(conversationIDKey) ?? []), -] +// what the user dismissed plus what could not be previewed: the send suppresses both, so +// the message unfurls exactly the cards the composer offered +export const getSuppressedURLs = (conversationIDKey: T.Chat.ConversationIDKey) => { + const {dismissed, failed} = useUnfurlPreviewState.getState() + return [ + ...new Set([...(dismissed.get(conversationIDKey) ?? []), ...(failed.get(conversationIDKey) ?? [])]), + ] +} // dropped once the send they belong to lands; a targeted remove rather than a // whole-conversation clear so a dismissal made while that send was in flight survives @@ -97,7 +118,7 @@ const fetchPreviews = async ( export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, text: string) => { const [fetched, setFetched] = React.useState>([]) const dismissedSet = useUnfurlPreviewState(s => s.dismissed.get(conversationIDKey)) - const {dismiss: dismissURL, keepOnly} = useUnfurlPreviewState(s => s.dispatch) + const {dismiss: dismissURL, keepOnly, setFailed} = useUnfurlPreviewState(s => s.dispatch) const requestIDRef = React.useRef(0) // the input subtree remounts per conversation (key={conversationIDKey} on the provider), // so the first render of a conversation we return to always has empty text before the @@ -113,8 +134,12 @@ export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, t fetchedConversationIDKey, infos.map(i => i.url) ) + setFailed( + fetchedConversationIDKey, + infos.filter(i => !i.unfurl).map(i => i.url) + ) }, - [keepOnly] + [keepOnly, setFailed] ) React.useEffect(() => { @@ -122,6 +147,7 @@ export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, t if (!text.includes('http')) { if (sawTextRef.current) { keepOnly(conversationIDKey, []) + setFailed(conversationIDKey, []) } sawTextRef.current = sawTextRef.current || !!text return @@ -133,7 +159,7 @@ export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, t return () => { clearTimeout(timeoutID) } - }, [conversationIDKey, keepOnly, text, onFetched]) + }, [conversationIDKey, keepOnly, setFailed, text, onFetched]) const dismiss = React.useCallback( (url: string) => { @@ -146,8 +172,17 @@ export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, t // replace these previews can fail or still be in flight, and showing a card for a url the // user has since deleted is worse than showing nothing: its X would suppress a link that // is not in the message, while the link that is about to send never gets offered one. + // an entry with no unfurl is a url the service could not preview. it is already + // suppressed for the send, so there is nothing to show and nothing to dismiss const visible = React.useMemo( - () => (hasLink ? fetched.filter(p => text.includes(p.url) && !dismissedSet?.has(p.url)) : []), + () => + hasLink + ? fetched.flatMap(p => + p.unfurl && text.includes(p.url) && !dismissedSet?.has(p.url) + ? [{unfurl: p.unfurl, url: p.url}] + : [] + ) + : [], [hasLink, fetched, text, dismissedSet] ) return {dismiss, previews: visible} diff --git a/shared/constants/rpc/rpc-chat-gen.tsx b/shared/constants/rpc/rpc-chat-gen.tsx index 7e73f07ab61c..3fac088cb733 100644 --- a/shared/constants/rpc/rpc-chat-gen.tsx +++ b/shared/constants/rpc/rpc-chat-gen.tsx @@ -1578,7 +1578,7 @@ export type UnfurlGiphyDisplay = {readonly favicon?: UnfurlImageDisplay | null,r export type UnfurlGiphyRaw = {readonly imageUrl?: string | null,readonly video?: UnfurlVideo | null,readonly faviconUrl?: string | null,} export type UnfurlImageDisplay = {readonly url: string,readonly height: number,readonly width: number,readonly isVideo: boolean,} export type UnfurlMapsRaw = {readonly title: string,readonly url: string,readonly siteName: string,readonly imageUrl: string,readonly historyImageUrl?: string | null,readonly description: string,readonly coord: Coordinate,readonly time: Gregor1.Time,readonly liveLocationEndTime?: Gregor1.Time | null,readonly liveLocationDone: boolean,} -export type UnfurlPreviewInfo = {readonly url: string,readonly unfurl: UnfurlDisplay,} +export type UnfurlPreviewInfo = {readonly url: string,readonly unfurl?: UnfurlDisplay | null,} export type UnfurlPromptResult ={ actionType: UnfurlPromptAction.always } | { actionType: UnfurlPromptAction.never } | { actionType: UnfurlPromptAction.notnow } | { actionType: UnfurlPromptAction.accept, accept: string } | { actionType: UnfurlPromptAction.onetime, onetime: string } export type UnfurlRaw ={ unfurlType: UnfurlType.generic, generic: UnfurlGenericRaw } | { unfurlType: UnfurlType.youtube, youtube: UnfurlYoutubeRaw } | { unfurlType: UnfurlType.giphy, giphy: UnfurlGiphyRaw } | { unfurlType: UnfurlType.maps, maps: UnfurlMapsRaw } export type UnfurlResult = {readonly unfurl: Unfurl,readonly url: string,} From 569afcc66d35743dfc6b8d7f43c22d3718ee24af Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Tue, 25 Aug 2026 21:16:34 -0400 Subject: [PATCH 15/17] fix(chat): keep preview failures apart from dismissals through a send Review of the previous commit turned up three ways the failed set could misbehave, plus one url class it should never have covered. A canceled stellar send restores the snapshot it took, and that snapshot had already been flattened into a single list, so a scrape failure came back as a user dismissal. A dismissal is never re-derived, so the url stayed suppressed for the rest of the conversation with no card to un-suppress it. The send now carries the two sets apart and restores only the dismissals; the failures come back on their own from the next fetch. The request guard lives in a ref, so it only discriminated against a mount's own older fetches. Leaving a conversation and returning remounts the hook, and the fetch the dead mount left in flight still matched its own id and wrote over what the new mount had since fetched, in either direction. Unmounting now retires the mount's ids. A giphy or a maps url gets no card either way, which is why previewable() excludes them, but a scrape failure on one was still reported and suppressed -- losing an unfurl the send's own retries would have landed. Those are skipped now, matching the success path. Tests: the failed set replacing wholesale rather than accumulating, the set clearing when the link leaves the text, the retired-mount response, the restore path not recording a failure as a dismissal, the union reaching the wire, and the auto-whitelist skip. Each was mutation-checked against the unfixed code. Also comments why sending before the previews land suppresses nothing. --- go/chat/unfurl/unfurler.go | 6 ++ go/chat/unfurl/unfurler_test.go | 26 ++++++++ .../input-area/input-state.test.tsx | 36 ++++++++++- .../conversation/input-area/input-state.tsx | 5 +- .../conversation/input-area/normal/index.tsx | 10 +++- shared/chat/conversation/send-actions.tsx | 15 +++-- .../unfurl-preview-state.test.tsx | 60 +++++++++++++++++++ .../conversation/unfurl-preview-state.tsx | 45 ++++++++++---- 8 files changed, 184 insertions(+), 19 deletions(-) diff --git a/go/chat/unfurl/unfurler.go b/go/chat/unfurl/unfurler.go index 250b9103e86b..0ce1ea8a3e35 100644 --- a/go/chat/unfurl/unfurler.go +++ b/go/chat/unfurl/unfurler.go @@ -479,6 +479,12 @@ func (u *Unfurler) PreviewURLs(ctx context.Context, uid gregor1.UID, convID chat if ctx.Err() != nil { return nil } + if u.extractor.isAutoWhitelistFromHit(ctx, hit.URL) { + // a giphy or a map, same as the previewable check below: it was never going + // to get a card, so one transient scrape failure here must not suppress an + // unfurl the send's own retries would have landed + continue + } // reported rather than dropped: UnfurlAndSend would still queue this url and // retry it in the background for minutes after the send, so a card the user // never saw could land in the sent message with no way to have declined it. diff --git a/go/chat/unfurl/unfurler_test.go b/go/chat/unfurl/unfurler_test.go index 56d15c51f0de..14e52c4f7289 100644 --- a/go/chat/unfurl/unfurler_test.go +++ b/go/chat/unfurl/unfurler_test.go @@ -475,3 +475,29 @@ func TestUnfurlerPreviewURLsCallerCancel(t *testing.T) { } require.Equal(t, int64(1), atomic.LoadInt64(&scrapes), "the two callers did not share one scrape") } + +// a giphy or a maps url gets no card either way, so a scrape failure on one must not be +// reported: suppressing it would lose an unfurl the send's own retries would have landed +func TestUnfurlerPreviewURLsAutoWhitelistFailureNotSuppressed(t *testing.T) { + tc := externalstest.SetupTest(t, "unfurler", 0) + defer tc.Cleanup() + g := globals.NewContext(tc.G, &globals.ChatContext{}) + + store := attachments.NewStoreTesting(g, nil) + s3signer := &ptsigner{} + g.ActivityNotifier = makeDummyActivityNotifier() + g.MessageDeliverer = dummyDeliverer{} + g.AttachmentURLSrv = types.DummyAttachmentHTTPSrv{} + sender := makeDummySender() + ri := func() chat1.RemoteInterface { return paramsRemote{} } + storage := newMemConversationBackedStorage() + unfurler := NewUnfurler(g, store, s3signer, storage, sender, ri) + + uid := gregor1.UID([]byte{0, 1}) + convID := chat1.ConversationID([]byte{0, 1, 2}) + // the maps domain is auto-whitelisted and scraped locally, so an unparseable coord + // fails the scrape without touching the network + url := fmt.Sprintf("https://%s/?lat=nope&lon=1&acc=1&done=true", types.MapsDomain) + + require.Empty(t, unfurler.PreviewURLs(context.TODO(), uid, convID, "check this out "+url)) +} diff --git a/shared/chat/conversation/input-area/input-state.test.tsx b/shared/chat/conversation/input-area/input-state.test.tsx index 7ecc294f1154..b03f94cfec77 100644 --- a/shared/chat/conversation/input-area/input-state.test.tsx +++ b/shared/chat/conversation/input-area/input-state.test.tsx @@ -497,13 +497,47 @@ test('a canceled stellar send restores the dismissed unfurl urls for the resend' const {result} = renderInput() act(() => { - result.current.dispatch.sendComposerText('hi http://a.com', ['http://a.com']) + result.current.dispatch.sendComposerText('hi http://a.com', {dismissed: ['http://a.com'], failed: []}) }) await flushPromises() expect(getSuppressedURLs(convID)).toEqual(['http://a.com']) }) +test('a canceled stellar send leaves a failed preview unrecorded as a dismissal', async () => { + // a failure is re-derived by the next fetch, a dismissal never is, so restoring one as + // the other would keep the url suppressed even after it starts scraping again + jest.spyOn(T.RPCChat, 'localPostTextNonblockRpcListener').mockImplementation(async p => { + p.incomingCallMap['chat.1.chatUi.chatStellarDone']?.({canceled: true}) + await Promise.resolve() + return {outboxID: makeRpcOutboxID('posted-outbox')} + }) + const {result} = renderInput() + + act(() => { + result.current.dispatch.sendComposerText('hi http://wsj.com', {dismissed: [], failed: ['http://wsj.com']}) + }) + await flushPromises() + + expect(useUnfurlPreviewState.getState().dismissed.get(convID)).toBeUndefined() + expect(getSuppressedURLs(convID)).toEqual([]) +}) + +test('a send suppresses what failed to preview as well as what was dismissed', async () => { + const getLastPost = mockPostText() + const {result} = renderInput() + + act(() => { + result.current.dispatch.sendComposerText('hi http://a.com http://wsj.com', { + dismissed: ['http://a.com'], + failed: ['http://wsj.com'], + }) + }) + await flushPromises() + + expect(getLastPost()?.params.unfurlSuppress).toEqual(['http://a.com', 'http://wsj.com']) +}) + test('toggleGiphyPrefill toggles the slash command text', () => { const {result} = renderInput() diff --git a/shared/chat/conversation/input-area/input-state.tsx b/shared/chat/conversation/input-area/input-state.tsx index 42255ce62902..b2b7178c0c38 100644 --- a/shared/chat/conversation/input-area/input-state.tsx +++ b/shared/chat/conversation/input-area/input-state.tsx @@ -7,6 +7,7 @@ import {useCurrentUserState} from '@/stores/current-user' import {useEngineActionListener} from '@/engine/action-listener' import {useConversationThreadStore} from '../thread-context' import {useConversationSendActions} from '../send-actions' +import type {SuppressSnapshot} from '../unfurl-preview-state' type ConversationInputStore = T.Immutable<{ commandMarkdown?: T.RPCChat.UICommandMarkdown @@ -22,7 +23,7 @@ type ConversationInputStore = T.Immutable<{ type ConversationInputDispatch = { injectIntoInput: (text?: string, focus?: boolean) => void resetState: () => void - sendComposerText: (text: string, unfurlSuppress?: ReadonlyArray) => void + sendComposerText: (text: string, unfurlSuppress?: SuppressSnapshot) => void sendGiphyResult: (result: T.RPCChat.GiphySearchResult) => void setCommandMarkdown: (md?: T.RPCChat.UICommandMarkdown) => void setCommandStatusInfo: (info?: T.Chat.CommandStatusInfo) => void @@ -174,7 +175,7 @@ export const ConversationInputProvider = (p: React.PropsWithChildren<{id: T.Chat }) } }) - const sendComposerText = React.useEffectEvent((text: string, unfurlSuppress?: ReadonlyArray) => { + const sendComposerText = React.useEffectEvent((text: string, unfurlSuppress?: SuppressSnapshot) => { sendMessage(text, { editingOrdinal: state.editing, onRestoreText: injectIntoInput, diff --git a/shared/chat/conversation/input-area/normal/index.tsx b/shared/chat/conversation/input-area/normal/index.tsx index cda7d6a8c568..6dce654e0c47 100644 --- a/shared/chat/conversation/input-area/normal/index.tsx +++ b/shared/chat/conversation/input-area/normal/index.tsx @@ -27,7 +27,7 @@ import {useConversationParticipantsSelector} from '../../data-hooks' import {useCurrentUserState} from '@/stores/current-user' import {useRoute} from '@react-navigation/native' import {metasReceived, unboxRows, useInboxMetadataState} from '@/chat/inbox/metadata' -import {getSuppressedURLs} from '@/chat/conversation/unfurl-preview-state' +import {takeSuppressSnapshot} from '@/chat/conversation/unfurl-preview-state' const useHintText = (p: { isExploding: boolean @@ -212,7 +212,13 @@ const ConnectedPlatformInput = function ConnectedPlatformInput() { // Snapshot the dismissed unfurls before clearing: the clear runs onChangeText('') // synchronously, and the preview hook drops every dismissal for empty text, which // would beat the deferred send to the store and unfurl a card the user dismissed. - const unfurlSuppress = getSuppressedURLs(conversationIDKey) + // + // Sending before the previews land (paste and hit enter: 200ms draft throttle + 500ms + // debounce + the scrape itself) suppresses nothing, so the message unfurls the url the + // way it always has. That is deliberate. The composer only promises to show what will + // unfurl once its previews have settled; suppressing urls it has not heard about yet + // would mean a link sent quickly never unfurls at all, which is how most links go out. + const unfurlSuppress = takeSuppressSnapshot(conversationIDKey) injectText('', true) setTimeout(() => { sendComposerText(text, unfurlSuppress) diff --git a/shared/chat/conversation/send-actions.tsx b/shared/chat/conversation/send-actions.tsx index ef2592e2166f..6f98b7dadbf0 100644 --- a/shared/chat/conversation/send-actions.tsx +++ b/shared/chat/conversation/send-actions.tsx @@ -4,7 +4,13 @@ import logger from '@/logger' import {RPCError} from '@/util/errors' import {ignorePromise} from '@/constants/utils' import {getClientPrevFromThread} from './attachment-actions' -import {getSuppressedURLs, removeSuppressedURLs, restoreSuppressedURLs} from './unfurl-preview-state' +import { + removeSuppressedURLs, + restoreSuppressedURLs, + suppressedURLsOf, + takeSuppressSnapshot, + type SuppressSnapshot, +} from './unfurl-preview-state' import {useInboxMetadataState} from '../inbox/metadata-store' import { useConversationThreadActions, @@ -138,7 +144,7 @@ export const useConversationSendActions = () => { editingOrdinal?: T.Chat.Ordinal onRestoreText?: (text: string) => void replyToOrdinal?: T.Chat.Ordinal - unfurlSuppress?: ReadonlyArray + unfurlSuppress?: SuppressSnapshot } ) => { const editOrdinal = context?.editingOrdinal @@ -153,7 +159,8 @@ export const useConversationSendActions = () => { const replyTo = threadStore.getState().messageMap.get(replyToOrdinal ?? T.Chat.numberToOrdinal(0))?.id // the caller passes a snapshot taken before it cleared the composer: clearing runs // synchronously and the preview hook drops every dismissal once the text is empty - const unfurlSuppress = context?.unfurlSuppress ?? getSuppressedURLs(conversationIDKey) + const snapshot = context?.unfurlSuppress ?? takeSuppressSnapshot(conversationIDKey) + const unfurlSuppress = suppressedURLsOf(snapshot) const onRestoreText = context?.onRestoreText sendTextMessageStoreless({ clientPrev: getClientPrev(), @@ -161,7 +168,7 @@ export const useConversationSendActions = () => { ephemeralLifetime: threadStore.getState().explodingMode, onRestoreText: onRestoreText ? (restored: string) => { - restoreSuppressedURLs(conversationIDKey, unfurlSuppress) + restoreSuppressedURLs(conversationIDKey, snapshot) onRestoreText(restored) } : undefined, diff --git a/shared/chat/conversation/unfurl-preview-state.test.tsx b/shared/chat/conversation/unfurl-preview-state.test.tsx index ee647ea8e14c..05fb390f38ce 100644 --- a/shared/chat/conversation/unfurl-preview-state.test.tsx +++ b/shared/chat/conversation/unfurl-preview-state.test.tsx @@ -124,6 +124,66 @@ describe('unfurl previews', () => { expect(getSuppressedURLs(convID)).toEqual([]) }) + it('replaces the failed set wholesale rather than accumulating', async () => { + const spy = jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise') + spy.mockResolvedValueOnce([failedInfo('http://a.com'), failedInfo('http://b.com')]) + spy.mockResolvedValueOnce([info('http://a.com'), failedInfo('http://b.com')]) + const {rerender} = render( {}} />) + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(getSuppressedURLs(convID)).toEqual(['http://a.com', 'http://b.com'])) + rerender( {}} />) + act(() => { + jest.advanceTimersByTime(500) + }) + // a recovered to a card while b is still failing: keeping a suppressed would hide the + // card the composer is now showing + await waitFor(() => expect(getSuppressedURLs(convID)).toEqual(['http://b.com'])) + }) + + it('forgets a failure once the url leaves the text', async () => { + jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise').mockResolvedValue([failedInfo('http://a.com')]) + const {rerender} = render( {}} />) + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(getSuppressedURLs(convID)).toEqual(['http://a.com'])) + rerender( {}} />) + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(getSuppressedURLs(convID)).toEqual([])) + }) + + it('drops a response left in flight by a mount that has gone away', async () => { + const spy = jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise') + let resolveFirst: (infos: ReadonlyArray) => void = () => {} + spy.mockImplementationOnce( + async () => new Promise>(resolve => (resolveFirst = resolve)) + ) + spy.mockResolvedValueOnce([info('http://a.com')]) + // the conversation the user leaves, with a scrape still running + const first = render( {}} />) + act(() => { + jest.advanceTimersByTime(500) + }) + first.unmount() + // and the one they come back to, which finishes its own fetch first + let last: ReturnType | undefined + render( (last = r)} />) + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(last?.previews.length).toBe(1)) + await act(async () => { + resolveFirst([failedInfo('http://a.com')]) + await Promise.resolve() + }) + expect(getSuppressedURLs(convID)).toEqual([]) + expect(last?.previews.length).toBe(1) + }) + it('forgets a dismissal once the url leaves the text', async () => { jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise').mockResolvedValue([]) let last: ReturnType | undefined diff --git a/shared/chat/conversation/unfurl-preview-state.tsx b/shared/chat/conversation/unfurl-preview-state.tsx index 863cbcecb288..500df5722504 100644 --- a/shared/chat/conversation/unfurl-preview-state.tsx +++ b/shared/chat/conversation/unfurl-preview-state.tsx @@ -64,15 +64,28 @@ export const useUnfurlPreviewState = Z.createZustand('unfurl-preview', se }, })) -// what the user dismissed plus what could not be previewed: the send suppresses both, so -// the message unfurls exactly the cards the composer offered -export const getSuppressedURLs = (conversationIDKey: T.Chat.ConversationIDKey) => { +// the two suppression sources kept apart. a send needs them apart because it may have to +// put them back: restoring a scrape failure as a dismissal would bury the url for good, +// since a dismissal is never re-derived while a failure is, on the next fetch +export type SuppressSnapshot = T.Immutable<{dismissed: ReadonlyArray; failed: ReadonlyArray}> + +export const takeSuppressSnapshot = (conversationIDKey: T.Chat.ConversationIDKey): SuppressSnapshot => { const {dismissed, failed} = useUnfurlPreviewState.getState() - return [ - ...new Set([...(dismissed.get(conversationIDKey) ?? []), ...(failed.get(conversationIDKey) ?? [])]), - ] + return { + dismissed: [...(dismissed.get(conversationIDKey) ?? [])], + failed: [...(failed.get(conversationIDKey) ?? [])], + } } +export const suppressedURLsOf = (snapshot: SuppressSnapshot) => [ + ...new Set([...snapshot.dismissed, ...snapshot.failed]), +] + +// what the user dismissed plus what could not be previewed: the send suppresses both, so +// the message unfurls exactly the cards the composer offered +export const getSuppressedURLs = (conversationIDKey: T.Chat.ConversationIDKey) => + suppressedURLsOf(takeSuppressSnapshot(conversationIDKey)) + // dropped once the send they belong to lands; a targeted remove rather than a // whole-conversation clear so a dismissal made while that send was in flight survives export const removeSuppressedURLs = ( @@ -82,13 +95,15 @@ export const removeSuppressedURLs = ( useUnfurlPreviewState.getState().dispatch.remove(conversationIDKey, urls) } -// put back the snapshot a send took when that send never posted, so the composer the -// user gets back still has those urls dismissed +// put back what a send took when that send never posted, so the composer the user gets +// back still has those urls dismissed. only the dismissals: the failures come back on +// their own from the next fetch, and recording one as a dismissal would keep the url +// suppressed even after it starts scraping again export const restoreSuppressedURLs = ( conversationIDKey: T.Chat.ConversationIDKey, - urls: ReadonlyArray + snapshot: SuppressSnapshot ) => { - useUnfurlPreviewState.getState().dispatch.dismiss(conversationIDKey, urls) + useUnfurlPreviewState.getState().dispatch.dismiss(conversationIDKey, snapshot.dismissed) } const debounceMS = 500 @@ -127,6 +142,16 @@ export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, t const sawTextRef = React.useRef(false) const hasLink = text.includes('http') + // the request guard lives in a ref, so it only ever discriminates against this mount's + // own older fetches. switching conversations and back remounts the hook, and a fetch the + // dead mount left in flight would still match its own id and write over what the new + // mount has since fetched. -1 matches no id, so unmounting retires the whole mount + React.useEffect(() => { + return () => { + requestIDRef.current = -1 + } + }, []) + const onFetched = React.useCallback( (fetchedConversationIDKey: T.Chat.ConversationIDKey, infos: ReadonlyArray) => { setFetched(infos) From 77322a59210ae5b244129c89ef4168f5d6085f3c Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Tue, 25 Aug 2026 21:46:31 -0400 Subject: [PATCH 16/17] fix(chat): match the preview's rules to the ones the send actually uses A second review round found the escape hatch from the last commit was cut on the wrong line, and turned up four older mismatches between what the composer shows and what the message posts. The skip for urls that get no card asked the auto-whitelist, which is an exact domain match, while the success path asks ClassifyDomain, which also calls gph.is and any giphy TLD a giphy. A giphy short link whose scrape blipped was therefore suppressed, losing the unfurl the send's own retries would have landed. Both paths now read the same rule, named `carded` for the half of it that is knowable before a scrape. Request ids were per mount and the retirement rewound the counter, so an id could be handed out twice: freezing a screen tears effects down and rebuilds them on the same ref, and a fetch the old one left running would match the new one's id. Ids are now module-wide and never reused. The send's snapshot owned more than it should: it restored dismissals it never took (any send with no snapshot, a coinflip resend, picked up the composer's), and dropped dismissals it never took either, including one the user made while that send was in flight. It carries only what it snapshotted now. The preview no longer renders while editing at all. An edit posts as MessageType_EDIT, which the unfurler does not extract urls from, so the card promised an unfurl the edit could never produce; disabling its X was not enough, and `canDismiss` goes with it. A card also survived the user typing on past its url, since the old url is a prefix of the new one and the check was a substring test. Its X would have suppressed a link the message no longer contained. The composer previews on every debounced edit, and edits of one url are all different singleflight keys, so nothing collapsed them: the detached scrapes are now bounded by a slot limit, a failure is remembered for 30s so a dead link is not re-fetched per keystroke, and Prefetch shares the same singleflight instead of duplicating the scrape the composer is already running. Tests: carded and previewable pinned directly, the failure cache, the keep-list keepOnly honours, the typed-past card, punctuation after a url, the dedup in the suppress list, a snapshot-less send, a dismissal made mid-send, the snapshot preceding the composer clear, and no preview while editing. Each was mutation-checked, including the four that survived the first attempt. --- go/chat/unfurl/cache.go | 21 +-- go/chat/unfurl/unfurler.go | 96 +++++++++---- go/chat/unfurl/unfurler_test.go | 63 +++++++++ .../input-area/input-state.test.tsx | 131 +++++++++++++++++- .../conversation/input-area/normal/index.tsx | 19 +-- .../input-area/unfurl-preview.test.tsx | 28 ++-- .../input-area/unfurl-preview.tsx | 19 +-- shared/chat/conversation/send-actions.tsx | 21 ++- .../unfurl-preview-state.test.tsx | 64 ++++++++- .../conversation/unfurl-preview-state.tsx | 77 +++++----- 10 files changed, 412 insertions(+), 127 deletions(-) diff --git a/go/chat/unfurl/cache.go b/go/chat/unfurl/cache.go index e45cb8d990fc..b474e8a3b2a0 100644 --- a/go/chat/unfurl/cache.go +++ b/go/chat/unfurl/cache.go @@ -21,18 +21,24 @@ type cacheItem struct { type unfurlCache struct { sync.Mutex - cache *lru.Cache - clock clockwork.Clock + cache *lru.Cache + clock clockwork.Clock + lifetime time.Duration } func newUnfurlCache() *unfurlCache { + return newUnfurlCacheWithLifetime(defaultCacheLifetime) +} + +func newUnfurlCacheWithLifetime(lifetime time.Duration) *unfurlCache { cache, err := lru.New(defaultCacheSize) if err != nil { panic(err) } return &unfurlCache{ - cache: cache, - clock: clockwork.NewRealClock(), + cache: cache, + clock: clockwork.NewRealClock(), + lifetime: lifetime, } } @@ -40,9 +46,8 @@ func (c *unfurlCache) setClock(clock clockwork.Clock) { c.clock = clock } -// get determines if the item is in the cache and newer than 10 -// minutes. We don't want to cache this value indefinitely in case the page -// content changes. +// get determines if the item is in the cache and newer than the cache's lifetime. We +// don't want to cache this value indefinitely in case the page content changes. func (c *unfurlCache) get(key string) (res cacheItem, ok bool) { c.Lock() defer c.Unlock() @@ -55,7 +60,7 @@ func (c *unfurlCache) get(key string) (res cacheItem, ok bool) { if !ok { return res, false } - valid := c.clock.Now().Sub(cacheItem.ctime.Time()) <= defaultCacheLifetime + valid := c.clock.Now().Sub(cacheItem.ctime.Time()) <= c.lifetime if !valid { c.cache.Remove(key) } diff --git a/go/chat/unfurl/unfurler.go b/go/chat/unfurl/unfurler.go index 0ce1ea8a3e35..a13cf6595a57 100644 --- a/go/chat/unfurl/unfurler.go +++ b/go/chat/unfurl/unfurler.go @@ -7,6 +7,7 @@ import ( "net" "net/url" "sync" + "time" "github.com/golang/groupcache/singleflight" "github.com/keybase/client/go/chat/attachments" @@ -48,6 +49,11 @@ type UnfurlMessageSender interface { msg chat1.MessagePlaintext, clientPrev chat1.MessageID, outboxID chat1.OutboxID) (chat1.OutboxID, error) } +const ( + maxConcurrentPreviewScrapes = 4 + previewFailureLifetime = 30 * time.Second +) + type Unfurler struct { sync.Mutex prefetchLock sync.Mutex @@ -60,11 +66,18 @@ type Unfurler struct { // called again while an earlier call is still in flight, which the composer does // whenever the user edits a link before its fetch comes back previewGroup singleflight.Group - extractor *Extractor - scraper *Scraper - packager *Packager - settings *Settings - sender UnfurlMessageSender + // the composer scrapes on every debounced edit, and each edit of a url is a different + // key, so the singleflight above collapses nothing across them. these bound what an + // editing session can leave running: a slot limit on the detached scrapes, and a short + // memory of the urls that just failed so a dead link is not re-fetched per keystroke. + // short, because a url that starts working has to become a card again + previewSem chan struct{} + previewFailures *unfurlCache + extractor *Extractor + scraper *Scraper + packager *Packager + settings *Settings + sender UnfurlMessageSender // testing unfurlCh chan *chat1.Unfurl @@ -81,14 +94,16 @@ func NewUnfurler(g *globals.Context, store attachments.Store, s3signer s3.Signer packager := NewPackager(g, store, s3signer, ri) settings := NewSettings(g, storage) return &Unfurler{ - Contextified: globals.NewContextified(g), - DebugLabeler: utils.NewDebugLabeler(g.ExternalG(), "Unfurler", false), - unfurlMap: make(map[string]bool), - extractor: extractor, - scraper: scraper, - packager: packager, - settings: settings, - sender: sender, + previewSem: make(chan struct{}, maxConcurrentPreviewScrapes), + previewFailures: newUnfurlCacheWithLifetime(previewFailureLifetime), + Contextified: globals.NewContextified(g), + DebugLabeler: utils.NewDebugLabeler(g.ExternalG(), "Unfurler", false), + unfurlMap: make(map[string]bool), + extractor: extractor, + scraper: scraper, + packager: packager, + settings: settings, + sender: sender, } } @@ -388,7 +403,13 @@ func (u *Unfurler) Prefetch(ctx context.Context, uid gregor1.UID, convID chat1.C } prevUnfurled[hit.URL] = true // only one action per unique URL if hit.Typ == ExtractorHitUnfurl { - if _, err := u.scrapeAndPackage(ctx, uid, convID, hit.URL); err != nil { + // through the same singleflight as the preview: UpdateUnsentText prefetches the + // text the composer is previewing, so the two would otherwise scrape it twice + key := previewScrapeKey(uid, convID, hit.URL) + _, err := u.previewGroup.Do(key, func() (any, error) { + return u.scrapeAndPackage(ctx, uid, convID, hit.URL) + }) + if err != nil { u.Debug(ctx, "Prefetch: unable to scrapeAndPackge: %s", err) } else { numPrefetched++ @@ -398,10 +419,6 @@ func (u *Unfurler) Prefetch(ctx context.Context, uid gregor1.UID, convID chat1.C return numPrefetched } -// PreviewURLs scrapes and packages the whitelisted URLs in text and returns -// display-ready unfurls, so a client can show a preview before sending. Only generic -// unfurls carry a display; a url that cannot be previewed at all comes back with a nil -// unfurl so the client can suppress it on send. // previewable reports whether an unfurl can be shown in the pre-send preview. only generic // unfurls render there, and a map unfurl is a generic unfurl with MapInfo set, which the // message view itself refuses to render. the frontend filters on the same rule, so keep the @@ -414,6 +431,11 @@ func previewable(unfurl chat1.Unfurl) bool { return unfurl.Generic().MapInfo == nil } +// %x, not %s: uid and convID are raw bytes and can contain the separator +func previewScrapeKey(uid gregor1.UID, convID chat1.ConversationID, url string) string { + return fmt.Sprintf("%x:%x:%s", uid, convID, url) +} + // previewScrape scrapes and packages one url, collapsing concurrent calls for the same // url into a single scrape. // @@ -425,8 +447,12 @@ func previewable(unfurl chat1.Unfurl) bool { func (u *Unfurler) previewScrape(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, url string, ) (chat1.Unfurl, error) { - // %x, not %s: uid and convID are raw bytes and can contain the separator - key := fmt.Sprintf("%x:%x:%s", uid, convID, url) + key := previewScrapeKey(uid, convID, url) + if item, ok := u.previewFailures.get(key); ok { + if err, ok := item.data.(string); ok { + return chat1.Unfurl{}, errors.New(err) + } + } // keeps the caller's log tags and identify behaviour and drops only the cancellation. // the scraper bounds the work with its own request timeout scrapeCtx := context.WithoutCancel(ctx) @@ -436,10 +462,13 @@ func (u *Unfurler) previewScrape(ctx context.Context, uid gregor1.UID, convID ch } ch := make(chan scrapeRes, 1) go func() { + u.previewSem <- struct{}{} + defer func() { <-u.previewSem }() scraped, err := u.previewGroup.Do(key, func() (any, error) { return u.scrapeAndPackage(scrapeCtx, uid, convID, url) }) if err != nil { + u.previewFailures.put(key, err.Error()) ch <- scrapeRes{err: err} return } @@ -458,6 +487,27 @@ func (u *Unfurler) previewScrape(ctx context.Context, uid gregor1.UID, convID ch } } +// carded reports whether a url could get a preview card at all, from the domain alone. +// previewable answers the same question from a scraped unfurl, but a failed scrape has no +// unfurl to ask about, and reporting one of these as un-previewable would suppress an +// unfurl the send's own retries would have landed. keep the two rules in step: this is the +// part of previewable that is knowable before the scrape. +func carded(url string) bool { + domain, err := GetDomain(url) + if err != nil { + return true + } + return ClassifyDomain(domain) == chat1.UnfurlType_GENERIC +} + +// PreviewURLs scrapes and packages the whitelisted URLs in text and returns display-ready +// unfurls, so a client can show a preview before sending. Only generic unfurls carry a +// display; a url that cannot be previewed at all comes back with a nil unfurl so the client +// can suppress it on send. +// +// URLs whose domain is not whitelisted are left out entirely: the send prompts for those, +// and the prompt is itself the chance to decline, so there is nothing for the composer to +// offer and nothing to suppress. func (u *Unfurler) PreviewURLs(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID, text string, ) (res []chat1.UnfurlPreviewInfo) { @@ -479,10 +529,7 @@ func (u *Unfurler) PreviewURLs(ctx context.Context, uid gregor1.UID, convID chat if ctx.Err() != nil { return nil } - if u.extractor.isAutoWhitelistFromHit(ctx, hit.URL) { - // a giphy or a map, same as the previewable check below: it was never going - // to get a card, so one transient scrape failure here must not suppress an - // unfurl the send's own retries would have landed + if !carded(hit.URL) { continue } // reported rather than dropped: UnfurlAndSend would still queue this url and @@ -500,7 +547,6 @@ func (u *Unfurler) PreviewURLs(ctx context.Context, uid gregor1.UID, convID chat disp, err := display.DisplayUnfurl(ctx, u.G().AttachmentURLSrv, convID, unfurl) if err != nil { u.Debug(ctx, "PreviewURLs: failed to display: %s", err) - res = append(res, chat1.UnfurlPreviewInfo{Url: hit.URL}) continue } res = append(res, chat1.UnfurlPreviewInfo{Url: hit.URL, Unfurl: &disp}) diff --git a/go/chat/unfurl/unfurler_test.go b/go/chat/unfurl/unfurler_test.go index 14e52c4f7289..783ec0a419c3 100644 --- a/go/chat/unfurl/unfurler_test.go +++ b/go/chat/unfurl/unfurler_test.go @@ -261,6 +261,44 @@ func TestUnfurlerPreviewURLsScrapeFailure(t *testing.T) { require.Nil(t, res[0].Unfurl) } +// the composer previews on every debounced edit, so a url that cannot be scraped would be +// re-fetched for as long as it sits in the text. the failure is remembered briefly instead +func TestUnfurlerPreviewURLsFailureNotRescraped(t *testing.T) { + tc := externalstest.SetupTest(t, "unfurler", 0) + defer tc.Cleanup() + g := globals.NewContext(tc.G, &globals.ChatContext{}) + + store := attachments.NewStoreTesting(g, nil) + s3signer := &ptsigner{} + g.ActivityNotifier = makeDummyActivityNotifier() + g.MessageDeliverer = dummyDeliverer{} + g.AttachmentURLSrv = types.DummyAttachmentHTTPSrv{} + sender := makeDummySender() + ri := func() chat1.RemoteInterface { return paramsRemote{} } + storage := newMemConversationBackedStorage() + unfurler := NewUnfurler(g, store, s3signer, storage, sender, ri) + + uid := gregor1.UID([]byte{0, 1}) + convID := chat1.ConversationID([]byte{0, 1, 2}) + var hits int32 + srv := newDummyHTTPSrv(t, func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(&hits, 1) + w.WriteHeader(http.StatusUnauthorized) + }) + addr := srv.Start() + defer srv.Stop() + + url := fmt.Sprintf("http://%s/?name=%s", addr, "wsj0.html") + require.NoError(t, unfurler.WhitelistAdd(context.TODO(), uid, "127.0.0.1")) + + for i := 0; i < 3; i++ { + res := unfurler.PreviewURLs(context.TODO(), uid, convID, "check this out "+url) + require.Len(t, res, 1) + require.Nil(t, res[0].Unfurl, "a cached failure still reports the url for suppression") + } + require.Equal(t, int32(1), atomic.LoadInt32(&hits)) +} + func makeTextMsgWithMsgID(msgBody string, outboxID chat1.OutboxID, msgID chat1.MessageID) chat1.MessageUnboxed { return chat1.NewMessageUnboxedWithValid(chat1.MessageUnboxedValid{ ClientHeader: chat1.MessageClientHeaderVerified{ @@ -476,6 +514,31 @@ func TestUnfurlerPreviewURLsCallerCancel(t *testing.T) { require.Equal(t, int64(1), atomic.LoadInt64(&scrapes), "the two callers did not share one scrape") } +// only generic unfurls get a card, and a map is a generic unfurl the message view itself +// refuses to render. the frontend filters on the same rule +func TestUnfurlerPreviewable(t *testing.T) { + generic := chat1.UnfurlGeneric{Title: "t", Url: "u", SiteName: "s"} + require.True(t, previewable(chat1.NewUnfurlWithGeneric(generic))) + + withMap := generic + withMap.MapInfo = &chat1.UnfurlGenericMapInfo{} + require.False(t, previewable(chat1.NewUnfurlWithGeneric(withMap))) + + require.False(t, previewable(chat1.NewUnfurlWithGiphy(chat1.UnfurlGiphy{}))) + require.False(t, previewable(chat1.NewUnfurlWithYoutube(chat1.UnfurlYoutube{}))) +} + +// the rule for what gets no card is the one previewable uses on a scraped unfurl, read +// from the domain: a giphy short link classifies as giphy without being on the +// auto-whitelist, and suppressing it would lose an unfurl the send would have landed +func TestUnfurlerCarded(t *testing.T) { + require.True(t, carded("https://example.com/a")) + require.True(t, carded("not a url at all")) + require.False(t, carded("https://giphy.com/gifs/abc")) + require.False(t, carded("https://gph.is/2X9abc")) + require.False(t, carded(fmt.Sprintf("https://%s/?lat=1&lon=2&acc=3&done=true", types.MapsDomain))) +} + // a giphy or a maps url gets no card either way, so a scrape failure on one must not be // reported: suppressing it would lose an unfurl the send's own retries would have landed func TestUnfurlerPreviewURLsAutoWhitelistFailureNotSuppressed(t *testing.T) { diff --git a/shared/chat/conversation/input-area/input-state.test.tsx b/shared/chat/conversation/input-area/input-state.test.tsx index b03f94cfec77..663171990ac7 100644 --- a/shared/chat/conversation/input-area/input-state.test.tsx +++ b/shared/chat/conversation/input-area/input-state.test.tsx @@ -10,11 +10,15 @@ import {resetAllStores} from '@/util/zustand' import {useCurrentUserState} from '@/stores/current-user' import Input from './normal' import type {PlatformInputProps} from './normal/input.shared' -import {ConversationInputProvider, useConversationInput} from './input-state' +import {ConversationInputProvider, useConversationInput, type ConversationInputState} from './input-state' import {ConversationThreadProvider, useConversationThreadActions} from '../thread-context' -import {getSuppressedURLs, useUnfurlPreviewState} from '../unfurl-preview-state' +import {suppressedURLsOf, takeSuppressSnapshot, useUnfurlPreviewState} from '../unfurl-preview-state' + +const getSuppressedURLs = (c: T.Chat.ConversationIDKey) => suppressedURLsOf(takeSuppressSnapshot(c)) let mockRouteParams: Record = {} +// jest.mock factories may only close over mock-prefixed names +let mockOnClear: (() => void) | undefined let mockPlatformInputProps: PlatformInputProps | undefined // stand in for the real composer input: it only has to hand back a ref whose clear() // fires onChangeText('') the way the desktop input does, which is what races the send @@ -24,7 +28,13 @@ jest.mock('./normal/input', () => ({ mockPlatformInputProps = p p.setInputRef({ blur: () => {}, - clear: () => mockPlatformInputProps?.onChangeText(''), + clear: () => { + // the real hook drops suppressions from an effect after the text goes empty; a test + // can ask for that to happen synchronously inside clear() instead, which is the + // ordering onSubmit's snapshot has to survive + mockOnClear?.() + mockPlatformInputProps?.onChangeText('') + }, focus: () => {}, getSelection: () => undefined, isFocused: () => false, @@ -112,6 +122,26 @@ function renderComposer(id = convID) { return render(, {wrapper: wrapperFor(id)}) } +type InputHandles = { + input: ReturnType> + thread: ReturnType +} + +const InputProbe = (p: {onRender: (h: InputHandles) => void}) => { + p.onRender({input: useConversationInput(s => s), thread: useConversationThreadActions()}) + return null +} + +function renderComposerWithProbe(onRender: (h: InputHandles) => void, id = convID) { + return render( + <> + + + , + {wrapper: wrapperFor(id)} + ) +} + const renderInputWithThreadActions = (id = convID) => renderHook( () => ({ @@ -139,6 +169,7 @@ beforeEach(() => { afterEach(() => { mockPlatformInputProps = undefined + mockOnClear = undefined cleanup() jest.restoreAllMocks() resetAllStores() @@ -429,7 +460,7 @@ test('sendComposerText sends dismissed unfurl urls as unfurlSuppress', async () const {result} = renderInput() act(() => { - result.current.dispatch.sendComposerText('hi http://a.com') + result.current.dispatch.sendComposerText('hi http://a.com', {dismissed: ['http://a.com'], failed: []}) }) await flushPromises() @@ -437,6 +468,23 @@ test('sendComposerText sends dismissed unfurl urls as unfurlSuppress', async () expect(getSuppressedURLs(convID)).toEqual([]) }) +// a send that carries no snapshot is not the composer sending its own text -- a coinflip +// resend goes through the same action -- so it must not pick up the composer's dismissals, +// nor clear them when it lands +test('a send with no snapshot leaves the composer dismissals alone', async () => { + const getLastPost = mockPostText() + useUnfurlPreviewState.getState().dispatch.dismiss(convID, ['http://a.com']) + const {result} = renderInput() + + act(() => { + result.current.dispatch.sendComposerText('/flip 2') + }) + await flushPromises() + + expect(getLastPost()?.params.unfurlSuppress).toEqual([]) + expect(getSuppressedURLs(convID)).toEqual(['http://a.com']) +}) + test('onSubmit sends dismissed unfurl urls even though clearing the composer drops them', async () => { jest.useFakeTimers() try { @@ -486,6 +534,81 @@ test('onSubmit sends dismissed unfurl urls even though clearing the composer dro } }) +test('onSubmit snapshots the dismissals before the composer clears them', async () => { + jest.useFakeTimers() + try { + mockOnClear = () => useUnfurlPreviewState.getState().dispatch.keepOnly(convID, []) + const getLastPost = mockPostText() + jest.spyOn(T.RPCChat, 'localUpdateTypingRpcPromise').mockResolvedValue(undefined) + jest.spyOn(T.RPCChat, 'localUpdateUnsentTextRpcPromise').mockResolvedValue(undefined) + jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise').mockResolvedValue([]) + renderComposer() + useUnfurlPreviewState.getState().dispatch.dismiss(convID, ['http://a.com']) + + act(() => { + mockPlatformInputProps?.onSubmit('look at http://a.com') + }) + // the clear has already emptied the store by now: only a snapshot taken ahead of it + // still has the dismissal to send + expect(getSuppressedURLs(convID)).toEqual([]) + + await act(async () => { + jest.advanceTimersByTime(1) + await flushPromises() + }) + + expect(getLastPost()?.params.unfurlSuppress).toEqual(['http://a.com']) + } finally { + jest.useRealTimers() + } +}) + +// the snapshot owns only what it took: a dismissal made while the send was in flight +// belongs to the next message, and clearing it would unfurl a card the user just declined +test('a landed send leaves a dismissal made while it was in flight alone', async () => { + mockPostText() + const {result} = renderInput() + + act(() => { + result.current.dispatch.sendComposerText('hi http://wsj.com', {dismissed: [], failed: ['http://wsj.com']}) + }) + useUnfurlPreviewState.getState().dispatch.dismiss(convID, ['http://wsj.com']) + await flushPromises() + + expect(getSuppressedURLs(convID)).toEqual(['http://wsj.com']) +}) + +// an edit posts as MessageType_EDIT, which the unfurler does not extract urls from, so +// there is nothing to preview and no scrape to pay for +test('no preview is fetched while editing', async () => { + const spy = jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise').mockResolvedValue([]) + jest.spyOn(T.RPCChat, 'localUpdateTypingRpcPromise').mockResolvedValue(undefined) + jest.spyOn(T.RPCChat, 'localUpdateUnsentTextRpcPromise').mockResolvedValue(undefined) + jest.useFakeTimers() + try { + // the composer and the handles that drive it have to share one provider, or the editing + // state never reaches the composer under test + let handles: InputHandles | undefined + renderComposerWithProbe(h => (handles = h)) + act(() => { + handles?.thread.addMessages([makeTextMessage({text: 'look at http://a.com'})], {markAsRead: false}) + }) + act(() => { + mockPlatformInputProps?.onChangeText('look at http://a.com') + }) + act(() => { + handles?.input.dispatch.setEditing('last') + }) + await act(async () => { + jest.advanceTimersByTime(1000) + await flushPromises() + }) + expect(spy).not.toHaveBeenCalled() + } finally { + jest.useRealTimers() + } +}) + test('a canceled stellar send restores the dismissed unfurl urls for the resend', async () => { // the composer clears its dismissals before the send resolves, so a cancel has to put // the snapshot back or the restored text re-unfurls what the user dismissed diff --git a/shared/chat/conversation/input-area/normal/index.tsx b/shared/chat/conversation/input-area/normal/index.tsx index 6dce654e0c47..58736824dd4c 100644 --- a/shared/chat/conversation/input-area/normal/index.tsx +++ b/shared/chat/conversation/input-area/normal/index.tsx @@ -209,15 +209,8 @@ const ConnectedPlatformInput = function ConnectedPlatformInput() { // A timeout rather than requestAnimationFrame: this callback owns the only copy of the text, and // frames stop in a hidden or backgrounded window, which would drop the message with the composer // already emptied. - // Snapshot the dismissed unfurls before clearing: the clear runs onChangeText('') - // synchronously, and the preview hook drops every dismissal for empty text, which - // would beat the deferred send to the store and unfurl a card the user dismissed. - // - // Sending before the previews land (paste and hit enter: 200ms draft throttle + 500ms - // debounce + the scrape itself) suppresses nothing, so the message unfurls the url the - // way it always has. That is deliberate. The composer only promises to show what will - // unfurl once its previews have settled; suppressing urls it has not heard about yet - // would mean a link sent quickly never unfurls at all, which is how most links go out. + // Before the clear, which runs onChangeText('') synchronously and drops every dismissal. + // Urls whose preview has not landed yet are not in here and so are not suppressed. const unfurlSuppress = takeSuppressSnapshot(conversationIDKey) injectText('', true) setTimeout(() => { @@ -344,10 +337,10 @@ const ConnectedPlatformInput = function ConnectedPlatformInput() { /> ) - // no dismiss while editing: postEditNonblock carries no unfurlSuppress, so the X would - // hide the card and change nothing about what the edit posts - const preview = ( - + // nothing while editing: an edit posts as MessageType_EDIT, which the unfurler does not + // extract urls from at all, so a card would promise an unfurl the edit cannot produce + const preview = isEditing ? null : ( + ) if (isMobile) { diff --git a/shared/chat/conversation/input-area/unfurl-preview.test.tsx b/shared/chat/conversation/input-area/unfurl-preview.test.tsx index d431500afc69..b52d0b7712ae 100644 --- a/shared/chat/conversation/input-area/unfurl-preview.test.tsx +++ b/shared/chat/conversation/input-area/unfurl-preview.test.tsx @@ -47,32 +47,32 @@ describe('UnfurlPreview', () => { it('renders nothing when every preview is non-generic', () => { mockPreviews = [nonGenericInfo] - const {container} = render() + const {container} = render() expect(container.firstChild).toBeNull() }) it('renders a card for a generic preview', () => { mockPreviews = [genericInfo] - const {container} = render() + const {container} = render() expect(container.firstChild).not.toBeNull() }) it('renders nothing for a map unfurl', () => { mockPreviews = [mapInfo] - const {container} = render() + const {container} = render() expect(container.firstChild).toBeNull() }) it('shows no pager for a single preview', () => { mockPreviews = [genericInfo] - const {queryByText} = render() + const {queryByText} = render() expect(queryByText('1/1')).toBeNull() }) it('pages between previews and disables the arrows at each end', () => { mockPreviews = [genericInfo, genericInfo2] const {getByText, container} = render( - + ) expect(getByText('1/2')).toBeTruthy() // the card shown is the first one @@ -97,21 +97,21 @@ describe('UnfurlPreview', () => { it('re-clamps the index when the shown card is dismissed away', () => { mockPreviews = [genericInfo, genericInfo2] const {getByText, container, rerender} = render( - + ) fireEvent.click(container.querySelector('.icon-gen-iconfont-arrow-right') as Element) expect(getByText('2/2')).toBeTruthy() // the second preview goes away; the index must fall back rather than blank the panel mockPreviews = [genericInfo] - rerender() + rerender() expect(container.textContent).toContain('Alpha') }) it('dismisses the shown card by its url when the close icon is clicked', () => { mockPreviews = [genericInfo, genericInfo2] const {container} = render( - + ) fireEvent.click(container.querySelector('.icon-gen-iconfont-close') as Element) expect(mockDismiss).toHaveBeenCalledWith('http://a.com') @@ -136,21 +136,11 @@ describe('UnfurlPreview', () => { } const {container} = render( - + ) fireEvent.click(container.querySelector('.icon-gen-iconfont-close') as Element) expect(focusInput).toHaveBeenCalled() }) - it('offers no dismiss while editing, since an edit cannot carry suppression', () => { - mockPreviews = [genericInfo] - const {container} = render( - - ) - // the card still shows what will unfurl; it just does not offer a control that would - // hide it and change nothing about the posted edit - expect(container.textContent).toContain('Alpha') - expect(container.querySelector('.icon-gen-iconfont-close')).toBeNull() - }) }) diff --git a/shared/chat/conversation/input-area/unfurl-preview.tsx b/shared/chat/conversation/input-area/unfurl-preview.tsx index 955fe823d999..49e18dc36466 100644 --- a/shared/chat/conversation/input-area/unfurl-preview.tsx +++ b/shared/chat/conversation/input-area/unfurl-preview.tsx @@ -6,15 +6,12 @@ import {useUnfurlPreviews} from '@/chat/conversation/unfurl-preview-state' import {ThreadRefsContext} from '@/chat/conversation/normal/context' type Props = { - // false while editing a message: the edit rpc cannot carry suppression, so offering a - // dismiss there would be a control that does nothing - canDismiss: boolean conversationIDKey: T.Chat.ConversationIDKey text: string } const UnfurlPreview = (p: Props) => { - const {canDismiss, conversationIDKey, text} = p + const {conversationIDKey, text} = p const styles = useStyles() const theme = Kb.Styles.useTheme() const {dismiss, previews} = useUnfurlPreviews(conversationIDKey, text) @@ -69,15 +66,11 @@ const UnfurlPreview = (p: Props) => { description={generic.description ?? undefined} favicon={generic.favicon ?? undefined} media={generic.media ?? undefined} - onClose={ - canDismiss - ? () => { - dismiss(preview.url) - // the X takes focus on the way out, and the composer is where the user was - focusInput() - } - : undefined - } + onClose={() => { + dismiss(preview.url) + // the X takes focus on the way out, and the composer is where the user was + focusInput() + }} publishTime={generic.publishTime ?? undefined} siteName={generic.siteName} title={generic.title} diff --git a/shared/chat/conversation/send-actions.tsx b/shared/chat/conversation/send-actions.tsx index 6f98b7dadbf0..372232587a80 100644 --- a/shared/chat/conversation/send-actions.tsx +++ b/shared/chat/conversation/send-actions.tsx @@ -4,13 +4,7 @@ import logger from '@/logger' import {RPCError} from '@/util/errors' import {ignorePromise} from '@/constants/utils' import {getClientPrevFromThread} from './attachment-actions' -import { - removeSuppressedURLs, - restoreSuppressedURLs, - suppressedURLsOf, - takeSuppressSnapshot, - type SuppressSnapshot, -} from './unfurl-preview-state' +import {removeDismissals, restoreDismissals, suppressedURLsOf, type SuppressSnapshot} from './unfurl-preview-state' import {useInboxMetadataState} from '../inbox/metadata-store' import { useConversationThreadActions, @@ -157,9 +151,10 @@ export const useConversationSendActions = () => { } const replyToOrdinal = context?.replyToOrdinal const replyTo = threadStore.getState().messageMap.get(replyToOrdinal ?? T.Chat.numberToOrdinal(0))?.id - // the caller passes a snapshot taken before it cleared the composer: clearing runs - // synchronously and the preview hook drops every dismissal once the text is empty - const snapshot = context?.unfurlSuppress ?? takeSuppressSnapshot(conversationIDKey) + // only what the caller snapshotted. a send that carries no snapshot is not the composer + // sending its own text (a coinflip resend, say), and the composer's dismissals have + // nothing to do with it + const snapshot = context?.unfurlSuppress ?? {dismissed: [], failed: []} const unfurlSuppress = suppressedURLsOf(snapshot) const onRestoreText = context?.onRestoreText sendTextMessageStoreless({ @@ -168,12 +163,14 @@ export const useConversationSendActions = () => { ephemeralLifetime: threadStore.getState().explodingMode, onRestoreText: onRestoreText ? (restored: string) => { - restoreSuppressedURLs(conversationIDKey, snapshot) + restoreDismissals(conversationIDKey, snapshot.dismissed) onRestoreText(restored) } : undefined, onSent: () => { - removeSuppressedURLs(conversationIDKey, unfurlSuppress) + // the dismissals only: a failure is never in `dismissed`, and the url may have been + // dismissed afresh while this send was in flight + removeDismissals(conversationIDKey, snapshot.dismissed) }, replyTo, text, diff --git a/shared/chat/conversation/unfurl-preview-state.test.tsx b/shared/chat/conversation/unfurl-preview-state.test.tsx index 05fb390f38ce..7047a1d6611f 100644 --- a/shared/chat/conversation/unfurl-preview-state.test.tsx +++ b/shared/chat/conversation/unfurl-preview-state.test.tsx @@ -2,7 +2,9 @@ /// import * as T from '@/constants/types' import {act, render, waitFor} from '@testing-library/react' -import {useUnfurlPreviews, getSuppressedURLs, useUnfurlPreviewState} from './unfurl-preview-state' +import {useUnfurlPreviews, suppressedURLsOf, takeSuppressSnapshot, useUnfurlPreviewState} from './unfurl-preview-state' + +const getSuppressedURLs = (c: T.Chat.ConversationIDKey) => suppressedURLsOf(takeSuppressSnapshot(c)) // stringToConversationIDKey('conv1') is not valid hex and would throw inside // T.Chat.keyToConversationID (used to build the RPC's convID param), so build @@ -124,6 +126,66 @@ describe('unfurl previews', () => { expect(getSuppressedURLs(convID)).toEqual([]) }) + it('keeps a dismissal that the next fetch still returns', async () => { + // keepOnly prunes what the fetch no longer mentions; a url still in the result and + // still dismissed has to survive, or the card the user declined comes back + const spy = jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise') + spy.mockResolvedValue([info('http://a.com')]) + let last: ReturnType | undefined + const {rerender} = render( (last = r)} />) + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(last?.previews.length).toBe(1)) + act(() => last?.dismiss('http://a.com')) + rerender( (last = r)} />) + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(spy).toHaveBeenCalledTimes(2)) + expect(getSuppressedURLs(convID)).toEqual(['http://a.com']) + expect(last?.previews.length).toBe(0) + }) + + it('drops the card for a url the user has typed on past', async () => { + // the old url is a prefix of the new one, so a substring test would keep the stale card + // showing and let its X suppress a link the message does not contain + const spy = jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise') + spy.mockResolvedValueOnce([info('http://a.com')]) + spy.mockResolvedValueOnce([info('http://a.com/foo')]) + let last: ReturnType | undefined + const {rerender} = render( (last = r)} />) + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(last?.previews.length).toBe(1)) + rerender( (last = r)} />) + await waitFor(() => expect(last?.previews.length).toBe(0)) + // and the card comes back once the fetch for the longer url lands + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(last?.previews.map(p => p.url)).toEqual(['http://a.com/foo'])) + }) + + it('keeps showing a card when the url is followed by punctuation', async () => { + jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise').mockResolvedValue([info('http://a.com')]) + let last: ReturnType | undefined + const {rerender} = render( (last = r)} />) + act(() => { + jest.advanceTimersByTime(500) + }) + await waitFor(() => expect(last?.previews.length).toBe(1)) + rerender( (last = r)} />) + expect(last?.previews.length).toBe(1) + }) + + it('sends a url that was both dismissed and unpreviewable only once', () => { + useUnfurlPreviewState.getState().dispatch.dismiss(convID, ['http://a.com']) + useUnfurlPreviewState.getState().dispatch.setFailed(convID, ['http://a.com', 'http://wsj.com']) + expect(getSuppressedURLs(convID)).toEqual(['http://a.com', 'http://wsj.com']) + }) + it('replaces the failed set wholesale rather than accumulating', async () => { const spy = jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise') spy.mockResolvedValueOnce([failedInfo('http://a.com'), failedInfo('http://b.com')]) diff --git a/shared/chat/conversation/unfurl-preview-state.tsx b/shared/chat/conversation/unfurl-preview-state.tsx index 500df5722504..8cf431f390f9 100644 --- a/shared/chat/conversation/unfurl-preview-state.tsx +++ b/shared/chat/conversation/unfurl-preview-state.tsx @@ -9,7 +9,7 @@ type State = T.Immutable<{ // urls the service could not preview. they suppress on send like a dismissal does, but // are kept apart from one: the next fetch replaces this set wholesale, and a url that // starts scraping again must come back as a card, which it could not do if a failure - // had been recorded as something the user dismissed + // had been recorded as something the user dismissed, since nothing re-derives a dismissal failed: Map> dispatch: { dismiss: (conversationIDKey: T.Chat.ConversationIDKey, urls: ReadonlyArray) => void @@ -64,9 +64,8 @@ export const useUnfurlPreviewState = Z.createZustand('unfurl-preview', se }, })) -// the two suppression sources kept apart. a send needs them apart because it may have to -// put them back: restoring a scrape failure as a dismissal would bury the url for good, -// since a dismissal is never re-derived while a failure is, on the next fetch +// the two suppression sources kept apart, because a send that never posts has to put the +// dismissals back without the failures: see the `failed` field above export type SuppressSnapshot = T.Immutable<{dismissed: ReadonlyArray; failed: ReadonlyArray}> export const takeSuppressSnapshot = (conversationIDKey: T.Chat.ConversationIDKey): SuppressSnapshot => { @@ -77,37 +76,42 @@ export const takeSuppressSnapshot = (conversationIDKey: T.Chat.ConversationIDKey } } +// the send suppresses both, so the message unfurls exactly the cards the composer offered export const suppressedURLsOf = (snapshot: SuppressSnapshot) => [ ...new Set([...snapshot.dismissed, ...snapshot.failed]), ] -// what the user dismissed plus what could not be previewed: the send suppresses both, so -// the message unfurls exactly the cards the composer offered -export const getSuppressedURLs = (conversationIDKey: T.Chat.ConversationIDKey) => - suppressedURLsOf(takeSuppressSnapshot(conversationIDKey)) - // dropped once the send they belong to lands; a targeted remove rather than a // whole-conversation clear so a dismissal made while that send was in flight survives -export const removeSuppressedURLs = ( - conversationIDKey: T.Chat.ConversationIDKey, - urls: ReadonlyArray -) => { +export const removeDismissals = (conversationIDKey: T.Chat.ConversationIDKey, urls: ReadonlyArray) => { useUnfurlPreviewState.getState().dispatch.remove(conversationIDKey, urls) } // put back what a send took when that send never posted, so the composer the user gets -// back still has those urls dismissed. only the dismissals: the failures come back on -// their own from the next fetch, and recording one as a dismissal would keep the url -// suppressed even after it starts scraping again -export const restoreSuppressedURLs = ( - conversationIDKey: T.Chat.ConversationIDKey, - snapshot: SuppressSnapshot -) => { - useUnfurlPreviewState.getState().dispatch.dismiss(conversationIDKey, snapshot.dismissed) +// back still has those urls dismissed +export const restoreDismissals = (conversationIDKey: T.Chat.ConversationIDKey, urls: ReadonlyArray) => { + useUnfurlPreviewState.getState().dispatch.dismiss(conversationIDKey, urls) } +// a send during this window (paste a link, hit enter) suppresses nothing, so the message +// unfurls the url the way it always has: the composer only promises to show what will +// unfurl once its previews have landed. suppressing urls it has not heard about yet would +// mean a link sent quickly never unfurls at all, which is how most links go out const debounceMS = 500 +// what can follow a url and still end it. typing on past one makes the old url a prefix of +// the new one, and a plain substring test would keep the stale card alive and dismissable +// while the message carries a different link +const urlEnd = /[\s.,;:!?)\]}'"]/ + +const stillInText = (text: string, url: string) => { + for (let from = text.indexOf(url); from >= 0; from = text.indexOf(url, from + 1)) { + const after = text[from + url.length] + if (after === undefined || urlEnd.test(after)) return true + } + return false +} + // kept outside the hook body: try/catch inside a hook trips the react-compiler bailout check const fetchPreviews = async ( conversationIDKey: T.Chat.ConversationIDKey, @@ -130,10 +134,18 @@ const fetchPreviews = async ( } } +// module-wide, so an id is never handed out twice. a per-mount counter would start over, +// and a mount is torn down and rebuilt without a fresh ref whenever a screen freezes, which +// would let a fetch left running by the old one match the new one's id +let nextRequestID = 0 +// through a helper: the compiler bails out of a hook that updates a global in place +const takeRequestID = () => ++nextRequestID + export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, text: string) => { const [fetched, setFetched] = React.useState>([]) const dismissedSet = useUnfurlPreviewState(s => s.dismissed.get(conversationIDKey)) const {dismiss: dismissURL, keepOnly, setFailed} = useUnfurlPreviewState(s => s.dispatch) + // the id of the only fetch whose result this mount will still take const requestIDRef = React.useRef(0) // the input subtree remounts per conversation (key={conversationIDKey} on the provider), // so the first render of a conversation we return to always has empty text before the @@ -142,13 +154,14 @@ export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, t const sawTextRef = React.useRef(false) const hasLink = text.includes('http') - // the request guard lives in a ref, so it only ever discriminates against this mount's - // own older fetches. switching conversations and back remounts the hook, and a fetch the - // dead mount left in flight would still match its own id and write over what the new - // mount has since fetched. -1 matches no id, so unmounting retires the whole mount + // retires every fetch this mount left in flight, so one cannot write into the mount that + // replaces it. 0 is not a request id, and ids are never reused, so nothing can match again React.useEffect(() => { + // the alias is for the lint rule about reading a ref in cleanup: this ref holds a + // request id, not a node, and retiring it is the whole point of the cleanup + const requests = requestIDRef return () => { - requestIDRef.current = -1 + requests.current = 0 } }, []) @@ -168,8 +181,9 @@ export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, t ) React.useEffect(() => { - const id = ++requestIDRef.current - if (!text.includes('http')) { + const id = takeRequestID() + requestIDRef.current = id + if (!hasLink) { if (sawTextRef.current) { keepOnly(conversationIDKey, []) setFailed(conversationIDKey, []) @@ -184,7 +198,7 @@ export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, t return () => { clearTimeout(timeoutID) } - }, [conversationIDKey, keepOnly, setFailed, text, onFetched]) + }, [conversationIDKey, hasLink, keepOnly, setFailed, text, onFetched]) const dismiss = React.useCallback( (url: string) => { @@ -197,13 +211,12 @@ export const useUnfurlPreviews = (conversationIDKey: T.Chat.ConversationIDKey, t // replace these previews can fail or still be in flight, and showing a card for a url the // user has since deleted is worse than showing nothing: its X would suppress a link that // is not in the message, while the link that is about to send never gets offered one. - // an entry with no unfurl is a url the service could not preview. it is already - // suppressed for the send, so there is nothing to show and nothing to dismiss + // no unfurl means the service could not preview it: nothing to show, nothing to dismiss const visible = React.useMemo( () => hasLink ? fetched.flatMap(p => - p.unfurl && text.includes(p.url) && !dismissedSet?.has(p.url) + p.unfurl && stillInText(text, p.url) && !dismissedSet?.has(p.url) ? [{unfurl: p.unfurl, url: p.url}] : [] ) From 93991db856df3a5e8b19910112a00278e048a8bf Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Fri, 28 Aug 2026 16:09:56 -0400 Subject: [PATCH 17/17] test(chat): wrap a dismiss the mounted preview subscribes to in act This test renders the composer, so UnfurlPreview is mounted and re-renders off the store; dispatching outside act() tripped React's warning, which fail-on-console turns into a failure. --- shared/chat/conversation/input-area/input-state.test.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/shared/chat/conversation/input-area/input-state.test.tsx b/shared/chat/conversation/input-area/input-state.test.tsx index 663171990ac7..9eebd56b7cb3 100644 --- a/shared/chat/conversation/input-area/input-state.test.tsx +++ b/shared/chat/conversation/input-area/input-state.test.tsx @@ -543,7 +543,9 @@ test('onSubmit snapshots the dismissals before the composer clears them', async jest.spyOn(T.RPCChat, 'localUpdateUnsentTextRpcPromise').mockResolvedValue(undefined) jest.spyOn(T.RPCChat, 'localUnfurlPreviewLocalRpcPromise').mockResolvedValue([]) renderComposer() - useUnfurlPreviewState.getState().dispatch.dismiss(convID, ['http://a.com']) + act(() => { + useUnfurlPreviewState.getState().dispatch.dismiss(convID, ['http://a.com']) + }) act(() => { mockPlatformInputProps?.onSubmit('look at http://a.com')