# Post-change web application review — darcsweb
Scope: the uncommitted darcs diff (`/tmp/darcs-diff.txt`) that touches
`app/Main.hs`, `src/DarcsWeb/Config.hs`, `src/DarcsWeb/Darcs.hs`,
`src/DarcsWeb/Html.hs`, `test/Properties/Clone.hs`, and
`test/Properties/Html.hs`. Review focuses on mobile-first design,
consistent behavior, and frontend performance; `static/style.css` was
consulted only where a Haskell-side change has CSS implications.
High-level verification
- Semantic landmarks: the swap from `<div class="page-header/page-body/page-footer">`
to `<header>/<main>/<footer>` at
`src/DarcsWeb/Html.hs:41,47,50` and the `<div class="log-entry">` →
`<article class="log-entry">` swap at `src/DarcsWeb/Html.hs:208,226`
are class-preserving and do not break the existing CSS (all selectors
target the classes, not the tag).
- Percent-encoded hrefs: every `esc repoName` / `esc (psHash ps)` used
inside an `href=` has been replaced with
`encodePathSegment (...)` (see calls at `src/DarcsWeb/Html.hs:83,101,
171-172,205-206,232,285-286,321,340,364,368,391,440,455`).
Scotty's regex routes read from WAI's `pathInfo`, which is already
percent-decoded, so a repo name or directory with `%`, `#`, `?`,
space or UTF-8 that was previously a broken link now routes back
cleanly. `pathSegments` at `src/DarcsWeb/Html.hs:357` guards
`init []` crashes in the blob breadcrumb for a single-segment path.
- Favicon: `<link rel="icon" type="image/png" href="/static/darcs-logo.png">`
at `src/DarcsWeb/Html.hs:38` is correct; the file exists in
`static/darcs-logo.png` and the MIME table in
`app/Main.hs` (`.png` → `image/png`) matches the `type=`.
- Top-level `<h1>` on the index: `src/DarcsWeb/Html.hs:62` renders the
configured title once per index page, giving every route a single
heading landmark. Escaping uses `esc`, which is correct.
- `aria-current="page"`: `navLink` at `src/DarcsWeb/Html.hs:453-458`
emits `aria-current="page"` only in the `path == active` branch, and
every caller of `repoNavBar` passes a section name that matches one
of the five emitted links (`src/DarcsWeb/Html.hs:108,152,196,267,
308,423`). Patch detail deliberately omits the nav (pre-existing).
---
## Consistency findings
### C1. Empty breadcrumb `<nav aria-label="Breadcrumb">` rendered on index and 404
- Severity: low
- File and line: `src/DarcsWeb/Html.hs:45`, called with `breadcrumbs = ""`
from `src/DarcsWeb/Html.hs:60` (index) and
`src/DarcsWeb/Html.hs:434` (404).
- Issue: `renderPage` always emits
`<nav class="breadcrumbs" aria-label="Breadcrumb">…</nav>` even when
`breadcrumbs` is empty. On the index page and on 404 the nav is
empty, but the landmark is present in the accessibility tree because
`aria-label` forces it to be exposed regardless of content.
- Why it matters for users: screen-reader users landing on the index
or a 404 will hear a "Breadcrumb navigation" landmark announced with
zero items. The same pages on other routes do have crumb entries, so
the landmark set is inconsistent across pages.
- Practical fix: in `renderPage`, only emit the breadcrumb `<nav>`
when `breadcrumbs` is non-empty, e.g.
`if T.null breadcrumbs then "" else "<nav class=…>" <> breadcrumbs <> "</nav>\n"`.
No CSS change needed.
### C2. Breadcrumb contents start with a leading `" / "`
- Severity: low
- File and line: `src/DarcsWeb/Html.hs:102, 149, 193, 233, 264, 305,
420` (every non-index page) and `src/DarcsWeb/Html.hs:440`
(`repoBreadcrumb`).
- Issue: every crumb string begins with `" / <a …>"`. Previously this
sat inside a `<nav class="breadcrumbs">` with no `aria-label`; now
`aria-label="Breadcrumb"` promotes it to a first-class landmark, so
the leading bare slash is the first thing a screen reader reads —
"Breadcrumb navigation, slash, repo-name, link".
- Why it matters for users: the inconsistency between the visible
separator (`" / "`) and the semantic meaning of the label worsens
now that the landmark is explicit. Also diverges from how
`renderTreeBreadcrumb` builds crumbs (no leading slash, anchored by
a `[root]` link).
- Practical fix: have `repoBreadcrumb` return an unprefixed crumb
(`<a href=…>repo</a>`) and let callers prepend their own separator
only when chaining further segments; or render breadcrumbs as an
ordered list (`<ol>`) and drop the inline separators entirely.
### C3. Tree breadcrumb ends with a trailing `" / "` when at the subtree root entry
- Severity: low
- File and line: `src/DarcsWeb/Html.hs:305` and
`src/DarcsWeb/Html.hs:318-332`.
- Issue: `renderTree` composes a header-bar breadcrumb by concatenating
`repoBreadcrumb repoName <> " / tree" <> pathSuffix` where
`pathSuffix` is `" / " <> esc subPath`. The in-body tree breadcrumb
(the `.tree-path` rendered by `renderTreeBreadcrumb`) is built
independently from `pathSegments` and emits a distinct trail. Both
now live in the same page, each with different escaping rules
(header bar uses raw `esc subPath`, body uses per-segment
percent-encoded hrefs), which is visually and semantically
redundant.
- Why it matters for users: a mobile user with a narrow viewport sees
the same path fragment rendered twice in two different styles, the
top one wrapping inside the sticky header, the bottom one scrolling
horizontally. Tapping them produces the same destinations, but the
URLs built by each branch are built by different code paths and
can drift.
- Practical fix: drive both from `pathSegments` + `encodePathList` so
that the two trails cannot disagree, or drop the in-body
`.tree-path` when the header-bar breadcrumb already carries the
subpath.
### C4. Index `<h1>` duplicates the value in `<title>` but tag nav is hidden on index
- Severity: low (inferred)
- File and line: `src/DarcsWeb/Html.hs:62,67`.
- Issue: the index page now renders `<h1>siteTitle</h1>` immediately
followed by the repo table. There is no `repoNavBar` on the index
(correct: no repo), but because the header `<nav>` becomes empty
too (see C1) the index has exactly one nav landmark (the
still-emitted empty one), while per-repo pages have three (header
breadcrumbs, `repo-nav`, footer-less) — a consistency gap worth
collapsing once C1 is fixed.
- Why it matters for users: screen-reader "list landmarks" / "list
navigations" output changes shape when moving between the index and
any repo page.
- Practical fix: combined with C1, skip the empty breadcrumb nav on
the index and 404 to keep the landmark set stable.
---
## Mobile-first / design findings
### D1. `<article class="log-entry">` inherits the desktop `box-shadow` hover rule on touch devices
- Severity: low
- File and line: `src/DarcsWeb/Html.hs:208` (tag change);
rule in `static/style.css` lines 474-486.
- Issue: the swap from `div` to `<article>` does not break the
selector (both are class-matched), but the `.log-entry:hover` rule
is now attached to a block that represents a full landmark in the
page. On mobile Safari / Chrome the `:hover` state sticks after the
first tap until the next tap elsewhere, so "hovered" cards linger
with `box-shadow: var(--shadow-md)` after navigation.
- Why it matters for users: phantom hover shadows accumulate on
scrolling lists of patches; this was the same on desktop, but
`<article>` is more likely to be interactive-looking, so users will
tap directly on the card rather than the inner link.
- Practical fix: gate the hover shadow to pointer devices, e.g.
wrap the `:hover` rule in `@media (hover: hover)`. CSS-only change,
not part of this review's scope, but flagged because the Haskell
change makes the card feel more clickable.
### D2. `<header class="page-header">` stays sticky while the breadcrumb `<nav>` can wrap to a second line
- Severity: low (inferred)
- File and line: `src/DarcsWeb/Html.hs:41-46` combined with
`static/style.css` lines 184-196 (`position: sticky; top: 0;`,
`flex-wrap: wrap`).
- Issue: on very long breadcrumb trails (deep tree subpath) the
breadcrumb `<nav>` wraps, making the sticky `<header>` grow to two
or three lines and eat a large share of a phone viewport. The
previous code had the same behavior, but the `<header>` landmark
and `aria-label="Breadcrumb"` promote the region to a more
prominent role, so the vertical cost is more visible.
- Why it matters for users: on a 360×640 viewport a three-line
sticky header can consume ~30% of vertical space above the fold.
- Practical fix: cap the breadcrumb height with
`max-height` + `overflow-x: auto` on the nav so it scrolls
horizontally instead of wrapping vertically. CSS-only; flagging
only because the diff touches this layout.
---
## Performance findings
### P1. `renderTreeBreadcrumb` / `renderBlobBreadcrumb` rebuild prefix lists in O(n²)
- Severity: low
- File and line: `src/DarcsWeb/Html.hs:322-323, 344`.
- Issue: `prefixes = drop 1 (scanl appendSeg [] parts)` where
`appendSeg acc p = acc ++ [p]` rebuilds the prefix list with
list-append at every step, so the total work is O(depth²) in the
number of segments, and each prefix is re-percent-encoded from
scratch by `encodePathList`.
- Why it matters for users: negligible for typical repo depths (≤10
segments), but rendering a tree page for a deep path walks this
cost twice (once for the header-bar crumb via `pathSuffix`, once in
the `.tree-path` body). It is wasted server CPU on every tree hit.
- Practical fix: build hrefs incrementally by threading the
percent-encoded accumulator through a single `foldl`:
`scanl (\acc seg -> acc <> "/" <> encodePathSegment seg) "" parts`.
O(depth) time, one pass, and removes the intermediate `[[Text]]`
allocation.
### P2. `encodePathSegment` is re-applied to the same repo name on every row of every table
- Severity: low
- File and line: `src/DarcsWeb/Html.hs:83,86,92,93` (repo row),
`src/DarcsWeb/Html.hs:171,178,185` (shortlog row),
`src/DarcsWeb/Html.hs:205,211` (full-log row),
`src/DarcsWeb/Html.hs:285,290,296` (tag row),
`src/DarcsWeb/Html.hs:368,377,391,395,396` (tree row).
- Issue: each `render*Row` re-runs `encodePathSegment repoName`
(byte-by-byte UTF-8 encode + hex escape) for every table row. A
tree with 500 entries pays this cost 500 times, even though
`repoName` never changes across a single render call.
- Why it matters for users: for small repos negligible, but a large
shortlog (thousands of patches) spends measurable time
re-encoding the same segment. It also materialises N copies of
the same `Text` via `T.concat` / `T.pack`.
- Practical fix: precompute `nameSeg` once in the caller
(`renderRepoTable`, `renderShortLogTable`, `renderFullLog`,
`renderTagList`, `renderTreeTable`) and pass it into the per-row
renderer. Same applies to the `"/repo/" <> nameSeg <> "/…"`
prefixes, which can be baked once per table.
### P3. `renderFullLog` still calls `renderFullLogEntry` with per-entry `encodePathSegment repoName`
- Severity: low
- File and line: `src/DarcsWeb/Html.hs:199,205`.
- Issue: same pattern as P2, but in the full-log path where the
inner `<pre>` already dominates CPU. Not a hot path, but the
redundant allocation is trivially avoidable.
- Why it matters for users: mostly GC pressure. Mobile browsers are
not affected; server-side throughput is.
- Practical fix: same as P2 — hoist `nameSeg` to `renderFullLog` and
pass it in.
---
## Areas worth testing on real mobile devices
- A sticky-header summary page on a 360×640 viewport with a very
long repository name — verify the breadcrumb wraps without
pushing the `<main>` below the fold.
- Tap-hold / long-press on `<article class="log-entry">` on iOS
Safari; confirm the hover shadow does not linger after scroll
(D1).
- Tree view with a directory whose name contains `#`, `?`, `%`,
space, or non-ASCII (e.g. `café`). Previously the browser
auto-encoded these; now they are pre-encoded server-side.
Navigating, pressing "back", and copying the URL should all
round-trip identically.
- `aria-current="page"` announcement on VoiceOver / TalkBack for
the active tab in `.repo-nav`, both when landing on the section
directly and after client-side back/forward.
## Performance paths worth measuring
- Rendering time of `/repo/:name/tree/<deep>` for a path with
~20 segments — verify the O(depth²) crumb build (P1) is not
visible in a flamegraph.
- Shortlog / full-log render time on a repo with many patches
(thousands) — verify that hoisting `nameSeg` per table (P2/P3)
actually reduces allocation.
- Favicon fetch: the linked `/static/darcs-logo.png` is a 10 KiB
PNG served with `Cache-Control: public, max-age=86400, immutable`
(`app/Main.hs:387`). On a cold cache a mobile client downloads
10 KiB for what browsers typically display at 16×16 — worth
measuring LCP impact and considering an optimized 16×16 /
32×32 `.ico` or inline SVG. The diff only adds the `<link>`;
the asset itself is unchanged.
## Consistency risks that could not be fully verified from code alone
- Whether Scotty 0.22's regex routes apply `pathInfo` (decoded)
consistently in all versions installed via `stack.yaml` — the
percent-encoding round-trip assumes that. Claim supported by
code inspection but not by a runtime test.
- Whether all previously-emitted raw hrefs (escaped via `esc`) are
now routed through `encodePathSegment`. I checked every
`href="/repo/` / `href="/clone/` site in `Html.hs`; a grep for
`esc repoName` inside `href=` in the module returns no results
(only inside visible anchor text). Verified.
- Whether existing bookmarks that used unencoded URLs (i.e., with
a literal space or `#`) will still work. They will not: the
server never decoded them correctly before either, so those
bookmarks were already broken — but this is worth mentioning in
release notes.
