Conversation
https://github.com/user-attachments/assets/d5578060-2c8c-47a5-ba65-ef2e9430518b This PR adds the ability to group-by date with configuration which an example is shown in the image below:  <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Date-based grouping modes (relative, day, week Sun/Mon, month, year), a date group renderer, and quick lookup for group-by configs by name. * **Improvements** * Enhanced group settings: date sub‑modes, week‑start, per‑group visibility, Hide All/Show All, date sort order, improved drag/drop and reorder. * Consistent popup placement/middleware, nested popup positioning, per‑item close-on-select, and enforced minimum menu heights. * UI: empty groups now display "No <property>"; views defensively handle null/hidden groups. * **Tests** * Added unit tests for date-key sorting and comparison. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Norkz <richardlora557@gmail.com> Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
This PR contains the following updates: | Package | Update | Change | |---|---|---| | [rustc](https://redirect.github.com/rust-lang/rust) | minor | `1.91.0` -> `1.92.0` | --- ### Release Notes <details> <summary>rust-lang/rust (rustc)</summary> ### [`v1.92.0`](https://redirect.github.com/rust-lang/rust/blob/HEAD/RELEASES.md#Version-1920-2025-12-11) [Compare Source](https://redirect.github.com/rust-lang/rust/compare/1.91.1...1.92.0) \========================== <a id="1.92.0-Language"></a> ## Language - [Document `MaybeUninit` representation and validity](https://redirect.github.com/rust-lang/rust/pull/140463) - [Allow `&raw [mut | const]` for union field in safe code](https://redirect.github.com/rust-lang/rust/pull/141469) - [Prefer item bounds of associated types over where-bounds for auto-traits and `Sized`](https://redirect.github.com/rust-lang/rust/pull/144064) - [Do not materialize `X` in `[X; 0]` when `X` is unsizing a const](https://redirect.github.com/rust-lang/rust/pull/145277) - [Support combining `#[track_caller]` and `#[no_mangle]` (requires every declaration specifying `#[track_caller]` as well)](https://redirect.github.com/rust-lang/rust/pull/145724) - [Make never type lints `never_type_fallback_flowing_into_unsafe` and `dependency_on_unit_never_type_fallback` deny-by-default](https://redirect.github.com/rust-lang/rust/pull/146167) - [Allow specifying multiple bounds for same associated item, except in trait objects](https://redirect.github.com/rust-lang/rust/pull/146593) - [Slightly strengthen higher-ranked region handling in coherence](https://redirect.github.com/rust-lang/rust/pull/146725) - [The `unused_must_use` lint no longer warns on `Result<(), Uninhabited>` (for instance, `Result<(), !>`), or `ControlFlow<Uninhabited, ()>`](https://redirect.github.com/rust-lang/rust/pull/147382). This avoids having to check for an error that can never happen. <a id="1.92.0-Compiler"></a> ## Compiler - [Make `mips64el-unknown-linux-muslabi64` link dynamically](https://redirect.github.com/rust-lang/rust/pull/146858) - [Remove current code for embedding command-line args in PDB](https://redirect.github.com/rust-lang/rust/pull/147022) Command-line information is typically not needed by debugging tools, and the removed code was causing problems for incremental builds even on targets that don't use PDB debuginfo. <a id="1.92.0-Libraries"></a> ## Libraries - [Specialize `Iterator::eq{_by}` for `TrustedLen` iterators](https://redirect.github.com/rust-lang/rust/pull/137122) - [Simplify `Extend` for tuples](https://redirect.github.com/rust-lang/rust/pull/138799) - [Added details to `Debug` for `EncodeWide`](https://redirect.github.com/rust-lang/rust/pull/140153). - [`iter::Repeat::last`](https://redirect.github.com/rust-lang/rust/pull/147258) and [`count`](https://redirect.github.com/rust-lang/rust/pull/146410) will now panic, rather than looping infinitely. <a id="1.92.0-Stabilized-APIs"></a> ## Stabilized APIs - [`NonZero<u{N}>::div_ceil`](https://doc.rust-lang.org/stable/std/num/struct.NonZero.html#method.div_ceil) - [`Location::file_as_c_str`](https://doc.rust-lang.org/stable/std/panic/struct.Location.html#method.file_as_c_str) - [`RwLockWriteGuard::downgrade`](https://doc.rust-lang.org/stable/std/sync/struct.RwLockWriteGuard.html#method.downgrade) - [`Box::new_zeroed`](https://doc.rust-lang.org/stable/std/boxed/struct.Box.html#method.new_zeroed) - [`Box::new_zeroed_slice`](https://doc.rust-lang.org/stable/std/boxed/struct.Box.html#method.new_zeroed_slice) - [`Rc::new_zeroed`](https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.new_zeroed) - [`Rc::new_zeroed_slice`](https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.new_zeroed_slice) - [`Arc::new_zeroed`](https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.new_zeroed) - [`Arc::new_zeroed_slice`](https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.new_zeroed_slice) - [`btree_map::Entry::insert_entry`](https://doc.rust-lang.org/stable/std/collections/btree_map/enum.Entry.html#method.insert_entry) - [`btree_map::VacantEntry::insert_entry`](https://doc.rust-lang.org/stable/std/collections/btree_map/struct.VacantEntry.html#method.insert_entry) - [`impl Extend<proc_macro::Group> for proc_macro::TokenStream`](https://doc.rust-lang.org/stable/proc_macro/struct.TokenStream.html#impl-Extend%3CGroup%3E-for-TokenStream) - [`impl Extend<proc_macro::Literal> for proc_macro::TokenStream`](https://doc.rust-lang.org/stable/proc_macro/struct.TokenStream.html#impl-Extend%3CLiteral%3E-for-TokenStream) - [`impl Extend<proc_macro::Punct> for proc_macro::TokenStream`](https://doc.rust-lang.org/stable/proc_macro/struct.TokenStream.html#impl-Extend%3CPunct%3E-for-TokenStream) - [`impl Extend<proc_macro::Ident> for proc_macro::TokenStream`](https://doc.rust-lang.org/stable/proc_macro/struct.TokenStream.html#impl-Extend%3CIdent%3E-for-TokenStream) These previously stable APIs are now stable in const contexts: - [`<[_]>::rotate_left`](https://doc.rust-lang.org/stable/std/primitive.slice.html#method.rotate_left) - [`<[_]>::rotate_right`](https://doc.rust-lang.org/stable/std/primitive.slice.html#method.rotate_right) <a id="1.92.0-Cargo"></a> ## Cargo - [Added a new chapter](https://redirect.github.com/rust-lang/cargo/issues/16119) to the Cargo book, ["Optimizing Build Performance"](https://doc.rust-lang.org/stable/cargo/guide/build-performance.html). <a id="1.92.0-Rustdoc"></a> ## Rustdoc - [If a trait item appears in rustdoc search, hide the corresponding impl items](https://redirect.github.com/rust-lang/rust/pull/145898). Previously a search for "last" would show both `Iterator::last` as well as impl methods like `std::vec::IntoIter::last`. Now these impl methods will be hidden, freeing up space for inherent methods like `BTreeSet::last`. - [Relax rules for identifiers in search](https://redirect.github.com/rust-lang/rust/pull/147860). Previously you could only search for identifiers that were valid in rust code, now searches only need to be valid as part of an identifier. For example, you can now perform a search that starts with a digit. <a id="1.92.0-Compatibility-Notes"></a> ## Compatibility Notes - [Fix backtraces with `-C panic=abort` on Linux by generating unwind tables by default](https://redirect.github.com/rust-lang/rust/pull/143613). Build with `-C force-unwind-tables=no` to keep omitting unwind tables. * As part of the larger effort refactoring compiler built-in attributes and their diagnostics, [the future-compatibility lint `invalid_macro_export_arguments` is upgraded to deny-by-default and will be reported in dependencies too.](https://redirect.github.com/rust-lang/rust/pull/143857) * [Update the minimum external LLVM to 20](https://redirect.github.com/rust-lang/rust/pull/145071) * [Prevent downstream `impl DerefMut for Pin<LocalType>`](https://redirect.github.com/rust-lang/rust/pull/145608) * [Don't apply temporary lifetime extension rules to the arguments of non-extended `pin!` and formatting macros](https://redirect.github.com/rust-lang/rust/pull/145838) ### [`v1.91.1`](https://redirect.github.com/rust-lang/rust/blob/HEAD/RELEASES.md#Version-1911-2025-11-10) [Compare Source](https://redirect.github.com/rust-lang/rust/compare/1.91.0...1.91.1) \=========================== <a id="1.91.1"></a> - [Enable file locking support in illumos](https://redirect.github.com/rust-lang/rust/pull/148322). This fixes Cargo not locking the build directory on illumos. - [Fix `wasm_import_module` attribute cross-crate](https://redirect.github.com/rust-lang/rust/pull/148363). This fixes linker errors on WASM targets. </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/toeverything/AFFiNE). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS45Ny4xMCIsInVwZGF0ZWRJblZlciI6IjQyLjQyLjIiLCJ0YXJnZXRCcmFuY2giOiJjYW5hcnkiLCJsYWJlbHMiOlsiZGVwZW5kZW5jaWVzIl19--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
In electron v36, all workers do not work. The webpack configuration is too complicated, so go back first. If start a new project with [forge](https://www.electronforge.io/) and latest electron, the worker works well. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Downgraded the Electron development/runtime used for building and testing the desktop app from v36 to v35; this is a development-environment change with no functional or API changes affecting end users. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
I used [pdfmake](https://www.npmjs.com/package/pdfmake) to implement an "export as PDF" feature, and I am happy to share with you! This should fix #13577, fix #8846, and fix #13959. A showcase: [Getting Started.pdf](https://github.com/user-attachments/files/24013057/Getting.Started.pdf) Although it might miss rendering some properties currently, it can evolve in the long run and provide a more native experience for the users. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** - Experimental "Export to PDF" option added to the export menu (behind a feature flag) - PDF export supports headings, paragraphs, lists, code blocks, tables, images, callouts, linked documents and embedded content * **Chores** - Added PDF rendering library and consolidated PDF utilities - Feature flag introduced to control rollout * **Tests** - Comprehensive unit tests added for PDF content rendering logic <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: DarkSky <darksky2048@gmail.com>
This feature enhances the /slash command by allowing users to search for 'checkbox' and have the to-do list item show up as a result. Users come from different systems and environments, and some may use the name 'checkbox' but be confused as they cannot find it in the search menu. This is achieved by adding a `searchAlias` property on the to-do list item block that contains the string `checkbox`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added search-alias support for slash menu items so entries can be found by alternative terms. * To-do List entry now includes "checkbox" as an additional searchable alias to improve discoverability. * Slash menu search results updated to reflect alias-driven matches (additional item appears when searching). <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: DarkSky <25152247+darkskygit@users.noreply.github.com>
This is related to issue/feature request #13962. This PR extends the Notion import functionality to properly handle date fields from databases. Previously, these were imported as text (see photo below), which served little purpose. These Notion date fields are now parsed as actual dates, and imported to AFFiNE as epoch time (which is what the date field in AFFiNe expects). Because of this, even date fields with time (e.g. 09:00 AM) are also handled correctly - although they are only shown as dates, since AFFiNE's `Date` field does not support time. Tested with several Notion imports both with and without time, and they all seem to work correctly. Affected files: - blocksuite/affine/blocks/database/src/adapters/notion-html.ts Old: <img width="802" height="305" alt="image" src="https://github.com/user-attachments/assets/44019dba-cffb-4a30-a5ea-69cd9f86e0a1" /> New: <img width="804" height="271" alt="image" src="https://github.com/user-attachments/assets/3f52f328-7ee3-4754-9726-10dcfa0f8462" /> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Enhanced Notion imports with automatic date column detection. When importing Notion databases, date fields are now automatically recognized, properly configured as date columns, and formatted correctly. This improvement ensures accurate data preservation, eliminates manual type corrections, and provides a streamlined import experience for all users working with date-rich Notion databases. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Corrected keyboard shortcut mapping for link function, ensuring it properly recognizes Ctrl+K command. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This PR is related to issue #13290 Keyboard shortcut for copying a private link works as expected, but the overview of shortcuts shows the Mac shortcut for Windows, web and Linux users. This fix shows the correct (Ctrl+Shift+C) shortcut to the aforementioned users. I have not tested this on a Mac (neither in browser nor in the app), but ideally this should not have an impact for Mac users as the logic for showing the correct shortcut is already implemented. Affected files: - packages/frontend/core/src/components/hooks/affine/use-shortcuts.ts Old: <img width="1402" height="946" alt="old_shortcut" src="https://github.com/user-attachments/assets/5c8f2133-2b4d-49c7-8054-851c7de8f3cd" /> New: <img width="650" height="379" alt="Keyboard shortcut fix" src="https://github.com/user-attachments/assets/a29e2f7a-53d7-4743-a9b1-aa30e7622dd1" /> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Corrected the keyboard shortcut for copying private links on Windows from Command+Shift+C to Ctrl+Shift+C. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds comprehensive PDF export functionality via PdfAdapter with document rendering and asset handling; significantly expands data-view grouping with date-based modes, group visibility control, and enhanced menu configuration including placement and close-on-select options; removes Changes
Sequence DiagramsequenceDiagram
participant User as User Action
participant UI as Export UI
participant Transform as PdfTransformer
participant Adapter as PdfAdapter
participant Native as Native Parser
participant PDF as pdfmake
participant Download as File Download
User->>UI: Trigger PDF Export
UI->>Transform: exportDoc(doc)
Transform->>Transform: Build Middleware Pipeline
Transform->>Transform: Convert doc to snapshot
alt Snapshot Present
Transform->>Adapter: new PdfAdapter()
Adapter->>Adapter: Validate snapshot
Adapter->>Native: Parse binary doc
Native-->>Adapter: Block tree + content
Adapter->>Adapter: Render blocks to pdfmake content
Adapter->>Adapter: Build styles + fonts
Adapter->>PDF: getDocDefinition()
PDF-->>Adapter: Document definition
Adapter->>PDF: pdfmake.createPdf()
PDF-->>Adapter: PDF blob
Adapter-->>Transform: PdfAdapterFile
Transform->>Download: Trigger file download
Download->>User: PDF file saved
else No Snapshot
Transform->>User: Early exit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
wellcode-ai
bot
added
review-effort-4
🔍 General Code Quality Feedback🔍 Comprehensive Code ReviewConsolidated Feedback
Overall Assessment: The pull request introduces significant changes across multiple files, including new functionalities and optimizations. However, the lack of clear documentation and potential performance concerns necessitate careful review before merging. Critical Issues:
Improvements:
Positive Notes:
Next Steps:
By addressing these issues and suggestions, the code quality and maintainability of the project will significantly improve, reducing the risk of bugs and enhancing team productivity. 🤖 Generated by Wellcode.ai |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This is a substantial PR with 107 changed files that appears to be syncing upstream changes from the AFFiNE project. While the changes include many new features and improvements, there are several critical issues that need to be addressed before merging:
Critical Issues Found
- Type Safety Violations: Multiple instances of unsafe type assertions (
as any,as never) that bypass TypeScript's type checking - Async Operation Without Error Handling: Clipboard operations lack proper error handling
- Breaking UI Changes: Hardcoded menu dimensions that could break existing layouts
- Import Path Changes: Moving imports between packages without verification could break builds
- API Breaking Changes: Function signature modifications that could affect existing callers
Recommendations
- Fix Type Safety: Remove or properly handle all
as anyandas nevertype assertions - Add Error Handling: Implement proper error handling for all async operations
- Make UI Changes Configurable: Use CSS custom properties instead of hardcoded values
- Verify Import Changes: Ensure all moved imports exist and function identically in their new locations
- Test Thoroughly: Given the scope of changes, comprehensive testing is essential
The PR contains valuable functionality but needs these critical issues resolved to ensure code quality and prevent runtime errors.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| name: 'Paste', | ||
| prefix: PasteIcon(), | ||
| select: () => { | ||
| // eslint-disable-next-line @typescript-eslint/no-floating-promises |
There was a problem hiding this comment.
🛑 Async Operation Issue: Adding an eslint-disable comment for no-floating-promises without proper error handling is a code smell. The clipboard.readText() promise should have proper error handling to prevent silent failures.
| // eslint-disable-next-line @typescript-eslint/no-floating-promises | |
| // Handle clipboard read with proper error handling | |
| navigator.clipboard.readText().then(text => { | |
| this.selectionController.doPaste(text, selected); | |
| }).catch(error => { | |
| console.error('Failed to read clipboard:', error); | |
| }); |
| { insert: 123 as any }, // Invalid type | ||
| { insert: 'Valid text' }, | ||
| ], |
There was a problem hiding this comment.
🛑 Type Safety Issue: Using as any to force an invalid type (number 123 as string) in test data creates unreliable test conditions. This could mask real type errors in production code.
| { insert: 123 as any }, // Invalid type | |
| { insert: 'Valid text' }, | |
| ], | |
| delta: [ | |
| { insert: 'Valid text' }, // Remove invalid test case | |
| ], |
| .group-sort-setting::-webkit-scrollbar { | ||
| width: 8px; | ||
| } | ||
|
|
||
| .group-sort-setting::-webkit-scrollbar-thumb { | ||
| background-color: #b0b0b0; /* Grey slider */ | ||
| border-radius: 4px; | ||
| } | ||
|
|
||
| .group-sort-setting::-webkit-scrollbar-track { | ||
| background: transparent; | ||
| } | ||
|
|
||
| .group-sort-setting { | ||
| scrollbar-width: thin; | ||
| scrollbar-color: #b0b0b0 transparent; | ||
| } |
There was a problem hiding this comment.
🛑 Hardcoded Color Values: Using hardcoded hex color values (#b0b0b0) instead of CSS custom properties breaks theming consistency and accessibility. This could cause issues in dark mode or custom themes.
| .group-sort-setting::-webkit-scrollbar { | |
| width: 8px; | |
| } | |
| .group-sort-setting::-webkit-scrollbar-thumb { | |
| background-color: #b0b0b0; /* Grey slider */ | |
| border-radius: 4px; | |
| } | |
| .group-sort-setting::-webkit-scrollbar-track { | |
| background: transparent; | |
| } | |
| .group-sort-setting { | |
| scrollbar-width: thin; | |
| scrollbar-color: #b0b0b0 transparent; | |
| } | |
| /* WebKit-based browser scrollbar styling */ | |
| .group-sort-setting::-webkit-scrollbar { | |
| width: 8px; | |
| } | |
| .group-sort-setting::-webkit-scrollbar-thumb { | |
| background-color: var(--affine-border-color); /* Use theme variable */ | |
| border-radius: 4px; | |
| } | |
| .group-sort-setting::-webkit-scrollbar-track { | |
| background: transparent; | |
| } | |
| .group-sort-setting { | |
| scrollbar-width: thin; | |
| scrollbar-color: var(--affine-border-color) transparent; /* Use theme variable */ | |
| } |
| min-width: 320px; | ||
| max-width: 320px; | ||
| max-height: 700px; |
There was a problem hiding this comment.
🛑 Breaking UI Change: Changing min-width from 180px to 320px and adding max-width/max-height constraints is a significant UI change that could break existing layouts and user expectations. This should be configurable rather than hardcoded.
| min-width: 320px; | |
| max-width: 320px; | |
| max-height: 700px; | |
| min-width: var(--affine-menu-min-width, 180px); | |
| max-width: var(--affine-menu-max-width, 320px); | |
| max-height: var(--affine-menu-max-height, 700px); |
| // Test | ||
| import type { GroupByConfig } from './types.js'; |
There was a problem hiding this comment.
Remove the stray "// Test" comment that appears to be leftover debugging code.
| // Test | |
| import type { GroupByConfig } from './types.js'; | |
| import { findGroupByConfigByName, getGroupByService } from './matcher.js'; | |
| import type { GroupByConfig } from './types.js'; |
| ): GroupByConfig => { | ||
| return config as never as GroupByConfig; | ||
| }; | ||
| ): GroupByConfig => config as never; |
There was a problem hiding this comment.
🛑 Type Safety Issue: The type assertion as never bypasses TypeScript's type checking completely. This removes all type safety guarantees and could lead to runtime errors.
| ): GroupByConfig => config as never; | |
| ): GroupByConfig<Data, MatchType, GroupValue> { | |
| return config; |
| @@ -1,4 +1,5 @@ | |||
| import type { ListBlockModel } from '@blocksuite/affine-model'; | |||
| import { getNumberPrefix } from '@blocksuite/affine-shared/utils'; | |||
There was a problem hiding this comment.
🛑 Import Path Issue: Moving the import from a local file to a shared utils package could break the build if the function doesn't exist in the target location or has different behavior. Verify that getNumberPrefix exists and works identically in @blocksuite/affine-shared/utils.
| const { name, description, icon, flavour, type, searchAlias = [] } = config; | ||
| return { | ||
| name, | ||
| group, | ||
| description, | ||
| icon, | ||
| searchAlias, |
There was a problem hiding this comment.
🛑 Breaking API Change: Adding searchAlias = [] as a default parameter and using it in the return object changes the function signature. This could break existing callers that don't provide this parameter.
| const { name, description, icon, flavour, type, searchAlias = [] } = config; | |
| return { | |
| name, | |
| group, | |
| description, | |
| icon, | |
| searchAlias, | |
| const { name, description, icon, flavour, type, searchAlias } = config; | |
| return { | |
| name, | |
| group, | |
| description, | |
| icon, | |
| searchAlias: searchAlias || [], |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
blocksuite/affine/blocks/table/src/table-cell.ts (1)
417-426: Add error handling for clipboard read operation.The clipboard read operation lacks error handling, which can lead to silent failures when clipboard access is denied, the API is unavailable, or other errors occur. Users won't receive feedback if the paste operation fails.
This is inconsistent with the error handling pattern used elsewhere in the file (e.g., line 725 uses
.catch(console.error)).🔎 Apply this diff to add proper error handling:
menu.action({ name: 'Paste', prefix: PasteIcon(), select: () => { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - navigator.clipboard.readText().then(text => { - this.selectionController.doPaste(text, selected); - }); + navigator.clipboard + .readText() + .then(text => { + this.selectionController.doPaste(text, selected); + }) + .catch(err => { + console.error('Failed to read clipboard:', err); + }); }, }),packages/frontend/media-capture-playground/web/components/saved-recording-item.tsx (1)
727-736: Handle the play() promise rejection.The
play()method returns a Promise that can reject due to browser autoplay policies, media loading errors, or codec issues. Silently ignoring this promise means users won't receive feedback when playback fails.🔎 Apply this diff to handle playback errors:
const handlePlayPause = React.useCallback(() => { if (audioRef.current) { if (audioRef.current.paused) { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - void audioRef.current.play(); + audioRef.current.play().catch((err) => { + console.error('Playback failed:', err); + setError('Failed to start playback. Please try again.'); + }); } else { audioRef.current.pause(); } } }, []);blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts (1)
469-485: Menu stays open after Duplicate/Delete - verify intended behavior.Both actions have
closeOnSelect: falsebut don't open submenus or close the menu explicitly. Afterview.delete(), the menu may reference a deleted view, potentially causing stale state or errors.If the intent is to keep the menu open, ensure the menu state remains valid. Otherwise, either remove
closeOnSelect: false(to auto-close) or callhandler.close()after the action.🔎 Suggested fix if menu should close after action
menu.action({ name: 'Duplicate', prefix: DuplicateIcon(), - closeOnSelect: false, select: () => { view.duplicate(); }, }), menu.action({ name: 'Delete', prefix: DeleteIcon(), - closeOnSelect: false, select: () => { view.delete(); }, class: { 'delete-item': true }, }),packages/frontend/core/src/desktop/dialogs/setting/workspace-setting/integration/mcp-server/setting-panel.tsx (1)
159-166: Handle the clipboard promise instead of suppressing the lint warning.The success notification displays before the clipboard write completes. If the operation fails (permissions, security context, browser limitations), users still see "Copied to clipboard", which is misleading.
🔎 Apply this diff to properly handle the clipboard operation:
<Button variant="primary" - onClick={() => { + onClick={async () => { if (!code) return; - // eslint-disable-next-line @typescript-eslint/no-floating-promises - navigator.clipboard.writeText(code); - notify.success({ - title: t['Copied to clipboard'](), - }); + try { + await navigator.clipboard.writeText(code); + notify.success({ + title: t['Copied to clipboard'](), + }); + } catch (err) { + notify.error({ + error: UserFriendlyError.fromAny(err), + }); + } }} disabled={!code || mutating} >blocksuite/affine/data-view/src/core/group-by/trait.ts (1)
305-342: Critical: Duplicate code inaddToGroupmethod.The method contains two separate implementations of the same logic. Lines 305-324 appear to be the old implementation, and lines 325-341 are the new implementation. This is likely a merge error that will cause the operation to run twice, potentially corrupting data.
🔎 Remove the duplicate old implementation:
addToGroup(rowId: string, key: string) { - const groupMap = this.groupDataMap$.value; - const groupInfo = this.groupInfo$.value; - if (!groupMap || !groupInfo) { - return; - } - const addTo = groupInfo.config.addToGroup; - if (addTo === false) { - return; - } - const v = groupMap[key]?.value; - if (v != null) { - const newValue = addTo( - v, - this.view.cellGetOrCreate(rowId, groupInfo.property.id).jsonValue$.value - ); - this.view - .cellGetOrCreate(rowId, groupInfo.property.id) - .valueSet(newValue); - } const map = this.groupDataMap$.value; const info = this.groupInfo$.value; if (!map || !info) return; const addFn = info.config.addToGroup; if (addFn === false) return; const group = map[key]; if (!group) return; const current = group.value; // Handle both null and non-null values to ensure proper group assignment const newVal = addFn( current, this.view.cellGetOrCreate(rowId, info.property.id).jsonValue$.value ); this.view.cellGetOrCreate(rowId, info.property.id).valueSet(newVal); }
🧹 Nitpick comments (36)
packages/frontend/core/src/blocksuite/ai/components/playground/content.ts (1)
283-307: Eslint suppressions are justified but consider simplifying the async pattern.The
@typescript-eslint/no-misused-promisessuppressions at lines 300 and 304 are appropriate here: the async handler intentionally uses fire-and-forget behavior, with state management (isSendingflag) and cleanup protection (try/finally) preventing race conditions. This is a defensible pattern for event listeners.However, the
triggerOtherSendButtonsfunction creates async functions in the map that don't actually await anything—the async keyword wraps synchronous event dispatch. Consider simplifying:// Current: async functions with no awaits in the body const clickPromises = otherButtons.map(async button => { try { button.dispatchEvent(new MouseEvent(...)); } catch (error) { console.error(error); } }); await Promise.allSettled(clickPromises); // Simpler: dispatch synchronously without the async wrapper otherButtons.forEach(button => { try { button.dispatchEvent(new MouseEvent(...)); } catch (error) { console.error(error); } });This eliminates unnecessary Promise wrapping while preserving the error handling.
packages/frontend/media-capture-playground/web/components/saved-recording-item.tsx (1)
378-378: Remove unused parameter.The
transcriptionErrorparameter is declared but never used in theRecordingHeadercomponent body.🔎 Apply this diff to remove the unused parameter:
handleDeleteClick: () => void; - transcriptionError: string | null; }): ReactElement {Also remove it from the call site:
<RecordingHeader metadata={metadata} fileName={fileName} recordingDate={recordingDate} duration={duration} error={error} isDeleting={isDeleting} showDeleteConfirm={showDeleteConfirm} setShowDeleteConfirm={setShowDeleteConfirm} handleDeleteClick={handleDeleteClick} - transcriptionError={transcriptionError} />tests/kit/src/utils/keyboard.ts (1)
117-118: Question the necessity of the clipboard clear and potential race condition.The
navigator.clipboard.writeText('')call is not awaited, creating a potential race condition where the paste event (line 119-127) might be dispatched before the clipboard is fully cleared. More importantly, since this function creates a syntheticClipboardEventwith its ownDataTransferobject (lines 119-126) that doesn't read from the actual clipboard, this clear operation may be entirely unnecessary.Consider one of these options:
- Remove the call entirely (preferred if testing shows it's unnecessary)
- Await the promise to eliminate the race condition
- Document why the floating promise is safe if there's a specific reason
🔎 Option 1: Remove the unnecessary clipboard clear
- // eslint-disable-next-line @typescript-eslint/no-floating-promises - navigator.clipboard.writeText(''); const e = new ClipboardEvent('paste', {🔎 Option 2: Await the promise to eliminate race condition
- // eslint-disable-next-line @typescript-eslint/no-floating-promises - navigator.clipboard.writeText(''); + await navigator.clipboard.writeText(''); const e = new ClipboardEvent('paste', {tests/blocksuite/e2e/slash-menu.spec.ts (1)
580-596: Consider adding explicit test coverage for searchAlias.The updated counts reflect the To-do List appearing in search results via the
'checkbox'alias. However, the test doesn't explicitly verify the searchAlias functionality.💡 Consider adding a dedicated test case
Add a test case that explicitly validates searchAlias behavior, such as:
test('should find items via searchAlias', async ({ page }) => { await enterPlaygroundRoom(page); await initEmptyParagraphState(page); await focusRichText(page); const slashMenu = page.locator(`.slash-menu`); const slashItems = slashMenu.locator('icon-button'); await type(page, '/checkbox'); await expect(slashItems).toHaveCount(1); await expect(slashItems.nth(0).locator('.text')).toHaveText(['To-do List']); });This would provide stronger validation that the searchAlias feature works as intended.
blocksuite/affine/blocks/database/src/adapters/notion-html.ts (1)
169-198: Consider caching thequerySelectorresult and clarifying the fallback behavior.Two observations:
Redundant query:
HastUtils.querySelector(child, 'time')is called twice (lines 171 and 172). Store the result in a variable once.Inconsistent fallback: When date parsing fails, the fallback (line 195) uses
HastUtils.getTextContent(child)(full cell content) rather than the already-extractedrawColumnData(time element content). If this is intentional, a brief comment would clarify the reasoning.🔎 Suggested refactor:
- // Check for <time> element to find date field from Notion. - if (HastUtils.querySelector(child, 'time')) { - const timeElement = HastUtils.querySelector(child, 'time'); - let rawColumnData = - HastUtils.getTextContent(timeElement).trim(); + // Check for <time> element to find date field from Notion. + const timeElement = HastUtils.querySelector(child, 'time'); + if (timeElement) { + let rawColumnData = + HastUtils.getTextContent(timeElement).trim(); if (rawColumnData.startsWith('@')) { rawColumnData = rawColumnData.slice(1); } const columnDate = new Date(rawColumnData); const timestamp = columnDate.getTime(); if (!Number.isNaN(timestamp)) { column.data = {}; if (column.type !== 'date') { column.type = 'date'; } row[column.id] = { columnId: column.id, value: timestamp, }; } else { + // Fallback to full cell content when date parsing fails row[column.id] = { columnId: column.id, value: HastUtils.getTextContent(child), }; } } else if (HastUtils.querySelector(child, '.selected-value')) {blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts (1)
123-127: Extract repeated middleware configuration to a constant.The same middleware configuration is duplicated ~10 times throughout the file. Consider extracting it to improve maintainability.
🔎 Suggested refactor
Add a constant near the top of the file (after imports):
const SUBMENU_PLACEMENT_MIDDLEWARE = [ autoPlacement({ allowedPlacements: ['bottom-start', 'top-start'] }), offset({ mainAxis: 15, crossAxis: -162 }), shift({ crossAxis: true }), ];Then replace all occurrences, e.g.:
popPropertiesSetting( target, { view: view, onBack: reopen, onClose: closeMenu, }, - [ - autoPlacement({ allowedPlacements: ['bottom-start', 'top-start'] }), - offset({ mainAxis: 15, crossAxis: -162 }), - shift({ crossAxis: true }), - ] + SUBMENU_PLACEMENT_MIDDLEWARE );packages/frontend/component/src/ui/icon-name-editor/icon-name-editor.css.ts (1)
22-24: Note:display: contentshas accessibility considerations.The
display: contentsproperty causes the element to be replaced by its children in the layout tree. While generally safe in this context, be aware that it can affect screen reader navigation and ARIA semantics. Ensure this doesn't impact keyboard navigation or accessibility features of the icon button.Consider testing with a screen reader to verify the button remains accessible and properly announced with its icon content.
packages/common/native/src/doc_parser.rs (1)
760-783: Consider optimizing multi-select lookup for large option sets.The current O(n*m) nested loop (iterating all IDs × all options) is acceptable for typical small option sets but could become slow with many selected values and options. For now this is fine, but consider building a HashMap lookup if performance becomes a concern.
packages/backend/server/src/core/utils/blocksuite.ts (1)
171-188: Consider removing unnecessaryasynckeyword.
parseYDocFromBinaryis a synchronous function (returnsNativeCrawlResultdirectly, not a Promise), making theasyncdeclaration unnecessary. While the current implementation works correctly (returns a resolved Promise), removingasyncwould be more accurate.🔎 Suggested change
-export async function readAllBlocksFromDocSnapshot( +export function readAllBlocksFromDocSnapshot( docId: string, docSnapshot: Uint8Array ) {Note: This would require updating callers to remove
await, or you can keepasyncfor API stability if callers already depend on it.blocksuite/affine/data-view/src/view-presets/kanban/kanban-view-manager.ts (1)
142-168: Simplify the unnecessary defensive check.Line 148 checks
if (!target)after accessinglist[idx]whereidx >= 0. Sinceidxis known to be a valid index (fromfindIndexreturning>= 0),list[idx]cannot be falsy. This defensive check adds unnecessary complexity.🔎 Simplify the logic:
const list = [...(this.view?.groupProperties ?? [])]; const idx = list.findIndex(g => g.key === key); if (idx >= 0) { const target = list[idx]; - if (!target) { - return { groupProperties: list }; - } list[idx] = { ...target, hide }; } else {blocksuite/affine/data-view/src/view-presets/table/pc-virtual/table-view-ui-logic.ts (1)
98-108: LGTM! Consistent null-safety pattern.The null filtering matches the approach in the mobile table view, providing consistent safety across platforms.
Consider extracting the null filtering logic to a shared utility function since this pattern appears in multiple table view implementations.
blocksuite/affine/data-view/src/__tests__/compare-date-keys.unit.spec.ts (1)
1-35: Good test coverage with a minor suggestion for completeness.The test suite covers the main scenarios (relative keys, numeric keys, and mixed keys) for both ascending and descending order.
Consider testing the full sorted order for mixed keys
Line 32-33 only verify the first and last elements of the mixed key sort. Consider asserting the complete sorted order to catch any middle-element ordering issues:
it('handles mixed relative and numeric keys', () => { const cmp = compareDateKeys('date-relative', true); const keys = ['today', '1', 'yesterday', '2']; const sorted = [...keys].sort(cmp); - expect(sorted[0]).toBe('1'); - expect(sorted[sorted.length - 1]).toBe('today'); + expect(sorted).toEqual(['1', '2', 'yesterday', 'today']); });blocksuite/affine/data-view/src/core/group-by/matcher.ts (1)
12-22: LGTM! Clean lookup implementation.The function correctly combines external and local group-by configurations and returns the first match by name. The return type properly indicates the possibility of no match.
Optional: Add JSDoc for clarity
Consider adding documentation to clarify the lookup behavior:
+/** + * Finds a GroupByConfig by name from both external and local matchers. + * @param dataSource - The data source to query for external configs + * @param name - The name of the config to find + * @returns The first matching config, or undefined if not found + */ export const findGroupByConfigByName = ( dataSource: DataSource, name: string ): GroupByConfig | undefined => {blocksuite/affine/data-view/src/view-presets/table/pc/table-view-ui-logic.ts (1)
158-181: LGTM! Defensive group rendering with proper type narrowing.The refactored rendering logic correctly filters out undefined groups and provides appropriate fallbacks for both empty groups and the no-groups case.
Minor note: Line 159 uses
g !== undefinedwhile other files in this PR (group-header.ts, group-footer.ts, boolean-group.ts) use!= nullto catch both null and undefined. Consider using!= nullfor consistency:- const groups = this.logic.view.groupTrait.groupsDataList$.value?.filter( - (g): g is Group => g !== undefined - ); + const groups = this.logic.view.groupTrait.groupsDataList$.value?.filter( + (g): g is Group => g != null + );blocksuite/affine/data-view/src/core/group-by/renderer/date-group.ts (1)
24-35: Unused counter styles.Lines 24-35 define
.counterstyles that are not referenced in the render method. This appears to be dead code.Remove unused styles or add TODO if planned for future use
If the counter badge is planned for future use, consider adding a comment:
background-color: var(--affine-hover-color); } + /* TODO: Counter badge for future use */ .counter { flex-shrink: 0;Otherwise, remove the unused styles to keep the codebase clean.
blocksuite/affine/data-view/src/view-presets/table/table-view-manager.ts (1)
169-192: Unreachable defensive check on Line 175.The check
if (!target)at Line 175 is unreachable becauseidx >= 0fromfindIndexguaranteeslist[idx]exists. While this is harmless, it adds unnecessary code.🔎 Simplify by removing the redundant check:
changeGroupHide: (key, hide) => { this.dataUpdate(() => { const list = [...this.groupProperties]; const idx = list.findIndex(g => g.key === key); if (idx >= 0) { const target = list[idx]; - if (!target) { - return { groupProperties: list }; - } list[idx] = { ...target, hide }; } else {blocksuite/affine/data-view/src/core/group-by/trait.ts (1)
22-22: Remove debug comment.The
// Testcomment appears to be a leftover debug artifact.🔎 Remove the stray comment:
import { findGroupByConfigByName, getGroupByService } from './matcher.js'; -// Test import type { GroupByConfig } from './types.js';blocksuite/affine/data-view/src/view-presets/kanban/pc/kanban-view-ui-logic.ts (2)
233-239: Redundant null check after filtering.After applying the filter with type guard on lines 233-238,
groupsis guaranteed to be a non-null array (either populated or empty). The check!groupson line 239 is redundant—onlygroups.length === 0is needed.🔎 Simplify the empty check:
- if (!groups || groups.length === 0) { + if (groups.length === 0) { return html``; }
199-217: Note: Groups are filtered twice.Groups are filtered in both
render()(lines 233-238) andrenderGroups()(lines 205-207). Sincerender()already produces a filtered array, the filtering inrenderGroups()is redundant.Consider passing the filtered
groupsarray fromrender()torenderGroups()to avoid duplicate filtering:🔎 Refactor to eliminate duplicate filtering:
- private renderGroups() { - const groups = this.logic.groups$.value; - if (!groups) { - return html``; - } - - const safeGroups = groups.filter( - (group): group is NonNullable<(typeof groups)[number]> => group != null - ); - - return html`${safeGroups.map(group => { + private renderGroups(groups: NonNullable<ReturnType<typeof this.logic.groups$.value>>[number][]) { + return html`${groups.map(group => { return html` <affine-data-view-kanban-group ${sortable(group.key)} data-key="${group.key}" .kanbanViewLogic="${this.logic}" .group="${group}" ></affine-data-view-kanban-group>`; })}`; }Then update the call site in
render():- ${this.renderGroups()} + ${this.renderGroups(groups)}blocksuite/affine/data-view/src/view-presets/kanban/mobile/menu.ts (1)
53-67: Unnecessary nullish coalescing after map.The
?? []on line 67 is unnecessary because.map()always returns an array, never null or undefined. The previous inline chain may have needed it, but with the precomputedgroupsarray, it's redundant.🔎 Remove unnecessary fallback:
groups .filter(v => v.key !== groupKey) .map(group => menu.action({ name: group.value != null ? group.name$.value : 'Ungroup', select: () => { groupTrait.moveCardTo( cardId, groupKey, group.key, 'start' ); }, }) - ) ?? [], + ),blocksuite/affine/data-view/src/view-presets/kanban/pc/menu.ts (1)
65-72: Unnecessary nullish coalescing after map.Similar to the mobile version, the
?? []on line 72 is unnecessary since.map()always returns an array.🔎 Remove unnecessary fallback:
.map(group => menu.action({ name: group.value != null ? group.name$.value : 'Ungroup', select: () => { selection.moveCard(rowId, group.key); }, }) - ) ?? [], + ),blocksuite/affine/data-view/src/core/group-by/setting.ts (1)
319-330: Consider extracting the magic number for menu height.The hardcoded
'550px'minHeight appears multiple times (lines 329, 399, 635). Consider defining this as a constant for maintainability.🔎 View suggested refactor:
At the top of the file, add:
+const MIN_MENU_HEIGHT = '550px';Then replace the hardcoded values:
- handler.menu.menuElement.style.minHeight = '550px'; + handler.menu.menuElement.style.minHeight = MIN_MENU_HEIGHT;Apply similar changes at lines 399 and 635.
packages/frontend/core/src/components/hooks/affine/use-shortcuts.ts (1)
240-240: Good fix for the typo.The typo
Ctr→Ctrlis correctly fixed. However, note there's a similar minor issue on line 238 whereinlineCodehas a leading space:[' Ctrl', 'E'].🔎 Consider fixing the leading space on line 238:
- [t('inlineCode')]: [' Ctrl', 'E'], + [t('inlineCode')]: ['Ctrl', 'E'],blocksuite/affine/data-view/src/core/filter/add-filter.ts (1)
27-67: LGTM - Pattern is consistent with other popup handlers.The approach of capturing the menu handler and setting
minHeightaligns with similar patterns used elsewhere in the codebase (e.g.,add-sort.ts,root-panel.ts).Consider extracting the magic number
550pxto a shared constant if this value is reused across multiple popup handlers for consistency and easier maintenance.packages/frontend/core/src/bootstrap/cleanup.ts (1)
7-30: Good addition of error handling for the outer promise.The
.catch()handler appropriately logs failures when enumerating databases.Note that individual
deleteDatabasecalls (lines 12, 15, 21, 24) are fire-and-forget and can fail silently. If visibility into deletion failures is desired, consider wrapping them:🔎 Optional: Add error handling for individual deletions
databases.forEach(database => { if (database.name?.endsWith(':server-clock')) { - indexedDB.deleteDatabase(database.name); + indexedDB.deleteDatabase(database.name).catch(() => { + // Deletion failure is non-critical, ignore silently + }); } // ... similar for other conditions });blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/condition-view.ts (2)
133-136: Setting bothminHeightandmaxHeightto'fit-content'is redundant.When both are set to the same value, only one is needed. Consider simplifying to just
height: 'fit-content'or removing one of the constraints.🔎 Suggested simplification:
// allow submenu height and width to adjust to content - subHandler.menu.menuElement.style.minHeight = 'fit-content'; - subHandler.menu.menuElement.style.maxHeight = 'fit-content'; + subHandler.menu.menuElement.style.height = 'fit-content'; subHandler.menu.menuElement.style.minWidth = '200px';
160-163: Same redundancy applies to the main menu sizing.Consider the same simplification here.
🔎 Suggested simplification:
// allow main menu height and width to adjust to calendar size - handler.menu.menuElement.style.minHeight = 'fit-content'; - handler.menu.menuElement.style.maxHeight = 'fit-content'; + handler.menu.menuElement.style.height = 'fit-content'; handler.menu.menuElement.style.minWidth = '200px';blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/root-panel-view.ts (1)
233-237: Consider a safer deep clone approach for filter duplication.
JSON.parse(JSON.stringify(...))works for plain data objects but silently dropsundefined,Date,Map,Set, and function properties. IfFilterobjects are strictly JSON-serializable (which they likely are as filter data), this is acceptable.🔎 Alternative using structuredClone (if targeting modern browsers/Node 17+):
conditions.splice( i + 1, 0, - JSON.parse(JSON.stringify(conditions[i])) + structuredClone(conditions[i]) );blocksuite/affine/widgets/linked-doc/src/transformers/pdf.ts (1)
11-28: Add error handling for robustness.The
exportDocfunction lacks error handling around several operations that could fail:
- Line 12:
doc.providercould be undefined- Line 23-26:
adapter.fromDocSnapshotis async and could throw- Line 27:
downloadcould failConsider wrapping the operations in a try-catch block to handle errors gracefully and provide user feedback when PDF generation fails.
🔎 Suggested enhancement with error handling:
async function exportDoc(doc: Store) { + try { const provider = doc.provider; + if (!provider) { + console.error('Document provider is not available'); + return; + } const job = doc.getTransformer([ docLinkBaseURLMiddleware(doc.workspace.id), titleMiddleware(doc.workspace.meta.docMetas), embedSyncedDocMiddleware('content'), ]); const snapshot = job.docToSnapshot(doc); if (!snapshot) { return; } const adapter = new PdfAdapter(job, provider); const { file } = await adapter.fromDocSnapshot({ snapshot, assets: job.assetsManager, }); download(file.blob, file.fileName); + } catch (error) { + console.error('Failed to export PDF:', error); + // Consider showing user-facing error notification + } }blocksuite/affine/shared/src/adapters/pdf/image-utils.ts (1)
59-87: Note: SVG dimension parsing doesn't handle units.The regex patterns extract numeric values but don't account for CSS units (e.g.,
width="100px",width="10em"). If SVG elements use units,parseFloatwill return only the numeric portion, which may not represent the correct pixel dimensions.If SVG assets are expected to have units, consider enhancing the regex to capture and convert units, or document that only unitless or pixel values are supported.
blocksuite/affine/data-view/src/core/sort/add-sort.ts (1)
58-58: Consider extracting the magic number.The hardcoded
'550px'minHeight could be extracted as a named constant for better maintainability if this value is used elsewhere or has specific meaning.const SORT_MENU_MIN_HEIGHT = '550px'; // ... subHandler.menu.menuElement.style.minHeight = SORT_MENU_MIN_HEIGHT;blocksuite/affine/shared/src/adapters/pdf/utils.ts (1)
50-57: Consider refining array content checking.The function checks
textContent.length > 0for arrays, but doesn't verify whether the array contains meaningful content. An array like['', ' ', '']would pass but contains no real content.If strict validation is needed:
if (typeof textContent === 'string') { return textContent.trim() !== ''; } return textContent.some(item => { const text = typeof item === 'string' ? item : item.text; return text.trim() !== ''; });blocksuite/affine/all/src/__tests__/adapters/pdf.unit.spec.ts (1)
838-843: Test doesn't verify thelineThroughdecoration for not-found pages.This test checks that the text becomes "Page not found" but doesn't assert the decoration. Given the bug in
delta-converter.ts, thelineThroughdecoration won't be applied. Consider adding an assertion once the bug is fixed.🔎 View suggested addition:
if (Array.isArray(textContent.text)) { const refText = textContent.text.find( (t: any) => typeof t === 'object' && t.text === 'Page not found' ); expect(refText).toBeDefined(); + // Once the delta-converter bug is fixed, verify decoration: + // expect(refText.decoration).toContain('lineThrough'); }blocksuite/affine/components/src/context-menu/sub-menu.ts (1)
91-151: Consider extracting duplicated menu styling logic.The code for setting
minHeight,maxHeight, andminWidthonmenu.menuElementis duplicated in three places (lines 108-112, 133-137, and 200-204). This could be extracted into a helper function.🔎 View suggested refactor:
// Helper function to apply menu element styles private applyMenuStyles(menuElement: HTMLElement, autoHeight?: boolean) { if (autoHeight) { menuElement.style.minHeight = 'fit-content'; menuElement.style.maxHeight = 'fit-content'; } menuElement.style.minWidth = '200px'; }Then replace the duplicated code blocks with:
this.applyMenuStyles(menu.menuElement, this.data.autoHeight);blocksuite/affine/shared/src/adapters/pdf/pdf.ts (2)
53-66: External font URLs may cause issues in offline/restricted environments.The pdfMake fonts are configured with absolute CDN URLs (
https://cdn.affine.pro/fonts/...). This will fail in offline environments or networks that block external resources. Consider documenting this requirement or providing a fallback mechanism.Do you want me to suggest a pattern for making fonts configurable or providing fallback behavior?
831-833: Silent catch swallows errors without logging.The empty catch block makes debugging difficult when image processing fails unexpectedly. Consider logging the error for observability.
🔎 Apply this diff to add error logging:
- } catch { + } catch (error) { + console.warn('Failed to process image asset:', sourceId, error); return [this._getImagePlaceholderContent(caption)]; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
packages/backend/server/src/__tests__/e2e/doc-service/__snapshots__/controller.spec.ts.snapis excluded by!**/*.snappackages/backend/server/src/core/doc/__tests__/__snapshots__/reader-from-database.spec.ts.snapis excluded by!**/*.snappackages/backend/server/src/core/doc/__tests__/__snapshots__/reader-from-rpc.spec.ts.snapis excluded by!**/*.snappackages/backend/server/src/core/utils/__tests__/__snapshots__/blocksute.spec.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (102)
.devcontainer/build.sh(0 hunks).github/actions/server-test-env/action.yml(0 hunks).github/workflows/build-images.yml(0 hunks).github/workflows/build-test.yml(0 hunks)blocksuite/affine/all/src/__tests__/adapters/pdf.unit.spec.ts(1 hunks)blocksuite/affine/blocks/callout/src/callout-block.ts(1 hunks)blocksuite/affine/blocks/database/src/adapters/notion-html.ts(2 hunks)blocksuite/affine/blocks/list/src/utils/get-list-icon.ts(1 hunks)blocksuite/affine/blocks/note/src/configs/slash-menu.ts(1 hunks)blocksuite/affine/blocks/table/src/table-cell.ts(1 hunks)blocksuite/affine/components/src/context-menu/button.ts(5 hunks)blocksuite/affine/components/src/context-menu/menu-renderer.ts(7 hunks)blocksuite/affine/components/src/context-menu/sub-menu.ts(7 hunks)blocksuite/affine/data-view/src/__tests__/compare-date-keys.unit.spec.ts(1 hunks)blocksuite/affine/data-view/src/core/common/properties.ts(3 hunks)blocksuite/affine/data-view/src/core/common/types.ts(1 hunks)blocksuite/affine/data-view/src/core/filter/add-filter.ts(2 hunks)blocksuite/affine/data-view/src/core/filter/literal/define.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/compare-date-keys.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/default.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/define.ts(2 hunks)blocksuite/affine/data-view/src/core/group-by/matcher.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/renderer/boolean-group.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/renderer/date-group.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/renderer/number-group.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/renderer/select-group.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/renderer/string-group.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/setting.ts(9 hunks)blocksuite/affine/data-view/src/core/group-by/trait.ts(6 hunks)blocksuite/affine/data-view/src/core/sort/add-sort.ts(3 hunks)blocksuite/affine/data-view/src/core/view-manager/single-view.ts(1 hunks)blocksuite/affine/data-view/src/property-presets/date/cell-renderer-css.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/kanban/kanban-view-manager.ts(2 hunks)blocksuite/affine/data-view/src/view-presets/kanban/mobile/kanban-view-ui-logic.ts(2 hunks)blocksuite/affine/data-view/src/view-presets/kanban/mobile/menu.ts(3 hunks)blocksuite/affine/data-view/src/view-presets/kanban/pc/kanban-view-ui-logic.ts(2 hunks)blocksuite/affine/data-view/src/view-presets/kanban/pc/menu.ts(2 hunks)blocksuite/affine/data-view/src/view-presets/table/mobile/table-view-ui-logic.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/table/pc-virtual/group/bottom/group-footer.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/table/pc-virtual/group/top/group-header.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/table/pc-virtual/table-view-ui-logic.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/table/pc/table-view-style.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/table/pc/table-view-ui-logic.ts(4 hunks)blocksuite/affine/data-view/src/view-presets/table/table-view-manager.ts(2 hunks)blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/condition-view.ts(5 hunks)blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/root-panel-view.ts(5 hunks)blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/sort/root-panel.ts(3 hunks)blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts(12 hunks)blocksuite/affine/rich-text/src/conversion.ts(2 hunks)blocksuite/affine/shared/package.json(2 hunks)blocksuite/affine/shared/src/adapters/index.ts(1 hunks)blocksuite/affine/shared/src/adapters/pdf/css-utils.ts(1 hunks)blocksuite/affine/shared/src/adapters/pdf/delta-converter.ts(1 hunks)blocksuite/affine/shared/src/adapters/pdf/image-utils.ts(1 hunks)blocksuite/affine/shared/src/adapters/pdf/index.ts(1 hunks)blocksuite/affine/shared/src/adapters/pdf/pdf.ts(1 hunks)blocksuite/affine/shared/src/adapters/pdf/svg-utils.ts(1 hunks)blocksuite/affine/shared/src/adapters/pdf/utils.ts(1 hunks)blocksuite/affine/shared/src/services/feature-flag-service.ts(2 hunks)blocksuite/affine/shared/src/utils/index.ts(1 hunks)blocksuite/affine/shared/src/utils/number-prefix.ts(2 hunks)blocksuite/affine/widgets/linked-doc/src/transformers/index.ts(1 hunks)blocksuite/affine/widgets/linked-doc/src/transformers/pdf.ts(1 hunks)docs/developing-server.md(0 hunks)package.json(1 hunks)packages/backend/native/Cargo.toml(1 hunks)packages/backend/native/index.d.ts(1 hunks)packages/backend/native/src/doc.rs(1 hunks)packages/backend/native/src/lib.rs(1 hunks)packages/backend/server/package.json(0 hunks)packages/backend/server/src/__tests__/e2e/doc-service/__snapshots__/controller.spec.ts.md(1 hunks)packages/backend/server/src/core/doc/__tests__/__snapshots__/reader-from-database.spec.ts.md(1 hunks)packages/backend/server/src/core/doc/__tests__/__snapshots__/reader-from-rpc.spec.ts.md(1 hunks)packages/backend/server/src/core/doc/reader.ts(1 hunks)packages/backend/server/src/core/utils/__tests__/__snapshots__/blocksute.spec.ts.md(22 hunks)packages/backend/server/src/core/utils/__tests__/blocksute.spec.ts(3 hunks)packages/backend/server/src/core/utils/blocksuite.ts(2 hunks)packages/backend/server/src/native.ts(1 hunks)packages/backend/server/src/plugins/indexer/service.ts(2 hunks)packages/backend/server/src/plugins/payment/revenuecat/service.ts(1 hunks)packages/backend/server/tsconfig.json(0 hunks)packages/common/native/src/doc_parser.rs(3 hunks)packages/frontend/admin/src/modules/accounts/components/use-user-management.ts(1 hunks)packages/frontend/apps/electron/forge.config.mjs(1 hunks)packages/frontend/apps/electron/package.json(1 hunks)packages/frontend/component/src/ui/icon-name-editor/icon-name-editor.css.ts(1 hunks)packages/frontend/component/src/ui/icon-name-editor/icon-name-editor.tsx(1 hunks)packages/frontend/component/src/ui/icon-picker/picker/emoji/emoji-button.tsx(1 hunks)packages/frontend/core/src/blocksuite/ai/components/playground/content.ts(1 hunks)packages/frontend/core/src/bootstrap/cleanup.ts(1 hunks)packages/frontend/core/src/components/hooks/affine/use-export-page.ts(3 hunks)packages/frontend/core/src/components/hooks/affine/use-shortcuts.ts(2 hunks)packages/frontend/core/src/components/page-list/operation-menu-items/export.tsx(5 hunks)packages/frontend/core/src/desktop/dialogs/import/index.tsx(1 hunks)packages/frontend/core/src/desktop/dialogs/setting/workspace-setting/integration/mcp-server/setting-panel.tsx(1 hunks)packages/frontend/core/src/modules/feature-flag/constant.ts(1 hunks)packages/frontend/media-capture-playground/web/components/saved-recording-item.tsx(1 hunks)rust-toolchain.toml(1 hunks)tests/blocksuite/e2e/database/selection.spec.ts(3 hunks)tests/blocksuite/e2e/slash-menu.spec.ts(3 hunks)tests/kit/src/utils/keyboard.ts(1 hunks)tools/utils/src/workspace.gen.ts(0 hunks)
💤 Files with no reviewable changes (8)
- docs/developing-server.md
- packages/backend/server/package.json
- packages/backend/server/tsconfig.json
- .github/actions/server-test-env/action.yml
- .github/workflows/build-images.yml
- .github/workflows/build-test.yml
- tools/utils/src/workspace.gen.ts
- .devcontainer/build.sh
🧰 Additional context used
🧬 Code graph analysis (28)
blocksuite/affine/data-view/src/__tests__/compare-date-keys.unit.spec.ts (1)
blocksuite/affine/data-view/src/core/group-by/compare-date-keys.ts (1)
compareDateKeys(41-62)
blocksuite/affine/data-view/src/view-presets/kanban/mobile/menu.ts (5)
blocksuite/affine/data-view/src/view-presets/table/mobile/cell.ts (1)
groupKey(98-100)blocksuite/affine/data-view/src/view-presets/table/pc/cell.ts (1)
groupKey(86-88)blocksuite/affine/data-view/src/view-presets/table/pc-virtual/row/cell.ts (1)
groupKey(206-208)blocksuite/affine/data-view/src/view-presets/table/pc/row/row.ts (1)
groupKey(162-164)packages/frontend/core/src/desktop/dialogs/setting/general-setting/editor/style.css.ts (1)
menu(19-24)
packages/frontend/core/src/components/page-list/operation-menu-items/export.tsx (2)
packages/common/infra/src/framework/react/index.tsx (1)
useService(15-17)blocksuite/affine/shared/src/services/feature-flag-service.ts (1)
FeatureFlagService(27-67)
blocksuite/affine/shared/src/adapters/pdf/image-utils.ts (3)
blocksuite/affine/shared/src/adapters/pdf/utils.ts (2)
MAX_PAPER_WIDTH(7-7)MAX_PAPER_HEIGHT(8-8)blocksuite/framework/std/src/gfx/viewport.ts (2)
width(274-276)height(194-196)blocksuite/affine/shared/src/adapters/image.ts (1)
Image(24-24)
blocksuite/affine/widgets/linked-doc/src/transformers/pdf.ts (4)
blocksuite/framework/store/src/model/store/store.ts (1)
Store(195-1298)blocksuite/affine/shared/src/adapters/index.ts (1)
PdfAdapter(64-64)blocksuite/affine/shared/src/adapters/pdf/pdf.ts (1)
PdfAdapter(86-1004)blocksuite/affine/widgets/linked-doc/src/transformers/index.ts (2)
download(6-6)PdfTransformer(5-5)
blocksuite/affine/shared/src/adapters/pdf/svg-utils.ts (1)
blocksuite/affine/shared/src/adapters/pdf/css-utils.ts (1)
resolveCssVariable(4-25)
blocksuite/affine/data-view/src/core/group-by/renderer/date-group.ts (2)
blocksuite/affine/data-view/src/core/group-by/trait.ts (2)
property(49-51)Group(35-76)blocksuite/affine/data-view/src/view-presets/table/table-view-manager.ts (1)
name(215-217)
packages/backend/native/index.d.ts (1)
packages/backend/server/src/native.ts (2)
parseDoc(41-41)readAllDocIdsFromRootDoc(45-46)
packages/backend/server/src/core/doc/reader.ts (1)
packages/backend/server/src/core/utils/blocksuite.ts (1)
parseDocToMarkdownFromDocSnapshot(190-205)
blocksuite/affine/data-view/src/core/common/properties.ts (1)
blocksuite/affine/components/src/context-menu/menu-renderer.ts (1)
popMenu(522-569)
packages/backend/server/src/core/utils/__tests__/blocksute.spec.ts (1)
packages/backend/server/src/core/utils/blocksuite.ts (1)
readAllBlocksFromDocSnapshot(171-188)
blocksuite/affine/data-view/src/core/filter/add-filter.ts (1)
blocksuite/affine/components/src/context-menu/menu-renderer.ts (1)
popMenu(522-569)
packages/backend/server/src/plugins/indexer/service.ts (1)
packages/backend/server/src/core/utils/blocksuite.ts (1)
readAllBlocksFromDocSnapshot(171-188)
blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/sort/root-panel.ts (1)
blocksuite/affine/components/src/context-menu/menu-renderer.ts (1)
popMenu(522-569)
packages/frontend/core/src/components/hooks/affine/use-shortcuts.ts (1)
blocksuite/affine/data-view/src/core/logical/type-presets.ts (1)
t(36-41)
blocksuite/affine/data-view/src/view-presets/table/pc/table-view-ui-logic.ts (2)
blocksuite/affine/data-view/src/core/group-by/trait.ts (1)
Group(35-76)blocksuite/affine/data-view/src/view-presets/table/pc/table-view-style.ts (2)
tableGroupsContainerStyle(62-66)groupsHiddenMessageStyle(95-105)
tests/blocksuite/e2e/database/selection.spec.ts (2)
tests/kit/src/utils/keyboard.ts (2)
pressArrowDown(42-46)pressArrowUp(36-40)tests/blocksuite/e2e/database/actions.ts (1)
assertKanbanCellSelected(446-482)
blocksuite/affine/data-view/src/view-presets/kanban/kanban-view-manager.ts (1)
blocksuite/affine/data-view/src/core/group-by/trait.ts (1)
sortByManually(483-499)
blocksuite/affine/shared/src/adapters/pdf/delta-converter.ts (1)
blocksuite/affine/shared/src/adapters/pdf/css-utils.ts (1)
resolveCssVariable(4-25)
blocksuite/affine/shared/src/adapters/pdf/pdf.ts (6)
blocksuite/affine/shared/src/adapters/pdf/delta-converter.ts (1)
extractTextWithInline(11-122)blocksuite/affine/shared/src/adapters/pdf/utils.ts (4)
PDF_COLORS(11-26)hasTextContent(50-57)TABLE_LAYOUT_NO_BORDERS(31-38)textContentToString(62-71)blocksuite/affine/shared/src/utils/number-prefix.ts (1)
getNumberPrefix(50-53)blocksuite/affine/shared/src/adapters/pdf/svg-utils.ts (2)
getToggleIconSvg(36-42)getBulletIconSvg(10-20)blocksuite/affine/shared/src/adapters/pdf/css-utils.ts (1)
resolveCssVariable(4-25)blocksuite/affine/shared/src/adapters/pdf/image-utils.ts (3)
extractSvgDimensions(59-87)calculateImageDimensions(10-54)extractImageDimensions(92-114)
blocksuite/affine/blocks/database/src/adapters/notion-html.ts (1)
blocksuite/affine/shared/src/adapters/utils/hast.ts (1)
HastUtils(282-295)
blocksuite/affine/all/src/__tests__/adapters/pdf.unit.spec.ts (4)
blocksuite/framework/store/src/model/block/block-model.ts (1)
children(96-98)blocksuite/framework/store/src/transformer/type.ts (2)
BlockSnapshot(6-13)DocSnapshot(44-48)blocksuite/affine/shared/src/adapters/pdf/pdf.ts (1)
PdfAdapter(86-1004)blocksuite/framework/store/src/transformer/assets.ts (1)
AssetsManager(21-105)
blocksuite/affine/components/src/context-menu/sub-menu.ts (2)
blocksuite/affine/components/src/context-menu/button.ts (1)
MenuButton(29-115)blocksuite/affine/components/src/context-menu/menu-renderer.ts (2)
popMenu(522-569)popupTargetFromElement(407-437)
blocksuite/affine/data-view/src/core/group-by/trait.ts (4)
blocksuite/affine/data-view/src/core/common/types.ts (2)
GroupProperty(10-14)GroupBy(1-9)blocksuite/affine/data-view/src/core/group-by/matcher.ts (2)
getGroupByService(44-49)findGroupByConfigByName(12-22)blocksuite/affine/data-view/src/core/group-by/compare-date-keys.ts (1)
compareDateKeys(41-62)blocksuite/affine/data-view/src/core/group-by/default.ts (1)
defaultGroupBy(6-24)
packages/backend/server/src/core/utils/blocksuite.ts (2)
packages/backend/server/src/native.ts (3)
readAllDocIdsFromRootDoc(45-46)parseYDocFromBinary(43-43)parseYDocToMarkdown(44-44)packages/common/reader/src/reader.ts (1)
readAllDocIdsFromRootDoc(893-917)
blocksuite/affine/data-view/src/core/sort/add-sort.ts (1)
blocksuite/affine/components/src/context-menu/menu-renderer.ts (1)
popMenu(522-569)
blocksuite/affine/data-view/src/core/group-by/define.ts (6)
blocksuite/affine/data-view/src/core/group-by/types.ts (1)
GroupByConfig(16-41)blocksuite/affine/data-view/src/core/group-by/renderer/date-group.ts (1)
DateGroupView(8-53)blocksuite/affine/data-view/src/core/group-by/renderer/select-group.ts (1)
SelectGroupView(14-120)blocksuite/affine/data-view/src/core/group-by/renderer/string-group.ts (1)
StringGroupView(10-54)blocksuite/affine/data-view/src/core/group-by/renderer/number-group.ts (1)
NumberGroupView(10-66)blocksuite/affine/data-view/src/core/group-by/renderer/boolean-group.ts (1)
BooleanGroupView(6-33)
packages/frontend/core/src/components/hooks/affine/use-export-page.ts (3)
blocksuite/affine/widgets/linked-doc/src/transformers/index.ts (1)
PdfTransformer(5-5)blocksuite/affine/widgets/linked-doc/src/transformers/pdf.ts (1)
PdfTransformer(30-32)packages/frontend/core/src/modules/pdf/entities/pdf.ts (1)
page(71-83)
| }; | ||
|
|
||
| export interface SingleView { | ||
| data$: any; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace any type with proper signal typing.
The data$ property is typed as any, which removes type safety. Based on the implementation at lines 87-89, this should be typed as ReadonlySignal<ViewData | undefined> to maintain consistency with other observable properties in the interface and provide proper type checking.
🔎 Apply this diff to use proper typing:
- data$: any;
+ readonly data$: ReadonlySignal<ViewData | undefined>;Note: You may need to make the SingleView interface generic over ViewData or use a base type if ViewData is not available in this scope.
📝 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.
| data$: any; | |
| readonly data$: ReadonlySignal<ViewData | undefined>; |
🤖 Prompt for AI Agents
In blocksuite/affine/data-view/src/core/view-manager/single-view.ts around line
23 the data$ property is declared as any which loses type safety; change its
type to ReadonlySignal<ViewData | undefined> (or ReadonlySignal<T | undefined>
by making SingleView generic over ViewData/T or importing/using a base ViewData
type available in this module), update the interface declaration to accept the
generic if necessary, and ensure any references and imports align with the new
type so the signal typing matches the implementation at lines 87-89.
| if (!isPageFound && styleObj.decoration) { | ||
| if (!Array.isArray(styleObj.decoration)) { | ||
| styleObj.decoration = [styleObj.decoration]; | ||
| } | ||
| if (!styleObj.decoration.includes('lineThrough')) { | ||
| styleObj.decoration.push('lineThrough'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: lineThrough decoration not applied when page is not found and no prior decorations exist.
When isPageFound is false, the code only adds lineThrough if styleObj.decoration already exists (from strike/underline). For a linked page reference without other decorations, the not-found state won't be visually indicated.
🔎 Apply this diff to fix the logic:
- if (!isPageFound && styleObj.decoration) {
- if (!Array.isArray(styleObj.decoration)) {
- styleObj.decoration = [styleObj.decoration];
- }
- if (!styleObj.decoration.includes('lineThrough')) {
- styleObj.decoration.push('lineThrough');
- }
+ if (!isPageFound) {
+ if (!styleObj.decoration) {
+ styleObj.decoration = ['lineThrough'];
+ } else {
+ if (!Array.isArray(styleObj.decoration)) {
+ styleObj.decoration = [styleObj.decoration];
+ }
+ if (!styleObj.decoration.includes('lineThrough')) {
+ styleObj.decoration.push('lineThrough');
+ }
+ }
}🤖 Prompt for AI Agents
In blocksuite/affine/shared/src/adapters/pdf/delta-converter.ts around lines 93
to 100, the not-found link decoration is only added when styleObj.decoration
already exists; change the logic to run when !isPageFound regardless of
decoration presence, initialize styleObj.decoration to an array if undefined (or
not an array), then add 'lineThrough' only if it isn't already present so links
without prior decorations get the visual strike-through.
| export function calculateImageDimensions( | ||
| blockWidth: number | undefined, | ||
| blockHeight: number | undefined, | ||
| originalWidth: number | undefined, | ||
| originalHeight: number | undefined | ||
| ): { width?: number; height?: number } { | ||
| let targetWidth = | ||
| blockWidth && blockWidth > 0 | ||
| ? blockWidth | ||
| : originalWidth && originalWidth > 0 | ||
| ? originalWidth | ||
| : undefined; | ||
|
|
||
| let targetHeight = | ||
| blockHeight && blockHeight > 0 | ||
| ? blockHeight | ||
| : originalHeight && originalHeight > 0 | ||
| ? originalHeight | ||
| : undefined; | ||
|
|
||
| if (!targetWidth && !targetHeight) { | ||
| return {}; | ||
| } | ||
|
|
||
| if (targetWidth && targetWidth > MAX_PAPER_WIDTH) { | ||
| const ratio = MAX_PAPER_WIDTH / targetWidth; | ||
| targetWidth = MAX_PAPER_WIDTH; | ||
| if (targetHeight) { | ||
| targetHeight = targetHeight * ratio; | ||
| } | ||
| } | ||
|
|
||
| if (targetHeight && targetHeight > MAX_PAPER_HEIGHT) { | ||
| const ratio = MAX_PAPER_HEIGHT / targetHeight; | ||
| targetHeight = MAX_PAPER_HEIGHT; | ||
| if (targetWidth) { | ||
| targetWidth = targetWidth * ratio; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| width: targetWidth, | ||
| height: targetHeight, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Review dimension constraint logic for edge cases.
The function applies width and height constraints sequentially, which could cause one dimension to exceed its limit after the second constraint is applied. For example, if an image is very wide (1000×500), after width constraint it becomes 550×275, which is correct. However, for a square image (1000×1000), it becomes 550×550, then after height constraint: 800×800, now violating the width constraint.
🔎 Consider applying constraints iteratively or using a single scaling factor:
- if (targetWidth && targetWidth > MAX_PAPER_WIDTH) {
- const ratio = MAX_PAPER_WIDTH / targetWidth;
- targetWidth = MAX_PAPER_WIDTH;
- if (targetHeight) {
- targetHeight = targetHeight * ratio;
- }
- }
-
- if (targetHeight && targetHeight > MAX_PAPER_HEIGHT) {
- const ratio = MAX_PAPER_HEIGHT / targetHeight;
- targetHeight = MAX_PAPER_HEIGHT;
- if (targetWidth) {
- targetWidth = targetWidth * ratio;
- }
- }
+ if (targetWidth && targetHeight) {
+ const widthScale = targetWidth > MAX_PAPER_WIDTH ? MAX_PAPER_WIDTH / targetWidth : 1;
+ const heightScale = targetHeight > MAX_PAPER_HEIGHT ? MAX_PAPER_HEIGHT / targetHeight : 1;
+ const scale = Math.min(widthScale, heightScale);
+ if (scale < 1) {
+ targetWidth = targetWidth * scale;
+ targetHeight = targetHeight * scale;
+ }
+ } else if (targetWidth && targetWidth > MAX_PAPER_WIDTH) {
+ targetWidth = MAX_PAPER_WIDTH;
+ } else if (targetHeight && targetHeight > MAX_PAPER_HEIGHT) {
+ targetHeight = MAX_PAPER_HEIGHT;
+ }🤖 Prompt for AI Agents
blocksuite/affine/shared/src/adapters/pdf/image-utils.ts lines 10-54: The
current sequential constraint logic can re-expand a dimension after applying the
second constraint (e.g., a 1000×1000 image becomes 550×550 then 800×800). Fix by
computing and applying a single uniform scale factor that preserves aspect
ratio: derive initial targetWidth/targetHeight from inputs, if both dimensions
exist compute scale = min(1, MAX_PAPER_WIDTH/targetWidth,
MAX_PAPER_HEIGHT/targetHeight) and multiply both dimensions by that scale; if
only one dimension exists clamp that dimension to its MAX and scale the other
proportionally if present; return the scaled width/height (or undefined if
neither input present).
| if flavour == "affine:table" { | ||
| let contents = gather_table_contents(block); | ||
| markdown.push_str(&contents.join("|")); | ||
| markdown.push('\n'); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Table markdown output may be malformed.
The affine:table handling joins cell contents with | but doesn't generate proper markdown table structure (header row, separator row |---|---|). This differs from the affine:database handling above which properly formats tables. Consider whether this produces the intended output.
🔎 Suggested fix for proper table formatting:
if flavour == "affine:table" {
let contents = gather_table_contents(block);
- markdown.push_str(&contents.join("|"));
- markdown.push('\n');
+ if !contents.is_empty() {
+ markdown.push_str("| ");
+ markdown.push_str(&contents.join(" | "));
+ markdown.push_str(" |\n");
+ }
continue;
}Note: This is a minimal fix. Full table support would require row/column structure from the block data.
📝 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.
| if flavour == "affine:table" { | |
| let contents = gather_table_contents(block); | |
| markdown.push_str(&contents.join("|")); | |
| markdown.push('\n'); | |
| continue; | |
| } | |
| if flavour == "affine:table" { | |
| let contents = gather_table_contents(block); | |
| if !contents.is_empty() { | |
| markdown.push_str("| "); | |
| markdown.push_str(&contents.join(" | ")); | |
| markdown.push_str(" |\n"); | |
| } | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In packages/common/native/src/doc_parser.rs around lines 235 to 240, the
affine:table branch currently joins cell contents with "|" which produces
malformed Markdown tables; change it to build a proper table by (1) treating the
first row returned by gather_table_contents as the header, (2) inserting a
separator row of the same column count using "|---|---|..." between header and
body, (3) rendering every row with leading and trailing pipes and pipes between
cells, and (4) ensuring a final newline; if gather_table_contents can be empty
handle that case gracefully (emit nothing).
| // eslint-disable-next-line @typescript-eslint/no-floating-promises | ||
| navigator.clipboard.writeText(JSON.stringify(dataToCopy, null, 2)); | ||
| callback?.(); |
There was a problem hiding this comment.
Handle the clipboard promise instead of suppressing the lint warning.
The clipboard write operation can fail due to permissions, security context, or browser limitations. Suppressing the lint warning silently ignores these failures, and the callback fires before the operation completes.
🔎 Apply this diff to properly handle the clipboard operation:
- // eslint-disable-next-line @typescript-eslint/no-floating-promises
- navigator.clipboard.writeText(JSON.stringify(dataToCopy, null, 2));
- callback?.();
+ try {
+ await navigator.clipboard.writeText(JSON.stringify(dataToCopy, null, 2));
+ toast('Copied to clipboard');
+ callback?.();
+ } catch (e) {
+ toast.error('Failed to copy to clipboard: ' + (e as Error).message);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
packages/frontend/admin/src/modules/accounts/components/use-user-management.ts
around lines 326 to 328: the clipboard write promise is being ignored and the
callback is invoked before the operation completes; change this to await the
navigator.clipboard.writeText call inside a try/catch (or use .then/.catch) so
the code waits for completion, on success invoke the callback, and on failure
handle the error (log it and/or show a user-facing notification) before
returning; do not suppress the lint warning—handle the promise explicitly and
ensure the callback is only fired after the write resolves or appropriate error
handling runs.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @supervoidcoder. * #2 (comment) The following files were modified: * `blocksuite/affine/blocks/note/src/configs/slash-menu.ts` * `blocksuite/affine/data-view/src/core/group-by/compare-date-keys.ts` * `blocksuite/affine/data-view/src/core/group-by/define.ts` * `blocksuite/affine/data-view/src/core/group-by/trait.ts` * `blocksuite/affine/shared/src/adapters/pdf/css-utils.ts` * `blocksuite/affine/shared/src/adapters/pdf/delta-converter.ts` * `blocksuite/affine/shared/src/adapters/pdf/image-utils.ts` * `blocksuite/affine/shared/src/adapters/pdf/svg-utils.ts` * `blocksuite/affine/shared/src/adapters/pdf/utils.ts` * `blocksuite/affine/shared/src/utils/number-prefix.ts` * `blocksuite/affine/widgets/linked-doc/src/transformers/pdf.ts` * `packages/backend/native/src/doc.rs` * `packages/backend/server/src/core/utils/blocksuite.ts` * `packages/common/native/src/doc_parser.rs` * `packages/frontend/core/src/bootstrap/cleanup.ts` * `packages/frontend/core/src/components/hooks/affine/use-export-page.ts` * `packages/frontend/media-capture-playground/web/components/saved-recording-item.tsx` * `tests/kit/src/utils/keyboard.ts`
6 participants
self exlplanarryteyrukjdfcnmx