fix(editor): get max file size from blob sync engine #11923
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
fundon
force-pushed
the
fundon/04_23_get_maxFileSize_from_blobSync
branch
from
1b084a6 to
e221fc2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## canary #11923 +/- ##
==========================================
- Coverage 55.88% 55.88% -0.01%
==========================================
Files 2588 2594 +6
Lines 115007 115059 +52
Branches 18554 18566 +12
==========================================
+ Hits 64277 64299 +22
+ Misses 49364 48781 -583
- Partials 1366 1979 +613
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:
|
fundon
force-pushed
the
fundon/04_23_get_maxFileSize_from_blobSync
branch
3 times, most recently
from
b995774 to
5c256c8
Compare
fundon
force-pushed
the
fundon/04_23_get_maxFileSize_from_blobSync
branch
3 times, most recently
from
28b2126 to
fe40ed2
Compare
| private readonly onReachedMaxBlobSizeCallbacks: Set< | ||
| (byteSize: number) => void | ||
| > = new Set(); | ||
|
|
||
| maxBlobSize = 1024 * 1024 * 100; // 100MB |
There was a problem hiding this comment.
@EYHN
Does it need to be changed to the default 10MB here?
| @@ -48,6 +48,7 @@ export class Workspace extends Entity { | |||
| }, | |||
| name: 'blob', | |||
| readonly: false, | |||
| maxFileSize: this.engine.blob.maxBlobSize, | |||
There was a problem hiding this comment.
i think this change is not good, because the maxBlobSize value will change
fundon
force-pushed
the
fundon/04_23_get_maxFileSize_from_blobSync
branch
from
fe40ed2 to
862b16b
Compare
WalkthroughThis change removes the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant BlockStdScope
participant BlobSync
participant BlobEngine
participant BlobSource
User->>UI: Uploads file(s)
UI->>BlockStdScope: Handle file(s)
BlockStdScope->>BlobSync: Access maxFileSize
BlobSync->>BlobEngine: Get maxFileSize
BlobEngine->>BlobSource: Get maxFileSize (if defined)
BlobEngine-->>BlobSync: Return maxFileSize
BlobSync-->>BlockStdScope: Return maxFileSize
BlockStdScope->>UI: Validate file size(s)
alt File size exceeded
UI->>User: Show toast "File too large"
else File size OK
UI->>User: Proceed with upload
end
Assessment against linked issues
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
blocksuite/affine/blocks/attachment/src/utils.ts (2)
192-205: Consider extractinghasExceededinto a shared util to avoid duplicationA function with identical behaviour now exists in both
attachment/src/utils.tsandimage/src/utils.ts. Copy-pasting nudges technical-debt upward and increases the likelihood that the two implementations silently diverge in the future (e.g. wording of the toast, unit size precision, etc.).-// local helper -function hasExceeded(std: BlockStdScope, files: File[], maxFileSize = std.store.blobSync.maxFileSize) { ... } +// utils/file-size.ts (proposed new module) +export function hasExceeded( + std: BlockStdScope, + files: File[], + maxFileSize: number = std.store.blobSync.maxFileSize +) { ... }Both
imageandattachmenthelpers could then import the single source of truth.
192-196: Default-parameter referencing a previous argument can be surprisingUsing
maxFileSize = std.store.blobSync.maxFileSizeworks, but the implicit ordering rule (defaults are evaluated left-to-right) is easy to forget, and ESLint/Biome usually flags it in larger codebases.A more explicit pattern is often clearer:
function hasExceeded(std: BlockStdScope, files: File[], maxFileSize?: number) { maxFileSize ??= std.store.blobSync.maxFileSize; ... }This keeps the signature simple and avoids potential foot-guns if the parameter list is ever reordered.
blocksuite/affine/blocks/image/src/utils.ts (2)
312-325: Duplicate implementation – please DRY this helper
hasExceededhere is byte-for-byte the same as the one added toattachment/src/utils.ts. Re-exporting a single helper (e.g. fromaffine-shared/utils/file-size.ts) will:
- Eliminate duplication
- Guarantee consistent wording / behaviour
- Simplify future maintenance (e.g. changing toast style or rounding rule)
450-451: Simplify unnecessary ternary
inTopLeftis already a boolean; the ternary can be replaced with a direct assignment to satisfy Biome’s no-useless-ternary lint and improve readability.-const isMultipleFiles = files.length > 1; -const inTopLeft = isMultipleFiles ? true : false; +const isMultipleFiles = files.length > 1; +const inTopLeft = isMultipleFiles;🧰 Tools
🪛 Biome (1.9.4)
[error] 451-451: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
blocksuite/affine/all/src/extensions/migrating-store.ts(0 hunks)blocksuite/affine/blocks/attachment/src/attachment-block.ts(2 hunks)blocksuite/affine/blocks/attachment/src/embed.ts(1 hunks)blocksuite/affine/blocks/attachment/src/utils.ts(4 hunks)blocksuite/affine/blocks/image/src/commands/insert-images.ts(1 hunks)blocksuite/affine/blocks/image/src/image-service.ts(2 hunks)blocksuite/affine/blocks/image/src/utils.ts(6 hunks)blocksuite/affine/shared/src/services/file-size-limit-service.ts(0 hunks)blocksuite/affine/shared/src/services/index.ts(0 hunks)blocksuite/framework/sync/src/blob/engine.ts(2 hunks)blocksuite/framework/sync/src/blob/source.ts(1 hunks)packages/common/nbstore/src/frontend/blob.ts(1 hunks)packages/frontend/core/src/modules/workspace/entities/workspace.ts(2 hunks)
💤 Files with no reviewable changes (3)
- blocksuite/affine/all/src/extensions/migrating-store.ts
- blocksuite/affine/shared/src/services/index.ts
- blocksuite/affine/shared/src/services/file-size-limit-service.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
blocksuite/affine/blocks/image/src/image-service.ts (1)
blocksuite/affine/blocks/image/src/utils.ts (1)
addSiblingImageBlock(336-364)
blocksuite/affine/blocks/image/src/commands/insert-images.ts (1)
blocksuite/affine/blocks/image/src/utils.ts (1)
addSiblingImageBlock(336-364)
blocksuite/affine/blocks/attachment/src/utils.ts (2)
blocksuite/framework/sync/src/blob/engine.ts (1)
maxFileSize(110-112)blocksuite/framework/std/src/gfx/index.ts (1)
GfxControllerIdentifier(17-17)
🪛 Biome (1.9.4)
blocksuite/affine/blocks/image/src/utils.ts
[error] 451-451: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (11)
blocksuite/affine/blocks/image/src/image-service.ts (2)
4-4: Clean import updateRemoved the
FileSizeLimitServiceimport while retainingTelemetryProvider. This is consistent with the PR objective of removing the service dependency.
20-20: Function call properly updatedThe call to
addSiblingImageBlockhas been correctly updated to:
- Pass
stdinstead ofstd.hostas the first argument- Remove the now-unnecessary
maxFileSizeparameterThis aligns with the updated function signature in
utils.tswhich now useshasExceeded(std, files)internally to check file size limits directly from the blob sync engine.blocksuite/framework/sync/src/blob/engine.ts (2)
5-6: Good constant definition for default file size.The addition of a clearly named constant with a comment accurately reflects the purpose of this default value (10MB) for local use. This follows best practices of avoiding magic numbers in code.
110-112: Well-implemented getter for maxFileSize.This getter provides a clear, centralized approach to retrieving the maximum file size, with appropriate fallback to the default constant. It aligns with the PR objective of centralizing file size limit management in the blob engine.
packages/frontend/core/src/modules/workspace/entities/workspace.ts (2)
26-26: Appropriate helper function to encapsulate max file size access.Good approach to create a function that encapsulates the retrieval of
maxBlobSizefrom the engine, making the code more maintainable.
53-55: Well-implemented dynamic getter for maxFileSize.The use of a getter here is excellent as it addresses the previously raised concern about
maxBlobSizevalues changing. This implementation ensures the code always retrieves the current value rather than capturing a potentially stale value at initialization time.blocksuite/affine/blocks/image/src/commands/insert-images.ts (1)
31-31: Good simplification of function call parameters.The modified call to
addSiblingImageBlockis cleaner and more maintainable. Passing thestdobject directly aligns with the PR objective of centralizing file size limit access through the blob engine rather than using a separate service.blocksuite/framework/sync/src/blob/source.ts (1)
4-4: Good addition to the BlobSource interfaceAdding the optional
maxFileSizeproperty to the BlobSource interface is appropriate and aligns well with the PR objective of centralizing file size limit management.blocksuite/affine/blocks/attachment/src/embed.ts (1)
70-70: Correctly refactored to use centralized maxFileSizeThe change to access
maxFileSizedirectly from the blob sync engine instead of using the FileSizeLimitService is aligned with the PR objective of centralizing file size limit management.blocksuite/affine/blocks/attachment/src/attachment-block.ts (2)
13-13: Removed FileSizeLimitService import correctlyThe import list has been properly updated to remove the FileSizeLimitService while retaining ThemeProvider.
43-43: Correctly refactored to use centralized maxFileSizeThe change to access
maxFileSizedirectly from the blob sync engine instead of using the FileSizeLimitService is aligned with the PR objective of centralizing file size limit management.
| private readonly onReachedMaxBlobSizeCallbacks: Set< | ||
| (byteSize: number) => void | ||
| > = new Set(); | ||
|
|
||
| maxBlobSize = 1024 * 1024 * 100; // 100MB |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify default file size value consistency across the codebase.
The default maxBlobSize here is set to 100MB, while the fallback in BlobEngine is 10MB. Consider aligning these values to maintain consistency, or document the reason for the difference to avoid confusion.
🏁 Script executed:
#!/bin/bash
# Find all occurrences of file size constants in the codebase
echo "Searching for file size constants in the codebase..."
rg -i "max.*size|max.*blob|file.*size" --type ts -g "*.ts" | grep -E "([0-9]+\s*\*\s*1024|1024\s*\*\s*[0-9]+)"Length of output: 723
Align or document file size limits
There are multiple file‐size thresholds in the codebase (100 MB, 50 MB and 10 MB). To avoid confusion, either extract these into shared constants or document why they differ:
• packages/common/nbstore/src/frontend/blob.ts
– maxBlobSize = 1024 * 1024 * 100 // 100 MB
• blocksuite/framework/sync/src/blob/engine.ts
– const MAX_FILE_SIZE = 10 * 1024 * 1024 // 10 MB
• packages/frontend/media-capture-playground/server/main.ts
– fileSize: 50 * 1024 * 1024 // 50 MB
• packages/frontend/core/src/blocksuite/ai/components/ai-chat-chips/add-popover.ts
– if (file.size > 50 * 1024 * 1024) { … }
Options:
- Define shared constants (e.g.
TOTAL_BLOB_LIMIT,CHUNK_SIZE_LIMIT) and import them where needed. - If these serve different purposes (total upload vs. sync chunk vs. UI filter), add comments or update docs to clarify each limit’s intent.
fundon
force-pushed
the
fundon/04_23_get_maxFileSize_from_blobSync
branch
from
862b16b to
ece22e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
blocksuite/affine/blocks/image/src/utils.ts (1)
451-452: Simplify boolean expression.The ternary operator here is unnecessary since
isMultipleFilesis already a boolean.const isMultipleFiles = files.length > 1; -const inTopLeft = isMultipleFiles ? true : false; +const inTopLeft = isMultipleFiles;🧰 Tools
🪛 Biome (1.9.4)
[error] 451-451: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
blocksuite/affine/all/src/extensions/migrating-store.ts(0 hunks)blocksuite/affine/blocks/attachment/src/attachment-block.ts(2 hunks)blocksuite/affine/blocks/attachment/src/embed.ts(1 hunks)blocksuite/affine/blocks/attachment/src/utils.ts(4 hunks)blocksuite/affine/blocks/image/src/commands/insert-images.ts(1 hunks)blocksuite/affine/blocks/image/src/image-service.ts(2 hunks)blocksuite/affine/blocks/image/src/utils.ts(6 hunks)blocksuite/affine/shared/src/services/file-size-limit-service.ts(0 hunks)blocksuite/affine/shared/src/services/index.ts(0 hunks)blocksuite/framework/sync/src/blob/engine.ts(2 hunks)blocksuite/framework/sync/src/blob/source.ts(1 hunks)packages/common/nbstore/src/frontend/blob.ts(1 hunks)packages/frontend/core/src/modules/workspace/entities/workspace.ts(2 hunks)
💤 Files with no reviewable changes (3)
- blocksuite/affine/all/src/extensions/migrating-store.ts
- blocksuite/affine/shared/src/services/index.ts
- blocksuite/affine/shared/src/services/file-size-limit-service.ts
✅ Files skipped from review due to trivial changes (1)
- packages/frontend/core/src/modules/workspace/entities/workspace.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- blocksuite/framework/sync/src/blob/source.ts
- blocksuite/affine/blocks/attachment/src/embed.ts
- blocksuite/framework/sync/src/blob/engine.ts
- blocksuite/affine/blocks/image/src/image-service.ts
- blocksuite/affine/blocks/attachment/src/attachment-block.ts
- blocksuite/affine/blocks/image/src/commands/insert-images.ts
- packages/common/nbstore/src/frontend/blob.ts
- blocksuite/affine/blocks/attachment/src/utils.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
blocksuite/affine/blocks/image/src/utils.ts (2)
blocksuite/framework/sync/src/blob/engine.ts (1)
maxFileSize(110-112)blocksuite/framework/std/src/gfx/index.ts (1)
GfxControllerIdentifier(17-17)
🪛 Biome (1.9.4)
blocksuite/affine/blocks/image/src/utils.ts
[error] 451-451: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (10)
blocksuite/affine/blocks/image/src/utils.ts (10)
8-8: Simplified import structure.The removal of
FileSizeLimitServiceimport aligns with the PR's goal of centralizing file size limit logic within the blob storage layer.
312-325: Good abstraction of file size validation logic.The new
hasExceededhelper function effectively centralizes file size validation that was likely duplicated across multiple functions. It provides a consistent way to check file sizes against limits and display user-friendly error messages.
337-338: Consistent parameter replacement across functions.The change from
editorHosttostd: BlockStdScopeprovides a more comprehensive access point to the editor's components, aligning with the codebase's architecture.Also applies to: 367-368, 424-425
342-345: Improved function flow with early returns and validation.The refactoring improves code efficiency by:
- Filtering files to only include images
- Returning early if no valid files exist
- Using the new
hasExceededfunction to validate file sizes- Aborting if files exceed size limits
This approach reduces unnecessary processing and improves code readability.
355-359: Updated API usage to match new parameter structure.The code now correctly uses
std.storemethods instead ofeditorHost.docand passesstd.hostinstead ofeditorHostto the upload function, maintaining consistency with the parameter changes.Also applies to: 361-362
372-375: Consistent validation pattern applied.The same pattern of filtering files and early validation is appropriately applied here, ensuring consistency across similar functions.
377-382: Updated block creation and upload logic.Block creation now properly uses
std.storemethods, and the upload function receives the correct host parameter.
432-438: Optimized processing order inaddImages.The function now filters files and validates file sizes before retrieving the graphics controller, which is more efficient as it avoids unnecessary operations when validation fails.
454-458: Consistent use of filtered files array.The code now correctly iterates through the filtered
filesarray, ensuring that only valid image files are processed for upload.
474-508: Proper file processing with error handling.The image upload and block update logic correctly handles errors, validates image dimensions, and updates the model with appropriate values. The function now uses
std.storemethods consistently.
Closes: BS-3287
What's Changed!
FileSizeLimitServicemaxFileSizegetter toBlobSyncengineSummary by CodeRabbit
Refactor
New Features
Style
Chores