Fix JS tests, update files validation response error #1347
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to d413048 in 6 seconds
More details
- Looked at
17lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/r2r-js-sdk-integration-tests.yml:36
- Draft comment:
Ensure that the removed environment variables (POSTGRES_HOST, POSTGRES_USER, POSTGRES_PASSWORD, POSTGRES_PORT, POSTGRES_DBNAME, R2R_PROJECT_NAME) are not required for the R2R server to start and function correctly. If they are needed, consider alternative ways to provide these configurations. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_tcxr38fguOnWynd2
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4f19359 in 6 seconds
More details
- Looked at
34lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/r2r-js-sdk-integration-tests.yml:6
- Draft comment:
The workflow is now set to trigger on all branches. Ensure this is intended, as it may lead to excessive workflow runs. - Reason this comment was not posted:
Confidence changes required:50%
The change to trigger on all branches is significant and should be noted.
Workflow ID: wflow_Z0vyJfbYRFEiR3Lk
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on bbd0587 in 15 seconds
More details
- Looked at
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/r2r-js-sdk-integration-tests.yml:57
- Draft comment:
Usingdocker logs $(docker ps -q)will fetch logs for all running containers, which might not be intended if multiple containers are running. Consider specifying the container name or ID explicitly. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_M3Vb2M9gObrlZHly
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -58,11 +47,11 @@ jobs: | |||
|
|
|||
| - name: Display R2R server logs if server not responding | |||
| if: failure() | |||
| run: cat r2r_server.log | |||
| run: docker logs $(docker ps -q) | |||
There was a problem hiding this comment.
Using docker logs $(docker ps -q) will fetch logs for all running containers, which might not be intended if multiple containers are running. Consider specifying the container name or ID explicitly.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 6445b4a in 6 seconds
More details
- Looked at
46lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. py/shared/api/models/ingestion/responses.py:40
- Draft comment:
The change fromUUIDtoOptional[UUID]fortask_idis appropriate, as it aligns with the logic iningestion_router.pywheretask_idis set toNoneif not present. This ensures the model can handle cases wheretask_idis not provided. - Reason this comment was not posted:
Confidence changes required:0%
The change from UUID to Optional[UUID] in the UpdateResponse model is consistent with the code changes in ingestion_router.py where task_id is set to None if not present. This ensures that the model can handle cases where task_id is not provided.
Workflow ID: wflow_i9dzvPdZLwR8qSi2
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on aaa08fb in 12 seconds
More details
- Looked at
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/r2r-js-sdk-integration-tests.yml:50
- Draft comment:
The change todocker logs r2r-r2r-1assumes the container name is alwaysr2r-r2r-1. Consider dynamically determining the container ID or ensuring the container name is constant and documented. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment addresses a potential issue with hardcoding a container name, which could lead to problems if the container name changes or is not consistent. This is a valid concern as it relates to the robustness and maintainability of the code. The comment is actionable, suggesting a way to improve the code by dynamically determining the container ID.
The comment assumes that the container name might change, but if the container name is indeed constant and documented, the change might be acceptable. Without additional context, it's hard to determine if this is a real issue.
Even if the container name is constant, documenting this assumption or dynamically determining the container ID would improve code clarity and robustness.
The comment is valid as it highlights a potential issue with hardcoding the container name and suggests an improvement. It should be kept.
Workflow ID: wflow_w8OWVu2VNl2BNMW6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 07c90eb in 8 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. py/core/providers/orchestration/hatchet.py:36
- Draft comment:
Remove the unnecessary blank line for better readability. - Reason this comment was not posted:
Confidence changes required:10%
The code contains an unnecessary blank line that should be removed for better readability and consistency.
Workflow ID: wflow_Av7rG1nAzG6JHf99
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
NolanTrem
changed the title
Important
Remove unnecessary environment variables from GitHub Actions, handle missing task IDs in API responses, and fix documentation whitespace.
POSTGRES_HOST,POSTGRES_USER,POSTGRES_PASSWORD,POSTGRES_PORT,POSTGRES_DBNAME,R2R_PROJECT_NAMEfromr2r-js-sdk-integration-tests.yml.ingestion_router.py, added handling for missingtask_idinupdate_files_app()andingest_files_app().task_idinUpdateResponsetoOptional[UUID]inresponses.py.whats-new.mdx.This description was created by
for 07c90eb. It will automatically update as commits are pushed.