Skip to content

Fix debugger exception during table creation#1758

Merged
PrzemyslawKlys merged 1 commit intomasterfrom
codex/fix-addtable-debugger-exception
Apr 9, 2026
Merged

Fix debugger exception during table creation#1758
PrzemyslawKlys merged 1 commit intomasterfrom
codex/fix-addtable-debugger-exception

Conversation

@PrzemyslawKlys
Copy link
Copy Markdown
Member

Summary

  • move the initial RefreshTblGridFromColumnWidths() call out of GenerateTable()
  • run the grid refresh only after _table has been assigned in each WordTable constructor path
  • preserve the tblGrid synchronization fix while removing the debugger-only first-chance NullReferenceException

Root Cause

GenerateTable() was calling RefreshTblGridFromColumnWidths() before the WordTable instance had assigned _table. The exception was caught during normal execution, but Visual Studio surfaced it as a handled first-chance exception when debugging, which made document.AddTable(2, 2) look broken.

Validation

  • dotnet build OfficeIMO.Word/OfficeIMO.Word.csproj
  • dotnet run --project $env:TEMP\officeimo-addtable-current\ReproCurrent\ReproCurrent.csproj

@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@claude
Copy link
Copy Markdown

claude bot commented Apr 9, 2026

Claude finished @PrzemyslawKlys's task —— View job


PR Review - Fix debugger exception during table creation

Todo List:

  • Read and analyze the modified WordTable.cs file
  • Review code quality and implementation
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Examine test coverage implications
  • Provide comprehensive feedback

Review Summary

This PR successfully addresses a debugger-only first-chance exception issue while preserving the important tblGrid synchronization functionality. The fix is well-executed and appropriate.

✅ Code Quality & Best Practices

Excellent approach:

  • Clean separation of concerns: Moved the RefreshTblGridFromColumnWidths() call out of the shared GenerateTable() method and into each specific constructor where _table is guaranteed to be assigned
  • Consistent pattern: All 5 constructor overloads now follow the same pattern: generate table → assign to _table → refresh grid
  • Preserved functionality: The fix maintains the critical tblGrid synchronization that prevents "50/50 column issues" in online viewers
  • Defensive programming: Maintains the try/catch wrapper around RefreshTblGridFromColumnWidths() calls

✅ Bug Analysis

Root cause correctly identified:

  • GenerateTable() was calling RefreshTblGridFromColumnWidths() before _table field assignment
  • The RefreshTblGridFromColumnWidths() method accesses this.Rows property which internally depends on _table.ChildElements.OfType<TableRow>()
  • When _table is null, this causes a NullReferenceException

Fix appropriateness: ⭐⭐⭐⭐⭐

  • Addresses the timing issue without changing the overall architecture
  • Exception was caught but triggered debugger first-chance exception notifications
  • Solution moves the call to after _table assignment in all constructor paths

✅ Performance Considerations

Minimal performance impact:

  • No change to runtime performance - same number of RefreshTblGridFromColumnWidths() calls
  • Slightly improved performance by avoiding unnecessary exception throwing/catching
  • The _suppressGridRefresh mechanism prevents recursive calls during table property adjustments

✅ Security Assessment

No security concerns identified:

  • Changes are purely internal timing/sequencing adjustments
  • No new attack vectors introduced
  • No exposure of sensitive data or operations

✅ Test Coverage

Existing coverage should be sufficient:

  • The fix preserves existing functionality exactly - just changes timing
  • Word.Tables.cs tests at line 26: WordTable wordTable = document.AddTable(3, 4); would verify the basic creation path works
  • The constructor paths are all tested through various table creation scenarios in the test suite

Validation approach is sound:

  • dotnet build ensures compilation succeeds
  • dotnet run with reproduction case verifies the debugger exception is eliminated

Minor Observations

Code consistency: The pattern is now perfectly consistent across all 5 constructor overloads:

  1. internal WordTable(WordDocument, WordParagraph, ...) - Line 476
  2. internal WordTable(WordDocument, int, int, ...) - Line 501
  3. public WordTable(WordDocument, TableCell, ...) - Line 524
  4. internal WordTable(WordDocument, Footer, ...) - Line 535
  5. internal WordTable(WordDocument, Header, ...) - Line 545

Documentation: The removed comment in GenerateTable() was appropriate since that method no longer handles grid refresh.

Recommendation: ✅ APPROVE

This is a high-quality fix that:

  • Solves the immediate debugger experience issue
  • Preserves critical functionality for online document viewers
  • Follows established patterns and defensive programming practices
  • Has minimal risk and no breaking changes

The implementation demonstrates good understanding of the codebase and addresses the root cause appropriately.


@PrzemyslawKlys PrzemyslawKlys mentioned this pull request Apr 9, 2026
@PrzemyslawKlys PrzemyslawKlys merged commit 347042a into master Apr 9, 2026
10 checks passed
@PrzemyslawKlys PrzemyslawKlys deleted the codex/fix-addtable-debugger-exception branch April 9, 2026 15:33
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.

1 participant