feat: articles are collectible, and Summary is a toggle

A library holds both articles and questions now. It held only questions, so the
bookmark on an article had nowhere to write and stood in for the questions
filed under the topic instead — which is not what a reader who saved the
reading asked for, and left a topic with no questions unsaveable. Its own
table rather than a nullable column beside `question_id`: that shape allows a
row with both or neither, and every read then has to say which kind it is
looking at.

Which libraries already hold an article is now asked of the server, as one
question. It was kept on the device because the API could not answer, which was
wrong on the second machine and silently so. Putting one back is the same
control rather than an undo somewhere else.

"Short" is called Summary, because that is what the section is called, and it
is a toggle rather than one tab of three — the whole topic, or the part of it
worth revising, which is a different kind of choice from Long versus Clinical.
It names its own state, so a reader can tell why two thirds of the contents are
not there. The stored variant stays `short`: renaming it would be a data
migration to change a word on a button.

Also: `litellm==1.28.13` has been withdrawn from PyPI, so requirements.txt
could not be edited at all without the pip layer failing to rebuild — which is
what blocked pinning Pillow. Repinned to 1.53.1, the nearest still published;
the three things we use are unchanged in it, and both suites pass on the new
set. Pillow is pinned properly now rather than arriving through PyMuPDF.

One consequence, handled: `litellm.utils.get_valid_models()` now returns
nothing unless a provider's own API key is in the environment, and ours is a
proxy. That branch is only reached when no proxy is configured, and it now says
so instead of answering with an empty list that reads as "this site has no
models".

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN
This commit is contained in:
Daniel 2026-09-12 14:52:20 +02:00
parent cedab6e91e
commit 52ef7acdea
14 changed files with 366 additions and 148 deletions

View file

@ -0,0 +1,44 @@
"""A library can hold articles as well as questions.
Collections were questions only, so the reading side of the site had no way to
put anything aside the bookmark on an article had nowhere to write. Its own
table rather than a nullable column beside `question_id`, so a row cannot claim
to be both kinds or neither.
Revision ID: c5d6e7f8091a
Revises: b4c5d6e7f809
"""
import sqlalchemy as sa
from alembic import op
revision = "c5d6e7f8091a"
down_revision = "b4c5d6e7f809"
branch_labels = None
depends_on = None
def upgrade():
# `Base.metadata.create_all()` still runs at startup and creates missing
# tables, so on a box that has already booted this code the table is here
# before the migration is. Checked rather than assumed, or the upgrade
# fails on exactly the machines that are up to date.
if "user_collection_articles" in sa.inspect(op.get_bind()).get_table_names():
return
op.create_table(
"user_collection_articles",
sa.Column("id", sa.Integer(), primary_key=True, index=True),
sa.Column("collection_id", sa.Integer(),
sa.ForeignKey("user_collections.id", ondelete="CASCADE"), nullable=False),
sa.Column("article_id", sa.Integer(),
sa.ForeignKey("articles.id", ondelete="CASCADE"), nullable=False),
sa.UniqueConstraint("collection_id", "article_id", name="uq_collection_article"),
)
op.create_index("ix_user_collection_articles_collection",
"user_collection_articles", ["collection_id"])
def downgrade():
if "user_collection_articles" not in sa.inspect(op.get_bind()).get_table_names():
return
op.drop_index("ix_user_collection_articles_collection", table_name="user_collection_articles")
op.drop_table("user_collection_articles")

View file

@ -23,3 +23,20 @@ class UserCollectionQuestion(Base):
id = Column(Integer, primary_key=True, index=True) id = Column(Integer, primary_key=True, index=True)
collection_id = Column(Integer, ForeignKey("user_collections.id", ondelete="CASCADE"), nullable=False) collection_id = Column(Integer, ForeignKey("user_collections.id", ondelete="CASCADE"), nullable=False)
question_id = Column(Integer, ForeignKey("questions.id", ondelete="CASCADE"), nullable=False) question_id = Column(Integer, ForeignKey("questions.id", ondelete="CASCADE"), nullable=False)
class UserCollectionArticle(Base):
"""An article put aside into a library.
Its own table rather than a nullable `article_id` beside `question_id` on
the row above: that shape allows a row with both, or with neither, and
every read then has to say which kind it is looking at. Two tables, one
unique constraint each, and a library is the union of them.
"""
__tablename__ = "user_collection_articles"
__table_args__ = (UniqueConstraint("collection_id", "article_id", name="uq_collection_article"),)
id = Column(Integer, primary_key=True, index=True)
collection_id = Column(Integer, ForeignKey("user_collections.id", ondelete="CASCADE"), nullable=False)
article_id = Column(Integer, ForeignKey("articles.id", ondelete="CASCADE"), nullable=False)

View file

