Open
Conversation
ユーザとタスクの情報を一度に取得するようにしました。 GetShiftCardsByUserAndDateAndWeather関数も同様に、JOINを使用して ユーザとタスクの情報を一度に取得するようにしました。
….com/NUTFes/SeeFT into feat/nagi/247-fix-backend-n-plus-one
ベンチマーク計測時にSQLログが大量出力され結果を歪めるため、 DEBUG_SQL=0で無効化できるようにした。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
shift_repository.UsersのJOIN化に合わせて、usecase側のループ内の 個別userRep.Find呼び出しを削除し、JOINの結果を直接Scanするように変更。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- BenchmarkGetShiftCardsByUserAndDateAndWeather: 大量データでの速度計測 - TestShiftCardsIntegrity: 冪等性チェックと構造検証 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MySQL形式の?プレースホルダーをpq.Array+ANY($1::text[])に置換し、 PostgreSQLで正しく動作するようにした Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
taminororo
force-pushed
the
feat/nagi/247-fix-backend-n-plus-one
branch
from
09427d3 to
f6cc23d
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis pull request adds SQL debug logging controlled by a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/lib/internals/repository/shift_repository.go (1)
68-68:⚠️ Potential issue | 🟠 MajorGuard SQL printing behind
DEBUG_SQLlike other repositories.Line [68] still logs SQL unconditionally. This is inconsistent with
api/lib/internals/repository/abstract/abstract_repository.goLines [34-36] and can leak operational data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/lib/internals/repository/shift_repository.go` at line 68, The unconditional SQL print in shift_repository.go (the fmt.Printf("\x1b[36m%s\n", query) call) should be guarded the same way as other repos: check the DEBUG_SQL environment/config flag before printing. Modify the code around the query print to only execute when DEBUG_SQL is enabled (matching the pattern used in abstract_repository.go's conditional logging), i.e., read the DEBUG_SQL flag and only call the fmt.Printf for the query when that flag is true.api/lib/internals/repository/session_repository.go (1)
32-33:⚠️ Potential issue | 🔴 CriticalParameterize all SQL queries to prevent SQL injection.
Lines 32-33, 46-47, 59-60, and 69-70 build SQL statements via string concatenation with user-supplied
accessTokenanduserIDvalues. This is vulnerable to SQL injection attacks. Use parameterized queries instead.Example fix pattern (PostgreSQL with pq driver)
- query := "delete from session where access_token = '" + accessToken + "'" - _, err := r.client.DB().ExecContext(c, query) + query := "delete from session where access_token = $1" + _, err := r.client.DB().ExecContext(c, query, accessToken)Additionally, the debug logging at lines 37-39, 51-53, 61-63, and 74-76 logs full queries to stdout including access tokens. Either remove debug logging of sensitive data or ensure it is only used in development environments with explicit opt-in.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/lib/internals/repository/session_repository.go` around lines 32 - 33, The SQL is built via string concatenation using userID and accessToken (e.g., the ExecContext call on r.client.DB().ExecContext) which is vulnerable to SQL injection; change each concatenated query to a parameterized query using placeholders (Postgres-style $1, $2) and pass userID and accessToken as arguments to ExecContext/QueryContext instead of interpolating them into the SQL (update the insert, select, delete/update calls in this file accordingly). Also remove or stop printing raw queries containing access tokens in debug logs (replace with non-sensitive placeholders or conditional dev-only logging) so accessToken values are never logged. Ensure the functions that currently build these strings (the repository methods in session_repository.go) are updated consistently to use parameterized statements and safe logging.
🧹 Nitpick comments (25)
gas/package.json (2)
2-3: Mark this package as private to prevent accidental publish.Since this appears to be internal deployment tooling, adding
private: trueis a useful guardrail.Proposed tweak
{ "name": "gas", "version": "1.0.0", + "private": true, "main": "index.js",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gas/package.json` around lines 2 - 3, Add the "private": true field to the package.json manifest to prevent accidental publishing; update the top-level object alongside the existing "name" and "version" entries (i.e., add the "private" boolean property) so the package is marked private for npm/yarn registries.
5-7: Avoid a hard-failing placeholder test script.A default
npm testfailure can create noisy CI failures for this tooling package. Consider a non-failing placeholder until real tests are added.Proposed tweak
"scripts": { - "test": "echo \"Error: no test specified\" && exit 1" + "test": "echo \"No tests configured for gas tooling\"" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gas/package.json` around lines 5 - 7, The package.json has a hard-failing "test" script under "scripts" that causes noisy CI failures; change the "test" script value (the scripts.test entry) to a non-failing placeholder (for example, echo a message and exit 0 or use a lightweight no-op command) so npm test succeeds until real tests are added; update the scripts object to replace the failing command with the non-failing placeholder and commit the package.json change.gas/shift/カスタム関数.js (1)
1-9: Remove or activate this function instead of committing it fully commented out.This file currently introduces dead code only. Prefer deleting it or shipping an executable version with usage/tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gas/shift/カスタム関数.js` around lines 1 - 9, The file contains a fully commented-out function JOIN_FILTERED_NAMES which introduces dead code; either remove this commented block or restore it to an executable function by uncommenting and ensuring it has proper usage/tests; specifically, reinstate the JOIN_FILTERED_NAMES function body (using SpreadsheetApp, getSheetByName("準々備日"), ranges A3:A and BO3:BO, filtering by targetValue and returning joined names), add a small unit/integration test or example invocation and any necessary exports so the function is actually run or validated before committing.api/lib/usecase/shift_usecase.go (5)
1369-1375: Same silent error handling issue for task scan.Proposed fix
for taskRows.Next() { var task entity.Task if err := taskRows.Scan(&task.ID, &task.Task, &task.PlaceID, &task.Url, &task.BureauID, &task.MaxMember, &task.Color, &task.Remark, &task.YearID, &task.CreatedAt, &task.UpdatedAt); err != nil { + log.Printf("Failed to scan task row: %v", err) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/lib/usecase/shift_usecase.go` around lines 1369 - 1375, The loop over taskRows uses taskRows.Scan(...) and silently continues on scan errors, losing error context; update the code in the loop that iterates taskRows.Next() (the Scan call for entity.Task and the population of taskMap) to handle scan errors explicitly—either return the error up to the caller or log it with context (include the offending row data if helpful) instead of using continue, so failures in taskRows.Scan are not swallowed and taskMap population errors are visible.
1351-1357: Silently swallowing scan errors loses diagnostic information.When
rows.Scanfails, the error is silently ignored withcontinue. This could hide data corruption or schema mismatches. Consider logging the error at minimum.Proposed fix
for userRows.Next() { var user entity.User if err := userRows.Scan(&user.ID, &user.Name, &user.Mail, &user.GradeID, &user.DepartmentID, &user.BureauID, &user.RoleID, &user.StudentNumber, &user.Tel, &user.Password, &user.CreatedAt, &user.UpdatedAt); err != nil { + log.Printf("Failed to scan user row: %v", err) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/lib/usecase/shift_usecase.go` around lines 1351 - 1357, The loop over userRows is silently discarding Scan errors (in the block using userRows.Next() and userRows.Scan(&user.ID, ...)), which hides failures; change the handler so that when userRows.Scan returns an error you log the error (including the error value and some context like the user name/index) via the package logger or return the error up (e.g., wrap with context and return) instead of just continue; update the code around userRows, the Scan call, and the population of userMap to ensure errors are not swallowed and diagnostics are recorded.
1476-1481: Silent error handling in loadGradeMap.Scan errors are silently ignored, which could mask schema issues.
Proposed fix
for rows.Next() { var grade entity.Grade if err := rows.Scan(&grade.ID, &grade.Grade, &grade.CreatedAt, &grade.UpdatedAt); err != nil { + log.Printf("Failed to scan grade row: %v", err) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/lib/usecase/shift_usecase.go` around lines 1476 - 1481, In loadGradeMap, do not silently ignore Scan errors: replace the current continue inside the rows.Scan error branch with proper error handling—either return the error up the stack or log it with context (including the row data and the error) and stop processing; specifically update the loop that uses rows.Next() and rows.Scan(&grade.ID, &grade.Grade, &grade.CreatedAt, &grade.UpdatedAt) so that on scan failure you propagate the error (return err) or call the package logger with a message referencing loadGradeMap and the failing grade/rows, rather than silently continuing and populating gradeMap.
1442-1447: Error fromFindByNameis discarded before checking scan result.The error returned by
FindByName(line 1443) is assigned to_and ignored. If the query itself fails (not just a missing row), the subsequentScanwill fail, but the root cause is lost.Proposed fix
// 新規作成したタスクを再取得してマップに追加 - taskRow, _ := u.taskRep.FindByName(ctx, change.TaskName) + taskRow, findErr := u.taskRep.FindByName(ctx, change.TaskName) + if findErr != nil { + return errors.Wrapf(findErr, "タスク再取得クエリ失敗: %v", change.TaskName) + } if err := taskRow.Scan(&task.ID, &task.Task, &task.PlaceID, &task.Url, &task.BureauID, &task.MaxMember, &task.Color, &task.Remark, &task.YearID, &task.CreatedAt, &task.UpdatedAt); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/lib/usecase/shift_usecase.go` around lines 1442 - 1447, The code discards the error from taskRep.FindByName; change the block so you capture and check that error before calling taskRow.Scan: replace the assignment to `_` with a named err (e.g., err) when calling u.taskRep.FindByName(ctx, change.TaskName), return or wrap that err if non-nil (using errors.Wrapf with context like "タスク再取得失敗: %v" or similar), and only then call taskRow.Scan(...) and check its error; keep taskMap[task.Task] = task after successful scan. This ensures FindByName errors are not lost and are surfaced correctly.
1495-1500: Silent error handling in loadBureauMap.Same issue as loadGradeMap.
Proposed fix
for rows.Next() { var bureau entity.Bureau if err := rows.Scan(&bureau.ID, &bureau.Bureau, &bureau.Color, &bureau.CreatedAt, &bureau.UpdatedAt); err != nil { + log.Printf("Failed to scan bureau row: %v", err) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/lib/usecase/shift_usecase.go` around lines 1495 - 1500, The loop in loadBureauMap silently ignores Scan errors (rows.Scan(...) { continue }) like loadGradeMap does; update loadBureauMap to surface failures instead of continuing: change the function to return (map[int]string, error) if it doesn't already, on rows.Scan errors return a wrapped error with context (including the bureau ID or row index if available), after the loop check and return rows.Err() as needed, and ensure you still populate bureauMap (bureauMap[bureau.ID] = bureau.Bureau) only on successful scans; mirror the error-handling pattern used in loadGradeMap.api/lib/usecase/shift_usecase_bench_test.go (3)
222-223: Uset.Setenvfor automatic cleanup in tests.Proposed fix
func TestShiftCardsIntegrity(t *testing.T) { - os.Setenv("DEBUG_SQL", "0") + t.Setenv("DEBUG_SQL", "0")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/lib/usecase/shift_usecase_bench_test.go` around lines 222 - 223, In TestShiftCardsIntegrity replace the manual environment setup using os.Setenv("DEBUG_SQL", "0") with t.Setenv("DEBUG_SQL", "0") so the test runtime will automatically restore the previous value after the test; locate the call in the TestShiftCardsIntegrity function and swap os.Setenv for t.Setenv (remove any manual cleanup/unset logic if present).
156-159:os.Setenvin tests may affect concurrent test execution.Setting environment variables with
os.Setenvaffects the entire process and can cause race conditions with parallel tests. Consider usingt.Setenvwhich automatically restores the original value after the test.Proposed fix
func BenchmarkGetShiftCardsByUserAndDateAndWeather(b *testing.B) { - // SQLログを無効化 - os.Setenv("DEBUG_SQL", "0") + // SQLログを無効化 (Note: b.Setenv not available in benchmarks, consider alternative) + origDebugSQL := os.Getenv("DEBUG_SQL") + os.Setenv("DEBUG_SQL", "0") + defer os.Setenv("DEBUG_SQL", origDebugSQL)Note: For the
TestShiftCardsIntegrityfunction, uset.Setenv("DEBUG_SQL", "0")instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/lib/usecase/shift_usecase_bench_test.go` around lines 156 - 159, The benchmark currently calls os.Setenv("DEBUG_SQL", "0") which mutates global process state and can race with other tests; replace that call in BenchmarkGetShiftCardsByUserAndDateAndWeather with b.Setenv("DEBUG_SQL", "0") so the value is tied to the benchmark and automatically restored, and likewise update TestShiftCardsIntegrity to use t.Setenv("DEBUG_SQL", "0") instead of os.Setenv to avoid affecting concurrent tests.
107-111: Cleanup assumes seed data hasuser_id <= 3.The cleanup query
DELETE FROM shifts WHERE user_id > 3assumes existing seed data has user IDs 1-3. This coupling to seed data could cause issues if seed data changes.Consider tracking inserted IDs explicitly or using a unique marker (e.g., a specific name prefix) for cleanup instead of relying on ID ranges.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/lib/usecase/shift_usecase_bench_test.go` around lines 107 - 111, The cleanup currently hard-codes an ID boundary in the t.Cleanup closure (sqlDB.ExecContext calls deleting FROM shifts WHERE user_id > 3 and users WHERE id > 3); instead, capture the IDs of test-inserted users when you create them (e.g., collect IDs into a slice in the test) or insert users with a unique marker (e.g., username/email prefix) and then use that marker to delete; update the t.Cleanup to delete shifts using "user_id IN (<captured IDs>)" or delete by joining on the marker and then delete users by the same captured IDs/marker so cleanup no longer depends on assumed seed ID ranges and uses the recorded IDs or marker for both sqlDB.ExecContext calls.gas/task/（内田）SeeFT送信.js (3)
55-55: HardcodedyearID = 43may need to be configurable.The year ID is hardcoded. If this needs to change annually, consider reading it from script properties or the sheet itself.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gas/task/`（内田）SeeFT送信.js at line 55, The hardcoded constant yearID = 43 should be made configurable; replace the literal with a lookup (e.g., read from ScriptProperties via PropertiesService.getScriptProperties().getProperty('YEAR_ID') or read a designated cell from the sheet) and fall back to a safe default if missing; update the declaration site where yearID is defined and ensure any code using yearID continues to use the variable (e.g., in functions that reference yearID) so the value can be changed without editing the script.
77-78: Variable shadowing:urlredefined afterurlColusage.The variable
urlat line 78 shadows/reuses a name that could be confused withurlCol(line 50). While not a bug sinceurlColis just an index, consider renaming for clarity.Proposed fix
// サーバーに送信 - const url = baseUrl + "/api/update_tasks_and_places"; + const apiUrl = baseUrl + "/api/update_tasks_and_places"; const options = { method: "post", contentType: "application/json", payload: JSON.stringify({ changes: changes }), muteHttpExceptions: false }; - const response = UrlFetchApp.fetch(url, options); + const response = UrlFetchApp.fetch(apiUrl, options);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gas/task/`（内田）SeeFT送信.js around lines 77 - 78, The local variable named url (assigned as baseUrl + "/api/update_tasks_and_places") should be renamed to avoid shadowing/confusion with urlCol; change url to a clearer name such as apiUrl or endpointUrl and update all subsequent references in the same function (where you send to the server) to use that new identifier; ensure urlCol (the index/column variable) remains unchanged and that no other variables still use the old name.
63-64:getLinkUrl()null handling could be cleaner.The ternary handles null, but consider optional chaining for readability:
Proposed fix
- const url = ritchTextValues[i][urlCol].getLinkUrl() ? String(ritchTextValues[i][urlCol].getLinkUrl()).trim(): ''; + const url = ritchTextValues[i][urlCol]?.getLinkUrl()?.trim() ?? '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gas/task/`（内田）SeeFT送信.js around lines 63 - 64, Replace the ternary null check around getLinkUrl with optional chaining and nullish coalescing for readability: use ritchTextValues[i][urlCol].getLinkUrl?.() ?? '' as the source before String(...).trim(); update the declaration of url (the variable that currently calls getLinkUrl) to use this pattern so null/undefined are handled cleanly without an explicit ? : expression.gas/shift/条件付き書式.js (1)
150-165: Performance: IndividualgetRange()calls in loop.Consider batching operations by building arrays and using
setBackgrounds()/setFontColors()once, similar to thesetBackgroundColorToSheetfunction already defined in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gas/shift/条件付き書式.js` around lines 150 - 165, The loop sets each cell's background and font color with repeated taskSheet.getRange(...) calls which is slow; instead, build two 2D arrays (backgrounds and fontColors) from values using the same index logic and getContrastTextColor(bgColor) for valid hexes, then call taskSheet.getRange(3,16, backgrounds.length, 1).setBackgrounds(backgrounds).setFontColors(fontColors) (or reuse the existing setBackgroundColorToSheet pattern) so all updates are batched in one operation.api/lib/internals/repository/user_repository.go (1)
14-14: DEBUG_SQL default is inverted—debug logging is ON by default.The condition
os.Getenv("DEBUG_SQL") != "0"evaluates totruewhen the environment variable is unset (empty string""!="0"), meaning SQL debug logging is enabled by default. This is typically undesirable in production.Consider inverting the logic to enable debug logging only when explicitly set:
Proposed fix
-var userDebugSQL = os.Getenv("DEBUG_SQL") != "0" +var userDebugSQL = os.Getenv("DEBUG_SQL") == "1"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/lib/internals/repository/user_repository.go` at line 14, The DEBUG_SQL flag is inverted because userDebugSQL is set with os.Getenv("DEBUG_SQL") != "0", which is true when the var is unset; change the condition so debug is enabled only when explicitly set (e.g., check equality to an explicit ON value such as "1" or "true"). Update the userDebugSQL initialization to explicitly compare DEBUG_SQL to a positive value (for example by matching "1" or case-insensitive "true") so the default (unset) state disables SQL debug logging.api/lib/internals/repository/task_repository.go (1)
15-15: Same DEBUG_SQL inversion issue as in user_repository.go.Debug logging is enabled by default when
DEBUG_SQLis unset.Proposed fix
-var taskDebugSQL = os.Getenv("DEBUG_SQL") != "0" +var taskDebugSQL = os.Getenv("DEBUG_SQL") == "1"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/lib/internals/repository/task_repository.go` at line 15, The taskDebugSQL variable is inverted and enables SQL debug logging when DEBUG_SQL is unset; change its initialization from var taskDebugSQL = os.Getenv("DEBUG_SQL") != "0" to check for an explicit enable value (e.g. var taskDebugSQL = os.Getenv("DEBUG_SQL") == "1") so debug is off by default; update the task_repository.go declaration of taskDebugSQL accordingly (matching the fix used in user_repository.go).gas/task/タスク塗り分け.js (1)
84-109: Performance: IndividualgetRange().setValue()calls inside loops are slow.Apps Script performs better with batch operations. Consider collecting values into arrays and using
setValues(),setBackgrounds()once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gas/task/タスク塗り分け.js` around lines 84 - 109, The code makes many individual destSheet.getRange(...).setValue()/setBackground() calls which is slow; instead build rowValues for columns A–G (formatting startTime/endTime/bindTime using Utilities.formatDate when Date instances) and write them once with destSheet.getRange(outputRow,1,1,7).setValues([rowValues]); for the Gantt fill, create a 2D values array and a matching 2D backgrounds array covering columns from startCol+1 to endCol (width = endCol - startCol) and call destSheet.getRange(outputRow, startCol+1, 1, width).setValues(valuesArray) and .setBackgrounds(backgroundsArray) in two batch calls; update references to destSheet, outputRow, startCol, endCol, properPeople, color, timezone, startTime, endTime, bindTime, taskName and level when building the arrays.gas/shift/コード.js (2)
6-6: Useconstinstead ofvarfor consistency.Line 6 uses
var uiwhile all other top-level declarations useconst. For consistency and to prevent accidental reassignment, useconst.🔧 Proposed fix
-var ui = SpreadsheetApp.getUi(); +const ui = SpreadsheetApp.getUi();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gas/shift/コード.js` at line 6, Replace the top-level mutable declaration "var ui = SpreadsheetApp.getUi();" with an immutable declaration to match other top-level declarations: change the declaration of ui from var to const (i.e., use const ui = SpreadsheetApp.getUi();) so ui cannot be reassigned and top-level declarations remain consistent.
60-61: Consider extracting hardcodedyearIDas a configurable constant.
yearID = 43is hardcoded in three places (onChange,updateShifts,updateShiftsRange). If this value changes for a new event year, multiple locations need updating. Consider defining it once at the top of the file or in script properties.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gas/shift/コード.js` around lines 60 - 61, Extract the hardcoded yearID (currently const yearID = 43) into a single configurable constant and replace the three local occurrences in onChange, updateShifts, and updateShiftsRange to reference that constant (or a getter that reads from ScriptProperties). Add a top-level const YEAR_ID = 43 (or implement getYearID() that reads ScriptProperties via PropertiesService.getScriptProperties()) and update all uses of yearID in the functions onChange, updateShifts, and updateShiftsRange to use YEAR_ID or getYearID() so you only change the year in one place.gas/rescue/コード.js (2)
63-64: Unreachable default case.The
defaultcase at lines 63-64 is unreachable becausegetSheetName()returnsnullfor any unknownrescue_type, which is already handled at lines 9-11. This isn't harmful but adds dead code.🔧 Proposed fix
break; - default: - return ContentService.createTextOutput('Invalid rescue_type').setMimeType(ContentService.MimeType.TEXT); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gas/rescue/コード.js` around lines 63 - 64, The default branch returning ContentService.createTextOutput('Invalid rescue_type') is unreachable because getSheetName(rescue_type) already returns null for unknown types and is handled earlier; remove the unreachable default case in the switch (the branch under default) or consolidate handling so only the prior null-check for getSheetName() returns the error response, ensuring switch statements (and the function getSheetName) no longer contain a dead default path.
1-3: Remove empty placeholder function.
myFunction()is empty and appears to be a placeholder. Consider removing it to keep the codebase clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gas/rescue/コード.js` around lines 1 - 3, Remove the empty placeholder function myFunction(): delete the entire function declaration for myFunction() from the file to avoid unused/empty code; ensure no other code references myFunction() (search for calls or exports) and if any exist, either remove those references or replace them with the intended implementation before deleting the function.gas/rescue/onChange.js (1)
85-100: Avoidfor...infor iterating arrays.Using
for...inon arrays iterates over property keys (indices as strings), which is not recommended. Use a traditionalforloop orfor...ofinstead for clearer intent and to avoid potential issues with inherited properties.🔧 Proposed fix
- for(let index in changes){ + for(let i = 0; i < changes.length; i++){ // サーバーに送信 - const url = baseUrl + "/" + type + "-rescues/" + changes[index].id; + const url = baseUrl + "/" + type + "-rescues/" + changes[i].id; const options = { method: "put", contentType: "application/json", payload: JSON.stringify({ - status: changes[index].status, - response: changes[index].response + status: changes[i].status, + response: changes[i].response }) };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gas/rescue/onChange.js` around lines 85 - 100, The loop uses "for...in" to iterate the changes array which iterates keys instead of elements; change it to a proper array iteration (e.g., a "for...of" loop or classic indexed "for" loop) when iterating "changes" so you access each change object directly; update references inside the loop that use changes[index] (used when building url, options, payload, and calling UrlFetchApp.fetch) to use the loop variable (e.g., "change") and keep the rest of the block (url, options, Logger/console, and UrlFetchApp.fetch) unchanged.gas/shift/Sidebar.html (1)
21-40: Consider removing commented-out code.Multiple blocks of commented-out code exist (lines 21-27, 28-40, 60-64, 84-87). If this code is no longer needed, consider removing it to improve readability. If it's for future reference, document the intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gas/shift/Sidebar.html` around lines 21 - 40, The file Sidebar.html contains multiple large commented-out blocks (a CSS button block and a script that defines isVisualizeSwapArea, updateCellInfo, and calls getSelectedCellValue/getShiftMemberCount) that reduce readability; either delete these commented sections if they are obsolete, or move them into a clearly marked archival comment at the top of the file or into a separate file, and if kept add a short comment explaining why (e.g., "kept for future use: auto-update cell info via updateCellInfo using getSelectedCellValue/getShiftMemberCount"). Ensure any removed code is recoverable via VCS and update/remove related references to isVisualizeSwapArea and updateCellInfo accordingly.gas/task/（内田）タスク適当色割り当て.js (1)
15-31: Hardcoded sheet names may require updates for future events.The sheet names are hardcoded with "44thTask_" prefix. Consider making this configurable or parameterized for easier maintenance in subsequent events.
Additionally,
getSheetByName()can returnnullif the sheet doesn't exist. The code should handle this case to avoid runtime errors.🛡️ Proposed fix with null checks
function assignRandomColors() { const ss = SpreadsheetApp.getActiveSpreadsheet(); - const sheet1 = ss.getSheetByName("44thTask_準々備日") - const sheet2 = ss.getSheetByName("44thTask_準備日") - const sheet3 = ss.getSheetByName("44thTask_当日1日目") - const sheet4 = ss.getSheetByName("44thTask_当日2日目") - const sheet5 = ss.getSheetByName("44thTask_片付け日") - - const sheets= [ - sheet1, - sheet2, - sheet3, - sheet4, - sheet5 + const sheetNames = [ + "44thTask_準々備日", + "44thTask_準備日", + "44thTask_当日1日目", + "44thTask_当日2日目", + "44thTask_片付け日" ]; - for (let i = 0; i < sheets.length; i++) { - assignRandomColorsToSheet(sheets[i]) + for (const name of sheetNames) { + const sheet = ss.getSheetByName(name); + if (sheet) { + assignRandomColorsToSheet(sheet); + } else { + Logger.log("Sheet not found: " + name); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gas/task/`（内田）タスク適当色割り当て.js around lines 15 - 31, The code hardcodes sheet names with the "44thTask_" prefix and assumes getSheetByName never returns null; update this by making the prefix (or full sheet names) configurable (e.g., a variable like taskPrefix or an array passed into the function) instead of fixed literals, and add null checks around ss.getSheetByName calls: when creating sheet1..sheet5 and when populating the sheets array, skip or log any null results and only call assignRandomColorsToSheet for non-null Sheet objects (use the existing assignRandomColorsToSheet and ss symbols to locate the call sites), so the routine tolerates missing sheets and can be reused for future events.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/lib/internals/repository/abstract/abstract_repository.go`:
- Line 13: The current var debugSQL = os.Getenv("DEBUG_SQL") != "0" makes SQL
logging opt-out; change it to be opt-in by reading DEBUG_SQL and enabling
logging only when it's explicitly set to a truthy value (e.g., "1" or "true"),
using strings.ToLower to handle case-insensitivity; update the declaration of
debugSQL (the variable in abstract_repository.go) to evaluate
os.Getenv("DEBUG_SQL") against those explicit truthy values and import the
strings package if needed.
In `@api/lib/internals/repository/session_repository.go`:
- Line 12: sessionDebugSQL currently enables debug logging when the env var is
unset and several debug print sites log full SQL including raw access tokens;
change the default so sessionDebugSQL is false unless explicitly enabled (e.g.,
os.Getenv("DEBUG_SQL") == "1") and update all debug log sites in this file that
emit SQL containing token values to never print raw tokens — instead log the SQL
with parameter placeholders and either omit token parameters or redact them
(e.g., replace token value with "<REDACTED>" or show only a fixed
prefix/suffix), using the existing sessionDebugSQL flag to gate output.
In `@api/lib/internals/repository/shift_repository.go`:
- Line 63: The query in shift_repository.go builds a SELECT that includes the
sensitive column u.password; remove u.password from the SELECT projection in the
query string (the variable named query) so the read path does not load
credentials into memory, and ensure any corresponding scan/struct mapping in the
code that consumes this query (e.g., the struct fields or rows.Scan calls used
after query execution) is updated to match the reduced column list (remove the
password field references).
- Around line 63-64: The SQL built in the variable query (used with
b.client.DB().QueryContext) concatenates inputs and leaks sensitive data by
selecting u.password; change it to a parameterized query with placeholders (e.g.
"... WHERE s.task_id = ? AND s.year_id = ? AND s.date_id = ? AND s.time_id = ?
AND s.weather_id = ?") and call b.client.DB().QueryContext(c, query, task, year,
date, time, weather) (or use $1..$5 placeholders if your driver expects them),
and remove u.password from the SELECT list so only non-sensitive user fields are
returned.
In `@gas/rescue/onChange.js`:
- Around line 13-16: Validate the retrieved API base URL from PropertiesService
before constructing requests: after calling
PropertiesService.getScriptProperties() and
properties.getProperty("API_BASE_URL") (the baseUrl variable), check that
baseUrl is a non-empty string; if it's null/empty, throw or log a clear error
and abort (e.g., throw new Error or return early) so you don't build malformed
URLs like "null/…"; update any callers in onChange.js that assume baseUrl exists
to handle the early exit.
In `@gas/shift/Sidebar.html`:
- Around line 99-105: The literal "<使い方>" in the user-facing string inside the
div with class "label" is using raw angle brackets; replace them with escaped
HTML entities (e.g. change "<使い方>" to "<使い方>") in Sidebar.html so the text
is valid HTML and renders safely.
In `@gas/shift/コード.js`:
- Line 99: The log uses an incorrect index: console.log(changes[j - 1]) can
access changes[-1] when j === 0; update the logging in the loop that references
j and changes to either log the item itself before pushing or use the last
element via changes[changes.length - 1], and/or guard the index (e.g., if (j >
0) then log changes[j - 1]); locate the console.log call referencing changes and
j and replace it with one of these safe alternatives.
- Around line 66-70: The sheet-name matching erroneously matches "準々備日" as
"準備日"; fix this by checking for the more specific substring "準々備日" before the
broader "準備日" check in all three places (the initial block, updateShifts(), and
updateShiftsRange()) where sheetName.includes(...) sets the date variable;
locate the chains using sheetName.includes("準備日") / sheetName.includes("1日目")
etc., and add a prior condition like sheetName.includes("準々備日") that assigns
date = "準々備日" before the existing "準備日" branch so the correct value is chosen.
In `@gas/shift/サイドバー動作.js`:
- Around line 91-92: The code calls getSheetByName(swappingSheetName) and
immediately uses swappingSheet.getRange(swappingCellRange) which will throw if
the sheet is null; add a null-check guard after retrieving swappingSheet (using
the existing swappingSheetName and swappingCellRange symbols) that logs or
handles the missing sheet and returns early before calling getRange; apply the
same pattern in visualizingSwapArea() where it retrieves a sheet by name so both
functions safely handle missing/renamed sheets.
- Around line 28-29: The filter is comparing a 2D-array row to a scalar—since
shifts comes from getValues() each item is an array like [value]; change the
predicate in the count line to compare the first element (e.g., use shift[0] or
destructure) against selectedShift so count = shifts.filter(shift => shift[0] ==
selectedShift).length; ensure the same fix is applied wherever shifts array
entries are compared.
- Around line 86-89: The null-check condition is wrong: change the if that
currently uses "swappingSheetName == null || !swappingCellRange == null" to
properly test both variables for null/undefined by checking "swappingSheetName
== null || swappingCellRange == null" so that the block in the function handling
the swap (references: swappingSheetName, swappingCellRange) returns early when
either value is missing.
In `@gas/shift/条件付き書式.js`:
- Around line 4-27: The loop builds sheet1..sheet7 via getSheetByName and passes
them directly to setConditionalFormattingToSheet which will throw if any are
null; before iterating, ensure each sheet is non-null (e.g., filter the sheets
array or check inside the loop) and skip missing sheets, or log a warning;
specifically validate sheet1..sheet7 and only call
setConditionalFormattingToSheet(sheet, taskSheet) for non-null sheets and ensure
taskSheet is also checked for null before use.
- Around line 38-40: The code uses ui.alert when building the confirmation
dialog (assigning to confirm) but never defines ui; replace the undefined ui
usage by obtaining the UI via SpreadsheetApp.getUi() before calling alert (e.g.,
const ui = SpreadsheetApp.getUi()), so the confirm = ui.alert(...) call inside
the conditional-formatting routine that references sheetName will use a valid UI
instance.
In `@gas/task/`（内田）タスク適当色割り当て.js:
- Around line 35-50: In assignRandomColorsToSheet, guard against sheets with no
data rows by checking sheet.getLastRow() before building the range; if lastRow
is 0 or 1 (only header or empty), return early or skip processing so you don’t
call sheet.getRange("A2:P" + lastRow) with an invalid range. Locate
assignRandomColorsToSheet and add a simple early-return when lastRow <= 1 (or
compute a valid rowCount and only call getRange when rowCount > 0) so the loop
and getRange are only executed for actual data rows.
In `@gas/task/SUMBYCOLOR.js`:
- Around line 7-8: The SUMBYCOLOR function currently assumes its parameter
`range` is a Range object and calls `range.getValues()`/`getBackgrounds()`, but
custom functions receive 2D value arrays instead; change the signature of
SUMBYCOLOR to accept two 2D arrays (e.g., valuesArray and backgroundsArray) plus
the target color, iterate both arrays in parallel, compare background color
strings to the target color, and sum numeric entries from valuesArray;
alternatively, if you must use Range methods keep the current logic but convert
SUMBYCOLOR into a server-side helper (not a custom spreadsheet function) that
retrieves a Range via SpreadsheetApp and calls getValues()/getBackgrounds();
update callers (sheet formulas or scripts) to pass the backgrounds array or call
the helper accordingly.
In `@gas/task/タスク塗り分け.js`:
- Around line 6-7: The thrown Error messages for srcSheet and destSheet are
incorrect; update the messages to match the actual sheet names being looked
up—replace the message for srcSheet to reference "44thTask_準備日" and the message
for destSheet to reference "準備日_ガントチャート_晴れ" (the variables involved are srcSheet
and destSheet in the current file).
- Around line 60-61: The variables suitable and properPeople are both assigned
from srcData[row][10] (K列) causing a duplicate; locate where suitable and
properPeople are used and either remove the redundant variable (delete the
unnecessary declaration for properPeople or suitable) or change properPeople to
read the correct column index (e.g., srcData[row][11] if it should be L列) so
they reference distinct data; update all uses of properPeople accordingly to
match the corrected source column or remove its usage if it's not needed.
In `@gas/user/コード.js`:
- Around line 8-10: In updateUsers(), validate that the script property
API_BASE_URL (retrieved via
PropertiesService.getScriptProperties().getProperty("API_BASE_URL")) is non-null
and non-empty before making any HTTP requests: if baseUrl is missing throw or
log a clear error and abort the function (or return early) so requests are not
attempted with a null URL, and include the property name in the message to aid
configuration debugging.
---
Outside diff comments:
In `@api/lib/internals/repository/session_repository.go`:
- Around line 32-33: The SQL is built via string concatenation using userID and
accessToken (e.g., the ExecContext call on r.client.DB().ExecContext) which is
vulnerable to SQL injection; change each concatenated query to a parameterized
query using placeholders (Postgres-style $1, $2) and pass userID and accessToken
as arguments to ExecContext/QueryContext instead of interpolating them into the
SQL (update the insert, select, delete/update calls in this file accordingly).
Also remove or stop printing raw queries containing access tokens in debug logs
(replace with non-sensitive placeholders or conditional dev-only logging) so
accessToken values are never logged. Ensure the functions that currently build
these strings (the repository methods in session_repository.go) are updated
consistently to use parameterized statements and safe logging.
In `@api/lib/internals/repository/shift_repository.go`:
- Line 68: The unconditional SQL print in shift_repository.go (the
fmt.Printf("\x1b[36m%s\n", query) call) should be guarded the same way as other
repos: check the DEBUG_SQL environment/config flag before printing. Modify the
code around the query print to only execute when DEBUG_SQL is enabled (matching
the pattern used in abstract_repository.go's conditional logging), i.e., read
the DEBUG_SQL flag and only call the fmt.Printf for the query when that flag is
true.
---
Nitpick comments:
In `@api/lib/internals/repository/task_repository.go`:
- Line 15: The taskDebugSQL variable is inverted and enables SQL debug logging
when DEBUG_SQL is unset; change its initialization from var taskDebugSQL =
os.Getenv("DEBUG_SQL") != "0" to check for an explicit enable value (e.g. var
taskDebugSQL = os.Getenv("DEBUG_SQL") == "1") so debug is off by default; update
the task_repository.go declaration of taskDebugSQL accordingly (matching the fix
used in user_repository.go).
In `@api/lib/internals/repository/user_repository.go`:
- Line 14: The DEBUG_SQL flag is inverted because userDebugSQL is set with
os.Getenv("DEBUG_SQL") != "0", which is true when the var is unset; change the
condition so debug is enabled only when explicitly set (e.g., check equality to
an explicit ON value such as "1" or "true"). Update the userDebugSQL
initialization to explicitly compare DEBUG_SQL to a positive value (for example
by matching "1" or case-insensitive "true") so the default (unset) state
disables SQL debug logging.
In `@api/lib/usecase/shift_usecase_bench_test.go`:
- Around line 222-223: In TestShiftCardsIntegrity replace the manual environment
setup using os.Setenv("DEBUG_SQL", "0") with t.Setenv("DEBUG_SQL", "0") so the
test runtime will automatically restore the previous value after the test;
locate the call in the TestShiftCardsIntegrity function and swap os.Setenv for
t.Setenv (remove any manual cleanup/unset logic if present).
- Around line 156-159: The benchmark currently calls os.Setenv("DEBUG_SQL", "0")
which mutates global process state and can race with other tests; replace that
call in BenchmarkGetShiftCardsByUserAndDateAndWeather with b.Setenv("DEBUG_SQL",
"0") so the value is tied to the benchmark and automatically restored, and
likewise update TestShiftCardsIntegrity to use t.Setenv("DEBUG_SQL", "0")
instead of os.Setenv to avoid affecting concurrent tests.
- Around line 107-111: The cleanup currently hard-codes an ID boundary in the
t.Cleanup closure (sqlDB.ExecContext calls deleting FROM shifts WHERE user_id >
3 and users WHERE id > 3); instead, capture the IDs of test-inserted users when
you create them (e.g., collect IDs into a slice in the test) or insert users
with a unique marker (e.g., username/email prefix) and then use that marker to
delete; update the t.Cleanup to delete shifts using "user_id IN (<captured
IDs>)" or delete by joining on the marker and then delete users by the same
captured IDs/marker so cleanup no longer depends on assumed seed ID ranges and
uses the recorded IDs or marker for both sqlDB.ExecContext calls.
In `@api/lib/usecase/shift_usecase.go`:
- Around line 1369-1375: The loop over taskRows uses taskRows.Scan(...) and
silently continues on scan errors, losing error context; update the code in the
loop that iterates taskRows.Next() (the Scan call for entity.Task and the
population of taskMap) to handle scan errors explicitly—either return the error
up to the caller or log it with context (include the offending row data if
helpful) instead of using continue, so failures in taskRows.Scan are not
swallowed and taskMap population errors are visible.
- Around line 1351-1357: The loop over userRows is silently discarding Scan
errors (in the block using userRows.Next() and userRows.Scan(&user.ID, ...)),
which hides failures; change the handler so that when userRows.Scan returns an
error you log the error (including the error value and some context like the
user name/index) via the package logger or return the error up (e.g., wrap with
context and return) instead of just continue; update the code around userRows,
the Scan call, and the population of userMap to ensure errors are not swallowed
and diagnostics are recorded.
- Around line 1476-1481: In loadGradeMap, do not silently ignore Scan errors:
replace the current continue inside the rows.Scan error branch with proper error
handling—either return the error up the stack or log it with context (including
the row data and the error) and stop processing; specifically update the loop
that uses rows.Next() and rows.Scan(&grade.ID, &grade.Grade, &grade.CreatedAt,
&grade.UpdatedAt) so that on scan failure you propagate the error (return err)
or call the package logger with a message referencing loadGradeMap and the
failing grade/rows, rather than silently continuing and populating gradeMap.
- Around line 1442-1447: The code discards the error from taskRep.FindByName;
change the block so you capture and check that error before calling
taskRow.Scan: replace the assignment to `_` with a named err (e.g., err) when
calling u.taskRep.FindByName(ctx, change.TaskName), return or wrap that err if
non-nil (using errors.Wrapf with context like "タスク再取得失敗: %v" or similar), and
only then call taskRow.Scan(...) and check its error; keep taskMap[task.Task] =
task after successful scan. This ensures FindByName errors are not lost and are
surfaced correctly.
- Around line 1495-1500: The loop in loadBureauMap silently ignores Scan errors
(rows.Scan(...) { continue }) like loadGradeMap does; update loadBureauMap to
surface failures instead of continuing: change the function to return
(map[int]string, error) if it doesn't already, on rows.Scan errors return a
wrapped error with context (including the bureau ID or row index if available),
after the loop check and return rows.Err() as needed, and ensure you still
populate bureauMap (bureauMap[bureau.ID] = bureau.Bureau) only on successful
scans; mirror the error-handling pattern used in loadGradeMap.
In `@gas/package.json`:
- Around line 2-3: Add the "private": true field to the package.json manifest to
prevent accidental publishing; update the top-level object alongside the
existing "name" and "version" entries (i.e., add the "private" boolean property)
so the package is marked private for npm/yarn registries.
- Around line 5-7: The package.json has a hard-failing "test" script under
"scripts" that causes noisy CI failures; change the "test" script value (the
scripts.test entry) to a non-failing placeholder (for example, echo a message
and exit 0 or use a lightweight no-op command) so npm test succeeds until real
tests are added; update the scripts object to replace the failing command with
the non-failing placeholder and commit the package.json change.
In `@gas/rescue/onChange.js`:
- Around line 85-100: The loop uses "for...in" to iterate the changes array
which iterates keys instead of elements; change it to a proper array iteration
(e.g., a "for...of" loop or classic indexed "for" loop) when iterating "changes"
so you access each change object directly; update references inside the loop
that use changes[index] (used when building url, options, payload, and calling
UrlFetchApp.fetch) to use the loop variable (e.g., "change") and keep the rest
of the block (url, options, Logger/console, and UrlFetchApp.fetch) unchanged.
In `@gas/rescue/コード.js`:
- Around line 63-64: The default branch returning
ContentService.createTextOutput('Invalid rescue_type') is unreachable because
getSheetName(rescue_type) already returns null for unknown types and is handled
earlier; remove the unreachable default case in the switch (the branch under
default) or consolidate handling so only the prior null-check for getSheetName()
returns the error response, ensuring switch statements (and the function
getSheetName) no longer contain a dead default path.
- Around line 1-3: Remove the empty placeholder function myFunction(): delete
the entire function declaration for myFunction() from the file to avoid
unused/empty code; ensure no other code references myFunction() (search for
calls or exports) and if any exist, either remove those references or replace
them with the intended implementation before deleting the function.
In `@gas/shift/Sidebar.html`:
- Around line 21-40: The file Sidebar.html contains multiple large commented-out
blocks (a CSS button block and a script that defines isVisualizeSwapArea,
updateCellInfo, and calls getSelectedCellValue/getShiftMemberCount) that reduce
readability; either delete these commented sections if they are obsolete, or
move them into a clearly marked archival comment at the top of the file or into
a separate file, and if kept add a short comment explaining why (e.g., "kept for
future use: auto-update cell info via updateCellInfo using
getSelectedCellValue/getShiftMemberCount"). Ensure any removed code is
recoverable via VCS and update/remove related references to isVisualizeSwapArea
and updateCellInfo accordingly.
In `@gas/shift/カスタム関数.js`:
- Around line 1-9: The file contains a fully commented-out function
JOIN_FILTERED_NAMES which introduces dead code; either remove this commented
block or restore it to an executable function by uncommenting and ensuring it
has proper usage/tests; specifically, reinstate the JOIN_FILTERED_NAMES function
body (using SpreadsheetApp, getSheetByName("準々備日"), ranges A3:A and BO3:BO,
filtering by targetValue and returning joined names), add a small
unit/integration test or example invocation and any necessary exports so the
function is actually run or validated before committing.
In `@gas/shift/コード.js`:
- Line 6: Replace the top-level mutable declaration "var ui =
SpreadsheetApp.getUi();" with an immutable declaration to match other top-level
declarations: change the declaration of ui from var to const (i.e., use const ui
= SpreadsheetApp.getUi();) so ui cannot be reassigned and top-level declarations
remain consistent.
- Around line 60-61: Extract the hardcoded yearID (currently const yearID = 43)
into a single configurable constant and replace the three local occurrences in
onChange, updateShifts, and updateShiftsRange to reference that constant (or a
getter that reads from ScriptProperties). Add a top-level const YEAR_ID = 43 (or
implement getYearID() that reads ScriptProperties via
PropertiesService.getScriptProperties()) and update all uses of yearID in the
functions onChange, updateShifts, and updateShiftsRange to use YEAR_ID or
getYearID() so you only change the year in one place.
In `@gas/shift/条件付き書式.js`:
- Around line 150-165: The loop sets each cell's background and font color with
repeated taskSheet.getRange(...) calls which is slow; instead, build two 2D
arrays (backgrounds and fontColors) from values using the same index logic and
getContrastTextColor(bgColor) for valid hexes, then call
taskSheet.getRange(3,16, backgrounds.length,
1).setBackgrounds(backgrounds).setFontColors(fontColors) (or reuse the existing
setBackgroundColorToSheet pattern) so all updates are batched in one operation.
In `@gas/task/`（内田）SeeFT送信.js:
- Line 55: The hardcoded constant yearID = 43 should be made configurable;
replace the literal with a lookup (e.g., read from ScriptProperties via
PropertiesService.getScriptProperties().getProperty('YEAR_ID') or read a
designated cell from the sheet) and fall back to a safe default if missing;
update the declaration site where yearID is defined and ensure any code using
yearID continues to use the variable (e.g., in functions that reference yearID)
so the value can be changed without editing the script.
- Around line 77-78: The local variable named url (assigned as baseUrl +
"/api/update_tasks_and_places") should be renamed to avoid shadowing/confusion
with urlCol; change url to a clearer name such as apiUrl or endpointUrl and
update all subsequent references in the same function (where you send to the
server) to use that new identifier; ensure urlCol (the index/column variable)
remains unchanged and that no other variables still use the old name.
- Around line 63-64: Replace the ternary null check around getLinkUrl with
optional chaining and nullish coalescing for readability: use
ritchTextValues[i][urlCol].getLinkUrl?.() ?? '' as the source before
String(...).trim(); update the declaration of url (the variable that currently
calls getLinkUrl) to use this pattern so null/undefined are handled cleanly
without an explicit ? : expression.
In `@gas/task/`（内田）タスク適当色割り当て.js:
- Around line 15-31: The code hardcodes sheet names with the "44thTask_" prefix
and assumes getSheetByName never returns null; update this by making the prefix
(or full sheet names) configurable (e.g., a variable like taskPrefix or an array
passed into the function) instead of fixed literals, and add null checks around
ss.getSheetByName calls: when creating sheet1..sheet5 and when populating the
sheets array, skip or log any null results and only call
assignRandomColorsToSheet for non-null Sheet objects (use the existing
assignRandomColorsToSheet and ss symbols to locate the call sites), so the
routine tolerates missing sheets and can be reused for future events.
In `@gas/task/タスク塗り分け.js`:
- Around line 84-109: The code makes many individual
destSheet.getRange(...).setValue()/setBackground() calls which is slow; instead
build rowValues for columns A–G (formatting startTime/endTime/bindTime using
Utilities.formatDate when Date instances) and write them once with
destSheet.getRange(outputRow,1,1,7).setValues([rowValues]); for the Gantt fill,
create a 2D values array and a matching 2D backgrounds array covering columns
from startCol+1 to endCol (width = endCol - startCol) and call
destSheet.getRange(outputRow, startCol+1, 1, width).setValues(valuesArray) and
.setBackgrounds(backgroundsArray) in two batch calls; update references to
destSheet, outputRow, startCol, endCol, properPeople, color, timezone,
startTime, endTime, bindTime, taskName and level when building the arrays.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20c9743f-f069-4ea9-882d-eb95572a404b
⛔ Files ignored due to path filters (1)
gas/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
.gitignoreapi/lib/internals/repository/abstract/abstract_repository.goapi/lib/internals/repository/session_repository.goapi/lib/internals/repository/shift_repository.goapi/lib/internals/repository/task_repository.goapi/lib/internals/repository/user_repository.goapi/lib/usecase/shift_usecase.goapi/lib/usecase/shift_usecase_bench_test.goapi/lib/usecase/testdata/shift_cards_actual.jsonapi/lib/usecase/testdata/shift_cards_golden.jsongas/.gitignoregas/package.jsongas/rescue/appsscript.jsongas/rescue/onChange.jsgas/rescue/コード.jsgas/shift/Sidebar.htmlgas/shift/appsscript.jsongas/shift/カスタム関数.jsgas/shift/コード.jsgas/shift/サイドバー動作.jsgas/shift/条件付き書式.jsgas/task/SUMBYCOLOR.jsgas/task/appsscript.jsongas/task/タスク塗り分け.jsgas/task/（内田）SeeFT送信.jsgas/task/（内田）タスク適当色割り当て.jsgas/user/appsscript.jsongas/user/コード.js
GASスクリプトは本PRの修正対象 (バックエンドのN+1問題) と無関係なため、 別ブランチで扱うべく本PRからは削除する。 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
今までの `!= "0"` は env 未設定時に true となり、本番環境で 意図せず SQL ログが出力されてしまう fail-open な状態だった。 `== "1"` に変更し、明示的に有効化したときだけログ出力する。 session_repository.go では access_token を含むクエリがログに 出ていたため、特に本番でのトークン漏洩リスクがあった。 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
対応Issue
resolve #251
概要
N + 1問題の修正
画面スクリーンショット等
URLスクリーンショット
テスト項目
備考
Summary by CodeRabbit
Refactor
Chores