feat: add the probe type admin filter #153
Conversation
WalkthroughThis PR adds an admin-only probeType filter (values: Possibly related PRs
Suggested labels`in progress` Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
app/components/probe/ProbeFilters/AdminFilterSettings.vue (2)
11-16: Avoid lodash for simple labels; use explicit label maps to ensure correct wording and enable i18nupperFirst is brittle for hyphenated values and adds an extra dep. Define labels and render them, which also makes later i18n trivial.
Apply:
- import upperFirst from 'lodash/upperFirst'; + // prefer explicit labels over runtime capitalization (better control and i18n-ready) + const ADOPTION_LABELS = { all: 'All', adopted: 'Adopted', 'non-adopted': 'Non-adopted' } as const; + const PROBE_TYPE_LABELS = { all: 'All', hardware: 'Hardware', software: 'Software' } as const;And update slots:
- <template #value="{value}"> - {{upperFirst(value)}} - </template> - <template #option="{option}"> - {{upperFirst(option)}} - </template> + <template #value="{ value }"> + {{ ADOPTION_LABELS[value] }} + </template> + <template #option="{ option }"> + {{ ADOPTION_LABELS[option] }} + </template>- <template #value="{value}"> - {{upperFirst(value)}} - </template> - <template #option="{option}"> - {{upperFirst(option)}} - </template> + <template #value="{ value }"> + {{ PROBE_TYPE_LABELS[value] }} + </template> + <template #option="{ option }"> + {{ PROBE_TYPE_LABELS[option] }} + </template>Also applies to: 27-32, 39-39
42-49: Check nested prop mutation of a v-modelled prop; emit updates if the parent expects update:filterWhen
filteris provided (custom filter mode),v-model="usedFilter.adoption"mutates a nested prop directly. If the parent relies onupdate:filterto react (instead of observing deep mutations), this may not propagate.If needed, wrap fields with computed setters that replace the object (emitting
update:filter) instead of mutating in place, e.g.,adoptionModel/probeTypeModelcomputed get/set and bindv-modelto those.app/composables/useProbeFilters.ts (3)
86-87: Use an explicit query key (probeType) instead of generic type; update watcher accordingly
typeis ambiguous and prone to collision. PreferprobeTypein the URL and parsing.Apply:
--- a/app/composables/useProbeFilters.ts +++ b/app/composables/useProbeFilters.ts @@ - ...auth.isAdmin && !isDefault('probeType') && { type: filter.value.probeType }, + ...auth.isAdmin && !isDefault('probeType') && { probeType: filter.value.probeType }, @@ - () => route.query.type, - ], async ([ search, by, desc, status, adoption, probeType ]) => { + () => route.query.probeType, + ], ([ search, by, desc, status, adoption, probeTypeParam ]) => { @@ - if (typeof probeType === 'string' && PROBE_TYPE_OPTIONS.includes(probeType) && auth.isAdmin) { - filter.value.probeType = probeType as ProbeTypeOption; + if (typeof probeTypeParam === 'string' && PROBE_TYPE_OPTIONS.includes(probeTypeParam) && auth.isAdmin) { + filter.value.probeType = probeTypeParam as ProbeTypeOption; } else { filter.value.probeType = DEFAULT_FILTER.probeType; }Optional: after parsing, if the user is not admin but admin-only params are present, normalize the URL by calling
onParamChange()to strip them.Also applies to: 145-146, 181-185
39-42: Tighten option array typing to catch mismatches at compile timeUse readonly, strongly-typed arrays to improve
includestype narrowing and IDE hints.-export const ADOPTION_OPTIONS: string[] = [ 'all', 'adopted', 'non-adopted' ] as const; -export const PROBE_TYPE_OPTIONS: string[] = [ 'all', 'hardware', 'software' ] as const; +export const ADOPTION_OPTIONS: readonly AdoptionOption[] = [ 'all', 'adopted', 'non-adopted' ] as const; +export const PROBE_TYPE_OPTIONS: readonly ProbeTypeOption[] = [ 'all', 'hardware', 'software' ] as const;
139-147: Remove unnecessary async and consider stripping admin-only params for non-adminsThe watcher is marked
asyncbut does not await; dropasync. Also, if a non-admin lands withadoption/probeTypein the URL, consider normalizing the query to avoid exposing no-op admin params.Example:
- ], async ([ search, by, desc, status, adoption, probeType ]) => { + ], ([ search, by, desc, status, adoption, probeTypeParam ]) => { if (!toValue(active)) { return; } + // Optionally normalize URL for non-admin users: + // if (!auth.isAdmin && (adoption || probeTypeParam)) onParamChange();app/composables/useCreditsFilters.ts (1)
59-68: Consider alternative for 'other' handling.Adding
'other'to thereasonarray when all permitted reasons are selected (line 66) could be confusing since it's not an actual reason value. Consider alternatives:
- A separate
includeOtherboolean parameter- Omitting the filter entirely when all values are selected
However, if the backend API expects this format, the current implementation is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/components/AsyncCell.vue(1 hunks)app/components/credits/CreditsFilters.vue(1 hunks)app/components/probe/ProbeFilters/AdminFilterSettings.vue(1 hunks)app/components/probe/ProbeFilters/ProbeListFilters.vue(2 hunks)app/composables/useCreditsFilters.ts(1 hunks)app/composables/useProbeFilters.ts(8 hunks)app/pages/credits.vue(5 hunks)app/presets/aura/index.js(3 hunks)app/presets/aura/skeleton/index.js(1 hunks)app/presets/aura/tree/index.js(4 hunks)app/presets/aura/treeselect/index.js(4 hunks)app/utils/tree-select.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/probe/ProbeFilters/ProbeListFilters.vue
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{vue,ts,js}
⚙️ CodeRabbit configuration file
We use Nuxt with auto imports enabled. Don't warn about missing imports.
Files:
app/components/AsyncCell.vueapp/components/credits/CreditsFilters.vueapp/composables/useProbeFilters.tsapp/presets/aura/skeleton/index.jsapp/components/probe/ProbeFilters/AdminFilterSettings.vueapp/pages/credits.vueapp/utils/tree-select.tsapp/presets/aura/tree/index.jsapp/composables/useCreditsFilters.tsapp/presets/aura/treeselect/index.jsapp/presets/aura/index.js
🧠 Learnings (3)
📚 Learning: 2025-10-12T14:51:10.711Z
Learnt from: PavelKopecky
PR: jsdelivr/globalping-dash#154
File: app/components/credits/CreditsFilters.vue:101-106
Timestamp: 2025-10-12T14:51:10.711Z
Learning: In app/components/credits/CreditsFilters.vue, the early return when selectedCount.value === 0 is intentional. Having nothing selected is considered an invalid state, so the component preserves the previous filter values rather than clearing or resetting them.
Applied to files:
app/components/credits/CreditsFilters.vue
📚 Learning: 2025-10-06T17:46:36.047Z
Learnt from: PavelKopecky
PR: jsdelivr/globalping-dash#146
File: app/pages/credits.vue:58-62
Timestamp: 2025-10-06T17:46:36.047Z
Learning: In `app/pages/credits.vue`, the `CreditsChange` items rendered in the mobile view (lines 58-62) do not have an `id` field available for use as a v-for key. Using the array index is acceptable in this scenario due to the short list length and pagination-only updates.
Applied to files:
app/pages/credits.vue
📚 Learning: 2025-07-17T14:33:26.596Z
Learnt from: PavelKopecky
PR: jsdelivr/globalping-dash#103
File: pages/probes/index.vue:416-428
Timestamp: 2025-07-17T14:33:26.596Z
Learning: In the GlobalPing Dashboard probes listing page, there's a performance optimization where an initial lightweight probe count check is performed in onMounted() using only getUserFilter('userId'). This avoids running the more expensive loadLazyData() function (which includes complex filtering and status aggregation) when a user has no probes at all. The loadLazyData() function is only called if the initial count check confirms probes exist.
Applied to files:
app/pages/credits.vue
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run e2e tests
🔇 Additional comments (12)
app/presets/aura/skeleton/index.js (1)
13-13: Clarify styling migration rationale
The update fromdark:bg-surface-700todark:bg-dark-500(token defined in tailwind.config.js:58) isn’t related to the probe type admin filter. Document this change’s intent in the PR description or split it into a dedicated styling PR.app/presets/aura/tree/index.js (2)
44-47: Verify that thechecked || selectedlogic matches the intended tree behavior.The styling conditions changed from
context.selectedtocontext.checked || context.selectedacross multiple properties (text color, hover states, icon colors, label colors). This will highlight both checked and selected nodes identically.Confirm this is the intended behavior for the tree component, especially if it's being used for the probe type filter where selection vs. checked states may have different semantics.
Also applies to: 69-71, 92-94, 100-102
32-32: Validate combinedchecked || selectedcondition (lines 44, 47, 69-70, 92-93, 100-101): ensure the new highlighting logic matches the intended UX.app/presets/aura/index.js (1)
100-101: LGTM!The tree and treeselect components are correctly imported and exported in their appropriate categories (forms and data).
Also applies to: 136-136, 152-152
app/presets/aura/treeselect/index.js (1)
46-46: Verify WCAG AA contrast ratio for text-bluegray-400 Ensuretext-bluegray-400meets at least a 4.5:1 contrast ratio againstbg-surface-0(light mode) anddark:bg-dark-900(dark mode).app/composables/useProbeFilters.ts (1)
129-131: Confirm Directus null-check semantics for hardwareDeviceUsing
{ _eq: null }assumes empty/absent values are truly null. If the field can be empty-string or otherwise falsy, adjust to{ _null: true }/{ _nnull: true }or broaden the condition.app/components/AsyncCell.vue (1)
7-18: LGTM: Size prop implementation is clean.The addition of the
sizeprop with a simple computed class mapping is straightforward and works correctly. The change from anonymousdefinePropstoconst props = definePropsproperly exposes the props for the computed property.app/utils/tree-select.ts (1)
1-75: LGTM: Well-structured tree utilities.The TreeNode type and utility functions are cleanly implemented:
renderTreeSelectValuecorrectly handles empty selections, full selections, and partial selections with proper subtree collapsingbuildNodesByKeyproperly constructs the tree hierarchy using dash-separated key prefixes- Edge cases are handled (e.g., keys without dashes at line 64)
app/components/credits/CreditsFilters.vue (1)
30-136: LGTM: Solid filtering implementation.The component correctly implements tree-based filtering with:
- Proper debounced change handling with cleanup on unmount
- Correct recursive state management in
buildSelection- Change detection using
isEqualto prevent unnecessary updates- Early return when nothing is selected (intentional per project learnings)
Based on learnings
app/composables/useCreditsFilters.ts (1)
1-108: LGTM: Well-structured filter composable.The composable correctly:
- Validates route query parameters against permitted values
- Synchronizes filter state bidirectionally with the route
- Provides helpers for query construction and default comparison
- Handles the business logic of clearing reasons when type excludes 'additions'
app/pages/credits.vue (2)
30-30: PR objectives mismatch with code changes.The PR objectives state this adds "probe type to admin filter" for classifying probes as hardware/software based on
hardwareDevicefield (issue #144). However, all code changes in this review are implementing credits filtering with type/reason filters, not probe filtering.Verify that:
- The correct files were included in this PR
- The PR objectives accurately describe these changes
1-276: LGTM: Comprehensive filtering integration.The credits page successfully integrates filtering with:
- Proper loading state management using
initialLoadingfor the initial render andloadingfor individual cells- Consistent
AsyncCellusage with appropriate size variants- Correct filter dependency tracking using
computedDebounced- Well-structured empty states for both desktop and mobile views
- Proper synchronization with the filter composable
The implementation provides good UX by maintaining table structure during filtered reloads while showing per-cell loading indicators.
Closes #144