@ -212,13 +212,22 @@ def search_litellm_models(
log.warning(f"LiteLLM model search failed: {e}") log.warning(f"LiteLLM model search failed: {e}")
raise HTTPException(status_code=400, detail=f"Failed to query models API: {e}") raise HTTPException(status_code=400, detail=f"Failed to query models API: {e}")
# No proxy configured, so all that is left is whatever the SDK can name by
# itself. It only lists providers whose own API keys are in the
# environment, and this deployment has none — everything goes through the
# proxy above. An empty list is therefore the normal answer here, and
# returning it plainly would read as "this site has no models".
try: try:
import litellm import litellm
models = sorted(litellm.utils.get_valid_models()) models = sorted(litellm.utils.get_valid_models())
return {"models": models, "source": "litellm-builtin"}
except Exception as e: except Exception as e:
log.warning(f"LiteLLM builtin model list failed: {e}") log.warning(f"LiteLLM builtin model list failed: {e}")
raise HTTPException(status_code=500, detail="Failed to retrieve LiteLLM built-in model list.") raise HTTPException(status_code=500, detail="Failed to retrieve LiteLLM built-in model list.")
if not models:
raise HTTPException(
status_code=400,
detail="No model endpoint is configured. Set the API base to your LLM proxy and try again.")
return {"models": models, "source": "litellm-builtin"}
@router.get("/models", response_model=list[AIModelConfigResponse]) @router.get("/models", response_model=list[AIModelConfigResponse])

View file

@ -1,4 +1,10 @@
"""Personal question libraries (saved questions).""" """Personal libraries: questions and articles somebody has put aside.
A library holds both. It used to hold only questions, so the reading side of
the site had a bookmark with nowhere to write and the two kinds are the same
act to whoever is doing it, which is why they share a library rather than each
getting their own list.
"""
from datetime import datetime from datetime import datetime
from fastapi import APIRouter, Depends, HTTPException from fastapi import APIRouter, Depends, HTTPException
@ -6,9 +12,12 @@ from pydantic import BaseModel, field_validator
from sqlalchemy.orm import Session from sqlalchemy.orm import Session
from app.database import get_db from app.database import get_db
from app.models.collection import UserCollection, UserCollectionQuestion from app.models.article import Article
from app.models.collection import (UserCollection, UserCollectionArticle,
UserCollectionQuestion)
from app.models.question import Question from app.models.question import Question
from app.models.user import User from app.models.user import User
from app.services.article_service import readable_articles
from app.services.quiz_builder import bank_question_predicate from app.services.quiz_builder import bank_question_predicate
from app.utils.auth import get_current_user from app.utils.auth import get_current_user
@ -42,6 +51,8 @@ def _as_json(db, collection) -> dict:
"title": collection.title, "title": collection.title,
"question_count": db.query(UserCollectionQuestion).filter( "question_count": db.query(UserCollectionQuestion).filter(
UserCollectionQuestion.collection_id == collection.id).count(), UserCollectionQuestion.collection_id == collection.id).count(),
"article_count": db.query(UserCollectionArticle).filter(
UserCollectionArticle.collection_id == collection.id).count(),
"created_at": collection.created_at, "created_at": collection.created_at,
"last_used_at": collection.last_used_at, "last_used_at": collection.last_used_at,
# Every library is one person's. Said plainly rather than assumed, # Every library is one person's. Said plainly rather than assumed,
@ -130,3 +141,60 @@ def remove_collection_question(collection_id: int, question_id: int, db: Session
synchronize_session=False) synchronize_session=False)
_touch(db, collection) _touch(db, collection)
db.commit() db.commit()
@router.get("/{collection_id}/articles")
def collection_articles(collection_id: int, db: Session = Depends(get_db),
user: User = Depends(get_current_user)):
collection = _own(db, user, collection_id)
rows = (db.query(Article)
.join(UserCollectionArticle, UserCollectionArticle.article_id == Article.id)
.filter(UserCollectionArticle.collection_id == collection.id)
.order_by(Article.title)
.all())
_touch(db, collection)
db.commit()
# An article saved and later unpublished stays in the library and says so,
# rather than vanishing from a list the learner built themselves.
return [{"id": a.id, "title": a.title, "slug": a.slug, "status": a.status} for a in rows]
@router.put("/{collection_id}/articles/{article_id}")
def add_collection_article(collection_id: int, article_id: int, db: Session = Depends(get_db),
user: User = Depends(get_current_user)):
collection = _own(db, user, collection_id)
if not readable_articles(db, user).filter(Article.id == article_id).first():
raise HTTPException(404, "Article not found")
if db.query(UserCollectionArticle.id).filter_by(
collection_id=collection.id, article_id=article_id).first():
return {"added": False}
db.add(UserCollectionArticle(collection_id=collection.id, article_id=article_id))
_touch(db, collection)
db.commit()
return {"added": True}
@router.delete("/{collection_id}/articles/{article_id}", status_code=204)
def remove_collection_article(collection_id: int, article_id: int, db: Session = Depends(get_db),
user: User = Depends(get_current_user)):
collection = _own(db, user, collection_id)
db.query(UserCollectionArticle).filter_by(
collection_id=collection_id, article_id=article_id).delete(synchronize_session=False)
_touch(db, collection)
db.commit()
@router.get("/for-article/{article_id}")
def libraries_holding_article(article_id: int, db: Session = Depends(get_db),
user: User = Depends(get_current_user)):
"""Which of this learner's libraries already hold this article.
The reader needs it to draw the bookmark filled or hollow. Asked as one
question rather than by fetching every library's contents, which would also
stamp each of them as used and reorder the learner's own list.
"""
held = {row.collection_id for row in db.query(UserCollectionArticle.collection_id)
.join(UserCollection, UserCollection.id == UserCollectionArticle.collection_id)
.filter(UserCollection.user_id == user.id,
UserCollectionArticle.article_id == article_id).all()}
return {"collection_ids": sorted(held)}

