feat(frontend): mobile-friendly config editor with find bar and unsaved-changes guard - #326
Merged
Merged
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds a mirrored code editor with search and highlighting, integrates it into AGENTS.md and OpenCode configuration editors, adds JSONC validation line mapping, consolidates destructive dialogs, and improves dialog behavior for nested Escape events and mobile keyboard insets. ChangesSettings editor controls
Dialog and viewport behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant OpenCodeConfigEditor
participant CodeEditor
participant OpenCodeConfigManager
participant JSONCUtilities
User->>CodeEditor: edit raw JSONC
CodeEditor->>OpenCodeConfigEditor: emit content change
OpenCodeConfigEditor->>JSONCUtilities: resolve validation issue lines
OpenCodeConfigEditor->>OpenCodeConfigManager: submit validated content
OpenCodeConfigManager-->>OpenCodeConfigEditor: complete save and refresh
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
frontend/src/hooks/useVisualViewport.test.tsx (1)
41-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the TypeScript suppression comments. Use
Reflect.deleteProperty(window, 'visualViewport')instead.
frontend/src/hooks/useVisualViewport.test.tsx#L41-L42: replace the suppression anddeleteexpression.frontend/src/components/ui/dialog.test.tsx#L409-L410: replace the suppression anddeleteexpression.Proposed fix
- // `@ts-expect-error` allow delete - delete window.visualViewport + Reflect.deleteProperty(window, 'visualViewport')🤖 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 `@frontend/src/hooks/useVisualViewport.test.tsx` around lines 41 - 42, Remove the TypeScript suppression and direct delete expression in frontend/src/hooks/useVisualViewport.test.tsx lines 41-42, replacing it with Reflect.deleteProperty(window, 'visualViewport'). Apply the same replacement in frontend/src/components/ui/dialog.test.tsx lines 409-410; no other changes are needed.Source: Coding guidelines
backend/src/routes/repos.ts (1)
363-385: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePre-flight validation duplicates
retryCloneRepo's own checks.Lines 371-380 mirror the service's existence/
repoUrl/state guards. Keeping both means two places to update when retryable states change (already diverging withclaimRepoForRetry). Consider having the service throw status-annotated errors and letting the route just map them.🤖 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 `@backend/src/routes/repos.ts` around lines 363 - 385, Remove the duplicated repository existence, remote URL, and clone-state validation from the `/:id/retry-clone` route, while retaining request ID parsing and the assistant-repository guard. Update `repoService.retryCloneRepo` to own these checks and throw status-annotated errors, then have the route catch and map those errors to the corresponding JSON responses and HTTP statuses.Source: Coding guidelines
backend/src/services/repo.ts (1)
885-889: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExplanatory comments across the new code conflict with the repo's no-comments rule.
backend/src/services/repo.ts#L885-L889: remove this and the other rationale blocks (Lines 645-648, 899-902, 924-927, 956-962).backend/src/db/queries.ts#L294-L299: remove the JSDoc block; the function name conveys the intent.backend/test/services/repo-retry.test.ts#L230-L232: remove the inline comments here and at Lines 277-278, 328-330, 352-354, 374-375.As per coding guidelines: "Do not add comments; code should be self-documenting."
🤖 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 `@backend/src/services/repo.ts` around lines 885 - 889, Remove the explanatory comments added for the retry/concurrency changes, keeping the implementation unchanged: delete the rationale blocks in backend/src/services/repo.ts at lines 645-648, 885-889, 899-902, 924-927, and 956-962; remove the JSDoc above the relevant function in backend/src/db/queries.ts at lines 294-299; and remove the inline comments in backend/test/services/repo-retry.test.ts at lines 230-232, 277-278, 328-330, 352-354, and 374-375.Source: Coding guidelines
backend/test/routes/repos.test.ts (1)
342-394: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the
cloningretryable state and the guard branches.The suite covers
erroronly. A test withcloneStatus: 'cloning'would surface the claim mismatch flagged inbackend/src/db/queries.ts. TheNumber.isNaN(id)andASSISTANT_REPO_IDbranches (route Lines 366-369) are also untested.🤖 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 `@backend/test/routes/repos.test.ts` around lines 342 - 394, The retry-clone route tests only cover the error state and miss key guard branches. Extend the suite around createRepoRoutes and the retry-clone endpoint with a successful retry case using cloneStatus: 'cloning', plus requests that exercise invalid numeric IDs triggering Number.isNaN(id) and the ASSISTANT_REPO_ID guard; assert each branch’s expected status and that retryCloneRepo is not called when validation rejects the request.Source: Coding guidelines
frontend/src/components/ui/code-editor.test.tsx (1)
247-269: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis test doesn't actually exercise the "fallback" path it claims to test.
With
highlights={[{ startIndex: 0, endIndex: 4 }]}andactiveHighlightIndex={0}, amark[data-active-match="true"]element is always rendered (per thefirst && h.index === activeHighlightIndexlogic incode-editor.tsx), sorevealActiveMark()succeeds and short-circuits beforerevealLineruns. The assertion passes only because both code paths coincidentally computescrollTop === 0here, not because the fallback branch was verified.Consider forcing the mark lookup to genuinely fail (e.g., stub
mirror.querySelectorto returnnullfor the active-match selector, or pass anactiveHighlightIndexthat has no corresponding row rendering) so the fallback logic is actually covered.🤖 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 `@frontend/src/components/ui/code-editor.test.tsx` around lines 247 - 269, Make the fallback test genuinely exercise revealLine by forcing the active mark lookup in revealActiveMark to return no element, such as stubbing the mirror’s querySelector for the active-match selector, while keeping the existing rerender and scrollTop assertion. Ensure the test no longer succeeds through the rendered mark path in CodeEditor.
🤖 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.
Inline comments:
In `@backend/src/db/queries.ts`:
- Around line 300-306: Align retryable clone-state validation with the atomic
claim: update claimRepoForRetry in backend/src/db/queries.ts (lines 300-306) to
support error and cloning with a staleness guard, or consistently remove cloning
from validation. Apply the same chosen definition in backend/src/routes/repos.ts
(lines 378-380) and backend/src/services/repo.ts (lines 881-883), preferably
through a shared isRetryableCloneStatus helper.
In `@backend/src/services/repo.ts`:
- Around line 890-892: Update the concurrent retry error in
backend/src/services/repo.ts#L890-L892 to carry statusCode 409, following the
existing directory-collision pattern. Leave
backend/src/routes/repos.ts#L386-L389 unchanged because it will map the
annotated error automatically, and add a route test covering the concurrent
retry case with an HTTP 409 response.
- Line 963: Restrict the missing-branch fallback in the repository handling
logic to messages matching the branch-specific “Remote branch ... not found in
upstream” phrasing, removing the broad message.includes('not found') check.
Apply the same predicate to the create-time path, preferably by extracting and
reusing a shared helper.
- Around line 850-856: Update the catch block containing updateRepoStatus so any
error from the status update is handled without replacing the original clone
failure. Preserve propagation of the original caught error while still
attempting to mark the repository as errored, including when the repository was
deleted concurrently.
In `@backend/test/services/repo-retry.test.ts`:
- Around line 246-251: Update the assertions for newBranchAddCalls and the
corresponding assertion at the other affected location to use the captured call
argument list directly, removing the unknown-to-string[] cast. Assert
command-array membership or the appropriate command element explicitly,
preserving the intended verification that the command includes “develop”.
---
Nitpick comments:
In `@backend/src/routes/repos.ts`:
- Around line 363-385: Remove the duplicated repository existence, remote URL,
and clone-state validation from the `/:id/retry-clone` route, while retaining
request ID parsing and the assistant-repository guard. Update
`repoService.retryCloneRepo` to own these checks and throw status-annotated
errors, then have the route catch and map those errors to the corresponding JSON
responses and HTTP statuses.
In `@backend/src/services/repo.ts`:
- Around line 885-889: Remove the explanatory comments added for the
retry/concurrency changes, keeping the implementation unchanged: delete the
rationale blocks in backend/src/services/repo.ts at lines 645-648, 885-889,
899-902, 924-927, and 956-962; remove the JSDoc above the relevant function in
backend/src/db/queries.ts at lines 294-299; and remove the inline comments in
backend/test/services/repo-retry.test.ts at lines 230-232, 277-278, 328-330,
352-354, and 374-375.
In `@backend/test/routes/repos.test.ts`:
- Around line 342-394: The retry-clone route tests only cover the error state
and miss key guard branches. Extend the suite around createRepoRoutes and the
retry-clone endpoint with a successful retry case using cloneStatus: 'cloning',
plus requests that exercise invalid numeric IDs triggering Number.isNaN(id) and
the ASSISTANT_REPO_ID guard; assert each branch’s expected status and that
retryCloneRepo is not called when validation rejects the request.
In `@frontend/src/components/ui/code-editor.test.tsx`:
- Around line 247-269: Make the fallback test genuinely exercise revealLine by
forcing the active mark lookup in revealActiveMark to return no element, such as
stubbing the mirror’s querySelector for the active-match selector, while keeping
the existing rerender and scrollTop assertion. Ensure the test no longer
succeeds through the rendered mark path in CodeEditor.
In `@frontend/src/hooks/useVisualViewport.test.tsx`:
- Around line 41-42: Remove the TypeScript suppression and direct delete
expression in frontend/src/hooks/useVisualViewport.test.tsx lines 41-42,
replacing it with Reflect.deleteProperty(window, 'visualViewport'). Apply the
same replacement in frontend/src/components/ui/dialog.test.tsx lines 409-410; no
other changes are needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d22fae1f-ffe8-4770-93ef-e626d79ad121
📒 Files selected for processing (31)
backend/src/db/queries.tsbackend/src/routes/repos.tsbackend/src/services/repo.tsbackend/test/db/queries.test.tsbackend/test/routes/repos.test.tsbackend/test/services/repo-auth-env.test.tsbackend/test/services/repo-retry.test.tsfrontend/src/components/settings/AgentsMdEditor.test.tsxfrontend/src/components/settings/AgentsMdEditor.tsxfrontend/src/components/settings/OpenCodeConfigEditor.test.tsxfrontend/src/components/settings/OpenCodeConfigEditor.tsxfrontend/src/components/settings/OpenCodeConfigManager.test.tsxfrontend/src/components/settings/OpenCodeConfigManager.tsxfrontend/src/components/settings/SettingsDialog.test.tsxfrontend/src/components/settings/SettingsDialog.tsxfrontend/src/components/ui/code-editor.test.tsxfrontend/src/components/ui/code-editor.tsxfrontend/src/components/ui/dialog.test.tsxfrontend/src/components/ui/dialog.tsxfrontend/src/components/ui/editor-find-bar.test.tsxfrontend/src/components/ui/editor-find-bar.tsxfrontend/src/components/ui/unsaved-changes-dialog.tsxfrontend/src/hooks/useMobile.tsfrontend/src/hooks/useVisualViewport.test.tsxfrontend/src/hooks/useVisualViewport.tsfrontend/src/index.cssfrontend/src/lib/editorScroll.test.tsfrontend/src/lib/editorScroll.tsfrontend/src/lib/jsonc.test.tsfrontend/src/lib/jsonc.tsshared/src/utils/jsonc.ts
Backend drops POST /:id/retry-clone and the supporting retryCloneRepo service and claimRepoForRetry query; failed clones now delete the repo row rather than lingering in error state. Frontend extracts ConfirmDestructiveDialog shared by Delete/Discard/UnsavedChanges dialogs, memoizes CodeEditor rows, fixes scroll jump while typing away from the active match, and aligns the responsive breakpoint to 768px.
Owner
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
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
Adds a mobile-friendly config editor experience and supporting infrastructure:
CodeEditorcomponent with aneditor-find-bar(find/replace),unsaved-changes-dialogguard, and safe-area-aware layout for mobile keyboards.OpenCodeConfigEditorandAgentsMdEditorto use the new editor;SettingsDialognow hosts the unsaved-changes guard.useVisualViewporthook andeditorScrollhelper to keep the cursor visible above the on-screen keyboard.jsoncutilities hardened with tests.Type of Change
Checklist
pnpm lintpasses locallypnpm typecheckpasses locallySummary by CodeRabbit