Skip to content

Disable option to export a "network_optimized_weights" sheet for an unweighted network#1282

Merged
dondi merged 5 commits intobetafrom
maika-1223
Nov 19, 2025
Merged

Disable option to export a "network_optimized_weights" sheet for an unweighted network#1282
dondi merged 5 commits intobetafrom
maika-1223

Conversation

@ntran18
Copy link
Collaborator

@ntran18 ntran18 commented Nov 16, 2025

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR disables the export option for the "network_optimized_weights" sheet when the network is unweighted (i.e., when optimized weights data is not available).

Key changes:

  • Added an isDataValid helper function to check if data exists and is not an empty object
  • Updated the validation logic for network sheets to use isDataValid instead of simple undefined checks
  • Fixed the "Select All" checkbox behavior to exclude disabled checkboxes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

};

const isDataValid = data => {
if (data == undefined || data == null) {
Copy link

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

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

This guard always evaluates to false.

Suggested change
if (data == undefined || data == null) {
if (data == null) {

Copilot uses AI. Check for mistakes.
Copy link
Owner

Choose a reason for hiding this comment

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

Good line to flag, but not the right suggestion, sorry Copilot 🫤

@ntran18 my counter-suggestion here is to use === rather than ==. That way you avoid all of the JavaScript idiosyncrasies that go with the double-version

Suggested change
if (data == undefined || data == null) {
if (data === undefined || data === null) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for catching this! I always forget about triple equal for JavaScript

Copy link
Owner

@dondi dondi left a comment

Choose a reason for hiding this comment

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

General approach looks good, but a few refinements are called for to improve robustness and consistency

};

const isDataValid = data => {
if (data == undefined || data == null) {
Copy link
Owner

Choose a reason for hiding this comment

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

Good line to flag, but not the right suggestion, sorry Copilot 🫤

@ntran18 my counter-suggestion here is to use === rather than ==. That way you avoid all of the JavaScript idiosyncrasies that go with the double-version

Suggested change
if (data == undefined || data == null) {
if (data === undefined || data === null) {

@coveralls
Copy link

coveralls commented Nov 19, 2025

Coverage Status

coverage: 80.06%. remained the same
when pulling a9fba94 on maika-1223
into 569311d on beta.

@ntran18
Copy link
Collaborator Author

ntran18 commented Nov 19, 2025

Thanks for the suggestions! All the suggestions are applied!

Copy link
Owner

@dondi dondi left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes! LGTM

@dondi dondi merged commit 78a8cf6 into beta Nov 19, 2025
5 checks passed
@dondi dondi deleted the maika-1223 branch November 19, 2025 07:45
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