feat: Moving the finished note to something other than _posts #410
Conversation
… the finished note elsewhere "Moving the finished note to a location other than the default `_posts` folder allows for better organization and management of content, especially in static site generators like Jekyll. This helps separate draft, published, or archived notes from ongoing posts, improving workflow and structure." fix songkg7#363
WalkthroughThe pull request introduces modifications to the Jekyll conversion process, specifically in file handling logic. A new method Changes
Sequence DiagramsequenceDiagram
participant C as Chirpy Converter
participant S as Jekyll Settings
participant F as File System
C->>S: Request target sub-path
S-->>C: Return dynamic path
C->>F: Move files to calculated path
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/jekyll/settings/JekyllSettings.ts (1)
79-81: Add input validation and documentation for consistency.While the implementation is straightforward, consider these improvements:
- Add input validation to handle empty or invalid folder names
- Add JSDoc comments to match the documentation style of other methods
Consider updating the implementation like this:
+ /** + * Generates a target sub-path within the Jekyll directory + * @param folder - The folder name to append to the Jekyll path + * @returns The complete sub-path + * @throws {Error} If folder is empty or contains invalid characters + */ targetSubPath(folder: string): string { + if (!folder || folder.trim().length === 0) { + throw new Error('Folder name cannot be empty'); + } + if (folder.includes('..') || folder.includes('/')) { + throw new Error('Invalid folder name'); + } return `${this._jekyllPath}/${folder}`; }src/jekyll/chirpy.ts (1)
55-75: Consider adding unit tests for the new file movement logic.The PR objectives mention that no tests were added for this feature. Given the complexity of path handling and file operations, unit tests would be valuable.
Would you like me to help create unit tests for:
- Path extraction logic
- File movement with different folder scenarios
- Error handling cases
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/jekyll/chirpy.ts(1 hunks)src/jekyll/settings/JekyllSettings.ts(1 hunks)
🔇 Additional comments (1)
src/jekyll/chirpy.ts (1)
55-75: Verify similar file operations across the codebase.
Let's ensure consistent path handling across the codebase.
✅ Verification successful
Let me check the implementation of these file operations to ensure consistency.
Path handling is consistent across the codebase
The code review concern can be resolved as the investigation shows:
- The
moveFilesutility is consistently used across both Jekyll (Chirpy) and Docusaurus implementations - Path handling is properly abstracted in
utils.tsusing Node'spathmodule for most operations - The specific path substring operation in the reviewed code is unique to that location and serves a specific purpose of folder name extraction
The changes in the review are safe and maintain consistency with the rest of the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar file movement patterns
rg -l "moveFiles|vaultAbsolutePath" --type ts
# Look for potential path manipulation
ast-grep --pattern 'path.substring($$$)' --lang ts
Length of output: 253
Script:
#!/bin/bash
# Check implementation of moveFiles and vaultAbsolutePath
rg "moveFiles|vaultAbsolutePath" -A 5 --type ts
# Check path handling in other files
rg "path\." --type ts -A 2
Length of output: 4610
src/jekyll/chirpy.ts
Outdated
| // Where can I create this code block(create a function)? | ||
| const path:string = file.path; | ||
| const directory = path.substring(0, path.lastIndexOf("/")); | ||
| const folder = directory.substring(directory.lastIndexOf("/") + 1); | ||
| // end code block | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract path parsing logic into a dedicated function.
The comment indicates this should be a separate function. Let's help implement that.
Consider extracting this logic:
function extractFolderFromPath(path: string): string {
const directory = path.substring(0, path.lastIndexOf("/"));
return directory.substring(directory.lastIndexOf("/") + 1);
}
src/jekyll/chirpy.ts
Outdated
| if(folder !== plugin.obsidianPathSettings.readyFolder){ | ||
| await moveFiles( | ||
| `${vaultAbsolutePath(plugin)}/${plugin.obsidianPathSettings.readyFolder}/${folder}`, | ||
| settings.targetSubPath(folder), | ||
| settings.pathReplacer, | ||
| ).then(() => new Notice('Moved files to Chirpy successfully.', 5000)); | ||
|
|
||
| } else { | ||
| await moveFiles( | ||
| `${vaultAbsolutePath(plugin)}/${plugin.obsidianPathSettings.readyFolder}`, | ||
| settings.targetPath(), | ||
| settings.pathReplacer, | ||
| ).then(() => new Notice('Moved files to Chirpy successfully.', 5000)); | ||
| } |
There was a problem hiding this comment.
Refactor duplicate notification logic and improve error handling.
The code has several issues to address:
- Duplicate success notifications
- Potential path traversal vulnerability
- Non-specific error handling
Consider this refactoring:
- if(folder !== plugin.obsidianPathSettings.readyFolder){
- await moveFiles(
- `${vaultAbsolutePath(plugin)}/${plugin.obsidianPathSettings.readyFolder}/${folder}`,
- settings.targetSubPath(folder),
- settings.pathReplacer,
- ).then(() => new Notice('Moved files to Chirpy successfully.', 5000));
-
- } else {
- await moveFiles(
- `${vaultAbsolutePath(plugin)}/${plugin.obsidianPathSettings.readyFolder}`,
- settings.targetPath(),
- settings.pathReplacer,
- ).then(() => new Notice('Moved files to Chirpy successfully.', 5000));
- }
+ try {
+ const sourcePath = folder !== plugin.obsidianPathSettings.readyFolder
+ ? path.join(vaultAbsolutePath(plugin), plugin.obsidianPathSettings.readyFolder, folder)
+ : path.join(vaultAbsolutePath(plugin), plugin.obsidianPathSettings.readyFolder);
+
+ const targetPath = folder !== plugin.obsidianPathSettings.readyFolder
+ ? settings.targetSubPath(folder)
+ : settings.targetPath();
+
+ await moveFiles(sourcePath, targetPath, settings.pathReplacer);
+ new Notice('Moved files to Chirpy successfully.', 5000);
+ } catch (error) {
+ console.error('Failed to move files:', error);
+ new Notice(`Failed to move files: ${error.message}`, 5000);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(folder !== plugin.obsidianPathSettings.readyFolder){ | |
| await moveFiles( | |
| `${vaultAbsolutePath(plugin)}/${plugin.obsidianPathSettings.readyFolder}/${folder}`, | |
| settings.targetSubPath(folder), | |
| settings.pathReplacer, | |
| ).then(() => new Notice('Moved files to Chirpy successfully.', 5000)); | |
| } else { | |
| await moveFiles( | |
| `${vaultAbsolutePath(plugin)}/${plugin.obsidianPathSettings.readyFolder}`, | |
| settings.targetPath(), | |
| settings.pathReplacer, | |
| ).then(() => new Notice('Moved files to Chirpy successfully.', 5000)); | |
| } | |
| try { | |
| const sourcePath = folder !== plugin.obsidianPathSettings.readyFolder | |
| ? path.join(vaultAbsolutePath(plugin), plugin.obsidianPathSettings.readyFolder, folder) | |
| : path.join(vaultAbsolutePath(plugin), plugin.obsidianPathSettings.readyFolder); | |
| const targetPath = folder !== plugin.obsidianPathSettings.readyFolder | |
| ? settings.targetSubPath(folder) | |
| : settings.targetPath(); | |
| await moveFiles(sourcePath, targetPath, settings.pathReplacer); | |
| new Notice('Moved files to Chirpy successfully.', 5000); | |
| } catch (error) { | |
| console.error('Failed to move files:', error); | |
| new Notice(`Failed to move files: ${error.message}`, 5000); | |
| } |
|
Hello @songkg7, I implemented the new functionality from issue #363 , and when testing it locally, everything seems to be working fine. However, when running the tests in the pull request, I received the following errors related to ESLint and the JSON file format: I couldn't identify the exact cause of these errors, as the local tests don't point to any issues and everything seems to be in line with the configured rules. Also, I didn't edit any files related to ESLint. Could you help me understand what might be happening or if there is any additional configuration that should be applied to resolve these errors in the tests? Thank you in advance for your help. |
There was a recent eslint update, and it hasn't been working since. I don't think these settings are needed yet. I'll disable it and reactivate it later if I feel I need it. |
|
Thanks for your contribution! 👍 I'll review it soon. |
src/jekyll/chirpy.ts
Outdated
| // Where can I create this code block(create a function)? | ||
| const path: string = file.path; | ||
| const directory = path.substring(0, path.lastIndexOf('/')); | ||
| const folder = directory.substring(directory.lastIndexOf('/') + 1); | ||
| // end code block | ||
|
|
||
| if (folder !== plugin.obsidianPathSettings.readyFolder) { | ||
| await moveFiles( | ||
| `${vaultAbsolutePath(plugin)}/${plugin.obsidianPathSettings.readyFolder}/${folder}`, | ||
| settings.targetSubPath(folder), | ||
| settings.pathReplacer, | ||
| ).then(() => new Notice('Moved files to Chirpy successfully.', 5000)); | ||
| } else { | ||
| await moveFiles( | ||
| `${vaultAbsolutePath(plugin)}/${plugin.obsidianPathSettings.readyFolder}`, | ||
| settings.targetPath(), | ||
| settings.pathReplacer, | ||
| ).then(() => new Notice('Moved files to Chirpy successfully.', 5000)); | ||
| } |
There was a problem hiding this comment.
It seems that chirpy.ts hasn't yet adopted functional programming, so there doesn't appear to be a suitable place for functions to fit naturally. Create a new class, such as a PathDecider class, which determines where to move files based on certain options, might be a better solution.
as follow:
const targetPath = new PathDecicder().decide(option)However, one of my goals is to refactor this into a function-base structure, similar to docusaurus.ts. So instead of introducing more classes, I think it's fine to apply the current approach for now and refactor it into a functional style later.
That said, the decision is up to you! :)
If it's okay to wrap up the work at this point, I'll proceed with merging and closing the PR.
There was a problem hiding this comment.
Since you'll be doing a refactoring later, I believe it's not necessary for now, and I think this proposed solution is excellent to avoid rework in the future. Congratulations on the code! :)
I'll remove the comments in the next commit.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #410 +/- ##
=======================================
Coverage 94.23% 94.23%
=======================================
Files 12 12
Lines 243 243
Branches 33 33
=======================================
Hits 229 229
Misses 13 13
Partials 1 1 ☔ View full report in Codecov by Sentry. |
|
you can ignore codecov :) |
Pull Request
Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently, all finished notes are moved to the default
_postsfolder, which may cause disorganization and difficulty managing content, especially for users of static site generators like Jekyll.Issue Number: #363
What is the new behavior?
This feature allows finished notes to be moved to a location other than the default
_postsfolder, improving organization and content management. It helps separate draft, published, or archived notes from ongoing posts, enhancing the workflow and structure of the content.Does this PR introduce a breaking change?
Other information
This change provides better content organization and is fully backward-compatible, as it adds flexibility in where finished notes can be stored without affecting existing workflows. I ran the existing tests, and they passed successfully; however, I did not create new tests specifically for this feature.
Summary by CodeRabbit
New Features
Documentation