[#363] 푸시 알람과 웹페이지 데이터의 hard deletion을 제거한다#364
Conversation
opficdev
commented
Apr 6, 2026
opficdev
commented
- closed 푸시 알람과 웹페이지 데이터의 hard deletion을 제거한다 #363
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions push notification and web page deletion to an immediate soft-delete model using the isDeleted flag and introduces a comprehensive unit and integration testing suite. Feedback suggests simplifying Firebase function error handling by removing redundant recovery logic and optimizing the test infrastructure. Key recommendations include avoiding #filePath for configuration discovery to ensure CI compatibility, reusing ISO8601DateFormatter for performance, and removing obsolete Task.sleep calls in integration tests.
| } catch (error) { | ||
| const currentNotificationSnapshot = await notificationRef.get(); | ||
| if (currentNotificationSnapshot.exists && currentNotificationSnapshot.data()?.isDeleted !== true) { | ||
| await notificationRef.update({ | ||
| deletingAt: admin.firestore.FieldValue.delete() | ||
| }); | ||
| try { | ||
| const currentNotificationSnapshot = await notificationRef.get(); | ||
| if (currentNotificationSnapshot.exists && currentNotificationSnapshot.data()?.isDeleted === true) { | ||
| await notificationRef.update({ | ||
| deletingAt: FieldValue.delete(), | ||
| isDeleted: false | ||
| }); | ||
| } | ||
| } catch (cleanupError) { | ||
| logger.error("푸시 알림 삭제 요청 cleanup 실패", toError(cleanupError), { userId, notificationId }); | ||
| } | ||
|
|
||
| logger.error("푸시 알림 삭제 요청 실패", toError(error), { | ||
| userId, | ||
| notificationId | ||
| }); | ||
| logger.error("푸시 알림 삭제 요청 실패", toError(error), { userId, notificationId }); | ||
| throw new HttpsError("internal", "푸시 알림 삭제 요청에 실패했습니다."); | ||
| } |
There was a problem hiding this comment.
이번 PR은 푸시 알림과 웹페이지 삭제 시 사용하던 deletingAt 기반의 유예 기간(5초) 및 Task Queue 로직을 제거하고, isDeleted 필드를 이용한 단순 Soft Deletion 방식으로 전환합니다.
catch 블록 내의 복구 로직이 부적절합니다. notificationRef.set 호출이 실패하여 에러가 발생했다면 isDeleted 필드가 true로 변경되지 않았을 것이므로, isDeleted === true를 체크하여 다시 false로 되돌리는 로직은 실행될 가능성이 낮고 혼란을 줄 수 있습니다. Task Queue 로직이 제거되었으므로 단순 로깅 후 에러를 던지는 것이 적절합니다.
} catch (error) {
logger.error("푸시 알림 삭제 요청 실패", toError(error), { userId, notificationId });
throw new HttpsError("internal", "푸시 알림 삭제 요청에 실패했습니다.");
}References
- 가독성과 불필요한 로직 제거를 우선시함 (Repository Style Guide 5) (link)
| } catch (error) { | ||
| const currentWebPageSnapshot = await webPageRef.get(); | ||
| if (currentWebPageSnapshot.exists && currentWebPageSnapshot.data()?.isDeleted !== true) { | ||
| await webPageRef.update({ | ||
| deletingAt: admin.firestore.FieldValue.delete() | ||
| try { | ||
| const currentWebPageSnapshot = await webPageRef.get(); | ||
| if (currentWebPageSnapshot.exists && currentWebPageSnapshot.data()?.isDeleted === true) { | ||
| await webPageRef.update({ | ||
| deletingAt: FieldValue.delete(), | ||
| isDeleted: false | ||
| }); | ||
| } | ||
| } catch (cleanupError) { | ||
| logger.error("웹페이지 삭제 요청 cleanup 실패", toError(cleanupError), { | ||
| userId, | ||
| urlString | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
위의 푸시 알림 삭제 로직과 마찬가지로, catch 블록 내의 복구 로직이 불필요합니다. set 연산 실패 시에는 데이터가 변경되지 않으므로, 복잡한 확인 및 업데이트 로직 대신 단순 에러 처리를 권장합니다.
} catch (error) {
logger.error("웹페이지 삭제 요청 실패", toError(error), {
userId,
urlString
});
throw new HttpsError("internal", "웹페이지 삭제 요청에 실패했습니다.");
}References
- 가독성과 불필요한 로직 제거를 우선시함 (Repository Style Guide 5) (link)
| var fileURL = URL(fileURLWithPath: #filePath) | ||
|
|
||
| while fileURL.lastPathComponent != "SwiftUI_DevLog" { | ||
| let nextURL = fileURL.deletingLastPathComponent() | ||
| if nextURL == fileURL { | ||
| throw RESTError.missingConfiguration | ||
| } | ||
| fileURL = nextURL | ||
| } |
There was a problem hiding this comment.
#filePath를 사용하여 프로젝트 루트를 찾는 방식은 CI 환경이나 다른 개발자의 환경에서 경로 구조가 다를 경우 테스트 실패의 원인이 될 수 있어 취약합니다. 테스트 타겟의 Bundle 리소스를 활용하거나 환경 변수를 통해 설정 파일 경로를 주입받는 방식이 더 안정적입니다.
| } | ||
|
|
||
| func timestampValue(_ value: Date) -> [String: Any] { | ||
| ["timestampValue": ISO8601DateFormatter().string(from: value)] |
There was a problem hiding this comment.
ISO8601DateFormatter를 매번 생성하는 것은 성능상 비효율적입니다. static 프로퍼티로 선언하여 재사용하는 것을 권장합니다.
References
- 성능 최적화 고려 (Repository Style Guide 5) (link)
| return visibleNotificationIds.contains(notificationId) | ||
| } | ||
|
|
||
| try await Task.sleep(for: .seconds(6)) |
There was a problem hiding this comment.
백엔드의 5초 유예 로직이 제거되었으므로, 통합 테스트에서 Task.sleep(for: .seconds(6))을 통해 대기할 필요가 없습니다. 불필요한 대기는 전체 테스트 실행 시간을 늘리고 가독성을 떨어뜨립니다.
| return visibleWebPageURLs.contains(seededWebPage.urlString) | ||
| } | ||
|
|
||
| try await Task.sleep(for: .seconds(6)) |
There was a problem hiding this comment.
웹페이지 삭제 역시 유예 로직이 제거되었으므로, 6초간의 대기 시간은 불필요합니다. 테스트의 효율성을 위해 제거를 권장합니다.
1 participant
