ENG-1285 Base user affordance to trigger node sharing #698
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a publishNode utility and a new Obsidian command that validates a node, retrieves the logged-in Supabase client and the user's group, inserts a ContentAccess record for the group's access to the node's content (handling duplicate key), and updates the file frontmatter to record the published group. Changes
Sequence DiagramsequenceDiagram
participant User
participant Plugin as Obsidian Plugin
participant File as File System
participant Supabase as Supabase Client
participant DB as Database
User->>Plugin: Trigger publish command
Plugin->>File: Read file & frontmatter
Plugin->>Plugin: Validate nodeInstanceId
Plugin->>Supabase: Get logged-in client
Supabase-->>Plugin: Client instance
Plugin->>DB: Query user's group
DB-->>Plugin: Group data
Plugin->>DB: Check existing ContentAccess / fetch content_id by source_local_id
DB-->>Plugin: Content/Access data
Plugin->>DB: Insert ContentAccess (handle 23505 duplicate)
DB-->>Plugin: Insert result
Plugin->>File: Update frontmatter.publishedToGroups and save
Plugin->>User: Show success or error Notice
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/obsidian/src/utils/registerCommands.ts`:
- Around line 181-182: The synchronous throw of new Error("Please sync the node
first") inside the checkCallback should be removed to avoid unhandled
exceptions; rely on publishNode's existing validation (which returns a rejected
Promise and is handled by .catch()), so delete the guard that checks
frontmatter.nodeInstanceId (or replace it with a user-facing Notice via new
Notice("Please sync the node first") if you prefer non-exception feedback) in
the checkCallback surrounding frontmatter.nodeInstanceId and keep publishNode as
the source of truth for validation.
🧹 Nitpick comments (3)
apps/obsidian/src/utils/publishNode.ts (2)
20-22: Redundant truthy check onexistingPublish.
existingPublishis guaranteed to be an array (due to|| []on line 21), so the truthy check is unnecessary.Suggested simplification
const existingPublish = (frontmatter.publishedToGroups as undefined | string[]) || []; - if (existingPublish && existingPublish.includes(myGroup)) return; // already published + if (existingPublish.includes(myGroup)) return; // already published
42-48: Mutating the frontmatter-derived array.
existingPublish.push(myGroup)mutates the array that may be referenced from the original frontmatter cache. While this works, creating a new array is safer and more explicit.Suggested change
- existingPublish.push(myGroup); await plugin.app.fileManager.processFrontMatter( file, (fm: Record<string, unknown>) => { - fm.publishedToGroups = existingPublish; + fm.publishedToGroups = [...existingPublish, myGroup]; }, );apps/obsidian/src/utils/registerCommands.ts (1)
170-173: Inconsistent return values in checkCallback.When validation fails inside
if (!checking), the barereturn;yieldsundefined. For consistency with other commands (e.g., lines 140-157), consider returningtrueto indicate the command is still available, orfalseif it should be hidden when conditions aren't met.Suggested change for consistency
if (!activeView || !activeView.file) { new Notice("Apply to a discourse node"); - return; + return true; } const file = activeView.file; const cache = plugin.app.metadataCache.getFileCache(file); const frontmatter = cache?.frontmatter || {}; if (!frontmatter.nodeTypeId) { new Notice("Apply to a discourse node"); - return; + return true; }Also applies to: 177-180
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/obsidian/src/utils/publishNode.tsapps/obsidian/src/utils/registerCommands.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefertypeoverinterfacein TypeScript
Use explicit return types for functions
Avoidanytypes when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability
Files:
apps/obsidian/src/utils/publishNode.tsapps/obsidian/src/utils/registerCommands.ts
apps/obsidian/**
📄 CodeRabbit inference engine (.cursor/rules/obsidian.mdc)
apps/obsidian/**: Prefer existing dependencies from apps/obsidian/package.json when adding dependencies to the Obsidian plugin
Follow the Obsidian style guide from help.obsidian.md/style-guide and docs.obsidian.md/Developer+policies for UI and code styling
Use Lucide and custom Obsidian icons alongside detailed elements to provide visual representation of features in platform-native UI
Files:
apps/obsidian/src/utils/publishNode.tsapps/obsidian/src/utils/registerCommands.ts
🧬 Code graph analysis (1)
apps/obsidian/src/utils/registerCommands.ts (1)
apps/obsidian/src/utils/publishNode.ts (1)
publishNode(5-49)
🔇 Additional comments (4)
apps/obsidian/src/utils/publishNode.ts (2)
1-3: LGTM on imports.Imports are appropriately typed and follow the project conventions.
14-19: The code silently publishes to the first group only; consider whether users with multiple group memberships should publish to a specific group or all groups.The query fetches all group memberships (
select("group_id")), but the code uses only the first one (data[0]?.group_id). If users can belong to multiple groups, this may silently ignore other memberships when publishing. The function doesn't accept a group parameter, so there's no way to publish to a different group.apps/obsidian/src/utils/registerCommands.ts (2)
1-1: LGTM on imports.Clean consolidation of obsidian imports and proper addition of publishNode import.
Also applies to: 10-10
187-194: LGTM on async handling pattern.The
.then()/.catch()pattern correctly handles async execution in a synchronous callback context, consistent with the existing sync command (lines 145-154).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/obsidian/src/utils/publishNode.ts`:
- Around line 14-19: The code in publishNode.ts currently takes only the first
membership by reading myGroupResponse.data[0]?.group_id, which ignores multiple
group memberships and conflicts with the publishedToGroups frontmatter; update
the logic to handle multiple groups by either filtering the query (e.g., add a
WHERE to pick a specific group based on publishedToGroups or a chosenGroup ID),
or iterate over myGroupResponse.data to collect all group_ids and use that array
when publishing (replace use of myGroup with a multi-group variable), or
explicitly document and enforce a single-group limitation if multi-group
publishing is not supported; adjust error handling around myGroupResponse and
the downstream publish calls to work with the chosen multi/single-group
approach.
🧹 Nitpick comments (2)
apps/obsidian/src/utils/publishNode.ts (1)
20-22: Potential stale data issue withexistingPublish.
existingPublishis captured at line 21, butprocessFrontMatteron line 42 executes later after async operations. If another process modifiespublishedToGroupsconcurrently, those changes could be overwritten.Consider reading the current state inside
processFrontMatter:♻️ Suggested fix to read fresh state inside processFrontMatter
await plugin.app.fileManager.processFrontMatter( file, (fm: Record<string, unknown>) => { - fm.publishedToGroups = [...existingPublish, myGroup]; + const current = (fm.publishedToGroups as string[] | undefined) || []; + if (!current.includes(myGroup)) { + fm.publishedToGroups = [...current, myGroup]; + } }, );Also applies to: 42-47
apps/obsidian/src/utils/registerCommands.ts (1)
177-196: Appropriate handling of missing nodeInstanceId.The previous review concern about synchronous throw has been addressed—the code now shows a Notice and returns
true, providing user feedback while keeping the command available.Minor typing note: Line 190 types the error as
Error, butpublishNodecan also throwPostgrestErrorfrom Supabase responses (lines 17, 30, 41 in publishNode.ts). Consider usingunknownand checking the type:♻️ Safer error handling
publishNode(plugin, file, frontmatter) .then(() => { new Notice("Published"); }) - .catch((error: Error) => { - new Notice(error.message); + .catch((error: unknown) => { + const message = error instanceof Error ? error.message : String(error); + new Notice(message); console.error(error); });
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/obsidian/src/utils/publishNode.tsapps/obsidian/src/utils/registerCommands.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefertypeoverinterfacein TypeScript
Use explicit return types for functions
Avoidanytypes when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability
Files:
apps/obsidian/src/utils/publishNode.tsapps/obsidian/src/utils/registerCommands.ts
apps/obsidian/**
📄 CodeRabbit inference engine (.cursor/rules/obsidian.mdc)
apps/obsidian/**: Prefer existing dependencies from apps/obsidian/package.json when adding dependencies to the Obsidian plugin
Follow the Obsidian style guide from help.obsidian.md/style-guide and docs.obsidian.md/Developer+policies for UI and code styling
Use Lucide and custom Obsidian icons alongside detailed elements to provide visual representation of features in platform-native UI
Files:
apps/obsidian/src/utils/publishNode.tsapps/obsidian/src/utils/registerCommands.ts
🧠 Learnings (1)
📚 Learning: 2025-10-18T18:58:16.100Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 504
File: apps/roam/src/utils/syncDgNodesToSupabase.ts:523-531
Timestamp: 2025-10-18T18:58:16.100Z
Learning: In `apps/roam/src/utils/syncDgNodesToSupabase.ts`, partial successes from `upsertNodesToSupabaseAsContent` and `addMissingEmbeddings` (indicated by numeric return values showing the count of successful operations) should NOT trigger backoff. Only complete failures (false) should trigger the exponential backoff mechanism. This design allows the sync process to continue making progress even when some items fail.
Applied to files:
apps/obsidian/src/utils/registerCommands.ts
🧬 Code graph analysis (1)
apps/obsidian/src/utils/registerCommands.ts (1)
apps/obsidian/src/utils/publishNode.ts (1)
publishNode(5-48)
🔇 Additional comments (4)
apps/obsidian/src/utils/publishNode.ts (2)
1-9: LGTM on function structure and signature.The function follows coding guidelines: arrow function, explicit return type, type imports, and clear parameter naming.
33-41: Good handling of duplicate key constraint.Treating PostgreSQL error code
23505(unique violation) as success is appropriate for idempotent publish operations.apps/obsidian/src/utils/registerCommands.ts (2)
1-1: LGTM on imports.Import additions are appropriate for the new command functionality.
Also applies to: 10-10
159-176: Good use of checkCallback pattern.The early returns for sync mode, active view, and nodeTypeId checks follow the established pattern from other commands in this file.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@CodeRabbit: PostgrestError extends Error, so typing is correct. |
|
Tip For best results, initiate chat on the files or code changes.
You're right about And I understand regarding the async concern — if no concurrent operations are planned that would affect that variable, then the current implementation is fine. I appreciate you taking the time to explain these points! 🧠 Learnings used |
| import { getLoggedInClient } from "./supabaseContext"; | ||
|
|
||
| export const publishNode = async ( | ||
| plugin: DiscourseGraphPlugin, |
There was a problem hiding this comment.
best to have parameter function here
|
lgtm, just a nit |
maparent
force-pushed
the
eng-1285-base-user-affordance-to-trigger-node-sharing
branch
from
621dc18 to
8d68f2c
Compare
maparent
force-pushed
the
eng-1285-base-user-affordance-to-trigger-node-sharing
branch
from
d34f8dd to
a45aadf
Compare
maparent
changed the base branch from
main
to
eng-1311-unify-conceptaccess-and-contentaccess-tables
https://linear.app/discourse-graphs/issue/ENG-1285/base-user-affordance-to-trigger-node-sharing
https://www.loom.com/share/74b40632fe4946d8b47f4d904424660f
This PR creates the command (affordance) to trigger node sharing, and invokes a minimal publishNode function.
Many things will be added to publishNode later: notably verifying that the schemas are shared, sending the assets.
We are considering factoring out sending the text so it is only sent for published nodes.
But we decided that would be another PR if decided.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.