fix(centralServer): RN-1681: omit Link header unless paginating
#6260
Conversation
jaskfla
changed the title
jaskfla
force-pushed
the
dashboard-rel
branch
2 times, most recently
from
2d29a7e to
4ae2233
Compare
WalkthroughThis update introduces several logging and configuration improvements across multiple server packages. Most server entry points now include a log statement that reports the current Winston logging level after startup. The Winston logger configuration was adjusted to make the log level dynamically configurable via the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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: 0
♻️ Duplicate comments (2)
packages/psss-server/src/index.ts (1)
28-28: Same logging improvement as other servers.This follows the same helpful pattern of logging the Winston level after startup that was added to other server packages.
packages/tupaia-web-server/src/index.ts (1)
29-29: Consistent logging improvement across servers.This logging addition matches the pattern applied to other server packages for better runtime observability.
🧹 Nitpick comments (8)
packages/datatrak-web-server/package.json (1)
23-25: Maybe addcross-envso theLOG_LEVELflag works on Windows too
start-verbosestill prefixes the command with a Unix-style env var (LOG_LEVEL=debug …). On Windows shells this pattern is ignored, so colleagues on that platform won’t get debug logs. You might swap incross-env(already used elsewhere in the repo) to keep parity:- "start-verbose": "LOG_LEVEL=debug yarn start-dev", + "start-verbose": "cross-env LOG_LEVEL=debug yarn start-dev",…and add
cross-envtodevDependenciesif it’s not already pulled in transitively.tupaia-packages.code-workspace (1)
226-229: Maybe keep entries alphabetically sorted for quicker scanningAdding “parsable” looks fine—perhaps just drop it into the right alphabetical spot next time so the list stays easy to navigate.
packages/meditrak-app-server/src/index.ts (1)
38-38: Logging level visibility is helpful, though pattern might benefit from consolidation.Adding the log level output after startup provides good runtime visibility into logging configuration. Since this appears to be repeated across multiple server files, perhaps there might be an opportunity to centralize this in a shared server utility, though the current approach is perfectly functional.
packages/entity-server/src/index.ts (1)
22-23: Maybe consolidate the two info logs into oneSince both lines fire at startup, perhaps merging them could keep the console a little tidier:
-winston.info(`Running on port ${port}`); -winston.info(`Logging at ${winston.level} level`); +winston.info(`Running on port ${port} (log level: ${winston.level})`);A single line still surfaces both details without adding an extra log entry.
packages/lesmis-server/src/index.ts (1)
28-30: Maybe collapse the duplicate startup infoSame thought as for the other servers—combining the port and log-level messages might reduce clutter while retaining the signal.
packages/report-server/src/index.ts (1)
33-35: Consider merging startup logsYou might combine the port and log-level details into one line for brevity, mirroring the suggestion in sibling services.
packages/data-table-server/src/index.ts (1)
25-27: Optional: compress the two startup logsIf you’d like a leaner startup output, maybe merge the messages as done elsewhere:
-winston.info(`Running on port ${port}`); -winston.info(`Logging at ${winston.level} level`); +winston.info(`Running on port ${port} (log level: ${winston.level})`);packages/central-server/src/apiV2/GETHandler/GETHandler.js (1)
175-190: Consider potential division by zero scenario.The
lastPagecalculation usestotalRecordCount / limit- while this should be safe given the earlier nullish checks, perhaps it might be worth considering if there are edge cases wherelimitcould be zero.The conditional Link header addition based on
isNotNullish(limit)nicely addresses the pagination concerns from past comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
packages/admin-panel-server/src/index.ts(1 hunks)packages/admin-panel/src/layout/navigation/SecondaryNavbar.jsx(5 hunks)packages/admin-panel/src/table/actions.js(2 hunks)packages/api-client/src/connections/ApiConnection.ts(3 hunks)packages/central-server/src/apiV2/GETHandler/GETHandler.js(5 hunks)packages/central-server/src/apiV2/GETHandler/helpers.js(2 hunks)packages/central-server/src/apiV2/dashboardItems/GETDashboardItems.js(2 hunks)packages/central-server/src/index.js(2 hunks)packages/data-table-server/package.json(1 hunks)packages/data-table-server/src/index.ts(1 hunks)packages/database/src/TupaiaDatabase.js(1 hunks)packages/datatrak-web-server/package.json(1 hunks)packages/entity-server/src/index.ts(1 hunks)packages/lesmis-server/package.json(1 hunks)packages/lesmis-server/src/index.ts(1 hunks)packages/meditrak-app-server/package.json(1 hunks)packages/meditrak-app-server/src/index.ts(1 hunks)packages/psss-server/package.json(1 hunks)packages/psss-server/src/index.ts(1 hunks)packages/report-server/src/index.ts(1 hunks)packages/server-boilerplate/src/microService/auth/middleware.ts(2 hunks)packages/server-boilerplate/src/utils/configureWinston.ts(2 hunks)packages/tupaia-web-server/package.json(1 hunks)packages/tupaia-web-server/src/index.ts(1 hunks)packages/tupaia-web-server/src/routes/DashboardsRoute.ts(4 hunks)packages/utils/src/errors.js(11 hunks)packages/web-config-server/src/index.js(2 hunks)tupaia-packages.code-workspace(1 hunks)
🔇 Additional comments (42)
packages/data-table-server/package.json (1)
23-25: Double-check thatstart-verbosestill does what you expectNow that
start-devno longer hard-codesLOG_LEVEL=debug,start-verboserelies on its own prefix to flip the log level. That should be fine, but you might runyarn start-verboseonce to confirm the logger actually picks updebug—just in case any future change in the global script overrides it.packages/lesmis-server/package.json (1)
23-25: Maybe sanity-check the runtime log levelWith the
LOG_LEVELremoval,start-verboseis now the only dev path that forcesdebug. Perhaps give it a quick run to be sure the intended verbosity still flows through Winston.packages/tupaia-web-server/package.json (1)
23-25: Confirmstart-verbosebehaviour after env var removalSince
start-devno longer injectsLOG_LEVEL=debug, a quick manual run ofstart-verbosemight help ensure the explicit env override is still respected.packages/psss-server/package.json (1)
23-25: Small follow-up: verify debug logs still appear viastart-verboseThe change is likely harmless, but maybe run
start-verboseonce to confirm Winston reportsdebuglevel as expected.packages/admin-panel/src/layout/navigation/SecondaryNavbar.jsx (3)
95-95: Modern array access looks good, but might want to verify browser support.The
.at(-1)method is a nice modernization that's more readable thanarr[arr.length - 1]. Perhaps worth confirming that your target browsers support this ES2022 feature, though it's widely supported now.Also applies to: 114-114, 132-132, 165-165
42-47: CSS focus handling update looks appropriate.Removing the
:focusselector while keeping:focus-visibleis a good modern accessibility practice. This might provide better user experience by only showing focus indicators when navigating with keyboard rather than mouse clicks.
197-197: Ref initialization simplification is clean.Passing
React.createRefdirectly tomapinstead of wrapping it in an arrow function is a nice little optimization that might also be slightly more readable.packages/admin-panel-server/src/index.ts (1)
30-30: Logging addition is good, though might not align with PR objectives.The logging level visibility is a helpful improvement. However, given that the PR title mentions fixing
Linkheaders' mishandling of invalid page numbers, these logging changes across multiple servers seem somewhat tangential to the stated objective. Perhaps this represents cleanup work alongside the main fix?Likely an incorrect or invalid review comment.
packages/meditrak-app-server/package.json (1)
23-24: Double-check default log level in devSince
LOG_LEVEL=debugwas dropped, dev runs will now respect whatever the environment (or default insideconfigureWinston) provides. You might want to confirm that this still surfaces enough detail for troubleshooting, or document the new expectation in the README.packages/database/src/TupaiaDatabase.js (1)
379-384: Good debugging enhancement.The debug logging here might be quite helpful for understanding timeout behavior in production. Perhaps this could help identify which record types are consistently timing out and need optimization.
packages/server-boilerplate/src/utils/configureWinston.ts (2)
1-1: Nice centralization of configuration.Adding the environment variable utility import might help standardize how configuration is handled across the codebase.
55-55: Good environment-driven configuration.Making the log level configurable via environment variable is perhaps a more flexible approach than hardcoding. The 'info' default seems reasonable for most scenarios.
packages/central-server/src/apiV2/dashboardItems/GETDashboardItems.js (2)
2-2: Clean import organization.Moving the GETHandler import up might help with logical grouping of related imports.
21-21: Nice simplification.Removing the intermediate variable and directly returning the awaited result perhaps makes the code more concise without affecting functionality.
packages/server-boilerplate/src/microService/auth/middleware.ts (2)
53-54: Clearer error messaging.The improved error message might help developers understand exactly what authentication methods are supported when the header is missing.
73-75: More specific error feedback.This error message perhaps provides better clarity about what went wrong when the authorization header format is incorrect.
packages/central-server/src/index.js (3)
4-30: Good import organization.The grouping and reordering of imports might help with code readability and logical organization of dependencies.
31-31: Sensible initialization order.Calling configureWinston() before configureEnv() perhaps ensures logging is properly set up before other configuration steps that might need to log messages.
94-94: Helpful operational visibility.Adding the log level output might be quite useful for understanding the effective logging configuration when troubleshooting issues in production.
packages/web-config-server/src/index.js (3)
23-23: Nice fix for the logging level reporting!This addresses the past review comment by dynamically outputting the current Winston level instead of hardcoding "debug". Much cleaner approach.
10-10: Good placement of configureWinston() call.Calling
configureWinston()beforeconfigureEnv()might make sense to ensure logging is set up early in the initialization process.
24-24: Sensible default for aggregation description.Using
getEnvVarOrDefaultwith 'production' as the default seems like a reasonable choice for the aggregation URL prefix.packages/central-server/src/apiV2/GETHandler/helpers.js (3)
8-18: Well-structured pageSize parsing function.The
parsePageSizeQueryParamfunction handles the different cases nicely - 'ALL', nullish values, and positive integers. The warning log for invalid values is helpful for debugging.
39-45: Improved validation for lastPage parameter.The explicit check for
Number.isInteger(lastPage) && lastPage > 0 && Number.isFinite(lastPage)is more robust than the previous logic. This should handle edge cases better when generating link headers.
56-57: Clearer logic for next link generation.The comment and condition
if (currentPage < lastPage || !lastPageIsKnown)makes the intent clear - include next link when last page is unknown or we haven't reached it yet.packages/tupaia-web-server/src/routes/DashboardsRoute.ts (3)
18-24: Good conversion from type alias to interface.Converting
DashboardsRequestfrom a type alias to an interface extendingRequestis cleaner and more explicit about the relationship.
67-73: Clearer destructuring in buildResponse.The explicit destructuring at the method start makes the dependencies more visible and the code easier to follow.
210-214: Improved condition logic readability.The explicit variable assignment for
dashboardCodewith the ternary operator makes the logic clearer than the previous inline approach.packages/api-client/src/connections/ApiConnection.ts (3)
76-81: Sensible early return for text responses.Returning text responses immediately without verification might make sense, though the TODO comment suggests this could be refined further with proper error handling.
102-116: Improved verifyResponse logic.The updated
verifyResponsemethod now only throws errors for JSON responses, which aligns with the content type handling in the request method. This prevents unnecessary error throwing for non-JSON responses.
85-90: Good fallback for non-JSON content.The logic to return the response object for non-JSON content allows the receiving code to handle parsing as needed, which is flexible.
packages/admin-panel/src/table/actions.js (2)
42-47: Well-thought-out parsing helper function.The
parseIntOrInfinityfunction handles the edge cases nicely - finite numbers, NaN, and non-finite values. The explicit checks make the behavior clear, even if some might be redundant as noted in the past review comments.
106-109: Consistent parsing for pagination headers.Using
parseIntOrInfinityfor both the total count and last page number creates consistency in how pagination metadata is handled. This should make the pagination logic more robust.packages/central-server/src/apiV2/GETHandler/GETHandler.js (6)
1-3: Clean import organization.The addition of nullish utility functions and the spacing adjustment help with readability.
6-12: Good addition of helper functions.The new imports
generateLinkHeaderandparsePageSizeQueryParamhelp centralize the pagination logic that was mentioned in the past review comments.
14-14: Proper constant export.Making
DEFAULT_PAGE_SIZEexportable might be useful for other modules that need consistent pagination defaults.
40-50: Elegant handling of nullish limits.The refactored logic nicely addresses the 'ALL' pageSize scenario mentioned in past comments by returning only the limit when it's nullish, preventing pagination parameters from being applied inappropriately.
80-84: Conditional offset calculation looks correct.The integer check for both
limitandpagebefore calculating offset should prevent the NaN issues that were mentioned in past comments. This might be a safer approach than the previous implementation.
146-154: Clean separation of concerns.Moving header construction to a separate method improves readability, and the parallel execution of record fetching and counting might provide better performance.
packages/utils/src/errors.js (3)
16-16: Consistent error name assignments improve debugging.The explicit
this.nameassignments across all error classes should help with error identification, especially when dealing with instanceof checks across different execution contexts as mentioned in past comments.Also applies to: 27-27, 39-39, 46-46, 53-53, 60-60, 67-67, 128-128, 136-136, 145-145, 158-158, 172-172, 179-179, 192-192, 208-208, 221-221, 235-235
122-127: Cleaner conditional logic in UploadError.The refactored constructor logic might be easier to follow than nested ternary operations. The control flow appears to handle the same cases as before but with better readability.
168-168: Nice use of filter(Boolean) for array cleanup.Using
filter(Boolean)to remove falsy values before joining might be more expressive than checking each condition separately.
jaskfla
changed the title
Link header when not paginatingLink header unless paginating
…paia into dashboard-rel
| @@ -13,6 +13,7 @@ export class RespondingError extends Error { | |||
| */ | |||
| constructor(message, statusCode, extraFields = {}, originalError = null) { | |||
| super(message, { cause: originalError }); | |||
| this.name = 'RespondingError'; | |||
There was a problem hiding this comment.
Used this for debugging, but feels smart to keep. Turns out instanceof isn’t necessarily reliable when crossing realm boundaries
|
|
||
| configureWinston(); |
There was a problem hiding this comment.
Unrelated change
All servers now use configureWinston() and print the log level when starting up. This was previously done inconsistently
There was a problem hiding this comment.
Sorry, this issue was a journey, so the commit history is unsalvageable
The actual fix is contained to two files in @tupaia/central-server (links scroll you there):
There was a problem hiding this comment.
Unrelated refactor
Committed to wrong branch. Key change in this file replaces arr[arr.length - 1] with arr.at(-1)
packages/ui-components/src/components/FilterableTable/FilterableTable.tsx
Show resolved
Hide resolved
Issue #:
Changes:
Screenshots: