diff --git a/apps/api/schema.sql b/apps/api/schema.sql index 3950a2d..dfbc96e 100644 --- a/apps/api/schema.sql +++ b/apps/api/schema.sql @@ -262,9 +262,30 @@ create table if not exists public.lesson_pull_requests ( resolved_at timestamptz, -- Recorded so an admin can later ban the address, as for lessons and comments. author_ip text, - created_at timestamptz not null default now() + created_at timestamptz not null default now(), + -- The branch of the fork this was proposed from, for display. A proposer can + -- work on a variation of their fork (see /web-app/lesson-variations) and offer + -- that, so "which one is this?" is a question the queue has to be able to + -- answer. Null on a proposal opened before this, and on one from the default + -- branch, where naming it would say nothing. + head_ref text, + -- A proposal can be updated while it is open, and each upload is a revision. + -- `revision` counts them from 1; `previous_head` is the commit the proposal + -- pointed at before the most recent one, which is what makes "what changed in + -- this update" answerable — both commits are in the stored pack, because an + -- update may only move the proposer's branch forward. + revision integer not null default 1, + previous_head text, + updated_at timestamptz ); +-- Columns added after the table shipped. Safe to re-run, and safe on a database +-- that already has them. +alter table public.lesson_pull_requests add column if not exists head_ref text; +alter table public.lesson_pull_requests add column if not exists revision integer not null default 1; +alter table public.lesson_pull_requests add column if not exists previous_head text; +alter table public.lesson_pull_requests add column if not exists updated_at timestamptz; + -- The lesson page and the editor both ask "the open proposals on this lesson, -- newest first"; index the filter + sort key. create index if not exists lesson_pull_requests_lesson_idx diff --git a/apps/api/src/lib/cors.js b/apps/api/src/lib/cors.js index 91db094..b785d49 100644 --- a/apps/api/src/lib/cors.js +++ b/apps/api/src/lib/cors.js @@ -33,13 +33,18 @@ export function corsHeaders(request, allowed) { headers.set('Access-Control-Allow-Origin', origin); headers.set('Vary', 'Origin'); headers.set('Access-Control-Allow-Methods', 'GET, POST, PUT, DELETE, OPTIONS'); - // X-Git-Head and X-Git-Parent carry a pushed history's tip and the head it - // expects to replace (the compare-and-swap in routes/git.js), and the same - // tip when a pull request's pack is uploaded. Neither is a safelisted - // header, so without naming them here the browser's preflight refuses the - // upload — invisibly, whenever the app is served from a different origin - // than the API. Same-origin deploys never preflight and so never noticed. - headers.set('Access-Control-Allow-Headers', 'Content-Type, Authorization, X-Git-Head, X-Git-Parent'); + // The X-Git-* headers carry a pushed history's tips and the tips it expects + // to replace (the compare-and-swap in routes/git.js) — X-Git-Head and + // X-Git-Parent for the lesson itself, and Refs/Expected/Deletes for its + // variations. The same tip travels on a pull request's pack upload. None is + // a safelisted header, so without naming them here the browser's preflight + // refuses the upload — invisibly, whenever the app is served from a + // different origin than the API. Same-origin deploys never preflight and so + // never noticed. + headers.set( + 'Access-Control-Allow-Headers', + 'Content-Type, Authorization, X-Git-Head, X-Git-Parent, X-Git-Refs, X-Git-Expected, X-Git-Deletes', + ); headers.set('Access-Control-Max-Age', '86400'); } } diff --git a/apps/api/src/lib/lessonGit.js b/apps/api/src/lib/lessonGit.js index 5d0ebb9..0f433c7 100644 --- a/apps/api/src/lib/lessonGit.js +++ b/apps/api/src/lib/lessonGit.js @@ -6,7 +6,14 @@ // here. Two objects per lesson: // // git//pack the packfile bytes -// git//refs.json { head, size, updatedAt } +// git//refs.json { head, refs, size, updatedAt } +// +// `head` is the lesson itself — the default branch — and `refs` maps every branch +// the repository holds to its tip, one per *variation* its author is trying out +// (see /web-app/lesson-variations). `head` came first and is kept as its own +// field rather than being read out of the map: everything that wants "the +// lesson" wants exactly that one branch, and a reader written before variations +// existed still gets the right answer from it. // // A pull request — someone proposing changes to a lesson they don't own — is a // packfile too, snapshotted when the request was opened, under its own key: @@ -28,8 +35,19 @@ import { supabaseHeaders } from './supabase.js'; // A lesson id is a UUID — pinned down because it's interpolated into an R2 key. export const LESSON_ID_RE = /^[0-9a-fA-F-]{36}$/; -// A commit oid is a 40-char lowercase hex SHA-1. -export const OID_RE = /^[0-9a-f]{40}$/; +// A commit oid is a 40-char lowercase hex SHA-1, and a branch name is what the +// editor is allowed to create. Both rules come from the shared module so the +// Worker validates a push against exactly what the client validated it against — +// the same arrangement as the pull-request length limits. +export { + DEFAULT_BRANCH, + OID_RE, + MAX_BRANCHES, + MAX_BRANCH_NAME, + isBranchName, + parseRefMap, + serializeRefMap, +} from '@spelling-creator/core/git/refs'; // Cap one repository's history. Packs hold only JSON (images are referenced by // hash and live in the images bucket, not in the repo), so even a long-lived diff --git a/apps/api/src/routes/git.js b/apps/api/src/routes/git.js index ede3717..28c8b37 100644 --- a/apps/api/src/routes/git.js +++ b/apps/api/src/routes/git.js @@ -6,8 +6,9 @@ // it here the way git itself moves history: as a packfile holding every object // reachable from the lesson's branch, plus the commit its branch points at. // -// GET /git/:lessonId/refs public* -> { head, size, updatedAt } | 404 -// GET /git/:lessonId/pack public* -> the packfile (X-Git-Head names its tip) +// GET /git/:lessonId/refs public* -> { head, refs, size, updatedAt } | 404 +// GET /git/:lessonId/pack public* -> the packfile (X-Git-Head names its tip, +// X-Git-Refs every branch it holds) // PUT /git/:lessonId/pack Bearer -> store it (the author, or a trusted collaborator) // // A pull request's proposed history is a packfile too, but it belongs to the @@ -24,11 +25,24 @@ // // Two R2 objects per lesson, mirroring the /images routes' use of the bucket: // git//pack the packfile bytes -// git//refs.json { head, size, updatedAt } +// git//refs.json { head, refs, size, updatedAt } // -// The pack carries its own tip in customMetadata (and in the X-Git-Head response -// header). A clone therefore reads the head from the *same object* as the bytes, -// so it can never pair a new refs.json with a stale pack. +// The pack carries its own tip *and* its branch map in customMetadata (echoed in +// the X-Git-Head and X-Git-Refs response headers). A clone therefore reads both +// from the *same object* as the bytes, so it can never pair a new refs.json with +// a stale pack. +// +// ---- More than one branch --------------------------------------------------- +// +// `head` is the lesson: the default branch, and the only one a reader, a forker +// or the lesson's own page ever asks for. `refs` maps every branch the repository +// holds to its tip — one per *variation* its author is trying out, kept here so a +// variation follows the lesson between the author's devices rather than living +// only in the browser it was started in. +// +// A variation is exactly as public as the lesson it belongs to: it is in the same +// pack, and the pack of a published lesson is public so that forking is. See +// /web-app/lesson-variations, which says so where an author can read it. // // ---- Who may push, and why it can't lose work ------------------------------- // @@ -48,20 +62,113 @@ // with 409 and the client must fetch, merge, and retry. Since the client only // pushes a history that already *contains* the head it merged, an accepted push // can only ever move the lesson forward. +// +// The same rule runs one level down for the variations, per branch: +// +// X-Git-Refs the branches to set, `{ "": "" }` +// X-Git-Expected what the client believes we hold for each name it touches, +// with "" meaning "I believe this one does not exist yet" +// X-Git-Deletes the branches to remove, comma-separated +// +// It is all-or-nothing, which costs nothing to arrange: refs.json is a single +// object and already the commit point, so every branch advances or none does. A +// branch the client doesn't mention is left exactly as it is — that is what stops +// a device which has never heard of a new variation from deleting it by omission, +// and it is why a delete has to be asked for by name rather than inferred. import { bearerToken } from '../lib/auth.js'; import { bannedResponse } from '../lib/bans.js'; import { canReadLesson, fetchLessonRow, isTrustedCollaborator } from '../lib/lesson.js'; -import { LESSON_ID_RE, MAX_PACK_BYTES, OID_RE, isPackfile, packKey, refsKey } from '../lib/lessonGit.js'; +import { + DEFAULT_BRANCH, + LESSON_ID_RE, + MAX_BRANCHES, + MAX_PACK_BYTES, + OID_RE, + isBranchName, + isPackfile, + packKey, + parseRefMap, + refsKey, + serializeRefMap, +} from '../lib/lessonGit.js'; import { supabaseBase, supabaseConfigured, verifySupabaseUser } from '../lib/supabase.js'; import { textResponse, jsonResponse } from '../lib/http.js'; -/** The head we currently hold for a lesson, or null if it has no history yet. */ -async function storedHead(env, lessonId) { +/** What we currently hold for a lesson: `{ head, refs }`, or null with no history. */ +async function stored(env, lessonId) { const object = await env.LESSON_GIT.get(refsKey(lessonId)); if (!object) return null; - const refs = await object.json().catch(() => null); - return refs && refs.head ? refs.head : null; + const value = await object.json().catch(() => null); + if (!value || !value.head) return null; + + // A lesson stored before variations existed has no map; the one branch it has + // is its head, and saying so here means the rest of this file has one shape to + // reason about rather than two. + // + // A map that is *present but unreadable* is a different thing entirely, and must + // not be flattened into the same answer. Both of parseRefMap's limits — the + // branch count and the name rules — can be tightened later, and if either is, + // every lesson past the new limit would read as single-branch here: the push + // path would skip its old-client guard and write the variations away. So say we + // don't know, and let the caller refuse. + const refs = value.refs === undefined ? { [DEFAULT_BRANCH]: value.head } : parseRefMap(value.refs); + if (!refs) return { head: value.head, refs: null, unreadable: true }; + return { head: value.head, refs }; +} + +/** + * Apply a push's ref instructions to what we hold, or explain why we won't. + * + * The rule is the one that has always guarded this endpoint, applied per branch + * rather than to the lesson as a whole: a client may only move a branch it has + * already seen the current state of. `expected` is what it believes, and a branch + * it doesn't mention at all is left untouched — which is what stops a device that + * has never heard of somebody's new variation from deleting it by omission. + * + * @returns {{ refs: object } | { error: string, status: number }} + */ +export function applyRefs(current, { refs, deletes, expected }) { + const next = { ...current }; + + const believes = (name) => (Object.hasOwn(expected, name) ? expected[name] : null); + const mismatch = (name) => { + const believed = believes(name); + // Nothing claimed about this branch: only safe when it is new to us. Moving + // one we already hold without saying what we hold is exactly the overwrite + // the compare-and-swap exists to refuse. + if (believed === null) return Boolean(current[name]); + return (current[name] || '') !== believed; + }; + + // A name in both halves is a request that contradicts itself, and we cannot know + // which half was meant. Refusing is the only honest answer — applying them in + // order would silently let the delete win, which is how a branch that is alive + // on the client disappears from the hub. + for (const name of deletes) { + if (Object.hasOwn(refs, name)) { + return { error: 'That push asks to both keep and remove the same version.', status: 400 }; + } + } + + for (const name of Object.keys(refs)) { + if (mismatch(name)) return { error: 'moved', status: 409 }; + next[name] = refs[name]; + } + for (const name of deletes) { + if (!isBranchName(name)) return { error: 'That is not a branch name.', status: 400 }; + if (mismatch(name)) return { error: 'moved', status: 409 }; + delete next[name]; + } + + // The lesson has to still be there afterwards. Deleting the branch that *is* + // the lesson would leave a row whose history advertises a tip nothing points + // at, and no client asks for that. + if (!next[DEFAULT_BRANCH]) return { error: 'The lesson’s own history cannot be removed.', status: 400 }; + if (Object.keys(next).length > MAX_BRANCHES) { + return { error: `A lesson can have at most ${MAX_BRANCHES} versions.`, status: 400 }; + } + return { refs: next }; } /** @@ -128,8 +235,13 @@ export async function handleGit(request, env, lessonId, rest, cors) { headers.set('Cache-Control', 'no-store'); const head = object.customMetadata?.head || ''; if (head) headers.set('X-Git-Head', head); - // The SPA reads X-Git-Head cross-origin, which needs it explicitly exposed. - headers.set('Access-Control-Expose-Headers', 'X-Git-Head'); + // The branch map comes off the *same object* as the bytes, for the reason the + // head does: a map read from refs.json a moment later could name a tip this + // pack doesn't contain. + const refs = object.customMetadata?.refs || ''; + if (refs) headers.set('X-Git-Refs', refs); + // The SPA reads these cross-origin, which needs them explicitly exposed. + headers.set('Access-Control-Expose-Headers', 'X-Git-Head, X-Git-Refs'); if (object.httpEtag) headers.set('ETag', object.httpEtag); return new Response(object.body, { status: 200, headers }); } @@ -162,12 +274,43 @@ export async function handleGit(request, env, lessonId, rest, cors) { return textResponse('Missing or invalid X-Git-Head.', 400, cors); } + // What the client wants to happen to the lesson's branches. A client that + // sends none of this is one written before a lesson could have more than one + // branch: it means "move the lesson, leave everything else alone", and the + // two X-Git-Head/X-Git-Parent headers already say that. + const requested = parseRefMap(request.headers.get('X-Git-Refs')); + const believed = parseRefMap(request.headers.get('X-Git-Expected')); + if (request.headers.get('X-Git-Refs') && !requested) { + return textResponse('Invalid X-Git-Refs.', 400, cors); + } + if (request.headers.get('X-Git-Expected') && !believed) { + return textResponse('Invalid X-Git-Expected.', 400, cors); + } + // A ref being *set* must name a real commit; only X-Git-Expected may say "" + // (meaning "I believe this branch does not exist yet"). + if (requested && Object.values(requested).some((oid) => !OID_RE.test(oid))) { + return textResponse('Invalid X-Git-Refs.', 400, cors); + } + + const deletes = (request.headers.get('X-Git-Deletes') || '') + .split(',') + .map((name) => name.trim()) + .filter(Boolean); + // Compare-and-swap. `parent` is the head the client merged before building // this pack; if the lesson has moved on since (someone else pushed), we // refuse — accepting would drop their commits. 409 tells the client to // fetch, merge and try again. const parent = (request.headers.get('X-Git-Parent') || '').trim(); - const current = await storedHead(env, lessonId); + const held = await stored(env, lessonId); + const current = held?.head || null; + + // We hold a branch map we can't read. Every path below decides what to keep + // and what to drop by comparing against it, so none of them can run safely. + if (held?.unreadable) { + return textResponse('This lesson’s stored history could not be read. Please report this rather than saving over it.', 409, cors); + } + if (current && parent !== current) { return textResponse( 'This lesson’s history has moved on since you last synced. Merge the latest changes, then save again.', @@ -175,11 +318,47 @@ export async function handleGit(request, env, lessonId, rest, cors) { cors, ); } - // Already there — the client is re-pushing a history we hold. Nothing to do. - if (current && current === head) { + + // A pack holds every object every branch it advertises needs. An old client + // packs only the lesson's own branch, so accepting its pack while keeping a + // variation's tip in the map would leave that tip pointing at objects the + // stored pack no longer contains — a history that can't be cloned. Rather + // than silently drop the variation, refuse and say why. + const heldBranches = Object.keys(held?.refs || {}); + if (!requested && heldBranches.length > 1) { + return textResponse( + 'This lesson has more than one version, which this client cannot save without dropping them. Please update it, or save from the web editor.', + 409, + cors, + ); + } + + const applied = applyRefs(held?.refs || {}, { + refs: requested || { [DEFAULT_BRANCH]: head }, + deletes, + expected: believed || (current ? { [DEFAULT_BRANCH]: current } : {}), + }); + if (applied.error) { + return textResponse( + applied.status === 409 + ? 'One of this lesson’s versions has moved on since you last synced. Merge the latest changes, then save again.' + : applied.error, + applied.status, + cors, + ); + } + // The lesson's own tip and the map have to agree — they are two readings of + // one fact, and a client that disagrees with itself is one we can't apply. + if (applied.refs[DEFAULT_BRANCH] !== head) { + return textResponse('X-Git-Head and X-Git-Refs disagree about this lesson.', 400, cors); + } + + // Already there — the client is re-pushing exactly what we hold, branches and + // all. Nothing to write. + if (current && current === head && serializeRefMap(applied.refs) === serializeRefMap(held.refs)) { const object = await env.LESSON_GIT.get(refsKey(lessonId)); - const refs = object ? await object.json().catch(() => null) : null; - if (refs) return jsonResponse(refs, 200, cors); + const existing = object ? await object.json().catch(() => null) : null; + if (existing) return jsonResponse(existing, 200, cors); } const declared = Number(request.headers.get('Content-Length') || 0); @@ -201,11 +380,12 @@ export async function handleGit(request, env, lessonId, rest, cors) { // Write the pack before the refs. The refs object is the commit point: if the // pack write fails, refs still names the previous (complete) pack, and a // clone of the old history beats a clone of a half-written one. + const map = serializeRefMap(applied.refs); await env.LESSON_GIT.put(packKey(lessonId), bytes, { httpMetadata: { contentType: 'application/x-git-packfile' }, - customMetadata: { head }, + customMetadata: { head, refs: map }, }); - const refs = { head, size: bytes.byteLength, updatedAt: new Date().toISOString() }; + const refs = { head, refs: applied.refs, size: bytes.byteLength, updatedAt: new Date().toISOString() }; await env.LESSON_GIT.put(refsKey(lessonId), JSON.stringify(refs), { httpMetadata: { contentType: 'application/json' }, }); diff --git a/apps/api/src/routes/git.test.js b/apps/api/src/routes/git.test.js new file mode 100644 index 0000000..2a563b6 --- /dev/null +++ b/apps/api/src/routes/git.test.js @@ -0,0 +1,91 @@ +// The per-branch compare-and-swap. +// +// This is the rule that decides whether a push loses somebody's work, and it is +// pure — so it is worth testing directly rather than through R2. The cases below +// are the ones that actually happen: two of the author's devices, an old client +// that knows nothing about variations, and a delete racing a rename. + +import { describe, expect, it } from 'vitest'; +import { applyRefs } from './git.js'; + +const A = 'a'.repeat(40); +const B = 'b'.repeat(40); +const C = 'c'.repeat(40); + +const push = (current, instructions) => applyRefs(current, { refs: {}, deletes: [], expected: {}, ...instructions }); + +describe('applyRefs', () => { + it('moves a branch the client had seen the current state of', () => { + const result = push({ main: A }, { refs: { main: B }, expected: { main: A } }); + expect(result.refs).toEqual({ main: B }); + }); + + it('refuses to move a branch the client is out of date on', () => { + const result = push({ main: B }, { refs: { main: C }, expected: { main: A } }); + expect(result.status).toBe(409); + }); + + it('refuses to move a branch the client says nothing about', () => { + // Silence means "I believe this is new". It is not, so the push would + // overwrite whatever moved it. + const result = push({ main: A }, { refs: { main: B } }); + expect(result.status).toBe(409); + }); + + it('accepts a branch the client correctly believes is new', () => { + const result = push({ main: A }, { refs: { main: A, 'Year-3': B }, expected: { main: A, 'Year-3': '' } }); + expect(result.refs).toEqual({ main: A, 'Year-3': B }); + }); + + it('refuses a new branch whose name somebody else has already used', () => { + const result = push({ main: A, 'Year-3': B }, { refs: { main: A, 'Year-3': C }, expected: { main: A, 'Year-3': '' } }); + expect(result.status).toBe(409); + }); + + it('leaves branches the push never mentions exactly as they were', () => { + // The case this exists for: one device saves without ever having heard of a + // variation another device made. It must not disappear. + const result = push({ main: A, 'Year-3': B }, { refs: { main: C }, expected: { main: A } }); + expect(result.refs).toEqual({ main: C, 'Year-3': B }); + }); + + it('removes a branch when asked by name, and only at the tip it was asked for', () => { + expect(push({ main: A, 'Year-3': B }, { refs: { main: A }, deletes: ['Year-3'], expected: { main: A, 'Year-3': B } }).refs).toEqual({ + main: A, + }); + + // Somebody added to the variation after we decided to delete it. + expect(push({ main: A, 'Year-3': C }, { refs: { main: A }, deletes: ['Year-3'], expected: { main: A, 'Year-3': B } }).status).toBe(409); + }); + + it('refuses a push that both sets and deletes one branch', () => { + // Reachable from a delete-then-recreate on the client: the request + // contradicts itself, and applying it in order would let the delete win. + const result = push( + { main: A, 'Year-3': B }, + { refs: { main: A, 'Year-3': C }, deletes: ['Year-3'], expected: { main: A, 'Year-3': B } }, + ); + expect(result.status).toBe(400); + }); + + it('will not delete the lesson itself', () => { + const result = push({ main: A }, { deletes: ['main'], expected: { main: A } }); + expect(result.status).toBe(400); + }); + + it('rejects a delete that is not a branch name at all', () => { + const result = push({ main: A }, { refs: { main: A }, deletes: ['../evil'], expected: { main: A } }); + expect(result.status).toBe(400); + }); + + it('caps how many branches a lesson can end up with', () => { + const current = { main: A }; + const refs = { main: A }; + const expected = { main: A }; + for (let i = 0; i < 20; i++) { + refs[`v${i}`] = B; + expected[`v${i}`] = ''; + } + expect(push(current, { refs, expected }).status).toBe(400); + }); +}); diff --git a/apps/api/src/routes/pulls.js b/apps/api/src/routes/pulls.js index 212a79a..a6af9de 100644 --- a/apps/api/src/routes/pulls.js +++ b/apps/api/src/routes/pulls.js @@ -48,13 +48,23 @@ import { authorFromUser, bearerToken, clientIp, displayNameOf, isModeratorRole, verifyUserAndRole } from '../lib/auth.js'; import { bannedResponse } from '../lib/bans.js'; import { canReadLesson, fetchLessonRow, isTrustedCollaborator } from '../lib/lesson.js'; -import { LESSON_ID_RE, MAX_PACK_BYTES, OID_RE, deletePullGit, isPackfile, pullPackKey, refsKey } from '../lib/lessonGit.js'; +import { + DEFAULT_BRANCH, + LESSON_ID_RE, + MAX_PACK_BYTES, + OID_RE, + deletePullGit, + isBranchName, + isPackfile, + pullPackKey, + refsKey, +} from '../lib/lessonGit.js'; import { profanityFilter } from '../lib/profanity.js'; import { supabaseBase, supabaseConfigured, supabaseHeaders, verifySupabaseUser } from '../lib/supabase.js'; import { textResponse, jsonResponse } from '../lib/http.js'; import { createNotification } from './notifications.js'; // The same limits the submission form counts against, so the two can't drift. -import { PULL_BODY_MAX, PULL_TITLE_MAX } from '@spelling-creator/core/pulls'; +import { MAX_PULL_REVISIONS, PULL_BODY_MAX, PULL_TITLE_MAX } from '@spelling-creator/core/pulls'; // How many proposals one person may have open against one lesson at a time. // High enough that nobody splitting real work across several requests will ever @@ -62,7 +72,7 @@ import { PULL_BODY_MAX, PULL_TITLE_MAX } from '@spelling-creator/core/pulls'; const MAX_OPEN_PER_AUTHOR = 5; const PULL_COLUMNS = - 'id,lesson_id,source_lesson_id,author_id,author,title,body,head,base,ready,status,merge_commit,resolved_by,resolved_at,created_at'; + 'id,lesson_id,source_lesson_id,author_id,author,title,body,head,head_ref,base,ready,status,merge_commit,resolved_by,resolved_at,created_at,revision,previous_head,updated_at'; /** Map a Supabase `lesson_pull_requests` row to the camelCase shape the frontend expects. */ function rowToPull(row) { @@ -80,6 +90,14 @@ function rowToPull(row) { // The commit the stored pack points at, and the lesson tip it was built on. head: row.head, base: row.base || null, + // Which branch of the fork this is, when it isn't the fork's own lesson. + headRef: row.head_ref || null, + // How many times the proposer has uploaded, and what the proposal pointed + // at before the most recent time — enough to show "updated, and here is + // what that update changed" without keeping a pack per revision. + revision: row.revision || 1, + previousHead: row.previous_head || null, + updatedAt: row.updated_at || null, // False until the packfile landed — see the note at the top of this file. ready: Boolean(row.ready), status: row.status, @@ -306,6 +324,8 @@ async function openPull(request, env, base, lessonId, cors) { // is dropped rather than rejected — the pack is what carries the changes. // // Resolved before the own-lesson check below, which turns on it. + const headRef = typeof body.headRef === 'string' ? body.headRef.trim() : ''; + let sourceLessonId = null; // Whether that source is a fork *of this lesson*, which is a stricter thing and // the only one that may unlock a self-proposal below. @@ -368,6 +388,10 @@ async function openPull(request, env, base, lessonId, cors) { title: parsed.title, body: parsed.text || null, head, + // Which branch of their fork this is, when it isn't the fork itself. Purely + // for display, and validated as a branch name because it is shown verbatim + // in the review queue. + head_ref: isBranchName(headRef) && headRef !== DEFAULT_BRANCH ? headRef : null, base: OID_RE.test(baseOid) ? baseOid : null, ready: false, status: 'open', @@ -395,13 +419,78 @@ async function openPull(request, env, base, lessonId, cors) { return jsonResponse({ pull: rowToPull(rows[0]) }, 201, cors); } +/** + * What an upload to a proposal should do — or why it shouldn't. + * + * The first upload completes the two-step open, so its tip has to be the one the + * row was created with. A later one is an update, and must actually move: + * re-sending the commit we already hold is a no-op dressed as a new revision. + * + * `expect` is the conditional the row write carries. Including `head` in it is + * what makes two of the proposer's own uploads racing safe — the loser finds the + * row no longer where it read it, and says so rather than overwriting. + * + * @returns {{ error: string, status: number } | { updating: boolean, patch: object, expect: object }} + */ +export function planPullUpload(pull, head, now = new Date().toISOString()) { + const updating = Boolean(pull.ready); + + if (!updating) { + if (head !== pull.head) { + return { + error: 'These changes don’t match the proposal they were opened with. Start the proposal again.', + status: 409, + }; + } + return { updating, patch: { ready: true }, expect: { status: 'open', ready: false, head: pull.head } }; + } + + if (head === pull.head) { + return { error: 'These changes are already what this proposal contains.', status: 409 }; + } + const revision = pull.revision || 1; + if (revision >= MAX_PULL_REVISIONS) { + return { + error: `This proposal has been updated ${MAX_PULL_REVISIONS} times. Close it and open a new one.`, + status: 409, + }; + } + return { + updating, + patch: { head, previous_head: pull.head, revision: revision + 1, updated_at: now }, + expect: { status: 'open', ready: true, head: pull.head }, + }; +} + /** * PUT /lessons/:id/pulls/:prId/pack — upload the proposal's packfile. * - * The proposer's only, once, while the request is open: the tip must match the - * head the row was opened with, and a request that is already ready is not - * re-writable. Together those are what make a proposal a snapshot — there is no - * way to swap the contents out from under a reviewer who has already read it. + * The proposer's only, while the request is open. + * + * ---- Updating an open proposal ---------------------------------------------- + * + * This used to be a once-only write, so that a reviewer could not have the + * contents swapped out from under them. That reasoning is right and is kept — but + * the consequence was that changing a proposal meant closing it and opening a new + * one, which threw away the conversation attached to it. + * + * So an upload to an already-ready proposal is allowed, and *recorded*: the + * revision number goes up, the head it used to point at is kept, and the row's + * `updated_at` says when. A reviewer is never silently shown different bytes; they + * are shown that it changed, and what the change was. The stability that mattered + * was never immutability, it was that nothing moves without saying so. + * + * One pack per proposal, not one per revision. The proposer's branch only moves + * forward, so the commit the previous revision pointed at is still reachable in + * the new pack — which is what lets "what changed in this update" be answered + * from the pack we already store. + * + * That forward-only rule is enforced by the client, not here: verifying it means + * walking the commit graph, and this Worker holds a proposal's history as an + * opaque packfile with no filesystem to index one into — the same limit that + * stops the merge endpoint checking ancestry (see mergePull below). The exposure + * is bounded the same way, too: the only proposal a proposer can rewrite is their + * own, which they could always have closed and reopened instead. */ async function putPullPack(request, env, base, lessonId, pullId, cors) { if (!env.LESSON_GIT) return textResponse('Lesson history is not configured.', 500, cors); @@ -416,13 +505,13 @@ async function putPullPack(request, env, base, lessonId, pullId, cors) { if (!pull) return textResponse('Proposal not found.', 404, cors); if (pull.author_id !== user.id) return textResponse('You can only upload changes for your own proposal.', 403, cors); if (pull.status !== 'open') return textResponse('This proposal is no longer open.', 409, cors); - if (pull.ready) return textResponse('This proposal’s changes have already been uploaded.', 409, cors); const head = (request.headers.get('X-Git-Head') || '').trim(); if (!OID_RE.test(head)) return textResponse('Missing or invalid X-Git-Head.', 400, cors); - if (head !== pull.head) { - return textResponse('These changes don’t match the proposal they were opened with. Start the proposal again.', 409, cors); - } + + const plan = planPullUpload(pull, head); + if (plan.error) return textResponse(plan.error, plan.status, cors); + const { updating } = plan; const declared = Number(request.headers.get('Content-Length') || 0); if (declared > MAX_PACK_BYTES) return textResponse('This proposal has too much history to store.', 413, cors); @@ -440,15 +529,33 @@ async function putPullPack(request, env, base, lessonId, pullId, cors) { // The pack is stored before the row is flipped: `ready` means "there is // something to review", so it must never be true ahead of the bytes. // - // Conditional on the request still being open and still unready. The status - // was checked above, but the row can be closed between that read and this - // write — and a close deletes the pack, so an unconditional flip would leave - // a resolved proposal advertising bytes that were just swept, or bytes we - // wrote back after the sweep. When the transition doesn't take, the pack we - // just stored is ours to clean up. - const updated = await patchPull(env, base, pullId, { ready: true }, { status: 'open', ready: false }); + // Conditional on the request still being open, and on it still pointing where + // we read it pointing. The status was checked above, but the row can be closed + // between that read and this write — and a close deletes the pack, so an + // unconditional write would leave a resolved proposal advertising bytes that + // were just swept, or bytes we wrote back after the sweep. The head condition + // does the same job for two of the proposer's own uploads racing. When the + // transition doesn't take, the pack we just stored is ours to clean up. + const updated = await patchPull(env, base, pullId, plan.patch, plan.expect); if (!updated) { - await deletePullGit(env, pullId); + // The write didn't take, and which pack we just orphaned depends on why. + // + // A first upload always owns its bytes: the proposal never became ready, so + // nothing else can be pointing at them. + // + // An update has two possible losers, and only re-reading the row tells them + // apart. Against a proposal that has since been *resolved*, closePull already + // deleted the pack and we have just written one back after the sweep — that + // one is ours to remove, or it stays in the bucket for ever. Against one still + // open, we lost a race with the proposer's own other upload: the pack there is + // theirs, it is a superset, and getPullPack serves the row's head regardless, + // so leaving it is both harmless and the only safe choice. + if (!updating) { + await deletePullGit(env, pullId); + } else { + const now = await fetchPull(env, base, lessonId, pullId); + if (!now || now.status !== 'open') await deletePullGit(env, pullId); + } return textResponse('This proposal is no longer open.', 409, cors); } @@ -464,10 +571,17 @@ async function putPullPack(request, env, base, lessonId, pullId, cors) { const lesson = await fetchLessonRow(env, base, lessonId); if (lesson) { const own = lesson.author_id === user.id; + const who = authorFromUser(user); await createNotification(env, base, { userId: lesson.author_id, type: 'pull_request', - title: own ? 'Changes are waiting for your review' : `${authorFromUser(user)} proposed changes to your lesson`, + title: updating + ? own + ? 'Changes waiting for your review were updated' + : `${who} updated the changes they proposed` + : own + ? 'Changes are waiting for your review' + : `${who} proposed changes to your lesson`, body: pull.title, link: `/hub/${lessonId}/proposals/${pullId}`, }).catch(() => {}); @@ -504,7 +618,14 @@ async function getPullPack(request, env, base, lessonId, pullId, cors) { // A proposal's pack never changes once uploaded, but it is deleted when the // proposal is resolved — so it may be cached briefly, never indefinitely. headers.set('Cache-Control', 'private, max-age=60'); - const head = object.customMetadata?.head || pull.head || ''; + // The *row* names the tip, in preference to the object's own metadata — the + // reverse of how a lesson's pack works, and for a reason particular to + // proposals. An update writes the pack before it moves the row, so between + // those two the stored bytes are ahead of the record. The bytes are a superset + // either way (a proposal only ever moves forward), so serving the row's head + // with them hands a reviewer exactly the revision the proposal claims to be, + // whichever way that write went. + const head = pull.head || object.customMetadata?.head || ''; if (head) headers.set('X-Git-Head', head); // The SPA reads X-Git-Head cross-origin, which needs it explicitly exposed. headers.set('Access-Control-Expose-Headers', 'X-Git-Head'); diff --git a/apps/api/src/routes/pulls.test.js b/apps/api/src/routes/pulls.test.js new file mode 100644 index 0000000..488611b --- /dev/null +++ b/apps/api/src/routes/pulls.test.js @@ -0,0 +1,66 @@ +// What an upload to a proposal is allowed to do. +// +// A proposal used to be write-once, which made this decision trivial and made +// changing a proposal impossible. Now it has three outcomes — complete the open, +// record a new revision, or refuse — and getting the refusals wrong is how a +// reviewer ends up reading one thing and merging another. Pure, so tested here +// rather than through R2. + +import { describe, expect, it } from 'vitest'; +import { planPullUpload } from './pulls.js'; +import { MAX_PULL_REVISIONS } from '@spelling-creator/core/pulls'; + +const A = 'a'.repeat(40); +const B = 'b'.repeat(40); + +const opened = (over = {}) => ({ head: A, ready: false, revision: 1, ...over }); +const live = (over = {}) => ({ head: A, ready: true, revision: 1, ...over }); + +describe('planPullUpload', () => { + it('completes the two-step open when the tip is the one the row was created with', () => { + const plan = planPullUpload(opened(), A); + expect(plan.updating).toBe(false); + expect(plan.patch).toEqual({ ready: true }); + // The row must still be open, still unready, and still where we read it. + expect(plan.expect).toEqual({ status: 'open', ready: false, head: A }); + }); + + it('refuses a first upload that does not match the proposal it belongs to', () => { + expect(planPullUpload(opened(), B).status).toBe(409); + }); + + it('records a revision when an open proposal is given something new', () => { + const plan = planPullUpload(live(), B, 'NOW'); + expect(plan.updating).toBe(true); + expect(plan.patch).toEqual({ + head: B, + previous_head: A, + revision: 2, + updated_at: 'NOW', + }); + // Conditional on the head we read: two of the proposer's own uploads racing + // must not both win. + expect(plan.expect).toEqual({ status: 'open', ready: true, head: A }); + }); + + it('keeps the commit it moved from, which is what makes the update readable', () => { + expect(planPullUpload(live({ head: A, revision: 3 }), B).patch).toMatchObject({ + previous_head: A, + revision: 4, + }); + }); + + it('refuses an update that changes nothing', () => { + expect(planPullUpload(live(), A).status).toBe(409); + }); + + it('stops once a proposal has been rewritten enough times', () => { + expect(planPullUpload(live({ revision: MAX_PULL_REVISIONS - 1 }), B).updating).toBe(true); + expect(planPullUpload(live({ revision: MAX_PULL_REVISIONS }), B).status).toBe(409); + }); + + it('treats a row with no revision recorded as the first one', () => { + const plan = planPullUpload({ head: A, ready: true }, B); + expect(plan.patch.revision).toBe(2); + }); +}); diff --git a/apps/docs/docs/.vitepress/config.mts b/apps/docs/docs/.vitepress/config.mts index 2812b2c..690b28f 100644 --- a/apps/docs/docs/.vitepress/config.mts +++ b/apps/docs/docs/.vitepress/config.mts @@ -109,6 +109,10 @@ export default defineConfig({ { text: "Search images", link: "/web-app/search-images" }, { text: "Save to Google Docs", link: "/web-app/save-to-google-docs" }, { text: "Live collaboration", link: "/web-app/live-collaboration" }, + { + text: "Variations (trying something out)", + link: "/web-app/lesson-variations", + }, { text: "Pull requests (proposing changes)", link: "/web-app/pull-requests", diff --git a/apps/docs/docs/mcp-server/tools.md b/apps/docs/docs/mcp-server/tools.md index 78c94da..675ae82 100644 --- a/apps/docs/docs/mcp-server/tools.md +++ b/apps/docs/docs/mcp-server/tools.md @@ -55,8 +55,13 @@ Some mechanics worth knowing: happened. - **A proposal is one commit**, made when it's opened, holding the fork as it then stands. Intermediate `patch_lesson` calls aren't separate commits, so make all the - edits first. Proposing again after further edits opens a second, separate proposal - (at most 5 open against one lesson). + edits first. +- **Proposing again updates the proposal already open** from that fork, rather than + stacking a second one beside it — same request, same discussion, new contents, with + the version number recorded. That's what you want after the human asks for a change; + the `title` and `body` passed are then ignored, since the ones already there are what + they have been reading. `updated` in the result says which happened. (At most 5 open + against one lesson, and at most 20 updates to one proposal.) - **Images aren't copied.** Blocks reference them by content hash and the bytes are already stored, so forking is cheap. - **Forks are private drafts** and count against the draft cap, so `delete_lesson` the diff --git a/apps/docs/docs/monorepo/version-history.md b/apps/docs/docs/monorepo/version-history.md index 974ccaa..d107ac3 100644 --- a/apps/docs/docs/monorepo/version-history.md +++ b/apps/docs/docs/monorepo/version-history.md @@ -88,13 +88,58 @@ tree (unchanged blocks resolve to oids git already has) and compare its oid with The editor shows this as a chip — _"Version saved 2 minutes ago"_, or _"3 unsaved changes"_ — which opens the history. -## Restoring +## Restoring, and undoing Restoring an old version is an ordinary **forward** commit whose tree happens to equal an older one. History is never rewritten: the version you restored _away from_ stays in the timeline, so the restore itself can be undone by restoring again. +Restoring is the blunt instrument, though — it takes the whole document back and +drops everything since. **Undo** is the precise one: put back what _that one +version_ changed, and keep the rest. It is the same three-way merge as everything +else here, with the sides pointed backwards: + +| Merge argument | Undoing commit C | +| -------------- | --------------------------------- | +| base | the document as C left it | +| ours | the document now | +| theirs | the document immediately before C | + +Every rule then falls out without a line of new logic, including the field-level +one: a block C changed differs between base and theirs, so theirs wins and it goes +back; a block changed since differs between base and ours, so ours wins and is +kept; a block in both _but in different fields_ merges field by field, with both +surviving. Only a block where the same field was changed on both sides is a genuine +conflict — the change being undone has been built on in the very place it touched, +and only the author can say what they meant — so that is what reaches the dialog. The result is a forward +commit with one parent, so an undo can itself be undone. + +The history view asks two questions about a selected version, because they have +different answers the moment anything has happened since: _what changed here_ +(history) and _difference from now_ (the decision you are about to make). + +## More than one branch + +A lesson's repository holds a branch per **variation** — an alternative version of +the lesson its author is trying out, kept apart from the one people are reading. +The default branch (`main`) is the lesson; the rest are drafts of what it might +become. + +Which one is being edited is `HEAD`, a symbolic ref, exactly as in git. That is +not just tidiness: `HEAD` lives inside the gitdir, so it survives the two places a +repository is copied wholesale — publishing a draft (`adoptDraftRepo`) and forking +a lesson locally (`copyRepo`) — neither of which knows branches exist. + +Everything on this page is per branch as a result. A commit moves whatever `HEAD` +points at; the history view reads the branch being edited unless given a ref; the +pack carries every branch, and the push compare-and-swaps each one separately. +What doesn't change is what "the lesson" means: `main`, and only `main`, is what a +reader sees, what a fork clones, and what a proposal is offered against. + +See [Variations](/web-app/lesson-variations) for what an author sees, why a +variation is as public as its lesson, and how a deletion travels. + ## Forking is cloning For someone else to fork a lesson, its repository has to travel. It travels the @@ -119,8 +164,9 @@ so a pack is pure JSON and stays small — a few KB for a typical lesson. ### Worker endpoints ``` -GET /git/:lessonId/refs public* -> { head, size, updatedAt } (404 = no history) -GET /git/:lessonId/pack public* -> the packfile (X-Git-Head names its tip) +GET /git/:lessonId/refs public* -> { head, refs, updatedAt } (404 = no history) +GET /git/:lessonId/pack public* -> the packfile (X-Git-Head names its tip, + X-Git-Refs every branch it holds) PUT /git/:lessonId/pack Bearer -> store it (the author, or a trusted collaborator) ``` @@ -129,13 +175,14 @@ open [pull request](/web-app/pull-requests), which is a packfile too: ``` git//pack the packfile bytes -git//refs.json { head, size, updatedAt } +git//refs.json { head, refs, size, updatedAt } git/pulls//pack a proposal's snapshot (no refs.json: its tip is fixed) ``` -The pack carries its own tip in R2 `customMetadata`, echoed in the `X-Git-Head` -response header — so a clone reads the bytes and the ref they belong to from the -_same object_, and can never pair a fresh ref with a stale pack. +The pack carries its own tip **and its branch map** in R2 `customMetadata`, echoed +in the `X-Git-Head` and `X-Git-Refs` response headers — so a clone reads the bytes +and the refs they belong to from the _same object_, and can never pair a fresh ref +with a stale pack. `GET` is public because forking a published lesson is public; a private draft's history (like the draft itself) 404s to everyone but its author, a trusted @@ -183,6 +230,13 @@ one to adjudicate, so a reorder on both sides resolves to ours. The result is committed with **two parents**, which genuinely joins the two histories — so the next merge can find _this_ commit as its base. +Unless it needn't be. When our side _is_ the merge base — their history already +contains ours and we have added nothing to it, neither commits nor uncommitted +edits — the merge is a **fast-forward**: our branch moves to their commit and no +merge commit is written. There is nothing for one to record, and manufacturing it +would put an entry in the lesson's timeline saying a decision was made when none +was. + ## Merging a fork back in (pull requests) Anyone can fork a lesson and pull the original's later changes in. Going the other @@ -290,6 +344,7 @@ in the browser, in Node and inside the Worker: | Module | Purpose | | -------- | ------------------------------------------------------------------ | | `doc` | Pure doc helpers: canonical JSON, manifest, block map. No git. | +| `refs` | Branch names, limits, and the ref map's wire format. No git. | | `ops` | Diff two docs into operations; render commit messages. No git. | | `merge` | Three-way merge by block id, field-level. No git. | | `layout` | Document ⇄ git tree (one file per block). | @@ -318,18 +373,19 @@ thing to it and uploads the result. See App-bound (`apps/web/src/lib/git/`) — what cannot leave the bundle: -| File | Purpose | -| ----------------------- | ---------------------------------------------------------- | -| `engine.js` + `load.js` | The git engine, behind one dynamic import. | -| `useLessonGit.js` | The editor's controller: setup, periodic commits, history. | +| File | Purpose | +| ----------------------- | ---------------------------------------------------------------------- | +| `engine.js` + `load.js` | The git engine, behind one dynamic import. | +| `useLessonGit.js` | The editor's controller: setup, periodic commits, history, variations. | `repo` and friends take their filesystem through `repoCtx` rather than opening one, which is exactly what lets the same commit/merge/restore logic run against LightningFS in the browser, and `memfs` in Node, in the Worker and in tests. A repo tracks remotes in git's own vocabulary: `origin` (this lesson's own -published history, which a trusted collaborator may have moved on without us), -`upstream` (the lesson it was forked from), and, while one is being reviewed, +published history, which a trusted collaborator may have moved on without us — +one `refs/remotes/origin/` per branch the hub holds), `upstream` (the +lesson it was forked from), and, while one is being reviewed, `refs/remotes/pull/` — one ref per proposal, so two open ones can't overwrite each other's tip. diff --git a/apps/docs/docs/web-app/lesson-variations.md b/apps/docs/docs/web-app/lesson-variations.md new file mode 100644 index 0000000..5b1125f --- /dev/null +++ b/apps/docs/docs/web-app/lesson-variations.md @@ -0,0 +1,204 @@ +--- +title: Variations (trying something without breaking the lesson) +--- + +# Variations (trying something without breaking the lesson) + +A **variation** is a separate copy of a lesson you can change freely. The lesson +everybody else reads doesn't move while you work on one, and nothing you do to a +variation reaches it until you say so. + +It exists for the thing authors were doing the hard way: rewriting half a lesson +to see whether the rewrite is better. Before this, the only way to do that +without risking the original was to fork it into a whole second lesson and open a +proposal against yourself. A variation is the same idea at the right size. + +## What it looks like + +The editor shows which copy you're on, next to the "Version saved" chip: + +```text + Version saved 2 minutes ago Main lesson ▾ + Version saved just now Simpler for Year 3 ▾ <- on a variation +``` + +Clicking it opens the list. From there you can start one, switch between them, +rename one, delete one, and — the point of the whole thing — **bring one into the +main lesson**. + +Each variation says how much work is sitting on it ("3 changes that aren't in the +main lesson"), which is the only number an author needs before deciding whether +to open it. + +### Bringing one in + +This runs the same block-by-block merge as everything else in +[Version history](/monorepo/version-history): the main lesson and the variation +are lined up against the commit they last agreed on, and only a block both sides +changed _in the same field_ reaches a dialog. Usually nothing does, and the merge +summary just says what it settled. + +The order matters and is fixed: the editor **switches to the main lesson first**, +then merges the variation into it. That is what makes the result the lesson with +your changes folded in, rather than the variation with the lesson folded in — +which is the same commit and the opposite meaning. Anything you had unsaved is +committed to the variation on the way out, so nothing in flight is carried across +by accident. + +Afterwards the variation is still there, now reading "0 changes that aren't in the +main lesson". Keep it and carry on, or delete it. + +### Trying somebody else's proposal in one + +A variation is also where a reviewer can put a [proposal](./pull-requests.md) +they aren't sure about — "Try it in a variation" on its page — and read the whole +lesson with the change in it before deciding. The lesson doesn't move and the +proposal stays open. + +## Variations are as public as the lesson + +They travel with the lesson, so a variation you start on your laptop is there when +you open the lesson on your phone. That is the point — but it means a variation +lives in the same packfile as the lesson, and **a published lesson's packfile is +public**, because that is what makes forking work. + +So: anyone who can open the lesson can read its variations. On a private draft +that's you (and anyone you trust); on a published lesson that's everyone. The +dialog says so, in those words, where an author will see it. + +If you want to try something genuinely privately, fork the lesson into a new +private draft instead — see [Pull requests](./pull-requests.md). + +## What it is underneath + +A variation is a **branch** of the lesson's git repository, and switching between +them is a checkout. None of that vocabulary appears in the app, deliberately: an +author isn't doing version control, they're trying something and keeping the +original safe while they do. + +The mapping is exact, though, and everything on this page falls out of it. + +| In the app | In the repository | +| --------------------------- | --------------------------------------------------- | +| The main lesson | `refs/heads/main` — the default branch | +| A variation | `refs/heads/` | +| Which one you're editing | `HEAD`, a symbolic ref | +| Switching | Writing `HEAD`, and adopting the doc at the new tip | +| Bringing one in | A merge commit on `main` — or a fast-forward\* | +| "3 changes that aren't in…" | Commits on the branch not reachable from `main` | + +\* When `main` is the merge base and the editor has nothing uncommitted, the +branch simply moves: there is nothing for a merge commit to record. Otherwise it +is a two-parent commit, and only blocks changed on both sides _in the same field_ +reach a dialog — a caption edited here and a width edited there merge field by +field, with both kept. + +Recording the current variation in `HEAD` rather than beside the repository is +what makes it survive a reload, a second tab, and the two places a repository gets +copied wholesale — publishing a draft (`adoptDraftRepo`) and forking a lesson +locally (`copyRepo`), neither of which knows branches exist. + +### Names + +Git bounds what a branch may be called, so what an author types is converted: +spaces become hyphens, anything git reserves is dropped, and the result is capped +at 32 characters. It is read back the other way for display, so "Simpler for Year +3" round-trips. The rules live in `@spelling-creator/core/git/refs` and are +imported by both the editor and the Worker, so what the app offers and what the +server accepts can't drift apart. + +A lesson may have at most **12 branches**. That ceiling isn't taste: the branch +map rides in the R2 object's `customMetadata` alongside the pack it belongs to +(so a reader can never pair one moment's bytes with another moment's refs), and R2 +caps that metadata at 2 KB. + +## How they travel + +The lesson's stored `refs.json` gained a map, and kept `head` meaning exactly what +it always did — the default branch, which is what a reader, a forker and the +lesson's own page ask for: + +```json +{ + "head": "", + "refs": { + "main": "", + "Simpler-for-Year-3": "" + }, + "size": 41203, + "updatedAt": "..." +} +``` + +The pack holds every object reachable from _any_ branch. That costs almost +nothing: branches of one lesson share nearly all of their objects, and the packer +dedupes by oid, so a second variation adds only the commits unique to it. + +A **fork** takes the default branch alone. Somebody else's half-finished ideas +aren't part of what was forked, and adopting them as branches of the fork would +claim they were. + +A **proposal** carries exactly one branch: the one you were working on when you +proposed it. Both halves matter. Offering the branch you are looking at is the +only reading of "propose these changes" that isn't a trap — work an idea up on a +variation, propose, and you would otherwise have sent the untouched lesson and +been told it worked. Offering _only_ that one is what keeps the rest to yourself. +See [Pull requests](./pull-requests.md). + +### Pushing more than one branch + +The compare-and-swap that has always guarded a push now runs per branch. Three +headers describe what a push wants, and the Worker applies all of it or none: + +```text +X-Git-Refs the branches to set, { "": "" } +X-Git-Expected what the client believes the hub holds for each name it touches, + with "" meaning "I believe this one does not exist yet" +X-Git-Deletes the branches to remove, comma-separated +``` + +Atomicity is free: `refs.json` is a single R2 object and already the commit point, +so every branch advances or none does. + +The rule that makes this safe with two devices is that **a branch a push doesn't +mention is left exactly as it is**. Otherwise a device that had never heard of a +variation would delete it simply by not knowing about it. A client sending neither +`X-Git-Refs` nor `X-Git-Expected` — one written before any of this — therefore +still means "move the lesson, leave everything else alone", and keeps working. + +### Deleting has to be asked for + +Which leaves a gap: if a push only ever _adds_, how does a deletion travel? It +can't be inferred, for the reason above — "I don't have it" and "I deleted it" +look identical from a ref map. + +So a delete leaves a marker in the repository (`refs/deleted/`, holding the +tip it pointed at), the next push turns that into an explicit `X-Git-Deletes` +instruction compare-and-swapped against that tip, and only a push that actually +landed clears the marker. Cleared any earlier and the variation would be gone +locally, alive on the hub, and back on the next device that opened the lesson. + +The same marker is why fetching doesn't undo a delete: a branch on the hub that we +hold a marker for is not adopted back. And reusing the name clears it — a name +used again is not the deleted variation returning, and a marker left behind would +make the next push ask to create and remove one name in a single request. The +Worker refuses that request rather than picking a half. + +Fetching prunes in the other direction too. A branch the hub no longer has, which +we still hold at exactly the tip it last told us about, holds nothing that isn't +already gone, so it goes — otherwise a deletion made on one device would be +undone by another that still had the branch. One that has _moved_ holds unpushed +work, and that is the author's to keep: it goes back up, and they can delete it +again. + +## Where it lives + +| Piece | What it does | +| ---------------------------------------------- | ------------------------------------------------------------------ | +| `@spelling-creator/core/git/refs` | Name rules, limits, and the ref map's wire format. No git. | +| `@spelling-creator/core/git/repo` | `currentBranch`, create / checkout / rename / delete, `aheadCount` | +| `@spelling-creator/core/git/pack` | Packing every branch; a clone writing them back | +| `@spelling-creator/core/browser/git/sync` | Per-branch push, adopting the hub's branches, `prepareBranchMerge` | +| `apps/api/src/routes/git.js` | The per-branch compare-and-swap (`applyRefs`) | +| `apps/web/src/lib/git/useLessonGit.js` | The editor's variation state and actions | +| `apps/web/src/components/VariationsDialog.jsx` | The list, and everything you can do from it | diff --git a/apps/docs/docs/web-app/pull-requests.md b/apps/docs/docs/web-app/pull-requests.md index 9e2ee4c..13cb404 100644 --- a/apps/docs/docs/web-app/pull-requests.md +++ b/apps/docs/docs/web-app/pull-requests.md @@ -31,8 +31,8 @@ lesson ───────▶ your copy ──────────── uploads it with a title and an optional note. Nothing in the original changes. 4. The lesson's author sees it on the lesson's **Proposals** tab (`/hub/:id/proposals`), and gets a notification. Each proposal also has a page - of its own at `/hub/:id/proposals/:prId` — read-only, because merging needs - the git objects and those live in the editor. They (or a trusted collaborator) + of its own at `/hub/:id/proposals/:prId`, which shows **what it changes** and + whether it would merge cleanly (see below). They (or a trusted collaborator) hit **Review & merge**. 5. That opens the lesson in _their_ editor (`?pull=&lesson=` — the link names both, so the review waits for the lesson it belongs to rather than @@ -50,15 +50,50 @@ visible); only its stored changes are dropped. ## A proposal is a snapshot -What a pull request actually contains is a **git packfile** — your whole -repository as it stood the moment you opened the request — stored in R2 under +What a pull request actually contains is a **git packfile** — your fork's lesson +as it stood the moment you opened the request — stored in R2 under `git/pulls//pack`, beside the lessons' own packs. +The lesson, and not your [variations](./lesson-variations.md) of it: a variation is +an idea you are still turning over, and offering one to somebody else to merge, +unasked and unmentioned, is not what "propose changes" means. + +Which version of your fork? **The one you were working on.** If you developed the +idea on a [variation](./lesson-variations.md), that is what gets proposed — and +only that one, so the rest of your variations stay yours. The proposal records +which branch it came from, and the review queue says so. + Snapshotting is deliberate. You carry on editing your fork after proposing, and a request that silently tracked your branch would mean the reviewer reading one -thing and merging another. So the pack is written once and never rewritten: the -Worker refuses a second upload, and refuses any upload whose tip isn't the commit -the request was opened with. +thing and merging another. + +## Updating a proposal + +The pack used to be written once and never rewritten, which made "silently" moot +by making _any_ change impossible — so being asked for a tweak meant closing the +proposal and opening another, throwing away the discussion attached to it. + +Proposing again from the same fork now **updates** the proposal you already have +open, and records that it happened: the version number goes up, the commit it used +to point at is kept, and the page shows _"Version 3 · updated 4 March"_ along with +what that last update changed. Nothing moves silently; what was actually being +protected was never immutability, it was that nothing changes without saying so. + +A proposal may only move **forward** — the new tip has to contain the one the +proposal already points at. That is what keeps one pack per proposal honest: the +previous version's commit is still reachable in the new pack, which is how "what +the last update changed" is answerable without storing a pack per version. It is +checked by the client, because the Worker holds a proposal's history as an opaque +packfile it cannot walk — the same limit that stops the merge endpoint verifying +ancestry, bounded the same way, since the only proposal you can rewrite is your +own and you could always have closed and reopened it. + +There is a ceiling of **20 updates**. Past a couple of dozen rewrites it is a +different change, and the thread attached to it has stopped being about what the +proposal now contains. + +An assistant working over MCP follows the same rule: `propose_changes` from a fork +that already has one open updates it rather than stacking a second one beside it. Because a fork is a real clone, that pack shares object ids with the lesson's own history. The reviewer indexes it into the lesson's repository, where its objects @@ -73,6 +108,71 @@ reviewer shouldn't have to tell a half-finished submission from a real one. If t upload fails, the client withdraws the empty request rather than leaving it in the author's queue. +## Reading one without merging it + +A proposal's own page shows its changes — block by block, in the same summary the +history view renders — and whether merging it would ask the reviewer for anything: + +- _"This merges cleanly — there's nothing to decide."_ +- _"2 blocks have been changed here and in the lesson, so merging will ask you + which to keep."_ +- _"These changes are already part of the lesson."_ + +That is computed on the page, from the git objects themselves. Both packs are +public exactly as far as the lesson is, so the browser indexes the proposal's pack +beside the lesson's, finds the commit the two diverged at, and diffs against +**that** — not against the lesson's current tip, which would show the author's own +later edits as though the proposer had made them, reversed. + +Merging still happens in the editor, and that split is deliberate rather than a +limitation: merging commits to the lesson's history and pushes it under the +reviewer's credentials, which needs the editor's repository. Reading needs none of +it. What changed is that nobody has to start a merge to find out whether they want +one. + +The git engine is ~200 KB and is fetched on demand when this page opens, so the +proposal's title, author and note render first and the diff arrives after. If it +can't be read at all, the page says so and the rest of it still works. + +## Trying it before deciding + +Reading a diff tells you what changed. It doesn't tell you whether the lesson +still works with the change in it — whether the new question fits where it was +put, whether the rewritten section still reads in order. + +So there is a third answer between merging and declining: **try it in a +variation**. That lands the proposal on a [variation](./lesson-variations.md) of +the reviewer's own, where they can click through the whole lesson with the change +in place. The lesson everyone reads is untouched, and the proposal stays open — +nothing about it is recorded, because nothing has been decided. + +Mechanically it is the two features meeting and needing almost nothing new: a +merge commits to whatever branch is checked out, so the editor starts a variation +named after the proposal and checks it out _before_ preparing the merge. What it +must not do is record the proposal as being reviewed, since that is what makes the +confirm push and mark it merged — a try-out deliberately does neither. + +Trying the same proposal twice returns to the variation the first attempt made, +rather than refusing on the name, so whatever the reviewer did to it last time is +still there. + +## Landing it without a merge commit + +When the lesson hasn't moved since the proposal was opened, the merge is a +**fast-forward**: the lesson's branch simply moves to the proposal's commit. No +merge commit is written, because there is nothing for one to record — no decision +was made and no content changed that the proposal's own commits don't already +describe. + +Two conditions, and both are needed. The lesson's tip must be the merge base (the +commit-graph half), and the reviewer must have nothing uncommitted in their editor +(the half the graph can't see — skipping it would drop whatever they had typed but +not yet paused long enough to commit). + +The proposal is still recorded as merged against the commit the lesson now points +at, which is the proposal's own head. If anything that is easier to verify than +before. + ## Who can do what | You are | Open | Merge | Close | @@ -184,14 +284,14 @@ which is why the submission dialog says so plainly before you send it. ## Worker endpoints -| Method & path | Auth | What it does | -| ------------------------------------- | ----------------------- | ---------------------------------------------------------------------------------------------- | -| `GET /lessons/:id/pulls` | none, unless a draft\* | `{ "pulls": [...], "canReview": bool }` — newest first; unready rows only for their own author | -| `POST /lessons/:id/pulls` | `Bearer ` | Opens a proposal (`{ title, body, head, base, sourceLessonId }`); the author only from a fork | -| `PUT /lessons/:id/pulls/:prId/pack` | `Bearer ` | Uploads its packfile (`X-Git-Head` must match). The proposer's, once | -| `GET /lessons/:id/pulls/:prId/pack` | none, unless a draft\* | The packfile; `X-Git-Head` names its tip | -| `POST /lessons/:id/pulls/:prId/merge` | `Bearer ` | Records the merge (`{ mergeCommit }`); author or trusted collaborator only | -| `POST /lessons/:id/pulls/:prId/close` | `Bearer ` | Closes it; proposer, author, trusted collaborator, or moderator | +| Method & path | Auth | What it does | +| ------------------------------------- | ----------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------- | +| `GET /lessons/:id/pulls` | none, unless a draft\* | `{ "pulls": [...], "canReview": bool }` — newest first; unready rows only for their own author | +| `POST /lessons/:id/pulls` | `Bearer ` | Opens a proposal (`{ title, body, head, headRef, base, sourceLessonId }`); anyone signed in — the lesson's own author only from a fork they own | +| `PUT /lessons/:id/pulls/:prId/pack` | `Bearer ` | Uploads its packfile — the proposer's only. The first must match the head the row was opened with; a later one records a revision | +| `GET /lessons/:id/pulls/:prId/pack` | none, unless a draft\* | The packfile; `X-Git-Head` names its tip | +| `POST /lessons/:id/pulls/:prId/merge` | `Bearer ` | Records the merge (`{ mergeCommit }`); author or trusted collaborator only | +| `POST /lessons/:id/pulls/:prId/close` | `Bearer ` | Closes it; proposer, author, trusted collaborator, or moderator | \* Reads follow the target lesson's own visibility — the single `canReadLesson` rule in `apps/api/src/lib/lesson.js` that also gates `GET /lessons/:id`, its @@ -202,20 +302,21 @@ frontend can surface `res.text()` directly. ## Where it lives -| Piece | What it does | -| -------------------------------------------------- | ------------------------------------------------------------------- | -| `apps/api/src/routes/pulls.js` | The endpoints above, and every permission rule | -| `apps/api/schema.sql` | `lesson_pull_requests` | -| `apps/api/src/lib/lessonGit.js` | R2 key layout (`git/pulls//pack`) and the sweeps that delete it | -| `@spelling-creator/core/pulls` | The browser client, and the shared length limits | -| `@spelling-creator/core/browser/git/sync` | `submitPullRequest` (propose) and `preparePullMerge` (review) | -| `apps/mcp/src/git.js` | The same two steps for an AI assistant — fork, then propose | -| `apps/web/src/components/ProposeChangesDialog.jsx` | The submission form | -| `apps/web/src/pages/lesson/LessonProposals.jsx` | The Proposals tab | -| `apps/web/src/pages/lesson/LessonProposal.jsx` | One proposal, read-only, with the hand-off into the editor | -| `apps/web/src/components/PullRequestsSection.jsx` | The list on a lesson's page | -| `apps/web/src/pages/EditorPage.jsx` | `?pull=&lesson=` — the review + merge flow | -| `apps/web/src/components/MergeDialog.jsx` | Settling conflicts, shared with the fork-sync direction | +| Piece | What it does | +| -------------------------------------------------- | ----------------------------------------------------------------------------------------- | +| `apps/api/src/routes/pulls.js` | The endpoints above, and every permission rule | +| `apps/api/schema.sql` | `lesson_pull_requests` | +| `apps/api/src/lib/lessonGit.js` | R2 key layout (`git/pulls//pack`) and the sweeps that delete it | +| `@spelling-creator/core/pulls` | The browser client, and the shared length limits | +| `@spelling-creator/core/browser/git/sync` | `submitPullRequest` (propose), `prepareProposalReview` (read), `preparePullMerge` (merge) | +| `apps/mcp/src/git.js` | The same two steps for an AI assistant — fork, then propose | +| `apps/web/src/components/ProposeChangesDialog.jsx` | The submission form | +| `apps/web/src/pages/lesson/LessonProposals.jsx` | The Proposals tab | +| `apps/web/src/pages/lesson/LessonProposal.jsx` | One proposal: its changes, its mergeability, and the hand-off into the editor | +| `apps/web/src/components/ChangeSummary.jsx` | The change chips and operation list, shared with the history view | +| `apps/web/src/components/PullRequestsSection.jsx` | The list on a lesson's page | +| `apps/web/src/pages/EditorPage.jsx` | `?pull=&lesson=` — the review + merge flow | +| `apps/web/src/components/MergeDialog.jsx` | Settling conflicts, shared with the fork-sync direction | A pack is swept when its proposal is **closed** — nothing there will ever be merged — and when the lesson is deleted, before the row goes, since the cascade diff --git a/apps/mcp/manifest.json b/apps/mcp/manifest.json index f806079..e933eb7 100644 --- a/apps/mcp/manifest.json +++ b/apps/mcp/manifest.json @@ -2,7 +2,7 @@ "manifest_version": "0.3", "name": "spelling-creator-hub", "display_name": "Spelling Creator Hub", - "version": "0.3.0", + "version": "0.5.0", "description": "Author and publish spelling lessons to the Spelling Creator hub from your AI assistant.", "long_description": "Connects your AI assistant to the Spelling Creator hub so it can compose and publish spelling lessons for you. You describe the lesson; the assistant builds it (reading passages with ALL-CAPS spelling words, explicit word lists, and quiz questions) and saves it to the hub through the same API the web app uses — with all the usual validation and author attribution. Lessons default to private drafts; publish them when you're ready. Requires a Supabase refresh token (run the bundled `login` helper, or copy one from the web app) and a display name set on your account.", "author": { diff --git a/apps/mcp/package.json b/apps/mcp/package.json index 2011206..272fb4a 100644 --- a/apps/mcp/package.json +++ b/apps/mcp/package.json @@ -1,6 +1,6 @@ { "name": "@spelling-creator/mcp", - "version": "0.3.0", + "version": "0.5.0", "private": true, "description": "MCP server for the Spelling Creator hub — lets any MCP-capable AI assistant author and publish spelling lessons.", "license": "AGPL-3.0-only", diff --git a/apps/mcp/src/api.js b/apps/mcp/src/api.js index 0d0f2d5..52f8b72 100644 --- a/apps/mcp/src/api.js +++ b/apps/mcp/src/api.js @@ -7,6 +7,11 @@ // token. On a 401 we transparently refresh the token once and retry, so a // long-lived server survives the access token expiring between calls. +import { + DEFAULT_BRANCH, + parseRefMap, + serializeRefMap, +} from "@spelling-creator/core/git/refs"; import { sha256Hex, extFromMime } from "./images.js"; /** @@ -88,7 +93,11 @@ export function createApi(config, auth) { * tip travels in X-Git-Head so the bytes and the ref they belong to can never * be paired from two different moments (see core/git/remote.js). */ - async function putPack(url, { packfile, head, parent }, badStatusMessage) { + async function putPack( + url, + { packfile, head, parent, refs, expected }, + badStatusMessage, + ) { const headers = { "Content-Type": "application/x-git-packfile", "X-Git-Head": head, @@ -96,6 +105,12 @@ export function createApi(config, auth) { // The compare-and-swap: the head we believe the lesson points at. Omitted // when it has no history yet. if (parent) headers["X-Git-Parent"] = parent; + // And the same claim for every branch the pack carries. A lesson can hold a + // branch per variation its author is trying out, and packRepo sends all of + // them — so the push has to name them all, or the hub would be left + // advertising tips whose objects the stored pack no longer contains. + if (refs) headers["X-Git-Refs"] = serializeRefMap(refs); + if (expected) headers["X-Git-Expected"] = serializeRefMap(expected); const res = await request(url, { method: "PUT", headers, body: packfile }); if (!res.ok) throw await readError(res, badStatusMessage); @@ -116,7 +131,13 @@ export function createApi(config, auth) { if (!head) return null; // a pack with no tip is unusable const packfile = new Uint8Array(await res.arrayBuffer()); if (packfile.byteLength === 0) return null; - return { packfile, head }; + // The branch map comes off the same response as the bytes, so the two can + // never be paired from different moments. A lesson stored before variations + // existed sends none, which reads as the one branch it has. + const refs = parseRefMap(res.headers.get("X-Git-Refs")) || { + [DEFAULT_BRANCH]: head, + }; + return { packfile, head, refs }; } return { @@ -249,10 +270,10 @@ export function createApi(config, auth) { * the Worker refuses the push if the lesson has moved on since, which is * what stops two writers erasing each other. */ - async pushLessonPack(lessonId, { packfile, head, parent }) { + async pushLessonPack(lessonId, { packfile, head, parent, refs, expected }) { return putPack( gitUrl(lessonId, "/pack"), - { packfile, head, parent }, + { packfile, head, parent, refs, expected }, "Could not save the lesson history.", ); }, diff --git a/apps/mcp/src/git.js b/apps/mcp/src/git.js index bf296f9..13a240d 100644 --- a/apps/mcp/src/git.js +++ b/apps/mcp/src/git.js @@ -35,9 +35,11 @@ import { memRepo } from "@spelling-creator/core/git/memfs"; import { describeOp } from "@spelling-creator/core/git/ops"; import { cloneFromPack, + contains, fetchRemotePack, packRepo, } from "@spelling-creator/core/git/pack"; +import { DEFAULT_BRANCH } from "@spelling-creator/core/git/refs"; import { UPSTREAM_REF, authorFrom, @@ -99,10 +101,19 @@ export function proposalBody(body, client) { * Build an in-memory repository holding a lesson's history, cloned from its * stored pack. The clone carries the original's commit oids, so the fork shares * ancestry with it — which is what a reviewer's three-way merge needs. + * + * `keepVariations` says whether the lesson's other branches come too. Cloning our + * own lesson to push it back again, they must — dropping one would delete it. But + * a fork takes the lesson and not its author's half-finished ideas, so forking + * asks for the default branch alone. */ -async function cloneRepo(pack) { +async function cloneRepo(pack, { keepVariations = true } = {}) { const ctx = memRepo(); - await cloneFromPack({ ...ctx, ...pack }); + await cloneFromPack({ + ...ctx, + ...pack, + refs: keepVariations ? pack.refs : null, + }); return ctx; } @@ -156,7 +167,7 @@ export async function forkLesson(api, { lessonId, title }) { const author = await commitAuthor(api); let ctx; if (pack) { - ctx = await cloneRepo(pack); + ctx = await cloneRepo(pack, { keepVariations: false }); // Record where we came from, so a later sync has a base before it fetches // anything new. await fetchRemotePack({ ...ctx, ...pack, ref: UPSTREAM_REF }); @@ -185,11 +196,14 @@ export async function forkLesson(api, { lessonId, title }) { const packed = await packRepo(ctx); try { // A brand-new lesson has no history, so there is nothing to compare and swap - // against. + // against — every branch we are sending is new to it, which is what an empty + // `expected` says. await api.pushLessonPack(lesson.id, { packfile: packed.packfile, head: packed.head, parent: null, + refs: packed.refs, + expected: {}, }); } catch (err) { // The row exists but has no history behind it, which is the one state the @@ -285,8 +299,68 @@ export async function proposeChanges( ); } + // Two packs, because they answer two different questions. The proposal carries + // the lesson as this fork has it and nothing else — a variation its author is + // still turning over is not part of what is being offered. The fork's own + // history, pushed further down, carries everything, because leaving a branch + // out of that one would delete it. + const proposed = await packRepo({ ...ctx, only: [DEFAULT_BRANCH] }); const packed = await packRepo(ctx); + // Is there already one open from this fork? An assistant asked for a further + // change should update the proposal a human is already reading, not stack a + // second one beside it — the review queue would then hold two overlapping + // proposals and any discussion would be attached to the wrong one. + // + // Matched on the fork, not merely on the account: proposing to one lesson from + // two different forks is legitimate, and those are not updates of each other. + const existing = await api + .listPulls(target) + .then(({ pulls }) => + pulls.find( + (p) => + p.status === "open" && p.ready && p.sourceLessonId === forkLessonId, + ), + ) + .catch(() => null); + + if (existing) { + // An update may only move the proposal *forward*: the new tip has to contain + // the one it already points at. Usually it does — the fork's history is one + // branch advancing — but not always. If an earlier proposal's history push + // failed (it is best-effort, see pushForkHistory), this call cloned a pack + // without that commit and built a sibling instead. Uploading it would drop + // `previous_head` out of the history the pack carries, which is exactly the + // invariant that lets one pack per proposal answer "what changed in this + // update". The Worker cannot check this — it holds the pack opaquely — so it + // is checked here. + const forward = await contains({ + ...ctx, + oid: proposed.head, + ancestor: existing.head, + }); + if (!forward) { + throw new Error( + `This fork's history no longer builds on proposal ${existing.id}, so updating it would replace ` + + "what the reviewer has been reading rather than adding to it. Close that proposal in the web app " + + "and call propose_changes again to open a fresh one.", + ); + } + + const updated = await api.uploadPullPack(target, existing.id, { + packfile: proposed.packfile, + head: proposed.head, + }); + return { + pull: updated || existing, + lessonId: target, + commit: commit.oid, + changes: ops.map(describeOp), + historyPushed: await pushForkHistory(api, forkLessonId, packed, forkPack), + updated: true, + }; + } + // The target's tip as it stands, recorded on the request so a reviewer can see // what it was built against. Informational: the merge finds its own base from // the shared ancestry. @@ -295,7 +369,7 @@ export async function proposeChanges( const pull = await api.createPull(target, { title: clamp(title, PULL_TITLE_MAX), body: proposalBody(body, client), - head: packed.head, + head: proposed.head, base, sourceLessonId: forkLessonId, }); @@ -303,8 +377,8 @@ export async function proposeChanges( let ready; try { ready = await api.uploadPullPack(target, pull.id, { - packfile: packed.packfile, - head: packed.head, + packfile: proposed.packfile, + head: proposed.head, }); } catch (err) { await api.closePull(target, pull.id).catch(() => {}); @@ -321,16 +395,12 @@ export async function proposeChanges( // operations and refuse — the changes safe but unproposable without making a // further edit. Pushing last, a failed proposal leaves the fork exactly as it // was and the retry simply works. - let historyPushed = true; - try { - await api.pushLessonPack(forkLessonId, { - packfile: packed.packfile, - head: packed.head, - parent: forkPack.head, - }); - } catch { - historyPushed = false; - } + const historyPushed = await pushForkHistory( + api, + forkLessonId, + packed, + forkPack, + ); return { pull: ready || pull, @@ -338,5 +408,32 @@ export async function proposeChanges( commit: commit.oid, changes: ops.map(describeOp), historyPushed, + updated: false, }; } + +/** + * Advance the fork's own stored history, and never let it fail the call. + * + * The proposal does not depend on it — its pack is stored separately, and that is + * what a reviewer merges — so this is bookkeeping: it keeps the fork's History tab + * honest and gives the next proposal this commit to build on. + */ +async function pushForkHistory(api, forkLessonId, packed, forkPack) { + try { + await api.pushLessonPack(forkLessonId, { + packfile: packed.packfile, + head: packed.head, + parent: forkPack.head, + // The fork may hold variations its author started in the editor. They came + // down in the pack we cloned and are going back up in the one we packed, so + // they are named here too — a push that mentioned only the lesson's own + // branch would leave the hub advertising tips this pack no longer carries. + refs: packed.refs, + expected: forkPack.refs, + }); + return true; + } catch { + return false; + } +} diff --git a/apps/mcp/src/tools.js b/apps/mcp/src/tools.js index 688697e..f9d77fe 100644 --- a/apps/mcp/src/tools.js +++ b/apps/mcp/src/tools.js @@ -618,8 +618,11 @@ export function registerTools(server, ctx) { "returned `url` — that is the page where they read the diff and decide. Their answer is theirs to give: " + "don't tell them it is done, and don't try to merge it yourself.\n\n" + "The proposal carries ONE commit holding the fork as it now stands, so make all your edits before calling " + - "this. You may propose again after further edits; each proposal is a separate request (at most 5 open " + - "against one lesson).\n\n" + + "this.\n\n" + + "Calling it AGAIN from the same fork while a proposal is still open UPDATES that proposal rather than " + + "opening another — same request, same discussion, new contents — which is what you want after the human " + + "asks for a change. The title and body you pass are then ignored, since the ones already there are what " + + "they have been reading. `updated` in the result says which happened.\n\n" + "Write the title and body for the reviewer, not for the log: say what changed and why it is an improvement, " + "so someone who has not read the diff can judge it.", inputSchema: { @@ -656,6 +659,7 @@ export function registerTools(server, ctx) { commit, changes, historyPushed, + updated, } = await proposeChanges(api, { forkLessonId, lessonId, @@ -673,9 +677,14 @@ export function registerTools(server, ctx) { commit, changes, url: proposalUrl(target, pull.id), + revision: pull.revision, note: - "Proposal opened. Nothing has changed in the lesson itself — give the user the `url` so they can read " + - "the diff and merge or decline it. Poll list_lesson_proposals if you need to know what they decided." + + (updated + ? "This fork already had a proposal open, so it was UPDATED rather than duplicated — same proposal, " + + "same discussion, new contents. Nothing has changed in the lesson itself." + : "Proposal opened. Nothing has changed in the lesson itself.") + + " Give the user the `url` so they can read the diff and merge or decline it. Poll " + + "list_lesson_proposals if you need to know what they decided." + // The proposal is complete either way — its changes are stored with it. // This only means the fork's own history didn't catch up. (historyPushed diff --git a/apps/mcp/test/fork.test.js b/apps/mcp/test/fork.test.js index 6ced3c9..c2046a4 100644 --- a/apps/mcp/test/fork.test.js +++ b/apps/mcp/test/fork.test.js @@ -117,12 +117,36 @@ function fakeHub() { return clone(pull); }, + async listPulls(lessonId) { + return { + pulls: pulls.filter((p) => p.lessonId === lessonId).map(clone), + canReview: false, + }; + }, + + // Mirrors the Worker's planPullUpload: the first upload has to match the head + // the row was opened with; a later one records a revision. async uploadPullPack(lessonId, pullId, { packfile, head }) { const pull = pulls.find((p) => p.id === pullId); if (!pull) throw new Error("Proposal not found."); - assert.equal(head, pull.head, "the pack must match the head opened with"); + // The Worker refuses an upload to a proposal that is no longer open, and a + // fake that accepted one would hide a caller that had stopped checking. + if (pull.status !== "open") + throw new Error("This proposal is no longer open."); + if (pull.ready) { + assert.notEqual(head, pull.head, "an update has to actually move"); + pull.previousHead = pull.head; + pull.head = head; + pull.revision = (pull.revision || 1) + 1; + } else { + assert.equal( + head, + pull.head, + "the pack must match the head opened with", + ); + pull.ready = true; + } pullPacks.set(pullId, { packfile, head }); - pull.ready = true; return clone(pull); }, @@ -358,7 +382,7 @@ test("proposing sends a pack that shares ancestry with the target lesson", async assert.equal(hub.packs.get(fork.id).head, uploaded.head); }); -test("proposing twice stacks on the first proposal instead of colliding", async () => { +test("proposing twice updates the proposal already open, rather than stacking one beside it", async () => { const hub = fakeHub(); const source = await seedLesson(hub, { title: "Volcanoes", @@ -371,6 +395,7 @@ test("proposing twice stacks on the first proposal instead of colliding", async forkLessonId: fork.id, title: "First pass", }); + assert.equal(first.updated, false); hub.lessons.get(fork.id).doc.sections[0].blocks[0].text = "Second revision."; const second = await proposeChanges(hub.api, { @@ -378,11 +403,21 @@ test("proposing twice stacks on the first proposal instead of colliding", async title: "Second pass", }); + assert.equal( + second.updated, + true, + "the second call updates rather than opens", + ); + assert.equal(hub.pulls.length, 1, "the reviewer sees one proposal, not two"); + assert.equal(second.pull.id, first.pull.id); + assert.equal(second.pull.revision, 2); + // Its title is the one the reviewer has been reading; an update doesn't rewrite + // the conversation it belongs to. + assert.equal(second.pull.title, "First pass"); assert.notEqual(second.commit, first.commit); - assert.equal(hub.pulls.length, 2, "each proposal is its own request"); - // The second builds on the first, which is what the compare-and-swap in - // pushLessonPack would have refused otherwise. + // The update builds on what the proposal already contained, which is the rule + // that keeps one pack per proposal enough — the earlier commit is still in it. const ctx = memRepo("review"); await cloneFromPack({ ...ctx, ...hub.pullPacks.get(second.pull.id) }); assert.equal( @@ -391,6 +426,32 @@ test("proposing twice stacks on the first proposal instead of colliding", async ); }); +test("a proposal that has been resolved is not updated — the next one is its own", async () => { + const hub = fakeHub(); + const source = await seedLesson(hub, { + title: "Volcanoes", + text: "A volcano ERUPTS.", + }); + const { lesson: fork } = await forkLesson(hub.api, { lessonId: source.id }); + + hub.lessons.get(fork.id).doc.sections[0].blocks[0].text = "First revision."; + const first = await proposeChanges(hub.api, { + forkLessonId: fork.id, + title: "First pass", + }); + await hub.api.closePull(source.id, first.pull.id); + + hub.lessons.get(fork.id).doc.sections[0].blocks[0].text = "Second revision."; + const second = await proposeChanges(hub.api, { + forkLessonId: fork.id, + title: "Second pass", + }); + + assert.equal(second.updated, false); + assert.notEqual(second.pull.id, first.pull.id); + assert.equal(second.pull.title, "Second pass"); +}); + test("proposing an unchanged fork is refused, and opens nothing", async () => { const hub = fakeHub(); const source = await seedLesson(hub, { diff --git a/apps/web/src/components/ChangeSummary.jsx b/apps/web/src/components/ChangeSummary.jsx new file mode 100644 index 0000000..b8b5c71 --- /dev/null +++ b/apps/web/src/components/ChangeSummary.jsx @@ -0,0 +1,79 @@ +// What a set of block operations changed, rendered the same way everywhere it is +// asked. +// +// Three places ask: a commit in the version history, a proposal's own page, and +// (through the first) a restore preview. They were never going to stay in step as +// three copies, and "what changed" is exactly the thing a reader compares between +// views — a proposal that reads differently from the commit it becomes would be +// worse than no summary at all. +// +// Both pieces take ops in the shape ops.js produces, and neither needs git: the +// operations are already derived by the time they get here, which is what lets +// the proposal page draw this without the engine loaded. + +import { useTranslation } from "react-i18next"; +import { Badge } from "./ui/badge.jsx"; +import { describeOp } from "@spelling-creator/core/git/ops"; + +// The counts badged against a change. Section and title operations are +// deliberately not tallied — they are structure, they appear in the list below, +// and a chip saying "1 changed" for a renamed section next to "1 changed" for a +// rewritten question would flatten a distinction that matters. +function tally(ops) { + const counts = { added: 0, edited: 0, removed: 0, moved: 0 }; + for (const op of ops) { + if (op.op === "block.add") counts.added++; + else if (op.op === "block.edit") counts.edited++; + else if (op.op === "block.remove") counts.removed++; + else if (op.op === "block.move") counts.moved++; + } + return counts; +} + +// Chip colors, mapped from MUI's semantic palette onto this app's tokens — +// success/destructive already exist; "changed" borrows --primary for the +// same blue-ish "info" read, "moved" stays neutral (MUI's "default"). +const CHIP_STYLES = { + success: "border-success/40 bg-success/10 text-success", + info: "border-primary/40 bg-primary/10 text-primary", + error: "border-destructive/40 bg-destructive/10 text-destructive", + default: "border-border bg-transparent text-muted-foreground", +}; + +/** "3 added · 1 changed · 2 removed" — the shape of a change, at a glance. */ +export function ChangeChips({ ops, className = "mt-1.5" }) { + const { t } = useTranslation("editorTools"); + const counts = tally(ops); + const chips = [ + ["added", counts.added, "success"], + ["changed", counts.edited, "info"], + ["removed", counts.removed, "error"], + ["moved", counts.moved, "default"], + ].filter(([, n]) => n > 0); + + if (chips.length === 0) return null; + return ( +
+ {chips.map(([label, n, color]) => ( + + {n} {t(`historyDialog.chips.${label}`)} + + ))} +
+ ); +} + +/** Every operation, spelled out. */ +export function ChangeList({ ops, className = "max-h-[260px]" }) { + return ( +
    + {ops.map((op, i) => ( +
  • + {/* describeOp renders "- edit text block "; drop the leading + marker, the list already provides one. */} + {describeOp(op).replace(/^- /, "")} +
    • + ))} +
+ ); +} diff --git a/apps/web/src/components/HistoryDialog.jsx b/apps/web/src/components/HistoryDialog.jsx index eb04717..7ee801f 100644 --- a/apps/web/src/components/HistoryDialog.jsx +++ b/apps/web/src/components/HistoryDialog.jsx @@ -8,7 +8,13 @@ import { useCallback, useEffect, useState } from "react"; import { useTranslation } from "react-i18next"; -import { GitMergeIcon, HistoryIcon, RotateCcwIcon, XIcon } from "lucide-react"; +import { + GitMergeIcon, + HistoryIcon, + RotateCcwIcon, + Undo2Icon, + XIcon, +} from "lucide-react"; import { Dialog, DialogContent, @@ -17,12 +23,11 @@ import { DialogFooter, } from "./ui/dialog.jsx"; import { Button } from "./ui/button.jsx"; -import { Badge } from "./ui/badge.jsx"; import { Alert, AlertDescription } from "./ui/alert.jsx"; import { Tooltip, TooltipTrigger, TooltipContent } from "./ui/tooltip.jsx"; import { HistorySkeleton } from "./Skeletons.jsx"; import { cn } from "../lib/utils.js"; -import { describeOp } from "@spelling-creator/core/git/ops"; +import { ChangeChips, ChangeList } from "./ChangeSummary.jsx"; import i18n from "../lib/i18n.js"; /** "just now" / "12 minutes ago" / "3 days ago" — then fall back to a date. */ @@ -56,55 +61,20 @@ export function timeAgo(ts) { return i18n.t(`editorTools:timeAgo.${unitKey}`, { count: rounded }); } -// The counts badged against a commit, derived from its ops. -function tally(ops) { - const counts = { added: 0, edited: 0, removed: 0, moved: 0 }; - for (const op of ops) { - if (op.op === "block.add") counts.added++; - else if (op.op === "block.edit") counts.edited++; - else if (op.op === "block.remove") counts.removed++; - else if (op.op === "block.move") counts.moved++; - } - return counts; -} - -// Chip colors, mapped from MUI's semantic palette onto this app's tokens — -// success/destructive already exist; "changed" borrows --primary for the -// same blue-ish "info" read, "moved" stays neutral (MUI's "default"). -const CHIP_STYLES = { - success: "border-success/40 bg-success/10 text-success", - info: "border-primary/40 bg-primary/10 text-primary", - error: "border-destructive/40 bg-destructive/10 text-destructive", - default: "border-border bg-transparent text-muted-foreground", -}; - -function ChangeChips({ ops }) { - const { t } = useTranslation("editorTools"); - const counts = tally(ops); - const chips = [ - ["added", counts.added, "success"], - ["changed", counts.edited, "info"], - ["removed", counts.removed, "error"], - ["moved", counts.moved, "default"], - ].filter(([, n]) => n > 0); - - if (chips.length === 0) return null; - return ( -
- {chips.map(([label, n, color]) => ( - - {n} {t(`historyDialog.chips.${label}`)} - - ))} -
- ); -} - /** * @param {object} props.git The useLessonGit controller. * @param {Function} props.onRestore Called with the restored doc; the editor adopts it. + * @param {Function} props.onUndo Called with a commit oid to undo just that + * change. The editor owns it because it may + * need the conflict dialog. */ -export default function HistoryDialog({ open, onClose, git, onRestore }) { +export default function HistoryDialog({ + open, + onClose, + git, + onRestore, + onUndo, +}) { const { t } = useTranslation("editorTools"); const [commits, setCommits] = useState(null); // null = still loading const [selected, setSelected] = useState(null); // oid @@ -112,7 +82,13 @@ export default function HistoryDialog({ open, onClose, git, onRestore }) { const [restoring, setRestoring] = useState(false); const [error, setError] = useState(null); - const { loadHistory, diffFor, restore, pending } = git; + const { loadHistory, diffFor, diffAgainstCurrent, restore, pending } = git; + + // Which question the right-hand panel is answering. "What changed in this + // version" is history; "what would I get back" is the decision someone is + // actually about to make, and the two have different answers the moment + // anything has happened since. + const [mode, setMode] = useState("changed"); // Re-read the history each time the dialog opens: the editor has very likely // committed since it was last closed. @@ -124,6 +100,7 @@ export default function HistoryDialog({ open, onClose, git, onRestore }) { setSelected(null); setDetail(null); setError(null); + setMode("changed"); loadHistory().then((list) => { if (cancelled) return; @@ -136,20 +113,21 @@ export default function HistoryDialog({ open, onClose, git, onRestore }) { }; }, [open, loadHistory]); - // What the selected commit changed, against its first parent. + // What the selected commit changed, or how it differs from the document now. useEffect(() => { if (!open || !selected) return; let cancelled = false; setDetail(null); - diffFor(selected).then((ops) => { + const ask = mode === "current" ? diffAgainstCurrent : diffFor; + ask(selected).then((ops) => { if (!cancelled) setDetail(ops); }); return () => { cancelled = true; }; - }, [open, selected, diffFor]); + }, [open, selected, mode, diffFor, diffAgainstCurrent]); const handleRestore = useCallback(async () => { if (!selected) return; @@ -167,6 +145,10 @@ export default function HistoryDialog({ open, onClose, git, onRestore }) { }, [selected, restore, onRestore, onClose, t]); const isCurrent = commits && selected === commits[0]?.oid; + // The first commit has nothing before it, so there is no "before" to put back — + // undoing it would mean emptying the lesson, which is a different request. + const canUndo = + commits && selected && commits[commits.length - 1]?.oid !== selected; return ( !next && onClose()}> @@ -250,33 +232,47 @@ export default function HistoryDialog({ open, onClose, git, onRestore }) { ))} - {/* What that version changed. */} + {/* What that version changed — or how it differs from the document + as it now stands, which is the question worth asking with a + finger over Restore. */}
+
+ {["changed", "current"].map((option) => ( + + ))} +
+ {detail === null ? ( ) : detail.length === 0 ? (

- {t("historyDialog.origin")} + {mode === "current" + ? t("historyDialog.sameAsNow") + : t("historyDialog.origin")}

) : ( <>

- {t("historyDialog.whatChanged")} + {mode === "current" + ? t("historyDialog.whatRestoringChanges") + : t("historyDialog.whatChanged")}

-
    - {detail.map((op, i) => ( -
  • - {/* describeOp renders "- edit text block "; drop the - leading marker, the list already provides one. */} - {describeOp(op).replace(/^- /, "")} -
    • - ))} -
+ )}
@@ -288,6 +284,31 @@ export default function HistoryDialog({ open, onClose, git, onRestore }) { + + {/* Undo one change, as against Restore's "put the whole lesson back to + here". They answer different questions and the difference matters: + restoring drops everything since, undoing keeps it. */} + {onUndo && ( + + + + + + {t("historyDialog.undoTooltip")} + + + )} + + + + + + ); +} + +/** One variation: what it is called, how much is on it, and what can be done. */ +function Row({ + item, + current, + busy, + confirming, + onSwitch, + onRename, + onBringIn, + onDelete, + onConfirmDelete, + onCancelDelete, +}) { + const { t } = useTranslation("editorTools"); + + // Asking in place rather than in a second dialog: the question is about this one + // row, and the answer is one press from where the finger already is. + if (confirming) { + return ( +
+

+ {t("variations.confirmDelete", { name: branchLabel(item.name) })} +

+ + +
+ ); + } + + return ( +
+ + + {!item.isDefault && ( + + + + + + + + {t("variations.bringIn")} + + + + {t("variations.rename")} + + + + {t("variations.delete")} + + + + )} +
+ ); +} + +/** Naming a variation, whether it is being created or renamed. */ +function NameField({ + value, + onChange, + onSubmit, + onCancel, + busy, + placeholder, + submitLabel, + autoFocus, +}) { + const { t } = useTranslation("editorTools"); + + return ( +
{ + event.preventDefault(); + onSubmit(); + }} + > + onChange(event.target.value)} + onKeyDown={(event) => event.key === "Escape" && onCancel()} + className="h-8" + /> + + +
+ ); +} diff --git a/apps/web/src/lib/git/engine.js b/apps/web/src/lib/git/engine.js index f774d85..4c658f6 100644 --- a/apps/web/src/lib/git/engine.js +++ b/apps/web/src/lib/git/engine.js @@ -49,17 +49,24 @@ export { ORIGIN_REF, UPSTREAM_REF, authorFrom, + branchRef, changedBlockIds, + checkoutBranch, commitDoc, + createBranch, + currentBranch, + deleteBranch, diffCommits, diffFromParent, ensureRepo, headOid, history, + listBranches, pendingOps, pullRef, readDocAt, readHeadDoc, + renameBranch, restoreCommit, treeOfCommit, } from "@spelling-creator/core/git/repo"; @@ -77,9 +84,13 @@ export { completeMerge, forkLessonRepo, forkLocalRepo, + prepareBranchMerge, prepareMerge, + prepareRevert, + prepareProposalReview, preparePullMerge, pushHistory, remoteStatus, submitPullRequest, + updatePullRequest, } from "@spelling-creator/core/browser/git/sync"; diff --git a/apps/web/src/lib/git/useLessonGit.js b/apps/web/src/lib/git/useLessonGit.js index 45f1dfe..7d0fc21 100644 --- a/apps/web/src/lib/git/useLessonGit.js +++ b/apps/web/src/lib/git/useLessonGit.js @@ -20,6 +20,8 @@ import { useCallback, useEffect, useRef, useState } from "react"; import { DRAFT_REPO, repoIdFor } from "@spelling-creator/core/git/doc"; import { loadGitEngine } from "./load.js"; import { fetchPack } from "@spelling-creator/core/git/remote"; +import { DEFAULT_BRANCH, toBranchName } from "@spelling-creator/core/git/refs"; +import { diffDocs } from "@spelling-creator/core/git/ops"; // Commit once the user has been still for this long. const IDLE_MS = 4000; @@ -45,6 +47,13 @@ export function useLessonGit({ doc, editingId, identity, enabled = true }) { const [lastCommit, setLastCommit] = useState(null); // { oid, at } | null const [error, setError] = useState(null); + // The variations this lesson holds, and which one is being edited. Both come + // out of the repository (branches, and HEAD) rather than being state of their + // own — so a reload, a second tab, or publishing a draft all find the same + // answer the repository already had. + const [branches, setBranches] = useState([]); + const [branch, setBranch] = useState(DEFAULT_BRANCH); + // Bumped to force the setup effect to run again. A fork or an import swaps the // repository out from under us *without* changing the repo id (both land in the // draft slot), and the effect keys on the id, so it needs telling. @@ -119,6 +128,8 @@ export function useLessonGit({ doc, editingId, identity, enabled = true }) { setPending( (await engine.pendingOps({ ...ctx, doc: docRef.current })).length, ); + setBranch(await engine.currentBranch(ctx)); + setBranches(await engine.listBranches(ctx)); setReady(true); } catch (err) { console.error("[lesson-git] setup failed", err); @@ -267,6 +278,36 @@ export function useLessonGit({ doc, editingId, identity, enabled = true }) { [repoId, run], ); + /** + * What restoring a version would change — the difference between it and the + * document as it now stands. + * + * A different question from `diffFor`, and the one actually being asked at the + * moment somebody hovers over Restore. "What changed in this version" is + * history; "what would I get back" is a decision. + * + * Against the *live* document, not the last commit. Restoring replaces what is + * on screen, so edits made inside the four-second commit window are part of what + * would be lost — and a preview that omitted them would understate the cost of + * the one action it exists to inform. + */ + const diffAgainstCurrent = useCallback( + (oid) => + run(async () => { + try { + const engine = await loadGitEngine(); + const ctx = engine.repoCtx(repoId); + return diffDocs( + docRef.current, + await engine.readDocAt({ ...ctx, oid }), + ); + } catch { + return []; + } + }), + [repoId, run], + ); + /** Restore an earlier version. Returns the restored doc for the editor to adopt. */ const restore = useCallback( (oid) => @@ -287,6 +328,123 @@ export function useLessonGit({ doc, editingId, identity, enabled = true }) { [repoId, run], ); + // ---- variations ---------------------------------------------------------- + // + // A variation is a branch, and the four things you can do to one are create, + // switch, rename and delete. Every one of them commits what's outstanding + // first: the editor's rule is that work is checkpointed when you pause, so a + // variation must never be the thing that loses the last few seconds of it. + // + // Which is why the checkpoint below is *not* wrapped in a catch. commitDoc + // returns null when there is nothing to commit — that is the ordinary case and + // needs no handling — so the only thing a catch could swallow is a real failure, + // and swallowing that would let the checkout replace the document and clear the + // dirty markers with the edits still uncommitted. The caller shows the error + // instead, and nothing moves. + + const refreshBranches = useCallback( + () => + run(async () => { + const engine = await loadGitEngine(); + const ctx = engine.repoCtx(repoId); + const list = await engine.listBranches(ctx); + setBranches(list); + setBranch(await engine.currentBranch(ctx)); + + // Re-read the tip too. A merge doesn't always leave a commit behind — a + // fast-forward moves the branch instead (see completeMerge) — so the + // chip would otherwise still be timing the commit before it. + const head = await engine.headOid(ctx); + setLastCommit(head ? { oid: head, at: Date.now() } : null); + return list; + }), + [repoId, run], + ); + + /** Move to a branch and hand back the document as it stands there. */ + const switchBranch = useCallback( + (name) => + run(async () => { + const engine = await loadGitEngine(); + const ctx = engine.repoCtx(repoId); + + await engine.commitDoc({ + ...ctx, + doc: docRef.current, + author: identityRef.current, + }); + + const result = await engine.checkoutBranch({ ...ctx, name }); + dirtySince.current = null; + setPending(0); + setBranch(result.name); + setBranches(await engine.listBranches(ctx)); + const head = await engine.headOid(ctx); + setLastCommit(head ? { oid: head, at: Date.now() } : null); + return result.doc; + }), + [repoId, run], + ); + + /** + * Start a variation from where we are and switch to it. + * + * `label` is what the author typed; the branch takes the name that survives + * git's rules (see core/git/refs.js). An empty result means they typed nothing + * git could keep, which is the caller's to report. + */ + const createVariation = useCallback( + (label) => + run(async () => { + const name = toBranchName(label); + if (!name) throw new Error("Please give this variation a name."); + + const engine = await loadGitEngine(); + const ctx = engine.repoCtx(repoId); + + await engine.commitDoc({ + ...ctx, + doc: docRef.current, + author: identityRef.current, + }); + + await engine.createBranch({ ...ctx, name }); + dirtySince.current = null; + setPending(0); + setBranch(name); + setBranches(await engine.listBranches(ctx)); + return name; + }), + [repoId, run], + ); + + const renameVariation = useCallback( + (from, label) => + run(async () => { + const to = toBranchName(label); + if (!to) throw new Error("Please give this variation a name."); + + const engine = await loadGitEngine(); + const ctx = engine.repoCtx(repoId); + await engine.renameBranch({ ...ctx, from, to }); + setBranch(await engine.currentBranch(ctx)); + setBranches(await engine.listBranches(ctx)); + return to; + }), + [repoId, run], + ); + + const deleteVariation = useCallback( + (name) => + run(async () => { + const engine = await loadGitEngine(); + const ctx = engine.repoCtx(repoId); + await engine.deleteBranch({ ...ctx, name }); + setBranches(await engine.listBranches(ctx)); + }), + [repoId, run], + ); + /** * Take the draft repo's history with us when a draft is first published. * Commits everything outstanding first, so nothing is stranded in the draft. @@ -315,9 +473,18 @@ export function useLessonGit({ doc, editingId, identity, enabled = true }) { error, clearError: () => setError(null), repoId, + branch, + branches, + onDefaultBranch: branch === DEFAULT_BRANCH, + refreshBranches, + switchBranch, + createVariation, + renameVariation, + deleteVariation, commitNow, loadHistory, diffFor, + diffAgainstCurrent, restore, adoptDraft, reload, diff --git a/apps/web/src/locales/en/editor.json b/apps/web/src/locales/en/editor.json index 08f4d22..d59fc75 100644 --- a/apps/web/src/locales/en/editor.json +++ b/apps/web/src/locales/en/editor.json @@ -44,7 +44,12 @@ "proposeTooltip": "Offer your work back to {{name}}. Nothing there changes: your version is sent as a proposal, and its author — or someone they trust — decides whether to merge it in.", "collapseAll": "Collapse all", "expandAll": "Expand all", - "untitledCrumb": "Untitled lesson" + "untitledCrumb": "Untitled lesson", + "mainLesson": "Main lesson", + "variationsTooltip": "You're working on the main lesson — the one everyone sees. Click to try changes on a separate copy instead.", + "onVariationTooltip": "You're working on a variation. The main lesson stays exactly as it is until you bring this in. Click to switch back or manage your variations.", + "updateProposalButton": "Update your proposal", + "updateProposalTooltip": "Replace what “{{title}}” contains with your work as it stands now. Its title, note and any discussion stay where they are." }, "stats": { "sections_one": "{{count}} section", @@ -142,7 +147,18 @@ "forkedWithoutHistory": "Forked into a new lesson — edit freely, then save it to the cloud as your own copy.", "loadedPublished": "Loaded your published lesson — edit and save to the cloud to update it.", "loadedDraft": "Loaded your draft — edit and save to the cloud, or publish it to the hub.", - "couldNotOpenLesson": "Could not open that lesson for {{action}}." + "couldNotOpenLesson": "Could not open that lesson for {{action}}.", + "variationBroughtIn": "“{{name}}” is now part of the main lesson.", + "variationAlreadyIn": "“{{name}}” is already part of the main lesson.", + "proposalUpdated": "“{{title}}” now has your latest changes.", + "couldNotUndo": "That change could not be undone: {{error}}", + "nothingToUndo": "That change isn’t in the lesson any more — there’s nothing to undo.", + "undone": "That change has been undone. Everything since it is still here.", + "tryingInVariation": "Started a variation called “{{name}}” to try these changes in.", + "proposalInVariation": "These changes are now in your “{{name}}” variation. The main lesson is untouched, and the proposal is still waiting for your decision.", + "proposalAlreadyIn": "The lesson already contains these changes.", + "tryingInExistingVariation": "Switched to your “{{name}}” variation to try these changes in.", + "nothingToMergeInto": "The main lesson has no versions yet, so there is nothing to bring these changes into." }, "labels": { "theOriginal": "the original", @@ -159,7 +175,8 @@ "publishToHub": "Publish to hub", "updatePublishedLesson": "Update published lesson", "saveAsDraft": "Save as draft", - "updateDraft": "Update draft" + "updateDraft": "Update draft", + "viewProposal": "View proposal" }, "defaultDoc": { "title": "Put your topic here..." diff --git a/apps/web/src/locales/en/editorTools.json b/apps/web/src/locales/en/editorTools.json index 574a0f1..2257a3b 100644 --- a/apps/web/src/locales/en/editorTools.json +++ b/apps/web/src/locales/en/editorTools.json @@ -27,7 +27,17 @@ "whatChanged": "What changed", "close": "Close", "restoreCurrent": "This is the current version", - "restore": "Restore this version" + "restore": "Restore this version", + "compare": { + "changed": "What changed here", + "current": "Difference from now" + }, + "whatRestoringChanges": "Going back to this would:", + "sameAsNow": "This is exactly what the lesson looks like now.", + "undo": "Undo this change", + "undoing": "Undoing...", + "undoError": "That change could not be undone.", + "undoTooltip": "Put back what this version changed, and keep everything since." }, "mergeDialog": { "emptyValue": "(empty)", @@ -54,11 +64,17 @@ "landing": "Merging it in...", "merging": "Merging...", "mergeProposal": "Merge these changes in", - "merge": "Merge" + "merge": "Merge", + "mergeVariation": "Bring it in", + "undo": "Undo it", + "tryIt": "Put it in the variation" }, "title": { "pullRequest": "Review the proposal: {{name}}", - "pull": "Merge changes from {{name}}" + "pull": "Merge changes from {{name}}", + "variation": "Bring “{{name}}” into the main lesson", + "undo": "Undo “{{name}}”", + "pullRequestTry": "Try “{{name}}” in a variation" }, "someone": "someone", "reviewingNotice": "Merging this updates the published lesson itself, for everyone reading it. Nothing is written until you confirm, and {{name}} will be told either way.", @@ -76,7 +92,8 @@ "noConflicts": "Nothing is in conflict — the whole merge resolved on its own. Confirm to apply it.", "conflictsIntro_one": "{{count}} block was edited on both sides in the same place. Choose which to keep.", "conflictsIntro_other": "{{count}} blocks were edited on both sides in the same place. Choose which to keep.", - "cancel": "Cancel" + "cancel": "Cancel", + "tryingNotice": "This goes into a variation of your own, not the lesson. Nothing anyone else sees changes, and the proposal stays open for you to decide on." }, "proposeDialog": { "defaultLessonName": "the original lesson", @@ -128,5 +145,28 @@ }, "noResults": "No images found. Try different words.", "close": "Close" + }, + "variations": { + "title": "Variations", + "description": "A variation is a separate copy of this lesson you can change freely. The main lesson stays exactly as it is until you bring a variation into it.", + "mainLesson": "Main lesson", + "mainLessonHint": "What everyone sees", + "editingNow": "You're working on this", + "changesAhead_one": "{{count}} change that isn't in the main lesson", + "changesAhead_other": "{{count}} changes that aren't in the main lesson", + "start": "New variation", + "create": "Create", + "namePlaceholder": "Simpler for Year 3", + "saveName": "Rename", + "cancel": "Cancel", + "close": "Close", + "rename": "Rename…", + "bringIn": "Bring into the main lesson", + "delete": "Delete", + "moreActions": "More actions for {{name}}", + "privacy": "Variations are saved with the lesson, so anyone who can open the lesson can see them.", + "confirmDelete": "Delete “{{name}}”? Anything only in this variation goes with it.", + "confirmDeleteYes": "Delete it", + "nameLabel": "Name for this variation" } } diff --git a/apps/web/src/locales/en/lesson.json b/apps/web/src/locales/en/lesson.json index ee74dd5..cc3ef50 100644 --- a/apps/web/src/locales/en/lesson.json +++ b/apps/web/src/locales/en/lesson.json @@ -121,7 +121,22 @@ "emptyState": "Nobody has proposed changes to this lesson yet. Fork it, make your changes, and offer them back.", "notFound": "That proposal no longer exists.", "backToProposals": "All proposals", - "reviewHint": "Opens this lesson in the editor with the proposal in hand. Nothing is written until you confirm the merge." + "reviewHint": "Opens this lesson in the editor with the proposal in hand. Nothing is written until you confirm the merge.", + "changes": { + "heading": "What this changes", + "none": "This proposal doesn't change anything in the lesson.", + "clean": "This merges cleanly — there's nothing to decide.", + "conflicts_one": "{{count}} block has been changed here and in the lesson, so merging will ask you which to keep.", + "conflicts_other": "{{count}} blocks have been changed here and in the lesson, so merging will ask you which to keep.", + "alreadyIn": "These changes are already part of the lesson.", + "sinceUpdate": "What the last update changed" + }, + "changesGone": "The proposed changes are no longer stored.", + "changesError": "The proposed changes could not be read.", + "revision": "Version {{n}} · updated {{when}}", + "fromVariation": "From the proposer’s “{{name}}” variation", + "tryIt": "Try it in a variation", + "tryItHint": "Or put these changes in a variation of your own first, so you can look through the lesson with them in it. The lesson everyone reads doesn’t change, and this proposal stays open." }, "comments": { "couldNotLoad": "Could not load comments.", diff --git a/apps/web/src/pages/EditorPage.jsx b/apps/web/src/pages/EditorPage.jsx index c952343..668e796 100644 --- a/apps/web/src/pages/EditorPage.jsx +++ b/apps/web/src/pages/EditorPage.jsx @@ -18,6 +18,7 @@ import { EyeIcon, FileTextIcon, FileUpIcon, + GitBranchIcon, GitForkIcon, GitMergeIcon, GitPullRequestIcon, @@ -73,6 +74,7 @@ import CollabChat from "../components/CollabChat.jsx"; import FirstLessonWizard from "../components/FirstLessonWizard.jsx"; import AiLessonIdeaDialog from "../components/AiLessonIdeaDialog.jsx"; import HistoryDialog, { timeAgo } from "../components/HistoryDialog.jsx"; +import VariationsDialog from "../components/VariationsDialog.jsx"; import MergeDialog from "../components/MergeDialog.jsx"; import ProposeChangesDialog from "../components/ProposeChangesDialog.jsx"; // The preview dialog renders the working doc with the very same read-only @@ -82,6 +84,12 @@ import { AGE_RANGES } from "@spelling-creator/core/ageRanges"; import { newId } from "@spelling-creator/core/id"; import { extractCapitalizedWords } from "@spelling-creator/core/spelling"; import { useLessonGit } from "../lib/git/useLessonGit.js"; +import { + DEFAULT_BRANCH as engineDefaultBranch, + branchLabel, + toBranchName, +} from "@spelling-creator/core/git/refs"; +import { diffDocs } from "@spelling-creator/core/git/ops"; // The git engine (isomorphic-git + LightningFS) is loaded on demand rather than // imported directly, so it stays out of the bundle every homepage and hub visitor // downloads. loadGitEngine() memoises the import; by the time any of these flows @@ -286,6 +294,13 @@ export default function EditorPage() { // for (author / trusted collaborator only, server-enforced) // "publish" a save found the hub ahead of us; merge, then save again const [mergeIntent, setMergeIntent] = useState("pull"); + // The variation being folded into the main lesson, so the merge dialog and the + // toast afterwards can name it. + const [mergeVariation, setMergeVariation] = useState(null); + // The proposal a try-out is for. `reviewPull` deliberately stays null on that + // path (it is what makes a confirm *land* the proposal), so the title it needs + // for the dialog and the merge message is held separately. + const [mergeProposalTitle, setMergeProposalTitle] = useState(""); const { enabled: authEnabled, @@ -315,6 +330,7 @@ export default function EditorPage() { const panel = location.pathname.replace(/^\/editor\/?/, "").split("/")[0]; const historyOpen = panel === "history"; const collabOpen = panel === "collaborate"; + const variationsOpen = panel === "variations"; // // Opening pushes; closing *replaces*. Both pushing would leave the history as // [/editor, /editor/history, /editor], so Back from a panel you had just @@ -1301,6 +1317,144 @@ export default function EditorPage() { } }; + // A variation name for trying a proposal out: the title, so a reviewer knows + // which one it is, and the proposal's first few characters, so they know *which + // proposal*. + // + // The suffix isn't decoration. Without it two proposals whose titles normalise + // the same share a variation, and trying the second lands it on top of the + // first — one branch holding two people's changes, presented as one. It also + // makes the name stable, so re-trying a proposal returns to the variation it + // made last time rather than to somebody else's. + const variationNameFor = (title, id) => { + const short = id.replace(/-/g, "").slice(0, 6); + return ( + toBranchName(`${title} ${short}`) || toBranchName(`Proposal ${short}`) + ); + }; + + // Undo one change from the history, keeping everything since it. + // + // Restoring puts the whole lesson back and drops what came after; this puts back + // only what that one version changed. Where the two overlap — the change has + // been built on since — there is a genuine decision to make, and it goes to the + // usual conflict dialog rather than being guessed at. + // What to call the other side of a merge, per intent. + // + // One function rather than the two parallel ternary chains this used to be — + // the merge dialog's title and the commit message have to agree, and a new + // intent added to one chain and not the other is exactly how they stopped. + // `dialog` differs only where a generic name reads better than an empty one. + const theirNameFor = (intent, { dialog = false } = {}) => { + switch (intent) { + case "publish": + return dialog ? t("labels.theSavedLesson") : doc.title; + case "pull-request": + return reviewPull?.title || t("labels.theProposal"); + case "pull-request-try": + return mergeProposalTitle || t("labels.theProposal"); + case "variation": + return branchLabel(mergeVariation || ""); + case "undo": + return merge?.summary || ""; + default: + return dialog + ? forkedFromTitle || t("labels.theOriginal") + : forkedFromTitle; + } + }; + + const handleUndoCommit = async (oid) => { + setBusy("merge"); + try { + await git.commitNow(); + + const engine = await loadGitEngine(); + const prepared = await engine.prepareRevert({ + repoId: git.repoId, + oid, + doc, + }); + + // Nothing of that change is still standing: it has already been undone, or + // everything it touched has since been changed again. Say so rather than + // opening a dialog that would commit nothing. + if ( + prepared.conflicts.length === 0 && + diffDocs(doc, prepared.doc).length === 0 + ) { + notify({ severity: "info", message: t("messages.nothingToUndo") }); + return; + } + + setMergeIntent("undo"); + setMerge(prepared); + } catch (err) { + console.error(err); + notify({ + severity: "error", + message: t("messages.couldNotUndo", { error: err.message || err }), + }); + } finally { + setBusy(null); + } + }; + + // Bring a variation into the main lesson. + // + // The order is the whole of it. We switch to the main lesson *first*, so the + // merge commits there and the document that comes back is the lesson with the + // variation folded in — not the variation with the lesson folded in, which is + // the same commit and the opposite meaning. Switching commits anything + // outstanding to the variation on its way out, so nothing in flight is lost. + const handleBringVariationIn = async (name) => { + setBusy("merge"); + try { + // Merging commits to whatever is checked out, so this has to have taken + // before anything is prepared — otherwise the lesson gets folded into the + // variation, which is the same commit and the opposite meaning. A failure + // throws and is caught below; a null document means the lesson has no + // commits at all, and there is nothing to merge into. + const onMain = await git.switchBranch(engineDefaultBranch); + if (!onMain) { + notify({ severity: "info", message: t("messages.nothingToMergeInto") }); + return; + } + setDoc(onMain); + + const engine = await loadGitEngine(); + const prepared = await engine.prepareBranchMerge({ + repoId: git.repoId, + name, + doc: onMain, + }); + + // Already in: every commit the variation has is in the lesson's history, so + // there is nothing to fold in. + if (!prepared || prepared.upToDate) { + notify({ + severity: "info", + message: t("messages.variationAlreadyIn", { + name: branchLabel(name), + }), + }); + return; + } + + setMergeIntent("variation"); + setMergeVariation(name); + setMerge(prepared); + } catch (err) { + console.error(err); + notify({ + severity: "error", + message: t("messages.couldNotMerge", { error: err.message || err }), + }); + } finally { + setBusy(null); + } + }; + // Offer this fork's work back to the lesson it came from — as a proposal, not // a write. // @@ -1309,6 +1463,95 @@ export default function EditorPage() { // collaborators they named) reviews and merges from their own editor. That // review step is deliberate: a fork can no longer push itself into someone // else's published lesson, however trusted its owner is. + // The proposal this fork already has open against the lesson it came from, if + // there is one. + // + // Without this, being asked for a change and making it meant opening a *second* + // proposal, leaving the reviewer two overlapping ones and the conversation on + // the wrong one. With it, the button says "update" and does. + // + // Scoped to proposals from *this* fork (`sourceLessonId`), not merely ones by + // this person: proposing to the same lesson from two different forks is a + // legitimate thing to do, and they are not updates of each other. + const [openProposal, setOpenProposal] = useState(null); + + // Bumped on every lookup, so a slow response for a lesson we have since left + // can't win. Without it, switching lessons while a request is in flight can + // leave `openProposal` holding one that belongs to a different fork — and the + // update button would then push this work to that proposal's id. + const proposalLookup = useRef(0); + + const refreshOpenProposal = useCallback(async () => { + const attempt = ++proposalLookup.current; + const current = () => proposalLookup.current === attempt; + + if (!forkedFrom || !editingId || !user?.id || !hasApi()) { + setOpenProposal(null); + return null; + } + try { + const { pulls } = await fetchPullRequests(forkedFrom, accessToken); + const mine = + pulls.find( + (p) => + p.status === "open" && + p.ready && + p.authorId === user.id && + p.sourceLessonId === editingId, + ) || null; + if (!current()) return null; + setOpenProposal(mine); + return mine; + } catch { + // A queue we couldn't read just means the button keeps its "propose" + // wording; opening a second proposal is recoverable, and failing the + // editor over it would not be. + return null; + } + }, [forkedFrom, editingId, user?.id, accessToken]); + + useEffect(() => { + refreshOpenProposal(); + }, [refreshOpenProposal]); + + // Replace what an already-open proposal contains with the work as it stands. + // No dialog: its title and note are already written and this endpoint doesn't + // change them, so there is nothing to ask. + const handleUpdateProposal = async () => { + if (!openProposal) return; + setProposing(true); + try { + await git.commitNow(); + + const engine = await loadGitEngine(); + const updated = await engine.updatePullRequest({ + repoId: git.repoId, + lessonId: forkedFrom, + pullId: openProposal.id, + head: openProposal.head, + accessToken, + }); + setOpenProposal(updated || openProposal); + + notify({ + severity: "success", + message: t("messages.proposalUpdated", { title: openProposal.title }), + route: { + to: `/hub/${forkedFrom}/proposals/${openProposal.id}`, + label: t("labels.viewProposal"), + }, + }); + } catch (err) { + console.error(err); + notify({ + severity: "error", + message: t("messages.couldNotPropose", { error: err.message || err }), + }); + } finally { + setProposing(false); + } + }; + const handleProposeChanges = async ({ title, body }) => { if (!forkedFrom) return; setProposing(true); @@ -1330,6 +1573,7 @@ export default function EditorPage() { }); setProposeOpen(false); + refreshOpenProposal(); notify({ severity: "success", message: t("messages.proposalOpened", { @@ -1356,7 +1600,7 @@ export default function EditorPage() { // meets the commits the two already share, and merged block by block against // the commit they diverged from. Only genuine clashes reach the dialog; the // merge is landed by finishPullMerge once they're settled. - const reviewPullRequest = async (pullId) => { + const reviewPullRequest = async (pullId, { intoVariation = false } = {}) => { if (!editingId) return; setBusy("review"); try { @@ -1375,12 +1619,56 @@ export default function EditorPage() { return; } + // Trying it out first: land the proposal on a variation instead of the + // lesson, so the reviewer can read it in place — click through it, run it, + // show it to somebody — before deciding. The merge below then targets that + // variation, because a merge commits to whatever branch is checked out. + // + // Nothing about the proposal changes. It stays open, and landing it for + // real is still a separate act on the main lesson; this only means the + // reviewer no longer has to choose between merging blind and not merging. + // The merge is computed against a document, and after a branch switch that + // must be the document on the branch being switched *to*. `doc` is this + // render's value and setDoc doesn't change it, so the switched-to document + // is threaded through explicitly — the same shape handleBringVariationIn + // uses, and for the same reason. + let mergeDoc = doc; + + if (intoVariation) { + const name = variationNameFor(pull.title, pull.id); + // Trying the same proposal twice should land back where the first attempt + // put it, not fail on the name being taken. Switching also picks up + // whatever the reviewer did to it last time, which is the point of having + // kept it. + const existing = git.branches.some((b) => b.name === name); + if (existing) { + const next = await git.switchBranch(name); + if (next) { + setDoc(next); + mergeDoc = next; + } + } else { + // A new variation starts at the commit we are already on, so the + // document doesn't move and `doc` is still the right one. + await git.createVariation(name); + } + notify({ + severity: "info", + message: t( + existing + ? "messages.tryingInExistingVariation" + : "messages.tryingInVariation", + { name: branchLabel(name) }, + ), + }); + } + const engine = await loadGitEngine(); const prepared = await engine.preparePullMerge({ repoId: git.repoId, lessonId: editingId, pullId, - doc, + doc: mergeDoc, accessToken, }); if (!prepared) { @@ -1388,16 +1676,28 @@ export default function EditorPage() { return; } - setReviewPull(pull); + // Only a review that is going to *land* the proposal records it as the one + // being reviewed — that is what confirmMerge reads to push and mark it + // merged. A try-out must not: the proposal stays open, waiting for a real + // decision on the lesson itself. + if (!intoVariation) setReviewPull(pull); // We already contain everything it proposes (it was merged some other way, // or it never diverged): there is nothing to settle, so land it as it is. if (prepared.upToDate) { - await finishPullMerge(pull, doc); + if (intoVariation) { + notify({ + severity: "info", + message: t("messages.proposalAlreadyIn"), + }); + return; + } + await finishPullMerge(pull, mergeDoc); return; } - setMergeIntent("pull-request"); + setMergeProposalTitle(pull.title || ""); + setMergeIntent(intoVariation ? "pull-request-try" : "pull-request"); setMerge(prepared); } catch (err) { console.error(err); @@ -1462,20 +1762,47 @@ export default function EditorPage() { prepared: merge, choices, author: identity, - theirName: - mergeIntent === "publish" - ? doc.title - : mergeIntent === "pull-request" - ? reviewPull?.title || t("labels.theProposal") - : forkedFromTitle, + theirName: theirNameFor(mergeIntent), currentDoc: doc, }); setDoc(merged); const intent = mergeIntent; + const variation = mergeVariation; setMerge(null); + setMergeVariation(null); + setMergeProposalTitle(""); + + if (intent === "undo") { + notify({ severity: "success", message: t("messages.undone") }); + git.refreshBranches(); + return; + } + if (intent === "variation") { + notify({ + severity: "success", + message: t("messages.variationBroughtIn", { + name: branchLabel(variation || ""), + }), + }); + git.refreshBranches(); + return; + } + if (intent === "pull-request-try") { + notify({ + severity: "success", + message: t("messages.proposalInVariation", { + name: branchLabel(git.branch), + }), + }); + git.refreshBranches(); + return; + } if (intent === "pull-request") { if (reviewPull) await finishPullMerge(reviewPull, merged); + // A fast-forward leaves no new commit, so the version chip has to be + // told the branch moved. + git.refreshBranches(); return; } if (intent === "publish") { @@ -1530,6 +1857,8 @@ export default function EditorPage() { // the doc changes underneath it. const pullParam = searchParams.get("pull") || ""; const pullLessonParam = searchParams.get("lesson") || ""; + // ?try=1 — land it on a variation to look at rather than on the lesson. + const pullTryParam = searchParams.get("try") === "1"; const reviewPullRef = useRef(reviewPullRequest); const pullHandledRef = useRef(""); useEffect(() => { @@ -1540,8 +1869,15 @@ export default function EditorPage() { if (editingId !== pullLessonParam) return; if (pullHandledRef.current === pullParam) return; pullHandledRef.current = pullParam; - reviewPullRef.current(pullParam); - }, [pullParam, pullLessonParam, editingId, git.ready, accessToken]); + reviewPullRef.current(pullParam, { intoVariation: pullTryParam }); + }, [ + pullParam, + pullLessonParam, + pullTryParam, + editingId, + git.ready, + accessToken, + ]); // Word import. We warn first (the conversion is lossy and can fail), then open // the file picker; the chosen file is parsed and validated by importDocxFile, @@ -2145,6 +2481,42 @@ export default function EditorPage() { )} + {/* Which copy of the lesson is being edited. On the main lesson + this is a quiet chip that mostly exists to say the feature is + there; on a variation it is the reminder that what you change + isn't what people are reading. */} + {git.ready && ( + + + + + + {git.onDefaultBranch + ? t("documentPanel.variationsTooltip") + : t("documentPanel.onVariationTooltip")} + + + )} + {editingId && ( @@ -2189,25 +2561,38 @@ export default function EditorPage() { {/* Offer this fork's work back to the lesson it came from. It goes as a proposal for that lesson's author (or a trusted - collaborator) to review — a fork never writes the original. */} + collaborator) to review — a fork never writes the original. + Once one is open, the same button updates it rather than + opening a second one about the same work. */} {forkedFrom && canPropose && hasApi() && ( - {t("documentPanel.proposeTooltip", { - name: forkedFromTitle || t("labels.theOriginalLesson"), - })} + {openProposal + ? t("documentPanel.updateProposalTooltip", { + title: openProposal.title, + }) + : t("documentPanel.proposeTooltip", { + name: + forkedFromTitle || t("labels.theOriginalLesson"), + })} )} @@ -2541,12 +2926,23 @@ export default function EditorPage() { onTrustedChange={setTrustedCollaborators} /> + {/* Variations: the other branches of this lesson's repository, as an author + sees them — separate copies to try things in. */} + openPanel(null)} + git={git} + onSwitch={(next) => next && setDoc(next)} + onBringIn={handleBringVariationIn} + /> + {/* The lesson's own version history, read out of its git repository. */} openPanel(null)} git={git} onRestore={setDoc} + onUndo={handleUndoCommit} /> {/* Settling a merge with the lesson this one was forked from. Only blocks @@ -2570,17 +2966,13 @@ export default function EditorPage() { // to be landed and land it. onClose={() => { setMerge(null); + setMergeVariation(null); + setMergeProposalTitle(""); setReviewPull(null); }} prepared={merge} intent={mergeIntent} - theirName={ - mergeIntent === "publish" - ? t("labels.theSavedLesson") - : mergeIntent === "pull-request" - ? reviewPull?.title || t("labels.theProposal") - : forkedFromTitle || t("labels.theOriginal") - } + theirName={theirNameFor(mergeIntent, { dialog: true })} proposerName={reviewPull?.author || ""} onConfirm={confirmMerge} busy={merging} diff --git a/apps/web/src/pages/lesson/LessonProposal.jsx b/apps/web/src/pages/lesson/LessonProposal.jsx index 65f5c43..f0bf42c 100644 --- a/apps/web/src/pages/lesson/LessonProposal.jsx +++ b/apps/web/src/pages/lesson/LessonProposal.jsx @@ -1,12 +1,28 @@ // One proposal's own page (/hub/:id/proposals/:prId). // -// Read-only, and that is not a shortcut. Merging a proposal is a genuine -// three-way merge against the lesson's git history, and that history lives in -// the editor's browser-side repository (LightningFS + isomorphic-git, loaded on -// demand) — not here, on a page whose whole point is to be cheap enough to -// server-render for a reader. So "Review & merge" does what it has always done: -// hands the editor the lesson and the proposal id, and the block-by-block merge -// happens there, where the objects are. +// You can **read** a proposal here — what it changes, block by block, and whether +// it would merge cleanly — but you cannot merge one. That split is the point, and +// it is not the same as the page being read-only for want of trying. +// +// Merging is the reviewer's act: it commits to the lesson's history and pushes it +// under their credentials, which needs the editor's repository. Reading needs none +// of that. Both packs are public exactly as far as the lesson is, so the diff is +// computed right here from the objects themselves — index the proposal's pack +// beside the lesson's, find the commit the two diverged at, and diff. See +// prepareProposalReview in core/browser/git/sync.js. +// +// So "Review & merge" still hands the editor the lesson and the proposal id, and +// the block-by-block merge still happens there. What changed is that nobody has +// to start that merge to find out whether they want it. +// +// There is a third answer between yes and no, too: **try it in a variation** +// (`&try=1`). A diff says what changed; it doesn't say whether the lesson still +// works with the change in it. That route lands the proposal on a variation of +// the reviewer's own, leaves the lesson alone, and leaves the proposal open. +// +// The engine is ~200 KB and is fetched on demand (lib/git/load.js) only once this +// page is open — the same arrangement the History tab uses, and the reason the +// diff arrives after the proposal's text rather than with it. // // There is no single-proposal endpoint (see apps/api/src/routes/pulls.js), so // this reads the lesson's list and picks its one out. The list is small by @@ -19,18 +35,27 @@ import { useTranslation } from "react-i18next"; import { Link as RouterLink, useNavigate, useParams } from "react-router-dom"; import { ArrowLeftIcon, + CircleCheckIcon, + GitBranchIcon, GitMergeIcon, GitPullRequestClosedIcon, GitPullRequestIcon, + TriangleAlertIcon, } from "lucide-react"; import PageBody from "../../components/layout/PageBody.jsx"; +import { ChangeChips, ChangeList } from "../../components/ChangeSummary.jsx"; import { Button } from "../../components/ui/button.jsx"; import { Badge } from "../../components/ui/badge.jsx"; import { Alert, AlertDescription } from "../../components/ui/alert.jsx"; import { Avatar, AvatarFallback } from "../../components/ui/avatar.jsx"; -import { ListRowsSkeleton } from "../../components/Skeletons.jsx"; +import { + HistorySkeleton, + ListRowsSkeleton, +} from "../../components/Skeletons.jsx"; import { cn } from "../../lib/utils.js"; import { useAuth } from "../../lib/auth.jsx"; +import { loadGitEngine } from "../../lib/git/load.js"; +import { repoIdFor } from "@spelling-creator/core/git/doc"; import { fetchPullRequests } from "@spelling-creator/core/pulls"; import { EDIT_REQUEST_KEY } from "@spelling-creator/core/lessons"; import { useLesson } from "./LessonLayout.jsx"; @@ -40,6 +65,50 @@ function initial(name) { return s ? s[0].toUpperCase() : "?"; } +/** + * Whether this would merge on its own, said plainly. + * + * The three answers are genuinely different actions for a reviewer, not shades of + * one: nothing to do, press the button, or set aside ten minutes. A count of + * conflicting blocks is the honest measure of the last, because that is exactly + * how many decisions the merge dialog will ask for. + */ +function Mergeability({ changes }) { + const { t } = useTranslation("lesson"); + + if (changes.contained) { + return ( + + + + {t("pulls.changes.alreadyIn")} + + + ); + } + + const count = changes.conflicts.length; + if (count === 0) { + return ( + + + + {t("pulls.changes.clean")} + + + ); + } + + return ( + + + + {t("pulls.changes.conflicts", { count })} + + + ); +} + export default function LessonProposal() { const { t } = useTranslation("lesson"); const { prId } = useParams(); @@ -74,18 +143,65 @@ export default function LessonProposal() { }; }, [lesson.id, prId, accessToken, t]); + // What the proposal changes, worked out from the git objects themselves. + // + // Deliberately a second effect rather than part of the fetch above: it needs a + // 200 KB engine and two packfile downloads, and the proposal's title, author and + // note should be on screen long before any of that lands. It also fails softly — + // a proposal whose changes can't be read is still a proposal worth showing, and + // the reviewer can always open it in the editor. + // + // It waits for `ready`, because an unready proposal has no pack to read. + const [changes, setChanges] = useState(null); // null = still working + const [changesError, setChangesError] = useState(""); + const ready = pull?.ready; + + useEffect(() => { + if (!ready) return; + let cancelled = false; + + setChanges(null); + setChangesError(""); + + (async () => { + try { + const engine = await loadGitEngine(); + const result = await engine.prepareProposalReview({ + repoId: repoIdFor(lesson.id), + lessonId: lesson.id, + pullId: prId, + previousHead: pull.previousHead, + accessToken, + }); + if (cancelled) return; + if (!result) { + setChangesError(t("pulls.changesGone")); + return; + } + setChanges(result); + } catch (err) { + if (!cancelled) setChangesError(err.message || t("pulls.changesError")); + } + })(); + + return () => { + cancelled = true; + }; + }, [lesson.id, prId, ready, pull?.previousHead, accessToken, t]); + // Same hand-off as the list's "Review & merge" — see the file header. The // lesson id goes in the query string as well as sessionStorage because the // editor may already have a *different* lesson open, and a proposal is only // meaningful against the lesson it was opened on. - const review = () => { + const review = ({ tryIt = false } = {}) => { try { sessionStorage.setItem(EDIT_REQUEST_KEY, lesson.id); } catch { /* ignore — the editor just won't preload if storage is unavailable */ } navigate( - `/editor?pull=${encodeURIComponent(prId)}&lesson=${encodeURIComponent(lesson.id)}`, + `/editor?pull=${encodeURIComponent(prId)}&lesson=${encodeURIComponent(lesson.id)}` + + (tryIt ? "&try=1" : ""), ); }; @@ -172,6 +288,20 @@ export default function LessonProposal() {

