feat: add property management utilities and schemas#166
Merged
Conversation
- Implemented property loading and attachment helpers in `properties.ts` for managing custom properties. - Created Zod schemas for property definitions and configurations in `schemas/property.ts`. - Introduced shared types and utility functions for properties in `shared/properties.ts`. Co-authored-by: Copilot <copilot@github.com>
|
🚅 Deployed to the reqcore-pr-166 environment in applirank
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds tenant-scoped custom properties: DB schema, migrations, server APIs for definitions/values (CRUD, reorder, validation, listing, filtering), utilities to load/match property values, client composables, many Vue components (display, editor, filters, drawers, saved views, columns), and integrates properties into applications, candidates, and jobs UIs. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as PropertyValueEditor (client)
participant Composable as useEntityPropertyMutations
participant Server as API PUT /<entity>/:id/properties/:propId
participant DB as property_value table
participant Parent as Parent Component
UI->>UI: User edits value (optimistic display)
UI->>Composable: setValue(entryId, newValue)
Composable->>Composable: optimistic update local entries ref
Composable->>Server: PUT normalized value
alt server success
Server->>DB: upsert property_value
DB-->>Server: persisted value
Server-->>Composable: return { value }
Composable->>Composable: reconcile local entries with server
Composable->>Parent: invoke onAfter / emit refresh
Parent->>UI: refresh displayed data
else server error
Server-->>Composable: error
Composable->>Composable: rollback local entries
Composable->>UI: surface toast/error
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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. Review rate limit: 0/1 reviews remaining, refill in 44 minutes and 9 seconds.Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (6)
shared/properties.ts (1)
117-121: Consider adding a type guard for themulti_selectcase.The cast
value as string[]assumes the value is an array. If malformed data is passed, this could cause a runtime error when calling.map(). A defensive check would improve robustness.🛡️ Optional defensive fix
case 'multi_select': { const opts = (config as PropertySelectConfig | null)?.options ?? [] - const ids = (value as string[]) ?? [] + const ids = Array.isArray(value) ? value : [] return ids.map((id) => opts.find((o) => o.id === id)?.label ?? '').filter(Boolean).join(', ') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/properties.ts` around lines 117 - 121, The multi_select branch currently casts value to string[] and calls .map(), which can throw if value is malformed; add a defensive type guard in the 'multi_select' case to verify Array.isArray(value) and that each element is a string (or coerce to an empty array) before mapping. Use the existing PropertySelectConfig/options (opts) and only compute ids = value when the guard passes (otherwise ids = []), then proceed with opts.find(...).label logic so .map() is never called on a non-array.app/components/FilterDrawer.vue (1)
45-57: Consider consolidating the two watchers onprops.modelValue.Both watchers observe the same property and could be merged into a single watcher for slightly cleaner code, though this is purely a stylistic preference.
♻️ Optional consolidation
-// Reset save form when drawer closes -watch(() => props.modelValue, (open) => { - if (!open) { - showSaveForm.value = false - newName.value = '' - } -}) - -// Lock body scroll while open -watch(() => props.modelValue, (open) => { - if (typeof document === 'undefined') return - document.body.style.overflow = open ? 'hidden' : '' -}) +// Handle drawer open/close side effects +watch(() => props.modelValue, (open) => { + // Reset save form when drawer closes + if (!open) { + showSaveForm.value = false + newName.value = '' + } + // Lock body scroll while open + if (typeof document !== 'undefined') { + document.body.style.overflow = open ? 'hidden' : '' + } +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/FilterDrawer.vue` around lines 45 - 57, Merge the two watchers on props.modelValue into a single watcher so both effects run together: when open is false reset showSaveForm.value and newName.value, and when open changes set document.body.style.overflow = open ? 'hidden' : '' (guarding for typeof document === 'undefined'). Update the existing watcher(s) so the single watcher handles both reset logic (showSaveForm, newName) and body scroll locking (document.body.style.overflow) to avoid duplicate observers.app/components/JobSubNavActions.vue (1)
270-283: Consider de-duplicating the property-editor menu items.Lines 270-283 repeat the same button structure; a small data-driven render (
v-forover scope/label) would reduce maintenance overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/JobSubNavActions.vue` around lines 270 - 283, The two nearly identical buttons in JobSubNavActions.vue should be data-driven: create an array (e.g., propertyEditorItems) in the component's setup/data containing objects with { key: 'job'|'org', label: 'Manage Job Properties'|'Manage Application Properties' } and replace the duplicated button markup with a single v-for that renders each item, calling openPropertyEditor(item.key) and including the Settings2 icon; this removes duplication and centralizes the labels/keys for easier maintenance.server/utils/schemas/application.ts (1)
30-31: Add a max-length guard forpropertyFilters.Line 31 should cap string length to prevent oversized JSON filter payloads from entering the parse path.
Proposed change
- propertyFilters: z.string().optional(), + propertyFilters: z.string().max(5000).optional(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/schemas/application.ts` around lines 30 - 31, The propertyFilters field currently allows unbounded strings which can let oversized JSON payloads through; update the schema's propertyFilters validator to enforce a maximum length (e.g., replace z.string().optional() with z.string().max(10000, "propertyFilters too long").optional()) so extremely large JSON arrays are rejected before parsing; locate the propertyFilters entry in server/utils/schemas/application.ts and add the .max(...) constraint (choose an appropriate max like 10000 or project standard) and an error message for clarity.server/utils/schemas/candidate.ts (1)
63-64: BoundpropertyFilterssize at schema level.Line 64 accepts an unbounded string; add a
.max(...)guard to reject oversized filter payloads earlier.Proposed change
- propertyFilters: z.string().optional(), + propertyFilters: z.string().max(5000).optional(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/schemas/candidate.ts` around lines 63 - 64, The schema currently allows an unbounded string for the JSON-encoded propertyFilters field; update the candidate schema's propertyFilters definition to add a maximum length check (e.g., z.string().max(10000) or another appropriate byte/char limit) before .optional() so oversized payloads are rejected at validation time; modify the propertyFilters schema entry in server/utils/schemas/candidate.ts (the propertyFilters z.string() declaration) to include .max(...) and an explicit error message if desired.app/components/PropertyTableCell.vue (1)
27-33: KeeplocalEntriesin sync withdefinitionchanges too.The watcher only tracks
props.value. If Vue reuses this cell for a differentdefinitionthat happens to have the same value,localEntries[0].definitionstays stale and the mutation/editor state can drift from the rendered column.Suggested change
watch( - () => props.value, - (next) => { - localEntries.value = [{ definition: props.definition, value: next }] + () => [props.definition, props.value] as const, + ([definition, value]) => { + localEntries.value = [{ definition, value }] }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/PropertyTableCell.vue` around lines 27 - 33, The watcher only observes props.value so localEntries[0].definition can become stale when the cell is reused with a new props.definition; update the watch to observe both props.value and props.definition (e.g. watch a tuple or array of () => props.value and () => props.definition) and set localEntries.value = [{ definition: props.definition, value: nextValue }] so localEntries is kept in sync with definition changes as well as value changes.
🤖 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/components/PropertyFilterBar.vue`:
- Around line 172-180: Replace the non-focusable <span
`@click.stop`="removeFilter(idx)"> with a real button element so keyboard and
screen-reader users can remove a chip: change the span to a <button
type="button"> that retains the same visual classes and the
`@click.stop`="removeFilter(idx)" handler, and add an accessible label such as
aria-label="Remove filter: {{ summarizeFilter(f) }}" (or similar) so assistive
tech announces which filter will be removed; keep the outer toggle button's
`@click` behavior intact by ensuring the inner button continues to stop
propagation.
In `@app/components/PropertyValueEditor.vue`:
- Around line 40-42: The v-model bindings are using non-writable expressions
like "draft as string" which Vue cannot assign to; replace those casts by
introducing a computed property (e.g., draftString) with a getter that returns
draft.value as a string and a setter that assigns the incoming value back to
draft.value, then update the v-model targets to bind to draftString instead of
"draft as string"; locate usages around the v-models referenced in the diff and
update them to use the new computed setter/getter so v-model assignments work
correctly.
In `@app/composables/useSavedViews.ts`:
- Around line 24-26: The TypeScript errors come from Vue's deep unwrapping when
using ref<SavedView<T>[]> for the views array; replace the deep ref with a
shallowRef to preserve the generic T type (use shallowRef<SavedView<T>[]>([]))
and import shallowRef from 'vue'; keep activeViewId and loaded as ref(...)
unless you also need shallow behavior; update any places that set views to
assign the whole shallowRef.value = newArray (same immutable pattern) so types
remain correct.
In `@app/pages/dashboard/applications/`[id].vue:
- Line 364: The binding currently forces entries with an unsafe cast
":entries='(application.properties ?? []) as never'"; remove the "as never" and
properly type the source instead: update the useApplication composable so the
useFetch call explicitly types its data (e.g. useFetch<{ properties:
PropertyEntry[] }>(...)) so that the application variable is strongly typed, or
cast to the correct PropertyEntry[] when passing to PropertyBlock; target
symbols: useApplication (composable), useFetch call inside it, the application
variable, PropertyEntry type, and the PropertyBlock entries prop.
In `@app/pages/dashboard/applications/index.vue`:
- Around line 238-266: The view settings type and flow need to include
propertyFilters so drawer filters are saved/reset and dirty-state is correct:
extend the ApplicationsViewSettings type to add propertyFilters (matching the
existing filter structure), add propertyFilters to defaultSettings with an
empty/default value, include propertyFilters in the currentSettings computed
(alongside status/jobId/sortKey/sortDir) and assign it in applySettings so
active property filters are applied when loading a view; update any
serialization/compare logic that uses ApplicationsViewSettings to account for
the new propertyFilters field.
In `@app/pages/dashboard/candidates/`[id].vue:
- Around line 436-441: The page uses a unsafe cast "(candidate.properties ?? [])
as never" because useCandidate's useFetch call is untyped; fix by adding an
explicit response type to useFetch inside the useCandidate composable
(app/composables/useCandidate.ts) so the returned candidate has properties:
PropertyEntry[]; update the generic/type parameter on useFetch (or the
composable's return type) so candidate.properties is typed correctly and you can
remove the as never cast in PropertyBlock usage.
In `@app/pages/dashboard/candidates/index.vue`:
- Around line 172-202: The saved view is missing propertyFilters, so add a
propertyFilters?: <type> field to CandidatesViewSettings and include
propertyFilters in defaultSettings, the currentSettings computed (return
propertyFilters: propertyFilters.value) and applySettings (set
propertyFilters.value = s.propertyFilters). Update all three occurrences (the
type declaration CandidatesViewSettings, the defaultSettings constant, and the
currentSettings and applySettings functions) so saved/select/reset flows persist
and restore propertyFilters and isDirty reflects it.
In `@server/api/applications/`[id]/properties/[propId].put.ts:
- Around line 13-24: The route docs state that omitting `value` clears the
property but the validator `setPropertyValueSchema` (used via
`readValidatedBody(event, setPropertyValueSchema.parse)`) currently requires
`value`, causing a mismatch; fix by either making `value` optional in
`setPropertyValueSchema` in server/utils/schemas/property.ts (so omission is
accepted and treated as clear) or update the route docs and callers to require
passing `null` to clear; ensure `readValidatedBody` usage in this handler (and
any downstream logic in the function that checks `value`) handles the new
optional type or explicit null case consistently.
In `@server/api/applications/index.get.ts`:
- Around line 36-49: The parsed propertyFilters array must be schema-validated
(not just cast) and rejected with 400 if any item is malformed, oversized, or
uses unsupported operators; update the parsing block in index.get.ts to validate
each item against a real PropertyFilter schema (use an existing validator or
implement checks) ensuring required fields exist, types are correct, the array
length ≤ 20 (reject if >20), and that op is one of the supported operators
(reject items like op: 'isEmpty'); only pass the validated, sanitized
propertyFilters to entityIdsMatchingFilters so server/utils/properties.ts never
receives unsupported ops.
In `@server/api/candidates/index.get.ts`:
- Around line 46-59: The code currently JSON.parses propertyFilters and casts to
PropertyFilter[] which allows malformed/unsupported filters through and forwards
unsupported flags (like isEmpty) to entityIdsMatchingFilters; instead, after
parsing propertyFilters (the variable) validate each item conforms to the
expected schema (object with required keys, correct types, and operator in an
allow-list of supported ops), reject with createError({statusCode:400,
statusMessage:'Invalid or unsupported propertyFilters'}) if any item fails,
truncate to 20 valid filters (slice), and ensure you only pass the validated
filters to entityIdsMatchingFilters (do not forward unsupported fields such as
isEmpty). Use the symbols PropertyFilter, propertyFilters, and
entityIdsMatchingFilters to locate and update the validation logic.
In `@server/api/properties/index.post.ts`:
- Line 1: The query that finds the last org-global property uses
eq(propertyDefinition.jobId, null as any) which generates SQL "jobId = NULL" and
never matches; replace that equality check with the proper NULL check (use
propertyDefinition.jobId.isNull() in the same query building code that computes
`last`/`nextOrder`) so org-global properties are matched correctly and nextOrder
doesn't reset to 0. Locate the query that uses eq(...) on
propertyDefinition.jobId and swap it to the isNull() form, adjusting
imports/usages if necessary.
In `@server/api/properties/reorder.post.ts`:
- Around line 18-44: Load scope metadata (e.g., entityType, jobId) for the
submitted ids from propertyDefinition and ensure all submitted ids belong to a
single scope; if multiple scopes are present, throw a 400/422. Then inside the
db.transaction: fetch all propertyDefinition rows for that single scope (filter
by organizationId and the scope columns) ordered by displayOrder; if you require
full-scope enforcement, validate ids.length === scopeRows.length and reject if
not; otherwise compute a new ordering by placing the submitted ids in the
requested order and then appending the remaining scope rows in their existing
relative order, and update displayOrder/updatedAt for every row in that scope in
sequence. Use the existing symbols propertyDefinition, ids, orgId,
db.transaction, tx.update(...) and the same eq/inArray filters to locate rows so
the updates only touch the single scope.
In `@server/database/migrations/0028_custom_properties.sql`:
- Around line 7-19: The DB currently allows rows in property_definition where
entity_type = 'candidate' and job_id is non-null; add a CHECK constraint on the
property_definition table (referencing columns entity_type and job_id and the
enum property_entity_type) to forbid that combination (e.g. CHECK (NOT
(entity_type = 'candidate' AND job_id IS NOT NULL))). In the migration, ensure
existing data is validated before adding the constraint (either fail the
migration with a clear error if violating rows exist or include a safe data-fix
step such as setting job_id = NULL for rows where entity_type = 'candidate') so
the constraint can be applied cleanly.
In `@server/utils/properties.ts`:
- Around line 113-123: The current loadPropertyEntriesForEntities function
builds a single union of org-global plus all job-scoped definitions (using the
jobIds param) and then attaches that same definitions set to every entity,
causing entities to receive properties from unrelated jobs; change
loadPropertyEntriesForEntities to attach only org-global defs plus the specific
job-scoped defs for each entity by either (A) accept an entityJobIds map (keyed
by entityId → jobId) and when expanding definitions select job-scoped defs where
def.jobId === null OR def.jobId === entityJobIds[entityId], or (B) group
entityIds by jobId, fetch definitions per group, and in the final loop merge
only the org-global defs with that group's job-specific defs before pushing into
the result Map<string, PropertyEntry[]> so each entity only gets its own job's
properties (update all uses of jobIds/entity handling in
loadPropertyEntriesForEntities, and the loops around definitions merging at the
end of the function).
- Around line 239-255: The isEmpty branch currently seeds working with an empty
set and returns no matches; fix this by either (A) requiring the caller to pass
the universe of entity ids into this helper and computing the complement: query
matchingRows via db.select(...) (as already done), build haveValue from
matchingRows, then set working = intersect(working or universe, universe \
haveValue) so isEmpty selects entities not in haveValue; or (B) if you cannot
access a universe here, explicitly reject the operator by throwing an error when
f.op === 'isEmpty' (instead of setting working = new Set()) so callers know to
handle isEmpty at a higher level; update the branch that references f.op,
propertyValue, matchingRows, haveValue, and working accordingly.
In `@server/utils/schemas/property.ts`:
- Around line 44-51: The schema createPropertyDefinitionSchema currently allows
a jobId with entityType 'candidate', but the loader only reads candidate
properties when jobId IS NULL; update createPropertyDefinitionSchema to reject
jobId for candidate properties by adding a conditional refinement (or
refine/superRefine) that enforces: if entityType === 'candidate' then jobId must
be nullish (fail with a clear message like "jobId must be null for candidate
properties"), otherwise keep the existing jobId nullish/validation; use the
schema names createPropertyDefinitionSchema and jobId to locate and change the
code.
- Around line 124-132: The URL validator in the 'url' case currently accepts any
scheme via new URL(rawValue); update the logic in the 'url' branch of
server/utils/schemas/property.ts to parse the value with new URL(rawValue) only
after confirming it's a string, then check url.protocol and only accept 'http:'
or 'https:' — otherwise return fail('Invalid URL scheme') (or similar). Keep the
existing try/catch for parsing errors but add the explicit protocol whitelist
check before returning rawValue so only safe http/https links are allowed.
- Around line 36-41: propertyConfigSchema currently permits arbitrary objects
via z.object({}).passthrough(), letting malformed configs bypass validation and
breaking validateValueForType; remove the passthrough branch from
propertyConfigSchema so the union is only selectConfigSchema |
numberConfigSchema | z.null(), and update createPropertyDefinitionSchema and
updatePropertyDefinitionSchema to rely on the tightened propertyConfigSchema; if
you need to allow future unknown configs, replace the passthrough branch with a
stricter shape (e.g., z.record(z.unknown()) only if you explicitly handle
generic records) and ensure validateValueForType handles missing required fields
(like select.options) defensively.
---
Nitpick comments:
In `@app/components/FilterDrawer.vue`:
- Around line 45-57: Merge the two watchers on props.modelValue into a single
watcher so both effects run together: when open is false reset
showSaveForm.value and newName.value, and when open changes set
document.body.style.overflow = open ? 'hidden' : '' (guarding for typeof
document === 'undefined'). Update the existing watcher(s) so the single watcher
handles both reset logic (showSaveForm, newName) and body scroll locking
(document.body.style.overflow) to avoid duplicate observers.
In `@app/components/JobSubNavActions.vue`:
- Around line 270-283: The two nearly identical buttons in JobSubNavActions.vue
should be data-driven: create an array (e.g., propertyEditorItems) in the
component's setup/data containing objects with { key: 'job'|'org', label:
'Manage Job Properties'|'Manage Application Properties' } and replace the
duplicated button markup with a single v-for that renders each item, calling
openPropertyEditor(item.key) and including the Settings2 icon; this removes
duplication and centralizes the labels/keys for easier maintenance.
In `@app/components/PropertyTableCell.vue`:
- Around line 27-33: The watcher only observes props.value so
localEntries[0].definition can become stale when the cell is reused with a new
props.definition; update the watch to observe both props.value and
props.definition (e.g. watch a tuple or array of () => props.value and () =>
props.definition) and set localEntries.value = [{ definition: props.definition,
value: nextValue }] so localEntries is kept in sync with definition changes as
well as value changes.
In `@server/utils/schemas/application.ts`:
- Around line 30-31: The propertyFilters field currently allows unbounded
strings which can let oversized JSON payloads through; update the schema's
propertyFilters validator to enforce a maximum length (e.g., replace
z.string().optional() with z.string().max(10000, "propertyFilters too
long").optional()) so extremely large JSON arrays are rejected before parsing;
locate the propertyFilters entry in server/utils/schemas/application.ts and add
the .max(...) constraint (choose an appropriate max like 10000 or project
standard) and an error message for clarity.
In `@server/utils/schemas/candidate.ts`:
- Around line 63-64: The schema currently allows an unbounded string for the
JSON-encoded propertyFilters field; update the candidate schema's
propertyFilters definition to add a maximum length check (e.g.,
z.string().max(10000) or another appropriate byte/char limit) before .optional()
so oversized payloads are rejected at validation time; modify the
propertyFilters schema entry in server/utils/schemas/candidate.ts (the
propertyFilters z.string() declaration) to include .max(...) and an explicit
error message if desired.
In `@shared/properties.ts`:
- Around line 117-121: The multi_select branch currently casts value to string[]
and calls .map(), which can throw if value is malformed; add a defensive type
guard in the 'multi_select' case to verify Array.isArray(value) and that each
element is a string (or coerce to an empty array) before mapping. Use the
existing PropertySelectConfig/options (opts) and only compute ids = value when
the guard passes (otherwise ids = []), then proceed with opts.find(...).label
logic so .map() is never called on a non-array.
🪄 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: 6a594d4f-68e7-4b10-8b51-bcc244c01a87
📒 Files selected for processing (41)
app/components/AppTopBar.vueapp/components/ColumnsMenu.vueapp/components/FilterDrawer.vueapp/components/JobSubNavActions.vueapp/components/PropertyBlock.vueapp/components/PropertyFilterBar.vueapp/components/PropertySchemaEditor.vueapp/components/PropertyTableCell.vueapp/components/PropertyValueDisplay.vueapp/components/PropertyValueEditor.vueapp/components/SavedViewsMenu.vueapp/composables/useApplications.tsapp/composables/useCandidates.tsapp/composables/useProperties.tsapp/composables/useSavedViews.tsapp/pages/dashboard/applications/[id].vueapp/pages/dashboard/applications/index.vueapp/pages/dashboard/candidates/[id].vueapp/pages/dashboard/candidates/index.vueapp/pages/dashboard/jobs/index.vueserver/api/applications/[id].get.tsserver/api/applications/[id]/properties/[propId].put.tsserver/api/applications/index.get.tsserver/api/candidates/[id].get.tsserver/api/candidates/[id]/properties/[propId].put.tsserver/api/candidates/index.get.tsserver/api/jobs/index.get.tsserver/api/properties/[id].delete.tsserver/api/properties/[id].patch.tsserver/api/properties/index.get.tsserver/api/properties/index.post.tsserver/api/properties/reorder.post.tsserver/database/migrations/0028_custom_properties.sqlserver/database/migrations/meta/0028_snapshot.jsonserver/database/migrations/meta/_journal.jsonserver/database/schema/app.tsserver/utils/properties.tsserver/utils/schemas/application.tsserver/utils/schemas/candidate.tsserver/utils/schemas/property.tsshared/properties.ts
Comment on lines
+24
to
+26
| const views = ref<SavedView<T>[]>([]) | ||
| const activeViewId = ref<string | null>(null) | ||
| const loaded = ref(false) |
There was a problem hiding this comment.
Fix TypeScript errors caused by Vue's ref deep unwrapping.
The pipeline is failing because ref<SavedView<T>[]> causes Vue to unwrap the generic T inside settings, resulting in UnwrapRef<T> instead of T. Since the code uses an immutable pattern (replacing the entire array on each update), shallowRef is the appropriate solution.
🐛 Proposed fix using shallowRef
+import { shallowRef } from 'vue'
+
export function useSavedViews<T extends Record<string, unknown>>(scope: string, defaultSettings: T) {
const storageKey = `${STORAGE_PREFIX}${scope}`
- const views = ref<SavedView<T>[]>([])
+ const views = shallowRef<SavedView<T>[]>([])
const activeViewId = ref<string | null>(null)
const loaded = ref(false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/composables/useSavedViews.ts` around lines 24 - 26, The TypeScript errors
come from Vue's deep unwrapping when using ref<SavedView<T>[]> for the views
array; replace the deep ref with a shallowRef to preserve the generic T type
(use shallowRef<SavedView<T>[]>([])) and import shallowRef from 'vue'; keep
activeViewId and loaded as ref(...) unless you also need shallow behavior;
update any places that set views to assign the whole shallowRef.value = newArray
(same immutable pattern) so types remain correct.
- PropertyValueEditor: replace v-model with cast (invalid esbuild assignment target) with :value + @input handlers for text/long_text/date inputs - useSavedViews: avoid UnwrapRef widening by typing ref<T> via cast
Security: - property URL validator restricts to http(s) schemes (block javascript:/data: XSS) Validation/correctness: - propertyConfigSchema drops passthrough() so malformed configs are rejected - createPropertyDefinitionSchema superRefine rejects jobId for candidate properties - properties POST uses isNull(jobId) instead of eq(jobId, null) so org-global displayOrder works - properties reorder enforces single (entityType, jobId) scope across submitted ids - applications/candidates list endpoints schema-validate propertyFilters (max 20, allow-list ops, reject isEmpty) - entityIdsMatchingFilters rejects isEmpty operator (cannot compute complement here) - loadPropertyEntriesForEntities supports per-entity job filtering via entityJobIds map (no cross-job leak) UX: - saved view settings persist propertyFilters for both applications and candidates pages - PropertyFilterBar splits chip into adjacent buttons so the remove affordance is keyboard/SR-accessible - replace 'as never' casts with PropertyEntry[] on application/candidate detail pages Cleanup: - remove unused sql import in applications/index.get.ts and unused defById in properties.ts - update PUT route docs: pass null to clear (matches validator)
- Implemented ApplicationDetailDrawer.vue for displaying application details, including status transitions, notes editing, and properties. - Implemented CandidateDetailDrawer.vue for displaying candidate details, including contact information, applications, and documents. - Enhanced UI with improved styling and responsiveness. - Added functionality for scheduling interviews and handling document previews. Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
server/api/properties/reorder.post.ts (1)
48-59:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRenumber the full scope, not just the submitted ids.
A partial reorder still leaves omitted definitions with their old
displayOrder, so this can create duplicates inside the same(entityType, jobId)scope. Example: reordering just one item to index0leaves the existing0untouched.Suggested direction
Inside the transaction, load every definition in the verified scope, build the new order as:
- submitted ids in request order
- remaining scope ids in their current relative order
Then rewrite
displayOrderfor the whole scope in one pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/properties/reorder.post.ts` around lines 48 - 59, The current transaction only updates the submitted ids causing duplicate displayOrder values; inside the db.transaction callback (where you use tx, propertyDefinition, ids and orgId) first query all definitions in the verified scope (filter by organizationId and the same scope keys like entityType and jobId) ordered by current displayOrder, then build a new ordering array consisting of the submitted ids in request order followed by the remaining scope ids in their current relative order, and finally iterate that full list to rewrite displayOrder (using tx.update(propertyDefinition).set({ displayOrder: i, updatedAt: new Date() }).where(eq(propertyDefinition.id, id))) for every id so the entire scope is renumbered atomically.app/composables/useSavedViews.ts (1)
22-25: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDon't mask Vue's deep-unwrapping here with a cast.
ref([]) as Ref<SavedView<T>[]>hides the generic unwrapping problem instead of fixing it. Since this composable always replaces the whole array,shallowRef<SavedView<T>[]>([])is the safer choice and keepssettings: Tintact.In Vue 3, does ref() deeply unwrap generic object types, and is shallowRef the recommended approach for preserving generic T in collections like SavedView<T>[]?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useSavedViews.ts` around lines 22 - 25, The current useSavedViews function masks Vue's deep-unwrapping by casting ref([]) to Ref<SavedView<T>[]>, which loses the generic T; change the views declaration to use shallowRef<SavedView<T>[]>([]) instead of ref([]) as Ref<SavedView<T>[]>, import shallowRef from Vue if not present, and keep the rest of the composable (activeViewId, STORAGE_PREFIX, settings: T handling) unchanged so the SavedView<T>[] preserves its generic settings type.
🤖 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/components/CandidateDetailDrawer.vue`:
- Around line 118-130: Save the previously focused element on mount and set
initial focus to the drawer's first focusable element; inside onMounted (and the
existing onKeydown) add logic to trap Tab/Shift+Tab within the drawer by
preventing default and moving focus between the first and last focusable nodes,
and ensure you add any additional keydown handling there; onUnmounted restore
document.body.style.overflow and call focus() on the saved previously focused
element and remove the keydown listener; reference the existing
onMounted/onUnmounted handlers and the onKeydown function and the drawer root
element (select the drawer container element via a ref used in the template) so
the focus-save, initial-focus, Tab-trap, and restoration are implemented and
cleaned up.
- Around line 172-176: The icon-only buttons in CandidateDetailDrawer.vue (e.g.,
the close button with `@click`="emit('close')" containing the <X /> icon and the
other icon-only buttons at the blocks around lines 377, 437, 444) need
accessible names; add an aria-label attribute to each icon-only <button>
describing the action (for the close button use aria-label="Close", and for the
others use clear verbs like "Edit", "Delete", "Expand" or the specific action
they perform) so screen readers can announce the purpose; keep the existing
click handlers (e.g., emit('close')) and icon components unchanged and ensure
aria-label matches the button’s intent.
In `@app/components/PropertyFilterBar.vue`:
- Around line 128-139: The template currently sets ref="editEl" inside a v-for
which makes editEl.value an array; change to a function ref so editEl.value
stays an HTMLElement | null: keep editEl as ref<HTMLElement|null> and in the
template replace ref="editEl" with a function ref like :ref="el => { if (index
=== editingIdx) editEl.value = el as HTMLElement | null }" (or clear it when not
active); keep closeEditOnOutside, watchEffect and editingIdx logic unchanged so
closeEditOnOutside can safely call editEl.value.contains(...).
In `@app/components/PropertySchemaEditor.vue`:
- Around line 42-44: When props.open changes, the watcher currently only calls
refresh() on open; update that watcher (watch(() => props.open, ...)) to also
reset transient refs when it closes by setting formMode and confirmDeleteId back
to their defaults (e.g., formMode.value = 'create' or your component's initial
mode and confirmDeleteId.value = null) so stale UI state doesn't persist; apply
the same reset logic in the other close-path referenced around the
confirm/delete code (the block handling confirmDeleteId and formMode) so both
the initial watcher and the close handler clear these refs.
In `@app/components/PropertyValueEditor.vue`:
- Around line 101-127: Add a guard to prevent duplicate commits by checking the
editing flag at the start of commit(): if editing.value is falsy, return
immediately; this ensures onDocPointerDown() calling commit() before the field's
blur handler doesn't cause a second emit. Update the commit() function (which
currently sets editing.value = false) to begin with this guard so only the first
commit proceeds to normalize and emit via emit('update', ...).
In `@app/pages/dashboard/jobs/`[id]/index.vue:
- Around line 2254-2260: The PropertyBlock's `@refresh` handler currently calls
executeDetailFetch() which only updates the detail payload; update the `@refresh`
handler to also call refreshApps() so the left pipeline list and client-side
property filters are refreshed after property edits—locate the PropertyBlock
usage (entity-type="application", :entity-id="resolvedCurrentApplication.id",
:job-id="jobId") and change the handler to invoke both executeDetailFetch() and
refreshApps() (or call a small wrapper method that runs both).
- Around line 169-188: The property filter logic can shrink the list but doesn't
adjust the selected row index; after applying the property filters (the block
that reads propertyFilters.value and computes result), re-clamp the selection by
updating currentIndex to Math.max(0, Math.min(currentIndex,
filteredApplications.length - 1)) (or call a helper like clampCurrentIndex if
one exists). Ensure you reference the same reactive variables used in this
component (propertyFilters.value, filteredApplications, currentIndex) and
perform the clamp immediately after the filtering so the center pane selection
stays valid.
In `@server/utils/schemas/property.ts`:
- Around line 43-68: The schema currently allows a null/incorrect config
regardless of the property's type, so you must enforce config compatibility with
the property's type: update createPropertyDefinitionSchema to superRefine that
when value.type is 'select' or 'multi_select' the config is non-null and matches
the expected select config shape (use propertyConfigSchema's select branch or
equivalent), and similarly reject number/null configs for those types; for
updates, ensure updatePropertyDefinitionSchema's validation occurs in the PATCH
handler after merging the incoming patch with the existing stored definition
(merge the stored definition's type with the patch, then run a compatibility
check against propertyConfigSchema/expected config for that resolved type) so
validateValueForType won't encounter an incompatible or empty options config at
runtime.
---
Duplicate comments:
In `@app/composables/useSavedViews.ts`:
- Around line 22-25: The current useSavedViews function masks Vue's
deep-unwrapping by casting ref([]) to Ref<SavedView<T>[]>, which loses the
generic T; change the views declaration to use shallowRef<SavedView<T>[]>([])
instead of ref([]) as Ref<SavedView<T>[]>, import shallowRef from Vue if not
present, and keep the rest of the composable (activeViewId, STORAGE_PREFIX,
settings: T handling) unchanged so the SavedView<T>[] preserves its generic
settings type.
In `@server/api/properties/reorder.post.ts`:
- Around line 48-59: The current transaction only updates the submitted ids
causing duplicate displayOrder values; inside the db.transaction callback (where
you use tx, propertyDefinition, ids and orgId) first query all definitions in
the verified scope (filter by organizationId and the same scope keys like
entityType and jobId) ordered by current displayOrder, then build a new ordering
array consisting of the submitted ids in request order followed by the remaining
scope ids in their current relative order, and finally iterate that full list to
rewrite displayOrder (using tx.update(propertyDefinition).set({ displayOrder: i,
updatedAt: new Date() }).where(eq(propertyDefinition.id, id))) for every id so
the entire scope is renumbered atomically.
🪄 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: 9f077884-4364-42ee-877b-2146878d3ad4
📒 Files selected for processing (21)
app/components/ApplicationDetailDrawer.vueapp/components/CandidateDetailDrawer.vueapp/components/CandidateDetailSidebar.vueapp/components/PropertyFilterBar.vueapp/components/PropertySchemaEditor.vueapp/components/PropertyValueEditor.vueapp/composables/useSavedViews.tsapp/pages/dashboard/applications/[id].vueapp/pages/dashboard/applications/index.vueapp/pages/dashboard/candidates/[id].vueapp/pages/dashboard/candidates/index.vueapp/pages/dashboard/jobs/[id]/index.vueapp/pages/dashboard/jobs/index.vueapp/pages/dashboard/timeline.vueserver/api/applications/[id]/properties/[propId].put.tsserver/api/applications/index.get.tsserver/api/candidates/index.get.tsserver/api/properties/index.post.tsserver/api/properties/reorder.post.tsserver/utils/properties.tsserver/utils/schemas/property.ts
✅ Files skipped from review due to trivial changes (1)
- app/pages/dashboard/timeline.vue
🚧 Files skipped from review as they are similar to previous changes (3)
- server/api/properties/index.post.ts
- app/pages/dashboard/candidates/index.vue
- app/pages/dashboard/candidates/[id].vue
Comment on lines
+118
to
+130
| function onKeydown(e: KeyboardEvent) { | ||
| if (e.key === 'Escape') emit('close') | ||
| } | ||
|
|
||
| onMounted(() => { | ||
| document.addEventListener('keydown', onKeydown) | ||
| document.body.style.overflow = 'hidden' | ||
| }) | ||
|
|
||
| onUnmounted(() => { | ||
| document.removeEventListener('keydown', onKeydown) | ||
| document.body.style.overflow = '' | ||
| }) |
There was a problem hiding this comment.
Implement focus trapping/restoration for the modal dialog.
The drawer declares a modal dialog (Line 155) but does not manage focus. Keyboard users can tab into background content, and focus is not restored on close. Add initial focus, tab-trap, and focus restoration to keep modal behavior accessible.
Also applies to: 155-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/CandidateDetailDrawer.vue` around lines 118 - 130, Save the
previously focused element on mount and set initial focus to the drawer's first
focusable element; inside onMounted (and the existing onKeydown) add logic to
trap Tab/Shift+Tab within the drawer by preventing default and moving focus
between the first and last focusable nodes, and ensure you add any additional
keydown handling there; onUnmounted restore document.body.style.overflow and
call focus() on the saved previously focused element and remove the keydown
listener; reference the existing onMounted/onUnmounted handlers and the
onKeydown function and the drawer root element (select the drawer container
element via a ref used in the template) so the focus-save, initial-focus,
Tab-trap, and restoration are implemented and cleaned up.
Comment on lines
+172
to
+176
| <button | ||
| class="rounded-lg p-1.5 text-surface-500 hover:text-surface-700 dark:hover:text-surface-200 hover:bg-surface-100 dark:hover:bg-surface-800 transition-colors" | ||
| @click="emit('close')" | ||
| > | ||
| <X class="size-4" /> |
There was a problem hiding this comment.
Add accessible names to icon-only buttons.
At Line 172, Line 377, Line 437, and Line 444, the buttons are icon-only and currently rely on visuals/title text. Add aria-label so screen readers can announce the action reliably.
Suggested patch
<button
class="rounded-lg p-1.5 text-surface-500 hover:text-surface-700 dark:hover:text-surface-200 hover:bg-surface-100 dark:hover:bg-surface-800 transition-colors"
+ aria-label="Close candidate detail drawer"
`@click`="emit('close')"
>
<X class="size-4" />
</button>
...
<button
v-if="previewDocId"
class="rounded-lg p-1.5 text-surface-400 hover:text-brand-600 hover:bg-surface-100 dark:hover:bg-surface-800 transition-colors"
title="Download"
+ aria-label="Download document"
`@click`="handleDownload(previewDocId!)"
>
<Download class="size-4" />
</button>
...
<button
v-if="doc.mimeType === 'application/pdf'"
class="rounded-lg p-1.5 text-surface-400 hover:text-brand-600 hover:bg-surface-100 dark:hover:bg-surface-800 transition-colors"
title="Preview PDF"
+ aria-label="Preview PDF"
`@click`="handlePreview(doc.id, doc.mimeType)"
>
<Eye class="size-4" />
</button>
<button
class="rounded-lg p-1.5 text-surface-400 hover:text-brand-600 hover:bg-surface-100 dark:hover:bg-surface-800 transition-colors"
title="Download"
+ aria-label="Download document"
`@click`="handleDownload(doc.id)"
>
<Download class="size-4" />
</button>Also applies to: 377-383, 437-450
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/CandidateDetailDrawer.vue` around lines 172 - 176, The
icon-only buttons in CandidateDetailDrawer.vue (e.g., the close button with
`@click`="emit('close')" containing the <X /> icon and the other icon-only buttons
at the blocks around lines 377, 437, 444) need accessible names; add an
aria-label attribute to each icon-only <button> describing the action (for the
close button use aria-label="Close", and for the others use clear verbs like
"Edit", "Delete", "Expand" or the specific action they perform) so screen
readers can announce the purpose; keep the existing click handlers (e.g.,
emit('close')) and icon components unchanged and ensure aria-label matches the
button’s intent.
Comment on lines
+43
to
+68
| export const createPropertyDefinitionSchema = z.object({ | ||
| entityType: z.enum(propertyEntityTypes), | ||
| type: z.enum(propertyTypes), | ||
| name: z.string().min(1, 'Name is required').max(80, 'Name too long'), | ||
| description: z.string().max(500).nullish(), | ||
| jobId: z.string().min(1).nullish(), | ||
| config: propertyConfigSchema.nullish(), | ||
| }).superRefine((value, ctx) => { | ||
| // Candidate properties are always org-global; the loader only reads | ||
| // candidate definitions where jobId IS NULL, so reject job-scoped candidates | ||
| // at the schema boundary to prevent unreadable rows. | ||
| if (value.entityType === 'candidate' && value.jobId) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| path: ['jobId'], | ||
| message: 'Candidate properties cannot be scoped to a job', | ||
| }) | ||
| } | ||
| }) | ||
|
|
||
| export const updatePropertyDefinitionSchema = z.object({ | ||
| name: z.string().min(1).max(80).optional(), | ||
| description: z.string().max(500).nullish(), | ||
| config: propertyConfigSchema.nullish(), | ||
| displayOrder: z.number().int().min(0).optional(), | ||
| }) |
There was a problem hiding this comment.
Validate config against the property type, not as an independent union.
Right now a select / multi_select property can still be created or updated with config: null or a number config. That passes this schema, but validateValueForType() later falls back to options ?? [] and turns the property into one that can never accept a value.
Suggested direction
-export const createPropertyDefinitionSchema = z.object({
+export const createPropertyDefinitionSchema = z.object({
entityType: z.enum(propertyEntityTypes),
type: z.enum(propertyTypes),
name: z.string().min(1, 'Name is required').max(80, 'Name too long'),
description: z.string().max(500).nullish(),
jobId: z.string().min(1).nullish(),
config: propertyConfigSchema.nullish(),
}).superRefine((value, ctx) => {
+ const needsOptions = value.type === 'select' || value.type === 'multi_select'
+ if (needsOptions && !selectConfigSchema.safeParse(value.config).success) {
+ ctx.addIssue({
+ code: z.ZodIssueCode.custom,
+ path: ['config'],
+ message: 'This property type requires select options',
+ })
+ }
+
+ if (value.type === 'number' && value.config != null && !numberConfigSchema.safeParse(value.config).success) {
+ ctx.addIssue({
+ code: z.ZodIssueCode.custom,
+ path: ['config'],
+ message: 'Invalid number property config',
+ })
+ }
+
if (value.entityType === 'candidate' && value.jobId) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
path: ['jobId'],
message: 'Candidate properties cannot be scoped to a job',
})
}
})updatePropertyDefinitionSchema needs the same compatibility check after merging the patch with the stored definition type in the PATCH handler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/utils/schemas/property.ts` around lines 43 - 68, The schema currently
allows a null/incorrect config regardless of the property's type, so you must
enforce config compatibility with the property's type: update
createPropertyDefinitionSchema to superRefine that when value.type is 'select'
or 'multi_select' the config is non-null and matches the expected select config
shape (use propertyConfigSchema's select branch or equivalent), and similarly
reject number/null configs for those types; for updates, ensure
updatePropertyDefinitionSchema's validation occurs in the PATCH handler after
merging the incoming patch with the existing stored definition (merge the stored
definition's type with the patch, then run a compatibility check against
propertyConfigSchema/expected config for that resolved type) so
validateValueForType won't encounter an incompatible or empty options config at
runtime.
… component interactions
…sistency Co-authored-by: Copilot <copilot@github.com>
…d element references and state management Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/components/JobSubNavActions.vue`:
- Around line 311-317: PropertySchemaEditor's changed event is not handled, so
after edits sibling UI (property filter bar / property block) stays stale; add a
handler on the PropertySchemaEditor component (e.g.
`@changed`="onPropertySchemaChanged") or inline (`@changed`="refreshProperties()")
that either triggers the page-level refresh method that reloads property
metadata (call existing method like fetchProperties / loadPropertyDefinitions or
emit a custom event such as $emit('properties-changed') up to the parent) and
also close the editor (showPropertyEditor = false) if desired; locate the
PropertySchemaEditor usage and wire the changed event to call the appropriate
refresh logic so consumers see updates immediately.
In `@app/pages/dashboard/jobs/`[id]/index.vue:
- Around line 169-188: The client-side property filter block should be removed
and the same propertyFilters sent to the applications API so the backend
performs the filtering; update the code that requests applications (the call
that currently sends { jobId, limit: 100 }) to include propertyFilters:
propertyFilters.value (or omit it when empty), and delete the local filter logic
that uses propertyFilters.value and filters result (the entire if
(propertyFilters.value.length > 0) { ... } block). Ensure the request payload
uses the same shape the backend expects and that any UI state relying on the
now-removed local filtering still receives the server-returned filtered list.
🪄 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: 8ac21ac0-41a4-4a94-bbc8-44e35d057d56
📒 Files selected for processing (8)
app/assets/css/main.cssapp/components/JobSubNavActions.vueapp/components/PropertyBlock.vueapp/components/PropertyFilterBar.vueapp/components/PropertySchemaEditor.vueapp/components/PropertyValueEditor.vueapp/pages/dashboard/jobs/[id]/index.vueshared/properties.ts
✅ Files skipped from review due to trivial changes (2)
- app/components/PropertyFilterBar.vue
- app/components/PropertySchemaEditor.vue
🚧 Files skipped from review as they are similar to previous changes (3)
- app/components/PropertyBlock.vue
- app/components/PropertyValueEditor.vue
- shared/properties.ts
Comment on lines
+311
to
+317
| <!-- Property schema editor (per-job application properties) --> | ||
| <PropertySchemaEditor | ||
| :open="showPropertyEditor" | ||
| entity-type="application" | ||
| :job-id="propertyEditorScope === 'job' ? jobId : null" | ||
| @close="showPropertyEditor = false" | ||
| /> |
There was a problem hiding this comment.
Handle the schema editor’s changed event.
PropertySchemaEditor emits changed after schema mutations, but this integration only closes it. On this page that leaves the newly added/renamed/deleted property definitions stale in sibling consumers like the property filter bar and property block until a hard reload. Please refresh the relevant property data, or bubble an event up so the page can do that when changed fires.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/JobSubNavActions.vue` around lines 311 - 317,
PropertySchemaEditor's changed event is not handled, so after edits sibling UI
(property filter bar / property block) stays stale; add a handler on the
PropertySchemaEditor component (e.g. `@changed`="onPropertySchemaChanged") or
inline (`@changed`="refreshProperties()") that either triggers the page-level
refresh method that reloads property metadata (call existing method like
fetchProperties / loadPropertyDefinitions or emit a custom event such as
$emit('properties-changed') up to the parent) and also close the editor
(showPropertyEditor = false) if desired; locate the PropertySchemaEditor usage
and wire the changed event to call the appropriate refresh logic so consumers
see updates immediately.
Comment on lines
+169
to
+188
| // Property filters | ||
| if (propertyFilters.value.length > 0) { | ||
| result = result.filter((app) => { | ||
| const props = (app as any).properties as PropertyEntry[] | undefined ?? [] | ||
| return propertyFilters.value.every((pf) => { | ||
| const entry = props.find((e) => e.definition.id === pf.propertyDefinitionId) | ||
| const val = entry?.value ?? null | ||
| switch (pf.op) { | ||
| case 'isEmpty': return val === null || val === '' || (Array.isArray(val) && val.length === 0) | ||
| case 'isNotEmpty': return val !== null && val !== '' && !(Array.isArray(val) && val.length === 0) | ||
| case 'equals': return String(val ?? '') === String(pf.value ?? '') | ||
| case 'contains': return String(val ?? '').toLowerCase().includes(String(pf.value ?? '').toLowerCase()) | ||
| case 'in': return Array.isArray(pf.value) && Array.isArray(val) | ||
| ? (pf.value as string[]).some((v) => (val as string[]).includes(v)) | ||
| : Array.isArray(pf.value) && (pf.value as string[]).includes(String(val ?? '')) | ||
| default: return true | ||
| } | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Move property filtering to the applications query.
This block filters against app.properties, but the page still fetches /api/applications with only { jobId, limit: 100 }. The API already supports propertyFilters, so doing the match here means results depend on summary rows carrying hydrated property entries and only ever search the first 100 applications. That can hide valid matches or make empty/non-empty filters wrong. Wire propertyFilters into the list request and let the backend own the filtered result set instead of filtering locally here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/pages/dashboard/jobs/`[id]/index.vue around lines 169 - 188, The
client-side property filter block should be removed and the same propertyFilters
sent to the applications API so the backend performs the filtering; update the
code that requests applications (the call that currently sends { jobId, limit:
100 }) to include propertyFilters: propertyFilters.value (or omit it when
empty), and delete the local filter logic that uses propertyFilters.value and
filters result (the entire if (propertyFilters.value.length > 0) { ... } block).
Ensure the request payload uses the same shape the backend expects and that any
UI state relying on the now-removed local filtering still receives the
server-returned filtered list.
… views Co-authored-by: Copilot <copilot@github.com>
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.
properties.tsfor managing custom properties.schemas/property.ts.shared/properties.ts.Co-authored-by: Copilot copilot@github.com
Summary
Type of change
Validation
DCO
Signed-off-by) viagit commit -sSummary by CodeRabbit