Skip to content

add [properties-fe] - filter and sort to unified list filter#633

Merged
danielkweon merged 46 commits intomainfrom
daniel/m-4977--add-properties-frontend-filter-and-sort-to-unified-list-filter
Jan 12, 2026
Merged

add [properties-fe] - filter and sort to unified list filter#633
danielkweon merged 46 commits intomainfrom
daniel/m-4977--add-properties-frontend-filter-and-sort-to-unified-list-filter

Conversation

@danielkweon
Copy link
Copy Markdown
Contributor

add [properties-fe] - filter and sort to unified list filter

@danielkweon danielkweon self-assigned this Dec 15, 2025
@linear
Copy link
Copy Markdown

linear bot commented Dec 15, 2025

@danielkweon danielkweon force-pushed the daniel/m-4977--add-properties-frontend-filter-and-sort-to-unified-list-filter branch from 5d76027 to e07593a Compare January 9, 2026 20:03
@danielkweon danielkweon marked this pull request as ready for review January 9, 2026 20:05
@danielkweon danielkweon requested a review from a team as a code owner January 9, 2026 20:05
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 9, 2026

Copy link
Copy Markdown
Contributor

@sedson sedson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes talked about in MacroChat:

  • fix menus closing on select for operators
  • cleanup multiple files of incorrect click outside pattern
  • remove all cursor-pointer classes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no cursor-pointer

- Add registerClearHandler prop to PropertyFilterControl
- Remove effect watching props.propertyFilters and isSyncing flag
- Parent explicitly calls clearFilters via registered handler on CLEAR
- Restore saved property filters from viewDataStore on mount
- Initialize FilterPropertyPill state from savedData and initialProperty
- Show loading state while fetching property definitions for saved filters
- Fix Save Changes button not disappearing after save (await mutateAsync)
- Fix isViewConfigChanged reactivity for propertyFilters
- Refactor click outside handlers to avoid closure recreation
@danielkweon danielkweon force-pushed the daniel/m-4977--add-properties-frontend-filter-and-sort-to-unified-list-filter branch from 5255a3b to 1a17a77 Compare January 12, 2026 15:17
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Jan 12, 2026

Code Review

I've reviewed this PR and found 4 CLAUDE.md violations that need to be addressed:

1. Direct service client calls (3 instances)

The following files make direct calls to service clients outside the queries package, violating CLAUDE.md line 27-28:

"Going forward, all network calls to serice clients should be done through Tanstack query in the queries package. DO NOT introduce code that calls any client from service-clients outside of the queries package."

a) FilterValueSelect.tsx (lines 36-38) - Direct call to propertiesServiceClient.getPropertyOptions()

b) FilterValueSelectMulti.tsx (lines 39-41) - Direct call to propertiesServiceClient.getPropertyOptions()

c) typeGuards.ts (lines 120-123) - Direct call to propertiesServiceClient.listProperties()

Fix: Create query hooks in the queries/properties package and use those instead of direct service client calls.


2. Use of any type (7 instances)

FilterPropertyPill.tsx uses 'as any' type assertions in 7 places (lines 352, 360, 367, 375, 382, 391, 398), violating CLAUDE.md line 13:

"Type-driven design - Let types guide function composition. DO NOT USE any"

Fix: Properly type the filterAction parameter for each data type case to eliminate the need for 'as any' casts.


Summary: Please refactor to use the queries package for all network calls and fix the type safety issues before merging.

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Jan 12, 2026

Code Review

I've reviewed this PR for bugs and CLAUDE.md compliance. Found the following issues:

Issue 1: Use of any type assertions in FilterPropertyPill.tsx

Lines: 352, 360, 367, 375, 382, 391, 398, 405

The code uses as any type assertions which violates the CLAUDE.md rule:

"Type-driven design - Let types guide function composition. DO NOT USE any"

Reference: CLAUDE.md L13

The buildPartialFilter function contains multiple action: filterAction as any statements that bypass TypeScript's type checking. The proper solution would be to use type guards or type narrowing to ensure the action type matches the data type being filtered.

Location: FilterPropertyPill.tsx#L350-L407


Issue 2: Direct service client call in FilterValueSelect.tsx

Line: 36

The code directly calls propertiesServiceClient.getPropertyOptions() outside the queries package, violating:

"Going forward, all network calls to serice clients should be done through Tanstack query in the queries package. DO NOT introduce code that calls any client from service-clients outside of the queries package."

Reference: CLAUDE.md L27

This network call should be wrapped in a Tanstack query hook and placed in the queries package, then imported and used in the component.

Location: FilterValueSelect.tsx#L34-L38


Issue 3: Direct service client call in FilterValueSelectMulti.tsx

Line: 39

Same issue as above - direct call to propertiesServiceClient.getPropertyOptions() outside the queries package.

Reference: CLAUDE.md L27

Location: FilterValueSelectMulti.tsx#L37-L41


Issue 4: Direct service client call in typeGuards.ts

Line: 120

The new listPropertiesFlat() function directly calls propertiesServiceClient.listProperties() outside the queries package.

Reference: CLAUDE.md L27

Network calls to service clients should be done through the queries package following the pattern in packages/queries/properties/.

Location: typeGuards.ts#L118-L122

@danielkweon danielkweon merged commit 4b04891 into main Jan 12, 2026
22 checks passed
@danielkweon danielkweon deleted the daniel/m-4977--add-properties-frontend-filter-and-sort-to-unified-list-filter branch January 12, 2026 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants