Enhance project features, UI improvements, and documentation updates - #7
Conversation
…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.
…s with enhanced output formatting and file path resolution.
…ession management
… event classification utilities
… and message tracking to stream utilities
…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
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (19)
backend/app/routes/files.py-47-49 (1)
47-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid broad catches that expose internal errors to clients.
Line 47 and Line 78 catch
Exceptionand return raw exception details in JSON. This can leak server internals and hide real bugs. Catch expected filesystem errors and return a generic 500 payload.Proposed fix
- except Exception as exc: - return jsonify({"error": f"Failed to list project files: {exc}"}), 500 + except OSError: + return jsonify({"error": "Failed to list project files"}), 500- except Exception as exc: - return jsonify({"error": f"Failed to list directory: {exc}"}), 500 + except OSError: + return jsonify({"error": "Failed to list directory"}), 500Also applies to: 78-80
🤖 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 47 - 49, The except blocks that currently catch broad Exception and return the raw exception in JSON (the handlers that return {"error": f"Failed to list project files: {exc}"} and the similar block at lines ~78) should be replaced with specific exception handling: catch expected filesystem errors (e.g., FileNotFoundError, PermissionError, OSError) and for any unexpected errors let them bubble or be handled by a global error handler; in each specific except, log the full exception details server-side using the app logger (e.g., app.logger.exception or logger.exception) and return a generic JSON error like {"error": "Failed to list project files"} with status 500 without embedding exc; keep the same function/context (the handler that produces "Failed to list project files" and the other handler at ~78) when implementing these changes.backend/app/routes/helpers.py-634-651 (1)
634-651:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t fail hard on stale cached
last_session_id.Line 649 currently raises when
get_sessionfails for any candidate, including cachedproject.last_session_id. That blocks fallback session resolution and can break projects with stale session pointers.Proposed fix
- had_candidate = candidate_session_id is not None + explicit_session_requested = ( + isinstance(session_id, str) and bool(session_id.strip()) + ) ... - except requests.HTTPError as exc: + except requests.HTTPError as exc: project.last_session_id = None db.session.commit() - if had_candidate: + if explicit_session_requested: raise ValueError("Selected session could not be loaded") from exc🤖 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/helpers.py` around lines 634 - 651, The code currently treats any candidate_session_id failure the same and raises a ValueError, which breaks fallback when the candidate came from the cached project.last_session_id; change the logic so only explicit caller-supplied candidates cause a hard failure. Record whether the candidate was provided by the caller (e.g., rename or set a flag like explicit_candidate = candidate_session_id_was_passed) before you fall back to project.last_session_id, then in the except requests.HTTPError block clear project.last_session_id and commit as now but only raise ValueError("Selected session could not be loaded") from exc when explicit_candidate is true; otherwise swallow the error and allow fallback session resolution to continue (do not raise).backend/app/routes/helpers.py-890-891 (1)
890-891:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove permissive substring matching in session-event filter.
Line 890 uses
session_id in payload, which can match unrelated IDs by substring and attach events to the wrong session.Proposed fix
- if session_id in payload: - return True - try: parsed = json.loads(payload) except json.JSONDecodeError: return True🤖 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/helpers.py` around lines 890 - 891, The current check uses permissive substring matching ("session_id in payload") which can incorrectly match unrelated IDs; change it to a strict equality check against the structured session field instead. Replace the substring test with something like payload.get("session_id") == session_id (or, if payload is a stringified JSON, parse it first and then compare the parsed_payload.get("session_id") == session_id) so only exact session ID matches attach the event; update the check where the session-event filter is implemented (the code referencing session_id and payload).backend/app/routes/helpers.py-985-987 (1)
985-987:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize numeric parsing before assignment to avoid 500s.
maxRunsandrunTimeoutMinutesare converted at assignment time (Line 985/986), outside the guarded parse block. Invalid values raise unhandledValueErrorand return 500 instead of a validation error.Proposed fix
try: interval_minutes = int(body.get("intervalMinutes") or 15) - max_runs = body.get("maxRuns") - run_timeout_minutes = body.get("runTimeoutMinutes") + raw_max_runs = body.get("maxRuns") + raw_run_timeout_minutes = body.get("runTimeoutMinutes") + max_runs = int(raw_max_runs) if raw_max_runs not in (None, "") else None + run_timeout_minutes = ( + int(raw_run_timeout_minutes) + if raw_run_timeout_minutes not in (None, "") + else None + ) retry_count = int(body.get("retryCount") or 0) retry_backoff_minutes = int(body.get("retryBackoffMinutes") or 5) - except (TypeError, ValueError): - raise ValueError("Numeric task fields are invalid") + except (TypeError, ValueError) as exc: + raise ValueError("Numeric task fields are invalid") from exc ... - task.max_runs = int(max_runs) if max_runs not in (None, "") else None - task.run_timeout_minutes = int(run_timeout_minutes) if run_timeout_minutes not in (None, "") else None + task.max_runs = max_runs + task.run_timeout_minutes = run_timeout_minutes🤖 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/helpers.py` around lines 985 - 987, The assignments to task.max_runs and task.run_timeout_minutes currently parse ints inline and can raise ValueError; move numeric parsing into the existing guarded/validated parse block (where you validate other body fields) and only assign task.max_runs and task.run_timeout_minutes after successful int conversion (e.g., parse max_runs and run_timeout_minutes with try/except or the existing validator, convert empty strings to None) so invalid inputs produce a validation error response instead of an unhandled exception; keep the heartbeat line as-is (task.heartbeat_enabled = bool(body.get("heartbeatEnabled", True))) and reference the variables max_runs/run_timeout_minutes and the task properties task.max_runs/task.run_timeout_minutes when locating code to change.backend/app/routes/messages.py-87-117 (1)
87-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle invalid
sessionIdas a 400 before marking the project as failed.
_resolve_project_session(..., create_if_missing=False)is resolving caller input here. If it raisesValueError, the generic handler turns that into a 502 and Lines 115/224 setproject.session_status = "error", even though this was just a bad request. CatchValueErrorseparately and leave the existing status alone.Suggested fix
- except Exception as exc: + except ValueError as exc: + return jsonify({"error": str(exc)}), 400 + except Exception as exc: project.session_status = "error" db.session.commit() return jsonify({"error": f"Failed to send message: {exc}"}), 502Apply the same split in
run_project_command().Also applies to: 146-226
🤖 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/messages.py` around lines 87 - 117, The try/except around sending messages treats caller-supplied bad session IDs (raised as ValueError by _resolve_project_session) as server errors and marks project.session_status="error"; update the handler to catch ValueError separately: when _resolve_project_session(...) raises ValueError, do not change project.session_status or commit a failure—return a 400 JSON response with the validation message; keep the existing broad except Exception block to set project.session_status="error", commit, and return 502 for real server failures. Apply the identical ValueError-vs-Exception split to run_project_command() as well so invalid sessionId inputs return 400 without mutating project.session_status.backend/app/routes/sessions.py-272-292 (1)
272-292:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't turn an invalid
sessionIdinto a 502 and an"error"project state.This route accepts a client-supplied
sessionId, but anyValueErrorfrom_resolve_project_session(..., create_if_missing=False)is swallowed by the generic handler on Line 289. That returns 502 and marks the project as failed for a bad request. HandleValueErrorlike the compact/summarize routes do and leave the current status unchanged.Suggested fix
- except Exception as exc: + except ValueError as exc: + return jsonify({"error": str(exc)}), 400 + except Exception as exc: project.session_status = "error" db.session.commit() return jsonify({"error": f"Failed to abort session: {exc}"}), 502🤖 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/sessions.py` around lines 272 - 292, The handler currently wraps all exceptions and sets project.session_status="error", but a ValueError raised by _resolve_project_session(..., create_if_missing=False) should be treated as a client error; update the try/except to catch ValueError separately (before the broad Exception) and return a 400 response with the message (without changing project.session_status), while keeping the existing broad Exception handler to set project.session_status="error" and return 502 for real server failures; make sure to reference the same session resolution flow (_resolve_project_session, _ensure_project_session) and the abort action (opencode_client.abort_session) so the new ValueError branch only handles invalid sessionId cases.backend/app/routes/messages.py-159-169 (1)
159-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve the default provider/model pair correctly before calling
summarize_session().
provider_data["default"]is used elsewhere in this PR as{provider_id: model_id}. Here Line 161 takes the first value and splits it on/, soprovider_idbecomes the model ID andmodel_idbecomes empty. That breaks the/compact//summarizepath whenever defaults are configured.Suggested fix
- default_model = default_map.get(list(default_map.keys())[0]) if default_map else None - if default_model and isinstance(default_model, str): - provider_id, _, model_id = default_model.partition("/") + provider_id = next(iter(default_map), None) + model_id = ( + str(default_map.get(provider_id)).strip() + if provider_id and default_map.get(provider_id) + else "" + ) + if provider_id and model_id: opencode_client.summarize_session( session_id, provider_id=provider_id, model_id=model_id, directory=project.path,🤖 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/messages.py` around lines 159 - 169, The code incorrectly extracts provider/model by taking the first value from default_map and partitioning it, which flips provider_id and model_id; instead, pull the first key as provider_id and use default_map[provider_id] as model_id (verify it's a string), then pass those to opencode_client.summarize_session with session_id and directory=project.path; update the logic around provider_data, default_map, default_model and the call to summarize_session accordingly so provider_id is the map key and model_id is the corresponding value.backend/app/routes/sessions.py-343-352 (1)
343-352:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve the default summarize model from
default_mapby key/value, not by splitting the value.
provider_data["default"]is treated elsewhere in this PR as{provider_id: model_id}. Here Lines 348-350 assume the first value looks like"provider/model", soprovider_idends up holding the model ID andmodel_idbecomes empty. Default summarize requests will fail once defaults are configured.Suggested fix
- if not provider_id or not model_id: - default_model = default_map.get(list(default_map.keys())[0]) if default_map else None - if default_model and isinstance(default_model, str): - provider_id, _, model_id = default_model.partition("/") + if not provider_id or not model_id: + provider_id = next(iter(default_map), None) + model_id = ( + str(default_map.get(provider_id)).strip() + if provider_id and default_map.get(provider_id) + else "" + ) + if provider_id and model_id: provider_id, _, model_id = default_model.partition("/") else: return jsonify({"error": "providerID and modelID are required"}), 400🤖 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/sessions.py` around lines 343 - 352, The code incorrectly parses the default provider/model by splitting a string from default_map; instead read the first key/value pair from default_map (which is structured as {provider_id: model_id}) and assign provider_id to the key and model_id to the value. In the block using provider_data/default_map (and variables provider_id, model_id, default_model), replace the logic that expects "provider/model" with logic that extracts the first item from default_map.items(), validates the types (key is str for provider_id and value is str for model_id), and uses those values as the defaults before returning the 400 error.backend/app/routes/tasks.py-173-174 (1)
173-174:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid returning raw exception details in client-facing errors.
These responses expose internal exception text directly. Return stable/generic error messages and keep details in server logs.
Suggested fix
- except Exception as exc: - return jsonify({"error": f"Failed to run task: {exc}"}), 502 + except Exception: + return jsonify({"error": "Failed to run task"}), 502 ... - except Exception as exc: - return jsonify({"error": str(exc)}), 400 + except Exception: + return jsonify({"error": "Invalid schedule payload"}), 400Also applies to: 262-263
🤖 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 173 - 174, Replace the client-facing error that returns raw exception text in the except blocks in backend/app/routes/tasks.py (the "except Exception as exc:" handlers around the task-running code and the similar handler at lines ~262-263) with a stable, generic JSON error message (e.g., {"error":"Failed to run task"}) and log the full exception server-side using a logger method such as logger.exception or current_app.logger.exception so the stacktrace is retained for debugging without exposing internals to clients; update both occurrences (the handler that currently does return jsonify({"error": f"Failed to run task: {exc}"}), 502) to follow this pattern.backend/app/git_routes.py-596-597 (1)
596-597:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn a stable 500 error message instead of raw exception text.
Returning
str(e)may leak internal command/runtime details to clients.Suggested fix
- except Exception as e: - return jsonify({"error": str(e)}), 500 + except Exception: + return jsonify({"error": "Failed to generate git diff"}), 500🤖 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/git_routes.py` around lines 596 - 597, The except block in backend/app/git_routes.py currently returns raw exception text via jsonify({"error": str(e)}), which can leak internals; change this to return a stable 500 message such as jsonify({"error":"Internal server error"}) with a 500 status, and instead log the original exception internally (e.g., use logger.exception or current_app.logger.exception referencing the caught variable e) so the details are preserved in server logs but not sent to clients; update the except block where jsonify and the exception variable e are used to implement this.backend/app/git_routes.py-578-591 (1)
578-591:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound untracked patch generation to avoid large/binary payload blowups.
Reading and returning full content for every untracked file is unbounded and can create very large responses (memory/latency/UI impact).
Suggested fix
for u in repo.untracked_files: try: from pathlib import Path - content = Path(repo.working_dir, u).read_text(errors="replace") + path = Path(repo.working_dir, u) + max_bytes = 200_000 + if path.stat().st_size > max_bytes: + entries.append({ + "path": u, + "changeType": "?", + "patch": f"[untracked file omitted: larger than {max_bytes} bytes]", + }) + continue + raw = path.read_bytes() + if b"\x00" in raw[:8192]: + entries.append({ + "path": u, + "changeType": "?", + "patch": "[binary file omitted]", + }) + continue + content = raw.decode("utf-8", errors="replace") lines = content.splitlines() line_count = len(lines) patch_lines = ["--- /dev/null", f"+++ b/{u}", f"@@ -0,0 +1,{line_count} @@"]🤖 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/git_routes.py` around lines 578 - 591, The current loop over repo.untracked_files in git_routes.py reads entire file contents (Path(...).read_text) which can return arbitrarily large or binary data; change this to read only a bounded amount and detect binary content: open the file in binary, read up to a max byte limit (e.g., 64KB), if a null byte or non-text bytes are present mark changeType as "?" with a note like "(binary or truncated)" and do not attempt to split into full lines, otherwise decode the bytes with errors="replace", split into lines limited to a max line count, build patch_lines from the truncated/decoded content and reflect the truncated line_count in the patch header and/or append a truncation marker so entries (path, changeType, patch) never contain unbounded payloads; update the code around the repo.untracked_files loop and the entries/potch_lines construction to implement these checks and limits.backend/app/git_routes.py-552-576 (1)
552-576:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle diff failures per file so one bad path doesn’t drop the whole section.
Current
tryscope wraps the full staged/changed blocks. A singleGitCommandErrorskips all remaining files in that section.Suggested fix
- try: - staged_output = repo.git.diff("--cached", unified=5) - if staged_output.strip(): - staged_paths = _staged_paths(repo) - for p in staged_paths: - patch = repo.git.diff("--cached", "--", p, unified=5) - entries.append({ - "path": p, - "changeType": "M", - "patch": patch, - }) - except exc.GitCommandError: - pass + staged_paths = _staged_paths(repo) + for p in staged_paths: + try: + patch = repo.git.diff("--cached", "--", p, unified=5) + except exc.GitCommandError: + continue + entries.append({ + "path": p, + "changeType": "M", + "patch": patch, + }) - try: - changed_paths = _changed_paths(repo) - for p in changed_paths: - patch = repo.git.diff("--", p, unified=5) - entries.append({ - "path": p, - "changeType": "M", - "patch": patch, - }) - except exc.GitCommandError: - pass + changed_paths = _changed_paths(repo) + for p in changed_paths: + try: + patch = repo.git.diff("--", p, unified=5) + except exc.GitCommandError: + continue + entries.append({ + "path": p, + "changeType": "M", + "patch": patch, + })🤖 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/git_routes.py` around lines 552 - 576, The current try/except around the whole staged and changed blocks causes one GitCommandError to skip all files; modify the code to catch errors per file by keeping the initial existence checks (repo.git.diff("--cached", unified=5) and _changed_paths(repo)) but move the try/except inside the loops so each call to repo.git.diff("--cached", "--", p, unified=5) and repo.git.diff("--", p, unified=5) is wrapped in its own try/except exc.GitCommandError that logs or silently continues and does not abort the rest of the loop; update the sections that reference _staged_paths, _changed_paths, repo.git.diff and entries so each file is handled independently.backend/app/routes/tasks.py-96-104 (1)
96-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t upsert-create when a
taskIdis explicitly provided but not found.If the client sends
taskIdand it misses, this currently creates a new task instead of failing. That can create unintended duplicates and masks client-side state bugs.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 + task = ScheduledTask.query.filter_by(id=int(task_id), project_id=project_id).first() if task_id else None + if task_id and task is None: + return jsonify({"error": "Scheduled task not found"}), 404 except (TypeError, ValueError): return jsonify({"error": "Invalid task id"}), 400🤖 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 96 - 104, When a client provides a taskId but no matching ScheduledTask exists, do not create (upsert) a new task; instead return a 404 error. Modify the logic around task_id and task in the handler so that after computing task_id and querying ScheduledTask (variables: body, task_id, task, ScheduledTask, project_id), you check: if task_id is non-empty and task is None then return jsonify({"error": "Task not found"}), 404; only create and db.session.add a new ScheduledTask when task_id was not provided (i.e., task_id is empty and task is None).frontend/src/utils/streamUtils.ts-83-86 (1)
83-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not treat every string response as a resolved permission.
At Line 84–86,
"pending"(or any arbitrary string) is treated as resolved, which can prematurely clear approval requests.🔧 Proposed fix
const response = record.response ?? record.decision ?? record.action; if (typeof response === "string") { - return true; + const normalized = response.toLowerCase(); + return ( + normalized.includes("allow") || + normalized.includes("approve") || + normalized.includes("deny") || + normalized.includes("reject") + ); }🤖 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 treats any string as a resolved permission (const response = record.response ?? record.decision ?? record.action; if (typeof response === "string") return true;), which will mark values like "pending" as resolved; change this to only accept explicit success tokens by replacing the typeof check with a whitelist comparison (e.g., response === "approved" || response === "granted" || response === "allowed") or a Set lookup of allowed success strings, so only those known resolved values cause the function (or helper that uses response) to return true and strings like "pending" remain unresolved.frontend/src/utils/streamUtils.ts-274-279 (1)
274-279:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback classification misses
message.part.updatedas a part update.When only
wrapper.typeis present,message.part.updatedis currently classified as a generic message update (Line 274–279) instead ofhasPartUpdate, which can skip part-specific handlers.🔧 Proposed fix
} else if ( + directType === "message.part.updated" + ) { + result.hasPartUpdate = true; + } else if ( directType.startsWith("text.") || directType.startsWith("tool.") || directType.startsWith("message.") || directType === "prompted" ) {🤖 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 274 - 279, The fallback classification currently treats wrapper.type values like "message.part.updated" as a generic message update and sets result.hasMessageUpdate; update the classification logic that inspects directType/wrapper.type so that when directType === "message.part.updated" (or wrapper.type resolves to that string) you set result.hasPartUpdate = true (and avoid only setting result.hasMessageUpdate), ensuring downstream part-specific handlers run; locate the branch that checks directType.startsWith(...) and directType === "prompted" and add an explicit check for "message.part.updated" before the generic message update assignment.frontend/src/utils/projectUtils.ts-15-18 (1)
15-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize slash-only roots to
/instead of empty string.At Line 18, inputs like
"//"normalize to"", which turns an absolute root into a relative path in later path-building flows.🔧 Proposed fix
export function normalizeProjectRootPath(path: string) { const trimmed = path.trim(); - if (!trimmed || trimmed === "/") { + if (!trimmed) { return trimmed; } + if (/^\/+$/.test(trimmed)) { + return "/"; + } return trimmed.replace(/\/+$/, ""); }🤖 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 15 - 18, The current normalization returns an empty string for inputs that are only slashes (e.g., "//") because trimmed.replace(/\/+$/, "") yields "", so update the logic that handles the variable trimmed: if trimmed is falsy or consists only of one or more slashes, return "/" instead of trimmed; specifically detect when trimmed.replace(/\/+$/, "") is empty and return "/" (preserving the existing behavior for other paths), adjusting the branch around the trimmed checks in the path-normalization function that contains the shown trimmed logic.frontend/src/components/projects/ProjectFilesPanel.tsx-218-224 (1)
218-224:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent duplicate directory expansion requests on ArrowRight.
Line 223 can fire repeatedly while a directory is already loading, causing redundant API calls and racey UI state.
Proposed fix
- } else if (!loadedDirectories.includes(focusedEntry.entry.path)) { + } else if ( + !loadedDirectories.includes(focusedEntry.entry.path) && + !loadingDirectories.includes(focusedEntry.entry.path) + ) { void onExpandDirectory(focusedEntry.entry.path); }🤖 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/projects/ProjectFilesPanel.tsx` around lines 218 - 224, The ArrowRight handler can call onExpandDirectory repeatedly while a directory is already loading; add a loading guard to prevent duplicate requests by tracking a set (e.g., loadingDirectories) of paths currently being expanded: check loadingDirectories.has(focusedEntry.entry.path) alongside collapsedDirectories and loadedDirectories before calling onExpandDirectory, add the path to loadingDirectories immediately before invoking the async expand (void onExpandDirectory(...)) and remove it from loadingDirectories in the expand completion/error handling; update the handler and the async expand logic (references: focusedEntry, collapsedDirectories, loadedDirectories, toggleDirectory, onExpandDirectory) to use this loading set so only one expansion request can be in-flight per directory.frontend/src/components/toolbar/RuntimeControls.tsx-120-135 (1)
120-135:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways expose “+ New session” in the session picker.
When there are no sessions, the selector renders only a disabled placeholder, so users can’t create a first/new session 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> - </> - )} + <option value="" disabled> + {sessionLoading ? "Loading..." : "Select 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>🤖 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 session picker currently hides the "+ New session" option when sortedSessions.length === 0, preventing creation of the first session; change the JSX in RuntimeControls so the <option value="__new__">+ New session</option> is always rendered (either move it outside the sortedSessions conditional or add it alongside the disabled placeholder), keeping the disabled placeholder logic for the empty state and preserving existing values/keys (use the existing "__new__" value and retain formatSessionTimestamp and sessionLoading checks).frontend/src/components/projects/VirtualizedProjectList.tsx-97-109 (1)
97-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent duplicate
onReachEnd()calls near the bottom.The current threshold check can fire
onReachEnd()repeatedly across rapid scroll events beforeisLoadingMoreflips, which can trigger duplicate pagination requests.Suggested fix
import React, { useEffect, useRef, useState } from "react"; @@ const containerRef = useRef<HTMLDivElement | null>(null); + const reachEndRequestedRef = useRef(false); @@ + useEffect(() => { + if (!isLoadingMore) { + reachEndRequestedRef.current = false; + } + }, [isLoadingMore]); + return ( @@ - if (visibleBottom >= event.currentTarget.scrollHeight - threshold) { + if ( + visibleBottom >= event.currentTarget.scrollHeight - threshold && + !reachEndRequestedRef.current + ) { + reachEndRequestedRef.current = true; onReachEnd(); } }}🤖 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/projects/VirtualizedProjectList.tsx` around lines 97 - 109, The onScroll handler can call onReachEnd multiple times before isLoadingMore flips; add a local reentrancy guard (e.g., a useRef like isFetchingRef) and check it alongside hasMore/isLoadingMore inside the onScroll callback, set isFetchingRef.current = true immediately before invoking onReachEnd, and clear it when loading finishes (watch isLoadingMore with an effect and set isFetchingRef.current = false when isLoadingMore becomes false); this uses the existing symbols onScroll, setScrollTop, hasMore, isLoadingMore, onReachEnd, threshold and rowHeight to prevent duplicate pagination requests.
🟡 Minor comments (5)
frontend/src/components/auth/LoginView.tsx-24-33 (1)
24-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrevent empty-password submissions client-side.
At Line 31, submit is disabled only during loading. Empty/whitespace passwords still submit and trigger unnecessary auth attempts.
🔧 Proposed fix
<input type="password" value={password} onChange={(event) => setPassword(event.target.value)} placeholder="Enter password" autoComplete="current-password" + required /> - <button disabled={loading} type="submit"> + <button disabled={loading || !password.trim()} type="submit"> {loading ? "Signing in..." : "Sign in"} </button>🤖 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 - 33, The submit button currently only disables during loading, allowing empty/whitespace passwords to be submitted; update the client-side check to prevent this by disabling the button when password.trim() === '' in addition to loading (i.e., change the button's disabled prop to loading || password.trim() === '') and/or add a guard at the start of the submit handler (e.g., handleSubmit) that returns early if password.trim() is empty to avoid unnecessary auth attempts.frontend/src/utils/streamUtils.ts-315-317 (1)
315-317:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSupport both camelCase and uppercase ID keys in part payload parsing.
At Line 315–317, only
sessionID/messageIDare read. If upstream emitssessionId/messageId, the part event is dropped as incomplete.🔧 Proposed fix
- const sessionID = typeof parsed.sessionID === "string" ? parsed.sessionID : null; - const messageID = typeof part.messageID === "string" ? part.messageID : null; + const sessionID = + typeof parsed.sessionID === "string" + ? parsed.sessionID + : typeof parsed.sessionId === "string" + ? parsed.sessionId + : null; + const messageID = + typeof part.messageID === "string" + ? part.messageID + : typeof part.messageId === "string" + ? part.messageId + : null;🤖 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 315 - 317, The parsing currently only accepts parsed.sessionID, part.messageID and part.id which drops events when upstream uses sessionId/messageId/ID variants; update the assignments for sessionID, messageID and partID to check both variants (e.g., sessionID OR sessionId, messageID OR messageId, part.id OR part.ID) and fall back to null if neither is a string so the rest of the logic in streamUtils.ts that uses parsed and part will accept both naming conventions.frontend/src/components/chat/MessagePartCard.tsx-124-126 (1)
124-126:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix step-finish placeholder conditions to match displayed fields.
Line 167 uses falsy checks (
!cost,!tokens) that don’t match the actual rendering predicates, socost = 0andtokens = {}can produce incorrect placeholder behavior.Proposed fix
const tokens = part.tokens as Record<string, unknown> | undefined; const cost = typeof part.cost === "number" ? part.cost : null; + const hasCost = cost !== null; + const hasTokens = !!tokens && Object.keys(tokens).length > 0; @@ - {cost !== null && ( + {hasCost && ( <div className="step-info"> <span className="step-label">Cost:</span> <code className="step-value">${cost.toFixed(4)}</code> </div> )} - {tokens && Object.keys(tokens).length > 0 && ( + {hasTokens && ( <div className="step-info"> <span className="step-label">Tokens:</span> <div className="step-token-grid"> {Object.entries(tokens).map(([key, val]) => ( @@ - {partType === "step-finish" && !reason && !cost && !tokens && ( + {partType === "step-finish" && !reason && !hasCost && !hasTokens && ( <p className="step-placeholder">Step finished</p> )}Also applies to: 143-169
🤖 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 124 - 126, The placeholder logic uses falsy checks (!cost, !tokens) which treat cost=0 and tokens={} incorrectly; update the conditions in MessagePartCard to check for explicit null/undefined instead (e.g., use cost === null or cost === undefined / cost == null, and tokens == null) or, for tokens, check emptiness with Object.keys(tokens).length === 0 if empty object should be treated as placeholder; update all occurrences in the 143-169 block (references: part, tokens, cost, and the rendering branches that show placeholders) to use these explicit null/undefined or emptiness checks so displayed fields match the placeholder behavior.frontend/src/components/projects/ProjectFilesPanel.tsx-143-143 (1)
143-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid contradictory empty-state messaging when a filter is active.
The current checks can show both “No files match this filter.” and “No files available in this project yet.” for the same filtered state.
Proposed fix
- const fileEntries = visibleEntries.filter((entry) => !entry.isDir); + const fileEntries = visibleEntries.filter((entry) => !entry.isDir); + const totalFileEntries = dedupedEntries.filter((entry) => !entry.isDir); @@ - {!loading && !loadError && fileEntries.length === 0 ? ( + {!loading && !loadError && !normalizedQuery && totalFileEntries.length === 0 ? ( <div className="project-files-muted">No files available in this project yet.</div> ) : null}Also applies to: 378-380, 493-495
🤖 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/projects/ProjectFilesPanel.tsx` at line 143, The empty-state logic currently derives fileEntries from visibleEntries and can render both “No files match this filter.” and “No files available in this project yet.” simultaneously; update the conditional rendering in ProjectFilesPanel (where fileEntries is computed and where empty states are rendered) to first check whether any filter/search/category is active (e.g., using the same predicates that produce visibleEntries or an explicit boolean like isFilterActive/searchQuery/selectedCategory) and if true render only the filtered-message (“No files match this filter.”), otherwise render the project-empty message (“No files available in this project yet.”); apply the same fix to the other two empty-state blocks referenced (around the other occurrences) so only one empty-state message shows depending solely on whether filters are active.frontend/src/components/ui/QuestionCard.tsx-38-56 (1)
38-56:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd defensive check for
info.optionsto prevent runtime error.If
info.optionsis undefined or null, calling.map()will throw a TypeError. Apply the same defensive pattern used forquestion.questionson line 29.🛡️ Proposed fix
<div className="question-options"> - {info.options.map((option, optionIndex) => { + {(info.options ?? []).map((option, optionIndex) => { const letter = String.fromCharCode(65 + optionIndex);🤖 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/QuestionCard.tsx` around lines 38 - 56, The component currently assumes info.options exists and calls info.options.map which can throw if options is null/undefined; update the rendering in QuestionCard (the block that iterates info.options) to guard the iteration the same way question.questions is protected — e.g., only map when info.options is truthy or fallback to an empty array — so the logic around info.options, selectedValues, onToggleOption, questionIndex and question.id remains unchanged but safe when info.options is missing.
🧹 Nitpick comments (1)
frontend/src/utils/messageUtils.ts (1)
368-390: ⚡ Quick winInconsistent type handling in
asBoolean—consider matching the defensive pattern ofasStringandasNumber.
asBooleanshould explicitly type-check for booleans before falling back to a default, consistent with the other coercers. While the actual payload yields proper JSON-deserialized booleans (not strings), defensive typing improves code clarity.Suggested improvement
- const asBoolean = (value: unknown) => Boolean(value); + const asBoolean = (value: unknown) => typeof value === "boolean" ? value : 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, asBoolean currently coerces with Boolean(value) which treats truthy non-boolean values (like strings) as true; change it to mirror asNumber/asString defensive pattern by returning the raw boolean only when typeof value === "boolean" and otherwise returning null (so asBoolean = (value: unknown) => (typeof value === "boolean" ? value : null)); update any downstream usages (e.g., heartbeatLoaded, goalAttempted) that expect a nullable boolean to rely on this stricter coerce behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31e1ae57-0717-4e90-aa2f-0af204e031dd
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (57)
.env.examplePDR.mdREADME.mdbackend/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.pydocs/task-feature-test-plan.mdfrontend/package.jsonfrontend/src/App.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.tstasks.mdui-spec.md
💤 Files with no reviewable changes (4)
- PDR.md
- docs/task-feature-test-plan.md
- tasks.md
- ui-spec.md
📜 Review details
🧰 Additional context used
🪛 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)
🪛 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/git_routes.py
[warning] 596-596: 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/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/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/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/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/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/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/tasks.py
[warning] 173-173: Do not catch blind exception: Exception
(BLE001)
[warning] 262-262: 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/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/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)
🔇 Additional comments (35)
.env.example (1)
14-14: LGTM!README.md (1)
5-506: LGTM!backend/app/config.py (1)
34-35: LGTM!Also applies to: 70-70, 123-124
backend/app/opencode.py (1)
229-307: LGTM!backend/app/routes.py (1)
1-25: LGTM!backend/app/routes/__init__.py (1)
1-42: LGTM!frontend/package.json (1)
13-15: LGTM!frontend/src/types.ts (1)
3-3: LGTM!Also applies to: 38-38, 47-55, 78-78, 114-191
frontend/src/types/internal.ts (1)
1-109: LGTM!frontend/src/api.ts (1)
5-5: LGTM!Also applies to: 13-13, 73-78, 141-148, 151-155, 161-162, 169-170, 203-260, 328-338
frontend/src/utils/fileUtils.ts (1)
1-91: LGTM!frontend/src/utils/formatting.ts (1)
1-174: LGTM!frontend/src/utils/messageUtils.ts (1)
1-367: LGTM!Also applies to: 391-397
frontend/src/utils/miscUtils.ts (1)
1-185: LGTM!frontend/src/utils/taskUtils.ts (1)
1-157: LGTM!frontend/src/components/chat/AgentActivityCard.tsx (1)
1-74: LGTM!frontend/src/components/chat/ChatStateCard.tsx (1)
1-18: LGTM!frontend/src/components/chat/ChatTransitionStrip.tsx (1)
1-18: LGTM!frontend/src/components/chat/DiffPanel.tsx (1)
1-119: LGTM!frontend/src/components/chat/EmptyState.tsx (1)
1-11: LGTM!frontend/src/components/chat/MessageBubble.tsx (1)
1-77: LGTM!frontend/src/components/chat/MessageParts.tsx (1)
1-78: LGTM!frontend/src/components/chat/RichMessageText.tsx (1)
1-32: LGTM!frontend/src/components/chat/TaskRunTimelineRow.tsx (1)
1-37: LGTM!frontend/src/components/projects/ProjectItem.tsx (1)
1-47: LGTM!frontend/src/components/projects/VirtualizedProjectList.tsx (1)
1-96: LGTM!Also applies to: 112-146
frontend/src/components/tasks/RalphLoopPanel.tsx (1)
1-102: LGTM!frontend/src/components/tasks/ScheduledTaskPanel.tsx (1)
1-455: LGTM!frontend/src/components/toolbar/InstallControls.tsx (1)
1-38: LGTM!frontend/src/components/toolbar/NotificationControls.tsx (1)
1-84: LGTM!frontend/src/components/toolbar/RuntimeControls.tsx (1)
1-119: LGTM!Also applies to: 136-161
frontend/src/components/ui/CommandPickerModal.tsx (1)
1-58: LGTM!frontend/src/components/ui/FixtureBanner.tsx (1)
1-7: LGTM!frontend/src/components/ui/QuestionCard.tsx (1)
1-37: LGTM!Also applies to: 57-80
frontend/src/styles.css (1)
686-948: LGTM!Also applies to: 1078-1192, 1759-1794, 2039-2535, 2943-2948, 3394-3527, 3598-3765, 3947-3952, 4477-4479, 4623-4636, 4744-4803
| ntfy_topic_url = str(body.get("ntfyTopicUrl") or "").strip() | ||
| _set_notification_settings(channel, ntfy_topic_url) | ||
| db.session.commit() | ||
| return jsonify({"ok": True, "channel": channel, "ntfyTopicUrl": ntfy_topic_url}) |
There was a problem hiding this comment.
Do not pass caller-controlled notification URLs straight to the outbound sender.
ntfyTopicUrl is accepted from settings updates and per-request bodies, then sent directly to _send_ntfy_notification(). That makes these routes an SSRF primitive against arbitrary internal or cloud-metadata URLs for any authenticated caller. Restrict this to an allowlisted ntfy origin, or accept only a topic name and construct the URL server-side.
Also applies to: 59-67, 82-90
🤖 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/notifications.py` around lines 44 - 47, The code currently
accepts caller-controlled ntfyTopicUrl (ntfy_topic_url) and persists/sends it
(via _set_notification_settings and later _send_ntfy_notification), which allows
SSRF; instead validate and normalize this input by either (A) accepting only a
simple topic name and constructing the full ntfy URL server-side, or (B)
enforcing an allowlist of permitted ntfy origins and rejecting any URL whose
origin is not on that list; apply the same change to all places that read
ntfyTopicUrl (including the other sections referenced around lines 59–67 and
82–90), and ensure callers receive a validation error when the value is
missing/invalid rather than forwarding it to _send_ntfy_notification.
| normalized_path = _normalize_project_path(path) | ||
|
|
||
| if not os.path.isdir(normalized_path): | ||
| try: | ||
| os.makedirs(normalized_path, exist_ok=True) | ||
| except OSError as exc: | ||
| return jsonify( | ||
| {"error": f"Unable to create project folder: {exc}"} | ||
| ), 400 | ||
|
|
There was a problem hiding this comment.
Reject / as a valid project path here too.
sync_projects_from_opencode() already treats "/" as unsafe, but create_project() currently accepts it. A project rooted at / would let the rest of the project-scoped file/session APIs operate over the entire host filesystem instead of a single repo.
Suggested fix
normalized_path = _normalize_project_path(path)
+ if normalized_path == "/":
+ return jsonify({"error": "Project path cannot be /"}), 400
if not os.path.isdir(normalized_path):
try:
os.makedirs(normalized_path, exist_ok=True)📝 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.
| normalized_path = _normalize_project_path(path) | |
| if not os.path.isdir(normalized_path): | |
| try: | |
| os.makedirs(normalized_path, exist_ok=True) | |
| except OSError as exc: | |
| return jsonify( | |
| {"error": f"Unable to create project folder: {exc}"} | |
| ), 400 | |
| normalized_path = _normalize_project_path(path) | |
| if normalized_path == "/": | |
| return jsonify({"error": "Project path cannot be /"}), 400 | |
| if not os.path.isdir(normalized_path): | |
| try: | |
| os.makedirs(normalized_path, exist_ok=True) | |
| except OSError as exc: | |
| return jsonify( | |
| {"error": f"Unable to create project folder: {exc}"} | |
| ), 400 |
🤖 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/projects.py` around lines 176 - 185, Reject a root path
after normalization to prevent creating a project at filesystem root: after
calling _normalize_project_path(path) (and before trying os.path.isdir /
os.makedirs) add a check that normalized_path is not the system root (e.g.,
normalized_path == os.path.sep or normalized_path == "/") and if it is, return a
400 JSON error (same style as existing error responses). Update the
create_project route logic (the block using normalized_path and os.makedirs) so
it refuses "/" and any equivalent normalized root path before attempting
directory creation.
| export function apiGitDiff(projectId: string) { | ||
| return request<GitDiffResponse>(`/api/projects/${projectId}/git/diff`).then(r => r.diff); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify missing/used git diff types in api and definitions in shared types.
rg -n 'GitDiffResponse|GitDiffEntry|from "./types"' frontend/src/api.ts frontend/src/types.tsRepository: Rishabh-Bajpai/mobile-opencode-control
Length of output: 447
Import GitDiffResponse to fix a compile-time type error.
apiGitDiff uses GitDiffResponse but it is not imported from ./types, causing TypeScript compilation to fail.
Proposed fix
import type {
ChatMessage,
AppStateResponse,
OpenCodeCommand,
NotificationSettings,
Project,
ProjectSession,
ProjectRuntimeOptions,
ProjectSessionsResponse,
ProjectFileContent,
ProjectDirectoryListResponse,
ProjectsResponse,
QuestionRequest,
ScheduledTaskDetails,
ScheduledTaskMetrics,
ScheduledTaskRun,
ScheduledTask,
ProjectsSyncResponse,
SessionDiffEntry,
GitDiffEntry,
+ GitDiffResponse,
TimelineEvent,
PrdData,
PrdResponse,
} from "./types";📝 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.
| export function apiGitDiff(projectId: string) { | |
| return request<GitDiffResponse>(`/api/projects/${projectId}/git/diff`).then(r => r.diff); | |
| } | |
| import type { | |
| ChatMessage, | |
| AppStateResponse, | |
| OpenCodeCommand, | |
| NotificationSettings, | |
| Project, | |
| ProjectSession, | |
| ProjectRuntimeOptions, | |
| ProjectSessionsResponse, | |
| ProjectFileContent, | |
| ProjectDirectoryListResponse, | |
| ProjectsResponse, | |
| QuestionRequest, | |
| ScheduledTaskDetails, | |
| ScheduledTaskMetrics, | |
| ScheduledTaskRun, | |
| ScheduledTask, | |
| ProjectsSyncResponse, | |
| SessionDiffEntry, | |
| GitDiffEntry, | |
| GitDiffResponse, | |
| TimelineEvent, | |
| PrdData, | |
| PrdResponse, | |
| } from "./types"; | |
| export function apiGitDiff(projectId: string) { | |
| return request<GitDiffResponse>(`/api/projects/${projectId}/git/diff`).then(r => r.diff); | |
| } |
🤖 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/api.ts` around lines 680 - 682, The TypeScript compile error is
caused by using the GitDiffResponse type in apiGitDiff without importing it;
open frontend/src/api.ts and add an import for GitDiffResponse from './types'
(or add GitDiffResponse to the existing import statement) so the
apiGitDiff(projectId: string) function can reference the type correctly; ensure
the import name matches the exported symbol from ./types.
PR Review: #7 - Enhance project features, UI improvements, and documentation updatesBranch: OverviewThis is a large feature PR that modularizes the backend route architecture, adds significant new features (git diff viewer, question workflow, notifications, session compaction, voice controls), and refactors the frontend with extracted utility modules and new components. Architecture ChangesBackend Route Modularization
Frontend Component Extraction
Issues & Concerns1. Backend route module pattern uses globals (Medium)Each route module ( app = None
settings = None
opencode_client = None
scheduler = None
voice_runtime = None
def configure(app_instance, settings_instance, ...):
global app, settings, opencode_client, ...
app = app_instance
...Why it matters: Fragile under concurrent requests, makes testing harder, and is not idiomatic Flask. Recommendation: Use Flask's 2. Dynamic module loading in
|
| Category | Count |
|---|---|
| High | 1 (no tests) |
| Medium | 2 (globals pattern, API signature change) |
| Low-Medium | 1 (dynamic module loading) |
| Low | 4 (wildcard imports, in-memory patches, React keys, commit history) |
Recommendation: Address the globals pattern (#1) and add at least minimal tests (#5) before merging. The remaining items are improvements that can be done post-merge.
|
@copilot please review this PR, find issues and then address them and all comments to this PR (if required) |
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>
Addressed the actionable items in
Reviewed the PR and addressed the concrete issues in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Summary by CodeRabbit
New Features
Improvements