Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Console error when opening context menu on tree table #2047

Merged

Conversation

AkshatJawne
Copy link
Contributor

Resolves #2029

Changes Implemented:

  • Added optional chaining when call formatValue function, to ensure that it is only called when it is defined

I had also considered the approach below, which achieved the same result of removing the exception in the console. Ultimately opted to not use this logic, under the assumption that we are okay with the resultRow being undefined.

However, if we want resultRow to be defined, this approach ensures that it is, via a default function that performs no formatting.

// Approach #2: Default function that performs no formatting if formatValue is undefined
const formatVal =
formatValue ?? ((value: unknown, column: DhType.Column) => value);
resultRow.push(
formatVal(viewportRow.data.get(c)?.value, this.columns[c])
);

@AkshatJawne AkshatJawne self-assigned this May 30, 2024
Copy link

codecov bot commented May 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.36%. Comparing base (8e6b6da) to head (3cec1b3).

Current head 3cec1b3 differs from pull request most recent head 310a928

Please upload reports for the commit 310a928 to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2047   +/-   ##
=======================================
  Coverage   46.36%   46.36%           
=======================================
  Files         673      673           
  Lines       38780    38779    -1     
  Branches     9826     9826           
=======================================
  Hits        17981    17981           
+ Misses      20748    20747    -1     
  Partials       51       51           
Flag Coverage Δ
unit 46.36% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

resultRow.push(
formatValue(viewportRow.data.get(c)?.value, this.columns[c])
formatValue?.(viewportRow.data.get(c)?.value, this.columns[c])
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems if formatValue is not defined, there is no point in entering the for loop at all. I would check before the loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, just tested and it seems like this also works, but definitely more efficient (since we are not going into the loop if its not defined anymore). Pushed change

@AkshatJawne AkshatJawne requested a review from bmingles May 31, 2024 14:20
@AkshatJawne AkshatJawne requested a review from bmingles May 31, 2024 14:53
Copy link
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

Code changes look good. I confirmed the error no longer shows up when opening the context menu on tree table.

@AkshatJawne AkshatJawne merged commit 77bea7d into deephaven:main May 31, 2024
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 31, 2024
c += 1
) {
resultRow.push(
formatValue(viewportRow.data.get(c)?.value, this.columns[c])
Copy link
Collaborator

Choose a reason for hiding this comment

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

We still want to populate the resultRow array with unformatted values if formatValue is not defined.
With this change, the resultRow ends up empty.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opening context menu on tree tables throws an exception in the browser console
3 participants