Migrate comment moderation filters to new filter abstraction#27043
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request adds comprehensive comment filtering infrastructure to the posts application. Changes include new comment field definitions with custom codecs, filter parsing and serialization functions supporting timezone-aware date filtering, and refactored filter state management using a canonical 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
codeCraft-Ritik
left a comment
codeCraft-Ritik
left a comment
There was a problem hiding this comment.
Excellent refactor! Bringing comment moderation filters in line with the canonical pipeline is a big improvement
jonatansberg
changed the title
jonatansberg
changed the title
jonatansberg
force-pushed
the
ber-3505-comment-filters-migration
branch
from
9e54b66 to
4b9b6d0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/posts/test/unit/utils/filter-normalization.test.ts (1)
5-11: Consider usingString.rawfor improved readability.Per SonarCloud, the heavily escaped string literals can be more readable using
String.raw:💡 Optional refactor for readability
it('escapes single quotes for NQL strings', () => { - expect(escapeNqlString('can\'t stop')).toBe('\'can\\\'t stop\''); + expect(escapeNqlString("can't stop")).toBe(String.raw`'can\'t stop'`); }); it('escapes backslashes before single quotes for NQL strings', () => { - expect(escapeNqlString('test\\\'value')).toBe('\'test\\\\\\\'value\''); + expect(escapeNqlString(String.raw`test\'value`)).toBe(String.raw`'test\\\'value'`); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/test/unit/utils/filter-normalization.test.ts` around lines 5 - 11, The test assertions use heavily escaped literal strings which reduce readability; update the two expectations in the escapeNqlString unit tests to use String.raw template literals instead of manually escaping backslashes and quotes so the expected strings are clearer while keeping the same runtime value. Locate the tests referencing escapeNqlString (the two it blocks shown) and replace the right-hand expected string literals with equivalent String.raw`` templates that include the surrounding NQL single quotes and the needed backslashes, verifying the tests still assert the same string values.apps/posts/test/unit/views/comments/use-filter-state.test.tsx (1)
74-75: Prefer order-insensitive query assertions.These expectations are semantically about params, not string order. Consider asserting via
URLSearchParamslookups to avoid brittle failures from serialization ordering.✅ More robust assertion pattern
- expect(result.current.query).toBe('thread=is%3Acomment_123&filter=count.reports%3A%3E0'); + const params = new URLSearchParams(result.current.query); + expect(params.get('thread')).toBe('is:comment_123'); + expect(params.get('filter')).toBe('count.reports:>0');- expect(result.current.query).toBe('id=is%3Acomment_123&thread=is%3Acomment_456'); + const params = new URLSearchParams(result.current.query); + expect(params.get('id')).toBe('is:comment_123'); + expect(params.get('thread')).toBe('is:comment_456'); + expect(params.has('filter')).toBe(false);Also applies to: 92-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/test/unit/views/comments/use-filter-state.test.tsx` around lines 74 - 75, The test currently asserts the full serialized query string (expect(result.current.query).toBe(...)) which is order-sensitive; change both assertions (the one using result.current.query at the shown location and the similar assertion around line 92) to parse result.current.query with URLSearchParams and assert individual params and their values (e.g., use new URLSearchParams(result.current.query).get('thread') and .get('filter') or check has()/getAll() as appropriate) so the test validates param semantics instead of serialization order.apps/posts/src/hooks/filter-sources/create-combined-value-source.ts (1)
15-27: Extract fallback-option generation out ofuseOptions.This is the nested branch SonarCloud is flagging. Pulling it into a tiny helper keeps the hook easier to scan and should clear the depth rule without changing behavior.
Refactor sketch
+function getMissingSelectedOptions<T>( + selectedValues: T[], + mergedOptions: FilterOption<T>[], + getMissingSelectedOption?: (selectedValue: T) => FilterOption<T> +): FilterOption<T>[] { + if (!getMissingSelectedOption) { + return []; + } + + return selectedValues.flatMap((selectedValue) => { + return mergedOptions.some(option => option.value === selectedValue) + ? [] + : [getMissingSelectedOption(selectedValue)]; + }); +} + export function createCombinedValueSource<T = string>( useFirstSource: ValueSourceHook<T>, useSecondSource: ValueSourceHook<T>, getMissingSelectedOption?: (selectedValue: T) => FilterOption<T> ): ValueSourceHook<T> { @@ const useOptions = useCallback(({query, selectedValues}: ValueSourceParams<T>): ValueSourceState<T> => { const firstState = firstSource.useOptions({query, selectedValues}); const secondState = secondSource.useOptions({query, selectedValues}); const mergedOptions = mergeFilterOptions(firstState.options, secondState.options); - const fallbackOptions = getMissingSelectedOption ? selectedValues.flatMap((selectedValue) => { - const hasMatch = mergedOptions.some(option => option.value === selectedValue); - - if (hasMatch) { - return []; - } - - return [getMissingSelectedOption(selectedValue)]; - }) : []; + const fallbackOptions = getMissingSelectedOptions( + selectedValues, + mergedOptions, + getMissingSelectedOption + ); return { options: mergeFilterOptions(mergedOptions, fallbackOptions),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/filter-sources/create-combined-value-source.ts` around lines 15 - 27, The nested branch that builds fallbackOptions inside useOptions should be extracted into a small helper to reduce cognitive depth: create a helper (e.g., buildFallbackOptions or getFallbackOptions) that accepts (selectedValues, mergedOptions, getMissingSelectedOption) and returns the same array of missing selected options, then replace the inline logic in useOptions with a call to that helper; ensure you reference and use mergedOptions (from mergeFilterOptions(firstState.options, secondState.options)), selectedValues, and getMissingSelectedOption so behavior of useOptions, firstSource.useOptions, and secondSource.useOptions remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/posts/src/hooks/filter-sources/create-remote-value-source.ts`:
- Around line 92-108: The nested selection/fallback logic inside the useMemo
that builds fallbackOptions should be extracted into a small helper to satisfy
the quality gate: create a pure function (e.g. buildFallbackOptions or
getFallbackOptions) that accepts getMissingSelectedOption, selectedValues and
mergedOptions and returns the computed array (iterate selectedValues, check
mergedOptions some match, call getMissingSelectedOption when missing); then
replace the inline nested logic in fallbackOptions with a call to that helper
inside useMemo and keep the dependency array as [mergedOptions, selectedValues,
getMissingSelectedOption].
In `@apps/posts/src/views/comments/comments.tsx`:
- Around line 22-27: The CommentsPage no longer forwards the legacy "thread"
scope to browse requests because useFilterState only returns filters now; update
CommentsPage to read the thread identifier from the URL (via useSearchParams) or
from singleCommentId and pass it into useBrowseComments so browse requests
receive {filter, thread} (or include thread inside the filters object) instead
of only {filter}. Concretely, locate the CommentsPage component and where
useBrowseComments is called, derive thread = searchParams.get('thread') ||
singleCommentId (or similar) and supply that thread into the hook call (and into
any places that build browse params at the other noted spots around the
component). Ensure setSearchParams behavior stays compatible (preserve thread
when updating filters) and update all instances referenced (the initial use of
useFilterState, the browse invocation, and the other blocks around the 54-56 and
133-155 ranges) to propagate thread consistently.
In `@apps/posts/test/unit/views/comments/comments.test.tsx`:
- Around line 123-129: Add an assertion that the thread browse param is
forwarded to useBrowseComments: mock or spy on useBrowseComments (the hook
imported/used by renderComments) and after calling
renderComments('/?thread=is:comment_456') and firing the filter update, assert
the mocked useBrowseComments was called with a browse params object that
includes thread: 'is:comment_456' (in addition to the existing expect on
'location-search'); this ensures renderComments/useBrowseComments receives the
thread param even after filter updates.
---
Nitpick comments:
In `@apps/posts/src/hooks/filter-sources/create-combined-value-source.ts`:
- Around line 15-27: The nested branch that builds fallbackOptions inside
useOptions should be extracted into a small helper to reduce cognitive depth:
create a helper (e.g., buildFallbackOptions or getFallbackOptions) that accepts
(selectedValues, mergedOptions, getMissingSelectedOption) and returns the same
array of missing selected options, then replace the inline logic in useOptions
with a call to that helper; ensure you reference and use mergedOptions (from
mergeFilterOptions(firstState.options, secondState.options)), selectedValues,
and getMissingSelectedOption so behavior of useOptions, firstSource.useOptions,
and secondSource.useOptions remains unchanged.
In `@apps/posts/test/unit/utils/filter-normalization.test.ts`:
- Around line 5-11: The test assertions use heavily escaped literal strings
which reduce readability; update the two expectations in the escapeNqlString
unit tests to use String.raw template literals instead of manually escaping
backslashes and quotes so the expected strings are clearer while keeping the
same runtime value. Locate the tests referencing escapeNqlString (the two it
blocks shown) and replace the right-hand expected string literals with
equivalent String.raw`` templates that include the surrounding NQL single quotes
and the needed backslashes, verifying the tests still assert the same string
values.
In `@apps/posts/test/unit/views/comments/use-filter-state.test.tsx`:
- Around line 74-75: The test currently asserts the full serialized query string
(expect(result.current.query).toBe(...)) which is order-sensitive; change both
assertions (the one using result.current.query at the shown location and the
similar assertion around line 92) to parse result.current.query with
URLSearchParams and assert individual params and their values (e.g., use new
URLSearchParams(result.current.query).get('thread') and .get('filter') or check
has()/getAll() as appropriate) so the test validates param semantics instead of
serialization order.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db6e8c5f-5e2b-4d14-be3c-202fafcd4fd0
📒 Files selected for processing (25)
apps/posts/src/hooks/filter-sources/create-combined-value-source.tsapps/posts/src/hooks/filter-sources/create-ghost-browse-value-source.tsapps/posts/src/hooks/filter-sources/create-remote-value-source.tsapps/posts/src/hooks/filter-sources/use-member-value-source.tsapps/posts/src/hooks/filter-sources/use-post-resource-value-source.tsapps/posts/src/views/comments/comment-fields.tsapps/posts/src/views/comments/comment-filter-query.tsapps/posts/src/views/comments/comments.tsxapps/posts/src/views/comments/components/comments-filters.tsxapps/posts/src/views/comments/hooks/use-filter-state.tsapps/posts/src/views/comments/use-comment-filter-fields.tsapps/posts/src/views/filters/filter-normalization.test.tsapps/posts/src/views/filters/filter-normalization.tsapps/posts/src/views/filters/filter-operator-options.tsapps/posts/src/views/members/member-fields.tsapps/posts/src/views/members/use-member-filter-fields.tsapps/posts/test/unit/hooks/create-combined-value-source.test.tsxapps/posts/test/unit/hooks/create-remote-value-source.test.tsxapps/posts/test/unit/utils/filter-normalization.test.tsapps/posts/test/unit/views/comments/comment-fields.test.tsapps/posts/test/unit/views/comments/comment-filter-query.test.tsapps/posts/test/unit/views/comments/comments.test.tsxapps/posts/test/unit/views/comments/use-filter-state.test.tsxapps/posts/vite.config.mjsapps/posts/vitest.config.ts
💤 Files with no reviewable changes (2)
- apps/posts/src/views/filters/filter-normalization.test.ts
- apps/posts/vitest.config.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b9b6d0002
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (singleCommentId) { | ||
| return `id:${escapeNqlString(singleCommentId)}`; | ||
| } |
There was a problem hiding this comment.
When id=is:<comment> is present, this branch always forces effectiveFilter to id:'...' and drops nql, so row-level quick filters (author/post) update URL state but never affect the API query while single-comment mode is active. In practice, moderators can click filter affordances in the comment row and get no result change until they manually leave single mode, which is a behavior regression introduced by separating id from filter state.
Useful? React with 👍 / 👎.
ref #27043 Preserves thread-scoped browsing and exits single-comment mode when row filters are applied
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7c9a3d500
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const {data: settingsData, isLoading: isSettingsLoading} = useBrowseSettings({}); | ||
| const singleCommentId = useMemo(() => getSingleCommentIdParam(searchParams), [searchParams]); | ||
| const filterParam = searchParams.get('filter') ?? undefined; | ||
| const shouldDelayHydration = !singleCommentId && shouldDelayCommentDateFilterHydration(filterParam, Boolean(settingsData), isSettingsLoading); |
There was a problem hiding this comment.
This hydration gate only waits when a filter query param already exists, so the page can render with fallback UTC while settings are still loading. In that window, adding a created_at filter serializes UTC day bounds; after settings resolve, parseCommentFilter no longer matches those bounds for the real site timezone and drops the predicate, so the date filter silently disappears and the API query is cleared. This is a user-visible race on first load for slow settings responses.
Useful? React with 👍 / 👎.
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 `@apps/posts/src/views/comments/comments.tsx`:
- Around line 74-81: The reset key for CommentsList only uses effectiveFilter
but must include the thread identifier too; update the logic that computes
resetKey (the variable named resetKey near the CommentsList usage) to
incorporate threadParam along with effectiveFilter (e.g. combine threadParam and
effectiveFilter into a single string or tuple) so the list resets when either
the thread or filter changes; apply the same change to the other occurrence
referenced around line 137 where resetKey is computed/used.
- Around line 61-63: handleShowAllComments currently clears the entire query
string which loses the thread scope; modify it to only remove the single-comment
identifier instead of replacing all params by creating a copy of the existing
URLSearchParams, deleting the 'id' (or whatever single-comment param is used)
while leaving 'thread' and other params intact, and then calling setSearchParams
with that modified params object (keeping {replace: false}) so exiting
single-comment mode preserves thread context.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e8ed441-ed5a-4da0-a4dd-4d906ff68588
📒 Files selected for processing (3)
apps/posts/src/hooks/filter-sources/create-remote-value-source.tsapps/posts/src/views/comments/comments.tsxapps/posts/test/unit/views/comments/comments.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/posts/test/unit/views/comments/comments.test.tsx
| const handleShowAllComments = useCallback(() => { | ||
| setSearchParams(new URLSearchParams(), {replace: false}); | ||
| }, [setSearchParams]); |
There was a problem hiding this comment.
Preserve the current thread scope when leaving single-comment mode.
Line 62 clears the whole query string, so a URL like ?thread=...&id=... loses its thread context here even though the row-filter path above preserves it. If this action is only meant to exit single-comment mode, remove the relevant params instead of replacing everything.
Suggested fix
const handleShowAllComments = useCallback(() => {
- setSearchParams(new URLSearchParams(), {replace: false});
- }, [setSearchParams]);
+ const nextSearchParams = new URLSearchParams(searchParams);
+ nextSearchParams.delete('id');
+ nextSearchParams.delete('filter');
+ setSearchParams(nextSearchParams, {replace: false});
+ }, [searchParams, setSearchParams]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleShowAllComments = useCallback(() => { | |
| setSearchParams(new URLSearchParams(), {replace: false}); | |
| }, [setSearchParams]); | |
| const handleShowAllComments = useCallback(() => { | |
| const nextSearchParams = new URLSearchParams(searchParams); | |
| nextSearchParams.delete('id'); | |
| nextSearchParams.delete('filter'); | |
| setSearchParams(nextSearchParams, {replace: false}); | |
| }, [searchParams, setSearchParams]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/posts/src/views/comments/comments.tsx` around lines 61 - 63,
handleShowAllComments currently clears the entire query string which loses the
thread scope; modify it to only remove the single-comment identifier instead of
replacing all params by creating a copy of the existing URLSearchParams,
deleting the 'id' (or whatever single-comment param is used) while leaving
'thread' and other params intact, and then calling setSearchParams with that
modified params object (keeping {replace: false}) so exiting single-comment mode
preserves thread context.
| searchParams: { | ||
| ...(threadParam && !singleCommentId ? {thread: threadParam} : {}), | ||
| ...(effectiveFilter ? {filter: effectiveFilter} : {}) | ||
| }, | ||
| keepPreviousData: true | ||
| }); | ||
| // If we are fetching comments, but not fetching the next page and not refetching, we should show the loading indicator | ||
| const shouldShowLoading = isFetching && !isFetchingNextPage && !isRefetching; | ||
| const resetKey = effectiveFilter ?? ''; |
There was a problem hiding this comment.
Include thread in the list reset key.
The browse query changes with both thread and filter here, but Line 81 only keys CommentsList by effectiveFilter. Switching between thread-scoped URLs with the same filter leaves resetKey unchanged, so any list state tied to that prop will carry across to the wrong dataset.
Suggested fix
- const resetKey = effectiveFilter ?? '';
+ const resetKey = singleCommentId
+ ? (effectiveFilter ?? '')
+ : `${threadParam ?? ''}|${effectiveFilter ?? ''}`;Also applies to: 137-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/posts/src/views/comments/comments.tsx` around lines 74 - 81, The reset
key for CommentsList only uses effectiveFilter but must include the thread
identifier too; update the logic that computes resetKey (the variable named
resetKey near the CommentsList usage) to incorporate threadParam along with
effectiveFilter (e.g. combine threadParam and effectiveFilter into a single
string or tuple) so the list resets when either the thread or filter changes;
apply the same change to the other occurrence referenced around line 137 where
resetKey is computed/used.
ref #27043 Preserves thread-scoped browsing and exits single-comment mode when row filters are applied
jonatansberg
force-pushed
the
ber-3505-comment-filters-migration
branch
2 times, most recently
from
c82ec7d to
7b007f8
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/hooks/pre-commit (1)
40-53:⚠️ Potential issue | 🟡 MinorUse staged-index check instead of working-tree check for empty-commit detection.
Line 51 uses
git status --porcelain, which includes unstaged changes. If staged changes are empty but unstaged files exist, this block won’t catch the “nothing staged to commit” case.Proposed fix
- if output=$(git status --porcelain) && [ -z "$output" ]; then + if git diff --cached --quiet --exit-code; then echo -e "nothing to commit, working tree clean" exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/hooks/pre-commit around lines 40 - 53, The pre-commit hook currently checks the working tree with `git status --porcelain` after removing MOD_SUBMODULES, which misses the case where there are unstaged changes but no staged changes; change the empty-commit detection to inspect the staged index instead (use git diff --cached --name-only or git diff --cached --quiet) and if the staged diff is empty print the "nothing to commit, working tree clean" message and exit 1; update the block that follows MOD_SUBMODULES handling (the if that assigns output and checks -z) to use the staged-index check and preserve the same message and exit behavior.
🧹 Nitpick comments (1)
apps/posts/src/hooks/filter-sources/create-combined-value-source.ts (1)
19-27: Consider reusing the fallback builder to avoid logic drift.Line 19 duplicates the same missing-selected synthesis now in
create-remote-value-source.ts. A shared helper would reduce divergence risk between remote and combined value-source behavior.♻️ Suggested direction
-const fallbackOptions = getMissingSelectedOption ? selectedValues.flatMap((selectedValue) => { - const hasMatch = mergedOptions.some(option => option.value === selectedValue); - - if (hasMatch) { - return []; - } - - return [getMissingSelectedOption(selectedValue)]; -}) : []; +const fallbackOptions = buildFallbackOptions( + selectedValues, + mergedOptions, + getMissingSelectedOption +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/filter-sources/create-combined-value-source.ts` around lines 19 - 27, This duplicate missing-selected synthesis logic (the getMissingSelectedOption usage that builds fallbackOptions from selectedValues and mergedOptions) should be extracted into a shared helper and reused to avoid drift; create or import a function like buildMissingSelectedFallback(selectedValues, mergedOptions, getMissingSelectedOption) (or reuse the helper from create-remote-value-source.ts), replace the inline flatMap in create-combined-value-source.ts with a call to that helper, and update create-remote-value-source.ts to call the same helper so both code paths share the identical logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/hooks/pre-commit:
- Around line 40-53: The pre-commit hook currently checks the working tree with
`git status --porcelain` after removing MOD_SUBMODULES, which misses the case
where there are unstaged changes but no staged changes; change the empty-commit
detection to inspect the staged index instead (use git diff --cached --name-only
or git diff --cached --quiet) and if the staged diff is empty print the "nothing
to commit, working tree clean" message and exit 1; update the block that follows
MOD_SUBMODULES handling (the if that assigns output and checks -z) to use the
staged-index check and preserve the same message and exit behavior.
---
Nitpick comments:
In `@apps/posts/src/hooks/filter-sources/create-combined-value-source.ts`:
- Around line 19-27: This duplicate missing-selected synthesis logic (the
getMissingSelectedOption usage that builds fallbackOptions from selectedValues
and mergedOptions) should be extracted into a shared helper and reused to avoid
drift; create or import a function like
buildMissingSelectedFallback(selectedValues, mergedOptions,
getMissingSelectedOption) (or reuse the helper from
create-remote-value-source.ts), replace the inline flatMap in
create-combined-value-source.ts with a call to that helper, and update
create-remote-value-source.ts to call the same helper so both code paths share
the identical logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c252334d-2786-481f-958e-065fbe58125e
📒 Files selected for processing (7)
.github/hooks/pre-commit.lintstagedrc.cjsapps/posts/src/hooks/filter-sources/create-combined-value-source.tsapps/posts/src/hooks/filter-sources/create-ghost-browse-value-source.tsapps/posts/src/hooks/filter-sources/create-remote-value-source.tsapps/posts/src/hooks/filter-sources/use-member-value-source.tsapps/posts/src/hooks/filter-sources/use-post-resource-value-source.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/posts/src/hooks/filter-sources/use-member-value-source.ts
- apps/posts/src/hooks/filter-sources/use-post-resource-value-source.ts
- apps/posts/src/hooks/filter-sources/create-ghost-browse-value-source.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/posts/src/views/comments/comments.tsx (1)
29-29:⚠️ Potential issue | 🟠 MajorThread-scoped deep links are not being propagated to browse params.
Line 75 only forwards
filter, and Line 81 keys resets only byeffectiveFilter. If?thread=is:...is intended to scope browsing, this drops scope at fetch time and can preserve stale list state across thread switches.Suggested fix
const CommentsPage: React.FC<{timezone: string; singleCommentId?: string}> = ({ timezone, singleCommentId }) => { const [searchParams, setSearchParams] = useSearchParams(); + const threadParam = searchParams.get('thread') ?? undefined; const {filters, nql, setFilters} = useFilterState(timezone); @@ } = useBrowseComments({ searchParams: { + ...(!singleCommentId && threadParam ? {thread: threadParam} : {}), ...(effectiveFilter ? {filter: effectiveFilter} : {}) }, keepPreviousData: true }); @@ - const resetKey = effectiveFilter ?? ''; + const resetKey = singleCommentId + ? (effectiveFilter ?? '') + : `${threadParam ?? ''}|${effectiveFilter ?? ''}`;#!/bin/bash # Verify whether thread-scoped browse is expected and whether it's forwarded from this page. # 1) Inspect call sites and params passed to useBrowseComments rg -nP --type=ts -C3 '\buseBrowseComments\s*\(' # 2) Inspect this file for thread extraction/forwarding rg -nP --type=ts -C3 '\bthread\b' apps/posts/src/views/comments/comments.tsx # 3) Inspect available type/contracts around browse search params for comments rg -nP --type=ts -C4 'searchParams\s*:\s*\{|thread\??\s*:'Also applies to: 74-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/comments/comments.tsx` at line 29, The browse call is only forwarding `filter` and the reset key is based only on `effectiveFilter`, dropping any `thread` scope from `useSearchParams`; update the forwarding and reset logic to include the thread-scoped param so thread-scoped deep links persist. Concretely, read the thread value from `useSearchParams()` (e.g. `const thread = searchParams.get('thread')`) and include it in the params passed into `useBrowseComments(...)` and in the reset/cache key (alongside `effectiveFilter`), and ensure any `setSearchParams` logic preserves or clears the `thread` param consistently when changing filters. Reference symbols: useSearchParams, searchParams, useBrowseComments, filter, effectiveFilter, setSearchParams.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/posts/src/views/comments/comments.tsx`:
- Line 29: The browse call is only forwarding `filter` and the reset key is
based only on `effectiveFilter`, dropping any `thread` scope from
`useSearchParams`; update the forwarding and reset logic to include the
thread-scoped param so thread-scoped deep links persist. Concretely, read the
thread value from `useSearchParams()` (e.g. `const thread =
searchParams.get('thread')`) and include it in the params passed into
`useBrowseComments(...)` and in the reset/cache key (alongside
`effectiveFilter`), and ensure any `setSearchParams` logic preserves or clears
the `thread` param consistently when changing filters. Reference symbols:
useSearchParams, searchParams, useBrowseComments, filter, effectiveFilter,
setSearchParams.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f7fb471-d5bf-4df7-83c1-82e68c8fc5e0
📒 Files selected for processing (8)
apps/posts/src/views/comments/comments.tsxapps/posts/src/views/comments/components/comments-filters.tsxapps/posts/src/views/comments/hooks/use-filter-state.tsapps/posts/src/views/comments/legacy-comment-filter-query.tsapps/posts/src/views/comments/use-comment-filter-fields.tsapps/posts/test/unit/views/comments/comments.test.tsxapps/posts/test/unit/views/comments/use-comment-filter-fields.test.tsxapps/posts/test/unit/views/comments/use-filter-state.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/posts/src/views/comments/components/comments-filters.tsx
- apps/posts/src/views/comments/use-comment-filter-fields.ts
- apps/posts/test/unit/views/comments/use-filter-state.test.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a60249ac07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const LEGACY_OPERATOR_MAP: Record<string, string> = { | ||
| is_not: 'is-not', | ||
| not_contains: 'does-not-contain', | ||
| before: 'is-less', | ||
| after: 'is-greater', | ||
| on_or_before: 'is-or-less', | ||
| on_or_after: 'is-or-greater' |
There was a problem hiding this comment.
Legacy comment URLs can contain created_at=is:<date> from the previous comments filter UI, but parseLegacyCommentFilters does not remap is to any supported date operator. That leaves a created_at predicate with operator is, which dateCodec.serialize cannot encode, so the migration pass drops the date clause and rewrites the URL without the user’s date filter. In practice, opening an old moderation link with an exact date silently broadens results to unfiltered comments.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27043 +/- ##
=======================================
Coverage 73.19% 73.19%
=======================================
Files 1561 1561
Lines 127072 127072
Branches 15394 15396 +2
=======================================
Hits 93011 93011
- Misses 33082 33102 +20
+ Partials 979 959 -20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ref https://linear.app/ghost/issue/BER-3505/migrate-comment-moderation-filters-to-new-filter-primitives Comments needed to match the members filter pipeline while keeping legacy single-comment deep links and separating thread and id state from canonical filter URLs.
ref https://linear.app/ghost/issue/BER-3505/migrate-comment-moderation-filters-to-new-filter-primitives Comments author and post filters should stay representable when selected records are deleted or inaccessible, and the follow-up filter-state helpers were simplified without changing the intended comments flow.
ref https://linear.app/ghost/issue/BER-3505/migrate-comment-moderation-filters-to-new-filter-primitives The combined post/page value source should prefer real hydrated labels over synthetic ID fallbacks so comment filters stay readable when one side can still resolve the selected resource.
ref https://linear.app/ghost/issue/BER-3505/migrate-comment-moderation-filters-to-new-filter-primitives The PR introduced two TypeScript regressions in apps/posts: an optional fallback callback was invoked unsafely and the comments page test used a spread signature that failed strict compilation in CI.
ref https://linear.app/ghost/issue/BER-3505/migrate-comment-moderation-filters-to-new-filter-primitives The comments filter follow-up no longer needs a separate Vitest config, so the app test setup now lives in the main Vite config and the new tests were rewritten to rely on the existing @src pathing.
ref #27043 Preserves thread-scoped browsing and exits single-comment mode when row filters are applied
ref https://linear.app/ghost/issue/BER-3505/migrate-comment-moderation-filters-to-new-filter-primitives Aligned comments filtering with members by preserving draft filter state, using site timezone date defaults, keeping thread as URL state, and isolating temporary legacy URL fallback code.
ref https://linear.app/ghost/issue/BER-3505/migrate-comment-moderation-filters-to-new-filter-primitives The comment filters branch should keep the existing Posts Vitest config instead of moving test setup into the Vite config.
ref https://linear.app/ghost/issue/BER-3505/migrate-comment-moderation-filters-to-new-filter-primitives Aligned comments and members on the shared date codec and removed divergent comment component tests before the filter state refactor.
jonatansberg
force-pushed
the
ber-3505-comment-filters-migration
branch
from
a60249a to
344ab8b
Compare
|
All working fine locally. Only thing I found was the comment text filtering not having any debounce so we fire an API request on every keypress but I think that was an existing issue? |
3 participants
What changed
This reworks comment moderation filters in
apps/poststo use the same canonical field/codec/NQL pipeline as members instead of the old ad-hoc query-param implementation.It also keeps
idandthreadout of filter state:filteris now the only concern of the comment filter-state hook?id=is:<commentId>deep links are handled separately at the page levelFollow-up work on this branch also tightened the shared value-source behavior so selected author/post filters remain representable when the backing record cannot be hydrated, without allowing synthetic
ID: ...placeholders to win over real hydrated labels in the combined post/page source.Why
The previous comments filter implementation had a hand-rolled
buildNqlFilter()path, custom URL shape, no NQL parsing, and browser-local date handling. That had drifted away from the mainline filter abstractions and made comments harder to reason about and maintain.This brings comments back onto the current shared model while keeping the deep-link behavior that moderation flows still rely on.
User impact
?filter=<NQL>URLs?id=is:<commentId>still workValidation
cd apps/posts && yarn vitest run test/unit/hooks/create-remote-value-source.test.tsx test/unit/hooks/create-combined-value-source.test.tsx test/unit/views/comments test/unit/utils/filter-normalization.test.tscd apps/posts && yarn eslint --ext .ts,.tsx src/hooks/filter-sources/create-combined-value-source.ts src/hooks/filter-sources/use-post-resource-value-source.ts test/unit/hooks/create-combined-value-source.test.tsxcd apps/posts && yarn tsc --noEmitNotes
?id=is:<commentId>in this pass.claude/worktrees/is an unrelated untracked local directory and is not part of this PR