Skip to content

Software rendering - #108

Open
kaiemjoy wants to merge 4 commits into
mainfrom
software-rendering
Open

Software rendering#108
kaiemjoy wants to merge 4 commits into
mainfrom
software-rendering

Conversation

@kaiemjoy

Copy link
Copy Markdown
Contributor

No description provided.

… in quotations. this fixed the issue where some dates were being rendered as "January 1970"
added serodynamics software and new software logos
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1-2-g6ad689f

QR code for preview link

🚀 View preview at
https://ucd-serg.github.io/pr-preview/pr-108/

Built to branch gh-pages at 2026-08-24 18:14 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@kaiemjoy
kaiemjoy requested a review from d-morrison August 20, 2026 06:37
@d-morrison

d-morrison commented Aug 24, 2026

Copy link
Copy Markdown
Member

Working on this --- paws off until I'm done.

Re-running the failed claude-review job now that the repo's CLAUDE_CODE_OAUTH_TOKEN secret has been refreshed (updated 2026-08-24T15:59Z). The 2026-08-20 failure was an auth-shaped one: the Claude step returned is_error: true after 362ms, 1 turn, and $0 cost, with no permission denials --- the signature of an expired token rather than a review that ran and got stuck.

Posted by Claude Code on Doug Morrison's behalf.

@d-morrison

d-morrison commented Aug 24, 2026

Copy link
Copy Markdown
Member

Code review (stand-in)

The repo's own claude-review workflow was failing on an expired CLAUDE_CODE_OAUTH_TOKEN; that is now fixed and the check is green. But it still posts no review comment, on this PR or any other, which is tracked in #105. So this review stands in for it rather than leaving the PR effectively unreviewed behind a green check.

Reviewed commit: 132ecb095fdcce0d77fdad1fd038b362c5652b8e. The live preview build at that head was used to verify rendering claims against the actual artifact rather than reasoning about the template.

The core rendering bug is genuinely fixed. On live main, titles render as visible escaped markup (&lt;strong style='font-size:24px;'...&gt;) and the Serocalculator description renders as literal [R](https://www.r-project.org/). In the preview both render correctly, with a working <a href="https://www.r-project.org/">R</a>. Everything below rides alongside that fix.


1. software/serocalculator_logo_copy.png is 1.28 MB for an image displayed at 280 px

software/serocalculator.yml:2. The new PNG is 1,279,012 bytes at 1024x1024, replacing a 364,983-byte JPG. The template renders it inside .software-image-box { height: 280px }, roughly 3.7x more pixels per axis than are ever displayed.

Total image payload on software.html goes from ~738 KB to ~1.71 MB, and this is the single largest asset on the page. loading="lazy" does not help: it is the first card, above the fold, so it is on the critical path for every visitor. On a slow mobile connection the primary lab-software page now takes several extra seconds to paint its hero image.

Downscaling to ~560 px (2x the 280 px box) and re-compressing fixes it. PNG is the right format here, since the alpha channel is genuinely used (alpha is 0 at all four corners and edge midpoints; only 5.6% of sampled pixels are fully opaque), but the file is about 10x larger than the job needs.

The filename is a second, smaller problem: _copy reads as an accidental duplicate rather than a deliberate asset, which will confuse whoever next decides whether it is safe to delete. serocalculator_logo.png would be clearer.

2. software/serodynamics.yml:3 alt text calls a data figure a logo

image-alt: "Serodynamics R package logo", applied to software/fig1-dynamics.png. That file is not a logo. It is a 576x288 scientific plot: a log-scale y-axis labelled "Antibody response (y)", an x-axis "Days since symptom onset (t)" running 0-720, scattered observations with grey subject trajectories, a shaded uncertainty band, a red growth curve, a blue decay curve, and rendered equations for y_growth(t) and y_decay(t).

The preview emits that alt text verbatim, so a screen-reader user is told there is a logo and receives nothing about the antibody-kinetics content that is the entire point of the image. A false description is worse than a missing one, because nothing signals that information was lost.

Worth flagging specifically because this PR fixes the same mistake one card over: serocalculator_image2.jpg was also a data figure carrying image-alt: "Serocalculator R package logo", and swapping in a real logo makes that alt text correct for the first time. The error is then reintroduced on the new card.

Serodynamics ships no logo (man/figures/ holds only lifecycle badges), so using a figure is fine. Something like "Modeled antibody response curve showing rise, peak, and decay over 720 days since symptom onset, with observed subject trajectories" would describe what is actually there.

3. software.qmd:12 sets image-height: 350px, which does nothing

The custom template never reads listing['image-height'] (that is how Quarto's own item-grid.ejs.md consumes it), and the previous template's style="height: 250px;" was likewise hard-coded rather than derived from the YAML. Confirmed against the rendered artifact: the strings 350px and 250px each appear zero times in the preview's software.html. The real height for both listings comes from .software-image-box { height: 280px } at _ejs/software-grid.ejs:5.

So software.qmd now states three heights, none of which is the height: 350px for #software (line 12), 250px for #apps (line 24), and the effective 280px in a third file. The 250 to 350 change in this diff has no effect at all.

The failure this sets up: a maintainer asked to make the logos bigger edits image-height to 450px, rebuilds, sees no change, and concludes Quarto or the cache is broken. The two differing YAML values also imply per-listing control that does not exist, since both listings are pinned to the same 280 px by one global CSS rule.

Either delete both image-height: keys as inert, or teach the template to consume them (height: <%= listing['image-height'] %> on .software-image-box), which would make the per-listing distinction real.

4. .DS_Store is tracked, modified here, and absent from .gitignore

The diff carries a binary .DS_Store hunk unrelated to software rendering. The repo tracks two of them (.DS_Store and figures/.DS_Store, 6148 bytes each), and the root .gitignore has no entry for the pattern.

Every macOS contributor who merely opens the repo root in Finder produces a spurious binary diff, exactly as happened here. Two contributors doing so concurrently produce a binary merge conflict, which git cannot auto-resolve and no reviewer can resolve by inspection: the only recourse is to pick a side arbitrarily.

Dropping the hunk from this PR, then git rm --cached .DS_Store figures/.DS_Store and adding **/.DS_Store to the root .gitignore, closes it permanently.

5. The <style> block is emitted twice on software.qmd

_ejs/software-grid.ejs:3-27. The <style> sits at the top of the template body, and software.qmd uses the same template for both listings (lines 8 and 20). Verified in the preview: grep -c 'software-image-box {' returns 2, once inside #listing-software and again inside #listing-apps.

Beyond the duplicated bytes, this is the structural cause of finding 3. Because the rule is global and emitted per listing rather than scoped per listing, the two listings cannot have different image heights whatever software.qmd says. Moving the CSS to styles.css, where the rest of the site's custom CSS lives, or scoping it per listing id, resolves both.

6. software/serocalculator_image2.jpg becomes an orphan

This PR repoints software/serocalculator.yml away from it. A scan of every .qmd, .yml, .yaml, .ejs, .scss, .css, and .html file at head finds no remaining reference, so 365 KB stays in the repo and keeps being copied into every build output.

Deleting it in this PR is the moment to do it: afterwards nobody can tell whether it is unused or referenced from somewhere they have not looked, which is how orphaned assets become permanent.

7. people/kaiemjoy/.gitignore is out of scope, and fails open everywhere else

Two separate problems.

Scope. A gitignore for an OAuth credential file is unrelated to the listing changes, and will be invisible to anyone reading this PR's title or reviewing the software diff.

Coverage. .httr-oauth is a credential cache that httr/googlesheets4 writes to whatever the working directory happens to be, typically the project root when a .qmd is rendered from the project, not the directory the .qmd lives in. Ignoring it in one person's folder leaves it unignored at the repo root and in every other people/*/ directory. The failure mode is a committed OAuth token, which is exactly what the entry exists to prevent, and the partial fix reads as though the problem is handled.

Nothing is leaked today: no .httr-oauth is currently tracked anywhere in the repo. Moving the pattern to the root .gitignore covers every directory.

8. Retitling the Serodynamics Shiny app breaks its section's naming pattern

software/serodynamics-shiny.yml:4 changes "Serodynamics Shiny App" to "Interactive post-infection serokinetics model". Under the "Shiny Apps" heading the two cards now read "Serocalculator Shiny App" and "Interactive post-infection serokinetics model", one named for its app and one for its method.

It also drops the only textual link between the app and the new Serodynamics package card added directly above it in this same PR, so a reader cannot tell the two are related. Applying the descriptive treatment to both cards, or keeping the package name in it ("Serodynamics: interactive post-infection serokinetics model"), would fix either half.


Candidates considered and dropped

Recorded so their absence is attributable rather than an oversight.

  • <%- item.description %> to <%= item.description %> escaping the new anchors. This was the leading hypothesis and it is wrong. EJS's docs are unambiguous (<%= outputs HTML-escaped, <%- outputs unescaped), and live main shows a <%--interpolated title arriving escaped, both of which pointed at a broken link. The preview build settles it: the anchor renders live. Quarto's pipeline does not behave the way the raw EJS semantics predict here.
  • "seroconversion rates" to "seroincidence rates" (software/serocalculator.yml:5) is an improvement. serocalculator's exported API is est_seroincidence(), est_seroincidence_by(), compare_seroincidence(), so the new wording matches the package's own vocabulary. Its DESCRIPTION still says "seroconversions (infections)", but function names are the stronger signal for user-facing copy.
  • The Serodynamics description checks out against the package's DESCRIPTION ("Modeling Longitudinal Antibody Responses to Infection"). All three linked URLs resolve.
  • Dropping <p class="card-img-top"> and class="thumbnail-image card-img" does not lose Bootstrap's corner rounding; the theme's .card rule carries no border-radius. The removal is the point: .card-img-top>img{object-fit:cover} was cropping the images, and .software-image-box img{object-fit:contain} letterboxes them instead. Deliberate and correct.
  • The logo rendering as a black square. The PNG is RGBA with a fully transparent background, so it composites correctly on both the light and dark Bootstrap themes the site ships.
  • Non-ASCII punctuation. All seven changed text files are ASCII-clean.
  • Missing trailing newline on software/serodynamics.yml. Real, but every sibling software/*.yml already lacks one, so the new file matches the existing convention rather than introducing a deviation.

Verdict: Needs more work

Findings 1-3 are the blocking ones: a 1.28 MB above-the-fold asset, alt text that actively misdescribes a data figure, and a config value that silently does nothing. Findings 4 and 6-7 are cheap to fix in the same round. Finding 5 is the structural fix that makes 3 resolvable properly.

The rendering fix itself is good and verified against the preview build.


Review generated by Claude Code and posted on Doug Morrison's behalf, standing in for the repo's own review workflow while it posts nothing (#105).

@github-actions
github-actions Bot removed the request for review from d-morrison August 24, 2026 18:14
@github-actions

Copy link
Copy Markdown
Contributor

Claude finished reviewView run


The diff at the current head (3b417a3) is byte-identical to the diff at 132ecb0, the commit the most recent review covered — the only changes since then are .github/workflows/* files picked up from a merge of main into the branch, which are outside this PR's diff against main and don't touch any of the reviewed content (.DS_Store, _ejs/software-grid.ejs, people/kaiemjoy/.gitignore, software.qmd, and the software/*.yml/image files). I verified this with git diff 132ecb0 3b417a3 --stat, which shows only the three workflow files changed.

That means there's nothing new to review. The most recent review on this PR — comment #issuecomment-5398108947, posted 2026-08-24T16:20:32Z — already covers this exact diff in depth, citing findings verified directly against the live PR-preview build:

  1. software/serocalculator_logo_copy.png is 1.28 MB for a 280px display box (blocking — page-weight/performance).
  2. software/serodynamics.yml:3 mislabels a scientific figure as a "logo" in alt text (blocking — accessibility).
  3. software.qmd:12's image-height: 350px is inert; the template never reads it (blocking — misleading config).
  4. .DS_Store tracked/modified, no .gitignore entry.
  5. The <style> block in _ejs/software-grid.ejs is emitted twice per page.
  6. software/serocalculator_image2.jpg becomes an orphaned asset.
  7. people/kaiemjoy/.gitignore is out of scope and covers only one directory, not the repo-wide .httr-oauth risk.
  8. Retitling the Serodynamics Shiny app breaks the section's naming pattern and severs its link to the new Serodynamics card.

I independently confirmed these are still present at the current head: software.qmd:12 still sets image-height: 350px, _ejs/software-grid.ejs still declares the <style> block once per template invocation (rendered twice across the two listings), people/kaiemjoy/.gitignore still only ignores .httr-oauth in that one directory, and software/serodynamics-shiny.yml:4 still retitles independently of the new serodynamics.yml card. Nothing in the diff has changed to address any of these since the prior review, so that verdict stands.

Verdict

Needs more work — no new findings; the prior review's assessment (findings 1–3 blocking, 4/6/7 cheap fixes, 5 as the structural fix enabling 3) still applies unchanged to this head.

Reviewed commit: 3b417a3

@github-actions

Copy link
Copy Markdown
Contributor

💰 Cost: $0.3273 (review) — run

@github-actions
github-actions Bot requested a review from d-morrison August 24, 2026 18:15

@d-morrison d-morrison left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

claude reviews are working now: #108 (comment)

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