feat: tolgee platform client #3201
Conversation
WalkthroughAdds a GitHub Actions workflow to publish the client and creates a new TypeScript client package (@tginternal/client) with build/test tooling, OpenAPI-based API client, schema utilities, error/conflict helpers, versioning script, tests, and related config and artifacts. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Repo as Repo (client/)
participant BE as Backend (Docker)
participant NPM as npm Registry
GH->>Repo: checkout + setup Node + npm ci
GH->>Repo: run release-dry -> produce .VERSION (optional)
GH->>BE: start backend (./gradlew runDockerE2e -x buildWebapp)
GH->>Repo: cd client -> npm install -> npm run schema
alt VERSION present (release) and not already published
GH->>Repo: update-version -> build -> test
GH->>NPM: npm publish --tag latest
else prerelease (computed) and not published
GH->>Repo: update-version (prerelease) -> build -> test
GH->>NPM: npm publish --tag prerelease
else already published
GH->>Repo: skip publish
end
sequenceDiagram
participant App as Consumer
participant Client as ApiClient
participant API as Tolgee API
App->>Client: createApiClient({ baseUrl, apiKey?, userToken? })
App->>Client: getApiKeyInformation(key) / login(body)
Client->>API: GET/POST (adds Authorization / ApiKey, User-Agent)
API-->>Client: response (data | error)
alt success
Client-->>App: return typed data
else error
Client->>Client: build message via errorFromLoadable / getUnresolvedConflictsMessage
Client-->>App: throw or return error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🔭 Outside diff range comments (1)
client/scripts/updateVersion.js (1)
1-23: Add error handling and input validation to the version update script.The script lacks error handling for file operations and version validation, which could lead to cryptic failures or silent errors during the CI/CD process.
Apply comprehensive error handling and validation:
import fs from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; -const packageJson = fs.readFileSync("./package.json"); +// Get directory of current module +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const packageJsonPath = path.join(__dirname, "..", "package.json"); +const versionTsPath = path.join(__dirname, "..", "src", "version.ts"); -const version = String(process.argv[2]).replace(/^v/, ""); +// Validate version argument +const versionArg = process.argv[2]; +if (!versionArg) { + console.error("Error: Version argument is required"); + console.error("Usage: node updateVersion.js <version>"); + process.exit(1); +} -console.log({ version }); +const version = String(versionArg).replace(/^v/, ""); + +// Basic semver/prerelease validation +const versionPattern = /^(\d+\.\d+\.\d+(-[a-zA-Z0-9.-]+)?|prerelease\.[a-f0-9]+)$/; +if (!versionPattern.test(version)) { + console.error(`Error: Invalid version format: ${version}`); + console.error("Expected format: x.y.z, x.y.z-tag, or prerelease.hash"); + process.exit(1); +} + +console.log(`Updating version to: ${version}`); + +try { + const packageJson = fs.readFileSync(packageJsonPath, "utf8"); const replaced = packageJson - .toString() .replaceAll(/"version": ".*"/g, `"version": "${version}"`); -fs.writeFileSync("./package.json", replaced); + fs.writeFileSync(packageJsonPath, replaced); + console.log(`✓ Updated ${packageJsonPath}`); const content = ` /** * This file is generated automatically by \`updateVersion.js\` script */ export const VERSION = "${version}"; `.trimStart(); -fs.writeFileSync("./src/version.ts", content); + fs.writeFileSync(versionTsPath, content); + console.log(`✓ Generated ${versionTsPath}`); +} catch (error) { + console.error(`Error updating version: ${error.message}`); + process.exit(1); +}
♻️ Duplicate comments (1)
.github/workflows/publish-client.yml (1)
35-37: Publishing will fail due to private package configuration.The workflow attempts to publish the package, but client/package.json has
"private": true, which prevents npm from publishing. This mismatch will cause the workflow to fail.
🧹 Nitpick comments (21)
client/src/version.ts (1)
1-4: Strengthen generated-file header to prevent manual editsRecommend clarifying the source and adding a "do not edit" notice to avoid accidental manual changes.
/** - * This file is generated automatically by `updateVersion.js` script + * This file is generated automatically by client/scripts/updateVersion.js. + * Do not edit this file manually – run the update-version script instead. */client/tsconfig.json (2)
2-27: Emit declaration maps to improve consumer DX and debuggingIncluding .d.ts.map helps consumers navigate types in editors and improves stack traces with source maps in many IDEs.
"declaration": true, + "declarationMap": true, "emitDeclarationOnly": true,
6-7: Sanity check: Do you really need DOM types in a Node-targeted CLI?Including DOM and DOM.Iterable can pollute the global type space and mask Node-vs-web API differences. If this package is CLI-only, consider dropping DOM libs and explicitly adding Node types via types: ["node"].
client/.gitignore (1)
14-16: Ignore additional common build/test artifactsMinor hardening to keep the repo clean across tools and CI.
*.local + +# Build/test artifacts +coverage +.nyc_output +*.tsbuildinfo +.eslintcache +.turbo.github/workflows/test.yml (1)
55-55: String quotation change is functionally equivalent but inconsistent with workflow patterns.While changing from single quotes to double quotes doesn't affect functionality (both result in the string "false"), this creates inconsistency within the workflow file where other string values and boolean-like strings typically use single quotes in GitHub Actions workflows.
Consider reverting to single quotes for consistency with typical GitHub Actions patterns:
- node: "false" + node: 'false'- java: "false" + java: 'false'Also applies to: 205-205, 254-254, 277-277, 308-308
client/src/schema.utils.ts (1)
26-28: Consider documenting why the "ak" parameter is excluded from QueryOf.The QueryOf type specifically excludes the "ak" parameter, which appears to be an API key parameter. This business logic embedded in the type utility should be documented for future maintainers.
Add a comment explaining the exclusion:
export type QueryOf< P extends Path, M extends MethodsOf<P> + // Excludes the "ak" (API key) parameter as it's handled separately by the client > = paths[P][M] extends { parameters: { query?: Record<string, any> } } ? Omit<Exclude<paths[P][M]["parameters"]["query"], undefined>, "ak"> : void;client/src/main.ts (1)
1-5: Consider re-exporting LoadableData and handleLoadableError if they are part of the intended public APITolgeeClient.ts appears to expose LoadableData and handleLoadableError; if consumers are expected to use these, re-export them here to keep a single, cohesive entry point.
Proposed change:
export { type TolgeeClient, createTolgeeClient, type TolgeeClientProps, + type LoadableData, + handleLoadableError, } from "./TolgeeClient";client/src/constants.ts (1)
1-6: USER_AGENT label likely mismatched with the package identityThe constant currently brands requests as Tolgee-CLI while this package is the client. If this module is shipped as @tolgee/client, consider aligning the UA string (and the link) to the client package to avoid confusion in logs/analytics.
Proposed change:
-export const USER_AGENT = `Tolgee-CLI/${VERSION} (+https://github.com/tolgee/tolgee-cli)`; +export const USER_AGENT = `Tolgee-Client/${VERSION} (+https://github.com/tolgee/tolgee-client)`;Follow-up:
- If this module is shared with the CLI and intentionally uses CLI branding, ignore this suggestion.
- Ensure the code that applies USER_AGENT only does so in Node runtimes. Browsers do not allow setting the User-Agent header; attempting to set it can be silently ignored or cause issues. If needed, gate setting the header behind a runtime check.
client/src/errorFromLoadable.ts (2)
29-33: Comment label mismatch for 400The comment says Unauthorized for status 400; that’s Bad Request.
- // Unauthorized + // Bad Request case 400: return `Invalid request data ${addErrorDetails(loadable)}`;
63-65: Optional: Handle common statuses 404 and 422 for clearer messagingConsider adding explicit handling for Not Found (404) and Unprocessable Entity (422) to improve actionable feedback.
Example:
// Server error case 500: return `API reported a server error. Please try again later ${addErrorDetails( loadable )}`; + // Not Found + case 404: + return `Requested resource was not found ${addErrorDetails(loadable)}`; + + // Unprocessable Entity + case 422: + return `Request could not be processed ${addErrorDetails(loadable)}`;client/src/ExportClient.ts (2)
20-27: Optional: Remove redundant variable and tighten typing via ApiClient genericsMinor cleanup and stronger typing:
- Use body shorthand.
- If ApiClient supports a generic tied to parseAs, you can avoid manual casting altogether.
Potential refactor:
- async export(req: ExportRequest) { - const body = { ...req, zip: true }; - const loadable = await apiClient.POST("/v2/projects/{projectId}/export", { - params: { path: { projectId: apiClient.getProjectId() } }, - body: body, - parseAs: "blob", - }); - return { ...loadable, data: loadable.data as unknown as Blob }; - }, + async export(req: ExportRequest) { + const loadable = await apiClient.POST("/v2/projects/{projectId}/export", { + params: { path: { projectId: apiClient.getProjectId() } }, + body: { ...req, zip: true }, + parseAs: "blob", + }); + return { ...loadable, data: loadable.data as Blob | undefined }; + },If ApiClient.POST can be declared like POST<"blob">(...): { data: Blob | undefined; ... }, that would eliminate the cast entirely.
20-36: Method name shadowing the reserved word ‘export’ can be confusingUsing export as a method name is valid, but can be confusing when imported elsewhere and when destructuring. Consider a more descriptive name like exportArchive or exportZip to clarify behavior and avoid cognitive clash with the JS export keyword.
Example:
- async export(req: ExportRequest) { + async exportArchive(req: ExportRequest) { @@ - async exportSingle(req: SingleExportRequest) { + async exportSingle(req: SingleExportRequest) { return apiClient.POST("/v2/projects/{projectId}/export", { params: { path: { projectId: apiClient.getProjectId() } }, body: { ...req, zip: false }, }); },client/src/ImportClient.ts (1)
10-13: Widen File.data type to portable Blob parts.To support both Node and browser uniformly, accept the standard BlobPart union rather than a narrow set with Node’s Buffer. This avoids DOM/Node type friction.
Apply this diff:
-export type File = { name: string; data: string | Buffer | Blob }; +export type File = { + name: string; + // BlobPart = string | Blob | BufferSource (Uint8Array/ArrayBuffer) + data: string | Blob | ArrayBuffer | Uint8Array | Buffer; +};client/src/printFailedKeys.ts (2)
8-12: Simplify nested template string.The nested template adds noise. It can be simplified without changing output.
Apply this diff:
- const namespace = key.namespace ? ` ${`(namespace: ${key.namespace})`}` : ""; + const namespace = key.namespace ? ` (namespace: ${key.namespace})` : "";
18-36: Avoid leading/trailing blank lines and use Array.some().Starting the message with an empty string forces a leading newline; likewise, extra pushes add trailing newlines. Also,
some()is clearer thanfind()for a boolean.Apply this diff:
- const someOverridable = Boolean(translations.find((c) => c.isOverridable)); - const result = [""]; + const someOverridable = translations.some((c) => c.isOverridable); + const result: string[] = []; @@ - result.push(""); + // spacer before hint when present @@ - result.push(""); + // trailing newline not necessary; caller can add if desiredclient/src/ApiClient.ts (3)
87-98: Minor: avoid duplicate header lookups.Reuse the
apiVersionvariable to prevent two header reads.Apply this diff:
- const apiVersion = response.headers.get("x-tolgee-version"); - if (apiVersion) { - responseText += ` [${response.headers.get("x-tolgee-version")}]`; - } + const apiVersion = response.headers.get("x-tolgee-version"); + if (apiVersion) { + responseText += ` [${apiVersion}]`; + }
9-16: parseResponse signature should accept undefined parseAs; currently typed too narrowly.
options.parseAsmay be undefined. Widen the parameter type to avoid type errors. No runtime change.Apply this diff:
-async function parseResponse(response: Response, parseAs: ParseAs) { +async function parseResponse(response: Response, parseAs?: ParseAs) { @@ - if (parseAs === "stream") { + if (parseAs === "stream") { return { data: response.body, response }; } return { data: await response[parseAs](), response };Also applies to: 28-36, 101-106
119-121: Include verbose in getSettings to reflect full configuration.Minor completeness nit.
Apply this diff:
- getSettings(): ApiClientProps { - return { baseUrl, apiKey, projectId, autoThrow }; - }, + getSettings(): ApiClientProps { + return { baseUrl, apiKey, projectId, autoThrow, verbose }; + },client/src/getApiKeyInformation.ts (3)
1-1: Import openapi-fetch type-only to avoid unnecessary runtime dependency.This import is used only for typing; make it type-only so TS erases it at emit.
Apply this diff:
-import createClient from "openapi-fetch"; +import type createClient from "openapi-fetch";
5-5: Break the ApiClient ↔ TolgeeClient ↔ getApiKeyInformation import cycle.Importing
handleLoadableErrorfrom TolgeeClient creates a runtime cycle: ApiClient -> getApiKeyInformation -> TolgeeClient -> ApiClient. Move/exporthandleLoadableErrorfromerrorFromLoadable.ts(pure utility) and import it here to eliminate the cycle.Apply this diff:
-import { handleLoadableError } from "./TolgeeClient.js"; +import { handleLoadableError } from "./errorFromLoadable";Then add the
handleLoadableErrorexport to errorFromLoadable.ts (see TolgeeClient comment for snippet). Keep import paths consistent (avoid mixing “.js” suffix in TS sources unless using bundler moduleResolution).
58-77: LGTM for PAT branch; consider guarding for unexpected schema changes.Accessing
info.user.name || info.user.usernameassumesuseris always present. If the schema evolves, a nullish coalescing with a fallback (like "") would be safer, but this is acceptable if the schema is stable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
.github/workflows/publish-client.yml(1 hunks).github/workflows/test.yml(5 hunks)client/.gitignore(1 hunks)client/package.json(1 hunks)client/scripts/updateVersion.js(1 hunks)client/src/ApiClient.ts(1 hunks)client/src/ExportClient.ts(1 hunks)client/src/ImportClient.ts(1 hunks)client/src/TolgeeClient.ts(1 hunks)client/src/constants.ts(1 hunks)client/src/errorFromLoadable.ts(1 hunks)client/src/getApiKeyInformation.ts(1 hunks)client/src/main.ts(1 hunks)client/src/pathToPosix.ts(1 hunks)client/src/printFailedKeys.ts(1 hunks)client/src/schema.utils.ts(1 hunks)client/src/version.ts(1 hunks)client/src/vite-env.d.ts(1 hunks)client/tsconfig.json(1 hunks)client/vite.config.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
client/src/ExportClient.ts (2)
client/src/schema.utils.ts (1)
BodyOf(30-35)client/src/ApiClient.ts (1)
ApiClient(125-125)
client/src/constants.ts (1)
client/src/version.ts (1)
VERSION(4-4)
client/src/ImportClient.ts (3)
client/src/schema.utils.ts (1)
BodyOf(30-35)client/src/ApiClient.ts (1)
ApiClient(125-125)client/src/pathToPosix.ts (1)
pathToPosix(3-5)
client/src/getApiKeyInformation.ts (3)
client/src/schema.generated.ts (1)
paths(6-5108)client/src/constants.ts (1)
API_KEY_PAK_PREFIX(4-4)client/src/TolgeeClient.ts (1)
handleLoadableError(22-26)
client/src/printFailedKeys.ts (1)
client/src/schema.generated.ts (1)
components(5110-9081)
client/src/errorFromLoadable.ts (1)
client/src/printFailedKeys.ts (1)
getUnresolvedConflictsMessage(14-37)
client/src/ApiClient.ts (4)
client/src/constants.ts (2)
API_KEY_PAK_PREFIX(4-4)USER_AGENT(6-6)client/src/schema.generated.ts (1)
paths(6-5108)client/src/errorFromLoadable.ts (1)
errorFromLoadable(27-66)client/src/getApiKeyInformation.ts (1)
getApiKeyInformation(29-78)
client/src/schema.utils.ts (1)
client/src/schema.generated.ts (1)
paths(6-5108)
🪛 actionlint (1.7.7)
.github/workflows/publish-client.yml
18-18: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: E2E testing 🔎 (15, 14)
- GitHub Check: E2E testing 🔎 (15, 12)
- GitHub Check: E2E testing 🔎 (15, 13)
- GitHub Check: E2E testing 🔎 (15, 5)
- GitHub Check: E2E testing 🔎 (15, 11)
- GitHub Check: E2E testing 🔎 (15, 6)
- GitHub Check: E2E testing 🔎 (15, 8)
- GitHub Check: E2E testing 🔎 (15, 2)
- GitHub Check: E2E testing 🔎 (15, 4)
- GitHub Check: E2E testing 🔎 (15, 3)
- GitHub Check: BT 🔎 (ee-test:test)
- GitHub Check: BT 🔎 (security:test)
- GitHub Check: BT 🔎 (server-app:runContextRecreatingTests)
- GitHub Check: BT 🔎 (ktlint:test)
- GitHub Check: BT 🔎 (data:test)
- GitHub Check: BT 🔎 (server-app:runStandardTests)
- GitHub Check: BT 🔎 (server-app:runWebsocketTests)
- GitHub Check: BT 🔎 (server-app:runWithoutEeTests)
- GitHub Check: Frontend static check 🪲
🔇 Additional comments (8)
client/tsconfig.json (1)
2-27: Verified TypeScript version supports the chosen strict compiler options
- client/package.json: typescript devDependency is ~5.8.3
- client/tsconfig.json enables noUncheckedSideEffectImports and erasableSyntaxOnly (both added in TS 5.5+)
Since ~5.8.3 ≥ 5.5, these options are supported—no changes required.
.github/workflows/publish-client.yml (1)
22-22: Missing NPM_TOKEN secret will cause publishing to fail.The workflow writes NPM_TOKEN to .npmrc but doesn't verify if the secret exists, which will result in an invalid auth token and publishing failure.
Add validation and use GitHub Actions environment variable syntax:
- - run: echo '//registry.npmjs.org/:_authToken=${NPM_TOKEN}' > ~/.npmrc + - name: Configure npm authentication + run: | + if [ -z "${{ secrets.NPM_TOKEN }}" ]; then + echo "::error::NPM_TOKEN secret is not configured" + exit 1 + fi + echo '//registry.npmjs.org/:_authToken=${{ secrets.NPM_TOKEN }}' > ~/.npmrc + env: + NPM_TOKEN: ${{ secrets.NPM_TOKEN }}client/src/schema.utils.ts (1)
1-49: Well-structured type utilities for OpenAPI schema extraction.The type utilities are well-designed, providing clean abstractions for extracting typed information from the generated OpenAPI schema. The conditional types and proper handling of undefined values demonstrate good TypeScript practices.
client/vite.config.js (1)
1-6: Verify ESM config compatibilityvite.config.js uses native ESM syntax. Ensure the workspace/package is configured with "type": "module" (or rename to vite.config.mjs) so Node can load this file without transpilation.
client/src/TolgeeClient.ts (1)
15-19: Nice composition of sub-clients on top of the base ApiClient.Merging the base apiClient with namespaced sub-clients is clean and extensible.
client/src/ImportClient.ts (1)
40-44: ProjectId resolution: consider explicit projectId override or validation.
apiClient.getProjectId()can be undefined with PAT keys (see ApiClient review). Either allow passingprojectIdexplicitly into this call or ensuregetProjectIdthrows with a clear message if not available.Would you like me to add an optional
projectId?: numberparameter toimport()to override when a PAT is used?client/src/ApiClient.ts (1)
77-81: Consider omitting user-agent header in browsers.Browsers forbid setting User-Agent; this will be ignored (or could warn). If this package is intended for browser use, gate the header behind a runtime check.
Do you target browsers with this client? If yes, I can add a small isBrowser guard to skip setting the header.
client/src/getApiKeyInformation.ts (1)
33-57: Good 401 handling and user-friendly messaging for PAK.The error message that includes the API host is helpful. The rest defers to shared error handling, which keeps concerns separated.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (8)
client/src/version.ts (1)
4-4: LGTM. Keep version sync with package.json enforced in CI.The generated constant looks good. Please ensure the version value continues to stay in lockstep with client/package.json via a CI check.
client/src/vite-env.d.ts (1)
1-1: Avoid leaking Vite ambient types from a published libraryReferencing vite/client can leak Vite globals (e.g., import.meta.env) into your published .d.ts and break non-Vite consumers. Since there’s no usage of import.meta.env in client/src, remove this reference or switch to Node types.
Apply one of the following:
Option A (preferred): delete the file.
-/// <reference types="vite/client" />Option B: switch to Node types.
-/// <reference types="vite/client" /> +/// <reference types="node" />client/scripts/updateVersion.js (1)
7-11: Fixed implementation follows JSON-based approach correctly.Good to see the implementation was updated to use JSON parsing and stringification instead of regex replacement, addressing the previous concern about unintended field modifications.
client/vite.config.js (1)
11-11: Library name is now valid for all build formats.The
name: "client"is a valid JavaScript identifier that will work for UMD/IIFE builds, addressing the previous concern about invalid UMD global names.client/src/errorFromLoadable.ts (1)
21-28: Bug: Passing entire params array instead of the conflicts payload to getUnresolvedConflictsMessageYou guard on params[0] being an object, but then pass the whole array. Pass the first element (the conflicts array/object) instead.
Apply this diff:
- additionalInfo += getUnresolvedConflictsMessage( - loadable.error.params as any - ); + additionalInfo += getUnresolvedConflictsMessage( + loadable.error.params[0] as any + );client/src/ApiClient.ts (3)
46-62: Good fix for browser compatibility!The function now correctly uses
TextDecoderinstead of Node'sBuffer, making it browser-compatible. The validation withNumber.isFinite()ensures the function returnsundefinedfor invalid IDs rather thanNaN.
132-140: Good error handling for getProjectId!The method now properly throws a descriptive error when the project ID is unavailable, providing clear guidance to users about how to fix the issue.
141-143: Add guard for missing apiKey in getApiKeyInfo.The method uses a non-null assertion on
apiKey!which will crash at runtime when no API key is configured. Add a guard to throw a clear error message.Apply this diff to add proper error handling:
getApiKeyInfo() { - return getApiKeyInformation(apiClient, apiKey!); + if (!apiKey) { + throw new Error("Cannot retrieve API key info: no apiKey configured."); + } + return getApiKeyInformation(apiClient, apiKey); },
🧹 Nitpick comments (11)
client/tsconfig.json (1)
6-6: Reconsider including DOM libs in a publishable Node/bundler libraryIncluding ["DOM", "DOM.Iterable"] pulls in web types which can conflict for Node consumers. If the library is isomorphic and actually uses DOM types, keep it; otherwise drop them to avoid leaking DOM into declaration output.
- "lib": ["ES2022", "DOM", "DOM.Iterable"], + "lib": ["ES2022"],client/.gitignore (1)
10-14: LGTM: ignoring build artifacts and typesIgnoring dist and lib is appropriate for Git; make sure package.json "files" or an .npmignore includes them for npm publishing.
client/scripts/updateVersion.js (2)
5-5: Add input validation for the version argument.The script doesn't validate that a version argument was provided, which could lead to confusing behavior if called without arguments.
-const version = String(process.argv[2]).replace(/^v/, ""); +const versionArg = process.argv[2]; +if (!versionArg) { + console.error("Error: Version argument is required"); + process.exit(1); +} +const version = String(versionArg).replace(/^v/, "");
3-3: Consider adding error handling for file operations.The script performs several file operations without error handling. If
package.jsondoesn't exist or thesrcdirectory is missing, the script will crash with unclear error messages.-const packageJson = fs.readFileSync("./package.json"); +try { + const packageJson = fs.readFileSync("./package.json"); + const packageData = JSON.parse(packageJson); + packageData.version = version; + const replaced = JSON.stringify(packageData, null, 2) + "\n"; + fs.writeFileSync("./package.json", replaced); + fs.writeFileSync("./src/version.ts", content); +} catch (error) { + console.error("Error updating version:", error.message); + process.exit(1); +}Also applies to: 7-11, 20-20
client/vite.config.js (1)
16-18: Test configuration should use explicit test framework setup.The
globals: trueconfiguration in Vite enables global test functions (describe, it, expect) without imports, but it's generally better practice to use explicit imports for clarity and type safety.Consider using explicit imports in test files instead of relying on globals:
test: { - globals: true, + globals: false, },And update test files to import test functions:
import { describe, it, expect } from 'vitest';client/src/constants.ts (1)
6-6: User agent string references incorrect project.The user agent string mentions "Tolgee-CLI" but this appears to be a client library, not a CLI tool. The GitHub URL also points to
tolgee-cliwhich may not be correct for this client package.-export const USER_AGENT = `Tolgee-CLI/${VERSION} (+https://github.com/tolgee/tolgee-cli)`; +export const USER_AGENT = `Tolgee-Client/${VERSION} (+https://github.com/tolgee/tolgee-platform)`;client/src/errorFromLoadable.ts (2)
35-41: Misleading comments for HTTP status casesComment labels don't match the status codes (400 is Bad Request, 401 is Unauthorized).
- // Unauthorized + // Bad Request case 400: return `Invalid request data ${addErrorDetails(loadable)}`; - // Unauthorized + // Unauthorized case 401: return `Missing or invalid authentication token ${addErrorDetails( loadable )}`;
30-31: Potential double newline when appending conflict detailsgetUnresolvedConflictsMessage currently returns a string starting with a newline. Combined with
"\n" + additionalInfo, this produces an extra blank line. Recommend removing the leading newline from getUnresolvedConflictsMessage (see comment in printFailedKeys.ts).client/src/printFailedKeys.ts (1)
8-12: Simplify renderKey string constructionThe nested template in
namespaceis unnecessary and slightly harder to read.export function renderKey(key: PartialKey, note?: string) { - const namespace = key.namespace ? ` ${`(namespace: ${key.namespace})`}` : ""; - const renderedNote = note ? ` ${note}` : ""; - return `${key.keyName}${namespace}${renderedNote}`; + const namespace = key.namespace ? ` (namespace: ${key.namespace})` : ""; + const renderedNote = note ? ` ${note}` : ""; + return `${key.keyName}${namespace}${renderedNote}`; }client/src/getApiKeyInformation.ts (2)
68-76: Add a fallback for missing PAT username to align with PAK handlingPAK path falls back to "", but PAT path omits a fallback which can yield
undefined. Make both consistent.- const username = info.user.name || info.user.username; + const username = info.user.name ?? info.user.username ?? "<unknown user>";
35-41: DRY: Factor out repeated 401 handling logicThe unauthorized error handling is duplicated for both endpoints. Consider extracting a small helper for clarity and consistency.
If you’d like, I can open a small follow-up PR to introduce a shared
assertAuthenticated(loadable)helper and update both branches accordingly.Also applies to: 59-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
client/package-lock.jsonis excluded by!**/package-lock.jsonwebapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
.github/workflows/publish-client.yml(1 hunks).github/workflows/test.yml(5 hunks)client/.gitignore(1 hunks)client/package.json(1 hunks)client/scripts/updateVersion.js(1 hunks)client/src/ApiClient.test.ts(1 hunks)client/src/ApiClient.ts(1 hunks)client/src/constants.ts(1 hunks)client/src/errorFromLoadable.ts(1 hunks)client/src/getApiKeyInformation.ts(1 hunks)client/src/main.ts(1 hunks)client/src/printFailedKeys.ts(1 hunks)client/src/schema.utils.ts(1 hunks)client/src/version.ts(1 hunks)client/src/vite-env.d.ts(1 hunks)client/tsconfig.json(1 hunks)client/vite.config.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
client/src/errorFromLoadable.ts (2)
client/src/ApiClient.ts (1)
LoadableData(10-12)client/src/printFailedKeys.ts (1)
getUnresolvedConflictsMessage(14-37)
client/src/ApiClient.test.ts (1)
client/src/ApiClient.ts (2)
createApiClient(74-171)projectIdFromKey(46-62)
client/src/constants.ts (1)
client/src/version.ts (1)
VERSION(4-4)
client/src/getApiKeyInformation.ts (3)
client/src/schema.generated.ts (1)
paths(6-5108)client/src/constants.ts (1)
API_KEY_PAK_PREFIX(4-4)client/src/errorFromLoadable.ts (1)
handleLoadableError(4-8)
client/src/ApiClient.ts (5)
client/src/constants.ts (2)
API_KEY_PAK_PREFIX(4-4)USER_AGENT(6-6)client/src/errorFromLoadable.ts (1)
errorFromLoadable(33-72)client/src/schema.generated.ts (1)
paths(6-5108)client/src/getApiKeyInformation.ts (1)
getApiKeyInformation(29-78)client/src/schema.utils.ts (1)
BodyOf(30-35)
client/src/schema.utils.ts (1)
client/src/schema.generated.ts (1)
paths(6-5108)
🔇 Additional comments (8)
.github/workflows/test.yml (1)
55-55: No functional change in quoting; safe to keepSwitching to double-quoted "false" keeps the value as a string (composite action inputs are strings). If ./.github/actions/setup-env relies on strict string comparison, this is fine; if it expects booleans, it would already have been receiving strings. No action needed.
If you want to be extra safe, confirm setup-env treats these inputs as strings:
- node
- java
Also applies to: 205-205, 254-254, 277-277, 308-308
client/tsconfig.json (1)
24-28: Emit declarations only is correct; ensure dist JS is produced by the bundlerWith "emitDeclarationOnly": true and outDir: ./lib, ensure your bundler (Vite/Rollup) outputs JS to dist and that package.json points "types" to lib and "exports"/"main"/"module" to dist. No changes needed if already configured.
client/src/ApiClient.test.ts (1)
1-29: Well-structured test suite with good coverage.The test suite effectively covers the key functionality of the API client:
- Basic client creation and configuration
- PAK key parsing (positive case)
- PAT key parsing (negative case)
- Invalid key handling
The tests are clear, focused, and use appropriate assertions.
client/src/constants.ts (1)
3-4: API key prefixes are well-defined.The constants clearly distinguish between Personal Access Tokens (PAT) and Project API Keys (PAK), which aligns with the key parsing logic in
ApiClient.ts.client/src/main.ts (1)
1-4: Clean module aggregation pattern.The main entry point follows a clean barrel export pattern, providing a single point of access to all public APIs from the various modules. This is a good practice for library distribution.
client/package.json (1)
1-40: Manifest looks publish-ready and consistent with dual ESM/CJS exportExports mapping is correct,
publishConfig.accessis set, and the absence of"private": truealigns with the new publish workflow.client/src/ApiClient.ts (1)
84-89: Add conditional x-api-key header to avoid "undefined" string.Currently, the x-api-key header is only set conditionally in the middleware (lines 103), but not in the initial headers. This is correct behavior. However, if the middleware ever gets bypassed or if there are edge cases, consider being defensive here as well.
client/src/schema.utils.ts (1)
1-49: Well-structured type utilities!The type utilities are well-designed and provide excellent type safety for the OpenAPI client. The use of conditional types and proper fallbacks to
voidensures type safety even when optional fields are absent. TheQueryOftype cleverly excludes the "ak" parameter, likely to prevent conflicts with the API key authentication.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
client/src/ApiClient.ts (5)
132-140: Good: getProjectId fails fast with a helpful message.This replaces the unsafe non-null assertion and prevents leaking undefined at runtime. Clear error text too.
100-104: Good: x-api-key is set conditionally.Avoids sending a literal "undefined" header value. Thanks for addressing this.
46-62: Harden projectIdFromKey against malformed PAKs (wrap decode/parse in try/catch).base32Decode/TextDecoder can throw for malformed keys. Today that would crash getProjectId(). Return undefined on failures.
export function projectIdFromKey(key: string) { if (!key.startsWith(API_KEY_PAK_PREFIX)) { return undefined; } - const keyBuffer = base32Decode( - key.slice(API_KEY_PAK_PREFIX.length).toUpperCase(), - "RFC4648" - ); - - const decoded = new TextDecoder("utf-8").decode( - keyBuffer as unknown as Uint8Array - ); - const idStr = decoded.split("_", 1)[0]; - const id = Number.parseInt(idStr, 10); - return Number.isFinite(id) ? id : undefined; + try { + const keyBuffer = base32Decode( + key.slice(API_KEY_PAK_PREFIX.length).toUpperCase(), + "RFC4648" + ) as unknown as Uint8Array; + const decoded = new TextDecoder("utf-8").decode(keyBuffer); + const idStr = decoded.split("_", 1)[0]; + const id = Number.parseInt(idStr, 10); + return Number.isFinite(id) ? id : undefined; + } catch { + return undefined; + } }
141-143: Guard getApiKeyInfo when apiKey is missing to avoid runtime crash.Non-null assertion can pass undefined to getApiKeyInformation, which immediately calls key.startsWith and throws.
- getApiKeyInfo() { - return getApiKeyInformation(apiClient, apiKey!); - }, + getApiKeyInfo() { + if (!apiKey) { + throw new Error("Cannot retrieve API key info: no apiKey configured."); + } + return getApiKeyInformation(apiClient, apiKey); + },
160-167: Fix Error constructor misuse in login fallback.Error only accepts a single message argument. Passing response.error as a 2nd arg causes a TS compile error and is ignored in JS.
} else { - throw new Error("Couldn't fetch access token", response.error); + throw new Error("Couldn't fetch access token"); }
🧹 Nitpick comments (3)
client/src/ApiClient.ts (3)
84-89: Avoid setting User-Agent header in browsers.Browsers disallow setting the User-Agent header and may throw “Refused to set unsafe header ‘User-Agent’”. Set it only in non-browser environments.
- const apiClient = createClient<paths>({ - baseUrl, - headers: { - "user-agent": USER_AGENT, - }, - }); + const apiClient = createClient<paths>({ baseUrl });And in the request hook:
onRequest: ({ request }) => { debug(`[HTTP] Requesting: ${request.method} ${request.url}`); + // Set user-agent only in non-browser environments + if (typeof window === "undefined") { + request.headers.set("user-agent", USER_AGENT); + } if (userToken) { request.headers.set("Authorization", "Bearer " + userToken); } else if (apiKey) { request.headers.set("x-api-key", apiKey); } },Also applies to: 97-105
107-111: Nit: reuse the apiVersion variable.Minor log cleanup; avoid redundant header lookups.
- const apiVersion = response.headers.get("x-tolgee-version"); - if (apiVersion) { - responseText += ` [${response.headers.get("x-tolgee-version")}]`; - } + const apiVersion = response.headers.get("x-tolgee-version"); + if (apiVersion) { + responseText += ` [${apiVersion}]`; + }
14-25: Reduce body cloning/reading in parseResponse; short-circuit stream first.Currently we clone+read text up front, even for large/streaming responses, and then parse again. Reorder to skip body reads for stream, and consult headers before falling back to reading text.
- // handle empty content - // note: we return `{}` because we want user truthy checks for `.data` or `.error` to succeed - const clonedBody = await response.clone().text(); - if ( - response.status === 204 || - response.headers.get("Content-Length") === "0" || - !response.body || - !clonedBody - ) { - return response.ok ? { data: {}, response } : { error: {}, response }; - } + // short-circuit streaming responses + if (response.ok && parseAs === "stream") { + return { data: response.body, response }; + } + // handle empty content (without eagerly reading the body) + // note: we return `{}` because we want user truthy checks for `.data` or `.error` to succeed + const contentLength = response.headers.get("Content-Length"); + if ( + response.status === 204 || + contentLength === "0" || + !response.body + ) { + return response.ok ? { data: {}, response } : { error: {}, response }; + } @@ - // if "stream", skip parsing entirely - if (parseAs === "stream") { - return { data: response.body, response }; - } return { data: await response[parseAs](), response };Also applies to: 27-34
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
client/src/ApiClient.ts(1 hunks)client/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/tsconfig.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
client/src/ApiClient.ts (5)
client/src/constants.ts (2)
API_KEY_PAK_PREFIX(4-4)USER_AGENT(6-6)client/src/errorFromLoadable.ts (1)
errorFromLoadable(33-72)client/src/schema.generated.ts (1)
paths(6-5108)client/src/getApiKeyInformation.ts (1)
getApiKeyInformation(29-78)client/src/schema.utils.ts (1)
BodyOf(30-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: BT 🔎 (security:test)
- GitHub Check: BT 🔎 (server-app:runWithoutEeTests)
- GitHub Check: BT 🔎 (ktlint:test)
- GitHub Check: BT 🔎 (server-app:runContextRecreatingTests)
- GitHub Check: BT 🔎 (ee-test:test)
- GitHub Check: BT 🔎 (data:test)
- GitHub Check: BT 🔎 (server-app:runStandardTests)
- GitHub Check: BT 🔎 (server-app:runWebsocketTests)
- GitHub Check: Frontend static check 🪲
- GitHub Check: Build frontend 🏗️
- GitHub Check: publish-client
|
This PR is stale because it has been open for 30 days with no activity. |
|
@Anty0, @dkrizan can you please get familiar with this and maybe do a CR? I think this will affect our future integration development. Thanks @stepan662 for finishing this! 🙏 |
| workflows: ["Test"] | ||
| branches: ["main"] | ||
| types: | ||
| - completed |
There was a problem hiding this comment.
Probably should be released when release (not the "Test" one) workflow is completed successfully.
| uses: ./.github/actions/setup-env | ||
| with: | ||
| node: 'false' | ||
| node: "false" |
There was a problem hiding this comment.
Probably should be reverted. Looks like we have different formatting preferences.
3 participants
Summary by CodeRabbit
New Features
Chores
Tests
Style