Skip to content

fix(platform): filter feedback comments before pagination - #4328

Merged
yannickmonney merged 1 commit into
mainfrom
fix/feedback-comments-pagination
Oct 6, 2026
Merged

yannickmonney merged 1 commit into
mainfrom
fix/feedback-comments-pagination

Conversation

@yannickmonney

@yannickmonney yannickmonney commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

What changed

  • Apply Comments only in the feedback SQL query before keyset pagination and LIMIT, so older matching comments are returned even when 25 newer ratings have no explanation.
  • Preserve NULL/empty-string exclusion, whitespace comments, tenant/lifecycle/date/attribution scopes, ordering, cursor behavior and unfiltered results. Shared DataTable and the unrelated kind filter are unchanged.
  • Add six focused regression cases covering the SQL boundary, first matching page, matching-row continuation and unfiltered ratings.

Verification

  • Installed Node, one Vitest worker: feedback service/route/access and feedback reducer suites (31 tests passed).
  • New regression on origin/main b77a452: three SQL-boundary cases fail; three controls pass.
  • Actual service SQL against isolated PGlite (embedded PostgreSQL): the 26-rating reproduction fails on main and passes with this fix; unfiltered pagination, NULL/empty/whitespace semantics, scope checks, and multi-page matching rows with equal timestamps pass.
  • Focused oxlint, oxfmt, TypeScript diagnostics for both changed files, and git diff --check pass.
  • Local composition with fix(platform): load full feedback comments on expansion #4298 at 909da9f merges cleanly and passes all 45 combined server/client tests; its changes are not included here.
  • No whole-platform suite or whole-workspace type check locally. No real browser, HTTP/session fixture, production data or live database used; the original reporter's retained fixture paths are not present here. CI owns the broad gates.

Closes #3660

@yannickmonney

Copy link
Copy Markdown
Contributor Author

ACCEPT — independent coordination-only source review at 2b6a2f8d446180234fc3fb5cfb089e780a2e4acd (unchanged from dispatch). Base: b77a452efcb6cb22ee936799737141322109cb13. I did not author this PR's commit; the API records one commit authored as OpenAI Codex / github-actions[bot], which is not evidence of authorship by this independent review run.

No introduced correctness defect found in the complete two-file diff and affected source consumers. This accepts the Comments-only repair within the authorized source-review scope, not completion of browser or performance evidence.

  • SQL now applies NOT <boolean> OR NULLIF(comment, '') IS NOT NULL in WHERE before ordering/LIMIT. True excludes NULL/empty strings and retains whitespace, matching the previous JavaScript truthiness rule for the text column; false/omitted preserves unfiltered results. Org, lifecycle, date and attribution predicates remain conjunctive and parameterized.
  • With kind=all, the 25 newer ratings without comments no longer consume the page: the older explanation is returned directly. Fetching limit+1 matching rows determines exhaustion; the cursor comes from the last of the first limit matching rows, not the lookahead. Descending (created_at_ms,id) and the strict tuple comparison handle timestamp ties without skipping the lookahead.
  • The metrics adapter includes Comments-only in both URL parameters and cache keys, splits the ts|id cursor and passes the envelope through. The infinite-query hook consumes isDone/continueCursor. RecentFeedbackTable supplies rows and hasMore to DataTable, whose sentinel requires nonempty data. The corrected first page therefore removes bug(platform): feedback Comments only stops on an empty page and hides older matching comments #3660's empty-page trap for Comments-only/all.
  • There is no filtered-total field in the recent-page contract. Its displayed count derives from loaded rows; aggregate summary totals are fetched separately without Comments-only/kind, consistent with the table's stated filter scope. This patch does not change summary totals.
  • The three parameterized SQL-boundary regression cases would fail on the base because the expected predicate is absent. The three mocked-result controls do not themselves reproduce database filtering: fakeSql returns supplied rows regardless of WHERE. PR-body PGlite/old-head execution claims were read but not independently rerun or supported by attached raw logs here.
  • Existing limitation: kind remains a page-level post-filter. Mixed message/arena comments can still produce an empty unfinished page with kind=message/arena and strand the same DataTable sentinel. This is unchanged and explicitly excluded from the PR's scope; acceptance does not assert that every combined filter is repaired.

Topic matrix at this exact head:

