Fix ClickUp view filter payload formatting - #31
Lovlace777 wants to merge 1 commit into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughAdds ChangesFilter Operator Mapping and Payload Normalization
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 2
🧹 Nitpick comments (1)
packages/core/src/tests/views-filter-payload.test.ts (1)
64-79: ⚡ Quick winAdd a direct
updateViewfilter payload regression test.The bug scope includes
updateViewas a caller offormatFilters, but this suite currently validates onlycreateViewandsetViewFilters. Add one explicitupdateViewassertion so all affected entry points are locked.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/tests/views-filter-payload.test.ts` around lines 64 - 79, The test suite is missing explicit coverage for updateView, which is also a caller of formatFilters that needs validation. Add a new test case in this file that calls client.updateView with filters (similar to the existing setViewFilters test shown in the diff), and assert that the filter payload is correctly transformed with proper operator names (like NOT ANY and IS NOT SET) and values in the putCalls data. This ensures updateView is locked into the regression test coverage alongside createView and setViewFilters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/clickup-client/views-enhanced.ts`:
- Around line 19-30: Change the CLICKUP_FILTER_OPERATOR_MAP type from
Partial<Record<FilterOperator, string>> to a total Record<FilterOperator,
string> to require all FilterOperator values to be mapped. Additionally, locate
where this map is used with a fallback operator (such as the `??
filter.operator` pattern mentioned around lines 334-335) and remove the fallback
entirely. This will ensure all FilterOperator cases are handled at compile time
rather than risking runtime 400 errors from unmapped operators being sent in
payloads.
- Around line 342-355: The formatFilterValues method currently checks for
filter.values and array filter.value before validating the operator type,
allowing is_set and is_not_set operators to emit non-null values when callers
provide them, which contradicts the intended ClickUp encoding. Reorder the
checks in formatFilterValues so that the operator validation for is_set and
is_not_set comes first, before any checks for filter.values or filter.value,
ensuring these operators always return [null] regardless of input values
provided.
---
Nitpick comments:
In `@packages/core/src/tests/views-filter-payload.test.ts`:
- Around line 64-79: The test suite is missing explicit coverage for updateView,
which is also a caller of formatFilters that needs validation. Add a new test
case in this file that calls client.updateView with filters (similar to the
existing setViewFilters test shown in the diff), and assert that the filter
payload is correctly transformed with proper operator names (like NOT ANY and IS
NOT SET) and values in the putCalls data. This ensures updateView is locked into
the regression test coverage alongside createView and setViewFilters.
🪄 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: 5849a5fd-5ca6-45d6-ab5f-18e7c00ae5c7
📒 Files selected for processing (2)
packages/core/src/clickup-client/views-enhanced.tspackages/core/src/tests/views-filter-payload.test.ts
| const CLICKUP_FILTER_OPERATOR_MAP: Partial<Record<FilterOperator, string>> = { | ||
| equals: 'EQ', | ||
| not_equals: 'NOT', | ||
| greater_than: 'GT', | ||
| less_than: 'LT', | ||
| greater_than_or_equal: 'GTE', | ||
| less_than_or_equal: 'LTE', | ||
| in: 'ANY', | ||
| not_in: 'NOT ANY', | ||
| is_set: 'IS SET', | ||
| is_not_set: 'IS NOT SET' | ||
| }; |
There was a problem hiding this comment.
Make operator mapping exhaustive and remove silent fallback.
Using Partial<Record<...>> with ?? filter.operator can emit raw schema operators for unmapped cases, which risks reintroducing 400s for valid FilterOperator inputs. Make this a total Record<FilterOperator, string> and drop the fallback so unmapped operators fail at compile time instead of runtime payload errors.
Suggested diff
-const CLICKUP_FILTER_OPERATOR_MAP: Partial<Record<FilterOperator, string>> = {
+const CLICKUP_FILTER_OPERATOR_MAP: Record<FilterOperator, string> = {
equals: 'EQ',
not_equals: 'NOT',
+ contains: 'CONTAINS',
+ not_contains: 'NOT CONTAINS',
+ starts_with: 'STARTS WITH',
+ ends_with: 'ENDS WITH',
+ is_empty: 'IS EMPTY',
+ is_not_empty: 'IS NOT EMPTY',
greater_than: 'GT',
less_than: 'LT',
greater_than_or_equal: 'GTE',
less_than_or_equal: 'LTE',
+ between: 'BETWEEN',
+ not_between: 'NOT BETWEEN',
in: 'ANY',
not_in: 'NOT ANY',
is_set: 'IS SET',
is_not_set: 'IS NOT SET'
};
...
- op: CLICKUP_FILTER_OPERATOR_MAP[filter.operator] ?? filter.operator,
+ op: CLICKUP_FILTER_OPERATOR_MAP[filter.operator],Also applies to: 334-335
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/clickup-client/views-enhanced.ts` around lines 19 - 30,
Change the CLICKUP_FILTER_OPERATOR_MAP type from Partial<Record<FilterOperator,
string>> to a total Record<FilterOperator, string> to require all FilterOperator
values to be mapped. Additionally, locate where this map is used with a fallback
operator (such as the `?? filter.operator` pattern mentioned around lines
334-335) and remove the fallback entirely. This will ensure all FilterOperator
cases are handled at compile time rather than risking runtime 400 errors from
unmapped operators being sent in payloads.
| private formatFilterValues(filter: ViewFilter): Array<string | number | null> { | ||
| if (filter.values !== undefined) { | ||
| return filter.values; | ||
| } | ||
|
|
||
| if (Array.isArray(filter.value)) { | ||
| return filter.value; | ||
| } | ||
|
|
||
| if (filter.operator === 'is_set' || filter.operator === 'is_not_set') { | ||
| return [null]; | ||
| } | ||
|
|
||
| return filter.value !== undefined ? [filter.value] : []; |
There was a problem hiding this comment.
Force [null] for is_set/is_not_set before reading input values.
formatFilterValues currently gives precedence to filter.values/array filter.value, so is_set/is_not_set can emit non-[null] payloads when callers provide values. This contradicts the intended ClickUp encoding and can produce invalid requests.
Suggested diff
private formatFilterValues(filter: ViewFilter): Array<string | number | null> {
+ if (filter.operator === 'is_set' || filter.operator === 'is_not_set') {
+ return [null];
+ }
+
if (filter.values !== undefined) {
return filter.values;
}
if (Array.isArray(filter.value)) {
return filter.value;
}
- if (filter.operator === 'is_set' || filter.operator === 'is_not_set') {
- return [null];
- }
-
return filter.value !== undefined ? [filter.value] : [];
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/clickup-client/views-enhanced.ts` around lines 342 - 355,
The formatFilterValues method currently checks for filter.values and array
filter.value before validating the operator type, allowing is_set and is_not_set
operators to emit non-null values when callers provide them, which contradicts
the intended ClickUp encoding. Reorder the checks in formatFilterValues so that
the operator validation for is_set and is_not_set comes first, before any checks
for filter.values or filter.value, ensuring these operators always return [null]
regardless of input values provided.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/clickup-client/views-enhanced.ts">
<violation number="1" location="packages/core/src/clickup-client/views-enhanced.ts:19">
P1: Operator map is non-exhaustive and silently falls back to raw schema names for unmapped operators, risking silent API failures.</violation>
<violation number="2" location="packages/core/src/clickup-client/views-enhanced.ts:343">
P2: Set/unset operators do not always force `[null]` because earlier branches return caller-supplied `values`/array `value`</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // ViewSettings | ||
| } from '../schemas/views-schemas.js'; | ||
|
|
||
| const CLICKUP_FILTER_OPERATOR_MAP: Partial<Record<FilterOperator, string>> = { |
There was a problem hiding this comment.
P1: Operator map is non-exhaustive and silently falls back to raw schema names for unmapped operators, risking silent API failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/clickup-client/views-enhanced.ts, line 19:
<comment>Operator map is non-exhaustive and silently falls back to raw schema names for unmapped operators, risking silent API failures.</comment>
<file context>
@@ -10,11 +10,25 @@ import type {
// ViewSettings
} from '../schemas/views-schemas.js';
+const CLICKUP_FILTER_OPERATOR_MAP: Partial<Record<FilterOperator, string>> = {
+ equals: 'EQ',
+ not_equals: 'NOT',
</file context>
| } | ||
|
|
||
| private formatFilterValues(filter: ViewFilter): Array<string | number | null> { | ||
| if (filter.values !== undefined) { |
There was a problem hiding this comment.
P2: Set/unset operators do not always force [null] because earlier branches return caller-supplied values/array value
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/clickup-client/views-enhanced.ts, line 343:
<comment>Set/unset operators do not always force `[null]` because earlier branches return caller-supplied `values`/array `value`</comment>
<file context>
@@ -317,14 +331,30 @@ export class ViewsEnhancedClient extends ClickUpClient {
}
+ private formatFilterValues(filter: ViewFilter): Array<string | number | null> {
+ if (filter.values !== undefined) {
+ return filter.values;
+ }
</file context>
|
Thanks for this! Same as #30 — the view-filter Generated by Claude Code |
2 participants
Summary
opandvalues[null]values payloadFixes #29
Validation
npx jest --config packages/core/jest.config.js --runInBand --coverage=false- passed 11 suites and 180 testsnpm run build --workspace=packages/core- passednpx eslint packages/core/src/clickup-client/views-enhanced.ts packages/core/src/tests/views-filter-payload.test.ts- passed with one pre-existing max-line-length warning onviews-enhanced.ts:2git diff --check- passednpm cireached the workspacepreparebuild and failed on existing TypeScript errors inpackages/intelligence; core dependencies were installed separately for the checks above, without modifying the lockfileNotes
Summary by cubic
Fixes ClickUp view filter payloads to match the API and prevent rejected requests. We now send
opandvalues, map operators, preserve falsy scalars, and encode set/unset as[null].op,values) and map operators:EQ,NOT,GT,LT,GTE,LTE,ANY,NOT ANY,IS SET,IS NOT SET.valuesarray; preserve falsy scalars; support arrays; encode set/unset as[null].Written for commit 3867dcd. Summary will update on new commits.