Conversation
📝 WalkthroughWalkthroughRenamed config key Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/components/MiscView.tsx (1)
37-41:ensureMisc()mutates the wrong object via stale closure—mutations are lost.
ensureMisc()reads/writes the outersettings(lines 38-40) but is called insideupdateSettings(settings => { ... }). SinceupdateSettingscreates a deep copy and only preserves mutations to the parameter, mutations to the outer scope closure are discarded. This meansensureMisc()has no effect.Proposed refactor (pass draft parameter instead of using closure)
- const ensureMisc = () => { - if (!settings.misc) { - settings.misc = { ...defaultMisc }; - } - }; + const ensureMisc = (draft: typeof settings) => { + if (!draft.misc) { + draft.misc = { ...defaultMisc }; + } + }; // ... in toggle functions: - toggle: () => { - updateSettings(settings => { - ensureMisc(); - settings.misc!.hideCtrlGToEdit = !settings.misc!.hideCtrlGToEdit; - }); - }, + toggle: () => { + updateSettings(draft => { + ensureMisc(draft); + draft.misc!.hideCtrlGToEdit = !draft.misc!.hideCtrlGToEdit; + }); + },Also applies to: 67-72, 80-86, 94-100, 108-114, 123-128, 136-141, 148-154, 161-167, 175-181
🧹 Nitpick comments (2)
src/patches/hideCtrlGToEdit.ts (1)
8-9: Consider pattern flexibility for potential future Claude Code format changes.The regex pattern is intentionally specific to match the minified bundle format (
if(X&&Y)p("tengu_external_editor_hint_shown",), as documented in the code comment. This specificity helps avoid false positives but creates fragility if Claude Code's bundler output changes (e.g., added whitespace, additional operands, or different operators). If the bundle format evolves, this pattern will silently fail to match. An optional refactor would be to make the pattern more flexible (e.g.,/if\(\s*([$\w\s&|]+)\s*\)p\("tengu_external_editor_hint_shown",/) if and when such changes occur.src/ui/components/MiscView.tsx (1)
25-35: Avoid duplicated defaults drift indefaultMisc.Now that
defaultMiscis being updated (Line 31), consider sourcing it from a single canonical place (e.g.,DEFAULT_SETTINGS.misc) to prevent the UI’s “initialization defaults” from diverging from actual config defaults.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.mdsrc/config.tssrc/defaultSettings.tssrc/migration.tssrc/patches/hideCtrlGToEdit.tssrc/patches/hideCtrlGToEditPrompt.tssrc/patches/index.tssrc/patches/toolsets.tssrc/types.tssrc/ui/components/MiscView.tsx
💤 Files with no reviewable changes (2)
- src/patches/toolsets.ts
- src/patches/hideCtrlGToEditPrompt.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/migration.ts (1)
src/types.ts (1)
TweakccConfig(143-151)
src/config.ts (1)
src/migration.ts (1)
migrateHideCtrlGToEditPrompt(93-99)
src/patches/index.ts (1)
src/patches/hideCtrlGToEdit.ts (1)
writeHideCtrlGToEdit(31-46)
src/patches/hideCtrlGToEdit.ts (1)
src/patches/index.ts (2)
LocationResult(71-75)showDiff(89-127)
🔇 Additional comments (7)
src/patches/hideCtrlGToEdit.ts (1)
31-46: LGTM! Clean patch implementation.The function correctly:
- Returns null on pattern match failure
- Replaces the condition with
falseto disable the Ctrl+G hint- Calls
showDifffor verbose debugging output- Preserves surrounding code structure
src/types.ts (1)
113-113: LGTM! Clean property rename.The property rename from
hideCtrlGToEditPrompttohideCtrlGToEditis straightforward and consistent with the PR objectives.src/migration.ts (1)
89-99: LGTM! Correct migration implementation.The migration function properly:
- Checks for the old property's existence before migrating
- Copies the value to the new property name
- Removes the old property to avoid confusion
- Is idempotent (safe to run multiple times)
- Follows the same pattern as existing migrations in the file
src/config.ts (2)
11-14: LGTM! Correct import addition.The migration function is properly imported alongside the existing userMessageDisplay migration.
251-252: LGTM! Proper migration integration.The migration is correctly positioned in the config read flow and will run automatically to update existing configurations with the renamed property.
src/patches/index.ts (1)
60-62: Rename wiring for ctrl-g patch is consistent.Import + gate + writer call all use
hideCtrlGToEdit/writeHideCtrlGToEdit.Please confirm the config migration runs before
applyCustomization(...)is invoked (so old configs still enable the patch). A quick pointer would be wheresrc/migration.tsis called in the config load flow.Also applies to: 666-669
src/defaultSettings.ts (1)
747-757: Default rename is correct and fully integrated.
DEFAULT_SETTINGS.misc.hideCtrlGToEdit(Line 753) aligns with the new config key. The old keyhideCtrlGToEditPromptexists only in migration code for backward compatibility, confirming the rename is complete. The new key is properly defined in types, used in the UI, and integrated into the patch system.
Co-authored-by: bl-ue <54780737+bl-ue@users.noreply.github.com>
Summary by CodeRabbit
Bug Fixes
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.