fix(web): bound workspace member pagination - #169
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs real behavior proof before merge. Reviewed August 29, 2026, 9:46 AM ET / 13:46 UTC. ClawSweeper reviewWhat this changesThe branch adds repeated-cursor and incomplete-page validation to the web chat member-directory loader, tests the helper, and regenerates the embedded web assets. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 5 items remain Keep open: the normal chat loader is guarded, but the same member-pagination loop remains unbounded in both supported embedded chat views; the submitted helper-only test output also does not prove production behavior. Priority: P2 Review scores
Verification
How this fits togetherThe web client loads workspace members page by page to populate chat mentions and member-aware controls. Each API page returns a cursor that either drives another request or ends the load. flowchart LR
A[Workspace selected] --> B[Member directory loader]
B --> C[Member page API]
C --> D[Cursor validation]
D -->|Fresh cursor| C
D -->|End or invalid cursor| E[Stop loading]
E --> F[Chat mention state]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Use the shared cursor validator in every automatic workspace-member pagination loop, regenerate the embedded assets, and attach a redacted browser or production-loader trace showing the repeated request sequence stops. Do we have a high-confidence way to reproduce the issue? Yes: return has_more=true with the same next_cursor twice from the workspace-members endpoint; both embedded loops at the cited lines will continue requesting that cursor on the reviewed head. Is this the best way to solve the issue? No: the shared validator is a maintainable solution, but applying it only to ChatApp leaves two equivalent supported loaders unprotected. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found, but no applicable review policy affected this item. Codex review notes: model internal, reasoning high; reviewed against 486fd23545af. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
Chat member directory followed has_more with no cursor-cycle cap. Settings overview already rejects a repeated cursor. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
19b6e01 to
b29f428
Compare
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
What Problem This Solves
The chat workspace member directory follows
has_morewith no cursor-cycle cap. Settings overview already rejects a repeated member-directory cursor. Chat did not, so a stuck page can hang the web client.Why This Change Was Made
Align chat pagination with the existing settings overview guard.
User Impact
A repeated member cursor stops the directory load instead of looping fetch forever.
Evidence
terminal output from the shared helper used by ChatApp:
Real behavior proof
Behavior addressed: Chat member pagination rejects an incomplete page and a repeated cursor.
Real environment tested: macOS, Node, clone at /tmp/pr-clickclack on the patched branch.
Exact steps or command run after this patch: cd apps/web && node --test src/lib/member-cursor.test.ts
Evidence after fix: terminal output copied below.
Observed result after fix: A repeated cursor throws
Member directory repeated a pagination cursor.What was not tested: Live ClickClack workspace UI against a wedged API.