Skip to content

feat: add private text feedback plugin - #242

Merged
kalvinnchau merged 11 commits into
mainfrom
am/feedback-text
Sep 25, 2026
Merged

kalvinnchau merged 11 commits into
mainfrom
am/feedback-text

Conversation

@kalvinnchau

@kalvinnchau kalvinnchau commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Scope

  • Bundled feedback plugin sends bounded private kind 42000 text and optional category to the deployment operator inbox, without a channel tag or ordinary timeline/read exposure.
  • Failed or unknown delivery can be explicitly discarded locally after confirmation. Discard does not undo a possibly delivered event; retry uses the original signed ID.

Evidence

  • Rebased on main 6fa0e9a. At 8d1fd8c: full Vitest package suite 324 files / 3,542 tests passed with two workers; typecheck and pre-push checks passed.
  • Synthetic broker/session tests exercise signed private publication, persistence, retry and local discard. Browser and native UI journeys and controlled live feedback submission at this head remain deferred. Relay acceptance does not prove operator review.

Keep draft until live validation and CI complete. No browser cases added or removed.

@kalvinnchau
kalvinnchau requested review from a team, comp615 and wesbillman as code owners September 24, 2026 21:41

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested. Four actionable findings are recorded inline: two P1s (no-outbox render loop and cross-deployment draft retention) and two P2s (native registration and stale Done completion).

Reviewed head c5316922cdeddc07e20e4659e477a5a3b529ca96 against target b86e8d75560277f6b61acdb8c807b17d42c88397; PR diff merge-base d6b01e6b5d159bfa16ace71978ea8230544b8dc4. Validation was static source/contract review, hosted CI inspection, and an isolated React hook-mechanism probe. No PR application code or suites were executed locally; live submission and native UI behavior remain unvalidated.

Merge criteria: fix the four inline defects with boundary/lifecycle coverage. Separately, resolve the current merge conflict and red browser journeys. Hosted run 36063108794 tested merge c57408e: JavaScript, Rust/tool integration, and browser measurements passed; two existing viewport journeys failed in both engines after bounded wheel traversal. The prepended Feedback row changes traversal geometry, but these failures do not establish product clipping or unreachable controls. Repair traversal without weakening visibility assertions and validate both affected files in Chromium/WebKit.

Comment thread src/bundled/feedback/FeedbackDialog.tsx Outdated
Comment thread src/bundled/feedback/FeedbackDialog.tsx Outdated
Comment thread src/bundled/index.ts
Comment thread src/bundled/feedback/FeedbackDialog.tsx Outdated
@kalvinnchau
kalvinnchau marked this pull request as draft September 24, 2026 22:06
@kalvinnchau
kalvinnchau added this pull request to stack #246 September 24, 2026 22:13
@kalvinnchau
kalvinnchau force-pushed the am/feedback-text branch 4 times, most recently from b1b05fb to af82c6c Compare September 25, 2026 05:17
@kalvinnchau
kalvinnchau marked this pull request as ready for review September 25, 2026 05:19

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Source review clear; validation remains incomplete. The four findings from review5310767267 are repaired at af82c6c9949a7a9c6effc1a5b64229789312db92: stable no-outbox rendering, session-scoped drafts, native catalog registration, and stale dismissal completion. I found no additional actionable code blockers in the text-only feedback slice against base df7b7e7f45739f3e06e12d81623385701acdc51d. This is not an approval.

CI run36097958821 tested the matching integration merge. At the 05:32Z snapshot, 658 browser journeys and Rust checks passed; JavaScript was 3719/3720 with the recurring AgentModelPicker failure, and its rerun was still pending. A green required gate was not established. Changed settings/integration tests retain their previous assertions.

Remaining acceptance gaps: no feedback-dialog browser journey (including keyboard focus handoff/return), desktop UI or controlled live-relay submission; Windows validation was skipped. Source/model tests do not prove deployed private-inbox routing or operator review. Resolve the required CI gate and complete the deferred feature acceptance before claiming merge readiness. No PR code was executed during this source-only review.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Source review clear; not approval or a green merge gate. Reviewed a6027d1f7f44f1125e9cbf80e48c05b60c49755c against e44b1e44f35b881b2dcaa44147d2ab607384b058 (also the merge-base). The rebased feedback production changes preserve the previously reviewed behavior and all four repaired findings. The two subsequent commits only align three test fixtures/assertions. Independent comparison checked the actual patches and new-base interactions, not just matching diff statistics.

Integration CI run 36168395905 tests this head/base pair. At the 17:52Z snapshot, JavaScript failed 1/3937 tests; WebKit 2/2 and the required gate were still pending, and Windows was skipped. The failing PluginImport assertion expects “stays enabled,” while the component renders “stays on”; both files are byte-identical at base and head, and the direct mocked render does not use this PR’s native catalog addition. This is an inherited mismatch, not an introduced finding. The red gate still needs resolution.

Source-only review; no PR code, tests, builds or CI reruns executed. Feature-dialog browser/focus handoff, packaged-desktop behavior and controlled live private-relay delivery remain unverified; relay acceptance would not prove operator review. Screenshot/diagnostics work in #245 is excluded. No accidental screenshots/generated artifacts found in this PR’s 30-file diff.

am and others added 11 commits September 25, 2026 11:23
Co-authored-by: Kalvin Chau <kalvin@block.xyz>

Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz>
Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz>

Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz>

Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz>

Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz>
Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz>

Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Remove unused closed-state render fencing and cover real outbox admission near the limit.

Co-authored-by: Kalvin Chau <kalvin@block.xyz>

Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz>

Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz>

Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz>

Signed-off-by: Kalvin Chau <kalvin@block.xyz>

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Star Lord, automated source reviewer, commenting via Wes’s GitHub account.

No new actionable source findings in this focused follow-up. Reviewed head 57c95c34b3ffa31513a7d7347d1554fd09442d73 against base/merge-base 09086638fc1c27f7e2299f042b8eeb6b19a7fece, comparing the actual code and rebased patches with the previously reviewed a6027d1f7f44f1125e9cbf80e48c05b60c49755c. This is not a fresh exhaustive audit of the full feature or intervening main changes.

  • git range-diff matches all ten previously reviewed feature commits; the only added patch updates PluginImport.test.tsx:98 to expect “stays on and may run immediately,” matching the unchanged component at PluginImport.tsx:268. Exact access-difference, visibility and no-install assertions remain intact; this is a stale-copy assertion repair, not a relaxed behavior check.
  • Of the current PR’s 31 changed paths, the other 30 are byte-identical to the previous reviewed head. The stable no-outbox snapshot, session-identity draft isolation, stale-completion fence and native catalog registration repairs remain present. I also checked the new base’s mention/message changes against feedback’s raw kind-42000 outbox path: it still bypasses kind-9 mention preparation and remains excluded from ordinary local/read/live views. Screenshot/diagnostic work in #245 is outside this review.

Validation: pinned-source analysis and byte comparisons only; no dirty source inputs, and git diff --check passed. No tests, builds, app launches, live submissions or CI reruns. At the single 18:32 UTC snapshot, CI run 36173817150 was still running JavaScript, Rust/tool integration, browser measurements and all four browser shards; Semgrep, zizmor and DCO passed, Windows was skipped. No green required gate or runtime pass is established here.

The existing acceptance gaps remain: feedback-dialog browser/focus handoff, packaged-desktop behavior and controlled private-relay delivery have not been exercised in this review. Relay acceptance would not establish operator review. This COMMENT is not approval, does not dismiss prior reviews and does not declare the PR merge-ready.

@kalvinnchau
kalvinnchau merged commit f761867 into main Sep 25, 2026
12 checks passed
@kalvinnchau
kalvinnchau deleted the am/feedback-text branch September 25, 2026 18:45
johnmatthewtennant pushed a commit that referenced this pull request Sep 25, 2026
* origin/main:
  ci: publish scheduled macOS test prereleases (#262)
  feat: add private text feedback plugin (#242)
  🤖 docs: add pre-PR checklist and review guidance to AGENTS.md (#268)
  perf(sidebar): stop rerendering every row's menu on channel switch (#265)
  Explain missing Pi provider models (#263)
  Browse Goose models and enter provider API keys (#230)
  test(agents): check model lookup Cancel by visible text (#259)

Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz>

# Conflicts:
#	src/bundled/profiles/ProfileAgentIdentity.test.tsx
zrmarley added a commit that referenced this pull request Sep 25, 2026
…-image

* origin/main: (23 commits)
  fix(agents): recover status polling and scope failure diagnostics (#283)
  Share avatar editing across community profiles and managed agents (#271)
  feat(profiles): archive, unarchive and delete agents from the profile pane (#256)
  ci: run browser journeys on three shards per engine (#280)
  ci: publish scheduled macOS test prereleases (#262)
  feat: add private text feedback plugin (#242)
  🤖 docs: add pre-PR checklist and review guidance to AGENTS.md (#268)
  perf(sidebar): stop rerendering every row's menu on channel switch (#265)
  Explain missing Pi provider models (#263)
  Browse Goose models and enter provider API keys (#230)
  test(agents): check model lookup Cancel by visible text (#259)
  Ask before mentioning people outside the channel (#257)
  Refine direct message opening (#107)
  feat(messages): report messages to community moderators (#255)
  perf(channels): stop rerendering message rows after each channel switch (#269)
  feat(profiles): open targeted agent editor from owner profile (#254)
  Let plugins declare local commands and HTTPS origins (#169)
  feat(profiles): show agent metadata and copyable nip05 (#253)
  Organize app and community settings (#173)
  Add status badge cutouts to avatars (#211)
  ...
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.

2 participants