fix(web): OSearch Favorites i18n, first-frame race, and edit - #268
Conversation
- Extract UserFilter and Favorites menu msgids into web.pot / zh_CN.po. - Provide Chinese msgstr for Favorites, save/share/delete dialogs, and model fields. Co-authored-by: Cursor <cursoragent@cursor.com>
- Drop mount-time defaults-ready so OSearchView IsDefault load is not superseded. - Defer OListView mount apply when searchView is OSearchView, matching Kanban. - Guard useUserFilters.load with a generation so stale responses cannot overwrite. Co-authored-by: Cursor <cursoragent@cursor.com>
- Let OSearch own the single Search and emit defaults-ready after load. - Drop OSearchView's duplicate IsDefault Search; wait on defaults-ready for first-frame emit. Co-authored-by: Cursor <cursoragent@cursor.com>
- Add useUserFilters.updateMeta via UpdateById without rewriting Condition. - Reuse the save dialog for edit mode with a pencil entry next to delete. - Extract and translate Edit favorite strings in zh_CN. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis change generalizes first-frame deferral to List and Kanban views, moves search-default synchronization into ChangesSearch view updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OSearchView
participant OSearch
participant useUserFilters
participant UserFilterAPI
OSearchView->>OSearch: pass codeDefaultFilters
OSearch->>useUserFilters: load favorites
useUserFilters->>UserFilterAPI: request favorites
UserFilterAPI-->>useUserFilters: return favorites
useUserFilters-->>OSearch: emit defaults-ready
OSearch-->>OSearchView: provide merged defaults
OSearchView-->>OSearchView: emit initial query-update once
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
- Add Created By / Updated By / Deleted By to core.pot and zh_CN.po. - Unblock CI i18n status pot-dirty after audit uid columns landed. Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/web/web/composables/search/useUserFilters.ts (1)
56-66: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear request state for an invalid load context.
At Line 60, this branch invalidates any earlier request but leaves
loadingtrue. The stale request cannot clear it at Lines 116-118. The favorites menu can then show a permanent loading state. ClearloadingandloadErrorwithfavoritesbefore returning. Add a regression test that starts a load, then removes the application or model before the first request resolves.Proposed fix
if (!app || !model) { if (gen === loadGeneration) { favorites.value = []; + loading.value = false; + loadError.value = null; } return; }🤖 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 `@modules/web/web/composables/search/useUserFilters.ts` around lines 56 - 66, Update the invalid-context branch in the load function around loadGeneration to clear loading and loadError alongside favorites before returning, while retaining the generation guard. Add a regression test that starts a load, removes the application or model before the request resolves, and verifies the loading state is cleared.
🤖 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 `@modules/web/web/components/view/OListView.firstframe.test.ts`:
- Around line 45-48: Update the kanbanFirstFrame mock’s
shouldDeferViewFirstFrame and shouldDeferKanbanFirstFrame implementations to
honor their searchView and OSearchView arguments, matching production behavior
by deferring only when the references are identical and the configured defer
state applies. Preserve the existing deferState-driven behavior for matching
references so SearchStub and custom search views exercise the correct fallback
mount path.
In `@modules/web/web/components/view/OSearchView.vue`:
- Around line 111-120: Prevent duplicate first-frame emissions in
emitFirstFrameIfNeeded by rechecking mounted.value after nextTick or setting a
pending latch before awaiting, so concurrent defaults-ready events produce only
one query-update. In modules/web/web/components/view/OSearchView.test.ts lines
142-155, emit defaults-ready twice before awaiting Vue updates and assert
exactly one query-update; both sites require changes.
---
Outside diff comments:
In `@modules/web/web/composables/search/useUserFilters.ts`:
- Around line 56-66: Update the invalid-context branch in the load function
around loadGeneration to clear loading and loadError alongside favorites before
returning, while retaining the generation guard. Add a regression test that
starts a load, removes the application or model before the request resolves, and
verifies the loading state is cleared.
🪄 Autofix
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: da0cd229-6719-44d5-a150-4eaa836b8d39
📒 Files selected for processing (16)
modules/web/i18n/web.potmodules/web/i18n/zh_CN.pomodules/web/web/components/view/OKanbanView.firstframe.test.tsmodules/web/web/components/view/OKanbanView.readonly.test.tsmodules/web/web/components/view/OKanbanView.vuemodules/web/web/components/view/OListView.firstframe.test.tsmodules/web/web/components/view/OListView.vuemodules/web/web/components/view/OSearchView.scopeKey.test.tsmodules/web/web/components/view/OSearchView.test.tsmodules/web/web/components/view/OSearchView.vuemodules/web/web/components/view/kanbanFirstFrame.test.tsmodules/web/web/components/view/kanbanFirstFrame.tsmodules/web/web/components/view/search/OSearch.behavior.test.tsmodules/web/web/components/view/search/OSearch.vuemodules/web/web/composables/search/useUserFilters.test.tsmodules/web/web/composables/search/useUserFilters.ts
💤 Files with no reviewable changes (1)
- modules/web/web/components/view/OSearchView.scopeKey.test.ts
| vi.mock('@/web/web/components/view/kanbanFirstFrame', () => ({ | ||
| shouldDeferViewFirstFrame: () => deferState.defer, | ||
| shouldDeferKanbanFirstFrame: () => deferState.defer, | ||
| })); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the mock preserve the component-reference contract.
The mock ignores searchView and OSearchView. Lines 115-130 therefore defer for SearchStub, but production defers only when both references are identical. This test can pass if custom search views incorrectly skip their fallback mount apply.
Proposed fix
vi.mock('`@/web/web/components/view/kanbanFirstFrame`', () => ({
- shouldDeferViewFirstFrame: () => deferState.defer,
+ shouldDeferViewFirstFrame: (searchView: unknown, oSearchView: unknown) =>
+ deferState.defer && searchView === oSearchView,
shouldDeferKanbanFirstFrame: () => deferState.defer,
}));📝 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.
| vi.mock('@/web/web/components/view/kanbanFirstFrame', () => ({ | |
| shouldDeferViewFirstFrame: () => deferState.defer, | |
| shouldDeferKanbanFirstFrame: () => deferState.defer, | |
| })); | |
| vi.mock('`@/web/web/components/view/kanbanFirstFrame`', () => ({ | |
| shouldDeferViewFirstFrame: (searchView: unknown, oSearchView: unknown) => | |
| deferState.defer && searchView === oSearchView, | |
| shouldDeferKanbanFirstFrame: () => deferState.defer, | |
| })); |
🤖 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 `@modules/web/web/components/view/OListView.firstframe.test.ts` around lines 45
- 48, Update the kanbanFirstFrame mock’s shouldDeferViewFirstFrame and
shouldDeferKanbanFirstFrame implementations to honor their searchView and
OSearchView arguments, matching production behavior by deferring only when the
references are identical and the configured defer state applies. Preserve the
existing deferState-driven behavior for matching references so SearchStub and
custom search views exercise the correct fallback mount path.
| async function emitFirstFrameIfNeeded(): Promise<void> { | ||
| if (mounted.value || !props.initialEmit) return; | ||
| await nextTick(); | ||
| const filtersAtFirstEmit = appliedFiltersForChild.value || []; | ||
| const groupsAtFirstEmit = appliedGroupsForChild.value || []; | ||
| const payload = buildQueryUpdatePayload<T>(keywordForChild.value, filtersAtFirstEmit, groupsAtFirstEmit, { | ||
| explicitGroups: false, | ||
| }); | ||
| emit('query-update', payload); | ||
| mounted.value = true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent duplicate first-frame query emissions.
At Lines 111-113, each defaults-ready handler checks mounted before awaiting nextTick(). If two events arrive in the same tick, both handlers pass that check and both emit query-update after the tick. This reintroduces redundant first-frame requests.
modules/web/web/components/view/OSearchView.vue#L111-L120: Recheckmountedafterawait nextTick(), or set a pending latch before awaiting.modules/web/web/components/view/OSearchView.test.ts#L142-L155: Emitdefaults-readytwice before awaiting Vue updates, then assert that exactly onequery-updateoccurs.
Proposed fix
async function emitFirstFrameIfNeeded(): Promise<void> {
if (mounted.value || !props.initialEmit) return;
await nextTick();
+ if (mounted.value || !props.initialEmit) return;
const filtersAtFirstEmit = appliedFiltersForChild.value || [];📝 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.
| async function emitFirstFrameIfNeeded(): Promise<void> { | |
| if (mounted.value || !props.initialEmit) return; | |
| await nextTick(); | |
| const filtersAtFirstEmit = appliedFiltersForChild.value || []; | |
| const groupsAtFirstEmit = appliedGroupsForChild.value || []; | |
| const payload = buildQueryUpdatePayload<T>(keywordForChild.value, filtersAtFirstEmit, groupsAtFirstEmit, { | |
| explicitGroups: false, | |
| }); | |
| emit('query-update', payload); | |
| mounted.value = true; | |
| async function emitFirstFrameIfNeeded(): Promise<void> { | |
| if (mounted.value || !props.initialEmit) return; | |
| await nextTick(); | |
| if (mounted.value || !props.initialEmit) return; | |
| const filtersAtFirstEmit = appliedFiltersForChild.value || []; | |
| const groupsAtFirstEmit = appliedGroupsForChild.value || []; | |
| const payload = buildQueryUpdatePayload<T>(keywordForChild.value, filtersAtFirstEmit, groupsAtFirstEmit, { | |
| explicitGroups: false, | |
| }); | |
| emit('query-update', payload); | |
| mounted.value = true; |
📍 Affects 2 files
modules/web/web/components/view/OSearchView.vue#L111-L120(this comment)modules/web/web/components/view/OSearchView.test.ts#L142-L155
🤖 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 `@modules/web/web/components/view/OSearchView.vue` around lines 111 - 120,
Prevent duplicate first-frame emissions in emitFirstFrameIfNeeded by rechecking
mounted.value after nextTick or setting a pending latch before awaiting, so
concurrent defaults-ready events produce only one query-update. In
modules/web/web/components/view/OSearchView.test.ts lines 142-155, emit
defaults-ready twice before awaiting Vue updates and assert exactly one
query-update; both sites require changes.
PR Code Suggestions ✨Explore these optional code suggestions:
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1 participant
User description
Summary
zh_CN(menu, save/share/delete dialogs, model fields).defaults-ready, defer OListView mount apply like Kanban, and guarduseUserFilters.loadwith a generation.Test plan
web.UserFilter/Search+ 1× model Search + 1× Count (no triple UserFilter / double Search)../choysum test unit web --fe./choysum i18n status web --lang zh_CN(orphan-only OK)Made with Cursor
PR Type
Bug fix, Enhancement, Tests
Description
Fix first-frame race and double UserFilter search: load favorites once in OSearch, guard with generation, defer list/kanban mount
Add edit favorite metadata (name, default, share) without changing Condition, using pencil button in menu
Provide Chinese (zh_CN) translations for all OSearch favorites UI strings, dialogs, and model fields
Add tests: OListView first-frame defer, edit metadata dialog, useUserFilters generation, and rewrite OSearchView tests
File Walkthrough
4 files
Refactor to await OSearch defaults-ready, remove own UserFilterSearch, defer first emitDefer mount apply when searchView is OSearchView usingshouldDeferViewFirstFrameSwitch to shouldDeferViewFirstFrame for deferring first frame inkanban viewAdd load generation guard to ignore stale responses; add updateMetafor editing favorites1 files
Add edit favorite metadata (pencil icon, dialog, updateMeta), importEditPen icon2 files
Rename to shouldDeferViewFirstFrame, deprecate old name, documentusage for list/kanbanRemove scope-key test file (logic moved out of OSearchView)2 files
Extract OSearch favorites and UserFilter model strings intotranslation templateProvide Chinese translations for OSearch favorites UI and model fields7 files
Rewrite tests to cover single-load defaults-ready flow and initialEmitbehaviorAdd first-frame defer test for OListView, verify mount apply anddeferralUpdate kanban first-frame test to use shouldDeferViewFirstFrame mockAdjust source check for shouldDeferViewFirstFrame in kanban view testAdd tests for the renamed shouldDeferViewFirstFrame functionAdd tests for load generation, updateMeta, and edge casesAdd test for edit favorite dialog, metadata update, and defaults-readycountSummary by CodeRabbit