feat: add customizable heading styles configuration #116

DemoMacro · 2025-09-21T01:32:41Z

Add heading configuration options to document options with support for:

Custom font family, font size, and formatting for headings 1-6
Configurable paragraph spacing before and after headings
Keep lines and keep next paragraph options
Outline level configuration for document structure
Merge user heading config with default settings

I created issues #115

nicolasiscoding · 2025-09-22T15:04:14Z

Thanks @DemoMacro ! Someone will QA this soon

nicolasiscoding · 2025-09-22T23:39:55Z

PR Review: feat: add customizable heading styles configuration

Overview

This PR adds comprehensive customizable heading styles configuration to the html-to-docx library. The changes allow users to configure font family, font size, formatting, spacing, and outline levels for headings 1-6.

✅ Strengths

Comprehensive Feature: Covers all heading levels (H1-H6) with detailed configuration options
Good Default Values: Provides sensible defaults that follow typical document hierarchy
Backward Compatibility: Changes are additive and don't break existing functionality
Documentation: README.md is updated with clear usage examples
Clean Architecture: Configuration is properly separated and merged with defaults

🔍 Issues & Suggestions

Critical Issues

Code Duplication: The default heading configuration is duplicated between src/constants.js:76-133 and src/schemas/styles.js:11-68. This violates DRY principle and creates maintenance issues.
Inconsistent Font Size Handling: In the default config, heading5.fontSize is set to null, but the XML generation doesn't handle this properly - it will render empty <w:sz w:val="" /> tags.

Code Quality Issues

Template Literal Formatting: The XML generation uses complex template literals with conditional rendering that's hard to read and maintain.
Missing Input Validation: No validation for heading configuration values (e.g., fontSize should be positive, outlineLevel should be 0-5).
Inconsistent Parameter Naming: The function parameter is headingConfig but the property is heading.

Minor Issues

Comments: Inline comments like // use default font are redundant when the value is null.
Spacing Logic: The spacing after logic config.heading1.spacing.after ? \w:after="${config.heading1.spacing.after}"` : ''will fail ifafteris0` (falsy but valid).

🛠️ Recommended Fixes

Remove duplication: Import the default config from constants.js into styles.js
Fix font size handling: Add proper null checks for fontSize after implementing DRY
Fix spacing logic: Use !== undefined instead of truthy checks
Add validation: Validate configuration values
Refactor XML generation: Extract heading style generation into separate functions

📝 Code Suggestions

// Fix spacing logic (line 112+)
<w:spacing w:before="${config.heading1.spacing.before}" ${
  config.heading1.spacing.after !== undefined ? `w:after="${config.heading1.spacing.after}"` : ''
} />

// Fix font size handling
${config.heading1.fontSize !== null ? `<w:sz w:val="${config.heading1.fontSize}" />` : ''}

🎯 Overall Assessment

Status: Needs work before merge

The feature is well-designed and useful, but has several implementation issues that should be addressed:

Critical: Remove code duplication
Critical: Fix null fontSize handling
Important: Improve spacing logic
Nice-to-have: Add input validation

The PR adds valuable functionality but needs refinement to meet production quality standards.

nicolasiscoding · 2025-09-22T23:40:30Z

@DemoMacro - Claude did the above review. The null thing looks wrong to me, but, after doing DRY make sure we handle this properly.

DemoMacro · 2025-09-23T01:36:30Z

@nicolasiscoding Thanks for the thorough review! I've made adjustments in my latest commit.

K-Kumar-01

Nice PR @DemoMacro.
Requested one change

K-Kumar-01 · 2025-09-23T17:24:49Z

src/schemas/styles.js

+	${generateHeadingStyleXML('Heading1', config.heading1)}
+	${generateHeadingStyleXML('Heading2', config.heading2)}
+	${generateHeadingStyleXML('Heading3', config.heading3)}
+	${generateHeadingStyleXML('Heading4', config.heading4)}
+	${generateHeadingStyleXML('Heading5', config.heading5)}
+	${generateHeadingStyleXML('Heading6', config.heading6)}


We should iterate over it using a map instead of writing like this.

Thanks for your feedback, I have changed the content to the mapping method.

nicolasiscoding · 2025-09-26T16:25:11Z

Code Review - Technical Issues Found

Found several technical issues that should be addressed:

🚨 Runtime Error Risk - Missing Null Safety

// src/schemas/styles.js:22-23 - Will crash if heading.spacing is undefined
const spacingAfterXml = heading.spacing.after !== undefined ? `w:after="${heading.spacing.after}"` : '';
const spacingXml = `<w:spacing w:before="${heading.spacing.before}" ${spacingAfterXml} />`;

Fix: Add null/undefined checks:

const spacingXml = heading.spacing 
  ? `<w:spacing w:before="${heading.spacing.before || 0}" ${heading.spacing.after !== undefined ? `w:after="${heading.spacing.after}"` : ''} />`
  : '<w:spacing w:before="0" />';

🔒 XML Injection Vulnerability

// User font names inserted directly into XML without escaping
`<w:rFonts w:ascii="${heading.font}" w:eastAsiaTheme="minorHAnsi" w:hAnsiTheme="minorHAnsi" w:cstheme="minorBidi" />`

Fix: Escape XML special characters in user inputs

⚠️ Invalid fontSize Handling

// This condition fails for fontSize: 0 (valid value)
heading.fontSize && heading.fontSize !== defaultFontSize

Fix:

heading.fontSize !== undefined && heading.fontSize !== defaultFontSize

📝 Type Safety Issues

TypeScript allows font?: string | null but JavaScript doesn't handle null properly
Missing outline level validation (should be 0-5 range)
Consider updating TypeScript types: outlineLevel?: 0 | 1 | 2 | 3 | 4 | 5

Please address the null safety and fontSize issues as they could cause runtime errors.

nicolasiscoding · 2025-09-26T16:25:20Z

Code Review - Technical Issues Found

Found several technical issues that should be addressed:

🚨 Runtime Error Risk - Missing Null Safety

// src/schemas/styles.js:22-23 - Will crash if heading.spacing is undefined
const spacingAfterXml = heading.spacing.after \!== undefined ? `w:after="${heading.spacing.after}"` : '';
const spacingXml = `<w:spacing w:before="${heading.spacing.before}" ${spacingAfterXml} />`;

Fix: Add null/undefined checks for heading.spacing before accessing properties

🔒 XML Injection Vulnerability

// User font names inserted directly into XML without escaping
`<w:rFonts w:ascii="${heading.font}" ...`

Fix: Escape XML special characters in user inputs

⚠️ Invalid fontSize Handling

// This condition fails for fontSize: 0 (valid value)
heading.fontSize && heading.fontSize \!== defaultFontSize

Fix: Use heading.fontSize \!== undefined && heading.fontSize \!== defaultFontSize

📝 Type Safety Issues

TypeScript allows font?: string | null but JavaScript doesn't handle null properly
Missing outline level validation (should be 0-5 range)
Consider updating TypeScript types: outlineLevel?: 0 | 1 | 2 | 3 | 4 | 5

Please address the null safety and fontSize issues as they could cause runtime errors.

nicolasiscoding · 2025-09-26T16:25:40Z

@DemoMacro , kindly rebase against develop and address the final pass of claude code and we should be good to merge and release this. Thank you for your contribution!

nicolasiscoding

See AI Code review feedback, nice work!

nicolasiscoding · 2025-09-29T18:34:37Z

I will test, and if good, we will merge this, then after merging will fix the feedback around:

// src/schemas/styles.js:22-23 - Will crash if heading.spacing is undefined
const spacingAfterXml = heading.spacing.after \!== undefined ? `w:after="${heading.spacing.after}"` : '';
const spacingXml = `<w:spacing w:before="${heading.spacing.before}" ${spacingAfterXml} />`;

K-Kumar-01 · 2025-10-06T18:35:27Z

Hey @DemoMacro
Apologies as your PR got closed by me. I was trying to push some changes but looks like it caused a PR closure. Can you create a new PR handling the below scenario.

I will test, and if good, we will merge this, then after merging will fix the feedback around:

