docs: repo review + master roadmap
This commit is contained in:
itsamejms
@@ -0,0 +1,211 @@
|
||||
# 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 (`james.twose2711@gmail.com`) and the real Team ID
|
||||
(`4759TW4SDC`). 5-minute fix, do it before the next push.
|
||||
|
||||
### 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.
|
||||
Reference in New Issue
Block a user