From 1dc89496cf292f4908fb91bb14f82d2ee64536f4 Mon Sep 17 00:00:00 2001
From: highperfocused <highperfocused@pm.me>
Date: Sun, 6 Sep 2026 18:09:41 +0200
Subject: [PATCH] fix: address review feedback on the Spells app
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Per review:
- Scope is now derived (forced to "discover" when signed out) rather
  than stored as the requested value directly, matching the Feed
  app's pattern — signing out mid-session can no longer leave "My
  Spells" selected.
- resolveSpellFilter() now validates a spell's tag-filter letter
  (single a-zA-Z char) before using it as a "#<letter>" filter key,
  and clamps limit to [1, 500] instead of trusting a relay-sourced
  spell's number outright — a malformed or hostile spell can no
  longer produce a "#undefined" filter key or an enormous/NaN/zero
  limit. Added regression tests for all of these.
- NewSpellForm's Field now renders a real <label htmlFor> connected
  to each input's id (via useId()), and the Authors button group
  moved to a <fieldset>/<legend> instead of a label sitting over
  unrelated buttons — screen readers can now name every control.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BYiUtZMQeA5RHggQw73wto
---
 src/apps/spells/NewSpellForm.tsx | 77 +++++++++++++++++++++++---------
 src/apps/spells/index.tsx        | 10 +++--
 src/hooks/useSpells.test.ts      | 37 ++++++++++++++-
 src/hooks/useSpells.ts           | 14 +++++-
 4 files changed, 112 insertions(+), 26 deletions(-)

diff --git a/src/apps/spells/NewSpellForm.tsx b/src/apps/spells/NewSpellForm.tsx
index 2dd322a..19fba53 100644
--- a/src/apps/spells/NewSpellForm.tsx
+++ b/src/apps/spells/NewSpellForm.tsx
@@ -1,4 +1,4 @@
-import { useState } from 'react';
+import { useId, useState } from 'react';
 import { Loader2 } from 'lucide-react';
 import { Button } from '@/components/ui/button';
 import { Input } from '@/components/ui/input';
@@ -12,6 +12,7 @@ type AuthorsMode = 'anyone' | 'me' | 'contacts' | 'custom';
 
 export function NewSpellForm({ onDone }: { onDone: () => void }) {
   const { user } = useCurrentUser();
+  const formId = useId();
   const [name, setName] = useState('');
   const [description, setDescription] = useState('');
   const [kinds, setKinds] = useState('1');
@@ -102,12 +103,18 @@ export function NewSpellForm({ onDone }: { onDone: () => void }) {
         </p>
       </div>
 
-      <Field label="Name (optional)">
-        <Input value={name} onChange={(event) => setName(event.target.value)} placeholder="Bitcoin from contacts" />
+      <Field id={`${formId}-name`} label="Name (optional)">
+        <Input
+          id={`${formId}-name`}
+          value={name}
+          onChange={(event) => setName(event.target.value)}
+          placeholder="Bitcoin from contacts"
+        />
       </Field>
 
-      <Field label="Description (optional)">
+      <Field id={`${formId}-description`} label="Description (optional)">
         <Textarea
+          id={`${formId}-description`}
           value={description}
           onChange={(event) => setDescription(event.target.value)}
           rows={2}
@@ -115,11 +122,17 @@ export function NewSpellForm({ onDone }: { onDone: () => void }) {
         />
       </Field>
 
-      <Field label="Kinds (comma separated)">
-        <Input value={kinds} onChange={(event) => setKinds(event.target.value)} placeholder="1, 30023" />
+      <Field id={`${formId}-kinds`} label="Kinds (comma separated)">
+        <Input
+          id={`${formId}-kinds`}
+          value={kinds}
+          onChange={(event) => setKinds(event.target.value)}
+          placeholder="1, 30023"
+        />
       </Field>
 
-      <Field label="Authors">
+      <fieldset className="space-y-1.5">
+        <legend className="text-xs font-medium text-muted-foreground">Authors</legend>
         <div className="flex flex-wrap gap-1.5">
           {(
             [
@@ -148,31 +161,53 @@ export function NewSpellForm({ onDone }: { onDone: () => void }) {
             value={customAuthors}
             onChange={(event) => setCustomAuthors(event.target.value)}
             placeholder="hex pubkeys, comma separated"
+            aria-label="Custom author pubkeys, comma separated"
             className="mt-2"
           />
         )}
-      </Field>
+      </fieldset>
 
       <div className="grid grid-cols-2 gap-3">
-        <Field label="Tag filter letter">
-          <Input value={tagLetter} onChange={(event) => setTagLetter(event.target.value)} placeholder="t" maxLength={1} />
+        <Field id={`${formId}-tag-letter`} label="Tag filter letter">
+          <Input
+            id={`${formId}-tag-letter`}
+            value={tagLetter}
+            onChange={(event) => setTagLetter(event.target.value)}
+            placeholder="t"
+            maxLength={1}
+          />
         </Field>
-        <Field label="Tag values">
-          <Input value={tagValues} onChange={(event) => setTagValues(event.target.value)} placeholder="bitcoin, nostr" />
+        <Field id={`${formId}-tag-values`} label="Tag values">
+          <Input
+            id={`${formId}-tag-values`}
+            value={tagValues}
+            onChange={(event) => setTagValues(event.target.value)}
+            placeholder="bitcoin, nostr"
+          />
         </Field>
       </div>
 
       <div className="grid grid-cols-2 gap-3">
-        <Field label="Since (e.g. 7d, now, or blank)">
-          <Input value={since} onChange={(event) => setSince(event.target.value)} placeholder="7d" />
+        <Field id={`${formId}-since`} label="Since (e.g. 7d, now, or blank)">
+          <Input id={`${formId}-since`} value={since} onChange={(event) => setSince(event.target.value)} placeholder="7d" />
         </Field>
-        <Field label="Limit">
-          <Input value={limit} onChange={(event) => setLimit(event.target.value)} inputMode="numeric" />
+        <Field id={`${formId}-limit`} label="Limit">
+          <Input
+            id={`${formId}-limit`}
+            value={limit}
+            onChange={(event) => setLimit(event.target.value)}
+            inputMode="numeric"
+          />
         </Field>
       </div>
 
-      <Field label="Topics (comma separated, for discovery)">
-        <Input value={topics} onChange={(event) => setTopics(event.target.value)} placeholder="bitcoin, social" />
+      <Field id={`${formId}-topics`} label="Topics (comma separated, for discovery)">
+        <Input
+          id={`${formId}-topics`}
+          value={topics}
+          onChange={(event) => setTopics(event.target.value)}
+          placeholder="bitcoin, social"
+        />
       </Field>
 
       <div className="flex justify-end gap-2 pt-2">
@@ -188,10 +223,12 @@ export function NewSpellForm({ onDone }: { onDone: () => void }) {
   );
 }
 
-function Field({ label, children }: { label: string; children: React.ReactNode }) {
+function Field({ id, label, children }: { id: string; label: string; children: React.ReactNode }) {
   return (
     <div className="space-y-1.5">
-      <span className="block text-xs font-medium text-muted-foreground">{label}</span>
+      <label htmlFor={id} className="block text-xs font-medium text-muted-foreground">
+        {label}
+      </label>
       {children}
     </div>
   );
diff --git a/src/apps/spells/index.tsx b/src/apps/spells/index.tsx
index 53c51b6..f4688b8 100644
--- a/src/apps/spells/index.tsx
+++ b/src/apps/spells/index.tsx
@@ -35,9 +35,13 @@ type Scope = 'mine' | 'discover';
 export default function SpellsApp({ params, setTitle, setParams }: AppProps) {
   const { user } = useCurrentUser();
   const isMobile = useIsMobile();
-  const [scope, setScope] = useState<Scope>(user ? 'mine' : 'discover');
+  const [requestedScope, setRequestedScope] = useState<Scope>('mine');
   const [formOpen, setFormOpen] = useState(false);
 
+  // Signing out mid-session must not leave "My Spells" showing the previous
+  // user's (still-cached) spells — same reasoning as the Feed app's scope.
+  const scope: Scope = user ? requestedScope : 'discover';
+
   const mine = useMySpells();
   const discover = useDiscoverSpells();
   const query = scope === 'mine' ? mine : discover;
@@ -80,13 +84,13 @@ export default function SpellsApp({ params, setTitle, setParams }: AppProps) {
       <ScopeTab
         active={scope === 'mine'}
         disabled={!user}
-        onClick={() => setScope('mine')}
+        onClick={() => setRequestedScope('mine')}
         icon={<UserIcon className="size-3.5" aria-hidden />}
         label="My Spells"
       />
       <ScopeTab
         active={scope === 'discover'}
-        onClick={() => setScope('discover')}
+        onClick={() => setRequestedScope('discover')}
         icon={<Globe className="size-3.5" aria-hidden />}
         label="Discover"
       />
diff --git a/src/hooks/useSpells.test.ts b/src/hooks/useSpells.test.ts
index d0bc1e7..83cf991 100644
--- a/src/hooks/useSpells.test.ts
+++ b/src/hooks/useSpells.test.ts
@@ -1,11 +1,23 @@
 import { describe, expect, it } from 'vitest';
 import type { NostrEvent } from '@nostrify/nostrify';
-import { encodeSpellTags, parseSpell, resolveSpellFilter, resolveTimestamp } from './useSpells';
+import { encodeSpellTags, parseSpell, resolveSpellFilter, resolveTimestamp, type ParsedSpell } from './useSpells';
 
 function spellEvent(tags: string[][], content = ''): NostrEvent {
   return { id: 'x', pubkey: 'author', created_at: 0, kind: 777, tags, content, sig: '' };
 }
 
+/** A ParsedSpell as if it came straight off a relay — encodeSpellTags always produces well-formed data, so malformed cases are built by hand. */
+function parsedSpell(overrides: Partial<ParsedSpell>): ParsedSpell {
+  return {
+    description: '',
+    kinds: [1],
+    authors: [],
+    topics: [],
+    event: spellEvent([]),
+    ...overrides,
+  };
+}
+
 describe('resolveTimestamp', () => {
   it('resolves a relative duration against now', () => {
     const now = Math.floor(Date.now() / 1000);
@@ -78,4 +90,27 @@ describe('resolveSpellFilter', () => {
     const filter = resolveSpellFilter(parseSpell(event), { me: undefined, contacts: [] });
     expect(filter?.['#t']).toEqual(['bitcoin', 'nostr']);
   });
+
+  it('ignores a malformed (non-single-letter) tag filter instead of producing "#undefined"', () => {
+    const spell = parsedSpell({ tagFilter: { letter: '', values: ['x'] } });
+    const filter = resolveSpellFilter(spell, { me: undefined, contacts: [] });
+    expect(Object.keys(filter ?? {}).some((key) => key.startsWith('#'))).toBe(false);
+  });
+
+  it('clamps an excessive limit to the maximum', () => {
+    const spell = parsedSpell({ limit: 1_000_000 });
+    const filter = resolveSpellFilter(spell, { me: undefined, contacts: [] });
+    expect(filter?.limit).toBe(500);
+  });
+
+  it('drops a zero or NaN limit rather than sending a degenerate query', () => {
+    expect(resolveSpellFilter(parsedSpell({ limit: 0 }), { me: undefined, contacts: [] })?.limit).toBeUndefined();
+    expect(resolveSpellFilter(parsedSpell({ limit: NaN }), { me: undefined, contacts: [] })?.limit).toBeUndefined();
+  });
+
+  it('keeps a normal, in-range limit as-is', () => {
+    const spell = parsedSpell({ limit: 50 });
+    const filter = resolveSpellFilter(spell, { me: undefined, contacts: [] });
+    expect(filter?.limit).toBe(50);
+  });
 });
diff --git a/src/hooks/useSpells.ts b/src/hooks/useSpells.ts
index 3b358a4..45a4d62 100644
--- a/src/hooks/useSpells.ts
+++ b/src/hooks/useSpells.ts
@@ -26,6 +26,11 @@ const UNIT_SECONDS: Record<string, number> = {
   y: 31_536_000,
 };
 
+/** A single NIP-01 tag-filter letter, e.g. the `t` in `#t`. */
+const TAG_LETTER_RE = /^[a-zA-Z]$/;
+/** Caps a relay-sourced spell's `limit` so Run can't be tricked into a huge query. */
+const MAX_SPELL_LIMIT = 500;
+
 /** Resolves `now`, `<n><unit>` (e.g. `7d`) or a literal unix timestamp string. */
 export function resolveTimestamp(value: string): number | undefined {
   const trimmed = value.trim();
@@ -125,11 +130,16 @@ export function resolveSpellFilter(
     filter.authors = resolved;
   }
 
-  if (spell.tagFilter) {
+  // Relay-provided events are untrusted: a malformed spell must not produce
+  // a filter key like "#undefined", or a limit that is 0/NaN/huge enough to
+  // lock up the UI on Run.
+  if (spell.tagFilter && TAG_LETTER_RE.test(spell.tagFilter.letter) && spell.tagFilter.values.length > 0) {
     filter[`#${spell.tagFilter.letter}`] = spell.tagFilter.values;
   }
 
-  if (spell.limit) filter.limit = spell.limit;
+  if (spell.limit !== undefined && Number.isFinite(spell.limit) && spell.limit > 0) {
+    filter.limit = Math.min(spell.limit, MAX_SPELL_LIMIT);
+  }
   if (spell.since) {
     const since = resolveTimestamp(spell.since);
     if (since !== undefined) filter.since = since;