Skip to content

chore: remove unused component registrations and slot-scope variables - #906

Merged
edwh merged 3 commits into
TheRestartProject:developfrom
marekl11:fix/unused-vue-refs
Oct 1, 2026
Merged

edwh merged 3 commits into
TheRestartProject:developfrom
marekl11:fix/unused-vue-refs

Conversation

@marekl11

@marekl11 marekl11 commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Clearing the eslint no-unused-components and no-unused-vars errors in
resources/js/components:

  • nine components imported and registered in components: but never used
    in their own template - ExternalLink (three files), FileUploader,
    EventDeviceSummary, DashboardEvent, DeviceModel, InfiniteLoading and
    Group
  • three b-modal footer slots that destructured { ok, cancel } and then
    never read ok

One of the nine is worth calling out separately: in GroupVolunteers.vue
the group mixin was imported a second time as Group and listed in
components:, so a mixin was registered as a component. The mixin is
already applied properly via mixins: [group] on the next line, so the
registration was doing nothing - but it is a mistake rather than just an
unused reference.

I checked each removed registration for both PascalCase and kebab-case use
in its own file before dropping it. No template markup changes.

Nine components were imported and registered but never used in their
templates, and three b-modal footer slots destructured scope variables
they never read; drop them all to clear the eslint
no-unused-components / no-unused-vars errors.
@marekl11

marekl11 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

The CircleCI build failed at "Run main Playwright tests" with a timeout - PHPUnit and Jest passed, and the Playwright step produced no failing test, it just went quiet until the step's no_output_timeout: 10m fired. I have rebased on develop to retrigger it.

I do not think this change can be the cause: all nine removals are local components: registrations, which only affect the template in the same file, and none of the removed tags appear there in PascalCase, kebab-case or :is= form. vue-infinite-loading / InfiniteLoading appears nowhere in resources/ at all - GroupEventScrollTable renders a b-table. The ok slot variable was unread in all three modals.

One thing you may want to look at separately: playwright.config.js sets timeout: 5 * 60 * 1000 with the comment that it "needs to be less than 10 minutes to avoid Circle CI timeout kicking in", but five test files call test.slow(), which triples that to 15 minutes - longer than the step's 10-minute silence ceiling. A single slow test can therefore take the whole step down with no output.

@marekl11

marekl11 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Small correction to my last comment: the rebase did not actually retrigger CircleCI - build 5801 is still the only one for this PR, so the red check is the original timeout rather than a fresh failure. It will need a re-run from your side whenever you get to it.

@edwh edwh left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — careful work, and easy to merge.

(AI-assisted review by Claude, run at a maintainer's request and posted with their supervision. Checks described below so they can be repeated.)

The nine removed registrations are all genuinely unused. Each checked against its own pre-removal file for PascalCase, kebab-case and dynamic <component :is> — no hits in any of the nine. That includes InfiniteLoading in GroupEventScrollTable.vue, where an accidental removal would have broken a live infinite scroll; it is import-and-register only.

The slot-scope changes are correct and complete. ok is unreferenced in all three footers, and exactly three slot-scope="{ ok, cancel }" blocks exist in resources/js. Dropping the binding entirely in EventAddVolunteerModal, rather than keeping { cancel }, is right — neither binding is used there.

One thing your description undersells. GroupVolunteers.vue had:

import Group from '../mixins/group'
components: {Group, CollapsibleSection, GroupVolunteer},

A mixin imported and registered as a component — an actual bug, not just an unused reference. The same mixin is already applied correctly via mixins: [group] two lines below. Worth a line in the description, as it is more than lint tidying.


This is good to merge as it stands. If you would like to take more on, either of these would be welcome — as a separate PR, or added here, whichever you prefer.

1. Drop the now-unused dependency. This removed the only import of vue-infinite-loading, so it survives only in package.json. A one-line removal plus a lockfile update.

2. Add a lint step for the Laravel-side JS — the more valuable one. vue/no-unused-components is already enabled through flat/vue2-essential in eslint.config.js, but nothing runs eslint in CI for resources/js, so these errors only appear locally and nothing stops them returning. .circleci/config.yml already lints the Nuxt client (cd client && npm run lint) and that step is the pattern to copy.

Fair warning on the second: a first run across resources/js will almost certainly surface more than the nine here, so it may want to be "fix the rest, then add the gate" rather than one commit. Entirely reasonable to stop at this PR and leave that to us — say the word either way.

Happy to see it go in.

@edwh edwh changed the title Remove unused component registrations and slot-scope variables chore: remove unused component registrations and slot-scope variables Sep 7, 2026
@marekl11

marekl11 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Thank you - and you are right about GroupVolunteers.vue, I had lumped it in with the other eight. The group mixin was imported a second time as Group and registered in components: while mixins: [group] was already doing the real work one line below. I have rewritten the description to call that one out on its own.

On CI: the re-run (build 5804) stopped in the same place as the first one - "Run main Playwright tests" went quiet and the step's no_output_timeout: 10m fired, with no failing test reported. Builds 5748 and 5767 on develop failed the same way, so I do not think this branch is involved.

On the follow-ups: I would be glad to do both.

  1. Dropping vue-infinite-loading needs this PR in first, otherwise develop briefly has the dependency removed while the import is still there. Happy to open it as soon as this merges.
  2. The eslint step is the one I would rather agree on before writing. Copying the cd client && npm run lint pattern is easy; the question is what to do with the first run's output. My suggestion is one PR that fixes whatever resources/js reports, then a second, tiny one that adds the CI step on top of a clean tree - that way the gate lands green and the fixes are reviewable on their own. If you would rather see the whole thing at once, or would rather keep the lint step non-blocking to start with, say so and I will follow that instead.

@edwh

edwh commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Thanks for your patience on this — the CI failure isn't yours.

Forked pull requests don't get CircleCI's project environment variables, so GOOGLE_API_CONSOLE_KEY is empty on these runs. With no key, geocoding returns nothing, group creation 422s on geocode_failed, and the create form never redirects — so createGroup times out and every test that needs a group fails. That's all 50, from the first one.

The giveaway is in the logs: a develop run masks the string "restarters" 68 times (a secret's value), while this run has no masked strings at all — nothing was injected. Blanking the key locally reproduces it exactly.

Your diff can't be involved: none of the eleven changed components is reachable from the group-create page, and all nine removed registrations are genuinely unused (I rechecked kebab-case template usage as well as PascalCase).

#908 fixes it with a stub geocoder that CI uses only when it has no key. Once that's merged, this branch will need develop merged into it — CircleCI builds the fork's head rather than a merge, so it won't pick the fix up on its own. Happy to push that if you'd rather; otherwise a merge from your side will do it.

@marekl11

Copy link
Copy Markdown
Contributor Author

That explains it — thanks for chasing it down, and the masked-strings tell is a nice catch.

I'll keep an eye on #908 and merge develop in here once it lands, so there's no need for you to push anything on my account. If you happen to be in there anyway, go ahead and I won't be surprised by the extra commit.

The two follow-ups still stand whenever you want them. I'll hold off writing the lint one until you've said which shape you'd prefer.

@ngm

ngm commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Hi @marekl11 - thanks for your contribution! Much appreciated.

FYI, I have merged in #908.

And, out of interest - if you don't mind me asking - are you a Restarters user?

marekl11 pushed a commit to marekl11/restarters.net that referenced this pull request Sep 20, 2026
CircleCI does not pass project environment variables to jobs built from
forked pull requests, so GOOGLE_API_CONSOLE_KEY is empty on those runs.
Every geocode then returns false, group creation 422s on
groups.geocode_failed, and the Playwright suite fails at its first
createGroup() - so all 50 tests fail and the job times out, looking
exactly like the contributor having broken something.

That is what is happening on TheRestartProject#906, whose diff removes eleven unused
component registrations and cannot affect group creation at all: none of
the changed components is reachable from the create page.

Adds a StubGeocoder answering from a fixed table, used only when
GEOCODER_STUB is set, which CI sets only when it has no key. It keeps
the ForceGeocodeFailure sentinel failing, and falls back to London for
an unlisted place so a new test fails on what it is testing rather than
on the stub.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@marekl11

Copy link
Copy Markdown
Contributor Author

No, I'm not. I should be straight about how this works.

I think of it as charity coding. I have a machine good enough to run a local model (Qwen3 27B), so I run overnight loops across a bunch of repos doing work I think is worth supporting. It doesn't write new features. It looks for hygiene: dead code, unused imports, comments that no longer match the code, small inconsistencies. That gives me a list, and then I spend leftover Claude tokens that would otherwise expire on checking the list, because Qwen is the weaker model and gets things wrong. What survives that becomes a PR.

So it's found by a free local model and verified with spare quota from a better one. The compute is really the thing I'm contributing, and I'd rather it went somewhere useful than nowhere. I try to keep each PR small enough to be quick to review, and if this isn't the kind of help you want here, just say and I'll stop sending them.

@ngm

ngm commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

No that's fine. I appreciate the explanation and the donated compute. It's good to know where contributions are coming from, so as some feedback I'd suggest putting that explanation in the initial PR comment in future (well, not for us now, as we now know, but if you do it on other projects). I also appreciate the intention to keep PRs small and self-contained and quick to review.

@edwh

edwh commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

@marekl11 I do something similar elsewhere. You'd be welcome to burn some tokens on http://github.com/Freegle/Iznik too :-).

@edwh

edwh commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

The remaining red check here is ours, not yours: StubGeocoderTest::testStubIsOffUnlessDeliberatelyEnabled (the only PHPUnit failure, build 5844). #908 makes CI turn on the stub geocoder when it has no key, which is every forked PR, and that test then asserted the stub was off. So it failed on exactly the runs the stub was added for.

#910 fixes the test. Once it's merged I'll merge develop into this branch myself (you said that was fine), so there's nothing for you to do.

(AI-assisted triage by Claude, posted with maintainer supervision.)

edwh added a commit to marekl11/restarters.net that referenced this pull request Sep 23, 2026
CI writes GEOCODER_STUB=true into .env when it has no geocoding key,
which is every forked PR. The test asserted the live config was false,
so it failed on exactly the runs the stub exists for (TheRestartProject#906 build 5844).

Check the config file's default with the variable cleared instead, and
add the converse so the default check can't pass vacuously.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@sonarqubecloud

Copy link
Copy Markdown

@ngm

ngm commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

@edwh Tests are passing - all good for me to merge this in to develop?

@marekl11

Copy link
Copy Markdown
Contributor Author

Thanks both, and thanks for the feedback. Good point about putting the explanation up front, I'll do that on future PRs. Thanks for the Iznik invite too, I'll take a look.

@marekl11

Copy link
Copy Markdown
Contributor Author

@edwh Thank you for the invite! I'll definitely support Iznik. It looks really cool, and it warms my heart :D

@edwh
edwh merged commit 27d1b99 into TheRestartProject:develop Oct 1, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants