Skip to content

fix: correct offline benzene bonds and guard corrupted localStorage - #45

Merged
djleamen merged 2 commits into
mainfrom
claude/awesome-planck-wzrudh
Sep 10, 2026
Merged

djleamen merged 2 commits into
mainfrom
claude/awesome-planck-wzrudh

Conversation

@djleamen

@djleamen djleamen commented Sep 8, 2026

Copy link
Copy Markdown
Owner

Summary

Two small, self-contained correctness fixes found during a cross-repo maintenance review. No behavior change on the normal (online) path.

1. Offline benzene fallback drew bonds between the wrong atoms

createBenzene() in src/utils/moleculeParser.ts built the atoms array interleaved (C0, H6, C1, H7, …), so carbon i landed at array index 2*i. But the bond list references atoms by array index (atomIndex1: i, atomIndex2: i + 6), and Molecule3DComponent consumes those indices directly (centeredAtoms[bond.atomIndex1]). The result: every bond in the offline benzene fallback was drawn between the wrong atoms (e.g. the C0–C1 bond went from C0 to H6). All the other fallbacks use sequential ids where id === index, so only benzene was affected.

Fix: push all six carbons first, then all six hydrogens, so each atom's array index matches the index the bond list references. Coordinates and bond orders are unchanged.

2. Corrupted localStorage could white-screen the app

loadHistory() / loadFavourites() in src/utils/storage.ts only guarded JSON.parse in their try/catch. A value that is valid JSON but not an array (e.g. {} after tampering or a schema change) was returned as MoleculeEntry[], then crashed at render when useFavourites/useMoleculeHistory called .some/.filter/.map on it. Now the parsed value's shape is validated and a non-array falls back to [].

Testing

  • tsc --noEmit — passes
  • eslint on both changed files — passes (0 warnings)

Notes

The scan also flagged a stale-closure history entry in App.tsx and a per-hover console.log in AtomTooltip.tsx; both are cosmetic and left out to keep this PR to correctness fixes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WRqVTYbgdzZKc9kB55P8w4


Generated by Claude Code

djleamen and others added 2 commits September 8, 2026 07:18
The offline benzene fallback interleaved carbon and hydrogen atoms in the
atoms array (C0, H6, C1, H7, ...), but the bond list references atoms by
their array index (carbon i -> i, hydrogen i -> i + 6). As a result every
bond was drawn between the wrong atoms whenever the offline fallback path
was hit. Build all carbons first, then all hydrogens, so each atom's array
index matches the index the bond list references.

Also harden loadHistory/loadFavourites: the try/catch only guarded
JSON.parse, so a valid-but-non-array stored value (e.g. {} after tampering
or a schema change) was returned as MoleculeEntry[] and crashed the app at
render when .some/.filter/.map ran on it. Validate the parsed shape and
fall back to an empty array.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Claude-Session: https://claude.ai/code/session_01WRqVTYbgdzZKc9kB55P8w4
The benzene offline fallback interleaved carbons and hydrogens in the atoms
array (C0, H6, C1, H7, ...), so carbon i landed at array index 2*i. The bond
list, however, references atoms by array index (carbon i -> i, hydrogen
i -> i + 6), which Molecule3DComponent consumes directly. Every benzene bond
was therefore drawn between the wrong atoms on the offline path. Push all
carbons first, then all hydrogens, so array indices line up with the bonds.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Claude-Session: https://claude.ai/code/session_01WRqVTYbgdzZKc9kB55P8w4
Copilot AI lite review requested due to automatic review settings September 8, 2026 11:20
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@sonarqubecloud

sonarqubecloud Bot commented Sep 8, 2026

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
B Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The changes are small, self-contained, and directly address confirmed correctness issues without introducing new external dependencies or behavioral changes on the normal path.

Pull request overview

This PR applies two small correctness fixes in core utilities: it corrects atom indexing in the offline benzene fallback so bonds render between the intended atoms, and it hardens localStorage history/favourites loading against non-array JSON values to prevent runtime crashes.

Changes:

  • Fix offline benzene atom ordering so bond indices align with the atoms array indices.
  • Validate parsed localStorage values are arrays before returning them from loadHistory() / loadFavourites().
File summaries
File Description
src/utils/storage.ts Ensures parsed localStorage values are arrays (otherwise returns []) to avoid .map/.filter/.some crashes on non-arrays.
src/utils/moleculeParser.ts Reorders benzene atom construction (carbons first, then hydrogens) so bond indices reference the intended atoms in the offline fallback.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

djleamen commented Sep 8, 2026

Copy link
Copy Markdown
Owner Author

The SonarCloud Code Analysis check is failing its Quality Gate on one condition: "B Security Rating on New Code" (required ≥ A). All other checks (Node.js CI, Dependency Review, Copilot review) are green.

I wasn't able to pin down the specific finding from this environment:

  • sonarcloud.io is unreachable here (blocked by the network egress proxy), so I can't open the dashboard or query the issues API.
  • The failed check run exposes no output.text and no inline annotations, and SonarCloud runs as an app integration (not a GitHub Actions workflow), so there are no job logs to inspect either.

Reviewing the diff itself, I don't see a security vulnerability introduced by this PR. The two changes are:

  1. createBenzene() — reordering two loops so atom array indices line up with the bond list (pure logic reorder, no new APIs).
  2. loadHistory() / loadFavourites() — adding an Array.isArray(...) guard around the pre-existing JSON.parse(localStorage.getItem(...)) (my edit touches that line, which likely re-dates it as "new code" for the gate).

Could you take a look at the finding on the SonarCloud dashboard? If it's the JSON.parse line being re-flagged as new code (or a rule you're comfortable accepting), it can be marked accordingly there; if it points to a genuine issue in the diff I've missed, tell me the rule/line and I'll push a fix. I'll keep watching this PR.


Generated by Claude Code

@djleamen
djleamen merged commit e930b43 into main Sep 10, 2026
9 of 10 checks passed
@djleamen
djleamen deleted the claude/awesome-planck-wzrudh branch September 10, 2026 23:58
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.

2 participants