feat: Grymoire x Interlinear — Old Norse public reading preview

[Peleke]

Summary

Public preview page for Grymoire Old Norse readings on Interlinear, with ensk.is dictionary integration for supplemental vocabulary.

• Grymoire MDX ingestion pipeline: parses frontmatter, passage, Modern Icelandic, vocabulary, generates TTS audio, upserts to Supabase
• Public preview page at /preview/readings/[slug] with word-level vocab tooltips
• Auto-injects Interlinear banner into Grymoire MDX source files
• Modern Icelandic passage extraction for ensk.is compatibility + display
• ensk.is batch API integration: lemma-first lookup, no fuzzy, primary-first definitions capped at 3
• MDX curated vocabulary always takes priority over ensk.is supplemental
• Client-side exact word/lemma matching only (no stem/prefix fuzzy)
• Stopwords for Icelandic function words (pronouns, prepositions, conjunctions, vera/hafa)
• Quality filter for garbage definitions from reversed 1932 Zoëga dictionary (band-aid — tracked in Peleke/ensk.is#27)

ensk.is changes (already merged + deployed)

• PR Habits & Programs Tiles Vertical Stack #25: Disable fuzzy matching in batch endpoint
• PR Journaling & Insights Single Column with Styled Cards #26: Reorder lookup chain to lemma → exact → fuzzy
• Issue Meals Single Column with Styled Cards #27: Comprehensive plan for IS→EN quality fixes (POS bug, broken frequency, lemmatizer verb bias)

Known limitations

The ensk.is reversed dictionary produces mediocre supplemental vocab. Some words get no usable definition (tólf, sjálfur, heitir, æðstur). A proper fix requires the work tracked in Peleke/ensk.is#27. The 8 MDX curated vocab entries are high quality; the ~26 ensk.is supplemental entries are best-effort.

Test plan

• vitest run scripts/__tests__/ingest-grymoire.test.ts — 31/31 passing
• E2E: verify preview page renders at /preview/readings/frigg-knows-the-fates-of-men-gylfaggining-20
• E2E: verify vocab tooltips appear on word click
• Visual: review vocab list for remaining garbage definitions
• Grymoire: verify banner injection is idempotent

🤖 Generated with Claude Code

[claude]

PR Review: feat: Grymoire x Interlinear — Old Norse public reading preview

Great feature overall — clean architecture, good test coverage, and the ingestion pipeline is nicely designed. Here are some issues worth addressing before merge.

---

🔴 Bugs

1. select('*') exposes all columns to anonymous users (app/preview/readings/[slug]/page.tsx)

The getReading() query returns every column on library_readings to unauthenticated users. This will grow as the schema does. Explicitly select only the fields the preview page actually needs:
```ts
.select('id, title, content, language, difficulty_level, reading_overview, slug, vocabulary_data, audio_url')
```

2. Old Norse vocabulary stored in spanish column (supabase/seeds/seed-prose-edda-course.ts:1562–1571)

The seed inserts Old Norse words into lesson_vocabulary_items.spanish:
```ts
.eq('spanish', vocab.old_norse)
// ...
.insert({ spanish: vocab.old_norse, english: vocab.english })
```
This is a schema mismatch that will silently corrupt data and break any UI that treats spanish as Spanish text. If the lesson_vocabulary_items table has a language-specific column structure, this entry needs an is/non column or a generic source_text column.

3. Audio player leaks event listeners (app/preview/readings/[slug]/preview-reading-client.tsx:183–194)

The cleanup in StaticAudioPlayer doesn't remove event listeners before destroying the audio element:
```ts
return () => {
audio.pause()
audio.src = '' // ← listeners still attached
}
```
Should be:
```ts
return () => {
audio.removeEventListener('loadedmetadata', ...)
audio.removeEventListener('timeupdate', ...)
audio.removeEventListener('ended', ...)
audio.pause()
audio.src = ''
}
```
Or use named functions, or AbortController. As-is, if audioUrl changes (e.g. component re-mounts), the old listeners stay active on a garbage-collected element, causing setState calls on stale closures.

4. Icelandic E2E tests silently removed from test run (playwright.config.ts:633)

```ts
// Before:
testMatch: /icelandic..spec.ts/
// After:
testMatch: /(signup|preview)..spec.ts/
```
tests/e2e/icelandic/icelandic-integration.spec.ts no longer runs in any project. The chromium (auth) project excludes it via testIgnore, and chromium-noauth no longer matches it. Verify this is intentional — if those tests need auth now, add them to the chromium project explicitly.

---

🟡 Issues

5. Double DB query per page load (app/preview/readings/[slug]/page.tsx)

generateMetadata and the page component each call getReading(slug) independently — two round-trips to Supabase per request. Consider using React's cache() to deduplicate:
```ts
import { cache } from 'react'
const getReading = cache(async (slug: string) => { ... })
```

6. Seed script falls back to anon key for service operations (supabase/seeds/seed-prose-edda-course.ts:1320)

```ts
const supabaseServiceKey = process.env.SUPABASE_SERVICE_ROLE_KEY || process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!
```
If SUPABASE_SERVICE_ROLE_KEY is missing, the script will silently use the anon key. Writes will fail silently due to RLS, leaving partial seed state. Better to fail fast:
```ts
if (!process.env.SUPABASE_SERVICE_ROLE_KEY) {
console.error('SUPABASE_SERVICE_ROLE_KEY is required for seeding')
process.exit(1)
}
```

7. Language badge hardcoded to "Old Norse" (app/preview/readings/[slug]/preview-reading-client.tsx:461)

```tsx
<span ...>Old Norse
```
This hardcodes the assumption that all preview readings will be Old Norse. Use reading.language with a lookup map if you need human-readable names.

8. Language code mismatch: 'is' vs Old Norse

VocabEntry.language is typed as 'is' (Icelandic ISO 639-1) and the ingestion script assigns language: 'is' to all entries. Old Norse is technically 'non'. If the rest of the system uses these codes for routing or display logic, this will cause silent mismatches. Worth aligning with whatever the rest of the app uses.

---

🟢 Minor / Nits

9. Hardcoded ElevenLabs voice ID (scripts/ingest-grymoire.ts:1247)
```ts
'onwK4e9ZLuTAKqWW03F9' // Icelandic voice: Daniel
```
Consider extracting this to a named constant (DANIEL_VOICE_ID) or a CLI flag (--voice-id) for clarity and future flexibility.

10. any types in ingestion script
Three eslint-disable-next-line @typescript-eslint/no-explicit-any suppressions for the supabase client. The SupabaseClient type from @supabase/supabase-js is generic — worth typing properly to catch column name errors at compile time (which would have caught issue #2 above).

11. is_public policy could benefit from an authenticated analog
The migration only adds an anon read policy. Authenticated users reading public content presumably hit the existing authenticated policy, but worth verifying there's no gap where an authenticated user can't read a public reading (e.g. if the existing auth policy filters by user_id).

---

✅ What's Working Well

• Migration is safe and additive — ADD COLUMN IF NOT EXISTS with sensible defaults, partial indexes are a nice touch.
• Ingestion script is well-structured — pure functions exported for testing, side effects isolated, idempotent upsert on slug.
• Test coverage is solid — 18 unit tests covering edge cases like missing sections, multi-word entries, and slug normalization.
• Accessibility — vocab words have role="button", tabIndex, onKeyDown, and aria-label. Good work.
• Middleware update is minimal and correct — /preview added to public paths without disturbing existing auth logic.

---

The spanish column misuse (issue #2) and the listener leak (issue #3) are the highest priority fixes. The select('*') on a public endpoint (issue #1) is worth locking down before this goes to production.

[claude]

PR Review: feat Grymoire x Interlinear - please see detailed review in next comment

[claude]

PR Review summary: 1) BUG auth.setup.ts has loginButton.click() called twice - remove the first call before the promise setup. 2) SCHEMA seed-prose-edda-course.ts stores Old Norse in a spanish column - migrate or document. 3) SECURITY preview page uses select star on a public route - use explicit column list. 4) CONFIG hardcoded ElevenLabs voice ID should be ELEVENLABS_VOICE_ID env var. 5) RUNTIME seed falls back to anon key instead of service role key. Minor: three any-typed supabase params, stem matching false positives, isPlaying not reset on audioUrl change. Good: idempotent upsert, safe migration, it.runIf pattern, correct anon client for SSR. Generated with Claude Code

[claude]

PR Review — feat: Grymoire x Interlinear — Old Norse public preview

Overall this is a well-scoped, thoughtfully structured PR. The feature set (ingestion pipeline to DB to public preview page) is coherent, test coverage is solid for the exercised paths, and the architectural decisions are sound. Below are findings across multiple dimensions.

Strengths

• Clear separation of concerns — pure/testable parsing functions (extractPassage, extractVocabulary, generateSlug, buildOverview) are side-effect-free and exported for unit testing.
• Idempotent ingestion — upsert on slug plus audio existence check make re-ingestion safe.
• RLS policy is correct — the anon read policy is scoped to is_public = true. No authenticated user data leaks.
• useMemo / useCallback usage — the vocab matcher and tokenizer are memoized correctly, avoiding recomputation on every render.
• Partial indexes in migration — WHERE slug IS NOT NULL and WHERE is_public = true keep the indexes small since most rows will have null slugs.
• Auth setup improvement — switching from networkidle to domcontentloaded plus intercepting the Supabase token response is a meaningful CI reliability improvement.

Bugs / Issues

1. Double loginButton.click() in auth.setup.ts

The button is clicked twice. The waitForResponse promise is set up before the first click (correct), but a second await loginButton.click() appears after await authResponsePromise. This likely double-submits the login form and should be removed.

2. seed-prose-edda-course.ts — wrong column used for Old Norse vocabulary

Old Norse words are stored and queried via a spanish column. This is presumably a pre-existing schema artefact, but it will silently store data in the wrong semantic field and is very confusing for future maintainers. At minimum add a comment explaining this; ideally rename the column when Old Norse support is formalized.

3. extractVocabulary — fragile definition splitting

The regex rest.match(/^([^.]+(?:\.[^.]*?)?)(?:\.\s+(.+))?$/) splits at a sentence boundary but will produce wrong results for definitions containing abbreviations with periods (e.g. "adj. strong, mighty. Also used figuratively."). Worth a unit test covering that edge case.

4. Hardcoded ElevenLabs voice ID

The voice ID 'onwK4e9ZLuTAKqWW03F9' is hardcoded with no env var override. Consider process.env.ELEVENLABS_VOICE_ID || 'onwK4e9ZLuTAKqWW03F9' so the voice can be changed without a code edit.

5. Audio player — no error handling on failed load

If the storage URL is broken or bucket permissions are wrong, the player renders controls but silently fails. An error event listener that hides the player or shows a fallback message would significantly improve the unauthenticated visitor experience.

Security

6. DB-sourced path concatenated into external link

parts[1] from reading_overview (a free-text TEXT column) is concatenated directly into a URL displayed to unauthenticated visitors. For a controlled ingestion pipeline this is acceptable today, but if reading_overview ever becomes writable via another path the risk increases. Consider validating that parts[1] matches a safe path pattern (e.g. /^\/[a-z0-9-\/]+$/) before constructing the URL.

Performance

7. select('*') fetched twice per SSR request

Both generateMetadata and the page component call getReading() independently, each fetching the full row including the potentially large vocabulary_data JSONB. Either deduplicate the fetch, or use a narrower select() in generateMetadata (e.g. only title, reading_overview).

8. buildVocabMatcher — O(tokens x vocab_size) stem scan

The linear scan over stems is fine for typical passage sizes and mitigated by useMemo, but worth noting if vocabulary grows to 500+ entries via the ensk.is batch path.

Code Quality

9. any on Supabase client parameters (three functions)

ensureReadingAudioBucket, generateAndUploadTTS, and upsertReading all use any for the Supabase client. SupabaseClient from @supabase/supabase-js can replace these.

10. language: 'is' for Old Norse content

Old Norse and Modern Icelandic are distinct languages; 'is' is ISO 639-1 for Modern Icelandic. This is a pragmatic approximation for ensk.is lookups, but as Old Norse support grows the distinction will matter for morphological analysis routing. A TODO comment would prevent silent confusion later.

Test Coverage Gaps

• buildVocabMatcher stem-matching logic has no unit test — non-trivial code with real edge cases (short-stem false positives, multi-word entry indexing).
• Audio error handling path is untested.
• extractVocabulary edge case: definition containing a period-abbreviated word.

Summary

Category	Finding	Severity
Bug	Double loginButton.click() in auth setup	Medium
Bug	spanish column used for Old Norse vocab	Medium
Bug	No audio load error handling	Low
Security	DB-sourced path concatenated into external URL without validation	Low
Performance	select('*') fetched twice per SSR render	Low
Quality	Hardcoded ElevenLabs voice ID	Low
Quality	any on Supabase client params	Low
Quality	language: 'is' for Old Norse	Informational

Items 1 (double click) and 2 (wrong column name) are the most actionable before merge. Everything else is low-severity or informational.

[claude]

PR Review: feat: Grymoire x Interlinear. Overall well-structured with good test coverage (18 unit + 11 E2E) and thoughtful gated-preview UX. Issues: (1) Double DB fetch - generateMetadata and PreviewReadingPage both call getReading(slug) independently. Fix with React cache(). (2) Unhandled audio play() promise - rejects on autoplay policy violations on mobile/Safari. Fix: audioRef.current.play().catch(() => setIsPlaying(false)). (3) select('*') over-fetches large columns. Use explicit column list. (4) Stem matcher 3-char minimum is too low - 'eru' will match unrelated words. Raise to 4-5 chars. Missing env var docs: OLLAMA_BASE_URL/MODEL, NEXT_PUBLIC_GRYMOIRE_URL, ENSK_API_URL/KEY. Minor: bare tsx vs npx tsx in ingest:grymoire script. The double DB call and audio promise are the only blocking items.

[claude]

PR Review: Grymoire x Interlinear

Nice feature overall. Notes below from most to least critical.

BUGS

1. Double DB fetch per page load (page.tsx)
   getReading(slug) is called in both generateMetadata and the default export -- two Supabase round-trips per render. Wrap with React.cache() to deduplicate so both share one cached result per render.

2. Unhandled Promise rejection in audio player (preview-reading-client.tsx)
   audioRef.current.play() returns a Promise whose rejection is not caught. Rapid play-then-pause triggers an AbortError in browsers. Add .catch(()=>{}) to suppress the expected interrupt error.

3. Seed script falls back to anon key (seed-prose-edda-course.ts:21)
   The anon key does not have INSERT permissions on RLS-protected tables, so the seed silently fails when SUPABASE_SERVICE_ROLE_KEY is not set. Better to exit early with a clear error message.

CODE QUALITY

4. Dead code in ingest-grymoire.ts
   After reverting to curated MDX vocab (last commit), fetchEnksVocabulary is never called in main(). The function, its EnksBatchResult/EnksBatchResponse interfaces, and tokenizePassageWords are dead code (~100 lines). Remove them, or add a comment explaining intent to revisit with a proper Old Norse lexicon.

5. select all columns on a public-facing page (page.tsx)
   Explicitly listing the columns used by PreviewReadingClient is safer -- any column added to library_readings in future would otherwise be silently included in the public response.

