fix: editor issues on mobile. Temp fix #1365 #1366
Conversation
|
Id rather not have two different editors as both of these are huge dependencies, and the netire frontnend is embeded in the go biniary which shouldnt be an issue but can make the biannry bigger. I have looked into the editor issues yet , but theys should be solve able with the monaco one on mobile id think? |
|
Thats fair. I poked around Monaco a bit trying to find a fix and apparently its a known issue on mobile. I was just tired of not being able to use mobile to edit so I chose this. |
|
I'll do some digging I've had to hack some stuff up to get get Monaco to work for the auto completions (that's why I switched to it), ill see what I can figure out |
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can stay up-to-date and reviewed. |
This reverts commit 5e4880e.
lucolvin
changed the title
|
@kmendell Quick question. You said you switched from code mirror to monaco for autocompletion support. Code mirror 6 has auto-completion and better mobile support. What are your thoughts about fully reverting to code mirror and dropping monaco? Or was the change away from code mirror an attempt to reduce dependencies as it has more packages? |
There was more to it the auto completion with monaco i can grab the latest compose spec and have that used as auto completion i never got it to work with code mirror. Also the svelte-codemirror package did not like me setting a static height for it , like the monaco ones are now, which i think looks alot cleaner than the editors only being as tall as the content. Overall monaco and code-mirror are both heavy deps. But monaco does have less packages, its possible to fix all of these issues with monaco im fairly confident you just have to do some hacky stuff. |
|
i suppose this is the easiest way to do this.... |
I have been messing around with having an invisible overlay on mobile that allows for native interaction with monaco. It works but its not perfect. |
|
Oh i mean feel free to keep working on it. But since mobile is a big focus of why people like arcane i guess bringing back code mirror is a bandaid fix. At least so its useable. |
That was my thinking when I added it back. I also looked into ace which could be an alternative. |
|
Honestly im not set on either or, i just didn't want to bloated deps in the binary. Do what you think is best, a requirement i think though is keeping that auto completion based on the compose spec. |
lucolvin
changed the title
|
@kmendell I re-pushed the code-mirror addition. I will continue working on a more permanent fix. I think it is totally possible to get monaco working for mobile but it will take some work. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } catch (e: unknown) { | ||
| const err = e as { mark?: { position: number }; message?: string }; | ||
| const start = err.mark?.position || 0; | ||
| const end = err.mark?.position !== undefined ? Math.max(start + 1, err.mark.position + 1) : start + 1; |
There was a problem hiding this comment.
The Math.max call is redundant here since both arguments evaluate to the same value when err.mark.position is defined.
When err.mark?.position exists, start is already set to err.mark.position (line 43), so this becomes Math.max(err.mark.position + 1, err.mark.position + 1), which is just err.mark.position + 1. Consider simplifying:
| const end = err.mark?.position !== undefined ? Math.max(start + 1, err.mark.position + 1) : start + 1; | |
| const end = err.mark?.position !== undefined ? start + 1 : start + 1; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/lib/components/codemirror-code-editor/editor.svelte
Line: 44:44
Comment:
The `Math.max` call is redundant here since both arguments evaluate to the same value when `err.mark.position` is defined.
When `err.mark?.position` exists, `start` is already set to `err.mark.position` (line 43), so this becomes `Math.max(err.mark.position + 1, err.mark.position + 1)`, which is just `err.mark.position + 1`. Consider simplifying:
```suggestion
const end = err.mark?.position !== undefined ? start + 1 : start + 1;
```
How can I resolve this? If you propose a fix, please make it concise.| value = $bindable(''), | ||
| language = 'yaml' as CodeLanguage, | ||
| readOnly = false, | ||
| fontSize = '13px', |
There was a problem hiding this comment.
Font size default inconsistency: CodeMirror defaults to '13px' while Monaco defaults to '12px' (monaco-code-editor/editor.svelte line 14). Since both editors are used interchangeably in CodePanel for the same content, they should have matching defaults to ensure consistent appearance across devices.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/lib/components/codemirror-code-editor/editor.svelte
Line: 20:20
Comment:
Font size default inconsistency: CodeMirror defaults to '13px' while Monaco defaults to '12px' (monaco-code-editor/editor.svelte line 14). Since both editors are used interchangeably in CodePanel for the same content, they should have matching defaults to ensure consistent appearance across devices.
How can I resolve this? If you propose a fix, please make it concise.| fontFamily: | ||
| '"Geist Mono", ui-monospace, SFMono-Regular, Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace' |
There was a problem hiding this comment.
Font family inconsistency: CodeMirror includes "Geist Mono" as the primary font, but Monaco editor (monaco-code-editor/editor.svelte line 115) uses 'ui-monospace' as primary and doesn't include "Geist Mono" at all. Since these editors display the same content on different devices, consider aligning the font stacks for visual consistency.
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/lib/components/codemirror-code-editor/theme.ts
Line: 52:53
Comment:
Font family inconsistency: CodeMirror includes "Geist Mono" as the primary font, but Monaco editor (monaco-code-editor/editor.svelte line 115) uses 'ui-monospace' as primary and doesn't include "Geist Mono" at all. Since these editors display the same content on different devices, consider aligning the font stacks for visual consistency.
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.| fontSize: size | ||
| }, | ||
| '.cm-content': { | ||
| padding: '12px' |
There was a problem hiding this comment.
Padding inconsistency between editors: CodeMirror uses 12px padding while Monaco uses padding: { top: 10, bottom: 10 } (monaco-code-editor/editor.svelte line 117). This creates a different visual appearance when switching between desktop and mobile. Consider standardizing the padding values.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/lib/components/codemirror-code-editor/editor.svelte
Line: 73:73
Comment:
Padding inconsistency between editors: CodeMirror uses 12px padding while Monaco uses `padding: { top: 10, bottom: 10 }` (monaco-code-editor/editor.svelte line 117). This creates a different visual appearance when switching between desktop and mobile. Consider standardizing the padding values.
How can I resolve this? If you propose a fix, please make it concise.| } = $props(); | ||
| const isMobile = new IsMobile(); | ||
| const isTouchDevice = new IsTouchDevice(); |
There was a problem hiding this comment.
The IsTouchDevice hook is already imported but you're also checking isMobile.current separately. On most mobile devices, both would be true (mobile screen size + touch capability), making the check somewhat redundant. Consider documenting why both checks are needed, or if the intent is to support desktop devices with touchscreens (like Surface devices), the current logic is correct.
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/routes/(app)/projects/components/CodePanel.svelte
Line: 30:30
Comment:
The `IsTouchDevice` hook is already imported but you're also checking `isMobile.current` separately. On most mobile devices, both would be true (mobile screen size + touch capability), making the check somewhat redundant. Consider documenting why both checks are needed, or if the intent is to support desktop devices with touchscreens (like Surface devices), the current logic is correct.
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.
|
@lucolvin See this c5a5c8a#diff-9a9751527e826ccdd1ddd4239030e98de6c8614b1cd53f3e8e63bbcd8f381fee. Thats when i removed it, i see some inconsistencies with what i had before vs what you have now, |
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can stay up-to-date and reviewed. |
2 participants
Greptile Overview
Greptile Summary
This PR reintroduces CodeMirror as the code editor for mobile and touch devices while preserving Monaco for desktop users. The implementation conditionally renders editors based on device capabilities.
Key changes:
@codemirror/*packages) to support mobile editingCodePanel.svelteto detect mobile/touch devices and switch between editors accordinglyIssues found:
theme.ts: accessingdocumentwithout browser environment check will cause server-side rendering failuresIsTouchDevicehookConfidence Score: 3/5
documentwithout checks) that will cause runtime failures during server-side rendering, plus minor code duplication. The SSR issue must be fixed before merging to prevent build/deployment failures.frontend/src/lib/components/codemirror-code-editor/theme.ts- the SSR bug will cause failuresImportant Files Changed
File Analysis
documentwithout browser checkDisclaimer Greptiles Reviews use AI, make sure to check over its work
Greptile Overview
Greptile Summary
This PR reintroduces CodeMirror as the mobile/touch editor while keeping Monaco for desktop, addressing mobile editor functionality issues. The implementation adds comprehensive CodeMirror support with YAML linting, dynamic theming, and proper Svelte 5 integration.
Changes Overview
New CodeMirror Implementation:
codemirror-code-editor/editor.sveltewith YAML/env language support, linting via js-yaml, and auto-height functionalitycodemirror-code-editor/theme.tswith dynamic color system that reads CSS variables to match app accent colorsDevice Detection & Routing:
CodePanel.svelteto conditionally render CodeMirror for mobile/touch devices and Monaco for desktopIsMobileandIsTouchDevicehooks for device detectionMonaco Editor Updates:
fixedOverflowWidgetsfromtruetofalseto allow suggestion widgets to overflow card boundariesselectionClipboard: truefor middle-click paste support on Linuxapp.cssto handle Monaco widget positioningBuild Configuration:
Critical Issue Found
Permission Bypass on Mobile (MUST FIX): The
readOnlyprop is not passed toMobileCodeEditorinCodePanel.svelteline 44. SincereadOnlyis used to enforce edit permissions based oncanEditCompose(see[projectId]/+page.sveltelines 494, 559), mobile/touch users can edit content even when they lack permission. This is a security/permission control issue.Style Issues
Multiple visual inconsistencies between CodeMirror and Monaco editors:
These inconsistencies mean the editor will look and feel different when switching between desktop and mobile devices viewing the same content.
Technical Implementation Quality
Strengths:
Note on "SSR Bug" Claim: The PR description mentions an SSR bug in theme.ts accessing
documentwithout checks, but the code actually has proper guards (typeof document === 'undefined') and the function is only called inonMountwhich is browser-only. The SSR concern appears to be unfounded.Testing Recommendations
Confidence Score: 2/5
Important Files Changed
File Analysis