docs: strip comment slop and dead commented-out code - #1480
docs: strip comment slop and dead commented-out code#1480VanshajPoonia wants to merge 7 commits into
Conversation
Comments that only restate the identifier below them carry no information and push the code that does matter further apart. Removed across the frontend API wrappers, hooks and settings components, and the route-path comments in the backend that duplicated the decorator underneath (one had already drifted -- albums.py labelled a POST route as GET). Comments that explain *why* an implementation was chosen are kept. A handful of the longest were tightened rather than dropped, since the reasoning is the part worth reading. Dead code removed with it: app/routes/test.py was 173 lines entirely commented out and imported nowhere, plus a debug print, a commented-out formula, and two leftover console.log lines. The commented-out folder-permission test is restored and passing instead of deleted. Closes AOSSIE-Org#1458
github-actions
Bot
added
cleanup
documentation
|
Important Review skippedToo many files! This PR contains 108 files, which is 8 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (108)
You can disable this status message by setting the WalkthroughThe pull request restructures comments across backend and frontend code, removes the backend test router, clarifies folder permission validation, activates a permission-denied test, and removes obsolete debug comments. Runtime behavior remains unchanged except for the folder test coverage. ChangesBackend validation and cleanup
Frontend cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/tests/test_folders.py (2)
249-262: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the permission mask.
mock_access.return_value = Falsemakes this test pass for any access flag. Capture the folder path and assert the exact mask used by the route. For the POSIX traversal fix, assertos.R_OK | os.X_OK.As per path instructions, test code must be comprehensive and cover critical functionality.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_folders.py` around lines 249 - 262, Update the test around the mocked access check to capture the folder path and permission mask passed by the add-folder route, then assert the path matches the requested folder and the mask equals os.R_OK | os.X_OK. Keep the existing unauthorized response assertions.Source: Path instructions
245-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the new test signature.
The new
test_add_folder_permission_deniedmethod has no fixture or return annotations. Add accurate types for the fixtures and mock, and add-> None.As per coding guidelines, backend Python function signatures and return types must be annotated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_folders.py` around lines 245 - 247, Annotate the test_add_folder_permission_denied signature with accurate types for mock_access, client, and temp_folder_structure, and add a -> None return annotation, following the existing fixture annotation conventions in the test module.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/routes/folders.py`:
- Around line 224-226: Update the access check in the folder validation flow
around folder_util_add_folder_tree to require both read and directory
traversal/search permission on request.folder_path, while keeping write
permission optional. Replace the read-only os.access mode with the appropriate
combined read-and-execute access check.
---
Nitpick comments:
In `@backend/tests/test_folders.py`:
- Around line 249-262: Update the test around the mocked access check to capture
the folder path and permission mask passed by the add-folder route, then assert
the path matches the requested folder and the mask equals os.R_OK | os.X_OK.
Keep the existing unauthorized response assertions.
- Around line 245-247: Annotate the test_add_folder_permission_denied signature
with accurate types for mock_access, client, and temp_folder_structure, and add
a -> None return annotation, following the existing fixture annotation
conventions in the test module.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 31d37b9f-d3c6-4d60-8893-d9744f461e21
📒 Files selected for processing (32)
backend/app/models/ONNXSessionBase.pybackend/app/routes/albums.pybackend/app/routes/folders.pybackend/app/routes/images.pybackend/app/routes/share.pybackend/app/routes/test.pybackend/app/schemas/album.pybackend/app/schemas/share.pybackend/app/utils/YOLO.pybackend/app/utils/images.pybackend/app/utils/memory_curator.pybackend/app/utils/videos.pybackend/tests/test_folders.pyfrontend/src/api/api-functions/albums.tsfrontend/src/api/api-functions/share.tsfrontend/src/components/Media/MediaView.tsxfrontend/src/components/VideoPlayer/NetflixStylePlayer.tsxfrontend/src/hooks/useFolderOperations.tsxfrontend/src/hooks/useMemories.tsxfrontend/src/hooks/useMutationFeedback.tsxfrontend/src/hooks/useUserPreferences.tsxfrontend/src/layout/layout.tsxfrontend/src/pages/SearchResults/SearchResults.tsxfrontend/src/pages/SettingsPage/Settings.tsxfrontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsxfrontend/src/pages/SettingsPage/components/SettingsCard.tsxfrontend/src/pages/SettingsPage/components/UserPreferencesCard.tsxfrontend/src/pages/__tests__/SettingsPage.test.tsxfrontend/src/store/hooks.tsfrontend/src/utils/PFPutils/cropImage.tsfrontend/src/utils/PFPutils/pickImagePFP.tsfrontend/src/utils/tauriUtils.ts
💤 Files with no reviewable changes (17)
- frontend/src/utils/tauriUtils.ts
- frontend/src/pages/tests/SettingsPage.test.tsx
- frontend/src/store/hooks.ts
- frontend/src/pages/SettingsPage/Settings.tsx
- backend/app/utils/memory_curator.py
- frontend/src/components/Media/MediaView.tsx
- backend/app/schemas/share.py
- backend/app/routes/share.py
- frontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx
- backend/app/utils/YOLO.py
- backend/app/schemas/album.py
- frontend/src/pages/SettingsPage/components/SettingsCard.tsx
- backend/app/routes/images.py
- frontend/src/hooks/useUserPreferences.tsx
- backend/app/routes/test.py
- frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx
- frontend/src/hooks/useFolderOperations.tsx
The first pass filtered on whether a comment said anything, not on how long it took to say it, so blocks carrying real reasoning were left at four to ten lines. This compresses them to the one-or-two-line house style, keeping the argument in every case -- what was cut is wordiness, not content. Reasoning comments now use // or # rather than a JSDoc frame, which costs two lines before a word is written. Also caught in this pass, all missed by the first scan because it only looked for runs of three or more comment lines: - 117 ASCII banner blocks in backend/tests, the same decoration already removed from backend/app - 145 single-line labels that restate the statement below them (# Initialize logger, // Set all folders, # Sort by path) - 27 "Step N:" narration comments across three route and util modules Kept the labels that carry something the code does not: the FK-ordering note in conftest, the "proper parent, not just a prefix" guard in the watcher, the colour-spec example in the log formatter.
Four dash-rule banners in the frontend test files, which the earlier sweep missed by only matching # and = rules, and the tunnel module header down from four lines to three.
CodeRabbit caught that the comment this PR put on the folder permission check was wrong. It read "Read access is all indexing asks for", but folder_util_add_folder_tree calls os.walk, and on POSIX a directory needs the search bit to be descended into, not just the read bit. Verified: a directory at r-- passes os.access(R_OK) and yields only its top level from os.walk, so the folder would be accepted and everything beneath it silently skipped. Checking R_OK | X_OK rejects it up front instead. The check itself is unchanged from before this PR -- the commented-out os.X_OK line removed earlier was hinting at exactly this. The revived test now asserts the mask, which return_value = False alone could never catch.
|
Thanks @coderabbitai. The major one was a real bug in this PR, and it was mine. Addressed in f6f35ef. 1. Traversal permission: accepted, and the comment was the actual defect. You're right, and the problem was worse than the check. The comment this PR added asserted something false. I wrote "Read access is all indexing asks for" while replacing the old commented-out Verified rather than assumed: A readable-but-unsearchable directory passes the old check, then Worth noting the check is unchanged relative to 2. Assert the permission mask: accepted. Good catch that 3. Annotate the test signature: declining, for consistency. No test method in
Happy to do it as a follow-up if maintainers want it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- commit summary ---'
git show --stat --oneline --decorate --no-renames f6f35ef
echo '--- changed hunks ---'
git show --format=fuller --no-ext-diff --unified=30 --no-renames f6f35ef -- \
backend/app/routes/folders.py \
backend/tests/test_folders.py
echo '--- folder utility outline ---'
ast-grep outline backend/app/utils/folders.py --items all --view expanded
echo '--- relevant folder utility implementation ---'
rg -n -A35 -B8 '\bdef\s+folder_util_add_folder_tree\s*\(' backend/app/utils/folders.py
echo '--- test function definitions and annotations ---'
rg -n -P '^\s*(async\s+)?def\s+test_[A-Za-z0-9_]+\s*\(' backend/tests/test_folders.pyLength of output: 15595
The test now asserts the exact access mask. This prevents a future regression to I agree with declining the isolated test annotation. 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
I agree that adding one annotation in 🐇 ✅ You are interacting with an AI system. |
Link your account with GitcordThanks for opening this PR, @VanshajPoonia! To receive Discord notifications and contributor tracking for this organization:
Once linked, Gitcord can notify you about reviews, merges, and more. — Posted by Gitcord |
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
…cture-comments # Conflicts: # backend/app/utils/images.py
Reviewing my own rewrites, this one repeated the mistake CodeRabbit caught in the folder permission check: a justification I inferred instead of read. It claimed a Tauri file path is not something an <img> can load. It is, via convertFileSrc, which utils/memories.ts and NetflixStylePlayer both rely on. The real reason is downstream: pickImageFile feeds avatarCropDialog, which calls getCroppedImg, which reads the canvas back with toDataURL. An asset:// source would taint the canvas and make that throw. A data URL is same-origin, so it cannot.
An independent review of every comment this branch adds or rewords found ten that assert something the code does not do. Four are regressions from this branch's compression, six were already inaccurate upstream and were carried forward when the comment was reworded. - test_memory_curator: the gate uses mean pairwise cosine, which is 0 for an orthogonal basis at any N, so scattered sets are rejected. The old note described centroid cosine (1/sqrt(N)) and drew the opposite conclusion. - useMemories: /generate writes 'running' before it returns. forcePolling exists because refetchInterval reads a status cache that still says 'complete'. - useUserPreferences: writeEpoch counts writes, not reads. - tunnel.rs: PROVIDERS holds one entry. Restore the note that srv.us is the intended second, pending a key of its own. - imageFallback: clearing img.onerror does not detach a React onError prop, so nothing detaches and the fallback has to be an asset that cannot fail. - ONNXSessionBase: get_session takes _lock before inference does, so inference can block. The split keeps a long run from holding up create and close. - memory_scoring: composite_score renormalizes, so a zero does not put every video below every photo. - layout.ts, useShareTunnel, memories.ts: three more claims narrowed to what the code actually guarantees. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
1 participant
Closes #1458
A slop audit over every comment in
frontend/src,backend/,frontend/src-tauri/src, andsync-microservice. 125 files, +222 / -1369.Following @rohan-pandeyy's steer on the issue (keep comments focused, strip the jargon, preserve the ones that explain why an implementation was chosen), plus the house preference for one line, sometimes two, rarely three.
Result
TODO/FIXME/FUTUREmarkersBlock length distribution went from
10, 9, 8x3, 7x10, 6x8, 5x14, 4x50, 3x201to a flat3x97.Removed: comments that restate the code below them
api-functions/albums.ts: 10 JSDoc blocks (/** Get all albums */,@param albumId - Album UUID). 7 of the 12 sibling API files already carry no JSDoc, so this file was the outlier rather than the convention.useMutationFeedback.tsx: 11 blocks, including a/** Card title */-shaped doc on every option field, and// Handle loading statesitting on the effect that handles loading.SettingsCard.tsx,useFolderOperations.tsx,useUserPreferences.tsx, the Settings page components,store/hooks.ts,tauriUtils.ts: same pattern.# GET /albums/{album_id}/imageslabelled a route declared as@router.post("/{album_id}/images/get"). A comment that can silently go stale like that is worth less than the decorator it duplicates.# Initialize logger,// Set all folders,# Sort by path,# Mock the executor state.Step N:narration comments acrossutils/images.py,routes/folders.py,routes/face_clusters.py.# ####...,# ====...,/* ----- */) acrossbackend/app, all 20backend/testsmodules, and two frontend test files.Restructured rather than removed
Blocks carrying real reasoning were compressed to one or two lines with the argument intact.
services/tunnel.rswent from 10 lines to 3,useUserPreferencesfrom 9 to 3, the SigLIP calibration note from 7 to 3. Reasoning comments now use//or#instead of a JSDoc frame, which costs two lines before a word is written.Kept deliberately: labels that carry something the code does not say. The FK-ordering note in
conftest.py, the "proper parent, not just a prefix" guard in the watcher, the"red,bg_white"format example in the log formatter.Dead code
backend/app/routes/test.py: 173 lines, entirely commented out, imported nowhere. Deleted; git remembers it.# print(...)inYOLO.py, a commented-out scoring formula inroutes/images.py, and two leftoverconsole.logcalls, one of which was printing a resolved video path on every render in the shipped player.routes/folders.pycarried an# Uncomment the following lines if you want to check for write and execute permissionsblock. See the permission fix below.One deletion turned into a restoration
tests/test_folders.pycarried a commented-outtest_add_folder_permission_denied. Rather than delete it, I restored it. It passes, and it covers the exactos.accessbranch discussed below. Backend suite goes from 1088 to 1089.One functional change, flagged deliberately
This PR is otherwise cosmetic, but
f6f35efchanges one line of behaviour, and reviewers should look at it as a bug fix rather than cleanup.Removing the commented-out permission block above, I replaced it with a comment reading "Read access is all indexing asks for". @coderabbitai correctly flagged that as false:
folder_util_add_folder_treecallsos.walk, and on POSIX descending into a directory needs the search bit, not the read bit. Verified:A folder at
r--passes the old check, thenos.walkreturns the top level and descends nowhere, so the folder is accepted and everything beneath it silently never indexed. Nowos.R_OK | os.X_OK, with the 401 message updated to match. Write access stays optional.Worth noting the commented-out code removed here had
os.X_OKin it. It was pointing at this the whole time.If maintainers would rather keep this PR strictly docs-only, I am happy to revert the route to
os.R_OKwith a neutral comment and raise the permission fix separately.Verification
npm run lint:check,npm run format:check,tsc --noEmit, 377 frontend tests, black, ruff, 1089 backend tests,cargo fmt --check. All clean.Notes for reviewers
backend/app/schemas/test.py(51 lines) is now provably dead. Its only reference was theroutes/test.pydeleted here. Left in place to keep this PR about comments; happy to remove it here or in a follow-up.Args:/Returns:blocks restating already-typed parameters. Out of scope here for two reasons: it would roughly double the diff, and FastAPI turns route-handler docstrings into OpenAPI descriptions, so those are user-facing output needing individual judgement rather than a sweep. Happy to take it as a second PR.test_folders.pyhas no type annotations on any of its ~60 test methods. Annotating only the one revived here would make it the odd one out, so I left it consistent with the module. A separate PR covering the whole file would be the right home for that.🤖 Generated with Claude Code