Skip to content

spec: shiroa fixes#533

Merged
RobinJadoul merged 4 commits into
spec/mainfrom
spec/shiroa-fixes
Apr 28, 2026
Merged

spec: shiroa fixes#533
RobinJadoul merged 4 commits into
spec/mainfrom
spec/shiroa-fixes

Conversation

@RobinJadoul
Copy link
Copy Markdown
Collaborator

  • Strip raw blocks from chapter titles for project argument
  • Add an explicit description (based on chapter title) to chapters to avoid compilation issues when context appears early in the chapter
  • Export interaction counts from the pdf version to use in shiroa, since otherwise we run into convergence issues that are hard to debug

- Strip raw blocks from chapter titles for `project` argument
- Add an explicit description (based on chapter title) to chapters
  to avoid compilation issues when context appears early in the chapter
- Export interaction counts from the pdf version to use in shiroa,
  since otherwise we run into convergence issues that are hard to debug
@RobinJadoul RobinJadoul self-assigned this Apr 23, 2026
@RobinJadoul RobinJadoul added bug Something isn't working spec Updates and improvements to the spec document labels Apr 23, 2026
@github-actions
Copy link
Copy Markdown

Codex Code Review

Findings:

  • Medium: spec/build_shiroa.sh assumes the current working directory is spec/. If someone runs it as spec/build_shiroa.sh from the repo root, typst query ebook.typ ... will resolve the wrong path, and the matching rm -f interaction_count.json at line 7 can delete/create the file in the caller’s cwd instead of alongside the script. This is a real build regression; the script should cd to its own directory or use script-relative paths.

  • Medium: spec/book.typ:187 makes every Shiroa build depend on an out-of-band generated file, interaction_count.json. The new wrapper script generates it, but nothing in book.typ enforces that path, so any existing workflow that still runs shiroa build directly now fails. Worse, if a stale interaction_count.json is present locally, the site can build with outdated interaction counts silently. This should either be wired into the canonical build entrypoint more defensibly or validated explicitly.

No security issues stood out in this diff. I couldn’t run the Typst/Shiroa build locally because typst and shiroa are not installed in this environment.

Comment thread spec/build_shiroa.sh Outdated
Comment thread spec/book.typ
@claude
Copy link
Copy Markdown

claude Bot commented Apr 23, 2026

Review: spec/shiroa-fixes

This PR fixes three concrete shiroa build problems (raw in titles, missing chapter descriptions, interaction-count convergence). No security issues; no VM/crypto logic touched.

Two minor notes left as inline comments:

  1. build_shiroa.sh - no cleanup of interaction_count.json if shiroa build fails (trap suggestion inline).
  2. book.typ line 187 - json() gives a cryptic file-not-found if shiroa build is run directly without the two-phase script (DX note inline).

Everything else looks good:

  • show ref: none inside xref-include correctly suppresses ref rendering in invisible cross-ref includes.
  • Moving show ref outside project.with() is the right fix so the rule covers the title/description scope too.
  • .flatten() to .sum(default: ()) is the correct Typst API migration.
  • The two-phase design (PDF query to JSON to shiroa) cleanly avoids the convergence issue.
  • set_nr_interactions returning early for shiroa builds is consistent with reading precomputed JSON.

Comment thread spec/book.typ Outdated
Comment thread spec/build_shiroa.sh
typst query ebook.typ '<interaction_count>' --field value > interaction_count.json

# And build
shiroa build
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.

Just as a safety precaution

Suggested change
shiroa build
RAYON_NUM_THREADS=1 shiroa build

Also, this stuff still freezes my PC due to RAM usage :(

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

idk if that should be a default in the build script, vs being set in your local env. It would also make the builds slower when the memory is not a restriction.
I could add a build_shiroa_slow.sh that adds the flag?

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.

Perhaps a better solution would be to allow the number of threads to be passed as a parameter/have a flag to enable multi-threading, where in the script it is documented that this might be a tricky thing to do. And then print a message at start such as

Starting shiroa build on X threads...

when executing to indicate what is happening & suggest to people new to the script that they can multi-thread the process.

Given that (even with THREADS=1) this program often crashes my PC (which has a not unreasonably low 16GB of RAM), I think that we should be cautious to enable multi-threading by default.

RobinJadoul and others added 2 commits April 24, 2026 11:37
Co-authored-by: Erik <159244975+erik-3milabs@users.noreply.github.com>
@RobinJadoul RobinJadoul merged commit 6363f3e into spec/main Apr 28, 2026
2 checks passed
@RobinJadoul RobinJadoul deleted the spec/shiroa-fixes branch April 28, 2026 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working spec Updates and improvements to the spec document

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants