From 76848c5947e0c3ace51a1d69cb2948a8f152b465 Mon Sep 17 00:00:00 2001 From: Daniel Date: Fri, 11 Sep 2026 21:24:04 +0200 Subject: [PATCH] fix: the reading browser counted the wrong thing, and the hover card fled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The number beside a folder in Topic reading was a question count while the browser lists articles, so "Hyperinflammatory Sepsis 4" meant four questions and opened onto no reading at all. It counts what it opens now, rolled up over the subtree, and a branch with nothing to read in it is not offered — a folder with a number on it is a promise. The hover card could not be reached. Its body was pointer-events: none, on the idea that a hint should not sit between the reader and the link — but the card is offset below the link and never covered it, while the pointer travelling down to Split view crossed a body it could not enter, so no mouseenter fired and the hide timer closed it on the way. The card takes the pointer now, with a bridge across the gap. And clicking the words opens the card rather than the article. A cross-reference is read mid-sentence, and navigating away to find out whether it was worth following is the thing that breaks the thread; the card's two controls — beside what you are reading, or a tab for later — are how you go. That also gives touch a route, where hover has none. Modified and middle clicks are still the browser's. The listing sent content and sections for all 331 articles, 214KB of prose a list never renders. It sends what a list needs, which is 21KB. The footer sat wherever the content stopped, so a page still loading put it halfway up the screen with background below it. The shell is a column the height of the window. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN --- .../b8c9d0e1f2a3_fix_question_quiz_fk.py | 38 +++++++++++++++++++ backend/app/models/quiz.py | 5 ++- backend/app/routers/articles.py | 30 ++++++++++++++- frontend/src/App.jsx | 10 +++-- frontend/src/components/ArticleLink.css | 29 ++++++++++---- frontend/src/components/ArticleLink.jsx | 37 +++++++++++++----- frontend/src/components/ArticleLink.test.jsx | 33 ++++++++++++++-- frontend/src/components/CategoryColumns.jsx | 30 ++++++++++----- frontend/src/index.css | 7 ++++ frontend/src/pages/ArticleSplitView.test.jsx | 3 ++ frontend/src/pages/ArticlesPage.jsx | 2 +- frontend/src/pages/ArticlesPage.test.jsx | 26 ++++++++++++- 12 files changed, 215 insertions(+), 35 deletions(-) create mode 100644 backend/alembic/versions/b8c9d0e1f2a3_fix_question_quiz_fk.py diff --git a/backend/alembic/versions/b8c9d0e1f2a3_fix_question_quiz_fk.py b/backend/alembic/versions/b8c9d0e1f2a3_fix_question_quiz_fk.py new file mode 100644 index 0000000..d54e4cf --- /dev/null +++ b/backend/alembic/versions/b8c9d0e1f2a3_fix_question_quiz_fk.py @@ -0,0 +1,38 @@ +"""Deleting a quiz must not destroy the questions extracted for it. + +`questions.quiz_id` was created ON DELETE CASCADE, while the model has long +declared SET NULL and its comment says the column is informational — "which +quiz this question was originally extracted for" — with real membership held +in `quiz_question_links`. The database was the one being obeyed, so removing a +quiz silently erased every question it had produced, along with their attempts, +exam membership, media and article links, and straight past the trash that was +built to make exactly that impossible. + +The model's intent is the correct one and is what this applies. + +`quizzes.user_id` is the opposite case: the database cascades and the model +said nothing, and here the database is right — deleting an account is +documented as removing what it made. The model is corrected to match rather +than the data. + +Revision ID: b8c9d0e1f2a3 +Revises: a7b8c9d0e1f2 +""" +from alembic import op + +revision = "b8c9d0e1f2a3" +down_revision = "a7b8c9d0e1f2" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.drop_constraint("questions_quiz_id_fkey", "questions", type_="foreignkey") + op.create_foreign_key("questions_quiz_id_fkey", "questions", "quizzes", + ["quiz_id"], ["id"], ondelete="SET NULL") + + +def downgrade() -> None: + op.drop_constraint("questions_quiz_id_fkey", "questions", type_="foreignkey") + op.create_foreign_key("questions_quiz_id_fkey", "questions", "quizzes", + ["quiz_id"], ["id"], ondelete="CASCADE") diff --git a/backend/app/models/quiz.py b/backend/app/models/quiz.py index d027bf8..49a54dc 100644 --- a/backend/app/models/quiz.py +++ b/backend/app/models/quiz.py @@ -18,7 +18,10 @@ class Quiz(Base): id = Column(Integer, primary_key=True, index=True) section_id = Column(Integer, ForeignKey("sections.id", ondelete="SET NULL"), nullable=True) - user_id = Column(Integer, ForeignKey("users.id"), nullable=False) + # Cascades: deleting an account is documented as removing what it made. + # Declared here because the database has always done it, and a model that + # says nothing is a model that will be believed. + user_id = Column(Integer, ForeignKey("users.id", ondelete="CASCADE"), nullable=False) category_id = Column(Integer, ForeignKey("quiz_categories.id", ondelete="SET NULL"), nullable=True) title = Column(String, nullable=False) questions_count = Column(Integer, default=0) diff --git a/backend/app/routers/articles.py b/backend/app/routers/articles.py index cedd2a8..5f0a647 100644 --- a/backend/app/routers/articles.py +++ b/backend/app/routers/articles.py @@ -183,6 +183,34 @@ def _views_for(db, user) -> list[str]: return views_for(db.get(Exam, exam_id) if exam_id else None) +def _article_card_json(article: Article) -> dict: + """An article as it appears in a list: enough to find it, not to read it. + + The listing used to send `content` and `sections` for every article — 214KB + of prose across the library, none of which a list renders. A browser that + waits for the whole corpus before it can draw a column of titles is slow + for no reason. + """ + sections = article.sections or [] + return { + "id": article.id, + "slug": article.slug, + "title": article.title, + "summary": article.summary, + "category_id": article.category_id, + "section_id": article.section_id, + "user_id": article.user_id, + "status": article.status, + "section_count": len(sections), + "variants": article_service.available_variants(article), + "generated_by": article.generated_by, + "reviewed_at": article.reviewed_at, + "submitted_at": article.submitted_at, + "created_at": article.created_at, + "updated_at": article.updated_at, + } + + def _article_json(article: Article) -> dict: return { "id": article.id, @@ -293,7 +321,7 @@ def list_articles( articles.sort(key=lambda article: rank_of.get(article.id, len(rank_of))) if not current_user.is_moderator: articles = [a for a in articles if a.status == "published"] - return [_article_json(a) for a in articles] + return [_article_card_json(a) for a in articles] @router.post("/") diff --git a/frontend/src/App.jsx b/frontend/src/App.jsx index 40a70cb..4a88f9d 100644 --- a/frontend/src/App.jsx +++ b/frontend/src/App.jsx @@ -58,15 +58,19 @@ function AppLayout() { // Keyed by path so navigating away from a broken page clears the error. const location = useLocation() return ( - <> + /* A column the height of the window, so the footer is at the bottom of the + screen rather than at the bottom of the content — a page still loading + is a spinner, and the footer used to sit halfway up with the page + background showing beneath it. */ +
-
+
- +
) } diff --git a/frontend/src/components/ArticleLink.css b/frontend/src/components/ArticleLink.css index 32c9c9d..6dfa1d3 100644 --- a/frontend/src/components/ArticleLink.css +++ b/frontend/src/components/ArticleLink.css @@ -25,17 +25,31 @@ font-size: 0.84rem; line-height: 1.5; text-align: left; - /* The card is a hint before it is a target: its body stays transparent to the - pointer so it never sits between the reader and the link it describes, and - only the controls below take clicks. */ - pointer-events: none; + /* The card must hold the pointer. It used to be transparent to it, on the + idea that a hint should never sit between the reader and the link — but + the card is offset below the link and never covered it, while the pointer + travelling down to Split view crossed a body it could not enter, so no + mouseenter fired and the hide timer closed the card on the way. */ + pointer-events: auto; } + +/* Bridges the 8px gap between the link and the card, so crossing it never + leaves the wrapper at all and the grace period is never needed. */ +.al-card::before { + content: ''; + position: absolute; + left: 0; + right: 0; + top: -9px; + height: 9px; +} +.al-card.is-above::before { top: auto; bottom: -9px; } .al-card.is-above { top: auto; bottom: calc(100% + 8px); } .al-card-title { font-weight: 700; font-size: 0.92rem; color: var(--text); } .al-card-excerpt { color: var(--text-muted); } .al-card-meta { font-size: 0.75rem; color: var(--text-subtle); } -.al-card-actions { display: flex; gap: 6px; margin-top: 2px; pointer-events: auto; } +.al-card-actions { display: flex; gap: 6px; margin-top: 2px; } .al-card-action { display: inline-flex; align-items: center; @@ -55,7 +69,8 @@ .al-card-action:hover { border-color: var(--primary); color: var(--primary); } .al-card-action:focus-visible { outline: 2px solid var(--primary); outline-offset: 1px; } -/* No hover on touch, so the card never appears there and the link is just a link. */ +/* Touch has no hover, so the card arrives on the tap that would have followed + the link. It is the only route to the article there, so it must be shown. */ @media (hover: none) { - .al-card { display: none; } + .al-card { width: min(340px, calc(100vw - 32px)); } } diff --git a/frontend/src/components/ArticleLink.jsx b/frontend/src/components/ArticleLink.jsx index b920d55..c8b9cfa 100644 --- a/frontend/src/components/ArticleLink.jsx +++ b/frontend/src/components/ArticleLink.jsx @@ -31,8 +31,9 @@ export function fetchPreview(slug) { * couple of sentences, how much is there, and — where the page can hold one — * the offer to open it beside what you are reading rather than in place of it. * - * Touch has no hover, so on a phone this is just a link — a card that appears on - * tap would sit between the finger and the thing it was about to open. + * Touch has no hover, so on a phone the tap that would have followed the link + * opens the card instead — which is the same journey a pointer makes, minus + * the hover it does not have. */ /** The numeric id behind a slug, reusing whatever the hover card already fetched. */ export const resolveArticleId = (slug) => fetchPreview(slug).then(data => data?.id ?? null) @@ -80,20 +81,37 @@ export default function ArticleLink({ slug, children, className = '' }) { timer.current = setTimeout(() => setOpen(false), HIDE_DELAY) }, []) + // Reaching the card is arriving, not leaving: whatever the pointer did on + // the way, it is here now. + const hideCancel = useCallback(() => clearTimeout(timer.current), []) + const hideNow = useCallback(() => { clearTimeout(timer.current) setOpen(false) }, []) + /** + * Clicking the words opens the card, not the article. + * + * A cross-reference is read mid-sentence, and navigating away to find out + * whether it was worth it is the thing that breaks the thread. So the link + * itself shows the card, and the card's two controls — beside what you are + * reading, or in a tab for later — are how you actually go. Nothing here + * takes you off the page you are on by accident. + * + * Modified and middle clicks are left to the browser, so ctrl-click, ⌘-click + * and "open in new tab" behave as they do on any other link. + */ const followLink = (event) => { - hideNow() - // Inside the second pane a cross-reference stays in that pane: the article - // you started from is on the left and should still be there afterwards. - // Modified and middle clicks are left alone so they still open a tab. - if (!split?.inPane || event.button !== 0) return + if (event.button !== 0) return if (event.metaKey || event.ctrlKey || event.shiftKey || event.altKey) return event.preventDefault() - split.open(slug) + clearTimeout(timer.current) + if (open) { + setOpen(false) + return + } + reveal() } return ( @@ -104,7 +122,8 @@ export default function ArticleLink({ slug, children, className = '' }) { {children} {open && preview && ( - + {preview.title} {preview.excerpt && {preview.excerpt}} diff --git a/frontend/src/components/ArticleLink.test.jsx b/frontend/src/components/ArticleLink.test.jsx index 11212e7..f7cfff8 100644 --- a/frontend/src/components/ArticleLink.test.jsx +++ b/frontend/src/components/ArticleLink.test.jsx @@ -151,18 +151,45 @@ describe('reading a cross-reference without leaving the article', () => { expect(within(card).getByRole('link', { name: /new tab/i })).toHaveFocus() }) - it('leaves an ordinary click alone: the link is still a link', async () => { + it('opens the card rather than the article, so reading is not interrupted', async () => { const open = vi.fn() mountBeside({ open, inPane: false }, 'plain-click') await userEvent.click(linkNamed()) - expect(await screen.findByText('Whole article')).toBeInTheDocument() + // The card is the answer to "is this worth following?", and the two + // controls on it are how you go. Nothing navigates by accident. + expect(await screen.findByRole('tooltip')).toBeInTheDocument() + expect(screen.queryByText('Whole article')).not.toBeInTheDocument() expect(open).not.toHaveBeenCalled() }) - it('keeps a link followed inside the pane inside the pane', async () => { + it('clicking the words again puts the card away', async () => { + mountBeside({ open: vi.fn(), inPane: false }, 'toggle-me') + await userEvent.click(linkNamed()) + await screen.findByRole('tooltip') + await userEvent.click(linkNamed()) + expect(screen.queryByRole('tooltip')).not.toBeInTheDocument() + }) + + it('leaves a modified click to the browser, as on any other link', async () => { + const open = vi.fn() + mountBeside({ open, inPane: true }, 'deeper-topic') + // ⌘-click, ctrl-click and "open in new tab" are the browser's to handle; + // this component does not intercept them. + await userEvent.keyboard('{Meta>}') + await userEvent.click(linkNamed()) + await userEvent.keyboard('{/Meta}') + // Not intercepted: the split view is not opened and the page is not + // changed, leaving the browser to do what it does with a modified click. + expect(open).not.toHaveBeenCalled() + expect(screen.queryByText('Whole article')).not.toBeInTheDocument() + }) + + it('goes to the pane from the card, even inside a pane', async () => { const open = vi.fn() mountBeside({ open, inPane: true }, 'deeper-topic') await userEvent.click(linkNamed()) + const card = await screen.findByRole('tooltip') + await userEvent.click(within(card).getByRole('button', { name: /split view/i })) expect(open).toHaveBeenCalledWith('deeper-topic') expect(screen.queryByText('Whole article')).not.toBeInTheDocument() }) diff --git a/frontend/src/components/CategoryColumns.jsx b/frontend/src/components/CategoryColumns.jsx index 10b7e59..70de05f 100644 --- a/frontend/src/components/CategoryColumns.jsx +++ b/frontend/src/components/CategoryColumns.jsx @@ -1,4 +1,4 @@ -import { useEffect, useMemo, useRef, useState } from 'react' +import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import './CategoryColumns.css' /** A folder of further topics. */ @@ -45,35 +45,47 @@ export default function CategoryColumns({ return map }, [categories]) + const articlesIn = useCallback( + (categoryId) => (articlesOf ? articlesOf(categoryId) : []), [articlesOf]) + + /** + * How much is behind each folder, counting the thing this browser opens. + * + * It counted questions while listing articles, so "Hyperinflammatory Sepsis + * 4" meant four questions and opened onto no reading at all. A number beside + * a folder is a promise about what is inside it. + */ const rollup = useMemo(() => { const totals = {} const walk = (cat) => { - let sum = cat.question_count || 0 + let sum = articlesOf ? articlesIn(cat.id).length : (cat.question_count || 0) for (const child of childrenOf[cat.id] || []) sum += walk(child) totals[cat.id] = sum return sum } for (const root of childrenOf[0] || []) walk(root) return totals - }, [childrenOf]) + }, [childrenOf, articlesOf, articlesIn]) const byId = useMemo(() => Object.fromEntries(categories.map(c => [c.id, c])), [categories]) - const articlesIn = (categoryId) => (articlesOf ? articlesOf(categoryId) : []) - // One column per opened level: the roots, then each opened parent's contents. // Anything filed nowhere is listed in the root column rather than being // unreachable because it has no heading to sit under. - const columns = [{ parentId: 0, items: childrenOf[0] || [], articles: articlesIn(null) }] + // Only folders with something behind them, once the subtree is counted — + // an empty branch is a row that promises reading and opens onto nothing. + const stocked = (list) => list.filter(cat => (rollup[cat.id] ?? 0) > 0) + + const columns = [{ parentId: 0, items: stocked(childrenOf[0] || []), articles: articlesIn(null) }] for (const id of path) { - const kids = childrenOf[id] || [] + const kids = stocked(childrenOf[id] || []) const articles = articlesIn(id) if (!kids.length && !articles.length) break columns.push({ parentId: id, items: kids, articles }) } const openAt = (level, cat) => { - const hasContents = (childrenOf[cat.id] || []).length > 0 || articlesIn(cat.id).length > 0 + const hasContents = stocked(childrenOf[cat.id] || []).length > 0 || articlesIn(cat.id).length > 0 setPath(hasContents ? [...path.slice(0, level), cat.id] : path.slice(0, level)) onSelect(cat.id) } @@ -96,7 +108,7 @@ export default function CategoryColumns({ {cat.name} {rollup[cat.id] ?? cat.question_count} - {(kids.length > 0 || articlesIn(cat.id).length > 0) && ( + {(stocked(kids).length > 0 || articlesIn(cat.id).length > 0) && ( )} diff --git a/frontend/src/index.css b/frontend/src/index.css index d848659..37bf012 100644 --- a/frontend/src/index.css +++ b/frontend/src/index.css @@ -119,6 +119,13 @@ html, body { overflow-x: hidden; max-width: 100%; } /* ── Layout ─────────────────────────────────────────────────── */ .container { max-width: 1200px; margin: 0 auto; padding: 0 28px; } +/* The footer belongs at the bottom of the window, not wherever the content + happens to stop. A loading page is a spinner, and without this the footer + sat mid-screen with the page background below it. */ +.app-shell { display: flex; flex-direction: column; min-height: 100dvh; } +.app-main { flex: 1 0 auto; width: 100%; } +.app-shell > .site-footer { flex: none; } + /* ── Navbar ─────────────────────────────────────────────────── */ /* Two bars. The primary one — identity, search, account — never moves. The section bar leaves while you scroll down and returns on the way up: it is diff --git a/frontend/src/pages/ArticleSplitView.test.jsx b/frontend/src/pages/ArticleSplitView.test.jsx index 9ea4a35..897c452 100644 --- a/frontend/src/pages/ArticleSplitView.test.jsx +++ b/frontend/src/pages/ArticleSplitView.test.jsx @@ -104,7 +104,10 @@ describe('reading a cross-reference beside the article', () => { await openSplit('meningitis') const pane = await screen.findByRole('region', { name: 'Split view: Meningitis' }) + // Clicking the words opens the card; the card's control is what goes. await userEvent.click(within(pane).getByRole('link', { name: 'sepsis' })) + const card = await screen.findByRole('tooltip') + await userEvent.click(within(card).getByRole('button', { name: /split view/i })) await waitFor(() => expect(api.get).toHaveBeenCalledWith('/articles/9')) expect(await screen.findByRole('region', { name: 'Split view: Sepsis' })).toBeInTheDocument() expect(document.querySelectorAll('.article-split-pane')).toHaveLength(1) diff --git a/frontend/src/pages/ArticlesPage.jsx b/frontend/src/pages/ArticlesPage.jsx index aa380a4..6f06098 100644 --- a/frontend/src/pages/ArticlesPage.jsx +++ b/frontend/src/pages/ArticlesPage.jsx @@ -141,7 +141,7 @@ export default function ArticlesPage() {

{article.title}

{article.summary &&

{article.summary}

} - {article.sections?.length || 0} sections + {article.section_count ?? 0} sections ))}
diff --git a/frontend/src/pages/ArticlesPage.test.jsx b/frontend/src/pages/ArticlesPage.test.jsx index f96a962..8a1a14f 100644 --- a/frontend/src/pages/ArticlesPage.test.jsx +++ b/frontend/src/pages/ArticlesPage.test.jsx @@ -61,15 +61,39 @@ describe('topic reading', () => { expect(screen.getByText('Draft')).toBeInTheDocument() expect(screen.queryByRole('button', { name: /Seizures/ })).not.toBeInTheDocument() + // Cardiology has seven questions and no reading. This browser opens + // articles, so it is not offered: a folder here promises something to read. + expect(screen.queryByRole('button', { name: /Cardiology/ })).not.toBeInTheDocument() + // Opening a topic opens its contents in the next column; the trail stays. await userEvent.click(screen.getByRole('button', { name: /Neurology/ })) expect(screen.getByRole('button', { name: /Seizures/ })).toBeInTheDocument() - expect(screen.getByRole('button', { name: /Cardiology/ })).toBeInTheDocument() await userEvent.click(screen.getByRole('button', { name: /Seizures/ })) expect(screen.getByRole('button', { name: /Febrile seizures/ })).toBeInTheDocument() }) + it('counts the reading behind a topic, not its questions', async () => { + const cats = [ + { id: 2, name: 'Neurology', parent_id: null, question_count: 10 }, + { id: 3, name: 'Seizures', parent_id: 2, question_count: 4 }, + ] + // Two articles, both filed three levels in. The old count showed the + // question total, so "Neurology 10" opened onto two pieces of reading. + const filed = { ...article, category_id: 3 } + const second = { id: 12, title: 'Status epilepticus', slug: 'status', status: 'published', category_id: 3, sections: [] } + api.get.mockImplementation(url => { + if (url === '/question-categories/') return Promise.resolve({ data: cats }) + if (url === '/articles/') return Promise.resolve({ data: [filed, second] }) + return Promise.resolve({ data: [] }) + }) + render(} />) + + const neurology = await screen.findByRole('button', { name: /Neurology/ }) + expect(neurology).toHaveTextContent('2') + expect(neurology).not.toHaveTextContent('10') + }) + it('searching answers with the matches, not the shelf they sit on', async () => { render(} />) await screen.findByText('Febrile seizures')