Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 29 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughIntroduces branded/domain validators (usernames, campaign slugs, sidebar item names/slugs, colors, icons), refactors sidebar validation into focused modules (access, name, slug, color, icon, parent, move, orchestration), updates Convex backend call sites/types, and tightens frontend types and input normalization across many components and tests. Changes
Sequence Diagram(s)(omitted — changes are broad and distributed across many modules without a single new multi-component sequential flow suitable for a compact diagram) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
wizard-archive | 430d262 | Commit Preview URL Branch Preview URL |
Apr 17 2026, 09:52 PM |
Preview Deployment
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 38
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/features/dnd/hooks/useFileDropHandler.tsx (1)
205-205: 🧹 Nitpick | 🔵 TrivialDrop the now-redundant
Id<'sidebarItems'>cast.
createItemalready returnsid: Id<'sidebarItems'>(as reflected byUploadSingleFileResult.idabove), so theas Id<'sidebarItems'>assertion here is noise and hides any future type drift increateItem's return shape.♻️ Proposed cleanup
- const folderId = result.id as Id<'sidebarItems'> + const folderId = result.id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/dnd/hooks/useFileDropHandler.tsx` at line 205, The explicit cast on result.id is redundant and should be removed: stop using "as Id<'sidebarItems'>" when assigning folderId (const folderId = result.id). Rely on the existing return type from createItem / UploadSingleFileResult to infer the correct Id type; if TypeScript complains, adjust the variable's declared type (or the createItem return type) rather than adding an inline assertion. Update references to folderId accordingly and remove the cast to avoid hiding future type drift.src/features/campaigns/components/campaign-dialog.tsx (2)
189-197:⚠️ Potential issue | 🟡 MinorAggressive
slugifyon every keystroke can break typing UX.Running
slugifyinsideonChangenormalizes the input on every character. Depending onslugify's rules, this can make certain intermediate states unreachable — e.g., if it strips leading/trailing separators or collapses repeated hyphens, users typingmy-cool-will see the trailing-vanish immediately and be unable to continue withmy-cool-campaignnaturally. It also fights with caret position, since replacing the input value on every keystroke can jump the cursor to the end.Consider normalizing more leniently while typing (e.g., lowercasing + replacing spaces only) and applying full
slugifyon blur/submit, which matches how the submission path at lines 83/93 already re-normalizes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/campaigns/components/campaign-dialog.tsx` around lines 189 - 197, The current onChange uses slugify(...) which aggressively normalizes on every keystroke (Input onChange -> slugify) and breaks typing/caret; change the onChange to apply a lightweight transformation (e.g., lowercasing and replacing spaces with single hyphens) before calling field.handleChange, and move the full slugify(...) call to the onBlur handler (Input onBlur -> call full slugify then field.handleBlur) and ensure the submit path already calling slugify remains as-is (so final normalization happens on blur/submit); reference the Input component's onChange/onBlur handlers, slugify, and field.handleChange/field.handleBlur when making this change.
62-76:⚠️ Potential issue | 🟡 MinorSeed
initialSlugwithslugify()to ensure min-length compliance.
Math.random().toString(36).substring(2, 15)produces variable-length strings that can fall below theCAMPAIGN_SLUG_MIN_LENGTHof 3 characters. Since the form validates on mount (line 169), it may open with a validation error. Applyslugify()to normalize and ensure the initial value meets validation rules:- const [initialSlug] = useState(() => Math.random().toString(36).substring(2, 15)) + const [initialSlug] = useState(() => slugify(Math.random().toString(36).substring(2, 15)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/campaigns/components/campaign-dialog.tsx` around lines 62 - 76, initialSlug is generated with Math.random().toString(36)... which can produce strings shorter than CAMPAIGN_SLUG_MIN_LENGTH and triggers validation on mount; replace that generation by passing the random string through slugify and ensure it meets CAMPAIGN_SLUG_MIN_LENGTH (e.g., generate slug = slugify(random), then if slug.length < CAMPAIGN_SLUG_MIN_LENGTH pad or regenerate/append characters until length >= CAMPAIGN_SLUG_MIN_LENGTH) before passing it into the useState initializer used by form (initialSlug and the useForm default slug).convex/_test/factories.helper.ts (1)
158-189: 🧹 Nitpick | 🔵 TrivialMinor: duplicated slug computation/validation in
insertSidebarItem.
sidebarItemBasealready runsassertSidebarItemSlug(slugify(name))at line 177;insertSidebarItemthen recomputes and re-asserts the same slug at line 223 before overwriting the base slug at line 229. When nooverrides.slugis passed,slugify(name)+assertSidebarItemSlugruns twice.Consider moving slug resolution out of
sidebarItemBase(or accepting an optionalslugarg there) so it's validated exactly once:♻️ Suggested refactor
const sidebarItemBase = ( campaignId: Id<'campaigns'>, creatorProfileId: Id<'userProfiles'>, name: string, + slug: SidebarItemSlug, ): { ... } & ReturnType<typeof commonFields> => ({ name, - slug: assertSidebarItemSlug(slugify(name)), + slug, ... }) @@ - const { inheritShares, imageStorageId, storageId, slug, ...sidebarOverrides } = overrides ?? {} - const validatedSlug = slug ? assertSidebarItemSlug(slug) : assertSidebarItemSlug(slugify(name)) + const { inheritShares, imageStorageId, storageId, slug, ...sidebarOverrides } = overrides ?? {} + const validatedSlug = assertSidebarItemSlug(slug ?? slugify(name)) const sharedData = { - ...sidebarItemBase(campaignId, creatorProfileId, name), + ...sidebarItemBase(campaignId, creatorProfileId, name, validatedSlug), type, ...sidebarOverrides, - slug: validatedSlug, }Also applies to: 219-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_test/factories.helper.ts` around lines 158 - 189, sidebarItemBase currently runs assertSidebarItemSlug(slugify(name)) and insertSidebarItem recomputes and re-asserts the slug, causing duplicate validation; change sidebarItemBase to accept an optional slug parameter (or remove its slug computation entirely) and have insertSidebarItem compute slug once using slugify(name) + assertSidebarItemSlug, then pass that slug into sidebarItemBase so the slug is validated and set in only one place (update references to sidebarItemBase and insertSidebarItem accordingly).convex/sidebarItems/functions/treeOperations.ts (1)
62-82:⚠️ Potential issue | 🟠 MajorRisk:
assertSidebarItemNameon persisted descendant names can break restore for legacy items.When restoring a folder,
restoreTreeDescendantsappliesassertSidebarItemNamedirectly to DB-sourced names in the descendant loop (line 73). If any descendant has a name that violates the validation rules (length bounds, forbidden characters, control characters, leading/trailing dots, or insufficient alphanumerics), the assertion will throw mid-traversal, leaving the subtree in an inconsistent state and making those items permanently unrestorable through this path.Compare this to the parent item restore in
moveSidebarItem.ts:62–81, which safely pre-processes the name viadeduplicateNamebefore assertion. Descendants lack this protection.Options to de-risk:
- Use
parseSidebarItemName(the non-throwing variant already in the codebase) and fall back to a sanitized value before callingfindUniqueSidebarItemSlug.- Or apply
deduplicateNameto descendant names before validation, matching the pattern used for the parent item.- Or confirm via data audit that no pre-existing rows violate the invariant before relying on the hard assertion here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/sidebarItems/functions/treeOperations.ts` around lines 62 - 82, The descendant restore loop in restoreTreeDescendants calls assertSidebarItemName on DB-sourced names (via applyToTree) which can throw and leave the subtree partially restored; update the loop so it first normalizes/sanitizes names instead of asserting raw DB values—use parseSidebarItemName to parse without throwing and if it returns invalid fall back to a sanitized name (or call deduplicateName as moveSidebarItem does) before passing the result to findUniqueSidebarItemSlug, then use that slug in the ctx.db.patch; ensure you do not call assertSidebarItemName directly on persisted descendant names to avoid throwing mid-traversal.convex/folders/functions/folderHelpers.ts (1)
22-37:⚠️ Potential issue | 🟡 MinorAsymmetric normalization creates lookup misses: line 22 dropped
.trim()but line 35 still trims for comparison.Line 22 changed
normalizedNametoname.toLowerCase()without trimming, while line 35 still compares againstitem.name.trim().toLowerCase(). This breaks the lookup if any caller passes aSidebarItemNamewith leading/trailing whitespace.The issue:
resolveOrCreateFolderPathpasses raw path segments fromparentTarget.pathSegments(validated asv.array(v.string()), notSidebarItemName) directly intoresolveOrCreateChildFolderwith aSidebarItemNametype annotation. TypeScript does not enforce the branded type's validation invariant at runtime, so untrimmed strings like" foo "can reachfindSidebarChildByName. With the missing trim on line 22:
normalizedName = " foo ".toLowerCase()=" foo "- Comparison:
"foo".trim().toLowerCase()="foo"≠" foo "→ lookup miss- Later validation in
ensureSidebarItemNameAvailablemight reject the name, but by then a duplicate folder could have been processed.Either restore
.trim()on line 22 or validatepathSegmentsbefore passing them asSidebarItemName.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/folders/functions/folderHelpers.ts` around lines 22 - 37, The lookup normalizes the caller string asymmetrically (normalizedName = name.toLowerCase() without trim) causing mismatches against item.name.trim().toLowerCase(); fix by trimming when normalizing the incoming name (set normalizedName = name.trim().toLowerCase()) in the function that performs the lookup (the normalizedName variable inside findSidebarChildByName / folderHelpers.ts), or alternatively validate/trim path segments in resolveOrCreateFolderPath/resolveOrCreateChildFolder before passing them as SidebarItemName; prefer restoring .trim() on normalizedName to keep normalization symmetric with item.name.trim().toLowerCase().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vscode/settings.json:
- Line 4: The workspace setting currently disables formatting on save via the
"editor.formatOnSave": false entry; restore automatic formatting for the
workspace by removing that setting or changing it to "editor.formatOnSave": true
(or move this preference to user settings instead) so contributors keep
consistent formatting and avoid review noise.
- Around line 7-8: The workspace settings currently disable save-time autofixes
by setting "source.fixAll.oxc": "never" and "source.fixAll.eslint": "never";
revert these two keys to their defaults (remove them or set to "prompt" or
"true" per your team's preference) so ESLint and OXC can run fixAll on save for
all contributors, or replace with a documented team-wide rationale if disabling
is required; update the .vscode/settings.json to either delete the
"source.fixAll.oxc" and "source.fixAll.eslint" entries or set them to
"true"/"prompt" and commit the change.
In `@convex/_test/factories.helper.ts`:
- Around line 72-87: The current truthiness checks (e.g., the username
conditional that does ...(username ? { username: assertUsername(username) } :
{})) silently treat empty-string overrides as absent; change these to explicit
undefined checks (username !== undefined) so any supplied value is validated,
and call the validator (assertUsername) on the provided value to fail fast on
empty strings; make the analogous change for slug/assertCampaignSlug in the
other block (lines around the slug handling) so explicit empty overrides are
validated rather than ignored.
In `@convex/campaigns/functions/updateCampaign.ts`:
- Around line 32-41: The guard comparing conflict._id to campaign._id is
redundant because the outer check slug !== campaign.slug guarantees the query
by_slug_dm can't return the same campaign; in updateCampaign replace the
conditional "if (conflict && conflict._id !== campaign._id)" with a simple
existence check "if (conflict)" (keeping the same thrown error and surrounding
logic) so that the ctx.db query result (conflict) alone triggers the CONFLICT
error.
In `@convex/campaigns/mutations.ts`:
- Line 55: Change the truthy check on args.slug to an explicit undefined check
so optional presence (not falsiness) is intended: replace the expression
currently using args.slug ? requireCampaignSlug(args.slug) : undefined with a
check for args.slug !== undefined ? requireCampaignSlug(args.slug) : undefined,
referencing the same requireCampaignSlug call and the args.slug property in the
mutation to ensure empty strings or other falsy-but-present values are still
validated.
In `@convex/campaigns/validation.ts`:
- Around line 50-52: prepareCampaignDescription currently only trims and allows
arbitrarily long strings; add a max-length guard similar to
prepareCampaignName/validateCampaignName. Define a
MAX_CAMPAIGN_DESCRIPTION_LENGTH constant (or reuse project convention), trim the
incoming description, return undefined for empty strings, and if trimmed.length
exceeds the max either throw the same validation error type used by
validateCampaignName or call a new validateCampaignDescription to enforce and
surface the violation; ensure function name prepareCampaignDescription and the
new constant/validator are referenced so callers can locate the change.
- Around line 58-60: The function requireCampaignUsername is just a direct
pass-through to requireUsername and should be removed or given campaign-specific
behavior: either delete requireCampaignUsername and update all call sites to
call requireUsername(username) directly, or implement campaign-specific
validation inside requireCampaignUsername (e.g., enforce DM reservation or other
campaign rules) before delegating to requireUsername; look for references to
requireCampaignUsername and requireUsername to update them accordingly.
- Around line 41-48: prepareCampaignName currently trims the input and then
calls validateCampaignName(trimmed), but validateCampaignName trims again; to
remove redundancy call validateCampaignName with the original name
(validateCampaignName(name)) and keep the existing trimmed = name.trim() for the
return and error flow (throwClientError(ERROR_CODE.VALIDATION_FAILED, error));
update prepareCampaignName to use validateCampaignName(name) so trimming happens
only once inside validateCampaignName while still returning the trimmed value.
In `@convex/canvases/mutations.ts`:
- Around line 33-45: The handler in createCanvas is using redundant "??
undefined" coalesces for iconName and color; remove the trailing "?? undefined"
so that requireOptionalSidebarItemIconName(args.iconName) and
requireOptionalSidebarItemColor(args.color) are assigned directly (in the
handler function that calls createCanvasFn) to match updateCanvas's pattern and
preserve the intended optional/undefined typing for iconName and color.
In `@convex/common/async.ts`:
- Around line 3-9: The type guard isPromiseLike currently narrows to Promise<T>
while only checking for a thenable; update the predicate to match the runtime
check by changing its return type to value is PromiseLike<T> (or, if you truly
need a native Promise, extend the runtime check to also verify methods like
catch and/or finally before narrowing to Promise<T>). Locate isPromiseLike and
either change its signature to return value is PromiseLike<T> or add checks for
typeof (value as { catch?: unknown }).catch === 'function' (and finally if
desired) so the runtime test matches the narrowed type.
In `@convex/common/slug.ts`:
- Around line 98-109: appendSuffix currently returns an empty string when
suffix.length >= maxLength which silently causes findUniqueSlug to fail later;
change appendSuffix to detect this case and throw a descriptive Error (include
suffixNumber, computed suffix length and maxLength) instead of returning '' so
callers (e.g., findUniqueSlug) get a clear failure; update the function body in
appendSuffix to perform this guard and ensure any callers handle or propagate
the thrown error.
- Around line 71-77: The assert function currently throws a plain Error (const
assert = (...) => { ... }) which bypasses the client-error path; update assert
so it either (A) delegates to parseOrThrowClientValidation(...) and throws a
client-classified validation error (using ERROR_CODE.VALIDATION_FAILED) when
called on user input, or (B) if assert is intended only for trusted/DB coercion,
add a clear JSDoc on assert stating it must only be used for trusted/DB values
(and callers like requireUsername/requireCampaignSlug should instead call
parseOrThrowClientValidation). Reference the symbols assert, parse, validate,
fallbackMessage, and parseOrThrowClientValidation when making the change.
- Around line 33-50: In buildCanonicalSlugSchema, avoid passing an empty string
as the refine message when minLength is undefined: either only call the
min-length refine when minLength is defined (i.e., conditionally chain the
.refine that checks minLength) or replace that block with
z.string().min(minLength, `${label} must be at least ${minLength} characters`)
when minLength is provided; update the use around the minLength predicate and
message so no refine is invoked with an empty error string and keep the other
validations (trim, whitespace, CANONICAL_SLUG_PATTERN) intact.
- Around line 122-139: The loop wastes attempts when slugify(name) returns an
empty string; detect if normalized is empty immediately after const normalized =
slugify(name) and fail fast with a clear error (or return a specific result)
instead of iterating MAX_UNIQUE_SLUG_ATTEMPTS. Update the function containing
normalized, using the symbols slugify, appendSuffix, MAX_UNIQUE_SLUG_ATTEMPTS,
isValidCandidate and checkFn to check for !normalized and throw a descriptive
Error like "Cannot generate slug: input normalized to empty" (or an appropriate
domain-specific error) so callers get a clear failure instead of 100 no-ops.
In `@convex/gameMaps/mutations.ts`:
- Line 39: The handler's explicit return type should use the branded type
SidebarItemSlug instead of string: change the Promise return annotation in the
handler at the signature (handler: async (ctx, args): Promise<{ mapId:
Id<'sidebarItems'>; slug: SidebarItemSlug }>) and do the same for the other
handler occurrence around line 66; also add an import for the SidebarItemSlug
type (e.g., import type { SidebarItemSlug } from '...') so the TS compiler
recognizes the branded slug type.
- Around line 42-43: createMap normalizes iconName/color to undefined using
requireOptionalSidebarItemIconName(...) ?? undefined and
requireOptionalSidebarItemColor(...) ?? undefined, while updateMap passes
through nullable values (null) to the helpers; make them consistent by
normalizing in updateMap as well: in the updateMap handler call the same helpers
and append "?? undefined" to both results before passing to updateMapFn (or
similarly ensure
requireOptionalSidebarItemIconName/requireOptionalSidebarItemColor always return
only undefined for absent values and update validators accordingly); reference
the functions createMap, updateMap, requireOptionalSidebarItemIconName,
requireOptionalSidebarItemColor, and updateMapFn when implementing the change.
In `@convex/notes/functions/createNote.ts`:
- Line 35: The function createNote currently declares its return type as {
noteId: Id<'sidebarItems'>; slug: string } but prepared.slug is a branded
SidebarItemSlug; change the return type to { noteId: Id<'sidebarItems'>; slug:
SidebarItemSlug } and return the branded prepared.slug directly so callers no
longer need to re-assert; apply the same change to the sibling creator handlers
createMap, createFile, and createFolder (update their return types to use
SidebarItemSlug and return the branded slug value).
In `@convex/sidebarItems/__tests__/sidebarItemValidation.test.ts`:
- Around line 321-394: Add a unit test asserting that validateLocalSidebarMove
still detects a circular-parent error even when isRestoring is true;
specifically, create a tree where an item's parent points to its child (as in
the existing circular test), call validateLocalSidebarMove with isRestoring:
true and the circular parentId, and expect the same { valid: false, error:
'Cannot move item: circular reference detected' } result to ensure the circular
check runs before the isRestoring shortcut.
In `@convex/sidebarItems/functions/treeOperations.ts`:
- Around line 12-21: Update the SidebarTreeBranchItem type to narrow its fields:
replace type: string with type: SidebarItemType and name: string with name:
SidebarItemName so compile-time checks catch typos and callers can avoid
re-validating; update any related usages of SidebarTreeBranchItem and
ItemOperation signatures as needed, and ensure applyToTree (which compares
against SIDEBAR_ITEM_TYPES.folders) and code that currently calls
assertSidebarItemName (around the current assertSidebarItemName usage) are
adjusted so validation is performed by producers rather than here.
In `@convex/sidebarItems/schema/sidebarItemsTable.ts`:
- Around line 16-20: The schema tightened validators (sidebarItemNameValidator,
sidebarItemSlugValidator, sidebarItemIconNameValidator,
sidebarItemColorValidator) may reject existing rows; either backfill/migrate
existing sidebarItems to conform to these rules or document and add a migration
that normalizes current documents (sanitize names to allowed chars/length,
canonicalize slugs to max 255, map or clear iconName values not in the 32-item
enum, and convert/fix color strings to `#RRGGBB` or `#RRGGBBAA`), and then enable
the new validators; update or add a migration script and a brief migration plan
describing the transformations and verification steps so reads won’t fail after
the validator change.
In `@convex/sidebarItems/types/types.ts`:
- Around line 40-43: Unify the two type helpers by replacing the long
conditional chain in WithContentSidebarItem with an Extract-based mapping like
the concise form used in WithContentBySidebarItemType: use
Extract<AnySidebarItemWithContent, { type: T['type'] }> (referencing
AnySidebarItemWithContent, WithContentBySidebarItemType, and
WithContentSidebarItem) so the type selects the matching item by its type
property instead of enumerating each variant.
In `@convex/sidebarItems/validation/color.ts`:
- Line 1: Run the formatter/linters locally and fix the formatting in this file
by running `vp check --fix`, then re-commit; specifically ensure the import line
for zodToConvex (import { zodToConvex } from 'convex-helpers/server/zod4')
adheres to the project's formatting rules (quotes, spacing, trailing newline)
and re-run `vp check` to confirm the CI error is resolved.
In `@convex/sidebarItems/validation/icon.ts`:
- Around line 67-70: The current validateSidebarItemIconName function returns
string|null (null meaning valid), which is confusing; change its return shape to
a clear discriminated result like { ok: true } | { ok: false; message: string }
(or a boolean + separate error accessor) so callers can't accidentally invert
the check. Update validateSidebarItemIconName to call
sidebarItemIconNameSchema.safeParse(iconName) and return { ok: true } on success
or { ok: false, message: result.error.issues[0]?.message ?? 'Invalid icon' } on
failure, and update any callers to use the new { ok } discriminator.
- Around line 99-110: The function coerceSidebarItemIconNameForInput currently
calls assertSidebarItemIconName which throws a generic Error; replace that call
with the client-validation helper requireSidebarItemIconName so invalid client
input yields a structured validation error (ERROR_CODE.VALIDATION_FAILED)
instead of an internal Error. Keep the existing overload behavior (return
null/undefined when passed null/undefined) and only call
requireSidebarItemIconName(iconName) for the non-null string branch; ensure the
function signature and return types (SidebarItemIconName | null | undefined)
remain unchanged.
In `@convex/sidebarItems/validation/move.ts`:
- Around line 32-52: The circular-parent validation (validateNoCircularParent)
currently runs even when isRestoring is true, but the summary/doc implied
restores skip validation; to make intent explicit, add a brief inline comment
next to the validateNoCircularParent(...) call (or above the conditional)
clarifying that circular-parent checks intentionally apply to restore operations
(and must block restores into cycles) so the behavior and documentation remain
aligned; reference the isRestoring flag and the validateNoCircularParent
function in the comment.
In `@convex/sidebarItems/validation/name.ts`:
- Around line 31-51: The new strict sidebarItemNameValueSchema will cause
runtime ClientValidationError for existing rows; run an audit query to find
sidebarItems whose name violates the schema, and if any exist add a DB migration
that sanitizes legacy names before enabling the validator: for each offending
row trim whitespace, strip leading/trailing dots, remove control characters and
forbidden chars (or replace them with a safe character), and ensure the result
produces a non-null parseSidebarItemSlug(slugify(name)) or otherwise set a safe
fallback name; also update deduplicateName to sanitize the base name (call the
same normalization used in the migration) before appending numeric suffixes and
keep assertSidebarItemName usage unchanged so runtime validation matches the
migration.
In `@convex/sidebarItems/validation/orchestration.ts`:
- Around line 167-184: The catch block around findUniqueSlug /
assertSidebarItemSlug loses the original error; update the handler in the
slug-generation flow to preserve and propagate known validation errors or attach
the original error as cause when wrapping: if the caught error is a
ClientValidationError (or similar expected error from
findUniqueSlug/assertSidebarItemSlug) rethrow it unchanged, otherwise call
throwClientError(ERROR_CODE.VALIDATION_FAILED, 'Failed to generate a valid slug
for this item', { cause: error }) (or equivalent API that preserves cause), and
remove the plain console.error logging in favor of using the preserved cause so
stack and structured data from findUniqueSlug/assertSidebarItemSlug (and
checkSlugConflict/validateSidebarItemSlug) remain available for debugging.
- Around line 118-135: validateSidebarMove currently calls
assertSidebarItemName(item.name) which runs runtime validation and can throw for
legacy DB rows; instead avoid re-validating here by passing the raw string as
the branded type or by changing the callee to accept a plain string. Update
validateSidebarMove to call ensureSidebarItemNameAvailable with name: item.name
as SidebarItemName (non-throwing cast) or change
ensureSidebarItemNameAvailable's name parameter from SidebarItemName to string
and leave the argument as item.name; references to checkNameConflict remain
unchanged since it accepts string.
In `@convex/sidebarItems/validation/parent.ts`:
- Around line 292-305: The function currently returns parentId: null when
resolvedParent === VIRTUAL_PARENT which can be misused; change the return shape
to a discriminated result by adding a boolean like resolved: false (or
parentIsVirtual: true) when resolvedParent === VIRTUAL_PARENT and keep parentId
absent or undefined in that branch, and resolved: true plus parentId and
siblings in the normal branch; update callers such as validateCreateItemLocally
to read the new discriminant (resolved or parentIsVirtual) before using parentId
or siblings so future callers cannot accidentally treat null as the campaign
root.
In `@convex/sidebarItems/validation/slug.ts`:
- Around line 10-33: Add concise one-line JSDoc comments above both
validateSidebarItemSlug and validateItemSlug clarifying their contracts and
return shapes: state that validateSidebarItemSlug returns the raw validator
result (string | null) while validateItemSlug returns a ValidationResult ({
valid: boolean, error?: string }). Place the comments immediately above the
exported constants/function declarations (validateSidebarItemSlug,
validateItemSlug) so readers can quickly see the difference without searching;
keep wording consistent with other patterns like
validateSidebarItemName/validateItemName and do not change existing function
logic or signatures (leave parseOrThrowClientValidation and
sidebarItemSlugSchema references untouched).
In `@convex/users/functions/getUserProfile.ts`:
- Around line 7-12: toUserProfileFromDb currently calls assertUsername which
will throw for legacy usernames and break getUserProfile,
getUserProfileByUsername, getUserProfileById and toAuthenticatedProfile; either
confirm a migration/backfill exists or replace the strict call with a soft
normalization routine (e.g., create safeNormalizeUsername used by
toUserProfileFromDb and toAuthenticatedProfile) that catches assertUsername
failures, logs the offending profile/username, coerces into a valid form (trim,
toLowerCase, remove/replace disallowed chars, truncate to 30) and returns the
coerced value instead of throwing, ensuring downstream callers no longer crash
while preserving logs for a later backfill.
In `@convex/users/mutations.ts`:
- Around line 6-12: The handler for updateUsername redundantly re-validates and
normalizes input: remove the requireUsername(args.username) call and any
subsequent unreachable normalization (slugify or normalizeUsernameInput) so the
handler uses the already-validated args.username directly; update the
updateUsername handler to operate on args.username and remove any now-unused
imports or variables (requireUsername, slugify, normalizeUsernameInput) to keep
the code clean.
In `@src/features/campaigns/components/join-campaign-page.tsx`:
- Around line 25-26: The module currently uses non-null assertions on
parseUsername('placeholder') and parseCampaignSlug('placeholder')
(PLACEHOLDER_USERNAME, PLACEHOLDER_SLUG) which can silently become null if
validation rules change; replace each "parseX('placeholder')!" with a runtime
check that throws a clear Error when the parse function returns null (e.g.,
const v = parseX('placeholder'); if (!v) throw new Error('...');) so failures
surface at module load time and keep references to the same symbols
(parseUsername, parseCampaignSlug, PLACEHOLDER_USERNAME, PLACEHOLDER_SLUG) to
locate the change.
In `@src/features/search/hooks/use-recent-items.ts`:
- Around line 21-48: The isRecentEntry type-guard duplicates parseRecentEntry
and discards the parsed slug; remove isRecentEntry and update useRecentItems to
call parseRecentEntry for each raw entry (use parsedEntries =
rawArray.flatMap(entry => { const p = parseRecentEntry(entry); return p ? [p] :
[] }) or equivalent) instead of filter(isRecentEntry) so you reuse parsing logic
and keep the validated SidebarItemSlug; also delete or stop referencing
isRecentEntry wherever else used and ensure parseEntries remains the single
source of truth for entry validation.
In `@src/features/sidebar/hooks/useCreateSidebarItem.ts`:
- Line 105: The client-side calls to assertSidebarItemSlug in
useCreateSidebarItem.ts are redundant because the server already validates slug
via sidebarItemSlugValidator; remove the runtime assertions in the return
statements (e.g., the return producing { id: noteId, slug:
assertSidebarItemSlug(slug), type: args.type } and the similar occurrences at
the other noted lines) and replace them with a simple type cast or a no-op
branding helper (e.g., toSidebarItemSlug that only narrows the type without
re-checking) so the value retains the branded SidebarItemSlug type without the
unnecessary runtime cost.
- Around line 81-88: The current order calls
assertSidebarItemName(args.name.trim()) before validateCreateItem, causing an
exception to be thrown for empty/whitespace names instead of returning the
structured { valid, error } from validateCreateItem; change the order to first
call validateCreateItem with the trimmed name (e.g., validateCreateItem({
...args, name: args.name.trim() })), check nameResult.valid and throw
nameResult.error if invalid, then call assertSidebarItemName on the trimmed
value to produce normalizedName for further use (referencing
assertSidebarItemName, validateCreateItem, nameResult, and normalizedName).
In `@src/features/sidebar/hooks/useEditSidebarItem.ts`:
- Around line 98-102: The current code calls assertSidebarItemName(name.trim())
which can throw before validation.validateName runs; change the flow to first
trim the input, then call validation.validateName(normalizedTrim) to get a
non-throwing result and only after it passes use a safe parser
(parseSidebarItemName) or convert to the branded type; specifically, replace the
direct assertSidebarItemName usage in useEditSidebarItem with a non-throwing
parse or defer branding until after validation.validateName returns valid, and
apply the same pattern for response-side checks (replace
assertSidebarItemSlug(res.slug) with a safe parse or guarded validation where
appropriate).
In `@src/features/sidebar/utils/__tests__/validate-search.test.ts`:
- Around line 46-58: Add a test asserting that overly long slugs are rejected by
validateSearch (and thus parseSidebarItemSlug) by constructing a slug longer
than the enforced maximum and expecting validateSearch({ item: longSlug })
toEqual({}) to lock in the upper bound; reference validateSearch and
parseSidebarItemSlug when adding this case to
src/features/sidebar/utils/__tests__/validate-search.test.ts so future changes
to the max length will be caught.
---
Outside diff comments:
In `@convex/_test/factories.helper.ts`:
- Around line 158-189: sidebarItemBase currently runs
assertSidebarItemSlug(slugify(name)) and insertSidebarItem recomputes and
re-asserts the slug, causing duplicate validation; change sidebarItemBase to
accept an optional slug parameter (or remove its slug computation entirely) and
have insertSidebarItem compute slug once using slugify(name) +
assertSidebarItemSlug, then pass that slug into sidebarItemBase so the slug is
validated and set in only one place (update references to sidebarItemBase and
insertSidebarItem accordingly).
In `@convex/folders/functions/folderHelpers.ts`:
- Around line 22-37: The lookup normalizes the caller string asymmetrically
(normalizedName = name.toLowerCase() without trim) causing mismatches against
item.name.trim().toLowerCase(); fix by trimming when normalizing the incoming
name (set normalizedName = name.trim().toLowerCase()) in the function that
performs the lookup (the normalizedName variable inside findSidebarChildByName /
folderHelpers.ts), or alternatively validate/trim path segments in
resolveOrCreateFolderPath/resolveOrCreateChildFolder before passing them as
SidebarItemName; prefer restoring .trim() on normalizedName to keep
normalization symmetric with item.name.trim().toLowerCase().
In `@convex/sidebarItems/functions/treeOperations.ts`:
- Around line 62-82: The descendant restore loop in restoreTreeDescendants calls
assertSidebarItemName on DB-sourced names (via applyToTree) which can throw and
leave the subtree partially restored; update the loop so it first
normalizes/sanitizes names instead of asserting raw DB values—use
parseSidebarItemName to parse without throwing and if it returns invalid fall
back to a sanitized name (or call deduplicateName as moveSidebarItem does)
before passing the result to findUniqueSidebarItemSlug, then use that slug in
the ctx.db.patch; ensure you do not call assertSidebarItemName directly on
persisted descendant names to avoid throwing mid-traversal.
In `@src/features/campaigns/components/campaign-dialog.tsx`:
- Around line 189-197: The current onChange uses slugify(...) which aggressively
normalizes on every keystroke (Input onChange -> slugify) and breaks
typing/caret; change the onChange to apply a lightweight transformation (e.g.,
lowercasing and replacing spaces with single hyphens) before calling
field.handleChange, and move the full slugify(...) call to the onBlur handler
(Input onBlur -> call full slugify then field.handleBlur) and ensure the submit
path already calling slugify remains as-is (so final normalization happens on
blur/submit); reference the Input component's onChange/onBlur handlers, slugify,
and field.handleChange/field.handleBlur when making this change.
- Around line 62-76: initialSlug is generated with Math.random().toString(36)...
which can produce strings shorter than CAMPAIGN_SLUG_MIN_LENGTH and triggers
validation on mount; replace that generation by passing the random string
through slugify and ensure it meets CAMPAIGN_SLUG_MIN_LENGTH (e.g., generate
slug = slugify(random), then if slug.length < CAMPAIGN_SLUG_MIN_LENGTH pad or
regenerate/append characters until length >= CAMPAIGN_SLUG_MIN_LENGTH) before
passing it into the useState initializer used by form (initialSlug and the
useForm default slug).
In `@src/features/dnd/hooks/useFileDropHandler.tsx`:
- Line 205: The explicit cast on result.id is redundant and should be removed:
stop using "as Id<'sidebarItems'>" when assigning folderId (const folderId =
result.id). Rely on the existing return type from createItem /
UploadSingleFileResult to infer the correct Id type; if TypeScript complains,
adjust the variable's declared type (or the createItem return type) rather than
adding an inline assertion. Update references to folderId accordingly and remove
the cast to avoid hiding future type drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fb486167-7060-40fc-97f5-e0eb9e64fc5e
⛔ Files ignored due to path filters (1)
convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (139)
.vscode/settings.jsonconvex/_test/factories.helper.tsconvex/auth/functions/onCreateUser.tsconvex/blockShares/functions/setBlocksShareStatus.tsconvex/blockShares/functions/shareBlocks.tsconvex/blockShares/functions/unshareBlocks.tsconvex/blocks/functions/getBlockWithShares.tsconvex/blocks/functions/getBlocksWithShares.tsconvex/bookmarks/functions/toggleItemBookmark.tsconvex/campaigns/functions/createCampaign.tsconvex/campaigns/functions/getCampaign.tsconvex/campaigns/functions/joinCampaign.tsconvex/campaigns/functions/updateCampaign.tsconvex/campaigns/mutations.tsconvex/campaigns/queries.tsconvex/campaigns/schema.tsconvex/campaigns/types.tsconvex/campaigns/validation.tsconvex/canvases/functions/createCanvas.tsconvex/canvases/functions/rollbackCanvas.tsconvex/canvases/functions/updateCanvas.tsconvex/canvases/mutations.tsconvex/common/__tests__/validation.test.tsconvex/common/async.tsconvex/common/slug.tsconvex/common/zod.tsconvex/documentSnapshots/functions/getSnapshot.tsconvex/documentSnapshots/functions/rollbackToSnapshot.tsconvex/editHistory/queries.tsconvex/files/functions/createFile.tsconvex/files/functions/updateFile.tsconvex/files/mutations.tsconvex/files/queries.tsconvex/folders/__tests__/folders.test.tsconvex/folders/functions/createFolder.tsconvex/folders/functions/folderHelpers.tsconvex/folders/functions/getFolderContentsForDownload.tsconvex/folders/functions/resolveOrCreateFolderPath.tsconvex/folders/functions/updateFolder.tsconvex/folders/mutations.tsconvex/functions.tsconvex/gameMaps/functions/createItemPin.tsconvex/gameMaps/functions/createMap.tsconvex/gameMaps/functions/requirePinAccess.tsconvex/gameMaps/functions/rollbackGameMap.tsconvex/gameMaps/functions/updateMap.tsconvex/gameMaps/mutations.tsconvex/gameMaps/queries.tsconvex/notes/__tests__/createNote.test.tsconvex/notes/__tests__/notes.test.tsconvex/notes/functions/createNote.tsconvex/notes/functions/rollbackNote.tsconvex/notes/functions/updateNote.tsconvex/notes/mutations.tsconvex/notes/queries.tsconvex/sidebarItems/__tests__/sidebarItemValidation.test.tsconvex/sidebarItems/createParentTarget.tsconvex/sidebarItems/functions/claimPreviewGeneration.tsconvex/sidebarItems/functions/defaultItemName.tsconvex/sidebarItems/functions/enhanceSidebarItem.tsconvex/sidebarItems/functions/getSidebarItemBySlug.tsconvex/sidebarItems/functions/getSidebarItemWithContent.tsconvex/sidebarItems/functions/hardDeleteItem.tsconvex/sidebarItems/functions/moveSidebarItem.tsconvex/sidebarItems/functions/setPreviewImage.tsconvex/sidebarItems/functions/treeOperations.tsconvex/sidebarItems/queries.tsconvex/sidebarItems/schema/sidebarItemsTable.tsconvex/sidebarItems/schema/validators.tsconvex/sidebarItems/sharedValidation.tsconvex/sidebarItems/types/baseTypes.tsconvex/sidebarItems/types/types.tsconvex/sidebarItems/validation.tsconvex/sidebarItems/validation/access.tsconvex/sidebarItems/validation/color.tsconvex/sidebarItems/validation/icon.tsconvex/sidebarItems/validation/move.tsconvex/sidebarItems/validation/name.tsconvex/sidebarItems/validation/orchestration.tsconvex/sidebarItems/validation/parent.tsconvex/sidebarItems/validation/slug.tsconvex/sidebarShares/functions/getSidebarItemShares.tsconvex/sidebarShares/functions/getSidebarItemWithShares.tsconvex/sidebarShares/functions/setAllPlayersPermission.tsconvex/sidebarShares/functions/setFolderInheritShares.tsconvex/sidebarShares/functions/shareSidebarItem.tsconvex/sidebarShares/functions/unshareSidebarItem.tsconvex/users/__tests__/users.test.tsconvex/users/functions/checkUsernameExists.tsconvex/users/functions/getUserProfile.tsconvex/users/mutations.tsconvex/users/queries.tsconvex/users/schema.tsconvex/users/types.tsconvex/users/validation.tsconvex/yjsSync/functions/checkYjsAccess.tssrc/features/campaigns/components/campaign-dialog.tsxsrc/features/campaigns/components/join-campaign-page.tsxsrc/features/campaigns/contexts/campaign-context.tsxsrc/features/campaigns/hooks/__tests__/use-campaign.test.tsxsrc/features/campaigns/hooks/useCampaign.tssrc/features/dnd/hooks/useFileDropHandler.tsxsrc/features/dnd/utils/dnd-registry.tssrc/features/editor/components/extensions/link-click-handler.tsxsrc/features/editor/components/forms/file-form/file-form.tsxsrc/features/editor/components/forms/map-form/map-form.tsxsrc/features/editor/components/viewer/map/map-viewer.tsxsrc/features/editor/components/viewer/map/pin-utils.tssrc/features/editor/hooks/useLinkResolver.tssrc/features/search/hooks/__tests__/use-recent-items.test.tssrc/features/search/hooks/use-recent-items.tssrc/features/settings/components/tabs/account-profile/components/username-row.tsxsrc/features/settings/components/tabs/campaign-people/people-tab.tsxsrc/features/sidebar/components/forms/color-picker.tsxsrc/features/sidebar/components/forms/icon-picker.tsxsrc/features/sidebar/components/forms/sidebar-item-edit-dialog.tsxsrc/features/sidebar/hooks/useCreateSidebarItem.tssrc/features/sidebar/hooks/useEditSidebarItem.tssrc/features/sidebar/hooks/useEditorLinkProps.tssrc/features/sidebar/hooks/useEditorNavigation.tssrc/features/sidebar/hooks/useLastEditorItem.tssrc/features/sidebar/hooks/useMoveSidebarItem.tssrc/features/sidebar/hooks/useSelectedItem.tssrc/features/sidebar/hooks/useSidebarValidation.tssrc/features/sidebar/stores/__tests__/sidebar-ui-store.test.tssrc/features/sidebar/stores/sidebar-ui-store.tssrc/features/sidebar/utils/__tests__/sidebar-create-validation.test.tssrc/features/sidebar/utils/__tests__/sidebar-item-utils.test.tssrc/features/sidebar/utils/__tests__/sidebar-validation.test.tssrc/features/sidebar/utils/__tests__/validate-search.test.tssrc/features/sidebar/utils/sidebar-create-validation.tssrc/features/sidebar/utils/sidebar-item-utils.tssrc/features/sidebar/utils/sidebar-validation.tssrc/features/sidebar/utils/validate-search.tssrc/shared/hooks/useNameValidation.tssrc/shared/utils/category-icons.tssrc/test/factories/campaign-factory.tssrc/test/factories/sidebar-item-factory.tssrc/test/factories/user-factory.ts
💤 Files with no reviewable changes (4)
- convex/sidebarItems/createParentTarget.ts
- convex/sidebarItems/sharedValidation.ts
- src/features/sidebar/utils/sidebar-create-validation.ts
- convex/sidebarItems/validation.ts
| name: args.name, | ||
| description: args.description, | ||
| slug: args.slug, | ||
| slug: args.slug ? requireCampaignSlug(args.slug) : undefined, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer explicit undefined check over truthy test.
campaignSlugValidator should already reject empty strings, so this works today, but args.slug ? … : undefined would silently fall through for any falsy value if the validator were ever loosened. Use args.slug !== undefined ? requireCampaignSlug(args.slug) : undefined to make the intent (optional presence, not truthiness) explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@convex/campaigns/mutations.ts` at line 55, Change the truthy check on
args.slug to an explicit undefined check so optional presence (not falsiness) is
intended: replace the expression currently using args.slug ?
requireCampaignSlug(args.slug) : undefined with a check for args.slug !==
undefined ? requireCampaignSlug(args.slug) : undefined, referencing the same
requireCampaignSlug call and the args.slug property in the mutation to ensure
empty strings or other falsy-but-present values are still validated.
Comment on lines
21
to
+48
| function isRecentEntry(e: unknown): e is RecentEntry { | ||
| return ( | ||
| typeof e === 'object' && | ||
| e !== null && | ||
| typeof (e as RecentEntry).slug === 'string' && | ||
| typeof (e as RecentEntry).timestamp === 'number' | ||
| ) | ||
| if (typeof e !== 'object' || e === null) return false | ||
| const rawSlug = (e as { slug?: unknown }).slug | ||
| const slug = typeof rawSlug === 'string' ? parseSidebarItemSlug(rawSlug) : null | ||
| return slug !== null && typeof (e as { timestamp?: unknown }).timestamp === 'number' | ||
| } | ||
|
|
||
| function parseRecentEntry(entry: unknown): RecentEntry | null { | ||
| if (typeof entry !== 'object' || entry === null) return null | ||
| const rawSlug = (entry as { slug?: unknown }).slug | ||
| const timestamp = (entry as { timestamp?: unknown }).timestamp | ||
| if (typeof rawSlug !== 'string' || typeof timestamp !== 'number') return null | ||
|
|
||
| const slug = parseSidebarItemSlug(rawSlug) | ||
| if (!slug) return null | ||
|
|
||
| return { slug, timestamp } | ||
| } | ||
|
|
||
| function parseEntries(raw: string | null, key: string): Array<RecentEntry> { | ||
| if (!raw) return [] | ||
| try { | ||
| const parsed: unknown = JSON.parse(raw) | ||
| if (!Array.isArray(parsed)) return [] | ||
| return parsed.filter(isRecentEntry) | ||
| return parsed.flatMap((entry) => { | ||
| const parsedEntry = parseRecentEntry(entry) | ||
| return parsedEntry ? [parsedEntry] : [] | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
DRY: isRecentEntry duplicates parseRecentEntry and discards parsed slug.
isRecentEntry re-implements the same validation as parseRecentEntry and then throws away the parsed SidebarItemSlug, forcing the useRecentItems path to re-trust the shape via filter(isRecentEntry) at Line 90 while parseEntries uses parseRecentEntry instead. Consolidate by having useRecentItems (Line 90) also go through parseRecentEntry via flatMap, and drop isRecentEntry.
♻️ Proposed refactor
-function isRecentEntry(e: unknown): e is RecentEntry {
- if (typeof e !== 'object' || e === null) return false
- const rawSlug = (e as { slug?: unknown }).slug
- const slug = typeof rawSlug === 'string' ? parseSidebarItemSlug(rawSlug) : null
- return slug !== null && typeof (e as { timestamp?: unknown }).timestamp === 'number'
-}
-
function parseRecentEntry(entry: unknown): RecentEntry | null {
@@
- const validEntries = Array.isArray(entries) ? entries.filter(isRecentEntry) : []
+ const validEntries = Array.isArray(entries)
+ ? entries.flatMap((e) => {
+ const parsed = parseRecentEntry(e)
+ return parsed ? [parsed] : []
+ })
+ : []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/search/hooks/use-recent-items.ts` around lines 21 - 48, The
isRecentEntry type-guard duplicates parseRecentEntry and discards the parsed
slug; remove isRecentEntry and update useRecentItems to call parseRecentEntry
for each raw entry (use parsedEntries = rawArray.flatMap(entry => { const p =
parseRecentEntry(entry); return p ? [p] : [] }) or equivalent) instead of
filter(isRecentEntry) so you reuse parsing logic and keep the validated
SidebarItemSlug; also delete or stop referencing isRecentEntry wherever else
used and ensure parseEntries remains the single source of truth for entry
validation.
| content: args.content, | ||
| }) | ||
| return { id: noteId, slug, type: args.type } | ||
| return { id: noteId, slug: assertSidebarItemSlug(slug), type: args.type } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant client-side slug assertion.
The server mutation already validates slug through sidebarItemSlugValidator on the return contract; re-asserting with assertSidebarItemSlug on the client is defensive but effectively dead code for any slug that successfully round-tripped through Convex. If you want the branded type at the boundary without the runtime cost, a plain cast (or a toSidebarItemSlug that brands without re-checking) is equivalent. Not blocking — just noise you may want to drop later.
Also applies to: 115-115, 126-126, 137-137, 147-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/sidebar/hooks/useCreateSidebarItem.ts` at line 105, The
client-side calls to assertSidebarItemSlug in useCreateSidebarItem.ts are
redundant because the server already validates slug via
sidebarItemSlugValidator; remove the runtime assertions in the return statements
(e.g., the return producing { id: noteId, slug: assertSidebarItemSlug(slug),
type: args.type } and the similar occurrences at the other noted lines) and
replace them with a simple type cast or a no-op branding helper (e.g.,
toSidebarItemSlug that only narrows the type without re-checking) so the value
retains the branded SidebarItemSlug type without the unnecessary runtime cost.
Comment on lines
+46
to
+58
| it('ignores invalid item slugs', () => { | ||
| expect(validateSearch({ item: 'bad slug' })).toEqual({}) | ||
| expect(validateSearch({ item: 'Bad-Slug' })).toEqual({}) | ||
| expect(validateSearch({ item: '-leading-hyphen' })).toEqual({}) | ||
| expect(validateSearch({ item: 'trailing-hyphen-' })).toEqual({}) | ||
| expect(validateSearch({ item: 'double--hyphen' })).toEqual({}) | ||
| expect(validateSearch({ item: 'special@char' })).toEqual({}) | ||
| }) | ||
| it('accepts valid slugs with numbers and single characters', () => { | ||
| expect(validateSearch({ item: 'item-123' })).toEqual({ item: 'item-123' }) | ||
| expect(validateSearch({ item: 'a' })).toEqual({ item: 'a' }) | ||
| expect(validateSearch({ item: 'a-b-c' })).toEqual({ item: 'a-b-c' }) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Solid coverage for the new slug parsing.
Consider also adding a test for a slug exceeding the max length (if one is enforced by parseSidebarItemSlug) to lock in the upper bound.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/sidebar/utils/__tests__/validate-search.test.ts` around lines 46
- 58, Add a test asserting that overly long slugs are rejected by validateSearch
(and thus parseSidebarItemSlug) by constructing a slug longer than the enforced
maximum and expecting validateSearch({ item: longSlug }) toEqual({}) to lock in
the upper bound; reference validateSearch and parseSidebarItemSlug when adding
this case to src/features/sidebar/utils/__tests__/validate-search.test.ts so
future changes to the max length will be caught.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/editor/components/note-content.tsx (1)
170-214:⚠️ Potential issue | 🔴 CriticalSame StrictMode use-after-destroy here, compounded by misleading effect deps.
- The cleanup at Line 211 destroys
editor._tiptapEditor, buteditoris held inuseState, so StrictMode's simulated unmount→remount will replay this effect against the already-destroyed editor —patchYUndoPluginDestroy,patchYSyncAfterTypeChanged, and all downstreamNoteView/WikiLinkAutocompleterenders then operate on a torn-down view.- The dep array
[doc, editor](Line 214) suggests the effect reacts todocchanges, but the editor was constructed withdoc.getXmlFragment('document'),provider, andusercaptured by theuseStateinitializer at first render. Adocchange would destroy the editor in cleanup and re-run the effect with a stale editor still bound to the original fragment. Today this is masked only because the parent forces a full remount viakey={instanceId}; the deps are still wrong.The cleanest fix is to move
BlockNoteEditor.create(...)into the same effect as its destroy, so creation/destruction are symmetric and collaboration options are always fresh:🔧 Proposed fix
- const [editor] = useState<CustomBlockNoteEditor>( - () => - BlockNoteEditor.create({ - schema: editorSchema, - collaboration: { - provider, - fragment: doc.getXmlFragment('document'), - user: { name: user.name, color: user.color }, - showCursorLabels: 'activity', - }, - }) as CustomBlockNoteEditor, - ) + const [editor, setEditor] = useState<CustomBlockNoteEditor | null>(null) const linkResolver = useLinkResolver(noteId) const onEditorChangeRef = useRef(onEditorChange) onEditorChangeRef.current = onEditorChange useEffect(() => { - onEditorChangeRef.current?.(editor, doc) + const instance = BlockNoteEditor.create({ + schema: editorSchema, + collaboration: { + provider, + fragment: doc.getXmlFragment('document'), + user: { name: user.name, color: user.color }, + showCursorLabels: 'activity', + }, + }) as CustomBlockNoteEditor + setEditor(instance) + onEditorChangeRef.current?.(instance, doc) let cancelled = false let retries = 0 const MAX_RETRIES = 30 const tryPatch = () => { if (cancelled) return - if (editor._tiptapEditor.view.state.plugins.length === 0) { + if (instance._tiptapEditor.view.state.plugins.length === 0) { if (++retries >= MAX_RETRIES) { logger.error('Failed to patch Yjs plugins', { maxRetries: MAX_RETRIES }) return } setTimeout(tryPatch, 50) return } - patchYUndoPluginDestroy(editor._tiptapEditor.view) - patchYSyncAfterTypeChanged(editor._tiptapEditor.view) + patchYUndoPluginDestroy(instance._tiptapEditor.view) + patchYSyncAfterTypeChanged(instance._tiptapEditor.view) } setTimeout(tryPatch, 50) return () => { cancelled = true - editor._tiptapEditor.destroy() + instance._tiptapEditor.destroy() onEditorChangeRef.current?.(null, null) + setEditor(null) } - }, [doc, editor]) + }, [doc, provider, user.name, user.color])Consumers of
editorfurther down will need a null guard (as they were before this refactor).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/editor/components/note-content.tsx` around lines 170 - 214, Move the BlockNoteEditor creation out of the useState initializer and into the same useEffect that currently destroys editor so creation and destruction are symmetric: make the editor state nullable (e.g., CustomBlockNoteEditor | null), create the editor via BlockNoteEditor.create(...) inside the effect using fresh doc, provider and user values, run patchYUndoPluginDestroy and patchYSyncAfterTypeChanged against the newly-created editor._tiptapEditor.view, and call editor._tiptapEditor.destroy() in the effect cleanup; update the effect dependency list to include the true inputs (doc, provider, user) instead of the stale editor and ensure all consumers (including useLinkResolver, onEditorChangeRef usage and NoteView/WikiLinkAutocomplete) guard for a null editor before accessing editor._tiptapEditor or rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 102-103: Remove the redundant per-step env override that
re-assigns VITE_SITE_URL in the CI step: since VITE_SITE_URL is already defined
at the workflow-level env it is inherited by steps, so delete the env:
VITE_SITE_URL: ${{ env.VITE_SITE_URL }} block from the step in
.github/workflows/ci.yml (or only add it back if you intend a different value
for this job/step).
In `@convex/common/slug.ts`:
- Around line 71-77: The assert function currently calls parse and then calls
validate again causing a double parse; change assert to call validate(value)
once, inspect the returned SafeParse result (or equivalent) and if success
return the parsed data cast to BrandedString<Kind>, otherwise throw new
Error(using the validation error message or fallbackMessage). Update assert to
reference validate (not call parse first) and return the parsed value from the
single validate result so you avoid double-parsing; keep the thrown type as a
plain Error but consider documenting that behavior for callers of assert/require
functions.
In `@src/features/campaigns/hooks/__tests__/use-campaign.test.tsx`:
- Line 107: The mock for useMatch should return undefined (the real API's
no-match value) instead of false; update the test mock call
mockUseMatch.mockReturnValue(false) to mockUseMatch.mockReturnValue(undefined)
so it mirrors TanStack Router behavior and prevents masking future
strict-equality checks in useOptionalCampaignRoute or other code that consumes
campaignMatch.
In `@src/features/campaigns/hooks/useCampaign.ts`:
- Around line 57-74: Add a short JSDoc above useOptionalCampaign explaining its
contract: unlike CampaignProvider it does not throw or surface CampaignNotFound
on query errors; it always calls useAuthQuery (skipped when context exists or
identity missing) and, when a query errors, returns the value produced by
buildCampaignContextValue with isCampaignLoaded set false and campaign.isError
true — callers (e.g., PeopleTab) must inspect campaign.isError to handle
not-found or error cases; mention interaction with CampaignProvider to clarify
subscription/skip behavior.
In `@src/features/canvas/components/canvas-remote-cursors.tsx`:
- Line 97: Replace the magic number 150 used in the will-change timeout
expression (isDragging ? PIN_LERP_DURATION : 150) with a named constant (e.g.
WILL_CHANGE_IDLE_TIMEOUT = 150) and use that constant in the ternary; add a
short comment by the constant explaining it is the timeout to clear the GPU hint
shortly after the last cursor update so the intent matches PIN_LERP_DURATION and
both timings are easy to adjust.
- Around line 143-154: The useEffect at the bottom of canvas-remote-cursors.tsx
is redundant: its cleanup duplicates the existing cleanup that already clears
willChangeTimeoutRef and manipulates element.style.willChange in the other
effect, and it captures a stale elementRef.current due to empty deps; remove
this entire useEffect (the one that references elementRef.current and
willChangeTimeoutRef and sets element.style.willChange = '') to avoid
duplicate/no-op cleanup and stale-capture confusion.
In
`@src/features/settings/components/tabs/campaign-people/__tests__/people-tab.test.tsx`:
- Around line 19-55: Add a test that mocks useOptionalCampaign to return a
campaign context with isDm: true (e.g., reuse createCampaign and mockAuthQuery)
and mocks useAuthQuery similarly, render <PeopleTab /> in TestWrapper, then
assert that InviteLinkSection is rendered and that the generated joinUrl (or the
element containing it) matches the expected value; target the PeopleTab
component and the InviteLinkSection/joinUrl outputs when adding the new test so
the DM-specific branch is covered.
In `@src/features/settings/components/tabs/campaign-people/people-tab.tsx`:
- Line 47: The guard in the PeopleTab component is too broad: dmUsername,
campaignSlug and campaign are non-optional properties on CampaignContextType, so
replace the multi-variable null-check with a single check for campaignContext
(e.g., if (!campaignContext) ...), then access campaignContext.dmUsername,
campaignContext.campaignSlug and campaignContext.campaign as before; update the
early return/placeholder logic only to trigger when campaignContext is falsy and
remove the redundant checks for dmUsername, campaignSlug and campaign.
In `@src/features/sidebar/hooks/useEditSidebarItem.ts`:
- Around line 117-120: optimisticFields' name property is declared as plain
string (Partial<Pick<AnySidebarItem, 'name' | ...>>) but you assign a branded
SidebarItemName (normalizedName), so preserve the branded type: change the
optimisticFields type to explicitly use SidebarItemName for name (e.g.,
Partial<{ name: SidebarItemName; iconName: ... }>) or narrow the field by
declaring a small local type that maps name: SidebarItemName while keeping
iconName and color as their branded types; update the declaration of
optimisticFields and keep assigning normalizedName, normalizedIconName, and
normalizedColor without casting so the branded type is preserved for downstream
uses (refer to optimisticFields, AnySidebarItem, normalizedName, and
SidebarItemName).
---
Outside diff comments:
In `@src/features/editor/components/note-content.tsx`:
- Around line 170-214: Move the BlockNoteEditor creation out of the useState
initializer and into the same useEffect that currently destroys editor so
creation and destruction are symmetric: make the editor state nullable (e.g.,
CustomBlockNoteEditor | null), create the editor via BlockNoteEditor.create(...)
inside the effect using fresh doc, provider and user values, run
patchYUndoPluginDestroy and patchYSyncAfterTypeChanged against the newly-created
editor._tiptapEditor.view, and call editor._tiptapEditor.destroy() in the effect
cleanup; update the effect dependency list to include the true inputs (doc,
provider, user) instead of the stale editor and ensure all consumers (including
useLinkResolver, onEditorChangeRef usage and NoteView/WikiLinkAutocomplete)
guard for a null editor before accessing editor._tiptapEditor or rendering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ff2a302-7c8a-494e-8c15-1953d46d1bdc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.github/workflows/ci.ymlconvex/campaigns/functions/updateCampaign.tsconvex/common/__tests__/validation.test.tsconvex/common/async.tsconvex/common/slug.tsconvex/gameMaps/mutations.tsconvex/notes/functions/createNote.tsconvex/notes/mutations.tsconvex/sidebarItems/validation/color.tspackage.jsonsrc/features/campaigns/contexts/__tests__/campaign-context.test.tsxsrc/features/campaigns/contexts/campaign-context.tsxsrc/features/campaigns/hooks/__tests__/use-campaign.test.tsxsrc/features/campaigns/hooks/useCampaign.tssrc/features/canvas/components/canvas-remote-cursors.tsxsrc/features/editor/components/note-content.tsxsrc/features/editor/components/viewer/map/map-viewer.tsxsrc/features/settings/components/tabs/campaign-people/__tests__/people-tab.test.tsxsrc/features/settings/components/tabs/campaign-people/people-tab.tsxsrc/features/sidebar/hooks/useCreateSidebarItem.tssrc/features/sidebar/hooks/useEditSidebarItem.ts
Comment on lines
+117
to
+120
| const optimisticFields: Partial<Pick<AnySidebarItem, 'name' | 'iconName' | 'color'>> = {} | ||
| if (normalizedName !== undefined) optimisticFields.name = normalizedName | ||
| if (normalizedIconName !== undefined) optimisticFields.iconName = normalizedIconName | ||
| if (normalizedColor !== undefined) optimisticFields.color = normalizedColor |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'types.ts' convex/sidebarItems/types | xargs -I{} sh -c 'echo "=== {} ==="; cat {}'
fd -t f 'baseTypes.ts' convex/sidebarItems/types | xargs -I{} sh -c 'echo "=== {} ==="; cat {}'Repository: ntietje1/wizard-archive
Length of output: 5993
🏁 Script executed:
# First, let's look at the complete context around lines 117-120 in useEditSidebarItem.ts
fd -t f 'useEditSidebarItem.ts' | xargs -I{} sh -c 'echo "=== {} ==="; cat -n {} | sed -n "100,130p"'Repository: ntietje1/wizard-archive
Length of output: 1531
🏁 Script executed:
# Also need to check what types normalizedName, normalizedIconName, normalizedColor have
fd -t f 'useEditSidebarItem.ts' | xargs -I{} sh -c 'cat -n {} | head -50'Repository: ntietje1/wizard-archive
Length of output: 2337
🏁 Script executed:
# Check one of the enhanced types to see if they override field types
fd -t f 'types.ts' convex/notes | xargs -I{} sh -c 'echo "=== {} ==="; cat {}'Repository: ntietje1/wizard-archive
Length of output: 1054
🏁 Script executed:
# Check the validation functions to see their return types
fd -t f 'name.ts' convex/sidebarItems/validation | xargs -I{} sh -c 'echo "=== {} ==="; cat {}'Repository: ntietje1/wizard-archive
Length of output: 4189
🏁 Script executed:
# Check if any enhanced types override the name/iconName/color field types
fd -t f 'types.ts' convex/folders | xargs -I{} sh -c 'echo "=== {} ==="; cat {}'Repository: ntietje1/wizard-archive
Length of output: 683
🏁 Script executed:
# Also check GameMap, SidebarFile, Canvas
fd -t f 'types.ts' convex/gameMaps | xargs -I{} sh -c 'echo "=== {} ==="; cat {}'
fd -t f 'types.ts' convex/files | xargs -I{} sh -c 'echo "=== {} ==="; cat {}'
fd -t f 'types.ts' convex/canvases | xargs -I{} sh -c 'echo "=== {} ==="; cat {}'Repository: ntietje1/wizard-archive
Length of output: 3854
🏁 Script executed:
# Check the color and icon validation functions to confirm they return branded types
fd -t f 'color.ts' convex/sidebarItems/validation | xargs -I{} sh -c 'echo "=== {} ==="; cat {} | head -50'
fd -t f 'icon.ts' convex/sidebarItems/validation | xargs -I{} sh -c 'echo "=== {} ==="; cat {} | head -50'Repository: ntietje1/wizard-archive
Length of output: 2921
The name field has a type inconsistency with branded input values.
optimisticFields picks AnySidebarItem fields typed as name: string (plain), iconName: SidebarItemIconName | null, and color: SidebarItemColor | null. The code assigns normalizedName (type SidebarItemName, a branded string) to the plain string field, while normalizedIconName and normalizedColor already match their branded field types in the base definition. TypeScript accepts this assignment via widening, but the optimistic object's name field will carry a plain string type despite coming from a branded value, creating a shape mismatch if the optimistic data is later used in contexts expecting SidebarItemName.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/sidebar/hooks/useEditSidebarItem.ts` around lines 117 - 120,
optimisticFields' name property is declared as plain string
(Partial<Pick<AnySidebarItem, 'name' | ...>>) but you assign a branded
SidebarItemName (normalizedName), so preserve the branded type: change the
optimisticFields type to explicitly use SidebarItemName for name (e.g.,
Partial<{ name: SidebarItemName; iconName: ... }>) or narrow the field by
declaring a small local type that maps name: SidebarItemName while keeping
iconName and color as their branded types; update the declaration of
optimisticFields and keep assigning normalizedName, normalizedIconName, and
normalizedColor without casting so the branded type is preserved for downstream
uses (refer to optimisticFields, AnySidebarItem, normalizedName, and
SidebarItemName).
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/features/editor/components/topbar/editable-breadcrumb.tsx (1)
73-90:⚠️ Potential issue | 🟠 MajorReset
isSubmittingon every validation exit path.Line 76 returns early when
checkNameUniquefails, but Line 90 is skipped, leaving the input disabled for the rest of the component lifetime.Suggested fix
setIsSubmitting(true) try { const error = checkNameUnique(trimmedName) if (error) { toast.error(error) setName(initialName) onChange?.(initialName) setIsEditing(false) return } await onRename?.(trimmedName) setIsEditing(false) } catch (error) { handleError(error, 'Failed to rename item') setName(initialName) onChange?.(initialName) + } finally { + setIsSubmitting(false) } - setIsSubmitting(false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/editor/components/topbar/editable-breadcrumb.tsx` around lines 73 - 90, The early return after validation in the EditableBreadcrumb handler leaves isSubmitting true; ensure setIsSubmitting(false) is executed on every exit path (successful rename, validation failure, and error) by moving the call into a finally block or explicitly calling setIsSubmitting(false) before the validation-return branch; update the function that calls checkNameUnique (and references setIsSubmitting, setIsEditing, setName, onChange, onRename) so isSubmitting is reset whether returning early, throwing, or completing.src/features/sidebar/components/sidebar-item/editable-item-name.tsx (1)
75-91:⚠️ Potential issue | 🟠 MajorClear
isSubmittingwhen validation rejects the rename.Line 79’s error branch returns before Line 91, so after a duplicate/invalid name the component stays in a submitting state and future rename attempts can open with a disabled input.
Suggested fix
// Validate the name before submitting setIsSubmitting(true) try { const error = checkNameUnique(trimmedName) if (error) { toast.error(error) setName(initialName) onCancelRename() return } await onFinishRename(trimmedName) } catch (error) { handleError(error, 'Failed to rename') setName(initialName) onCancelRename() + } finally { + setIsSubmitting(false) } - setIsSubmitting(false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/sidebar/components/sidebar-item/editable-item-name.tsx` around lines 75 - 91, The validation branch returns early leaving isSubmitting true; update the rename flow in the EditableItemName component so setIsSubmitting(false) always runs even on validation rejection or errors—either move setIsSubmitting(false) into a finally block around the try/catch or call setIsSubmitting(false) just before the early return after checkNameUnique(trimmedName) fails; ensure this change touches the block that calls checkNameUnique, toast.error, setName(initialName), onCancelRename, onFinishRename and handleError so subsequent rename attempts re-enable the input.src/features/sidebar/components/forms/sidebar-item-edit-dialog.tsx (1)
73-79:⚠️ Potential issue | 🟠 MajorReset the form when the dialog opens or the target item changes.
With the current dependencies, canceling after local edits and reopening the same item can show stale unsaved values. Switching to another item with identical
name,iconName, andcoloralso skips reset becauseitem._idis no longer tracked.Suggested fix
useEffect(() => { form.reset({ name: item.name ?? '', iconName: item.iconName ?? null, color: item.color ?? null, }) - }, [form, item.color, item.iconName, item.name]) + }, [form, isOpen, item._id, item.color, item.iconName, item.name])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/sidebar/components/forms/sidebar-item-edit-dialog.tsx` around lines 73 - 79, The form reset effect currently only depends on form and item fields (name, iconName, color) which lets reopening the same item or switching to a different item with identical fields skip reset; update the useEffect that calls form.reset to also depend on the item's unique identifier (item._id) and the dialog open state (e.g., open or isOpen if present) so the effect runs whenever the dialog opens or the target item changes, ensuring form.reset({ name: item.name ?? '', iconName: item.iconName ?? null, color: item.color ?? null }) is invoked at those times.convex/sidebarItems/functions/moveSidebarItem.ts (1)
63-82:⚠️ Potential issue | 🟠 MajorLegacy DB names can hard-fail the restore flow.
assertSidebarItemName(deduplicateName(item.name, otherNames))runs the fullSidebarItemNamevalidation pipeline (trim/whitespace/forbidden chars/length) on a DB-sourced value. Any pre-existingsidebarItemsrow whosenamedoes not satisfy the current strict rules (legacy data, older character rules, etc.) will cause the entire restore-from-trash to throwVALIDATION_FAILED, with no way for the user to recover short of admin intervention. This mirrors the concern already raised inconvex/sidebarItems/validation/orchestration.ts(validateSidebarMove).Prefer a non-throwing brand cast (since downstream
findUniqueSidebarItemSlug/checkNameConflicttreat it structurally) or loosenfindUniqueSidebarItemSlug'snametype tostring:♻️ Suggested fix
- const uniqueName = assertSidebarItemName(deduplicateName(item.name, otherNames)) + const uniqueName = deduplicateName(item.name, otherNames) as SidebarItemNameAlso minor: the return type and local
patchdeclareslug?: string, but the assigned value isSidebarItemSlug. Considerslug?: SidebarItemSlugfor symmetry withname.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/sidebarItems/functions/moveSidebarItem.ts` around lines 63 - 82, resolveRestoreConflicts currently calls assertSidebarItemName(deduplicateName(...)) which will throw if DB legacy names don't meet current SidebarItemName validation; change this to a non-throwing approach by either casting the deduplicated name to SidebarItemName using a safe/explicit brand cast (instead of assertSidebarItemName) or update findUniqueSidebarItemSlug to accept a plain string name so you don't validate DB-sourced names here; then keep using findUniqueSidebarItemSlug(ctx, { itemId: item._id, name: uniqueName }) and only set patch.name if the deduplicated value structurally differs, and update the patch type and return signature to use slug?: SidebarItemSlug (matching SidebarItemName/SidebarItemSlug types) for symmetry with name.convex/sidebarItems/functions/treeOperations.ts (1)
53-74:⚠️ Potential issue | 🟠 MajorRestore-descendants path can hard-fail on legacy names.
assertSidebarItemName(i.name)at line 62 runs the fullSidebarItemNamevalidator against a DB value during folder restore. If any descendant has a legacy name that fails the current rules,restoreTreeDescendantsthrows partway through the traversal and the tree is left half-restored (no transaction rollback across iterations once a patch has been applied earlier in the loop). Prefer a non-throwing brand cast here, since the only downstream use is as input tofindUniqueSidebarItemSlug, which only needs the string for slug generation and campaign-scoped uniqueness:♻️ Suggested fix
- const name = assertSidebarItemName(i.name) + const name = i.name as SidebarItemName const slug = await findUniqueSidebarItemSlug(ctx, { itemId: i._id, name, })This is the same root-cause concern raised on
enhanceSidebarItem.ts/moveSidebarItem.ts/orchestration.ts— re-validating DB data with assertion functions turns latent schema drift into loud runtime failures in flows unrelated to the user's intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/sidebarItems/functions/treeOperations.ts` around lines 53 - 74, restoreTreeDescendants currently uses assertSidebarItemName(i.name) which can throw on legacy DB names and leave the tree half-restored; replace the assertion with a non-throwing brand cast so slug generation still gets a string but no exception is thrown (e.g. const name = String(i.name) as SidebarItemName or i.name as unknown as SidebarItemName) in restoreTreeDescendants before calling findUniqueSidebarItemSlug, keeping the rest of the loop unchanged so patches don’t abort mid-traversal.
♻️ Duplicate comments (1)
convex/_test/factories.helper.ts (1)
88-88:⚠️ Potential issue | 🟡 MinorTruthiness-based override handling still silently swallows empty strings.
Lines 88, 121, and 232 all use
value ? validate(value) : fallback, so an explicit empty-string override is indistinguishable fromundefinedand is replaced by the generated default (or, at L232, byslugify(name)) rather than failing validation. This is inconsistent with the!== undefinedpattern used insrc/test/factories/sidebar-item-factory.tsin the same PR, and hides test bugs where an empty override was unintentional.🔧 Suggested change
- ...(username ? { username: assertUsername(username) } : {}), + ...(username !== undefined ? { username: assertUsername(username) } : {}),- ...(slug ? { slug: assertCampaignSlug(slug) } : {}), + ...(slug !== undefined ? { slug: assertCampaignSlug(slug) } : {}),- const validatedSlug = slug ? assertSidebarItemSlug(slug) : assertSidebarItemSlug(slugify(name)) + const validatedSlug = + slug !== undefined ? assertSidebarItemSlug(slug) : assertSidebarItemSlug(slugify(name))Also applies to: 121-121, 232-232
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_test/factories.helper.ts` at line 88, The current truthiness checks (e.g. the spread using "(username ? { username: assertUsername(username) } : {})" and the similar patterns at the other two sites) treat empty string as absent and silently replace it; change those ternaries to check explicitly for undefined (e.g. "username !== undefined ? { username: assertUsername(username) } : {}") so an empty-string override is passed to the validator instead of being dropped, and do the same for the other occurrences (including the case where a provided slug override should be validated rather than replaced by slugify(name)). Ensure you call the same validators used today (e.g. assertUsername and the slug validator) when the value !== undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 50-54: Update the fallow action invocation: change the input
"format: json" to "format: sarif" (or add a short comment why json is
intentional) to restore SARIF-based annotation support, and replace the mutable
action reference "fallow-rs/fallow@v2" with a pinned commit SHA (use the
repository's full commit SHA for the desired v2 release) to follow SHA-pinning
guidance; ensure you keep existing inputs "annotations: true" and
"fail-on-issues: false" intact when making these edits.
In `@convex/sidebarItems/validation/move.ts`:
- Around line 40-52: The code must reject moves to non-existent/stale targets
before running circular/name validations: call getParent(parentId) (or otherwise
verify the target parent exists) and if it returns null/undefined return {
valid: false, error: 'Cannot move item: target parent does not exist' }
immediately (before invoking validateNoCircularParent, isRestoring checks, or
validateSidebarItemNameWithSiblings). Update the logic around
validateNoCircularParent, getParent, getSiblings,
validateSidebarItemNameWithSiblings and isRestoring so the existence check for
parentId happens first and short-circuits the flow when missing.
In `@convex/sidebarItems/validation/orchestration.ts`:
- Around line 73-91: In validateSidebarParentChange remove the redundant
"parentFromDb &&" guards after the throwClientError calls (since
throwClientError is typed never and parentFromDb is already narrowed to
non-null) — directly reference parentFromDb in the subsequent checks and calls
(e.g., the type check, location check, and requireItemAccess call). Also make
validation behavior consistent by updating validateSidebarCreateParent to use
requireItemAccess (instead of getSidebarItemPermissionLevel +
hasAtLeastPermissionLevel) so both flows centralize access gating via
requireItemAccess.
In `@convex/sidebarItems/validation/parent.ts`:
- Around line 238-241: The parent validation currently only checks existence via
buildParentStack(parentTarget.baseParentId, itemsMap) but not that the resolved
base parent is a folder; update the branch handling parentTarget to ensure the
resolved parent item (from parentStack or itemsMap using
parentTarget.baseParentId) has the folder type (e.g., type === 'folder' or
isFolder flag) and return { valid: false, error: 'Parent is not a folder' } when
it isn't; keep using buildParentStack, parentTarget.baseParentId, itemsMap and
also consider pathSegments resolution path so both direct and path branches
enforce folder-only parents.
In `@src/features/campaigns/hooks/useCampaign.ts`:
- Around line 14-17: The exported type CampaignRouteIdentity is not used outside
this module, so remove the export to keep it internal: change the declaration of
CampaignRouteIdentity (the type that contains dmUsername: Username and
campaignSlug: CampaignSlug) from an exported type to a plain internal type by
deleting the "export" keyword and ensure any local usages in functions/hooks in
this file continue to reference CampaignRouteIdentity without import changes.
In
`@src/features/settings/components/tabs/campaign-people/__tests__/people-tab.test.tsx`:
- Around line 129-131: The test currently asserts against
import.meta.env.VITE_SITE_URL but PeopleTab builds the join URL using
getOrigin(), so mock the getOrigin() utility in the test to return a
deterministic origin (e.g., 'https://example.test') and update the expectation
to use that mocked origin combined with `/join/testdm/${campaign.slug}`; ensure
you call jest.mock(...) for the module that exports getOrigin and set the mock
implementation before rendering PeopleTab so the invite-link-section assertion
is stable.
---
Outside diff comments:
In `@convex/sidebarItems/functions/moveSidebarItem.ts`:
- Around line 63-82: resolveRestoreConflicts currently calls
assertSidebarItemName(deduplicateName(...)) which will throw if DB legacy names
don't meet current SidebarItemName validation; change this to a non-throwing
approach by either casting the deduplicated name to SidebarItemName using a
safe/explicit brand cast (instead of assertSidebarItemName) or update
findUniqueSidebarItemSlug to accept a plain string name so you don't validate
DB-sourced names here; then keep using findUniqueSidebarItemSlug(ctx, { itemId:
item._id, name: uniqueName }) and only set patch.name if the deduplicated value
structurally differs, and update the patch type and return signature to use
slug?: SidebarItemSlug (matching SidebarItemName/SidebarItemSlug types) for
symmetry with name.
In `@convex/sidebarItems/functions/treeOperations.ts`:
- Around line 53-74: restoreTreeDescendants currently uses
assertSidebarItemName(i.name) which can throw on legacy DB names and leave the
tree half-restored; replace the assertion with a non-throwing brand cast so slug
generation still gets a string but no exception is thrown (e.g. const name =
String(i.name) as SidebarItemName or i.name as unknown as SidebarItemName) in
restoreTreeDescendants before calling findUniqueSidebarItemSlug, keeping the
rest of the loop unchanged so patches don’t abort mid-traversal.
In `@src/features/editor/components/topbar/editable-breadcrumb.tsx`:
- Around line 73-90: The early return after validation in the EditableBreadcrumb
handler leaves isSubmitting true; ensure setIsSubmitting(false) is executed on
every exit path (successful rename, validation failure, and error) by moving the
call into a finally block or explicitly calling setIsSubmitting(false) before
the validation-return branch; update the function that calls checkNameUnique
(and references setIsSubmitting, setIsEditing, setName, onChange, onRename) so
isSubmitting is reset whether returning early, throwing, or completing.
In `@src/features/sidebar/components/forms/sidebar-item-edit-dialog.tsx`:
- Around line 73-79: The form reset effect currently only depends on form and
item fields (name, iconName, color) which lets reopening the same item or
switching to a different item with identical fields skip reset; update the
useEffect that calls form.reset to also depend on the item's unique identifier
(item._id) and the dialog open state (e.g., open or isOpen if present) so the
effect runs whenever the dialog opens or the target item changes, ensuring
form.reset({ name: item.name ?? '', iconName: item.iconName ?? null, color:
item.color ?? null }) is invoked at those times.
In `@src/features/sidebar/components/sidebar-item/editable-item-name.tsx`:
- Around line 75-91: The validation branch returns early leaving isSubmitting
true; update the rename flow in the EditableItemName component so
setIsSubmitting(false) always runs even on validation rejection or errors—either
move setIsSubmitting(false) into a finally block around the try/catch or call
setIsSubmitting(false) just before the early return after
checkNameUnique(trimmedName) fails; ensure this change touches the block that
calls checkNameUnique, toast.error, setName(initialName), onCancelRename,
onFinishRename and handleError so subsequent rename attempts re-enable the
input.
---
Duplicate comments:
In `@convex/_test/factories.helper.ts`:
- Line 88: The current truthiness checks (e.g. the spread using "(username ? {
username: assertUsername(username) } : {})" and the similar patterns at the
other two sites) treat empty string as absent and silently replace it; change
those ternaries to check explicitly for undefined (e.g. "username !== undefined
? { username: assertUsername(username) } : {}") so an empty-string override is
passed to the validator instead of being dropped, and do the same for the other
occurrences (including the case where a provided slug override should be
validated rather than replaced by slugify(name)). Ensure you call the same
validators used today (e.g. assertUsername and the slug validator) when the
value !== undefined.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 13796649-4f5a-48f5-84ed-3a88c89f1d57
📒 Files selected for processing (32)
.github/workflows/ci.ymlconvex/_test/factories.helper.tsconvex/common/slug.tsconvex/links/__tests__/linkResolution.test.tsconvex/links/functions/syncNoteLinks.tsconvex/sidebarItems/functions/collectDescendants.tsconvex/sidebarItems/functions/defaultItemName.tsconvex/sidebarItems/functions/enhanceSidebarItem.tsconvex/sidebarItems/functions/hardDeleteItem.tsconvex/sidebarItems/functions/moveSidebarItem.tsconvex/sidebarItems/functions/purgeExpiredTrash.tsconvex/sidebarItems/functions/treeOperations.tsconvex/sidebarItems/types/baseTypes.tsconvex/sidebarItems/types/types.tsconvex/sidebarItems/validation/access.tsconvex/sidebarItems/validation/move.tsconvex/sidebarItems/validation/orchestration.tsconvex/sidebarItems/validation/parent.tsconvex/sidebarShares/functions/sidebarItemPermissions.tssrc/features/campaigns/hooks/__tests__/use-campaign.test.tsxsrc/features/campaigns/hooks/useCampaign.tssrc/features/canvas/components/canvas-remote-cursors.tsxsrc/features/editor/components/topbar/editable-breadcrumb.tsxsrc/features/search/utils/__tests__/merge-search-results.test.tssrc/features/settings/components/tabs/campaign-people/__tests__/people-tab.test.tsxsrc/features/settings/components/tabs/campaign-people/people-tab.tsxsrc/features/sidebar/components/forms/sidebar-item-edit-dialog.tsxsrc/features/sidebar/components/sidebar-item/editable-item-name.tsxsrc/features/sidebar/components/sidebar-item/types.tssrc/features/sidebar/hooks/useSidebarValidation.tssrc/routes/__root.tsxsrc/test/factories/sidebar-item-factory.ts
Comment on lines
+50
to
+54
| - uses: fallow-rs/fallow@v2 | ||
| with: | ||
| node-version: ${{ env.NODE_VERSION }} | ||
| cache: true | ||
| - run: vp install --frozen-lockfile | ||
| - name: Run Fallow | ||
| continue-on-error: true | ||
| run: | | ||
| if [ "${{ github.event_name }}" = "pull_request" ]; then | ||
| vp run fallow audit --base origin/main --format annotations --format json > fallow-report.json | ||
| else | ||
| vp run fallow --format json > fallow-report.json | ||
| fi | ||
| annotations: true | ||
| fail-on-issues: false | ||
| format: json |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
fallow-rs/fallow GitHub Action v2 inputs annotations fail-on-issues format
💡 Result:
The fallow-rs/fallow GitHub Action v2 defines the following relevant inputs in its action.yml: - annotations: description: 'Emit findings as inline PR annotations via workflow commands (no Advanced Security required)' required: false default: 'true' - fail-on-issues: description: 'Exit with code 1 if issues are found' required: false default: 'true' - format: description: 'Output format (human, json, sarif, compact, markdown, codeclimate, badge)' required: false default: 'sarif' These inputs control inline PR annotations (enabled by default), workflow failure on detected issues (enabled by default), and output formatting (SARIF by default, which supports annotations when uploaded). The action emits annotations in a dedicated step if annotations is true, review-comments is false, issues are found, and command is not 'fix'. Example usage: - uses: fallow-rs/fallow@v2 with: annotations: true fail-on-issues: true format: sarif Source-grounded details from official action.yml and docs confirm v2 supports these for TypeScript/JS codebase analysis in CI.
Citations:
- 1: https://github.com/fallow-rs/fallow/blob/main/action.yml
- 2: https://github.com/fallow-rs/fallow
- 3: https://docs.fallow.tools/integrations/ci
Consider using SARIF format and pin to a commit SHA for security.
The action inputs are valid, but two refinements are recommended:
-
The
fallow-rs/fallow@v2action defaults toformat: sarif. Usingformat: jsoninstead may reduce the effectiveness of inline PR annotations. Ifjsonis intentional, document why; otherwise, switch tosariffor better annotation integration. -
Pin this third-party action to a full commit SHA instead of the floating
@v2tag (tags are mutable). GitHub's security hardening guide recommends SHA-pinning for non-first-party actions to prevent supply-chain compromises.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 50 - 54, Update the fallow action
invocation: change the input "format: json" to "format: sarif" (or add a short
comment why json is intentional) to restore SARIF-based annotation support, and
replace the mutable action reference "fallow-rs/fallow@v2" with a pinned commit
SHA (use the repository's full commit SHA for the desired v2 release) to follow
SHA-pinning guidance; ensure you keep existing inputs "annotations: true" and
"fail-on-issues: false" intact when making these edits.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor