ENG-1656: Rename imported vault folders to owner-username-spaceName#1130
ENG-1656: Rename imported vault folders to owner-username-spaceName#1130trangdoan982 wants to merge 10 commits into
Conversation
Use .dg.metadata for folder identity and auto-rename legacy import/ folders on load and import so multi-user lab vaults are easier to tell apart. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
PR size/scope checkThis PR is over our review-size guideline.
Please split this into smaller PRs unless there is a clear reason the changes need to land together. If keeping it as one PR, please add a brief justification covering:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Return undefined when owner username is not cached, treat legacy collision-suffixed folders as eligible for migration, and resolve lint warnings in changed files. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
A few issues picked up, noted here and in comments. I won't block on them, but combined probably worth another review and discussion in the dev meeting. Especially the last question.
Preserve pre-existing custom import folder names
discourse-graph\apps\obsidian\src\utils\importFolderMetadata.ts:252-253
The task description states: "We should rename the folder when its original name is just the vault name.". For vaults where a user manually renamed an import folder before thismigratedflag existed, the metadata will not havemigrated: true, so this branch treats the custom basename as stale and renames it to the generateduser-spacename as soon as a username is available. Since the previous resolver intentionally used.dg.metadatato find folders independent of their names, this discards existing custom organization instead of marking those folders as custom/migrated.
Migration picks first author not majority
resolveUserNameFromFolderreturns the username for the first markdown file (in vault iteration order) with a resolvable authorId, while imports useresolveOwnerUserNameto pick the most frequent author in the space. Migration can rename folders with the wrong user prefix, and migrated: true prevents correction on later imports.
Question: why do imports derive the folder prefix from resolveOwnerUserName()?
- As written, imports still create one folder per
spaceUri, not one folder per author. For a multi-author space,resolveOwnerUserName()picks the most frequent author in the current import batch, whilemigrateImportFolderNames()appears to use the first knownauthorIdfound in existing files. That means a space likediscourse-graphscould be renamed to one contributor’s username, and future imports from that same space would continue landing in that single folder via.dg.metadata.spaceUri. - Is that intended? If the desired schema is
{owner-username}-{spaceName}, should we fetch the actual space owner and have one folder per author-spaceName instead of deriving it from imported node authors?
cc @maparent
|
Actually, re "Migration picks first author not majority". Why are we using top author/first author instead of querying for the actual synced author? Why are we guessing instead of going to the source (username in supabase)? |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Suggested PR scope change: https://linear.app/discourse-graphs/issue/ENG-1656/rename-folder-for-imported-vault#comment-c8adda2b
… retry - Replace as-cast with isImportFolderMetadata type guard that validates both spaceUri and spaceName - Inline tryParse wrapper; no behaviour change - Add comment explaining why trailing-comma retry exists Co-authored-by: Cursor <cursoragent@cursor.com>
Remove resolveOwnerUserName (which picked the most-frequent node author) in favour of getSpaceOwnerNames, which queries LocalAccess joined with my_accounts to get the actual person account for each space. Also remove the on-load fetchUserNames call that was only needed for the migration flow. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace two-step LocalAccess/my_accounts query with fetchUserNames (already called pre-import in the modal) so plugin.settings.userNames is warm, then resolve the owner name via the first node authorId for the space. Co-authored-by: Cursor <cursoragent@cursor.com>
|
did not look in depth, but immediate reaction to reading this: |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
I truly don’t think deriving ownerUserName from the first imported node’s is the right move here, but I won't block on it.
Also, @trangdoan982 don't forget to update the title/body to match the PR's new intent.
| nodes: ImportableNode[], | ||
| plugin: DiscourseGraphPlugin, | ||
| ): string | undefined => { | ||
| const authorId = nodes.find((n) => n.authorId !== undefined)?.authorId; |
There was a problem hiding this comment.
I don’t think deriving ownerUserName from the first imported node’s authorId matches the intent here. This gives us “an author of one selected node,” not the username/account associated with the source space. If the selected nodes are authored by multiple people, or the first selected node was authored by someone other than the vault account, the import folder could be named after the wrong person.
For Obsidian, the configured username is uploaded as PlatformAccount.name via create_account_in_space, and that account is linked to the vault’s space through LocalAccess(space_id, account_id). Since this flow already has the source spaceId, could we derive the username from that space/account relationship instead? For example, query LocalAccess for this spaceId, join to my_accounts, prefer the relevant agent_type === "person" account, and fall back to spaceName if the lookup is unavailable or ambiguous.
|
Finally reading this seriously. I understand the motivation, and I think it's appropriate for Obsidian vaults which have a defined username. "multi-user" vaults currently refers to Roam vaults, where there is no name collision by design. This may not be true of future multi-user vaults. So… Intuition: I think that vault owner in the name is only appropriate for mono-user vaults. For Roam, maybe we should prefix with the platform, and leave it at that. And always prefix with (foreign) platform. Will we need another distinguisher for an eventual non-roam multi-user platform? We'll see when we get there, but I doubt that most common author will be the right one. (Admin authors? Org? I know we don't have those yet.) |
| plugin: DiscourseGraphPlugin, | ||
| ): string | undefined => { | ||
| const authorId = nodes.find((n) => n.authorId !== undefined)?.authorId; | ||
| return authorId !== undefined |
There was a problem hiding this comment.
I have a concern, let me know if I'm missing something. We are looking at first author; but in obsidian first author is the default author, which is created in src/utils/supabaseContext.ts:101, and unless we've set a username in settings, the default name is the same as the vault name (L80). So we'd basically repeat the vault name in most cases. Can we detect that case, and consider that the author name is undefined if it's identical to the vault name?
| }): Promise<{ folderPath: string; metadata: ImportFolderMetadata } | null> => { | ||
| const importExists = await adapter.exists(IMPORT_ROOT); | ||
| if (!importExists) return map; | ||
| if (!importExists) return null; |
There was a problem hiding this comment.
I think that's the right call, but flagging that means we're not resilient to the user renaming the folder.
3 participants
https://www.loom.com/share/39deddfaa529440d96484590a8c0d53f
Summary
import/from{spaceName}to{owner-username}-{spaceName}so multi-user lab vaults are easier to distinguish.dg.metadata(spaceUri,userName,migrated) for folder identity instead of the folder name, respecting user-customized namesuserNamesTest plan
import/{username}-{vaultName}/import/{vaultName}/folder with.dg.metadatarenames on plugin reloadMade with Cursor