Clarify settings requirements - #4670
Conversation
📝 WalkthroughWalkthroughChangesConfiguration dependencies
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🔵 Low · up to Settings dependencies may remain inactive when their controlling value is stored in another category, and keyboard users may still edit controls presented as inactive. The change is otherwise mergeable with explicit owner follow-up on these bounded issues. Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. (3 skipped: 3 unsupported.)
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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a2d354a-9dc1-43d9-be8d-edec546dd213
📒 Files selected for processing (6)
app/Http/Resources/Models/ConfigResource.phpapp/Models/Configs.phpdatabase/migrations/2026_08_25_000001_add_config_required_keys.phpresources/js/lychee.d.tsresources/js/v8/components/settings/ConfigGroup.vueresources/js/v8/views/admin/LandingConfig.vue
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a21e9bcd-5517-4863-a6fc-58794e81726d
📒 Files selected for processing (3)
resources/js/v8/components/settings/AllSettings.vueresources/js/v8/components/settings/ConfigGroup.vueresources/js/v8/views/admin/Settings.vue
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| function currentValue(key: string): string | undefined { | ||
| const pending = props.modified.find((m) => m.key === key); | ||
| if (pending !== undefined) { | ||
| return pending.value ?? undefined; | ||
| } | ||
|
|
||
| return props.configs.find((c) => c.key === key)?.value; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect dependency declarations and category assignments.
# Expect: every required key belongs to the dependent configuration's category,
# or the frontend must receive a cross-category saved-value lookup.
rg -n -C 4 --glob '*.php' 'required_keys|->cat|["'\'']cat["'\'']' database app
rg -n -C 3 --glob '*.vue' '<ConfigGroup|:configs=' resources/jsRepository: LycheeOrg/Lychee
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- applicable repository conventions ---'
find /tmp/coderabbit-repo-knowledge/lycheeorg-lychee-cc271fdf -type f -name '*.md' -maxdepth 3 -print
for f in /tmp/coderabbit-repo-knowledge/lycheeorg-lychee-cc271fdf/*/*.md; do
[ -f "$f" ] || continue
case "$f" in
* coding-conventions.md|*learnings*settings*.md|*learnings*frontend*.md)
printf '\n--- %s ---\n' "$f"
sed -n '1,220p' "$f"
;;
esac
done
printf '\n--- required_keys declarations only ---\n'
rg -n -C 5 --glob '*.php' 'required_keys' database app
printf '\n--- ConfigGroup dependency flow ---\n'
sed -n '245,280p' resources/js/v8/components/settings/ConfigGroup.vue
printf '\n--- ConfigGroup callers ---\n'
rg -n -C 5 --glob '*.vue' '<ConfigGroup|:configs=' resources/js/v8/views resources/js/v8/components/settingsRepository: LycheeOrg/Lychee
Length of output: 3129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dependency declarations ---'
rg -n -C 8 --glob '*.php' 'required_keys' database app
printf '\n%s\n' '--- reviewed component flow ---'
sed -n '200,285p' resources/js/v8/components/settings/ConfigGroup.vue
printf '\n%s\n' '--- direct callers ---'
rg -n -C 8 --glob '*.vue' '<ConfigGroup' resources/js/v8
printf '\n%s\n' '--- settings data types and assembly ---'
rg -n -C 6 --glob '*.{php,ts,vue}' 'modified|required_keys|config_categories|configGroups|configGroup' app resources/js | head -n 500Repository: LycheeOrg/Lychee
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- required-keys migration ---'
sed -n '1,240p' database/migrations/2026_08_25_000001_add_config_required_keys.php
printf '\n%s\n' '--- all explicit required-key assignments ---'
rg -n -C 4 --glob '*.{php,sql,json,yaml,yml}' 'required_keys\s*=>|required_keys["'\'']\s*:|required_keys\s*=' database app resources
printf '\n%s\n' '--- config grouping and resource construction ---'
rg -n -C 8 --glob '*.php' 'ConfigResource|config_categories|groupBy\(.*cat|->groupBy\(.*cat' appRepository: LycheeOrg/Lychee
Length of output: 25914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import pathlib, re
migration = pathlib.Path("database/migrations/2026_08_25_000001_add_config_required_keys.php").read_text()
keys = re.findall(r"'([A-Za-z0-9_]+)'", migration)
# Keep only keys that occur in update whereIn lists, excluding category names and column names.
keys = [k for k in keys if k not in {"required_keys", "landing_page_enable", "landing_about_enabled",
"landing_featured_items_enabled", "timeline_photos_enabled",
"timeline_albums_enabled", "ai_vision_enabled",
"ai_vision_face_enabled", "ai_vision_face_overlay_enabled",
"ai_vision_nsfw_enabled", "watermark_enabled", "mod_frame_enabled",
"map_display", "flow_enabled", "flow_min_max_enabled",
"flow_image_header_enabled", "flow_carousel_enabled"}]
files = list(pathlib.Path("database/migrations").glob("*.php"))
for key in keys:
hits = []
for path in files:
lines = path.read_text(errors="replace").splitlines()
for i, line in enumerate(lines):
if re.search(r"'key'\s*=>\s*'" + re.escape(key) + r"'", line):
block = "\n".join(lines[i:min(i + 18, len(lines))])
cats = re.findall(r"'cat'\s*=>\s*([^,\n]+)", block)
hits.append((str(path), i + 1, cats[:1]))
print(f"{key}: {hits}")
PYRepository: LycheeOrg/Lychee
Length of output: 11947
Resolve saved dependency values across all categories.
The migration assigns flow_enabled as a required_keys value for flow_blur_nsfw_enabled and hide_nsfw_in_flow, but those settings belong to the Mod NSFW category while flow_enabled belongs to Mod Flow. Each ConfigGroup receives only its category’s configs; therefore currentValue returns undefined for the saved flow_enabled value unless it has a pending edit. The dependent settings remain inactive.
Pass a complete saved-value lookup to ConfigGroup, while keeping modified as the higher-priority override.
1 participant
Summary by CodeRabbit