v2.0.0 updated with changes removing legacy support#2594
Conversation
📝 WalkthroughWalkthroughThis PR removes v1.x legacy compatibility across config, search, activity, share routing, notifications, and related tests. It also updates the changelog and bumps the ffmpeg Docker base image. ChangesLegacy v1 compatibility removal
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/tests/playwright/noauth/share.spec.ts (1)
57-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale
checkForErrors(0,1)likely breaks after redirect removal.This "share download single file" flow now navigates directly to
/files/public/share/.../testdata/(line 57), but line 63 still asserts 1 expected API error with comment// redirect errors are expected. The functionally identical test infrontend/tests/playwright/sharing/share.spec.ts(lines 33-39) was updated tocheckForErrors()for the same scenario, since the legacy redirect that caused this error was removed in this cohort. This test was likely missed during migration and will fail once the redirect no longer fires.🐛 Proposed fix
await checkForNotification(page, "Downloading..."); - checkForErrors(0,1); // redirect errors are expected + checkForErrors(); });🤖 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 `@frontend/tests/playwright/noauth/share.spec.ts` around lines 57 - 64, The share download single-file Playwright test still expects a redirect-related API error via checkForErrors(0,1), but that legacy redirect path has been removed. Update the test in share.spec.ts to match the newer share flow used by the corresponding test in sharing/share.spec.ts by removing the stale error expectation and calling checkForErrors() without arguments after the Download action.backend/indexing/conditionalRules_test.go (1)
11-90: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winTest helper duplicates
setConditionals' resolved-rules construction logic.
setupConditionalTestIndexreimplements essentially the same map-building loop assettings.setConditionals(config.go, lines 1282-1360): initializing the same map/slice fields, inferring the same root-level global flags, and populatingFileNames/FolderNames/FilePaths/FolderPaths/etc. from the sameConditionalRulefields. SincesetConditionalsis unexported in packagesettings, this test in packageindexingcan't call it directly, so the logic was copied instead.This duplication is a real drift risk: if
setConditionalschanges (e.g., a new rule field is added or the root-level-flag inference logic changes), the test helper can silently diverge from production behavior and continue passing while the app misbehaves — which is effectively the same failure mode as the missingFileNames/FolderNamesmigration flagged inconfig.go.Consider exporting a single source-of-truth function in the
settingspackage (e.g.,settings.ResolveRules(rules []ConditionalRule) ResolvedRulesConfig) that bothsetConditionalsand this test helper call, eliminating the duplicate implementation.As per path instructions, "reduce duplication where there is clear duplication found, use consolidated functions."
♻️ Sketch of a consolidated helper
+// ResolveRules builds optimized map/slice lookup structures from a set of rules. +// Exported so callers (production config loading and tests) share one implementation. +func ResolveRules(rules []ConditionalRule) ResolvedRulesConfig { + resolved := ResolvedRulesConfig{ + FileNames: make(map[string]ConditionalRule), + FolderNames: make(map[string]ConditionalRule), + FilePaths: make(map[string]ConditionalRule), + FolderPaths: make(map[string]ConditionalRule), + FileEndsWith: make([]ConditionalRule, 0), + FolderEndsWith: make([]ConditionalRule, 0), + FileStartsWith: make([]ConditionalRule, 0), + FolderStartsWith: make([]ConditionalRule, 0), + NeverWatchPaths: make(map[string]struct{}), + IncludeRootItems: make(map[string]struct{}), + NoRules: len(rules) == 0, + } + // ... existing loop body from setConditionals ... + return resolved +} func setConditionals(config *Source) { - rules := config.Config.Rules - resolved := ResolvedRulesConfig{ ... } - for _, rule := range rules { ... } - config.Config.ResolvedRules = resolved + config.Config.ResolvedRules = ResolveRules(config.Config.Rules) }func setupConditionalTestIndex(rules []settings.ConditionalRule) *Index { source := &settings.Source{ Name: "test", Path: "/test/path", Config: settings.SourceConfig{ Rules: rules, + ResolvedRules: settings.ResolveRules(rules), }, } - resolved := settings.ResolvedRulesConfig{ ... } - for _, rule := range rules { ... } - source.Config.ResolvedRules = resolved return &Index{Source: *source, mock: true} }🤖 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/indexing/conditionalRules_test.go` around lines 11 - 90, The test helper setupConditionalTestIndex is duplicating the resolved-rules construction logic from settings.setConditionals, which creates drift risk. Extract the shared rule-resolution logic into a single settings package helper, such as ResolveRules or similar, and have both setConditionals and setupConditionalTestIndex use it. Keep the helper responsible for building ResolvedRulesConfig from ConditionalRule fields, including the root-level flags and all name/path maps.Source: Path instructions
🧹 Nitpick comments (4)
_docker/Dockerfile (1)
1-1: 🧹 Nitpick | 🔵 TrivialVerify the new ffmpeg tag before merging.
This update changes the upstream binaries copied into the image. Please confirm
8.1.2-decodeis intentional and that/ffmpegand/ffprobestill exist in the base image.🤖 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 `@_docker/Dockerfile` at line 1, The Dockerfile’s ffmpeg base image tag needs to be verified before merging because it changes the upstream binaries used by the build. Check the FROM instruction that uses gtstef/ffmpeg:8.1.2-decode and confirm the tag is intentional, then verify the ffmpeg and ffprobe assets are still present at the expected locations in the base image so the later copy steps continue to work.backend/database/users/users.go (1)
136-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a marshal/unmarshal round-trip test for the embedded legacy struct.
Given the subtlety of anonymous-embedding + JSON tag interactions (see prior comment) and multiple downstream consumers (
backend/cmd/user.go,backend/http/users.go,backend/state/users.go) relying onuser.ApiKeysand the"apiKeys"JSON key still working after this refactor, a small unit test confirmingjson.Marshal/Unmarshalstill round-tripsApiKeyscorrectly onUserwould guard against regressions if the embedding is changed again later.As per path instructions, "ensure functional features include new unit tests where its easily possible."
🤖 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/database/users/users.go` around lines 136 - 154, The embedded legacy fields in User need a round-trip JSON regression test to ensure anonymous embedding and the apiKeys tag still behave correctly. Add a unit test around User (and UserLegacyFields if needed) that marshals then unmarshals a User with ApiKeys populated, and verifies the "apiKeys" key and user.ApiKeys value survive unchanged; use the User and UserLegacyFields types so future embedding/tag changes don’t break downstream consumers.Source: Path instructions
backend/http/search.go (1)
131-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd table-driven tests for
parseRepeatedScopeParams. Cover the newsourceName:relativePathcontract: empty-source rejection, missing-colon rejection, path sanitization, and the default/for blank paths.🤖 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/http/search.go` around lines 131 - 157, Add table-driven tests for parseRepeatedScopeParams to cover the new sourceName:relativePath parsing contract: verify it rejects inputs with no colon and with an empty source name, confirm it sanitizes the path part before building scopedSourcePath, and check that a blank path after the colon defaults to "/". Use parseRepeatedScopeParams and scopedSourcePath in the test cases so the behavior stays tied to the existing helper.Source: Path instructions
backend/common/settings/config.go (1)
1362-1418: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a focused test for
normalizeNametrimming behavior.The new
normalizeNamehelper (trims/fromFileName/FolderName) is exercised only indirectly through the broaderShouldSkip/IsViewabletest suite inconditionalRules_test.go. A small table test asserting that a rule withFileName: "/foo/"orFolderName: "/bar/"gets normalized to"foo"/"bar"before being written back toconfig.Config.Ruleswould make this behavior explicit and regression-proof.As per path instructions, "ensure functional features include new unit tests where its easily possible."
🤖 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/common/settings/config.go` around lines 1362 - 1418, Add a focused unit test for modifyExcludeInclude that directly verifies normalizeName behavior on config.Config.Rules. Create a small table test covering FileName and FolderName inputs like "/foo/" and "/bar/" and assert they are written back as "foo" and "bar" after modifyExcludeInclude runs. Keep the test close to the existing conditional rules tests so this normalization step is explicitly covered, not only exercised indirectly through ShouldSkip or IsViewable.Source: Path instructions
🤖 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.
Outside diff comments:
In `@backend/indexing/conditionalRules_test.go`:
- Around line 11-90: The test helper setupConditionalTestIndex is duplicating
the resolved-rules construction logic from settings.setConditionals, which
creates drift risk. Extract the shared rule-resolution logic into a single
settings package helper, such as ResolveRules or similar, and have both
setConditionals and setupConditionalTestIndex use it. Keep the helper
responsible for building ResolvedRulesConfig from ConditionalRule fields,
including the root-level flags and all name/path maps.
In `@frontend/tests/playwright/noauth/share.spec.ts`:
- Around line 57-64: The share download single-file Playwright test still
expects a redirect-related API error via checkForErrors(0,1), but that legacy
redirect path has been removed. Update the test in share.spec.ts to match the
newer share flow used by the corresponding test in sharing/share.spec.ts by
removing the stale error expectation and calling checkForErrors() without
arguments after the Download action.
---
Nitpick comments:
In `@_docker/Dockerfile`:
- Line 1: The Dockerfile’s ffmpeg base image tag needs to be verified before
merging because it changes the upstream binaries used by the build. Check the
FROM instruction that uses gtstef/ffmpeg:8.1.2-decode and confirm the tag is
intentional, then verify the ffmpeg and ffprobe assets are still present at the
expected locations in the base image so the later copy steps continue to work.
In `@backend/common/settings/config.go`:
- Around line 1362-1418: Add a focused unit test for modifyExcludeInclude that
directly verifies normalizeName behavior on config.Config.Rules. Create a small
table test covering FileName and FolderName inputs like "/foo/" and "/bar/" and
assert they are written back as "foo" and "bar" after modifyExcludeInclude runs.
Keep the test close to the existing conditional rules tests so this
normalization step is explicitly covered, not only exercised indirectly through
ShouldSkip or IsViewable.
In `@backend/database/users/users.go`:
- Around line 136-154: The embedded legacy fields in User need a round-trip JSON
regression test to ensure anonymous embedding and the apiKeys tag still behave
correctly. Add a unit test around User (and UserLegacyFields if needed) that
marshals then unmarshals a User with ApiKeys populated, and verifies the
"apiKeys" key and user.ApiKeys value survive unchanged; use the User and
UserLegacyFields types so future embedding/tag changes don’t break downstream
consumers.
In `@backend/http/search.go`:
- Around line 131-157: Add table-driven tests for parseRepeatedScopeParams to
cover the new sourceName:relativePath parsing contract: verify it rejects inputs
with no colon and with an empty source name, confirm it sanitizes the path part
before building scopedSourcePath, and check that a blank path after the colon
defaults to "/". Use parseRepeatedScopeParams and scopedSourcePath in the test
cases so the behavior stays tied to the existing helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 63e7c398-8508-4fa7-a444-73e5a03f2797
⛔ Files ignored due to path filters (3)
backend/swagger/docs/docs.gois excluded by!backend/swagger/**backend/swagger/docs/swagger.jsonis excluded by!backend/swagger/**backend/swagger/docs/swagger.yamlis excluded by!backend/swagger/**
📒 Files selected for processing (34)
CHANGELOG.md_docker/Dockerfile_docker/src/noauth/backend/config.yamlbackend/common/settings/config.gobackend/common/settings/generator_simple_test.gobackend/common/settings/generator_test.gobackend/common/settings/migration_test.gobackend/common/settings/structs.gobackend/database/activity/activity.gobackend/database/activity/event_type.gobackend/database/sqldb/activity.gobackend/database/sqldb/activity_test.gobackend/database/users/users.gobackend/http/activity_filter.gobackend/http/httpRouter.gobackend/http/search.gobackend/http/share.gobackend/icons/manifest_share.gobackend/icons/manifest_share_test.gobackend/indexing/conditionalRules_test.gobackend/indexing/indexingFiles.gobackend/indexing/indexingSchedulerLoop.gobackend/indexing/indexing_test.gofrontend/src/api/search.jsfrontend/src/api/tools.jsfrontend/src/utils/activityDetails.jsfrontend/src/utils/appNotifications.jsfrontend/src/views/tools/AdvancedSearch.vuefrontend/tests/playwright/noauth/share.mobile.spec.tsfrontend/tests/playwright/noauth/share.spec.tsfrontend/tests/playwright/previews/share.spec.tsfrontend/tests/playwright/sharing/navigation.spec.tsfrontend/tests/playwright/sharing/share.mobile.spec.tsfrontend/tests/playwright/sharing/share.spec.ts
💤 Files with no reviewable changes (7)
- backend/icons/manifest_share.go
- backend/icons/manifest_share_test.go
- backend/indexing/indexingSchedulerLoop.go
- backend/http/activity_filter.go
- backend/http/httpRouter.go
- backend/http/share.go
- frontend/src/utils/activityDetails.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@frontend/src/components/prompts/UserEdit.vue`:
- Around line 322-346: `normalizeFormUser` in `UserEdit.vue` uses loose null
checks that violate `eqeqeq`; update the `user.permissions` and
`user.account?.permissions` conditionals to use strict null comparisons (`===
null` / `!== null`) while preserving the same fallback behavior. Keep the
changes local to `normalizeFormUser` and `defaultPermissions` so the permissions
normalization logic remains unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e8c1f7ed-b99e-48c5-96bf-f9ab265de168
📒 Files selected for processing (4)
_docker/src/noauth/backend/config.yamlfrontend/src/components/prompts/UserEdit.vuefrontend/tests/playwright/noauth/share.spec.tsfrontend/tests/playwright/sharing/navigation.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/playwright/sharing/navigation.spec.ts
gtsteffaniak
changed the title
1 participant
Description
According to the contributing guide, A PR should contain:
Additional Details
Summary by CodeRabbit
/public/shareURL paths.