fix: Address sharepoint folder handling#1557
Merged
Merged
Conversation
github-actions
Bot
added
frontend
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds folder-aware filtering to connector file-ID expansion/fallbacks, refactors SharePoint connector (auth, Graph requests, selective listing, downloads, ACL extraction, webhook handling), stops picker handlers from requesting download URLs, updates FileItem to render folders distinctly, and includes an isFolder boolean in upload sync payloads. ChangesConnector sync & SharePoint refactor
Sequence Diagram(s)sequenceDiagram
participant Browser
participant UploadPage
participant FilePicker
participant ConnectorService
Browser->>UploadPage: open /upload/[provider]
alt non-direct provider (picker)
UploadPage->>FilePicker: render file-picker
FilePicker->>UploadPage: user selects items (no download URLs requested)
UploadPage->>ConnectorService: syncMutation(selected_files includes isFolder)
else connector sync path
UploadPage->>ConnectorService: trigger sync with file_ids
ConnectorService->>ConnectorService: list_files() expansion
ConnectorService->>ConnectorService: filter out folders when needed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
github-actions
Bot
added
bug
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/connectors/service.py`:
- Around line 392-400: The code currently raises ValueError when only folders
are selected (from the block using file_infos/non_folder_ids/expanded_file_ids)
but a later except block catches that ValueError and resets expanded_file_ids to
the original file_ids, reintroducing folders; remove or change that except so
the ValueError is not swallowed—either let the ValueError propagate or re-raise
it in the except around the expansion logic (do not assign expanded_file_ids =
file_ids), ensuring the validation that disallows folders-only selection remains
enforced (refer to variables expanded_file_ids, file_infos, non_folder_ids and
the try/except that handles the folder-expansion result).
🪄 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: f584ddf7-dc77-4151-90c7-8989ae50d8ed
📒 Files selected for processing (3)
frontend/app/upload/[provider]/page.tsxsrc/connectors/service.pysrc/connectors/sharepoint/connector.py
github-actions
Bot
added
bug
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/connectors/service.py (1)
410-413:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid reintroducing folder IDs in the non-
ValueErrorfallbackIf
list_files()fails with a non-ValueErrorandfile_infoscontains only folders,expanded_file_ids = non_folder_ids or file_idsfalls back to the original folder IDs. That can enqueue non-downloadable items again.Suggested fix
- if file_infos: - non_folder_ids = [f["id"] for f in file_infos if f.get("id") and not f.get("isFolder")] - expanded_file_ids = non_folder_ids or file_ids + if file_infos: + non_folder_ids = [ + f["id"] for f in file_infos if f.get("id") and not f.get("isFolder") + ] + if non_folder_ids: + expanded_file_ids = non_folder_ids + else: + raise ValueError("No files to sync after excluding folders") else: expanded_file_ids = file_ids🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/connectors/service.py` around lines 410 - 413, The current logic in the block that processes file_infos after list_files() (variables file_infos, non_folder_ids, expanded_file_ids) can reintroduce folder IDs because expanded_file_ids = non_folder_ids or file_ids falls back to the original file_ids when non_folder_ids is empty; change the fallback so that when no non-folder IDs are found (i.e., file_infos contains only folders or is empty after a non-ValueError failure) you set expanded_file_ids to an empty list (or otherwise exclude folder IDs) instead of file_ids, and ensure downstream code that enqueues downloads handles an empty expanded_file_ids correctly to avoid re-enqueuing non-downloadable folder items.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/connectors/service.py`:
- Around line 410-413: The current logic in the block that processes file_infos
after list_files() (variables file_infos, non_folder_ids, expanded_file_ids) can
reintroduce folder IDs because expanded_file_ids = non_folder_ids or file_ids
falls back to the original file_ids when non_folder_ids is empty; change the
fallback so that when no non-folder IDs are found (i.e., file_infos contains
only folders or is empty after a non-ValueError failure) you set
expanded_file_ids to an empty list (or otherwise exclude folder IDs) instead of
file_ids, and ensure downstream code that enqueues downloads handles an empty
expanded_file_ids correctly to avoid re-enqueuing non-downloadable folder items.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a198bf66-1ee3-4733-920d-07ecf55a32d2
📒 Files selected for processing (1)
src/connectors/service.py
github-actions
Bot
added
bug
…ag into fix/sharepoint-folders
github-actions
Bot
added
bug
github-actions
Bot
added
bug
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/components/cloud-picker/file-item.tsx (1)
19-24: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRemove dead code from helper functions.
Since folders are now explicitly handled via
file.isFolderbefore these helper functions are called, the folder-specific logic in these functions is unreachable:
- Lines 20-22 in
getFileIcon: ThemimeType.includes("folder")check can never execute becausegetFileIconis only called whenfile.isFolderisfalse(line 79).- Line 31 in
getMimeTypeLabel: The folder mime type case is unreachable becausegetMimeTypeLabelis only called whenfile.isFolderisfalse(line 85).♻️ Proposed cleanup
const getFileIcon = (mimeType: string) => { - if (mimeType.includes("folder")) { - return <Folder className="h-6 w-6" />; - } return <FileText className="h-6 w-6" />; };const getMimeTypeLabel = (mimeType: string) => { const typeMap: { [key: string]: string } = { "application/vnd.google-apps.document": "Google Doc", "application/vnd.google-apps.spreadsheet": "Google Sheet", "application/vnd.google-apps.presentation": "Google Slides", - "application/vnd.google-apps.folder": "Folder", "application/pdf": "PDF", "text/plain": "Text", "application/vnd.openxmlformats-officedocument.wordprocessingml.document": "Word Doc", "application/vnd.openxmlformats-officedocument.presentationml.presentation": "PowerPoint", };Also applies to: 31-31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/cloud-picker/file-item.tsx` around lines 19 - 24, Remove unreachable folder-specific logic from the helper functions: edit getFileIcon and getMimeTypeLabel in file-item.tsx to drop any branches that check mimeType.includes("folder") or return a "folder" mime label, because callers already guard with file.isFolder; simplify getFileIcon to always return the file icon (e.g., FileText) for non-folder files and simplify getMimeTypeLabel to only handle non-folder mime types so there are no dead branches left.src/connectors/sharepoint/connector.py (1)
1-11:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winImport
get_loggerfromutils.logging_configinstead of stdliblogging.The code at line 274–276 passes
status_code=as a keyword argument tologger.warning(). Python's stdliblogging.Loggerdoes not accept arbitrary keyword arguments—onlyexc_info,extra,stack_info, andstacklevel. This will raiseTypeError: warning() got an unexpected keyword argument 'status_code'when the SharePoint URL detection fails.The project's structured logger from
utils.logging_config.get_logger()supports these kwargs. Switch to the project convention to fix the runtime bug:♻️ Proposed fix
-import logging from pathlib import Path from typing import List, Dict, Any, Optional from urllib.parse import urlparse from datetime import datetime import httpx from ..base import BaseConnector, ConnectorDocument, DocumentACL from .oauth import SharePointOAuth +from utils.logging_config import get_logger -logger = logging.getLogger(__name__) +logger = get_logger(__name__)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/connectors/sharepoint/connector.py` around lines 1 - 11, Replace the stdlib logger with the project's structured logger: import get_logger from utils.logging_config and replace logger = logging.getLogger(__name__) with logger = get_logger(__name__); this will allow the existing logger.warning(...) calls that pass extra kwargs like status_code to work without raising TypeError (ensure the module-level symbol logger is updated so functions such as the SharePoint URL detection that call logger.warning(status_code=...) use the new get_logger-backed logger).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/connectors/sharepoint/connector.py`:
- Around line 274-276: Replace the use of the stdlib logging module with the
project's structured logger: remove or replace any "import logging" and
initialize logger using get_logger(__name__) from utils.logging_config (i.e.,
call get_logger and assign to logger at module scope) so that calls like
logger.warning("[CONNECTOR] SharePoint detect URL failed",
status_code=response.status_code) (in the SharePoint connector functions such as
the URL autodetect path that calls /me/drive) will accept keyword args; ensure
you update any other logger initializations in this module to use
get_logger(__name__) to match other connectors.
---
Outside diff comments:
In `@frontend/components/cloud-picker/file-item.tsx`:
- Around line 19-24: Remove unreachable folder-specific logic from the helper
functions: edit getFileIcon and getMimeTypeLabel in file-item.tsx to drop any
branches that check mimeType.includes("folder") or return a "folder" mime label,
because callers already guard with file.isFolder; simplify getFileIcon to always
return the file icon (e.g., FileText) for non-folder files and simplify
getMimeTypeLabel to only handle non-folder mime types so there are no dead
branches left.
In `@src/connectors/sharepoint/connector.py`:
- Around line 1-11: Replace the stdlib logger with the project's structured
logger: import get_logger from utils.logging_config and replace logger =
logging.getLogger(__name__) with logger = get_logger(__name__); this will allow
the existing logger.warning(...) calls that pass extra kwargs like status_code
to work without raising TypeError (ensure the module-level symbol logger is
updated so functions such as the SharePoint URL detection that call
logger.warning(status_code=...) use the new get_logger-backed logger).
🪄 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: b69b4b50-5c85-4a33-82c7-86beb9595433
📒 Files selected for processing (5)
frontend/components/cloud-picker/file-item.tsxfrontend/components/cloud-picker/onedrive-v8-handler.tsfrontend/components/cloud-picker/sharepoint-v8-handler.tssrc/connectors/service.pysrc/connectors/sharepoint/connector.py
💤 Files with no reviewable changes (2)
- frontend/components/cloud-picker/onedrive-v8-handler.ts
- frontend/components/cloud-picker/sharepoint-v8-handler.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/connectors/service.py
github-actions
Bot
added
bug
github-actions
Bot
added
bug
edwinjosechittilappilly
self-requested a review
May 12, 2026 20:13
github-actions
Bot
added
bug
github-actions
Bot
added
bug
github-actions
Bot
added
bug
ricofurtado
pushed a commit
that referenced
this pull request
May 23, 2026
* sharepoint folder handling * code rabbit fix * remove download url * style: ruff format (auto) * logging issue * linting --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This was referenced May 25, 2026
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
2 participants
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.
This pull request introduces improvements to file handling and logging, particularly in how folders are treated during file synchronization and how errors are logged for SharePoint file metadata retrieval. It also ensures that folder information is surfaced to the frontend.
File synchronization improvements:
sync_specific_files, preventing attempts to download folders which have no downloadable content. If only folders are selected, an error is raised.Error logging enhancements:
Frontend data improvements:
isFolderproperty is now included in the file metadata sent to the frontend, allowing the UI to distinguish between files and folders. (frontend/app/upload/[provider]/page.tsxR416)Summary by CodeRabbit
New Features
Improvements
Bug Fixes