close
Skip to content

feat(chat): message-level pinning with pinned messages panel - #1676

Merged
iamwhatever merged 1 commit into
kirodotdev:mainfrom
RohanK6:feat/chat-message-pins
Aug 10, 2026
Merged

feat(chat): message-level pinning with pinned messages panel#1676
iamwhatever merged 1 commit into
kirodotdev:mainfrom
RohanK6:feat/chat-message-pins

Conversation

@RohanK6

@RohanK6 RohanK6 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Closes #1662

What

Users can now pin individual messages (user or agent) in a chat session and review them in a dedicated Pinned messages panel.

  • Pin/unpin toggle on the message hover action bar (next to Copy link), on both user and assistant messages. Optimistic update with rollback on API error.
  • Pinned messages panel opened from the chat header: role badge, line-clamped preview, relative time, and per-entry hover actions (Copy, Copy link, Unpin). Clicking an entry jumps to the message using the same scroll + highlight mechanism as message deep links.
  • Persistence: pins survive reloads and gateway restarts.

How

  • Backend follows the chat-folders precedent: chat_pins.json JSON store on DashboardState with atomic writes and startup load; new /api/chat/pins routes (GET list by slot, POST create — idempotent per (slot_key, message_ts), DELETE by id or by slot+ts).
  • Message identity reuses what Copy link already uses: session slot key + message ts.
  • Frontend: useChatPins hook + PinnedMessagesPanel component; wiring in ChatPage keeps new props minimal on the memoized message components to avoid re-render pressure in the virtualized list.
  • i18n: new keys added to all 10 locale catalogs, context sidecar entries, pseudolocale regenerated.

Testing

  • Backend: 15 new handler tests (create/idempotency/list/delete/400/404/persistence roundtrip/corrupt-file tolerance); test_dashboard_chat.py suite passes (504 tests).
  • Frontend: 13 new vitest tests (hook optimistic+rollback, panel render/unpin/jump/empty state); existing UserMessage (27) and AssistantMessage (52) suites pass; i18n catalog parity passes.
  • Full local gate sweep: complete vitest run (9210 passed — the single failure, ContextBreakdownPanel.test.tsx, also fails on clean origin/main with these changes stashed, i.e. pre-existing), npm run build clean, eslint src/ 0 errors, ./scripts/docs-lint.sh clean.
  • Live-tested end-to-end on an isolated gateway serving the real built dist: full API cycle (create → idempotent re-create → list → gateway restart → pins survive → both delete variants → 400/404 paths) and UI flow (pin from hover bar, panel with 1 and 3 pins, jump-to, unpin, empty state), with the on-disk store verified after every UI action.

Screenshot Evidence

Live-tested on a real gateway serving the built dist:

Feature Screenshot
Pin button on hover action bar (user & assistant messages) Hover action bar with Pin button
Pinned state — hover bar shows unpin affordance Pinned hover state
Pinned messages panel (single pin: role badge, preview, time) Panel with one pin
Panel with multiple pins from both roles Panel with three pins
Empty state after unpinning all Empty state

@RohanK6
RohanK6 requested a review from a team August 5, 2026 20:45
@RohanK6
RohanK6 requested a review from a team as a code owner August 5, 2026 20:45
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention labels Aug 5, 2026
@RohanK6

RohanK6 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Screenshots (live-tested on a real gateway serving the built dist)

Pin button on the message hover action bar (both user and assistant messages):
Hover action bar with Pin button

Pinned state — hover bar flips to the unpin affordance:
Pinned hover state

Pinned messages panel with a single pin (role badge, preview, relative time):
Panel with one pin

Panel with multiple pins from both roles:
Panel with three pins

Empty state after unpinning:
Empty state

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed 1a42a5f7d44e0df0a667a8cae4379c43992b0143 via the fork AI-review pipeline; updated in place on each push.

BLOCKING -- website/src/hooks/useChatPins.ts:41 -- Pinning crashes on supported non-secure dashboard origins
id: \temp-${crypto.randomUUID()}`,Plain-HTTP non-loopback dashboard -> Pin click -> unavailablerandomUUIDthrows -> no pin request. Fix: Use the existingsecureRandomId()` helper.

BLOCKING -- src/kiro_crew/dashboard/state.py:3755 -- Transient read failures cause destructive pin-store replacement
self._chat_pins = []
Valid file + transient read error -> empty state -> next mutation overwrites the file -> saved pins are lost.
Fix: Re-raise I/O errors; tolerate only parse or shape corruption.

FINDING -- website/src/hooks/useChatPins.ts:53 -- With no cached query data, "if (ctx?.prev)" skips rollback and leaves a failed optimistic pin visible -> Fix: remove the query when ctx.prev is undefined; otherwise restore it.

[GPT-REVIEWED] 1a42a5f
[BLOCK-MERGE] 1a42a5f

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

Advisory design-level review of 1a42a5f7d44e0df0a667a8cae4379c43992b0143 via the fork AI-review pipeline — updated in place on each push; does not block merge.

Design-Verdict: CONCERNS

Sound feature on the chat-folders precedent, but it ships "legacy" compat and an uncalled public endpoint for pin data that has never existed.

Watch

  • Speculative compat surface at birth. DELETE /api/chat/pins/by-query (with message_ts fallback "for legacy compatibility") has no frontend caller — pins.ts only deletes by id — and load_chat_pins tolerates "pre-mid era" pins that cannot exist, since this feature has never shipped. That's a permanent public route carrying its own app-isolation/ownership checks and tests, maintained for zero users; public API is easy to add and hard to retract. Drop the route and the ts-fallback until a caller exists.
  • Unbounded jump loop. The pinned-jump effect loops loadOlderMessages() (100/page) with "No arbitrary cap on pinned-jump page loads" — clicking a pin near the top of a very long transcript issues dozens of serial fetches and materializes the entire history in the list before scrolling. Fine today; a server-side "index of mid" lookup is the eventual fix if long sessions make this noticeable.

Suggestions

  • Derive preview server-side from the message identified by (slot_key, mid) instead of trusting client text — that removes the 413/truncation/re-redaction input ceremony (the bulk of chat_pins.py) and previews can't go stale on edit-resend.
  • The reformat-only hunks in handlers/sessions.py and server.py are unrelated churn in security-adjacent files; split them into their own commit/PR.

[DESIGN-REVIEWED] 1a42a5f

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 1a42a5f7d44e0df0a667a8cae4379c43992b0143 via the fork AI-review pipeline; updated in place on each push.

Review details

I've now read the complete diff and verified the security-sensitive paths: redaction helper signatures (redact_credentials/redact_exfiltration_urls both return tuples, correctly indexed with [0]), atomic_write (mode/fsync args match), read_bounded_json (codes match), slot._app attribute existence, and sel().log_api_access signature.

Falsification of Phase A candidates:

  • Blocking-call-on-loop: save_chat_pins (sync atomic_write + fsync) and load_chat_pins (sync read/JSON) — every call site wraps them in asyncio.to_thread. Killed.
  • Credential redaction boundary: create redacts before truncation; list, idempotent-existing, and duplicate paths all route through _redacted_pin; the 201 path returns a pin whose preview was already redacted. Killed.
  • Deny-by-default authorization: _authorize_app_slot returns None only for dashboard callers (the owner, by design) or when the app positively owns the slot; every other case denies with 404 + SEL audit. In-lock re-check closes the TOCTOU. Record-level origin_app checks prevent cross-app IDOR. Killed.
  • Crash on malformed stored data: load_chat_pins validates types (id/slot_key/preview/pinned_at strings, mid-or-message_ts present) and drops bad records; direct index accesses (p["slot_key"], pin["id"]) are on fields the loader guarantees. Killed.
  • Persist-failure data loss: create/delete/remove all roll back in-memory state on save_chat_pins exception. Killed.
  • Frontend slot-swap contamination: loadOlderMessages captures slot at dispatch, reducer guards payload.slot === activeSlot; React Query keys are per-slot. Killed.

No candidate survived Phase B.

No findings.

[OPUS-REVIEWED] 1a42a5f

@RohanK6
RohanK6 force-pushed the feat/chat-message-pins branch from c3bcf7b to 5f9f2d6 Compare August 5, 2026 22:21
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — 🟡 CONCERNS

Advisory UX-level review of 1a42a5f7d44e0df0a667a8cae4379c43992b0143 via the fork AI-review pipeline — updated in place on each push; does not block merge.

UX-Verdict: CONCERNS

Solid pin flow overall, but Korean ships raw English, a failed pin-list renders as "no pins yet," and the hard 50-pin limit says "Try again."

