Skip to content

fix(view): inline Semantic UI icon font so dev page icons render#2563

Merged
bpamiri merged 2 commits into
developfrom
claude/busy-dhawan-af9230
May 11, 2026
Merged

fix(view): inline Semantic UI icon font so dev page icons render#2563
bpamiri merged 2 commits into
developfrom
claude/busy-dhawan-af9230

Conversation

@bpamiri
Copy link
Copy Markdown
Collaborator

@bpamiri bpamiri commented May 11, 2026

Summary

  • Closes Icons not rendering correctly in Guides section #2421
  • Fixes broken/empty-box icons on every framework dev page (/wheels/guides, /wheels/info, /wheels/migrator, /wheels/packages, error pages, etc.).
  • Root cause: the framework's dev-page layouts inline semantic.min.css into a <style> block. Semantic UI's CSS expects icon fonts at themes/default/assets/fonts/icons.woff2 relative to where the CSS file lives. With CSS inlined, those URLs resolve against the page URL (e.g. /wheels/guides), and the framework has no static route serving font files — so every <i class="…icon"> rendered as the bordered fallback square seen in the issue screenshot.
  • Fix: in both _header.cfm and _header_simple.cfm, read vendor/wheels/public/assets/css/woff_files/icons.woff2 once at application scope, base64-encode it, and emit a @font-face override after the inlined Semantic CSS. Later @font-face rules for the same font-family win in the CSS cascade.

Before / After

The reporter's screenshot shows empty bordered squares next to Start Here, Core Concepts, etc. on the Wheels Guides have moved page (/wheels/guides).

After this PR, the same page emits a valid @font-face { font-family: 'Icons'; src: url(data:font/woff2;base64,…) } block after semantic.min.css. I verified end-to-end that:

  • the override is present in rendered HTML on /wheels/guides, /wheels/info, /wheels/migrator, /wheels/packages;
  • the base64 decodes to a byte-identical copy of icons.woff2 (40,148 bytes; wOF2 magic);
  • no CFML tags or errors leak into the rendered HTML.

Notes / Decisions

  • Why both header files? _header.cfm is used by normal dev pages; _header_simple.cfm is used by error screens which can render before any "normal" dev page has been hit. The init block is duplicated rather than factored to a shared include to keep the blast radius small.
  • Why not fix semantic.min.css directly? Touching semantic.min.css would (a) drift from upstream and (b) require a separate static-file route to serve the font. Inlining the font matches the framework's existing "single-file dev page" pattern (jQuery, Semantic JS/CSS, marked.js, highlight.js are all <cfinclude>-inlined too).
  • Size impact: ~54 KB of base64 added to each dev page. Negligible against the ~1 MB the page already inlines, and these are dev-only pages.
  • Cross-engine: uses only standard CFML (FileExists, FileReadBinary, ToBase64, ExpandPath, StructKeyExists) — works on Lucee 5/6/7, Adobe CF 2018+, BoxLang.
  • Failure mode: wrapped in try/catch; if the font can't be read for any reason, pages render with the same broken-icon symptom they have today. No new failure modes introduced.

Test plan

  • /wheels/guides icons render (smoke-tested against a Lucee 7 + SQLite server pointed at this worktree).
  • Other affected pages (/wheels/info, /wheels/migrator, /wheels/packages) also receive the override.
  • Base64-decoded font is byte-identical to icons.woff2.
  • No CFML tags / error markers leak in rendered HTML.
  • CI: compat-matrix.yml should remain green (this change uses only baseline CFML built-ins; no engine-specific syntax).

🤖 Generated with Claude Code

The framework dev pages (/wheels/guides, /wheels/info, /wheels/migrator,
/wheels/packages, etc.) inline semantic.min.css into a <style> block.
Its @font-face declarations point at relative URLs like
themes/default/assets/fonts/icons.woff2, which resolve against the
page URL (e.g. /wheels/guides) — there is no static route to serve
them, so every <i class="...icon"> rendered as an empty bordered box.

Read icons.woff2 once at application scope, base64-encode it, and
emit a data-URI @font-face override after the inlined Semantic CSS.
Later @font-face for the same font-family wins in the CSS cascade,
so this overrides the broken declarations without modifying
semantic.min.css. Added in both _header.cfm and _header_simple.cfm
(the latter is used by error pages and may render before any
"full" page has visited).

Closes #2421.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@wheels-bot wheels-bot Bot left a comment

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer A

TL;DR: PR #2563 fixes a real, reproducible visual bug on every framework dev page by inlining the Semantic UI icon font as a base64 data URI. The approach is well-reasoned and consistent with the existing "single-file dev page" pattern. One correctness issue needs addressing before merge: the check-then-act on application scope is not guarded by a cflock, which creates a TOCTOU window that the framework's own codebase explicitly protects against elsewhere. One docs nit (no CHANGELOG entry) is also present.


Correctness

Missing cflock on application-scope check-then-act — both files

_header.cfm lines 16–24 and _header_simple.cfm lines 16–24:

if (!StructKeyExists(application.wheels, "iconsFontDataUri")) {
    local.iconsFontPath = ExpandPath("...");
    application.wheels.iconsFontDataUri = "";    // ← intermediate empty-string state
    if (FileExists(local.iconsFontPath)) {
        try {
            application.wheels.iconsFontDataUri = "data:font/woff2;base64," & ...;
        } catch (any e) {}
    }
}

The StructKeyExists check and the subsequent write are not atomic. The sequence:

  1. Thread A: !StructKeyExists(...)true — enters the block, writes "" to the key
  2. Thread B: !StructKeyExists(...)false — key now exists with value ""
  3. Thread B: Len(application.wheels.iconsFontDataUri)0 — skips @font-face
  4. Thread A: completes the ToBase64 / file read and sets the real value

Thread B renders without the font override during the window of Thread A's file I/O — the exact symptom this PR is fixing.

The framework's own BrowserTest.cfc uses double-checked locking for exactly this pattern (lines 198–210, with the comment "Wrapped in cflock to guard the check-then-act on application scope against parallel invocations"):

if (structKeyExists(application, "$wheelsBrowserLauncher")) {
    return application.$wheelsBrowserLauncher;
}
lock name="wheelsBrowserLauncherInit" type="exclusive" timeout="60" {
    if (structKeyExists(application, "$wheelsBrowserLauncher")) {
        return application.$wheelsBrowserLauncher;
    }
    // ... expensive init ...
}

Apply the same double-checked locking here (in both files):

if (!StructKeyExists(application.wheels, "iconsFontDataUri")) {
    lock name="wheelsIconsFontInit" type="exclusive" timeout="10" {
        if (!StructKeyExists(application.wheels, "iconsFontDataUri")) {
            local.iconsFontPath = ExpandPath("/wheels/public/assets/css/woff_files/icons.woff2");
            local.dataUri = "";
            if (FileExists(local.iconsFontPath)) {
                try {
                    local.dataUri = "data:font/woff2;base64," & ToBase64(FileReadBinary(local.iconsFontPath));
                } catch (any e) {}
            }
            application.wheels.iconsFontDataUri = local.dataUri;
        }
    }
}

Note the additional improvement: build the value in a local variable and assign it to application scope only once, eliminating the intermediate empty-string state entirely.


Docs

No CHANGELOG entryCHANGELOG.md [Unreleased] section has no entry for this fix. The ### Fixed subsection under [Unreleased] (or add one if missing) should include a line like:

