refactor(gas-phase): synchronize calculations in ReactionDetailsScheme by adambasha0 · Pull Request #2789 · ComPlat/chemotion_ELN

adambasha0 · 2025-11-22T09:09:29Z

Refactor:

correct gas product volume equation
update grams when gas product moles change
update TON when catalyst mole value changes
recalculate dependent gas-product fields when vessel volume changes
refactor unit conversion tests for gas-phase scheme
add ReactionDetailsScheme unit tests for gas-phase reactions

- correct gas product volume equation - update grams when gas product moles change - update TON when catalyst mole value changes - recalculate dependent gas-product fields when vessel volume changes - refactor unit conversion tests for gas-phase scheme - add ReactionDetailsScheme unit tests for gas-phase reactions

Copilot

Pull request overview

This PR refactors gas-phase reaction calculations in the ReactionDetailsScheme component to improve synchronization and correctness of dependent field calculations. The changes focus on ensuring that gas product properties (volume, moles, grams, TON) are properly recalculated when vessel volume or catalyst mole values change.

Key changes:

Corrected gas product volume calculation formula and updated related unit conversion logic
Added automatic recalculation of gas product fields when vessel size changes
Enhanced TON/TOF calculations to update when catalyst moles change
Improved test coverage with comprehensive gas-phase reaction test suite

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
`app/javascript/src/utilities/UnitsConversion.js`	Refactored `calculateVolumeForFeedstockOrGas` to accept vessel volume parameter; improved `determineTONFrequencyValue` with better input validation and documentation
`app/javascript/src/models/Sample.js`	Updated volume and gram conversions for gas-type samples to use vessel volume; added gas-specific gram calculation when amount unit is 'g'
`app/javascript/src/apps/mydb/elements/details/reactions/schemeTab/ReactionDetailsScheme.js`	Added vessel size change event handler to trigger gas product recalculations; updated TON values when catalyst amount changes; improved template state handling
`app/javascript/src/apps/mydb/elements/details/reactions/ReactionDetails.js`	Removed async wrapper from `updateReactionVesselSize`; added synchronous store updates for vessel size changes; initialized gas phase store on component mount and update
`app/javascript/src/apps/mydb/elements/details/reactions/variationsTab/ReactionVariationsMaterials.js`	Added zero-purity guard in `getGramFromMol` to prevent division by zero
`spec/javascripts/utils/UnitConversion.spec.js`	Updated test parameters for `calculateVolumeForFeedstockOrGas` to match refactored function signature
`spec/javascripts/packs/src/apps/mydb/elements/details/reactions/schemeTab/GasPhaseReactionDetailsScheme.spec.js`	Added comprehensive test suite covering feedstock, catalyst, and gas product calculations including edge cases
`spec/javascripts/packs/src/apps/mydb/elements/details/reactions/variationsTab/ReactionVariationsComponents.spec.js`	Updated volume assertion to verify it's defined rather than expecting proportional scaling
`spec/javascripts/helper/gasPhaseReactionTestHelpers.js`	New helper file providing factory functions for creating gas-phase test data
`spec/javascripts/helper/reactionVariationsHelpers.js`	Added `molecule_molecular_weight` property definition to prevent getter loss during deep cloning; updated gas phase test data with realistic values

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

...cks/src/apps/mydb/elements/details/reactions/schemeTab/GasPhaseReactionDetailsScheme.spec.js

Copilot · 2025-11-24T16:00:26Z

app/javascript/src/utilities/UnitsConversion.js

  if (gasType === 'gas') {
+    molAmount = calculateGasMoles(vesselVolume, part_per_million, temperatureInKelvin);
    return calculateGasVolume(molAmount, gasPhaseData);


[nitpick] There's a circular calculation issue in the calculateVolumeForFeedstockOrGas function for gas type. When gasType === 'gas', the function:

Calculates moles from vessel volume using calculateGasMoles(vesselVolume, part_per_million, temperatureInKelvin)

Then immediately calculates volume from those moles using calculateGasVolume(molAmount, gasPhaseData)

This means for gas products, the function is calculating: volume = moles * R * T where moles = (ppm * vesselVolume) / (R * T * 1e6), which simplifies to volume = (ppm * vesselVolume) / 1e6. This appears to be calculating the actual volume of gas based on ppm concentration, not verifying the calculation round-trip.

According to the test comment on line 79-80, the expected behavior seems correct (calculating actual gas volume from ppm), but the function name calculateVolumeForFeedstockOrGas and the circular pattern may be confusing. Consider clarifying the purpose in documentation or renaming the function to better reflect that it's calculating the actual volume occupied by the gas based on ppm concentration.

@copilot there is a difference between vesselVolume which is vessel size of the reaction, and volume of gas product!

app/javascript/src/models/Sample.js

app/javascript/src/apps/mydb/elements/details/reactions/schemeTab/ReactionDetailsScheme.js

...cks/src/apps/mydb/elements/details/reactions/schemeTab/GasPhaseReactionDetailsScheme.spec.js

app/javascript/src/models/Sample.js

app/javascript/src/utilities/UnitsConversion.js

...cks/src/apps/mydb/elements/details/reactions/schemeTab/GasPhaseReactionDetailsScheme.spec.js

…ction save

app/models/concerns/reactable.rb

app/models/concerns/unit_convertable.rb

github-actions · 2025-11-24T16:26:19Z

app/models/concerns/unit_convertable.rb

+    if gas_type == 'feedstock' && unit == 'l'
+      calculate_feedstock_mmol(value)
+    else
+      val_g = self.convert_to_gram(value, unit)


Style/RedundantSelf: Redundant self detected.

github-actions · 2025-11-24T16:26:19Z

app/models/concerns/unit_convertable.rb

+      calculate_feedstock_mmol(value)
+    else
+      val_g = self.convert_to_gram(value, unit)
+      self.convert_to_unit(val_g, 'mol', true)


Style/RedundantSelf: Redundant self detected.

Copilot · 2025-11-25T14:37:35Z

@adambasha0 I've opened a new pull request, #2793, to work on those changes. Once the pull request is ready, I'll request review from you.

- solve copilot messages - allow calculation of feedstock mmol value with gram unit on server level

github-actions · 2025-11-25T15:36:18Z

app/models/concerns/unit_convertable.rb

  end

-  def amount_mmol(type = 'target')
+  def calculate_feedstock_mmol(value, unit)


Metrics/ModuleLength: Module has too many lines. [114/100]

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

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

app/models/concerns/unit_convertable.rb

...cks/src/apps/mydb/elements/details/reactions/schemeTab/GasPhaseReactionDetailsScheme.spec.js

app/javascript/src/apps/mydb/elements/details/reactions/schemeTab/ReactionDetailsScheme.js

spec/javascripts/utils/UnitConversion.spec.js

- Guard `calculateGasMoles` against missing/zero temperature to prevent divide-by-zero/NaN - Accept an optional `moles` argument in `calculateVolumeForFeedstockOrGas` and prefer explicit mole input - Update `Sample` model to correctly handle decoupled samples and unify gas-phase condition checks - Normalize purity handling and simplify gram-from-mol conversion in reaction variations helper - Update unit-conversion spec to match new function signature/behavior

…Tab/ReactionDetailsScheme.js Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.

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

app/javascript/src/models/Sample.js

Copilot · 2025-11-27T08:35:58Z

spec/javascripts/utils/UnitConversion.spec.js

+    // vesselVolume=10L, ppm=1000, T=294K => moles=0.000414 => volume=0.01L
+    // For feedstock: volume = (moles * R * 294K) / purity, where moles = amountInGram / molecularWeight
+    // purity=0.5 => moles=5 => volume=241.374L
+    const gasResult = calculateVolumeForFeedstockOrGas(10, 0.5, 'gas', gasPhaseData, 5);


The comment at line 79 states "For gas: volume = (moles * R * T), where moles = (ppm * V) / (R * T * 1e6)", but the function signature passes moles=5 as the last parameter. This creates confusion because:

When gasType === 'gas', the function recalculates molAmount using calculateGasMoles (line 145 in UnitsConversion.js), ignoring the passed moles parameter.

The comment suggests moles = 0.000414 (calculated from ppm), but then passes moles=5 which is never used.

Either update the comment to clarify that the moles parameter is ignored for gas types, or remove the misleading moles=5 from the test call since it has no effect on the gas calculation.

Suggested change

// vesselVolume=10L, ppm=1000, T=294K => moles=0.000414 => volume=0.01L

// For feedstock: volume = (moles * R * 294K) / purity, where moles = amountInGram / molecularWeight

// purity=0.5 => moles=5 => volume=241.374L

const gasResult = calculateVolumeForFeedstockOrGas(10, 0.5, 'gas', gasPhaseData, 5);

// Note: The 'moles' parameter is ignored for gas; it is recalculated from ppm, vessel volume, R, and T.

// vesselVolume=10L, ppm=1000, T=294K => moles=0.000414 => volume=0.01L

// For feedstock: volume = (moles * R * 294K) / purity, where moles = amountInGram / molecularWeight

// purity=0.5 => moles=5 => volume=241.374L

const gasResult = calculateVolumeForFeedstockOrGas(10, 0.5, 'gas', gasPhaseData, null);

Copilot · 2025-11-27T08:35:58Z

app/models/concerns/unit_convertable.rb

+      purity_factor = purity || 1.0
+      ideal_gas_constant = 0.0821
+      default_temp_k = 294.0
+      (value * purity_factor * 1000) / (ideal_gas_constant * default_temp_k)


The calculate_feedstock_mmol method doesn't handle the case when unit is neither 'g' nor 'l'. This will cause the method to implicitly return nil for unexpected units, which could lead to runtime errors when the return value is used in calculations.

Add an else clause to return 0 or raise an appropriate error for unsupported units:

else 0 end

Suggested change

(value * purity_factor * 1000) / (ideal_gas_constant * default_temp_k)

(value * purity_factor * 1000) / (ideal_gas_constant * default_temp_k)

else

0

Copilot · 2025-11-27T08:35:58Z

app/javascript/src/apps/mydb/elements/details/reactions/schemeTab/ReactionDetailsScheme.js

+  updatedSamplesForVesselSizeChange(samples) {
+    const vesselSize = GasPhaseReactionStore.getState().reactionVesselSizeValue;
+    return samples.map((sample) => {
+      if (sample.gas_type === 'gas' && sample.gas_phase_data) {
+        const { part_per_million, temperature } = sample.gas_phase_data;
+        let temperatureInKelvin = temperature.value;
+        if (temperature.unit !== 'K') {
+          temperatureInKelvin = convertTemperatureToKelvin({ value: temperature.value, unit: temperature.unit });
+        }
+        const moles = calculateGasMoles(vesselSize, part_per_million, temperatureInKelvin);
+        sample.setAmount({ value: moles, unit: 'mol' });
+        sample.updateTONValue(moles);
+      }
+      return sample;
+    });
+  }


The updatedSamplesForVesselSizeChange method updates moles and TON for gas products when vessel size changes, but it doesn't recalculate the equivalent (yield) value. According to the PR description, "recalculate dependent gas-product fields when vessel volume changes" should include the equivalent calculation.

The equivalent for gas products is calculated in calculateEquivalentForGasProduct() which depends on vessel volume. When vessel volume changes, the equivalent should be updated similar to how it's done in updatedReactionForGasProductFieldsChange (lines 595-596):

const equivalent = this.calculateEquivalentForGasProduct(sample); sample.equivalent = equivalent;

Copilot · 2025-11-27T08:35:59Z

...cks/src/apps/mydb/elements/details/reactions/schemeTab/GasPhaseReactionDetailsScheme.spec.js

+  });
+
+  describe('Feedstock Material Calculations', () => {
+    it('should calculate moles from volume using Calculation1: Mol = Purity × 1 × V/(0.0821 × 294)', () => {


The comment at line 80 contains a typo in the formula. It states "Mol = Purity × 1 × V/(0.0821 × 294)" but the "1" multiplier is redundant and confusing. The formula should be simplified to "Mol = Purity × V/(0.0821 × 294)" to match the actual calculation on line 93.

Suggested change

it('should calculate moles from volume using Calculation1: Mol = Purity × 1 × V/(0.0821 × 294)', () => {

it('should calculate moles from volume using Calculation1: Mol = Purity × V/(0.0821 × 294)', () => {

Copilot · 2025-11-27T08:35:59Z

spec/javascripts/utils/UnitConversion.spec.js

-    assert.strictEqual(calculateVolumeForFeedstockOrGas(null, 2, 0.5, 'gas', gasPhaseData), 0);
+    // Parameters: vesselVolume, purity, gasType, gasPhaseData, amountInGram, molecularWeight
+    // Invalid temperature should cause calculateGasVolume to return 0
+    assert.strictEqual(calculateVolumeForFeedstockOrGas(10, 0.5, 'gas', gasPhaseData, null, null), 0);


Superfluous argument passed to function calculateVolumeForFeedstockOrGas.

Suggested change

assert.strictEqual(calculateVolumeForFeedstockOrGas(10, 0.5, 'gas', gasPhaseData, null, null), 0);

assert.strictEqual(calculateVolumeForFeedstockOrGas(10, 0.5, 'gas', gasPhaseData), 0);

…ount

…st from gas phase reaction

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

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

app/javascript/src/apps/mydb/elements/details/reactions/schemeTab/ReactionDetailsScheme.js

app/javascript/src/utilities/UnitsConversion.js

Copilot · 2025-12-05T08:29:02Z

spec/javascripts/helper/reactionVariationsHelpers.js

+  // Ensure molecule_molecular_weight is set as a property on ALL materials, not just a getter
+  // This is needed because cloneDeep in getReactionMaterials won't preserve getters
+  [reaction.starting_materials, reaction.reactants, reaction.products].flat().forEach((material) => {
+    if (material && material.molecule && material.molecule.molecular_weight) {
+      Object.defineProperty(material, 'molecule_molecular_weight', {
+        value: material.molecule.molecular_weight,
+        writable: true,
+        enumerable: true,
+        configurable: true
+      });
+    }
+  });


[nitpick] The comment at line 24-25 states "This is needed because cloneDeep in getReactionMaterials won't preserve getters". While this is a valid workaround, consider whether getReactionMaterials should be updated to handle getters properly instead of adding this property definition logic in multiple places (this pattern is duplicated at lines 24-35 and 78-89). This could be a maintenance concern if the same pattern needs to be repeated elsewhere.

Suggested change

// Ensure molecule_molecular_weight is set as a property on ALL materials, not just a getter

// This is needed because cloneDeep in getReactionMaterials won't preserve getters

[reaction.starting_materials, reaction.reactants, reaction.products].flat().forEach((material) => {

if (material && material.molecule && material.molecule.molecular_weight) {

Object.defineProperty(material, 'molecule_molecular_weight', {

value: material.molecule.molecular_weight,

writable: true,

enumerable: true,

configurable: true

});

}

});

// molecule_molecular_weight property is now set in getReactionMaterials

the review comment is valid, but might be suitable for another PR

...script/src/apps/mydb/elements/details/reactions/variationsTab/ReactionVariationsMaterials.js

TasnimMehzabin · 2025-12-05T08:35:13Z

app/javascript/src/apps/mydb/elements/details/reactions/schemeTab/ReactionDetailsScheme.js

+        if (sample.isGas()) {
          const equivalent = this.calculateEquivalentForGasProduct(sample);
          sample.equivalent = equivalent;
+          if ((prevGasType === 'CAT' && updatedSample.gas_type === 'off') || updatedSample.isCatalyst()) {


updatedSample.gas_type === 'off' can be moved to Sample.js

port of refactor(gas-phase): synchronize calculations in ReactionDetailsScheme (#2789) * Refactor(gas-phase): synchronize calculations in ReactionDetailsScheme - correct gas product volume equation - update grams when gas product moles change - update TON when catalyst mole value changes - recalculate dependent gas-product fields when vessel volume changes - refactor unit conversion tests for gas-phase scheme - add ReactionDetailsScheme unit tests for gas-phase reactions * refactor spect tests for gas phase ReactionDetailsScheme * fix: calculate correct equivalent value for feedstock material on reaction save * refactor: - solve copilot messages - allow calculation of feedstock mmol value with gram unit on server level * refactor(gas-phase): fix feedstock/gas mole & volume calculations - Guard `calculateGasMoles` against missing/zero temperature to prevent divide-by-zero/NaN - Accept an optional `moles` argument in `calculateVolumeForFeedstockOrGas` and prefer explicit mole input - Update `Sample` model to correctly handle decoupled samples and unify gas-phase condition checks - Normalize purity handling and simplify gram-from-mol conversion in reaction variations helper - Update unit-conversion spec to match new function signature/behavior * refactor: fix equation for calculating feedstock volume from moles amount * fix: reset TON and TOF on unassigning or deleting a catalyst a catalyst from gas phase reaction

adambasha0 self-assigned this Nov 22, 2025

adambasha0 added the bug label Nov 22, 2025

refactor spect tests for gas phase ReactionDetailsScheme

27c530f

TasnimMehzabin requested a review from Copilot November 24, 2025 15:55

Copilot started reviewing on behalf of TasnimMehzabin November 24, 2025 15:55 View session

Copilot finished reviewing on behalf of TasnimMehzabin November 24, 2025 15:58

Copilot AI reviewed Nov 24, 2025

View reviewed changes

fix: calculate correct equivalent value for feedstock material on rea…

0af84e9

…ction save

github-actions bot reviewed Nov 24, 2025

View reviewed changes

Copilot AI mentioned this pull request Nov 25, 2025

Clarify vessel volume vs gas product volume distinction in gas-phase calculations #2793

Closed

10 tasks

refactor:

bdc8807

- solve copilot messages - allow calculation of feedstock mmol value with gram unit on server level

github-actions bot reviewed Nov 25, 2025

View reviewed changes

adambasha0 requested a review from Copilot November 25, 2025 21:17

Copilot started reviewing on behalf of adambasha0 November 25, 2025 21:17 View session

Copilot finished reviewing on behalf of adambasha0 November 25, 2025 21:20

Copilot AI reviewed Nov 25, 2025

View reviewed changes

adambasha0 and others added 2 commits November 26, 2025 11:58

Update app/javascript/src/apps/mydb/elements/details/reactions/scheme…

28e6cef

…Tab/ReactionDetailsScheme.js Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

TasnimMehzabin requested a review from Copilot November 27, 2025 08:31

Copilot started reviewing on behalf of TasnimMehzabin November 27, 2025 08:31 View session

Copilot finished reviewing on behalf of TasnimMehzabin November 27, 2025 08:34

Copilot AI reviewed Nov 27, 2025

View reviewed changes

adambasha0 added 2 commits December 2, 2025 13:55

refactor: fix equation for calculating feedstock volume from moles am…

f9a4260

…ount

fix: reset TON and TOF on unassigning or deleting a catalyst a cataly…

2ff918b

…st from gas phase reaction

TasnimMehzabin requested a review from Copilot December 5, 2025 08:23

Copilot started reviewing on behalf of TasnimMehzabin December 5, 2025 08:23 View session

Copilot finished reviewing on behalf of TasnimMehzabin December 5, 2025 08:27

Copilot AI reviewed Dec 5, 2025

View reviewed changes

TasnimMehzabin reviewed Dec 5, 2025

View reviewed changes

TasnimMehzabin approved these changes Dec 5, 2025

View reviewed changes

PiTrem changed the title ~~Refactor(gas-phase): synchronize calculations in ReactionDetailsScheme~~ refactor(gas-phase): synchronize calculations in ReactionDetailsScheme Dec 8, 2025

PiTrem merged commit b2c509e into v2.x Dec 8, 2025
9 of 12 checks passed

PiTrem mentioned this pull request Dec 19, 2025

fix: gas phase scheme calculations #2839

Merged

	it('should calculate moles from volume using Calculation1: Mol = Purity × 1 × V/(0.0821 × 294)', () => {
	it('should calculate moles from volume using Calculation1: Mol = Purity × V/(0.0821 × 294)', () => {

	assert.strictEqual(calculateVolumeForFeedstockOrGas(10, 0.5, 'gas', gasPhaseData, null, null), 0);
	assert.strictEqual(calculateVolumeForFeedstockOrGas(10, 0.5, 'gas', gasPhaseData), 0);

Conversation

adambasha0 commented Nov 22, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

adambasha0 Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Nov 25, 2025

Uh oh!

github-actions bot Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

adambasha0 Dec 5, 2025

Choose a reason for hiding this comment