Skip to content

fix(web): bound workspace member pagination - #169

Open
SebTardif wants to merge 2 commits into
openclaw:mainfrom
SebTardif:fix/web-member-pagination-cursor
Open

fix(web): bound workspace member pagination#169
SebTardif wants to merge 2 commits into
openclaw:mainfrom
SebTardif:fix/web-member-pagination-cursor

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

What Problem This Solves

The chat workspace member directory follows has_more with 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:

$ cd apps/web && node --test src/lib/member-cursor.test.ts
# tests 4
# pass 4
# fail 0

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.

$ cd apps/web && node --test src/lib/member-cursor.test.ts
# tests 4
# pass 4
# fail 0

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.

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>
@clawsweeper

clawsweeper Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 18, 2026
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed August 18, 2026, 6:01 PM ET / 22:01 UTC.

ClawSweeper review

What this changes

The PR adds a cursor-cycle guard to Chat’s workspace-member pagination, tests the helper, and refreshes the checked-in web assets.

Merge readiness

Blocked until real behavior proof from a real setup is added - 5 items remain

Keep open: the prior P2 blocker remains because the channel and thread embed views still use the same unbounded member-pagination loop, and the PR has only isolated test proof.

Priority: P2
Reviewed head: 19b6e018e51c95e7ba85dd6124b8de6c02a3e9e3

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The focused helper and tests are useful, but the remaining embed defect and mock-only proof prevent merge readiness.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The PR body supplies isolated Node test output, not after-fix behavior through a real Chat or embedded client. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The PR body supplies isolated Node test output, not after-fix behavior through a real Chat or embedded client. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 5 items Chat guard added: Chat now records cursors and delegates page progression to the shared validator.
Embed channel remains unguarded: The embedded channel view still directly advances a do/while loop from has_more and next_cursor without detecting a repeated cursor.
Embed thread remains unguarded: The embedded thread view has the same direct cursor loop and remains susceptible to a repeated page cursor.
Findings 1 actionable finding [P2] Guard both embedded member loaders
Security None None.

Live Verification

Command: cd apps/web && node --test src/lib/member-cursor.test.ts

Result: FAIL (partial) — step 2 expect_output # pass 4: expected terminal output was not visible within 30 seconds: "# pass 4"

cd apps/web && node --test src/lib/member-cursor.test.ts
runner@runnervmzvulz:/tmp/clawsweeper-live-proof-169-7ciI7q/target$ cd apps/web && node --test src/lib/member-cursor.test.ts
✔ nextMemberCursor returns undefined when the directory is exhausted (0.92206ms)
✔ nextMemberCursor returns and records a fresh next cursor (0.746921ms)
✔ nextMemberCursor throws when has_more is set without a cursor (0.492126ms)
✔ nextMemberCursor throws when the same pagination cursor repeats (0.205503ms)
ℹ tests 4
ℹ suites 0
ℹ pass 4
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 171.579674
runner@runnervmzvulz:/tmp/clawsweeper-live-proof-169-7ciI7q/target/apps/web$ cd apps/web && node --test src/lib/member-cursor.test.ts
-bash: cd: apps/web: No such file or directory
runner@runnervmzvulz:/tmp/clawsweeper-live-proof-169-7ciI7q/target/apps/web$


































Assertions:

  • FAIL expect_output: # pass 4

How this fits together

ClickClack fetches workspace members in pages for Chat and embedded channel/thread views. Each client follows API pagination metadata to build member lists used for mentions and message UI.

flowchart LR
  A[Workspace selection] --> B[Chat and embed views]
  B --> C[Member directory API]
  C --> D[Cursor validation]
  D --> E[Member collection]
  E --> F[Mention and message UI]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR body supplies isolated Node test output, not after-fix behavior through a real Chat or embedded client. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Guard both embedded member loaders (P2) - This repeats the prior blocker: EmbedChannelView and EmbedThreadView still advance directly from has_more and next_cursor, so the repeated-cursor response fixed in ChatApp remains an infinite fetch loop for embeds. Route both through the shared validator before merging.
  • Resolve merge risk (P1) - The embedded channel and thread routes remain capable of looping indefinitely when the member API repeats a cursor.
  • Resolve merge risk (P1) - The supplied test output exercises the helper only; no real client-boundary behavior has been shown after the fix.
  • Complete next step (P2) - The remaining code defect is a narrow mechanical repair, while real behavior proof remains the contributor’s separate merge requirement.

Findings

  • [P2] Guard both embedded member loaders — apps/web/src/components/embed/EmbedChannelView.svelte:91-94
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Full-directory loaders 1 guarded, 2 unguarded The PR fixes ChatApp but leaves both embedded consumers on the repeated-cursor loop.
Source and test delta production +21 net, tests +42 The focused helper is substantially covered, but its remaining consumers are not yet wired to it.

Merge-risk options

Maintainer options:

  1. Guard both embedded member loaders (recommended)
    Apply the shared cursor validator to the channel and thread embed loops before merging so all full-directory consumers reject malformed pagination.

Technical review

Best possible solution:

Use the shared validator in Chat and both embedded member loaders, then provide a redacted served-client or production-boundary trace showing a repeated cursor terminates safely.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: a repeated non-empty cursor causes both embed do/while loops to request the same page indefinitely because neither records prior cursors.

Is this the best way to solve the issue?

No: centralizing the validator is appropriate, but this patch must apply it to every full-directory caller of the same API rather than only ChatApp.

Full review comments:

  • [P2] Guard both embedded member loaders — apps/web/src/components/embed/EmbedChannelView.svelte:91-94
    This repeats the prior blocker: EmbedChannelView and EmbedThreadView still advance directly from has_more and next_cursor, so the repeated-cursor response fixed in ChatApp remains an infinite fetch loop for embeds. Route both through the shared validator before merging.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found, but no applicable review policy affected this item.

Codex review notes: model internal, reasoning high; reviewed against acf70f8206c5.

Labels

Label changes:

  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.

Label justifications:

  • P2: A malformed member page can keep an embedded client loading indefinitely, but the impact is limited to workspace-member fetches.
  • merge-risk: 🚨 availability: Merging the partial fix leaves two shipped client paths vulnerable to an unbounded request loop.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body supplies isolated Node test output, not after-fix behavior through a real Chat or embedded client. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Acceptance criteria:

  • [P1] pnpm --filter @clickclack/web test.
  • [P1] pnpm --filter @clickclack/web typecheck.
  • [P1] pnpm build:web && pnpm embed:webassets.

What I checked:

Likely related people:

  • Shakker: Commit history identifies this contributor with the paginated workspace-members endpoint and related member-count work. (role: introduced workspace-member pagination; confidence: high; commits: db8bb6dfcdc4, d28b990e8b5f; files: apps/web/src/lib/workspace-members.ts, apps/api/internal/store/workspace_member_pages.go)
  • Peter Steinberger: History includes embedded web-asset consistency work, relevant because this PR updates checked-in web bundles. (role: recent embedded-web asset contributor; confidence: medium; commits: 190af7012936, f1002dd70f96; files: apps/api/internal/webassets/dist)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Apply the shared guard to the channel and thread embed loaders.
  • Add redacted served-client or production-boundary proof of repeated-cursor handling.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-08-18T21:52:08.400Z sha 71b3d10 :: needs real behavior proof before merge. :: [P2] Guard both embedded member loaders

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant