* fix(settings): honor persisted auto-follow across boot failures and partial bodies (#6819) The Auto-follow new content toggle was silently re-enabled by two paths: - boot.js settings-fetch-failure fallback hardcoded _autoScrollFollow=true, clobbering an explicit OFF for the whole session (post-turn scroll yanks until refresh); - _applySavedSettingsUi assigned body.auto_scroll_follow!==false unconditionally, so a partial settings body without the key evaluated undefined!==false -> true. Fix: mirror the resolved value into profile-namespaced localStorage (JSON map keyed by active profile), make the boot fallback read the mirror instead of hardcoding ON, and only override the runtime flag when a settings body actually owns the key. Fresh users (no mirror) still default to ON. Tests: update test_issue4006 fallback assertion; add test_issue6819_auto_scroll_follow_persistence covering source-shape guards and extracted-helper behavior (fresh default, persisted OFF, profile isolation, partial-body no-clobber, save-path persist). * fix(settings): resolve auto-follow mirror profile from cookie before S.activeProfile init (#6856 review) P1 (greptile-apps review): the profile-namespaced mirror was keyed by S.activeProfile, but the boot settings-fetch-failure fallback runs before boot resolves S.activeProfile, so a non-default profile with a failed fetch read the 'default' mirror entry. Resolve the key from the hermes_profile cookie (set per-client by /api/profile/switch, readable synchronously at page load) first, falling back to S.activeProfile, then 'default'. P2: document the persistence contract in the helper block (storage shape, profile-key resolution order, read semantics, upgrade path). Tests: extend the extracted-helper node test with the P1 scenario (cookie authoritative over stale S.activeProfile, per-profile isolation via cookie, no-cookie fallback) and add source-shape guards for the cookie read. * fix(settings): defer auto-follow mirror read until profile resolves (P1 follow-up) greptile-apps review: the hermes_profile cookie is HttpOnly — document.cookie can never read it, so the cookie-based profile key from 39fed70b always fell back to 'default', re-introducing the wrong-namespace bug for non-default profiles on a failed settings fetch. Correct model: the active profile is only known asynchronously via /api/profile/active. The boot settings-fetch-failure path now sets a safe default (ON) plus a deferred-reapply flag; once S.activeProfile resolves, the mirror is read with the profile-correct namespace and the flag clears. The success path (which already applied+persisted the authoritative value) never touches the flag. Tests updated to the deferred model: source-shape guard asserts the re-apply runs AFTER profile resolution (fallback < profile-resolve < reapply ordering), no document.cookie read in the profile key, and the node behavioral test simulates boot-failure -> profile-resolve -> deferred re-apply. * fix(settings): defer auto-follow mirror write until profile resolves (P1 round 3) greptile-apps review: the SUCCESS boot path called _persistAutoScrollFollow before S.activeProfile initialized, so a non-default profile's successful settings fetch stored its value in the 'default' namespace. A later failed-fetch boot would then find no entry for the real profile and restore auto-follow ON. The success path now defers the mirror write the same way the failure path defers its read: it records the value in _autoScrollFollowDeferredPersist, and the post-profile-resolution block persists it under the correct namespace (then clears the flag). Both deferred branches now run together after S.activeProfile resolves. Tests: success path must not call _persistAutoScrollFollow before profile resolution (source-shape), deferred-persist anchor ordered after profile resolve, and the node behavioral test covers success-boot -> profile-resolve -> deferred write under the resolved profile (and NOT under default). * fix(settings): make auto-follow mirror global to match the backend contract (maintainer review) Maintainer review on #6856: the mirror was profile-keyed, but the backend authority is GLOBAL — SETTINGS_FILE = STATE_DIR / 'settings.json' (api/config.py), and /api/settings calls load_settings() with no profile-specific file. A profile-keyed map leaves a freshly-opened profile with no mirror entry, so a failed settings fetch on that profile wrongly restored ON despite the global OFF — the original failure stayed reproducible. Revert to a single global last-known value ('1'/'0' under one key): - _persistAutoScrollFollow / _readPersistedAutoScrollFollow now use a plain scalar, not a JSON profile map. - Boot success path persists synchronously (no deferred write — the global mirror has no profile dimension). - Boot failure fallback reads synchronously (no deferred re-apply — no profile resolution needed). - Remove _autoScrollFollowProfileKey, _autoScrollFollowDeferredPersist, _autoScrollFollowDeferredReapply entirely. Tests: behavioral node test now asserts the maintainer-requested regression (global OFF survives profile change + failed fetch, single scalar storage); source-shape guards assert the profile map and deferred machinery are gone; boot success persists; partial-body and save-path assertions unchanged. 14/14 pass. * fix(settings): never persist synthesized auto-follow from a failed autosave (Greptile P1) Greptile review on #6856: in _autosaveAppearanceSettings, when the POST fails (saved is falsy), '!saved' synthesizes _autoScrollFollow=true, and the added persist call wrote that synthesized ON into the mirror. A later boot settings-fetch failure would then restore ON from the corrupted mirror, reintroducing post-turn scroll yanks for a user who explicitly disabled auto-follow. Persist ONLY from an explicit boolean in the server response: if(saved && typeof saved.auto_scroll_follow==='boolean' && typeof _persistAutoScrollFollow==='function'){ _persistAutoScrollFollow(saved.auto_scroll_follow); } A failed save or a partial response without the key leaves the mirror untouched (last authoritative value preserved). Tests: source-shape guard asserts the explicit-boolean requirement; the node behavioral test replicates the failed-autosave path (mirror stays OFF) and the successful-autosave path (explicit boolean persists). * fix(settings): sync auto-follow mirror on successful settings-panel GET Gate review (Codex) found a stale-mirror gap in #6856's own contract: a successful loadSettingsPanel() GET applied the authoritative auto_scroll_follow value to window._autoScrollFollow but did not persist the global mirror, so an explicit OFF applied via the panel left a stale ON mirror that a later boot-fetch failure would restore (reopening #6819). Persist from the panel GET too, only when the server sent an explicit boolean (never a synthesized default) — matching the boot + autosave contract. Adds a bite-tested regression. Co-authored-by: sand01chi <sand01chi@users.noreply.github.com> --------- Co-authored-by: sand01chi <sand01chi@users.noreply.github.com> Co-authored-by: nesquena-hermes <nesquena-hermes@users.noreply.github.com>
191 lines
9.4 KiB
Python
191 lines
9.4 KiB
Python
"""Pin the auto_scroll_follow setting's default + hydration consistency (#4006).
|
|
|
|
The viewport-follow default is `True` (Codex/Claude-Code-style sticky bottom:
|
|
follow new output to the bottom while streaming, but a deliberate scroll-up
|
|
unpins and is respected). This pins the default in every place it is read so a
|
|
future edit can't silently flip it or, worse, default it ON in config.py while
|
|
hydrating it OFF in the browser (the classic default-mismatch bug, where an
|
|
existing user with no saved value sees the feature as disabled).
|
|
"""
|
|
import json
|
|
import pathlib
|
|
import re
|
|
import shutil
|
|
import subprocess
|
|
import textwrap
|
|
|
|
import pytest
|
|
|
|
ROOT = pathlib.Path(__file__).resolve().parent.parent
|
|
|
|
|
|
def _read(rel):
|
|
return (ROOT / rel).read_text(encoding="utf-8")
|
|
|
|
|
|
def test_auto_scroll_follow_default_is_true_in_config():
|
|
src = _read("api/config.py")
|
|
assert re.search(r'["\']auto_scroll_follow["\']\s*:\s*True', src), (
|
|
"auto_scroll_follow must default to True in _SETTINGS_DEFAULTS "
|
|
"(sticky-bottom follow; scroll-up unpins)"
|
|
)
|
|
|
|
|
|
def test_auto_scroll_follow_in_bool_keys():
|
|
src = _read("api/config.py")
|
|
m = re.search(r"_SETTINGS_BOOL_KEYS\s*=\s*\{([^}]+)\}", src, re.DOTALL)
|
|
assert m, "_SETTINGS_BOOL_KEYS not found"
|
|
assert "auto_scroll_follow" in m.group(1), (
|
|
"auto_scroll_follow must be in _SETTINGS_BOOL_KEYS so it round-trips as a bool"
|
|
)
|
|
|
|
|
|
def test_boot_hydration_defaults_true_when_setting_absent():
|
|
"""boot.js must hydrate _autoScrollFollow as True when the saved settings
|
|
omit the key — `!!s.auto_scroll_follow` would wrongly default it OFF for
|
|
every existing user, contradicting the config.py default."""
|
|
src = _read("static/boot.js")
|
|
# Settings path: default-true read (=== false), not the truthy-coerce form,
|
|
# and the resolved value is persisted into the #6819 mirror (global —
|
|
# matches the one global settings.json, per maintainer review on #6856).
|
|
assert "window._autoScrollFollow=_persistAutoScrollFollow(s.auto_scroll_follow!==false)" in src, (
|
|
"boot.js settings path must default _autoScrollFollow True when absent "
|
|
"(and persist the resolved value into the #6819 mirror)"
|
|
)
|
|
assert "window._autoScrollFollow=!!s.auto_scroll_follow" not in src, (
|
|
"boot.js must not use !!s.auto_scroll_follow — that defaults the True "
|
|
"setting OFF for users with no saved value"
|
|
)
|
|
# Fallback (no-settings) path must also default true for a fresh user
|
|
# (no persisted mirror), while honoring a persisted OFF (#6819).
|
|
assert "window._autoScrollFollow=_readPersistedAutoScrollFollow()" in src, (
|
|
"boot.js fallback path must read the persisted auto-follow mirror "
|
|
"(defaults true for fresh users, honors saved OFF after a transient "
|
|
"settings-fetch failure)"
|
|
)
|
|
|
|
|
|
def test_settings_checkbox_renders_checked_by_default():
|
|
"""The Appearance checkbox must render checked when the setting is absent,
|
|
matching the True default (panels.js settings-load)."""
|
|
src = _read("static/panels.js")
|
|
assert "autoScrollFollowCb.checked=settings.auto_scroll_follow!==false" in src, (
|
|
"the auto-follow checkbox must default checked (=== false), not "
|
|
"!!settings.auto_scroll_follow which would render it unchecked by default"
|
|
)
|
|
|
|
|
|
def test_follow_gate_references_auto_scroll_follow_and_unpin():
|
|
"""The DOM-replace follow gate must consult both _autoScrollFollow (the
|
|
setting) and _messageUserUnpinned (the user's scroll-up), so the opt-out and
|
|
the read-while-streaming behaviors both hold."""
|
|
src = _read("static/ui.js")
|
|
assert "_shouldFollowMessagesOnDomReplace" in src
|
|
assert "_autoScrollFollow" in src and "_messageUserUnpinned" in src, (
|
|
"the follow gate must reference both _autoScrollFollow and _messageUserUnpinned"
|
|
)
|
|
|
|
|
|
def test_ui_sync_init_defaults_auto_scroll_follow_before_consumers():
|
|
"""ui.js must establish window._autoScrollFollow synchronously at the
|
|
top-level scroll-state section — before boot.js hydrates it from the awaited
|
|
settings request. ui.js/messages.js load before boot.js (index.html), so any
|
|
scroll listener / settle path / scrollIfPinned / DOM-replace gate running in
|
|
that window reads an undeclared binding and throws ReferenceError (#6606).
|
|
|
|
The typeof guard makes the default `true` WITHOUT clobbering an explicit
|
|
saved `false` if boot.js already hydrated it."""
|
|
src = _read("static/ui.js")
|
|
assert "if(typeof window._autoScrollFollow==='undefined'){ window._autoScrollFollow=true; }" in src, (
|
|
"ui.js must synchronously default window._autoScrollFollow to true before "
|
|
"boot.js hydration (bare reads otherwise throw ReferenceError pre-boot)"
|
|
)
|
|
# The sync init must sit BEFORE every consumer it protects in the file.
|
|
init_idx = src.index("window._autoScrollFollow=true;")
|
|
scroll_listener_idx = src.index("!movedUp && window._autoScrollFollow && _scrollPinned")
|
|
gate_idx = src.index("window._autoScrollFollow && !_messageUserUnpinned")
|
|
settle_idx = src.index("!window._autoScrollFollow&&!explicit")
|
|
pinned_idx = src.index("function scrollIfPinned")
|
|
for label, idx in (
|
|
("scroll listener", scroll_listener_idx),
|
|
("DOM-replace gate", gate_idx),
|
|
("settle guard", settle_idx),
|
|
("scrollIfPinned", pinned_idx),
|
|
):
|
|
assert init_idx < idx, f"sync init must precede the {label} consumer"
|
|
|
|
|
|
def test_ui_readers_use_explicit_window_owner():
|
|
"""Every _autoScrollFollow reader in ui.js must go through the explicit
|
|
window._autoScrollFollow owner — no bare undeclared reads that can throw
|
|
ReferenceError before boot.js assigns the global (#6606)."""
|
|
src = _read("static/ui.js")
|
|
# Any bare `_autoScrollFollow` token not preceded by `window.` is a bug.
|
|
for m in re.finditer(r"(?<![\w.])_autoScrollFollow", src):
|
|
before = src[max(0, m.start() - 7):m.start()]
|
|
assert before == "window.", (
|
|
f"bare _autoScrollFollow read at ui.js offset {m.start()}: "
|
|
f"...{src[max(0, m.start()-40):m.start()+20]!r}"
|
|
)
|
|
|
|
|
|
@pytest.mark.skipif(shutil.which("node") is None, reason="node required for behavioral test")
|
|
def test_auto_scroll_follow_undefined_true_false_behavior():
|
|
"""Behavioral: extract the real follow expressions from ui.js and evaluate
|
|
them in Node across undefined / true / false:
|
|
|
|
- undefined must NOT throw (property read, not a bare undeclared binding)
|
|
and must not block an explicit bottom settlement;
|
|
- false must suppress automatic following (DOM-replace gate, settle guard,
|
|
scrollIfPinned) WITHOUT suppressing explicit bottom settlement;
|
|
- true keeps following enabled."""
|
|
src = _read("static/ui.js")
|
|
# Extract the exact live expressions (not hand-copied replicas).
|
|
dom_replace_expr = "window._autoScrollFollow && !_messageUserUnpinned && (_scrollPinned || _isMessagePaneNearBottom(120))"
|
|
assert dom_replace_expr in src, "DOM-replace follow gate expression changed"
|
|
settle_expr = "(!window._autoScrollFollow&&!explicit)||!_scrollPinned||_messageUserUnpinned||_recentNonMessageScrollIntent()"
|
|
assert settle_expr in src, "settle guard expression changed"
|
|
pinned_guard_expr = "!window._autoScrollFollow"
|
|
assert f"if({pinned_guard_expr}) return;" in src, "scrollIfPinned guard changed"
|
|
|
|
def run(expr, follow, explicit=False, pinned=True, unpinned=False, near=True):
|
|
follow_js = "undefined" if follow is None else ("true" if follow else "false")
|
|
harness = textwrap.dedent(f"""
|
|
const window = {{ _autoScrollFollow: {follow_js} }};
|
|
let _scrollPinned = {str(pinned).lower()};
|
|
let _messageUserUnpinned = {str(unpinned).lower()};
|
|
let explicit = {str(explicit).lower()};
|
|
function _isMessagePaneNearBottom(px){{ return {str(near).lower()}; }}
|
|
function _recentNonMessageScrollIntent(){{ return false; }}
|
|
let _out;
|
|
try {{
|
|
_out = ({expr});
|
|
}} catch (e) {{
|
|
console.error('THREW: ' + e);
|
|
process.exit(2);
|
|
}}
|
|
console.log(JSON.stringify(_out === undefined ? '__UNDEF__' : _out));
|
|
""")
|
|
res = subprocess.run(["node", "-e", harness], capture_output=True, text=True, timeout=30)
|
|
assert res.returncode == 0, res.stderr
|
|
val = json.loads(res.stdout.strip())
|
|
return None if val == "__UNDEF__" else val
|
|
|
|
# undefined: property read → falsy, never throws, explicit settle still allowed.
|
|
assert not run(dom_replace_expr, None) # undefined && … → undefined (falsy)
|
|
assert run(settle_expr, None) is True # suppress implicit settle
|
|
assert run(settle_expr, None, explicit=True) is False # explicit settle proceeds
|
|
assert run(pinned_guard_expr, None) is True # scrollIfPinned returns early
|
|
# true: follow enabled everywhere.
|
|
assert run(dom_replace_expr, True) is True
|
|
assert run(settle_expr, True) is False # implicit settle proceeds
|
|
assert run(pinned_guard_expr, True) is False # scrollIfPinned does NOT early-return
|
|
# false: automatic follow suppressed…
|
|
assert run(dom_replace_expr, False) is False
|
|
assert run(settle_expr, False) is True
|
|
assert run(pinned_guard_expr, False) is True
|
|
# …but an EXPLICIT bottom settlement still runs (false must not block ↓/open).
|
|
assert run(settle_expr, False, explicit=True) is False
|
|
# Unpinned reader is never re-followed, regardless of the setting.
|
|
assert run(dom_replace_expr, True, unpinned=True) is False
|