// src/schemas/styles.js:22-23 - Will crash if heading.spacing is undefined
const spacingAfterXml = heading.spacing.after \!== undefined ? `w:after="${heading.spacing.after}"` : '';
const spacingXml = `<w:spacing w:before="${heading.spacing.before}" ${spacingAfterXml} />`;

nicolasiscoding requested review from K-Kumar-01 and nicolasiscoding September 22, 2025 15:07

nicolasiscoding changed the base branch from main to develop September 22, 2025 23:35

DemoMacro force-pushed the main branch from bb056cd to 129c987 Compare September 23, 2025 01:35

DemoMacro force-pushed the main branch from 129c987 to 3ed22d8 Compare September 23, 2025 01:44

K-Kumar-01 requested changes Sep 23, 2025

View reviewed changes

DemoMacro requested a review from K-Kumar-01 September 23, 2025 20:31

nicolasiscoding requested changes Sep 26, 2025

View reviewed changes

DemoMacro requested a review from nicolasiscoding September 29, 2025 02:59

nicolasiscoding assigned DemoMacro, nicolasiscoding and K-Kumar-01 Oct 6, 2025

K-Kumar-01 closed this Oct 6, 2025

K-Kumar-01 force-pushed the main branch from 77bfb16 to 8aa2b4d Compare October 6, 2025 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

feat: add customizable heading styles configuration #116

feat: add customizable heading styles configuration #116

DemoMacro commented Sep 21, 2025 •

edited

Loading

Uh oh!

nicolasiscoding commented Sep 22, 2025

Uh oh!

nicolasiscoding commented Sep 22, 2025

Uh oh!

nicolasiscoding commented Sep 22, 2025

Uh oh!

DemoMacro commented Sep 23, 2025

Uh oh!

K-Kumar-01 left a comment

Uh oh!

K-Kumar-01 Sep 23, 2025

Uh oh!

DemoMacro Sep 23, 2025

Uh oh!

nicolasiscoding commented Sep 26, 2025

Uh oh!

nicolasiscoding commented Sep 26, 2025

Uh oh!

nicolasiscoding commented Sep 26, 2025

Uh oh!

nicolasiscoding left a comment

Uh oh!

nicolasiscoding commented Sep 29, 2025

Uh oh!

K-Kumar-01 commented Oct 6, 2025

Uh oh!

Uh oh!

Uh oh!

feat: add customizable heading styles configuration #116

feat: add customizable heading styles configuration #116

Conversation

DemoMacro commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolasiscoding commented Sep 22, 2025

Uh oh!

nicolasiscoding commented Sep 22, 2025

PR Review: feat: add customizable heading styles configuration

Overview

✅ Strengths

🔍 Issues & Suggestions

Critical Issues

Code Quality Issues

Minor Issues

🛠️ Recommended Fixes

📝 Code Suggestions

🎯 Overall Assessment

Uh oh!

nicolasiscoding commented Sep 22, 2025

Uh oh!

DemoMacro commented Sep 23, 2025

Uh oh!

K-Kumar-01 left a comment

Choose a reason for hiding this comment

Uh oh!

K-Kumar-01 Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

DemoMacro Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

nicolasiscoding commented Sep 26, 2025

Code Review - Technical Issues Found

🚨 Runtime Error Risk - Missing Null Safety

🔒 XML Injection Vulnerability

⚠️ Invalid fontSize Handling

📝 Type Safety Issues

Uh oh!

nicolasiscoding commented Sep 26, 2025

Code Review - Technical Issues Found

🚨 Runtime Error Risk - Missing Null Safety

🔒 XML Injection Vulnerability

⚠️ Invalid fontSize Handling

📝 Type Safety Issues

Uh oh!

nicolasiscoding commented Sep 26, 2025

Uh oh!

nicolasiscoding left a comment

Choose a reason for hiding this comment

Uh oh!

nicolasiscoding commented Sep 29, 2025

Uh oh!

K-Kumar-01 commented Oct 6, 2025

Uh oh!

Uh oh!

DemoMacro commented Sep 21, 2025 •

edited

Loading