Conversation
WalkthroughA new shared file processing module is created in Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Editor/<br/>Component
participant Shared as processFileToAssets<br/>(Shared Utility)
participant Tauri as Tauri Commands
participant Result as Return Result
Editor->>Shared: Call with sourcePath,<br/>projectPath, copyStrategy
alt copyStrategy: 'always'
Shared->>Tauri: copy_file_to_assets<br/>or<br/>copy_file_to_assets_with_override
Tauri-->>Shared: copied file path
Shared->>Shared: Extract filename,<br/>normalize path
Shared-->>Result: {relativePath,<br/>wasCopied: true,<br/>filename}
else copyStrategy: 'only-if-outside-project'
Shared->>Tauri: is_path_in_project
alt File outside project
Tauri-->>Shared: false
Shared->>Tauri: copy_file_to_assets<br/>or<br/>copy_file_to_assets_with_override
Tauri-->>Shared: copied file path
Shared-->>Result: {relativePath,<br/>wasCopied: true,<br/>filename}
else File inside project
Tauri-->>Shared: true
Shared->>Tauri: get_relative_path
Tauri-->>Shared: existing relative path
Shared-->>Result: {relativePath,<br/>wasCopied: false,<br/>filename}
end
end
Result-->>Editor: Use result.relativePath<br/>for markdown/frontmatter
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes span multiple files with distinct concerns—new module creation (types, constants, implementation, tests), refactoring of existing integrations, and documentation. The logic is fairly straightforward with clear intent, but understanding the interaction between the shared utility and Tauri invocations, plus verifying that both copy strategies preserve their respective behavioral semantics ('always' vs. 'conditional'), requires careful review across cohorts. Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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: 1
🧹 Nitpick comments (6)
src/lib/files/types.ts (1)
7-22: ExtractCopyStrategynamed type for reuse.Improves ergonomics across callers and tests; keeps options stable.
Apply:
import type { ProjectSettings } from '../project-registry/types' import type { ImageExtension } from './constants' +/** Strategy for copying files */ +export type CopyStrategy = 'always' | 'only-if-outside-project' + /** * Options for processing files to assets directory */ export interface ProcessFileToAssetsOptions { /** Path to the source file to process */ sourcePath: string /** Path to the project root */ projectPath: string /** Name of the collection (for collection-specific asset directory resolution) */ collection: string /** Current project settings (for path overrides) */ projectSettings?: ProjectSettings | null /** * Strategy for copying files: * - 'always': Always copy and rename (editor drag-and-drop) * - 'only-if-outside-project': Only copy if file is outside project (frontmatter fields) */ - copyStrategy: 'always' | 'only-if-outside-project' + copyStrategy: CopyStrategy }And re-export in the barrel (see comment on index.ts).
src/lib/files/index.ts (1)
3-7: Re-exportCopyStrategyfrom the barrel.Keeps consumers on the public API.
Apply:
export type { ProcessFileToAssetsOptions, ProcessFileToAssetsResult, ImageExtension, + CopyStrategy, } from './types'src/lib/files/fileProcessing.test.ts (2)
1-4: Import via the public barrel to test the exported surface.Prevents drift if internals move; matches how consumers import.
Apply:
-import { describe, it, expect, vi, beforeEach } from 'vitest' -import { processFileToAssets } from './fileProcessing' -import type { ProcessFileToAssetsOptions } from './types' +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { processFileToAssets } from '.' +import type { ProcessFileToAssetsOptions } from '.'
40-43: Optional: prefer resetAllMocks in beforeEach.Ensures both call history and implementations are reset between tests.
Apply:
- beforeEach(() => { - vi.clearAllMocks() + beforeEach(() => { + vi.resetAllMocks() vi.mocked(getEffectiveAssetsDirectory).mockReturnValue('src/assets') })src/lib/editor/dragdrop/fileProcessing.ts (1)
20-31: Consider consolidatingextractFilenamewith the shared module.The
extractFilenamefunction is duplicated in both this file and the sharedsrc/lib/files/fileProcessing.ts(lines 94-98). While the implementations differ slightly in edge case handling (this version explicitly handles empty filenames from trailing separators), this duplication contradicts the refactoring goal of eliminating duplicated logic.Option 1 (Recommended): Export
extractFilenamefrom the shared module and import it here:In
src/lib/files/fileProcessing.ts, change line 94 from:-function extractFilename(filePath: string): string { +export function extractFilename(filePath: string): string { const parts = filePath.split(/[/\\]/) const filename = parts[parts.length - 1] - return filename || filePath + // If the filename is empty (path ends with separator), return empty string + if (filename === '') { + return '' + } + return filename || filePath }Then in this file, import and use it:
import { ProcessedFile } from './types' import { useProjectStore } from '../../../store/projectStore' -import { processFileToAssets, IMAGE_EXTENSIONS_WITH_DOTS } from '../../files' +import { processFileToAssets, IMAGE_EXTENSIONS_WITH_DOTS, extractFilename } from '../../files' -/** - * Extract filename from a file path - * @param filePath - Full file path - * @returns Just the filename portion - */ -export const extractFilename = (filePath: string): string => { - // Handle both Unix and Windows path separators - const parts = filePath.split(/[/\\]/) - const filename = parts[parts.length - 1] - - // If the filename is empty (path ends with separator), return empty string - if (filename === '') { - return '' - } - - return filename || filePath -}Option 2: Keep the duplication if there's a specific reason for different behavior in the editor context (though this seems unlikely).
docs/tasks-todo/task-1-refactor-file-copying-logic.md (1)
253-954: Excellent documentation of the refactoring process!The implementation plan and summary are thorough and valuable:
- Clear phase breakdown with acceptance criteria
- Accurate code examples matching the actual implementation
- Comprehensive tracking of completed work
- Well-documented benefits (125+ lines of duplication removed, single source of truth)
- Risk mitigation and rollback plans included
This documentation will be invaluable for understanding the refactoring decisions and maintaining the code going forward.
Optional: For consistency with standard formatting language naming, consider capitalizing "Markdown" (proper noun) on line 924:
-- Preserved editor-specific concerns: markdown formatting, error fallback +- Preserved editor-specific concerns: Markdown formatting, error fallback
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/developer/architecture-guide.md(1 hunks)docs/tasks-todo/task-1-refactor-file-copying-logic.md(1 hunks)src/components/frontmatter/fields/ImageField.tsx(3 hunks)src/lib/editor/dragdrop/fileProcessing.test.ts(11 hunks)src/lib/editor/dragdrop/fileProcessing.ts(4 hunks)src/lib/editor/urls/detection.ts(2 hunks)src/lib/files/constants.ts(1 hunks)src/lib/files/fileProcessing.test.ts(1 hunks)src/lib/files/fileProcessing.ts(1 hunks)src/lib/files/index.ts(1 hunks)src/lib/files/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
docs/developer/architecture-guide.md
📄 CodeRabbit inference engine (CLAUDE.md)
Update docs/developer/architecture-guide.md whenever adding new architectural patterns
Files:
docs/developer/architecture-guide.md
src/lib/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Write unit tests for src/lib modules as .test.ts files under src/lib
Files:
src/lib/files/fileProcessing.test.tssrc/lib/editor/dragdrop/fileProcessing.test.ts
src/components/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Direct Store Pattern in React components (access Zustand stores directly) and never use React Hook Form
Files:
src/components/frontmatter/fields/ImageField.tsx
🧬 Code graph analysis (4)
src/lib/files/fileProcessing.ts (3)
src/lib/files/types.ts (2)
ProcessFileToAssetsOptions(7-22)ProcessFileToAssetsResult(27-34)src/lib/project-registry/path-resolution.ts (1)
getEffectiveAssetsDirectory(58-79)src/lib/constants.ts (1)
ASTRO_PATHS(2-8)
src/lib/files/fileProcessing.test.ts (4)
src/lib/project-registry/types.ts (1)
ProjectSettings(15-37)src/lib/files/types.ts (1)
ProcessFileToAssetsOptions(7-22)src/lib/project-registry/path-resolution.ts (1)
getEffectiveAssetsDirectory(58-79)src/lib/files/fileProcessing.ts (1)
processFileToAssets(20-87)
src/lib/files/types.ts (1)
src/lib/project-registry/types.ts (1)
ProjectSettings(15-37)
src/lib/editor/dragdrop/fileProcessing.ts (2)
src/store/projectStore.ts (1)
useProjectStore(46-428)src/lib/editor/dragdrop/index.ts (1)
formatAsMarkdown(31-31)
🪛 LanguageTool
docs/tasks-todo/task-1-refactor-file-copying-logic.md
[uncategorized] ~924-~924: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...y - Preserved editor-specific concerns: markdown formatting, error fallback - All 28 tes...
(MARKDOWN_NNP)
🪛 markdownlint-cli2 (0.18.1)
docs/developer/architecture-guide.md
143-143: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (13)
docs/developer/architecture-guide.md (1)
139-197: Good addition; aligns with “shared utility” objective.Clear separation of strategies, public API, and test guidance look solid. Nice cross-references.
src/lib/editor/urls/detection.ts (1)
5-5: Switch to centralized constants looks correct.Importing IMAGE_EXTENSIONS_WITH_DOTS from the barrel and using a lowercase endsWith check is clean and robust.
Also applies to: 98-100
src/lib/files/constants.ts (1)
5-14: Extension parity is already maintained—no regression risk.The current list correctly preserves the previous state. Documentation confirms both prior implementations (
src/lib/editor/dragdrop/fileProcessing.tsandImageField.tsx) contained identical extensions: png, jpg, jpeg, gif, webp, svg, bmp, and ico. The consolidation maintains this exact set.The proposed additions (avif, tiff, tif, apng, heic, heif) were never part of the supported extensions in either previous location, so adding them would constitute a feature addition rather than parity preservation. There is no regression risk from the current consolidation—the behavior is unchanged.
If you wish to support additional formats, consider whether there are actual project requirements or user-facing needs that justify a feature enhancement; if so, propose them as such and provide justification separate from parity concerns.
Likely an incorrect or invalid review comment.
src/lib/files/fileProcessing.test.ts (1)
12-15: Mocks are correctly configured and match implementation imports—no changes needed.Verification confirms the implementation imports directly from
'../project-registry'and'../constants'(lines 2–3 of fileProcessing.ts), and the test file mocks at lines 11–13 and 16–19 use these exact import specifiers. No subpath imports or alternative sources detected.Likely an incorrect or invalid review comment.
src/lib/editor/dragdrop/fileProcessing.ts (3)
1-13: LGTM! Clean refactor to use shared constants.The imports and
isImageFilefunction have been correctly updated to use the sharedIMAGE_EXTENSIONS_WITH_DOTSconstant, eliminating the local duplication while preserving the original logic.
40-50: LGTM! Editor-specific helper correctly maintained.The
formatAsMarkdownfunction is appropriately kept in the editor module as it handles editor-specific markdown formatting concerns, aligning with the PR objective to preserve UI-specific responsibilities in their components.
59-104: Excellent refactor! Shared utility integration done right.The
processDroppedFilefunction correctly:
- Uses the shared
processFileToAssetswith the'always'copy strategy (appropriate for drag-and-drop)- Follows the Direct Store Pattern by accessing
useProjectStore.getState()directly- Preserves the existing error handling behavior (silent fallback to original path)
- Maintains separation of concerns by keeping markdown formatting in the editor layer
src/lib/files/fileProcessing.ts (1)
20-87: LGTM! Core shared utility is well-implemented.The
processFileToAssetsfunction correctly implements:
- Strategy pattern: Clear handling of both
'always'and'only-if-outside-project'copy strategies- Path resolution: Proper use of
getEffectiveAssetsDirectoryfor collection/project overrides- Tauri integration: Correct invocation of backend commands (
is_path_in_project,copy_file_to_assets,copy_file_to_assets_with_override,get_relative_path)- Path normalization: Ensures consistent leading slash in returned paths
- Error propagation: Throws errors for callers to handle (good separation of concerns)
This achieves the PR's goal of consolidating duplicated file processing logic into a single, well-tested utility.
src/components/frontmatter/fields/ImageField.tsx (3)
13-13: LGTM! Successfully using shared constants and utilities.The imports correctly bring in
processFileToAssetsandIMAGE_EXTENSIONSfrom the shared files module, eliminating the previous local duplication.
38-77: Excellent refactor! Frontmatter integration is clean and correct.The
handleFileSelectfunction properly:
- Uses the Direct Store Pattern (
useProjectStore.getState()) as per coding guidelines- Validates context before processing (prevents errors when project/collection unavailable)
- Applies the
'only-if-outside-project'strategy (correct for reusable frontmatter assets like cover images)- Maintains component-specific UI concerns (toast notifications, loading state)
- Uses the normalized
result.relativePathdirectly
185-185: Array spread is appropriate for TypeScript compatibility.The spread operator
[...IMAGE_EXTENSIONS]converts the readonly array to a mutable array, ensuring compatibility with theFileUploadButtoncomponent'sacceptprop type. This aligns with the AI summary noting TypeScript compatibility fixes.src/lib/editor/dragdrop/fileProcessing.test.ts (2)
25-37: LGTM! Proper mock setup with preserved actual exports.The mock configuration correctly:
- Uses
vi.importActualto preserve real exports likeIMAGE_EXTENSIONS_WITH_DOTS- Only mocks
processFileToAssets(the external dependency)- Obtains a properly typed mock reference for test assertions
This follows best practices by minimizing mocking scope and maintaining real constant values for testing.
181-396: Comprehensive test coverage for refactored functionality.The updated tests thoroughly verify:
- Integration with
processFileToAssetsusing the'always'copy strategy- Correct argument passing (sourcePath, projectPath, collection, projectSettings, copyStrategy)
- Success paths with proper markdown formatting using the returned
relativePath,wasCopied, andfilename- Error handling with fallback to original path (preserving existing behavior)
- Concurrent processing of multiple files
- Edge cases (empty arrays, Windows paths, complex filenames)
All 28 tests align with the new shared utility API while maintaining behavioral expectations.
1 participant
Closes #41
Summary by CodeRabbit
Bug Fixes
Documentation
Refactor