Refactor dfns #464
Refactor dfns #464
Conversation
WalkthroughThe changes update the package version in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant DateFnsLocale
participant ReduxStore
Client->>Server: Request isomorphic route
Server->>Server: Parse config for time translation
alt Time translation enabled
Server->>DateFnsLocale: Dynamically import locale module (hi, ta, de, or en-US)
DateFnsLocale-->>Server: Return locale module
else Time translation disabled
Server->>Server: Use default locale (en-US)
end
Server->>ReduxStore: createStoreFromResult(url, result, { localeModule })
ReduxStore-->>Server: Store instance
Server-->>Client: Send rendered response
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error code ERESOLVE ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
server/handlers/isomorphic-handler.js (1)
475-497:⚠️ Potential issue
matchis still undefined – SEO tag generation will throw
writeResponsereferencesmatch.pageType, butmatchis not in scope anywhere insidehandleIsomorphicRoute.
This will raise aReferenceErrorthe very first time a request hits this code path.- const seoTags = seoInstance && seoInstance.getMetaTags(config, result.pageType || match.pageType, result, { url }); + const pageTypeForSeo = result.pageType /* || fallbackPageTypeIfAny */; + const seoTags = + seoInstance && seoInstance.getMetaTags(config, pageTypeForSeo, result, { url });Consider passing the
matchobject intowriteResponse, or just rely onresult.pageType.🧰 Tools
🪛 Biome (1.9.4)
[error] 490-490: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🧹 Nitpick comments (1)
server/handlers/isomorphic-handler.js (1)
574-605:getStoreOptscan be simplified & made more robust
- Being declared
async, you can justreturn opts;instead ofPromise.resolve(opts).- Only exact language codes (
hi,ta,de) are handled. Codes likehi-IN,de-DEor lower-cased values will silently fall back toen-US.console.warnbypasses the centrallogError/logger, fragmenting logs.-async function getStoreOpts(config) { - const opts = {}; - const enableTimeTranslation = _.get(config, ['pbConfig', 'general', 'enableTimeTranslation'], false); - if (!enableTimeTranslation) { - return Promise.resolve(opts); - } - const languageCode = _.get(config, ['language', 'ietf-code']); +function getStoreOpts(config) { + const opts = {}; + if (!_.get(config, ['pbConfig', 'general', 'enableTimeTranslation'])) { + return opts; + } + const rawCode = _.get(config, ['language', 'ietf-code'], '').toLowerCase(); + const languageCode = rawCode.split('-')[0]; // strip region, eg hi-IN -> hi try { let localeModule; switch (languageCode) { @@ opts.localeModule = localeModule.default || localeModule; - return Promise.resolve(opts); + return opts; } catch (err) { - console.warn(`Falling back to en-US due to error loading locale: ${languageCode}`, err); + logError?.(err); // reuse existing logger if available const fallbackLocale = require('date-fns/locale/en-US'); opts.localeModule = fallbackLocale.default || fallbackLocale; - return Promise.resolve(opts); + return opts; } }This removes needless promises, supports region codes, and centralises logging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(1 hunks)server/handlers/isomorphic-handler.js(4 hunks)
🔇 Additional comments (2)
package.json (1)
3-3: Version suffix looks non-semver-compliantThe string
7.34.2-refactor-dfns.0is a valid npm pre-release tag, yet the additional dot after the dash (-dfns.0) is unusual in this repo and may break tooling that assumesx.y.z-rc.Nor similar.
Please double-check that your internal publishing pipeline (standard-version, changelog, docker tags, etc.) recognises this pattern before merging.server/handlers/isomorphic-handler.js (1)
196-210: Good: store now receiveslocaleModule
The prop is conditionally injected without breaking existing callers – neat!
| .then(async (result) => { | ||
| if (!result) { | ||
| return next(); | ||
| } | ||
| return new Promise((resolve) => resolve(writeResponse(result))) | ||
| .catch((e) => { | ||
| logError(e); | ||
| res.status(500); | ||
| res.send(e.message); | ||
| }) | ||
| .finally(() => res.end()); | ||
| }); | ||
| return getStoreOpts(config).then((opts) => { | ||
| return new Promise((resolve) => resolve(writeResponse(result, opts))) | ||
| .catch((e) => { | ||
| logError(e); | ||
| res.status(500); | ||
| res.send(e.message); | ||
| }) | ||
| .finally(() => res.end()); | ||
| }); | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unnecessary promise-wrapping & potential double res.end()
- The
asyncarrow already returns a promise. WrappingwriteResponseinnew Promise(resolve => resolve(...))is redundant noise. writeResponseultimately writes tores; most implementations ofrenderLayoutcallres.end()/res.send().
The outer.finally(() => res.end())therefore risks callingres.end()twice.
- .then(async (result) => {
+ .then((result) => {
if (!result) {
return next();
}
- return getStoreOpts(config).then((opts) => {
- return new Promise((resolve) => resolve(writeResponse(result, opts)))
- .catch((e) => {
- logError(e);
- res.status(500);
- res.send(e.message);
- })
- .finally(() => res.end());
- });
+ return getStoreOpts(config)
+ .then((opts) => writeResponse(result, opts))
+ .catch((e) => {
+ logError(e);
+ res.status(500).send(e.message);
+ });
})If renderLayout already closes the stream, drop the outer finally.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .then(async (result) => { | |
| if (!result) { | |
| return next(); | |
| } | |
| return new Promise((resolve) => resolve(writeResponse(result))) | |
| .catch((e) => { | |
| logError(e); | |
| res.status(500); | |
| res.send(e.message); | |
| }) | |
| .finally(() => res.end()); | |
| }); | |
| return getStoreOpts(config).then((opts) => { | |
| return new Promise((resolve) => resolve(writeResponse(result, opts))) | |
| .catch((e) => { | |
| logError(e); | |
| res.status(500); | |
| res.send(e.message); | |
| }) | |
| .finally(() => res.end()); | |
| }); | |
| }) | |
| .then((result) => { | |
| if (!result) { | |
| return next(); | |
| } | |
| return getStoreOpts(config) | |
| .then((opts) => writeResponse(result, opts)) | |
| .catch((e) => { | |
| logError(e); | |
| res.status(500).send(e.message); | |
| }); | |
| }) |
🤖 Prompt for AI Agents
In server/handlers/isomorphic-handler.js around lines 558 to 571, remove the
unnecessary wrapping of writeResponse in a new Promise since the async function
already returns a promise. Also, if writeResponse or its underlying renderLayout
implementation already calls res.end(), eliminate the outer finally block that
calls res.end() to avoid ending the response twice. Adjust the code to directly
return writeResponse with the appropriate options and handle errors without
redundant promise wrapping or double res.end() calls.
Summary by CodeRabbit
New Features
Chores