Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| this.filter_hostnames = nextHostnames; | ||
| } | ||
| } else { | ||
| const nextHostnames = this.filter_hostnames.filter(host => this.hosts.includes(host)); |
There was a problem hiding this comment.
When a selected host or client is absent from a newly queried interval, this code permanently removes it from the applied filter. For example, selecting host A, navigating to an interval containing only host B, and then returning to the original interval leaves the selection empty and the chart blank because nothing restores A. Preserve applied selections independently of the options available in the current interval. The same issue affects client pruning at line 605.
Knowledge Base Used: Timeline visualizations
| normalizeDuration(value, unit) { | ||
| if (value === '' || value === null || value === undefined) return null; | ||
| const duration = Number(value) * this.durationUnitFactor(unit); | ||
| return Number.isFinite(duration) && duration >= 0 ? duration : null; | ||
| }, |
There was a problem hiding this comment.
Negative duration values are silently converted to an empty endpoint instead of being rejected. Because Confirm is a normal button rather than a form submission, the input's min="0" constraint does not block -1; normalizeDuration returns null, range validation accepts it, and applying a negative maximum removes the upper bound and unexpectedly restores long events. Show a validation error instead of treating an invalid value as blank.
Knowledge Base Used: Timeline visualizations
There was a problem hiding this comment.
Fixed in b1e6f92. Negative duration values are now explicitly rejected on Confirm instead of being normalized to an empty bound, and tests were added for negative minimum and maximum values.
| scheduleBucketsRefresh() { | ||
| if (this.buckets_refresh_scheduled) return; | ||
|
|
||
| this.buckets_refresh_scheduled = true; | ||
| this.$nextTick(() => { | ||
| this.buckets_refresh_scheduled = false; | ||
| this.getBuckets(); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
This scheduler adds refresh batching as a performance optimization, but the repository requires actual performance measurements with profiling tools before implementing such optimizations. No profiling or benchmark evidence accompanies this change. The required measurements must be captured and used to justify or adjust the optimization before merging.
Rule Used: Before implementing performance optimizations, measure actual performance using profiling tools like pytest-profiling and --durations to identify real bottlenecks rather than assuming where performance issues exist. (source)
Learned From
gptme/gptme#707
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comments Outside DiffThese findings sit on lines the diff does not cover, so they could not be posted inline. Each one leaves this list once its file changes.
|
1 participant
Summary
This branch improves the Timeline page filters and adds i18n support for all currently supported application languages.
Changes
Filter functionality
Alloption to each checkbox group.Duration filtering
AFK and merge filters
Timeline behavior and performance
Internationalization
Validation
git diff --checkpassed.nav.aiSummarykeys in several locales.Notes and limitations