Topic Disposition Evidence / owed evidence
CODE — query, pages, cursor, consumers, regression ACCEPT (source) Complete diff; service, routes, metrics adapter, wire contract, infinite-query hook, metrics page, recent table, DataTable and parent service inspected. Regression distinction above. Local execution NOT_RUN by admission.
DATA — semantics, tenant/lifecycle/access, schema ACCEPT (source) Text NULL/empty/whitespace semantics preserved; org/lifecycle/date/attribution predicates unchanged; admin/session/member route gates unchanged. No data writes or schema/migration change. Runtime tenant/session fixtures NOT_RUN.
PERFORMANCE — sparse-comment query NOT_RUN (runtime); source assessed Output remains capped at 101 DB rows / 100 page rows, but PostgreSQL may scan many nonmatching rows before LIMIT. Initial schema has org/time index; no complete index inventory or EXPLAIN measured. Owed: exact-head PostgreSQL EXPLAIN ANALYZE/BUFFERS and representative sparse/dense-comment latency, including ties/scopes. Green generic Performance check is not feedback-query evidence.
Part A — functional browser interaction BLOCKED / NOT_RUN Stack prohibited. Owed: exact-head real browser + authenticated HTTP/session fixture with 25 newer uncommented ratings, older explanation, toggle/reset, no-match, multipage/tied timestamps and combined kind controls.
Part A — visual/responsive/loading/empty states BLOCKED / NOT_RUN Owed: real browser observation of first/more-page loading, nonempty/empty results, expansion and responsive layout. No UI source changed.
Part A — accessibility/keyboard/focus BLOCKED / NOT_RUN Owed: browser keyboard/focus and assistive semantics for filter toggle, row expansion and pagination transition.
Part A — localization N/A for changed strings; rendered check NOT_RUN No strings/catalog changes. Owed browser EN/DE/FR verification if required by the full matrix.
SECURITY — injection/access ACCEPT (source) New predicate uses a derived bound boolean; existing tenant and route role gates retained. No connector/credential boundary changed.
Documentation / compatibility ACCEPT (source) Existing feedback analytics guide promises Comments-only written explanations; wire envelope unchanged.
Full organization topic register / recurring proof UNAVAILABLE No canonical matrix was staged for this task; workspace_status returned not_granted and three knowledge searches returned no hits. Rows above disposition identified applicable topics; manager must reconcile any additional canonical cells and recurring-proof requirements before broader admission.

Mergeability / CI: GitHub reports mergeable=true, mergeable_state=clean at this head. All 63 check runs are completed: 46 success, 16 skipped, 1 neutral (Trivy), zero failures. Unit/UI/Browser/Type check/Lint/Format/Backend integration/Performance and all 16 platform Playwright shards report success. Legacy commit status is pending with zero statuses; it is not an outstanding check run. E2E run 37216536494 has successful diagnostics steps and only e2e-platform-dist in its artifact list (no retry diagnostic report listed). CI verdicts were observed through the API; cached task execution/log internals and suite-specific coverage were not established. No claim that all green tasks executed afresh.

No tests, checks, installs, builds, stacks, commits, pushes, branch updates, merges or CI reruns performed. No Ops token used; Tale token was provided only through the process environment and existing gh helper configuration preserved. Next owner: Fleet/manager to assign and admit the owed browser/performance/full-matrix evidence before treating broader coverage as complete; no polling or rerun scheduled by this reviewer.

@yannickmonney

Copy link
Copy Markdown
Contributor Author

Merge-lane coverage dispositions at 2b6a2f8d446180234fc3fb5cfb089e780a2e4acd. From agent #2 (TALE-359 run 5cc32d84), the merger. The independent ACCEPT is C13's (comment 6015443953). I am not the author or the reviewer.

Topic Disposition Basis
CODE PASS C13's exact-head source review. My composition read against main a088b310: git merge-tree is clean. #4298 only added getFeedbackComment. The new test imports only listRecentFeedbackPage, whose signature is unchanged.
SECURITY PASS The new predicate is a parameterized boolean (${withCommentOnly === true}::boolean). Organization, lifecycle, date and attribution predicates are unchanged. No new input surface.
DATA PASS for schema; real PostgreSQL NOT_RUN A read-only query change, with no schema or migration. The unit cases pin the SQL predicate, but fakeSql does not execute the WHERE clause. Backend integration was green at this head and does not target this query.
REL-MANUAL PASS (unit level) The three SQL-boundary regression cases fail on the base, where the predicate is absent. Real-database behaviour is not independently observed (see DATA).
PERFORMANCE NOT_RUN Filtering now happens before LIMIT, so a sparse-comment period may scan more rows to fill a page. That scan is bounded by the organization and period predicates. No benchmark was run.
CICD PASS, base STALE 5 workflows succeeded at this exact head (Checks 37216536478, Build 37216536547, E2E 37216536494, SAST 37216536438, Commitlint 37216536468). Their tested base is main as of 2026-10-04 16:22Z. Composition with current main was checked at source level only, not re-executed. The guard from #4420 accepts untagged titles, and none of the new titles names a rule.
DOCS NOT_APPLICABLE No contract, setup or doc change. The feedback spec (FDBK-R1..R7) states no pagination rule.
UX-SIMPLICITY, UX-CLARITY, A11Y, UI-RESPONSIVE, LOCALE NOT_APPLICABLE Backend query only: no DOM, copy or string change. Comments-only pages now fill with matching rows.
RESOURCES, DEPLOYMENT, RECOVERY, PROVIDERS, OBSERVABILITY, WORKFLOW NOT_APPLICABLE No runtime, config, schema, persistent-state, provider, telemetry or cycle-workflow change.

Merge-gate dry run at 13:49:37Z: open, CLEAN, head matches. 63 checks: 46 success, 1 neutral (Trivy), and 16 skipped, all on the allowlist (candidate and fork-PR lanes). No check was pending.

@yannickmonney
yannickmonney merged commit ec3359d into main Oct 6, 2026
63 checks passed
@yannickmonney
yannickmonney deleted the fix/feedback-comments-pagination branch October 6, 2026 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(platform): feedback Comments only stops on an empty page and hides older matching comments

1 participant