fix: add padId to padUpdate/padCreate hook context#7452
fix: add padId to padUpdate/padCreate hook context#7452JohnMcLear merged 1 commit intoether:developfrom
Conversation
The pad object's toJSON() intentionally strips the id property (since it's part of the database key), which caused confusion when plugins serialized the hook context. Adding padId as a top-level property on the hook context makes it directly accessible without relying on the pad object's internal properties. Fixes ether#5814 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/review |
Code Review by QodoNew Review StartedThis review has been superseded by a new analysisⓘ The new review experience is currently in Beta. Learn more |
Review Summary by QodoAdd padId to padUpdate/padCreate hook context
WalkthroughsDescription• Adds padId property to padUpdate and padCreate hook context • Ensures pad ID is accessible after hook context serialization • Resolves issue where plugins couldn't access pad ID from serialized context Diagramflowchart LR
A["Hook Context Object"] -->|"Add padId property"| B["padId: this.id"]
A -->|"Existing properties"| C["pad, authorId"]
B -->|"Survives serialization"| D["Plugin Access"]
File Changes1. src/node/db/Pad.ts
|
Code Review by Qodo
1. padId undocumented in hooks
|
| hooks.aCallAll(hook, { | ||
| pad: this, | ||
| padId: this.id, | ||
| authorId, |
There was a problem hiding this comment.
1. padid undocumented in hooks 📘 Rule violation ⚙ Maintainability
The PR adds a new top-level padId property to the padCreate/padUpdate hook context, but the server-side hooks documentation does not mention it. This violates the requirement to update doc/ alongside public hook payload changes, leaving plugin authors unaware of the new field.
Agent Prompt
## Issue description
The `padCreate` and `padUpdate` hook context now includes a new top-level `padId` field, but the hook documentation does not list it.
## Issue Context
Plugins often serialize the hook context, and `pad.toJSON()` strips `id`, so `padId` is the reliable serialized identifier. Docs should reflect the new payload shape.
## Fix Focus Areas
- doc/api/hooks_server-side.md[267-328]
- doc/api/hooks_server-side.adoc[270-331]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| hooks.aCallAll(hook, { | ||
| pad: this, | ||
| padId: this.id, | ||
| authorId, |
There was a problem hiding this comment.
2. No regression test added 📘 Rule violation ☼ Reliability
The PR changes the public hook payload by adding padId to the padCreate/padUpdate context, but the provided diff includes no accompanying automated regression test. Without a test, this bug can easily reappear if the field is removed or refactored away.
Agent Prompt
## Issue description
There is no automated regression test asserting that the `padCreate` and `padUpdate` hook context includes `padId` (and that it survives serialization).
## Issue Context
The change is meant to ensure plugins can reliably determine the pad identifier even if they serialize the hook context. A regression test should fail if `padId` is removed/reverted.
## Fix Focus Areas
- src/tests/backend/specs/Pad.ts[1-142]
- src/node/db/Pad.ts[110-142]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Summary
One-line fix: adds
padId: this.idto thepadUpdateandpadCreatehook context object.Root Cause
The
padobject'stoJSON()intentionally strips theidproperty (line 151 in Pad.ts) because it's redundant with the database key (pad:{id}). This meant that when plugins serialized the hook context (e.g., logging), the pad ID was missing.While
context.pad.idworks on the live object, addingpadIdas a top-level property is clearer and survives serialization.Test plan
Fixes #5814
🤖 Generated with Claude Code