feat: Add lasso function and add shortcut key operations related to excalidraw #12762
Conversation
|
mingrui seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
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. |
WalkthroughThis update introduces advanced keyboard-driven graph traversal and shape preview features for the edgeless canvas, a new lasso selection mode with supporting UI overlay, and integrates these capabilities into the relevant components and exports. It includes new classes, event handling logic, and utility methods to support these interactive behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant KeyboardManager
participant ShapePreviewOverlay
participant Canvas
User->>KeyboardManager: Press Alt/Option + Arrow
KeyboardManager->>KeyboardManager: Traverse graph, update selection
KeyboardManager->>Canvas: Ensure element visible
User->>KeyboardManager: Press Cmd/Ctrl + Arrow
KeyboardManager->>KeyboardManager: Compute preview position
KeyboardManager->>ShapePreviewOverlay: Show shape preview
User->>KeyboardManager: Release Cmd/Ctrl
KeyboardManager->>ShapePreviewOverlay: Create shapes from previews
ShapePreviewOverlay->>Canvas: Render new shapes
sequenceDiagram
participant User
participant DefaultTool
participant LassoDraggingAreaWidget
participant Canvas
User->>DefaultTool: Drag with Alt key
DefaultTool->>DefaultTool: Track lasso path
DefaultTool->>LassoDraggingAreaWidget: Update lasso overlay
DefaultTool->>DefaultTool: Update selection (elements inside lasso)
LassoDraggingAreaWidget->>Canvas: Render lasso overlay
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
blocksuite/affine/blocks/root/src/edgeless/shape-preview-overlay.ts (1)
26-28: Remove the unnecessary constructor.The constructor only calls
super(gfx)without adding any additional initialization logic.Apply this diff to remove the redundant constructor:
- constructor(gfx: GfxController) { - super(gfx); - }🧰 Tools
🪛 Biome (1.9.4)
[error] 26-28: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
blocksuite/affine/blocks/surface/src/tool/default-tool.ts (1)
375-397: Consider making the distance threshold configurable.The hard-coded threshold of 2 pixels for adding lasso points works well, but making it configurable would allow fine-tuning based on different use cases or display densities.
Consider extracting the threshold as a constant:
+const LASSO_POINT_THRESHOLD = 2; + case DefaultModeDragType.LassoSelecting: { // Add current point to lasso path const [currentX, currentY] = this.gfx.viewport.toModelCoord(e.x, e.y); const lastPoint = this.lassoPath[this.lassoPath.length - 1]; // Only add point if it's far enough from the last point to avoid too many points if ( !lastPoint || - Math.abs(currentX - lastPoint[0]) > 2 || - Math.abs(currentY - lastPoint[1]) > 2 + Math.abs(currentX - lastPoint[0]) > LASSO_POINT_THRESHOLD || + Math.abs(currentY - lastPoint[1]) > LASSO_POINT_THRESHOLD ) { this.lassoPath.push([currentX, currentY]); }blocksuite/affine/blocks/root/src/edgeless/edgeless-keyboard.ts (3)
95-259: Consider moving the detailed algorithm documentation to a separate documentation file.While the documentation is comprehensive and well-written, embedding 165 lines of algorithm documentation in the source code makes the file harder to navigate. Consider moving this to a separate markdown file in the project documentation.
1348-1420: Consider breaking down the traversal method for better maintainability.The
_traverseGraphmethod is handling multiple responsibilities and could be decomposed into smaller, more focused methods.Consider extracting logic into helper methods:
_initializeTraversalSession()_handleSiblingCycling()_selectNextSuccessor()_fallbackToPredecessor()This would make the code more testable and easier to understand.
729-863: Break down the _bindToggleHand method for better organization.This method handles multiple keyboard combinations and is becoming difficult to maintain. Consider splitting it into focused handlers.
Consider refactoring into separate methods:
_handleGraphTraversal(event: KeyboardEvent)_handleShapePreview(event: KeyboardEvent)_handleSpaceKey(event: KeyboardEvent)_handleMindMapTextInput(event: KeyboardEvent)This would improve readability and make the code easier to test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
blocksuite/affine/blocks/root/src/edgeless/edgeless-keyboard.ts(11 hunks)blocksuite/affine/blocks/root/src/edgeless/shape-preview-overlay.ts(1 hunks)blocksuite/affine/blocks/surface/src/tool/default-tool.ts(7 hunks)blocksuite/affine/widgets/edgeless-dragging-area/src/edgeless-lasso-dragging-area.ts(1 hunks)blocksuite/affine/widgets/edgeless-dragging-area/src/effects.ts(1 hunks)blocksuite/affine/widgets/edgeless-dragging-area/src/index.ts(1 hunks)blocksuite/affine/widgets/edgeless-dragging-area/src/view.ts(2 hunks)blocksuite/affine/widgets/edgeless-selected-rect/src/edgeless-selected-rect.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
blocksuite/affine/widgets/edgeless-dragging-area/src/view.ts (1)
blocksuite/affine/widgets/edgeless-dragging-area/src/edgeless-lasso-dragging-area.ts (1)
edgelessLassoDraggingAreaWidget(129-133)
blocksuite/affine/blocks/root/src/edgeless/edgeless-keyboard.ts (10)
blocksuite/affine/blocks/root/src/edgeless/shape-preview-overlay.ts (1)
ShapePreviewOverlay(14-121)blocksuite/framework/std/src/gfx/index.ts (1)
GfxModel(65-65)blocksuite/affine/widgets/edgeless-selected-rect/src/edgeless-selected-rect.ts (2)
selection(507-509)surface(511-513)blocksuite/affine/gfx/shape/src/shape-tool.ts (1)
ShapeTool(36-367)blocksuite/affine/model/src/consts/shape.ts (2)
getShapeType(29-34)getShapeRadius(36-41)blocksuite/affine/blocks/surface/src/index.ts (2)
getSurfaceComponent(38-38)getSurfaceBlock(37-37)blocksuite/affine/model/src/themes/default.ts (1)
DefaultTheme(112-136)blocksuite/affine/widgets/edgeless-selected-rect/src/utils.ts (2)
nextBound(178-243)getPosition(245-268)blocksuite/affine/model/src/elements/connector/connector.ts (1)
ConnectorElementModel(106-513)blocksuite/affine/blocks/root/src/edgeless/edgeless-root-service.ts (1)
viewport(103-105)
🪛 Biome (1.9.4)
blocksuite/affine/blocks/root/src/edgeless/shape-preview-overlay.ts
[error] 26-28: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (18)
blocksuite/affine/widgets/edgeless-selected-rect/src/edgeless-selected-rect.ts (1)
34-34: LGTM: Clean utility re-export.This export statement properly exposes utility functions from the utils module, following established patterns for widget module APIs.
blocksuite/affine/widgets/edgeless-dragging-area/src/view.ts (2)
7-7: LGTM: Proper widget import.The import follows the established pattern and naming convention.
22-22: LGTM: Correct widget registration.The lasso widget is properly registered alongside the existing dragging area widget within the edgeless scope check.
blocksuite/affine/widgets/edgeless-dragging-area/src/effects.ts (2)
5-8: LGTM: Consistent import pattern.The import structure follows the established pattern for widget components.
15-18: LGTM: Proper custom element registration.The lasso widget is correctly registered as a custom element following the same pattern as the existing dragging area widget.
blocksuite/affine/widgets/edgeless-dragging-area/src/index.ts (1)
2-2: LGTM: Standard module export.The export statement properly exposes the lasso dragging area module as part of the public API.
blocksuite/affine/widgets/edgeless-dragging-area/src/edgeless-lasso-dragging-area.ts (8)
12-13: LGTM: Clear widget identifier.The constant naming follows established conventions for widget identifiers.
16-37: LGTM: Well-structured CSS styling.The styles properly use theme variables and include appropriate visual properties for the lasso selection overlay. The dashed stroke pattern is a good UX choice for selection indication.
39-41: LGTM: Standard GFX controller access.Follows the established pattern for accessing the graphics controller.
43-68: LGTM: Robust path generation logic.The
_getPathStringmethod handles edge cases well:
- Empty points array
- Single point (renders as circle)
- Multiple points (renders as polygon)
- Proper coordinate transformation to viewport space
- Correct SVG path syntax
The logic is clear and handles the different lasso states appropriately.
70-79: LGTM: Proper conditional rendering.The render method correctly checks for the lasso selection tool state before rendering anything.
88-96: LGTM: Clean SVG rendering.The SVG structure is simple and appropriate for rendering the lasso path.
99-126: LGTM: Comprehensive subscription management.The widget properly subscribes to all relevant state changes:
- Tool changes
- Dragging state
- Dragging area updates
The disposables are correctly managed and the conditional update logic is appropriate.
129-133: LGTM: Standard widget registration.The widget view extension follows the established pattern for widget registration.
blocksuite/affine/blocks/root/src/edgeless/shape-preview-overlay.ts (1)
81-121: Well-implemented preview rendering logic.The render method properly handles canvas state, draws connectors before shapes, and provides clear visual feedback with dashed lines and transparency.
blocksuite/affine/blocks/surface/src/tool/default-tool.ts (2)
80-111: Correct implementation of lasso selection logic.The
_getLassoSelectedElementsmethod properly:
- Validates minimum points for a valid lasso
- Closes the path if needed
- Filters elements that are completely inside the lasso
- Excludes locked elements
130-144: Correct ray-casting point-in-polygon algorithm.The implementation follows the standard ray-casting approach for determining if a point is inside a polygon.
blocksuite/affine/blocks/root/src/edgeless/edgeless-keyboard.ts (1)
669-687:❓ Verification inconclusive
Avoid direct DOM manipulation for toolbar interaction.
Using querySelector and casting to
anyto trigger the shape menu is fragile and not type-safe. Consider using a proper API or event system.Instead of direct DOM manipulation, consider:
- // Trigger the shape switch menu in the floating toolbar to show - requestAnimationFrame(() => { - // Find the floating toolbar - const toolbarWidget = rootComponent.querySelector( - 'affine-toolbar-widget' - ); - if (toolbarWidget) { - // Find the shape switch menu button - const shapeMenuButton = - toolbarWidget.shadowRoot?.querySelector( - 'editor-menu-button[aria-label="switch shape type-menu"]' - ); - if ( - shapeMenuButton && - typeof (shapeMenuButton as any).show === 'function' - ) { - (shapeMenuButton as any).show(); - } - } - }); + // Use an event or API to trigger the shape menu + this.slots.requestShapeMenuDisplay?.emit();
🏁 Script executed:
#!/bin/bash echo "Searching for requestShapeMenuDisplay..." rg "requestShapeMenuDisplay" -n echo "Searching for slots usage..." rg "slots" -n echo "Searching for affine-toolbar-widget usage..." rg "affine-toolbar-widget" -nLength of output: 49224
🏁 Script executed:
#!/bin/bash sed -n '650,720p' blocksuite/affine/blocks/root/src/edgeless/edgeless-keyboard.tsLength of output: 2591
Avoid direct DOM access to display the shape‐switch menu
The nested
requestAnimationFramecalls withquerySelector+(shapeMenuButton as any).show()is brittle and bypasses the toolbar’s public API. Instead, surface a proper event or slot so the toolbar widget can react:• File:
blocksuite/affine/blocks/root/src/edgeless/edgeless-keyboard.ts
Lines ~672–687• Replace:
requestAnimationFrame(() => { // Find the floating toolbar const toolbarWidget = rootComponent.querySelector('affine-toolbar-widget'); if (toolbarWidget) { const shapeMenuButton = toolbarWidget.shadowRoot?.querySelector( 'editor-menu-button[aria-label="switch shape type-menu"]' ); if (shapeMenuButton && typeof (shapeMenuButton as any).show === 'function') { (shapeMenuButton as any).show(); } } });• With something like:
// emit a named slot or DOM event instead of digging into the toolbar this.slots.showShapeMenu.next(); // — or — rootComponent.dispatchEvent( new CustomEvent('show-shape-menu', { bubbles: true }) );Then have the
affine-toolbar-widgetlisten forshowShapeMenu(or your custom event) and call its ownshow()internally.This keeps the keyboard handler decoupled from the toolbar’s internal DOM structure.
blocksuite/affine/widgets/edgeless-dragging-area/src/edgeless-lasso-dragging-area.ts
Outdated
Show resolved
Hide resolved
| // Create connector between original and new shape | ||
| const { startPosition, endPosition } = getPosition(direction); | ||
| crud.addElement('connector', { | ||
| mode: 1, // ConnectorMode.Orthogonal (Elbowed) |
There was a problem hiding this comment.
Replace magic number with named constant.
The connector mode value 1 should use the appropriate enum value for clarity.
- mode: 1, // ConnectorMode.Orthogonal (Elbowed)
+ mode: ConnectorMode.Orthogonal,Import the enum if not already available:
import { ConnectorMode } from '@blocksuite/affine-model';🤖 Prompt for AI Agents
In blocksuite/affine/blocks/root/src/edgeless/edgeless-keyboard.ts at line 1306,
replace the magic number 1 used for the connector mode with the named enum
constant ConnectorMode.Orthogonal for better clarity. Import ConnectorMode from
'@blocksuite/affine-model' if it is not already imported, then update the mode
property to use ConnectorMode.Orthogonal instead of the literal 1.
| private _initShapePreviewOverlay() { | ||
| this._shapePreviewOverlay = new ShapePreviewOverlay(this.gfx); | ||
| this._pathGenerator = new ConnectorPathGenerator({ | ||
| getElementById: id => this.gfx.getElementById(id), | ||
| }); | ||
|
|
||
| const surface = getSurfaceComponent(this.std); | ||
| if (surface) { | ||
| surface.renderer.addOverlay(this._shapePreviewOverlay); | ||
| } else { | ||
| // Try again after a short delay | ||
| setTimeout(() => { | ||
| const surface = getSurfaceComponent(this.std); | ||
| if (surface && this._shapePreviewOverlay) { | ||
| surface.renderer.addOverlay(this._shapePreviewOverlay); | ||
| } | ||
| }, 100); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve overlay initialization timing.
The retry logic with setTimeout suggests a race condition. Consider using a more robust initialization approach.
Instead of using setTimeout, consider using a lifecycle hook or event:
private _initShapePreviewOverlay() {
this._shapePreviewOverlay = new ShapePreviewOverlay(this.gfx);
this._pathGenerator = new ConnectorPathGenerator({
getElementById: id => this.gfx.getElementById(id),
});
- const surface = getSurfaceComponent(this.std);
- if (surface) {
- surface.renderer.addOverlay(this._shapePreviewOverlay);
- } else {
- // Try again after a short delay
- setTimeout(() => {
- const surface = getSurfaceComponent(this.std);
- if (surface && this._shapePreviewOverlay) {
- surface.renderer.addOverlay(this._shapePreviewOverlay);
- }
- }, 100);
- }
+ // Use a more reliable approach, such as waiting for the surface component to be ready
+ const addOverlay = () => {
+ const surface = getSurfaceComponent(this.std);
+ if (surface && this._shapePreviewOverlay) {
+ surface.renderer.addOverlay(this._shapePreviewOverlay);
+ return true;
+ }
+ return false;
+ };
+
+ if (!addOverlay()) {
+ // Consider using an event listener or lifecycle hook instead of setTimeout
+ this.rootComponent.std.provider.whenReady(() => addOverlay());
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In blocksuite/affine/blocks/root/src/edgeless/edgeless-keyboard.ts around lines
1099 to 1117, the overlay initialization uses setTimeout to retry adding the
overlay, which is a fragile approach to handle race conditions. Replace this
retry logic with a more reliable method such as subscribing to a lifecycle event
or hook that signals when the surface component is ready, and add the overlay in
that event handler to ensure proper timing without arbitrary delays.
|
View your CI Pipeline Execution ↗ for commit e68ee6a.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## canary #12762 +/- ##
==========================================
- Coverage 55.94% 55.52% -0.42%
==========================================
Files 2653 2655 +2
Lines 125311 125683 +372
Branches 19901 19927 +26
==========================================
- Hits 70099 69784 -315
- Misses 53467 54142 +675
- Partials 1745 1757 +12
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:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
tools/cli/src/webpack/index.ts (1)
588-609:context.endsWith('packages/backend/native')is platform-dependent
contexton Windows contains back-slashes.
'.../packages\\backend\\native'.endsWith('packages/backend/native')➜false, so.nodefiles won’t be ignored and the bundle may contain wrong binaries.- context.endsWith('packages/backend/native'); + context && + path + .normalize(context) + .replaceAll(path.sep, '/') + .endsWith('packages/backend/native');Import
pathat the top if not already.
🧹 Nitpick comments (4)
tools/cli/src/webpack/index.ts (1)
588-609: Minor: early return for readabilitySmall nit – compute the two predicates first and return their
||, avoids nested logic:- const shouldIgnoreLazyImport = ... - const isNativeModuleSpecificArch = ... - return shouldIgnoreLazyImport || isNativeModuleSpecificArch; + const shouldIgnoreLazyImport = ... + const isSpecificArchNative = ... + return shouldIgnoreLazyImport || isSpecificArchNative;.github/deployment/node/Dockerfile (1)
3-18: Run as non-root for better container hardeningThe image stays on the default
rootuser which can be problematic in multi-tenant hosts.
Consider:# after installing deps RUN useradd -m affine USER affineor the slim‐Debian recommended
nodeuser (UID 1000).DOCKER_SELFHOST_PLAN.md (1)
68-70: Path reference appears outdatedThe text suggests copying from
.docker/dev/compose.yml.example, but the repo now has.docker/selfhost/compose.yml.
Verify the example path to avoid confusion for readers.docker-compose.yml (1)
61-70: PostgreSQL starts withtrustauthentication – warn admins
POSTGRES_HOST_AUTH_METHOD: trustdisables password checks for every incoming connection inside the container network.
Fine for demos, risky for production if other containers are compromised.Advise users to:
- omit this variable and rely on
md5(default), or- scope access via
pg_hba.conf.At minimum, call this out prominently in the docs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.env(1 hunks).github/deployment/node/Dockerfile(1 hunks).yarnrc.yml(1 hunks)DOCKER_SELFHOST_PLAN.md(1 hunks)docker-compose.yml(1 hunks)tools/cli/src/webpack/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .yarnrc.yml
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env
[warning] 18-18: [UnorderedKey] The CONFIG_LOCATION key should go before the DB_DATA_LOCATION key
[warning] 22-22: [UnorderedKey] The DB_PASSWORD key should go before the DB_USERNAME key
[warning] 23-23: [UnorderedKey] The DB_DATABASE key should go before the DB_PASSWORD key
🪛 LanguageTool
DOCKER_SELFHOST_PLAN.md
[uncategorized] ~65-~65: 您的意思是“在"于"”吗？
Context: ....yml文件引用了一个.env文件来配置数据库凭据、存储位置等。请在与docker-compose.yml**相同的目录**下创建一个.e...
(YU7_YU8)
🔇 Additional comments (2)
docker-compose.yml (2)
23-24: Variable substitution inside single-quoted YAML may surpriseWhile Docker Compose expands
${VAR}in most contexts, quoting the whole string is unnecessary and can trip up newcomers. Consider:- - DATABASE_URL=postgresql://${DB_USERNAME}:${DB_PASSWORD}@postgres:5432/${DB_DATABASE:-affine} + - DATABASE_URL=postgresql://${DB_USERNAME}:${DB_PASSWORD}@postgres:5432/${DB_DATABASE:-affine}or, even clearer, put the URL into the
.envfile.
15-18: Unmounted variables will create anonymous volumesIf
UPLOAD_LOCATION/CONFIG_LOCATIONare not defined in.env, Compose creates anonymous host-paths such as/:/root/.affine/storage. Provide sane defaults or addrequired: truedocs.
| DB_USERNAME=affine | ||
| DB_PASSWORD=affine | ||
| DB_DATABASE=affine |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not ship real credentials – provide examples instead
DB_USERNAME=affine, DB_PASSWORD=affine, DB_DATABASE=affine are fine for local demos but dangerous if somebody commits this file verbatim to production.
Best practice:
-DB_USERNAME=affine
-DB_PASSWORD=affine
-DB_DATABASE=affine
+# Copy this block into a local .env, never commit real passwords
+DB_USERNAME=affine
+DB_PASSWORD=change-me
+DB_DATABASE=affineand add:
# .gitignore
.env
to avoid accidental leaks.
Alternatively commit a .env.example instead of the real file.
📝 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.
| DB_USERNAME=affine | |
| DB_PASSWORD=affine | |
| DB_DATABASE=affine | |
| # Copy this block into a local .env, never commit real passwords | |
| DB_USERNAME=affine | |
| DB_PASSWORD=change-me | |
| DB_DATABASE=affine |
🧰 Tools
🪛 dotenv-linter (3.3.0)
[warning] 22-22: [UnorderedKey] The DB_PASSWORD key should go before the DB_USERNAME key
[warning] 23-23: [UnorderedKey] The DB_DATABASE key should go before the DB_PASSWORD key
🤖 Prompt for AI Agents
In the .env file around lines 21 to 23, replace the real database credentials
with placeholder example values to avoid exposing sensitive information. Then,
add the .env file to .gitignore to prevent it from being committed to the
repository. Alternatively, create and commit a .env.example file containing only
example environment variables without real secrets.
| DB_DATA_LOCATION=~/.affine/self-host/postgres/pgdata | ||
| # position of the upload data(images, files, etc.) to persist | ||
| UPLOAD_LOCATION=~/.affine/self-host/storage | ||
| # position of the configuration files to persist | ||
| CONFIG_LOCATION=~/.affine/self-host/config |
There was a problem hiding this comment.
~ path expansion may not work inside containers
~/.affine/... will resolve to the home directory of the host that runs docker compose, not to /root (or any other) inside the containers.
When these variables are injected into the docker-compose.yml volumes they will be mounted literally, leading to unexpected host-path creation like /home/ci/.affine/... on Linux and a very different place on macOS/Windows.
Use explicit absolute paths (or paths relative to $PWD) instead:
-DB_DATA_LOCATION=~/.affine/self-host/postgres/pgdata
-UPLOAD_LOCATION=~/.affine/self-host/storage
-CONFIG_LOCATION=~/.affine/self-host/config
+DB_DATA_LOCATION=${PWD}/data/postgres
+UPLOAD_LOCATION=${PWD}/data/storage
+CONFIG_LOCATION=${PWD}/data/config📝 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.
| DB_DATA_LOCATION=~/.affine/self-host/postgres/pgdata | |
| # position of the upload data(images, files, etc.) to persist | |
| UPLOAD_LOCATION=~/.affine/self-host/storage | |
| # position of the configuration files to persist | |
| CONFIG_LOCATION=~/.affine/self-host/config | |
| DB_DATA_LOCATION=${PWD}/data/postgres | |
| # position of the upload data(images, files, etc.) to persist | |
| UPLOAD_LOCATION=${PWD}/data/storage | |
| # position of the configuration files to persist | |
| CONFIG_LOCATION=${PWD}/data/config |
🧰 Tools
🪛 dotenv-linter (3.3.0)
[warning] 18-18: [UnorderedKey] The CONFIG_LOCATION key should go before the DB_DATA_LOCATION key
🤖 Prompt for AI Agents
In the .env file around lines 14 to 18, the use of `~` for home directory paths
will not expand correctly inside Docker containers, causing volumes to mount to
unintended host directories. Replace all `~` prefixes with explicit absolute
paths or paths relative to the current working directory ($PWD) to ensure
correct volume mounting inside containers.
| COPY packages/backend/server ./ | ||
| COPY node_modules /app/node_modules | ||
|
|
||
| # Copy built frontend applications | ||
| COPY packages/frontend/apps/web/dist ./static | ||
| COPY packages/frontend/admin/dist ./static/admin | ||
| COPY packages/frontend/apps/mobile/dist ./static/mobile | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Copying host-built node_modules bakes OS-specific binaries
The host’s node_modules may hold native add-ons compiled for the host, not for the slim Debian image used here.
This often breaks when the host is macOS/Windows or a different CPU architecture.
Recommended pattern:
-COPY node_modules /app/node_modules
+COPY package*.json ./
+RUN npm ci --production --ignore-scriptsthen copy only the packages that need post-install scripts (or rebuild them with npm rebuild).
This keeps the image deterministic and multi-platform.
📝 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.
| COPY packages/backend/server ./ | |
| COPY node_modules /app/node_modules | |
| # Copy built frontend applications | |
| COPY packages/frontend/apps/web/dist ./static | |
| COPY packages/frontend/admin/dist ./static/admin | |
| COPY packages/frontend/apps/mobile/dist ./static/mobile | |
| COPY packages/backend/server ./ | |
| COPY package*.json ./ | |
| RUN npm ci --production --ignore-scripts | |
| # Copy built frontend applications | |
| COPY packages/frontend/apps/web/dist ./static | |
| COPY packages/frontend/admin/dist ./static/admin | |
| COPY packages/frontend/apps/mobile/dist ./static/mobile |
🤖 Prompt for AI Agents
In .github/deployment/node/Dockerfile around lines 6 to 13, copying the host's
node_modules directory directly into the image causes OS-specific binaries to be
included, which can break the container on different platforms. To fix this,
remove the COPY node_modules line and instead run npm install or yarn install
inside the Dockerfile after copying package.json and package-lock.json files,
ensuring dependencies are installed within the container environment. For
packages requiring post-install scripts or native builds, run npm rebuild or the
equivalent after installation to maintain cross-platform compatibility and
deterministic builds.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
affine_docker_setup.sh (5)
5-9: Verify presence of required CLI tools
Before checkingnode_modules, validate thatyarnis installed (command -v yarn) and ensure Docker/Compose commands exist to fail fast with clear messages.
35-43: Check Docker & Compose, add health‐check waits
Ensuredockeranddocker composeare available, then wait for Redis/Postgres to become healthy rather than proceeding immediately. This avoids race conditions if services take longer to initialize.
44-51: Reuse build helper for Docker images
Similar to the package builds, wrap the Docker image builds in the same helper to unify error handling and reduce boilerplate.
52-59: Add readiness checks after service startup
Afterdocker compose up -d affine_server affine_front, poll endpoints or container health status to confirm services are responsive before printing completion.
60-61: Consider cleanup instructions and environment‐variable reminder
At the end, remind users how to stop the stack (docker compose down) and verify that a.envfile with required variables exists.
| #!/bin/bash | ||
|
|
||
| echo "=== 开始 AFFiNE 自托管部署脚本 ===" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add robust error handling flags
Introduce set -euo pipefail right after the shebang to exit on any error, treat unset variables as errors, and catch failures in pipelines.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In affine_docker_setup.sh at lines 1 to 4, add the line 'set -euo pipefail'
immediately after the shebang (#!/bin/bash) to enable strict error handling,
causing the script to exit on any command failure, treat unset variables as
errors, and catch errors in pipelines.
| echo "--- 1. 构建 @affine/reader ---" | ||
| yarn affine build -p @affine/reader | ||
| if [ $? -ne 0 ]; then | ||
| echo "构建 @affine/reader 失败。请检查错误信息。" | ||
| exit 1 | ||
| fi | ||
| echo "--- @affine/reader 构建完成 ---" | ||
|
|
||
| echo "--- 2. 构建 @affine/server-native ---" | ||
| yarn affine @affine/server-native build | ||
| if [ $? -ne 0 ]; then | ||
| echo "构建 @affine/server-native 失败。请检查错误信息。" | ||
| exit 1 | ||
| fi | ||
| echo "--- @affine/server-native 构建完成 ---" | ||
|
|
||
| echo "--- 3. 构建 @affine/server ---" | ||
| yarn affine build -p @affine/server | ||
| if [ $? -ne 0 ]; then | ||
| echo "构建 @affine/server 失败。请检查错误信息。" | ||
| exit 1 | ||
| fi | ||
| echo "--- @affine/server 构建完成 ---" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate repetitive build steps into a helper function
The block for building each package repeats the same error‐checking pattern. Factor it into a function like run_step <description> <command …>. This reduces duplication and centralizes logging.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In affine_docker_setup.sh around lines 11 to 33, the build steps for each
package repeat the same pattern of running a command, checking its exit status,
and logging success or failure messages. Refactor this by creating a helper
function, for example run_step, that takes a description and a command as
arguments, runs the command, checks for errors, and logs messages accordingly.
Replace the repeated blocks with calls to this function to reduce duplication
and centralize error handling and logging.
Select the geometric figure and press enter to enter the editing process
Select the geometric shape and press tab to switch the shape type
Select the geometric figure and press commond+ direction keys or control+ direction keys to add the geometric figure in a certain direction
Select a geometric figure and press option+ arrow keys or alt+ arrow keys to jump to another geometric figure
Add lasso function
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores