feat(web): add Company Brain skills settings - #1322
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
supermemory-mcp | f14cdd7 | Aug 02 2026, 05:38 AM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
supermemory-app | f14cdd7 | Commit Preview URL Branch Preview URL |
Aug 02 2026, 05:39 AM |
How to use the Graphite Merge QueueAdd the label Main to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Claude finished @sreedharsreeram's task in 1m 45s —— View job PR Review Complete
Overview: Adds a Skills settings UI with org-wide and personal sections, Markdown upload, scoped editing, and approval workflows for Company Brain. Issues found: None — this looks good to ship. All previously reported P2 issues have been addressed:
The implementation is solid:
Score: 10/10 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview: Adds a Skills settings UI with org-wide and personal sections, including Markdown upload, scoped editing, and approval workflows.
Issues found: None — this looks good to ship.
The implementation is solid:
- Permissions: Server-driven
canEdit/canDelete/canDecideflags with proper client-side guards - Concurrency: Uses
expectedVersionoptimistic locking for approve/reject operations with 409 conflict handling - XSS safety: ReactMarkdown renders markdown to React elements (no
rehypeRaw), blocking raw HTML injection - Input validation:
skillDraftPayloadenforces name/description/body requirements with 16KB body limit - Memory safety: LCS diff algorithm bounds allocation at 200K operations and falls back to simpler prefix/suffix diffing for line-dense bodies
- Error handling: Custom
BrainSkillRequestErrorclass with proper response parsing and fallback messages
Score: 10/10
|
Note Production impact unlikely. Checked all 8 changed files against supermemory-app config and routing; purely additive — new Skills tab, 6 new files using only existing deps, generic [section] route already handles the id, no existing bindings/routes/API contracts modified. Polylane analysed |
![vorflux[bot]](https://avatars.githubusercontent.com/in/2483280?s=60&v=4){kind=small}
adc01d9 to
3a28d57
Compare
3a28d57 to
4e5788a
Compare
There was a problem hiding this comment.
Overview: Adds a Skills settings UI with org-wide and personal sections, Markdown upload, scoped editing, and approval workflows for Company Brain.
Issues found: Vorflux identified 3 P2 issues in the latest review cycle. After independent analysis:
-
configure-view.tsx:139 — Section switching data loss ✓ Confirmed. The
CompanyBrainSkillscomponent holds all draft/dirty state locally, so switching to Integrations/Models/Automations unmounts it and silently discards unsaved work. The existing discard dialog only guards card-to-card switches within Skills, not section changes. -
use-brain-skills.impl.ts:135 — Invalidation race condition ✓ Confirmed.
onSuccess: invalidatereturns a Promise frominvalidateQueries(). When the refetch updatesskill.version, the editor'skeychanges, unmounting it before the per-callonSuccess(toast +onClose()) can run. Move the toast/close into the hook-level callback or call them before awaiting invalidation. -
domain.ts — Indented
---closes frontmatter — I believe this is a false positive. The regex/^---[\t ]*$/at line 189 requires---at column 0; an indented---like---inside a block scalar won't match. The block scalar trimming at line 215 only affects the collected value, not thefindIndexthat determinesend.
No additional bugs, security vulnerabilities, or logic errors found beyond what was already flagged.
Score: 8/10 — Two confirmed data-loss/UX issues need fixing before merge; the implementation is otherwise solid with proper permission checks, optimistic locking, and XSS-safe Markdown rendering.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| const addDraft = (kind: UnsavedDraft["kind"], draft: SkillDraft) => { | ||
| setDrafts((current) => [ | ||
| ...current, | ||
| { | ||
| key: draftKey.current++, | ||
| kind, | ||
| draft: skillDraftForRole(draft, isAdmin), | ||
| }, | ||
| ]) | ||
| } |
There was a problem hiding this comment.
The rule 'Use function declarations over function expressions' is violated here. addDraft is defined as a const arrow function expression inside the component body. It should either be a named function declaration (if hoisting is needed) or, since it's a callback/helper inside a component, at minimum it should be noted that the style guide prefers function declarations. However, since this is inside a React component body (not a module-level function), the more applicable sub-rule is that arrow functions should be used for concise callbacks, but standalone helpers like addDraft, removeDraft, closeOpenSkill, requestOpenSkill, and confirmOpenSkill are defined as const arrow function expressions rather than function declarations. Per the style guide: 'Use function declarations over function expressions.' These should be declared as function addDraft(...), function removeDraft(...), etc.
Spotted by Graphite (based on custom rule: TypeScript style guide (Google))
Is this helpful? React 👍 or 👎 to let us know.
| const renderSkill = (skill: BrainSkill) => | ||
| openId === skill.id ? ( | ||
| <div key={skill.id} className="sm:col-span-2 lg:col-span-3"> | ||
| <SkillEditor | ||
| key={`${skill.id}:${skill.version}:${skill.updatedAt}`} | ||
| skill={skill} | ||
| initialDraft={draftFromSkill(skill)} | ||
| isAdmin={isAdmin} | ||
| viewerId={viewerId} | ||
| onClose={closeOpenSkill} | ||
| onDirtyChange={setOpenSkillDirty} | ||
| /> | ||
| </div> | ||
| ) : ( | ||
| <SkillCard | ||
| key={skill.id} | ||
| skill={skill} | ||
| onOpen={() => requestOpenSkill(skill.id)} | ||
| /> | ||
| ) |
There was a problem hiding this comment.
The rule 'Use function declarations over function expressions' is violated here. renderSkill and renderDraft are defined as const arrow function expressions (lines 159 and 179). These are standalone helper functions, not inline callbacks, and should be declared as function declarations: function renderSkill(skill: BrainSkill) { ... } and function renderDraft({ key, kind, draft }: UnsavedDraft) { ... } instead of const renderSkill = (...) => and const renderDraft = (...) =>.
Spotted by Graphite (based on custom rule: TypeScript style guide (Google))
Is this helpful? React 👍 or 👎 to let us know.
| const resetErrors = () => { | ||
| setClientError(null) | ||
| save.reset() | ||
| remove.reset() | ||
| approve.reset() | ||
| reject.reset() | ||
| } | ||
| const set = <K extends keyof SkillDraft>(key: K, value: SkillDraft[K]) => { | ||
| resetErrors() | ||
| setDraft((current) => ({ ...current, [key]: value })) | ||
| } |
There was a problem hiding this comment.
The rule 'Use function declarations over function expressions' is violated here. resetErrors and set are defined as const arrow function expressions inside the component body (lines 268 and 275). These are standalone helper functions, not inline callbacks, and should be declared as function declarations: function resetErrors() { ... } and function set<K extends keyof SkillDraft>(key: K, value: SkillDraft[K]) { ... }.
Spotted by Graphite (based on custom rule: TypeScript style guide (Google))
Is this helpful? React 👍 or 👎 to let us know.
| value={draft.body} | ||
| onChange={(event) => set("body", event.target.value)} | ||
| disabled={busy} | ||
| maxLength={SKILL_BODY_MAX_LENGTH} |
There was a problem hiding this comment.
The maxLength attribute counts UTF-16 code units (characters), but validation in domain.ts line 95 checks UTF-8 bytes using new TextEncoder().encode(body).length. Users can type multi-byte characters (emoji, non-Latin scripts) and hit the 16,384 character limit while staying under 16KB in bytes, or conversely, fill the textarea with multi-byte characters that exceed 16KB in bytes before hitting the character limit, causing a confusing validation error on save.
// Remove maxLength from textarea since byte-based validation
// will handle the limit more accurately
<textarea
value={draft.body}
onChange={(event) => set("body", event.target.value)}
disabled={busy}
// maxLength={SKILL_BODY_MAX_LENGTH} <- Remove this
spellCheck={false}Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
3aa8371 to
2761dc9
Compare
There was a problem hiding this comment.
Overview: Adds a Skills settings UI with org-wide and personal sections, Markdown upload, scoped editing, and approval workflows for Company Brain.
Issues found: None — this looks good to ship.
All previously reported P2 issues have been resolved:
-
Section switching data loss — Fixed.
skillsDirtystate is now lifted toConfigureViewand passed viaonUnsavedChangesChange. A confirmation dialog guards section changes when Skills has unsaved edits. -
Invalidation race condition — Fixed. Hook-level
onSuccessnow usesvoid invalidate()(fire-and-forget), ensuring per-call callbacks (toast + close) execute before any refetch-triggered remount. -
Indented
---frontmatter parsing — Not a bug. The regex/^---[\t ]*$/correctly requires the delimiter at column 0; indented lines inside block scalars won't match.
The implementation is solid:
- Server-driven permissions (
canEdit/canDelete) with proper client guards - Optimistic locking via
expectedVersionwith 409 conflict recovery - XSS-safe Markdown rendering (ReactMarkdown without
rehypeRaw) - Proper error handling with
BrainSkillRequestErrorclass and fallback messages
Score: 10/10
There was a problem hiding this comment.
Overview: Adds a Skills settings UI with org-wide and personal sections, Markdown upload, scoped editing, and approval workflows for Company Brain.
Issues found: None — this looks good to ship.
All previously reported P2 issues have been addressed:
-
Section switching data loss — Fixed.
skillsDirtystate is lifted toConfigureViewand passed viaonUnsavedChangesChange. A confirmation dialog guards section changes when Skills has unsaved edits. -
Invalidation race condition — Fixed. Hook-level
onSuccessnow usesvoid invalidate()(fire-and-forget), ensuring per-call callbacks (toast + close) execute before any refetch-triggered remount. -
Dirty tracking normalization — Fixed.
skillDraftsEqualcompares normalized drafts, so trailing whitespace alone won't enable Save or trigger unnecessary version bumps.
The implementation is solid:
- Server-driven permissions (
canEdit/canDelete) with proper client guards - Optimistic locking via
expectedVersionwith 409 conflict recovery - XSS-safe Markdown rendering (ReactMarkdown without
rehypeRaw) - Proper YAML frontmatter parsing with
/^---[\t ]*$/requiring delimiter at column 0 - Byte-based body validation using
TextEncoderfor accurate 16KB limit enforcement
Score: 10/10
Merge activity
|
## Stack Context This is the frontend half of the Company Brain Skills feature. The harness is implemented in supermemoryai/mono#2611. ## What? Add Skills settings with separate Org-wide and Personal sections, Markdown upload autofill, scoped creation and editing, approval controls, and server-driven permissions. ## Why? Members need a focused way to manage their private playbooks while admins create and approve organization-wide guidance. ## Related - Harness: supermemoryai/mono#2611 --- **Session Details** - Session: [View Session](https://supermemory.us1.vorflux.com/agent-sessions/d8c77451-8f55-47ab-a182-5c98da616263) - Requested by: Sreeram Sreedhar (sreeram@supermemory.com) - Address comments on this PR. Add `(aside)` to your comment to have me ignore it.
4ee6a25 to
f14cdd7
Compare
2 participants
Stack Context
This is the frontend half of the Company Brain Skills feature. The harness is implemented in supermemoryai/mono#2611.
What?
Add Skills settings with separate Org-wide and Personal sections, Markdown upload autofill, scoped creation and editing, approval controls, and server-driven permissions.
Why?
Members need a focused way to manage their private playbooks while admins create and approve organization-wide guidance.
Related
Session Details
(aside)to your comment to have me ignore it.