refactor(frontend): migrate desktop app to solid#30
Merged
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
markbang
force-pushed
the
refactor/use_solid
branch
from
6334c17 to
0770b08
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
36 issues found across 156 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="README.md">
<violation number="1" location="README.md:87">
P2: Replace the placeholder clone URL with this repository's real GitHub URL; otherwise the documented setup command does not work.</violation>
</file>
<file name="src/components/i18n-provider.tsx">
<violation number="1" location="src/components/i18n-provider.tsx:26">
P2: Initialize the locale from `detectLocale()` instead of hard-coding `'en'`, otherwise the first render flashes English before switching to the user's saved/detected language.</violation>
</file>
<file name="src/components/writing-area.tsx">
<violation number="1" location="src/components/writing-area.tsx:74">
P1: Race condition: rapid file-path changes can cause a stale `readText` result to overwrite the correct content. The async IIFE has no guard to discard results that belong to a superseded effect run.
Capture a version counter (or use an `AbortController`-style flag) and check it after the `await` to discard stale reads.</violation>
</file>
<file name="test/components/theme-consistency.test.tsx">
<violation number="1" location="test/components/theme-consistency.test.tsx:7">
P2: This rewrite turns the theme-consistency suite into route smoke tests, so it no longer verifies the styling regressions the file is supposed to catch.</violation>
</file>
<file name="vite.config.ts">
<violation number="1" location="vite.config.ts:36">
P1: `esnext` is too new for Tauri's macOS/Linux webviews; use Tauri's platform-specific build targets instead.</violation>
</file>
<file name="src/components/s3-config-dialog.tsx">
<violation number="1" location="src/components/s3-config-dialog.tsx:45">
P2: Reset form/config state in the load-error path to avoid showing stale S3 data after a failed reload.</violation>
</file>
<file name="tsconfig.json">
<violation number="1" location="tsconfig.json:24">
P2: This tsconfig stops type-checking the `test/` suite and test configs, so `pnpm lint`/`pnpm build` can miss TypeScript errors outside `src/`.</violation>
</file>
<file name="src/state/workspace.ts">
<violation number="1" location="src/state/workspace.ts:36">
P1: Provide fallback values for properties parsed from local storage to prevent runtime crashes from missing keys.</violation>
<violation number="2" location="src/state/workspace.ts:90">
P2: Add a check to prevent async race conditions when loading files.</violation>
</file>
<file name="src/lib/tauri.ts">
<violation number="1" location="src/lib/tauri.ts:112">
P2: Missing `isTauri()` guard on filesystem and path operations. The rest of the module consistently checks `isTauri()` before dynamic-importing Tauri plugins, but these seven functions (`readDirectory`, `readText`, `writeText`, `readBinary`, `writeBinary`, `ensureDirectory`, `joinPaths`) skip the check. A call outside the Tauri context will produce a confusing dynamic-import error instead of the clear desktop-only message.</violation>
</file>
<file name="test/lib/utils.test.ts">
<violation number="1" location="test/lib/utils.test.ts:13">
P2: This replacement test is too weak to catch the `cn()` regression where conflicting Tailwind classes are no longer merged.</violation>
</file>
<file name="test/components/color-harmony.test.tsx">
<violation number="1" location="test/components/color-harmony.test.tsx:10">
P2: Anchor this assertion to the start of a CSS declaration; the current substring check also matches `--card-foreground` and other `*-foreground` tokens, so it can pass when `--foreground` is wrong.</violation>
<violation number="2" location="test/components/color-harmony.test.tsx:18">
P2: This dark-mode assertion needs the same declaration-line anchoring; otherwise `--card-foreground`/`--secondary-foreground` can satisfy it even if `.dark` no longer overrides `--foreground`.</violation>
</file>
<file name="src/components/theme-toggle.tsx">
<violation number="1" location="src/components/theme-toggle.tsx:9">
P2: Make `isDark` react to system color-scheme changes. In `system` mode the app theme updates on `matchMedia` events, but this memo only depends on `settings.theme`, so the toggle icon and labels can drift out of sync.</violation>
</file>
<file name="src/lib/s3.ts">
<violation number="1" location="src/lib/s3.ts:17">
P1: Normalize blank optional S3 fields before saving. The dialog submits `''` for `endpoint_url` and `path_prefix`, and Rust treats those as configured values, so leaving the endpoint blank can break every upload.</violation>
</file>
<file name="src/routes/login.tsx">
<violation number="1" location="src/routes/login.tsx:37">
P1: This `onSubmit` permanently disables the email/password login flow by preventing submission without invoking any auth logic.</violation>
<violation number="2" location="src/routes/login.tsx:39">
P2: These social sign-in buttons are inert because they have no `onClick` handler and `Button` defaults to `type="button"`.</violation>
<violation number="3" location="src/routes/login.tsx:61">
P2: These links are broken because `href="#"` only navigates to the current page hash instead of the intended destination.</violation>
</file>
<file name="test/components/interaction-states.test.tsx">
<violation number="1" location="test/components/interaction-states.test.tsx:8">
P2: This rewrite drops the hover/focus/pressed-state assertions, so interaction-style regressions now go untested.</violation>
<violation number="2" location="test/components/interaction-states.test.tsx:37">
P3: Clear the global `window.alert` mock and assert the fallback message explicitly; `toHaveBeenCalled()` can pass from leftover call history instead of this click.</violation>
</file>
<file name="src/lib/utils.ts">
<violation number="1" location="src/lib/utils.ts:17">
P2: Strip trailing separators before taking the last segment; paths ending in `/` or `\\` currently return the full path instead of the folder name.</violation>
<violation number="2" location="src/lib/utils.ts:28">
P2: Match word tokens instead of splitting on whitespace here; Chinese punctuation is currently counted as an extra word.</violation>
</file>
<file name="src/lib/markdown.ts">
<violation number="1" location="src/lib/markdown.ts:10">
P2: Handle rendered markdown links explicitly; plain anchors in the preview will navigate the app window away from the editor.</violation>
<violation number="2" location="src/lib/markdown.ts:11">
P1: Resolve workspace image paths before returning preview HTML; relative markdown image URLs will not display in the Tauri preview.</violation>
</file>
<file name="src/components/file-browser.tsx">
<violation number="1" location="src/components/file-browser.tsx:45">
P1: Restrict new-file creation to the active workspace folder. Right now the save dialog can return a path outside `workspace.folderPath`, which leaves the editor on a file the browser cannot list or refresh correctly.</violation>
</file>
<file name="index.html">
<violation number="1" location="index.html:2">
P2: Hard-coding `class="light"` causes a wrong initial paint for saved dark-theme users until `applyTheme()` runs.</violation>
</file>
<file name="src/components/ui.tsx">
<violation number="1" location="src/components/ui.tsx:112">
P2: Forward native label attributes here; this wrapper currently drops `id`, `aria-*`, `data-*`, and other standard `<label>` props.</violation>
</file>
<file name="test/hooks/useI18n.test.tsx">
<violation number="1" location="test/hooks/useI18n.test.tsx:59">
P2: Assert the translated string here instead of any truthy value. Returning the key itself would still satisfy `toBeTruthy()` and hide i18n regressions.</violation>
</file>
<file name="src/state/editor.ts">
<violation number="1" location="src/state/editor.ts:33">
P1: `markAsSaved` should not overwrite the live `markdown` value or force `isModified` to `false`. If the user types while a save is in flight, this can lose those newer edits or mark them as already saved.</violation>
</file>
<file name="src/routes/dashboard.tsx">
<violation number="1" location="src/routes/dashboard.tsx:42">
P2: Use the autosave setting for this stat instead of `folderPath`; it currently reports autosave as ON whenever a folder is open.</violation>
<violation number="2" location="src/routes/dashboard.tsx:115">
P2: Restore the recent file's workspace before navigating home; setting only `selectedFilePath` can leave the editor hidden or the file browser out of sync.</violation>
<violation number="3" location="src/routes/dashboard.tsx:163">
P2: This `S3 Settings` shortcut only navigates home; it does not open any settings UI.</violation>
</file>
<file name="test/components/theme-core.test.tsx">
<violation number="1" location="test/components/theme-core.test.tsx:21">
P3: This test duplicates the existing `color-harmony.test.tsx` palette assertions and adds maintenance-only coverage.</violation>
</file>
<file name="test/components/comprehensive-theme.test.tsx">
<violation number="1" location="test/components/comprehensive-theme.test.tsx:13">
P2: This file no longer tests theme behavior, so the documented comprehensive theme suite loses its intended coverage.</violation>
</file>
<file name="src/routes/index.tsx">
<violation number="1" location="src/routes/index.tsx:35">
P2: This extra `loadFiles()` call duplicates the file scan that `FileBrowser` already triggers on `folderPath` changes.</violation>
<violation number="2" location="src/routes/index.tsx:121">
P2: `overflow-hidden` makes the welcome screen non-scrollable, so parts of the empty-state UI can be clipped behind the fixed header/footer.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| createEffect(() => { | ||
| const path = workspace.selectedFilePath | ||
|
|
||
| void (async () => { |
There was a problem hiding this comment.
P1: Race condition: rapid file-path changes can cause a stale readText result to overwrite the correct content. The async IIFE has no guard to discard results that belong to a superseded effect run.
Capture a version counter (or use an AbortController-style flag) and check it after the await to discard stale reads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/writing-area.tsx, line 74:
<comment>Race condition: rapid file-path changes can cause a stale `readText` result to overwrite the correct content. The async IIFE has no guard to discard results that belong to a superseded effect run.
Capture a version counter (or use an `AbortController`-style flag) and check it after the `await` to discard stale reads.</comment>
<file context>
@@ -0,0 +1,492 @@
+ createEffect(() => {
+ const path = workspace.selectedFilePath
+
+ void (async () => {
+ if (!path) {
+ editorActions.reset()
</file context>
| solid(), | ||
| ], | ||
| build: { | ||
| target: 'esnext', |
There was a problem hiding this comment.
P1: esnext is too new for Tauri's macOS/Linux webviews; use Tauri's platform-specific build targets instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At vite.config.ts, line 36:
<comment>`esnext` is too new for Tauri's macOS/Linux webviews; use Tauri's platform-specific build targets instead.</comment>
<file context>
@@ -0,0 +1,38 @@
+ solid(),
+ ],
+ build: {
+ target: 'esnext',
+ },
+})
</file context>
| recentFiles: [], | ||
| }) | ||
|
|
||
| const [workspace, setWorkspace] = createStore<WorkspaceState>({ |
There was a problem hiding this comment.
P1: Provide fallback values for properties parsed from local storage to prevent runtime crashes from missing keys.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/state/workspace.ts, line 36:
<comment>Provide fallback values for properties parsed from local storage to prevent runtime crashes from missing keys.</comment>
<file context>
@@ -0,0 +1,146 @@
+ recentFiles: [],
+})
+
+const [workspace, setWorkspace] = createStore<WorkspaceState>({
+ folderPath: persisted.folderPath,
+ selectedFilePath: persisted.selectedFilePath,
</file context>
| } | ||
|
|
||
| export async function saveS3Config(config: S3Config) { | ||
| return invokeCommand('save_s3_config', { config }) |
There was a problem hiding this comment.
P1: Normalize blank optional S3 fields before saving. The dialog submits '' for endpoint_url and path_prefix, and Rust treats those as configured values, so leaving the endpoint blank can break every upload.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/s3.ts, line 17:
<comment>Normalize blank optional S3 fields before saving. The dialog submits `''` for `endpoint_url` and `path_prefix`, and Rust treats those as configured values, so leaving the endpoint blank can break every upload.</comment>
<file context>
@@ -0,0 +1,29 @@
+}
+
+export async function saveS3Config(config: S3Config) {
+ return invokeCommand('save_s3_config', { config })
+}
+
</file context>
| <CardDescription class="text-sm font-medium">{t('login.subtitle')}</CardDescription> | ||
| </CardHeader> | ||
| <CardContent> | ||
| <form class="grid gap-6" onSubmit={(event) => event.preventDefault()}> |
There was a problem hiding this comment.
P1: This onSubmit permanently disables the email/password login flow by preventing submission without invoking any auth logic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/routes/login.tsx, line 37:
<comment>This `onSubmit` permanently disables the email/password login flow by preventing submission without invoking any auth logic.</comment>
<file context>
@@ -0,0 +1,96 @@
+ <CardDescription class="text-sm font-medium">{t('login.subtitle')}</CardDescription>
+ </CardHeader>
+ <CardContent>
+ <form class="grid gap-6" onSubmit={(event) => event.preventDefault()}>
+ <div class="flex flex-col gap-3">
+ <Button variant="outline" class="w-full">
</file context>
| })) | ||
|
|
||
| describe('solid route integration', () => { | ||
| it('renders dashboard stats from workspace state', () => { |
There was a problem hiding this comment.
P2: This file no longer tests theme behavior, so the documented comprehensive theme suite loses its intended coverage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/components/comprehensive-theme.test.tsx, line 13:
<comment>This file no longer tests theme behavior, so the documented comprehensive theme suite loses its intended coverage.</comment>
<file context>
@@ -1,222 +1,46 @@
+}))
+
+describe('solid route integration', () => {
+ it('renders dashboard stats from workspace state', () => {
+ workspaceActions.setFolderPath('/workspace')
+ workspaceActions.setFiles([
</file context>
|
|
||
| const [isCheckingUpdate, setIsCheckingUpdate] = useState(false); | ||
| workspaceActions.setFolderPath(result) | ||
| await workspaceActions.loadFiles() |
There was a problem hiding this comment.
P2: This extra loadFiles() call duplicates the file scan that FileBrowser already triggers on folderPath changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/routes/index.tsx, line 35:
<comment>This extra `loadFiles()` call duplicates the file scan that `FileBrowser` already triggers on `folderPath` changes.</comment>
<file context>
@@ -0,0 +1,257 @@
+ if (!result) return
+
+ workspaceActions.setFolderPath(result)
+ await workspaceActions.loadFiles()
+ } catch (error) {
+ toast.error(getErrorMessage(error))
</file context>
| <div className="h-[8px] flex-1 bg-foreground"></div> | ||
| <div className="border-2 border-foreground p-1"> | ||
| <div className="w-4 h-4 bg-foreground"></div> | ||
| <div class="flex flex-1 overflow-hidden pb-20 pt-24"> |
There was a problem hiding this comment.
P2: overflow-hidden makes the welcome screen non-scrollable, so parts of the empty-state UI can be clipped behind the fixed header/footer.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/routes/index.tsx, line 121:
<comment>`overflow-hidden` makes the welcome screen non-scrollable, so parts of the empty-state UI can be clipped behind the fixed header/footer.</comment>
<file context>
@@ -0,0 +1,257 @@
+ </Card>
+ </div>
+
+ <div class="flex flex-1 overflow-hidden pb-20 pt-24">
+ <Show
+ when={workspace.folderPath}
</file context>
| }, | ||
| ] | ||
| it('switches the app language from the landing page selector', async () => { | ||
| render(() => <HomePage />) |
There was a problem hiding this comment.
P3: Clear the global window.alert mock and assert the fallback message explicitly; toHaveBeenCalled() can pass from leftover call history instead of this click.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/components/interaction-states.test.tsx, line 37:
<comment>Clear the global `window.alert` mock and assert the fallback message explicitly; `toHaveBeenCalled()` can pass from leftover call history instead of this click.</comment>
<file context>
@@ -1,182 +1,56 @@
- },
- ]
+ it('switches the app language from the landing page selector', async () => {
+ render(() => <HomePage />)
- render(<NavDocuments items={mockDocuments} />, { withSidebar: true })
</file context>
| expect(container.innerHTML).toContain('Bordered content') | ||
| expect(container.innerHTML).toContain('<div') | ||
| }) | ||
| it('defines light and dark theme tokens in the stylesheet', () => { |
There was a problem hiding this comment.
P3: This test duplicates the existing color-harmony.test.tsx palette assertions and adds maintenance-only coverage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/components/theme-core.test.tsx, line 21:
<comment>This test duplicates the existing `color-harmony.test.tsx` palette assertions and adds maintenance-only coverage.</comment>
<file context>
@@ -1,204 +1,39 @@
- expect(container.innerHTML).toContain('Bordered content')
- expect(container.innerHTML).toContain('<div')
- })
+ it('defines light and dark theme tokens in the stylesheet', () => {
+ expect(globalsCss).toContain('--background: #ffffff;')
+ expect(globalsCss).toContain('--foreground: #000000;')
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Notes
ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL Command "test:visual" not found
Did you mean "pnpm test:visual"? after installing the required system libraries
Summary by cubic
Migrated the desktop frontend from React/Next to SolidJS with
viteto simplify the stack, speed up startup, and reduce dependencies. Updated tests, Tauri config, and docs to match the new toolchain.Refactors
vite(index.html,src/app.tsx) and added Solid implementations for i18n, theme toggle, file browser, S3 config dialog, toast viewport, and lightweight UI insrc/components/ui.tsx.vitest+playwright; set baseURL tohttp://127.0.0.1:3000, default to Chromium, add locale/timezone; enable WebKit withPLAYWRIGHT_ENABLE_WEBKIT=1.frontendDist: dist) and addedpnpm tauriscript.Migration
pnpm dev, Build:pnpm build, Preview:pnpm preview, Lint:pnpm lint(type-check only).pnpm test:visual(Chromium). Enable WebKit withPLAYWRIGHT_ENABLE_WEBKIT=1 pnpm test:visual.http://127.0.0.1:3000; web server usespnpm dev.pnpm tauri.Written for commit 0770b08. Summary will update on new commits.