feat: allow owners to edit skill summary#1411
feat: allow owners to edit skill summary#1411SylvanXiao wants to merge 1 commit intoopenclaw:mainfrom
Conversation
|
Someone is attempting to deploy a commit to the 0xBuns Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR adds the ability for skill owners and moderators to edit a skill's summary text directly from the skill detail page, without needing to republish the skill. Key changes:
Issues found:
Confidence Score: 3/5Not safe to merge yet — a broken DOM structure causes layout issues in all browsers, and publisher members are shown an edit button they cannot use. Two P1 issues are present: (1) the
|
| Filename | Overview |
|---|---|
| convex/skills.ts | Adds updateSummary mutation; auth check is ownerUserId or moderator, which mismatches the canManage gate used in the UI (publisher members see the edit button but get rejected). No length limit on the summary string. |
| src/components/SkillDetailPage.tsx | Wires up summary editing state and passes it down to SkillHeader; logic and state management look correct aside from the canManage/publisher-member mismatch inherited from the backend. |
| src/components/SkillHeader.tsx | Renders the edit form inside a <p> element, which is invalid HTML; the <form> and its block children cause browsers to implicitly close the paragraph, breaking layout. Otherwise the UI flow is reasonable. |
| src/styles.css | Adds focused, self-contained CSS for the edit button and form; dark-mode variants are included and styles are properly scoped. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/components/SkillHeader.tsx
Line: 214-263
Comment:
**`<form>` nested inside `<p>` is invalid HTML**
A `<form>` (flow content) cannot be a child of `<p>` (which only permits phrasing content). When browsers parse this, they implicitly close the `<p>` before the `<form>`, producing a broken DOM that causes layout issues — e.g. the form floats outside the intended container and `.section-subtitle` styles don't apply to the edit view.
Replace the outer `<p>` with a `<div>` (keeping the `section-subtitle` class) so both the static text and the form share the same valid parent:
```suggestion
<div className="section-subtitle">
{isSummaryEditing ? (
<form
onSubmit={(event) => {
event.preventDefault();
onSummarySubmit();
}}
className="summary-edit-form"
>
<textarea
className="search-input summary-textarea"
value={summary}
onChange={(event) => onSummaryChange(event.target.value)}
placeholder="Enter a brief summary..."
rows={2}
/>
<div className="summary-edit-actions">
<button
className="btn"
type="submit"
disabled={isSummarySubmitting}
>
{isSummarySubmitting ? "Saving..." : "Save"}
</button>
<button
className="btn btn-ghost"
type="button"
onClick={onSummaryEditToggle}
disabled={isSummarySubmitting}
>
Cancel
</button>
</div>
</form>
) : (
<>
{skill.summary ?? "No summary provided."}
{canManage && (
<button
className="edit-summary-btn"
type="button"
onClick={onSummaryEditToggle}
title="Edit summary"
>
✎
</button>
)}
</>
)}
</div>
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: convex/skills.ts
Line: 4799-4820
Comment:
**Publisher members can see "Edit summary" but are blocked by the backend**
The edit button is shown to anyone where `canManage` is true, which includes members of the publisher that owns the skill (via `myPublisherIds.has(skill.ownerPublisherId)`). However, `updateSummary` only permits `skill.ownerUserId === user._id` or a moderator — publisher members are rejected.
When a publisher member (who is not the `ownerUserId` and not a moderator) hits "Save", they will receive an error and see `window.alert("Failed to update summary. Please try again.")`.
Either extend the backend to accept publisher members, or restrict the edit button to `isOwner` only:
**Option A — extend backend to also allow publisher members:**
```ts
if (skill.ownerUserId !== user._id) {
let isPublisherMember = false;
if (skill.ownerPublisherId) {
const membership = await ctx.db
.query("publisherMembers")
.withIndex("by_publisher_user", (q) =>
q.eq("publisherId", skill.ownerPublisherId!).eq("userId", user._id),
)
.unique();
isPublisherMember = membership !== null;
}
if (!isPublisherMember) {
assertModerator(user);
}
}
```
**Option B — gate the button behind `isOwner` instead of `canManage`** (simpler, matches the current backend check).
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: convex/skills.ts
Line: 4800-4803
Comment:
**No maximum length validation on summary**
`v.string()` imposes no upper bound, so a client can submit an arbitrarily long string. Consider capping the summary (e.g. 500 characters) both in the validator and with a `maxLength` attribute on the textarea:
In the handler, a quick guard:
```ts
if (args.summary.length > 500) throw new Error("Summary must be 500 characters or fewer.");
```
And in the textarea:
```tsx
<textarea maxLength={500} ... />
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat: allow owners to edit skill summary" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <p className="section-subtitle"> | ||
| {isSummaryEditing ? ( | ||
| <form | ||
| onSubmit={(event) => { | ||
| event.preventDefault(); | ||
| onSummarySubmit(); | ||
| }} | ||
| className="summary-edit-form" | ||
| > | ||
| <textarea | ||
| className="search-input summary-textarea" | ||
| value={summary} | ||
| onChange={(event) => onSummaryChange(event.target.value)} | ||
| placeholder="Enter a brief summary..." | ||
| rows={2} | ||
| /> | ||
| <div className="summary-edit-actions"> | ||
| <button | ||
| className="btn" | ||
| type="submit" | ||
| disabled={isSummarySubmitting} | ||
| > | ||
| {isSummarySubmitting ? "Saving..." : "Save"} | ||
| </button> | ||
| <button | ||
| className="btn btn-ghost" | ||
| type="button" | ||
| onClick={onSummaryEditToggle} | ||
| disabled={isSummarySubmitting} | ||
| > | ||
| Cancel | ||
| </button> | ||
| </div> | ||
| </form> | ||
| ) : ( | ||
| <> | ||
| {skill.summary ?? "No summary provided."} | ||
| {canManage && ( | ||
| <button | ||
| className="edit-summary-btn" | ||
| type="button" | ||
| onClick={onSummaryEditToggle} | ||
| title="Edit summary" | ||
| > | ||
| ✎ | ||
| </button> | ||
| )} | ||
| </> | ||
| )} | ||
| </p> |
There was a problem hiding this comment.
<form> nested inside <p> is invalid HTML
A <form> (flow content) cannot be a child of <p> (which only permits phrasing content). When browsers parse this, they implicitly close the <p> before the <form>, producing a broken DOM that causes layout issues — e.g. the form floats outside the intended container and .section-subtitle styles don't apply to the edit view.
Replace the outer <p> with a <div> (keeping the section-subtitle class) so both the static text and the form share the same valid parent:
| <p className="section-subtitle"> | |
| {isSummaryEditing ? ( | |
| <form | |
| onSubmit={(event) => { | |
| event.preventDefault(); | |
| onSummarySubmit(); | |
| }} | |
| className="summary-edit-form" | |
| > | |
| <textarea | |
| className="search-input summary-textarea" | |
| value={summary} | |
| onChange={(event) => onSummaryChange(event.target.value)} | |
| placeholder="Enter a brief summary..." | |
| rows={2} | |
| /> | |
| <div className="summary-edit-actions"> | |
| <button | |
| className="btn" | |
| type="submit" | |
| disabled={isSummarySubmitting} | |
| > | |
| {isSummarySubmitting ? "Saving..." : "Save"} | |
| </button> | |
| <button | |
| className="btn btn-ghost" | |
| type="button" | |
| onClick={onSummaryEditToggle} | |
| disabled={isSummarySubmitting} | |
| > | |
| Cancel | |
| </button> | |
| </div> | |
| </form> | |
| ) : ( | |
| <> | |
| {skill.summary ?? "No summary provided."} | |
| {canManage && ( | |
| <button | |
| className="edit-summary-btn" | |
| type="button" | |
| onClick={onSummaryEditToggle} | |
| title="Edit summary" | |
| > | |
| ✎ | |
| </button> | |
| )} | |
| </> | |
| )} | |
| </p> | |
| <div className="section-subtitle"> | |
| {isSummaryEditing ? ( | |
| <form | |
| onSubmit={(event) => { | |
| event.preventDefault(); | |
| onSummarySubmit(); | |
| }} | |
| className="summary-edit-form" | |
| > | |
| <textarea | |
| className="search-input summary-textarea" | |
| value={summary} | |
| onChange={(event) => onSummaryChange(event.target.value)} | |
| placeholder="Enter a brief summary..." | |
| rows={2} | |
| /> | |
| <div className="summary-edit-actions"> | |
| <button | |
| className="btn" | |
| type="submit" | |
| disabled={isSummarySubmitting} | |
| > | |
| {isSummarySubmitting ? "Saving..." : "Save"} | |
| </button> | |
| <button | |
| className="btn btn-ghost" | |
| type="button" | |
| onClick={onSummaryEditToggle} | |
| disabled={isSummarySubmitting} | |
| > | |
| Cancel | |
| </button> | |
| </div> | |
| </form> | |
| ) : ( | |
| <> | |
| {skill.summary ?? "No summary provided."} | |
| {canManage && ( | |
| <button | |
| className="edit-summary-btn" | |
| type="button" | |
| onClick={onSummaryEditToggle} | |
| title="Edit summary" | |
| > | |
| ✎ | |
| </button> | |
| )} | |
| </> | |
| )} | |
| </div> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/SkillHeader.tsx
Line: 214-263
Comment:
**`<form>` nested inside `<p>` is invalid HTML**
A `<form>` (flow content) cannot be a child of `<p>` (which only permits phrasing content). When browsers parse this, they implicitly close the `<p>` before the `<form>`, producing a broken DOM that causes layout issues — e.g. the form floats outside the intended container and `.section-subtitle` styles don't apply to the edit view.
Replace the outer `<p>` with a `<div>` (keeping the `section-subtitle` class) so both the static text and the form share the same valid parent:
```suggestion
<div className="section-subtitle">
{isSummaryEditing ? (
<form
onSubmit={(event) => {
event.preventDefault();
onSummarySubmit();
}}
className="summary-edit-form"
>
<textarea
className="search-input summary-textarea"
value={summary}
onChange={(event) => onSummaryChange(event.target.value)}
placeholder="Enter a brief summary..."
rows={2}
/>
<div className="summary-edit-actions">
<button
className="btn"
type="submit"
disabled={isSummarySubmitting}
>
{isSummarySubmitting ? "Saving..." : "Save"}
</button>
<button
className="btn btn-ghost"
type="button"
onClick={onSummaryEditToggle}
disabled={isSummarySubmitting}
>
Cancel
</button>
</div>
</form>
) : (
<>
{skill.summary ?? "No summary provided."}
{canManage && (
<button
className="edit-summary-btn"
type="button"
onClick={onSummaryEditToggle}
title="Edit summary"
>
✎
</button>
)}
</>
)}
</div>
```
How can I resolve this? If you propose a fix, please make it concise.| export const updateSummary = mutation({ | ||
| args: { | ||
| skillId: v.id("skills"), | ||
| summary: v.string(), | ||
| }, | ||
| handler: async (ctx, args) => { | ||
| const { user } = await requireUser(ctx); | ||
| const skill = await ctx.db.get(args.skillId); | ||
| if (!skill) throw new Error("Skill not found"); | ||
| if (skill.ownerUserId !== user._id) { | ||
| assertModerator(user); | ||
| } | ||
|
|
||
| const now = Date.now(); | ||
| const patch: Partial<Doc<"skills">> = { | ||
| summary: args.summary, | ||
| updatedAt: now, | ||
| }; | ||
|
|
||
| await ctx.db.patch(skill._id, patch); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The edit button is shown to anyone where canManage is true, which includes members of the publisher that owns the skill (via myPublisherIds.has(skill.ownerPublisherId)). However, updateSummary only permits skill.ownerUserId === user._id or a moderator — publisher members are rejected.
When a publisher member (who is not the ownerUserId and not a moderator) hits "Save", they will receive an error and see window.alert("Failed to update summary. Please try again.").
Either extend the backend to accept publisher members, or restrict the edit button to isOwner only:
Option A — extend backend to also allow publisher members:
if (skill.ownerUserId !== user._id) {
let isPublisherMember = false;
if (skill.ownerPublisherId) {
const membership = await ctx.db
.query("publisherMembers")
.withIndex("by_publisher_user", (q) =>
q.eq("publisherId", skill.ownerPublisherId!).eq("userId", user._id),
)
.unique();
isPublisherMember = membership !== null;
}
if (!isPublisherMember) {
assertModerator(user);
}
}Option B — gate the button behind isOwner instead of canManage (simpler, matches the current backend check).
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/skills.ts
Line: 4799-4820
Comment:
**Publisher members can see "Edit summary" but are blocked by the backend**
The edit button is shown to anyone where `canManage` is true, which includes members of the publisher that owns the skill (via `myPublisherIds.has(skill.ownerPublisherId)`). However, `updateSummary` only permits `skill.ownerUserId === user._id` or a moderator — publisher members are rejected.
When a publisher member (who is not the `ownerUserId` and not a moderator) hits "Save", they will receive an error and see `window.alert("Failed to update summary. Please try again.")`.
Either extend the backend to accept publisher members, or restrict the edit button to `isOwner` only:
**Option A — extend backend to also allow publisher members:**
```ts
if (skill.ownerUserId !== user._id) {
let isPublisherMember = false;
if (skill.ownerPublisherId) {
const membership = await ctx.db
.query("publisherMembers")
.withIndex("by_publisher_user", (q) =>
q.eq("publisherId", skill.ownerPublisherId!).eq("userId", user._id),
)
.unique();
isPublisherMember = membership !== null;
}
if (!isPublisherMember) {
assertModerator(user);
}
}
```
**Option B — gate the button behind `isOwner` instead of `canManage`** (simpler, matches the current backend check).
How can I resolve this? If you propose a fix, please make it concise.| args: { | ||
| skillId: v.id("skills"), | ||
| summary: v.string(), | ||
| }, |
There was a problem hiding this comment.
v.string() imposes no upper bound, so a client can submit an arbitrarily long string. Consider capping the summary (e.g. 500 characters) both in the validator and with a maxLength attribute on the textarea:
In the handler, a quick guard:
if (args.summary.length > 500) throw new Error("Summary must be 500 characters or fewer.");And in the textarea:
<textarea maxLength={500} ... />Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/skills.ts
Line: 4800-4803
Comment:
**No maximum length validation on summary**
`v.string()` imposes no upper bound, so a client can submit an arbitrarily long string. Consider capping the summary (e.g. 500 characters) both in the validator and with a `maxLength` attribute on the textarea:
In the handler, a quick guard:
```ts
if (args.summary.length > 500) throw new Error("Summary must be 500 characters or fewer.");
```
And in the textarea:
```tsx
<textarea maxLength={500} ... />
```
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d93742ef9a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (skill.ownerUserId !== user._id) { | ||
| assertModerator(user); | ||
| } |
There was a problem hiding this comment.
This mutation only authorizes skill.ownerUserId (or moderators), but org-owned skills are managed by publisher members in the UI (canManage becomes true from ownerPublisherId membership), so those legitimate owners can open the editor and then always hit an authorization failure on save. As written, the new summary-edit feature is broken for publisher-owned skills unless a moderator performs the edit.
Useful? React with 👍 / 👎.
| <p className="section-subtitle"> | ||
| {isSummaryEditing ? ( | ||
| <form | ||
| onSubmit={(event) => { |
There was a problem hiding this comment.
When edit mode is active, this renders a <form> as a child of <p className="section-subtitle">, which is invalid HTML content nesting. Browsers will auto-correct that structure, which can cause SSR hydration warnings and inconsistent header layout/behavior when toggling summary edit mode. Wrapping the editable block in a non-<p> container avoids this.
Useful? React with 👍 / 👎.
1 participant
make the summary editable