Refactor Perpetua frontend to Valora with UI enhancements

[zeeghazi]

Summary

• Clean rewrite of Perpetua's frontend as Valora under core/features/valora/ using TanStack Query instead of Redux thunks
• All data fetching via React Query hooks with automatic cache invalidation, Redux only for UI state (feed type, tag filter, edit
  mode)
• Full feature parity with Perpetua: shelves, items (NFT/Markdown/Shelf), three feed types (recency/random/storyline), tags,
  following, drag-and-drop reordering, permissions
• Three view modes: grid, list, and blog (prose markdown with visual sections)
• Masonry layout for shelf grid, shelf info dialog, Add-to-Shelf with multi-select
• Removed asset manager and ICP asset rendering logic
• Valora hidden from app dashboard (accessible via /app/valora route only)

[github-actions]

Code Review: Valora Frontend Refactor

Overall this is a well-structured rewrite. The migration from Redux thunks to TanStack Query is a clear improvement — data fetching is now co-located with the components that need it, and the removal of ~900 lines of asset-manager plumbing significantly reduces complexity. Below is detailed feedback.

---

Bugs

useUpdateShelfMetadata and useAddItem don't return the mutation result

src/alex_frontend/core/features/valora/hooks/useMutations.ts

// useUpdateShelfMetadata (~line 44)
const result = await actor!.update_shelf_metadata(shelfId, t, d);
unwrapResult(result);   // ← return value discarded

// useAddItem (~line 80)
const result = await actor!.add_item_to_shelf(...);
unwrapResult(result);   // ← same issue

Both mutationFns swallow the return value of unwrapResult(). The mutation's data will always be undefined. Every other mutation in the file returns the result. This is inconsistent, and could silently break callers that rely on mutation.data.

unwrapResult loses error details

src/alex_frontend/core/features/valora/utils.ts:36-37

const key = Object.keys(err)[0];
throw new Error(key || "Unknown error");

When a variant error comes back (e.g., { NotAuthorized: null }), only the variant name is thrown — the associated value is dropped. This makes errors harder to debug. A small improvement:

const key = Object.keys(err)[0];
const val = err[key];
const detail = val !== null && val !== undefined ? `: ${JSON.stringify(val)}` : "";
throw new Error(key ? `${key}${detail}` : "Unknown error");

getItemContentValue uses unchecked non-null assertions

src/alex_frontend/core/features/valora/utils.ts:50-58

if ("Nft" in content) return content.Nft!;
if ("Markdown" in content) return content.Markdown!;
return content.Shelf!;

The last line asserts content.Shelf is always defined if neither Nft nor Markdown is present. If an unexpected variant arrives from the backend, this will silently return undefined (TypeScript is satisfied but the runtime value may not be). Consider adding a guard:

if ("Shelf" in content && content.Shelf !== undefined) return content.Shelf;
throw new Error(`Unknown item content variant: ${JSON.stringify(content)}`);

---

Actor Non-null Assertions in Mutations

Every mutationFn in useMutations.ts uses actor! without a guard:

const result = await actor!.store_shelf(title, desc, [], tagList);

The useQuery hooks correctly use enabled: !!actor, which prevents queries from running without an actor. Mutations don't have an equivalent guard — they rely on the caller not to invoke them when actor is undefined. A runtime crash would still occur if a mutation is triggered at the wrong time (e.g., a button press before auth completes). Consider a lightweight guard at the top of each mutationFn:

if (!actor) throw new Error("Not authenticated");

---

Minor Code Quality Notes

assetSource type narrowing in useNftData.ts — removing 'ic_canister' from the union is a good cleanup. The state type now accurately reflects what the code sets.

ShelfDetail.tsx size — at 582 lines it's on the larger side, but the logic is cohesive (view modes, DnD, rendering). Not a blocker.

SortableRow defined inside the module but not memoised — since ShelfDetail re-renders when viewMode changes, and SortableRow is defined at module scope (not inside the component), this is fine. Just noting it for clarity.

---

Input Validation

User-supplied strings (tags, titles, descriptions, markdown content) flow from form inputs directly to canister calls with no visible length or format validation in the UI layer. A few guardrails to consider:

• Tag length limit (the ManageTags UI accepts free-form input)
• Title/description max length checks before the optimistic update in useUpdateShelfMetadata

The backend likely enforces these, but early client-side validation improves UX (instant feedback vs. a rejected canister call).

---

Test Coverage

The two updated test files only swap the actor argument out of fetchNftTransactions. There are no new tests for the Valora feature. Given this is a full rewrite of Perpetua's frontend, even a handful of unit tests for:

• normalizeShelf / unwrapResult in utils.ts
• Query key structure in types.ts
• The Redux slice in store/slice.ts

...would meaningfully reduce regression risk.

---

Dependency Changes

• react-beautiful-dnd → @dnd-kit/*: a good swap; dnd-kit has better accessibility and is actively maintained.
• @dfinity/identity-secp256k1 added — is this actually used? Not seeing it referenced in the new Valora code. Worth double-checking it isn't a stray dependency.

---

Summary

Area	Status
Architecture / separation of concerns	✅ Clear improvement
Missing return values in two mutations	⚠️ Inconsistency / silent data loss
Actor null safety in mutations	⚠️ Potential runtime crash
Error message quality in unwrapResult	⚠️ Debug friction
Input validation	ℹ️ Worth adding
Test coverage	ℹ️ No new tests

The core approach is solid. The two missing return statements are the most important thing to fix before merge, as they silently break the mutation data contract.

[github-actions]

test

[github-actions]

Review comment from automated code review agent: the full structured review will follow in a subsequent comment due to formatting constraints.

[github-actions]

Code Review: Perpetua to Valora Refactor

Overall this is a well-executed migration from Redux thunks to TanStack Query. The separation of concerns is clean, optimistic updates are implemented correctly, and the architecture is much easier to follow. Issues below should be addressed before merge.

---

BUGS

1. unwrapResult return values dropped in 3 mutations (useMutations.ts)

useRemoveItem, useSetItemOrder, and useReorderProfileShelf call unwrapResult(result) but discard the return value. Contrast with useCreateShelf and useUpdateShelfMetadata which correctly return it. The mutationFn returns undefined for these three, so mutation.data is always undefined for callers. Fix: add return before unwrapResult(result).

2. useAddItem does not invalidate feed queries (useMutations.ts)

Adding an item updates the shelf updated_at timestamp, changing its position in the recency feed. The feed will show stale ordering until the 30-second staleTime expires. The onSuccess handler should also call:

qc.invalidateQueries({ queryKey: valoraKeys.recentFeed() });

---

PERFORMANCE

3. N+1 queries in ShelfItemCard (components/ShelfItemCard.tsx)

Each nested-shelf item fires an independent useShelf(shelfId) query. A shelf with many shelf-type items generates N parallel canister calls. React Query deduplicates identical keys, but distinct shelf IDs each pay full round-trip cost. Consider passing title and item count as props from the parent when available, falling back to useShelf only when needed.

4. getId must be stable in useReorder (hooks/useReorder.ts)

onDragEnd has getId in its useCallback dependency array. If the caller passes an inline arrow function, onDragEnd is recreated every render — with dnd-kit/core this can cause dropped drag events on slower devices. The hook should document that getId must be a stable reference (useCallback at the call site), or stabilize it internally with useRef.

5. Infinite query pages accumulate without bound (hooks/useFeeds.ts)

useRecentFeed and useStorylineFeed have no maxPages limit (TanStack Query v5 supports this). After extended scrolling the cache grows indefinitely. Consider maxPages: 5 with a matching getPreviousPageParam.

---

CODE QUALITY

6. Empty-string description coerced to null in optimistic update (useMutations.ts)

In useUpdateShelfMetadata the optimistic update uses description: description || null. An empty string is falsy and becomes null in the optimistic state even if the backend stores it as an empty string, causing a visible flash when the query resolves. Use description ?? null instead.

7. Separator imported but unused (components/ValoraLayout.tsx)

8. Commented-out extraReducers cases (arweaveAssetsSlice.ts)

Commented-out addCase handlers for checkAssetsAvailability and checkAssetAvailability were left in the file. Since the corresponding thunks are deleted in this PR, these should be removed.

---

TYPE SAFETY

9. Non-null assertions on discriminated union (utils.ts)

In getItemContentValue, the in check confirms the key exists but not that the value is non-null: content.Nft!, content.Markdown!, content.Shelf!. Safe with Candid-generated types but fragile with hand-constructed objects. Consider accepting the Candid-generated ItemContent type directly.

---

TEST COVERAGE

No tests were added for the Valora feature. Most valuable areas to cover:

• normalizeShelf in utils.ts: position-based sorting logic is non-trivial
• unwrapResult: the Object.keys(err)[0] path is fragile if err is an empty object
• useReorder: drag-and-drop reorder state transitions

The existing tests were correctly updated to reflect removal of the actor parameter from updateTransactions and toggleSort.

---

MINOR

• useUserShelves calls Principal.fromText(principalStr!) which throws for malformed non-null/undefined strings. enabled: !!principalStr only guards against null/undefined.
• lucide-react bumped from 0.539.0 to 0.577.0: worth a visual pass to confirm no icon renames caused silent blank spots in the UI.

---

Overall the architecture is solid and the asset manager / ICP assets cleanup is a welcome simplification. Items 1 and 2 are highest priority to fix before merge.

[github-actions]

Code Review: Valora Frontend Rewrite (PR #140)

Overall this is a well-executed migration from Redux thunks to TanStack Query. The separation of concerns is clear — React Query owns server state, Redux owns UI state — and the code is noticeably cleaner than what it replaces. The query key factory, optimistic updates, and useReorder abstraction are all solid patterns. A few issues worth addressing before merge:

---

Bugs / Correctness

1. Type-unsafe cursor cast in useShelvesByTag (hooks/useTags.ts:28)

const cursor = pageParam ? [pageParam] : [];
const result = await actor!.get_shelves_by_tag(tag!, { cursor: cursor as any, limit: 20n });

pageParam is typed any, so cursor here ends up as string[] at runtime, but the canister method expects [] | [bigint]. The as any hides a real type mismatch. The correct form (matching the pattern used in useRecentFeed and useStorylineFeed) is:

const cursor: [] | [bigint] = pageParam ? [BigInt(pageParam)] : [];

This could cause silent failures when paginating tag search results.

2. getItemContentValue / getItemContentType unsafe fallthrough (utils.ts:52-62)

Both functions treat Shelf as the default/fallback case with a non-null assertion:

export function getItemContentValue(content: { Nft?: string; Markdown?: string; Shelf?: string }): string {
    if ("Nft" in content) return content.Nft!;
    if ("Markdown" in content) return content.Markdown!;
    return content.Shelf!;  // returns undefined! if type is unknown
}

If a new ItemContent variant is ever added on the canister side, this silently returns undefined. Consider adding an explicit guard or throwing on the unknown branch.

---

Performance

3. Sequential mutations for shelf reordering (components/UserShelvesView.tsx:83-91)

for (let i = 0; i < reordered.length; i++) {
    await reorderShelf.mutateAsync({ shelfId: shelf.shelfId, ... });
}

This makes N sequential round-trips to reorder N shelves. For a user with 20 shelves this will be noticeably slow. If the canister supports a bulk reorder call, use it. If not, Promise.all the mutations (accepting that partial failure is possible) or at minimum document why sequential ordering is necessary here.

---

Inconsistency

4. AddToShelf bypasses React Query for tag-based fetch (actions/AddToShelf.tsx:104-125)

searchPublicShelvesByTagFn makes a direct actor call and stores results in useState, while the nearly identical use case in actions/AddItem.tsx correctly uses the useShelvesByTag hook. The direct-call path gets no caching, no deduplication, and no background invalidation. It also calls actor.get_public_shelves_by_tag(tag) — a different method than useShelvesByTag uses (get_shelves_by_tag) — which may intentionally differ, but this should be explicit.

---

Minor Issues

5. Stale data risk in useReorder on re-entry (hooks/useReorder.ts:16-19)

const enterEditMode = useCallback(() => {
    setEditedItems([...items]);  // snapshot taken here
    setIsEditMode(true);
}, [items]);

If useShelf returns a refetched result while the user is in edit mode, items (the prop) updates but editedItems stays at the snapshot. If the user cancels and re-enters, they get the fresh list — which is correct. But if items changes mid-edit (e.g., another tab added an item), the user's reorder silently drops the new item. Worth a comment at minimum.

6. useTogglePublicAccess discards return value (hooks/useMutations.ts:167)

mutationFn: async ({ shelfId, publicEditing }) => {
    const result = await actor.toggle_shelf_public_access(shelfId, publicEditing);
    unwrapResult(result);  // return value dropped
},

All other mutations return unwrapResult(result). The function works correctly (errors throw), but it's inconsistent and makes the mutation's data field undefined. Match the pattern of the other mutations for uniformity.

7. Empty string query key fallback (multiple hooks)

valoraKeys.shelf(""), valoraKeys.userShelves(""), etc. are used as query keys when shelfId/principalStr is undefined. The enabled: !!shelfId guard prevents fetching, but it leaves a stale ["valora", "shelf", ""] entry in the cache. Prefer valoraKeys.shelf(shelfId!) inside queryFn and rely solely on enabled to disable, which avoids polluting the cache key space with empty strings.

---

What's Good

• dnd-kit migration: Dropping react-beautiful-dnd (archived) for @dnd-kit is the right call.
• Optimistic updates: useRemoveItem, useUpdateShelfMetadata, useTogglePublicAccess, useAddTag, useRemoveTag, useFollowTag/User all implement optimistic updates with proper rollback. Clean.
• unwrapResult helper: Simple, reusable, correctly surfaces canister error variants as typed JS errors.
• useReorder hook: Well-abstracted, generic, used consistently in both ShelfDetail and UserShelvesView.
• Infinite scroll: Clean IntersectionObserver-based implementation in FeedView for both recency and storyline feeds.
• ValoraErrorBoundary: Good to have a feature-scoped error boundary isolating failures to this feature.