View file

@ -171,3 +171,18 @@ def set_status(db: Session, article: Article, status: str, user_id: int | None)
article.reviewed_at = now article.reviewed_at = now
article.reviewed_by = user_id article.reviewed_by = user_id
article.status = status article.status = status
def readable_articles(db, user):
"""A query over the articles this person may read.
Published, plus their own drafts, plus everything if they moderate. The
rule was written out inline at every place that needed it, which is how a
draft ends up reachable from one route and not another.
"""
from app.models.article import Article
query = db.query(Article)
if getattr(user, "is_moderator", False):
return query
return query.filter((Article.status == "published") | (Article.user_id == user.id))

View file

@ -9,7 +9,19 @@ python-multipart==0.0.9
pydantic[email]==2.6.1 pydantic[email]==2.6.1
pydantic-settings==2.1.0 pydantic-settings==2.1.0
PyMuPDF==1.23.22 PyMuPDF==1.23.22
litellm==1.28.13 # Thumbnails. It arrives transitively today, which is not a reason to let a
# feature depend on somebody else's dependency tree.
Pillow==12.3.0
# 1.28.13 was withdrawn from PyPI, so this file could not be edited at all
# without the whole pip layer failing to rebuild. 1.53.1 is the nearest version
# still published; the three things we use — completion, acompletion and
# utils.get_valid_models — are unchanged in it.
#
# We use the SDK only as an OpenAI-compatible HTTP client pointed at the proxy
# in LITELLM_API_BASE: every call sets api_base and prefixes the model with
# `openai/`. Embeddings already go over plain httpx. There is a smaller
# dependency in here waiting to be taken.
litellm==1.53.1
chromadb==0.4.24 chromadb==0.4.24
celery[redis]==5.3.6 celery[redis]==5.3.6
redis==5.0.1 redis==5.0.1

View file

@ -3,7 +3,7 @@ import RichEditor from './RichEditor'
import './ArticleEditor.css' import './ArticleEditor.css'
const VIEWS = [ const VIEWS = [
{ key: 'short', label: 'Short', hint: 'Bullets a learner could revise from the night before an exam.' }, { key: 'short', label: 'Summary', hint: 'Bullets a learner could revise from the night before an exam.' },
{ key: 'long', label: 'Long', hint: 'The full article: definition through management.' }, { key: 'long', label: 'Long', hint: 'The full article: definition through management.' },
{ key: 'clinical', label: 'Clinical', hint: 'What to do at the bedside, with doses and routes.' }, { key: 'clinical', label: 'Clinical', hint: 'What to do at the bedside, with doses and routes.' },
] ]

View file

@ -41,14 +41,14 @@ describe('editing an article', () => {
expect(screen.queryByDisplayValue('In short')).not.toBeInTheDocument() expect(screen.queryByDisplayValue('In short')).not.toBeInTheDocument()
expect(screen.queryByDisplayValue('Management')).not.toBeInTheDocument() expect(screen.queryByDisplayValue('Management')).not.toBeInTheDocument()
await openView('Short') await openView('Summary')
expect(screen.getByDisplayValue('In short')).toBeInTheDocument() expect(screen.getByDisplayValue('In short')).toBeInTheDocument()
expect(screen.queryByDisplayValue('Definition')).not.toBeInTheDocument() expect(screen.queryByDisplayValue('Definition')).not.toBeInTheDocument()
}) })
it('counts what each view holds, so an empty one is visible before you open it', async () => { it('counts what each view holds, so an empty one is visible before you open it', async () => {
mount() mount()
expect(within(screen.getByRole('tab', { name: /^Short/ })).getByText('1')).toBeInTheDocument() expect(within(screen.getByRole('tab', { name: /^Summary/ })).getByText('1')).toBeInTheDocument()
expect(within(screen.getByRole('tab', { name: /^Long/ })).getByText('2')).toBeInTheDocument() expect(within(screen.getByRole('tab', { name: /^Long/ })).getByText('2')).toBeInTheDocument()
}) })

View file

