Skip to content

feat(ui): add lore entity CRUD frontend - #18

Closed
KareemN1936 wants to merge 1 commit into
m2-worlds-frontend-integration-kareemfrom
feature/m2-lore-entities-crud-frontend-kareem
Closed

KareemN1936 wants to merge 1 commit into
m2-worlds-frontend-integration-kareemfrom
feature/m2-lore-entities-crud-frontend-kareem

Conversation

@KareemN1936

Copy link
Copy Markdown
Collaborator

Summary

  • Adds frontend lore entity CRUD surfaces for the M2 card.
  • Implements the 6 Trello card types: Character, City/Kingdom, Faction, Quest, Historical Event, Story/Contribution.
  • Replaces the contribution placeholder with a structured create/edit lore form.
  • Replaces the lore detail placeholder with a readable entity page.
  • Adds world hub lore entity tabs and a typed frontend API wrapper for expected /worlds/:worldId/lore endpoints.

Validation

  • npm run typecheck --workspace @hivelore/web
  • npm run lint --workspace @hivelore/web
  • npm run build --workspace @hivelore/web

Notes

Frontend is wired to expected lore CRUD endpoints. Current remotes show LoreEntry schema/foundation, but I do not see exposed backend CRUD routes yet.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

Vercel preview is ready:

https://hivelore-orm8efqbh-kareemn1936s-projects.vercel.app

Commit: 5eb40d2

@mcfarhat

mcfarhat commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

@KareemN1936 the frontend craft here is good — clean component split, no mocks, no secrets, no hardcoded URLs, correct race handling in the fetch effect (the isActive flag), and you disclosed the missing backend in the PR body rather than hiding it. I want this work. But it can't merge as-is, and one item is a genuine security bug that needs fixing regardless of everything else.

🔴 1. Stored XSS — fix this first, independent of the rest.
apps/web/src/app/worlds/[worldId]/lore/[entryId]/page.tsx:105:

<div className="prose-text max-w-none" dangerouslySetInnerHTML={{ __html: body }} />

body is content.body — an arbitrary API-supplied string. This is the only dangerouslySetInnerHTML in all of apps/web/src, and there's no DOMPurify / sanitize-html / rehype-sanitize in package.json. "TipTap generates it" isn't a defence: the backend validator takes content: z.unknown() (contributions.ts:33), so an attacker just POSTs raw HTML directly and skips the editor. And because this is a Server Component, the payload is server-rendered into the initial document — so even a bare <script> executes, not only <img onerror>.

It chains, too: the access token lives in localStorage (lib/api/session.ts:6) and is sent as a Bearer header from the form, so that XSS reads the token → account takeover. Sanitize on write (server-side allow-list) and on render, or drop dangerouslySetInnerHTML for a structured/plaintext renderer.

🔴 2. The backend doesn't exist — this is a sequencing problem, not a "stop working" one.
All five endpoints lib/api/lore.ts calls return 404. There is no routes/lore.ts on develop, on Perla's branch, on your #16 branch, or here — app.ts registers only health/auth/worlds/contributions, and LoreEntry exists as a Prisma model with no route surface. So today 100% of users would hit a 404 and see "Lore entity endpoints are not available on this backend branch yet." (lore-entity-tabs.tsx:41) — an internal branch note rendered as product copy. Please remove that string; it should never ship.

You've also unilaterally defined the API contract — response shapes ({entries, pagination}, {entry}, {ok:true}), the q/loreType/status params, and the content sub-document schema. Whoever writes the backend now has to either match your guesses or break this PR.

You're the right person to write that backend — you built the indexer and the Hive integration layer. So the ask isn't to drop this; it's to land it in the right order:

  • Option A (preferred): open a separate PR with routes/lore.ts + tests implementing this contract, get it reviewed and merged, then this PR merges on top of a real API.
  • Option B: rebuild these surfaces on the /worlds/:worldId/contributions API that already exists.
    Either way, let's agree the contract before more UI is built against it.

🔴 3. No permission gating — and it fails open.
The backend has a full model (world-permissions.ts, CREATE_LORE_DRAFT, EDIT_OWN_DRAFT, SUBMIT_PROPOSAL), but the UI checks none of it: lore-entry-form.tsx:144-149 only tests that a token string exists, and Add / Edit / Suggest edit / Delete render unconditionally — including for logged-out visitors. Gate on the viewer's per-world permission and default to hidden unless known-permitted. That needs a viewer/membership field on the hub response, which doesn't exist yet — flag it as a backend dependency now rather than discovering it later.

🔴 4. Drafts leak to anonymous readers. lore-entity-tabs.tsx:70-73 sends only q and loreType — never status — then renders whatever comes back, DRAFT/ARCHIVED badge and all, on a public page. Request status: 'PUBLISHED_CANON' explicitly for anonymous viewers instead of trusting the server to filter.

🔴 5. A user can self-publish to canon. lore-entry-form.tsx:302-313 offers a free select over all five statuses, so an author can create an entry directly as PUBLISHED_CANON, bypassing the DRAFT → SUBMITTED → proposal/vote flow. That contradicts the schema's own note (schema.prisma:434) that published canon must be confirmed via an indexed HiveReference. Make status read-only here; promotion goes through the proposal path.

🟠 6. Two of the six card types are unfilterable. constants.ts:50 and :58 both map to apiType: 'OTHER' (Quest and Story/Contribution), so both tabs fire the identical request and render the identical mixed list — and because getLoreTypeLabel('OTHER') .find()s the Quest row first, every Story renders labelled "Quest". The card asks for 6 distinct types; the API layer only distinguishes 4.

Nits: no client-side validation mirroring the backend's caps (100 KB body, 200-char title, 1,000-char summary) so a long entry is silently accepted then rejected, losing the user's work; handleDelete (:187) fires with no confirmation — a misclick destroys canon; search fires a request per keystroke (no debounce); content: LoreEntryContent | unknown collapses to unknown; and the as LoreEntry | null cast at contribute/page.tsx:67 masks the real type.

Also worth naming: this is your second PR stacked on an unmerged branch — #18 → #16 → #14, and #14 is stalled with open review items. If Perla reworks the hub or contributions shape, both of your PRs need rebasing. Worth pausing new stacked work until #14 lands.

@KareemN1936

Copy link
Copy Markdown
Collaborator Author

Agreed, thanks for the detailed review.

I’ll pause this UI PR and split the backend contract out first. Plan:

  • Fix the dangerouslySetInnerHTML XSS issue immediately.
  • Open a separate API PR for routes/lore.ts with tests.
  • Rebase this UI PR on top of the real lore API once reviewed.
  • Remove the internal backend-missing copy from the UI.
  • Gate Add/Edit/Delete on viewer permissions once the backend exposes them.
  • Make lore creation draft-only and keep canon publishing in the proposal/vote flow.

@mcfarhat

mcfarhat commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

@KareemN1936 first — you said you'd pause this one and split the backend out, and you did exactly that (#19). That was the right call and I want to acknowledge it before the rest.

But to be clear about the state: no code has changed on this branch. The 08-08 activity was a comment, not a commit — head is still 1e4bdf7 from 08-07, and #18 is now 2 commits behind its own base. So every item from my last review stands verbatim, and the green CI badge is a stale run.

Please mark this PR as a draft so it stops presenting as merge-ready with a CLEAN badge it hasn't earned.

Fix the XSS now, independently of the rebase — worlds/[worldId]/lore/[entryId]/page.tsx:105 still renders API-supplied content.body through dangerouslySetInnerHTML in a Server Component, and there's still no sanitizer in package.json. You committed to doing this first; it shouldn't wait on branch surgery.

There's now an extra reason to drop dangerouslySetInnerHTML entirely rather than bolt on a sanitizer: #19 HTML-escapes every string at write time, so content.body comes back as &lt;p&gt;…&lt;/p&gt; and that div will render visible literal tags. So it's not just dangerous, it's about to be functionally wrong. Use a structured/plaintext renderer.

Two ordering problems you'll hit, worth knowing before you start:

  1. Merging feat(ui): add lore entity CRUD frontend #18 into M2 worlds frontend integration kareem #16 auto-merges cleanly and then fails to build. M2 worlds frontend integration kareem #16 deleted apps/web/src/lib/api/session.ts, but lore-entry-form.tsx:15 still imports getStoredAccessToken from it. Module-not-found. You must switch to useAuthSession() when you rebase — this is mandatory regardless of everything else.
  2. Don't rebase feat(ui): add lore entity CRUD frontend #18 onto feat(api): add lore entity CRUD routes #19 yet. feat(api): add lore entity CRUD routes #19's base is Perla's m2-worlds-seed-bible-hub, which predates the merged Fix/auth secret hardening #15 — so on that branch requireSession is still missing database and missing its await. That's the same silent auth bypass you already fixed in M2 worlds frontend integration kareem #16, and rebasing onto it would drag it back in. Rebase feat(api): add lore entity CRUD routes #19 onto develop first, then feat(ui): add lore entity CRUD frontend #18 onto feat(api): add lore entity CRUD routes #19.

In the same pass, the still-open items:

  • Remove the branch-note string at lore-entity-tabs.tsx:41 ("Lore entity endpoints are not available on this backend branch yet") — internal notes mustn't ship as product copy.
  • Make status draft-only / read-only (lore-entry-form.tsx:302-313). feat(api): add lore entity CRUD routes #19 accepts only z.literal('DRAFT') under .strict(), so any other choice now 400s after the user fills the whole form.
  • Map Quest→QUEST and Story→STORY (constants.ts:50,58). feat(api): add lore entity CRUD routes #19 added both to the enum, so this is now a two-line fix and it stops Stories rendering labelled "Quest".
  • Send status: 'PUBLISHED_CANON' for anonymous viewers (lore-entity-tabs.tsx:70-73). feat(api): add lore entity CRUD routes #19 defaults correctly server-side, so this is now defence-in-depth rather than a leak — but the DRAFT/ARCHIVED badge rendering becomes dead code either way.
  • Permission gating still needs a backend dependency: the hub response has no viewer/membership field, so the UI can't tell whether the visitor may create or edit. feat(api): add lore entity CRUD routes #19 didn't add one. Please raise it explicitly rather than gating on "a token exists".

Nits: client-side caps mirroring #19's 100 KB / 200 / 1000 limits; delete confirmation; search debounce; content: LoreEntryContent | unknown collapses to unknown; the as LoreEntry cast.

@KareemN1936

Copy link
Copy Markdown
Collaborator Author

@mcfarhat Keeping this PR open only as the base branch for stacked PR #21.

Do not merge #18 by itself. The CRUD UI/API work has been superseded by the integrated PR #21, which carries this frontend work, the lore CRUD API, relationship linking, and the latest review fixes.

Review should continue on #21.

@mcfarhat

Copy link
Copy Markdown
Contributor

Closing this — its work is fully carried by #21, so nothing is lost.

Verified before closing: this branch's head 1e4bdf7d is an ancestor of feature/m2-relationship-lore-graph-kareem, so every commit here is already in #21. Same reasoning you applied yourself when you closed #19 in favour of #21.

Two reasons to close rather than leave it open:

  1. It's been CONFLICTING / DIRTY against its own base (m2-worlds-frontend-integration-kareem) for a while, so it can't merge anyway.
  2. An open PR sitting in a review queue is a signal that someone owes it a review. This one doesn't need one, and leaving it open risks a second reviewer picking it up and acting on it.

The branch itself is NOT deleted — #21 is still based on it, so the ref stays until #21 is rebased.

What happens next with #21 — and it isn't a retarget

I looked at simply retargeting #21 at develop, and it isn't safe. Against develop it's 19 commits / 40 files, and 7 of those files are @perlaimad's — lib/worlds.ts, lib/contributions.ts, seed.ts, two migrations and both her test files. They're in your history because this branch forked from m2-worlds-frontend-integration-kareem while it still carried her backend, and that branch has moved on since — its current head is no longer an ancestor of #21. So what #21 holds is an outdated snapshot of her work.

Retargeting would land that stale copy on develop, ahead of her #14 being reviewed, and potentially undo changes she's made since.

So the order is:

  1. Implement world seed, initial bible, browse worlds, and world hub #14 merges (Perla's backend — three blockers outstanding, but it's MERGEABLE/CLEAN)
  2. M2 worlds frontend integration kareem #16 rebases onto develop and drops the apps/api files, which are then already there
  3. Feature/m2 relationship lore graph kareem #21 rebases onto develop — Perla's files fall out of the diff naturally and it becomes one clean PR carrying the lore CRUD frontend, the lore API and the relationship graph

So: rebase, not retarget, and after #14 lands. Nothing for you to do on this right now beyond the #21 fixes already on that review — the two small ones (RELATIONSHIP_TRANSACTION_RETRIES 3 → 10, and scoping the inbound quota to cross-author) plus dropping the audit migration.

@mcfarhat mcfarhat closed this Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants