Add opinion monitor quickstart - #5
Conversation
|
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:
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 |
|
✅ Health: 8.0 🚨 Change risk: 9.9/10 (high)
🔥 Hotspots touched (5)
2 more
🔗 Hidden coupling (2 files)
💀 Dead code (6 findings)
3 more
👀 Suggested reviewers @xujinghua 📊 Full report · ⭐ Star Repowise · 📥 Install bot · Last updated 2026-07-06 19:27 UTC |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an 'Opinion Monitor' feature, adding backend endpoints for dashboard projection and preset application, frontend UI components for real-time visualization, and a PowerShell acceptance script. The code review identified several critical issues: a missing database commit in the preset application endpoint, hardcoded absolute paths in the acceptance script, a logic flaw where invalid cron expressions block data source creation, potential crashes in dashboard helpers due to missing type checks on AI enrichment data, potential type mismatches with record IDs, and duplicate React keys in the frontend.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| db.add(rule) | ||
| await db.flush() | ||
| await db.refresh(rule) |
There was a problem hiding this comment.
The endpoint apply_opinion_monitor_preset creates sources, schedules, and a notification rule, but never calls await db.commit(). In production, when the request finishes and the session is closed, all these newly created records will be rolled back and lost. Please ensure await db.commit() is called to persist the changes.
| db.add(rule) | |
| await db.flush() | |
| await db.refresh(rule) | |
| db.add(rule) | |
| await db.flush() | |
| await db.refresh(rule) | |
| await db.commit() |
| Run-RegressionGate -Gate "regression-sentrux" -Label "sentrux-check-rules" -FilePath $PwshExe -Arguments @( | ||
| "-NoProfile", "-ExecutionPolicy", "Bypass", | ||
| "-File", "C:\c\Users\Administrator\projects\code-intel-pipeline\Invoke-SentruxAgentTool.ps1", | ||
| "check_rules", $RepoRoot | ||
| ) -TimeoutSeconds $RegressionTimeoutSeconds -Hard $true | Out-Null | ||
|
|
||
| if (-not $SkipCodeIntel) { | ||
| Run-RegressionGate -Gate "regression-code-intel-doctor" -Label "code-intel-doctor" -FilePath $PwshExe -Arguments @( | ||
| "-NoProfile", "-ExecutionPolicy", "Bypass", | ||
| "-File", "C:\c\Users\Administrator\projects\code-intel-pipeline\check-code-intel-tools.ps1", | ||
| "-RepoPath", $RepoRoot, | ||
| "-RequireRepowise", | ||
| "-Json" | ||
| ) -TimeoutSeconds $RegressionTimeoutSeconds -Hard $true | Out-Null | ||
|
|
||
| Run-RegressionGate -Gate "regression-code-intel-normal" -Label "code-intel-normal" -FilePath $PwshExe -Arguments @( | ||
| "-NoProfile", "-ExecutionPolicy", "Bypass", | ||
| "-File", "C:\c\Users\Administrator\projects\code-intel-pipeline\invoke-code-intel.ps1", | ||
| "-RepoPath", $RepoRoot, | ||
| "-Mode", "normal" | ||
| ) -TimeoutSeconds $RegressionTimeoutSeconds -Hard $true -KnownDebtRegex "graph_missing|Understand graph: False|baseline_missing|rules_missing|known debt|known_debt|Sentrux fail|sentrux_fail|Sentrux gate|Blocking Sentrux debt|worsened_debt|god_files|Quality degraded during this session" | Out-Null |
There was a problem hiding this comment.
The script hardcodes absolute paths to a specific user's directory (C:\c\Users\Administrator\projects\code-intel-pipeline\...) for executing regression tools. This makes the script non-portable, and it will fail on any other developer's machine or standard CI environment. Consider using an environment variable with a fallback to make it configurable.
$PipelinePath = $env:CODE_INTEL_PIPELINE_PATH
if (-not $PipelinePath) {
$PipelinePath = "C:\\c\\Users\\Administrator\\projects\\code-intel-pipeline"
}
Run-RegressionGate -Gate "regression-sentrux" -Label "sentrux-check-rules" -FilePath $PwshExe -Arguments @(
"-NoProfile", "-ExecutionPolicy", "Bypass",
"-File", (Join-Path $PipelinePath "Invoke-SentruxAgentTool.ps1"),
"check_rules", $RepoRoot
) -TimeoutSeconds $RegressionTimeoutSeconds -Hard $true | Out-Null
if (-not $SkipCodeIntel) {
Run-RegressionGate -Gate "regression-code-intel-doctor" -Label "code-intel-doctor" -FilePath $PwshExe -Arguments @(
"-NoProfile", "-ExecutionPolicy", "Bypass",
"-File", (Join-Path $PipelinePath "check-code-intel-tools.ps1"),
"-RepoPath", $RepoRoot,
"-RequireRepowise",
"-Json"
) -TimeoutSeconds $RegressionTimeoutSeconds -Hard $true | Out-Null
Run-RegressionGate -Gate "regression-code-intel-normal" -Label "code-intel-normal" -FilePath $PwshExe -Arguments @(
"-NoProfile", "-ExecutionPolicy", "Bypass",
"-File", (Join-Path $PipelinePath "invoke-code-intel.ps1"),
"-RepoPath", $RepoRoot,
"-Mode", "normal"
) -TimeoutSeconds $RegressionTimeoutSeconds -Hard $true -KnownDebtRegex "graph_missing|Understand graph: False|baseline_missing|rules_missing|known debt|known_debt|Sentrux fail|sentrux_fail|Sentrux gate|Blocking Sentrux debt|worsened_debt|god_files|Quality degraded during this session" | Out-Null
| for slot in body.account_slots: | ||
| if body.create_schedules and not schedule_service.validate_cron_expression( | ||
| slot.cron_expression | ||
| ): | ||
| warnings.append( | ||
| { | ||
| "slot": slot.label, | ||
| "warning": "invalid_cron_expression", | ||
| "cron_expression": slot.cron_expression, | ||
| } | ||
| ) | ||
| continue | ||
|
|
||
| source_data = DataSourceCreate(**_opinion_source_payload(body, slot)) | ||
| source = await source_service.create_source(db, source_data) | ||
| created_sources.append( | ||
| { | ||
| "id": source.id, | ||
| "name": source.name, | ||
| "site": slot.site, | ||
| "command": slot.command, | ||
| "account_label": slot.label, | ||
| } | ||
| ) | ||
|
|
||
| if body.create_schedules: | ||
| schedule = await schedule_service.create_schedule( | ||
| db, | ||
| CronScheduleCreate( | ||
| source_id=source.id, | ||
| name=f"{source.name} · 定时采集", | ||
| cron_expression=slot.cron_expression, | ||
| timezone=slot.timezone, | ||
| parameters={"limit": slot.limit}, | ||
| enabled=body.schedule_enabled, | ||
| ), | ||
| ) | ||
| created_schedules.append( | ||
| { | ||
| "id": schedule.id, | ||
| "source_id": source.id, | ||
| "cron_expression": schedule.cron_expression, | ||
| "timezone": schedule.timezone, | ||
| "enabled": schedule.enabled, | ||
| } | ||
| ) |
There was a problem hiding this comment.
If body.create_schedules is True and a slot has an invalid cron expression, the loop continues, which skips creating the DataSource for that slot entirely. An invalid cron expression should only prevent the creation of the schedule, not the data source itself. Consider creating the source anyway and appending a warning about the skipped schedule.
for slot in body.account_slots:
source_data = DataSourceCreate(**_opinion_source_payload(body, slot))
source = await source_service.create_source(db, source_data)
created_sources.append(
{
"id": source.id,
"name": source.name,
"site": slot.site,
"command": slot.command,
"account_label": slot.label,
}
)
if body.create_schedules:
if not schedule_service.validate_cron_expression(slot.cron_expression):
warnings.append(
{
"slot": slot.label,
"warning": "invalid_cron_expression",
"cron_expression": slot.cron_expression,
"detail": f"Source '{source.name}' was created, but its schedule was skipped due to an invalid cron expression.",
}
)
else:
schedule = await schedule_service.create_schedule(
db,
CronScheduleCreate(
source_id=source.id,
name=f"{source.name} · 定时采集",
cron_expression=slot.cron_expression,
timezone=slot.timezone,
parameters={"limit": slot.limit},
enabled=body.schedule_enabled,
),
)
created_schedules.append(
{
"id": schedule.id,
"source_id": source.id,
"cron_expression": schedule.cron_expression,
"timezone": schedule.timezone,
"enabled": schedule.enabled,
}
)| def _summary_from_ai(ai: dict[str, Any] | None) -> str: | ||
| if not ai: | ||
| return "" |
There was a problem hiding this comment.
The helper function _summary_from_ai assumes that ai is always a dictionary or None. However, ai_enrichment is a JSON column and could contain other types (like a raw string if the LLM output failed to parse as JSON). Calling .get() on a non-dict object will raise an AttributeError and crash the dashboard. Adding a type check ensures robust and defensive handling.
| def _summary_from_ai(ai: dict[str, Any] | None) -> str: | |
| if not ai: | |
| return "" | |
| def _summary_from_ai(ai: dict[str, Any] | None) -> str: | |
| if not isinstance(ai, dict): | |
| return "" |
| def _tags_from_ai(ai: dict[str, Any] | None) -> list[str]: | ||
| if not ai: | ||
| return [] |
There was a problem hiding this comment.
The helper function _tags_from_ai assumes that ai is always a dictionary or None. However, ai_enrichment is a JSON column and could contain other types (like a raw string if the LLM output failed to parse as JSON). Calling .get() on a non-dict object will raise an AttributeError and crash the dashboard. Adding a type check ensures robust and defensive handling.
| def _tags_from_ai(ai: dict[str, Any] | None) -> list[str]: | |
| if not ai: | |
| return [] | |
| def _tags_from_ai(ai: dict[str, Any] | None) -> list[str]: | |
| if not isinstance(ai, dict): | |
| return [] |
| def _sentiment_from_ai(ai: dict[str, Any] | None) -> str: | ||
| if not ai: | ||
| return "unknown" |
There was a problem hiding this comment.
The helper function _sentiment_from_ai assumes that ai is always a dictionary or None. However, ai_enrichment is a JSON column and could contain other types (like a raw string if the LLM output failed to parse as JSON). Calling .get() on a non-dict object will raise an AttributeError and crash the dashboard. Adding a type check ensures robust and defensive handling.
| def _sentiment_from_ai(ai: dict[str, Any] | None) -> str: | |
| if not ai: | |
| return "unknown" | |
| def _sentiment_from_ai(ai: dict[str, Any] | None) -> str: | |
| if not isinstance(ai, dict): | |
| return "unknown" |
| for record_id, status, notifier_type in notification_rows.all(): | ||
| if record_id and notifier_type == "feishu": | ||
| notification_by_record[record_id][status] += 1 |
There was a problem hiding this comment.
notification_by_record uses record_id as keys, which might be UUID objects or integers depending on the database dialect and model definition. Later, it queries this dictionary using record.id (which might be a string). To prevent type mismatches (e.g., UUID object vs. string), all keys should be consistently converted to strings using str().
| for record_id, status, notifier_type in notification_rows.all(): | |
| if record_id and notifier_type == "feishu": | |
| notification_by_record[record_id][status] += 1 | |
| for record_id, status, notifier_type in notification_rows.all(): | |
| if record_id and notifier_type == "feishu": | |
| notification_by_record[str(record_id)][status] += 1 |
| if record.ai_enrichment: | ||
| source_bucket["ai_processed"] += 1 | ||
|
|
||
| notify_counts = notification_by_record.get(record.id, {}) |
There was a problem hiding this comment.
To ensure consistency with the string keys used in notification_by_record, convert record.id to a string when retrieving notification counts.
| notify_counts = notification_by_record.get(record.id, {}) | |
| notify_counts = notification_by_record.get(str(record.id), {}) |
| {[...topTags, ...topSentiment].slice(0, 7).map((item) => ( | ||
| <Badge key={`${item.label}-${item.count}`} variant="secondary"> | ||
| {item.label} · {item.count} | ||
| </Badge> | ||
| ))} |
There was a problem hiding this comment.
The combined list [...topTags, ...topSentiment] is mapped to render badges using key={${item.label}-${item.count}}. Since a tag and a sentiment can have the same label (e.g., "positive") and count, this can easily result in duplicate keys, causing React rendering warnings and potential UI bugs. Appending the index to the key guarantees uniqueness.
| {[...topTags, ...topSentiment].slice(0, 7).map((item) => ( | |
| <Badge key={`${item.label}-${item.count}`} variant="secondary"> | |
| {item.label} · {item.count} | |
| </Badge> | |
| ))} | |
| {[...topTags, ...topSentiment].slice(0, 7).map((item, index) => ( | |
| <Badge key={`${item.label}-${item.count}-${index}`} variant="secondary"> | |
| {item.label} · {item.count} | |
| </Badge> | |
| ))} |
…ckManifest schema (PR-A) Vendor github.com/browser-act/skills @a23131e solutions/ verbatim into backend/browser_act_packs/ (78 packs, MIT, LICENSE + VENDOR.md attribution). Packs = upstream SKILL.md + scripts/*.py, byte-unchanged; channel.manifest.json is our addition (PR-D). - backend/browser_act_packs/catalog.py: PackCatalog scans <category>/<pack>/SKILL.md, parses YAML frontmatter (name/description), derives domain=category / capability=pack-dir; reads utf-8-sig (one pack ships a BOM); missing/broken frontmatter skipped with a warning, never crashes. - backend/browser_act_packs/manifest.py: PackManifest schema (param_schema/steps/ pagination/success per GOAL-7 #5) + load_manifest. Schema only; no manifest content yet. - tests/unit/browser_act_packs/: 18 tests (catalog scan >=20, BOM/bad-frontmatter skip, get_pack, manifest validate/reject). Isolated from the DB-backed backend/skills subsystem (different concept). 1430 -> 1448 passed, zero regression.
2 participants
Summary
Verification
.\.venv\Scripts\python.exe -m ruff check backend/api/v1/presets.py backend/api/v1/dashboard.py backend/notifiers/feishu_notifier.py backend/pipeline/runner.py backend/agent_server.py backend/ws_agent_manager.py tests/integration/test_presets_api.py tests/integration/test_dashboard_api.py tests/unit/test_messaging_notifiers.py.\.venv\Scripts\python.exe -m pytest -q --no-cov tests/integration/test_presets_api.py tests/integration/test_dashboard_api.py tests/unit/test_messaging_notifiers.py tests/integration/test_workflow_fleet_api.py tests/integration/test_opencli_channel_api.py::test_collect_agent_mode_prefers_site_bound_agent tests/unit/channels/test_opencli_channel.pynpm run typecheck:frontendnpm run build:frontendInvoke-SentruxAgentTool.ps1 check_rules C:\c\Users\Administrator\projects\opencli-admin-backendscripts/acceptance/fleet-acceptance.ps1Notes
lunnynight/opencli-adminwas rejected withpermission denied, so this PR is opened on the accessible2233admin/opencli-adminfork branch.