Table extraction rework with RTL support (v1.4.0) - #9
Conversation
- Page-wide column detection via coverage histogram: empty cells stay aligned, title/total lines no longer break detection - Full RTL support: auto-detected page direction, RTL column order, reading-order joins for mixed Hebrew/English cells, RTL Excel sheet view and RTL preview layout - Merge overlaid fragments (decimal points) back into their numbers - Non-destructive cleanup options (re-derive from raw extraction) - Excel autofilter on header row when Include Headers is checked - Formula-injection guard scoped to CSV only; xlsx string cells are never evaluated as formulas, so cell text stays unmangled - Bilingual status messages, page count, and column labels - Turnstile verification once per session instead of once per file - Fix filename extension stripping, empty-MIME drops, CSV CR quoting, PDF memory release, spread-based stack overflow risk - Service worker precaches all local assets for working offline mode - Bump cache and app version to 1.4.0, update changelog
📝 WalkthroughWalkthroughVersion 1.4.0 adds RTL-aware PDF extraction, bilingual status messaging, updated Excel/CSV export behavior, improved PDF upload lifecycle handling, default RTL support, and expanded service-worker precaching. ChangesRTL extraction and export release
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PDFFile
participant PDFjs
participant extractDataFromPDF
participant PreviewAndExport
PDFFile->>PDFjs: Load PDF pages
PDFjs->>extractDataFromPDF: Provide page text content
extractDataFromPDF->>extractDataFromPDF: Reconstruct RTL text and tables
extractDataFromPDF->>PreviewAndExport: Provide cleaned extracted data
PreviewAndExport->>PreviewAndExport: Render preview and generate exports
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Deploying easyconvert-website with
|
| Latest commit: |
143a94a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://daa4c791.easyconvert-website.pages.dev |
| Branch Preview URL: | https://feat-converter-quality-rtl.easyconvert-website.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
public/app.js (1)
733-734: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmpty results leave the previous preview on screen.
updatePreview()bails before clearing whenextractedDatais empty, so if a cleanup-option toggle (e.g. Skip Empty Rows) reduces the data to zero rows, the stale table from the previous state remains visible and downloads stay enabled.🐛 Clear instead of returning early
function updatePreview() { - if (extractedData.length === 0) return; + if (extractedData.length === 0) { + document.getElementById('previewHeader').innerHTML = ''; + document.getElementById('previewBody').innerHTML = ''; + previewSection.style.display = 'none'; + return; + }🤖 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 `@public/app.js` around lines 733 - 734, Update updatePreview() so it clears the existing preview and disables download actions before handling an empty extractedData array, then return without rendering rows. Preserve the current rendering behavior for non-empty data.
🧹 Nitpick comments (5)
public/app.js (5)
269-272: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueSpread-based
pushcontradicts the anti-spread rationale used elsewhere.Lines 696 and 826 deliberately avoid
Math.max(...arr)because spreading large arrays overflows the argument stack;push(...pageData)has the same limit. A loop or a running array concat avoids it, and also lets you release page resources.♻️ Proposed change
if (pageData.length > 0) { - rawExtractedData.push(...pageData); + for (let r = 0; r < pageData.length; r++) rawExtractedData.push(pageData[r]); } + page.cleanup();🤖 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 `@public/app.js` around lines 269 - 272, Replace the spread-based append in the page-processing flow around rawExtractedData with a stack-safe approach that does not pass pageData as individual arguments, such as iterative insertion or array concatenation. Preserve the existing empty-page guard and ensure page resources can be released as each page is processed.
123-129: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider guarding against overlapping re-extractions.
extractDataFromPDF()is async and unawaited here; toggling the checkbox quickly (or toggling while an extraction is already running) lets two runs interleave writes torawExtractedData/lastExtractionRTL, producing mixed results. A simple in-flight flag (or disabling the option inputs during extraction) removes the hazard.🤖 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 `@public/app.js` around lines 123 - 129, Guard the rtlSupport change handler around extractDataFromPDF so a new extraction cannot start while one is already in flight. Add and manage an in-flight flag (or disable the relevant option inputs) across the async extraction lifecycle, while preserving updatePreview() behavior when no PDF is loaded and ensuring the guard is cleared after completion.
486-493: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThreshold is stricter than the comment claims for small tables.
coverage[b] > Math.max(1, Math.floor(n * 0.34))requires ≥2 covering rows whenevern <= 5, i.e. 40-100% coverage rather than "more than ~a third". A 2-3 row table needs near-universal coverage or column detection silently falls back to one-cell-per-item. Either relax to>=with a proportional threshold or update the comment to match.🤖 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 `@public/app.js` around lines 486 - 493, The threshold in the column-detection loop is stricter than the stated “above ~a third” behavior for small multiRows collections. Update the threshold calculation and isColumn comparison near columns so coverage representing more than roughly one-third of rows is accepted, including the intended small-table boundary cases, while preserving the existing column grouping and gap logic.
340-360: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winShort numeric fragments can be folded into a neighbouring host.
The only guards are
text.length <= 2and horizontal containment (with +1 slack), so a legitimate standalone cell like12or%that happens to sit inside an over-wide neighbour's bounds is silently absorbed and its own cell disappears. Since this only ever replaces a space, it's plausible to also require that the fragment be non-alphanumeric punctuation (.,,,:,-) which is the documented motivating case.♻️ Narrow the trigger
- if (item.text && item.text.length <= 2) { + if (item.text && item.text.length <= 2 && !/[A-Za-z0-9\u0590-\u05FF\u0600-\u06FF]/.test(item.text)) {🤖 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 `@public/app.js` around lines 340 - 360, In the sorted item folding logic, narrow the trigger guarded by the item.text length check so only documented non-alphanumeric punctuation fragments (such as ., ,, :, or -) can be merged into a host. Preserve the existing host containment, space-selection, and replacement behavior while preventing numeric or other standalone alphanumeric cells from being absorbed.
297-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
\uXXXXescapes for these character classes.Raw RTL characters inside a regex literal are hard to review and easy for editors/encoding conversions to mangle (the class boundaries are literally invisible here). Line 777 already uses escapes for the same purpose, so unifying on escapes also removes the duplicate definition. Note the current upper range also swallows U+FEFF (BOM/ZWNBSP), which would count as an RTL character.
♻️ Example
-const RTL_CHARS = /[-߿ࡠ-ࣿיִ-﷿ﹰ-]/g; +const RTL_CHARS = /[\u0590-\u05FF\u0600-\u06FF\u0700-\u07BF\u0860-\u08FF\uFB1D-\uFDFF\uFE70-\uFEFC]/g; const LTR_CHARS = /[A-Za-z0-9]/g;🤖 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 `@public/app.js` around lines 297 - 298, Update the RTL character-class definition near RTL_CHARS to use explicit Unicode escapes, preserving the intended ranges while excluding U+FEFF. Reuse the existing escaped definition at the later occurrence and remove the duplicate declaration; keep LTR_CHARS unchanged.
🤖 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 `@public/app.js`:
- Around line 1224-1228: Update the parameter substitution loop in the fileLabel
interpolation logic to replace every occurrence of each placeholder and treat
parameter values literally, so dollar sequences such as $&, $', and $1 are
preserved rather than interpreted by String.replace. Use a replacement approach
or callback that avoids replacement-pattern expansion while retaining the
existing behavior for missing or ordinary parameters.
- Around line 254-257: Update the no-pages early-exit branch in the
page-processing function to call hideProgress() before returning, matching the
cleanup performed by the other early exits while preserving the existing error
status.
- Around line 186-199: Update the new-PDF cleanup path around currentPDF
destruction to also clear the previous extractedData, preview, and download
output before assigning the new fileName. Reset the associated extraction
state/UI using the existing state variables and cleanup mechanisms, ensuring
failed extraction or pending verification cannot expose the prior document’s
rows under the new name.
- Around line 393-399: Pass the effective RTL flag from the method-selection
logic to extractStructuredData and extractTextData, just as the table paths use
pageRTL && rtlSupportEnabled(). Update those functions to use the passed flag
when determining pageDirectionRTL and reading order, rather than recomputing RTL
support unconditionally, while preserving existing behavior when RTL support is
enabled.
---
Outside diff comments:
In `@public/app.js`:
- Around line 733-734: Update updatePreview() so it clears the existing preview
and disables download actions before handling an empty extractedData array, then
return without rendering rows. Preserve the current rendering behavior for
non-empty data.
---
Nitpick comments:
In `@public/app.js`:
- Around line 269-272: Replace the spread-based append in the page-processing
flow around rawExtractedData with a stack-safe approach that does not pass
pageData as individual arguments, such as iterative insertion or array
concatenation. Preserve the existing empty-page guard and ensure page resources
can be released as each page is processed.
- Around line 123-129: Guard the rtlSupport change handler around
extractDataFromPDF so a new extraction cannot start while one is already in
flight. Add and manage an in-flight flag (or disable the relevant option inputs)
across the async extraction lifecycle, while preserving updatePreview() behavior
when no PDF is loaded and ensuring the guard is cleared after completion.
- Around line 486-493: The threshold in the column-detection loop is stricter
than the stated “above ~a third” behavior for small multiRows collections.
Update the threshold calculation and isColumn comparison near columns so
coverage representing more than roughly one-third of rows is accepted, including
the intended small-table boundary cases, while preserving the existing column
grouping and gap logic.
- Around line 340-360: In the sorted item folding logic, narrow the trigger
guarded by the item.text length check so only documented non-alphanumeric
punctuation fragments (such as ., ,, :, or -) can be merged into a host.
Preserve the existing host containment, space-selection, and replacement
behavior while preventing numeric or other standalone alphanumeric cells from
being absorbed.
- Around line 297-298: Update the RTL character-class definition near RTL_CHARS
to use explicit Unicode escapes, preserving the intended ranges while excluding
U+FEFF. Reuse the existing escaped definition at the later occurrence and remove
the duplicate declaration; keep LTR_CHARS unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c04b79ad-36b7-4bdb-9e02-e1caf06dd2f9
📒 Files selected for processing (6)
CHANGELOG.mdREADME.mdpackage.jsonpublic/app.jspublic/index.htmlpublic/sw.js
| // Release the previous document before loading a new one | ||
| if (currentPDF) { | ||
| currentPDF.destroy(); | ||
| currentPDF = null; | ||
| } | ||
|
|
||
| fileName = file.name.replace(/\.pdf$/i, ''); | ||
| showFileInfo(file); | ||
|
|
||
| // Already verified this session - skip straight to processing | ||
| if (turnstileVerified) { | ||
| processPDFAfterVerification(file); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=js '\b(enableDownloads|disableDownloads)\b' -C5 public/Repository: NX1X/EasyConvert
Length of output: 1068
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline public/app.js --view expanded 2>/dev/null | sed -n '1,220p' || true
echo "== relevant public/app.js sections =="
sed -n '130,215p' public/app.js
sed -n '250,290p' public/app.js
sed -n '900,940p' public/app.js
echo "== state usages and updatePreview =="
rg -n --type=js '(rawExtractedData|extractedData|lastExtractionRTL|updatePreview|currentPDF|previewSection|downloadExcel|downloadCSV|disableDownloads|enableDownloads)"' public/app.js
rg -n --type=js 'function updatePreview|const previewSection|document.getElementById\(.*preview' public/app.jsRepository: NX1X/EasyConvert
Length of output: 8970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== public/app.js lines 290-330 =="
sed -n '290,330p' public/app.js
echo "== public/app.js lines 370-395 =="
sed -n '370,395p' public/app.js
echo "== public/app.js lines 718-795 =="
sed -n '718,795p' public/app.js
echo "== public/app.js lines 205-260 =="
sed -n '205,260p' public/app.js
echo "== download filename generation =="
sed -n '780,810p' public/app.js
echo "== exact state/preview/disabled usage =="
rg -n --type=js 'rawExtractedData|extractedData|lastExtractionRTL|previewSection|downloadExcel\.disabled|downloadCSV\.disabled|function downloadFile' public/app.jsRepository: NX1X/EasyConvert
Length of output: 9604
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== public/app.js lines 100-125 =="
sed -n '100,125p' public/app.js
echo "== public/app.js lines 818-880 =="
sed -n '818,880p' public/app.jsRepository: NX1X/EasyConvert
Length of output: 4101
Reset extraction state when loading a new PDF.
Cleaning up currentPDF is not enough: enableDownloads() leaves the old extractedData, preview, and download output in place while fileName already reflects the newly selected file. If extraction fails or verification is still pending, users can download the previous document’s rows under the new file name. Reset these state/UI elements during the cleanup path.
🤖 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 `@public/app.js` around lines 186 - 199, Update the new-PDF cleanup path around
currentPDF destruction to also clear the previous extractedData, preview, and
download output before assigning the new fileName. Reset the associated
extraction state/UI using the existing state variables and cleanup mechanisms,
ensuring failed extraction or pending verification cannot expose the prior
document’s rows under the new name.
| if (pagesToProcess.length === 0) { | ||
| showStatus('No valid pages to process.', 'error'); | ||
| showStatus(t('statusNoPages'), 'error'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Progress bar is left visible on the no-pages path.
Every other early exit in this function calls hideProgress(); this one returns at 60%, leaving a stuck progress indicator.
🐛 Proposed fix
if (pagesToProcess.length === 0) {
showStatus(t('statusNoPages'), 'error');
+ hideProgress();
return;
}📝 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.
| if (pagesToProcess.length === 0) { | |
| showStatus('No valid pages to process.', 'error'); | |
| showStatus(t('statusNoPages'), 'error'); | |
| return; | |
| } | |
| if (pagesToProcess.length === 0) { | |
| showStatus(t('statusNoPages'), 'error'); | |
| hideProgress(); | |
| return; | |
| } |
🤖 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 `@public/app.js` around lines 254 - 257, Update the no-pages early-exit branch
in the page-processing function to call hideProgress() before returning,
matching the cleanup performed by the other early exits while preserving the
existing error status.
| if (method === 'table' || method === 'advanced') { | ||
| return extractTableData(textContent, viewport, method === 'advanced'); | ||
| return extractTableData(textContent, viewport, method === 'advanced', pageRTL && rtlSupportEnabled()); | ||
| } else if (method === 'structured') { | ||
| return extractStructuredData(textContent, viewport); | ||
| } else { | ||
| return extractTextData(textContent); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
RTL checkbox is only honored by the table methods.
rtlSupportEnabled() gates the table/advanced paths, but extractStructuredData (line 595) and extractTextData (line 645) recompute pageDirectionRTL and reorder reading order unconditionally. Unchecking "Hebrew/Arabic RTL Support" therefore has no effect in Text/Structured modes, while the preview dir and Excel RTL view (which do check it) disagree with the emitted ordering. Suggest passing the effective flag down from here.
♻️ Thread the effective flag through
- if (method === 'table' || method === 'advanced') {
- return extractTableData(textContent, viewport, method === 'advanced', pageRTL && rtlSupportEnabled());
- } else if (method === 'structured') {
- return extractStructuredData(textContent, viewport);
- } else {
- return extractTextData(textContent);
- }
+ const effectiveRTL = pageRTL && rtlSupportEnabled();
+ if (method === 'table' || method === 'advanced') {
+ return extractTableData(textContent, viewport, method === 'advanced', effectiveRTL);
+ } else if (method === 'structured') {
+ return extractStructuredData(textContent, effectiveRTL);
+ } else {
+ return extractTextData(textContent, effectiveRTL);
+ }🤖 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 `@public/app.js` around lines 393 - 399, Pass the effective RTL flag from the
method-selection logic to extractStructuredData and extractTextData, just as the
table paths use pageRTL && rtlSupportEnabled(). Update those functions to use
the passed flag when determining pageDirectionRTL and reading order, rather than
recomputing RTL support unconditionally, while preserving existing behavior when
RTL support is enabled.
| if (params) { | ||
| Object.keys(params).forEach(function (name) { | ||
| str = str.replace('{' + name + '}', params[name]); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
String.replace with a string replacement mangles $ in params.
fileLabel interpolates a user-supplied file name; if it contains $&, $' or $1, those are treated as replacement patterns and the rendered label is wrong. Also only the first occurrence of each placeholder is substituted.
🐛 Proposed fix
if (params) {
Object.keys(params).forEach(function (name) {
- str = str.replace('{' + name + '}', params[name]);
+ str = str.split('{' + name + '}').join(String(params[name]));
});
}📝 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.
| if (params) { | |
| Object.keys(params).forEach(function (name) { | |
| str = str.replace('{' + name + '}', params[name]); | |
| }); | |
| } | |
| if (params) { | |
| Object.keys(params).forEach(function (name) { | |
| str = str.split('{' + name + '}').join(String(params[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 `@public/app.js` around lines 1224 - 1228, Update the parameter substitution
loop in the fileLabel interpolation logic to replace every occurrence of each
placeholder and treat parameter values literally, so dollar sequences such as
$&, $', and $1 are preserved rather than interpreted by String.replace. Use a
replacement approach or callback that avoids replacement-pattern expansion while
retaining the existing behavior for missing or ordinary parameters.
1 participant
Summary
Rework of the PDF table extraction engine plus a set of correctness and UX fixes. Bumps the app to 1.4.0.
Extraction quality
RTL support
Fixes
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Chores