+ {/* A proposal can be updated while it is open, and a reviewer coming back to + one needs to know that happened before they read it again. */} + {(pull.revision > 1 || pull.headRef) && ( +

+ {pull.revision > 1 && + t("pulls.revision", { + n: pull.revision, + when: pull.updatedAt ? formatDate(pull.updatedAt) : "", + })} + {pull.revision > 1 && pull.headRef ? " · " : ""} + {pull.headRef && t("pulls.fromVariation", { name: pull.headRef })} +

+ )} + {pull.body && (

{pull.body}

@@ -189,15 +319,71 @@ export default function LessonProposal() {

)} + {/* What it changes, and whether it would land cleanly. Drawn for a closed + or merged proposal too — "what did that one do?" is a question people + ask most often about the ones already resolved. */} + {pull.ready && ( +
+

+ {t("pulls.changes.heading")} +

+ + {changesError ? ( +

{changesError}

+ ) : changes === null ? ( + + ) : changes.ops.length === 0 ? ( +

+ {t("pulls.changes.none")} +

+ ) : ( + <> + +
+ + + )} + + {/* What the most recent update did, for a reviewer who has read this + before. Only drawn when there has been one and its commits are + still readable. */} + {changes?.updateOps?.length > 0 && ( +
+

+ {t("pulls.changes.sinceUpdate")} +

+ + +
+ )} + + {/* Whether a reviewer would have anything to decide. Worth saying + before they open the editor, because the usual answer is "no". */} + {changes && isOpen && } +
+ )} + {isOpen && pull.ready && canReview && (
- +
+ + {/* The middle option, which used to not exist: put it somewhere you + can look at it. Reading a diff tells you what changed; it doesn't + tell you whether the lesson still works. */} + +

{t("pulls.reviewHint")}

+

+ {t("pulls.tryItHint")} +

)} diff --git a/packages/core/package.json b/packages/core/package.json index c4da43f..a245ce4 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -35,6 +35,7 @@ "./git/merge": "./src/git/merge.js", "./git/ops": "./src/git/ops.js", "./git/pack": "./src/git/pack.js", + "./git/refs": "./src/git/refs.js", "./git/remote": "./src/git/remote.js", "./git/repo": "./src/git/repo.js", "./id": "./src/id.js", diff --git a/packages/core/src/browser/git/sync.js b/packages/core/src/browser/git/sync.js index 9f4df1a..5c5a76f 100644 --- a/packages/core/src/browser/git/sync.js +++ b/packages/core/src/browser/git/sync.js @@ -32,6 +32,7 @@ import { newId } from "../../id.js"; import { preserveLocalFields } from "../../git/doc.js"; import { DRAFT_REPO, copyRepo, deleteRepo, repoCtx } from "./fs.js"; import { applyResolutions, mergeDocs } from "../../git/merge.js"; +import { diffDocs } from "../../git/ops.js"; import { cloneFromPack, contains, @@ -47,9 +48,15 @@ import { uploadPullPack, } from "../../pulls.js"; import { + BRANCH, ORIGIN_REF, UPSTREAM_REF, + branchRef, + clearDeletedBranch, commitDoc, + currentBranch, + currentBranchRef, + deletedBranches, headOid, pullRef, readDocAt, @@ -73,17 +80,59 @@ const EMPTY_AUTO = { merged: [], tookTheirs: [], added: [], removed: [] }; */ export async function remoteStatus({ repoId, lessonId, ref = ORIGIN_REF }) { const ctx = repoCtx(repoId); + const branch = await currentBranch(ctx); const ours = await headOid(ctx); const pack = await fetchPack(lessonId).catch(() => null); - if (!pack) return { state: "fresh", ours, theirs: null, needsMerge: false }; + if (!pack) { + return { + state: "fresh", + ours, + theirs: null, + branch, + hasRemote: false, + needsMerge: false, + }; + } - await fetchRemotePack({ ...ctx, ...pack, ref }); - const theirs = pack.head; + await fetchRemotePack({ + ...ctx, + ...pack, + ref, + refs: pack.refs, + remote: "origin", + }); + await syncRemoteBranches(ctx, pack.refs); + + // Compare like with like: the branch we are on against the hub's copy of *that* + // branch, not against the lesson. A variation and the lesson are supposed to + // differ — that is what a variation is — so measuring one against the other + // would report a conflict on every save. + const theirs = pack.refs?.[branch] || (branch === BRANCH ? pack.head : null); + + // The hub has never seen this branch, so there is nothing there to overwrite. + if (!theirs) { + return { + state: "fresh", + ours, + theirs: null, + branch, + hasRemote: true, + needsMerge: false, + }; + } // Nothing committed locally: whatever the hub has, take it. if (!ours) { - return { state: "behind", ours, theirs, base: null, needsMerge: true }; + return { + state: "behind", + ours, + theirs, + base: null, + branch, + hasRemote: true, + needsMerge: true, + }; } if (ours === theirs) { return { @@ -91,6 +140,8 @@ export async function remoteStatus({ repoId, lessonId, ref = ORIGIN_REF }) { ours, theirs, base: theirs, + branch, + hasRemote: true, needsMerge: false, }; } @@ -99,13 +150,97 @@ export async function remoteStatus({ repoId, lessonId, ref = ORIGIN_REF }) { // We already contain their tip, so pushing can only move the lesson forward. if (await contains({ ...ctx, oid: ours, ancestor: theirs })) { - return { state: "ahead", ours, theirs, base, needsMerge: false }; + return { + state: "ahead", + ours, + theirs, + base, + branch, + hasRemote: true, + needsMerge: false, + }; } // They contain ours: someone pushed commits we don't have. if (await contains({ ...ctx, oid: theirs, ancestor: ours })) { - return { state: "behind", ours, theirs, base, needsMerge: true }; + return { + state: "behind", + ours, + theirs, + base, + branch, + hasRemote: true, + needsMerge: true, + }; + } + return { + state: "diverged", + ours, + theirs, + base, + branch, + hasRemote: true, + needsMerge: true, + }; +} + +/** + * Bring our idea of the hub's branches in line with what it just told us. + * + * Two halves, and they are both about the next push being able to say something + * true. Creating local branches for the hub's is what makes a variation started + * on one device turn up on another — the objects are already here, off the pack + * we have just indexed, so it costs a ref write. Dropping remote-tracking refs + * for branches the hub no longer has is what stops us claiming a tip for a branch + * that isn't there, which the compare-and-swap would refuse for ever. + * + * A branch we deliberately deleted is not adopted back: its marker is still + * waiting to be pushed, and undoing a delete on the way to reporting it would be + * the opposite of what the author asked for. + * + * Every write here is best-effort. This is bookkeeping so that the *next* push can + * say something true, and one branch that won't adopt is no reason to fail the + * save it is riding along with — including the lesson's own. + */ +async function syncRemoteBranches(ctx, refs) { + if (!refs) return; + const deleted = await deletedBranches(ctx); + + for (const [name, oid] of Object.entries(refs)) { + if (!oid || deleted[name]) continue; + if (await headOid({ ...ctx, ref: branchRef(name) })) continue; + await git + .writeRef({ ...ctx, ref: branchRef(name), value: oid, force: false }) + .catch(() => {}); + } + + // The other direction: a branch the hub no longer has, which we do. + // + // Dropping only the remote-tracking ref would leave the local branch to be + // pushed back as new on the next save — so a variation deleted on one device + // would quietly reappear, put there by another device that still had it. What + // makes the delete safe is the tracking ref itself: it is the last thing the hub + // told us this branch was, so a local branch still sitting exactly there holds + // nothing that isn't already gone. One that has moved holds unpushed work, and + // that is the author's to keep — it goes back up, and they can delete it again. + const tracked = await git + .listRefs({ ...ctx, filepath: "refs/remotes/origin" }) + .catch(() => []); + + for (const name of tracked) { + if (refs[name]) continue; + + const trackedOid = await headOid({ + ...ctx, + ref: `refs/remotes/origin/${name}`, + }); + const localOid = await headOid({ ...ctx, ref: branchRef(name) }); + if (name !== BRANCH && localOid && localOid === trackedOid) { + await git.deleteRef({ ...ctx, ref: branchRef(name) }).catch(() => {}); + } + await git + .deleteRef({ ...ctx, ref: `refs/remotes/origin/${name}` }) + .catch(() => {}); } - return { state: "diverged", ours, theirs, base, needsMerge: true }; } /** @@ -131,10 +266,11 @@ export async function prepareMerge({ lessonId, doc, ref = UPSTREAM_REF, + theirs, }) { const pack = await fetchPack(lessonId).catch(() => null); if (!pack) return null; - return mergeAgainstPack({ repoId, pack, doc, ref }); + return mergeAgainstPack({ repoId, pack, doc, ref, theirs }); } /** @@ -145,13 +281,29 @@ export async function prepareMerge({ * The objects land in our own store, so the commits the two sides share are * literally the same objects and the merge base below is a real answer. */ -async function mergeAgainstPack({ repoId, pack, doc, ref }) { - const ctx = repoCtx(repoId); +async function mergeAgainstPack({ repoId, pack, doc, ref, theirs }) { + await fetchRemotePack({ ...repoCtx(repoId), ...pack, ref }); + // `theirs` names which of the pack's commits to merge. It defaults to the + // lesson itself, which is what pulling an original's changes or reviewing a + // proposal means — but catching up with our own hub while editing a variation + // has to merge the hub's copy of *that* variation, or we would fold the whole + // lesson into the variation and call it a sync. + return mergeAgainstCommit({ repoId, theirs: theirs || pack.head, doc }); +} - await fetchRemotePack({ ...ctx, ...pack, ref }); +/** + * Merge one commit into the branch we are on — the part of a merge that has + * nothing to do with where the other side came from. + * + * Downloaded from the hub, unpacked from a proposal, or simply another branch of + * this same repository: by the time we are here it is an oid whose objects we + * hold, and the answer is the same three-way merge against the commit the two + * sides last agreed on. + */ +async function mergeAgainstCommit({ repoId, theirs, doc }) { + const ctx = repoCtx(repoId); const ours = await headOid(ctx); - const theirs = pack.head; if (!ours) return null; const identical = ours === theirs; @@ -182,6 +334,18 @@ async function mergeAgainstPack({ repoId, pack, doc, ref }) { // uncommitted edits must not be silently dropped by a merge. const result = mergeDocs(baseDoc, doc, theirDoc); + // A fast-forward: their history already contains ours (we are the merge base), + // and we have nothing of our own on top — no commits, and nothing uncommitted + // in the editor either. Then "merging" is only moving our branch to theirs, and + // manufacturing a merge commit for it would put an entry in the lesson's + // timeline that records no decision and changes no content. + // + // Both halves matter. `base === ours` is the commit-graph half; diffing the + // live document against ours is the half the graph can't see, and skipping it + // would drop whatever the reviewer had typed but not yet paused long enough to + // commit. + const fastForward = base === ours && diffDocs(baseDoc, doc).length === 0; + return { doc: preserveLocalFields(result.doc, doc), conflicts: result.conflicts, @@ -191,10 +355,119 @@ async function mergeAgainstPack({ repoId, pack, doc, ref }) { base, identical: false, ahead: false, + fastForward, + // The branch this merge is *for*. A fast-forward moves a ref, and it must move + // the one the merge was computed against — not whatever HEAD happens to point + // at when the user finally confirms, which a variation switch in between would + // have changed. + ref: await currentBranchRef(ctx), upToDate: false, }; } +/** + * Undo one commit, leaving everything since it alone. + * + * `restoreCommit` (repo.js) is the blunt instrument: it takes the whole document + * back to a point and drops every change after it. This is the precise one — undo + * *that* change, keep the rest — and it is the same three-way merge as everything + * else here, with the sides pointed backwards: + * + * base the document as that commit left it + * ours the document now + * theirs the document as it was immediately before + * + * Every rule then falls out correctly without a line of new logic. A block the + * commit changed differs between base and theirs, so theirs wins and it goes + * back. A block changed since differs between base and ours, so ours wins and is + * kept. A block in both is a genuine conflict — the change being undone has been + * built on, and only the author can say what they meant — so it reaches the usual + * dialog. + * + * The result is an ordinary forward commit with one parent, like a restore: + * history is never rewritten, so an undo can itself be undone. + * + * @returns {Promise} A prepared merge, for completeMerge(). + */ +export async function prepareRevert({ repoId, oid, doc }) { + const ctx = repoCtx(repoId); + + const { commit } = await git.readCommit({ ...ctx, oid }); + const parent = commit.parent[0] || null; + // The very first commit has nothing before it, so there is no "before" to put + // back — undoing it would mean emptying the lesson, which is a different + // request and not this one. + if (!parent) { + throw new Error( + "This is where the lesson starts — there is nothing before it to go back to.", + ); + } + + const ours = await headOid(ctx); + if (!ours) throw new Error("There is no history to undo."); + + const [afterDoc, beforeDoc] = await Promise.all([ + readDocAt({ ...ctx, oid }), + readDocAt({ ...ctx, oid: parent }), + ]); + + const result = mergeDocs(afterDoc, doc, beforeDoc); + const summary = commit.message.split("\n")[0].trim(); + + return { + doc: preserveLocalFields(result.doc, doc), + conflicts: result.conflicts, + auto: result.auto, + ours, + theirs: oid, + base: oid, + identical: false, + ahead: false, + upToDate: false, + // One parent: this is a new change that happens to reverse an old one, not a + // join of two histories. Two would claim the reverted commit was being + // merged in, which is the opposite of what happened. + parents: [ours], + // Carried so the dialog and the toast can name the change being undone + // without reading the commit again. + summary, + message: `Undo "${summary}"\n\nReverses ${oid.slice(0, 7)}, keeping everything changed since.\n`, + }; +} + +/** + * Bring a variation into the lesson: the same three-way merge, with both sides + * already in this repository. + * + * The switch happens *first*, and that ordering is the whole of it. A merge + * commits to the branch you are standing on, so we move to the lesson before + * preparing anything, and what comes back is the lesson's document with the + * variation merged into it — not the other way round. Pending edits are committed + * to the variation on the way out (checkoutBranch's caller does that), so nothing + * in progress is carried across by accident. + * + * Feed the result to completeMerge(), exactly as with a merge from the hub. + * + * @param {string} args.name The variation to bring in. + * @param {string} [args.into] The branch to bring it into. The lesson by default. + * @returns {Promise} The prepared merge, or null when the variation + * is already contained in the target — there is nothing to bring in. + */ +export async function prepareBranchMerge({ repoId, name, into = BRANCH, doc }) { + const ctx = repoCtx(repoId); + + const theirs = await headOid({ ...ctx, ref: branchRef(name) }); + if (!theirs) throw new Error("That version no longer exists."); + + const ours = await headOid({ ...ctx, ref: branchRef(into) }); + // Already in: the lesson's history contains every commit the variation has, so + // a merge would produce a commit that changes nothing. + if (ours && (await contains({ ...ctx, oid: ours, ancestor: theirs }))) { + return null; + } + return mergeAgainstCommit({ repoId, theirs, doc }); +} + /** * Finish a merge once the user has settled any conflicts, recording it as a * commit with *two* parents — ours and theirs. That's what joins the two @@ -220,12 +493,35 @@ export async function completeMerge({ ); const doc = preserveLocalFields(resolved, currentDoc); + // A fast-forward has nothing to record. Their history already contains ours and + // we added nothing to it, so the merge is our branch moving to their commit — + // and writing a merge commit instead would leave a permanent entry in the + // lesson's timeline saying a decision was made when none was. + // + // The conflicts guard is belt and braces: mergeAgainstCommit can't produce both, + // because a merge whose base is ours takes their side of everything. But the + // flag arrives here from a prepared object the caller has held across a dialog, + // and silently discarding somebody's conflict resolutions would be the worst + // possible way to find out that assumption had stopped holding. + if (prepared.fastForward && prepared.conflicts.length === 0) { + // Against the branch it was prepared for, and only if we are still on it. + // Anything else means the ground moved under the dialog, and moving a ref + // somebody has since navigated away from is worse than making a merge commit. + const ref = prepared.ref || (await currentBranchRef(ctx)); + if (ref === (await currentBranchRef(ctx))) { + await git.writeRef({ ...ctx, ref, value: prepared.theirs, force: true }); + return doc; + } + } + await commitDoc({ ...ctx, doc, author, - message: mergeSummary(prepared, choices, theirName), - parents: [prepared.ours, prepared.theirs], + // An undo brings its own wording and its own shape; anything else is a join + // of two histories and gets the two parents that say so. + message: prepared.message || mergeSummary(prepared, choices, theirName), + parents: prepared.parents || [prepared.ours, prepared.theirs], }); return doc; @@ -244,35 +540,86 @@ export async function completeMerge({ export async function pushHistory({ repoId, lessonId, doc, accessToken }) { const status = await remoteStatus({ repoId, lessonId, ref: ORIGIN_REF }); - if (status.state === "identical") return { pushed: false, status }; - if (status.needsMerge) { const prepared = await prepareMerge({ repoId, lessonId, doc, ref: ORIGIN_REF, + // The hub's copy of the branch we are on, which remoteStatus has just + // resolved — not the lesson's tip, unless they are the same thing. + theirs: status.theirs, }); return { pushed: false, needsMerge: true, prepared, status }; } - const packed = await packRepo(repoCtx(repoId)); + const ctx = repoCtx(repoId); + const packed = await packRepo(ctx); if (!packed) return { pushed: false, status }; // nothing committed yet + // What the hub held when remoteStatus fetched a moment ago, which is what the + // per-branch compare-and-swap is against. Read from the remote-tracking refs + // that fetch wrote rather than kept in a variable, so it is the same answer the + // objects in our store came with. + const remote = status.hasRemote ? await remoteBranches(ctx) : {}; + const deleted = status.hasRemote ? await deletedBranches(ctx) : {}; + + // "Nothing to do" is asked of the whole repository, not of the branch being + // edited. Asking only about that one would strand the others: deleting a + // variation, or merging one into the lesson from somewhere else, changes what + // the hub should hold without moving the branch we happen to be standing on. + const settled = + Object.keys(deleted).length === 0 && + Object.keys(packed.refs).length === Object.keys(remote).length && + Object.entries(packed.refs).every(([name, oid]) => remote[name] === oid); + if (settled) return { pushed: false, status }; + + // Say what we believe about every branch we are touching. A branch we hold that + // the hub doesn't gets "" — "I believe this is new" — which is the claim that + // fails if somebody else created the same name in the meantime. + const expected = {}; + for (const name of Object.keys(packed.refs)) + expected[name] = remote[name] || ""; + for (const [name, oid] of Object.entries(deleted)) expected[name] = oid; + await pushPack( lessonId, { packfile: packed.packfile, head: packed.head, - // The compare-and-swap: null on a lesson with no history yet, otherwise the - // tip we just confirmed we contain. - parent: status.theirs, + // The compare-and-swap for the lesson itself: null on a lesson with no + // history yet, otherwise the tip we just confirmed we contain. + parent: remote[BRANCH] || null, + refs: packed.refs, + expected, + deletes: Object.keys(deleted), }, accessToken, ); + + // The deletions have landed, so stop asking for them. Only now: a marker + // cleared before the push succeeded would leave a variation deleted here and + // alive on the hub, ready to come back on the next device that clones. + for (const name of Object.keys(deleted)) { + await clearDeletedBranch({ ...ctx, name }); + } return { pushed: true, status }; } +/** The hub's branches as of our last fetch, from the remote-tracking refs. */ +async function remoteBranches(ctx) { + const names = await git + .listRefs({ ...ctx, filepath: "refs/remotes/origin" }) + .catch(() => []); + + const out = {}; + for (const name of names) { + const oid = await headOid({ ...ctx, ref: `refs/remotes/origin/${name}` }); + if (oid) out[name] = oid; + } + return out; +} + /** * Offer this fork's work back to the lesson it came from, as a pull request. * @@ -298,7 +645,19 @@ export async function submitPullRequest({ sourceLessonId = null, accessToken, }) { - const packed = await packRepo(repoCtx(repoId)); + const ctx = repoCtx(repoId); + + // The branch the proposer is actually looking at, and only that one. + // + // Both halves matter. Offering *the branch you are on* is the only reading of + // "propose these changes" that isn't a trap: someone who worked up their idea + // on a variation and then proposed the untouched lesson would have sent nothing, + // and been told it succeeded. And offering *only* that branch is what keeps the + // rest to themselves — a variation is an idea its author is still turning over, + // and handing somebody else half a dozen of them to merge is not what the button + // says. + const branch = await currentBranch(ctx); + const packed = await packRepo({ ...ctx, only: [branch], headBranch: branch }); if (!packed) { throw new Error("There is nothing to propose yet — make an edit first."); } @@ -314,6 +673,9 @@ export async function submitPullRequest({ title, body, head: packed.head, + // Named so the review queue can say which version of the fork this is. The + // default branch is the fork itself and needs no saying. + headRef: branch === BRANCH ? null : branch, base: refs?.head || null, sourceLessonId, }, @@ -334,6 +696,53 @@ export async function submitPullRequest({ } } +/** + * Replace what an open proposal contains with the work as it now stands. + * + * The alternative was closing it and opening another, which threw away the + * conversation attached to it — so this exists for the ordinary case of being + * asked for a change and making it. + * + * A proposal may only move **forward**: the new tip has to contain the one the + * proposal already points at. That is what keeps one pack per proposal honest — + * the previous revision's commit is still reachable in the new pack, so "what + * changed in this update" stays answerable — and it is checked here because the + * Worker holds a proposal's history as an opaque packfile and cannot walk it. + * + * @returns {Promise} The updated proposal. + */ +export async function updatePullRequest({ + repoId, + lessonId, + pullId, + head, + accessToken, +}) { + const ctx = repoCtx(repoId); + + const branch = await currentBranch(ctx); + const packed = await packRepo({ ...ctx, only: [branch], headBranch: branch }); + if (!packed) { + throw new Error("There is nothing to propose yet — make an edit first."); + } + if (packed.head === head) { + throw new Error("This proposal already contains everything you have here."); + } + if (!(await contains({ ...ctx, oid: packed.head, ancestor: head }))) { + throw new Error( + "This doesn’t build on what the proposal already contains — it would replace it rather than update it. " + + "Withdraw the proposal and open a new one.", + ); + } + + return uploadPullPack( + lessonId, + pullId, + { packfile: packed.packfile, head: packed.head }, + accessToken, + ); +} + /** * Merge a pull request's proposed changes into this lesson — the reviewer's side * of the flow, and the mirror of prepareMerge. @@ -366,6 +775,99 @@ export async function preparePullMerge({ return mergeAgainstPack({ repoId, pack, doc, ref: pullRef(pullId) }); } +/** + * What a proposal changes, and whether it would merge cleanly — computed without + * an editor, so a proposal can be *read* on its own page. + * + * Merging a proposal needs the reviewer's editor, because the merge is theirs to + * make and to push. Reading one doesn't, and the difference matters: the whole + * queue was previously a list of titles nobody could look inside without opening + * the lesson and starting a merge they might not want. + * + * Both packs are public exactly as far as the lesson is (a proposal is as + * readable as what it targets), so this works for any viewer, signed in or not. + * It answers two questions: + * + * ops what the proposer changed, as blocks, against the commit the two + * histories diverged at — a proposal's diff, in the same shape the + * history view renders + * conflicts which blocks the lesson and the proposal have both changed in the + * same field, i.e. what a reviewer would actually have to decide + * + * Nothing is committed and no branch moves; the only writes are objects and a + * remote-tracking ref, which is what indexing a pack means. + * + * @returns {Promise} null when the proposal's changes are no longer stored. + */ +export async function prepareProposalReview({ + repoId, + lessonId, + pullId, + previousHead, + accessToken, +}) { + const ctx = repoCtx(repoId); + + // The lesson's own history, so there is something to compare against. A lesson + // with none (one written before version history) still gets a diff below — just + // against nothing, which reads as "everything in it is new", and is true. + const lessonPack = await fetchPack(lessonId).catch(() => null); + if (lessonPack) { + await fetchRemotePack({ ...ctx, ...lessonPack, ref: ORIGIN_REF }); + } + + const pack = await fetchPullPack(lessonId, pullId, accessToken); + if (!pack) return null; + await fetchRemotePack({ ...ctx, ...pack, ref: pullRef(pullId) }); + + const theirs = pack.head; + const ours = lessonPack?.head || null; + + const base = ours ? await mergeBase({ ...ctx, ours, theirs }) : null; + // Already landed: every commit the proposal has is in the lesson's history. + const contained = ours + ? await contains({ ...ctx, oid: ours, ancestor: theirs }) + : false; + + const [baseDoc, ourDoc, theirDoc] = await Promise.all([ + base ? readDocAt({ ...ctx, oid: base }) : Promise.resolve(null), + ours ? readDocAt({ ...ctx, oid: ours }) : Promise.resolve(null), + readDocAt({ ...ctx, oid: theirs }), + ]); + + // Against the merge base where there is one — that is a proposal's diff, and it + // shows what the proposer did rather than every way the two now differ. Without + // shared ancestry there is no such point, so fall back to the lesson as it + // stands, which is the question a reader is really asking anyway. + const ops = diffDocs(baseDoc || ourDoc, theirDoc); + const merged = ourDoc ? mergeDocs(baseDoc, ourDoc, theirDoc) : null; + + // What the proposer's most recent update changed, when there has been one. The + // commit it moved from is still in this pack — a proposal may only move forward + // — so this costs a tree read rather than another download. It is best-effort: + // a proposal from before updates existed has no previous head to read, and a + // reader who can see the current changes shouldn't lose them over that. + let updateOps = null; + if (previousHead && previousHead !== theirs) { + updateOps = await readDocAt({ ...ctx, oid: previousHead }) + .then((previousDoc) => diffDocs(previousDoc, theirDoc)) + .catch(() => null); + } + + return { + ops, + updateOps, + conflicts: merged?.conflicts || [], + auto: merged?.auto || EMPTY_AUTO, + base, + ours, + theirs, + contained, + hasLesson: Boolean(ours), + }; +} + /** * Fork a lesson: clone its published repository into the local draft repo. * @@ -385,7 +887,11 @@ export async function forkLessonRepo(sourceLessonId) { await deleteRepo(DRAFT_REPO); const ctx = repoCtx(DRAFT_REPO); - await cloneFromPack({ ...ctx, ...pack }); + // The lesson, and only the lesson. Its author's variations came down in the same + // pack — they are in it so that *they* can reach them from another device — but + // somebody else's half-finished ideas are not part of what was forked, and + // adopting them as branches of the fork would say they were. + await cloneFromPack({ ...ctx, ...pack, refs: null }); // Record where we came from, so the first sync has a base even before it // fetches anything new. diff --git a/packages/core/src/git/branches.test.js b/packages/core/src/git/branches.test.js new file mode 100644 index 0000000..b42f4f3 --- /dev/null +++ b/packages/core/src/git/branches.test.js @@ -0,0 +1,265 @@ +// Branches — the thing an author sees as "another version of this lesson I'm +// trying out". +// +// These drive the real engine over the in-memory filesystem, for the reason +// memfs.test.js does: the contract that matters is not "we wrote a ref" but +// "isomorphic-git agrees, a pack carries it, and a clone gets it back". The +// round trip below is exactly the one a second device makes. + +import { describe, expect, it } from "vitest"; +import { memRepo } from "./memfs.js"; +import { cloneFromPack, packRepo } from "./pack.js"; +import { + branchLabel, + isBranchName, + parseRefMap, + toBranchName, +} from "./refs.js"; +import { + BRANCH, + checkoutBranch, + commitDoc, + createBranch, + currentBranch, + deleteBranch, + deletedBranches, + headOid, + history, + listBranches, + renameBranch, +} from "./repo.js"; + +const author = { name: "Test", email: "test@example.com" }; + +function doc(title, text) { + return { + title, + sections: [ + { id: "s1", name: "One", blocks: [{ id: "b1", type: "text", text }] }, + ], + }; +} + +/** A repo with one commit on the default branch. */ +async function seeded() { + const ctx = memRepo(); + await commitDoc({ ...ctx, doc: doc("Lesson", "first"), author }); + return ctx; +} + +describe("branch names", () => { + it("turns what someone typed into something git will store", () => { + expect(toBranchName("Simpler for Year 3")).toBe("Simpler-for-Year-3"); + expect(toBranchName(" spaced out ")).toBe("spaced-out"); + expect(toBranchName("emoji ✨ and ?marks")).toBe("emoji-and-marks"); + }); + + it("reads a stored name back as the words it came from", () => { + expect(branchLabel(toBranchName("Simpler for Year 3"))).toBe( + "Simpler for Year 3", + ); + }); + + it("refuses the names git refuses, and anything with nothing left in it", () => { + expect(toBranchName("???")).toBe(""); + expect(toBranchName(" ")).toBe(""); + expect(isBranchName("a..b")).toBe(false); + expect(isBranchName("thing.lock")).toBe(false); + expect(isBranchName("-leading")).toBe(false); + expect(isBranchName("with space")).toBe(false); + expect(isBranchName("x".repeat(33))).toBe(false); + // git refuses a ref that ends in a dot, so we must too. + expect(isBranchName("Year-3.")).toBe(false); + expect(toBranchName("Year 3.")).toBe("Year-3"); + }); + + it("keeps a long name inside the limit, cut on a separator", () => { + const name = toBranchName("A really quite long name for a variation here"); + expect(name.length).toBeLessThanOrEqual(32); + expect(isBranchName(name)).toBe(true); + expect(name.endsWith("-")).toBe(false); + }); + + it("reads a ref map only when every part of it is one", () => { + const oid = "a".repeat(40); + expect(parseRefMap(`{"main":"${oid}"}`)).toEqual({ main: oid }); + // "" is a claim of absence, which an expected-map is allowed to make. + expect(parseRefMap('{"main":""}')).toEqual({ main: "" }); + expect(parseRefMap('{"main":"nope"}')).toBe(null); + expect(parseRefMap('{"bad name":"' + oid + '"}')).toBe(null); + expect(parseRefMap("not json")).toBe(null); + expect(parseRefMap('["main"]')).toBe(null); + }); +}); + +describe("committing on a branch", () => { + it("follows HEAD rather than always writing the default branch", async () => { + const ctx = await seeded(); + const start = await headOid(ctx); + + await createBranch({ ...ctx, name: "Year-3" }); + expect(await currentBranch(ctx)).toBe("Year-3"); + + await commitDoc({ ...ctx, doc: doc("Lesson", "on the variation"), author }); + + // The variation moved; the lesson did not. + expect(await headOid({ ...ctx, ref: `refs/heads/${BRANCH}` })).toBe(start); + expect(await headOid({ ...ctx, ref: "refs/heads/Year-3" })).not.toBe(start); + }); + + it("gives each branch its own history", async () => { + const ctx = await seeded(); + await createBranch({ ...ctx, name: "Year-3" }); + await commitDoc({ ...ctx, doc: doc("Lesson", "second"), author }); + + expect(await history(ctx)).toHaveLength(2); + expect(await history({ ...ctx, ref: `refs/heads/${BRANCH}` })).toHaveLength( + 1, + ); + }); + + it("hands back the document at the branch being switched to", async () => { + const ctx = await seeded(); + await createBranch({ ...ctx, name: "Year-3" }); + await commitDoc({ ...ctx, doc: doc("Lesson", "changed here"), author }); + + const back = await checkoutBranch({ ...ctx, name: BRANCH }); + expect(back.doc.sections[0].blocks[0].text).toBe("first"); + expect(await currentBranch(ctx)).toBe(BRANCH); + }); + + it("counts how far a variation has run ahead of the lesson", async () => { + const ctx = await seeded(); + await createBranch({ ...ctx, name: "Year-3" }); + await commitDoc({ ...ctx, doc: doc("Lesson", "a"), author }); + await commitDoc({ ...ctx, doc: doc("Lesson", "b"), author }); + + const branches = await listBranches(ctx); + expect(branches.map((b) => b.name)).toEqual([BRANCH, "Year-3"]); + expect(branches.find((b) => b.name === BRANCH).ahead).toBe(0); + expect(branches.find((b) => b.name === "Year-3").ahead).toBe(2); + }); +}); + +describe("travelling", () => { + it("packs every branch, and a clone gets them all back", async () => { + const ctx = await seeded(); + await createBranch({ ...ctx, name: "Year-3" }); + await commitDoc({ ...ctx, doc: doc("Lesson", "variation"), author }); + await checkoutBranch({ ...ctx, name: BRANCH }); + + const packed = await packRepo(ctx); + expect(new Set(Object.keys(packed.refs))).toEqual( + new Set([BRANCH, "Year-3"]), + ); + // `head` is the lesson, not whichever branch happens to be furthest along. + expect(packed.head).toBe( + await headOid({ ...ctx, ref: `refs/heads/${BRANCH}` }), + ); + + const clone = memRepo("clone"); + await cloneFromPack({ ...clone, ...packed }); + + expect((await listBranches(clone)).map((b) => b.name)).toEqual([ + BRANCH, + "Year-3", + ]); + // A clone arrives at the lesson, whatever the author was last editing. + expect(await currentBranch(clone)).toBe(BRANCH); + expect(await headOid({ ...clone, ref: "refs/heads/Year-3" })).toBe( + packed.refs["Year-3"], + ); + }); + + it("packs only what was asked for, so a proposal carries the lesson alone", async () => { + const ctx = await seeded(); + await createBranch({ ...ctx, name: "Year-3" }); + await commitDoc({ ...ctx, doc: doc("Lesson", "variation"), author }); + + const packed = await packRepo({ ...ctx, only: [BRANCH] }); + expect(Object.keys(packed.refs)).toEqual([BRANCH]); + }); + + it("treats a pack with no branch map as the one branch it has", async () => { + const ctx = await seeded(); + const packed = await packRepo(ctx); + + const clone = memRepo("clone"); + await cloneFromPack({ ...clone, ...packed, refs: null }); + expect((await listBranches(clone)).map((b) => b.name)).toEqual([BRANCH]); + }); +}); + +describe("removing a variation", () => { + it("leaves a marker, so the deletion can reach the hub", async () => { + const ctx = await seeded(); + await createBranch({ ...ctx, name: "Year-3" }); + await commitDoc({ ...ctx, doc: doc("Lesson", "variation"), author }); + const tip = await headOid(ctx); + await checkoutBranch({ ...ctx, name: BRANCH }); + + expect(await deleteBranch({ ...ctx, name: "Year-3" })).toBe(true); + expect((await listBranches(ctx)).map((b) => b.name)).toEqual([BRANCH]); + expect(await deletedBranches(ctx)).toEqual({ "Year-3": tip }); + }); + + it("forgets the deletion when the same name is used again", async () => { + const ctx = await seeded(); + await createBranch({ ...ctx, name: "Year-3" }); + await commitDoc({ ...ctx, doc: doc("Lesson", "variation"), author }); + await checkoutBranch({ ...ctx, name: BRANCH }); + await deleteBranch({ ...ctx, name: "Year-3" }); + + // Reusing the name is not the deleted variation coming back. Left behind, the + // marker would make the next push ask to create and delete one name at once. + await createBranch({ ...ctx, name: "Year-3" }); + expect(await deletedBranches(ctx)).toEqual({}); + }); + + it("forgets it when a rename takes the name instead", async () => { + const ctx = await seeded(); + await createBranch({ ...ctx, name: "Year-3" }); + await checkoutBranch({ ...ctx, name: BRANCH }); + await createBranch({ ...ctx, name: "Year-5" }); + await checkoutBranch({ ...ctx, name: BRANCH }); + await deleteBranch({ ...ctx, name: "Year-3" }); + + await renameBranch({ ...ctx, from: "Year-5", to: "Year-3" }); + // "Year-5" is gone and marked; "Year-3" exists again and must not be. + expect(Object.keys(await deletedBranches(ctx))).toEqual(["Year-5"]); + }); + + it("refuses to remove the lesson, or the branch being edited", async () => { + const ctx = await seeded(); + await expect(deleteBranch({ ...ctx, name: BRANCH })).rejects.toThrow(); + + await createBranch({ ...ctx, name: "Year-3" }); + await expect(deleteBranch({ ...ctx, name: "Year-3" })).rejects.toThrow(); + }); +}); + +describe("renaming", () => { + it("carries HEAD across, and records the old name as gone", async () => { + const ctx = await seeded(); + await createBranch({ ...ctx, name: "Year-3" }); + await commitDoc({ ...ctx, doc: doc("Lesson", "variation"), author }); + const tip = await headOid(ctx); + + await renameBranch({ ...ctx, from: "Year-3", to: "Year-4" }); + + expect(await currentBranch(ctx)).toBe("Year-4"); + expect(await headOid(ctx)).toBe(tip); + expect(await deletedBranches(ctx)).toEqual({ "Year-3": tip }); + }); + + it("won't take a name already in use", async () => { + const ctx = await seeded(); + await createBranch({ ...ctx, name: "Year-3" }); + await checkoutBranch({ ...ctx, name: BRANCH }); + await createBranch({ ...ctx, name: "Year-4" }); + + await expect( + renameBranch({ ...ctx, from: "Year-3", to: "Year-4" }), + ).rejects.toThrow(); + }); +}); diff --git a/packages/core/src/git/pack.js b/packages/core/src/git/pack.js index 3fabe46..43d96dd 100644 --- a/packages/core/src/git/pack.js +++ b/packages/core/src/git/pack.js @@ -19,7 +19,15 @@ // a pack is pure JSON and stays small. import * as git from "isomorphic-git"; -import { BRANCH_REF, UPSTREAM_REF, ensureRepo, headOid } from "./repo.js"; +import { + BRANCH, + BRANCH_REF, + UPSTREAM_REF, + branchRef, + ensureRepo, + headOid, +} from "./repo.js"; +import { isBranchName } from "./refs.js"; /** * Every object reachable from a commit: the commit itself, its ancestors, and @@ -65,23 +73,55 @@ async function collectTree({ fs, gitdir, oid, oids }) { } /** - * Pack the lesson's whole history for upload. + * Pack the lesson's whole history for upload — every branch it holds, not only + * the one being edited. + * + * A variation is worth nothing if it only exists on the machine it was started + * on, so all of them travel. They cost almost nothing to carry: branches of one + * lesson share nearly all of their objects, and reachableOids dedupes by oid, so + * a second variation adds only the commits that are actually unique to it. + * + * @param {string[]} [args.only] Restrict the pack to these branches. A proposal + * offers one version of the lesson, not every variation its author + * happens to have open, so submitPullRequest asks for a single branch. + * @param {string} [args.headBranch] Which branch the pack's `head` names. The + * default branch — the lesson — unless the caller says otherwise, which a + * proposal does: it offers the branch its author was working on. * @returns {Promise<{ packfile: Uint8Array, filename: string, head: string, refs: object } | null>} - * null when the repo has no commits to send. + * `refs` maps every packed branch name to its tip. null when there is + * nothing to send. */ -export async function packRepo({ fs, gitdir }) { - const head = await headOid({ fs, gitdir }); +export async function packRepo({ fs, gitdir, only, headBranch = BRANCH }) { + const names = + only || (await git.listBranches({ fs, gitdir }).catch(() => [])); + + const refs = {}; + for (const name of names) { + if (!isBranchName(name)) continue; // not one of ours; don't publish it + const oid = await headOid({ fs, gitdir, ref: branchRef(name) }); + if (oid) refs[name] = oid; + } + + // A pack has to name a tip, and a repository whose head branch has no commits + // has nothing to send under it. + const head = refs[headBranch] || null; if (!head) return null; - const oids = await reachableOids({ fs, gitdir, oid: head }); + const oids = new Set(); + for (const oid of Object.values(refs)) { + for (const reachable of await reachableOids({ fs, gitdir, oid })) { + oids.add(reachable); + } + } + const { packfile, filename } = await git.packObjects({ fs, gitdir, - oids, + oids: [...oids], write: false, }); - return { packfile, filename, head, refs: { [BRANCH_REF]: head } }; + return { packfile, filename, head, refs }; } /** @@ -115,10 +155,34 @@ async function absorbPack({ fs, gitdir, packfile, filename }) { * common ancestor with the lesson it came from — so the fork can later be merged * back (or pull the original's changes in) with a true three-way merge. */ -export async function cloneFromPack({ fs, gitdir, packfile, filename, head }) { +export async function cloneFromPack({ + fs, + gitdir, + packfile, + filename, + head, + refs, +}) { await ensureRepo({ fs, gitdir }); await absorbPack({ fs, gitdir, packfile, filename }); + // Every branch the pack carries, so opening a lesson on a second machine brings + // the variations along with it and not just the published version. A pack from + // before branches existed advertises no map at all, which is the same thing as + // a map holding only the default branch. + const all = refs && Object.keys(refs).length ? refs : { [BRANCH]: head }; + for (const [name, oid] of Object.entries(all)) { + if (!isBranchName(name) || !oid) continue; + await git.writeRef({ + fs, + gitdir, + ref: branchRef(name), + value: oid, + force: true, + }); + } + // Always land on the lesson itself, whatever the author was last editing + // elsewhere. A clone is somebody arriving, and the lesson is what they came for. await git.writeRef({ fs, gitdir, ref: BRANCH_REF, value: head, force: true }); await git.writeRef({ fs, @@ -141,6 +205,11 @@ export async function cloneFromPack({ fs, gitdir, packfile, filename, head }) { * `ref` says *which* remote this is: UPSTREAM_REF for the lesson we forked from, * ORIGIN_REF for this lesson's own published history (which a trusted * collaborator may have moved on without us). + * + * `refs` and `remote` together record the rest of what the far side holds, as + * remote-tracking branches (`refs/remotes//`). That is what lets a + * push know which of the hub's branches it is replacing and which it has never + * seen — the difference between moving somebody's work forward and erasing it. */ export async function fetchRemotePack({ fs, @@ -149,10 +218,25 @@ export async function fetchRemotePack({ filename, head, ref = UPSTREAM_REF, + refs, + remote, }) { await ensureRepo({ fs, gitdir }); await absorbPack({ fs, gitdir, packfile, filename }); await git.writeRef({ fs, gitdir, ref, value: head, force: true }); + + if (remote && refs) { + for (const [name, oid] of Object.entries(refs)) { + if (!isBranchName(name) || !oid) continue; + await git.writeRef({ + fs, + gitdir, + ref: `refs/remotes/${remote}/${name}`, + value: oid, + force: true, + }); + } + } return head; } diff --git a/packages/core/src/git/refs.js b/packages/core/src/git/refs.js new file mode 100644 index 0000000..9d20136 --- /dev/null +++ b/packages/core/src/git/refs.js @@ -0,0 +1,141 @@ +// The names a lesson's branches may have, and how a branch's name relates to the +// words a person actually typed. +// +// Deliberately free of any git dependency, for the same reason doc.js and ops.js +// are: the Worker validates a push's ref map (apps/api/src/routes/git.js) and the +// editor validates a name before offering it, and neither should have to pull in +// isomorphic-git to do it. Both import this file, so the rules can't drift apart. +// +// ---- Two names for one thing ------------------------------------------------ +// +// A branch has a *ref name*, which git stores and which git's own rules bound +// ("Simpler-for-Year-3"), and a *label*, which is what the person sees ("Simpler +// for Year 3"). We don't store the label anywhere: it is the ref name with its +// hyphens read back as spaces, which round-trips the case that matters — someone +// typing a short phrase — without inventing a second place for a name to live and +// go stale. + +/** The branch a lesson is, as far as everyone but its author is concerned. */ +export const DEFAULT_BRANCH = "main"; + +// Bounds, enforced on both sides of the push. +// +// The ceiling on the count is not arbitrary: a lesson's ref map rides in the R2 +// object's customMetadata, alongside the pack it belongs to, so that a reader can +// never pair one lesson's bytes with another moment's refs. R2 caps that metadata +// at 2 KB in total, and MAX_BRANCHES * (MAX_BRANCH_NAME + an oid + JSON syntax) +// has to stay comfortably inside it. Twelve is far more variations than a lesson +// has any use for, and leaves room to spare. +export const MAX_BRANCHES = 12; +export const MAX_BRANCH_NAME = 32; + +// Characters git allows that we also want: letters, digits, and the three +// separators. Everything else — spaces, slashes, and the punctuation git reserves +// for its own syntax — is either converted or dropped by toBranchName below. +const BRANCH_NAME_RE = /^[A-Za-z0-9][A-Za-z0-9._-]*$/; + +/** A 40-character lowercase hex SHA-1, as git writes them. */ +export const OID_RE = /^[0-9a-f]{40}$/; + +/** + * Whether `name` is a branch name we will store. + * + * The exclusions past the character set are git's own: `..` is how a revision + * range is written, a ref file may not end in `.lock` (the name git gives its own + * lock files), and a ref may not end in a dot at all. + */ +export function isBranchName(name) { + if (typeof name !== "string") return false; + if (name.length === 0 || name.length > MAX_BRANCH_NAME) return false; + if (!BRANCH_NAME_RE.test(name)) return false; + if (name.includes("..")) return false; + if (name.endsWith(".lock")) return false; + // git check-ref-format: a ref may not end with a dot. Ours could, because the + // character set allows one anywhere, and the name would then be one the Worker + // stored and git refused to write. + if (name.endsWith(".")) return false; + return true; +} + +/** + * The branch name for something a person typed. + * + * Spaces become hyphens (so branchLabel can turn them back), anything git would + * refuse is dropped, and the result is trimmed to the length limit on a + * separator where there is one nearby — cutting "Year-3" to "Year-" reads worse + * than cutting it to "Year". + * + * @returns {string} A valid branch name, or "" when nothing usable was left. + */ +export function toBranchName(label) { + let name = (label || "") + .trim() + .replace(/\s+/g, "-") + .replace(/[^A-Za-z0-9._-]/g, "") + .replace(/-{2,}/g, "-") + .replace(/\.{2,}/g, "."); + + // Must start on a letter or digit: leading separators are git's own syntax. + name = name.replace(/^[._-]+/, ""); + if (name.length > MAX_BRANCH_NAME) { + const cut = name.slice(0, MAX_BRANCH_NAME); + const sep = cut.lastIndexOf("-"); + name = sep > MAX_BRANCH_NAME * 0.6 ? cut.slice(0, sep) : cut; + } + name = name.replace(/[._-]+$/, ""); + + if (name.endsWith(".lock")) name = name.slice(0, -5).replace(/[._-]+$/, ""); + return isBranchName(name) ? name : ""; +} + +/** What to show a person for a branch name. The inverse of toBranchName's spaces. */ +export function branchLabel(name) { + return (name || "").replace(/-/g, " "); +} + +/** + * Read a ref map off the wire — the `{ "": "" }` object a push + * sends and a pack's metadata carries. + * + * Every part of it is checked, because both callers are reading something they + * did not write: the Worker is reading a client's headers, and a client is + * reading a response. An unparseable or over-long map is null rather than a + * partial one, so a caller can't half-apply somebody's intent. + * + * @returns {object|null} The validated map, or null if it is not one. + */ +export function parseRefMap(json) { + if (!json) return null; + let value; + try { + value = typeof json === "string" ? JSON.parse(json) : json; + } catch { + return null; + } + if (!value || typeof value !== "object" || Array.isArray(value)) return null; + + const names = Object.keys(value); + if (names.length > MAX_BRANCHES) return null; + + const map = {}; + for (const name of names) { + if (!isBranchName(name)) return null; + const oid = value[name]; + // An empty string is meaningful in an *expected* map: "I believe this branch + // does not exist yet". Callers that don't accept absence reject it themselves. + if (oid === "") { + map[name] = ""; + continue; + } + if (typeof oid !== "string" || !OID_RE.test(oid)) return null; + map[name] = oid; + } + return map; +} + +/** A ref map as it travels: compact JSON, keys sorted so it is stable to compare. */ +export function serializeRefMap(map) { + const out = {}; + for (const name of Object.keys(map || {}).sort()) out[name] = map[name]; + return JSON.stringify(out); +} diff --git a/packages/core/src/git/remote.js b/packages/core/src/git/remote.js index 0dfab26..b888ef0 100644 --- a/packages/core/src/git/remote.js +++ b/packages/core/src/git/remote.js @@ -2,7 +2,8 @@ // packed repository in R2 (see apps/api/src/routes/git.js). // // GET {API}/git/:lessonId/refs public -> { head, refs } (404 when never pushed) -// GET {API}/git/:lessonId/pack public -> the packfile bytes +// GET {API}/git/:lessonId/pack public -> the packfile bytes, with its tip and +// branch map in X-Git-Head/X-Git-Refs // PUT {API}/git/:lessonId/pack Bearer -> store them (the author, or a trusted // collaborator merging a fork back in) // @@ -13,6 +14,7 @@ // Because a lesson has more than one possible writer, PUT is a compare-and-swap: // see pushPack below. import { apiUrl, hasApi } from "../config.js"; +import { DEFAULT_BRANCH, parseRefMap, serializeRefMap } from "./refs.js"; /** Whether the lesson hub (and so the shared history) is reachable at all. */ @@ -22,9 +24,12 @@ function endpoint(lessonId, path) { } /** - * The tip commit of a lesson's published history. - * @returns {Promise<{ head: string } | null>} null when the lesson has no repo - * on the server (it predates this feature, or was never pushed). + * The tip commits of a lesson's published history: `head` for the lesson itself, + * and `refs` naming every branch it holds. + * + * @returns {Promise<{ head: string, refs?: object } | null>} null when the lesson + * has no repo on the server (it predates this feature, or was never + * pushed). */ export async function fetchRefs(lessonId) { if (!hasApi() || !lessonId) return null; @@ -51,8 +56,12 @@ export async function fetchRefs(lessonId) { * the downloaded pack doesn't contain. Reading both from one response makes that * impossible. * - * @returns {Promise<{ packfile: Uint8Array, head: string } | null>} null when the - * lesson has no published history. + * The branch map rides in the same response for the same reason, as X-Git-Refs. + * A lesson stored before it could have more than one branch sends no map, which + * reads as the one branch it had. + * + * @returns {Promise<{ packfile: Uint8Array, head: string, refs: object } | null>} + * null when the lesson has no published history. */ export async function fetchPack(lessonId) { if (!hasApi() || !lessonId) return null; @@ -71,7 +80,13 @@ export async function fetchPack(lessonId) { const bytes = new Uint8Array(await res.arrayBuffer()); if (bytes.byteLength === 0) return null; - return { packfile: bytes, head }; + + // A map we can't read is treated as absent rather than fatal: the default + // branch is in X-Git-Head regardless, so the lesson itself still clones. + const refs = parseRefMap(res.headers.get("X-Git-Refs")) || { + [DEFAULT_BRANCH]: head, + }; + return { packfile: bytes, head, refs }; } /** @@ -95,10 +110,26 @@ export class HistoryMovedError extends Error { * compare-and-swap. The Worker rejects the push (409) if that isn't the head it * holds, which is what stops two writers from overwriting each other's commits. * Pass null only when the lesson has no history at all yet. + * + * ---- Pushing more than one branch ------------------------------------------- + * + * A lesson holds a branch per variation, and a push moves as many of them as + * changed. Three headers describe that, and the Worker applies all of it or none: + * + * X-Git-Refs the branches to set, `{ "": "" }` + * X-Git-Expected what we believe the hub currently holds for each name we are + * touching — "" meaning "I believe this one does not exist" + * X-Git-Deletes the branches to remove, comma-separated + * + * The compare-and-swap is per branch, and it guards the same thing it always did, + * one level down: a branch we don't mention is left exactly as it is, so a device + * that has never heard of somebody's new variation cannot delete it by omission. + * Pushing only `parent` and `head` — which is what a client written before any of + * this does — still means "move the lesson, leave everything else alone". */ export async function pushPack( lessonId, - { packfile, head, parent }, + { packfile, head, parent, refs, expected, deletes }, accessToken, ) { if (!hasApi()) throw new Error("The lesson hub is not configured."); @@ -111,6 +142,9 @@ export async function pushPack( Authorization: `Bearer ${accessToken}`, }; if (parent) headers["X-Git-Parent"] = parent; + if (refs) headers["X-Git-Refs"] = serializeRefMap(refs); + if (expected) headers["X-Git-Expected"] = serializeRefMap(expected); + if (deletes?.length) headers["X-Git-Deletes"] = deletes.join(","); let res; try { diff --git a/packages/core/src/git/repo.js b/packages/core/src/git/repo.js index 637a754..e55d822 100644 --- a/packages/core/src/git/repo.js +++ b/packages/core/src/git/repo.js @@ -18,10 +18,43 @@ import { writeDocTree, } from "./layout.js"; import { describeOps, diffDocs } from "./ops.js"; +import { DEFAULT_BRANCH, isBranchName } from "./refs.js"; -export const BRANCH = "main"; +// The branch a lesson *is*. A lesson can hold several — see the block below — +// but exactly one of them is what the hub publishes, what a reader sees, and +// what a fork clones, and this is it. +export const BRANCH = DEFAULT_BRANCH; export const BRANCH_REF = `refs/heads/${BRANCH}`; +// ---- More than one branch --------------------------------------------------- +// +// A lesson's repository holds a branch per *variation*: an alternative version of +// the lesson its author is trying out, kept apart from the one people are reading. +// The default branch is the lesson; the rest are drafts of what it might become. +// +// Which one is being edited is recorded the way git records it — HEAD, a symbolic +// ref pointing at a branch — rather than as state beside the repository. That +// matters for more than tidiness: HEAD is inside the gitdir, so it survives the +// copy that publishes a draft (adoptDraftRepo) and the copy that forks a lesson +// locally (copyRepo), neither of which knows anything about branches. +export const HEADS_PREFIX = "refs/heads/"; + +/** The full ref for a branch name. */ +export const branchRef = (name) => `${HEADS_PREFIX}${name}`; + +/** The branch name in a full ref, or the ref unchanged if it isn't one. */ +export const branchNameOf = (ref) => + ref?.startsWith(HEADS_PREFIX) ? ref.slice(HEADS_PREFIX.length) : ref; + +// A branch the author deleted, remembered until the deletion has been pushed. +// +// Without this a delete would not travel: a push sends the branches we hold, and +// a branch we no longer hold is indistinguishable from one another device added +// while we weren't looking — which must be kept, not dropped. So a delete leaves +// a marker naming what it removed, the next push turns that into an explicit +// instruction, and only then is the marker cleared. +const DELETED_PREFIX = "refs/deleted/"; + // The two remotes a lesson can have, in git's own vocabulary. // // origin this lesson's own published history — what the hub holds for it. @@ -68,10 +101,40 @@ async function exists(fs, path) { } } -/** The current head commit oid, or null in a repo with no commits yet. */ -export async function headOid({ fs, gitdir }) { +/** + * The branch being edited, read from HEAD. + * + * Falls back to the default branch, which covers a repository written before + * HEAD was anything but decorative as well as the moment before the first commit + * exists. + */ +export async function currentBranch({ fs, gitdir }) { try { - return await git.resolveRef({ fs, gitdir, ref: BRANCH_REF }); + const name = await git.currentBranch({ fs, gitdir, fullname: false }); + return name && isBranchName(name) ? name : BRANCH; + } catch { + return BRANCH; + } +} + +/** The ref HEAD points at — what a commit will move. */ +export async function currentBranchRef(ctx) { + return branchRef(await currentBranch(ctx)); +} + +/** + * The head commit oid, or null when there is nothing there yet. + * + * With no `ref` this answers for the branch being edited, which is what almost + * every caller means. Pass one to ask about a particular branch. + */ +export async function headOid({ fs, gitdir, ref }) { + try { + return await git.resolveRef({ + fs, + gitdir, + ref: ref || (await currentBranchRef({ fs, gitdir })), + }); } catch { return null; } @@ -115,8 +178,12 @@ export async function readHeadDoc({ fs, gitdir }) { export async function commitDoc({ fs, gitdir, doc, author, message, parents }) { await ensureRepo({ fs, gitdir }); + // Whatever HEAD points at — the lesson itself, or the variation being tried + // out. Resolved once, so a commit and the ref it moves can't disagree. + const ref = await currentBranchRef({ fs, gitdir }); + const treeOid = await writeDocTree({ fs, gitdir, doc }); - const head = await headOid({ fs, gitdir }); + const head = await headOid({ fs, gitdir, ref }); // Derive the ops from the previous commit's doc so the message describes what // this commit actually changed, rather than what the editor happened to touch @@ -150,13 +217,7 @@ export async function commitDoc({ fs, gitdir, doc, author, message, parents }) { }, }); - await git.writeRef({ - fs, - gitdir, - ref: BRANCH_REF, - value: oid, - force: true, - }); + await git.writeRef({ fs, gitdir, ref, value: oid, force: true }); return { oid, ops }; } @@ -164,7 +225,7 @@ export async function commitDoc({ fs, gitdir, doc, author, message, parents }) { /** * The lesson's history, newest first. * - * `ref` defaults to the local branch, which is what the editor wants: the + * `ref` defaults to the branch being edited, which is what the editor wants: the * repository it is committing to. A reader wants the *published* history * instead, and those are not the same thing — a lesson open in the editor has * local commits that were never pushed. So a caller may pass any ref or oid, @@ -172,12 +233,13 @@ export async function commitDoc({ fs, gitdir, doc, author, message, parents }) { * * @returns {Promise>} */ -export async function history({ fs, gitdir, depth = 100, ref = BRANCH_REF }) { - // Only the default branch needs the "is there anything here yet" guard: it - // reads HEAD, which says nothing about an explicit oid a caller handed us. - if (ref === BRANCH_REF && !(await headOid({ fs, gitdir }))) return []; +export async function history({ fs, gitdir, depth = 100, ref }) { + // Only the implicit case needs the "is there anything here yet" guard: an + // explicit oid a caller handed us says nothing about whether HEAD resolves. + const target = ref || (await currentBranchRef({ fs, gitdir })); + if (!ref && !(await headOid({ fs, gitdir, ref: target }))) return []; - const commits = await git.log({ fs, gitdir, ref, depth }); + const commits = await git.log({ fs, gitdir, ref: target, depth }); return commits.map(({ oid, commit }) => ({ oid, message: commit.message, @@ -289,3 +351,236 @@ export async function pendingOps({ fs, gitdir, doc }) { const committed = head ? await readDocAt({ fs, gitdir, oid: head }) : null; return diffDocs(committed, doc); } + +// ---- Branches --------------------------------------------------------------- + +/** + * Every branch in the repository, with the one being edited marked. + * + * `ahead` is how many commits this branch has that the default branch doesn't — + * which is the only number about a variation anybody actually wants: how much + * work is sitting on it. The default branch's own `ahead` is 0 by definition. + * + * @returns {Promise>} + */ +export async function listBranches({ fs, gitdir, depth = 200 }) { + const [names, current] = await Promise.all([ + git.listBranches({ fs, gitdir }).catch(() => []), + currentBranch({ fs, gitdir }), + ]); + + // A repository with no commits has no branch files yet, but it is still "on" + // a branch — HEAD says so — and the editor has to be able to show it. + const all = names.includes(current) ? names : [...names, current]; + const defaultOid = await headOid({ fs, gitdir, ref: BRANCH_REF }); + + return Promise.all( + all.sort(sortBranches).map(async (name) => { + const oid = await headOid({ fs, gitdir, ref: branchRef(name) }); + return { + name, + ref: branchRef(name), + oid, + isDefault: name === BRANCH, + isCurrent: name === current, + ahead: + name === BRANCH || !oid + ? 0 + : await aheadCount({ + fs, + gitdir, + ours: oid, + theirs: defaultOid, + depth, + }), + }; + }), + ); +} + +// The lesson first, then the variations alphabetically. The default branch is not +// one variation among others — it is the thing they are variations of. +function sortBranches(a, b) { + if (a === BRANCH) return -1; + if (b === BRANCH) return 1; + return a.localeCompare(b); +} + +/** + * How many commits `ours` has that `theirs` does not. + * + * Bounded by `depth` on both sides, because this is drawn in a menu and an exact + * answer for a very long history is worth less than a fast one. A branch that has + * outrun the window reports the window, which reads as "a lot" and is true. + */ +export async function aheadCount({ fs, gitdir, ours, theirs, depth = 200 }) { + if (!ours || ours === theirs) return 0; + if (!theirs) { + const log = await git.log({ fs, gitdir, ref: ours, depth }).catch(() => []); + return log.length; + } + + const [oursLog, theirsLog] = await Promise.all([ + git.log({ fs, gitdir, ref: ours, depth }).catch(() => []), + git.log({ fs, gitdir, ref: theirs, depth }).catch(() => []), + ]); + const shared = new Set(theirsLog.map((c) => c.oid)); + return oursLog.filter((c) => !shared.has(c.oid)).length; +} + +/** + * Start a new branch at a commit — by default wherever we are now, which is what + * "try something different from here" means. + * + * Refuses to overwrite an existing branch: the caller asked to create one, and + * silently moving somebody's work somewhere else is not a version of that. + */ +export async function createBranch({ fs, gitdir, name, from }) { + if (!isBranchName(name)) throw new Error("That name can't be used."); + + const ref = branchRef(name); + if (await headOid({ fs, gitdir, ref })) { + throw new Error("There is already one with that name."); + } + + const oid = from || (await headOid({ fs, gitdir })); + // A branch has to point at something. Before the first commit there is nothing + // to point at, so there is nothing to branch from either. + if (!oid) throw new Error("There is nothing to base it on yet."); + + await git.writeRef({ fs, gitdir, ref, value: oid, force: false }); + // A name deleted earlier and used again is not the deleted one coming back, and + // the marker must go with it. Left behind, the next push would send this name in + // `refs` *and* in `deletes` — one request contradicting itself, which the hub + // would resolve by deleting a branch that is alive here. + await clearDeletedBranch({ fs, gitdir, name }); + // Creating a variation and then not being on it is never what was meant. + await checkoutBranch({ fs, gitdir, name }); + return { name, ref, oid }; +} + +/** + * Switch to a branch, and hand back the document as it stands there. + * + * This is a checkout in the only sense a bare repository has one: HEAD moves, and + * the caller adopts the document at the new tip. There is no working tree to + * update and no uncommitted state to carry over — the editor commits on a pause, + * so anything worth keeping is already a commit on the branch being left. + * + * @returns {Promise<{ name, oid, doc }>} `doc` is null on a branch with no commits. + */ +export async function checkoutBranch({ fs, gitdir, name }) { + if (!isBranchName(name)) throw new Error("That name can't be used."); + + const ref = branchRef(name); + const oid = await headOid({ fs, gitdir, ref }); + if (!oid && name !== BRANCH) + throw new Error("That version no longer exists."); + + await git.writeRef({ + fs, + gitdir, + ref: "HEAD", + value: ref, + force: true, + symbolic: true, + }); + return { name, oid, doc: oid ? await readDocAt({ fs, gitdir, oid }) : null }; +} + +/** + * Rename a branch, moving HEAD with it when it is the one being edited. + * + * Write the new ref before removing the old one: interrupted the other way round + * the branch would be gone and its commits unreachable, and interrupted this way + * the worst case is a duplicate the author can delete. + */ +export async function renameBranch({ fs, gitdir, from, to }) { + if (from === BRANCH) + throw new Error("The lesson itself can't be renamed here."); + if (!isBranchName(to)) throw new Error("That name can't be used."); + if (from === to) return { name: to }; + + const oid = await headOid({ fs, gitdir, ref: branchRef(from) }); + if (!oid) throw new Error("That version no longer exists."); + if (await headOid({ fs, gitdir, ref: branchRef(to) })) { + throw new Error("There is already one with that name."); + } + + await git.writeRef({ + fs, + gitdir, + ref: branchRef(to), + value: oid, + force: false, + }); + // As in createBranch: renaming *onto* a name deleted earlier must clear that + // name's marker, or the push would create and delete it in one breath. + await clearDeletedBranch({ fs, gitdir, name: to }); + const wasCurrent = (await currentBranch({ fs, gitdir })) === from; + if (wasCurrent) await checkoutBranch({ fs, gitdir, name: to }); + await git.deleteRef({ fs, gitdir, ref: branchRef(from) }); + + // The rename reaches the hub as a delete of the old name plus the new branch, + // which is what it is: refs have no identity of their own to carry across. + await markDeleted({ fs, gitdir, name: from, oid }); + return { name: to, oid }; +} + +/** + * Delete a branch, leaving behind the marker that will carry the deletion to the + * hub on the next push (see DELETED_PREFIX above). + * + * The default branch can't go — it is the lesson — and neither can the one being + * edited, because there would then be no answer to "what am I looking at". + */ +export async function deleteBranch({ fs, gitdir, name }) { + if (name === BRANCH) throw new Error("The lesson itself can't be deleted."); + if ((await currentBranch({ fs, gitdir })) === name) { + throw new Error("Switch to another version before deleting this one."); + } + + const oid = await headOid({ fs, gitdir, ref: branchRef(name) }); + if (!oid) return false; + + await git.deleteRef({ fs, gitdir, ref: branchRef(name) }); + await markDeleted({ fs, gitdir, name, oid }); + return true; +} + +async function markDeleted({ fs, gitdir, name, oid }) { + await git.writeRef({ + fs, + gitdir, + ref: `${DELETED_PREFIX}${name}`, + value: oid, + force: true, + }); +} + +/** + * The branches deleted here that the hub may not know about yet, as + * `{ name: theOidItPointedAt }` — the oid being what the deletion is compared and + * swapped against, so a push can't remove work somebody else put on that name in + * the meantime. + */ +export async function deletedBranches({ fs, gitdir }) { + const refs = await git + .listRefs({ fs, gitdir, filepath: DELETED_PREFIX.replace(/\/$/, "") }) + .catch(() => []); + + const out = {}; + for (const name of refs) { + if (!isBranchName(name)) continue; + const oid = await headOid({ fs, gitdir, ref: `${DELETED_PREFIX}${name}` }); + if (oid) out[name] = oid; + } + return out; +} + +/** Forget a deletion, once the hub has been told about it. */ +export async function clearDeletedBranch({ fs, gitdir, name }) { + await git + .deleteRef({ fs, gitdir, ref: `${DELETED_PREFIX}${name}` }) + .catch(() => {}); +} diff --git a/packages/core/src/git/revert.test.js b/packages/core/src/git/revert.test.js new file mode 100644 index 0000000..427bbd7 --- /dev/null +++ b/packages/core/src/git/revert.test.js @@ -0,0 +1,121 @@ +// Undoing one change without losing everything after it. +// +// Restoring is the blunt instrument and always worked; this is the precise one, +// and it is the same three-way merge with the sides pointed backwards: +// +// base the document as that commit left it +// ours the document now +// theirs the document as it was immediately before +// +// The property worth protecting is the one in the name — everything changed since +// survives — and the property worth protecting *next* is that a change built on +// since raises a conflict rather than being quietly reversed under the author. +// prepareRevert is bound to the browser's filesystem; the call it makes is this. + +import { describe, expect, it } from "vitest"; +import { mergeDocs } from "./merge.js"; +import { diffDocs } from "./ops.js"; + +function doc(one, two, three) { + return { + title: "Lesson", + sections: [ + { + id: "s1", + name: "One", + blocks: [ + { id: "b1", type: "text", text: one }, + { id: "b2", type: "text", text: two }, + { id: "b3", type: "text", text: three }, + ], + }, + ], + }; +} + +const textOf = (d, id) => + d.sections[0].blocks.find((b) => b.id === id)?.text ?? null; + +/** The revert, expressed exactly as prepareRevert expresses it. */ +const revert = ({ before, after, now }) => mergeDocs(after, now, before); + +describe("undoing one change", () => { + it("puts back what that change touched", () => { + const before = doc("original", "b", "c"); + const after = doc("CHANGED", "b", "c"); + + const result = revert({ before, after, now: after }); + expect(textOf(result.doc, "b1")).toBe("original"); + expect(result.conflicts).toHaveLength(0); + }); + + it("keeps everything changed since — which is the whole point", () => { + const before = doc("original", "b", "c"); + const after = doc("CHANGED", "b", "c"); + // Two later edits, in blocks the reverted change never touched. + const now = doc("CHANGED", "LATER", "ALSO LATER"); + + const result = revert({ before, after, now }); + expect(textOf(result.doc, "b1")).toBe("original"); // undone + expect(textOf(result.doc, "b2")).toBe("LATER"); // kept + expect(textOf(result.doc, "b3")).toBe("ALSO LATER"); // kept + expect(result.conflicts).toHaveLength(0); + }); + + it("asks when the change being undone has been built on", () => { + const before = doc("original", "b", "c"); + const after = doc("CHANGED", "b", "c"); + // Somebody has since edited the very block the change altered. Reversing it + // silently would throw their edit away. + const now = doc("CHANGED AND THEN SOME", "b", "c"); + + const result = revert({ before, after, now }); + expect(result.conflicts.map((c) => c.blockId)).toEqual(["b1"]); + }); + + it("puts back a block the change removed", () => { + const before = doc("a", "b", "c"); + const after = { + ...before, + sections: [ + { + ...before.sections[0], + blocks: before.sections[0].blocks.filter((b) => b.id !== "b2"), + }, + ], + }; + + const result = revert({ before, after, now: after }); + expect(textOf(result.doc, "b2")).toBe("b"); + }); + + it("removes a block the change added", () => { + const before = doc("a", "b", "c"); + const added = { + ...before, + sections: [ + { + ...before.sections[0], + blocks: [ + ...before.sections[0].blocks, + { id: "b4", type: "text", text: "new" }, + ], + }, + ], + }; + + const result = revert({ before, after: added, now: added }); + expect(textOf(result.doc, "b4")).toBe(null); + expect(textOf(result.doc, "b1")).toBe("a"); + }); + + it("is a no-op when the change is no longer in the lesson", () => { + const before = doc("original", "b", "c"); + const after = doc("CHANGED", "b", "c"); + // It was already undone by hand; there is nothing left to reverse. + const now = doc("original", "b", "c"); + + const result = revert({ before, after, now }); + expect(diffDocs(now, result.doc)).toHaveLength(0); + }); +}); diff --git a/packages/core/src/git/review.test.js b/packages/core/src/git/review.test.js new file mode 100644 index 0000000..374b295 --- /dev/null +++ b/packages/core/src/git/review.test.js @@ -0,0 +1,226 @@ +// Reading a proposal without merging it. +// +// The proposal page now answers two questions off the git objects alone — what +// did the proposer change, and would it merge cleanly — and both answers hinge on +// using the **merge base** rather than the lesson's current tip. Getting that +// wrong doesn't crash; it quietly shows a reviewer the author's own edits as +// though the proposer had made them, which is worse. +// +// So these build the real situation: a lesson, a fork of it, and both sides +// moving on. prepareProposalReview itself is bound to the browser's filesystem, +// but the three calls it makes are these, over the same objects. + +import { describe, expect, it } from "vitest"; +import { memRepo } from "./memfs.js"; +import { + cloneFromPack, + contains, + fetchRemotePack, + mergeBase, + packRepo, +} from "./pack.js"; +import { mergeDocs } from "./merge.js"; +import { diffDocs } from "./ops.js"; +import { commitDoc, headOid, readDocAt } from "./repo.js"; + +const author = { name: "Test", email: "test@example.com" }; + +/** A lesson of two text blocks, each addressable so a test can move one. */ +function doc(one, two) { + return { + title: "Lesson", + sections: [ + { + id: "s1", + name: "One", + blocks: [ + { id: "b1", type: "text", text: one }, + { id: "b2", type: "text", text: two }, + ], + }, + ], + }; +} + +/** + * A lesson and a fork of it, sharing ancestry the way a real fork does — through + * a packfile, so the commits really are the same objects. + */ +async function lessonAndFork() { + const lesson = memRepo("lesson"); + await commitDoc({ ...lesson, doc: doc("first", "second"), author }); + + const fork = memRepo("fork"); + await cloneFromPack({ ...fork, ...(await packRepo(lesson)) }); + return { lesson, fork }; +} + +describe("what a proposal changes", () => { + it("is measured from where the two histories diverged, not from the lesson now", async () => { + const { lesson, fork } = await lessonAndFork(); + + // Both move on, in different blocks. + await commitDoc({ ...lesson, doc: doc("first", "AUTHOR"), author }); + await commitDoc({ ...fork, doc: doc("PROPOSER", "second"), author }); + + // One store holding both histories — the reviewer's repository. + const shared = memRepo("shared"); + await cloneFromPack({ ...shared, ...(await packRepo(lesson)) }); + const ours = await headOid(shared); + const theirPack = await packRepo(fork); + await fetchRemotePack({ + ...shared, + ...theirPack, + ref: "refs/remotes/pull/1", + }); + const theirs = theirPack.head; + + const base = await mergeBase({ ...shared, ours, theirs }); + expect(base).toBeTruthy(); + + const [baseDoc, theirDoc] = [ + await readDocAt({ ...shared, oid: base }), + await readDocAt({ ...shared, oid: theirs }), + ]; + + // Against the base: exactly the one block the proposer touched. + const ops = diffDocs(baseDoc, theirDoc); + expect( + ops.filter((o) => o.op === "block.edit").map((o) => o.blockId), + ).toEqual(["b1"]); + + // Against the lesson's tip it would have read as two — the proposer's edit + // *and* the author's, reversed. That is the mistake this guards. + const ourDoc = await readDocAt({ ...shared, oid: ours }); + expect( + diffDocs(ourDoc, theirDoc).filter((o) => o.op === "block.edit").length, + ).toBe(2); + }); + + it("reports no conflicts when the two sides touched different blocks", async () => { + const { lesson, fork } = await lessonAndFork(); + await commitDoc({ ...lesson, doc: doc("first", "AUTHOR"), author }); + await commitDoc({ ...fork, doc: doc("PROPOSER", "second"), author }); + + const shared = memRepo("shared"); + await cloneFromPack({ ...shared, ...(await packRepo(lesson)) }); + const ours = await headOid(shared); + const theirPack = await packRepo(fork); + await fetchRemotePack({ + ...shared, + ...theirPack, + ref: "refs/remotes/pull/1", + }); + + const base = await mergeBase({ ...shared, ours, theirs: theirPack.head }); + const merged = mergeDocs( + await readDocAt({ ...shared, oid: base }), + await readDocAt({ ...shared, oid: ours }), + await readDocAt({ ...shared, oid: theirPack.head }), + ); + expect(merged.conflicts).toHaveLength(0); + }); + + it("counts a conflict when both changed the same field of one block", async () => { + const { lesson, fork } = await lessonAndFork(); + await commitDoc({ ...lesson, doc: doc("AUTHOR", "second"), author }); + await commitDoc({ ...fork, doc: doc("PROPOSER", "second"), author }); + + const shared = memRepo("shared"); + await cloneFromPack({ ...shared, ...(await packRepo(lesson)) }); + const ours = await headOid(shared); + const theirPack = await packRepo(fork); + await fetchRemotePack({ + ...shared, + ...theirPack, + ref: "refs/remotes/pull/1", + }); + + const base = await mergeBase({ ...shared, ours, theirs: theirPack.head }); + const merged = mergeDocs( + await readDocAt({ ...shared, oid: base }), + await readDocAt({ ...shared, oid: ours }), + await readDocAt({ ...shared, oid: theirPack.head }), + ); + expect(merged.conflicts.map((c) => c.blockId)).toEqual(["b1"]); + }); + + it("knows when a proposal is already part of the lesson", async () => { + const { lesson, fork } = await lessonAndFork(); + await commitDoc({ ...fork, doc: doc("PROPOSER", "second"), author }); + + const shared = memRepo("shared"); + await cloneFromPack({ ...shared, ...(await packRepo(lesson)) }); + const before = await headOid(shared); + const theirPack = await packRepo(fork); + await fetchRemotePack({ + ...shared, + ...theirPack, + ref: "refs/remotes/pull/1", + }); + + expect( + await contains({ ...shared, oid: before, ancestor: theirPack.head }), + ).toBe(false); + + // Landing it — a merge commit joining the two histories, which is what a + // reviewer's confirm produces. + await commitDoc({ + ...shared, + doc: doc("PROPOSER", "second"), + author, + parents: [before, theirPack.head], + }); + + expect( + await contains({ + ...shared, + oid: await headOid(shared), + ancestor: theirPack.head, + }), + ).toBe(true); + }); +}); + +describe("fast-forwarding", () => { + it("is available exactly when the lesson hasn't moved since the fork", async () => { + const { lesson, fork } = await lessonAndFork(); + await commitDoc({ ...fork, doc: doc("PROPOSER", "second"), author }); + + const shared = memRepo("shared"); + await cloneFromPack({ ...shared, ...(await packRepo(lesson)) }); + const ours = await headOid(shared); + const theirPack = await packRepo(fork); + await fetchRemotePack({ + ...shared, + ...theirPack, + ref: "refs/remotes/pull/1", + }); + + // The lesson is the merge base: nothing of ours sits on top of it, so + // "merging" is only moving our branch to theirs. + expect(await mergeBase({ ...shared, ours, theirs: theirPack.head })).toBe( + ours, + ); + }); + + it("is not available once the lesson has its own commits", async () => { + const { lesson, fork } = await lessonAndFork(); + await commitDoc({ ...fork, doc: doc("PROPOSER", "second"), author }); + await commitDoc({ ...lesson, doc: doc("first", "AUTHOR"), author }); + + const shared = memRepo("shared"); + await cloneFromPack({ ...shared, ...(await packRepo(lesson)) }); + const ours = await headOid(shared); + const theirPack = await packRepo(fork); + await fetchRemotePack({ + ...shared, + ...theirPack, + ref: "refs/remotes/pull/1", + }); + + expect( + await mergeBase({ ...shared, ours, theirs: theirPack.head }), + ).not.toBe(ours); + }); +}); diff --git a/packages/core/src/pulls.js b/packages/core/src/pulls.js index 9ee9f4b..6ba383a 100644 --- a/packages/core/src/pulls.js +++ b/packages/core/src/pulls.js @@ -37,6 +37,14 @@ import { apiUrl, hasApi } from "./config.js"; export const PULL_TITLE_MAX = 200; export const PULL_BODY_MAX = 4000; +// How many times one proposal may be updated before it has to be closed and +// reopened. A proposal is a conversation about a specific change; past a couple +// of dozen rewrites it is a different change, and the thread attached to it has +// stopped being about what it now contains. It also bounds the one thing an +// update costs the hub — a full pack rewrite — for a caller who could otherwise +// repeat it indefinitely. +export const MAX_PULL_REVISIONS = 20; + function endpoint(lessonId, path = "") { return `${apiUrl()}/lessons/${encodeURIComponent(lessonId)}/pulls${path}`; } @@ -105,7 +113,14 @@ export async function fetchPullRequests(lessonId, accessToken) { */ export async function createPullRequest( lessonId, - { title, body = "", head, base = null, sourceLessonId = null }, + { + title, + body = "", + head, + headRef = null, + base = null, + sourceLessonId = null, + }, accessToken, ) { if (!hasApi()) throw new Error("The lesson hub is not configured."); @@ -124,6 +139,7 @@ export async function createPullRequest( title, body, head, + ...(headRef ? { headRef } : {}), ...(base ? { base } : {}), ...(sourceLessonId ? { sourceLessonId } : {}), }),