fix: pull --watch #188
fix: pull --watch #188
Conversation
WalkthroughAdds WebSocket/STOMP client and watch-based pull flow with conditional export using If-Modified-Since. Introduces last-modified storage utilities, updates pull command (watch mode, debounce, backup polling), adjusts ExportClient to set conditional header, changes error handling to throw LoadableError, updates credentials API, and adds runtime dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI pull command
participant Watch as WatchHandler
participant WS as WebsocketClient
participant API as Tolgee API
participant FS as Filesystem
User->>CLI: tolgee pull --watch
CLI->>Watch: startWatching({ apiUrl, apiKey, projectId, doPull })
Watch->>WS: connectIfNotAlready(auth, serverUrl/websocket)
WS-->>Watch: onConnected
Note right of Watch: Schedule initial pull<br/>and start 60s backup polling
Watch->>CLI: doPull()
CLI->>API: export(zip=true, If-Modified-Since=lastModified?)
API-->>CLI: 200 with zip + Last-Modified OR 304/412
alt 200 OK
CLI->>FS: extract + write files
CLI->>CLI: store Last-Modified
else 304/412 Not modified
CLI-->>CLI: skip work
end
WS-->>Watch: message /projects/{id}/translation-data-modified
Watch->>CLI: debounced doPull()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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. ✨ 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (1)
33-47: Remove unused “ws” dependency & add SockJS types
- No references to the “ws” package were found anywhere in the TypeScript code—only SockJS is ever instantiated for STOMP transport (
Stomp.over(() => new SockJS(...))in src/client/WebsocketClient.ts) ; you can safely remove"ws": "^8.18.3"from package.json.- Since you’re importing ‘sockjs-client’ directly in TS (no
@ts-ignoreis applied), add@types/sockjs-clientto devDependencies so the compiler has proper type definitions for SockJS v1.x.src/client/TolgeeClient.ts (1)
1-1: Fix ESLint failure: remove unused importexitWithError is no longer used after switching to exceptions. Drop the import to get CI green.
-import { exitWithError } from './../utils/logger.js';
🧹 Nitpick comments (13)
src/client/TolgeeClient.ts (1)
29-33: Enrich LoadableError for better diagnostics (name + stack + optional cause)Minor ergonomics: set the error name and capture a proper stack. Optionally support a cause field if an underlying fetch error exists.
export class LoadableError extends Error { - constructor(public loadable: LoadableData) { - super(errorFromLoadable(loadable)); - } + constructor(public loadable: LoadableData, cause?: unknown) { + super(errorFromLoadable(loadable)); + this.name = 'LoadableError'; + // Preserve original error in modern runtimes + // @ts-expect-error cause is supported in Node 18+ + if (cause) (this as any).cause = cause; + if (Error.captureStackTrace) { + Error.captureStackTrace(this, LoadableError); + } + } }Optionally, consider a type-asserting helper to improve TS narrowing:
export function assertOk(loadable: LoadableData): asserts loadable is LoadableData & { error?: undefined } { if (loadable.error) throw new LoadableError(loadable); }src/client/ExportClient.ts (1)
23-33: Normalize If-Modified-Since and consider Date inputGood call moving conditional logic to headers. Two small polish items:
- Accept Date | string for ifModifiedSince and normalize to RFC 1123 (toUTCString()).
- Optionally omit the headers field entirely when empty (nit).
-export type ExportRequest = Omit< +export type ExportRequest = Omit< BodyOf<'/v2/projects/{projectId}/export', 'post'>, 'zip' > & { - ifModifiedSince?: string; + ifModifiedSince?: string | Date; }; ... - const { ifModifiedSince, ...exportReq } = req; + const { ifModifiedSince, ...exportReq } = req; const body = { ...exportReq, zip: true }; - const headers: Record<string, string> = {}; - if (ifModifiedSince) { - headers['If-Modified-Since'] = ifModifiedSince; - } + const headers = + ifModifiedSince !== undefined + ? { + 'If-Modified-Since': + typeof ifModifiedSince === 'string' + ? ifModifiedSince + : ifModifiedSince.toUTCString(), + } + : undefined; ... - headers, + headers,Please confirm how 304 Not Modified is surfaced by ApiClient.POST (is it a non-error with empty body, or an error?). Ensure pull.ts handles the chosen behavior correctly.
src/config/credentials.ts (1)
110-117: Enhance getApiKey resilience: support both URL and string inputsTo avoid unexpected CLI crashes when callers pass malformed URLs (or URL objects), update
getApiKeyinsrc/config/credentials.tsto acceptstring | URLand guard thenew URL()call. Existing call sites already pass either aURLinstance (in tests) or a string (from the CLI), so this change is fully backwards-compatible.• File
src/config/credentials.ts, lines 109–117:
- Change the signature
- Wrap the constructor in a
try/catchthat logs a warning and returnsnullon invalid inputProposed diff:
-export async function getApiKey( - apiUrl: string, +export async function getApiKey( + apiUrl: string | URL, projectId: number ): Promise<string | null> { const store = await loadStore(); - const apiUrlObj = new URL(apiUrl); + let apiUrlObj: URL; + try { + apiUrlObj = apiUrl instanceof URL ? apiUrl : new URL(apiUrl); + } catch { + warn('Invalid API URL provided to getApiKey().'); + return null; + } if (!store[apiUrlObj.hostname]) { // …This will ensure:
- Tests using
new URL(...)still work seamlessly.- CLI calls passing a string URL will be normalized.
- Malformed inputs no longer throw uncaught errors—instead they yield
nullwith a warning.src/utils/lastModifiedStorage.ts (2)
9-12: Consider scoping by host+project to avoid collisionsKeying only by projectId risks collisions if the same numeric projectId is used on multiple servers within a single process. Not critical for a typical single-target CLI run, but easy to future-proof by keying with
${hostname}:${projectId}.Is watch mode ever intended to connect to multiple servers concurrently? If yes, I can provide a small adapter to add host scoping.
24-29: Add a clear/remove utility and optional TTL (future-proofing)A tiny helper like clearLastModified(projectId?: number) would help tests and long-lived watch sessions. You already store timestamp; adding a simple TTL eviction (e.g., 24h) could prevent stale growth.
src/client/WebsocketClient.ts (2)
50-63: Use logger instead of console; fix callback spacing and typing; avoid trailing comma.
- Prefer
debugoverconsole.log.- Prettier expects a space in
function (message: any).- Parse to a typed envelope instead of the ad-hoc
Messagetype for better DX.- const subscribeToStompChannel = (subscription: Subscription<any>) => { + const subscribeToStompChannel = (subscription: Subscription<ChannelProject | ChannelUser>) => { if (connected) { debug(`Subscribing to ${subscription.channel}`); const stompSubscription = _client!.subscribe( subscription.channel, - function(message: any) { - const parsed = JSON.parse(message.body) as Message; - subscription.callback(parsed as any); - }, + function (message: any) { + const parsed = JSON.parse(message.body) as IncomingEvent; + subscription.callback(parsed as any); + } ); - console.log('Subscribed to: ', subscription.channel, ' with id: ', stompSubscription.id); + debug( + `Subscribed to ${subscription.channel} with id: ${stompSubscription.id}` + ); subscription.unsubscribe = stompSubscription.unsubscribe; subscription.id = stompSubscription.id; } };Also add near the top (replace the old
Messagetype):-type Message = { - type: string; - actor: any; - data: any; -}; +type IncomingEvent = WebsocketEvent<any> & { type?: string };
163-173: Return an empty headers object instead of null from getAuthentication.
connectexpects headers; returning{}avoids null checks inside stomp client.function getAuthentication(options: WebsocketClientOptions) { if (options.authentication.jwtToken) { return { jwtToken: options.authentication.jwtToken }; } if (options.authentication.apiKey) { return { 'x-api-key': options.authentication.apiKey }; } - return null; + return {}; }src/utils/watchHandler.ts (4)
3-9: Remove unused imports and dead “lastModified” plumbing.
getLastModifiedandextractLastModifiedFromResponseare unused.- Passing
lastModifiedthrough schedule/execute adds noise and is never set by the WS callback;doPullalready persists last-modified.-import { - getLastModified, - setLastModified, - extractLastModifiedFromResponse, -} from './lastModifiedStorage.js'; -import { clearInterval } from 'node:timers'; +// no-op @@ - let lastExecutionTime = 0; + let lastExecutionTime = 0; @@ - const executePull = async (lastModified?: string) => { + const executePull = async () => { if (pulling) return; pulling = true; lastExecutionTime = Date.now(); try { await doPull(); - // Store last modified timestamp after successful pull - if (lastModified) { - setLastModified(projectId, lastModified); - } } catch (e: any) { error('Error during pull: ' + e.message); debug(e); } finally { pulling = false; } }; @@ - const schedulePull = async (lastModified?: string) => { + const schedulePull = async () => { const now = Date.now(); const timeSinceLastExecution = now - lastExecutionTime; @@ - if (timeSinceLastExecution >= SCHEDULE_PULL_DEBOUNCE_MS) { - await executePull(lastModified); + if (timeSinceLastExecution >= SCHEDULE_PULL_DEBOUNCE_MS) { + await executePull(); } else { // Otherwise, schedule the update with debounce if (debounceTimer) clearTimeout(debounceTimer); - debounceTimer = setTimeout(() => executePull(lastModified), SCHEDULE_PULL_DEBOUNCE_MS); + debounceTimer = setTimeout(() => executePull(), SCHEDULE_PULL_DEBOUNCE_MS); } };Also applies to: 35-51, 53-65
8-11: Timer API usage: avoid importing clearInterval; guard undefined handles.Importing
clearIntervalfromnode:timerscan cause typing friction and is inconsistent with theclearTimeoutusage. Use globals and guard undefined.-import { clearInterval } from 'node:timers'; +// rely on global clearInterval @@ - const startPolling = () => { - clearInterval(pollingTimer); + const startPolling = () => { + if (pollingTimer) { + clearInterval(pollingTimer); + } const poll = async () => { if (pulling) return; await schedulePull(); }; @@ - if (pollingTimer) { - clearInterval(pollingTimer); - } + if (pollingTimer) clearInterval(pollingTimer);Also applies to: 68-77, 109-114
79-85: Use already-typed URL: no need to re-wrap withnew URL(...).
apiUrlis already aURL. Use itsorigindirectly.- const wsClient = WebsocketClient({ - serverUrl: new URL(apiUrl).origin, + const wsClient = WebsocketClient({ + serverUrl: apiUrl.origin,
97-101: Optional: surface event payload for future use.You currently ignore the event payload. Consider capturing it for future optimizations (e.g., selective pulls or logging), even if you don’t use it yet.
src/commands/pull.ts (2)
8-14: Remove unusedinfoimport.
infoisn’t used; drop it to satisfy ESLint.import { exitWithError, loading, success, - info, debug, } from '../utils/logger.js';
1-1: Remove unusedBlobimport.Not referenced in this module.
-import { Blob } from 'buffer'; +// removed unused Blob import
📜 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 (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.json(2 hunks)src/client/ExportClient.ts(2 hunks)src/client/TolgeeClient.ts(1 hunks)src/client/WebsocketClient.ts(1 hunks)src/commands/pull.ts(4 hunks)src/config/credentials.ts(2 hunks)src/utils/lastModifiedStorage.ts(1 hunks)src/utils/watchHandler.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test
src/client/TolgeeClient.ts
[warning] 1-1: ESLint: 'exitWithError' is defined but never used. (no-unused-vars)
src/client/WebsocketClient.ts
[error] 55-55: Prettier formatting error. Insert · (prettier/prettier). Run 'prettier --write' to fix formatting.
🪛 GitHub Check: ESLint
src/client/WebsocketClient.ts
[failure] 199-199:
Insert ··
[failure] 198-198:
Insert ··
[failure] 130-130:
Delete ⏎······
[failure] 127-127:
Delete ,
[failure] 107-107:
Delete ,
[failure] 89-89:
Insert ·
[failure] 83-83:
Insert ·
[failure] 60-60:
Replace 'Subscribed·to:·',·subscription.channel,·'·with·id:·',·stompSubscription.id with ⏎········'Subscribed·to:·',⏎········subscription.channel,⏎········'·with·id:·',⏎········stompSubscription.id⏎······
[failure] 58-58:
Delete ,
[failure] 55-55:
Insert ·
src/utils/watchHandler.ts
[warning] 6-6:
'extractLastModifiedFromResponse' is defined but never used. Allowed unused vars must match /^_/u
[warning] 4-4:
'getLastModified' is defined but never used. Allowed unused vars must match /^_/u
src/commands/pull.ts
[warning] 12-12:
'info' is defined but never used. Allowed unused vars must match /^_/u
🔇 Additional comments (8)
src/client/TolgeeClient.ts (1)
23-27: Switch to throwing LoadableError—please verify all call sitesYou've replaced the old process-exit behavior with a typed
LoadableError, which is a cleaner API. Please confirm that each of the following calls tohandleLoadableError(...)now either:
- Is wrapped in a
try/catchthat handlesLoadableErrorand invokes the proper exit logic, or- Is invoked under your global CLI error handler that catches
LoadableError, formats the message, and callsprocess.exit(1)(orexitWithError).Affected call sites:
src/commands/tag.ts(line 63)src/commands/pull.ts(line 106)src/commands/sync/sync.ts(lines 41, 101, 128, 169, 188)src/commands/push.ts(lines 257, 267)src/commands/sync/compare.ts(line 33)src/client/getApiKeyInformation.ts(lines 41, 63)src/config/credentials.ts (2)
127-129: Minor: align storage helpers with new param shapePassing apiUrlObj into storePat is correct. No action needed—just noting the intentional shift from instance.hostname to apiUrlObj.hostname.
143-146: Expiry handling is correct—keep message consistentThe warning message correctly reflects host scoping with apiUrlObj.hostname. Looks good.
src/utils/lastModifiedStorage.ts (1)
31-35: LGTM: header extraction is straightforwardUsing Response.headers.get('Last-Modified') aligns with fetch semantics in Node 18+. Looks good.
src/client/WebsocketClient.ts (2)
1-74: Formatting Fixes & CI Setup
It looks like the CI commands failed because dependencies weren’t installed, so the formatter and test runner aren’t available. Before fixing the formatting, please install the node modules and then re-run the lint/format steps.• Install dependencies and run the formatter/tester:
pnpm install pnpm eslint --fix . pnpm test -s• If you prefer using npm-scripts, ensure your package.json defines a
lintscript (e.g."lint": "eslint ."), then you can do:pnpm install pnpm lint pnpm test -s• After that, apply Prettier/ESLint fixes in these ranges to satisfy CI:
– src/client/WebsocketClient.ts: lines 1–74, 83–101, 125–143, 198–199Once the formatting errors are resolved and tests pass, CI should unblock.
163-170: Please verify WebSocket/STOMP authentication headersI wasn’t able to find any official documentation or server-side code that confirms which headers Tolgee’s WebSocket/STOMP endpoint actually reads. Before merging, please double-check with the backend team or in the server code whether:
- The STOMP CONNECT frame must use an
Authorization: Bearer <token>header (or a customjwtTokenheader)- The server supports
X-API-Keyfor project API keys, or if it expects it under a different header name (e.g.X-API-Keyvs.x-api-key)Affected location:
src/client/WebsocketClient.ts, lines 163–170src/commands/pull.ts (2)
69-85: LGTM: not-modified short-circuit and last-modified persistence.The early return on
notModifiedavoids unnecessary I/O, and persistinglastModifiedonly after a successful extraction is sound.
111-118: Confirmed HTTP 412 is used for POST exportsTolgee’s export endpoint returns 412 (Precondition Failed) when the
If-Modified-Sinceheader indicates no updates for a POST request, and 304 (Not Modified) only applies to GET requests. The existing code correctly checks for both status codes, so no changes are required.
| function connectIfNotAlready() { | ||
| if (_deactivated) { | ||
| return; | ||
| } | ||
|
|
||
| const client = getClient(); | ||
|
|
||
| const onConnected = function() { | ||
| connected = true; | ||
| resubscribe(); | ||
| options.onConnected?.(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent multiple concurrent connects with a “connecting” guard.
Rapid successive subscribe() calls can race and call connect() multiple times before connected flips to true. Guard with a connecting flag that resets on success or failure.
@@
- let connected = false;
+ let connected = false;
+ let connecting = false;
@@
function connectIfNotAlready() {
if (_deactivated) {
return;
}
+ if (connected || connecting) {
+ return;
+ }
+ connecting = true;
+
const client = getClient();
@@
- const onConnected = function() {
+ const onConnected = function() {
connected = true;
+ connecting = false;
resubscribe();
options.onConnected?.();
};
@@
- const onError = () => {
+ const onError = () => {
+ connecting = false;
options.onError?.();
};Also applies to: 99-109, 111-117, 151-154
🧰 Tools
🪛 GitHub Check: ESLint
[failure] 83-83:
Insert ·
🤖 Prompt for AI Agents
In src/client/WebsocketClient.ts around lines 76-86 (and similarly at 99-109,
111-117, and 151-154), rapid successive subscribe() calls can race and call
connect() multiple times; add a boolean connecting guard at the module/instance
level, set connecting=true immediately when attempting to connect, skip new
connect attempts while connecting is true, and reset connecting=false on both
successful connection (in onConnected) and any failure/cleanup paths (e.g., on
error/close/catch), ensuring connected remains authoritative and that the guard
is cleared in all code paths so subsequent reconnects still work.
| const onDisconnect = function() { | ||
| connected = false; | ||
| subscriptions.forEach((s) => { | ||
| s.unsubscribe = undefined; | ||
| s.id = undefined; | ||
| removeSubscription(s); | ||
| }); | ||
| options.onConnectionClose?.(); | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Disconnect handler is never wired; subscriptions are also incorrectly removed on disconnect (breaks auto-resubscribe).
client.connectin stompjs does not accept a 4thonDisconnectargument, so youronDisconnectwill never run.- Additionally, in
onDisconnectyou callremoveSubscription(s), which wipes the in-memory list. After reconnect,resubscribe()finds nothing to re-subscribe.
Fix:
- Wire
onDisconnectviaclient.onWebSocketClose(and optionallyonWebSocketError/onStompError). - Do not remove stored subscriptions on transient disconnects; just clear
id/unsubscribesoresubscribe()can re-create them.
Apply this diff:
@@
- const onDisconnect = function() {
+ const onDisconnect = function() {
connected = false;
- subscriptions.forEach((s) => {
- s.unsubscribe = undefined;
- s.id = undefined;
- removeSubscription(s);
- });
+ subscriptions.forEach((s) => {
+ s.unsubscribe = undefined;
+ s.id = undefined;
+ });
options.onConnectionClose?.();
};
@@
- client.connect(
- getAuthentication(options),
- onConnected,
- onError,
- onDisconnect,
- );
+ // Wire lifecycle handlers properly
+ client.onWebSocketClose = onDisconnect;
+ client.onWebSocketError = onError;
+ // Some brokers emit STOMP errors instead of WS close
+ // @ts-ignore - available on CompatClient at runtime
+ client.onStompError = onError;
+ // Connect (no 4th argument)
+ client.connect(getAuthentication(options) ?? {}, onConnected, onError);Also applies to: 103-109
🧰 Tools
🪛 GitHub Check: ESLint
[failure] 89-89:
Insert ·
🤖 Prompt for AI Agents
In src/client/WebsocketClient.ts around lines 89-98 (also applies to 103-109):
the disconnect handler is never wired because stompjs client.connect does not
accept a 4th onDisconnect argument, and you currently remove subscriptions from
the in-memory list which prevents auto-resubscribe after reconnect. Fix by
registering the handler on client.onWebSocketClose (and optionally
onWebSocketError/onStompError) instead of passing it to connect; in the handler
set connected = false and clear each subscription's id and unsubscribe function
(set to undefined) but do NOT call removeSubscription or delete the subscription
entries so resubscribe() can recreate them after reconnect; ensure
options.onConnectionClose?.() is still invoked from the wired handler.
Summary by CodeRabbit