Skip to content

Conversation

@mikekobit-stripe
Copy link

@mikekobit-stripe mikekobit-stripe commented Mar 13, 2025

The getColumn method is already typed to return Column<TData, unknown> | undefined, indicating to callers that the result might be undefined. The current console.error logging when a column is not found:

  1. Adds noise in development environments
  2. Can mask actual issues
  3. Forces users to implement workarounds (as discussed in Remove error from row.getValue when column is not found or create new function #5505)

This change removes the error logging while maintaining the expected return type, allowing consumers to handle undefined columns as already expected by the API.

Summary by CodeRabbit

  • Refactor
    • Simplified internal column retrieval implementation, removing redundant development-mode error logging for missing columns.

✏️ Tip: You can customize this high-level summary in your review settings.

…lementation

The `getColumn` method is already typed to return `Column<TData, unknown> | undefined`,  indicating to callers that the result might be undefined. The current `console.error` logging when a column is not found:

1. Adds noise in development environments
2. Can mask actual issues
3. Forces users to implement workarounds (as discussed in TanStack#5505)

This change removes the error logging while maintaining the expected return type, allowing consumers to handle undefined columns as already expected by the API.
@mikekobit-stripe mikekobit-stripe changed the title Remove unnecessary console.error log from table.getColumn(id) implementation chore: Remove unnecessary console.error log from table.getColumn(id) implementation Mar 13, 2025
@mikekobit-stripe
Copy link
Author

Is anybody available to review this?

Copy link

@deepan-ashby deepan-ashby left a comment

Choose a reason for hiding this comment

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

Just ran into this myself, this seems like a reasonable fix

@changeset-bot
Copy link

changeset-bot bot commented Dec 19, 2025

⚠️ No Changeset found

Latest commit: ca34828

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link

coderabbitai bot commented Dec 19, 2025

Walkthrough

The getColumn function in the table core module was simplified by removing a development-mode runtime check and associated console logging. The function now directly returns the column lookup result without intermediate error handling.

Changes

Cohort / File(s) Summary
Simplified getColumn implementation
packages/table-core/src/core/table.ts
Removed development-mode runtime check and console.error from getColumn function; now directly returns table._getAllFlatColumnsById()[columnId] without logging when a column is missing

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Single-file refactoring with straightforward removal of error checking logic
  • No functional behavior changes; undefined will be returned silently in error cases

Poem

A bunny hops through simpler code,
Console.error bids adieu,
Straight returns, no checks in tow,
Cleaner paths the function flew,
Less noise, more flow! 🐰✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
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.
Description check ❓ Inconclusive The description provides clear context for the change, but is missing the required checklist items (Contributing guide confirmation and test verification) and release impact declaration. Complete the checklist by confirming adherence to Contributing guide and test completion, and specify whether a changeset was generated for this change.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: removing an unnecessary console.error log from the getColumn method implementation.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e172109 and ca34828.

📒 Files selected for processing (1)
  • packages/table-core/src/core/table.ts (1 hunks)
🔇 Additional comments (1)
packages/table-core/src/core/table.ts (1)

510-510: LGTM! Simplified implementation aligns with the type signature.

The direct return of the column lookup is appropriate since undefined is explicitly part of the return type. Removing the console.error eliminates unnecessary development noise for an expected outcome.


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.

❤️ Share

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

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.

4 participants