Add a synthetic ZIM landing page when root index is missing - #75
Add a synthetic ZIM landing page when root index is missing#75SihanTeng wants to merge 2 commits into
Conversation
# Conflicts: # CHANGELOG.md # docs/content/reference/release-notes.md
tamnd
left a comment
tamnd
left a comment
There was a problem hiding this comment.
Good diagnosis and the right shape for a fix. Synthesising the landing page at pack time is better than special casing Kiwix, and I like that it works for every reader.
Two things block it for me, both small:
- The trigger fires on single page mirrors, so
kage clone --max-pages 1 && kage packnow opens a stub with one link on it. That is literally the command in issue #62 and it is the most common small workflow we have. - Reassigning
mainsilently changes the archive title metadata from the article's real title to the bare host name.
Third thing I would like while you are in the file: list the page titles rather than the raw paths. You already compute them a few lines up and throw them away, and for someone picking an article by name that is the whole point of the feature.
For context on why the mirror and the archive differ here: kage serve is a bare http.FileServer, so its index is a freebie from the standard library rather than something kage creates. Worth having kage own that page on both paths eventually.
| // articles cloned into the same host), kage serve still offers a directory | ||
| // listing. A ZIM has no such listing, so synthesise an index that links | ||
| // every HTML page and use it as the main page (issue #62). | ||
| if main != "index.html" && len(htmlPages) > 0 { |
There was a problem hiding this comment.
len(htmlPages) > 0 is too loose. Take the most common small workflow:
kage clone https://example.com/some/article --max-pages 1
kage pack example.com
One page, at some/article/index.html, no root index.html, so this branch fires and replaces a perfectly good main page with a generated index whose entire content is one link. The user now has to click through a stub to reach the only thing in the archive. The same happens to any mirror where every page lives under a common prefix, which is exactly what --scope-prefix produces.
Please make it len(htmlPages) > 1. With a single page pickMainPage is not being arbitrary, it is picking the only option there is.
| listing := syntheticIndexHTML(htmlPages, firstNonEmpty(opts.Title, filepath.Base(mirrorDir))) | ||
| w.AddContent(zim.NamespaceContent, "index.html", firstNonEmpty(opts.Title, filepath.Base(mirrorDir)), "text/html", listing) | ||
| counts["text/html"]++ | ||
| main = "index.html" |
There was a problem hiding this comment.
This line has a side effect that is not obvious from here. Further down, buildWriter does:
title := firstNonEmpty(opts.Title, htmlTitleOf(mirrorDir, main), host)main is now "index.html", which does not exist on disk, so htmlTitleOf fails its os.ReadFile and returns "", and the title falls through to host.
Concretely: packing a single article without --title used to produce an archive titled "The 1990s - Wikipedia" and now produces one titled "en.wikipedia.org". That is the string Kiwix shows in its library list, so every archive built without an explicit title gets noticeably worse.
Fix is to capture the real main page before overriding it:
realMain := pickMainPage(htmlPages)
...
title := firstNonEmpty(opts.Title, htmlTitleOf(mirrorDir, realMain), host)The new test passes Title: "Wiki picks" explicitly, so it never exercises this path. Please add a case that packs without a title and asserts the metadata.
| b.WriteString(`<li><a href="`) | ||
| b.WriteString(htmlEscape(p)) | ||
| b.WriteString(`">`) | ||
| b.WriteString(htmlEscape(p)) |
There was a problem hiding this comment.
The reader gets a list of file paths here. But the walk in buildWriter already computes each page's real title with htmlTitleOfBytes(data) and then discards it.
Carrying a map[string]string or a small []pageEntry{path, title} out of the walk turns this from a file listing into a table of contents:
<li><a href="wiki/1990s/index.html">The 1990s - Wikipedia</a></li>Sorting by title instead of path follows naturally. The doc comment on htmlTitleOfBytes says its whole purpose is "so a ZIM reader shows the page's real title instead of its URL path", and this would be the one place in the codebase doing the opposite. Given the point of the PR is a browsable landing page, I think this is the difference between solving #62 and half solving it.
| b.WriteString("<h1>") | ||
| b.WriteString(htmlEscape(title)) | ||
| b.WriteString("</h1><ul>\n") | ||
| for _, p := range sorted { |
There was a problem hiding this comment.
No bound on this loop. A mirror with 50,000 pages produces one <ul> with 50,000 <li> in it. It will compress fine inside the cluster but it is unusable as a landing page and slow to render in Kiwix on a phone.
Any of these is fine with me: cap at N with a "and 49,900 more" note, group by top level directory with <details>, or only synthesise below some threshold and fall back to the current behaviour above it. I would just rather not ship the unbounded version.
| } | ||
|
|
||
| // htmlEscape escapes text for inclusion in HTML text and attribute values. | ||
| func htmlEscape(s string) string { |
There was a problem hiding this comment.
This duplicates html.EscapeString, and pack/zim.go already imports golang.org/x/net/html. Please use the stdlib one.
Two reasons beyond taste: this output is HTML a user will open, so it is not the place for a hand rolled escaper, and strings.NewReplacer allocates on every call here.
| title = "Offline mirror" | ||
| } | ||
| var b strings.Builder | ||
| b.WriteString("<!doctype html>\n<html><head>") |
There was a problem hiding this comment.
Small thing worth saying out loud: this generated page has a doctype, a charset, and a viewport meta, which makes it the only HTML kage produces that is not in quirks mode. Every page we save from Chrome loses its <!DOCTYPE html> because page.HTML() returns the outerHTML of <html> and the doctype is a sibling of that element.
Not your problem in this PR, I am fixing it separately under #16, but you did the right thing here.
2 participants
Summary
index.html, synthesize a simple directory-index landing page and use it as the ZIM main page.kage servealready offers for the same layout.Why
Opening a multi-page host without a homepage in Kiwix/
kage openjumped to an arbitrary first page (#62).Test plan
TestSyntheticIndexWhenNoRootIndexinpackgo test -short -count=1 ./pack/Closes #62