Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new auth package, a Svelte UI library, a Profile Editor platform (API + client) with EVault/Registry integrations and TypeORM-backed sessions/users, DreamSync profile features, many UI components/stores, README/env/docs tweaks, and a control-panel admin-list cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Client as Profile Editor Client
participant API as Profile Editor API
participant Registry as Registry Service
participant EVault as eVault GraphQL
participant DB as Database
User->>Client: Scan QR or open deeplink
Client->>API: GET /api/auth/offer
API->>API: buildAuthOffer(PUBLIC_PROFILE_EDITOR_BASE_URL,...)
API-->>Client: { uri, session }
User->>API: POST /api/auth (ename, signature, session)
API->>Registry: verify signature / resolve ename
Registry-->>API: verification result + evault URL
API->>EVault: fetch user profile
EVault-->>API: profile data
API->>DB: registerSession(userId, ename, token)
DB-->>API: session persisted
API-->>Client: { token, user }
Client->>Client: persist token, update auth state
Client->>API: GET /api/professional-profile (auth)
API->>EVault: getProfile(ename)
EVault-->>API: full profile
API-->>Client: profile
sequenceDiagram
participant Sync as EVault Sync Service
participant Registry as Registry Service
participant EVault as eVault GraphQL
participant Search as User Search Service
participant API as Profile Editor API
Sync->>Registry: GET /list
Registry-->>Sync: vault list
Sync->>EVault: fetch profiles for each ename
EVault-->>Sync: profile data
Sync->>Search: upsertFromWebhook(profilePayload)
Search-->>Sync: ack
API->>EVault: upsertProfile(ename, data) (via controller)
EVault-->>API: updated FullProfile
API-->>Client: response (profile updated)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
platforms/dreamsync/client/client/src/lib/apiClient.ts (1)
6-10:⚠️ Potential issue | 🟡 MinorRemove or guard debug logging that exposes environment variables.
Logging
allEnv: import.meta.envmay expose sensitive configuration in production browser consoles. Consider removing this debug statement or guarding it with a development-only check.🛡️ Proposed fix
-console.log("🔍 Environment variables:", { - VITE_DREAMSYNC_BASE_URL: import.meta.env.VITE_DREAMSYNC_BASE_URL, - baseURL: baseURL, - allEnv: import.meta.env -}); +if (import.meta.env.DEV) { + console.log("🔍 Environment variables:", { + VITE_DREAMSYNC_BASE_URL: import.meta.env.VITE_DREAMSYNC_BASE_URL, + baseURL: baseURL, + }); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/dreamsync/client/client/src/lib/apiClient.ts` around lines 6 - 10, Remove or guard the debug console.log in apiClient.ts that prints import.meta.env and other env details; either delete the statement or wrap it in a development-only check (e.g., import.meta.env.DEV) so the log never runs in production, and ensure any remaining log only prints non-sensitive values like baseURL (variable name baseURL) or sanitized info.README.md (1)
20-39:⚠️ Potential issue | 🟡 MinorDocument the new
profile-editorservice in the repo map.This README section is still missing the
platforms/profile-editorAPI/client introduced by this PR, so the service inventory and project structure become stale as soon as the feature lands.Also applies to: 69-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 20 - 39, The README's service inventory is missing the new platforms/profile-editor API/client; update the platforms list to include the profile-editor entry (e.g., add a new bullet under "Platform APIs" for the profile-editor service with its port or description) and also add any corresponding frontend entry if the PR introduced a frontend (e.g., under "Frontend Services"); modify the README section that lists Platform APIs and Frontend Services (the blocks showing ports like 3000, 3002, etc.) so the new "profile-editor" service appears consistently (also replicate the change in the later occurrence around lines 69-104 mentioned in the review).
🟠 Major comments (38)
platforms/profile-editor/api/src/web3adapter/watchers/subscriber.ts-73-95 (1)
73-95:⚠️ Potential issue | 🟠 MajorThis drops later writes instead of debouncing them.
Once a key is present in
pendingChanges, every subsequent change for that row is ignored until the timer fires. If the same profile is edited twice within 3 seconds, the adapter receives the first snapshot, not the latest one. Store the timeout handle and latest payload per key, then clear/reschedule on each event.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/web3adapter/watchers/subscriber.ts` around lines 73 - 95, The current logic using pendingChanges as a Set-like map causes later updates to be dropped because once a changeKey is present subsequent events return; modify pendingChanges to map changeKey -> { timer: NodeJS.Timeout, latestData: RowType } (store the timeout handle and the latest payload), on each event if an entry exists clearTimeout(entry.timer) and replace entry.latestData, then schedule a new setTimeout that calls this.adapter.mappingDb.getGlobalId(...), checks this.adapter.lockedIds, and finally calls this.adapter.handleChange with entry.latestData and tableName.toLowerCase(); when the timeout runs delete the entry from pendingChanges. Ensure references to pendingChanges, changeKey, this.adapter.mappingDb.getGlobalId, this.adapter.lockedIds and this.adapter.handleChange are used so the reschedule/update behavior uses the latest payload.platforms/profile-editor/api/src/web3adapter/watchers/subscriber.ts-45-48 (1)
45-48:⚠️ Potential issue | 🟠 MajorDon't synthesize table names by appending
"s".
event.metadata.tableNameshould already be the canonical table name. This breaks irregular names (category→categorys) and can make inserts use a different key than updates/removes, which also bypasses the skip-list inhandleChangewhen entity tables are singular.Suggested fix
- const tableName = event.metadata.tableName.endsWith("s") - ? event.metadata.tableName - : event.metadata.tableName + "s"; - await this.handleChange(this.entityToPlain(entity), tableName); + await this.handleChange( + this.entityToPlain(entity), + event.metadata.tableName, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/web3adapter/watchers/subscriber.ts` around lines 45 - 48, The code is incorrectly synthesizing table names by appending "s" to event.metadata.tableName; instead, use the canonical name provided by event.metadata.tableName directly. Update the call in subscriber.ts so handleChange receives this.entityToPlain(entity) and event.metadata.tableName (no pluralization logic or conditional that appends "s"), ensuring you remove the conditional that modifies tableName and keep references to handleChange and entityToPlain unchanged.platforms/profile-editor/api/src/web3adapter/watchers/subscriber.ts-105-129 (1)
105-129:⚠️ Potential issue | 🟠 MajorGuard
entityToPlainagainst cyclic relations.TypeORM entity graphs commonly contain back-references. A loaded bidirectional relation will recurse forever here and blow the stack. Track visited objects with a
WeakSetor serialize relation IDs instead of walking arbitrary nested objects.Suggested fix
- private entityToPlain(entity: any): any { + private entityToPlain(entity: any, seen = new WeakSet<object>()): any { if (!entity || typeof entity !== "object") return entity; if (entity instanceof Date) return entity.toISOString(); + if (seen.has(entity)) return undefined; + seen.add(entity); if (Array.isArray(entity)) - return entity.map((item) => this.entityToPlain(item)); + return entity.map((item) => this.entityToPlain(item, seen)); @@ } else if (value instanceof Date) { plain[key] = value.toISOString(); } else { - plain[key] = this.entityToPlain(value); + plain[key] = this.entityToPlain(value, seen); } } else { plain[key] = value;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/web3adapter/watchers/subscriber.ts` around lines 105 - 129, The entityToPlain function can infinite-recurse on cyclic TypeORM relations; modify entityToPlain to accept an optional WeakSet seen (default new WeakSet()) and at the start check if seen.has(entity) then return a safe representation (e.g., entity.id || null or a short marker) to break cycles; before recursing into an object add seen.add(entity) and pass seen into all recursive calls (entityToPlain(item, seen) and entityToPlain(value, seen)) so back-references are detected and prevented from infinite recursion.platforms/profile-editor/api/src/web3adapter/watchers/subscriber.ts-77-102 (1)
77-102:⚠️ Potential issue | 🟠 MajorMove the try-catch block inside the async callback to properly handle rejections.
The outer
try/catchcannot catch errors from the async callback becausesetTimeout()is synchronous—it returns immediately after scheduling. Rejections fromawait this.adapter.mappingDb.getGlobalId(data.id)andawait this.adapter.handleChange(...)at lines 81 and 88 will become unhandled promise rejections since the innertryblock has nocatchclause.Suggested fix
- try { - setTimeout(async () => { - try { + setTimeout(() => { + void (async () => { + try { let globalId = await this.adapter.mappingDb.getGlobalId(data.id); globalId = globalId ?? ""; @@ -88,14 +88,14 @@ await this.adapter.handleChange({ data, tableName: tableName.toLowerCase(), }); + } catch (error) { + console.error( + `Error processing change for ${tableName}:`, + error, + ); } finally { this.pendingChanges.delete(changeKey); } - }, 3_000); - } catch (error) { - console.error( - `Error processing change for ${tableName}:`, - error, - ); - this.pendingChanges.delete(changeKey); - } + })(); + }, 3_000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/web3adapter/watchers/subscriber.ts` around lines 77 - 102, The outer try/catch around setTimeout cannot catch rejections from the async callback; move the error handling into the async callback (or add a catch) so awaits inside it are guarded: wrap the code inside the setTimeout callback in a try/catch that logs the error (include tableName and error) and always calls this.pendingChanges.delete(changeKey) in finally; remove the outer try/catch or keep it but ensure it does not assume it will catch async errors from setTimeout; update references to this.adapter.mappingDb.getGlobalId, this.adapter.handleChange, this.adapter.lockedIds, and this.pendingChanges.delete accordingly.platforms/profile-editor/api/src/web3adapter/watchers/subscriber.ts-51-66 (1)
51-66:⚠️ Potential issue | 🟠 MajorUse fallback fields or load entity explicitly to capture all TypeORM writes.
event.entityis absent or partial forrepository.update(),repository.delete(), and query-builder updates, causing lines 53 and 62 to silently skip these operations. Since the subscriber mirrors changes, it will miss writes executed via these common paths. Useevent.entityId/event.databaseEntityas fallbacks, or explicitly load the entity before forwarding tohandleChange().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/web3adapter/watchers/subscriber.ts` around lines 51 - 66, afterUpdate and afterRemove currently bail out when event.entity is missing, which causes missed writes from repository.update/delete and query-builder operations; update afterUpdate and afterRemove to fallback to event.databaseEntity or load the entity by id via event.entityId (or repository.findOne) before calling this.entityToPlain(...) and this.handleChange(...); ensure you reference the subscriber methods afterUpdate/afterRemove, use event.entityId and event.databaseEntity as fallbacks, and keep calls to entityToPlain and handleChange unchanged once a full entity is obtained.packages/auth/src/verify-login.ts-10-21 (1)
10-21:⚠️ Potential issue | 🟠 MajorAdd error handling to normalize exceptions into the return type.
verifySignature()may throw (network errors, timeouts, etc.), but this function lacks a try-catch. Exceptions bypass the declared return shape{ valid, error, publicKey }, forcing callers to handle both exception paths and result checks. Since the return type includes an optionalerrorfield, exceptions should be caught and normalized.Suggested fix
export async function verifyLoginSignature( config: LoginVerificationConfig, ): Promise<LoginVerificationResult> { - const result = await verifySignature({ - eName: config.eName, - signature: config.signature, - payload: config.session, - registryBaseUrl: config.registryBaseUrl, - }); - - return { - valid: result.valid, - error: result.error, - publicKey: result.publicKey, - }; + try { + const result = await verifySignature({ + eName: config.eName, + signature: config.signature, + payload: config.session, + registryBaseUrl: config.registryBaseUrl, + }); + + return { + valid: result.valid, + error: result.error, + publicKey: result.publicKey, + }; + } catch (error) { + return { + valid: false, + error: + error instanceof Error ? error.message : "Signature verification failed", + }; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth/src/verify-login.ts` around lines 10 - 21, Wrap the call to verifySignature in a try/catch inside the function in verify-login.ts: call verifySignature(...) in a try block and return its result as before when successful, and in the catch return a normalized object like { valid: false, error: String(err) or err.message, publicKey: undefined } so all failure modes conform to the { valid, error, publicKey } return shape; reference the verifySignature call and the returned fields (valid, error, publicKey) when making the change.packages/auth/src/jwt.ts-14-18 (1)
14-18:⚠️ Potential issue | 🟠 MajorGuard the decoded value before returning it as
JwtPayload.
jwt.verify()without thecomplete: trueoption returnsstring | JwtPayload. The cast on line 16 bypasses TypeScript's type safety, allowing a string to be returned at runtime even though the function signature promises aJwtPayloadobject. Callers will fail when accessing properties likedecoded.userId.Suggested fix
export function verifyToken(token: string, secret: string): JwtPayload { try { - return jwt.verify(token, secret) as JwtPayload; + const decoded = jwt.verify(token, secret); + if (typeof decoded !== "object" || decoded === null) { + throw new Error("Invalid token"); + } + return decoded as JwtPayload; } catch { throw new Error("Invalid token"); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth/src/jwt.ts` around lines 14 - 18, The function verifyToken currently casts jwt.verify(...) to JwtPayload which can hide a string result; update verifyToken to call jwt.verify(token, secret) into a local const (e.g., const decoded = jwt.verify(...)) and then guard that decoded is an object (and has expected properties like userId) before returning it as JwtPayload—if decoded is a string or missing required fields, throw a clear Error ("Invalid token" or "Unexpected token payload"); reference the verifyToken function and JwtPayload type when making this change.packages/ui/src/lib/components/ui/select/select-label.svelte-13-20 (1)
13-20:⚠️ Potential issue | 🟠 MajorUse
SelectPrimitive.GroupHeadinginstead of a plain<div>.The current implementation is only a visual heading and lacks the ARIA semantics needed for accessible grouped selects. In Bits UI v2,
SelectPrimitive.GroupHeading(already used byselect-group-heading.sveltein this project) is the designated component for labeling select groups. Using a plain<div>breaks the accessibility connection between the group and its label, causing screen readers to announce options without their group context.Apply the same pattern used in
select-group-heading.svelte: replace the<div>wrapper with<SelectPrimitive.GroupHeading>and update the props type to match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/lib/components/ui/select/select-label.svelte` around lines 13 - 20, Replace the plain div in select-label.svelte with the ARIA-aware SelectPrimitive.GroupHeading used elsewhere (see select-group-heading.svelte) so the label is announced as a group heading; update the component wrapper from <div> to SelectPrimitive.GroupHeading, ensure props/types align with SelectPrimitive.GroupHeading (pass bind:this={ref}, class via cn('px-2 py-1.5 text-xs text-muted-foreground', className), spread {...restProps}) and render children the same way to preserve visuals and accessibility.packages/auth/src/middleware.ts-19-29 (1)
19-29:⚠️ Potential issue | 🟠 MajorDon't turn repository failures into
401 Invalid token.
config.findUserat Line 19 is an operational dependency. If it throws, the catch currently returns the same 401 as a bad JWT, which will hide DB outages and make retries/debugging much harder. Keep 401 for auth failures only and forward unexpected errors withnext(error)or a 500.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth/src/middleware.ts` around lines 19 - 29, The catch block in the auth middleware currently turns any exception (including operational errors from config.findUser) into a 401 "Invalid token"; change it so only JWT/authentication failures result in a 401 while unexpected errors are propagated: specifically keep the logic that returns res.status(401) for token/verification problems, but when config.findUser (or other calls) throws, call next(error) (or respond with 500) instead of returning 401; update the try/catch around the decoded = verify(...) / config.findUser(decoded.userId) flow in the middleware to distinguish token verification errors from operational errors and set req.user + next() unchanged on success.platforms/profile-editor/api/src/database/data-source.ts-9-9 (1)
9-9:⚠️ Potential issue | 🟠 MajorFix
.envpath resolution to point to repository root.From
platforms/profile-editor/api/src/database,../../../../.envresolves toplatforms/.env, not the repository root. This causesPROFILE_EDITOR_DATABASE_URLand other environment variables to remain unset unless explicitly exported by the shell.Fix
-config({ path: path.resolve(__dirname, "../../../../.env") }); +config({ path: path.resolve(__dirname, "../../../../../.env") });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/database/data-source.ts` at line 9, The .env path in the config call is incorrect: update the path passed to config(path.resolve(...)) so it resolves to the repository root instead of platforms/.env; specifically change the path resolution expression used in the config(...) invocation (the call that currently uses path.resolve(__dirname, "../../../../.env")) to climb one more level (e.g. use ../../../../../.env or equivalent use of path.resolve(__dirname, '..', '..', '..', '..', '..', '.env')) so PROFILE_EDITOR_DATABASE_URL and other env vars load correctly.packages/ui/src/lib/components/ui/input/input.svelte-33-35 (1)
33-35:⚠️ Potential issue | 🟠 MajorRemove
bind:valuefrom file input.Line 35 binds both
filesandvalueto<input type="file">. File input values are read-only in browsers; Svelte's file-input binding API isbind:filesonly. Bindingvaluewill fail when state tries to drive it.Suggested fix
type="file" bind:files - bind:value {...restProps} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/lib/components/ui/input/input.svelte` around lines 33 - 35, The file input in input.svelte is incorrectly binding both bind:files and bind:value; remove the read-only binding bind:value from the <input type="file"> element so only bind:files remains. Locate the file input element (the <input> with type="file" in the component) and delete the bind:value attribute, keeping bind:files to preserve file selection handling.platforms/profile-editor/client/src/routes/(auth)/auth/+page.svelte-108-108 (1)
108-108:⚠️ Potential issue | 🟠 MajorPotential null dereference on session extraction.
If
qrDataURL lacks asessionparameter,searchParams.get('session')returnsnull, but theas stringcast masks this.watchEventStreamwould then connect to an invalid SSE endpoint.🛡️ Proposed fix to validate session param
- watchEventStream(new URL(qrData).searchParams.get('session') as string); + const sessionId = new URL(qrData).searchParams.get('session'); + if (!sessionId) { + errorMessage = 'Invalid authentication offer. Please refresh the page.'; + return; + } + watchEventStream(sessionId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/routes/`(auth)/auth/+page.svelte at line 108, The call to watchEventStream(new URL(qrData).searchParams.get('session') as string) can pass null because searchParams.get('session') may be absent; update the code that uses qrData and the URL to first extract const session = new URL(qrData).searchParams.get('session') and validate it (e.g., if (!session) handle the error/notify user and return) before calling watchEventStream(session); ensure you do not use an "as string" cast to mask null and add a clear fallback or early exit to avoid connecting to an invalid SSE endpoint.platforms/profile-editor/client/src/routes/(auth)/auth/+page.svelte-73-74 (1)
73-74:⚠️ Potential issue | 🟠 MajorMissing error handling for auth offer fetch.
If the
/api/auth/offerrequest fails, the error propagates unhandled, leaving the user on a page with an infinite skeleton loader and no feedback.🛡️ Proposed fix to handle offer fetch errors
+ try { const { data } = await apiClient.get('/api/auth/offer'); qrData = data.uri; + } catch (error) { + console.error('Failed to fetch auth offer:', error); + errorMessage = 'Failed to initialize login. Please refresh the page.'; + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/routes/`(auth)/auth/+page.svelte around lines 73 - 74, The fetch of the auth offer using apiClient.get('/api/auth/offer') lacks error handling, so failures leave the UI stuck; wrap the call that assigns qrData in a try/catch around the apiClient.get call (the code that sets qrData) and on error set an error state/flag (e.g., set a local error message or boolean) and clear any loading state so the skeleton stops, then surface a user-friendly message or fallback UI; ensure the catch references the same variables (qrData, loading/error flags used by the page) so the page can render the error instead of hanging.platforms/profile-editor/client/src/routes/(protected)/+layout.svelte-11-28 (1)
11-28:⚠️ Potential issue | 🟠 MajorGuard protected routes from
$isAuthenticated, not raw token presence.Lines 11-15 redirect only when the token is missing, but lines 18-28 render only when
$isAuthenticatedis true. A stale or invalid token will leave this layout blank instead of sending the user to/auth.🐛 Suggested change
$effect(() => { - if (!getAuthToken()) { - goto('/auth'); + if (!$isAuthenticated) { + goto('/auth', { replaceState: true }); } });After this change,
getAuthTokencan be dropped from the imports as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/routes/`(protected)/+layout.svelte around lines 11 - 28, The redirect effect currently uses getAuthToken() causing a mismatch with the render guard ($isAuthenticated) and allowing stale/invalid tokens to produce a blank layout; update the $effect callback to check the reactive store $isAuthenticated (negated) and call goto('/auth') when !$isAuthenticated, and remove the now-unused getAuthToken import; make sure you reference the same symbols ($effect, $isAuthenticated, goto) in the change so the redirect and conditional rendering match.platforms/profile-editor/client/src/lib/components/profile/ProfileCard.svelte-10-13 (1)
10-13:⚠️ Potential issue | 🟠 MajorEncode
enamebefore putting it into a path segment.Both the avatar URL and the profile link interpolate raw user data. An
enamecontaining/,?,#, or spaces will produce the wrong route/request. UseencodeURIComponentonce and reuse the encoded value for both paths.Suggested fix
let { result }: { result: ProfileSearchResult } = $props(); + const encodedEname = result.ename ? encodeURIComponent(result.ename) : null; function avatarProxyUrl(): string | null { - if (result.ename) { - return `${PUBLIC_PROFILE_EDITOR_BASE_URL}/api/profiles/${result.ename}/avatar`; + if (encodedEname) { + return `${PUBLIC_PROFILE_EDITOR_BASE_URL}/api/profiles/${encodedEname}/avatar`; } return null; } </script> -<a href="/profile/{result.ename}" class="block"> +<a href={encodedEname ? `/profile/${encodedEname}` : '#'} class="block">Also applies to: 18-18
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/lib/components/profile/ProfileCard.svelte` around lines 10 - 13, The avatar URL is interpolating raw result.ename into a path; encode result.ename with encodeURIComponent and reuse that encoded value for all path/interpolations (e.g., inside avatarProxyUrl() and the profile link generation at the other occurrence) so characters like "/", "?", "#" and spaces are escaped; compute const encodedEname = encodeURIComponent(result.ename) (or similar) and use encodedEname in the template strings for both the avatar endpoint and the profile URL.platforms/profile-editor/client/src/lib/components/profile/EducationSection.svelte-14-15 (1)
14-15:⚠️ Potential issue | 🟠 MajorEditing by array index breaks after a removal.
editIndexis positional, but rows can still be deleted while the form is open. If I edit item 3, delete item 1, then save, the stale index now points at the wrong entry and can overwrite or append the wrong row. Track the edited record by a stable identifier, or at least adjust/cleareditIndexafter removals.Also applies to: 29-37, 45-48, 120-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/lib/components/profile/EducationSection.svelte` around lines 14 - 15, The current editIndex is a positional index and breaks when rows are removed; change the editing logic to track the edited record by a stable identifier (e.g., use an id field on Education and replace editIndex with editId) or, if you cannot add an id, update/clear editIndex on removals (in the remove handler: if removedIndex === editIndex clear editIndex and reset form; if removedIndex < editIndex decrement editIndex). Update all places using editIndex and form (references to editIndex, form, and any save/remove handlers) so saving finds the item by id or recalculated index before mutating the list. Ensure the save routine locates the target entry by stable id (or current index lookup) instead of assuming the original positional index.platforms/profile-editor/client/src/routes/(protected)/settings/+page.svelte-17-25 (1)
17-25:⚠️ Potential issue | 🟠 MajorReset the fetch guard when loading the profile fails.
lastFetchedEnameis updated beforefetchProfile()completes. If that request fails once, the effect will never retry for the same user because the guard is already tripped. Clear the sentinel in thecatch, or track an explicit in-flight state instead.Suggested fix
$effect(() => { const ename = $currentUser?.ename; if (ename && ename !== lastFetchedEname) { lastFetchedEname = ename; untrack(() => { fetchProfile().catch(() => { + if (lastFetchedEname === ename) { + lastFetchedEname = ''; + } toast.error('Failed to load profile'); }); }); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/routes/`(protected)/settings/+page.svelte around lines 17 - 25, The effect updates the sentinel lastFetchedEname before awaiting fetchProfile(), so a failed fetch permanently prevents retries; modify the $effect block that calls fetchProfile() (the untrack wrapper around fetchProfile()) to reset lastFetchedEname in the fetchProfile().catch handler (or alternatively track an explicit in-flight flag) so that on error you clear the sentinel and allow future attempts; reference lastFetchedEname, $effect, fetchProfile, untrack and the existing .catch/toast.error to implement the reset.platforms/profile-editor/client/src/lib/components/profile/EducationSection.svelte-122-126 (1)
122-126:⚠️ Potential issue | 🟠 MajorAdd accessible names to the icon-only action buttons.
These controls render only SVGs, so screen-reader users do not get a usable label for the edit/delete actions. Add
aria-label(or visible text) to both buttons.Suggested fix
- <Button variant="ghost" size="icon-sm" onclick={() => openEdit(i)}> + <Button + variant="ghost" + size="icon-sm" + aria-label="Edit education entry" + onclick={() => openEdit(i)} + > <svg class="h-4 w-4" fill="none" stroke="currentColor" viewBox="0 0 24 24"><path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M15.232 5.232l3.536 3.536m-2.036-5.036a2.5 2.5 0 113.536 3.536L6.5 21.036H3v-3.572L16.732 3.732z" /></svg> </Button> - <Button variant="ghost" size="icon-sm" onclick={() => remove(i)}> + <Button + variant="ghost" + size="icon-sm" + aria-label="Remove education entry" + onclick={() => remove(i)} + > <svg class="h-4 w-4 text-destructive" fill="none" stroke="currentColor" viewBox="0 0 24 24"><path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M19 7l-.867 12.142A2 2 0 0116.138 21H7.862a2 2 0 01-1.995-1.858L5 7m5 4v6m4-6v6m1-10V4a1 1 0 00-1-1h-4a1 1 0 00-1 1v3M4 7h16" /></svg> </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/lib/components/profile/EducationSection.svelte` around lines 122 - 126, The icon-only action Buttons for editing and deleting lack accessible names; update the Button elements that call openEdit(i) and remove(i) to include descriptive accessible labels (e.g., aria-label="Edit education entry" and aria-label="Delete education entry" or visible text) so screen readers can identify their purpose, ensuring the labels reflect the action and context (edit/delete on the EducationSection item).platforms/profile-editor/client/src/routes/(protected)/settings/+page.svelte-100-113 (1)
100-113:⚠️ Potential issue | 🟠 MajorGuard
professionalbefore readingisPublic.
{#if$profile}does not guarantee$profile.professionalexists. On a new or partially populated profile, this section will throw during render/click handling before the user can change visibility.Suggested fix
+ {`@const` isPublic = $profile.professional?.isPublic !== false} <Button - variant={$profile.professional.isPublic !== false ? 'default' : 'outline'} + variant={isPublic ? 'default' : 'outline'} size="sm" onclick={async () => { - const newValue = $profile?.professional.isPublic === false; + const newValue = !isPublic; try { await updateProfile({ isPublic: newValue }); toast.success(newValue ? 'Profile is now public' : 'Profile is now private'); } catch { toast.error('Failed to update visibility'); } }} > - {$profile.professional.isPublic !== false ? 'Public' : 'Private'} + {isPublic ? 'Public' : 'Private'} </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/routes/`(protected)/settings/+page.svelte around lines 100 - 113, The code reads $profile.professional.isPublic without guaranteeing professional exists; update the Button text and variant logic and the onclick handler to guard against a missing professional (use optional chaining / null coalescing) — e.g., reference the same symbols ($profile.professional, isPublic, updateProfile, the onclick handler and Button) and change expressions to use $profile?.professional?.isPublic ?? false (or similar) when deciding variant/text and compute newValue from $profile?.professional?.isPublic; also ensure updateProfile is called with a proper payload creating/patching professional if needed or handle the missing professional case before calling updateProfile to avoid runtime errors.packages/ui/package.json-111-136 (1)
111-136:⚠️ Potential issue | 🟠 MajorShip runtime imports as regular dependencies.
@lucide/svelteis imported by an exported UI component, but it is only listed indevDependencieshere. Consumers of@metastate-foundation/uiwill not install dev dependencies, so published components can fail with module-resolution errors. Please move every package imported by shipped source fromdevDependenciestodependencies— at least@lucide/svelte, and likely any runtime helper packages used fromsrc/libas well.Run this to list any external modules imported from
packages/ui/src/libthat are only declared indevDependencies. The expected result is no output.#!/bin/bash set -euo pipefail python - <<'PY' import json import pathlib import re pkg = json.loads(pathlib.Path("packages/ui/package.json").read_text()) deps = set((pkg.get("dependencies") or {}).keys()) dev_deps = set((pkg.get("devDependencies") or {}).keys()) imports = {} for path in pathlib.Path("packages/ui/src/lib").rglob("*"): if path.suffix not in {".ts", ".js", ".svelte"}: continue text = path.read_text() for match in re.finditer(r"""from\s+['"]([^'"]+)['"]""", text): mod = match.group(1) if mod.startswith(".") or mod.startswith("$"): continue base = "/".join(mod.split("/")[:2]) if mod.startswith("@") else mod.split("/")[0] imports.setdefault(base, set()).add(str(path)) for mod in sorted(imports): if mod in dev_deps and mod not in deps: print(mod) for p in sorted(imports[mod]): print(f" - {p}") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/package.json` around lines 111 - 136, The package.json currently lists runtime-shipped packages like `@lucide/svelte` under devDependencies, causing consumers of the exported UI components to fail at runtime; edit packages/ui/package.json to move every module imported by shipped source (scan packages/ui/src/lib for non-relative imports) from devDependencies into dependencies (at minimum move "@lucide/svelte"), ensure dependencies contains each runtime package referenced by exported components (e.g., any imports found in src/lib .svelte/.ts/.js files), and re-run the provided verification script to confirm there are no dev-only runtime imports remaining.platforms/profile-editor/client/src/lib/components/profile/SocialLinksSection.svelte-91-93 (1)
91-93:⚠️ Potential issue | 🟠 MajorAdd an accessible name to the remove-link button.
The control at Line 91 is icon-only, so assistive tech will announce it as just “button”. Give it an
aria-labelthat includes the platform or label being removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/lib/components/profile/SocialLinksSection.svelte` around lines 91 - 93, The remove-link Button in SocialLinksSection.svelte is icon-only and needs an accessible name: update the Button element (used where removeLink(i) is invoked) to include an aria-label that describes the action and target (e.g., "Remove {platform}" or "Remove {link.label}") so screen readers announce which link will be removed; use the available data in the surrounding scope (the link object or its platform/label) to interpolate the label and keep the onclick calling removeLink(i) unchanged.platforms/profile-editor/client/src/routes/(protected)/discover/+page.svelte-91-95 (1)
91-95:⚠️ Potential issue | 🟠 MajorLabel the filter-removal button.
The chip delete control at Line 93 is icon-only. Add an
aria-labellikeRemove React filterso screen-reader users can tell which filter they are removing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/routes/`(protected)/discover/+page.svelte around lines 91 - 95, The icon-only filter chip delete button lacks an accessible label; update the button inside the Badge (the element that calls removeSkillFilter(skill)) to include an aria-label that incorporates the skill name (e.g., "Remove {skill} filter") so screen-readers announce which filter will be removed when the button is activated.platforms/profile-editor/client/src/routes/(protected)/discover/+page.svelte-14-18 (1)
14-18:⚠️ Potential issue | 🟠 MajorBlank searches keep showing stale results.
Line 15 exits before the discovery store is updated, but the render branch at Lines 103-110 still prefers
$searchResults.lengthoverquery.trim(). After one successful search, clearing the input and pressing Search leaves the old cards on screen instead of returning to the empty state. Either clear the search state for blank queries or let blank/skill-only searches fetch a fresh result set.Also applies to: 103-110, 131-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/routes/`(protected)/discover/+page.svelte around lines 14 - 18, handleSearch currently bails out when query.trim() is empty which leaves prior results in the UI because the render logic checks $searchResults.length vs query.trim(); modify handleSearch to either clear the discovery/search state or trigger a fresh search for blank queries: explicitly reset searchResults (and currentPage) when query.trim() === "" OR call searchProfiles(query, { skills: activeSkills, page: 1, limit: 12 }) for empty query so skill-only searches fetch fresh results; update the same logic paths used by the renderer (references: handleSearch, query, activeSkills, currentPage, searchProfiles, searchResults) so clearing the input shows the empty state instead of stale cards.platforms/dreamsync/client/client/src/components/professional-profile-editor.tsx-272-280 (1)
272-280:⚠️ Potential issue | 🟠 MajorAdd accessible names to the remove buttons.
All three remove controls are icon-only. Screen readers currently just get “button”, which makes it unclear whether the action removes a skill, a work-experience row, or an education row. Please add contextual
aria-labels for each button.Also applies to: 303-311, 415-420
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/dreamsync/client/client/src/components/professional-profile-editor.tsx` around lines 272 - 280, The icon-only remove buttons in the ProfessionalProfileEditor component lack accessible names; update each icon-only <button> (e.g., the one invoking removeSkill and the analogous remove buttons for work-experience and education) to include descriptive aria-label attributes like "Remove skill: {skill.name}", "Remove work experience", and "Remove education" so screen readers get contextual actions; ensure labels are set where the onClick handlers are attached (removeSkill and the corresponding remove handlers) and include the item name when available for clearer context.platforms/profile-editor/client/src/routes/(protected)/discover/+page.svelte-12-23 (1)
12-23:⚠️ Potential issue | 🟠 MajorAvoid a second pagination source of truth.
currentPagecan drift from$searchPagebecause the discovery store survives route navigation while this local state resets to1. That gives you cases where the page label/buttons describe page 1 while the results are still page N, and the next click requests the wrong page. Render directly from$searchPagehere.Also applies to: 118-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/routes/`(protected)/discover/+page.svelte around lines 12 - 23, The local variable currentPage should be removed to avoid a second pagination source of truth; instead read and render directly from the store $searchPage and update the store when navigating. In practice: delete currentPage and stop assigning it in handleSearch and goToPage, change handleSearch to call searchProfiles(query, { skills: activeSkills, page: $searchPage || 1, limit: 12 }) (or set the store to 1 then call search) and change goToPage to set the searchPage store to the target page (or call whatever setter is used by the discovery store) before calling searchProfiles(query, { skills: activeSkills, page, limit: 12 }); update any UI bindings to reference $searchPage instead of currentPage and ensure handleSearch/goToPage reference query, activeSkills, searchProfiles and the searchPage store consistently.platforms/profile-editor/client/src/lib/components/profile/ExperienceSection.svelte-121-128 (1)
121-128:⚠️ Potential issue | 🟠 MajorLabel the edit and delete action buttons.
Both controls are icon-only right now, so assistive tech can't tell “edit this role” from “remove this role”. Add contextual
aria-labels, ideally including the role or company name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/lib/components/profile/ExperienceSection.svelte` around lines 121 - 128, The edit and delete Buttons (the Button components that call openEdit(i) and remove(i)) are icon-only and lack accessible labels; update those Button elements to include descriptive aria-label attributes (e.g., aria-label={`Edit ${experience.title || experience.role || experience.company}`} and aria-label={`Remove ${experience.title || experience.role || experience.company}`} or similar based on the loop item variable name) so assistive tech can announce which role/company is being edited or removed; ensure you use the same item identifier used in the surrounding loop (the variable passed as index i and the experience object) and keep labels concise.platforms/profile-editor/api/src/index.ts-46-47 (1)
46-47:⚠️ Potential issue | 🟠 MajorRun the public profile route through
authMiddlewaretoo.The comment says this endpoint should populate
req.userwhen the caller is logged in, but Line 47 is the only public-profile route that skipsauthMiddleware. That meansgetPublicProfilecan never tell “anonymous viewer” from “owner viewing their own private profile” on this path.🛠️ Suggested fix
-app.get("/api/profiles/:ename", profileController.getPublicProfile); +app.get("/api/profiles/:ename", authMiddleware, profileController.getPublicProfile);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/index.ts` around lines 46 - 47, The public profile route currently registers app.get("/api/profiles/:ename", profileController.getPublicProfile) without running authMiddleware, so req.user is never populated; update the route to include authMiddleware before profileController.getPublicProfile (i.e., app.get("/api/profiles/:ename", authMiddleware, profileController.getPublicProfile)) so the controller can distinguish anonymous viewers from owners via req.user while preserving public access.platforms/profile-editor/client/src/lib/utils/axios.ts-12-15 (1)
12-15:⚠️ Potential issue | 🟠 MajorMissing SSR guard in
setAuthToken.
getAuthTokencheckstypeof window === 'undefined', butsetAuthTokendirectly callslocalStorage.setItemwithout a guard. This will throw during SSR.🛡️ Proposed fix
export function setAuthToken(token: string): void { + if (typeof window === 'undefined') return; localStorage.setItem(TOKEN_KEY, token); apiClient.defaults.headers.common['Authorization'] = `Bearer ${token}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/lib/utils/axios.ts` around lines 12 - 15, The setAuthToken function currently calls localStorage.setItem unconditionally which breaks SSR; update setAuthToken(token: string) so it guards access to localStorage by checking typeof window !== 'undefined' before calling localStorage.setItem(TOKEN_KEY, token), while still setting apiClient.defaults.headers.common['Authorization'] = `Bearer ${token}` as needed (and consider clearing the header when token is falsy); reference the setAuthToken function and the localStorage.setItem and apiClient.defaults.headers.common['Authorization'] usages to locate the change.platforms/profile-editor/client/src/lib/utils/axios.ts-17-20 (1)
17-20:⚠️ Potential issue | 🟠 MajorMissing SSR guard in
removeAuthToken.Same issue as
setAuthToken— will throw during SSR when accessinglocalStorage.🛡️ Proposed fix
export function removeAuthToken(): void { + if (typeof window === 'undefined') return; localStorage.removeItem(TOKEN_KEY); delete apiClient.defaults.headers.common['Authorization']; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/lib/utils/axios.ts` around lines 17 - 20, removeAuthToken accesses localStorage during SSR which will throw; update the function (the removeAuthToken implementation) to guard access to localStorage by checking for a browser environment (e.g. typeof window !== 'undefined' and window.localStorage) before calling localStorage.removeItem(TOKEN_KEY), while still removing the Authorization header from apiClient.defaults.headers.common['Authorization'] as before; reference the removeAuthToken function and TOKEN_KEY and apiClient.defaults.headers.common['Authorization'] when making this change.platforms/profile-editor/client/src/lib/utils/axios.ts-27-33 (1)
27-33:⚠️ Potential issue | 🟠 MajorMissing SSR guards in
setAuthIdandremoveAuthId.Both functions access
localStoragewithout checking for browser environment.🛡️ Proposed fix
export function setAuthId(id: string): void { + if (typeof window === 'undefined') return; localStorage.setItem(ID_KEY, id); } export function removeAuthId(): void { + if (typeof window === 'undefined') return; localStorage.removeItem(ID_KEY); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/lib/utils/axios.ts` around lines 27 - 33, setAuthId and removeAuthId use localStorage directly and will break in SSR; add a guard that checks for browser environment (e.g. typeof window !== 'undefined' and window.localStorage existence) and return early if not available, then perform localStorage.setItem/removeItem inside that guarded block so these functions are no-op on the server; update both setAuthId and removeAuthId accordingly.platforms/profile-editor/api/src/services/EVaultSyncService.ts-17-19 (1)
17-19:⚠️ Potential issue | 🟠 MajorInconsistent error handling for missing environment variable.
Other services in this PR throw an error when
PUBLIC_REGISTRY_URLis missing, but this service silently falls back tohttp://localhost:4321. In production, this could cause the sync to target a non-existent local service without obvious errors.🛡️ Suggested fix for consistency
private get registryUrl(): string { - return process.env.PUBLIC_REGISTRY_URL || "http://localhost:4321"; + const url = process.env.PUBLIC_REGISTRY_URL; + if (!url) throw new Error("PUBLIC_REGISTRY_URL not configured"); + return url; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/services/EVaultSyncService.ts` around lines 17 - 19, The registryUrl getter in EVaultSyncService currently returns a permissive default ("http://localhost:4321") when PUBLIC_REGISTRY_URL is not set; change it to mirror other services by throwing a clear error if process.env.PUBLIC_REGISTRY_URL is missing. Locate the private get registryUrl() accessor in EVaultSyncService and replace the fallback behavior with a thrown Error (including a message that PUBLIC_REGISTRY_URL is required) so callers fail fast instead of silently using localhost.platforms/profile-editor/api/src/controllers/AuthController.ts-11-11 (1)
11-11:⚠️ Potential issue | 🟠 MajorAvoid non-null assertion for required environment variable.
Using
!causes a silentundefinedvalue ifPROFILE_EDITOR_JWT_SECRETis missing, leading to cryptic JWT signing failures. Validate the variable explicitly with a clear error message.🛡️ Suggested fix
-const JWT_SECRET = process.env.PROFILE_EDITOR_JWT_SECRET!; +const JWT_SECRET = process.env.PROFILE_EDITOR_JWT_SECRET; +if (!JWT_SECRET) { + throw new Error("PROFILE_EDITOR_JWT_SECRET environment variable is required"); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/controllers/AuthController.ts` at line 11, Replace the non-null assertion on JWT secret with an explicit validation: read process.env.PROFILE_EDITOR_JWT_SECRET into a variable (referenced as JWT_SECRET) and if it's missing or empty throw an error with a clear message (e.g. "PROFILE_EDITOR_JWT_SECRET must be set") during module initialization so any use of JWT signing (in AuthController) fails fast and with an actionable log; update any tests/initialization that expect the value accordingly.platforms/profile-editor/api/src/services/RegistryService.ts-47-53 (1)
47-53:⚠️ Potential issue | 🟠 MajorMissing normalization of eNames before Registry resolution.
The dreamsync version of
RegistryServiceincludes anormalizeW3id(eName: string)method that prefixes eNames with@if not already present, and calls it before sending the w3id parameter to the Registry/resolveendpoint. The profile-editor version passeseNamedirectly without normalization. Since eNames should be W3IDs with the@prefix and the Registry stores entries with this prefix, this could cause resolution failures if eNames are passed without the prefix.Add the
normalizeW3idmethod and call it inresolveEName()before making the API request, matching the dreamsync implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/services/RegistryService.ts` around lines 47 - 53, Add a normalizeW3id helper to RegistryService that ensures an eName is prefixed with "@" when missing (match the dreamsync behavior), and call it at the start of resolveEName(eName: string) to normalize the value before using this.registryUrl/resolve; update resolveEName to send { params: { w3id: normalized } } and return the same { uri, evault } response shape so Registry resolution succeeds for names both with and without the "@" prefix.platforms/profile-editor/api/src/services/EVaultProfileService.ts-9-10 (1)
9-10:⚠️ Potential issue | 🟠 MajorUse the real user ontology ID here.
The sibling service in this PR resolves the user envelope with
550e8400-e29b-41d4-a716-446655440000, not"User". With this constant,getProfile()will miss the user envelope and drophandle/isVerifieddata.Minimal fix
-const USER_ONTOLOGY = "User"; +const USER_ONTOLOGY = "550e8400-e29b-41d4-a716-446655440000";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts` around lines 9 - 10, USER_ONTOLOGY is set to the literal "User" which causes getProfile() in EVaultProfileService to miss the user envelope; change the USER_ONTOLOGY constant to the actual ontology ID used elsewhere ("550e8400-e29b-41d4-a716-446655440000") so getProfile()/resolve logic picks up handle and isVerified fields (update the USER_ONTOLOGY constant in EVaultProfileService and any related references).platforms/dreamsync/api/src/services/AIMatchingService.ts-63-77 (1)
63-77:⚠️ Potential issue | 🟠 MajorHonor
isPublicbefore enriching DreamSync matches.This gate only checks
isDreamsyncVisible. The profile editor added in this PR only lets users toggleisPublic, so a profile marked private still gets injected into matching. Filter on both flags, or go through a public-profile read path.Minimal fix
- if (prof.isDreamsyncVisible !== false) { + if (prof.isPublic !== false && prof.isDreamsyncVisible !== false) { profileCache.set(ename, prof); } else { profileCache.set(ename, null); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/dreamsync/api/src/services/AIMatchingService.ts` around lines 63 - 77, The code in AIMatchingService (inside the Promise.all block using evaultProfileService.getProfessionalProfile) only checks prof.isDreamsyncVisible but must also honor prof.isPublic; update the conditional that sets profileCache (and the fallback null branch) to only cache the profile when both prof.isDreamsyncVisible !== false and prof.isPublic === true (or alternatively call the public-profile read path instead of getProfessionalProfile) so private profiles are not injected into DreamSync matching; adjust references around getProfessionalProfile, isDreamsyncVisible, isPublic, and profileCache accordingly.platforms/profile-editor/api/src/controllers/WebhookController.ts-67-80 (1)
67-80:⚠️ Potential issue | 🟠 MajorAllow webhook clears to propagate to the local projection.
These truthy assignments only sync additions. If the source removes
avatarFileId,bannerFileId,location,cvFileId, or similar fields, the old local value survives indefinitely. Use field-presence checks plus??/?? nullso clears and empty-string updates are applied too.Possible fix pattern
- name: rawBody.data?.displayName || localData.name, + name: rawBody.data?.displayName ?? localData.name, - if (localData.avatarFileId) userData.avatarFileId = localData.avatarFileId; + if ("avatarFileId" in localData) { + userData.avatarFileId = localData.avatarFileId ?? null; + } - if (localData.location) profileData.location = localData.location; + if ("location" in localData) { + profileData.location = localData.location ?? null; + }Also applies to: 102-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/controllers/WebhookController.ts` around lines 67 - 80, The current userData assembly in WebhookController.ts only assigns optional fields when they are truthy, so clears or empty-string updates from rawBody/localData never overwrite existing projection values; update the mapping for userData (and the similar block later) to check for field presence (e.g., property in localData or rawBody.data) and assign using nullish coalescing or explicit null (e.g., userData.avatarFileId = localData.hasOwnProperty('avatarFileId') ? localData.avatarFileId ?? null : undefined) so that removals (avatarFileId, bannerFileId, location, cvFileId, etc.) propagate and existing values are cleared when intended.platforms/profile-editor/api/src/controllers/WebhookController.ts-52-54 (1)
52-54:⚠️ Potential issue | 🟠 MajorDon't acknowledge failed webhooks as success.
The catch path still returns
200, which prevents the sender from retrying transient failures and silently drops sync events.Minimal fix
- } catch (e) { - console.error("Webhook error:", e); - res.status(200).send(); - } + } catch (e) { + console.error("Webhook error:", e); + res.status(500).send("Webhook processing failed"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/controllers/WebhookController.ts` around lines 52 - 54, The catch block in WebhookController.ts currently logs the error but still responds with 200, preventing sender retries; update the catch handler in the webhook processing method (the try/catch around the webhook handling logic in WebhookController) to return an error status (e.g., res.status(500).send() or another 5xx) instead of 200, and ensure the error details remain logged (console.error or processLogger) so transient failures are retriable by the sender.platforms/profile-editor/client/src/lib/stores/profile.ts-62-76 (1)
62-76:⚠️ Potential issue | 🟠 MajorClear stale
profiledata on cross-user fetches and failures.
fetchProfile()leaves the previous value in the store until the request succeeds. If the user changes or the new request fails, the UI keeps rendering the old profile, which can expose or edit the wrong person's data.One way to avoid stale cross-user state
export async function fetchProfile(): Promise<ProfileData> { + const requestedEname = get(currentUser)?.ename; + if (get(profile)?.ename !== requestedEname) { + profile.set(null); + } profileLoading.set(true); try { const response = await apiClient.get('/api/profile'); const data = response.data; profile.set(data); @@ if (user?.ename === data.ename && data.name) { currentUser.update((u) => (u ? { ...u, name: data.name } : u)); } return data; + } catch (error) { + if (get(profile)?.ename !== requestedEname) { + profile.set(null); + } + throw error; } finally { profileLoading.set(false); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/lib/stores/profile.ts` around lines 62 - 76, fetchProfile currently leaves the previous profile in the store until the GET succeeds, causing stale cross-user data to flash; modify fetchProfile to clear the profile store at start (e.g., profile.set(null) or undefined) so the UI doesn't show the old profile while loading, and also ensure the profile is cleared on failure inside a catch block before rethrowing/returning; keep the existing header-sync logic (currentUser.update(...)) but run it only after a successful fetch and after verifying ename matches the returned data.
platforms/profile-editor/api/src/services/EVaultProfileService.ts
Outdated
Show resolved
Hide resolved
platforms/profile-editor/api/src/web3adapter/mappings/user.mapping.json
Outdated
Show resolved
Hide resolved
platforms/profile-editor/client/src/lib/components/profile/SocialLinksSection.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
platforms/profile-editor/client/package.json (1)
2-2: Consider using a scoped package name for consistency.The package name
profile-editordoesn't follow the scoped naming convention used by other internal packages (e.g.,@metastate-foundation/ui). For consistency within the monorepo, consider renaming to@metastate-foundation/profile-editor-clientor similar.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/package.json` at line 2, The package name in package.json is currently "profile-editor"; change it to a scoped name consistent with the monorepo (for example "@metastate-foundation/profile-editor-client") by updating the "name" field in platforms/profile-editor/client/package.json and then update any workspace references, build scripts, and internal imports that reference "profile-editor" (e.g., workspaces, package.json dependencies, and any CI/config entries) so they use the new scoped identifier.packages/ui/package.json (1)
87-91: Narrow the published source surface.Because
./stylesat Lines 13-15 points tosrc/app.css,"src"here ships the whole source tree just to expose one stylesheet. If that's not intentional, publish only that CSS file or add matching!src/**/*.test.*/!src/**/*.spec.*negations.One minimal option
"files": [ "dist", - "src", + "src/app.css", "!dist/**/*.test.*" ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/package.json` around lines 87 - 91, The package.json "files" array currently publishes "src" (alongside "dist"), which exposes the entire source tree just to surface the stylesheet referenced by "styles" (src/app.css); update the "files" array to either replace "src" with the specific stylesheet path(s) (e.g., "src/app.css") or keep "src" but add negations to exclude test/spec files (e.g., add "!src/**/*.test.*" and/or "!src/**/*.spec.*") so only the intended source artifacts are published; modify the entry in package.json that defines the "files" array accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/package.json`:
- Around line 111-137: The package.json currently lists runtime packages as
devDependencies causing consumer breakage; move "svelte", "@lucide/svelte",
"tailwind-merge", and "tailwind-variants" from the devDependencies section into
the dependencies section so they are installed for consumers; update the
dependencies object in packages/ui/package.json to include those four entries
(keeping the same semver versions) and remove them from devDependencies, then
run a fresh install to verify no missing-module errors in exported components
that import Svelte, Lucide icons, tailwind-merge, or tailwind-variants.
- Line 99: Update the package.json "prepare" script to stop swallowing failures:
remove the "|| echo ''" fallback so the command reads "svelte-kit sync &&
svelte-package" (keeping the existing "svelte-kit sync" and "svelte-package"
sequence) to ensure any errors from svelte-package surface and cause the install
to fail; do not change other scripts.
In `@platforms/dreamsync/api/src/services/EVaultProfileService.ts`:
- Line 190: The ACL entry for private profiles uses raw eName which doesn't
match the normalized JWT-derived owner id; update the branch that sets const acl
= merged.isPublic !== false ? ["*"] : [eName] to normalize the private entry by
calling this.registryService.normalizeW3id(eName) so the ACL becomes ["*"] for
public or [this.registryService.normalizeW3id(eName)] for private; ensure you
reference merged, eName, acl, and this.registryService.normalizeW3id in the
change so owner lookup comparisons succeed.
In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts`:
- Around line 164-165: The code currently treats a missing isPublic as true;
change the logic in EVaultProfileService where profile objects are constructed
(the isPublic assignments at the shown locations and the other occurrences
around the file) to only consider the profile public when profData.isPublic ===
true (not via nullish coalescing to true), and update the ACL write path that
sets acl: ["*"] to only do so when the computed isPublic is true so
missing/undefined isPublic defaults to private; adjust all instances referenced
(the isPublic at the shown block and the other occurrences around lines 173-178
and 192-197) to follow this explicit-true rule.
- Around line 118-130: The upsert flow around findMetaEnvelopeByOntology and
createMetaEnvelope can double-create envelopes under concurrent saves; ensure
uniqueness by using a backend upsert/unique constraint for MetaEnvelope
(preferred) or, if backend changes are not possible, change the client flow in
EVaultProfileService: when creating an envelope (createMetaEnvelope) detect
duplicate-create conflicts/errors, then re-query via findMetaEnvelopeByOntology
and perform an update instead (or retry the create-to-update path) to guarantee
idempotence; apply the same pattern to the similar flow around the create/update
logic referenced in the other block (the create/update sequence around the
181–223 logic).
- Line 197: The ACL entry for private profiles uses the raw eName; update the
assignment that creates acl (the line using merged and eName) to normalize eName
before storing so it matches BindingDocumentService's format; use the same
normalization logic (the normalizeSubject pattern/function used in
BindingDocumentService) to prefix or transform eName consistently, so acl
becomes either ["*"] or [normalized(eName)] and ensures owner principal matching
works for private profiles.
In `@platforms/profile-editor/client/package.json`:
- Around line 19-25: The listed runtime packages (`@lucide/svelte`,
tw-animate-css, tailwind-merge, clsx) are currently in devDependencies but must
be actual dependencies; open package.json, remove those four entries from
devDependencies and add them under dependencies with the same version strings
(ensuring "svelte" and "tailwindcss" remain where appropriate), then run
npm/yarn install to update the lockfile so SSR and non-bundled consumers will
install them at runtime.
- Around line 33-34: Remove the unused dependency "svelte-qrcode" from
package.json to avoid bundling an unreferenced package; edit the dependencies
list to delete the line containing "svelte-qrcode": "^1.0.1" while leaving
"svelte-qrcode-action": "^1.0.2" intact, then run dependency install (e.g.,
npm/yarn install) and verify no imports/reference to svelte-qrcode exist; update
lockfile accordingly.
In
`@platforms/profile-editor/client/src/lib/components/profile/SocialLinksSection.svelte`:
- Around line 103-109: In SocialLinksSection.svelte, re-validate each persisted
link at render time instead of trusting normalizeAndValidateUrl() which only
runs on new form input: for each item in links (used to render the <a>), call
the existing normalizeAndValidateUrl() or a dedicated validation helper before
binding link.url to href; if validation fails, do not set href (render as plain
text or omit the anchor) or skip the entry entirely to avoid unsafe schemes
becoming clickable—update the rendering logic that currently uses link.url
directly so only validated URLs are bound to the anchor.
- Around line 19-32: normalizeAndValidateUrl currently prepends "https://" to
any scheme-less input, so bare emails like "alice@example.com" become
"https://alice@example.com/" despite mailto: being allowed; update
normalizeAndValidateUrl to detect bare email addresses (e.g., using a simple
/^[^\s@]+@[^\s@]+\.[^\s@]+$/ test) and if matched, prepend "mailto:" before
parsing, then continue to validate against ALLOWED_PROTOCOLS (referencing
normalizeAndValidateUrl and ALLOWED_PROTOCOLS) so emails are normalized to
mailto: URIs instead of https: ones.
---
Nitpick comments:
In `@packages/ui/package.json`:
- Around line 87-91: The package.json "files" array currently publishes "src"
(alongside "dist"), which exposes the entire source tree just to surface the
stylesheet referenced by "styles" (src/app.css); update the "files" array to
either replace "src" with the specific stylesheet path(s) (e.g., "src/app.css")
or keep "src" but add negations to exclude test/spec files (e.g., add
"!src/**/*.test.*" and/or "!src/**/*.spec.*") so only the intended source
artifacts are published; modify the entry in package.json that defines the
"files" array accordingly.
In `@platforms/profile-editor/client/package.json`:
- Line 2: The package name in package.json is currently "profile-editor"; change
it to a scoped name consistent with the monorepo (for example
"@metastate-foundation/profile-editor-client") by updating the "name" field in
platforms/profile-editor/client/package.json and then update any workspace
references, build scripts, and internal imports that reference "profile-editor"
(e.g., workspaces, package.json dependencies, and any CI/config entries) so they
use the new scoped identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 837b1617-78cc-4084-b71d-28395be203ab
⛔ Files ignored due to path filters (2)
platforms/profile-editor/client/static/favicon.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.npmrcdocs/docusaurus.config.tspackages/ui/package.jsonplatforms/dreamsync/api/src/services/EVaultProfileService.tsplatforms/profile-editor/api/src/services/EVaultProfileService.tsplatforms/profile-editor/api/src/web3adapter/mappings/professional-profile.mapping.jsonplatforms/profile-editor/api/src/web3adapter/mappings/user.mapping.jsonplatforms/profile-editor/client/package.jsonplatforms/profile-editor/client/src/lib/components/profile/SocialLinksSection.svelteplatforms/profile-editor/client/src/lib/utils/file-manager.tsplatforms/profile-editor/client/src/routes/(protected)/profile/[ename]/+page.svelteplatforms/profile-editor/client/svelte.config.js
💤 Files with no reviewable changes (1)
- .npmrc
🚧 Files skipped from review as they are similar to previous changes (4)
- platforms/profile-editor/client/svelte.config.js
- platforms/profile-editor/api/src/web3adapter/mappings/professional-profile.mapping.json
- platforms/profile-editor/api/src/web3adapter/mappings/user.mapping.json
- platforms/profile-editor/client/src/lib/utils/file-manager.ts
platforms/profile-editor/api/src/services/EVaultProfileService.ts
Outdated
Show resolved
Hide resolved
platforms/profile-editor/api/src/services/EVaultProfileService.ts
Outdated
Show resolved
Hide resolved
platforms/profile-editor/client/src/lib/components/profile/SocialLinksSection.svelte
Outdated
Show resolved
Hide resolved
platforms/profile-editor/client/src/lib/components/profile/SocialLinksSection.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
platforms/profile-editor/client/src/lib/utils/file-manager.ts (1)
13-20:⚠️ Potential issue | 🔴 CriticalDrop the explicit multipart
Content-Typeheader.Axios’s browser examples post
FormDatawithout overriding this header, and MDN explicitly warns that manually settingmultipart/form-dataprevents the browser from adding the requiredboundary. That can make/api/filesuploads fail to parse server-side. (axios-http.com)🐛 Proposed fix
const response = await apiClient.post('/api/files', formData, { - headers: { 'Content-Type': 'multipart/form-data' }, onUploadProgress: (e) => { if (onProgress && e.total) { onProgress(Math.round((e.loaded * 100) / e.total)); } } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/lib/utils/file-manager.ts` around lines 13 - 20, The API upload call in file-manager.ts currently sets a hardcoded 'Content-Type': 'multipart/form-data' when calling apiClient.post('/api/files', formData, { ... }), which prevents the browser from adding the required boundary; remove the explicit Content-Type header from the options passed to apiClient.post (i.e., delete the headers object or at least the 'Content-Type' key) and keep the onUploadProgress handler as-is so the browser can set the correct multipart boundary automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@platforms/profile-editor/client/src/lib/utils/file-manager.ts`:
- Around line 13-20: The API upload call in file-manager.ts currently sets a
hardcoded 'Content-Type': 'multipart/form-data' when calling
apiClient.post('/api/files', formData, { ... }), which prevents the browser from
adding the required boundary; remove the explicit Content-Type header from the
options passed to apiClient.post (i.e., delete the headers object or at least
the 'Content-Type' key) and keep the onUploadProgress handler as-is so the
browser can set the correct multipart boundary automatically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78cbcedf-d875-49c4-8876-b55b0b57c851
📒 Files selected for processing (1)
platforms/profile-editor/client/src/lib/utils/file-manager.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts`:
- Around line 178-184: getPublicProfile currently calls getProfile(eName) which
loads the full owner-scoped professional payload before checking
professional.isPublic; change it to perform a lightweight public-check first
(e.g., call a new or existing method that reads only visibility like
getProfessionalVisibility or a non-owner read path) and only if
professional.isPublic is true call getProfile(eName) to load the full profile.
Update getPublicProfile to first query the visibility (without owner/X-ENAME
context), check professional.isPublic, and then load the full profile via
getProfile(eName) only when allowed.
- Around line 230-255: The current create fallback unconditionally treats any
createMetaEnvelope.errors as a TOCTOU conflict and re-queries/updates, which can
hide real validation/auth errors; modify the logic in the create flow around
result.createMetaEnvelope (the branch that calls findMetaEnvelopeByOntology and
then UPDATE_MUTATION) to first inspect result.createMetaEnvelope.errors for a
specific conflict code (e.g., "ONTOLOGY_ALREADY_EXISTS") and only perform the
re-query+update when that code is present; for other error codes, surface/throw
the original error messages from result.createMetaEnvelope.errors instead of
attempting the update. Ensure this check is applied before calling
findMetaEnvelopeByOntology/update and preserve the existing
updateResult.updateMetaEnvelope error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6be5300c-ac07-4091-b6a4-99a1020ee1c9
📒 Files selected for processing (5)
packages/ui/package.jsonplatforms/dreamsync/api/src/services/EVaultProfileService.tsplatforms/profile-editor/api/src/services/EVaultProfileService.tsplatforms/profile-editor/client/package.jsonplatforms/profile-editor/client/src/lib/components/profile/SocialLinksSection.svelte
🚧 Files skipped from review as they are similar to previous changes (4)
- platforms/dreamsync/api/src/services/EVaultProfileService.ts
- platforms/profile-editor/client/src/lib/components/profile/SocialLinksSection.svelte
- platforms/profile-editor/client/package.json
- packages/ui/package.json
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
platforms/profile-editor/api/src/services/EVaultProfileService.ts (2)
178-183:⚠️ Potential issue | 🟠 MajorDon't load the private profile before the public visibility check.
Line 179 still calls
getProfile(eName), which uses the target user'sX-ENAMEcontext and fetches the full professional payload before Line 180 rejects private profiles. The public path needs a visibility-only/public read first, then the full load only whenisPublic === true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts` around lines 178 - 183, getPublicProfile currently calls getProfile(eName) (which loads the full professional payload) before checking professional.isPublic; change it so you first perform a lightweight visibility-only read for the target's professional.public flag (e.g., a new or existing method like fetchProfessionalVisibility/fetchPublicFlag) that does not use the target user's X-ENAME context or load private fields, and only if that check returns true call getProfile(eName) to load the full profile; if the visibility check is false return null immediately. Ensure you reference getPublicProfile and getProfile when implementing the visibility-only call and preserve existing behavior when isPublic === true.
230-267:⚠️ Potential issue | 🟠 Major
CREATE_FAILEDis still too broad for the retry-as-update path.Line 233 still treats generic
CREATE_FAILEDas a duplicate-create signal. If the resolver uses that code for validation/auth failures too, this branch can re-query and overwrite an existing envelope after an unrelated create error. Only retry on a dedicated conflict code from the backend; otherwise surface the original create errors.#!/bin/bash set -euo pipefail echo "=== createMetaEnvelope resolver and emitted error codes ===" rg -n -C3 'createMetaEnvelope|CREATE_FAILED|ONTOLOGY_ALREADY_EXISTS|code:' infrastructure/evault-core/src/core/protocol/graphql-server.ts echo echo "=== Any ontology-specific conflict codes elsewhere ===" rg -n -C2 'ONTOLOGY_ALREADY_EXISTS|ALREADY_EXISTS|CONFLICT' infrastructure/evault-core/src --type ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts` around lines 230 - 267, The code treats the generic "CREATE_FAILED" as a conflict and may wrongly retry-as-update; change the conflict detection in the createMetaEnvelope error handling (the block around result.createMetaEnvelope.errors in EVaultProfileService.ts) to only consider the explicit duplicate/conflict code emitted by the backend (e.g. "ONTOLOGY_ALREADY_EXISTS" or another dedicated conflict code) as a retryable condition, and do not treat "CREATE_FAILED" as a trigger for re-query-and-update; if the errors do not include the explicit conflict code, immediately surface the original errors instead of performing the TOCTOU re-query/update path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts`:
- Around line 112-120: getClient currently sends the raw eName in the "X-ENAME"
header which can mismatch ACL entries that are stored using normalizeEName;
update getClient to call normalizeEName(eName) and use that normalized value for
the "X-ENAME" header when constructing the GraphQLClient (keep Authorization the
same). Reference the existing normalizeEName function used elsewhere in this
class to ensure the header matches stored ACL principals.
---
Duplicate comments:
In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts`:
- Around line 178-183: getPublicProfile currently calls getProfile(eName) (which
loads the full professional payload) before checking professional.isPublic;
change it so you first perform a lightweight visibility-only read for the
target's professional.public flag (e.g., a new or existing method like
fetchProfessionalVisibility/fetchPublicFlag) that does not use the target user's
X-ENAME context or load private fields, and only if that check returns true call
getProfile(eName) to load the full profile; if the visibility check is false
return null immediately. Ensure you reference getPublicProfile and getProfile
when implementing the visibility-only call and preserve existing behavior when
isPublic === true.
- Around line 230-267: The code treats the generic "CREATE_FAILED" as a conflict
and may wrongly retry-as-update; change the conflict detection in the
createMetaEnvelope error handling (the block around
result.createMetaEnvelope.errors in EVaultProfileService.ts) to only consider
the explicit duplicate/conflict code emitted by the backend (e.g.
"ONTOLOGY_ALREADY_EXISTS" or another dedicated conflict code) as a retryable
condition, and do not treat "CREATE_FAILED" as a trigger for
re-query-and-update; if the errors do not include the explicit conflict code,
immediately surface the original errors instead of performing the TOCTOU
re-query/update path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 556a5cc0-9ddf-435e-9c90-aed38d4051bf
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
platforms/profile-editor/api/src/services/EVaultProfileService.ts
2 participants
Description of change
adds a professional profile editor
Issue Number
closes #875
closes #876
Type of change
How the change has been tested
manual
Change checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores