This repository was archived by the owner on Jul 3, 2026. It is now read-only.
fix: remove unnecessary packages - #641
Merged
Merged
Conversation
refactor: remove dead code
This reverts commit fbd6f61.
Drop TemplateSettings (unused feature) which pulled in the full tiptap/prosemirror editor tree via frappe-ui barrel. Remove direct tiptap, yjs, and y-* packages that are no longer needed. Clean up stale optimizeDeps entries. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Confidence Score: 3/5Safe to merge for the dependency cleanup and dead-code removal; the PDF viewer and date/slug utilities need the noted fixes before the mobile PDF viewer behaves reliably across locales and error states. The PDF canvas renderer introduces an unhandled async error path that leaves users staring at a permanent loading spinner when any network or parse error occurs. The date formatter hardcodes US locale, silently breaking 24-hour time for non-US users. These are real regressions introduced on changed paths, not theoretical risks. frontend/src/components/FileTypePreview/PDFPreview.vue (error handling), frontend/src/utils/format.js (locale), frontend/src/ui/drive/js/utils.js and frontend/src/utils/files.js (slugger unicode behavior)
|
| Filename | Overview |
|---|---|
| frontend/src/components/FileTypePreview/PDFPreview.vue | Replaced @tato30/vue-pdf with a custom pdfjs-dist canvas renderer; no error handling in loadPDF/renderPage leaves an infinite spinner on load failure. |
| frontend/package.json | Removes ~15 unused packages (tiptap, yjs, slugify, date-fns, etc.) and adds pdfjs-dist directly; adds license field and typescript devDep. |
| frontend/src/utils/format.js | Replaces date-fns formatDate with Intl.DateTimeFormat but hardcodes 'en-US' locale, breaking 24-hour time display for non-US users. |
| frontend/src/ui/drive/js/utils.js | Replaces slugify with a native implementation; non-ASCII chars are dropped instead of transliterated, changing slug output for international filenames. |
| frontend/src/components/FileRender.vue | Preview components converted to defineAsyncComponent for lazy loading; clean improvement with no issues. |
| frontend/vite.config.js | Removes yjs deduplication, drops console.log, adds lightningcss customAtRules for Tailwind @-directives, and trims optimizeDeps — all safe cleanup. |
| frontend/src/utils/files.js | Mirrors the slugify-to-native-slugger replacement; same non-ASCII character concern as utils.js. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
FR[FileRender.vue\ndefineAsyncComponent] -->|lazy load| PDF[PDFPreview.vue]
FR -->|lazy load| IMG[ImagePreview.vue]
FR -->|lazy load| VID[VideoPreview.vue]
FR -->|lazy load| TXT[TextPreview.vue]
FR -->|lazy load| AUD[AudioPreview.vue]
FR -->|lazy load| MSO[MSOfficePreview.vue]
PDF --> PDFJS[pdfjs-dist\ndirect]
PDF -->|isMobile| CANVAS[Canvas Renderer\nonMounted → loadPDF]
PDF -->|desktop| EMBED[embed tag]
CANVAS -->|watch currentPage| RP[renderPage]
CANVAS -->|watch scale| RP
RP -->|no try/catch| ERR[❌ load error\n= infinite spinner]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
FR[FileRender.vue\ndefineAsyncComponent] -->|lazy load| PDF[PDFPreview.vue]
FR -->|lazy load| IMG[ImagePreview.vue]
FR -->|lazy load| VID[VideoPreview.vue]
FR -->|lazy load| TXT[TextPreview.vue]
FR -->|lazy load| AUD[AudioPreview.vue]
FR -->|lazy load| MSO[MSOfficePreview.vue]
PDF --> PDFJS[pdfjs-dist\ndirect]
PDF -->|isMobile| CANVAS[Canvas Renderer\nonMounted → loadPDF]
PDF -->|desktop| EMBED[embed tag]
CANVAS -->|watch currentPage| RP[renderPage]
CANVAS -->|watch scale| RP
RP -->|no try/catch| ERR[❌ load error\n= infinite spinner]
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
frontend/src/components/FileTypePreview/PDFPreview.vue:52-68
**Unhandled load failure leaves infinite spinner**
`loadPDF` and `renderPage` have no `try/catch`. If `PDFJS.getDocument(src.value)` or `page.render(...)` rejects (network error, corrupted PDF, permission error), `loading.value` stays `true` indefinitely — the spinner never clears and `canvasRef` stays hidden with no feedback to the user.
### Issue 2 of 3
frontend/src/utils/format.js:23-31
`Intl.DateTimeFormat` is hardcoded to `'en-US'`, so all non-US users always see 12-hour AM/PM time. The sibling `formatDate` in `src/ui/drive/js/utils.js` already handles this correctly with `navigator.language`. Consider using `undefined` (system locale) or referencing the locale-aware version.
```suggestion
const locale = navigator.language || 'en-US'
const formattedDate = new Intl.DateTimeFormat(locale, {
month: '2-digit',
day: '2-digit',
year: '2-digit',
}).format(dateObj)
const formattedTime = new Intl.DateTimeFormat(locale, {
hour: '2-digit',
minute: '2-digit',
}).format(dateObj)
```
### Issue 3 of 3
frontend/src/ui/drive/js/utils.js:46-54
**Non-ASCII characters silently dropped by native slugger**
JavaScript's `\w` matches only `[a-zA-Z0-9_]` (ASCII), so the `.replace(/[^\w\s']|_/g, '')` step strips accented or CJK characters entirely rather than transliterating them as `slugify` did. A file named `"Résumé.pdf"` now produces `"-pdf"` instead of `"resume-pdf"`. The same change is duplicated in `src/utils/files.js`.
Reviews (1): Last reviewed commit: "Merge branch 'develop' into dep-diet" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+52
to
+68
| async function loadPDF() { | ||
| loading.value = true | ||
| const task = PDFJS.getDocument(src.value) | ||
| pdfDoc = await task.promise | ||
| totalPages.value = pdfDoc.numPages | ||
| await renderPage(currentPage.value) | ||
| loading.value = false | ||
| } | ||
|
|
||
| async function renderPage(num) { | ||
| if (!pdfDoc || !canvasRef.value) return | ||
| const page = await pdfDoc.getPage(num) | ||
| const viewport = page.getViewport({ scale: scale.value }) | ||
| const canvas = canvasRef.value | ||
| canvas.height = viewport.height | ||
| canvas.width = viewport.width | ||
| await page.render({ canvasContext: canvas.getContext('2d'), viewport }).promise |
There was a problem hiding this comment.
loadPDF and renderPage have no try/catch. If PDFJS.getDocument(src.value) or page.render(...) rejects (network error, corrupted PDF, permission error), loading.value stays true indefinitely — the spinner never clears and canvasRef stays hidden with no feedback to the user.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/components/FileTypePreview/PDFPreview.vue
Line: 52-68
Comment:
**Unhandled load failure leaves infinite spinner**
`loadPDF` and `renderPage` have no `try/catch`. If `PDFJS.getDocument(src.value)` or `page.render(...)` rejects (network error, corrupted PDF, permission error), `loading.value` stays `true` indefinitely — the spinner never clears and `canvasRef` stays hidden with no feedback to the user.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+23
to
+31
| const formattedDate = new Intl.DateTimeFormat('en-US', { | ||
| month: '2-digit', | ||
| day: '2-digit', | ||
| year: '2-digit', | ||
| }).format(dateObj) | ||
| const formattedTime = new Intl.DateTimeFormat('en-US', { | ||
| hour: '2-digit', | ||
| minute: '2-digit', | ||
| }).format(dateObj) |
There was a problem hiding this comment.
Intl.DateTimeFormat is hardcoded to 'en-US', so all non-US users always see 12-hour AM/PM time. The sibling formatDate in src/ui/drive/js/utils.js already handles this correctly with navigator.language. Consider using undefined (system locale) or referencing the locale-aware version.
Suggested change
| const formattedDate = new Intl.DateTimeFormat('en-US', { | |
| month: '2-digit', | |
| day: '2-digit', | |
| year: '2-digit', | |
| }).format(dateObj) | |
| const formattedTime = new Intl.DateTimeFormat('en-US', { | |
| hour: '2-digit', | |
| minute: '2-digit', | |
| }).format(dateObj) | |
| const locale = navigator.language || 'en-US' | |
| const formattedDate = new Intl.DateTimeFormat(locale, { | |
| month: '2-digit', | |
| day: '2-digit', | |
| year: '2-digit', | |
| }).format(dateObj) | |
| const formattedTime = new Intl.DateTimeFormat(locale, { | |
| hour: '2-digit', | |
| minute: '2-digit', | |
| }).format(dateObj) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/utils/format.js
Line: 23-31
Comment:
`Intl.DateTimeFormat` is hardcoded to `'en-US'`, so all non-US users always see 12-hour AM/PM time. The sibling `formatDate` in `src/ui/drive/js/utils.js` already handles this correctly with `navigator.language`. Consider using `undefined` (system locale) or referencing the locale-aware version.
```suggestion
const locale = navigator.language || 'en-US'
const formattedDate = new Intl.DateTimeFormat(locale, {
month: '2-digit',
day: '2-digit',
year: '2-digit',
}).format(dateObj)
const formattedTime = new Intl.DateTimeFormat(locale, {
hour: '2-digit',
minute: '2-digit',
}).format(dateObj)
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
46
to
54
| function slugger(file_name) { | ||
| return slugify(file_name.split('.').join(' '), { | ||
| lower: true, | ||
| trim: true, | ||
| remove: /[^\w\s\']|_/, | ||
| }) | ||
| return file_name | ||
| .split('.') | ||
| .join(' ') | ||
| .toLowerCase() | ||
| .trim() | ||
| .replace(/[^\w\s']|_/g, '') | ||
| .replace(/\s+/g, '-') | ||
| } |
There was a problem hiding this comment.
JavaScript's \w matches only [a-zA-Z0-9_] (ASCII), so the .replace(/[^\w\s']|_/g, '') step strips accented or CJK characters entirely rather than transliterating them as slugify did. A file named "Résumé.pdf" now produces "-pdf" instead of "resume-pdf". The same change is duplicated in src/utils/files.js.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/ui/drive/js/utils.js
Line: 46-54
Comment:
**Non-ASCII characters silently dropped by native slugger**
JavaScript's `\w` matches only `[a-zA-Z0-9_]` (ASCII), so the `.replace(/[^\w\s']|_/g, '')` step strips accented or CJK characters entirely rather than transliterating them as `slugify` did. A file named `"Résumé.pdf"` now produces `"-pdf"` instead of `"resume-pdf"`. The same change is duplicated in `src/utils/files.js`.
How can I resolve this? If you propose a fix, please make it concise.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
No description provided.