(Feature) Add drag-and-drop arrangement for JPG to PDF conversion#323
(Feature) Add drag-and-drop arrangement for JPG to PDF conversion#323AB527 wants to merge 3 commits intoalam00000:mainfrom
Conversation
|
Closing as stale/conflicting. If this idea still matters, please reopen it as a fresh PR rebased on current main. |
📝 WalkthroughWalkthroughAdded drag-and-drop reordering to the JPG→PDF page by rendering per-file drag handles, creating/destroying a Sortable instance for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as JPG→PDF UI
participant Sortable
participant State as files[]
Note over UI,Sortable: Initial setup binds Sortable to `#file-display-area`
User->>UI: Drag file row (via .drag-handle)
UI->>Sortable: drag event
Sortable->>UI: onEnd(oldIndex,newIndex)
UI->>State: splice files array (move oldIndex→newIndex)
UI->>UI: updateUI() re-renders rows and indices
UI->>Sortable: re-initialize / maintain instance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
|
Hi, I’ve resolved the conflicts and rebased the PR on the latest main. Since the changes are small, there’s no need to open another PR. Would really appreciate it if you could merge it soon to avoid any further conflicts. Thanks! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/js/logic/jpg-to-pdf-page.ts (2)
80-86: Inconsistent indentation.Lines 80-86 use 4-space indentation while the rest of the file uses 2-space indentation. This should be corrected for consistency.
🔧 Suggested fix
- document.getElementById('back-to-tools')?.addEventListener('click', () => { - window.location.href = import.meta.env.BASE_URL; - }); - - setTimeout(() => { - initializeFileListSortable(); - }, 0); + document.getElementById('back-to-tools')?.addEventListener('click', () => { + window.location.href = import.meta.env.BASE_URL; + }); + + setTimeout(() => { + initializeFileListSortable(); + }, 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/logic/jpg-to-pdf-page.ts` around lines 80 - 86, The block adding the click handler to document.getElementById('back-to-tools') and the setTimeout call that invokes initializeFileListSortable() uses 4-space indentation while the rest of src/js/logic/jpg-to-pdf-page.ts uses 2-space indentation; update the indentation of that block to 2 spaces to match the file style (ensure the lines with the click listener and the setTimeout/initializeFileListSortable() call use 2-space indents).
5-5: Unused import and type suppression.The
loadPyMuPDFimport on line 5 is no longer used since the code now instantiatesPyMuPDFdirectly. Additionally, consider installing@types/sortablejsto remove the@ts-ignoreand gain type safety.♻️ Suggested cleanup
-import { loadPyMuPDF } from '../utils/pymupdf-loader.js'; +import { PyMuPDF } from '../utils/pymupdf-loader.js';Or if you prefer to keep using the loader utility:
- pymupdf = new PyMuPDF(import.meta.env.BASE_URL + 'pymupdf-wasm/'); - await pymupdf.load(); + pymupdf = await loadPyMuPDF();Also applies to: 11-13
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/logic/jpg-to-pdf-page.ts` at line 5, Remove the unused import loadPyMuPDF from the top of the file and delete the corresponding unused references (since you now instantiate PyMuPDF directly via the PyMuPDF symbol); also remove the `@ts-ignore` used around Sortable usage and either install `@types/sortablejs` or add the correct Sortable import/type annotations so the code has proper type safety instead of suppressing errors; alternatively, if you prefer the loader pattern, revert instantiation to use loadPyMuPDF and ensure PyMuPDF is obtained from that loader.
🤖 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/js/logic/jpg-to-pdf-page.ts`:
- Around line 209-215: The function ensurePyMuPDF references the PyMuPDF class
directly (new PyMuPDF(...) and Promise<PyMuPDF>) but PyMuPDF isn't
imported/exported and is loaded via loadPyMuPDF(), causing a runtime
ReferenceError; change ensurePyMuPDF to call the existing loadPyMuPDF() helper
to obtain and cache the instance (use the existing pymupdf variable), remove
direct uses of new PyMuPDF, and update the return type to match loadPyMuPDF's
result (or use a generic/any type) so the function returns the dynamically
loaded instance from loadPyMuPDF instead of trying to construct PyMuPDF
directly.
---
Nitpick comments:
In `@src/js/logic/jpg-to-pdf-page.ts`:
- Around line 80-86: The block adding the click handler to
document.getElementById('back-to-tools') and the setTimeout call that invokes
initializeFileListSortable() uses 4-space indentation while the rest of
src/js/logic/jpg-to-pdf-page.ts uses 2-space indentation; update the indentation
of that block to 2 spaces to match the file style (ensure the lines with the
click listener and the setTimeout/initializeFileListSortable() call use 2-space
indents).
- Line 5: Remove the unused import loadPyMuPDF from the top of the file and
delete the corresponding unused references (since you now instantiate PyMuPDF
directly via the PyMuPDF symbol); also remove the `@ts-ignore` used around
Sortable usage and either install `@types/sortablejs` or add the correct
Sortable import/type annotations so the code has proper type safety instead of
suppressing errors; alternatively, if you prefer the loader pattern, revert
instantiation to use loadPyMuPDF and ensure PyMuPDF is obtained from that
loader.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fc79e57-8ee7-4a11-a798-d96445e7029c
📒 Files selected for processing (1)
src/js/logic/jpg-to-pdf-page.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/js/logic/jpg-to-pdf-page.ts (2)
83-85: Avoid duplicate sortable initialization paths.Line 83-Line 85 initializes sortable via
setTimeout, and Line 181 initializes again after rendering. Keeping a single init path (post-render inupdateUI) will reduce create/destroy churn and simplify lifecycle behavior.Also applies to: 181-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/logic/jpg-to-pdf-page.ts` around lines 83 - 85, There are two initialization paths for the sortable: a delayed call via setTimeout that invokes initializeFileListSortable() and a second initialization after rendering inside updateUI; remove the setTimeout-based initialization (the anonymous function calling initializeFileListSortable) so sortable is only created once from updateUI after render, and ensure updateUI is responsible for creating (and if needed destroying/recreating) the sortable to avoid duplicate create/destroy churn.
182-185: Destroy Sortable when list becomes empty.When
files.length === 0, the UI is hidden but the previous sortable instance remains attached until a later re-init. Explicitly destroy and clear_sortableInstancein the empty-state branch to avoid stale listeners.Proposed patch
} else { + const sortable = (fileDisplayArea as any)._sortableInstance; + if (sortable) { + sortable.destroy(); + (fileDisplayArea as any)._sortableInstance = null; + } fileControls.classList.add('hidden'); optionsDiv.classList.add('hidden'); }Also applies to: 191-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/logic/jpg-to-pdf-page.ts` around lines 182 - 185, The empty-state branch that hides fileControls and optionsDiv must also destroy and clear the previous Sortable instance to remove stale listeners: when handling files.length === 0 (the else branch that toggles fileControls.classList.add('hidden') and optionsDiv.classList.add('hidden')), call _sortableInstance.destroy() if _sortableInstance exists and then set _sortableInstance = null; make the same change in the other empty-state branch around the lines referencing the same UI hiding logic (the second occurrence noted at 191-193) so both branches clean up the Sortable instance.
🤖 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/js/logic/jpg-to-pdf-page.ts`:
- Around line 163-166: The drag handle is currently a non-focusable div
(dragHandle) which prevents keyboard users from reordering; replace the div with
a semantic focusable control (create a button element with type="button") or at
minimum add tabindex="0", role="button", and keydown handlers, and give it an
accessible label (e.g., aria-label="Move page" or visually hidden text). Update
the element creation where dragHandle is constructed so it uses button (or adds
tabindex/role and implements Enter/Space keyboard activation that triggers the
same drag/reorder logic), keep the same className/styling, and ensure event
listeners that start dragging reference this button element.
- Around line 10-11: Remove the unnecessary TypeScript suppression before the
Sortable import: delete the "// `@ts-ignore`" that precedes "import Sortable from
'sortablejs';" in jpg-to-pdf-page.ts so TypeScript uses the installed
`@types/sortablejs` and esModuleInterop handling; keep the import as-is (Sortable)
and run type-check to ensure no errors, removing any other redundant `@ts-ignore`
around the same import if present.
---
Nitpick comments:
In `@src/js/logic/jpg-to-pdf-page.ts`:
- Around line 83-85: There are two initialization paths for the sortable: a
delayed call via setTimeout that invokes initializeFileListSortable() and a
second initialization after rendering inside updateUI; remove the
setTimeout-based initialization (the anonymous function calling
initializeFileListSortable) so sortable is only created once from updateUI after
render, and ensure updateUI is responsible for creating (and if needed
destroying/recreating) the sortable to avoid duplicate create/destroy churn.
- Around line 182-185: The empty-state branch that hides fileControls and
optionsDiv must also destroy and clear the previous Sortable instance to remove
stale listeners: when handling files.length === 0 (the else branch that toggles
fileControls.classList.add('hidden') and optionsDiv.classList.add('hidden')),
call _sortableInstance.destroy() if _sortableInstance exists and then set
_sortableInstance = null; make the same change in the other empty-state branch
around the lines referencing the same UI hiding logic (the second occurrence
noted at 191-193) so both branches clean up the Sortable instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9662df4e-966e-41c3-bbef-f77365785c46
📒 Files selected for processing (1)
src/js/logic/jpg-to-pdf-page.ts
| const dragHandle = document.createElement('div'); | ||
| dragHandle.className = | ||
| 'drag-handle cursor-move text-gray-400 hover:text-white p-1 rounded transition-colors'; | ||
| dragHandle.innerHTML = `<svg width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2"><circle cx="9" cy="5" r="1"/><circle cx="15" cy="5" r="1"/><circle cx="9" cy="12" r="1"/><circle cx="15" cy="12" r="1"/><circle cx="9" cy="19" r="1"/><circle cx="15" cy="19" r="1"/></svg>`; |
There was a problem hiding this comment.
Make drag handle keyboard-accessible.
The reorder control is a non-focusable div (Line 163), so keyboard-only users cannot access reordering. Use a semantic button (or add tabindex, role, and keyboard handlers) plus an accessible label.
🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 165-165: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: dragHandle.innerHTML = <svg width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2"><circle cx="9" cy="5" r="1"/><circle cx="15" cy="5" r="1"/><circle cx="9" cy="12" r="1"/><circle cx="15" cy="12" r="1"/><circle cx="9" cy="19" r="1"/><circle cx="15" cy="19" r="1"/></svg>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 165-165: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: dragHandle.innerHTML = <svg width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2"><circle cx="9" cy="5" r="1"/><circle cx="15" cy="5" r="1"/><circle cx="9" cy="12" r="1"/><circle cx="15" cy="12" r="1"/><circle cx="9" cy="19" r="1"/><circle cx="15" cy="19" r="1"/></svg>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/logic/jpg-to-pdf-page.ts` around lines 163 - 166, The drag handle is
currently a non-focusable div (dragHandle) which prevents keyboard users from
reordering; replace the div with a semantic focusable control (create a button
element with type="button") or at minimum add tabindex="0", role="button", and
keydown handlers, and give it an accessible label (e.g., aria-label="Move page"
or visually hidden text). Update the element creation where dragHandle is
constructed so it uses button (or adds tabindex/role and implements Enter/Space
keyboard activation that triggers the same drag/reorder logic), keep the same
className/styling, and ensure event listeners that start dragging reference this
button element.
|
Hi @MrFreePress, just wanted to check on the status of this PR. If possible, it would be great to get it merged soon since it’s a relatively simple feature. Also, please let me know if any changes are required from my side |
2 participants
Description
This change adds a drag-and-drop image reordering interface to the JPG-to-PDF conversion flow. It enables users to rearrange uploaded JPG images before generating the final PDF, ensuring correct page order.
Previously, the output PDF page order depended on upload sequence or file naming, which was inconvenient and error-prone when handling multiple images. This feature brings parity with the existing Merge PDFs workflow by reusing the same UI/UX pattern.
No external dependencies were introduced.
Fixes #320
Type of change
🧪 How Has This Been Tested?
The changes were tested manually in a local development environment by uploading multiple JPG images, rearranging them using drag-and-drop, and generating PDFs multiple times to verify that the final page order matches the arranged order.
Checklist:
Expected Results:
Actual Results:
Checklist:
Summary by CodeRabbit