allow dual functionality of save and create buttons between sample and sbmm sample and chemicals tabs#3069
Conversation
… update sample and sbmm sample and chemicals in a chain order
There was a problem hiding this comment.
Pull request overview
Enables the primary Save/Create actions in Sample and SBMM detail views to also persist ChemicalTab edits (including for “new” elements where the chemical cannot be saved until after the parent element exists).
Changes:
- Updated
ChemicalTabto initialize an empty chemical on first edit and expose agetChemicalSnapshot()method for deferred creation. - Added “chain save” behavior in Sample and SBMM details: save/create the parent element first, then save/create the chemical, keeping the Inventory tab mounted to avoid losing state.
- Implemented deferred chemical creation after navigating to the newly-created element via module-level “pending snapshot” slots.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| app/javascript/src/components/chemicals/ChemicalTab.js | Initializes chemical on edit; adds snapshot method for deferred chemical create. |
| app/javascript/src/apps/mydb/elements/details/sequenceBasedMacromoleculeSamples/SequenceBasedMacromoleculeSampleDetails.js | Adds chain-save and deferred chemical creation for SBMM samples; keeps Inventory tab mounted. |
| app/javascript/src/apps/mydb/elements/details/samples/SampleDetails.js | Adds chain-save and deferred chemical creation for samples; keeps Inventory tab mounted; updates save button enablement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .then(() => { | ||
| // Re-fetch chemical so ChemicalTab renders the newly-created record | ||
| // without requiring a page refresh. | ||
| this.chemicalTabRef.current?.fetchChemical(sample); | ||
| }) | ||
| .catch((err) => console.log(err)); |
There was a problem hiding this comment.
ChemicalFetcher.create() already catches/logs internally and won’t reject, so errors here won’t reach a caller-side .catch(...). Since this is an out-of-band chemical create (not via ChemicalTab.handleSubmitSave()), handle/validate the returned JSON in this .then(...) chain and surface failures to users (e.g., NotificationActions.add(...)).
| .then(() => { | |
| // Re-fetch chemical so ChemicalTab renders the newly-created record | |
| // without requiring a page refresh. | |
| this.chemicalTabRef.current?.fetchChemical(sample); | |
| }) | |
| .catch((err) => console.log(err)); | |
| .then((createdChemical) => { | |
| // Validate the response; ChemicalFetcher.create does not reject on API | |
| // errors, so we need to inspect the returned JSON and notify the user. | |
| const hasError = | |
| !createdChemical || | |
| createdChemical.error || | |
| (Array.isArray(createdChemical.errors) && createdChemical.errors.length > 0) || | |
| createdChemical.success === false; | |
| if (hasError) { | |
| NotificationActions.add({ | |
| message: 'Unable to create chemical for this sample. Please try again or contact an administrator.', | |
| level: 'error', | |
| autoDismiss: 5 | |
| }); | |
| return; | |
| } | |
| // Re-fetch chemical so ChemicalTab renders the newly-created record | |
| // without requiring a page refresh. | |
| this.chemicalTabRef.current?.fetchChemical(sample); | |
| }) | |
| .catch((err) => { | |
| // This should only capture unexpected failures (e.g., coding/transport errors) | |
| // since ChemicalFetcher.create handles and logs API-level errors internally. | |
| console.log(err); | |
| }); |
| saveInventory: PropTypes.bool.isRequired, | ||
| setSaveInventory: PropTypes.func.isRequired, | ||
| editChemical: PropTypes.func.isRequired, | ||
| getChemicalSnapshot: PropTypes.func.isRequired, |
There was a problem hiding this comment.
getChemicalSnapshot is defined as an instance method on ChemicalTab, but it was added to ChemicalTab.propTypes as a required prop. Since no caller passes a getChemicalSnapshot prop (callers use a ref), this will trigger PropTypes warnings and is misleading. Remove this propType entry (or, if you intended a callback prop, implement/consume it via this.props).
| getChemicalSnapshot: PropTypes.func.isRequired, |
| // Module-level slot: holds chemical data to be created after a new SBMM sample is | ||
| // persisted (survives the component unmount/remount caused by navigateToNewElement). | ||
| let _sbmmPendingChemicalCreate = null; |
There was a problem hiding this comment.
Using a module-level mutable variable (_sbmmPendingChemicalCreate) to carry state across unmounts is not scoped to a specific SBMM element. If multiple SBMM detail panels/new SBMM creates happen concurrently (the store supports multiple open SBMM samples), the snapshot can be overwritten or consumed by the wrong instance. Consider storing pending snapshots keyed by the temporary element id (or in the mobx store/UI store keyed per open element) and only consuming the entry that matches the newly-created SBMM sample.
| }).then(() => { | ||
| // Re-fetch chemical so ChemicalTab renders the newly-created record | ||
| // without requiring a page refresh. | ||
| chemicalTabRef.current?.fetchChemical(sbmmSample); | ||
| }).catch((err) => console.log(err)); |
There was a problem hiding this comment.
ChemicalFetcher.create() already swallows errors internally (it has its own .catch(...)), so this chain will not reject and the later .catch(...) won’t run. To reliably detect failures, handle/validate the returned JSON here (e.g., check for error) or change ChemicalFetcher.create to rethrow so callers can catch.
| }).then(() => { | |
| // Re-fetch chemical so ChemicalTab renders the newly-created record | |
| // without requiring a page refresh. | |
| chemicalTabRef.current?.fetchChemical(sbmmSample); | |
| }).catch((err) => console.log(err)); | |
| }).then((response) => { | |
| // If ChemicalFetcher.create resolves with an error payload instead of | |
| // rejecting, detect it here so failures are not silently ignored. | |
| if (response && response.error) { | |
| // eslint-disable-next-line no-console | |
| console.error('Failed to create chemical for SBMM sample', response.error); | |
| return; | |
| } | |
| // Re-fetch chemical so ChemicalTab renders the newly-created record | |
| // without requiring a page refresh. | |
| chemicalTabRef.current?.fetchChemical(sbmmSample); | |
| }).catch((err) => { | |
| // This catch is for unexpected runtime errors (e.g., coding errors), | |
| // not for API-level failures swallowed inside ChemicalFetcher.create. | |
| // eslint-disable-next-line no-console | |
| console.error('Unexpected error while creating chemical for SBMM sample', err); | |
| }); |
| // Module-level slot: holds chemical data to be created after a new sample is | ||
| // persisted (survives the SampleDetails unmount/remount caused by navigateToNewElement). | ||
| let _pendingChemicalCreate = null; | ||
|
|
There was a problem hiding this comment.
Using a module-level mutable variable (_pendingChemicalCreate) to carry state across SampleDetails unmount/remount is not scoped to a specific sample. If multiple sample panels are open or a create flow fails/doesn’t navigate as expected, the snapshot can be consumed by the next non-new SampleDetails that mounts, creating a chemical for the wrong sample. Prefer storing pending snapshots keyed by a temporary element id in a store (or pass it through the create success callback) and only consuming the matching entry.
| // Re-fetch chemical so ChemicalTab renders the newly-created record | ||
| // without requiring a page refresh. | ||
| chemicalTabRef.current?.fetchChemical(sbmmSample); | ||
| }).catch((err) => console.log(err)); |
There was a problem hiding this comment.
After the pending chemical is created, sbmmStore.isChemicalEdited is never reset, so the UI can remain in a “dirty” state (save button stays visible / DetailCard.isPendingToSave stays true). On successful create, call sbmmStore.editChemical(false) (and consider surfacing failures to the user rather than only logging).
| }).catch((err) => console.log(err)); | |
| // Reset edited state now that the pending chemical has been created. | |
| sbmmStore.editChemical(false); | |
| }).catch((err) => console.error(err)); |
rather 1-story 1-commit than sub-atomic commits
commit title is meaningful => git history search
commit description is helpful => helps the reviewer to understand the changes
code is up-to-date with the latest developments of the target branch (rebased to it or whatever) => ⏩-merge for linear history is favoured
added code is linted
tests are passing (at least locally): we still have some random test failure on CI. thinking of asking spec/examples.txt to be commited
in case the changes are visible to the end-user, video or screenshots should be added to the PR => helps with user testing
testing coverage improvement is improved.
CHANGELOG : add a bullet point on top (optional: reference to github issue/PR )
parallele PR for documentation on docusaurus if the feature/fix is tagged for a release