Add artifact folders and web artifact sorting#40
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds folder-based organization for session artifacts: manifest and validation changes, Add/Filter support for a Folder field, CLI flags and tree output, web API upload/list folder passthrough, UI grouping/sorting/search updates, tests, and documentation. ChangesArtifact Folder Organization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
artifact/manifest_test.go (1)
62-72:⚠️ Potential issue | 🟠 MajorPre-existing Windows test failure in path validation (not introduced by this PR).
The test
TestValidateRelativePathRejectsTraversalhas a platform-specific issue at line 66. It expects/tmp/fileto be rejected as an absolute path, but on Windows this assertion fails. The root cause is thatfilepath.IsAbs()returnsfalsefor Unix-style paths like/tmp/fileon Windows, since Windows only recognizes absolute paths with drive letters (e.g.,C:\). This test remains unchanged in the current PR and is a pre-existing issue in the base code that should be addressed separately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifact/manifest_test.go` around lines 62 - 72, Test TestValidateRelativePathRejectsTraversal fails on Windows because the hardcoded Unix absolute path "/tmp/file" isn't considered absolute there; update the test to use an OS-appropriate absolute path before calling ValidateRelativePath (e.g., detect runtime.GOOS == "windows" and use a Windows absolute like "C:\\tmp\\file" or construct with filepath.Join/FromSlash accordingly, else keep "/tmp/file"), or skip that specific subcase on Windows; update only TestValidateRelativePathRejectsTraversal to select the platform-correct absolute path so the assertion that absolute paths are rejected holds across OSes.
🧹 Nitpick comments (1)
cmd/artifact_add.go (1)
29-49: ⚡ Quick winAdd a Cobra
Examplefor the new--folderflow.The flag help was updated, but
devx artifact add --helpstill doesn't show a foldered invocation. Adding one flat example and one--folderexample would make the new behavior discoverable from the CLI itself.As per coding guidelines, "When modifying CLI commands, update help text and examples".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/artifact_add.go` around lines 29 - 49, The cobra command artifactAddCmd lacks Example text for the new --folder behavior; update the artifactAddCmd definition (or set artifactAddCmd.Example in init) to include two concise usage examples: one flat invocation (e.g., devx artifact add --title "..." path/to/file) and one showing the new folder flow (e.g., devx artifact add --folder my/group --file sub/path --title "..." <file|-> or piping from stdin with --file relative to --folder), ensuring the examples mention the --folder flag and relative file semantics and reference the same flags used in init (artifactAddFlags.folder, .file, .title) so users see the new behavior in devx artifact add --help.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@artifact/manifest.go`:
- Around line 184-193: The folder validation must also canonicalize and store
the normalized value so downstream comparisons use a single canonical
representation: in the block that calls NormalizeFolderPath(a.Folder) (the code
around the file/folder validation in artifact/manifest.go where
NormalizeFolderPath is used), replace the current pattern of only checking the
returned folder with assigning the normalized folder back to the artifact (e.g.,
set a.Folder = folder) after successful normalization, and keep the subsequent
file membership checks using that normalized value; ensure this normalization
occurs whenever manifests are loaded/saved so Folder is always canonicalized.
---
Outside diff comments:
In `@artifact/manifest_test.go`:
- Around line 62-72: Test TestValidateRelativePathRejectsTraversal fails on
Windows because the hardcoded Unix absolute path "/tmp/file" isn't considered
absolute there; update the test to use an OS-appropriate absolute path before
calling ValidateRelativePath (e.g., detect runtime.GOOS == "windows" and use a
Windows absolute like "C:\\tmp\\file" or construct with filepath.Join/FromSlash
accordingly, else keep "/tmp/file"), or skip that specific subcase on Windows;
update only TestValidateRelativePathRejectsTraversal to select the
platform-correct absolute path so the assertion that absolute paths are rejected
holds across OSes.
---
Nitpick comments:
In `@cmd/artifact_add.go`:
- Around line 29-49: The cobra command artifactAddCmd lacks Example text for the
new --folder behavior; update the artifactAddCmd definition (or set
artifactAddCmd.Example in init) to include two concise usage examples: one flat
invocation (e.g., devx artifact add --title "..." path/to/file) and one showing
the new folder flow (e.g., devx artifact add --folder my/group --file sub/path
--title "..." <file|-> or piping from stdin with --file relative to --folder),
ensuring the examples mention the --folder flag and relative file semantics and
reference the same flags used in init (artifactAddFlags.folder, .file, .title)
so users see the new behavior in devx artifact add --help.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fcd61add-f9cc-46aa-9cf0-d49666b80849
⛔ Files ignored due to path filters (5)
web/dist/assets/index-C-eGZceh.jsis excluded by!**/dist/**web/dist/assets/index-Cb9dmfi2.cssis excluded by!**/dist/**web/dist/assets/index-CkFD9JOf.jsis excluded by!**/dist/**web/dist/assets/index-D3TGCEBb.cssis excluded by!**/dist/**web/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (13)
README.mdartifact/add.goartifact/filter.goartifact/manifest.goartifact/manifest_test.gocmd/artifact_add.gocmd/artifact_list.gocmd/artifact_test.godocs/artifacts.mdweb/app/src/lib/Terminal.svelteweb/app/src/lib/artifacts/ArtifactPane.svelteweb/artifacts_api.goweb/artifacts_api_test.go
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.
Summary
.artifacts/<folder>/...artifact addandartifact list, including grouped tree outputTesting
Summary by CodeRabbit