-
Notifications
You must be signed in to change notification settings - Fork 95
Folder posts #1027
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Folder posts #1027
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (stat && stat.isDirectory()) { | ||
| maybeEnableInjectTitle(blog, path, function () { | ||
| // there is nothing else to do for directories | ||
| done(); | ||
| var multiInfo = build.findMultiFolder(path); | ||
|
|
||
| if (multiInfo) { | ||
| var targetPath = multiInfo.folderPath; | ||
|
|
||
| log(path, "Saving multi-folder in database"); | ||
|
|
||
| set(blog, targetPath, function (err) { | ||
| if (err) { | ||
| log(targetPath, "Error saving multi-folder in database", err); | ||
| } else { | ||
| log(targetPath, "Saving multi-folder in database succeeded"); | ||
| } | ||
| done(err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop multi-folder entry when directory is removed
This branch builds aggregated content when a + directory exists, but the ENOENT branch above still drops only the directory path (/album+). The published entry for that folder lives at the plus-stripped entryPath, so deleting the folder leaves /album and its preview in the database because nothing ever removes it or rebuilds dependents. Consider also dropping multiInfo.entryPath when the folder disappears so that multi-folder deletions clean up their synthesized entry.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fs.stat(localPath(blog.id, path), function (err, stat) { | ||
| if (err && err.code === "ENOENT") { | ||
| log(path, "Dropping from database"); | ||
| drop(blog.id, path, function (err) { | ||
| if (err) { | ||
| log(path, "Error dropping from database", err); | ||
| } else { | ||
| log(path, "Dropping from database succeeded"); | ||
| } | ||
| done(err); | ||
| }); | ||
| var dropTargets = [path]; | ||
| var multiInfo = build.findMultiFolder(path); | ||
|
|
||
| if ( | ||
| multiInfo && | ||
| multiInfo.folderPath === path && | ||
| multiInfo.entryPath && | ||
| dropTargets.indexOf(multiInfo.entryPath) === -1 | ||
| ) { | ||
| dropTargets.push(multiInfo.entryPath); | ||
| } | ||
|
|
||
| var dropError = null; | ||
|
|
||
| (function nextDrop(index) { | ||
| if (index >= dropTargets.length) return done(dropError); | ||
|
|
||
| var target = dropTargets[index]; | ||
| log(target, "Dropping from database"); | ||
| drop(blog.id, target, function (err) { | ||
| if (err) { | ||
| log(target, "Error dropping from database", err); | ||
| if (!dropError) dropError = err; | ||
| } else { | ||
| log(target, "Dropping from database succeeded"); | ||
| } | ||
|
|
||
| nextDrop(index + 1); | ||
| }); | ||
| })(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebuild multi-folder entry when a child file is removed
The delete branch only drops path and, if the removed path is the +/ folder, its aggregate. When a single file inside a multi-folder is deleted (e.g. /album+/two.md), this branch does not rebuild or drop /album, so the published entry continues to contain content from the deleted file until some other change touches the folder. Nothing elsewhere schedules a rebuild for /album on child deletion, so multi-folder posts can display stale content after a source file is removed. Consider triggering a rebuild of the aggregated entry whenever any path under the multi-folder disappears.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex files which should be ignored (e.g. _1.jpg or _images/1.jpg) are still added to multi file posts. Please revise to prevent these files from inclusion in these posts |
|
Summary
Testing
|
|
@codex This should work recursively for subfolders? up to a limit of say 50 files? if there are too many files, don't build the post at all. sort all the paths alphabetically before concatenating, e.g. 1.jpg, 2/a.jpg, 2/b.jpg, 3.jpg etc. implement please |
|
Summary
Testing
|
|
@codex Can we inject a title based on the folder name into the post HTML when a title tag doesn't exist in the post html? Add a classname, e.g. multi-file-title to the h1 so it can easily be hidden if so desired |
|
Summary
Testing
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
<section class="multi-file-post"><section data-source="{{path}}">...https://chatgpt.com/codex/tasks/task_e_68f618afe9188329b32153d161c70a5b