Fedibook Code Review 1
Two weeks of creative freedom — trying things, changing direction, letting the AI run — and the technical debt quietly piled up in the background.
It's time for a code review. And yes, it found what I'd expect to find.
The good news first: the overall architecture holds. The handler/service/repository separation is consistent, the ActivityPub federation logic is well-contained, and the core domain error handling is clean. No showstoppers, no structural collapses.
But two weeks of autonomy leaves traces. Mixed Danish and English error messages scattered across the codebase. The same actor URL assembled by hand in five different files. Nine hundred lines of business logic living in a single worker file with no internal structure. Small things individually — the kind that quietly make the code harder to work with every week I leave them.
Nothing catastrophic. But an honest bill for two productive weeks. Time to pay it.
Reviewed against the current main branch (2026-04-02). The codebase is well-structured overall — the handler/service/repository separation holds consistently and the ActivityPub federation logic is well-contained. This report focuses on areas where inconsistency or unnecessary complexity make the code harder to work with than it needs to be.
Consistency
Mixed-language error messages
Files: service/post.go, service/friendship.go, service/group.go, handler/activitypub.go, handler/post.go, handler/user.go, handler/problem.go
Error messages are split between English and Danish with no clear rule for which language is used where. Examples: "opslaget skal indeholde tekst" alongside "can only share public posts"; "Brugeren '%s' eksisterer ikke." alongside "User '%s' could not be found.". The validation problem title ("Valideringsfejl", the Danish word for "validation error") is hardcoded in problem.go, which is the one place all validation errors converge.
Pick one language for all user-facing strings in the Go layer and apply it consistently. The fix is mechanical and the candidates are obvious from a grep.
Auth middleware uses a different error format
File: middleware/jwt.go
All handler code produces RFC 7807 problem responses via writeProblem. The auth middleware uses http.Error() with manually constructed JSON strings ({"status":401,"title":"Unauthorized",...}). The format is structurally similar but the content type is text/plain instead of application/problem+json and it bypasses the shared problem struct entirely.
Extract writeProblem into a package that both middleware and handlers can import, or at minimum use json.Marshal + proper content-type here.
Inconsistent limit parsing
Files: handler/post.go:parseCursorParams, handler/user.go:Search, handler/friendship.go:List
parseCursorParams enforces 0 < limit <= 100. The Search handler in handler/user.go has its own limit parsing (strconv.Atoi) with no upper bound. FriendshipHandler.List calls parseCursorParams but discards the after return value and re-reads after manually from the query string — _, limit := parseCursorParams(r).
Use parseCursorParams uniformly, or if a handler needs custom behaviour, document why.
Group actor document is a raw map
File: handler/activitypub.go:GroupActor
The user actor is served via WebFingerHandler.GetActor, which constructs an activitypub.Actor struct. GroupActor builds the same document as a map[string]any with string literals for every key. The two code paths have no shared structure: adding a field to the actor format requires changes in both places, and the group actor silently lacks fields (e.g. cover photo "image") that the struct would expose. The activitypub.Actor struct itself is also missing an Image field for the cover photo.
Add the Image *Image field to activitypub.Actor. Use the struct for both user and group actors.
NodeInfo ignores registration mode
File: handler/activitypub.go:NodeInfo
"openRegistrations": true is hardcoded regardless of the RegistrationMode config value. An instance in "approval" mode still advertises open registrations to the Fediverse.
Pass cfg.RegistrationMode through to the handler and derive the boolean from it.
Complexity
In-function regexp compilation
File: service/group.go:slugifyHandle
s = regexp.MustCompile(`[^a-z0-9]+`).ReplaceAllString(s, "-")
The regexp is compiled on every call to slugifyHandle. The file already has a package-level handleRE; make the replacement regexp a package-level variable in the same way.
Worker logic in cmd/worker/main.go
File: cmd/worker/main.go (~900 lines)
All task handler implementations — publish note, delete note, follow, undo-follow, group announce, group delete, friends note, comment, share, PeerTube upload/delete — are closures defined inline in main.go. The file is a single large bag of business logic with no internal structure. Each handler closure closes over raw repo pointers and the config struct.
Move handlers to a worker/ or tasks/ package (mirroring how service/ is structured). Keep main.go to wiring and startup only. This also makes handler functions independently testable.
actorURLsToMentions vs actorURLsToMentionMaps
Files: cmd/worker/main.go:883, service/ap_group.go:1349
Two functions that build Mention tag objects from the same input — a slice of actor URLs — exist because the worker builds typed activitypub.Note structs while the service builds map[string]any. The divergence forces the duplication.
Consolidate on the typed activitypub.Note struct in both code paths; then one actorURLsToMentions function suffices and the map[string]any AP-building approach in ap_group.go can be retired.
Opaque group membership status encoding
Files: repository/group.go, service/group.go, domain/group.go
The database query concatenates membership strings like "member_pending" and "admin_invited" from two columns, and the service layer decodes them with a switch statement. The connection between what the DB produces and what the code expects is implicit and fragile. A typo in either the SQL or the switch case silently produces "none" status.
Use two separate columns returned from the query (role + status) and assemble domain.Group.MembershipStatus explicitly in code — no string concatenation.
Structure
Duplicate constructors
Files: handler/post.go, handler/group.go
Both PostHandler and GroupHandler have two constructors: one without uploadDir and one with. The without-uploadDir form is the same as calling the with-uploadDir form with an empty string. Neither variant has tests that depend on the distinction.
Delete the shorter constructors and update cmd/api/main.go callsites. Reduces noise in the API and one fewer thing to keep in sync.
HTTP signature verification is duplicated
File: handler/activitypub.go:Inbox and GroupInbox
Both methods contain identical blocks that read and size-limit the body, call activitypub.VerifyRequest, log a warning on failure, and return 401. This is 10 lines copy-pasted. Extract into a helper:
func readAndVerifyBody(r *http.Request, w http.ResponseWriter) ([]byte, bool)
Setter methods for optional dependencies
Files: handler/group.go, handler/webfinger.go, service/ap_group.go, service/channel.go
Post-construction setters (SetAPMemberInviter, SetGroupResolver, SetGroupFinder, SetGroupJoiner, SetLocalInviter) are used to break circular dependencies during wiring. Missing any of them produces a nil panic at the first relevant request, not a compile-time error, and the wiring is currently spread across cmd/api/main.go without any enforcement. This is the right pattern for circular dependency breaking, but each setter should guard against nil use with a clear error message.
UserIDFromContext wrapper
File: handler/auth.go
// UserIDFromContext re-exports middleware helper for use in handlers.
func UserIDFromContext(r *http.Request) (uuid.UUID, bool) {
return middleware.UserIDFromContext(r.Context())
}
This wrapper exists so handler code can write UserIDFromContext(r) instead of middleware.UserIDFromContext(r.Context()). It adds a layer of indirection and two signatures in the codebase for the same concept. The handler package already imports middleware for IsAdminFromContext. Just use middleware.UserIDFromContext(r.Context()) directly in handlers, or alias the import.
Dead Code and Duplication
summaryMap vs userSummaryResponse
Files: handler/friendship.go:summaryMap, handler/user.go:userSummaryResponse
These two functions produce structurally identical map[string]any objects for a user summary. summaryMap takes *domain.UserSummary; userSummaryResponse takes *domain.User. Both appear in the same handler package. Pick one, adjust the argument type, delete the other.
fediverse_handle constructed three times
Files: handler/post.go:postResponse, handler/post.go:commentResponse, handler/user.go (via fediverseHandle)
postResponse and commentResponse both inline "@" + p.Author.Username + "@" + p.Author.InstanceDomain without calling fediverseHandle(). The helper exists in user.go; it just isn't used where it should be. For remote users it produces the wrong value — a remote user's username is already stored as "user" not "user@domain", so the inline formula gives @user@domain while fediverseHandle() gives @user (correct for remote). This is a latent display bug, not just duplication.
Fix postResponse and commentResponse to use fediverseHandle().
AP actor URL constructed in five places
Files: handler/activitypub.go, handler/user.go, cmd/worker/main.go, service/ap.go, service/ap_group.go
"https://" + domain + "/ap/users/" + username appears inline throughout. The same pattern for group URLs. A centralized helper (actorURL(domain, username string) string, groupActorURL(domain, id string) string) in the activitypub package would eliminate this and make future URL changes a one-line edit.
processGroupAnnounce legacy path
File: service/ap.go:775
This function handles the old Announce{Create{Note}} format for group posts. The dev guide says it's kept for in-flight activities from before the format change. The format change was shipped in the most recent commit series. A concrete deprecation window and removal ticket would give this a defined endpoint rather than indefinite existence.
ActivityPub Specifics
The federation logic is well-contained. Both APService (user inbox) and APGroupService (group inbox) are in dedicated service files, and the activitypub package handles protocol-level concerns (signing, verification, types) separately from business logic. The key issues are in consistency and URL management.
Scattered AP URL construction
As noted above, actor URL strings are assembled inline throughout the codebase. This is the most actionable single change in the AP area: an activitypub.ActorURL(domain, username) helper used everywhere would make the code more readable and resilient to future URL scheme changes.
Fragile fallback URL in ServeNote
File: handler/activitypub.go:ServeNote
When post.Author.APID is nil or empty, the handler falls back to reconstructing the actor URL as:
parts := strings.SplitN(post.Author.Username, "@", 2)
if len(parts) == 2 {
authorActorURL = "https://" + parts[1] + "/users/" + parts[0]
}
The /users/ path segment is Mastodon-specific. Pixelfed uses /users/ too but other AP software may not. This fallback should be eliminated by ensuring ap_id is always populated for remote users, which UpsertRemoteUser should guarantee. Add a check at upsert time rather than working around the gap here.
openRegistrations hardcoded
File: handler/activitypub.go:NodeInfo
Covered under Consistency above, but worth calling out in the AP context: other Fediverse software uses openRegistrations to decide whether to show an instance in discovery directories. Advertising true when the instance requires approval misleads users and potentially Fediverse crawlers.
What Is Working Well
- The handler/service/repository separation is consistent across all features. Repository methods return domain types; handlers know nothing about SQL; services don't import
net/http. - Domain errors (
ErrNotFound,ErrConflict, etc.) flow cleanly from repository through service to handler without leaking implementation details. - The group federation implementation — unified
Create{Note}format,attributedToarray, Mention tags for Mastodon DM routing — is thoughtfully documented in both code and the dev guide. - Cursor-based pagination is consistent in concept across all list endpoints (though the implementation has the capping inconsistency noted above).
- The SSE hub is simple and correct: non-blocking sends with backpressure dropping, periodic heartbeats, clean unsubscribe.
- Test coverage for the non-trivial service logic that exists (
share_test.go,visibility_test.go,ap_test.go,ap_group_test.go) is well-targeted.
Priority List
1. Mixed-language strings — Danish error messages appearing in API responses undermine consistency and make the code appear unfinished to contributors. A grep-and-replace pass across service/ and handler/ is low-risk and high-visibility.
2. AP actor URL duplication — The inline "https://" + domain + "/ap/..." construction scattered across five files is the most likely source of future inconsistency bugs. Centralising it into two helpers is a safe, mechanical refactor.
3. Worker main.go decomposition — Keeping 900 lines of business logic in a single file with no internal structure makes the worker untestable and hard to navigate. Moving handlers to a dedicated package is the largest change here but also the most impactful for long-term maintainability.
4. Middleware error format — The auth middleware returning text/plain JSON instead of RFC 7807 problem responses is a small inconsistency but it affects every unauthenticated request and will confuse clients that parse the content type.
5. fediverse_handle display bug — postResponse and commentResponse produce incorrect fediverse handles for remote users. This is the only item on this list that is a correctness issue rather than a style issue and should be fixed promptly.