feat: add error count status-bar widget - #12
Conversation
GabrielBBaldez
left a comment
There was a problem hiding this comment.
Pulling the poll into a project service was the right instinct — one reader, two views, and the widget disposes itself properly (removeListener in dispose(), statusBar nulled). Compiles clean here and the plugin.xml registration is right: a @Service(Service.Level.PROJECT) light service needs no <projectService> entry, and you didn't add one.
Three things before this goes in, two of which come from the widget changing when the polling happens.
1. The panel's listener is never removed, and can't be
reportService.addListener(this::reportsChanged);A method reference allocates a fresh object each time, so even a later removeListener(this::reportsChanged) would hand over a different instance and match nothing. There's no dispose() on the panel either.
Tool windows get disposed and recreated — closing and reopening Stacktale is enough. Each recreation adds a listener that captures the old StacktalePanel, which captures its JBList, DefaultListModel and detail pane. The service then calls reportsChanged on every dead panel on every poll, mutating Swing models nobody can see.
The platform idiom solves this in one move — take the parent disposable at registration:
void addListener(@NotNull Listener listener, @NotNull Disposable parent) {
listeners.add(listener);
Disposer.register(parent, () -> listeners.remove(listener));
listener.reportsChanged(currentLog, currentReports);
}Then the caller can't forget, and the widget's manual removeListener can go away too.
2. FilenameIndex runs before the index exists
findLog() falls back to:
FilenameIndex.getVirtualFilesByName("errors-ai.log", GlobalSearchScope.projectScope(project))which throws IndexNotReadyException in dumb mode. On main that was unreachable in practice: polling only started once the user opened the tool window, which is essentially always after indexing. Now the status-bar widget instantiates the service at project open and the constructor fires alarm.addRequest(this::poll, 0) — straight into indexing.
It won't fire on every project, since findLog() returns early when errors-ai.log sits at the project root. It fires on exactly the multi-module layout the index lookup exists to serve.
DumbService.getInstance(project).isDumb() → skip that branch and let the next tick handle it is enough; there's no need to report anything during indexing.
3. The EDT read is now permanent
Files.readString on Alarm.ThreadToUse.SWING_THREAD predates this PR, so it isn't yours. What changes is its reach: it used to run only while the tool window was open, and now runs every 3s for the whole life of every project with the plugin installed, whether or not anyone is looking at Stacktale. On a long errors-ai.log that's a recurring EDT stall, and recent platform versions assert against slow operations there.
Your refactor is what makes this cheap to fix, which is worth saying — there's one read left instead of one per view. Alarm.ThreadToUse.POOLED_THREAD for the poll, then invokeLater for the notify, and both views get it.
Two notes outside the diff:
./gradlew test reports success because the plugin module has no tests, so that line in the description is weaker than it looks. Not something to fix here; the manual verification is what counts for now.
Heads up that @kycasdzxc is on #5, which adds settings for the log path and poll interval — the same polling code. Whichever lands second will have to rebase, and it's better if you both know now than at merge time.
|
Thanks for the detailed review. I addressed all three requested changes in
Validation completed:
No Thanks also for the heads-up about #5. I’ll rebase if it lands first. |
GabrielBBaldez
left a comment
There was a problem hiding this comment.
All three, and two of them better than what I proposed.
The dumb-mode guard sits after the project-root check rather than at the top of findLog(), so a log at the root still resolves while the index is building — I would have skipped both. And refresh() now holds its state when the log is null during indexing instead of broadcasting "nothing found", which would have blanked the widget on every project open. Neither was in the review.
Traced the disposal chain rather than assume it: content.setDisposer(panel) in the factory → panel.dispose() → the Disposer.register(parent, …) inside addListener drops it. That closes the loop the method-reference could not, and the widget's manual removeListener is correctly gone now that registration owns it.
synchronized on refresh is the right call once the alarm is POOLED_THREAD — refreshNow and the poll can genuinely overlap there, which they could not before.
CI is green on both jobs. Merging.
Re #5: no need to plan around it. Nothing has been pushed there yet, and if it lands after this it will be the one to rebase.
…#25) The workflow has never once posted a comment. Every run was green, because a 403 on the POST only raises a ::warning::, and nobody reads a warning on a green run. pull_request_target: closed is the obvious trigger and cannot work here. A run triggered by a fork's pull request gets a read-only GITHUB_TOKEN whatever the repository's Workflow permissions say — setting that to write, which was done yesterday, does not reach it. Granting it means enabling "send write tokens to workflows from fork pull requests", which hands a write token to every fork-triggered run in the repository. That is not a trade worth making for a thank-you note. A push to main is not fork-triggered, so its token honours the permissions block. The PR behind the pushed commit comes from repos/:repo/commits/:sha/pulls rather than the commit subject, because squash writes "(#12)", a merge commit writes "Merge pull request #12", and a rebase merge writes neither. Confirmed on the real failure: ashudhanda's first merged PR (#16) counted correctly as 1 and then 403'd on the comment. workflow_dispatch with a pr input is kept so the path can be exercised without waiting for someone's first contribution.
Summary
errors-ai.logreport countplugin.xmlValidation
./gradlew.bat test./gradlew.bat buildPluginerrors-ai.logchangesCloses #4