dm-pal/docs/repo-review.md

# DM-Pal — Repo Review & Next Steps (2026-09-06)

*A fresh whole-repo pass: shell, components, Rust backend, scripts, git history,
and the two existing docs. Findings are verified against the code, not copied
from `plan.md` — several items in `plan.md` §17 have since shipped, and a few
claims there are stale (noted below). Each finding ships with a concrete fix.*

**Overall: healthy.** The architecture is clean (self-contained tools behind one
`renderView`, single shared `generate` path with lore injection, two SQLite
stores + a debounced UI-state store), the ponytail discipline is visible
everywhere, and the Rust side has real unit tests. The P0/P1 items from
`ui-ux-improvements.md` have genuinely landed. The biggest problems now are
one P0 functional bug (settings don't survive restart), one UX gap (streaming
is fake), and repo/distribution hygiene (CI, signing, stale docs).

---

## 1. Findings — ranked

### P0-1. LLM config is never persisted to disk — restart wipes Settings

`set_llm_config` (`src-tauri/src/commands/llm_commands.rs`) only mutates an
in-memory `Mutex<LlmConfig>`; `lib.rs` setup always starts from
`LlmConfig::default()`. Nothing writes the config to the prefs store —
`dm-pal-prefs.json` only holds `dataDir`. So **every app restart silently
resets API URL, API key, model, embed model, and image server URL to
defaults**. Worse, the wizard-completed flag lives in `localStorage`, so after
restart the DM isn't even re-prompted — they just get a broken connection.

**Fix (~30 lines):** in `set_llm_config`, `store.set("llmConfig", config)` on
the same `dm-pal-prefs.json` store used for `dataDir` (the store plumbing is
already there in `data_commands.rs`); in `lib.rs` setup, read it back before
`manage(AppState{...})`. The API key lands in plaintext in the OS app-data dir
— same trust level as every other local LLM client; fine for now, note it in
the README's data-locations table.

### P0-2. PII still in `scripts/apple-signing.env.example`

`plan.md` §17.1 marks "Scrub PII" as done. It isn't: the file still contains
the real Apple ID email and Team ID (and both docs quote them). 5-minute fix,
do it before the next push. Note: the values are already in git history from
earlier commits — scrubbing stops future copies, not the past.

### P1-1. "Streaming" is a single fake token — the biggest UX lever left

`generate_stream` does a **non-streaming** call and emits the whole response as
one `Token` (see the comment in `llm_commands.rs`). SessionLogger's "streaming
AI summary" and every generator therefore show 10–20 s of nothing on a local
model. Both transport deps are **already installed**: `reqwest` (`stream`
feature) + `futures-util`. Ollama needs `"stream": true` + NDJSON line
parsing; OpenAI-compatible needs SSE `data:` line parsing. Emit
`LlmEvent::Token` per line; `inject_lore`, busy-balancing, and the frontend
Channel plumbing all stay as-is. SessionLogger's progressive display already
handles multi-token arrival. ~3 h, highest UX-per-line-of-code item in the repo.

### P1-2. RAG: three real issues in the lore pipeline

1. **UTF-8 chunk-drop bug** (`rag/mod.rs::chunk`): long paragraphs are split
   with `para.as_bytes().chunks(MAX_CHUNK_CHARS)` then
   `if let Ok(s) = std::str::from_utf8(chunk)` — when the 1000-byte boundary
   lands mid multibyte character (any accented name, CJK, emoji), `from_utf8`
   errors and the **whole chunk is silently dropped**. Fix: back the split
   index off to a `char_boundary`, e.g. scan back while
   `!chunk.is_char_boundary(i)` (~6 lines). Add a test with a long accented
   paragraph. (Also: `MAX_CHUNK_CHARS` is actually bytes — rename or note.)
2. **Re-adding a source duplicates every chunk**: `add_document` always
   INSERTs. Adding the same file twice doubles its embedding weight and skews
   search. Fix: `DELETE FROM chunks WHERE source = ?` before the insert loop
   (one line).
3. **Changing `embed_model` silently poisons the index**: old-model vectors
   and new-model query vectors are compared by dot product — garbage results.
   Fix options: store the embed model per source and warn/refuse on mismatch
   in `list_sources`, or a "Reindex all" button in the Lore panel (needs
   re-chunk from `text`, which is already stored — feasible).

### P1-3. No CI, and the self-checks aren't aggregated

The repo has `oxlint`, `tsc -b`, three `node scripts/check-*.ts` self-checks,
and `cargo test` — **nothing runs them automatically**. Two-part fix:

1. Add to `package.json`:
   `"check": "node scripts/check-dice.ts && node scripts/check-encounter-budget.ts && node scripts/check-worldmap.ts"`
2. A `.gitea/workflows/ci.yml` (release ships via `gitea-release.sh`, so
   Gitea, not GitHub) running `npm ci`, `npm run lint`, `npm run build`,
   `npm run check`, `cargo test` on push. Even lint-only beats none.

**Gotcha:** the check scripts are bare `.ts` run via Node type-stripping —
that needs **Node ≥ 23.6** (22.6 with `--experimental-strip-types`), but the
README says "Node.js 20+". Bump the README prerequisite or note it.

### P1-4. Version drift + stale docs

- `tauri.conf.json` says `0.1.3`, `Cargo.toml` says `0.1.0`, `package.json`
  says `0.1.3`. Pick one source (tauri.conf is what ships) and have
  `gitea-release.sh` assert they match.
- `README.md` line 21 still says image generation is "(macOS, via Ollama)" —
  the backend moved to stable-diffusion.cpp `sd-server` (cross-platform) and
  the rest of the README says so. Fix the one line.
- `plan.md` §4.4 documents the old Ollama image path + "macOS-only" gating as
  current design; §10 has the same. The ui-ux doc's image section and
  `GeneratedImage` notes reference macOS gating too. One short
  "superseded by sd-server (A1111 API)" banner at the top of each stale
  section beats rewriting them.
- `public/icons.svg` is referenced by nothing (`favicon.svg` is the real icon)
  — delete it, and the stale `dist/` copy with it.

### P2-1. `csp: null` in `tauri.conf.json`

All LLM/image HTTP calls happen in Rust (reqwest), so the WebView needs almost
nothing: a minimal CSP like
`default-src 'self'; img-src 'self' data:; style-src 'self' 'unsafe-inline'; connect-src 'self' ipc: http://ipc.localhost`
closes the standard Tauri warning. Verify against dev-mode HMR before
committing (Vite may need `ws:` under `connect-src` in dev).

### P2-2. `is_ollama()` is URL-substring sniffing

`url.contains("localhost:11434")` breaks the moment someone runs Ollama on a
custom port or behind a proxy (falls into the OpenAI branch and fails
confusingly). Cheapest robust fix: add a `provider: "ollama" | "openai"`
field to `LlmConfig` defaulted by URL sniff, selectable in Settings presets
(the ui-ux doc already prescribes provider presets). Alternatively probe
`/api/tags` then `/v1/models`.

### P2-3. Small nits (batch into one cleanup PR)

- `App.tsx` wizard effect re-checks `localStorage` on **every view change** —
  harmless (modal blocks) but a one-time mount check reads cleaner.
- `HistoryView.tsx` is 806 lines — fine today; split only when the next
  feature lands there, not before.
- `tokio` is pulled with `features = ["full"]`; only the async-runtime
  subset is used. Harmless (Tauri drags tokio in anyway) — trim only if
  compile time bothers you.
- Consider `store.get("llmConfig")` versioning (`configVersion: 1` field) so
  future shape changes don't brick startup — cheap insurance while touching
  P0-1.

---

## 2. Over-engineering audit (ponytail pass)

**Verdict: lean already, ship.** Specifically checked and *kept*:

- Hand-rolled base64 encode/decode in `image_commands.rs` — no stdlib
  equivalent in Rust; avoids a dep; has a roundtrip test. Correct call.
- `src/lib/bus.ts` (59 lines) — used by 3 components
  (Encounter → Initiative/Soundboard push). Not dead.
