refactor: change requests list#592
Conversation
- Replace custom API endpoints (get_my_change_requests, get_pending_reviews) with standard createListResource, eliminating N+1 query overhead from per-row diff_change_request calls - Add pagination with load more (25 per page) - Add loading spinner for initial data fetch - Persist active tab state in URL query param (?tab=) using @vueuse/router - Use router.back() for back navigation to preserve tab state Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
NagariaHussain
changed the title
WalkthroughThis PR refactors the change requests and contributions UI layer. It adds a new router utility dependency, updates navigation in 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 4
🧹 Nitpick comments (3)
frontend/src/pages/ContributionReview.vue (1)
278-283: Organize imports and declarations for better readability.The
routerconstant is declared between imports, breaking the expected grouping. Move all imports together, then declarations.♻️ Suggested reorganization
import { ref, computed, reactive } from 'vue'; import { useRouter } from 'vue-router'; import { createDocumentResource, createResource, Button, Badge, Dialog, FormControl, LoadingIndicator, toast } from 'frappe-ui'; import { useUserStore } from '@/stores/user'; - -const router = useRouter(); import { useChangeRequestStore } from '@/stores/changeRequest'; import DiffViewer from '@/components/DiffViewer.vue'; import LucideChevronDown from '~icons/lucide/chevron-down'; import LucideAlertCircle from '~icons/lucide/alert-circle'; import LucideAlertTriangle from '~icons/lucide/alert-triangle'; import { useChangeTypeDisplay } from '@/composables/useChangeTypeDisplay'; + +const router = useRouter();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/ContributionReview.vue` around lines 278 - 283, Move the out-of-place declaration of router so all import statements are grouped together: keep the imports (useRouter, createDocumentResource, createResource, Button, Badge, Dialog, FormControl, LoadingIndicator, toast, useUserStore, useChangeRequestStore) at the top and then declare const router = useRouter() after the import block; update any nearby declarations to follow the same pattern so the file imports are contiguous and runtime declarations (router, stores) come after.frontend/package.json (1)
28-29: Consider aligning@vueuse/coreand@vueuse/routerversions.The
@vueuse/router(^14.2.1) and@vueuse/core(^14.1.0) packages are from the same monorepo. While the caret ranges ensure compatible resolutions, aligning their minimum versions avoids potential edge-case mismatches.🔧 Suggested alignment
- "@vueuse/core": "^14.1.0", - "@vueuse/router": "^14.2.1", + "@vueuse/core": "^14.2.1", + "@vueuse/router": "^14.2.1",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/package.json` around lines 28 - 29, The package versions for the VueUse monorepo are mismatched: "@vueuse/core": "^14.1.0" and "@vueuse/router": "^14.2.1"; update the package.json to align their minimum versions (for example set "@vueuse/core" to "^14.2.1" or both to the same 14.x version) so both resolve to the same release line and avoid edge-case mismatches during installs, then run yarn/npm install and update lockfile accordingly.frontend/src/pages/Contributions.vue (1)
123-139: Consider usinguseListper coding guidelines.The coding guidelines recommend using
useListfrom frappe-ui for managing list resources, which provides built-in support for DocType operations, pagination, filtering, and sorting without needing to specify API URLs. WhilecreateListResourceworks,useListmay offer a more standardized approach.As per coding guidelines: "Use
useListfrom frappe-ui for managing list resources with built-in support for DocType operations, pagination, filtering, and sorting without needing to specify API URLs".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/Contributions.vue` around lines 123 - 139, Replace the createListResource usage with frappe-ui's useList for both myChangeRequests and pendingReviews: locate the createListResource calls for myChangeRequests and pendingReviews and instantiate useList for Doctype "Wiki Change Request" passing equivalent options (fields, filters, orderBy/sort, pagination size, and auto-loading behavior), ensuring the pendingReviews auto/loading toggles via computed(() => isManager.value); keep the same field list and filters (owner filter for myChangeRequests, status filter for pendingReviews) and map pageLength to the useList pagination option so behavior remains identical while conforming to the coding guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/pages/ContributionReview.vue`:
- Around line 5-7: The Back button currently calls router.back() which can
navigate unexpectedly when there is no browser history; replace the direct call
with a safe handler (e.g., handleBack) that checks window.history.length > 1 and
calls router.back() only if true, otherwise navigates to a stable route using
router.push({ name: 'ChangeRequests' }); update the Button click binding to call
handleBack and ensure the new handler is defined in the component methods/setup
alongside router usage.
In `@frontend/src/pages/Contributions.vue`:
- Around line 123-130: The fields list for the createListResource call
(myChangeRequests) only includes the linked child field 'wiki_space.space_name',
so the parent linked object 'wiki_space' is not present and getRowRoute (used
for row navigation) receives undefined; update the fields array in the
myChangeRequests createListResource invocation to include the parent linked
field name 'wiki_space' (in addition to 'wiki_space.space_name') so
row.wiki_space is available, and apply the same change to the pendingReviews
createListResource if pendingReviews uses getRowRoute for navigation.
- Around line 123-130: The fields list for the createListResource calls is
returning the linked field as "wiki_space.space_name" while the table columns
expect "space_name"; update the fields in the myChangeRequests and
pendingReviews createListResource definitions to alias the linked field to
space_name (e.g., change 'wiki_space.space_name' to 'wiki_space.space_name as
space_name') so the returned row keys match the column key 'space_name' used in
the template.
- Around line 132-139: The createListResource call uses auto: computed(() =>
isManager.value) which is evaluated only once; change this by removing the
computed auto and instead add a watcher on isManager that, when it becomes true,
either creates/initializes pendingReviews or calls pendingReviews.reload() to
fetch data; locate the pendingReviews variable and the createListResource
invocation and implement a watcher (watch(isManager, ...)) or conditional
creation logic so the resource is loaded/refetched when isManager.value
transitions to true.
---
Nitpick comments:
In `@frontend/package.json`:
- Around line 28-29: The package versions for the VueUse monorepo are
mismatched: "@vueuse/core": "^14.1.0" and "@vueuse/router": "^14.2.1"; update
the package.json to align their minimum versions (for example set "@vueuse/core"
to "^14.2.1" or both to the same 14.x version) so both resolve to the same
release line and avoid edge-case mismatches during installs, then run yarn/npm
install and update lockfile accordingly.
In `@frontend/src/pages/ContributionReview.vue`:
- Around line 278-283: Move the out-of-place declaration of router so all import
statements are grouped together: keep the imports (useRouter,
createDocumentResource, createResource, Button, Badge, Dialog, FormControl,
LoadingIndicator, toast, useUserStore, useChangeRequestStore) at the top and
then declare const router = useRouter() after the import block; update any
nearby declarations to follow the same pattern so the file imports are
contiguous and runtime declarations (router, stores) come after.
In `@frontend/src/pages/Contributions.vue`:
- Around line 123-139: Replace the createListResource usage with frappe-ui's
useList for both myChangeRequests and pendingReviews: locate the
createListResource calls for myChangeRequests and pendingReviews and instantiate
useList for Doctype "Wiki Change Request" passing equivalent options (fields,
filters, orderBy/sort, pagination size, and auto-loading behavior), ensuring the
pendingReviews auto/loading toggles via computed(() => isManager.value); keep
the same field list and filters (owner filter for myChangeRequests, status
filter for pendingReviews) and map pageLength to the useList pagination option
so behavior remains identical while conforming to the coding guideline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1674384f-8a0f-4ca2-9082-aa910ae0bb5a
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
frontend/package.jsonfrontend/src/pages/ContributionReview.vuefrontend/src/pages/Contributions.vuewiki/api/change_requests.py
💤 Files with no reviewable changes (1)
- wiki/api/change_requests.py
| <Button variant="ghost" icon-left="arrow-left" @click="router.back()"> | ||
| {{ __('Back') }} | ||
| </Button> |
There was a problem hiding this comment.
router.back() may navigate unexpectedly when there's no history.
If a user navigates directly to this page (e.g., via bookmark or shared link), router.back() will exit the application or go to an unrelated page. Consider a fallback.
🛡️ Suggested fallback
-<Button variant="ghost" icon-left="arrow-left" `@click`="router.back()">
+<Button variant="ghost" icon-left="arrow-left" `@click`="handleBack">Then add:
function handleBack() {
if (window.history.length > 1) {
router.back();
} else {
router.push({ name: 'ChangeRequests' });
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/ContributionReview.vue` around lines 5 - 7, The Back
button currently calls router.back() which can navigate unexpectedly when there
is no browser history; replace the direct call with a safe handler (e.g.,
handleBack) that checks window.history.length > 1 and calls router.back() only
if true, otherwise navigates to a stable route using router.push({ name:
'ChangeRequests' }); update the Button click binding to call handleBack and
ensure the new handler is defined in the component methods/setup alongside
router usage.
| const myChangeRequests = createListResource({ | ||
| doctype: 'Wiki Change Request', | ||
| fields: ['name', 'title', 'wiki_space.space_name', 'status', 'modified', 'archived_at', 'merged_at'], | ||
| filters: { owner: ['=', userStore.user] }, | ||
| orderBy: 'modified desc', | ||
| pageLength: 25, | ||
| auto: true, | ||
| }); |
There was a problem hiding this comment.
Missing wiki_space field breaks navigation for Draft items.
The getRowRoute function (Line 170) uses row.wiki_space for navigation, but the fields array only includes wiki_space.space_name. The linked field's parent wiki_space value won't be present in the row data, causing the route parameter to be undefined.
🐛 Proposed fix: Include the parent field
const myChangeRequests = createListResource({
doctype: 'Wiki Change Request',
- fields: ['name', 'title', 'wiki_space.space_name', 'status', 'modified', 'archived_at', 'merged_at'],
+ fields: ['name', 'title', 'wiki_space', 'wiki_space.space_name', 'status', 'modified', 'archived_at', 'merged_at'],
filters: { owner: ['=', userStore.user] },
orderBy: 'modified desc',
pageLength: 25,
auto: true,
});Apply the same fix to pendingReviews if it also needs navigation using wiki_space.
Also applies to: 168-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/Contributions.vue` around lines 123 - 130, The fields list
for the createListResource call (myChangeRequests) only includes the linked
child field 'wiki_space.space_name', so the parent linked object 'wiki_space' is
not present and getRowRoute (used for row navigation) receives undefined; update
the fields array in the myChangeRequests createListResource invocation to
include the parent linked field name 'wiki_space' (in addition to
'wiki_space.space_name') so row.wiki_space is available, and apply the same
change to the pendingReviews createListResource if pendingReviews uses
getRowRoute for navigation.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how frappe-ui createListResource handles linked field names
# Search for similar patterns in the codebase
rg -n "createListResource" --type=vue -A 15 | head -80
# Look for any existing usage of dot notation in field specifications
rg -n "fields:.*\.\w+" --type=vue -C 3Repository: frappe/wiki
Length of output: 110
🌐 Web query:
How does frappe-ui createListResource return linked field data when using dot notation like 'wiki_space.space_name'?
💡 Result:
frappe-ui's createListResource fetches data by calling Frappe's frappe.client.get_list method (or equivalent REST API /api/resource/{doctype}), passing the specified 'fields' array, 'filters', etc. as parameters. When using dot notation like 'wiki_space.space_name' in the fields array (where 'wiki_space' is a Link field to the Wiki Space doctype, and 'space_name' is a field in that doctype), Frappe's backend SQL query uses LEFT JOIN on the linked table. This returns a flat structure with the linked field value as a scalar (string), aliased if using 'as', e.g.: [ { "name": "DOC-001", "wiki_space": "WS-001", "wiki_space.space_name": "My Space" // or aliased like "space_name": "My Space" if using 'wiki_space.space_name as space_name' } ] It does NOT return nested objects/embedded documents by default. Nested expansion requires additional parameters like 'expand_links=always' or 'expand=["wiki_space"]' (recent feature), but createListResource docs do not mention an 'expand' option, so dot notation provides joined scalar values, not full linked documents.
Citations:
- 1: https://ui.frappe.io/docs/data-fetching/list-resource
- 2: https://frappeframework.com/docs/v13/user/en/api/database
- 3: https://docs.frappe.io/framework/user/en/api/rest
- 4: https://docs.frappe.io/framework/get_query
🏁 Script executed:
# Find and read the Contributions.vue file
find . -name "Contributions.vue" -type f 2>/dev/nullRepository: frappe/wiki
Length of output: 93
🏁 Script executed:
# Search for createListResource calls in Vue files using a broader pattern
find . -name "*.vue" -type f -exec grep -l "createListResource" {} \; 2>/dev/null | head -5Repository: frappe/wiki
Length of output: 133
🏁 Script executed:
# Also check for existing patterns of linked field usage in Vue files
find . -name "*.vue" -type f -exec grep -n "fields:.*\." {} + 2>/dev/null | head -20Repository: frappe/wiki
Length of output: 327
🏁 Script executed:
cat -n ./frontend/src/pages/Contributions.vue | sed -n '120,155p'Repository: frappe/wiki
Length of output: 1471
🏁 Script executed:
cat -n ./frontend/src/pages/Contributions.vue | sed -n '155,220p'Repository: frappe/wiki
Length of output: 1826
🏁 Script executed:
cat -n ./frontend/src/pages/Contributions.vue | sed -n '1,119p'Repository: frappe/wiki
Length of output: 4615
Add field alias for linked field wiki_space.space_name to match column key space_name.
The fields array (lines 125, 134) includes 'wiki_space.space_name' without an alias, but the column definitions (lines 143, 151) use key: 'space_name'. Without an alias, the Frappe API returns the field as 'wiki_space.space_name', causing the template (lines 30, 70) to try accessing row['space_name'] which doesn't exist, resulting in empty Space columns.
Update the field specification to: 'wiki_space.space_name as space_name' in both myChangeRequests and pendingReviews resource definitions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/Contributions.vue` around lines 123 - 130, The fields list
for the createListResource calls is returning the linked field as
"wiki_space.space_name" while the table columns expect "space_name"; update the
fields in the myChangeRequests and pendingReviews createListResource definitions
to alias the linked field to space_name (e.g., change 'wiki_space.space_name' to
'wiki_space.space_name as space_name') so the returned row keys match the column
key 'space_name' used in the template.
| const pendingReviews = createListResource({ | ||
| doctype: 'Wiki Change Request', | ||
| fields: ['name', 'title', 'wiki_space.space_name', 'status', 'owner', 'modified'], | ||
| filters: { status: ['in', ['In Review', 'Approved']] }, | ||
| orderBy: 'modified desc', | ||
| pageLength: 25, | ||
| auto: computed(() => isManager.value), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how auto property is used with createListResource in the codebase
# Search for createListResource with auto property patterns
rg -n "createListResource" --type=vue -A 10 | rg -A 5 -B 5 "auto:"Repository: frappe/wiki
Length of output: 82
🌐 Web query:
Does frappe-ui createListResource support reactive or computed values for the auto option?
💡 Result:
No, frappe-ui's createListResource does not natively support reactive or computed values directly in the 'auto' option. The 'auto: true' option triggers an initial automatic fetch when the resource is created, but the resource object itself is reactive (properties like data, loading are reactive). However, options like filters passed at creation time are evaluated once and do not automatically refetch if those values change later, as they are not deeply watched for reactivity. Evidence from sources: - Official docs show 'auto: true' as a static boolean for initial fetch, with no mention of reactive/computed support in options (result 2). - GitHub issue #64 demonstrates the problem: when filters use another resource's data (reactive), it doesn't refetch when that data updates, indicating lack of automatic reactivity for dependent values (result 1). - Resource docs note cache can accept "reactive variable" but examples show serializable arrays/strings; no explicit computed support for auto (result 3). - Users must manually call reload or update options after changes for refetching. Workarounds: Use watchers on dependent refs to call resource.reload or resource.update(newOptions).
Citations:
- 1: Serial resource requests? frappe-ui#64
- 2: https://ui.frappe.io/docs/data-fetching/list-resource
- 3: https://ui.frappe.io/docs/data-fetching/resource
🏁 Script executed:
#!/bin/bash
# Search for createListResource usage in all Vue files
fd -e vue -type f | xargs grep -l "createListResource" | head -20Repository: frappe/wiki
Length of output: 225
🏁 Script executed:
#!/bin/bash
# Look at the Contributions.vue file to understand isManager and pendingReviews context
cat -n frontend/src/pages/Contributions.vue | head -150Repository: frappe/wiki
Length of output: 5828
🏁 Script executed:
#!/bin/bash
# Search for how isManager is used and if there's a watcher handling it
rg -n "isManager" frontend/src/pages/Contributions.vue -B 2 -A 2Repository: frappe/wiki
Length of output: 523
🏁 Script executed:
#!/bin/bash
# Check the rest of Contributions.vue for watchers or effects
cat -n frontend/src/pages/Contributions.vue | tail -n +140 | head -100Repository: frappe/wiki
Length of output: 2481
🏁 Script executed:
#!/bin/bash
# Check imports and beginning of script setup to see if there are any watchers
head -n 95 frontend/src/pages/Contributions.vue | tail -n 10Repository: frappe/wiki
Length of output: 324
Replace auto: computed(() => isManager.value) with a watcher-based reload or conditional creation.
The auto option in createListResource expects a static boolean, not a reactive value. The computed ref will be evaluated only once at resource creation, so if isManager is false initially and later becomes true, the resource will not refetch automatically. Add a watcher to call pendingReviews.reload() when isManager.value becomes true, or use conditional resource creation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/Contributions.vue` around lines 132 - 139, The
createListResource call uses auto: computed(() => isManager.value) which is
evaluated only once; change this by removing the computed auto and instead add a
watcher on isManager that, when it becomes true, either creates/initializes
pendingReviews or calls pendingReviews.reload() to fetch data; locate the
pendingReviews variable and the createListResource invocation and implement a
watcher (watch(isManager, ...)) or conditional creation logic so the resource is
loaded/refetched when isManager.value transitions to true.
1 participant
Summary
get_my_change_requests,get_pending_reviews) with standardcreateListResource, eliminating the N+1 query overhead from per-rowdiff_change_requestcalls that were causing ~7.7s load timeschange_countcolumn from list views (available inside the detail page)?tab=) using@vueuse/routerrouter.back()to preserve tab stateSummary by CodeRabbit
New Features
Improvements