Skip to content

fix(folders): stop archived rows skewing sortOrder, hide them from admin, and cover the two untested modules - #6062

Merged
waleedlatif1 merged 2 commits into
stagingfrom
feat/folder-followups
Jul 29, 2026
Merged

fix(folders): stop archived rows skewing sortOrder, hide them from admin, and cover the two untested modules#6062
waleedlatif1 merged 2 commits into
stagingfrom
feat/folder-followups

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Follow-ups from the generic-folders work, on a fresh branch off origin/staging. Two correctness fixes and the first tests for the two modules that had none.

Fixes

nextFolderSortOrder counted soft-deleted rows. It returns min - 1 to put a new folder at the top, so including archived rows let every delete ratchet the floor further negative and never recover — an archived folder at -400 forces the next new folder to -401, forever. Both minima (folders and child resources) now see only rows a user can still see. The Files path (workspace-file-folder-manager) has always filtered this way; the generic engine never did.

The admin workspace-folders endpoint listed soft-deleted folders. Both the count and the page omitted deletedAt, so an operator inspecting a workspace saw folders sitting in Recently Deleted plus an inflated total — disagreeing with every user-facing folder list. Pre-existing, not cutover fallout.

Tests for naming.ts and queries.ts

Both modules had zero tests and are vi.mock'd at every call site, so their bodies executed in no test anywhere. That left the two bug classes that caused real defects in the folder migration completely unasserted server-side:

  • the suffix sequence — must start at (1), skip taken suffixes, and fill gaps. Its client twin nextUntitledFolderName is tested and pins the same shape, so the tested client was the only guard on a contract the server could silently drift from.
  • resourceType scoping on the id-keyed lookups (findActiveFolder, wouldCreateFolderCycle). Every other query is scoped through a list filter; these accept a caller-supplied id, so a missing clause silently crosses resource trees — filing a knowledge base under a table folder where no page renders it.

Also covers resolveRestoredFolderId's restoringFolderIds short-circuit, which is what stops a folder cascade dumping an entire subtree at the workspace root, and toFolderApi, the single normalizer keeping every folder route on one wire shape.

Every assertion is mutation-checked

Each was verified to FAIL when the behaviour it claims to guard is removed:

mutation caught by
suffix loop seeded at 2 3 naming tests
drop resourceType from the sibling query sibling-scoping test
drop deletedAt from the sibling query archived-sibling test
drop resourceType from findActiveFolder scoping test
drop resourceType from the cycle walk walk-scoping test
drop the restoringFolderIds short-circuit short-circuit test
drop either deletedAt filter in nextFolderSortOrder sortOrder test
drop deletedAt from the admin route admin test

Worth flagging: the first version of the sortOrder test was vacuous. For a root folder the parent condition is itself an isNull node, so a "some isNull exists" check passed with the soft-delete filter deleted. It now asserts the specific column. That is the third vacuous test caught by mutation-checking in this line of work, which is why every assertion here went through it.

Overlap with realtime-rooms

Checked explicitly. This branch touches 6 files; exactly one, lib/folders/lifecycle.ts", is also touched by realtime-rooms— which only addsnotifyFolderResourceChangedcalls at lines 22/227/313/393/555, while this change is confined tonextFolderSortOrderat 131–158. A test-merge ofrealtime-roomsinto this branch produces **zero conflicts in any file here**. It does conflict intable-grid.tsxandcheck-api-validation-contracts.ts, but those conflicts already exist between realtime-roomsandorigin/staging` and are unrelated to this work.

Not included, deliberately

  • Restore does not undo delete (highest remaining user impact — restoring a folder of deployed workflows reports success while schedules stay disabled). Needs a design decision: snapshot prior state at archive time (schema change + migration) versus reporting what needs redeploying. Too big to fold in silently.
  • Workflow-surface consolidation (pinning, breadcrumbs, unified folder naming, maxLength 50 vs 100). Broad UI work needing visual verification, and it lands in files.tsx/tables.tsx — both touched by realtime-rooms, so it would create exactly the overlap this branch was asked to avoid.

Testing

Typecheck, check:api-validation, check:migrations, lint, and 150 tests across the affected suites pass. No migration in this PR.

Type of Change

  • Bug fix
  • Tests

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

