✨(drive) use url_permalink and limit request to drive resource server#587
Merged
Conversation
📝 WalkthroughWalkthroughBackend: GET title param made optional; POST implements get-or-create: searches Drive by title+size, returns 200 if found or creates/uploads and returns 201; network errors return 502. Frontend: added in-memory driveUploadStore and updated upload button to use get-or-create flow. Tests updated for new flows. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant API as Backend API
participant Drive as Third‑party Drive API
Client->>API: POST /third-party/drive/ (attachment: title, size, content/policy)
API->>Drive: GET /items/?title={title}&is_creator_me=true
alt Drive returns item with matching size
Drive-->>API: 200 PartialDriveItem (existing)
API-->>Client: 200 OK (existing item)
else No match or size differs
Drive-->>API: 200 No matching item
API->>Drive: POST /items/ (create file)
Drive-->>API: 201 Created (item id)
API->>Drive: POST /items/{id}/upload (request upload policy)
Drive-->>API: 200 Upload policy (presigned URL)
API->>Drive: PUT presigned-url (upload content)
Drive-->>API: 200 Upload success
API->>Drive: POST /items/{id}/upload-ended (finalize)
Drive-->>API: 200 Finalized
API-->>Client: 201 Created (new item)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/core/api/viewsets/drive.py (1)
186-213:⚠️ Potential issue | 🟠 MajorWrap Drive/S3 requests in try-except to prevent transient failures from bubbling as 500 errors.
The
_create_drive_itemmethod has three consecutiveraise_for_status()calls (lines 195, 205, 213) without exception handling. Any transient Drive or S3 failure will escape uncaught. The same file demonstrates the correct pattern in_find_existing_drive_item(lines 171–176): wrap withtry-except requests.exceptions.RequestException, log the failure, and return a controlled error. Apply the same approach to all three requests here, and consider usingcapture_exception()for Sentry reporting as per the backend's standard error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets/drive.py` around lines 186 - 213, The three external calls in _create_drive_item (the initial requests.post to create the item, the requests.put upload_response to S3 using item["policy"], and the final requests.post to notify upload-ended) should each be wrapped in try-except blocks catching requests.exceptions.RequestException; on exception call capture_exception(), log the error (using the same logger used in _find_existing_drive_item), and return a controlled error/response (not raise) consistent with _find_existing_drive_item’s behavior so transient Drive/S3 failures don’t bubble as 500s. Ensure you reference and use the same variables (item, upload_response) and headers flow, and keep response.raise_for_status() inside the try so HTTP errors are also caught and handled the same way.
🧹 Nitpick comments (2)
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsx (1)
31-33: Re-syncdriveFileIdwhenattachment.blobIdchanges.This state reads from
driveUploadStoreonly on the first render. If this component is ever reused for a different attachment instance, it will keep the previous file ID and can show the wrong preview link until remount. If the parent does not guarantee remounts on blob changes, add a small sync effect here.♻️ Suggested change
const [driveFileId, setDriveFileId] = useState<string | undefined>( () => driveUploadStore.get(attachment.blobId), ); + + useEffect(() => { + setDriveFileId(driveUploadStore.get(attachment.blobId)); + }, [attachment.blobId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsx` around lines 31 - 33, The driveFileId state is only initialized once from driveUploadStore and won't update if attachment.blobId changes; add a useEffect that watches attachment.blobId and calls setDriveFileId(driveUploadStore.get(attachment.blobId)) (or undefined if absent) so driveFileId stays in sync with the store when a new attachment is reused; reference the existing driveFileId/setDriveFileId state and driveUploadStore.get(...) in the effect and ensure the dependency array includes attachment.blobId.src/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsx (1)
32-35: Keep a fallback untilurl_permalinkis guaranteed everywhere.
DriveFile.urlis later used verbatim for attachment links, so a missingurl_permalinkwould turn both thread links and serialized mail links into broken URLs. If mixed Drive versions are still possible during rollout,item.url_permalink ?? item.urlis a safer transition.♻️ Suggested change
const serializeToDriveFile = (item: Item): DriveFile => ({ id: item.id, name: item.title, - url: item.url_permalink, + url: item.url_permalink ?? item.url, type: item.type, size: item.size, created_at: new Date().toISOString(), });Is `url_permalink` guaranteed in `PickerResult.items` from `@gouvfr-lasuite/drive-sdk`, and from which Drive/SDK version is it available?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsx` around lines 32 - 35, serializeToDriveFile currently maps Drive item.url_permalink directly to DriveFile.url which can produce broken links when url_permalink is absent; change the mapping in serializeToDriveFile (function) to use a fallback: set DriveFile.url = item.url_permalink ?? item.url so both older and newer Drive/SDK items serialize correctly (update the Item/DriveFile mapping logic and any callers that assume url_permalink exists).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/viewsets/drive.py`:
- Around line 163-176: The current dedupe lookup only inspects the first page
and treats request errors as “not found”; update the search flow in the method
that calls search_response and _create_drive_item to fully paginate the items/
endpoint (iterating through response.json()['results'] and following a
next/next_page or next link until exhausted) and check every page for
attachment["name"], and change the error handling so
RequestException/5xx/timeouts are propagated or returned as an error (not
silently treated as not found) so callers won’t proceed to _create_drive_item()
when the lookup is inconclusive.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-store.ts`:
- Around line 6-13: The driveFileIds Map used by driveUploadStore.get and .set
is global and only keyed by blobId, causing stale cross-user/workspace lookups;
modify driveUploadStore to scope entries by adding a discriminator (e.g.,
prepend or compose keys with currentUserId or currentWorkspaceId when calling
driveUploadStore.set/get) or add a clear method that is invoked on
auth/workspace change events; update callers to use the new scoped key or call
driveUploadStore.clear() on user/workspace switch so driveFileIds cannot leak
between contexts.
---
Outside diff comments:
In `@src/backend/core/api/viewsets/drive.py`:
- Around line 186-213: The three external calls in _create_drive_item (the
initial requests.post to create the item, the requests.put upload_response to S3
using item["policy"], and the final requests.post to notify upload-ended) should
each be wrapped in try-except blocks catching
requests.exceptions.RequestException; on exception call capture_exception(), log
the error (using the same logger used in _find_existing_drive_item), and return
a controlled error/response (not raise) consistent with
_find_existing_drive_item’s behavior so transient Drive/S3 failures don’t bubble
as 500s. Ensure you reference and use the same variables (item, upload_response)
and headers flow, and keep response.raise_for_status() inside the try so HTTP
errors are also caught and handled the same way.
---
Nitpick comments:
In
`@src/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsx`:
- Around line 32-35: serializeToDriveFile currently maps Drive
item.url_permalink directly to DriveFile.url which can produce broken links when
url_permalink is absent; change the mapping in serializeToDriveFile (function)
to use a fallback: set DriveFile.url = item.url_permalink ?? item.url so both
older and newer Drive/SDK items serialize correctly (update the Item/DriveFile
mapping logic and any callers that assume url_permalink exists).
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsx`:
- Around line 31-33: The driveFileId state is only initialized once from
driveUploadStore and won't update if attachment.blobId changes; add a useEffect
that watches attachment.blobId and calls
setDriveFileId(driveUploadStore.get(attachment.blobId)) (or undefined if absent)
so driveFileId stays in sync with the store when a new attachment is reused;
reference the existing driveFileId/setDriveFileId state and
driveUploadStore.get(...) in the effect and ensure the dependency array includes
attachment.blobId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ca235e5-1cc9-4bd3-9a6e-0ac7cd639bcf
📒 Files selected for processing (7)
src/backend/core/api/viewsets/drive.pysrc/backend/core/tests/api/test_drive.pysrc/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-preview-link.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-store.ts
jbpenrath
force-pushed
the
drive/permalink
branch
2 times, most recently
from
6a3f7ba to
f8996ff
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/core/api/openapi.json (1)
4724-4744:⚠️ Potential issue | 🟡 MinorAdd the upstream failure response to the contract.
This endpoint now documents only the two success cases. If the backend can still surface Drive upstream failures for the lookup/upload flow, the
502response should be declared here too so generated clients and frontend handlers can distinguish that failure mode from local request errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/openapi.json` around lines 4724 - 4744, The OpenAPI operation's "responses" object currently lists only 200 and 201; add a "502" response entry to declare upstream Drive failures so clients can handle that distinct error. Modify the same responses object shown in openapi.json to include a "502" key with a description like "Upstream Drive failure" and an application/json content block whose schema references the existing common error model (for example "#/components/schemas/ErrorResponse" or create "#/components/schemas/UpstreamError" if none exists); ensure the response shape matches other error schemas used elsewhere in the spec so generated clients can parse it.
🧹 Nitpick comments (5)
src/backend/core/api/openapi.json (2)
4674-4674: Document what happens whentitleis omitted.Line 4674 now describes an optional query parameter, but it does not say whether omitting
titlereturns all user files or applies some other default behavior. That is worth making explicit in the schema text for client consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/openapi.json` at line 4674, Update the "description" for the optional query parameter "title" in src/backend/core/api/openapi.json to explicitly state the behavior when it is omitted (e.g., "If omitted, returns all user files" or the actual default behavior used by the API). Locate the parameter named "title" in the relevant operation/object and append a clear sentence such as "If omitted, the endpoint returns all files for the authenticated user" so client consumers know the default semantics.
4700-4700: Call out the new200success path in migration notes.Returning
200for deduplicated uploads is a contract change for any client that previously treated201as the only successful outcome. Please make that explicit in the release/migration docs before release. Based on learnings: In src/backend/core/api/openapi.json, allow breaking API changes prior to a stable release. Do not maintain deprecated aliases for renamed fields (e.g., count_messages/count_unread_messages -> count_threads/count_unread_threads). Ensure consumers are notified and updated, and document the migration path (including versioning notes) so downstream services can adapt before the release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/openapi.json` at line 4700, Update the release/migration notes to explicitly call out the new 200 success path for deduplicated uploads described by the OpenAPI operation whose description begins "Save an attachment to the user's Drive workspace..." so clients that relied on 201-only semantics can update; in that migration note reference the exact OpenAPI change (200 vs 201 behavior), state the recommended client behavior change, include versioning notes and a concrete migration path for downstream services, and remove any guidance about keeping deprecated aliases for renamed fields (e.g., count_messages/count_unread_messages → count_threads/count_unread_threads) while instructing consumers to switch to the new field names before the stable release.src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsx (1)
90-91: Simplify redundant disabled condition.The condition
state === 'uploading' || state !== 'idle'is redundant sincestate !== 'idle'already covers the'uploading'case.♻️ Simplified condition
- disabled={state === 'uploading' || state !== 'idle'} + disabled={state !== 'idle'}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsx` around lines 90 - 91, The disabled prop condition is redundant: replace the expression used in the DriveUploadButton component (currently "disabled={state === 'uploading' || state !== 'idle'}") with the simplified check "disabled={state !== 'idle'}" so it correctly disables the button for any non-idle state while leaving aria-busy={state === 'uploading'} unchanged.src/backend/core/api/viewsets/drive.py (1)
217-226: Consider validating thepolicyfield before use.The code assumes
item["policy"]exists in the Drive API response. If this field is missing, aKeyErrorwould propagate and be caught by the outer exception handler (returning 502), which is acceptable. However, explicit validation could provide a clearer error message.💡 Optional: Add explicit validation
response.raise_for_status() item = response.json() + if "policy" not in item: + raise ValueError("Drive API response missing 'policy' field") + # Upload file content using the presigned URL upload_response = requests.put( item["policy"],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets/drive.py` around lines 217 - 226, The code assumes item["policy"] exists before calling requests.put; add explicit validation right after parsing the response (the variable item) to check for the "policy" key (e.g., if "policy" not in item or not item["policy"]): raise or return a clear error (with context about missing policy and any request id) instead of letting a KeyError propagate, and only call requests.put with item["policy"] after that check; reference variables item, attachment, and the requests.put/upload_response flow when making this change.src/backend/core/tests/api/test_drive.py (1)
438-489: Consider adding test for search failure during POST.The test suite covers the S3 upload failure path but doesn't test the case when the initial search request fails (lines 159-166 in drive.py). This would verify the "Failed to search Drive for existing file" error response.
📝 Example test case
`@responses.activate` `@patch`( "lasuite.oidc_login.middleware.RefreshOIDCAccessToken.is_expired", return_value=False, ) def test_api_third_party_drive_post_search_fails( self, _mock, api_client_with_user, mailbox_with_message ): """Test handling of search failure when checking for existing file.""" client, _ = api_client_with_user _, message = mailbox_with_message blob_id = f"msg_{message.id}_0" # Mock the search to fail responses.add( responses.GET, "http://drive.test/external_api/v1.0/items/", status=status.HTTP_500_INTERNAL_SERVER_ERROR, ) response = client.post( reverse("drive"), {"blob_id": blob_id}, format="json", ) assert response.status_code == status.HTTP_502_BAD_GATEWAY assert response.json()["error"] == "Failed to search Drive for existing file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_drive.py` around lines 438 - 489, Add a new test (e.g., test_api_third_party_drive_post_search_fails) in src/backend/core/tests/api/test_drive.py that mirrors test_api_third_party_drive_post_file_already_exists but mocks the Drive search GET to return a 500 (using responses.add for "http://drive.test/external_api/v1.0/items/") and asserts the POST to reverse("drive") with the same blob_id returns HTTP_502_BAD_GATEWAY and JSON error "Failed to search Drive for existing file"; keep the RefreshOIDCAccessToken.is_expired patch and mailbox/api_client fixtures the same so only the search request fails and no other calls are expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/backend/core/api/openapi.json`:
- Around line 4724-4744: The OpenAPI operation's "responses" object currently
lists only 200 and 201; add a "502" response entry to declare upstream Drive
failures so clients can handle that distinct error. Modify the same responses
object shown in openapi.json to include a "502" key with a description like
"Upstream Drive failure" and an application/json content block whose schema
references the existing common error model (for example
"#/components/schemas/ErrorResponse" or create
"#/components/schemas/UpstreamError" if none exists); ensure the response shape
matches other error schemas used elsewhere in the spec so generated clients can
parse it.
---
Nitpick comments:
In `@src/backend/core/api/openapi.json`:
- Line 4674: Update the "description" for the optional query parameter "title"
in src/backend/core/api/openapi.json to explicitly state the behavior when it is
omitted (e.g., "If omitted, returns all user files" or the actual default
behavior used by the API). Locate the parameter named "title" in the relevant
operation/object and append a clear sentence such as "If omitted, the endpoint
returns all files for the authenticated user" so client consumers know the
default semantics.
- Line 4700: Update the release/migration notes to explicitly call out the new
200 success path for deduplicated uploads described by the OpenAPI operation
whose description begins "Save an attachment to the user's Drive workspace..."
so clients that relied on 201-only semantics can update; in that migration note
reference the exact OpenAPI change (200 vs 201 behavior), state the recommended
client behavior change, include versioning notes and a concrete migration path
for downstream services, and remove any guidance about keeping deprecated
aliases for renamed fields (e.g., count_messages/count_unread_messages →
count_threads/count_unread_threads) while instructing consumers to switch to the
new field names before the stable release.
In `@src/backend/core/api/viewsets/drive.py`:
- Around line 217-226: The code assumes item["policy"] exists before calling
requests.put; add explicit validation right after parsing the response (the
variable item) to check for the "policy" key (e.g., if "policy" not in item or
not item["policy"]): raise or return a clear error (with context about missing
policy and any request id) instead of letting a KeyError propagate, and only
call requests.put with item["policy"] after that check; reference variables
item, attachment, and the requests.put/upload_response flow when making this
change.
In `@src/backend/core/tests/api/test_drive.py`:
- Around line 438-489: Add a new test (e.g.,
test_api_third_party_drive_post_search_fails) in
src/backend/core/tests/api/test_drive.py that mirrors
test_api_third_party_drive_post_file_already_exists but mocks the Drive search
GET to return a 500 (using responses.add for
"http://drive.test/external_api/v1.0/items/") and asserts the POST to
reverse("drive") with the same blob_id returns HTTP_502_BAD_GATEWAY and JSON
error "Failed to search Drive for existing file"; keep the
RefreshOIDCAccessToken.is_expired patch and mailbox/api_client fixtures the same
so only the search request fails and no other calls are expected.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsx`:
- Around line 90-91: The disabled prop condition is redundant: replace the
expression used in the DriveUploadButton component (currently "disabled={state
=== 'uploading' || state !== 'idle'}") with the simplified check
"disabled={state !== 'idle'}" so it correctly disables the button for any
non-idle state while leaving aria-busy={state === 'uploading'} unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a47dc2b-279f-401a-aa2b-3184cd68ed29
⛔ Files ignored due to path filters (2)
src/frontend/src/features/api/gen/models/third_party_drive_retrieve_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/third-party-drive/third-party-drive.tsis excluded by!**/gen/**
📒 Files selected for processing (8)
src/backend/core/api/openapi.jsonsrc/backend/core/api/viewsets/drive.pysrc/backend/core/tests/api/test_drive.pysrc/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-preview-link.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-store.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-preview-link.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/_index.scss
jbpenrath
force-pushed
the
drive/permalink
branch
from
f8996ff to
51ba88d
Compare
With the latest version of Drive, items have no a url_permalink property which is more robust than the previous url property we were using as download link for drive attachments. Furthermore, we totally revamp the logic to save attachment to drive. Actually the current implementation triggered n requests for n attachments each time a user opened a thread... that was great for ux as the user always know if the attachment exists in its workspace but it triggers too muck request to be a production ready implementation. So now, the user is no more able to know if an attachment exists in its workspace until it clicks to upload the attachment and the backend checks if the file already exists in the user workspace. Finally, we also replace the DriveIcon which was used to open drive item preview by an eye icon. Resolve #567
jbpenrath
force-pushed
the
drive/permalink
branch
from
51ba88d to
7fba088
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/backend/core/api/viewsets/drive.py (1)
187-203:⚠️ Potential issue | 🟠 MajorTighten the de-dup match before treating a search hit as “existing.”
This helper still accepts the first same-size result from the first page only. Because Drive search is not guaranteed to be exact, that can alias a different file with a partial title match; and if the real match is on a later page, the code will still create a duplicate. Re-check the returned item's exact name/title and follow
nextpages before falling back to creation.Suggested fix
- search_response = requests.get( - f"{self.drive_external_api}/items/", - params={ - "is_creator_me": True, - "type": "file", - "title": attachment["name"], - }, - headers=headers, - timeout=5, - ) - search_response.raise_for_status() - - for item in search_response.json().get("results", []): - if item.get("size") == attachment["size"]: - return item + next_url = f"{self.drive_external_api}/items/" + params = { + "is_creator_me": True, + "type": "file", + "title": attachment["name"], + } + + while next_url: + search_response = requests.get( + next_url, + params=params, + headers=headers, + timeout=5, + ) + search_response.raise_for_status() + payload = search_response.json() + + for item in payload.get("results", []): + item_name = item.get("filename") or item.get("title") + if item_name == attachment["name"] and item.get("size") == attachment["size"]: + return item + + next_url = payload.get("next") + params = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets/drive.py` around lines 187 - 203, The current search only checks the first page and accepts a same-size result that may be a partial title match; update the lookup loop in the drive search logic that builds search_response (using self.drive_external_api, search_response, headers, and attachment) to iterate through paginated results by following the response JSON "next" link until exhausted, and for each item require both exact title/name equality (compare item["title"] or item["name"] to attachment["name"]) and matching size before returning the item; only return None after all pages are checked.
🧹 Nitpick comments (2)
src/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsx (1)
18-19: Consider makingurl_permalinkoptional to match runtime behavior.The type declares
url_permalinkas required, but line 38 uses nullish coalescing (item.url_permalink ?? item.url), implying it may be absent. Making it optional aligns the type with actual runtime behavior:-type PatchedItem = Item & { url_permalink: string }; +type PatchedItem = Item & { url_permalink?: string };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsx` around lines 18 - 19, The PatchedItem type currently forces url_permalink as required but runtime uses it as possibly absent (see usage of item.url_permalink ?? item.url); change the type definition to make url_permalink optional (e.g., url_permalink?: string) so it matches actual runtime behavior and avoid false positives in type checks involving Item and PatchedItem.src/backend/core/api/viewsets/drive.py (1)
85-101: Capture handled Drive failures before returning502.These branches log and translate the
RequestException, but they never callcapture_exception(...), so swallowed upstream failures will not be reported to Sentry. Please capture the exception before building the 502 response.As per coding guidelines "Capture and report exceptions to Sentry; use capture_exception() for custom errors".
Also applies to: 159-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets/drive.py` around lines 85 - 101, The except block handling requests.exceptions.RequestException around the requests.get call should capture the exception with capture_exception(...) before logging and returning the 502; modify the except handler in the requests.get block (and the similar handler at the later branch covering lines 159-179) to accept the exception as a variable (e.g., except requests.exceptions.RequestException as e:) and call capture_exception(e) prior to logger.exception(...) and before returning the Response so the failure is reported to Sentry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsx`:
- Line 63: The useCallback for the pick function captures onPick and
config.DRIVE but only lists isDriveDisabled in its dependency array, causing a
stale closure; update the dependency array of the pick callback (the useCallback
that defines pick in drive-attachment-picker.tsx) to include onPick and
config.DRIVE (and any other referenced values) so the callback updates when
those change, ensuring pick always calls the current onPick and uses the current
config.DRIVE.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsx`:
- Around line 31-33: The local state driveFileId (initialized from
driveUploadStore.get in the useState) is never reseeded when attachment.blobId
changes; add a useEffect in drive-upload-button.tsx that watches
attachment.blobId and calls
setDriveFileId(driveUploadStore.get(attachment.blobId)) so the component resets
the cached value whenever the attachment changes (use dependency
[attachment.blobId]).
---
Duplicate comments:
In `@src/backend/core/api/viewsets/drive.py`:
- Around line 187-203: The current search only checks the first page and accepts
a same-size result that may be a partial title match; update the lookup loop in
the drive search logic that builds search_response (using
self.drive_external_api, search_response, headers, and attachment) to iterate
through paginated results by following the response JSON "next" link until
exhausted, and for each item require both exact title/name equality (compare
item["title"] or item["name"] to attachment["name"]) and matching size before
returning the item; only return None after all pages are checked.
---
Nitpick comments:
In `@src/backend/core/api/viewsets/drive.py`:
- Around line 85-101: The except block handling
requests.exceptions.RequestException around the requests.get call should capture
the exception with capture_exception(...) before logging and returning the 502;
modify the except handler in the requests.get block (and the similar handler at
the later branch covering lines 159-179) to accept the exception as a variable
(e.g., except requests.exceptions.RequestException as e:) and call
capture_exception(e) prior to logger.exception(...) and before returning the
Response so the failure is reported to Sentry.
In
`@src/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsx`:
- Around line 18-19: The PatchedItem type currently forces url_permalink as
required but runtime uses it as possibly absent (see usage of item.url_permalink
?? item.url); change the type definition to make url_permalink optional (e.g.,
url_permalink?: string) so it matches actual runtime behavior and avoid false
positives in type checks involving Item and PatchedItem.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 349a2166-db14-4e5e-869f-1c07c8c0558d
⛔ Files ignored due to path filters (2)
src/frontend/src/features/api/gen/models/third_party_drive_retrieve_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/third-party-drive/third-party-drive.tsis excluded by!**/gen/**
📒 Files selected for processing (8)
src/backend/core/api/openapi.jsonsrc/backend/core/api/viewsets/drive.pysrc/backend/core/tests/api/test_drive.pysrc/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-preview-link.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-store.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-store.ts
- src/backend/core/api/openapi.json
| onPick(result.items.map(serializeToDriveFile)); | ||
| onPick((result.items as PatchedItem[]).map(serializeToDriveFile)); | ||
| } | ||
| }, [isDriveDisabled]); |
There was a problem hiding this comment.
Missing dependencies in useCallback.
The pick callback references onPick and config.DRIVE but neither is included in the dependency array. If onPick changes between renders, the stale closure will invoke the old callback.
🐛 Proposed fix
- }, [isDriveDisabled]);
+ }, [isDriveDisabled, onPick, config.DRIVE]);📝 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
| }, [isDriveDisabled]); | |
| }, [isDriveDisabled, onPick, config.DRIVE]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsx`
at line 63, The useCallback for the pick function captures onPick and
config.DRIVE but only lists isDriveDisabled in its dependency array, causing a
stale closure; update the dependency array of the pick callback (the useCallback
that defines pick in drive-attachment-picker.tsx) to include onPick and
config.DRIVE (and any other referenced values) so the callback updates when
those change, ensuring pick always calls the current onPick and uses the current
config.DRIVE.
Comment on lines
+31
to
+33
| const [driveFileId, setDriveFileId] = useState<string | undefined>( | ||
| () => driveUploadStore.get(attachment.blobId), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsxRepository: suitenumerique/messages
Length of output: 5654
🏁 Script executed:
fd -t f "thread-attachment-list" src/frontend/src/features/layouts/components/thread-view/components/Repository: suitenumerique/messages
Length of output: 49
🏁 Script executed:
ls -la src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/Repository: suitenumerique/messages
Length of output: 591
🏁 Script executed:
cat -n src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsxRepository: suitenumerique/messages
Length of output: 2435
🏁 Script executed:
cat -n src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsxRepository: suitenumerique/messages
Length of output: 6317
🏁 Script executed:
cat -n src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-store.tsRepository: suitenumerique/messages
Length of output: 621
Resync local state when the attachment's blobId changes.
The useState initializer only runs on component mount. If this component instance is reused for a different attachment (with a different blobId), the cached driveFileId will persist with the old value, potentially displaying the wrong Drive preview link until another upload completes. Add a useEffect to reset/reseed driveFileId when attachment.blobId changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsx`
around lines 31 - 33, The local state driveFileId (initialized from
driveUploadStore.get in the useState) is never reseeded when attachment.blobId
changes; add a useEffect in drive-upload-button.tsx that watches
attachment.blobId and calls
setDriveFileId(driveUploadStore.get(attachment.blobId)) so the component resets
the cached value whenever the attachment changes (use dependency
[attachment.blobId]).
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.
Purpose
With the latest version of Drive, items have now a url_permalink property which is more robust than the previous url property we were using as download link for drive attachments.
Furthermore, we totally revamp the logic to save attachment to drive. Actually the current implementation triggered n requests for n attachments each time a user opened a new thread... that was great for ux as the user always know if the attachment exists in its workspace but it triggers too much request to be a production ready implementation. So now, the user is no more able to know if an attachment exists in its workspace until it clicks to upload the attachment and the backend checks if the file already exists in the user workspace.
Finally, we also replace the DriveIcon which was used to open drive item preview by an eye icon.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests