Add native editor embed blocks#19
Conversation
Greptile SummaryThis PR adds native editor embed blocks and related navigation updates. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (6): Last reviewed commit: "fix: require fresh spaces for settings" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 25 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 597eb28320
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #if os(iOS) | ||
| .safeAreaInset(edge: .bottom) { |
There was a problem hiding this comment.
With this #if os(iOS) wrapping the inset, macOS builds of PageReaderView (used by MacMainShellDetailView and MacPageWindowView) never render NativeEditorToolbar; the only other production toolbar call is this block, so focusing an editable block on Mac loses the bold/italic/link/attachment/inline-comment controls and the Done/autosave action. Please keep a macOS toolbar/menu replacement before excluding this view.
Useful? React with 👍 / 👎.
|
|
||
| init(source: String) { | ||
| self.source = source | ||
| self._player = State(initialValue: YouTubePlayer(urlString: source)) |
There was a problem hiding this comment.
When an existing YouTube embed is edited through the block editor, SwiftUI preserves this child view's identity, so this @State initializer runs only once; because body never uses source to update player, changing the URL leaves YouTubePlayerView attached to the old video until the row is recreated. Recreate the view with .id(source) or update the player when source changes.
Useful? React with 👍 / 👎.
| static func youtubeWatchSource(from source: String?) -> String? { | ||
| guard let videoID = youtubeVideoID(from: source) else { return nil } | ||
| return "https://www.youtube.com/watch?v=\(videoID)" |
There was a problem hiding this comment.
For user-provided YouTube links such as ...?v=abc&start=90 or &t=90, this rebuilds the player URL from only the video ID, so the native embed silently starts at 0 instead of the timestamp the page author saved. Carry supported time query items through when canonicalizing the player source.
Useful? React with 👍 / 👎.
| isa = PBXFrameworksBuildPhase; | ||
| buildActionMask = 2147483647; | ||
| files = ( | ||
| 3FEC0B2EC8EF1D5311589C30 /* YouTubePlayerKit in Frameworks */, |
There was a problem hiding this comment.
This links YouTubePlayerKit into the docmostly app's Frameworks phase, but the docmostly target's packageProductDependencies block is still empty while the same product ref is only listed for DocmostlyMacTests. Since NativeEditorEmbedBlockView.swift imports this module from the shared app sources, the iOS app target needs the package product dependency too; otherwise the package product is not made available consistently when building the iOS target.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| private func loadSpacesIfNeeded() async { | ||
| guard appState.spaces.isEmpty else { return } |
There was a problem hiding this comment.
When an authenticated session has cached spaces because AppState.loadSpaces() fell back after a transient spaces request failure, this guard prevents later MainShellView appearances from retrying the network load at all. That leaves isOffline, memberships, and the space list stuck on cache until some other explicit refresh happens; keep the duplicate-load avoidance tied to an in-flight task rather than treating any non-empty cache as fresh.
Useful? React with 👍 / 👎.
|
|
||
| private var sourceURL: URL? { | ||
| guard let source = embed.source else { return nil } | ||
| return URL(string: source) |
There was a problem hiding this comment.
For generic embeds imported from page content or typed into the Source field, URL(string:) accepts non-web schemes such as file: or custom app URLs, and the view then exposes them as tappable Links. Since these are web embed sources, guard for http/https and render other values as plain text to avoid dispatching arbitrary document-controlled URLs through the system opener.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Consolidates page navigation patterns by introducing PageOpenTarget, a unified type for page navigation across the app. Removes redundant destination views (NotificationDestinationView, PageBrowserDestinationView, SearchResultDestinationView) and replaces NavigationLink usage with PageOpenLink for consistent platform-specific behavior. Refactors SpaceSettingsDetailFormView to use a tab-based UI with Settings, Members, and Security tabs, and introduces SpaceSettingsDialog for macOS modal presentation. Updates MacDesktopCommandController and command handlers to request space settings presentation instead of direct sidebar selection. Includes tests for PageOpenTarget initialization and navigation behavior with space switching.
There was a problem hiding this comment.
1 issue found across 28 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
1 participant
Summary
Validation
git diff --cached --checknpm test --prefix Tools/CRDTRuntimeswiftlint lint --strict --quiet --cache-path /private/tmp/docmostly-swiftlint-cacheNot run: Xcode builds/tests or simulator verification, per repo instruction.