- `sql_viewer.rs` read-only SQL passthrough — gated power-user tool with
  proper read-only enforcement. Justified.
- `zustand` + `framer-motion` — both actually used (Toast/HistoryView);
  framer-motion is only in `HistoryView`, but replacing working animation
  with CSS is churn, not deletion.

Only real delete: `public/icons.svg` (see P1-4) and the tracked-vs-reality
doc drift above. Net: ~0 lines to remove; the codebase discipline is good.

---

## 3. Verified status of `plan.md` §17 (what actually shipped)

Items marked done there that are **confirmed done in code**: README rewrite,
Greet deletion (marker comment in `lib.rs`), nav-map consolidation
(`TOOL_META` in `App.tsx`), dice/encounter/worldmap self-checks, lore
directory picker (`rag_add_directory` is in the invoke handler), first-run
wizard, `?` shortcut help, global busy indicator (`emit_busy` +
`GeneratingIndicator`), and the sd-server migration (cross-platform image gen,
macOS gate gone from the components).

Still genuinely open from §17/§ui-ux (re-verified): Gitea CI, code signing +
notarization, auto-updater, quest branching graph, world hierarchy tree, real
ambience packs + sound import, campaign namespacing, image-gen into World
Builder (battle maps), handout renderer. Don't re-plan those here — this doc
just re-ranks them against the fresh findings.

---

## 4. Next steps — one ranked list

Merges fresh findings with the still-open roadmap items. Do them roughly in
order; #1–5 are one focused week.

| # | Item | Effort | Impact | Tracked in |
|---|------|--------|--------|------------|
| 1 | Scrub PII from `apple-signing.env.example` | 5 min | High (before next push) | here |
| 2 | Persist `LlmConfig` to prefs store (P0-1) | 1 h | **Critical** | here |
| 3 | RAG: UTF-8 boundary fix + dedupe on re-add (P1-2.1–2) | 2 h | High | here |
| 4 | `npm run check` + Gitea Actions CI (P1-3) | 2 h | High | §17.2 |
| 5 | Real token streaming from Ollama/OpenAI (P1-1) | 3 h | **High** | here |
| 6 | Doc drift cleanup: README line 21, plan §4.4/§10 banners, version sync, delete `icons.svg`, Node ≥ 23.6 note (P1-4) | 1 h | Medium | here |
| 7 | Minimal CSP (P2-1) | 30 min | Medium | here |
| 8 | Provider select instead of URL sniff (P2-2) | 1 h | Medium | here |
| 9 | RAG embed-model mismatch warning / Reindex (P1-2.3) | 2 h | Medium | here |
| 10 | Code signing + notarization (env example + script already exist) | 3 h | High (distribution) | §17.2 |
| 11 | Campaign namespacing (`<dataDir>/<campaign>/`, title-bar selector) | 4 h | High (DMs run 2+ campaigns) | §17.4 |
| 12 | Quest branching graph | 4 h | High | §17.3 |
| 13 | World hierarchy tree (drill into region) | 3 h | High | §17.3 |
| 14 | Auto-updater (`tauri-plugin-updater`) | 3 h | Medium | §17.2 |
| 15 | Image-gen into World Builder maps + Encounter battle maps | 3 h | Medium | §16.10 |
| 16 | Real ambience packs (CC0) + custom sound import | 2 h | Medium | §17.3 |
| 17 | Handout renderer (Markdown → PDF) | 3 h | Low–Med | §15 |

**Skipped deliberately:** i18n, player-facing web view, compendium/SRD stat
blocks, voice-to-text, draggable bento, session replay — all P3 in §17.4 and
correctly deferred. YAGNI until a real user asks.

---

## 5. How to verify the P0 fix in 60 seconds

1. `npm run tauri dev`, set a non-default model in Settings, quit, relaunch.
2. Settings shows the saved model (today: it shows `llama3.2` default).
3. `cat $APPDATA/../dm-pal-prefs.json`-equivalent (the prefs store next to
   `dm-toolkit/`) now has an `llmConfig` key.