Skip to content

ENG-1656 Rename folder for imported vault - #1130

Open
trangdoan982 wants to merge 13 commits into
mainfrom
eng-1656-rename-folder-for-imported-vault
Open

ENG-1656 Rename folder for imported vault#1130
trangdoan982 wants to merge 13 commits into
mainfrom
eng-1656-rename-folder-for-imported-vault

Conversation

@trangdoan982

@trangdoan982 trangdoan982 commented Jun 18, 2026

Copy link
Copy Markdown
Member

https://www.loom.com/share/39deddfaa529440d96484590a8c0d53f

Note: the Loom predates review feedback — it shows the original {owner}-{spaceName} naming plus the auto-migration that has since been dropped.

Summary

Imported vaults all landed in import/{spaceName}, which is just the source vault's name and does not say whose vault it was. This adds the owner to the folder name and moves folder identity off the folder name entirely.

  • New import folders are named {ownerName}-{spaceName} under import/.
  • The owner name is resolved from space membership — LocalAccess for the source space_id, joined to my_accounts filtered on agent_type = "person". Node authors are deliberately not used: they tell us who wrote a given node, which can differ from the account the source space belongs to.
  • The owner prefix is dropped when it adds nothing: Obsidian defaults the account name to the vault name, which is also the space name, so without this the folder would read my-vault-my-vault for anyone who has not set a username in settings. Same fallback when the owner lookup returns nothing, so a blocked query or a space with no person account degrades to the old {spaceName} naming rather than failing.
  • Folder identity comes from .dg.metadata (spaceUri, spaceName, optional userName) rather than the folder name, so a user who renames an import folder keeps it — the next import finds it again by spaceUri and reuses it instead of creating a duplicate.
  • Pre-existing folders with no .dg.metadata whose name matches the space name are adopted by writing metadata into them, rather than being abandoned.
  • Duplicate metadata pointing at the same spaceUri is resolved by keeping the more recently modified one.

Out of scope

Dropped or deferred after review discussion:

  • Auto-migration of existing import folders — removed (72bf26d). Existing folders keep their names; only new imports use the new scheme. This also removes the migrated metadata flag that came up in review.
  • Username changes after import — flagged by @maparent; the folder is not renamed when the owner's username changes. Separate, low priority.
  • Renaming or moving the import rootIMPORT_ROOT is a fixed path and only its immediate children are scanned, so moving the root or nesting import folders deeper hides existing imports and the next import recreates them. Recorded as a known limitation in the code (0f388d8).
  • Multi-user (Roam) source vaults@maparent's point that owner-in-name suits mono-user vaults and a platform prefix may be the better shape for Roam. Not addressed here since only Obsidian vaults are shared today.

Test plan

Automated (run on the merge result, current main merged in):

  • tsc --noEmit clean for apps/obsidian
  • eslint clean on both changed files (0 errors, 0 new warnings)

Manual — needs a re-check since the naming rules changed after the Loom was recorded:

  • Import from a space whose owner has a username set in settings → import/{username}-{spaceName}/
  • Import from a space whose owner has no username set (name defaults to vault name) → import/{spaceName}/, not import/{spaceName}-{spaceName}/
  • Rename an import folder, then import from that space again → the renamed folder is reused, no duplicate created
  • Existing import/{spaceName}/ with no .dg.metadata → adopted, metadata written, folder not renamed

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>
@vercel

vercel Bot commented Jun 18, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
discourse-graph Skipped Skipped Jul 30, 2026 8:21pm

Request Review

@linear-code

linear-code Bot commented Jun 18, 2026

Copy link
Copy Markdown

ENG-1656

@supabase

supabase Bot commented Jun 18, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@graphite-app

graphite-app Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

PR size/scope check

This PR is over our review-size guideline.

  • Recommended: ~200 lines changed
  • Acceptable limit: up to 400 lines when well-scoped/self-contained
  • Preferred file count: fewer than 5 files

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:

  • What single problem this PR solves
  • Why the files/changes are coupled

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Open in Devin Review

Comment thread apps/obsidian/src/utils/importNodes.ts Outdated
Comment thread apps/obsidian/src/utils/importFolderNaming.ts Outdated
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>

@mdroidian mdroidian left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 this migrated flag existed, the metadata will not have migrated: true, so this branch treats the custom basename as stale and renames it to the generated user-space name as soon as a username is available. Since the previous resolver intentionally used .dg.metadata to 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

  • resolveUserNameFromFolder returns the username for the first markdown file (in vault iteration order) with a resolvable authorId, while imports use resolveOwnerUserName to 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, while migrateImportFolderNames() appears to use the first known authorId found in existing files. That means a space like discourse-graphs could 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

Comment thread apps/obsidian/src/types.ts Outdated
Comment thread apps/obsidian/src/utils/importFolderMetadata.ts Outdated
Comment thread apps/obsidian/src/utils/importFolderMetadata.ts Outdated
Comment thread apps/obsidian/src/utils/importFolderMetadata.ts Outdated
Comment thread apps/obsidian/src/utils/importFolderMetadata.ts Outdated
@mdroidian

mdroidian commented Jun 21, 2026

Copy link
Copy Markdown
Member

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 mdroidian left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

… 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>
@trangdoan982
trangdoan982 requested a review from mdroidian June 22, 2026 13:05

Copy link
Copy Markdown
Collaborator

did not look in depth, but immediate reaction to reading this: migrated: true is an anti-pattern, it should be version: 1, so we can know to do subsequent migrations.

@mdroidian mdroidian left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread apps/obsidian/src/utils/importNodes.ts Outdated

Copy link
Copy Markdown
Collaborator

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.)
All that said, I won't block on this, as we're only sharing obsidian vaults at this point in time and I'm happy with this heuristic for obsidian vaults. But I want to flag that the "most common author" may not be the right intuition to extend. (Is there a page where options are enumerated and discussed?)

@maparent maparent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the author name comment, lgtm.
I also want to flag that we're not handling the case where the username changes. I also think that's a different issue, and of low priority.

Comment thread apps/obsidian/src/utils/importNodes.ts Outdated
}): Promise<{ folderPath: string; metadata: ImportFolderMetadata } | null> => {
const importExists = await adapter.exists(IMPORT_ROOT);
if (!importExists) return map;
if (!importExists) return null;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's the right call, but flagging that means we're not resilient to the user renaming the folder.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth splitting into two cases, because the answer differs:

Renaming an individual import folder is handled. Folder identity is carried by .dg.metadata (spaceUri), not by the basename. findImportFolderBySpaceUri scans every child of import/ and matches on spaceUri, so import/alice-vaultimport/whatever is found again on the next import and reused rather than duplicated. That is the reason the auto-rename migration could be dropped in 72bf26d.

Renaming or moving the import root is not handled — and I think that is the case you are pointing at, since the line is the if (!importExists) return null; guard. IMPORT_ROOT is the fixed string "import" and adapter.list is non-recursive, so renaming the root to imported/, moving it, or nesting import folders one level deeper makes existing imports invisible and the next import recreates them under a fresh import/. Same if .dg.metadata is deleted.

Not fixing that here — making the root configurable or the scan recursive is a bigger change than this ticket — but I recorded it as a known limitation on IMPORT_ROOT in 0f388d8 so it does not have to be rediscovered. Happy to file a follow-up if you think it is worth handling.

trangdoan982 and others added 2 commits July 30, 2026 16:19
…authors

Address review feedback on owner name resolution:

- Replace the first-node authorId lookup with a LocalAccess -> my_accounts
  query scoped to the source space. Node authors identify who wrote a given
  node, which can differ from the account the source space belongs to, so the
  folder could be named after the wrong person.
- Omit the owner prefix when it matches the space name. Obsidian defaults the
  account name to the vault name, which is also the space name, so the folder
  would otherwise read "my-vault-my-vault" for anyone who has not set a
  username in settings.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Folder identity lives in .dg.metadata, so renaming an individual import folder
is already safe. The fixed, non-recursive import root is not — record that.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@trangdoan982 trangdoan982 changed the title ENG-1656: Rename imported vault folders to owner-username-spaceName ENG-1656 Rename folder for imported vault Jul 30, 2026
@trangdoan982

trangdoan982 commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

Addressed the outstanding review feedback — summary of what changed and what I deliberately did not change.

Owner name no longer comes from node authors (@mdroidian, @maparent). resolveOwnerUserName is gone. getSpaceOwnerNames now queries LocalAccess for the source space_id and joins to my_accounts filtered on agent_type = "person". @maparent — this also answers your "most common author may not be the right intuition to extend" point: there is no author heuristic left to extend. The name comes from space membership, so it stays correct for a space whose nodes were written by several people.

Owner prefix suppressed when redundant (@maparent). Confirmed your reading of supabaseContext.ts — the account is created with name: plugin.settings.username || vaultName and the space with name: vaultName, so the two are identical unless a username is set. buildImportFolderBasename now compares the sanitized forms and returns the bare space name when they match, so the default case is import/my-vault, not import/my-vault-my-vault. The real owner name is still recorded in .dg.metadata; only the basename drops it.

migrated: true is moot (@maparent). The auto-migration was removed in 72bf26d, and with it the flag — so the version: 1 vs migrated: true question does not apply to the current diff. ImportFolderMetadata is now just spaceUri, spaceName, and optional userName. If versioned metadata is wanted later, agreed it should be a monotonic version rather than a boolean.

Username changes after import (@maparent) — not handled, agreed it is a separate low-priority issue. Noted in the PR body under out of scope rather than silently dropped.

Multi-user / Roam vaults (@maparent) — not addressed. Let me know if team has decided on a naming convention for Roam-Obsidian sync for folder name

Known limitation recorded in code: IMPORT_ROOT is a fixed path and only its immediate children are scanned, so renaming an individual import folder is safe (identity is .dg.metadata/spaceUri) but renaming or moving the import root is not. Documented on the constant in 0f388d8.

Also updated the title to the repo convention (ENG-1656 Rename folder for imported vault) and rewrote the body, which still described the removed migration flow — thanks for the nudge, @mdroidian.

Merged current main in (the branch was 92 commits behind) and re-verified against the merge result: tsc --noEmit clean, eslint clean on both changed files, all CI checks green.

@maparent this needs your re-review to clear the change request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants