fix(browser-extension): API login error when server address includes /api/v1 suffix#2630
Open
xingzihai wants to merge 6 commits intokarakeep-app:mainfrom
Open
fix(browser-extension): API login error when server address includes /api/v1 suffix#2630xingzihai wants to merge 6 commits intokarakeep-app:mainfrom
xingzihai wants to merge 6 commits intokarakeep-app:mainfrom
Conversation
Fixes karakeep-app#2622 - Karakeep API login error Users were experiencing "Unexpected non-whitespace character after JSON at position 4" errors when entering server addresses with /api/v1 or /api suffixes. Changes: - Add normalizeServerAddress() to strip common API path suffixes from server address - Add customFetch() to detect non-JSON responses and provide better error messages - Add getErrorMessage() to convert tRPC errors to user-friendly messages - Show warning when server address is automatically corrected - Add example hint for server address input field Root cause: Users entering server address as "http://localhost:4000/api/v1" instead of just "http://localhost:4000", causing the tRPC client to call non-existent endpoints and receive HTML error pages instead of JSON.
…ngs schema - Add URL scheme and format validation to normalizeServerAddress() - Add HTTP/HTTPS URL validation to zSettingsSchema for address field - Throws error for invalid URLs to prevent security bypass
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdd server-address normalization/validation and a user-warning flow in the NotConfigured page; introduce URL normalization util; add a customFetch and error-formatting helpers for tRPC; adjust settings schema to normalize stored address; update pages to use the new error formatting. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 SummaryThis PR fixes the browser extension login failure that occurred when users entered a server address with a Key changes and issues found:
Confidence Score: 4/5Safe to merge after fixing the dead warning (never shown) and the uncaught exception in onSave. Two P1 issues exist in NotConfiguredPage.tsx: the warning is immediately overwritten by navigation and never seen by the user, and normalizeServerAddress() can throw uncaught for certain valid-looking inputs. These are functional bugs in the core user flow this PR targets. apps/browser-extension/src/NotConfiguredPage.tsx requires the most attention (warning visibility + uncaught throw). apps/browser-extension/src/utils/settings.ts warrants a second look for the Zod refine side-effects on stored data. Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: apps/browser-extension/src/NotConfiguredPage.tsx
Line: 41-53
Comment:
**Warning is never visible to the user**
`setWarning(...)` is called but `navigate("/signin")` runs unconditionally on the very next line. The component unmounts before React can render the warning message, so the user never sees it. The correction is silently applied without any feedback.
To fix this, the navigation should be deferred when an address correction occurs (e.g., only navigate after the user acknowledges the warning by pressing "Configure" a second time), or the warning approach should be dropped in favour of simply updating the input field value to the normalized address so the user can review it before confirming:
```suggestion
// Normalize the address by stripping /api/v1 or /api suffixes
const normalizedAddress = normalizeServerAddress(input);
// If the address was normalized, update the input and show a warning
// instead of navigating away, so the user can see the correction.
if (normalizedAddress !== input) {
setWarning(
`Address was automatically corrected from "${input}" to "${normalizedAddress}". ` +
`The server address should be the base URL without /api/v1 or /api suffix.`,
);
setServerAddress(normalizedAddress);
return;
}
setSettings((s) => ({ ...s, address: normalizedAddress }));
navigate("/signin");
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/browser-extension/src/NotConfiguredPage.tsx
Line: 42
Comment:
**Uncaught exception from `normalizeServerAddress`**
`normalizeServerAddress` can throw if `new URL(normalized)` fails — for example when the user types `http://` (empty host). This input passes the `isHttpUrl` guard above (it starts with `"http://"`), but `new URL("http://")` throws a `TypeError: Invalid URL` that propagates unhandled from `onSave`, crashing the handler silently.
Wrap the call in a `try/catch` and surface the error to the user:
```suggestion
let normalizedAddress: string;
try {
normalizedAddress = normalizeServerAddress(input);
} catch (e) {
setError(e instanceof Error ? e.message : "Invalid server address");
return;
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/browser-extension/src/utils/trpc.ts
Line: 53-56
Comment:
**`customFetch` doubles the error message for HTML responses**
When `customFetch` throws `"Server returned an HTML error page instead of JSON. <SERVER_ADDRESS_ERROR_MESSAGE>"`, the error propagates as a `TRPCClientError`. In `getErrorMessage`, `isJsonParseError(error.cause)` returns `false` (the cause message contains "Server returned…" not "unexpected…json/position"), so the function falls through to `return error.message`, which includes the full duplicated sentence.
The user would see: `"Server returned an HTML error page instead of JSON. Unable to connect to the Karakeep server…"` — information that is stated twice in different ways.
Consider either:
- Having `customFetch` throw with only `SERVER_ADDRESS_ERROR_MESSAGE`, or
- Having `getErrorMessage` check whether `error.message` already contains the well-known server error message before returning it.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/browser-extension/src/utils/settings.ts
Line: 12-19
Comment:
**Zod `refine` may silently reset settings for edge-case stored values**
The new `.refine(isHttpUrl, …)` is added to the `address` field. If any user has an address stored in Chrome sync storage that doesn't pass `isHttpUrl` (e.g., an empty string from a previous bug, or a value written outside the normal UI flow), `zSettingsSchema.safeParse` will now fail for those users.
Looking at `getPluginSettings`, a `safeParse` failure returns `DEFAULT_SETTINGS` (which has `apiKey: ""`), effectively logging those users out and resetting all their settings. Given the extension previously accepted _any_ string for `address`, this is a silent breaking change for an existing persisted state edge case.
A safer approach is to keep the schema permissive for storage (since validation and normalization are already enforced on input) and only apply `refine` at the point of user input, or to apply `.catch("https://cloud.karakeep.app")` on the field so an invalid value falls back only for that field rather than invalidating the whole settings object.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "style: add trailing newlines to modified..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
41
to
53
| // Normalize the address by stripping /api/v1 or /api suffixes | ||
| const normalizedAddress = normalizeServerAddress(input); | ||
|
|
||
| // Show a warning if the address was normalized | ||
| if (normalizedAddress !== input) { | ||
| setWarning( | ||
| `Address was automatically corrected from "${input}" to "${normalizedAddress}". ` + | ||
| `The server address should be the base URL without /api/v1 or /api suffix.`, | ||
| ); | ||
| } | ||
|
|
||
| setSettings((s) => ({ ...s, address: normalizedAddress })); | ||
| navigate("/signin"); |
There was a problem hiding this comment.
setWarning(...) is called but navigate("/signin") runs unconditionally on the very next line. The component unmounts before React can render the warning message, so the user never sees it. The correction is silently applied without any feedback.
To fix this, the navigation should be deferred when an address correction occurs (e.g., only navigate after the user acknowledges the warning by pressing "Configure" a second time), or the warning approach should be dropped in favour of simply updating the input field value to the normalized address so the user can review it before confirming:
Suggested change
| // Normalize the address by stripping /api/v1 or /api suffixes | |
| const normalizedAddress = normalizeServerAddress(input); | |
| // Show a warning if the address was normalized | |
| if (normalizedAddress !== input) { | |
| setWarning( | |
| `Address was automatically corrected from "${input}" to "${normalizedAddress}". ` + | |
| `The server address should be the base URL without /api/v1 or /api suffix.`, | |
| ); | |
| } | |
| setSettings((s) => ({ ...s, address: normalizedAddress })); | |
| navigate("/signin"); | |
| // Normalize the address by stripping /api/v1 or /api suffixes | |
| const normalizedAddress = normalizeServerAddress(input); | |
| // If the address was normalized, update the input and show a warning | |
| // instead of navigating away, so the user can see the correction. | |
| if (normalizedAddress !== input) { | |
| setWarning( | |
| `Address was automatically corrected from "${input}" to "${normalizedAddress}". ` + | |
| `The server address should be the base URL without /api/v1 or /api suffix.`, | |
| ); | |
| setServerAddress(normalizedAddress); | |
| return; | |
| } | |
| setSettings((s) => ({ ...s, address: normalizedAddress })); | |
| navigate("/signin"); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/browser-extension/src/NotConfiguredPage.tsx
Line: 41-53
Comment:
**Warning is never visible to the user**
`setWarning(...)` is called but `navigate("/signin")` runs unconditionally on the very next line. The component unmounts before React can render the warning message, so the user never sees it. The correction is silently applied without any feedback.
To fix this, the navigation should be deferred when an address correction occurs (e.g., only navigate after the user acknowledges the warning by pressing "Configure" a second time), or the warning approach should be dropped in favour of simply updating the input field value to the normalized address so the user can review it before confirming:
```suggestion
// Normalize the address by stripping /api/v1 or /api suffixes
const normalizedAddress = normalizeServerAddress(input);
// If the address was normalized, update the input and show a warning
// instead of navigating away, so the user can see the correction.
if (normalizedAddress !== input) {
setWarning(
`Address was automatically corrected from "${input}" to "${normalizedAddress}". ` +
`The server address should be the base URL without /api/v1 or /api suffix.`,
);
setServerAddress(normalizedAddress);
return;
}
setSettings((s) => ({ ...s, address: normalizedAddress }));
navigate("/signin");
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| setSettings((s) => ({ ...s, address: input.replace(/\/$/, "") })); | ||
| // Normalize the address by stripping /api/v1 or /api suffixes | ||
| const normalizedAddress = normalizeServerAddress(input); |
There was a problem hiding this comment.
normalizeServerAddress
normalizeServerAddress can throw if new URL(normalized) fails — for example when the user types http:// (empty host). This input passes the isHttpUrl guard above (it starts with "http://"), but new URL("http://") throws a TypeError: Invalid URL that propagates unhandled from onSave, crashing the handler silently.
Wrap the call in a try/catch and surface the error to the user:
Suggested change
| const normalizedAddress = normalizeServerAddress(input); | |
| let normalizedAddress: string; | |
| try { | |
| normalizedAddress = normalizeServerAddress(input); | |
| } catch (e) { | |
| setError(e instanceof Error ? e.message : "Invalid server address"); | |
| return; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/browser-extension/src/NotConfiguredPage.tsx
Line: 42
Comment:
**Uncaught exception from `normalizeServerAddress`**
`normalizeServerAddress` can throw if `new URL(normalized)` fails — for example when the user types `http://` (empty host). This input passes the `isHttpUrl` guard above (it starts with `"http://"`), but `new URL("http://")` throws a `TypeError: Invalid URL` that propagates unhandled from `onSave`, crashing the handler silently.
Wrap the call in a `try/catch` and surface the error to the user:
```suggestion
let normalizedAddress: string;
try {
normalizedAddress = normalizeServerAddress(input);
} catch (e) {
setError(e instanceof Error ? e.message : "Invalid server address");
return;
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+53
to
+56
| throw new Error( | ||
| `Server returned an unexpected response: "${preview}...". ${SERVER_ADDRESS_ERROR_MESSAGE}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
customFetch doubles the error message for HTML responses
When customFetch throws "Server returned an HTML error page instead of JSON. <SERVER_ADDRESS_ERROR_MESSAGE>", the error propagates as a TRPCClientError. In getErrorMessage, isJsonParseError(error.cause) returns false (the cause message contains "Server returned…" not "unexpected…json/position"), so the function falls through to return error.message, which includes the full duplicated sentence.
The user would see: "Server returned an HTML error page instead of JSON. Unable to connect to the Karakeep server…" — information that is stated twice in different ways.
Consider either:
- Having
customFetchthrow with onlySERVER_ADDRESS_ERROR_MESSAGE, or - Having
getErrorMessagecheck whethererror.messagealready contains the well-known server error message before returning it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/browser-extension/src/utils/trpc.ts
Line: 53-56
Comment:
**`customFetch` doubles the error message for HTML responses**
When `customFetch` throws `"Server returned an HTML error page instead of JSON. <SERVER_ADDRESS_ERROR_MESSAGE>"`, the error propagates as a `TRPCClientError`. In `getErrorMessage`, `isJsonParseError(error.cause)` returns `false` (the cause message contains "Server returned…" not "unexpected…json/position"), so the function falls through to `return error.message`, which includes the full duplicated sentence.
The user would see: `"Server returned an HTML error page instead of JSON. Unable to connect to the Karakeep server…"` — information that is stated twice in different ways.
Consider either:
- Having `customFetch` throw with only `SERVER_ADDRESS_ERROR_MESSAGE`, or
- Having `getErrorMessage` check whether `error.message` already contains the well-known server error message before returning it.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+12
to
+19
| address: z | ||
| .string() | ||
| .refine( | ||
| (val) => isHttpUrl(val), | ||
| "Server address must be a valid HTTP or HTTPS URL", | ||
| ) | ||
| .optional() | ||
| .default("https://cloud.karakeep.app"), |
There was a problem hiding this comment.
refine may silently reset settings for edge-case stored values
The new .refine(isHttpUrl, …) is added to the address field. If any user has an address stored in Chrome sync storage that doesn't pass isHttpUrl (e.g., an empty string from a previous bug, or a value written outside the normal UI flow), zSettingsSchema.safeParse will now fail for those users.
Looking at getPluginSettings, a safeParse failure returns DEFAULT_SETTINGS (which has apiKey: ""), effectively logging those users out and resetting all their settings. Given the extension previously accepted any string for address, this is a silent breaking change for an existing persisted state edge case.
A safer approach is to keep the schema permissive for storage (since validation and normalization are already enforced on input) and only apply refine at the point of user input, or to apply .catch("https://cloud.karakeep.app") on the field so an invalid value falls back only for that field rather than invalidating the whole settings object.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/browser-extension/src/utils/settings.ts
Line: 12-19
Comment:
**Zod `refine` may silently reset settings for edge-case stored values**
The new `.refine(isHttpUrl, …)` is added to the `address` field. If any user has an address stored in Chrome sync storage that doesn't pass `isHttpUrl` (e.g., an empty string from a previous bug, or a value written outside the normal UI flow), `zSettingsSchema.safeParse` will now fail for those users.
Looking at `getPluginSettings`, a `safeParse` failure returns `DEFAULT_SETTINGS` (which has `apiKey: ""`), effectively logging those users out and resetting all their settings. Given the extension previously accepted _any_ string for `address`, this is a silent breaking change for an existing persisted state edge case.
A safer approach is to keep the schema permissive for storage (since validation and normalization are already enforced on input) and only apply `refine` at the point of user input, or to apply `.catch("https://cloud.karakeep.app")` on the field so an invalid value falls back only for that field rather than invalidating the whole settings object.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
apps/browser-extension/src/utils/trpc.ts (1)
53-55: Avoid exposing raw response previews in user-facing errors.Line 54 includes server response snippets directly in the error message. Prefer a sanitized message for UI and keep raw payload details only for internal/debug logging.
Proposed fix
- throw new Error( - `Server returned an unexpected response: "${preview}...". ${SERVER_ADDRESS_ERROR_MESSAGE}`, - ); + throw new Error( + `Server returned an unexpected non-JSON response. ${SERVER_ADDRESS_ERROR_MESSAGE}`, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-extension/src/utils/trpc.ts` around lines 53 - 55, The thrown Error in utils/trpc.ts embeds the raw `preview` response into a user-facing message; remove the raw snippet from the Error text and replace it with a sanitized, generic message (e.g., "Server returned an unexpected response.") combined with the existing `SERVER_ADDRESS_ERROR_MESSAGE`; meanwhile send the raw `preview` to an internal debug log (e.g., `console.debug` or your app logger) before throwing so developers can inspect it without exposing it to users. Target the throw site that references `preview` and `SERVER_ADDRESS_ERROR_MESSAGE` and ensure no other user-facing errors include un-sanitized payloads.apps/browser-extension/src/NotConfiguredPage.tsx (1)
30-30: Prefer strict equality===over loose equality==.TypeScript best practice is to use strict equality comparison.
✨ Suggested change
- if (input == "") { + if (input === "") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-extension/src/NotConfiguredPage.tsx` at line 30, In NotConfiguredPage.tsx update the loose equality check in the conditional that reads "if (input == "")" to use strict equality instead; locate the conditional inside the NotConfiguredPage component (the input validation branch) and change the comparison operator from == to === so the check becomes a strict comparison against the empty string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/browser-extension/src/NotConfiguredPage.tsx`:
- Around line 41-53: The current flow normalizes the server address
(normalizeServerAddress/input -> normalizedAddress), sets a warning via
setWarning, then immediately calls navigate("/signin"), which prevents the user
from seeing the warning; change the behavior so navigation is not automatic when
an address normalization occurs: when normalizedAddress !== input, setWarning
and update settings (setSettings) but DO NOT call navigate("/signin"); instead
require explicit user confirmation (e.g., a "Continue" button in the UI that
calls navigate("/signin") when clicked) so the user can read the warning before
proceeding; keep automatic navigation only when no normalization occurred.
- Line 42: normalizeServerAddress(input) can throw even when isHttpUrl(input)
previously passed; wrap the call to normalizeServerAddress inside a try-catch in
NotConfiguredPage.tsx (where normalizedAddress is assigned) to catch and handle
errors. On catch, display a user-friendly validation error or set the component
state to mark the input invalid (same flow as isHttpUrl failure), and avoid
letting the exception bubble; ensure you reference normalizeServerAddress and
isHttpUrl so the input validation path consistently handles invalid URL formats.
In `@apps/browser-extension/src/utils/settings.ts`:
- Around line 12-19: The address field's new .refine(isHttpUrl) causes safeParse
failures and silent fallback to DEFAULT_SETTINGS in getPluginSettings() and
subscribeToSettingsChanges(); update settings.ts to handle legacy bare-host
addresses by (1) attempting a migration: if the stored address fails isHttpUrl,
try prepending "https://" and then "http://" and accept the first valid URL, (2)
if migration fails, log a clear error via the same logger used in
getPluginSettings()/subscribeToSettingsChanges() and preserve the original
stored value (or surface a user-visible warning) instead of silently replacing
it with DEFAULT_SETTINGS, and (3) ensure the schema validation still uses the
refined check but that these migration/logging steps run before deciding to
fallback so users with self-hosted hosts like "localhost:5000" or "example.com"
are handled gracefully.
In `@apps/browser-extension/src/utils/trpc.ts`:
- Around line 33-37: The content-type header check uses a case-sensitive
substring test and can miss JSON when header casing differs; normalize the
header once by taking the contentType string (the local variable `contentType`)
and converting it to lower-case (e.g., `contentType = contentType.toLowerCase()`
or use a lower-cased copy) before testing `.includes("application/json")` in the
`response` handling block so the condition `!response.ok && (!contentType ||
!contentType.includes("application/json"))` correctly detects JSON responses
regardless of header casing.
In `@apps/browser-extension/src/utils/url.ts`:
- Around line 37-43: The scheme check is case-sensitive and can disagree with
isHttpUrl(); fix by performing the same case-insensitive check used in
isHttpUrl(): convert the URL string (the variable normalized) to lowercase
before testing startsWith("http://") / startsWith("https://"), or reuse/call
isHttpUrl(normalized) here instead of manual startsWith checks so inputs like
"HTTP://localhost:3000" won't erroneously throw in this block.
---
Nitpick comments:
In `@apps/browser-extension/src/NotConfiguredPage.tsx`:
- Line 30: In NotConfiguredPage.tsx update the loose equality check in the
conditional that reads "if (input == "")" to use strict equality instead; locate
the conditional inside the NotConfiguredPage component (the input validation
branch) and change the comparison operator from == to === so the check becomes a
strict comparison against the empty string.
In `@apps/browser-extension/src/utils/trpc.ts`:
- Around line 53-55: The thrown Error in utils/trpc.ts embeds the raw `preview`
response into a user-facing message; remove the raw snippet from the Error text
and replace it with a sanitized, generic message (e.g., "Server returned an
unexpected response.") combined with the existing
`SERVER_ADDRESS_ERROR_MESSAGE`; meanwhile send the raw `preview` to an internal
debug log (e.g., `console.debug` or your app logger) before throwing so
developers can inspect it without exposing it to users. Target the throw site
that references `preview` and `SERVER_ADDRESS_ERROR_MESSAGE` and ensure no other
user-facing errors include un-sanitized payloads.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be403cc3-df72-430c-a58b-0f8bee11a1e4
📒 Files selected for processing (6)
apps/browser-extension/src/NotConfiguredPage.tsxapps/browser-extension/src/OptionsPage.tsxapps/browser-extension/src/SignInPage.tsxapps/browser-extension/src/utils/settings.tsapps/browser-extension/src/utils/trpc.tsapps/browser-extension/src/utils/url.ts
… in NotConfiguredPage - Fix warning never shown: when address is normalized, update input field and show warning instead of navigating away immediately - Add try/catch around normalizeServerAddress to handle potential exceptions - Let user review correction before confirming
- Normalize content-type header to lowercase for consistent JSON detection - Simplify error messages to avoid duplication - Fix URL scheme validation to be case-insensitive (consistent with isHttpUrl) - Use transform instead of refine for address validation to prevent silent settings reset
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/browser-extension/src/utils/trpc.ts (1)
66-75: Consider tightening the JSON parse error heuristic.The check for
"position"without"json"(line 71) is broad and could match unrelated errors (e.g., "Unexpected value at position 5 in array"). This may cause false positives where users see server address guidance for unrelated issues.Suggested refinement
export function isJsonParseError(error: unknown): boolean { if (error instanceof Error) { const message = error.message.toLowerCase(); return ( - message.includes("unexpected") && - (message.includes("json") || message.includes("position")) + message.includes("json") && + (message.includes("unexpected") || message.includes("parse")) ); } return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-extension/src/utils/trpc.ts` around lines 66 - 75, The current isJsonParseError(error: unknown) heuristic is too broad because matching "position" alone can produce false positives; update isJsonParseError to only return true for clearer JSON parse indicators (e.g., require "unexpected token", "unexpected", "invalid character", or the presence of both "position" and "json"/"token"), or explicitly check for phrases like "unexpected token" and "invalid character" in the error.message (still normalizing to lowercase) so unrelated errors containing "position" no longer trigger the JSON parse guidance; modify the logic inside isJsonParseError to tighten these checks accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/browser-extension/src/utils/settings.ts`:
- Around line 12-21: The address transform in the Zod schema (the address
field's z.string().transform) only validates but leaves full URLs like
https://host/api/v1 intact; update the transform to normalize and persist only
the origin/base URL: if isHttpUrl(val) parse it with URL (catch errors), then
return url.origin (so protocol+host[:port] only) after trimming whitespace; if
parsing fails or val is falsy, return the default "https://cloud.karakeep.app".
Keep the existing isHttpUrl check and default fallback but replace returning val
with the normalized origin to ensure persisted settings never contain path
components.
---
Nitpick comments:
In `@apps/browser-extension/src/utils/trpc.ts`:
- Around line 66-75: The current isJsonParseError(error: unknown) heuristic is
too broad because matching "position" alone can produce false positives; update
isJsonParseError to only return true for clearer JSON parse indicators (e.g.,
require "unexpected token", "unexpected", "invalid character", or the presence
of both "position" and "json"/"token"), or explicitly check for phrases like
"unexpected token" and "invalid character" in the error.message (still
normalizing to lowercase) so unrelated errors containing "position" no longer
trigger the JSON parse guidance; modify the logic inside isJsonParseError to
tighten these checks accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 471b6d31-5ab6-4df5-b1cd-dfc19555baba
📒 Files selected for processing (3)
apps/browser-extension/src/utils/settings.tsapps/browser-extension/src/utils/trpc.tsapps/browser-extension/src/utils/url.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/browser-extension/src/utils/url.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2622
Summary
Fix API login error when server address includes /api/v1 suffix.
Changes
Testing