Refactor GeneralTab settings sections#1123
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe monolithic ChangesGeneralTab Module Decomposition
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: one or more packages not found in the registry. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR splits the 1,698-line
Confidence Score: 4/5Safe to merge for most users; the storage migration path has a conditional UX gap worth addressing before it reaches production. The refactoring is mechanical and well-tested. The one substantive concern is in chooseSyncFolder: if the sync IPC handler ever returns success: true alongside a non-empty errors array, the restart-required banner and migration count are both silently hidden because both UI guards check !syncError. This is a latent defect that depends on backend behavior; it does not affect the happy path tested today, but it would produce confusing UX when it triggers. src/renderer/components/Settings/tabs/GeneralTab/hooks/useSyncStorageState.ts — specifically the unconditional result.errors override at the bottom of the chooseSyncFolder inner try block. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[chooseSyncFolder called] --> B[selectSyncFolder IPC]
B --> C{folder selected?}
C -- No --> D[return early]
C -- Yes --> E[setSyncMigrating true\nsetSyncError null\nsetSyncMigratedCount null]
E --> F[setCustomPath IPC]
F --> G{result.success?}
G -- Yes --> H[setCustomSyncPath\nsetCurrentStoragePath\nsetSyncRestartRequired true\nsetSyncMigratedCount]
G -- No --> I[setSyncError via syncResultErrorMessage]
H --> J{result.errors?.length > 0?}
I --> J
J -- Yes --> K[setSyncError errors.join]
J -- No --> L[setSyncMigrating false]
K --> L
L --> M[UI: restart banner only if syncRestartRequired AND NOT syncError]
L --> N[UI: migration count only if syncMigratedCount AND NOT syncError]
K -.->|syncError now set| O[restart banner and migration count hidden]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[chooseSyncFolder called] --> B[selectSyncFolder IPC]
B --> C{folder selected?}
C -- No --> D[return early]
C -- Yes --> E[setSyncMigrating true\nsetSyncError null\nsetSyncMigratedCount null]
E --> F[setCustomPath IPC]
F --> G{result.success?}
G -- Yes --> H[setCustomSyncPath\nsetCurrentStoragePath\nsetSyncRestartRequired true\nsetSyncMigratedCount]
G -- No --> I[setSyncError via syncResultErrorMessage]
H --> J{result.errors?.length > 0?}
I --> J
J -- Yes --> K[setSyncError errors.join]
J -- No --> L[setSyncMigrating false]
K --> L
L --> M[UI: restart banner only if syncRestartRequired AND NOT syncError]
L --> N[UI: migration count only if syncMigratedCount AND NOT syncError]
K -.->|syncError now set| O[restart banner and migration count hidden]
Reviews (1): Last reviewed commit: "Refactor GeneralTab settings sections" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+55
to
+67
| if (result.success) { | ||
| setCustomSyncPath(folder); | ||
| setCurrentStoragePath(folder); | ||
| setSyncRestartRequired(true); | ||
| if (result.migrated !== undefined) { | ||
| setSyncMigratedCount(result.migrated); | ||
| } | ||
| } else { | ||
| setSyncError(syncResultErrorMessage(result, 'Failed to change storage location')); | ||
| } | ||
| if (result.errors && result.errors.length > 0) { | ||
| setSyncError(result.errors.join(', ')); | ||
| } |
There was a problem hiding this comment.
When the backend returns { success: true, errors: ['some-warning'], migrated: 3 }, the success branch correctly sets syncRestartRequired = true and syncMigratedCount = 3, but then the unconditional check at lines 65-67 immediately sets syncError = 'some-warning'. In the UI, both the restart banner (syncRestartRequired && !syncError) and the migration count display (syncMigratedCount !== null && !syncError) are gated on syncError being falsy, so both become invisible. The user sees only the error text with no indication that the migration succeeded and a restart is required.
The same result.errors join is also already handled inside syncResultErrorMessage on the failure path, making the standalone block redundant for that case and problematic for the success case.
Comment on lines
+18
to
+32
| const loadShells = useCallback(async () => { | ||
| if (shellsLoaded) return; | ||
| setShellsLoading(true); | ||
| try { | ||
| const detected = await window.maestro.shells.detect(); | ||
| setShells(detected); | ||
| if (detected && detected.length > 0) { | ||
| setShellsLoaded(true); | ||
| } | ||
| } catch (error) { | ||
| logger.error('Failed to load shells:', undefined, error); | ||
| } finally { | ||
| setShellsLoading(false); | ||
| } | ||
| }, [shellsLoaded]); |
There was a problem hiding this comment.
useMaestroCliState and useSyncStorageState both use isOpen to re-fetch on every tab open. useShellSettingsState intentionally uses shellsLoaded as a one-time guard instead, meaning a shell installed after the first detection (e.g., the user installs Homebrew Fish while Maestro is open) will not appear even after closing and reopening Settings. The hook has no mechanism to reset shellsLoaded when the tab is closed. Whether this is intentional for performance or an oversight is worth confirming, as it is the only hook of the four that does not follow the isOpen-reset pattern established in this PR.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/renderer/components/Settings/tabs/GeneralTab/hooks/useShellSettingsState.ts (1)
24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant null check on array.
The
detectedvariable is typed asShellInfo[]from thewindow.maestro.shells.detect()promise, so it will always be an array. The null/undefined check is unnecessary.♻️ Simplify the condition
- if (detected && detected.length > 0) { + if (detected.length > 0) { setShellsLoaded(true); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/components/Settings/tabs/GeneralTab/hooks/useShellSettingsState.ts` at line 24, The condition checking `detected && detected.length > 0` contains a redundant null check since the `detected` variable is already typed as `ShellInfo[]` from the `window.maestro.shells.detect()` promise, which means it will always be an array and never null or undefined. Simplify the condition to only check `detected.length > 0` to remove the unnecessary null check and make the code cleaner.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/renderer/components/Settings/tabs/GeneralTab/components/BrowserSection.tsx`:
- Around line 142-144: The onChange handler for setBrowserTabKeepAliveLimit only
enforces a minimum value of 1 using Math.max but does not enforce the upper
bound of 100, allowing values above 100 to propagate to the state despite the
max={100} attribute on the input. Modify the onChange handler to clamp the
parsed value between 1 and 100 by wrapping the Math.max expression with Math.min
to ensure both the lower and upper bounds are enforced before calling
setBrowserTabKeepAliveLimit.
In
`@src/renderer/components/Settings/tabs/GeneralTab/components/ConductorProfileSection.tsx`:
- Around line 26-37: The textarea element for the conductor profile input lacks
an accessible name for screen readers. Add either a label element with htmlFor
attribute that references a unique id on the textarea element, or add an
aria-label attribute directly to the textarea. Ensure the textarea has an id
property if using the label approach, and make sure the accessible name clearly
describes the field's purpose (e.g., "Conductor Profile" or similar).
In
`@src/renderer/components/Settings/tabs/GeneralTab/components/InputBehaviorSection.tsx`:
- Around line 32-35: The fallback value in the forcedParallelShortcut variable
uses a hardcoded macOS-specific keyboard representation that is not appropriate
for Windows or Linux users. Replace the hardcoded string '⌘ ⇧ ↩' with a
platform-aware approach that detects the operating system and returns the
correct keyboard shortcut representation based on whether the user is on macOS,
Windows, or Linux.
In
`@src/renderer/components/Settings/tabs/GeneralTab/components/PowerSection.tsx`:
- Line 3: Replace the import statement for isLinuxPlatform from
utils/platformUtils with an import of isLinux from
src/shared/platformDetection.ts. Then update any calls to isLinuxPlatform()
within the PowerSection component to use isLinux() instead, ensuring consistency
with the repository's shared platform detection contract.
In
`@src/renderer/components/Settings/tabs/GeneralTab/hooks/useShellSettingsState.ts`:
- Around line 27-28: In the useShellSettingsState hook, the catch block that
handles shell detection failures is missing Sentry error reporting. Add a
captureException call after the logger.error statement to report shell detection
failures to Sentry, following the same pattern used in other settings hooks like
useSyncStorageState and useMaestroCliState. The captureException should accept
the error (converting it to an Error instance if it's not already one) and
include an extra object with the action context set to 'maestro.shells.detect'
to provide debugging context.
In
`@src/renderer/components/Settings/tabs/GeneralTab/hooks/useSyncStorageState.ts`:
- Around line 65-67: The conditional block checking `result.errors` at lines
65-67 is redundant because the `syncResultErrorMessage` utility function called
at line 63 already handles extracting and processing errors from the result
object. Remove the entire if block that checks `if (result.errors &&
result.errors.length > 0)` and the corresponding `setSyncError` call, as the
error handling is already correctly implemented through the
`syncResultErrorMessage` function which properly sets the error message based on
the result state.
---
Nitpick comments:
In
`@src/renderer/components/Settings/tabs/GeneralTab/hooks/useShellSettingsState.ts`:
- Line 24: The condition checking `detected && detected.length > 0` contains a
redundant null check since the `detected` variable is already typed as
`ShellInfo[]` from the `window.maestro.shells.detect()` promise, which means it
will always be an array and never null or undefined. Simplify the condition to
only check `detected.length > 0` to remove the unnecessary null check and make
the code cleaner.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48e6fb05-e748-43b4-9eb6-7658b0ad6751
📒 Files selected for processing (33)
src/__tests__/renderer/components/Settings/tabs/GeneralTab/hooks.test.tsxsrc/__tests__/renderer/components/Settings/tabs/GeneralTab/sections.test.tsxsrc/renderer/components/Settings/tabs/GeneralTab.tsxsrc/renderer/components/Settings/tabs/GeneralTab/GeneralTab.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/AutoRunInactivitySection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/BrowserSection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/ConductorProfileSection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/GitHubCliSection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/GlobalHotkeySection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/HistorySection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/InputBehaviorSection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/LogLevelSection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/MaestroCliSection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/PowerSection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/PrivacySection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/RenderingSection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/ShellSettingsSection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/SpellCheckSection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/StorageLocationSection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/TabBehaviorSection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/ThinkingModeSection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/UpdatesSection.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/index.tssrc/renderer/components/Settings/tabs/GeneralTab/hooks/index.tssrc/renderer/components/Settings/tabs/GeneralTab/hooks/useForcedParallelWarningState.tssrc/renderer/components/Settings/tabs/GeneralTab/hooks/useMaestroCliState.tssrc/renderer/components/Settings/tabs/GeneralTab/hooks/useShellSettingsState.tssrc/renderer/components/Settings/tabs/GeneralTab/hooks/useSyncStorageState.tssrc/renderer/components/Settings/tabs/GeneralTab/index.tssrc/renderer/components/Settings/tabs/GeneralTab/types.tssrc/renderer/components/Settings/tabs/GeneralTab/utils/browserDefaults.tssrc/renderer/components/Settings/tabs/GeneralTab/utils/index.tssrc/renderer/components/Settings/tabs/GeneralTab/utils/syncErrors.ts
💤 Files with no reviewable changes (1)
- src/renderer/components/Settings/tabs/GeneralTab.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactors
GeneralTabfrom a 1,698-line monolith into a directory-based Settings tab with a 161-line shell, focused section components, side-effect hooks, local utilities, and expanded regression coverage.Behavior is intended to remain unchanged. Public imports are preserved through the new
GeneralTab/index.tsbarrel.Changes
Settings/tabs/GeneralTab.tsxintoSettings/tabs/GeneralTab/.GeneralTabas the onlyuseSettings()caller.data-setting-idvalues for Settings search parity.Testing
npm run test -- src/__tests__/renderer/components/Settings/tabs/GeneralTab.test.tsx src/__tests__/renderer/components/Settings/tabs/GeneralTab src/__tests__/renderer/components/Settings/searchableSettings.test.tsnpm run test -- src/__tests__/renderer/components/Settings/tabs/DisplayTab.test.tsx src/__tests__/renderer/components/Settings/tabs/EncoreTab.test.tsxnpm run format:checknpm run lintnpm run lint:eslintnpm run testManual QA Checklist
Summary by CodeRabbit