-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
feat(core): workspace indexing settings #11802
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## canary #11802 +/- ##
==========================================
- Coverage 55.15% 55.15% -0.01%
==========================================
Files 2614 2614
Lines 121648 121648
Branches 19380 19380
==========================================
- Hits 67094 67092 -2
+ Misses 53027 52498 -529
- Partials 1527 2058 +531
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
||
this.workspaceServerService.server.gql({}); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file is missing a trailing newline at the end. This violates standard code formatting practices and may cause issues with some tools. Please add a newline character at the end of the file to ensure consistency with the codebase's style guidelines.
Spotted by Diamond (based on custom rules)
Is this helpful? React 👍 or 👎 to let us know.
const handleReindexClick = React.useCallback(() => { | ||
setIsIndexing(true); | ||
setIndexingProgress(0); | ||
|
||
// Simulated progress for demo | ||
const interval = setInterval(() => { | ||
setIndexingProgress(prev => { | ||
if (prev >= 100) { | ||
clearInterval(interval); | ||
setIsIndexing(false); | ||
return 100; | ||
} | ||
return prev + 10; | ||
}); | ||
}, 1000); | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interval created in handleReindexClick
isn't being cleaned up if the component unmounts during the indexing process. This could lead to memory leaks and continued state updates on an unmounted component. Consider adding a cleanup mechanism using either:
useEffect(() => {
return () => {
if (interval) {
clearInterval(interval);
}
};
}, []);
Or store the interval ID in a ref and clear it in a cleanup function:
const intervalRef = useRef(null);
const handleReindexClick = React.useCallback(() => {
// ...
intervalRef.current = setInterval(() => {
// ...
}, 1000);
}, []);
useEffect(() => {
return () => {
if (intervalRef.current) {
clearInterval(intervalRef.current);
}
};
}, []);
const handleReindexClick = React.useCallback(() => { | |
setIsIndexing(true); | |
setIndexingProgress(0); | |
// Simulated progress for demo | |
const interval = setInterval(() => { | |
setIndexingProgress(prev => { | |
if (prev >= 100) { | |
clearInterval(interval); | |
setIsIndexing(false); | |
return 100; | |
} | |
return prev + 10; | |
}); | |
}, 1000); | |
}, []); | |
const intervalRef = React.useRef(null); | |
const handleReindexClick = React.useCallback(() => { | |
setIsIndexing(true); | |
setIndexingProgress(0); | |
// Clear any existing interval | |
if (intervalRef.current) { | |
clearInterval(intervalRef.current); | |
} | |
// Simulated progress for demo | |
intervalRef.current = setInterval(() => { | |
setIndexingProgress(prev => { | |
if (prev >= 100) { | |
clearInterval(intervalRef.current); | |
intervalRef.current = null; | |
setIsIndexing(false); | |
return 100; | |
} | |
return prev + 10; | |
}); | |
}, 1000); | |
}, []); | |
React.useEffect(() => { | |
return () => { | |
if (intervalRef.current) { | |
clearInterval(intervalRef.current); | |
} | |
}; | |
}, []); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
} | ||
|
||
export class Indexer extends Entity { | ||
status$ = new LiveData<number>(this.store.status); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The status$
property is initialized with this.store.status
at the class field level, which executes before the constructor runs. At that point, this.store
is undefined, leading to a runtime error.
Consider either:
- Moving this initialization to the constructor:
constructor(
private readonly workspaceService: WorkspaceService,
private readonly store: IndexerStore,
) {
super();
this.status$ = new LiveData<number>(this.store.status);
}
- Using a getter pattern to ensure the value is accessed only when needed:
get status$() {
return new LiveData<number>(this.store.status);
}
This will ensure the property is properly initialized after dependencies are available.
status$ = new LiveData<number>(this.store.status); | |
status$: LiveData<number>; | |
constructor( | |
private readonly workspaceService: WorkspaceService, | |
private readonly store: IndexerStore, | |
) { | |
super(); | |
this.status$ = new LiveData<number>(this.store.status); | |
} | |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
20c3068
to
f4c7749
Compare
1912ef3
to
44b54e6
Compare
f4c7749
to
7ca422a
Compare
44b54e6
to
cde22a1
Compare
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
cde22a1
to
768b084
Compare
4f5659b
to
b64a4df
Compare
4cf8e54
to
d487be5
Compare
b64a4df
to
83865db
Compare
d487be5
to
2e559b8
Compare
83865db
to
95480b1
Compare
2e559b8
to
ad6527f
Compare
95480b1
to
41c158f
Compare
ad6527f
to
777bdf8
Compare
41c158f
to
f865cf2
Compare
777bdf8
to
69135f7
Compare
4795161
to
ced25fb
Compare
4a16e32
to
f4534d3
Compare
ced25fb
to
80ad126
Compare
f4534d3
to
7c97c84
Compare
80ad126
to
df95d7e
Compare
7c97c84
to
d666a0e
Compare
657ebfe
to
5f11632
Compare
d666a0e
to
b376f0b
Compare
5f11632
to
bbf634f
Compare
9c9337f
to
825c630
Compare
bbf634f
to
9086e35
Compare
825c630
to
5dfa32d
Compare
9086e35
to
ed5627f
Compare
5dfa32d
to
6c9f28e
Compare
ed5627f
to
5edc615
Compare
5edc615
to
fbeca51
Compare
1c0c9fc
to
8d77c48
Compare
No description provided.