Watch

  • ko.json is untranslated: "pin_message": "Pin message" and the whole pins block are English while all 11 other locales are translated — every Korean user sees a mixed-language toolbar and panel, every session. Translate before merge.
  • List failure masquerades as empty: useChatPins has no query-error branch, so a failed GET /api/chat/pins yields pins = [] and the panel shows "No pinned messages yet" — users conclude their pins were deleted. Add an error state distinct from empty.
  • pin_limit_reached (409, 50-pin cap) maps to "Could not pin the message. Try again." — retrying can never succeed; the copy promises a repair that doesn't exist. Branch on the code field and say the limit was reached.
  • Jump to an unloaded pin gives no feedback at the click locus: handleJumpToPinnedMessage silently pages loadOlderMessages (possibly many round-trips, unbounded per the diff comment) while the panel shows nothing; the eventual "unavailable" notice lands in a banner at the top of the chat, not in the panel. Show a busy/seeking state on the clicked pin entry.
  • Fixed 300px panel width has no narrow-screen handling — the toggle renders on mobile, and animate={{ width: 300 }} leaves ~60px of chat on a 360px phone; the adjacent side-panel comment in this very hunk documents the full-width-below-breakpoint pattern this panel skips.

Suggestions

  • The header toggle keeps aria-label="Open pinned messages" and color-only state while open — add aria-pressed={pinsPanelOpen}; and on Escape-close, return focus to the toggle (the focused panel unmounts, dropping focus to <body>).

[UX-REVIEWED] 1a42a5f

@RohanK6
RohanK6 force-pushed the feat/chat-message-pins branch 6 times, most recently from a85547c to 64a07a2 Compare August 6, 2026 04:20
@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 6, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator

There are AI review comment and conflicts

@RohanK6
RohanK6 force-pushed the feat/chat-message-pins branch from 64a07a2 to 465d845 Compare August 6, 2026 11:10
@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 6, 2026
@RohanK6
RohanK6 force-pushed the feat/chat-message-pins branch 5 times, most recently from be861b3 to ea2b955 Compare August 6, 2026 15:19
@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 6, 2026
@RohanK6
RohanK6 force-pushed the feat/chat-message-pins branch from ea2b955 to 2a05212 Compare August 6, 2026 16:16
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 6, 2026
@RohanK6
RohanK6 force-pushed the feat/chat-message-pins branch from 2a05212 to af53d52 Compare August 6, 2026 17:19
@github-actions github-actions Bot added the readiness: action required A blocking check or review needs attention label Aug 7, 2026
@RohanK6
RohanK6 marked this pull request as draft August 7, 2026 14:06
@RohanK6
RohanK6 marked this pull request as ready for review August 7, 2026 14:06
@RohanK6
RohanK6 force-pushed the feat/chat-message-pins branch from 3e38f3d to 77b2522 Compare August 7, 2026 14:20
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 7, 2026
@RohanK6
RohanK6 force-pushed the feat/chat-message-pins branch from 77b2522 to 157dcf8 Compare August 7, 2026 16:21
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 7, 2026
@RohanK6
RohanK6 force-pushed the feat/chat-message-pins branch from 157dcf8 to 1549640 Compare August 7, 2026 17:06
@github-actions github-actions Bot added readiness: checking Automated validation is still running merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 7, 2026
@RohanK6
RohanK6 force-pushed the feat/chat-message-pins branch from 1549640 to bf07e1e Compare August 7, 2026 23:53
@github-actions github-actions Bot added merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Aug 8, 2026
@RohanK6
RohanK6 force-pushed the feat/chat-message-pins branch from bf07e1e to c4650bc Compare August 8, 2026 02:24
@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 8, 2026
@RohanK6

RohanK6 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

This PR is otherwise ready. New translation changes merged into main and introduced merge conflicts, so I’m resolving those and refreshing the branch now. No functional changes to the message-pinning feature are inbound.

@RohanK6

RohanK6 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Confirmed this was the same upstream catalog-parity failure independently seen on PR #2299: the Korean pages.sessionStorage.* catalog had 29 missing and 36 stray keys relative to English. I included a translation-only Korean catalog sync while refreshing this branch; the complete local suite is now green. No functional message-pinning changes were made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No way to pin important messages within a chat session

2 participants