fix: the reading browser counted the wrong thing, and the hover card fled
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN
This commit is contained in:
Daniel
parent
fc4df94a6a
commit
76848c5947
12 changed files with 215 additions and 35 deletions
|
|
@ -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")
|
||||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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("/")
|
||||
|
|
|
|||
|
|
@ -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. */
|
||||
<div className="app-shell">
|
||||
<Navbar />
|
||||
<div className="container">
|
||||
<div className="container app-main">
|
||||
<ErrorBoundary key={location.pathname}>
|
||||
<Outlet />
|
||||
</ErrorBoundary>
|
||||
</div>
|
||||
<SiteFooter />
|
||||
</>
|
||||
</div>
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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)); }
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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}
|
||||
</Link>
|
||||
{open && preview && (
|
||||
<span className={`al-card${above ? ' is-above' : ''}`} role="tooltip">
|
||||
<span className={`al-card${above ? ' is-above' : ''}`} role="tooltip"
|
||||
onMouseEnter={hideCancel} onMouseLeave={hideSoon}>
|
||||
<span className="al-card-title">{preview.title}</span>
|
||||
{preview.excerpt && <span className="al-card-excerpt">{preview.excerpt}</span>}
|
||||
<span className="al-card-meta">
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
})
|
||||
|
|
|
|||
|
|
@ -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({
|
|||
<FolderIcon />
|
||||
<span className="cc-name">{cat.name}</span>
|
||||
<span className="cc-count">{rollup[cat.id] ?? cat.question_count}</span>
|
||||
{(kids.length > 0 || articlesIn(cat.id).length > 0) && (
|
||||
{(stocked(kids).length > 0 || articlesIn(cat.id).length > 0) && (
|
||||
<span className="cc-chevron" aria-hidden="true">›</span>
|
||||
)}
|
||||
</button>
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -141,7 +141,7 @@ export default function ArticlesPage() {
|
|||
<Link key={article.id} to={`/articles/${article.id}`} className="article-card">
|
||||
<h3>{article.title} <DraftBadge status={article.status} /></h3>
|
||||
{article.summary && <p>{article.summary}</p>}
|
||||
<span className="article-card-meta">{article.sections?.length || 0} sections</span>
|
||||
<span className="article-card-meta">{article.section_count ?? 0} sections</span>
|
||||
</Link>
|
||||
))}
|
||||
</div>
|
||||
|
|
|
|||
|
|
@ -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(<MemoryRouter initialEntries={['/articles']}><Routes><Route path="/articles" element={<ArticlesPage />} /></Routes></MemoryRouter>)
|
||||
|
||||
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(<MemoryRouter initialEntries={['/articles']}><Routes><Route path="/articles" element={<ArticlesPage />} /></Routes></MemoryRouter>)
|
||||
await screen.findByText('Febrile seizures')
|
||||
|
|
|
|||
Loading…
Reference in a new issue