feat: pull --watch #188
feat: pull --watch #188
Conversation
WalkthroughAdds websocket-based watch mode for pull with STOMP/SockJS, ETag-aware conditional export, auth-aware handlers, in-memory ETag storage, version utility, CI/type-check updates, runtime websocket deps, and supporting unit and E2E tests and utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Pull CLI
participant WS as WebSocket (STOMP/SockJS)
participant Server
participant Poll as Polling
participant FS as Filesystem
User->>CLI: run `pull --watch`
CLI->>CLI: load stored ETag
CLI->>WS: connect & subscribe (/projects/{id}/translation-data-modified)
WS->>Server: STOMP SUBSCRIBE
loop While watching
alt Server event
Server->>WS: translation-data-modified
WS->>CLI: notify -> schedulePull()
else Poll interval
Poll->>CLI: trigger schedulePull()
end
CLI->>CLI: debounce (≤500ms)
CLI->>Server: POST /v2/projects/{id}/export (If-None-Match: ETag)
alt Changed
Server-->>CLI: 200 + ZIP + ETag
CLI->>FS: extract ZIP -> update files
CLI->>CLI: setETag(newEtag)
else Not modified
Server-->>CLI: 304/412 (notModified)
CLI->>CLI: skip extraction
end
end
User->>CLI: SIGINT/SIGTERM
CLI->>WS: unsubscribe & deactivate
CLI->>Poll: clear timers
CLI->>CLI: shutdown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@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.
- made it working - added an option to provide an alternative backend for testing - made it fetching the data at the beginning
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
src/client/WebsocketClient.ts (2)
75-108: Prevent multiple concurrent connects with a "connecting" guard.Rapid successive
subscribe()calls can race and callconnect()multiple times beforeconnectedflips to true. Guard with aconnectingflag that resets on success or failure.Apply this diff:
let connected = false; + let connecting = false; let subscriptions: Subscription<any>[] = []; function connectIfNotAlready() { if (_deactivated) { return; } + + if (connected || connecting) { + return; + } + connecting = true; const client = getClient(); const onConnected = function () { connected = true; + connecting = false; resubscribe(); options.onConnected?.(); }; const onDisconnect = function () { connected = false; + connecting = false; subscriptions.forEach((s) => { s.unsubscribe = undefined; s.id = undefined; removeSubscription(s); }); options.onConnectionClose?.(); }; const onError = (error: any) => { + connecting = false; options.onError?.(error); };Based on learnings from past review comments.
88-107: 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 () { connected = false; subscriptions.forEach((s) => { s.unsubscribe = undefined; s.id = undefined; - removeSubscription(s); }); options.onConnectionClose?.(); }; const onError = (error: any) => { options.onError?.(error); }; - 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 + client.onStompError = onError; + // Connect (no 4th argument) + client.connect(getAuthentication(options) ?? {}, onConnected, onError);Based on learnings from past review comments.
🧹 Nitpick comments (8)
test/e2e/__internal__/setup.ts (1)
6-13: LGTM! Conditional Docker startup logic is correct.The implementation properly checks for
TOLGEE_TEST_BACKEND_URLbefore starting the Docker backend, with helpful console logs for debugging.Note: ESLint flagged formatting issues on lines 8 and 11 (Prettier formatting). These are purely stylistic and can be auto-fixed with
npm run format.src/utils/lastModifiedStorage.ts (2)
1-6: Consider adding cleanup for long-running watch processes.The in-memory Map will grow indefinitely as projects are accessed. For CLI watch mode running continuously, this could become a minor memory leak over time.
Consider adding a cleanup mechanism, such as:
- LRU eviction after a certain number of entries
- Time-based expiration using the timestamp field
- A periodic cleanup function
This is less critical for short-lived CLI invocations but important for long-running watch mode.
25-29: Redundant|| undefinedin extractLastModifiedFromResponse.The expression
response.headers.get('Last-Modified') || undefinedis redundant because.get()already returnsstring | null, and the function return type allowsundefined. The|| undefinedconvertsnulltoundefinedbut adds no meaningful value.Simplify to:
export function extractLastModifiedFromResponse( response: Response ): string | undefined { - return response.headers.get('Last-Modified') || undefined; + return response.headers.get('Last-Modified') ?? undefined; }Or even simpler (if
nullis acceptable asundefinedin the return type):-): string | undefined { - return response.headers.get('Last-Modified') || undefined; +): string | null | undefined { + return response.headers.get('Last-Modified'); }test/e2e/pull-watch.test.ts (1)
31-35: Consider reducing the test timeout.A 300-second (5-minute) timeout is very long for an E2E test. Given that the test performs just two pulls with a 1-second sleep between them, a timeout of 30-60 seconds should be sufficient while still providing buffer for CI environments.
const process = run( ['pull', '--api-key', pak, '--path', TMP_FOLDER, '--watch', '--verbose'], undefined, - 300000 + 30000 // 30 seconds );src/utils/watchHandler.ts (3)
109-124: Empty catch blocks silently suppress errors.Lines 112 and 115 have empty catch blocks that silently swallow any errors during shutdown. While this might be intentional for cleanup code, it makes debugging difficult and could hide issues.
Consider at least logging errors at debug level:
const shutdown = () => { try { unsubscribe?.(); - } catch {} + } catch (e) { + debug('Error during unsubscribe:', e); + } try { wsClient.deactivate(); - } catch {} + } catch (e) { + debug('Error during WebSocket deactivate:', e); + } if (pollingTimer) { clearInterval(pollingTimer); }Based on learnings, empty catch blocks are generally discouraged unless there's a clear reason to suppress errors.
66-77: Polling runs continuously alongside WebSocket connection.The polling mechanism starts on every update event (line 105), which means it runs continuously even when the WebSocket connection is working perfectly. This creates unnecessary load.
Consider these approaches:
Option 1: Only use polling as a fallback when WebSocket fails:
- Start polling when
onErrororonConnectionCloseis triggered- Stop polling when
onConnectedis triggered and subscription succeedsOption 2: Use polling as a heartbeat with a longer interval (e.g., 5 minutes) to catch missed events
Example for Option 1:
let pollingActive = false; const wsClient = WebsocketClient({ serverUrl: new URL(apiUrl).origin, authentication: { apiKey: apiKey }, onConnected: () => { stopPolling(); // Stop polling when WebSocket is connected subscribe(); }, onError: (error) => { handleAuthErrors(error, shutdown); startPolling(); // Start polling as fallback info('Websocket error encountered. Using polling fallback...'); }, onConnectionClose: () => { startPolling(); // Start polling as fallback info('Websocket connection closed. Using polling fallback...'); }, }); const stopPolling = () => { if (pollingTimer) { clearInterval(pollingTimer); pollingTimer = undefined; pollingActive = false; } }; const startPolling = () => { if (pollingActive) return; // Don't start multiple polling intervals pollingActive = true; // ... rest of startPolling logic };
129-134: Keep-alive pattern is correct but could be documented.The
await new Promise<void>(() => {})on line 133 is an intentional infinite await to keep the process alive, which is correct for watch mode. However, this might be confusing to future maintainers.Consider adding a comment to clarify the intent:
+ // Keep the process alive indefinitely in watch mode + // The process will only exit via SIGINT/SIGTERM handlers await new Promise<void>(() => {}); }src/client/WebsocketClient.ts (1)
161-171: Consider returning an empty object instead of null.Line 170: Returning
nullwhen no authentication is provided might cause issues with the STOMP connect method. Consider returning an empty object{}instead, or ensure the connect call handles null properly (as suggested in the earlier fix with?? {}).Apply this diff:
if (options.authentication.apiKey) { return { 'x-api-key': options.authentication.apiKey }; } - return null; + return {}; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
HACKING.md(1 hunks)src/client/WebsocketClient.ts(1 hunks)src/commands/pull.ts(4 hunks)src/utils/lastModifiedStorage.ts(1 hunks)src/utils/watchHandler.ts(1 hunks)test/e2e/__internal__/setup.ts(1 hunks)test/e2e/pull-watch.test.ts(1 hunks)test/e2e/tags.test.ts(2 hunks)test/e2e/utils/api/common.ts(1 hunks)test/e2e/utils/run.ts(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
test/e2e/pull-watch.test.ts
[error] 48-62: Do not export from a test file.
(lint/suspicious/noExportsInTest)
[error] 63-74: Do not export from a test file.
(lint/suspicious/noExportsInTest)
[error] 75-88: Do not export from a test file.
(lint/suspicious/noExportsInTest)
🪛 GitHub Check: ESLint
test/e2e/pull-watch.test.ts
[warning] 31-31:
'process' is assigned a value but never used. Allowed unused vars must match /^_/u
test/e2e/__internal__/setup.ts
[failure] 11-11:
Replace Using·alternative·backend:·${process.env.TOLGEE_TEST_BACKEND_URL} with ⏎······Using·alternative·backend:·${process.env.TOLGEE_TEST_BACKEND_URL}⏎····
[failure] 8-8:
Replace 'Starting·Docker·backend·(no·TOLGEE_TEST_BACKEND_URL·provided)' with ⏎······'Starting·Docker·backend·(no·TOLGEE_TEST_BACKEND_URL·provided)'⏎····
src/utils/watchHandler.ts
[failure] 115-115:
Empty block statement
[failure] 112-112:
Empty block statement
⏰ 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). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
🔇 Additional comments (12)
HACKING.md (1)
35-49: LGTM! Clear documentation for alternative backend configuration.The new subsection effectively documents the
TOLGEE_TEST_BACKEND_URLenvironment variable with practical examples. This aligns well with the test infrastructure changes introduced in this PR.test/e2e/tags.test.ts (1)
4-4: LGTM! Good refactoring to use centralized API_URL constant.Replacing the hardcoded URL with the imported
API_URLconstant improves maintainability and ensures consistency across test files.Also applies to: 25-25
test/e2e/utils/run.ts (1)
40-41: LGTM! Environment-driven backend URL configuration is correct.The fallback to
http://localhost:22222ensures backward compatibility while enabling alternative backend testing.test/e2e/utils/api/common.ts (1)
6-7: LGTM! Exporting API_URL centralizes backend URL configuration.Making
API_URLan exported constant allows test files to consistently reference the configured backend, improving maintainability.src/utils/lastModifiedStorage.ts (1)
1-6: Verify: Is in-memory storage acceptable for watch mode?The comment notes "In the future we can use filesystem to store it." For the watch mode use case, in-memory storage means:
- Last-Modified state is lost when the CLI process restarts
- First pull after restart will fetch all data (no If-Modified-Since header)
This is likely acceptable for short-lived CLI watch sessions, but please confirm this behavior aligns with user expectations. If watch mode is intended for long-running server-side deployments, filesystem persistence would provide better UX.
src/utils/watchHandler.ts (1)
100-107: Verify: Subscription logic appears to unsubscribe immediately on first event.The current implementation calls
unsubscribe?.()inside the subscription handler (line 102), which appears to unsubscribe immediately upon receiving the first event. This seems incorrect if the intent is to keep watching for changes.Looking at the flow:
- Line 101: Subscribe to channel
- Event received → Line 102: Unsubscribe
- Line 104: Schedule pull
- Line 105: Start polling
This pattern suggests the WebSocket is only used for the initial trigger, then falls back to polling. However, this defeats the purpose of a WebSocket connection for real-time updates.
Expected behavior questions:
- Should the subscription remain active to receive multiple update events?
- Or is the intent to receive one event, then rely on polling?
- Why unsubscribe after each event if the
onConnectedcallback will re-subscribe?If you want continuous WebSocket updates, remove the
unsubscribe()call from inside the handler:function subscribe() { unsubscribe = wsClient.subscribe(channel, () => { - unsubscribe?.(); debug('Data change detected by websocket. Pulling now... '); schedulePull(); - startPolling(); }); }And consider whether
startPolling()should only be called as a fallback when WebSocket connection fails, not on every event.src/commands/pull.ts (3)
40-67: LGTM!The watch/non-watch orchestration is clear and delegates appropriately to either a one-shot doPull or the continuous startWatching flow.
191-196: LGTM!The --watch option is correctly defined with an appropriate description and default value.
88-119: The export API correctly supports the ifModifiedSince parameter.The
ExportRequesttype includesifModifiedSince?: string, and the export method properly converts it to theIf-Modified-SinceHTTP header for conditional requests. The implementation in pull.ts correctly passes this parameter to the API, which will handle the 304/412 status codes appropriately.src/client/WebsocketClient.ts (3)
124-141: LGTM, pending the connectIfNotAlready fixes.The subscribe function correctly orchestrates connection and subscription management. However, it depends on the fixes to
connectIfNotAlreadyto avoid race conditions.
143-156: LGTM!The disconnect, deactivate, and removeSubscription functions are straightforward and correctly implemented.
173-257: LGTM!The type definitions are well-structured and provide strong type safety for channel subscriptions and event payloads. The use of conditional types in
Data<T>ensures type-correct callbacks for each channel.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/utils/watchHandler.ts (1)
135-143: Fixerr(...)typo to call the logger’serror(...)function.
handleAuthErrorscallserr("You're not authorized. Insufficient permissions?"), but there is noerrfunction imported. This will throw at runtime and preventshutdown()from executing correctly.Replace
err(...)with the importederror(...):if (err?.headers?.message == 'Unauthorized') { - err("You're not authorized. Insufficient permissions?"); + error("You're not authorized. Insufficient permissions?"); shutdown(); }src/client/WebsocketClient.ts (3)
51-63: Harden subscription callback: guard_clientand handle malformed JSON.
subscribeToStompChannelassumes_clientis non-null and thatmessage.bodyis always valid JSON. A bad message would currently crash the process.Consider:
- const subscribeToStompChannel = (subscription: Subscription<any>) => { - if (connected) { + const subscribeToStompChannel = (subscription: Subscription<any>) => { + if (!connected || !_client) { + return; + } + try { debug(`Subscribing to ${subscription.channel}`); - const stompSubscription = _client!.subscribe( + const stompSubscription = _client.subscribe( subscription.channel, function (message: any) { - const parsed = JSON.parse(message.body) as Message; - subscription.callback(parsed as any); + try { + const parsed = JSON.parse(message.body) as Message; + subscription.callback(parsed as any); + } catch (error) { + debug( + `Failed to parse message from ${subscription.channel}: ` + + (error as any)?.message + ); + } } ); subscription.unsubscribe = stompSubscription.unsubscribe; subscription.id = stompSubscription.id; - } + } catch (error) { + debug( + `Failed to subscribe to ${subscription.channel}: ` + + (error as any)?.message + ); + } };For @stomp/stompjs CompatClient, does `client.subscribe` ever invoke the callback with non-JSON payloads or structured error frames that might not match `{ body: string }`? Verify recommended error-handling patterns for message parsing.
66-74: ValidateserverUrlbefore constructing the SockJS endpoint.
initClientassumesoptions.serverUrlis defined and concatenates it into a URL string. If callers omitserverUrl, you’ll end up connecting to"undefined/websocket".Consider enforcing a non-empty
serverUrl:function initClient() { - _client = Stomp.over(() => new SockJS(`${options.serverUrl}/websocket`)); + if (!options.serverUrl) { + throw new Error('serverUrl is required for WebSocket connection'); + } + const base = options.serverUrl.replace(/\/+$/, ''); + _client = Stomp.over(() => new SockJS(`${base}/websocket`));What is the recommended way to configure SockJS endpoint URLs with @stomp/stompjs and sockjs-client (e.g., expected base URL format and trailing slash handling)?
103-108: Pass an object (notnull) as headers toclient.connect.
getAuthentication(options)returnsnullwhen neither JWT token nor API key is provided, but@stomp/stompjsCompatClient.connect expects TypeScript-typedStompHeadersobject, notnull. Although the library acceptsnullat runtime due to its varargs implementation, this violates the library's type contract and should be avoided.+ const headers = getAuthentication(options) ?? {}; - client.connect( - getAuthentication(options), + client.connect( + headers, onConnected, onError, onDisconnect );
🧹 Nitpick comments (2)
src/utils/watchHandler.ts (2)
108-120: Avoid emptycatch {}blocks or at least document why errors are ignored.Both
try { unsubscribe?.(); } catch {}andtry { wsClient.deactivate(); } catch {}silently swallow all errors and trigger ESLint’s “empty block statement” warnings. This can hide real shutdown issues and complicate debugging.Consider at least logging at debug level or adding a comment to justify intentional suppression:
- const shutdown = () => { - try { - unsubscribe?.(); - } catch {} - try { - wsClient.deactivate(); - } catch {} + const shutdown = () => { + try { + unsubscribe?.(); + } catch (e) { + debug('Error while unsubscribing websocket: ' + (e as any)?.message); + } + try { + wsClient.deactivate(); + } catch (e) { + debug('Error while deactivating websocket client: ' + (e as any)?.message); + }[static_analysis_hints]
66-77: Consider starting the polling backup loop immediately, not only after the first WS event.
startPolling()is called only from the WebSocket message handler. If the WebSocket connects but notranslation-data-modifiedevents are emitted (or events are missed), there will be no periodic polling, and changes may only be picked up on future WS events.If you want a true “backup” poller independent of WS messages, you might call
startPolling()once during initialization:subscribe(); - schedulePull(); + schedulePull(); + startPolling();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/WebsocketClient.ts(1 hunks)src/utils/watchHandler.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/watchHandler.ts (3)
src/utils/logger.ts (4)
info(42-44)error(69-71)debug(31-35)success(51-53)src/utils/lastModifiedStorage.ts (1)
setLastModified(18-23)src/client/WebsocketClient.ts (1)
WebsocketClient(32-160)
🪛 GitHub Check: ESLint
src/utils/watchHandler.ts
[failure] 114-114:
Empty block statement
[failure] 111-111:
Empty block statement
⏰ 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). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
test/e2e/pullWatch.test.ts (2)
49-93: Move helper functions to a utility module.Biome correctly flags that exporting functions from test files is problematic. These helpers (
waitFilesystemDataUpdated,isFilesystemDataUpdated,changeLocalizationData) should be moved totest/e2e/utils/for better organization and reusability.
28-46: Critical: Missingawaitand process cleanup causing test reliability issues.
Line 29:
changeLocalizationData('A key')should be awaited to ensure data is set before the watch process starts.Lines 31-35: The spawned process is never terminated, causing resource leaks and potential 5-minute hangs.
it('pulls strings from Tolgee with watch', async () => { - changeLocalizationData('A key'); + await changeLocalizationData('A key'); - const process = run( + const watchProcess = run( ['pull', '--api-key', pak, '--path', TMP_FOLDER, '--watch', '--verbose'], undefined, 300000 ); // Tests that it pulls at the beginning... await waitFilesystemDataUpdated('A key'); // We need to sleep a bit to make sure the Last-Modified header is updated. // The Last-Modified has second precision, so we need to wait at least 1 second. await sleep(1000); // Tests that it pulls after change... await changeLocalizationData('Another key'); await waitFilesystemDataUpdated('Another key'); + + // Clean up: terminate the watch process + // Note: You may need to modify run() to expose the child process for termination + try { + (await watchProcess).kill?.('SIGTERM'); + } catch { /* Process may have already exited */ } });src/client/WebsocketClient.ts (2)
51-64: Add error handling for JSON.parse and validate client state.This concern was raised in a previous review and remains unaddressed:
- Line 57:
JSON.parse(message.body)can throw if the message body is malformed, causing the subscription callback to fail unexpectedly.- Line 52-54: The function checks
connectedbut uses_client!without verifying_clientis non-null. Defensive programming suggests checking both.Apply this diff to add error handling:
const subscribeToStompChannel = (subscription: Subscription<any>) => { - if (connected) { + if (connected && _client) { debug(`Subscribing to ${subscription.channel}`); - const stompSubscription = _client!.subscribe( + const stompSubscription = _client.subscribe( subscription.channel, function (message: any) { - const parsed = JSON.parse(message.body) as Message; - subscription.callback(parsed as any); + try { + const parsed = JSON.parse(message.body) as Message; + subscription.callback(parsed as any); + } catch (error) { + debug(`Failed to parse message from ${subscription.channel}: ${error}`); + } } ); subscription.unsubscribe = stompSubscription.unsubscribe; subscription.id = stompSubscription.id; } };
66-74: Validate serverUrl before constructing the SockJS URL.This concern was raised in a previous review and remains unaddressed:
Line 67: If
options.serverUrlis undefined or empty, the URL construction will produce an invalid URL likeundefined/websocket, causing the WebSocket connection to fail.Apply this diff to validate serverUrl:
function initClient() { + if (!options.serverUrl) { + throw new Error('serverUrl is required for WebSocket connection'); + } _client = Stomp.over(() => new SockJS(`${options.serverUrl}/websocket`)); _client.configure({ reconnectDelay: 3000, debug: (msg: string) => { debug(msg); }, }); }
🧹 Nitpick comments (4)
src/utils/pullWatch/AuthErrorHandler.ts (2)
32-49: Redundantclientparameter - already available via closure.The
isAppSupportedVersionfunction receivesclientas a parameter, butclientis already available in the outer closure fromAuthErrorHandler. The same applies togetTolgeeServerVersion.- async function isAppSupportedVersion( - client: ReturnType<typeof createTolgeeClient> - ) { + async function isAppSupportedVersion() { const serverVersion = await getTolgeeServerVersion(client); // ... } - async function getTolgeeServerVersion( - client: ReturnType<typeof createTolgeeClient> - ): Promise<string | null> { + async function getTolgeeServerVersion(): Promise<string | null> {
9-14: Use strict equality (===) for string comparisons.While
==works here,===is the idiomatic choice in TypeScript for comparing strings.- if (err?.headers?.message == 'Unauthenticated') { + if (err?.headers?.message === 'Unauthenticated') { await printUnauthenticatedError(); } - if (err?.headers?.message == 'Unauthorized') { + if (err?.headers?.message === 'Unauthorized') { error("You're not authorized. Insufficient permissions?"); }src/utils/pullWatch/watchHandler.ts (2)
85-93: Use importederror()logger instead ofconsole.errorfor consistency.The module imports
errorfrom../logger.jsbut usesconsole.errorhere. For consistent log formatting across the CLI, use the imported logger.onError: (error) => { - console.error('Error: handling auth error'); + debug('WebSocket error received, checking auth...'); AuthErrorHandler(client) .handleAuthErrors(error, shutdown) .catch((err: any) => { debug('Error in handleAuthErrors: ' + err); }); // Non-fatal: just inform info('Websocket error encountered. Reconnecting...'); },
102-102: Fix type annotation forunsubscribe.The type
() => void | undefinedmeans a function returningvoid | undefined, but the intent is thatunsubscribeitself can be undefined. Consider:- let unsubscribe: () => void | undefined; + let unsubscribe: (() => void) | undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/client/TolgeeClient.ts(1 hunks)src/client/WebsocketClient.ts(1 hunks)src/commands/pull.ts(4 hunks)src/utils/isVersionAtLeast.ts(1 hunks)src/utils/pullWatch/AuthErrorHandler.ts(1 hunks)src/utils/pullWatch/watchHandler.ts(1 hunks)test/e2e/pullWatch.test.ts(1 hunks)test/unit/isVersionAtLeast.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/TolgeeClient.ts
- src/commands/pull.ts
🧰 Additional context used
🧬 Code graph analysis (6)
src/utils/isVersionAtLeast.ts (1)
src/utils/logger.ts (1)
debug(31-35)
test/e2e/pullWatch.test.ts (5)
src/client/TolgeeClient.ts (1)
TolgeeClient(34-34)test/e2e/utils/tmp.ts (2)
setupTemporaryFolder(9-23)TMP_FOLDER(7-7)test/e2e/utils/api/common.ts (3)
createProjectWithClient(49-92)createPak(106-112)deleteProject(39-43)test/e2e/utils/api/project1.ts (1)
PROJECT_1(6-15)test/e2e/utils/run.ts (1)
run(98-105)
src/utils/pullWatch/AuthErrorHandler.ts (3)
src/client/TolgeeClient.ts (1)
createTolgeeClient(12-20)src/utils/logger.ts (2)
error(69-71)debug(31-35)src/utils/isVersionAtLeast.ts (1)
isVersionAtLeast(7-35)
src/utils/pullWatch/watchHandler.ts (5)
src/client/TolgeeClient.ts (1)
createTolgeeClient(12-20)src/utils/logger.ts (4)
info(42-44)error(69-71)debug(31-35)success(51-53)src/utils/lastModifiedStorage.ts (1)
setLastModified(18-23)src/client/WebsocketClient.ts (1)
WebsocketClient(32-160)src/utils/pullWatch/AuthErrorHandler.ts (1)
AuthErrorHandler(5-65)
test/unit/isVersionAtLeast.test.ts (1)
src/utils/isVersionAtLeast.ts (1)
isVersionAtLeast(7-35)
src/client/WebsocketClient.ts (1)
src/utils/logger.ts (2)
error(69-71)debug(31-35)
🪛 Biome (2.1.2)
test/e2e/pullWatch.test.ts
[error] 48-62: Do not export from a test file.
(lint/suspicious/noExportsInTest)
[error] 63-79: Do not export from a test file.
(lint/suspicious/noExportsInTest)
[error] 80-93: Do not export from a test file.
(lint/suspicious/noExportsInTest)
🪛 GitHub Check: ESLint
test/e2e/pullWatch.test.ts
[warning] 31-31:
'process' is assigned a value but never used. Allowed unused vars must match /^_/u
src/utils/pullWatch/watchHandler.ts
[failure] 118-118:
Empty block statement
[failure] 115-115:
Empty block statement
⏰ 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). (4)
- GitHub Check: Unit Tests (windows-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
🔇 Additional comments (3)
src/utils/isVersionAtLeast.ts (1)
7-35: LGTM! Clean semver comparison implementation.The logic correctly handles version comparison with proper early returns. The handling of
??for local builds and strippingvprefixes is well-documented.One edge case to be aware of: versions with non-numeric suffixes (e.g.,
1.0.0-beta) will produceNaNvalues frommap(Number), which could cause unexpected comparison results. If pre-release version support is needed in the future, consider using a dedicated semver library.test/unit/isVersionAtLeast.test.ts (1)
1-117: LGTM! Comprehensive test coverage.Excellent test organization with clear describe blocks covering equality, greater-than/less-than semantics, edge cases (local build
??), prefix handling, and mixed-length versions. The test cases provide strong confidence in the implementation's correctness.src/utils/pullWatch/watchHandler.ts (1)
21-137: LGTM overall - well-structured watch handler.The implementation correctly:
- Uses debouncing to prevent excessive pulls
- Has a polling fallback mechanism
- Handles graceful shutdown with signal handlers
- Integrates with lastModified storage for HTTP caching
- Properly guards against concurrent pulls with the
pullingflag
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/e2e/pullWatch.test.ts (2)
49-93: Move helper functions to a utility module or remove exports.Biome correctly flags that exporting from test files is problematic. If these helpers are only used within this file, remove the
exportkeywords. If they need to be shared across tests, move them totest/e2e/utils/.Quick fix if helpers are only used locally:
-export async function waitFilesystemDataUpdated(aKeyValue: string) { +async function waitFilesystemDataUpdated(aKeyValue: string) {-export async function isFilesystemDataUpdated(newValue: string) { +async function isFilesystemDataUpdated(newValue: string) {-export async function changeLocalizationData(newEnText: string) { +async function changeLocalizationData(newEnText: string) {
31-46: Process cleanup is still missing - test reliability concern.The spawned watch process is assigned to
processbut never terminated. This will cause:
- The process to run for up to 5 minutes until the timeout
- Potential resource leaks and port conflicts between tests
- ESLint warning about unused variable
Apply this diff to properly clean up the process:
- const process = run( + const childProcess = run( ['pull', '--api-key', pak, '--path', TMP_FOLDER, '--watch', '--verbose'], undefined, 300000 ); // Tests that it pulls at the beginning... await waitFilesystemDataUpdated('A key'); // We need to sleep a bit to make sure the Last-Modified header is updated. // The Last-Modified has second precision, so we need to wait at least 1 second. await sleep(1000); // Tests that it pulls after change... await changeLocalizationData('Another key'); await waitFilesystemDataUpdated('Another key'); + + // Clean up the watch process + const result = await childProcess; + // Note: You may need to modify run() to expose the underlying process for terminationNote: The
run()utility returnsPromise<RunResult>. You may need to extend it to provide a way to terminate long-running watch processes, or use a different approach that gives you control over the child process lifecycle.
🧹 Nitpick comments (6)
test/unit/pullWatch/AuthErrorHandler.test.ts (3)
20-20: Remove stale comment.This comment references removed code and provides no value.
-// Mock console.log to capture output (removed since it's not working properly with vi.mock)
61-65: Rename localerrorvariable to avoid shadowing the importederrorfunction.The local variable
errorshadows the importederrorfunction from the logger module, which can cause confusion when reading the tests.- const error = { + const authError = { headers: { message: 'Unauthenticated', }, }; - await authErrorHandler.handleAuthErrors(error, mockShutdown); + await authErrorHandler.handleAuthErrors(authError, mockShutdown);Apply the same renaming pattern to other test cases (lines 91, 112, 138, 155, 170, 209).
169-182: Consider testing withnullorundefinederror input.The current "other errors" test uses an error with a different message. Consider also testing how
handleAuthErrorsbehaves when called withnull,undefined, or an error object without aheadersproperty, as these could occur in real-world scenarios.it('should handle null/undefined errors gracefully', async () => { await authErrorHandler.handleAuthErrors(null, mockShutdown); expect(mockShutdown).toHaveBeenCalled(); vi.clearAllMocks(); await authErrorHandler.handleAuthErrors(undefined, mockShutdown); expect(mockShutdown).toHaveBeenCalled(); }); it('should handle errors without headers property', async () => { await authErrorHandler.handleAuthErrors({ message: 'error' }, mockShutdown); expect(mockShutdown).toHaveBeenCalled(); });src/utils/pullWatch/AuthErrorHandler.ts (3)
8-15: Use strict equality (===) for string comparisons.Lines 9 and 12 use loose equality (
==). While this works for string-to-string comparison, strict equality is the idiomatic choice in TypeScript.async function handleAuthErrors(err: any, shutdown: () => void) { - if (err?.headers?.message == 'Unauthenticated') { + if (err?.headers?.message === 'Unauthenticated') { await printUnauthenticatedError(); } - if (err?.headers?.message == 'Unauthorized') { + if (err?.headers?.message === 'Unauthorized') { error("You're not authorized. Insufficient permissions?"); } shutdown(); }
24-27: Improve error message when server version is unknown.When
serverVersionisnull, the error message displays "Server version null does not support..." which is confusing for users. Consider a more informative message.+ async function printUnauthenticatedError() { + const { isSupported, serverVersion } = await isAppSupportedVersion(client); + if (isSupported) { + error("You're not authenticated. Invalid API key?"); + return; + } + if (!serverVersion) { + error( + `Could not determine server version. CLI watch mode requires Tolgee server version ${REQUIRED_VERSION} or higher.` + ); + return; + } error( `Server version ${serverVersion} does not support CLI watch mode. Please update your Tolgee server to version ${REQUIRED_VERSION} or higher.` ); - return; }Note: This would require updating the corresponding unit test expectations.
65-65: Consider moving constant to the top of the file.
REQUIRED_VERSIONis defined at the bottom but used throughout the module. Convention typically places constants near the top after imports for better visibility.import { debug, error } from '../logger.js'; import { isVersionAtLeast } from '../isVersionAtLeast.js'; import { createTolgeeClient } from '../../client/TolgeeClient.js'; + +const REQUIRED_VERSION = '3.143.0'; export function AuthErrorHandler(And remove from line 65.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/utils/pullWatch/AuthErrorHandler.ts(1 hunks)test/e2e/pullWatch.test.ts(1 hunks)test/unit/pullWatch/AuthErrorHandler.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/unit/pullWatch/AuthErrorHandler.test.ts (4)
src/client/TolgeeClient.ts (1)
createTolgeeClient(12-20)src/utils/pullWatch/AuthErrorHandler.ts (1)
AuthErrorHandler(5-63)src/utils/isVersionAtLeast.ts (1)
isVersionAtLeast(7-35)src/utils/logger.ts (1)
error(69-71)
src/utils/pullWatch/AuthErrorHandler.ts (3)
src/client/TolgeeClient.ts (1)
createTolgeeClient(12-20)src/utils/logger.ts (2)
error(69-71)debug(31-35)src/utils/isVersionAtLeast.ts (1)
isVersionAtLeast(7-35)
test/e2e/pullWatch.test.ts (5)
src/client/TolgeeClient.ts (1)
TolgeeClient(34-34)test/e2e/utils/tmp.ts (2)
setupTemporaryFolder(9-23)TMP_FOLDER(7-7)test/e2e/utils/api/common.ts (3)
createProjectWithClient(49-92)createPak(106-112)deleteProject(39-43)test/e2e/utils/api/project1.ts (1)
PROJECT_1(6-15)test/e2e/utils/run.ts (1)
run(98-105)
🪛 Biome (2.1.2)
test/e2e/pullWatch.test.ts
[error] 48-62: Do not export from a test file.
(lint/suspicious/noExportsInTest)
[error] 63-79: Do not export from a test file.
(lint/suspicious/noExportsInTest)
[error] 80-93: Do not export from a test file.
(lint/suspicious/noExportsInTest)
🪛 GitHub Check: ESLint
test/unit/pullWatch/AuthErrorHandler.test.ts
[failure] 220-220:
Replace "You're·not·authenticated.·Invalid·API·key?" with ⏎············"You're·not·authenticated.·Invalid·API·key?"⏎··········
[failure] 217-217:
Replace '3.143.0',·testCase.version with ⏎··········'3.143.0',⏎··········testCase.version⏎········
[failure] 165-165:
Replace "You're·not·authorized.·Insufficient·permissions?" with ⏎········"You're·not·authorized.·Insufficient·permissions?"⏎······
[failure] 101-101:
Replace '3.143.0',·unsupportedVersion with ⏎········'3.143.0',⏎········unsupportedVersion⏎······
[failure] 72-72:
Replace "You're·not·authenticated.·Invalid·API·key?" with ⏎········"You're·not·authenticated.·Invalid·API·key?"⏎······
test/e2e/pullWatch.test.ts
[warning] 31-31:
'process' is assigned a value but never used. Allowed unused vars must match /^_/u
⏰ 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). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
🔇 Additional comments (1)
src/utils/pullWatch/AuthErrorHandler.ts (1)
5-62: Overall structure looks good.The factory pattern with a frozen return object provides good encapsulation. Internal helpers are properly scoped, and error handling is appropriately defensive with optional chaining and try-catch blocks.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
src/commands/pull.ts (1)
69-86: Dynamic import ofsetLastModifiedis inconsistent (and already raised before).If there’s no circular-dep reason, statically import
setLastModifiedalongsidegetLastModified/extractLastModifiedFromResponse.src/client/WebsocketClient.ts (1)
8-17: MakeserverUrlrequired (or validate) before constructing SockJS URL.Right now
${options.serverUrl}/websocketcan becomeundefined/websocketif the parameter is omitted, sinceserverUrlis optional in the type definition but used without validation on line 71.type WebsocketClientOptions = { - serverUrl?: string; + serverUrl: string; authentication: { jwtToken?: string; apiKey?: string; }; onConnected?: (message: any) => void; onError?: (error: any) => void; onConnectionClose?: () => void; };If making it required is not feasible, add a runtime guard in
initClient().
🧹 Nitpick comments (3)
test/e2e/utils/sleep.ts (1)
1-3: Add an explicit return type (Promise<void>) for clarity/TS strictness.-export function sleep(ms: number) { +export function sleep(ms: number): Promise<void> { return new Promise((resolve) => setTimeout(resolve, ms)); }test/e2e/utils/pullWatchUtil.ts (1)
9-22: Polling loop looks fine; consider making timeout configurable for slower CI.Hardcoding
maxAttempts = 1000andsleep(10)is ~10s. If CI is slower, making these optional params (or env-driven) can reduce flakes.src/utils/pullWatch/watchHandler.ts (1)
82-98: Avoid mixingconsole.*with your logger + clarify WS error severity.Prefer
error(...)/debug(...)overconsole.error('Error: handling auth error')to keep output consistent (and controllable via--verbose).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/client/WebsocketClient.ts(1 hunks)src/commands/pull.ts(4 hunks)src/utils/lastModifiedStorage.ts(1 hunks)src/utils/pullWatch/watchHandler.ts(1 hunks)test/e2e/__internal__/setup.ts(1 hunks)test/e2e/pullWatch.test.ts(1 hunks)test/e2e/utils/pullWatchUtil.ts(1 hunks)test/e2e/utils/sleep.ts(1 hunks)test/unit/pullWatch/AuthErrorHandler.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/unit/pullWatch/AuthErrorHandler.test.ts
- src/utils/lastModifiedStorage.ts
- test/e2e/internal/setup.ts
🧰 Additional context used
🧬 Code graph analysis (4)
test/e2e/utils/pullWatchUtil.ts (3)
src/client/TolgeeClient.ts (1)
TolgeeClient(34-34)test/e2e/utils/sleep.ts (1)
sleep(1-3)test/e2e/utils/tmp.ts (1)
TMP_FOLDER(7-7)
src/client/WebsocketClient.ts (1)
src/utils/logger.ts (2)
error(69-71)debug(31-35)
src/commands/pull.ts (7)
src/utils/logger.ts (3)
exitWithError(103-116)debug(31-35)info(42-44)src/utils/checkPathNotAFile.ts (1)
checkPathNotAFile(4-18)src/utils/pullWatch/watchHandler.ts (1)
startWatching(21-141)src/utils/lastModifiedStorage.ts (3)
getLastModified(5-13)setLastModified(15-19)extractLastModifiedFromResponse(21-25)src/utils/prepareDir.ts (1)
prepareDir(4-15)src/utils/zip.ts (1)
unzipBuffer(32-45)src/client/TolgeeClient.ts (1)
handleLoadableError(22-26)
src/utils/pullWatch/watchHandler.ts (5)
src/client/TolgeeClient.ts (1)
createTolgeeClient(12-20)src/utils/logger.ts (4)
info(42-44)error(69-71)debug(31-35)success(51-53)src/utils/lastModifiedStorage.ts (1)
setLastModified(15-19)src/client/WebsocketClient.ts (1)
WebsocketClient(32-164)src/utils/pullWatch/AuthErrorHandler.ts (1)
AuthErrorHandler(5-63)
⏰ 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). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
🔇 Additional comments (1)
src/commands/pull.ts (1)
88-120: The use of If-Modified-Since with POST is non-standard HTTP; verify Tolgee server's actual response.Sending
If-Modified-Sinceheader with a POST request deviates from HTTP specification, which reservesIf-Modified-Sincefor conditional GET/HEAD requests. Per HTTP specs, conditional headers on POST should useIf-MatchorIf-Unmodified-Since(which return 412 Precondition Failed on failure). The Tolgee server may return 304, 412, 200, or handle this unexpectedly. Verify the actual status codes the Tolgee export endpoint returns for this scenario, and consider whether a GET request or standard conditional headers would be more appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/utils/pullWatch/watchHandler.ts (1)
83-113: Polling fallback still won’t start if WS never delivers a message.
startPolling()is only called from the subscription callback (Line 123-124). If the WS connection fails early (or auth fails), polling never starts, despite being described as a backup.Suggestion: start polling immediately (or at least on
onError/onConnectionClose) and keepstartPolling()idempotent (it already clears any existing interval).Also applies to: 119-125, 151-156
src/client/WebsocketClient.ts (1)
70-78: Validate/normalizeoptions.serverUrlbefore creating SockJS client.
serverUrlis optional (Line 9) but used unconditionally (Line 71). This can buildundefined/websocketat runtime in other call sites.function initClient() { - _client = Stomp.over(() => new SockJS(`${options.serverUrl}/websocket`)); + if (!options.serverUrl) { + throw new Error('WebsocketClient: serverUrl is required'); + } + const base = options.serverUrl.replace(/\/+$/, ''); + _client = Stomp.over(() => new SockJS(`${base}/websocket`)); _client.configure({ reconnectDelay: 3000, debug: (msg: string) => { debug(msg); }, }); }
🧹 Nitpick comments (3)
test/e2e/utils/run.ts (1)
107-117: LGTM! Appropriate for watch mode test orchestration.The function provides manual process control needed for watch mode tests while maintaining the existing timeout safety net. The API is clean and follows established patterns in this file.
Optionally, you could capture the return value of
kill()for additional robustness:export function runWithKill( args: string[], env?: Record<string, string>, timeout = 10e3 ) { const cliProcess = spawn(args, false, env); return { promise: runProcess(cliProcess, timeout), - kill: (signal: NodeJS.Signals) => cliProcess.kill(signal), + kill: (signal: NodeJS.Signals) => { + const success = cliProcess.kill(signal); + if (!success) { + console.warn(`Failed to send signal ${signal} to process ${cliProcess.pid}`); + } + return success; + }, }; }This would help debug test failures if process termination doesn't work as expected, though the existing implementation is adequate for the test utility's purpose.
src/utils/pullWatch/watchHandler.ts (1)
119-125: Avoid “floating” async call from WS callback (prevent unhandled rejections).
Line 122 callsschedulePull()without awaiting. Even if today it’s unlikely to throw, it’s safer to make intent explicit:- schedulePull(); + void schedulePull();src/client/WebsocketClient.ts (1)
96-105: Consider clearing per-subscriptionid/unsubscribeon disconnect (stale handles across reconnects).
On reconnect,resubscribe()re-subscribes, but oldsubscription.unsubscribe/subscription.idcan remain stale and misleading. Clearing them ononDisconnectis cheap and keeps internal state consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/WebsocketClient.ts(1 hunks)src/utils/pullWatch/watchHandler.ts(1 hunks)test/e2e/pullWatch.test.ts(1 hunks)test/e2e/utils/run.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/pullWatch.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/pullWatch/watchHandler.ts (5)
src/client/TolgeeClient.ts (1)
createTolgeeClient(12-20)src/utils/logger.ts (4)
info(42-44)error(69-71)debug(31-35)success(51-53)src/utils/lastModifiedStorage.ts (1)
setLastModified(15-19)src/client/WebsocketClient.ts (1)
WebsocketClient(32-164)src/utils/pullWatch/AuthErrorHandler.ts (1)
AuthErrorHandler(5-63)
⏰ 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). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
🔇 Additional comments (2)
test/e2e/utils/run.ts (1)
41-41: Good addition for test flexibility.Using the environment variable allows E2E tests to target different backend instances while maintaining a sensible default.
src/client/WebsocketClient.ts (1)
51-68: Message parsing guard looks good.
Try/catch aroundJSON.parse+ logging prevents malformed payloads from crashing the client.
- Last modified has 1s precision, which is not optimal
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/commands/pull.ts (1)
66-85: Avoid loggingETag: undefined+ prefer static import forsetETag(if no circular dep).
- Guard the info log so it doesn’t print
undefined.- Unless there’s a known circular dependency reason, the dynamic import for
setETagis inconsistent with the rest of the file.src/utils/pullWatch/watchHandler.ts (2)
40-64: Fix broken “pending pull” logic (follow-up pull is currently dropped).
executePull()is invoked whilepulling === true(Line 57-62), so the recursive call immediately setspending = trueand returns; thenpullingis set to false and nothing re-triggers the pending pull.const executePull = async (etag?: string) => { if (pulling) { pending = true; return; } pulling = true; @@ } finally { - // If there was a pending pull (data changed when pulling), execute it now - if (pending) { - pending = false; - void executePull(); - } pulling = false; + // If there was a pending pull (data changed when pulling), execute it now + if (pending) { + pending = false; + void executePull(); + } } };
83-124: Polling fallback still won’t start if WS never connects / never emits.
startPolling()is only called inside the subscription callback (Line 120-124), so a dead WS connection means no fallback polling at all.
🧹 Nitpick comments (1)
src/utils/eTagStorage.ts (1)
1-25: LGTM; consider storingMap<number, string>unless you expect metadata soon.Current
{ etag: string }wrapper adds indirection without benefit unless you plan to extend stored fields imminently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/client/ExportClient.ts(2 hunks)src/commands/pull.ts(4 hunks)src/utils/eTagStorage.ts(1 hunks)src/utils/pullWatch/watchHandler.ts(1 hunks)test/e2e/pullWatch.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/ExportClient.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/utils/pullWatch/watchHandler.ts (5)
src/client/TolgeeClient.ts (1)
createTolgeeClient(12-20)src/utils/logger.ts (4)
info(42-44)error(69-71)debug(31-35)success(51-53)src/utils/eTagStorage.ts (1)
setETag(15-19)src/client/WebsocketClient.ts (1)
WebsocketClient(32-164)src/utils/pullWatch/AuthErrorHandler.ts (1)
AuthErrorHandler(5-63)
test/e2e/pullWatch.test.ts (6)
src/client/TolgeeClient.ts (1)
TolgeeClient(34-34)test/e2e/utils/pullWatchUtil.ts (1)
PullWatchUtil(6-54)test/e2e/utils/tmp.ts (2)
setupTemporaryFolder(9-23)TMP_FOLDER(7-7)test/e2e/utils/api/common.ts (1)
createProjectWithClient(49-92)test/e2e/utils/api/project1.ts (1)
PROJECT_1(6-15)test/e2e/utils/run.ts (1)
runWithKill(107-117)
🪛 GitHub Check: ESLint
test/e2e/pullWatch.test.ts
[failure] 47-47:
'newEnText' is never reassigned. Use 'const' instead
⏰ 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). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
🔇 Additional comments (1)
src/commands/pull.ts (1)
87-119: RemoveifNoneMatchparameter: Tolgee API does not support conditional POSTs with If-None-Match headers.The Tolgee API export endpoint (POST
/v2/projects/:projectId/export) does not document or support ETag-based conditional requests with If-None-Match headers. ETag/If-None-Match is designed for conditional GETs, not POSTs. PassingifNoneMatchtoopts.client.export.export()will be silently ignored by the API, and the response will never include a 412 or 304 status code. The code currently expects conditional behavior that the API does not provide.If conditional export caching is needed, implement client-side caching or request a feature from Tolgee.
(Note:
handleLoadableErroris safe to call before status checks since it only throws ifloadable.erroris set, not based on HTTP status codes.)Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/commands/pull.ts (1)
69-69: Guard against loggingETag: undefined.When
result.etagis absent, this will log "ETag: undefined". Consider only logging when present or adjusting the message format.Apply this diff to guard the log:
- info(`ETag: ${result.etag} (${new Date().toLocaleString()})`); + if (result.etag) { + info(`ETag: ${result.etag} (${new Date().toLocaleString()})`); + }
🧹 Nitpick comments (2)
src/utils/logger.ts (1)
125-135: Nice non-TTY output fix; consider guarding against double-newlines incomment.
process.stdout.write(comment + '\n')makes CI/non-interactive logs much cleaner. Only possible nit: if any caller passes acommentalready ending in\n, this will add an extra blank line—optional to normalize viacomment.replace(/\n$/, '')(or similar).src/commands/pull.ts (1)
74-75: Consolidate the dynamic import ofsetETag.You're statically importing
getETagandextractETagFromResponsefrom the same module on line 13, but dynamically importingsetETaghere. Unless there's a specific reason (e.g., circular dependency), consolidate all three at the top for consistency.Apply this diff to consolidate imports:
import { extractETagFromResponse, getETag, + setETag, } from '../utils/eTagStorage.js';And update the usage:
// Store ETag after a successful pull if (result.etag) { - const { setETag } = await import('../utils/eTagStorage.js'); setETag(opts.projectId, result.etag); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/commands/pull.ts(4 hunks)src/utils/logger.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/commands/pull.ts (7)
src/utils/logger.ts (3)
exitWithError(103-116)loading(125-155)info(42-44)src/utils/checkPathNotAFile.ts (1)
checkPathNotAFile(4-18)src/utils/pullWatch/watchHandler.ts (1)
startWatching(21-156)src/utils/eTagStorage.ts (3)
getETag(5-13)setETag(15-19)extractETagFromResponse(21-25)src/utils/prepareDir.ts (1)
prepareDir(4-15)src/utils/zip.ts (1)
unzipBuffer(32-45)src/client/TolgeeClient.ts (1)
handleLoadableError(22-26)
⏰ 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). (4)
- GitHub Check: Unit Tests (windows-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
🔇 Additional comments (2)
src/commands/pull.ts (2)
30-58: LGTM! Clean separation of concerns.The refactored handler properly validates the path, checks it's not a file, then branches between single-pull and watch modes. The watch mode integration with
startWatchingis well-structured.
79-111: Verify the Tolgee API's handling of conditional requests with 412 status code.The
ifNoneMatchparameter is properly supported and correctly passed to the export API via theIf-None-Matchheader. However, the code treats HTTP 412 (Precondition Failed) as "not modified" on line 109, which contradicts standard HTTP semantics. In standard HTTP, 412 indicates the precondition failed (ETag doesn't match), not that the resource is unchanged. Please confirm this is intentional Tolgee API behavior by checking the API documentation, or clarify if 412 should instead trigger an error condition rather than be treated as "data unchanged."
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/utils/pullWatch/watchHandler.ts (2)
105-112: onError handler has conflicting logic.When
AuthErrorHandlerencounters an authentication or authorization error, it callsshutdown()which exits the process (line 149). However, line 112 then logs "Reconnecting..." which will never execute after shutdown. This makes the "Reconnecting..." message misleading.Consider restructuring the error handling to only call
shutdown()for auth errors, and allow reconnection for other WebSocket errors:onError: (error) => { + const isAuthError = error?.headers?.message === 'Unauthenticated' || + error?.headers?.message === 'Forbidden'; + if (isAuthError) { AuthErrorHandler(client) .handleAuthErrors(error, shutdown) .catch((err: any) => { debug('Error in handleAuthErrors: ' + err); }); + return; + } // Non-fatal: just inform info('Websocket error encountered. Reconnecting...'); + startPolling(); },
88-100: Polling never starts if WebSocket fails to connect or receive events.The
startPolling()function is only invoked inside the WebSocket event handler (line 127), meaning if the WebSocket connection fails or never receives a translation-data-modified event, polling never begins. This undermines the "backup" mechanism described in the comment at line 8.Consider starting polling immediately after its definition (e.g., call
startPolling()at line 101) or in the WebSocketonErrorandonConnectionClosehandlers to ensure it serves as a true fallback.Apply this diff to start polling immediately:
// Set up periodic polling pollingTimer = setInterval(poll, POLLING_INTERVAL_SECONDS * 1000); }; + + // Start polling immediately as a fallback + startPolling();Or add to error handlers:
onError: (error) => { AuthErrorHandler(client) .handleAuthErrors(error, shutdown) .catch((err: any) => { debug('Error in handleAuthErrors: ' + err); }); // Non-fatal: just inform info('Websocket error encountered. Reconnecting...'); + startPolling(); }, onConnectionClose: () => { info('Websocket connection closed. Attempting to reconnect...'); + startPolling(); },
🧹 Nitpick comments (4)
src/utils/pullWatch/AuthErrorHandler.ts (4)
8-18: Use strict equality operators.Lines 9 and 14 use loose equality (
==) when comparingerr?.headers?.message. Prefer strict equality (===) for consistency and to avoid unexpected type coercion.Apply this diff:
async function handleAuthErrors(err: any, shutdown: () => void) { - if (err?.headers?.message == 'Unauthenticated') { + if (err?.headers?.message === 'Unauthenticated') { await printUnauthenticatedError(); shutdown(); return; } - if (err?.headers?.message == 'Forbidden') { + if (err?.headers?.message === 'Forbidden') { error("You're not authorized. Insufficient permissions?"); shutdown(); return; } }
21-31: Optional: Remove unnecessary return statements.The explicit
returnstatements at lines 25 and 30 are not needed since the function returnsPromise<void>and there's no code after these branches.Apply this diff:
async function printUnauthenticatedError() { const { isSupported, serverVersion } = await isAppSupportedVersion(client); if (isSupported) { error("You're not authenticated. Invalid API key?"); - return; } - error( - `Server version ${serverVersion} does not support CLI watch mode. Please update your Tolgee server to version ${REQUIRED_VERSION} or higher.` - ); - return; + else { + error( + `Server version ${serverVersion} does not support CLI watch mode. Please update your Tolgee server to version ${REQUIRED_VERSION} or higher.` + ); + } }
33-50: Use ES6 object property shorthand.Lines 40-43 and 46-49 can use object property shorthand since the property name matches the variable name.
Apply this diff:
async function isAppSupportedVersion( client: ReturnType<typeof createTolgeeClient> ) { const serverVersion = await getTolgeeServerVersion(client); if (!serverVersion) { debug('Could not determine server version'); return { isSupported: false, - serverVersion: serverVersion, + serverVersion, }; } return { isSupported: isVersionAtLeast(REQUIRED_VERSION, serverVersion), - serverVersion: serverVersion, + serverVersion, }; }
68-68: Consider moving constant to the top of the file.The
REQUIRED_VERSIONconstant is defined after the factory function, which works due to hoisting but is unconventional. Consider moving it near the imports (e.g., after line 3) for better code organization and readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/utils/pullWatch/AuthErrorHandler.ts(1 hunks)src/utils/pullWatch/watchHandler.ts(1 hunks)test/e2e/pullWatch.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/pullWatch.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/pullWatch/watchHandler.ts (5)
src/client/TolgeeClient.ts (1)
createTolgeeClient(12-20)src/utils/logger.ts (4)
info(42-44)error(69-71)debug(31-35)success(51-53)src/utils/eTagStorage.ts (1)
setETag(15-19)src/client/WebsocketClient.ts (1)
WebsocketClient(32-164)src/utils/pullWatch/AuthErrorHandler.ts (1)
AuthErrorHandler(5-66)
⏰ 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). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
🔇 Additional comments (8)
src/utils/pullWatch/AuthErrorHandler.ts (1)
52-63: LGTM!The function correctly fetches the server version from the configuration endpoint and handles errors gracefully with debug logging.
src/utils/pullWatch/watchHandler.ts (7)
1-11: LGTM!Imports and constants are well-organized and appropriately documented.
13-19: LGTM!The
WatchHandlerOptionstype is well-defined with appropriate fields for the watch functionality.
41-68: LGTM!The pending pull logic correctly captures the ETag when a pull is requested during an active pull, then triggers a follow-up pull with the captured ETag after the current pull completes.
70-85: LGTM!The debouncing logic correctly executes immediately if enough time has passed or schedules with a delay otherwise, preventing excessive pull operations.
119-129: Subscription logic is correct.The WebSocket subscription correctly schedules a pull when translation data changes are detected.
131-150: LGTM!The shutdown function properly cleans up all resources (WebSocket subscription, timers) and gracefully exits the process.
152-160: Initialization structure is sound.The signal handlers ensure graceful shutdown, and the immediate
schedulePull()triggers an initial pull before watching begins. The pending promise correctly keeps the process alive.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/commands/pull.ts (1)
136-137: Consolidate thesetETagimport at the top.This dynamic import was flagged in previous review comments. Since
getETagis already statically imported from the same module (line 13),setETagshould also be imported statically unless there's a circular dependency.Apply this diff to consolidate the import:
-import { getETag } from '../utils/eTagStorage.js'; +import { getETag, setETag } from '../utils/eTagStorage.js';Then update the usage:
// Store ETag after a successful pull if (result.etag) { - const { setETag } = await import('../utils/eTagStorage.js'); setETag(opts.projectId, result.etag); }
🧹 Nitpick comments (1)
src/commands/pull.ts (1)
175-177: LGTM! Helper correctly extracts ETag header.The function appropriately handles the case where the ETag header is absent by returning
undefined.Optional: You could use nullish coalescing (
??) instead of logical OR (||) for stricter null handling:- return response.headers.get('ETag') || undefined; + return response.headers.get('ETag') ?? undefined;This only matters if an empty-string ETag is possible (which is unlikely).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/commands/pull.ts(2 hunks)src/utils/eTagStorage.ts(1 hunks)test/e2e/utils/run.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/utils/run.ts
- src/utils/eTagStorage.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/commands/pull.ts (8)
src/utils/logger.ts (3)
exitWithError(103-116)success(51-53)info(42-44)src/utils/checkPathNotAFile.ts (1)
checkPathNotAFile(4-18)src/utils/pullWatch/watchHandler.ts (1)
startWatching(21-160)src/utils/eTagStorage.ts (2)
getETag(5-13)setETag(15-19)src/utils/prepareDir.ts (1)
prepareDir(4-15)src/utils/mapExportFormat.ts (1)
mapExportFormat(9-96)src/client/TolgeeClient.ts (1)
handleLoadableError(22-26)src/schema.d.ts (1)
Schema(78-250)
⏰ 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). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
🔇 Additional comments (5)
src/commands/pull.ts (5)
7-7: LGTM! New imports support watch functionality.The additional imports (
info,loading,startWatching,getETag) are all used appropriately throughout the file to implement the watch mode feature.Also applies to: 12-13
84-89: LGTM! Watch option is well-defined.The
--watchoption follows the established pattern and appropriately defaults tofalse.
92-120: LGTM! Handler correctly branches between single-pull and watch modes.The refactored handler:
- Validates the path upfront before any mode-specific logic
- Cleanly separates single-pull (lines 104-108) and watch (lines 111-119) workflows
- Properly wraps
doPullfor the watch handler
179-192: LGTM! Type definition correctly includes watch field.The
PullOptionstype is properly extended with the optionalwatchfield.
168-171: Confirm this behavior against Tolgee API documentation.The code uses status 412 to detect "not modified" for POST requests with
If-None-Match, which is non-standard HTTP behavior. While the code comment acknowledges this design (referencing MDN's 412 status), proper verification requires confirming this matches Tolgee's actual API specification. Consider linking to the export endpoint documentation in the comment for future clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/test.yml (2)
74-75: Consider removing commented code or documenting its purpose.The commented-out Docker pull step adds clutter. If this is intentionally left for future debugging, consider adding a comment explaining why. Otherwise, remove it to keep the workflow clean.
78-79: Consider removing or conditionalizing RUNNER_DEBUG.
RUNNER_DEBUG: 1enables verbose GitHub Actions logging, which is typically used during development. If this was added for debugging and is no longer needed, consider removing it to reduce log verbosity in production runs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(1 hunks)
⏰ 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). (4)
- GitHub Check: Unit Tests (windows-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
🔇 Additional comments (2)
.github/workflows/test.yml (2)
87-93: LGTM! Artifact upload is well-configured.The step correctly uses
if: always()to ensure logs are uploaded even on test failure, includes the Node version in the artifact name for traceability, and sets a reasonable 7-day retention period.
82-86: Good implementation of log capture with appropriate error handling.The step correctly uses
if: always()to capture logs even when tests fail, and includes proper error handling with the|| echofallback. The container nametolgee_cli_e2ecorrectly matches the container created bynpm run tolgee:start.
JanCizmar
force-pushed
the
jancizmar/pull-watch
branch
from
56cd67d to
f2130f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/login.test.ts (1)
36-37: Good addition for test isolation.Adding the logout command in
afterAllensures authentication state is properly cleaned up between test suites, preventing potential pollution in subsequent tests.Minor optional suggestion: The comment could be slightly clearer as "Cleanup authentication to prevent pollution to next tests" but this is purely stylistic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/test.yml(1 hunks)src/commands/login.ts(2 hunks)test/e2e/login.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yml
🧰 Additional context used
🧬 Code graph analysis (2)
src/commands/login.ts (2)
src/utils/logger.ts (1)
debug(31-35)src/utils/getStackTrace.ts (1)
getStackTrace(1-8)
test/e2e/login.test.ts (1)
test/e2e/utils/run.ts (1)
run(98-104)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/client/WebsocketClient.ts (1)
70-78: ValidateserverUrlbefore constructing the SockJS URL.If
options.serverUrlis undefined, this produces an invalid URLundefined/websocket. Add validation to fail fast with a clear error.Apply this diff:
function initClient() { + if (!options.serverUrl) { + throw new Error('serverUrl is required for WebSocket connection'); + } client = Stomp.over(() => new SockJS(`${options.serverUrl}/websocket`));
🧹 Nitpick comments (2)
src/commands/pull.ts (1)
122-138: Consider consolidating thesetETagimport at the top of the file.The
getETagis statically imported at line 13, butsetETagis dynamically imported here. Unless there's a specific reason (circular dependency, lazy loading for performance), consolidate for consistency.Apply this diff:
-import { getETag } from '../utils/eTagStorage.js'; +import { getETag, setETag } from '../utils/eTagStorage.js';And update the usage:
// Store ETag after a successful pull if (result.etag) { - const { setETag } = await import('../utils/eTagStorage.js'); setETag(opts.projectId, result.etag); }Based on the past review comment that flagged the same pattern with
setLastModified.src/client/WebsocketClient.ts (1)
85-113: Avoid shadowing the module-levelclientvariable.Line 87 declares
const client = getClient()which shadows thelet clientat line 33. While functionally correct (both reference the same object), this shadowing is confusing and error-prone for future modifications.Apply this diff:
connecting = true; - const client = getClient(); + const stompClient = getClient(); const onConnected = function (message: any) { connected = true; connecting = false; resubscribe(); options.onConnected?.(message); }; const onDisconnect = function () { connected = false; connecting = false; options.onConnectionClose?.(); }; const onError = (error: any) => { connecting = false; options.onError?.(error); }; - client.connect( + stompClient.connect( getAuthentication(options), onConnected, onError, onDisconnect ); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/workflows/test.yml(2 hunks)package.json(3 hunks)src/client/WebsocketClient.ts(1 hunks)src/commands/login.ts(1 hunks)src/commands/pull.ts(2 hunks)test/unit/credentials.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/commands/login.ts
🧰 Additional context used
🧬 Code graph analysis (1)
test/unit/credentials.test.ts (1)
src/config/credentials.ts (1)
getApiKey(109-153)
⏰ 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). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
🔇 Additional comments (8)
test/unit/credentials.test.ts (1)
115-119: LGTM! Test updates correctly adapt to the new API signature.The test calls to
getApiKeyhave been correctly updated to pass string URLs (TG_X.toString()) instead of URL objects, aligning with the updated function signature that now acceptsapiUrl: stringas the first parameter.Also applies to: 129-129, 137-137
.github/workflows/test.yml (3)
27-44: LGTM: Well-structured type check job.The new type-checking job follows the established pattern and appropriately runs on a single Node version, as type checks are runtime-independent.
94-95: Verify RUNNER_DEBUG placement.Setting
RUNNER_DEBUGin a step'senvblock may not enable runner diagnostics as intended. This variable typically needs to be set at the workflow or job level to control GitHub Actions runner debug logging.If debug logging is needed for the entire test-e2e job, consider moving it to the job level:
test-e2e: + env: + RUNNER_DEBUG: 1 strategy: matrix:Alternatively, verify whether step-level RUNNER_DEBUG achieves the desired debugging behavior for this specific step.
98-109: LGTM: Good observability enhancement.The Docker log capture and upload steps are well-implemented with proper error handling and
if: always()conditions to ensure logs are available for debugging failed test runs. The container nametolgee_cli_e2ecorrectly matches the container started by thestartDocker.tsscript.src/commands/pull.ts (2)
140-172: Well-structured ETag-based conditional fetch implementation.The
fetchZipBlobfunction correctly:
- Passes
ifNoneMatchto the export API- Extracts ETag from response headers
- Handles both 412 (POST) and 304 (GET) as "not modified" per HTTP semantics
The comment at lines 167-169 explaining the 412 usage is helpful for future maintainers.
84-120: Clean branching for watch mode.The handler correctly validates
pathupfront, then branches between single pull and watch mode. ThestartWatchingintegration passes the required context and adoPullcallback.src/client/WebsocketClient.ts (2)
32-68: Robust connection state management and error handling.The implementation correctly:
- Uses a
connectingguard to prevent concurrent connection attempts- Wraps
JSON.parsein try-catch with error logging- Validates both
connectedandclientbefore subscribing- Stores subscription metadata for resubscription on reconnect
178-262: Comprehensive type definitions for WebSocket events.The type system provides strong typing for project/user channels and their corresponding payloads (
TranslationsModifiedData,BatchJobProgress). TheData<T>conditional type elegantly maps channel patterns to their event types.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/commands/login.ts (1)
9-9: Remove debugging code before merging.This debug statement logs sensitive information (even a truncated API key should never be logged) and includes a stack trace that provides no value for the login operation. This appears to be debugging code accidentally left in and is unrelated to the PR's stated objective of adding watch mode to the pull command.
Security and maintenance concerns:
- API keys are sensitive credentials that should never appear in logs, even partially masked
- The stack trace serves no purpose in the login flow
- The debug and getStackTrace imports are only used for this statement
Apply this diff to remove the debugging code:
-import { debug, exitWithError, success } from '../utils/logger.js'; +import { exitWithError, success } from '../utils/logger.js';-import { getStackTrace } from '../utils/getStackTrace.js';- debug( - `Logging in with API key ${key?.slice(0, 5)}...${key?.slice(-4)}.\n${getStackTrace()}` - ); -Also applies to: 12-12, 30-32
🧹 Nitpick comments (1)
test/e2e/error.test.ts (1)
31-32: Good addition for test isolation; consider adding error handling.Adding the logout step in
afterAllcorrectly prevents authentication state from leaking between test suites, improving test reliability.However, consider wrapping the logout in a try-catch block similar to the pattern in
afterEach(lines 14-20). If the logout operation fails or throws, it could cause the entireafterAllhook to fail, potentially masking the actual test results or making debugging harder.Apply this diff to add defensive error handling:
afterAll(async () => { await deleteProject(client); - // cleanup after logout, otherwise it pollutes to next tests - await run(['logout']); + // logout for cleanup, otherwise it pollutes to next tests + try { + await run(['logout']); + } catch (e) { + // Ignore logout errors during cleanup to avoid masking test failures + console.warn('Logout failed during cleanup:', e); + } });Note: The comment wording has been adjusted slightly for clarity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/commands/login.ts(2 hunks)test/e2e/error.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/commands/login.ts (2)
src/utils/logger.ts (1)
debug(31-35)src/utils/getStackTrace.ts (1)
getStackTrace(1-8)
test/e2e/error.test.ts (1)
test/e2e/utils/run.ts (1)
run(98-104)
⏰ 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). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
JanCizmar
force-pushed
the
jancizmar/pull-watch
branch
from
f0ed141 to
4bff501
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/e2e/pullWatch.test.ts (2)
4-4: Fix import formatting to match project style.ESLint expects multiline formatting for this import statement.
Apply this diff:
-import { createPak, createProjectWithClient, deleteProject, } from './utils/api/common.js'; +import { + createPak, + createProjectWithClient, + deleteProject, +} from './utils/api/common.js';
47-47: Add comment explaining the sleep delay.The 1-second sleep between starting the watch process and the first update iteration lacks explanation. Consider adding a comment clarifying why this delay is necessary (e.g., waiting for watch initialization, WebSocket connection, etc.).
+ // Wait for watch process to initialize and establish WebSocket connection await sleep(1000);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/e2e/pullWatch.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: ESLint
test/e2e/pullWatch.test.ts
[failure] 4-4:
Replace ·createPak,·createProjectWithClient,·deleteProject,· with ⏎··createPak,⏎··createProjectWithClient,⏎··deleteProject,⏎
⏰ 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). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
🔇 Additional comments (3)
test/e2e/pullWatch.test.ts (3)
10-25: LGTM! Test setup and teardown are properly structured.The test lifecycle correctly initializes resources in
beforeEachand cleans them up inafterEach, ensuring test isolation.
49-54: LGTM! Watch process management and cleanup are correct.The test properly terminates the watch process with SIGTERM and awaits its exit before completing, preventing race conditions with test cleanup. The 10-iteration loop provides thorough coverage of the continuous watch behavior.
56-64: LGTM! Helper function is well-structured.The
testFetchAfterUpdatehelper cleanly encapsulates a single watch-update-verify cycle with appropriate logging for debugging. The use ofconstfornewEnTextis correct.
JanCizmar
force-pushed
the
jancizmar/pull-watch
branch
from
e79ab6a to
53bd130
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/utils/pullWatch/watchHandler.ts (1)
87-100: Polling fallback may never start if WebSocket fails to connect.
startPolling()is only invoked inside the WebSocket message callback (line 133), meaning if the WebSocket connection fails initially or errors before any message arrives, polling never starts. Consider starting polling immediately as a true fallback, or invokingstartPolling()inonError/onConnectionClosehandlers.const wsClient = WebsocketClient({ serverUrl: new URL(apiUrl).origin, authentication: { apiKey: apiKey }, onConnected: () => { debug( 'WebSocket connected and subscriptions active. Performing initial pull...' ); schedulePull(); }, onError: (error) => { AuthErrorHandler(client) .handleAuthErrors(error, shutdown) .catch((err: any) => { debug('Error in handleAuthErrors: ' + err); }); // Non-fatal: just inform info('Websocket error encountered. Reconnecting...'); + startPolling(); }, onConnectionClose: () => { info('Websocket connection closed. Attempting to reconnect...'); + startPolling(); }, });
🧹 Nitpick comments (3)
src/utils/pullWatch/watchHandler.ts (3)
111-118:onErrorflow may log misleading "Reconnecting..." after auth errors call shutdown.
AuthErrorHandler.handleAuthErrorscallsshutdown()(which exits the process) for auth errors, but the code continues toinfo('Websocket error encountered. Reconnecting...'). Whileprocess.exit(0)prevents this message from appearing for auth errors, the control flow is unclear. Consider returning early afterhandleAuthErrorsif it handled the error:onError: (error) => { - AuthErrorHandler(client) - .handleAuthErrors(error, shutdown) - .catch((err: any) => { - debug('Error in handleAuthErrors: ' + err); - }); - // Non-fatal: just inform - info('Websocket error encountered. Reconnecting...'); + AuthErrorHandler(client) + .handleAuthErrors(error, shutdown) + .then(() => { + // Auth errors call shutdown(); non-auth errors reach here + info('Websocket error encountered. Reconnecting...'); + startPolling(); + }) + .catch((err: any) => { + debug('Error in handleAuthErrors: ' + err); + }); },
129-135: Consider performing initial pull and starting polling on subscribe.Currently,
startPolling()is only called when a WebSocket message is received. If no messages arrive (e.g., no changes on the server), the polling fallback never activates. You may want to start polling immediately after subscribing as a safety net:function subscribe() { unsubscribe = wsClient.subscribe(channel, () => { debug('Data change detected by websocket. Pulling now... '); schedulePull(); - startPolling(); }); + // Start polling as a fallback immediately + startPolling(); }
163-164: Consider documenting the never-resolving promise pattern.
await new Promise<void>(() => {})is a valid idiom to keep the process alive, but a brief comment would help future maintainers understand the intent:+ // Keep process alive indefinitely; shutdown() handles termination via process.exit() await new Promise<void>(() => {});
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/utils/pullWatch/watchHandler.ts(1 hunks)test/e2e/pullWatch.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/e2e/pullWatch.test.ts (6)
src/client/TolgeeClient.ts (1)
TolgeeClient(34-34)test/e2e/utils/pullWatchUtil.ts (1)
PullWatchUtil(6-54)test/e2e/utils/tmp.ts (2)
setupTemporaryFolder(9-23)TMP_FOLDER(7-7)test/e2e/utils/api/common.ts (1)
createProjectWithClient(49-92)test/e2e/utils/api/project1.ts (1)
PROJECT_1(6-15)test/e2e/utils/run.ts (2)
run(106-112)runWithKill(114-125)
src/utils/pullWatch/watchHandler.ts (4)
src/client/TolgeeClient.ts (1)
createTolgeeClient(12-20)src/utils/logger.ts (4)
info(42-44)error(69-71)debug(31-35)success(51-53)src/utils/eTagStorage.ts (1)
setETag(15-19)src/client/WebsocketClient.ts (1)
WebsocketClient(32-164)
⏰ 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). (1)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
🔇 Additional comments (6)
test/e2e/pullWatch.test.ts (4)
1-28: Test setup looks well-structured.The test properly initializes the temporary folder, project, PAK, and utility instance in
beforeEach, and cleans up viadeleteProjectinafterEach. The use ofPullWatchUtilencapsulates the filesystem and API interactions cleanly.
50-55: Process cleanup pattern is correct.The test now properly awaits both the
kill('SIGTERM')and thepromiseresolution, ensuring the watch process terminates beforeafterEachcleanup runs. This addresses the prior race condition concern.
57-65: Helper function is clear and well-scoped.
testFetchAfterUpdateencapsulates the update-and-verify cycle with appropriate logging for debugging flaky tests. Theconstdeclaration fornewEnTextis correct.
30-55: Clarify or remove the credentials-less login call.Line 33 calls
run(['login', '-l'])without any credentials, yet the subsequent pull command already provides--api-key pakexplicitly. Other login calls in the codebase pass credentials (e.g.,login pak). If this step is genuinely required for watch mode initialization, add a brief comment explaining why; otherwise, consider removing it to avoid confusion.#!/bin/bash # Check how login -l is used elsewhere and if it's necessary before watch rg -n "run\(\['login'" test/e2e/ -A 2 -B 2src/utils/pullWatch/watchHandler.ts (2)
41-68: Pending pull logic is now correctly implemented.The fix properly:
- Sets
pending = trueand capturespendingEtagwhen a pull is requested during an active pull- In
finally, after settingpulling = false, checks pending and recursively calls with the captured etagThis ensures no events are dropped while a pull is in-flight.
137-156: Shutdown cleanup is robust and correctly handles edge cases.The cleanup properly:
- Guards
unsubscribe?.()andwsClient.deactivate()with try-catch and comments- Clears both
pollingTimeranddebounceTimerwith null guards- Exits cleanly with success message
|
🎉 This PR is included in version 2.15.0-rc.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.15.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
Improvements
Chores
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.