feat(parity): real getHTML export, padCopy/padRemove hooks, public-status guard, listAuthors filter

[SamTV12345]

Next parity round vs the original etherpad, building on #276/#279/#280.

getHTML

The HTTP API getHTML returned fake HTML: it string-replaced \n with <br> and dropped every other formatting attribute (bold/italic/underline, links, lists, headings, line markers). It now renders through the existing HTML exporter (GetPadHTMLDocument), mirroring the original Etherpad getHTML, which delegates to exportHtml.getPadHTMLDocument.

padCopy / padRemove plugin hooks

Ported the two missing pad-lifecycle hooks:

• padRemove fires from Manager.RemovePad — the single removal choke point covering deletePad, movePad source removal, and force-overwrite of a copy destination — capturing the loaded pad before deletion.
• padCopy fires from copyPad, copyPadWithoutHistory and movePad with the source and destination pads, matching the upstream {srcPad, dstPad} context.

public-status group guard

getPublicStatus / setPublicStatus now reject non-group pads (id without a $) with a 400, mirroring the original checkGroupPad guard, instead of silently operating on pads that can never be public.

listAuthorsOfPad

Filters out the synthetic SystemAuthorId (a.etherpad-system), used for unattributed inserts (HTTP API writes without an authorId, server-side imports). It is a changeset-bookkeeping detail, not a real contributor, so it no longer surfaces through the public API — matching upstream's Pad.SYSTEM_AUTHOR_ID filter.

Tests

• padCopy/padRemove hook firing (assert context src/dst/padId)
• public-status non-group rejection (GET + POST give 400)
• system-author exclusion from listAuthorsOfPad
• existing public-status tests updated to use valid group pad ids

🤖 Generated with Claude Code

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0)

1. padRemove pad can be nil 🐞 Bug ☼ Reliability

Description

Manager.RemovePad only captures the pad from the in-memory cache and passes it to the new padRemove
hook, so ctx.Pad will be nil whenever the pad wasn't already loaded. This can happen on
force-overwrite copy destinations (removed without loading), causing plugins to panic on nil
dereference or lose access to removed pad state.

Code

lib/pad/padManager.go[R131-145]

+	// Capture the loaded pad (if any) before deletion so the padRemove hook can
+	// hand listeners the pad context, mirroring the original Etherpad which
+	// fires padRemove from Pad.remove() with `this`.
+	removedPad := m.globalPadCache.GetPad(padID)
+
	if err := m.store.RemovePad(padID); err != nil {
		return err
	}
	m.globalPadCache.DeletePad(padID)
	m.padList.RemovePad(padID)

+	m.hook.ExecuteHooks(hooks.PadRemoveString, pad.Remove{
+		Pad:   removedPad,
+		PadId: padID,
+	})

Evidence

RemovePad grabs the pad only from the cache and forwards it to hook callbacks;
prepareCopyDestination can delete an existing destination pad without loading it first, making
ctx.Pad nil. Hook execution has no panic recovery, so a nil dereference in plugin code can crash the
server.

lib/pad/padManager.go[130-147]
lib/api/pad/copyMove.go[21-44]
lib/hooks/hook.go[85-96]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Manager.RemovePad()` fires the new `padRemove` hook with a `Pad` value taken only from `globalPadCache`. If the pad was never loaded (common for force-overwrite copy destinations), the hook receives `ctx.Pad == nil`, which can break plugin expectations and can crash the process if a hook callback dereferences it.

### Issue Context
The PR intent says the hook should capture the loaded pad before deletion (mirroring upstream). Today, `prepareCopyDestination()` can call `RemovePad(destinationID)` without ever loading that pad into the cache.

### Fix Focus Areas
- lib/pad/padManager.go[130-146]
- lib/api/pad/copyMove.go[33-44]

### Suggested fix approach
- In `Manager.RemovePad`, if `removedPad == nil`, check existence (`m.store.DoesPadExist(padID)`), and if it exists, load it (`m.GetPad(padID, nil, nil)`) *before* deleting so the hook gets a non-nil pad context.
- Alternatively (or additionally), in `prepareCopyDestination` (force path), load the destination pad via `GetPadSafe(destinationID, true, nil, nil, initStore.PadManager)` before calling `RemovePad`.
- Ensure you do not accidentally create a new pad when deleting a non-existent pad (so existence check must gate any load).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. PublicStatus skips padId validation 🐞 Bug ≡ Correctness

Description

GetPublicStatus/SetPublicStatus run the new isGroupPad() guard before any padId format validation,
so malformed pad IDs can return a misleading NotAGroupPadError (or fall through and be mapped to
404). This changes error semantics for invalid input and makes the API behavior inconsistent with
other endpoints that validate pad IDs first.

Code

lib/api/pad/copyMove.go[R299-301]

+		if !isGroupPad(padId) {
+			return c.Status(400).JSON(errors2.NotAGroupPadError)
+		}

Evidence

The handlers now return 400 based solely on a string contains check before reaching GetPadSafe,
which is where padId validity is actually enforced.

lib/api/pad/copyMove.go[295-345]
lib/api/utils/pad.go[10-14]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`GetPublicStatus` / `SetPublicStatus` check `isGroupPad(padId)` before validating `padId` syntax, which can cause malformed IDs to return the wrong error body (NotAGroupPadError) and skip the normal invalid-id handling.

### Issue Context
`utils.GetPadSafe()` is the shared guard that enforces `PadManager.IsValidPadId()`, but these handlers now bypass it via an early return.

### Fix Focus Areas
- lib/api/pad/copyMove.go[295-345]
- lib/api/utils/pad.go[10-29]

### Suggested fix approach
- Add an explicit `if !initStore.PadManager.IsValidPadId(padId) { return c.Status(400).JSON(errors2.NewInvalidParamError("padId is not a valid pad ID")) }` before the `isGroupPad()` check.
- Keep the group check next (so non-group *valid* pad IDs still return 400 as intended).
- (Optional but recommended) update the swagger annotations for `GetPublicStatus` to include the new 400 response.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix getHTML export, add padCopy/padRemove hooks, guard publicStatus, filter system author
🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

Walkthroughs

Description

• Render getHTML via the real HTML exporter to preserve rich formatting.
• Add missing padCopy/padRemove plugin hooks and fire them from copy/move/remove paths.
• Enforce group-pad-only publicStatus and hide the synthetic system author from listAuthorsOfPad.

Diagram

graph TD
  A["Pad API"] --> B["Copy/Move & Status"] --> G{"Group pad?"}
  G -->|"yes"| E["Pad manager"] --> F["Plugin hooks"]
  G -->|"no"| H["400 NotAGroup"]
  A --> C["HTML/Authors ops"] --> E
  C --> D["HTML exporter"] --> E

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Pass dstPad through copy/move flow (avoid reload in padCopy hook)

• ➕ Avoids an extra PadManager.GetPad() call for the destination pad.
• ➕ Guarantees hook sees the exact in-memory dst pad produced by the operation (no cache/DB divergence).
• ➖ Requires refactoring copy/move helpers to return or thread dstPad up to the handler (wider signature changes).
• ➖ More coupling between API layer and pad creation/copy internals.

2. Centralize publicStatus group-pad validation in shared middleware/helper

• ➕ Eliminates duplicate guard logic if more group-only APIs are added.
• ➕ Ensures consistent error behavior across endpoints.
• ➖ Adds indirection for a single current use-case.
• ➖ May be premature unless multiple endpoints need the same constraint.

Recommendation: Current approach is sound for parity: using the existing HTML exporter minimizes drift and preserves formatting, firing padRemove from PadManager.RemovePad makes deletion hook coverage comprehensive, and adding an explicit group-pad guard matches upstream semantics. If padCopy hook performance becomes a concern, consider threading dstPad through the copy/move code paths to avoid reloading it for the hook context.

File Changes

Enhancement (4)

copyMove.go Fire padCopy hook, add group-pad guard for publicStatus +43/-3

Fire padCopy hook, add group-pad guard for publicStatus

• Adds padCopy hook execution for copy/move endpoints with a source+destination pad context. Introduces an isGroupPad check and applies it to getPublicStatus/setPublicStatus to reject non-group pad IDs with a 400.

lib/api/pad/copyMove.go

---

HookConstants.go Define padCopy and padRemove hook keys +2/-0

Define padCopy and padRemove hook keys

• Adds hook constant strings for padCopy and padRemove so plugins can register listeners consistently.

lib/hooks/HookConstants.go

---

PadDefaultContent.go Add hook context structs for padCopy and padRemove +16/-0

Add hook context structs for padCopy and padRemove

• Introduces pad.Copy and pad.Remove structs to carry hook context (source/destination pads and removed pad info) in a way that mirrors upstream Etherpad expectations.

lib/models/pad/PadDefaultContent.go

---

padManager.go Emit padRemove hook from the central RemovePad path +10/-0

Emit padRemove hook from the central RemovePad path

• Captures the cached pad (if loaded) before deletion and executes the padRemove hook after removing the pad from the store/cache/list, making RemovePad the single hook-emission choke point.

lib/pad/padManager.go

---

Bug fix (2)

httpErrors.go Add 400 NotAGroupPadError for publicStatus APIs +7/-0

Add 400 NotAGroupPadError for publicStatus APIs

• Introduces a dedicated HTTP 400 error that matches upstream semantics when get/set publicStatus is called on non-group pads.

lib/api/errors/httpErrors.go

---

operations.go Filter system author from listAuthors and use real HTML exporter in getHTML +15/-15

Filter system author from listAuthors and use real HTML exporter in getHTML

• Updates listAuthorsOfPad to exclude the synthetic system author ID from the returned author list. Replaces the previous newline-to-<br> HTML stub in getHTML with full-document rendering via io.ExportHtml.GetPadHTMLDocument.

lib/api/pad/operations.go

---

Tests (1)

pad_api_test.go Add tests for new hooks and guards; adjust publicStatus fixtures +105/-7

Add tests for new hooks and guards; adjust publicStatus fixtures

• Adds coverage for padCopy and padRemove hook firing and validates publicStatus rejects non-group pads. Updates existing publicStatus tests to use valid group pad IDs and asserts listAuthorsOfPad excludes the system author.

lib/test/api/pad/pad_api_test.go

---

[qodo-free-for-open-source-projects]

1. Padremove pad can be nil 🐞 Bug ☼ Reliability

Manager.RemovePad only captures the pad from the in-memory cache and passes it to the new padRemove
hook, so ctx.Pad will be nil whenever the pad wasn't already loaded. This can happen on
force-overwrite copy destinations (removed without loading), causing plugins to panic on nil
dereference or lose access to removed pad state.

Agent Prompt

### Issue description
`Manager.RemovePad()` fires the new `padRemove` hook with a `Pad` value taken only from `globalPadCache`. If the pad was never loaded (common for force-overwrite copy destinations), the hook receives `ctx.Pad == nil`, which can break plugin expectations and can crash the process if a hook callback dereferences it.

### Issue Context
The PR intent says the hook should capture the loaded pad before deletion (mirroring upstream). Today, `prepareCopyDestination()` can call `RemovePad(destinationID)` without ever loading that pad into the cache.

### Fix Focus Areas
- lib/pad/padManager.go[130-146]
- lib/api/pad/copyMove.go[33-44]

### Suggested fix approach
- In `Manager.RemovePad`, if `removedPad == nil`, check existence (`m.store.DoesPadExist(padID)`), and if it exists, load it (`m.GetPad(padID, nil, nil)`) *before* deleting so the hook gets a non-nil pad context.
- Alternatively (or additionally), in `prepareCopyDestination` (force path), load the destination pad via `GetPadSafe(destinationID, true, nil, nil, initStore.PadManager)` before calling `RemovePad`.
- Ensure you do not accidentally create a new pad when deleting a non-existent pad (so existence check must gate any load).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools