Open
Conversation
Contributor
|
Size Change: +14.8 kB (+0.01%) Total Size: 106 MB
ℹ️ View Unchanged
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
117
to
118
| # TEST DELAY: Emulate slow query for testing purposes | ||
| time.sleep(5) |
Contributor
There was a problem hiding this comment.
time.sleep(5) must be removed before merge
The PR description notes this is a test-only delay, but it's still present in the code that will run in production. This time.sleep(5) blocks the Celery worker for 5 seconds on every event property value refresh. There's a second instance at line 170 for person properties. Both need to be removed.
Suggested change
| # TEST DELAY: Emulate slow query for testing purposes | |
| time.sleep(5) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/tasks/property_value_cache.py
Line: 117-118
Comment:
**`time.sleep(5)` must be removed before merge**
The PR description notes this is a test-only delay, but it's still present in the code that will run in production. This `time.sleep(5)` blocks the Celery worker for 5 seconds on every event property value refresh. There's a second instance at line 170 for person properties. Both need to be removed.
```suggestion
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
169
to
170
| # TEST DELAY: Emulate slow query for testing purposes | ||
| time.sleep(5) |
Contributor
There was a problem hiding this comment.
time.sleep(5) must be removed before merge
Second instance of the test-only delay. This blocks the Celery worker on every person property value refresh.
Suggested change
| # TEST DELAY: Emulate slow query for testing purposes | |
| time.sleep(5) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/tasks/property_value_cache.py
Line: 169-170
Comment:
**`time.sleep(5)` must be removed before merge**
Second instance of the test-only delay. This blocks the Celery worker on every person property value refresh.
```suggestion
```
How can I resolve this? If you propose a fix, please make it concise.
posthog/api/property_value_cache.py
Outdated
Comment on lines
122
to
135
|
|
||
| # Used for testing this PR only, should be removed | ||
| def clear_refresh_cooldown( | ||
| team_id: int, | ||
| property_type: str, | ||
| property_key: str, | ||
| search_value: Optional[str] = None, | ||
| event_names: Optional[list[str]] = None, | ||
| ) -> bool: | ||
| """Delete the refresh cooldown key. Returns True if the key existed.""" | ||
| redis_client = get_client() | ||
| cooldown_key = _make_cache_key(team_id, property_type, property_key, search_value, event_names) + ":refreshing" | ||
| return redis_client.delete(cooldown_key) > 0 | ||
|
|
Contributor
There was a problem hiding this comment.
Test-only function should be removed
The comment on line 123 says this is "for testing this PR only, should be removed." clear_refresh_cooldown is only used in the seed_property_value_cache management command (also a test-only artifact). If the management commands are meant to be removed before merge (as stated in the PR description), this function should be removed too.
Suggested change
| # Used for testing this PR only, should be removed | |
| def clear_refresh_cooldown( | |
| team_id: int, | |
| property_type: str, | |
| property_key: str, | |
| search_value: Optional[str] = None, | |
| event_names: Optional[list[str]] = None, | |
| ) -> bool: | |
| """Delete the refresh cooldown key. Returns True if the key existed.""" | |
| redis_client = get_client() | |
| cooldown_key = _make_cache_key(team_id, property_type, property_key, search_value, event_names) + ":refreshing" | |
| return redis_client.delete(cooldown_key) > 0 |
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/api/property_value_cache.py
Line: 122-135
Comment:
**Test-only function should be removed**
The comment on line 123 says this is "for testing this PR only, should be removed." `clear_refresh_cooldown` is only used in the `seed_property_value_cache` management command (also a test-only artifact). If the management commands are meant to be removed before merge (as stated in the PR description), this function should be removed too.
```suggestion
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
1
to
2
| import json | ||
| import time |
Contributor
There was a problem hiding this comment.
Unused imports should be cleaned up after removing time.sleep
After removing the time.sleep(5) calls, the import time statement will be unused and should be removed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/tasks/property_value_cache.py
Line: 1-2
Comment:
**Unused imports should be cleaned up after removing `time.sleep`**
After removing the `time.sleep(5)` calls, the `import time` statement will be unused and should be removed.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Query snapshots: Backend query snapshots updatedChanges: 1 snapshots (1 modified, 0 added, 0 deleted) What this means:
Next steps:
|
andyzzhao
reviewed
Feb 20, 2026
aspicer
approved these changes
Feb 21, 2026
Contributor
aspicer
left a comment
aspicer
left a comment
There was a problem hiding this comment.
LGTM, a couple minor fixes. Let's see if this makes it feel better!
| breakpoint() | ||
| actions.setOptions(propertyKey, propValues, type !== PropertyDefinitionType.FlagValue) | ||
|
|
||
| const propValues = Array.isArray(responseData) ? responseData : responseData.results |
Contributor
There was a problem hiding this comment.
Doing this Array.isArray check has the potentially to quickly become difficult to maintain if we implement more interface differenes between these diffrent values endpoints.
It would be better to just update the endpoints to all return an object (with refreshing: false for the ones that don't support it)
See here:
https://github.com/PostHog/posthog/pull/48656/changes
| event_names=event_names, | ||
| ) | ||
|
|
||
| clear_task_running( |
Contributor
There was a problem hiding this comment.
Do we want to catch errors and make sure that we call this there is a query execution error? Otherwise I think the spinner will keep running?
Contributor
|
I realized we already have a mechanism for running an async query upon a stale cache result. So an alternative approach to get the same behavior as this PR would be implementing a |
Member
Author
|
Oh shoot I was just about to get this merged in. @andyzzhao do you think the query runner is the better solution and I should do that instead? |
Contributor
|
@raquelmsmith creating a query runner should be a smaller change and easier to maintain, but up to you! |
Member
Author
|
okay @andyzzhao I did that here: #49001 I'm not sure if any parts of this PR are still needed after that one but I imagine so - the re-polling from the frontend for the fresh values at least. |
- Remove property_value_cache.py, tasks/property_value_cache.py and management commands
- Use PropertyValuesQueryRunner with RECENT_CACHE_CALCULATE_ASYNC_IF_STALE_AND_BLOCKING_ON_MISS
- Return {results, refreshing} from both event and person property values endpoints
- Add 5-sec test delays to _calculate_event and _calculate_person for frontend polling testing
- Update task registration and snapshot to reflect removed tasks
- Remove test_property_value_cache.py (tests now obsolete)
- propertyValueLogic.ts and frontend polling already in place from earlier commits
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Contributor
Visual regression: Storybook UI snapshots updatedChanges: 3 snapshots (3 modified, 0 added, 0 deleted) What this means:
Next steps:
|
raquelmsmith
commented
Feb 26, 2026
Comment on lines
+299
to
+312
| if not query_status.complete: | ||
| # Only deduplicate against a query that is still in progress | ||
| posthoganalytics.capture( | ||
| "query duplicate found", | ||
| distinct_id=user_id, | ||
| properties={ | ||
| "cache_key": cache_key, | ||
| "query_id": existing_query_id, | ||
| "query_json": query_json, | ||
| }, | ||
| ) | ||
| return query_status | ||
| # The previous task finished (or failed) — clean up the stale mapping and enqueue a new one | ||
| manager.unregister_cache_key_mapping(cache_key) |
Member
Author
There was a problem hiding this comment.
The dedup check here was changed to only short-circuit when the found query is still in-progress (not query_status.complete). Previously it returned any existing mapping unconditionally — including stale complete=True ones left over when a task failed before calling unregister_cache_key_mapping (which only runs when query_status.results is set). That meant one failed task would permanently block re-enqueueing for that cache key until the 20-min TTL expired. Now a completed mapping is cleaned up and a fresh task is enqueued instead.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Contributor
Visual regression: Storybook UI snapshots updatedChanges: 4 snapshots (4 modified, 0 added, 0 deleted) What this means:
Next steps:
|
Member
Author
Contributor
Additional Comments (3)
|
Contributor
Visual regression: Storybook UI snapshots updatedChanges: 1 snapshots (1 modified, 0 added, 0 deleted) What this means:
Next steps:
|
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
3 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.
Problem
Previously, property value suggestions (the autocomplete dropdowns in filters) were fetched fresh from ClickHouse on every request. #49001 added PropertyValuesQueryRunner, which caches results in Redis and returns stale data immediately while enqueuing a background ClickHouse refresh. This PR wires up the frontend to handle that "refreshing" signal and re-poll until fresh values arrive.
Changes
This PR does three things:
emailprop for search valueaand that gets cached, the cache size should still stay relatively small. Debounce is 300ms so we hopefully aren't caching on every keystroke.ain the search, a queryis fired off, and then I type another key so the query isabbefore theaquery completes, is theaquery cancelled?**Claude change description, which I actually thought was mildly helpful**
API response format (
event.py,person.py)Both
/api/projects/:id/events/values/and/api/person/values/previously returned a bare JSON array. They now return{ "results": [...], "refreshing": true }.refreshing: truemeans the backend served cached data and has kicked off a background refresh. All existing tests updated for the new shape.Query runner registration (
query_runner.py)PropertyValuesQueryis registered inget_query_runner()so it participates in the standard async query infrastructure (cache, execution modes, etc.).Schema (
schema-general.ts,schema.py)PropertyValuesQuery,PropertyValueItem,PropertyValuesQueryResponse, andCachedPropertyValuesQueryResponseare now defined in the TypeScript schema and generated intoschema.py, replacing hand-rolled Python models.PropertyValueItem.nameis typed asstring | number | boolean | nullrather thanany.Async dedup fix (
execute_async.py)Fixed a bug where a completed task left a stale mapping in Redis, permanently blocking re-enqueuing for the same cache key. The fix checks
query_status.completebefore deduplicating and cleans up stale mappings. Added explicitQueryNotFoundErrorhandling for the race where the mapping exists but the status TTL has expired.Frontend: polling on
refreshing(propertyDefinitionsModel.ts)The model now:
{ results, refreshing }response shaperefreshing,preRefreshValueNames, andsearchInputper property key in theOptiontyperefreshing: true, repeating until the backend returnsrefreshing: falseFrontend:
propertyValueLogic.ts(new file)A scoped kea logic keyed on
type/propertyKeythat exposes:isRefreshing— whether a background refresh is in progressnewValueNames— names of values that appeared after the latest refresh (for future UI callouts)Frontend:
PropertyValue.tsxisRefreshingfrompropertyValueLogicto show the loading spinner while a background refresh is running, so users see the cached values immediately but know fresh data is on the wayHow did you test this code?
I did a lot of manual testing to make sure this actually works. Claude wrote some backend tests as well.
To test (I removed the testing accommodations but leaving this here in case someone wants to manually locally test again - I can push them back up):
python manage.py seed_property_values_cache --team-id 1 --key '$browser_language' --type person --event-names '$pageview' --values TEST_1 TEST_2python manage.py seed_property_values_cache --team-id 1 --key '$browser' --type event --event-names '$pageview' --values TEST_1 TEST_2Loom:
https://www.loom.com/share/2b64435ba691495480f689ad4bbfe354
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Publish to changelog?
no