fix: NaN rows in dataframe output renderer

[saltenasl]

Open Snowflake-in-Deepnote.deepnote.txt(rm the .txt, github cached this and won't let me re-upload) - the dataframe outputs for the blocks shouldn't say NaN rows (e.g. the one that starts with SELECT row_number() over (ORDER)

Summary by CodeRabbit

• Bug Fixes
  • Preview row count is now optional: when a preview value is provided it’s used; otherwise the full row count is shown.
  • Improved handling of invalid preview values (e.g., NaN or non-finite values) so they no longer disrupt selection, resulting in more consistent and predictable dataframe previews.

[linear]

GRN-5007 The number of rows are not displayed correctly in the data frame output

[coderabbitai]

📝 Walkthrough

Walkthrough

The DataframeRenderer prop type and the DataFrameObject interface were changed so preview_row_count is optional (number | undefined). Runtime logic now uses Number.isFinite(data.preview_row_count) to choose preview_row_count when it is a finite number; otherwise it falls back to data.row_count. This changes how numberOfRows is computed and how undefined or NaN preview values are handled.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant UI as DataframeRenderer
    participant Data as Input Data
    participant Calc as RowCountCalc
    participant Render as Renderer

    Note right of UI #DDEBF7: New flow for determining row count
    UI->>Data: receive data (row_count, preview_row_count?)
    UI->>Calc: compute numberOfRows
    alt preview_row_count defined and finite
        Calc-->>UI: return preview_row_count
    else preview_row_count undefined or non-finite
        Calc-->>UI: return row_count
    end
    UI->>Render: render numberOfRows rows
    Render-->>UI: rendered output

Loading

sequenceDiagram
    autonumber
    participant OldUI as Old DataframeRenderer
    participant Data as Input Data
    participant CalcOld as OldCalc
    participant Render as Renderer

    Note right of OldUI #FBE4D6: Previous flow used min(row_count, preview_row_count)
    OldUI->>Data: receive data (row_count, preview_row_count)
    OldUI->>CalcOld: compute numberOfRows = min(row_count, preview_row_count)
    CalcOld-->>OldUI: return numberOfRows
    OldUI->>Render: render numberOfRows rows
    Render-->>OldUI: rendered output

Loading

Suggested reviewers

• Artmann
• jamesbhobbs
• andyjakubowski

Pre-merge checks

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title "fix: NaN rows in dataframe output renderer" directly describes the problem being addressed by the PR. The changes make preview_row_count optional across both the DataframeRenderer component and DataFrameObject interface, then implement conditional logic to use preview_row_count when finite or fall back to row_count otherwise. This fallback mechanism prevents NaN values from appearing in the output, making the title fully aligned with the core fix.

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 9659fac and a08be2e.

📒 Files selected for processing (2)

• src/webviews/extension-side/dataframe/dataframeController.ts (1 hunks)
• src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (2 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation with CancellationToken

Files:

• src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
• src/webviews/extension-side/dataframe/dataframeController.ts

**/!(*.node|*.web).ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place shared cross-platform logic in common .ts files (not suffixed with .node or .web)

Files:

• src/webviews/extension-side/dataframe/dataframeController.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Build & Test

🔇 Additional comments (3)

src/webviews/extension-side/dataframe/dataframeController.ts (1)

50-50: LGTM! Type consistency maintained.

Making preview_row_count optional aligns with the webview-side interface and correctly handles cases where the value is unavailable or invalid.

src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (2)

41-41: Type definition correctly updated.

Optional preview_row_count matches the extension-side interface.

---

78-78: Correctly handles all edge cases.

Number.isFinite() properly guards against NaN, undefined, null, and Infinity while treating 0 as valid. Addresses all past review concerns.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73%. Comparing base (2cf42fd) to head (a08be2e).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #156   +/-   ##
=====================================
  Coverage     73%     73%           
=====================================
  Files        550     550           
  Lines      44004   44004           
  Branches    5312    5312           
=====================================
  Hits       32189   32189           
  Misses     10048   10048           
  Partials    1767    1767           

Files with missing lines	Coverage Δ
...ws/extension-side/dataframe/dataframeController.ts	60% <ø> (ø)

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (2)

1-10: Missing Microsoft copyright header.

Coding guidelines require the Microsoft copyright header in source files.

As per coding guidelines.

---

65-69: Replace console.log with ILogger service.

Multiple console.log calls throughout (lines 65, 88, 96, 104, 112, 118, 129) violate coding guidelines.

As per coding guidelines.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 2cf42fd and 5bd05bc.

📒 Files selected for processing (1)

• src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation with CancellationToken

Files:

• src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx

🔇 Additional comments (1)

src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (1)

81-81: Verify pagination uses correct row count.

totalPages uses data.row_count while display shows numberOfRows. Confirm this is intentional.

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 5bd05bc and 9659fac.

📒 Files selected for processing (1)

• src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation with CancellationToken

Files:

• src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx

[saltenasl]

@coderabbitai i addressed your comments, can you check now?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch all review comments from coderabbitai on PR #156
gh pr view 156 --json reviews,comments --jq '.reviews[] | select(.author.login == "coderabbitai") | {state: .state, body: .body, submittedAt: .submittedAt}'

# Also fetch review thread comments
gh api /repos/{owner}/{repo}/pulls/156/comments --jq '.[] | select(.user.login == "coderabbitai") | {path: .path, body: .body, line: .line, created_at: .created_at}'

Length of output: 5672

---

🏁 Script executed:

#!/bin/bash
# Get the current content of the modified files
cat src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx

Length of output: 11215

---

🏁 Script executed:

#!/bin/bash
cat src/webviews/extension-side/dataframe/dataframeController.ts

Length of output: 12342

---

@saltenasl The core NaN fix looks good—making preview_row_count optional and using Number.isFinite() should resolve the issue.

However, three items from my previous review remain unaddressed:

1. Missing Microsoft copyright header at the top of DataframeRenderer.tsx
2. console.log calls throughout the file (lines 65, 88, 96, 104, 112, 118, 129) should use the ILogger service instead, per coding guidelines
3. Pagination discrepancy: totalPages (line 81) uses data.row_count, but the display shows numberOfRows which may differ when preview_row_count is used

Would you like to address these, or should we defer them?

[Artmann]

Can you add a test to reproduce? 🙏