Add quick actions menu with [cmd|ctrl] + K#106
Conversation
WalkthroughAdds a QuickActions command palette and global shortcut; introduces a modal subsystem (provider, BaseModal, SearchableModal, hooks, focus-trap); implements a settings registry, types, and definitions; adds new settings UI components and fields; adds fuzzy-match utility; updates store UI flags and TS target; and adjusts tests to the new UI state. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant KeyHandler as GlobalKey
participant Store
participant QuickActions
participant Registry
User->>KeyHandler: Press Cmd/Ctrl+K
KeyHandler->>App: keydown
App->>Store: toggleQuickActions(true)
Store-->>App: isQuickActionsOpen = true
App->>QuickActions: render (auto-focus input)
User->>QuickActions: type query
QuickActions->>Registry: search(query)
Registry-->>QuickActions: results
User->>QuickActions: Enter
QuickActions->>Store: navigate/select (server/channel/dm/setting)
QuickActions->>Store: toggleQuickActions(false)
sequenceDiagram
participant User
participant ChannelList
participant Store
participant App
participant UserSettings
participant Registry
User->>ChannelList: click settings button
ChannelList->>Store: toggleSettingsModal(true)
Store-->>App: isSettingsModalOpen = true
App->>UserSettings: render
UserSettings->>Registry: getGrouped()
Registry-->>UserSettings: grouped settings
User->>UserSettings: change setting
UserSettings->>Registry: validate(settingId, value)
User->>UserSettings: save
UserSettings->>Store: persist and clearSettingsNavigation()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Areas to focus review on:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
Automated deployment preview for the PR in the Cloudflare Pages. |
matheusfillipe
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (19)
src/store/index.ts (1)
668-688: Consider whether closing modals should also resetsettingsNavigationThe implementations of
toggleQuickActions,setSettingsNavigation, andclearSettingsNavigationare straightforward and correct. One thing to think about: if the settings modal or quick actions are closed, do you always want any previoussettingsNavigationhighlight to be cleared, or is reusing the last highlight intentional? If not intentional, you might want to callclearSettingsNavigationwhen closing the settings/quick actions modals.Also applies to: 2273-2281, 2311-2327
src/lib/fuzzySearch.ts (1)
1-53: Fuzzy matcher is well-structured; clarify empty-query behaviorThe scoring and early-return paths (exact match, substring, then sequential matching) are consistent and efficient for a command palette use case. One nuance: because
targetLower.includes(queryLower)is true forquery === "", an empty query will always return{ matches: true, score: 80 }. If you ever want “no query” to mean “no fuzzy filter applied” or “no match,” you may want to special-casequery.trim().length === 0before running the matcher.src/lib/modal/hooks/useModal.ts (1)
21-90: Modal hooks look solid; route-change handling is conservative
useModal’s open/close/toggle helpers and the mounted guard are correct, and thepopstatelistener is scoped to “modal is open + closeOnRouteChange” which avoids leaks.useModalscorrectly clones theSetwhen mutating. If you ever need to react to non-popstateroute changes (e.g., client router events), you might want to optionally accept a custom close trigger instead of hard-codingwindow.popstate, but as-is this is perfectly fine.Also applies to: 95-140
src/lib/modal/utils/focusTrap.ts (1)
18-40: Focus trap implementation is robust; options are effectively staticThe focus trap correctly constrains Tab navigation, supports Escape/outer-click deactivation, restores prior focus, and toggles body scroll. The React
useFocusTraphook manages activation and cleanup as expected. Note thattrapOptionsare only applied when the trap is first created; later changes to options (e.g., togglingpreventScrollorclickOutsideDeactivateswhile active) won’t affect the existing instance. If you ever need truly dynamic behavior, you may want to recreate the trap when those options change.Also applies to: 45-238, 243-280
src/lib/modal/hooks/useKeyboardNavigation.ts (2)
30-125: Keyboard navigation core is good; consider disabled items in grid movesThe navigation helpers and global key handling cover the common palette behaviors well (arrows, Home/End, PageUp/Down, Tab cycling). One edge case: in
orientation: "grid", the ArrowUp/Down branches computeprevRow/nextRowdirectly and don’t consultdisabledIndices, so the highlight can land on a disabled item even though linear navigation paths usefindNextValidIndex. If you rely on disabled indices heavily in grids, it might be worth reusingfindNextValidIndex(or a grid-aware variant) for those moves as well.Also applies to: 141-260
291-326: Guard command shortcuts when focus is in text inputs
useCommandShortcut’s parser and matching logic are straightforward and handle ctrl/cmd variations correctly. Because it listens onwindowand only checks the key/modifier combo, it will also fire while the user is typing in an<input>,<textarea>, or contentEditable region. For a command palette shortcut likectrl+k/cmd+k, you may want to ignore events originating from editable elements (and possibly respectevent.defaultPrevented) to avoid surprising the user while they’re editing text:const target = event.target as HTMLElement | null; const isEditable = !target || target.isContentEditable || ["INPUT", "TEXTAREA", "SELECT"].includes(target.tagName); if (isEditable) return;This keeps the global shortcut behavior without hijacking typing contexts.
src/lib/modal/context/ModalProvider.tsx (1)
87-146: Clarify the distinction betweenopenModalandpushModalbehavior.The
openModalfunction respects themodalSettings.stackModalssetting (lines 92-95), replacing the current modal whenstackModalsis false. However,pushModal(lines 135-146) always pushes to the stack regardless of thestackModalssetting. This inconsistency could confuse API consumers.Consider:
- Documenting this behavioral difference in JSDoc comments
- Or making
pushModalalso respectstackModalsfor consistency- Or renaming to make the intent clearer (e.g.,
forceStackModal)src/components/ui/settings/CustomMentionsField.tsx (1)
1-80: LGTM! Clean implementation of the custom mentions field.The component correctly manages the mentions list with add/remove functionality and keyboard support. The structure is clear and follows React best practices.
Optional: Consider case-insensitive duplicate detection.
Line 18 uses a case-sensitive check, meaning "test" and "Test" would both be allowed. If case-insensitive matching is desired:
- if (newMention.trim() && !mentions.includes(newMention.trim())) { + const trimmed = newMention.trim(); + if (trimmed && !mentions.some(m => m.toLowerCase() === trimmed.toLowerCase())) {src/hooks/useModalBehavior.ts (1)
3-45: Modal behavior hook looks solid; only very minor optional refinementsThe escape handling and click‑outside wiring are clean, and using a ref for
onCloseavoids stale closures while keeping the effect deps minimal. For symmetry, you could optionally also callonCloseRef.current()ingetBackdropProps(instead of closing overonClose) so both escape and backdrop always use the latest callback, but the current approach is already correct for typical React usage.src/lib/settings/types.ts (1)
6-246: Type system is solid; consider aligningSettingControlTypewith actual renderersThe type definitions are nicely structured and give you room to grow (validation, dependencies, search metadata, quick actions, etc.).
One thing to watch:
SettingControlTypealready includes"multiselect","checkbox","date", and"time", butSettingRenderercurrently doesn’t implement branches for those. If a new setting is added with one of those types, it will fall through to the “Unknown setting type” path.Options:
- Either add at least minimal renderers for these types, or
- Keep the union restricted to types that are actually supported today and extend it as you add renderers.
That keeps the type system honest and avoids surprises when someone defines a new setting based solely on the union.
src/components/ui/UserSettings.tsx (4)
271-401: Settings initialization and change tracking are well thought outThe pattern of:
- Initializing a local
settingsmap fromglobalSettingsonce per modal open,- Capturing a deep‑cloned
originalValuessnapshot (including profile metadata and account fields), and- Deriving
hasUnsavedChangesviadeepEqualon the settings map + explicit checks for scalar fields,is a solid approach. It keeps the Save button logic simple and avoids reference pitfalls.
As long as
globalSettingsis the single source of truth andsettingsRegistryis immutable at runtime, this should behave reliably across repeated open/close cycles.
203-255: Highlight/scroll navigation works, but timers aren’t cleared on unmountThe
ui.settingsNavigationhandling plus delayed scroll into view is a nice touch; it should make command‑palette navigation feel smooth.One edge case: you clear
highlightTimeoutRefandscrollTimeoutRefwhenui.isSettingsModalOpenflips tofalse, but there’s no cleanup function in this effect for the component unmount path. If the component were ever unmounted while a timeout is pending (e.g., rapid navigation or hot reload), the queued callbacks would still run and callsetHighlightedSettingon an unmounted component.Consider adding an effect cleanup:
useEffect(() => { if (!ui.settingsNavigation) return; // ... existing logic ... return () => { if (highlightTimeoutRef.current) { clearTimeout(highlightTimeoutRef.current); highlightTimeoutRef.current = null; } if (scrollTimeoutRef.current) { clearTimeout(scrollTimeoutRef.current); scrollTimeoutRef.current = null; } }; }, [ui.settingsNavigation, clearSettingsNavigation]);Not critical in most cases, but it makes the navigation logic more robust.
687-905: Profile highlighting is wired; account settings aren’t yet integrated with quick‑action highlightingThe custom
renderProfileFieldssection:
- Uses ids like
id="setting-profile.displayName"and- Applies highlight styles when
highlightedSetting === "profile.displayName",which lines up nicely with the
profile.*ids fromallSettings.tsand thesettingsNavigation.highlightedSettingIdlogic.In contrast,
renderAccountFieldsrenders the operator/account UI without:
- Assigning
id="setting-account.*"to its sections, or- Hooking into
highlightedSettingat all.That means command‑palette navigation to account‑related settings (e.g., anything with an
account.*id in the registry) will successfully switch the active category to “Account”, but it can’t scroll to or visually highlight a specific field the way it does for profile/other categories.If you want a consistent experience for quick actions, consider:
- Mirroring the profile pattern for relevant account fields:
- Wrapping each block in a div with
id={setting-account.}and highlight classes based onhighlightedSetting.- Or, model the oper/account pieces as proper
SettingDefinitions and render them viaSettingField, similar to the other categories, to reuse the existing highlight and navigation wiring.Also applies to: 907-979, 1013-1055
662-680: Modal integration is correct; consider future reuse with shared modal infrastructureUsing
useModalBehaviorhere to handle escape and backdrop clicks, and wiring those props onto the outer container/content, is correct and keeps modal behavior consistent with other parts of the app.If you continue to expand modal usage, you might later want to factor this layout into a shared BaseModal‑style component (since this file already has a fair bit of modal markup and styling), but that’s purely an optional refactor.
Also applies to: 982-989
src/lib/settings/definitions/allSettings.ts (1)
9-508: Registry is idempotent; design patterns are solidThe registry implementation uses a
Map<string, SettingDefinition>with ID-based keying, so repeated calls toregisterAllSettings()are safe—each call simply overwrites entries with the same ID rather than creating duplicates or corrupting state. The concern about idempotency is resolved.The broader suggestions from the original comment remain worth keeping in mind:
Account/oper password settings
- These are still defined as
"text"type with TODO comments. When you wire them into the UI:
- Use a dedicated control type (e.g.
"password") so the renderer can use<input type="password">.- Ensure sensitive values never leak into logs, exports, or debug output.
- The registry itself doesn't expose special handling for secrets, so control it at the store/renderer layer.
Account field highlighting
- Account settings definitions exist in the registry but aren't yet surfaced with
id="setting-account.*"anchors in the quick-action renderer, so command-palette navigation won't highlight them. Mirror the profile pattern when you implement the UI.src/lib/modal/SearchableModal.tsx (2)
60-66: Type and lifecycle ofsearchTimeoutRef
searchTimeoutRefis typed asuseRef<NodeJS.Timeout>()(Line 64). In a browser‑only React app this can cause friction unless Node types are globally available, and there’s no cleanup on unmount (even though the callback only callsonSearch).A safer pattern is:
- const searchTimeoutRef = useRef<NodeJS.Timeout>(); + const searchTimeoutRef = useRef<ReturnType<typeof setTimeout>>();Optionally, add a cleanup effect:
useEffect(() => { return () => { if (searchTimeoutRef.current) { clearTimeout(searchTimeoutRef.current); } }; }, []);Not strictly required for correctness, but it tightens typing and lifecycle.
184-191: Consider resetting query when the modal is reopenedCurrently,
queryandselectedIndexpersist across open/close cycles because the component stays mounted and there’s no effect that clears state whenisOpentoggles from false → true. For a Cmd/Ctrl+K palette, it’s often preferable to start with an empty query and first item selected on each open.If desired, you could add:
useEffect(() => { if (baseModalProps.isOpen) { setQuery(""); setSelectedIndex(0); } }, [baseModalProps.isOpen]);If the “remember last query” behavior is intentional, feel free to ignore this.
src/lib/settings/registry.ts (2)
27-100:"preferences"category metadata is missing frominitializeCategories
SettingCategoryincludes"preferences", butinitializeCategories(Lines 27–95) does not define metadata for this category. As a result, any settings withcategory: "preferences"will:
- Not appear in
getAllCategories/getGrouped.- Have no title/description metadata for navigation UIs.
Either add a
"preferences"entry here, or remove it fromSettingCategoryif it’s not meant to be used. For example:{ + id: "preferences", + title: "Preferences", + description: "General app preferences", + order: 3, // adjust orders of following categories accordingly + },and renumber subsequent
orderfields as needed.
185-196:searchIndexis maintained but not used in scoring
updateSearchIndexbuilds and stores asearchableTextarray (Lines 185–195), butsearchnever uses it beyond agetthat’s not referenced:const searchableText = this.searchIndex.get(setting.id) || []; // `searchableText` is never read afterwardsEither:
- Use
searchableTextas the basis for scoring (e.g., iterating over it when checking terms), or- Remove
searchIndexand the related maintenance if you don’t need precomputed fields.This will simplify the registry and avoid unnecessary work on registration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
package.json(1 hunks)src/App.tsx(4 hunks)src/components/layout/ChannelList.tsx(3 hunks)src/components/ui/QuickActions.tsx(1 hunks)src/components/ui/UserSettings.tsx(12 hunks)src/components/ui/settings/CustomMentionsField.tsx(1 hunks)src/components/ui/settings/IgnoreListField.tsx(1 hunks)src/components/ui/settings/SettingRenderer.tsx(1 hunks)src/hooks/useModalBehavior.ts(1 hunks)src/index.css(1 hunks)src/lib/fuzzySearch.ts(1 hunks)src/lib/modal/BaseModal.tsx(1 hunks)src/lib/modal/SearchableModal.tsx(1 hunks)src/lib/modal/context/ModalProvider.tsx(1 hunks)src/lib/modal/hooks/useKeyboardNavigation.ts(1 hunks)src/lib/modal/hooks/useModal.ts(1 hunks)src/lib/modal/index.ts(1 hunks)src/lib/modal/utils/focusTrap.ts(1 hunks)src/lib/settings/definitions/allSettings.ts(1 hunks)src/lib/settings/index.ts(1 hunks)src/lib/settings/registry.ts(1 hunks)src/lib/settings/types.ts(1 hunks)src/store/index.ts(7 hunks)tests/App.test.tsx(1 hunks)tests/components/ChatArea.test.tsx(2 hunks)tests/components/MetadataDisplay.test.tsx(2 hunks)tests/components/UserSettings.test.tsx(1 hunks)tests/lib/nicknameRetry.test.ts(4 hunks)tests/protocol/mode.test.ts(2 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (15)
src/components/ui/settings/CustomMentionsField.tsx (1)
src/lib/settings/types.ts (1)
SettingComponentProps(165-171)
src/lib/settings/types.ts (1)
src/lib/settings/index.ts (16)
SettingValue(23-23)SettingControlType(13-13)SettingValidation(22-22)SettingDependency(15-15)SettingDefinition(14-14)SettingCategory(10-10)SettingOption(17-17)SettingComponentProps(12-12)CategoryMetadata(9-9)SubcategoryMetadata(24-24)SettingSearchResult(19-19)SettingChangeEvent(11-11)SettingsState(21-21)SettingsExport(20-20)SettingGroup(16-16)SettingQuickAction(18-18)
src/components/ui/settings/SettingRenderer.tsx (1)
src/lib/settings/types.ts (2)
SettingDefinition(56-110)SettingValue(6-11)
src/components/ui/QuickActions.tsx (4)
src/lib/settings/types.ts (1)
SettingSearchResult(176-185)src/types/index.ts (3)
Channel(68-88)PrivateChat(90-106)Server(25-48)src/lib/settings/registry.ts (1)
settingsRegistry(436-436)src/lib/fuzzySearch.ts (1)
fuzzyMatch(6-53)
src/components/ui/settings/IgnoreListField.tsx (2)
src/lib/settings/types.ts (1)
SettingComponentProps(165-171)src/lib/ignoreUtils.ts (1)
isValidIgnorePattern(87-114)
src/lib/modal/SearchableModal.tsx (1)
src/lib/modal/BaseModal.tsx (2)
BaseModalProps(6-19)BaseModal(21-139)
src/lib/settings/definitions/allSettings.ts (4)
src/lib/settings/types.ts (1)
SettingDefinition(56-110)src/components/ui/settings/CustomMentionsField.tsx (1)
CustomMentionsField(9-78)src/components/ui/settings/IgnoreListField.tsx (1)
IgnoreListField(10-123)src/lib/settings/registry.ts (1)
settingsRegistry(436-436)
src/lib/modal/hooks/useKeyboardNavigation.ts (1)
src/lib/modal/index.ts (4)
UseKeyboardNavigationOptions(22-22)UseKeyboardNavigationReturn(23-23)useKeyboardNavigation(27-27)useCommandShortcut(26-26)
src/lib/modal/hooks/useModal.ts (1)
src/lib/modal/index.ts (4)
UseModalOptions(30-30)UseModalReturn(31-31)useModal(35-35)useModals(36-36)
src/App.tsx (1)
src/components/ui/UserSettings.tsx (1)
UserSettings(124-1092)
src/lib/modal/BaseModal.tsx (1)
src/lib/modal/index.ts (2)
BaseModalProps(5-5)BaseModal(7-7)
tests/components/UserSettings.test.tsx (1)
src/store/index.ts (2)
loadSavedServers(108-110)serverSupportsMetadata(189-189)
src/lib/modal/context/ModalProvider.tsx (1)
src/lib/modal/index.ts (8)
ModalType(13-13)ModalState(12-12)ModalContextValue(9-9)ModalSettings(11-11)ModalProviderProps(10-10)ModalProvider(17-17)useModalContext(18-18)useModalForType(19-19)
src/lib/settings/registry.ts (1)
src/lib/settings/types.ts (7)
SettingDefinition(56-110)SettingCategory(126-138)CategoryMetadata(143-150)SettingQuickAction(236-246)SettingSearchResult(176-185)SettingGroup(225-231)SettingValue(6-11)
src/components/ui/UserSettings.tsx (5)
src/lib/settings/types.ts (1)
SettingValue(6-11)src/lib/ircClient.ts (2)
sendRaw(767-780)changeNick(1103-1105)src/hooks/useModalBehavior.ts (1)
useModalBehavior(10-45)src/lib/settings/registry.ts (1)
settingsRegistry(436-436)src/components/ui/settings/SettingRenderer.tsx (1)
SettingField(238-297)
🔇 Additional comments (21)
src/index.css (1)
187-195: Blink animation is correctly applied with one-time iteration.The animation is properly used in
src/components/ui/settings/SettingRenderer.tsx(line 264) with the inline styleanimation: "blink 0.5s ease-in-out 1". The trailing1in the CSS animation shorthand setsanimation-iteration-count: 1, which matches the comment's intent of "one time only." The keyframes definition is syntactically correct and functioning as intended.src/store/index.ts (1)
370-422: Quick actions and settings navigation state wiring looks consistent
isQuickActionsOpenandsettingsNavigationare correctly added toUIState, initialized in the default UI slice, and exposed via matching actions inAppState. The toggle and setter/clearer mirror existing UI patterns, so this should integrate cleanly with the new command palette and settings flows.src/lib/modal/context/ModalProvider.tsx (1)
1-223: LGTM! Well-structured modal management system.The implementation provides a comprehensive, type-safe modal management solution with:
- Clear separation between single-modal and stacked-modal modes
- Flexible runtime configuration via
ModalSettings- Convenient per-type hooks via
useModalForType- Proper React patterns (Context, useCallback with deps)
The API surface is well-designed and should support the QuickActions feature effectively.
tests/App.test.tsx (1)
190-191: LGTM! Test correctly updated for settings modal refactor.The test now validates
toggleSettingsModalinstead oftoggleUserProfileModal, aligning with the centralized settings modal management introduced in this PR.tests/components/ChatArea.test.tsx (1)
78-100: LGTM! Test state updated for new UI fields.The addition of
isQuickActionsOpenandsettingsNavigationto the test setup correctly reflects the extended UI state introduced in this PR.tests/components/MetadataDisplay.test.tsx (1)
140-162: LGTM! Test state updated for new UI fields.The test setup now includes the new
isQuickActionsOpenandsettingsNavigationfields, maintaining consistency with the updated UI state structure.tests/lib/nicknameRetry.test.ts (1)
47-71: LGTM! Test state updated for new UI fields.Mock state correctly extended with
isQuickActionsOpenandsettingsNavigationto match the updated store structure.tests/protocol/mode.test.ts (1)
32-56: LGTM! Test state updated for new UI fields.The test initialization now includes
isQuickActionsOpenandsettingsNavigation, completing the consistent test state updates across all test files.package.json (1)
36-36: The fuse.js dependency is current and secure—no action required.Version 7.1.0 is the latest release of fuse.js, and no publicly reported CVEs or direct vulnerabilities exist for the npm package fuse.js v7.1.0 in major vulnerability databases.
src/components/layout/ChannelList.tsx (1)
41-41: LGTM! Consistent refactoring to the new settings modal system.The rename from
toggleUserProfileModaltotoggleSettingsModalis applied consistently across all three locations (destructuring and both onClick handlers), aligning with the centralized modal management introduced in this PR.Also applies to: 1279-1279, 1344-1344
src/App.tsx (2)
154-167: LGTM! Proper implementation of global keyboard shortcut.The keyboard listener correctly handles both Cmd (macOS) and Ctrl (Windows/Linux), prevents default browser behavior, and properly cleans up the event listener on unmount.
179-180: LGTM! Clean modal rendering integration.The conditional rendering of UserSettings and QuickActions aligns with the new centralized modal state management system.
src/components/ui/settings/IgnoreListField.tsx (1)
1-125: LGTM! Robust implementation with thorough validation.The component correctly validates ignore patterns using
isValidIgnorePattern, provides clear user feedback for validation errors, and implements duplicate detection. The UI includes helpful placeholder text and examples for the expected format.tests/components/UserSettings.test.tsx (2)
3-12: Acknowledged: Smoke test approach.The comment clearly explains the rationale for using smoke tests instead of full render tests. This is a reasonable pragmatic decision when component testing becomes overly complex due to store dependencies, especially if the component works correctly in production.
15-80: LGTM! Mock structure properly validates dependencies.The smoke tests verify that:
- The store mock structure is correct
loadSavedServersreturns expected server dataserverSupportsMetadatareturns the expected value- All mock functions are properly defined
This provides basic confidence that the module wiring is correct.
src/components/ui/QuickActions.tsx (2)
50-147: LGTM! Comprehensive search implementation with intelligent scoring.The search logic effectively combines:
- Settings search via the registry (limited to 10 results)
- Fuzzy matching across servers, channels, and private chats
- Smart score adjustments (-30 for currently selected items, +20 for items on current server)
- Results sorted by score and truncated to 15 items
The score adjustments are particularly well-designed, deprioritizing already-selected items while boosting relevance of items from the active server.
210-254: LGTM! Proper keyboard navigation with ref pattern.The keyboard handling correctly uses refs to avoid stale closures in the event listener. This is the right pattern when the handler needs to reference frequently-changing state (searchResults, selectedIndex) while being set up once in a useEffect.
src/lib/settings/index.ts (1)
1-45: LGTM! Clean public API surface for the settings library.The index file properly organizes exports for registry, types, and settings definitions. The commented-out hooks section includes a clear note about future integration with the store.
src/lib/modal/BaseModal.tsx (1)
1-141: LGTM! Well-implemented accessible modal component.The BaseModal provides:
- Portal rendering to document.body
- Proper focus management (saves and restores previous focus)
- Keyboard support (ESC to close)
- Click-outside-to-close functionality
- Full accessibility attributes (role, aria-modal, aria-labelledby)
- Configurable max widths and animation options
The 50ms setTimeout for focus management (line 58-60) is a common pattern to wait for DOM updates and is acceptable.
src/lib/modal/index.ts (1)
5-46: Modal barrel exports look consistentThe barrel cleanly re-exports modal components, hooks, and utilities with clear grouping comments. No functional issues detected; API surface reads cohesive and discoverable.
src/lib/settings/registry.ts (1)
435-441: Export pattern: class is value-private but type-exportedYou’re exporting a singleton instance (
settingsRegistry) plusexport type { SettingsRegistry };while keeping the class itself non‑exported as a value. This is a nice pattern to prevent ad‑hoc instances while still allowing typed parameters.If you ever need to expose the class as a constructible value, you’d have to promote it to
export class SettingsRegistry, but as long as the singleton is the intended usage, this setup is good.
| // Notification sound file | ||
| const [notificationSoundFile, setNotificationSoundFile] = | ||
| useState<File | null>(null); |
There was a problem hiding this comment.
Custom notification sound setting is currently non‑functional
The new notification sound plumbing appears incomplete:
- You declare
const [notificationSoundFile, setNotificationSoundFile] = useState<File | null>(null);. - You define
playNotificationSoundthat can handleFile | string. - In
handleSave, you have logic to convertnotificationSoundFileto a data URL and persist it viaupdateGlobalSettings({ notificationSound: dataUrl }).
However:
setNotificationSoundFileis never called anywhere in this component.- The generic
"file"SettingRenderer only passesfile.namethroughonChange, so noFileever reaches this state. playNotificationSoundis never invoked.
As a result, the "Notification Sound" setting lets the user pick a file, but there is no path that actually reads the file or persists its contents; only the filename (via the setting value) is stored.
To make this work, you could either:
-
Move file reading into the renderer (recommended for simplicity)
Update the"file"branch inSettingRendererto read the selected file and pass a data URL string as theSettingValue(see my other comment inSettingRenderer.tsx). Then you can:- Drop
notificationSoundFilestate and the FileReader logic inhandleSave. - Rely on
updateGlobalSettings(settings)to persist the data URL under the setting’s id.
- Drop
-
Keep renderer generic, but wire
notificationSoundFileexplicitly here- Provide a custom component for the notification sound setting (via
customComponent) that:- Calls both
onChange(for whatever you want to store as the SettingValue) and - Updates
setNotificationSoundFilewith the rawFile.
- Calls both
- Keep
handleSave’s FileReader logic as the single place that convertsFile→ data URL →updateGlobalSettings.
- Provide a custom component for the notification sound setting (via
Right now, the user‑visible “custom notification sound” affordance is misleading because it never actually takes effect.
Also applies to: 563-585, 615-623
| // Scroll selected item into view | ||
| useEffect(() => { | ||
| const selectedElement = resultsRef.current?.children[ | ||
| selectedIndex | ||
| ] as HTMLElement; | ||
| if (selectedElement) { | ||
| selectedElement.scrollIntoView({ block: "nearest", behavior: "smooth" }); | ||
| } | ||
| }, [selectedIndex]); |
There was a problem hiding this comment.
Selected item scrolling uses the wrong DOM element
resultsRef.current?.children[selectedIndex] (Line 195) indexes direct children of the results container, but each child is a category wrapper <div>, not an individual result. With categories enabled, selectedIndex is an index into filteredItems, so the scrolled element will not reliably correspond to the selected item (and with a single category, it always selects the same wrapper).
Consider tracking the selected item element via a dedicated ref and scrolling that instead, e.g.:
+ const selectedItemRef = useRef<HTMLDivElement | null>(null);
...
- useEffect(() => {
- const selectedElement = resultsRef.current?.children[
- selectedIndex
- ] as HTMLElement;
- if (selectedElement) {
- selectedElement.scrollIntoView({ block: "nearest", behavior: "smooth" });
- }
- }, [selectedIndex]);
+ useEffect(() => {
+ if (selectedItemRef.current) {
+ selectedItemRef.current.scrollIntoView({
+ block: "nearest",
+ behavior: "smooth",
+ });
+ }
+ }, [selectedIndex]);
...
- {categoryItems.map((item, index) => {
+ {categoryItems.map((item, index) => {
const globalIndex = filteredItems.indexOf(item);
const isSelected = globalIndex === selectedIndex;
-
- return (
- <div key={item.id}>
+ return (
+ <div
+ key={item.id}
+ ref={isSelected ? selectedItemRef : undefined}
+ >
{renderItem
? renderItem(item, isSelected, query)
: defaultRenderItem(item, isSelected)}
</div>
);
})}This keeps the scroll behavior aligned with the actual selected result.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lib/modal/SearchableModal.tsx around lines 193 to 201, the effect
currently calls resultsRef.current?.children[selectedIndex] which targets
category wrapper DIVs instead of the actual result element when categories are
enabled; replace this approach by creating a dedicated ref (e.g.,
selectedItemRef) that you assign to the rendered result element when its index
matches selectedIndex, update the effect to call
selectedItemRef.current?.scrollIntoView({ block: "nearest", behavior: "smooth"
}), and remove reliance on resultsRef.children; ensure the per-item ref is
applied only to the selected item, stays up-to-date when filteredItems or
selectedIndex change, and clean up or leave undefined when no selection exists.
| // Required check | ||
| if (validation.required && !value) { | ||
| return { valid: false, error: "This field is required" }; | ||
| } |
There was a problem hiding this comment.
required validation incorrectly rejects false and 0
The required check (Lines 348–351):
if (validation.required && !value) {
return { valid: false, error: "This field is required" };
}treats any falsy value as missing. This will incorrectly fail validation for legitimate values like false or 0, which are common for boolean/number settings.
Consider a more explicit emptiness check, e.g.:
- if (validation.required && !value) {
- return { valid: false, error: "This field is required" };
- }
+ const isEmpty =
+ value === null ||
+ value === undefined ||
+ (typeof value === "string" && value.trim().length === 0) ||
+ (Array.isArray(value) && value.length === 0);
+
+ if (validation.required && isEmpty) {
+ return { valid: false, error: "This field is required" };
+ }This keeps false and 0 valid while still enforcing non‑empty strings/arrays.
🤖 Prompt for AI Agents
In src/lib/settings/registry.ts around lines 348 to 351, the required check uses
a generic falsy test which incorrectly rejects valid values like false and 0;
replace the "!value" test with an explicit emptiness check that treats
null/undefined or truly empty containers as missing while accepting false and 0
— e.g. test value === null || value === undefined || (typeof value === 'string'
&& value.trim() === '') || (Array.isArray(value) && value.length === 0); return
the same error only when that explicit emptiness check is true.
| "target": "ES2022", | ||
| "useDefineForClassFields": true, | ||
| "lib": ["ES2020", "DOM", "DOM.Iterable"], | ||
| "lib": ["ES2022", "DOM", "DOM.Iterable"], |
There was a problem hiding this comment.
🧩 Analysis chain
Verify browser compatibility for ES2022 target.
Upgrading from ES2020 to ES2022 enables newer JavaScript features (top-level await, class fields, Array.at(), etc.). Ensure that your target browsers/environments support ES2022, or that your build pipeline includes appropriate transpilation/polyfills.
🏁 Script executed:
#!/bin/bash
# Check if there's a browserslist config or similar compatibility declaration
fd -t f "browserslist|.browserslistrc" . --exec cat {}
# Check vite config for build target settings
fd -t f "vite.config" . --exec cat {}Length of output: 2128
I found a critical mismatch that requires immediate attention. Let me search for additional configuration:
Let me also check if there's any transpilation or polyfill setup:
🏁 Script executed:
#!/bin/bash
# Check package.json for build and transpilation configuration
fd -t f "package.json" . --exec cat {}Length of output: 2313
🏁 Script executed:
#!/bin/bash
# Look for any .babelrc, tsconfig configuration, or build setup
fd -t f "babel.config|\.babelrc|tsconfig\.json" . --exec head -30 {}Length of output: 552
I've found a critical mismatch. Let me verify the compatibility details:
I need to verify what ES2022 features are actually being used in the codebase:
Now let me check if there are any actual ES2022-specific features being used that couldn't be transpiled:
Configuration verified—browser compatibility is correctly set for this Tauri application.
Based on verification:
Tauri's default template uses ['chrome115', 'safari13'] as a good baseline that produces code compatible with all Windows versions, most Linux, and practically all macOS versions, which is exactly what your project implements. Vite ignores the target value in tsconfig.json, following the same behavior as esbuild, so the transform is performed with esbuild and the build.target value controls browser compatibility.
Your configuration is correct: Vite's esbuild will convert newer syntax to older syntax as appropriate based on the target option. The ES2022 in tsconfig.json is intentional—it provides type information without affecting runtime output.
Verification complete—no action required. The build pipeline properly transpiles ES2022 code down to Safari 13 compatibility. Monitor only if using untranspilable features like top-level await (which would cause build failures, not silent incompatibility).
[resolve_review_comment]
🏁 Script executed:
#!/bin/bash
# Search for ES2022-specific features that might not transpile well to Safari 13
# Look for: top-level await, private fields, static fields, Array.at(), Object.hasOwn()
rg -n "top.?level.?await|^[[:space:]]*#[a-zA-Z]|static\s+#|\.at\(|Object\.hasOwn" src/ --type=ts --type=tsx 2>/dev/null | head -20Length of output: 0
Let me verify the search worked correctly:
🏁 Script executed:
#!/bin/bash
# Check if src/ directory exists and has TypeScript files
echo "=== Checking src/ directory ==="
fd -t f "\.(ts|tsx)$" src/ --max-results 5
echo "=== Verifying absence of ES2022 risk features with simpler pattern ==="
# Try simpler patterns
rg -n "\.at\(" src/ 2>/dev/null || echo "No Array.at() found"
rg -n "Object\.hasOwn" src/ 2>/dev/null || echo "No Object.hasOwn found"
rg -n "await\s" src/ --type=ts --type=tsx 2>/dev/null | head -5 || echo "No await patterns found"Length of output: 292
Object.hasOwn() usage is incompatible with Safari 13 target.
The verification found one ES2022-specific feature that Safari 13 doesn't support:
src/components/ui/UserSettings.tsx:44 uses Object.hasOwn(), which was added in ES2022 but is only available in Safari 15.4+. This will cause a runtime error on macOS/Linux Tauri builds targeting Safari 13.
Replace with the polyfill-compatible approach:
Object.prototype.hasOwnProperty.call(value, key)The configuration itself is correct—your build pipeline follows Tauri v1 best practices. However, the original review comment's caution was justified: ensure all code is compatible with the declared target.
🤖 Prompt for AI Agents
In tsconfig.json around lines 3 to 5 and in src/components/ui/UserSettings.tsx
at the reported usage (line ~44), replace the ES2022-only call to
Object.hasOwn(...) with the polyfill-compatible pattern using
Object.prototype.hasOwnProperty.call(target, key); update that specific
expression to call Object.prototype.hasOwnProperty.call(value, key) (or the
appropriate variable names in that file) so the code remains compatible with the
Safari 13 runtime target without changing tsconfig.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/lib/modal/SearchableModal.tsx (1)
194-201: Selected item scrolling uses the wrong DOM element.This issue was previously reported and remains unresolved. When categories are enabled,
resultsRef.current?.children[selectedIndex]targets category wrapper<div>elements (lines 294-312) rather than the actual result items. SinceselectedIndexis an index intofilteredItems, the scrolled element won't correspond to the selected item.The recommended fix is to use a dedicated ref attached to the selected item:
+ const selectedItemRef = useRef<HTMLDivElement | null>(null); ... useEffect(() => { - const selectedElement = resultsRef.current?.children[ - selectedIndex - ] as HTMLElement; - if (selectedElement) { - selectedElement.scrollIntoView({ block: "nearest", behavior: "smooth" }); + if (selectedItemRef.current) { + selectedItemRef.current.scrollIntoView({ + block: "nearest", + behavior: "smooth", + }); } }, [selectedIndex]);Then in the render (line 305), attach the ref to the selected item:
return ( - <div key={item.id}> + <div + key={item.id} + ref={isSelected ? selectedItemRef : undefined} + >src/components/ui/QuickActions.tsx (1)
61-61: Remove unused variable declaration.The variable
isCurrentServeron line 61 is declared but never used. Line 137 declaresisCurrentlySelectedServerwith identical logic, which is actually used in the code.Apply this diff:
- const isCurrentServer = server.id === currentServerId; - // Add channels with unread mentions or messages (excluding currently selected)src/lib/settings/registry.ts (1)
348-395:requiredvalidation still treats whitespace and empty arrays as “present”You’ve fixed the false/0 case, but required now only rejects
null | undefined | "". Whitespace-only strings (" ") and empty arrays ([]) will pass a required check, which is usually not what you want for user-entered text or list-style settings.If you want stricter semantics, consider something like:
- if ( - validation.required && - (value === null || value === undefined || value === "") - ) { + const isEmpty = + value === null || + value === undefined || + (typeof value === "string" && value.trim().length === 0) || + (Array.isArray(value) && value.length === 0); + + if (validation.required && isEmpty) { return { valid: false, error: "This field is required" }; }This keeps
falseand0valid while treating truly empty inputs as missing.
🧹 Nitpick comments (9)
src/lib/modal/SearchableModal.tsx (3)
68-83: Remove redundant|| falseexpression.Line 79 ends with
|| false, but this is redundant since the||chain already evaluates tofalseif all conditions are falsy.Apply this diff:
item.keywords?.some((keyword) => keyword.toLowerCase().includes(lowerQuery), - ) || - false + ) );
204-205: Move pure helper function outside component.The
escapeRegExpfunction is recreated on every render. Since it's a pure utility function with no dependencies on props or state, it should be defined outside the component for better performance and code organization.Apply this diff:
+// Helper to escape regex metacharacters +const escapeRegExp = (value: string) => + value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + export const SearchableModal: React.FC<SearchableModalProps> = (props) => { ... - // Helper to escape regex metacharacters - const escapeRegExp = (value: string) => - value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); - // Highlight search query in text const highlightText = (text: string, searchQuery: string) => {
300-302: Consider optimizing global index lookup.The code calls
filteredItems.indexOf(item)for each item inside a nested loop, resulting in O(n²) complexity. While this is acceptable for the defaultmaxResultsof 50, it could be optimized by precomputing the indices.Consider computing a Map before the render loop:
// Before the return statement, around line 240 const itemIndexMap = useMemo( () => new Map(filteredItems.map((item, idx) => [item.id, idx])), [filteredItems] );Then replace line 301:
{categoryItems.map((item, index) => { - const globalIndex = filteredItems.indexOf(item); + const globalIndex = itemIndexMap.get(item.id) ?? -1; const isSelected = globalIndex === selectedIndex;src/components/ui/QuickActions.tsx (3)
42-42: Remove unusedinputRef.The
inputRefis created and attached to the input element but never accessed in the code. SinceautoFocushandles the initial focus (line 377), this ref serves no purpose.Apply this diff to remove the unused ref:
- const inputRef = useRef<HTMLInputElement>(null);- ref={inputRef} type="text"Also applies to: 371-371
299-301: Remove redundant initialization effect.The
selectedIndexstate is already initialized to0in theuseStatecall on line 41. ThisuseEffectthat sets it to0on mount serves no purpose.Apply this diff:
- useEffect(() => { - setSelectedIndex(0); - }, []); - const searchResultsRef = useRef<QuickActionResult[]>([]);
137-137: Consider hoisting repeated computation.Both lines 137 and 176 compute
isCurrentlySelectedServerwith identical logic inside theservers.forEachloop. While the performance impact is negligible, hoisting this computation once per server iteration would reduce duplication.Consider computing
isCurrentlySelectedServeronce at the start of the server loop:servers.forEach((server) => { + const isCurrentlySelectedServer = server.id === currentServerId; + const isCurrentServer = server.id === currentServerId;Then remove the duplicate declarations on lines 137 and 176.
Also applies to: 176-176
src/components/ui/settings/SettingRenderer.tsx (1)
20-47:isHighlightedprop onSettingRendereris currently unused
SettingRendereracceptsisHighlightedbut never uses it, and the custom component branch doesn’t forward it either.SettingFieldalready handles highlight styling at the wrapper level.Either drop
isHighlightedfromSettingRendererPropsand the call site, or (if you plan to style internals/custom components based on it) wire it through toCustomComponent.src/lib/settings/registry.ts (2)
185-196:searchIndexis populated but never used insearch()
updateSearchIndexbuildssearchableTextand stores it inthis.searchIndex, butsearch()recomputes matches directly fromsettingfields and never readssearchableText(the localsearchableTextvariable is unused).Either:
- Use
searchIndexas the primary match source (e.g., iterate and score over the precomputed strings), or- Drop
searchIndexandupdateSearchIndexto simplify the registry until you need that optimization.
216-225: Comment says “hidden or disabled” but onlyhiddenis filteredIn
search(), the comment mentions skipping “hidden or disabled settings”, but the code only checkssetting.hidden. If disabled settings should also be excluded from search, add the extra guard; otherwise update the comment to avoid confusion.- // Skip hidden or disabled settings in search - if (setting.hidden) { + // Skip hidden settings in search + if (setting.hidden) { continue; }(or extend the condition to include
setting.disabledif that was the intent).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/ui/QuickActions.tsx(1 hunks)src/components/ui/settings/SettingRenderer.tsx(1 hunks)src/lib/modal/SearchableModal.tsx(1 hunks)src/lib/settings/registry.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/components/ui/settings/SettingRenderer.tsx (1)
src/lib/settings/types.ts (2)
SettingDefinition(56-110)SettingValue(6-11)
src/lib/settings/registry.ts (1)
src/lib/settings/types.ts (7)
SettingDefinition(56-110)SettingCategory(126-138)CategoryMetadata(143-150)SettingQuickAction(236-246)SettingSearchResult(176-185)SettingGroup(225-231)SettingValue(6-11)
src/lib/modal/SearchableModal.tsx (1)
src/lib/modal/BaseModal.tsx (2)
BaseModalProps(6-19)BaseModal(21-139)
src/components/ui/QuickActions.tsx (4)
src/lib/settings/types.ts (1)
SettingSearchResult(176-185)src/types/index.ts (3)
Channel(68-88)PrivateChat(90-106)Server(25-48)src/lib/settings/registry.ts (1)
settingsRegistry(439-439)src/lib/fuzzySearch.ts (1)
fuzzyMatch(6-53)
🔇 Additional comments (10)
src/lib/modal/SearchableModal.tsx (2)
12-65: LGTM! Well-structured interfaces and component setup.The
SearchableItemandSearchableModalPropsinterfaces are well-designed with appropriate optional fields. The component setup with state management and refs is clean and follows React best practices.
110-182: LGTM! Well-implemented event handlers.The debounced search, selection handler, and keyboard navigation are all correctly implemented with appropriate dependency arrays and edge case handling.
src/components/ui/QuickActions.tsx (4)
313-357: LGTM! Keyboard handling implementation is solid.The keyboard event listener uses the refs pattern correctly to avoid recreating the listener on every render while still having access to current values. The logic properly handles Escape, arrow keys, Tab, and Enter with appropriate
preventDefaultcalls and boundary clamping.
252-297: LGTM! Result selection logic is well-structured.The
handleSelectcallback correctly handles each result type with appropriate navigation actions. The type assertions are safe becauseresult.typediscriminates the union, and the server ID extraction (line 283) is safe given the consistent ID format used when creating results.
50-250: LGTM! Search logic is comprehensive and well-designed.The
searchResultsmemo implements a sophisticated two-mode search strategy:
- Empty query intelligently surfaces unread mentions and messages with appropriate priority scoring
- Non-empty query combines settings search with fuzzy matching across all entities
The score adjustments based on current selection context (deprioritizing current items, boosting same-server items) enhance UX by surfacing the most relevant results.
359-492: LGTM! Rendering implementation is clean and functional.The modal rendering properly implements the standard backdrop pattern with
stopPropagationon the content to prevent accidental dismissal. The results list provides clear visual feedback with icons, type badges, and unread indicators. The conditional styling for the selected item enhances keyboard navigation UX.src/components/ui/settings/SettingRenderer.tsx (2)
50-201: Overall SettingRenderer control mappings look solidThe per-type branches (toggle/text/number/textarea/select/radio/color/range) correctly wire
value/onChangeand respectdisabled/placeholder/range props. The radio handling now preserves option value types viahandleChange(option.value), which avoids the earlier string-casting issue.
240-299: SettingField wrapper nicely encapsulates labeling and highlight behaviorThe wrapper cleanly handles label/description/tooltip, highlight styling, and error display while delegating the actual control to
SettingRenderer. This separation should make it easy to evolve the renderer without touching layout.src/lib/settings/registry.ts (2)
14-181: Registry structure and category wiring look goodThe registry cleanly centralizes settings, categories, grouping, search, defaults, and quick actions. Category initialization covers all
SettingCategoryvariants andgetGrouped/getDefaultValuesare straightforward and easy to reason about.
398-435: Quick actions API is simple and fits the command-palette use case
registerQuickAction,getQuickActions, andsearchQuickActionsprovide a minimal, usable surface for the new quick actions menu. The case-insensitive substring matching over title/description/keywords/category should work well with a command palette.
| selectServer, | ||
| } = useStore(); | ||
| const [searchQuery, setSearchQuery] = useState(""); | ||
| const [selectedIndex, setSelectedIndex] = useState(0); |
There was a problem hiding this comment.
Clamp selectedIndex when search results change.
When the user navigates to a result (e.g., index 8) and then modifies the search query to return fewer results (e.g., only 2 results), selectedIndex remains at 8. This causes no item to be highlighted and the Enter key to do nothing until the user presses arrow keys.
Add an effect to clamp selectedIndex when searchResults.length changes:
useEffect(() => {
if (selectedIndex >= searchResults.length && searchResults.length > 0) {
setSelectedIndex(searchResults.length - 1);
} else if (searchResults.length === 0) {
setSelectedIndex(0);
}
}, [searchResults.length, selectedIndex]);Also applies to: 250-250
🤖 Prompt for AI Agents
In src/components/ui/QuickActions.tsx around lines 41 and 250, selectedIndex is
not clamped when the number of searchResults changes causing stale out-of-range
selection; add a useEffect that watches searchResults.length (and selectedIndex)
and sets selectedIndex to searchResults.length - 1 when selectedIndex >=
searchResults.length and searchResults.length > 0, and resets selectedIndex to 0
when searchResults.length === 0 so the highlighted item and Enter behavior
remain valid.
| case "file": | ||
| return ( | ||
| <input | ||
| type="file" | ||
| onChange={(e) => { | ||
| const file = e.target.files?.[0]; | ||
| if (file) { | ||
| const reader = new FileReader(); | ||
| reader.onload = () => handleChange(reader.result as string); | ||
| reader.readAsDataURL(file); | ||
| } | ||
| }} | ||
| accept={setting.accept} | ||
| multiple={setting.multiple} | ||
| disabled={disabled} | ||
| className="w-full text-discord-text-normal file:mr-4 file:py-2 file:px-4 file:rounded file:border-0 file:bg-discord-primary file:text-white hover:file:bg-discord-primary-hover disabled:opacity-50" |
There was a problem hiding this comment.
multiple file inputs silently ignore all but the first file
The "file" case always reads only files[0] but still exposes multiple={setting.multiple} on the input. If any setting enables multiple, this will drop all additional selections with no feedback.
Consider either:
- Implementing multi-file handling (e.g., map all selected files to an array of data URLs or a more structured value), or
- Removing/ignoring
setting.multiplehere until there’s a clear multi-file value contract.
🤖 Prompt for AI Agents
In src/components/ui/settings/SettingRenderer.tsx around lines 202 to 217, the
input exposes multiple={setting.multiple} but the onChange handler only reads
files[0], silently dropping extra files; either implement full multi-file
handling or stop exposing multiple. To implement multi-file support: when
e.target.files exists, convert FileList to an array, use Promise.all to read
each file as a data URL (or whatever structured value the setting expects) and
then call handleChange with the array of results (keep existing single-file
string behavior when setting.multiple is false). Alternatively, if multi-file is
not supported yet, remove/ignore the multiple prop (set multiple={false} or omit
it) so the UI does not allow selecting multiple files.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/ui/UserSettings.tsx (1)
289-291: Custom notification sound setting remains non-functional (duplicate concern).As flagged in the previous review,
notificationSoundFilestate is declared butsetNotificationSoundFileis never called anywhere in this component. TheplayNotificationSoundfunction is also never invoked. The logic inhandleSave(lines 616-624) to persist the file exists, but no File object ever reaches this state.The recommended solutions from the previous review still apply:
- Move file reading into
SettingRendererto handle the "file" type properly- Wire
notificationSoundFileexplicitly with a custom component for the notification sound settingAlso applies to: 566-587, 616-624
🧹 Nitpick comments (2)
src/components/ui/UserSettings.tsx (2)
30-76: Consider using an established library for deep operations.The custom
deepCloneanddeepEqualimplementations handle common cases but may miss edge cases like circular references,Map,Set, or other complex object types. Libraries like lodash (_.cloneDeep,_.isEqual) orfast-deep-equalprovide well-tested, optimized implementations that handle these scenarios.For settings values, the current implementation may be sufficient, but using a proven library reduces maintenance burden and potential bugs.
688-906: Consider extracting profile fields into a reusable component.The profile field rendering contains significant duplication—each field (displayName, avatar, realname, homepage, status, color, bot) follows the same structure with:
- Conditional ID for highlighting (
setting-profile.{fieldName})- Conditional styling for the highlight animation
- Label, description, and input with similar styling
Extracting this into a reusable
ProfileFieldcomponent would reduce duplication and improve maintainability. However, given the current focus of this PR on adding the quick actions menu, this refactor can be deferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ui/UserSettings.tsx(12 hunks)src/store/index.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/store/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ui/UserSettings.tsx (7)
src/lib/settings/index.ts (2)
SettingValue(23-23)settingsRegistry(6-6)src/lib/settings/types.ts (1)
SettingValue(6-11)src/lib/ircClient.ts (2)
sendRaw(767-780)changeNick(1103-1105)src/store/index.ts (1)
GlobalSettings(424-454)src/hooks/useModalBehavior.ts (1)
useModalBehavior(10-45)src/lib/settings/registry.ts (1)
settingsRegistry(439-439)src/components/ui/settings/SettingRenderer.tsx (1)
SettingField(240-299)
| // Handle save | ||
| const handleSave = useCallback(async () => { | ||
| if (!currentServer) return; | ||
|
|
||
| // Save profile metadata | ||
| if (supportsMetadata) { | ||
| const metadata: Record<string, string> = {}; | ||
| if (avatar) metadata.avatar = avatar; | ||
| if (displayName) metadata["display-name"] = displayName; | ||
| if (realname) metadata.realname = realname; | ||
| if (homepage) metadata.homepage = homepage; | ||
| if (status) metadata.status = status; | ||
| if (color) metadata.color = color; | ||
| if (bot) metadata.bot = bot; | ||
|
|
||
| for (const [key, value] of Object.entries(metadata)) { | ||
| sendRaw(currentServer.id, `METADATA * SET ${key} :${value}`); | ||
| } | ||
| } | ||
|
|
||
| // Handle nickname change | ||
| if (newNickname && newNickname !== currentUser?.username) { | ||
| changeNick(currentServer.id, newNickname); | ||
| } | ||
|
|
||
| updateGlobalSettings(settings as Partial<GlobalSettings>); | ||
|
|
||
| setEnableHighlights(globalEnableHighlights); | ||
| setSendTypingNotifications(globalSendTypingNotifications); | ||
| setNickname(globalNickname || currentUser?.username || ""); | ||
| setAccountName(globalAccountName); | ||
| setAccountPassword(globalAccountPassword); | ||
|
|
||
| // Set original values for change tracking - only once | ||
| setOriginalValues({ | ||
| avatar: avatarValue, | ||
| displayName: displayNameValue, | ||
| realname: realnameValue, | ||
| homepage: homepageValue, | ||
| status: statusValue, | ||
| color: colorValue, | ||
| bot: botValue, | ||
| newNickname: nicknameValue, | ||
| enableNotificationSounds: globalEnableNotificationSounds, | ||
| notificationSound: migratedNotificationSound, | ||
| enableHighlights: globalEnableHighlights, | ||
| sendTypingNotifications: globalSendTypingNotifications, | ||
| nickname: globalNickname || currentUser?.username || "", | ||
| accountName: globalAccountName, | ||
| accountPassword: globalAccountPassword, | ||
| showSafeMedia: globalShowSafeMedia, | ||
| showExternalContent: globalShowExternalContent, | ||
| enableMarkdownRendering: globalEnableMarkdownRendering, | ||
| showEvents: globalShowEvents, | ||
| showNickChanges: globalShowNickChanges, | ||
| showJoinsParts: globalShowJoinsParts, | ||
| showQuits: globalShowQuits, | ||
| showKicks: globalShowKicks, | ||
| enableMultilineInput: globalEnableMultilineInput, | ||
| multilineOnShiftEnter: globalMultilineOnShiftEnter, | ||
| autoFallbackToSingleLine: globalAutoFallbackToSingleLine, | ||
| operName: operName, | ||
| operPassword: operPassword, | ||
| operOnConnect: operOnConnect, | ||
| // Save notification sound file | ||
| if (notificationSoundFile) { | ||
| const reader = new FileReader(); | ||
| reader.onload = (e) => { | ||
| const dataUrl = e.target?.result as string; | ||
| updateGlobalSettings({ notificationSound: dataUrl }); | ||
| }; | ||
| reader.readAsDataURL(notificationSoundFile); | ||
| } | ||
|
|
||
| // Save oper settings if in hosted chat mode | ||
| if (isHostedChatMode && serverConfig) { | ||
| updateServer(serverConfig.id, { | ||
| ...serverConfig, | ||
| operUsername: operName, | ||
| operOnConnect, | ||
| }); | ||
| } | ||
|
|
||
| // Reset original values | ||
| setOriginalValues(null); | ||
| toggleSettingsModal(false); | ||
| }, [ | ||
| currentUser?.id, | ||
| currentUser?.displayName, | ||
| currentUser?.metadata?.["display-name"]?.value, | ||
| currentUser?.metadata?.avatar?.value, | ||
| currentUser?.metadata?.bot?.value, | ||
| currentUser?.metadata?.color?.value, | ||
| currentUser?.metadata?.homepage?.value, | ||
| currentUser?.metadata?.status?.value, | ||
| currentUser?.username, | ||
| globalAccountName, | ||
| globalAccountPassword, | ||
| globalEnableHighlights, | ||
| globalEnableNotificationSounds, | ||
| globalNickname, | ||
| globalNotificationSound, | ||
| globalSendTypingNotifications, | ||
| globalShowSafeMedia, | ||
| globalShowExternalContent, | ||
| currentServer, | ||
| supportsMetadata, | ||
| avatar, | ||
| displayName, | ||
| realname, | ||
| homepage, | ||
| status, | ||
| color, | ||
| bot, | ||
| newNickname, | ||
| currentUser, | ||
| originalValues, | ||
| updateGlobalSettings, | ||
| globalAutoFallbackToSingleLine, | ||
| globalEnableMarkdownRendering, | ||
| globalEnableMultilineInput, | ||
| globalMultilineOnShiftEnter, | ||
| globalShowEvents, | ||
| globalShowJoinsParts, | ||
| globalShowKicks, | ||
| globalShowNickChanges, | ||
| globalShowQuits, | ||
| settings, | ||
| notificationSoundFile, | ||
| isHostedChatMode, | ||
| serverConfig, | ||
| operName, | ||
| operPassword, | ||
| operOnConnect, | ||
| ]); // Only depend on user ID - removed all other dependencies | ||
| sendRaw, | ||
| changeNick, | ||
| updateGlobalSettings, | ||
| updateServer, | ||
| toggleSettingsModal, | ||
| ]); |
There was a problem hiding this comment.
Add error handling and await async operations in save handler.
The save handler performs multiple operations sequentially without error handling or proper async handling:
-
Race condition with FileReader (lines 616-624): The FileReader operation is asynchronous but not awaited. The modal closes immediately (line 637), potentially before the file is fully read and saved.
-
No error handling: If any operation fails (e.g.,
sendRawfor metadata,changeNick,updateGlobalSettings), there's no feedback to the user and subsequent operations still execute, potentially leaving the state inconsistent. -
Inconsistent async/await: The function is marked
asyncbut doesn'tawaitany operations.
Consider wrapping the save operations in try-catch blocks and awaiting the FileReader operation before closing the modal.
Example for the FileReader issue:
- // Save notification sound file
- if (notificationSoundFile) {
- const reader = new FileReader();
- reader.onload = (e) => {
- const dataUrl = e.target?.result as string;
- updateGlobalSettings({ notificationSound: dataUrl });
- };
- reader.readAsDataURL(notificationSoundFile);
- }
+ // Save notification sound file
+ if (notificationSoundFile) {
+ await new Promise<void>((resolve, reject) => {
+ const reader = new FileReader();
+ reader.onload = (e) => {
+ try {
+ const dataUrl = e.target?.result as string;
+ updateGlobalSettings({ notificationSound: dataUrl });
+ resolve();
+ } catch (error) {
+ reject(error);
+ }
+ };
+ reader.onerror = () => reject(reader.error);
+ reader.readAsDataURL(notificationSoundFile);
+ });
+ }🤖 Prompt for AI Agents
In src/components/ui/UserSettings.tsx around lines 589 to 661, the save handler
fires multiple async operations without awaiting them and lacks error handling:
wrap the sequence in a try-catch, await each async operation (sendRaw if it
returns a promise, changeNick, updateGlobalSettings, updateServer) and only
reset originalValues and close the modal after all awaited operations succeed;
convert the FileReader usage into a Promise (await readAsDataURL) so the
notificationSound is saved before closing, and on any caught error show
user-visible feedback (toast/modal) and avoid leaving state half-applied.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/components/ui/QuickActions.tsx (3)
56-56: Remove unusedinputRef.The
inputRefis declared and assigned to the input element (line 509) but is never used for any imperative operations. The input already hasautoFocus(line 515), making the ref unnecessary.Apply this diff:
- const inputRef = useRef<HTMLInputElement>(null);And on line 509:
<input - ref={inputRef} type="text"
437-439: Remove redundantuseEffect.This effect sets
selectedIndexto 0 on mount, butselectedIndexis already initialized to 0 on line 55. The effect is redundant.Apply this diff:
- useEffect(() => { - setSelectedIndex(0); - }, []); -
311-349: Consider optimizing user collection for large servers.The code collects all users from all channels on the current server into a Map (lines 312-323) for each search query change. On servers with many channels and users, this could impact search responsiveness. Consider memoizing the user collection separately or using an index.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ui/QuickActions.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ui/QuickActions.tsx (4)
src/lib/settings/types.ts (1)
SettingSearchResult(176-185)src/types/index.ts (3)
Channel(68-88)PrivateChat(90-106)Server(25-48)src/lib/fuzzySearch.ts (1)
fuzzyMatch(6-53)src/lib/ircClient.ts (2)
ircClient(2728-2728)joinChannel(829-871)
| const searchResults: QuickActionResult[] = useMemo(() => { | ||
| const query = searchQuery.trim(); | ||
| const results: QuickActionResult[] = []; | ||
| const currentServerId = ui.selectedServerId; | ||
| const currentSelection = currentServerId | ||
| ? ui.perServerSelections[currentServerId] | ||
| : null; | ||
|
|
||
| // If no search query, show unread mentions and messages | ||
| if (query.length === 0) { | ||
| servers.forEach((server) => { | ||
| const isCurrentServer = server.id === currentServerId; | ||
|
|
||
| // Add channels with unread mentions or messages (excluding currently selected) | ||
| server.channels.forEach((channel) => { | ||
| const isCurrentlySelected = | ||
| isCurrentServer && | ||
| currentSelection?.selectedChannelId === channel.id; | ||
|
|
||
| // Skip currently selected channel | ||
| if (isCurrentlySelected) return; | ||
|
|
||
| // Only show channels with unread mentions or messages | ||
| if (channel.isMentioned && channel.unreadCount > 0) { | ||
| // High priority for mentions (score: 1000 + unreadCount) | ||
| results.push({ | ||
| type: "channel", | ||
| id: `channel-${server.id}-${channel.id}`, | ||
| title: channel.name, | ||
| description: `${server.name}${channel.topic ? ` - ${channel.topic}` : ""}`, | ||
| serverId: server.id, | ||
| score: 1000 + channel.unreadCount, | ||
| data: channel, | ||
| }); | ||
| } else if (channel.unreadCount > 0) { | ||
| // Lower priority for unread messages (score: 500 + unreadCount) | ||
| results.push({ | ||
| type: "channel", | ||
| id: `channel-${server.id}-${channel.id}`, | ||
| title: channel.name, | ||
| description: `${server.name}${channel.topic ? ` - ${channel.topic}` : ""}`, | ||
| serverId: server.id, | ||
| score: 500 + channel.unreadCount, | ||
| data: channel, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| // Add private chats with unread mentions or messages (excluding currently selected) | ||
| server.privateChats.forEach((pm) => { | ||
| const isCurrentlySelected = | ||
| isCurrentServer && | ||
| currentSelection?.selectedPrivateChatId === pm.id; | ||
|
|
||
| // Skip currently selected PM | ||
| if (isCurrentlySelected) return; | ||
|
|
||
| // Only show PMs with unread mentions or messages | ||
| if (pm.isMentioned && pm.unreadCount > 0) { | ||
| // High priority for mentions (score: 1000 + unreadCount) | ||
| results.push({ | ||
| type: "dm", | ||
| id: `dm-${server.id}-${pm.id}`, | ||
| title: pm.username, | ||
| description: server.name, | ||
| serverId: server.id, | ||
| score: 1000 + pm.unreadCount, | ||
| data: pm, | ||
| }); | ||
| } else if (pm.unreadCount > 0) { | ||
| // Lower priority for unread messages (score: 500 + unreadCount) | ||
| results.push({ | ||
| type: "dm", | ||
| id: `dm-${server.id}-${pm.id}`, | ||
| title: pm.username, | ||
| description: server.name, | ||
| serverId: server.id, | ||
| score: 500 + pm.unreadCount, | ||
| data: pm, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| // Add servers with mentions (excluding currently selected) | ||
| const hasMentions = | ||
| server.channels.some((ch) => ch.isMentioned) || | ||
| server.privateChats?.some((pc) => pc.isMentioned); | ||
| const isCurrentlySelectedServer = server.id === currentServerId; | ||
|
|
||
| if (hasMentions && !isCurrentlySelectedServer) { | ||
| // Count total mentions in server | ||
| const totalMentions = | ||
| server.channels | ||
| .filter((ch) => ch.isMentioned) | ||
| .reduce((sum, ch) => sum + ch.unreadCount, 0) + | ||
| server.privateChats | ||
| .filter((pc) => pc.isMentioned) | ||
| .reduce((sum, pc) => sum + pc.unreadCount, 0); | ||
|
|
||
| results.push({ | ||
| type: "server", | ||
| id: `server-${server.id}`, | ||
| title: server.name, | ||
| description: server.host, | ||
| serverId: server.id, | ||
| score: 800 + totalMentions, // Between unread messages and mentions | ||
| data: server, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| return results.sort((a, b) => b.score - a.score).slice(0, 15); | ||
| } | ||
|
|
||
| settingsRegistry.search(query, { limit: 10 }).forEach((settingResult) => { | ||
| results.push({ | ||
| type: "setting", | ||
| id: `setting-${settingResult.setting.id}`, | ||
| title: settingResult.setting.title, | ||
| description: settingResult.setting.description, | ||
| score: settingResult.score, | ||
| data: settingResult, | ||
| }); | ||
| }); | ||
|
|
||
| servers.forEach((server) => { | ||
| const isCurrentlySelectedServer = server.id === currentServerId; | ||
|
|
||
| const serverMatch = fuzzyMatch(query, server.name); | ||
| if (serverMatch.matches) { | ||
| let scoreAdjustment = 0; | ||
| if (isCurrentlySelectedServer) { | ||
| scoreAdjustment = -30; | ||
| } | ||
|
|
||
| results.push({ | ||
| type: "server", | ||
| id: `server-${server.id}`, | ||
| title: server.name, | ||
| description: server.host, | ||
| serverId: server.id, | ||
| score: serverMatch.score + scoreAdjustment, | ||
| data: server, | ||
| }); | ||
| } | ||
|
|
||
| server.channels.forEach((channel) => { | ||
| const channelMatch = fuzzyMatch(query, channel.name); | ||
| if (channelMatch.matches) { | ||
| const isCurrentlySelected = | ||
| isCurrentlySelectedServer && | ||
| currentSelection?.selectedChannelId === channel.id; | ||
|
|
||
| let scoreAdjustment = 0; | ||
| if (isCurrentlySelected) { | ||
| scoreAdjustment = -30; | ||
| } else if (isCurrentlySelectedServer) { | ||
| scoreAdjustment = 20; | ||
| } | ||
|
|
||
| results.push({ | ||
| type: "channel", | ||
| id: `channel-${server.id}-${channel.id}`, | ||
| title: channel.name, | ||
| description: `${server.name}${channel.topic ? ` - ${channel.topic}` : ""}`, | ||
| serverId: server.id, | ||
| score: channelMatch.score + scoreAdjustment, | ||
| data: channel, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| server.privateChats.forEach((pm) => { | ||
| const pmMatch = fuzzyMatch(query, pm.username); | ||
| if (pmMatch.matches) { | ||
| const isCurrentlySelected = | ||
| isCurrentlySelectedServer && | ||
| currentSelection?.selectedPrivateChatId === pm.id; | ||
|
|
||
| let scoreAdjustment = 0; | ||
| if (isCurrentlySelected) { | ||
| scoreAdjustment = -30; | ||
| } else if (isCurrentlySelectedServer) { | ||
| scoreAdjustment = 20; | ||
| } | ||
|
|
||
| results.push({ | ||
| type: "dm", | ||
| id: `dm-${server.id}-${pm.id}`, | ||
| title: pm.username, | ||
| description: server.name, | ||
| serverId: server.id, | ||
| score: pmMatch.score + scoreAdjustment, | ||
| data: pm, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| if ( | ||
| currentServerId && | ||
| server.id === currentServerId && | ||
| query.startsWith("#") | ||
| ) { | ||
| const channelName = query.trim(); | ||
| const availableChannels = channelList[server.id] || []; | ||
| const alreadyShown = new Set<string>(); | ||
|
|
||
| availableChannels.forEach((availChannel) => { | ||
| const channelMatch = fuzzyMatch( | ||
| query.slice(1), | ||
| availChannel.channel.slice(1), | ||
| ); | ||
| if (channelMatch.matches) { | ||
| const alreadyJoined = server.channels.find( | ||
| (ch) => ch.name === availChannel.channel, | ||
| ); | ||
| if (!alreadyJoined) { | ||
| results.push({ | ||
| type: "join-channel", | ||
| id: `join-channel-${server.id}-${availChannel.channel}`, | ||
| title: `Join ${availChannel.channel}`, | ||
| description: `${availChannel.userCount} users - ${server.name}`, | ||
| serverId: server.id, | ||
| score: channelMatch.score + 50, | ||
| data: { channelName: availChannel.channel }, | ||
| }); | ||
| alreadyShown.add(availChannel.channel.toLowerCase()); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| if ( | ||
| channelName.length > 1 && | ||
| !alreadyShown.has(channelName.toLowerCase()) | ||
| ) { | ||
| results.push({ | ||
| type: "join-channel", | ||
| id: `join-channel-${server.id}-${channelName}`, | ||
| title: `Join ${channelName}`, | ||
| description: `Join channel on ${server.name}`, | ||
| serverId: server.id, | ||
| score: 100, | ||
| data: { channelName }, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| if (currentServerId && server.id === currentServerId) { | ||
| const allUsers = new Map< | ||
| string, | ||
| { username: string; isOnline: boolean } | ||
| >(); | ||
| for (const channel of server.channels) { | ||
| for (const user of channel.users) { | ||
| allUsers.set(user.username, { | ||
| username: user.username, | ||
| isOnline: user.isOnline, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| const currentUser = ircClient.getCurrentUser(server.id); | ||
| const availableUsers = Array.from(allUsers.values()).filter( | ||
| (user) => user.username !== currentUser?.username, | ||
| ); | ||
|
|
||
| availableUsers.forEach((user) => { | ||
| const existingPM = server.privateChats.find( | ||
| (pm) => pm.username === user.username, | ||
| ); | ||
| if (!existingPM) { | ||
| const userMatch = fuzzyMatch(query, user.username); | ||
| if (userMatch.matches) { | ||
| results.push({ | ||
| type: "start-dm", | ||
| id: `start-dm-${server.id}-${user.username}`, | ||
| title: `Message ${user.username}`, | ||
| description: `Start private message on ${server.name}`, | ||
| serverId: server.id, | ||
| score: userMatch.score + (user.isOnline ? 10 : 0), | ||
| data: undefined, | ||
| }); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| return results.sort((a, b) => b.score - a.score).slice(0, 15); | ||
| }, [ | ||
| searchQuery, | ||
| servers, | ||
| channelList, | ||
| ui.selectedServerId, | ||
| ui.perServerSelections, | ||
| ]); |
There was a problem hiding this comment.
Clamp selectedIndex when search results change.
The past review comment about clamping selectedIndex when searchResults.length changes has not been addressed. When the user navigates to a result (e.g., index 8) and then modifies the search query to return fewer results (e.g., only 2 results), selectedIndex remains at 8. This causes no item to be highlighted and the Enter key to do nothing until the user presses arrow keys.
Add an effect to clamp selectedIndex:
useEffect(() => {
if (selectedIndex >= searchResults.length && searchResults.length > 0) {
setSelectedIndex(searchResults.length - 1);
} else if (searchResults.length === 0) {
setSelectedIndex(0);
}
}, [searchResults.length, selectedIndex]);🤖 Prompt for AI Agents
In src/components/ui/QuickActions.tsx around lines 64-359, selectedIndex is not
clamped when searchResults length changes which can leave it out of range after
a query change; add a useEffect that watches searchResults.length and adjusts
setSelectedIndex to Math.min(selectedIndex, searchResults.length - 1) when
results exist and sets it to 0 when results are empty (ensure the effect depends
on searchResults.length and selectedIndex only as needed to avoid update loops).
| results.push({ | ||
| type: "start-dm", | ||
| id: `start-dm-${server.id}-${user.username}`, | ||
| title: `Message ${user.username}`, | ||
| description: `Start private message on ${server.name}`, | ||
| serverId: server.id, | ||
| score: userMatch.score + (user.isOnline ? 10 : 0), | ||
| data: undefined, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Store username in data field instead of extracting from title.
The username is embedded in the title as Message ${username} and later extracted via string replacement on line 406. This approach is fragile and will break if the title format changes.
Apply this diff to store the username in the data field:
results.push({
type: "start-dm",
id: `start-dm-${server.id}-${user.username}`,
title: `Message ${user.username}`,
description: `Start private message on ${server.name}`,
serverId: server.id,
score: userMatch.score + (user.isOnline ? 10 : 0),
- data: undefined,
+ data: { username: user.username },
});Then update the interface on line 26:
interface JoinChannelData {
channelName: string;
}
+
+interface StartDmData {
+ username: string;
+}Update the union type on line 37:
- data?: SettingSearchResult | Channel | PrivateChat | Server | JoinChannelData;
+ data?: SettingSearchResult | Channel | PrivateChat | Server | JoinChannelData | StartDmData;And update line 406 in the handler:
case "start-dm": {
if (result.serverId) {
- const username = result.title.replace("Message ", "");
+ const username = (result.data as StartDmData).username;
openPrivateChat(result.serverId, username);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/ui/QuickActions.tsx around lines 337 to 346, the code pushes a
"start-dm" result that embeds the username in the title and sets data to
undefined; change the push to store the username in the data field (e.g., data:
{ username: user.username }) rather than relying on the title. Also update the
QuickAction result interface at line 26 to include the username on the start-dm
variant, update the union type declared around line 37 to reflect that
start-dm.results have data with a username string, and modify the handler at
line 406 to read the username from result.data.username instead of extracting it
from the title. Ensure types and runtime checks handle data possibly being
undefined for other action types.
* vibed * default to showing mention and activity channels * fixes * fix settings not being saved * join and DM quick actions
2 participants
Don't want to touch mouse.... A must have to me
Switch between servers channels and DMs fuzzy search
Also a complete retake of #102 Trying to do less things at once here
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.