Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds utilities and hooks to resolve a shared document's publish mode from URL, server, and editor state; syncs editor mode with the URL; removes shared-mode gating from mode-switch logic; propagates resolved mode into share-page initialization and copy-link flows; and adds tests for the new utilities and default-mode helper. Changes
Sequence DiagramsequenceDiagram
participant User
participant SharePage as "Share Page"
participant Hooks as "Mode Hooks"
participant Server as "Server/GraphQL"
participant Editor as "Editor Service"
participant URL as "Browser / Location"
User->>SharePage: Open shared page link (may include ?mode=)
rect rgba(100,150,200,0.5)
SharePage->>Hooks: useSharedPublishMode(docId, publishMode?, workspaceId)
Hooks->>Server: Fetch page public mode if publishMode absent
Server-->>Hooks: Return public mode
Hooks->>Hooks: getResolvedPublishMode(queryMode, publicMode)
Hooks-->>SharePage: resolvedPublishMode
end
rect rgba(150,100,200,0.5)
SharePage->>Editor: Initialize / setMode(resolvedPublishMode)
Editor->>Editor: editor.mode$ = resolvedPublishMode
end
rect rgba(100,200,150,0.5)
SharePage->>Hooks: useSharedModeQuerySync(editor, resolvedPublishMode)
Hooks->>Editor: subscribe to editor.mode$
Editor-->>Hooks: currentMode
Hooks->>Hooks: getSearchWithMode(location.search, currentMode)
Hooks->>URL: replaceState with updated search (if changed)
end
User->>Editor: Toggle mode (page ↔ edgeless)
Editor->>Hooks: Emit mode change
Hooks->>URL: Sync mode to query param
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/frontend/core/src/desktop/pages/workspace/share/share-page.utils.ts (1)
4-8: Accept raw query values here.
URLSearchParams.get('mode')yieldsstring | null, so callers currently have to cast arbitrary input toDocModebefore this helper can validate it. Widening the parameter keeps that unsafe cast out of the caller and makesDocModes.includes(...)the single validation point.Suggested refactor
export const getResolvedPublishMode = ( - queryMode: DocMode | null, + queryMode: string | null | undefined, publicMode?: PublicDocMode | null ): DocMode => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/core/src/desktop/pages/workspace/share/share-page.utils.ts` around lines 4 - 8, The getResolvedPublishMode function should accept raw query input instead of forcing callers to cast: change the queryMode parameter type from DocMode | null to string | null and update its validation to use DocModes.includes(queryMode as DocMode) (or similar runtime check) so DocModes.includes(...) is the single validation point; ensure existing logic that checks publicMode (PublicDocMode) remains unchanged and that any returned DocMode values are still typed correctly by narrowing after the includes check.packages/frontend/core/src/desktop/pages/workspace/share/share-page.tsx (1)
219-225: Avoid replaying the same selector on first load.The editor already receives
selectorduring bootstrap at Line 198, so this effect immediately reapplies the samesetSelectoroncesetEditorcommits. IfsetSelectorscrolls or focuses, shared deep links will jump twice. Consider skipping the initial run or comparing the previous selector before reapplying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/core/src/desktop/pages/workspace/share/share-page.tsx` around lines 219 - 225, The effect re-applies the same selector on mount because editor was already initialized with selector; change the useEffect that calls editor.setSelector(selector) so it skips the initial replay: introduce a ref like hasAppliedSelectorRef or prevSelectorRef, on first run avoid calling editor.setSelector if prevSelectorRef.current === selector (or if hasAppliedSelectorRef is false and editor was bootstrapped), and only call editor.setSelector when selector differs from prevSelectorRef.current; after calling, update the ref. Target the useEffect with dependencies [editor, selector] and the editor.setSelector call to implement this guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/frontend/core/src/components/hooks/affine/use-share-url.utils.ts`:
- Around line 3-4: The helper getDefaultShareMode currently coerces an absent
currentMode into 'page', which breaks the intended fallback in onClickCopyLink;
change getDefaultShareMode (signature and implementation) to return undefined
when currentMode is not 'edgeless' so callers can treat an omitted mode as "use
current view" (i.e., make the return type DocMode | undefined and return
'edgeless' only when currentMode === 'edgeless', otherwise return undefined).
In `@packages/frontend/core/src/desktop/pages/workspace/share/share-page.tsx`:
- Around line 133-145: The component is treating a coerced default of 'page'
from useSharedPublishMode as authoritative (via resolvedPublishMode), which lets
transient getWorkspacePageByIdQuery failures boot an empty page; change
useSharedPublishMode (and/or getWorkspacePageByIdQuery) to avoid falling back to
the literal 'page' on query errors — instead return a tri-state (loading | error
| value) or undefined for unresolved mode so failures are distinguishable, and
update the caller in share-page.tsx (the useEffect that checks
resolvedPublishMode, editor, workspace, page and the later application at Line
~196) to keep the UI in loading state until resolvedPublishMode is a reliable
value or to attempt resolving from the public-share source; reference hooks:
useSharedPublishMode, getWorkspacePageByIdQuery, and useSharedModeQuerySync, and
ensure no implicit coercion to 'page' occurs on query errors.
In
`@packages/frontend/core/src/desktop/pages/workspace/share/use-shared-publish-mode.ts`:
- Around line 19-27: The hook initializes resolvedPublishMode from publishMode
only on mount, so when docId changes and publishMode is undefined it retains the
previous doc's mode; update the effect that runs when docId or publishMode
change (the useEffect around AbortController and fetching) to explicitly reset
resolvedPublishMode to null when docId changes and publishMode is not provided,
e.g. call setResolvedPublishMode(null) at the start of that effect before
issuing the GraphQL fetch; ensure the effect dependencies include docId and
publishMode so the reset happens on navigation and the subsequent fetch sets the
correct mode.
---
Nitpick comments:
In `@packages/frontend/core/src/desktop/pages/workspace/share/share-page.tsx`:
- Around line 219-225: The effect re-applies the same selector on mount because
editor was already initialized with selector; change the useEffect that calls
editor.setSelector(selector) so it skips the initial replay: introduce a ref
like hasAppliedSelectorRef or prevSelectorRef, on first run avoid calling
editor.setSelector if prevSelectorRef.current === selector (or if
hasAppliedSelectorRef is false and editor was bootstrapped), and only call
editor.setSelector when selector differs from prevSelectorRef.current; after
calling, update the ref. Target the useEffect with dependencies [editor,
selector] and the editor.setSelector call to implement this guard.
In
`@packages/frontend/core/src/desktop/pages/workspace/share/share-page.utils.ts`:
- Around line 4-8: The getResolvedPublishMode function should accept raw query
input instead of forcing callers to cast: change the queryMode parameter type
from DocMode | null to string | null and update its validation to use
DocModes.includes(queryMode as DocMode) (or similar runtime check) so
DocModes.includes(...) is the single validation point; ensure existing logic
that checks publicMode (PublicDocMode) remains unchanged and that any returned
DocMode values are still typed correctly by narrowing after the includes check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44e385ab-a889-44ff-8c3b-cb7387c42788
📒 Files selected for processing (10)
packages/frontend/core/src/__tests__/share-page.spec.tspackages/frontend/core/src/__tests__/use-share-url.utils.spec.tspackages/frontend/core/src/blocksuite/block-suite-mode-switch/index.tsxpackages/frontend/core/src/components/hooks/affine/use-register-copy-link-commands.tsxpackages/frontend/core/src/components/hooks/affine/use-share-url.utils.tspackages/frontend/core/src/desktop/pages/workspace/share/share-page.tsxpackages/frontend/core/src/desktop/pages/workspace/share/share-page.utils.tspackages/frontend/core/src/desktop/pages/workspace/share/use-shared-mode-query-sync.tspackages/frontend/core/src/desktop/pages/workspace/share/use-shared-publish-mode.tspackages/frontend/core/src/modules/share-menu/view/share-menu/copy-link-button.tsx
packages/frontend/core/src/components/hooks/affine/use-share-url.utils.ts
Outdated
Show resolved
Hide resolved
packages/frontend/core/src/desktop/pages/workspace/share/share-page.tsx
Outdated
Show resolved
Hide resolved
packages/frontend/core/src/desktop/pages/workspace/share/use-shared-publish-mode.ts
Show resolved
Hide resolved
ibex088
force-pushed
the
fix/shared-mode-link-sync
branch
from
784e271 to
d3fd46d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/frontend/core/src/desktop/pages/workspace/share/share-page.tsx (1)
143-217: Cancel the async share-page bootstrap when inputs change.This effect never cleans up its promise chain, so a route or mode change while either
waitForDocLoaded(...)call is pending can still run the oldsetPage/setEditorpath and apply a stale mode at Line 196. Add a cancelled flag and bail out before each state mutation; ifWorkspace/Editorhas explicit disposal APIs, use them in the cleanup too.Possible guard
useEffect(() => { + let cancelled = false; + if (!resolvedPublishMode || editor || workspace || page) { return; } // create a workspace for share page const { workspace: sharedWorkspace } = workspacesService.open( @@ - setWorkspace(sharedWorkspace); + setWorkspace(sharedWorkspace); sharedWorkspace.engine.doc .waitForDocLoaded(sharedWorkspace.id) .then(async () => { + if (cancelled) { + return; + } + const { doc } = sharedWorkspace.scope.get(DocsService).open(docId); doc.blockSuiteDoc.load(); doc.blockSuiteDoc.readonly = true; await sharedWorkspace.engine.doc.waitForDocLoaded(docId); + + if (cancelled) { + return; + } if (!doc.blockSuiteDoc.root) { throw new Error('Doc is empty'); } setPage(doc); const editor = doc.scope.get(EditorsService).createEditor(); editor.setMode(resolvedPublishMode); if (selector) { editor.setSelector(selector); } - setEditor(editor); + if (!cancelled) { + setEditor(editor); + } }) .catch(err => { + if (cancelled) { + return; + } + console.error(err); setNoPermission(true); }); + + return () => { + cancelled = true; + }; }, [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/core/src/desktop/pages/workspace/share/share-page.tsx` around lines 143 - 217, The effect that opens a shared workspace and awaits sharedWorkspace.engine.doc.waitForDocLoaded(...) lacks cleanup, so add a cancellation guard and dispose logic: introduce a local "cancelled" boolean (or AbortController) at the top of the useEffect and in the returned cleanup set cancelled = true and call any relevant disposal methods (e.g., sharedWorkspace.close()/dispose() or editor.dispose()/workspace.dispose() if available) to prevent stale mutations; before every setWorkspace, setPage, setEditor, editor.setMode, editor.setSelector and setNoPermission call inside the .then/.catch handlers check the cancelled flag and bail out if true so stale async completions don't apply old state. Ensure you reference the same symbols: sharedWorkspace, sharedWorkspace.engine.doc.waitForDocLoaded, doc.blockSuiteDoc, setWorkspace, setPage, setEditor, and editor (and dispose APIs if present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/frontend/core/src/desktop/pages/workspace/share/share-page.tsx`:
- Around line 268-277: The current guards turn publish-mode lookup failures into
a blank page; update the conditions so hasPublishModeError does not fall through
to the generic null loading guard: remove hasPublishModeError from the early
"return null" check and add an explicit branch that when hasPublishModeError &&
!currentPublishMode renders an error UI (e.g., a PublishModeError component or
reuse PageNotFound with an error message) instead of returning null; adjust the
three-way guard that checks workspace, page, editor, currentPublishMode so only
genuine loading states return null while publish-mode failures are handled by
the new error branch (refer to hasPublishModeError, currentPublishMode,
workspace, page, editor, and PageNotFound/use-shared-publish-mode.ts).
---
Nitpick comments:
In `@packages/frontend/core/src/desktop/pages/workspace/share/share-page.tsx`:
- Around line 143-217: The effect that opens a shared workspace and awaits
sharedWorkspace.engine.doc.waitForDocLoaded(...) lacks cleanup, so add a
cancellation guard and dispose logic: introduce a local "cancelled" boolean (or
AbortController) at the top of the useEffect and in the returned cleanup set
cancelled = true and call any relevant disposal methods (e.g.,
sharedWorkspace.close()/dispose() or editor.dispose()/workspace.dispose() if
available) to prevent stale mutations; before every setWorkspace, setPage,
setEditor, editor.setMode, editor.setSelector and setNoPermission call inside
the .then/.catch handlers check the cancelled flag and bail out if true so stale
async completions don't apply old state. Ensure you reference the same symbols:
sharedWorkspace, sharedWorkspace.engine.doc.waitForDocLoaded, doc.blockSuiteDoc,
setWorkspace, setPage, setEditor, and editor (and dispose APIs if present).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a94a0427-3738-41fa-a7de-ddf013050921
📒 Files selected for processing (2)
packages/frontend/core/src/desktop/pages/workspace/share/share-page.tsxpackages/frontend/core/src/desktop/pages/workspace/share/use-shared-publish-mode.ts
Comment on lines
+268
to
277
| if (hasPublishModeError && !workspace && !page && !editor) { | ||
| return null; | ||
| } | ||
|
|
||
| if (noPermission) { | ||
| return <PageNotFound noPermission />; | ||
| } | ||
|
|
||
| if (!workspace || !page || !editor) { | ||
| if (!workspace || !page || !editor || !currentPublishMode) { | ||
| return null; |
Contributor
There was a problem hiding this comment.
Don’t turn publish-mode lookup failures into a blank page.
When packages/frontend/core/src/desktop/pages/workspace/share/use-shared-publish-mode.ts sets hasPublishModeError at Lines 54-60, this component still renders null here, and the loading guard below does the same. A failed mode lookup now looks like an infinite blank screen instead of a recoverable error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/frontend/core/src/desktop/pages/workspace/share/share-page.tsx`
around lines 268 - 277, The current guards turn publish-mode lookup failures
into a blank page; update the conditions so hasPublishModeError does not fall
through to the generic null loading guard: remove hasPublishModeError from the
early "return null" check and add an explicit branch that when
hasPublishModeError && !currentPublishMode renders an error UI (e.g., a
PublishModeError component or reuse PageNotFound with an error message) instead
of returning null; adjust the three-way guard that checks workspace, page,
editor, currentPublishMode so only genuine loading states return null while
publish-mode failures are handled by the new error branch (refer to
hasPublishModeError, currentPublishMode, workspace, page, editor, and
PageNotFound/use-shared-publish-mode.ts).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## canary #14756 +/- ##
==========================================
- Coverage 57.83% 57.34% -0.50%
==========================================
Files 2959 2962 +3
Lines 165709 165746 +37
Branches 24418 24294 -124
==========================================
- Hits 95843 95045 -798
- Misses 66816 67648 +832
- Partials 3050 3053 +3
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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This fixes a few inconsistencies in shared page behavior:
fixes #14751
What Changed
Demo
https://www.loom.com/share/a287172321fb4fc5b94f7c67a39298a9
Summary by CodeRabbit
New Features
Tests
Bug Fixes