Conversation
Co-authored-by: aider (gemini/gemini-2.5-pro-preview-06-05) <aider@aider.chat>
Co-authored-by: aider (gemini/gemini-2.5-pro-preview-06-05) <aider@aider.chat>
beastoin
force-pushed
the
c2o4y_macos
branch
from
76f6b10 to
f990b27
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces UI simplifications for several desktop components, replacing complex animations and layouts with cleaner designs, and changes the "Share" functionality to "Copy Link". My review identifies a critical build error due to a missing required parameter in a widget constructor. Additionally, I've pointed out a UX bug in the new "Copy Link" feature, regressions in functionality (keyboard shortcut removal) and style guide adherence, and a maintainability issue with a hardcoded URL.
Comment on lines
+501
to
+520
| Future<void> _handleCopyConversationLink() async { | ||
| if (_isSharing) return; | ||
| setState(() => _isSharing = true); | ||
|
|
||
| try { | ||
| bool shared = await setConversationVisibility(widget.conversation.id); | ||
| if (!shared) { | ||
| _showSnackBar('Conversation URL could not be generated.'); | ||
| setState(() => _isSharing = false); | ||
| return; | ||
| } | ||
|
|
||
| String content = 'https://h.omi.me/conversations/${widget.conversation.id}'; | ||
| await Clipboard.setData(ClipboardData(text: content)); | ||
| } catch (e) { | ||
| _showSnackBar('Failed to generate conversation link'); | ||
| } finally { | ||
| setState(() => _isSharing = false); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
There are a couple of issues in _handleCopyConversationLink:
- UX Issue: The button label changes to 'Copied!' but immediately reverts to 'Copy Link' because
_isSharingis set tofalsein thefinallyblock, which executes right afterClipboard.setData. The user won't see the confirmation. You should introduce a delay to keep the 'Copied!' state visible. - Redundant Code:
setState(() => _isSharing = false);on line 509 is redundant because thefinallyblock already handles this. It should be removed. - Safety: The
setStatecalls should be guarded with aif (mounted)check to prevent errors if the widget is disposed during an async operation.
Future<void> _handleCopyConversationLink() async {
if (_isSharing) return;
if (mounted) {
setState(() => _isSharing = true);
}
try {
bool shared = await setConversationVisibility(widget.conversation.id);
if (!shared) {
_showSnackBar('Conversation URL could not be generated.');
return;
}
String content = 'https://h.omi.me/conversations/${widget.conversation.id}';
await Clipboard.setData(ClipboardData(text: content));
// Keep "Copied!" state visible for user feedback.
await Future.delayed(const Duration(seconds: 2));
} catch (e) {
_showSnackBar('Failed to generate conversation link');
} finally {
if (mounted) {
setState(() => _isSharing = false);
}
}
}| return; | ||
| } | ||
|
|
||
| String content = 'https://h.omi.me/conversations/${widget.conversation.id}'; |
Contributor
There was a problem hiding this comment.
The base URL https://h.omi.me is hardcoded. This can cause issues when deploying to different environments (e.g., staging, production). It's better to use a centralized configuration, similar to how Env.apiBaseUrl is used for API calls. Please define and use a constant from your environment configuration, for example Env.webBaseUrl.
String content = '${Env.webBaseUrl}/conversations/${widget.conversation.id}';
Comment on lines
+100
to
136
| child: GestureDetector( | ||
| onSecondaryTapDown: _showContextMenu, | ||
| child: GestureDetector( | ||
| onSecondaryTapDown: _showContextMenu, | ||
| child: AnimatedBuilder( | ||
| animation: _animationController, | ||
| builder: (context, child) { | ||
| return Transform.scale( | ||
| scale: _scaleAnimation.value, | ||
| child: Material( | ||
| color: Colors.transparent, | ||
| child: InkWell( | ||
| onTap: () { | ||
| if (widget.conversation.isLocked) { | ||
| Navigator.of(context).push( | ||
| MaterialPageRoute( | ||
| builder: (context) => const UsagePage(showUpgradeDialog: true), | ||
| ), | ||
| ); | ||
| return; | ||
| } | ||
| widget.onTap(); | ||
| }, | ||
| borderRadius: BorderRadius.circular(16), | ||
| child: Container( | ||
| width: double.maxFinite, | ||
| margin: const EdgeInsets.only(bottom: 4), | ||
| decoration: BoxDecoration( | ||
| color: Colors.white.withValues(alpha: 0.05), | ||
| borderRadius: BorderRadius.circular(16), | ||
| border: Border.all( | ||
| color: _isHovered | ||
| ? ResponsiveHelper.purplePrimary.withValues(alpha: 0.4) | ||
| : ResponsiveHelper.backgroundTertiary.withValues(alpha: 0.3), | ||
| width: 1, | ||
| ), | ||
| boxShadow: [ | ||
| BoxShadow( | ||
| color: Colors.black.withValues(alpha: 0.08 + (_elevationAnimation.value * 0.12)), | ||
| blurRadius: 12 + (_elevationAnimation.value * 8), | ||
| offset: Offset(0, 4 + (_elevationAnimation.value * 4)), | ||
| spreadRadius: _elevationAnimation.value * 2, | ||
| ), | ||
| if (_isHovered) | ||
| BoxShadow( | ||
| color: ResponsiveHelper.purplePrimary.withValues(alpha: 0.01), | ||
| blurRadius: 20, | ||
| offset: const Offset(0, 8), | ||
| ), | ||
| ], | ||
| ), | ||
| child: ClipRRect( | ||
| borderRadius: BorderRadius.circular(16), | ||
| child: Column( | ||
| children: [ | ||
| // Main card content | ||
| Container( | ||
| padding: const EdgeInsets.all(20), | ||
| child: Column( | ||
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| children: [ | ||
| // Modern header with integrated action area | ||
| _buildModernHeader(), | ||
|
|
||
| const SizedBox(height: 16), | ||
|
|
||
| // Content section with improved layout | ||
| _buildContentSection(), | ||
|
|
||
| const SizedBox(height: 14), | ||
|
|
||
| // Footer with metadata | ||
| _buildFooter(), | ||
| ], | ||
| ), | ||
| ), | ||
|
|
||
| // Action bar that slides up on hover | ||
| if (_isHovered) | ||
| SizeTransition( | ||
| sizeFactor: _actionBarAnimation, | ||
| axisAlignment: -1.0, | ||
| child: Container( | ||
| width: double.infinity, | ||
| height: 44, | ||
| decoration: BoxDecoration( | ||
| color: ResponsiveHelper.backgroundTertiary.withValues(alpha: 0.8), | ||
| border: Border( | ||
| top: BorderSide( | ||
| color: ResponsiveHelper.backgroundQuaternary.withValues(alpha: 0.5), | ||
| width: 1, | ||
| ), | ||
| ), | ||
| ), | ||
| child: Row( | ||
| children: [ | ||
| const SizedBox(width: 16), | ||
|
|
||
| // Delete action using OmiButton | ||
| Tooltip( | ||
| message: 'Delete conversation (Del)', | ||
| decoration: BoxDecoration( | ||
| color: ResponsiveHelper.backgroundTertiary, | ||
| borderRadius: BorderRadius.circular(6), | ||
| ), | ||
| textStyle: const TextStyle( | ||
| color: ResponsiveHelper.textPrimary, | ||
| fontSize: 12, | ||
| ), | ||
| child: OmiButton( | ||
| label: 'Delete', | ||
| icon: Icons.delete_outline_rounded, | ||
| type: OmiButtonType.neutral, | ||
| color: ResponsiveHelper.errorColor, | ||
| onPressed: () { | ||
| HapticFeedback.lightImpact(); | ||
| _showDeleteConfirmation(); | ||
| }, | ||
| ), | ||
| ), | ||
|
|
||
| const Spacer(), | ||
|
|
||
| // Additional actions hint | ||
| const Text( | ||
| 'Right-click for more options', | ||
| style: TextStyle( | ||
| fontSize: 11, | ||
| color: ResponsiveHelper.textTertiary, | ||
| ), | ||
| ), | ||
|
|
||
| const SizedBox(width: 16), | ||
| ], | ||
| ), | ||
| ), | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| onTap: () { | ||
| if (widget.conversation.isLocked) { | ||
| Navigator.of(context).push( | ||
| MaterialPageRoute( | ||
| builder: (context) => const UsagePage(showUpgradeDialog: true), | ||
| ), | ||
| ); | ||
| }, | ||
| return; | ||
| } | ||
| widget.onTap(); | ||
| }, | ||
| child: Container( | ||
| width: double.maxFinite, | ||
| margin: const EdgeInsets.only(bottom: 8), | ||
| padding: const EdgeInsets.all(16), | ||
| decoration: BoxDecoration( | ||
| color: Colors.white.withOpacity(0.03), | ||
| borderRadius: BorderRadius.circular(12), | ||
| border: Border.all( | ||
| color: _isHovered ? Colors.white.withOpacity(0.3) : Colors.white.withOpacity(0.05), | ||
| width: 1, | ||
| ), | ||
| ), | ||
| child: Column( | ||
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| children: [ | ||
| _buildHeader(), | ||
| const SizedBox(height: 12), | ||
| _buildContent(), | ||
| ], | ||
| ), | ||
| ), | ||
| ), | ||
| ), |
Contributor
There was a problem hiding this comment.
The Focus widget that enabled deleting a conversation via the Delete key has been removed. This removes a useful keyboard shortcut that was previously available and advertised in a tooltip. Please reintroduce this functionality to maintain keyboard accessibility.
Comment on lines
+118
to
+124
| decoration: BoxDecoration( | ||
| color: Colors.white.withOpacity(0.03), | ||
| borderRadius: BorderRadius.circular(12), | ||
| border: Border.all( | ||
| color: _isHovered ? Colors.white.withOpacity(0.3) : Colors.white.withOpacity(0.05), | ||
| width: 1, | ||
| ), |
Contributor
There was a problem hiding this comment.
The general rules for this repository state: 'Use the custom withValues({double? alpha}) extension method instead of the deprecated withOpacity() to modify a color's opacity.'1 This code uses withOpacity(), which violates this rule. Please use withValues() instead.
Suggested change
| decoration: BoxDecoration( | |
| color: Colors.white.withOpacity(0.03), | |
| borderRadius: BorderRadius.circular(12), | |
| border: Border.all( | |
| color: _isHovered ? Colors.white.withOpacity(0.3) : Colors.white.withOpacity(0.05), | |
| width: 1, | |
| ), | |
| decoration: BoxDecoration( | |
| color: Colors.white.withValues(alpha: 0.03), | |
| borderRadius: BorderRadius.circular(12), | |
| border: Border.all( | |
| color: _isHovered ? Colors.white.withValues(alpha: 0.3) : Colors.white.withValues(alpha: 0.05), | |
| width: 1, | |
| ), | |
Rules References
Footnotes
-
Use the custom
withValues({double? alpha})extension method instead of the deprecatedwithOpacity()to modify a color's opacity. ↩
Comment on lines
+124
to
+138
| boxShadow: _isHovered | ||
| ? [ | ||
| BoxShadow( | ||
| color: ResponsiveHelper.purplePrimary.withOpacity(0.3), | ||
| blurRadius: 25, | ||
| offset: const Offset(0, 10), | ||
| ), | ||
| ] | ||
| : [ | ||
| BoxShadow( | ||
| color: ResponsiveHelper.purplePrimary.withOpacity(0.2), | ||
| blurRadius: 15, | ||
| offset: const Offset(0, 5), | ||
| ), | ||
| ], |
Contributor
There was a problem hiding this comment.
The general rules for this repository state: 'Use the custom withValues({double? alpha}) extension method instead of the deprecated withOpacity() to modify a color's opacity.'1 This code uses withOpacity(), which violates this rule. Please use withValues() instead.
Suggested change
| boxShadow: _isHovered | |
| ? [ | |
| BoxShadow( | |
| color: ResponsiveHelper.purplePrimary.withOpacity(0.3), | |
| blurRadius: 25, | |
| offset: const Offset(0, 10), | |
| ), | |
| ] | |
| : [ | |
| BoxShadow( | |
| color: ResponsiveHelper.purplePrimary.withOpacity(0.2), | |
| blurRadius: 15, | |
| offset: const Offset(0, 5), | |
| ), | |
| ], | |
| boxShadow: _isHovered | |
| ? [ | |
| BoxShadow( | |
| color: ResponsiveHelper.purplePrimary.withValues(alpha: 0.3), | |
| blurRadius: 25, | |
| offset: const Offset(0, 10), | |
| ), | |
| ] | |
| : [ | |
| BoxShadow( | |
| color: ResponsiveHelper.purplePrimary.withValues(alpha: 0.2), | |
| blurRadius: 15, | |
| offset: const Offset(0, 5), | |
| ), | |
| ], | |
Rules References
Footnotes
-
Use the custom
withValues({double? alpha})extension method instead of the deprecatedwithOpacity()to modify a color's opacity. ↩
Glucksberg
pushed a commit
to Glucksberg/omi-local
that referenced
this pull request
Apr 28, 2026
* Fix share button -> copy link only; simplify the UI on conversations page * refactor: remove subtitles from context menu items for minimal UI Co-authored-by: aider (gemini/gemini-2.5-pro-preview-06-05) <aider@aider.chat> * fix: add missing subtitle parameter to OmiContextMenuItem Co-authored-by: aider (gemini/gemini-2.5-pro-preview-06-05) <aider@aider.chat> * Simplify the convos context menu --------- Co-authored-by: aider (gemini/gemini-2.5-pro-preview-06-05) <aider@aider.chat>
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.
No description provided.