Page MenuHomePhabricator
Differential D8496

[native] clean up InlineEngagement component
ClosedPublic
Actions

Authored by ginsu on Jul 13 2023, 9:53 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 25, 12:07 AM
Unknown Object (File)
Tue, Mar 18, 4:24 PM
Unknown Object (File)
Tue, Mar 18, 3:17 AM
Unknown Object (File)
Mon, Mar 17, 6:16 PM
Unknown Object (File)
Mon, Mar 17, 6:16 PM
Unknown Object (File)
Mon, Mar 17, 6:16 PM
Unknown Object (File)
Mon, Mar 17, 6:16 PM
Unknown Object (File)
Mon, Mar 17, 6:15 PM
View All Files
Subscribers
ashoat
tomek

Details

Reviewers
atul
kamil
Commits
rCOMM2d893db09936: [native] clean up InlineEngagement component
Summary

Small cleanup diff to better organize and make the InlineEngagement component easier to read/go through. The two changes to note are

  1. Renamed threadInfo to sidebarInfo: I thought that this was a more accurate name since this was holding the sidebar thread info
  2. Reorganized/moved the styles unboundStyles so that the order of the styles went from usage in parent elements/containers to the children elements. Before this the styles were in some random order that didn't make any sense to me

Depends on D8317

Test Plan

Confirmed that InlineEngagment still looked and behaved the same.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu created this revision.Jul 13 2023, 9:53 AM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 13 2023, 9:53 AM
ginsu edited the summary of this revision. (Show Details)Jul 13 2023, 10:03 AM
ginsu added reviewers: atul, kamil.
ginsu added a parent revision: D8317: [lib] add reply/replies back to sidebar inline engagement.
Harbormaster completed remote builds in B20922: Diff 28676.Jul 13 2023, 10:10 AM
ginsu requested review of this revision.Jul 13 2023, 10:10 AM
ginsu added a child revision: D8497: [native] split the inline engagement pill into two.Jul 13 2023, 2:49 PM
kamil accepted this revision.Jul 19 2023, 5:00 AM
kamil added inline comments.
native/chat/inline-engagement.react.js
34 ↗(On Diff #28676)

after this change feels a bit not natural that this is not a SidebarInfo type, but overall your reasoning is convincing

This revision is now accepted and ready to land.Jul 19 2023, 5:00 AM
ginsu added inline comments.Jul 26 2023, 1:04 PM
native/chat/inline-engagement.react.js
34 ↗(On Diff #28676)

That is a fair point. I think an even better name for this would be sidebarThreadInfo. Will update this diff to reflect this

ginsu updated this revision to Diff 29073.Jul 26 2023, 1:11 PM

address comments

Harbormaster completed remote builds in B21229: Diff 29073.Jul 26 2023, 1:30 PM
ginsu mentioned this in D8677: [web] clean up InlineEngagement component.Jul 31 2023, 7:18 AM
ginsu updated this revision to Diff 29300.Jul 31 2023, 11:31 AM

rebase before landing

Harbormaster completed remote builds in B21388: Diff 29300.Jul 31 2023, 12:36 PM
Closed by commit rCOMM2d893db09936: [native] clean up InlineEngagement component (authored by ginsu). · Explain WhyJul 31 2023, 1:19 PM
This revision was automatically updated to reflect the committed changes.
ginsu added a commit: rCOMM2d893db09936: [native] clean up InlineEngagement component.
ginsu mentioned this in rCOMM9c1905dc7549: [web] clean up InlineEngagement component.Jul 31 2023, 1:28 PM

Revision Contents
Changeset List

Diff 29352

View Options

native/chat/composed-message.react.js

#include "DatabaseManagerBase.h" #include "DatabaseManagerBase.h"
#include "Item.h" #include "Item.h"
#include <aws/core/utils/Outcome.h> #include <aws/core/utils/Outcome.h>
#include <aws/dynamodb/model/BatchWriteItemRequest.h>
#include <aws/dynamodb/model/BatchWriteItemResult.h>
#include <aws/dynamodb/model/DeleteItemRequest.h> #include <aws/dynamodb/model/DeleteItemRequest.h>
#include <iostream> #include <iostream>
namespace comm { namespace comm {
namespace network { namespace network {
namespace database { namespace database {
Show All 23 Linesvoid DatabaseManagerBase::innerRemoveItem(const Item &item) {
const Aws::DynamoDB::Model::DeleteItemOutcome &outcome = const Aws::DynamoDB::Model::DeleteItemOutcome &outcome =
getDynamoDBClient()->DeleteItem(request); getDynamoDBClient()->DeleteItem(request);
if (!outcome.IsSuccess()) { if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage()); throw std::runtime_error(outcome.GetError().GetMessage());
} }
} }
void DatabaseManagerBase::innerBatchWriteItem(
const std::string &tableName,
std::vector<Aws::DynamoDB::Model::WriteRequest> &writeRequests) {
// Split write requests to chunks by MAX_DYNAMODB_BATCH_ITEMS size and write
// them by batch
Aws::DynamoDB::Model::BatchWriteItemOutcome outcome;
std::vector<Aws::DynamoDB::Model::WriteRequest> writeRequestsChunk;
std::vector<Aws::DynamoDB::Model::WriteRequest>::iterator chunkPositionStart,
chunkPositionEnd;
for (size_t i = 0; i < writeRequests.size(); i += MAX_DYNAMODB_BATCH_ITEMS) {
chunkPositionStart = writeRequests.begin() + i;
chunkPositionEnd = writeRequests.begin() +
std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS);
writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
chunkPositionStart, chunkPositionEnd);
tomekUnsubmitted
Done
Inline Actions
Aws::DynamoDB::Model::BatchWriteItemOutcome outcome;
- std::vector<Aws::DynamoDB::Model::WriteRequest> writeRequestsChunk;
- std::vector<Aws::DynamoDB::Model::WriteRequest>::iterator chunkPositionStart,
- chunkPositionEnd;
for (size_t i = 0; i < writeRequests.size(); i += MAX_DYNAMODB_BATCH_ITEMS) {
- chunkPositionStart = writeRequests.begin() + i;
- chunkPositionEnd = writeRequests.begin() +
+ auto chunkPositionStart = writeRequests.begin() + i;
+ auto chunkPositionEnd = writeRequests.begin() +
std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS);
- writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
+ auto writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
chunkPositionStart, chunkPositionEnd);
Aws::DynamoDB::Model::BatchWriteItemRequest writeBatchRequest;

You can simplify the code by avoiding reusing the variables and using auto

tomek: You can simplify the code by avoiding reusing the variables and using `auto`
maxAuthorUnsubmitted
Done
Inline Actions

You can simplify the code by avoiding reusing the variables and using auto

I'm not a big fan of using auto where the type can be declared explicitly. Also, reusing the same variables is a part of loop optimization here (we don't create new ones inside the loop and reuse existing allocated memory for it).

max: > You can simplify the code by avoiding reusing the variables and using `auto` I'm not a big…
tomekUnsubmitted
Not Done
Inline Actions

Also, reusing the same variables is a part of loop optimization here

Have you measured the performance difference? I don't think this type of premature optimization is beneficial - if the variable is used only locally, it is easier to reason about it: you don't have to think about its initial value and about possible usages after the loop. Also, having a variable defined inside a loop might enable some optimization (I'm guessing now). Currently, we only reuse the name - new variables are constructed in every iteration.

So overall, we have a lot more important performance issues and saving a couple of machine instructions wouldn't be noticeable. On the other hand, from readability point of view, this is a noticeable difference - you are certain that the variable is used only where it is valid.

tomek: > Also, reusing the same variables is a part of loop optimization here Have you measured the…
Aws::DynamoDB::Model::BatchWriteItemRequest writeBatchRequest;
writeBatchRequest.AddRequestItems(tableName, writeRequestsChunk);
outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage());
}
tomekUnsubmitted
Done
Inline Actions
writeBatchRequest.AddRequestItems(tableName, writeRequestsChunk);
- outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
+ auto outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage());
}
}
}

outcome isn't used outside of this loop so we can declare it here

tomek: `outcome` isn't used outside of this loop so we can declare it here
maxAuthorUnsubmitted
Done
Inline Actions

outcome isn't used outside of this loop so we can declare it here

Same as above. I'm sure we should not use auto, where the type can explicitly be used, and allocating and reusing this variable here, is an optimized way from my perspective.

max: > `outcome` isn't used outside of this loop so we can declare it here Same as above. I'm sure…
}
}
} // namespace database } // namespace database
} // namespace network } // namespace network
} // namespace comm } // namespace comm
View Options

native/chat/inline-engagement.react.js

#include "DatabaseManagerBase.h" #include "DatabaseManagerBase.h"
#include "Item.h" #include "Item.h"
#include <aws/core/utils/Outcome.h> #include <aws/core/utils/Outcome.h>
#include <aws/dynamodb/model/BatchWriteItemRequest.h>
#include <aws/dynamodb/model/BatchWriteItemResult.h>
#include <aws/dynamodb/model/DeleteItemRequest.h> #include <aws/dynamodb/model/DeleteItemRequest.h>
#include <iostream> #include <iostream>
namespace comm { namespace comm {
namespace network { namespace network {
namespace database { namespace database {
Show All 23 Linesvoid DatabaseManagerBase::innerRemoveItem(const Item &item) {
const Aws::DynamoDB::Model::DeleteItemOutcome &outcome = const Aws::DynamoDB::Model::DeleteItemOutcome &outcome =
getDynamoDBClient()->DeleteItem(request); getDynamoDBClient()->DeleteItem(request);
if (!outcome.IsSuccess()) { if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage()); throw std::runtime_error(outcome.GetError().GetMessage());
} }
} }
void DatabaseManagerBase::innerBatchWriteItem(
const std::string &tableName,
std::vector<Aws::DynamoDB::Model::WriteRequest> &writeRequests) {
// Split write requests to chunks by MAX_DYNAMODB_BATCH_ITEMS size and write
// them by batch
Aws::DynamoDB::Model::BatchWriteItemOutcome outcome;
std::vector<Aws::DynamoDB::Model::WriteRequest> writeRequestsChunk;
std::vector<Aws::DynamoDB::Model::WriteRequest>::iterator chunkPositionStart,
chunkPositionEnd;
for (size_t i = 0; i < writeRequests.size(); i += MAX_DYNAMODB_BATCH_ITEMS) {
chunkPositionStart = writeRequests.begin() + i;
chunkPositionEnd = writeRequests.begin() +
std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS);
writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
chunkPositionStart, chunkPositionEnd);
tomekUnsubmitted
Done
Inline Actions
Aws::DynamoDB::Model::BatchWriteItemOutcome outcome;
- std::vector<Aws::DynamoDB::Model::WriteRequest> writeRequestsChunk;
- std::vector<Aws::DynamoDB::Model::WriteRequest>::iterator chunkPositionStart,
- chunkPositionEnd;
for (size_t i = 0; i < writeRequests.size(); i += MAX_DYNAMODB_BATCH_ITEMS) {
- chunkPositionStart = writeRequests.begin() + i;
- chunkPositionEnd = writeRequests.begin() +
+ auto chunkPositionStart = writeRequests.begin() + i;
+ auto chunkPositionEnd = writeRequests.begin() +
std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS);
- writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
+ auto writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
chunkPositionStart, chunkPositionEnd);
Aws::DynamoDB::Model::BatchWriteItemRequest writeBatchRequest;

You can simplify the code by avoiding reusing the variables and using auto

tomek: You can simplify the code by avoiding reusing the variables and using `auto`
maxAuthorUnsubmitted
Done
Inline Actions

You can simplify the code by avoiding reusing the variables and using auto

I'm not a big fan of using auto where the type can be declared explicitly. Also, reusing the same variables is a part of loop optimization here (we don't create new ones inside the loop and reuse existing allocated memory for it).

max: > You can simplify the code by avoiding reusing the variables and using `auto` I'm not a big…
tomekUnsubmitted
Not Done
Inline Actions

Also, reusing the same variables is a part of loop optimization here

Have you measured the performance difference? I don't think this type of premature optimization is beneficial - if the variable is used only locally, it is easier to reason about it: you don't have to think about its initial value and about possible usages after the loop. Also, having a variable defined inside a loop might enable some optimization (I'm guessing now). Currently, we only reuse the name - new variables are constructed in every iteration.

So overall, we have a lot more important performance issues and saving a couple of machine instructions wouldn't be noticeable. On the other hand, from readability point of view, this is a noticeable difference - you are certain that the variable is used only where it is valid.

tomek: > Also, reusing the same variables is a part of loop optimization here Have you measured the…
Aws::DynamoDB::Model::BatchWriteItemRequest writeBatchRequest;
writeBatchRequest.AddRequestItems(tableName, writeRequestsChunk);
outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage());
}
tomekUnsubmitted
Done
Inline Actions
writeBatchRequest.AddRequestItems(tableName, writeRequestsChunk);
- outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
+ auto outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage());
}
}
}

outcome isn't used outside of this loop so we can declare it here

tomek: `outcome` isn't used outside of this loop so we can declare it here
maxAuthorUnsubmitted
Done
Inline Actions

outcome isn't used outside of this loop so we can declare it here

Same as above. I'm sure we should not use auto, where the type can explicitly be used, and allocating and reusing this variable here, is an optimized way from my perspective.

max: > `outcome` isn't used outside of this loop so we can declare it here Same as above. I'm sure…
}
}
} // namespace database } // namespace database
} // namespace network } // namespace network
} // namespace comm } // namespace comm
View Options

native/chat/robotext-message.react.js

#include "DatabaseManagerBase.h" #include "DatabaseManagerBase.h"
#include "Item.h" #include "Item.h"
#include <aws/core/utils/Outcome.h> #include <aws/core/utils/Outcome.h>
#include <aws/dynamodb/model/BatchWriteItemRequest.h>
#include <aws/dynamodb/model/BatchWriteItemResult.h>
#include <aws/dynamodb/model/DeleteItemRequest.h> #include <aws/dynamodb/model/DeleteItemRequest.h>
#include <iostream> #include <iostream>
namespace comm { namespace comm {
namespace network { namespace network {
namespace database { namespace database {
Show All 23 Linesvoid DatabaseManagerBase::innerRemoveItem(const Item &item) {
const Aws::DynamoDB::Model::DeleteItemOutcome &outcome = const Aws::DynamoDB::Model::DeleteItemOutcome &outcome =
getDynamoDBClient()->DeleteItem(request); getDynamoDBClient()->DeleteItem(request);
if (!outcome.IsSuccess()) { if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage()); throw std::runtime_error(outcome.GetError().GetMessage());
} }
} }
void DatabaseManagerBase::innerBatchWriteItem(
const std::string &tableName,
std::vector<Aws::DynamoDB::Model::WriteRequest> &writeRequests) {
// Split write requests to chunks by MAX_DYNAMODB_BATCH_ITEMS size and write
// them by batch
Aws::DynamoDB::Model::BatchWriteItemOutcome outcome;
std::vector<Aws::DynamoDB::Model::WriteRequest> writeRequestsChunk;
std::vector<Aws::DynamoDB::Model::WriteRequest>::iterator chunkPositionStart,
chunkPositionEnd;
for (size_t i = 0; i < writeRequests.size(); i += MAX_DYNAMODB_BATCH_ITEMS) {
chunkPositionStart = writeRequests.begin() + i;
chunkPositionEnd = writeRequests.begin() +
std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS);
writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
chunkPositionStart, chunkPositionEnd);
tomekUnsubmitted
Done
Inline Actions
Aws::DynamoDB::Model::BatchWriteItemOutcome outcome;
- std::vector<Aws::DynamoDB::Model::WriteRequest> writeRequestsChunk;
- std::vector<Aws::DynamoDB::Model::WriteRequest>::iterator chunkPositionStart,
- chunkPositionEnd;
for (size_t i = 0; i < writeRequests.size(); i += MAX_DYNAMODB_BATCH_ITEMS) {
- chunkPositionStart = writeRequests.begin() + i;
- chunkPositionEnd = writeRequests.begin() +
+ auto chunkPositionStart = writeRequests.begin() + i;
+ auto chunkPositionEnd = writeRequests.begin() +
std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS);
- writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
+ auto writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
chunkPositionStart, chunkPositionEnd);
Aws::DynamoDB::Model::BatchWriteItemRequest writeBatchRequest;

You can simplify the code by avoiding reusing the variables and using auto

tomek: You can simplify the code by avoiding reusing the variables and using `auto`
maxAuthorUnsubmitted
Done
Inline Actions

You can simplify the code by avoiding reusing the variables and using auto

I'm not a big fan of using auto where the type can be declared explicitly. Also, reusing the same variables is a part of loop optimization here (we don't create new ones inside the loop and reuse existing allocated memory for it).

max: > You can simplify the code by avoiding reusing the variables and using `auto` I'm not a big…
tomekUnsubmitted
Not Done
Inline Actions

Also, reusing the same variables is a part of loop optimization here

Have you measured the performance difference? I don't think this type of premature optimization is beneficial - if the variable is used only locally, it is easier to reason about it: you don't have to think about its initial value and about possible usages after the loop. Also, having a variable defined inside a loop might enable some optimization (I'm guessing now). Currently, we only reuse the name - new variables are constructed in every iteration.

So overall, we have a lot more important performance issues and saving a couple of machine instructions wouldn't be noticeable. On the other hand, from readability point of view, this is a noticeable difference - you are certain that the variable is used only where it is valid.

tomek: > Also, reusing the same variables is a part of loop optimization here Have you measured the…
Aws::DynamoDB::Model::BatchWriteItemRequest writeBatchRequest;
writeBatchRequest.AddRequestItems(tableName, writeRequestsChunk);
outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage());
}
tomekUnsubmitted
Done
Inline Actions
writeBatchRequest.AddRequestItems(tableName, writeRequestsChunk);
- outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
+ auto outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage());
}
}
}

outcome isn't used outside of this loop so we can declare it here

tomek: `outcome` isn't used outside of this loop so we can declare it here
maxAuthorUnsubmitted
Done
Inline Actions

outcome isn't used outside of this loop so we can declare it here

Same as above. I'm sure we should not use auto, where the type can explicitly be used, and allocating and reusing this variable here, is an optimized way from my perspective.

max: > `outcome` isn't used outside of this loop so we can declare it here Same as above. I'm sure…
}
}
} // namespace database } // namespace database
} // namespace network } // namespace network
} // namespace comm } // namespace comm