Repository navigation
feat(wework): add conversation search navigation - #1257
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds an end-to-end WeWork-scoped conversation search: backend search adapter and GET /wework/conversation-search endpoint; frontend task API, a DesktopSearchDialog merging local/remote results with lazy detail fetching; sidebar/layout/provider wiring; tests; and i18n strings. ChangesTask Search Feature
Sequence DiagramsequenceDiagram
participant User as User
participant SearchDialog as DesktopSearchDialog
participant RemoteAPI as searchTasks API
participant DetailAPI as searchTaskDetail API
participant Sidebar as DesktopSidebar
User->>SearchDialog: Opens dialog and types keyword
SearchDialog->>SearchDialog: Filter local projects/recent tasks
alt Remote search available
SearchDialog->>RemoteAPI: Debounced searchTasks(keyword,page,limit)
RemoteAPI-->>SearchDialog: TaskListResponse
SearchDialog->>SearchDialog: Merge remote + local by taskId
end
SearchDialog->>DetailAPI: searchTaskDetail(taskId) for missing items (parallel)
DetailAPI-->>SearchDialog: TaskDetail with searchable text
User->>SearchDialog: Clicks result
SearchDialog->>Sidebar: onOpenTask(taskId, projectId)
Sidebar->>Sidebar: Expand project and scroll row into view
SearchDialog->>SearchDialog: Close dialog
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wework/src/components/layout/DesktopWorkbenchLayout.test.tsx (1)
568-616: 💤 Low valueTest name doesn't match its behavior.
This test never opens the search dialog (no
search-buttonclick); it only renders withcurrentTaskset and asserts the auto-expand/scroll-into-view effect. The phrase "after opening from search" is misleading for future maintainers — consider renaming to reflect that it verifies the current-project task is expanded/highlighted/scrolled on render.♻️ Suggested rename
- test('expands and scrolls to the current project task after opening from search', async () => { + test('expands, highlights, and scrolls to the current project task on render', async () => {🤖 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 `@wework/src/components/layout/DesktopWorkbenchLayout.test.tsx` around lines 568 - 616, The test named "expands and scrolls to the current project task after opening from search" is misleading because it never clicks the search button; update the test name to reflect that it verifies automatic expansion/highlighting/scrolling on render (e.g., "expands and scrolls to the current project task on render when currentTask is set"), and ensure references to DesktopWorkbenchLayout's state props (currentTask/currentProject/projects), the queried test id `project-chat-row-{task_id}`, and the `search-button` are not implied; leave the test logic that asserts Element.prototype.scrollIntoView was called and the row has the active class and correct text intact.
🤖 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 `@wework/src/pages/WorkbenchPage.tsx`:
- Around line 79-80: Layout is being rendered with
onSearchTasks/onSearchTaskDetail from WorkbenchPage
(searchTasks/searchTaskDetail) but MobileWorkbenchLayoutProps doesn't declare
them; update MobileWorkbenchLayoutProps to include optional onSearchTasks and
onSearchTaskDetail callback signatures and propagate those props through the
MobileWorkbenchLayout component (accept props in MobileWorkbenchLayout and pass
them to the internal handler or child that needs them), or alternatively render
those props only when Layout resolves to the desktop layout. Modify
MobileWorkbenchLayoutProps and the MobileWorkbenchLayout component to accept and
forward onSearchTasks/onSearchTaskDetail (matching the types used by
WorkbenchPage: searchTasks/searchTaskDetail).
---
Nitpick comments:
In `@wework/src/components/layout/DesktopWorkbenchLayout.test.tsx`:
- Around line 568-616: The test named "expands and scrolls to the current
project task after opening from search" is misleading because it never clicks
the search button; update the test name to reflect that it verifies automatic
expansion/highlighting/scrolling on render (e.g., "expands and scrolls to the
current project task on render when currentTask is set"), and ensure references
to DesktopWorkbenchLayout's state props (currentTask/currentProject/projects),
the queried test id `project-chat-row-{task_id}`, and the `search-button` are
not implied; leave the test logic that asserts Element.prototype.scrollIntoView
was called and the row has the active class and correct text intact.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fac0066b-0ac8-4ece-8cdd-747a351d0e5b
📒 Files selected for processing (17)
backend/app/api/endpoints/adapter/tasks.pybackend/app/services/adapters/task_kinds/queries.pybackend/tests/services/adapters/test_task_archive_operations.pydocs/en/user-guide/chat/managing-tasks.mddocs/zh/user-guide/chat/managing-tasks.mdfrontend/src/__tests__/contexts/SocketContext.reconnect.test.tsxfrontend/src/__tests__/features/resource-library/ResourceCreateButton.test.tsxwework/src/api/adapters.test.tswework/src/api/tasks.tswework/src/components/layout/DesktopSearchDialog.tsxwework/src/components/layout/DesktopSidebar.tsxwework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/layout/DesktopWorkbenchLayout.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.jsonwework/src/pages/WorkbenchPage.tsx
| onSearchTasks={searchTasks} | ||
| onSearchTaskDetail={searchTaskDetail} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether MobileWorkbenchLayout declares the new search props
fd -i 'MobileWorkbenchLayout.tsx' wework/src --exec sh -c 'echo "== {} =="; rg -nP -C2 "onSearchTasks|onSearchTaskDetail|interface .*LayoutProps" "{}"'Repository: wecode-ai/Wegent
Length of output: 277
Fix prop mismatch: wire or conditionally pass onSearchTasks / onSearchTaskDetail on mobile
wework/src/pages/WorkbenchPage.tsx renders Layout with onSearchTasks and onSearchTaskDetail (lines 79-80), but wework/src/components/layout/MobileWorkbenchLayout.tsx’s MobileWorkbenchLayoutProps does not declare these props. Add them to the mobile layout props (and pass them through) or only pass these callbacks when rendering the desktop layout.
🤖 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 `@wework/src/pages/WorkbenchPage.tsx` around lines 79 - 80, Layout is being
rendered with onSearchTasks/onSearchTaskDetail from WorkbenchPage
(searchTasks/searchTaskDetail) but MobileWorkbenchLayoutProps doesn't declare
them; update MobileWorkbenchLayoutProps to include optional onSearchTasks and
onSearchTaskDetail callback signatures and propagate those props through the
MobileWorkbenchLayout component (accept props in MobileWorkbenchLayout and pass
them to the internal handler or child that needs them), or alternatively render
those props only when Layout resolves to the desktop layout. Modify
MobileWorkbenchLayoutProps and the MobileWorkbenchLayout component to accept and
forward onSearchTasks/onSearchTaskDetail (matching the types used by
WorkbenchPage: searchTasks/searchTaskDetail).
c225ca6 to
108ddfe
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/app/services/adapters/wework_conversation_search.py (1)
119-139: Unbounded task load on the request path may not scale.
_load_visible_wework_tasksfetches every active WeWork task for the user via.all()with no cap, and_match_task_titlesthen validates each task's JSON in Python (a secondTask.model_validatepass also happens in_format_task_results). For users with large histories this loads and parses the full task set on every search. Consider pushing the title match into SQL (JSON field access, as the other lite queries do) and bounding the candidate set, or caching the per-taskTaskparse to avoid double validation.🤖 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/services/adapters/wework_conversation_search.py` around lines 119 - 139, _load_visible_wework_tasks currently loads all TaskResource rows then filters and causes repeated Task.model_validate passes in _match_task_titles and _format_task_results; change it to push the title-match into the DB query (use JSON/->>'title' access on TaskResource.model or the DB-specific JSON path used elsewhere) and add a bounded candidate set (e.g. .limit(n) with sensible page size and order_by TaskResource.updated_at.desc()) so we only load a finite recent subset, and additionally introduce an in-memory cache of parsed Task objects (keyed by TaskResource.id) used by _match_task_titles and _format_task_results to avoid double Task.model_validate for the same resource.
🤖 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/services/adapters/wework_conversation_search.py`:
- Around line 168-187: In _match_subtasks_in_database: escape SQL LIKE wildcards
in the incoming keyword before building like_pattern by first replacing
backslashes with double backslashes then prefixing any '%' and '_' with a
backslash (or chosen escape char), build like_pattern = f"%{escaped_keyword}%",
and pass the same escape char into the ilike calls (e.g., .ilike(like_pattern,
escape='\\')) for Subtask.prompt, Subtask.error_message and cast(Subtask.result,
String) so user input is treated literally.
- Around line 44-53: This function is a public service entrypoint that lacks
OpenTelemetry tracing; add the tracing decorator `@trace_sync` to both
search_wework_conversation_tasks and the new
search_wework_conversation_task_list (they are synchronous), and also add the
corresponding import for trace_sync at the top of the module so the decorator is
available. Ensure the decorator wraps the entire function definition (above def)
so the entrypoints are traced end-to-end.
- Around line 66-116: The current bug is that _collect_matched_task_ids() gates
calling _match_subtasks_with_python_fallback() on a page-dependent
required_count (skip+limit), causing matched_task_ids/total to differ between
pages; fix by making the fallback deterministic and skip-independent: introduce
a fixed SEARCH_RESULT_FALLBACK_TASK_LIMIT constant (or hardcap value) and always
run _match_subtasks_with_python_fallback() against
task_ids[:SEARCH_RESULT_FALLBACK_TASK_LIMIT] (or pass that cap into the
function) to extend matched_task_ids, then compute ordered_matched_task_ids from
the full task_ids using that final matched_task_ids so total and paging are
stable; update the _collect_matched_task_ids() logic to remove the
skip-dependent gating and reference _match_subtasks_with_python_fallback,
_match_subtasks_in_database, _match_task_titles, and restore_task_order
accordingly.
---
Nitpick comments:
In `@backend/app/services/adapters/wework_conversation_search.py`:
- Around line 119-139: _load_visible_wework_tasks currently loads all
TaskResource rows then filters and causes repeated Task.model_validate passes in
_match_task_titles and _format_task_results; change it to push the title-match
into the DB query (use JSON/->>'title' access on TaskResource.model or the
DB-specific JSON path used elsewhere) and add a bounded candidate set (e.g.
.limit(n) with sensible page size and order_by TaskResource.updated_at.desc())
so we only load a finite recent subset, and additionally introduce an in-memory
cache of parsed Task objects (keyed by TaskResource.id) used by
_match_task_titles and _format_task_results to avoid double Task.model_validate
for the same resource.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16376d52-4fe0-4145-8fda-eb29e9597199
📒 Files selected for processing (13)
backend/app/api/endpoints/adapter/tasks.pybackend/app/services/adapters/wework_conversation_search.pybackend/tests/services/adapters/test_task_archive_operations.pywework/src/api/adapters.test.tswework/src/api/tasks.tswework/src/components/layout/DesktopSearchDialog.tsxwework/src/components/layout/DesktopSidebar.tsxwework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/layout/DesktopWorkbenchLayout.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.jsonwework/src/pages/WorkbenchPage.tsx
✅ Files skipped from review due to trivial changes (1)
- wework/src/i18n/locales/zh-CN/common.json
🚧 Files skipped from review as they are similar to previous changes (10)
- wework/src/api/tasks.ts
- wework/src/api/adapters.test.ts
- wework/src/components/layout/DesktopSearchDialog.tsx
- wework/src/i18n/locales/en/common.json
- wework/src/components/layout/DesktopWorkbenchLayout.tsx
- backend/tests/services/adapters/test_task_archive_operations.py
- wework/src/pages/WorkbenchPage.tsx
- wework/src/features/workbench/WorkbenchProvider.tsx
- wework/src/components/layout/DesktopSidebar.tsx
- wework/src/components/layout/DesktopWorkbenchLayout.test.tsx
| def search_wework_conversation_tasks( | ||
| *, | ||
| db: Session, | ||
| user_id: int, | ||
| keyword: str, | ||
| skip: int = 0, | ||
| limit: int = 100, | ||
| client_origin: str = CLIENT_ORIGIN_WEWORK, | ||
| ) -> Tuple[List[Dict[str, Any]], int]: | ||
| """Search WeWork-owned tasks by title or conversation content.""" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add an OpenTelemetry tracing decorator to this entrypoint.
search_wework_conversation_tasks is a public service entrypoint but is not traced. The new endpoint search_wework_conversation_task_list is likewise untraced. As per coding guidelines: "Use OpenTelemetry tracing decorators: @trace_async or @trace_sync for entire functions".
Proposed change
+from shared.telemetry.decorators import trace_sync
+
+@trace_sync("search_wework_conversation_tasks", "tasks.search")
def search_wework_conversation_tasks(
*,
db: Session,🤖 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/services/adapters/wework_conversation_search.py` around lines 44
- 53, This function is a public service entrypoint that lacks OpenTelemetry
tracing; add the tracing decorator `@trace_sync` to both
search_wework_conversation_tasks and the new
search_wework_conversation_task_list (they are synchronous), and also add the
corresponding import for trace_sync at the top of the module so the decorator is
available. Ensure the decorator wraps the entire function definition (above def)
so the entrypoints are traced end-to-end.
| matched_task_ids, ordered_matched_task_ids = _collect_matched_task_ids( | ||
| db=db, | ||
| task_ids=task_ids, | ||
| visible_task_by_id=visible_task_by_id, | ||
| keyword=keyword, | ||
| required_count=skip + limit, | ||
| ) | ||
| id_to_task = { | ||
| task_id: visible_task_by_id[task_id] | ||
| for task_id in matched_task_ids | ||
| if task_id in visible_task_by_id | ||
| } | ||
| total = len(id_to_task) | ||
| filtered_tasks = restore_task_order( | ||
| ordered_matched_task_ids[skip : skip + limit], id_to_task, limit | ||
| ) | ||
| if not filtered_tasks: | ||
| return [], total | ||
|
|
||
| return _format_task_results(db, filtered_tasks, user_id), total | ||
|
|
||
|
|
||
| def _collect_matched_task_ids( | ||
| *, | ||
| db: Session, | ||
| task_ids: List[int], | ||
| visible_task_by_id: Dict[int, TaskResource], | ||
| keyword: str, | ||
| required_count: int, | ||
| ) -> Tuple[set[int], List[int]]: | ||
| keyword_lower = keyword.lower() | ||
| matched_task_ids = _match_task_titles(visible_task_by_id, keyword_lower) | ||
| matched_task_ids.update(_match_subtasks_in_database(db, task_ids, keyword)) | ||
| ordered_matched_task_ids = [ | ||
| task_id for task_id in task_ids if task_id in matched_task_ids | ||
| ] | ||
|
|
||
| if len(ordered_matched_task_ids) < required_count: | ||
| matched_task_ids.update( | ||
| _match_subtasks_with_python_fallback( | ||
| db=db, | ||
| task_ids=task_ids, | ||
| matched_task_ids=matched_task_ids, | ||
| keyword_lower=keyword_lower, | ||
| ) | ||
| ) | ||
| ordered_matched_task_ids = [ | ||
| task_id for task_id in task_ids if task_id in matched_task_ids | ||
| ] | ||
|
|
||
| return matched_task_ids, ordered_matched_task_ids |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep --pattern 'def restore_task_order($$$):
$$$'
rg -nP --type=py -C3 '\brestore_task_order\b'Repository: wecode-ai/Wegent
Length of output: 6303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant part of wework_conversation_search.py (around the snippet)
sed -n '1,140p' backend/app/services/adapters/wework_conversation_search.py
# Show the fallback/matching helpers referenced in the snippet
rg -n "def _collect_matched_task_ids|_match_subtasks_in_database|_match_subtasks_with_python_fallback|def _match_task_titles" backend/app/services/adapters/wework_conversation_search.py
sed -n '140,320p' backend/app/services/adapters/wework_conversation_search.py
# Inspect restore_task_order implementation
sed -n '360,460p' backend/app/services/adapters/task_kinds/query_utils.pyRepository: wecode-ai/Wegent
Length of output: 8141
Stabilize pagination by making the matched-task set (and total) independent of skip
_collect_matched_task_ids()runs_match_subtasks_with_python_fallback()only whenlen(ordered_matched_task_ids) < (skip + limit), somatched_task_ids(and thereforetotal = len(id_to_task)) changes between page 1 and page 2.- Since
search_wework_conversation_tasks()slicesordered_matched_task_ids[skip : skip + limit]using that page-dependent matched set, tasks can be duplicated or skipped across pages when fallback gets added on later page requests. restore_task_order()does not reorder—it only projects the providedtask_idsslice in order untillimit, so the instability originates from the fallback gating.
Compute fallback/matched IDs once using a skip-independent criterion (e.g., always apply fallback for the same bounded ID set like task_ids[:SEARCH_RESULT_FALLBACK_TASK_LIMIT], or gate on a fixed cap not based on page offset) and derive both ordered_matched_task_ids and total from that same final matched set.
🤖 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/services/adapters/wework_conversation_search.py` around lines 66
- 116, The current bug is that _collect_matched_task_ids() gates calling
_match_subtasks_with_python_fallback() on a page-dependent required_count
(skip+limit), causing matched_task_ids/total to differ between pages; fix by
making the fallback deterministic and skip-independent: introduce a fixed
SEARCH_RESULT_FALLBACK_TASK_LIMIT constant (or hardcap value) and always run
_match_subtasks_with_python_fallback() against
task_ids[:SEARCH_RESULT_FALLBACK_TASK_LIMIT] (or pass that cap into the
function) to extend matched_task_ids, then compute ordered_matched_task_ids from
the full task_ids using that final matched_task_ids so total and paging are
stable; update the _collect_matched_task_ids() logic to remove the
skip-dependent gating and reference _match_subtasks_with_python_fallback,
_match_subtasks_in_database, _match_task_titles, and restore_task_order
accordingly.
| def _match_subtasks_in_database( | ||
| db: Session, | ||
| task_ids: List[int], | ||
| keyword: str, | ||
| ) -> set[int]: | ||
| like_pattern = f"%{keyword}%" | ||
| rows = ( | ||
| db.query(Subtask.task_id) | ||
| .filter(Subtask.task_id.in_(task_ids)) | ||
| .filter( | ||
| or_( | ||
| Subtask.prompt.ilike(like_pattern), | ||
| Subtask.error_message.ilike(like_pattern), | ||
| cast(Subtask.result, String).ilike(like_pattern), | ||
| ) | ||
| ) | ||
| .distinct() | ||
| .all() | ||
| ) | ||
| return {row[0] for row in rows} |
There was a problem hiding this comment.
Escape LIKE wildcards in the keyword.
% and _ in the user keyword are treated as wildcards inside ilike, so a query like 100% or a_b matches more broadly than the literal text the user typed. Escape them (and set an escape char) before interpolating into like_pattern.
Proposed change
- like_pattern = f"%{keyword}%"
+ escaped = keyword.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_")
+ like_pattern = f"%{escaped}%"
rows = (
db.query(Subtask.task_id)
.filter(Subtask.task_id.in_(task_ids))
.filter(
or_(
- Subtask.prompt.ilike(like_pattern),
- Subtask.error_message.ilike(like_pattern),
- cast(Subtask.result, String).ilike(like_pattern),
+ Subtask.prompt.ilike(like_pattern, escape="\\"),
+ Subtask.error_message.ilike(like_pattern, escape="\\"),
+ cast(Subtask.result, String).ilike(like_pattern, escape="\\"),
)
)🤖 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/services/adapters/wework_conversation_search.py` around lines 168
- 187, In _match_subtasks_in_database: escape SQL LIKE wildcards in the incoming
keyword before building like_pattern by first replacing backslashes with double
backslashes then prefixing any '%' and '_' with a backslash (or chosen escape
char), build like_pattern = f"%{escaped_keyword}%", and pass the same escape
char into the ilike calls (e.g., .ilike(like_pattern, escape='\\')) for
Subtask.prompt, Subtask.error_message and cast(Subtask.result, String) so user
input is treated literally.
Summary by CodeRabbit
New Features
Tests
Documentation / Localization