6. Sidebar conditional translation class is unreachable (preview-reading-client.tsx)
   The className ternary on the sidebar div is dead code. PreviewVocabSidebar returns null when entry is null, so this component only mounts when entry is truthy -- the slide-in animation never plays. Either remove the dead conditional, or keep the DOM mounted so the animation fires.

7. Hard-coded ElevenLabs voice ID (ingest-grymoire.ts)
   The voice ID is undocumented. A named constant or ELEVENLABS_VOICE_ID env var makes it configurable and self-documenting.

8. Hard-coded difficulty_level C1 in upsertReading
   Every ingested entry gets C1 regardless of content. If extended to A2 or B2 texts this will silently mislabel them.

9. Duplicate VocabEntry type
   VocabEntry is defined in both ingest-grymoire.ts and preview-reading-client.tsx with slightly different shapes. A shared type in @/types would eliminate the drift.

10. Multiple eslint-disable for no-explicit-any (ingest-grymoire.ts)
    Three functions use any for the Supabase client. SupabaseClient from @supabase/supabase-js types it properly.

MINOR

• VocabularyGrid uses key={i}. key={entry.word} is more stable.
• Seed script is not idempotent -- running twice creates duplicate courses.
• NEXT_PUBLIC_GRYMOIRE_URL is not documented in CLAUDE.md. The localhost:3000 fallback will point to the wrong URL in staging/production.

WHAT IS WORKING WELL

• Migration is clean: partial indexes on slug and is_public are right for the access patterns. The anon RLS policy is tightly scoped to is_public = true with no write access.
• Ingestion script cleanly separates pure functions from I/O side effects -- exactly why the 23 unit tests are easy to write.
• Audio skip-on-re-ingest (existingAudio.length > 0) is a thoughtful idempotency detail.
• buildVocabMatcher stem matching is a pragmatic solution to Old Norse inflection with good inline comments.
• E2E tests gracefully skip when the reading is not ingested -- avoids CI failures on fresh environments.
• Middleware change is one line and correct.
• Ollama fallback in the Icelandic lookup route is a clean, non-intrusive addition for local dev cost savings.

[claude]

PR Review: Grymoire x Interlinear — Old Norse public reading preview

Overall this is a well-scoped feature. The migration, RLS policy, ingestion pipeline, and preview page fit together cleanly. Test coverage (18 unit + 11 E2E) is solid. A few issues worth addressing before merge.

---

BUGS / CORRECTNESS

Double DB call per page load (app/preview/readings/[slug]/page.tsx)

getReading(slug) is called independently in both generateMetadata and PreviewReadingPage. Since the page is force-dynamic, Next.js does not dedupe these — two DB round-trips per request. Fix: wrap with React cache() or pass data through props from a layout.

Localhost fallback bundled into client JS (preview-reading-client.tsx)

This line ships in the production JS bundle:
const baseUrl = process.env.NEXT_PUBLIC_GRYMOIRE_URL || 'http://localhost:3000'

If NEXT_PUBLIC_GRYMOIRE_URL is unset in prod, real users will be linked to localhost. The fallback should be null and the source link should not render when the env var is absent.

Vocab stem matcher false-positive risk (buildVocabMatcher)

'Does the token start with a vocab stem?' with a 3-char minimum is too broad — a stem like 'er' (3 chars) matches 'ert', 'ertu', etc. Consider requiring the stem to cover at least 60% of the token length.

Audio player has no error state (StaticAudioPlayer)

new Audio(audioUrl) fires a network request with no 'error' event listener. A missing or inaccessible URL silently breaks the player. Add an error handler.

---

SECURITY

Hardcoded default API key (scripts/ingest-grymoire.ts)

const apiKey = process.env.ENSK_API_KEY || 'dev-key-local'

'dev-key-local' is committed to source. If the ensk.is service ever accepts this string in a shared environment it becomes an accidental credential. Prefer failing loudly when the key is absent in non-local contexts.

Seed script silently falls back to anon key (supabase/seeds/seed-prose-edda-course.ts)

const supabaseServiceKey = process.env.SUPABASE_SERVICE_ROLE_KEY || process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY\!

The anon key is rejected by RLS on write operations, producing a confusing error. Throw early with a clear message when SUPABASE_SERVICE_ROLE_KEY is missing.

Migration RLS policy is correctly scoped (FOR SELECT TO anon USING (is_public = true)). No issues there.

---

CODE QUALITY

select('*') in getReading fetches all columns including the potentially large vocabulary_data JSONB on every call. generateMetadata only needs title and reading_overview — specifying columns would reduce payload.

Array index as key in VocabularyGrid: use entry.lemma or entry.word so reconciliation is stable if list order ever changes.

Duplicated type definitions — Reading and VocabEntry are defined independently in preview-reading-client.tsx and ingest-grymoire.ts. A schema change requires edits in two places. A shared types file or re-exporting from the script would keep them in sync.

ingest:grymoire npm script inconsistency — other scripts use 'npx tsx', this one uses bare 'tsx'. Pick one convention and confirm tsx is in devDependencies.

---

TESTS

Unit coverage of extractPassage, extractVocabulary, generateSlug, and buildOverview is thorough. One gap: no test for stem-matcher false positives — a token that starts with a vocab stem but is a different word. A test case would prevent regressions in buildVocabMatcher.

Playwright config: moving icelandic tests out of chromium-noauth looks correct (testIgnore only excludes signup/preview specs from the auth project, so icelandic specs should still run). Worth verifying in the test run output.

---

SUMMARY

The RLS + anon client architecture for the public preview is the right call. Main items before merge: double DB call per request, localhost fallback in client-bundled JS, and seed script silent key fallback. Everything else is polish.

[claude]

PR Review: Grymoire × Interlinear — Old Norse Public Reading Preview

Overall this is a well-structured feature with solid test coverage (18 unit + 11 E2E), clean component decomposition, and a thoughtful ingestion pipeline. A few items worth addressing before merge:

---

🔴 Issues

1. Double DB fetch per page load (page.tsx)

generateMetadata and PreviewReadingPage both call getReading(slug) independently, making two round-trips to Supabase per request. Wrap with React's cache() to deduplicate:

```ts
import { cache } from 'react'

const getReading = cache(async (slug: string) => {
const supabase = createAnonClient()
const { data, error } = await supabase
.from('library_readings')
.select('id, title, content, language, difficulty_level, reading_overview, slug, vocabulary_data, audio_url')
.eq('slug', slug)
.eq('is_public', true)
.single()
if (error || !data) return null
return data
})
```

2. select('*') fetches all columns including unexpected/sensitive future columns

page.tsx:28 uses .select('*'). Should explicitly select only the columns the Reading interface needs. This is both a performance and a defensive data-exposure concern — if new columns are added to library_readings (e.g. internal flags, user data), they'd be exposed to anonymous visitors.

3. Stem matching causes false positive definitions

buildVocabMatcher registers every vocab entry's first word as a stem and uses cleanText.startsWith(stem) for fallback matching. With a 3-char minimum, a vocab entry for "hér" (stem "hér") would match the word "hérna". For a passage with dozens of inflected Old Norse forms, this will silently show wrong definitions. Consider making stem matching opt-in (only for entries where lemma !== word) or requiring a minimum stem-to-token length ratio.

---

🟡 Warnings

4. LockedTabBar hover timeout not cleaned up on unmount

hoverTimeoutRef.current is set in hideTabDelayed() but there's no useEffect cleanup to clear it if the component unmounts while a timeout is pending. This causes a state update on an unmounted component:

```ts
useEffect(() => {
return () => {
if (hoverTimeoutRef.current) clearTimeout(hoverTimeoutRef.current)
}
}, [])
```

5. StaticAudioPlayer — event listeners not removed in cleanup

The useEffect cleanup sets audio.src = '' but doesn't call removeEventListener for loadedmetadata, timeupdate, and ended. While the audio object is paused and de-referenced, explicitly removing listeners is cleaner and avoids potential GC delay:

```ts
return () => {
audio.removeEventListener('loadedmetadata', onMetadata)
audio.removeEventListener('timeupdate', onTimeUpdate)
audio.removeEventListener('ended', onEnded)
audio.pause()
audio.src = ''
}
```

6. Hardcoded ElevenLabs voice ID in ingest-grymoire.ts

'onwK4e9ZLuTAKqWW03F9' is passed as a positional argument to generateAndUploadTTS. If the voice ever needs to be changed or tested, it requires a code edit. An ELEVENLABS_VOICE_ID env var with this as default would be more flexible.

7. seed-prose-edda-course.ts silently falls back to anon key

```ts
const supabaseServiceKey = process.env.SUPABASE_SERVICE_ROLE_KEY || process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!
```

The seed script needs service role permissions to write to courses, lessons, etc. Falling back to anon key will produce cryptic RLS errors. Should hard-fail with a clear message if SUPABASE_SERVICE_ROLE_KEY is missing.

8. Multiple supabase: any types in ingest-grymoire.ts

Three functions suppress the type with eslint-disable-next-line @typescript-eslint/no-explicit-any. These could use SupabaseClient from @supabase/supabase-js with a generic type parameter instead.

---

🟢 Minor / Suggestions

9. NEXT_PUBLIC_GRYMOIRE_URL default is http://localhost:3000 in a client component

In PreviewReadingClient, the fallback process.env.NEXT_PUBLIC_GRYMOIRE_URL || 'http://localhost:3000' will point preview readers to localhost in production if the env var isn't set at build time. Either add a prod default or validate the var is set during the build.

10. Multi-word vocab entry indexing could silently shadow single-word entries

In buildVocabMatcher, multi-word entries like "trúa á" are indexed under their first word "trúa". The comment if (!exact.has(firstWord)) correctly avoids overwriting an existing single-word entry, but users clicking "trúa" will see the sidebar for "trúa á" which may feel confusing. Worth a comment explaining the intentional design.

11. extractVocabulary test asserts no notes but the function always sets notes: ''

The test at ingest-grymoire.test.ts:186 validates expect(orlog!.notes).toBe('') — which will always pass since the implementation always sets notes: ''. If notes are intended to be populated in future, this is a useful guard; otherwise it's testing an implementation constant rather than behavior.

---

✅ What's working well

• The injectBanner idempotency check is a great safeguard for re-runs
• parseGrymoireMDX tests use it.runIf(mdxExists) cleanly — tests won't fail in CI without the Grymoire repo
• Playwright config correctly separates auth/noauth test projects
• Migration uses IF NOT EXISTS guards and partial indexes — production-safe
• The reading_overview pipe-separated format (SourceLabel|/path) is a clever way to encode structured data without a schema change
• force-dynamic export correctly ensures the page re-validates on each request

[claude]

PR Review placeholder

[claude]

PR Review: Grymoire x Interlinear (Part 1 of 2 - Bugs and Performance)

BUGS

1. Double login click (auth.setup.ts): loginButton.click() fires twice around the authResponsePromise setup -- once before and once after the await. This submits the form twice and causes flaky auth. Remove one call.

2. Case-sensitive vocab matching (preview-reading-client.tsx): buildVocabMatcher keys the map by entry.word.toLowerCase() but matchToken calls exact.get(cleanText) without lowercasing cleanText. Any sentence-initial capital will fail to match. Fix: return exact.get(cleanText.toLowerCase()) ?? null

3. Audio listeners not removed (StaticAudioPlayer): The useEffect cleanup sets audio.src to empty string but never calls removeEventListener for loadedmetadata, timeupdate, or ended. These listeners stay attached causing a memory leak on every audioUrl change.

PERFORMANCE

4. getReading() called twice per render: generateMetadata and the page component both call getReading(slug) independently. With force-dynamic, Next.js does not cache or deduplicate. Wrap with React.cache() to share one Supabase round-trip.

5. Wide select on a JSONB-heavy table: vocabulary_data can be large but generateMetadata only needs title and reading_overview. Narrow the select, or share the cache()-wrapped full fetch.

[claude]

PR Review (Part 2 of 2 - Security, Code Quality, Tests, Positives)

SECURITY

6. grymoireUrl built from unvalidated DB content: reading_overview is split on pipe and the second segment becomes a URL path. The startsWith('/') check prevents javascript: URIs which is good, but any DB update can inject an arbitrary path on the Grymoire domain. Add a stricter allowlist pattern (e.g. only /[a-z0-9/_-]+).

7. Anon client fetches all columns: If library_readings gains columns that should not be public, the anon client will serve them. Enumerate only the fields the preview page needs.

CODE QUALITY

8. Old Norse in the 'spanish' column (seed-prose-edda-course.ts): .insert({ spanish: vocab.old_norse }) stores Old Norse in a column named 'spanish'. Add a TODO at minimum; ideally a migration adds a language-agnostic source_word column.

9. Hardcoded ElevenLabs voice ID: The ID is buried in main(). Move to a module-level constant or ELEVENLABS_VOICE_ID env var.

10. any-typed Supabase client: ensureReadingAudioBucket, generateAndUploadTTS, and upsertReading all take supabase: any. Use SupabaseClient from @supabase/supabase-js for type safety.

11. GARBAGE_DEFS Set inside batch loop: The Set is re-created on every iteration in fetchEnksVocabulary. It is a static constant -- move it to module scope.

12. Difficulty hardcoded to C1 in upsertReading: Correct for Prose Edda but wrong for other entries. Accept as a parameter or derive from frontmatter.

TESTS

13. E2E preview tests need live DB data: Tests require the ingest script to have populated the slug first. Mock Supabase or seed a fixture row in the E2E setup hook for CI reliability.

THINGS DONE WELL

• Pure parsing functions (extractPassage, extractVocabulary, generateSlug, etc.) cleanly separated from I/O and covered by 18 unit tests.
• Idempotent injectBanner with a sentinel string -- safe to re-run.
• is_public RLS policy correctly scopes anon access to only flagged rows.
• Partial indexes on slug and is_public fit the access pattern.
• NonVocabWord CTA tooltip handles keyboard events and outside-click correctly.
• buildVocabMatcher first-word indexing for multi-word entries is pragmatic.
• Ollama fallback in the lookup API is a useful dev ergonomic with no production overhead.
• Auth setup: domcontentloaded over networkidle and token-response interception are meaningfully more reliable.

[claude]

PR Review: feat: Grymoire x Interlinear

Nice feature. The ingestion pipeline is well-structured with pure functions separated from side effects, strong unit test coverage (31/31), and the preview UX is solid. Here are the issues I found:

---

Bugs

1. Double DB fetch in page.tsx — use React.cache()

Both generateMetadata and PreviewReadingPage call getReading(slug) independently. This fires two identical queries per render. Wrap the function with cache() from react to deduplicate.

2. audio.play() unhandled Promise in StaticAudioPlayer

HTMLAudioElement.play() returns a Promise. A rapid play/pause triggers an AbortError unhandled rejection. Fix:

audioRef.current.play().then(() => setIsPlaying(true)).catch(() => {})

3. ensureCltkCacheTable calls supabase.rpc("exec_sql") which likely does not exist

Redundant — the migration already handles table creation. Silently no-ops if the RPC is not defined. Drop this function.

---

Security / Data Exposure

4. SELECT * on a public unauthenticated page

page.tsx does select("*") from library_readings with no auth. Any column added to this table in the future will be silently exposed. Enumerate only the columns that the preview page actually needs.

5. cltk_lemma_cache RLS policy grants access to everyone, not just service role

Service role bypasses RLS by default and does not need a policy. USING (true) means anon and authenticated roles can also read and write this table. Remove the policy (service role skips RLS anyway) or scope it with TO service_role.

---

Code Quality

6. supabase: any in multiple ingest script functions

getCachedLemma, getCachedLemmas, storeCachedLemmas, enrichOldNorseWords, generateAndUploadTTS, upsertReading, and others all take supabase: any. Use SupabaseClient from @supabase/supabase-js.

7. Array index as key in VocabularyGrid

vocabulary.map((entry, i) => <div key={i}> is a React antipattern. Use a stable key like key={entry.lemma || entry.word}.

8. Seed script falls back to anon key silently

process.env.SUPABASE_SERVICE_ROLE_KEY || process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!

If the service role key is absent, inserts will silently fail at RLS without an obvious error. Throw early if the service role key is missing.

9. GARBAGE_DEFS Set recreated on every batch iteration

In fetchEnksVocabulary the Set is defined inside the for loop body. Move it outside the function.

10. Hardcoded "Old Norse" in the preview header

The language badge renders a literal string "Old Norse" rather than {reading.language}. Future readings in other languages will display incorrectly.

---

Minor

• NEXT_PUBLIC_GRYMOIRE_URL is used in preview-reading-client.tsx but is not documented in CLAUDE.md. Its default is http://localhost:3000, which is wrong in production.
• E2E preview tests skip gracefully when the reading is not ingested. CI will silently pass without exercising the preview page unless the test reading is pre-seeded. Worth noting in the test plan.
• Serial CLTK enrichment runs at ~30s/word. For 20+ words this is a 10+ minute first run. One-time cost due to caching, but worth documenting in the script usage comment.

---

What is Good

• Pure functions in ingest-grymoire.ts are well-tested (31/31 unit tests)
• Idempotent banner injection is handled correctly
• is_public = true filter on both the RLS policy and the query is solid defense-in-depth
• CLTK Postgres cache is a smart solution for ephemeral Cloud Run state
• Graceful fallback when ENSK_API_URL is not set
• Middleware change is minimal and correct

[claude]

PR Review: Grymoire x Interlinear — see full review in follow-up comment

[claude]

done

[claude]

PR REVIEW PART 1 of 4 -- Bugs. (1) DOUBLE-CLICK in auth.setup.ts: loginButton.click() is called twice. Once before authResponsePromise is set up, and once after. The first click fires the auth request before the listener is active so the listener may miss the response, and the second click re-submits the form. Drop the first click. (2) WRONG COLUMN NAME in seed-prose-edda-course.ts: lesson_vocabulary_items is queried with .eq('spanish', vocab.old_norse). This was copy-pasted from a Spanish seeder. If the column is actually named 'spanish' in the schema that is a semantic problem; if it is named something else the query always returns null and silently inserts duplicates. (3) NULL-UNSAFE reading.id in seed summary: The final summary uses reading.id inside a ternary but reading can be null when the slug is not ingested. The ternary handles the string correctly but reading.id will throw if reading is null.

[claude]

PR REVIEW PART 2 of 4 -- Security and Performance. (4) exec_sql RPC WITH RAW SQL in ensureCltkCacheTable (scripts/ingest-grymoire.ts): The fallback table-creation path calls supabase.rpc('exec_sql', { sql_query: 'CREATE TABLE IF NOT EXISTS ...' }). A generic exec_sql RPC in production is arbitrary SQL execution via the service role. The migration 20260307_cltk_lemma_cache.sql already creates this table, making the runtime auto-create completely redundant. Remove it and rely on the migration. If a runtime guard is wanted, use a purpose-specific RPC that does not accept arbitrary SQL. (5) SILENT ANON-KEY FALLBACK in seed script: When SUPABASE_SERVICE_ROLE_KEY is missing the seed falls back to NEXT_PUBLIC_SUPABASE_ANON_KEY and silently fails on RLS-protected writes. Fail fast with a clear error message instead. (6) DUPLICATE SUPABASE QUERY in app/preview/readings/[slug]/page.tsx: getReading(slug) is called independently in both generateMetadata and PreviewReadingPage -- two round-trips for the same row per request. Wrap with React.cache (import { cache } from 'react') to deduplicate within a single render pass at no cost.

[claude]

PR REVIEW PART 3 of 4 -- Code Quality. (7) supabase TYPED AS any IN ingest-grymoire.ts: Seven no-explicit-any suppressions for the supabase parameter. Use SupabaseClient from @supabase/supabase-js to eliminate them all. (8) MISLEADING STOPWORD COMMENT: ingest-grymoire.test.ts says 'ok' is a content word not in stopwords, but 'ok' IS in OLD_NORSE_STOPWORDS. The test passes because it calls tokenizePassageWords with lang='is' (Icelandic, the default), not Old Norse. Clarify the comment so it does not mislead future readers.

[claude]

PR REVIEW PART 4 of 4 -- What Is Good and Summary. WHAT IS GOOD: (a) Pure/impure separation in ingest-grymoire.ts is excellent: all parsing logic is exported and unit-tested independently of Supabase/ElevenLabs. (b) Idempotency is thorough: slug-based upsert, banner sentinel check, TTS skip-if-exists. Safe to re-run at any point. (c) RLS migration is correct: anon read policy scoped to is_public=true is exactly the right pattern for public previews. (d) E2E tests gracefully skip when the reading is not yet ingested. testIgnore/testMatch split in playwright.config.ts is clean. (e) buildVocabMatcher uses a pre-built Map for O(1) lookups; multi-word entry indexing by first word is a practical touch. (f) Auth setup refactor (waiting for auth/v1/token response instead of networkidle) is a meaningfully more reliable signal -- will be solid once the double-click is fixed. (g) Known limitations honestly documented: GARBAGE_DEFS band-aid with an issue tracker reference is much better than silent quality degradation. BEFORE MERGE: Fix the double-click (issue 1) and exec_sql RPC (issue 4). The seed script issues (2, 3, 5) can be batched. Performance and code quality items are low-priority follow-up. Reviewed with Claude Code.

[claude]

PR Review: Grymoire x Interlinear — Old Norse Public Reading Preview

This is a solid feature PR. The ingestion pipeline is well-engineered — pure functions separated from side effects, idempotent upserts and banner injection, and meaningful test coverage (31/31). Some issues worth addressing before merging:

---

Bugs

1. Double click in auth.setup.ts

loginButton.click() is called twice — once at line ~2930 (before the response promise is awaited) and again at line ~2954 (after). This fires two auth requests and will produce flaky test behavior.

The correct pattern: set up waitForResponse first, click once, then await the promise.

---

2. GARBAGE_DEFS Set created inside a loop (scripts/ingest-grymoire.ts)

Inside fetchEnksVocabulary, GARBAGE_DEFS is new Set([...]) defined inside the inner for (const [, result] of Object.entries(data.results)) loop. This re-allocates the Set on every word. Move it to module scope alongside ICELANDIC_STOPWORDS.

---

3. reading may be undefined in the final summary (supabase/seeds/seed-prose-edda-course.ts)

The final console.log summary references reading?.id, but reading is only conditionally assigned when the DB lookup succeeds. If the reading was not found, the script warns and continues — and the final summary template literal will print undefined. Add an explicit guard or use the outer-scoped reading variable consistently.

---

Schema / Naming

4. Old Norse vocabulary stored in the spanish column

seed-prose-edda-course.ts inserts Old Norse words via:

{ spanish: vocab.old_norse, english: vocab.english }
.eq('spanish', vocab.old_norse)

This is a schema workaround that will cause confusion in queries and tooling. Consider renaming the column to something language-agnostic (source_word) or adding a proper column in a follow-up migration. Noted this may be pre-existing debt, but the seed script perpetuates it.

---

Security

5. select('*') on the public preview page

app/preview/readings/[slug]/page.tsx fetches from library_readings with .select('*'). If the schema gains sensitive columns (internal notes, draft metadata, etc.), they will be exposed to anonymous visitors. Prefer explicit column selection:

id, title, content, language, difficulty_level, reading_overview, slug, vocabulary_data, audio_url

6. exec_sql RPC in ensureCltkCacheTable

The ingestion script calls supabase.rpc('exec_sql', { sql_query: '...' }) as a fallback to create the CLTK cache table if missing. The SQL is hardcoded and the script runs with the service role key, so immediate risk is low. However, confirm that the exec_sql RPC function is restricted to service_role only and not callable by anon or authenticated roles. The proper migration at 20260307_cltk_lemma_cache.sql is the right solution; the auto-create fallback in the script can be removed once the migration is applied everywhere.

---

Minor

7. NEXT_PUBLIC_GRYMOIRE_URL defaults to localhost in client component

const baseUrl = process.env.NEXT_PUBLIC_GRYMOIRE_URL || 'http://localhost:3000'

NEXT_PUBLIC_* vars are inlined at build time. If this is unset in production, all Grymoire source links will point to localhost. Add it to environment variable documentation.

8. Audio event listeners not explicitly removed

StaticAudioPlayer adds loadedmetadata, timeupdate, and ended listeners in useEffect, but cleanup only calls audio.src = '' without removeEventListener. Low-risk since the audio object is local to the effect closure, but explicit listener removal is correct practice.

9. VocabularyGrid uses array index as key

key={i} for vocabulary list items is harmless given static data, but entry.word would be a more stable and semantically correct key.

---

Positive Notes

• The pure/impure function split in ingest-grymoire.ts makes unit testing clean and the 31 unit tests cover all the extraction logic well.
• Idempotent upsert (on slug) and banner injection (sentinel string check) are both well-designed for re-runnable scripts.
• The ensk.is vocabulary priority system (MDX curated > ON Zoega > Modern IS) is clearly documented and correctly implemented.
• New migration adds a partial index on is_public = true and a targeted anon RLS policy — no unnecessary privilege escalation.
• auth.setup.ts intercepting the Supabase token response rather than polling networkidle is a meaningful reliability improvement overall.
• Middleware /preview path addition is minimal and correct.

---

Summary

Issue	Severity
Double click in auth.setup.ts	Medium — flaky tests
GARBAGE_DEFS allocated in loop	Low — perf
reading possibly undefined in summary	Low — script crash edge case
Old Norse in spanish column	Medium — schema correctness
select('*') on public page	Low-Medium — defense in depth
exec_sql RPC exposure	Low — confirm service_role restriction
Localhost default for Grymoire URL	Low — prod config

[claude]

PR Review: see inline notes

[claude]

PR Review: feat: Grymoire x Interlinear — Old Norse public preview

Overall this is solid work. Clean separation between pure functions (tested) and side-effectful I/O in the ingestion pipeline, clever use of bioinformatics sequence alignment for parallel text, and the preview page handles the public/authenticated split correctly. A few things worth addressing:

---

Potential Bugs

LockedTabBar — hover timeout not cleared on unmount

hoverTimeoutRef is cleared inside showTab/cancelHide but not when the component unmounts. If the 200ms delay fires after unmount you get a React warning. Add a dedicated cleanup effect:

useEffect(() => {
  return () => { if (hoverTimeoutRef.current) clearTimeout(hoverTimeoutRef.current) }
}, [])

buildOverview — pipe in subtitle breaks preview page parsing

When sourceLabel is empty the function returns frontmatter.subtitle raw. If any subtitle contains |, the preview page incorrectly treats it as "label|/path" and tries to construct a Grymoire URL. Low probability today but worth guarding against.

buildVocabMatcher — multi-word entries silently shadowed

if (!exact.has(firstWord)) { exact.set(firstWord, entry) } — a single-word entry whose key matches the first word of a multi-word phrase wins silently. Worth a comment explaining the precedence rule.

---

Security

seed-prose-edda-course.ts — anon key fallback

const supabaseServiceKey = process.env.SUPABASE_SERVICE_ROLE_KEY || process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!

If SUPABASE_SERVICE_ROLE_KEY is unset the seed falls back silently to the anon key, causing cryptic write failures. Better to fail fast with process.exit(1).

page.tsx — select('*') on public endpoint

All columns are sent to unauthenticated users. If library_readings ever gains sensitive internal columns they would be exposed. Explicitly listing needed columns is a cheap defense-in-depth improvement.

---

Performance

getLookupModel() recreates provider instance per request

When Ollama is configured, createOpenAICompatible(...) runs on every API call. Moving it to module scope avoids unnecessary allocation, though negligible at low volume.

---

Code Quality

VocabEntry interface duplicated between client and script

preview-reading-client.tsx defines a local VocabEntry that mirrors the exported one from ingest-grymoire.ts. The duplication is understandable since the script is not safely importable from app code, but a comment explaining why would prevent future confusion.

key={i} in VocabularyGrid

entry.word or entry.lemma would be semantically cleaner keys than array index, even though the list is static.

NEXT_PUBLIC_GRYMOIRE_URL fallback to localhost in client component

If this env var is not set at build time in production, all Grymoire source links point to http://localhost:3000. Should be documented in .env.example.

---

What is Working Well

• Migration: partial indexes and USING (is_public = true) RLS for anon are exactly right.
• Ingestion architecture: pure functions exported and unit-tested, I/O-heavy orchestration is not. 31/31 passing is reassuring.
• Needleman-Wunsch alignment: smart use of global sequence alignment for parallel literary text. Three-tier matching (exact, normalized, Levenshtein) is well thought out with solid test coverage for edge cases.
• Middleware: adding /preview to publicPaths is correct, and the Playwright config properly routes preview tests to the no-auth project.
• E2E graceful skips: test.skip when data has not been ingested is pragmatic for a seed-dependent feature.
• StaticAudioPlayer: cleanup correctly pauses and clears audio.src to stop network buffering.

---

Minor Nits

• INTERLINEAR_BANNER_MARKER = 'Study this passage interactively' is the idempotency sentinel for banner injection. If the generated copy in buildBannerHtml ever changes, deduplication breaks silently. A stable HTML attribute like data-interlinear-banner would be more robust.
• The passage extraction regex (<details[^>]*>) works for current MDX but would need updating if <details> elements gain open attributes or custom classes in the source files.

[claude]

PR Review: Grymoire x Interlinear --- Solid feature. The NW alignment for ON/IS vocab pairing is clever and well-documented. 31/31 tests passing. Bugs to address: (1) auth.setup.ts: loginButton.click() is called twice. The waitForResponse promise is established after the first click fires so the token response may be in-flight before the listener attaches, then the form is submitted a second time. Fix: create the promise before clicking and only click once. (2) VocabularyGrid: onCardClick prop is silently dropped. PreviewReadingClient passes onCardClick to VocabularyGrid but the component only accepts { vocabulary: VocabEntry[] }. Clicking a grid card never opens the sidebar. Add onCardClick to the props interface or remove the prop. (3) tokenizeForAlignment uses a Lokasenna-specific citation regex. Other Eddic citations (Voluspa 1, Havamal 77, etc.) pollute the token stream. A general pattern for dash-prefixed citation lines would be safer. Issues: (4) ensureCltkCacheTable calls exec_sql RPC which Supabase does not expose by default. Since the migration already creates this table the fallback always fails with a confusing warning. Remove the function and rely on the migration. (5) Seed script falls back to anon key when SUPABASE_SERVICE_ROLE_KEY is missing, causing silent RLS failures. Prefer a hard exit. (6) Old Norse words are stored in lesson_vocabulary_items.spanish column. A comment would help. Minor: NonVocabWord attaches a document event listener per instance; matchVocab is computed twice per token; createAnonClient could be a shared lib. What works well: NW alignment is clean and well-tested; idempotent ingest; correct RLS scoping; honest GARBAGE_DEFS comment. Summary: Fix bugs 1 and 2 before merge (functional regressions). The rest can be tracked as follow-ups.

[claude]

Code Review — Grymoire x Interlinear (PR 271)

Solid PR overall. NW alignment for ON/IS pairing is a genuinely interesting approach, the pure-function test coverage is thorough (31 tests), and the public preview architecture is clean. Issues to address before merge:

---

BUGS

1. loginButton.click() called twice (tests/e2e/auth.setup.ts)

The button fires before the response watcher is registered, so the first auth response is missed, then the button is clicked a second time. Fix: create the waitForResponse promise BEFORE clicking, then click once.

2. select('*') on a public unauthenticated endpoint (app/preview/readings/[slug]/page.tsx)

getReading() does .select('*') via the anon client. RLS filters rows correctly but all columns are returned to unauthenticated visitors — any future internal field is silently exposed. Use an explicit column list instead.

3. ensureCltkCacheTable creates tables at runtime via exec_sql RPC (scripts/ingest-grymoire.ts)

A migration already exists (20260307_cltk_lemma_cache.sql) — this runtime fallback is dead code once migrations are applied, and exec_sql may not exist in all Supabase projects. Remove it; rely solely on the migration.

---

ISSUES

4. Hardcoded ElevenLabs voice ID — 'onwK4e9ZLuTAKqWW03F9' buried in main() ~line 878. Move to process.env.ELEVENLABS_VOICE_ID_ICELANDIC or a named constant.

5. Seed script uses 'spanish' column for Old Norse words — supabase/seeds/seed-prose-edda-course.ts ~line 295 does .insert({ spanish: vocab.old_norse }). Schema abuse that will collide with Spanish vocabulary if the table is reused. Add a TODO comment.

6. reading_overview pipe encoding is fragile — 'SourceLabel|/realm/path' breaks silently if the label contains a pipe. Two separate columns (source_label TEXT, grymoire_path TEXT) would be cleaner.

7. NEXT_PUBLIC_GRYMOIRE_URL defaults to localhost:3000 in production — if not set, Grymoire links silently point to localhost. Document in CLAUDE.md env vars.

8. StaticAudioPlayer uses imperative Audio object — new Audio(audioUrl) in useEffect bypasses the native media session API and misbehaves on iOS Safari. An audio element ref with preload=metadata is more robust and accessible.

9. VocabEntry.language always 'is' for Old Norse entries — fetchEnksVocabulary(passage, 'non') produces entries with language: 'is' hardcoded. Should reflect the actual language if this field matters downstream.

---

POSITIVES

• NW implementation is clean and well-documented; scoring constants (SCORE_EXACT, SCORE_NORMALIZED, GAP_PENALTY) make the algorithm readable.
• Pure functions well-separated from side effects; 31-case test suite covers meaningful edge cases.
• Middleware change is minimal and correct — /preview as a single publicPaths entry.
• is_public + anon RLS policy is exactly the right pattern for anonymous preview access.
• Idempotent banner injection via sentinel string is a solid design.
• Garbage definition blocklist explicitly flagged as a band-aid with Peleke/ensk.is#27 reference — the right way to ship known debt.
• Ollama fallback in getLookupModel() is clean for local dev.

---

SUMMARY — Bugs: 3 | Issues: 6 | Positives: many

Highest priority: the double-click bug in auth.setup.ts and select('*') on the public endpoint. The 'spanish' column for Old Norse is worth a TODO comment before merge.

[claude]

test comment please ignore

[claude]

PR Review: Grymoire x Interlinear

This is a well-scoped feature. The NW alignment architecture, test coverage (31/31), and RLS policy for anonymous reads are all solid. Here are the issues worth addressing before merge.

Bugs

1. Double login click in tests/e2e/auth.setup.ts: The refactor left the original loginButton.click() in place AND added a new one after setting up authResponsePromise. The form gets submitted twice causing a race or duplicate auth token request. Fix: remove the first click so the response promise is set up before the single click fires.

2. buildVocabMatcher applies stem matching despite the PR description saying exact-match-only: preview-reading-client.tsx includes STEM_SUFFIXES stripping and ON-form index building. Either update the description or remove the stem logic since it can produce false-positive matches on short shared stems.

Security and Architecture

3. ensureCltkCacheTable uses an exec_sql RPC not defined in any migration: supabase.rpc with exec_sql requires a custom DB function with elevated permissions. The migration 20260307_cltk_lemma_cache.sql already creates the table correctly, making this runtime fallback redundant and risky. Recommend removing ensureCltkCacheTable entirely and documenting that the migration must be applied first.

4. seed-prose-edda-course.ts silently falls back to the anon key: SUPABASE_SERVICE_ROLE_KEY falling back to NEXT_PUBLIC_SUPABASE_ANON_KEY produces cryptic RLS errors instead of a clear failure. Better to fail fast.

5. seed-prose-edda-course.ts repurposes the spanish column for Old Norse vocabulary via .insert({ spanish: vocab.old_norse }). Works today but is schema debt worth tracking.

Minor Nits

6. Two env vars for the same concept: buildBannerHtml reads NEXT_PUBLIC_INTERLINEAR_URL, PreviewNavBar reads NEXT_PUBLIC_APP_URL. Worth consolidating and documenting in CLAUDE.md.

7. GatedButton accessibility: no aria-disabled attribute. Screen reader users will not know the buttons are restricted before activating them.

8. NEXT_PUBLIC_GRYMOIRE_URL defaults to localhost:3000 in prod fallback -- if unset in staging/prod, public preview pages link to localhost. Should be in CLAUDE.md.

What is working well

• Needleman-Wunsch alignment is elegant and well-tested, including edge cases for gaps and structural rewrites.
• The is_public RLS policy for anonymous reads is correctly scoped.
• Ingestion script is properly idempotent (upsert on slug, banner injection guard, audio skip-if-exists).
• vocabulary_data JSONB and client-side matching keep the preview page fast with zero extra API calls at render time.
• force-dynamic on the preview page and the /preview public path in middleware are both correctly handled.
• E2E tests gracefully skip when the reading has not been ingested yet.

Generated with Claude Code (https://claude.com/claude-code)

[claude]

PR Review: Grymoire x Interlinear Old Norse Public Reading Preview

Solid feature work overall. The Needleman-Wunsch alignment is a clever application of bioinformatics to text alignment, and the pipeline is well-layered (MDX curated vocab → ensk.is supplemental → ON/IS pairing). The 68 unit tests and graceful E2E skips when a reading is not ingested are both good practices.

Bugs / Correctness

1. Double DB fetch in page.tsx

getReading(slug) is called independently in both generateMetadata() and the page component. With force-dynamic, these are two separate Supabase round-trips per request. Next.js fetch deduplication only applies to native fetch() calls, not Supabase client calls. Consider unstable_cache or restructuring to avoid the duplicate query.

2. GARBAGE_DEFS set re-created inside the batch entry loop

In fetchEnksVocabulary() at ingest-grymoire.ts:613, const GARBAGE_DEFS = new Set([...]) is defined inside the loop over batch results, allocating a new Set on every iteration. This should be a module-level constant.

3. LockedTabBar timeout ref not cleared on unmount

hoverTimeoutRef.current is never cleared in a useEffect cleanup. If the component unmounts while a hover timeout is pending, the callback will try to call setVisibleTab on an unmounted component. Add a cleanup: return () => { if (hoverTimeoutRef.current) clearTimeout(hoverTimeoutRef.current) }.

Security

4. select('*') exposes all columns of public readings to anon users

getReading() in page.tsx uses .select('*') on library_readings with the anon key. The is_public = true RLS filter is correct, but returning all columns means any future column added to the table is automatically public. Prefer an explicit column list scoped to what the page actually needs.

5. exec_sql RPC in ensureCltkCacheTable()

The ingestion script calls supabase.rpc('exec_sql', { sql_query: ... }) to create the table at runtime (ingest-grymoire.ts:402). The migration file 20260307_cltk_lemma_cache.sql already handles table creation, making this redundant. More importantly, the existence of a generic exec_sql RPC that accepts arbitrary SQL should be audited: if it is callable by any role beyond service_role, it is a significant vulnerability. Recommend removing the runtime fallback and letting a missing-table error surface clearly.

6. ENSK_API_KEY silently defaults to 'dev-key-local'

Both enrichOldNorseWords() and fetchEnksVocabulary() (ingest-grymoire.ts:467, 555) fall back to 'dev-key-local' when ENSK_API_KEY is unset. If the production ensk.is API accepts that key, this is a credential bypass. Prefer failing loudly when the env var is missing and the API path is actually being exercised.

Performance / Design

7. Hardcoded fallback URL in fetchEnksVocabulary()

ingest-grymoire.ts:552 falls back to the production ensk.is Cloud Run URL when ENSK_API_URL is not set. The high-level guard at line 843 prevents this in normal flow, but the fallback URL being present means the function silently hits production if called directly without the env var. Removing the fallback URL makes the invariant explicit.

8. generateMetadata fetches large JSONB columns unnecessarily

generateMetadata only uses title and reading_overview, but the shared getReading() call also fetches vocabulary_data and token_vocab_map, which can be large JSONB blobs. A lighter metadata-only query would reduce unnecessary data transfer on every page load.

Minor / Nits

9. Localhost fallback for Grymoire URL in client component

preview-reading-client.tsx:694 falls back to http://localhost:3000 when NEXT_PUBLIC_GRYMOIRE_URL is unset. This generates localhost links in production for unconfigured deployments. Render no link when the env var is absent, or document it as a required env var.

10. Hardcoded ElevenLabs voice ID

ingest-grymoire.ts:984: 'onwK4e9ZLuTAKqWW03F9' should be an env var (ELEVENLABS_VOICE_ID) or at minimum a named constant at the top of the file with the voice name noted in a comment.

11. VocabularyGrid uses index as key

key={i} is fine for a static list but key={entry.word} is semantically stronger.

12. Missing slug format validation

No length or format check on the slug URL param before the DB query. A simple guard at the top of getReading() is defensive: reject slugs over 200 chars or containing non-slug characters before hitting the database.

What is Working Well

• NW alignment is well-documented and correctly scoped to ingestion time (no runtime cost)
• Idempotent ingestion: upsert on slug, skip TTS if audio exists, skip banner if already present
• cltk_lemma_cache RLS correctly restricted to service_role
• audio_url pre-cached at ingestion, no TTS calls on page load
• E2E tests gracefully skip when the reading has not been ingested (no flaky CI failures)
• token_vocab_map approach (build surface-to-lemma at ingestion via NW + batch API) is cleaner than the previous runtime suffix stripping

Items 1-3 (bugs) and 4-6 (security) should be addressed before merge, particularly the select('*') on public reads and the exec_sql RPC audit.

[claude]

test comment from review bot

[claude]

Posting full review now (replacing placeholder comment above)...

[claude]

PR Review: feat - Grymoire x Interlinear Public Reading Preview

Overall this is a well-scoped, thoughtfully documented PR. The Needleman-Wunsch alignment approach is clever and the test coverage for pure parsing/alignment logic is solid (31 tests). A few things worth addressing before merge.

[claude]

Bugs

1. Double login click in tests/e2e/auth.setup.ts

loginButton.click() is called twice — once before authResponsePromise is set up, and once after. The first click fires before the response listener is registered; the second click may not trigger a second auth request (button may be disabled/loading by then). This will cause flaky auth setup.

Fix: register the listener before the click, then click once.

---

2. seed-prose-edda-course.ts uses spanish column for Old Norse

The vocabulary insert uses { spanish: vocab.old_norse, english: vocab.english }. The column is named spanish but stores Old Norse. This silently succeeds and produces confusing data. Either rename the column to source_word or add a comment explaining the repurposing.

---

3. seed-prose-edda-course.ts falls back to anon key when service role key is missing

supabaseServiceKey falls back to NEXT_PUBLIC_SUPABASE_ANON_KEY when SUPABASE_SERVICE_ROLE_KEY is absent. Course/lesson inserts require elevated permissions to bypass RLS, so this produces silent failures. Better to fail fast with a clear error if the service role key is absent.

[claude]

Performance

Double DB fetch per page load in app/preview/readings/[slug]/page.tsx

getReading(slug) is called independently in both generateMetadata and PreviewReadingPage, doubling Supabase queries on every request. Wrapping with React cache() deduplicates calls within a single render pass:

import { cache } from 'react'
const getReading = cache(async (slug: string) => {
  const supabase = createAnonClient()
  const { data, error } = await supabase
    .from('library_readings')
    .select('*')
    .eq('slug', slug)
    .eq('is_public', true)
    .single()
  if (error || \!data) return null
  return data
})

[claude]

Security and Configuration

Hardcoded ElevenLabs voice ID

The voice ID 'onwK4e9ZLuTAKqWW03F9' is hardcoded in the generateAndUploadTTS call. Voice IDs are account-specific and will break silently if the ElevenLabs account changes. Should be an env var (ELEVENLABS_VOICE_ID_ICELANDIC) or at minimum a named constant at the top of the file.

---

ensureCltkCacheTable relies on a non-standard exec_sql RPC

exec_sql is not a standard Supabase RPC — it must be manually created. If absent, the failure message is easy to miss in CLI output. Since 20260307_cltk_lemma_cache.sql already defines the table correctly, the runtime auto-creation adds complexity with little benefit. Recommend removing ensureCltkCacheTable and relying solely on the migration.

[claude]

Code Quality

Repeated any casts in ingest-grymoire.ts — There are 6+ eslint-disable-next-line @typescript-eslint/no-explicit-any suppressions on Supabase client params. The Supabase JS client is fully typed — importing SupabaseClient from @supabase/supabase-js eliminates all of these without changing behavior.

---

preview-reading-client.tsx is 851 lines — The file handles audio playback, vocab sidebar, vocab grid, gated buttons, locked tab bar, nav bar, vocab matching, and the main component. Not a blocker for this PR, but splitting sub-components into separate files would improve navigability and testability. Worth a follow-up issue.

---

NEXT_PUBLIC_APP_URL is undocumented — PreviewNavBar uses process.env.NEXT_PUBLIC_APP_URL but this var is not in CLAUDE.md's environment variables section. Worth adding so devs know to set it locally.

---

tokenizeForAlignment hardcodes source names in citation regex — The regex hardcodes Lokasenna and Gylfaginning. Future passages citing other sources will not have citation lines stripped. Consider a more general pattern (em-dash followed by italicized text + optional numbers) or add a comment noting the limitation.

[claude]

What's Good

• The Needleman-Wunsch alignment for ON/IS vocabulary pairing is well-chosen and well-documented; using a bioinformatics algorithm for parallel text alignment is a creative and appropriate solution
• Unit test coverage for pure functions (parsing, alignment, NW algorithm) is thorough — 31 tests covering edge cases
• injectBanner idempotency is handled cleanly with a sentinel string check
• RLS migration correctly restricts anonymous access to is_public = true rows only
• Middleware change is minimal and correct
• Ollama fallback for the Icelandic lookup route is a nice DX improvement
• Auth setup now intercepts the Supabase token response instead of networkidle — should reduce flakiness considerably
• The PR honestly documents ensk.is quality limitations and tracks the fix in Peleke/ensk.is#27

---

Summary: The double-click bug in auth setup and the spanish column issue are the most pressing fixes. The double DB fetch and hardcoded voice ID are worth addressing before or shortly after merge. Everything else is polish.