-
-
Notifications
You must be signed in to change notification settings - Fork 109
fix: editor issues on mobile. Temp fix #1365 #1366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
5e4880e
9b2d9e8
07dab1f
b79f786
d21649d
8bd5afc
e31e4dc
02f9df6
0d36f33
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,176 @@ | ||
| <script lang="ts"> | ||
| import { onDestroy, onMount } from 'svelte'; | ||
| import { Compartment, EditorState, type Extension } from '@codemirror/state'; | ||
| import { EditorView, keymap, type ViewUpdate } from '@codemirror/view'; | ||
| import { defaultKeymap, indentWithTab, historyKeymap } from '@codemirror/commands'; | ||
| import { history } from '@codemirror/history'; | ||
| import { StreamLanguage } from '@codemirror/language'; | ||
| import { linter, lintGutter, type Diagnostic, type LintSource } from '@codemirror/lint'; | ||
| import { yaml } from '@codemirror/lang-yaml'; | ||
| import { properties } from '@codemirror/legacy-modes/mode/properties'; | ||
| import jsyaml from 'js-yaml'; | ||
| import { arcaneCodeMirrorTheme } from './theme'; | ||
|
|
||
| type CodeLanguage = 'yaml' | 'env'; | ||
|
|
||
| let { | ||
| value = $bindable(''), | ||
| language = 'yaml' as CodeLanguage, | ||
| readOnly = false, | ||
| fontSize = '12px', | ||
| autoHeight = false | ||
| }: { | ||
| value: string; | ||
| language: CodeLanguage; | ||
| readOnly?: boolean; | ||
| fontSize?: string; | ||
| autoHeight?: boolean; | ||
| } = $props(); | ||
|
|
||
| let container = $state<HTMLDivElement>(); | ||
| let view = $state.raw<EditorView | null>(null); | ||
|
|
||
| const languageCompartment = new Compartment(); | ||
| const readOnlyCompartment = new Compartment(); | ||
| const themeCompartment = new Compartment(); | ||
|
|
||
| const yamlLinter: LintSource = (view): Diagnostic[] => { | ||
| const diagnostics: Diagnostic[] = []; | ||
| try { | ||
| jsyaml.load(view.state.doc.toString()); | ||
| } 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; | ||
| diagnostics.push({ | ||
| from: start, | ||
| to: end, | ||
| severity: 'error', | ||
| message: err.message || 'YAML error' | ||
| }); | ||
| } | ||
| return diagnostics; | ||
| }; | ||
|
|
||
| function getLanguageExtensions(lang: CodeLanguage): Extension[] { | ||
| if (lang === 'yaml') { | ||
| const exts: Extension[] = [yaml()]; | ||
| if (!readOnly) { | ||
| exts.push(lintGutter(), linter(yamlLinter)); | ||
| } | ||
| return exts; | ||
| } | ||
| // Best-effort .env highlighting (key/value) | ||
| return [StreamLanguage.define(properties)]; | ||
| } | ||
|
|
||
| function getThemeExtensions(size: string) { | ||
| return EditorView.theme({ | ||
| '&': { | ||
| fontSize: size | ||
| }, | ||
| '.cm-content': { | ||
| padding: '12px' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Padding inconsistency between editors: CodeMirror uses 12px padding while Monaco uses Prompt To Fix With AIThis 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. |
||
| }, | ||
| '&.cm-editor[contenteditable=false]': { | ||
| cursor: 'not-allowed' | ||
| }, | ||
| '.cm-content[contenteditable=false]': { | ||
| cursor: 'not-allowed' | ||
| }, | ||
| '.cm-scroller': { | ||
| overflow: 'auto' | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| function updateAutoHeight() { | ||
| if (!view || !container || !autoHeight) return; | ||
| const scroller = view.dom.querySelector('.cm-scroller') as HTMLElement | null; | ||
| if (!scroller) return; | ||
| container.style.height = `${scroller.scrollHeight}px`; | ||
| } | ||
|
|
||
| onMount(() => { | ||
| if (!container) return; | ||
|
|
||
| // Match prior behavior: compute theme from CSS variables at runtime. | ||
| const theme = arcaneCodeMirrorTheme(); | ||
|
|
||
| const initialState = EditorState.create({ | ||
| doc: value, | ||
| extensions: [ | ||
| history(), | ||
| keymap.of([indentWithTab, ...defaultKeymap, ...historyKeymap]), | ||
| EditorView.lineWrapping, | ||
| languageCompartment.of(getLanguageExtensions(language)), | ||
| readOnlyCompartment.of([EditorView.editable.of(!readOnly), EditorState.readOnly.of(readOnly)]), | ||
| themeCompartment.of(getThemeExtensions(fontSize)), | ||
| theme, | ||
| EditorView.updateListener.of((update: ViewUpdate) => { | ||
| if (update.docChanged) { | ||
| value = update.state.doc.toString(); | ||
| } | ||
| if (autoHeight) { | ||
| requestAnimationFrame(updateAutoHeight); | ||
| } | ||
| }) | ||
| ] | ||
| }); | ||
|
|
||
| view = new EditorView({ | ||
| state: initialState, | ||
| parent: container | ||
| }); | ||
|
|
||
| if (autoHeight) { | ||
| requestAnimationFrame(updateAutoHeight); | ||
| } | ||
| }); | ||
|
|
||
| onDestroy(() => { | ||
| view?.destroy(); | ||
| view = null; | ||
| }); | ||
|
|
||
| $effect(() => { | ||
| if (!view) return; | ||
| view.dispatch({ | ||
| effects: languageCompartment.reconfigure(getLanguageExtensions(language)) | ||
| }); | ||
| }); | ||
|
|
||
| $effect(() => { | ||
| if (!view) return; | ||
| view.dispatch({ | ||
| effects: readOnlyCompartment.reconfigure([EditorView.editable.of(!readOnly), EditorState.readOnly.of(readOnly)]) | ||
| }); | ||
| }); | ||
|
|
||
| $effect(() => { | ||
| if (!view) return; | ||
| view.dispatch({ | ||
| effects: themeCompartment.reconfigure(getThemeExtensions(fontSize)) | ||
| }); | ||
| }); | ||
|
|
||
| $effect(() => { | ||
| if (!view) return; | ||
| const current = view.state.doc.toString(); | ||
| if (value === current) return; | ||
| view.dispatch({ | ||
| changes: { from: 0, to: current.length, insert: value } | ||
| }); | ||
| }); | ||
|
|
||
| $effect(() => { | ||
| if (!container) return; | ||
| if (!autoHeight) { | ||
| container.style.height = ''; | ||
| return; | ||
| } | ||
| requestAnimationFrame(updateAutoHeight); | ||
| }); | ||
| </script> | ||
|
|
||
| <div bind:this={container} class="relative min-h-0 w-full overflow-hidden {autoHeight ? '' : 'h-full'}"></div> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| import { tags as t } from '@lezer/highlight'; | ||
| import { createTheme, type CreateThemeOptions } from '@uiw/codemirror-themes'; | ||
|
|
||
| function getAccentColor(): string { | ||
| if (typeof document === 'undefined') return 'oklch(0.606 0.25 292.717)'; | ||
| const primary = getComputedStyle(document.documentElement).getPropertyValue('--primary').trim(); | ||
| return primary || 'oklch(0.606 0.25 292.717)'; | ||
| } | ||
|
|
||
| function getAccentColorWithAlpha(alpha: number): string { | ||
| const accent = getAccentColor(); | ||
|
|
||
| if (accent.startsWith('oklch')) { | ||
| const hasAlpha = accent.includes('/'); | ||
| if (hasAlpha) { | ||
| return accent.replace(/\/\s*[\d.]+\s*\)/, ` / ${alpha})`); | ||
| } | ||
| return accent.replace(')', ` / ${alpha})`); | ||
| } | ||
|
|
||
| if (accent.startsWith('#') && accent.length >= 7) { | ||
| const r = parseInt(accent.slice(1, 3), 16); | ||
| const g = parseInt(accent.slice(3, 5), 16); | ||
| const b = parseInt(accent.slice(5, 7), 16); | ||
| return `rgba(${r}, ${g}, ${b}, ${alpha})`; | ||
| } | ||
|
|
||
| return accent; | ||
| } | ||
|
|
||
| export function arcaneCodeMirrorTheme(options?: Partial<CreateThemeOptions>) { | ||
| const { theme = 'dark', settings = {}, styles = [] } = options || {}; | ||
|
|
||
| const accentColor = getAccentColor(); | ||
| const accentWithAlpha35 = getAccentColorWithAlpha(0.35); | ||
| const accentWithAlpha15 = getAccentColorWithAlpha(0.15); | ||
| const accentWithAlpha05 = getAccentColorWithAlpha(0.05); | ||
|
|
||
| // Keep backgrounds transparent so the editor inherits the card background. | ||
| const dynamicSettings: CreateThemeOptions['settings'] = { | ||
| background: 'transparent', | ||
| foreground: 'hsl(var(--foreground))', | ||
| caret: accentColor, | ||
| selection: accentWithAlpha35, | ||
| selectionMatch: accentWithAlpha15, | ||
| lineHighlight: accentWithAlpha05, | ||
| gutterBackground: 'transparent', | ||
| gutterForeground: 'hsl(var(--muted-foreground))', | ||
| gutterActiveForeground: 'hsl(var(--foreground))', | ||
| gutterBorder: 'transparent', | ||
|
|
||
| fontFamily: | ||
| '"Geist Mono", ui-monospace, SFMono-Regular, Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace' | ||
|
Comment on lines
+52
to
+53
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 AIThis 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. |
||
| }; | ||
|
|
||
| // These colors mirror the project's previous CodeMirror implementation to ensure | ||
| // highlighting is clearly visible on mobile. | ||
| const dynamicStyles: CreateThemeOptions['styles'] = [ | ||
| { tag: [t.comment, t.meta], color: 'hsl(var(--muted-foreground))' }, | ||
| { tag: [t.keyword, t.modifier, t.operatorKeyword], color: '#ff7b72' }, | ||
|
|
||
| { tag: [t.typeName, t.namespace, t.number, t.atom, t.bool], color: '#ffa657' }, | ||
| { tag: [t.function(t.variableName), t.labelName], color: accentColor }, | ||
| { tag: [t.className, t.definition(t.variableName), t.propertyName, t.attributeName], color: '#d2a8ff' }, | ||
|
|
||
| { tag: [t.variableName, t.name], color: 'hsl(var(--foreground))' }, | ||
| { tag: [t.string, t.inserted, t.regexp, t.special(t.string)], color: '#7ee787' }, | ||
| { tag: [t.operator, t.url, t.link, t.escape], color: '#a5d6ff' }, | ||
|
|
||
| { tag: [t.separator, t.punctuation], color: 'hsl(var(--muted-foreground))' }, | ||
|
|
||
| { tag: t.heading, color: 'hsl(var(--foreground))', fontWeight: 'bold' }, | ||
| { tag: t.strong, fontWeight: 'bold' }, | ||
| { tag: t.emphasis, fontStyle: 'italic' }, | ||
| { tag: t.strikethrough, textDecoration: 'line-through' }, | ||
| { tag: t.invalid, color: 'hsl(var(--destructive))' }, | ||
| { tag: t.link, textDecoration: 'underline' } | ||
| ]; | ||
|
|
||
| return createTheme({ | ||
| theme, | ||
| settings: { ...dynamicSettings, ...settings }, | ||
| styles: [...dynamicStyles, ...styles] | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,10 @@ | ||
| <script lang="ts"> | ||
| import * as Card from '$lib/components/ui/card'; | ||
| import CodeEditor from '$lib/components/monaco-code-editor/editor.svelte'; | ||
| import MobileCodeEditor from '$lib/components/codemirror-code-editor/editor.svelte'; | ||
| import { CodeIcon } from '$lib/icons'; | ||
| import { IsMobile } from '$lib/hooks/is-mobile.svelte.js'; | ||
| import { IsTouchDevice } from '$lib/hooks/is-touch-device.svelte.js'; | ||
|
|
||
| type CodeLanguage = 'yaml' | 'env'; | ||
|
|
||
|
|
@@ -25,7 +27,9 @@ | |
| } = $props(); | ||
|
|
||
| const isMobile = new IsMobile(); | ||
| const isTouchDevice = new IsTouchDevice(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The 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 AIThis 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. |
||
| const effectiveAutoHeight = $derived(autoHeight || isMobile.current); | ||
| const useMobileEditor = $derived(isMobile.current || isTouchDevice.current); | ||
| </script> | ||
|
|
||
| <Card.Root class="flex {effectiveAutoHeight ? '' : 'flex-1'} min-h-0 flex-col overflow-hidden"> | ||
|
|
@@ -35,8 +39,10 @@ | |
| </Card.Title> | ||
| </Card.Header> | ||
| <Card.Content class="relative z-0 flex min-h-0 {effectiveAutoHeight ? '' : 'flex-1'} flex-col overflow-visible p-0"> | ||
| <div class="{effectiveAutoHeight ? '' : 'relative flex-1'} min-h-0 w-full min-w-0"> | ||
| {#if effectiveAutoHeight} | ||
| <div class="{effectiveAutoHeight ? '' : 'relative flex-1'} min-h-0 w-full min-w-0 overflow-visible"> | ||
| {#if useMobileEditor} | ||
| <MobileCodeEditor bind:value {language} fontSize="13px" autoHeight={effectiveAutoHeight} {readOnly} /> | ||
| {:else if effectiveAutoHeight} | ||
| <CodeEditor bind:value {language} fontSize="13px" autoHeight={true} {readOnly} /> | ||
| {:else} | ||
| <div class="absolute inset-0"> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
Math.maxcall is redundant here since both arguments evaluate to the same value whenerr.mark.positionis defined.When
err.mark?.positionexists,startis already set toerr.mark.position(line 43), so this becomesMath.max(err.mark.position + 1, err.mark.position + 1), which is justerr.mark.position + 1. Consider simplifying:Prompt To Fix With AI