feat: revamp link preivew #7692
Open
+3,054
−149
Conversation
This file contains 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
Reviewer's Guide by SourceryThis pull request revamps the link preview functionality in the document editor, introducing a more flexible and feature-rich link preview system with enhanced UI and interaction capabilities. It includes new features such as a 'Paste as' menu for link types, a new link preview menu with multiple conversion options, and enhanced link preview UI with loading and error states. Enhancements include improved link preview component with more robust handling of different link states, more interactive menu options for link preview, and enhanced link preview styling and responsiveness. Chores include refactoring link preview and embed block components and updating theme and styling configurations. Sequence diagram for converting URL preview to MentionsequenceDiagram
participant User
participant EditorState
participant Transaction
participant Node
User->>EditorState: Initiates convertUrlPreviewNodeToMention(editorState, node)
EditorState->>Transaction: Creates a new transaction
Transaction->>Transaction: Inserts MentionBlock node with URL
Transaction->>Transaction: Deletes the original URL preview node
EditorState->>Transaction: Applies the transaction
Transaction-->>EditorState: Transaction applied
EditorState-->>User: Updates the editor with the MentionBlock
Sequence diagram for converting URL to Link PreviewsequenceDiagram
participant User
participant EditorState
participant Transaction
participant Node
User->>EditorState: Initiates convertUrlToLinkPreview(editorState, selection, url)
EditorState->>Node: getNodeAtPath(selection.end.path)
EditorState->>Transaction: Creates a new transaction
Transaction->>Transaction: Deletes the original node
Transaction->>Transaction: Inserts linkPreviewNode with URL
EditorState->>Transaction: Applies the transaction
Transaction-->>EditorState: Transaction applied
EditorState-->>User: Updates the editor with the Link Preview
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
🥷 Ninja i18n – 🛎️ Translations need to be updatedProject
|
| lint rule | new reports | level | link |
|---|---|---|---|
| Missing translation | 464 | warning | contribute (via Fink 🐦) |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @asjqkkkk - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding comments to the new methods to explain their purpose.
- The diff is quite large; consider breaking it down into smaller, more manageable PRs.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey @asjqkkkk - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider extracting some of the complex widget building logic into separate methods for better readability.
- The number of popover controllers might be excessive; consider consolidating them where possible.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -41,6 +43,13 @@ extension PasteFromPlainText on EditorState { | |||
| } | |||
| if (nodes.length == 1) { | |||
| await pasteSingleLineNode(nodes.first); | |||
| final href = _getLinkFromNode(nodes.first); | |||
There was a problem hiding this comment.
suggestion: Centralize link extraction logic in paste handling.
Introducing the _getLinkFromNode helper improves modularity when obtaining a hyperlink from a node. Check that this logic covers all necessary URL formats for pasted content.
Suggested implementation:
String? _getLinkFromNode(DocumentNode node) {
// Extract trimmed content from the node
final text = node.content.trim();
// Try to parse the content as a valid URL
final uri = Uri.tryParse(text);
if (uri != null && (uri.hasScheme || uri.host.isNotEmpty)) {
return text;
}
// Fallback: use regex extraction to cover common URL formats (e.g., if the scheme is missing)
final regex = RegExp(r'(https?://[^\s]+)');
final match = regex.firstMatch(text);
if (match != null) {
return match.group(0);
}
// Return null if no URL is found
return null;
}
final href = _getLinkFromNode(nodes.first); // Centralized link extraction for pasted content
Ensure that:
- The DocumentNode type (or similar) used in _getLinkFromNode has a proper "content" property representing its text.
- The regular expression used covers all necessary URL formats in your application’s context; update the regex if additional URL patterns need to be supported.
- This helper is placed in a location in the file where it is accessible by pastePlainText.
| ), | ||
| tooltipText: LocaleKeys.editor_copyLink.tr(), | ||
| preferBelow: false, | ||
| onPressed: () => copyLink(context), |
There was a problem hiding this comment.
suggestion: Review command mapping in LinkEmbedMenu for clarity.
The LinkEmbedMenu now offers multiple actions such as copy, turn into, and more options. Double-check that the menus (turn-into and more options) are intuitive for users and that their corresponding commands are clearly mapped.
| import 'package:appflowy_editor/appflowy_editor.dart'; | ||
| import 'package:flutter/material.dart'; | ||
|
|
||
| extension MenuExtension on EditorState { |
There was a problem hiding this comment.
suggestion (testing): Validate flexible menu positioning logic.
The MenuExtension now calculates menu offsets based on the editor's dimensions and selection rectangles. Please review the calculations to ensure they produce the intended positioning across various screen sizes.
Suggested implementation:
late Rect startRect;
if (rect != null) {
startRect = rect;
} else {
// Fallback to the first selection rect if available; otherwise, use Rect.zero
startRect = selectionRects.isNotEmpty ? selectionRects.first : Rect.zero;
}
// Obtain the screen size from context for flexible positioning
final BuildContext context = this.context;
final Size screenSize = MediaQuery.of(context).size;
final double dxCandidate = startRect.left + menuOffset.dx;
final double dyCandidate = startRect.bottom + menuOffset.dy;
// Adjust horizontal position if the menu exceeds screen boundaries
final double dx = (dxCandidate + menuWidth > screenSize.width)
? screenSize.width - menuWidth
: dxCandidate;
// Adjust vertical position: if the menu overshoots the bottom, try positioning it above the selection
double dy = dyCandidate;
if (dyCandidate + menuHeight > screenSize.height) {
dy = (startRect.top - menuHeight - menuOffset.dy >= 0)
? startRect.top - menuHeight - menuOffset.dy
: screenSize.height - menuHeight;
}
return MenuPosition(offset: Offset(dx, dy));
Note:
- Ensure that EditorState indeed has access to a BuildContext (as is the case with State subclasses in Flutter). If this is not true, you may need to pass down a BuildContext to this method.
- Test the menu positioning on various screen sizes to verify that the fallback logic works as intended.
| // ignore: depend_on_referenced_packages | ||
| import 'package:flutter_link_previewer/flutter_link_previewer.dart' hide Size; | ||
|
|
||
| class LinkParser { |
There was a problem hiding this comment.
suggestion (performance): Caching link preview data to optimize performance.
The implementation of LinkParser now leverages a caching mechanism (using KeyValueStorage) to reduce redundant network calls. Ensure that cache invalidation or staleness checks are handled appropriately in long-running sessions.
Suggested implementation:
Future<void> start(String url) async {
final data = await _cache.get(url);
if (data != null && !_isStale(data)) {
refreshLinkInfo(data);
}
await _getLinkInfo(url);
}
// Add a helper to check if the cached LinkInfo is stale.
// Adjust this method if your LinkInfo model uses a different timestamp property.
bool _isStale(LinkInfo data) {
const Duration cacheExpiryDuration = Duration(minutes: 10);
return DateTime.now().difference(data.timestamp) > cacheExpiryDuration;
}
Future<LinkInfo?> _getLinkInfo(String url) async {
Ensure that your LinkInfo class includes a timestamp (for example, a property named "timestamp" of type DateTime) that records when it was fetched. You may need to update the LinkInfoCache to store this timestamp when caching data. Adjust the cacheExpiryDuration as needed for your application.
asjqkkkk
force-pushed
the
feat/link_preview
branch
from
8ddb486 to
c5fe9fc
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Paste asmenu (URL, Mention, Bookmark, Embed)Convert tomenu(Mention, Bookmark, Embed)Convert tomenu(URL, Bookmark, Embed, Remove Link)More optionsmenu(Mention, URL, Embed, Copy Link, Replace, Reload, Remove Link)More optionsmenu(Alignment, Open link, Replace, Reload, Remove Link)Convert tomenu(Mention, URL, Bookmark)Feature Preview
PR Checklist
Summary by Sourcery
Revamp the link preview functionality in the document editor, introducing a more flexible and feature-rich link preview system with enhanced UI and interaction capabilities.
New Features:
Enhancements:
Chores:
Summary by Sourcery
Revamp the link preview functionality in the document editor, introducing a more flexible and feature-rich link preview system with enhanced UI and interaction capabilities.
New Features:
Enhancements:
Chores: