Conversation
| * @param path - Optional path to a subdirectory (defaults to root) | ||
| * @returns Array of tree entries with name, type, oid, and mode | ||
| */ | ||
| async getTree( |
There was a problem hiding this comment.
This code is duplicated, can we reuse the git implementation from here?
| * @param path - Path to the file | ||
| * @returns File content (utf-8 or base64 encoded), encoding type, size, and blob SHA | ||
| */ | ||
| async getBlob( |
There was a problem hiding this comment.
Also duplicated and the return types are mismatching?
| message: error instanceof Error ? error.message : 'Path not found', | ||
| }), | ||
| { | ||
| status: 404, |
There was a problem hiding this comment.
This will return 404 also for 'Error(Path not found: ${normalizedPath})' and 'Error(Path is not a directory: ${normalizedPath})'. It depends on the quality we want for this API, but an optional improvement would be to handle not found cases explicitly by cascading the error from getTree
| message: error instanceof Error ? error.message : 'File not found', | ||
| }), | ||
| { | ||
| status: 404, |
There was a problem hiding this comment.
See comment above. Same thing here.
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // Handle tree requests (GET /apps/{app_id}/tree/{ref}) | ||
| const treeMatch = pathname.match(TREE_PATTERN); | ||
| if (treeMatch && request.method === 'GET') { | ||
| const decodedRef = decodeURIComponent(treeMatch[2]); |
There was a problem hiding this comment.
WARNING: decodeURIComponent() can throw on malformed percent-encoding
If a client sends an invalid URL escape sequence in the {ref} segment (e.g. %E0), decodeURIComponent() throws URIError: URI malformed, which will bubble up as an unhandled exception (500). Consider guarding decoding and returning a 400 for invalid encoding.
Code Review SummaryStatus: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (6 files)
|
|
thank you @marius-kilocode, PR is ready for another pass |
| "@types/better-sqlite3": "^7.6.13", | ||
| "@types/jsonwebtoken": "^9.0.9", | ||
| "@types/node": "^22.10.1", | ||
| "better-sqlite3": "^12.6.0", |
There was a problem hiding this comment.
We can already use: 12.6.2, same applies for the other new deps and their patch releases.
There was a problem hiding this comment.
Still the old version?
| try { | ||
| db.close(); | ||
| } catch { | ||
| // Ignore - might already be closed |
There was a problem hiding this comment.
Should we not log this?
| @@ -0,0 +1,72 @@ | |||
| /** | |||
| * Test utilities for GitVersionControl unit tests | |||
There was a problem hiding this comment.
The code does't appears really domain specific to git. It rather looks like common code that should be sharable and reusable?
| run: pnpm --filter app-builder typecheck | ||
|
|
||
| - name: Run app-builder tests | ||
| run: pnpm --filter app-builder test |
There was a problem hiding this comment.
I am a bit confused here should we not run the integration tests?
| echo "" | ||
|
|
||
| if pnpm exec tsx "$test_file"; then | ||
| TOTAL_PASSED=$((TOTAL_PASSED + 1)) |
There was a problem hiding this comment.
Is there a reason why we do all of this manually? Should we not use a proper test runner instead?
|
|
||
| if (result.status === 201 && result.body.success === true) { | ||
| logSuccess('Init with valid special chars (underscore, hyphen) succeeds'); | ||
| return true; |
There was a problem hiding this comment.
I don't get why we are returning true or false here? We need assertions here or not and a proper test framework to detect failures.
| import { logger } from '../utils/logger'; | ||
| import type { Env } from '../types'; | ||
|
|
||
| const notFoundPatterns = [ |
There was a problem hiding this comment.
This is a really hacky model reward hack, once any error string changes the logic changes. We should introduce either proper error codes or error implementations.
2 participants
This pull request introduces improvements to the Cloudflare App Builder project focusing on enhanced test coverage.
Testing & CI Improvements
API Schema Additions
/treeand/blobendpoints, defining types and validation for tree and blob responses.