Repository navigation
fix: SMILES cache collision, multi-bond rendering, and identifier restore crash - #46
Merged
Merged
Conversation
getCacheKey lowercased the whole value, but SMILES is case-sensitive
(uppercase 'C' is aliphatic carbon, lowercase 'c' is aromatic). Looking
up benzene ('c1ccccc1') and then cyclohexane ('C1CCCCC1') collided on the
key 'smiles:c1ccccc1', so the cache returned benzene's identifiers (CAS,
IUPAC name, PubChem CID) for cyclohexane and merged them into history.
Preserve case for SMILES keys, mirroring getLocalIdentifiers and
moleculeParser which already special-case SMILES.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Claude-Session: https://claude.ai/code/session_0179jy7dFsNKpjfEQCmeQTQE
Double and triple bonds separated their parallel cylinders by adding the offset to world X (position[0]) only. For a bond aligned with the X axis the cylinders were offset *along* the bond instead of beside it, so they overlapped and poked past the atoms. CO2 (O=C=O, a built-in example with all atoms colinear on X) rendered both double bonds incorrectly. Compute a unit vector perpendicular to each bond (falling back to another axis for vertical bonds) and offset the parallel cylinders along it. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_0179jy7dFsNKpjfEQCmeQTQE
loadHistory/loadFavourites only validate that the stored value is an array, not the shape of each entry's identifiers. The ChemicalIdentifiers set has grown over time, so restoring a history/favourite entry saved by an older build (or any hand-edited localStorage) could leave a field undefined. InputForm then called .trim() on it, throwing and white- screening the app with no error boundary. Default missing identifier fields to '' at the read sites. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_0179jy7dFsNKpjfEQCmeQTQE
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
The perpendicular-offset fix rendered double/triple bonds with four near-identical Cylinder + meshStandardMaterial blocks and repeated the three-axis offset math inline, which tripped SonarCloud's new-code duplication gate. Extract a BondCylinder component and a shiftAlong helper and collapse the single/double/triple cases to use them. No rendering change: a double bond is still two offset cylinders and a triple is a centered cylinder plus two offset ones. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_0179jy7dFsNKpjfEQCmeQTQE
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three independent correctness fixes found during a cross-repo code-quality sweep, one per commit.
1.
fix(lookup): SMILES cache key collided case-insensitivelygetCacheKeylowercased the whole identifier value, but SMILES is case-sensitive — uppercaseCis aliphatic carbon, lowercasecis aromatic. Looking up benzene (c1ccccc1) and then cyclohexane (C1CCCCC1) both mapped tosmiles:c1ccccc1, so the second lookup returned benzene's cached identifiers (CAS71-43-2, IUPAC name, PubChem CID241) for cyclohexane and merged them into history. Now SMILES keys preserve case, matching the case-sensitive handling already ingetLocalIdentifiersandmoleculeParser.2.
fix(viewer): double/triple bonds offset along the wrong axisMolecule3DComponentseparated the parallel cylinders of a multi-bond by adding the offset to world X (position[0]) only. For a bond aligned with the X axis, the cylinders were shifted along the bond instead of beside it, so they overlapped and poked past the atoms — e.g. CO₂ (O=C=O, a built-in Quick Example with all atoms colinear on X) rendered both double bonds wrong. Now a unit vector perpendicular to each bond is computed (with a fallback axis for vertical bonds) and the cylinders are offset along it.3.
fix(form): restoring an older stored entry could white-screen the apploadHistory/loadFavouritesonly validate that the parsed value is an array, not the shape of each entry'sidentifiers. The identifier set has grown over time, so restoring a history/favourite entry saved by an older build (or any hand-editedlocalStorage) could leave a field undefined;InputFormthen called.trim()on it and threw, blanking the app since there's no error boundary. The read sites now default missing fields to''.Testing
npm run build(tsc && vite build) — succeeds, no type errors. (Pre-existing unrelatedBatchedMesh/chunk-size warnings only.)npx eslinton the three changed files with--max-warnings 0— clean.🤖 Generated with Claude Code
https://claude.ai/code/session_0179jy7dFsNKpjfEQCmeQTQE
Generated by Claude Code