-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
fix(core): ai apply ui opt #13238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: canary
Are you sure you want to change the base?
fix(core): ai apply ui opt #13238
Conversation
WalkthroughThe update refactors the UI and interaction elements in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DocEditTool
participant LoadingIcon
User->>DocEditTool: Clicks "Apply" or "Accept"
DocEditTool->>LoadingIcon: Show spinner
DocEditTool-->>User: Display "Applying"/"Accepting..." with spinner
DocEditTool->>DocEditTool: Process action
DocEditTool-->>User: Update UI based on result
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/frontend/core/src/blocksuite/ai/components/ai-tools/doc-edit.ts
(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/frontend/core/src/blocksuite/ai/components/ai-tools/doc-edit.ts (1)
Learnt from: yifeiyin
PR: toeverything/AFFiNE#13056
File: blocksuite/affine/blocks/embed-doc/src/embed-linked-doc-block/styles.ts:12-14
Timestamp: 2025-07-06T22:07:55.476Z
Learning: The design token `layer/background/tertiary` exists and is widely used throughout the AFFiNE codebase with `unsafeCSSVarV2`. It's used for borders in embed blocks, backgrounds in UI components, and is part of the established theme system. Don't flag this token as missing.
🧬 Code Graph Analysis (1)
packages/frontend/core/src/blocksuite/ai/components/ai-tools/doc-edit.ts (2)
blocksuite/affine/components/src/icons/text.ts (3)
ExpandFullIcon
(414-417)ExpandCloseIcon
(419-422)CopyIcon
(139-142)blocksuite/affine/components/src/icons/loading.ts (1)
LoadingIcon
(4-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (49)
- GitHub Check: fuzzing
- GitHub Check: y-octo binding test on aarch64-pc-windows-msvc
- GitHub Check: y-octo binding test on x86_64-apple-darwin
- GitHub Check: y-octo binding test on x86_64-pc-windows-msvc
- GitHub Check: loom thread test
- GitHub Check: Run native tests
- GitHub Check: Build AFFiNE native (aarch64-apple-darwin)
- GitHub Check: Build AFFiNE native (x86_64-apple-darwin)
- GitHub Check: E2E Mobile Test (3)
- GitHub Check: E2E Mobile Test (4)
- GitHub Check: Build AFFiNE native (aarch64-pc-windows-msvc)
- GitHub Check: E2E Mobile Test (5)
- GitHub Check: E2E BlockSuite Test (7)
- GitHub Check: Build AFFiNE native (x86_64-pc-windows-msvc)
- GitHub Check: E2E BlockSuite Test (2)
- GitHub Check: E2E Test (1)
- GitHub Check: E2E BlockSuite Test (9)
- GitHub Check: E2E BlockSuite Test (8)
- GitHub Check: E2E BlockSuite Test (10)
- GitHub Check: E2E Mobile Test (2)
- GitHub Check: E2E Mobile Test (1)
- GitHub Check: E2E BlockSuite Test (6)
- GitHub Check: E2E Test (5)
- GitHub Check: E2E BlockSuite Test (4)
- GitHub Check: E2E Test (9)
- GitHub Check: E2E BlockSuite Test (5)
- GitHub Check: E2E BlockSuite Test (3)
- GitHub Check: E2E Test (8)
- GitHub Check: E2E BlockSuite Test (1)
- GitHub Check: E2E BlockSuite Cross Browser Test (1, chromium)
- GitHub Check: E2E Test (10)
- GitHub Check: E2E BlockSuite Cross Browser Test (2, webkit)
- GitHub Check: E2E Test (4)
- GitHub Check: E2E Test (7)
- GitHub Check: E2E Test (2)
- GitHub Check: E2E BlockSuite Cross Browser Test (1, firefox)
- GitHub Check: E2E BlockSuite Cross Browser Test (2, chromium)
- GitHub Check: E2E BlockSuite Cross Browser Test (1, webkit)
- GitHub Check: E2E Test (3)
- GitHub Check: E2E Test (6)
- GitHub Check: E2E BlockSuite Cross Browser Test (2, firefox)
- GitHub Check: Typecheck
- GitHub Check: Build Server native
- GitHub Check: Analyze (typescript, affine)
- GitHub Check: Analyze (typescript, blocksuite)
- GitHub Check: Analyze (javascript, blocksuite)
- GitHub Check: Build @affine/electron renderer
- GitHub Check: Analyze (javascript, affine)
- GitHub Check: Lint
🔇 Additional comments (8)
packages/frontend/core/src/blocksuite/ai/components/ai-tools/doc-edit.ts (8)
5-5
: Good improvement: Loading icon importThe change from
AIStarIconWithAnimation
toLoadingIcon
provides better visual consistency for loading states throughout the application.
138-138
: Good styling improvement: Content borderAdding a top border to separate the content section improves visual hierarchy and structure.
120-131
: Excellent accessibility improvement: Button stylingConverting clickable spans to proper buttons with hover states significantly improves accessibility and user experience. The styling is consistent and well-implemented.
149-159
: Good consistency: Footer button stylingThe footer button styling matches the header operations and maintains visual consistency throughout the component.
435-444
: Excellent UX improvements: Tooltips and proper buttonsThe conversion of spans to buttons with tooltips for collapse/expand and copy operations greatly improves accessibility and user experience. The tooltip text is clear and helpful.
450-451
: Good loading state improvement: Apply buttonReplacing the animated star with
LoadingIcon
and "Applying" text provides clearer feedback about the loading state.
456-482
: Excellent accessibility refactor: Footer controlsConverting the footer controls from divs to proper buttons with appropriate styling, disabled states, and visual feedback significantly improves accessibility and user experience. The loading states are well-implemented.
475-481
: Good loading state consistency: Accept buttonUsing
LoadingIcon
for the accept button loading state maintains consistency with the apply button and provides clear visual feedback. The disabled styling with reduced opacity is appropriate.
@@ -83,7 +83,7 @@ export class DocEditTool extends WithDisposable(ShadowlessElement) { | |||
display: flex; | |||
flex-direction: column; | |||
align-items: flex-start; | |||
background: ${unsafeCSSVarV2('layer/background/overlayPanel')}; | |||
background: ${unsafeCSSVar('--affine-overlay-panel-shadow')}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential issue: Inconsistent CSS variable usage
The background color change from unsafeCSSVarV2('layer/background/overlayPanel')
to unsafeCSSVar('--affine-overlay-panel-shadow')
appears problematic:
- Mixed usage of
unsafeCSSVar
andunsafeCSSVarV2
functions - The new variable name suggests it's for shadow, not background
- This may not provide the intended visual styling
Consider reverting to the original variable or using the correct background variable:
- background: ${unsafeCSSVar('--affine-overlay-panel-shadow')};
+ background: ${unsafeCSSVarV2('layer/background/overlayPanel')};
Or if a different background is needed, use the appropriate variable with consistent function:
- background: ${unsafeCSSVar('--affine-overlay-panel-shadow')};
+ background: ${unsafeCSSVarV2('layer/background/tertiary')};
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
background: ${unsafeCSSVar('--affine-overlay-panel-shadow')}; | |
// revert to the original overlay‐panel background | |
background: ${unsafeCSSVarV2('layer/background/overlayPanel')}; |
background: ${unsafeCSSVar('--affine-overlay-panel-shadow')}; | |
// or, if you want a tertiary background instead | |
background: ${unsafeCSSVarV2('layer/background/tertiary')}; |
🤖 Prompt for AI Agents
In packages/frontend/core/src/blocksuite/ai/components/ai-tools/doc-edit.ts at
line 86, the background CSS variable was changed from using unsafeCSSVarV2 with
'layer/background/overlayPanel' to unsafeCSSVar with
'--affine-overlay-panel-shadow', which is inconsistent and likely incorrect for
background styling. To fix this, revert to using the original
unsafeCSSVarV2('layer/background/overlayPanel') for the background property or
replace it with the correct background-related CSS variable using the same
unsafeCSSVarV2 function to maintain consistency and ensure the intended visual
style.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## canary #13238 +/- ##
==========================================
- Coverage 56.87% 56.86% -0.02%
==========================================
Files 2709 2711 +2
Lines 132995 133006 +11
Branches 20590 20591 +1
==========================================
- Hits 75647 75630 -17
- Misses 55678 55705 +27
- Partials 1670 1671 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
Style
New Features
Bug Fixes
Refactor