Skip to content

docs(folders): correct four comments that claimed more than the code does - #6063

Merged
waleedlatif1 merged 1 commit into
stagingfrom
fix/folder-comment-accuracy
Jul 29, 2026
Merged

docs(folders): correct four comments that claimed more than the code does#6063
waleedlatif1 merged 1 commit into
stagingfrom
fix/folder-comment-accuracy

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Follow-up to #6062, which merged before these landed. A comment-accuracy pass over that branch found four factual errors — all mine, all introduced in #6062 — plus some comments that restated the code.

This matters beyond tidiness: in the folder migration work, comments asserting more than the code delivered were the single most recurring defect. A comment that documents a guarantee the code does not provide is worse than no comment, because the next reader trusts it.

The four inaccuracies

1. Reasoning copied to a place it doesn't apply. route.test.ts justified its column-pinned assertion with "resourceType/workspaceId are eq nodes, so a bare 'some isNull exists' check could pass on an unrelated clause." That reasoning is true in lifecycle.test.ts — where a root folder's isNull(parentId) genuinely makes a presence-only check vacuous — but this route's condition has no second isNull node, so the stated failure cannot occur. Replaced with the reason that is actually true here (future-proofing against a nullable filter being added).

2. "Every clause mirrors one column of the partial unique index" — wrong on two counts: deletedAt is the index predicate, not an indexed column, and name is an indexed column with no clause (it's filtered in JS).

3. "Optimistic client reparents can write one" (about a pre-existing folder cycle) — unsupported. wouldCreateFolderCycle has exactly one caller and no write path bypasses it, so a client reparent cannot land a cycle. The realistic origin is two concurrent reparents each passing the check independently.

4. "restoreFolder runs its restoreChildren hook BEFORE un-archiving the folder rows" — true only for the injected hook path. The in-transaction fallback restoreFolderChildren runs after restoreFolderRows, so the claim covered one of two paths.

Also

Trims comments that restated the code or duplicated a TSDoc block on the module under test (the admin route's rationale lives in the route; toFolderApi's lives in queries.ts), and drops two lines of narration about this work's own history.

Pins three presence-only isNull assertions to the deletedAt column. They were not vacuous today — each of those conditions happens to contain only one isNull — but that is incidental, and it is exactly the fragility that already made one assertion in this suite vacuous. Mutation-checked: dropping isNull(folder.deletedAt) from findActiveFolder now fails the test, which it would not have before.

Testing

Typecheck, lint:check (23/23), check:api-validation, and 107 tests pass. No behavioural change to product code — the only non-comment edits are test assertions being tightened.

Type of Change

  • Chore / docs

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)

…does

A comment pass over this branch found four factual errors, all mine:

- the admin route test justified its column-pinned assertion with reasoning
  copied from lifecycle.test.ts — that condition has no second isNull node, so
  the stated vacuity could not occur there
- 'every clause mirrors one column of the partial unique index' was wrong:
  deletedAt is the index PREDICATE, and name has no clause at all
- 'optimistic client reparents can write a cycle' is unsupported —
  wouldCreateFolderCycle has one caller and no write path bypasses it; the real
  origin is two concurrent reparents each passing the check
- 'restoreFolder runs restoreChildren BEFORE un-archiving' holds only for the
  injected hook path; the in-transaction fallback runs after restoreFolderRows

The rest trims comments that restated the code or duplicated a TSDoc block on
the module under test, and pins three presence-only isNull assertions to the
deletedAt column so they cannot be satisfied by an unrelated nullable filter —
the same fragility that made an earlier assertion here vacuous.
@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:53pm

Request Review

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor

cursor Bot commented Jul 29, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Only comments and test assertions change; folder runtime logic is untouched, so reviewer risk is limited to documentation and test maintenance.

Overview
This PR is a comment-accuracy and test-hygiene pass on folder admin routes, lifecycle, naming, and queries—no product behavior changes in the implementation files beyond wording.

Comments are corrected where they overstated guarantees: admin folder list rationale is shortened; nextFolderSortOrder no longer claims cross-module parity; naming tests no longer say every clause mirrors a unique-index column; cycle-restore notes refer to concurrent reparents and the restoreChildren hook path instead of incorrect client-optimistic or single-path ordering claims. Redundant or duplicated commentary (e.g. around toFolderApi) is trimmed.

Tests now pin isNull / isNotNull checks to folder.deletedAt (and keep column-specific checks elsewhere) so assertions cannot pass if another nullable filter is added—matching the stricter pattern already used in lifecycle tests.

Reviewed by Cursor Bugbot for commit dd27264. Configure here.

@greptile-apps

greptile-apps Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Comment-accuracy and test-hardening follow-up with no product behavior change.

  • Corrects four overstated comments in folder admin route, lifecycle, naming, and queries tests/docs.
  • Tightens soft-delete isNull/isNotNull assertions to pin the deletedAt column.
  • Trims restating or duplicated comments; leaves runtime folder logic unchanged.

Confidence Score: 5/5

Safe to merge; only comments and stronger test assertions change, with no runtime product logic edits.

Diff is limited to documentation wording and test expectation pinning; no behavioral paths, contracts, or security boundaries are altered.

Important Files Changed

Filename Overview
apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.test.ts Rewords the soft-delete assertion rationale and keeps the column-pinned deletedAt check.
apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.ts Shortens the soft-delete filter comment without changing the query.
apps/sim/lib/folders/lifecycle.test.ts Moves/clarifies sortOrder filter comments; retains column-pinned soft-delete assertion.
apps/sim/lib/folders/lifecycle.ts Tightens the soft-delete minima comment only.
apps/sim/lib/folders/naming.test.ts Fixes partial-index comment accuracy and notes dual isNull nodes in sibling scoping.
apps/sim/lib/folders/queries.test.ts Pins soft-delete assertions to deletedAt, corrects cycle/restore comments, drops restating toFolderApi block.

Reviews (1): Last reviewed commit: "docs(folders): correct four comments tha..." | Re-trigger Greptile

@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 dd27264. Configure here.

@waleedlatif1
waleedlatif1 merged commit 9edf892 into staging Jul 29, 2026
21 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/folder-comment-accuracy branch July 29, 2026 21:56
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