Skip to content

settings: align Worklog defaults with live-to-final model#3868

Closed
franksong2702 wants to merge 3 commits into
nesquena:masterfrom
franksong2702:franksong2702/worklog-settings-cleanup-v2
Closed

settings: align Worklog defaults with live-to-final model#3868
franksong2702 wants to merge 3 commits into
nesquena:masterfrom
franksong2702:franksong2702/worklog-settings-cleanup-v2

Conversation

@franksong2702

@franksong2702 franksong2702 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • rename the old Activity expanded-default setting surface to Worklog details and keep the default folded
  • hide/deprecate the old Compact tool activity preference so it no longer implies a legacy transparent stream mode
  • keep the Worklog renderer enabled even when older installs saved simplified_tool_calling=false
  • make the Worklog details toggle apply immediately only when that toggle changes, without unrelated Appearance autosaves overriding per-turn manual expand/collapse choices

Scope

Refs #3400 and #3820.

This does not implement Transparent Stream and does not add a Compact Worklog / Transparent Stream selector. That remains the separate #3820 implementation track. This PR only aligns existing settings with the current Compact Worklog default model.

Verification

  • node --check static/boot.js static/panels.js static/ui.js static/i18n.js
  • python3 -m py_compile api/config.py tests/test_1003_preferences_autosave.py tests/test_issue3595_activity_default_expanded.py tests/test_simplified_tool_calling_setting.py tests/test_ui_tool_call_cleanup.py
  • pytest tests/test_ui_tool_call_cleanup.py tests/test_issue3595_activity_default_expanded.py tests/test_simplified_tool_calling_setting.py tests/test_1003_preferences_autosave.py tests/test_issue1824_cli_patch_diff_rendering.py
  • pytest tests/ -v --timeout=60 --shard-id=0 --num-shards=3
  • git diff --check
  • python3 scripts/scope_undef_gate.py attempted locally, skipped because eslint is not installed on PATH; GitHub CI lint passed the scope/undefined-reference gate

@greptile-apps

greptile-apps Bot commented Jun 9, 2026

Copy link
Copy Markdown

Greptile Summary

This PR renames the activity_feed_expanded_default setting to worklog_details_expanded_default, hard-codes simplified_tool_calling to true (making the Worklog renderer permanent), and ensures the toggle applies immediately to existing cards without autosave interactions overriding per-turn manual expand/collapse choices.

  • Settings rename with full migration path: Both load_settings and save_settings in api/config.py carry forward old installs via a one-way key migration; both old and new keys are handled on read, while legacy keys are stripped before writing. boot.js and panels.js include matching fallback logic for servers that still return the old key during rolling updates.
  • Worklog toggle now applies eagerly: The new _applyWorklogDetailsExpandedDefault() helper in ui.js is called only from the checkbox onchange handler — not from _autosaveAppearanceSettings — so unrelated appearance saves cannot override cards that a user has already manually expanded or collapsed.
  • simplified_tool_calling deprecated cleanly: The UI checkbox is removed, the key is added to _SETTINGS_LEGACY_DROP_KEYS so existing false values on disk are silently ignored, and _SETTINGS_ALLOWED_KEYS prevents it from being re-persisted by any client.

Confidence Score: 5/5

Safe to merge — all migration paths are well-guarded and the toggle-applies-immediately change is correctly scoped to the onchange handler only.

The rename is covered end-to-end: disk migration in both load and save, boot.js and panels.js fallback for old-server rolling updates, and the i18n/HTML surface. The decision to not call _applyWorklogDetailsExpandedDefault in the autosave handler is the key behavioral invariant, and the new test explicitly asserts it. The simplified_tool_calling deprecation is belt-and-suspenders: the key is removed from the UI, popped in save_settings, excluded from _SETTINGS_ALLOWED_KEYS, and blocked via _SETTINGS_LEGACY_DROP_KEYS on load, so existing installs with the legacy value on disk will silently fall through to the correct default.

No files require special attention.

Important Files Changed

Filename Overview
api/config.py Renames setting key and adds migration in both load and save paths; drops simplified_tool_calling from allowed/bool key sets and adds it to legacy-drop; all paths verified by updated tests.
static/boot.js Hardcodes _simplifiedToolCalling=true; initializes _worklogDetailsExpandedByDefault with a hasOwnProperty-gated fallback to the legacy key for old-server compat.
static/panels.js Autosave guard updated so _worklogDetailsExpandedByDefault is refreshed only when the payload carried the new key, and _applyWorklogDetailsExpandedDefault is never called from the autosave handler (preserving per-turn manual choices).
static/ui.js Adds _worklogDetailsExpandedDefault() helper and _applyWorklogDetailsExpandedDefault(); updates _thinkingCardHtml, buildToolCard, _syncToolRowsContainer, and ensureActivityGroup to use the new helpers instead of the deprecated flag.
static/index.html Renames checkbox ID and i18n keys; removes the deprecated Compact tool activity field entirely.
static/i18n.js Updates all 12 locale blocks to replace the old activity_feed_expanded_default label/desc keys with worklog_details_expanded_default; non-translated locales continue to carry English strings (pre-existing pattern).
tests/test_issue3595_activity_default_expanded.py Substantially expanded with round-trip migration tests, toggle-applies-immediately assertions, and an explicit check that autosave does not call _applyWorklogDetailsExpandedDefault.
tests/test_simplified_tool_calling_setting.py Tests updated to verify legacy false values on disk are ignored and the key is no longer in BOOL_KEYS or ALLOWED_KEYS.
tests/test_1003_preferences_autosave.py Removes simplified_tool_calling from the tracked preference fields list; count adjusted from 15 to 14.
tests/test_ui_tool_call_cleanup.py Inverts assertions for simplified_tool_calling UI presence; adds check that boot.js hardcodes the flag to true.
tests/test_issue1824_cli_patch_diff_rendering.py Adds _worklogDetailsExpandedDefault to the list of helpers injected into the Node eval harness and stubs window._worklogDetailsExpandedByDefault=false so buildToolCard can be exercised in isolation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Settings file on disk] -->|load_settings| B{Has worklog_details_expanded_default?}
    B -- No but has activity_feed_expanded_default --> C[Migrate: copy old to new key]
    B -- Yes --> D[Use new key directly]
    C --> D
    D --> E[Skip legacy keys via _SETTINGS_LEGACY_DROP_KEYS]
    E --> F[Return merged settings]
    F -->|boot.js| G[window._worklogDetailsExpandedByDefault with fallback]
    F -->|panels.js| H[Checkbox wired with hasOwnProperty fallback]
    H --> I[checkbox.onchange]
    I --> J[Update window flag]
    J --> K[_applyWorklogDetailsExpandedDefault: update all existing cards]
    K --> L[_scheduleAppearanceAutosave]
    L -->|POST /api/settings| M[save_settings: pop legacy keys, write new key]
    M --> N[_autosaveAppearanceSettings: refresh flag only, NO apply call]
Loading

Reviews (3): Last reviewed commit: "Trim dead Thinking markup branch" | Re-trigger Greptile

Comment thread static/ui.js Outdated
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Pulled the branch (ed48dc77) into a read-only worktree and read the full diff plus surrounding context in api/config.py, static/boot.js, static/ui.js, static/panels.js, and the locale blocks of static/i18n.js. The migration is solid and the rename is complete. A few confirmations and one cleanup-follow-up observation.

What I verified

Legacy migration round-trips correctly, both directions. load_settings seeds the new key from the old one when only the legacy key is on disk (api/config.py:5604):

if (
    "worklog_details_expanded_default" not in stored
    and "activity_feed_expanded_default" in stored
):
    settings["worklog_details_expanded_default"] = bool(
        stored.get("activity_feed_expanded_default")
    )

and save_settings migrates + pops the legacy key before persisting (config.py:5685). With both activity_feed_expanded_default and simplified_tool_calling added to _SETTINGS_LEGACY_DROP_KEYS and simplified_tool_calling removed from _SETTINGS_BOOL_KEYS / _SETTINGS_ALLOWED_KEYS, neither old key can round-trip back onto disk. test_legacy_activity_feed_setting_migrates_on_load_and_save exercises exactly that path.

Rename is total — no dangling references. grep for settingsActivityFeedExpandedDefault, _activityFeedExpandedDefault, and settings_label_activity_feed_expanded_default across static/ returns nothing on the branch; the checkbox id, window flag, and i18n keys (all 14 locale blocks of static/i18n.js) are renamed consistently to the Worklog variants.

Appearance-autosave wiring is consistent with the prior design. I checked whether the explicit Save Settings button (saveSettings(), panels.js:~7965) carries the new key — it doesn't, but neither did activity_feed_expanded_default on origin/master; that toggle has always lived in the Appearance section and persists via _appearancePayloadFromUi() + the onchange autosave (panels.js:6100, 6398). So the explicit-save body correctly only drops the deprecated simplified_tool_calling line and adds nothing — not a regression. The dual-write of both worklog_details_expanded_default and activity_feed_expanded_default in the appearance POST (panels.js:6107-6108) is the right call for rolling-update compat, since an older server still reading the legacy key gets a correct value.

Worklog renderer stays on regardless of stored simplified_tool_calling=false. boot.js:1797 and _applySavedSettingsUi (panels.js:7636) both hard-pin window._simplifiedToolCalling=true, so an old install that saved false no longer flips into a legacy stream mode — matches the PR intent and #3820's "Compact Worklog is the model" framing.

One cleanup follow-up (non-blocking)

Now that _simplifiedToolCalling is permanently true, isSimplifiedToolCalling() (ui.js:6439) always returns true, which makes a couple of branches dead. The clearest is in _thinkingMarkup (ui.js:10182):

const openClass=_worklogDetailsExpandedDefault()||!isSimplifiedToolCalling()?' open':'';

!isSimplifiedToolCalling() is now permanently false, so this reduces to _worklogDetailsExpandedDefault()?' open':''. Behaviorally correct, just carrying a dead disjunct. Greptile flagged the same spot plus the unused root parameter in _applyWorklogDetailsExpandedDefault (ui.js:6416). Both are harmless to leave, but since #3820's Transparent Stream work is likely to revisit these same render paths, it'd be worth either trimming the dead isSimplifiedToolCalling()-gated branches now or leaving a TODO so the next editor doesn't assume the toggle still has two live states.

Test plan

Per the execution ban I didn't run anything from the worktree. The Python migration is well covered by test_issue3595_activity_default_expanded.py (load+save round-trip, defaults, bool-keys membership, legacy-drop), and test_ui_tool_call_cleanup.py / test_1003_preferences_autosave.py correctly invert the simplified_tool_calling assertions and drop the field from the autosave set. Recommend the maintainer run those four suites plus node --check on the touched JS before merge.

@franksong2702

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I pushed 631ba853 to trim the dead _thinkingMarkup disjunct so the Thinking card now opens only from the Worklog details default:

const openClass = _worklogDetailsExpandedDefault() ? ' open' : '';

I also added a small regression assertion so the deprecated compact-tool toggle does not creep back into that fallback markup. I left _applyWorklogDetailsExpandedDefault(root) as-is because root is still used to scope the existing-card update when a caller passes a subtree.

CI is green on the updated head.

nesquena-hermes added a commit that referenced this pull request Jun 10, 2026
…#3892 #3898 #3885 #3882 #3868) (#3902)

* stage v0.51.347: render/stream cluster (#3892 #3898 #3885 #3882 #3868) + 2 Opus SHOULD-FIX

* stage v0.51.347: trim #3885 error-guard comment to fit diagnostic-test window

* Stamp v0.51.347 — Release LK (streaming & render reliability cluster)

* Remove stray uv.lock accidentally staged (not part of any cluster PR)

---------

Co-authored-by: nesquena-hermes <[email protected]>
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Shipped in v0.51.347 (Release LK — streaming & render reliability cluster) 🎉 — live now.

Merged via the combined release PR #3902 alongside #3892, #3898, #3885, #3882, and #3868 (all in the live-to-final streaming/render family). Each was applied onto fresh master, gated through Codex (SAFE TO SHIP) + Opus (ship, no MUST-FIX) + the full suite (8532 passing), with two Opus SHOULD-FIX folded in before merge. Authorship preserved via co-authored commit + CHANGELOG credit.

Thanks for the fix! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants