feat: configurable default folders for media and per-type new notes#521
feat: configurable default folders for media and per-type new notes#521yusoufu wants to merge 23 commits into
Conversation
Double-clicking any image inside the editor opens it in a centered shadcn Dialog at up to 90vw / 90vh. Esc, backdrop click, or the overlay close it. Single-click is left alone so BlockNote selection keeps working. The shared dblclick listener on the editor container picks up image targets via getDoubleClickedImageSrc, which also filters tracking pixels (<16 px naturalWidth/Height) so empty inline GIFs don't open a blank viewer.
Adds two global Settings — default_image_folder and default_video_folder — that route pasted/dropped images and videos into a user-chosen vault folder (falls back to attachments/ when null). Generalises image.rs to a shared media pipeline that also handles videos via new save_video and copy_video_to_vault Tauri commands. Adds an opt-in _default_folder field on type-note frontmatter. When set, new notes of that type are created in the named subfolder; when unset, they continue to land at the vault root preserving the flat-vault default. Existing notes are not moved and changing a note's type does not relocate the file. Adds a shared shadcn FolderPicker used by the Settings "Media" section and the per-type Customize popover. Validation lives in a single Rust helper normalize_vault_relative_folder that rejects absolute paths, .. traversal and empty input across settings, frontmatter and media writes. See ADR-0085 for design decisions and the deliberate non-supersession of ADR-0006 (flat vault remains the default). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The test created its fixture inside std::env::current_dir(), so when the test binary ran from a checkout under a dotfile-prefixed parent (e.g. .claude/worktrees/...), search_vault's hidden-component filter silently dropped the fixture and the assertion failed with 0 vs 1 results. Using tempfile's default OS tempdir keeps the path free of hidden segments regardless of where the workspace lives. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The Claude Code worktree directory was being surfaced as an untracked path in every git status. It's purely a local agent scratch area and should never reach the index. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| UnusedCode | 7 medium |
| BestPractice | 11 medium |
| ErrorProne | 3 medium 7 high |
| Security | 2 high |
| Complexity | 2 medium |
🟢 Metrics 172 complexity · 22 duplication
Metric Results Complexity 172 Duplication 22
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
Although the project remains 'Up to Standards' according to Codacy, this PR introduces 25 new static analysis issues across four files, with src/components/FolderPicker.tsx accounting for 16 of them. There is a lack of evidence regarding the verification of the media pipeline and path validation logic (e.g., rejecting directory traversal). These areas represent high-risk logic that should be covered by automated tests before merging. Furthermore, the specialized agents reported an inability to verify the code changes, suggesting that a manual verification of the implementation logic for 'default_folder' and media fallback is required.
About this PR
- The implementation of the path validation logic (preventing absolute paths and directory traversal) and the media storage pipeline could not be verified by automated agents. Please ensure these have been manually reviewed or that the missing test scenarios have been addressed.
- There are 16 new issues in
src/components/FolderPicker.tsxand 5 new issues insrc/components/ImageLightbox.tsx. These should be resolved to maintain code health, especially in the shared FolderPicker component.
Test suggestions
- Verify image paste lands in configured subfolder
- Verify media fallback to 'attachments/' when settings are empty
- Verify note creation respects '_default_folder' frontmatter
- Verify path validation rejects '../' traversal and absolute paths
- Verify FolderPicker component rendering and selection logic
- Verify the fix for the flaky Rust search test in dotfile-prefixed paths
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify image paste lands in configured subfolder
2. Verify media fallback to 'attachments/' when settings are empty
3. Verify note creation respects '_default_folder' frontmatter
4. Verify path validation rejects '../' traversal and absolute paths
5. Verify FolderPicker component rendering and selection logic
6. Verify the fix for the flaky Rust search test in dotfile-prefixed paths
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
There was a problem hiding this comment.
Pull Request Overview
This PR introduces configurable media folders and per-type note storage, but contains critical performance and accessibility issues that should be addressed before merging.
Codacy analysis indicates the PR is not up to standards, with 28 new issues. Most notably, the useImageDrop hook uses a manual string concatenation loop for base64 conversion which is highly inefficient for the large video files this PR now supports; this could lead to memory exhaustion or main-thread blocking.
From an architectural standpoint, there is a discrepancy between the ADR/documentation—which states absolute paths will be rejected—and the implementation, which instead normalizes them by stripping slashes. Furthermore, several accessibility gaps were identified in the new FolderPicker component, including non-compliant ARIA listbox behavior and disconnected form labels. Finally, the Image Lightbox feature was added without being documented in the PR description.
About this PR
- Discrepancy in path handling: The implementation normalizes paths (stripping slashes) whereas the ADR/documentation suggests they should be rejected. Please align the documentation with the code or update the validation to reject absolute paths as planned.
- The PR introduces a new Image Lightbox feature and double-click handler that are not mentioned in the PR description or summary. Please update the description to reflect these changes.
2 comments outside of the diff
src/components/sidebar/SidebarSections.tsx
line 273🟡 MEDIUM RISK
Suggestion: The button element should have an explicittype="button"attribute to prevent it from defaulting to a submit button.
src/hooks/useNoteCreation.ts
line 62⚪ LOW RISK
Suggestion: The callback passed toforEachimplicitly returns a value becauseSet.addreturns the updated set. Use curly braces to clarify this is a side-effect-only operation.
Test suggestions
- Validate Rust folder normalization logic (rejection of traversal segments and redundancy checks)
- Verify media save pipeline correctly resolves custom folders and handles fallback to 'attachments'
- Verify TypeScript folder picker validation and clear/escape interactions
- Verify note creation logic correctly computes absolute paths using type-specific folders
- Verify image lightbox double-click detection and size filtering for tracking pixels
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| }) { | ||
| return ( | ||
| <div className="flex flex-col gap-1.5" data-testid={testId}> | ||
| <label className="text-xs font-medium text-muted-foreground">{label}</label> |
There was a problem hiding this comment.
🔴 HIGH RISK
The <label> element is not programmatically associated with any input control. Update FolderPicker to accept an id prop and use it in the label's htmlFor attribute to ensure screen reader compatibility.
| {matches.length === 0 ? ( | ||
| <div className="px-2 py-1.5 text-xs text-muted-foreground">No matching folders</div> | ||
| ) : ( | ||
| <ul role="listbox" className="flex flex-col"> |
There was a problem hiding this comment.
🔴 HIGH RISK
The <ul> element is assigned role="listbox" but lacks the necessary keyboard interaction logic (e.g., arrow key navigation) to be fully accessible. Implement keyboard navigation or use a semantically neutral container if strict ARIA compliance isn't intended.
| const buf = await file.arrayBuffer() | ||
| const bytes = new Uint8Array(buf) | ||
| let binary = '' | ||
| for (let i = 0; i < bytes.length; i++) binary += String.fromCharCode(bytes[i]) |
There was a problem hiding this comment.
🔴 HIGH RISK
The manual character-by-character string concatenation loop is inefficient for large media files like videos. Use FileReader.readAsDataURL and strip the metadata prefix to perform the base64 conversion natively and avoid blocking the main thread.
| return None; | ||
| } | ||
|
|
||
| if stripped.starts_with('/') { |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: This condition is redundant and unreachable because the trim_matches('/') call on line 94 has already removed all leading and trailing slashes from the string.
Resolve conflicts between the configurable default-folder feature (default_image_folder, default_video_folder, per-type _default_folder) and upstream additions (image lightbox, gitignored visibility, all-notes file toggles, sidebar refactor with extracted runtime hooks, frontmatter key registry, mermaid/whiteboard support, localization). - Settings struct, normalization, and persistence now carry both feature sets; FolderPicker is wired through SettingsPanel and the per-type customize popover. - Sidebar uses upstream's SidebarRuntimeNavigation/SidebarInteractionOverlays while threading folders + onChangeDefaultFolder through the customize overlay. - Frontmatter parsing adopts upstream's FrontmatterKey registry; added a rule for _default_folder so the field continues to round-trip. - Backfill OS tempdir fix to the two scan_cmds search tests and the gitignored-search test that share the same dotfile-path pitfall fixed by fb132ee for the h1 title test. - Image drop hook now uses upstream's useTauriDragDropEvent + helper decomposition while preserving video media support.
The merged VaultEntry contract requires defaultFolder; the LOADING_BREADCRUMB_ENTRY fixture was missing it, causing tsc -b to fail.
Resolve conflicts with upstream's `feat: add direct AI model providers` which added default_ai_target and ai_model_providers settings alongside yusoufu's default_image_folder/default_video_folder media routing. - Settings struct, normalization, persistence, locales, and docs now carry both feature sets. - SettingsPanel adds folders to SettingsBodyFromDraft and renders MediaSettingsSection alongside upstream's grouped Appearance + Language section. - Editor.tsx adopts upstream's runtime variable while keeping image and video folder pass-through to useEditorSetup.
- Extract useFolderPickerState hook and FolderMatchesList component to drop FolderPicker's cyclomatic complexity below the gate. - Switch the suggestion list from `<ul role="listbox">` to a `<div>` with proper `role="option"` buttons so the role/element pairing is valid. - Mark Media Settings row's visual label aria-hidden because the FolderPicker input already exposes its own accessible name via aria-label.
Summary
default_image_folderanddefault_video_folderglobal settings to route pasted/dropped media into a user-chosen vault folder (falls back toattachments/when unset)image.rsinto a shared media pipeline that also handles videos via newsave_video/copy_video_to_vaultTauri commands_default_folderfield on type-note frontmatter — when set, new notes of that type are created in the named subfolder; existing notes are not movedFolderPickercomponent used by Settings ("Media" section) and the per-type Customize popovernormalize_vault_relative_folder) in a single Rust helper rejects absolute paths,..traversal, and empty input across settings, frontmatter, and media writes.claude/worktrees/…) by using the OS tempdir for the fixtureSee ADR-0085 for design decisions.
Test plan
default_image_folderset → lands in correct subfolderattachments/_default_folderset → note created in that folder_default_folder→ note created at vault root../escapein FolderPicker → validation rejects itcargo testpasses (including the fixed search test)pnpm testpasses includingFolderPicker.test.tsx,useImageDrop.test.ts,useNoteCreation.test.ts🤖 Generated with Claude Code