Fix resilient link preview providers#361
Merged
Merged
Conversation
Reviewer's GuideIntroduces a shared, extensible link-preview provider system with caching and uses it from both the editor link-preview block and external link mentions, replacing direct Microlink calls and improving fallback behavior and resilience. Sequence diagram for link preview fetching with caching and fallback renderingsequenceDiagram
actor User
participant LinkPreview as LinkPreviewComponent
participant Utils as link_preview_utils
participant Cache as linkPreviewDataCache
participant Providers as LinkPreviewProviders
User->>LinkPreview: Paste URL
activate LinkPreview
LinkPreview->>LinkPreview: buildFallbackLinkPreviewData(url)
LinkPreview->>User: Render fallbackData immediately
LinkPreview->>Utils: fetchLinkPreviewData(url, signal)
activate Utils
Utils->>Cache: getCachedLinkPreviewData(cacheKey)
alt cache hit
Cache-->>Utils: LinkPreviewData
Utils-->>LinkPreview: LinkPreviewData
else cache miss
Utils->>Providers: fetchLinkPreviewDataFromProviders(normalizedUrl)
activate Providers
Providers->>Providers: getProviderGroups(context)
Providers->>Providers: fetchFirstSuccessfulProviderData(providers, context)
Providers-->>Utils: LinkPreviewData or fallbackData
deactivate Providers
Utils->>Cache: setCachedLinkPreviewData(cacheKey, data)
Utils-->>LinkPreview: LinkPreviewData
end
deactivate Utils
LinkPreview->>LinkPreview: setRemotePreview(data)
LinkPreview->>User: Re-render with remote preview data
deactivate LinkPreview
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
AbortSignalpassed intofetchLinkPreviewData(url, signal)is only used inraceWithAbortSignaland never threaded intoLinkPreviewProviderContext, so providers always seecontext.signal === undefinedand their axios calls can’t actually be aborted; consider passing the signal intofetchLinkPreviewDataFromProvidersand storing it on the context so provider-level requests are cancelable. - The
RemoteLinkPreviewDatatype and fallback/remote merge logic are duplicated betweenLinkPreviewandMentionExternalLink; consider extracting a shared type and/or a small hook/helper to centralize this behavior and avoid divergence over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `AbortSignal` passed into `fetchLinkPreviewData(url, signal)` is only used in `raceWithAbortSignal` and never threaded into `LinkPreviewProviderContext`, so providers always see `context.signal === undefined` and their axios calls can’t actually be aborted; consider passing the signal into `fetchLinkPreviewDataFromProviders` and storing it on the context so provider-level requests are cancelable.
- The `RemoteLinkPreviewData` type and fallback/remote merge logic are duplicated between `LinkPreview` and `MentionExternalLink`; consider extracting a shared type and/or a small hook/helper to centralize this behavior and avoid divergence over time.
## Individual Comments
### Comment 1
<location path="src/utils/link-preview.ts" line_range="241-250" />
<code_context>
+ }
+}
+
+export async function fetchLinkPreviewData(url: string, signal?: AbortSignal): Promise<LinkPreviewData> {
+ const normalizedUrl = processUrl(url) || url;
+ const cacheKey = getLinkPreviewCacheKey(normalizedUrl);
+ const cachedData = getCachedLinkPreviewData(cacheKey);
+
+ if (cachedData) return cachedData;
+
+ let request = inFlightLinkPreviewRequests.get(cacheKey);
+
+ if (!request) {
+ const registryVersion = providerRegistryVersion;
+
+ request = fetchLinkPreviewDataFromProviders(normalizedUrl)
+ .then((data) => {
+ if (registryVersion === providerRegistryVersion) {
</code_context>
<issue_to_address>
**issue (bug_risk):** AbortSignal is not propagated into provider fetches, so network requests cannot actually be cancelled.
In `fetchLinkPreviewData`, the `signal` is only used by `raceWithAbortSignal` and never passed into `fetchLinkPreviewDataFromProviders`, so providers see `signal: undefined` and the axios calls in `microlinkProvider`/`githubProvider` are never cancelled. This wastes network/resources when callers abort (e.g., unmounts or rapid URL changes).
Please update `fetchLinkPreviewDataFromProviders` to accept an optional `signal` and include it in `LinkPreviewProviderContext` (and update its callers), so axios receives the real `AbortSignal` and `isAbortError(result.error, context.signal)` can reliably detect aborts.
</issue_to_address>
### Comment 2
<location path="src/utils/__tests__/link-preview.test.ts" line_range="27-34" />
<code_context>
+ mockedAxios.isCancel.mockReturnValue(false);
+ });
+
+ it('builds a readable fallback for any URL', () => {
+ expect(buildFallbackLinkPreviewData('https://example.com/docs/getting-started?tab=web')).toEqual({
+ title: 'example.com/docs/getting-started',
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen `buildFallbackLinkPreviewData` tests with non-URL and edge-case inputs.
Given this is the last-resort path, consider adding tests for:
- A clearly invalid string (e.g. `'not a url'`) to verify we return `{ title: originalString, description: '' }`.
- A URL with percent-encoded segments (including malformed sequences, e.g. `'https://example.com/%E0%A4%A'`) to exercise `safeDecodeURIComponent`.
- A bare domain without scheme (if `processUrl` supports it) to confirm host extraction and `www.` stripping.
These will better pin down behavior for edge inputs.
```suggestion
it('builds a readable fallback for any URL', () => {
expect(buildFallbackLinkPreviewData('https://example.com/docs/getting-started?tab=web')).toEqual({
title: 'example.com/docs/getting-started',
description: '',
});
});
it('falls back to the original string for clearly invalid input', () => {
expect(buildFallbackLinkPreviewData('not a url')).toEqual({
title: 'not a url',
description: '',
});
});
it('handles URLs with percent-encoded segments, including malformed sequences', () => {
const encodedUrl = 'https://example.com/%E0%A4%A';
expect(buildFallbackLinkPreviewData(encodedUrl)).toEqual({
title: 'example.com/%E0%A4%A',
description: '',
});
});
it('handles bare domains without a scheme when supported by processUrl', () => {
expect(buildFallbackLinkPreviewData('example.com/docs/getting-started')).toEqual({
title: 'example.com/docs/getting-started',
description: '',
});
});
it('uses generic metadata when the universal provider succeeds', async () => {
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+241
to
+250
| export async function fetchLinkPreviewData(url: string, signal?: AbortSignal): Promise<LinkPreviewData> { | ||
| const normalizedUrl = processUrl(url) || url; | ||
| const cacheKey = getLinkPreviewCacheKey(normalizedUrl); | ||
| const cachedData = getCachedLinkPreviewData(cacheKey); | ||
|
|
||
| if (cachedData) return cachedData; | ||
|
|
||
| let request = inFlightLinkPreviewRequests.get(cacheKey); | ||
|
|
||
| if (!request) { |
There was a problem hiding this comment.
issue (bug_risk): AbortSignal is not propagated into provider fetches, so network requests cannot actually be cancelled.
In fetchLinkPreviewData, the signal is only used by raceWithAbortSignal and never passed into fetchLinkPreviewDataFromProviders, so providers see signal: undefined and the axios calls in microlinkProvider/githubProvider are never cancelled. This wastes network/resources when callers abort (e.g., unmounts or rapid URL changes).
Please update fetchLinkPreviewDataFromProviders to accept an optional signal and include it in LinkPreviewProviderContext (and update its callers), so axios receives the real AbortSignal and isAbortError(result.error, context.signal) can reliably detect aborts.
Comment on lines
+27
to
+34
| it('builds a readable fallback for any URL', () => { | ||
| expect(buildFallbackLinkPreviewData('https://example.com/docs/getting-started?tab=web')).toEqual({ | ||
| title: 'example.com/docs/getting-started', | ||
| description: '', | ||
| }); | ||
| }); | ||
|
|
||
| it('uses generic metadata when the universal provider succeeds', async () => { |
There was a problem hiding this comment.
suggestion (testing): Strengthen buildFallbackLinkPreviewData tests with non-URL and edge-case inputs.
Given this is the last-resort path, consider adding tests for:
- A clearly invalid string (e.g.
'not a url') to verify we return{ title: originalString, description: '' }. - A URL with percent-encoded segments (including malformed sequences, e.g.
'https://example.com/%E0%A4%A') to exercisesafeDecodeURIComponent. - A bare domain without scheme (if
processUrlsupports it) to confirm host extraction andwww.stripping.
These will better pin down behavior for edge inputs.
Suggested change
| it('builds a readable fallback for any URL', () => { | |
| expect(buildFallbackLinkPreviewData('https://example.com/docs/getting-started?tab=web')).toEqual({ | |
| title: 'example.com/docs/getting-started', | |
| description: '', | |
| }); | |
| }); | |
| it('uses generic metadata when the universal provider succeeds', async () => { | |
| it('builds a readable fallback for any URL', () => { | |
| expect(buildFallbackLinkPreviewData('https://example.com/docs/getting-started?tab=web')).toEqual({ | |
| title: 'example.com/docs/getting-started', | |
| description: '', | |
| }); | |
| }); | |
| it('falls back to the original string for clearly invalid input', () => { | |
| expect(buildFallbackLinkPreviewData('not a url')).toEqual({ | |
| title: 'not a url', | |
| description: '', | |
| }); | |
| }); | |
| it('handles URLs with percent-encoded segments, including malformed sequences', () => { | |
| const encodedUrl = 'https://example.com/%E0%A4%A'; | |
| expect(buildFallbackLinkPreviewData(encodedUrl)).toEqual({ | |
| title: 'example.com/%E0%A4%A', | |
| description: '', | |
| }); | |
| }); | |
| it('handles bare domains without a scheme when supported by processUrl', () => { | |
| expect(buildFallbackLinkPreviewData('example.com/docs/getting-started')).toEqual({ | |
| title: 'example.com/docs/getting-started', | |
| description: '', | |
| }); | |
| }); | |
| it('uses generic metadata when the universal provider succeeds', async () => { |
appflowy
force-pushed
the
fix/link-preview-provider-fallbacks
branch
from
f46e010 to
3473f8b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests
Summary by Sourcery
Introduce a shared, extensible link preview infrastructure with fallbacks and caching, and wire it into link preview blocks and external link mentions.
New Features:
Enhancements:
Tests: