fix(git-sync): address review — configurable poll, always-on loop-guard, cleanup
Comprehensive-review follow-ups (APPROVE WITH SUGGESTIONS; no critical issues):
- poll interval is now actually configurable: replaced the hardcoded
@Interval('git-sync-poll', 15000) with a dynamic SchedulerRegistry interval
registered in onModuleInit from getGitSyncPollIntervalMs() (cleared in
onModuleDestroy); /status and the real cadence now share one config source.
Boots logging 'poll interval registered (Nms)'.
- loop-guard now ALWAYS applies: the lastUpdatedSource==='git-sync' skip was
nested inside the !spaceId/!workspaceId branch, so structural self-writes
(CREATE/MOVE/RESTORE/SOFT_DELETE, which carry spaceId+workspaceId) bypassed it
and re-triggered cycles. Fetch the page row once, guard unconditionally, then
resolve space/workspace.
- remove the dead PAGE_CONTENT_UPDATED subscription (it's a BullMQ job, never an
EventEmitter event; body edits arrive via PAGE_UPDATED).
- fix the stale datasource comment (PageService DOES stamp 'git-sync' now).
- env getters: parseInt radix 10 + NaN/<=0 fallback for poll/debounce (+ max
deletes), with 6 new environment.service.spec tests.
tsc clean; jest 723 pass; live cycle re-verified post-refactor (ran, push
applied, unflagged 92-page space untouched).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -35,4 +35,46 @@ describe('EnvironmentService', () => {
|
||||
expect(withEnv('not-a-number').getGitSyncMaxDeletesPerCycle()).toBe(5);
|
||||
});
|
||||
});
|
||||
|
||||
describe('getGitSyncPollIntervalMs', () => {
|
||||
const withEnv = (value?: string) =>
|
||||
new EnvironmentService({
|
||||
get: (_key: string, fallback?: string) => value ?? fallback,
|
||||
} as any);
|
||||
|
||||
it('defaults to 15000 when unset', () => {
|
||||
expect(withEnv().getGitSyncPollIntervalMs()).toBe(15000);
|
||||
});
|
||||
|
||||
it('parses a valid positive int', () => {
|
||||
expect(withEnv('30000').getGitSyncPollIntervalMs()).toBe(30000);
|
||||
});
|
||||
|
||||
it('falls back to 15000 for non-positive or unparseable values', () => {
|
||||
expect(withEnv('0').getGitSyncPollIntervalMs()).toBe(15000);
|
||||
expect(withEnv('-100').getGitSyncPollIntervalMs()).toBe(15000);
|
||||
expect(withEnv('not-a-number').getGitSyncPollIntervalMs()).toBe(15000);
|
||||
});
|
||||
});
|
||||
|
||||
describe('getGitSyncDebounceMs', () => {
|
||||
const withEnv = (value?: string) =>
|
||||
new EnvironmentService({
|
||||
get: (_key: string, fallback?: string) => value ?? fallback,
|
||||
} as any);
|
||||
|
||||
it('defaults to 2000 when unset', () => {
|
||||
expect(withEnv().getGitSyncDebounceMs()).toBe(2000);
|
||||
});
|
||||
|
||||
it('parses a valid positive int', () => {
|
||||
expect(withEnv('500').getGitSyncDebounceMs()).toBe(500);
|
||||
});
|
||||
|
||||
it('falls back to 2000 for non-positive or unparseable values', () => {
|
||||
expect(withEnv('0').getGitSyncDebounceMs()).toBe(2000);
|
||||
expect(withEnv('-5').getGitSyncDebounceMs()).toBe(2000);
|
||||
expect(withEnv('not-a-number').getGitSyncDebounceMs()).toBe(2000);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -348,18 +348,28 @@ export class EnvironmentService {
|
||||
return this.configService.get<string>('GIT_SYNC_REMOTE_TEMPLATE');
|
||||
}
|
||||
|
||||
/** Poll-safety interval in ms (default 15000). */
|
||||
/**
|
||||
* Poll-safety interval in ms (default 15000). A NaN / non-positive value falls
|
||||
* back to the default so a bad override can never disable or zero the poll loop.
|
||||
*/
|
||||
getGitSyncPollIntervalMs(): number {
|
||||
return parseInt(
|
||||
const parsed = parseInt(
|
||||
this.configService.get<string>('GIT_SYNC_POLL_INTERVAL_MS', '15000'),
|
||||
10,
|
||||
);
|
||||
return Number.isFinite(parsed) && parsed > 0 ? parsed : 15000;
|
||||
}
|
||||
|
||||
/** Event debounce window in ms (default 2000). */
|
||||
/**
|
||||
* Event debounce window in ms (default 2000). A NaN / non-positive value falls
|
||||
* back to the default so a bad override can never disable the debounce.
|
||||
*/
|
||||
getGitSyncDebounceMs(): number {
|
||||
return parseInt(
|
||||
const parsed = parseInt(
|
||||
this.configService.get<string>('GIT_SYNC_DEBOUNCE_MS', '2000'),
|
||||
10,
|
||||
);
|
||||
return Number.isFinite(parsed) && parsed > 0 ? parsed : 2000;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -372,6 +382,7 @@ export class EnvironmentService {
|
||||
getGitSyncMaxDeletesPerCycle(): number {
|
||||
const parsed = parseInt(
|
||||
this.configService.get<string>('GIT_SYNC_MAX_DELETES_PER_CYCLE', '5'),
|
||||
10,
|
||||
);
|
||||
return Number.isFinite(parsed) && parsed > 0 ? parsed : 5;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user