Skip to content

refactor: rename misleading variables and apply targeted code quality improvements#236

Merged
Aarebecca merged 5 commits into
mainfrom
copilot/rename-options-to-preview-code
Apr 16, 2026
Merged

refactor: rename misleading variables and apply targeted code quality improvements#236
Aarebecca merged 5 commits into
mainfrom
copilot/rename-options-to-preview-code

Conversation

Copilot AI commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Several small but meaningful issues across the codebase: a misleading state variable name, inconsistent gradient colors, missing NaN guards, a redundant code branch, and readability/performance gaps in utility functions.

dev/src/Playground.tsx

  • Renamed options/setOptionspreviewCode/setPreviewCode — the variable holds debounced code string, not options

src/designs/structures/chart-line.tsx

  • Added ITEM_POSITION_H/ITEM_POSITION_V typed constants (as const) for position literals
  • Fixed area gradient's final stop to use colorStopsData[last]?.color ?? colorPrimary instead of always colorPrimary, matching the stroke gradient's behavior

src/exporter/svg.ts

  • Added Number.isNaN guards before > 0 checks on parseAbsoluteLength results (returns NaN for invalid input)
  • Added braces around single-statement if (symbolElement) in embedIcons
  • Moved empty id guard before visited.add(id) to avoid polluting the visited set
  • Replaced the regex-based CSS.escape fallback with a spec-compliant implementation handling null chars, control chars, leading-digit/hyphen rules, and non-ASCII characters

src/resource/loaders/search.ts

  • Removed redundant if (mimeType === 'image/svg+xml' && format === 'svg' && isBase64) branch — it fell through to the identical return loadImageBase64Resource(result) call

src/templates/utils.ts

  • getCommonPrefixLength: switched from bracket indexing to charCodeAt for tighter inner-loop performance
  • Extracted isBetterMatch(...) helper from findClosestTemplateKey to reduce nesting depth
Original prompt
Please apply the following diffs and create a pull request.
Once the PR is ready, give it a title based on the messages of the fixes being applied.

[{"message":"The state variable 'options' is misleading. It doesn't store options but rather the code string to be rendered. Consider renaming to 'debouncedCode' or 'previewCode' to better reflect its purpose.","fixFiles":[{"filePath":"dev/src/Playground.tsx","diff":"diff --git a/dev/src/Playground.tsx b/dev/src/Playground.tsx\n--- a/dev/src/Playground.tsx\n+++ b/dev/src/Playground.tsx\n@@ -30,12 +30,12 @@\n     const saved = getStoredValues<{ code: string }>(STORAGE_KEY);\n     return saved?.code || DEFAULT_CODE;\n   });\n-  const [options, setOptions] = useState(code);\n+  const [previewCode, setPreviewCode] = useState(code);\n \n   // Debounce: update preview 500ms after last edit, also persist\n   useEffect(() => {\n     const timer = setTimeout(() => {\n-      setOptions(code);\n+      setPreviewCode(code);\n       setStoredValues(STORAGE_KEY, { code });\n     }, 500);\n     return () => clearTimeout(timer);\n@@ -86,7 +82,7 @@\n \n       <div style={{ flex: 1, overflow: 'auto' }}>\n         <Card title=\"预览\" size=\"small\" style={{ height: '100%' }}>\n-          <Infographic options={options} />\n+          <Infographic options={previewCode} />\n         </Card>\n       </div>\n     </div>\n"}]},{"message":"The prop name 'options' doesn't clearly communicate that it expects code/syntax string. Consider renaming the prop to 'code' or 'syntax' for better API clarity and consistency with the state variable holding the code.","fixFiles":[{"filePath":"dev/src/Playground.tsx","diff":"diff --git a/dev/src/Playground.tsx b/dev/src/Playground.tsx\n--- a/dev/src/Playground.tsx\n+++ b/dev/src/Playground.tsx\n@@ -30,12 +30,12 @@\n     const saved = getStoredValues<{ code: string }>(STORAGE_KEY);\n     return saved?.code || DEFAULT_CODE;\n   });\n-  const [options, setOptions] = useState(code);\n+  const [previewCode, setPreviewCode] = useState(code);\n \n   // Debounce: update preview 500ms after last edit, also persist\n   useEffect(() => {\n     const timer = setTimeout(() => {\n-      setOptions(code);\n+      setPreviewCode(code);\n       setStoredValues(STORAGE_KEY, { code });\n     }, 500);\n     return () => clearTimeout(timer);\n@@ -86,7 +82,7 @@\n \n       <div style={{ flex: 1, overflow: 'auto' }}>\n         <Card title=\"预览\" size=\"small\" style={{ height: '100%' }}>\n-          <Infographic options={options} />\n+          <Infographic options={previewCode} />\n         </Card>\n       </div>\n     </div>\n"}]},{"message":"The `positionH` and `positionV` values are string literals ('center', 'normal') but their expected types are unclear. Consider using constants or an enum to make these values more explicit and type-safe, preventing potential typos or invalid values.","fixFiles":[{"filePath":"src/designs/structures/chart-line.tsx","diff":"diff --git a/src/designs/structures/chart-line.tsx b/src/designs/structures/chart-line.tsx\n--- a/src/designs/structures/chart-line.tsx\n+++ b/src/designs/structures/chart-line.tsx\n@@ -51,12 +51,15 @@\n   const [paddingTop, paddingRight, paddingBottom, paddingLeft] =\n     parsePadding(padding);\n \n+  const ITEM_POSITION_H = 'center' as const;\n+  const ITEM_POSITION_V = 'normal' as const;\n+\n   const itemProps = {\n     indexes: [0],\n     datum: items[0],\n     data,\n-    positionH: 'center',\n-    positionV: 'normal',\n+    positionH: ITEM_POSITION_H,\n+    positionV: ITEM_POSITION_V,\n   };\n   const sampleBounds = getElementBounds(<Item {...itemProps} />);\n   const labelWidth = sampleBounds.width;\n"}]},{"message":"This hardcoded stop element uses `colorPrimary` instead of the last color from `colorStopsData`. This creates an inconsistency with the gradient definition above (lines 230-236) where the last color from `colorStopsData` is used. The area gradient should use the same color as the stroke gradient for consistency.","fixFiles":[{"filePath":"src/designs/structures/chart-line.tsx","diff":"diff --git a/src/designs/structures/chart-line.tsx b/src/designs/structures/chart-line.tsx\n--- a/src/designs/structures/chart-line.tsx\n+++ b/src/designs/structures/chart-line.tsx\n@@ -396,7 +396,11 @@\n           </linearGradient>\n           <linearGradient id={gradientAreaId} x1=\"0%\" y1=\"0%\" x2=\"100%\" y2=\"0%\">\n             {areaStops}\n-            <stop offset=\"100%\" stopColor={colorPrimary} stopOpacity=\"0.04\" />\n+            <stop\n+              offset=\"100%\"\n+              stopColor={colorStopsData[colorStopsData.length - 1]?.color ?? colorPrimary}\n+              stopOpacity=\"0.04\"\n+            />\n           </linearGradient>\n         </Defs>\n         <Group>{gridElements}</Group>\n"}]},{"message":"The `parseAbsoluteLength` function returns `Number.NaN` for invalid inputs, but the code checks `width > 0` without first verifying that width is not NaN. This could lead to un...

Copilot AI and others added 2 commits April 16, 2026 06:42
- Rename `options` state to `previewCode` in Playground.tsx for clarity
- Add ITEM_POSITION_H/ITEM_POSITION_V constants in chart-line.tsx
- Fix gradient area color to use last colorStopsData color instead of colorPrimary
- Add NaN check for width/height in svg.ts getExportViewBox
- Add braces around single-statement if in embedIcons
- Add empty id guard in collectDefElements
- Replace simple regex CSS escape fallback with full CSS.escape algorithm
- Remove redundant condition in search.ts loadSearchResource
- Use charCodeAt comparison in getCommonPrefixLength for performance
- Extract isBetterMatch helper function in templates/utils.ts

Agent-Logs-Url: https://github.com/antvis/Infographic/sessions/289607cf-6b2e-4c1c-b79d-ec538a1b098c

Co-authored-by: Aarebecca <25787943+Aarebecca@users.noreply.github.com>
…ator precedence

- Move !id guard before visited.add() to prevent empty strings being added
- Add parentheses around range checks in CSS escape fallback for correct precedence

Agent-Logs-Url: https://github.com/antvis/Infographic/sessions/289607cf-6b2e-4c1c-b79d-ec538a1b098c

Co-authored-by: Aarebecca <25787943+Aarebecca@users.noreply.github.com>
@Aarebecca

Copy link
Copy Markdown
Contributor

/gemini review

Copilot AI changed the title [WIP] Rename state variable 'options' to 'previewCode' for clarity refactor: rename misleading variables and apply targeted code quality improvements Apr 16, 2026
Copilot AI requested a review from Aarebecca April 16, 2026 06:47

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors state management in the Playground, enhances SVG export validation, and implements a comprehensive CSS escape fallback. It also optimizes template key matching and removes redundant resource loading logic. Review feedback identifies critical bugs in the CSS escape algorithm regarding leading digits and hyphen-digit sequences, which require hex-based escaping. Additionally, improvements are suggested to enhance performance by hoisting static constants and helper functions, and to remove a redundant SVG stop element.

Comment thread src/exporter/svg.ts Outdated
Comment thread src/exporter/svg.ts Outdated
Comment thread src/designs/structures/chart-line.tsx Outdated
Comment thread src/designs/structures/chart-line.tsx Outdated
Comment thread src/exporter/svg.ts Outdated
@Aarebecca
Aarebecca marked this pull request as ready for review April 16, 2026 08:09
@codecov-commenter

codecov-commenter commented Apr 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.86957% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.55%. Comparing base (43f3150) to head (8bf7c1e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/exporter/svg.ts 78.33% 13 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #236      +/-   ##
==========================================
+ Coverage   46.85%   50.55%   +3.69%     
==========================================
  Files         342      343       +1     
  Lines       28417    28941     +524     
  Branches     2400     2698     +298     
==========================================
+ Hits        13315    14631    +1316     
+ Misses      15089    14297     -792     
  Partials       13       13              
Flag Coverage Δ
infographic 50.55% <85.86%> (+3.69%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/designs/structures/chart-line.tsx 94.64% <100.00%> (+91.25%) ⬆️
src/resource/loaders/search.ts 58.13% <ø> (+3.79%) ⬆️
src/templates/utils.ts 100.00% <100.00%> (ø)
src/exporter/svg.ts 80.30% <78.33%> (-0.39%) ⬇️

... and 19 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Aarebecca
Aarebecca merged commit 0ee1cf3 into main Apr 16, 2026
3 checks passed
@Aarebecca
Aarebecca deleted the copilot/rename-options-to-preview-code branch April 16, 2026 08:31
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.

3 participants