chore: tune default config#244
Merged
Merged
Conversation
Closed
5 tasks
📝 WalkthroughWalkthroughThis PR introduces a storage abstraction (IFilesStorage) with React contexts (FilesStorageContext, VaultStorage), replaces Electron-specific file controllers with RootedFS-backed IFilesStorage across the app, removes workspaceId from FilesIntegrityController, and adds path utilities and RootedFS tests. Database filename for profiles changed to Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (React)
participant FilesCtx as FilesStorageContext
participant ElectronFS as ElectronFilesController
participant Rooted as RootedFS
participant Integrity as FilesIntegrityController
participant DB as Config/ProfilesManager
App->>FilesCtx: mount provider with ElectronFilesController (storageApi, "/")
App->>App: useFilesStorage() -> FilesStorageContext value
App->>Rooted: new RootedFS(filesStorage, getWorkspaceFilesPath(id))
App->>DB: construct Config/ProfilesManager using RootedFS
App->>Integrity: construct FilesIntegrityController(filesStorage, { files, attachments })
Integrity->>Rooted: list/get/delete using rooted paths ("/{id}" resolution)
DB->>FilesCtx: read/write via filesStorage (RootedFS resolves to host paths)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/features/integrity/FilesIntegrityController.ts (1)
26-40:⚠️ Potential issue | 🟠 MajorNormalize storage entries before comparing them to file ids.
These methods now require
list()to return entries shaped exactly like/${id}.IFilesStoragedoes not guarantee that, andRootedFS('/')in particular returns bare names, so valid files can be treated as orphaned/missing and then deleted from the FS or DB. Compare normalized ids instead of raw path strings.♻️ Proposed normalization
+const normalizeStoragePath = (path: string) => path.replace(/^\/+/, ''); + public async deleteOrphanedFilesInFs() { - const filePathsInDB = await this.controllers.files + const fileIdsInDB = await this.controllers.files .query() - .then((files) => new Set(files.map((file) => `/${file.id}`))); + .then((files) => new Set(files.map((file) => file.id))); const filesInStorage = await this.files.list(); const orphanedFilePaths = filesInStorage.filter((filePath) => { - const basePath = '/'; - - // Skip files out of workspace directory and a workspace directory itself - if (!filePath.startsWith(basePath) || filePath === basePath) return false; - - return !filePathsInDB.has(filePath); + const fileId = normalizeStoragePath(filePath); + if (fileId === '') return false; + return !fileIdsInDB.has(fileId); }); await this.files.delete(orphanedFilePaths); } ... public async fixFiles() { - const filesInStorage = await this.files.list().then((files) => new Set(files)); + const filesInStorage = await this.files + .list() + .then((files) => new Set(files.map(normalizeStoragePath))); ... - if (!filesInStorage.has(`/${file.id}`)) { + if (!filesInStorage.has(file.id)) { lostFileIds.push(file.id); } }); }Also applies to: 88-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/features/integrity/FilesIntegrityController.ts` around lines 26 - 40, FilesIntegrityController currently compares raw storage paths to DB ids, causing valid files (e.g., RootedFS returning bare names) to be treated as orphaned; change the comparison to normalize both sides to the same id form. Build the DB id set from this.controllers.files.query() as plain ids (files.map(f => f.id) or new Set(files.map(f => f.id))), then map filesInStorage to extract/normalize the id (strip leading/trailing slashes or directory prefixes so "foo", "/foo", and "/foo/" all become "foo") and use that normalized id to check presence in the DB set before deleting; update the orphan detection in FilesIntegrityController (the filePathsInDB, filesInStorage, orphanedFilePaths logic) and apply the same normalization in the other occurrence mentioned (lines 88-96).
🧹 Nitpick comments (2)
src/core/features/integrity/FilesIntegrityController.test.ts (1)
27-30: Add one regression case with the production storage shape.After removing
workspaceIdfromFilesIntegrityController, callers must pass workspace-scoped storage. This test still uses a flat mock, so it won't catch path-normalization regressions from the rooted setup used insrc/features/App/Profile/services/useFilesIntegrityService.tsx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/features/integrity/FilesIntegrityController.test.ts` around lines 27 - 30, The test instantiates FilesIntegrityController with a flat mock storage shape but after removing workspaceId callers must use workspace-scoped (rooted) storage, so add a regression test that mirrors the production storage shape used by useFilesIntegrityService.tsx: create a storage mock that nests files/attachments under the workspace root (or includes workspace-scoped paths) and pass that into new FilesIntegrityController(fileManager, { files, attachments }) to verify path-normalization and rooted lookups; ensure the new test exercises the same code paths as FilesIntegrityController (e.g., normalization/lookup functions) to catch regressions.src/features/files/index.ts (1)
11-18: Make the no-root case a true pass-through.
useVaultStorage()wraps the context value innew RootedFS(storage, '/')whenrootis omitted. That changeslist()semantics even though the caller did not request a sub-root, so the default case is not actually equivalent to the underlying vault storage. Returningstoragedirectly keeps the hook predictable and avoids more slash-format drift.♻️ Proposed change
export const useVaultStorage = (root?: string) => { const storage = useContext(VaultStorage); if (!storage) throw new Error('Vault storage is not provided'); return useMemo( - () => new RootedFS(storage, root ? getResolvedPath(root, '/') : '/'), + () => (root ? new RootedFS(storage, getResolvedPath(root, '/')) : storage), [root, storage], ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/files/index.ts` around lines 11 - 18, The hook useVaultStorage wraps the context VaultStorage in a RootedFS('/') when root is undefined, changing semantics (e.g., list()) compared to returning the raw storage; modify useVaultStorage so that if root is omitted or falsy it returns the original storage directly, and only constructs new RootedFS(storage, getResolvedPath(root, '/')) when a non-empty root is provided—update the return logic around the existing useMemo and references to RootedFS, storage, root, and getResolvedPath accordingly to preserve pass-through behavior for the no-root case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/features/files/FilesController.ts`:
- Around line 15-19: The constructor currently accepts a generic IFilesStorage
and a workspace string but relies on callers to root the storage to that
workspace; update FilesController to enforce workspace scoping by deriving a
rooted storage inside the constructor (or by changing the parameter to a
workspace-scoped storage type) so DB SQL and filesystem operations always target
the same logical workspace: use the existing constructor, workspace and
filesStorage symbols to create an internal rootedFilesStorage instance (rooted
to workspace) and replace uses of filesStorage in methods that map to the
SQL-scoped workspace (the calls referenced near lines 35, 59, and 79) with
rootedFilesStorage; ensure the constructor validates or normalizes the workspace
and throws or logs if rooting fails so the invariant is enforced by
FilesController rather than callers.
In `@src/features/App/Profile/services/useFilesIntegrityService.tsx`:
- Around line 7-8: The code nests a second RootedFS on top of useVaultStorage()
which already returns a RootedFS, causing RootedFS.list() to strip the leading
'/' and produce an empty file set; update useFilesIntegrityService.tsx to stop
wrapping the result of useVaultStorage() in another RootedFS — either use the
returned RootedFS directly when calling getWorkspaceFilesPath()/list() or obtain
the underlying storage (not RootedFS) before creating a single RootedFS for the
workspace; ensure fixFiles() uses that single RootedFS instance (references:
useVaultStorage(), RootedFS.list(), getWorkspaceFilesPath(), fixFiles()) so
workspace files are discovered correctly.
In `@src/features/App/Profiles/hooks/useProfileContainers.ts`:
- Line 70: Before creating RootedFS with `/vaults/${profile.id}`, detect a
legacy vault at `/${profile.id}/deepink.db` (and its adjacent key file) and
perform a one-time migration: if legacy files exist and
`/vaults/${profile.id}/vault.db` does not, create the `/vaults/${profile.id}`
directory (via the same `files` API), move/rename `/${profile.id}/deepink.db` to
`/vaults/${profile.id}/vault.db` and move the key file alongside it, then
continue to instantiate RootedFS as `new RootedFS(files,
\`/vaults/${profile.id}\`)`; apply the same migration logic before the second
open that currently uses the same path near lines 114-115 so existing profiles
aren’t overwritten or left unopenable.
---
Outside diff comments:
In `@src/core/features/integrity/FilesIntegrityController.ts`:
- Around line 26-40: FilesIntegrityController currently compares raw storage
paths to DB ids, causing valid files (e.g., RootedFS returning bare names) to be
treated as orphaned; change the comparison to normalize both sides to the same
id form. Build the DB id set from this.controllers.files.query() as plain ids
(files.map(f => f.id) or new Set(files.map(f => f.id))), then map filesInStorage
to extract/normalize the id (strip leading/trailing slashes or directory
prefixes so "foo", "/foo", and "/foo/" all become "foo") and use that normalized
id to check presence in the DB set before deleting; update the orphan detection
in FilesIntegrityController (the filePathsInDB, filesInStorage,
orphanedFilePaths logic) and apply the same normalization in the other
occurrence mentioned (lines 88-96).
---
Nitpick comments:
In `@src/core/features/integrity/FilesIntegrityController.test.ts`:
- Around line 27-30: The test instantiates FilesIntegrityController with a flat
mock storage shape but after removing workspaceId callers must use
workspace-scoped (rooted) storage, so add a regression test that mirrors the
production storage shape used by useFilesIntegrityService.tsx: create a storage
mock that nests files/attachments under the workspace root (or includes
workspace-scoped paths) and pass that into new
FilesIntegrityController(fileManager, { files, attachments }) to verify
path-normalization and rooted lookups; ensure the new test exercises the same
code paths as FilesIntegrityController (e.g., normalization/lookup functions) to
catch regressions.
In `@src/features/files/index.ts`:
- Around line 11-18: The hook useVaultStorage wraps the context VaultStorage in
a RootedFS('/') when root is undefined, changing semantics (e.g., list())
compared to returning the raw storage; modify useVaultStorage so that if root is
omitted or falsy it returns the original storage directly, and only constructs
new RootedFS(storage, getResolvedPath(root, '/')) when a non-empty root is
provided—update the return logic around the existing useMemo and references to
RootedFS, storage, root, and getResolvedPath accordingly to preserve
pass-through behavior for the no-root case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a49679e-1dd1-41e7-ba28-8524f632264e
📒 Files selected for processing (13)
src/core/features/files/FilesController.tssrc/core/features/integrity/FilesIntegrityController.test.tssrc/core/features/integrity/FilesIntegrityController.tssrc/features/App/Profile/services/useFilesIntegrityService.tsxsrc/features/App/Profiles/hooks/useProfileContainers.tssrc/features/App/Profiles/index.tsxsrc/features/App/Settings/sections/WorkspaceSettings.tsxsrc/features/App/Workspace/useWorkspace.tssrc/features/App/index.tsxsrc/features/App/useProfilesList.tssrc/features/files/index.tssrc/features/files/paths.tssrc/windows/main/renderer.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/features/files/RootedFS.ts (1)
33-42: Avoid Iterator helpers here unless the shipped runtime guarantees them.test('iterator helpers are available in the target runtime', () => { const iter = ['a'].values(); expect(typeof iter.map).toBe('function'); expect(typeof iter.filter).toBe('function'); expect(typeof iter.toArray).toBe('function'); });
filesis already an array. If the app's Electron/Node target lacks Iterator helpers,list()will throw at runtime; plain arraymap/filteravoids that dependency entirely.Suggested simplification
return files - .values() .map((path) => { const rootedPath = getRootedPath(path, this.root); return this.root === '/' ? rootedPath : joinPathSegments(rootedPath.split('/').slice(rootSegmentsCount)); }) - .filter((path) => path !== '/') - .toArray(); + .filter((path) => path !== '/');Based on learnings: when flagging a bug in TypeScript/TSX, include a minimal reproducing unit test first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/features/files/RootedFS.ts` around lines 33 - 42, The code in RootedFS.ts uses Iterator helpers on files (files.values().map(...).filter(...).toArray()) which may not exist in some runtimes; replace that iterator-helper chain with plain Array methods: use files.map(...) to transform each path with getRootedPath(path, this.root) and then conditional slicing via joinPathSegments(...slice(rootSegmentsCount)), followed by Array.prototype.filter to remove '/' and return the resulting array (no toArray call). Ensure you reference the existing symbols getRootedPath, joinPathSegments, rootSegmentsCount and the variable files when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/features/files/RootedFS.test.ts`:
- Around line 25-27: The tests use `.resolves.not.toThrow()` against
promise-returning void functions (e.g., workspaceFS.write and workspaceFS.mkdir)
which is invalid; replace each `.resolves.not.toThrow()` with
`.resolves.toBeUndefined()` for assertions that await Promise<void> (apply to
the occurrences around workspaceFS.write and workspaceFS.mkdir at the other
mentioned locations).
In `@src/core/features/files/RootedFS.ts`:
- Around line 13-15: The getHostPath method currently collapses escaped paths
into the root which allows mutated calls to hit the root; modify getHostPath to
resolve/normalize the joined path (using joinPathSegments/getRootedPath
behavior) and detect when the resulting host path is outside of this.root, and
in that case throw a descriptive error (e.g., "path escapes root") so callers
like write(), get(), and delete() reject traversal inputs instead of silently
mapping them to the root.
---
Nitpick comments:
In `@src/core/features/files/RootedFS.ts`:
- Around line 33-42: The code in RootedFS.ts uses Iterator helpers on files
(files.values().map(...).filter(...).toArray()) which may not exist in some
runtimes; replace that iterator-helper chain with plain Array methods: use
files.map(...) to transform each path with getRootedPath(path, this.root) and
then conditional slicing via joinPathSegments(...slice(rootSegmentsCount)),
followed by Array.prototype.filter to remove '/' and return the resulting array
(no toArray call). Ensure you reference the existing symbols getRootedPath,
joinPathSegments, rootSegmentsCount and the variable files when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a69b003b-e1a1-404c-88f3-95582465f166
📒 Files selected for processing (6)
src/core/features/files/RootedFS.test.tssrc/core/features/files/RootedFS.tssrc/state/redux/profiles/profiles.tssrc/state/redux/settings/settings.tssrc/utils/fs/paths.test.tssrc/utils/fs/paths.ts
This was referenced Mar 16, 2026
4 tasks
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
1 participant
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.
Closes #239
Summary by CodeRabbit
Refactor
New Features
Chores