diff --git a/scripts/chrome-reddit-verify-phase17b.py b/scripts/chrome-reddit-verify-phase17b.py index 419610c..7cd3ad7 100644 --- a/scripts/chrome-reddit-verify-phase17b.py +++ b/scripts/chrome-reddit-verify-phase17b.py @@ -39,6 +39,19 @@ AUTH_DIR = ROOT / "playwright" / ".auth" STATE = AUTH_DIR / "reddit-com.json" HEADLESS = os.environ.get("HEADLESS", "1") == "1" +FULLSCREEN = os.environ.get("FULLSCREEN", "0") == "1" +def _safe_int(env_name: str, default: int) -> int: + """Parse an int env var; on garbage input fall back to the default + rather than crashing the recording session (CodeRabbit #2 PR #49).""" + raw = os.environ.get(env_name, str(default)) + try: + return int(raw) + except (ValueError, TypeError): + print(f"[verify] WARN: {env_name}={raw!r} is not an int — using default {default}") + return default + + +SLOW_MO = _safe_int("SLOW_MO", 0) # ms between actions, 0 = no slow-down SUB = os.environ.get("REDDIT_SUB", "SocialSeeding") # Ambiguous input designed to trigger the clarify path on the first compile. COMPOSE_INPUT = os.environ.get( @@ -236,19 +249,34 @@ async def main() -> None: report = RunReport(sub=SUB, input=COMPOSE_INPUT) async with async_playwright() as p: + launch_args = ["--disable-blink-features=AutomationControlled"] + if FULLSCREEN: + # macOS-friendly recording mode: `--start-maximized` opens the + # Chrome window at the user's full screen size (with the address + # bar visible — useful for "this is reddit.com" demo context). + # Pair with `no_viewport=True` so the page matches the window + # exactly, and let Retina DPR happen naturally for sharp text. + # `--start-fullscreen` is unreliable on macOS through Playwright + # (frequently ignored) and `--kiosk` hides the URL bar. + launch_args += ["--start-maximized"] browser = await p.chromium.launch( headless=HEADLESS, - args=["--disable-blink-features=AutomationControlled"], + slow_mo=SLOW_MO, + args=launch_args, ) - ctx = await browser.new_context( + ctx_kwargs = dict( storage_state=str(STATE), - viewport={"width": 1600, "height": 1000}, user_agent=( "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) " "AppleWebKit/537.36 (KHTML, like Gecko) Chrome/130.0.0.0 Safari/537.36" ), locale="en-US", ) + if FULLSCREEN: + ctx_kwargs["no_viewport"] = True # let the page match the maximized window + else: + ctx_kwargs["viewport"] = {"width": 1600, "height": 1000} + ctx = await browser.new_context(**ctx_kwargs) page = await ctx.new_page() async def shot(name: str) -> str: @@ -311,16 +339,14 @@ async def shot(name: str) -> str: extra={"form": clarify_form, "round_visible": round_visible}, )) - if not clarify_pass: - # The model didn't ask for clarification on this input — the - # Confirm form should have opened directly. Fall through to - # confirm-form check. - print(f"[verify] no clarify modal — assuming confirm path.") - else: - # Pick a suggested option via direct DOM mutation (the Devvit - # `faceplate-select` Lit element doesn't respond to a vanilla - # locator.click, but assigning .value + dispatching change/input - # events drives the same code path the human click would). + # Multi-round clarify loop — if the LLM asks again after our first + # answer, pick another option and re-compile, up to MAX_CLARIFY_TURNS. + # Devvit caps at MAX_CLARIFY_TURNS=3 server-side so we'll bottom out + # one way or another. + clarify_rounds_handled = 0 + while clarify_pass and clarify_rounds_handled < 3: + clarify_rounds_handled += 1 + print(f"[verify] handling clarify round #{clarify_rounds_handled}") picker_result = {} try: picker_result = await page.evaluate( @@ -375,29 +401,33 @@ async def shot(name: str) -> str: except Exception as e: picker_result = {"ok": False, "exception": str(e)} + # Only the first picker iteration counts toward the official + # `clarify-select-pick` step — later rounds get suffixed names + # so the report.overall() check stays meaningful. + step_name = "clarify-select-pick" if clarify_rounds_handled == 1 else f"clarify-select-pick-r{clarify_rounds_handled}" report.add(StepResult( - "clarify-select-pick", + step_name, bool(picker_result.get('ok')), f"picked={picker_result.get('picked')} controls_seen={len(picker_result.get('found', []))} opts_seen={len(picker_result.get('opts', []))}", extra={"picker": picker_result}, )) - # Mark PASS when we successfully drove some select-like control. - # The form's defaultValue already pre-selects opts[0], so even - # if our picker only reaffirmed it the next compile will see - # this value — that's still a real round-trip. - if 'clarify-select-pick' not in {s.name for s in report.steps}: - report.add(StepResult( - "clarify-select-pick", - bool(picker_ok), - f"picker_ok={picker_ok} picked_value={picked_val!r}", - )) - - await shot("05-clarify-picked") + await shot(f"05-clarify-r{clarify_rounds_handled}-picked") recompiled = await submit_form(page, r"recompile|re-compile|compile") - report.add(StepResult("clarify-recompile-submit", recompiled, "clicked Re-compile")) + recompile_step = "clarify-recompile-submit" if clarify_rounds_handled == 1 else f"clarify-recompile-submit-r{clarify_rounds_handled}" + report.add(StepResult(recompile_step, recompiled, "clicked Re-compile")) await page.wait_for_timeout(8_000) - await shot("06-after-recompile") + await shot(f"06-after-recompile-r{clarify_rounds_handled}") + + # Re-dump the form so the loop condition reflects what came + # back. If the LLM is satisfied, the next form is the Confirm + # form and clarify_pass flips to False, breaking the loop. + clarify_form = await dump_form(page) + field_names = {f.get('name', '') for f in clarify_form.get('fields', [])} + still_clarify = 'clarificationTurn' in field_names + still_select = any('select' in (f.get('type') or '') for f in clarify_form.get('fields', [])) + clarify_pass = still_clarify and still_select + print(f"[verify] round #{clarify_rounds_handled} done. still_clarify={still_clarify}") # ── 4 · expect Confirm form ───────────────────────────────────── confirm_form = await dump_form(page) diff --git a/src/server/routes-compose.test.ts b/src/server/routes-compose.test.ts index b1aa0ed..fd152f4 100644 --- a/src/server/routes-compose.test.ts +++ b/src/server/routes-compose.test.ts @@ -386,18 +386,19 @@ describe('POST /internal/form/compose-rule-submit', () => { expect(ruleField.disabled).toBeFalsy(); }); - // ── Phase 1.6 (audit finding #6): success toast carries rule summary + menu hint ── - // After Phase 1.7b, the toast comes from /compose-confirm-submit (not the - // compose-rule-submit handler), but the wording lives in - // persistRuleAndStartDryRun() so the assertions still apply to the saved toast. - it('success toast includes a 1-line rule summary and the View-rules menu hint', async () => { + // ── Phase 2c demo-recording UX clean-up: success toast is short ── + // The previous wording packed 4 clauses into one line and Devvit's toast + // truncated mid-sentence in the recording. The compose-confirm form + // already shows the full rule + cost so the toast just needs to confirm + // what happened. + it('success toast is short and names the rule that was saved', async () => { asMod(); fakeSettings.get.mockImplementation(async (k: string) => (k === 'openaiApiKey' ? 'sk-dev' : undefined)); fakeFetch.mockResolvedValue(openaiResponse(VALID_COMPILED)); const { saveBody } = await compileAndConfirm(VALID_COMPILED.sourceNL, false); expect(saveBody.showToast.appearance).toBe('success'); - expect(saveBody.showToast.text).toMatch(/→\s*post[\w+]*:\s*modqueue/); - expect(saveBody.showToast.text).toContain('vibe-mod: View rules + log'); + expect(saveBody.showToast.text).toContain('Flag low-karma posts'); + expect(saveBody.showToast.text.length).toBeLessThan(120); // Devvit toast budget }); it('compiles a valid rule → shows confirm form → on Save: stores draft, bumps counter, schedules dry-run', async () => { @@ -407,24 +408,48 @@ describe('POST /internal/form/compose-rule-submit', () => { ); fakeFetch.mockResolvedValue(openaiResponse(VALID_COMPILED)); - const { confirmFormBody, saveBody } = await compileAndConfirm(VALID_COMPILED.sourceNL, false); - // Phase 1.7b (audit finding #2): compile-rule-submit returns a confirm form, - // not a success toast. Persistence happens on /compose-confirm-submit. + // Phase 1.7b + Phase 2c (audit finding #2): compile-rule-submit returns + // a confirm form, not a success toast. The form now carries a single + // short pendingId — the actual compile state lives under a Redis key + // (audit finding #B from the demo recording — internal carriers were + // bloating the modal). We run the two steps manually here so we can + // assert on the pending entry between them. + const composeRes = await call('/internal/form/compose-rule-submit', { + rule: VALID_COMPILED.sourceNL, + allowGuarded: false, + }); + const confirmFormBody = await composeRes.json(); expect(confirmFormBody.showForm.name).toBe('composeConfirmForm'); expect(confirmFormBody.showForm.form.title).toContain('Flag low-karma posts'); - // The form embeds the compiled rule + token counts as state carriers. const fieldsByName = Object.fromEntries( (confirmFormBody.showForm.form.fields as Array<{ name: string; defaultValue: unknown }>).map((f) => [ f.name, f.defaultValue, ]), ); - expect(fieldsByName.serializedRule).toContain('r_low_karma_flag'); - expect(fieldsByName.llmModel).toBe('gpt-5.4-nano'); - - // After Save: draft persisted, counter bumped, dry-run scheduled. + expect(fieldsByName.compiledSummary).toContain('Flag low-karma posts'); + expect(typeof fieldsByName.pendingId).toBe('string'); + expect((fieldsByName.pendingId as string).length).toBeGreaterThan(0); + // No more raw internal carriers in the modal. + expect(fieldsByName.serializedRule).toBeUndefined(); + expect(fieldsByName.llmModel).toBeUndefined(); + expect(fieldsByName.usingBYOK).toBeUndefined(); + // The pending entry should round-trip the model the test stubbed. + const pendingJson = JSON.parse((await fakeRedis.get(`testsub:compose:pending:${fieldsByName.pendingId}`))!); + expect(pendingJson.llmModel).toBe('gpt-5.4-nano'); + expect(pendingJson.validated.id).toBe('r_low_karma_flag'); + + // Now run Save — pending entry gets consumed, draft persisted. + const savePayload: Record = {}; + for (const f of confirmFormBody.showForm.form.fields as Array<{ name: string; defaultValue: unknown }>) { + savePayload[f.name] = f.defaultValue; + } + savePayload.editInsteadOfSave = false; + const saveRes = await call('/internal/form/compose-confirm-submit', savePayload); + const saveBody = await saveRes.json(); expect(saveBody.showToast.appearance).toBe('success'); expect(saveBody.showToast.text).toContain('Flag low-karma posts'); + expect(await fakeRedis.get(`testsub:compose:pending:${fieldsByName.pendingId}`)).toBeUndefined(); const draft = JSON.parse((await fakeRedis.get('testsub:rules:draft'))!); expect(draft.rules).toHaveLength(1); diff --git a/src/server/routes-dashboard.test.ts b/src/server/routes-dashboard.test.ts index 894c690..8df0c3b 100644 --- a/src/server/routes-dashboard.test.ts +++ b/src/server/routes-dashboard.test.ts @@ -29,6 +29,18 @@ async function seedAudit(actionId: string, fields: Record, score await fakeRedis.hSet(`testsub:audit:${actionId}`, fields); } +// Helper: concatenate every visible label + paragraph value the dashboard +// form renders, so the assertions still work against the Phase 2c +// multi-block layout (was: one big string in `description`). +function dashTexts(body: any): string { + return [ + body.showForm.form.description ?? '', + ...body.showForm.form.fields.map( + (f: { label?: string; defaultValue?: unknown }) => `${f.label ?? ''}\n${String(f.defaultValue ?? '')}`, + ), + ].join('\n'); +} + describe('POST /internal/menu/dashboard', () => { it('rejects a non-moderator', async () => { const body = await ( @@ -43,8 +55,9 @@ describe('POST /internal/menu/dashboard', () => { await call('/internal/menu/dashboard', { location: 'subreddit', targetId: 't5_testsub' }) ).json(); expect(body.showForm.name).toBe('dashboardForm'); - expect(body.showForm.form.description).toContain('Active rules: 0'); - expect(body.showForm.form.description).toContain('Draft rules: 0'); + const text = dashTexts(body); + expect(text).toContain('Active rules: 0'); + expect(text).toContain('Draft rules: 0'); expect(body.showForm.form.acceptLabel).toBe('Close'); }); @@ -78,12 +91,13 @@ describe('POST /internal/menu/dashboard', () => { const body = await ( await call('/internal/menu/dashboard', { location: 'subreddit', targetId: 't5_testsub' }) ).json(); - expect(body.showForm.form.description).toContain('Active rules: 2'); - expect(body.showForm.form.description).toContain('Draft rules: 5'); - expect(body.showForm.form.description).toContain('Recent actions: 2'); - expect(body.showForm.form.description).toContain('modqueue (applied)'); - expect(body.showForm.form.description).toContain('remove (shadow)'); - expect(body.showForm.form.description).toMatch(/Tokens used \(lifetime\): 1,500 in \/ 300 out/); + const text = dashTexts(body); + expect(text).toContain('Active rules: 2'); + expect(text).toContain('Draft rules: 5'); + expect(text).toContain('Recent actions: 2'); + expect(text).toContain('modqueue (applied)'); + expect(text).toContain('remove (shadow)'); + expect(text).toMatch(/1,500 in \/ 300 out/); // Dashboard no longer triggers activation — that moved to Manage rules. expect(body.showForm.form.acceptLabel).toBe('Close'); }); @@ -118,11 +132,10 @@ describe('POST /internal/menu/dashboard', () => { const body = await ( await call('/internal/menu/dashboard', { location: 'subreddit', targetId: 't5_testsub' }) ).json(); - expect(body.showForm.form.description).toContain('Dry-run preview (draft rules):'); - expect(body.showForm.form.description).toContain( - 'r_new_account_fast_post: would match 1/10 recent post(s) → modqueue', - ); - expect(body.showForm.form.description).toContain('r_wall_of_caps_comment: comment events; shadow mode it'); + const text = dashTexts(body); + expect(text).toContain('Dry-run preview (draft rules)'); + expect(text).toContain('r_new_account_fast_post: would match 1/10 recent post(s) → modqueue'); + expect(text).toContain('r_wall_of_caps_comment: comment events; shadow mode it'); }); }); @@ -155,8 +168,9 @@ describe('Dashboard onboarding + empty state (Phase 1.7b Tier-3 #C, Tier-2 #A)', const body = await ( await call('/internal/menu/dashboard', { location: 'subreddit', targetId: 't5_testsub' }) ).json(); - expect(body.showForm.form.description).toContain('Welcome to vibe-mod'); - expect(body.showForm.form.description).toContain('3 quick steps'); + const text = dashTexts(body); + expect(text).toContain('Welcome to vibe-mod'); + expect(text).toContain('3 quick steps'); const fieldNames = body.showForm.form.fields.map((f: { name: string }) => f.name); expect(fieldNames).toContain('dismissOnboarding'); }); @@ -167,8 +181,11 @@ describe('Dashboard onboarding + empty state (Phase 1.7b Tier-3 #C, Tier-2 #A)', const body = await ( await call('/internal/menu/dashboard', { location: 'subreddit', targetId: 't5_testsub' }) ).json(); - expect(body.showForm.form.description).not.toContain('Welcome to vibe-mod'); - expect(body.showForm.form.fields).toEqual([]); + const text = dashTexts(body); + expect(text).not.toContain('Welcome to vibe-mod'); + // No `dismissOnboarding` toggle once the user has already dismissed it. + const fieldNames = body.showForm.form.fields.map((f: { name: string }) => f.name); + expect(fieldNames).not.toContain('dismissOnboarding'); }); it('emits a clear empty state when there are zero rules and zero recent actions (Tier-2 #A)', async () => { @@ -177,7 +194,8 @@ describe('Dashboard onboarding + empty state (Phase 1.7b Tier-3 #C, Tier-2 #A)', const body = await ( await call('/internal/menu/dashboard', { location: 'subreddit', targetId: 't5_testsub' }) ).json(); - expect(body.showForm.form.description).toContain('No rules yet'); - expect(body.showForm.form.description).toContain('vibe-mod: Compose rule'); + const text = dashTexts(body); + expect(text).toContain('No rules yet'); + expect(text).toContain('vibe-mod: Compose rule'); }); }); diff --git a/src/server/routes/compose.ts b/src/server/routes/compose.ts index 149475b..8defb4f 100644 --- a/src/server/routes/compose.ts +++ b/src/server/routes/compose.ts @@ -33,7 +33,6 @@ import { humanizeRule, isClarification, readOpenaiModel, - summarizeRule, todayKey, unwrapFormString, } from '../helpers/openai'; @@ -313,16 +312,58 @@ export function registerComposeRoutes(app: Hono): void { }); } - // Audit finding #2 — confirmation form before persistence. + // Audit finding #2 + Phase 2c demo-recording UX clean-up — confirmation + // form before persistence. The previous shape carried 7 internal fields + // through the form (rule / allowGuarded / serializedRule / tokensIn / + // tokensOut / llmModel / usingBYOK), which (a) bloated the modal so the + // mod had to scroll past a wall of disabled fields and (b) leaked + // implementation detail. Now we stash the whole compile under a + // 10-min Redis key (`composePending`) and only carry the short + // pendingId across the form chain — see compose-confirm-submit. const llmModel = await readOpenaiModel(); const humanized = humanizeRule(validated); const cost = estimateTokenCost(llmModel, tokensIn, tokensOut); + const pendingId = newPendingId(); + try { + // Atomic set + TTL via the `expiration` option (CodeRabbit #3 PR + // #49). The previous two-step `set` + `expire` could leak a + // TTL-less pending key if expire failed after set succeeded. + await redis.set( + keys.composePending(subredditName, pendingId), + JSON.stringify({ validated, tokensIn, tokensOut, llmModel, usingBYOK, originalRule: rule, allowGuarded }), + { expiration: new Date(Date.now() + 600_000) }, + ); + } catch (err) { + console.warn('[vibe-mod] submit: redis.set(composePending) threw:', describeErr(err)); + return c.json({ + showToast: { + text: 'Plugin RPC unreachable — could not stage the compile for confirmation. Try again in a moment.', + appearance: 'neutral', + }, + }); + } + + // Increment the daily compile counter HERE (right after a successful + // OpenAI compile + Redis stash) — NOT in the Save path. The previous + // version only counted on Save, which let a moderator drive arbitrary + // OpenAI cost by repeatedly compiling and cancelling out of the + // confirm form (CodeRabbit #5 PR #49). The token cost is real either + // way, so the quota should reflect it either way. + if (!usingBYOK) { + try { + await redis.set(todayCounterKey, String(todayCount + 1), { + expiration: new Date(Date.now() + 86_400_000), + }); + } catch (err) { + console.warn('[vibe-mod] submit: redis.set(todayCount) threw — quota not incremented:', describeErr(err)); + } + } const summaryHeader = [ `Rule name: ${validated.name}`, `Original sentence: "${validated.sourceNL}"`, - ``, + '', humanized, - ``, + '', `Compile cost: ${tokensIn} in / ${tokensOut} out tokens (~$${cost.toFixed(5)} on ${llmModel}).`, ].join('\n'); @@ -332,13 +373,13 @@ export function registerComposeRoutes(app: Hono): void { form: { title: `Confirm: "${validated.name}"`, description: - 'Review the deterministic compile below. Save to keep this rule as a draft (with a 24h shadow period) and run a dry-run preview against recent posts. Tick "Edit instead" to go back and revise the English.', + 'Review the deterministic compile below. Save to keep this rule as a draft (with a 24h shadow period) and run a dry-run preview against recent posts.', acceptLabel: 'Save + run dry-run preview', cancelLabel: 'Cancel', fields: [ { name: 'compiledSummary', - label: 'Compiled rule (read-only)', + label: 'Compiled rule', type: 'paragraph', defaultValue: summaryHeader, disabled: true, @@ -350,47 +391,15 @@ export function registerComposeRoutes(app: Hono): void { defaultValue: false, helpText: 'Tick to re-open the compose form with your original text pre-filled.', }, - { name: 'rule', label: '(internal) original NL', type: 'paragraph', defaultValue: rule, disabled: true }, - { - name: 'allowGuarded', - label: '(internal) allowGuarded', - type: 'boolean', - defaultValue: !!allowGuarded, - disabled: true, - }, - { - name: 'serializedRule', - label: '(internal) compiled rule JSON', - type: 'paragraph', - defaultValue: JSON.stringify(validated), - disabled: true, - }, + // Single short carrier — Devvit forms don't support hidden fields, + // so the smallest visible token (8-char id) is the best we can + // do. Used by /internal/form/compose-confirm-submit to look up + // the staged compile in Redis. { - name: 'tokensIn', - label: '(internal) tokensIn', - type: 'paragraph', - defaultValue: String(tokensIn), - disabled: true, - }, - { - name: 'tokensOut', - label: '(internal) tokensOut', - type: 'paragraph', - defaultValue: String(tokensOut), - disabled: true, - }, - { - name: 'llmModel', - label: '(internal) llmModel', - type: 'paragraph', - defaultValue: llmModel, - disabled: true, - }, - { - name: 'usingBYOK', - label: '(internal) usingBYOK', - type: 'boolean', - defaultValue: usingBYOK, + name: 'pendingId', + label: '(internal session id, do not edit)', + type: 'string', + defaultValue: pendingId, disabled: true, }, ], @@ -400,7 +409,10 @@ export function registerComposeRoutes(app: Hono): void { }); // Form: compose-confirm-submit — Save persists + schedules dry-run, Edit - // re-opens compose with the original NL pre-filled. + // re-opens compose with the original NL pre-filled. Only one piece of + // state crosses from compose: a short pendingId that points at a Redis + // entry we wrote in the previous step. Everything else (validated rule, + // tokens, model) is read back from there. app.post('/internal/form/compose-confirm-submit', async (c) => { if (!(await isCallerModerator())) { return c.json({ showToast: { text: 'Only moderators can use this.', appearance: 'neutral' } }); @@ -409,31 +421,74 @@ export function registerComposeRoutes(app: Hono): void { const raw = await c.req.json<{ compiledSummary?: string | string[]; editInsteadOfSave?: boolean; - rule?: string | string[]; - allowGuarded?: boolean; - serializedRule?: string | string[]; - tokensIn?: string | string[]; - tokensOut?: string | string[]; - llmModel?: string | string[]; - usingBYOK?: boolean; + pendingId?: string | string[]; }>(); - const rule = unwrapFormString(raw.rule); - const allowGuarded = !!raw.allowGuarded; - const serializedRule = unwrapFormString(raw.serializedRule); - const tokensIn = Math.max(0, Number.parseInt(unwrapFormString(raw.tokensIn), 10) || 0); - const tokensOut = Math.max(0, Number.parseInt(unwrapFormString(raw.tokensOut), 10) || 0); - const llmModel = unwrapFormString(raw.llmModel) || 'gpt-5.4-mini'; - const usingBYOK = !!raw.usingBYOK; + const pendingId = unwrapFormString(raw.pendingId); + if (!pendingId) { + return c.json({ + showToast: { + text: 'Confirmation session expired or missing — please re-compile.', + appearance: 'neutral', + }, + }); + } + + const subredditName = getCurrentSubredditName(); + const pendingKey = keys.composePending(subredditName, pendingId); + + // Atomic GET-then-DEL via WATCH/MULTI/EXEC (CodeRabbit #4 PR #49). The + // previous shape did a plain GET at the top and a DEL at the bottom, + // which left a window where two concurrent submits with the same + // pendingId (back-button + re-submit, double-click on Save, multi-tab + // moderator) could each read the entry and run the persistence flow, + // doubling the bundle write + dry-run schedule. The WATCH-checked + // EXEC means whichever call commits first wins; the loser sees a + // null result and surfaces the "session expired" toast. + let pending: ComposePending | null = null; + let pendingFoundButLost = false; + try { + const txn = await redis.watch(pendingKey); + const pendingJson = await redis.get(pendingKey); + if (pendingJson) { + await txn.multi(); + await txn.del(pendingKey); + const result = await txn.exec(); + if (result == null) { + // Another submitter raced us and consumed the entry first. + pendingFoundButLost = true; + } else { + pending = JSON.parse(pendingJson) as ComposePending; + } + } else { + try { + await txn.discard(); + } catch { + /* ignore */ + } + } + } catch (err) { + console.warn('[vibe-mod] confirm: redis.watch/get/del threw:', describeErr(err)); + } + if (!pending) { + return c.json({ + showToast: { + text: pendingFoundButLost + ? 'Another submission consumed this confirmation. Please re-compile if you still want this rule.' + : 'Confirmation session expired (10 min TTL). Please re-compile to try again.', + appearance: 'neutral', + }, + }); + } if (raw.editInsteadOfSave) { - const subredditName = getCurrentSubredditName(); let dailyCountDisplay = '—'; try { dailyCountDisplay = String(Number((await redis.get(keys.compileCount(subredditName, todayKey()))) ?? '0')); } catch (err) { console.warn('[vibe-mod] confirm: redis.get(dailyCount) threw:', describeErr(err)); } + // Pending entry was already consumed atomically above. return c.json({ showForm: { name: 'ruleComposerForm', @@ -447,14 +502,14 @@ export function registerComposeRoutes(app: Hono): void { name: 'rule', label: 'Describe your rule in plain English (max 1000 characters)', type: 'paragraph', - defaultValue: rule, + defaultValue: pending.originalRule, helpText: 'Edit the sentence and re-compile.', }, { name: 'allowGuarded', label: 'Allow this rule to ban/mute (otherwise removes only)', type: 'boolean', - defaultValue: !!allowGuarded, + defaultValue: !!pending.allowGuarded, helpText: ALLOW_GUARDED_HELP, }, ], @@ -463,9 +518,12 @@ export function registerComposeRoutes(app: Hono): void { }); } + // Save branch — defence-in-depth re-validate (the redis entry could in + // principle have been tampered with, although the only writer is this + // very code path). let validated: RuleType; try { - validated = Rule.parse(JSON.parse(serializedRule)); + validated = Rule.parse(pending.validated); checkTreeDepth(validated.when as Parameters[0]); validatePredicateRegexes(validated.when as PredicateTreeShape); } catch (_err) { @@ -477,26 +535,17 @@ export function registerComposeRoutes(app: Hono): void { }); } - const subredditName = getCurrentSubredditName(); - const todayCounterKey = keys.compileCount(subredditName, todayKey()); - let todayCount = 0; - try { - todayCount = Number((await redis.get(todayCounterKey)) ?? '0'); - } catch (err) { - console.warn('[vibe-mod] confirm: redis.get(todayCount) threw — skipping quota:', describeErr(err)); - } - const persistResult = await persistRuleAndStartDryRun({ validated, subredditName, - tokensIn, - tokensOut, - llmModel, - usingBYOK, - todayCount, - todayCounterKey, + tokensIn: pending.tokensIn, + tokensOut: pending.tokensOut, + llmModel: pending.llmModel, }); + // (pending entry already consumed atomically at the top of the + // handler via WATCH/MULTI/DEL, so we don't need a second DEL here.) + if (persistResult.toast) { return c.json({ showToast: persistResult.toast }); } @@ -509,24 +558,48 @@ export function registerComposeRoutes(app: Hono): void { }); } +// Shape of the transient compile state stashed under +// `keys.composePending(sub, pendingId)` between compose-rule-submit (which +// writes it) and compose-confirm-submit (which reads + deletes it). +// Phase 2c rework so the confirm modal carries one short id instead of +// 7 disabled `(internal)` fields. +interface ComposePending { + validated: RuleType; + tokensIn: number; + tokensOut: number; + llmModel: string; + usingBYOK: boolean; + originalRule: string; + allowGuarded: boolean; +} + +// Crypto-random short id for the composePending Redis key. 12 chars of +// hex is enough to make collisions astronomically unlikely (1 in 16^12), +// while staying short enough that the disabled "(internal session id)" +// field at the bottom of the confirm modal doesn't dominate the UI. +function newPendingId(): string { + const bytes = new Uint8Array(6); + globalThis.crypto.getRandomValues(bytes); + return Array.from(bytes, (b) => b.toString(16).padStart(2, '0')).join(''); +} + // Persist a validated rule into the draft bundle and schedule a dry-run. -// Private to this module — only compose-confirm-submit calls it. +// Private to this module — only compose-confirm-submit calls it. The daily +// quota counter has already been bumped at compile time (see +// compose-rule-submit), so this helper no longer touches it. async function persistRuleAndStartDryRun(opts: { validated: RuleType; subredditName: string; tokensIn: number; tokensOut: number; llmModel: string; - usingBYOK: boolean; - todayCount: number; - todayCounterKey: string; }): Promise<{ persisted: boolean; dryRunQueued: boolean; lines: string[]; toast?: { text: string; appearance: 'neutral' | 'success' }; }> { - const { validated, subredditName, tokensIn, tokensOut, llmModel, usingBYOK, todayCount, todayCounterKey } = opts; + const { validated, subredditName, tokensIn, tokensOut, llmModel } = opts; const draftKey = keys.rulesDraft(subredditName); let draftJson: string | undefined; @@ -572,14 +645,10 @@ async function persistRuleAndStartDryRun(opts: { console.warn('[vibe-mod] persist: redis.set(draft) threw — rule NOT persisted:', describeErr(err)); } - if (!usingBYOK) { - try { - await redis.set(todayCounterKey, String(todayCount + 1)); - await redis.expire(todayCounterKey, 86_400); - } catch (err) { - console.warn('[vibe-mod] persist: redis.set(todayCount) threw — quota not incremented:', describeErr(err)); - } - } + // (Daily compile counter is incremented in compose-rule-submit, right + // after the OpenAI call returns — not here. The token cost is real + // regardless of whether the moderator clicks Save or Cancel on the + // confirm form, so the quota must reflect that. CodeRabbit #5 PR #49.) let dryRunQueued = true; try { @@ -596,15 +665,20 @@ async function persistRuleAndStartDryRun(opts: { // RuleBundle's strict schema accepts the partial we built; coerce safely. void RuleBundle; // mark used for tree-shaking awareness - const summary = summarizeRule(validated); - const lines = [`Compiled rule "${validated.name}". ${summary}.`]; + // Phase 2c demo-recording UX clean-up — Devvit toasts truncate at ~120 + // chars, so the previous 4-clause line ('Compiled rule "X". → trigger: + // action. Dry-run started — open ⋯ menu → "vibe-mod: View rules + log" + // to see preview.') was getting cut off mid-sentence in the recording. + // The compose-confirm form already showed the full humanizeRule output + // and the rule-name detail, so the toast just needs to confirm what + // actually happened. + let line: string; if (persisted && dryRunQueued) { - lines.push('Dry-run started — open the subreddit ⋯ menu → "vibe-mod: View rules + log" to see preview.'); + line = `Saved "${validated.name}". Dry-run starts now.`; } else if (persisted) { - lines.push('Saved as draft. Open the subreddit ⋯ menu → "vibe-mod: View rules + log".'); + line = `Saved "${validated.name}" as draft (dry-run unavailable).`; } else { - lines.push('Rule compiled but not persisted — plugin RPC unreachable (reddit/devvit#258).'); + line = `Compiled but not persisted — plugin RPC unreachable.`; } - - return { persisted, dryRunQueued, lines }; + return { persisted, dryRunQueued, lines: [line] }; } diff --git a/src/server/routes/dashboard.ts b/src/server/routes/dashboard.ts index dec6445..875f297 100644 --- a/src/server/routes/dashboard.ts +++ b/src/server/routes/dashboard.ts @@ -6,7 +6,7 @@ import type { Hono } from 'hono'; import { redis } from '@devvit/web/server'; -import type { MenuItemRequest, UiResponse } from '@devvit/web/shared'; +import type { FormField, MenuItemRequest, UiResponse } from '@devvit/web/shared'; import { keys } from '../../shared/redis-keys'; import { type RuleBundleType } from '../../shared/rule-schema'; import { getCurrentSubredditName } from '../devvit-helpers'; @@ -105,55 +105,94 @@ export function registerDashboardRoutes(app: Hono): void { const totalRules = (active?.rules.length ?? 0) + (draft?.rules.length ?? 0); const isEmpty = totalRules === 0 && recent.length === 0; - const summary = [ - ...(rpcOk - ? [] - : [ - '⚠ Plugin RPC unreachable (reddit/devvit#258 — OPEN platform bug).', - 'Persistence is offline; this view reflects what redis would return.', - '', - ]), - ...(firstVisit - ? [ - '👋 Welcome to vibe-mod. 3 quick steps:', - ' 1. We seeded 5 starter rules — see them below.', - ' 2. Open ⋯ → "vibe-mod: Manage rules" to activate one (shadow mode for 24h first).', - ' 3. Open ⋯ → "vibe-mod: Compose rule" to write your own in plain English.', - '', - ] - : []), - ...(isEmpty - ? ['No rules yet — open the subreddit ⋯ menu → "vibe-mod: Compose rule" to write your first rule.', ''] - : []), - `Active rules: ${active?.rules.length ?? 0}`, - `Draft rules: ${draft?.rules.length ?? 0}`, - `Recent actions: ${recent.length}`, - `Tokens used (lifetime): ${totalIn.toLocaleString()} in / ${totalOut.toLocaleString()} out (~$${totalCost.toFixed(4)} on ${llmModel}).`, - ...(dryRunLines.length ? ['', 'Dry-run preview (draft rules):', ...dryRunLines] : []), - '', - 'Recent actions:', - ...recent.slice(0, 10).map((r) => ` ${r.action} (${r.outcome}) — ${(r.ruleSourceNL ?? '').slice(0, 60)}…`), - ].join('\n'); + // Phase 2c demo-recording UX clean-up — Devvit's modal `description` + // collapses every \n into a single soft-wrapping paragraph, so the + // previous one-big-string version produced the unreadable wall of + // text the user flagged in the recording. Splitting the same content + // into multiple disabled-paragraph fields gives Devvit a separate + // block per section, which it renders with proper spacing. + const fields: FormField[] = []; + const addBlock = (name: string, label: string, value: string) => { + if (!value.trim()) return; + fields.push({ name, label, type: 'paragraph', defaultValue: value, disabled: true }); + }; + + if (!rpcOk) { + addBlock( + 'rpcWarning', + '⚠ Plugin RPC unreachable', + 'reddit/devvit#258 (OPEN platform bug). Persistence is offline; this view reflects what redis would return.', + ); + } + + if (firstVisit) { + addBlock( + 'welcome', + '👋 Welcome to vibe-mod', + '3 quick steps:\n1. We seeded 5 starter rules — see them below.\n2. Open ⋯ → "vibe-mod: Manage rules" to activate one (shadow mode for 24h first).\n3. Open ⋯ → "vibe-mod: Compose rule" to write your own in plain English.', + ); + } + + if (isEmpty) { + addBlock( + 'emptyState', + 'No rules yet', + 'Open the subreddit ⋯ menu → "vibe-mod: Compose rule" to write your first rule.', + ); + } + + addBlock( + 'counts', + 'At a glance', + `Active rules: ${active?.rules.length ?? 0}\nDraft rules: ${draft?.rules.length ?? 0}\nRecent actions: ${recent.length}`, + ); + + addBlock( + 'tokenCost', + 'Tokens used (lifetime)', + `${totalIn.toLocaleString()} in / ${totalOut.toLocaleString()} out\n(~$${totalCost.toFixed(4)} on ${llmModel})`, + ); + + if (dryRunLines.length) { + addBlock('dryRunPreview', 'Dry-run preview (draft rules)', dryRunLines.join('\n')); + } + + if (recent.length) { + addBlock( + 'recentActions', + 'Recent actions', + recent + .slice(0, 10) + .map((r) => `${r.action} (${r.outcome}) — ${(r.ruleSourceNL ?? '').slice(0, 60)}…`) + .join('\n'), + ); + } + + if (firstVisit) { + fields.push({ + name: 'dismissOnboarding', + label: 'Dismiss the welcome intro for this sub', + type: 'boolean', + defaultValue: false, + helpText: 'Tick to hide the 3-step intro on future visits.', + }); + } return c.json({ showForm: { name: 'dashboardForm', form: { title: 'vibe-mod Dashboard', - description: summary, + description: 'Read-only summary. Per-rule activation lives in "vibe-mod: Manage rules".', + // The Cancel button on a Devvit form does NOT trigger the submit + // handler, so labelling it "Don't show intro again" was actively + // misleading — clicking it would never actually persist the + // dismiss flag (Gemini #1 PR #49). The dismissOnboarding + // boolean toggle above is the real opt-out; submit (Close) sends + // the form values through to /internal/form/dashboard-action. acceptLabel: 'Close', - cancelLabel: firstVisit ? "Don't show intro again" : 'Cancel', - fields: firstVisit - ? [ - { - name: 'dismissOnboarding', - label: 'Dismiss the welcome intro for this sub', - type: 'boolean', - defaultValue: false, - helpText: 'Tick to hide the 3-step intro on future visits.', - }, - ] - : [], + cancelLabel: 'Cancel', + fields, }, }, }); diff --git a/src/shared/redis-keys.ts b/src/shared/redis-keys.ts index 5512751..9545a3c 100644 --- a/src/shared/redis-keys.ts +++ b/src/shared/redis-keys.ts @@ -53,6 +53,17 @@ export const keys = { /** "1" if the moderator dismissed the dashboard onboarding intro for this sub. */ onboardingDismissed: (sub: string) => `${sub}:onboarding:dismissed`, + + /** + * Transient state for the 2-step compose flow (Phase 2c UX clean-up): + * compose-rule-submit stores the validated rule + tokens + model under + * this key, then composeConfirmForm only carries the short pendingId so + * the modal stays clean (prior version surfaced 7 internal carrier + * fields). compose-confirm-submit reads + DELs the entry. 10 min TTL + * caps abandoned compiles; the moderator's daily compile counter has + * already been bumped, so a stale pending is just wasted tokens. + */ + composePending: (sub: string, id: string) => `${sub}:compose:pending:${id}`, } as const; /** Global keys that are NOT subreddit-scoped (rare). */ diff --git a/test/devvit-testkit.ts b/test/devvit-testkit.ts index 3b0b869..13bc9c1 100644 --- a/test/devvit-testkit.ts +++ b/test/devvit-testkit.ts @@ -48,6 +48,7 @@ export interface FakeTxn { exec: () => Promise; get: (k: string) => Promise; set: (k: string, v: string) => Promise; + del: (k: string) => Promise; expire: (k: string, ttl: number) => Promise; hSet: (k: string, fields: Record) => Promise; zAdd: (k: string, entry: { member: string; score: number }) => Promise; @@ -121,6 +122,7 @@ export function makeFakeRedis(): FakeRedis { exec: async () => [], get: base.get, set: base.set, + del: base.del, expire: base.expire, hSet: base.hSet, zAdd: base.zAdd,