fixes #511 - #512
Open
Somil450 wants to merge 2 commits into
Open
Conversation
👋 Thanks for opening a PR, @Somil450!Your PR has entered the 🚦 PR Review Pipeline.
What happens next
A pipeline status comment will appear below and update automatically as your PR progresses. While you wait
This comment is posted only once. |
WalkthroughThe playground adds a dedicated preferences store and dialog for editor theme and font ligatures. Header controls open the dialog through new UI state, while Monaco reads the preferences and applies them to the editor. ChangesEditor preferences
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant PlaygroundHeader
participant PlaygroundUI
participant PreferencesDialog
participant PreferencesStore
participant PlaygroundEditor
Developer->>PlaygroundHeader: Select Preferences
PlaygroundHeader->>PlaygroundUI: setShowPreferences(true)
PlaygroundUI->>PreferencesDialog: Open dialog
PreferencesDialog->>PreferencesStore: Read editorTheme and fontLigatures
PreferencesDialog->>PreferencesStore: Persist preference changes
PreferencesStore->>PlaygroundEditor: Provide updated preferences
PlaygroundEditor->>PlaygroundEditor: Apply Monaco theme and options
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
PR Summary by QodoPlayground: add Editor Preferences dialog (theme + font ligatures)
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
modules/playground/hooks/usePreferences.ts (1)
19-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the magic string to
STORAGE_KEYS.For consistency with
EDITOR_THEMEand to prevent potential typos across the codebase, consider extracting"editron:preferences:ligatures"into theSTORAGE_KEYSconfiguration constant.♻️ Proposed refactor
Update your
STORAGE_KEYSobject in@/lib/constants/config.tsfirst:export const STORAGE_KEYS = { // ... FONT_LIGATURES: "editron:preferences:ligatures", };Then apply this change:
-const getInitialLigatures = () => { - if (typeof window === "undefined") return true; - return localStorage.getItem("editron:preferences:ligatures") !== "false"; -}; +const getInitialLigatures = () => { + if (typeof window === "undefined") return true; + return localStorage.getItem(STORAGE_KEYS.FONT_LIGATURES) !== "false"; +}; export const usePreferences = create<PreferencesState>((set) => ({ editorTheme: getInitialTheme(), fontLigatures: getInitialLigatures(), setEditorTheme: (theme: string) => { try { localStorage.setItem(STORAGE_KEYS.EDITOR_THEME, theme); } catch {} set({ editorTheme: theme }); }, setFontLigatures: (enabled: boolean) => { try { - localStorage.setItem("editron:preferences:ligatures", String(enabled)); + localStorage.setItem(STORAGE_KEYS.FONT_LIGATURES, String(enabled)); } catch {} set({ fontLigatures: enabled }); } }));🤖 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 `@modules/playground/hooks/usePreferences.ts` around lines 19 - 38, Extract the "editron:preferences:ligatures" value into a FONT_LIGATURES entry in the STORAGE_KEYS constant, then update getInitialLigatures and setFontLigatures to reference STORAGE_KEYS.FONT_LIGATURES instead of the literal string. Keep the existing preference behavior unchanged.modules/playground/components/playground-editor.tsx (1)
485-495: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMemoize Monaco editor options and verify parent callbacks.
Passing a fresh object to the
optionsprop on every render can cause the Monaco wrapper to diff and update options unnecessarily. Consider memoizing this object.
Additionally, sincePlaygroundEditoris now exported withReact.memo(line 495), ensure that the parent component passes stable references (e.g., viauseCallback) for theonContentChangeandonCursorChangeprops; otherwise, thememowrapper will be ineffective.♻️ Proposed refactor for memoizing options
+ const editorOptions = useMemo(() => ({ + ...defaultEditorOptions, + fontLigatures, + inlineSuggest: { enabled: true }, + }), [fontLigatures]); return ( <div className="h-full relative"> <Editor height={"100%"} defaultValue={content} onChange={(value) => onContentChange(value || "")} onMount={handleEditorDidMount} language={ activeFile ? getEditorLanguage(activeFile.fileExtension || "") : "plaintext" } - options={{ - ...defaultEditorOptions, - fontLigatures: fontLigatures, - inlineSuggest: { enabled: true }, - }} + options={editorOptions} /> </div> );🤖 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 `@modules/playground/components/playground-editor.tsx` around lines 485 - 495, Memoize the Monaco `options` object in `PlaygroundEditor` so it is recreated only when `defaultEditorOptions` or `fontLigatures` changes. Also update the parent component’s `onContentChange` and `onCursorChange` props to use stable `useCallback` references, preserving their existing behavior so `memo(PlaygroundEditor)` can prevent unnecessary renders.
🤖 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.
Nitpick comments:
In `@modules/playground/components/playground-editor.tsx`:
- Around line 485-495: Memoize the Monaco `options` object in `PlaygroundEditor`
so it is recreated only when `defaultEditorOptions` or `fontLigatures` changes.
Also update the parent component’s `onContentChange` and `onCursorChange` props
to use stable `useCallback` references, preserving their existing behavior so
`memo(PlaygroundEditor)` can prevent unnecessary renders.
In `@modules/playground/hooks/usePreferences.ts`:
- Around line 19-38: Extract the "editron:preferences:ligatures" value into a
FONT_LIGATURES entry in the STORAGE_KEYS constant, then update
getInitialLigatures and setFontLigatures to reference
STORAGE_KEYS.FONT_LIGATURES instead of the literal string. Keep the existing
preference behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f839ae3-c6cd-4c53-ad50-32e1fd9d51ed
📒 Files selected for processing (8)
modules/playground/components/playground-editor.tsxmodules/playground/components/playground-header.tsxmodules/playground/components/playground-modals.tsxmodules/playground/components/preferences-dialog.tsxmodules/playground/hooks/useAI.test.tsmodules/playground/hooks/useAI.tsmodules/playground/hooks/usePlaygroundUI.tsmodules/playground/hooks/usePreferences.ts
💤 Files with no reviewable changes (1)
- modules/playground/hooks/useAI.test.ts
Code Review by Qodo
Context used✅ Compliance rules (platform):
22 rules 1. Ligatures preference overridden
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| </div> | ||
| ); | ||
| } No newline at end of file | ||
| export default function PreferencesDialog({ open, onOpenChange }: PreferencesDialogProps) { |
There was a problem hiding this comment.
1. preferencesdialog missing return type 📘 Rule violation ⚙ Maintainability
The newly exported PreferencesDialog component relies on an inferred return type rather than declaring an explicit return type annotation. This makes the module’s public API less explicit and can weaken type-safety guarantees over time.
Agent Prompt
## Issue description
A newly exported (public) component is missing an explicit return type annotation.
## Issue Context
Compliance requires explicit typing for public API parameters and return values; exported components count as public APIs within the module boundary.
## Fix Focus Areas
- modules/playground/components/preferences-dialog.tsx[74-75]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @@ -483,11 +484,12 @@ const PlaygroundEditor = ({ | |||
| } | |||
There was a problem hiding this comment.
3. Ligatures preference overridden 🐞 Bug ≡ Correctness
PlaygroundEditor sets fontLigatures via the Editor options prop, but handleEditorDidMount
immediately calls editor.updateOptions({ ...defaultEditorOptions, ... }), which re-applies
defaultEditorOptions.fontLigatures (true) and can override a persisted false setting on initial
load. This causes the PreferencesDialog toggle and the actual editor rendering to disagree until the
user changes the setting again.
Agent Prompt
### Issue description
`fontLigatures` is now user-configurable, but the editor mount handler re-applies `defaultEditorOptions` (which sets `fontLigatures: true`), overriding a saved `false` preference on initial mount.
### Issue Context
- The React wrapper receives `options.fontLigatures`, but `handleEditorDidMount` calls `editor.updateOptions({...defaultEditorOptions,...})` which can overwrite it.
### Fix Focus Areas
- modules/playground/components/playground-editor.tsx[54-63]
- modules/playground/components/playground-editor.tsx[484-489]
### Suggested fix
- Ensure the mount-time `editor.updateOptions(...)` does **not** force ligatures back to the default.
- Option A (simplest): remove the redundant `editor.updateOptions({ ...defaultEditorOptions, ... })` call and only set the specific mount-only options you need (e.g., `inlineSuggest`).
- Option B: include the current preference when calling `updateOptions`, e.g. `editor.updateOptions({ ...defaultEditorOptions, fontLigatures, inlineSuggest: { enabled: true } })` (requires moving `usePreferences()` above the callback so `fontLigatures` is in scope).
- Option C: add a `useEffect` that calls `editorRef.current?.updateOptions({ fontLigatures })` when `fontLigatures` changes, to guarantee the preference always wins.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| setFontLigatures: (enabled: boolean) => void; | ||
| } | ||
|
|
||
| const getInitialTheme = () => { |
There was a problem hiding this comment.
4. Unsafe localstorage reads 🐞 Bug ☼ Reliability
usePreferences initializes Zustand state by calling localStorage.getItem(...) without a try/catch, which can throw (e.g., blocked storage / restricted contexts) and prevent the preferences store from initializing. This can break rendering for any component importing the store (notably PlaygroundEditor and PreferencesDialog).
Agent Prompt
### Issue description
`getInitialTheme()` / `getInitialLigatures()` call `localStorage.getItem` without exception handling. In browsers where storage access throws, this can crash store creation.
### Issue Context
The codebase already treats localStorage as fallible in other stores (e.g., `useAI` wraps reads in try/catch).
### Fix Focus Areas
- modules/playground/hooks/usePreferences.ts[14-22]
### Suggested fix
Wrap both `localStorage.getItem(...)` calls in `try/catch` and return safe defaults when an exception occurs, e.g.:
- `try { return localStorage.getItem(...) ?? DEFAULT } catch { return DEFAULT }`
Also consider centralizing the ligatures key into `STORAGE_KEYS` for consistency (optional).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
2 participants
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.
fixes issue #511
Summary
Type of change
Related issue
Closes #
Validation
npm run lintnpm testnpm run buildList any additional manual verification you performed:
Screenshots or recordings
Add screenshots or short recordings for UI changes when relevant.
Checklist
Summary by CodeRabbit
New Features
Improvements