feat(join): say what actually went wrong instead of nothing
A failed join looked exactly like a clean one (TODO #145), and the reason turned out to be structural rather than a matter of wording. `joinWithNickname` called `setPendingJoin(null)` unconditionally, which unmounted the join sheet the moment the call returned - before the sheet could render anything. So the inline error path was DEAD: whatever message the UI set was set on an unmounted component and never appeared. That is why a dead end was indistinguishable from a clean join, and it means the old "Could not join group. Check the invite link and try again" was never actually shown either. The sheet is now dismissed only when there is nothing left to say - success, a blocked link (which raises its own toast), or a benign outcome. On top of that, the outcomes are finally reported: - `reason` is threaded through the repair_failed return in src/invite.js. It was already computed by repairKeylessGroupFromInvite, whose own comment says it exists "so the UI can say something honest either way", and simply dropped - src/lib/joinOutcome.js maps (error, reason) to a plain-language message and a tone, pure and testable. Each repair reason gets its own advice: a key-conflict cannot be fixed by retrying, so it says to get re-added, while reconcile-failed says to try again - outcomes that are not failures (already_member, already-keyed) are toned `info` rather than red, because colouring a non-problem red teaches people to ignore red - a throw inside handleInviteLink is caught and reported. It used to leave the sheet spinning forever, since the caller only ever reacted to a returned result - an unknown code names itself rather than inventing a cause Also fixes the debugging trap #145 records: `Joined group swarm: … topic:` was printing the groupKey, not the topic. For a keyless group they are identical, for a keyed one the topic is hash('pearcal-enc-topic-v1:' + groupKey), so the line looked like proof a device had joined the unencrypted topic while proving nothing. It now prints both under their own names plus (encrypted)/(unencrypted). 330 unit tests, 18 new, including that no two repair reasons produce the same advice, that no message leaks an error code as jargon, and that a missing group name never renders "undefined". Verified on the TCL, both tones on screen: - malformed link -> "That invite link does not look right. Ask for a new one and paste the whole thing." inline in red, sheet stays open - already a member -> "You are already in "KeyGuard39"." as a neutral toast, sheet closes And the corrected swarm log distinguishes the two damaged groups (groupKey == topic, unencrypted) from the healthy one (distinct derived topic, encrypted). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HG8ayyquJuDMPSVLKDQKVh
This commit is contained in:
co-authored by
Claude Opus 5
parent
d01e454d75
commit
a368292101
+11
-1
@@ -2541,7 +2541,17 @@ async function _joinGroupImpl (group) {
|
||||
const topic = groupSwarmTopic(topicKey, group.encryptionKey)
|
||||
swarm.join(topic, { server: true, client: true })
|
||||
|
||||
console.log('Joined group swarm:', group.id, 'topic:', topicKey.slice(0,16))
|
||||
// TODO #145 - this used to print `topicKey` under the label "topic:", which is
|
||||
// the groupKey, not the topic. For a KEYLESS group the two are the same
|
||||
// (groupSwarmTopic falls back to the raw groupKey), but for a keyed one the
|
||||
// topic is hash('pearcal-enc-topic-v1:' + groupKey). So the line looked like
|
||||
// proof a device had joined the unencrypted topic while proving nothing, and
|
||||
// it nearly produced a bogus "the repair rejoins the wrong topic" report.
|
||||
// Print both, each under its own name.
|
||||
console.log('Joined group swarm:', group.id,
|
||||
'groupKey:', String(topicKey).slice(0, 16),
|
||||
'topic:', b4a.toString(topic, 'hex').slice(0, 16),
|
||||
group.encryptionKey ? '(encrypted)' : '(unencrypted)')
|
||||
|
||||
// Register with blind peer so cores stay available when app is closed
|
||||
if (blind) blind.addAutobaseBackground(base)
|
||||
|
||||
+5
-1
@@ -83,7 +83,11 @@ export async function handleInviteLink (url, db, sync, onJoined, nickname = null
|
||||
if (onJoined) onJoined(healed)
|
||||
return { ok: true, repaired: true, group: healed }
|
||||
}
|
||||
return { ok: false, error: 'repair_failed', group: existing }
|
||||
// Pass the reason through. repairKeylessGroupFromInvite has always
|
||||
// returned one, and its comment says it exists "so the UI can say
|
||||
// something honest either way" - it was simply dropped here, so a
|
||||
// key-conflict and a reconcile-failure looked identical (TODO #145).
|
||||
return { ok: false, error: 'repair_failed', reason: res?.reason ?? 'reconcile-failed', group: existing }
|
||||
}
|
||||
return { ok: false, error: 'already_member', group: existing }
|
||||
}
|
||||
|
||||
@@ -0,0 +1,98 @@
|
||||
// TODO #145 - a failed join has to say something true, and something the person
|
||||
// can act on.
|
||||
//
|
||||
// handleInviteLink already distinguishes a dozen outcomes and
|
||||
// repairKeylessGroupFromInvite already returns a `reason` whose own comment says
|
||||
// it exists "so the UI can say something honest either way". The UI never used
|
||||
// either: the join sheet closed silently on `already_member`, and everything
|
||||
// else collapsed into "Check the invite link and try again" - which is actively
|
||||
// wrong when the link is perfect and the repair is what failed.
|
||||
//
|
||||
// The cost of that silence is not hypothetical. A genuinely failed join looked
|
||||
// exactly like a clean one twice while chasing #123 and #148, and both times it
|
||||
// took a logcat dig to find out which had happened.
|
||||
//
|
||||
// Kept pure and separate so the wording is testable without a WebView, and so
|
||||
// there is one place to be right about which outcome maps to which advice.
|
||||
|
||||
'use strict'
|
||||
|
||||
// What the user should be told, and whether it is bad news.
|
||||
//
|
||||
// { message, tone } tone: 'error' | 'warn' | 'info'
|
||||
//
|
||||
// `info` is deliberate: some outcomes are not failures at all from the user's
|
||||
// point of view. Being told "you are already in this group" is an answer, not an
|
||||
// error, and colouring it red would train people to ignore red.
|
||||
function joinOutcomeMessage ({ error, reason, groupName } = {}) {
|
||||
const name = groupName ? `"${groupName}"` : 'that group'
|
||||
|
||||
switch (error) {
|
||||
case 'blocked_from_group':
|
||||
return { tone: 'error', message: `You were removed from ${name}, so this invite will not work. Ask whoever runs the group to invite you again.` }
|
||||
|
||||
case 'already_member':
|
||||
return { tone: 'info', message: `You are already in ${name}.` }
|
||||
|
||||
// handleInviteLink threw rather than returning. Before this was caught, the
|
||||
// sheet spun forever with nothing said, because the caller only ever reacted
|
||||
// to a returned result.
|
||||
case 'join_threw':
|
||||
return { tone: 'error', message: `Something went wrong joining ${name}. Try the link again, and if it keeps failing ask for a fresh one.` }
|
||||
|
||||
case 'repair_failed':
|
||||
// The group is one this device cannot decrypt, and the fresh invite that
|
||||
// should have cured it did not. Each reason is a genuinely different
|
||||
// story, which is the whole point of surfacing it (TODO #124/#123).
|
||||
switch (reason) {
|
||||
case 'key-conflict':
|
||||
return { tone: 'error', message: `This invite is for a different version of ${name} than the one on this device. Ask the sender to remove you from the group and invite you back.` }
|
||||
case 'not-a-member':
|
||||
return { tone: 'error', message: `${name} is no longer on this device, so there was nothing to repair. Ask for a fresh invite link to join again.` }
|
||||
case 'already-keyed':
|
||||
return { tone: 'info', message: `${name} is already working on this device. Nothing needed repairing.` }
|
||||
case 'missing-args':
|
||||
return { tone: 'error', message: `That invite link is missing the part needed to repair ${name}. Ask the sender for a new one.` }
|
||||
case 'reconcile-failed':
|
||||
default:
|
||||
return { tone: 'error', message: `Could not repair ${name} on this device. Try the link again, and if it still fails ask the sender to re-share it.` }
|
||||
}
|
||||
|
||||
// Every parse failure from parseInviteLink. Distinct from the above: here
|
||||
// the link itself is the problem, so "check the link" is the right advice
|
||||
// rather than the catch-all it used to be.
|
||||
case 'invalid_url':
|
||||
case 'malformed_url':
|
||||
case 'wrong_path':
|
||||
case 'missing_params':
|
||||
case 'invalid_group_id':
|
||||
case 'empty_name':
|
||||
case 'invalid_key':
|
||||
case 'invalid_inviter':
|
||||
case 'invalid_enc':
|
||||
return { tone: 'error', message: 'That invite link does not look right. Ask for a new one and paste the whole thing.' }
|
||||
|
||||
default:
|
||||
// Unknown code. Say so plainly rather than inventing a cause - an honest
|
||||
// "something went wrong" beats a confident wrong diagnosis, and the code
|
||||
// is included so a report names it.
|
||||
return { tone: 'error', message: `Could not join ${name}${error ? ` (${error})` : ''}. Try again, or ask for a fresh invite link.` }
|
||||
}
|
||||
}
|
||||
|
||||
// May the join sheet close on this outcome?
|
||||
//
|
||||
// Only for the genuinely-nothing-wrong cases. Note this decides CLOSING, not
|
||||
// silence: the message above is shown either way. Closing while saying nothing
|
||||
// is exactly what made these dead ends invisible, and a benign outcome the user
|
||||
// never sees is still a question left unanswered.
|
||||
function isBenignJoinOutcome ({ error, reason } = {}) {
|
||||
if (error === 'already_member') return true
|
||||
if (error === 'repair_failed' && reason === 'already-keyed') return true
|
||||
return false
|
||||
}
|
||||
|
||||
module.exports = {
|
||||
joinOutcomeMessage,
|
||||
isBenignJoinOutcome,
|
||||
}
|
||||
+32
-7
@@ -15,6 +15,7 @@ import { useState, useEffect, useMemo, useRef, useCallback } from 'react'
|
||||
import { createPortal } from 'react-dom'
|
||||
import { buildInviteLink, handleInviteLink } from '../invite.js'
|
||||
import { SEEDER_PAIR_SCAN_TIMEOUT_MS, secondsRemaining, formatCountdown } from '../lib/seederPairTiming.js'
|
||||
import { joinOutcomeMessage, isBenignJoinOutcome } from '../lib/joinOutcome.js'
|
||||
import QRCode from 'qrcode'
|
||||
import { FONT, colors, injectGlobalStyles, setTheme as applyTheme } from './theme.js'
|
||||
import {
|
||||
@@ -1325,9 +1326,17 @@ export default function App ({ db, notifs, sync }) {
|
||||
|
||||
const joinWithNickname = useCallback(async (url, nickname) => {
|
||||
const nick = nickname && nickname !== profile?.name ? nickname : null
|
||||
const result = await handleInviteLink(url, db, sync, g => {
|
||||
setTab('groups')
|
||||
}, nick)
|
||||
let result
|
||||
try {
|
||||
result = await handleInviteLink(url, db, sync, g => {
|
||||
setTab('groups')
|
||||
}, nick)
|
||||
} catch (e) {
|
||||
// A throw in here used to leave the sheet spinning forever with nothing
|
||||
// said: handleJoin only reacts to a returned result, so an exception was
|
||||
// indistinguishable from a hang (TODO #145).
|
||||
result = { ok: false, error: 'join_threw', reason: e?.message }
|
||||
}
|
||||
if (result?.ok && result.group) {
|
||||
setGroups(prev => prev.find(x => x.id === result.group.id) ? prev : [...prev, result.group])
|
||||
setReadyGroupKeys(prev => { const s = new Set(prev); s.add(result.group.id); return s })
|
||||
@@ -1348,7 +1357,15 @@ export default function App ({ db, notifs, sync }) {
|
||||
if (evts) setEvents(evts)
|
||||
})
|
||||
}
|
||||
setPendingJoin(null)
|
||||
// TODO #145 - dismiss ONLY when there is nothing left to say. This used to
|
||||
// run unconditionally, which unmounted the sheet before handleJoin could
|
||||
// render anything: the inline error path was structurally dead, so every
|
||||
// failed join looked silent no matter what the UI tried to show. That, not
|
||||
// the wording, is why a dead end was indistinguishable from a clean join.
|
||||
// blocked_from_group is dismissed here because it raises its own toast above.
|
||||
if (result?.ok || result?.error === 'blocked_from_group' || isBenignJoinOutcome(result ?? {})) {
|
||||
setPendingJoin(null)
|
||||
}
|
||||
return result
|
||||
}, [db, sync, profile])
|
||||
|
||||
@@ -1882,6 +1899,7 @@ export default function App ({ db, notifs, sync }) {
|
||||
<NicknameBeforeJoinSheet groupName={pendingJoin.groupName}
|
||||
defaultName={profile?.name ?? ''} closeRef={closePendingJoinRef}
|
||||
onConfirm={nickname => joinWithNickname(pendingJoin.url, nickname)}
|
||||
onOutcome={o => { setJoinToast(o); setTimeout(() => setJoinToast(null), 8000) }}
|
||||
onClose={() => setPendingJoin(null)} />
|
||||
)}
|
||||
{newGroupOpen && (
|
||||
@@ -4993,7 +5011,7 @@ function JoinGroupModal ({ onClose, closeRef, db, sync, onJoined, onPendingJoin
|
||||
)
|
||||
}
|
||||
|
||||
function NicknameBeforeJoinSheet ({ groupName, defaultName, onConfirm, onClose, closeRef }) {
|
||||
function NicknameBeforeJoinSheet ({ groupName, defaultName, onConfirm, onClose, closeRef, onOutcome }) {
|
||||
const bsCloseRef = useRef(null)
|
||||
const [nickname, setNickname] = useState(defaultName)
|
||||
const [joining, setJoining] = useState(false)
|
||||
@@ -5011,9 +5029,16 @@ function NicknameBeforeJoinSheet ({ groupName, defaultName, onConfirm, onClose,
|
||||
const result = await onConfirm(nickname.trim())
|
||||
if (result && !result.ok) {
|
||||
setJoining(false)
|
||||
if (result.error === 'already_member') { bsCloseRef.current?.(); return }
|
||||
// TODO #145: every outcome but blocked_from_group used to end here as
|
||||
// either a silent close or "Check the invite link and try again" - which
|
||||
// is the wrong advice whenever the link is fine and the repair is what
|
||||
// failed. Say which of the dozen things actually happened.
|
||||
// blocked_from_group keeps its dedicated toast, raised by joinWithNickname,
|
||||
// so saying it twice here would be worse than saying it once.
|
||||
if (result.error === 'blocked_from_group') { bsCloseRef.current?.(); return }
|
||||
setErr('Could not join group. Check the invite link and try again.')
|
||||
const outcome = joinOutcomeMessage({ error: result.error, reason: result.reason, groupName })
|
||||
if (isBenignJoinOutcome(result)) { onOutcome?.(outcome); bsCloseRef.current?.(); return }
|
||||
setErr(outcome.message)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,148 @@
|
||||
// TODO #145 - a failed join has to say something true and something actionable.
|
||||
// The UI dropped every outcome but blocked_from_group, so a genuinely failed
|
||||
// join looked exactly like a clean one. Pure decision in src/lib/joinOutcome.js.
|
||||
// (feature/surface-join-errors)
|
||||
const test = require('node:test')
|
||||
const assert = require('node:assert/strict')
|
||||
const { joinOutcomeMessage, isBenignJoinOutcome } = require('../src/lib/joinOutcome.js')
|
||||
|
||||
// Every code the join path can actually produce: parseInviteLink's failures plus
|
||||
// handleInviteLink's own, plus repairKeylessGroupFromInvite's reasons.
|
||||
const PARSE_ERRORS = [
|
||||
'invalid_url', 'malformed_url', 'wrong_path', 'missing_params',
|
||||
'invalid_group_id', 'empty_name', 'invalid_key', 'invalid_inviter', 'invalid_enc',
|
||||
]
|
||||
const REPAIR_REASONS = ['missing-args', 'not-a-member', 'already-keyed', 'key-conflict', 'reconcile-failed']
|
||||
|
||||
test('THE #145 REGRESSION: a failed repair does not read as a clean join', () => {
|
||||
// The old behaviour was to say nothing at all, or to blame the link. Both are
|
||||
// wrong: the link is fine, the repair is what failed.
|
||||
const r = joinOutcomeMessage({ error: 'repair_failed', reason: 'reconcile-failed', groupName: 'Family' })
|
||||
assert.equal(r.tone, 'error')
|
||||
assert.match(r.message, /Family/)
|
||||
assert.doesNotMatch(r.message, /invite link does not look right/)
|
||||
})
|
||||
|
||||
test('each repair reason tells a different story', () => {
|
||||
// The point of surfacing `reason` at all. If two reasons produced the same
|
||||
// sentence, the plumbing would be pointless.
|
||||
const messages = REPAIR_REASONS.map(reason =>
|
||||
joinOutcomeMessage({ error: 'repair_failed', reason, groupName: 'Family' }).message)
|
||||
assert.equal(new Set(messages).size, messages.length, 'two reasons produced identical advice')
|
||||
})
|
||||
|
||||
test('key-conflict advises the one thing that actually helps', () => {
|
||||
// A stale invite for a rekeyed group cannot be repaired by retrying, so the
|
||||
// advice has to be "get removed and re-added", not "try again".
|
||||
const r = joinOutcomeMessage({ error: 'repair_failed', reason: 'key-conflict', groupName: 'Work' })
|
||||
assert.equal(r.tone, 'error')
|
||||
assert.match(r.message, /different version/i)
|
||||
})
|
||||
|
||||
test('an unknown repair reason still gets a message, not a blank', () => {
|
||||
// Defensive: a reason added to bare.js later must not fall through to nothing.
|
||||
const r = joinOutcomeMessage({ error: 'repair_failed', reason: 'something-new' })
|
||||
assert.equal(r.tone, 'error')
|
||||
assert.ok(r.message.length > 0)
|
||||
})
|
||||
|
||||
test('already-keyed is not an error, and is not coloured like one', () => {
|
||||
// Colouring a non-problem red trains people to ignore red.
|
||||
const r = joinOutcomeMessage({ error: 'repair_failed', reason: 'already-keyed', groupName: 'Family' })
|
||||
assert.equal(r.tone, 'info')
|
||||
})
|
||||
|
||||
test('already_member reads as an answer rather than a failure', () => {
|
||||
const r = joinOutcomeMessage({ error: 'already_member', groupName: 'Family' })
|
||||
assert.equal(r.tone, 'info')
|
||||
assert.match(r.message, /already in "Family"/)
|
||||
})
|
||||
|
||||
test('being blocked says who to ask, since retrying cannot help', () => {
|
||||
const r = joinOutcomeMessage({ error: 'blocked_from_group', groupName: 'Family' })
|
||||
assert.equal(r.tone, 'error')
|
||||
assert.match(r.message, /removed/i)
|
||||
})
|
||||
|
||||
test('every parse failure blames the link, because there the link IS the problem', () => {
|
||||
for (const error of PARSE_ERRORS) {
|
||||
const r = joinOutcomeMessage({ error })
|
||||
assert.equal(r.tone, 'error', error)
|
||||
assert.match(r.message, /invite link does not look right/, error)
|
||||
}
|
||||
})
|
||||
|
||||
test('an unknown error names its code instead of inventing a cause', () => {
|
||||
// An honest "something went wrong (x)" beats a confident wrong diagnosis, and
|
||||
// the code makes a user report actionable.
|
||||
const r = joinOutcomeMessage({ error: 'brand_new_code', groupName: 'Family' })
|
||||
assert.equal(r.tone, 'error')
|
||||
assert.match(r.message, /brand_new_code/)
|
||||
})
|
||||
|
||||
test('a missing group name never leaves a dangling quote or "undefined"', () => {
|
||||
// The name comes from a URL parameter, so it is routinely absent.
|
||||
for (const error of [...PARSE_ERRORS, 'already_member', 'blocked_from_group', 'repair_failed', 'nonsense']) {
|
||||
const r = joinOutcomeMessage({ error })
|
||||
assert.doesNotMatch(r.message, /undefined|""/, error)
|
||||
}
|
||||
})
|
||||
|
||||
test('called with nothing at all, it still returns a usable message', () => {
|
||||
const r = joinOutcomeMessage()
|
||||
assert.equal(r.tone, 'error')
|
||||
assert.ok(r.message.length > 0)
|
||||
})
|
||||
|
||||
test('every message is plain language: no error codes leaking as jargon', () => {
|
||||
// Rule: these are read by the person using the app. The only code allowed
|
||||
// through is the unknown-error fallback, which is deliberate.
|
||||
const known = [
|
||||
...PARSE_ERRORS.map(error => ({ error })),
|
||||
{ error: 'already_member' },
|
||||
{ error: 'blocked_from_group' },
|
||||
...REPAIR_REASONS.map(reason => ({ error: 'repair_failed', reason })),
|
||||
]
|
||||
for (const c of known) {
|
||||
const { message } = joinOutcomeMessage({ ...c, groupName: 'Family' })
|
||||
assert.doesNotMatch(message, /repair_failed|already_member|blocked_from_group|encryptionKey|reconcile-failed|key-conflict/, JSON.stringify(c))
|
||||
}
|
||||
})
|
||||
|
||||
// ── isBenignJoinOutcome ───────────────────────────────────────────────────
|
||||
test('only the genuinely-nothing-wrong outcomes let the sheet close', () => {
|
||||
assert.equal(isBenignJoinOutcome({ error: 'already_member' }), true)
|
||||
assert.equal(isBenignJoinOutcome({ error: 'repair_failed', reason: 'already-keyed' }), true)
|
||||
})
|
||||
|
||||
test('a real failure keeps the sheet open', () => {
|
||||
// Closing is what made these dead ends invisible in the first place.
|
||||
assert.equal(isBenignJoinOutcome({ error: 'repair_failed', reason: 'reconcile-failed' }), false)
|
||||
assert.equal(isBenignJoinOutcome({ error: 'repair_failed', reason: 'key-conflict' }), false)
|
||||
assert.equal(isBenignJoinOutcome({ error: 'invalid_key' }), false)
|
||||
assert.equal(isBenignJoinOutcome({ error: 'blocked_from_group' }), false)
|
||||
assert.equal(isBenignJoinOutcome({}), false)
|
||||
assert.equal(isBenignJoinOutcome(), false)
|
||||
})
|
||||
|
||||
test('benign means closeable, NOT silent', () => {
|
||||
// The pairing that matters: anything benign still has a message to show, or
|
||||
// closing it would be the old silence under a new name.
|
||||
for (const c of [{ error: 'already_member' }, { error: 'repair_failed', reason: 'already-keyed' }]) {
|
||||
assert.equal(isBenignJoinOutcome(c), true)
|
||||
assert.ok(joinOutcomeMessage({ ...c, groupName: 'Family' }).message.length > 0)
|
||||
}
|
||||
})
|
||||
|
||||
test('a thrown join is reported, not left spinning', () => {
|
||||
// handleInviteLink throwing used to be indistinguishable from a hang: the
|
||||
// caller only reacted to a RETURNED result, so nothing was ever shown.
|
||||
const r = joinOutcomeMessage({ error: 'join_threw', reason: 'boom', groupName: 'Family' })
|
||||
assert.equal(r.tone, 'error')
|
||||
assert.match(r.message, /Family/)
|
||||
assert.doesNotMatch(r.message, /boom|join_threw/, 'the internal detail must not surface as jargon')
|
||||
})
|
||||
|
||||
test('a thrown join keeps the sheet open, since there is something to read', () => {
|
||||
assert.equal(isBenignJoinOutcome({ error: 'join_threw' }), false)
|
||||
})
|
||||
Reference in New Issue
Block a user