Integrate Ralph Loop and enhance Git control UI/UX features - #8
Conversation
- Adopted Ralph Loop pattern from ChristianKuri/ralph-and-opencode - Added `ralph.sh` script to orchestrate the loop execution - Added `.opencode/skills/` globally available OpenCode agent skills - Added `prompt.md` instructions and `prd.json.example` format definition - Updated `AGENTS.md` with instructions on how to use Ralph Loop Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
…, files, and tasks views. Uses GitPython in the backend to provide basic repository management, allowing users to initialize repos, view status/changes, stage and commit files, and push/pull to/from a remote origin. Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
- Adopted Ralph Loop pattern from ChristianKuri/ralph-and-opencode - Added `ralph.sh` script to orchestrate the loop execution - Added `.opencode/skills/` globally available OpenCode agent skills - Added `prompt.md` instructions and `prd.json.example` format definition - Updated `AGENTS.md` with instructions on how to use Ralph Loop - Added UI checkboxes to "Create Chat Project" modals to scaffold Ralph Loop - Backend copies Ralph Loop files securely without using shell subprocesses Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
…git push by auto-setting upstream when pushing from a new branch.\n- Enhances Git UI styling so background cards look consistent with the rest of the application.\n- Updates GitView to correctly use React FormEvent typings. Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Rishabh-Bajpai/mobile-opencode-control/sessions/ae4cf8a5-8b07-4e1d-98a3-f3be8c995b13 Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Rishabh-Bajpai/mobile-opencode-control/sessions/ae4cf8a5-8b07-4e1d-98a3-f3be8c995b13 Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Rishabh-Bajpai/mobile-opencode-control/sessions/ae4cf8a5-8b07-4e1d-98a3-f3be8c995b13 Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Rishabh-Bajpai/mobile-opencode-control/sessions/ae4cf8a5-8b07-4e1d-98a3-f3be8c995b13 Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
…s for Git repository management.
…116035049 feat: add Git control to project view
… inline-styled checkbox Agent-Logs-Url: https://github.com/Rishabh-Bajpai/mobile-opencode-control/sessions/e8245915-d516-4bab-b1a0-3909966004a2 Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
…prd routes Agent-Logs-Url: https://github.com/Rishabh-Bajpai/mobile-opencode-control/sessions/e8245915-d516-4bab-b1a0-3909966004a2 Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
…trace exposure Agent-Logs-Url: https://github.com/Rishabh-Bajpai/mobile-opencode-control/sessions/e8245915-d516-4bab-b1a0-3909966004a2 Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
…p-integration-4511484040334154221 # Conflicts: # frontend/src/api.ts Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
…-4511484040334154221 feat: Integrate Ralph Loop mechanism for automated app development
…ent, and button consistency
…for GitView components
…idation for empty question sets
…d layout from notification-controls class
…ant settings module
…nd comprehensive API documentation
… session bug progress
…g, and tool call card improvements
…new backend endpoint and tabbed diff panel.
…om MessagePartCard
…-met toggle to the row
… to use standardized selects
… messages by adding horizontal scrolling wrappers
…rontend support for tracking context usage and compaction updates.
…omponent structure
…orporating global defaults
…d approvals, and add debouncing for assistant message notifications
Agent-Logs-Url: https://github.com/Rishabh-Bajpai/mobile-opencode-control/sessions/21222144-c657-46cf-957d-18faf75e6449 Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Rishabh-Bajpai/mobile-opencode-control/sessions/21222144-c657-46cf-957d-18faf75e6449 Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Rishabh-Bajpai/mobile-opencode-control/sessions/21222144-c657-46cf-957d-18faf75e6449 Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
Enhance project features, UI improvements, and documentation updates
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughBackend routes were modularized with helpers and new endpoints across sessions, messages/streaming, files, runtime, tasks, STT/TTS, notifications, and opencode. A full Git API was added and registered. Frontend gained Git UI, file browser, chat rendering, runtime/notification controls, PRD/tasks panels, extensive utils/types, API wrappers, and styles. Docs/skills/scripts updated. ChangesUnified Backend and Frontend Expansion
Sequence Diagram(s)sequenceDiagram
participant Frontend
participant API as Backend /api
participant OpenCode as OpenCodeClient
participant Git as GitPython Repo
Frontend->>API: GET /projects/:id/messages
API->>OpenCode: list_messages(session)
OpenCode-->>API: messages
API-->>Frontend: messages + timeline
Frontend->>API: GET /projects/:id/git/status
API->>Git: diff/status/heads
Git-->>API: repo state
API-->>Frontend: status/branches/history
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
PR #8 Review: "Integrate Ralph Loop and enhance Git control UI/UX features"49 commits | 81 files changed | +18,003 / -7,483 lines | 🔴 Critical BugB1.
|
| # | File | Issue | Severity |
|---|---|---|---|
| S1 | backend/app/__init__.py:30-36 |
SQL built with f-strings in _copy_legacy_rows — fragile pattern, should use SQLAlchemy text() with bound parameters |
Low |
| S2 | backend/app/routes/sessions.py:108, projects.py:91, messages.py:57 |
str(exc) in error responses leaks internal paths, SQL details, library internals. Fixed in PRD routes only, but all other routes still expose raw exception strings |
Medium |
| S3 | backend/app/routes/helpers.py |
ntfy topic_url not validated — a malicious user could set it to an internal endpoint (SSRF) |
Medium |
| S4 | backend/app/git_routes.py:591 |
Git remote URL accepts any scheme including file:// — no validation |
Low |
| S5 | backend/app/routes/helpers.py:130-143 |
Symlink traversal possible in _resolve_project_relative_path — .resolve() follows symlinks, check could be bypassed |
Low |
🟠 Other Bugs
| # | Issue | File |
|---|---|---|
| B2 | apiGitDiff has no explicit return type, .then(r => r.diff) loses wrapper type |
frontend/src/api.ts:637 |
| B3 | Race condition in _dispatch_queued_runs — _active_runs counter can desync under concurrent threads; max(0, ...) is a band-aid |
backend/app/scheduler.py |
| B4 | handleRejectQuestion referenced in QuestionCard callback — verify this function is defined |
frontend/src/App.tsx:4700 |
| B5 | apiGitDiff exported from api.ts but no loading mechanism visible in App.tsx — gitDiffEntries state may never be populated |
frontend/src/api.ts / App.tsx |
| B6 | handleSaveGlobalDefaultModel referenced in Settings card — verify it exists |
frontend/src/App.tsx:4330 |
📝 Code Quality Concerns
| # | Issue | File(s) |
|---|---|---|
| Q1 | App.tsx remains enormous despite refactoring — should split into ChatView, TasksView, FilesView |
frontend/src/App.tsx |
| Q2 | All route modules use from .helpers import * with # ruff: noqa: F405 — impossible to trace symbol origins, should use explicit imports |
backend/app/routes/*.py (all 10) |
| Q3 | Duplicate global app, settings, ... boilerplate repeated across 10 route files — should extract to shared base or Flask extension pattern |
backend/app/routes/*.py |
| Q4 | ralph.sh hardcodes --model=claude-3-5-sonnet-20241022 — should be configurable via env var |
scripts/ralph/ralph.sh:107 |
| Q5 | No timeout on requests.post in _send_ntfy_notification — could block notification thread indefinitely |
backend/app/routes/helpers.py |
| Q6 | Cron parser brute-forces up to 527,040 iterations (minute-by-minute scan for a year) — should use smarter algorithm or add timeout | backend/app/scheduler.py:100 |
| Q7 | git diff endpoint loads full diff for every changed file into memory — no pagination or file-specific requests |
backend/app/git_routes.py:565-610 |
| Q8 | _build_untracked_patch reads entire file into memory — even with 1MB limit, slow for large repos |
backend/app/git_routes.py:52-76 |
| Q9 | No rate limiting on git operations — rapid successive push/pull/commit could corrupt git index | backend/app/git_routes.py |
| Q10 | Branch names only .strip() checked for emptiness — no validation against reserved names (HEAD, .) or length limits |
backend/app/git_routes.py:371,400 |
📋 Commit History Issues
- 4 merge commits should be squashed:
2f019cb— Merge PR Enhance project features, UI improvements, and documentation updates #73df7d98— Merge PR feat: Integrate Ralph Loop mechanism for automated app development #5f01ccb3— Merge remote-tracking branch1fe9eae— Merge PR feat: add Git control to project view #6
- 49 commits is excessive — many are iterative polish commits that should be squashed into parent feature commits:
1d4bbce— polish git sync labels706e8ec— tighten git request parsing28c5393— address review notesa0296b7— refine git tab workflow86e9a52— polish review follow-up details6ba5aaf— address review follow-ups1614650— refine review polish
- Duplicate commit messages:
69b03cdand74e7954both say "feat: Integrate Ralph Loop mechanism" — likely a cherry-pick or rebase artifact
✅ Positives
- Path traversal protection in
_resolve_project_relative_pathis well-implemented - GitPython used instead of shell subprocesses — eliminates shell injection risk
@auth_requireddecorators consistently applied on all new routes- Input validation with
_get_request_data()helper in git routes - Schema migration logic is careful with SQLite-specific handling
- TypeScript types are comprehensive for new types (
GitDiffEntry,QuestionRequest, etc.) - PWA support (
beforeinstallprompt, service worker) is well-structured - Notification debouncing (2.5s) prevents notification spam
- Ralph Loop integration with PRD tracking is clean and well-scoped
- No
print()debug statements found in backend code - Stack trace exposure was fixed in PRD routes (commit
33ebe71)
Recommendation
Do not merge as-is. Fix the following before merging:
- B1 (Critical): Fix
remote.pull()— this is a runtime crash - S2 (Medium): Replace
str(exc)with generic error messages in all route error responses, log details server-side - S3 (Medium): Validate ntfy
topic_url— ensure it's https or an allowed domain - B4/B6: Verify
handleRejectQuestionandhandleSaveGlobalDefaultModelare defined inApp.tsx - Commit history: Squash 49 commits down to ~8-10 logical feature commits, remove merge commits
Everything else can be addressed in follow-up PRs.
There was a problem hiding this comment.
Pull request overview
This PR expands the mobile OpenCode controller into a more full-featured control surface by adding Ralph (PRD-driven loop) tooling, Git controls, notifications, and richer streaming/timeline UI support across the backend + frontend.
Changes:
- Add Ralph loop support (CLI script + PRD scaffold + Tasks UI panel) and new OpenCode “skills” docs.
- Introduce Git integration (backend routes + frontend API/types + diff/timeline UI).
- Enhance runtime/session controls, notifications, question/approval handling, and project list virtualization.
Reviewed changes
Copilot reviewed 69 out of 71 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ui-spec.md | Removed old UI spec document |
| scripts/ralph/ralph.sh | Adds Ralph CLI iteration loop |
| scripts/ralph/prompt.md | Ralph agent prompt template |
| README.md | Refresh docs for new features/endpoints |
| prd.json.example | Example PRD JSON template |
| PDR.md | Removed old PRD document |
| frontend/src/utils/taskUtils.ts | Task status/date-time helpers |
| frontend/src/utils/streamUtils.ts | Stream parsing + classification helpers |
| frontend/src/utils/projectUtils.ts | Project path + session sorting helpers |
| frontend/src/utils/miscUtils.ts | Misc UI helpers (telemetry, install, etc.) |
| frontend/src/utils/messageUtils.ts | Timeline/activity summarization utilities |
| frontend/src/utils/formatting.ts | Formatting helpers for UI labels/times |
| frontend/src/utils/fileUtils.ts | File tree + language/path helpers |
| frontend/src/types/internal.ts | Internal UI/telemetry timeline types |
| frontend/src/types.ts | Expand shared API/types (git, PRD, notifications, etc.) |
| frontend/src/components/ui/QuestionCard.tsx | UI for multi-choice agent questions |
| frontend/src/components/ui/FixtureBanner.tsx | Dev fixture mode banner |
| frontend/src/components/ui/CommandPickerModal.tsx | Modal to pick server commands |
| frontend/src/components/toolbar/RuntimeControls.tsx | Model/agent/session runtime toolbar |
| frontend/src/components/toolbar/NotificationControls.tsx | Notification settings UI |
| frontend/src/components/toolbar/InstallControls.tsx | PWA install UI |
| frontend/src/components/tasks/RalphLoopPanel.tsx | Ralph PRD tracker/task creator UI |
| frontend/src/components/projects/VirtualizedProjectList.tsx | Virtualized project list rendering |
| frontend/src/components/projects/ProjectItem.tsx | Project list row UI |
| frontend/src/components/chat/TaskRunTimelineRow.tsx | Timeline rendering for task runs |
| frontend/src/components/chat/RichMessageText.tsx | Markdown rendering for messages |
| frontend/src/components/chat/MessageParts.tsx | Render message parts + collapse group |
| frontend/src/components/chat/MessagePartCard.tsx | UI for tool/step/file/patch parts |
| frontend/src/components/chat/MessageBubble.tsx | Message bubble + parts + TTS button |
| frontend/src/components/chat/EmptyState.tsx | Chat empty state component |
| frontend/src/components/chat/DiffPanel.tsx | Session/git diff panel UI |
| frontend/src/components/chat/ChatTransitionStrip.tsx | Status strip for transitions |
| frontend/src/components/chat/ChatStateCard.tsx | Generic chat state card |
| frontend/src/components/chat/AgentActivityCard.tsx | Collapsed activity grouping UI |
| frontend/src/components/auth/LoginView.tsx | Single-password login view |
| frontend/src/api.ts | Expand frontend API client (git, notifications, PRD, questions, summarize) |
| frontend/package.json | Add markdown dependencies |
| docs/task-feature-test-plan.md | Removed old manual test plan doc |
| backend/tests/test_git_diff.py | Tests for git diff endpoint (untracked/limits) |
| backend/requirements.txt | Adds GitPython dependency |
| backend/app/routes/tasks.py | Task + PRD routes |
| backend/app/routes/stt.py | STT/TTS routes |
| backend/app/routes/sessions.py | Session list/create/switch/delete/abort/summarize routes |
| backend/app/routes/scheduler_routes.py | Scheduler status endpoint |
| backend/app/routes/runtime.py | Runtime model/agent options endpoints |
| backend/app/routes/projects.py | Project list/create/sync + app settings/state |
| backend/app/routes/opencode.py | OpenCode health/commands endpoints |
| backend/app/routes/notifications.py | Notification settings + ntfy endpoints |
| backend/app/routes/files.py | File tree/list/read/download/archive endpoints |
| backend/app/routes/init.py | API blueprint + route registration |
| backend/app/opencode.py | Adds question + compact/summarize client methods |
| backend/app/config.py | Adds NOTIFICATION_NTFY_TOPIC_URL setting |
| backend/app/init.py | Registers git routes in app factory |
| AGENTS.md | Documents Ralph loop usage/files |
| .opencode/skills/ralph/SKILL.md | Ralph PRD converter skill docs |
| .opencode/skills/prd/SKILL.md | PRD generator skill docs |
| .opencode/skills/frontend-design/SKILL.md | Frontend design skill docs update |
| .opencode/skills/dev-browser/SKILL.md | Dev-browser (Chrome MCP) skill docs |
| .opencode/skills/compound-engineering/SKILL.md | Compound engineering workflow docs |
| .env.example | Adds NOTIFICATION_NTFY_TOPIC_URL env var |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function findPermissionId(value: unknown): string | null { | ||
| if (typeof value === "string") { | ||
| return null; | ||
| } | ||
|
|
||
| if (Array.isArray(value)) { | ||
| for (const item of value) { | ||
| const found = findPermissionId(item); |
| function DiffLine({ line }: { line: string }) { | ||
| const cls = line.startsWith("+") | ||
| ? "diff-line-add" | ||
| : line.startsWith("-") | ||
| ? "diff-line-del" | ||
| : line.startsWith("@@") | ||
| ? "diff-line-hunk" | ||
| : ""; |
| onSelect, | ||
| emptyLabel, | ||
| searchQuery, | ||
| totalLabel, | ||
| hasMore, |
| MAX_ITERATIONS=${1:-10} | ||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| PRD_FILE="$SCRIPT_DIR/prd.json" |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (4)
frontend/src/components/chat/MessagePartCard.tsx-143-169 (1)
143-169:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix zero-cost handling in step-finish placeholder logic.
Line 167 uses
!cost, socost = 0is treated as absent and can render a contradictory fallback message.Proposed patch
function StepCard({ part }: { part: Record<string, unknown> }) { @@ const tokens = part.tokens as Record<string, unknown> | undefined; const cost = typeof part.cost === "number" ? part.cost : null; + const hasTokens = !!tokens && Object.keys(tokens).length > 0; @@ - {tokens && Object.keys(tokens).length > 0 && ( + {hasTokens && ( <div className="step-info"> @@ - {partType === "step-finish" && !reason && !cost && !tokens && ( + {partType === "step-finish" && !reason && cost === null && !hasTokens && ( <p className="step-placeholder">Step finished</p> )}🤖 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/chat/MessagePartCard.tsx` around lines 143 - 169, The placeholder for the "step-finish" branch treats cost=0 as falsy because it uses "!cost"; change the absence checks to explicitly test for null/emptiness: replace the condition `partType === "step-finish" && !reason && !cost && !tokens` with checks that reason is falsy, cost is null (e.g., cost === null), and tokens is null/empty (e.g., !tokens || Object.keys(tokens).length === 0) so a zero cost still displays correctly; update the conditional around the step-placeholder rendering to use these explicit checks (refer to variables partType, reason, cost, tokens and the "step-finish" placeholder).frontend/src/components/auth/LoginView.tsx-24-30 (1)
24-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an explicit label for the password field.
Line 24 currently relies on placeholder text only; add a real label for accessibility and better form usability.
Suggested fix
<form className="auth-card" onSubmit={handleSubmit}> <h1>OpenCode Controller</h1> <p>Single-password access</p> + <label htmlFor="login-password">Password</label> <input + id="login-password" type="password" value={password} onChange={(event) => setPassword(event.target.value)} placeholder="Enter password" autoComplete="current-password" />🤖 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/auth/LoginView.tsx` around lines 24 - 30, The password input in LoginView.tsx lacks an explicit label; add a real <label> associated with the input (e.g., label text "Password" and htmlFor="password") and give the input a matching id (e.g., id="password") so setPassword and the existing onChange remain unchanged; if the design requires hiding the visual label, use a visually-hidden CSS class but keep the semantic label for accessibility.frontend/src/utils/messageUtils.ts-368-390 (1)
368-390:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winParse boolean-like payload values explicitly.
Line 368 currently uses
Boolean(value), so"false"/"0"are interpreted astrue, which can flip task run flags in the UI.Suggested fix
- const asBoolean = (value: unknown) => Boolean(value); + const asBoolean = (value: unknown) => { + if (typeof value === "boolean") { + return value; + } + if (typeof value === "number") { + return value !== 0; + } + if (typeof value === "string") { + const normalized = value.trim().toLowerCase(); + if (normalized === "true" || normalized === "1") { + return true; + } + if (normalized === "false" || normalized === "0" || normalized === "") { + return false; + } + } + return false; + };🤖 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/utils/messageUtils.ts` around lines 368 - 390, Replace the current asBoolean implementation (used by heartbeatLoaded and goalAttempted) so it returns true/false/null instead of using Boolean(value); specifically, have asBoolean(value: unknown) return true for boolean true, number 1, or strings "true"/"1" (case-insensitive), return false for boolean false, number 0, or strings "false"/"0", and return null for any other values; keep the rest of the mapping (taskRunId, runNumber, goalMet) unchanged and ensure callers like heartbeatLoaded and goalAttempted accept the new boolean | null result.frontend/src/utils/projectUtils.ts-7-8 (1)
7-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid returning
//when the source path is filesystem root.At Line 8,
sourcePath === "/"becomes"//". That creates malformed root suggestions and can cascade into broken joined paths.Suggested fix
const slashIndex = sourcePath.lastIndexOf("/"); if (slashIndex <= 0) { - return sourcePath ? `${sourcePath}/` : ""; + if (!sourcePath) return ""; + return sourcePath === "/" ? "/" : `${sourcePath}/`; }🤖 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/utils/projectUtils.ts` around lines 7 - 8, The current conditional returns `${sourcePath}/` when slashIndex <= 0, which yields "//" for sourcePath === "/"; update the branch that checks slashIndex and sourcePath so that when sourcePath === "/" you return "/" (not "//"), otherwise return `${sourcePath}/`; locate the conditional that uses slashIndex and sourcePath in frontend/src/utils/projectUtils.ts and replace the single return with a check like if (sourcePath === "/") return "/"; else return `${sourcePath}/`.
🧹 Nitpick comments (5)
scripts/ralph/ralph.sh (3)
118-118: ⚡ Quick winQuote the variable to prevent word splitting.
The unquoted
$MAX_ITERATIONSin theseqcommand could cause unexpected behavior if the variable contains whitespace or special characters.🛡️ Proposed fix
-for i in $(seq 1 $MAX_ITERATIONS); do +for i in $(seq 1 "$MAX_ITERATIONS"); do🤖 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 `@scripts/ralph/ralph.sh` at line 118, The loop uses an unquoted variable in the seq call which can cause word-splitting; update the for-loop to quote MAX_ITERATIONS in the command substitution so seq receives it as a single argument (look for the for loop that uses $(seq 1 $MAX_ITERATIONS) and change the invocation to use a quoted variable).
55-64: ⚡ Quick winPrevent archive folder name collisions.
If the same branch is archived multiple times on the same date, the second archive will overwrite the first. Adding a timestamp would prevent data loss.
📁 Proposed fix
- DATE=$(date +%Y-%m-%d) + TIMESTAMP=$(date +%Y-%m-%d-%H%M%S) # Strip "ralph/" prefix from branch name for folder FOLDER_NAME=$(echo "$LAST_BRANCH" | sed 's|^ralph/||') - ARCHIVE_FOLDER="$ARCHIVE_DIR/$DATE-$FOLDER_NAME" + ARCHIVE_FOLDER="$ARCHIVE_DIR/$TIMESTAMP-$FOLDER_NAME" echo "Archiving previous run: $LAST_BRANCH"🤖 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 `@scripts/ralph/ralph.sh` around lines 55 - 64, The archive path can collide when the same branch is archived on the same DATE; modify the ARCHIVE_FOLDER computation to include a time component (e.g., append a timestamp or seconds since epoch) so each archive is unique. Locate the variables DATE, FOLDER_NAME and ARCHIVE_FOLDER and change DATE or ARCHIVE_FOLDER to include a timestamp (from date +%s or date +%Y-%m-%d_%H%M%S) before mkdir -p and the cp commands so the new folder name is unique while keeping the existing echo messages and file-copy logic intact.
92-92: ⚡ Quick winMake model name configurable via environment variable.
The model is currently hardcoded to
claude-3-5-sonnet-20241022, limiting flexibility for users who want to use different models or test with newer versions.♻️ Proposed fix
-MODEL_FLAGS=(--model=claude-3-5-sonnet-20241022) +RALPH_MODEL="${RALPH_MODEL:-claude-3-5-sonnet-20241022}" +MODEL_FLAGS=(--model="$RALPH_MODEL")This allows users to override with
RALPH_MODEL=other-model ./ralph.shwhile preserving the current default.🤖 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 `@scripts/ralph/ralph.sh` at line 92, Replace the hardcoded model string in MODEL_FLAGS with a shell parameter expansion that reads RALPH_MODEL and falls back to the current default; update the assignment to set MODEL_FLAGS using the value of RALPH_MODEL if present (e.g., use ${RALPH_MODEL:-claude-3-5-sonnet-20241022}) so callers can override by exporting RALPH_MODEL, while preserving the existing default for MODEL_FLAGS.scripts/ralph/prompt.md (1)
27-36: 💤 Low valueAdd language identifiers to fenced code blocks.
Markdown linters prefer explicit language tags on code blocks for proper syntax highlighting and rendering.
📝 Proposed fix
At line 27, specify the language:
-``` +```markdown ## [Date/Time] - [Story ID]At line 44, specify the language:
-``` +```markdown ## Codebase PatternsAlso applies to: 44-49
🤖 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 `@scripts/ralph/prompt.md` around lines 27 - 36, The Markdown file uses fenced code blocks without language identifiers which fails linters; update the two fenced blocks shown (the one containing the "## [Date/Time] - [Story ID]" header and the one containing "## Codebase Patterns") to include explicit language tags (e.g., add ```markdown after the opening backticks) so linters and renderers get proper syntax highlighting; locate those blocks in scripts/ralph/prompt.md and add the language identifier to each opening fence.frontend/src/components/chat/DiffPanel.tsx (1)
62-65: ⚡ Quick winDefault to the first non-empty diff source.
Starting on
"session"can show an empty state even when git diffs exist. Prefer selecting the first non-empty tab and syncing when counts change.Proposed patch
-import React, { useState } from "react"; +import React, { useEffect, useState } from "react"; @@ - const [tab, setTab] = useState<"session" | "git">("session"); + const [tab, setTab] = useState<"session" | "git">( + sessionDiff.length > 0 ? "session" : "git" + ); const sessionCount = sessionDiff.length; const gitCount = gitDiff.length; if (sessionCount === 0 && gitCount === 0) return null; + + useEffect(() => { + if (tab === "session" && sessionCount === 0 && gitCount > 0) setTab("git"); + if (tab === "git" && gitCount === 0 && sessionCount > 0) setTab("session"); + }, [tab, sessionCount, gitCount]);🤖 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/chat/DiffPanel.tsx` around lines 62 - 65, The component currently initializes tab state to "session" and can render an empty panel when sessionDiff is empty even though gitDiff has items; modify the initialization and add an effect that watches sessionDiff and gitDiff (or sessionCount/gitCount) to setTab to the first non-empty source ("session" if sessionCount>0 else "git" if gitCount>0) and keep it in sync when counts change; update the useState default to choose based on initial sessionDiff/gitDiff and add a useEffect that calls setTab appropriately when sessionCount or gitCount change so the UI always shows the first available non-empty tab.
🤖 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/app/git_routes.py`:
- Around line 279-283: The endpoints in git_routes.py currently return raw
exceptions (str(e)) to clients (e.g., the Repo.init(project.path) block and the
other listed try/excepts); replace these by logging the exception server-side
and returning a generic error response. Concretely, inside each except block
(for example in the handler that calls Repo.init, and the other handlers
referenced at lines ~314, 332, 357, etc.), call the Flask logger or module
logger with full exception info (e.g., app.logger.exception(...) or
logger.exception(...)) and then return jsonify({"error": "Internal server
error"}), 500 instead of jsonify({"error": str(e)}), 500; do this consistently
across all handlers in git_routes.py that currently expose str(e).
- Around line 636-647: Validate the incoming url string before calling
_get_named_remote / existing_remote.set_url / repo.create_remote: parse the url
(e.g., with urllib.parse), reject unsafe or local schemes like file: and any
missing/invalid components, and only accept a whitelisted set of schemes (e.g.,
http, https, ssh/git formats) and ensure netloc or valid host/path is present;
if validation fails return a 400 JSON error and do not call
existing_remote.set_url or repo.create_remote.
In `@backend/app/routes/files.py`:
- Around line 187-194: The temp ZIP is being unlinked too early via
`@after_this_request`; instead register deletion on response close using
response.call_on_close so the file is removed only after send_file(archive_path)
finishes; locate the cleanup handler named cleanup_archive (and the similar
cleanup block around the other occurrence using archive_path) and replace the
after_this_request decorator logic with a response.call_on_close(lambda:
archive_path.unlink(missing_ok=True) wrapped with try/except OSError) to ensure
safe deletion after the response is fully sent.
In `@backend/app/routes/helpers.py`:
- Around line 483-502: Validate ntfyTopicUrl before storing or posting: update
_set_notification_settings to parse and validate the provided ntfy_topic_url and
update _send_ntfy_notification to re-validate topic_url before making the
request. Ensure validation enforces scheme is http or https, hostname is not
localhost/127.0.0.1 and does not resolve to loopback, link-local, or
private-network IP ranges (e.g., 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, ::1,
fe80::/10, fc00::/7), and reject or raise/abort on invalid URLs; use
urllib.parse to parse the URL and socket.getaddrinfo (or equivalent) to resolve
and check IPs, and only call requests.post from _send_ntfy_notification after
the validation passes so untrusted hosts cannot be contacted.
In `@backend/app/routes/messages.py`:
- Around line 39-61: The handler in messages.py (the get_project_messages route)
is currently catching all exceptions and echoing raw exception text to clients;
instead, validate inputs first (parse limit with int(...) inside a try/except
ValueError and return a 400 with a stable message like "invalid 'limit'
parameter"), keep the session validation branch that returns 400 for mismatched
sessions (_session_matches_project), and for upstream calls
(opencode_client.get_session, _ensure_project_session,
opencode_client.list_messages, and TimelineEvent.query) catch only
upstream/network/database errors, log the full exception server-side (use
logger.error(..., exc_info=True)) and return a generic 502 message like "failed
to load messages" without including exc text or internal details; make analogous
changes for the other handlers referenced so input parsing errors return 400 and
upstream failures return generic 502 with server-side logging.
In `@backend/app/routes/opencode.py`:
- Around line 33-40: The except blocks that currently return jsonify({"healthy":
False, "error": str(exc), "upstreamBaseUrl": settings.opencode_base_url}) should
be changed to log the full exception server-side (e.g., using
current_app.logger.exception or app.logger.error with the exc) and instead
return a generic JSON payload such as {"healthy": False, "error": "upstream
service error"} without including str(exc) or settings.opencode_base_url; update
both the except handling around the health check and the similar handler at the
other occurrence (the blocks that capture Exception as exc and call jsonify) to
perform logging then return the sanitized response.
In `@backend/app/routes/projects.py`:
- Around line 72-73: Replace responses that embed raw exception text (the except
Exception as exc blocks that call jsonify with f"Failed ...: {exc}" at the shown
locations) so they log the full exception locally and return a stable, generic
error message and status to the client (e.g., "Failed to sync projects" with 502
or "Failed to create project" with 4xx/5xx as appropriate). Use the existing
application logger (e.g., current_app.logger or app.logger) to log the exception
and stack trace, and remove the exception string interpolation from the JSON
response; keep the same HTTP status codes currently returned.
In `@backend/app/routes/runtime.py`:
- Around line 68-72: The endpoint currently returns raw exception text from
calls to opencode_client.get_config() and
opencode_client.list_config_providers() (and similarly in the other 502 path
around lines 155-159); change these handlers to log the exception server-side
(use the existing logger) and return a generic error JSON like {"error": "Failed
to load runtime options"} with the 502 status, removing f"{exc}" from the
response body; keep the try/except blocks around opencode_client.get_config() /
list_config_providers() and the other failing call, but replace the
client-facing message with a generic one and call logger.exception or
logger.error to record exc.
- Around line 31-39: The /health route (api_bp.get("/health") -> app_health)
currently always returns healthy: true; update app_health to perform a real
liveness check against the OpenCode proxy (use the configured OpenCode URL from
settings, e.g. settings.opencode_proxy_url or equivalent) by issuing a
short-timeout HTTP request and treating non-2xx/non-reachable responses as
unhealthy, catching exceptions and returning healthy: False (and appropriate
non-200 status) when the proxy is down; ensure you log the failure and keep
returning service and env fields (settings.app_env) as before.
In `@backend/app/routes/sessions.py`:
- Around line 37-41: The except blocks in sessions route handlers (e.g., around
calls to _clear_project_session_cache and _list_project_sessions) currently
interpolate the exception object into the JSON error response; replace those
responses so they return a generic error message (e.g., {"error": "Failed to
load sessions"}) without including exc, and keep the same 502 status; apply the
same change to all similar handlers in this file (the other except blocks that
wrap calls like _get_project_session, _create_session, _delete_session, etc.) so
no upstream/internal exception details are echoed to clients while preserving
logging of exc internally if needed.
- Around line 343-352: The fallback incorrectly parses the default mapping by
taking default_map's first value and treating it as "provider/model"; instead,
use the default_map's first key and value as provider_id and model_id
respectively. In the block around provider_data =
opencode_client.list_config_providers() where default_map is derived, when
provider_id or model_id are missing, set provider_id to the first key from
default_map and model_id to default_map[first_key] (ensuring the value is a
string) rather than partitioning a single string; update handling of
default_model and the variables provider_id and model_id accordingly.
- Around line 272-292: The handler currently treats any exception from
_resolve_project_session and friends as a backend error; catch ValueError raised
by _resolve_project_session (and any input-validation related errors)
separately, return a 400 JSON response with the validation message and do not
set project.session_status to "error" (leave status/last_activity_at unchanged
or set to the appropriate client-error state), while keeping the existing broad
except for unexpected exceptions that still set project.session_status="error"
and return 502; reference _resolve_project_session, abort_session,
project.session_status, and the existing except block when implementing this
change.
In `@backend/app/routes/stt.py`:
- Around line 64-72: The code currently calls upload.read() (the local variable
upload from request.files.get("audio")) which can load an unbounded file into
memory; before calling upload.read() in the STT route, check
request.content_length (or a configured max like
current_app.config['MAX_UPLOAD_SIZE'] / MAX_CONTENT_LENGTH) and return 413 if it
exceeds the limit, or alternatively stream the file to disk using
upload.save(temp_path) or read in bounded chunks to enforce the size cap; update
the conditional around upload.read() (and the error responses for
empty/oversized payloads) to use this size check so large uploads are rejected
or streamed before they are fully read into memory.
- Around line 107-128: Replace the handlers that return the raw exception text
(currently returning jsonify({"error": f"STT transcription failed: {exc}"}) in
the try/except blocks around the requests.post/response handling and the similar
block at lines 165-176) so they return a generic error payload (e.g.,
jsonify({"error": "STT transcription failed"}), 502) while logging the full
exception server-side using the module/app logger (call logger.exception(...) or
app.logger.exception(...) with context like "STT transcription failed") so
provider/backend details are recorded in logs but not leaked to clients.
In `@backend/app/routes/tasks.py`:
- Around line 110-120: The handler currently treats a provided but-not-found id
as "no id" and proceeds to create a new ScheduledTask; change the flow so that
after parsing task_id and resolving task via ScheduledTask.query.filter_by(...),
if a non-empty task_id was provided and task is None you return an HTTP 404 (or
400) JSON error instead of falling through to the creation branch. Locate the
task_id variable and the lookup using ScheduledTask.query.filter_by(...) and
prevent calling db.session.add(task) when the client explicitly supplied an id
that wasn't found; keep the existing TypeError/ValueError handling for malformed
ids.
In `@frontend/src/components/toolbar/RuntimeControls.tsx`:
- Around line 120-135: The dropdown currently omits the "+ New session" option
when sortedSessions.length === 0, preventing onSessionCreate from being
triggered; update the JSX in RuntimeControls (the component rendering
sortedSessions.map and the option with value="__new__") to always include an
<option value="__new__">+ New session</option> regardless of whether
sortedSessions is empty—e.g., keep the placeholder ("Loading..." or "No
session") when there are no sessions but render the "__new__" option outside or
in both branches so the create action remains reachable.
In `@frontend/src/GitView.tsx`:
- Around line 265-335: Add a request-id guard so async responses from
loadGitData and loadHistory don't overwrite state for a newer projectId: create
a mutable ref (e.g., requestsRef) that you increment/set to a unique token
inside useEffect before calling loadGitData, pass or capture the token inside
loadGitData and loadHistory, and before every setState call (setStatus,
setBranches, setRemoteUrl, setHistory, setHistoryHasMore, setError, setLoading,
setRefreshing, etc.) check that the current token still matches the ref; if it
does not, return early and skip state updates; also update the token on
projectId change and optionally clear/abort any outstanding work on unmount.
In `@frontend/src/utils/streamUtils.ts`:
- Around line 83-86: The current check in streamUtils.ts that treats any string
response as resolved (const response = record.response ?? record.decision ??
record.action; if (typeof response === "string") return true;) is too broad;
change it to only treat explicit resolved decision values as resolved (e.g.,
check response === "allow" || response === "deny" or map to a small whitelist of
resolved string constants), or implement a helper like
isResolvedDecision(response) that returns true only for those known resolved
values; update the conditional that references response (and any callers that
rely on it) so pending/unknown string decisions are not treated as resolved.
---
Minor comments:
In `@frontend/src/components/auth/LoginView.tsx`:
- Around line 24-30: The password input in LoginView.tsx lacks an explicit
label; add a real <label> associated with the input (e.g., label text "Password"
and htmlFor="password") and give the input a matching id (e.g., id="password")
so setPassword and the existing onChange remain unchanged; if the design
requires hiding the visual label, use a visually-hidden CSS class but keep the
semantic label for accessibility.
In `@frontend/src/components/chat/MessagePartCard.tsx`:
- Around line 143-169: The placeholder for the "step-finish" branch treats
cost=0 as falsy because it uses "!cost"; change the absence checks to explicitly
test for null/emptiness: replace the condition `partType === "step-finish" &&
!reason && !cost && !tokens` with checks that reason is falsy, cost is null
(e.g., cost === null), and tokens is null/empty (e.g., !tokens ||
Object.keys(tokens).length === 0) so a zero cost still displays correctly;
update the conditional around the step-placeholder rendering to use these
explicit checks (refer to variables partType, reason, cost, tokens and the
"step-finish" placeholder).
In `@frontend/src/utils/messageUtils.ts`:
- Around line 368-390: Replace the current asBoolean implementation (used by
heartbeatLoaded and goalAttempted) so it returns true/false/null instead of
using Boolean(value); specifically, have asBoolean(value: unknown) return true
for boolean true, number 1, or strings "true"/"1" (case-insensitive), return
false for boolean false, number 0, or strings "false"/"0", and return null for
any other values; keep the rest of the mapping (taskRunId, runNumber, goalMet)
unchanged and ensure callers like heartbeatLoaded and goalAttempted accept the
new boolean | null result.
In `@frontend/src/utils/projectUtils.ts`:
- Around line 7-8: The current conditional returns `${sourcePath}/` when
slashIndex <= 0, which yields "//" for sourcePath === "/"; update the branch
that checks slashIndex and sourcePath so that when sourcePath === "/" you return
"/" (not "//"), otherwise return `${sourcePath}/`; locate the conditional that
uses slashIndex and sourcePath in frontend/src/utils/projectUtils.ts and replace
the single return with a check like if (sourcePath === "/") return "/"; else
return `${sourcePath}/`.
---
Nitpick comments:
In `@frontend/src/components/chat/DiffPanel.tsx`:
- Around line 62-65: The component currently initializes tab state to "session"
and can render an empty panel when sessionDiff is empty even though gitDiff has
items; modify the initialization and add an effect that watches sessionDiff and
gitDiff (or sessionCount/gitCount) to setTab to the first non-empty source
("session" if sessionCount>0 else "git" if gitCount>0) and keep it in sync when
counts change; update the useState default to choose based on initial
sessionDiff/gitDiff and add a useEffect that calls setTab appropriately when
sessionCount or gitCount change so the UI always shows the first available
non-empty tab.
In `@scripts/ralph/prompt.md`:
- Around line 27-36: The Markdown file uses fenced code blocks without language
identifiers which fails linters; update the two fenced blocks shown (the one
containing the "## [Date/Time] - [Story ID]" header and the one containing "##
Codebase Patterns") to include explicit language tags (e.g., add ```markdown
after the opening backticks) so linters and renderers get proper syntax
highlighting; locate those blocks in scripts/ralph/prompt.md and add the
language identifier to each opening fence.
In `@scripts/ralph/ralph.sh`:
- Line 118: The loop uses an unquoted variable in the seq call which can cause
word-splitting; update the for-loop to quote MAX_ITERATIONS in the command
substitution so seq receives it as a single argument (look for the for loop that
uses $(seq 1 $MAX_ITERATIONS) and change the invocation to use a quoted
variable).
- Around line 55-64: The archive path can collide when the same branch is
archived on the same DATE; modify the ARCHIVE_FOLDER computation to include a
time component (e.g., append a timestamp or seconds since epoch) so each archive
is unique. Locate the variables DATE, FOLDER_NAME and ARCHIVE_FOLDER and change
DATE or ARCHIVE_FOLDER to include a timestamp (from date +%s or date
+%Y-%m-%d_%H%M%S) before mkdir -p and the cp commands so the new folder name is
unique while keeping the existing echo messages and file-copy logic intact.
- Line 92: Replace the hardcoded model string in MODEL_FLAGS with a shell
parameter expansion that reads RALPH_MODEL and falls back to the current
default; update the assignment to set MODEL_FLAGS using the value of RALPH_MODEL
if present (e.g., use ${RALPH_MODEL:-claude-3-5-sonnet-20241022}) so callers can
override by exporting RALPH_MODEL, while preserving the existing default for
MODEL_FLAGS.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 108fdb16-ac6a-414e-8611-9787d9a7811b
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (70)
.env.example.opencode/skills/compound-engineering/SKILL.md.opencode/skills/dev-browser/SKILL.md.opencode/skills/frontend-design/SKILL.md.opencode/skills/prd/SKILL.md.opencode/skills/ralph/SKILL.mdAGENTS.mdPDR.mdREADME.mdbackend/app/__init__.pybackend/app/config.pybackend/app/git_routes.pybackend/app/opencode.pybackend/app/routes.pybackend/app/routes/__init__.pybackend/app/routes/files.pybackend/app/routes/helpers.pybackend/app/routes/messages.pybackend/app/routes/notifications.pybackend/app/routes/opencode.pybackend/app/routes/projects.pybackend/app/routes/runtime.pybackend/app/routes/scheduler_routes.pybackend/app/routes/sessions.pybackend/app/routes/stt.pybackend/app/routes/tasks.pybackend/requirements.txtbackend/tests/test_git_diff.pydocs/task-feature-test-plan.mdfrontend/package.jsonfrontend/src/App.tsxfrontend/src/GitView.tsxfrontend/src/api.tsfrontend/src/components/auth/LoginView.tsxfrontend/src/components/chat/AgentActivityCard.tsxfrontend/src/components/chat/ChatStateCard.tsxfrontend/src/components/chat/ChatTransitionStrip.tsxfrontend/src/components/chat/DiffPanel.tsxfrontend/src/components/chat/EmptyState.tsxfrontend/src/components/chat/MessageBubble.tsxfrontend/src/components/chat/MessagePartCard.tsxfrontend/src/components/chat/MessageParts.tsxfrontend/src/components/chat/RichMessageText.tsxfrontend/src/components/chat/TaskRunTimelineRow.tsxfrontend/src/components/projects/ProjectFilesPanel.tsxfrontend/src/components/projects/ProjectItem.tsxfrontend/src/components/projects/VirtualizedProjectList.tsxfrontend/src/components/tasks/RalphLoopPanel.tsxfrontend/src/components/tasks/ScheduledTaskPanel.tsxfrontend/src/components/toolbar/InstallControls.tsxfrontend/src/components/toolbar/NotificationControls.tsxfrontend/src/components/toolbar/RuntimeControls.tsxfrontend/src/components/ui/CommandPickerModal.tsxfrontend/src/components/ui/FixtureBanner.tsxfrontend/src/components/ui/QuestionCard.tsxfrontend/src/styles.cssfrontend/src/types.tsfrontend/src/types/internal.tsfrontend/src/utils/fileUtils.tsfrontend/src/utils/formatting.tsfrontend/src/utils/messageUtils.tsfrontend/src/utils/miscUtils.tsfrontend/src/utils/projectUtils.tsfrontend/src/utils/streamUtils.tsfrontend/src/utils/taskUtils.tsprd.json.examplescripts/ralph/prompt.mdscripts/ralph/ralph.shtasks.mdui-spec.md
💤 Files with no reviewable changes (4)
- PDR.md
- tasks.md
- docs/task-feature-test-plan.md
- ui-spec.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Frontend uses Vite + React 18 + TypeScript for type-safe component development
Files:
frontend/src/components/ui/FixtureBanner.tsxfrontend/src/components/chat/ChatStateCard.tsxfrontend/src/components/auth/LoginView.tsxfrontend/src/components/chat/TaskRunTimelineRow.tsxfrontend/src/components/toolbar/InstallControls.tsxfrontend/src/components/chat/DiffPanel.tsxfrontend/src/components/chat/ChatTransitionStrip.tsxfrontend/src/components/ui/CommandPickerModal.tsxfrontend/src/components/chat/EmptyState.tsxfrontend/src/components/chat/MessagePartCard.tsxfrontend/src/components/projects/VirtualizedProjectList.tsxfrontend/src/components/tasks/RalphLoopPanel.tsxfrontend/src/components/toolbar/NotificationControls.tsxfrontend/src/components/chat/AgentActivityCard.tsxfrontend/src/components/projects/ProjectItem.tsxfrontend/src/components/chat/RichMessageText.tsxfrontend/src/components/chat/MessageParts.tsxfrontend/src/components/toolbar/RuntimeControls.tsxfrontend/src/utils/projectUtils.tsfrontend/src/utils/fileUtils.tsfrontend/src/types/internal.tsfrontend/src/components/ui/QuestionCard.tsxfrontend/src/components/chat/MessageBubble.tsxfrontend/src/components/projects/ProjectFilesPanel.tsxfrontend/src/GitView.tsxfrontend/src/utils/taskUtils.tsfrontend/src/components/tasks/ScheduledTaskPanel.tsxfrontend/src/types.tsfrontend/src/utils/streamUtils.tsfrontend/src/utils/formatting.tsfrontend/src/utils/miscUtils.tsfrontend/src/utils/messageUtils.tsfrontend/src/api.ts
.env*
📄 CodeRabbit inference engine (AGENTS.md)
Set up environment configuration by copying
.env.exampleto.env
Files:
.env.example
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend uses Flask + Flask-SQLAlchemy with SQLite as default database (can be overridden with PostgreSQL via DATABASE_URL)
Files:
backend/app/__init__.pybackend/tests/test_git_diff.pybackend/app/opencode.pybackend/app/config.pybackend/app/routes/opencode.pybackend/app/routes/projects.pybackend/app/routes/stt.pybackend/app/routes.pybackend/app/routes/notifications.pybackend/app/routes/runtime.pybackend/app/routes/scheduler_routes.pybackend/app/routes/__init__.pybackend/app/routes/files.pybackend/app/routes/sessions.pybackend/app/routes/messages.pybackend/app/routes/helpers.pybackend/app/routes/tasks.pybackend/app/git_routes.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Rishabh-Bajpai/mobile-opencode-control
Timestamp: 2026-05-21T00:33:27.030Z
Learning: Start services in required order: OpenCode server first (needs available port), then Backend (Flask on localhost:8080), then Frontend (Vite on localhost:5173)
Learnt from: CR
Repo: Rishabh-Bajpai/mobile-opencode-control
Timestamp: 2026-05-21T00:33:27.030Z
Learning: Backend `/api/health` endpoint checks OpenCode proxy availability - if OpenCode isn't running, the health check will fail
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 14-14: [UnorderedKey] The NOTIFICATION_NTFY_TOPIC_URL key should go before the OPENCODE_CORS_ORIGINS key
(UnorderedKey)
🪛 LanguageTool
.opencode/skills/frontend-design/SKILL.md
[style] ~39-~39: “thinking outside the box” can be a clichéd phrase in professional communication. Consider an alternative expression to make your writing more engaging and original.
Context: ...ck, show what can truly be created when thinking outside the box and committing fully to a distinctive v...
(THINK_OUTSIDE_THE_BOX)
.opencode/skills/compound-engineering/SKILL.md
[style] ~22-~22: The wording of this phrase can be improved.
Context: ...*: Feed results back into the system to make the next loop better 80% of compound engineering is in plan...
(MAKE_STYLE_BETTER)
[style] ~189-~189: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...hat pattern did I discover or create? - What decision was made and why? Document th...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.22.1)
scripts/ralph/prompt.md
[warning] 27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.opencode/skills/compound-engineering/SKILL.md
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 196-196: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 205-205: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 215-215: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 225-225: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.opencode/skills/prd/SKILL.md
[warning] 36-36: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.opencode/skills/ralph/SKILL.md
[warning] 102-102: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 107-107: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 112-112: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 OpenGrep (1.21.0)
frontend/src/utils/messageUtils.ts
[ERROR] 284-284: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🪛 Ruff (0.15.13)
backend/app/routes/opencode.py
[warning] 33-33: Do not catch blind exception: Exception
(BLE001)
[warning] 65-65: Do not catch blind exception: Exception
(BLE001)
backend/app/routes/projects.py
[warning] 72-72: Do not catch blind exception: Exception
(BLE001)
[warning] 204-204: Do not catch blind exception: Exception
(BLE001)
backend/app/routes/stt.py
[warning] 127-127: Do not catch blind exception: Exception
(BLE001)
[warning] 175-175: Do not catch blind exception: Exception
(BLE001)
backend/app/routes.py
[warning] 25-25: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
backend/app/routes/notifications.py
[warning] 69-69: Do not catch blind exception: Exception
(BLE001)
[warning] 92-92: Do not catch blind exception: Exception
(BLE001)
backend/app/routes/runtime.py
[warning] 71-71: Do not catch blind exception: Exception
(BLE001)
[warning] 158-158: Do not catch blind exception: Exception
(BLE001)
backend/app/routes/__init__.py
[warning] 41-41: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
backend/app/routes/files.py
[warning] 47-47: Do not catch blind exception: Exception
(BLE001)
[warning] 78-78: Do not catch blind exception: Exception
(BLE001)
backend/app/routes/sessions.py
[warning] 40-40: Do not catch blind exception: Exception
(BLE001)
[warning] 89-89: Do not catch blind exception: Exception
(BLE001)
[warning] 131-131: Do not catch blind exception: Exception
(BLE001)
[warning] 222-222: Do not catch blind exception: Exception
(BLE001)
[warning] 259-259: Do not catch blind exception: Exception
(BLE001)
[warning] 289-289: Do not catch blind exception: Exception
(BLE001)
[warning] 319-319: Do not catch blind exception: Exception
(BLE001)
[warning] 348-348: Prefer next(iter(default_map.keys())) over single element slice
Replace with next(iter(default_map.keys()))
(RUF015)
[warning] 365-365: Do not catch blind exception: Exception
(BLE001)
backend/app/routes/messages.py
[warning] 60-60: Do not catch blind exception: Exception
(BLE001)
[warning] 114-114: Do not catch blind exception: Exception
(BLE001)
[warning] 161-161: Prefer next(iter(default_map.keys())) over single element slice
Replace with next(iter(default_map.keys()))
(RUF015)
[warning] 223-223: Do not catch blind exception: Exception
(BLE001)
[warning] 244-244: Do not catch blind exception: Exception
(BLE001)
[warning] 264-264: Do not catch blind exception: Exception
(BLE001)
[warning] 288-288: Do not catch blind exception: Exception
(BLE001)
[warning] 298-298: Do not catch blind exception: Exception
(BLE001)
[warning] 313-313: Do not catch blind exception: Exception
(BLE001)
[warning] 359-359: Do not catch blind exception: Exception
(BLE001)
[warning] 405-405: Do not catch blind exception: Exception
(BLE001)
[warning] 476-476: Do not catch blind exception: Exception
(BLE001)
[warning] 517-517: Do not catch blind exception: Exception
(BLE001)
[warning] 544-544: Do not catch blind exception: Exception
(BLE001)
backend/app/routes/helpers.py
[warning] 12-66: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
[warning] 198-198: Do not catch blind exception: Exception
(BLE001)
[warning] 396-396: Do not catch blind exception: Exception
(BLE001)
[warning] 440-440: Do not catch blind exception: Exception
(BLE001)
[warning] 943-943: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/routes/tasks.py
[warning] 189-189: Do not catch blind exception: Exception
(BLE001)
[warning] 278-278: Do not catch blind exception: Exception
(BLE001)
backend/app/git_routes.py
[warning] 29-29: Do not catch blind exception: Exception
(BLE001)
[warning] 92-92: Do not catch blind exception: Exception
(BLE001)
[warning] 100-100: Do not catch blind exception: Exception
(BLE001)
[warning] 190-190: Do not catch blind exception: Exception
(BLE001)
[warning] 200-200: Do not catch blind exception: Exception
(BLE001)
[warning] 205-205: Do not catch blind exception: Exception
(BLE001)
[warning] 224-224: Do not catch blind exception: Exception
(BLE001)
[error] 253-254: try-except-continue detected, consider logging the exception
(S112)
[warning] 253-253: Do not catch blind exception: Exception
(BLE001)
[warning] 282-282: Do not catch blind exception: Exception
(BLE001)
[warning] 314-314: Do not catch blind exception: Exception
(BLE001)
[warning] 332-332: Do not catch blind exception: Exception
(BLE001)
[warning] 357-357: Do not catch blind exception: Exception
(BLE001)
[warning] 391-391: Do not catch blind exception: Exception
(BLE001)
[warning] 429-429: Do not catch blind exception: Exception
(BLE001)
[warning] 460-460: Do not catch blind exception: Exception
(BLE001)
[warning] 477-477: Do not catch blind exception: Exception
(BLE001)
[warning] 505-505: Do not catch blind exception: Exception
(BLE001)
[warning] 536-536: Do not catch blind exception: Exception
(BLE001)
[warning] 567-567: Do not catch blind exception: Exception
(BLE001)
[warning] 622-622: Do not catch blind exception: Exception
(BLE001)
[warning] 648-648: Do not catch blind exception: Exception
(BLE001)
🪛 Shellcheck (0.11.0)
scripts/ralph/ralph.sh
[style] 57-57: See if you can use ${variable//search/replace} instead.
(SC2001)
[info] 118-118: Double quote to prevent globbing and word splitting.
(SC2086)
🪛 Stylelint (17.11.1)
frontend/src/styles.css
[error] 1426-1426: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
[error] 1800-1800: Expected keyframe name "transitionStripIn" to be kebab-case (keyframes-name-pattern)
(keyframes-name-pattern)
🔇 Additional comments (43)
frontend/src/GitView.tsx (2)
39-99: LGTM!Also applies to: 101-245
337-488: LGTM!Also applies to: 526-875
.env.example (1)
14-14: LGTM!backend/app/config.py (1)
34-34: LGTM!Also applies to: 70-70, 123-123
.opencode/skills/compound-engineering/SKILL.md (1)
1-250: LGTM!.opencode/skills/dev-browser/SKILL.md (1)
1-53: LGTM!.opencode/skills/frontend-design/SKILL.md (1)
1-39: LGTM!.opencode/skills/prd/SKILL.md (1)
1-243: LGTM!.opencode/skills/ralph/SKILL.md (1)
1-260: LGTM!AGENTS.md (1)
41-82: LGTM!README.md (1)
5-506: LGTM!frontend/src/components/chat/AgentActivityCard.tsx (1)
4-74: LGTM!frontend/src/components/chat/ChatStateCard.tsx (1)
3-18: LGTM!frontend/src/components/chat/ChatTransitionStrip.tsx (1)
3-18: LGTM!frontend/src/components/chat/EmptyState.tsx (1)
4-11: LGTM!frontend/src/components/chat/MessageBubble.tsx (1)
7-77: LGTM!frontend/src/components/chat/MessageParts.tsx (1)
5-78: LGTM!frontend/src/components/chat/RichMessageText.tsx (1)
1-32: LGTM!frontend/src/components/chat/TaskRunTimelineRow.tsx (1)
6-36: LGTM!frontend/src/components/toolbar/InstallControls.tsx (1)
3-37: LGTM!frontend/src/components/toolbar/NotificationControls.tsx (1)
4-83: LGTM!frontend/src/components/ui/CommandPickerModal.tsx (1)
4-57: LGTM!frontend/src/components/ui/FixtureBanner.tsx (1)
4-6: LGTM!frontend/src/components/ui/QuestionCard.tsx (1)
5-86: LGTM!frontend/src/styles.css (1)
1426-1426: ⚡ Quick winProvide the original
<review_comment>(and the related diff/code snippet) so I can rewrite it in the required format.frontend/package.json (1)
14-15: Confirm react-markdown@10.1.0 and remark-gfm@4.0.1 are safe and React 18 compatible.
- No known security vulnerabilities/advisories are reported for
react-markdown@10.1.0orremark-gfm@4.0.1(direct package security listings show none).- These are the latest stable releases for the React 18+ line and are compatible with React 18.3.1.
- Still run
npm audit/Snyk for transitive dependencies, and ensure you’re using the officialremark-gfmpackage (not similarly namedremark-gfm-v4).frontend/src/components/projects/ProjectFilesPanel.tsx (1)
1-501: LGTM!frontend/src/components/projects/ProjectItem.tsx (1)
1-47: LGTM!frontend/src/components/projects/VirtualizedProjectList.tsx (2)
1-91: LGTM!Also applies to: 112-145
97-109: ⚡ Quick winMissing review context: No
<review_comment>content (and no diff/code locations) was provided, so I can’t rewrite or verify the comment. Paste the original review comment and the relevant diff/snippet.frontend/src/components/tasks/RalphLoopPanel.tsx (1)
1-101: LGTM!frontend/src/components/tasks/ScheduledTaskPanel.tsx (1)
1-454: LGTM!backend/app/__init__.py (1)
10-10: LGTM!Also applies to: 266-266
backend/tests/test_git_diff.py (1)
1-106: LGTM!frontend/src/api.ts (1)
74-79: LGTM!Also applies to: 142-172, 204-261, 329-339, 520-690
frontend/src/types.ts (1)
3-4: LGTM!Also applies to: 38-39, 47-56, 78-79, 114-191, 146-155, 296-315
frontend/src/types/internal.ts (1)
1-110: LGTM!frontend/src/utils/fileUtils.ts (1)
1-92: LGTM!frontend/src/utils/formatting.ts (1)
1-175: LGTM!frontend/src/utils/messageUtils.ts (1)
1-367: LGTM!Also applies to: 391-397
frontend/src/utils/miscUtils.ts (1)
1-186: LGTM!frontend/src/utils/taskUtils.ts (1)
1-157: LGTM!prd.json.example (1)
1-65: LGTM!
| @after_this_request | ||
| def cleanup_archive(response): | ||
| if archive_path is not None: | ||
| try: | ||
| archive_path.unlink(missing_ok=True) | ||
| except OSError: | ||
| pass | ||
| return response |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Flask/Werkzeug, does after_this_request run before a send_file(path) response is fully sent, and is response.call_on_close the recommended hook for deleting a temporary file after the download completes?
💡 Result:
Yes for the first part: Flask’s after_this_request runs before the response is actually sent to the client. Flask’s after_request/after_this_request callbacks are meant to “modify or replace the response before it is sent” [1]. So any cleanup you do in an after_this_request handler happens during request processing while Flask/Werkzeug are still preparing to transmit the response, not after the socket write/send is complete. This is consistent with reports where deleting the file in after_this_request happens “before send_file has completed” [2]. For the second part: response.call_on_close is the recommended hook for deferred cleanup when you need it to run after the response is finished/closed. Werkzeug’s Response.call_on_close registers functions to be called when the response is closed (Response.close runs the functions in the internal _on_close list) [3]. Flask community guidance for “after returning the response” uses after_this_request only to get access to the response object, then defers cleanup with response.call_on_close so it runs at response close time [4]. Recommendation: - If your goal is “delete the temporary file after the download completes”, don’t delete it directly in after_this_request; use response.call_on_close for cleanup instead [1][3][4]. - If you delete by pathname, be aware this still relies on OS/filesystem semantics; a safer approach is to pass a file object/stream (or a NamedTemporaryFile-style object) that remains readable until the response is done [5].
Citations:
- 1: https://flask.palletsprojects.com/en/stable/api/
- 2: https://stackoverflow.com/questions/69944395/flask-unable-to-delete-file-after-send-file/69944629
- 3: https://github.com/pallets/werkzeug/blob/main/src/werkzeug/wrappers/response.py
- 4: https://stackoverflow.com/questions/18082683/need-to-execute-a-function-after-returning-the-response-in-flask
- 5: https://stackoverflow.com/questions/24612366/delete-an-uploaded-file-after-downloading-it-from-flask
Move temporary ZIP cleanup to response.call_on_close (not after_this_request)
after_this_request runs before send_file(archive_path) has finished sending the response, so unlinking the temp file there can break the download or leave files behind. Use response.call_on_close to delete the ZIP after the response is closed.
Also applies to backend/app/routes/files.py (lines 223-228).
♻️ Safer cleanup pattern
- `@after_this_request`
- def cleanup_archive(response):
- if archive_path is not None:
- try:
- archive_path.unlink(missing_ok=True)
- except OSError:
- pass
- return response
-
with zipfile.ZipFile(
archive_path, mode="w", compression=zipfile.ZIP_DEFLATED
) as archive:
...
@@
- return send_file(
+ response = send_file(
archive_path,
mimetype="application/zip",
as_attachment=True,
download_name=archive_name,
)
+ `@response.call_on_close`
+ def cleanup_archive():
+ if archive_path is not None:
+ try:
+ archive_path.unlink(missing_ok=True)
+ except OSError:
+ pass
+ return response🤖 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/app/routes/files.py` around lines 187 - 194, The temp ZIP is being
unlinked too early via `@after_this_request`; instead register deletion on
response close using response.call_on_close so the file is removed only after
send_file(archive_path) finishes; locate the cleanup handler named
cleanup_archive (and the similar cleanup block around the other occurrence using
archive_path) and replace the after_this_request decorator logic with a
response.call_on_close(lambda: archive_path.unlink(missing_ok=True) wrapped with
try/except OSError) to ensure safe deletion after the response is fully sent.
| try: | ||
| body = request.get_json(silent=True) or {} | ||
| task_id = str(body.get("id") or body.get("taskId") or "").strip() | ||
| task = ScheduledTask.query.filter_by(id=int(task_id), project_id=project_id).first() if task_id else None | ||
| except (TypeError, ValueError): | ||
| return jsonify({"error": "Invalid task id"}), 400 | ||
|
|
||
| now = _current_utc_now() | ||
| if task is None: | ||
| task = ScheduledTask(project_id=project_id, instruction="", interval_minutes=15) | ||
| db.session.add(task) |
There was a problem hiding this comment.
A missing task lookup silently turns into create.
When id/taskId is provided but doesn't match this project, task stays None and the code falls into the create path. That turns stale UI IDs into duplicate tasks instead of returning a 404/400.
Suggested fix
try:
body = request.get_json(silent=True) or {}
task_id = str(body.get("id") or body.get("taskId") or "").strip()
task = ScheduledTask.query.filter_by(id=int(task_id), project_id=project_id).first() if task_id else None
except (TypeError, ValueError):
return jsonify({"error": "Invalid task id"}), 400
+
+ if task_id and task is None:
+ return jsonify({"error": "Scheduled task not found"}), 404
now = _current_utc_now()
if task is None:
task = ScheduledTask(project_id=project_id, instruction="", interval_minutes=15)
db.session.add(task)🤖 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/app/routes/tasks.py` around lines 110 - 120, The handler currently
treats a provided but-not-found id as "no id" and proceeds to create a new
ScheduledTask; change the flow so that after parsing task_id and resolving task
via ScheduledTask.query.filter_by(...), if a non-empty task_id was provided and
task is None you return an HTTP 404 (or 400) JSON error instead of falling
through to the creation branch. Locate the task_id variable and the lookup using
ScheduledTask.query.filter_by(...) and prevent calling db.session.add(task) when
the client explicitly supplied an id that wasn't found; keep the existing
TypeError/ValueError handling for malformed ids.
| {sortedSessions.length === 0 ? ( | ||
| <option value="" disabled>{sessionLoading ? "Loading..." : "No session"}</option> | ||
| ) : ( | ||
| <> | ||
| {sortedSessions.map((session) => { | ||
| const label = session.title || "Untitled session"; | ||
| const ts = formatSessionTimestamp(session.updatedAt ?? session.createdAt); | ||
| return ( | ||
| <option key={session.id} value={session.id}> | ||
| {label} — {ts} | ||
| </option> | ||
| ); | ||
| })} | ||
| <option value="__new__">+ New session</option> | ||
| </> | ||
| )} |
There was a problem hiding this comment.
Keep “+ New session” available when the list is empty.
Line 120 currently removes the "__new__" option when there are no sessions, so onSessionCreate() can’t be reached from this control.
Suggested fix
<select
value={activeSessionId ?? ""}
onChange={(event) => handleSessionChange(event.currentTarget.value)}
disabled={sessionLoading || sessionSwitching}
>
{sortedSessions.length === 0 ? (
<option value="" disabled>{sessionLoading ? "Loading..." : "No session"}</option>
) : (
- <>
- {sortedSessions.map((session) => {
- const label = session.title || "Untitled session";
- const ts = formatSessionTimestamp(session.updatedAt ?? session.createdAt);
- return (
- <option key={session.id} value={session.id}>
- {label} — {ts}
- </option>
- );
- })}
- <option value="__new__">+ New session</option>
- </>
+ sortedSessions.map((session) => {
+ const label = session.title || "Untitled session";
+ const ts = formatSessionTimestamp(session.updatedAt ?? session.createdAt);
+ return (
+ <option key={session.id} value={session.id}>
+ {label} — {ts}
+ </option>
+ );
+ })
)}
+ <option value="__new__">+ New session</option>
</select>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {sortedSessions.length === 0 ? ( | |
| <option value="" disabled>{sessionLoading ? "Loading..." : "No session"}</option> | |
| ) : ( | |
| <> | |
| {sortedSessions.map((session) => { | |
| const label = session.title || "Untitled session"; | |
| const ts = formatSessionTimestamp(session.updatedAt ?? session.createdAt); | |
| return ( | |
| <option key={session.id} value={session.id}> | |
| {label} — {ts} | |
| </option> | |
| ); | |
| })} | |
| <option value="__new__">+ New session</option> | |
| </> | |
| )} | |
| {sortedSessions.length === 0 ? ( | |
| <option value="" disabled>{sessionLoading ? "Loading..." : "No session"}</option> | |
| ) : ( | |
| sortedSessions.map((session) => { | |
| const label = session.title || "Untitled session"; | |
| const ts = formatSessionTimestamp(session.updatedAt ?? session.createdAt); | |
| return ( | |
| <option key={session.id} value={session.id}> | |
| {label} — {ts} | |
| </option> | |
| ); | |
| }) | |
| )} | |
| <option value="__new__">+ New session</option> |
🤖 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/toolbar/RuntimeControls.tsx` around lines 120 - 135,
The dropdown currently omits the "+ New session" option when
sortedSessions.length === 0, preventing onSessionCreate from being triggered;
update the JSX in RuntimeControls (the component rendering sortedSessions.map
and the option with value="__new__") to always include an <option
value="__new__">+ New session</option> regardless of whether sortedSessions is
empty—e.g., keep the placeholder ("Loading..." or "No session") when there are
no sessions but render the "__new__" option outside or in both branches so the
create action remains reachable.
| const loadHistory = async (skip = 0, append = false) => { | ||
| setHistoryLoading(true); | ||
| try { | ||
| const data: GitHistoryResponse = await apiGitHistory(projectId, { | ||
| limit: HISTORY_PAGE_SIZE, | ||
| skip, | ||
| }); | ||
| setHistory((current) => (append ? [...current, ...data.commits] : data.commits)); | ||
| setHistoryHasMore(data.hasMore); | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : "Failed to load commit history"; | ||
| setError(message); | ||
| } finally { | ||
| setHistoryLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| const loadGitData = async (background = false) => { | ||
| if (background) { | ||
| setRefreshing(true); | ||
| } else { | ||
| setLoading(true); | ||
| } | ||
| setError(null); | ||
|
|
||
| try { | ||
| const [statusData, branchData] = await Promise.all([apiGitStatus(projectId), apiGitBranches(projectId)]); | ||
| setStatus(statusData); | ||
| setBranches(branchData); | ||
|
|
||
| const primaryRemote = statusData.remoteDetails.find((remote) => remote.name === "origin") ?? statusData.remoteDetails[0]; | ||
| setRemoteUrl((currentRemoteUrl) => { | ||
| if (!primaryRemote) { | ||
| return ""; | ||
| } | ||
| return primaryRemote.url ?? currentRemoteUrl; | ||
| }); | ||
|
|
||
| if (statusData.hasCommits) { | ||
| await loadHistory(0, false); | ||
| } else { | ||
| setHistory([]); | ||
| setHistoryHasMore(false); | ||
| } | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : "Failed to load git workspace"; | ||
| setError(message); | ||
| setStatus(null); | ||
| setBranches(null); | ||
| setHistory([]); | ||
| setHistoryHasMore(false); | ||
| } finally { | ||
| if (background) { | ||
| setRefreshing(false); | ||
| } else { | ||
| setLoading(false); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| useEffect(() => { | ||
| if (!projectId) { | ||
| return; | ||
| } | ||
| void loadGitData(); | ||
| setActiveSection("overview"); | ||
| setCommitMessage(""); | ||
| setNewBranchName(""); | ||
| setTrackLocalName(""); | ||
| setNotice(null); | ||
| }, [projectId]); |
There was a problem hiding this comment.
Guard async state updates to avoid stale project data overwriting current view.
loadGitData/loadHistory can resolve out of order when projectId changes quickly, and stale responses still call setState.
Suggested fix pattern (request id guard)
-import { FormEvent, useEffect, useMemo, useState } from "react";
+import { FormEvent, useEffect, useMemo, useRef, useState } from "react";
...
export default function GitView({ projectId, mobile = false }: GitViewProps) {
+ const requestIdRef = useRef(0);
...
- const loadHistory = async (skip = 0, append = false) => {
+ const loadHistory = async (skip = 0, append = false, requestId = requestIdRef.current) => {
setHistoryLoading(true);
try {
const data: GitHistoryResponse = await apiGitHistory(projectId, {
limit: HISTORY_PAGE_SIZE,
skip,
});
+ if (requestId !== requestIdRef.current) return;
setHistory((current) => (append ? [...current, ...data.commits] : data.commits));
setHistoryHasMore(data.hasMore);
} catch (err) {
+ if (requestId !== requestIdRef.current) return;
const message = err instanceof Error ? err.message : "Failed to load commit history";
setError(message);
} finally {
- setHistoryLoading(false);
+ if (requestId === requestIdRef.current) setHistoryLoading(false);
}
};
const loadGitData = async (background = false) => {
+ const requestId = ++requestIdRef.current;
...
const [statusData, branchData] = await Promise.all([apiGitStatus(projectId), apiGitBranches(projectId)]);
+ if (requestId !== requestIdRef.current) return;
setStatus(statusData);
setBranches(branchData);
...
- await loadHistory(0, false);
+ await loadHistory(0, false, requestId);
...
} catch (err) {
+ if (requestId !== requestIdRef.current) return;
...
} finally {
+ if (requestId !== requestIdRef.current) return;
if (background) {
setRefreshing(false);
} else {
setLoading(false);
}
}
};
useEffect(() => {
if (!projectId) {
+ setLoading(false);
return;
}
void loadGitData();
...
+ return () => {
+ requestIdRef.current += 1;
+ };
}, [projectId]);🤖 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/GitView.tsx` around lines 265 - 335, Add a request-id guard so
async responses from loadGitData and loadHistory don't overwrite state for a
newer projectId: create a mutable ref (e.g., requestsRef) that you increment/set
to a unique token inside useEffect before calling loadGitData, pass or capture
the token inside loadGitData and loadHistory, and before every setState call
(setStatus, setBranches, setRemoteUrl, setHistory, setHistoryHasMore, setError,
setLoading, setRefreshing, etc.) check that the current token still matches the
ref; if it does not, return early and skip state updates; also update the token
on projectId change and optionally clear/abort any outstanding work on unmount.
| remoteBranches.map((branch) => ( | ||
| <GitBranchRow | ||
| key={branch.name} | ||
| branch={branch} | ||
| action={() => { | ||
| void handleTrackRemoteBranch(branch); | ||
| }} | ||
| actionLabel={branch.trackedBy ? `Open ${branch.trackedBy}` : "Track branch"} | ||
| actionBusy={operation === "track"} | ||
| actionDisabled={operation !== null} |
There was a problem hiding this comment.
Open <local-branch> action is wired to the wrong handler.
On Line 758 the label implies checkout, but the click handler always calls tracking logic. For already tracked remotes, this can fail or perform the wrong action.
Suggested fix
remoteBranches.map((branch) => (
<GitBranchRow
key={branch.name}
branch={branch}
action={() => {
- void handleTrackRemoteBranch(branch);
+ if (branch.trackedBy) {
+ void handleCheckoutLocalBranch(branch.trackedBy);
+ return;
+ }
+ void handleTrackRemoteBranch(branch);
}}
actionLabel={branch.trackedBy ? `Open ${branch.trackedBy}` : "Track branch"}
- actionBusy={operation === "track"}
+ actionBusy={operation === "track" || operation === "checkout"}
actionDisabled={operation !== null}
/>
))| const response = record.response ?? record.decision ?? record.action; | ||
| if (typeof response === "string") { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Do not treat every string decision as “resolved.”
Line 85 currently marks permission state as resolved for any string value. That can incorrectly resolve pending/unknown decisions and drop active approval prompts.
Suggested fix
const response = record.response ?? record.decision ?? record.action;
if (typeof response === "string") {
- return true;
+ return isPermissionResolved(response);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = record.response ?? record.decision ?? record.action; | |
| if (typeof response === "string") { | |
| return true; | |
| } | |
| const response = record.response ?? record.decision ?? record.action; | |
| if (typeof response === "string") { | |
| return ["approved", "denied", "resolved"].includes(response); | |
| } |
🤖 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/utils/streamUtils.ts` around lines 83 - 86, The current check in
streamUtils.ts that treats any string response as resolved (const response =
record.response ?? record.decision ?? record.action; if (typeof response ===
"string") return true;) is too broad; change it to only treat explicit resolved
decision values as resolved (e.g., check response === "allow" || response ===
"deny" or map to a small whitelist of resolved string constants), or implement a
helper like isResolvedDecision(response) that returns true only for those known
resolved values; update the conditional that references response (and any
callers that rely on it) so pending/unknown string decisions are not treated as
resolved.
|
@copilot Please address all the comments on this PR and prepare it for merge. |
Agent-Logs-Url: https://github.com/Rishabh-Bajpai/mobile-opencode-control/sessions/7ec5ccc8-ef02-4677-9d62-bbad579b7bbd Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
Addressed the blocking review items in
Addressed the requested blockers in |
Agent-Logs-Url: https://github.com/Rishabh-Bajpai/mobile-opencode-control/sessions/7ec5ccc8-ef02-4677-9d62-bbad579b7bbd Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Rishabh-Bajpai/mobile-opencode-control/sessions/7ec5ccc8-ef02-4677-9d62-bbad579b7bbd Co-authored-by: Rishabh-Bajpai <28703138+Rishabh-Bajpai@users.noreply.github.com>
Summary by CodeRabbit