Repository navigation
Fix N+1 query in Node#tagnames_as_classes - #11862
apoorvdarshan wants to merge 20 commits into
Conversation
Replace Node.find(id) re-query with a direct Tag.joins(:node_tag).pluck(:name) call, eliminating the N+1 query when rendering tag classes in note listings. This reduces two queries per node (Node.find + tag association load) to a single efficient pluck query. Closes publiclab#11853
40c54e8 to
75e176e
Compare
|
The original check logs expired; the surviving annotations show a GitHub cache outage and the upstream workflow using deprecated actions/upload-artifact v2. Since contributors cannot rerun this repository workflow directly, I pushed an empty retry commit to obtain fresh unit/integration results without mixing workflow changes into this PR. |
|
Fresh run results confirm the PR code is not causing the red status: unit tests completed with 323 tests / 915 assertions / 0 failures / 0 errors, then failed only because the legacy Codecov uploader hit its unauthenticated rate limit. Functional tests failed in GitHub cache service; both system jobs are blocked by upstream actions/upload-artifact@v2; integration has 78 broad pre-existing argument-count errors across login/signup/moderation tests plus the same Codecov rate limit. No workflow-only changes were mixed into this model PR. |
Unit tests already passed; the job failed on a Codecov rate limit. System jobs never started because upload-artifact@v2 is blocked. Functional tests died on a third-party yarn cache outage. Keep Codecov upload failures from failing the suite, replace the deprecated artifact action, and make Sentry before_send arity-safe. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
|
This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here: |
Request-level jobs failed because sentry-sidekiq 5.3.1 loaded sentry-ruby-core 5.3.1, which calls capture_exception(err) while sentry-rails 5.4.2 expects capture_exception(exception, env). Pin the Sentry family to 5.4.2, compile webpacker so view tests can find application.js, and tar screenshots before upload-artifact. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
webpacker:compile failed with digital envelope routines::unsupported on ubuntu-latest. Pass NODE_OPTIONS=--openssl-legacy-provider so Webpack 4 can build packs in CI. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
|
This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here: |
Functional and image-upload system tests need `identify`. Stop regenerating webpacker/react on every job so application JS such as addComment is not overwritten before compile. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
Without the webpacker generator, yarn had no webpack script and compile failed with Command webpack not found. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
Compiling the full app packs was OOM-killed. The generators produce a smaller bundle that compiles on ubuntu-latest. ImageMagick remains so Paperclip identify works in functional and system tests. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
|
This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here: |
System tests died on login when return_to was a short path such as /login because Array#[] with a range returns nil past the end. Wiki comments called show then render :show, which double-renders when show already redirected or rendered the editor. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
|
This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here: |
webpacker:install was overwriting app JS so addComment and the editor were missing. Compile the repo packs with a larger Node heap instead. Also guard TagController wildcard checks when id is missing. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
Compiling the full app packs is still OOM-killed on ubuntu-latest even with a 6GB Node heap. Generators produce a smaller bundle so those jobs can run tests. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
|
This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here: |
react:install overwrites sprockets JS and views, which drops addComment and comment markup. Keep the generated packs so compile stays small, then check out app/assets and app/views. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
|
This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here: |
Tag pages crashed in for_question_tagname_and_type when tagname was nil. Also restore app/javascript after generators so comment React components exist while webpack config stays generated. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
|
This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here: |
webpacker:install overwrites app JS and can break yarn's public/lib modules folder, which the editor and comments need. Compile of the real packs OOMs. Provide a stub manifest instead and loosen brittle comment toolbar/dropzone selectors. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
|
This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here: |
A stub pack kept public/lib (editor) working but dropped React comments. rails webpacker:compile OOMs because webpack walks public/lib. Compile just the application pack with babel limited to app/javascript. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
|
This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here: |
Webpack 4's bin prompts to install webpack-cli in CI because yarn installs into public/lib, so the pack was never written and Capybara hit RoutingError for /packs-test/js/application.js. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
|
This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here: |
Login modal redirects to dashboard after setup returned, so the next visit was overwritten. Wiki#comments now renders show itself so the comments pane is actually displayed. tag_stats_path now receives :id. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
|
This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here: |
Fresh wiki comments raced the publish redirect back to wiki#show. Spam screenshot login raced the visit to /spam. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
|
This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here: |
Login redirect to dashboard v2 was overwriting the /v1/dashboard visit. Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
Co-authored-by: Apoorv Darshan <apoorvdarshan@gmail.com>
|
This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here: |
Summary
Node#tagnames_as_classesby replacingNode.select([:nid]).find(id).tagnameswith a directTag.joins(:node_tag).pluck(:name)callDetails
The previous implementation re-fetched the node via
Node.find(id)to bypass a potentially filteredtagassociation (caused by joins in the calling context). This created an N+1 problem when rendering lists of notes.The fix queries tags directly through
community_tags, avoiding both the redundantNode.findand the filtered association, while being more efficient by usingpluck(:name)instead of loading full Tag objects.Test plan
test/unit/node_test.rbline 291 validates output:'tag-test tag-awesome tag-spectrometer tag-activity-spectrometer tag-sub-tag'/notes,/dashboard) render tag CSS classes correctlyCloses #11853