Skip to content

Remove Disqus identifier for comment migration#4478

Draft
leslieyip02 wants to merge 3 commits into
masterfrom
feat/disqus-comment-migration
Draft

Remove Disqus identifier for comment migration#4478
leslieyip02 wants to merge 3 commits into
masterfrom
feat/disqus-comment-migration

Conversation

@leslieyip02

@leslieyip02 leslieyip02 commented Jul 23, 2026

Copy link
Copy Markdown
Member

Apparently, disqus_identifier needs to be removed in order for URL mapper comment migration to work.

I have re-submitted the URL mapping:

http://nusmods.com/courses/GEH1043/reviews,http://nusmods.com/courses/GEC1011/reviews
http://nusmods.com/modules/GEH1043/reviews,http://nusmods.com/courses/GEC1011/reviews

DO NOT MERGE.

I'll be back in 24 hours.

@vercel

vercel Bot commented Jul 23, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nusmods-export Ready Ready Preview, Comment Jul 23, 2026 2:23pm
nusmods-website Ready Ready Preview, Comment Jul 23, 2026 2:23pm

Request Review

@greptile-apps

greptile-apps Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR removes the disqus_identifier field from the Disqus configuration to unblock URL-based comment migration. Without removing the identifier, Disqus's URL mapper cannot reassign comments from old module URLs to new ones.

  • Removes identifier from the DisqusConfig type and all call sites (ModulePageContent.tsx, CommentCount.tsx, DisqusComments.tsx, and the corresponding test).
  • After this change, Disqus thread identity is determined solely by page.url and data-disqus-url, which is the correct setup for URL-mapped migrations.

Confidence Score: 5/5

Safe to merge — the change is a consistent, targeted removal of a single field across all its usages, with no remaining references to identifier in the Disqus integration.

The removal is complete and consistent: the type definition, both Disqus components, the page-level config object, and the test are all updated. Disqus falls back to URL-only thread resolution when no identifier is provided, which is the intended behavior for the URL migration.

No files require special attention.

Sequence Diagram

sequenceDiagram
    participant Page as ModulePageContent
    participant DC as DisqusComments
    participant CC as CommentCount
    participant Disqus as Disqus API

    Page->>DC: "disqusConfig { url, title }"
    Page->>CC: "disqusConfig { url }"
    DC->>Disqus: "configDisqus() — page.url, page.title (no identifier)"
    CC->>Disqus: "span data-disqus-url only (no data-disqus-identifier)"
    Disqus-->>Page: "Loads thread matched by URL only"
Loading

Reviews (2): Last reviewed commit: "cleanup: remove identifier field [build]" | Re-trigger Greptile

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.92%. Comparing base (988c6fd) to head (a2e86cd).
⚠️ Report is 267 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4478      +/-   ##
==========================================
+ Coverage   54.52%   58.92%   +4.39%     
==========================================
  Files         274      319      +45     
  Lines        6076     7403    +1327     
  Branches     1455     1824     +369     
==========================================
+ Hits         3313     4362    +1049     
- Misses       2763     3041     +278     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

1 participant