Skip to content

Conversation

@emir-karabeg
Copy link
Collaborator

Summary

Fixes table not coming up when no rows already defined.

Type of Change

  • Bug fix

Testing

How has this been tested? What should reviewers focus on?

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link

vercel bot commented Nov 12, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
docs Skipped Skipped Nov 12, 2025 5:05pm

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 12, 2025

Greptile Overview

Greptile Summary

Fixed table component initialization by adding a useEffect hook that creates an initial empty row when no rows are defined. Improved row validation by introducing a memoized emptyCellsTemplate and refactoring validation logic to avoid in-place mutations.

Key Changes:

  • Added useEffect hook to initialize table with empty row when storeValue is missing or empty
  • Introduced memoized emptyCellsTemplate based on columns prop for consistent empty cell structure
  • Refactored row validation in rows useMemo to create new objects instead of mutating existing ones
  • Updated updateCellValue and renderCell to use emptyCellsTemplate for defensive programming

Issues Found:

  • Missing emptyCellsTemplate from dependency array in the new useEffect hook (line 73-81) - this creates a stale closure bug where the effect uses outdated column structure if columns change

Confidence Score: 3/5

  • Safe to merge after fixing the dependency array issue in the useEffect hook
  • The PR successfully addresses the bug of tables not appearing when no rows are defined. However, there is a critical logic error in the new useEffect hook - the dependency array is missing emptyCellsTemplate, which will cause stale closures if columns change after mount. This should be fixed before merging to avoid potential bugs.
  • Review the useEffect hook dependency array at table.tsx:73-81

Important Files Changed

File Analysis

Filename Score Overview
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel-new/components/editor/components/sub-block/components/table/table.tsx 4/5 Added useEffect to initialize empty table and improved row validation logic with memoized template

Sequence Diagram

sequenceDiagram
    participant Component as Table Component
    participant Hook as useSubBlockValue
    participant Store as SubBlockStore
    participant Effect as useEffect

    Note over Component: Mount with no rows defined
    Component->>Component: columns prop received
    Component->>Component: useMemo emptyCellsTemplate
    Component->>Hook: useSubBlockValue(blockId, subBlockId)
    Hook->>Store: Get storeValue
    Store-->>Hook: null or []
    Hook-->>Component: storeValue = [] or null
    
    Component->>Effect: useEffect triggered
    Effect->>Effect: Check !isPreview && !disabled && empty storeValue
    Effect->>Effect: Create initialRow with emptyCellsTemplate
    Effect->>Hook: setStoreValue([initialRow])
    Hook->>Store: Update workflowValues
    Store-->>Hook: Updated
    Hook-->>Component: Re-render with [initialRow]
    
    Component->>Component: useMemo rows
    Component->>Component: Render table with 1 empty row
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@waleedlatif1 waleedlatif1 merged commit e1a46c9 into staging Nov 12, 2025
9 checks passed
@waleedlatif1 waleedlatif1 deleted the fix/table branch November 12, 2025 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants