fix: address review feedback on the Spells app

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
This commit is contained in:
2026-09-06 18:09:41 +02:00
parent 89692ad382
commit 1dc89496cf
4 changed files with 112 additions and 26 deletions

View File

@@ -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>
);

View File

@@ -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"
/>

View File

@@ -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);
});
});

View File

@@ -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;