…min, and cover the two untested modules

nextFolderSortOrder returned min - 1 over ALL rows including soft-deleted ones,
so every delete ratcheted the floor further negative and never recovered — an
archived folder at -400 forced the next new folder to -401 forever. Both minima
(folders and child resources) now see only rows a user can still see, which is
how the Files path has always worked.

The admin workspace-folders endpoint counted and paginated soft-deleted folders,
so an operator saw phantom folders and an inflated total, disagreeing with every
user-facing list.

Adds naming.test.ts and queries.test.ts. Both modules had zero tests and are
mocked at every call site, so their bodies executed in no test anywhere. That
left unasserted the two bug classes that caused real defects in the folder
migration: the suffix sequence (must start at (1) and skip taken suffixes) and
resourceType scoping on the id-keyed lookups, where a missing clause silently
files a knowledge base under a table folder.

Every assertion is mutation-checked. The first version of the sortOrder test was
vacuous — for a root folder the parent condition is itself an isNull node, so a
presence-only check passed with the soft-delete filter deleted; it now asserts
the specific column.
@vercel

vercel Bot commented Jul 29, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jul 29, 2026 9:48pm

Request Review

@cursor

cursor Bot commented Jul 29, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Targeted query-filter fixes and test-only additions; no schema or auth changes. Behavior aligns admin and create-folder ordering with existing user lists.

Overview
Two soft-delete bugs are fixed so folder behavior matches user-facing lists. nextFolderSortOrder now ignores archived folders and child resources when computing min - 1, preventing sort order from drifting further negative after every delete. The admin workspace folders list and count share a filter that excludes rows with deletedAt set.

@sim/testing gains flattenMockConditions and hasMockCondition so tests can assert Drizzle WHERE clauses; cascade tests switch to these shared helpers.

New coverage pins behavior that was previously only mocked at call sites: admin folder listing filters, lifecycle sort-order queries, deduplicateFolderName (suffix (1) contract and sibling scoping), and queries (findActiveFolder, cycle walks, restore short-circuit, list scopes, toFolderApi wire shape).

Reviewed by Cursor Bugbot for commit c029210. Configure here.

@greptile-apps

greptile-apps Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Fixes soft-deleted folder handling in sort-order and admin listing, and adds first unit coverage for folder naming/queries plus shared mock condition helpers.

  • nextFolderSortOrder now excludes soft-deleted folders and archived child resources when computing min - 1.
  • Admin workspace folders GET filters deletedAt on both count and page queries.
  • New tests for naming.ts, queries.ts, admin folders route, and sortOrder filter behavior; cascade tests use shared flattenMockConditions/hasMockCondition.

Confidence Score: 5/5

This PR appears safe to merge; no blocking failures remain from prior or current review.

No blocking failure remains.

Important Files Changed

Filename Overview
apps/sim/lib/folders/lifecycle.ts Excludes soft-deleted folders and archived child rows from sortOrder minima so deletes no longer ratchet negative sort values.
apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.ts Admin folder list/count now filter deletedAt so operators match user-facing active folder lists.
packages/testing/src/mocks/database.mock.ts Shared helpers flatten and inspect mock SQL condition trees for WHERE-clause assertions in folder tests.
apps/sim/lib/folders/naming.test.ts Covers suffix sequencing and sibling scoping for deduplicateFolderName.
apps/sim/lib/folders/queries.test.ts Covers resourceType scoping, cycle walk, restore short-circuit, list scopes, and toFolderApi serialization.

Reviews (2): Last reviewed commit: "refactor(testing): share the drizzle con..." | Re-trigger Greptile

Asserting on WHERE clauses is the only way to pin a filter the row-queue mocks
cannot enforce — a mock returns whatever was queued regardless of the predicate
— so this pattern spreads to every test that guards a query's scoping. It had
reached five local copies of the same flatten/has pair, four of them added by
the tests in this branch.

Moved to @sim/testing beside createMockSqlOperators, whose output shape they
parse, so the helper and the node types it depends on live together.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit c029210. Configure here.

@waleedlatif1
waleedlatif1 merged commit 0f12799 into staging Jul 29, 2026
21 checks passed
@waleedlatif1
waleedlatif1 deleted the feat/folder-followups branch July 29, 2026 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant