Conversation
- xlsxのZIPからdrawing XMLを直接パースし、xdr:sp/xdr:cxnSpから シェイプ情報（アンカー位置・線スタイル・色・フリップ）を抽出 - CSS transform: scale()方式のSVGオーバーレイで描画オブジェクトを テーブル上に正確に配置（ai-zyusetuと同じアプローチ） - セル斜線（border.diagonal）をSVGで描画 - jszip依存を追加（xlsx ZIP展開用）
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 26 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe PR introduces support for rendering Excel drawing objects (lines/rectangles) and cell diagonal borders in the spreadsheet viewer. It adds XML parsing logic to extract drawing metadata from XLSX files, extends data models with shape and diagonal border information, and implements CSS-transform-based SVG overlay rendering for accurate positioning. Changes
Sequence DiagramsequenceDiagram
participant UI as SpreadsheetViewer
participant Parser as parseWorkbook()
participant ZIP as jszip
participant Render as DiagonalOverlay<br/>& ShapeOverlay
UI->>Parser: Load XLSX file (bytes)
Parser->>ZIP: Extract drawing*.xml<br/>from ZIP buffer
ZIP-->>Parser: Drawing XML content
Parser->>Parser: Parse twoCellAnchor<br/>shapes & diagonals
Parser-->>UI: Shapes[], DiagonalBorder[]
UI->>UI: useEffect: Measure<br/>DOM row Y positions
UI->>Render: Render cell diagonals<br/>via DiagonalOverlay SVG
UI->>Render: Render shapes overlay<br/>with correct anchors
Render-->>UI: SVG overlays positioned
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/SpreadsheetViewer/SpreadsheetViewer.tsx (3)
65-110: Consider extractingDiagonalOverlayto its own file.Per coding guidelines, components should be one-per-file with a dedicated folder structure. Since
DiagonalOverlayis small and tightly coupled to this viewer, this is optional but would improve consistency.Additionally, the SVG lacks accessibility attributes. Since this is decorative, adding
aria-hidden="true"would be appropriate.♻️ Proposed accessibility fix
return ( <svg + aria-hidden="true" style={{ position: "absolute",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/SpreadsheetViewer/SpreadsheetViewer.tsx` around lines 65 - 110, Extract the DiagonalOverlay component into its own file and export it so the parent SpreadsheetViewer imports it (move the function DiagonalOverlay and its prop type for diagonal/ParsedCell into a new component file and update the import in SpreadsheetViewer), and update the SVG element to include aria-hidden="true" to mark it decorative; ensure the new module preserves the same prop signature and vectorEffect/stroke handling so behavior is unchanged.
468-480: Consider usingrow.excelRowfor more stable cell keys.The current key uses
rowIdx(array index) which could cause issues if rows are filtered or reordered in the future. Using the Excel row number would be more stable.♻️ Proposed fix
- key={`${rowIdx + 1}-${getColumnLabel(colIdx)}`} + key={`${row.excelRow}-${getColumnLabel(colIdx)}`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/SpreadsheetViewer/SpreadsheetViewer.tsx` around lines 468 - 480, The cell keys are unstable because they use the array index rowIdx; update the key for the <td> to use a stable identifier like row.excelRow combined with the column label (replace `${rowIdx + 1}-${getColumnLabel(colIdx)}` with something like `${row.excelRow}-${getColumnLabel(colIdx)}`) so keys remain stable when rows are filtered/reordered; locate the <td> rendering in SpreadsheetViewer.tsx (around the <CellContent /> and <DiagonalOverlay /> usage) and change only the key generation to use row.excelRow while keeping colSpan, rowSpan, CellContent, and DiagonalOverlay intact.
262-274: Add accessibility attribute to shape overlay SVG.The shape overlay SVG is decorative and should be hidden from assistive technologies.
♻️ Proposed fix
return ( <svg + aria-hidden="true" style={{ position: "absolute",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/SpreadsheetViewer/SpreadsheetViewer.tsx` around lines 262 - 274, The decorative shape overlay SVG in SpreadsheetViewer should be hidden from assistive tech; update the SVG returned in the render (the <svg> using naturalTableWidth and totalHeight) to include accessibility attributes such as aria-hidden="true" and focusable="false" (and optionally role="presentation") so screen readers ignore it and it cannot receive keyboard focus.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/SpreadsheetViewer/parseWorkbook.ts (1)
248-260: Consider adding error handling for XML parsing.The
DOMParser.parseFromStringcalls at lines 271 and 291 may produce an error document if the XML is malformed. While rare in valid xlsx files, adding a check would improve robustness.♻️ Proposed fix
+function parseXmlSafe(parser: DOMParser, xml: string): Document | null { + const doc = parser.parseFromString(xml, "application/xml"); + const parseError = doc.querySelector("parsererror"); + if (parseError) return null; + return doc; +} + async function parseDrawingsFromZip( zipBuffer: ArrayBuffer, ): Promise<Map<number, RenderShape[]>> {Then use
parseXmlSafeand skip processing if it returns null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/SpreadsheetViewer/parseWorkbook.ts` around lines 248 - 260, In parseDrawingsFromZip, add robust XML parse error handling around the DOMParser.parseFromString calls used to parse drawing and drawingRel XML: replace direct parseFromString usage with a safe helper (e.g., parseXmlSafe) that returns null on parse errors or detects a parsererror node, and if it returns null skip processing that drawing entry (do not throw); update all places in parseDrawingsFromZip where drawing or drawingRel documents are assumed valid (reference parseDrawingsFromZip, the DOMParser usage, and drawing/drawingRel processing) so malformed XML is safely ignored and the function continues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/SpreadsheetViewer/SpreadsheetViewer.tsx`:
- Around line 433-435: The displayed row number currently uses rowIdx + 1 but
the data-row-num attribute uses row.excelRow, causing mismatch when rows are
hidden or the sheet doesn't start at 1; in the SpreadsheetViewer.tsx change the
cell content inside the <td> that currently renders rowIdx + 1 to render
row.excelRow (with a fallback to rowIdx + 1 when row.excelRow is undefined or
null) so the visual row number matches the actual Excel row number while
preserving a safe fallback.
In `@README.md`:
- Line 47: The README entry for "Excel 描画オブジェクト・斜線表示" references PR `#14` but
the PR is actually `#16`; update the PR link text and URL fragment from `#14` to
`#16` in the README line that contains the string "Excel 描画オブジェクト・斜線表示" (replace
the `[ `#14` ](https://github.com/MocA-Love/superset/pull/14)` fragment with
`[`#16`](https://github.com/MocA-Love/superset/pull/16)`).
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/SpreadsheetViewer/parseWorkbook.ts`:
- Around line 248-260: In parseDrawingsFromZip, add robust XML parse error
handling around the DOMParser.parseFromString calls used to parse drawing and
drawingRel XML: replace direct parseFromString usage with a safe helper (e.g.,
parseXmlSafe) that returns null on parse errors or detects a parsererror node,
and if it returns null skip processing that drawing entry (do not throw); update
all places in parseDrawingsFromZip where drawing or drawingRel documents are
assumed valid (reference parseDrawingsFromZip, the DOMParser usage, and
drawing/drawingRel processing) so malformed XML is safely ignored and the
function continues.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/SpreadsheetViewer/SpreadsheetViewer.tsx`:
- Around line 65-110: Extract the DiagonalOverlay component into its own file
and export it so the parent SpreadsheetViewer imports it (move the function
DiagonalOverlay and its prop type for diagonal/ParsedCell into a new component
file and update the import in SpreadsheetViewer), and update the SVG element to
include aria-hidden="true" to mark it decorative; ensure the new module
preserves the same prop signature and vectorEffect/stroke handling so behavior
is unchanged.
- Around line 468-480: The cell keys are unstable because they use the array
index rowIdx; update the key for the <td> to use a stable identifier like
row.excelRow combined with the column label (replace `${rowIdx +
1}-${getColumnLabel(colIdx)}` with something like
`${row.excelRow}-${getColumnLabel(colIdx)}`) so keys remain stable when rows are
filtered/reordered; locate the <td> rendering in SpreadsheetViewer.tsx (around
the <CellContent /> and <DiagonalOverlay /> usage) and change only the key
generation to use row.excelRow while keeping colSpan, rowSpan, CellContent, and
DiagonalOverlay intact.
- Around line 262-274: The decorative shape overlay SVG in SpreadsheetViewer
should be hidden from assistive tech; update the SVG returned in the render (the
<svg> using naturalTableWidth and totalHeight) to include accessibility
attributes such as aria-hidden="true" and focusable="false" (and optionally
role="presentation") so screen readers ignore it and it cannot receive keyboard
focus.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5062efa-2ec9-48cc-a6ac-2530b9e79ecb
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
README.mdapps/desktop/package.jsonapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/SpreadsheetViewer/SpreadsheetViewer.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/SpreadsheetViewer/parseWorkbook.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/SpreadsheetViewer/useSpreadsheetData.ts
| > | ||
| <CellContent cell={cell} /> | ||
| {rowIdx + 1} | ||
| </td> |
There was a problem hiding this comment.
Row number display may not match actual Excel row numbers.
Line 434 displays rowIdx + 1 (sequential index) while data-row-num uses row.excelRow (actual Excel row). If there are hidden rows or the sheet doesn't start at row 1, the displayed row numbers won't match Excel's row numbers.
🐛 Proposed fix
>
- {rowIdx + 1}
+ {row.excelRow}
</td>📝 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.
| > | |
| <CellContent cell={cell} /> | |
| {rowIdx + 1} | |
| </td> | |
| > | |
| {row.excelRow} | |
| </td> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/SpreadsheetViewer/SpreadsheetViewer.tsx`
around lines 433 - 435, The displayed row number currently uses rowIdx + 1 but
the data-row-num attribute uses row.excelRow, causing mismatch when rows are
hidden or the sheet doesn't start at 1; in the SpreadsheetViewer.tsx change the
cell content inside the <td> that currently renders rowIdx + 1 to render
row.excelRow (with a fallback to rowIdx + 1 when row.excelRow is undefined or
null) so the visual row number matches the actual Excel row number while
preserving a safe fallback.
1 participant
Summary
transform: scale()方式の SVG オーバーレイで、画面サイズに依存しない正確な配置Changes
parseDrawingsFromZip）、セル斜線パーサー追加（getCellDiagonal）、RenderShape/RenderAnchor/DiagonalBorder型追加、ParsedRow.excelRow/ParsedSheet.minCol/shapesフィールド追加DiagonalOverlayコンポーネント追加、列幅スケーリングを CSS transform に統一Technical Details
xl/drawings/drawing*.xml内のxdr:twoCellAnchor→xdr:sp/xdr:cxnSpをパースして描画情報を抽出transform: scale()でコンテナに収める。これによりテーブルと SVG が完全に同じ座標空間を共有し、EMU オフセットを直接加算可能Test plan
Summary by CodeRabbit
Release Notes