Skip to content

[CDX-492] Add _dt to recommendations module#479

Open
HHHindawy wants to merge 1 commit into
masterfrom
cdx-492-js-client-add-_dt-query-param-to-recommendations-requests
Open

[CDX-492] Add _dt to recommendations module#479
HHHindawy wants to merge 1 commit into
masterfrom
cdx-492-js-client-add-_dt-query-param-to-recommendations-requests

Conversation

@HHHindawy

Copy link
Copy Markdown
Contributor

No description provided.

@HHHindawy
HHHindawy requested a review from a team July 24, 2026 15:16
@HHHindawy
HHHindawy requested a review from a team as a code owner July 24, 2026 15:16
Copilot AI review requested due to automatic review settings July 24, 2026 15:16

@constructor-claude-bedrock constructor-claude-bedrock Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This PR adds the _dt (timestamp) query parameter to the recommendations module URL builder, consistent with how it is already implemented in search, autocomplete, browse, pia, agent, quizzes, and tracker modules. The change is minimal, correct, and well-tested.

Inline comments: 2 discussions added

Overall Assessment: ⚠️ Needs Work

expect(requestedUrlParams).to.have.property('s');
expect(requestedUrlParams).to.have.property('c').to.equal(clientVersion);
expect(requestedUrlParams).to.have.property('item_id').to.equal(itemId);
expect(requestedUrlParams).to.have.property('_dt');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: The three new _dt assertions only verify that the parameter exists in the URL, but do not verify that it is a numeric timestamp (which is what Date.now() returns). Other test suites in the project also only do the existence check, so this is consistent — however, it means a regression where _dt is set to undefined or an empty string would still pass.

Consider asserting a reasonable value, e.g.:

expect(requestedUrlParams).to.have.property('_dt').to.match(/^\d+$/);

This is a low-priority suggestion; staying consistent with the existing test pattern is also acceptable.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a cache-busting timestamp query parameter (_dt) to the Recommendations module’s request URL, aligning it with other modules that already send _dt.

Changes:

  • Append _dt=Date.now() to recommendations request query params before serialization.
  • Extend recommendations module specs to assert _dt is present in outgoing requests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/modules/recommendations.js Adds _dt to the recommendations request query params.
spec/src/modules/recommendations.js Updates tests to expect _dt in recommendations request URLs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +113 to +114
// eslint-disable-next-line no-underscore-dangle
queryParams._dt = Date.now();

@Alexey-Pavlov Alexey-Pavlov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

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