Open
Conversation
Contributor
📝 WalkthroughWalkthroughA Changes
Sequence DiagramsequenceDiagram
participant Client
participant ServerCtrl as Server<br/>Controller
participant CoreStage as Core<br/>Staging
participant Storage
Client->>ServerCtrl: POST /import or upload<br/>(with force_update flag)
ServerCtrl->>ServerCtrl: Parse force_update<br/>from request
ServerCtrl->>CoreStage: add_version_file(...,<br/>force_update)
alt force_update = true
CoreStage->>CoreStage: Override Unmodified→Modified<br/>Use FileTime::now()
CoreStage->>Storage: Store file with new mtime
else force_update = false
CoreStage->>CoreStage: Skip if unmodified
CoreStage->>Storage: Only stage if modified
end
CoreStage->>ServerCtrl: Return result
ServerCtrl->>Client: Commit response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/lib/src/api/client/workspaces/files.rs (1)
207-225: Consider exposingforce_updateinadd_filesfor consistency.The
add_filesfunction hardcodesforce_update: false. While this may be intentional for the current use case (bulk importing existing file structures), consider whether this function should also support theforce_updateparameter for API consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/lib/src/api/client/workspaces/files.rs` around lines 207 - 225, The add_files function currently calls upload_multiple_files with force_update hardcoded to false; add a force_update parameter to add_files' signature (e.g., force_update: bool) and pass it through to upload_multiple_files instead of the literal false, updating call sites of add_files to supply the appropriate value (or default to false where callers aren’t changed) and updating any docs/tests accordingly so API behavior is consistent with other file operations that accept force_update.crates/lib/src/api/client/import.rs (1)
233-234: Consider test stability with external URL dependency.The test relies on an external URL (
https://hub.oxen.ai/...). While this is likely a stable Oxen-hosted resource, external dependencies can cause test flakiness. Consider whether this test should be marked as an integration test or if there's a way to use a mock/local resource.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/lib/src/api/client/import.rs` around lines 233 - 234, The test hard-codes an external URL into the download_url variable which can make the test flaky; change the test in crates::lib::src::api::client::import.rs to avoid the external dependency by either (a) using a local test fixture/bundled asset path instead of "download_url", (b) mocking HTTP responses with a test HTTP server (e.g., httptest/wiremock) and point download_url at the mock server, or (c) mark the test as an integration/ignored test (#[ignore] or a dedicated integration test cfg) if it must hit the real Oxen endpoint; update the test setup and assertions accordingly so they reference the download_url symbol you modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/server/src/controllers/import.rs`:
- Around line 183-186: The current extraction of force_update silently defaults
to false for non-boolean values; update the import handler where force_update is
computed (the let force_update = body.get("force_update")... expression) to
validate types: if body contains the "force_update" key but
body.get("force_update").as_bool() is None, return a 400 Bad Request response
indicating an invalid type; otherwise set force_update to the boolean value or
false when the key is absent. Ensure the new branch returns the appropriate
error early from the same function handling the request.
---
Nitpick comments:
In `@crates/lib/src/api/client/import.rs`:
- Around line 233-234: The test hard-codes an external URL into the download_url
variable which can make the test flaky; change the test in
crates::lib::src::api::client::import.rs to avoid the external dependency by
either (a) using a local test fixture/bundled asset path instead of
"download_url", (b) mocking HTTP responses with a test HTTP server (e.g.,
httptest/wiremock) and point download_url at the mock server, or (c) mark the
test as an integration/ignored test (#[ignore] or a dedicated integration test
cfg) if it must hit the real Oxen endpoint; update the test setup and assertions
accordingly so they reference the download_url symbol you modify.
In `@crates/lib/src/api/client/workspaces/files.rs`:
- Around line 207-225: The add_files function currently calls
upload_multiple_files with force_update hardcoded to false; add a force_update
parameter to add_files' signature (e.g., force_update: bool) and pass it through
to upload_multiple_files instead of the literal false, updating call sites of
add_files to supply the appropriate value (or default to false where callers
aren’t changed) and updating any docs/tests accordingly so API behavior is
consistent with other file operations that accept force_update.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2a0eb18-da7a-4c09-bed9-65f21eff1ff3
📒 Files selected for processing (13)
crates/lib/src/api/client/import.rscrates/lib/src/api/client/versions.rscrates/lib/src/api/client/workspaces/commits.rscrates/lib/src/api/client/workspaces/files.rscrates/lib/src/core/v_latest/add.rscrates/lib/src/core/v_latest/push.rscrates/lib/src/core/v_latest/workspaces/files.rscrates/lib/src/repositories/commits/commit_writer.rscrates/lib/src/repositories/workspaces/files.rscrates/lib/src/view/versions.rscrates/server/src/controllers/import.rscrates/server/src/controllers/versions/chunks.rscrates/server/src/controllers/workspaces/files.rs
Comment on lines
+183
to
+186
| let force_update = body | ||
| .get("force_update") | ||
| .and_then(|v| v.as_bool()) | ||
| .unwrap_or(false); |
Contributor
There was a problem hiding this comment.
Reject invalid force_update types instead of silently disabling the feature.
Line 183 currently falls back to false for non-boolean values, so malformed payloads can silently ignore a requested force update. Return 400 when the key is present but not boolean.
💡 Suggested fix
- let force_update = body
- .get("force_update")
- .and_then(|v| v.as_bool())
- .unwrap_or(false);
+ let force_update = match body.get("force_update") {
+ None => false,
+ Some(v) => v.as_bool().ok_or_else(|| {
+ OxenHttpError::BadRequest("`force_update` must be a boolean".into())
+ })?,
+ };📝 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.
Suggested change
| let force_update = body | |
| .get("force_update") | |
| .and_then(|v| v.as_bool()) | |
| .unwrap_or(false); | |
| let force_update = match body.get("force_update") { | |
| None => false, | |
| Some(v) => v.as_bool().ok_or_else(|| { | |
| OxenHttpError::BadRequest("`force_update` must be a boolean".into()) | |
| })?, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/server/src/controllers/import.rs` around lines 183 - 186, The current
extraction of force_update silently defaults to false for non-boolean values;
update the import handler where force_update is computed (the let force_update =
body.get("force_update")... expression) to validate types: if body contains the
"force_update" key but body.get("force_update").as_bool() is None, return a 400
Bad Request response indicating an invalid type; otherwise set force_update to
the boolean value or false when the key is absent. Ensure the new branch returns
the appropriate error early from the same function handling the request.
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.
We needed a way to bump a file's latest commit / timestamp, without bumping the files content. So I introduced this force_update parameter. Open to ideas on parameter naming...but this is the usage with our import and upload APIs:
Import from multi-part:
curl -v -X POST "http://localhost:3001/api/repos/Subaru/playground/workspaces/upload/main/uploads"
-H "Authorization: Bearer $API_KEY"
-F "commit_message=testing from cli"
-F "force_update=true"
-F "file[]=@/path/to/file.png"
and import from url:
curl -X POST \
"${BASE_API_URL}/repos/${NAMESPACE}/playground/import/main/uploads" \
-H "Authorization: Bearer ${TOKEN}"
-H "Content-Type: application/json" \
-d '{
"download_url": "https://example.com/image.jpg",
"name": "Greg",
"email": "greg@example.com",
"force_update": true
}'