fix(intake): remove implicit 30-day lookback from span/trace lists; s…#580
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughRemoves implicit started_at defaults from intake spans/traces APIs and adds tests for explicit bounds. The frontend now seeds the 30-day started_at filter into URL state before rendering IntakeSpansTable and IntakeTracesTable. ChangesBackend: remove default lookback
Frontend: seeded default started_at filter
Sequence Diagram(s)sequenceDiagram
participant Browser
participant IntakeTable as IntakeSpansTable/IntakeTracesTable
participant SeedHook as useSeededStartedAtFilter
participant URL as URL search params
participant API as Intake API
Browser->>IntakeTable: mount
IntakeTable->>SeedHook: useSeededStartedAtFilter(defaultStartedAtFilter)
SeedHook->>URL: check filters param
alt filters missing
SeedHook->>URL: write default started_at filter (replace)
end
SeedHook-->>IntakeTable: seeded=true
IntakeTable->>API: request list with filter[started_at][$gte]
API-->>IntakeTable: results
Browser->>IntakeTable: click clear-filters
IntakeTable->>API: request list with started_at=null
API-->>IntakeTable: results
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/packages/studio/src/components/IntakeLists/IntakeSpansTable.tsx (1)
150-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate default-filter/hasActiveFilters wiring vs IntakeTracesTable.
Same
useState(makeDefaultStartedAtFilter)+hasActiveFilterspattern is repeated inIntakeTracesTable.tsx(lines 52-62). Consider extracting a small shared hook (e.g.useDefaultStartedAtFilter(fixedFilter?)) intodefaultStartedAtFilter.tsreturning{ defaultStartedAtFilter, hasActiveFilters }to avoid drift between the two call sites.♻️ Sketch of shared hook
+// in defaultStartedAtFilter.ts +export const useDefaultStartedAtFilter = ( + fixedFilter?: unknown, + debouncedColumnFilters: { id: string; value: unknown }[] = [] +) => { + const [defaultStartedAtFilter] = useState(() => + fixedFilter ? null : makeDefaultStartedAtFilter() + ); + const hasActiveFilters = debouncedColumnFilters.some( + (filter) => + defaultStartedAtFilter === null || !isDefaultStartedAtFilter(filter, defaultStartedAtFilter) + ); + return { defaultStartedAtFilter, hasActiveFilters }; +};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/components/IntakeLists/IntakeSpansTable.tsx` around lines 150 - 166, The default started_at filter and hasActiveFilters logic is duplicated between IntakeSpansTable and IntakeTracesTable, so extract the shared behavior into a small hook or helper such as useDefaultStartedAtFilter in defaultStartedAtFilter.ts. Have the shared symbol return both defaultStartedAtFilter and hasActiveFilters, and update IntakeSpansTable and IntakeTracesTable to consume it so the fixedFilter/makeDefaultStartedAtFilter/isDefaultStartedAtFilter behavior stays consistent in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/packages/studio/src/components/IntakeLists/IntakeSpansTable.tsx`:
- Around line 150-166: The default started_at filter and hasActiveFilters logic
is duplicated between IntakeSpansTable and IntakeTracesTable, so extract the
shared behavior into a small hook or helper such as useDefaultStartedAtFilter in
defaultStartedAtFilter.ts. Have the shared symbol return both
defaultStartedAtFilter and hasActiveFilters, and update IntakeSpansTable and
IntakeTracesTable to consume it so the
fixedFilter/makeDefaultStartedAtFilter/isDefaultStartedAtFilter behavior stays
consistent in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 945acadc-17d8-4bfe-9c4d-f5ca749d1c92
📒 Files selected for processing (13)
services/intake/src/nmp/intake/spans/api/spans.pyservices/intake/src/nmp/intake/spans/api/traces.pyservices/intake/tests/integration/spans/test_atif_ingest.pyservices/intake/tests/integration/spans/test_chat_completions_ingest.pyservices/intake/tests/test_spans_api.pyservices/intake/tests/test_traces_api.pyweb/packages/common/src/hooks/useStudioDataViewState/index.test.tsxweb/packages/common/src/hooks/useStudioDataViewState/index.tsweb/packages/studio/src/components/IntakeLists/IntakeSpansTable.test.tsxweb/packages/studio/src/components/IntakeLists/IntakeSpansTable.tsxweb/packages/studio/src/components/IntakeLists/IntakeTracesTable.test.tsxweb/packages/studio/src/components/IntakeLists/IntakeTracesTable.tsxweb/packages/studio/src/components/IntakeLists/defaultStartedAtFilter.ts
💤 Files with no reviewable changes (3)
- services/intake/tests/integration/spans/test_chat_completions_ingest.py
- services/intake/src/nmp/intake/spans/api/spans.py
- services/intake/src/nmp/intake/spans/api/traces.py
BrianNewsom
force-pushed
the
intake-remove-implicit-lookback/brnewsom
branch
from
July 6, 2026 21:38
cefcf21 to
d3828c4
Compare
shanaiabuggy
approved these changes
Jul 6, 2026
BrianNewsom
force-pushed
the
intake-remove-implicit-lookback/brnewsom
branch
from
July 6, 2026 22:14
d3828c4 to
df6e6cf
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/packages/studio/src/components/IntakeLists/defaultStartedAtFilter.ts (1)
19-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo direct unit tests for these pure helpers.
makeDefaultStartedAtFilterandisDefaultStartedAtFilterare pure, easily-testable logic (date math, equality contract) but tests are only mentioned for the table components downstream. Direct tests here would pin the date-window and equality behavior independent of table rendering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/components/IntakeLists/defaultStartedAtFilter.ts` around lines 19 - 32, Add direct unit tests for the pure helpers in defaultStartedAtFilter, specifically makeDefaultStartedAtFilter and isDefaultStartedAtFilter, instead of relying only on downstream table tests. Cover the date-window behavior in makeDefaultStartedAtFilter by asserting the returned started_at filter uses the expected lookback and start-of-day normalization, and cover the equality contract in isDefaultStartedAtFilter by checking it returns true only for an untouched seeded default and false for changed id or value cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/packages/studio/src/components/IntakeLists/defaultStartedAtFilter.ts`:
- Around line 19-32: Add direct unit tests for the pure helpers in
defaultStartedAtFilter, specifically makeDefaultStartedAtFilter and
isDefaultStartedAtFilter, instead of relying only on downstream table tests.
Cover the date-window behavior in makeDefaultStartedAtFilter by asserting the
returned started_at filter uses the expected lookback and start-of-day
normalization, and cover the equality contract in isDefaultStartedAtFilter by
checking it returns true only for an untouched seeded default and false for
changed id or value cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5a18c10b-5ebb-4b4c-b201-f1ac639b68cc
📒 Files selected for processing (10)
services/intake/src/nmp/intake/spans/api/spans.pyservices/intake/src/nmp/intake/spans/api/traces.pyservices/intake/tests/integration/spans/test_atif_ingest.pyservices/intake/tests/integration/spans/test_chat_completions_ingest.pyservices/intake/tests/test_traces_api.pyweb/packages/studio/src/components/IntakeLists/IntakeSpansTable.test.tsxweb/packages/studio/src/components/IntakeLists/IntakeSpansTable.tsxweb/packages/studio/src/components/IntakeLists/IntakeTracesTable.test.tsxweb/packages/studio/src/components/IntakeLists/IntakeTracesTable.tsxweb/packages/studio/src/components/IntakeLists/defaultStartedAtFilter.ts
💤 Files with no reviewable changes (3)
- services/intake/tests/integration/spans/test_chat_completions_ingest.py
- services/intake/src/nmp/intake/spans/api/traces.py
- services/intake/src/nmp/intake/spans/api/spans.py
🚧 Files skipped from review as they are similar to previous changes (6)
- services/intake/tests/test_traces_api.py
- web/packages/studio/src/components/IntakeLists/IntakeSpansTable.test.tsx
- web/packages/studio/src/components/IntakeLists/IntakeTracesTable.test.tsx
- web/packages/studio/src/components/IntakeLists/IntakeSpansTable.tsx
- web/packages/studio/src/components/IntakeLists/IntakeTracesTable.tsx
- services/intake/tests/integration/spans/test_atif_ingest.py
Contributor
|
…eed Studio browse filter The span and trace list endpoints silently added started_at >= now()-30d whenever no time filter was given. Any id-scoped query (trace detail spans, old bookmarks, SDK calls) returned silently empty results once the data aged past 30 days, with nothing in the request or response hinting a filter was applied. The API now returns exactly what was asked for. The browsing default moves to Studio, where it belongs to the UX: the intake traces/spans tables seed a visible, clearable 30-day started_at filter chip via a new defaultColumnFilters option on useStudioDataViewState (written to the URL on mount so chips, shared links, and clearing behave like user-set filters). Behavior change for API consumers: unscoped list queries now scan and return full history instead of the last 30 days. Co-Authored-By: Claude Fable 5 <[email protected]> Signed-off-by: Brian Newsom <[email protected]>
BrianNewsom
force-pushed
the
intake-remove-implicit-lookback/brnewsom
branch
from
July 6, 2026 22:38
df6e6cf to
d9b6212
Compare
arpitsardhana
pushed a commit
that referenced
this pull request
Jul 9, 2026
…eed Studio browse filter (#580) The span and trace list endpoints silently added started_at >= now()-30d whenever no time filter was given. Any id-scoped query (trace detail spans, old bookmarks, SDK calls) returned silently empty results once the data aged past 30 days, with nothing in the request or response hinting a filter was applied. The API now returns exactly what was asked for. The browsing default moves to Studio, where it belongs to the UX: the intake traces/spans tables seed a visible, clearable 30-day started_at filter chip via a new defaultColumnFilters option on useStudioDataViewState (written to the URL on mount so chips, shared links, and clearing behave like user-set filters). Behavior change for API consumers: unscoped list queries now scan and return full history instead of the last 30 days. Signed-off-by: Brian Newsom <[email protected]> Co-authored-by: Claude Fable 5 <[email protected]>
arpitsardhana
pushed a commit
that referenced
this pull request
Jul 9, 2026
…TL (#587) test_atif_ingest_accepts_example_trajectory_and_reconstructs_read_side_data seeded spans at a fixed `_BASE_TIME = 2026-01-15`. The spans/traces ClickHouse tables carry `TTL toDate(start_time) + INTERVAL 90 DAY`, so once that fixed date is >90 days in the past the rows fall out of retention and the read returns nothing (assert 0 == 7). Because TTL eviction runs on async background merges, the failure is flaky — it passes only when no merge evicts the rows before the read, which is why it intermittently blocked the merge queue. Seed relative to `now()` (~45 days back): beyond the removed 30-day list lookback that this test exercises, yet safely inside the 90-day TTL, and no longer a fixed date that rots. Use a .500000 microsecond offset so no derived timestamp lands on a whole second, which the span read-side returns without a fractional part and would fail the exact-timestamp assertions (the pre-#580 `now()-2h` seed avoided this; the fixed µs=0 base introduced in #580 did not). Full module passes repeatedly. Signed-off-by: Sandy Chapman <[email protected]>
Zenodia
pushed a commit
to Zenodia/nemo-platform
that referenced
this pull request
Jul 10, 2026
…TL (NVIDIA-NeMo#587) test_atif_ingest_accepts_example_trajectory_and_reconstructs_read_side_data seeded spans at a fixed `_BASE_TIME = 2026-01-15`. The spans/traces ClickHouse tables carry `TTL toDate(start_time) + INTERVAL 90 DAY`, so once that fixed date is >90 days in the past the rows fall out of retention and the read returns nothing (assert 0 == 7). Because TTL eviction runs on async background merges, the failure is flaky — it passes only when no merge evicts the rows before the read, which is why it intermittently blocked the merge queue. Seed relative to `now()` (~45 days back): beyond the removed 30-day list lookback that this test exercises, yet safely inside the 90-day TTL, and no longer a fixed date that rots. Use a .500000 microsecond offset so no derived timestamp lands on a whole second, which the span read-side returns without a fractional part and would fail the exact-timestamp assertions (the pre-NVIDIA-NeMo#580 `now()-2h` seed avoided this; the fixed µs=0 base introduced in NVIDIA-NeMo#580 did not). Full module passes repeatedly. Signed-off-by: Sandy Chapman <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…eed Studio browse filter
The span and trace list endpoints silently added started_at >= now()-30d whenever no time filter was given. Any id-scoped query (trace detail spans, old bookmarks, SDK calls) returned silently empty results once the data aged past 30 days, with nothing in the request or response hinting a filter was applied.
The API now returns exactly what was asked for. The browsing default moves to Studio, where it belongs to the UX: the intake traces/spans tables seed a visible, clearable 30-day started_at filter chip via a new defaultColumnFilters option on useStudioDataViewState (written to the URL on mount so chips, shared links, and clearing behave like user-set filters).
Behavior change for API consumers: unscoped list queries now scan and return full history instead of the last 30 days.
Summary by CodeRabbit