fix: handle non-string user/team prefs to prevent Preferences UI crash#2885
fix: handle non-string user/team prefs to prevent Preferences UI crash#2885HarshMN2345 merged 1 commit intomainfrom
Conversation
Console (appwrite/console)Project ID: Tip Build commands execute in runtime containers during deployment |
WalkthroughThe pull request updates the preferences handling system to improve type flexibility and consistency. The Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Greptile SummaryPrevented UI crashes by adding defensive type coercion to handle non-string user/team preference values throughout the preferences system. Key changes:
Why this matters: Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 5525372 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.svelte (1)
37-39: SameString(key ?? '')over-defensiveness as in the userupdatePrefs.svelte.
Object.entrieskeys are alwaysstring; the fix needed here is only for values. Consider simplifying tocreatePrefRow(key, String(value ?? '')).♻️ Proposed simplification
- ? entries.map(([key, value]) => - createPrefRow(String(key ?? ''), String(value ?? '')) - ) + ? entries.map(([key, value]) => + createPrefRow(key, String(value ?? '')) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.svelte around lines 37 - 39, The code is over-defensive converting entry keys with String(key ?? ''), but Object.entries always yields string keys; update the map call to pass the key directly and only coerce values: use createPrefRow(key, String(value ?? '')) in the entries.map callback so createPrefRow receives the original string key and a safe stringified value; locate the mapping that currently calls createPrefRow(String(key ?? ''), String(value ?? '')) and replace it accordingly.src/lib/helpers/prefs.ts (3)
32-33:stringTrimis redundant insanitizePrefs—PrefRowfields are alreadystring.
sanitizePrefsacceptsPrefRow[]where both fields are typedstring, so theString(s ?? '')wrapping insidestringTrimis a no-op. Using.trim()directly keeps the intent clear.♻️ Proposed simplification
export function sanitizePrefs(prefs: PrefRow[]) { - return prefs.filter((p) => stringTrim(p.key).length > 0 && stringTrim(p.value).length > 0); + return prefs.filter((p) => p.key.trim().length > 0 && p.value.trim().length > 0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/helpers/prefs.ts` around lines 32 - 33, The sanitizePrefs function uses stringTrim on PrefRow.key and PrefRow.value even though PrefRow fields are already strings; replace stringTrim(p.key) and stringTrim(p.value) with direct p.key.trim() and p.value.trim() to simplify and clarify intent, updating the sanitizePrefs function accordingly (refer to sanitizePrefs and stringTrim to locate the code).
7-9:PrefRow[]branch in the union is a subtype of{ key: unknown; value: unknown }[]and is redundant.
PrefRowis{ key: string; value: string }, which satisfies{ key: unknown; value: unknown }. Listing it explicitly in the union adds noise without widening the accepted type.♻️ Proposed simplification
export function normalizePrefs( - entries: [string, unknown][] | PrefRow[] | { key: unknown; value: unknown }[] + entries: [string, unknown][] | { key: unknown; value: unknown }[] ): [string, string][] {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/helpers/prefs.ts` around lines 7 - 9, The function signature for normalizePrefs includes a redundant branch PrefRow[] in the union because PrefRow is { key: string; value: string } which already matches { key: unknown; value: unknown }; update the parameter type to remove PrefRow[] (i.e., change entries: [string, unknown][] | { key: unknown; value: unknown }[]), leaving normalizePrefs and any callers unchanged, and run type-check to ensure no other places relied on the explicit PrefRow[] union member.
16-16: Redundant re-stringification infilteraftermapalready yields[string, string].After the
.map()step every element is a[string, string]tuple, sostringTrim(k)just adds a needlessString(k ?? '')wrap. Use.trim()directly on the already-string values.♻️ Proposed simplification
- .filter(([k, v]) => stringTrim(k).length > 0 && stringTrim(v).length > 0) + .filter(([k, v]) => k.trim().length > 0 && v.trim().length > 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/helpers/prefs.ts` at line 16, The filter currently calls stringTrim(k) and stringTrim(v) even though the preceding .map already produced [string, string], so change the predicate in the .filter to use the native .trim() on the existing strings (e.g. .filter(([k, v]) => k.trim().length > 0 && v.trim().length > 0)); remove the redundant stringTrim wrapping while keeping the same semantics; reference the existing stringTrim identifier only if needed elsewhere but do not re-stringify k or v in the filter.src/routes/(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte (1)
37-39:String(key ?? '')is over-defensive —Object.entrieskeys are alwaysstring.
Object.entriesalways returnsstringkeys, sokeyis nevernull/undefinedand theString(key ?? '')wrapper adds no protection. The value coercionString(value ?? '')is the meaningful part of the fix and is correct.♻️ Proposed simplification
- ? entries.map(([key, value]) => - createPrefRow(String(key ?? ''), String(value ?? '')) - ) + ? entries.map(([key, value]) => + createPrefRow(key, String(value ?? '')) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte around lines 37 - 39, The map callback is over-defensive by coercing the Object.entries key with String(key ?? ''), since keys from Object.entries are always strings; update the entries.map call that builds createPrefRow (the arrow callback using [key, value]) to pass the existing key directly (or at most String(key)) and keep the value coercion as String(value ?? ''), e.g. replace the first argument to createPrefRow with key instead of String(key ?? '') so locate the entries.map callback and change its first parameter usage accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/helpers/prefs.ts`:
- Around line 32-33: The sanitizePrefs function uses stringTrim on PrefRow.key
and PrefRow.value even though PrefRow fields are already strings; replace
stringTrim(p.key) and stringTrim(p.value) with direct p.key.trim() and
p.value.trim() to simplify and clarify intent, updating the sanitizePrefs
function accordingly (refer to sanitizePrefs and stringTrim to locate the code).
- Around line 7-9: The function signature for normalizePrefs includes a
redundant branch PrefRow[] in the union because PrefRow is { key: string; value:
string } which already matches { key: unknown; value: unknown }; update the
parameter type to remove PrefRow[] (i.e., change entries: [string, unknown][] |
{ key: unknown; value: unknown }[]), leaving normalizePrefs and any callers
unchanged, and run type-check to ensure no other places relied on the explicit
PrefRow[] union member.
- Line 16: The filter currently calls stringTrim(k) and stringTrim(v) even
though the preceding .map already produced [string, string], so change the
predicate in the .filter to use the native .trim() on the existing strings (e.g.
.filter(([k, v]) => k.trim().length > 0 && v.trim().length > 0)); remove the
redundant stringTrim wrapping while keeping the same semantics; reference the
existing stringTrim identifier only if needed elsewhere but do not re-stringify
k or v in the filter.
In
`@src/routes/`(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.svelte:
- Around line 37-39: The code is over-defensive converting entry keys with
String(key ?? ''), but Object.entries always yields string keys; update the map
call to pass the key directly and only coerce values: use createPrefRow(key,
String(value ?? '')) in the entries.map callback so createPrefRow receives the
original string key and a safe stringified value; locate the mapping that
currently calls createPrefRow(String(key ?? ''), String(value ?? '')) and
replace it accordingly.
In
`@src/routes/`(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte:
- Around line 37-39: The map callback is over-defensive by coercing the
Object.entries key with String(key ?? ''), since keys from Object.entries are
always strings; update the entries.map call that builds createPrefRow (the arrow
callback using [key, value]) to pass the existing key directly (or at most
String(key)) and keep the value coercion as String(value ?? ''), e.g. replace
the first argument to createPrefRow with key instead of String(key ?? '') so
locate the entries.map callback and change its first parameter usage
accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/helpers/prefs.tssrc/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte
2 participants
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit