Skip to content

Upgrade create_pdf action with themes, visual layout, and unicode safety - #286

Merged
zfoong merged 4 commits into
V1.3.2from
pdf_actions
May 28, 2026
Merged

zfoong merged 4 commits into
V1.3.2from
pdf_actions

Conversation

@AlanAAG

@AlanAAG AlanAAG commented May 27, 2026

Copy link
Copy Markdown
Collaborator
  • Replace deprecated FPDF/HTMLMixin pattern with direct write_html on FPDF 2.8
  • Add gradient banner header rendered from the first H1 heading
  • Add 5 colour themes: default, corporate, minimal, warm, forest
  • Add tag_styles for h1-h5 with colour hierarchy, code blocks with background fill, and page number footer
  • Add unicode sanitizer (_CHAR_MAP) to prevent latin-1 crashes on em dashes, smart quotes, currency symbols, and other common characters
  • Add optional subtitle and page_numbers input fields
  • Return pages and size_bytes in success payload for output verification
  • Add parallelizable=False to prevent concurrent file write corruption
  • Remove FPDF from requirement list (was silently skipped by executor heuristic)
  • Add tiered error handling: PermissionError, OSError, and generic Exception with actionable messages instead of a single bare except
  • Auto-create missing parent directories instead of crashing
  • Validate .pdf extension upfront

- Replace deprecated FPDF/HTMLMixin pattern with direct write_html on FPDF 2.8
- Add gradient banner header rendered from the first H1 heading
- Add 5 colour themes: default, corporate, minimal, warm, forest
- Add tag_styles for h1-h5 with colour hierarchy, code blocks with background fill,
  and page number footer
- Add unicode sanitizer (_CHAR_MAP) to prevent latin-1 crashes on em dashes,
  smart quotes, currency symbols, and other common characters
- Add optional subtitle and page_numbers input fields
- Return pages and size_bytes in success payload for output verification
- Add parallelizable=False to prevent concurrent file write corruption
- Remove FPDF from requirement list (was silently skipped by executor heuristic)
- Add tiered error handling: PermissionError, OSError, and generic Exception
  with actionable messages instead of a single bare except
- Auto-create missing parent directories instead of crashing
- Validate .pdf extension upfront
@AlanAAG AlanAAG assigned ahmad-ajmal and unassigned ahmad-ajmal May 27, 2026
@AlanAAG
AlanAAG requested a review from ahmad-ajmal May 27, 2026 00:09
Comment thread app/data/action/create_pdf.py Outdated
Comment thread app/data/action/create_pdf.py

@ahmad-ajmal ahmad-ajmal left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Code is very good and clean. I also tested it and it works perfectly. I just have some suggestions and please make sure to do a lint check too. Goodjob.

ops_done += 1

# ── fill_field (AcroForm via pypdf) ───────────────────────
elif op_type == "fill_field":

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fill_field ops are silently skipped in the main loop and lose per-op accounting on post-process failure

In the main operation loop here, fill_field is a no-op:

elif op_type == "fill_field":
    # Defer all fill_field ops to after PyMuPDF saves
    # (pypdf needs to open the saved file)
    # We flag these for post-processing below
    pass  # handled in post-processing step

Two problems with this:

  1. No ops_done increment here. ops_done only gets bumped inside the post-processing loop , so if a fill_field op succeeds, the count is right - but if the outer post-process try fails wholesale, every fill_field op in the batch is dropped without per-op accounting:

    except Exception as e:
        warnings.append(f"AcroForm fill failed: {type(exc).__name__}: {e}.")

    The caller sees one aggregate "AcroForm fill failed: PdfReadError: …" warning and has no idea which fields were attempted, which succeeded before the failure, or which were skipped.

  2. No early validation in the main loop. A user passing a fill_field op without field_name won't be told about it until post-processing - and if post-processing crashes before reaching that op, the missing-field warning is never emitted at all.

Suggested fix: validate up-front in the main loop, then do per-op try/except inside the post-process loop so each failed field gets its own warning.

# In the main loop — fail fast on shape problems before we even reach pypdf
elif op_type == "fill_field":
    if not str(op.get("field_name", "")).strip():
        warnings.append(f"{op_tag}: 'field_name' is required.")
    # actual fill is deferred — see post-processing block below

# In the post-processing block, replace the outer try/except with per-op:
if acroform_ops:
    try:
        reader = pypdf.PdfReader(abs_output)
        writer = pypdf.PdfWriter()
        writer.append(reader)
        existing_fields = reader.get_fields() or {}
    except Exception as e:
        for j, _ in enumerate(acroform_ops):
            warnings.append(f"op[fill_field {j}]: could not open PDF for AcroForm: {type(e).__name__}: {e}.")
    else:
        for j, op in enumerate(acroform_ops):
            op_tag = f"op[fill_field {j}]"
            field_name = str(op.get("field_name", "")).strip()
            value      = str(op.get("value", ""))
            if not field_name:
                continue  # already warned in main loop
            if field_name not in existing_fields:
                warnings.append(
                    f"{op_tag}: field '{field_name}' not found in AcroForm. "
                    f"Available: {list(existing_fields.keys())[:10]}."
                )
                continue
            try:
                for page_obj in writer.pages:
                    writer.update_page_form_field_values(page_obj, {field_name: value})
                ops_done += 1
            except Exception as e:
                warnings.append(f"{op_tag}: {type(e).__name__}: {e}.")
        try:
            with open(abs_output, "wb") as f:
                writer.write(f)
        except Exception as e:
            warnings.append(f"AcroForm write failed: {type(e).__name__}: {e}.")

Now the caller can distinguish:

  • "couldn't open the PDF at all" (all ops failed with the same upstream cause)

  • "field X wasn't in the form" (that specific op, ops continue)

  • "writing the result to disk failed" (separate from the per-field operations)

  • versus the current behaviour where any of those three failure modes collapses to one opaque aggregate warning.

@ahmad-ajmal ahmad-ajmal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is a small quality of life change - other than that the PR is perfect.

@zfoong
zfoong merged commit ae344ac into V1.3.2 May 28, 2026
@AlanAAG AlanAAG mentioned this pull request Jun 2, 2026
5 tasks done
@ahmad-ajmal
ahmad-ajmal deleted the pdf_actions branch June 10, 2026 04:00
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