Added file_upload and create_comment functions #12
Conversation
Added functionality to upload files (returning the asset url) and creating comments (returning the comment object)
📝 WalkthroughWalkthroughIssueManager received import reorganization, stronger GraphQL response/input validation, defensive guards and cache invalidation tweaks, standardized URL handling, and two new public methods: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client code
participant IM as IssueManager
participant API as GraphQL API
participant Storage as Object Storage
Note over IM,API: file_upload flow (new async method)
Client->>IM: file_upload(file_data, content_type, filename)
IM->>API: GraphQL mutation -> request upload info (filename, content_type)
API-->>IM: { upload_url, asset_url, headers? }
IM->>Storage: HTTP PUT to upload_url (with headers, file_data)
alt upload success
Storage-->>IM: 200/201
IM-->>Client: return asset_url
else upload failure
Storage-->>IM: error/other status
IM-->>Client: raise/return upload error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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: 2
Fix all issues with AI Agents 🤖
In @linear_api/managers/issue_manager.py:
- Around line 853-861: The inner aiohttp response variable shadows the outer
GraphQL `response` and lacks a timeout and specific error type; rename the inner
variable (e.g., upload_resp) to avoid shadowing, create a ClientTimeout (or pass
timeout=ClientTimeout(total=30)) to the ClientSession or session.put call, and
replace the generic Exception with a more specific aiohttp.ClientResponseError
(or RuntimeError if you prefer) including status and URL; update the block
around upload_info['uploadUrl'], headers, and data=file_data accordingly so the
new variable and timeout are used and the raised error contains the response
status and request URL for better diagnostics.
- Line 8: The project imports aiohttp in linear_api/managers/issue_manager.py
(used by the file_upload method) but aiohttp is not declared as a dependency;
add "aiohttp>=3.0.0" to the project's install_requires (e.g., in setup.py's
install_requires list) so the package is installed and ImportError is avoided;
ensure the version constraint matches the project's compatibility and update any
requirements.txt or pyproject.toml similarly if those are used for dependency
management.
🧹 Nitpick comments (1)
linear_api/managers/issue_manager.py (1)
753-755: Use explicitOptionaltype annotation.Per PEP 484, use explicit union type for optional parameters instead of implicit
Nonedefault.🔎 Proposed fix
+from typing import Any, Dict, List, Optional + def create_comment( - self, issue_id: str, body: str, parent_id: str = None + self, issue_id: str, body: str, parent_id: Optional[str] = None ) -> Comment:Or using modern Python 3.10+ syntax:
def create_comment( - self, issue_id: str, body: str, parent_id: str = None + self, issue_id: str, body: str, parent_id: str | None = None ) -> Comment:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
linear_api/managers/issue_manager.py
🧰 Additional context used
🪛 Ruff (0.14.10)
linear_api/managers/issue_manager.py
754-754: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
792-792: Avoid specifying long messages outside the exception class
(TRY003)
842-842: Avoid specifying long messages outside the exception class
(TRY003)
860-860: Create your own exception
(TRY002)
860-860: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
linear_api/managers/issue_manager.py (6)
767-799: LGTM!The comment creation logic is well-structured: proper GraphQL mutation, optional parent comment support for replies, appropriate response validation, and cache invalidation.
846-851: LGTM!Header construction correctly combines the required headers with those provided by the Linear API's upload response.
187-191: LGTM!Enhanced validation properly checks the response structure before accessing nested keys.
265-269: LGTM!Consistent validation pattern applied to the update mutation response.
251-253: LGTM!Defensive check prevents AttributeError when
parentIdis not present on the issue object.
946-951: LGTM!Comprehensive validation guards against missing response keys.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @linear_api/managers/issue_manager.py:
- Around line 753-755: Update the create_comment signature to use Optional[str]
for parent_id (e.g., parent_id: Optional[str] = None) to satisfy PEP 484, and
decide whether username is required or optional; if optional, change its type to
Optional[str] and default to None in the signature (and adjust any callers as
needed), and update the create_comment docstring to state whether username may
be omitted and what an empty or None username will be treated as; ensure the
body still uses username if not None else "" where appropriate to preserve
current behavior if you choose optional.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
linear_api/managers/issue_manager.py
🧰 Additional context used
🧬 Code graph analysis (1)
linear_api/managers/issue_manager.py (6)
linear_api/domain/common_models.py (1)
Comment(23-30)linear_api/domain/issue_models.py (4)
CustomerNeedResponse(406-418)IssueRelation(396-403)LinearIssue(86-310)comments(189-201)linear_api/domain/enums.py (1)
LinearPriority(10-16)linear_api/domain/user_models.py (2)
LinearUser(31-162)Reaction(173-180)linear_api/managers/base_manager.py (3)
_handle_pagination(74-194)_cache_set(318-330)_execute_query(48-64)linear_api/domain/base_domain.py (1)
model_dump(50-72)
🪛 Ruff (0.14.10)
linear_api/managers/issue_manager.py
754-754: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
792-792: Avoid specifying long messages outside the exception class
(TRY003)
842-842: Avoid specifying long messages outside the exception class
(TRY003)
861-861: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
linear_api/managers/issue_manager.py (2)
187-191: LGTM: Improved readability of response validation.The multi-line conditionals for GraphQL response validation improve readability and make the validation logic clearer.
Also applies to: 265-269, 303-307, 946-952, 1059-1066, 1120-1127, 1182-1189
251-253: Good defensive guard.Using
hasattrto check for theparentIdattribute prevents potentialAttributeErrorexceptions.
| def create_comment( | ||
| self, issue_id: str, body: str, username: str, parent_id: str = None | ||
| ) -> Comment: |
There was a problem hiding this comment.
Fix type hint and clarify username parameter.
The parent_id parameter should use Optional[str] = None instead of str = None to comply with PEP 484. Additionally, the username parameter appears required in the signature but is treated as optional in line 781 (username if username is not None else ""), creating inconsistency.
Clarify whether username is required or optional, and document it in the docstring.
🔎 Proposed fix
def create_comment(
- self, issue_id: str, body: str, username: str, parent_id: str = None
+ self, issue_id: str, body: str, username: str, parent_id: Optional[str] = None
) -> Comment:
"""
Create a comment for an issue (or reply to an existing comment).
Args:
issue_id: The ID of the issue to comment on
body: The text body of the comment
+ username: The username to create the comment as
parent_id: Optional ID of the parent comment to reply to
Returns:
The created Comment object
"""If username should be optional, update the signature and docstring accordingly:
def create_comment(
- self, issue_id: str, body: str, username: str, parent_id: str = None
+ self, issue_id: str, body: str, username: Optional[str] = None, parent_id: Optional[str] = None
) -> Comment:
"""
Create a comment for an issue (or reply to an existing comment).
Args:
issue_id: The ID of the issue to comment on
body: The text body of the comment
+ username: Optional username to create the comment as
parent_id: Optional ID of the parent comment to reply to
Returns:
The created Comment object
"""📝 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.
| def create_comment( | |
| self, issue_id: str, body: str, username: str, parent_id: str = None | |
| ) -> Comment: | |
| def create_comment( | |
| self, issue_id: str, body: str, username: str, parent_id: Optional[str] = None | |
| ) -> Comment: | |
| """ | |
| Create a comment for an issue (or reply to an existing comment). | |
| Args: | |
| issue_id: The ID of the issue to comment on | |
| body: The text body of the comment | |
| username: The username to create the comment as | |
| parent_id: Optional ID of the parent comment to reply to | |
| Returns: | |
| The created Comment object | |
| """ |
🧰 Tools
🪛 Ruff (0.14.10)
754-754: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
In @linear_api/managers/issue_manager.py around lines 753-755, Update the
create_comment signature to use Optional[str] for parent_id (e.g., parent_id:
Optional[str] = None) to satisfy PEP 484, and decide whether username is
required or optional; if optional, change its type to Optional[str] and default
to None in the signature (and adjust any callers as needed), and update the
create_comment docstring to state whether username may be omitted and what an
empty or None username will be treated as; ensure the body still uses username
if not None else "" where appropriate to preserve current behavior if you choose
optional.
| async def file_upload(self, file_data: bytes, content_type: str, filename: str) -> str: | ||
| """ | ||
| Upload a file to Linear. | ||
| Args: | ||
| file_data: The binary content of the file | ||
| content_type: The MIME type of the file (e.g., 'image/jpeg') | ||
| filename: The name of the file | ||
| Returns: | ||
| str: The final asset URL for the uploaded file | ||
| """ | ||
| mutation = """ | ||
| mutation fileUpload($contentType: String!, $filename: String!, $size: Int!) { | ||
| fileUpload(contentType: $contentType, filename: $filename, size: $size) { | ||
| success | ||
| uploadFile { | ||
| uploadUrl | ||
| assetUrl | ||
| headers { | ||
| key | ||
| value | ||
| } | ||
| } | ||
| } | ||
| } | ||
| """ | ||
|
|
||
| variables = { | ||
| "contentType": content_type, | ||
| "filename": filename, | ||
| "size": len(file_data) | ||
| } | ||
|
|
||
| response = self._execute_query(mutation, variables) | ||
|
|
||
| if ( | ||
| not response | ||
| or "fileUpload" not in response | ||
| or not response["fileUpload"].get("uploadFile") | ||
| ): | ||
| raise ValueError("Failed to get file upload URL") | ||
|
|
||
| upload_info = response["fileUpload"]["uploadFile"] | ||
|
|
||
| headers = { | ||
| 'Content-Type': content_type, | ||
| 'Cache-Control': 'public, max-age=31536000' | ||
| } | ||
| for header in upload_info['headers']: | ||
| headers[header['key']] = header['value'] | ||
|
|
||
| async with aiohttp.ClientSession() as session: | ||
| async with session.put( | ||
| upload_info['uploadUrl'], | ||
| headers=headers, | ||
| data=file_data, | ||
| timeout=aiohttp.ClientTimeout(total=300) | ||
| ) as upload_response: | ||
| if upload_response.status not in (200, 201): | ||
| raise ValueError(f"Upload failed with status {upload_response.status}") | ||
| return upload_info['assetUrl'] |
There was a problem hiding this comment.
Async method in synchronous class creates API inconsistency.
The file_upload method is async, while all other methods in IssueManager are synchronous. This creates an inconsistent API where users must mix async/await with regular synchronous calls, requiring an async context (event loop) for just this one operation.
This forces users into awkward patterns:
# Synchronous operations
issue = manager.get(issue_id)
manager.create(issue_data)
# Suddenly need async context
import asyncio
url = asyncio.run(manager.file_upload(file_data, content_type, filename))Consider one of these approaches:
- Make the method synchronous using
requestslibrary for consistency with the rest of the codebase - Use threading to wrap the async operation in a synchronous interface
- Document prominently that this method requires async context and provide usage examples
🔎 Option 1: Synchronous implementation using requests
- async def file_upload(self, file_data: bytes, content_type: str, filename: str) -> str:
+ def file_upload(self, file_data: bytes, content_type: str, filename: str) -> str:
"""
Upload a file to Linear.
Args:
file_data: The binary content of the file
content_type: The MIME type of the file (e.g., 'image/jpeg')
filename: The name of the file
Returns:
str: The final asset URL for the uploaded file
"""
mutation = """
mutation fileUpload($contentType: String!, $filename: String!, $size: Int!) {
fileUpload(contentType: $contentType, filename: $filename, size: $size) {
success
uploadFile {
uploadUrl
assetUrl
headers {
key
value
}
}
}
}
"""
variables = {
"contentType": content_type,
"filename": filename,
"size": len(file_data)
}
response = self._execute_query(mutation, variables)
if (
not response
or "fileUpload" not in response
or not response["fileUpload"].get("uploadFile")
):
raise ValueError("Failed to get file upload URL")
upload_info = response["fileUpload"]["uploadFile"]
headers = {
'Content-Type': content_type,
'Cache-Control': 'public, max-age=31536000'
}
for header in upload_info['headers']:
headers[header['key']] = header['value']
- async with aiohttp.ClientSession() as session:
- async with session.put(
- upload_info['uploadUrl'],
- headers=headers,
- data=file_data,
- timeout=aiohttp.ClientTimeout(total=300)
- ) as upload_response:
- if upload_response.status not in (200, 201):
- raise ValueError(f"Upload failed with status {upload_response.status}")
+ import requests
+ upload_response = requests.put(
+ upload_info['uploadUrl'],
+ headers=headers,
+ data=file_data,
+ timeout=300
+ )
+ if upload_response.status_code not in (200, 201):
+ raise ValueError(f"Upload failed with status {upload_response.status_code}")
+
return upload_info['assetUrl']Note: requests library is already a dependency (see setup.py), so no additional dependencies needed.
🔎 Option 2: Wrap async in synchronous using asyncio
- async def file_upload(self, file_data: bytes, content_type: str, filename: str) -> str:
+ def file_upload(self, file_data: bytes, content_type: str, filename: str) -> str:
"""
Upload a file to Linear.
Args:
file_data: The binary content of the file
content_type: The MIME type of the file (e.g., 'image/jpeg')
filename: The name of the file
Returns:
str: The final asset URL for the uploaded file
"""
+ import asyncio
+ try:
+ loop = asyncio.get_event_loop()
+ except RuntimeError:
+ loop = asyncio.new_event_loop()
+ asyncio.set_event_loop(loop)
+
+ return loop.run_until_complete(self._file_upload_async(file_data, content_type, filename))
+
+ async def _file_upload_async(self, file_data: bytes, content_type: str, filename: str) -> str:
+ """Internal async implementation of file upload."""
mutation = """
mutation fileUpload($contentType: String!, $filename: String!, $size: Int!) {
fileUpload(contentType: $contentType, filename: $filename, size: $size) {
success
uploadFile {
uploadUrl
assetUrl
headers {
key
value
}
}
}
}
"""
variables = {
"contentType": content_type,
"filename": filename,
"size": len(file_data)
}
response = self._execute_query(mutation, variables)
if (
not response
or "fileUpload" not in response
or not response["fileUpload"].get("uploadFile")
):
raise ValueError("Failed to get file upload URL")
upload_info = response["fileUpload"]["uploadFile"]
headers = {
'Content-Type': content_type,
'Cache-Control': 'public, max-age=31536000'
}
for header in upload_info['headers']:
headers[header['key']] = header['value']
async with aiohttp.ClientSession() as session:
async with session.put(
upload_info['uploadUrl'],
headers=headers,
data=file_data,
timeout=aiohttp.ClientTimeout(total=300)
) as upload_response:
if upload_response.status not in (200, 201):
raise ValueError(f"Upload failed with status {upload_response.status}")
return upload_info['assetUrl']🧰 Tools
🪛 Ruff (0.14.10)
842-842: Avoid specifying long messages outside the exception class
(TRY003)
861-861: Avoid specifying long messages outside the exception class
(TRY003)
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.