@ -31,15 +31,17 @@ export function Markdown({ children, attemptId }) {
* links and the pane keeps it to itself; everything else what is open, the * links and the pane keeps it to itself; everything else what is open, the
* rail, the mobile drawer belongs to this reader alone. * rail, the mobile drawer belongs to this reader alone.
*/ */
// One topic, three readings. Short is what you revise from, long is what you // One topic, three readings. The summary is what you revise from, long is what
// study from, clinical is what you act from at the bedside. They are views of // you study from, clinical is what you act from at the bedside. They are views
// one article rather than three articles, so the numbers cannot drift apart and // of one article rather than three articles, so the numbers cannot drift apart
// a question linked to the topic still means one thing. // and a question linked to the topic still means one thing.
// //
// Short leads because it is the quickest way to tell whether this is the article // `short` is the stored variant and stays that way renaming it would be a
// you wanted; the full text is one click away. // data migration to change a word on a button. What a reader sees is "Summary",
// because that is what the section itself is called.
const SUMMARY = 'short'
const VIEWS = [ const VIEWS = [
{ key: 'short', label: 'Short' }, { key: SUMMARY, label: 'Summary' },
{ key: 'long', label: 'Long' }, { key: 'long', label: 'Long' },
{ key: 'clinical', label: 'Clinical' }, { key: 'clinical', label: 'Clinical' },
] ]
@ -244,7 +246,7 @@ export default function ArticleReader({
<div className="asec-list"> <div className="asec-list">
{/* A view with one section does not need a heading over it, or a {/* A view with one section does not need a heading over it, or a
control to collapse the only thing there is. The tab already control to collapse the only thing there is. The tab already
names it "Short" then a heading reading "In short" says the names it "Summary" then a heading reading "In short" says the
same word twice and hides the content behind a chevron. */} same word twice and hides the content behind a chevron. */}
{soleSection ? ( {soleSection ? (
<div className="asec-sole" id={`section-${idPrefix}${soleSection.id}`}> <div className="asec-sole" id={`section-${idPrefix}${soleSection.id}`}>
@ -292,16 +294,39 @@ export default function ArticleReader({
* it and the rail is filtered by the same choice, so a reader who cannot * it and the rail is filtered by the same choice, so a reader who cannot
* see the switch cannot explain why half the contents just went away. * see the switch cannot explain why half the contents just went away.
*/ */
const choose = (key) => { setView(key); setOpenIds({}); setReading('') }
const hasSummary = present.some(option => option.key === SUMMARY)
const full = present.filter(option => option.key !== SUMMARY)
// Where the reader goes when the summary is switched off: the fullest reading
// there is, which is Long unless the article only has the clinical view.
const fallback = full[0]?.key
const viewSwitch = present.length > 1 && ( const viewSwitch = present.length > 1 && (
<div className="aview-switch" role="tablist" aria-label="How much of this topic to read"> <div className="aview-group">
{present.map(option => ( {/* Summary is a toggle rather than one tab among three, because that is
<button key={option.key} type="button" role="tab" what it is: the whole topic, or the part of it worth revising. The
aria-selected={view === option.key} other two are different readings of the same length and belong beside
className={`aview${view === option.key ? ' is-active' : ''}`} each other. */}
onClick={() => { setView(option.key); setOpenIds({}); setReading('') }}> {hasSummary && fallback && (
{option.label} <button type="button" className={`aview-only${view === SUMMARY ? ' is-on' : ''}`}
aria-pressed={view === SUMMARY}
onClick={() => choose(view === SUMMARY ? fallback : SUMMARY)}>
<span aria-hidden="true"></span>
Summary {view === SUMMARY ? 'on' : 'off'}
</button> </button>
))} )}
{full.length > 1 && (
<div className="aview-switch" role="tablist" aria-label="How much of this topic to read">
{full.map(option => (
<button key={option.key} type="button" role="tab"
aria-selected={view === option.key}
className={`aview${view === option.key ? ' is-active' : ''}`}
onClick={() => choose(option.key)}>
{option.label}
</button>
))}
</div>
)}
</div> </div>
) )

View file

@ -1,43 +1,39 @@
import { useCallback, useEffect, useRef, useState } from 'react' import { useEffect, useRef, useState } from 'react'
import api from '../api/client' import api from '../api/client'
import './ArticleSaveButton.css' import './ArticleSaveButton.css'
/** /**
* Keep this topic. * Keep this article.
* *
* Libraries hold questions `user_collection_questions` has a question_id and * A library holds both articles and questions, because putting one of each
* nothing else so saving a topic saves the questions written against it, * aside is the same act to whoever is doing it. The article goes in as itself
* which is what you would want it for: the reading and the practice on one * rather than standing in for the questions written against it a topic with
* subject, filed together, ready to sit later. Nothing here invents a second * no questions is still worth keeping, and a reader who saved the reading did
* place to save things; it is the same endpoint the session player writes to * not ask for a session.
* when you file a question mid-attempt.
* *
* The consequence is that the server cannot be asked "is this article saved?", * Which libraries already hold it is asked of the server, as one question. It
* only "is this question in that library?", and answering it properly would * was kept on the device for a while, because the API could not answer; that
* mean fetching every library's contents on every article which also stamps * was wrong on the second machine and silently so.
* each one as used and reorders the reader's own list. So the answer is kept
* on the device that gave it. It is honest about a save that happened; it will
* not know about one made on another machine. An `article_id` on the join
* table would settle it, and is the right fix when the API can change.
*/ */
const SAVED_KEY = 'pedshub.articleLibraries' export default function ArticleSaveButton({ articleId }) {
const readSaved = () => {
try { return JSON.parse(localStorage.getItem(SAVED_KEY) || '{}') } catch { return {} }
}
const writeSaved = (next) => {
try { localStorage.setItem(SAVED_KEY, JSON.stringify(next)) } catch { /* the visit still knows */ }
}
export default function ArticleSaveButton({ articleId, questionIds = [] }) {
const [open, setOpen] = useState(false) const [open, setOpen] = useState(false)
const [libraries, setLibraries] = useState([]) const [libraries, setLibraries] = useState([])
const [query, setQuery] = useState('') const [query, setQuery] = useState('')
const [busy, setBusy] = useState(false) const [busy, setBusy] = useState(false)
const [saved, setSaved] = useState(() => readSaved()[String(articleId)] || []) const [saved, setSaved] = useState([])
const wrap = useRef(null) const wrap = useRef(null)
useEffect(() => { setSaved(readSaved()[String(articleId)] || []) }, [articleId]) // Asked once per article rather than per library: fetching each library's
// contents to find out would also stamp every one of them as used and
// reorder the reader's own list.
useEffect(() => {
if (!articleId) return undefined
let live = true
api.get(`/collections/for-article/${articleId}`)
.then(res => { if (live) setSaved(res.data?.collection_ids || []) })
.catch(() => { if (live) setSaved([]) })
return () => { live = false }
}, [articleId])
useEffect(() => { useEffect(() => {
if (!open) return if (!open) return
@ -54,26 +50,24 @@ export default function ArticleSaveButton({ articleId, questionIds = [] }) {
return () => document.removeEventListener('mousedown', away) return () => document.removeEventListener('mousedown', away)
}, [open]) }, [open])
const record = useCallback((collectionId) => {
setSaved(prev => {
if (prev.includes(collectionId)) return prev
const next = [...prev, collectionId]
writeSaved({ ...readSaved(), [String(articleId)]: next })
return next
})
}, [articleId])
const saveInto = async (collectionId) => { const saveInto = async (collectionId) => {
setBusy(true) setBusy(true)
try { try {
// Sequential rather than parallel: a topic with forty questions firing await api.put(`/collections/${collectionId}/articles/${articleId}`)
// forty writes at once is a burst the rate limiter reads as abuse. setSaved(prev => (prev.includes(collectionId) ? prev : [...prev, collectionId]))
for (const questionId of questionIds) {
await api.put(`/collections/${collectionId}/questions/${questionId}`)
}
record(collectionId)
setQuery('') setQuery('')
} catch { /* nothing is recorded, which is the honest signal */ } } catch { /* nothing is marked, which is the honest signal */ }
finally { setBusy(false) }
}
// Putting it back is the same control, so the star is a toggle per library
// rather than a one-way door with the undo somewhere else.
const removeFrom = async (collectionId) => {
setBusy(true)
try {
await api.delete(`/collections/${collectionId}/articles/${articleId}`)
setSaved(prev => prev.filter(id => id !== collectionId))
} catch { /* it stays marked, which is what the server still believes */ }
finally { setBusy(false) } finally { setBusy(false) }
} }
@ -100,53 +94,45 @@ export default function ArticleSaveButton({ articleId, questionIds = [] }) {
<span className="asave" ref={wrap}> <span className="asave" ref={wrap}>
<button type="button" className={`article-tool${isSaved ? ' is-on' : ''}`} <button type="button" className={`article-tool${isSaved ? ' is-on' : ''}`}
aria-expanded={open} aria-haspopup="dialog" aria-expanded={open} aria-haspopup="dialog"
aria-label={isSaved ? `Saved to ${saved.length} librar${saved.length === 1 ? 'y' : 'ies'}` : 'Save this topic to a library'} aria-label={isSaved ? `Saved to ${saved.length} librar${saved.length === 1 ? 'y' : 'ies'}` : 'Save this article to a library'}
onClick={() => setOpen(v => !v)}> onClick={() => setOpen(v => !v)}>
<span aria-hidden="true">{isSaved ? '★' : '☆'}</span> <span aria-hidden="true">{isSaved ? '★' : '☆'}</span>
</button> </button>
{open && ( {open && (
<div className="asave-pop" role="dialog" aria-label="Save this topic to a library"> <div className="asave-pop" role="dialog" aria-label="Save this article to a library">
<p className="asave-title">Save this topic</p> <p className="asave-title">Save this article</p>
{questionIds.length === 0 ? ( {/* One box for both. Searching what you have and naming what you do
/* Said plainly rather than offering a control that would file not are the same act you type the name of the library you want
nothing: a library holds questions, and this topic has none yet. */ and it either exists or it doesn't. */}
<p className="asave-empty">No questions are linked to this topic yet, so there is nothing to file. Link one and it can be saved with the reading.</p> <input className="asave-find" value={query} disabled={busy}
) : ( placeholder="Create or find a library" aria-label="Create or find a library"
<> onChange={event => setQuery(event.target.value)}
<p className="asave-note">Files its {questionIds.length} linked question{questionIds.length === 1 ? '' : 's'} so you can sit them later.</p> onKeyDown={event => {
{/* One box for both. Searching what you have and naming what you if (event.key === 'Enter' && canCreate) { event.preventDefault(); createAndSave(query) }
do not are the same act you type the name of the library }} />
you want and it either exists or it doesn't. */} {canCreate && (
<input className="asave-find" value={query} disabled={busy} <button type="button" className="asave-new" disabled={busy} onClick={() => createAndSave(query)}>
placeholder="Create or find a library" aria-label="Create or find a library" <span>{query.trim()}</span><span aria-hidden="true">+</span>
onChange={event => setQuery(event.target.value)} </button>
onKeyDown={event => { )}
if (event.key === 'Enter' && canCreate) { event.preventDefault(); createAndSave(query) } {found.length > 0 && (
}} /> <ul className="asave-list">
{canCreate && ( {found.map(row => {
<button type="button" className="asave-new" disabled={busy} onClick={() => createAndSave(query)}> const inIt = saved.includes(row.id)
<span>{query.trim()}</span><span aria-hidden="true">+</span> return (
</button> <li key={row.id}>
)} <button type="button" className={inIt ? 'is-in' : ''} disabled={busy}
{found.length > 0 && ( aria-pressed={inIt}
<ul className="asave-list"> onClick={() => (inIt ? removeFrom(row.id) : saveInto(row.id))}>
{found.map(row => { {inIt ? '✓ ' : '+ '}{row.title}
const inIt = saved.includes(row.id) </button>
return ( </li>
<li key={row.id}> )
<button type="button" className={inIt ? 'is-in' : ''} disabled={inIt || busy} })}
onClick={() => saveInto(row.id)}> </ul>
{inIt ? '✓ ' : '+ '}{row.title} )}
</button> {!found.length && !canCreate && (
</li> <p className="asave-empty">{libraries.length ? 'No library by that name.' : 'Type a name to make your first library.'}</p>
)
})}
</ul>
)}
{!found.length && !canCreate && (
<p className="asave-empty">{libraries.length ? 'No library by that name.' : 'Type a name to make your first library.'}</p>
)}
</>
)} )}
</div> </div>
)} )}

View file

@ -4,53 +4,70 @@ import userEvent from '@testing-library/user-event'
import ArticleSaveButton from './ArticleSaveButton' import ArticleSaveButton from './ArticleSaveButton'
import api from '../api/client' import api from '../api/client'
vi.mock('../api/client', () => ({ default: { get: vi.fn(), post: vi.fn(), put: vi.fn() } })) vi.mock('../api/client', () => ({
default: { get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn() },
}))
const libraries = [{ id: 4, title: 'Neurology' }, { id: 9, title: 'Airway' }] const libraries = [{ id: 4, title: 'Neurology' }, { id: 9, title: 'Airway' }]
describe('keeping a topic', () => { // Two different GETs: which libraries exist, and which of them already hold
// this article.
const answer = (held = []) => api.get.mockImplementation(url => Promise.resolve({
data: url.startsWith('/collections/for-article/') ? { collection_ids: held } : libraries,
}))
describe('keeping an article', () => {
beforeEach(() => { beforeEach(() => {
vi.resetAllMocks() vi.resetAllMocks()
localStorage.clear() answer()
api.get.mockResolvedValue({ data: libraries })
}) })
it('files the topics questions into a library and says so afterwards', async () => { it('files the article itself into a library and says so afterwards', async () => {
api.put.mockResolvedValue({ data: {} }) api.put.mockResolvedValue({ data: { added: true } })
render(<ArticleSaveButton articleId={1} questionIds={[11, 12]} />) render(<ArticleSaveButton articleId={7} />)
await userEvent.click(screen.getByRole('button', { name: 'Save this topic to a library' })) await userEvent.click(screen.getByRole('button', { name: 'Save this article to a library' }))
const panel = await screen.findByRole('dialog', { name: /Save this topic/ }) const panel = await screen.findByRole('dialog', { name: /Save this article/ })
await userEvent.click(within(panel).getByRole('button', { name: /Neurology/ })) await userEvent.click(within(panel).getByRole('button', { name: /Neurology/ }))
await waitFor(() => expect(api.put).toHaveBeenCalledWith('/collections/4/questions/11')) // The article, not the questions written against it: a topic with none is
expect(api.put).toHaveBeenCalledWith('/collections/4/questions/12') // still worth keeping, and a reader who saved the reading did not ask for
// A reader should be able to tell at a glance that this one is already put // a session.
// away, without opening the panel to find out. await waitFor(() => expect(api.put).toHaveBeenCalledWith('/collections/4/articles/7'))
expect(await screen.findByRole('button', { name: 'Saved to 1 library' })).toBeInTheDocument() expect(await screen.findByRole('button', { name: 'Saved to 1 library' })).toBeInTheDocument()
}) })
it('shows it is already kept, from the server rather than from this device', async () => {
answer([9])
render(<ArticleSaveButton articleId={7} />)
// Nothing was saved in this browser; the server was asked.
expect(await screen.findByRole('button', { name: 'Saved to 1 library' })).toBeInTheDocument()
})
it('takes it back out again from the same control', async () => {
answer([4])
api.delete.mockResolvedValue({})
render(<ArticleSaveButton articleId={7} />)
await userEvent.click(await screen.findByRole('button', { name: 'Saved to 1 library' }))
const panel = await screen.findByRole('dialog', { name: /Save this article/ })
await userEvent.click(within(panel).getByRole('button', { name: /Neurology/ }))
await waitFor(() => expect(api.delete).toHaveBeenCalledWith('/collections/4/articles/7'))
expect(await screen.findByRole('button', { name: 'Save this article to a library' })).toBeInTheDocument()
})
it('names a new library and files into it in one press', async () => { it('names a new library and files into it in one press', async () => {
api.post.mockResolvedValue({ data: { id: 21, title: 'Seizures' } }) api.post.mockResolvedValue({ data: { id: 21, title: 'Seizures' } })
api.put.mockResolvedValue({ data: {} }) api.put.mockResolvedValue({ data: { added: true } })
render(<ArticleSaveButton articleId={1} questionIds={[11]} />) render(<ArticleSaveButton articleId={7} />)
await userEvent.click(screen.getByRole('button', { name: 'Save this topic to a library' })) await userEvent.click(screen.getByRole('button', { name: 'Save this article to a library' }))
await screen.findByRole('dialog', { name: /Save this topic/ }) await screen.findByRole('dialog', { name: /Save this article/ })
await userEvent.type(screen.getByLabelText('Create or find a library'), 'Seizures') await userEvent.type(screen.getByLabelText('Create or find a library'), 'Seizures')
await userEvent.click(screen.getByRole('button', { name: /Seizures/ })) await userEvent.click(screen.getByRole('button', { name: /Seizures/ }))
await waitFor(() => expect(api.post).toHaveBeenCalledWith('/collections/', { title: 'Seizures' })) await waitFor(() => expect(api.post).toHaveBeenCalledWith('/collections/', { title: 'Seizures' }))
expect(api.put).toHaveBeenCalledWith('/collections/21/questions/11') expect(api.put).toHaveBeenCalledWith('/collections/21/articles/7')
})
// A library holds questions. Offering the control anyway would file nothing
// and say nothing about why.
it('says plainly when a topic has no questions to file', async () => {
render(<ArticleSaveButton articleId={1} questionIds={[]} />)
await userEvent.click(screen.getByRole('button', { name: 'Save this topic to a library' }))
expect(await screen.findByText(/No questions are linked to this topic yet/)).toBeInTheDocument()
expect(screen.queryByLabelText('Create or find a library')).toBeNull()
}) })
}) })

View file

@ -307,9 +307,28 @@
.asec-body { font-size: 0.92rem; } .asec-body { font-size: 0.92rem; }
} }
/* One topic, three readings where AMBOSS puts High-yield, at the right of /* One topic, three readings at the right of the row over the sections. The
the row over the sections. The rail is filtered by the same choice, so the rail is filtered by the same choice, so the contents can never list a
contents can never list a section the body is no longer showing. */ section the body is no longer showing. */
.aview-group { display: inline-flex; align-items: center; gap: 10px; }
/* Summary is a toggle, not a tab: it is the whole topic or the part of it
worth revising, and it names its own state so a reader can tell at a glance
why two thirds of the contents are not there. */
.aview-only {
display: inline-flex; align-items: center; gap: 8px;
min-height: 36px; padding: 7px 14px; cursor: pointer;
font: inherit; font-size: 0.83rem; font-weight: 600;
color: var(--text-muted); background: var(--card-bg);
border: 1px solid var(--border); border-radius: 8px;
}
.aview-only span { font-size: 0.95rem; line-height: 1; }
.aview-only:hover { color: var(--text); border-color: var(--text-subtle); }
.aview-only.is-on {
color: var(--primary); border-color: var(--primary);
background: color-mix(in srgb, var(--primary) 8%, transparent);
}
.aview-switch { display: inline-flex; gap: 2px; padding: 3px; background: var(--bg); border: 1px solid var(--border); border-radius: 9px; } .aview-switch { display: inline-flex; gap: 2px; padding: 3px; background: var(--bg); border: 1px solid var(--border); border-radius: 9px; }
.aview { .aview {
min-height: 34px; padding: 6px 13px; border: 0; border-radius: 7px; cursor: pointer; min-height: 34px; padding: 6px 13px; border: 0; border-radius: 7px; cursor: pointer;

View file

@ -405,7 +405,7 @@ export function ArticlePage() {
breadcrumbs={breadcrumbs} breadcrumbs={breadcrumbs}
toolbar={ toolbar={
<> <>
<ArticleSaveButton articleId={article.id} questionIds={questions.map(q => q.question_id)} /> <ArticleSaveButton articleId={article.id} />
{editorActions} {editorActions}
</> </>
} }

View file

@ -108,7 +108,7 @@ describe('topic reading', () => {
expect(screen.getByRole('navigation', { name: 'Breadcrumb' })).toHaveTextContent('Neurology') expect(screen.getByRole('navigation', { name: 'Breadcrumb' })).toHaveTextContent('Neurology')
expect(screen.getByText('Introduction markdown')).toBeInTheDocument() expect(screen.getByText('Introduction markdown')).toBeInTheDocument()
// A view of one section is the prose, not a contents page of one entry. // A view of one section is the prose, not a contents page of one entry.
// The tab already names the view "Short" over a heading reading "In // The control already names the view "Summary" over a heading reading "In
// short" says the same word twice and hides the only content behind a // short" says the same word twice and hides the only content behind a
// chevron. // chevron.
expect(screen.getByText('Section markdown')).toBeInTheDocument() expect(screen.getByText('Section markdown')).toBeInTheDocument()
@ -242,9 +242,10 @@ describe('topic reading', () => {
expect(within(card).queryByRole('link')).toBeNull() expect(within(card).queryByRole('link')).toBeNull()
}) })
// Short is our high-yield. The contents are filtered by the same choice as // Summary is our high-yield, and it is a toggle rather than one tab of
// the body, so the rail can never offer a heading the article is no longer // three. The contents are filtered by the same choice as the body, so the
// showing which is the failure worth guarding, not the switch itself. // rail can never offer a heading the article is no longer showing which is
// the failure worth guarding, not the switch itself.
it('keeps the contents rail in step with the depth being read', async () => { it('keeps the contents rail in step with the depth being read', async () => {
const layered = { ...article, sections: [ const layered = { ...article, sections: [
{ id: 'a'.repeat(32), slug: 'key', title: 'Key points', content: 'Key body', variant: 'short' }, { id: 'a'.repeat(32), slug: 'key', title: 'Key points', content: 'Key body', variant: 'short' },
@ -262,13 +263,18 @@ describe('topic reading', () => {
const toc = () => document.querySelector('.article-sections') const toc = () => document.querySelector('.article-sections')
expect(within(toc()).getByRole('button', { name: 'Key points' })).toBeInTheDocument() expect(within(toc()).getByRole('button', { name: 'Key points' })).toBeInTheDocument()
expect(within(toc()).queryByRole('button', { name: 'Pathophysiology' })).toBeNull() expect(within(toc()).queryByRole('button', { name: 'Pathophysiology' })).toBeNull()
expect(screen.getByRole('tab', { name: 'Short' })).toHaveAttribute('aria-selected', 'true') // It names its own state, so a reader can tell why two thirds of the
// contents are not there.
const summary = () => screen.getByRole('button', { name: /^Summary (on|off)$/ })
expect(summary()).toHaveAttribute('aria-pressed', 'true')
expect(summary()).toHaveTextContent('Summary on')
await userEvent.click(screen.getByRole('tab', { name: 'Long' })) await userEvent.click(summary())
expect(within(toc()).getByRole('button', { name: 'Pathophysiology' })).toBeInTheDocument() expect(within(toc()).getByRole('button', { name: 'Pathophysiology' })).toBeInTheDocument()
expect(within(toc()).queryByRole('button', { name: 'Key points' })).toBeNull() expect(within(toc()).queryByRole('button', { name: 'Key points' })).toBeNull()
expect(screen.queryByText('Key body')).not.toBeInTheDocument() expect(screen.queryByText('Key body')).not.toBeInTheDocument()
expect(screen.getByRole('tab', { name: 'Long' })).toHaveAttribute('aria-selected', 'true') expect(summary()).toHaveAttribute('aria-pressed', 'false')
expect(summary()).toHaveTextContent('Summary off')
}) })
it('nests a sub-section under its parent and keeps references last', async () => { it('nests a sub-section under its parent and keeps references last', async () => {