Skip unsupported file types during source crawl - #303
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Only sync Docling-compatible document extensions from S3/GDrive and surface skipped files in the SourceCrawler Ready status message.
158b794 to
5c95467
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change defines supported document extensions, filters unsupported files during S3 and Google Drive synchronization, records skipped files, and includes capped skipped-file details in successful reconciliation messages. ChangesUnsupported File Handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/unstructured/file_types.go (1)
26-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making the extension set immutable to callers.
SupportedFileExtensionsis an exportedmap[string]bool. Any package that importsunstructuredcan add or remove entries at runtime, for exampleunstructured.SupportedFileExtensions[".txt"] = true. This would change filtering behavior for allS3BucketSourceandGDriveSourceinstances without going throughIsSupportedFileType.Unexport the map and keep only
IsSupportedFileTypeandFileExtensionas the public surface, or expose a copy through an accessor function.♻️ Proposed fix
-// SupportedFileExtensions mirrors Docling defaultFromFormats, excluding image. -// Supported: docx, pptx, html, pdf, asciidoc, md, csv, xlsx. -var SupportedFileExtensions = map[string]bool{ +// supportedFileExtensions mirrors Docling defaultFromFormats, excluding image. +// Supported: docx, pptx, html, pdf, asciidoc, md, csv, xlsx. +var supportedFileExtensions = map[string]bool{ ".docx": true, ".pptx": true, ".html": true, ".htm": true, ".pdf": true, ".adoc": true, ".asciidoc": true, ".md": true, ".markdown": true, ".csv": true, ".xlsx": true, } func IsSupportedFileType(fileName string) bool { - return SupportedFileExtensions[strings.ToLower(path.Ext(fileName))] + return supportedFileExtensions[strings.ToLower(path.Ext(fileName))] +} + +// IsSupportedExtension checks a pre-computed, normalized extension (e.g. an +// extension overridden for Google-native document conversion). +func IsSupportedExtension(ext string) bool { + return supportedFileExtensions[strings.ToLower(ext)] }🤖 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 `@pkg/unstructured/file_types.go` around lines 26 - 38, Make the supported extension collection private instead of exposing the mutable SupportedFileExtensions map. Update IsSupportedFileType and FileExtension to use the private collection, preserving their existing public behavior; if callers require access, provide a copy-returning accessor rather than the underlying map.internal/controller/sourcecrawler_controller.go (1)
315-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider moving skipped-file reporting onto the
DataSourceinterface.
skippedUnsupportedFromSourcetype-switches on*unstructured.S3BucketSourceand*unstructured.GDriveSource. This works today becauseReconcileonly constructs these two types. If a futureDataSourceimplementation is added and this switch is not updated, its skipped files silently disappear from the status message with no compile-time warning.Add a method to the
DataSourceinterface inpkg/unstructured/source.goinstead, so every implementation must supply its own skipped-file list.♻️ Proposed fix
type DataSource interface { // SyncFilesToFilestore will store all files from the source to the filestore and return the list of file paths SyncFilesToFilestore(ctx context.Context, fs *filestore.FileStore) ([]RawFileMetadata, error) + // SkippedUnsupportedFiles returns file paths skipped during the last sync + // due to an unsupported extension. + SkippedUnsupportedFiles() []string }-func skippedUnsupportedFromSource(source unstructured.DataSource) []string { - switch s := source.(type) { - case *unstructured.S3BucketSource: - return s.SkippedUnsupported - case *unstructured.GDriveSource: - return s.SkippedUnsupported - default: - return nil - } -} +func skippedUnsupportedFromSource(source unstructured.DataSource) []string { + return source.SkippedUnsupportedFiles() +}🤖 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 `@internal/controller/sourcecrawler_controller.go` around lines 315 - 324, Move skipped-file reporting into the DataSource interface by adding a method that returns the source’s skipped unsupported files, and implement it for S3BucketSource and GDriveSource. Update skippedUnsupportedFromSource to call that interface method directly and remove its concrete-type switch, ensuring future DataSource implementations must provide the behavior.
🤖 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.
Nitpick comments:
In `@internal/controller/sourcecrawler_controller.go`:
- Around line 315-324: Move skipped-file reporting into the DataSource interface
by adding a method that returns the source’s skipped unsupported files, and
implement it for S3BucketSource and GDriveSource. Update
skippedUnsupportedFromSource to call that interface method directly and remove
its concrete-type switch, ensuring future DataSource implementations must
provide the behavior.
In `@pkg/unstructured/file_types.go`:
- Around line 26-38: Make the supported extension collection private instead of
exposing the mutable SupportedFileExtensions map. Update IsSupportedFileType and
FileExtension to use the private collection, preserving their existing public
behavior; if callers require access, provide a copy-returning accessor rather
than the underlying map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Enterprise
Run ID: cbf8ee34-b863-44a0-b8a5-15ccce6fe5e8
📒 Files selected for processing (3)
internal/controller/sourcecrawler_controller.gopkg/unstructured/file_types.gopkg/unstructured/source.go
1 participant
Summary
from_formats, excludingimage)Truewhen skips occur; real store/API failures still fail reconcileSupported file types
.pdf.md/.markdown.docx.pptx.html/.htm.csv.xlsx.adoc/.asciidocNot supported yet (skipped)
.png,.jpg,.jpeg,.gif,.bmp,.tif,.tiff,.webp) — Doclingimageformat intentionally excluded for now.txt.doc(legacy Word).ppt/.xls(legacy Office).odt/.ods/.odp.rtf.zipand other archivesExample status