Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 375047b The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Additional context: spans packages but only 80 prod lines — under the 100-line cross-layer bar Review process: Full human review — logic, architecture, edge cases. Stats
|
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }), | ||
| disabled: z.boolean().optional(), | ||
| querySettings: QuerySettingsSchema.optional(), | ||
| /** |
There was a problem hiding this comment.
🔵 minor — New zod field copies the existing MinAutoGranularitySchema line for line (breaks the REQUIRED DRY rule)
MinAutoGranularitySchema (types.ts:2840) is already z.preprocess(v => (v === '' ? undefined : v), SQLIntervalSchema.optional()). Move it above BaseSourceSchema and give it a generic name such as OptionalSQLIntervalSchema. Then use it for both minAutoGranularity and filesystemCacheReadOnlyMinRange, so the ''-to-unset rule is defined in one place.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| message: '{PATH} exceeds the limit of 10', | ||
| }, | ||
| }, | ||
| // Mirrors the '' -> undefined preprocess in BaseSourceSchema. |
There was a problem hiding this comment.
🔵 minor — Mongoose field copies the existing minAutoGranularityField
minAutoGranularityField in the same file (source.ts:285) is already { type: String, set: v => (v === '' ? undefined : v) }. Rename it to something generic, such as optionalIntervalField, move it above sourceBaseSchema, and reuse it here instead of writing the setter a second time.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| metadata, | ||
| source?.querySettings, | ||
| // The full range, not the page's time window. | ||
| getSourceQuerySettings(source, config.dateRange), |
There was a problem hiding this comment.
🔵 minor — No test checks that search pages use the full range rather than the page window
The only tests cover getSourceQuerySettings and the chart chunking path. If this call were changed to windowedConfig.dateRange, the setting would silently disappear from later search windows and every test would still pass. Add a useOffsetPaginatedQuery test with a multi-window range longer than the threshold, and assert that every rendered query includes the setting.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| { wrapper }, | ||
| ); | ||
| await waitFor(() => expect(result.current.isFetching).toBe(false)); | ||
| useSourceSpy.mockRestore(); |
There was a problem hiding this comment.
🔵 minor — useSource spy is only restored if waitFor succeeds
useSourceSpy.mockRestore() runs after await waitFor(...). If that wait times out, the mocked useSource stays in place for the rest of the suite and later tests can fail for the wrong reason. Restore the spy in a try/finally or an afterEach.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
PR Review
4 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 4 minor 4 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
E2E Test Results✅ All tests passed • 466 passed • 1 skipped • 1299s
Tests ran across 4 shards in parallel. |
Deep Review✅ No critical (P0/P1) issues found. The core 🟡 P2 -- recommended
🔵 P3 nitpicks (5)
Reviewers (7): correctness, testing, maintainability, kieran-typescript, api-contract, performance, adversarial. Prior PR comments were automated bot notices only (changeset-bot, vercel-bot) with no human feedback to verify. Testing gaps:
No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
Summary
Long-range charts and searches pull a lot of cold data through ClickHouse's filesystem cache and can evict the recent data most queries need. This adds a per-source setting, Skip filesystem cache writes. When a query's selected time range is longer than the chosen threshold, the query sets
read_from_filesystem_cache_if_exists_otherwise_bypass_cache = 1: it still reads anything already cached, but doesn't write new data into the cache.useQueriedChartConfig) and search results (useOffsetPaginatedQuery) split long ranges into smaller windows. The threshold is checked once against the range the user picked, so every window of a 3-day query gets the setting even though each window is shorter than 2 days.read_from_filesystem_cache_if_exists_otherwise_bypass_cacheunder Query Settings, that value is used.getSourceQuerySettings(common-utils), and the new fieldfilesystemCacheReadOnlyMinRangeis stored on the base source schema (zod and Mongoose).Not covered in this PR: alerts, MCP tools, the external API, sessions, service map, and the rendered-SQL / explain views still use the source's plain query settings. The external API OpenAPI docs and the MCP source schema don't list the new field yet.
Screenshots or video
I couldn't capture screenshots from my setup. The UI change is one new "Skip filesystem cache writes" select under "Query Settings" in the source form. @aaronknudtson to fill in.
How to reproduce
Starting point:
yarn dev(needs the API to save the source setting).On
main:SETTINGSclause contains only the source's Query Settings.On this branch:
read_from_filesystem_cache_if_exists_otherwise_bypass_cache = 1in itsSETTINGSclause.How to test on Vercel preview
Preview routes: /team
Steps:
References