fixed-issue-#51-imroved-pipline#54
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe changes introduce route query synchronization across three dashboard pages, enabling filter states to persist in the URL and be restored on navigation. Additionally, the main dashboard redesigns the pipeline visualization with a segmented progress bar and cards-based stage display. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
🚅 Deployed to the reqcore-pr-54 environment in applirank |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/pages/dashboard/applications/index.vue (1)
44-54: Watch triggersrouter.replaceon every client-side navigation.When
activeStatus.valueis reassigned at line 41, the watcher fires and callsrouter.replaceeven if the URL already matches. This is unnecessary overhead and may cause subtle issues with browser history.Consider adding a guard to skip the replace if the query already matches:
Proposed optimization
// Sync the URL when the status filter changes watch(activeStatus, (newStatus) => { + const currentStatus = route.query.status as string | undefined + if (newStatus === currentStatus || (!newStatus && !currentStatus)) return + const query = { ...route.query } if (newStatus) { query.status = newStatus } else { delete query.status } router.replace({ query }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/dashboard/applications/index.vue` around lines 44 - 54, The watcher on activeStatus currently calls router.replace on every change even when the URL already reflects the same status; modify the watch callback (the activeStatus watcher that receives newStatus) to first compare route.query.status to the intended value and only call router.replace if they differ (handle the case where newStatus is falsy by checking absence/undefined of route.query.status), i.e., compute the desired query.status and skip router.replace when route.query.status === desiredStatus so you avoid unnecessary replacements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/pages/dashboard/applications/index.vue`:
- Around line 35-54: The cached activeStatus can remain stale when
route.query.status is absent; update the initialization logic so
activeStatus.value is always set to reflect initialAppStatus (including when
initialAppStatus is undefined) instead of only when !== undefined. In practice,
replace the conditional guard around activeStatus.value assignment for the
useState('app-filter-status') so that activeStatus.value = initialAppStatus runs
unconditionally (or explicitly set activeStatus.value = undefined when
route.query.status is missing), referencing initialAppStatus, activeStatus,
useState, route.query, watch and router.replace to keep the UI state and URL in
sync.
---
Nitpick comments:
In `@app/pages/dashboard/applications/index.vue`:
- Around line 44-54: The watcher on activeStatus currently calls router.replace
on every change even when the URL already reflects the same status; modify the
watch callback (the activeStatus watcher that receives newStatus) to first
compare route.query.status to the intended value and only call router.replace if
they differ (handle the case where newStatus is falsy by checking
absence/undefined of route.query.status), i.e., compute the desired query.status
and skip router.replace when route.query.status === desiredStatus so you avoid
unnecessary replacements.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/pages/dashboard/applications/index.vueapp/pages/dashboard/index.vueapp/pages/dashboard/jobs/index.vue
Comment on lines
+35
to
+54
| const initialAppStatus = STATUS_OPTIONS.includes(route.query.status as any) | ||
| ? (route.query.status as Status) | ||
| : undefined | ||
| const activeStatus = useState<Status | undefined>('app-filter-status', () => initialAppStatus) | ||
| // Ensure the state matches the URL on navigation (useState caches across client-side navigations) | ||
| if (initialAppStatus !== undefined) { | ||
| activeStatus.value = initialAppStatus | ||
| } | ||
|
|
||
| // Sync the URL when the status filter changes | ||
| watch(activeStatus, (newStatus) => { | ||
| const query = { ...route.query } | ||
| if (newStatus) { | ||
| query.status = newStatus | ||
| } | ||
| else { | ||
| delete query.status | ||
| } | ||
| router.replace({ query }) | ||
| }) |
There was a problem hiding this comment.
Potential stale state issue when navigating without query params.
The guard at lines 40-42 only updates activeStatus when initialAppStatus !== undefined. If a user navigates to /dashboard/applications (no ?status= param) after previously visiting /dashboard/applications?status=new, the useState cache retains the old value ('new'), but the URL shows no filter.
This creates a mismatch: the UI filters by 'new' while the URL suggests no filter is active.
Proposed fix
const initialAppStatus = STATUS_OPTIONS.includes(route.query.status as any)
? (route.query.status as Status)
: undefined
const activeStatus = useState<Status | undefined>('app-filter-status', () => initialAppStatus)
// Ensure the state matches the URL on navigation (useState caches across client-side navigations)
-if (initialAppStatus !== undefined) {
- activeStatus.value = initialAppStatus
-}
+activeStatus.value = initialAppStatus🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/pages/dashboard/applications/index.vue` around lines 35 - 54, The cached
activeStatus can remain stale when route.query.status is absent; update the
initialization logic so activeStatus.value is always set to reflect
initialAppStatus (including when initialAppStatus is undefined) instead of only
when !== undefined. In practice, replace the conditional guard around
activeStatus.value assignment for the useState('app-filter-status') so that
activeStatus.value = initialAppStatus runs unconditionally (or explicitly set
activeStatus.value = undefined when route.query.status is missing), referencing
initialAppStatus, activeStatus, useState, route.query, watch and router.replace
to keep the UI state and URL in sync.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Type of change
Validation
DCO
Signed-off-by) viagit commit -sSummary by CodeRabbit
Release Notes
New Features
UI/UX Improvements