- Fix broken icon rendering on dev pages by inlining Semantic UI icon font as a base64 data URI (#2563)

Tests

No automated test coverage. For a visual rendering fix on internal dev-only layout pages, this is acceptable — the test plan documents manual smoke testing on Lucee 7 + SQLite, and a browser spec would be the only way to assert @font-face presence. Not blocking.


Commits

fix(view): inline Semantic UI icon font so dev page icons render — valid type (fix), valid scope (view), subject ≤ 100 chars, not ALL-CAPS. No issues.


Security

The data URI is constructed from ToBase64(FileReadBinary(...)) on a hardcoded filesystem path within vendor/wheels/. Base64 output contains only A-Za-z0-9+/=, so the interpolated value #application.wheels.iconsFontDataUri# inside the <cfoutput> block cannot contain CFML expression delimiters or CSS injection characters. No concern.

@wheels-bot
Copy link
Copy Markdown
Contributor

wheels-bot Bot commented May 11, 2026

Wheels Bot — Reviewer B (round 1)

A's review is solid. The TOCTOU finding is real and well-supported — A verified it against the actual diff, correctly traced the intermediate-empty-string assignment, and pointed to an accurate framework precedent in BrowserTest.cfc (lines 198–210 match exactly). The CHANGELOG nit is also valid. Aligned with A's verdict: two concrete, low-risk changes needed before merge.

Sycophancy

None detected. A found a real correctness issue and requested changes rather than approving.

False positives

None detected.

  • cflock / TOCTOU: Verified against the diff. _header.cfm and _header_simple.cfm both write application.wheels.iconsFontDataUri = "" before the file I/O completes, creating the exact race A described. A concurrent request that passes the StructKeyExists check after the first write but before ToBase64 finishes will read "" and skip the @font-face block. Not fabricated.
  • BrowserTest.cfc reference: Confirmed accurate. Lines 198–210 contain the double-checked locking pattern with the comment A quoted verbatim. Supporting evidence is real.
  • CHANGELOG claim: CHANGELOG.md exists; [Unreleased] ### Fixed subsection is present and ready for an entry.

Missed issues

None detected. A correctly scoped the security analysis (base64 alphabet cannot produce CFML expression delimiters or CSS injection), noted the dev-only context, and correctly assessed test coverage as non-blocking.

Verdict alignment

A's CHANGES_REQUESTED is consistent with the findings: one correctness bug (TOCTOU) and one docs nit, both with concrete fix paths.

Convergence

Aligned with A. Two changes needed: (1) apply the double-checked locking pattern A specified to both _header.cfm and _header_simple.cfm — build the value into a local variable inside an exclusive cflock, assign to application.wheels only once; (2) add a ### Fixed entry to CHANGELOG.md [Unreleased] referencing #2563.

- Wrap icon-font init in double-checked cflock on both _header.cfm and
  _header_simple.cfm. Build the data URI into a local variable inside
  an exclusive lock and assign to application scope only once,
  eliminating the intermediate empty-string state that let a concurrent
  request read "" and skip the @font-face block. Mirrors the pattern
  in BrowserTest.cfc::$ensureLauncher.
- Add CHANGELOG [Unreleased] ### Fixed entry referencing #2563.
@wheels-bot
Copy link
Copy Markdown
Contributor

wheels-bot Bot commented May 11, 2026

Wheels Bot — Address Review (round 1)

Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1):

  • Correctness — cflock on application-scope check-then-act: Wrapped the icon-font initialization in _header.cfm (lines 16–32) and _header_simple.cfm (lines 16–32) with an exclusive lock name="wheelsIconsFontInit". Double-checked the StructKeyExists inside the lock, built the data URI into a local variable, and assigned to application.wheels.iconsFontDataUri only once. The intermediate empty-string write is gone, so a concurrent reader can no longer observe "" while the first thread's file I/O is in flight. Pattern mirrors BrowserTest.cfc::$ensureLauncher() (vendor/wheels/wheelstest/BrowserTest.cfc:198–227).
  • Docs — CHANGELOG entry: Added a line under [Unreleased] ### Fixed in CHANGELOG.md:155 summarizing the icon-font fix and referencing fix(view): inline Semantic UI icon font so dev page icons render #2563.

No findings were skipped — both items in the converged-changes verdict were addressed.

Tests were not re-run: Reviewer A explicitly assessed test coverage as non-blocking for this visual rendering fix on internal dev-only layout pages, and there are no specs targeting vendor/wheels/public/layout/*.cfm. The change is surgical (existing logic re-wrapped, no new code paths) and uses only baseline CFML built-ins.

The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on `approve` or the outer-loop cap (5 rounds) is reached.

@github-actions github-actions Bot added the docs label May 11, 2026
Copy link
Copy Markdown
Contributor

@wheels-bot wheels-bot Bot left a comment

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer A

TL;DR: This is the round-1 follow-up to the original review on SHA 7ea529928. Both findings have been addressed: double-checked locking is now in place in both header files, and a CHANGELOG entry has been added. The implementation is correct, cross-engine safe, and the TOCTOU window described in the prior review is fully closed. Approving with one minor commit-message style note.


Correctness

The prior finding (TOCTOU race on application.wheels.iconsFontDataUri) is resolved. The pattern in both _header.cfm (lines 25–38) and _header_simple.cfm (lines 18–31) is now correct double-checked locking:

if (!StructKeyExists(application.wheels, "iconsFontDataUri")) {
    lock name="wheelsIconsFontInit" type="exclusive" timeout="10" {
        if (!StructKeyExists(application.wheels, "iconsFontDataUri")) {
            local.dataUri = "";
            if (FileExists(local.iconsFontPath)) {
                try { local.dataUri = "data:font/woff2;base64," & ToBase64(...); } catch (any e) {}
            }
            application.wheels.iconsFontDataUri = local.dataUri;
        }
    }
}

Value is built into local.dataUri and assigned to application scope exactly once — no intermediate empty-string state is visible to other threads. The outer if is an unsynchronised fast-path read; that is safe because it only causes a thread to skip the lock block when the key already exists with its final value.

The Len(application.wheels.iconsFontDataUri) read at _header.cfm:159 and _header_simple.cfm:44 is also safe: both CFM files execute sequentially, so the init block always completes before the output section runs.


Docs

CHANGELOG [Unreleased] entry is present and clearly explains the root cause, fix, and affected pages. No remaining issue here.


Commits

fix(view): address Reviewer A/B consensus findings (round 1) passes commitlint (valid type, valid scope, ≤ 100 chars, not ALL-CAPS). Minor style note: the subject describes the process that triggered the commit ("address reviewer findings") rather than the actual technical change. A subject like fix(view): guard icon font init with double-checked locking would make the commit history self-explanatory without reference to the review thread. Not a blocker.


Cross-engine

cflock is a base CFML tag present in Lucee 5/6/7, Adobe CF 2018+, and BoxLang. No cross-engine concern introduced.


Security

Unchanged from the original analysis: ToBase64(FileReadBinary(...)) on a hardcoded path within vendor/wheels/ produces only A-Za-z0-9+/= output. The interpolated value at _header.cfm:162 and _header_simple.cfm:47 cannot contain CFML expression delimiters or CSS injection characters. No concern.

@wheels-bot
Copy link
Copy Markdown
Contributor

wheels-bot Bot commented May 11, 2026

Wheels Bot — Reviewer B (round 1)

A's review is accurate and appropriately scoped to the round-1 changes. The TOCTOU fix is verified correctly, the CHANGELOG claim is real, and the commit-message style note is valid. Aligned with A's approve verdict.

Sycophancy

None detected. A approved based on concrete verification of the double-checked locking pattern, not on general impressions.

False positives

None detected.

  • Double-checked locking description: A's code snippet matches the actual diff exactly — local.dataUri built inside the lock, assigned to application.wheels.iconsFontDataUri only once, no intermediate empty-string state visible to other threads. Claim is accurate.
  • Outer fast-path read safety: A's analysis is correct. The unsynchronized outer if (!StructKeyExists(...)) is safe because once the key exists it is never removed or reassigned; a thread that passes the outer check and is then preempted will re-check inside the lock.
  • CHANGELOG entry: Present in the diff at the expected location under [Unreleased] ### Fixed. Claim is accurate.
  • Commit message style note: fix(view): address Reviewer A/B consensus findings (round 1) passes commitlint (valid type, valid scope, under 100 chars, not ALL-CAPS). A's note that the subject describes the bot process rather than the technical change is correct but non-blocking. Accurate.

Missed issues

None detected. A correctly assessed security (base64 alphabet cannot produce CFML delimiters or CSS injection characters), cross-engine (cflock is standard CFML across all supported engines), and test coverage (non-blocking for a visual rendering fix on internal dev-only pages with no existing specs targeting these templates).

One non-blocking observation A did not raise: in _header_simple.cfm, the new StructKeyExists(application.wheels, "iconsFontDataUri") call has no guard for application.wheels being undefined — a concern specific to error pages that fire before onApplicationStart completes. However, the original file already accesses application.wheels without this guard in other places, so the PR does not introduce a new failure mode. Not a missed issue; noting for completeness.

Verdict alignment

A's APPROVED verdict is consistent with the findings: the two prior CHANGES_REQUESTED items (TOCTOU and CHANGELOG) are both resolved with no new issues introduced.

Convergence

Aligned with A. The TOCTOU fix is correct, the CHANGELOG entry is present, and the only remaining item is a non-blocking commit-message style note that does not warrant another change cycle. Recommending approve.

@bpamiri bpamiri merged commit f6c2fbc into develop May 11, 2026
6 checks passed
@bpamiri bpamiri deleted the claude/busy-dhawan-af9230 branch May 11, 2026 05:52
bpamiri added a commit that referenced this pull request May 11, 2026
…r view

Adds vendor/wheels/tests/specs/migrator/MigratorViewIconsSpec.cfc that
asserts (1) the migrator view's Database Error placeholder uses an
inline SVG icon, and (2) the file contains no Semantic UI icon-font
tags. Documents the inline-SVG convention used throughout
vendor/wheels/public/views/migrator.cfm and guards against drift.

The original bug (broken icon glyph on Database Error placeholder,
#2427) was fixed by:
- #2562 - replaced the offending icon-font tag with an inline SVG
  matching the sibling "No migration files found" placeholder.
- #2563 - inlined the Semantic UI icon font globally so font-based
  glyphs render across all framework dev pages.

This test is convention enforcement - it catches reintroduction of
icon-font tags in this file even though the font now loads.

Closes #2427.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bpamiri added a commit that referenced this pull request May 11, 2026
…r view (#2565)

Adds vendor/wheels/tests/specs/migrator/MigratorViewIconsSpec.cfc that
asserts (1) the migrator view's Database Error placeholder uses an
inline SVG icon, and (2) the file contains no Semantic UI icon-font
tags. Documents the inline-SVG convention used throughout
vendor/wheels/public/views/migrator.cfm and guards against drift.

The original bug (broken icon glyph on Database Error placeholder,
#2427) was fixed by:
- #2562 - replaced the offending icon-font tag with an inline SVG
  matching the sibling "No migration files found" placeholder.
- #2563 - inlined the Semantic UI icon font globally so font-based
  glyphs render across all framework dev pages.

This test is convention enforcement - it catches reintroduction of
icon-font tags in this file even though the font now loads.

Closes #2427.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bpamiri bpamiri mentioned this pull request May 12, 2026
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Icons not rendering correctly in Guides section

1 participant